[PATCH v3 00/26] Add support for PCIe RC and EP mode in TI's AM654 SoC

[PATCH v3 00/26] Add support for PCIe RC and EP mode in TI's AM654 SoC

[Kishon Vijay Abraham I]

Add PCIe RC support for TI's AM654 SoC. The PCIe controller in AM654
uses Synopsys core revision 4.90a and uses the same TI wrapper as used
in keystone2 with certain modification. Hence AM654 will use the same
pci wrapper driver pci-keystone.c

This series was initially part of [1]. This series only includes patches
that has to be merged via Lorenzo's tree. The PHY patches and dt patches
will be sent separately.

This series is created over keystone MSI cleanup series [2].

This series:
*) Cleanup pci-keystone driver so that both RC mode and EP mode of
   AM654 can be supported
*) Modify epc-core to support allocation of aligned buffers required for
   AM654
*) Fix ATU unroll identification
*) Add support for both host mode and device mode in AM654

Changes from v2:
*) Missed updating "Reviewed-by: Rob Herring <robh@kernel.org>" tags
   in the version that was sent to list.
*) Add const qualifier to struct dw_pcie_ep_ops in pci-layerscape-ep.c

Changes from v1:
*) Support for legacy interrupt in AM654 is removed (see background here
   [3])
*) Allow of_pci_get_max_link_speed to be used by Endpoint controller
driver
*) Add support to set max-link-speed from DT in pci-keystone driver
*) Update "Reviewed-by: Rob Herring <robh@kernel.org>" tags.

[1] -> https://lore.kernel.org/patchwork/cover/989487/
[2] -> https://lkml.org/lkml/2019/3/21/193
[3] -> https://lkml.org/lkml/2019/3/19/235

Kishon Vijay Abraham I (26):
  PCI: keystone: Add start_link/stop_link dw_pcie_ops
  PCI: keystone: Cleanup error_irq configuration
  dt-bindings: PCI: keystone: Add "reg-names" binding information
  PCI: keystone: Perform host initialization in a single function
  PCI: keystone: Use platform_get_resource_byname to get memory
    resources
  PCI: keystone: Move initializations to appropriate places
  dt-bindings: PCI: Add dt-binding to configure PCIe mode
  PCI: keystone: Explicitly set the PCIe mode
  dt-bindings: PCI: Document "atu" reg-names
  PCI: dwc: Enable iATU unroll for endpoint too
  PCI: dwc: Fix ATU identification for designware version >= 4.80
  PCI: keystone: Prevent ARM32 specific code to be compiled for ARM64
  dt-bindings: PCI: Add PCI RC dt binding documentation for AM654
  PCI: keystone: Add support for PCIe RC in AM654x Platforms
  PCI: keystone: Invoke phy_reset API before enabling PHY
  PCI: OF: Allow of_pci_get_max_link_speed() to be used by PCI Endpoint
    drivers
  PCI: keystone: Add support to set the max link speed from DT
  PCI: endpoint: Add support to allocate aligned buffers to be mapped in
    BARs
  PCI: dwc: Add const qualifier to struct dw_pcie_ep_ops
  PCI: dwc: Fix dw_pcie_ep_find_capability to return correct capability
    offset
  PCI: dwc: Add callbacks for accessing dbi2 address space
  PCI: keystone: Add support for PCIe EP in AM654x Platforms
  PCI: designware-ep: Configure RESBAR to advertise the smallest size
  PCI: designware-ep: Use aligned ATU window for raising MSI interrupts
  misc: pci_endpoint_test: Add support to test PCI EP in AM654x
  misc: pci_endpoint_test: Fix test_reg_bar to be updated in
    pci_endpoint_test

 .../bindings/pci/designware-pcie.txt          |   7 +-
 .../devicetree/bindings/pci/pci-keystone.txt  |  14 +-
 drivers/misc/pci_endpoint_test.c              |  18 +
 drivers/pci/Makefile                          |   2 +-
 drivers/pci/controller/dwc/Kconfig            |  25 +-
 drivers/pci/controller/dwc/pci-dra7xx.c       |   2 +-
 drivers/pci/controller/dwc/pci-keystone.c     | 577 +++++++++++++++---
 .../pci/controller/dwc/pci-layerscape-ep.c    |   2 +-
 drivers/pci/controller/dwc/pcie-artpec6.c     |   2 +-
 .../pci/controller/dwc/pcie-designware-ep.c   |  55 +-
 .../pci/controller/dwc/pcie-designware-host.c |  19 -
 .../pci/controller/dwc/pcie-designware-plat.c |   2 +-
 drivers/pci/controller/dwc/pcie-designware.c  |  52 ++
 drivers/pci/controller/dwc/pcie-designware.h  |  15 +-
 drivers/pci/endpoint/functions/pci-epf-test.c |   5 +-
 drivers/pci/endpoint/pci-epf-core.c           |  10 +-
 drivers/pci/of.c                              |  44 +-
 include/linux/pci-epc.h                       |   2 +
 include/linux/pci-epf.h                       |   3 +-
 19 files changed, 683 insertions(+), 173 deletions(-)

-- 
2.17.1

[PATCH v3 01/26] PCI: keystone: Add start_link/stop_link dw_pcie_ops

[Kishon Vijay Abraham I]

Add start_link/stop_link dw_pcie_ops and invoke ks_pcie_start_link
directly from host_init. start_link/stop_link ops is required for
adding EP mode support.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/pci/controller/dwc/pci-keystone.c | 44 +++++++++++------------
 1 file changed, 22 insertions(+), 22 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
index b757692e2848..07f55b355d75 100644
--- a/drivers/pci/controller/dwc/pci-keystone.c
+++ b/drivers/pci/controller/dwc/pci-keystone.c
@@ -460,18 +460,33 @@ static int ks_pcie_link_up(struct dw_pcie *pci)
 	return (val == PORT_LOGIC_LTSSM_STATE_L0);
 }
 
-static void ks_pcie_initiate_link_train(struct keystone_pcie *ks_pcie)
+static void ks_pcie_stop_link(struct dw_pcie *pci)
 {
+	struct keystone_pcie *ks_pcie = to_keystone_pcie(pci);
 	u32 val;
 
 	/* Disable Link training */
 	val = ks_pcie_app_readl(ks_pcie, CMD_STATUS);
 	val &= ~LTSSM_EN_VAL;
 	ks_pcie_app_writel(ks_pcie, CMD_STATUS, LTSSM_EN_VAL | val);
+}
+
+static int ks_pcie_start_link(struct dw_pcie *pci)
+{
+	struct keystone_pcie *ks_pcie = to_keystone_pcie(pci);
+	struct device *dev = pci->dev;
+	u32 val;
+
+	if (dw_pcie_link_up(pci)) {
+		dev_dbg(dev, "link is already up\n");
+		return 0;
+	}
 
 	/* Initiate Link Training */
 	val = ks_pcie_app_readl(ks_pcie, CMD_STATUS);
 	ks_pcie_app_writel(ks_pcie, CMD_STATUS, LTSSM_EN_VAL | val);
+
+	return 0;
 }
 
 /**
@@ -556,26 +571,6 @@ static void ks_pcie_quirk(struct pci_dev *dev)
 }
 DECLARE_PCI_FIXUP_ENABLE(PCI_ANY_ID, PCI_ANY_ID, ks_pcie_quirk);
 
-static int ks_pcie_establish_link(struct keystone_pcie *ks_pcie)
-{
-	struct dw_pcie *pci = ks_pcie->pci;
-	struct device *dev = pci->dev;
-
-	if (dw_pcie_link_up(pci)) {
-		dev_info(dev, "Link already up\n");
-		return 0;
-	}
-
-	ks_pcie_initiate_link_train(ks_pcie);
-
-	/* check if the link is up or not */
-	if (!dw_pcie_wait_for_link(pci))
-		return 0;
-
-	dev_err(dev, "phy link never came up\n");
-	return -ETIMEDOUT;
-}
-
 static void ks_pcie_msi_irq_handler(struct irq_desc *desc)
 {
 	unsigned int irq = desc->irq_data.hwirq;
@@ -813,7 +808,7 @@ static int __init ks_pcie_host_init(struct pcie_port *pp)
 
 	dw_pcie_setup_rc(pp);
 
-	ks_pcie_establish_link(ks_pcie);
+	ks_pcie_stop_link(pci);
 	ks_pcie_setup_rc_app_regs(ks_pcie);
 	ks_pcie_setup_interrupts(ks_pcie);
 	writew(PCI_IO_RANGE_TYPE_32 | (PCI_IO_RANGE_TYPE_32 << 8),
@@ -830,6 +825,9 @@ static int __init ks_pcie_host_init(struct pcie_port *pp)
 	hook_fault_code(17, ks_pcie_fault, SIGBUS, 0,
 			"Asynchronous external abort");
 
+	ks_pcie_start_link(pci);
+	dw_pcie_wait_for_link(pci);
+
 	return 0;
 }
 
@@ -892,6 +890,8 @@ static const struct of_device_id ks_pcie_of_match[] = {
 };
 
 static const struct dw_pcie_ops ks_pcie_dw_pcie_ops = {
+	.start_link = ks_pcie_start_link,
+	.stop_link = ks_pcie_stop_link,
 	.link_up = ks_pcie_link_up,
 };
 
-- 
2.17.1

[PATCH v3 02/26] PCI: keystone: Cleanup error_irq configuration

[Kishon Vijay Abraham I]

pci-keystone driver uses irq_of_parse_and_map to get irq number of
error_irq. Use platform_get_irq instead and move platform_get_irq()
and request_irq() of error_irq from ks_pcie_add_pcie_port to ks_pcie_probe
since error_irq is common to both RC mode and EP mode.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/pci/controller/dwc/pci-keystone.c | 43 +++++++++--------------
 1 file changed, 17 insertions(+), 26 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
index 07f55b355d75..e50f8773e768 100644
--- a/drivers/pci/controller/dwc/pci-keystone.c
+++ b/drivers/pci/controller/dwc/pci-keystone.c
@@ -98,8 +98,6 @@ struct keystone_pcie {
 	struct irq_domain	*legacy_irq_domain;
 	struct device_node	*np;
 
-	int error_irq;
-
 	/* Application register space */
 	void __iomem		*va_app_base;	/* DT 1st resource */
 	struct resource		app;
@@ -743,12 +741,6 @@ static int ks_pcie_config_legacy_irq(struct keystone_pcie *ks_pcie)
 	return ret;
 }
 
-static void ks_pcie_setup_interrupts(struct keystone_pcie *ks_pcie)
-{
-	if (ks_pcie->error_irq > 0)
-		ks_pcie_enable_error_irq(ks_pcie);
-}
-
 /*
  * When a PCI device does not exist during config cycles, keystone host gets a
  * bus error instead of returning 0xffffffff. This handler always returns 0
@@ -810,7 +802,6 @@ static int __init ks_pcie_host_init(struct pcie_port *pp)
 
 	ks_pcie_stop_link(pci);
 	ks_pcie_setup_rc_app_regs(ks_pcie);
-	ks_pcie_setup_interrupts(ks_pcie);
 	writew(PCI_IO_RANGE_TYPE_32 | (PCI_IO_RANGE_TYPE_32 << 8),
 			pci->dbi_base + PCI_IO_BASE);
 
@@ -854,23 +845,6 @@ static int __init ks_pcie_add_pcie_port(struct keystone_pcie *ks_pcie,
 	struct device *dev = &pdev->dev;
 	int ret;
 
-	/*
-	 * Index 0 is the platform interrupt for error interrupt
-	 * from RC.  This is optional.
-	 */
-	ks_pcie->error_irq = irq_of_parse_and_map(ks_pcie->np, 0);
-	if (ks_pcie->error_irq <= 0)
-		dev_info(dev, "no error IRQ defined\n");
-	else {
-		ret = request_irq(ks_pcie->error_irq, ks_pcie_err_irq_handler,
-				  IRQF_SHARED, "pcie-error-irq", ks_pcie);
-		if (ret < 0) {
-			dev_err(dev, "failed to request error IRQ %d\n",
-				ks_pcie->error_irq);
-			return ret;
-		}
-	}
-
 	pp->ops = &ks_pcie_host_ops;
 	ret = ks_pcie_dw_host_init(ks_pcie);
 	if (ret) {
@@ -946,6 +920,7 @@ static int __init ks_pcie_probe(struct platform_device *pdev)
 	u32 num_lanes;
 	char name[10];
 	int ret;
+	int irq;
 	int i;
 
 	ks_pcie = devm_kzalloc(dev, sizeof(*ks_pcie), GFP_KERNEL);
@@ -965,6 +940,20 @@ static int __init ks_pcie_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		dev_err(dev, "missing IRQ resource: %d\n", irq);
+		return irq;
+	}
+
+	ret = request_irq(irq, ks_pcie_err_irq_handler, IRQF_SHARED,
+			  "ks-pcie-error-irq", ks_pcie);
+	if (ret < 0) {
+		dev_err(dev, "failed to request error IRQ %d\n",
+			irq);
+		return ret;
+	}
+
 	ret = of_property_read_u32(np, "num-lanes", &num_lanes);
 	if (ret)
 		num_lanes = 1;
@@ -1020,6 +1009,8 @@ static int __init ks_pcie_probe(struct platform_device *pdev)
 	if (ret < 0)
 		goto err_get_sync;
 
+	ks_pcie_enable_error_irq(ks_pcie);
+
 	return 0;
 
 err_get_sync:
-- 
2.17.1

Re: [PATCH v3 02/26] PCI: keystone: Cleanup error_irq configuration

[Bjorn Helgaas]

Hi Kishon,

On Mon, Mar 25, 2019 at 03:09:23PM +0530, Kishon Vijay Abraham I wrote:
> pci-keystone driver uses irq_of_parse_and_map to get irq number of
> error_irq. Use platform_get_irq instead and move platform_get_irq()
> and request_irq() of error_irq from ks_pcie_add_pcie_port to ks_pcie_probe
> since error_irq is common to both RC mode and EP mode.

Does this have any DT implications?

It's not obvious that platform_get_irq() and irq_of_parse_and_map() work
similarly or that they get the same result from DT.

I'm sure they *do*, but it would be nice to have some hints in the commit
log about why that's the case.

> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  drivers/pci/controller/dwc/pci-keystone.c | 43 +++++++++--------------
>  1 file changed, 17 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
> index 07f55b355d75..e50f8773e768 100644
> --- a/drivers/pci/controller/dwc/pci-keystone.c
> +++ b/drivers/pci/controller/dwc/pci-keystone.c
> @@ -98,8 +98,6 @@ struct keystone_pcie {
>  	struct irq_domain	*legacy_irq_domain;
>  	struct device_node	*np;
>  
> -	int error_irq;
> -
>  	/* Application register space */
>  	void __iomem		*va_app_base;	/* DT 1st resource */
>  	struct resource		app;
> @@ -743,12 +741,6 @@ static int ks_pcie_config_legacy_irq(struct keystone_pcie *ks_pcie)
>  	return ret;
>  }
>  
> -static void ks_pcie_setup_interrupts(struct keystone_pcie *ks_pcie)
> -{
> -	if (ks_pcie->error_irq > 0)
> -		ks_pcie_enable_error_irq(ks_pcie);
> -}
> -
>  /*
>   * When a PCI device does not exist during config cycles, keystone host gets a
>   * bus error instead of returning 0xffffffff. This handler always returns 0
> @@ -810,7 +802,6 @@ static int __init ks_pcie_host_init(struct pcie_port *pp)
>  
>  	ks_pcie_stop_link(pci);
>  	ks_pcie_setup_rc_app_regs(ks_pcie);
> -	ks_pcie_setup_interrupts(ks_pcie);
>  	writew(PCI_IO_RANGE_TYPE_32 | (PCI_IO_RANGE_TYPE_32 << 8),
>  			pci->dbi_base + PCI_IO_BASE);
>  
> @@ -854,23 +845,6 @@ static int __init ks_pcie_add_pcie_port(struct keystone_pcie *ks_pcie,
>  	struct device *dev = &pdev->dev;
>  	int ret;
>  
> -	/*
> -	 * Index 0 is the platform interrupt for error interrupt
> -	 * from RC.  This is optional.
> -	 */
> -	ks_pcie->error_irq = irq_of_parse_and_map(ks_pcie->np, 0);
> -	if (ks_pcie->error_irq <= 0)
> -		dev_info(dev, "no error IRQ defined\n");
> -	else {
> -		ret = request_irq(ks_pcie->error_irq, ks_pcie_err_irq_handler,
> -				  IRQF_SHARED, "pcie-error-irq", ks_pcie);
> -		if (ret < 0) {
> -			dev_err(dev, "failed to request error IRQ %d\n",
> -				ks_pcie->error_irq);
> -			return ret;
> -		}
> -	}
> -
>  	pp->ops = &ks_pcie_host_ops;
>  	ret = ks_pcie_dw_host_init(ks_pcie);
>  	if (ret) {
> @@ -946,6 +920,7 @@ static int __init ks_pcie_probe(struct platform_device *pdev)
>  	u32 num_lanes;
>  	char name[10];
>  	int ret;
> +	int irq;
>  	int i;
>  
>  	ks_pcie = devm_kzalloc(dev, sizeof(*ks_pcie), GFP_KERNEL);
> @@ -965,6 +940,20 @@ static int __init ks_pcie_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0) {
> +		dev_err(dev, "missing IRQ resource: %d\n", irq);
> +		return irq;
> +	}
> +
> +	ret = request_irq(irq, ks_pcie_err_irq_handler, IRQF_SHARED,
> +			  "ks-pcie-error-irq", ks_pcie);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to request error IRQ %d\n",
> +			irq);
> +		return ret;
> +	}
> +
>  	ret = of_property_read_u32(np, "num-lanes", &num_lanes);
>  	if (ret)
>  		num_lanes = 1;
> @@ -1020,6 +1009,8 @@ static int __init ks_pcie_probe(struct platform_device *pdev)
>  	if (ret < 0)
>  		goto err_get_sync;
>  
> +	ks_pcie_enable_error_irq(ks_pcie);
> +
>  	return 0;
>  
>  err_get_sync:
> -- 
> 2.17.1
> 

Re: [PATCH v3 02/26] PCI: keystone: Cleanup error_irq configuration

[Kishon Vijay Abraham I]

Hi Bjorn,

On 13/04/19 7:33 PM, Bjorn Helgaas wrote:
> Hi Kishon,
> 
> On Mon, Mar 25, 2019 at 03:09:23PM +0530, Kishon Vijay Abraham I wrote:
>> pci-keystone driver uses irq_of_parse_and_map to get irq number of
>> error_irq. Use platform_get_irq instead and move platform_get_irq()
>> and request_irq() of error_irq from ks_pcie_add_pcie_port to ks_pcie_probe
>> since error_irq is common to both RC mode and EP mode.
> 
> Does this have any DT implications?

No. platform_get_irq() uses of_irq_get(), which in-turn uses of_irq_parse_one()
and irq_create_of_mapping() while irq_of_parse_and_map() directly uses
of_irq_parse_one() and irq_create_of_mapping().

The only difference is platform_get_irq() falls back to using
platform_get_resource() if of_irq_get fails.

I thought it's better to use platform_get_irq() for platform devices.

Thanks
Kishon

> 
> It's not obvious that platform_get_irq() and irq_of_parse_and_map() work
> similarly or that they get the same result from DT.
> 
> I'm sure they *do*, but it would be nice to have some hints in the commit
> log about why that's the case.
> 
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> ---
>>  drivers/pci/controller/dwc/pci-keystone.c | 43 +++++++++--------------
>>  1 file changed, 17 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
>> index 07f55b355d75..e50f8773e768 100644
>> --- a/drivers/pci/controller/dwc/pci-keystone.c
>> +++ b/drivers/pci/controller/dwc/pci-keystone.c
>> @@ -98,8 +98,6 @@ struct keystone_pcie {
>>  	struct irq_domain	*legacy_irq_domain;
>>  	struct device_node	*np;
>>  
>> -	int error_irq;
>> -
>>  	/* Application register space */
>>  	void __iomem		*va_app_base;	/* DT 1st resource */
>>  	struct resource		app;
>> @@ -743,12 +741,6 @@ static int ks_pcie_config_legacy_irq(struct keystone_pcie *ks_pcie)
>>  	return ret;
>>  }
>>  
>> -static void ks_pcie_setup_interrupts(struct keystone_pcie *ks_pcie)
>> -{
>> -	if (ks_pcie->error_irq > 0)
>> -		ks_pcie_enable_error_irq(ks_pcie);
>> -}
>> -
>>  /*
>>   * When a PCI device does not exist during config cycles, keystone host gets a
>>   * bus error instead of returning 0xffffffff. This handler always returns 0
>> @@ -810,7 +802,6 @@ static int __init ks_pcie_host_init(struct pcie_port *pp)
>>  
>>  	ks_pcie_stop_link(pci);
>>  	ks_pcie_setup_rc_app_regs(ks_pcie);
>> -	ks_pcie_setup_interrupts(ks_pcie);
>>  	writew(PCI_IO_RANGE_TYPE_32 | (PCI_IO_RANGE_TYPE_32 << 8),
>>  			pci->dbi_base + PCI_IO_BASE);
>>  
>> @@ -854,23 +845,6 @@ static int __init ks_pcie_add_pcie_port(struct keystone_pcie *ks_pcie,
>>  	struct device *dev = &pdev->dev;
>>  	int ret;
>>  
>> -	/*
>> -	 * Index 0 is the platform interrupt for error interrupt
>> -	 * from RC.  This is optional.
>> -	 */
>> -	ks_pcie->error_irq = irq_of_parse_and_map(ks_pcie->np, 0);
>> -	if (ks_pcie->error_irq <= 0)
>> -		dev_info(dev, "no error IRQ defined\n");
>> -	else {
>> -		ret = request_irq(ks_pcie->error_irq, ks_pcie_err_irq_handler,
>> -				  IRQF_SHARED, "pcie-error-irq", ks_pcie);
>> -		if (ret < 0) {
>> -			dev_err(dev, "failed to request error IRQ %d\n",
>> -				ks_pcie->error_irq);
>> -			return ret;
>> -		}
>> -	}
>> -
>>  	pp->ops = &ks_pcie_host_ops;
>>  	ret = ks_pcie_dw_host_init(ks_pcie);
>>  	if (ret) {
>> @@ -946,6 +920,7 @@ static int __init ks_pcie_probe(struct platform_device *pdev)
>>  	u32 num_lanes;
>>  	char name[10];
>>  	int ret;
>> +	int irq;
>>  	int i;
>>  
>>  	ks_pcie = devm_kzalloc(dev, sizeof(*ks_pcie), GFP_KERNEL);
>> @@ -965,6 +940,20 @@ static int __init ks_pcie_probe(struct platform_device *pdev)
>>  		return ret;
>>  	}
>>  
>> +	irq = platform_get_irq(pdev, 0);
>> +	if (irq < 0) {
>> +		dev_err(dev, "missing IRQ resource: %d\n", irq);
>> +		return irq;
>> +	}
>> +
>> +	ret = request_irq(irq, ks_pcie_err_irq_handler, IRQF_SHARED,
>> +			  "ks-pcie-error-irq", ks_pcie);
>> +	if (ret < 0) {
>> +		dev_err(dev, "failed to request error IRQ %d\n",
>> +			irq);
>> +		return ret;
>> +	}
>> +
>>  	ret = of_property_read_u32(np, "num-lanes", &num_lanes);
>>  	if (ret)
>>  		num_lanes = 1;
>> @@ -1020,6 +1009,8 @@ static int __init ks_pcie_probe(struct platform_device *pdev)
>>  	if (ret < 0)
>>  		goto err_get_sync;
>>  
>> +	ks_pcie_enable_error_irq(ks_pcie);
>> +
>>  	return 0;
>>  
>>  err_get_sync:
>> -- 
>> 2.17.1
>>

Re: [PATCH v3 02/26] PCI: keystone: Cleanup error_irq configuration

[Bjorn Helgaas]

On Mon, Mar 25, 2019 at 03:09:23PM +0530, Kishon Vijay Abraham I wrote:
> pci-keystone driver uses irq_of_parse_and_map to get irq number of
> error_irq. Use platform_get_irq instead and move platform_get_irq()
> and request_irq() of error_irq from ks_pcie_add_pcie_port to ks_pcie_probe
> since error_irq is common to both RC mode and EP mode.

Part of my confusion here is that there's no hint about why
you need to use platform_get_irq() instead of irq_of_parse_and_map().

> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  drivers/pci/controller/dwc/pci-keystone.c | 43 +++++++++--------------
>  1 file changed, 17 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
> index 07f55b355d75..e50f8773e768 100644
> --- a/drivers/pci/controller/dwc/pci-keystone.c
> +++ b/drivers/pci/controller/dwc/pci-keystone.c
> @@ -98,8 +98,6 @@ struct keystone_pcie {
>  	struct irq_domain	*legacy_irq_domain;
>  	struct device_node	*np;
>  
> -	int error_irq;
> -
>  	/* Application register space */
>  	void __iomem		*va_app_base;	/* DT 1st resource */
>  	struct resource		app;
> @@ -743,12 +741,6 @@ static int ks_pcie_config_legacy_irq(struct keystone_pcie *ks_pcie)
>  	return ret;
>  }
>  
> -static void ks_pcie_setup_interrupts(struct keystone_pcie *ks_pcie)
> -{
> -	if (ks_pcie->error_irq > 0)
> -		ks_pcie_enable_error_irq(ks_pcie);
> -}
> -
>  /*
>   * When a PCI device does not exist during config cycles, keystone host gets a
>   * bus error instead of returning 0xffffffff. This handler always returns 0
> @@ -810,7 +802,6 @@ static int __init ks_pcie_host_init(struct pcie_port *pp)
>  
>  	ks_pcie_stop_link(pci);
>  	ks_pcie_setup_rc_app_regs(ks_pcie);
> -	ks_pcie_setup_interrupts(ks_pcie);
>  	writew(PCI_IO_RANGE_TYPE_32 | (PCI_IO_RANGE_TYPE_32 << 8),
>  			pci->dbi_base + PCI_IO_BASE);
>  
> @@ -854,23 +845,6 @@ static int __init ks_pcie_add_pcie_port(struct keystone_pcie *ks_pcie,
>  	struct device *dev = &pdev->dev;
>  	int ret;
>  
> -	/*
> -	 * Index 0 is the platform interrupt for error interrupt
> -	 * from RC.  This is optional.
> -	 */
> -	ks_pcie->error_irq = irq_of_parse_and_map(ks_pcie->np, 0);
> -	if (ks_pcie->error_irq <= 0)
> -		dev_info(dev, "no error IRQ defined\n");
> -	else {
> -		ret = request_irq(ks_pcie->error_irq, ks_pcie_err_irq_handler,
> -				  IRQF_SHARED, "pcie-error-irq", ks_pcie);
> -		if (ret < 0) {
> -			dev_err(dev, "failed to request error IRQ %d\n",
> -				ks_pcie->error_irq);
> -			return ret;
> -		}
> -	}
> -
>  	pp->ops = &ks_pcie_host_ops;
>  	ret = ks_pcie_dw_host_init(ks_pcie);
>  	if (ret) {
> @@ -946,6 +920,7 @@ static int __init ks_pcie_probe(struct platform_device *pdev)
>  	u32 num_lanes;
>  	char name[10];
>  	int ret;
> +	int irq;
>  	int i;
>  
>  	ks_pcie = devm_kzalloc(dev, sizeof(*ks_pcie), GFP_KERNEL);
> @@ -965,6 +940,20 @@ static int __init ks_pcie_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0) {
> +		dev_err(dev, "missing IRQ resource: %d\n", irq);
> +		return irq;
> +	}
> +
> +	ret = request_irq(irq, ks_pcie_err_irq_handler, IRQF_SHARED,
> +			  "ks-pcie-error-irq", ks_pcie);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to request error IRQ %d\n",
> +			irq);
> +		return ret;
> +	}
> +
>  	ret = of_property_read_u32(np, "num-lanes", &num_lanes);
>  	if (ret)
>  		num_lanes = 1;
> @@ -1020,6 +1009,8 @@ static int __init ks_pcie_probe(struct platform_device *pdev)
>  	if (ret < 0)
>  		goto err_get_sync;
>  
> +	ks_pcie_enable_error_irq(ks_pcie);
> +
>  	return 0;
>  
>  err_get_sync:
> -- 
> 2.17.1
> 

