Re: [PATCH V2 3/5] PCI: tegra: Add support for PCIe endpoint mode in Tegra194
From: Thierry Reding <hidden>
Date: 2020-01-06 13:03:07
Also in:
linux-devicetree, linux-pci, linux-tegra, lkml
On Fri, Jan 03, 2020 at 06:14:02PM +0530, Vidya Sagar wrote:
quoted hunk ↗ jump to hunk
Add support for the endpoint mode of Synopsys DesignWare core based dual mode PCIe controllers present in Tegra194 SoC. Signed-off-by: Vidya Sagar <redacted> --- 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
[...]
quoted hunk ↗ jump to hunk
@@ -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. [...]
quoted hunk ↗ jump to hunk
@@ -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.
quoted hunk ↗ jump to hunk
@@ -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()?
quoted hunk ↗ jump to hunk
+ 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