On 1/6/20 6:32 PM, Thierry Reding wrote:

On Fri, Jan 03, 2020 at 06:14:02PM +0530, Vidya Sagar wrote:

quoted

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

@@ -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.

quoted

+		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.

quoted

+				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.

[...]

quoted

@@ -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.

[...]

quoted

+static void pex_ep_event_hot_rst_done(struct tegra_pcie_dw *pcie)
+{
+	u32 val = 0;

The initialization here seems unnecessary.

Done.

[...]

quoted

+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).

quoted

@@ -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.

quoted

+
  	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

quoted

  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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel