diff mbox series

[2/3] dt-bindings: arm: Add SolidRun LX2162A SoM & Clearfog Board

Message ID 20230616110610.32173-3-josua@solid-run.com
State New
Headers
Series arm64: dts: freescale: Add support for LX2162 SoM & Clearfog Board |

Commit Message

Josua Mayer June 16, 2023, 11:06 a.m. UTC 
  Add DT compatible for SolidRun LX2162A SoM and Clearfog board.

Signed-off-by: Josua Mayer <josua@solid-run.com>
---
 Documentation/devicetree/bindings/arm/fsl.yaml | 2 ++
 1 file changed, 2 insertions(+)

Comments

Krzysztof Kozlowski June 16, 2023, 11:36 a.m. UTC | #1 
On 16/06/2023 13:06, Josua Mayer wrote:
> Add DT compatible for SolidRun LX2162A SoM and Clearfog board.
> 
> Signed-off-by: Josua Mayer <josua@solid-run.com>
> ---
>  Documentation/devicetree/bindings/arm/fsl.yaml | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/arm/fsl.yaml b/Documentation/devicetree/bindings/arm/fsl.yaml
> index 15d411084065..438a4ece8157 100644
> --- a/Documentation/devicetree/bindings/arm/fsl.yaml
> +++ b/Documentation/devicetree/bindings/arm/fsl.yaml
> @@ -1373,9 +1373,11 @@ properties:
>        - description: SolidRun LX2160A based Boards
>          items:
>            - enum:
> +              - solidrun,clearfog
>                - solidrun,clearfog-cx
>                - solidrun,honeycomb
>            - const: solidrun,lx2160a-cex7
> +          - const: solidrun,lx2162a-som
>            - const: fsl,lx2160a

You change existing entries, breaking boards and changing the meaning,
without any explanation in commit msg. That's not how it is done. Please
provide rationale in commit msg.

Best regards,
Krzysztof
Josua Mayer June 16, 2023, 1:32 p.m. UTC | #2 
HI Krzysztof,

Am 16.06.23 um 14:36 schrieb Krzysztof Kozlowski:
> On 16/06/2023 13:06, Josua Mayer wrote:
>> Add DT compatible for SolidRun LX2162A SoM and Clearfog board.
>>
>> Signed-off-by: Josua Mayer <josua@solid-run.com>
>> ---
>>   Documentation/devicetree/bindings/arm/fsl.yaml | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/arm/fsl.yaml b/Documentation/devicetree/bindings/arm/fsl.yaml
>> index 15d411084065..438a4ece8157 100644
>> --- a/Documentation/devicetree/bindings/arm/fsl.yaml
>> +++ b/Documentation/devicetree/bindings/arm/fsl.yaml
>> @@ -1373,9 +1373,11 @@ properties:
>>         - description: SolidRun LX2160A based Boards
>>           items:
>>             - enum:
>> +              - solidrun,clearfog
>>                 - solidrun,clearfog-cx
>>                 - solidrun,honeycomb
>>             - const: solidrun,lx2160a-cex7
>> +          - const: solidrun,lx2162a-som
>>             - const: fsl,lx2160a
> You change existing entries, breaking boards and changing the meaning,
> without any explanation in commit msg. That's not how it is done. Please
> provide rationale in commit msg.

I'm sorry. Given your comment I think I did not understand how these 
entries are supposed to work.
So perhaps you can provide some guidance based on my explanation?:

- NXP LX2162 is a smaller physical package of the same LX2160 SoC, with 
reduced IOs and some silicon blocks disabled.
- SolidRun LX2162 SoM is essentially a different form factor of LX2160 CEX
- SolidRun LX2162 Clearfog is the reference platform for the SoM. 
Despite it's naming similarity to clearfog-cx, it has a different 
feature set more similar to SolidRun Armada 388 Clearfog Pro

So I believed I could just add to the existing entry "SolidRun LX2160A 
based Boards" also the new LX2162 Board & SoM.
I see now that adding a fourth const messes upthe existing 3-part 
compatible for those already existing boards.

Please can you confirm if it would have been more correct to replace 
"const: solidrun,lx2160a-cex7" with an enum?:
enum:
   - solidrun,lx2160a-cex7
   - solidrun,lx2162a-som

Finally, is it okay to add a "solidrun,clearfog" given my explanation 
above, or should it be more specific "solidrun,lx2162a-clearfog"?

> Best regards,
> Krzysztof
Sincerely
Josua Mayer
Andreas Kemnade June 16, 2023, 4:57 p.m. UTC | #3 
On Fri, 16 Jun 2023 16:32:01 +0300
Josua Mayer <josua@solid-run.com> wrote:

