| Message ID | 20190516055307.25737-28-mmaddireddy@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Enable Tegra PCIe root port features | expand |
On Thu, May 16, 2019 at 11:23:06AM +0530, Manikanta Maddireddy wrote: > Add support for GPIO based PERST# signal. GPIO number comes from per port > PCIe device tree node. > > Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com> > --- > 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 | 41 +++++++++++++++++++++++++----- > 1 file changed, 35 insertions(+), 6 deletions(-) > > diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c > index 06b99fcbf382..09b4d384ba38 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> > @@ -400,6 +401,8 @@ struct tegra_pcie_port { > unsigned int lanes; > > struct phy **phys; > + > + struct gpio_desc *reset_gpiod; Nit: I'd leave away the "d" at the end there. Or perhaps even the _gpio suffix entirely. But it's fine either way. > }; > > struct tegra_pcie_bus { > @@ -583,15 +586,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_gpiod) { > + gpiod_set_value(port->reset_gpiod, 0); So is this actually deasserting the reset pin, or is it asserting a low-active reset? I think it's the latter, because ... > + } 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_gpiod) { > + gpiod_set_value(port->reset_gpiod, 1); After this the port should be functional, right? I think it'd be better to reverse the logic here and move the polarity of the GPIO into device tree. gpiod_set_value() takes care of inverting the level internally if the GPIO is marked as low-active in DT. The end result is obviously the same, but it makes the usage much clearer. If somebody want to write a DT for their board, they will look at the schematics and see a low-active reset line and may be tempted to describe it as such in DT, but with your current code that would be exactly the wrong way around. > + } 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) > @@ -2238,6 +2249,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) { > @@ -2296,6 +2308,23 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie) > if (IS_ERR(rp->base)) > return PTR_ERR(rp->base); > > + label = kasprintf(GFP_KERNEL, "pex-reset-%u", index); devm_kasprintf()? Thierry > + if (!label) { > + dev_err(dev, "failed to create reset GPIO label\n"); > + return -ENOMEM; > + } > + > + rp->reset_gpiod = devm_gpiod_get_from_of_node(dev, port, > + "reset-gpios", 0, > + GPIOD_OUT_LOW, > + label); > + kfree(label); > + if (IS_ERR(rp->reset_gpiod)) { > + err = PTR_ERR(rp->reset_gpiod); > + dev_err(dev, "failed to get reset GPIO: %d\n", err); > + return err; > + } > + > list_add_tail(&rp->list, &pcie->ports); > } > > -- > 2.17.1 >
On Tue, Jun 04, 2019 at 03:22:33PM +0200, Thierry Reding wrote: [...] > > + } 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_gpiod) { > > + gpiod_set_value(port->reset_gpiod, 1); > > After this the port should be functional, right? I think it'd be better > to reverse the logic here and move the polarity of the GPIO into device > tree. gpiod_set_value() takes care of inverting the level internally if > the GPIO is marked as low-active in DT. > > The end result is obviously the same, but it makes the usage much > clearer. If somebody want to write a DT for their board, they will look > at the schematics and see a low-active reset line and may be tempted to > describe it as such in DT, but with your current code that would be > exactly the wrong way around. I agree with Thierry here, you should change the logic. Question: what's the advantage of adding GPIO reset support if that's architected already in port registers ? I am pretty sure there is a reason behind it (and forgive me the dumb question) and I would like to have it written in the commit log. Thanks, Lorenzo > > + } 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) > > @@ -2238,6 +2249,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) { > > @@ -2296,6 +2308,23 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie) > > if (IS_ERR(rp->base)) > > return PTR_ERR(rp->base); > > > > + label = kasprintf(GFP_KERNEL, "pex-reset-%u", index); > > devm_kasprintf()? > > Thierry > > > + if (!label) { > > + dev_err(dev, "failed to create reset GPIO label\n"); > > + return -ENOMEM; > > + } > > + > > + rp->reset_gpiod = devm_gpiod_get_from_of_node(dev, port, > > + "reset-gpios", 0, > > + GPIOD_OUT_LOW, > > + label); > > + kfree(label); > > + if (IS_ERR(rp->reset_gpiod)) { > > + err = PTR_ERR(rp->reset_gpiod); > > + dev_err(dev, "failed to get reset GPIO: %d\n", err); > > + return err; > > + } > > + > > list_add_tail(&rp->list, &pcie->ports); > > } > > > > -- > > 2.17.1 > >
On 13-Jun-19 8:54 PM, Lorenzo Pieralisi wrote: > On Tue, Jun 04, 2019 at 03:22:33PM +0200, Thierry Reding wrote: > > [...] > >>> + } 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_gpiod) { >>> + gpiod_set_value(port->reset_gpiod, 1); >> After this the port should be functional, right? I think it'd be better >> to reverse the logic here and move the polarity of the GPIO into device >> tree. gpiod_set_value() takes care of inverting the level internally if >> the GPIO is marked as low-active in DT. >> >> The end result is obviously the same, but it makes the usage much >> clearer. If somebody want to write a DT for their board, they will look >> at the schematics and see a low-active reset line and may be tempted to >> describe it as such in DT, but with your current code that would be >> exactly the wrong way around. > I agree with Thierry here, you should change the logic. > > Question: what's the advantage of adding GPIO reset support if that's > architected already in port registers ? I am pretty sure there is a > reason behind it (and forgive me the dumb question) and I would like to > have it written in the commit log. > > Thanks, > Lorenzo Each PCIe controller has a dedicated SFIO pin to support PERST# signal. Port register can control only this particular SFIO pin. However, in one of the Nvidia platform, instead of using PCIe SFIO pin, different gpio is routed PCIe slot. This happened because of a confusion in IO ball naming convention. To support this particular platform, driver has provide gpio support. I will update the commit log in V5. Manikanta > >>> + } 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) >>> @@ -2238,6 +2249,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) { >>> @@ -2296,6 +2308,23 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie) >>> if (IS_ERR(rp->base)) >>> return PTR_ERR(rp->base); >>> >>> + label = kasprintf(GFP_KERNEL, "pex-reset-%u", index); >> devm_kasprintf()? >> >> Thierry >> >>> + if (!label) { >>> + dev_err(dev, "failed to create reset GPIO label\n"); >>> + return -ENOMEM; >>> + } >>> + >>> + rp->reset_gpiod = devm_gpiod_get_from_of_node(dev, port, >>> + "reset-gpios", 0, >>> + GPIOD_OUT_LOW, >>> + label); >>> + kfree(label); >>> + if (IS_ERR(rp->reset_gpiod)) { >>> + err = PTR_ERR(rp->reset_gpiod); >>> + dev_err(dev, "failed to get reset GPIO: %d\n", err); >>> + return err; >>> + } >>> + >>> list_add_tail(&rp->list, &pcie->ports); >>> } >>> >>> -- >>> 2.17.1 >>> >
On Fri, Jun 14, 2019 at 04:07:35PM +0530, Manikanta Maddireddy wrote: > > > On 13-Jun-19 8:54 PM, Lorenzo Pieralisi wrote: > > On Tue, Jun 04, 2019 at 03:22:33PM +0200, Thierry Reding wrote: > > > > [...] > > > >>> + } 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_gpiod) { > >>> + gpiod_set_value(port->reset_gpiod, 1); > >> After this the port should be functional, right? I think it'd be better > >> to reverse the logic here and move the polarity of the GPIO into device > >> tree. gpiod_set_value() takes care of inverting the level internally if > >> the GPIO is marked as low-active in DT. > >> > >> The end result is obviously the same, but it makes the usage much > >> clearer. If somebody want to write a DT for their board, they will look > >> at the schematics and see a low-active reset line and may be tempted to > >> describe it as such in DT, but with your current code that would be > >> exactly the wrong way around. > > I agree with Thierry here, you should change the logic. > > > > Question: what's the advantage of adding GPIO reset support if that's > > architected already in port registers ? I am pretty sure there is a > > reason behind it (and forgive me the dumb question) and I would like to > > have it written in the commit log. > > > > Thanks, > > Lorenzo > > Each PCIe controller has a dedicated SFIO pin to support PERST# > signal. Port register can control only this particular SFIO pin. > However, in one of the Nvidia platform, instead of using PCIe SFIO > pin, different gpio is routed PCIe slot. This happened because of a > confusion in IO ball naming convention. To support this particular > platform, driver has provide gpio support. I will update the commit > log in V5. What happens on that platform where you trigger reset through a port register with : value = afi_readl(port->pcie, ctrl); value |= AFI_PEX_CTRL_RST; afi_writel(port->pcie, value, ctrl); (imagine the DT is not updated for instance or on current mainline) ? Lorenzo > Manikanta > > > > >>> + } 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) > >>> @@ -2238,6 +2249,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) { > >>> @@ -2296,6 +2308,23 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie) > >>> if (IS_ERR(rp->base)) > >>> return PTR_ERR(rp->base); > >>> > >>> + label = kasprintf(GFP_KERNEL, "pex-reset-%u", index); > >> devm_kasprintf()? > >> > >> Thierry > >> > >>> + if (!label) { > >>> + dev_err(dev, "failed to create reset GPIO label\n"); > >>> + return -ENOMEM; > >>> + } > >>> + > >>> + rp->reset_gpiod = devm_gpiod_get_from_of_node(dev, port, > >>> + "reset-gpios", 0, > >>> + GPIOD_OUT_LOW, > >>> + label); > >>> + kfree(label); > >>> + if (IS_ERR(rp->reset_gpiod)) { > >>> + err = PTR_ERR(rp->reset_gpiod); > >>> + dev_err(dev, "failed to get reset GPIO: %d\n", err); > >>> + return err; > >>> + } > >>> + > >>> list_add_tail(&rp->list, &pcie->ports); > >>> } > >>> > >>> -- > >>> 2.17.1 > >>> > > >
On 14-Jun-19 8:02 PM, Lorenzo Pieralisi wrote: > On Fri, Jun 14, 2019 at 04:07:35PM +0530, Manikanta Maddireddy wrote: >> >> On 13-Jun-19 8:54 PM, Lorenzo Pieralisi wrote: >>> On Tue, Jun 04, 2019 at 03:22:33PM +0200, Thierry Reding wrote: >>> >>> [...] >>> >>>>> + } 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_gpiod) { >>>>> + gpiod_set_value(port->reset_gpiod, 1); >>>> After this the port should be functional, right? I think it'd be better >>>> to reverse the logic here and move the polarity of the GPIO into device >>>> tree. gpiod_set_value() takes care of inverting the level internally if >>>> the GPIO is marked as low-active in DT. >>>> >>>> The end result is obviously the same, but it makes the usage much >>>> clearer. If somebody want to write a DT for their board, they will look >>>> at the schematics and see a low-active reset line and may be tempted to >>>> describe it as such in DT, but with your current code that would be >>>> exactly the wrong way around. >>> I agree with Thierry here, you should change the logic. >>> >>> Question: what's the advantage of adding GPIO reset support if that's >>> architected already in port registers ? I am pretty sure there is a >>> reason behind it (and forgive me the dumb question) and I would like to >>> have it written in the commit log. >>> >>> Thanks, >>> Lorenzo >> Each PCIe controller has a dedicated SFIO pin to support PERST# >> signal. Port register can control only this particular SFIO pin. >> However, in one of the Nvidia platform, instead of using PCIe SFIO >> pin, different gpio is routed PCIe slot. This happened because of a >> confusion in IO ball naming convention. To support this particular >> platform, driver has provide gpio support. I will update the commit >> log in V5. > What happens on that platform where you trigger reset through a port > register with : > > value = afi_readl(port->pcie, ctrl); > value |= AFI_PEX_CTRL_RST; > afi_writel(port->pcie, value, ctrl); > > (imagine the DT is not updated for instance or on current > mainline) ? > > Lorenzo Lets take an example of PCIe controller-0, SFIO ball name which is controlled by the port-0 register is PEX_L0_RST. It will deassert PEX_L0_RST SFIO line but it doesn't go to PCIe slot, so fundamental reset(PERST# deassert) is not applied to the endpoint connected to that slot. Manikanta >> Manikanta >> >>>>> + } 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) >>>>> @@ -2238,6 +2249,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) { >>>>> @@ -2296,6 +2308,23 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie) >>>>> if (IS_ERR(rp->base)) >>>>> return PTR_ERR(rp->base); >>>>> >>>>> + label = kasprintf(GFP_KERNEL, "pex-reset-%u", index); >>>> devm_kasprintf()? >>>> >>>> Thierry >>>> >>>>> + if (!label) { >>>>> + dev_err(dev, "failed to create reset GPIO label\n"); >>>>> + return -ENOMEM; >>>>> + } >>>>> + >>>>> + rp->reset_gpiod = devm_gpiod_get_from_of_node(dev, port, >>>>> + "reset-gpios", 0, >>>>> + GPIOD_OUT_LOW, >>>>> + label); >>>>> + kfree(label); >>>>> + if (IS_ERR(rp->reset_gpiod)) { >>>>> + err = PTR_ERR(rp->reset_gpiod); >>>>> + dev_err(dev, "failed to get reset GPIO: %d\n", err); >>>>> + return err; >>>>> + } >>>>> + >>>>> list_add_tail(&rp->list, &pcie->ports); >>>>> } >>>>> >>>>> -- >>>>> 2.17.1 >>>>>
On Fri, Jun 14, 2019 at 08:08:26PM +0530, Manikanta Maddireddy wrote: > > > On 14-Jun-19 8:02 PM, Lorenzo Pieralisi wrote: > > On Fri, Jun 14, 2019 at 04:07:35PM +0530, Manikanta Maddireddy wrote: > >> > >> On 13-Jun-19 8:54 PM, Lorenzo Pieralisi wrote: > >>> On Tue, Jun 04, 2019 at 03:22:33PM +0200, Thierry Reding wrote: > >>> > >>> [...] > >>> > >>>>> + } 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_gpiod) { > >>>>> + gpiod_set_value(port->reset_gpiod, 1); > >>>> After this the port should be functional, right? I think it'd be better > >>>> to reverse the logic here and move the polarity of the GPIO into device > >>>> tree. gpiod_set_value() takes care of inverting the level internally if > >>>> the GPIO is marked as low-active in DT. > >>>> > >>>> The end result is obviously the same, but it makes the usage much > >>>> clearer. If somebody want to write a DT for their board, they will look > >>>> at the schematics and see a low-active reset line and may be tempted to > >>>> describe it as such in DT, but with your current code that would be > >>>> exactly the wrong way around. > >>> I agree with Thierry here, you should change the logic. > >>> > >>> Question: what's the advantage of adding GPIO reset support if that's > >>> architected already in port registers ? I am pretty sure there is a > >>> reason behind it (and forgive me the dumb question) and I would like to > >>> have it written in the commit log. > >>> > >>> Thanks, > >>> Lorenzo > >> Each PCIe controller has a dedicated SFIO pin to support PERST# > >> signal. Port register can control only this particular SFIO pin. > >> However, in one of the Nvidia platform, instead of using PCIe SFIO > >> pin, different gpio is routed PCIe slot. This happened because of a > >> confusion in IO ball naming convention. To support this particular > >> platform, driver has provide gpio support. I will update the commit > >> log in V5. > > What happens on that platform where you trigger reset through a port > > register with : > > > > value = afi_readl(port->pcie, ctrl); > > value |= AFI_PEX_CTRL_RST; > > afi_writel(port->pcie, value, ctrl); > > > > (imagine the DT is not updated for instance or on current > > mainline) ? > > > > Lorenzo > > Lets take an example of PCIe controller-0, SFIO ball name which is > controlled by the port-0 register is PEX_L0_RST. It will deassert > PEX_L0_RST SFIO line but it doesn't go to PCIe slot, so fundamental > reset(PERST# deassert) is not applied to the endpoint connected to > that slot. That's the point I am making, if the reset is not applied nothing will work (provided PEX_L0_RST does not do any damage either). For the platform in question you should make reset-gpios mandatory and fail if not present (instead of toggling the wrong reset line) there is no chance the driver can work without that property AFAICS. Lorenzo > > > Manikanta > > >> Manikanta > >> > >>>>> + } 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) > >>>>> @@ -2238,6 +2249,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) { > >>>>> @@ -2296,6 +2308,23 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie) > >>>>> if (IS_ERR(rp->base)) > >>>>> return PTR_ERR(rp->base); > >>>>> > >>>>> + label = kasprintf(GFP_KERNEL, "pex-reset-%u", index); > >>>> devm_kasprintf()? > >>>> > >>>> Thierry > >>>> > >>>>> + if (!label) { > >>>>> + dev_err(dev, "failed to create reset GPIO label\n"); > >>>>> + return -ENOMEM; > >>>>> + } > >>>>> + > >>>>> + rp->reset_gpiod = devm_gpiod_get_from_of_node(dev, port, > >>>>> + "reset-gpios", 0, > >>>>> + GPIOD_OUT_LOW, > >>>>> + label); > >>>>> + kfree(label); > >>>>> + if (IS_ERR(rp->reset_gpiod)) { > >>>>> + err = PTR_ERR(rp->reset_gpiod); > >>>>> + dev_err(dev, "failed to get reset GPIO: %d\n", err); > >>>>> + return err; > >>>>> + } > >>>>> + > >>>>> list_add_tail(&rp->list, &pcie->ports); > >>>>> } > >>>>> > >>>>> -- > >>>>> 2.17.1 > >>>>> >
On 14-Jun-19 8:20 PM, Lorenzo Pieralisi wrote: > On Fri, Jun 14, 2019 at 08:08:26PM +0530, Manikanta Maddireddy wrote: >> >> On 14-Jun-19 8:02 PM, Lorenzo Pieralisi wrote: >>> On Fri, Jun 14, 2019 at 04:07:35PM +0530, Manikanta Maddireddy wrote: >>>> On 13-Jun-19 8:54 PM, Lorenzo Pieralisi wrote: >>>>> On Tue, Jun 04, 2019 at 03:22:33PM +0200, Thierry Reding wrote: >>>>> >>>>> [...] >>>>> >>>>>>> + } 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_gpiod) { >>>>>>> + gpiod_set_value(port->reset_gpiod, 1); >>>>>> After this the port should be functional, right? I think it'd be better >>>>>> to reverse the logic here and move the polarity of the GPIO into device >>>>>> tree. gpiod_set_value() takes care of inverting the level internally if >>>>>> the GPIO is marked as low-active in DT. >>>>>> >>>>>> The end result is obviously the same, but it makes the usage much >>>>>> clearer. If somebody want to write a DT for their board, they will look >>>>>> at the schematics and see a low-active reset line and may be tempted to >>>>>> describe it as such in DT, but with your current code that would be >>>>>> exactly the wrong way around. >>>>> I agree with Thierry here, you should change the logic. >>>>> >>>>> Question: what's the advantage of adding GPIO reset support if that's >>>>> architected already in port registers ? I am pretty sure there is a >>>>> reason behind it (and forgive me the dumb question) and I would like to >>>>> have it written in the commit log. >>>>> >>>>> Thanks, >>>>> Lorenzo >>>> Each PCIe controller has a dedicated SFIO pin to support PERST# >>>> signal. Port register can control only this particular SFIO pin. >>>> However, in one of the Nvidia platform, instead of using PCIe SFIO >>>> pin, different gpio is routed PCIe slot. This happened because of a >>>> confusion in IO ball naming convention. To support this particular >>>> platform, driver has provide gpio support. I will update the commit >>>> log in V5. >>> What happens on that platform where you trigger reset through a port >>> register with : >>> >>> value = afi_readl(port->pcie, ctrl); >>> value |= AFI_PEX_CTRL_RST; >>> afi_writel(port->pcie, value, ctrl); >>> >>> (imagine the DT is not updated for instance or on current >>> mainline) ? >>> >>> Lorenzo >> Lets take an example of PCIe controller-0, SFIO ball name which is >> controlled by the port-0 register is PEX_L0_RST. It will deassert >> PEX_L0_RST SFIO line but it doesn't go to PCIe slot, so fundamental >> reset(PERST# deassert) is not applied to the endpoint connected to >> that slot. > That's the point I am making, if the reset is not applied nothing > will work (provided PEX_L0_RST does not do any damage either). > > For the platform in question you should make reset-gpios mandatory and > fail if not present (instead of toggling the wrong reset line) there is > no chance the driver can work without that property AFAICS. > > Lorenzo In upstream kernel, device tree file is not available for the platform in question. That is the reason for not send device tree patch as part of this series. Manikanta >> >> Manikanta >> >>>> Manikanta >>>> >>>>>>> + } 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) >>>>>>> @@ -2238,6 +2249,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) { >>>>>>> @@ -2296,6 +2308,23 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie) >>>>>>> if (IS_ERR(rp->base)) >>>>>>> return PTR_ERR(rp->base); >>>>>>> >>>>>>> + label = kasprintf(GFP_KERNEL, "pex-reset-%u", index); >>>>>> devm_kasprintf()? >>>>>> >>>>>> Thierry >>>>>> >>>>>>> + if (!label) { >>>>>>> + dev_err(dev, "failed to create reset GPIO label\n"); >>>>>>> + return -ENOMEM; >>>>>>> + } >>>>>>> + >>>>>>> + rp->reset_gpiod = devm_gpiod_get_from_of_node(dev, port, >>>>>>> + "reset-gpios", 0, >>>>>>> + GPIOD_OUT_LOW, >>>>>>> + label); >>>>>>> + kfree(label); >>>>>>> + if (IS_ERR(rp->reset_gpiod)) { >>>>>>> + err = PTR_ERR(rp->reset_gpiod); >>>>>>> + dev_err(dev, "failed to get reset GPIO: %d\n", err); >>>>>>> + return err; >>>>>>> + } >>>>>>> + >>>>>>> list_add_tail(&rp->list, &pcie->ports); >>>>>>> } >>>>>>> >>>>>>> -- >>>>>>> 2.17.1 >>>>>>>
On Fri, Jun 14, 2019 at 03:50:23PM +0100, Lorenzo Pieralisi wrote: > On Fri, Jun 14, 2019 at 08:08:26PM +0530, Manikanta Maddireddy wrote: > > > > > > On 14-Jun-19 8:02 PM, Lorenzo Pieralisi wrote: > > > On Fri, Jun 14, 2019 at 04:07:35PM +0530, Manikanta Maddireddy wrote: > > >> > > >> On 13-Jun-19 8:54 PM, Lorenzo Pieralisi wrote: > > >>> On Tue, Jun 04, 2019 at 03:22:33PM +0200, Thierry Reding wrote: > > >>> > > >>> [...] > > >>> > > >>>>> + } 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_gpiod) { > > >>>>> + gpiod_set_value(port->reset_gpiod, 1); > > >>>> After this the port should be functional, right? I think it'd be better > > >>>> to reverse the logic here and move the polarity of the GPIO into device > > >>>> tree. gpiod_set_value() takes care of inverting the level internally if > > >>>> the GPIO is marked as low-active in DT. > > >>>> > > >>>> The end result is obviously the same, but it makes the usage much > > >>>> clearer. If somebody want to write a DT for their board, they will look > > >>>> at the schematics and see a low-active reset line and may be tempted to > > >>>> describe it as such in DT, but with your current code that would be > > >>>> exactly the wrong way around. > > >>> I agree with Thierry here, you should change the logic. > > >>> > > >>> Question: what's the advantage of adding GPIO reset support if that's > > >>> architected already in port registers ? I am pretty sure there is a > > >>> reason behind it (and forgive me the dumb question) and I would like to > > >>> have it written in the commit log. > > >>> > > >>> Thanks, > > >>> Lorenzo > > >> Each PCIe controller has a dedicated SFIO pin to support PERST# > > >> signal. Port register can control only this particular SFIO pin. > > >> However, in one of the Nvidia platform, instead of using PCIe SFIO > > >> pin, different gpio is routed PCIe slot. This happened because of a > > >> confusion in IO ball naming convention. To support this particular > > >> platform, driver has provide gpio support. I will update the commit > > >> log in V5. > > > What happens on that platform where you trigger reset through a port > > > register with : > > > > > > value = afi_readl(port->pcie, ctrl); > > > value |= AFI_PEX_CTRL_RST; > > > afi_writel(port->pcie, value, ctrl); > > > > > > (imagine the DT is not updated for instance or on current > > > mainline) ? > > > > > > Lorenzo > > > > Lets take an example of PCIe controller-0, SFIO ball name which is > > controlled by the port-0 register is PEX_L0_RST. It will deassert > > PEX_L0_RST SFIO line but it doesn't go to PCIe slot, so fundamental > > reset(PERST# deassert) is not applied to the endpoint connected to > > that slot. > > That's the point I am making, if the reset is not applied nothing > will work (provided PEX_L0_RST does not do any damage either). > > For the platform in question you should make reset-gpios mandatory and > fail if not present (instead of toggling the wrong reset line) there is > no chance the driver can work without that property AFAICS. I'm not sure I understand what you're proposing here. Are you suggesting that we put a check in the driver to see if we're running on a specific board and then fail if the reset-gpios are not there? Thierry
On Fri, Jun 14, 2019 at 05:23:04PM +0200, Thierry Reding wrote: > On Fri, Jun 14, 2019 at 03:50:23PM +0100, Lorenzo Pieralisi wrote: > > On Fri, Jun 14, 2019 at 08:08:26PM +0530, Manikanta Maddireddy wrote: > > > > > > > > > On 14-Jun-19 8:02 PM, Lorenzo Pieralisi wrote: > > > > On Fri, Jun 14, 2019 at 04:07:35PM +0530, Manikanta Maddireddy wrote: > > > >> > > > >> On 13-Jun-19 8:54 PM, Lorenzo Pieralisi wrote: > > > >>> On Tue, Jun 04, 2019 at 03:22:33PM +0200, Thierry Reding wrote: > > > >>> > > > >>> [...] > > > >>> > > > >>>>> + } 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_gpiod) { > > > >>>>> + gpiod_set_value(port->reset_gpiod, 1); > > > >>>> After this the port should be functional, right? I think it'd be better > > > >>>> to reverse the logic here and move the polarity of the GPIO into device > > > >>>> tree. gpiod_set_value() takes care of inverting the level internally if > > > >>>> the GPIO is marked as low-active in DT. > > > >>>> > > > >>>> The end result is obviously the same, but it makes the usage much > > > >>>> clearer. If somebody want to write a DT for their board, they will look > > > >>>> at the schematics and see a low-active reset line and may be tempted to > > > >>>> describe it as such in DT, but with your current code that would be > > > >>>> exactly the wrong way around. > > > >>> I agree with Thierry here, you should change the logic. > > > >>> > > > >>> Question: what's the advantage of adding GPIO reset support if that's > > > >>> architected already in port registers ? I am pretty sure there is a > > > >>> reason behind it (and forgive me the dumb question) and I would like to > > > >>> have it written in the commit log. > > > >>> > > > >>> Thanks, > > > >>> Lorenzo > > > >> Each PCIe controller has a dedicated SFIO pin to support PERST# > > > >> signal. Port register can control only this particular SFIO pin. > > > >> However, in one of the Nvidia platform, instead of using PCIe SFIO > > > >> pin, different gpio is routed PCIe slot. This happened because of a > > > >> confusion in IO ball naming convention. To support this particular > > > >> platform, driver has provide gpio support. I will update the commit > > > >> log in V5. > > > > What happens on that platform where you trigger reset through a port > > > > register with : > > > > > > > > value = afi_readl(port->pcie, ctrl); > > > > value |= AFI_PEX_CTRL_RST; > > > > afi_writel(port->pcie, value, ctrl); > > > > > > > > (imagine the DT is not updated for instance or on current > > > > mainline) ? > > > > > > > > Lorenzo > > > > > > Lets take an example of PCIe controller-0, SFIO ball name which is > > > controlled by the port-0 register is PEX_L0_RST. It will deassert > > > PEX_L0_RST SFIO line but it doesn't go to PCIe slot, so fundamental > > > reset(PERST# deassert) is not applied to the endpoint connected to > > > that slot. > > > > That's the point I am making, if the reset is not applied nothing > > will work (provided PEX_L0_RST does not do any damage either). > > > > For the platform in question you should make reset-gpios mandatory and > > fail if not present (instead of toggling the wrong reset line) there is > > no chance the driver can work without that property AFAICS. > > I'm not sure I understand what you're proposing here. Are you suggesting > that we put a check in the driver to see if we're running on a specific > board and then fail if the reset-gpios are not there? I am just trying to understand what this patch does. By reading it again it looks like it makes GPIO PERST# reset mandatory for all platforms supported by this driver (because if the driver does not grab an handle to the GPIO tegra_pcie_parse_dt() fails), if I read the code correctly, apologies if not. Which makes me question the check: if (port->reset_gpiod) { gpiod_set_value(port->reset_gpiod, 0); in tegra_pcie_port_reset(), if we are there port->reset_gpiod can't be NULL or I am missing something and also make: } else { value = afi_readl(port->pcie, ctrl); value &= ~AFI_PEX_CTRL_RST; afi_writel(port->pcie, value, ctrl); } path dead code. Is this GPIO based #PERST a per-platform requirement or you want to update the driver to always use GPIO based #PERST ? And if it is a per-platform requirement I assume that a missing DT property describing the GPIO #PERST should cause a probe failure, not a fallback to port registers reset (which may have unintended consequences). From the commit log it is not clear what this patch does and for what reason it does it but it should be, let's define it here and update the log accordingly for everyone's benefit. Lorenzo
On 14-Jun-19 9:29 PM, Lorenzo Pieralisi wrote: > On Fri, Jun 14, 2019 at 05:23:04PM +0200, Thierry Reding wrote: >> On Fri, Jun 14, 2019 at 03:50:23PM +0100, Lorenzo Pieralisi wrote: >>> On Fri, Jun 14, 2019 at 08:08:26PM +0530, Manikanta Maddireddy wrote: >>>> >>>> On 14-Jun-19 8:02 PM, Lorenzo Pieralisi wrote: >>>>> On Fri, Jun 14, 2019 at 04:07:35PM +0530, Manikanta Maddireddy wrote: >>>>>> On 13-Jun-19 8:54 PM, Lorenzo Pieralisi wrote: >>>>>>> On Tue, Jun 04, 2019 at 03:22:33PM +0200, Thierry Reding wrote: >>>>>>> >>>>>>> [...] >>>>>>> >>>>>>>>> + } 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_gpiod) { >>>>>>>>> + gpiod_set_value(port->reset_gpiod, 1); >>>>>>>> After this the port should be functional, right? I think it'd be better >>>>>>>> to reverse the logic here and move the polarity of the GPIO into device >>>>>>>> tree. gpiod_set_value() takes care of inverting the level internally if >>>>>>>> the GPIO is marked as low-active in DT. >>>>>>>> >>>>>>>> The end result is obviously the same, but it makes the usage much >>>>>>>> clearer. If somebody want to write a DT for their board, they will look >>>>>>>> at the schematics and see a low-active reset line and may be tempted to >>>>>>>> describe it as such in DT, but with your current code that would be >>>>>>>> exactly the wrong way around. >>>>>>> I agree with Thierry here, you should change the logic. >>>>>>> >>>>>>> Question: what's the advantage of adding GPIO reset support if that's >>>>>>> architected already in port registers ? I am pretty sure there is a >>>>>>> reason behind it (and forgive me the dumb question) and I would like to >>>>>>> have it written in the commit log. >>>>>>> >>>>>>> Thanks, >>>>>>> Lorenzo >>>>>> Each PCIe controller has a dedicated SFIO pin to support PERST# >>>>>> signal. Port register can control only this particular SFIO pin. >>>>>> However, in one of the Nvidia platform, instead of using PCIe SFIO >>>>>> pin, different gpio is routed PCIe slot. This happened because of a >>>>>> confusion in IO ball naming convention. To support this particular >>>>>> platform, driver has provide gpio support. I will update the commit >>>>>> log in V5. >>>>> What happens on that platform where you trigger reset through a port >>>>> register with : >>>>> >>>>> value = afi_readl(port->pcie, ctrl); >>>>> value |= AFI_PEX_CTRL_RST; >>>>> afi_writel(port->pcie, value, ctrl); >>>>> >>>>> (imagine the DT is not updated for instance or on current >>>>> mainline) ? >>>>> >>>>> Lorenzo >>>> Lets take an example of PCIe controller-0, SFIO ball name which is >>>> controlled by the port-0 register is PEX_L0_RST. It will deassert >>>> PEX_L0_RST SFIO line but it doesn't go to PCIe slot, so fundamental >>>> reset(PERST# deassert) is not applied to the endpoint connected to >>>> that slot. >>> That's the point I am making, if the reset is not applied nothing >>> will work (provided PEX_L0_RST does not do any damage either). >>> >>> For the platform in question you should make reset-gpios mandatory and >>> fail if not present (instead of toggling the wrong reset line) there is >>> no chance the driver can work without that property AFAICS. >> I'm not sure I understand what you're proposing here. Are you suggesting >> that we put a check in the driver to see if we're running on a specific >> board and then fail if the reset-gpios are not there? > I am just trying to understand what this patch does. By reading it again > it looks like it makes GPIO PERST# reset mandatory for all platforms > supported by this driver (because if the driver does not grab an handle > to the GPIO tegra_pcie_parse_dt() fails), if I read the code correctly, > apologies if not. > > Which makes me question the check: > > if (port->reset_gpiod) { > gpiod_set_value(port->reset_gpiod, 0); > > in tegra_pcie_port_reset(), if we are there port->reset_gpiod can't be > NULL or I am missing something and also make: > > } else { > value = afi_readl(port->pcie, ctrl); > value &= ~AFI_PEX_CTRL_RST; > afi_writel(port->pcie, value, ctrl); > } > > path dead code. > > Is this GPIO based #PERST a per-platform requirement or you want > to update the driver to always use GPIO based #PERST ? > > And if it is a per-platform requirement I assume that a missing > DT property describing the GPIO #PERST should cause a probe failure, > not a fallback to port registers reset (which may have unintended > consequences). > > From the commit log it is not clear what this patch does and for what > reason it does it but it should be, let's define it here and update the > log accordingly for everyone's benefit. > > Lorenzo GPIO based PERST# is per-platform requirement. If DT prop is not present, then devm_gpiod_get_from_of_node() returns NULL gpio_desc. struct gpio_desc *gpiod_get_from_of_node(struct device_node *node, const char *propname, int index, enum gpiod_flags dflags, const char *label) { struct gpio_desc *desc; unsigned long lflags = 0; enum of_gpio_flags flags; bool active_low = false; bool single_ended = false; bool open_drain = false; bool transitory = false; int ret; desc = of_get_named_gpiod_flags(node, propname, index, &flags); if (!desc || IS_ERR(desc)) { */* If it is not there, just return NULL */****if (PTR_ERR(desc) == -ENOENT)****return NULL;* return desc; } ... } Manikanta
On Fri, Jun 14, 2019 at 10:00:49PM +0530, Manikanta Maddireddy wrote: [...] > GPIO based PERST# is per-platform requirement. > If DT prop is not present, then devm_gpiod_get_from_of_node() returns > NULL gpio_desc. > > struct gpio_desc *gpiod_get_from_of_node(struct device_node *node, > const char *propname, int index, > enum gpiod_flags dflags, > const char *label) > { > struct gpio_desc *desc; > unsigned long lflags = 0; > enum of_gpio_flags flags; > bool active_low = false; > bool single_ended = false; > bool open_drain = false; > bool transitory = false; > int ret; > > desc = of_get_named_gpiod_flags(node, propname, > index, &flags); > > if (!desc || IS_ERR(desc)) { > */* If it is not there, just return NULL */****if (PTR_ERR(desc) == -ENOENT)****return NULL;* > return desc; > } > ... > > } Ok. My point then is that you have no way to enforce this requirement on platforms that actually need it, I do not even know if there is a way you can do it (I was thinking along the lines of using a compatible string to detect whether the GPIO #PERST reset is mandatory) but maybe this is not even a SOC property. Maybe what I am asking is overkill, I just wanted to understand. I was just asking a question to understand how you handle the case where a GPIO pin definition is missing in DT for a platform that actually needs it, the driver will probe but nothing will work. It would be good to describe this and capture it in the commit log. Thanks, Lorenzo
On 14-Jun-19 10:23 PM, Lorenzo Pieralisi wrote: > On Fri, Jun 14, 2019 at 10:00:49PM +0530, Manikanta Maddireddy wrote: > > [...] > >> GPIO based PERST# is per-platform requirement. >> If DT prop is not present, then devm_gpiod_get_from_of_node() returns >> NULL gpio_desc. >> >> struct gpio_desc *gpiod_get_from_of_node(struct device_node *node, >> const char *propname, int index, >> enum gpiod_flags dflags, >> const char *label) >> { >> struct gpio_desc *desc; >> unsigned long lflags = 0; >> enum of_gpio_flags flags; >> bool active_low = false; >> bool single_ended = false; >> bool open_drain = false; >> bool transitory = false; >> int ret; >> >> desc = of_get_named_gpiod_flags(node, propname, >> index, &flags); >> >> if (!desc || IS_ERR(desc)) { >> */* If it is not there, just return NULL */****if (PTR_ERR(desc) == -ENOENT)****return NULL;* >> return desc; >> } >> ... >> >> } > Ok. My point then is that you have no way to enforce this requirement on > platforms that actually need it, I do not even know if there is a > way you can do it (I was thinking along the lines of using a > compatible string to detect whether the GPIO #PERST reset is mandatory) > but maybe this is not even a SOC property. > > Maybe what I am asking is overkill, I just wanted to understand. > > I was just asking a question to understand how you handle the case > where a GPIO pin definition is missing in DT for a platform that > actually needs it, the driver will probe but nothing will work. > > It would be good to describe this and capture it in the commit log. > > Thanks, > Lorenzo I can't think of a easy way to enforce this requirement. As you said compatible string is per SOC, so we can't use it for a platform. This issue is present on only one platform, so it is hard to miss the DT property. That is the reason for publishing this patch with out this enforcement in driver. I thought for changing PERST# to GPIO for all platform, but testing is a tedious job. Also I don't have Tegra20 and Tegra30 platforms. Do you want me to drop the patch or update the limitation in the commit log? Manikanta
On Fri, Jun 14, 2019 at 10:53:13PM +0530, Manikanta Maddireddy wrote: [...] > > Ok. My point then is that you have no way to enforce this requirement on > > platforms that actually need it, I do not even know if there is a > > way you can do it (I was thinking along the lines of using a > > compatible string to detect whether the GPIO #PERST reset is mandatory) > > but maybe this is not even a SOC property. > > > > Maybe what I am asking is overkill, I just wanted to understand. > > > > I was just asking a question to understand how you handle the case > > where a GPIO pin definition is missing in DT for a platform that > > actually needs it, the driver will probe but nothing will work. > > > > It would be good to describe this and capture it in the commit log. > > > > Thanks, > > Lorenzo > > I can't think of a easy way to enforce this requirement. As you said > compatible string is per SOC, so we can't use it for a platform. > This issue is present on only one platform, so it is hard to miss the > DT property. That is the reason for publishing this patch with out this > enforcement in driver. > > I thought for changing PERST# to GPIO for all platform, but testing is > a tedious job. Also I don't have Tegra20 and Tegra30 platforms. I can't help with that. > Do you want me to drop the patch or update the limitation in the commit > log? It is Thierry's call, if he is OK with it fine by me, please do update the commit log, it will help everybody understand. Lorenzo
On 17-Jun-19 3:18 PM, Lorenzo Pieralisi wrote: > On Fri, Jun 14, 2019 at 10:53:13PM +0530, Manikanta Maddireddy wrote: > > [...] > >>> Ok. My point then is that you have no way to enforce this requirement on >>> platforms that actually need it, I do not even know if there is a >>> way you can do it (I was thinking along the lines of using a >>> compatible string to detect whether the GPIO #PERST reset is mandatory) >>> but maybe this is not even a SOC property. >>> >>> Maybe what I am asking is overkill, I just wanted to understand. >>> >>> I was just asking a question to understand how you handle the case >>> where a GPIO pin definition is missing in DT for a platform that >>> actually needs it, the driver will probe but nothing will work. >>> >>> It would be good to describe this and capture it in the commit log. >>> >>> Thanks, >>> Lorenzo >> I can't think of a easy way to enforce this requirement. As you said >> compatible string is per SOC, so we can't use it for a platform. >> This issue is present on only one platform, so it is hard to miss the >> DT property. That is the reason for publishing this patch with out this >> enforcement in driver. >> >> I thought for changing PERST# to GPIO for all platform, but testing is >> a tedious job. Also I don't have Tegra20 and Tegra30 platforms. > I can't help with that. > >> Do you want me to drop the patch or update the limitation in the commit >> log? > It is Thierry's call, if he is OK with it fine by me, please do > update the commit log, it will help everybody understand. > > Lorenzo Sure, I will update the commit log in V5. Please let me know if you completed reviewing this series, I will send V5 addressing review comments in this patch. Manikanta
On Mon, Jun 17, 2019 at 03:57:09PM +0530, Manikanta Maddireddy wrote: > > > On 17-Jun-19 3:18 PM, Lorenzo Pieralisi wrote: > > On Fri, Jun 14, 2019 at 10:53:13PM +0530, Manikanta Maddireddy wrote: > > > > [...] > > > >>> Ok. My point then is that you have no way to enforce this requirement on > >>> platforms that actually need it, I do not even know if there is a > >>> way you can do it (I was thinking along the lines of using a > >>> compatible string to detect whether the GPIO #PERST reset is mandatory) > >>> but maybe this is not even a SOC property. > >>> > >>> Maybe what I am asking is overkill, I just wanted to understand. > >>> > >>> I was just asking a question to understand how you handle the case > >>> where a GPIO pin definition is missing in DT for a platform that > >>> actually needs it, the driver will probe but nothing will work. > >>> > >>> It would be good to describe this and capture it in the commit log. > >>> > >>> Thanks, > >>> Lorenzo > >> I can't think of a easy way to enforce this requirement. As you said > >> compatible string is per SOC, so we can't use it for a platform. > >> This issue is present on only one platform, so it is hard to miss the > >> DT property. That is the reason for publishing this patch with out this > >> enforcement in driver. > >> > >> I thought for changing PERST# to GPIO for all platform, but testing is > >> a tedious job. Also I don't have Tegra20 and Tegra30 platforms. > > I can't help with that. > > > >> Do you want me to drop the patch or update the limitation in the commit > >> log? > > It is Thierry's call, if he is OK with it fine by me, please do > > update the commit log, it will help everybody understand. > > > > Lorenzo > > Sure, I will update the commit log in V5. > Please let me know if you completed reviewing this series, I will > send V5 addressing review comments in this patch. Post v5, we should be able to get it in v5.3, thanks. Lorenzo
On Fri, Jun 14, 2019 at 05:53:53PM +0100, Lorenzo Pieralisi wrote: > On Fri, Jun 14, 2019 at 10:00:49PM +0530, Manikanta Maddireddy wrote: > > [...] > > > GPIO based PERST# is per-platform requirement. > > If DT prop is not present, then devm_gpiod_get_from_of_node() returns > > NULL gpio_desc. > > > > struct gpio_desc *gpiod_get_from_of_node(struct device_node *node, > > const char *propname, int index, > > enum gpiod_flags dflags, > > const char *label) > > { > > struct gpio_desc *desc; > > unsigned long lflags = 0; > > enum of_gpio_flags flags; > > bool active_low = false; > > bool single_ended = false; > > bool open_drain = false; > > bool transitory = false; > > int ret; > > > > desc = of_get_named_gpiod_flags(node, propname, > > index, &flags); > > > > if (!desc || IS_ERR(desc)) { > > */* If it is not there, just return NULL */****if (PTR_ERR(desc) == -ENOENT)****return NULL;* > > return desc; > > } > > ... > > > > } > > Ok. My point then is that you have no way to enforce this requirement on > platforms that actually need it, I do not even know if there is a > way you can do it (I was thinking along the lines of using a > compatible string to detect whether the GPIO #PERST reset is mandatory) > but maybe this is not even a SOC property. So this is definitely not an SoC property. From what Manikanta said, the only reason why we need this is because on one particular design (that we know of), the PCIe #PERST signal was connected to a pin that does not originate from the PCIe controller. This happened by accident. > Maybe what I am asking is overkill, I just wanted to understand. > > I was just asking a question to understand how you handle the case > where a GPIO pin definition is missing in DT for a platform that > actually needs it, the driver will probe but nothing will work. I think we should handle this the way we handle other GPIOs as well. If some device requires a GPIO to be toggled to put it into or take it out of reset, then that's something that needs to be described in DT. In this case it just so happens that typically we don't need to worry about it because the signals are properly connected and then the PCIe controller will do the right with the reset. For the one design where it doesn't work the reset GPIO is a workaround and it's board-specific knowledge that the engineer writing the DT will have to know. I suspect that if the same accident happened on another board, then as part of writing the DT somebody would have noticed that we need an external pin to be hooked up because the SFIO doesn't work. > It would be good to describe this and capture it in the commit log. Agreed. Let's be more verbose about the situation where this is required and make it very clear that this is a workaround for a board design mistake that shouldn't be necessary if best practices are followed. Maybe add that to both the commit message and the device tree bindings to make it difficult for people to miss. Thierry
On Fri, Jun 14, 2019 at 10:53:13PM +0530, Manikanta Maddireddy wrote: > > > On 14-Jun-19 10:23 PM, Lorenzo Pieralisi wrote: > > On Fri, Jun 14, 2019 at 10:00:49PM +0530, Manikanta Maddireddy wrote: > > > > [...] > > > >> GPIO based PERST# is per-platform requirement. > >> If DT prop is not present, then devm_gpiod_get_from_of_node() returns > >> NULL gpio_desc. > >> > >> struct gpio_desc *gpiod_get_from_of_node(struct device_node *node, > >> const char *propname, int index, > >> enum gpiod_flags dflags, > >> const char *label) > >> { > >> struct gpio_desc *desc; > >> unsigned long lflags = 0; > >> enum of_gpio_flags flags; > >> bool active_low = false; > >> bool single_ended = false; > >> bool open_drain = false; > >> bool transitory = false; > >> int ret; > >> > >> desc = of_get_named_gpiod_flags(node, propname, > >> index, &flags); > >> > >> if (!desc || IS_ERR(desc)) { > >> */* If it is not there, just return NULL */****if (PTR_ERR(desc) == -ENOENT)****return NULL;* > >> return desc; > >> } > >> ... > >> > >> } > > Ok. My point then is that you have no way to enforce this requirement on > > platforms that actually need it, I do not even know if there is a > > way you can do it (I was thinking along the lines of using a > > compatible string to detect whether the GPIO #PERST reset is mandatory) > > but maybe this is not even a SOC property. > > > > Maybe what I am asking is overkill, I just wanted to understand. > > > > I was just asking a question to understand how you handle the case > > where a GPIO pin definition is missing in DT for a platform that > > actually needs it, the driver will probe but nothing will work. > > > > It would be good to describe this and capture it in the commit log. > > > > Thanks, > > Lorenzo > > I can't think of a easy way to enforce this requirement. As you said > compatible string is per SOC, so we can't use it for a platform. > This issue is present on only one platform, so it is hard to miss the > DT property. That is the reason for publishing this patch with out this > enforcement in driver. > > I thought for changing PERST# to GPIO for all platform, but testing is > a tedious job. Also I don't have Tegra20 and Tegra30 platforms. Yeah, let's not go that way. The standard way to do this is to use the SFIO and let the PCIe controller and driver handle this. It's working just fine on all platforms currently supported upstream. Using direct GPIO for PERST# is a workaround, so let's not proliferate unless it is absolutely necessary. With an updated commit message, this is: Acked-by: Thierry Reding <treding@nvidia.com>
diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c index 06b99fcbf382..09b4d384ba38 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> @@ -400,6 +401,8 @@ struct tegra_pcie_port { unsigned int lanes; struct phy **phys; + + struct gpio_desc *reset_gpiod; }; struct tegra_pcie_bus { @@ -583,15 +586,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_gpiod) { + gpiod_set_value(port->reset_gpiod, 0); + } 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_gpiod) { + gpiod_set_value(port->reset_gpiod, 1); + } 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) @@ -2238,6 +2249,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) { @@ -2296,6 +2308,23 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie) if (IS_ERR(rp->base)) return PTR_ERR(rp->base); + label = kasprintf(GFP_KERNEL, "pex-reset-%u", index); + if (!label) { + dev_err(dev, "failed to create reset GPIO label\n"); + return -ENOMEM; + } + + rp->reset_gpiod = devm_gpiod_get_from_of_node(dev, port, + "reset-gpios", 0, + GPIOD_OUT_LOW, + label); + kfree(label); + if (IS_ERR(rp->reset_gpiod)) { + err = PTR_ERR(rp->reset_gpiod); + dev_err(dev, "failed to get reset GPIO: %d\n", err); + return err; + } + list_add_tail(&rp->list, &pcie->ports); }
Add support for GPIO based PERST# signal. GPIO number comes from per port PCIe device tree node. Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com> --- 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 | 41 +++++++++++++++++++++++++----- 1 file changed, 35 insertions(+), 6 deletions(-)