[PATCH v3 03/26] dt-bindings: PCI: keystone: Add "reg-names" binding information

[Kishon Vijay Abraham I]

Add "reg-names" binding information in order for device tree node
to be populated with the correct register strings. This will break
old dt compatibility. However Keystone PCI has never worked
in upstream kernel due to lack of SERDES support. Before SERDES
support is added, cleanup the Keystone PCI dt-bindngs. This new
binding will also be used by PCI in AM654 platform.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/pci/pci-keystone.txt | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/pci-keystone.txt b/Documentation/devicetree/bindings/pci/pci-keystone.txt
index 2030ee0dc4f9..3a551687cfa2 100644
--- a/Documentation/devicetree/bindings/pci/pci-keystone.txt
+++ b/Documentation/devicetree/bindings/pci/pci-keystone.txt
@@ -12,8 +12,10 @@ described here as well as properties that are not applicable.
 Required Properties:-
 
 compatibility: "ti,keystone-pcie"
-reg:	index 1 is the base address and length of DW application registers.
-	index 2 is the base address and length of PCI device ID register.
+reg: Three register ranges as listed in the reg-names property
+reg-names: "dbics" for the DesignWare PCIe registers, "app" for the
+	   TI specific application registers, "config" for the
+	   configuration space address
 
 pcie_msi_intc : Interrupt controller device node for MSI IRQ chip
 	interrupt-cells: should be set to 1
-- 
2.17.1

[PATCH v3 04/26] PCI: keystone: Perform host initialization in a single function

[Kishon Vijay Abraham I]

No functional change. Instead of having two functions
ks_pcie_add_pcie_port and ks_pcie_dw_host_init for initializing host,
have a single function to perform all the host initialization.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/pci/controller/dwc/pci-keystone.c | 63 +++++++++--------------
 1 file changed, 23 insertions(+), 40 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
index e50f8773e768..566718ea7ebf 100644
--- a/drivers/pci/controller/dwc/pci-keystone.c
+++ b/drivers/pci/controller/dwc/pci-keystone.c
@@ -487,45 +487,6 @@ static int ks_pcie_start_link(struct dw_pcie *pci)
 	return 0;
 }
 
-/**
- * ks_pcie_dw_host_init() - initialize host for v3_65 dw hardware
- *
- * Ioremap the register resources, initialize legacy irq domain
- * and call dw_pcie_v3_65_host_init() API to initialize the Keystone
- * PCI host controller.
- */
-static int __init ks_pcie_dw_host_init(struct keystone_pcie *ks_pcie)
-{
-	struct dw_pcie *pci = ks_pcie->pci;
-	struct pcie_port *pp = &pci->pp;
-	struct device *dev = pci->dev;
-	struct platform_device *pdev = to_platform_device(dev);
-	struct resource *res;
-
-	/* Index 0 is the config reg. space address */
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	pci->dbi_base = devm_pci_remap_cfg_resource(dev, res);
-	if (IS_ERR(pci->dbi_base))
-		return PTR_ERR(pci->dbi_base);
-
-	/*
-	 * We set these same and is used in pcie rd/wr_other_conf
-	 * functions
-	 */
-	pp->va_cfg0_base = pci->dbi_base + SPACE0_REMOTE_CFG_OFFSET;
-	pp->va_cfg1_base = pp->va_cfg0_base;
-
-	/* Index 1 is the application reg. space address */
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
-	ks_pcie->va_app_base = devm_ioremap_resource(dev, res);
-	if (IS_ERR(ks_pcie->va_app_base))
-		return PTR_ERR(ks_pcie->va_app_base);
-
-	ks_pcie->app = *res;
-
-	return dw_pcie_host_init(pp);
-}
-
 static void ks_pcie_quirk(struct pci_dev *dev)
 {
 	struct pci_bus *bus = dev->bus;
@@ -843,10 +804,32 @@ static int __init ks_pcie_add_pcie_port(struct keystone_pcie *ks_pcie,
 	struct dw_pcie *pci = ks_pcie->pci;
 	struct pcie_port *pp = &pci->pp;
 	struct device *dev = &pdev->dev;
+	struct resource *res;
 	int ret;
 
+	/* Index 0 is the config reg. space address */
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	pci->dbi_base = devm_pci_remap_cfg_resource(dev, res);
+	if (IS_ERR(pci->dbi_base))
+		return PTR_ERR(pci->dbi_base);
+
+	/*
+	 * We set these same and is used in pcie rd/wr_other_conf
+	 * functions
+	 */
+	pp->va_cfg0_base = pci->dbi_base + SPACE0_REMOTE_CFG_OFFSET;
+	pp->va_cfg1_base = pp->va_cfg0_base;
+
+	/* Index 1 is the application reg. space address */
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	ks_pcie->va_app_base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(ks_pcie->va_app_base))
+		return PTR_ERR(ks_pcie->va_app_base);
+
+	ks_pcie->app = *res;
+
 	pp->ops = &ks_pcie_host_ops;
-	ret = ks_pcie_dw_host_init(ks_pcie);
+	ret = dw_pcie_host_init(pp);
 	if (ret) {
 		dev_err(dev, "failed to initialize host\n");
 		return ret;
-- 
2.17.1

[PATCH v3 05/26] PCI: keystone: Use platform_get_resource_byname to get memory resources

[Kishon Vijay Abraham I]

Use platform_get_resource_byname() instead of platform_get_resource() which
uses index to get memory resources. While at that get the memory resource
defined specifically for configuration space instead of deriving the
configuration space address from dbics address space. Since pci-keystone
driver has never worked out of the box in mainline kernel, dt backward
compatibility is ignored.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/pci/controller/dwc/pci-keystone.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
index 566718ea7ebf..5eebef9b9ada 100644
--- a/drivers/pci/controller/dwc/pci-keystone.c
+++ b/drivers/pci/controller/dwc/pci-keystone.c
@@ -44,7 +44,6 @@
 #define CFG_TYPE1			BIT(24)
 
 #define OB_SIZE				0x030
-#define SPACE0_REMOTE_CFG_OFFSET	0x1000
 #define OB_OFFSET_INDEX(n)		(0x200 + (8 * (n)))
 #define OB_OFFSET_HI(n)			(0x204 + (8 * (n)))
 #define OB_ENABLEN			BIT(0)
@@ -807,21 +806,19 @@ static int __init ks_pcie_add_pcie_port(struct keystone_pcie *ks_pcie,
 	struct resource *res;
 	int ret;
 
-	/* Index 0 is the config reg. space address */
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbics");
 	pci->dbi_base = devm_pci_remap_cfg_resource(dev, res);
 	if (IS_ERR(pci->dbi_base))
 		return PTR_ERR(pci->dbi_base);
 
-	/*
-	 * We set these same and is used in pcie rd/wr_other_conf
-	 * functions
-	 */
-	pp->va_cfg0_base = pci->dbi_base + SPACE0_REMOTE_CFG_OFFSET;
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "config");
+	pp->va_cfg0_base = devm_pci_remap_cfg_resource(dev, res);
+	if (IS_ERR(pp->va_cfg0_base))
+		return PTR_ERR(pp->va_cfg0_base);
+
 	pp->va_cfg1_base = pp->va_cfg0_base;
 
-	/* Index 1 is the application reg. space address */
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "app");
 	ks_pcie->va_app_base = devm_ioremap_resource(dev, res);
 	if (IS_ERR(ks_pcie->va_app_base))
 		return PTR_ERR(ks_pcie->va_app_base);
-- 
2.17.1

[PATCH v3 06/26] PCI: keystone: Move initializations to appropriate places

[Kishon Vijay Abraham I]

No functional change. Move host specific platform_get_resource to
ks_add_pcie_port and the common platform_get_resource (applicable
to both host and endpoint) to probe. This is in preparation for
adding endpoint support to pci-keystone driver.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/pci/controller/dwc/pci-keystone.c | 27 +++++++++++++----------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
index 5eebef9b9ada..95997885a05c 100644
--- a/drivers/pci/controller/dwc/pci-keystone.c
+++ b/drivers/pci/controller/dwc/pci-keystone.c
@@ -806,11 +806,6 @@ static int __init ks_pcie_add_pcie_port(struct keystone_pcie *ks_pcie,
 	struct resource *res;
 	int ret;
 
-	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbics");
-	pci->dbi_base = devm_pci_remap_cfg_resource(dev, res);
-	if (IS_ERR(pci->dbi_base))
-		return PTR_ERR(pci->dbi_base);
-
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "config");
 	pp->va_cfg0_base = devm_pci_remap_cfg_resource(dev, res);
 	if (IS_ERR(pp->va_cfg0_base))
@@ -818,13 +813,6 @@ static int __init ks_pcie_add_pcie_port(struct keystone_pcie *ks_pcie,
 
 	pp->va_cfg1_base = pp->va_cfg0_base;
 
-	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "app");
-	ks_pcie->va_app_base = devm_ioremap_resource(dev, res);
-	if (IS_ERR(ks_pcie->va_app_base))
-		return PTR_ERR(ks_pcie->va_app_base);
-
-	ks_pcie->app = *res;
-
 	pp->ops = &ks_pcie_host_ops;
 	ret = dw_pcie_host_init(pp);
 	if (ret) {
@@ -895,6 +883,8 @@ static int __init ks_pcie_probe(struct platform_device *pdev)
 	struct dw_pcie *pci;
 	struct keystone_pcie *ks_pcie;
 	struct device_link **link;
+	struct resource *res;
+	void __iomem *base;
 	u32 num_viewport;
 	struct phy **phy;
 	u32 num_lanes;
@@ -911,6 +901,19 @@ static int __init ks_pcie_probe(struct platform_device *pdev)
 	if (!pci)
 		return -ENOMEM;
 
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "app");
+	ks_pcie->va_app_base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(ks_pcie->va_app_base))
+		return PTR_ERR(ks_pcie->va_app_base);
+
+	ks_pcie->app = *res;
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbics");
+	base = devm_pci_remap_cfg_resource(dev, res);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	pci->dbi_base = base;
 	pci->dev = dev;
 	pci->ops = &ks_pcie_dw_pcie_ops;
 
-- 
2.17.1

[PATCH v3 07/26] dt-bindings: PCI: Add dt-binding to configure PCIe mode

[Kishon Vijay Abraham I]

Add "ti,syscon-pcie-mode" dt-binding to hold phandle to the syscon
register that should be used to configure PCIe in RC mode or EP mode.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/pci/pci-keystone.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/pci-keystone.txt b/Documentation/devicetree/bindings/pci/pci-keystone.txt
index 3a551687cfa2..8ee07197a063 100644
--- a/Documentation/devicetree/bindings/pci/pci-keystone.txt
+++ b/Documentation/devicetree/bindings/pci/pci-keystone.txt
@@ -23,6 +23,8 @@ pcie_msi_intc : Interrupt controller device node for MSI IRQ chip
 
 ti,syscon-pcie-id : phandle to the device control module required to set device
 		    id and vendor id.
+ti,syscon-pcie-mode : phandle to the device control module required to configure
+		      PCI in either RC mode or EP mode.
 
  Example:
 	pcie_msi_intc: msi-interrupt-controller {
-- 
2.17.1

[PATCH v3 08/26] PCI: keystone: Explicitly set the PCIe mode

[Kishon Vijay Abraham I]

Explicitly set the PCIe mode to BOOTCFG_DEVCFG instead of always
relying on the default values. This is required when EP mode has to
be explicitly written to BOOTCFG_DEVCFG register.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/pci/controller/dwc/pci-keystone.c | 37 +++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
index 95997885a05c..dfe54553d832 100644
--- a/drivers/pci/controller/dwc/pci-keystone.c
+++ b/drivers/pci/controller/dwc/pci-keystone.c
@@ -79,6 +79,15 @@
 #define PCIE_RC_K2L			0xb00a
 #define PCIE_RC_K2G			0xb00b
 
+#define KS_PCIE_DEV_TYPE_MASK		(0x3 << 1)
+#define KS_PCIE_DEV_TYPE(mode)		((mode) << 1)
+
+#define EP				0x0
+#define LEG_EP				0x1
+#define RC				0x2
+
+#define KS_PCIE_SYSCLOCKOUTEN		BIT(0)
+
 #define to_keystone_pcie(x)		dev_get_drvdata((x)->dev)
 
 struct keystone_pcie {
@@ -876,6 +885,30 @@ static int ks_pcie_enable_phy(struct keystone_pcie *ks_pcie)
 	return ret;
 }
 
+static int ks_pcie_set_mode(struct device *dev)
+{
+	struct device_node *np = dev->of_node;
+	struct regmap *syscon;
+	u32 val;
+	u32 mask;
+	int ret = 0;
+
+	syscon = syscon_regmap_lookup_by_phandle(np, "ti,syscon-pcie-mode");
+	if (IS_ERR(syscon))
+		return 0;
+
+	mask = KS_PCIE_DEV_TYPE_MASK | KS_PCIE_SYSCLOCKOUTEN;
+	val = KS_PCIE_DEV_TYPE(RC) | KS_PCIE_SYSCLOCKOUTEN;
+
+	ret = regmap_update_bits(syscon, 0, mask, val);
+	if (ret) {
+		dev_err(dev, "failed to set pcie mode\n");
+		return ret;
+	}
+
+	return 0;
+}
+
 static int __init ks_pcie_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -988,6 +1021,10 @@ static int __init ks_pcie_probe(struct platform_device *pdev)
 		goto err_get_sync;
 	}
 
+	ret = ks_pcie_set_mode(dev);
+	if (ret < 0)
+		goto err_get_sync;
+
 	ret = ks_pcie_add_pcie_port(ks_pcie, pdev);
 	if (ret < 0)
 		goto err_get_sync;
-- 
2.17.1

[PATCH v3 09/26] dt-bindings: PCI: Document "atu" reg-names

[Kishon Vijay Abraham I]

Document "atu" reg-names required to get the register space for ATU in
Synopsys designware core version >= 4.80.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 Documentation/devicetree/bindings/pci/designware-pcie.txt | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt b/Documentation/devicetree/bindings/pci/designware-pcie.txt
index c124f9bc11f3..5561a1c060d0 100644
--- a/Documentation/devicetree/bindings/pci/designware-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt
@@ -4,8 +4,11 @@ Required properties:
 - compatible:
 	"snps,dw-pcie" for RC mode;
 	"snps,dw-pcie-ep" for EP mode;
-- reg: Should contain the configuration address space.
-- reg-names: Must be "config" for the PCIe configuration space.
+- reg: For designware cores version < 4.80 contains the configuration
+       address space. For designware core version >= 4.80, contains
+       the configuration and ATU address space
+- reg-names: Must be "config" for the PCIe configuration space and "atu" for
+	     the ATU address space.
     (The old way of getting the configuration address space from "ranges"
     is deprecated and should be avoided.)
 - num-lanes: number of lanes to use
-- 
2.17.1

[PATCH v3 10/26] PCI: dwc: Enable iATU unroll for endpoint too

[Kishon Vijay Abraham I]

iatu_unroll_enabled flag is set only for Designware in host mode.
However iATU unroll can be applicable for endpoint mode too. Set
iatu_unroll_enabled flag in dw_pcie_setup which is common for
both host mode and endpoint mode.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 .../pci/controller/dwc/pcie-designware-ep.c   |  4 ----
 .../pci/controller/dwc/pcie-designware-host.c | 19 -------------------
 drivers/pci/controller/dwc/pcie-designware.c  | 19 +++++++++++++++++++
 3 files changed, 19 insertions(+), 23 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
index 24f5a775ad34..dc6a4bbd3ace 100644
--- a/drivers/pci/controller/dwc/pcie-designware-ep.c
+++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
@@ -517,10 +517,6 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
 		dev_err(dev, "dbi_base/dbi_base2 is not populated\n");
 		return -EINVAL;
 	}
-	if (pci->iatu_unroll_enabled && !pci->atu_base) {
-		dev_err(dev, "atu_base is not populated\n");
-		return -EINVAL;
-	}
 
 	ret = of_property_read_u32(np, "num-ib-windows", &ep->num_ib_windows);
 	if (ret < 0) {
diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index 7e0ff7d428a9..7bf6558341b6 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -608,17 +608,6 @@ static struct pci_ops dw_pcie_ops = {
 	.write = dw_pcie_wr_conf,
 };
 
-static u8 dw_pcie_iatu_unroll_enabled(struct dw_pcie *pci)
-{
-	u32 val;
-
-	val = dw_pcie_readl_dbi(pci, PCIE_ATU_VIEWPORT);
-	if (val == 0xffffffff)
-		return 1;
-
-	return 0;
-}
-
 void dw_pcie_setup_rc(struct pcie_port *pp)
 {
 	u32 val, ctrl, num_ctrls;
@@ -672,14 +661,6 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
 	 * we should not program the ATU here.
 	 */
 	if (!pp->ops->rd_other_conf) {
-		/* Get iATU unroll support */
-		pci->iatu_unroll_enabled = dw_pcie_iatu_unroll_enabled(pci);
-		dev_dbg(pci->dev, "iATU unroll: %s\n",
-			pci->iatu_unroll_enabled ? "enabled" : "disabled");
-
-		if (pci->iatu_unroll_enabled && !pci->atu_base)
-			pci->atu_base = pci->dbi_base + DEFAULT_DBI_ATU_OFFSET;
-
 		dw_pcie_prog_outbound_atu(pci, PCIE_ATU_REGION_INDEX0,
 					  PCIE_ATU_TYPE_MEM, pp->mem_base,
 					  pp->mem_bus_addr, pp->mem_size);
diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index 31f6331ca46f..a14ca00f72aa 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -339,6 +339,17 @@ int dw_pcie_link_up(struct dw_pcie *pci)
 		(!(val & PCIE_PHY_DEBUG_R1_LINK_IN_TRAINING)));
 }
 
+static u8 dw_pcie_iatu_unroll_enabled(struct dw_pcie *pci)
+{
+	u32 val;
+
+	val = dw_pcie_readl_dbi(pci, PCIE_ATU_VIEWPORT);
+	if (val == 0xffffffff)
+		return 1;
+
+	return 0;
+}
+
 void dw_pcie_setup(struct dw_pcie *pci)
 {
 	int ret;
@@ -347,6 +358,14 @@ void dw_pcie_setup(struct dw_pcie *pci)
 	struct device *dev = pci->dev;
 	struct device_node *np = dev->of_node;
 
+	/* Get iATU unroll support */
+	pci->iatu_unroll_enabled = dw_pcie_iatu_unroll_enabled(pci);
+	dev_dbg(pci->dev, "iATU unroll: %s\n",
+		pci->iatu_unroll_enabled ? "enabled" : "disabled");
+
+	if (pci->iatu_unroll_enabled && !pci->atu_base)
+		pci->atu_base = pci->dbi_base + DEFAULT_DBI_ATU_OFFSET;
+
 	ret = of_property_read_u32(np, "num-lanes", &lanes);
 	if (ret)
 		lanes = 0;
-- 
2.17.1

[PATCH v3 11/26] PCI: dwc: Fix ATU identification for designware version >= 4.80

[Kishon Vijay Abraham I]

Synopsys designware version >= 4.80 uses a separate register space
for programming ATU. The current code identifies if there exists a
separate register space by accessing the register address of ATUs
in designware version < 4.80. Accessing this address results in
abort in the case of K2G.

Fix it here by adding "version" member to struct dw_pcie. This should
be set by platform specific drivers and designware core will use it
to identify if the platform has a separate ATU space. For platforms
which hasn't populated the version member, the old method of
identification will still be used.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/pci/controller/dwc/pcie-designware.c | 14 ++++++++------
 drivers/pci/controller/dwc/pcie-designware.h |  1 +
 2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index a14ca00f72aa..4e2f7946da89 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -358,13 +358,15 @@ void dw_pcie_setup(struct dw_pcie *pci)
 	struct device *dev = pci->dev;
 	struct device_node *np = dev->of_node;
 
-	/* Get iATU unroll support */
-	pci->iatu_unroll_enabled = dw_pcie_iatu_unroll_enabled(pci);
-	dev_dbg(pci->dev, "iATU unroll: %s\n",
-		pci->iatu_unroll_enabled ? "enabled" : "disabled");
+	if (pci->version >= 0x480A || (!pci->version &&
+				       dw_pcie_iatu_unroll_enabled(pci))) {
+		pci->iatu_unroll_enabled = true;
+		if (!pci->atu_base)
+			pci->atu_base = pci->dbi_base + DEFAULT_DBI_ATU_OFFSET;
+	}
+	dev_dbg(pci->dev, "iATU unroll: %s\n", pci->iatu_unroll_enabled ?
+		"enabled" : "disabled");
 
-	if (pci->iatu_unroll_enabled && !pci->atu_base)
-		pci->atu_base = pci->dbi_base + DEFAULT_DBI_ATU_OFFSET;
 
 	ret = of_property_read_u32(np, "num-lanes", &lanes);
 	if (ret)
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index ca3a3190a6f5..90a5b1215344 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -234,6 +234,7 @@ struct dw_pcie {
 	struct pcie_port	pp;
 	struct dw_pcie_ep	ep;
 	const struct dw_pcie_ops *ops;
+	unsigned int		version;
 };
 
 #define to_dw_pcie_from_pp(port) container_of((port), struct dw_pcie, pp)
-- 
2.17.1

[PATCH v3 12/26] PCI: keystone: Prevent ARM32 specific code to be compiled for ARM64

[Kishon Vijay Abraham I]

hook_fault_code is an ARM32 specific API for hooking into data abort.
Since pci-keystone.c will be used for AM65X platforms which is an
ARM64 platform, allow hook_fault_code to be compiled only for ARM32.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/pci/controller/dwc/pci-keystone.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
index dfe54553d832..93296d434f40 100644
--- a/drivers/pci/controller/dwc/pci-keystone.c
+++ b/drivers/pci/controller/dwc/pci-keystone.c
@@ -710,6 +710,7 @@ static int ks_pcie_config_legacy_irq(struct keystone_pcie *ks_pcie)
 	return ret;
 }
 
+#ifdef CONFIG_ARM
 /*
  * When a PCI device does not exist during config cycles, keystone host gets a
  * bus error instead of returning 0xffffffff. This handler always returns 0
@@ -729,6 +730,7 @@ static int ks_pcie_fault(unsigned long addr, unsigned int fsr,
 
 	return 0;
 }
+#endif
 
 static int __init ks_pcie_init_id(struct keystone_pcie *ks_pcie)
 {
@@ -778,12 +780,14 @@ static int __init ks_pcie_host_init(struct pcie_port *pp)
 	if (ret < 0)
 		return ret;
 
+#ifdef CONFIG_ARM
 	/*
 	 * PCIe access errors that result into OCP errors are caught by ARM as
 	 * "External aborts"
 	 */
 	hook_fault_code(17, ks_pcie_fault, SIGBUS, 0,
 			"Asynchronous external abort");
+#endif
 
 	ks_pcie_start_link(pci);
 	dw_pcie_wait_for_link(pci);
-- 
2.17.1

Re: [PATCH v3 12/26] PCI: keystone: Prevent ARM32 specific code to be compiled for ARM64

[Lorenzo Pieralisi]

On Mon, Mar 25, 2019 at 03:09:33PM +0530, Kishon Vijay Abraham I wrote:
> hook_fault_code is an ARM32 specific API for hooking into data abort.
> Since pci-keystone.c will be used for AM65X platforms which is an
> ARM64 platform,

Hi Kishon,

How is the problem plugged by the fault hook fixed on ARM64 ?

Thanks,
Lorenzo

> allow hook_fault_code to be compiled only for ARM32.
> 
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  drivers/pci/controller/dwc/pci-keystone.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
> index dfe54553d832..93296d434f40 100644
> --- a/drivers/pci/controller/dwc/pci-keystone.c
> +++ b/drivers/pci/controller/dwc/pci-keystone.c
> @@ -710,6 +710,7 @@ static int ks_pcie_config_legacy_irq(struct keystone_pcie *ks_pcie)
>  	return ret;
>  }
>  
> +#ifdef CONFIG_ARM
>  /*
>   * When a PCI device does not exist during config cycles, keystone host gets a
>   * bus error instead of returning 0xffffffff. This handler always returns 0
> @@ -729,6 +730,7 @@ static int ks_pcie_fault(unsigned long addr, unsigned int fsr,
>  
>  	return 0;
>  }
> +#endif
>  
>  static int __init ks_pcie_init_id(struct keystone_pcie *ks_pcie)
>  {
> @@ -778,12 +780,14 @@ static int __init ks_pcie_host_init(struct pcie_port *pp)
>  	if (ret < 0)
>  		return ret;
>  
> +#ifdef CONFIG_ARM
>  	/*
>  	 * PCIe access errors that result into OCP errors are caught by ARM as
>  	 * "External aborts"
>  	 */
>  	hook_fault_code(17, ks_pcie_fault, SIGBUS, 0,
>  			"Asynchronous external abort");
> +#endif
>  
>  	ks_pcie_start_link(pci);
>  	dw_pcie_wait_for_link(pci);
> -- 
> 2.17.1
> 

Re: [PATCH v3 12/26] PCI: keystone: Prevent ARM32 specific code to be compiled for ARM64

[Kishon Vijay Abraham I]

Hi Lorenzo,

On 11/04/19 8:33 PM, Lorenzo Pieralisi wrote:
> On Mon, Mar 25, 2019 at 03:09:33PM +0530, Kishon Vijay Abraham I wrote:
>> hook_fault_code is an ARM32 specific API for hooking into data abort.
>> Since pci-keystone.c will be used for AM65X platforms which is an
>> ARM64 platform,
> 
> Hi Kishon,
> 
> How is the problem plugged by the fault hook fixed on ARM64 ?

At least in AM654 platform, I don't see a bus error when PCIe device is not
connected but returns 0xffffffff. So there is no necessary for hook_fault_code
in AM654 platform.

Thanks
Kishon

> 
> Thanks,
> Lorenzo
> 
>> allow hook_fault_code to be compiled only for ARM32.
>>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> ---
>>  drivers/pci/controller/dwc/pci-keystone.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
>> index dfe54553d832..93296d434f40 100644
>> --- a/drivers/pci/controller/dwc/pci-keystone.c
>> +++ b/drivers/pci/controller/dwc/pci-keystone.c
>> @@ -710,6 +710,7 @@ static int ks_pcie_config_legacy_irq(struct keystone_pcie *ks_pcie)
>>  	return ret;
>>  }
>>  
>> +#ifdef CONFIG_ARM
>>  /*
>>   * When a PCI device does not exist during config cycles, keystone host gets a
>>   * bus error instead of returning 0xffffffff. This handler always returns 0
>> @@ -729,6 +730,7 @@ static int ks_pcie_fault(unsigned long addr, unsigned int fsr,
>>  
>>  	return 0;
>>  }
>> +#endif
>>  
>>  static int __init ks_pcie_init_id(struct keystone_pcie *ks_pcie)
>>  {
>> @@ -778,12 +780,14 @@ static int __init ks_pcie_host_init(struct pcie_port *pp)
>>  	if (ret < 0)
>>  		return ret;
>>  
>> +#ifdef CONFIG_ARM
>>  	/*
>>  	 * PCIe access errors that result into OCP errors are caught by ARM as
>>  	 * "External aborts"
>>  	 */
>>  	hook_fault_code(17, ks_pcie_fault, SIGBUS, 0,
>>  			"Asynchronous external abort");
>> +#endif
>>  
>>  	ks_pcie_start_link(pci);
>>  	dw_pcie_wait_for_link(pci);
>> -- 
>> 2.17.1
>>

Re: [PATCH v3 12/26] PCI: keystone: Prevent ARM32 specific code to be compiled for ARM64

[Russell King - ARM Linux admin]

