| Message ID | 20240822-en7581-pinctrl-v2-1-ba1559173a7f@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add pinctrl support to EN7581 SoC | expand |
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
diff --git a/Documentation/devicetree/bindings/pinctrl/airoha,en7581-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/airoha,en7581-pinctrl.yaml new file mode 100644 index 000000000000..a4b58bc30416 --- /dev/null +++ b/Documentation/devicetree/bindings/pinctrl/airoha,en7581-pinctrl.yaml @@ -0,0 +1,454 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/pinctrl/airoha,en7581-pinctrl.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Airoha EN7581 Pin Controller + +maintainers: + - Lorenzo Bianconi <lorenzo@kernel.org> + +description: + The Airoha's EN7581 Pin controller is used to control SoC pins. + +allOf: + - $ref: pinctrl.yaml# + +properties: + compatible: + enum: + - airoha,en7581-pinctrl + + 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 + + reg-names: + items: + - const: iomux + - const: led-iomux + - const: gpio-flash-mode + - const: gpio-flash-mode-ext + - const: ioconf + - const: pcie-rst-od + - const: gpio-bank0 + - const: gpio-ctrl1 + - const: gpio-ctrl2 + - const: gpio-bank1 + + gpio-controller: true + + "#gpio-cells": + const: 2 + description: + Number of cells in GPIO specifier. Since the generic GPIO binding is + used, the amount of cells must be specified as 2. See the below mentioned + gpio binding representation for description of particular cells. + + gpio-ranges: + maxItems: 1 + description: + GPIO valid number range. + + interrupt-controller: true + + interrupts: + maxItems: 1 + + "#interrupt-cells": + const: 2 + +patternProperties: + '-pins$': + type: object + additionalProperties: false + + patternProperties: + '^.*mux.*$': + type: object + additionalProperties: false + description: + pinmux configuration nodes. + + $ref: /schemas/pinctrl/pinmux-node.yaml + properties: + function: + description: + A string containing the name of the function to mux to the group. + enum: [pon, tod_1pps, sipo, mdio, uart, i2c, jtag, pcm, spi, + pcm_spi, i2s, emmc, pnand, pcie_reset, pwm, phy1_led0, + phy2_led0, phy3_led0, phy4_led0, phy1_led1, phy2_led1, + phy3_led1, phy4_led1] + groups: + description: + An array of strings. Each string contains the name of a group. + required: + - function + - groups + + allOf: + - if: + properties: + function: + const: pon + then: + properties: + groups: + enum: [pon] + - if: + properties: + function: + const: tod_1pps + then: + properties: + groups: + enum: [pon_tod_1pps, gsw_tod_1pps] + - if: + properties: + function: + const: sipo + then: + properties: + groups: + enum: [sipo, sipo_rclk] + - if: + properties: + function: + const: mdio + then: + properties: + groups: + enum: [mdio] + - if: + properties: + function: + const: uart + then: + properties: + groups: + items: + enum: [uart2, uart2_cts_rts, hsuart, hsuart_cts_rts, uart4, + uart5] + maxItems: 2 + - if: + properties: + function: + const: i2c + then: + properties: + groups: + enum: [i2c1] + - if: + properties: + function: + const: jtag + then: + properties: + groups: + enum: [jtag_udi, jtag_dfd] + - if: + properties: + function: + const: pcm + then: + properties: + groups: + enum: [pcm1, pcm2] + - if: + properties: + function: + const: spi + then: + properties: + groups: + items: + enum: [spi_quad, spi_cs1] + maxItems: 2 + - if: + properties: + function: + const: pcm_spi + then: + properties: + groups: + items: + enum: [pcm_spi, pcm_spi_int, pcm_spi_rst, pcm_spi_cs1, + pcm_spi_cs2_p156, pcm_spi_cs2_p128, pcm_spi_cs3, + pcm_spi_cs4] + maxItems: 7 + - if: + properties: + function: + const: i2c + then: + properties: + groups: + enum: [i2s] + - if: + properties: + function: + const: emmc + then: + properties: + groups: + enum: [emmc] + - if: + properties: + function: + const: pnand + then: + properties: + groups: + enum: [pnand] + - if: + properties: + function: + const: pcie_reset + then: + properties: + groups: + enum: [pcie_reset0, pcie_reset1, pcie_reset2] + - if: + properties: + function: + const: pwm + then: + properties: + groups: + enum: [gpio0, gpio1, gpio2, gpio3, gpio4, gpio5, gpio6, + gpio7, gpio8, gpio9, gpio10, gpio11, gpio12, gpio13, + gpio14, gpio15, gpio16, gpio17, gpio18, gpio19, + gpio20, gpio21, gpio22, gpio23, gpio24, gpio25, + gpio26, gpio27, gpio28, gpio29, gpio30, gpio31, + gpio36, gpio37, gpio38, gpio39, gpio40, gpio41, + gpio42, gpio43, gpio44, gpio45, gpio46, gpio47] + - if: + properties: + function: + const: phy1_led0 + then: + properties: + groups: + enum: [gpio33, gpio34, gpio35, gpio42] + - if: + properties: + function: + const: phy2_led0 + then: + properties: + groups: + enum: [gpio33, gpio34, gpio35, gpio42] + - if: + properties: + function: + const: phy3_led0 + then: + properties: + groups: + enum: [gpio33, gpio34, gpio35, gpio42] + - if: + properties: + function: + const: phy4_led0 + then: + properties: + groups: + enum: [gpio33, gpio34, gpio35, gpio42] + - if: + properties: + function: + const: phy1_led1 + then: + properties: + groups: + enum: [gpio43, gpio44, gpio45, gpio46] + - if: + properties: + function: + const: phy2_led1 + then: + properties: + groups: + enum: [gpio43, gpio44, gpio45, gpio46] + - if: + properties: + function: + const: phy3_led1 + then: + properties: + groups: + enum: [gpio43, gpio44, gpio45, gpio46] + - if: + properties: + function: + const: phy4_led1 + then: + properties: + groups: + enum: [gpio43, gpio44, gpio45, gpio46] + + '^.*conf.*$': + type: object + additionalProperties: false + description: + pinconf configuration nodes. + $ref: /schemas/pinctrl/pincfg-node.yaml + + properties: + pins: + description: + An array of strings. Each string contains the name of a pin. + items: + enum: [uart1_txd, uart1_rxd, i2c_scl, i2c_sda, spi_cs0, spi_clk, + spi_mosi, spi_miso, gpio0, gpio1, gpio2, gpio3, gpio4, + gpio5, gpio6, gpio7, gpio8, gpio9, gpio10, gpio11, gpio12, + gpio13, gpio14, gpio15, gpio16, gpio17, gpio18, gpio19, + gpio20, gpio21, gpio22, gpio23, gpio24, gpio25, gpio26, + gpio27, gpio28, gpio29, gpio30, gpio31, gpio32, gpio33, + gpio34, gpio35, gpio36, gpio37, gpio38, gpio39, gpio40, + gpio41, gpio42, gpio43, gpio44, gpio45, gpio46, + pcie_reset0, pcie_reset1, pcie_reset2] + minItems: 1 + maxItems: 58 + + bias-disable: true + bias-pull-up: true + bias-pull-down: true + input-enable: true + output-enable: true + output-low: true + output-high: true + drive-open-drain: true + + drive-strength: + description: + Selects the drive strength for MIO pins, in mA. + enum: [2, 4, 6, 8] + + required: + - pins + +required: + - compatible + - reg + - reg-names + - gpio-controller + - "#gpio-cells" + +additionalProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/irq.h> + #include <dt-bindings/interrupt-controller/arm-gic.h> + + soc { + #address-cells = <2>; + #size-cells = <2>; + + pio: 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"; + + gpio-controller; + #gpio-cells = <2>; + gpio-ranges = <&pio 0 13 47>; + + interrupt-controller; + #interrupt-cells = <2>; + interrupt-parent = <&gic>; + interrupts = <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>; + + pcie1-rst-pins { + conf { + pins = "pcie_reset1"; + drive-open-drain = <1>; + }; + }; + + pwm-pins { + mux { + function = "pwm"; + groups = "gpio18"; + }; + }; + + spi-pins { + mux { + function = "spi"; + groups = "spi_quad", "spi_cs1"; + }; + }; + + uart2-pins { + mux { + function = "uart"; + groups = "uart2", "uart2_cts_rts"; + }; + }; + + uar5-pins { + mux { + function = "uart"; + groups = "uart5"; + }; + }; + + mmc-pins { + mux { + function = "emmc"; + groups = "emmc"; + }; + }; + + mdio-pins { + mux { + function = "mdio"; + groups = "mdio"; + }; + + conf { + pins = "gpio2"; + output-enable; + }; + }; + + gswp1-led0-pins { + mux { + function = "phy1_led0"; + groups = "gpio33"; + }; + }; + + gswp2-led1-pins { + mux { + function = "phy2_led1"; + groups = "gpio44"; + }; + }; + }; + };
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(+)