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