mbox series

[RESEND,v4,0/4] platform: surface: Introduce Surface XBL Driver

Message ID 20211221182826.2141789-1-jaschultzMS@gmail.com
Headers show
Series platform: surface: Introduce Surface XBL Driver | expand

Message

Jarrett Schultz Dec. 21, 2021, 6:28 p.m. UTC
From: Jarrett Schultz <jaschultz@microsoft.com>

Introduce the Surface Extensible Boot Loader driver for the Surface Duo.
Exposes information about the driver to user space via sysfs for
consumption in manufacturing mode.

Resend due to not showing in mailing list.

Signed-off-by: Jarrett Schultz <jaschultz@microsoft.com>

---

Changes in v4:

 - Small binding definition changes
 - Removed ACPI propagation from patch series since it has been
   cherry-picked
 - Fixed the Signed-off-by: and From: mismatch

---

Changes in v3:
 - For the yaml documentation:
    * Updated description
    * Fixed examples
    * Updated 'required' field
 - Further propogated ACPI dependency in Kconfigs
 - Updated sysfs several binding descriptions
 - Renamed files to conform to naming conventions

---

Changes in v2:
 - Per Maximilian, added patch 2: propagated ACPI dependency from the
   directory as a whole to each individual driver
 - For the yaml documentation:
    * Removed json-schema dependence
    * Elaborated on description of driver
    * Updated example
 - Changed target KernelVersion in sysfs documentation
 - Updated MAINTAINER changes to be properly applied across patches
 - For the driver itself,
    * Added types.h inclusion and removed unused inclusions
    * Minor updates to code and acronym style
    * Remove __packed attribute on driver struct
    * Use .dev_groups for sysfs
 - Added more in-depth description of driver in Kconfig
 - Modified dts to reference a newly added section in sm8150.dtsi

---

Jarrett Schultz (4):
  dt-bindings: platform: microsoft: Document surface xbl
  platform: surface: Add surface xbl
  arm64: dts: qcom: sm8150: Add imem section
  arm64: dts: qcom: surface-duo: Add surface xbl

 .../ABI/testing/sysfs-platform-surface-xbl    |  79 ++++++++
 .../platform/microsoft/surface-xbl.yaml       |  64 ++++++
 MAINTAINERS                                   |   9 +
 .../dts/qcom/sm8150-microsoft-surface-duo.dts |  10 +
 arch/arm64/boot/dts/qcom/sm8150.dtsi          |   8 +
 drivers/platform/surface/Kconfig              |  12 ++
 drivers/platform/surface/Makefile             |   1 +
 drivers/platform/surface/surface_xbl.c        | 186 ++++++++++++++++++
 8 files changed, 369 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-platform-surface-xbl
 create mode 100644 Documentation/devicetree/bindings/platform/microsoft/surface-xbl.yaml
 create mode 100644 drivers/platform/surface/surface_xbl.c

Comments

