| Message ID | 20220820195750.70861-7-brad@pensando.io |
|---|---|
| State | New |
| Headers | show |
| Series | Support AMD Pensando Elba SoC | expand |
On Sat, 20 Aug 2022 12:57:39 -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 > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: dtschema/dtc warnings/errors: ./Documentation/devicetree/bindings/mfd/amd,pensando-elbasr.yaml: Unable to find schema file matching $id: http://devicetree.org/schemas/reset/amd,pensando-elbasr-reset.yaml /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/mfd/amd,pensando-elbasr.example.dtb: system-controller@0: reset-controller@0: False schema does not allow {'compatible': ['amd,pensando-elbasr-reset'], 'reg': [[0]], '#reset-cells': [[1]]} From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/mfd/amd,pensando-elbasr.yaml Documentation/devicetree/bindings/mfd/amd,pensando-elbasr.example.dtb:0:0: /example-0/spi/system-controller@0/reset-controller@0: failed to match any schema with compatible: ['amd,pensando-elbasr-reset'] doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/patch/ This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit.
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 8/22/22 7:25 AM, Rob Herring wrote: > 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: https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevicetree.org%2Fschemas%2Fmfd%2Famd%2Cpensando-elbasr.yaml%23&data=05%7C01%7CBradley.Larson%40amd.com%7Cd02c183f9a29492180fb08da844a3458%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637967751571358185%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=WHkA6tPbaDanQuMSaAiWUG3fBTfDVlWXMdeaO5t%2F3Ok%3D&reserved=0 >> +$schema: https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevicetree.org%2Fmeta-schemas%2Fcore.yaml%23&data=05%7C01%7CBradley.Larson%40amd.com%7Cd02c183f9a29492180fb08da844a3458%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637967751571358185%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=FDig31luqeo4pOZXfuOAGiOLi0kVqFU8ExnBi5gorlY%3D&reserved=0 >> + >> +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. > Removed mention of the dev interfaces >> + 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. >> + >> + 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 list_for_each_entry(r, &reset_controller_list, list) { if (args.np == r->of_node) { rcdev = r; break; } } where in sdhci_cdns_probe(...) this lookup fails priv->rst_hw = devm_reset_control_get_optional_exclusive(dev, "hw"); which results in a non-functioning mmc hardware reset. >> + #reset-cells = <1>; >> + }; >> + }; >> + >> + i2c1: i2c@1 { >> + compatible = "amd,pensando-elbasr"; > You can't reuse the same compatible to represent different things. Changed to system-controller@1 and adjusted description above >> + 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. > Same as above, changed this to system-controller@2 >> + 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? A userspace utility understands how to program this internal flash. Changed to system-controller@3 Regards, Brad
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. > >>> + >>> + 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? Best regards, Krzysztof
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 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. 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. Regards, Brad
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 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: >>>>>>> + 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 ? I repeat - why? >> >>> 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 Does not matter really... We do not talk about driver, but about hardware and bindings. > which creates /dev/pensr0.<cs> for spi chip-selects defined in the > parent node as: Wait, can we skip the driver entirely? I am not reviewing your driver and what it creates under /dev. > > 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 Again, why does it matter for the bindings? > 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. You did not provide reason. You described Linux driver implementation which we do not talk about. We talk about bindings, which are not really related to implementation (at least in most cases). > 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", > }, I know Linux drivers. No need to paste them here. They are unrelated to this talk. > }; > > 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>; > }; > }; 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? This however does not answer my questions before.... You keep ignoring them. So please answer yes or no: "Are there other sub-devices?" " and your binding is incomplete?" 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)?" Because from what you wrote you should just put reset-cells in the parent... Best regards, Krzysztof
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. > 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 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..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>; + }; + }; + +...