[PATCH v7 0/9] Designware EP support and code clean up

[PATCH v7 0/9] Designware EP support and code clean up

[Gustavo Pimentel]

The patch set was made against the Lorenzo's master branch.

Adds support Designware EP support.

Increases the maximum number of interrupts allowed for Designware IP
controller.

Does a code cleanup on Designware driver:
 - Replaces magic numbers without a easy meaning by a well known define
   that helps the human compreension.
 - Replaces a division by 2 by a simple right shift rotation of 1 bit.
 - Fixes all first letter characters on comments and debug messages to
   upper case to maintain coherency.

Gustavo Pimentel (9):
  bindings: PCI: designware: Example update
  PCI: dwc: Add support for endpoint mode
  PCI: endpoint: functions/pci-epf-test: Add second entry
  bindings: PCI: designware: Add support for the EP in Designware driver
  misc: pci_endpoint_test: Add designware EP entry
  PCI: dwc: Define maximum number of vectors
  PCI: dwc: Replace lower into upper case characters
  PCI: dwc: Small computation improvement
  PCI: dwc: Replace magic number by defines

 .../PCI/endpoint/function/binding/pci-test.txt     |   2 +
 .../devicetree/bindings/pci/designware-pcie.txt    |  24 +++-
 drivers/misc/pci_endpoint_test.c                   |   1 +
 drivers/pci/dwc/Kconfig                            |  41 ++++--
 drivers/pci/dwc/pcie-designware-ep.c               |  16 +--
 drivers/pci/dwc/pcie-designware-host.c             |  77 +++++-----
 drivers/pci/dwc/pcie-designware-plat.c             | 155 +++++++++++++++++++--
 drivers/pci/dwc/pcie-designware.c                  |  22 +--
 drivers/pci/dwc/pcie-designware.h                  |   1 +
 drivers/pci/endpoint/functions/pci-epf-test.c      |   8 ++
 10 files changed, 267 insertions(+), 80 deletions(-)

-- 
2.7.4

[PATCH v7 1/9] bindings: PCI: designware: Example update

[Gustavo Pimentel]

Replaces "ctrlreg" reg-name by "dbi" to be coherent with similar drivers,
however it still be compatible with any previous DT that uses the old
reg-name.

Replaces the PCIe base address example by a real PCIe base address in use.

Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
Changes v1->v2:
 - Removed any iATU reference or changes to avoid confusion.
 - Add "snps,dw-pcie" compatible string following Kishon's suggestion.
Changes v2->v3:
 - Nothing changed, just to follow the patch set version.
Changes v3->v4:
 - Reverted "snps,dw-pcie-rc" compatible string requested by Rob Herring.
Changes v4->v5:
 - Nothing changed, just to follow the patch set version.
Changes v5->v6:
 - Nothing changed, just to follow the patch set version.
Changes v6->v7:
 - Nothing changed, just to follow the patch set version.

 Documentation/devicetree/bindings/pci/designware-pcie.txt | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt b/Documentation/devicetree/bindings/pci/designware-pcie.txt
index 1da7ade..7f9804d 100644
--- a/Documentation/devicetree/bindings/pci/designware-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt
@@ -1,7 +1,8 @@
 * Synopsys DesignWare PCIe interface
 
 Required properties:
-- compatible: should contain "snps,dw-pcie" to identify the core.
+- compatible:
+	"snps,dw-pcie" for RC mode;
 - reg: Should contain the configuration address space.
 - reg-names: Must be "config" for the PCIe configuration space.
     (The old way of getting the configuration address space from "ranges"
@@ -41,11 +42,11 @@ EP mode:
 
 Example configuration:
 
-	pcie: pcie@dffff000 {
+	pcie: pcie@dfc00000 {
 		compatible = "snps,dw-pcie";
-		reg = <0xdffff000 0x1000>, /* Controller registers */
-		      <0xd0000000 0x2000>; /* PCI config space */
-		reg-names = "ctrlreg", "config";
+		reg = <0xdfc00000 0x0001000>, /* IP registers */
+		      <0xd0000000 0x0002000>; /* Configuration space */
+		reg-names = "dbi", "config";
 		#address-cells = <3>;
 		#size-cells = <2>;
 		device_type = "pci";
@@ -54,5 +55,4 @@ Example configuration:
 		interrupts = <25>, <24>;
 		#interrupt-cells = <1>;
 		num-lanes = <1>;
-		num-viewport = <3>;
 	};
-- 
2.7.4

[PATCH v7 2/9] PCI: dwc: Add support for endpoint mode

[Gustavo Pimentel]

The PCIe controller dual mode is capable of operating in host mode as well
as endpoint mode by configuration, therefore this patch aims to add
endpoint mode support to the designware driver.

Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
Acked-by: Kishon Vijay Abraham I <kishon@ti.com>
---
Change v1->v2:
 - Removed dw_plat_pcie_stop_link empty function.
 - Implemented Kishon's suggestions about dw-pcie-rc and dw-pcie strings.
compatibility.
 - Added second entry on pci_epf_test_ids structure.
Changes v2->v3:
 - Reverted additions in dw_pcie_ep_linkup function.
 - Replaced devm_ioremap by devm_ioremap_resource function.
 - Moved second entry in pci_epf_test_ids structure into a new patch file.
Changes v3->v4:
 - Reverted "snps,dw-pcie-rc" compatible string requested by Rob Herring.
Changes v4->v5:
 - Nothing changed, just to follow the patch set version.
Changes v5->v6:
 - Removed PCIE_DW_PLAT entry helps and description in order to be hidden.
Changes v6->v7:
 - Nothing changed, just to follow the patch set version.

 drivers/pci/dwc/Kconfig                |  41 ++++++---
 drivers/pci/dwc/pcie-designware-plat.c | 149 ++++++++++++++++++++++++++++++---
 2 files changed, 169 insertions(+), 21 deletions(-)

diff --git a/drivers/pci/dwc/Kconfig b/drivers/pci/dwc/Kconfig
index 2f3f5c5..62f7cdf 100644
--- a/drivers/pci/dwc/Kconfig
+++ b/drivers/pci/dwc/Kconfig
@@ -7,8 +7,7 @@ config PCIE_DW
 
 config PCIE_DW_HOST
         bool
-	depends on PCI
-	depends on PCI_MSI_IRQ_DOMAIN
+	depends on PCI && PCI_MSI_IRQ_DOMAIN
         select PCIE_DW
 
 config PCIE_DW_EP
@@ -51,17 +50,37 @@ config PCI_DRA7XX_EP
 	  This uses the DesignWare core.
 
 config PCIE_DW_PLAT
-	bool "Platform bus based DesignWare PCIe Controller"
-	depends on PCI
-	depends on PCI_MSI_IRQ_DOMAIN
-	select PCIE_DW_HOST
-	---help---
-	 This selects the DesignWare PCIe controller support. Select this if
-	 you have a PCIe controller on Platform bus.
+	bool
 
-	 If you have a controller with this interface, say Y or M here.
+config PCIE_DW_PLAT_HOST
+	bool "Platform bus based DesignWare PCIe Controller - Host mode"
+	depends on PCI && PCI_MSI_IRQ_DOMAIN
+	select PCIE_DW_HOST
+	select PCIE_DW_PLAT
+	default y
+	help
+	  Enables support for the PCIe controller in the Designware IP to
+	  work in host mode. There are two instances of PCIe controller in
+	  Designware IP.
+	  This controller can work either as EP or RC. In order to enable
+	  host-specific features PCIE_DW_PLAT_HOST must be selected and in
+	  order to enable device-specific features PCI_DW_PLAT_EP must be
+	  selected.
 
-	 If unsure, say N.
+config PCIE_DW_PLAT_EP
+	bool "Platform bus based DesignWare PCIe Controller - Endpoint mode"
+	depends on PCI && PCI_MSI_IRQ_DOMAIN
+	depends on PCI_ENDPOINT
+	select PCIE_DW_EP
+	select PCIE_DW_PLAT
+	help
+	  Enables support for the PCIe controller in the Designware IP to
+	  work in endpoint mode. There are two instances of PCIe controller
+	  in Designware IP.
+	  This controller can work either as EP or RC. In order to enable
+	  host-specific features PCIE_DW_PLAT_HOST must be selected and in
+	  order to enable device-specific features PCI_DW_PLAT_EP must be
+	  selected.
 
 config PCI_EXYNOS
 	bool "Samsung Exynos PCIe controller"
diff --git a/drivers/pci/dwc/pcie-designware-plat.c b/drivers/pci/dwc/pcie-designware-plat.c
index 5416aa8..efc315c 100644
--- a/drivers/pci/dwc/pcie-designware-plat.c
+++ b/drivers/pci/dwc/pcie-designware-plat.c
@@ -12,19 +12,29 @@
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
 #include <linux/init.h>
+#include <linux/of_device.h>
 #include <linux/of_gpio.h>
 #include <linux/pci.h>
 #include <linux/platform_device.h>
 #include <linux/resource.h>
 #include <linux/signal.h>
 #include <linux/types.h>
+#include <linux/regmap.h>
 
 #include "pcie-designware.h"
 
 struct dw_plat_pcie {
-	struct dw_pcie		*pci;
+	struct dw_pcie			*pci;
+	struct regmap			*regmap;
+	enum dw_pcie_device_mode	mode;
 };
 
+struct dw_plat_pcie_of_data {
+	enum dw_pcie_device_mode	mode;
+};
+
+static const struct of_device_id dw_plat_pcie_of_match[];
+
 static int dw_plat_pcie_host_init(struct pcie_port *pp)
 {
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
@@ -42,9 +52,53 @@ static const struct dw_pcie_host_ops dw_plat_pcie_host_ops = {
 	.host_init = dw_plat_pcie_host_init,
 };
 
-static int dw_plat_add_pcie_port(struct pcie_port *pp,
+static int dw_plat_pcie_establish_link(struct dw_pcie *pci)
+{
+	return 0;
+}
+
+static const struct dw_pcie_ops dw_pcie_ops = {
+	.start_link = dw_plat_pcie_establish_link,
+};
+
+static void dw_plat_pcie_ep_init(struct dw_pcie_ep *ep)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
+	enum pci_barno bar;
+
+	for (bar = BAR_0; bar <= BAR_5; bar++)
+		dw_pcie_ep_reset_bar(pci, bar);
+}
+
+static int dw_plat_pcie_ep_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
+				     enum pci_epc_irq_type type,
+				     u8 interrupt_num)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
+
+	switch (type) {
+	case PCI_EPC_IRQ_LEGACY:
+		dev_err(pci->dev, "EP cannot trigger legacy IRQs\n");
+		return -EINVAL;
+	case PCI_EPC_IRQ_MSI:
+		return dw_pcie_ep_raise_msi_irq(ep, func_no, interrupt_num);
+	default:
+		dev_err(pci->dev, "UNKNOWN IRQ type\n");
+	}
+
+	return 0;
+}
+
+static struct dw_pcie_ep_ops pcie_ep_ops = {
+	.ep_init = dw_plat_pcie_ep_init,
+	.raise_irq = dw_plat_pcie_ep_raise_irq,
+};
+
+static int dw_plat_add_pcie_port(struct dw_plat_pcie *dw_plat_pcie,
 				 struct platform_device *pdev)
 {
+	struct dw_pcie *pci = dw_plat_pcie->pci;
+	struct pcie_port *pp = &pci->pp;
 	struct device *dev = &pdev->dev;
 	int ret;
 
@@ -63,15 +117,44 @@ static int dw_plat_add_pcie_port(struct pcie_port *pp,
 
 	ret = dw_pcie_host_init(pp);
 	if (ret) {
-		dev_err(dev, "failed to initialize host\n");
+		dev_err(dev, "Failed to initialize host\n");
 		return ret;
 	}
 
 	return 0;
 }
 
-static const struct dw_pcie_ops dw_pcie_ops = {
-};
+static int dw_plat_add_pcie_ep(struct dw_plat_pcie *dw_plat_pcie,
+			       struct platform_device *pdev)
+{
+	int ret;
+	struct dw_pcie_ep *ep;
+	struct resource *res;
+	struct device *dev = &pdev->dev;
+	struct dw_pcie *pci = dw_plat_pcie->pci;
+
+	ep = &pci->ep;
+	ep->ops = &pcie_ep_ops;
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi2");
+	pci->dbi_base2 = devm_ioremap_resource(dev, res);
+	if (IS_ERR(pci->dbi_base2))
+		return PTR_ERR(pci->dbi_base2);
+
+	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 int dw_plat_pcie_probe(struct platform_device *pdev)
 {
@@ -80,6 +163,16 @@ static int dw_plat_pcie_probe(struct platform_device *pdev)
 	struct dw_pcie *pci;
 	struct resource *res;  /* Resource from DT */
 	int ret;
+	const struct of_device_id *match;
+	const struct dw_plat_pcie_of_data *data;
+	enum dw_pcie_device_mode mode;
+
+	match = of_match_device(dw_plat_pcie_of_match, dev);
+	if (!match)
+		return -EINVAL;
+
+	data = (struct dw_plat_pcie_of_data *)match->data;
+	mode = (enum dw_pcie_device_mode)data->mode;
 
 	dw_plat_pcie = devm_kzalloc(dev, sizeof(*dw_plat_pcie), GFP_KERNEL);
 	if (!dw_plat_pcie)
@@ -93,23 +186,59 @@ static int dw_plat_pcie_probe(struct platform_device *pdev)
 	pci->ops = &dw_pcie_ops;
 
 	dw_plat_pcie->pci = pci;
+	dw_plat_pcie->mode = mode;
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi");
+	if (!res)
+		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	pci->dbi_base = devm_ioremap_resource(dev, res);
 	if (IS_ERR(pci->dbi_base))
 		return PTR_ERR(pci->dbi_base);
 
 	platform_set_drvdata(pdev, dw_plat_pcie);
 
-	ret = dw_plat_add_pcie_port(&pci->pp, pdev);
-	if (ret < 0)
-		return ret;
+	switch (dw_plat_pcie->mode) {
+	case DW_PCIE_RC_TYPE:
+		if (!IS_ENABLED(CONFIG_PCIE_DW_PLAT_HOST))
+			return -ENODEV;
+
+		ret = dw_plat_add_pcie_port(dw_plat_pcie, pdev);
+		if (ret < 0)
+			return ret;
+		break;
+	case DW_PCIE_EP_TYPE:
+		if (!IS_ENABLED(CONFIG_PCIE_DW_PLAT_EP))
+			return -ENODEV;
+
+		ret = dw_plat_add_pcie_ep(dw_plat_pcie, pdev);
+		if (ret < 0)
+			return ret;
+		break;
+	default:
+		dev_err(dev, "INVALID device type %d\n", dw_plat_pcie->mode);
+	}
 
 	return 0;
 }
 
+static const struct dw_plat_pcie_of_data dw_plat_pcie_rc_of_data = {
+	.mode = DW_PCIE_RC_TYPE,
+};
+
+static const struct dw_plat_pcie_of_data dw_plat_pcie_ep_of_data = {
+	.mode = DW_PCIE_EP_TYPE,
+};
+
 static const struct of_device_id dw_plat_pcie_of_match[] = {
-	{ .compatible = "snps,dw-pcie", },
+	{
+		.compatible = "snps,dw-pcie",
+		.data = &dw_plat_pcie_rc_of_data,
+	},
+	{
+		.compatible = "snps,dw-pcie-ep",
+		.data = &dw_plat_pcie_ep_of_data,
+	},
 	{},
 };
 
-- 
2.7.4

[PATCH v7 3/9] PCI: endpoint: functions/pci-epf-test: Add second entry

[Gustavo Pimentel]

Adds a seconds entry on the pci_epf_test_ids structure that disables the
linkup_notifier parameter on driver for the designware EP.

This allows designware EPs that doesn't have linkup notification signal
to work with pcitest.

Updates the binding documentation accordingly.

Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
Acked-by: Kishon Vijay Abraham I <kishon@ti.com>
---
Change v2->v3:
 - Added second entry in pci_epf_test_ids structure.
 - Remove test_reg_bar field assignment on second entry.
Changes v3->v4:
 - Nothing changed, just to follow the patch set version.
Changes v4->v5:
 - Changed pci_epf_test_cfg2 to pci_epf_test_designware.
Changes v5->v6:
 - Changed name field from pci_epf_test_designware to pci_epf_test_dw.
Changes v6->v7:
 - Changed variable name from data_cfg2 to data_linkup_notifier_disabled.

 Documentation/PCI/endpoint/function/binding/pci-test.txt | 2 ++
 drivers/pci/endpoint/functions/pci-epf-test.c            | 8 ++++++++
 2 files changed, 10 insertions(+)

diff --git a/Documentation/PCI/endpoint/function/binding/pci-test.txt b/Documentation/PCI/endpoint/function/binding/pci-test.txt
index 3b68b95..dc39f47 100644
--- a/Documentation/PCI/endpoint/function/binding/pci-test.txt
+++ b/Documentation/PCI/endpoint/function/binding/pci-test.txt
@@ -1,6 +1,8 @@
 PCI TEST ENDPOINT FUNCTION
 
 name: Should be "pci_epf_test" to bind to the pci_epf_test driver.
+name: Should be "pci_epf_test_dw" to bind to the pci_epf_test driver
+      with a custom configuration for the designware EP.
 
 Configurable Fields:
 vendorid	 : should be 0x104c
diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
index 7cef851..4ab463b 100644
--- a/drivers/pci/endpoint/functions/pci-epf-test.c
+++ b/drivers/pci/endpoint/functions/pci-epf-test.c
@@ -459,10 +459,18 @@ static int pci_epf_test_bind(struct pci_epf *epf)
 	return 0;
 }
 
+static const struct pci_epf_test_data data_linkup_notifier_disabled = {
+	.linkup_notifier = false
+};
+
 static const struct pci_epf_device_id pci_epf_test_ids[] = {
 	{
 		.name = "pci_epf_test",
 	},
+	{
+		.name = "pci_epf_test_dw",
+		.driver_data = (kernel_ulong_t)&data_linkup_notifier_disabled,
+	},
 	{},
 };
 
-- 
2.7.4

Re: [PATCH v7 3/9] PCI: endpoint: functions/pci-epf-test: Add second entry

[Lorenzo Pieralisi]

On Tue, Apr 24, 2018 at 02:44:40PM +0100, Gustavo Pimentel wrote:
> Adds a seconds entry on the pci_epf_test_ids structure that disables the

"Add a second entry to..."

> linkup_notifier parameter on driver for the designware EP.
> 
> This allows designware EPs that doesn't have linkup notification signal
> to work with pcitest.
> 
> Updates the binding documentation accordingly.

Valid for all the series: use imperative sentences.

eg:

"Update the binding documentation accordingly".

not

"Updates the binding documentation accordingly".

> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> Acked-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
> Change v2->v3:
>  - Added second entry in pci_epf_test_ids structure.
>  - Remove test_reg_bar field assignment on second entry.
> Changes v3->v4:
>  - Nothing changed, just to follow the patch set version.
> Changes v4->v5:
>  - Changed pci_epf_test_cfg2 to pci_epf_test_designware.
> Changes v5->v6:
>  - Changed name field from pci_epf_test_designware to pci_epf_test_dw.
> Changes v6->v7:
>  - Changed variable name from data_cfg2 to data_linkup_notifier_disabled.
> 
>  Documentation/PCI/endpoint/function/binding/pci-test.txt | 2 ++
>  drivers/pci/endpoint/functions/pci-epf-test.c            | 8 ++++++++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/Documentation/PCI/endpoint/function/binding/pci-test.txt b/Documentation/PCI/endpoint/function/binding/pci-test.txt
> index 3b68b95..dc39f47 100644
> --- a/Documentation/PCI/endpoint/function/binding/pci-test.txt
> +++ b/Documentation/PCI/endpoint/function/binding/pci-test.txt
> @@ -1,6 +1,8 @@
>  PCI TEST ENDPOINT FUNCTION
>  
>  name: Should be "pci_epf_test" to bind to the pci_epf_test driver.
> +name: Should be "pci_epf_test_dw" to bind to the pci_epf_test driver
> +      with a custom configuration for the designware EP.

The link between the "name" and the device created is quite obscure and
reading pci-test-howto.txt certainly does not clarify it.

In pci-test-howto.txt an explanation should be added to the configs
device creation paragraph to clarify it.

>  Configurable Fields:
>  vendorid	 : should be 0x104c
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index 7cef851..4ab463b 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -459,10 +459,18 @@ static int pci_epf_test_bind(struct pci_epf *epf)
>  	return 0;
>  }
>  
> +static const struct pci_epf_test_data data_linkup_notifier_disabled = {
> +	.linkup_notifier = false
> +};
> +
>  static const struct pci_epf_device_id pci_epf_test_ids[] = {
>  	{
>  		.name = "pci_epf_test",
>  	},
> +	{
> +		.name = "pci_epf_test_dw",
> +		.driver_data = (kernel_ulong_t)&data_linkup_notifier_disabled,
> +	},
>  	{},

Should not this be a property derived from the controller compatible
property instead of the test device name written in configfs ?

I think I am missing something in the whole scheme of things, please
clarify.

Thanks,
Lorenzo

Re: [PATCH v7 3/9] PCI: endpoint: functions/pci-epf-test: Add second entry

[Gustavo Pimentel]

On 26/04/2018 17:56, Lorenzo Pieralisi wrote:
> On Tue, Apr 24, 2018 at 02:44:40PM +0100, Gustavo Pimentel wrote:
>> Adds a seconds entry on the pci_epf_test_ids structure that disables the
> 
> "Add a second entry to..."
> 
>> linkup_notifier parameter on driver for the designware EP.
>>
>> This allows designware EPs that doesn't have linkup notification signal
>> to work with pcitest.
>>
>> Updates the binding documentation accordingly.
> 
> Valid for all the series: use imperative sentences.
> 
> eg:
> 
> "Update the binding documentation accordingly".
> 
> not
> 
> "Updates the binding documentation accordingly".
> 

Ok, I will do an overall check to fix the entries.

>> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
>> Acked-by: Kishon Vijay Abraham I <kishon@ti.com>
>> ---
>> Change v2->v3:
>>  - Added second entry in pci_epf_test_ids structure.
>>  - Remove test_reg_bar field assignment on second entry.
>> Changes v3->v4:
>>  - Nothing changed, just to follow the patch set version.
>> Changes v4->v5:
>>  - Changed pci_epf_test_cfg2 to pci_epf_test_designware.
>> Changes v5->v6:
>>  - Changed name field from pci_epf_test_designware to pci_epf_test_dw.
>> Changes v6->v7:
>>  - Changed variable name from data_cfg2 to data_linkup_notifier_disabled.
>>
>>  Documentation/PCI/endpoint/function/binding/pci-test.txt | 2 ++
>>  drivers/pci/endpoint/functions/pci-epf-test.c            | 8 ++++++++
>>  2 files changed, 10 insertions(+)
>>
>> diff --git a/Documentation/PCI/endpoint/function/binding/pci-test.txt b/Documentation/PCI/endpoint/function/binding/pci-test.txt
>> index 3b68b95..dc39f47 100644
>> --- a/Documentation/PCI/endpoint/function/binding/pci-test.txt
>> +++ b/Documentation/PCI/endpoint/function/binding/pci-test.txt
>> @@ -1,6 +1,8 @@
>>  PCI TEST ENDPOINT FUNCTION
>>  
>>  name: Should be "pci_epf_test" to bind to the pci_epf_test driver.
>> +name: Should be "pci_epf_test_dw" to bind to the pci_epf_test driver
>> +      with a custom configuration for the designware EP.
> 
> The link between the "name" and the device created is quite obscure and
> reading pci-test-howto.txt certainly does not clarify it.

I will add more information about it.

> 
> In pci-test-howto.txt an explanation should be added to the configs
> device creation paragraph to clarify it.

That's true, it should exist some explanation of that. To be honest I didn't
remember of the pci-test-howto.txt file existence.

> 
>>  Configurable Fields:
>>  vendorid	 : should be 0x104c
>> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
>> index 7cef851..4ab463b 100644
>> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
>> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
>> @@ -459,10 +459,18 @@ static int pci_epf_test_bind(struct pci_epf *epf)
>>  	return 0;
>>  }
>>  
>> +static const struct pci_epf_test_data data_linkup_notifier_disabled = {
>> +	.linkup_notifier = false
>> +};
>> +
>>  static const struct pci_epf_device_id pci_epf_test_ids[] = {
>>  	{
>>  		.name = "pci_epf_test",
>>  	},
>> +	{
>> +		.name = "pci_epf_test_dw",
>> +		.driver_data = (kernel_ulong_t)&data_linkup_notifier_disabled,
>> +	},
>>  	{},
> 
> Should not this be a property derived from the controller compatible
> property instead of the test device name written in configfs ?
> 
> I think I am missing something in the whole scheme of things, please
> clarify.

This type of configuration / configuration was suggested by Kishon. I can only
assume that it would not be possible (or no one thought of it) to correlate the
compatible string of the controller to select the configuration, perhaps Kishon
could give his opinion on the feasibility of this and even to provide some
example of it. :)

If it is possible, it will be quite straightforward.

> 
> Thanks,
> Lorenzo
> 

Re: [PATCH v7 3/9] PCI: endpoint: functions/pci-epf-test: Add second entry

[Kishon Vijay Abraham I]

Hi Lorenzo,

On Thursday 26 April 2018 10:26 PM, Lorenzo Pieralisi wrote:
> On Tue, Apr 24, 2018 at 02:44:40PM +0100, Gustavo Pimentel wrote:
>> Adds a seconds entry on the pci_epf_test_ids structure that disables the
> 
> "Add a second entry to..."
> 
>> linkup_notifier parameter on driver for the designware EP.
>>
>> This allows designware EPs that doesn't have linkup notification signal
>> to work with pcitest.
>>
>> Updates the binding documentation accordingly.
> 
> Valid for all the series: use imperative sentences.
> 
> eg:
> 
> "Update the binding documentation accordingly".
> 
> not
> 
> "Updates the binding documentation accordingly".
> 
>> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
>> Acked-by: Kishon Vijay Abraham I <kishon@ti.com>
>> ---
>> Change v2->v3:
>>  - Added second entry in pci_epf_test_ids structure.
>>  - Remove test_reg_bar field assignment on second entry.
>> Changes v3->v4:
>>  - Nothing changed, just to follow the patch set version.
>> Changes v4->v5:
>>  - Changed pci_epf_test_cfg2 to pci_epf_test_designware.
>> Changes v5->v6:
>>  - Changed name field from pci_epf_test_designware to pci_epf_test_dw.
>> Changes v6->v7:
>>  - Changed variable name from data_cfg2 to data_linkup_notifier_disabled.
>>
>>  Documentation/PCI/endpoint/function/binding/pci-test.txt | 2 ++
>>  drivers/pci/endpoint/functions/pci-epf-test.c            | 8 ++++++++
>>  2 files changed, 10 insertions(+)
>>
>> diff --git a/Documentation/PCI/endpoint/function/binding/pci-test.txt b/Documentation/PCI/endpoint/function/binding/pci-test.txt
>> index 3b68b95..dc39f47 100644
>> --- a/Documentation/PCI/endpoint/function/binding/pci-test.txt
>> +++ b/Documentation/PCI/endpoint/function/binding/pci-test.txt
>> @@ -1,6 +1,8 @@
>>  PCI TEST ENDPOINT FUNCTION
>>  
>>  name: Should be "pci_epf_test" to bind to the pci_epf_test driver.
>> +name: Should be "pci_epf_test_dw" to bind to the pci_epf_test driver
>> +      with a custom configuration for the designware EP.
> 
> The link between the "name" and the device created is quite obscure and
> reading pci-test-howto.txt certainly does not clarify it.
> 
> In pci-test-howto.txt an explanation should be added to the configs
> device creation paragraph to clarify it.
> 
>>  Configurable Fields:
>>  vendorid	 : should be 0x104c
>> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
>> index 7cef851..4ab463b 100644
>> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
>> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
>> @@ -459,10 +459,18 @@ static int pci_epf_test_bind(struct pci_epf *epf)
>>  	return 0;
>>  }
>>  
>> +static const struct pci_epf_test_data data_linkup_notifier_disabled = {
>> +	.linkup_notifier = false
>> +};
>> +
>>  static const struct pci_epf_device_id pci_epf_test_ids[] = {
>>  	{
>>  		.name = "pci_epf_test",
>>  	},
>> +	{
>> +		.name = "pci_epf_test_dw",
>> +		.driver_data = (kernel_ulong_t)&data_linkup_notifier_disabled,
>> +	},
>>  	{},
> 
> Should not this be a property derived from the controller compatible
> property instead of the test device name written in configfs ?

pci_epf_test is an independent driver on its own that operates in a layer above
the controller driver. So it does not get the controller compatible (which is
used in controller drivers like pcie-designware-plat.c). pci_epf_test uses
"pci_epf_device_id" which is _similar_ to "of_device_id" used by platform drivers.

Thanks
Kishon

Re: [PATCH v7 3/9] PCI: endpoint: functions/pci-epf-test: Add second entry

[Kishon Vijay Abraham I]

Hi Lorenzo,

On Thursday 26 April 2018 10:26 PM, Lorenzo Pieralisi wrote:
> On Tue, Apr 24, 2018 at 02:44:40PM +0100, Gustavo Pimentel wrote:
>> Adds a seconds entry on the pci_epf_test_ids structure that disables the
> 
> "Add a second entry to..."
> 
>> linkup_notifier parameter on driver for the designware EP.
>>
>> This allows designware EPs that doesn't have linkup notification signal
>> to work with pcitest.
>>
>> Updates the binding documentation accordingly.
> 
> Valid for all the series: use imperative sentences.
> 
> eg:
> 
> "Update the binding documentation accordingly".
> 
> not
> 
> "Updates the binding documentation accordingly".
> 
>> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
>> Acked-by: Kishon Vijay Abraham I <kishon@ti.com>
>> ---
>> Change v2->v3:
>>  - Added second entry in pci_epf_test_ids structure.
>>  - Remove test_reg_bar field assignment on second entry.
>> Changes v3->v4:
>>  - Nothing changed, just to follow the patch set version.
>> Changes v4->v5:
>>  - Changed pci_epf_test_cfg2 to pci_epf_test_designware.
>> Changes v5->v6:
>>  - Changed name field from pci_epf_test_designware to pci_epf_test_dw.
>> Changes v6->v7:
>>  - Changed variable name from data_cfg2 to data_linkup_notifier_disabled.
>>
>>  Documentation/PCI/endpoint/function/binding/pci-test.txt | 2 ++
>>  drivers/pci/endpoint/functions/pci-epf-test.c            | 8 ++++++++
>>  2 files changed, 10 insertions(+)
>>
>> diff --git a/Documentation/PCI/endpoint/function/binding/pci-test.txt b/Documentation/PCI/endpoint/function/binding/pci-test.txt
>> index 3b68b95..dc39f47 100644
>> --- a/Documentation/PCI/endpoint/function/binding/pci-test.txt
>> +++ b/Documentation/PCI/endpoint/function/binding/pci-test.txt
>> @@ -1,6 +1,8 @@
>>  PCI TEST ENDPOINT FUNCTION
>>  
>>  name: Should be "pci_epf_test" to bind to the pci_epf_test driver.
>> +name: Should be "pci_epf_test_dw" to bind to the pci_epf_test driver
>> +      with a custom configuration for the designware EP.
> 
> The link between the "name" and the device created is quite obscure and
> reading pci-test-howto.txt certainly does not clarify it.
> 
> In pci-test-howto.txt an explanation should be added to the configs
> device creation paragraph to clarify it.
> 
>>  Configurable Fields:
>>  vendorid	 : should be 0x104c
>> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
>> index 7cef851..4ab463b 100644
>> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
>> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
>> @@ -459,10 +459,18 @@ static int pci_epf_test_bind(struct pci_epf *epf)
>>  	return 0;
>>  }
>>  
>> +static const struct pci_epf_test_data data_linkup_notifier_disabled = {
>> +	.linkup_notifier = false
>> +};
>> +
>>  static const struct pci_epf_device_id pci_epf_test_ids[] = {
>>  	{
>>  		.name = "pci_epf_test",
>>  	},
>> +	{
>> +		.name = "pci_epf_test_dw",
>> +		.driver_data = (kernel_ulong_t)&data_linkup_notifier_disabled,
>> +	},
>>  	{},
> 
> Should not this be a property derived from the controller compatible
> property instead of the test device name written in configfs ?

pci_epf_test is an independent driver on its own that operates in a layer above
the controller driver. So it does not get the controller compatible (which is
used in controller drivers like pcie-designware-plat.c). pci_epf_test uses
"pci_epf_device_id" which is _similar_ to "of_device_id" used by platform drivers.

Thanks
Kishon

Re: [PATCH v7 3/9] PCI: endpoint: functions/pci-epf-test: Add second entry

[Lorenzo Pieralisi]

On Tue, May 01, 2018 at 03:37:47PM +0530, Kishon Vijay Abraham I wrote:
> Hi Lorenzo,
> 
> On Thursday 26 April 2018 10:26 PM, Lorenzo Pieralisi wrote:
> > On Tue, Apr 24, 2018 at 02:44:40PM +0100, Gustavo Pimentel wrote:
> >> Adds a seconds entry on the pci_epf_test_ids structure that disables the
> > 
> > "Add a second entry to..."
> > 
> >> linkup_notifier parameter on driver for the designware EP.
> >>
> >> This allows designware EPs that doesn't have linkup notification signal
> >> to work with pcitest.
> >>
> >> Updates the binding documentation accordingly.
> > 
> > Valid for all the series: use imperative sentences.
> > 
> > eg:
> > 
> > "Update the binding documentation accordingly".
> > 
> > not
> > 
> > "Updates the binding documentation accordingly".
> > 
> >> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> >> Acked-by: Kishon Vijay Abraham I <kishon@ti.com>
> >> ---
> >> Change v2->v3:
> >>  - Added second entry in pci_epf_test_ids structure.
> >>  - Remove test_reg_bar field assignment on second entry.
> >> Changes v3->v4:
> >>  - Nothing changed, just to follow the patch set version.
> >> Changes v4->v5:
> >>  - Changed pci_epf_test_cfg2 to pci_epf_test_designware.
> >> Changes v5->v6:
> >>  - Changed name field from pci_epf_test_designware to pci_epf_test_dw.
> >> Changes v6->v7:
> >>  - Changed variable name from data_cfg2 to data_linkup_notifier_disabled.
> >>
> >>  Documentation/PCI/endpoint/function/binding/pci-test.txt | 2 ++
> >>  drivers/pci/endpoint/functions/pci-epf-test.c            | 8 ++++++++
> >>  2 files changed, 10 insertions(+)
> >>
> >> diff --git a/Documentation/PCI/endpoint/function/binding/pci-test.txt b/Documentation/PCI/endpoint/function/binding/pci-test.txt
> >> index 3b68b95..dc39f47 100644
> >> --- a/Documentation/PCI/endpoint/function/binding/pci-test.txt
> >> +++ b/Documentation/PCI/endpoint/function/binding/pci-test.txt
> >> @@ -1,6 +1,8 @@
> >>  PCI TEST ENDPOINT FUNCTION
> >>  
> >>  name: Should be "pci_epf_test" to bind to the pci_epf_test driver.
> >> +name: Should be "pci_epf_test_dw" to bind to the pci_epf_test driver
> >> +      with a custom configuration for the designware EP.
> > 
> > The link between the "name" and the device created is quite obscure and
> > reading pci-test-howto.txt certainly does not clarify it.
> > 
> > In pci-test-howto.txt an explanation should be added to the configs
> > device creation paragraph to clarify it.
> > 
> >>  Configurable Fields:
> >>  vendorid	 : should be 0x104c
> >> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> >> index 7cef851..4ab463b 100644
> >> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> >> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> >> @@ -459,10 +459,18 @@ static int pci_epf_test_bind(struct pci_epf *epf)
> >>  	return 0;
> >>  }
> >>  
> >> +static const struct pci_epf_test_data data_linkup_notifier_disabled = {
> >> +	.linkup_notifier = false
> >> +};
> >> +
> >>  static const struct pci_epf_device_id pci_epf_test_ids[] = {
> >>  	{
> >>  		.name = "pci_epf_test",
> >>  	},
> >> +	{
> >> +		.name = "pci_epf_test_dw",
> >> +		.driver_data = (kernel_ulong_t)&data_linkup_notifier_disabled,
> >> +	},
> >>  	{},
> > 
> > Should not this be a property derived from the controller compatible
> > property instead of the test device name written in configfs ?
> 
> pci_epf_test is an independent driver on its own that operates in a layer above
> the controller driver. So it does not get the controller compatible (which is
> used in controller drivers like pcie-designware-plat.c). pci_epf_test uses
> "pci_epf_device_id" which is _similar_ to "of_device_id" used by platform drivers.

I understand that, the problem is that the independent driver depends on
features of the related controller driver as this patch shows. This
patch basically says that if you use a specific path in configfs (that
includes pci_epf_test_dw) your device has specific HW features (eg
linkup notifier above), that obviously depends on the platform HW not on
the string you use in configfs.

What I am questioning is a) if I understand this right and b) whether
this is the right approach.

