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

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

On 2024-06-20 02:54, Dmitry Torokhov wrote:
> On Fri, Jun 14, 2024 at 10:43:29AM +0200, Martin Schiller wrote:
>> On 2024-06-13 22:06, Hauke Mehrtens wrote:
>> > On 6/12/24 21:47, 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.
>> > >
>> > > 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.
> 
> Yes, cleaning up DTS files when it is possible is nice.
> 
>> > >
>> > > Then we won't need any quirks.
> 
> Quirks are fairly cheap and we are not in a hot path here.
> 
>> >
>> > I am not aware that anyone is using a recent kernel on the VRX200
>> > outside of OpenWrt. I am also not aware that anyone is *not* appending
>> > the DTB to the kernel. The SoC is pretty old now, the successor of
>> > this SoC was released about 10 years ago.
>> >
>> 
>> We're not just talking about VRX200 (VR9) here, but even older devices
>> such as AR9 and Danube.
>> 
>> > For me it would be fine if you fix the broken device device trees
>> > shipped with the upstream kernel and with OpenWrt to make them work
>> > with the PCI driver instead of investing too much time into handling
>> > old DTBs.
>> >
>> > The PCI reset is inverted on some boards to handle a dying gasp. If
>> > the power breaks down the reset should get triggered and the PCIe
>> > device can send a dying gasp signal to the other side. This is done on
>> > the reference designs of some Lantiq PCIe DSL card for the VRX318 and
>> > probably also some other components.
>> >
>> > Hauke
>> 
>> What I missed so far is the fact that the driver used 
>> '__gpio_set_value'
>> before Dmitry's commit and thus used raw access to the GPIO.
>> 
>> This effectively means that every device that has worked with the 
>> driver
>> so far must have an ACTIVE_LOW reset, no matter what was configured in
>> the device tree.
>> 
>> 
>> So renaming the property in the dts from "gpio-reset" to "reset-gpios"
>> and setting the FLAGS to "GPIO_ACTIVE_LOW" should actually solve the
>> problem.
> 
> Right, luckily (to a definition of luckily) the driver and DTB used
> "wrong" syntax for the gpio property, so we can quirk it and make
> force ACTIVE_LOW polarity on old DTBs, and new DTBs with "reset-gpios"
> property will follow polarity specified in DTB.

We have already adapted the device trees in openWrt. [1]
Will you create a quirk patch for the old DTBs or should I create one?

> 
>> 
>> What still bothers me about the driver itself are 2 things:
>> 1. the initial value of GPIOD_OUT_LOW. This means that there is no 
>> real
>>    defined HIGH -> LOW -> HIGH on reset.
> 
> Is this actually needed? Typically a card requires certain time in 
> reset
> state (with reset line active) before it can be released, however there
> usually no restrictions on line being inactive beforehand. But 
> typically
> it will be pulled up to avoid leakage...
> 

No, I don't think that's absolutely necessary. Several tests have so far
shown that it works as it is at the moment.

[1] 
https://git.openwrt.org/?p=openwrt/openwrt.git;a=commitdiff;h=f6fe19ed6dfaf0444cd80f530bf89f6878fd5936