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@xxxxxxxxx> > > > > --- > > > > 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? > 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. 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). > > > 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