> HI Krzysztof,
> 
> Am 16.06.23 um 14:36 schrieb Krzysztof Kozlowski:
> > On 16/06/2023 13:06, Josua Mayer wrote:  
> >> Add DT compatible for SolidRun LX2162A SoM and Clearfog board.
> >>
> >> Signed-off-by: Josua Mayer <josua@solid-run.com>
> >> ---
> >>   Documentation/devicetree/bindings/arm/fsl.yaml | 2 ++
> >>   1 file changed, 2 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/arm/fsl.yaml b/Documentation/devicetree/bindings/arm/fsl.yaml
> >> index 15d411084065..438a4ece8157 100644
> >> --- a/Documentation/devicetree/bindings/arm/fsl.yaml
> >> +++ b/Documentation/devicetree/bindings/arm/fsl.yaml
> >> @@ -1373,9 +1373,11 @@ properties:
> >>         - description: SolidRun LX2160A based Boards
> >>           items:
> >>             - enum:
> >> +              - solidrun,clearfog
> >>                 - solidrun,clearfog-cx
> >>                 - solidrun,honeycomb
> >>             - const: solidrun,lx2160a-cex7
> >> +          - const: solidrun,lx2162a-som
> >>             - const: fsl,lx2160a  
> > You change existing entries, breaking boards and changing the meaning,
> > without any explanation in commit msg. That's not how it is done. Please
> > provide rationale in commit msg.  
> 
> I'm sorry. Given your comment I think I did not understand how these 
> entries are supposed to work.
> So perhaps you can provide some guidance based on my explanation?:
> 

it breaks:
arm64/boot/dts/freescale/fsl-lx2160a-clearfog-cx.dts
compatible = "solidrun,clearfog-cx",
                "solidrun,lx2160a-cex7", "fsl,lx2160a";

now you would require:
compatible = "solidrun,clearfog-cx",
                "solidrun,lx2160a-cex7", "solidrun,lx2162a-som", "fsl,lx2160a"

which is probably not what you want.

Regards,
Andreas
Krzysztof Kozlowski June 16, 2023, 4:59 p.m. UTC | #4 
On 16/06/2023 15:32, Josua Mayer wrote:
> HI Krzysztof,
> 
> Am 16.06.23 um 14:36 schrieb Krzysztof Kozlowski:
>> On 16/06/2023 13:06, Josua Mayer wrote:
>>> Add DT compatible for SolidRun LX2162A SoM and Clearfog board.
>>>
>>> Signed-off-by: Josua Mayer <josua@solid-run.com>
>>> ---
>>>   Documentation/devicetree/bindings/arm/fsl.yaml | 2 ++
>>>   1 file changed, 2 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/arm/fsl.yaml b/Documentation/devicetree/bindings/arm/fsl.yaml
>>> index 15d411084065..438a4ece8157 100644
>>> --- a/Documentation/devicetree/bindings/arm/fsl.yaml
>>> +++ b/Documentation/devicetree/bindings/arm/fsl.yaml
>>> @@ -1373,9 +1373,11 @@ properties:
>>>         - description: SolidRun LX2160A based Boards
>>>           items:
>>>             - enum:
>>> +              - solidrun,clearfog
>>>                 - solidrun,clearfog-cx
>>>                 - solidrun,honeycomb
>>>             - const: solidrun,lx2160a-cex7
>>> +          - const: solidrun,lx2162a-som
>>>             - const: fsl,lx2160a
>> You change existing entries, breaking boards and changing the meaning,
>> without any explanation in commit msg. That's not how it is done. Please
>> provide rationale in commit msg.
> 
> I'm sorry. Given your comment I think I did not understand how these 
> entries are supposed to work.
> So perhaps you can provide some guidance based on my explanation?:
> 
> - NXP LX2162 is a smaller physical package of the same LX2160 SoC, with 
> reduced IOs and some silicon blocks disabled.
> - SolidRun LX2162 SoM is essentially a different form factor of LX2160 CEX
> - SolidRun LX2162 Clearfog is the reference platform for the SoM. 
> Despite it's naming similarity to clearfog-cx, it has a different 
> feature set more similar to SolidRun Armada 388 Clearfog Pro
> 
> So I believed I could just add to the existing entry "SolidRun LX2160A 
> based Boards" also the new LX2162 Board & SoM.

But you added much more, didn't you?

> I see now that adding a fourth const messes upthe existing 3-part 
> compatible for those already existing boards.
> 
> Please can you confirm if it would have been more correct to replace 
> "const: solidrun,lx2160a-cex7" with an enum?:
> enum:
>    - solidrun,lx2160a-cex7
>    - solidrun,lx2162a-som
> 
> Finally, is it okay to add a "solidrun,clearfog" given my explanation 
> above, or should it be more specific "solidrun,lx2162a-clearfog"?
> 

