Message-ID: <Z4kUWxR9VWkzQ9aW@smile.fi.intel.com>
Date: Thu, 16 Jan 2025 16:14:51 +0200
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Thomas Richard <thomas.richard@...tlin.com>
Cc: Linus Walleij <linus.walleij@...aro.org>,
	Geert Uytterhoeven <geert+renesas@...der.be>,
	Bartosz Golaszewski <brgl@...ev.pl>, Lee Jones <lee@...nel.org>,
	Pavel Machek <pavel@....cz>, linux-gpio@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-leds@...r.kernel.org,
	thomas.petazzoni@...tlin.com, DanieleCleri@...on.eu,
	GaryWang@...on.com.tw
Subject: Re: [PATCH 4/5] pinctrl: Add pin controller driver for AAEON UP
 boards

On Thu, Jan 16, 2025 at 02:23:57PM +0100, Thomas Richard wrote:
> On 1/16/25 13:31, Andy Shevchenko wrote:
> > On Thu, Jan 16, 2025 at 01:21:16PM +0100, Thomas Richard wrote:
> >> On 1/16/25 10:12, Andy Shevchenko wrote:
> >>> On Tue, Jan 14, 2025 at 11:28:26AM +0100, Thomas Richard wrote:
> >>>> On 1/13/25 10:46, Andy Shevchenko wrote:
> >>>>> On Fri, Jan 03, 2025 at 11:28:30AM +0100, Thomas Richard wrote:
> >>>>>> On 12/22/24 00:43, Linus Walleij wrote:
> >>>>>>> On Fri, Dec 20, 2024 at 2:50 PM Thomas Richard
> >>>>>>> <thomas.richard@...tlin.com> wrote:

...

