| Message ID | 20221116193940.67445-1-blarson@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
Hi Brad, On Thu, Nov 17, 2022 at 7:39 PM Larson, Bradley <Bradley.Larson@amd.com> wrote: > > > > > --- /dev/null > > > > > +++ b/Documentation/devicetree/bindings/mfd/amd,pensando-elbasr.yaml > > > ... > > > > > + > > > > > +title: AMD Pensando Elba SoC Resource Controller > > > > > + > > > > > +description: | > > > > > + AMD Pensando Elba SoC Resource Controller functions are > > > > > + accessed with four chip-selects. Reset control is on CS0. > > > > > > > > One device with 4 chip-selects? Then I'd expect 'reg = <0 1 2 3>;' > > > > > > > > Hard to say more because I don't have the whole thread nor remember what > > > > exactly we discussed before. That was 100s of bindings ago... > > > > > > I agree and the example for v7 had all 4 chip-selects shown. > > > > That is not what I said. Look at 'reg' above again. You have 1 device, > > but you have 4 nodes which looks like separate 4 devices. The > > exception would be if what's on each chip select is independent from > > each other. > > > > Describe what your h/w has/is/does so we can provide better guidance. > > This is the hardware design for every implementation with the Elba SoC > > Elba <-- spi --> One FPGA or CPLD (4 chip-selects) > > where there are four functions in the FPGA accessed by userspace applications except for emmc reset control which is a bit in a CS0 register. The IP at CS0 is a bunch of miscellaneous mgmt registers. CS1 is a designware I2C master/slave. CS2 is a Lattice dual I2C master. CS3 is internal storage for the CPLD or FPGA depending on the hardware implementation. > > 'reg = <0 1 2 3>' in the dt fragment below is indicating the chip-select which is what other bindings appear to be doing. Maybe one answer is delete this and add our compatible to spidev.c in the patchset we provide to customers. Adding our compatible to spidev.c was nack'ed. Recommendation? The fragment below does not have "reg = <0 1 2 3>", but individual subnodes, each with their own "reg" property. > Not including 'reg' results in this compile warning: > > DTC arch/arm64/boot/dts/amd/elba-asic.dtb > arch/arm64/boot/dts/amd/elba-asic-common.dtsi:73.28-78.4: Warning (spi_bus_reg): /soc/spi@2800/system-controller@0: missing or empty reg property > arch/arm64/boot/dts/amd/elba-asic-common.dtsi:80.22-84.4: Warning (spi_bus_reg): /soc/spi@2800/system-controller@1: missing or empty reg property > arch/arm64/boot/dts/amd/elba-asic-common.dtsi:86.22-92.4: Warning (spi_bus_reg): /soc/spi@2800/system-controller@2: missing or empty reg property > arch/arm64/boot/dts/amd/elba-asic-common.dtsi:94.22-98.4: Warning (spi_bus_reg): /soc/spi@2800/system-controller@3: missing or empty reg property > CALL scripts/atomic/check-atomics.sh > > &emmc { > bus-width = <8>; > cap-mmc-hw-reset; > reset-names = "hw"; > resets = <&rstc 0>; > status = "okay"; > }; > > &spi0 { > #address-cells = <1>; > #size-cells = <0>; > num-cs = <4>; > cs-gpios = <0>, <0>, <&porta 1 GPIO_ACTIVE_LOW>, > <&porta 7 GPIO_ACTIVE_LOW>; > status = "okay"; > > rstc: system-controller@0 { > compatible = "amd,pensando-elbasr"; > reg = <0>; > spi-max-frequency = <12000000>; > #reset-cells = <1>; > }; > > system-controller@1 { > compatible = "amd,pensando-elbasr"; > reg = <1>; > spi-max-frequency = <12000000>; > }; > > system-controller@2 { > compatible = "amd,pensando-elbasr"; > reg = <2>; > spi-max-frequency = <12000000>; > interrupt-parent = <&porta>; > interrupts = <0 IRQ_TYPE_LEVEL_LOW>; > }; > > system-controller@3 { > compatible = "amd,pensando-elbasr"; > reg = <3>; > spi-max-frequency = <12000000>; > }; The above describes 4 individual but further identical (they have the same compatible value) devices, while above you say they are not identical: The IP at CS0 is a bunch of miscellaneous mgmt registers. CS1 is a designware I2C master/slave. CS2 is a Lattice dual I2C master. CS3 is internal storage for the CPLD or FPGA depending on the hardware implementation. So either this should be modelled as a single subnode with 4 chip selects[*]: system-controller@0 { compatible = "amd,pensando-elbasr"; reg = <0 1 2 3 4>; spi-max-frequency = <12000000>; #reset-cells = <1>; }; or as 4 separate subnodes, each using 4 different compatible values. Giving the wildly different functionalities provided by each, you also need 4 binding documents. [*] I'm not sure the Linux SPI core actually supports this yet. > }; Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
[AMD Official Use Only - General] Hi Geert, From: Geert Uytterhoeven <geert@linux-m68k.org> Sent: Friday, November 18, 2022 12:42 AM > ... > > &spi0 { > > #address-cells = <1>; > > #size-cells = <0>; > > num-cs = <4>; > > cs-gpios = <0>, <0>, <&porta 1 GPIO_ACTIVE_LOW>, > > <&porta 7 GPIO_ACTIVE_LOW>; > > status = "okay"; > > > > rstc: system-controller@0 { > > compatible = "amd,pensando-elbasr"; > > reg = <0>; > > spi-max-frequency = <12000000>; > > #reset-cells = <1>; > > }; > > > > system-controller@1 { > > compatible = "amd,pensando-elbasr"; > > reg = <1>; > > spi-max-frequency = <12000000>; > > }; > > > > system-controller@2 { > > compatible = "amd,pensando-elbasr"; > > reg = <2>; > > spi-max-frequency = <12000000>; > > interrupt-parent = <&porta>; > > interrupts = <0 IRQ_TYPE_LEVEL_LOW>; > > }; > > > > system-controller@3 { > > compatible = "amd,pensando-elbasr"; > > reg = <3>; > > spi-max-frequency = <12000000>; > > }; > > The above describes 4 individual but further identical (they have the > same compatible value) devices, while above you say they are not identical: > > The IP at CS0 is a bunch of miscellaneous mgmt registers. > CS1 is a designware I2C master/slave. > CS2 is a Lattice dual I2C master. > CS3 is internal storage for the CPLD or FPGA depending on the > hardware implementation. > > So either this should be modelled as a single subnode with 4 chip > selects[*]: > > system-controller@0 { > compatible = "amd,pensando-elbasr"; > reg = <0 1 2 3 4>; > spi-max-frequency = <12000000>; > #reset-cells = <1>; > }; > > or as 4 separate subnodes, each using 4 different compatible values. > Giving the wildly different functionalities provided by each, you also need > 4 binding documents. > > [*] I'm not sure the Linux SPI core actually supports this yet. Thanks, I was unfamiliar with reg <0 ...> with differences between the nodes such as reset-cells for one and interrupts on another. Below dts boots ok and will change binding and driver as needed. &spi0 { #address-cells = <1>; #size-cells = <0>; num-cs = <4>; cs-gpios = <0>, <0>, <&porta 1 GPIO_ACTIVE_LOW>, <&porta 7 GPIO_ACTIVE_LOW>; status = "okay"; rstc: system-controller@0 { compatible = "amd,pensando-elbasr"; reg = <0 1 2 3>; spi-max-frequency = <12000000>; interrupt-parent = <&porta>; interrupts = <0 IRQ_TYPE_LEVEL_LOW>; #reset-cells = <1>; }; }; Regards, Brad
diff --git a/Documentation/devicetree/bindings/mfd/amd,pensando-elbasr.yaml b/Documentation/devicetree/bindings/mfd/amd,pensando-elbasr.yaml new file mode 100644 index 000000000000..622c93402a86 --- /dev/null +++ b/Documentation/devicetree/bindings/mfd/amd,pensando-elbasr.yaml @@ -0,0 +1,60 @@ +# SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/mfd/amd,pensando-elbasr.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: AMD Pensando Elba SoC Resource Controller + +description: | + AMD Pensando Elba SoC Resource Controller functions are + accessed with four chip-selects. Reset control is on CS0. + +maintainers: + - Brad Larson <blarson@amd.com> + +properties: + compatible: + enum: + - amd,pensando-elbasr + + "#reset-cells": + const: 1 + + interrupts: + maxItems: 1 + +required: + - compatible + - reg + - spi-max-frequency + +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>; + + system-controller@0 { + compatible = "amd,pensando-elbasr"; + reg = <0>; + spi-max-frequency = <12000000>; + #reset-cells = <1>; + }; + + system-controller@2 { + compatible = "amd,pensando-elbasr"; + reg = <2>; + spi-max-frequency = <12000000>; + interrupt-parent = <&porta>; + interrupts = <0 IRQ_TYPE_LEVEL_LOW>; + }; + };
Add support for the AMD Pensando Elba SoC System Resource chip using the SPI interface. Signed-off-by: Brad Larson <blarson@amd.com> --- v8: - Apply review request changes and picked the two unique examples for the 4 chip-selects as one has the reset control support and the other an interrupt. Missed the --in-reply-to in git send-email for v7, included in this update. v7: - Use system-controller for the device with four chip-selects connected over spi. - Delete child by moving reset-controller into the parent. - Updated and used dtschema-2022.11 and yamllint-1.28.0 v6: - Expand description, rename nodes and change compatible usage v5: - Change to AMD Pensando instead of Pensando v4: - Change Maintained to Supported .../bindings/mfd/amd,pensando-elbasr.yaml | 60 +++++++++++++++++++ 1 file changed, 60 insertions(+) create mode 100644 Documentation/devicetree/bindings/mfd/amd,pensando-elbasr.yaml