| Message ID | 20220820195750.70861-7-brad@pensando.io |
|---|---|
| State | New |
| Headers | show |
| Series | Support AMD Pensando Elba SoC | expand |
On Sat, Aug 20, 2022 at 12:57:39PM -0700, Brad Larson wrote: > From: Brad Larson <blarson@amd.com> > > Add support for the AMD Pensando Elba SoC System Resource chip > using the SPI interface. The Elba SR is a Multi-function Device > supporting device register access using CS0, smbus interface for > FRU and board peripherals using CS1, dual Lattice I2C masters for > transceiver management using CS2, and CS3 for flash access. > > Signed-off-by: Brad Larson <blarson@amd.com> > --- > .../bindings/mfd/amd,pensando-elbasr.yaml | 97 +++++++++++++++++++ > 1 file changed, 97 insertions(+) > create mode 100644 Documentation/devicetree/bindings/mfd/amd,pensando-elbasr.yaml > > 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..ded347c3352c > --- /dev/null > +++ b/Documentation/devicetree/bindings/mfd/amd,pensando-elbasr.yaml > @@ -0,0 +1,97 @@ > +# 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 bindings > + > +description: | > + AMD Pensando Elba SoC Resource Controller is a set of > + miscellaneous control/status registers accessed on CS0, > + a designware i2c master/slave on CS1, a Lattice rd1173 > + dual i2c master on CS2, and flash on CS3. The /dev interfaces > + created are /dev/pensr0.<CS>. Hardware reset of the eMMC /dev is a Linux thing and not relevant for the bindings. > + is implemented by a sub-device reset-controller which accesses > + a CS0 control register. > + > +maintainers: > + - Brad Larson <blarson@amd.com> > + > +properties: > + compatible: > + items: > + - enum: > + - amd,pensando-elbasr > + > + spi-max-frequency: > + description: Maximum SPI frequency of the device in Hz. No need for generic descriptions of common properties. > + > + reg: > + maxItems: 1 > + > + '#address-cells': > + const: 1 > + > + '#size-cells': > + const: 0 > + > + interrupts: > + maxItems: 1 > + > +required: > + - compatible > + - reg > + - spi-max-frequency > + > +patternProperties: > + '^reset-controller@[a-f0-9]+$': > + $ref: /schemas/reset/amd,pensando-elbasr-reset.yaml > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/interrupt-controller/arm-gic.h> > + > + spi { > + #address-cells = <1>; > + #size-cells = <0>; > + num-cs = <4>; > + > + sysc: system-controller@0 { > + compatible = "amd,pensando-elbasr"; > + reg = <0>; > + #address-cells = <1>; > + #size-cells = <0>; > + spi-max-frequency = <12000000>; > + > + rstc: reset-controller@0 { > + compatible = "amd,pensando-elbasr-reset"; > + reg = <0>; What does 0 represent here? A register address within 'elbasr' device? Why do you need a child node for this? Are there other sub-devices and your binding is incomplete? Just put '#reset-cells' in the parent. > + #reset-cells = <1>; > + }; > + }; > + > + i2c1: i2c@1 { > + compatible = "amd,pensando-elbasr"; You can't reuse the same compatible to represent different things. > + reg = <1>; > + spi-max-frequency = <12000000>; > + }; > + > + i2c2: i2c@2 { > + compatible = "amd,pensando-elbasr"; As this is a Lattice RD1173, I would expect a compatible based on that. > + reg = <2>; > + spi-max-frequency = <12000000>; > + interrupt-parent = <&porta>; > + interrupts = <0 IRQ_TYPE_LEVEL_LOW>; > + }; > + > + flash@3 { > + compatible = "amd,pensando-elbasr"; Isn't this a flash device? > + reg = <3>; > + spi-max-frequency = <12000000>; > + }; > + }; > + > +... > -- > 2.17.1 > >
On 01/09/2022 22:37, Larson, Bradley wrote: > On 9/1/22 12:20 AM, Krzysztof Kozlowski wrote: >> On 01/09/2022 02:01, Larson, Bradley wrote: >>>>> + is implemented by a sub-device reset-controller which accesses >>>>> + a CS0 control register. >>>>> + >>>>> +maintainers: >>>>> + - Brad Larson <blarson@amd.com> >>>>> + >>>>> +properties: >>>>> + compatible: >>>>> + items: >>>>> + - enum: >>>>> + - amd,pensando-elbasr >>>>> + >>>>> + spi-max-frequency: >>>>> + description: Maximum SPI frequency of the device in Hz. >>>> No need for generic descriptions of common properties. >>> Changed to "spi-max-frequency: true" and moved to end of properties. >> Then you should rather reference spi-peripheral-props just like other >> SPI devices. > > > Will look at this dependent on the result of below > > >>>>> + >>>>> + reg: >>>>> + maxItems: 1 >>>>> + >>>>> + '#address-cells': >>>>> + const: 1 >>>>> + >>>>> + '#size-cells': >>>>> + const: 0 >>>>> + >>>>> + interrupts: >>>>> + maxItems: 1 >>>>> + >>>>> +required: >>>>> + - compatible >>>>> + - reg >>>>> + - spi-max-frequency >>>>> + >>>>> +patternProperties: >>>>> + '^reset-controller@[a-f0-9]+$': >>>>> + $ref: /schemas/reset/amd,pensando-elbasr-reset.yaml >>>>> + >>>>> +additionalProperties: false >>>>> + >>>>> +examples: >>>>> + - | >>>>> + #include <dt-bindings/interrupt-controller/arm-gic.h> >>>>> + >>>>> + spi { >>>>> + #address-cells = <1>; >>>>> + #size-cells = <0>; >>>>> + num-cs = <4>; >>>>> + >>>>> + sysc: system-controller@0 { >>>>> + compatible = "amd,pensando-elbasr"; >>>>> + reg = <0>; >>>>> + #address-cells = <1>; >>>>> + #size-cells = <0>; >>>>> + spi-max-frequency = <12000000>; >>>>> + >>>>> + rstc: reset-controller@0 { >>>>> + compatible = "amd,pensando-elbasr-reset"; >>>>> + reg = <0>; >>>> What does 0 represent here? A register address within 'elbasr' device? >>> Removed, I recall a check threw a warning or error without reg. >>> >>>> Why do you need a child node for this? Are there other sub-devices and >>>> your binding is incomplete? Just put '#reset-cells' in the parent. >>> Without a reset-controller node and booting the function >>> __of_reset_control_get(...) fails to find a match in the list here >> That's not actually the answer to the question. There was no concerns >> whether you need or not reset controller. The question was why do you >> need a child device instead of elbasr being the reset controller. >> >> Your answer does not cover this at all, so again - why do you need a >> child for this? >> > > If the parent becomes a reset-controller compatible with > "amd,pensando-elbasr-reset" then the /dev node will not be created Why /dev node will not be created? How bindings affect having or not having something in /dev ? > as there is no match to "amd,pensando-elbasr" for cs0. For cs0 internal > to linux the reset-controller manages one register bit to hardware reset > the mmc device. A userspace application opens the device node to manage > transceiver leds, system leds, reporting temps to host, other reset > controls, etc. Looking at future requirements there likely will be other > child devices. You mean "amd,pensando-elbasr" will instantiate some more devices? Why you cannot add the binding for them now? This is actually important because earlier we agreed you remove unit address from children. > > Going down this path with one compatible should reset-elbasr.c just be > deleted and fold it into the parent driver pensando-elbasr.c? Then I > wonder if it even belongs in drivers/mfd and should just be modified > and put in drivers/spi. How is it related to bindings? Best regards, Krzysztof
On 9/8/22 4:27 AM, Krzysztof Kozlowski wrote: > On 01/09/2022 22:37, Larson, Bradley wrote: >> On 9/1/22 12:20 AM, Krzysztof Kozlowski wrote: >>> On 01/09/2022 02:01, Larson, Bradley wrote: >>>>>> + is implemented by a sub-device reset-controller which accesses >>>>>> + a CS0 control register. >>>>>> + >>>>>> +maintainers: >>>>>> + - Brad Larson <blarson@amd.com> >>>>>> + >>>>>> +properties: >>>>>> + compatible: >>>>>> + items: >>>>>> + - enum: >>>>>> + - amd,pensando-elbasr >>>>>> + >>>>>> + spi-max-frequency: >>>>>> + description: Maximum SPI frequency of the device in Hz. >>>>> No need for generic descriptions of common properties. >>>> Changed to "spi-max-frequency: true" and moved to end of properties. >>> Then you should rather reference spi-peripheral-props just like other >>> SPI devices. >> >> Will look at this dependent on the result of below >> >> >>>>>> + >>>>>> + reg: >>>>>> + maxItems: 1 >>>>>> + >>>>>> + '#address-cells': >>>>>> + const: 1 >>>>>> + >>>>>> + '#size-cells': >>>>>> + const: 0 >>>>>> + >>>>>> + interrupts: >>>>>> + maxItems: 1 >>>>>> + >>>>>> +required: >>>>>> + - compatible >>>>>> + - reg >>>>>> + - spi-max-frequency >>>>>> + >>>>>> +patternProperties: >>>>>> + '^reset-controller@[a-f0-9]+$': >>>>>> + $ref: /schemas/reset/amd,pensando-elbasr-reset.yaml >>>>>> + >>>>>> +additionalProperties: false >>>>>> + >>>>>> +examples: >>>>>> + - | >>>>>> + #include <dt-bindings/interrupt-controller/arm-gic.h> >>>>>> + >>>>>> + spi { >>>>>> + #address-cells = <1>; >>>>>> + #size-cells = <0>; >>>>>> + num-cs = <4>; >>>>>> + >>>>>> + sysc: system-controller@0 { >>>>>> + compatible = "amd,pensando-elbasr"; >>>>>> + reg = <0>; >>>>>> + #address-cells = <1>; >>>>>> + #size-cells = <0>; >>>>>> + spi-max-frequency = <12000000>; >>>>>> + >>>>>> + rstc: reset-controller@0 { >>>>>> + compatible = "amd,pensando-elbasr-reset"; >>>>>> + reg = <0>; >>>>> What does 0 represent here? A register address within 'elbasr' device? >>>> Removed, I recall a check threw a warning or error without reg. >>>> >>>>> Why do you need a child node for this? Are there other sub-devices and >>>>> your binding is incomplete? Just put '#reset-cells' in the parent. >>>> Without a reset-controller node and booting the function >>>> __of_reset_control_get(...) fails to find a match in the list here >>> That's not actually the answer to the question. There was no concerns >>> whether you need or not reset controller. The question was why do you >>> need a child device instead of elbasr being the reset controller. >>> >>> Your answer does not cover this at all, so again - why do you need a >>> child for this? >>> >> If the parent becomes a reset-controller compatible with >> "amd,pensando-elbasr-reset" then the /dev node will not be created > Why /dev node will not be created? How bindings affect having or not > having something in /dev ? > >> as there is no match to "amd,pensando-elbasr" for cs0. For cs0 internal >> to linux the reset-controller manages one register bit to hardware reset >> the mmc device. A userspace application opens the device node to manage >> transceiver leds, system leds, reporting temps to host, other reset >> controls, etc. Looking at future requirements there likely will be other >> child devices. > You mean "amd,pensando-elbasr" will instantiate some more devices? Why > you cannot add the binding for them now? This is actually important > because earlier we agreed you remove unit address from children. > >> Going down this path with one compatible should reset-elbasr.c just be >> deleted and fold it into the parent driver pensando-elbasr.c? Then I >> wonder if it even belongs in drivers/mfd and should just be modified >> and put in drivers/spi. > How is it related to bindings? The compatible "amd,pensando-elbasr" is matched in drivers/mfd/pensando-elbasr.c which creates /dev/pensr0.<cs> for spi chip-selects defined in the parent node as: num-cs = <4>; cs-gpios = <0>, <0>, <&porta 1 GPIO_ACTIVE_LOW>, <&porta 7 GPIO_ACTIVE_LOW>; The compatible "amd,pensando-elbasr-reset" is in drivers/reset/reset-elbasr.c which uses regmap to control one bit in the function at cs0 to hardware reset the eMMC. This is the reason for the reset-controller child and the two driver files. The probe in drivers/mfd/pensando-elbasr.c is called 4 times, once per spi chip-select and for cs0 mfd_add_devices() is called for the reset controller. I'll change "rstc: reset-controller@0" to "rstc: reset-controller". Maybe I've gotten off track following what looked like an appropriate model to follow ("altr,a10sr") that was initially added in 2017 and has the same device topology which is SoC <= spi => CPLD/FPGA where a10sr has a 3rd driver file for a gpio controller resulting in two child nodes. In our case its not one function its four in the device where the function at chip-select 0 is where the hardware team provided the bit to control eMMC hardware reset. Is this a bad approach to follow and if so please recommend an acceptable approach. Also, "amd,pensando-elbasr" will not instantiate more devices. Snippets below for a10sr in linux-next: Device on other end of spi, one chip select, two child devices and 3 driver files in mfd, reset, and gpio. FILE: arch/arm/boot/dts/socfpga_arria10_socdk.dtsi: &spi1 { status = "okay"; resource-manager@0 { compatible = "altr,a10sr"; reg = <0>; spi-max-frequency = <100000>; /* low-level active IRQ at GPIO1_5 */ interrupt-parent = <&portb>; interrupts = <5 IRQ_TYPE_LEVEL_LOW>; interrupt-controller; #interrupt-cells = <2>; a10sr_gpio: gpio-controller { compatible = "altr,a10sr-gpio"; gpio-controller; #gpio-cells = <2>; }; a10sr_rst: reset-controller { compatible = "altr,a10sr-reset"; #reset-cells = <1>; }; }; }; FILE: drivers/mfd/altera-a10sr.c (parent) static const struct mfd_cell altr_a10sr_subdev_info[] = { { .name = "altr_a10sr_gpio", .of_compatible = "altr,a10sr-gpio", }, { .name = "altr_a10sr_reset", .of_compatible = "altr,a10sr-reset", }, }; static const struct of_device_id altr_a10sr_spi_of_match[] = { { .compatible = "altr,a10sr" }, { }, }; FILE: drivers/reset/reset-a10sr.c (reset driver) static const struct of_device_id a10sr_reset_of_match[] = { { .compatible = "altr,a10sr-reset" }, { }, }; FILE: ./drivers/gpio/gpio-altera-a10sr.c (gpio driver) static const struct of_device_id altr_a10sr_gpio_of_match[] = { { .compatible = "altr,a10sr-gpio" }, { }, }; In comparision, the pensando device is also on the other end of spi, four chip selects with /dev created for each for userspace control, and one child device on cs0 for hw reset emmc that the Linux block layer controls (single bit managed only by kernel). Pensando: &spi0 { num-cs = <4>; cs-gpios = <0>, <0>, <&porta 1 GPIO_ACTIVE_LOW>, <&porta 7 GPIO_ACTIVE_LOW>; status = "okay"; system-controller@0 { compatible = "amd,pensando-elbasr"; reg = <0>; #address-cells = <1>; #size-cells = <0>; spi-max-frequency = <12000000>; rstc: reset-controller { compatible = "amd,pensando-elbasr-reset"; #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>; }; }; Regards, Brad
On 30/09/2022 00:50, Larson, Bradley wrote: > On 9/16/22 2:56 AM, Krzysztof Kozlowski wrote: >> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding. >> >> >> On 13/09/2022 22:57, Larson, Bradley wrote: >>> On 9/8/22 4:27 AM, Krzysztof Kozlowski wrote: >>>> On 01/09/2022 22:37, Larson, Bradley wrote: >>>>> On 9/1/22 12:20 AM, Krzysztof Kozlowski wrote: >>>>>> On 01/09/2022 02:01, Larson, Bradley wrote: >>>>>> >> Wait, can we skip the driver entirely? I am not reviewing your driver >> and what it creates under /dev. > > Yes, see precise answer requested below. > >>> In comparision, the pensando device is also on the other end of spi, >>> four chip selects with /dev created for each for userspace control, >>> and one child device on cs0 for hw reset emmc that the Linux block >>> layer controls (single bit managed only by kernel). >>> >>> Pensando: >>> &spi0 { >>> num-cs = <4>; >>> cs-gpios = <0>, <0>, <&porta 1 GPIO_ACTIVE_LOW>, >>> <&porta 7 GPIO_ACTIVE_LOW>; >>> status = "okay"; >>> system-controller@0 { >>> compatible = "amd,pensando-elbasr"; >>> reg = <0>; >>> #address-cells = <1>; >>> #size-cells = <0>; >>> spi-max-frequency = <12000000>; >>> >>> rstc: reset-controller { >>> compatible = "amd,pensando-elbasr-reset"; >>> #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>; >>> }; >>> }; >> You replied with quite a response of which 90% is unrelated talk about >> driver. Please be specific. We talk here only about hardware. >> Your last DTS might be the answer, but you never explicitly wrote >> it... So let's check if I understand it correctly. Only some of elbasr >> block contain reset control? > > Yes, only the elbasr block accessed on CS0 provides reset control. The > other 3 blocks don't have any reset control and never will. I see, that could explain the subnode. However: 1. You still do not use any resources in the subnode (it does not have any in DT). 2. Your driver instantiates subdevice not based on existing of subnode or characteristics of a device (e.g. compatible), but on hard-coded chip-select line. The reset driver directly takes parent's regmap - no other resources. Therefore this does not look like dedicated piece of hardware and should be just part of parent node. > >> This however does not answer my questions before.... You keep ignoring >> them. So please answer yes or no: "Are there other sub-devices?" > > No > >> " and your binding is incomplete?" > > No > >> and a new question: "Is reset block (amd,pensando-elbasr-reset) >> re-usable so it will appear in different device (not in >> amd,pensando-elbasr)?" > > No its not re-usable So squash it with parent node. Best regards, Krzysztof
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..ded347c3352c --- /dev/null +++ b/Documentation/devicetree/bindings/mfd/amd,pensando-elbasr.yaml @@ -0,0 +1,97 @@ +# 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 bindings + +description: | + AMD Pensando Elba SoC Resource Controller is a set of + miscellaneous control/status registers accessed on CS0, + a designware i2c master/slave on CS1, a Lattice rd1173 + dual i2c master on CS2, and flash on CS3. The /dev interfaces + created are /dev/pensr0.<CS>. Hardware reset of the eMMC + is implemented by a sub-device reset-controller which accesses + a CS0 control register. + +maintainers: + - Brad Larson <blarson@amd.com> + +properties: + compatible: + items: + - enum: + - amd,pensando-elbasr + + spi-max-frequency: + description: Maximum SPI frequency of the device in Hz. + + reg: + maxItems: 1 + + '#address-cells': + const: 1 + + '#size-cells': + const: 0 + + interrupts: + maxItems: 1 + +required: + - compatible + - reg + - spi-max-frequency + +patternProperties: + '^reset-controller@[a-f0-9]+$': + $ref: /schemas/reset/amd,pensando-elbasr-reset.yaml + +additionalProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/arm-gic.h> + + spi { + #address-cells = <1>; + #size-cells = <0>; + num-cs = <4>; + + sysc: system-controller@0 { + compatible = "amd,pensando-elbasr"; + reg = <0>; + #address-cells = <1>; + #size-cells = <0>; + spi-max-frequency = <12000000>; + + rstc: reset-controller@0 { + compatible = "amd,pensando-elbasr-reset"; + reg = <0>; + #reset-cells = <1>; + }; + }; + + i2c1: i2c@1 { + compatible = "amd,pensando-elbasr"; + reg = <1>; + spi-max-frequency = <12000000>; + }; + + i2c2: i2c@2 { + compatible = "amd,pensando-elbasr"; + reg = <2>; + spi-max-frequency = <12000000>; + interrupt-parent = <&porta>; + interrupts = <0 IRQ_TYPE_LEVEL_LOW>; + }; + + flash@3 { + compatible = "amd,pensando-elbasr"; + reg = <3>; + spi-max-frequency = <12000000>; + }; + }; + +...