Re: [PATCH v2 1/2] dt-bindings: pinctrl: airoha: Add EN7581 pinctrl controller

[Krzysztof Kozlowski <krzk@xxxxxxxxxx>]

On 30/08/2024 10:50, Christian Marangi wrote:
> On Thu, Aug 29, 2024 at 08:20:10AM +0200, Krzysztof Kozlowski wrote:
>> On Tue, Aug 27, 2024 at 08:29:20PM +0200, Christian Marangi wrote:
>>> On Tue, Aug 27, 2024 at 09:35:07AM -0500, Rob Herring wrote:
>>>> On Tue, Aug 27, 2024 at 3:47 AM Lorenzo Bianconi
>>>> <lorenzo.bianconi83@xxxxxxxxx> wrote:
>>>>>
>>>>>>
>>>>>> On Fri, Aug 23, 2024 at 11:17:34PM +0200, Lorenzo Bianconi wrote:
>>>>>>>> On Fri, Aug 23, 2024 at 05:14:30PM +0100, Conor Dooley wrote:
>>>>>>>>> On Thu, Aug 22, 2024 at 10:50:52PM +0200, Benjamin Larsson wrote:
>>>>>>>>>> On 22/08/2024 18:06, Conor Dooley wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Hi.
>>>>>>>>>>
>>>>>>>>>>> before looking at v1:
>>>>>>>>>>> I would really like to see an explanation for why this is a correct
>>>>>>>>>>> model of the hardware as part of the commit message. To me this screams
>>>>>>>>>>> syscon/MFD and instead of describing this as a child of a syscon and
>>>>>>>>>>> using regmap to access it you're doing whatever this is...
>>>>>>>>>>
>>>>>>>>>> Can you post a link to a good example dts that uses syscon/MFD ?
>>>>>>>>>>
>>>>>>>>>> It is not only pinctrl, pwm and gpio that are entangled in each other. A
>>>>>>>>>> good example would help with developing a proper implementation.
>>>>>>>>>
>>>>>>>>> Off the top of my head, no unfortunately. Maybe Rob or Krzk have a good
>>>>>>>>> example. I would suggest to start by looking at drivers within gpio or
>>>>>>>>> pinctrl that use syscon_to_regmap() where the argument is sourced from
>>>>>>>>> either of_node->parent or dev.parent->of_node (which you use depends on
>>>>>>>>> whether or not you have a child node or not).
>>>>>>>>>
>>>>>>>>> I recently had some questions myself for Rob about child nodes for mfd
>>>>>>>>> devices and when they were suitable to use:
>>>>>>>>> https://lore.kernel.org/all/20240815200003.GA2956351-robh@xxxxxxxxxx/
>>>>>>>>>
>>>>>>>>> Following Rob's line of thought, I'd kinda expect an mfd driver to create
>>>>>>>>> the devices for gpio and pwm using devm_mfd_add_devices() and the
>>>>>>>>> pinctrl to have a child node.
>>>>>>>>
>>>>>>>> Just to not get confused and staring to focus on the wrong kind of
>>>>>>>> API/too complex solution, I would suggest to check the example from
>>>>>>>> Lorenzo.
>>>>>>>>
>>>>>>>> The pinctrl/gpio is an entire separate block and is mapped separately.
>>>>>>>> What is problematic is that chip SCU is a mix and address are not in
>>>>>>>> order and is required by many devices. (clock, pinctrl, gpio...)
>>>>>>>>
>>>>>>>> IMHO a mfd is overkill and wouldn't suite the task. MDF still support a
>>>>>>>> single big region and in our case we need to map 2 different one (gpio
>>>>>>>> AND chip SCU) (or for clock SCU and chip SCU)
>>>>>>>>
>>>>>>>> Similar problem is present in many other place and syscon is just for
>>>>>>>> the task.
>>>>>>>>
>>>>>>>> Simple proposed solution is:
>>>>>>>> - chip SCU entirely mapped and we use syscon
>>>>>>
>>>>>> That seems reasonable.
>>>>>
>>>>> ack
>>>>>
>>>>>>
>>>>>>>> - pinctrl mapped and reference chip SCU by phandle
>>>>>>
>>>>>> ref by phandle shouldn't be needed here, looking up by compatible should
>>>>>> suffice, no?
>>>>>
>>>>> ack, I think it would be fine
>>>>>
>>>>>>
>>>>>>>> - pwm a child of pinctrl as it's scrambled in the pinctrl mapped regs
>>>>>>
>>>>>> The pwm is not a child of the pinctrl though, they're both subfunctions of
>>>>>> the register region they happen to both be in. I don't agree with that
>>>>>> appraisal, sounds like an MFD to me.
>>>>>
>>>>> ack
>>>>>
>>>>>>
>>>>>>>> Hope this can clear any confusion.
>>>>>>>
>>>>>>> To clarify the hw architecture we are discussing about 3 memory regions:
>>>>>>> - chip_scu: <0x1fa20000 0x384>
>>>>>>> - scu: <0x1fb00020 0x94c>
>>>>>>                   ^
>>>>>> I'm highly suspicious of a register region that begins at 0x20. What is
>>>>>> at 0x1fb00000?
>>>>>
>>>>> sorry, my fault
>>>>>
>>>>>>
>>>>>>> - gpio: <0x1fbf0200 0xbc>
>>>>>>
>>>>>> Do you have a link to the register map documentation for this hardware?
>>>>>>
>>>>>>> The memory regions above are used by the following IC blocks:
>>>>>>> - clock: chip_scu and scu
>>>>>>
>>>>>> What is the differentiation between these two different regions? Do they
>>>>>> provide different clocks? Are registers from both of them required in
>>>>>> order to provide particular clocks?
>>>>>
>>>>> In chip-scu and scu memory regions we have heterogeneous registers.
>>>>> Regarding clocks in chip-scu we have fixed clock registers (e.g. spi
>>>>> clock divider, switch clock source and divider, main bus clock source
>>>>> and divider, ...) while in scu (regarding clock configuration) we have
>>>>> pcie clock regs (e.g. reset and other registers). This is the reason
>>>>> why, in en7581-scu driver, we need both of them, but we can access
>>>>> chip-scu via the compatible string (please take a look at the dts
>>>>> below).
>>>>>
>>>>>>
>>>>>>> - pinctrl (io-muxing/gpio_chip/irq_chip): chip_scu and gpio
>>>>>>
>>>>>> Ditto here. Are these actually two different sets of iomuxes, or are
>>>>>> registers from both required to mux a particular pin?
>>>>>
>>>>> Most of the io-muxes configuration registers are in chip-scu block,
>>>>> just pwm ones are in gpio memory block.
>>>>> Gpio block is mainly used for gpio_chip and irq_chip functionalities.
>>>>>
>>>>>>
>>>>>>> - pwm: gpio
>>>>>>>
>>>>>>> clock and pinctrl devices share the chip_scu memory region but they need to
>>>>>>> access even other separated memory areas (scu and gpio respectively).
>>>>>>> pwm needs to just read/write few gpio registers.
>>>>>>> As pointed out in my previous email, we can define the chip_scu block as
>>>>>>> syscon node that can be accessed via phandle by clock and pinctrl drivers.
>>>>>>> clock driver will map scu area while pinctrl one will map gpio memory block.
>>>>>>> pwm can be just a child of pinctrl node.
>>>>>>
>>>>>> As I mentioned above, the last statement here I disagree with. As
>>>>>> someone that's currently in the process of fixing making a mess of this
>>>>>> exact kind of thing, I'm going to strongly advocate not taking shortcuts
>>>>>> like this.
>>>>>>
>>>>>> IMO, all three of these regions need to be described as syscons in some
>>>>>> form, how exactly it's hard to say without a better understanding of the
>>>>>> breakdown between regions.
>>>>>>
>>>>>> If, for example, the chip_scu only provides a few "helper" bits, I'd
>>>>>> expect something like
>>>>>>
>>>>>> syscon@1fa20000 {
>>>>>>         compatible = "chip-scu", "syscon";
>>>>>>         reg = <0x1fa2000 0x384>;
>>>>>> };
>>>>>>
>>>>>> syscon@1fb00000 {
>>>>>>         compatible = "scu", "syscon", "simple-mfd";
>>>>>>         #clock-cells = 1;
>>>>>> };
>>>>>>
>>>>>> syscon@1fbf0200 {
>>>>>>         compatible = "gpio-scu", "syscon", "simple-mfd";
>>>>>>         #pwm-cells = 1;
>>>>>>
>>>>>>         pinctrl@x {
>>>>>>                 compatible = "pinctrl";
>>>>>>                 reg = x;
>>>>>>                 #pinctrl-cells = 1;
>>>>>>                 #gpio-cells = 1;
>>>>>>         };
>>>>>> };
>>>>>>
>>>>>
>>>>> ack, so we could use the following dts nodes for the discussed memory
>>>>> regions (chip-scu, scu and gpio):
>>>>>
>>>>> syscon@1fa20000 {
>>>>>     compatible = "airoha,chip-scu", "syscon";
>>>>>     reg = <0x0 0x1fa2000 0x0 0x384>;
>>>>> };
>>>>>
>>>>> clock-controller@1fb00000 {
>>>>>     compatible = "airoha,en7581-scu", "syscon";
>>>>>     reg = <0x0 0x1fb00000 0x0 0x94c>;
>>>>>     #clock-cells = <1>;
>>>>>     #reset-cells = <1>;
>>>>> };
>>>>>
>>>>> mfd@1fbf0200 {
>>>>>     compatible = "airoha,en7581-gpio-mfd", "simple-mfd";
>>>>>     reg = <0x0 0x1fbf0200 0x0 0xc0>;
>>>>>
>>>>>     pio: pinctrl {
>>>>>         compatible = "airoha,en7581-pinctrl"
>>>>>         gpio-controller;
>>>>>         #gpio-cells = <2>;
>>>>>
>>>>>         interrupt-controller;
>>>>>         #interrupt-cells = <2>;
>>>>>         interrupt-parent = <&gic>;
>>>>>         interrupts = <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>;
>>>>>     }
>>>>>
>>>>>     pwm: pwm {
>>>>>         compatible = "airoha,en7581-pwm";
>>>>>         #pwm-cells = <3>;
>>>>>     }
>>>>> };
>>>>
>>>> I think this can be simplified down to this:
>>>>
>>>> mfd@1fbf0200 {
>>>>     compatible = "airoha,en7581-gpio-mfd";  // MFD is a Linuxism. What
>>>> is this h/w block called?
>>>>     reg = <0x0 0x1fbf0200 0x0 0xc0>;
>>>>     gpio-controller;
>>>>     #gpio-cells = <2>;
>>>>     interrupt-controller;
>>>>     #interrupt-cells = <2>;
>>>>     interrupts = <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>;
>>>>
>>>>     #pwm-cells = <3>;
>>>>
>>>>     pio: pinctrl {
>>>>         foo-pins {};
>>>>         bar-pins {};
>>>>     };
>>>> };
>>>>
>>>> Maybe we keep the compatible in 'pinctrl'...
>>>>
>>>
>>> Hi Rob, thanks a lot for the hint, I hope we can finally find a solution
>>> on how to implement this.
>>>
>>> In Documentation the block is called GPIO Controller. As explained it does
>>> expose pinctrl function AND pwm (with regs in the middle)
>>>
>>> Is this semplification really needed? It does pose some problem driver
>>> wise (on where to put the driver, in what subsystem) and also on the
>>
>> Sorry, but no, dt-bindings do not affect the driver at all in such way.
>> Nothing changes in your driver in such aspect, no dilemma where to put
>> it (the same place as before).
>>
> 
> Ok, from the proposed node structure, is it problematic to move the
> gpio-controller and -cells in the pinctrl node? And also the pwm-cells
> to the pwm node?

