[v3,05/12] PCI: aardvark: Issue PERST via GPIO

Add support for issuing PERST via GPIO specified in 'reset-gpios'
property (as described in PCI device tree bindings).

Some buggy cards (e.g. Compex WLE900VX or WLE1216) are not detected
after reboot when PERST is not issued during driver initialization.

If bootloader already enabled link training then issuing PERST has no
effect for some buggy cards (e.g. Compex WLE900VX) and these cards are
not detected. We therefore clear the LINK_TRAINING_EN register before.

It was observed that Compex WLE900VX card needs to be in PERST reset
for at least 10ms if bootloader enabled link training.

Tested on Turris MOX.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 drivers/pci/controller/pci-aardvark.c | 43 ++++++++++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)

On Fri, Apr 24, 2020 at 10:39 AM Pali Rohár <pali@kernel.org> wrote:
>
> Add support for issuing PERST via GPIO specified in 'reset-gpios'
> property (as described in PCI device tree bindings).
>
> Some buggy cards (e.g. Compex WLE900VX or WLE1216) are not detected
> after reboot when PERST is not issued during driver initialization.
>
> If bootloader already enabled link training then issuing PERST has no
> effect for some buggy cards (e.g. Compex WLE900VX) and these cards are
> not detected. We therefore clear the LINK_TRAINING_EN register before.
>
> It was observed that Compex WLE900VX card needs to be in PERST reset
> for at least 10ms if bootloader enabled link training.
>
> Tested on Turris MOX.
>
> Signed-off-by: Pali Rohár <pali@kernel.org>
> ---
>  drivers/pci/controller/pci-aardvark.c | 43 ++++++++++++++++++++++++++-
>  1 file changed, 42 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> index a6c4d4d52631..9e2f44213b5e 100644
> --- a/drivers/pci/controller/pci-aardvark.c
> +++ b/drivers/pci/controller/pci-aardvark.c
> @@ -9,6 +9,7 @@
>   */
>
>  #include <linux/delay.h>
> +#include <linux/gpio.h>
>  #include <linux/interrupt.h>
>  #include <linux/irq.h>
>  #include <linux/irqdomain.h>
> @@ -18,6 +19,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/msi.h>
>  #include <linux/of_address.h>
> +#include <linux/of_gpio.h>
>  #include <linux/of_pci.h>
>
>  #include "../pci.h"
> @@ -204,6 +206,7 @@ struct advk_pcie {
>         int root_bus_nr;
>         int link_gen;
>         struct pci_bridge_emul bridge;
> +       struct gpio_desc *reset_gpio;
>  };
>
>  static inline void advk_writel(struct advk_pcie *pcie, u32 val, u64 reg)
> @@ -330,10 +333,31 @@ static void advk_pcie_train_link(struct advk_pcie *pcie)
>         dev_err(dev, "link never came up\n");
>  }
>
> +static void advk_pcie_issue_perst(struct advk_pcie *pcie)
> +{
> +       u32 reg;
> +
> +       if (!pcie->reset_gpio)
> +               return;
> +
> +       /* PERST does not work for some cards when link training is enabled */
> +       reg = advk_readl(pcie, PCIE_CORE_CTRL0_REG);
> +       reg &= ~LINK_TRAINING_EN;
> +       advk_writel(pcie, reg, PCIE_CORE_CTRL0_REG);
> +
> +       /* 10ms delay is needed for some cards */
> +       dev_info(&pcie->pdev->dev, "issuing PERST via reset GPIO for 10ms\n");
> +       gpiod_set_value_cansleep(pcie->reset_gpio, 1);
> +       usleep_range(10000, 11000);
> +       gpiod_set_value_cansleep(pcie->reset_gpio, 0);
> +}
> +
>  static void advk_pcie_setup_hw(struct advk_pcie *pcie)
>  {
>         u32 reg;
>
> +       advk_pcie_issue_perst(pcie);
> +
>         /* Set to Direct mode */
>         reg = advk_readl(pcie, CTRL_CONFIG_REG);
>         reg &= ~(CTRL_MODE_MASK << CTRL_MODE_SHIFT);
> @@ -406,7 +430,8 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
>
>         /*
>          * PERST# signal could have been asserted by pinctrl subsystem before
> -        * probe() callback has been called, making the endpoint going into
> +        * probe() callback has been called or issued explicitly by reset gpio
> +        * function advk_pcie_issue_perst(), making the endpoint going into
>          * fundamental reset. As required by PCI Express spec a delay for at
>          * least 100ms after such a reset before link training is needed.
>          */
> @@ -1046,6 +1071,22 @@ static int advk_pcie_probe(struct platform_device *pdev)
>         }
>         pcie->root_bus_nr = bus->start;
>
> +       pcie->reset_gpio = devm_gpiod_get_from_of_node(dev, dev->of_node,
> +                                                      "reset-gpios", 0,
> +                                                      GPIOD_OUT_LOW,
> +                                                      "pcie1-reset");
> +       ret = PTR_ERR_OR_ZERO(pcie->reset_gpio);
> +       if (ret) {
> +               if (ret == -ENOENT) {
> +                       pcie->reset_gpio = NULL;
> +               } else {
> +                       if (ret != -EPROBE_DEFER)
> +                               dev_err(dev, "Failed to get reset-gpio: %i\n",
> +                                       ret);
> +                       return ret;
> +               }
> +       }

I believe all this can be replaced with devm_gpiod_get_optional.

Rob

On Friday 24 April 2020 12:05:29 Rob Herring wrote:
> On Fri, Apr 24, 2020 at 10:39 AM Pali Rohár <pali@kernel.org> wrote:
> > @@ -1046,6 +1071,22 @@ static int advk_pcie_probe(struct platform_device *pdev)
> >         }
> >         pcie->root_bus_nr = bus->start;
> >
> > +       pcie->reset_gpio = devm_gpiod_get_from_of_node(dev, dev->of_node,
> > +                                                      "reset-gpios", 0,
> > +                                                      GPIOD_OUT_LOW,
> > +                                                      "pcie1-reset");
> > +       ret = PTR_ERR_OR_ZERO(pcie->reset_gpio);
> > +       if (ret) {
> > +               if (ret == -ENOENT) {
> > +                       pcie->reset_gpio = NULL;
> > +               } else {
> > +                       if (ret != -EPROBE_DEFER)
> > +                               dev_err(dev, "Failed to get reset-gpio: %i\n",
> > +                                       ret);
> > +                       return ret;
> > +               }
> > +       }
> 
> I believe all this can be replaced with devm_gpiod_get_optional.

I'm looking at the devm_gpiod_get_optional() code and it calls
fwnode_gpiod_get_index() which mangle passed gpio name by appending
suffix from array gpio_suffixes[]. And then mangled name is passed to
fwnode_get_named_gpiod(). At the end devm_gpiod_get_optional() handles
-ENOENT and transform it to NULL.

So via devm_gpiod_get_optional() I cannot request for "reset-gpios" DT
property directly, but I need to specify implicit name "reset" due to
appending suffix from gpio_suffixes[] array. And the only benefit is
that I do not need to handle ret == -ENOENT state.

Handling of -EPROBE_DEFER is still needed in aardvark driver.

For me it makes usage of devm_gpiod_get_optional() in this case harder
to debug code in future. As I would not be able to find a place which
reads 'reset-gpios' DT property via 'git grep reset-gpios'.

So it is really useful for this particular case to use
devm_gpiod_get_optional() instead of devm_gpiod_get_from_of_node().