Lorenzo

Re: [PATCH v7 3/9] PCI: endpoint: functions/pci-epf-test: Add second entry

[Kishon Vijay Abraham I]

Hi Lorenzo,

On Tuesday 01 May 2018 05:24 PM, Lorenzo Pieralisi wrote:
> On Tue, May 01, 2018 at 03:37:47PM +0530, Kishon Vijay Abraham I wrote:
>> Hi Lorenzo,
>>
>> On Thursday 26 April 2018 10:26 PM, Lorenzo Pieralisi wrote:
>>> On Tue, Apr 24, 2018 at 02:44:40PM +0100, Gustavo Pimentel wrote:
>>>> Adds a seconds entry on the pci_epf_test_ids structure that disables the
>>>
>>> "Add a second entry to..."
>>>
>>>> linkup_notifier parameter on driver for the designware EP.
>>>>
>>>> This allows designware EPs that doesn't have linkup notification signal
>>>> to work with pcitest.
>>>>
>>>> Updates the binding documentation accordingly.
>>>
>>> Valid for all the series: use imperative sentences.
>>>
>>> eg:
>>>
>>> "Update the binding documentation accordingly".
>>>
>>> not
>>>
>>> "Updates the binding documentation accordingly".
>>>
>>>> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
>>>> Acked-by: Kishon Vijay Abraham I <kishon@ti.com>
>>>> ---
>>>> Change v2->v3:
>>>>  - Added second entry in pci_epf_test_ids structure.
>>>>  - Remove test_reg_bar field assignment on second entry.
>>>> Changes v3->v4:
>>>>  - Nothing changed, just to follow the patch set version.
>>>> Changes v4->v5:
>>>>  - Changed pci_epf_test_cfg2 to pci_epf_test_designware.
>>>> Changes v5->v6:
>>>>  - Changed name field from pci_epf_test_designware to pci_epf_test_dw.
>>>> Changes v6->v7:
>>>>  - Changed variable name from data_cfg2 to data_linkup_notifier_disabled.
>>>>
>>>>  Documentation/PCI/endpoint/function/binding/pci-test.txt | 2 ++
>>>>  drivers/pci/endpoint/functions/pci-epf-test.c            | 8 ++++++++
>>>>  2 files changed, 10 insertions(+)
>>>>
>>>> diff --git a/Documentation/PCI/endpoint/function/binding/pci-test.txt b/Documentation/PCI/endpoint/function/binding/pci-test.txt
>>>> index 3b68b95..dc39f47 100644
>>>> --- a/Documentation/PCI/endpoint/function/binding/pci-test.txt
>>>> +++ b/Documentation/PCI/endpoint/function/binding/pci-test.txt
>>>> @@ -1,6 +1,8 @@
>>>>  PCI TEST ENDPOINT FUNCTION
>>>>  
>>>>  name: Should be "pci_epf_test" to bind to the pci_epf_test driver.
>>>> +name: Should be "pci_epf_test_dw" to bind to the pci_epf_test driver
>>>> +      with a custom configuration for the designware EP.
>>>
>>> The link between the "name" and the device created is quite obscure and
>>> reading pci-test-howto.txt certainly does not clarify it.
>>>
>>> In pci-test-howto.txt an explanation should be added to the configs
>>> device creation paragraph to clarify it.
>>>
>>>>  Configurable Fields:
>>>>  vendorid	 : should be 0x104c
>>>> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
>>>> index 7cef851..4ab463b 100644
>>>> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
>>>> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
>>>> @@ -459,10 +459,18 @@ static int pci_epf_test_bind(struct pci_epf *epf)
>>>>  	return 0;
>>>>  }
>>>>  
>>>> +static const struct pci_epf_test_data data_linkup_notifier_disabled = {
>>>> +	.linkup_notifier = false
>>>> +};
>>>> +
>>>>  static const struct pci_epf_device_id pci_epf_test_ids[] = {
>>>>  	{
>>>>  		.name = "pci_epf_test",
>>>>  	},
>>>> +	{
>>>> +		.name = "pci_epf_test_dw",
>>>> +		.driver_data = (kernel_ulong_t)&data_linkup_notifier_disabled,
>>>> +	},
>>>>  	{},
>>>
>>> Should not this be a property derived from the controller compatible
>>> property instead of the test device name written in configfs ?
>>
>> pci_epf_test is an independent driver on its own that operates in a layer above
>> the controller driver. So it does not get the controller compatible (which is
>> used in controller drivers like pcie-designware-plat.c). pci_epf_test uses
>> "pci_epf_device_id" which is _similar_ to "of_device_id" used by platform drivers.
> 
> I understand that, the problem is that the independent driver depends on
> features of the related controller driver as this patch shows. This
> patch basically says that if you use a specific path in configfs (that
> includes pci_epf_test_dw) your device has specific HW features (eg
> linkup notifier above), that obviously depends on the platform HW not on
> the string you use in configfs.
> 
> What I am questioning is a) if I understand this right and b) whether
> this is the right approach.

Your understanding is right. Ideally pci-epf-test driver shouldn't have any HW
specific configuration. But different HW have different configurations and
pci-epf-test should be informed of the configuration the HW supports.

configfs is just one way of creating epf_device and it was mainly added since
pci-epf-test cannot have a dt entry because it doesn't represent anything in
the HW.

The other option was to have a callback to EPC driver to get the features it
supports. But a particular feature that is required might be specific to a EPF
driver.

I find the driver_data approach in pci_epf_device_id to be more clean.

Thanks
Kishon

Re: [PATCH v7 3/9] PCI: endpoint: functions/pci-epf-test: Add second entry

[Kishon Vijay Abraham I]

Hi Lorenzo,

On Tuesday 01 May 2018 05:24 PM, Lorenzo Pieralisi wrote:
> On Tue, May 01, 2018 at 03:37:47PM +0530, Kishon Vijay Abraham I wrote:
>> Hi Lorenzo,
>>
>> On Thursday 26 April 2018 10:26 PM, Lorenzo Pieralisi wrote:
>>> On Tue, Apr 24, 2018 at 02:44:40PM +0100, Gustavo Pimentel wrote:
>>>> Adds a seconds entry on the pci_epf_test_ids structure that disables the
>>>
>>> "Add a second entry to..."
>>>
>>>> linkup_notifier parameter on driver for the designware EP.
>>>>
>>>> This allows designware EPs that doesn't have linkup notification signal
>>>> to work with pcitest.
>>>>
>>>> Updates the binding documentation accordingly.
>>>
>>> Valid for all the series: use imperative sentences.
>>>
>>> eg:
>>>
>>> "Update the binding documentation accordingly".
>>>
>>> not
>>>
>>> "Updates the binding documentation accordingly".
>>>
>>>> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
>>>> Acked-by: Kishon Vijay Abraham I <kishon@ti.com>
>>>> ---
>>>> Change v2->v3:
>>>>  - Added second entry in pci_epf_test_ids structure.
>>>>  - Remove test_reg_bar field assignment on second entry.
>>>> Changes v3->v4:
>>>>  - Nothing changed, just to follow the patch set version.
>>>> Changes v4->v5:
>>>>  - Changed pci_epf_test_cfg2 to pci_epf_test_designware.
>>>> Changes v5->v6:
>>>>  - Changed name field from pci_epf_test_designware to pci_epf_test_dw.
>>>> Changes v6->v7:
>>>>  - Changed variable name from data_cfg2 to data_linkup_notifier_disabled.
>>>>
>>>>  Documentation/PCI/endpoint/function/binding/pci-test.txt | 2 ++
>>>>  drivers/pci/endpoint/functions/pci-epf-test.c            | 8 ++++++++
>>>>  2 files changed, 10 insertions(+)
>>>>
>>>> diff --git a/Documentation/PCI/endpoint/function/binding/pci-test.txt b/Documentation/PCI/endpoint/function/binding/pci-test.txt
>>>> index 3b68b95..dc39f47 100644
>>>> --- a/Documentation/PCI/endpoint/function/binding/pci-test.txt
>>>> +++ b/Documentation/PCI/endpoint/function/binding/pci-test.txt
>>>> @@ -1,6 +1,8 @@
>>>>  PCI TEST ENDPOINT FUNCTION
>>>>  
>>>>  name: Should be "pci_epf_test" to bind to the pci_epf_test driver.
>>>> +name: Should be "pci_epf_test_dw" to bind to the pci_epf_test driver
>>>> +      with a custom configuration for the designware EP.
>>>
>>> The link between the "name" and the device created is quite obscure and
>>> reading pci-test-howto.txt certainly does not clarify it.
>>>
>>> In pci-test-howto.txt an explanation should be added to the configs
>>> device creation paragraph to clarify it.
>>>
>>>>  Configurable Fields:
>>>>  vendorid	 : should be 0x104c
>>>> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
>>>> index 7cef851..4ab463b 100644
>>>> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
>>>> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
>>>> @@ -459,10 +459,18 @@ static int pci_epf_test_bind(struct pci_epf *epf)
>>>>  	return 0;
>>>>  }
>>>>  
>>>> +static const struct pci_epf_test_data data_linkup_notifier_disabled = {
>>>> +	.linkup_notifier = false
>>>> +};
>>>> +
>>>>  static const struct pci_epf_device_id pci_epf_test_ids[] = {
>>>>  	{
>>>>  		.name = "pci_epf_test",
>>>>  	},
>>>> +	{
>>>> +		.name = "pci_epf_test_dw",
>>>> +		.driver_data = (kernel_ulong_t)&data_linkup_notifier_disabled,
>>>> +	},
>>>>  	{},
>>>
>>> Should not this be a property derived from the controller compatible
>>> property instead of the test device name written in configfs ?
>>
>> pci_epf_test is an independent driver on its own that operates in a layer above
>> the controller driver. So it does not get the controller compatible (which is
>> used in controller drivers like pcie-designware-plat.c). pci_epf_test uses
>> "pci_epf_device_id" which is _similar_ to "of_device_id" used by platform drivers.
> 
> I understand that, the problem is that the independent driver depends on
> features of the related controller driver as this patch shows. This
> patch basically says that if you use a specific path in configfs (that
> includes pci_epf_test_dw) your device has specific HW features (eg
> linkup notifier above), that obviously depends on the platform HW not on
> the string you use in configfs.
> 
> What I am questioning is a) if I understand this right and b) whether
> this is the right approach.

Your understanding is right. Ideally pci-epf-test driver shouldn't have any HW
specific configuration. But different HW have different configurations and
pci-epf-test should be informed of the configuration the HW supports.

configfs is just one way of creating epf_device and it was mainly added since
pci-epf-test cannot have a dt entry because it doesn't represent anything in
the HW.

The other option was to have a callback to EPC driver to get the features it
supports. But a particular feature that is required might be specific to a EPF
driver.

I find the driver_data approach in pci_epf_device_id to be more clean.

Thanks
Kishon

Re: [PATCH v7 3/9] PCI: endpoint: functions/pci-epf-test: Add second entry

[Lorenzo Pieralisi]

