| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Z4j8NmKMEL7PALmw@smile.fi.intel.com>
Date: Thu, 16 Jan 2025 14:31:50 +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 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?
> >> 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.
> >> 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.
> 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