> >>>>>>>> Yes my cover letter was a bit short, and maybe some context was missing.
> >>>>>>>
> >>>>>>> The text and graphics below explain it very well, so please include them
> >>>>>>> into the commit message so we have it there!
> >>>>>>>
> >>>>>>>> This FPGA acts as a level shifter between the Intel SoC pins and the pin
> >>>>>>>> header, and also makes a kind of switch/mux.
> >>>>>>>
> >>>>>>> Since it's Intel we need to notify Andy to help out with this so that
> >>>>>>> it gets done in a way that works with how he think consumers
> >>>>>>> should interact with Intel pin control and GPIO.
> >>>>>>>
> >>>>>>>> +---------+         +--------------+             +---+
> >>>>>>>>           |         |              |             | H |
> >>>>>>>>           |---------|              |-------------| E |
> >>>>>>>>           |         |              |             | A |
> >>>>>>>> Intel Soc |---------|    FPGA      |-------------| D |
> >>>>>>>>           |         |              |             | E |
> >>>>>>>>           |---------|              |-------------| R |
> >>>>>>>>           |         |              |             |   |
> >>>>>>>> ----------+         +--------------+             +---+
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> For most of the pins, the FPGA opens/closes a switch to enable/disable
> >>>>>>>> the access to the SoC pin from a pin header.
> >>>>>>>> Each "switch", has a direction flag that shall be set in tandem with the
> >>>>>>>> status of the SoC pin.
> >>>>>>>> For example, if the SoC pin is in PWM mode, the "switch" shall be
> >>>>>>>> configured in output direction.
> >>>>>>>> If the SoC pin is set in GPIO mode, the direction of the "switch" shall
> >>>>>>>> corresponds to the GPIO direction.
> >>>>>>>>
> >>>>>>>> +---------+              +--------------+             +---+
> >>>>>>>>           |              |              |             | H |
> >>>>>>>>           |              |      \       |             | E |
> >>>>>>>>           |   PWM1       |       \      |             | A |
> >>>>>>>> Intel Soc |--------------|-----   \-----|-------------| D |
> >>>>>>>>           |              |              |             | E |
> >>>>>>>>           |              |              |             | R |
> >>>>>>>>           |              |    FPGA      |             |   |
> >>>>>>>> ----------+              +--------------+             +---+
> >>>>>>>>
> >>>>>>>> (PWM1 pin from Intel SoC can be used as PWM, and also in GPIO mode,
> >>>>>>>> thanks to the Intel pinctrl driver).
> >>>>>>>>
> >>>>>>>> Few pins (PINMUX_* pins) work differently. The FPGA acts as a mux and
> >>>>>>>> routes for example the I2C0_SDA pin or GPIOX (of the SoC) to the pin header.
> >>>
> >>> Yep, that's clear.
> >>>
> >>>>>>>> +---------+           +--------------+             +---+
> >>>>>>>>           | I2C0_SDA  |              |             | H |
> >>>>>>>>           |-----------|----- \       |             | E |
> >>>>>>>>           |           |       \      |             | A |
> >>>>>>>> Intel Soc |           |        \-----|-------------| D |
> >>>>>>>>           | GPIOX     |              |             | E |
> >>>>>>>>           |-----------|-----         |             | R |
> >>>>>>>>           |           |    FPGA      |             |   |
> >>>>>>>> ----------+           +--------------+             +---+
> >>>>>>>>
> >>>>>>>> The pin header looks like this:
> >>>>>>>> +--------------------+--------------------+
> >>>>>>>> |      3.3V          |       5V           |
> >>>>>>>> | GPIO2 / I2C1_SDA   |       5V           |
> >>>>>>>> | GPIO3 / I2C1_SCL   |       GND          |
> >>>>>>>> | GPIO4 / ADC0       | GPIO14 / UART1_TX  |
> >>>>>>>> |      GND           | GPIO15 / UART1_RX  |
> >>>>>>>> | GPIO17 / UART1_RTS | GPIO18 / I2S_CLK   |
> >>>>>>>> |     GPIO27         |       GND          |
> >>>>>>>> |     GPIO22         |      GPIO23        |
> >>>>>>>> |      3.3V          |      GPIO24        |
> >>>>>>>> | GPIO10 / SPI_MOSI  |       GND          |
> >>>>>>>> | GPIO9 / SPI_MISO   |      GPIO25        |
> >>>>>>>> | GPIO11 / SPI_CLK   | GPIO8 / SPI_CS0    |
> >>>>>>>> |      GND           | GPIO7 / SPI_CS1    |
> >>>>>>>> | GPIO0 / I2C0_SDA   | GPIO1 / I2C0_SCL   |
> >>>>>>>> |     GPIO5          |       GND          |
> >>>>>>>> |     GPIO6          | GPIO12 / PWM0      |
> >>>>>>>> | GPIO13 / PWM1      |       GND          |
> >>>>>>>> | GPIO19 / I2S_FRM   | GPIO16 / UART1_CTS |
> >>>>>>>> |     GPIO26         | GPIO20 / I2S_DIN   |
> >>>>>>>> |      GND           | GPIO21 / I2S_DOUT  |
> >>>>>>>> +--------------------+--------------------+
> >>>>>>>>
> >>>>>>>> The GPIOs in the pin header corresponds to the gpiochip I declare in
> >>>>>>>> this driver.
> >>>>>>>> So when I want to use a pin in GPIO mode, the upboard pinctrl driver
> >>>>>>>> requests the corresponding SoC GPIO to the Intel pinctrl driver.
> >>>>>>>> The SoC pins connected to the FPGA, are identified with "external" id.
> >>>>>>>>
> >>>>>>>> The hardware and the FPGA were designed in tandem, so you know for
> >>>>>>>> example that for the GPIOX you need to request the Nth "external" GPIO.
> >>>>>>>>
> >>>>>>>> When you drive your GPIO, the upboard gpiochip manages in the same time
> >>>>>>>> the direction of the "switch" and the value/direction of the
> >>>>>>>> corresponding SoC pin.
> >>>>>>>>
> >>>>>>>> +------------------+         +--------------+             +---+
> >>>>>>>>                    |---------|              |-------------| H |
> >>>>>>>>                    |---------|   GPIOCHIP   |-------------| E |
> >>>>>>>>    Intel gpiochip  |---------|              |-------------| A |
> >>>>>>>>  provided by Intel |---------|    FPGA      |-------------| D |
> >>>>>>>>   pinctrl driver   |---------|              |-------------| E |
> >>>>>>>>                    |---------|              |-------------| R |
> >>>>>>>>                    |---------|              |-------------|   |
> >>>>>>>> +------------------+         +--------------+             +---+
> >>>>>>>>
> >>>>>>>> About gpiochip_add_pinlist_range(), I added it because the FPGA pins
> >>>>>>>> used by the gpiochip are not consecutive.
> >>>>>>>>
> >>>>>>>> Please let me know if it is not clear.
> >>>>>>>> And sorry I'm not very good to make ascii art.
> >>>>>>>
> >>>>>>> I get it! We have a similar driver in the kernel already, look into:
> >>>>>>> drivers/gpio/gpio-aggregator.c
> >>>>>>>
> >>>>>>> The aggregator abstraction is however just software. What you
> >>>>>>> need here is a gpio-aggregator that adds some hardware
> >>>>>>> control on top. But it has a very nice design using a bitmap
> >>>>>>> to keep track of the GPIOs etc, and it supports operations
> >>>>>>> on multiple GPIOs (many man-hours of hard coding and
> >>>>>>> design went into that driver, ask Geert and Andy...)
> >>>>>>>
> >>>>>>> So I would proceed like this:
> >>>>>>>
> >>>>>>> - The pin control part of the driver looks sound, except
> >>>>>>>   for the way you add ranges.
> >>>>>>>
> >>>>>>> - The gpiochip part needs to be refactored using the
> >>>>>>>   ideas from gpio-aggregator.c.
> >>>>>>>
> >>>>>>> - Look closely at aggregator and see what you can do
> >>>>>>>   based on that code, if you can mimic how it picks up
> >>>>>>>   and forwards all GPIO functions. Maybe part of it
> >>>>>>>   needs to be made into a library?
> >>>>>>>  <linux/gpio/gpio-aggregator.h>?
> >>>>>>>   For example if you start to feel like "I would really like
> >>>>>>>   to just call gpio_fwd_get_multiple() then this is what
> >>>>>>>   you want to do. The library can probably still be
> >>>>>>>   inside gpio-aggregator.c the way we do it in
> >>>>>>>   e.g. gpio-mmio.c, just export and keep library functions
> >>>>>>>   separately.
> >>>>>>
> >>>>>> Ok I think I understand what you expect.
> >>>>>> I started to look at the gpio-aggregator code, play a bit with it, and
> >>>>>> refactor it to use it from my driver.
> >>>>>>
> >>>>>> My main issue is about the request of the SoC GPIOs done by the aggregator.
> >>>>>> If from my driver I call the aggregator library to create a gpiochip,
> >>>>>> the SoC pins will be requested. So the SoC pins will be set in GPIO
> >>>>>> mode, and the pins will never be in function mode.
> >>>>>> There is no way to set the pins back to function mode (even if the GPIO
> >>>>>> is free).
> >>>>>>
> >>>>>> I tried to add a feature in the aggregator to defer the request of the gpio.
> >>>>>> So at the beginning of each ops the gpio_desc is checked. If it is
> >>>>>> valid, the gpio can be used. Otherwise, the gpio is requested.
> >>>>>> For example:
> >>>>>>
> >>>>>> gpio_fwd_get() {
> >>>>>> 	if (!gpio_desc_is_valid(desc))
> >>>>>> 		desc = request_gpio()
> >>>>>>
> >>>>>> 	return gpiod_get_value(desc)
> >>>>>> }
> >>>>>>
> >>>>>> But when a gpiochip is registered, the core calls get_direction() or
> >>>>>> direction_input(), so all GPIOs are requested and it does not solve my
> >>>>>> problem.
> >>>>>>
> >>>>>> I expect to register a gpiochip without setting all pins in GPIO mode at
> >>>>>> probe time (like all pinctrl driver do).
> >>>>>> But I did not find a solution.
> >>>>>
> >>>>> Basically what you need is a pinctrl-aggregattor (an analogue for the pin
> >>>>> muxing and configuration).
> >>>>
> >>>> I found a trick to workaround the get_direction() issue in the
> >>>> gpio-aggregator.
> >>>>
> >>>> I added a "request on first use" feature on the aggregator.
> >>>> The GPIO is requested during the request() operation of the fowarder.
> >>>>
> >>>> static int gpio_fwd_request(struct gpio_chip *chip, unsigned int offset)
> >>>> {
> >>>> 	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
> >>>>
> >>>> 	if (!IS_ERR_OR_NULL(fwd->descs[offset]))
> >>>> 		return 0;
> >>>>
> >>>> 	fwd->descs[offset] = devm_gpiod_get_index(fwd->dev, NULL,
> >>>> 						offset, GPIOD_ASIS);
> >>>>
> >>>> 	return PTR_ERR_OR_ZERO(fwd->descs[offset]);
> >>>> }
> >>>>
> >>>> The remaining problem is that the get_direction() callback is called
> >>>> during gpiochip registration. For now if the gpio_desc is not valid (so
> >>>> the GPIO was not yet requested) I return GPIO_LINE_DIRECTION_OUT by
> >>>> default. But I'm not very convinced by this hack.
> >>>> Maybe I could retrieve the gpio_chip and call its get_direction()
> >>>> callback, but it seems not to be a better idea.
> >>>>
> >>>> For the pinctrl-aggregator you mentioned, if I understand correctly the
> >>>> idea to aggregate the SoC pins in a pinctrl aggregator (with a
> >>>> gpio_chip) which just forwards gpio_request_enable(),
> >>>> gpio_disable_free(), gpio_set_direction() and also all gpio_chip operations.
> >>>
> >>> No only these, all of the pin mux, pin configuration, and GPIO operations
> >>> should be proxied.
> >>
> >> Why should I proxy operations that will never be used (pin mux, pin
> >> conf) ? I mean there will never be a driver that will configure FPGA pins.
> > 
> > According to your picture above the FPGA works as a pin function selector,
> > which also implies different pin configuration (e.g., slew rate for I²C pins).
> > Did I get it wrong?
> 
> Yes you are right. In function mode the FPGA selects the I2C pin. When
> the FPGA GPIO is requested, the FPGA changes the mux to select a SoC
> GPIO pin (a pure GPIO pin).
> 
> >>>> But how to deal with the pinctrl of my FPGA ? For one of its fake pin
> >>>> the dummy pinctrl drives the corresponding SoC pin and FPGA pin ?
> >>>
> >>> What's the "fake pin"? I can't find the one up in your schemas.
> >>
> >> I thought that the idea was a virtual pinctrl which drive the SoC
> >> pinctrl and the FPGA pinctrl. But what you mean is the FPGA pinctrl acts
> >> as a proxy.
> > 
> > When one wants to configure the pin parameters (let's say pin bias) the driver
> > delegates that to the SoC, in case the FPGA doesn't repeat this itself. Or both,
> > but this makes things too complicated already.
> 
> The FPGA cannot configure pin parameter. It can only configures the
> forward direction.
> For example, for UART_TX pin the FPGA will set direction as "output" and
> for UART_RX pin the FPGA will set direction as "input".
> It can also enable/disable the forward of the SoC pin. If the forward of
> a pin is disabled, the FPGA pin is in HIGH-Z.

So, which is exactly why proxying is needed:
1) it delegates pin configuration when user asks for a change;
2) it properly configures SoC pin directions, etc in accordance to the signal
on the header and the user ask.