On Tue, May 01, 2018 at 05:53:59PM +0530, Kishon Vijay Abraham I wrote:
> Hi Lorenzo,
> 
> On Tuesday 01 May 2018 05:24 PM, Lorenzo Pieralisi wrote:
> > On Tue, May 01, 2018 at 03:37:47PM +0530, Kishon Vijay Abraham I wrote:
> >> Hi Lorenzo,
> >>
> >> On Thursday 26 April 2018 10:26 PM, Lorenzo Pieralisi wrote:
> >>> On Tue, Apr 24, 2018 at 02:44:40PM +0100, Gustavo Pimentel wrote:
> >>>> Adds a seconds entry on the pci_epf_test_ids structure that disables the
> >>>
> >>> "Add a second entry to..."
> >>>
> >>>> linkup_notifier parameter on driver for the designware EP.
> >>>>
> >>>> This allows designware EPs that doesn't have linkup notification signal
> >>>> to work with pcitest.
> >>>>
> >>>> Updates the binding documentation accordingly.
> >>>
> >>> Valid for all the series: use imperative sentences.
> >>>
> >>> eg:
> >>>
> >>> "Update the binding documentation accordingly".
> >>>
> >>> not
> >>>
> >>> "Updates the binding documentation accordingly".
> >>>
> >>>> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> >>>> Acked-by: Kishon Vijay Abraham I <kishon@ti.com>
> >>>> ---
> >>>> Change v2->v3:
> >>>>  - Added second entry in pci_epf_test_ids structure.
> >>>>  - Remove test_reg_bar field assignment on second entry.
> >>>> Changes v3->v4:
> >>>>  - Nothing changed, just to follow the patch set version.
> >>>> Changes v4->v5:
> >>>>  - Changed pci_epf_test_cfg2 to pci_epf_test_designware.
> >>>> Changes v5->v6:
> >>>>  - Changed name field from pci_epf_test_designware to pci_epf_test_dw.
> >>>> Changes v6->v7:
> >>>>  - Changed variable name from data_cfg2 to data_linkup_notifier_disabled.
> >>>>
> >>>>  Documentation/PCI/endpoint/function/binding/pci-test.txt | 2 ++
> >>>>  drivers/pci/endpoint/functions/pci-epf-test.c            | 8 ++++++++
> >>>>  2 files changed, 10 insertions(+)
> >>>>
> >>>> diff --git a/Documentation/PCI/endpoint/function/binding/pci-test.txt b/Documentation/PCI/endpoint/function/binding/pci-test.txt
> >>>> index 3b68b95..dc39f47 100644
> >>>> --- a/Documentation/PCI/endpoint/function/binding/pci-test.txt
> >>>> +++ b/Documentation/PCI/endpoint/function/binding/pci-test.txt
> >>>> @@ -1,6 +1,8 @@
> >>>>  PCI TEST ENDPOINT FUNCTION
> >>>>  
> >>>>  name: Should be "pci_epf_test" to bind to the pci_epf_test driver.
> >>>> +name: Should be "pci_epf_test_dw" to bind to the pci_epf_test driver
> >>>> +      with a custom configuration for the designware EP.
> >>>
> >>> The link between the "name" and the device created is quite obscure and
> >>> reading pci-test-howto.txt certainly does not clarify it.
> >>>
> >>> In pci-test-howto.txt an explanation should be added to the configs
> >>> device creation paragraph to clarify it.
> >>>
> >>>>  Configurable Fields:
> >>>>  vendorid	 : should be 0x104c
> >>>> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> >>>> index 7cef851..4ab463b 100644
> >>>> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> >>>> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> >>>> @@ -459,10 +459,18 @@ static int pci_epf_test_bind(struct pci_epf *epf)
> >>>>  	return 0;
> >>>>  }
> >>>>  
> >>>> +static const struct pci_epf_test_data data_linkup_notifier_disabled = {
> >>>> +	.linkup_notifier = false
> >>>> +};
> >>>> +
> >>>>  static const struct pci_epf_device_id pci_epf_test_ids[] = {
> >>>>  	{
> >>>>  		.name = "pci_epf_test",
> >>>>  	},
> >>>> +	{
> >>>> +		.name = "pci_epf_test_dw",
> >>>> +		.driver_data = (kernel_ulong_t)&data_linkup_notifier_disabled,
> >>>> +	},
> >>>>  	{},
> >>>
> >>> Should not this be a property derived from the controller compatible
> >>> property instead of the test device name written in configfs ?
> >>
> >> pci_epf_test is an independent driver on its own that operates in a layer above
> >> the controller driver. So it does not get the controller compatible (which is
> >> used in controller drivers like pcie-designware-plat.c). pci_epf_test uses
> >> "pci_epf_device_id" which is _similar_ to "of_device_id" used by platform drivers.
> > 
> > I understand that, the problem is that the independent driver depends on
> > features of the related controller driver as this patch shows. This
> > patch basically says that if you use a specific path in configfs (that
> > includes pci_epf_test_dw) your device has specific HW features (eg
> > linkup notifier above), that obviously depends on the platform HW not on
> > the string you use in configfs.
> > 
> > What I am questioning is a) if I understand this right and b) whether
> > this is the right approach.
> 
> Your understanding is right. Ideally pci-epf-test driver shouldn't have any HW
> specific configuration. But different HW have different configurations and
> pci-epf-test should be informed of the configuration the HW supports.

I am honestly very confused. First off, I do not understand what this
patch really does (or better what DW can't do so that it needs a
specific configfs string and therefore a specific configuration).

What's the purpose of the linkup notifier ? What does it mean that
the DW HW can't handle it ?

Are we referring to the pci_epf_linkup() function ?

If it is a HW configuration (ie the DW HW does not have a signal to
report that the link is up ?) its enablement must depend on HW
controller properties not configfs entries, I do not like what this
patch does (probably because I am confused and I do not understand it).

Please let me know your thoughts on this, thanks.

Lorenzo

> 
> configfs is just one way of creating epf_device and it was mainly added since
> pci-epf-test cannot have a dt entry because it doesn't represent anything in
> the HW.
> 
> The other option was to have a callback to EPC driver to get the features it
> supports. But a particular feature that is required might be specific to a EPF
> driver.
> 
> I find the driver_data approach in pci_epf_device_id to be more clean.
> 
> Thanks
> Kishon

Re: [PATCH v7 3/9] PCI: endpoint: functions/pci-epf-test: Add second entry

[Gustavo Pimentel]

Hi Lorenzo,

On 01/05/2018 15:26, Lorenzo Pieralisi wrote:
> On Tue, May 01, 2018 at 05:53:59PM +0530, Kishon Vijay Abraham I wrote:
>> Hi Lorenzo,
>>
>> On Tuesday 01 May 2018 05:24 PM, Lorenzo Pieralisi wrote:
>>> On Tue, May 01, 2018 at 03:37:47PM +0530, Kishon Vijay Abraham I wrote:
>>>> Hi Lorenzo,
>>>>
>>>> On Thursday 26 April 2018 10:26 PM, Lorenzo Pieralisi wrote:
>>>>> On Tue, Apr 24, 2018 at 02:44:40PM +0100, Gustavo Pimentel wrote:
>>>>>> Adds a seconds entry on the pci_epf_test_ids structure that disables the
>>>>>
>>>>> "Add a second entry to..."
>>>>>
>>>>>> linkup_notifier parameter on driver for the designware EP.
>>>>>>
>>>>>> This allows designware EPs that doesn't have linkup notification signal
>>>>>> to work with pcitest.
>>>>>>
>>>>>> Updates the binding documentation accordingly.
>>>>>
>>>>> Valid for all the series: use imperative sentences.
>>>>>
>>>>> eg:
>>>>>
>>>>> "Update the binding documentation accordingly".
>>>>>
>>>>> not
>>>>>
>>>>> "Updates the binding documentation accordingly".
>>>>>
>>>>>> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
>>>>>> Acked-by: Kishon Vijay Abraham I <kishon@ti.com>
>>>>>> ---
>>>>>> Change v2->v3:
>>>>>>  - Added second entry in pci_epf_test_ids structure.
>>>>>>  - Remove test_reg_bar field assignment on second entry.
>>>>>> Changes v3->v4:
>>>>>>  - Nothing changed, just to follow the patch set version.
>>>>>> Changes v4->v5:
>>>>>>  - Changed pci_epf_test_cfg2 to pci_epf_test_designware.
>>>>>> Changes v5->v6:
>>>>>>  - Changed name field from pci_epf_test_designware to pci_epf_test_dw.
>>>>>> Changes v6->v7:
>>>>>>  - Changed variable name from data_cfg2 to data_linkup_notifier_disabled.
>>>>>>
>>>>>>  Documentation/PCI/endpoint/function/binding/pci-test.txt | 2 ++
>>>>>>  drivers/pci/endpoint/functions/pci-epf-test.c            | 8 ++++++++
>>>>>>  2 files changed, 10 insertions(+)
>>>>>>
>>>>>> diff --git a/Documentation/PCI/endpoint/function/binding/pci-test.txt b/Documentation/PCI/endpoint/function/binding/pci-test.txt
>>>>>> index 3b68b95..dc39f47 100644
>>>>>> --- a/Documentation/PCI/endpoint/function/binding/pci-test.txt
>>>>>> +++ b/Documentation/PCI/endpoint/function/binding/pci-test.txt
>>>>>> @@ -1,6 +1,8 @@
>>>>>>  PCI TEST ENDPOINT FUNCTION
>>>>>>  
>>>>>>  name: Should be "pci_epf_test" to bind to the pci_epf_test driver.
>>>>>> +name: Should be "pci_epf_test_dw" to bind to the pci_epf_test driver
>>>>>> +      with a custom configuration for the designware EP.
>>>>>
>>>>> The link between the "name" and the device created is quite obscure and
>>>>> reading pci-test-howto.txt certainly does not clarify it.
>>>>>
>>>>> In pci-test-howto.txt an explanation should be added to the configs
>>>>> device creation paragraph to clarify it.
>>>>>
>>>>>>  Configurable Fields:
>>>>>>  vendorid	 : should be 0x104c
>>>>>> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
>>>>>> index 7cef851..4ab463b 100644
>>>>>> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
>>>>>> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
>>>>>> @@ -459,10 +459,18 @@ static int pci_epf_test_bind(struct pci_epf *epf)
>>>>>>  	return 0;
>>>>>>  }
>>>>>>  
>>>>>> +static const struct pci_epf_test_data data_linkup_notifier_disabled = {
>>>>>> +	.linkup_notifier = false
>>>>>> +};
>>>>>> +
>>>>>>  static const struct pci_epf_device_id pci_epf_test_ids[] = {
>>>>>>  	{
>>>>>>  		.name = "pci_epf_test",
>>>>>>  	},
>>>>>> +	{
>>>>>> +		.name = "pci_epf_test_dw",
>>>>>> +		.driver_data = (kernel_ulong_t)&data_linkup_notifier_disabled,
>>>>>> +	},
>>>>>>  	{},
>>>>>
>>>>> Should not this be a property derived from the controller compatible
>>>>> property instead of the test device name written in configfs ?
>>>>
>>>> pci_epf_test is an independent driver on its own that operates in a layer above
>>>> the controller driver. So it does not get the controller compatible (which is
>>>> used in controller drivers like pcie-designware-plat.c). pci_epf_test uses
>>>> "pci_epf_device_id" which is _similar_ to "of_device_id" used by platform drivers.
>>>
>>> I understand that, the problem is that the independent driver depends on
>>> features of the related controller driver as this patch shows. This
>>> patch basically says that if you use a specific path in configfs (that
>>> includes pci_epf_test_dw) your device has specific HW features (eg
>>> linkup notifier above), that obviously depends on the platform HW not on
>>> the string you use in configfs.
>>>
>>> What I am questioning is a) if I understand this right and b) whether
>>> this is the right approach.
>>
>> Your understanding is right. Ideally pci-epf-test driver shouldn't have any HW
>> specific configuration. But different HW have different configurations and
>> pci-epf-test should be informed of the configuration the HW supports.
> 
> I am honestly very confused. First off, I do not understand what this
> patch really does (or better what DW can't do so that it needs a
> specific configfs string and therefore a specific configuration).
> 
> What's the purpose of the linkup notifier ? What does it mean that
> the DW HW can't handle it ?
> 
> Are we referring to the pci_epf_linkup() function ?
> 
> If it is a HW configuration (ie the DW HW does not have a signal to
> report that the link is up ?) its enablement must depend on HW
> controller properties not configfs entries, I do not like what this
> patch does (probably because I am confused and I do not understand it).
> 
> Please let me know your thoughts on this, thanks.

On my setup, the EP is unable generate a signal which is responsible for notify
that the link is established, being therefore necessary to emulate it, this is
done by setting the linkup_notifier variable to false.

This signal allows the pci-epf-test driver on the EP side to startup with a
cyclic task defined on pci_epf_test_cmd_handler() that checks a command register
located on the BAR (by default on BAR 0) settled by pci_endpoint_test driver on
the RC side that defines, by his turn defines which type of command to perform.

> 
> Lorenzo
> 
>>
>> configfs is just one way of creating epf_device and it was mainly added since
>> pci-epf-test cannot have a dt entry because it doesn't represent anything in
>> the HW.
>>
>> The other option was to have a callback to EPC driver to get the features it
>> supports. But a particular feature that is required might be specific to a EPF
>> driver.
>>
>> I find the driver_data approach in pci_epf_device_id to be more clean.

Since the linkup notifier and BAR index (where auxiliary registers are located)
may be configurable and is something platform dependent, perhaps the
configuration of this variables should be done by module parameter and not by
configfs, leaving this configuration responsibility in charge of each platform.

I think this option is also a good alternative, simple and clean. What do you think?

>>
>> Thanks
>> Kishon

Regards,
Gustavo

Re: [PATCH v7 3/9] PCI: endpoint: functions/pci-epf-test: Add second entry

[Lorenzo Pieralisi]

On Wed, May 02, 2018 at 11:39:00AM +0100, Gustavo Pimentel wrote:
> Hi Lorenzo,
> 
> On 01/05/2018 15:26, Lorenzo Pieralisi wrote:
> > On Tue, May 01, 2018 at 05:53:59PM +0530, Kishon Vijay Abraham I wrote:
> >> Hi Lorenzo,
> >>
> >> On Tuesday 01 May 2018 05:24 PM, Lorenzo Pieralisi wrote:
> >>> On Tue, May 01, 2018 at 03:37:47PM +0530, Kishon Vijay Abraham I wrote:
> >>>> Hi Lorenzo,
> >>>>
> >>>> On Thursday 26 April 2018 10:26 PM, Lorenzo Pieralisi wrote:
> >>>>> On Tue, Apr 24, 2018 at 02:44:40PM +0100, Gustavo Pimentel wrote:
> >>>>>> Adds a seconds entry on the pci_epf_test_ids structure that disables the
> >>>>>
> >>>>> "Add a second entry to..."
> >>>>>
> >>>>>> linkup_notifier parameter on driver for the designware EP.
> >>>>>>
> >>>>>> This allows designware EPs that doesn't have linkup notification signal
> >>>>>> to work with pcitest.
> >>>>>>
> >>>>>> Updates the binding documentation accordingly.
> >>>>>
> >>>>> Valid for all the series: use imperative sentences.
> >>>>>
> >>>>> eg:
> >>>>>
> >>>>> "Update the binding documentation accordingly".
> >>>>>
> >>>>> not
> >>>>>
> >>>>> "Updates the binding documentation accordingly".
> >>>>>
> >>>>>> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> >>>>>> Acked-by: Kishon Vijay Abraham I <kishon@ti.com>
> >>>>>> ---
> >>>>>> Change v2->v3:
> >>>>>>  - Added second entry in pci_epf_test_ids structure.
> >>>>>>  - Remove test_reg_bar field assignment on second entry.
> >>>>>> Changes v3->v4:
> >>>>>>  - Nothing changed, just to follow the patch set version.
> >>>>>> Changes v4->v5:
> >>>>>>  - Changed pci_epf_test_cfg2 to pci_epf_test_designware.
> >>>>>> Changes v5->v6:
> >>>>>>  - Changed name field from pci_epf_test_designware to pci_epf_test_dw.
> >>>>>> Changes v6->v7:
> >>>>>>  - Changed variable name from data_cfg2 to data_linkup_notifier_disabled.
> >>>>>>
> >>>>>>  Documentation/PCI/endpoint/function/binding/pci-test.txt | 2 ++
> >>>>>>  drivers/pci/endpoint/functions/pci-epf-test.c            | 8 ++++++++
> >>>>>>  2 files changed, 10 insertions(+)
> >>>>>>
> >>>>>> diff --git a/Documentation/PCI/endpoint/function/binding/pci-test.txt b/Documentation/PCI/endpoint/function/binding/pci-test.txt
> >>>>>> index 3b68b95..dc39f47 100644
> >>>>>> --- a/Documentation/PCI/endpoint/function/binding/pci-test.txt
> >>>>>> +++ b/Documentation/PCI/endpoint/function/binding/pci-test.txt
> >>>>>> @@ -1,6 +1,8 @@
> >>>>>>  PCI TEST ENDPOINT FUNCTION
> >>>>>>  
> >>>>>>  name: Should be "pci_epf_test" to bind to the pci_epf_test driver.
> >>>>>> +name: Should be "pci_epf_test_dw" to bind to the pci_epf_test driver
> >>>>>> +      with a custom configuration for the designware EP.
> >>>>>
> >>>>> The link between the "name" and the device created is quite obscure and
> >>>>> reading pci-test-howto.txt certainly does not clarify it.
> >>>>>
> >>>>> In pci-test-howto.txt an explanation should be added to the configs
> >>>>> device creation paragraph to clarify it.
> >>>>>
> >>>>>>  Configurable Fields:
> >>>>>>  vendorid	 : should be 0x104c
> >>>>>> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> >>>>>> index 7cef851..4ab463b 100644
> >>>>>> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> >>>>>> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> >>>>>> @@ -459,10 +459,18 @@ static int pci_epf_test_bind(struct pci_epf *epf)
> >>>>>>  	return 0;
> >>>>>>  }
> >>>>>>  
> >>>>>> +static const struct pci_epf_test_data data_linkup_notifier_disabled = {
> >>>>>> +	.linkup_notifier = false
> >>>>>> +};
> >>>>>> +
> >>>>>>  static const struct pci_epf_device_id pci_epf_test_ids[] = {
> >>>>>>  	{
> >>>>>>  		.name = "pci_epf_test",
> >>>>>>  	},
> >>>>>> +	{
> >>>>>> +		.name = "pci_epf_test_dw",
> >>>>>> +		.driver_data = (kernel_ulong_t)&data_linkup_notifier_disabled,
> >>>>>> +	},
> >>>>>>  	{},
> >>>>>
> >>>>> Should not this be a property derived from the controller compatible
> >>>>> property instead of the test device name written in configfs ?
> >>>>
> >>>> pci_epf_test is an independent driver on its own that operates in a layer above
> >>>> the controller driver. So it does not get the controller compatible (which is
> >>>> used in controller drivers like pcie-designware-plat.c). pci_epf_test uses
> >>>> "pci_epf_device_id" which is _similar_ to "of_device_id" used by platform drivers.
> >>>
> >>> I understand that, the problem is that the independent driver depends on
> >>> features of the related controller driver as this patch shows. This
> >>> patch basically says that if you use a specific path in configfs (that
> >>> includes pci_epf_test_dw) your device has specific HW features (eg
> >>> linkup notifier above), that obviously depends on the platform HW not on
> >>> the string you use in configfs.
> >>>
> >>> What I am questioning is a) if I understand this right and b) whether
> >>> this is the right approach.
> >>
> >> Your understanding is right. Ideally pci-epf-test driver shouldn't have any HW
> >> specific configuration. But different HW have different configurations and
> >> pci-epf-test should be informed of the configuration the HW supports.
> > 
> > I am honestly very confused. First off, I do not understand what this
> > patch really does (or better what DW can't do so that it needs a
> > specific configfs string and therefore a specific configuration).
> > 
> > What's the purpose of the linkup notifier ? What does it mean that
> > the DW HW can't handle it ?
> > 
> > Are we referring to the pci_epf_linkup() function ?
> > 
> > If it is a HW configuration (ie the DW HW does not have a signal to
> > report that the link is up ?) its enablement must depend on HW
> > controller properties not configfs entries, I do not like what this
> > patch does (probably because I am confused and I do not understand it).
> > 
> > Please let me know your thoughts on this, thanks.
> 
> On my setup, the EP is unable generate a signal which is responsible
> for notify that the link is established, being therefore necessary to
> emulate it, this is done by setting the linkup_notifier variable to
> false.

