[v10,05/15] dt-bindings: soc: amd: amd,pensando-elbasr: Add AMD Pensando SoC System Controller

  Support the AMD Pensando SoC Controller which is a SPI connected device
providing a miscellaneous set of essential board control/status registers.
This device is present in all Pensando SoC based designs.

Signed-off-by: Brad Larson <blarson@amd.com>
---

v10 changes:
- Property renamed to amd,pensando-ctrl
- Driver is renamed and moved to soc/drivers/amd affecting binding
- Delete cs property, driver handles device node creation from parent num-cs
  fixing schema reg error in a different way

v9 changes:
- Instead of four nodes, one per chip-select, a single
  node is used with reset-cells in the parent.
- No MFD API is used anymore in the driver so it made
  sense to move this to drivers/spi.
- This driver is common for all Pensando SoC based designs
  so changed the name to pensando-sr.c to not make it Elba
  SoC specific.
- Added property cs for the chip-select number which is used
  by the driver to create /dev/pensr0.<cs>

---
 .../bindings/soc/amd/amd,pensando-ctrl.yaml   | 60 +++++++++++++++++++
 1 file changed, 60 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/amd/amd,pensando-ctrl.yaml
  

On 06/03/2023 05:07, Brad Larson wrote:
> Support the AMD Pensando SoC Controller which is a SPI connected device
> providing a miscellaneous set of essential board control/status registers.
> This device is present in all Pensando SoC based designs.
> 
> Signed-off-by: Brad Larson <blarson@amd.com>
> ---
> 
> v10 changes:
> - Property renamed to amd,pensando-ctrl
> - Driver is renamed and moved to soc/drivers/amd affecting binding
> - Delete cs property, driver handles device node creation from parent num-cs
>   fixing schema reg error in a different way
> 
> v9 changes:
> - Instead of four nodes, one per chip-select, a single
>   node is used with reset-cells in the parent.
> - No MFD API is used anymore in the driver so it made
>   sense to move this to drivers/spi.
> - This driver is common for all Pensando SoC based designs
>   so changed the name to pensando-sr.c to not make it Elba
>   SoC specific.
> - Added property cs for the chip-select number which is used
>   by the driver to create /dev/pensr0.<cs>
> 
> ---
>  .../bindings/soc/amd/amd,pensando-ctrl.yaml   | 60 +++++++++++++++++++
>  1 file changed, 60 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/soc/amd/amd,pensando-ctrl.yaml
> 
> diff --git a/Documentation/devicetree/bindings/soc/amd/amd,pensando-ctrl.yaml b/Documentation/devicetree/bindings/soc/amd/amd,pensando-ctrl.yaml
> new file mode 100644
> index 000000000000..36694077b2e6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/amd/amd,pensando-ctrl.yaml

Your subject suggests this is pensando-elbasr but you write everywhere
pensando-ctrl. Confusing. Pick one.

> @@ -0,0 +1,60 @@
> +# SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/soc/amd/amd,pensando-ctrl.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: AMD Pensando SoC Controller
> +
> +description: |

No need for |

> +  The AMD Pensando SoC Controller is a SPI connected device with essential
> +  control/status registers accessed on chip select 0.  This device is present
> +  in all Pensando SoC based designs.
> +
> +maintainers:
> +  - Brad Larson <blarson@amd.com>
> +
> +properties:
> +  compatible:
> +    contains:

Drop 'contains'. That's not a correct syntax here.

> +      enum:
> +        - amd,pensando-ctrl
> +
> +  reg:
> +    minItems: 1

maxItems instead

> +
> +  '#reset-cells':
> +    const: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  spi-max-frequency: true

Drop, not needed.

> +
> +required:
> +  - compatible
> +  - spi-max-frequency
> +  - '#reset-cells'

allOf with ref to spi-peripheral-props.yaml

> +
> +unevaluatedProperties: false

This is not correct without allOf (should be additionalProperties if you
are not using allOf), which leads you to the missing allOf.

> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +    spi {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +        num-cs = <4>;

Drop num-cs, not important in this context.

> +
> +        system-controller@0 {
> +            compatible = "amd,pensando-ctrl";
> +            reg = <0>;
> +            spi-max-frequency = <12000000>;
> +            interrupt-parent = <&porta>;
> +            interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
> +            #reset-cells = <1>;
> +        };
> +    };
> +
> +...

Best regards,
Krzysztof
  

