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

[Christian Marangi <ansuelsmth@xxxxxxxxx>]

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?
This is similar to how it's done by broadcom GPIO MFD [1] that also
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>;
                        };

                        pwm: pwm {
                                compatible = "airoha,en7581-pwm";

                                #pwm-cells = <3>;
                                status = "disabled";
                        };
                };

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

> > yaml side with mixed property for pinctrl and pwm controller.
> 
> Hardware people mixed it, not we...
> 
> > 
> > I feel mixing the 2 thing might cause some confusion on the 2 block
> > device that are well separated aside from the unlucky position of the
> > regs.
> 
> I think the feedback you got is that essentially these are parts of the
> same device. Are you saying that hardware is really two separate
> devices?
> 
> > 
> > The suggested MFD implementation would consist of
> > - main node with MFD (map the entire GPIO controller regs)
> > -   2 child for PWM and pinctrl (no regs)
> > 
> > - driver in mfd/
> > - driver in pinctrl/
> > - driver in pwm/
> 
> This has nothing to do with bindings, except that you will need one
> driver somewhere which adds child devices, but you still could do
> everything as two drivers (as before).
> 
> Anyway how to structure drivers is rarely good argument about bindings.
> 
> > 
> > An alternative is the previous solution with pinctrl mapping all the
> > GPIO controller regs and PWM a child but Conor suggested that a MFD
> > structure might be better suited for the task. We have both implemented
> > and ready to be submitted. Hope we can find a common decision on how to
> > implement this simple but annoying block of devices.
> 
> Best regards,
> Krzysztof
> 

-- 
	Ansuel