What I do not understand is why the workqueue notification can't be called
irrespective of the linkup notifier value from pci_epf_test_bind(),
what's the point in calling it earlier with pci_epf_linkup() (or the
other way around why is it safe to call it a bind() time if there
is no notification available - which is the DW case).

[...]

> Since the linkup notifier and BAR index (where auxiliary registers are
> located) may be configurable and is something platform dependent,
> perhaps the configuration of this variables should be done by module
> parameter and not by configfs, leaving this configuration
> responsibility in charge of each platform.

They are platform dependent because they depend on the EP controller.
That's why I said that those are EP controller parameters. I do not
think they are module parameters either - they should be part of HW
description in firmware.

Lorenzo

> I think this option is also a good alternative, simple and clean. What
> do you think?
> 
> >>
> >> Thanks
> >> Kishon
> 
> Regards,
> Gustavo
> 

Re: [PATCH v7 3/9] PCI: endpoint: functions/pci-epf-test: Add second entry

[Kishon Vijay Abraham I]

Hi Lorenzo,

On Wednesday 02 May 2018 10:21 PM, Lorenzo Pieralisi wrote:
> On Wed, May 02, 2018 at 11:39:00AM +0100, Gustavo Pimentel wrote:
>> Hi Lorenzo,
>>
>> On 01/05/2018 15:26, Lorenzo Pieralisi wrote:
>>> On Tue, May 01, 2018 at 05:53:59PM +0530, Kishon Vijay Abraham I wrote:
>>>> Hi Lorenzo,
>>>>
>>>> On Tuesday 01 May 2018 05:24 PM, Lorenzo Pieralisi wrote:
>>>>> On Tue, May 01, 2018 at 03:37:47PM +0530, Kishon Vijay Abraham I wrote:
>>>>>> Hi Lorenzo,
>>>>>>
>>>>>> On Thursday 26 April 2018 10:26 PM, Lorenzo Pieralisi wrote:
>>>>>>> On Tue, Apr 24, 2018 at 02:44:40PM +0100, Gustavo Pimentel wrote:
>>>>>>>> Adds a seconds entry on the pci_epf_test_ids structure that disables the
>>>>>>>
>>>>>>> "Add a second entry to..."
>>>>>>>
>>>>>>>> linkup_notifier parameter on driver for the designware EP.
>>>>>>>>
>>>>>>>> This allows designware EPs that doesn't have linkup notification signal
>>>>>>>> to work with pcitest.
>>>>>>>>
>>>>>>>> Updates the binding documentation accordingly.
>>>>>>>
>>>>>>> Valid for all the series: use imperative sentences.
>>>>>>>
>>>>>>> eg:
>>>>>>>
>>>>>>> "Update the binding documentation accordingly".
>>>>>>>
>>>>>>> not
>>>>>>>
>>>>>>> "Updates the binding documentation accordingly".
>>>>>>>
>>>>>>>> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
>>>>>>>> Acked-by: Kishon Vijay Abraham I <kishon@ti.com>
>>>>>>>> ---
>>>>>>>> Change v2->v3:
>>>>>>>>  - Added second entry in pci_epf_test_ids structure.
>>>>>>>>  - Remove test_reg_bar field assignment on second entry.
>>>>>>>> Changes v3->v4:
>>>>>>>>  - Nothing changed, just to follow the patch set version.
>>>>>>>> Changes v4->v5:
>>>>>>>>  - Changed pci_epf_test_cfg2 to pci_epf_test_designware.
>>>>>>>> Changes v5->v6:
>>>>>>>>  - Changed name field from pci_epf_test_designware to pci_epf_test_dw.
>>>>>>>> Changes v6->v7:
>>>>>>>>  - Changed variable name from data_cfg2 to data_linkup_notifier_disabled.
>>>>>>>>
>>>>>>>>  Documentation/PCI/endpoint/function/binding/pci-test.txt | 2 ++
>>>>>>>>  drivers/pci/endpoint/functions/pci-epf-test.c            | 8 ++++++++
>>>>>>>>  2 files changed, 10 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/Documentation/PCI/endpoint/function/binding/pci-test.txt b/Documentation/PCI/endpoint/function/binding/pci-test.txt
>>>>>>>> index 3b68b95..dc39f47 100644
>>>>>>>> --- a/Documentation/PCI/endpoint/function/binding/pci-test.txt
>>>>>>>> +++ b/Documentation/PCI/endpoint/function/binding/pci-test.txt
>>>>>>>> @@ -1,6 +1,8 @@
>>>>>>>>  PCI TEST ENDPOINT FUNCTION
>>>>>>>>  
>>>>>>>>  name: Should be "pci_epf_test" to bind to the pci_epf_test driver.
>>>>>>>> +name: Should be "pci_epf_test_dw" to bind to the pci_epf_test driver
>>>>>>>> +      with a custom configuration for the designware EP.
>>>>>>>
>>>>>>> The link between the "name" and the device created is quite obscure and
>>>>>>> reading pci-test-howto.txt certainly does not clarify it.
>>>>>>>
>>>>>>> In pci-test-howto.txt an explanation should be added to the configs
>>>>>>> device creation paragraph to clarify it.
>>>>>>>
>>>>>>>>  Configurable Fields:
>>>>>>>>  vendorid	 : should be 0x104c
>>>>>>>> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
>>>>>>>> index 7cef851..4ab463b 100644
>>>>>>>> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
>>>>>>>> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
>>>>>>>> @@ -459,10 +459,18 @@ static int pci_epf_test_bind(struct pci_epf *epf)
>>>>>>>>  	return 0;
>>>>>>>>  }
>>>>>>>>  
>>>>>>>> +static const struct pci_epf_test_data data_linkup_notifier_disabled = {
>>>>>>>> +	.linkup_notifier = false
>>>>>>>> +};
>>>>>>>> +
>>>>>>>>  static const struct pci_epf_device_id pci_epf_test_ids[] = {
>>>>>>>>  	{
>>>>>>>>  		.name = "pci_epf_test",
>>>>>>>>  	},
>>>>>>>> +	{
>>>>>>>> +		.name = "pci_epf_test_dw",
>>>>>>>> +		.driver_data = (kernel_ulong_t)&data_linkup_notifier_disabled,
>>>>>>>> +	},
>>>>>>>>  	{},
>>>>>>>
>>>>>>> Should not this be a property derived from the controller compatible
>>>>>>> property instead of the test device name written in configfs ?
>>>>>>
>>>>>> pci_epf_test is an independent driver on its own that operates in a layer above
>>>>>> the controller driver. So it does not get the controller compatible (which is
>>>>>> used in controller drivers like pcie-designware-plat.c). pci_epf_test uses
>>>>>> "pci_epf_device_id" which is _similar_ to "of_device_id" used by platform drivers.
>>>>>
>>>>> I understand that, the problem is that the independent driver depends on
>>>>> features of the related controller driver as this patch shows. This
>>>>> patch basically says that if you use a specific path in configfs (that
>>>>> includes pci_epf_test_dw) your device has specific HW features (eg
>>>>> linkup notifier above), that obviously depends on the platform HW not on
>>>>> the string you use in configfs.
>>>>>
>>>>> What I am questioning is a) if I understand this right and b) whether
>>>>> this is the right approach.
>>>>
>>>> Your understanding is right. Ideally pci-epf-test driver shouldn't have any HW
>>>> specific configuration. But different HW have different configurations and
>>>> pci-epf-test should be informed of the configuration the HW supports.
>>>
>>> I am honestly very confused. First off, I do not understand what this
>>> patch really does (or better what DW can't do so that it needs a
>>> specific configfs string and therefore a specific configuration).
>>>
>>> What's the purpose of the linkup notifier ? What does it mean that
>>> the DW HW can't handle it ?
>>>
>>> Are we referring to the pci_epf_linkup() function ?
>>>
>>> If it is a HW configuration (ie the DW HW does not have a signal to
>>> report that the link is up ?) its enablement must depend on HW
>>> controller properties not configfs entries, I do not like what this
>>> patch does (probably because I am confused and I do not understand it).
>>>
>>> Please let me know your thoughts on this, thanks.
>>
>> On my setup, the EP is unable generate a signal which is responsible
>> for notify that the link is established, being therefore necessary to
>> emulate it, this is done by setting the linkup_notifier variable to
>> false.
> 
> What I do not understand is why the workqueue notification can't be called
> irrespective of the linkup notifier value from pci_epf_test_bind(),
> what's the point in calling it earlier with pci_epf_linkup() (or the
> other way around why is it safe to call it a bind() time if there
> is no notification available - which is the DW case).

The linkup notifier is used just to prevent the system from wasting cpu cycles.
Without linkup notifier, pci_epf_test will start checking for commands from
host after the pci_epf_test is bound to the EP controller (bind() callback)
though the host can issue commands only when after the link is up. So for
platforms which supports linkup notification, it's better we poll command
register only after link is established.
> 
> [...]
> 
>> Since the linkup notifier and BAR index (where auxiliary registers are
>> located) may be configurable and is something platform dependent,
>> perhaps the configuration of this variables should be done by module
>> parameter and not by configfs, leaving this configuration
>> responsibility in charge of each platform.
> 
> They are platform dependent because they depend on the EP controller.
> That's why I said that those are EP controller parameters. I do not
> think they are module parameters either - they should be part of HW
> description in firmware.

The problem is because pci-epf-test cannot be described in HW. pci-epf-test is
also not automatically bound to the EP controller but is bound by the user like
below
ln -s functions/pci_epf_test/func1 controllers/51000000.pcie_ep/

So given that user anyways has to bind the epf device to the controller, based
on the platform the user can use a different configfs entry like below
ln -s functions/pci_epf_test_dw/func1 controllers/51000000.pcie_ep/ or
ln -s functions/pci_epf_test_k2g/func1 controllers/21800000.pcie-ep/

If the epf can be described in dt, then something like below can be done
pcie1_ep: pcie_ep@51000000 {
	compatible = "ti,dra7-pcie-ep";
	interrupts = <0 232 0x4>;
	num-lanes = <1>;
	num-ib-windows = <4>;
	num-ob-windows = <16>;
	phys = <&pcie1_phy>;
	phy-names = "pcie-phy0";
	pci_epf_test: pci_epf_test@0 {
		name = "pci_epf_test_dw";
		<other properties>;
	}
};

With this pci-dra7xx.c driver should create pci_epf_device using
pci_epf_create("pci_epf_test_dw");

Then the driver_data corresponding to "pci_epf_test_dw" will select linkup
notifier or BAR index etc.

Having different pci_epf_device_id entries and corresponding driver_data for
each entry seems fine to me (as done in this patch). We use configfs only since
pci_epf_test cannot be described in dt.

Thanks
Kishon

Re: [PATCH v7 3/9] PCI: endpoint: functions/pci-epf-test: Add second entry

[Lorenzo Pieralisi]

On Thu, May 03, 2018 at 12:03:15PM +0530, Kishon Vijay Abraham I wrote:

[...]

> >> Since the linkup notifier and BAR index (where auxiliary registers are
> >> located) may be configurable and is something platform dependent,
> >> perhaps the configuration of this variables should be done by module
> >> parameter and not by configfs, leaving this configuration
> >> responsibility in charge of each platform.
> > 
> > They are platform dependent because they depend on the EP controller.
> > That's why I said that those are EP controller parameters. I do not
> > think they are module parameters either - they should be part of HW
> > description in firmware.
> 
> The problem is because pci-epf-test cannot be described in HW. pci-epf-test is
> also not automatically bound to the EP controller but is bound by the user like
> below
> ln -s functions/pci_epf_test/func1 controllers/51000000.pcie_ep/
> 
> So given that user anyways has to bind the epf device to the controller, based
> on the platform the user can use a different configfs entry like below
> ln -s functions/pci_epf_test_dw/func1 controllers/51000000.pcie_ep/ or
> ln -s functions/pci_epf_test_k2g/func1 controllers/21800000.pcie-ep/
> 
> If the epf can be described in dt, then something like below can be done
> pcie1_ep: pcie_ep@51000000 {
> 	compatible = "ti,dra7-pcie-ep";
> 	interrupts = <0 232 0x4>;
> 	num-lanes = <1>;
> 	num-ib-windows = <4>;
> 	num-ob-windows = <16>;
> 	phys = <&pcie1_phy>;
> 	phy-names = "pcie-phy0";
> 	pci_epf_test: pci_epf_test@0 {
> 		name = "pci_epf_test_dw";
> 		<other properties>;
> 	}
> };
> 
> With this pci-dra7xx.c driver should create pci_epf_device using
> pci_epf_create("pci_epf_test_dw");
> 
> Then the driver_data corresponding to "pci_epf_test_dw" will select linkup
> notifier or BAR index etc.

Those two properties are properties of the EP controller (it is not 100%
clear to me how the test BAR register is defined), is this correct ?

If yes, given that those properties are not useful before an EPF is
bound to an EPC, can't they be retrieved at bind time from the EPC
controller data (that we can add through DT bindings) ?

Thanks,
Lorenzo

Re: [PATCH v7 3/9] PCI: endpoint: functions/pci-epf-test: Add second entry

[Kishon Vijay Abraham I]

Hi Lorenzo,

On Thursday 03 May 2018 07:46 PM, Lorenzo Pieralisi wrote:
> On Thu, May 03, 2018 at 12:03:15PM +0530, Kishon Vijay Abraham I wrote:
> 
> [...]
> 
>>>> Since the linkup notifier and BAR index (where auxiliary registers are
>>>> located) may be configurable and is something platform dependent,
>>>> perhaps the configuration of this variables should be done by module
>>>> parameter and not by configfs, leaving this configuration
>>>> responsibility in charge of each platform.
>>>
>>> They are platform dependent because they depend on the EP controller.
>>> That's why I said that those are EP controller parameters. I do not
>>> think they are module parameters either - they should be part of HW
>>> description in firmware.
>>
>> The problem is because pci-epf-test cannot be described in HW. pci-epf-test is
>> also not automatically bound to the EP controller but is bound by the user like
>> below
>> ln -s functions/pci_epf_test/func1 controllers/51000000.pcie_ep/
>>
>> So given that user anyways has to bind the epf device to the controller, based
>> on the platform the user can use a different configfs entry like below
>> ln -s functions/pci_epf_test_dw/func1 controllers/51000000.pcie_ep/ or
>> ln -s functions/pci_epf_test_k2g/func1 controllers/21800000.pcie-ep/
>>
>> If the epf can be described in dt, then something like below can be done
>> pcie1_ep: pcie_ep@51000000 {
>> 	compatible = "ti,dra7-pcie-ep";
>> 	interrupts = <0 232 0x4>;
>> 	num-lanes = <1>;
>> 	num-ib-windows = <4>;
>> 	num-ob-windows = <16>;
>> 	phys = <&pcie1_phy>;
>> 	phy-names = "pcie-phy0";
>> 	pci_epf_test: pci_epf_test@0 {
>> 		name = "pci_epf_test_dw";
>> 		<other properties>;
>> 	}
>> };
>>
>> With this pci-dra7xx.c driver should create pci_epf_device using
>> pci_epf_create("pci_epf_test_dw");
>>
>> Then the driver_data corresponding to "pci_epf_test_dw" will select linkup
>> notifier or BAR index etc.
> 
> Those two properties are properties of the EP controller (it is not 100%
> clear to me how the test BAR register is defined), is this correct ?

