On 30/08/2024 10:50, Christian Marangi wrote:

On Thu, Aug 29, 2024 at 08:20:10AM +0200, Krzysztof Kozlowski wrote:

quoted

On Tue, Aug 27, 2024 at 08:29:20PM +0200, Christian Marangi wrote:

quoted

On Tue, Aug 27, 2024 at 09:35:07AM -0500, Rob Herring wrote:

quoted

On Tue, Aug 27, 2024 at 3:47 AM Lorenzo Bianconi
[off-list ref] wrote:

quoted

quoted

On Fri, Aug 23, 2024 at 11:17:34PM +0200, Lorenzo Bianconi wrote:

quoted

quoted

On Fri, Aug 23, 2024 at 05:14:30PM +0100, Conor Dooley wrote:

quoted

On Thu, Aug 22, 2024 at 10:50:52PM +0200, Benjamin Larsson wrote:

quoted

On 22/08/2024 18:06, Conor Dooley wrote:


Hi.

quoted

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@kernel.org/ (local)

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

quoted

quoted

quoted

- 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

quoted

quoted

quoted

- 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

quoted

quoted

quoted

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

quoted

quoted

- gpio: <0x1fbf0200 0xbc>

Do you have a link to the register map documentation for this hardware?

quoted

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

quoted

quoted

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

quoted

quoted

- 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