| Message ID | 20190618180206.4908-27-mmaddireddy@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | Enable Tegra PCIe root port features | expand |
On 18/06/2019 19:02, Manikanta Maddireddy wrote: > Tegra PCIe has fixed per port SFIO line to signal PERST#, which can be > controlled by AFI port register. However, if a platform routes a different > GPIO to the PCIe slot, then port register cannot control it. Add support > for GPIO based PERST# signal for such platforms. GPIO number comes from per > port PCIe device tree node. PCIe driver probe doesn't fail if per port > "reset-gpios" property is not populated, make sure that DT property is not > missed for such platforms. Really? So how come on Jetson TK1 I see ... [ 1.073165] tegra-pcie 1003000.pcie: failed to get reset GPIO: -2 [ 1.073210] tegra-pcie: probe of 1003000.pcie failed with error -2 And now ethernet is broken? Why has this not been tested on all Tegra platforms? There is no reason why code submitted to upstream by us (people at NVIDIA) have not tested this on our internal test farm. This is disappointing. > Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com> > Acked-by: Thierry Reding <treding@nvidia.com> > --- > V6: No change > > V5: > * Updated reset gpio toggle logic to reflect active low usage > * Replaced kasprintf() with devm_kasprintf() > * Updated commit message with more information. > > V4: Using devm_gpiod_get_from_of_node() to get reset-gpios > > V3: Using helper function to get reset-gpios > > V2: Using standard "reset-gpio" property > > drivers/pci/controller/pci-tegra.c | 45 ++++++++++++++++++++++++++---- > 1 file changed, 39 insertions(+), 6 deletions(-) > > diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c > index d2841532ff0e..a819bc087a05 100644 > --- a/drivers/pci/controller/pci-tegra.c > +++ b/drivers/pci/controller/pci-tegra.c > @@ -17,6 +17,7 @@ > #include <linux/debugfs.h> > #include <linux/delay.h> > #include <linux/export.h> > +#include <linux/gpio.h> > #include <linux/interrupt.h> > #include <linux/iopoll.h> > #include <linux/irq.h> > @@ -399,6 +400,8 @@ struct tegra_pcie_port { > unsigned int lanes; > > struct phy **phys; > + > + struct gpio_desc *reset_gpio; > }; > > struct tegra_pcie_bus { > @@ -544,15 +547,23 @@ static void tegra_pcie_port_reset(struct tegra_pcie_port *port) > unsigned long value; > > /* pulse reset signal */ > - value = afi_readl(port->pcie, ctrl); > - value &= ~AFI_PEX_CTRL_RST; > - afi_writel(port->pcie, value, ctrl); > + if (port->reset_gpio) { > + gpiod_set_value(port->reset_gpio, 1); > + } else { > + value = afi_readl(port->pcie, ctrl); > + value &= ~AFI_PEX_CTRL_RST; > + afi_writel(port->pcie, value, ctrl); > + } > > usleep_range(1000, 2000); > > - value = afi_readl(port->pcie, ctrl); > - value |= AFI_PEX_CTRL_RST; > - afi_writel(port->pcie, value, ctrl); > + if (port->reset_gpio) { > + gpiod_set_value(port->reset_gpio, 0); > + } else { > + value = afi_readl(port->pcie, ctrl); > + value |= AFI_PEX_CTRL_RST; > + afi_writel(port->pcie, value, ctrl); > + } > } > > static void tegra_pcie_enable_rp_features(struct tegra_pcie_port *port) > @@ -2199,6 +2210,7 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie) > struct tegra_pcie_port *rp; > unsigned int index; > u32 value; > + char *label; > > err = of_pci_get_devfn(port); > if (err < 0) { > @@ -2257,6 +2269,27 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie) > if (IS_ERR(rp->base)) > return PTR_ERR(rp->base); > > + label = devm_kasprintf(dev, GFP_KERNEL, "pex-reset-%u", index); > + if (!label) { > + dev_err(dev, "failed to create reset GPIO label\n"); > + return -ENOMEM; > + } > + > + /* > + * Returns null if reset-gpios property is not populated and > + * fall back to using AFI per port register to toggle PERST# > + * SFIO line. > + */ > + rp->reset_gpio = devm_gpiod_get_from_of_node(dev, port, > + "reset-gpios", 0, > + GPIOD_OUT_LOW, > + label); > + if (IS_ERR(rp->reset_gpio)) { > + err = PTR_ERR(rp->reset_gpio); > + dev_err(dev, "failed to get reset GPIO: %d\n", err); > + return err; > + } > + So I believe that we can just remove the above. I will give it a test and see. Cheers Jon
On 04-Jul-19 8:18 PM, Jon Hunter wrote: > On 18/06/2019 19:02, Manikanta Maddireddy wrote: >> Tegra PCIe has fixed per port SFIO line to signal PERST#, which can be >> controlled by AFI port register. However, if a platform routes a different >> GPIO to the PCIe slot, then port register cannot control it. Add support >> for GPIO based PERST# signal for such platforms. GPIO number comes from per >> port PCIe device tree node. PCIe driver probe doesn't fail if per port >> "reset-gpios" property is not populated, make sure that DT property is not >> missed for such platforms. > Really? So how come on Jetson TK1 I see ... > > [ 1.073165] tegra-pcie 1003000.pcie: failed to get reset GPIO: -2 > > > [ 1.073210] tegra-pcie: probe of 1003000.pcie failed with error -2 > > And now ethernet is broken? Why has this not been tested on all Tegra > platforms? There is no reason why code submitted to upstream by us > (people at NVIDIA) have not tested this on our internal test farm. This > is disappointing. I did verified this patch on our internal TK1 test farm. I also tested on TX1 and TX2. This issue popped up because below fix in gpiolib driver went in at the same time. https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=025bf37725f1929542361eef2245df30badf242e >> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com> >> Acked-by: Thierry Reding <treding@nvidia.com> >> --- >> V6: No change >> >> V5: >> * Updated reset gpio toggle logic to reflect active low usage >> * Replaced kasprintf() with devm_kasprintf() >> * Updated commit message with more information. >> >> V4: Using devm_gpiod_get_from_of_node() to get reset-gpios >> >> V3: Using helper function to get reset-gpios >> >> V2: Using standard "reset-gpio" property >> >> drivers/pci/controller/pci-tegra.c | 45 ++++++++++++++++++++++++++---- >> 1 file changed, 39 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c >> index d2841532ff0e..a819bc087a05 100644 >> --- a/drivers/pci/controller/pci-tegra.c >> +++ b/drivers/pci/controller/pci-tegra.c >> @@ -17,6 +17,7 @@ >> #include <linux/debugfs.h> >> #include <linux/delay.h> >> #include <linux/export.h> >> +#include <linux/gpio.h> >> #include <linux/interrupt.h> >> #include <linux/iopoll.h> >> #include <linux/irq.h> >> @@ -399,6 +400,8 @@ struct tegra_pcie_port { >> unsigned int lanes; >> >> struct phy **phys; >> + >> + struct gpio_desc *reset_gpio; >> }; >> >> struct tegra_pcie_bus { >> @@ -544,15 +547,23 @@ static void tegra_pcie_port_reset(struct tegra_pcie_port *port) >> unsigned long value; >> >> /* pulse reset signal */ >> - value = afi_readl(port->pcie, ctrl); >> - value &= ~AFI_PEX_CTRL_RST; >> - afi_writel(port->pcie, value, ctrl); >> + if (port->reset_gpio) { >> + gpiod_set_value(port->reset_gpio, 1); >> + } else { >> + value = afi_readl(port->pcie, ctrl); >> + value &= ~AFI_PEX_CTRL_RST; >> + afi_writel(port->pcie, value, ctrl); >> + } >> >> usleep_range(1000, 2000); >> >> - value = afi_readl(port->pcie, ctrl); >> - value |= AFI_PEX_CTRL_RST; >> - afi_writel(port->pcie, value, ctrl); >> + if (port->reset_gpio) { >> + gpiod_set_value(port->reset_gpio, 0); >> + } else { >> + value = afi_readl(port->pcie, ctrl); >> + value |= AFI_PEX_CTRL_RST; >> + afi_writel(port->pcie, value, ctrl); >> + } >> } >> >> static void tegra_pcie_enable_rp_features(struct tegra_pcie_port *port) >> @@ -2199,6 +2210,7 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie) >> struct tegra_pcie_port *rp; >> unsigned int index; >> u32 value; >> + char *label; >> >> err = of_pci_get_devfn(port); >> if (err < 0) { >> @@ -2257,6 +2269,27 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie) >> if (IS_ERR(rp->base)) >> return PTR_ERR(rp->base); >> >> + label = devm_kasprintf(dev, GFP_KERNEL, "pex-reset-%u", index); >> + if (!label) { >> + dev_err(dev, "failed to create reset GPIO label\n"); >> + return -ENOMEM; >> + } >> + >> + /* >> + * Returns null if reset-gpios property is not populated and >> + * fall back to using AFI per port register to toggle PERST# >> + * SFIO line. >> + */ >> + rp->reset_gpio = devm_gpiod_get_from_of_node(dev, port, >> + "reset-gpios", 0, >> + GPIOD_OUT_LOW, >> + label); >> + if (IS_ERR(rp->reset_gpio)) { >> + err = PTR_ERR(rp->reset_gpio); >> + dev_err(dev, "failed to get reset GPIO: %d\n", err); >> + return err; >> + } >> + > So I believe that we can just remove the above. I will give it a test > and see. > > Cheers > Jon > Error check should be modified as follows, if (PTR_ERR(rp->reset_gpio) == -ENOENT) rp->reset_gpio = NULL; else if (IS_ERR(rp->reset_gpio)) return PTR_ERR(rp->reset_gpio); Manikanta
On 04/07/2019 16:29, Manikanta Maddireddy wrote: > > > On 04-Jul-19 8:18 PM, Jon Hunter wrote: >> On 18/06/2019 19:02, Manikanta Maddireddy wrote: >>> Tegra PCIe has fixed per port SFIO line to signal PERST#, which can be >>> controlled by AFI port register. However, if a platform routes a different >>> GPIO to the PCIe slot, then port register cannot control it. Add support >>> for GPIO based PERST# signal for such platforms. GPIO number comes from per >>> port PCIe device tree node. PCIe driver probe doesn't fail if per port >>> "reset-gpios" property is not populated, make sure that DT property is not >>> missed for such platforms. >> Really? So how come on Jetson TK1 I see ... >> >> [ 1.073165] tegra-pcie 1003000.pcie: failed to get reset GPIO: -2 >> >> >> [ 1.073210] tegra-pcie: probe of 1003000.pcie failed with error -2 >> >> And now ethernet is broken? Why has this not been tested on all Tegra >> platforms? There is no reason why code submitted to upstream by us >> (people at NVIDIA) have not tested this on our internal test farm. This >> is disappointing. > > I did verified this patch on our internal TK1 test farm. I also tested > on TX1 and TX2. > This issue popped up because below fix in gpiolib driver went in > at the same time. > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=025bf37725f1929542361eef2245df30badf242e Ah! OK now that was unlucky. I guess a bisect would have told me that but it was clear to see PCI was broken and so quicker to debug this directly. OK, then sorry for the rant! >>> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com> >>> Acked-by: Thierry Reding <treding@nvidia.com> >>> --- >>> V6: No change >>> >>> V5: >>> * Updated reset gpio toggle logic to reflect active low usage >>> * Replaced kasprintf() with devm_kasprintf() >>> * Updated commit message with more information. >>> >>> V4: Using devm_gpiod_get_from_of_node() to get reset-gpios >>> >>> V3: Using helper function to get reset-gpios >>> >>> V2: Using standard "reset-gpio" property >>> >>> drivers/pci/controller/pci-tegra.c | 45 ++++++++++++++++++++++++++---- >>> 1 file changed, 39 insertions(+), 6 deletions(-) >>> >>> diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c >>> index d2841532ff0e..a819bc087a05 100644 >>> --- a/drivers/pci/controller/pci-tegra.c >>> +++ b/drivers/pci/controller/pci-tegra.c >>> @@ -17,6 +17,7 @@ >>> #include <linux/debugfs.h> >>> #include <linux/delay.h> >>> #include <linux/export.h> >>> +#include <linux/gpio.h> >>> #include <linux/interrupt.h> >>> #include <linux/iopoll.h> >>> #include <linux/irq.h> >>> @@ -399,6 +400,8 @@ struct tegra_pcie_port { >>> unsigned int lanes; >>> >>> struct phy **phys; >>> + >>> + struct gpio_desc *reset_gpio; >>> }; >>> >>> struct tegra_pcie_bus { >>> @@ -544,15 +547,23 @@ static void tegra_pcie_port_reset(struct tegra_pcie_port *port) >>> unsigned long value; >>> >>> /* pulse reset signal */ >>> - value = afi_readl(port->pcie, ctrl); >>> - value &= ~AFI_PEX_CTRL_RST; >>> - afi_writel(port->pcie, value, ctrl); >>> + if (port->reset_gpio) { >>> + gpiod_set_value(port->reset_gpio, 1); >>> + } else { >>> + value = afi_readl(port->pcie, ctrl); >>> + value &= ~AFI_PEX_CTRL_RST; >>> + afi_writel(port->pcie, value, ctrl); >>> + } >>> >>> usleep_range(1000, 2000); >>> >>> - value = afi_readl(port->pcie, ctrl); >>> - value |= AFI_PEX_CTRL_RST; >>> - afi_writel(port->pcie, value, ctrl); >>> + if (port->reset_gpio) { >>> + gpiod_set_value(port->reset_gpio, 0); >>> + } else { >>> + value = afi_readl(port->pcie, ctrl); >>> + value |= AFI_PEX_CTRL_RST; >>> + afi_writel(port->pcie, value, ctrl); >>> + } >>> } >>> >>> static void tegra_pcie_enable_rp_features(struct tegra_pcie_port *port) >>> @@ -2199,6 +2210,7 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie) >>> struct tegra_pcie_port *rp; >>> unsigned int index; >>> u32 value; >>> + char *label; >>> >>> err = of_pci_get_devfn(port); >>> if (err < 0) { >>> @@ -2257,6 +2269,27 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie) >>> if (IS_ERR(rp->base)) >>> return PTR_ERR(rp->base); >>> >>> + label = devm_kasprintf(dev, GFP_KERNEL, "pex-reset-%u", index); >>> + if (!label) { >>> + dev_err(dev, "failed to create reset GPIO label\n"); >>> + return -ENOMEM; >>> + } >>> + >>> + /* >>> + * Returns null if reset-gpios property is not populated and >>> + * fall back to using AFI per port register to toggle PERST# >>> + * SFIO line. >>> + */ >>> + rp->reset_gpio = devm_gpiod_get_from_of_node(dev, port, >>> + "reset-gpios", 0, >>> + GPIOD_OUT_LOW, >>> + label); >>> + if (IS_ERR(rp->reset_gpio)) { >>> + err = PTR_ERR(rp->reset_gpio); >>> + dev_err(dev, "failed to get reset GPIO: %d\n", err); >>> + return err; >>> + } >>> + >> So I believe that we can just remove the above. I will give it a test >> and see. >> >> Cheers >> Jon >> > Error check should be modified as follows, > > if (PTR_ERR(rp->reset_gpio) == -ENOENT) > rp->reset_gpio = NULL; > else if (IS_ERR(rp->reset_gpio)) > return PTR_ERR(rp->reset_gpio); OK, but I think that this should be ... + if (IS_ERR(rp->reset_gpio)) { + if (PTR_ERR(rp->reset_gpio) == -ENOENT) { + rp->reset_gpio = NULL; + } else { + dev_err(dev, "failed to get reset GPIO: %d\n", err); + return PTR_ERR(rp->reset_gpio); + } + } Cheers Jon
diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c index d2841532ff0e..a819bc087a05 100644 --- a/drivers/pci/controller/pci-tegra.c +++ b/drivers/pci/controller/pci-tegra.c @@ -17,6 +17,7 @@ #include <linux/debugfs.h> #include <linux/delay.h> #include <linux/export.h> +#include <linux/gpio.h> #include <linux/interrupt.h> #include <linux/iopoll.h> #include <linux/irq.h> @@ -399,6 +400,8 @@ struct tegra_pcie_port { unsigned int lanes; struct phy **phys; + + struct gpio_desc *reset_gpio; }; struct tegra_pcie_bus { @@ -544,15 +547,23 @@ static void tegra_pcie_port_reset(struct tegra_pcie_port *port) unsigned long value; /* pulse reset signal */ - value = afi_readl(port->pcie, ctrl); - value &= ~AFI_PEX_CTRL_RST; - afi_writel(port->pcie, value, ctrl); + if (port->reset_gpio) { + gpiod_set_value(port->reset_gpio, 1); + } else { + value = afi_readl(port->pcie, ctrl); + value &= ~AFI_PEX_CTRL_RST; + afi_writel(port->pcie, value, ctrl); + } usleep_range(1000, 2000); - value = afi_readl(port->pcie, ctrl); - value |= AFI_PEX_CTRL_RST; - afi_writel(port->pcie, value, ctrl); + if (port->reset_gpio) { + gpiod_set_value(port->reset_gpio, 0); + } else { + value = afi_readl(port->pcie, ctrl); + value |= AFI_PEX_CTRL_RST; + afi_writel(port->pcie, value, ctrl); + } } static void tegra_pcie_enable_rp_features(struct tegra_pcie_port *port) @@ -2199,6 +2210,7 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie) struct tegra_pcie_port *rp; unsigned int index; u32 value; + char *label; err = of_pci_get_devfn(port); if (err < 0) { @@ -2257,6 +2269,27 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie) if (IS_ERR(rp->base)) return PTR_ERR(rp->base); + label = devm_kasprintf(dev, GFP_KERNEL, "pex-reset-%u", index); + if (!label) { + dev_err(dev, "failed to create reset GPIO label\n"); + return -ENOMEM; + } + + /* + * Returns null if reset-gpios property is not populated and + * fall back to using AFI per port register to toggle PERST# + * SFIO line. + */ + rp->reset_gpio = devm_gpiod_get_from_of_node(dev, port, + "reset-gpios", 0, + GPIOD_OUT_LOW, + label); + if (IS_ERR(rp->reset_gpio)) { + err = PTR_ERR(rp->reset_gpio); + dev_err(dev, "failed to get reset GPIO: %d\n", err); + return err; + } + list_add_tail(&rp->list, &pcie->ports); }