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