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

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

On 2024-06-12 20:39, Martin Schiller wrote:
> 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).

So you mean we should add an entry for "lantiq,pci-xway" to the
of_gpio_try_fixup_polarity()?
Do you know any dts / device outside the openWrt universe which is using
this driver.

For the lantiq targets in openWrt, the devicetree blob is appended to
the kernel image and therefore also updated when doing a firmware
upgrade. So, maybe it would also be an option to fix the driver (using
GPIO_ACTIVE_* flag for the initial level and set it to 0 -> 1 -> 0) and
rework all the dts files to use GPIO_ACTIVE_LOW.

Then we won't need any quirks.

>> 
>> 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