| Message ID | 20230306040739.51488-6-blarson@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Support AMD Pensando Elba SoC | expand |
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
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 @@ -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: | + 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: + enum: + - amd,pensando-ctrl + + reg: + minItems: 1 + + '#reset-cells': + const: 1 + + interrupts: + maxItems: 1 + + spi-max-frequency: true + +required: + - compatible + - spi-max-frequency + - '#reset-cells' + +unevaluatedProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/arm-gic.h> + + spi { + #address-cells = <1>; + #size-cells = <0>; + num-cs = <4>; + + 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>; + }; + }; + +...
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