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

[Josua Mayer]

Am 26.03.24 um 07:41 schrieb Krzysztof Kozlowski:
> On 25/03/2024 21:12, Josua Mayer wrote:
>> 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.
> No wildcards, but if the SoM name is 9130 then use 9130.
> The problem is that you use cn9130 SoC as fallback.
>
>>>>           - 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.
> OK, but how does it affect compatibility between them? Which parts are
> the same? Or how much is shared?
9130, 9131, 9132 belong together.
9130 is single chip including two dies: AP, CP.
The CP is available as an individual chip,
up to two can be connected to one 9130.

What does this mean for compatibility?
Which compatibility specifically?
Is there a definition we can refer to?

From software perspective we can always down-grade,
i.e. run software only aware of the AP on 9130, 9131 or 9132.
But we can't run software referencing the external CPs
if they are not connected.

>>>>           - 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.
> That's also not the best example.Might be correct but also looks
> over-complicated. The point of board-level compatibles is to identify
> machine and its common parts. It has little impact inside of kernel (at
> least should be almost no users inside!)
Indeed, the temperature coefficients are handled by the thermal device
compatible string, not board-level.
> , but there can be some users,
> e.g. firmware or user-space.
>
> This claims that cn9132 is compatible with ap807, so you have exactly
> the same base. The same base is not CPU! It's about the S in SoC, so
> "System".
I would think since the base is always a single chip combining 1x AP+CP,
the "system" is marvell,cn9130.
For Armada 8040, the system would be marvell,armada8040 by same
logic (also combining 1x AP+CP, different version, not extensible).
> Could firmware use marvell,armada-ap807 compatible to properly
> detect type of system and treat all these boards as ap807?
I have not looked into presence detection for CP's during initialization.
U-Boot support without spaghetti is a future Me task.
I suspect it is possible with asterisk *, because so far I have only seen
configuration with at least 1 CP, never with 0.
Presence of a boot-rom on each die e.g. supports this idea.


sincerely

Josua Mayer