Jarrett Schultz Dec. 27, 2021, 2:54 p.m. UTC | #1
> -----Original Message-----
> From: Rob Herring <robh@kernel.org>
> Sent: Tuesday, December 21, 2021 5:24 PM
> To: Jarrett Schultz <jaschultzms@gmail.com>
> Cc: Andy Gross <agross@kernel.org>; bjorn.andersson@linaro.org; Hans de
> Goede <hdegoede@redhat.com>; Mark Gross <markgross@kernel.org>;
> Maximilian Luz <luzmaximilian@gmail.com>; Felipe Balbi
> <balbi@kernel.org>; linux-arm-msm@vger.kernel.org; platform-driver-
> x86@vger.kernel.org; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org; Jarrett Schultz <jaschultz@microsoft.com>
> Subject: [EXTERNAL] Re: [PATCH RESEND v4 1/4] dt-bindings: platform:
> microsoft: Document surface xbl
> 
> On Tue, Dec 21, 2021 at 10:28:23AM -0800, Jarrett Schultz wrote:
> > From: Jarrett Schultz <jaschultz@microsoft.com>
> >
> > Introduce yaml for surface xbl driver.
> >
> > Signed-off-by: Jarrett Schultz <jaschultz@microsoft.com>
> >
> > ---
> >
> > Changes in v4:
> >  - Addressed small formatting changes
> >  - Removed unnecessary lines
> >
> > ---
> >
> > Changes in v3:
> >  - Updated description to only pertain to the hardware
> >  - Updated the required field to properly reflect the binding
> >  - Removed the first example
> >  - Fixed the size of the reg field in the second example
> >
> > ---
> >
> > Changes in v2:
> >  - Removed json-schema dependence
> >  - Elaborated on description of driver
> >  - Updated example
> > ---
> >  .../platform/microsoft/surface-xbl.yaml       | 64 +++++++++++++++++++
> >  MAINTAINERS                                   |  7 ++
> >  2 files changed, 71 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/platform/microsoft/surface-xbl.yaml
> >
> > diff --git
> > a/Documentation/devicetree/bindings/platform/microsoft/surface-
> xbl.yam
> > l
> > b/Documentation/devicetree/bindings/platform/microsoft/surface-
> xbl.yam
> > l
> > new file mode 100644
> > index 000000000000..df5a87a016f4
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/platform/microsoft/surface-xbl
> > +++ .yaml
> > @@ -0,0 +1,64 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2
> > +---
> > +$id:
> >
> +https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi
> > +cetree.org%2Fschemas%2Fplatform%2Fmicrosoft%2Fsurface-
> xbl.yaml%23&amp
> >
> +;data=04%7C01%7Cjaschultz%40microsoft.com%7C77abe729b69a40624472
> 08d9c
> >
> +4d8f44e%7C72f988bf86f141af91ab2d7cd011db47%7C0%7C0%7C637757258
> 3887312
> >
> +05%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luM
> zIiLCJBTi
> >
> +I6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=jG0qubz122GG2s8%2B
> 1ZuMCcPIa
> > +33KWwPoO2VNlDySmvs%3D&amp;reserved=0
> > +$schema:
> >
> +https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi
> > +cetree.org%2Fmeta-
> schemas%2Fcore.yaml%23&amp;data=04%7C01%7Cjaschultz
> >
> +%40microsoft.com%7C77abe729b69a4062447208d9c4d8f44e%7C72f988bf8
> 6f141a
> >
> +f91ab2d7cd011db47%7C0%7C0%7C637757258388731205%7CUnknown%7C
> TWFpbGZsb3
> >
> +d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0
> %3D%
> >
> +7C3000&amp;sdata=FxKNLPok9eW%2FBPmuGzU6x7dPwpnev0Ngr8epO4NA
> UzA%3D&amp
> > +;reserved=0
> > +
> > +title: Surface Extensible Bootloader for Microsoft Surface Duo
> > +
> > +maintainers:
> > +  - Jarrett Schultz <jaschultz@microsoft.com>
> > +
> > +description: |
> > +  Defined to expose information that is used during production when
> > +  device is in manufacturing mode. Some of the information included
> > +  in this imem section is -
> 
> If this is onchip sram, we have a binding for that. That's not an MFD.
> 
> > +    * board_id
> > +    * battery_present
> > +    * hw_init_retries
> > +    * is_customer_mode
> > +    * is_act_mode
> > +    * pmic_reset_reason
> > +    * touch_fw_version
> > +    * ocp_error_location
> 
> nvmem binding doesn't work for describing these fields?
> 
> > +  See sysfs documentation for more information.
> 
> sysfs? Not relevant to the binding.
> 
> > +
> > +properties:
> > +  compatible:
> > +    const: simple-mfd
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +additionalProperties: false
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - ranges
> > +  - address-cells
> > +  - size-cells
> > +
> > +examples:
> > +  - |
> > +    / {
> > +        compatible = "foo";
> > +        model = "foo";
> 
> No need to make this the root node with a fake compatible.

I included this root node in order to make #address-cells = #size-cells = 2,
since this example raises errors otherwise.

> 
> > +        #address-cells = <2>;
> > +        #size-cells = <2>;
> > +
> > +        imem@146bf000 {
> > +          compatible = "simple-mfd";
> > +          reg = <0x0 0x146bf000 0x0 0x1000>;
> > +          ranges = <0x0 0x0 0x146bf000 0x1000>;
> > +          #address-cells = <1>;
> > +          #size-cells = <1>;
> > +          status = "okay";
> 
> Don't show status in examples.
> 
> > +
> > +          xbl@a94 {
> > +            compatible = "microsoft,sm8150-surface-duo-xbl";
> > +            reg = <0xa94 0x100>;
> > +            status = "okay";
> > +          };
> > +        };
> > +      };
> > diff --git a/MAINTAINERS b/MAINTAINERS index
> > 13f9a84a617e..5d0ca2a98b57 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -12649,6 +12649,13 @@ F:	Documentation/driver-
> api/surface_aggregator/clients/dtx.rst
> >  F:	drivers/platform/surface/surface_dtx.c
> >  F:	include/uapi/linux/surface_aggregator/dtx.h
> >
> > +MICROSOFT SURFACE DUO XBL DRIVER
> > +M:	Jarrett Schultz <jaschultz@microsoft.com>
> > +L:	linux-arm-msm@vger.kernel.org
> > +L:	platform-driver-x86@vger.kernel.org
> > +S:	Supported
> > +F:	Documentation/devicetree/bindings/platform/microsoft/surface-
> xbl.yaml
> > +
> >  MICROSOFT SURFACE GPE LID SUPPORT DRIVER
> >  M:	Maximilian Luz <luzmaximilian@gmail.com>
> >  L:	platform-driver-x86@vger.kernel.org
> > --
> > 2.25.1
> >
> >
Rob Herring Dec. 27, 2021, 3:45 p.m. UTC | #2
On Mon, Dec 27, 2021 at 10:54 AM Jarrett Schultz
<jaschultz@microsoft.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Rob Herring <robh@kernel.org>
> > Sent: Tuesday, December 21, 2021 5:24 PM
> > To: Jarrett Schultz <jaschultzms@gmail.com>
> > Cc: Andy Gross <agross@kernel.org>; bjorn.andersson@linaro.org; Hans de
> > Goede <hdegoede@redhat.com>; Mark Gross <markgross@kernel.org>;
> > Maximilian Luz <luzmaximilian@gmail.com>; Felipe Balbi
> > <balbi@kernel.org>; linux-arm-msm@vger.kernel.org; platform-driver-
> > x86@vger.kernel.org; linux-kernel@vger.kernel.org;
> > devicetree@vger.kernel.org; Jarrett Schultz <jaschultz@microsoft.com>
> > Subject: [EXTERNAL] Re: [PATCH RESEND v4 1/4] dt-bindings: platform:
> > microsoft: Document surface xbl
> >
> > On Tue, Dec 21, 2021 at 10:28:23AM -0800, Jarrett Schultz wrote:
> > > From: Jarrett Schultz <jaschultz@microsoft.com>
> > >
> > > Introduce yaml for surface xbl driver.
> > >
> > > Signed-off-by: Jarrett Schultz <jaschultz@microsoft.com>
> > >
> > > ---
> > >
> > > Changes in v4:
> > >  - Addressed small formatting changes
> > >  - Removed unnecessary lines
> > >
> > > ---
> > >
> > > Changes in v3:
> > >  - Updated description to only pertain to the hardware
> > >  - Updated the required field to properly reflect the binding
> > >  - Removed the first example
> > >  - Fixed the size of the reg field in the second example
> > >
> > > ---
> > >
> > > Changes in v2:
> > >  - Removed json-schema dependence
> > >  - Elaborated on description of driver
> > >  - Updated example
> > > ---
> > >  .../platform/microsoft/surface-xbl.yaml       | 64 +++++++++++++++++++
> > >  MAINTAINERS                                   |  7 ++
> > >  2 files changed, 71 insertions(+)
> > >  create mode 100644
> > > Documentation/devicetree/bindings/platform/microsoft/surface-xbl.yaml
> > >
> > > diff --git
> > > a/Documentation/devicetree/bindings/platform/microsoft/surface-
> > xbl.yam
> > > l
> > > b/Documentation/devicetree/bindings/platform/microsoft/surface-
> > xbl.yam
> > > l
> > > new file mode 100644
> > > index 000000000000..df5a87a016f4
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/platform/microsoft/surface-xbl
> > > +++ .yaml
> > > @@ -0,0 +1,64 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2
> > > +---
> > > +$id:
> > >
> > +https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi
> > > +cetree.org%2Fschemas%2Fplatform%2Fmicrosoft%2Fsurface-
> > xbl.yaml%23&amp
> > >
> > +;data=04%7C01%7Cjaschultz%40microsoft.com%7C77abe729b69a40624472
> > 08d9c
> > >
> > +4d8f44e%7C72f988bf86f141af91ab2d7cd011db47%7C0%7C0%7C637757258
> > 3887312
> > >
> > +05%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luM
> > zIiLCJBTi
> > >
> > +I6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=jG0qubz122GG2s8%2B
> > 1ZuMCcPIa
> > > +33KWwPoO2VNlDySmvs%3D&amp;reserved=0
> > > +$schema:
> > >
> > +https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi
> > > +cetree.org%2Fmeta-
> > schemas%2Fcore.yaml%23&amp;data=04%7C01%7Cjaschultz
> > >
> > +%40microsoft.com%7C77abe729b69a4062447208d9c4d8f44e%7C72f988bf8
> > 6f141a
> > >
> > +f91ab2d7cd011db47%7C0%7C0%7C637757258388731205%7CUnknown%7C
> > TWFpbGZsb3
> > >
> > +d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0
> > %3D%
> > >
> > +7C3000&amp;sdata=FxKNLPok9eW%2FBPmuGzU6x7dPwpnev0Ngr8epO4NA
> > UzA%3D&amp
> > > +;reserved=0
> > > +
> > > +title: Surface Extensible Bootloader for Microsoft Surface Duo
> > > +
> > > +maintainers:
> > > +  - Jarrett Schultz <jaschultz@microsoft.com>
> > > +
> > > +description: |
> > > +  Defined to expose information that is used during production when
> > > +  device is in manufacturing mode. Some of the information included
> > > +  in this imem section is -
> >
> > If this is onchip sram, we have a binding for that. That's not an MFD.
> >
> > > +    * board_id
> > > +    * battery_present
> > > +    * hw_init_retries
> > > +    * is_customer_mode
> > > +    * is_act_mode
> > > +    * pmic_reset_reason
> > > +    * touch_fw_version
> > > +    * ocp_error_location
> >
> > nvmem binding doesn't work for describing these fields?
> >
> > > +  See sysfs documentation for more information.
> >
> > sysfs? Not relevant to the binding.
> >
> > > +
> > > +properties:
> > > +  compatible:
> > > +    const: simple-mfd
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +additionalProperties: false
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +  - ranges
> > > +  - address-cells
> > > +  - size-cells
> > > +
> > > +examples:
> > > +  - |
> > > +    / {
> > > +        compatible = "foo";
> > > +        model = "foo";
> >
> > No need to make this the root node with a fake compatible.
>
> I included this root node in order to make #address-cells = #size-cells = 2,
> since this example raises errors otherwise.

Why does your example require 2 cells? You only have 4KB of address space.

Though you can do:

bus {
  #address-cells = <2>;
  #size-cells = <2>;
  ...
};

Rob
Jarrett Schultz Dec. 27, 2021, 4:16 p.m. UTC | #3
> -----Original Message-----
> From: Rob Herring <robh@kernel.org>
> Sent: Monday, December 27, 2021 9:46 AM
> To: Jarrett Schultz <jaschultz@microsoft.com>
> Cc: Jarrett Schultz <jaschultzms@gmail.com>; Andy Gross
> <agross@kernel.org>; bjorn.andersson@linaro.org; Hans de Goede
> <hdegoede@redhat.com>; Mark Gross <markgross@kernel.org>; Maximilian
> Luz <luzmaximilian@gmail.com>; Felipe Balbi <balbi@kernel.org>; linux-arm-
> msm@vger.kernel.org; platform-driver-x86@vger.kernel.org; linux-
> kernel@vger.kernel.org; devicetree@vger.kernel.org
> Subject: Re: [EXTERNAL] Re: [PATCH RESEND v4 1/4] dt-bindings: platform:
> microsoft: Document surface xbl
> 
> On Mon, Dec 27, 2021 at 10:54 AM Jarrett Schultz
> <jaschultz@microsoft.com> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Rob Herring <robh@kernel.org>
> > > Sent: Tuesday, December 21, 2021 5:24 PM
> > > To: Jarrett Schultz <jaschultzms@gmail.com>
> > > Cc: Andy Gross <agross@kernel.org>; bjorn.andersson@linaro.org; Hans
> > > de Goede <hdegoede@redhat.com>; Mark Gross
> <markgross@kernel.org>;
> > > Maximilian Luz <luzmaximilian@gmail.com>; Felipe Balbi
> > > <balbi@kernel.org>; linux-arm-msm@vger.kernel.org; platform-driver-
> > > x86@vger.kernel.org; linux-kernel@vger.kernel.org;
> > > devicetree@vger.kernel.org; Jarrett Schultz
> > > <jaschultz@microsoft.com>
> > > Subject: [EXTERNAL] Re: [PATCH RESEND v4 1/4] dt-bindings: platform:
> > > microsoft: Document surface xbl
> > >
> > > On Tue, Dec 21, 2021 at 10:28:23AM -0800, Jarrett Schultz wrote:
> > > > From: Jarrett Schultz <jaschultz@microsoft.com>
> > > >
> > > > Introduce yaml for surface xbl driver.
> > > >
> > > > Signed-off-by: Jarrett Schultz <jaschultz@microsoft.com>
> > > >
> > > > ---
> > > >
> > > > Changes in v4:
> > > >  - Addressed small formatting changes
> > > >  - Removed unnecessary lines
> > > >
> > > > ---
> > > >
> > > > Changes in v3:
> > > >  - Updated description to only pertain to the hardware
> > > >  - Updated the required field to properly reflect the binding
> > > >  - Removed the first example
> > > >  - Fixed the size of the reg field in the second example
> > > >
> > > > ---
> > > >
> > > > Changes in v2:
> > > >  - Removed json-schema dependence
> > > >  - Elaborated on description of driver
> > > >  - Updated example
> > > > ---
> > > >  .../platform/microsoft/surface-xbl.yaml       | 64 +++++++++++++++++++
> > > >  MAINTAINERS                                   |  7 ++
> > > >  2 files changed, 71 insertions(+)  create mode 100644
> > > > Documentation/devicetree/bindings/platform/microsoft/surface-xbl.y
> > > > aml
> > > >
> > > > diff --git
> > > > a/Documentation/devicetree/bindings/platform/microsoft/surface-
> > > xbl.yam
> > > > l
> > > > b/Documentation/devicetree/bindings/platform/microsoft/surface-
> > > xbl.yam
> > > > l
> > > > new file mode 100644
> > > > index 000000000000..df5a87a016f4
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/platform/microsoft/surface
> > > > +++ -xbl
> > > > +++ .yaml
> > > > @@ -0,0 +1,64 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML
> > > > +1.2
> > > > +---
> > > > +$id:
> > > >
> > >
> +https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fde
> > > +vi
> > > > +cetree.org%2Fschemas%2Fplatform%2Fmicrosoft%2Fsurface-
> > > xbl.yaml%23&amp
> > > >
> > >
> +;data=04%7C01%7Cjaschultz%40microsoft.com%7C77abe729b69a40624472
> > > 08d9c
> > > >
> > >
> +4d8f44e%7C72f988bf86f141af91ab2d7cd011db47%7C0%7C0%7C637757258
> > > 3887312
> > > >
> > >
> +05%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luM
> > > zIiLCJBTi
> > > >
> > >
> +I6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=jG0qubz122GG2s8%2B
> > > 1ZuMCcPIa
> > > > +33KWwPoO2VNlDySmvs%3D&amp;reserved=0
> > > > +$schema:
> > > >
> > >
> +https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fde
> > > +vi
> > > > +cetree.org%2Fmeta-
> > > schemas%2Fcore.yaml%23&amp;data=04%7C01%7Cjaschultz
> > > >
> > >
> +%40microsoft.com%7C77abe729b69a4062447208d9c4d8f44e%7C72f988bf8
> > > 6f141a
> > > >
> > >
> +f91ab2d7cd011db47%7C0%7C0%7C637757258388731205%7CUnknown%7C
> > > TWFpbGZsb3
> > > >
> > >
> +d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0
> > > %3D%
> > > >
> > >
> +7C3000&amp;sdata=FxKNLPok9eW%2FBPmuGzU6x7dPwpnev0Ngr8epO4NA
> > > UzA%3D&amp
> > > > +;reserved=0
> > > > +
> > > > +title: Surface Extensible Bootloader for Microsoft Surface Duo
> > > > +
> > > > +maintainers:
> > > > +  - Jarrett Schultz <jaschultz@microsoft.com>
> > > > +
> > > > +description: |
> > > > +  Defined to expose information that is used during production
> > > > +when
> > > > +  device is in manufacturing mode. Some of the information
> > > > +included
> > > > +  in this imem section is -
> > >
> > > If this is onchip sram, we have a binding for that. That's not an MFD.
> > >
> > > > +    * board_id
> > > > +    * battery_present
> > > > +    * hw_init_retries
> > > > +    * is_customer_mode
> > > > +    * is_act_mode
> > > > +    * pmic_reset_reason
> > > > +    * touch_fw_version
> > > > +    * ocp_error_location
> > >
> > > nvmem binding doesn't work for describing these fields?
> > >
> > > > +  See sysfs documentation for more information.
> > >
> > > sysfs? Not relevant to the binding.
> > >
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    const: simple-mfd
> > > > +
> > > > +  reg:
> > > > +    maxItems: 1
> > > > +
> > > > +additionalProperties: false
> > > > +
> > > > +required:
> > > > +  - compatible
> > > > +  - reg
> > > > +  - ranges
> > > > +  - address-cells
> > > > +  - size-cells
> > > > +
> > > > +examples:
> > > > +  - |
> > > > +    / {
> > > > +        compatible = "foo";
> > > > +        model = "foo";
> > >
> > > No need to make this the root node with a fake compatible.
> >
> > I included this root node in order to make #address-cells =
> > #size-cells = 2, since this example raises errors otherwise.
> 
> Why does your example require 2 cells? You only have 4KB of address space.

The soc node that imem sits under in sm8150.dtsi defines it that way so for
continuity I figured that it would make sense for the example to mirror that.

Also, should imem and xbl be documented in separate files?

> 
> Though you can do:
> 
> bus {
>   #address-cells = <2>;
>   #size-cells = <2>;
>   ...
> };
> 
> Rob
Jarrett Schultz Feb. 3, 2022, 11:23 p.m. UTC | #4
Rob Herring wrote:
> From: Rob Herring <robh@kernel.org>
> 
> On Tue, Dec 21, 2021 at 10:28:23AM -0800, Jarrett Schultz wrote:
> > From: Jarrett Schultz <jaschultz@microsoft.com>
> >
> > +title: Surface Extensible Bootloader for Microsoft Surface Duo
> > +
> > +maintainers:
> > +  - Jarrett Schultz <jaschultz@microsoft.com>
> > +
> > +description: |
> > +  Defined to expose information that is used during production when
> > +  device is in manufacturing mode. Some of the information included
> > +  in this imem section is -
> 
> If this is onchip sram, we have a binding for that. That's not an MFD.

I now have this driver working with nvmem, but I could not find the binding
that you are talking about here. Could you point me to the binding?

Thanks,
Jarrett

> 
> > +    * board_id
> > +    * battery_present
> > +    * hw_init_retries
> > +    * is_customer_mode
> > +    * is_act_mode
> > +    * pmic_reset_reason
> > +    * touch_fw_version
> > +    * ocp_error_location
> 
> nvmem binding doesn't work for describing these fields?
> 
> > +  See sysfs documentation for more information.
> 
> sysfs? Not relevant to the binding.
> 
> > +
> > +properties:
> > +  compatible:
> > +    const: simple-mfd
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +additionalProperties: false
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - ranges
> > +  - address-cells
> > +  - size-cells
> > +
> > +examples:
> > +  - |
> > +    / {
> > +        compatible = "foo";
> > +        model = "foo";
> 
> No need to make this the root node with a fake compatible.
> 
> > +        #address-cells = <2>;
> > +        #size-cells = <2>;
> > +
> > +        imem@146bf000 {
> > +          compatible = "simple-mfd";
> > +          reg = <0x0 0x146bf000 0x0 0x1000>;
> > +          ranges = <0x0 0x0 0x146bf000 0x1000>;
> > +          #address-cells = <1>;
> > +          #size-cells = <1>;
> > +          status = "okay";
> 
> Don't show status in examples.
> 
> > +
> > +          xbl@a94 {
> > +            compatible = "microsoft,sm8150-surface-duo-xbl";
> > +            reg = <0xa94 0x100>;
> > +            status = "okay";
> > +          };
> > +        };
> > +      };
> > diff --git a/MAINTAINERS b/MAINTAINERS index
> > 13f9a84a617e..5d0ca2a98b57 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -12649,6 +12649,13 @@ F:	Documentation/driver-
> api/surface_aggregator/clients/dtx.rst
> >  F:	drivers/platform/surface/surface_dtx.c
> >  F:	include/uapi/linux/surface_aggregator/dtx.h
> >
> > +MICROSOFT SURFACE DUO XBL DRIVER
> > +M:	Jarrett Schultz <jaschultz@microsoft.com>
> > +L:	linux-arm-msm@vger.kernel.org
> > +L:	platform-driver-x86@vger.kernel.org
> > +S:	Supported
> > +F:	Documentation/devicetree/bindings/platform/microsoft/surface-
> xbl.yaml
> > +
> >  MICROSOFT SURFACE GPE LID SUPPORT DRIVER
> >  M:	Maximilian Luz <luzmaximilian@gmail.com>
> >  L:	platform-driver-x86@vger.kernel.org
> > --
> > 2.25.1
> >
> >
Jarrett Schultz Feb. 24, 2022, 11:42 p.m. UTC | #5
On Tuesday, February 15, 2022 8:39 AM, Bjorn Andersson
> 
> On Mon 14 Feb 19:04 CST 2022, Jarrett Schultz wrote:
> 
> > Jarrett Schultz wrote:
> > > From: Jarrett Schultz
> > >
> > > Rob Herring wrote:
> > > > From: Rob Herring <robh@kernel.org>
> > > >
> > > > On Tue, Dec 21, 2021 at 10:28:23AM -0800, Jarrett Schultz wrote:
> > > > > From: Jarrett Schultz <jaschultz@microsoft.com>
> > > > >
> > > > > +title: Surface Extensible Bootloader for Microsoft Surface Duo
> > > > > +
> > > > > +maintainers:
> > > > > +  - Jarrett Schultz <jaschultz@microsoft.com>
> > > > > +
> > > > > +description: |
> > > > > +  Defined to expose information that is used during production
> > > > > +when
> > > > > +  device is in manufacturing mode. Some of the information
> > > > > +included
> > > > > +  in this imem section is -
> > > >
> > > > If this is onchip sram, we have a binding for that. That's not an MFD.
> > >
> > > I now have this driver working with nvmem, but I could not find the
> > > binding that you are talking about here. Could you point me to the
> binding?
> > >
> > > Thanks,
> > > Jarrett
> > >
> >
> > Rob,
> >
> > I followed up with my team members who have let me know that this lies
> > in DDR rather than SRAM. Could you please point me to the correct
> > binding to use?
> >
> 
> It might be DDR, but it's on-chip and the memory region that you describe is
> a region within "imem" - something used for various purposes by Qualcomm,
> presumably also in your device. Unfortunately we haven't specified a binding
> for "imem", only some of its regions.
> 
> Perhaps it would be appropriate to express the entire imem as nvmem, in
> addition to the region-specific logic, if that suits you.
> 
> Regards,
> Bjorn

Bjorn et al.,

This xbl driver was meant to be something simple and the intent was to merge it into the tree with an existing structure. Your suggestion makes sense, and I believe that it could be implemented as a separate patch after this patch with the initial xbl driver has been accepted and merged.

Please let me know what you think,
Jarrett Schultz

> 
> > Thanks,
> > Jarrett
> >
> > > >
> > > > > +    * board_id
> > > > > +    * battery_present
> > > > > +    * hw_init_retries
> > > > > +    * is_customer_mode
> > > > > +    * is_act_mode
> > > > > +    * pmic_reset_reason
> > > > > +    * touch_fw_version
> > > > > +    * ocp_error_location
> > > >
> > > > nvmem binding doesn't work for describing these fields?
> > > >
> > > > > +  See sysfs documentation for more information.
> > > >
> > > > sysfs? Not relevant to the binding.
> > > >
> > > > > +
> > > > > +properties:
> > > > > +  compatible:
> > > > > +    const: simple-mfd
> > > > > +
> > > > > +  reg:
> > > > > +    maxItems: 1
> > > > > +
> > > > > +additionalProperties: false
> > > > > +
> > > > > +required:
> > > > > +  - compatible
> > > > > +  - reg
> > > > > +  - ranges
> > > > > +  - address-cells
> > > > > +  - size-cells
> > > > > +
> > > > > +examples:
> > > > > +  - |
> > > > > +    / {
> > > > > +        compatible = "foo";
> > > > > +        model = "foo";
> > > >
> > > > No need to make this the root node with a fake compatible.
> > > >
> > > > > +        #address-cells = <2>;
> > > > > +        #size-cells = <2>;
> > > > > +
> > > > > +        imem@146bf000 {
> > > > > +          compatible = "simple-mfd";
> > > > > +          reg = <0x0 0x146bf000 0x0 0x1000>;
> > > > > +          ranges = <0x0 0x0 0x146bf000 0x1000>;
> > > > > +          #address-cells = <1>;
> > > > > +          #size-cells = <1>;
> > > > > +          status = "okay";
> > > >
> > > > Don't show status in examples.
> > > >
> > > > > +
> > > > > +          xbl@a94 {
> > > > > +            compatible = "microsoft,sm8150-surface-duo-xbl";
> > > > > +            reg = <0xa94 0x100>;
> > > > > +            status = "okay";
> > > > > +          };
> > > > > +        };
> > > > > +      };
> > > > > diff --git a/MAINTAINERS b/MAINTAINERS index
> > > > > 13f9a84a617e..5d0ca2a98b57 100644
> > > > > --- a/MAINTAINERS
> > > > > +++ b/MAINTAINERS
> > > > > @@ -12649,6 +12649,13 @@ F:	Documentation/driver-
> > > > api/surface_aggregator/clients/dtx.rst
> > > > >  F:	drivers/platform/surface/surface_dtx.c
> > > > >  F:	include/uapi/linux/surface_aggregator/dtx.h
> > > > >
> > > > > +MICROSOFT SURFACE DUO XBL DRIVER
> > > > > +M:	Jarrett Schultz <jaschultz@microsoft.com>
> > > > > +L:	linux-arm-msm@vger.kernel.org
> > > > > +L:	platform-driver-x86@vger.kernel.org
> > > > > +S:	Supported
> > > > > +F:
> 	Documentation/devicetree/bindings/platform/microsoft/surface-
> > > > xbl.yaml
> > > > > +
> > > > >  MICROSOFT SURFACE GPE LID SUPPORT DRIVER
> > > > >  M:	Maximilian Luz <luzmaximilian@gmail.com>
> > > > >  L:	platform-driver-x86@vger.kernel.org
> > > > > --
> > > > > 2.25.1
> > > > >
> > > > >