On Fri, Apr 12, 2019 at 02:20:06PM +0530, Kishon Vijay Abraham I wrote:
> Hi Lorenzo,
> 
> On 11/04/19 8:33 PM, Lorenzo Pieralisi wrote:
> > On Mon, Mar 25, 2019 at 03:09:33PM +0530, Kishon Vijay Abraham I wrote:
> >> hook_fault_code is an ARM32 specific API for hooking into data abort.
> >> Since pci-keystone.c will be used for AM65X platforms which is an
> >> ARM64 platform,
> > 
> > Hi Kishon,
> > 
> > How is the problem plugged by the fault hook fixed on ARM64 ?
> 
> At least in AM654 platform, I don't see a bus error when PCIe device is not
> connected but returns 0xffffffff. So there is no necessary for hook_fault_code
> in AM654 platform.

... which needs to be explained somewhere, either in the commit
message or comments in the file, so that years later when someone
has to look at this code, they know why it is the way that it is.

When writing commits, please think about whether there is enough
information to understand the change in ten years time.

> 
> Thanks
> Kishon
> 
> > 
> > Thanks,
> > Lorenzo
> > 
> >> allow hook_fault_code to be compiled only for ARM32.
> >>
> >> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> >> ---
> >>  drivers/pci/controller/dwc/pci-keystone.c | 4 ++++
> >>  1 file changed, 4 insertions(+)
> >>
> >> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
> >> index dfe54553d832..93296d434f40 100644
> >> --- a/drivers/pci/controller/dwc/pci-keystone.c
> >> +++ b/drivers/pci/controller/dwc/pci-keystone.c
> >> @@ -710,6 +710,7 @@ static int ks_pcie_config_legacy_irq(struct keystone_pcie *ks_pcie)
> >>  	return ret;
> >>  }
> >>  
> >> +#ifdef CONFIG_ARM
> >>  /*
> >>   * When a PCI device does not exist during config cycles, keystone host gets a
> >>   * bus error instead of returning 0xffffffff. This handler always returns 0
> >> @@ -729,6 +730,7 @@ static int ks_pcie_fault(unsigned long addr, unsigned int fsr,
> >>  
> >>  	return 0;
> >>  }
> >> +#endif
> >>  
> >>  static int __init ks_pcie_init_id(struct keystone_pcie *ks_pcie)
> >>  {
> >> @@ -778,12 +780,14 @@ static int __init ks_pcie_host_init(struct pcie_port *pp)
> >>  	if (ret < 0)
> >>  		return ret;
> >>  
> >> +#ifdef CONFIG_ARM
> >>  	/*
> >>  	 * PCIe access errors that result into OCP errors are caught by ARM as
> >>  	 * "External aborts"
> >>  	 */
> >>  	hook_fault_code(17, ks_pcie_fault, SIGBUS, 0,
> >>  			"Asynchronous external abort");
> >> +#endif
> >>  
> >>  	ks_pcie_start_link(pci);
> >>  	dw_pcie_wait_for_link(pci);
> >> -- 
> >> 2.17.1
> >>
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH v3 12/26] PCI: keystone: Prevent ARM32 specific code to be compiled for ARM64

[Kishon Vijay Abraham I]

Hi Russell,

On 12/04/19 3:01 PM, Russell King - ARM Linux admin wrote:
> On Fri, Apr 12, 2019 at 02:20:06PM +0530, Kishon Vijay Abraham I wrote:
>> Hi Lorenzo,
>>
>> On 11/04/19 8:33 PM, Lorenzo Pieralisi wrote:
>>> On Mon, Mar 25, 2019 at 03:09:33PM +0530, Kishon Vijay Abraham I wrote:
>>>> hook_fault_code is an ARM32 specific API for hooking into data abort.
>>>> Since pci-keystone.c will be used for AM65X platforms which is an
>>>> ARM64 platform,
>>>
>>> Hi Kishon,
>>>
>>> How is the problem plugged by the fault hook fixed on ARM64 ?
>>
>> At least in AM654 platform, I don't see a bus error when PCIe device is not
>> connected but returns 0xffffffff. So there is no necessary for hook_fault_code
>> in AM654 platform.
> 
> ... which needs to be explained somewhere, either in the commit
> message or comments in the file, so that years later when someone
> has to look at this code, they know why it is the way that it is.
> 
> When writing commits, please think about whether there is enough
> information to understand the change in ten years time.

sure, I'll fix this one.

Lorenzo, let me know if you want me to resend with updated commit log.

Thanks
Kishon

> 
>>
>> Thanks
>> Kishon
>>
>>>
>>> Thanks,
>>> Lorenzo
>>>
>>>> allow hook_fault_code to be compiled only for ARM32.
>>>>
>>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>>>> ---
>>>>  drivers/pci/controller/dwc/pci-keystone.c | 4 ++++
>>>>  1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
>>>> index dfe54553d832..93296d434f40 100644
>>>> --- a/drivers/pci/controller/dwc/pci-keystone.c
>>>> +++ b/drivers/pci/controller/dwc/pci-keystone.c
>>>> @@ -710,6 +710,7 @@ static int ks_pcie_config_legacy_irq(struct keystone_pcie *ks_pcie)
>>>>  	return ret;
>>>>  }
>>>>  
>>>> +#ifdef CONFIG_ARM
>>>>  /*
>>>>   * When a PCI device does not exist during config cycles, keystone host gets a
>>>>   * bus error instead of returning 0xffffffff. This handler always returns 0
>>>> @@ -729,6 +730,7 @@ static int ks_pcie_fault(unsigned long addr, unsigned int fsr,
>>>>  
>>>>  	return 0;
>>>>  }
>>>> +#endif
>>>>  
>>>>  static int __init ks_pcie_init_id(struct keystone_pcie *ks_pcie)
>>>>  {
>>>> @@ -778,12 +780,14 @@ static int __init ks_pcie_host_init(struct pcie_port *pp)
>>>>  	if (ret < 0)
>>>>  		return ret;
>>>>  
>>>> +#ifdef CONFIG_ARM
>>>>  	/*
>>>>  	 * PCIe access errors that result into OCP errors are caught by ARM as
>>>>  	 * "External aborts"
>>>>  	 */
>>>>  	hook_fault_code(17, ks_pcie_fault, SIGBUS, 0,
>>>>  			"Asynchronous external abort");
>>>> +#endif
>>>>  
>>>>  	ks_pcie_start_link(pci);
>>>>  	dw_pcie_wait_for_link(pci);
>>>> -- 
>>>> 2.17.1
>>>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>
> 

Re: [PATCH v3 12/26] PCI: keystone: Prevent ARM32 specific code to be compiled for ARM64

[Lorenzo Pieralisi]

On Fri, Apr 12, 2019 at 02:20:06PM +0530, Kishon Vijay Abraham I wrote:
> Hi Lorenzo,
> 
> On 11/04/19 8:33 PM, Lorenzo Pieralisi wrote:
> > On Mon, Mar 25, 2019 at 03:09:33PM +0530, Kishon Vijay Abraham I wrote:
> >> hook_fault_code is an ARM32 specific API for hooking into data abort.
> >> Since pci-keystone.c will be used for AM65X platforms which is an
> >> ARM64 platform,
> > 
> > Hi Kishon,
> > 
> > How is the problem plugged by the fault hook fixed on ARM64 ?
> 
> At least in AM654 platform, I don't see a bus error when PCIe device
> is not connected but returns 0xffffffff. So there is no necessary for
> hook_fault_code in AM654 platform.

That can't have much to do with ARM32<->ARM64, it is rather a platform
integration issue AFAICS. Russell has a point, this has to be documented
I can do it for you but I need additional information.

Thanks,
Lorenzo

> Thanks
> Kishon
> 
> > 
> > Thanks,
> > Lorenzo
> > 
> >> allow hook_fault_code to be compiled only for ARM32.
> >>
> >> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> >> ---
> >>  drivers/pci/controller/dwc/pci-keystone.c | 4 ++++
> >>  1 file changed, 4 insertions(+)
> >>
> >> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
> >> index dfe54553d832..93296d434f40 100644
> >> --- a/drivers/pci/controller/dwc/pci-keystone.c
> >> +++ b/drivers/pci/controller/dwc/pci-keystone.c
> >> @@ -710,6 +710,7 @@ static int ks_pcie_config_legacy_irq(struct keystone_pcie *ks_pcie)
> >>  	return ret;
> >>  }
> >>  
> >> +#ifdef CONFIG_ARM
> >>  /*
> >>   * When a PCI device does not exist during config cycles, keystone host gets a
> >>   * bus error instead of returning 0xffffffff. This handler always returns 0
> >> @@ -729,6 +730,7 @@ static int ks_pcie_fault(unsigned long addr, unsigned int fsr,
> >>  
> >>  	return 0;
> >>  }
> >> +#endif
> >>  
> >>  static int __init ks_pcie_init_id(struct keystone_pcie *ks_pcie)
> >>  {
> >> @@ -778,12 +780,14 @@ static int __init ks_pcie_host_init(struct pcie_port *pp)
> >>  	if (ret < 0)
> >>  		return ret;
> >>  
> >> +#ifdef CONFIG_ARM
> >>  	/*
> >>  	 * PCIe access errors that result into OCP errors are caught by ARM as
> >>  	 * "External aborts"
> >>  	 */
> >>  	hook_fault_code(17, ks_pcie_fault, SIGBUS, 0,
> >>  			"Asynchronous external abort");
> >> +#endif
> >>  
> >>  	ks_pcie_start_link(pci);
> >>  	dw_pcie_wait_for_link(pci);
> >> -- 
> >> 2.17.1
> >>

Re: [PATCH v3 12/26] PCI: keystone: Prevent ARM32 specific code to be compiled for ARM64

[Kishon Vijay Abraham I]

Hi Lorenzo,

On 12/04/19 4:41 PM, Lorenzo Pieralisi wrote:
> On Fri, Apr 12, 2019 at 02:20:06PM +0530, Kishon Vijay Abraham I wrote:
>> Hi Lorenzo,
>>
>> On 11/04/19 8:33 PM, Lorenzo Pieralisi wrote:
>>> On Mon, Mar 25, 2019 at 03:09:33PM +0530, Kishon Vijay Abraham I wrote:
>>>> hook_fault_code is an ARM32 specific API for hooking into data abort.
>>>> Since pci-keystone.c will be used for AM65X platforms which is an
>>>> ARM64 platform,
>>>
>>> Hi Kishon,
>>>
>>> How is the problem plugged by the fault hook fixed on ARM64 ?
>>
>> At least in AM654 platform, I don't see a bus error when PCIe device
>> is not connected but returns 0xffffffff. So there is no necessary for
>> hook_fault_code in AM654 platform.
> 
> That can't have much to do with ARM32<->ARM64, it is rather a platform
> integration issue AFAICS. Russell has a point, this has to be documented
> I can do it for you but I need additional information.

Right now only ARM32 exports hook_fault_code which was used by K2G (ARM32
platform) and since AM654 (ARM64 platform) uses the same driver, it'll result
in compilation error. Also AM654 doesn't require hook_fault_code (or something
similar) because it doesn't throw bus error on accessing PCIe address space
when PCI device is not connected).

Are you looking for some other information?

Thanks
Kishon
> 
> Thanks,
> Lorenzo
> 
>> Thanks
>> Kishon
>>
>>>
>>> Thanks,
>>> Lorenzo
>>>
>>>> allow hook_fault_code to be compiled only for ARM32.
>>>>
>>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>>>> ---
>>>>  drivers/pci/controller/dwc/pci-keystone.c | 4 ++++
>>>>  1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
>>>> index dfe54553d832..93296d434f40 100644
>>>> --- a/drivers/pci/controller/dwc/pci-keystone.c
>>>> +++ b/drivers/pci/controller/dwc/pci-keystone.c
>>>> @@ -710,6 +710,7 @@ static int ks_pcie_config_legacy_irq(struct keystone_pcie *ks_pcie)
>>>>  	return ret;
>>>>  }
>>>>  
>>>> +#ifdef CONFIG_ARM
>>>>  /*
>>>>   * When a PCI device does not exist during config cycles, keystone host gets a
>>>>   * bus error instead of returning 0xffffffff. This handler always returns 0
>>>> @@ -729,6 +730,7 @@ static int ks_pcie_fault(unsigned long addr, unsigned int fsr,
>>>>  
>>>>  	return 0;
>>>>  }
>>>> +#endif
>>>>  
>>>>  static int __init ks_pcie_init_id(struct keystone_pcie *ks_pcie)
>>>>  {
>>>> @@ -778,12 +780,14 @@ static int __init ks_pcie_host_init(struct pcie_port *pp)
>>>>  	if (ret < 0)
>>>>  		return ret;
>>>>  
>>>> +#ifdef CONFIG_ARM
>>>>  	/*
>>>>  	 * PCIe access errors that result into OCP errors are caught by ARM as
>>>>  	 * "External aborts"
>>>>  	 */
>>>>  	hook_fault_code(17, ks_pcie_fault, SIGBUS, 0,
>>>>  			"Asynchronous external abort");
>>>> +#endif
>>>>  
>>>>  	ks_pcie_start_link(pci);
>>>>  	dw_pcie_wait_for_link(pci);
>>>> -- 
>>>> 2.17.1
>>>>

Re: [PATCH v3 12/26] PCI: keystone: Prevent ARM32 specific code to be compiled for ARM64

[Lorenzo Pieralisi]

On Fri, Apr 12, 2019 at 04:59:36PM +0530, Kishon Vijay Abraham I wrote:
> Hi Lorenzo,
> 
> On 12/04/19 4:41 PM, Lorenzo Pieralisi wrote:
> > On Fri, Apr 12, 2019 at 02:20:06PM +0530, Kishon Vijay Abraham I wrote:
> >> Hi Lorenzo,
> >>
> >> On 11/04/19 8:33 PM, Lorenzo Pieralisi wrote:
> >>> On Mon, Mar 25, 2019 at 03:09:33PM +0530, Kishon Vijay Abraham I wrote:
> >>>> hook_fault_code is an ARM32 specific API for hooking into data abort.
> >>>> Since pci-keystone.c will be used for AM65X platforms which is an
> >>>> ARM64 platform,
> >>>
> >>> Hi Kishon,
> >>>
> >>> How is the problem plugged by the fault hook fixed on ARM64 ?
> >>
> >> At least in AM654 platform, I don't see a bus error when PCIe device
> >> is not connected but returns 0xffffffff. So there is no necessary for
> >> hook_fault_code in AM654 platform.
> > 
> > That can't have much to do with ARM32<->ARM64, it is rather a platform
> > integration issue AFAICS. Russell has a point, this has to be documented
> > I can do it for you but I need additional information.
> 
> Right now only ARM32 exports hook_fault_code which was used by K2G (ARM32
> platform) and since AM654 (ARM64 platform) uses the same driver, it'll result
> in compilation error. Also AM654 doesn't require hook_fault_code (or something
> similar) because it doesn't throw bus error on accessing PCIe address space
> when PCI device is not connected).
> 
> Are you looking for some other information?

No thanks it was to confirm my understanding, it is really not
an ARM<->ARM64 issue, I will try to phrase it in a way that
is more explicit.

Lorenzo

> 
> Thanks
> Kishon
> > 
> > Thanks,
> > Lorenzo
> > 
> >> Thanks
> >> Kishon
> >>
> >>>
> >>> Thanks,
> >>> Lorenzo
> >>>
> >>>> allow hook_fault_code to be compiled only for ARM32.
> >>>>
> >>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> >>>> ---
> >>>>  drivers/pci/controller/dwc/pci-keystone.c | 4 ++++
> >>>>  1 file changed, 4 insertions(+)
> >>>>
> >>>> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
> >>>> index dfe54553d832..93296d434f40 100644
> >>>> --- a/drivers/pci/controller/dwc/pci-keystone.c
> >>>> +++ b/drivers/pci/controller/dwc/pci-keystone.c
> >>>> @@ -710,6 +710,7 @@ static int ks_pcie_config_legacy_irq(struct keystone_pcie *ks_pcie)
> >>>>  	return ret;
> >>>>  }
> >>>>  
> >>>> +#ifdef CONFIG_ARM
> >>>>  /*
> >>>>   * When a PCI device does not exist during config cycles, keystone host gets a
> >>>>   * bus error instead of returning 0xffffffff. This handler always returns 0
> >>>> @@ -729,6 +730,7 @@ static int ks_pcie_fault(unsigned long addr, unsigned int fsr,
> >>>>  
> >>>>  	return 0;
> >>>>  }
> >>>> +#endif
> >>>>  
> >>>>  static int __init ks_pcie_init_id(struct keystone_pcie *ks_pcie)
> >>>>  {
> >>>> @@ -778,12 +780,14 @@ static int __init ks_pcie_host_init(struct pcie_port *pp)
> >>>>  	if (ret < 0)
> >>>>  		return ret;
> >>>>  
> >>>> +#ifdef CONFIG_ARM
> >>>>  	/*
> >>>>  	 * PCIe access errors that result into OCP errors are caught by ARM as
> >>>>  	 * "External aborts"
> >>>>  	 */
> >>>>  	hook_fault_code(17, ks_pcie_fault, SIGBUS, 0,
> >>>>  			"Asynchronous external abort");
> >>>> +#endif
> >>>>  
> >>>>  	ks_pcie_start_link(pci);
> >>>>  	dw_pcie_wait_for_link(pci);
> >>>> -- 
> >>>> 2.17.1
> >>>>

[PATCH v3 13/26] dt-bindings: PCI: Add PCI RC dt binding documentation for AM654

[Kishon Vijay Abraham I]

Add devicetree binding documentation for PCIe in RC mode present in
AM654 SoC.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/pci/pci-keystone.txt | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/pci/pci-keystone.txt b/Documentation/devicetree/bindings/pci/pci-keystone.txt
index 8ee07197a063..5c60e911b8b1 100644
--- a/Documentation/devicetree/bindings/pci/pci-keystone.txt
+++ b/Documentation/devicetree/bindings/pci/pci-keystone.txt
@@ -11,7 +11,8 @@ described here as well as properties that are not applicable.
 
 Required Properties:-
 
-compatibility: "ti,keystone-pcie"
+compatibility: Should be "ti,keystone-pcie" for RC on Keystone2 SoC
+	       Should be "ti,am654-pcie-rc" for RC on AM654x SoC
 reg: Three register ranges as listed in the reg-names property
 reg-names: "dbics" for the DesignWare PCIe registers, "app" for the
 	   TI specific application registers, "config" for the
@@ -20,6 +21,9 @@ reg-names: "dbics" for the DesignWare PCIe registers, "app" for the
 pcie_msi_intc : Interrupt controller device node for MSI IRQ chip
 	interrupt-cells: should be set to 1
 	interrupts: GIC interrupt lines connected to PCI MSI interrupt lines
+	(required if the compatible is "ti,keystone-pcie")
+msi-map: As specified in Documentation/devicetree/bindings/pci/pci-msi.txt
+	 (required if the compatible is "ti,am654-pcie-rc".
 
 ti,syscon-pcie-id : phandle to the device control module required to set device
 		    id and vendor id.
-- 
2.17.1

[PATCH v3 14/26] PCI: keystone: Add support for PCIe RC in AM654x Platforms

[Kishon Vijay Abraham I]

Add PCIe RC support for AM654x Platforms in pci-keystone.c

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/pci/controller/dwc/Kconfig        |   2 +-
 drivers/pci/controller/dwc/pci-keystone.c | 161 ++++++++++++++++++++--
 2 files changed, 148 insertions(+), 15 deletions(-)

diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
index 6ea74b1c0d94..d1d00833e0b3 100644
--- a/drivers/pci/controller/dwc/Kconfig
+++ b/drivers/pci/controller/dwc/Kconfig
@@ -104,7 +104,7 @@ config PCIE_SPEAR13XX
 
 config PCI_KEYSTONE
 	bool "TI Keystone PCIe controller"
-	depends on ARCH_KEYSTONE || (ARM && COMPILE_TEST)
+	depends on ARCH_KEYSTONE || ARCH_K3 || ((ARM || ARM64) && COMPILE_TEST)
 	depends on PCI_MSI_IRQ_DOMAIN
 	select PCIE_DW_HOST
 	help
diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
index 93296d434f40..a6a482bd648f 100644
--- a/drivers/pci/controller/dwc/pci-keystone.c
+++ b/drivers/pci/controller/dwc/pci-keystone.c
@@ -11,6 +11,7 @@
 
 #include <linux/clk.h>
 #include <linux/delay.h>
+#include <linux/gpio/consumer.h>
 #include <linux/init.h>
 #include <linux/interrupt.h>
 #include <linux/irqchip/chained_irq.h>
@@ -18,6 +19,7 @@
 #include <linux/mfd/syscon.h>
 #include <linux/msi.h>
 #include <linux/of.h>
+#include <linux/of_device.h>
 #include <linux/of_irq.h>
 #include <linux/of_pci.h>
 #include <linux/phy/phy.h>
@@ -65,6 +67,7 @@
 #define ERR_IRQ_STATUS			0x1c4
 #define ERR_IRQ_ENABLE_SET		0x1c8
 #define ERR_AER				BIT(5)	/* ECRC error */
+#define AM6_ERR_AER			BIT(4)	/* AM6 ECRC error */
 #define ERR_AXI				BIT(4)	/* AXI tag lookup fatal error */
 #define ERR_CORR			BIT(3)	/* Correctable error */
 #define ERR_NONFATAL			BIT(2)	/* Non-fatal error */
@@ -88,8 +91,15 @@
 
 #define KS_PCIE_SYSCLOCKOUTEN		BIT(0)
 
+#define AM654_PCIE_DEV_TYPE_MASK	0x3
+
 #define to_keystone_pcie(x)		dev_get_drvdata((x)->dev)
 
+struct ks_pcie_of_data {
+	const struct dw_pcie_host_ops *host_ops;
+	unsigned int version;
+};
+
 struct keystone_pcie {
 	struct dw_pcie		*pci;
 	/* PCI Device ID */
@@ -109,6 +119,7 @@ struct keystone_pcie {
 	/* Application register space */
 	void __iomem		*va_app_base;	/* DT 1st resource */
 	struct resource		app;
+	bool			is_am6;
 };
 
 static u32 ks_pcie_app_readl(struct keystone_pcie *ks_pcie, u32 offset)
@@ -250,6 +261,16 @@ static void ks_pcie_handle_legacy_irq(struct keystone_pcie *ks_pcie,
 	ks_pcie_app_writel(ks_pcie, IRQ_EOI, offset);
 }
 
+static int ks_pcie_am654_msi_host_init(struct pcie_port *pp)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+	struct device *dev = pci->dev;
+
+	dev_vdbg(dev, "dummy function so that DW core doesn't configure MSI\n");
+
+	return 0;
+}
+
 static void ks_pcie_enable_error_irq(struct keystone_pcie *ks_pcie)
 {
 	ks_pcie_app_writel(ks_pcie, ERR_IRQ_ENABLE_SET, ERR_IRQ_ALL);
@@ -276,10 +297,10 @@ static irqreturn_t ks_pcie_handle_error_irq(struct keystone_pcie *ks_pcie)
 	if (reg & ERR_CORR)
 		dev_dbg(dev, "Correctable Error\n");
 
-	if (reg & ERR_AXI)
+	if (!ks_pcie->is_am6 && (reg & ERR_AXI))
 		dev_err(dev, "AXI tag lookup fatal Error\n");
 
-	if (reg & ERR_AER)
+	if (reg & ERR_AER || (ks_pcie->is_am6 && (reg & AM6_ERR_AER)))
 		dev_err(dev, "ECRC Error\n");
 
 	ks_pcie_app_writel(ks_pcie, ERR_IRQ_STATUS, reg);
@@ -377,6 +398,9 @@ static void ks_pcie_setup_rc_app_regs(struct keystone_pcie *ks_pcie)
 	dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_1, 0);
 	ks_pcie_clear_dbi_mode(ks_pcie);
 
+	if (ks_pcie->is_am6)
+		return;
+
 	val = ilog2(OB_WIN_SIZE);
 	ks_pcie_app_writel(ks_pcie, OB_SIZE, val);
 
@@ -619,6 +643,8 @@ static int ks_pcie_config_msi_irq(struct keystone_pcie *ks_pcie)
 
 	intc_np = of_get_child_by_name(np, "msi-interrupt-controller");
 	if (!intc_np) {
+		if (ks_pcie->is_am6)
+			return 0;
 		dev_warn(dev, "msi-interrupt-controller node is absent\n");
 		return -EINVAL;
 	}
@@ -668,6 +694,12 @@ static int ks_pcie_config_legacy_irq(struct keystone_pcie *ks_pcie)
 
 	intc_np = of_get_child_by_name(np, "legacy-interrupt-controller");
 	if (!intc_np) {
+		/*
+		 * Since legacy interrupts are modeled as edge-interrupts in
+		 * AM6, keep it disabled for now.
+		 */
+		if (ks_pcie->is_am6)
+			return 0;
 		dev_warn(dev, "legacy-interrupt-controller node is absent\n");
 		return -EINVAL;
 	}
@@ -749,8 +781,10 @@ static int __init ks_pcie_init_id(struct keystone_pcie *ks_pcie)
 	if (ret)
 		return ret;
 
+	dw_pcie_dbi_ro_wr_en(pci);
 	dw_pcie_writew_dbi(pci, PCI_VENDOR_ID, id & PCIE_VENDORID_MASK);
 	dw_pcie_writew_dbi(pci, PCI_DEVICE_ID, id >> PCIE_DEVICEID_SHIFT);
+	dw_pcie_dbi_ro_wr_dis(pci);
 
 	return 0;
 }
@@ -803,6 +837,11 @@ static const struct dw_pcie_host_ops ks_pcie_host_ops = {
 	.scan_bus = ks_pcie_v3_65_scan_bus,
 };
 
