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

[Krzysztof Kozlowski]

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@xxxxxxx>
>>>>>>> +
>>>>>>> +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