Right, these properties are specific to a platform. In some of the platforms
like K2G (BAR0 is reserved i.e it is used to map PCIe app registers and cannot
be used by pci_epf_test. In such cases we should use a BAR other than BAR0).
> 
> If yes, given that those properties are not useful before an EPF is
> bound to an EPC, can't they be retrieved at bind time from the EPC
> controller data (that we can add through DT bindings) ?

hmm..

We can have quirk in pci_epc, something like below

struct pci_epc {
	.
	.
	unsigned int quirks;
	.
	.
};

#define EPC_QUIRKS_NO_LINKUP_NOTIFIER	BIT(0)
#define EPC_QUIRKS_BAR0_RESERVED	BIT(1)
#define EPC_QUIRKS_BAR1_RESERVED	BIT(2)
#define EPC_QUIRKS_BAR2_RESERVED	BIT(3)
#define EPC_QUIRKS_BAR3_RESERVED	BIT(4)
#define EPC_QUIRKS_BAR4_RESERVED	BIT(5)
#define EPC_QUIRKS_BAR5_RESERVED	BIT(6)

The controller driver can set the appropriate quirks
epc->quirks |= EPC_QUIRKS_NO_LINKUP_NOTIFIER | EPC_QUIRKS_BAR0_RESERVED;

Then pci-epf-test driver can checks the quirks to see the supported EPC features.

Does something like above looks okay to you?

Thanks
Kishon

Re: [PATCH v7 3/9] PCI: endpoint: functions/pci-epf-test: Add second entry

[Lorenzo Pieralisi]

On Fri, May 04, 2018 at 11:28:58AM +0530, Kishon Vijay Abraham I wrote:
> Hi Lorenzo,
> 
> On Thursday 03 May 2018 07:46 PM, Lorenzo Pieralisi wrote:
> > On Thu, May 03, 2018 at 12:03:15PM +0530, Kishon Vijay Abraham I wrote:
> > 
> > [...]
> > 
> >>>> Since the linkup notifier and BAR index (where auxiliary registers are
> >>>> located) may be configurable and is something platform dependent,
> >>>> perhaps the configuration of this variables should be done by module
> >>>> parameter and not by configfs, leaving this configuration
> >>>> responsibility in charge of each platform.
> >>>
> >>> They are platform dependent because they depend on the EP controller.
> >>> That's why I said that those are EP controller parameters. I do not
> >>> think they are module parameters either - they should be part of HW
> >>> description in firmware.
> >>
> >> The problem is because pci-epf-test cannot be described in HW. pci-epf-test is
> >> also not automatically bound to the EP controller but is bound by the user like
> >> below
> >> ln -s functions/pci_epf_test/func1 controllers/51000000.pcie_ep/
> >>
> >> So given that user anyways has to bind the epf device to the controller, based
> >> on the platform the user can use a different configfs entry like below
> >> ln -s functions/pci_epf_test_dw/func1 controllers/51000000.pcie_ep/ or
> >> ln -s functions/pci_epf_test_k2g/func1 controllers/21800000.pcie-ep/
> >>
> >> If the epf can be described in dt, then something like below can be done
> >> pcie1_ep: pcie_ep@51000000 {
> >> 	compatible = "ti,dra7-pcie-ep";
> >> 	interrupts = <0 232 0x4>;
> >> 	num-lanes = <1>;
> >> 	num-ib-windows = <4>;
> >> 	num-ob-windows = <16>;
> >> 	phys = <&pcie1_phy>;
> >> 	phy-names = "pcie-phy0";
> >> 	pci_epf_test: pci_epf_test@0 {
> >> 		name = "pci_epf_test_dw";
> >> 		<other properties>;
> >> 	}
> >> };
> >>
> >> With this pci-dra7xx.c driver should create pci_epf_device using
> >> pci_epf_create("pci_epf_test_dw");
> >>
> >> Then the driver_data corresponding to "pci_epf_test_dw" will select linkup
> >> notifier or BAR index etc.
> > 
> > Those two properties are properties of the EP controller (it is not 100%
> > clear to me how the test BAR register is defined), is this correct ?
> 
> Right, these properties are specific to a platform. In some of the platforms
> like K2G (BAR0 is reserved i.e it is used to map PCIe app registers and cannot
> be used by pci_epf_test. In such cases we should use a BAR other than BAR0).

I do not think those properties are pci_epf_test specific, that's the
whole point I am making. Those are EPC controller features.

> > If yes, given that those properties are not useful before an EPF is
> > bound to an EPC, can't they be retrieved at bind time from the EPC
> > controller data (that we can add through DT bindings) ?
> 
> hmm..
> 
> We can have quirk in pci_epc, something like below
> 
> struct pci_epc {
> 	.
> 	.
> 	unsigned int quirks;
> 	.
> 	.
> };
> 
> #define EPC_QUIRKS_NO_LINKUP_NOTIFIER	BIT(0)
> #define EPC_QUIRKS_BAR0_RESERVED	BIT(1)
> #define EPC_QUIRKS_BAR1_RESERVED	BIT(2)
> #define EPC_QUIRKS_BAR2_RESERVED	BIT(3)
> #define EPC_QUIRKS_BAR3_RESERVED	BIT(4)
> #define EPC_QUIRKS_BAR4_RESERVED	BIT(5)
> #define EPC_QUIRKS_BAR5_RESERVED	BIT(6)
> 
> The controller driver can set the appropriate quirks
> epc->quirks |= EPC_QUIRKS_NO_LINKUP_NOTIFIER | EPC_QUIRKS_BAR0_RESERVED;
> 
> Then pci-epf-test driver can checks the quirks to see the supported EPC features.
> 
> Does something like above looks okay to you?

Well, it is better than the driver_data approach, I would not call them
quirks but features and for the BARs you should define a mask it does
not make sense to enumerate them. It is probably a good idea to leave
room for additional "features" so please choose the bits placement
wisely.

Thanks,
Lorenzo

Re: [PATCH v7 3/9] PCI: endpoint: functions/pci-epf-test: Add second entry

[Gustavo Pimentel]

Hi Lorenzo and Kishon,

On 03/05/2018 07:33, Kishon Vijay Abraham I wrote:
> Hi Lorenzo,
> 
> On Wednesday 02 May 2018 10:21 PM, Lorenzo Pieralisi wrote:
>> On Wed, May 02, 2018 at 11:39:00AM +0100, Gustavo Pimentel wrote:
>>> Hi Lorenzo,
>>>
>>> On 01/05/2018 15:26, Lorenzo Pieralisi wrote:
>>>> On Tue, May 01, 2018 at 05:53:59PM +0530, Kishon Vijay Abraham I wrote:
>>>>> Hi Lorenzo,
>>>>>
>>>>> On Tuesday 01 May 2018 05:24 PM, Lorenzo Pieralisi wrote:
>>>>>> On Tue, May 01, 2018 at 03:37:47PM +0530, Kishon Vijay Abraham I wrote:
>>>>>>> Hi Lorenzo,
>>>>>>>
>>>>>>> On Thursday 26 April 2018 10:26 PM, Lorenzo Pieralisi wrote:
>>>>>>>> On Tue, Apr 24, 2018 at 02:44:40PM +0100, Gustavo Pimentel wrote:
>>>>>>>>> Adds a seconds entry on the pci_epf_test_ids structure that disables the
>>>>>>>>
>>>>>>>> "Add a second entry to..."
>>>>>>>>
>>>>>>>>> linkup_notifier parameter on driver for the designware EP.
>>>>>>>>>
>>>>>>>>> This allows designware EPs that doesn't have linkup notification signal
>>>>>>>>> to work with pcitest.
>>>>>>>>>
>>>>>>>>> Updates the binding documentation accordingly.
>>>>>>>>
>>>>>>>> Valid for all the series: use imperative sentences.
>>>>>>>>
>>>>>>>> eg:
>>>>>>>>
>>>>>>>> "Update the binding documentation accordingly".
>>>>>>>>
>>>>>>>> not
>>>>>>>>
>>>>>>>> "Updates the binding documentation accordingly".
>>>>>>>>
>>>>>>>>> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
>>>>>>>>> Acked-by: Kishon Vijay Abraham I <kishon@ti.com>
>>>>>>>>> ---
>>>>>>>>> Change v2->v3:
>>>>>>>>>  - Added second entry in pci_epf_test_ids structure.
>>>>>>>>>  - Remove test_reg_bar field assignment on second entry.
>>>>>>>>> Changes v3->v4:
>>>>>>>>>  - Nothing changed, just to follow the patch set version.
>>>>>>>>> Changes v4->v5:
>>>>>>>>>  - Changed pci_epf_test_cfg2 to pci_epf_test_designware.
>>>>>>>>> Changes v5->v6:
>>>>>>>>>  - Changed name field from pci_epf_test_designware to pci_epf_test_dw.
>>>>>>>>> Changes v6->v7:
>>>>>>>>>  - Changed variable name from data_cfg2 to data_linkup_notifier_disabled.
>>>>>>>>>
>>>>>>>>>  Documentation/PCI/endpoint/function/binding/pci-test.txt | 2 ++
>>>>>>>>>  drivers/pci/endpoint/functions/pci-epf-test.c            | 8 ++++++++
>>>>>>>>>  2 files changed, 10 insertions(+)
>>>>>>>>>
>>>>>>>>> diff --git a/Documentation/PCI/endpoint/function/binding/pci-test.txt b/Documentation/PCI/endpoint/function/binding/pci-test.txt
>>>>>>>>> index 3b68b95..dc39f47 100644
>>>>>>>>> --- a/Documentation/PCI/endpoint/function/binding/pci-test.txt
>>>>>>>>> +++ b/Documentation/PCI/endpoint/function/binding/pci-test.txt
>>>>>>>>> @@ -1,6 +1,8 @@
>>>>>>>>>  PCI TEST ENDPOINT FUNCTION
>>>>>>>>>  
>>>>>>>>>  name: Should be "pci_epf_test" to bind to the pci_epf_test driver.
>>>>>>>>> +name: Should be "pci_epf_test_dw" to bind to the pci_epf_test driver
>>>>>>>>> +      with a custom configuration for the designware EP.
>>>>>>>>
>>>>>>>> The link between the "name" and the device created is quite obscure and
>>>>>>>> reading pci-test-howto.txt certainly does not clarify it.
>>>>>>>>
>>>>>>>> In pci-test-howto.txt an explanation should be added to the configs
>>>>>>>> device creation paragraph to clarify it.
>>>>>>>>
>>>>>>>>>  Configurable Fields:
>>>>>>>>>  vendorid	 : should be 0x104c
>>>>>>>>> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
>>>>>>>>> index 7cef851..4ab463b 100644
>>>>>>>>> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
>>>>>>>>> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
>>>>>>>>> @@ -459,10 +459,18 @@ static int pci_epf_test_bind(struct pci_epf *epf)
>>>>>>>>>  	return 0;
>>>>>>>>>  }
>>>>>>>>>  
>>>>>>>>> +static const struct pci_epf_test_data data_linkup_notifier_disabled = {
>>>>>>>>> +	.linkup_notifier = false
>>>>>>>>> +};
>>>>>>>>> +
>>>>>>>>>  static const struct pci_epf_device_id pci_epf_test_ids[] = {
>>>>>>>>>  	{
>>>>>>>>>  		.name = "pci_epf_test",
>>>>>>>>>  	},
>>>>>>>>> +	{
>>>>>>>>> +		.name = "pci_epf_test_dw",
>>>>>>>>> +		.driver_data = (kernel_ulong_t)&data_linkup_notifier_disabled,
>>>>>>>>> +	},
>>>>>>>>>  	{},
>>>>>>>>
>>>>>>>> Should not this be a property derived from the controller compatible
>>>>>>>> property instead of the test device name written in configfs ?
>>>>>>>
>>>>>>> pci_epf_test is an independent driver on its own that operates in a layer above
>>>>>>> the controller driver. So it does not get the controller compatible (which is
>>>>>>> used in controller drivers like pcie-designware-plat.c). pci_epf_test uses
>>>>>>> "pci_epf_device_id" which is _similar_ to "of_device_id" used by platform drivers.
>>>>>>
>>>>>> I understand that, the problem is that the independent driver depends on
>>>>>> features of the related controller driver as this patch shows. This
>>>>>> patch basically says that if you use a specific path in configfs (that
>>>>>> includes pci_epf_test_dw) your device has specific HW features (eg
>>>>>> linkup notifier above), that obviously depends on the platform HW not on
>>>>>> the string you use in configfs.
>>>>>>
>>>>>> What I am questioning is a) if I understand this right and b) whether
>>>>>> this is the right approach.
>>>>>
>>>>> Your understanding is right. Ideally pci-epf-test driver shouldn't have any HW
>>>>> specific configuration. But different HW have different configurations and
>>>>> pci-epf-test should be informed of the configuration the HW supports.
>>>>
>>>> I am honestly very confused. First off, I do not understand what this
>>>> patch really does (or better what DW can't do so that it needs a
>>>> specific configfs string and therefore a specific configuration).
>>>>
>>>> What's the purpose of the linkup notifier ? What does it mean that
>>>> the DW HW can't handle it ?
>>>>
>>>> Are we referring to the pci_epf_linkup() function ?
>>>>
>>>> If it is a HW configuration (ie the DW HW does not have a signal to
>>>> report that the link is up ?) its enablement must depend on HW
>>>> controller properties not configfs entries, I do not like what this
>>>> patch does (probably because I am confused and I do not understand it).
>>>>
>>>> Please let me know your thoughts on this, thanks.
>>>
>>> On my setup, the EP is unable generate a signal which is responsible
>>> for notify that the link is established, being therefore necessary to
>>> emulate it, this is done by setting the linkup_notifier variable to
>>> false.
>>
>> What I do not understand is why the workqueue notification can't be called
>> irrespective of the linkup notifier value from pci_epf_test_bind(),
>> what's the point in calling it earlier with pci_epf_linkup() (or the
>> other way around why is it safe to call it a bind() time if there
>> is no notification available - which is the DW case).
> 
> The linkup notifier is used just to prevent the system from wasting cpu cycles.
> Without linkup notifier, pci_epf_test will start checking for commands from
> host after the pci_epf_test is bound to the EP controller (bind() callback)
> though the host can issue commands only when after the link is up. So for
> platforms which supports linkup notification, it's better we poll command
> register only after link is established.
>>
>> [...]
>>
>>> Since the linkup notifier and BAR index (where auxiliary registers are
>>> located) may be configurable and is something platform dependent,
>>> perhaps the configuration of this variables should be done by module
>>> parameter and not by configfs, leaving this configuration
>>> responsibility in charge of each platform.
>>
>> They are platform dependent because they depend on the EP controller.
>> That's why I said that those are EP controller parameters. I do not
>> think they are module parameters either - they should be part of HW
>> description in firmware.
> 
> The problem is because pci-epf-test cannot be described in HW. pci-epf-test is
> also not automatically bound to the EP controller but is bound by the user like
> below
> ln -s functions/pci_epf_test/func1 controllers/51000000.pcie_ep/
> 
> So given that user anyways has to bind the epf device to the controller, based
> on the platform the user can use a different configfs entry like below
> ln -s functions/pci_epf_test_dw/func1 controllers/51000000.pcie_ep/ or
> ln -s functions/pci_epf_test_k2g/func1 controllers/21800000.pcie-ep/
> 
> If the epf can be described in dt, then something like below can be done
> pcie1_ep: pcie_ep@51000000 {
> 	compatible = "ti,dra7-pcie-ep";
> 	interrupts = <0 232 0x4>;
> 	num-lanes = <1>;
> 	num-ib-windows = <4>;
> 	num-ob-windows = <16>;
> 	phys = <&pcie1_phy>;
> 	phy-names = "pcie-phy0";
> 	pci_epf_test: pci_epf_test@0 {
> 		name = "pci_epf_test_dw";
> 		<other properties>;
> 	}
> };
> 
> With this pci-dra7xx.c driver should create pci_epf_device using
> pci_epf_create("pci_epf_test_dw");
> 
> Then the driver_data corresponding to "pci_epf_test_dw" will select linkup
> notifier or BAR index etc.
> 
> Having different pci_epf_device_id entries and corresponding driver_data for
> each entry seems fine to me (as done in this patch). We use configfs only since
> pci_epf_test cannot be described in dt.

