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

Introduce device-tree binding documentation for Airoha EN7581 pinctrl
controller.

Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 .../bindings/pinctrl/airoha,en7581-pinctrl.yaml    | 454 +++++++++++++++++++++
 1 file changed, 454 insertions(+)

On Thu, Aug 22, 2024 at 11:40:52AM +0200, Lorenzo Bianconi wrote:
> Introduce device-tree binding documentation for Airoha EN7581 pinctrl
> controller.
> 
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> +  reg:
> +    items:
> +      - description: IOMUX base address
> +      - description: LED IOMUX base address
> +      - description: GPIO flash mode base address
> +      - description: GPIO flash mode extended base address
> +      - description: IO pin configuration base address
> +      - description: PCIE reset open-drain base address
> +      - description: GPIO bank0 register base address
> +      - description: GPIO bank0 second control register base address
> +      - description: GPIO bank1 second control register base address
> +      - description: GPIO bank1 register base address

> +      pinctrl@1fa20214 {
> +        compatible = "airoha,en7581-pinctrl";
> +        reg = <0x0 0x1fa20214 0x0 0x30>,
> +              <0x0 0x1fa2027c 0x0 0x8>,
> +              <0x0 0x1fbf0234 0x0 0x4>,
> +              <0x0 0x1fbf0268 0x0 0x4>,
> +              <0x0 0x1fa2001c 0x0 0x50>,
> +              <0x0 0x1fa2018c 0x0 0x4>,
> +              <0x0 0x1fbf0200 0x0 0x18>,
> +              <0x0 0x1fbf0220 0x0 0x4>,
> +              <0x0 0x1fbf0260 0x0 0x8>,
> +              <0x0 0x1fbf0270 0x0 0x28>;
> +        reg-names = "iomux", "led-iomux",
> +                    "gpio-flash-mode", "gpio-flash-mode-ext",
> +                    "ioconf", "pcie-rst-od",
> +                    "gpio-bank0", "gpio-ctrl1",
> +                    "gpio-ctrl2", "gpio-bank1";

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

after looking at v1:
AFAICT the PWM driver does not currently exist in mainline, so I am now
doubly of the opinion that this needs to be an MFD and a wee bit annoyed
that you didn't include any rationale in your cover letter or w/e for
not going with an MFD given there was discussion on the topic in v1.

Thanks,
Conor.

On Aug 22, Conor Dooley wrote:
> On Thu, Aug 22, 2024 at 11:40:52AM +0200, Lorenzo Bianconi wrote:
> > Introduce device-tree binding documentation for Airoha EN7581 pinctrl
> > controller.
> > 
> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > +  reg:
> > +    items:
> > +      - description: IOMUX base address
> > +      - description: LED IOMUX base address
> > +      - description: GPIO flash mode base address
> > +      - description: GPIO flash mode extended base address
> > +      - description: IO pin configuration base address
> > +      - description: PCIE reset open-drain base address
> > +      - description: GPIO bank0 register base address
> > +      - description: GPIO bank0 second control register base address
> > +      - description: GPIO bank1 second control register base address
> > +      - description: GPIO bank1 register base address
> 
> > +      pinctrl@1fa20214 {
> > +        compatible = "airoha,en7581-pinctrl";
> > +        reg = <0x0 0x1fa20214 0x0 0x30>,
> > +              <0x0 0x1fa2027c 0x0 0x8>,
> > +              <0x0 0x1fbf0234 0x0 0x4>,
> > +              <0x0 0x1fbf0268 0x0 0x4>,
> > +              <0x0 0x1fa2001c 0x0 0x50>,
> > +              <0x0 0x1fa2018c 0x0 0x4>,
> > +              <0x0 0x1fbf0200 0x0 0x18>,
> > +              <0x0 0x1fbf0220 0x0 0x4>,
> > +              <0x0 0x1fbf0260 0x0 0x8>,
> > +              <0x0 0x1fbf0270 0x0 0x28>;
> > +        reg-names = "iomux", "led-iomux",
> > +                    "gpio-flash-mode", "gpio-flash-mode-ext",
> > +                    "ioconf", "pcie-rst-od",
> > +                    "gpio-bank0", "gpio-ctrl1",
> > +                    "gpio-ctrl2", "gpio-bank1";
> 
> 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...
> 
> after looking at v1:
> AFAICT the PWM driver does not currently exist in mainline, so I am now
> doubly of the opinion that this needs to be an MFD and a wee bit annoyed
> that you didn't include any rationale in your cover letter or w/e for
> not going with an MFD given there was discussion on the topic in v1.

based on the reply from Rob I was thinking it is fine to just reduce the number
of IO mappings, sorry for that.

> 
> Thanks,
> Conor.

clock, pinctrl and pwm controllers need to map 3 main memory areas:
- chip-scu: <0x0 0x1fa20000 0x0 0x384>
  it is used by the clock driver for fixed freq clock configuration,
  by the pinctrl driver for io-muxing (and by the future pon drivers)
- scu: <0x1fb00020 0x0 0x94c>
  it is used by the clock/rst driver
- gpio: <0x1fbf0200 0x0 0xbc>
  it is used by the pinctrl driver to implement gpio/irq controller and
  by the pwm driver.

I guess we can model chip_scu as single syscon node used by clock and pinctrl
while pwm can be a child of the pinctrl node. Something like:

...

chip_scu: chip-scu@1fa20000 {
	compatible = "airoha,en7581-chip-scu", "syscon";
	reg = <0x0 0x1fa20000 0x0 0x384>;
};

scuclk: clock-controller@1fb00020 {
	compatible = "airoha,en7581-scu";
	reg = <0x1fb00020 0x0 0x94c>;
	airoha,chip-scu = <&chip_scu>;
	...
};

pio: pinctrl@1fbf0200 {
	compatible = "airoha,en7581-pinctrl", "simple-mfd", "syscon";
	reg = <0x1fbf0200 0x0 0xbc>;
	airoha,chip-scu = <&chip_scu>;
	....

	pwm {
		compatible = "airoha,en7581-pwm";
		...
	};
};

...

Does it work for you?

Regards,
Lorenzo

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.

MvH

Benjamin Larsson

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@kernel.org/
> 
> 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
- pinctrl mapped and reference chip SCU by phandle
- pwm a child of pinctrl as it's scrambled in the pinctrl mapped regs

Hope this can clear any confusion.

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

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.

> 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@kernel.org/
> > 
> > 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
> - pinctrl mapped and reference chip SCU by phandle
> - pwm a child of pinctrl as it's scrambled in the pinctrl mapped regs
> 
> Hope this can clear any confusion.
> 
> -- 
> 	Ansuel

To clarify the hw architecture we are discussing about 3 memory regions:
- chip_scu: <0x1fa20000 0x384>
- scu: <0x1fb00020 0x94c>
- gpio: <0x1fbf0200 0xbc>

The memory regions above are used by the following IC blocks:
- clock: chip_scu and scu
- pinctrl (io-muxing/gpio_chip/irq_chip): chip_scu and gpio
- 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.
What do you think about this approach? Can we address the requirements above
via classic mfd driver?

Regards,
Lorenzo

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

> > - pinctrl mapped and reference chip SCU by phandle

ref by phandle shouldn't be needed here, looking up by compatible should
suffice, no?

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

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

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

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

> - 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;
	};
};

