diff mbox series

[RFC,v3,5/9] dt-bindings: board: Document board-ids for Qualcomm devices

Message ID 20240521-board-ids-v3-5-e6c71d05f4d2@quicinc.com (mailing list archive)
State New, archived
Headers show
Series dt-bindings: hwinfo: Introduce board-id | expand

Commit Message

Elliot Berman May 21, 2024, 6:38 p.m. UTC
Document board identifiers for devices from Qualcomm Technologies, Inc.
These platforms are described with two mechanisms: the hardware SoC
registers and the "CDT" which is in a RO storage.

The hardware SoC registers describe both the SoC (e.g. SM8650, SC7180)
as well as revision. Add qcom,soc to describe only the SoC itself and
qcom,soc-version when the devicetree only works with a certain revision.

The CDT describes all other information about the board/platform.
Besides the platform type (e.g. MTP, ADP, CRD), there are 3 further
levels of versioning as well as additional fields to describe the PMIC
and boot storage device attached. The 3 levels of versioning are a
subtype, major, and minor version of the platform. Support describing
just the platform type (qcom,platform), the platform type and subtype
(qcom,platform-type), and all 4 numbers (qcom,platform-version).

Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>
---
 .../devicetree/bindings/board/qcom,board-id.yaml   | 144 +++++++++++++++++++++
 1 file changed, 144 insertions(+)

Comments

Rob Herring May 21, 2024, 7:19 p.m. UTC | #1
On Tue, 21 May 2024 11:38:02 -0700, Elliot Berman wrote:
> Document board identifiers for devices from Qualcomm Technologies, Inc.
> These platforms are described with two mechanisms: the hardware SoC
> registers and the "CDT" which is in a RO storage.
> 
> The hardware SoC registers describe both the SoC (e.g. SM8650, SC7180)
> as well as revision. Add qcom,soc to describe only the SoC itself and
> qcom,soc-version when the devicetree only works with a certain revision.
> 
> The CDT describes all other information about the board/platform.
> Besides the platform type (e.g. MTP, ADP, CRD), there are 3 further
> levels of versioning as well as additional fields to describe the PMIC
> and boot storage device attached. The 3 levels of versioning are a
> subtype, major, and minor version of the platform. Support describing
> just the platform type (qcom,platform), the platform type and subtype
> (qcom,platform-type), and all 4 numbers (qcom,platform-version).
> 
> Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>
> ---
>  .../devicetree/bindings/board/qcom,board-id.yaml   | 144 +++++++++++++++++++++
>  1 file changed, 144 insertions(+)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:
./Documentation/devicetree/bindings/board/qcom,board-id.yaml:15:12: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/board/qcom,board-id.yaml:74:8: [error] empty value in block mapping (empty-values)
./Documentation/devicetree/bindings/board/qcom,board-id.yaml:81:8: [error] empty value in block mapping (empty-values)
./Documentation/devicetree/bindings/board/qcom,board-id.yaml:88:8: [error] empty value in block mapping (empty-values)
./Documentation/devicetree/bindings/board/qcom,board-id.yaml:97:8: [error] empty value in block mapping (empty-values)
./Documentation/devicetree/bindings/board/qcom,board-id.yaml:103:8: [error] empty value in block mapping (empty-values)

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/board/qcom,board-id.yaml: allOf:2:if: None is not of type 'object', 'boolean'
	from schema $id: http://json-schema.org/draft-07/schema#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/board/qcom,board-id.yaml: allOf:3:if: None is not of type 'object', 'boolean'
	from schema $id: http://json-schema.org/draft-07/schema#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/board/qcom,board-id.yaml: allOf:4:if: None is not of type 'object', 'boolean'
	from schema $id: http://json-schema.org/draft-07/schema#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/board/qcom,board-id.yaml: allOf:5:if: None is not of type 'object', 'boolean'
	from schema $id: http://json-schema.org/draft-07/schema#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/board/qcom,board-id.yaml: allOf:6:if: None is not of type 'object', 'boolean'
	from schema $id: http://json-schema.org/draft-07/schema#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/board/qcom,board-id.yaml: $id: 'http://devicetree.org/schemas/board/qcom,board-id.yaml' does not match 'http://devicetree.org/schemas/.*\\.yaml#'
	from schema $id: http://devicetree.org/meta-schemas/base.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/board/qcom,board-id.yaml: $schema: 'http://devicetree.org/meta-schemas/core.yaml' is not one of ['http://devicetree.org/meta-schemas/core.yaml#', 'http://devicetree.org/meta-schemas/base.yaml#']
	from schema $id: http://devicetree.org/meta-schemas/base.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/board/qcom,board-id.yaml: ignoring, error in schema: allOf: 2: if
