diff mbox series

[v2,2/2] dt-bindings: qcom: Update DT bindings for multiple DT

Message ID 1710418312-6559-3-git-send-email-quic_amrianan@quicinc.com
State Changes Requested, archived
Headers show
Series Add board-id support for multiple DT selection | expand

Checks

Context Check Description
robh/checkpatch warning total: 0 errors, 1 warnings, 100 lines checked
robh/patch-applied success
robh/dt-meta-schema fail build log

Commit Message

Amrit Anand March 14, 2024, 12:11 p.m. UTC
Qualcomm produces a lot of "unique" boards with slight differences in
SoC's and board's configuration. For eg, there can be SM8150v1 on MTPv1,
SM8150v1 on MTPv2, SM8150v2 on MTPv2, SM8150v2 on MTPv2 with a different
PMIC, SM8150v2 with no modem support and so on. For instance, suppose we
have 3 SoC, each with 4 boards supported, along with 2 PMIC support for
each case which would lead to total of 24 DTB files. Along with these
configurations, OEMs may also add certain additional board variants. Thus
a mechanism is required to pick the correct DTB for the corresponding board.

Introduce mechanism to select required DTB using newly introduced device
tree properties "board-id" and "board-id-type". "board-id" will contain
the list of values of "qcom,soc-id", "qcom,board-id", "qcom,pmic-id" or
"qcom,oem-id". "board-id-types" contains the type of parameter which is
entered. It can be either "qcom,soc-id", "qcom,board-id", "qcom,pmic-id"
or "qcom,oem-id".

Qualcomm based bootloader will use these properties to pick the best
matched DTB to boot the device with.

Signed-off-by: Amrit Anand <quic_amrianan@quicinc.com>
---
 Documentation/devicetree/bindings/arm/qcom.yaml | 90 +++++++++++++++++++++++++
 1 file changed, 90 insertions(+)

Comments

Rob Herring (Arm) March 14, 2024, 1:35 p.m. UTC | #1
On Thu, 14 Mar 2024 17:41:52 +0530, Amrit Anand wrote:
> Qualcomm produces a lot of "unique" boards with slight differences in
> SoC's and board's configuration. For eg, there can be SM8150v1 on MTPv1,
> SM8150v1 on MTPv2, SM8150v2 on MTPv2, SM8150v2 on MTPv2 with a different
> PMIC, SM8150v2 with no modem support and so on. For instance, suppose we
> have 3 SoC, each with 4 boards supported, along with 2 PMIC support for
> each case which would lead to total of 24 DTB files. Along with these
> configurations, OEMs may also add certain additional board variants. Thus
> a mechanism is required to pick the correct DTB for the corresponding board.
> 
> Introduce mechanism to select required DTB using newly introduced device
> tree properties "board-id" and "board-id-type". "board-id" will contain
> the list of values of "qcom,soc-id", "qcom,board-id", "qcom,pmic-id" or
> "qcom,oem-id". "board-id-types" contains the type of parameter which is
> entered. It can be either "qcom,soc-id", "qcom,board-id", "qcom,pmic-id"
> or "qcom,oem-id".
> 
> Qualcomm based bootloader will use these properties to pick the best
> matched DTB to boot the device with.
> 
> Signed-off-by: Amrit Anand <quic_amrianan@quicinc.com>
> ---
>  Documentation/devicetree/bindings/arm/qcom.yaml | 90 +++++++++++++++++++++++++
>  1 file changed, 90 insertions(+)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:
./Documentation/devicetree/bindings/arm/qcom.yaml:1159:8: [warning] wrong indentation: expected 6 but found 7 (indentation)
./Documentation/devicetree/bindings/arm/qcom.yaml:1162:8: [warning] wrong indentation: expected 6 but found 7 (indentation)
./Documentation/devicetree/bindings/arm/qcom.yaml:1165:15: [warning] wrong indentation: expected 13 but found 14 (indentation)

dtschema/dtc warnings/errors:

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/1710418312-6559-3-git-send-email-quic_amrianan@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.
Caleb Connolly March 14, 2024, 2:20 p.m. UTC | #2
Hi Amrit,

On 14/03/2024 12:11, Amrit Anand wrote:
> Qualcomm produces a lot of "unique" boards with slight differences in
> SoC's and board's configuration. For eg, there can be SM8150v1 on MTPv1,
> SM8150v1 on MTPv2, SM8150v2 on MTPv2, SM8150v2 on MTPv2 with a different
> PMIC, SM8150v2 with no modem support and so on. For instance, suppose we
> have 3 SoC, each with 4 boards supported, along with 2 PMIC support for
> each case which would lead to total of 24 DTB files. Along with these
> configurations, OEMs may also add certain additional board variants. Thus
> a mechanism is required to pick the correct DTB for the corresponding board.
> 
> Introduce mechanism to select required DTB using newly introduced device
> tree properties "board-id" and "board-id-type". "board-id" will contain
> the list of values of "qcom,soc-id", "qcom,board-id", "qcom,pmic-id" or
> "qcom,oem-id". "board-id-types" contains the type of parameter which is
> entered. It can be either "qcom,soc-id", "qcom,board-id", "qcom,pmic-id"
> or "qcom,oem-id".

