Re: [PATCH 4/5] pinctrl: Add pin controller driver for AAEON UP boards

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Fri, Jan 03, 2025 at 11:28:30AM +0100, Thomas Richard wrote:
> On 12/22/24 00:43, Linus Walleij wrote:
> > Hi Thomas,
> > 
> > thanks for your detailed reply!
> > 
> > On Fri, Dec 20, 2024 at 2:50 PM Thomas Richard
> > <thomas.richard@bootlin.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.
> >>
> >> +---------+           +--------------+             +---+
> >>           | 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.
> 
> Hi Linus,
> 
> 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).

-- 
With Best Regards,
Andy Shevchenko