Message-ID: <20250214115410-GYA21743@gentoo>
Date: Fri, 14 Feb 2025 11:54:10 +0000
From: Yixun Lan <dlan@...too.org>
To: Linus Walleij <linus.walleij@...aro.org>
Cc: Rob Herring <robh@...nel.org>, Olof Johansson <olof@...om.net>,
	Bartosz Golaszewski <brgl@...ev.pl>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, Conor Dooley <conor@...nel.org>,
	Paul Walmsley <paul.walmsley@...ive.com>,
	Palmer Dabbelt <palmer@...belt.com>, Yangyu Chen <cyy@...self.name>,
	Jisheng Zhang <jszhang@...nel.org>,
	Jesse Taube <mr.bossman075@...il.com>,
	Inochi Amaoto <inochiama@...look.com>,
	Icenowy Zheng <uwu@...nowy.me>,
	Meng Zhang <zhangmeng.kevin@...ux.spacemit.com>,
	linux-gpio@...r.kernel.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-riscv@...ts.infradead.org
Subject: Re: [PATCH v4 1/4] dt-bindings: gpio: spacemit: add support for K1
 SoC

Hi Linus:

On 14:07 Thu 13 Feb     , Linus Walleij wrote:
> On Thu, Feb 6, 2025 at 2:32 PM Yixun Lan <dlan@...too.org> wrote:
> 
> > > > foo-gpios <&gpio 2 7 GPIO_ACTIVE_LOW>;
> >
> > if we model the dts as above, then "&gpio" will register itself as one sole "struct gpio_chip",
> >  which mean one gpio chip combine three banks..
> 
> Not really: the fact that there is just one gpio node in the device
> tree does not
> mean that it needs to correspond to one single gpio_chip instance inside the
> Linux kernel.
> 
> It's just what the current existing bindings and the code in the GPIO subsystem
> assumes. It does not have to assume that: we can change it.
> 
> I'm sorry if this is not entirely intuitive :(
> 
> One node can very well spawn three gpio_chip instances, but it requires
> some core changes. But I think it's the most elegant.
> 
> > if taking "one gpio chip support multi banks" direction, then it will be reverted back as patch V1,
> > then, even the three gpio-cells model is unnecessary needed, as we can map gpio number
> >  to the <bank, offset> array in the underlying gpio driver
> >
> > the v4 patch is very similar to drivers/gpio/gpio-dwapb.c
> >
> > If had to choose the direction between v1 and v4, I personally would favor the latter,
> >  as from hw perspective, each gpio bank is quite indepedent - has its own io/irq registers,
> >  merely has interleaved io memory space, one shared IRQ line.. also the patch v4 leverage
> >  lots underlying generic gpio APIs, result in much simplified/clean code base..
> 
> So what I would suggest is a combination of the two.
> 
> One gpio node in the device tree, like the DT maintainers want it.
> 
> Three struct gpio_chip instances inside the driver, all three spawn from
> that single gpio device, and from that single platform_device.
> 
> What we are suggesting is a three-cell phandle in the device tree:
> 
> foo-gpios = <&gpio 0 7 GPIO_ACTIVE_HIGH>;
> bar-gpios = <&gpio 2 31 GPIO_ACTIVE_HIGH>;
> 
> Notice the new first cell which is 0 or 2.
> 
> The first one is what was previously called gpio 7.
> The second one is what was 2*32+31 = gpio 95.
> 
> So internally in the driver it is easy to use the first cell (0 or 2) to map to
> the right struct gpio_chip if you have it in your driver something like this:
> 
> struct spacemit_gpio {
>     struct gpio_chip gcs[3];
> ...
> };
> 
> struct spacemit_gpio *sg;
> struct gpio_chip *gc;
> int ret;
> 
> for (i = 0; i++; i < 3) {
>      ret = devm_gpiochip_add_data(dev, &sg->gcs[i], sg);
>      if (ret)
>         return ret;
>      gc = sg->gcs[i];
>      .... do stuff with this instance ....
> }
> 
> Callbacks etc should work as before.
> 
> Then these phandles needs to be properly translated, which is done with the
> struct gpio_chip .of_xlate() callback. (If you look inside gpiolib-of.c
> you will see that chip->of_xlate() is called to map the phandle cells
> to a certain GPIO line).
> 
> In most cases, drivers do not assign the chip->of_xlate callback
> (one exception is gpio-pxa.c) and then it is default-assigned to
> of_gpio_simple_xlate() which you can find in gpiolib-of.c as well.
> 
> You need to copy this callback to your driver and augment it
> properly.
> 
> The xlate callback is used to locate the struct gpio_chip and
> struct gpio_device as well, by just calling the xlate callback, so if
> you code up the right xlate callback, everything should just
> work by itself.
> 
> this is a guess on what it would look like (just dry coding,
> but hopefully the idea works!):
> 
> static int spacemit_gpio_xlate(struct gpio_chip *gc,
>                                 const struct of_phandle_args *gpiospec,
>                                 u32 *flags)
> {
>         struct spacemit_gpio *sg = gpiochip_get_data(gc);
>         int i;
> 
>         if (gc->of_gpio_n_cells != 3)
>                 return -EINVAL;
> 
>         if (gpiospec->args_count < gc->of_gpio_n_cells)
>                 return -EINVAL;
> 
>         /* We support maximum 3 gpio_chip instances */
>         i = gpiospec->args[0];
>         if (i >= 3)
>                 return -EINVAL;
> 
>         /* OK is this the right gpio_chip out of the three ? */
>         if (gc != sg->gcs[i])
>                 return -EINVAL;
> 
>         /* Are we in range for this GPIO chip */
>         if (gpiospec->args[1] >= gc->ngpio)
>                 return -EINVAL;
> 
>         if (flags)
>                 *flags = gpiospec->args[2];
> 
>         /* Return the hw index */
>         return gpiospec->args[1];
> }
> 
thanks for this very detail prototype! it works mostly, with one problem:

