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.
Then we won't need any quirks.
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.
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.
2. if we change 1., then I think "mdelay(1)" is too short and we would
have to change it to at least "mdelay(100)".
Martin
