Message-ID: <20220906220901.p2c44we7i4c35uvx@pali>
Date:   Wed, 7 Sep 2022 00:09:01 +0200
From:   Pali Rohár <pali@...nel.org>
To:     Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc:     Shawn Guo <shawn.guo@...aro.org>,
        Lorenzo Pieralisi <lpieralisi@...nel.org>,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        Rob Herring <robh@...nel.org>,
        Krzysztof Wilczyński <kw@...ux.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        Bartosz Golaszewski <brgl@...ev.pl>, linux-pci@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] PCI: mvebu: switch to using gpiod API

On Tuesday 06 September 2022 14:52:42 Dmitry Torokhov wrote:
> On Tue, Sep 06, 2022 at 11:41:14PM +0200, Pali Rohár wrote:
> > On Tuesday 06 September 2022 14:26:32 Dmitry Torokhov wrote:
> > > Hi Pali,
> > > 
> > > On Tue, Sep 06, 2022 at 11:16:28PM +0200, Pali Rohár wrote:
> > > > Hello!
> > > > 
> > > > On Tuesday 06 September 2022 13:43:01 Dmitry Torokhov wrote:
> > > > > This patch switches the driver away from legacy gpio/of_gpio API to
> > > > > gpiod API, and removes use of of_get_named_gpio_flags() which I want to
> > > > > make private to gpiolib.
> > > > 
> > > > There are many pending pci-mvebu.c patches waiting for review and merge,
> > > > so I would suggest to wait until all other mvebu patches are processed
> > > > and then process this one... longer waiting period :-(
> > > 
> > > OK, it is not super urgent. OTOH it is a very simple patch :)
> > 
> > In the worst case, I will take it into my pending list of pci-mvebu.c
> > patches, so it would not be lost. Just yesterday I collected patches and
> > created pending list:
> > https://git.kernel.org/pub/scm/linux/kernel/git/pali/linux.git/log/?h=pci-mvebu-pending
> > 
> > > > 
> > > > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@...il.com>
> > > > > ---
> > > > >  drivers/pci/controller/pci-mvebu.c | 48 +++++++++---------------------
> > > > >  1 file changed, 14 insertions(+), 34 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/pci/controller/pci-mvebu.c b/drivers/pci/controller/pci-mvebu.c
> > > > > index 1ced73726a26..a54beb8f611c 100644
> > > > > --- a/drivers/pci/controller/pci-mvebu.c
> > > > > +++ b/drivers/pci/controller/pci-mvebu.c
> > > > > @@ -11,14 +11,13 @@
> > > > >  #include <linux/bitfield.h>
> > > > >  #include <linux/clk.h>
> > > > >  #include <linux/delay.h>
> > > > > -#include <linux/gpio.h>
> > > > > +#include <linux/gpio/consumer.h>
> > > > >  #include <linux/init.h>
> > > > >  #include <linux/mbus.h>
> > > > >  #include <linux/slab.h>
> > > > >  #include <linux/platform_device.h>
> > > > >  #include <linux/of_address.h>
> > > > >  #include <linux/of_irq.h>
> > > > > -#include <linux/of_gpio.h>
> > > > >  #include <linux/of_pci.h>
> > > > >  #include <linux/of_platform.h>
> > > > >  
> > > > > @@ -1261,9 +1260,8 @@ static int mvebu_pcie_parse_port(struct mvebu_pcie *pcie,
> > > > >  	struct mvebu_pcie_port *port, struct device_node *child)
> > > > >  {
> > > > >  	struct device *dev = &pcie->pdev->dev;
> > > > > -	enum of_gpio_flags flags;
> > > > >  	u32 slot_power_limit;
> > > > > -	int reset_gpio, ret;
> > > > > +	int ret;
> > > > >  	u32 num_lanes;
> > > > >  
> > > > >  	port->pcie = pcie;
> > > > > @@ -1327,40 +1325,22 @@ static int mvebu_pcie_parse_port(struct mvebu_pcie *pcie,
> > > > >  			 port->name, child);
> > > > >  	}
> > > > >  
> > > > > -	reset_gpio = of_get_named_gpio_flags(child, "reset-gpios", 0, &flags);
> > > > > -	if (reset_gpio == -EPROBE_DEFER) {
> > > > > -		ret = reset_gpio;
> > > > > +	port->reset_name = devm_kasprintf(dev, GFP_KERNEL, "%s-reset",
> > > > > +					  port->name);
> > > > > +	if (!port->reset_name) {
> > > > > +		ret = -ENOMEM;
> > > > >  		goto err;
> > > > >  	}
> > > > >  
> > > > > -	if (gpio_is_valid(reset_gpio)) {
> > > > > -		unsigned long gpio_flags;
> > > > > -
> > > > > -		port->reset_name = devm_kasprintf(dev, GFP_KERNEL, "%s-reset",
> > > > > -						  port->name);
> > > > > -		if (!port->reset_name) {
> > > > > -			ret = -ENOMEM;
> > > > > +	port->reset_gpio = devm_fwnode_gpiod_get(dev, of_fwnode_handle(child),
> > > > > +						 "reset", GPIOD_OUT_HIGH,
> > > > 
> > > > What does it mean that there is a new GPIOD_OUT_HIGH flag passed to the
> > > > devm_fwnode_gpiod_get() function?
> > > 
> > > This means that we drive the line as "active" as soon as we successfully
> > > grab GPIO. This is the same as we had with devm_gpio_request_one(), but
> > 
> > Ah :-( Another thing to fix. Driver should not change the signal line at
> > this stage, but only when it is explicitly asked - at later stage. Some
> > PCIe card do not like flipping reset line too quick. I see this fix
> 
> As far as I can see the driver has a delay of 100 usec before releasing
> reset line, plus additional delay for post-reset. Is this really not
> sufficient?

I do not know right now. Something which I planning to measure and check
after this... One point is that we do not want to sleep in kernel probe
functions for a longer time than needed as it is serialized and slow
down kernel boot. And another point is that there is still open question
how long must kernel hold reset line of endpoint card to ensure that any
connected PCIe card is properly reset... I remember from past testing
that some Qualcomm wifi ath10k cards requires at least 10ms.

> > would not be such easy as during startup we need to reset endpoint card.
> > Normally just putting it from reset, but if card was not reset state
> > prior probing driver then it is needed to first put it into reset...
> > 
> > I would fix it this issue after your patch is merged to prevent any
> > other merge conflicts.
> > 
> > How to tell devm_fwnode_gpiod_get() function that caller is not
> > interested in changing signal line? Just by changing GPIOD_OUT_HIGH to 0?
> 
> I think there are 2 options:
> 
> 1. Start with GPIOD_OUT_LOW (i.e. reset is explicitly deasserted), and
> then in powerup/powerdown you do explicit on/off transitions with proper
> timings.

PERST# is active-low. So deasserting means to put it into high state.
But device tree can specify if line is active-high as on some board
designs is GPIO output connected to inverter (or to level shifter with
additional logic of signal inversion). So what [GPIOD_]OUT_LOW means in
this context? Just it is needed that from driver point of view always
value 1 means reset active and 0 means reset inactive, independently of
double (triple?) inversions.

> 2. Start with GPIOD_ASIS (i.e. do not configure line at all), and then
> when powering up you need
> 
> 	gpiod_direction_output(port->reset_gpio, GPIOD_OUT_HIGH);
> 
> on the first invocation (and you can skip call to
> gpiod_set_value_cansleep(port->reset_gpio, 1) in that case).

This option is probably better as reset line logic is only at
appropriate place. And also this was my idea.

Anyway, in the past I wrote proposal how to cleanup and fixup it:
https://lore.kernel.org/linux-pci/20211022183808.jdeo7vntnagqkg7g@pali/

> > 
> > > we do not need to figure out actual polarity.
> > > 
> > > > 
> > > > > +						 port->name);
> > > > > +	ret = PTR_ERR_OR_ZERO(port->reset_gpio);
> > > > > +	if (ret) {
> > > > > +		if (ret	!= -ENOENT)
> > 
> > Just one check, I think that between "ret" and "!=" is TAB instead of
> > space. But I'm not sure if it was mangled by email client or of there is
> > really TAB.
> 
> Ah, indeed, sorry about that.
> 
> > 
> > > > >  			goto err;
> > > > > -		}
> > > > > -
> > > > > -		if (flags & OF_GPIO_ACTIVE_LOW) {
> > > > > -			dev_info(dev, "%pOF: reset gpio is active low\n",
> > > > > -				 child);
> > > > > -			gpio_flags = GPIOF_ACTIVE_LOW |
> > > > > -				     GPIOF_OUT_INIT_LOW;
> > > > > -		} else {
> > > > > -			gpio_flags = GPIOF_OUT_INIT_HIGH;
> > > > > -		}
> > > > > -
> > > > > -		ret = devm_gpio_request_one(dev, reset_gpio, gpio_flags,
> > > > > -					    port->reset_name);
> > > > > -		if (ret) {
> > > > > -			if (ret == -EPROBE_DEFER)
> > > > > -				goto err;
> > > > > -			goto skip;
> > > > > -		}
> > > > > -
> > > > > -		port->reset_gpio = gpio_to_desc(reset_gpio);
> > > > > +		/* reset gpio is optional */
> > > > > +		port->reset_gpio = NULL;
> > > > 
> > > > Maybe you can also release port->reset_name as it is not used at this
> > > > stage?
> > > 
> > > OK, I figured it was just a few bytes, but sure, I'll add devm_kfree().
> > > 
> > > Thanks for the review.
> > > 
> > > -- 
> > > Dmitry
> 
> -- 
> Dmitry

Powered by blists - more mailing lists