Received-SPF: pass (google.com: domain of linux-kernel+bounces-212627-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Wed, 12 Jun 2024 23:29:13 -0700
From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
To: Martin Schiller <ms@dev.tdt.de>, olek2@wp.pl
CC: hauke@hauke-m.de, tsbogend@alpha.franken.de, rdunlap@infradead.org,
 robh@kernel.org, bhelgaas@google.com, linux-mips@vger.kernel.org,
 linux-kernel@vger.kernel.org, stable@vger.kernel.org
Subject: Re: [PATCH] MIPS: pci: lantiq: restore reset gpio polarity
User-Agent: K-9 Mail for Android
In-Reply-To: <5452d21c1491d2c8c8ea07ac319b3850@dev.tdt.de>
References: <20240607090400.1816612-1-ms@dev.tdt.de> <ZmnfQWFoIw5UCV-k@google.com> <7d34eb4017e809245daa342e3ccddf4f@dev.tdt.de> <b6bea9239050ed39ce3a051a5985b86d@dev.tdt.de> <ZmoXAR79emQBNyhj@google.com> <Zmov-8IGm-misoRs@google.com> <5452d21c1491d2c8c8ea07ac319b3850@dev.tdt.de>
Message-ID: <85ADA7E0-ABCF-4C9D-AD92-C4864347700F@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain;
 charset=utf-8
Content-Transfer-Encoding: quoted-printable

On June 12, 2024 11:01:42 PM PDT, Martin Schiller <ms@dev=2Etdt=2Ede> wrote=
:
>On 2024-06-13 01:32, Dmitry Torokhov wrote:
>> On Wed, Jun 12, 2024 at 02:45:37PM -0700, Dmitry Torokhov wrote:
>>> On Wed, Jun 12, 2024 at 09:47:39PM +0200, Martin Schiller wrote:
>>> > 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 th=
e
>>> > > > > polarity
>>> > > > > of the GPIO=2E
>>> > > > >
>>> > > > > According to the PCI specification, the RST# pin is an active-=
low
>>> > > > > signal=2E However, most of the device trees that have been wid=
ely
>>> > > > > used for
>>> > > > > a long time (mainly in the openWrt project) define this GPIO a=
s
>>> > > > > active-high and the old driver code inverted the signal intern=
ally=2E
>>> > > > >
>>> > > > > Apparently there are actually boards where the reset gpio must=
 be
>>> > > > > operated inverted=2E For this reason, we cannot use the
>>> > > > > GPIOD_OUT_LOW/HIGH
>>> > > > > flag for initialization=2E Instead, we must explicitly set the=
 gpio to
>>> > > > > value 1 in order to take into account any "GPIO_ACTIVE_LOW" fl=
ag that
>>> > > > > may have been set=2E
>>> > > >
>>> > > > Do you have example of such boards? They could not have worked b=
efore
>>> > > > 90c2d2eb7ab5 because it was actively setting the reset line to
>>> > > > physical
>>> > > > high, which should leave the device in reset state if there is a=
n
>>> > > > inverter between the AP and the device=2E
>>> > >
>>> > > Oh, you're right=2E I totally missed that '__gpio_set_value' was u=
sed in
>>> > > the original code and that raw accesses took place without paying
>>> > > attention to the GPIO_ACTIVE_* flags=2E
>>> > >
>>> > > You can find the device trees I am talking about in [1]=2E
>>> > >
>>> > > @Thomas Bogendoerfer:
>>> > > Would it be possible to stop the merging of this patch?
>>> > > I think We have to do do some further/other changes=2E
>>> > >
>>> > > >
>>> > > > >
>>> > > > > In order to remain compatible with all these existing device
>>> > > > > trees, we
>>> > > > > should therefore keep the logic as it was before the commit=2E
>>> > > >
>>> > > > 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 (whi=
ch is
>>> > > > apparently what you want for boards with broken DTS) but for boa=
rds
>>> > > > that accurately describe GPIO as GPIO_ACTIVE_LOW it well drive G=
PIO to
>>> > > > 0, leaving the card in reset state=2E
>>> > > >
>>> > > > You should either use gpiod_set_raw_value_calsleep() or we can t=
ry and
>>> > > > quirk it in gpiolib (like we do for many other cases of incorrec=
t GPIO
>>> > > > polarity descriptions and which is my preference)=2E
>>> >
>>> > 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 u=
sing
>>> > this driver=2E
>>>=20
>>> No, I don't=2E
>>>=20
>>> Could you please try this:
>>>=20
>>> diff --git a/drivers/gpio/gpiolib-of=2Ec b/drivers/gpio/gpiolib-of=2Ec
>>> index 59c7f8a2431a=2E=2E4948ecaa422c 100644
>>> --- a/drivers/gpio/gpiolib-of=2Ec
>>> +++ b/drivers/gpio/gpiolib-of=2Ec
>>> @@ -203,6 +203,16 @@ static void of_gpio_try_fixup_polarity(const stru=
ct device_node *np,
>>>  		 */
>>>  		{ "qi,lb60",		"rb-gpios",	true },
>>>  #endif
>>> +#if IS_ENABLED(CONFIG_PCI_LANTIQ)
>>> +		/*
>>> +		 * According to the PCI specification, the RST# pin is an
>>> +		 * active-low signal=2E However, most of the device trees that
>>> +		 * have been widely used for a long time incorrectly describe
>>> +		 * reset GPIO as active-high, and were also using wrong name
>>> +		 * for the property=2E
>>> +		 */
>>> +		{ "lantiq,pci-xway",	"gpios-reset",	false },
>>=20
>> Sorry, "gpios-reset" is wrong, the driver used "gpio-reset"=2E So:
>>=20
>> diff --git a/drivers/gpio/gpiolib-of=2Ec b/drivers/gpio/gpiolib-of=2Ec
>> index 59c7f8a2431a=2E=2Ed21085830632 100644
>> --- a/drivers/gpio/gpiolib-of=2Ec
>> +++ b/drivers/gpio/gpiolib-of=2Ec
>> @@ -203,6 +203,16 @@ static void of_gpio_try_fixup_polarity(const
>> struct device_node *np,
>>  		 */
>>  		{ "qi,lb60",		"rb-gpios",	true },
>>  #endif
>> +#if IS_ENABLED(CONFIG_PCI_LANTIQ)
>> +		/*
>> +		 * According to the PCI specification, the RST# pin is an
>> +		 * active-low signal=2E However, most of the device trees that
>> +		 * have been widely used for a long time incorrectly describe
>> +		 * reset GPIO as active-high, and were also using wrong name
>> +		 * for the property=2E
>> +		 */
>> +		{ "lantiq,pci-xway",	"gpio-reset",	false },
>> +#endif
>>  #if IS_ENABLED(CONFIG_TOUCHSCREEN_TSC2005)
>>  		/*
>>  		 * DTS for Nokia N900 incorrectly specified "active high"
>> @@ -512,9 +522,9 @@ static struct gpio_desc
>> *of_find_gpio_rename(struct device_node *np,
>>  		{ "reset",	"reset-n-io",	"marvell,nfc-uart" },
>>  		{ "reset",	"reset-n-io",	"mrvl,nfc-uart" },
>>  #endif
>> -#if !IS_ENABLED(CONFIG_PCI_LANTIQ)
>> +#if IS_ENABLED(CONFIG_PCI_LANTIQ)
>>  		/* MIPS Lantiq PCI */
>> -		{ "reset",	"gpios-reset",	"lantiq,pci-xway" },
>> +		{ "reset",	"gpio-reset",	"lantiq,pci-xway" },
>>  #endif
>>=20
>>  		/*
>
>I wonder, when this renaming did not work so far, why did we not see the
>error message "failed to request gpio" in the log?

The GPIO is requested as optional, so if it's not found there's no error=
=2E

Interestingly the only user in the mainline kernel uses "gpios-reset" in i=
s device tree which never worked=2E=20

--=20
Dmitry