Documentation/devicetree/bindings/board/qcom,board-id.example.dts:26:56: error: macro "QCOM_BOARD_ID" passed 4 arguments, but takes just 3
   26 |     qcom,platform-version = <QCOM_BOARD_ID(MTP, 0, 1, 0)>,
      |                                                        ^
In file included from Documentation/devicetree/bindings/board/qcom,board-id.example.dts:4:
./scripts/dtc/include-prefixes/dt-bindings/arm/qcom,ids.h:279: note: macro "QCOM_BOARD_ID" defined here
  279 | #define QCOM_BOARD_ID(a, major, minor) \
      | 
Documentation/devicetree/bindings/board/qcom,board-id.example.dts:27:56: error: macro "QCOM_BOARD_ID" passed 4 arguments, but takes just 3
   27 |                             <QCOM_BOARD_ID(MTP, 0, 1, 1)>;
      |                                                        ^
./scripts/dtc/include-prefixes/dt-bindings/arm/qcom,ids.h:279: note: macro "QCOM_BOARD_ID" defined here
  279 | #define QCOM_BOARD_ID(a, major, minor) \
      | 
make[2]: *** [scripts/Makefile.lib:427: Documentation/devicetree/bindings/board/qcom,board-id.example.dtb] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1430: dt_binding_check] Error 2
make: *** [Makefile:240: __sub-make] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240521-board-ids-v3-5-e6c71d05f4d2@quicinc.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
Conor Dooley May 25, 2024, 5:08 p.m. UTC | #2
On Tue, May 21, 2024 at 11:38:02AM -0700, Elliot Berman wrote:
> Document board identifiers for devices from Qualcomm Technologies, Inc.
> These platforms are described with two mechanisms: the hardware SoC
> registers and the "CDT" which is in a RO storage.
> 
> The hardware SoC registers describe both the SoC (e.g. SM8650, SC7180)
> as well as revision. Add qcom,soc to describe only the SoC itself and
> qcom,soc-version when the devicetree only works with a certain revision.
> 
> The CDT describes all other information about the board/platform.
> Besides the platform type (e.g. MTP, ADP, CRD), there are 3 further
> levels of versioning as well as additional fields to describe the PMIC
> and boot storage device attached. The 3 levels of versioning are a
> subtype, major, and minor version of the platform. Support describing
> just the platform type (qcom,platform), the platform type and subtype
> (qcom,platform-type), and all 4 numbers (qcom,platform-version).
> 
> Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>
> ---
>  .../devicetree/bindings/board/qcom,board-id.yaml   | 144 +++++++++++++++++++++
>  1 file changed, 144 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/board/qcom,board-id.yaml b/Documentation/devicetree/bindings/board/qcom,board-id.yaml
> new file mode 100644
> index 000000000000..53ba7acab4c3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/board/qcom,board-id.yaml
> @@ -0,0 +1,144 @@
> +# SPDX-License-Identifier: BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/board/qcom,board-id.yaml
> +$schema: http://devicetree.org/meta-schemas/core.yaml
> +
> +title: Board identifiers for devices from Qualcomm Technologies, Inc.
> +description: Board identifiers for devices from Qualcomm Technologies, Inc.
> +
> +maintainers:
> +  - Elliot Berman <quic_eberman@quicinc.com>
> +
> +properties:
> +  $nodename:
> +    const: 'board-id'
> +
> +  qcom,soc:
> +    description:
> +      List of Qualcomm SoCs this devicetree is applicable to.
> +
> +  qcom,soc-version:
> +    items:
> +      items:
> +        - description: Qualcomm SoC identifier
> +        - description: SoC version
> +
> +  qcom,platform:
> +    description:
> +      List of Qualcomm platforms this devicetree is applicable to.
> +
> +  qcom,platform-type:
> +    items:
> +      items:
> +        - description: Qualcomm platform type identifier
> +        - description: Qualcomm platform subtype
> +
> +  qcom,platform-version:
> +    items:
> +      items:
> +        - description: Qualcomm platform type identifier
> +        - description: Qualcomm platform subtype
> +        - description: Qualcomm platform major and minor version.
> +
> +  qcom,boot-device:
> +    description:
> +      Boot device type
> +
> +  qcom,pmic:
> +    description:
> +      List of Qualcomm PMIC attaches
> +
> +  qcom,pmic-id:
> +    items:
> +      items:
> +        - description: Qualcomm PMIC identifier
> +        - description: Qualcomm PMIC revision
> +
> +allOf:
> +  # either describe soc or soc-version; it's confusing to have both

