Received-SPF: pass (google.com: domain of linux-kernel+bounces-212023-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Wed, 12 Jun 2024 10:47:45 -0700
From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
To: Martin Schiller <ms@dev.tdt.de>
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
Message-ID: <ZmnfQWFoIw5UCV-k@google.com>
References: <20240607090400.1816612-1-ms@dev.tdt.de>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240607090400.1816612-1-ms@dev.tdt.de>

Hi Marton,

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.

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

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?

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

-- 
Dmitry