And you could look up the chip-scu by its compatible from the clock or
pinctrl drivers. Perhaps the "helper" bits assumption is incorrect
however, and both the scu and chip scu provide independent clocks?

> What do you think about this approach? Can we address the requirements above
> via classic mfd driver?

Hi.

On 26/08/2024 19:07, Conor Dooley wrote:
> 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?

Unknown, no documentation of those registers.


>
>> - gpio: <0x1fbf0200 0xbc>
> Do you have a link to the register map documentation for this hardware?

There is no public documentation, what is available is the current 
driver source and this (less useful) partial map here:

https://github.com/gchmiel/en7512_kernel5/blob/master/linux-5-new/arch/mips/include/asm/tc3162/tc3162.h#L860

Registers with FMAP and FLAP are parts of the PWM functionality.

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

chip-scu contains the registers the clock driver handles. But scu has 
registers with the word clock in the description but both regions does 
not seem to be needed for a specific clock.

chip-scu contains pinctrl, iomux and clocks

scu contains random bits and functional muxes for serdes

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

io-muxes for pins are done in chip-scu, pwm muxing is done in the gpio 
register range together with chip-scu (ensure there are no conflicts).

MvH

Benjamin Larsson

>
> 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@kernel.org/
> > > >
> > > > 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>;
    }
};