On 06/03/2023 09:35, Krzysztof Kozlowski wrote:
> On 06/03/2023 05:07, Brad Larson wrote:
>> Support the AMD Pensando SoC Controller which is a SPI connected device
>> providing a miscellaneous set of essential board control/status registers.
>> This device is present in all Pensando SoC based designs.
>>
>> Signed-off-by: Brad Larson <blarson@amd.com>
>> ---
>>
>> v10 changes:
>> - Property renamed to amd,pensando-ctrl
>> - Driver is renamed and moved to soc/drivers/amd affecting binding
>> - Delete cs property, driver handles device node creation from parent num-cs
>>   fixing schema reg error in a different way
>>
>> v9 changes:
>> - Instead of four nodes, one per chip-select, a single
>>   node is used with reset-cells in the parent.
>> - No MFD API is used anymore in the driver so it made
>>   sense to move this to drivers/spi.
>> - This driver is common for all Pensando SoC based designs
>>   so changed the name to pensando-sr.c to not make it Elba
>>   SoC specific.
>> - Added property cs for the chip-select number which is used
>>   by the driver to create /dev/pensr0.<cs>
>>
>> ---
>>  .../bindings/soc/amd/amd,pensando-ctrl.yaml   | 60 +++++++++++++++++++
>>  1 file changed, 60 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/soc/amd/amd,pensando-ctrl.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/soc/amd/amd,pensando-ctrl.yaml b/Documentation/devicetree/bindings/soc/amd/amd,pensando-ctrl.yaml
>> new file mode 100644
>> index 000000000000..36694077b2e6
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/soc/amd/amd,pensando-ctrl.yaml
> 
> Your subject suggests this is pensando-elbasr but you write everywhere
> pensando-ctrl. Confusing. Pick one.

Actually pensando-ctrl is for sure not correct, because it misses the
name of the SoC (you call it everywhere "elba").

Best regards,
Krzysztof
  

On 06/03/2023 8:35, Krzysztof Kozlowski wrote:
> On 06/03/2023 05:07, Brad Larson wrote:
>> Support the AMD Pensando SoC Controller which is a SPI connected device
>> providing a miscellaneous set of essential board control/status registers.
>> This device is present in all Pensando SoC based designs.
>> 
>> Signed-off-by: Brad Larson <blarson@amd.com>
>> ---
>> 
>> v10 changes:
>> - Property renamed to amd,pensando-ctrl
>> - Driver is renamed and moved to soc/drivers/amd affecting binding
>> - Delete cs property, driver handles device node creation from parent num-cs
>>   fixing schema reg error in a different way
>> 
>> v9 changes:
>> - Instead of four nodes, one per chip-select, a single
>>   node is used with reset-cells in the parent.
>> - No MFD API is used anymore in the driver so it made
>>   sense to move this to drivers/spi.
>> - This driver is common for all Pensando SoC based designs
>>   so changed the name to pensando-sr.c to not make it Elba
>>   SoC specific.
>> - Added property cs for the chip-select number which is used
>>   by the driver to create /dev/pensr0.<cs>
>> 
>> ---
>>  .../bindings/soc/amd/amd,pensando-ctrl.yaml   | 60 +++++++++++++++++++
>>  1 file changed, 60 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/soc/amd/amd,pensando-ctrl.yaml
>> 
>> diff --git a/Documentation/devicetree/bindings/soc/amd/amd,pensando-ctrl.yaml b/Documentation/devicetree/bindings/soc/amd/amd,pensando-ctrl.yaml
>> new file mode 100644
>> index 000000000000..36694077b2e6
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/soc/amd/amd,pensando-ctrl.yaml
>
> Your subject suggests this is pensando-elbasr but you write everywhere
> pensando-ctrl. Confusing. Pick one.

I'll fix the commit message.  This driver is common across multiple Pensando SoCs
and embedding elba in the name is misleading which is why I changed it to pensando
controller (pensando-ctrl).  Sorry for the churn.

>> @@ -0,0 +1,60 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/soc/amd/amd,pensando-ctrl.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: AMD Pensando SoC Controller
>> +
>> +description: |
>
> No need for |

Removed |

>> +  The AMD Pensando SoC Controller is a SPI connected device with essential
>> +  control/status registers accessed on chip select 0.  This device is present
>> +  in all Pensando SoC based designs.
>> +
>> +maintainers:
>> +  - Brad Larson <blarson@amd.com>
>> +
>> +properties:
>> +  compatible:
>> +    contains:
>
> Drop 'contains'. That's not a correct syntax here.
>

Removed contains and looks like this now:

properties:
  compatible:
    enum:
      - amd,pensando-ctrl

>> +      enum:
>> +        - amd,pensando-ctrl
>> +
>> +  reg:
>> +    minItems: 1
>
> maxItems instead

Changed to maxItems: 1

>> +
>> +  '#reset-cells':
>> +    const: 1
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  spi-max-frequency: true
>
> Drop, not needed.
>

Removed spi-max-frequency and included alOff w/spi-peripheral-props.yaml

>> +
>> +required:
>> +  - compatible
>> +  - spi-max-frequency
>> +  - '#reset-cells'
>
> allOf with ref to spi-peripheral-props.yaml
>
>> +
>> +unevaluatedProperties: false
>
> This is not correct without allOf (should be additionalProperties if you
> are not using allOf), which leads you to the missing allOf.

Thanks for pointing out use of spi-peripheral-props.yaml, looks like this now

+required:
+  - compatible
+  - reg
+  - '#reset-cells'
+
+allOf:
+  - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+unevaluatedProperties: false


>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>> +
>> +    spi {
>> +        #address-cells = <1>;
>> +        #size-cells = <0>;
>> +        num-cs = <4>;
>
> Drop num-cs, not important in this context.

Removed num-cs

>> +
>> +        system-controller@0 {
>> +            compatible = "amd,pensando-ctrl";
>> +            reg = <0>;
>> +            spi-max-frequency = <12000000>;
>> +            interrupt-parent = <&porta>;
>> +            interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
>> +            #reset-cells = <1>;
>> +        };
>> +    };
>> +
>> +...

Regards,
Brad
  

On 06/03/2023 09:35, Krzysztof Kozlowski wrote:
>> On 06/03/2023 05:07, Brad Larson wrote:
>>> Support the AMD Pensando SoC Controller which is a SPI connected device
>>> providing a miscellaneous set of essential board control/status registers.
>>> This device is present in all Pensando SoC based designs.
>>>
>>> Signed-off-by: Brad Larson <blarson@amd.com>
>>> ---
>>>
>>> v10 changes:
>>> - Property renamed to amd,pensando-ctrl
>>> - Driver is renamed and moved to soc/drivers/amd affecting binding
>>> - Delete cs property, driver handles device node creation from parent num-cs
>>>   fixing schema reg error in a different way
>>>
>>> v9 changes:
>>> - Instead of four nodes, one per chip-select, a single
>>>   node is used with reset-cells in the parent.
>>> - No MFD API is used anymore in the driver so it made
>>>   sense to move this to drivers/spi.
>>> - This driver is common for all Pensando SoC based designs
>>>   so changed the name to pensando-sr.c to not make it Elba
>>>   SoC specific.
>>> - Added property cs for the chip-select number which is used
>>>   by the driver to create /dev/pensr0.<cs>
>>>
>>> ---
>>>  .../bindings/soc/amd/amd,pensando-ctrl.yaml   | 60 +++++++++++++++++++
>>>  1 file changed, 60 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/soc/amd/amd,pensando-ctrl.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/soc/amd/amd,pensando-ctrl.yaml b/Documentation/devicetree/bindings/soc/amd/amd,pensando-ctrl.yaml
>>> new file mode 100644
>>> index 000000000000..36694077b2e6
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/soc/amd/amd,pensando-ctrl.yaml
>> 
>> Your subject suggests this is pensando-elbasr but you write everywhere
>> pensando-ctrl. Confusing. Pick one.
>
> Actually pensando-ctrl is for sure not correct, because it misses the
> name of the SoC (you call it everywhere "elba").

The reason I dropped elba as part of the name is this driver and its associated 
SPI attached device (cpld or fpga depending on the board design) will be used
across a series of SoCs starting with Elba.  Implying its Elba specific is misleading.

Regards,
Brad
  

