[PATCH v6 04/17] dt-bindings: spi: dw: Add AMD Pensando Elba SoC SPI Controller bindings

Brad Larson posted 17 patches 3 years, 7 months ago
There is a newer version of this series
[PATCH v6 04/17] dt-bindings: spi: dw: Add AMD Pensando Elba SoC SPI Controller bindings
Posted by Brad Larson 3 years, 7 months ago
From: Brad Larson <blarson@amd.com>

The AMD Pensando Elba SoC has integrated the DW APB SPI Controller

Signed-off-by: Brad Larson <blarson@amd.com>
---
 .../devicetree/bindings/spi/snps,dw-apb-ssi.yaml      | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
index 37c3c272407d..403d6416f7ac 100644
--- a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
+++ b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
@@ -37,6 +37,15 @@ allOf:
     else:
       required:
         - interrupts
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - amd,pensando-elba-spi
+    then:
+      required:
+        - amd,pensando-elba-syscon
 
 properties:
   compatible:
@@ -75,6 +84,8 @@ properties:
               - renesas,r9a06g032-spi # RZ/N1D
               - renesas,r9a06g033-spi # RZ/N1S
           - const: renesas,rzn1-spi   # RZ/N1
+      - description: AMD Pensando Elba SoC SPI Controller
+        const: amd,pensando-elba-spi
 
   reg:
     minItems: 1
-- 
2.17.1
Re: [PATCH v6 04/17] dt-bindings: spi: dw: Add AMD Pensando Elba SoC SPI Controller bindings
Posted by Krzysztof Kozlowski 3 years, 7 months ago
On 20/08/2022 22:57, Brad Larson wrote:
> From: Brad Larson <blarson@amd.com>
> 
> The AMD Pensando Elba SoC has integrated the DW APB SPI Controller
> 
> Signed-off-by: Brad Larson <blarson@amd.com>
> ---
>  .../devicetree/bindings/spi/snps,dw-apb-ssi.yaml      | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
> index 37c3c272407d..403d6416f7ac 100644
> --- a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
> +++ b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
> @@ -37,6 +37,15 @@ allOf:
>      else:
>        required:
>          - interrupts
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - amd,pensando-elba-spi
> +    then:
> +      required:
> +        - amd,pensando-elba-syscon

There is no such property. You cannot make it required without first
defining it.

>  
>  properties:
>    compatible:
> @@ -75,6 +84,8 @@ properties:
>                - renesas,r9a06g032-spi # RZ/N1D
>                - renesas,r9a06g033-spi # RZ/N1S
>            - const: renesas,rzn1-spi   # RZ/N1
> +      - description: AMD Pensando Elba SoC SPI Controller
> +        const: amd,pensando-elba-spi

Don't add stuff at the end, but in some logical (usually alphabetical)
place. The order is already broken as everyone likes to add stuff in
conflict-style, so just add it before baikal, for example.


Best regards,
Krzysztof
Re: [PATCH v6 04/17] dt-bindings: spi: dw: Add AMD Pensando Elba SoC SPI Controller bindings
Posted by Larson, Bradley 3 years, 7 months ago
On 8/22/22 11:19 AM, Krzysztof Kozlowski wrote:
> On 20/08/2022 22:57, Brad Larson wrote:
>> From: Brad Larson <blarson@amd.com>
>>
>> The AMD Pensando Elba SoC has integrated the DW APB SPI Controller
>>
>> Signed-off-by: Brad Larson <blarson@amd.com>
>> ---
>>   .../devicetree/bindings/spi/snps,dw-apb-ssi.yaml      | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
>> index 37c3c272407d..403d6416f7ac 100644
>> --- a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
>> +++ b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
>> @@ -37,6 +37,15 @@ allOf:
>>       else:
>>         required:
>>           - interrupts
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            enum:
>> +              - amd,pensando-elba-spi
>> +    then:
>> +      required:
>> +        - amd,pensando-elba-syscon
> There is no such property. You cannot make it required without first
> defining it.