how to map gpio correctly to the pin from pinctrl subsystem?

for example, I specify gpio-ranges in dts, then 
                gpio0: gpio@...19000 {
                        compatible = "spacemit,k1-gpio";
                        reg = <0x0 0xd4019000 0x0 0x100>;
			...
                        gpio-ranges = <&pinctrl 0 0 96>;
                };

		foo-gpios = <&gpio0 2 28 GPIO_ACTIVE_LOW>;

It should get GPIO_92 ( 92 = 2 * 32 + 28), but turns out GPIO_28

Probably there is something I missed...
> ...
> gc->of_gpio_n_cells = 3;
> gc->of_xlate = spacemit_gpio_xlate;
> 
> If it works as I hope, this will make the code in gpiolib-of.c in
> of_find_gpio_device_by_xlate() calling gpio_device_find()
> (which will iterate over all registered gpio_chips and then
> of_gpiochip_match_node_and_xlate() will call this custom function
> to see if it's the right one and return > 0 when we have the right
> chip.
> 
> This should work for gpios *only*. When we then come to irqs,
> these assume (see gpiolib.c) that we are using
> irq_domain_xlate_twocell() when using GPIOLIB_IRQCHIP, so
> you either need to roll your own irqchip code or we should fix
Sounds I should implement something like irq_domain_xlate_threecell()?

> the core (I can help with this if the above works).
> 
> Several gpio chips use their own domain translation outside
> of the gpiolib so you can use this as an intermediate step:
> git grep irq_domain_ops drivers/gpio/
..
> ... but if you get here, let's patch the core to deal with custom
> irqdomain xlate functions in the same manner as above.
> 
I like this direction, but how we should proceed?

> I hope this isn't terribly unclear or complicated?
> Otherwise tell me and I will try to ... explain more or give
> up and say you can use a single 96-pin gpio_chip.
> 
Let's try first, sounds it's a feasible way.

Many thanks!

> Yours,
> Linus Walleij

-- 
Yixun Lan (dlan)
Gentoo Linux Developer
GPG Key ID AABEFD55

Powered by blists - more mailing lists