Re: [PATCH v2 1/2] dt-bindings: arm: qcom: document qcom,msm-id and qcom,board-id

[Krzysztof Kozlowski]

On 21/06/2022 21:49, Dmitry Baryshkov wrote:
> On Tue, 21 Jun 2022 at 22:32, Krzysztof Kozlowski
> <krzysztof.kozlowski@xxxxxxxxxx> wrote:
>>
>> On 21/06/2022 21:26, Dmitry Baryshkov wrote:
>>> On 21/06/2022 21:56, Krzysztof Kozlowski wrote:
>>>> The top level qcom,msm-id and qcom,board-id properties are utilized by
>>>> bootloaders on Qualcomm MSM platforms to determine which device tree
>>>> should be used and passed to the kernel.
>>>>
>>>> The commit b32e592d3c28 ("devicetree: bindings: Document qcom board
>>>> compatible format") from 2015 was a consensus during discussion about
>>>> upstreaming qcom,msm-id and qcom,board-id fields.  There are however still
>>>> problems with that consensus:
>>>> 1. It was reached 7 years ago but it turned out its implementation did
>>>>     not reach all possible products.
>>>>
>>>> 2. Initially additional tool (dtbTool) was needed for parsing these
>>>>     fields to create a QCDT image consisting of multiple DTBs, later the
>>>>     bootloaders were improved and they use these qcom,msm-id and
>>>>     qcom,board-id properties directly.
>>>
>>> I might be mistaken here. I think it was expected that dtbTool would use
>>> board compat strings to generate qcom,msm-id and qcom,board-id
>>> properties. It's not that the bootloaders were improved.
>>
>> Don't ask me, I am new to this.
>>
>> https://lore.kernel.org/all/02ab0276-b078-fe66-8596-fcec4378722b@xxxxxxxxxxxxxx/
> 
> 
> 
> 
>>
>>>
>>>>
>>>> 3. Extracting relevant information from the board compatible requires
>>>>     this additional tool (dtbTool), which makes the build process more
>>>>     complicated and not easily reproducible (DTBs are modified after the
>>>>     kernel build).
>>>>
>>>> 4. Some versions of Qualcomm bootloaders expect these properties even
>>>>     when booting with a single DTB.  The community is stuck with these
>>>>     bootloaders thus they require properties in the DTBs.
>>>>
>>>> Since several upstreamed Qualcomm SoC-based boards require these
>>>> properties to properly boot and the properties are reportedly used by
>>>> bootloaders, document them.
>>>>
>>>> Link: https://lore.kernel.org/r/a3c932d1-a102-ce18-deea-18cbbd05ecab@xxxxxxxxxx/
>>>> Co-developed-by: Kumar Gala <galak@xxxxxxxxxxxxxx>
>>>> Signed-off-by: Kumar Gala <galak@xxxxxxxxxxxxxx>
>>>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
>>>> ---
>>>>   .../devicetree/bindings/arm/qcom.yaml         | 123 ++++++++++++++++++
>>>>   include/dt-bindings/arm/qcom,ids.h            |  30 +++++
>>>>   2 files changed, 153 insertions(+)
>>>>   create mode 100644 include/dt-bindings/arm/qcom,ids.h
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/arm/qcom.yaml b/Documentation/devicetree/bindings/arm/qcom.yaml
>>>> index 6c38c1387afd..05b98cde4653 100644
>>>> --- a/Documentation/devicetree/bindings/arm/qcom.yaml
>>>> +++ b/Documentation/devicetree/bindings/arm/qcom.yaml
>>>> @@ -403,6 +403,129 @@ properties:
>>>>                 - qcom,sm8450-qrd
>>>>             - const: qcom,sm8450
>>>>
>>>> +  # Board compatibles go above
>>>> +
>>>> +  qcom,msm-id:
>>>> +    $ref: /schemas/types.yaml#/definitions/uint32-matrix
>>>> +    minItems: 1
>>>> +    maxItems: 8
>>>> +    items:
>>>> +      items:
>>>> +        - description: |
>>>> +            MSM chipset ID - an exact match value consisting of three bitfields::
>>>
>>> two bitfields
>>
>> Right, thanks.
>>
>>>
>>>> +             - bits 0-15  - The unique MSM chipset ID
>>>> +             - bits 16-31 - Reserved; should be 0
>>>> +        - description: |
>>>> +            Hardware revision ID - a chipset specific 32-bit ID representing
>>>> +            the version of the chipset.  It is best a match value - the
>>>> +            bootloader will look for the closest possible match.
>>>> +    deprecated: true
>>>> +    description:
>>>> +      The MSM chipset and hardware revision use by Qualcomm bootloaders.  It
>>>> +      can optionally be an array of these to indicate multiple hardware that
>>>> +      use the same device tree.  It is expected that the bootloader will use
>>>> +      this information at boot-up to decide which device tree to use when given
>>>> +      multiple device trees, some of which may not be compatible with the
>>>> +      actual hardware.  It is the bootloader's responsibility to pass the
>>>> +      correct device tree to the kernel.
>>>> +      The property is deprecated - it is not expected on newer boards
>>>> +      (starting with SM8350).
>>>
>>> Could you please elaborate this?
>>
>> Second paragraph:
>> https://lore.kernel.org/all/20220522195138.35943-1-konrad.dybcio@xxxxxxxxxxxxxx/
> 
> I think this is something peculiar to Sony. Public lahaina (sm8350)
> dts files contain both these properties:
> 
> https://github.com/MiCode/kernel_devicetree/blob/zeus-s-oss/qcom/lahaina-hdk.dts
> https://github.com/MiCode/kernel_devicetree/blob/zeus-s-oss/qcom/lahaina-v2.1.dtsi
> 
>>
>> Plus consensus with Rob:
>> https://lore.kernel.org/all/CAL_JsqKL-mtAQ8Q9H4vLGM8izVVzDPbUAVWSdS8AmGjN6X6kcA@xxxxxxxxxxxxxx/
> 
> I'm not sure here. But sm8350 and sm8450 dtsi files use these
> properties. I've linked lahaina files above.
> The waiptio dtsi (sm8450) are present at the same URL.

If you did not like where the consensus is going during the discussion
last week, I would expect to join the discussion. Not to comment after I
implement it.

> 
>>
>>> If the AOSP team were to add e.g.
>>> SM8350-HDK to their single RB3+RB5 images, they would still need the
>>> qcom,board-id/qcom,msm-id properties to let the bootloader choose proper
>>> DTB.
>>
>> If you have any email addresses in mind, please Cc them to invite in
>> discussions. Otherwise I am afraid it won't be allowed. The feedback I
>> got before was that SM8350 and newer do not require this property. Feel
>> free to propose other way to solve comments (see "consensus with Rob"
>> above).
> 
> Amit is in CC list. In the past he used these properties to allow
> single-image booting of RB3 and RB5.
> In fact I might prefer adding more of these properties to the dts
> files, where it makes sense, to allow adding more dt files to the
> images we create.
> I'd really like to be able to boot a single image on all my boards
> (rb3, rb5, db410c, db820, ifc6560, etc).

You have several options here. Use the board-compatible-encoded-scheme,
which was merged like 6 years ago or something. Bootloader could parse
it, dtbTool as well. Add a generic property, like Rob wanted (and
probably fix bootloader). Or find any other way to satisfy Rob's
comments. These properties were not accepted 6 years ago and the board
compatible approach was merged instead. If 6 years is not enough to
change the bootloaders, nothing will happen here ever, so we need to
make some statement.


Best regards,
Krzysztof