Test the binding and test DTS against it:
Please run `make dtbs_check` (see
Documentation/devicetree/bindings/writing-schema.rst for instructions).
https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/

It might point you to answer.

Why do you make solidrun,honeycomb compatible with cex7 and som?

Best regards,
Krzysztof
Josua Mayer June 17, 2023, 1:34 p.m. UTC | #5 
Hi Andreas,

Am 16.06.23 um 19:57 schrieb Andreas Kemnade:
> On Fri, 16 Jun 2023 16:32:01 +0300
> Josua Mayer <josua@solid-run.com> wrote:
>
>> HI Krzysztof,
>>
>> Am 16.06.23 um 14:36 schrieb Krzysztof Kozlowski:
>>> On 16/06/2023 13:06, Josua Mayer wrote:
>>>> Add DT compatible for SolidRun LX2162A SoM and Clearfog board.
>>>>
>>>> Signed-off-by: Josua Mayer <josua@solid-run.com>
>>>> ---
>>>>    Documentation/devicetree/bindings/arm/fsl.yaml | 2 ++
>>>>    1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/arm/fsl.yaml b/Documentation/devicetree/bindings/arm/fsl.yaml
>>>> index 15d411084065..438a4ece8157 100644
>>>> --- a/Documentation/devicetree/bindings/arm/fsl.yaml
>>>> +++ b/Documentation/devicetree/bindings/arm/fsl.yaml
>>>> @@ -1373,9 +1373,11 @@ properties:
>>>>          - description: SolidRun LX2160A based Boards
>>>>            items:
>>>>              - enum:
>>>> +              - solidrun,clearfog
>>>>                  - solidrun,clearfog-cx
>>>>                  - solidrun,honeycomb
>>>>              - const: solidrun,lx2160a-cex7
>>>> +          - const: solidrun,lx2162a-som
>>>>              - const: fsl,lx2160a
>>> You change existing entries, breaking boards and changing the meaning,
>>> without any explanation in commit msg. That's not how it is done. Please
>>> provide rationale in commit msg.
>> I'm sorry. Given your comment I think I did not understand how these
>> entries are supposed to work.
>> So perhaps you can provide some guidance based on my explanation?:
>>
> it breaks:
> arm64/boot/dts/freescale/fsl-lx2160a-clearfog-cx.dts
> compatible = "solidrun,clearfog-cx",
>                  "solidrun,lx2160a-cex7", "fsl,lx2160a";
>
> now you would require:
> compatible = "solidrun,clearfog-cx",
>                  "solidrun,lx2160a-cex7", "solidrun,lx2162a-som", "fsl,lx2160a"
I see, thanks!
The more I look at it, the more logical this behaviour seems.
> which is probably not what you want.

Yep. I wanted to keep the 3 components, and allow forking in the middle:

"solidrun,<board>", "solidrun,<module>", "fsl,lx2160a"

This however creates many combinations that are undesirable.
Existing boards using com express don't suddenly become compatible with 
the new SoM.
Therefore I prepared a different approach vor v2 now.

> Regards,
> Andreas
- Josua Mayer
Josua Mayer June 17, 2023, 2:09 p.m. UTC | #6 
Hi Krzysztof,

Am 16.06.23 um 19:59 schrieb Krzysztof Kozlowski:
> Test the binding and test DTS against it:
> Please run `make dtbs_check` (see
> Documentation/devicetree/bindings/writing-schema.rst for instructions).
> https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
This was helpful! I ran it a few times and studied the results.
Many complaints related to lx2160a.dtsi, some common among different 
layerscape based dts.
Finally some mistakes of my own.

v2 addresses all that were under my control, and a common one that I 
understood well enough.

> It might point you to answer.
>
> Why do you make solidrun,honeycomb compatible with cex7 and som?

Exactly - unintentional.


> Best regards,
> Krzysztof
- Josua Mayer
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/arm/fsl.yaml b/Documentation/devicetree/bindings/arm/fsl.yaml
index 15d411084065..438a4ece8157 100644
--- a/Documentation/devicetree/bindings/arm/fsl.yaml
+++ b/Documentation/devicetree/bindings/arm/fsl.yaml
@@ -1373,9 +1373,11 @@  properties:
       - description: SolidRun LX2160A based Boards
         items:
           - enum:
+              - solidrun,clearfog
               - solidrun,clearfog-cx
               - solidrun,honeycomb
           - const: solidrun,lx2160a-cex7
+          - const: solidrun,lx2162a-som
           - const: fsl,lx2160a
 
       - description: S32G2 based Boards