Why not just use the one that has the most information and discard the
others? If your dtb picker for this platform doesn't care about the soc
version, then just don't look at that cell?

Likewise for platform and PMIC, why can't you ignore the cells you don't
care about, rather than having a new property for each variant? Nothing
in this patch explains why multiple variants are required rather than
just dealing with the most informational.

Thanks,
Conor.

> +  - if:
> +      properties:
> +        qcom,soc: true
> +    then:
> +      properties:
> +        qcom,soc-version: false
> +  - if:
> +      properties:
> +        qcom,soc-version: true
> +    then:
> +      properties:
> +        qcom,soc: false
> +
> +  # describe one of platform, platform-type, or platform-version; it's confusing to have multiple
> +  - if:
> +    properties:
> +      qcom,platform: true
> +    then:
> +      properties:
> +        qcom,platform-type: false
> +        qcom,platform-version: false
> +  - if:
> +    properties:
> +      qcom,platform-type: true
> +    then:
> +      properties:
> +        qcom,platform: false
> +        qcom,platform-version: false
> +  - if:
> +    properties:
> +      qcom,platform: true
> +    then:
> +      properties:
> +        qcom,platform: false
> +        qcom,platform-type: false
> +
> +  # either describe pmic or pmic-id; it's confusing to have both
> +  - if:
> +    properties:
> +      qcom,pmic: true
> +    then:
> +      properties:
> +        qcom,pmic-id: false
> +  - if:
> +    properties:
> +      qcom,pmic-id: true
> +    then:
> +      properties:
> +        qcom,pmic: false
> +
> +additionalProperties: true
> +
> +examples:
> +  - |
> +    #include <dt-bindings/arm/qcom,ids.h>
> +    / {
> +      compatible = "qcom,sm8650";
> +      board-id {
> +        qcom,soc = <QCOM_ID_SM8650>;
> +        qcom,platform = <QCOM_BOARD_ID_MTP>;
> +      };
> +    };
> +
> +  - |
> +    #include <dt-bindings/arm/qcom,ids.h>
> +    / {
> +      compatible = "qcom,sm8650";
> +      board-id {
> +        qcom,soc-version = <QCOM_ID_SM8650 QCOM_SOC_REVISION(1)>,
> +                           <QCOM_ID_SM8650 QCOM_SOC_REVISION(2)>;
> +        qcom,platform-type = <QCOM_BOARD_ID_MTP 0>, <QCOM_BOARD_ID_MTP 1>;
> +      };
> +    };
> +
> +  - |
> +    #include <dt-bindings/arm/qcom,ids.h>
> +    / {
> +      compatible = "qcom,sm8650";
> +      board-id {
> +        qcom,soc = <QCOM_ID_SM8650>;
> +        qcom,platform-version = <QCOM_BOARD_ID(MTP, 0, 1, 0)>,
> +                                <QCOM_BOARD_ID(MTP, 0, 1, 1)>;
> +        qcom,boot-device = <QCOM_BOARD_BOOT_UFS>;
> +      };
> +    };
> 
> -- 
> 2.34.1
>
Elliot Berman May 29, 2024, 3:09 p.m. UTC | #3
On Sat, May 25, 2024 at 06:08:46PM +0100, Conor Dooley wrote:
> On Tue, May 21, 2024 at 11:38:02AM -0700, Elliot Berman wrote:
> > +
> > +allOf:
> > +  # either describe soc or soc-version; it's confusing to have both
> 
> Why not just use the one that has the most information and discard the
> others? If your dtb picker for this platform doesn't care about the soc
> version, then just don't look at that cell?

The dtb picker for the platform doesn't know whether to care about the
SoC version/platform version/whatever. That's a property of the DTB
itself and I don't think it makes much sense to bake that into the DTB
picker which would otherwise be unaware of this.

> 
> Likewise for platform and PMIC, why can't you ignore the cells you don't
> care about, rather than having a new property for each variant? Nothing
> in this patch explains why multiple variants are required rather than
> just dealing with the most informational.
>

