Re: [PATCH v6 06/17] dt-bindings: mfd: amd,pensando-elbasr: Add AMD Pensando Elba System Resource chip

["Larson, Bradley" <Bradley.Larson@amd.com>]

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