I think I understood your point of view Lorenzo and it makes sense.
However I think I have an alternative, since this 2 parameters are platform
dependent, but there aren't quite a HW description, so they shouldn't be
described on DT, right?

So I would propose another alternative. What if each EP driver (in my case
pcie-designware-plat) provided a callback that could be used to set the test
configuration data, if needed? Similar to the dw_plat_set_num_vectors callback.

In fact, something similar mechanism already exists, for example when the
pci_epf_test driver tries to get the number of MSI/MSI-X, the driver retrieves
the value through callbacks.

In this way each driver would be responsible for adapting to their needs, there
would be no changes to the DT modification or extra configfs entries.

What do you think?

Regards,
Gustavo

> 
> Thanks
> Kishon
> 

[PATCH v7 4/9] bindings: PCI: designware: Add support for the EP in Designware driver

[Gustavo Pimentel]

Add device tree binding documentation for the Endpoint in PCIe Designware
driver.

Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
Change v1->v2:
 - Add a missing log description.
 - Add "snps,dw-pcie" compatible string following Kishon's suggestion.
Change v2->v3:
 - Reverted pcie_ep name to pcie.
Changes v3->v4:
 - Reverted "snps,dw-pcie-rc" compatible string requested by Rob Herring.
Changes v4->v5:
 - Removed device_type entry from EP requested by Rob Herring.
Changes v5->v6:
 - Nothing changed, just to follow the patch set version.
Changes v6->v7:
 - Nothing changed, just to follow the patch set version.

 Documentation/devicetree/bindings/pci/designware-pcie.txt | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt b/Documentation/devicetree/bindings/pci/designware-pcie.txt
index 7f9804d..c124f9b 100644
--- a/Documentation/devicetree/bindings/pci/designware-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt
@@ -3,6 +3,7 @@
 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.
     (The old way of getting the configuration address space from "ranges"
@@ -56,3 +57,14 @@ Example configuration:
 		#interrupt-cells = <1>;
 		num-lanes = <1>;
 	};
+or
+	pcie: pcie@dfc00000 {
+		compatible = "snps,dw-pcie-ep";
+		reg = <0xdfc00000 0x0001000>, /* IP registers 1 */
+		      <0xdfc01000 0x0001000>, /* IP registers 2 */
+		      <0xd0000000 0x2000000>; /* Configuration space */
+		reg-names = "dbi", "dbi2", "addr_space";
+		num-ib-windows = <6>;
+		num-ob-windows = <2>;
+		num-lanes = <1>;
+	};
-- 
2.7.4

[PATCH v7 5/9] misc: pci_endpoint_test: Add designware EP entry

[Gustavo Pimentel]

Adds the designware EP device ID entry to pci_endpoint_test driver table
to allow this device to be recognize and handle by the pci_endpoint_test
driver.

Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
Acked-by: Kishon Vijay Abraham I <kishon@ti.com>
---
Change v1->v2:
 - Changed device id following Kishon's suggestion.
Change v2->v3:
 - Nothing changed, just to follow the patch set version.
Changes v3->v4:
 - Nothing changed, just to follow the patch set version.
Changes v4->v5:
 - Nothing changed, just to follow the patch set version.
Changes v5->v6:
 - Reverted Kishon's suggestion by Bjorn request.
Changes v6->v7:
 - Nothing changed, just to follow the patch set version.

 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 fe8897e..58a88ba 100644
--- a/drivers/misc/pci_endpoint_test.c
+++ b/drivers/misc/pci_endpoint_test.c
@@ -634,6 +634,7 @@ static void pci_endpoint_test_remove(struct pci_dev *pdev)
 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_SYNOPSYS, 0xedda) },
 	{ }
 };
 MODULE_DEVICE_TABLE(pci, pci_endpoint_test_tbl);
-- 
2.7.4

[PATCH v7 6/9] PCI: dwc: Define maximum number of vectors

[Gustavo Pimentel]

Adds a callback that defines the maximum number of vectors that can be use
by the Root Complex.

Since this is a parameter associated to each SoC IP setting, makes sense to
be configurable and easily visible to future modifications.

The designware IP supports a maximum of 256 vectors.

Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
Acked-by: Joao Pinto <jpinto@synopsys.com>
---
Change v1->v2:
 - Nothing changed, just to follow the patch set version.
Change v2->v3:
 - Nothing changed, just to follow the patch set version.
Changes v3->v4:
 - Nothing changed, just to follow the patch set version.
Changes v4->v5:
 - Nothing changed, just to follow the patch set version.
Changes v5->v6:
 - Nothing changed, just to follow the patch set version.
Changes v6->v7:
 - Nothing changed, just to follow the patch set version.

 drivers/pci/dwc/pcie-designware-plat.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/pci/dwc/pcie-designware-plat.c b/drivers/pci/dwc/pcie-designware-plat.c
index efc315c..5937fed 100644
--- a/drivers/pci/dwc/pcie-designware-plat.c
+++ b/drivers/pci/dwc/pcie-designware-plat.c
@@ -48,8 +48,14 @@ static int dw_plat_pcie_host_init(struct pcie_port *pp)
 	return 0;
 }
 
+static void dw_plat_set_num_vectors(struct pcie_port *pp)
+{
+	pp->num_vectors = MAX_MSI_IRQS;
+}
+
 static const struct dw_pcie_host_ops dw_plat_pcie_host_ops = {
 	.host_init = dw_plat_pcie_host_init,
+	.set_num_vectors = dw_plat_set_num_vectors,
 };
 
 static int dw_plat_pcie_establish_link(struct dw_pcie *pci)
-- 
2.7.4

Re: [PATCH v7 6/9] PCI: dwc: Define maximum number of vectors

[Jingoo Han]

On Tuesday, April 24, 2018 9:45 AM, Gustavo Pimentel wrote:
> 
> Adds a callback that defines the maximum number of vectors that can be use
> by the Root Complex.
> 
> Since this is a parameter associated to each SoC IP setting, makes sense
> to
> be configurable and easily visible to future modifications.
> 
> The designware IP supports a maximum of 256 vectors.
> 
> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> Acked-by: Joao Pinto <jpinto@synopsys.com>

Acked-by: Jingoo Han <jingoohan1@gmail.com>

Best regards,
Jingoo Han


> ---
> Change v1->v2:
>  - Nothing changed, just to follow the patch set version.
> Change v2->v3:
>  - Nothing changed, just to follow the patch set version.
> Changes v3->v4:
>  - Nothing changed, just to follow the patch set version.
> Changes v4->v5:
>  - Nothing changed, just to follow the patch set version.
> Changes v5->v6:
>  - Nothing changed, just to follow the patch set version.
> Changes v6->v7:
>  - Nothing changed, just to follow the patch set version.
> 
>  drivers/pci/dwc/pcie-designware-plat.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/pci/dwc/pcie-designware-plat.c
> b/drivers/pci/dwc/pcie-designware-plat.c
> index efc315c..5937fed 100644
> --- a/drivers/pci/dwc/pcie-designware-plat.c
> +++ b/drivers/pci/dwc/pcie-designware-plat.c
> @@ -48,8 +48,14 @@ static int dw_plat_pcie_host_init(struct pcie_port *pp)
>  	return 0;
>  }
> 
> +static void dw_plat_set_num_vectors(struct pcie_port *pp)
> +{
> +	pp->num_vectors = MAX_MSI_IRQS;
> +}
> +
>  static const struct dw_pcie_host_ops dw_plat_pcie_host_ops = {
>  	.host_init = dw_plat_pcie_host_init,
> +	.set_num_vectors = dw_plat_set_num_vectors,
>  };
> 
>  static int dw_plat_pcie_establish_link(struct dw_pcie *pci)
> --
> 2.7.4
> 

Re: [PATCH v7 6/9] PCI: dwc: Define maximum number of vectors

[Jingoo Han]

On Tuesday, April 24, 2018 9:45 AM, Gustavo Pimentel wrote:
> 
> Adds a callback that defines the maximum number of vectors that can be use
> by the Root Complex.
> 
> Since this is a parameter associated to each SoC IP setting, makes sense
> to
> be configurable and easily visible to future modifications.
> 
> The designware IP supports a maximum of 256 vectors.
> 
> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> Acked-by: Joao Pinto <jpinto@synopsys.com>

Acked-by: Jingoo Han <jingoohan1@gmail.com>

Best regards,
Jingoo Han


> ---
> Change v1->v2:
>  - Nothing changed, just to follow the patch set version.
> Change v2->v3:
>  - Nothing changed, just to follow the patch set version.
> Changes v3->v4:
>  - Nothing changed, just to follow the patch set version.
> Changes v4->v5:
>  - Nothing changed, just to follow the patch set version.
> Changes v5->v6:
>  - Nothing changed, just to follow the patch set version.
> Changes v6->v7:
>  - Nothing changed, just to follow the patch set version.
> 
>  drivers/pci/dwc/pcie-designware-plat.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/pci/dwc/pcie-designware-plat.c
> b/drivers/pci/dwc/pcie-designware-plat.c
> index efc315c..5937fed 100644
> --- a/drivers/pci/dwc/pcie-designware-plat.c
> +++ b/drivers/pci/dwc/pcie-designware-plat.c
> @@ -48,8 +48,14 @@ static int dw_plat_pcie_host_init(struct pcie_port *pp)
>  	return 0;
>  }
> 
> +static void dw_plat_set_num_vectors(struct pcie_port *pp)
> +{
> +	pp->num_vectors = MAX_MSI_IRQS;
> +}
> +
>  static const struct dw_pcie_host_ops dw_plat_pcie_host_ops = {
>  	.host_init = dw_plat_pcie_host_init,
> +	.set_num_vectors = dw_plat_set_num_vectors,
>  };
> 
>  static int dw_plat_pcie_establish_link(struct dw_pcie *pci)
> --
> 2.7.4
> 

[PATCH v7 7/9] PCI: dwc: Replace lower into upper case characters

[Gustavo Pimentel]

Replaces lower into upper case characters in comments and debug printks.

This is an attempt to keep the messages coherent within the designware
driver.

Also fixed code style on dw_pcie_irq_domain_free function.

Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
Acked-by: Jingoo Han <jingoohan1@gmail.com>
Acked-by: Joao Pinto <jpinto@synopsys.com>
---
Change v1->v2:
 - Added an extra log description line about code style following Fabio
Estevam suggestion.
Change v2->v3:
 - Nothing changed, just to follow the patch set version.
Changes v3->v4:
 - Nothing changed, just to follow the patch set version.
Changes v4->v5:
 - Nothing changed, just to follow the patch set version.
Changes v5->v6:
 - Nothing changed, just to follow the patch set version.
Changes v6->v7:
 - Nothing changed, just to follow the patch set version.

 drivers/pci/dwc/pcie-designware-ep.c   | 16 ++++++++--------
 drivers/pci/dwc/pcie-designware-host.c | 35 ++++++++++++++++++----------------
 drivers/pci/dwc/pcie-designware.c      | 22 ++++++++++-----------
 3 files changed, 38 insertions(+), 35 deletions(-)

diff --git a/drivers/pci/dwc/pcie-designware-ep.c b/drivers/pci/dwc/pcie-designware-ep.c
index f07678b..15b22a6 100644
--- a/drivers/pci/dwc/pcie-designware-ep.c
+++ b/drivers/pci/dwc/pcie-designware-ep.c
@@ -75,7 +75,7 @@ static int dw_pcie_ep_inbound_atu(struct dw_pcie_ep *ep, enum pci_barno bar,
 
 	free_win = find_first_zero_bit(ep->ib_window_map, ep->num_ib_windows);
 	if (free_win >= ep->num_ib_windows) {
-		dev_err(pci->dev, "no free inbound window\n");
+		dev_err(pci->dev, "No free inbound window\n");
 		return -EINVAL;
 	}
 
@@ -100,7 +100,7 @@ static int dw_pcie_ep_outbound_atu(struct dw_pcie_ep *ep, phys_addr_t phys_addr,
 
 	free_win = find_first_zero_bit(ep->ob_window_map, ep->num_ob_windows);
 	if (free_win >= ep->num_ob_windows) {
-		dev_err(pci->dev, "no free outbound window\n");
+		dev_err(pci->dev, "No free outbound window\n");
 		return -EINVAL;
 	}
 
@@ -204,7 +204,7 @@ static int dw_pcie_ep_map_addr(struct pci_epc *epc, u8 func_no,
 
 	ret = dw_pcie_ep_outbound_atu(ep, addr, pci_addr, size);
 	if (ret) {
-		dev_err(pci->dev, "failed to enable address\n");
+		dev_err(pci->dev, "Failed to enable address\n");
 		return ret;
 	}
 
@@ -348,21 +348,21 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
 
 	ret = of_property_read_u32(np, "num-ib-windows", &ep->num_ib_windows);
 	if (ret < 0) {
-		dev_err(dev, "unable to read *num-ib-windows* property\n");
+		dev_err(dev, "Unable to read *num-ib-windows* property\n");
 		return ret;
 	}
 	if (ep->num_ib_windows > MAX_IATU_IN) {
-		dev_err(dev, "invalid *num-ib-windows*\n");
+		dev_err(dev, "Invalid *num-ib-windows*\n");
 		return -EINVAL;
 	}
 
 	ret = of_property_read_u32(np, "num-ob-windows", &ep->num_ob_windows);
 	if (ret < 0) {
-		dev_err(dev, "unable to read *num-ob-windows* property\n");
+		dev_err(dev, "Unable to read *num-ob-windows* property\n");
 		return ret;
 	}
 	if (ep->num_ob_windows > MAX_IATU_OUT) {
-		dev_err(dev, "invalid *num-ob-windows*\n");
+		dev_err(dev, "Invalid *num-ob-windows*\n");
 		return -EINVAL;
 	}
 
@@ -389,7 +389,7 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
 
 	epc = devm_pci_epc_create(dev, &epc_ops);
 	if (IS_ERR(epc)) {
-		dev_err(dev, "failed to create epc device\n");
+		dev_err(dev, "Failed to create epc device\n");
 		return PTR_ERR(epc);
 	}
 
diff --git a/drivers/pci/dwc/pcie-designware-host.c b/drivers/pci/dwc/pcie-designware-host.c
index 6c409079..5a23f78 100644
--- a/drivers/pci/dwc/pcie-designware-host.c
+++ b/drivers/pci/dwc/pcie-designware-host.c
@@ -248,8 +248,10 @@ static void dw_pcie_irq_domain_free(struct irq_domain *domain,
 	unsigned long flags;
 
 	raw_spin_lock_irqsave(&pp->lock, flags);
+
 	bitmap_release_region(pp->msi_irq_in_use, data->hwirq,
 			      order_base_2(nr_irqs));
+
 	raw_spin_unlock_irqrestore(&pp->lock, flags);
 }
 
@@ -266,7 +268,7 @@ int dw_pcie_allocate_domains(struct pcie_port *pp)
 	pp->irq_domain = irq_domain_create_linear(fwnode, pp->num_vectors,
 					       &dw_pcie_msi_domain_ops, pp);
 	if (!pp->irq_domain) {
-		dev_err(pci->dev, "failed to create IRQ domain\n");
+		dev_err(pci->dev, "Failed to create IRQ domain\n");
 		return -ENOMEM;
 	}
 
@@ -274,7 +276,7 @@ int dw_pcie_allocate_domains(struct pcie_port *pp)
 						   &dw_pcie_msi_domain_info,
 						   pp->irq_domain);
 	if (!pp->msi_domain) {
-		dev_err(pci->dev, "failed to create MSI domain\n");
+		dev_err(pci->dev, "Failed to create MSI domain\n");
 		irq_domain_remove(pp->irq_domain);
 		return -ENOMEM;
 	}
@@ -301,13 +303,13 @@ void dw_pcie_msi_init(struct pcie_port *pp)
 	page = alloc_page(GFP_KERNEL);
 	pp->msi_data = dma_map_page(dev, page, 0, PAGE_SIZE, DMA_FROM_DEVICE);
 	if (dma_mapping_error(dev, pp->msi_data)) {
-		dev_err(dev, "failed to map MSI data\n");
+		dev_err(dev, "Failed to map MSI data\n");
 		__free_page(page);
 		return;
 	}
 	msi_target = (u64)pp->msi_data;
 
-	/* program the msi_data */
+	/* Program the msi_data */
 	dw_pcie_wr_own_conf(pp, PCIE_MSI_ADDR_LO, 4,
 			    lower_32_bits(msi_target));
 	dw_pcie_wr_own_conf(pp, PCIE_MSI_ADDR_HI, 4,
@@ -335,7 +337,7 @@ int dw_pcie_host_init(struct pcie_port *pp)
 		pp->cfg0_base = cfg_res->start;
 		pp->cfg1_base = cfg_res->start + pp->cfg0_size;
 	} else if (!pp->va_cfg0_base) {
-		dev_err(dev, "missing *config* reg space\n");
+		dev_err(dev, "Missing *config* reg space\n");
 	}
 
 	bridge = pci_alloc_host_bridge(0);
@@ -357,7 +359,7 @@ int dw_pcie_host_init(struct pcie_port *pp)
 		case IORESOURCE_IO:
 			ret = pci_remap_iospace(win->res, pp->io_base);
 			if (ret) {
-				dev_warn(dev, "error %d: failed to map resource %pR\n",
+				dev_warn(dev, "Error %d: failed to map resource %pR\n",
 					 ret, win->res);
 				resource_list_destroy_entry(win);
 			} else {
@@ -391,7 +393,7 @@ int dw_pcie_host_init(struct pcie_port *pp)
 						pp->cfg->start,
 						resource_size(pp->cfg));
 		if (!pci->dbi_base) {
-			dev_err(dev, "error with ioremap\n");
+			dev_err(dev, "Error with ioremap\n");
 			ret = -ENOMEM;
 			goto error;
 		}
@@ -403,7 +405,7 @@ int dw_pcie_host_init(struct pcie_port *pp)
 		pp->va_cfg0_base = devm_pci_remap_cfgspace(dev,
 					pp->cfg0_base, pp->cfg0_size);
 		if (!pp->va_cfg0_base) {
-			dev_err(dev, "error with ioremap in function\n");
+			dev_err(dev, "Error with ioremap in function\n");
 			ret = -ENOMEM;
 			goto error;
 		}
@@ -414,7 +416,7 @@ int dw_pcie_host_init(struct pcie_port *pp)
 						pp->cfg1_base,
 						pp->cfg1_size);
 		if (!pp->va_cfg1_base) {
-			dev_err(dev, "error with ioremap\n");
+			dev_err(dev, "Error with ioremap\n");
 			ret = -ENOMEM;
 			goto error;
 		}
@@ -586,7 +588,7 @@ static int dw_pcie_valid_device(struct pcie_port *pp, struct pci_bus *bus,
 			return 0;
 	}
 
-	/* access only one slot on each root port */
+	/* Access only one slot on each root port */
 	if (bus->number == pp->root_bus_nr && dev > 0)
 		return 0;
 
@@ -652,11 +654,12 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
 	for (ctrl = 0; ctrl < num_ctrls; ctrl++)
 		dw_pcie_rd_own_conf(pp, PCIE_MSI_INTR0_ENABLE + (ctrl * 12), 4,
 				    &pp->irq_status[ctrl]);
-	/* setup RC BARs */
+
+	/* Setup RC BARs */
 	dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_0, 0x00000004);
 	dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_1, 0x00000000);
 