+static const struct dw_pcie_host_ops ks_pcie_am654_host_ops = {
+	.host_init = ks_pcie_host_init,
+	.msi_host_init = ks_pcie_am654_msi_host_init,
+};
+
 static irqreturn_t ks_pcie_err_irq_handler(int irq, void *priv)
 {
 	struct keystone_pcie *ks_pcie = priv;
@@ -826,7 +865,6 @@ static int __init ks_pcie_add_pcie_port(struct keystone_pcie *ks_pcie,
 
 	pp->va_cfg1_base = pp->va_cfg0_base;
 
-	pp->ops = &ks_pcie_host_ops;
 	ret = dw_pcie_host_init(pp);
 	if (ret) {
 		dev_err(dev, "failed to initialize host\n");
@@ -836,14 +874,6 @@ static int __init ks_pcie_add_pcie_port(struct keystone_pcie *ks_pcie,
 	return 0;
 }
 
-static const struct of_device_id ks_pcie_of_match[] = {
-	{
-		.type = "pci",
-		.compatible = "ti,keystone-pcie",
-	},
-	{ },
-};
-
 static const struct dw_pcie_ops ks_pcie_dw_pcie_ops = {
 	.start_link = ks_pcie_start_link,
 	.stop_link = ks_pcie_stop_link,
@@ -913,14 +943,67 @@ static int ks_pcie_set_mode(struct device *dev)
 	return 0;
 }
 
+static int ks_pcie_am654_set_mode(struct device *dev)
+{
+	struct device_node *np = dev->of_node;
+	struct regmap *syscon;
+	u32 val;
+	u32 mask;
+	int ret = 0;
+
+	syscon = syscon_regmap_lookup_by_phandle(np, "ti,syscon-pcie-mode");
+	if (IS_ERR(syscon))
+		return 0;
+
+	mask = AM654_PCIE_DEV_TYPE_MASK;
+	val = RC;
+
+	ret = regmap_update_bits(syscon, 0, mask, val);
+	if (ret) {
+		dev_err(dev, "failed to set pcie mode\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static const struct ks_pcie_of_data ks_pcie_rc_of_data = {
+	.host_ops = &ks_pcie_host_ops,
+	.version = 0x365A,
+};
+
+static const struct ks_pcie_of_data ks_pcie_am654_rc_of_data = {
+	.host_ops = &ks_pcie_am654_host_ops,
+	.version = 0x490A,
+};
+
+static const struct of_device_id ks_pcie_of_match[] = {
+	{
+		.type = "pci",
+		.data = &ks_pcie_rc_of_data,
+		.compatible = "ti,keystone-pcie",
+	},
+	{
+		.data = &ks_pcie_am654_rc_of_data,
+		.compatible = "ti,am654-pcie-rc",
+	},
+	{ },
+};
+
 static int __init ks_pcie_probe(struct platform_device *pdev)
 {
+	const struct dw_pcie_host_ops *host_ops;
 	struct device *dev = &pdev->dev;
 	struct device_node *np = dev->of_node;
+	const struct ks_pcie_of_data *data;
+	const struct of_device_id *match;
 	struct dw_pcie *pci;
 	struct keystone_pcie *ks_pcie;
 	struct device_link **link;
+	struct gpio_desc *gpiod;
+	void __iomem *atu_base;
 	struct resource *res;
+	unsigned int version;
 	void __iomem *base;
 	u32 num_viewport;
 	struct phy **phy;
@@ -930,6 +1013,14 @@ static int __init ks_pcie_probe(struct platform_device *pdev)
 	int irq;
 	int i;
 
+	match = of_match_device(of_match_ptr(ks_pcie_of_match), dev);
+	data = (struct ks_pcie_of_data *)match->data;
+	if (!data)
+		return -EINVAL;
+
+	version = data->version;
+	host_ops = data->host_ops;
+
 	ks_pcie = devm_kzalloc(dev, sizeof(*ks_pcie), GFP_KERNEL);
 	if (!ks_pcie)
 		return -ENOMEM;
@@ -950,9 +1041,13 @@ static int __init ks_pcie_probe(struct platform_device *pdev)
 	if (IS_ERR(base))
 		return PTR_ERR(base);
 
+	if (of_device_is_compatible(np, "ti,am654-pcie-rc"))
+		ks_pcie->is_am6 = true;
+
 	pci->dbi_base = base;
 	pci->dev = dev;
 	pci->ops = &ks_pcie_dw_pcie_ops;
+	pci->version = version;
 
 	ret = of_property_read_u32(np, "num-viewport", &num_viewport);
 	if (ret < 0) {
@@ -1011,6 +1106,15 @@ static int __init ks_pcie_probe(struct platform_device *pdev)
 	ks_pcie->num_viewport = num_viewport;
 	ks_pcie->phy = phy;
 
+	gpiod = devm_gpiod_get_optional(dev, "reset",
+					GPIOD_OUT_LOW);
+	if (IS_ERR(gpiod)) {
+		ret = PTR_ERR(gpiod);
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "Failed to get reset GPIO\n");
+		goto err_link;
+	}
+
 	ret = ks_pcie_enable_phy(ks_pcie);
 	if (ret) {
 		dev_err(dev, "failed to enable phy\n");
@@ -1025,10 +1129,39 @@ static int __init ks_pcie_probe(struct platform_device *pdev)
 		goto err_get_sync;
 	}
 
-	ret = ks_pcie_set_mode(dev);
-	if (ret < 0)
-		goto err_get_sync;
+	if (pci->version >= 0x480A) {
+		res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "atu");
+		atu_base = devm_ioremap_resource(dev, res);
+		if (IS_ERR(atu_base)) {
+			ret = PTR_ERR(atu_base);
+			goto err_get_sync;
+		}
+
+		pci->atu_base = atu_base;
+
+		ret = ks_pcie_am654_set_mode(dev);
+		if (ret < 0)
+			goto err_get_sync;
+	} else {
+		ret = ks_pcie_set_mode(dev);
+		if (ret < 0)
+			goto err_get_sync;
+	}
+
+	/*
+	 * "Power Sequencing and Reset Signal Timings" table in
+	 * PCI EXPRESS CARD ELECTROMECHANICAL SPECIFICATION, REV. 2.0
+	 * indicates PERST# should be deasserted after minimum of 100us
+	 * once REFCLK is stable. The REFCLK to the connector in RC
+	 * mode is selected while enabling the PHY. So deassert PERST#
+	 * after 100 us.
+	 */
+	if (gpiod) {
+		usleep_range(100, 200);
+		gpiod_set_value_cansleep(gpiod, 1);
+	}
 
+	pci->pp.ops = host_ops;
 	ret = ks_pcie_add_pcie_port(ks_pcie, pdev);
 	if (ret < 0)
 		goto err_get_sync;
-- 
2.17.1

Re: [PATCH v3 14/26] PCI: keystone: Add support for PCIe RC in AM654x Platforms

[Bjorn Helgaas]

On Mon, Mar 25, 2019 at 03:09:35PM +0530, Kishon Vijay Abraham I wrote:
> Add PCIe RC support for AM654x Platforms in pci-keystone.c

> +static int ks_pcie_am654_msi_host_init(struct pcie_port *pp)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +	struct device *dev = pci->dev;
> +
> +	dev_vdbg(dev, "dummy function so that DW core doesn't configure MSI\n");

Drive-by nit: this is the first occurrence of dev_vdbg() in
drivers/pci, and while dev_vdbg() might be useful somewhere, I'm not
sure this is the place.

It took me 10 minutes of archaeology to figure out what it is and how
to use it.  Apparently it does nothing at all unless you define
VERBOSE_DEBUG, which can be set via Kconfig in a few cases:

  drivers/dma/Makefile:subdir-ccflags-$(CONFIG_DMADEVICES_VDEBUG) += -DVERBOSE_DEBUG
  drivers/usb/dwc2/Makefile:ccflags-$(CONFIG_USB_DWC2_VERBOSE)    += -DVERBOSE_DEBUG
  drivers/usb/gadget/Makefile:subdir-ccflags-$(CONFIG_USB_GADGET_VERBOSE) += -DVERBOSE_DEBUG

but is more commonly set by adding "#define VERBOSE_DEBUG" manually
in a source file.  Neither one applies here, so I don't think this
actually does anything.

Anyway, I suspect this particular message would make a lot of sense as
a *comment*, but maybe not as a mysterious dev_vdbg() that never
actually does anything.

> +	return 0;
> +}

Re: [PATCH v3 14/26] PCI: keystone: Add support for PCIe RC in AM654x Platforms

[Lorenzo Pieralisi]

On Sat, Apr 13, 2019 at 10:26:33AM -0500, Bjorn Helgaas wrote:
> On Mon, Mar 25, 2019 at 03:09:35PM +0530, Kishon Vijay Abraham I wrote:
> > Add PCIe RC support for AM654x Platforms in pci-keystone.c
> 
> > +static int ks_pcie_am654_msi_host_init(struct pcie_port *pp)
> > +{
> > +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > +	struct device *dev = pci->dev;
> > +
> > +	dev_vdbg(dev, "dummy function so that DW core doesn't configure MSI\n");
> 
> Drive-by nit: this is the first occurrence of dev_vdbg() in
> drivers/pci, and while dev_vdbg() might be useful somewhere, I'm not
> sure this is the place.
> 
> It took me 10 minutes of archaeology to figure out what it is and how
> to use it.  Apparently it does nothing at all unless you define
> VERBOSE_DEBUG, which can be set via Kconfig in a few cases:
> 
>   drivers/dma/Makefile:subdir-ccflags-$(CONFIG_DMADEVICES_VDEBUG) += -DVERBOSE_DEBUG
>   drivers/usb/dwc2/Makefile:ccflags-$(CONFIG_USB_DWC2_VERBOSE)    += -DVERBOSE_DEBUG
>   drivers/usb/gadget/Makefile:subdir-ccflags-$(CONFIG_USB_GADGET_VERBOSE) += -DVERBOSE_DEBUG
> 
> but is more commonly set by adding "#define VERBOSE_DEBUG" manually
> in a source file.  Neither one applies here, so I don't think this
> actually does anything.
> 
> Anyway, I suspect this particular message would make a lot of sense as
> a *comment*, but maybe not as a mysterious dev_vdbg() that never
> actually does anything.

I agree. Kishon ? I would do what Bjorn suggests here, please let me
know.

Lorenzo

Re: [PATCH v3 14/26] PCI: keystone: Add support for PCIe RC in AM654x Platforms

[Kishon Vijay Abraham I]

Hi Lorenzo,

On 16/04/19 7:45 PM, Lorenzo Pieralisi wrote:
> On Sat, Apr 13, 2019 at 10:26:33AM -0500, Bjorn Helgaas wrote:
>> On Mon, Mar 25, 2019 at 03:09:35PM +0530, Kishon Vijay Abraham I wrote:
>>> Add PCIe RC support for AM654x Platforms in pci-keystone.c
>>
>>> +static int ks_pcie_am654_msi_host_init(struct pcie_port *pp)
>>> +{
>>> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>>> +	struct device *dev = pci->dev;
>>> +
>>> +	dev_vdbg(dev, "dummy function so that DW core doesn't configure MSI\n");
>>
>> Drive-by nit: this is the first occurrence of dev_vdbg() in
>> drivers/pci, and while dev_vdbg() might be useful somewhere, I'm not
>> sure this is the place.
>>
>> It took me 10 minutes of archaeology to figure out what it is and how
>> to use it.  Apparently it does nothing at all unless you define
>> VERBOSE_DEBUG, which can be set via Kconfig in a few cases:
>>
>>   drivers/dma/Makefile:subdir-ccflags-$(CONFIG_DMADEVICES_VDEBUG) += -DVERBOSE_DEBUG
>>   drivers/usb/dwc2/Makefile:ccflags-$(CONFIG_USB_DWC2_VERBOSE)    += -DVERBOSE_DEBUG
>>   drivers/usb/gadget/Makefile:subdir-ccflags-$(CONFIG_USB_GADGET_VERBOSE) += -DVERBOSE_DEBUG
>>
>> but is more commonly set by adding "#define VERBOSE_DEBUG" manually
>> in a source file.  Neither one applies here, so I don't think this
>> actually does anything.
>>
>> Anyway, I suspect this particular message would make a lot of sense as
>> a *comment*, but maybe not as a mysterious dev_vdbg() that never
>> actually does anything.
> 
> I agree. Kishon ? I would do what Bjorn suggests here, please let me
> know.

Sure.

Thanks
Kishon

[PATCH v3 15/26] PCI: keystone: Invoke phy_reset API before enabling PHY

[Kishon Vijay Abraham I]

SERDES connected to the PCIe controller in AM654 requires
power on reset enable (POR_EN) to be set in the SERDES. The
SERDES driver sets POR_EN in the reset ops and it has to be
invoked before init or enable ops. In order for SERDES driver
to set POR_EN, invoke phy_reset API in pci-keystone driver.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/pci/controller/dwc/pci-keystone.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
index a6a482bd648f..e4a816f53b8e 100644
--- a/drivers/pci/controller/dwc/pci-keystone.c
+++ b/drivers/pci/controller/dwc/pci-keystone.c
@@ -897,6 +897,10 @@ static int ks_pcie_enable_phy(struct keystone_pcie *ks_pcie)
 	int num_lanes = ks_pcie->num_lanes;
 
 	for (i = 0; i < num_lanes; i++) {
+		ret = phy_reset(ks_pcie->phy[i]);
+		if (ret < 0)
+			goto err_phy;
+
 		ret = phy_init(ks_pcie->phy[i]);
 		if (ret < 0)
 			goto err_phy;
-- 
2.17.1

[PATCH v3 16/26] PCI: OF: Allow of_pci_get_max_link_speed() to be used by PCI Endpoint drivers

[Kishon Vijay Abraham I]

of_pci_get_max_link_speed() is built only if CONFIG_PCI is enabled.
Make of_pci_get_max_link_speed() to be also used by PCI Endpoint
controllers with just CONFIG_PCI_ENDPOINT enabled.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
Signed-off-by: Sekhar Nori <nsekhar@ti.com>
---
 drivers/pci/Makefile |  2 +-
 drivers/pci/of.c     | 44 +++++++++++++++++++++++---------------------
 2 files changed, 24 insertions(+), 22 deletions(-)

diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
index 657d642fcc67..28cdd8c0213a 100644
--- a/drivers/pci/Makefile
+++ b/drivers/pci/Makefile
@@ -10,10 +10,10 @@ obj-$(CONFIG_PCI)		+= access.o bus.o probe.o host-bridge.o \
 ifdef CONFIG_PCI
 obj-$(CONFIG_PROC_FS)		+= proc.o
 obj-$(CONFIG_SYSFS)		+= slot.o
-obj-$(CONFIG_OF)		+= of.o
 obj-$(CONFIG_ACPI)		+= pci-acpi.o
 endif
 
+obj-$(CONFIG_OF)		+= of.o
 obj-$(CONFIG_PCI_QUIRKS)	+= quirks.o
 obj-$(CONFIG_PCIEPORTBUS)	+= pcie/
 obj-$(CONFIG_HOTPLUG_PCI)	+= hotplug/
diff --git a/drivers/pci/of.c b/drivers/pci/of.c
index 3d32da15c215..8095933f8452 100644
--- a/drivers/pci/of.c
+++ b/drivers/pci/of.c
@@ -15,6 +15,7 @@
 #include <linux/of_pci.h>
 #include "pci.h"
 
+#ifdef CONFIG_PCI
 void pci_set_of_node(struct pci_dev *dev)
 {
 	if (!dev->bus->dev.of_node)
@@ -196,27 +197,6 @@ int of_get_pci_domain_nr(struct device_node *node)
 }
 EXPORT_SYMBOL_GPL(of_get_pci_domain_nr);
 
-/**
- * This function will try to find the limitation of link speed by finding
- * a property called "max-link-speed" of the given device node.
- *
- * @node: device tree node with the max link speed information
- *
- * Returns the associated max link speed from DT, or a negative value if the
- * required property is not found or is invalid.
- */
-int of_pci_get_max_link_speed(struct device_node *node)
-{
-	u32 max_link_speed;
-
-	if (of_property_read_u32(node, "max-link-speed", &max_link_speed) ||
-	    max_link_speed > 4)
-		return -EINVAL;
-
-	return max_link_speed;
-}
-EXPORT_SYMBOL_GPL(of_pci_get_max_link_speed);
-
 /**
  * of_pci_check_probe_only - Setup probe only mode if linux,pci-probe-only
  *                           is present and valid
@@ -537,3 +517,25 @@ int pci_parse_request_of_pci_ranges(struct device *dev,
 	return err;
 }
 
+#endif /* CONFIG_PCI */
+
+/**
+ * This function will try to find the limitation of link speed by finding
+ * a property called "max-link-speed" of the given device node.
+ *
+ * @node: device tree node with the max link speed information
+ *
+ * Returns the associated max link speed from DT, or a negative value if the
+ * required property is not found or is invalid.
+ */
+int of_pci_get_max_link_speed(struct device_node *node)
+{
+	u32 max_link_speed;
+
+	if (of_property_read_u32(node, "max-link-speed", &max_link_speed) ||
+	    max_link_speed > 4)
+		return -EINVAL;
+
+	return max_link_speed;
+}
+EXPORT_SYMBOL_GPL(of_pci_get_max_link_speed);
-- 
2.17.1

Re: [PATCH v3 16/26] PCI: OF: Allow of_pci_get_max_link_speed() to be used by PCI Endpoint drivers

[Lorenzo Pieralisi]

On Mon, Mar 25, 2019 at 03:09:37PM +0530, Kishon Vijay Abraham I wrote:
> of_pci_get_max_link_speed() is built only if CONFIG_PCI is enabled.
> Make of_pci_get_max_link_speed() to be also used by PCI Endpoint
> controllers with just CONFIG_PCI_ENDPOINT enabled.
> 
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> Signed-off-by: Sekhar Nori <nsekhar@ti.com>
> ---
>  drivers/pci/Makefile |  2 +-
>  drivers/pci/of.c     | 44 +++++++++++++++++++++++---------------------
>  2 files changed, 24 insertions(+), 22 deletions(-)

I delegated this patch to Bjorn in the patch queue since it needs his
review/ACK to proceed.

Thanks,
Lorenzo

> diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
> index 657d642fcc67..28cdd8c0213a 100644
> --- a/drivers/pci/Makefile
> +++ b/drivers/pci/Makefile
> @@ -10,10 +10,10 @@ obj-$(CONFIG_PCI)		+= access.o bus.o probe.o host-bridge.o \
>  ifdef CONFIG_PCI
>  obj-$(CONFIG_PROC_FS)		+= proc.o
>  obj-$(CONFIG_SYSFS)		+= slot.o
> -obj-$(CONFIG_OF)		+= of.o
>  obj-$(CONFIG_ACPI)		+= pci-acpi.o
>  endif
>  
> +obj-$(CONFIG_OF)		+= of.o
>  obj-$(CONFIG_PCI_QUIRKS)	+= quirks.o
>  obj-$(CONFIG_PCIEPORTBUS)	+= pcie/
>  obj-$(CONFIG_HOTPLUG_PCI)	+= hotplug/
> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> index 3d32da15c215..8095933f8452 100644
> --- a/drivers/pci/of.c
> +++ b/drivers/pci/of.c
> @@ -15,6 +15,7 @@
>  #include <linux/of_pci.h>
>  #include "pci.h"
>  
> +#ifdef CONFIG_PCI
>  void pci_set_of_node(struct pci_dev *dev)
>  {
>  	if (!dev->bus->dev.of_node)
> @@ -196,27 +197,6 @@ int of_get_pci_domain_nr(struct device_node *node)
>  }
>  EXPORT_SYMBOL_GPL(of_get_pci_domain_nr);
>  
> -/**
> - * This function will try to find the limitation of link speed by finding
> - * a property called "max-link-speed" of the given device node.
> - *
> - * @node: device tree node with the max link speed information
> - *
> - * Returns the associated max link speed from DT, or a negative value if the
> - * required property is not found or is invalid.
> - */
> -int of_pci_get_max_link_speed(struct device_node *node)
> -{
> -	u32 max_link_speed;
> -
> -	if (of_property_read_u32(node, "max-link-speed", &max_link_speed) ||
> -	    max_link_speed > 4)
> -		return -EINVAL;
> -
> -	return max_link_speed;
> -}
> -EXPORT_SYMBOL_GPL(of_pci_get_max_link_speed);
> -
>  /**
>   * of_pci_check_probe_only - Setup probe only mode if linux,pci-probe-only
>   *                           is present and valid
> @@ -537,3 +517,25 @@ int pci_parse_request_of_pci_ranges(struct device *dev,
>  	return err;
>  }
>  
> +#endif /* CONFIG_PCI */
> +
> +/**
> + * This function will try to find the limitation of link speed by finding
> + * a property called "max-link-speed" of the given device node.
> + *
> + * @node: device tree node with the max link speed information
> + *
> + * Returns the associated max link speed from DT, or a negative value if the
> + * required property is not found or is invalid.
> + */
> +int of_pci_get_max_link_speed(struct device_node *node)
> +{
> +	u32 max_link_speed;
> +
> +	if (of_property_read_u32(node, "max-link-speed", &max_link_speed) ||
> +	    max_link_speed > 4)
> +		return -EINVAL;
> +
> +	return max_link_speed;
> +}
> +EXPORT_SYMBOL_GPL(of_pci_get_max_link_speed);
> -- 
> 2.17.1
> 

Re: [PATCH v3 16/26] PCI: OF: Allow of_pci_get_max_link_speed() to be used by PCI Endpoint drivers

[Bjorn Helgaas]

On Mon, Mar 25, 2019 at 03:09:37PM +0530, Kishon Vijay Abraham I wrote:
> of_pci_get_max_link_speed() is built only if CONFIG_PCI is enabled.
> Make of_pci_get_max_link_speed() to be also used by PCI Endpoint
> controllers with just CONFIG_PCI_ENDPOINT enabled.
> 
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> Signed-off-by: Sekhar Nori <nsekhar@ti.com>

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

> ---
>  drivers/pci/Makefile |  2 +-
>  drivers/pci/of.c     | 44 +++++++++++++++++++++++---------------------
>  2 files changed, 24 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
> index 657d642fcc67..28cdd8c0213a 100644
> --- a/drivers/pci/Makefile
> +++ b/drivers/pci/Makefile
> @@ -10,10 +10,10 @@ obj-$(CONFIG_PCI)		+= access.o bus.o probe.o host-bridge.o \
>  ifdef CONFIG_PCI
>  obj-$(CONFIG_PROC_FS)		+= proc.o
>  obj-$(CONFIG_SYSFS)		+= slot.o
> -obj-$(CONFIG_OF)		+= of.o
>  obj-$(CONFIG_ACPI)		+= pci-acpi.o
>  endif
>  
> +obj-$(CONFIG_OF)		+= of.o
>  obj-$(CONFIG_PCI_QUIRKS)	+= quirks.o
>  obj-$(CONFIG_PCIEPORTBUS)	+= pcie/
>  obj-$(CONFIG_HOTPLUG_PCI)	+= hotplug/
> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> index 3d32da15c215..8095933f8452 100644
> --- a/drivers/pci/of.c
> +++ b/drivers/pci/of.c
> @@ -15,6 +15,7 @@
>  #include <linux/of_pci.h>
>  #include "pci.h"
>  
> +#ifdef CONFIG_PCI
>  void pci_set_of_node(struct pci_dev *dev)
>  {
>  	if (!dev->bus->dev.of_node)
> @@ -196,27 +197,6 @@ int of_get_pci_domain_nr(struct device_node *node)
>  }
>  EXPORT_SYMBOL_GPL(of_get_pci_domain_nr);
>  
> -/**
> - * This function will try to find the limitation of link speed by finding
> - * a property called "max-link-speed" of the given device node.
> - *
> - * @node: device tree node with the max link speed information
> - *
> - * Returns the associated max link speed from DT, or a negative value if the
> - * required property is not found or is invalid.
> - */
> -int of_pci_get_max_link_speed(struct device_node *node)
> -{
> -	u32 max_link_speed;
> -
> -	if (of_property_read_u32(node, "max-link-speed", &max_link_speed) ||
> -	    max_link_speed > 4)
> -		return -EINVAL;
> -
> -	return max_link_speed;
> -}
> -EXPORT_SYMBOL_GPL(of_pci_get_max_link_speed);
> -
>  /**
>   * of_pci_check_probe_only - Setup probe only mode if linux,pci-probe-only
>   *                           is present and valid
> @@ -537,3 +517,25 @@ int pci_parse_request_of_pci_ranges(struct device *dev,
>  	return err;
>  }
>  
> +#endif /* CONFIG_PCI */
> +
> +/**
> + * This function will try to find the limitation of link speed by finding
> + * a property called "max-link-speed" of the given device node.
> + *
> + * @node: device tree node with the max link speed information
> + *
> + * Returns the associated max link speed from DT, or a negative value if the
> + * required property is not found or is invalid.
> + */
> +int of_pci_get_max_link_speed(struct device_node *node)
> +{
> +	u32 max_link_speed;
> +
> +	if (of_property_read_u32(node, "max-link-speed", &max_link_speed) ||
> +	    max_link_speed > 4)
> +		return -EINVAL;
> +
> +	return max_link_speed;
> +}
> +EXPORT_SYMBOL_GPL(of_pci_get_max_link_speed);
> -- 
> 2.17.1
> 

[PATCH v3 17/26] PCI: keystone: Add support to set the max link speed from DT

[Kishon Vijay Abraham I]

PCIe in TI's AM654 devices is by default configured to work in GEN3 mode.
However PCIe doesn't work reliably in GEN3 mode because of SERDES
configuration.

Add support to set the link speed to GEN1, GEN2 or GEN3 based on
"max-link-speed" dt property with GEN2 as the default speed if
"max-link-speed" is absent.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/pci/controller/dwc/pci-keystone.c | 35 +++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
index e4a816f53b8e..312fd0c49bbb 100644
--- a/drivers/pci/controller/dwc/pci-keystone.c
+++ b/drivers/pci/controller/dwc/pci-keystone.c
@@ -28,6 +28,7 @@
 #include <linux/resource.h>
 #include <linux/signal.h>
 
+#include "../../pci.h"
 #include "pcie-designware.h"
 
 #define PCIE_VENDORID_MASK	0xffff
@@ -89,6 +90,8 @@
 #define LEG_EP				0x1
 #define RC				0x2
 
+#define EXP_CAP_ID_OFFSET		0x70
+
 #define KS_PCIE_SYSCLOCKOUTEN		BIT(0)
 
 #define AM654_PCIE_DEV_TYPE_MASK	0x3
@@ -971,6 +974,31 @@ static int ks_pcie_am654_set_mode(struct device *dev)
 	return 0;
 }
 
+static void ks_pcie_set_link_speed(struct dw_pcie *pci, int link_speed)
+{
+	u32 val;
+
+	dw_pcie_dbi_ro_wr_en(pci);
+
+	val = dw_pcie_readl_dbi(pci, EXP_CAP_ID_OFFSET + PCI_EXP_LNKCAP);
+	if ((val & PCI_EXP_LNKCAP_SLS) != link_speed) {
+		val &= ~((u32)PCI_EXP_LNKCAP_SLS);
+		val |= link_speed;
+		dw_pcie_writel_dbi(pci, EXP_CAP_ID_OFFSET + PCI_EXP_LNKCAP,
+				   val);
+	}
+
+	val = dw_pcie_readl_dbi(pci, EXP_CAP_ID_OFFSET + PCI_EXP_LNKCTL2);
+	if ((val & PCI_EXP_LNKCAP_SLS) != link_speed) {
+		val &= ~((u32)PCI_EXP_LNKCAP_SLS);
+		val |= link_speed;
+		dw_pcie_writel_dbi(pci, EXP_CAP_ID_OFFSET + PCI_EXP_LNKCTL2,
+				   val);
+	}
+
+	dw_pcie_dbi_ro_wr_dis(pci);
+}
+
 static const struct ks_pcie_of_data ks_pcie_rc_of_data = {
 	.host_ops = &ks_pcie_host_ops,
 	.version = 0x365A,
@@ -1011,6 +1039,7 @@ static int __init ks_pcie_probe(struct platform_device *pdev)
 	void __iomem *base;
 	u32 num_viewport;
 	struct phy **phy;
+	int link_speed;
 	u32 num_lanes;
 	char name[10];
 	int ret;
@@ -1165,6 +1194,12 @@ static int __init ks_pcie_probe(struct platform_device *pdev)
 		gpiod_set_value_cansleep(gpiod, 1);
 	}
 
+	link_speed = of_pci_get_max_link_speed(np);
+	if (link_speed < 0)
+		link_speed = 2;
+
+	ks_pcie_set_link_speed(pci, link_speed);
+
 	pci->pp.ops = host_ops;
 	ret = ks_pcie_add_pcie_port(ks_pcie, pdev);
 	if (ret < 0)
-- 
2.17.1

[PATCH v3 18/26] PCI: endpoint: Add support to allocate aligned buffers to be mapped in BARs

[Kishon Vijay Abraham I]

Modify pci_epf_alloc_space API to take alignment size as argument in
order to argument in order to allocate aligned buffers to be mapped to
BARs.

Add 'align' parameter to epc_features which can be used by platform
drivers to specifiy the BAR allocation alignment requirements and use
this while invoking pci_epf_alloc_space.

This is mainly required for Synopsys Designware PCIe core which masks
the lower bits based on the BAR size (See "I/O and MEM Match Modes"
section in DesignWare Cores PCI Express Controller Databook version
4.90a).

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/pci/endpoint/functions/pci-epf-test.c |  5 +++--
 drivers/pci/endpoint/pci-epf-core.c           | 10 ++++++++--
 include/linux/pci-epc.h                       |  2 ++
 include/linux/pci-epf.h                       |  3 ++-
 4 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
index d0b91da49bf4..c0786ca74312 100644
--- a/drivers/pci/endpoint/functions/pci-epf-test.c
+++ b/drivers/pci/endpoint/functions/pci-epf-test.c
@@ -438,7 +438,7 @@ static int pci_epf_test_alloc_space(struct pci_epf *epf)
 	epc_features = epf_test->epc_features;
 
 	base = pci_epf_alloc_space(epf, sizeof(struct pci_epf_test_reg),
-				   test_reg_bar);
+				   test_reg_bar, epc_features->align);
 	if (!base) {
 		dev_err(dev, "Failed to allocated register space\n");
 		return -ENOMEM;
@@ -453,7 +453,8 @@ static int pci_epf_test_alloc_space(struct pci_epf *epf)
 		if (!!(epc_features->reserved_bar & (1 << bar)))
 			continue;
 
-		base = pci_epf_alloc_space(epf, bar_size[bar], bar);
+		base = pci_epf_alloc_space(epf, bar_size[bar], bar,
+					   epc_features->align);
 		if (!base)
 			dev_err(dev, "Failed to allocate space for BAR%d\n",
 				bar);
diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
index 8bfdcd291196..fb1306de8f40 100644
--- a/drivers/pci/endpoint/pci-epf-core.c
+++ b/drivers/pci/endpoint/pci-epf-core.c
@@ -109,10 +109,12 @@ EXPORT_SYMBOL_GPL(pci_epf_free_space);
  * pci_epf_alloc_space() - allocate memory for the PCI EPF register space
  * @size: the size of the memory that has to be allocated
  * @bar: the BAR number corresponding to the allocated register space
+ * @align: alignment size for the allocation region
  *
  * Invoke to allocate memory for the PCI EPF register space.
  */
-void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar)
+void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar,
+			  size_t align)
 {
 	void *space;
 	struct device *dev = epf->epc->dev.parent;
@@ -120,7 +122,11 @@ void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar)
 
 	if (size < 128)
 		size = 128;
-	size = roundup_pow_of_two(size);
+
+	if (align)
+		size = ALIGN(size, align);
+	else
+		size = roundup_pow_of_two(size);
 
 	space = dma_alloc_coherent(dev, size, &phys_addr, GFP_KERNEL);
 	if (!space) {
diff --git a/include/linux/pci-epc.h b/include/linux/pci-epc.h
index c3ffa3917f88..f641badc2c61 100644
--- a/include/linux/pci-epc.h
+++ b/include/linux/pci-epc.h
@@ -109,6 +109,7 @@ struct pci_epc {
  * @reserved_bar: bitmap to indicate reserved BAR unavailable to function driver
  * @bar_fixed_64bit: bitmap to indicate fixed 64bit BARs
  * @bar_fixed_size: Array specifying the size supported by each BAR
+ * @align: alignment size required for BAR buffer allocation
  */
 struct pci_epc_features {
 	unsigned int	linkup_notifier : 1;
@@ -117,6 +118,7 @@ struct pci_epc_features {
 	u8	reserved_bar;
 	u8	bar_fixed_64bit;
 	u64	bar_fixed_size[BAR_5 + 1];
+	size_t	align;
 };
 
 #define to_pci_epc(device) container_of((device), struct pci_epc, dev)
diff --git a/include/linux/pci-epf.h b/include/linux/pci-epf.h
index ec02f58758c8..2d6f07556682 100644
--- a/include/linux/pci-epf.h
+++ b/include/linux/pci-epf.h
@@ -149,7 +149,8 @@ void pci_epf_destroy(struct pci_epf *epf);
 int __pci_epf_register_driver(struct pci_epf_driver *driver,
 			      struct module *owner);
 void pci_epf_unregister_driver(struct pci_epf_driver *driver);
-void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar);
+void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar,
+			  size_t align);
 void pci_epf_free_space(struct pci_epf *epf, void *addr, enum pci_barno bar);
 int pci_epf_bind(struct pci_epf *epf);
 void pci_epf_unbind(struct pci_epf *epf);
-- 
2.17.1

Re: [PATCH v3 18/26] PCI: endpoint: Add support to allocate aligned buffers to be mapped in BARs

[Lorenzo Pieralisi]

On Mon, Mar 25, 2019 at 03:09:39PM +0530, Kishon Vijay Abraham I wrote:
> Modify pci_epf_alloc_space API to take alignment size as argument in
> order to argument in order to allocate aligned buffers to be mapped to
> BARs.
> 
> Add 'align' parameter to epc_features which can be used by platform
> drivers to specifiy the BAR allocation alignment requirements and use
> this while invoking pci_epf_alloc_space.
> 
> This is mainly required for Synopsys Designware PCIe core which masks
> the lower bits based on the BAR size (See "I/O and MEM Match Modes"
> section in DesignWare Cores PCI Express Controller Databook version
> 4.90a).

Hi Kishon,

isn't this a generic PCI BAR requirement rather than a Designware
specific one ?

The patch is fine I just would like to understand the statement above.

Thanks,
Lorenzo

> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  drivers/pci/endpoint/functions/pci-epf-test.c |  5 +++--
>  drivers/pci/endpoint/pci-epf-core.c           | 10 ++++++++--
>  include/linux/pci-epc.h                       |  2 ++
>  include/linux/pci-epf.h                       |  3 ++-
>  4 files changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index d0b91da49bf4..c0786ca74312 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -438,7 +438,7 @@ static int pci_epf_test_alloc_space(struct pci_epf *epf)
>  	epc_features = epf_test->epc_features;
>  
>  	base = pci_epf_alloc_space(epf, sizeof(struct pci_epf_test_reg),
> -				   test_reg_bar);
> +				   test_reg_bar, epc_features->align);
>  	if (!base) {
>  		dev_err(dev, "Failed to allocated register space\n");
>  		return -ENOMEM;
> @@ -453,7 +453,8 @@ static int pci_epf_test_alloc_space(struct pci_epf *epf)
>  		if (!!(epc_features->reserved_bar & (1 << bar)))
>  			continue;
>  
> -		base = pci_epf_alloc_space(epf, bar_size[bar], bar);
> +		base = pci_epf_alloc_space(epf, bar_size[bar], bar,
> +					   epc_features->align);
>  		if (!base)
>  			dev_err(dev, "Failed to allocate space for BAR%d\n",
>  				bar);
> diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
> index 8bfdcd291196..fb1306de8f40 100644
> --- a/drivers/pci/endpoint/pci-epf-core.c
> +++ b/drivers/pci/endpoint/pci-epf-core.c
> @@ -109,10 +109,12 @@ EXPORT_SYMBOL_GPL(pci_epf_free_space);
>   * pci_epf_alloc_space() - allocate memory for the PCI EPF register space
>   * @size: the size of the memory that has to be allocated
>   * @bar: the BAR number corresponding to the allocated register space
> + * @align: alignment size for the allocation region
>   *
>   * Invoke to allocate memory for the PCI EPF register space.
>   */
> -void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar)
> +void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar,
> +			  size_t align)
>  {
>  	void *space;
>  	struct device *dev = epf->epc->dev.parent;
> @@ -120,7 +122,11 @@ void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar)
>  
>  	if (size < 128)
>  		size = 128;
> -	size = roundup_pow_of_two(size);
> +
> +	if (align)
> +		size = ALIGN(size, align);
> +	else
> +		size = roundup_pow_of_two(size);
>  
>  	space = dma_alloc_coherent(dev, size, &phys_addr, GFP_KERNEL);
>  	if (!space) {
> diff --git a/include/linux/pci-epc.h b/include/linux/pci-epc.h
> index c3ffa3917f88..f641badc2c61 100644
> --- a/include/linux/pci-epc.h
> +++ b/include/linux/pci-epc.h
> @@ -109,6 +109,7 @@ struct pci_epc {
>   * @reserved_bar: bitmap to indicate reserved BAR unavailable to function driver
>   * @bar_fixed_64bit: bitmap to indicate fixed 64bit BARs
>   * @bar_fixed_size: Array specifying the size supported by each BAR
> + * @align: alignment size required for BAR buffer allocation
>   */
>  struct pci_epc_features {
>  	unsigned int	linkup_notifier : 1;
> @@ -117,6 +118,7 @@ struct pci_epc_features {
>  	u8	reserved_bar;
>  	u8	bar_fixed_64bit;
>  	u64	bar_fixed_size[BAR_5 + 1];
> +	size_t	align;
>  };
>  
>  #define to_pci_epc(device) container_of((device), struct pci_epc, dev)
> diff --git a/include/linux/pci-epf.h b/include/linux/pci-epf.h
> index ec02f58758c8..2d6f07556682 100644
> --- a/include/linux/pci-epf.h
> +++ b/include/linux/pci-epf.h
> @@ -149,7 +149,8 @@ void pci_epf_destroy(struct pci_epf *epf);
>  int __pci_epf_register_driver(struct pci_epf_driver *driver,
>  			      struct module *owner);
>  void pci_epf_unregister_driver(struct pci_epf_driver *driver);
> -void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar);
> +void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar,
> +			  size_t align);
>  void pci_epf_free_space(struct pci_epf *epf, void *addr, enum pci_barno bar);
>  int pci_epf_bind(struct pci_epf *epf);
>  void pci_epf_unbind(struct pci_epf *epf);
> -- 
> 2.17.1
> 

Re: [PATCH v3 18/26] PCI: endpoint: Add support to allocate aligned buffers to be mapped in BARs

[Kishon Vijay Abraham I]

Hi Lorenzo,

On 11/04/19 9:02 PM, Lorenzo Pieralisi wrote:
> On Mon, Mar 25, 2019 at 03:09:39PM +0530, Kishon Vijay Abraham I wrote:
>> Modify pci_epf_alloc_space API to take alignment size as argument in
>> order to argument in order to allocate aligned buffers to be mapped to
>> BARs.
>>
>> Add 'align' parameter to epc_features which can be used by platform
>> drivers to specifiy the BAR allocation alignment requirements and use
>> this while invoking pci_epf_alloc_space.
>>
>> This is mainly required for Synopsys Designware PCIe core which masks
>> the lower bits based on the BAR size (See "I/O and MEM Match Modes"
>> section in DesignWare Cores PCI Express Controller Databook version
>> 4.90a).
> 
> Hi Kishon,
> 
> isn't this a generic PCI BAR requirement rather than a Designware
> specific one ?
> 
> The patch is fine I just would like to understand the statement above.

The address that is allocated using pci_epf_alloc_space would be directly
written to the target address of the Inbound address translation unit.
Designware IP has a configuration parameter (CX_ATU_MIN_REGION_SIZE [1]) which
has 64KB as default value and the lower 16 bits of the Base, Limit and Target
registers of the Inbound ATU are fixed to zero.  So if the programmed memory
address is not aligned to 64 KB boundary there will be memory corruption.

PCI spec has restrictions on the minimum memory address range requested by BAR
as 128 Bytes and the size to be a power of 2, however it doesn't have any
restriction on the local endpoint memory addresses that are mapped to BAR. That
is an endpoint controller restriction.

[1] -> http://www.ti.com/lit/ug/spruid7c/spruid7c.pdf

Thanks
Kishon

> Thanks,
> Lorenzo
> 
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> ---
>>  drivers/pci/endpoint/functions/pci-epf-test.c |  5 +++--
>>  drivers/pci/endpoint/pci-epf-core.c           | 10 ++++++++--
>>  include/linux/pci-epc.h                       |  2 ++
>>  include/linux/pci-epf.h                       |  3 ++-
>>  4 files changed, 15 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
>> index d0b91da49bf4..c0786ca74312 100644
>> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
>> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
>> @@ -438,7 +438,7 @@ static int pci_epf_test_alloc_space(struct pci_epf *epf)
>>  	epc_features = epf_test->epc_features;
>>  
>>  	base = pci_epf_alloc_space(epf, sizeof(struct pci_epf_test_reg),
>> -				   test_reg_bar);
>> +				   test_reg_bar, epc_features->align);
>>  	if (!base) {
>>  		dev_err(dev, "Failed to allocated register space\n");
>>  		return -ENOMEM;
>> @@ -453,7 +453,8 @@ static int pci_epf_test_alloc_space(struct pci_epf *epf)
>>  		if (!!(epc_features->reserved_bar & (1 << bar)))
>>  			continue;
>>  
>> -		base = pci_epf_alloc_space(epf, bar_size[bar], bar);
>> +		base = pci_epf_alloc_space(epf, bar_size[bar], bar,
>> +					   epc_features->align);
>>  		if (!base)
>>  			dev_err(dev, "Failed to allocate space for BAR%d\n",
>>  				bar);
>> diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
>> index 8bfdcd291196..fb1306de8f40 100644
>> --- a/drivers/pci/endpoint/pci-epf-core.c
>> +++ b/drivers/pci/endpoint/pci-epf-core.c
>> @@ -109,10 +109,12 @@ EXPORT_SYMBOL_GPL(pci_epf_free_space);
>>   * pci_epf_alloc_space() - allocate memory for the PCI EPF register space
>>   * @size: the size of the memory that has to be allocated
>>   * @bar: the BAR number corresponding to the allocated register space
>> + * @align: alignment size for the allocation region
>>   *
>>   * Invoke to allocate memory for the PCI EPF register space.
>>   */
>> -void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar)
>> +void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar,
>> +			  size_t align)
>>  {
>>  	void *space;
>>  	struct device *dev = epf->epc->dev.parent;
>> @@ -120,7 +122,11 @@ void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar)
>>  
>>  	if (size < 128)
>>  		size = 128;
>> -	size = roundup_pow_of_two(size);
>> +
>> +	if (align)
>> +		size = ALIGN(size, align);
>> +	else
>> +		size = roundup_pow_of_two(size);
>>  
>>  	space = dma_alloc_coherent(dev, size, &phys_addr, GFP_KERNEL);
>>  	if (!space) {
>> diff --git a/include/linux/pci-epc.h b/include/linux/pci-epc.h
>> index c3ffa3917f88..f641badc2c61 100644
>> --- a/include/linux/pci-epc.h
>> +++ b/include/linux/pci-epc.h
>> @@ -109,6 +109,7 @@ struct pci_epc {
>>   * @reserved_bar: bitmap to indicate reserved BAR unavailable to function driver
>>   * @bar_fixed_64bit: bitmap to indicate fixed 64bit BARs
>>   * @bar_fixed_size: Array specifying the size supported by each BAR
>> + * @align: alignment size required for BAR buffer allocation
>>   */
>>  struct pci_epc_features {
>>  	unsigned int	linkup_notifier : 1;
>> @@ -117,6 +118,7 @@ struct pci_epc_features {
>>  	u8	reserved_bar;
>>  	u8	bar_fixed_64bit;
>>  	u64	bar_fixed_size[BAR_5 + 1];
>> +	size_t	align;
>>  };
>>  
>>  #define to_pci_epc(device) container_of((device), struct pci_epc, dev)
>> diff --git a/include/linux/pci-epf.h b/include/linux/pci-epf.h
>> index ec02f58758c8..2d6f07556682 100644
>> --- a/include/linux/pci-epf.h
>> +++ b/include/linux/pci-epf.h
>> @@ -149,7 +149,8 @@ void pci_epf_destroy(struct pci_epf *epf);
>>  int __pci_epf_register_driver(struct pci_epf_driver *driver,
>>  			      struct module *owner);
>>  void pci_epf_unregister_driver(struct pci_epf_driver *driver);
>> -void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar);
>> +void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar,
>> +			  size_t align);
>>  void pci_epf_free_space(struct pci_epf *epf, void *addr, enum pci_barno bar);
>>  int pci_epf_bind(struct pci_epf *epf);
>>  void pci_epf_unbind(struct pci_epf *epf);
>> -- 
>> 2.17.1
>>

Re: [PATCH v3 18/26] PCI: endpoint: Add support to allocate aligned buffers to be mapped in BARs

[Lorenzo Pieralisi]

On Fri, Apr 12, 2019 at 04:07:49PM +0530, Kishon Vijay Abraham I wrote:
> Hi Lorenzo,
> 
> On 11/04/19 9:02 PM, Lorenzo Pieralisi wrote:
> > On Mon, Mar 25, 2019 at 03:09:39PM +0530, Kishon Vijay Abraham I wrote:
> >> Modify pci_epf_alloc_space API to take alignment size as argument in
> >> order to argument in order to allocate aligned buffers to be mapped to
> >> BARs.
> >>
> >> Add 'align' parameter to epc_features which can be used by platform
> >> drivers to specifiy the BAR allocation alignment requirements and use
> >> this while invoking pci_epf_alloc_space.
> >>
> >> This is mainly required for Synopsys Designware PCIe core which masks
> >> the lower bits based on the BAR size (See "I/O and MEM Match Modes"
> >> section in DesignWare Cores PCI Express Controller Databook version
> >> 4.90a).
> > 
> > Hi Kishon,
> > 
> > isn't this a generic PCI BAR requirement rather than a Designware
> > specific one ?
> > 
> > The patch is fine I just would like to understand the statement above.
> 
> The address that is allocated using pci_epf_alloc_space would be directly
> written to the target address of the Inbound address translation unit.
> Designware IP has a configuration parameter (CX_ATU_MIN_REGION_SIZE [1]) which
> has 64KB as default value and the lower 16 bits of the Base, Limit and Target
> registers of the Inbound ATU are fixed to zero.  So if the programmed memory
> address is not aligned to 64 KB boundary there will be memory corruption.
> 
> PCI spec has restrictions on the minimum memory address range requested by BAR
> as 128 Bytes and the size to be a power of 2, however it doesn't have any
> restriction on the local endpoint memory addresses that are mapped to BAR. That
> is an endpoint controller restriction.
> 
> [1] -> http://www.ti.com/lit/ug/spruid7c/spruid7c.pdf

That's much clearer, it should go into the commit log, I will try
to update it myself and notify you when I push it out.

Lorenzo

> Thanks
> Kishon
> 
> > Thanks,
> > Lorenzo
> > 
> >> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> >> ---
> >>  drivers/pci/endpoint/functions/pci-epf-test.c |  5 +++--
> >>  drivers/pci/endpoint/pci-epf-core.c           | 10 ++++++++--
> >>  include/linux/pci-epc.h                       |  2 ++
> >>  include/linux/pci-epf.h                       |  3 ++-
> >>  4 files changed, 15 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> >> index d0b91da49bf4..c0786ca74312 100644
> >> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> >> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> >> @@ -438,7 +438,7 @@ static int pci_epf_test_alloc_space(struct pci_epf *epf)
> >>  	epc_features = epf_test->epc_features;
> >>  
> >>  	base = pci_epf_alloc_space(epf, sizeof(struct pci_epf_test_reg),
> >> -				   test_reg_bar);
> >> +				   test_reg_bar, epc_features->align);
> >>  	if (!base) {
> >>  		dev_err(dev, "Failed to allocated register space\n");
> >>  		return -ENOMEM;
> >> @@ -453,7 +453,8 @@ static int pci_epf_test_alloc_space(struct pci_epf *epf)
> >>  		if (!!(epc_features->reserved_bar & (1 << bar)))
> >>  			continue;
> >>  
> >> -		base = pci_epf_alloc_space(epf, bar_size[bar], bar);
> >> +		base = pci_epf_alloc_space(epf, bar_size[bar], bar,
> >> +					   epc_features->align);
> >>  		if (!base)
> >>  			dev_err(dev, "Failed to allocate space for BAR%d\n",
> >>  				bar);
> >> diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
> >> index 8bfdcd291196..fb1306de8f40 100644
> >> --- a/drivers/pci/endpoint/pci-epf-core.c
> >> +++ b/drivers/pci/endpoint/pci-epf-core.c
> >> @@ -109,10 +109,12 @@ EXPORT_SYMBOL_GPL(pci_epf_free_space);
> >>   * pci_epf_alloc_space() - allocate memory for the PCI EPF register space
> >>   * @size: the size of the memory that has to be allocated
> >>   * @bar: the BAR number corresponding to the allocated register space
> >> + * @align: alignment size for the allocation region
> >>   *
> >>   * Invoke to allocate memory for the PCI EPF register space.
> >>   */
> >> -void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar)
> >> +void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar,
> >> +			  size_t align)
> >>  {
> >>  	void *space;
> >>  	struct device *dev = epf->epc->dev.parent;
> >> @@ -120,7 +122,11 @@ void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar)
> >>  
> >>  	if (size < 128)
> >>  		size = 128;
> >> -	size = roundup_pow_of_two(size);
> >> +
> >> +	if (align)
> >> +		size = ALIGN(size, align);
> >> +	else
> >> +		size = roundup_pow_of_two(size);
> >>  
> >>  	space = dma_alloc_coherent(dev, size, &phys_addr, GFP_KERNEL);
> >>  	if (!space) {
> >> diff --git a/include/linux/pci-epc.h b/include/linux/pci-epc.h
> >> index c3ffa3917f88..f641badc2c61 100644
> >> --- a/include/linux/pci-epc.h
> >> +++ b/include/linux/pci-epc.h
> >> @@ -109,6 +109,7 @@ struct pci_epc {
> >>   * @reserved_bar: bitmap to indicate reserved BAR unavailable to function driver
> >>   * @bar_fixed_64bit: bitmap to indicate fixed 64bit BARs
> >>   * @bar_fixed_size: Array specifying the size supported by each BAR
> >> + * @align: alignment size required for BAR buffer allocation
> >>   */
> >>  struct pci_epc_features {
> >>  	unsigned int	linkup_notifier : 1;
> >> @@ -117,6 +118,7 @@ struct pci_epc_features {
> >>  	u8	reserved_bar;
> >>  	u8	bar_fixed_64bit;
> >>  	u64	bar_fixed_size[BAR_5 + 1];
> >> +	size_t	align;
> >>  };
> >>  
> >>  #define to_pci_epc(device) container_of((device), struct pci_epc, dev)
> >> diff --git a/include/linux/pci-epf.h b/include/linux/pci-epf.h
> >> index ec02f58758c8..2d6f07556682 100644
> >> --- a/include/linux/pci-epf.h
> >> +++ b/include/linux/pci-epf.h
> >> @@ -149,7 +149,8 @@ void pci_epf_destroy(struct pci_epf *epf);
> >>  int __pci_epf_register_driver(struct pci_epf_driver *driver,
> >>  			      struct module *owner);
> >>  void pci_epf_unregister_driver(struct pci_epf_driver *driver);
> >> -void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar);
> >> +void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar,
> >> +			  size_t align);
> >>  void pci_epf_free_space(struct pci_epf *epf, void *addr, enum pci_barno bar);
> >>  int pci_epf_bind(struct pci_epf *epf);
> >>  void pci_epf_unbind(struct pci_epf *epf);
> >> -- 
> >> 2.17.1
> >>

[PATCH v3 19/26] PCI: dwc: Add const qualifier to struct dw_pcie_ep_ops

[Kishon Vijay Abraham I]

Add const qualifier to struct dw_pcie_ep_ops member of
struct dw_pcie_ep.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/pci/controller/dwc/pci-dra7xx.c           | 2 +-
 drivers/pci/controller/dwc/pci-layerscape-ep.c    | 2 +-
 drivers/pci/controller/dwc/pcie-artpec6.c         | 2 +-
 drivers/pci/controller/dwc/pcie-designware-plat.c | 2 +-
 drivers/pci/controller/dwc/pcie-designware.h      | 2 +-
 5 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-dra7xx.c b/drivers/pci/controller/dwc/pci-dra7xx.c
index ae84a69ae63a..b287dbf6914c 100644
--- a/drivers/pci/controller/dwc/pci-dra7xx.c
+++ b/drivers/pci/controller/dwc/pci-dra7xx.c
@@ -406,7 +406,7 @@ dra7xx_pcie_get_features(struct dw_pcie_ep *ep)
 	return &dra7xx_pcie_epc_features;
 }
 
-static struct dw_pcie_ep_ops pcie_ep_ops = {
+static const struct dw_pcie_ep_ops pcie_ep_ops = {
 	.ep_init = dra7xx_pcie_ep_init,
 	.raise_irq = dra7xx_pcie_raise_irq,
 	.get_features = dra7xx_pcie_get_features,
diff --git a/drivers/pci/controller/dwc/pci-layerscape-ep.c b/drivers/pci/controller/dwc/pci-layerscape-ep.c
index a42c9c3ae1cc..be61d96cc95e 100644
--- a/drivers/pci/controller/dwc/pci-layerscape-ep.c
+++ b/drivers/pci/controller/dwc/pci-layerscape-ep.c
@@ -79,7 +79,7 @@ static int ls_pcie_ep_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
 	}
 }
 
-static struct dw_pcie_ep_ops pcie_ep_ops = {
+static const struct dw_pcie_ep_ops pcie_ep_ops = {
 	.ep_init = ls_pcie_ep_init,
 	.raise_irq = ls_pcie_ep_raise_irq,
 	.get_features = ls_pcie_ep_get_features,
diff --git a/drivers/pci/controller/dwc/pcie-artpec6.c b/drivers/pci/controller/dwc/pcie-artpec6.c
index dba83abfe764..d00252bd8fae 100644
--- a/drivers/pci/controller/dwc/pcie-artpec6.c
+++ b/drivers/pci/controller/dwc/pcie-artpec6.c
@@ -444,7 +444,7 @@ static int artpec6_pcie_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
 	return 0;
 }
 
-static struct dw_pcie_ep_ops pcie_ep_ops = {
+static const struct dw_pcie_ep_ops pcie_ep_ops = {
 	.ep_init = artpec6_pcie_ep_init,
 	.raise_irq = artpec6_pcie_raise_irq,
 };
diff --git a/drivers/pci/controller/dwc/pcie-designware-plat.c b/drivers/pci/controller/dwc/pcie-designware-plat.c
index 932dbd0b34b6..b58fdcbc664b 100644
--- a/drivers/pci/controller/dwc/pcie-designware-plat.c
+++ b/drivers/pci/controller/dwc/pcie-designware-plat.c
@@ -106,7 +106,7 @@ dw_plat_pcie_get_features(struct dw_pcie_ep *ep)
 	return &dw_plat_pcie_epc_features;
 }
 
-static struct dw_pcie_ep_ops pcie_ep_ops = {
+static const struct dw_pcie_ep_ops pcie_ep_ops = {
 	.ep_init = dw_plat_pcie_ep_init,
 	.raise_irq = dw_plat_pcie_ep_raise_irq,
 	.get_features = dw_plat_pcie_get_features,
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 90a5b1215344..e36941ff7cf6 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -196,7 +196,7 @@ struct dw_pcie_ep_ops {
 
 struct dw_pcie_ep {
 	struct pci_epc		*epc;
-	struct dw_pcie_ep_ops	*ops;
+	const struct dw_pcie_ep_ops *ops;
 	phys_addr_t		phys_base;
 	size_t			addr_size;
 	size_t			page_size;
-- 
2.17.1

[PATCH v3 20/26] PCI: dwc: Fix dw_pcie_ep_find_capability to return correct capability offset

[Kishon Vijay Abraham I]

commit beb4641a787df79a ("PCI: dwc: Add MSI-X callbacks handler") while
adding MSI-X callback handler, introduced dw_pcie_ep_find_capability and
__dw_pcie_ep_find_next_cap for finding the MSI and MSIX capability.

However if MSI or MSIX capability is the last capability (i.e there are
no additional items in the capabilities list and the Next Capability
Pointer is set to '0'), __dw_pcie_ep_find_next_cap will return '0'
even though MSI or MSIX capability may be present. This is because of
incorrect ordering of "next_cap_ptr" check. Fix it here.

Fixes: beb4641a787df79a142 ("PCI: dwc: Add MSI-X callbacks handler")
Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/pci/controller/dwc/pcie-designware-ep.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
index dc6a4bbd3ace..74477ad7467f 100644
--- a/drivers/pci/controller/dwc/pcie-designware-ep.c
+++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
@@ -46,16 +46,19 @@ static u8 __dw_pcie_ep_find_next_cap(struct dw_pcie *pci, u8 cap_ptr,
 	u8 cap_id, next_cap_ptr;
 	u16 reg;
 
+	if (!cap_ptr)
+		return 0;
+
 	reg = dw_pcie_readw_dbi(pci, cap_ptr);
-	next_cap_ptr = (reg & 0xff00) >> 8;
 	cap_id = (reg & 0x00ff);
 
-	if (!next_cap_ptr || cap_id > PCI_CAP_ID_MAX)
+	if (cap_id > PCI_CAP_ID_MAX)
 		return 0;
 
 	if (cap_id == cap)
 		return cap_ptr;
 
+	next_cap_ptr = (reg & 0xff00) >> 8;
 	return __dw_pcie_ep_find_next_cap(pci, next_cap_ptr, cap);
 }
 
@@ -67,9 +70,6 @@ static u8 dw_pcie_ep_find_capability(struct dw_pcie *pci, u8 cap)
 	reg = dw_pcie_readw_dbi(pci, PCI_CAPABILITY_LIST);
 	next_cap_ptr = (reg & 0x00ff);
 
-	if (!next_cap_ptr)
-		return 0;
-
 	return __dw_pcie_ep_find_next_cap(pci, next_cap_ptr, cap);
 }
 
-- 
2.17.1

Re: [PATCH v3 20/26] PCI: dwc: Fix dw_pcie_ep_find_capability to return correct capability offset

[Bjorn Helgaas]

On Mon, Mar 25, 2019 at 03:09:41PM +0530, Kishon Vijay Abraham I wrote:
> commit beb4641a787df79a ("PCI: dwc: Add MSI-X callbacks handler") while
> adding MSI-X callback handler, introduced dw_pcie_ep_find_capability and
> __dw_pcie_ep_find_next_cap for finding the MSI and MSIX capability.
> 
> However if MSI or MSIX capability is the last capability (i.e there are
> no additional items in the capabilities list and the Next Capability
> Pointer is set to '0'), __dw_pcie_ep_find_next_cap will return '0'
> even though MSI or MSIX capability may be present. This is because of
> incorrect ordering of "next_cap_ptr" check. Fix it here.

Nice fix.  It looks like dw_pcie_ep_find_capability() is currently
only used for MSI and MSI-X, but IIUC, it *could* be used for any
capability, so this commit message seems a little too specific.

It would be slightly less confusing if beb4641a787d ("PCI: dwc: Add
MSI-X callbacks handler") had been split into a commit that added the
general-purpose dw_pcie_ep_find_capability() and a commit that used it
for MSI and MSI-X, but obviously we can't change history.

> Fixes: beb4641a787df79a142 ("PCI: dwc: Add MSI-X callbacks handler")

These commit references are longer than necessary (and different -
19-char SHA1 here and 16 chars above).  I typically use 12 chars via

  gsr is aliased to `git --no-pager show -s --abbrev-commit --abbrev=12 --pretty=format:"%h (\"%s\")%n"'


> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  drivers/pci/controller/dwc/pcie-designware-ep.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> index dc6a4bbd3ace..74477ad7467f 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> @@ -46,16 +46,19 @@ static u8 __dw_pcie_ep_find_next_cap(struct dw_pcie *pci, u8 cap_ptr,
>  	u8 cap_id, next_cap_ptr;
>  	u16 reg;
>  
> +	if (!cap_ptr)
> +		return 0;
> +
>  	reg = dw_pcie_readw_dbi(pci, cap_ptr);
> -	next_cap_ptr = (reg & 0xff00) >> 8;
>  	cap_id = (reg & 0x00ff);
>  
> -	if (!next_cap_ptr || cap_id > PCI_CAP_ID_MAX)
> +	if (cap_id > PCI_CAP_ID_MAX)
>  		return 0;
>  
>  	if (cap_id == cap)
>  		return cap_ptr;
>  
> +	next_cap_ptr = (reg & 0xff00) >> 8;
>  	return __dw_pcie_ep_find_next_cap(pci, next_cap_ptr, cap);
>  }
>  
> @@ -67,9 +70,6 @@ static u8 dw_pcie_ep_find_capability(struct dw_pcie *pci, u8 cap)
>  	reg = dw_pcie_readw_dbi(pci, PCI_CAPABILITY_LIST);
>  	next_cap_ptr = (reg & 0x00ff);
>  
> -	if (!next_cap_ptr)
> -		return 0;
> -
>  	return __dw_pcie_ep_find_next_cap(pci, next_cap_ptr, cap);
>  }
>  
> -- 
> 2.17.1
> 

Re: [PATCH v3 20/26] PCI: dwc: Fix dw_pcie_ep_find_capability to return correct capability offset

[Lorenzo Pieralisi]

On Sat, Apr 13, 2019 at 10:44:37AM -0500, Bjorn Helgaas wrote:
> On Mon, Mar 25, 2019 at 03:09:41PM +0530, Kishon Vijay Abraham I wrote:
> > commit beb4641a787df79a ("PCI: dwc: Add MSI-X callbacks handler") while
> > adding MSI-X callback handler, introduced dw_pcie_ep_find_capability and
> > __dw_pcie_ep_find_next_cap for finding the MSI and MSIX capability.
> > 
> > However if MSI or MSIX capability is the last capability (i.e there are
> > no additional items in the capabilities list and the Next Capability
> > Pointer is set to '0'), __dw_pcie_ep_find_next_cap will return '0'
> > even though MSI or MSIX capability may be present. This is because of
> > incorrect ordering of "next_cap_ptr" check. Fix it here.
> 
> Nice fix.  It looks like dw_pcie_ep_find_capability() is currently
> only used for MSI and MSI-X, but IIUC, it *could* be used for any
> capability, so this commit message seems a little too specific.
> 
> It would be slightly less confusing if beb4641a787d ("PCI: dwc: Add
> MSI-X callbacks handler") had been split into a commit that added the
> general-purpose dw_pcie_ep_find_capability() and a commit that used it
> for MSI and MSI-X, but obviously we can't change history.
> 
> > Fixes: beb4641a787df79a142 ("PCI: dwc: Add MSI-X callbacks handler")
> 
> These commit references are longer than necessary (and different -
> 19-char SHA1 here and 16 chars above).  I typically use 12 chars via
> 
>   gsr is aliased to `git --no-pager show -s --abbrev-commit --abbrev=12 --pretty=format:"%h (\"%s\")%n"'

Fixed while queueing it.

Thanks,
Lorenzo

> > > Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> > ---
> >  drivers/pci/controller/dwc/pcie-designware-ep.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > index dc6a4bbd3ace..74477ad7467f 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > @@ -46,16 +46,19 @@ static u8 __dw_pcie_ep_find_next_cap(struct dw_pcie *pci, u8 cap_ptr,
> >  	u8 cap_id, next_cap_ptr;
> >  	u16 reg;
> >  
> > +	if (!cap_ptr)
> > +		return 0;
> > +
> >  	reg = dw_pcie_readw_dbi(pci, cap_ptr);
> > -	next_cap_ptr = (reg & 0xff00) >> 8;
> >  	cap_id = (reg & 0x00ff);
> >  
> > -	if (!next_cap_ptr || cap_id > PCI_CAP_ID_MAX)
> > +	if (cap_id > PCI_CAP_ID_MAX)
> >  		return 0;
> >  
> >  	if (cap_id == cap)
> >  		return cap_ptr;
> >  
> > +	next_cap_ptr = (reg & 0xff00) >> 8;
> >  	return __dw_pcie_ep_find_next_cap(pci, next_cap_ptr, cap);
> >  }
> >  
> > @@ -67,9 +70,6 @@ static u8 dw_pcie_ep_find_capability(struct dw_pcie *pci, u8 cap)
> >  	reg = dw_pcie_readw_dbi(pci, PCI_CAPABILITY_LIST);
> >  	next_cap_ptr = (reg & 0x00ff);
> >  
> > -	if (!next_cap_ptr)
> > -		return 0;
> > -
> >  	return __dw_pcie_ep_find_next_cap(pci, next_cap_ptr, cap);
> >  }
> >  
> > -- 
> > 2.17.1
> > 

[PATCH v3 21/26] PCI: dwc: Add callbacks for accessing dbi2 address space

[Kishon Vijay Abraham I]

Certain platforms like TI's AM654 doesn't have separate address space for
dbi2 instead they are accessed using the same address space as dbi
with some configuration bit set. In order to support such platforms,
add callbacks for accessing dbi2 address space.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/pci/controller/dwc/pcie-designware.c | 31 ++++++++++++++++++++
 drivers/pci/controller/dwc/pcie-designware.h | 12 ++++++--
 2 files changed, 41 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index 4e2f7946da89..d7cc1a0c1de6 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -89,6 +89,37 @@ void __dw_pcie_write_dbi(struct dw_pcie *pci, void __iomem *base, u32 reg,
 		dev_err(pci->dev, "Write DBI address failed\n");
 }
 
+u32 __dw_pcie_read_dbi2(struct dw_pcie *pci, void __iomem *base, u32 reg,
+			size_t size)
+{
+	int ret;
+	u32 val;
+
+	if (pci->ops->read_dbi2)
+		return pci->ops->read_dbi2(pci, base, reg, size);
+
+	ret = dw_pcie_read(base + reg, size, &val);
+	if (ret)
+		dev_err(pci->dev, "read DBI address failed\n");
+
+	return val;
+}
+
+void __dw_pcie_write_dbi2(struct dw_pcie *pci, void __iomem *base, u32 reg,
+			  size_t size, u32 val)
+{
+	int ret;
+
+	if (pci->ops->write_dbi2) {
+		pci->ops->write_dbi2(pci, base, reg, size, val);
+		return;
+	}
+
+	ret = dw_pcie_write(base + reg, size, val);
+	if (ret)
+		dev_err(pci->dev, "write DBI address failed\n");
+}
+
 static u32 dw_pcie_readl_ob_unroll(struct dw_pcie *pci, u32 index, u32 reg)
 {
 	u32 offset = PCIE_GET_ATU_OUTB_UNR_REG_OFFSET(index);
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index e36941ff7cf6..7abca9eb00bf 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -218,6 +218,10 @@ struct dw_pcie_ops {
 			    size_t size);
 	void	(*write_dbi)(struct dw_pcie *pcie, void __iomem *base, u32 reg,
 			     size_t size, u32 val);
+	u32     (*read_dbi2)(struct dw_pcie *pcie, void __iomem *base, u32 reg,
+			     size_t size);
+	void    (*write_dbi2)(struct dw_pcie *pcie, void __iomem *base, u32 reg,
+			      size_t size, u32 val);
 	int	(*link_up)(struct dw_pcie *pcie);
 	int	(*start_link)(struct dw_pcie *pcie);
 	void	(*stop_link)(struct dw_pcie *pcie);
@@ -249,6 +253,10 @@ u32 __dw_pcie_read_dbi(struct dw_pcie *pci, void __iomem *base, u32 reg,
 		       size_t size);
 void __dw_pcie_write_dbi(struct dw_pcie *pci, void __iomem *base, u32 reg,
 			 size_t size, u32 val);
+u32 __dw_pcie_read_dbi2(struct dw_pcie *pci, void __iomem *base, u32 reg,
+			size_t size);
+void __dw_pcie_write_dbi2(struct dw_pcie *pci, void __iomem *base, u32 reg,
+			  size_t size, u32 val);
 int dw_pcie_link_up(struct dw_pcie *pci);
 int dw_pcie_wait_for_link(struct dw_pcie *pci);
 void dw_pcie_prog_outbound_atu(struct dw_pcie *pci, int index,
@@ -292,12 +300,12 @@ static inline u8 dw_pcie_readb_dbi(struct dw_pcie *pci, u32 reg)
 
 static inline void dw_pcie_writel_dbi2(struct dw_pcie *pci, u32 reg, u32 val)
 {
-	__dw_pcie_write_dbi(pci, pci->dbi_base2, reg, 0x4, val);
+	__dw_pcie_write_dbi2(pci, pci->dbi_base2, reg, 0x4, val);
 }
 
 static inline u32 dw_pcie_readl_dbi2(struct dw_pcie *pci, u32 reg)
 {
-	return __dw_pcie_read_dbi(pci, pci->dbi_base2, reg, 0x4);
+	return __dw_pcie_read_dbi2(pci, pci->dbi_base2, reg, 0x4);
 }
 
 static inline void dw_pcie_writel_atu(struct dw_pcie *pci, u32 reg, u32 val)
-- 
2.17.1

[PATCH v3 22/26] PCI: keystone: Add support for PCIe EP in AM654x Platforms

[Kishon Vijay Abraham I]

Add PCIe EP support for AM654x Platforms in pci-keystone.c

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/pci/controller/dwc/Kconfig        |  23 +-
 drivers/pci/controller/dwc/pci-keystone.c | 242 +++++++++++++++++++---
 2 files changed, 236 insertions(+), 29 deletions(-)

diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
index d1d00833e0b3..b450ad2823a5 100644
--- a/drivers/pci/controller/dwc/Kconfig
+++ b/drivers/pci/controller/dwc/Kconfig
@@ -105,14 +105,33 @@ config PCIE_SPEAR13XX
 config PCI_KEYSTONE
 	bool "TI Keystone PCIe controller"
 	depends on ARCH_KEYSTONE || ARCH_K3 || ((ARM || ARM64) && COMPILE_TEST)
-	depends on PCI_MSI_IRQ_DOMAIN
-	select PCIE_DW_HOST
 	help
 	  Say Y here if you want to enable PCI controller support on Keystone
 	  SoCs. The PCI controller on Keystone is based on DesignWare hardware
 	  and therefore the driver re-uses the DesignWare core functions to
 	  implement the driver.
 
+if PCI_KEYSTONE
+
+config PCI_KEYSTONE_HOST
+	bool "PCI Keystone Host Mode"
+	depends on PCI_MSI_IRQ_DOMAIN
+	select PCIE_DW_HOST
+	default y
+	help
+	 Enables support for the PCIe controller in the Keystone SoC to work in
+	 host mode.
+
+config PCI_KEYSTONE_EP
+	bool "PCI Keystone Endpoint Mode"
+	depends on PCI_ENDPOINT
+	select PCIE_DW_EP
+	help
+	 Enables support for the PCIe controller in the Keystone SoC to work in
+	 endpoint mode.
+
+endif
+
 config PCI_LAYERSCAPE
 	bool "Freescale Layerscape PCIe controller"
 	depends on OF && (ARM || ARCH_LAYERSCAPE || COMPILE_TEST)
diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
index 312fd0c49bbb..459485b0f65d 100644
--- a/drivers/pci/controller/dwc/pci-keystone.c
+++ b/drivers/pci/controller/dwc/pci-keystone.c
@@ -52,6 +52,12 @@
 #define OB_ENABLEN			BIT(0)
 #define OB_WIN_SIZE			8	/* 8MB */
 
+#define PCIE_LEGACY_IRQ_ENABLE_SET(n)	(0x188 + (0x10 * ((n) - 1)))
+#define PCIE_LEGACY_IRQ_ENABLE_CLR(n)	(0x18c + (0x10 * ((n) - 1)))
+#define PCIE_EP_IRQ_SET			0x64
+#define PCIE_EP_IRQ_CLR			0x68
+#define INT_ENABLE			BIT(0)
+
 /* IRQ register defines */
 #define IRQ_EOI				0x050
 
@@ -95,11 +101,16 @@
 #define KS_PCIE_SYSCLOCKOUTEN		BIT(0)
 
 #define AM654_PCIE_DEV_TYPE_MASK	0x3
+#define AM654_WIN_SIZE			SZ_64K
+
+#define APP_ADDR_SPACE_0		(16 * SZ_1K)
 
 #define to_keystone_pcie(x)		dev_get_drvdata((x)->dev)
 
 struct ks_pcie_of_data {
+	enum dw_pcie_device_mode mode;
 	const struct dw_pcie_host_ops *host_ops;
+	const struct dw_pcie_ep_ops *ep_ops;
 	unsigned int version;
 };
 
@@ -877,12 +888,139 @@ static int __init ks_pcie_add_pcie_port(struct keystone_pcie *ks_pcie,
 	return 0;
 }
 
+static u32 ks_pcie_am654_read_dbi2(struct dw_pcie *pci, void __iomem *base,
+				   u32 reg, size_t size)
+{
+	struct keystone_pcie *ks_pcie = to_keystone_pcie(pci);
+	u32 val;
+
+	ks_pcie_set_dbi_mode(ks_pcie);
+	dw_pcie_read(base + reg, size, &val);
+	ks_pcie_clear_dbi_mode(ks_pcie);
+	return val;
+}
+
+static void ks_pcie_am654_write_dbi2(struct dw_pcie *pci, void __iomem *base,
+				     u32 reg, size_t size, u32 val)
+{
+	struct keystone_pcie *ks_pcie = to_keystone_pcie(pci);
+
+	ks_pcie_set_dbi_mode(ks_pcie);
+	dw_pcie_write(base + reg, size, val);
+	ks_pcie_clear_dbi_mode(ks_pcie);
+}
+
 static const struct dw_pcie_ops ks_pcie_dw_pcie_ops = {
 	.start_link = ks_pcie_start_link,
 	.stop_link = ks_pcie_stop_link,
 	.link_up = ks_pcie_link_up,
+	.read_dbi2 = ks_pcie_am654_read_dbi2,
+	.write_dbi2 = ks_pcie_am654_write_dbi2,
 };
 
+static void ks_pcie_am654_ep_init(struct dw_pcie_ep *ep)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
+	int flags;
+
+	ep->page_size = AM654_WIN_SIZE;
+	flags = PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32;
+	dw_pcie_writel_dbi2(pci, PCI_BASE_ADDRESS_0, APP_ADDR_SPACE_0 - 1);
+	dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_0, flags);
+}
+
+static void ks_pcie_am654_raise_legacy_irq(struct keystone_pcie *ks_pcie)
+{
+	struct dw_pcie *pci = ks_pcie->pci;
+	u8 int_pin;
+
+	int_pin = dw_pcie_readb_dbi(pci, PCI_INTERRUPT_PIN);
+	if (int_pin == 0 || int_pin > 4)
+		return;
+
+	ks_pcie_app_writel(ks_pcie, PCIE_LEGACY_IRQ_ENABLE_SET(int_pin),
+			   INT_ENABLE);
+	ks_pcie_app_writel(ks_pcie, PCIE_EP_IRQ_SET, INT_ENABLE);
+	mdelay(1);
+	ks_pcie_app_writel(ks_pcie, PCIE_EP_IRQ_CLR, INT_ENABLE);
+	ks_pcie_app_writel(ks_pcie, PCIE_LEGACY_IRQ_ENABLE_CLR(int_pin),
+			   INT_ENABLE);
+}
+
+static int ks_pcie_am654_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
+				   enum pci_epc_irq_type type,
+				   u16 interrupt_num)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
+	struct keystone_pcie *ks_pcie = to_keystone_pcie(pci);
+
+	switch (type) {
+	case PCI_EPC_IRQ_LEGACY:
+		ks_pcie_am654_raise_legacy_irq(ks_pcie);
+		break;
+	case PCI_EPC_IRQ_MSI:
+		dw_pcie_ep_raise_msi_irq(ep, func_no, interrupt_num);
+		break;
+	default:
+		dev_err(pci->dev, "UNKNOWN IRQ type\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static const struct pci_epc_features ks_pcie_am654_epc_features = {
+	.linkup_notifier = false,
+	.msi_capable = true,
+	.msix_capable = false,
+	.reserved_bar = 1 << BAR_0 | 1 << BAR_1,
+	.bar_fixed_64bit = 1 << BAR_0,
+	.bar_fixed_size[2] = SZ_1M,
+	.bar_fixed_size[3] = SZ_64K,
+	.bar_fixed_size[4] = 256,
+	.bar_fixed_size[5] = SZ_1M,
+	.align = SZ_1M,
+};
+
+static const struct pci_epc_features*
+ks_pcie_am654_get_features(struct dw_pcie_ep *ep)
+{
+	return &ks_pcie_am654_epc_features;
+}
+
+static const struct dw_pcie_ep_ops ks_pcie_am654_ep_ops = {
+	.ep_init = ks_pcie_am654_ep_init,
+	.raise_irq = ks_pcie_am654_raise_irq,
+	.get_features = &ks_pcie_am654_get_features,
+};
+
+static int __init ks_pcie_add_pcie_ep(struct keystone_pcie *ks_pcie,
+				      struct platform_device *pdev)
+{
+	int ret;
+	struct dw_pcie_ep *ep;
+	struct resource *res;
+	struct device *dev = &pdev->dev;
+	struct dw_pcie *pci = ks_pcie->pci;
+
+	ep = &pci->ep;
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "addr_space");
+	if (!res)
+		return -EINVAL;
+
+	ep->phys_base = res->start;
+	ep->addr_size = resource_size(res);
+
+	ret = dw_pcie_ep_init(ep);
+	if (ret) {
+		dev_err(dev, "failed to initialize endpoint\n");
+		return ret;
+	}
+
+	return 0;
+}
+
 static void ks_pcie_disable_phy(struct keystone_pcie *ks_pcie)
 {
 	int num_lanes = ks_pcie->num_lanes;
@@ -950,7 +1088,8 @@ static int ks_pcie_set_mode(struct device *dev)
 	return 0;
 }
 
-static int ks_pcie_am654_set_mode(struct device *dev)
+static int ks_pcie_am654_set_mode(struct device *dev,
+				  enum dw_pcie_device_mode mode)
 {
 	struct device_node *np = dev->of_node;
 	struct regmap *syscon;
@@ -963,7 +1102,18 @@ static int ks_pcie_am654_set_mode(struct device *dev)
 		return 0;
 
 	mask = AM654_PCIE_DEV_TYPE_MASK;
-	val = RC;
+
+	switch (mode) {
+	case DW_PCIE_RC_TYPE:
+		val = RC;
+		break;
+	case DW_PCIE_EP_TYPE:
+		val = EP;
+		break;
+	default:
+		dev_err(dev, "INVALID device type %d\n", mode);
+		return -EINVAL;
+	}
 
 	ret = regmap_update_bits(syscon, 0, mask, val);
 	if (ret) {
@@ -1006,6 +1156,13 @@ static const struct ks_pcie_of_data ks_pcie_rc_of_data = {
 
 static const struct ks_pcie_of_data ks_pcie_am654_rc_of_data = {
 	.host_ops = &ks_pcie_am654_host_ops,
+	.mode = DW_PCIE_RC_TYPE,
+	.version = 0x490A,
+};
+
+static const struct ks_pcie_of_data ks_pcie_am654_ep_of_data = {
+	.ep_ops = &ks_pcie_am654_ep_ops,
+	.mode = DW_PCIE_EP_TYPE,
 	.version = 0x490A,
 };
 
@@ -1019,16 +1176,22 @@ static const struct of_device_id ks_pcie_of_match[] = {
 		.data = &ks_pcie_am654_rc_of_data,
 		.compatible = "ti,am654-pcie-rc",
 	},
+	{
+		.data = &ks_pcie_am654_ep_of_data,
+		.compatible = "ti,am654-pcie-ep",
+	},
 	{ },
 };
 
 static int __init ks_pcie_probe(struct platform_device *pdev)
 {
 	const struct dw_pcie_host_ops *host_ops;
+	const struct dw_pcie_ep_ops *ep_ops;
 	struct device *dev = &pdev->dev;
 	struct device_node *np = dev->of_node;
 	const struct ks_pcie_of_data *data;
 	const struct of_device_id *match;
+	enum dw_pcie_device_mode mode;
 	struct dw_pcie *pci;
 	struct keystone_pcie *ks_pcie;
 	struct device_link **link;
@@ -1053,6 +1216,8 @@ static int __init ks_pcie_probe(struct platform_device *pdev)
 
 	version = data->version;
 	host_ops = data->host_ops;
+	ep_ops = data->ep_ops;
+	mode = data->mode;
 
 	ks_pcie = devm_kzalloc(dev, sizeof(*ks_pcie), GFP_KERNEL);
 	if (!ks_pcie)
@@ -1078,16 +1243,11 @@ static int __init ks_pcie_probe(struct platform_device *pdev)
 		ks_pcie->is_am6 = true;
 
 	pci->dbi_base = base;
+	pci->dbi_base2 = base;
 	pci->dev = dev;
 	pci->ops = &ks_pcie_dw_pcie_ops;
 	pci->version = version;
 
-	ret = of_property_read_u32(np, "num-viewport", &num_viewport);
-	if (ret < 0) {
-		dev_err(dev, "unable to read *num-viewport* property\n");
-		return ret;
-	}
-
 	irq = platform_get_irq(pdev, 0);
 	if (irq < 0) {
 		dev_err(dev, "missing IRQ resource: %d\n", irq);
@@ -1136,7 +1296,6 @@ static int __init ks_pcie_probe(struct platform_device *pdev)
 	ks_pcie->pci = pci;
 	ks_pcie->link = link;
 	ks_pcie->num_lanes = num_lanes;
-	ks_pcie->num_viewport = num_viewport;
 	ks_pcie->phy = phy;
 
 	gpiod = devm_gpiod_get_optional(dev, "reset",
@@ -1172,7 +1331,7 @@ static int __init ks_pcie_probe(struct platform_device *pdev)
 
 		pci->atu_base = atu_base;
 
-		ret = ks_pcie_am654_set_mode(dev);
+		ret = ks_pcie_am654_set_mode(dev, mode);
 		if (ret < 0)
 			goto err_get_sync;
 	} else {
@@ -1181,29 +1340,58 @@ static int __init ks_pcie_probe(struct platform_device *pdev)
 			goto err_get_sync;
 	}
 
-	/*
-	 * "Power Sequencing and Reset Signal Timings" table in
-	 * PCI EXPRESS CARD ELECTROMECHANICAL SPECIFICATION, REV. 2.0
-	 * indicates PERST# should be deasserted after minimum of 100us
-	 * once REFCLK is stable. The REFCLK to the connector in RC
-	 * mode is selected while enabling the PHY. So deassert PERST#
-	 * after 100 us.
-	 */
-	if (gpiod) {
-		usleep_range(100, 200);
-		gpiod_set_value_cansleep(gpiod, 1);
-	}
-
 	link_speed = of_pci_get_max_link_speed(np);
 	if (link_speed < 0)
 		link_speed = 2;
 
 	ks_pcie_set_link_speed(pci, link_speed);
 
-	pci->pp.ops = host_ops;
-	ret = ks_pcie_add_pcie_port(ks_pcie, pdev);
-	if (ret < 0)
-		goto err_get_sync;
+	switch (mode) {
+	case DW_PCIE_RC_TYPE:
+		if (!IS_ENABLED(CONFIG_PCI_KEYSTONE_HOST)) {
+			ret = -ENODEV;
+			goto err_get_sync;
+		}
+
+		ret = of_property_read_u32(np, "num-viewport", &num_viewport);
+		if (ret < 0) {
+			dev_err(dev, "unable to read *num-viewport* property\n");
+			return ret;
+		}
+
+		/*
+		 * "Power Sequencing and Reset Signal Timings" table in
+		 * PCI EXPRESS CARD ELECTROMECHANICAL SPECIFICATION, REV. 2.0
+		 * indicates PERST# should be deasserted after minimum of 100us
+		 * once REFCLK is stable. The REFCLK to the connector in RC
+		 * mode is selected while enabling the PHY. So deassert PERST#
+		 * after 100 us.
+		 */
+		if (gpiod) {
+			usleep_range(100, 200);
+			gpiod_set_value_cansleep(gpiod, 1);
+		}
+
+		ks_pcie->num_viewport = num_viewport;
+		pci->pp.ops = host_ops;
+		ret = ks_pcie_add_pcie_port(ks_pcie, pdev);
+		if (ret < 0)
+			goto err_get_sync;
+		break;
+	case DW_PCIE_EP_TYPE:
+		if (!IS_ENABLED(CONFIG_PCI_KEYSTONE_EP)) {
+			ret = -ENODEV;
+			goto err_get_sync;
+		}
+
+		pci->ep.ops = ep_ops;
+		ret = ks_pcie_add_pcie_ep(ks_pcie, pdev);
+		if (ret < 0)
+			goto err_get_sync;
+		break;
+	default:
+		dev_err(dev, "INVALID device type %d\n", mode);
+	}
 
 	ks_pcie_enable_error_irq(ks_pcie);
 
-- 
2.17.1

Re: [PATCH v3 22/26] PCI: keystone: Add support for PCIe EP in AM654x Platforms

[Lorenzo Pieralisi]

On Mon, Mar 25, 2019 at 03:09:43PM +0530, Kishon Vijay Abraham I wrote:
> Add PCIe EP support for AM654x Platforms in pci-keystone.c
> 
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  drivers/pci/controller/dwc/Kconfig        |  23 +-
>  drivers/pci/controller/dwc/pci-keystone.c | 242 +++++++++++++++++++---
>  2 files changed, 236 insertions(+), 29 deletions(-)

Hi Kishon,

WARNING: DT compatible string "ti,am654-pcie-ep" appears un-documented -- check /work/sandbox/pci/Documentation/devicetree/bindings/
#301: FILE: drivers/pci/controller/dwc/pci-keystone.c:1179:
+		.compatible = "ti,am654-pcie-ep",

I would like this warning to be fixed please on this patch.

Thanks,
Lorenzo

> diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
> index d1d00833e0b3..b450ad2823a5 100644
> --- a/drivers/pci/controller/dwc/Kconfig
> +++ b/drivers/pci/controller/dwc/Kconfig
> @@ -105,14 +105,33 @@ config PCIE_SPEAR13XX
>  config PCI_KEYSTONE
>  	bool "TI Keystone PCIe controller"
>  	depends on ARCH_KEYSTONE || ARCH_K3 || ((ARM || ARM64) && COMPILE_TEST)
> -	depends on PCI_MSI_IRQ_DOMAIN
> -	select PCIE_DW_HOST
>  	help
>  	  Say Y here if you want to enable PCI controller support on Keystone
>  	  SoCs. The PCI controller on Keystone is based on DesignWare hardware
>  	  and therefore the driver re-uses the DesignWare core functions to
>  	  implement the driver.
>  
> +if PCI_KEYSTONE
> +
> +config PCI_KEYSTONE_HOST
> +	bool "PCI Keystone Host Mode"
> +	depends on PCI_MSI_IRQ_DOMAIN
> +	select PCIE_DW_HOST
> +	default y
> +	help
> +	 Enables support for the PCIe controller in the Keystone SoC to work in
> +	 host mode.
> +
> +config PCI_KEYSTONE_EP
> +	bool "PCI Keystone Endpoint Mode"
> +	depends on PCI_ENDPOINT
> +	select PCIE_DW_EP
> +	help
> +	 Enables support for the PCIe controller in the Keystone SoC to work in
> +	 endpoint mode.
> +
> +endif
> +
>  config PCI_LAYERSCAPE
>  	bool "Freescale Layerscape PCIe controller"
>  	depends on OF && (ARM || ARCH_LAYERSCAPE || COMPILE_TEST)
> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
> index 312fd0c49bbb..459485b0f65d 100644
> --- a/drivers/pci/controller/dwc/pci-keystone.c
> +++ b/drivers/pci/controller/dwc/pci-keystone.c
> @@ -52,6 +52,12 @@
>  #define OB_ENABLEN			BIT(0)
>  #define OB_WIN_SIZE			8	/* 8MB */
>  
> +#define PCIE_LEGACY_IRQ_ENABLE_SET(n)	(0x188 + (0x10 * ((n) - 1)))
> +#define PCIE_LEGACY_IRQ_ENABLE_CLR(n)	(0x18c + (0x10 * ((n) - 1)))
> +#define PCIE_EP_IRQ_SET			0x64
> +#define PCIE_EP_IRQ_CLR			0x68
> +#define INT_ENABLE			BIT(0)
> +
>  /* IRQ register defines */
>  #define IRQ_EOI				0x050
>  
> @@ -95,11 +101,16 @@
>  #define KS_PCIE_SYSCLOCKOUTEN		BIT(0)
>  
>  #define AM654_PCIE_DEV_TYPE_MASK	0x3
> +#define AM654_WIN_SIZE			SZ_64K
> +
> +#define APP_ADDR_SPACE_0		(16 * SZ_1K)
>  
>  #define to_keystone_pcie(x)		dev_get_drvdata((x)->dev)
>  
>  struct ks_pcie_of_data {
> +	enum dw_pcie_device_mode mode;
>  	const struct dw_pcie_host_ops *host_ops;
> +	const struct dw_pcie_ep_ops *ep_ops;
>  	unsigned int version;
>  };
>  
> @@ -877,12 +888,139 @@ static int __init ks_pcie_add_pcie_port(struct keystone_pcie *ks_pcie,
>  	return 0;
>  }
>  
> +static u32 ks_pcie_am654_read_dbi2(struct dw_pcie *pci, void __iomem *base,
> +				   u32 reg, size_t size)
> +{
> +	struct keystone_pcie *ks_pcie = to_keystone_pcie(pci);
> +	u32 val;
> +
> +	ks_pcie_set_dbi_mode(ks_pcie);
> +	dw_pcie_read(base + reg, size, &val);
> +	ks_pcie_clear_dbi_mode(ks_pcie);
> +	return val;
> +}
> +
> +static void ks_pcie_am654_write_dbi2(struct dw_pcie *pci, void __iomem *base,
> +				     u32 reg, size_t size, u32 val)
> +{
> +	struct keystone_pcie *ks_pcie = to_keystone_pcie(pci);
> +
> +	ks_pcie_set_dbi_mode(ks_pcie);
> +	dw_pcie_write(base + reg, size, val);
> +	ks_pcie_clear_dbi_mode(ks_pcie);
> +}
> +
>  static const struct dw_pcie_ops ks_pcie_dw_pcie_ops = {
>  	.start_link = ks_pcie_start_link,
>  	.stop_link = ks_pcie_stop_link,
>  	.link_up = ks_pcie_link_up,
> +	.read_dbi2 = ks_pcie_am654_read_dbi2,
> +	.write_dbi2 = ks_pcie_am654_write_dbi2,
>  };
>  
> +static void ks_pcie_am654_ep_init(struct dw_pcie_ep *ep)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> +	int flags;
> +
> +	ep->page_size = AM654_WIN_SIZE;
> +	flags = PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32;
> +	dw_pcie_writel_dbi2(pci, PCI_BASE_ADDRESS_0, APP_ADDR_SPACE_0 - 1);
> +	dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_0, flags);
> +}
> +
> +static void ks_pcie_am654_raise_legacy_irq(struct keystone_pcie *ks_pcie)
> +{
> +	struct dw_pcie *pci = ks_pcie->pci;
> +	u8 int_pin;
> +
> +	int_pin = dw_pcie_readb_dbi(pci, PCI_INTERRUPT_PIN);
> +	if (int_pin == 0 || int_pin > 4)
> +		return;
> +
> +	ks_pcie_app_writel(ks_pcie, PCIE_LEGACY_IRQ_ENABLE_SET(int_pin),
> +			   INT_ENABLE);
> +	ks_pcie_app_writel(ks_pcie, PCIE_EP_IRQ_SET, INT_ENABLE);
> +	mdelay(1);
> +	ks_pcie_app_writel(ks_pcie, PCIE_EP_IRQ_CLR, INT_ENABLE);
> +	ks_pcie_app_writel(ks_pcie, PCIE_LEGACY_IRQ_ENABLE_CLR(int_pin),
> +			   INT_ENABLE);
> +}
> +
> +static int ks_pcie_am654_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
> +				   enum pci_epc_irq_type type,
> +				   u16 interrupt_num)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> +	struct keystone_pcie *ks_pcie = to_keystone_pcie(pci);
> +
> +	switch (type) {
> +	case PCI_EPC_IRQ_LEGACY:
> +		ks_pcie_am654_raise_legacy_irq(ks_pcie);
> +		break;
> +	case PCI_EPC_IRQ_MSI:
> +		dw_pcie_ep_raise_msi_irq(ep, func_no, interrupt_num);
> +		break;
> +	default:
> +		dev_err(pci->dev, "UNKNOWN IRQ type\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct pci_epc_features ks_pcie_am654_epc_features = {
> +	.linkup_notifier = false,
> +	.msi_capable = true,
> +	.msix_capable = false,
> +	.reserved_bar = 1 << BAR_0 | 1 << BAR_1,
> +	.bar_fixed_64bit = 1 << BAR_0,
> +	.bar_fixed_size[2] = SZ_1M,
> +	.bar_fixed_size[3] = SZ_64K,
> +	.bar_fixed_size[4] = 256,
> +	.bar_fixed_size[5] = SZ_1M,
> +	.align = SZ_1M,
> +};
> +
> +static const struct pci_epc_features*
> +ks_pcie_am654_get_features(struct dw_pcie_ep *ep)
> +{
> +	return &ks_pcie_am654_epc_features;
> +}
> +
> +static const struct dw_pcie_ep_ops ks_pcie_am654_ep_ops = {
> +	.ep_init = ks_pcie_am654_ep_init,
> +	.raise_irq = ks_pcie_am654_raise_irq,
> +	.get_features = &ks_pcie_am654_get_features,
> +};
> +
> +static int __init ks_pcie_add_pcie_ep(struct keystone_pcie *ks_pcie,
> +				      struct platform_device *pdev)
> +{
> +	int ret;
> +	struct dw_pcie_ep *ep;
> +	struct resource *res;
> +	struct device *dev = &pdev->dev;
> +	struct dw_pcie *pci = ks_pcie->pci;
> +
> +	ep = &pci->ep;
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "addr_space");
> +	if (!res)
> +		return -EINVAL;
> +
> +	ep->phys_base = res->start;
> +	ep->addr_size = resource_size(res);
> +
> +	ret = dw_pcie_ep_init(ep);
> +	if (ret) {
> +		dev_err(dev, "failed to initialize endpoint\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  static void ks_pcie_disable_phy(struct keystone_pcie *ks_pcie)
>  {
>  	int num_lanes = ks_pcie->num_lanes;
> @@ -950,7 +1088,8 @@ static int ks_pcie_set_mode(struct device *dev)
>  	return 0;
>  }
>  
> -static int ks_pcie_am654_set_mode(struct device *dev)
> +static int ks_pcie_am654_set_mode(struct device *dev,
> +				  enum dw_pcie_device_mode mode)
>  {
>  	struct device_node *np = dev->of_node;
>  	struct regmap *syscon;
> @@ -963,7 +1102,18 @@ static int ks_pcie_am654_set_mode(struct device *dev)
>  		return 0;
>  
>  	mask = AM654_PCIE_DEV_TYPE_MASK;
> -	val = RC;
> +
> +	switch (mode) {
> +	case DW_PCIE_RC_TYPE:
> +		val = RC;
> +		break;
> +	case DW_PCIE_EP_TYPE:
> +		val = EP;
> +		break;
> +	default:
> +		dev_err(dev, "INVALID device type %d\n", mode);
> +		return -EINVAL;
> +	}
>  
>  	ret = regmap_update_bits(syscon, 0, mask, val);
>  	if (ret) {
> @@ -1006,6 +1156,13 @@ static const struct ks_pcie_of_data ks_pcie_rc_of_data = {
>  
>  static const struct ks_pcie_of_data ks_pcie_am654_rc_of_data = {
>  	.host_ops = &ks_pcie_am654_host_ops,
> +	.mode = DW_PCIE_RC_TYPE,
> +	.version = 0x490A,
> +};
> +
> +static const struct ks_pcie_of_data ks_pcie_am654_ep_of_data = {
> +	.ep_ops = &ks_pcie_am654_ep_ops,
> +	.mode = DW_PCIE_EP_TYPE,
>  	.version = 0x490A,
>  };
>  
> @@ -1019,16 +1176,22 @@ static const struct of_device_id ks_pcie_of_match[] = {
>  		.data = &ks_pcie_am654_rc_of_data,
>  		.compatible = "ti,am654-pcie-rc",
>  	},
> +	{
> +		.data = &ks_pcie_am654_ep_of_data,
> +		.compatible = "ti,am654-pcie-ep",
> +	},
>  	{ },
>  };
>  
>  static int __init ks_pcie_probe(struct platform_device *pdev)
>  {
>  	const struct dw_pcie_host_ops *host_ops;
> +	const struct dw_pcie_ep_ops *ep_ops;
>  	struct device *dev = &pdev->dev;
>  	struct device_node *np = dev->of_node;
>  	const struct ks_pcie_of_data *data;
>  	const struct of_device_id *match;
> +	enum dw_pcie_device_mode mode;
>  	struct dw_pcie *pci;
>  	struct keystone_pcie *ks_pcie;
>  	struct device_link **link;
> @@ -1053,6 +1216,8 @@ static int __init ks_pcie_probe(struct platform_device *pdev)
>  
>  	version = data->version;
>  	host_ops = data->host_ops;
> +	ep_ops = data->ep_ops;
> +	mode = data->mode;
>  
>  	ks_pcie = devm_kzalloc(dev, sizeof(*ks_pcie), GFP_KERNEL);
>  	if (!ks_pcie)
> @@ -1078,16 +1243,11 @@ static int __init ks_pcie_probe(struct platform_device *pdev)
>  		ks_pcie->is_am6 = true;
>  
>  	pci->dbi_base = base;
> +	pci->dbi_base2 = base;
>  	pci->dev = dev;
>  	pci->ops = &ks_pcie_dw_pcie_ops;
>  	pci->version = version;
>  
> -	ret = of_property_read_u32(np, "num-viewport", &num_viewport);
> -	if (ret < 0) {
> -		dev_err(dev, "unable to read *num-viewport* property\n");
> -		return ret;
> -	}
> -
>  	irq = platform_get_irq(pdev, 0);
>  	if (irq < 0) {
>  		dev_err(dev, "missing IRQ resource: %d\n", irq);
> @@ -1136,7 +1296,6 @@ static int __init ks_pcie_probe(struct platform_device *pdev)
>  	ks_pcie->pci = pci;
>  	ks_pcie->link = link;
>  	ks_pcie->num_lanes = num_lanes;
> -	ks_pcie->num_viewport = num_viewport;
>  	ks_pcie->phy = phy;
>  
>  	gpiod = devm_gpiod_get_optional(dev, "reset",
> @@ -1172,7 +1331,7 @@ static int __init ks_pcie_probe(struct platform_device *pdev)
>  
>  		pci->atu_base = atu_base;
>  
> -		ret = ks_pcie_am654_set_mode(dev);
> +		ret = ks_pcie_am654_set_mode(dev, mode);
>  		if (ret < 0)
>  			goto err_get_sync;
>  	} else {
> @@ -1181,29 +1340,58 @@ static int __init ks_pcie_probe(struct platform_device *pdev)
>  			goto err_get_sync;
>  	}
>  
> -	/*
> -	 * "Power Sequencing and Reset Signal Timings" table in
> -	 * PCI EXPRESS CARD ELECTROMECHANICAL SPECIFICATION, REV. 2.0
> -	 * indicates PERST# should be deasserted after minimum of 100us
> -	 * once REFCLK is stable. The REFCLK to the connector in RC
> -	 * mode is selected while enabling the PHY. So deassert PERST#
> -	 * after 100 us.
> -	 */
> -	if (gpiod) {
> -		usleep_range(100, 200);
> -		gpiod_set_value_cansleep(gpiod, 1);
> -	}
> -
>  	link_speed = of_pci_get_max_link_speed(np);
>  	if (link_speed < 0)
>  		link_speed = 2;
>  
>  	ks_pcie_set_link_speed(pci, link_speed);
>  
> -	pci->pp.ops = host_ops;
> -	ret = ks_pcie_add_pcie_port(ks_pcie, pdev);
> -	if (ret < 0)
> -		goto err_get_sync;
> +	switch (mode) {
> +	case DW_PCIE_RC_TYPE:
> +		if (!IS_ENABLED(CONFIG_PCI_KEYSTONE_HOST)) {
> +			ret = -ENODEV;
> +			goto err_get_sync;
> +		}
> +
> +		ret = of_property_read_u32(np, "num-viewport", &num_viewport);
> +		if (ret < 0) {
> +			dev_err(dev, "unable to read *num-viewport* property\n");
> +			return ret;
> +		}
> +
> +		/*
> +		 * "Power Sequencing and Reset Signal Timings" table in
> +		 * PCI EXPRESS CARD ELECTROMECHANICAL SPECIFICATION, REV. 2.0
> +		 * indicates PERST# should be deasserted after minimum of 100us
> +		 * once REFCLK is stable. The REFCLK to the connector in RC
> +		 * mode is selected while enabling the PHY. So deassert PERST#
> +		 * after 100 us.
> +		 */
> +		if (gpiod) {
> +			usleep_range(100, 200);
> +			gpiod_set_value_cansleep(gpiod, 1);
> +		}
> +
> +		ks_pcie->num_viewport = num_viewport;
> +		pci->pp.ops = host_ops;
> +		ret = ks_pcie_add_pcie_port(ks_pcie, pdev);
> +		if (ret < 0)
> +			goto err_get_sync;
> +		break;
> +	case DW_PCIE_EP_TYPE:
> +		if (!IS_ENABLED(CONFIG_PCI_KEYSTONE_EP)) {
> +			ret = -ENODEV;
> +			goto err_get_sync;
> +		}
> +
> +		pci->ep.ops = ep_ops;
> +		ret = ks_pcie_add_pcie_ep(ks_pcie, pdev);
> +		if (ret < 0)
> +			goto err_get_sync;
> +		break;
> +	default:
> +		dev_err(dev, "INVALID device type %d\n", mode);
> +	}
>  
>  	ks_pcie_enable_error_irq(ks_pcie);
>  
> -- 
> 2.17.1
> 

Re: [PATCH v3 22/26] PCI: keystone: Add support for PCIe EP in AM654x Platforms

[Kishon Vijay Abraham I]

Hi Lorenzo,

On 26/04/19 3:10 PM, Lorenzo Pieralisi wrote:
> On Mon, Mar 25, 2019 at 03:09:43PM +0530, Kishon Vijay Abraham I wrote:
>> Add PCIe EP support for AM654x Platforms in pci-keystone.c
>>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> ---
>>  drivers/pci/controller/dwc/Kconfig        |  23 +-
>>  drivers/pci/controller/dwc/pci-keystone.c | 242 +++++++++++++++++++---
>>  2 files changed, 236 insertions(+), 29 deletions(-)
> 
> Hi Kishon,
> 
> WARNING: DT compatible string "ti,am654-pcie-ep" appears un-documented -- check /work/sandbox/pci/Documentation/devicetree/bindings/
> #301: FILE: drivers/pci/controller/dwc/pci-keystone.c:1179:
> +		.compatible = "ti,am654-pcie-ep",
> 
> I would like this warning to be fixed please on this patch.

Looks like I've missed adding the dt-binding Documentation patch. I'll send a
patch to add the EP mode dt-binding Documentation.

Thanks
Kishon
> 
> Thanks,
> Lorenzo
> 
>> diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
>> index d1d00833e0b3..b450ad2823a5 100644
>> --- a/drivers/pci/controller/dwc/Kconfig
>> +++ b/drivers/pci/controller/dwc/Kconfig
>> @@ -105,14 +105,33 @@ config PCIE_SPEAR13XX
>>  config PCI_KEYSTONE
>>  	bool "TI Keystone PCIe controller"
>>  	depends on ARCH_KEYSTONE || ARCH_K3 || ((ARM || ARM64) && COMPILE_TEST)
>> -	depends on PCI_MSI_IRQ_DOMAIN
>> -	select PCIE_DW_HOST
>>  	help
>>  	  Say Y here if you want to enable PCI controller support on Keystone
>>  	  SoCs. The PCI controller on Keystone is based on DesignWare hardware
>>  	  and therefore the driver re-uses the DesignWare core functions to
>>  	  implement the driver.
>>  
>> +if PCI_KEYSTONE
>> +
>> +config PCI_KEYSTONE_HOST
>> +	bool "PCI Keystone Host Mode"
>> +	depends on PCI_MSI_IRQ_DOMAIN
>> +	select PCIE_DW_HOST
>> +	default y
>> +	help
>> +	 Enables support for the PCIe controller in the Keystone SoC to work in
>> +	 host mode.
>> +
>> +config PCI_KEYSTONE_EP
>> +	bool "PCI Keystone Endpoint Mode"
>> +	depends on PCI_ENDPOINT
>> +	select PCIE_DW_EP
>> +	help
>> +	 Enables support for the PCIe controller in the Keystone SoC to work in
>> +	 endpoint mode.
>> +
>> +endif
>> +
>>  config PCI_LAYERSCAPE
>>  	bool "Freescale Layerscape PCIe controller"
>>  	depends on OF && (ARM || ARCH_LAYERSCAPE || COMPILE_TEST)
>> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
>> index 312fd0c49bbb..459485b0f65d 100644
>> --- a/drivers/pci/controller/dwc/pci-keystone.c
>> +++ b/drivers/pci/controller/dwc/pci-keystone.c
>> @@ -52,6 +52,12 @@
>>  #define OB_ENABLEN			BIT(0)
>>  #define OB_WIN_SIZE			8	/* 8MB */
>>  
>> +#define PCIE_LEGACY_IRQ_ENABLE_SET(n)	(0x188 + (0x10 * ((n) - 1)))
>> +#define PCIE_LEGACY_IRQ_ENABLE_CLR(n)	(0x18c + (0x10 * ((n) - 1)))
>> +#define PCIE_EP_IRQ_SET			0x64
>> +#define PCIE_EP_IRQ_CLR			0x68
>> +#define INT_ENABLE			BIT(0)
>> +
>>  /* IRQ register defines */
>>  #define IRQ_EOI				0x050
>>  
>> @@ -95,11 +101,16 @@
>>  #define KS_PCIE_SYSCLOCKOUTEN		BIT(0)
>>  
>>  #define AM654_PCIE_DEV_TYPE_MASK	0x3
>> +#define AM654_WIN_SIZE			SZ_64K
>> +
>> +#define APP_ADDR_SPACE_0		(16 * SZ_1K)
>>  
>>  #define to_keystone_pcie(x)		dev_get_drvdata((x)->dev)
>>  
>>  struct ks_pcie_of_data {
>> +	enum dw_pcie_device_mode mode;
>>  	const struct dw_pcie_host_ops *host_ops;
>> +	const struct dw_pcie_ep_ops *ep_ops;
>>  	unsigned int version;
>>  };
>>  
>> @@ -877,12 +888,139 @@ static int __init ks_pcie_add_pcie_port(struct keystone_pcie *ks_pcie,
>>  	return 0;
>>  }
>>  
>> +static u32 ks_pcie_am654_read_dbi2(struct dw_pcie *pci, void __iomem *base,
>> +				   u32 reg, size_t size)
>> +{
>> +	struct keystone_pcie *ks_pcie = to_keystone_pcie(pci);
>> +	u32 val;
>> +
>> +	ks_pcie_set_dbi_mode(ks_pcie);
>> +	dw_pcie_read(base + reg, size, &val);
>> +	ks_pcie_clear_dbi_mode(ks_pcie);
>> +	return val;
>> +}
>> +
>> +static void ks_pcie_am654_write_dbi2(struct dw_pcie *pci, void __iomem *base,
>> +				     u32 reg, size_t size, u32 val)
>> +{
>> +	struct keystone_pcie *ks_pcie = to_keystone_pcie(pci);
>> +
>> +	ks_pcie_set_dbi_mode(ks_pcie);
>> +	dw_pcie_write(base + reg, size, val);
>> +	ks_pcie_clear_dbi_mode(ks_pcie);
>> +}
>> +
>>  static const struct dw_pcie_ops ks_pcie_dw_pcie_ops = {
>>  	.start_link = ks_pcie_start_link,
>>  	.stop_link = ks_pcie_stop_link,
>>  	.link_up = ks_pcie_link_up,
>> +	.read_dbi2 = ks_pcie_am654_read_dbi2,
>> +	.write_dbi2 = ks_pcie_am654_write_dbi2,
>>  };
>>  
>> +static void ks_pcie_am654_ep_init(struct dw_pcie_ep *ep)
>> +{
>> +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>> +	int flags;
>> +
>> +	ep->page_size = AM654_WIN_SIZE;
>> +	flags = PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_32;
>> +	dw_pcie_writel_dbi2(pci, PCI_BASE_ADDRESS_0, APP_ADDR_SPACE_0 - 1);
>> +	dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_0, flags);
>> +}
>> +
>> +static void ks_pcie_am654_raise_legacy_irq(struct keystone_pcie *ks_pcie)
>> +{
>> +	struct dw_pcie *pci = ks_pcie->pci;
>> +	u8 int_pin;
>> +
>> +	int_pin = dw_pcie_readb_dbi(pci, PCI_INTERRUPT_PIN);
>> +	if (int_pin == 0 || int_pin > 4)
>> +		return;
>> +
>> +	ks_pcie_app_writel(ks_pcie, PCIE_LEGACY_IRQ_ENABLE_SET(int_pin),
>> +			   INT_ENABLE);
>> +	ks_pcie_app_writel(ks_pcie, PCIE_EP_IRQ_SET, INT_ENABLE);
>> +	mdelay(1);
>> +	ks_pcie_app_writel(ks_pcie, PCIE_EP_IRQ_CLR, INT_ENABLE);
>> +	ks_pcie_app_writel(ks_pcie, PCIE_LEGACY_IRQ_ENABLE_CLR(int_pin),
>> +			   INT_ENABLE);
>> +}
>> +
>> +static int ks_pcie_am654_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
>> +				   enum pci_epc_irq_type type,
>> +				   u16 interrupt_num)
>> +{
>> +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>> +	struct keystone_pcie *ks_pcie = to_keystone_pcie(pci);
>> +
>> +	switch (type) {
>> +	case PCI_EPC_IRQ_LEGACY:
>> +		ks_pcie_am654_raise_legacy_irq(ks_pcie);
>> +		break;
>> +	case PCI_EPC_IRQ_MSI:
>> +		dw_pcie_ep_raise_msi_irq(ep, func_no, interrupt_num);
>> +		break;
>> +	default:
>> +		dev_err(pci->dev, "UNKNOWN IRQ type\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct pci_epc_features ks_pcie_am654_epc_features = {
>> +	.linkup_notifier = false,
>> +	.msi_capable = true,
>> +	.msix_capable = false,
>> +	.reserved_bar = 1 << BAR_0 | 1 << BAR_1,
>> +	.bar_fixed_64bit = 1 << BAR_0,
>> +	.bar_fixed_size[2] = SZ_1M,
>> +	.bar_fixed_size[3] = SZ_64K,
>> +	.bar_fixed_size[4] = 256,
>> +	.bar_fixed_size[5] = SZ_1M,
>> +	.align = SZ_1M,
>> +};
>> +
>> +static const struct pci_epc_features*
>> +ks_pcie_am654_get_features(struct dw_pcie_ep *ep)
>> +{
>> +	return &ks_pcie_am654_epc_features;
>> +}
>> +
>> +static const struct dw_pcie_ep_ops ks_pcie_am654_ep_ops = {
>> +	.ep_init = ks_pcie_am654_ep_init,
>> +	.raise_irq = ks_pcie_am654_raise_irq,
>> +	.get_features = &ks_pcie_am654_get_features,
>> +};
>> +
>> +static int __init ks_pcie_add_pcie_ep(struct keystone_pcie *ks_pcie,
>> +				      struct platform_device *pdev)
>> +{
>> +	int ret;
>> +	struct dw_pcie_ep *ep;
>> +	struct resource *res;
>> +	struct device *dev = &pdev->dev;
>> +	struct dw_pcie *pci = ks_pcie->pci;
>> +
>> +	ep = &pci->ep;
>> +
>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "addr_space");
>> +	if (!res)
>> +		return -EINVAL;
>> +
>> +	ep->phys_base = res->start;
>> +	ep->addr_size = resource_size(res);
>> +
>> +	ret = dw_pcie_ep_init(ep);
>> +	if (ret) {
>> +		dev_err(dev, "failed to initialize endpoint\n");
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>  static void ks_pcie_disable_phy(struct keystone_pcie *ks_pcie)
>>  {
>>  	int num_lanes = ks_pcie->num_lanes;
>> @@ -950,7 +1088,8 @@ static int ks_pcie_set_mode(struct device *dev)
>>  	return 0;
>>  }
>>  
>> -static int ks_pcie_am654_set_mode(struct device *dev)
>> +static int ks_pcie_am654_set_mode(struct device *dev,
>> +				  enum dw_pcie_device_mode mode)
>>  {
>>  	struct device_node *np = dev->of_node;
>>  	struct regmap *syscon;
>> @@ -963,7 +1102,18 @@ static int ks_pcie_am654_set_mode(struct device *dev)
>>  		return 0;
>>  
>>  	mask = AM654_PCIE_DEV_TYPE_MASK;
>> -	val = RC;
>> +
>> +	switch (mode) {
>> +	case DW_PCIE_RC_TYPE:
>> +		val = RC;
>> +		break;
>> +	case DW_PCIE_EP_TYPE:
>> +		val = EP;
>> +		break;
>> +	default:
>> +		dev_err(dev, "INVALID device type %d\n", mode);
>> +		return -EINVAL;
>> +	}
>>  
>>  	ret = regmap_update_bits(syscon, 0, mask, val);
>>  	if (ret) {
>> @@ -1006,6 +1156,13 @@ static const struct ks_pcie_of_data ks_pcie_rc_of_data = {
>>  
>>  static const struct ks_pcie_of_data ks_pcie_am654_rc_of_data = {
>>  	.host_ops = &ks_pcie_am654_host_ops,
>> +	.mode = DW_PCIE_RC_TYPE,
>> +	.version = 0x490A,
>> +};
>> +
>> +static const struct ks_pcie_of_data ks_pcie_am654_ep_of_data = {
>> +	.ep_ops = &ks_pcie_am654_ep_ops,
>> +	.mode = DW_PCIE_EP_TYPE,
>>  	.version = 0x490A,
>>  };
>>  
>> @@ -1019,16 +1176,22 @@ static const struct of_device_id ks_pcie_of_match[] = {
>>  		.data = &ks_pcie_am654_rc_of_data,
>>  		.compatible = "ti,am654-pcie-rc",
>>  	},
>> +	{
>> +		.data = &ks_pcie_am654_ep_of_data,
>> +		.compatible = "ti,am654-pcie-ep",
>> +	},
>>  	{ },
>>  };
>>  
>>  static int __init ks_pcie_probe(struct platform_device *pdev)
>>  {
>>  	const struct dw_pcie_host_ops *host_ops;
>> +	const struct dw_pcie_ep_ops *ep_ops;
>>  	struct device *dev = &pdev->dev;
>>  	struct device_node *np = dev->of_node;
>>  	const struct ks_pcie_of_data *data;
>>  	const struct of_device_id *match;
>> +	enum dw_pcie_device_mode mode;
>>  	struct dw_pcie *pci;
>>  	struct keystone_pcie *ks_pcie;
>>  	struct device_link **link;
>> @@ -1053,6 +1216,8 @@ static int __init ks_pcie_probe(struct platform_device *pdev)
>>  
>>  	version = data->version;
>>  	host_ops = data->host_ops;
>> +	ep_ops = data->ep_ops;
>> +	mode = data->mode;
>>  
>>  	ks_pcie = devm_kzalloc(dev, sizeof(*ks_pcie), GFP_KERNEL);
>>  	if (!ks_pcie)
>> @@ -1078,16 +1243,11 @@ static int __init ks_pcie_probe(struct platform_device *pdev)
>>  		ks_pcie->is_am6 = true;
>>  
>>  	pci->dbi_base = base;
>> +	pci->dbi_base2 = base;
>>  	pci->dev = dev;
>>  	pci->ops = &ks_pcie_dw_pcie_ops;
>>  	pci->version = version;
>>  
>> -	ret = of_property_read_u32(np, "num-viewport", &num_viewport);
>> -	if (ret < 0) {
>> -		dev_err(dev, "unable to read *num-viewport* property\n");
>> -		return ret;
>> -	}
>> -
>>  	irq = platform_get_irq(pdev, 0);
>>  	if (irq < 0) {
>>  		dev_err(dev, "missing IRQ resource: %d\n", irq);
>> @@ -1136,7 +1296,6 @@ static int __init ks_pcie_probe(struct platform_device *pdev)
>>  	ks_pcie->pci = pci;
>>  	ks_pcie->link = link;
>>  	ks_pcie->num_lanes = num_lanes;
>> -	ks_pcie->num_viewport = num_viewport;
>>  	ks_pcie->phy = phy;
>>  
>>  	gpiod = devm_gpiod_get_optional(dev, "reset",
>> @@ -1172,7 +1331,7 @@ static int __init ks_pcie_probe(struct platform_device *pdev)
>>  
>>  		pci->atu_base = atu_base;
>>  
>> -		ret = ks_pcie_am654_set_mode(dev);
>> +		ret = ks_pcie_am654_set_mode(dev, mode);
>>  		if (ret < 0)
>>  			goto err_get_sync;
>>  	} else {
>> @@ -1181,29 +1340,58 @@ static int __init ks_pcie_probe(struct platform_device *pdev)
>>  			goto err_get_sync;
>>  	}
>>  
>> -	/*
>> -	 * "Power Sequencing and Reset Signal Timings" table in
>> -	 * PCI EXPRESS CARD ELECTROMECHANICAL SPECIFICATION, REV. 2.0
>> -	 * indicates PERST# should be deasserted after minimum of 100us
>> -	 * once REFCLK is stable. The REFCLK to the connector in RC
>> -	 * mode is selected while enabling the PHY. So deassert PERST#
>> -	 * after 100 us.
>> -	 */
>> -	if (gpiod) {
>> -		usleep_range(100, 200);
>> -		gpiod_set_value_cansleep(gpiod, 1);
>> -	}
>> -
>>  	link_speed = of_pci_get_max_link_speed(np);
>>  	if (link_speed < 0)
>>  		link_speed = 2;
>>  
>>  	ks_pcie_set_link_speed(pci, link_speed);
>>  
>> -	pci->pp.ops = host_ops;
>> -	ret = ks_pcie_add_pcie_port(ks_pcie, pdev);
>> -	if (ret < 0)
>> -		goto err_get_sync;
>> +	switch (mode) {
>> +	case DW_PCIE_RC_TYPE:
>> +		if (!IS_ENABLED(CONFIG_PCI_KEYSTONE_HOST)) {
>> +			ret = -ENODEV;
>> +			goto err_get_sync;
>> +		}
>> +
>> +		ret = of_property_read_u32(np, "num-viewport", &num_viewport);
>> +		if (ret < 0) {
>> +			dev_err(dev, "unable to read *num-viewport* property\n");
>> +			return ret;
>> +		}
>> +
>> +		/*
>> +		 * "Power Sequencing and Reset Signal Timings" table in
>> +		 * PCI EXPRESS CARD ELECTROMECHANICAL SPECIFICATION, REV. 2.0
>> +		 * indicates PERST# should be deasserted after minimum of 100us
>> +		 * once REFCLK is stable. The REFCLK to the connector in RC
>> +		 * mode is selected while enabling the PHY. So deassert PERST#
>> +		 * after 100 us.
>> +		 */
>> +		if (gpiod) {
>> +			usleep_range(100, 200);
>> +			gpiod_set_value_cansleep(gpiod, 1);
>> +		}
>> +
>> +		ks_pcie->num_viewport = num_viewport;
>> +		pci->pp.ops = host_ops;
>> +		ret = ks_pcie_add_pcie_port(ks_pcie, pdev);
>> +		if (ret < 0)
>> +			goto err_get_sync;
>> +		break;
>> +	case DW_PCIE_EP_TYPE:
>> +		if (!IS_ENABLED(CONFIG_PCI_KEYSTONE_EP)) {
>> +			ret = -ENODEV;
>> +			goto err_get_sync;
>> +		}
>> +
>> +		pci->ep.ops = ep_ops;
>> +		ret = ks_pcie_add_pcie_ep(ks_pcie, pdev);
>> +		if (ret < 0)
>> +			goto err_get_sync;
>> +		break;
>> +	default:
>> +		dev_err(dev, "INVALID device type %d\n", mode);
>> +	}
>>  
>>  	ks_pcie_enable_error_irq(ks_pcie);
>>  
>> -- 
>> 2.17.1
>>

[PATCH v3 23/26] PCI: designware-ep: Configure RESBAR to advertise the smallest size

[Kishon Vijay Abraham I]

Configure RESBAR capability to advertise the smallest size (1MB) for
couple of reasons. A) Host side resource allocation of BAR fails for
larger sizes. B) Endpoint function driver does not allocate memory
for all supported sizes in RESBAR capability.
If and when there is a usecase required to add more flexibility using
RESBAR, this can be revisited.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 .../pci/controller/dwc/pcie-designware-ep.c   | 34 +++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
index 74477ad7467f..0c208b9bda43 100644
--- a/drivers/pci/controller/dwc/pcie-designware-ep.c
+++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
@@ -504,10 +504,32 @@ void dw_pcie_ep_exit(struct dw_pcie_ep *ep)
 	pci_epc_mem_exit(epc);
 }
 
+static unsigned int dw_pcie_ep_find_ext_capability(struct dw_pcie *pci, int cap)
+{
+	u32 header;
+	int pos = PCI_CFG_SPACE_SIZE;
+
+	while (pos) {
+		header = dw_pcie_readl_dbi(pci, pos);
+		if (PCI_EXT_CAP_ID(header) == cap)
+			return pos;
+
+		pos = PCI_EXT_CAP_NEXT(header);
+		if (!pos)
+			break;
+	}
+
+	return 0;
+}
+
 int dw_pcie_ep_init(struct dw_pcie_ep *ep)
 {
+	int i;
 	int ret;
+	u32 reg;
 	void *addr;
+	unsigned int nbars;
+	unsigned int offset;
 	struct pci_epc *epc;
 	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
 	struct device *dev = pci->dev;
@@ -591,6 +613,18 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
 
 	ep->msix_cap = dw_pcie_ep_find_capability(pci, PCI_CAP_ID_MSIX);
 
+	offset = dw_pcie_ep_find_ext_capability(pci, PCI_EXT_CAP_ID_REBAR);
+	if (offset) {
+		reg = dw_pcie_readl_dbi(pci, offset + PCI_REBAR_CTRL);
+		nbars = (reg & PCI_REBAR_CTRL_NBAR_MASK) >>
+			PCI_REBAR_CTRL_NBAR_SHIFT;
+
+		dw_pcie_dbi_ro_wr_en(pci);
+		for (i = 0; i < nbars; i++, offset += PCI_REBAR_CTRL)
+			dw_pcie_writel_dbi(pci, offset + PCI_REBAR_CAP, 0x0);
+		dw_pcie_dbi_ro_wr_dis(pci);
+	}
+
 	dw_pcie_setup(pci);
 
 	return 0;
-- 
2.17.1

[PATCH v3 24/26] PCI: designware-ep: Use aligned ATU window for raising MSI interrupts

[Kishon Vijay Abraham I]

Certain platforms like K2G reguires the outbound ATU window to be
aligned. The alignment size is already present in mem->page_size.
Use the alignment size present in mem->page_size to configre a
aligned ATU window. In order to raise an interrupt, CPU has to write
to address offset from the start of the window unlike before where
writes were always to the beginning of the ATU window.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/pci/controller/dwc/pcie-designware-ep.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
index 0c208b9bda43..2bf5a35c0570 100644
--- a/drivers/pci/controller/dwc/pcie-designware-ep.c
+++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
@@ -397,6 +397,7 @@ int dw_pcie_ep_raise_msi_irq(struct dw_pcie_ep *ep, u8 func_no,
 {
 	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
 	struct pci_epc *epc = ep->epc;
+	unsigned int aligned_offset;
 	u16 msg_ctrl, msg_data;
 	u32 msg_addr_lower, msg_addr_upper, reg;
 	u64 msg_addr;
@@ -422,13 +423,15 @@ int dw_pcie_ep_raise_msi_irq(struct dw_pcie_ep *ep, u8 func_no,
 		reg = ep->msi_cap + PCI_MSI_DATA_32;
 		msg_data = dw_pcie_readw_dbi(pci, reg);
 	}
-	msg_addr = ((u64) msg_addr_upper) << 32 | msg_addr_lower;
+	aligned_offset = msg_addr_lower & (epc->mem->page_size - 1);
+	msg_addr = ((u64)msg_addr_upper) << 32 |
+			(msg_addr_lower & ~aligned_offset);
 	ret = dw_pcie_ep_map_addr(epc, func_no, ep->msi_mem_phys, msg_addr,
 				  epc->mem->page_size);
 	if (ret)
 		return ret;
 
-	writel(msg_data | (interrupt_num - 1), ep->msi_mem);
+	writel(msg_data | (interrupt_num - 1), ep->msi_mem + aligned_offset);
 
 	dw_pcie_ep_unmap_addr(epc, func_no, ep->msi_mem_phys);
 
-- 
2.17.1

Re: [PATCH v3 24/26] PCI: designware-ep: Use aligned ATU window for raising MSI interrupts

[Bjorn Helgaas]

On Mon, Mar 25, 2019 at 03:09:45PM +0530, Kishon Vijay Abraham I wrote:
> Certain platforms like K2G reguires the outbound ATU window to be

s/reguires/require/ (note two changes: "g->q" and drop s)

[PATCH v3 25/26] misc: pci_endpoint_test: Add support to test PCI EP in AM654x

[Kishon Vijay Abraham I]

TI's AM654x PCIe EP has a restriction that BAR_0 is mapped to
application registers. "PCIe Inbound Address Translation" section in
AM65x Sitara Processors TRM (SPRUID7 – April 2018) describes BAR0 is
reserved. Configure pci_endpoint_test to use BAR_2 instead.
Also set alignment to 64K since "PCIe Subsystem Address Translation"
section in TRM indicates minimum ATU window size is 64K.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/misc/pci_endpoint_test.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
index 29582fe57151..e015e8fa9bd3 100644
--- a/drivers/misc/pci_endpoint_test.c
+++ b/drivers/misc/pci_endpoint_test.c
@@ -75,6 +75,11 @@
 #define PCI_ENDPOINT_TEST_IRQ_TYPE		0x24
 #define PCI_ENDPOINT_TEST_IRQ_NUMBER		0x28
 
+#define PCI_DEVICE_ID_TI_AM654			0xb00c
+
+#define is_am654_pci_dev(pdev)		\
+		((pdev)->device == PCI_DEVICE_ID_TI_AM654)
+
 static DEFINE_IDA(pci_endpoint_test_ida);
 
 #define to_endpoint_test(priv) container_of((priv), struct pci_endpoint_test, \
@@ -588,6 +593,7 @@ static long pci_endpoint_test_ioctl(struct file *file, unsigned int cmd,
 	int ret = -EINVAL;
 	enum pci_barno bar;
 	struct pci_endpoint_test *test = to_endpoint_test(file->private_data);
+	struct pci_dev *pdev = test->pdev;
 
 	mutex_lock(&test->mutex);
 	switch (cmd) {
@@ -595,6 +601,8 @@ static long pci_endpoint_test_ioctl(struct file *file, unsigned int cmd,
 		bar = arg;
 		if (bar < 0 || bar > 5)
 			goto ret;
+		if (is_am654_pci_dev(pdev) && bar == BAR_0)
+			goto ret;
 		ret = pci_endpoint_test_bar(test, bar);
 		break;
 	case PCITEST_LEGACY_IRQ:
@@ -785,11 +793,20 @@ static void pci_endpoint_test_remove(struct pci_dev *pdev)
 	pci_disable_device(pdev);
 }
 
+static const struct pci_endpoint_test_data am654_data = {
+	.test_reg_bar = BAR_2,
+	.alignment = SZ_64K,
+	.irq_type = IRQ_TYPE_MSI,
+};
+
 static const struct pci_device_id pci_endpoint_test_tbl[] = {
 	{ PCI_DEVICE(PCI_VENDOR_ID_TI, PCI_DEVICE_ID_TI_DRA74x) },
 	{ PCI_DEVICE(PCI_VENDOR_ID_TI, PCI_DEVICE_ID_TI_DRA72x) },
 	{ PCI_DEVICE(PCI_VENDOR_ID_FREESCALE, 0x81c0) },
 	{ PCI_DEVICE(PCI_VENDOR_ID_SYNOPSYS, 0xedda) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_TI, PCI_DEVICE_ID_TI_AM654),
+	  .driver_data = (kernel_ulong_t)&am654_data
+	},
 	{ }
 };
 MODULE_DEVICE_TABLE(pci, pci_endpoint_test_tbl);
-- 
2.17.1

[PATCH v3 26/26] misc: pci_endpoint_test: Fix test_reg_bar to be updated in pci_endpoint_test

[Kishon Vijay Abraham I]

commit 834b9051992580ac8fd3966d023b ("misc: pci_endpoint_test: Add support
for PCI_ENDPOINT_TEST regs to be mapped to any BAR") while adding
test_reg_bar in order to map PCI_ENDPOINT_TEST regs to be mapped to any
BAR failed to update test_reg_bar in pci_endpoint_test. This results in
test_reg_bar having invalid value when used outside probe. Fix it here.

Fixes: 834b9051992580ac8fd3966d023b ("misc: pci_endpoint_test: Add support
for PCI_ENDPOINT_TEST regs to be mapped to any BAR")

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/misc/pci_endpoint_test.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
index e015e8fa9bd3..7b015f2a1c6f 100644
--- a/drivers/misc/pci_endpoint_test.c
+++ b/drivers/misc/pci_endpoint_test.c
@@ -670,6 +670,7 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev,
 	data = (struct pci_endpoint_test_data *)ent->driver_data;
 	if (data) {
 		test_reg_bar = data->test_reg_bar;
+		test->test_reg_bar = test_reg_bar;
 		test->alignment = data->alignment;
 		irq_type = data->irq_type;
 	}
-- 
2.17.1

Re: [PATCH v3 00/26] Add support for PCIe RC and EP mode in TI's AM654 SoC

[Lorenzo Pieralisi]

On Mon, Mar 25, 2019 at 03:09:21PM +0530, Kishon Vijay Abraham I wrote:
> Add PCIe RC support for TI's AM654 SoC. The PCIe controller in AM654
> uses Synopsys core revision 4.90a and uses the same TI wrapper as used
> in keystone2 with certain modification. Hence AM654 will use the same
> pci wrapper driver pci-keystone.c
> 
> This series was initially part of [1]. This series only includes patches
> that has to be merged via Lorenzo's tree. The PHY patches and dt patches
> will be sent separately.
> 
> This series is created over keystone MSI cleanup series [2].
> 
> This series:
> *) Cleanup pci-keystone driver so that both RC mode and EP mode of
>    AM654 can be supported
> *) Modify epc-core to support allocation of aligned buffers required for
>    AM654
> *) Fix ATU unroll identification
> *) Add support for both host mode and device mode in AM654
> 
> Changes from v2:
> *) Missed updating "Reviewed-by: Rob Herring <robh@kernel.org>" tags
>    in the version that was sent to list.
> *) Add const qualifier to struct dw_pcie_ep_ops in pci-layerscape-ep.c
> 
> Changes from v1:
> *) Support for legacy interrupt in AM654 is removed (see background here
>    [3])
> *) Allow of_pci_get_max_link_speed to be used by Endpoint controller
> driver
> *) Add support to set max-link-speed from DT in pci-keystone driver
> *) Update "Reviewed-by: Rob Herring <robh@kernel.org>" tags.
> 
> [1] -> https://lore.kernel.org/patchwork/cover/989487/
> [2] -> https://lkml.org/lkml/2019/3/21/193
> [3] -> https://lkml.org/lkml/2019/3/19/235
> 
> Kishon Vijay Abraham I (26):
>   PCI: keystone: Add start_link/stop_link dw_pcie_ops
>   PCI: keystone: Cleanup error_irq configuration
>   dt-bindings: PCI: keystone: Add "reg-names" binding information
>   PCI: keystone: Perform host initialization in a single function
>   PCI: keystone: Use platform_get_resource_byname to get memory
>     resources
>   PCI: keystone: Move initializations to appropriate places
>   dt-bindings: PCI: Add dt-binding to configure PCIe mode
>   PCI: keystone: Explicitly set the PCIe mode
>   dt-bindings: PCI: Document "atu" reg-names
>   PCI: dwc: Enable iATU unroll for endpoint too
>   PCI: dwc: Fix ATU identification for designware version >= 4.80
>   PCI: keystone: Prevent ARM32 specific code to be compiled for ARM64
>   dt-bindings: PCI: Add PCI RC dt binding documentation for AM654
>   PCI: keystone: Add support for PCIe RC in AM654x Platforms
>   PCI: keystone: Invoke phy_reset API before enabling PHY
>   PCI: OF: Allow of_pci_get_max_link_speed() to be used by PCI Endpoint
>     drivers
>   PCI: keystone: Add support to set the max link speed from DT
>   PCI: endpoint: Add support to allocate aligned buffers to be mapped in
>     BARs
>   PCI: dwc: Add const qualifier to struct dw_pcie_ep_ops
>   PCI: dwc: Fix dw_pcie_ep_find_capability to return correct capability
>     offset
>   PCI: dwc: Add callbacks for accessing dbi2 address space
>   PCI: keystone: Add support for PCIe EP in AM654x Platforms
>   PCI: designware-ep: Configure RESBAR to advertise the smallest size
>   PCI: designware-ep: Use aligned ATU window for raising MSI interrupts
>   misc: pci_endpoint_test: Add support to test PCI EP in AM654x
>   misc: pci_endpoint_test: Fix test_reg_bar to be updated in
>     pci_endpoint_test
> 
>  .../bindings/pci/designware-pcie.txt          |   7 +-
>  .../devicetree/bindings/pci/pci-keystone.txt  |  14 +-
>  drivers/misc/pci_endpoint_test.c              |  18 +
>  drivers/pci/Makefile                          |   2 +-
>  drivers/pci/controller/dwc/Kconfig            |  25 +-
>  drivers/pci/controller/dwc/pci-dra7xx.c       |   2 +-
>  drivers/pci/controller/dwc/pci-keystone.c     | 577 +++++++++++++++---
>  .../pci/controller/dwc/pci-layerscape-ep.c    |   2 +-
>  drivers/pci/controller/dwc/pcie-artpec6.c     |   2 +-
>  .../pci/controller/dwc/pcie-designware-ep.c   |  55 +-
>  .../pci/controller/dwc/pcie-designware-host.c |  19 -
>  .../pci/controller/dwc/pcie-designware-plat.c |   2 +-
>  drivers/pci/controller/dwc/pcie-designware.c  |  52 ++
>  drivers/pci/controller/dwc/pcie-designware.h  |  15 +-
>  drivers/pci/endpoint/functions/pci-epf-test.c |   5 +-
>  drivers/pci/endpoint/pci-epf-core.c           |  10 +-
>  drivers/pci/of.c                              |  44 +-
>  include/linux/pci-epc.h                       |   2 +
>  include/linux/pci-epf.h                       |   3 +-
>  19 files changed, 683 insertions(+), 173 deletions(-)

Hi Kishon,

I have applied the series, after rewriting the commit logs we
discussed, branch: pci/keystone, please have a look and let me know
if that's good to go.

Thanks,
Lorenzo

Re: [PATCH v3 00/26] Add support for PCIe RC and EP mode in TI's AM654 SoC

[Bjorn Helgaas]

On Fri, Apr 12, 2019 at 04:48:36PM +0100, Lorenzo Pieralisi wrote:
> On Mon, Mar 25, 2019 at 03:09:21PM +0530, Kishon Vijay Abraham I wrote:
> > Add PCIe RC support for TI's AM654 SoC. The PCIe controller in AM654
> > uses Synopsys core revision 4.90a and uses the same TI wrapper as used
> > in keystone2 with certain modification. Hence AM654 will use the same
> > pci wrapper driver pci-keystone.c
> > 
> > This series was initially part of [1]. This series only includes patches
> > that has to be merged via Lorenzo's tree. The PHY patches and dt patches
> > will be sent separately.
> > 
> > This series is created over keystone MSI cleanup series [2].
> > 
> > This series:
> > *) Cleanup pci-keystone driver so that both RC mode and EP mode of
> >    AM654 can be supported
> > *) Modify epc-core to support allocation of aligned buffers required for
> >    AM654
> > *) Fix ATU unroll identification
> > *) Add support for both host mode and device mode in AM654
> > 
> > Changes from v2:
> > *) Missed updating "Reviewed-by: Rob Herring <robh@kernel.org>" tags
> >    in the version that was sent to list.
> > *) Add const qualifier to struct dw_pcie_ep_ops in pci-layerscape-ep.c
> > 
> > Changes from v1:
> > *) Support for legacy interrupt in AM654 is removed (see background here
> >    [3])
> > *) Allow of_pci_get_max_link_speed to be used by Endpoint controller
> > driver
> > *) Add support to set max-link-speed from DT in pci-keystone driver
> > *) Update "Reviewed-by: Rob Herring <robh@kernel.org>" tags.
> > 
> > [1] -> https://lore.kernel.org/patchwork/cover/989487/
> > [2] -> https://lkml.org/lkml/2019/3/21/193
> > [3] -> https://lkml.org/lkml/2019/3/19/235
> > 
> > Kishon Vijay Abraham I (26):
> >   PCI: keystone: Add start_link/stop_link dw_pcie_ops
> >   PCI: keystone: Cleanup error_irq configuration
> >   dt-bindings: PCI: keystone: Add "reg-names" binding information
> >   PCI: keystone: Perform host initialization in a single function
> >   PCI: keystone: Use platform_get_resource_byname to get memory
> >     resources
> >   PCI: keystone: Move initializations to appropriate places
> >   dt-bindings: PCI: Add dt-binding to configure PCIe mode
> >   PCI: keystone: Explicitly set the PCIe mode
> >   dt-bindings: PCI: Document "atu" reg-names
> >   PCI: dwc: Enable iATU unroll for endpoint too
> >   PCI: dwc: Fix ATU identification for designware version >= 4.80
> >   PCI: keystone: Prevent ARM32 specific code to be compiled for ARM64
> >   dt-bindings: PCI: Add PCI RC dt binding documentation for AM654
> >   PCI: keystone: Add support for PCIe RC in AM654x Platforms
> >   PCI: keystone: Invoke phy_reset API before enabling PHY
> >   PCI: OF: Allow of_pci_get_max_link_speed() to be used by PCI Endpoint
> >     drivers
> >   PCI: keystone: Add support to set the max link speed from DT
> >   PCI: endpoint: Add support to allocate aligned buffers to be mapped in
> >     BARs
> >   PCI: dwc: Add const qualifier to struct dw_pcie_ep_ops
> >   PCI: dwc: Fix dw_pcie_ep_find_capability to return correct capability
> >     offset
> >   PCI: dwc: Add callbacks for accessing dbi2 address space
> >   PCI: keystone: Add support for PCIe EP in AM654x Platforms
> >   PCI: designware-ep: Configure RESBAR to advertise the smallest size
> >   PCI: designware-ep: Use aligned ATU window for raising MSI interrupts
> >   misc: pci_endpoint_test: Add support to test PCI EP in AM654x
> >   misc: pci_endpoint_test: Fix test_reg_bar to be updated in
> >     pci_endpoint_test
> > 
> >  .../bindings/pci/designware-pcie.txt          |   7 +-
> >  .../devicetree/bindings/pci/pci-keystone.txt  |  14 +-
> >  drivers/misc/pci_endpoint_test.c              |  18 +
> >  drivers/pci/Makefile                          |   2 +-
> >  drivers/pci/controller/dwc/Kconfig            |  25 +-
> >  drivers/pci/controller/dwc/pci-dra7xx.c       |   2 +-
> >  drivers/pci/controller/dwc/pci-keystone.c     | 577 +++++++++++++++---
> >  .../pci/controller/dwc/pci-layerscape-ep.c    |   2 +-
> >  drivers/pci/controller/dwc/pcie-artpec6.c     |   2 +-
> >  .../pci/controller/dwc/pcie-designware-ep.c   |  55 +-
> >  .../pci/controller/dwc/pcie-designware-host.c |  19 -
> >  .../pci/controller/dwc/pcie-designware-plat.c |   2 +-
> >  drivers/pci/controller/dwc/pcie-designware.c  |  52 ++
> >  drivers/pci/controller/dwc/pcie-designware.h  |  15 +-
> >  drivers/pci/endpoint/functions/pci-epf-test.c |   5 +-
> >  drivers/pci/endpoint/pci-epf-core.c           |  10 +-
> >  drivers/pci/of.c                              |  44 +-
> >  include/linux/pci-epc.h                       |   2 +
> >  include/linux/pci-epf.h                       |   3 +-
> >  19 files changed, 683 insertions(+), 173 deletions(-)
> 
> Hi Kishon,
> 
> I have applied the series, after rewriting the commit logs we
> discussed, branch: pci/keystone, please have a look and let me know
> if that's good to go.

It all looks good to me; I responded to a few things I noticed while
writing the notes for merging the branch.  I don't think there were
any must-fix things, but if you update the branch for any other
reason, I'll happily remerge it (I remerge the "next" branch from
scratch every time I add things anyway, so it's no work on my side).

Bjorn