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

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

Hi Thomas,

thanks for your detailed reply!

On Fri, Dec 20, 2024 at 2:50 PM Thomas Richard
<thomas.richard@xxxxxxxxxxx> 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.

- The way you split up gpiochip_add_pin_range() I
  still do not understand at all, in my view you just want
  this gpiochip to refer to the pin controller pins in the
  same file so I don't see it. How can e.g.
  pinctrl-sx150x.c do this trick but not your driver?
  I might be missing something though.

Yours,
Linus Walleij