What do you think?
If we all agree on the approach above, we can proceed with the
required changes in the clk/pinctrl and pwm drivers.

Regards,
Lorenzo

> And you could look up the chip-scu by its compatible from the clock or
> pinctrl drivers. Perhaps the "helper" bits assumption is incorrect
> however, and both the scu and chip scu provide independent clocks?
>
> > What do you think about this approach? Can we address the requirements above
> > via classic mfd driver?
>
>

On Tue, Aug 27, 2024 at 3:47 AM Lorenzo Bianconi
<lorenzo.bianconi83@gmail.com> 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@kernel.org/
> > > > >
> > > > > 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

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@gmail.com> 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@kernel.org/
> > > > > >
> > > > > > 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
yaml side with mixed property for pinctrl and pwm controller.

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.

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/

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.

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@gmail.com> 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@kernel.org/
> > > > > > >
> > > > > > > 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).

> 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

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@gmail.com> 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@kernel.org/
> > > > > > > >
> > > > > > > > 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
>

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@gmail.com> 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@kernel.org/
>>>>>>>>>
>>>>>>>>> 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

[...]

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

ack. iiuc, all the properties will be in the parent node (mfd) and we will
have just the compatible strings in the child ones, right? Something like:

		mfd@1fbf0200 {
			compatible = "airoha,en7581-gpio-mfd";
			reg = <0x0 0x1fbf0200 0x0 0xc0>;
			gpio-controller;
			#gpio-cells = <2>;

			...
			#pwm-cells = <3>;

			pio: pinctrl {
				compatible = "airoha,en7581-pinctrl";
			};

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

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

This is just a copy-paster error.

Regards,
Lorenzo

> 
> >                         };
> >                 };
> > 
> > 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
>

On Fri, Aug 30, 2024 at 12:55:32PM +0200, Lorenzo Bianconi wrote:
> [...]
> 
> > >>>
> > >>> 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...
> 
> ack. iiuc, all the properties will be in the parent node (mfd) and we will
> have just the compatible strings in the child ones, right? Something like:
> 
> 		mfd@1fbf0200 {
> 			compatible = "airoha,en7581-gpio-mfd";
> 			reg = <0x0 0x1fbf0200 0x0 0xc0>;
> 			gpio-controller;
> 			#gpio-cells = <2>;
> 
> 			...
> 			#pwm-cells = <3>;
> 
> 			pio: pinctrl {
> 				compatible = "airoha,en7581-pinctrl";
> 			};
> 
> 			pwm: pwm {
> 				compatible = "airoha,en7581-pwm";
> 			};
> 		};


Didn't Rob basically tell you how to do it earlier in the thread?
What you've got now makes no sense, the compatibles only exist in that
to probe drivers, which you can do from the mfd driver with
mfd_add_devices() or w/e that function is called.

Cheers,
Conor.

On 30/08/2024 13:01, Conor Dooley wrote:
> On Fri, Aug 30, 2024 at 12:55:32PM +0200, Lorenzo Bianconi wrote:
>> [...]
>>
>>>>>>
>>>>>> 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...
>>
>> ack. iiuc, all the properties will be in the parent node (mfd) and we will
>> have just the compatible strings in the child ones, right? Something like:
>>
>> 		mfd@1fbf0200 {
>> 			compatible = "airoha,en7581-gpio-mfd";
>> 			reg = <0x0 0x1fbf0200 0x0 0xc0>;
>> 			gpio-controller;
>> 			#gpio-cells = <2>;
>>
>> 			...
>> 			#pwm-cells = <3>;
>>
>> 			pio: pinctrl {
>> 				compatible = "airoha,en7581-pinctrl";
>> 			};
>>
>> 			pwm: pwm {
>> 				compatible = "airoha,en7581-pwm";
>> 			};
>> 		};
> 
> 
> Didn't Rob basically tell you how to do it earlier in the thread?
> What you've got now makes no sense, the compatibles only exist in that
> to probe drivers, which you can do from the mfd driver with
> mfd_add_devices() or w/e that function is called.

Yep, we are making circles.

I will repeat:
"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."

Best regards,
Krzysztof