Message ID | 20200103124404.20662-1-vidyas@nvidia.com |
---|---|
Headers | show |
Series | Add support for PCIe endpoint mode in Tegra194 | expand |
On Fri, Jan 03, 2020 at 06:14:02PM +0530, Vidya Sagar wrote: > Add support for the endpoint mode of Synopsys DesignWare core based > dual mode PCIe controllers present in Tegra194 SoC. > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com> > --- > V2: > * Addressed Bjorn's review comments > * Made changes as part of addressing review comments for other patches > > drivers/pci/controller/dwc/Kconfig | 30 +- > drivers/pci/controller/dwc/pcie-tegra194.c | 782 ++++++++++++++++++++- > 2 files changed, 796 insertions(+), 16 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c [...] > @@ -411,11 +466,66 @@ static irqreturn_t tegra_pcie_rp_irq_handler(struct tegra_pcie_dw *pcie) > return IRQ_HANDLED; > } > > +static irqreturn_t tegra_pcie_ep_irq_handler(struct tegra_pcie_dw *pcie) > +{ > + struct dw_pcie_ep *ep = &pcie->pci.ep; > + int spurious = 1; > + u32 val, tmp; > + > + val = appl_readl(pcie, APPL_INTR_STATUS_L0); > + if (val & APPL_INTR_STATUS_L0_LINK_STATE_INT) { > + val = appl_readl(pcie, APPL_INTR_STATUS_L1_0_0); > + appl_writel(pcie, val, APPL_INTR_STATUS_L1_0_0); > + if (val & APPL_INTR_STATUS_L1_0_0_HOT_RESET_DONE) { > + /* clear any stale PEX_RST interrupt */ > + if (!kfifo_put(&pcie->event_fifo, EP_HOT_RST_DONE)) { > + dev_err(pcie->dev, "EVENT FIFO is full\n"); > + return IRQ_HANDLED; > + } > + wake_up(&pcie->wq); > + } Overall this patch looks a little cluttered. A few blank lines before and after (or between) block statements would help make this more readable, in my opinion. > + if (val & APPL_INTR_STATUS_L1_0_0_RDLH_LINK_UP_CHGED) { > + tmp = appl_readl(pcie, APPL_LINK_STATUS); > + if (tmp & APPL_LINK_STATUS_RDLH_LINK_UP) { > + dev_info(pcie->dev, "Link is up with Host\n"); Do we want this to be an info message? Looks to me like this is mostly useful for debug purposes, as a quick way to check if the link is up. For production use, this would perhaps be better to expose as a sysfs attribute so that userspace can query it at runtime rather than search through kernel logs. > + dw_pcie_ep_linkup(ep); > + } > + } > + spurious = 0; > + } > + > + if (val & APPL_INTR_STATUS_L0_PCI_CMD_EN_INT) { > + val = appl_readl(pcie, APPL_INTR_STATUS_L1_15); > + appl_writel(pcie, val, APPL_INTR_STATUS_L1_15); > + if (val & APPL_INTR_STATUS_L1_15_CFG_BME_CHGED) { > + if (!kfifo_put(&pcie->event_fifo, EP_BME_CHANGE)) { > + dev_err(pcie->dev, "EVENT FIFO is full\n"); > + return IRQ_HANDLED; > + } > + wake_up(&pcie->wq); > + } > + spurious = 0; > + } > + > + if (spurious) { > + dev_warn(pcie->dev, "Random interrupt (STATUS = 0x%08X)\n", > + val); > + appl_writel(pcie, val, APPL_INTR_STATUS_L0); > + } > + > + return IRQ_HANDLED; > +} > + > static irqreturn_t tegra_pcie_irq_handler(int irq, void *arg) > { > struct tegra_pcie_dw *pcie = arg; > > - return tegra_pcie_rp_irq_handler(pcie); > + if (pcie->mode == DW_PCIE_RC_TYPE) > + return tegra_pcie_rp_irq_handler(pcie); > + else if (pcie->mode == DW_PCIE_EP_TYPE) > + return tegra_pcie_ep_irq_handler(pcie); > + > + return IRQ_NONE; > } We already know at probe time whether the controller is in root complex or endpoint mode, right? Couldn't we just install the correct handler rather than multiplex here? It's not a very big deal, but given that these are interrupts, avoiding the additional indirection might be a good idea. [...] > @@ -986,6 +1115,42 @@ static int tegra_pcie_dw_parse_dt(struct tegra_pcie_dw *pcie) > pcie->enable_cdm_check = > of_property_read_bool(np, "snps,enable-cdm-check"); > > + if (pcie->mode == DW_PCIE_RC_TYPE) > + return 0; > + > + /* Endpoint mode specific DT entries */ > + name = devm_kasprintf(pcie->dev, GFP_KERNEL, > + "tegra_pcie_%u_pex_rst_gpio", pcie->cid); > + if (!name) { > + dev_err(pcie->dev, "Failed to create PERST GPIO string\n"); > + return -ENOMEM; > + } > + pcie->pex_rst_gpiod = devm_gpiod_get_from_of_node(pcie->dev, np, If np == pcie->dev.of_node, you can simply use devm_gpiod_get() here, can't you? [...] > +static void pex_ep_event_hot_rst_done(struct tegra_pcie_dw *pcie) > +{ > + u32 val = 0; The initialization here seems unnecessary. [...] > +static int tegra_pcie_ep_raise_legacy_irq(struct tegra_pcie_dw *pcie, u16 irq) > +{ > + /* Tegra194 supports only INTA */ > + if (irq > 1) > + return -EINVAL; > + > + appl_writel(pcie, 1, APPL_LEGACY_INTX); > + mdelay(1); Spinning for 1 ms these days is quite a lot. Does this have to be a busy loop or could you use something like usleep_range(1000, 2000) to allow the CPU to do something else in the meantime? Also, does the legacy INTX pulse have to be a whole millisecond wide? Or could this be shorter? A one millisecond pulse implies a maximum of 1000 interrupts per second, which seems a bit low. > @@ -1440,6 +2178,12 @@ static int tegra_pcie_dw_probe(struct platform_device *pdev) > int ret; > u32 i; > > + match = of_match_device(of_match_ptr(tegra_pcie_dw_of_match), dev); > + if (!match) > + return -EINVAL; > + > + data = (struct tegra_pcie_dw_of_data *)match->data; of_device_get_match_data()? > + > pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL); > if (!pcie) > return -ENOMEM; > @@ -1449,6 +2193,7 @@ static int tegra_pcie_dw_probe(struct platform_device *pdev) > pci->ops = &tegra_dw_pcie_ops; > pp = &pci->pp; > pcie->dev = &pdev->dev; > + pcie->mode = (enum dw_pcie_device_mode)data->mode; > > ret = tegra_pcie_dw_parse_dt(pcie); > if (ret < 0) { > @@ -1462,6 +2207,9 @@ static int tegra_pcie_dw_probe(struct platform_device *pdev) > return ret; > } > > + if (pcie->pex_refclk_sel_gpiod) > + gpiod_set_value(pcie->pex_refclk_sel_gpiod, 1); > + > pcie->pex_ctl_supply = devm_regulator_get(dev, "vddio-pex-ctl"); > if (IS_ERR(pcie->pex_ctl_supply)) { > ret = PTR_ERR(pcie->pex_ctl_supply); > @@ -1570,11 +2318,24 @@ static int tegra_pcie_dw_probe(struct platform_device *pdev) > > platform_set_drvdata(pdev, pcie); > > - ret = tegra_pcie_config_rp(pcie); > - if (ret && ret != -ENOMEDIUM) > - goto fail; > - else > - return 0; > + switch (pcie->mode) { > + case DW_PCIE_RC_TYPE: > + ret = tegra_pcie_config_rp(pcie); > + if (ret && ret != -ENOMEDIUM) > + goto fail; > + else > + return 0; > + break; > + > + case DW_PCIE_EP_TYPE: > + ret = tegra_pcie_config_ep(pcie, pdev); > + if (ret < 0) > + goto fail; > + break; > + > + default: > + dev_err(dev, "Invalid PCIe device type %d\n", pcie->mode); > + } > > fail: > tegra_bpmp_put(pcie->bpmp); > @@ -1593,6 +2354,8 @@ static int tegra_pcie_dw_remove(struct platform_device *pdev) > pm_runtime_put_sync(pcie->dev); > pm_runtime_disable(pcie->dev); > tegra_bpmp_put(pcie->bpmp); > + if (pcie->pex_refclk_sel_gpiod) > + gpiod_set_value(pcie->pex_refclk_sel_gpiod, 0); > > return 0; > } > @@ -1697,13 +2460,6 @@ static void tegra_pcie_dw_shutdown(struct platform_device *pdev) > __deinit_controller(pcie); > } > > -static const struct of_device_id tegra_pcie_dw_of_match[] = { > - { > - .compatible = "nvidia,tegra194-pcie", > - }, > - {}, > -}; > - No need to move this around if you use of_device_get_match_data(). Thierry > static const struct dev_pm_ops tegra_pcie_dw_pm_ops = { > .suspend_late = tegra_pcie_dw_suspend_late, > .suspend_noirq = tegra_pcie_dw_suspend_noirq, > -- > 2.17.1 >
On Fri, Jan 03, 2020 at 06:14:00PM +0530, Vidya Sagar wrote: > Update the firmware header to support uninitialization of UPHY PLL > when the PCIe controller is operating in endpoint mode and host cuts > the PCIe reference clock. > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com> > --- > V2: > * Changed Copyright year from 2019 to 2020 > > include/soc/tegra/bpmp-abi.h | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) Bjorn, Lorenzo, subsequent patches in this series depend on this patch, so I think it'd be best if you took this into the PCI tree along with the DT bindings and the PCI driver changes, so: Acked-by: Thierry Reding <treding@nvidia.com>
On Fri, Jan 03, 2020 at 06:14:02PM +0530, Vidya Sagar wrote: > Add support for the endpoint mode of Synopsys DesignWare core based > dual mode PCIe controllers present in Tegra194 SoC. > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com> > --- > V2: > * Addressed Bjorn's review comments > * Made changes as part of addressing review comments for other patches > > drivers/pci/controller/dwc/Kconfig | 30 +- > drivers/pci/controller/dwc/pcie-tegra194.c | 782 ++++++++++++++++++++- > 2 files changed, 796 insertions(+), 16 deletions(-) > [...] > diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c > index cbe95f0ea0ca..6621ac79efee 100644 > --- a/drivers/pci/controller/dwc/pcie-tegra194.c > +++ b/drivers/pci/controller/dwc/pcie-tegra194.c [...] > @@ -1427,8 +1620,553 @@ static int tegra_pcie_config_rp(struct tegra_pcie_dw *pcie) [...] > +static int tegra_pcie_config_ep(struct tegra_pcie_dw *pcie, > + struct platform_device *pdev) > +{ [...] > + ret = devm_request_irq(dev, pcie->pex_rst_irq, > + tegra_pcie_ep_pex_rst_irq, > + IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING, > + name, (void *)pcie); > + if (ret < 0) { > + dev_err(dev, "Failed to request IRQ for PERST: %d\n", ret); > + return ret; > + } > + disable_irq(pcie->pex_rst_irq); I just came across this while reviewing another patch: it looks like a better way to handle "disabled by default" interrupts is to do this: irq_set_status_flags(rtc->irq, IRQ_NOAUTOEN); before calling devm_request_irq(). See here for an example: http://patchwork.ozlabs.org/patch/1217944/ Thierry
On 1/6/20 6:32 PM, Thierry Reding wrote: > On Fri, Jan 03, 2020 at 06:14:02PM +0530, Vidya Sagar wrote: >> Add support for the endpoint mode of Synopsys DesignWare core based >> dual mode PCIe controllers present in Tegra194 SoC. >> >> Signed-off-by: Vidya Sagar <vidyas@nvidia.com> >> --- >> V2: >> * Addressed Bjorn's review comments >> * Made changes as part of addressing review comments for other patches >> >> drivers/pci/controller/dwc/Kconfig | 30 +- >> drivers/pci/controller/dwc/pcie-tegra194.c | 782 ++++++++++++++++++++- >> 2 files changed, 796 insertions(+), 16 deletions(-) >> >> diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c > [...] >> @@ -411,11 +466,66 @@ static irqreturn_t tegra_pcie_rp_irq_handler(struct tegra_pcie_dw *pcie) >> return IRQ_HANDLED; >> } >> >> +static irqreturn_t tegra_pcie_ep_irq_handler(struct tegra_pcie_dw *pcie) >> +{ >> + struct dw_pcie_ep *ep = &pcie->pci.ep; >> + int spurious = 1; >> + u32 val, tmp; >> + >> + val = appl_readl(pcie, APPL_INTR_STATUS_L0); >> + if (val & APPL_INTR_STATUS_L0_LINK_STATE_INT) { >> + val = appl_readl(pcie, APPL_INTR_STATUS_L1_0_0); >> + appl_writel(pcie, val, APPL_INTR_STATUS_L1_0_0); >> + if (val & APPL_INTR_STATUS_L1_0_0_HOT_RESET_DONE) { >> + /* clear any stale PEX_RST interrupt */ >> + if (!kfifo_put(&pcie->event_fifo, EP_HOT_RST_DONE)) { >> + dev_err(pcie->dev, "EVENT FIFO is full\n"); >> + return IRQ_HANDLED; >> + } >> + wake_up(&pcie->wq); >> + } > > Overall this patch looks a little cluttered. A few blank lines before > and after (or between) block statements would help make this more > readable, in my opinion. Done. > >> + if (val & APPL_INTR_STATUS_L1_0_0_RDLH_LINK_UP_CHGED) { >> + tmp = appl_readl(pcie, APPL_LINK_STATUS); >> + if (tmp & APPL_LINK_STATUS_RDLH_LINK_UP) { >> + dev_info(pcie->dev, "Link is up with Host\n"); > > Do we want this to be an info message? Looks to me like this is mostly > useful for debug purposes, as a quick way to check if the link is up. > For production use, this would perhaps be better to expose as a sysfs > attribute so that userspace can query it at runtime rather than search > through kernel logs. I changed it to dev_dbg message. > >> + dw_pcie_ep_linkup(ep); >> + } >> + } >> + spurious = 0; >> + } >> + >> + if (val & APPL_INTR_STATUS_L0_PCI_CMD_EN_INT) { >> + val = appl_readl(pcie, APPL_INTR_STATUS_L1_15); >> + appl_writel(pcie, val, APPL_INTR_STATUS_L1_15); >> + if (val & APPL_INTR_STATUS_L1_15_CFG_BME_CHGED) { >> + if (!kfifo_put(&pcie->event_fifo, EP_BME_CHANGE)) { >> + dev_err(pcie->dev, "EVENT FIFO is full\n"); >> + return IRQ_HANDLED; >> + } >> + wake_up(&pcie->wq); >> + } >> + spurious = 0; >> + } >> + >> + if (spurious) { >> + dev_warn(pcie->dev, "Random interrupt (STATUS = 0x%08X)\n", >> + val); >> + appl_writel(pcie, val, APPL_INTR_STATUS_L0); >> + } >> + >> + return IRQ_HANDLED; >> +} >> + >> static irqreturn_t tegra_pcie_irq_handler(int irq, void *arg) >> { >> struct tegra_pcie_dw *pcie = arg; >> >> - return tegra_pcie_rp_irq_handler(pcie); >> + if (pcie->mode == DW_PCIE_RC_TYPE) >> + return tegra_pcie_rp_irq_handler(pcie); >> + else if (pcie->mode == DW_PCIE_EP_TYPE) >> + return tegra_pcie_ep_irq_handler(pcie); >> + >> + return IRQ_NONE; >> } > > We already know at probe time whether the controller is in root complex > or endpoint mode, right? Couldn't we just install the correct handler > rather than multiplex here? It's not a very big deal, but given that > these are interrupts, avoiding the additional indirection might be a > good idea. Done. > > [...] >> @@ -986,6 +1115,42 @@ static int tegra_pcie_dw_parse_dt(struct tegra_pcie_dw *pcie) >> pcie->enable_cdm_check = >> of_property_read_bool(np, "snps,enable-cdm-check"); >> >> + if (pcie->mode == DW_PCIE_RC_TYPE) >> + return 0; >> + >> + /* Endpoint mode specific DT entries */ >> + name = devm_kasprintf(pcie->dev, GFP_KERNEL, >> + "tegra_pcie_%u_pex_rst_gpio", pcie->cid); >> + if (!name) { >> + dev_err(pcie->dev, "Failed to create PERST GPIO string\n"); >> + return -ENOMEM; >> + } >> + pcie->pex_rst_gpiod = devm_gpiod_get_from_of_node(pcie->dev, np, > > If np == pcie->dev.of_node, you can simply use devm_gpiod_get() here, > can't you? Done. > > [...] >> +static void pex_ep_event_hot_rst_done(struct tegra_pcie_dw *pcie) >> +{ >> + u32 val = 0; > > The initialization here seems unnecessary. Done. > > [...] >> +static int tegra_pcie_ep_raise_legacy_irq(struct tegra_pcie_dw *pcie, u16 irq) >> +{ >> + /* Tegra194 supports only INTA */ >> + if (irq > 1) >> + return -EINVAL; >> + >> + appl_writel(pcie, 1, APPL_LEGACY_INTX); >> + mdelay(1); > > Spinning for 1 ms these days is quite a lot. Does this have to be a busy > loop or could you use something like usleep_range(1000, 2000) to allow > the CPU to do something else in the meantime? > > Also, does the legacy INTX pulse have to be a whole millisecond wide? Or > could this be shorter? A one millisecond pulse implies a maximum of 1000 > interrupts per second, which seems a bit low. 1 ms is what all the other implementations also are using and they are using mdelay also :( . But, I think this doesn't have to be a busy loop and I'll change it to usleep_range(1000, 2000). > >> @@ -1440,6 +2178,12 @@ static int tegra_pcie_dw_probe(struct platform_device *pdev) >> int ret; >> u32 i; >> >> + match = of_match_device(of_match_ptr(tegra_pcie_dw_of_match), dev); >> + if (!match) >> + return -EINVAL; >> + >> + data = (struct tegra_pcie_dw_of_data *)match->data; > > of_device_get_match_data()? Done. > >> + >> pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL); >> if (!pcie) >> return -ENOMEM; >> @@ -1449,6 +2193,7 @@ static int tegra_pcie_dw_probe(struct platform_device *pdev) >> pci->ops = &tegra_dw_pcie_ops; >> pp = &pci->pp; >> pcie->dev = &pdev->dev; >> + pcie->mode = (enum dw_pcie_device_mode)data->mode; >> >> ret = tegra_pcie_dw_parse_dt(pcie); >> if (ret < 0) { >> @@ -1462,6 +2207,9 @@ static int tegra_pcie_dw_probe(struct platform_device *pdev) >> return ret; >> } >> >> + if (pcie->pex_refclk_sel_gpiod) >> + gpiod_set_value(pcie->pex_refclk_sel_gpiod, 1); >> + >> pcie->pex_ctl_supply = devm_regulator_get(dev, "vddio-pex-ctl"); >> if (IS_ERR(pcie->pex_ctl_supply)) { >> ret = PTR_ERR(pcie->pex_ctl_supply); >> @@ -1570,11 +2318,24 @@ static int tegra_pcie_dw_probe(struct platform_device *pdev) >> >> platform_set_drvdata(pdev, pcie); >> >> - ret = tegra_pcie_config_rp(pcie); >> - if (ret && ret != -ENOMEDIUM) >> - goto fail; >> - else >> - return 0; >> + switch (pcie->mode) { >> + case DW_PCIE_RC_TYPE: >> + ret = tegra_pcie_config_rp(pcie); >> + if (ret && ret != -ENOMEDIUM) >> + goto fail; >> + else >> + return 0; >> + break; >> + >> + case DW_PCIE_EP_TYPE: >> + ret = tegra_pcie_config_ep(pcie, pdev); >> + if (ret < 0) >> + goto fail; >> + break; >> + >> + default: >> + dev_err(dev, "Invalid PCIe device type %d\n", pcie->mode); >> + } >> >> fail: >> tegra_bpmp_put(pcie->bpmp); >> @@ -1593,6 +2354,8 @@ static int tegra_pcie_dw_remove(struct platform_device *pdev) >> pm_runtime_put_sync(pcie->dev); >> pm_runtime_disable(pcie->dev); >> tegra_bpmp_put(pcie->bpmp); >> + if (pcie->pex_refclk_sel_gpiod) >> + gpiod_set_value(pcie->pex_refclk_sel_gpiod, 0); >> >> return 0; >> } >> @@ -1697,13 +2460,6 @@ static void tegra_pcie_dw_shutdown(struct platform_device *pdev) >> __deinit_controller(pcie); >> } >> >> -static const struct of_device_id tegra_pcie_dw_of_match[] = { >> - { >> - .compatible = "nvidia,tegra194-pcie", >> - }, >> - {}, >> -}; >> - > > No need to move this around if you use of_device_get_match_data(). Yes. Thanks for the review. Vidya Sagar > > Thierry > >> static const struct dev_pm_ops tegra_pcie_dw_pm_ops = { >> .suspend_late = tegra_pcie_dw_suspend_late, >> .suspend_noirq = tegra_pcie_dw_suspend_noirq, >> -- >> 2.17.1 >>
On 1/6/20 6:44 PM, Thierry Reding wrote: > On Fri, Jan 03, 2020 at 06:14:02PM +0530, Vidya Sagar wrote: >> Add support for the endpoint mode of Synopsys DesignWare core based >> dual mode PCIe controllers present in Tegra194 SoC. >> >> Signed-off-by: Vidya Sagar <vidyas@nvidia.com> >> --- >> V2: >> * Addressed Bjorn's review comments >> * Made changes as part of addressing review comments for other patches >> >> drivers/pci/controller/dwc/Kconfig | 30 +- >> drivers/pci/controller/dwc/pcie-tegra194.c | 782 ++++++++++++++++++++- >> 2 files changed, 796 insertions(+), 16 deletions(-) >> > [...] >> diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c >> index cbe95f0ea0ca..6621ac79efee 100644 >> --- a/drivers/pci/controller/dwc/pcie-tegra194.c >> +++ b/drivers/pci/controller/dwc/pcie-tegra194.c > [...] >> @@ -1427,8 +1620,553 @@ static int tegra_pcie_config_rp(struct tegra_pcie_dw *pcie) > [...] >> +static int tegra_pcie_config_ep(struct tegra_pcie_dw *pcie, >> + struct platform_device *pdev) >> +{ > [...] >> + ret = devm_request_irq(dev, pcie->pex_rst_irq, >> + tegra_pcie_ep_pex_rst_irq, >> + IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING, >> + name, (void *)pcie); >> + if (ret < 0) { >> + dev_err(dev, "Failed to request IRQ for PERST: %d\n", ret); >> + return ret; >> + } >> + disable_irq(pcie->pex_rst_irq); > > I just came across this while reviewing another patch: it looks like a > better way to handle "disabled by default" interrupts is to do this: > > irq_set_status_flags(rtc->irq, IRQ_NOAUTOEN); > > before calling devm_request_irq(). See here for an example: > > http://patchwork.ozlabs.org/patch/1217944/ I'll take care of it. Thanks for the review. Vidya Sagar > > Thierry >