[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <7d34eb4017e809245daa342e3ccddf4f@dev.tdt.de>
Date: Wed, 12 Jun 2024 20:39:59 +0200
From: Martin Schiller <ms@....tdt.de>
To: Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc: hauke@...ke-m.de, tsbogend@...ha.franken.de, rdunlap@...radead.org,
robh@...nel.org, bhelgaas@...gle.com, linux-mips@...r.kernel.org,
linux-kernel@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH] MIPS: pci: lantiq: restore reset gpio polarity
On 2024-06-12 19:47, Dmitry Torokhov wrote:
> Hi Marton,
Hi Dmitry,
>
> On Fri, Jun 07, 2024 at 11:04:00AM +0200, Martin Schiller wrote:
>> Commit 90c2d2eb7ab5 ("MIPS: pci: lantiq: switch to using gpiod API")
>> not
>> only switched to the gpiod API, but also inverted / changed the
>> polarity
>> of the GPIO.
>>
>> According to the PCI specification, the RST# pin is an active-low
>> signal. However, most of the device trees that have been widely used
>> for
>> a long time (mainly in the openWrt project) define this GPIO as
>> active-high and the old driver code inverted the signal internally.
>>
>> Apparently there are actually boards where the reset gpio must be
>> operated inverted. For this reason, we cannot use the
>> GPIOD_OUT_LOW/HIGH
>> flag for initialization. Instead, we must explicitly set the gpio to
>> value 1 in order to take into account any "GPIO_ACTIVE_LOW" flag that
>> may have been set.
>
> Do you have example of such boards? They could not have worked before
> 90c2d2eb7ab5 because it was actively setting the reset line to physical
> high, which should leave the device in reset state if there is an
> inverter between the AP and the device.
Oh, you're right. I totally missed that '__gpio_set_value' was used in
the original code and that raw accesses took place without paying
attention to the GPIO_ACTIVE_* flags.
You can find the device trees I am talking about in [1].
@Thomas Bogendoerfer:
Would it be possible to stop the merging of this patch?
I think We have to do do some further/other changes.
>
>>
>> In order to remain compatible with all these existing device trees, we
>> should therefore keep the logic as it was before the commit.
>
> With gpiod API operating with logical states there's still difference
> in
> logic:
>
> gpiod_set_value_cansleep(reset_gpio, 1);
>
> will leave GPIO at 1 if it is described as GPIO_ACTIVE_HIGH (which is
> apparently what you want for boards with broken DTS) but for boards
> that accurately describe GPIO as GPIO_ACTIVE_LOW it well drive GPIO to
> 0, leaving the card in reset state.
>
> You should either use gpiod_set_raw_value_calsleep() or we can try and
> quirk it in gpiolib (like we do for many other cases of incorrect GPIO
> polarity descriptions and which is my preference).
>
> This still leaves the question about boards that require inversion. Are
> you saying that they have real signal inverter on the line or that
> their
> device trees correctly describe the signal as GPIO_ACTIVE_LOW?
>
> BTW, please consider getting DTS trees for your devices into mainline.
> Why do you keep them separate?
Unfortunately, these are not "my" devices and I can't even test them.
I've got feedback from some users when I updated the lantiq target to
linux 6.1 in openwrt.
Let's assume that all boards physically expect an active-low signal.
If the GPIO_ACTIVE_LOW flag were now set in the device tree, the
original (old) driver would have an incorrect initial level (LOW instead
of HIGH) due to the
gpio_direction_output(reset_gpio, 1);
This is probably the reason why the flag GPIO_ACTIVE_HIGH is set in
almost all dts files in openwrt.
But with commit 90c2d2eb7ab5 the initial level (LOW) is guaranteed to be
wrong because of the "GPIOD_OUT_LOW" and cannot be changed by "wrong"
device tree settings.
The signal curve is LOW -> LOW -> HIGH instead of HIGH -> LOW -> HIGH.
>
>>
>> Fixes: 90c2d2eb7ab5 ("MIPS: pci: lantiq: switch to using gpiod API")
>> Cc: stable@...r.kernel.org
>> Signed-off-by: Martin Schiller <ms@....tdt.de>
>> ---
>> arch/mips/pci/pci-lantiq.c | 8 ++++----
>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/mips/pci/pci-lantiq.c b/arch/mips/pci/pci-lantiq.c
>> index 68a8cefed420..0844db34022e 100644
>> --- a/arch/mips/pci/pci-lantiq.c
>> +++ b/arch/mips/pci/pci-lantiq.c
>> @@ -124,14 +124,14 @@ static int ltq_pci_startup(struct
>> platform_device *pdev)
>> clk_disable(clk_external);
>>
>> /* setup reset gpio used by pci */
>> - reset_gpio = devm_gpiod_get_optional(&pdev->dev, "reset",
>> - GPIOD_OUT_LOW);
>> + reset_gpio = devm_gpiod_get_optional(&pdev->dev, "reset",
>> GPIOD_ASIS);
>> error = PTR_ERR_OR_ZERO(reset_gpio);
>> if (error) {
>> dev_err(&pdev->dev, "failed to request gpio: %d\n", error);
>> return error;
>> }
>> gpiod_set_consumer_name(reset_gpio, "pci_reset");
>> + gpiod_direction_output(reset_gpio, 1);
>>
>> /* enable auto-switching between PCI and EBU */
>> ltq_pci_w32(0xa, PCI_CR_CLK_CTRL);
>> @@ -194,10 +194,10 @@ static int ltq_pci_startup(struct
>> platform_device *pdev)
>>
>> /* toggle reset pin */
>> if (reset_gpio) {
>> - gpiod_set_value_cansleep(reset_gpio, 1);
>> + gpiod_set_value_cansleep(reset_gpio, 0);
>> wmb();
>> mdelay(1);
>> - gpiod_set_value_cansleep(reset_gpio, 0);
>> + gpiod_set_value_cansleep(reset_gpio, 1);
>> }
>> return 0;
>> }
>> --
>> 2.39.2
>>
>
> Thanks.
[1]
https://git.openwrt.org/?p=openwrt/openwrt.git;a=tree;f=target/linux/lantiq/files/arch/mips/boot/dts/lantiq
Powered by blists - more mailing lists