Added the definition of 'amd,pensando-elba-syscon' to snps,dw-apb-ssi.yaml

>>   properties:
>>     compatible:
>> @@ -75,6 +84,8 @@ properties:
>>                 - renesas,r9a06g032-spi # RZ/N1D
>>                 - renesas,r9a06g033-spi # RZ/N1S
>>             - const: renesas,rzn1-spi   # RZ/N1
>> +      - description: AMD Pensando Elba SoC SPI Controller
>> +        const: amd,pensando-elba-spi
> Don't add stuff at the end, but in some logical (usually alphabetical)
> place. The order is already broken as everyone likes to add stuff in
> conflict-style, so just add it before baikal, for example.

Yes, tried to follow existing style.  Will add it before baikal.

Regards,
Brad
Re: [PATCH v6 04/17] dt-bindings: spi: dw: Add AMD Pensando Elba SoC SPI Controller bindings
Posted by Serge Semin 3 years, 7 months ago
On Sat, Aug 20, 2022 at 12:57:37PM -0700, Brad Larson wrote:
> From: Brad Larson <blarson@amd.com>
> 
> The AMD Pensando Elba SoC has integrated the DW APB SPI Controller
> 
> Signed-off-by: Brad Larson <blarson@amd.com>
> ---
>  .../devicetree/bindings/spi/snps,dw-apb-ssi.yaml      | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
> index 37c3c272407d..403d6416f7ac 100644
> --- a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
> +++ b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
> @@ -37,6 +37,15 @@ allOf:
>      else:
>        required:
>          - interrupts
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - amd,pensando-elba-spi
> +    then:
> +      required:
> +        - amd,pensando-elba-syscon

Please add the "amd,pensando-elba-syscon" property definition as I
asked here:
https://lore.kernel.org/lkml/20220704131810.kabkuy6e4qmhfm3n@mobilestation/

-Sergey

>  
>  properties:
>    compatible:
> @@ -75,6 +84,8 @@ properties:
>                - renesas,r9a06g032-spi # RZ/N1D
>                - renesas,r9a06g033-spi # RZ/N1S
>            - const: renesas,rzn1-spi   # RZ/N1
> +      - description: AMD Pensando Elba SoC SPI Controller
> +        const: amd,pensando-elba-spi
>  
>    reg:
>      minItems: 1
> -- 
> 2.17.1
>
Re: [PATCH v6 04/17] dt-bindings: spi: dw: Add AMD Pensando Elba SoC SPI Controller bindings
Posted by Larson, Bradley 3 years, 7 months ago
On 8/21/22 10:49 AM, Serge Semin wrote:
> On Sat, Aug 20, 2022 at 12:57:37PM -0700, Brad Larson wrote:
>> From: Brad Larson <blarson@amd.com>
>>
>> The AMD Pensando Elba SoC has integrated the DW APB SPI Controller
>>
>> Signed-off-by: Brad Larson <blarson@amd.com>
>> ---
>>   .../devicetree/bindings/spi/snps,dw-apb-ssi.yaml      | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
>> index 37c3c272407d..403d6416f7ac 100644
>> --- a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
>> +++ b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
>> @@ -37,6 +37,15 @@ allOf:
>>       else:
>>         required:
>>           - interrupts
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            enum:
>> +              - amd,pensando-elba-spi
>> +    then:
>> +      required:
>> +        - amd,pensando-elba-syscon
> Please add the "amd,pensando-elba-syscon" property definition as I
> asked here:
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2F20220704131810.kabkuy6e4qmhfm3n%40mobilestation%2F&amp;data=05%7C01%7Cbradley.larson%40amd.com%7C1c4f822c81424048873508da839d90fc%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637967010019245894%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=xl9OU9P9QK3wLHc25hQZK393ylULd41qc4HB2Zt%2F0BQ%3D&amp;reserved=0

Proposing this addition:

--- a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
+++ b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
@@ -148,6 +148,15 @@ properties:
        of the designware controller, and the upper limit is also subject to
        controller configuration.

+  amd,pensando-elba-syscon:
+    $ref: "/schemas/types.yaml#/definitions/phandle-array"
+    maxItems: 1
+    description:
+      A phandle to syscon used to access the spi chip-select override 
register.
+    items:
+      - items:
+        - description: phandle to the syscon node
+
  patternProperties:
    "^.*@[0-9a-f]+$":
      type: object

Regards,
Brad
Re: [PATCH v6 04/17] dt-bindings: spi: dw: Add AMD Pensando Elba SoC SPI Controller bindings
Posted by Serge Semin 3 years, 6 months ago
On Wed, Aug 31, 2022 at 06:28:46PM +0000, Larson, Bradley wrote:
> On 8/21/22 10:49 AM, Serge Semin wrote:
> > On Sat, Aug 20, 2022 at 12:57:37PM -0700, Brad Larson wrote:
> >> From: Brad Larson <blarson@amd.com>
> >>
> >> The AMD Pensando Elba SoC has integrated the DW APB SPI Controller
> >>
> >> Signed-off-by: Brad Larson <blarson@amd.com>
> >> ---
> >>   .../devicetree/bindings/spi/snps,dw-apb-ssi.yaml      | 11 +++++++++++
> >>   1 file changed, 11 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
> >> index 37c3c272407d..403d6416f7ac 100644
> >> --- a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
> >> +++ b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
> >> @@ -37,6 +37,15 @@ allOf:
> >>       else:
> >>         required:
> >>           - interrupts
> >> +  - if:
> >> +      properties:
> >> +        compatible:
> >> +          contains:
> >> +            enum:
> >> +              - amd,pensando-elba-spi
> >> +    then:
> >> +      required:
> >> +        - amd,pensando-elba-syscon
> > Please add the "amd,pensando-elba-syscon" property definition as I
> > asked here:
> > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2F20220704131810.kabkuy6e4qmhfm3n%40mobilestation%2F&amp;data=05%7C01%7Cbradley.larson%40amd.com%7C1c4f822c81424048873508da839d90fc%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637967010019245894%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=xl9OU9P9QK3wLHc25hQZK393ylULd41qc4HB2Zt%2F0BQ%3D&amp;reserved=0
> 

> Proposing this addition:
> 
> --- a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
> +++ b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
> @@ -148,6 +148,15 @@ properties:
>         of the designware controller, and the upper limit is also subject to
>         controller configuration.
> 
> +  amd,pensando-elba-syscon:
> +    $ref: "/schemas/types.yaml#/definitions/phandle-array"
> +    maxItems: 1
> +    description:
> +      A phandle to syscon used to access the spi chip-select override 
> register.
> +    items:
> +      - items:
> +        - description: phandle to the syscon node
> +

No. What Krzysztof and I asked was to add the property definition
into the allOf: [ if ...,  ] statement. Please read more carefully my
last comment:
https://lore.kernel.org/lkml/20220704131810.kabkuy6e4qmhfm3n@mobilestation/
The definition is supposed to look like this:

> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: amd,pensando-elba-spi
> +    then:
  +      properties:
  +        amd,pensando-elba-syscon
  +          $ref: /schemas/types.yaml#/definitions/phandle
  +          description: AMD Pensando Elba SoC system controller
> +      required:
> +        - amd,pensando-elba-syscon

* Please also note that I've replaced "enum:" with "const:" in the if
statement above.

The difference with what you suggested is that my version is
applicable for the Pensando ELBA SPI controller only, while your
update will cause applying the "amd,pensando-elba-syscon" property
constraints for all DW SSI controllers which isn't what we would want.

-Sergey

