Re: [PATCH 1/2] dt-bindings: arm64: marvell: add solidrun cn9130 clearfog boards

[Josua Mayer]

Am 25.03.24 um 20:34 schrieb Krzysztof Kozlowski:
> On 22/03/2024 11:08, Josua Mayer wrote:
>> Am 21.03.24 um 22:47 schrieb Josua Mayer:
>>> Add bindings for SolidRun Clearfog boards, using a new SoM based on
>>> CN9130 SoC.
>>> The carrier boards are identical to the older Armada 388 based Clearfog
>>> boards. For consistency the carrier part of compatible strings are
>>> copied, including the established "-a1" suffix.
>>>
>>> Signed-off-by: Josua Mayer <josua@xxxxxxxxxxxxx>
>>> ---
>>>  .../devicetree/bindings/arm/marvell/armada-7k-8k.yaml        | 12 ++++++++++++
>>>  1 file changed, 12 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/arm/marvell/armada-7k-8k.yaml b/Documentation/devicetree/bindings/arm/marvell/armada-7k-8k.yaml
>>> index 16d2e132d3d1..36bdfd1bedd9 100644
>>> --- a/Documentation/devicetree/bindings/arm/marvell/armada-7k-8k.yaml
>>> +++ b/Documentation/devicetree/bindings/arm/marvell/armada-7k-8k.yaml
>>> @@ -82,4 +82,16 @@ properties:
>>>            - const: marvell,armada-ap807-quad
>>>            - const: marvell,armada-ap807
>>>  
>>> +      - description:
>>> +          SolidRun CN9130 clearfog family single-board computers
>>> +        items:
>>> +          - enum:
>>> +              - solidrun,clearfog-base-a1
>>> +              - solidrun,clearfog-pro-a1
>>> +          - const: solidrun,clearfog-a1
>>> +          - const: solidrun,cn9130-sr-som
>>> +          - const: marvell,cn9130
>>> +          - const: marvell,armada-ap807-quad
>>> +          - const: marvell,armada-ap807
>>> +
>>>  additionalProperties: true
>> Before merging I would like some feedback about adding
>> another product later, to ensure the compatibles above
>> are adequate? In particular:
>> - sequence of soc, cp, carrier compatibles
>> - name of som compatible
>>
>> Draft for future bindings:
>>       - description:
>>           SolidRun CN9130 SoM based single-board computers
>>           with 1 external CP on the Carrier.
>>         items:
>>           - enum:
>>               - solidrun,cn9131-solidwan
>>           - const: marvell,cn9131
>>           - const: solidrun,cn9130-sr-som
> This does not look correct. cn9131 is not compatible with your som.
This is partially my question.
I considered changing the som to "cn913x-sr-som".

The SoM itself is always 9130, it contains the base SoC
with 1x AP and 1x CP in a single chip.
9131 and 9132 <happen> on the carrier boards.

>
>>           - const: marvell,cn9130
> SoCs are compatible only in some cases, e.g. one is a subset of another
> like stripped out of modem. Are you sure this is your case?
This is more complex, CN9131 and CN9132 are not single SoCs.
A "9132" is instantiated by connecting two southbridge chips
via a Marvell defined bus, each providing additional IO
such as network, i2c, gpio.

Note that even the first, "9130", while a single chip, contains two dies:
An "AP" (Application Processor I assume) with very limited IO (1xsdio, 1xi2c),
and a "CP" (Communication Processor I assume) with lots of IO.
This CP as far as I know today is identical to the southbridges
mentioned above.

>>           - const: marvell,armada-ap807-quad
>>           - const: marvell,armada-ap807
> Anyway, 6 compatibles is beyond useful amount. What are you expressing
> here?
I copied this part from the examples earlier in the file, such as:
      - description: Armada CN9132 SoC with two external CPs
        items:
          - const: marvell,cn9132
          - const: marvell,cn9131
          - const: marvell,cn9130
          - const: marvell,armada-ap807-quad
          - const: marvell,armada-ap807
>  Why is this even armada ap807?
We noticed ap807 != ap806 (cn913x != 8040),
because the thermal sensor coefficients converting
raw values to celsius differed.