-	/* setup interrupt pins */
+	/* Setup interrupt pins */
 	dw_pcie_dbi_ro_wr_en(pci);
 	val = dw_pcie_readl_dbi(pci, PCI_INTERRUPT_LINE);
 	val &= 0xffff00ff;
@@ -664,13 +667,13 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
 	dw_pcie_writel_dbi(pci, PCI_INTERRUPT_LINE, val);
 	dw_pcie_dbi_ro_wr_dis(pci);
 
-	/* setup bus numbers */
+	/* Setup bus numbers */
 	val = dw_pcie_readl_dbi(pci, PCI_PRIMARY_BUS);
 	val &= 0xff000000;
 	val |= 0x00ff0100;
 	dw_pcie_writel_dbi(pci, PCI_PRIMARY_BUS, val);
 
-	/* setup command register */
+	/* Setup command register */
 	val = dw_pcie_readl_dbi(pci, PCI_COMMAND);
 	val &= 0xffff0000;
 	val |= PCI_COMMAND_IO | PCI_COMMAND_MEMORY |
@@ -683,7 +686,7 @@ 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 */
+		/* 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");
@@ -701,7 +704,7 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
 
 	/* Enable write permission for the DBI read-only register */
 	dw_pcie_dbi_ro_wr_en(pci);
-	/* program correct class for RC */
+	/* Program correct class for RC */
 	dw_pcie_wr_own_conf(pp, PCI_CLASS_DEVICE, 2, PCI_CLASS_BRIDGE_PCI);
 	/* Better disable write permission right after the update */
 	dw_pcie_dbi_ro_wr_dis(pci);
diff --git a/drivers/pci/dwc/pcie-designware.c b/drivers/pci/dwc/pcie-designware.c
index 1b7282e..778c4f7 100644
--- a/drivers/pci/dwc/pcie-designware.c
+++ b/drivers/pci/dwc/pcie-designware.c
@@ -69,7 +69,7 @@ u32 __dw_pcie_read_dbi(struct dw_pcie *pci, void __iomem *base, u32 reg,
 
 	ret = dw_pcie_read(base + reg, size, &val);
 	if (ret)
-		dev_err(pci->dev, "read DBI address failed\n");
+		dev_err(pci->dev, "Read DBI address failed\n");
 
 	return val;
 }
@@ -86,7 +86,7 @@ void __dw_pcie_write_dbi(struct dw_pcie *pci, void __iomem *base, u32 reg,
 
 	ret = dw_pcie_write(base + reg, size, val);
 	if (ret)
-		dev_err(pci->dev, "write DBI address failed\n");
+		dev_err(pci->dev, "Write DBI address failed\n");
 }
 
 static u32 dw_pcie_readl_ob_unroll(struct dw_pcie *pci, u32 index, u32 reg)
@@ -137,7 +137,7 @@ static void dw_pcie_prog_outbound_atu_unroll(struct dw_pcie *pci, int index,
 
 		usleep_range(LINK_WAIT_IATU_MIN, LINK_WAIT_IATU_MAX);
 	}
-	dev_err(pci->dev, "outbound iATU is not being enabled\n");
+	dev_err(pci->dev, "Outbound iATU is not being enabled\n");
 }
 
 void dw_pcie_prog_outbound_atu(struct dw_pcie *pci, int index, int type,
@@ -180,7 +180,7 @@ void dw_pcie_prog_outbound_atu(struct dw_pcie *pci, int index, int type,
 
 		usleep_range(LINK_WAIT_IATU_MIN, LINK_WAIT_IATU_MAX);
 	}
-	dev_err(pci->dev, "outbound iATU is not being enabled\n");
+	dev_err(pci->dev, "Outbound iATU is not being enabled\n");
 }
 
 static u32 dw_pcie_readl_ib_unroll(struct dw_pcie *pci, u32 index, u32 reg)
@@ -238,7 +238,7 @@ static int dw_pcie_prog_inbound_atu_unroll(struct dw_pcie *pci, int index,
 
 		usleep_range(LINK_WAIT_IATU_MIN, LINK_WAIT_IATU_MAX);
 	}
-	dev_err(pci->dev, "inbound iATU is not being enabled\n");
+	dev_err(pci->dev, "Inbound iATU is not being enabled\n");
 
 	return -EBUSY;
 }
@@ -284,7 +284,7 @@ int dw_pcie_prog_inbound_atu(struct dw_pcie *pci, int index, int bar,
 
 		usleep_range(LINK_WAIT_IATU_MIN, LINK_WAIT_IATU_MAX);
 	}
-	dev_err(pci->dev, "inbound iATU is not being enabled\n");
+	dev_err(pci->dev, "Inbound iATU is not being enabled\n");
 
 	return -EBUSY;
 }
@@ -313,16 +313,16 @@ int dw_pcie_wait_for_link(struct dw_pcie *pci)
 {
 	int retries;
 
-	/* check if the link is up or not */
+	/* Check if the link is up or not */
 	for (retries = 0; retries < LINK_WAIT_MAX_RETRIES; retries++) {
 		if (dw_pcie_link_up(pci)) {
-			dev_info(pci->dev, "link up\n");
+			dev_info(pci->dev, "Link up\n");
 			return 0;
 		}
 		usleep_range(LINK_WAIT_USLEEP_MIN, LINK_WAIT_USLEEP_MAX);
 	}
 
-	dev_err(pci->dev, "phy link never came up\n");
+	dev_err(pci->dev, "Phy link never came up\n");
 
 	return -ETIMEDOUT;
 }
@@ -351,7 +351,7 @@ void dw_pcie_setup(struct dw_pcie *pci)
 	if (ret)
 		lanes = 0;
 
-	/* set the number of lanes */
+	/* Set the number of lanes */
 	val = dw_pcie_readl_dbi(pci, PCIE_PORT_LINK_CONTROL);
 	val &= ~PORT_LINK_MODE_MASK;
 	switch (lanes) {
@@ -373,7 +373,7 @@ void dw_pcie_setup(struct dw_pcie *pci)
 	}
 	dw_pcie_writel_dbi(pci, PCIE_PORT_LINK_CONTROL, val);
 
-	/* set link width speed control register */
+	/* Set link width speed control register */
 	val = dw_pcie_readl_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL);
 	val &= ~PORT_LOGIC_LINK_WIDTH_MASK;
 	switch (lanes) {
-- 
2.7.4

[PATCH v7 8/9] PCI: dwc: Small computation improvement

[Gustavo Pimentel]

Replaces a simple division by 2 to a right shift rotation of 1 bit.

Probably any recent and decent compiler does this kind of substitution
in order to improve code performance. Nevertheless it's a coding good
practice whenever there is a division / multiplication by multiple of 2
to replace it by the equivalent operation in this case, the shift
rotation.

Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
Acked-by: Jingoo Han <jingoohan1@gmail.com>
Acked-by: Joao Pinto <jpinto@synopsys.com>
---
Change v1->v2:
 - Nothing changed, just to follow the patch set version.
Change v2->v3:
 - Nothing changed, just to follow the patch set version.
Changes v3->v4:
 - Added a small explication to the log description.
Changes v4->v5:
 - Nothing changed, just to follow the patch set version.
Changes v5->v6:
 - Nothing changed, just to follow the patch set version.
Changes v6->v7:
 - Nothing changed, just to follow the patch set version.

 drivers/pci/dwc/pcie-designware-host.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/dwc/pcie-designware-host.c b/drivers/pci/dwc/pcie-designware-host.c
index 5a23f78..fc55fde 100644
--- a/drivers/pci/dwc/pcie-designware-host.c
+++ b/drivers/pci/dwc/pcie-designware-host.c
@@ -332,8 +332,8 @@ int dw_pcie_host_init(struct pcie_port *pp)
 
 	cfg_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "config");
 	if (cfg_res) {
-		pp->cfg0_size = resource_size(cfg_res) / 2;
-		pp->cfg1_size = resource_size(cfg_res) / 2;
+		pp->cfg0_size = resource_size(cfg_res) >> 1;
+		pp->cfg1_size = resource_size(cfg_res) >> 1;
 		pp->cfg0_base = cfg_res->start;
 		pp->cfg1_base = cfg_res->start + pp->cfg0_size;
 	} else if (!pp->va_cfg0_base) {
@@ -377,8 +377,8 @@ int dw_pcie_host_init(struct pcie_port *pp)
 			break;
 		case 0:
 			pp->cfg = win->res;
-			pp->cfg0_size = resource_size(pp->cfg) / 2;
-			pp->cfg1_size = resource_size(pp->cfg) / 2;
+			pp->cfg0_size = resource_size(pp->cfg) >> 1;
+			pp->cfg1_size = resource_size(pp->cfg) >> 1;
 			pp->cfg0_base = pp->cfg->start;
 			pp->cfg1_base = pp->cfg->start + pp->cfg0_size;
 			break;
-- 
2.7.4

[PATCH v7 9/9] PCI: dwc: Replace magic number by defines

[Gustavo Pimentel]

Replace magic numbers by a well known define in order to make the code
human readable and also facilitate the code reusability.

Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
Acked-by: Jingoo Han <jingoohan1@gmail.com>
---
Change v1->v2:
 - Nothing changed, just to follow the patch set version.
Change v2->v3:
 - Nothing changed, just to follow the patch set version.
Changes v3->v4:
 - Nothing changed, just to follow the patch set version.
Changes v4->v5:
 - Nothing changed, just to follow the patch set version.
Changes v5->v6:
 - Nothing changed, just to follow the patch set version.
Changes v6->v7:
 - Nothing changed, just to follow the patch set version.

 drivers/pci/dwc/pcie-designware-host.c | 34 ++++++++++++++++++++--------------
 drivers/pci/dwc/pcie-designware.h      |  1 +
 2 files changed, 21 insertions(+), 14 deletions(-)

diff --git a/drivers/pci/dwc/pcie-designware-host.c b/drivers/pci/dwc/pcie-designware-host.c
index fc55fde..a7657ab 100644
--- a/drivers/pci/dwc/pcie-designware-host.c
+++ b/drivers/pci/dwc/pcie-designware-host.c
@@ -83,18 +83,23 @@ irqreturn_t dw_handle_msi_irq(struct pcie_port *pp)
 	num_ctrls = pp->num_vectors / MAX_MSI_IRQS_PER_CTRL;
 
 	for (i = 0; i < num_ctrls; i++) {
-		dw_pcie_rd_own_conf(pp, PCIE_MSI_INTR0_STATUS + i * 12, 4,
-				    &val);
+		dw_pcie_rd_own_conf(pp, PCIE_MSI_INTR0_STATUS +
+					(i * MSI_REG_CTRL_BLOCK_SIZE),
+				    4, &val);
 		if (!val)
 			continue;
 
 		ret = IRQ_HANDLED;
 		pos = 0;
-		while ((pos = find_next_bit((unsigned long *) &val, 32,
-					    pos)) != 32) {
-			irq = irq_find_mapping(pp->irq_domain, i * 32 + pos);
+		while ((pos = find_next_bit((unsigned long *) &val,
+					    MAX_MSI_IRQS_PER_CTRL,
+					    pos)) != MAX_MSI_IRQS_PER_CTRL) {
+			irq = irq_find_mapping(pp->irq_domain,
+					       (i * MAX_MSI_IRQS_PER_CTRL) +
+					       pos);
 			generic_handle_irq(irq);
-			dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_STATUS + i * 12,
+			dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_STATUS +
+						(i * MSI_REG_CTRL_BLOCK_SIZE),
 					    4, 1 << pos);
 			pos++;
 		}
@@ -157,9 +162,9 @@ static void dw_pci_bottom_mask(struct irq_data *data)
 	if (pp->ops->msi_clear_irq) {
 		pp->ops->msi_clear_irq(pp, data->hwirq);
 	} else {
-		ctrl = data->hwirq / 32;
-		res = ctrl * 12;
-		bit = data->hwirq % 32;
+		ctrl = data->hwirq / MAX_MSI_IRQS_PER_CTRL;
+		res = ctrl * MSI_REG_CTRL_BLOCK_SIZE;
+		bit = data->hwirq % MAX_MSI_IRQS_PER_CTRL;
 
 		pp->irq_status[ctrl] &= ~(1 << bit);
 		dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_ENABLE + res, 4,
@@ -180,9 +185,9 @@ static void dw_pci_bottom_unmask(struct irq_data *data)
 	if (pp->ops->msi_set_irq) {
 		pp->ops->msi_set_irq(pp, data->hwirq);
 	} else {
-		ctrl = data->hwirq / 32;
-		res = ctrl * 12;
-		bit = data->hwirq % 32;
+		ctrl = data->hwirq / MAX_MSI_IRQS_PER_CTRL;
+		res = ctrl * MSI_REG_CTRL_BLOCK_SIZE;
+		bit = data->hwirq % MAX_MSI_IRQS_PER_CTRL;
 
 		pp->irq_status[ctrl] |= 1 << bit;
 		dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_ENABLE + res, 4,
@@ -652,8 +657,9 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
 
 	/* Initialize IRQ Status array */
 	for (ctrl = 0; ctrl < num_ctrls; ctrl++)
-		dw_pcie_rd_own_conf(pp, PCIE_MSI_INTR0_ENABLE + (ctrl * 12), 4,
-				    &pp->irq_status[ctrl]);
+		dw_pcie_rd_own_conf(pp, PCIE_MSI_INTR0_ENABLE +
+					(ctrl * MSI_REG_CTRL_BLOCK_SIZE),
+				    4, &pp->irq_status[ctrl]);
 
 	/* Setup RC BARs */
 	dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_0, 0x00000004);
diff --git a/drivers/pci/dwc/pcie-designware.h b/drivers/pci/dwc/pcie-designware.h
index fe811db..bee4e25 100644
--- a/drivers/pci/dwc/pcie-designware.h
+++ b/drivers/pci/dwc/pcie-designware.h
@@ -110,6 +110,7 @@
 #define MAX_MSI_IRQS			256
 #define MAX_MSI_IRQS_PER_CTRL		32
 #define MAX_MSI_CTRLS			(MAX_MSI_IRQS / MAX_MSI_IRQS_PER_CTRL)
+#define MSI_REG_CTRL_BLOCK_SIZE		12
 #define MSI_DEF_NUM_VECTORS		32
 
 /* Maximum number of inbound/outbound iATUs */
-- 
2.7.4