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

[Rob Herring <robh@xxxxxxxxxx>]

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'...

Rob