>   patternProperties:
>     "^.*@[0-9a-f]+$":
>       type: object
> 
> Regards,
> Brad
Re: [PATCH v6 04/17] dt-bindings: spi: dw: Add AMD Pensando Elba SoC SPI Controller bindings
Posted by Larson, Bradley 3 years, 6 months ago
On 9/11/22 11:34 AM, Serge Semin wrote:
> On Wed, Aug 31, 2022 at 06:28:46PM +0000, Larson, Bradley wrote:
>> On 8/21/22 10:49 AM, Serge Semin wrote:
>>> On Sat, Aug 20, 2022 at 12:57:37PM -0700, Brad Larson wrote:
>>>> From: Brad Larson <blarson@amd.com>
>>>>
>>>> The AMD Pensando Elba SoC has integrated the DW APB SPI Controller
>>>>
>>>> Signed-off-by: Brad Larson <blarson@amd.com>
>>>> ---
>>>>    .../devicetree/bindings/spi/snps,dw-apb-ssi.yaml      | 11 +++++++++++
>>>>    1 file changed, 11 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
>>>> index 37c3c272407d..403d6416f7ac 100644
>>>> --- a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
>>>> +++ b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
>>>> @@ -37,6 +37,15 @@ allOf:
>>>>        else:
>>>>          required:
>>>>            - interrupts
>>>> +  - if:
>>>> +      properties:
>>>> +        compatible:
>>>> +          contains:
>>>> +            enum:
>>>> +              - amd,pensando-elba-spi
>>>> +    then:
>>>> +      required:
>>>> +        - amd,pensando-elba-syscon
>>> Please add the "amd,pensando-elba-syscon" property definition as I
>>> asked here:
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2F20220704131810.kabkuy6e4qmhfm3n%40mobilestation%2F&amp;data=05%7C01%7CBradley.Larson%40amd.com%7C5f61c8f8130c47fa814208da94243e17%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637985180686357705%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=uzcSBjuxs1JZFXiRGFVLGojAsipJACXEGskYdGmr7qA%3D&amp;reserved=0
>> Proposing this addition:
>>
>> --- a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
>> +++ b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
>> @@ -148,6 +148,15 @@ properties:
>>          of the designware controller, and the upper limit is also subject to
>>          controller configuration.
>>
>> +  amd,pensando-elba-syscon:
>> +    $ref: "/schemas/types.yaml#/definitions/phandle-array"
>> +    maxItems: 1
>> +    description:
>> +      A phandle to syscon used to access the spi chip-select override
>> register.
>> +    items:
>> +      - items:
>> +        - description: phandle to the syscon node
>> +
> No. What Krzysztof and I asked was to add the property definition
> into the allOf: [ if ...,  ] statement. Please read more carefully my
> last comment:
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2F20220704131810.kabkuy6e4qmhfm3n%40mobilestation%2F&amp;data=05%7C01%7CBradley.Larson%40amd.com%7C5f61c8f8130c47fa814208da94243e17%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637985180686357705%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=uzcSBjuxs1JZFXiRGFVLGojAsipJACXEGskYdGmr7qA%3D&amp;reserved=0
> The definition is supposed to look like this:
>
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            const: amd,pensando-elba-spi
>> +    then:
>    +      properties:
>    +        amd,pensando-elba-syscon
>    +          $ref: /schemas/types.yaml#/definitions/phandle
>    +          description: AMD Pensando Elba SoC system controller
>> +      required:
>> +        - amd,pensando-elba-syscon
> * Please also note that I've replaced "enum:" with "const:" in the if
> statement above.
>
> The difference with what you suggested is that my version is
> applicable for the Pensando ELBA SPI controller only, while your
> update will cause applying the "amd,pensando-elba-syscon" property
> constraints for all DW SSI controllers which isn't what we would want.


Yes, I see by moving this property into the allOf its only applicable 
for compatible "amd,pensando-elba-spi". Also changing "enum:" to 
"const:" as shown.  Yes on the DT-bindings check. Rob Herring's bot 
indicated an error but I had none in checking V6 patchset.  I did a 
dtschema update and it went from dtschema 2022.3.2 to dtschema-2022.8.3 
and will see if that is the reason.

Regards,
Brad