On Thu, Aug 29, 2024 at 08:20:10AM +0200, Krzysztof Kozlowski wrote: > On Tue, Aug 27, 2024 at 08:29:20PM +0200, Christian Marangi wrote: > > On Tue, Aug 27, 2024 at 09:35:07AM -0500, Rob Herring wrote: > > > On Tue, Aug 27, 2024 at 3:47 AM Lorenzo Bianconi > > > <lorenzo.bianconi83@xxxxxxxxx> wrote: > > > > > > > > > > > > > > On Fri, Aug 23, 2024 at 11:17:34PM +0200, Lorenzo Bianconi wrote: > > > > > > > On Fri, Aug 23, 2024 at 05:14:30PM +0100, Conor Dooley wrote: > > > > > > > > On Thu, Aug 22, 2024 at 10:50:52PM +0200, Benjamin Larsson wrote: > > > > > > > > > On 22/08/2024 18:06, Conor Dooley wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > Hi. > > > > > > > > > > > > > > > > > > > before looking at v1: > > > > > > > > > > I would really like to see an explanation for why this is a correct > > > > > > > > > > model of the hardware as part of the commit message. To me this screams > > > > > > > > > > syscon/MFD and instead of describing this as a child of a syscon and > > > > > > > > > > using regmap to access it you're doing whatever this is... > > > > > > > > > > > > > > > > > > Can you post a link to a good example dts that uses syscon/MFD ? > > > > > > > > > > > > > > > > > > It is not only pinctrl, pwm and gpio that are entangled in each other. A > > > > > > > > > good example would help with developing a proper implementation. > > > > > > > > > > > > > > > > Off the top of my head, no unfortunately. Maybe Rob or Krzk have a good > > > > > > > > example. I would suggest to start by looking at drivers within gpio or > > > > > > > > pinctrl that use syscon_to_regmap() where the argument is sourced from > > > > > > > > either of_node->parent or dev.parent->of_node (which you use depends on > > > > > > > > whether or not you have a child node or not). > > > > > > > > > > > > > > > > I recently had some questions myself for Rob about child nodes for mfd > > > > > > > > devices and when they were suitable to use: > > > > > > > > https://lore.kernel.org/all/20240815200003.GA2956351-robh@xxxxxxxxxx/ > > > > > > > > > > > > > > > > Following Rob's line of thought, I'd kinda expect an mfd driver to create > > > > > > > > the devices for gpio and pwm using devm_mfd_add_devices() and the > > > > > > > > pinctrl to have a child node. > > > > > > > > > > > > > > Just to not get confused and staring to focus on the wrong kind of > > > > > > > API/too complex solution, I would suggest to check the example from > > > > > > > Lorenzo. > > > > > > > > > > > > > > The pinctrl/gpio is an entire separate block and is mapped separately. > > > > > > > What is problematic is that chip SCU is a mix and address are not in > > > > > > > order and is required by many devices. (clock, pinctrl, gpio...) > > > > > > > > > > > > > > IMHO a mfd is overkill and wouldn't suite the task. MDF still support a > > > > > > > single big region and in our case we need to map 2 different one (gpio > > > > > > > AND chip SCU) (or for clock SCU and chip SCU) > > > > > > > > > > > > > > Similar problem is present in many other place and syscon is just for > > > > > > > the task. > > > > > > > > > > > > > > Simple proposed solution is: > > > > > > > - chip SCU entirely mapped and we use syscon > > > > > > > > > > That seems reasonable. > > > > > > > > ack > > > > > > > > > > > > > > > > - pinctrl mapped and reference chip SCU by phandle > > > > > > > > > > ref by phandle shouldn't be needed here, looking up by compatible should > > > > > suffice, no? > > > > > > > > ack, I think it would be fine > > > > > > > > > > > > > > > > - pwm a child of pinctrl as it's scrambled in the pinctrl mapped regs > > > > > > > > > > The pwm is not a child of the pinctrl though, they're both subfunctions of > > > > > the register region they happen to both be in. I don't agree with that > > > > > appraisal, sounds like an MFD to me. > > > > > > > > ack > > > > > > > > > > > > > > > > Hope this can clear any confusion. > > > > > > > > > > > > To clarify the hw architecture we are discussing about 3 memory regions: > > > > > > - chip_scu: <0x1fa20000 0x384> > > > > > > - scu: <0x1fb00020 0x94c> > > > > > ^ > > > > > I'm highly suspicious of a register region that begins at 0x20. What is > > > > > at 0x1fb00000? > > > > > > > > sorry, my fault > > > > > > > > > > > > > > > - gpio: <0x1fbf0200 0xbc> > > > > > > > > > > Do you have a link to the register map documentation for this hardware? > > > > > > > > > > > The memory regions above are used by the following IC blocks: > > > > > > - clock: chip_scu and scu > > > > > > > > > > What is the differentiation between these two different regions? Do they > > > > > provide different clocks? Are registers from both of them required in > > > > > order to provide particular clocks? > > > > > > > > In chip-scu and scu memory regions we have heterogeneous registers. > > > > Regarding clocks in chip-scu we have fixed clock registers (e.g. spi > > > > clock divider, switch clock source and divider, main bus clock source > > > > and divider, ...) while in scu (regarding clock configuration) we have > > > > pcie clock regs (e.g. reset and other registers). This is the reason > > > > why, in en7581-scu driver, we need both of them, but we can access > > > > chip-scu via the compatible string (please take a look at the dts > > > > below). > > > > > > > > > > > > > > > - pinctrl (io-muxing/gpio_chip/irq_chip): chip_scu and gpio > > > > > > > > > > Ditto here. Are these actually two different sets of iomuxes, or are > > > > > registers from both required to mux a particular pin? > > > > > > > > Most of the io-muxes configuration registers are in chip-scu block, > > > > just pwm ones are in gpio memory block. > > > > Gpio block is mainly used for gpio_chip and irq_chip functionalities. > > > > > > > > > > > > > > > - pwm: gpio > > > > > > > > > > > > clock and pinctrl devices share the chip_scu memory region but they need to > > > > > > access even other separated memory areas (scu and gpio respectively). > > > > > > pwm needs to just read/write few gpio registers. > > > > > > As pointed out in my previous email, we can define the chip_scu block as > > > > > > syscon node that can be accessed via phandle by clock and pinctrl drivers. > > > > > > clock driver will map scu area while pinctrl one will map gpio memory block. > > > > > > pwm can be just a child of pinctrl node. > > > > > > > > > > As I mentioned above, the last statement here I disagree with. As > > > > > someone that's currently in the process of fixing making a mess of this > > > > > exact kind of thing, I'm going to strongly advocate not taking shortcuts > > > > > like this. > > > > > > > > > > IMO, all three of these regions need to be described as syscons in some > > > > > form, how exactly it's hard to say without a better understanding of the > > > > > breakdown between regions. > > > > > > > > > > If, for example, the chip_scu only provides a few "helper" bits, I'd > > > > > expect something like > > > > > > > > > > syscon@1fa20000 { > > > > > compatible = "chip-scu", "syscon"; > > > > > reg = <0x1fa2000 0x384>; > > > > > }; > > > > > > > > > > syscon@1fb00000 { > > > > > compatible = "scu", "syscon", "simple-mfd"; > > > > > #clock-cells = 1; > > > > > }; > > > > > > > > > > syscon@1fbf0200 { > > > > > compatible = "gpio-scu", "syscon", "simple-mfd"; > > > > > #pwm-cells = 1; > > > > > > > > > > pinctrl@x { > > > > > compatible = "pinctrl"; > > > > > reg = x; > > > > > #pinctrl-cells = 1; > > > > > #gpio-cells = 1; > > > > > }; > > > > > }; > > > > > > > > > > > > > ack, so we could use the following dts nodes for the discussed memory > > > > regions (chip-scu, scu and gpio): > > > > > > > > syscon@1fa20000 { > > > > compatible = "airoha,chip-scu", "syscon"; > > > > reg = <0x0 0x1fa2000 0x0 0x384>; > > > > }; > > > > > > > > clock-controller@1fb00000 { > > > > compatible = "airoha,en7581-scu", "syscon"; > > > > reg = <0x0 0x1fb00000 0x0 0x94c>; > > > > #clock-cells = <1>; > > > > #reset-cells = <1>; > > > > }; > > > > > > > > mfd@1fbf0200 { > > > > compatible = "airoha,en7581-gpio-mfd", "simple-mfd"; > > > > reg = <0x0 0x1fbf0200 0x0 0xc0>; > > > > > > > > pio: pinctrl { > > > > compatible = "airoha,en7581-pinctrl" > > > > gpio-controller; > > > > #gpio-cells = <2>; > > > > > > > > interrupt-controller; > > > > #interrupt-cells = <2>; > > > > interrupt-parent = <&gic>; > > > > interrupts = <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>; > > > > } > > > > > > > > pwm: pwm { > > > > compatible = "airoha,en7581-pwm"; > > > > #pwm-cells = <3>; > > > > } > > > > }; > > > > > > I think this can be simplified down to this: > > > > > > mfd@1fbf0200 { > > > compatible = "airoha,en7581-gpio-mfd"; // MFD is a Linuxism. What > > > is this h/w block called? > > > reg = <0x0 0x1fbf0200 0x0 0xc0>; > > > gpio-controller; > > > #gpio-cells = <2>; > > > interrupt-controller; > > > #interrupt-cells = <2>; > > > interrupts = <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>; > > > > > > #pwm-cells = <3>; > > > > > > pio: pinctrl { > > > foo-pins {}; > > > bar-pins {}; > > > }; > > > }; > > > > > > Maybe we keep the compatible in 'pinctrl'... > > > > > > > Hi Rob, thanks a lot for the hint, I hope we can finally find a solution > > on how to implement this. > > > > In Documentation the block is called GPIO Controller. As explained it does > > expose pinctrl function AND pwm (with regs in the middle) > > > > Is this semplification really needed? It does pose some problem driver > > wise (on where to put the driver, in what subsystem) and also on the > > Sorry, but no, dt-bindings do not affect the driver at all in such way. > Nothing changes in your driver in such aspect, no dilemma where to put > it (the same place as before). > Ok, from the proposed node structure, is it problematic to move the gpio-controller and -cells in the pinctrl node? And also the pwm-cells to the pwm node? This is similar to how it's done by broadcom GPIO MFD [1] that also expose pinctrl and other device in the same register block as MFD childs. This would be the final node block. mfd@1fbf0200 { compatible = "airoha,en7581-gpio-mfd"; reg = <0x0 0x1fbf0200 0x0 0xc0>; interrupt-parent = <&gic>; interrupts = <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>; pio: pinctrl { compatible = "airoha,en7581-pinctrl"; gpio-controller; #gpio-cells = <2>; interrupt-controller; #interrupt-cells = <2>; }; pwm: pwm { compatible = "airoha,en7581-pwm"; #pwm-cells = <3>; status = "disabled"; }; }; I also link the implementation of the MFD driver [2] [1] https://elixir.bootlin.com/linux/v6.10.7/source/Documentation/devicetree/bindings/mfd/brcm,bcm6318-gpio-sysctl.yaml [2] https://github.com/Ansuel/linux/blob/airoha-mfd/drivers/mfd/airoha-en7581-gpio-mfd.c > > yaml side with mixed property for pinctrl and pwm controller. > > Hardware people mixed it, not we... > > > > > I feel mixing the 2 thing might cause some confusion on the 2 block > > device that are well separated aside from the unlucky position of the > > regs. > > I think the feedback you got is that essentially these are parts of the > same device. Are you saying that hardware is really two separate > devices? > > > > > The suggested MFD implementation would consist of > > - main node with MFD (map the entire GPIO controller regs) > > - 2 child for PWM and pinctrl (no regs) > > > > - driver in mfd/ > > - driver in pinctrl/ > > - driver in pwm/ > > This has nothing to do with bindings, except that you will need one > driver somewhere which adds child devices, but you still could do > everything as two drivers (as before). > > Anyway how to structure drivers is rarely good argument about bindings. > > > > > An alternative is the previous solution with pinctrl mapping all the > > GPIO controller regs and PWM a child but Conor suggested that a MFD > > structure might be better suited for the task. We have both implemented > > and ready to be submitted. Hope we can find a common decision on how to > > implement this simple but annoying block of devices. > > Best regards, > Krzysztof > -- Ansuel