On 07/03/2023 03:18, Brad Larson wrote:
> On 06/03/2023 09:35, Krzysztof Kozlowski wrote:
>>> On 06/03/2023 05:07, Brad Larson wrote:
>>>> Support the AMD Pensando SoC Controller which is a SPI connected device
>>>> providing a miscellaneous set of essential board control/status registers.
>>>> This device is present in all Pensando SoC based designs.
>>>>
>>>> Signed-off-by: Brad Larson <blarson@amd.com>
>>>> ---
>>>>
>>>> v10 changes:
>>>> - Property renamed to amd,pensando-ctrl
>>>> - Driver is renamed and moved to soc/drivers/amd affecting binding
>>>> - Delete cs property, driver handles device node creation from parent num-cs
>>>>   fixing schema reg error in a different way
>>>>
>>>> v9 changes:
>>>> - Instead of four nodes, one per chip-select, a single
>>>>   node is used with reset-cells in the parent.
>>>> - No MFD API is used anymore in the driver so it made
>>>>   sense to move this to drivers/spi.
>>>> - This driver is common for all Pensando SoC based designs
>>>>   so changed the name to pensando-sr.c to not make it Elba
>>>>   SoC specific.
>>>> - Added property cs for the chip-select number which is used
>>>>   by the driver to create /dev/pensr0.<cs>
>>>>
>>>> ---
>>>>  .../bindings/soc/amd/amd,pensando-ctrl.yaml   | 60 +++++++++++++++++++
>>>>  1 file changed, 60 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/soc/amd/amd,pensando-ctrl.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/soc/amd/amd,pensando-ctrl.yaml b/Documentation/devicetree/bindings/soc/amd/amd,pensando-ctrl.yaml
>>>> new file mode 100644
>>>> index 000000000000..36694077b2e6
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/soc/amd/amd,pensando-ctrl.yaml
>>>
>>> Your subject suggests this is pensando-elbasr but you write everywhere
>>> pensando-ctrl. Confusing. Pick one.
>>
>> Actually pensando-ctrl is for sure not correct, because it misses the
>> name of the SoC (you call it everywhere "elba").
> 
> The reason I dropped elba as part of the name is this driver and its associated 
> SPI attached device (cpld or fpga depending on the board design) will be used
> across a series of SoCs starting with Elba.  Implying its Elba specific is misleading.

Compatibles must be specific.
https://elixir.bootlin.com/linux/v6.1-rc1/source/Documentation/devicetree/bindings/writing-bindings.rst#L42
If this is SoC part it must match SoC. What is misleading in this? Why
Pensando is different than all other SoCs (I am really getting tired
everytime asking why people think their solution is special)?

If this is not part of the SoC, then your commit msg is misleading.
Maybe bindings as well, so rework it.

Best regards,
Krzysztof
  

On 06/09/2023 09:46, Krzysztof Kozlowski wrote:
> On 07/03/2023 03:18, Brad Larson wrote:
>> On 06/03/2023 09:35, Krzysztof Kozlowski wrote:
>>>> On 06/03/2023 05:07, Brad Larson wrote:
>>>>> Support the AMD Pensando SoC Controller which is a SPI connected device
>>>>> providing a miscellaneous set of essential board control/status registers.
>>>>> This device is present in all Pensando SoC based designs.
>>>>>
...
>>>>>  .../bindings/soc/amd/amd,pensando-ctrl.yaml   | 60 +++++++++++++++++++
>>>>>  1 file changed, 60 insertions(+)
>>>>>  create mode 100644 Documentation/devicetree/bindings/soc/amd/amd,pensando-ctrl.yaml
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/soc/amd/amd,pensando-ctrl.yaml b/Documentation/devicetree/bindings/soc/amd/amd,pensando-ctrl.yaml
>>>>> new file mode 100644
>>>>> index 000000000000..36694077b2e6
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/soc/amd/amd,pensando-ctrl.yaml
>>>>
>>>> Your subject suggests this is pensando-elbasr but you write everywhere
>>>> pensando-ctrl. Confusing. Pick one.
>>>
>>> Actually pensando-ctrl is for sure not correct, because it misses the
>>> name of the SoC (you call it everywhere "elba").
>> 
>> The reason I dropped elba as part of the name is this driver and its associated 
>> SPI attached device (cpld or fpga depending on the board design) will be used
>> across a series of SoCs starting with Elba.  Implying its Elba specific is misleading.
>
> Compatibles must be specific.
> https://elixir.bootlin.com/linux/v6.1-rc1/source/Documentation/devicetree/bindings/writing-bindings.rst#L42
> If this is SoC part it must match SoC. What is misleading in this? Why
> Pensando is different than all other SoCs (I am really getting tired
> everytime asking why people think their solution is special)?
>
> If this is not part of the SoC, then your commit msg is misleading.
> Maybe bindings as well, so rework it.

Yes, changed it back to 'amd,pensando-elba-ctrl' and fixed the dts, driver and commit message.

Regards,
Brad