On June 12, 2024 11:01:42 PM PDT, Martin Schiller <ms@xxxxxxxxxx> 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 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.
>>>
>>> No, I don't.
>>>
>>> Could you please try this:
>>>
>>> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
>>> index 59c7f8a2431a..4948ecaa422c 100644
>>> --- a/drivers/gpio/gpiolib-of.c
>>> +++ b/drivers/gpio/gpiolib-of.c
>>> @@ -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. 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.
>>> + */
>>> + { "lantiq,pci-xway", "gpios-reset", false },
>>
>> Sorry, "gpios-reset" is wrong, the driver used "gpio-reset". So:
>>
>> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
>> index 59c7f8a2431a..d21085830632 100644
>> --- a/drivers/gpio/gpiolib-of.c
>> +++ b/drivers/gpio/gpiolib-of.c
>> @@ -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. 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.
>> + */
>> + { "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
>>
>> /*
>
>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.
Interestingly the only user in the mainline kernel uses "gpios-reset" in is device tree which never worked.
--
Dmitry