> >>>> So for each pin, the aggregator may have multiple pins to drive ?
> >>>
> >>> Depending on the case, but yes. Currently we have implementations of
> >>> the discrete pin controls on Intel platforms based on ACPI (see Intel Minnow,
> >>> Intel Galileo, Intel Edison/Arduino boards in meta-acpi project [1]).
> >>> You probably want something similar to be written in C.
> >>>
> >>>> Was it your idea Andy ?
> >>>>
> >>>> Other challenge is to retrieve all the pins to add in the
> >>>> pinctrl-aggregator. As input I have only GPIO descriptors, but I guess
> >>>> it should be feasible.
> >>>
> >>> In general your "proxy" (FPGA) pin control driver should be consumer of all SoC
> >>> pins (and their respective muxing and configurations) and be provider of the
> >>> pins that are available to the user.
> >>
> >> Isn't that a bit over-engineered for my use case ?
> > 
> > Yes, but it's already over-engineered in the HW, no?
> > 
> >> For the pinconf / pinmux, the FPGA is just a voltage translator.
> > 
> > It doesn't correspond to your picture where the pin function selector is depicted.
> 
> Yes you're right. I mean the mux can only select one function or GPIO mode.

Which is fine, but it is still a pin muxing.

> I don't know why there is a mux to select a GPIO only pin, I'm pretty
> sure the I2C pins can be set in GPIO mode.
> It's probably for an hardware reason that a mux was added for only few pins.
> 
> >> It is transparent. The only relevant thing for the FPGA is the direction to
> >> set for the switch of each pin. And the drivers knows which directions
> >> to apply during the probe. This direction will only change in GPIO mode,
> >> but in GPIO mode we know which direction to set.
> >>
> >> For the GPIO part, the FPGA provides GPIOs. And in this case it is a
> >> consumer of SoC GPIOs, it is already a kind of aggregator.
> >>
> >>> [1]: https://github.com/westeri/meta-acpi/tree/master/recipes-bsp/acpi-tables/samples

-- 
With Best Regards,
Andy Shevchenko

Powered by blists - more mailing lists