The move is just unnecessary and not neat. You design DTS based on your
drivers architecture and this is exactly what we want to avoid.

> This is similar to how it's done by broadcom GPIO MFD [1] that also

There are 'reg' fields, which is the main problem here. I don't like
that arguments because it entirely misses the discussions - about that
binding or other bindings - happening prior to merge.

> expose pinctrl and other device in the same register block as MFD
> childs.
> 
> This would be the final node block.
> 
>                 mfd@1fbf0200 {
>                         compatible = "airoha,en7581-gpio-mfd";
>                         reg = <0x0 0x1fbf0200 0x0 0xc0>;
> 
>                         interrupt-parent = <&gic>;
>                         interrupts = <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>;
> 
>                         pio: pinctrl {
>                                 compatible = "airoha,en7581-pinctrl";
> 
>                                 gpio-controller;
>                                 #gpio-cells = <2>;
> 
>                                 interrupt-controller;
>                                 #interrupt-cells = <2>;

No resources here...

>                         };
> 
>                         pwm: pwm {
>                                 compatible = "airoha,en7581-pwm";
> 
>                                 #pwm-cells = <3>;
>                                 status = "disabled";

And why is it disabled? No external resources. There is no benefit of
this node.

>                         };
>                 };
> 
> I also link the implementation of the MFD driver [2]
> 
> [1] https://elixir.bootlin.com/linux/v6.10.7/source/Documentation/devicetree/bindings/mfd/brcm,bcm6318-gpio-sysctl.yaml
> [2] https://github.com/Ansuel/linux/blob/airoha-mfd/drivers/mfd/airoha-en7581-gpio-mfd.c


Best regards,
Krzysztof