| 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: <b50444f7-4dd1-4440-af36-783b1b4f8625@bootlin.com>
Date: Tue, 14 Jan 2025 11:28:26 +0100
From: Thomas Richard <thomas.richard@...tlin.com>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.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 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:
>>> Hi Thomas,
>>>
>>> thanks for your detailed reply!
>>>
>>> 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.
>>>>
>>>> +---------+ +--------------+ +---+
>>>> | 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).
>
Hi Andy,
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.
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 ? So for
each pin, the aggregator may have multiple pins to drive ?
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.
Best Regards,
Thomas
Powered by blists - more mailing lists