Re: [PATCH] MIPS: pci: lantiq: restore reset gpio polarity

[Martin Schiller <ms@dev.tdt.de>]

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@vger.kernel.org
>> Signed-off-by: Martin Schiller <ms@dev.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