On Sat, Aug 20, 2022 at 12:57:39PM -0700, Brad Larson wrote:
> From: Brad Larson <[email protected]>
>
> 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 <[email protected]>
> ---
> .../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 <[email protected]>
> +
> +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 <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>> .../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 <[email protected]>
>> +
>> +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 <[email protected]>
>>> +
>>> +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 <[email protected]>
>>>> +
>>>> +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 <[email protected]>
>>>>> +
>>>>> +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 <[email protected]>
>>>>>> +
>>>>>> +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 <[email protected]>
>>>>>>> +
>>>>>>> +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
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