Sure, I will explain in future revision.

- Elliot
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/board/qcom,board-id.yaml b/Documentation/devicetree/bindings/board/qcom,board-id.yaml
new file mode 100644
index 000000000000..53ba7acab4c3
--- /dev/null
+++ b/Documentation/devicetree/bindings/board/qcom,board-id.yaml
@@ -0,0 +1,144 @@ 
+# SPDX-License-Identifier: BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/board/qcom,board-id.yaml
+$schema: http://devicetree.org/meta-schemas/core.yaml
+
+title: Board identifiers for devices from Qualcomm Technologies, Inc.
+description: Board identifiers for devices from Qualcomm Technologies, Inc.
+
+maintainers:
+  - Elliot Berman <quic_eberman@quicinc.com>
+
+properties:
+  $nodename:
+    const: 'board-id'
+
+  qcom,soc:
+    description:
+      List of Qualcomm SoCs this devicetree is applicable to.
+
+  qcom,soc-version:
+    items:
+      items:
+        - description: Qualcomm SoC identifier
+        - description: SoC version
+
+  qcom,platform:
+    description:
+      List of Qualcomm platforms this devicetree is applicable to.
+
+  qcom,platform-type:
+    items:
+      items:
+        - description: Qualcomm platform type identifier
+        - description: Qualcomm platform subtype
+
+  qcom,platform-version:
+    items:
+      items:
+        - description: Qualcomm platform type identifier
+        - description: Qualcomm platform subtype
+        - description: Qualcomm platform major and minor version.
+
+  qcom,boot-device:
+    description:
+      Boot device type
+
+  qcom,pmic:
+    description:
+      List of Qualcomm PMIC attaches
+
+  qcom,pmic-id:
+    items:
+      items:
+        - description: Qualcomm PMIC identifier
+        - description: Qualcomm PMIC revision
+
+allOf:
+  # either describe soc or soc-version; it's confusing to have both
+  - if:
+      properties:
+        qcom,soc: true
+    then:
+      properties:
+        qcom,soc-version: false
+  - if:
+      properties:
+        qcom,soc-version: true
+    then:
+      properties:
+        qcom,soc: false
+
+  # describe one of platform, platform-type, or platform-version; it's confusing to have multiple
+  - if:
+    properties:
+      qcom,platform: true
+    then:
+      properties:
+        qcom,platform-type: false
+        qcom,platform-version: false
+  - if:
+    properties:
+      qcom,platform-type: true
+    then:
+      properties:
+        qcom,platform: false
+        qcom,platform-version: false
+  - if:
+    properties:
+      qcom,platform: true
+    then:
+      properties:
+        qcom,platform: false
+        qcom,platform-type: false
+
+  # either describe pmic or pmic-id; it's confusing to have both
+  - if:
+    properties:
+      qcom,pmic: true
+    then:
+      properties:
+        qcom,pmic-id: false
+  - if:
+    properties:
+      qcom,pmic-id: true
+    then:
+      properties:
+        qcom,pmic: false
+
+additionalProperties: true
+
+examples:
+  - |
+    #include <dt-bindings/arm/qcom,ids.h>
+    / {
+      compatible = "qcom,sm8650";
+      board-id {
+        qcom,soc = <QCOM_ID_SM8650>;
+        qcom,platform = <QCOM_BOARD_ID_MTP>;
+      };
+    };
+
+  - |
+    #include <dt-bindings/arm/qcom,ids.h>
+    / {
+      compatible = "qcom,sm8650";
+      board-id {
+        qcom,soc-version = <QCOM_ID_SM8650 QCOM_SOC_REVISION(1)>,
+                           <QCOM_ID_SM8650 QCOM_SOC_REVISION(2)>;
+        qcom,platform-type = <QCOM_BOARD_ID_MTP 0>, <QCOM_BOARD_ID_MTP 1>;
+      };
+    };
+
+  - |
+    #include <dt-bindings/arm/qcom,ids.h>
+    / {
+      compatible = "qcom,sm8650";
+      board-id {
+        qcom,soc = <QCOM_ID_SM8650>;
+        qcom,platform-version = <QCOM_BOARD_ID(MTP, 0, 1, 0)>,
+                                <QCOM_BOARD_ID(MTP, 0, 1, 1)>;
+        qcom,boot-device = <QCOM_BOARD_BOOT_UFS>;
+      };
+    };