mbox series

[V2,0/5] Add support for PCIe endpoint mode in Tegra194

Message ID 20200103124404.20662-1-vidyas@nvidia.com
Headers show
Series Add support for PCIe endpoint mode in Tegra194 | expand

Message

Vidya Sagar Jan. 3, 2020, 12:43 p.m. UTC
Tegra194 has three (C0, C4 & C5) dual mode PCIe controllers that can operate
either in root port mode or in end point mode but only in one mode at a time.
Platform P2972-0000 supports enabling endpoint mode for C5 controller. This
patch series adds support for PCIe endpoint mode in both the driver as well as
in DT.
This patch series depends on the changes made for Synopsys DesignWare endpoint
mode subsystem that are currently under review
@ https://patchwork.kernel.org/project/linux-pci/list/?series=202211
which in turn depends on the patch made by Kishon
@ https://patchwork.kernel.org/patch/10975123/
which is also under review.

V2:
* Addressed Thierry & Bjorn's review comments
* Added EP mode specific binding documentation to already existing binding documentation file
* Removed patch that enables GPIO controller nodes explicitly as they are enabled already

Vidya Sagar (5):
  soc/tegra: bpmp: Update ABI header
  dt-bindings: PCI: tegra: Add DT support for PCIe EP nodes in Tegra194
  PCI: tegra: Add support for PCIe endpoint mode in Tegra194
  arm64: tegra: Add PCIe endpoint controllers nodes for Tegra194
  arm64: tegra: Add support for PCIe endpoint mode in P2972-0000
    platform

 .../bindings/pci/nvidia,tegra194-pcie.txt     | 125 ++-
 .../boot/dts/nvidia/tegra194-p2972-0000.dts   |  18 +
 arch/arm64/boot/dts/nvidia/tegra194.dtsi      |  99 +++
 drivers/pci/controller/dwc/Kconfig            |  30 +-
 drivers/pci/controller/dwc/pcie-tegra194.c    | 782 +++++++++++++++++-
 include/soc/tegra/bpmp-abi.h                  |  10 +-
 6 files changed, 1021 insertions(+), 43 deletions(-)

Comments

Thierry Reding Jan. 6, 2020, 1:02 p.m. UTC | #1
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
>
Thierry Reding Jan. 6, 2020, 1:04 p.m. UTC | #2
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>
Thierry Reding Jan. 6, 2020, 1:14 p.m. UTC | #3
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
Vidya Sagar Jan. 13, 2020, 6:12 p.m. UTC | #4
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
>>
Vidya Sagar Jan. 13, 2020, 6:12 p.m. UTC | #5
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
>