Thanks for working on this, it's nice to finally see this logic
documented in the kernel.
> 
> Qualcomm based bootloader will use these properties to pick the best
> matched DTB to boot the device with.
> 
> Signed-off-by: Amrit Anand <quic_amrianan@quicinc.com>
> ---
>  Documentation/devicetree/bindings/arm/qcom.yaml | 90 +++++++++++++++++++++++++
>  1 file changed, 90 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/arm/qcom.yaml b/Documentation/devicetree/bindings/arm/qcom.yaml
> index 7f80f48..dc66ae9 100644
> --- a/Documentation/devicetree/bindings/arm/qcom.yaml
> +++ b/Documentation/devicetree/bindings/arm/qcom.yaml
> @@ -1100,6 +1100,76 @@ properties:
>        kernel
>        The property is deprecated.
>  
> +  board-id:
> +    $ref: /schemas/types.yaml#/definitions/uint32-matrix
> +    minItems: 2
> +    description: |
> +      Qualcomm specific bootloader uses multiple different identifiers
> +      (qcom,soc-id, qcom,board-id, qcom,pmic-id, qcom,oem-id) to select
> +      single Devicetree among list of Devicetrees. For different identifiers,
> +      the selection can be done either based on exact match (where the
> +      identifiers information coming from firmware should exactly match
> +      the ones described in devicetree) or best match (firmware provided
> +      identifier information closely matches with the one of the Devicetree).
> +      Below table describes matching criteria for each identifier::
> +      |----------------------------------------------------------------------|
> +      |  DT property  |  Individual fields   |   Exact  |  Best  |  Default  |
> +      |----------------------------------------------------------------------|
> +      | qcom,soc-id   |                                                      |
> +      |               |  Chipset Id          |     Y    |    N   |     -     |
> +      |               |  SoC Revision        |     N    |    Y   |     -     |
> +      | qcom,board-id |                                                      |
> +      |               |  Board Id            |     Y    |    N   |     -     |
> +      |               |  Board Major         |     N    |    Y   |     -     |
> +      |               |  Board Minor         |     N    |    Y   |     -     |
> +      |               |  Subtype             |     Y    |    N   |     0     |
> +      |               |  DDRtype             |     Y    |    N   |     0     |
> +      |               |  BootDevice Type     |     Y    |    N   |     0     |
> +      | qcom,pmic-id  |                                                      |
> +      |               |  Slave Id            |     Y    |    N   |     0     |
> +      |               |  PMIC Id             |     Y    |    N   |     0     |
> +      |               |  PMIC Major          |     N    |    Y   |     0     |
> +      |               |  PMIC Minor          |     N    |    Y   |     0     |
> +      | qcom,oem-id   |                                                      |
> +      |               |  OEM Id              |     Y    |    N   |     0     |
> +      |----------------------------------------------------------------------|
> +      For best match, identifiers are matched based on following priority order::
> +      SoC Revision > Board Major > Board Minor > PMIC Major > PMIC Minor
> +
> +  board-id-types:
> +    $ref: /schemas/types.yaml#/definitions/non-unique-string-array
> +    description:
> +       Each field and helper macros are defined at include/dt-bindings/arm/qcom,ids.
> +    minItems: 2
> +    items:
> +       oneOf:
> +         - const: qcom,soc-id
> +           description:
> +              Matches Qualcomm Technologies, Inc. boards with the specified SoC.
> +              2 integers are needed to describe a soc-id. The first integer is the
> +              SoC ID and the second integer is the SoC revision.
> +              qcom,soc-id = <soc-id  soc-revision>
> +         - const: qcom,board-id
> +           description: |
> +              Matches Qualcomm Technologies, Inc. boards with the specified board.
> +              2 integers are needed to describe a board-id. The first integer is the
> +              board ID. The second integer is the board-subtype.
> +              qcom,board-id = <board-id  board-subtype>
> +         - const: qcom,pmic-id
> +           description: |
> +              Qualcomm boards can be attached to multiple PMICs where slave-id (SID)
> +              indicates the address of the bus on which the PMIC is attached. It can be
> +              any number. The model for a PMIC indicates the PMIC name attached to bus
> +              described by SID along with  major and minor version. 2 integers are needed
> +              to describe qcom,pmic-id. The first integer is the slave-id and the second integer
> +              is the pmic model.
> +              qcom,pmic-id = <pmic-sid pmic-model>
> +         - const: qcom,oem-id
> +           description: |
> +              Matches Qualcomm Technologies, Inc. boards with the specified OEM ID.
> +              1 integer is needed to describe the oem-id.
> +              qcom,oem-id = <oem-id>
> +
>  allOf:
>    # Explicit allow-list for older SoCs. The legacy properties are not allowed
>    # on newer SoCs.
> @@ -1167,4 +1237,24 @@ allOf:
>  
>  additionalProperties: true
>  
> +examples:
> +  - |
> +    #include <dt-bindings/arm/qcom,ids.h>
> +    / {
> +         model = "Qualcomm Technologies, Inc. sc7280 IDP SKU1 platform";
> +         compatible = "qcom,sc7280-idp", "google,senor", "qcom,sc7280";
> +
> +         #board-id-cells = <2>;
> +         board-id = <QCOM_SOC_ID(SC7280) QCOM_SOC_REVISION(1)>,
> +                    <QCOM_SOC_ID(SC7280) QCOM_SOC_REVISION(2)>,
> +                    <QCOM_BOARD_ID(IDP, 1, 0) QCOM_BOARD_SUBTYPE(UFS, ANY, 1)>;
> +         board-id-types = "qcom,soc-id",
> +                          "qcom,soc-id",
> +                          "qcom,board-id";
Forgive me if this is a particularly cynical view, but this seems
incredibly blatant, the "qcom,board-id" property is deprecated for
various good reasons, just using a key/value map where "qcom,board-id"
is a key doesn't change that. There are two main issues I have with the
proposal here:

1. This breaks backwards compatibility, millions of production devices
with bootloaders that will never receive another update might be
compatible with the downstream "qcom,board-id" property, but they won't
work with this.
2. A top level board-id property that isn't namespaced implies that it
isn't vendor specific, but the proposed implementation doesn't even
pretend to be vendor agnostic.

U-Boot also has some ideas around this issue, there you can pass in
multiple DTBs and provide some board specific "best match" function.
I think there's definitely some value in exposing this information, but
there's no good reason to define the same data as `qcom,board-id` while
breaking production bootloaders.
> +
> +         #address-cells = <2>;
> +         #size-cells = <2>;
> +    };
> +
> +
>  ...
Caleb Connolly March 14, 2024, 2:35 p.m. UTC | #3
On 14/03/2024 14:20, Caleb Connolly wrote:
> Hi Amrit,
> 
> On 14/03/2024 12:11, Amrit Anand wrote:
>> Qualcomm produces a lot of "unique" boards with slight differences in
>> SoC's and board's configuration. For eg, there can be SM8150v1 on MTPv1,
>> SM8150v1 on MTPv2, SM8150v2 on MTPv2, SM8150v2 on MTPv2 with a different
>> PMIC, SM8150v2 with no modem support and so on. For instance, suppose we
>> have 3 SoC, each with 4 boards supported, along with 2 PMIC support for
>> each case which would lead to total of 24 DTB files. Along with these
>> configurations, OEMs may also add certain additional board variants. Thus
>> a mechanism is required to pick the correct DTB for the corresponding board.
>>
>> Introduce mechanism to select required DTB using newly introduced device
>> tree properties "board-id" and "board-id-type". "board-id" will contain
>> the list of values of "qcom,soc-id", "qcom,board-id", "qcom,pmic-id" or
>> "qcom,oem-id". "board-id-types" contains the type of parameter which is
>> entered. It can be either "qcom,soc-id", "qcom,board-id", "qcom,pmic-id"
>> or "qcom,oem-id".
> 
> Thanks for working on this, it's nice to finally see this logic
> documented in the kernel.
>>
>> Qualcomm based bootloader will use these properties to pick the best
>> matched DTB to boot the device with.
>>
>> Signed-off-by: Amrit Anand <quic_amrianan@quicinc.com>
>> ---
>>  Documentation/devicetree/bindings/arm/qcom.yaml | 90 +++++++++++++++++++++++++
>>  1 file changed, 90 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/arm/qcom.yaml b/Documentation/devicetree/bindings/arm/qcom.yaml
>> index 7f80f48..dc66ae9 100644
>> --- a/Documentation/devicetree/bindings/arm/qcom.yaml
>> +++ b/Documentation/devicetree/bindings/arm/qcom.yaml
>> @@ -1100,6 +1100,76 @@ properties:
>>        kernel
>>        The property is deprecated.
>>  
>> +  board-id:
>> +    $ref: /schemas/types.yaml#/definitions/uint32-matrix
>> +    minItems: 2
>> +    description: |
>> +      Qualcomm specific bootloader uses multiple different identifiers
>> +      (qcom,soc-id, qcom,board-id, qcom,pmic-id, qcom,oem-id) to select
>> +      single Devicetree among list of Devicetrees. For different identifiers,
>> +      the selection can be done either based on exact match (where the
>> +      identifiers information coming from firmware should exactly match
>> +      the ones described in devicetree) or best match (firmware provided
>> +      identifier information closely matches with the one of the Devicetree).
>> +      Below table describes matching criteria for each identifier::
>> +      |----------------------------------------------------------------------|
>> +      |  DT property  |  Individual fields   |   Exact  |  Best  |  Default  |
>> +      |----------------------------------------------------------------------|
>> +      | qcom,soc-id   |                                                      |
>> +      |               |  Chipset Id          |     Y    |    N   |     -     |
>> +      |               |  SoC Revision        |     N    |    Y   |     -     |
>> +      | qcom,board-id |                                                      |
>> +      |               |  Board Id            |     Y    |    N   |     -     |
>> +      |               |  Board Major         |     N    |    Y   |     -     |
>> +      |               |  Board Minor         |     N    |    Y   |     -     |
>> +      |               |  Subtype             |     Y    |    N   |     0     |
>> +      |               |  DDRtype             |     Y    |    N   |     0     |
>> +      |               |  BootDevice Type     |     Y    |    N   |     0     |
>> +      | qcom,pmic-id  |                                                      |
>> +      |               |  Slave Id            |     Y    |    N   |     0     |
>> +      |               |  PMIC Id             |     Y    |    N   |     0     |
>> +      |               |  PMIC Major          |     N    |    Y   |     0     |
>> +      |               |  PMIC Minor          |     N    |    Y   |     0     |
>> +      | qcom,oem-id   |                                                      |
>> +      |               |  OEM Id              |     Y    |    N   |     0     |
>> +      |----------------------------------------------------------------------|
>> +      For best match, identifiers are matched based on following priority order::
>> +      SoC Revision > Board Major > Board Minor > PMIC Major > PMIC Minor
>> +
>> +  board-id-types:
>> +    $ref: /schemas/types.yaml#/definitions/non-unique-string-array
>> +    description:
>> +       Each field and helper macros are defined at include/dt-bindings/arm/qcom,ids.
>> +    minItems: 2
>> +    items:
>> +       oneOf:
>> +         - const: qcom,soc-id
>> +           description:
>> +              Matches Qualcomm Technologies, Inc. boards with the specified SoC.
>> +              2 integers are needed to describe a soc-id. The first integer is the
>> +              SoC ID and the second integer is the SoC revision.
>> +              qcom,soc-id = <soc-id  soc-revision>
>> +         - const: qcom,board-id
>> +           description: |
>> +              Matches Qualcomm Technologies, Inc. boards with the specified board.
>> +              2 integers are needed to describe a board-id. The first integer is the
>> +              board ID. The second integer is the board-subtype.
>> +              qcom,board-id = <board-id  board-subtype>
This is a recursive definition. You partially described the individual
fields above, you should do that here.

What is DDR type? What information is encoded that doesn't make sense to
describe elsewhere in DT?

Same for "bootdevice type", why would you pick a different DT based on
whether the bootloader was loaded from UFS or NAND? Why does this
information belong in DT?
>> +         - const: qcom,pmic-id
>> +           description: |
>> +              Qualcomm boards can be attached to multiple PMICs where slave-id (SID)
>> +              indicates the address of the bus on which the PMIC is attached. It can be
>> +              any number. The model for a PMIC indicates the PMIC name attached to bus
>> +              described by SID along with  major and minor version. 2 integers are needed
>> +              to describe qcom,pmic-id. The first integer is the slave-id and the second integer
>> +              is the pmic model.
>> +              qcom,pmic-id = <pmic-sid pmic-model>

Same questions here, why don't you just walk the DT and read the
compatible properties of PMIC nodes?
>> +         - const: qcom,oem-id
>> +           description: |
>> +              Matches Qualcomm Technologies, Inc. boards with the specified OEM ID.
>> +              1 integer is needed to describe the oem-id.
>> +              qcom,oem-id = <oem-id>
>> +
>>  allOf:
>>    # Explicit allow-list for older SoCs. The legacy properties are not allowed
>>    # on newer SoCs.
>> @@ -1167,4 +1237,24 @@ allOf:
>>  
>>  additionalProperties: true
>>  
>> +examples:
>> +  - |
>> +    #include <dt-bindings/arm/qcom,ids.h>
>> +    / {
>> +         model = "Qualcomm Technologies, Inc. sc7280 IDP SKU1 platform";
>> +         compatible = "qcom,sc7280-idp", "google,senor", "qcom,sc7280";
>> +
>> +         #board-id-cells = <2>;
>> +         board-id = <QCOM_SOC_ID(SC7280) QCOM_SOC_REVISION(1)>,
>> +                    <QCOM_SOC_ID(SC7280) QCOM_SOC_REVISION(2)>,
>> +                    <QCOM_BOARD_ID(IDP, 1, 0) QCOM_BOARD_SUBTYPE(UFS, ANY, 1)>;
>> +         board-id-types = "qcom,soc-id",
>> +                          "qcom,soc-id",
>> +                          "qcom,board-id";
> Forgive me if this is a particularly cynical view, but this seems
> incredibly blatant, the "qcom,board-id" property is deprecated for
> various good reasons, just using a key/value map where "qcom,board-id"
> is a key doesn't change that. There are two main issues I have with the
> proposal here:
> 
> 1. This breaks backwards compatibility, millions of production devices
> with bootloaders that will never receive another update might be
> compatible with the downstream "qcom,board-id" property, but they won't
> work with this.
> 2. A top level board-id property that isn't namespaced implies that it
> isn't vendor specific, but the proposed implementation doesn't even
> pretend to be vendor agnostic.
> 
> U-Boot also has some ideas around this issue, there you can pass in
> multiple DTBs and provide some board specific "best match" function.
> I think there's definitely some value in exposing this information, but
> there's no good reason to define the same data as `qcom,board-id` while
> breaking production bootloaders.
>> +
>> +         #address-cells = <2>;
>> +         #size-cells = <2>;
>> +    };
>> +
>> +
>>  ...
>
Elliot Berman March 14, 2024, 8:07 p.m. UTC | #4
On Thu, Mar 14, 2024 at 02:20:38PM +0000, Caleb Connolly wrote:
> Hi Amrit,
> 
> On 14/03/2024 12:11, Amrit Anand wrote:
...
> >  
> > +examples:
> > +  - |
> > +    #include <dt-bindings/arm/qcom,ids.h>
> > +    / {
> > +         model = "Qualcomm Technologies, Inc. sc7280 IDP SKU1 platform";
> > +         compatible = "qcom,sc7280-idp", "google,senor", "qcom,sc7280";
> > +
> > +         #board-id-cells = <2>;
> > +         board-id = <QCOM_SOC_ID(SC7280) QCOM_SOC_REVISION(1)>,
> > +                    <QCOM_SOC_ID(SC7280) QCOM_SOC_REVISION(2)>,
> > +                    <QCOM_BOARD_ID(IDP, 1, 0) QCOM_BOARD_SUBTYPE(UFS, ANY, 1)>;
> > +         board-id-types = "qcom,soc-id",
> > +                          "qcom,soc-id",
> > +                          "qcom,board-id";
> Forgive me if this is a particularly cynical view, but this seems
> incredibly blatant, the "qcom,board-id" property is deprecated for
> various good reasons, just using a key/value map where "qcom,board-id"
> is a key doesn't change that. There are two main issues I have with the
> proposal here:
> 
> 1. This breaks backwards compatibility, millions of production devices
> with bootloaders that will never receive another update might be
> compatible with the downstream "qcom,board-id" property, but they won't
> work with this.
> 2. A top level board-id property that isn't namespaced implies that it
> isn't vendor specific, but the proposed implementation doesn't even
> pretend to be vendor agnostic.
> 

I agree with the concerns you listed.

One point I wanted to bring is that if you provide a boot image that has
only one DTB, all production Qualcomm bootloaders I'm aware of will use
that DTB so long as "qcom,board-id" isn't a mismatch. I believe this is
what most everyone is doing if using the DTBs from kernel.org. We'd like
to use an open standard for DTB selection and that would very likely be
incompatible with existing bootloaders that don't have whatever that
standard is.

> U-Boot also has some ideas around this issue, there you can pass in
> multiple DTBs and provide some board specific "best match" function.
> I think there's definitely some value in exposing this information, but
> there's no good reason to define the same data as `qcom,board-id` while
> breaking production bootloaders.

One concern we have with U-Boot's approach is that it's based on
metadata external to the DTB and, in our experience, makes it hard to
track which board goes to which DTB. This approach also isn't
necessarily portable to other image types/boot flows.

Thanks,
Elliot
Conor Dooley March 16, 2024, 4:20 p.m. UTC | #5
On Thu, Mar 14, 2024 at 02:20:38PM +0000, Caleb Connolly wrote:
> On 14/03/2024 12:11, Amrit Anand wrote:
> 2. A top level board-id property that isn't namespaced implies that it
> isn't vendor specific, but the proposed implementation doesn't even
> pretend to be vendor agnostic.

I pointed out previously that the Chromebook guys had some similar
issues with dtb selection when the OEM varies parts but there does not
seem to be any of them on CC here.
There's definitely a generic problem to be solved here but I don't see
an effort made to involve the other people known to have the same
problems and this is all highly vendor specific.
Conor Dooley March 16, 2024, 4:51 p.m. UTC | #6
On Sat, Mar 16, 2024 at 04:20:03PM +0000, Conor Dooley wrote:
> On Thu, Mar 14, 2024 at 02:20:38PM +0000, Caleb Connolly wrote:
> > On 14/03/2024 12:11, Amrit Anand wrote:
> > 2. A top level board-id property that isn't namespaced implies that it
> > isn't vendor specific, but the proposed implementation doesn't even
> > pretend to be vendor agnostic.
> 
> I pointed out previously that the Chromebook guys had some similar
> issues with dtb selection when the OEM varies parts but there does not
> seem to be any of them on CC here.

That's maybe a bit harsh of me actually, I see that there's a
chrome-platform address on CC, but I don't know if that's gonna reach
the guys that work on these devices (Chen-Yu Tsai and Doug Anderson in
particular).
Doug Anderson March 18, 2024, 9:36 p.m. UTC | #7
Hi,

On Sat, Mar 16, 2024 at 9:51 AM Conor Dooley <conor@kernel.org> wrote:
>
> On Sat, Mar 16, 2024 at 04:20:03PM +0000, Conor Dooley wrote:
> > On Thu, Mar 14, 2024 at 02:20:38PM +0000, Caleb Connolly wrote:
> > > On 14/03/2024 12:11, Amrit Anand wrote:
> > > 2. A top level board-id property that isn't namespaced implies that it
> > > isn't vendor specific, but the proposed implementation doesn't even
> > > pretend to be vendor agnostic.
> >
> > I pointed out previously that the Chromebook guys had some similar
> > issues with dtb selection when the OEM varies parts but there does not
> > seem to be any of them on CC here.
>
> That's maybe a bit harsh of me actually, I see that there's a
> chrome-platform address on CC, but I don't know if that's gonna reach
> the guys that work on these devices (Chen-Yu Tsai and Doug Anderson in
> particular).

Thanks for the CC. Yeah, I don't watch the "chrome-platform" list
myself, though maybe I should...

The Chromebook boot flow and how we've handled this so far is
documented in the kernel [1]. This method is what we've been using
(with slight modifications over the years) since the earlier ARM
Chromebooks and is, I believe, supported in both the depthcharge
loader (used in Chromebooks) and also in U-Boot, though it's possible
(?) that the U-Boot rules might vary ever so slightly. I haven't tried
using U-Boot to boot a Chromebook in years.

The current way things work for Chromebooks involves a heavy amount of
duplication. We bundle an entire "DTB" for every relevant
board/rev/sku combination even though many of those DTBs are 99% the
same as the other ones. The DTBs have been relatively small and we
compress them so this hasn't been a massive deal, but it's always been
on the TODO list to come up with a scheme to use DT overlays. We've
also talked about bundling a device tree that has the superset of
components and then using an in-kernel driver to set the status of
some components to okay and there is some overlap there in the
possible way to represent board variants. I think Chen-Yu is going to
talk about a few of these topics next month at EOSS [2].

In terms of looking at your specific proposal, it's definitely trying
to factor in a lot more things than the current one that Chromebooks
use. The Chromebook model was "simple" enough that we could just
leverage the compatible string, though the way we leverage it has
ended up controversial over the years. Yours is definitely too
complicated to work the same way. It seems like device tree overlays
would be a better fit? I'm not an expert so maybe this is already
solved somewhere, but I'd imagine the hard part is getting everyone to
agree on how to specify stuff in the DT overlay that allows the
bootloader to know whether to overlay it or not...

[1] https://docs.kernel.org/arch/arm/google/chromebook-boot-flow.html
[2] https://eoss24.sched.com/event/1aBGe/second-source-component-probing-on-device-tree-platforms-chen-yu-tsai-google-llc
Chen-Yu Tsai March 28, 2024, 5:49 a.m. UTC | #8
On Tue, Mar 19, 2024 at 5:36 AM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Sat, Mar 16, 2024 at 9:51 AM Conor Dooley <conor@kernel.org> wrote:
> >
> > On Sat, Mar 16, 2024 at 04:20:03PM +0000, Conor Dooley wrote:
> > > On Thu, Mar 14, 2024 at 02:20:38PM +0000, Caleb Connolly wrote:
> > > > On 14/03/2024 12:11, Amrit Anand wrote:
> > > > 2. A top level board-id property that isn't namespaced implies that it
> > > > isn't vendor specific, but the proposed implementation doesn't even
> > > > pretend to be vendor agnostic.
> > >
> > > I pointed out previously that the Chromebook guys had some similar
> > > issues with dtb selection when the OEM varies parts but there does not
> > > seem to be any of them on CC here.
> >
> > That's maybe a bit harsh of me actually, I see that there's a
> > chrome-platform address on CC, but I don't know if that's gonna reach
> > the guys that work on these devices (Chen-Yu Tsai and Doug Anderson in
> > particular).
>
> Thanks for the CC. Yeah, I don't watch the "chrome-platform" list
> myself, though maybe I should...
>
> The Chromebook boot flow and how we've handled this so far is
> documented in the kernel [1]. This method is what we've been using
> (with slight modifications over the years) since the earlier ARM
> Chromebooks and is, I believe, supported in both the depthcharge
> loader (used in Chromebooks) and also in U-Boot, though it's possible
> (?) that the U-Boot rules might vary ever so slightly. I haven't tried
> using U-Boot to boot a Chromebook in years.
>
> The current way things work for Chromebooks involves a heavy amount of
> duplication. We bundle an entire "DTB" for every relevant
> board/rev/sku combination even though many of those DTBs are 99% the
> same as the other ones. The DTBs have been relatively small and we
> compress them so this hasn't been a massive deal, but it's always been
> on the TODO list to come up with a scheme to use DT overlays. We've
> also talked about bundling a device tree that has the superset of
> components and then using an in-kernel driver to set the status of
> some components to okay and there is some overlap there in the
> possible way to represent board variants. I think Chen-Yu is going to
> talk about a few of these topics next month at EOSS [2].

That's right. It's the last talk before the closing game on the last day.

Elliot is also presenting the board-id proposal at EOSS [3], which is the
last talk of day 2.

> In terms of looking at your specific proposal, it's definitely trying
> to factor in a lot more things than the current one that Chromebooks
> use. The Chromebook model was "simple" enough that we could just
> leverage the compatible string, though the way we leverage it has
> ended up controversial over the years. Yours is definitely too
> complicated to work the same way. It seems like device tree overlays
> would be a better fit? I'm not an expert so maybe this is already
> solved somewhere, but I'd imagine the hard part is getting everyone to
> agree on how to specify stuff in the DT overlay that allows the
> bootloader to know whether to overlay it or not...

I think qcom,board-id + qcom,oem-id maps to our board name + SKU ID + revision.
The difference is probably because we make our device codenames public? We
don't actually have integer identifiers for each board family, though I do
see the appeal of it.

So it looks like this proposal is more about identifying boards without
polluting (?) the compatible namespace with all sorts of combinations
and hacks like we have.

> [1] https://docs.kernel.org/arch/arm/google/chromebook-boot-flow.html
> [2] https://eoss24.sched.com/event/1aBGe/second-source-component-probing-on-device-tree-platforms-chen-yu-tsai-google-llc

[3] https://eoss24.sched.com/event/1aBFy/shipping-multiple-devicetrees-how-to-identify-which-dtb-is-for-my-board-elliot-berman-qualcomm
Amrit Anand May 14, 2024, 3:14 a.m. UTC | #9
On 3/14/2024 8:05 PM, Caleb Connolly wrote:
> On 14/03/2024 14:20, Caleb Connolly wrote:
>> Hi Amrit,
>>
>> On 14/03/2024 12:11, Amrit Anand wrote:
>>> Qualcomm produces a lot of "unique" boards with slight differences in
>>> SoC's and board's configuration. For eg, there can be SM8150v1 on MTPv1,
>>> SM8150v1 on MTPv2, SM8150v2 on MTPv2, SM8150v2 on MTPv2 with a different
>>> PMIC, SM8150v2 with no modem support and so on. For instance, suppose we
>>> have 3 SoC, each with 4 boards supported, along with 2 PMIC support for
>>> each case which would lead to total of 24 DTB files. Along with these
>>> configurations, OEMs may also add certain additional board variants. Thus
>>> a mechanism is required to pick the correct DTB for the corresponding board.
>>>
>>> Introduce mechanism to select required DTB using newly introduced device
>>> tree properties "board-id" and "board-id-type". "board-id" will contain
>>> the list of values of "qcom,soc-id", "qcom,board-id", "qcom,pmic-id" or
>>> "qcom,oem-id". "board-id-types" contains the type of parameter which is
>>> entered. It can be either "qcom,soc-id", "qcom,board-id", "qcom,pmic-id"
>>> or "qcom,oem-id".
>> Thanks for working on this, it's nice to finally see this logic
>> documented in the kernel.
>>> Qualcomm based bootloader will use these properties to pick the best
>>> matched DTB to boot the device with.
>>>
>>> Signed-off-by: Amrit Anand<quic_amrianan@quicinc.com>
>>> ---
>>>   Documentation/devicetree/bindings/arm/qcom.yaml | 90 +++++++++++++++++++++++++
>>>   1 file changed, 90 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/arm/qcom.yaml b/Documentation/devicetree/bindings/arm/qcom.yaml
>>> index 7f80f48..dc66ae9 100644
>>> --- a/Documentation/devicetree/bindings/arm/qcom.yaml
>>> +++ b/Documentation/devicetree/bindings/arm/qcom.yaml
>>> @@ -1100,6 +1100,76 @@ properties:
>>>         kernel
>>>         The property is deprecated.
>>>   
>>> +  board-id:
>>> +    $ref: /schemas/types.yaml#/definitions/uint32-matrix
>>> +    minItems: 2
>>> +    description: |
>>> +      Qualcomm specific bootloader uses multiple different identifiers
>>> +      (qcom,soc-id, qcom,board-id, qcom,pmic-id, qcom,oem-id) to select
>>> +      single Devicetree among list of Devicetrees. For different identifiers,
>>> +      the selection can be done either based on exact match (where the
>>> +      identifiers information coming from firmware should exactly match
>>> +      the ones described in devicetree) or best match (firmware provided
>>> +      identifier information closely matches with the one of the Devicetree).
>>> +      Below table describes matching criteria for each identifier::
>>> +      |----------------------------------------------------------------------|
>>> +      |  DT property  |  Individual fields   |   Exact  |  Best  |  Default  |
>>> +      |----------------------------------------------------------------------|
>>> +      | qcom,soc-id   |                                                      |
>>> +      |               |  Chipset Id          |     Y    |    N   |     -     |
>>> +      |               |  SoC Revision        |     N    |    Y   |     -     |
>>> +      | qcom,board-id |                                                      |
>>> +      |               |  Board Id            |     Y    |    N   |     -     |
>>> +      |               |  Board Major         |     N    |    Y   |     -     |
>>> +      |               |  Board Minor         |     N    |    Y   |     -     |
>>> +      |               |  Subtype             |     Y    |    N   |     0     |
>>> +      |               |  DDRtype             |     Y    |    N   |     0     |
>>> +      |               |  BootDevice Type     |     Y    |    N   |     0     |
>>> +      | qcom,pmic-id  |                                                      |
>>> +      |               |  Slave Id            |     Y    |    N   |     0     |
>>> +      |               |  PMIC Id             |     Y    |    N   |     0     |
>>> +      |               |  PMIC Major          |     N    |    Y   |     0     |
>>> +      |               |  PMIC Minor          |     N    |    Y   |     0     |
>>> +      | qcom,oem-id   |                                                      |
>>> +      |               |  OEM Id              |     Y    |    N   |     0     |
>>> +      |----------------------------------------------------------------------|
>>> +      For best match, identifiers are matched based on following priority order::
>>> +      SoC Revision > Board Major > Board Minor > PMIC Major > PMIC Minor
>>> +
>>> +  board-id-types:
>>> +    $ref: /schemas/types.yaml#/definitions/non-unique-string-array
>>> +    description:
>>> +       Each field and helper macros are defined at include/dt-bindings/arm/qcom,ids.
>>> +    minItems: 2
>>> +    items:
>>> +       oneOf:
>>> +         - const: qcom,soc-id
>>> +           description:
>>> +              Matches Qualcomm Technologies, Inc. boards with the specified SoC.
>>> +              2 integers are needed to describe a soc-id. The first integer is the
>>> +              SoC ID and the second integer is the SoC revision.
>>> +              qcom,soc-id = <soc-id  soc-revision>
>>> +         - const: qcom,board-id
>>> +           description: |
>>> +              Matches Qualcomm Technologies, Inc. boards with the specified board.
>>> +              2 integers are needed to describe a board-id. The first integer is the
>>> +              board ID. The second integer is the board-subtype.
>>> +              qcom,board-id = <board-id  board-subtype>
> This is a recursive definition. You partially described the individual
> fields above, you should do that here.

The information about these fields are documented in header 
include/dt-bindings/arm/qcom,ids.h
sent in patch 1.

> What is DDR type? What information is encoded that doesn't make sense to
> describe elsewhere in DT?
>
> Same for "bootdevice type", why would you pick a different DT based on
> whether the bootloader was loaded from UFS or NAND? Why does this
> information belong in DT?

We can have multiple DT for different DDR types and boot device types. 
In order to distinguish different DT when
all other parameters are same, we are using DDR type, boot device type 
as distinguishable parameters.

>>> +         - const: qcom,pmic-id
>>> +           description: |
>>> +              Qualcomm boards can be attached to multiple PMICs where slave-id (SID)
>>> +              indicates the address of the bus on which the PMIC is attached. It can be
>>> +              any number. The model for a PMIC indicates the PMIC name attached to bus
>>> +              described by SID along with  major and minor version. 2 integers are needed
>>> +              to describe qcom,pmic-id. The first integer is the slave-id and the second integer
>>> +              is the pmic model.
>>> +              qcom,pmic-id = <pmic-sid pmic-model>
> Same questions here, why don't you just walk the DT and read the
> compatible properties of PMIC nodes?
>>> +         - const: qcom,oem-id
>>> +           description: |
>>> +              Matches Qualcomm Technologies, Inc. boards with the specified OEM ID.
>>> +              1 integer is needed to describe the oem-id.
>>> +              qcom,oem-id = <oem-id>
>>> +
>>>   allOf:
>>>     # Explicit allow-list for older SoCs. The legacy properties are not allowed
>>>     # on newer SoCs.
>>> @@ -1167,4 +1237,24 @@ allOf:
>>>   
>>>   additionalProperties: true
>>>   
>>> +examples:
>>> +  - |
>>> +    #include <dt-bindings/arm/qcom,ids.h>
>>> +    / {
>>> +         model = "Qualcomm Technologies, Inc. sc7280 IDP SKU1 platform";
>>> +         compatible = "qcom,sc7280-idp", "google,senor", "qcom,sc7280";
>>> +
>>> +         #board-id-cells = <2>;
>>> +         board-id = <QCOM_SOC_ID(SC7280) QCOM_SOC_REVISION(1)>,
>>> +                    <QCOM_SOC_ID(SC7280) QCOM_SOC_REVISION(2)>,
>>> +                    <QCOM_BOARD_ID(IDP, 1, 0) QCOM_BOARD_SUBTYPE(UFS, ANY, 1)>;
>>> +         board-id-types = "qcom,soc-id",
>>> +                          "qcom,soc-id",
>>> +                          "qcom,board-id";
>> Forgive me if this is a particularly cynical view, but this seems
>> incredibly blatant, the "qcom,board-id" property is deprecated for
>> various good reasons, just using a key/value map where "qcom,board-id"
>> is a key doesn't change that. There are two main issues I have with the
>> proposal here:
>>
>> 1. This breaks backwards compatibility, millions of production devices
>> with bootloaders that will never receive another update might be
>> compatible with the downstream "qcom,board-id" property, but they won't
>> work with this.
>> 2. A top level board-id property that isn't namespaced implies that it
>> isn't vendor specific, but the proposed implementation doesn't even
>> pretend to be vendor agnostic.

ok, will try to redefine it. Meantime, since Elliot has some suggestions 
from his EOSS conference presentation,
will also co-work with him towards making another attempt at vendor 
agnostic approach as well.


>>
>> U-Boot also has some ideas around this issue, there you can pass in
>> multiple DTBs and provide some board specific "best match" function.
>> I think there's definitely some value in exposing this information, but
>> there's no good reason to define the same data as `qcom,board-id` while
>> breaking production bootloaders.
>>> +
>>> +         #address-cells = <2>;
>>> +         #size-cells = <2>;
>>> +    };
>>> +
>>> +
>>>   ...
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/arm/qcom.yaml b/Documentation/devicetree/bindings/arm/qcom.yaml
index 7f80f48..dc66ae9 100644
--- a/Documentation/devicetree/bindings/arm/qcom.yaml
+++ b/Documentation/devicetree/bindings/arm/qcom.yaml
@@ -1100,6 +1100,76 @@  properties:
       kernel
       The property is deprecated.
 
+  board-id:
+    $ref: /schemas/types.yaml#/definitions/uint32-matrix
+    minItems: 2
+    description: |
+      Qualcomm specific bootloader uses multiple different identifiers
+      (qcom,soc-id, qcom,board-id, qcom,pmic-id, qcom,oem-id) to select
+      single Devicetree among list of Devicetrees. For different identifiers,
+      the selection can be done either based on exact match (where the
+      identifiers information coming from firmware should exactly match
+      the ones described in devicetree) or best match (firmware provided
+      identifier information closely matches with the one of the Devicetree).
+      Below table describes matching criteria for each identifier::
+      |----------------------------------------------------------------------|
+      |  DT property  |  Individual fields   |   Exact  |  Best  |  Default  |
+      |----------------------------------------------------------------------|
+      | qcom,soc-id   |                                                      |
+      |               |  Chipset Id          |     Y    |    N   |     -     |
+      |               |  SoC Revision        |     N    |    Y   |     -     |
+      | qcom,board-id |                                                      |
+      |               |  Board Id            |     Y    |    N   |     -     |
+      |               |  Board Major         |     N    |    Y   |     -     |
+      |               |  Board Minor         |     N    |    Y   |     -     |
+      |               |  Subtype             |     Y    |    N   |     0     |
+      |               |  DDRtype             |     Y    |    N   |     0     |
+      |               |  BootDevice Type     |     Y    |    N   |     0     |
+      | qcom,pmic-id  |                                                      |
+      |               |  Slave Id            |     Y    |    N   |     0     |
+      |               |  PMIC Id             |     Y    |    N   |     0     |
+      |               |  PMIC Major          |     N    |    Y   |     0     |
+      |               |  PMIC Minor          |     N    |    Y   |     0     |
+      | qcom,oem-id   |                                                      |
+      |               |  OEM Id              |     Y    |    N   |     0     |
+      |----------------------------------------------------------------------|
+      For best match, identifiers are matched based on following priority order::
+      SoC Revision > Board Major > Board Minor > PMIC Major > PMIC Minor
+
+  board-id-types:
+    $ref: /schemas/types.yaml#/definitions/non-unique-string-array
+    description:
+       Each field and helper macros are defined at include/dt-bindings/arm/qcom,ids.
+    minItems: 2
+    items:
+       oneOf:
+         - const: qcom,soc-id
+           description:
+              Matches Qualcomm Technologies, Inc. boards with the specified SoC.
+              2 integers are needed to describe a soc-id. The first integer is the
+              SoC ID and the second integer is the SoC revision.
+              qcom,soc-id = <soc-id  soc-revision>
+         - const: qcom,board-id
+           description: |
+              Matches Qualcomm Technologies, Inc. boards with the specified board.
+              2 integers are needed to describe a board-id. The first integer is the
+              board ID. The second integer is the board-subtype.
+              qcom,board-id = <board-id  board-subtype>
+         - const: qcom,pmic-id
+           description: |
+              Qualcomm boards can be attached to multiple PMICs where slave-id (SID)
+              indicates the address of the bus on which the PMIC is attached. It can be
+              any number. The model for a PMIC indicates the PMIC name attached to bus
+              described by SID along with  major and minor version. 2 integers are needed
+              to describe qcom,pmic-id. The first integer is the slave-id and the second integer
+              is the pmic model.
+              qcom,pmic-id = <pmic-sid pmic-model>
+         - const: qcom,oem-id
+           description: |
+              Matches Qualcomm Technologies, Inc. boards with the specified OEM ID.
+              1 integer is needed to describe the oem-id.
+              qcom,oem-id = <oem-id>
+
 allOf:
   # Explicit allow-list for older SoCs. The legacy properties are not allowed
   # on newer SoCs.
@@ -1167,4 +1237,24 @@  allOf:
 
 additionalProperties: true
 
+examples:
+  - |
+    #include <dt-bindings/arm/qcom,ids.h>
+    / {
+         model = "Qualcomm Technologies, Inc. sc7280 IDP SKU1 platform";
+         compatible = "qcom,sc7280-idp", "google,senor", "qcom,sc7280";
+
+         #board-id-cells = <2>;
+         board-id = <QCOM_SOC_ID(SC7280) QCOM_SOC_REVISION(1)>,
+                    <QCOM_SOC_ID(SC7280) QCOM_SOC_REVISION(2)>,
+                    <QCOM_BOARD_ID(IDP, 1, 0) QCOM_BOARD_SUBTYPE(UFS, ANY, 1)>;
+         board-id-types = "qcom,soc-id",
+                          "qcom,soc-id",
+                          "qcom,board-id";
+
+         #address-cells = <2>;
+         #size-cells = <2>;
+    };
+
+
 ...