[PATCH 00/10] dwc MSI fixes, ARTPEC-6 EP mode support, ARTPEC-7 SoC support

All of lore.kernel.org
 help / color / mirror / Atom feed

* [PATCH 00/10] dwc MSI fixes, ARTPEC-6 EP mode support, ARTPEC-7 SoC support
@ 2017-10-13 16:09 ` Niklas Cassel
  0 siblings, 0 replies; 41+ messages in thread
From: Niklas Cassel @ 2017-10-13 16:09 UTC (permalink / raw)
  To: linux-arm-kernel-VrBV9hrLPhE, linux-pci-u79uwXL29TY76Z2rM5mHXA,
	linux-omap-u79uwXL29TY76Z2rM5mHXA
  Cc: Niklas Cassel, devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA

This is a series that adds:
- PCI endpoint mode support in the ARTPEC-6 driver.
- ARTPEC-7 SoC support in the ARTPEC-6 driver (the SoCs are very similar).
- Small fixes for MSI in designware-ep and designware-host,
  needed to get endpoint mode support working for ARTPEC-6.

Niklas Cassel (10):
  PCI: dwc: use DMA-API for allocating MSI data
  PCI: designware-ep: set_msi should only set MMC bits
  PCI: designware-ep: read-only registers need DBI_RO_WR_EN to be
    writable
  PCI: designware-ep: pre-allocate memory for MSI in dw_pcie_ep_init
  PCI: artpec6: remove unused defines
  PCI: dwc: artpec6: use BIT and GENMASK macros
  PCI: dwc: artpec6: split artpec6_pcie_establish_link to smaller
    functions
  PCI: dwc: artpec6: add support for endpoint mode
  PCI: dwc: make cpu_addr_fixup take struct dw_pcie as argument
  PCI: dwc: artpec6: add support for the ARTPEC-7 SoC

 .../devicetree/bindings/pci/axis,artpec6-pcie.txt  |   5 +-
 drivers/pci/dwc/Kconfig                            |  41 +-
 drivers/pci/dwc/Makefile                           |   4 +-
 drivers/pci/dwc/pci-dra7xx.c                       |   2 +-
 drivers/pci/dwc/pcie-artpec6.c                     | 482 +++++++++++++++++++--
 drivers/pci/dwc/pcie-designware-ep.c               |  20 +-
 drivers/pci/dwc/pcie-designware-host.c             |  23 +-
 drivers/pci/dwc/pcie-designware.c                  |   2 +-
 drivers/pci/dwc/pcie-designware.h                  |   8 +-
 9 files changed, 514 insertions(+), 73 deletions(-)

-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 41+ messages in thread

* [PATCH 00/10] dwc MSI fixes, ARTPEC-6 EP mode support, ARTPEC-7 SoC support
@ 2017-10-13 16:09 ` Niklas Cassel
  0 siblings, 0 replies; 41+ messages in thread
From: Niklas Cassel @ 2017-10-13 16:09 UTC (permalink / raw)
  To: linux-arm-kernel, linux-pci, linux-omap
  Cc: Niklas Cassel, devicetree, linux-kernel

This is a series that adds:
- PCI endpoint mode support in the ARTPEC-6 driver.
- ARTPEC-7 SoC support in the ARTPEC-6 driver (the SoCs are very similar).
- Small fixes for MSI in designware-ep and designware-host,
  needed to get endpoint mode support working for ARTPEC-6.

Niklas Cassel (10):
  PCI: dwc: use DMA-API for allocating MSI data
  PCI: designware-ep: set_msi should only set MMC bits
  PCI: designware-ep: read-only registers need DBI_RO_WR_EN to be
    writable
  PCI: designware-ep: pre-allocate memory for MSI in dw_pcie_ep_init
  PCI: artpec6: remove unused defines
  PCI: dwc: artpec6: use BIT and GENMASK macros
  PCI: dwc: artpec6: split artpec6_pcie_establish_link to smaller
    functions
  PCI: dwc: artpec6: add support for endpoint mode
  PCI: dwc: make cpu_addr_fixup take struct dw_pcie as argument
  PCI: dwc: artpec6: add support for the ARTPEC-7 SoC

 .../devicetree/bindings/pci/axis,artpec6-pcie.txt  |   5 +-
 drivers/pci/dwc/Kconfig                            |  41 +-
 drivers/pci/dwc/Makefile                           |   4 +-
 drivers/pci/dwc/pci-dra7xx.c                       |   2 +-
 drivers/pci/dwc/pcie-artpec6.c                     | 482 +++++++++++++++++++--
 drivers/pci/dwc/pcie-designware-ep.c               |  20 +-
 drivers/pci/dwc/pcie-designware-host.c             |  23 +-
 drivers/pci/dwc/pcie-designware.c                  |   2 +-
 drivers/pci/dwc/pcie-designware.h                  |   8 +-
 9 files changed, 514 insertions(+), 73 deletions(-)

-- 
2.11.0

^ permalink raw reply	[flat|nested] 41+ messages in thread

* [PATCH 01/10] PCI: dwc: use DMA-API for allocating MSI data
  2017-10-13 16:09 ` Niklas Cassel
  (?)
@ 2017-10-13 16:09 ` Niklas Cassel
  2017-10-13 16:42   ` David Laight
                     ` (3 more replies)
  -1 siblings, 4 replies; 41+ messages in thread
From: Niklas Cassel @ 2017-10-13 16:09 UTC (permalink / raw)
  To: Jingoo Han, Joao Pinto, Bjorn Helgaas
  Cc: Niklas Cassel, linux-pci, linux-kernel

Since it is a PCIe endpoint device, rather than the CPU, that is supposed
to write to this location, the proper way to get the address to this this
location is really to use the DMA API, rather than virt_to_phys.

Using virt_to_phys might work on some systems, but by using the DMA API,
we know that it will work on all systems.

This is essentially the same thing as allocating a buffer in a driver,
to which the endpoint will write to. To do this, we use the DMA API.

Signed-off-by: Niklas Cassel <niklas.cassel@axis.com>
---
 drivers/pci/dwc/pcie-designware-host.c | 23 ++++++++++++++++-------
 drivers/pci/dwc/pcie-designware.h      |  3 ++-
 2 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/dwc/pcie-designware-host.c b/drivers/pci/dwc/pcie-designware-host.c
index 81e2157a7cfb..f6d152ea2a03 100644
--- a/drivers/pci/dwc/pcie-designware-host.c
+++ b/drivers/pci/dwc/pcie-designware-host.c
@@ -83,16 +83,25 @@ irqreturn_t dw_handle_msi_irq(struct pcie_port *pp)
 
 void dw_pcie_msi_init(struct pcie_port *pp)
 {
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+	struct device *dev = pci->dev;
+	struct page *page;
 	u64 msi_target;
 
-	pp->msi_data = __get_free_pages(GFP_KERNEL, 0);
-	msi_target = virt_to_phys((void *)pp->msi_data);
+	page = alloc_page(GFP_KERNEL | GFP_DMA32);
+	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");
+		__free_page(page);
+		return;
+	}
+	msi_target = (u64)pp->msi_data;
 
 	/* program the msi_data */
 	dw_pcie_wr_own_conf(pp, PCIE_MSI_ADDR_LO, 4,
-			    (u32)(msi_target & 0xffffffff));
+			    lower_32_bits(msi_target));
 	dw_pcie_wr_own_conf(pp, PCIE_MSI_ADDR_HI, 4,
-			    (u32)(msi_target >> 32 & 0xffffffff));
+			    upper_32_bits(msi_target));
 }
 
 static void dw_pcie_msi_clear_irq(struct pcie_port *pp, int irq)
@@ -187,10 +196,10 @@ static void dw_msi_setup_msg(struct pcie_port *pp, unsigned int irq, u32 pos)
 	if (pp->ops->get_msi_addr)
 		msi_target = pp->ops->get_msi_addr(pp);
 	else
-		msi_target = virt_to_phys((void *)pp->msi_data);
+		msi_target = (u64)pp->msi_data;
 
-	msg.address_lo = (u32)(msi_target & 0xffffffff);
-	msg.address_hi = (u32)(msi_target >> 32 & 0xffffffff);
+	msg.address_lo = lower_32_bits(msi_target);
+	msg.address_hi = upper_32_bits(msi_target);
 
 	if (pp->ops->get_msi_data)
 		msg.data = pp->ops->get_msi_data(pp, pos);
diff --git a/drivers/pci/dwc/pcie-designware.h b/drivers/pci/dwc/pcie-designware.h
index e5d9d77b778e..547352a317f8 100644
--- a/drivers/pci/dwc/pcie-designware.h
+++ b/drivers/pci/dwc/pcie-designware.h
@@ -17,6 +17,7 @@
 #include <linux/irq.h>
 #include <linux/msi.h>
 #include <linux/pci.h>
+#include <linux/dma-mapping.h>
 
 #include <linux/pci-epc.h>
 #include <linux/pci-epf.h>
@@ -168,7 +169,7 @@ struct pcie_port {
 	const struct dw_pcie_host_ops *ops;
 	int			msi_irq;
 	struct irq_domain	*irq_domain;
-	unsigned long		msi_data;
+	dma_addr_t		msi_data;
 	DECLARE_BITMAP(msi_irq_in_use, MAX_MSI_IRQS);
 };
 
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 41+ messages in thread

* RE: [PATCH 01/10] PCI: dwc: use DMA-API for allocating MSI data
  2017-10-13 16:09 ` [PATCH 01/10] PCI: dwc: use DMA-API for allocating MSI data Niklas Cassel
@ 2017-10-13 16:42   ` David Laight
  2017-10-16 11:43     ` Niklas Cassel
  2017-10-13 16:43   ` Jingoo Han
                     ` (2 subsequent siblings)
  3 siblings, 1 reply; 41+ messages in thread
From: David Laight @ 2017-10-13 16:42 UTC (permalink / raw)
  To: 'Niklas Cassel', Jingoo Han, Joao Pinto, Bjorn Helgaas
  Cc: Niklas Cassel, linux-pci@vger.kernel.org,
	linux-kernel@vger.kernel.org

From: Behalf Of Niklas Cassel
> Sent: 13 October 2017 17:09
> Since it is a PCIe endpoint device, rather than the CPU, that is supposed
> to write to this location, the proper way to get the address to this this
> location is really to use the DMA API, rather than virt_to_phys.
> 
> Using virt_to_phys might work on some systems, but by using the DMA API,
> we know that it will work on all systems.
> 
> This is essentially the same thing as allocating a buffer in a driver,
> to which the endpoint will write to. To do this, we use the DMA API.
> 
> Signed-off-by: Niklas Cassel <niklas.cassel@axis.com>
> ---
>  drivers/pci/dwc/pcie-designware-host.c | 23 ++++++++++++++++-------
>  drivers/pci/dwc/pcie-designware.h      |  3 ++-
>  2 files changed, 18 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pci/dwc/pcie-designware-host.c b/drivers/pci/dwc/pcie-designware-host.c
> index 81e2157a7cfb..f6d152ea2a03 100644
> --- a/drivers/pci/dwc/pcie-designware-host.c
> +++ b/drivers/pci/dwc/pcie-designware-host.c
> @@ -83,16 +83,25 @@ irqreturn_t dw_handle_msi_irq(struct pcie_port *pp)
> 
>  void dw_pcie_msi_init(struct pcie_port *pp)
>  {
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +	struct device *dev = pci->dev;
> +	struct page *page;
>  	u64 msi_target;
> 
> -	pp->msi_data = __get_free_pages(GFP_KERNEL, 0);
> -	msi_target = virt_to_phys((void *)pp->msi_data);
> +	page = alloc_page(GFP_KERNEL | GFP_DMA32);
> +	pp->msi_data = dma_map_page(dev, page, 0, PAGE_SIZE, DMA_FROM_DEVICE);

Where are the matching frees?

	David

> +	if (dma_mapping_error(dev, pp->msi_data)) {
> +		dev_err(dev, "failed to map msi data\n");
> +		__free_page(page);
> +		return;
> +	}
> +	msi_target = (u64)pp->msi_data;
> 
>  	/* program the msi_data */
>  	dw_pcie_wr_own_conf(pp, PCIE_MSI_ADDR_LO, 4,
> -			    (u32)(msi_target & 0xffffffff));
> +			    lower_32_bits(msi_target));
>  	dw_pcie_wr_own_conf(pp, PCIE_MSI_ADDR_HI, 4,
> -			    (u32)(msi_target >> 32 & 0xffffffff));
> +			    upper_32_bits(msi_target));
>  }
> 
>  static void dw_pcie_msi_clear_irq(struct pcie_port *pp, int irq)
> @@ -187,10 +196,10 @@ static void dw_msi_setup_msg(struct pcie_port *pp, unsigned int irq, u32 pos)
>  	if (pp->ops->get_msi_addr)
>  		msi_target = pp->ops->get_msi_addr(pp);
>  	else
> -		msi_target = virt_to_phys((void *)pp->msi_data);
> +		msi_target = (u64)pp->msi_data;
> 
> -	msg.address_lo = (u32)(msi_target & 0xffffffff);
> -	msg.address_hi = (u32)(msi_target >> 32 & 0xffffffff);
> +	msg.address_lo = lower_32_bits(msi_target);
> +	msg.address_hi = upper_32_bits(msi_target);
> 
>  	if (pp->ops->get_msi_data)
>  		msg.data = pp->ops->get_msi_data(pp, pos);
> diff --git a/drivers/pci/dwc/pcie-designware.h b/drivers/pci/dwc/pcie-designware.h
> index e5d9d77b778e..547352a317f8 100644
> --- a/drivers/pci/dwc/pcie-designware.h
> +++ b/drivers/pci/dwc/pcie-designware.h
> @@ -17,6 +17,7 @@
>  #include <linux/irq.h>
>  #include <linux/msi.h>
>  #include <linux/pci.h>
> +#include <linux/dma-mapping.h>
> 
>  #include <linux/pci-epc.h>
>  #include <linux/pci-epf.h>
> @@ -168,7 +169,7 @@ struct pcie_port {
>  	const struct dw_pcie_host_ops *ops;
>  	int			msi_irq;
>  	struct irq_domain	*irq_domain;
> -	unsigned long		msi_data;
> +	dma_addr_t		msi_data;
>  	DECLARE_BITMAP(msi_irq_in_use, MAX_MSI_IRQS);
>  };
> 
> --
> 2.11.0

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH 01/10] PCI: dwc: use DMA-API for allocating MSI data
  2017-10-13 16:42   ` David Laight
@ 2017-10-16 11:43     ` Niklas Cassel
  0 siblings, 0 replies; 41+ messages in thread
From: Niklas Cassel @ 2017-10-16 11:43 UTC (permalink / raw)
  To: David Laight, Jingoo Han, Joao Pinto, Bjorn Helgaas
  Cc: linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org

On 10/13/2017 06:42 PM, David Laight wrote:
> From: Behalf Of Niklas Cassel
>> Sent: 13 October 2017 17:09
>> Since it is a PCIe endpoint device, rather than the CPU, that is supposed
>> to write to this location, the proper way to get the address to this this
>> location is really to use the DMA API, rather than virt_to_phys.
>>
>> Using virt_to_phys might work on some systems, but by using the DMA API,
>> we know that it will work on all systems.
>>
>> This is essentially the same thing as allocating a buffer in a driver,
>> to which the endpoint will write to. To do this, we use the DMA API.
>>
>> Signed-off-by: Niklas Cassel <niklas.cassel@axis.com>
>> ---
>>  drivers/pci/dwc/pcie-designware-host.c | 23 ++++++++++++++++-------
>>  drivers/pci/dwc/pcie-designware.h      |  3 ++-
>>  2 files changed, 18 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/pci/dwc/pcie-designware-host.c b/drivers/pci/dwc/pcie-designware-host.c
>> index 81e2157a7cfb..f6d152ea2a03 100644
>> --- a/drivers/pci/dwc/pcie-designware-host.c
>> +++ b/drivers/pci/dwc/pcie-designware-host.c
>> @@ -83,16 +83,25 @@ irqreturn_t dw_handle_msi_irq(struct pcie_port *pp)
>>
>>  void dw_pcie_msi_init(struct pcie_port *pp)
>>  {
>> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>> +	struct device *dev = pci->dev;
>> +	struct page *page;
>>  	u64 msi_target;
>>
>> -	pp->msi_data = __get_free_pages(GFP_KERNEL, 0);
>> -	msi_target = virt_to_phys((void *)pp->msi_data);
>> +	page = alloc_page(GFP_KERNEL | GFP_DMA32);
>> +	pp->msi_data = dma_map_page(dev, page, 0, PAGE_SIZE, DMA_FROM_DEVICE);
> 
> Where are the matching frees?

Hello David.

I couldn't find any matching free_pages call to the
__get_free_pages() allocation, so following the same design,
I did not add a dma_unmap_page.

The current design of most (all?) DWC PCI drivers is that
they can only be built-in (i.e. not built as a module),
and that they cannot be unbinded (suppress_bind_attrs
= true in the platform_driver struct).

I will be happy to add a dma_unmap_page (and free_pages),
if the maintainer suggests somewhere where I should put it.

Regards,
Niklas

> 
> 	David
> 
>> +	if (dma_mapping_error(dev, pp->msi_data)) {
>> +		dev_err(dev, "failed to map msi data\n");
>> +		__free_page(page);
>> +		return;
>> +	}
>> +	msi_target = (u64)pp->msi_data;
>>
>>  	/* program the msi_data */
>>  	dw_pcie_wr_own_conf(pp, PCIE_MSI_ADDR_LO, 4,
>> -			    (u32)(msi_target & 0xffffffff));
>> +			    lower_32_bits(msi_target));
>>  	dw_pcie_wr_own_conf(pp, PCIE_MSI_ADDR_HI, 4,
>> -			    (u32)(msi_target >> 32 & 0xffffffff));
>> +			    upper_32_bits(msi_target));
>>  }
>>
>>  static void dw_pcie_msi_clear_irq(struct pcie_port *pp, int irq)
>> @@ -187,10 +196,10 @@ static void dw_msi_setup_msg(struct pcie_port *pp, unsigned int irq, u32 pos)
>>  	if (pp->ops->get_msi_addr)
>>  		msi_target = pp->ops->get_msi_addr(pp);
>>  	else
>> -		msi_target = virt_to_phys((void *)pp->msi_data);
>> +		msi_target = (u64)pp->msi_data;
>>
>> -	msg.address_lo = (u32)(msi_target & 0xffffffff);
>> -	msg.address_hi = (u32)(msi_target >> 32 & 0xffffffff);
>> +	msg.address_lo = lower_32_bits(msi_target);
>> +	msg.address_hi = upper_32_bits(msi_target);
>>
>>  	if (pp->ops->get_msi_data)
>>  		msg.data = pp->ops->get_msi_data(pp, pos);
>> diff --git a/drivers/pci/dwc/pcie-designware.h b/drivers/pci/dwc/pcie-designware.h
>> index e5d9d77b778e..547352a317f8 100644
>> --- a/drivers/pci/dwc/pcie-designware.h
>> +++ b/drivers/pci/dwc/pcie-designware.h
>> @@ -17,6 +17,7 @@
>>  #include <linux/irq.h>
>>  #include <linux/msi.h>
>>  #include <linux/pci.h>
>> +#include <linux/dma-mapping.h>
>>
>>  #include <linux/pci-epc.h>
>>  #include <linux/pci-epf.h>
>> @@ -168,7 +169,7 @@ struct pcie_port {
>>  	const struct dw_pcie_host_ops *ops;
>>  	int			msi_irq;
>>  	struct irq_domain	*irq_domain;
>> -	unsigned long		msi_data;
>> +	dma_addr_t		msi_data;
>>  	DECLARE_BITMAP(msi_irq_in_use, MAX_MSI_IRQS);
>>  };
>>
>> --
>> 2.11.0
> 

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH 01/10] PCI: dwc: use DMA-API for allocating MSI data
  2017-10-13 16:09 ` [PATCH 01/10] PCI: dwc: use DMA-API for allocating MSI data Niklas Cassel
  2017-10-13 16:42   ` David Laight
@ 2017-10-13 16:43   ` Jingoo Han
  2017-10-16 12:08     ` Niklas Cassel
  2017-10-13 16:47   ` Jingoo Han
  2017-10-16 22:16   ` Bjorn Helgaas
  3 siblings, 1 reply; 41+ messages in thread
From: Jingoo Han @ 2017-10-13 16:43 UTC (permalink / raw)
  To: 'Niklas Cassel', 'Joao Pinto',
	'Bjorn Helgaas'
  Cc: 'Niklas Cassel', linux-pci, linux-kernel

On Friday, October 13, 2017 12:09 PM, Niklas Cassel wrote:
> 
> Since it is a PCIe endpoint device, rather than the CPU, that is supposed
> to write to this location, the proper way to get the address to this this
> location is really to use the DMA API, rather than virt_to_phys.
> 
> Using virt_to_phys might work on some systems, but by using the DMA API,
> we know that it will work on all systems.
> 
> This is essentially the same thing as allocating a buffer in a driver,
> to which the endpoint will write to. To do this, we use the DMA API.

I have no objection. However, did you test your patch?
In my opinion, your company does not handle DWC PCIe controller, right?
If so, you need to get tested-by from other people.

Best regards,
Jingoo Han

> 
> Signed-off-by: Niklas Cassel <niklas.cassel@axis.com>
> ---
>  drivers/pci/dwc/pcie-designware-host.c | 23 ++++++++++++++++-------
>  drivers/pci/dwc/pcie-designware.h      |  3 ++-
>  2 files changed, 18 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pci/dwc/pcie-designware-host.c
> b/drivers/pci/dwc/pcie-designware-host.c
> index 81e2157a7cfb..f6d152ea2a03 100644
> --- a/drivers/pci/dwc/pcie-designware-host.c
> +++ b/drivers/pci/dwc/pcie-designware-host.c
> @@ -83,16 +83,25 @@ irqreturn_t dw_handle_msi_irq(struct pcie_port *pp)
> 
>  void dw_pcie_msi_init(struct pcie_port *pp)
>  {
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +	struct device *dev = pci->dev;
> +	struct page *page;
>  	u64 msi_target;
> 
> -	pp->msi_data = __get_free_pages(GFP_KERNEL, 0);
> -	msi_target = virt_to_phys((void *)pp->msi_data);
> +	page = alloc_page(GFP_KERNEL | GFP_DMA32);
> +	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");
> +		__free_page(page);
> +		return;
> +	}
> +	msi_target = (u64)pp->msi_data;
> 
>  	/* program the msi_data */
>  	dw_pcie_wr_own_conf(pp, PCIE_MSI_ADDR_LO, 4,
> -			    (u32)(msi_target & 0xffffffff));
> +			    lower_32_bits(msi_target));
>  	dw_pcie_wr_own_conf(pp, PCIE_MSI_ADDR_HI, 4,
> -			    (u32)(msi_target >> 32 & 0xffffffff));
> +			    upper_32_bits(msi_target));
>  }
> 
>  static void dw_pcie_msi_clear_irq(struct pcie_port *pp, int irq)
> @@ -187,10 +196,10 @@ static void dw_msi_setup_msg(struct pcie_port *pp,
> unsigned int irq, u32 pos)
>  	if (pp->ops->get_msi_addr)
>  		msi_target = pp->ops->get_msi_addr(pp);
>  	else
> -		msi_target = virt_to_phys((void *)pp->msi_data);
> +		msi_target = (u64)pp->msi_data;
> 
> -	msg.address_lo = (u32)(msi_target & 0xffffffff);
> -	msg.address_hi = (u32)(msi_target >> 32 & 0xffffffff);
> +	msg.address_lo = lower_32_bits(msi_target);
> +	msg.address_hi = upper_32_bits(msi_target);
> 
>  	if (pp->ops->get_msi_data)
>  		msg.data = pp->ops->get_msi_data(pp, pos);
> diff --git a/drivers/pci/dwc/pcie-designware.h b/drivers/pci/dwc/pcie-
> designware.h
> index e5d9d77b778e..547352a317f8 100644
> --- a/drivers/pci/dwc/pcie-designware.h
> +++ b/drivers/pci/dwc/pcie-designware.h
> @@ -17,6 +17,7 @@
>  #include <linux/irq.h>
>  #include <linux/msi.h>
>  #include <linux/pci.h>
> +#include <linux/dma-mapping.h>
> 
>  #include <linux/pci-epc.h>
>  #include <linux/pci-epf.h>
> @@ -168,7 +169,7 @@ struct pcie_port {
>  	const struct dw_pcie_host_ops *ops;
>  	int			msi_irq;
>  	struct irq_domain	*irq_domain;
> -	unsigned long		msi_data;
> +	dma_addr_t		msi_data;
>  	DECLARE_BITMAP(msi_irq_in_use, MAX_MSI_IRQS);
>  };
> 
> --
> 2.11.0

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH 01/10] PCI: dwc: use DMA-API for allocating MSI data
  2017-10-13 16:43   ` Jingoo Han
@ 2017-10-16 12:08     ` Niklas Cassel
  0 siblings, 0 replies; 41+ messages in thread
From: Niklas Cassel @ 2017-10-16 12:08 UTC (permalink / raw)
  To: Jingoo Han, 'Joao Pinto', 'Bjorn Helgaas'
  Cc: linux-pci, linux-kernel, Kishon Vijay Abraham I

On 10/13/2017 06:43 PM, Jingoo Han wrote:
> On Friday, October 13, 2017 12:09 PM, Niklas Cassel wrote:
>>
>> Since it is a PCIe endpoint device, rather than the CPU, that is supposed
>> to write to this location, the proper way to get the address to this this
>> location is really to use the DMA API, rather than virt_to_phys.
>>
>> Using virt_to_phys might work on some systems, but by using the DMA API,
>> we know that it will work on all systems.
>>
>> This is essentially the same thing as allocating a buffer in a driver,
>> to which the endpoint will write to. To do this, we use the DMA API.
> 

Hello Jingoo.

> I have no objection. However, did you test your patch?

I've tested the patch on the ARTPEC-6 and the ARTPEC-7 SoC,
one which has a 64-bit dma_addr_t, one which has a 32-bit dma_addr_t.

However, since device drivers use dma_map_page all the time,
for this exact purpose, I don't think there should be a problem.
(Famous last words..)

> In my opinion, your company does not handle DWC PCIe controller, right?

Right, we are not Synopsys ;)

> If so, you need to get tested-by from other people.

I agree, a couple of Tested-by would be a good idea.

Joao and Kishon, if you have the time, could you please help me
out and see MSI still works on your platform with this patch?


Regards,
Niklas


> 
> Best regards,
> Jingoo Han
> 
>>
>> Signed-off-by: Niklas Cassel <niklas.cassel@axis.com>
>> ---
>>  drivers/pci/dwc/pcie-designware-host.c | 23 ++++++++++++++++-------
>>  drivers/pci/dwc/pcie-designware.h      |  3 ++-
>>  2 files changed, 18 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/pci/dwc/pcie-designware-host.c
>> b/drivers/pci/dwc/pcie-designware-host.c
>> index 81e2157a7cfb..f6d152ea2a03 100644
>> --- a/drivers/pci/dwc/pcie-designware-host.c
>> +++ b/drivers/pci/dwc/pcie-designware-host.c
>> @@ -83,16 +83,25 @@ irqreturn_t dw_handle_msi_irq(struct pcie_port *pp)
>>
>>  void dw_pcie_msi_init(struct pcie_port *pp)
>>  {
>> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>> +	struct device *dev = pci->dev;
>> +	struct page *page;
>>  	u64 msi_target;
>>
>> -	pp->msi_data = __get_free_pages(GFP_KERNEL, 0);
>> -	msi_target = virt_to_phys((void *)pp->msi_data);
>> +	page = alloc_page(GFP_KERNEL | GFP_DMA32);
>> +	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");
>> +		__free_page(page);
>> +		return;
>> +	}
>> +	msi_target = (u64)pp->msi_data;
>>
>>  	/* program the msi_data */
>>  	dw_pcie_wr_own_conf(pp, PCIE_MSI_ADDR_LO, 4,
>> -			    (u32)(msi_target & 0xffffffff));
>> +			    lower_32_bits(msi_target));
>>  	dw_pcie_wr_own_conf(pp, PCIE_MSI_ADDR_HI, 4,
>> -			    (u32)(msi_target >> 32 & 0xffffffff));
>> +			    upper_32_bits(msi_target));
>>  }
>>
>>  static void dw_pcie_msi_clear_irq(struct pcie_port *pp, int irq)
>> @@ -187,10 +196,10 @@ static void dw_msi_setup_msg(struct pcie_port *pp,
>> unsigned int irq, u32 pos)
>>  	if (pp->ops->get_msi_addr)
>>  		msi_target = pp->ops->get_msi_addr(pp);
>>  	else
>> -		msi_target = virt_to_phys((void *)pp->msi_data);
>> +		msi_target = (u64)pp->msi_data;
>>
>> -	msg.address_lo = (u32)(msi_target & 0xffffffff);
>> -	msg.address_hi = (u32)(msi_target >> 32 & 0xffffffff);
>> +	msg.address_lo = lower_32_bits(msi_target);
>> +	msg.address_hi = upper_32_bits(msi_target);
>>
>>  	if (pp->ops->get_msi_data)
>>  		msg.data = pp->ops->get_msi_data(pp, pos);
>> diff --git a/drivers/pci/dwc/pcie-designware.h b/drivers/pci/dwc/pcie-
>> designware.h
>> index e5d9d77b778e..547352a317f8 100644
>> --- a/drivers/pci/dwc/pcie-designware.h
>> +++ b/drivers/pci/dwc/pcie-designware.h
>> @@ -17,6 +17,7 @@
>>  #include <linux/irq.h>
>>  #include <linux/msi.h>
>>  #include <linux/pci.h>
>> +#include <linux/dma-mapping.h>
>>
>>  #include <linux/pci-epc.h>
>>  #include <linux/pci-epf.h>
>> @@ -168,7 +169,7 @@ struct pcie_port {
>>  	const struct dw_pcie_host_ops *ops;
>>  	int			msi_irq;
>>  	struct irq_domain	*irq_domain;
>> -	unsigned long		msi_data;
>> +	dma_addr_t		msi_data;
>>  	DECLARE_BITMAP(msi_irq_in_use, MAX_MSI_IRQS);
>>  };
>>
>> --
>> 2.11.0
> 
> 

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH 01/10] PCI: dwc: use DMA-API for allocating MSI data
  2017-10-13 16:09 ` [PATCH 01/10] PCI: dwc: use DMA-API for allocating MSI data Niklas Cassel
  2017-10-13 16:42   ` David Laight
  2017-10-13 16:43   ` Jingoo Han
@ 2017-10-13 16:47   ` Jingoo Han
  2017-10-16 12:11     ` Niklas Cassel
  2017-10-16 22:16   ` Bjorn Helgaas
  3 siblings, 1 reply; 41+ messages in thread
From: Jingoo Han @ 2017-10-13 16:47 UTC (permalink / raw)
  To: 'Niklas Cassel', 'Joao Pinto',
	'Bjorn Helgaas'
  Cc: 'Niklas Cassel', linux-pci, linux-kernel

On Friday, October 13, 2017 12:09 PM, Niklas Cassel wrote:
> 
> Since it is a PCIe endpoint device, rather than the CPU, that is supposed
> to write to this location, the proper way to get the address to this this
> location is really to use the DMA API, rather than virt_to_phys.
> 
> Using virt_to_phys might work on some systems, but by using the DMA API,
> we know that it will work on all systems.
> 
> This is essentially the same thing as allocating a buffer in a driver,
> to which the endpoint will write to. To do this, we use the DMA API.
> 
> Signed-off-by: Niklas Cassel <niklas.cassel@axis.com>
> ---
>  drivers/pci/dwc/pcie-designware-host.c | 23 ++++++++++++++++-------
>  drivers/pci/dwc/pcie-designware.h      |  3 ++-
>  2 files changed, 18 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pci/dwc/pcie-designware-host.c
> b/drivers/pci/dwc/pcie-designware-host.c
> index 81e2157a7cfb..f6d152ea2a03 100644
> --- a/drivers/pci/dwc/pcie-designware-host.c
> +++ b/drivers/pci/dwc/pcie-designware-host.c
> @@ -83,16 +83,25 @@ irqreturn_t dw_handle_msi_irq(struct pcie_port *pp)
> 
>  void dw_pcie_msi_init(struct pcie_port *pp)
>  {
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +	struct device *dev = pci->dev;
> +	struct page *page;
>  	u64 msi_target;
> 
> -	pp->msi_data = __get_free_pages(GFP_KERNEL, 0);
> -	msi_target = virt_to_phys((void *)pp->msi_data);
> +	page = alloc_page(GFP_KERNEL | GFP_DMA32);
> +	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");
> +		__free_page(page);
> +		return;
> +	}
> +	msi_target = (u64)pp->msi_data;
> 
>  	/* program the msi_data */
>  	dw_pcie_wr_own_conf(pp, PCIE_MSI_ADDR_LO, 4,
> -			    (u32)(msi_target & 0xffffffff));
> +			    lower_32_bits(msi_target));
>  	dw_pcie_wr_own_conf(pp, PCIE_MSI_ADDR_HI, 4,
> -			    (u32)(msi_target >> 32 & 0xffffffff));
> +			    upper_32_bits(msi_target));
>  }
> 
>  static void dw_pcie_msi_clear_irq(struct pcie_port *pp, int irq)
> @@ -187,10 +196,10 @@ static void dw_msi_setup_msg(struct pcie_port *pp,
> unsigned int irq, u32 pos)
>  	if (pp->ops->get_msi_addr)
>  		msi_target = pp->ops->get_msi_addr(pp);
>  	else
> -		msi_target = virt_to_phys((void *)pp->msi_data);
> +		msi_target = (u64)pp->msi_data;
> 
> -	msg.address_lo = (u32)(msi_target & 0xffffffff);
> -	msg.address_hi = (u32)(msi_target >> 32 & 0xffffffff);
> +	msg.address_lo = lower_32_bits(msi_target);
> +	msg.address_hi = upper_32_bits(msi_target);
> 
>  	if (pp->ops->get_msi_data)
>  		msg.data = pp->ops->get_msi_data(pp, pos);
> diff --git a/drivers/pci/dwc/pcie-designware.h b/drivers/pci/dwc/pcie-
> designware.h
> index e5d9d77b778e..547352a317f8 100644
> --- a/drivers/pci/dwc/pcie-designware.h
> +++ b/drivers/pci/dwc/pcie-designware.h
> @@ -17,6 +17,7 @@
>  #include <linux/irq.h>
>  #include <linux/msi.h>
>  #include <linux/pci.h>
> +#include <linux/dma-mapping.h>

If possible, please add this header in alphabetical order.

Best regards,
Jingoo Han

> 
>  #include <linux/pci-epc.h>
>  #include <linux/pci-epf.h>
> @@ -168,7 +169,7 @@ struct pcie_port {
>  	const struct dw_pcie_host_ops *ops;
>  	int			msi_irq;
>  	struct irq_domain	*irq_domain;
> -	unsigned long		msi_data;
> +	dma_addr_t		msi_data;
>  	DECLARE_BITMAP(msi_irq_in_use, MAX_MSI_IRQS);
>  };
> 
> --
> 2.11.0

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH 01/10] PCI: dwc: use DMA-API for allocating MSI data
  2017-10-13 16:47   ` Jingoo Han
@ 2017-10-16 12:11     ` Niklas Cassel
  0 siblings, 0 replies; 41+ messages in thread
From: Niklas Cassel @ 2017-10-16 12:11 UTC (permalink / raw)
  To: Jingoo Han, 'Joao Pinto', 'Bjorn Helgaas'
  Cc: linux-pci, linux-kernel

On 10/13/2017 06:47 PM, Jingoo Han wrote:
> On Friday, October 13, 2017 12:09 PM, Niklas Cassel wrote:
>>
>> Since it is a PCIe endpoint device, rather than the CPU, that is supposed
>> to write to this location, the proper way to get the address to this this
>> location is really to use the DMA API, rather than virt_to_phys.
>>
>> Using virt_to_phys might work on some systems, but by using the DMA API,
>> we know that it will work on all systems.
>>
>> This is essentially the same thing as allocating a buffer in a driver,
>> to which the endpoint will write to. To do this, we use the DMA API.
>>
>> Signed-off-by: Niklas Cassel <niklas.cassel@axis.com>
>> ---
>>  drivers/pci/dwc/pcie-designware-host.c | 23 ++++++++++++++++-------
>>  drivers/pci/dwc/pcie-designware.h      |  3 ++-
>>  2 files changed, 18 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/pci/dwc/pcie-designware-host.c
>> b/drivers/pci/dwc/pcie-designware-host.c
>> index 81e2157a7cfb..f6d152ea2a03 100644
>> --- a/drivers/pci/dwc/pcie-designware-host.c
>> +++ b/drivers/pci/dwc/pcie-designware-host.c
>> @@ -83,16 +83,25 @@ irqreturn_t dw_handle_msi_irq(struct pcie_port *pp)
>>
>>  void dw_pcie_msi_init(struct pcie_port *pp)
>>  {
>> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>> +	struct device *dev = pci->dev;
>> +	struct page *page;
>>  	u64 msi_target;
>>
>> -	pp->msi_data = __get_free_pages(GFP_KERNEL, 0);
>> -	msi_target = virt_to_phys((void *)pp->msi_data);
>> +	page = alloc_page(GFP_KERNEL | GFP_DMA32);
>> +	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");
>> +		__free_page(page);
>> +		return;
>> +	}
>> +	msi_target = (u64)pp->msi_data;
>>
>>  	/* program the msi_data */
>>  	dw_pcie_wr_own_conf(pp, PCIE_MSI_ADDR_LO, 4,
>> -			    (u32)(msi_target & 0xffffffff));
>> +			    lower_32_bits(msi_target));
>>  	dw_pcie_wr_own_conf(pp, PCIE_MSI_ADDR_HI, 4,
>> -			    (u32)(msi_target >> 32 & 0xffffffff));
>> +			    upper_32_bits(msi_target));
>>  }
>>
>>  static void dw_pcie_msi_clear_irq(struct pcie_port *pp, int irq)
>> @@ -187,10 +196,10 @@ static void dw_msi_setup_msg(struct pcie_port *pp,
>> unsigned int irq, u32 pos)
>>  	if (pp->ops->get_msi_addr)
>>  		msi_target = pp->ops->get_msi_addr(pp);
>>  	else
>> -		msi_target = virt_to_phys((void *)pp->msi_data);
>> +		msi_target = (u64)pp->msi_data;
>>
>> -	msg.address_lo = (u32)(msi_target & 0xffffffff);
>> -	msg.address_hi = (u32)(msi_target >> 32 & 0xffffffff);
>> +	msg.address_lo = lower_32_bits(msi_target);
>> +	msg.address_hi = upper_32_bits(msi_target);
>>
>>  	if (pp->ops->get_msi_data)
>>  		msg.data = pp->ops->get_msi_data(pp, pos);
>> diff --git a/drivers/pci/dwc/pcie-designware.h b/drivers/pci/dwc/pcie-
>> designware.h
>> index e5d9d77b778e..547352a317f8 100644
>> --- a/drivers/pci/dwc/pcie-designware.h
>> +++ b/drivers/pci/dwc/pcie-designware.h
>> @@ -17,6 +17,7 @@
>>  #include <linux/irq.h>
>>  #include <linux/msi.h>
>>  #include <linux/pci.h>
>> +#include <linux/dma-mapping.h>
> 
> If possible, please add this header in alphabetical order.

Absolutely, I will send a V2 series as soon as I get some
review comments on any of the other patches in the series.


Regards,
Niklas


> 
> Best regards,
> Jingoo Han
> 
>>
>>  #include <linux/pci-epc.h>
>>  #include <linux/pci-epf.h>
>> @@ -168,7 +169,7 @@ struct pcie_port {
>>  	const struct dw_pcie_host_ops *ops;
>>  	int			msi_irq;
>>  	struct irq_domain	*irq_domain;
>> -	unsigned long		msi_data;
>> +	dma_addr_t		msi_data;
>>  	DECLARE_BITMAP(msi_irq_in_use, MAX_MSI_IRQS);
>>  };
>>
>> --
>> 2.11.0
> 
> 

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH 01/10] PCI: dwc: use DMA-API for allocating MSI data
  2017-10-13 16:09 ` [PATCH 01/10] PCI: dwc: use DMA-API for allocating MSI data Niklas Cassel
                     ` (2 preceding siblings ...)
  2017-10-13 16:47   ` Jingoo Han
@ 2017-10-16 22:16   ` Bjorn Helgaas
  3 siblings, 0 replies; 41+ messages in thread
From: Bjorn Helgaas @ 2017-10-16 22:16 UTC (permalink / raw)
  To: Niklas Cassel
  Cc: Jingoo Han, Joao Pinto, Bjorn Helgaas, Niklas Cassel, linux-pci,
	linux-kernel

On Fri, Oct 13, 2017 at 06:09:04PM +0200, Niklas Cassel wrote:
> Since it is a PCIe endpoint device, rather than the CPU, that is supposed
> to write to this location, the proper way to get the address to this this
> location is really to use the DMA API, rather than virt_to_phys.
> 
> Using virt_to_phys might work on some systems, but by using the DMA API,
> we know that it will work on all systems.

virt_to_phys() is definitely deprecated for this case.

> This is essentially the same thing as allocating a buffer in a driver,
> to which the endpoint will write to. To do this, we use the DMA API.
> 
> Signed-off-by: Niklas Cassel <niklas.cassel@axis.com>
> ---
>  drivers/pci/dwc/pcie-designware-host.c | 23 ++++++++++++++++-------
>  drivers/pci/dwc/pcie-designware.h      |  3 ++-
>  2 files changed, 18 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pci/dwc/pcie-designware-host.c b/drivers/pci/dwc/pcie-designware-host.c
> index 81e2157a7cfb..f6d152ea2a03 100644
> --- a/drivers/pci/dwc/pcie-designware-host.c
> +++ b/drivers/pci/dwc/pcie-designware-host.c
> @@ -83,16 +83,25 @@ irqreturn_t dw_handle_msi_irq(struct pcie_port *pp)
>  
>  void dw_pcie_msi_init(struct pcie_port *pp)
>  {
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +	struct device *dev = pci->dev;
> +	struct page *page;
>  	u64 msi_target;
>  
> -	pp->msi_data = __get_free_pages(GFP_KERNEL, 0);
> -	msi_target = virt_to_phys((void *)pp->msi_data);
> +	page = alloc_page(GFP_KERNEL | GFP_DMA32);
> +	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");

s/msi/MSI/ since it's English text.

> +		__free_page(page);
> +		return;
> +	}
> +	msi_target = (u64)pp->msi_data;
>  
>  	/* program the msi_data */
>  	dw_pcie_wr_own_conf(pp, PCIE_MSI_ADDR_LO, 4,
> -			    (u32)(msi_target & 0xffffffff));
> +			    lower_32_bits(msi_target));
>  	dw_pcie_wr_own_conf(pp, PCIE_MSI_ADDR_HI, 4,
> -			    (u32)(msi_target >> 32 & 0xffffffff));
> +			    upper_32_bits(msi_target));

These are cleanups unrelated to the DMA change, right?  I think they're
fine, but should be in a different patch.  It looks like there are a few
more occurrences of this pattern in drivers/pci that could be changed.

>  }
>  
>  static void dw_pcie_msi_clear_irq(struct pcie_port *pp, int irq)
> @@ -187,10 +196,10 @@ static void dw_msi_setup_msg(struct pcie_port *pp, unsigned int irq, u32 pos)
>  	if (pp->ops->get_msi_addr)
>  		msi_target = pp->ops->get_msi_addr(pp);
>  	else
> -		msi_target = virt_to_phys((void *)pp->msi_data);
> +		msi_target = (u64)pp->msi_data;
>  
> -	msg.address_lo = (u32)(msi_target & 0xffffffff);
> -	msg.address_hi = (u32)(msi_target >> 32 & 0xffffffff);
> +	msg.address_lo = lower_32_bits(msi_target);
> +	msg.address_hi = upper_32_bits(msi_target);
>  
>  	if (pp->ops->get_msi_data)
>  		msg.data = pp->ops->get_msi_data(pp, pos);
> diff --git a/drivers/pci/dwc/pcie-designware.h b/drivers/pci/dwc/pcie-designware.h
> index e5d9d77b778e..547352a317f8 100644
> --- a/drivers/pci/dwc/pcie-designware.h
> +++ b/drivers/pci/dwc/pcie-designware.h
> @@ -17,6 +17,7 @@
>  #include <linux/irq.h>
>  #include <linux/msi.h>
>  #include <linux/pci.h>
> +#include <linux/dma-mapping.h>
>  
>  #include <linux/pci-epc.h>
>  #include <linux/pci-epf.h>
> @@ -168,7 +169,7 @@ struct pcie_port {
>  	const struct dw_pcie_host_ops *ops;
>  	int			msi_irq;
>  	struct irq_domain	*irq_domain;
> -	unsigned long		msi_data;
> +	dma_addr_t		msi_data;
>  	DECLARE_BITMAP(msi_irq_in_use, MAX_MSI_IRQS);
>  };
>  
> -- 
> 2.11.0
> 

^ permalink raw reply	[flat|nested] 41+ messages in thread

* [PATCH 02/10] PCI: designware-ep: set_msi should only set MMC bits
  2017-10-13 16:09 ` Niklas Cassel
  (?)
  (?)
@ 2017-10-13 16:09 ` Niklas Cassel
  2017-10-16 22:26   ` Bjorn Helgaas
  -1 siblings, 1 reply; 41+ messages in thread
From: Niklas Cassel @ 2017-10-13 16:09 UTC (permalink / raw)
  To: Jingoo Han, Joao Pinto, Bjorn Helgaas
  Cc: Niklas Cassel, linux-pci, linux-kernel

Previously, set_msi wrote all bits in the Message Control
register, thus overwriting the 64 bit address capable bit.
By clearing the 64 bit address capable bit, we break MSI
on systems where the RC has set a 64 bit MSI address.

Signed-off-by: Niklas Cassel <niklas.cassel@axis.com>
---
 drivers/pci/dwc/pcie-designware-ep.c | 4 +++-
 drivers/pci/dwc/pcie-designware.h    | 1 +
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/dwc/pcie-designware-ep.c b/drivers/pci/dwc/pcie-designware-ep.c
index d53d5f168363..c92ab87fd660 100644
--- a/drivers/pci/dwc/pcie-designware-ep.c
+++ b/drivers/pci/dwc/pcie-designware-ep.c
@@ -220,7 +220,9 @@ static int dw_pcie_ep_set_msi(struct pci_epc *epc, u8 encode_int)
 	struct dw_pcie_ep *ep = epc_get_drvdata(epc);
 	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
 
-	val = (encode_int << MSI_CAP_MMC_SHIFT);
+	val = dw_pcie_readw_dbi(pci, MSI_MESSAGE_CONTROL);
+	val &= ~MSI_CAP_MMC_MASK;
+	val |= (encode_int << MSI_CAP_MMC_SHIFT) & MSI_CAP_MMC_MASK;
 	dw_pcie_writew_dbi(pci, MSI_MESSAGE_CONTROL, val);
 
 	return 0;
diff --git a/drivers/pci/dwc/pcie-designware.h b/drivers/pci/dwc/pcie-designware.h
index 547352a317f8..36183906e1d2 100644
--- a/drivers/pci/dwc/pcie-designware.h
+++ b/drivers/pci/dwc/pcie-designware.h
@@ -101,6 +101,7 @@
 
 #define MSI_MESSAGE_CONTROL		0x52
 #define MSI_CAP_MMC_SHIFT		1
+#define MSI_CAP_MMC_MASK		(7 << MSI_CAP_MMC_SHIFT)
 #define MSI_CAP_MME_SHIFT		4
 #define MSI_CAP_MME_MASK		(7 << MSI_CAP_MME_SHIFT)
 #define MSI_MESSAGE_ADDR_L32		0x54
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 41+ messages in thread

* Re: [PATCH 02/10] PCI: designware-ep: set_msi should only set MMC bits
  2017-10-13 16:09 ` [PATCH 02/10] PCI: designware-ep: set_msi should only set MMC bits Niklas Cassel
@ 2017-10-16 22:26   ` Bjorn Helgaas
  0 siblings, 0 replies; 41+ messages in thread
From: Bjorn Helgaas @ 2017-10-16 22:26 UTC (permalink / raw)
  To: Niklas Cassel
  Cc: Jingoo Han, Joao Pinto, Bjorn Helgaas, Niklas Cassel, linux-pci,
	linux-kernel

Would you mind capitalizing the first letter of the changelog summary,
i.e., to follow the convention of "git log --oneline
drivers/pci/dwc/pcie-designware-ep.c"?  I usually fix that up
manually, but it saves me time if you do it.

On Fri, Oct 13, 2017 at 06:09:05PM +0200, Niklas Cassel wrote:
> Previously, set_msi wrote all bits in the Message Control
> register, thus overwriting the 64 bit address capable bit.
> By clearing the 64 bit address capable bit, we break MSI
> on systems where the RC has set a 64 bit MSI address.

s/set_msi/dw_pcie_ep_set_msi()/ to be specific.

I think we overwrote PCI_MSI_FLAGS_64BIT (and PCI_MSI_FLAGS_ENABLE).

> Signed-off-by: Niklas Cassel <niklas.cassel@axis.com>
> ---
>  drivers/pci/dwc/pcie-designware-ep.c | 4 +++-
>  drivers/pci/dwc/pcie-designware.h    | 1 +
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/dwc/pcie-designware-ep.c b/drivers/pci/dwc/pcie-designware-ep.c
> index d53d5f168363..c92ab87fd660 100644
> --- a/drivers/pci/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/dwc/pcie-designware-ep.c
> @@ -220,7 +220,9 @@ static int dw_pcie_ep_set_msi(struct pci_epc *epc, u8 encode_int)
>  	struct dw_pcie_ep *ep = epc_get_drvdata(epc);
>  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>  
> -	val = (encode_int << MSI_CAP_MMC_SHIFT);
> +	val = dw_pcie_readw_dbi(pci, MSI_MESSAGE_CONTROL);
> +	val &= ~MSI_CAP_MMC_MASK;
> +	val |= (encode_int << MSI_CAP_MMC_SHIFT) & MSI_CAP_MMC_MASK;
>  	dw_pcie_writew_dbi(pci, MSI_MESSAGE_CONTROL, val);
>  
>  	return 0;
> diff --git a/drivers/pci/dwc/pcie-designware.h b/drivers/pci/dwc/pcie-designware.h
> index 547352a317f8..36183906e1d2 100644
> --- a/drivers/pci/dwc/pcie-designware.h
> +++ b/drivers/pci/dwc/pcie-designware.h
> @@ -101,6 +101,7 @@
>  
>  #define MSI_MESSAGE_CONTROL		0x52
>  #define MSI_CAP_MMC_SHIFT		1
> +#define MSI_CAP_MMC_MASK		(7 << MSI_CAP_MMC_SHIFT)
>  #define MSI_CAP_MME_SHIFT		4
>  #define MSI_CAP_MME_MASK		(7 << MSI_CAP_MME_SHIFT)
>  #define MSI_MESSAGE_ADDR_L32		0x54

It looks like these are really other names for PCI_MSI_FLAGS_QMASK and
PCI_MSI_FLAGS_QSIZE.  I wish the DW code used those to make the
connection more obvious.

But I think this patch is correct as-is and that would be a different
patch.

^ permalink raw reply	[flat|nested] 41+ messages in thread

* [PATCH 03/10] PCI: designware-ep: read-only registers need DBI_RO_WR_EN to be writable
  2017-10-13 16:09 ` Niklas Cassel
                   ` (2 preceding siblings ...)
  (?)
@ 2017-10-13 16:09 ` Niklas Cassel
  -1 siblings, 0 replies; 41+ messages in thread
From: Niklas Cassel @ 2017-10-13 16:09 UTC (permalink / raw)
  To: Jingoo Han, Joao Pinto, Bjorn Helgaas
  Cc: Niklas Cassel, linux-pci, linux-kernel

Certain registers that pcie-designware-ep tries to write are read-only
registers. However, these registers can become read/write if we first
enable the DBI_RO_WR_EN bit.

Signed-off-by: Niklas Cassel <niklas.cassel@axis.com>
---
 drivers/pci/dwc/pcie-designware-ep.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/pci/dwc/pcie-designware-ep.c b/drivers/pci/dwc/pcie-designware-ep.c
index c92ab87fd660..3fb34be99715 100644
--- a/drivers/pci/dwc/pcie-designware-ep.c
+++ b/drivers/pci/dwc/pcie-designware-ep.c
@@ -35,8 +35,10 @@ static void dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar)
 	u32 reg;
 
 	reg = PCI_BASE_ADDRESS_0 + (4 * bar);
+	dw_pcie_dbi_ro_wr_en(pci);
 	dw_pcie_writel_dbi2(pci, reg, 0x0);
 	dw_pcie_writel_dbi(pci, reg, 0x0);
+	dw_pcie_dbi_ro_wr_dis(pci);
 }
 
 static int dw_pcie_ep_write_header(struct pci_epc *epc,
@@ -45,6 +47,7 @@ static int dw_pcie_ep_write_header(struct pci_epc *epc,
 	struct dw_pcie_ep *ep = epc_get_drvdata(epc);
 	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
 
+	dw_pcie_dbi_ro_wr_en(pci);
 	dw_pcie_writew_dbi(pci, PCI_VENDOR_ID, hdr->vendorid);
 	dw_pcie_writew_dbi(pci, PCI_DEVICE_ID, hdr->deviceid);
 	dw_pcie_writeb_dbi(pci, PCI_REVISION_ID, hdr->revid);
@@ -58,6 +61,7 @@ static int dw_pcie_ep_write_header(struct pci_epc *epc,
 	dw_pcie_writew_dbi(pci, PCI_SUBSYSTEM_ID, hdr->subsys_id);
 	dw_pcie_writeb_dbi(pci, PCI_INTERRUPT_PIN,
 			   hdr->interrupt_pin);
+	dw_pcie_dbi_ro_wr_dis(pci);
 
 	return 0;
 }
@@ -142,8 +146,10 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, enum pci_barno bar,
 	if (ret)
 		return ret;
 
+	dw_pcie_dbi_ro_wr_en(pci);
 	dw_pcie_writel_dbi2(pci, reg, size - 1);
 	dw_pcie_writel_dbi(pci, reg, flags);
+	dw_pcie_dbi_ro_wr_dis(pci);
 
 	return 0;
 }
@@ -223,7 +229,9 @@ static int dw_pcie_ep_set_msi(struct pci_epc *epc, u8 encode_int)
 	val = dw_pcie_readw_dbi(pci, MSI_MESSAGE_CONTROL);
 	val &= ~MSI_CAP_MMC_MASK;
 	val |= (encode_int << MSI_CAP_MMC_SHIFT) & MSI_CAP_MMC_MASK;
+	dw_pcie_dbi_ro_wr_en(pci);
 	dw_pcie_writew_dbi(pci, MSI_MESSAGE_CONTROL, val);
+	dw_pcie_dbi_ro_wr_dis(pci);
 
 	return 0;
 }
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 41+ messages in thread

* [PATCH 04/10] PCI: designware-ep: pre-allocate memory for MSI in dw_pcie_ep_init
  2017-10-13 16:09 ` Niklas Cassel
                   ` (3 preceding siblings ...)
  (?)
@ 2017-10-13 16:09 ` Niklas Cassel
  -1 siblings, 0 replies; 41+ messages in thread
From: Niklas Cassel @ 2017-10-13 16:09 UTC (permalink / raw)
  To: Jingoo Han, Joao Pinto, Bjorn Helgaas
  Cc: Niklas Cassel, linux-pci, linux-kernel

Certain SoCs need to map the MSI address in raise_irq.
To map an address, you first need to call pci_epc_mem_alloc_addr,
however, pci_epc_mem_alloc_addr calls ioremap (which can sleep).

Since raise_irq is only called from atomic context, we can't call
pci_epc_mem_alloc_addr from raise_irq, instead we pre-allocate
a page in dw_pcie_ep_init, so this page can later be used to map/unmap
the MSI address in raise_irq.

Signed-off-by: Niklas Cassel <niklas.cassel@axis.com>
---
 drivers/pci/dwc/pcie-designware-ep.c | 8 ++++++++
 drivers/pci/dwc/pcie-designware.h    | 2 ++
 2 files changed, 10 insertions(+)

diff --git a/drivers/pci/dwc/pcie-designware-ep.c b/drivers/pci/dwc/pcie-designware-ep.c
index 3fb34be99715..c291da2a10ba 100644
--- a/drivers/pci/dwc/pcie-designware-ep.c
+++ b/drivers/pci/dwc/pcie-designware-ep.c
@@ -286,6 +286,8 @@ void dw_pcie_ep_exit(struct dw_pcie_ep *ep)
 {
 	struct pci_epc *epc = ep->epc;
 
+	pci_epc_mem_free_addr(epc, ep->msi_mem_phys, ep->msi_mem, PAGE_SIZE);
+
 	pci_epc_mem_exit(epc);
 }
 
@@ -341,6 +343,12 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
 		return ret;
 	}
 
+	ep->msi_mem = pci_epc_mem_alloc_addr(epc, &ep->msi_mem_phys, PAGE_SIZE);
+	if (!ep->msi_mem) {
+		dev_err(dev, "Failed to reserve memory for MSI\n");
+		return -ENOMEM;
+	}
+
 	ep->epc = epc;
 	epc_set_drvdata(epc, ep);
 	dw_pcie_setup(pci);
diff --git a/drivers/pci/dwc/pcie-designware.h b/drivers/pci/dwc/pcie-designware.h
index 36183906e1d2..f56d040afde4 100644
--- a/drivers/pci/dwc/pcie-designware.h
+++ b/drivers/pci/dwc/pcie-designware.h
@@ -198,6 +198,8 @@ struct dw_pcie_ep {
 	unsigned long		ob_window_map;
 	u32			num_ib_windows;
 	u32			num_ob_windows;
+	void __iomem		*msi_mem;
+	phys_addr_t		msi_mem_phys;
 };
 
 struct dw_pcie_ops {
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 41+ messages in thread

* [PATCH 05/10] PCI: artpec6: remove unused defines
  2017-10-13 16:09 ` Niklas Cassel
                   ` (4 preceding siblings ...)
  (?)
@ 2017-10-13 16:09 ` Niklas Cassel
  -1 siblings, 0 replies; 41+ messages in thread
From: Niklas Cassel @ 2017-10-13 16:09 UTC (permalink / raw)
  To: Niklas Cassel, Jesper Nilsson, Bjorn Helgaas
  Cc: linux-arm-kernel, linux-pci, linux-kernel

Commit b015b37e6693 ("PCI: artpec6: Stop enabling writes to
DBI read-only registers") removed the only write using these
defines, but it did not remove the defines.
Remove the defines since they are now unused.

Signed-off-by: Niklas Cassel <niklas.cassel@axis.com>
---
 drivers/pci/dwc/pcie-artpec6.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/pci/dwc/pcie-artpec6.c b/drivers/pci/dwc/pcie-artpec6.c
index 6653619db6a1..4b8ef266dc2f 100644
--- a/drivers/pci/dwc/pcie-artpec6.c
+++ b/drivers/pci/dwc/pcie-artpec6.c
@@ -37,9 +37,6 @@ struct artpec6_pcie {
 #define PCIE_PHY_DEBUG_R0		(PL_OFFSET + 0x28)
 #define PCIE_PHY_DEBUG_R1		(PL_OFFSET + 0x2c)
 
-#define MISC_CONTROL_1_OFF		(PL_OFFSET + 0x1bc)
-#define  DBI_RO_WR_EN			1
-
 /* ARTPEC-6 specific registers */
 #define PCIECFG				0x18
 #define  PCIECFG_DBG_OEN		(1 << 24)
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 41+ messages in thread

* [PATCH 06/10] PCI: dwc: artpec6: use BIT and GENMASK macros
  2017-10-13 16:09 ` Niklas Cassel
                   ` (5 preceding siblings ...)
  (?)
@ 2017-10-13 16:09 ` Niklas Cassel
  -1 siblings, 0 replies; 41+ messages in thread
From: Niklas Cassel @ 2017-10-13 16:09 UTC (permalink / raw)
  To: Niklas Cassel, Jesper Nilsson, Bjorn Helgaas
  Cc: linux-arm-kernel, linux-pci, linux-kernel

This greatly improves readability.

Signed-off-by: Niklas Cassel <niklas.cassel@axis.com>
---
 drivers/pci/dwc/pcie-artpec6.c | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/drivers/pci/dwc/pcie-artpec6.c b/drivers/pci/dwc/pcie-artpec6.c
index 4b8ef266dc2f..18075e0fab80 100644
--- a/drivers/pci/dwc/pcie-artpec6.c
+++ b/drivers/pci/dwc/pcie-artpec6.c
@@ -39,28 +39,28 @@ struct artpec6_pcie {
 
 /* ARTPEC-6 specific registers */
 #define PCIECFG				0x18
-#define  PCIECFG_DBG_OEN		(1 << 24)
-#define  PCIECFG_CORE_RESET_REQ		(1 << 21)
-#define  PCIECFG_LTSSM_ENABLE		(1 << 20)
-#define  PCIECFG_CLKREQ_B		(1 << 11)
-#define  PCIECFG_REFCLK_ENABLE		(1 << 10)
-#define  PCIECFG_PLL_ENABLE		(1 << 9)
-#define  PCIECFG_PCLK_ENABLE		(1 << 8)
-#define  PCIECFG_RISRCREN		(1 << 4)
-#define  PCIECFG_MODE_TX_DRV_EN		(1 << 3)
-#define  PCIECFG_CISRREN		(1 << 2)
-#define  PCIECFG_MACRO_ENABLE		(1 << 0)
+#define  PCIECFG_DBG_OEN		BIT(24)
+#define  PCIECFG_CORE_RESET_REQ		BIT(21)
+#define  PCIECFG_LTSSM_ENABLE		BIT(20)
+#define  PCIECFG_CLKREQ_B		BIT(11)
+#define  PCIECFG_REFCLK_ENABLE		BIT(10)
+#define  PCIECFG_PLL_ENABLE		BIT(9)
+#define  PCIECFG_PCLK_ENABLE		BIT(8)
+#define  PCIECFG_RISRCREN		BIT(4)
+#define  PCIECFG_MODE_TX_DRV_EN		BIT(3)
+#define  PCIECFG_CISRREN		BIT(2)
+#define  PCIECFG_MACRO_ENABLE		BIT(0)
 
 #define NOCCFG				0x40
-#define NOCCFG_ENABLE_CLK_PCIE		(1 << 4)
-#define NOCCFG_POWER_PCIE_IDLEACK	(1 << 3)
-#define NOCCFG_POWER_PCIE_IDLE		(1 << 2)
-#define NOCCFG_POWER_PCIE_IDLEREQ	(1 << 1)
+#define  NOCCFG_ENABLE_CLK_PCIE		BIT(4)
+#define  NOCCFG_POWER_PCIE_IDLEACK	BIT(3)
+#define  NOCCFG_POWER_PCIE_IDLE		BIT(2)
+#define  NOCCFG_POWER_PCIE_IDLEREQ	BIT(1)
 
 #define PHY_STATUS			0x118
-#define PHY_COSPLLLOCK			(1 << 0)
+#define  PHY_COSPLLLOCK			BIT(0)
 
-#define ARTPEC6_CPU_TO_BUS_ADDR		0x0fffffff
+#define ARTPEC6_CPU_TO_BUS_ADDR		GENMASK(27, 0)
 
 static u32 artpec6_pcie_readl(struct artpec6_pcie *artpec6_pcie, u32 offset)
 {
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 41+ messages in thread

* [PATCH 07/10] PCI: dwc: artpec6: split artpec6_pcie_establish_link to smaller functions
  2017-10-13 16:09 ` Niklas Cassel
                   ` (6 preceding siblings ...)
  (?)
@ 2017-10-13 16:09 ` Niklas Cassel
  -1 siblings, 0 replies; 41+ messages in thread
From: Niklas Cassel @ 2017-10-13 16:09 UTC (permalink / raw)
  To: Niklas Cassel, Jesper Nilsson, Bjorn Helgaas
  Cc: linux-arm-kernel, linux-pci, linux-kernel

This is done to better match other drivers such as dra7xx and imx6,
but also to prepare for endpoint mode support.

Signed-off-by: Niklas Cassel <niklas.cassel@axis.com>
---
 drivers/pci/dwc/pcie-artpec6.c | 59 +++++++++++++++++++++++-------------------
 1 file changed, 32 insertions(+), 27 deletions(-)

diff --git a/drivers/pci/dwc/pcie-artpec6.c b/drivers/pci/dwc/pcie-artpec6.c
index 18075e0fab80..c5d7f98dc6b2 100644
--- a/drivers/pci/dwc/pcie-artpec6.c
+++ b/drivers/pci/dwc/pcie-artpec6.c
@@ -80,18 +80,11 @@ static u64 artpec6_pcie_cpu_addr_fixup(u64 pci_addr)
 	return pci_addr & ARTPEC6_CPU_TO_BUS_ADDR;
 }
 
-static int artpec6_pcie_establish_link(struct artpec6_pcie *artpec6_pcie)
+static void artpec6_pcie_init_phy(struct artpec6_pcie *artpec6_pcie)
 {
-	struct dw_pcie *pci = artpec6_pcie->pci;
-	struct pcie_port *pp = &pci->pp;
 	u32 val;
 	unsigned int retries;
 
-	/* Hold DW core in reset */
-	val = artpec6_pcie_readl(artpec6_pcie, PCIECFG);
-	val |= PCIECFG_CORE_RESET_REQ;
-	artpec6_pcie_writel(artpec6_pcie, PCIECFG, val);
-
 	val = artpec6_pcie_readl(artpec6_pcie, PCIECFG);
 	val |=  PCIECFG_RISRCREN |	/* Receiver term. 50 Ohm */
 		PCIECFG_MODE_TX_DRV_EN |
@@ -131,30 +124,18 @@ static int artpec6_pcie_establish_link(struct artpec6_pcie *artpec6_pcie)
 		val = readl(artpec6_pcie->phy_base + PHY_STATUS);
 		retries--;
 	} while (retries && !(val & PHY_COSPLLLOCK));
+}
 
-	/* Take DW core out of reset */
-	val = artpec6_pcie_readl(artpec6_pcie, PCIECFG);
-	val &= ~PCIECFG_CORE_RESET_REQ;
-	artpec6_pcie_writel(artpec6_pcie, PCIECFG, val);
-	usleep_range(100, 200);
-
-	/* setup root complex */
-	dw_pcie_setup_rc(pp);
+static int artpec6_pcie_establish_link(struct dw_pcie *pci)
+{
+	struct artpec6_pcie *artpec6_pcie = to_artpec6_pcie(pci);
+	u32 val;
 
-	/* assert LTSSM enable */
 	val = artpec6_pcie_readl(artpec6_pcie, PCIECFG);
 	val |= PCIECFG_LTSSM_ENABLE;
 	artpec6_pcie_writel(artpec6_pcie, PCIECFG, val);
 
-	/* check if the link is up or not */
-	if (!dw_pcie_wait_for_link(pci))
-		return 0;
-
-	dev_dbg(pci->dev, "DEBUG_R0: 0x%08x, DEBUG_R1: 0x%08x\n",
-		dw_pcie_readl_dbi(pci, PCIE_PHY_DEBUG_R0),
-		dw_pcie_readl_dbi(pci, PCIE_PHY_DEBUG_R1));
-
-	return -ETIMEDOUT;
+	return 0;
 }
 
 static void artpec6_pcie_enable_interrupts(struct artpec6_pcie *artpec6_pcie)
@@ -166,12 +147,36 @@ static void artpec6_pcie_enable_interrupts(struct artpec6_pcie *artpec6_pcie)
 		dw_pcie_msi_init(pp);
 }
 
+static void artpec6_pcie_assert_core_reset(struct artpec6_pcie *artpec6_pcie)
+{
+	u32 val;
+
+	val = artpec6_pcie_readl(artpec6_pcie, PCIECFG);
+	val |= PCIECFG_CORE_RESET_REQ;
+	artpec6_pcie_writel(artpec6_pcie, PCIECFG, val);
+}
+
+static void artpec6_pcie_deassert_core_reset(struct artpec6_pcie *artpec6_pcie)
+{
+	u32 val;
+
+	val = artpec6_pcie_readl(artpec6_pcie, PCIECFG);
+	val &= ~PCIECFG_CORE_RESET_REQ;
+	artpec6_pcie_writel(artpec6_pcie, PCIECFG, val);
+	usleep_range(100, 200);
+}
+
 static int artpec6_pcie_host_init(struct pcie_port *pp)
 {
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
 	struct artpec6_pcie *artpec6_pcie = to_artpec6_pcie(pci);
 
-	artpec6_pcie_establish_link(artpec6_pcie);
+	artpec6_pcie_assert_core_reset(artpec6_pcie);
+	artpec6_pcie_init_phy(artpec6_pcie);
+	artpec6_pcie_deassert_core_reset(artpec6_pcie);
+	dw_pcie_setup_rc(pp);
+	artpec6_pcie_establish_link(pci);
+	dw_pcie_wait_for_link(pci);
 	artpec6_pcie_enable_interrupts(artpec6_pcie);
 
 	return 0;
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 41+ messages in thread

* [PATCH 08/10] PCI: dwc: artpec6: add support for endpoint mode
  2017-10-13 16:09 ` Niklas Cassel
                   ` (7 preceding siblings ...)
  (?)
@ 2017-10-13 16:09 ` Niklas Cassel
  2017-10-16 23:43   ` Bjorn Helgaas
                     ` (2 more replies)
  -1 siblings, 3 replies; 41+ messages in thread
From: Niklas Cassel @ 2017-10-13 16:09 UTC (permalink / raw)
  To: Niklas Cassel, Jesper Nilsson, Bjorn Helgaas, Rob Herring,
	Mark Rutland, Kishon Vijay Abraham I, Jingoo Han, Xiaowei Song,
	Peter Robinson, Tomasz Nowicki, Gabriele Paoloni, Duc Dang
  Cc: linux-arm-kernel, linux-pci, devicetree, linux-kernel

Signed-off-by: Niklas Cassel <niklas.cassel@axis.com>
---
 .../devicetree/bindings/pci/axis,artpec6-pcie.txt  |   3 +-
 drivers/pci/dwc/Kconfig                            |  41 +++--
 drivers/pci/dwc/Makefile                           |   4 +-
 drivers/pci/dwc/pcie-artpec6.c                     | 202 ++++++++++++++++++++-
 4 files changed, 233 insertions(+), 17 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/axis,artpec6-pcie.txt b/Documentation/devicetree/bindings/pci/axis,artpec6-pcie.txt
index 4e4aee4439ea..33eef7ae5a23 100644
--- a/Documentation/devicetree/bindings/pci/axis,artpec6-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/axis,artpec6-pcie.txt
@@ -4,7 +4,8 @@ This PCIe host controller is based on the Synopsys DesignWare PCIe IP
 and thus inherits all the common properties defined in designware-pcie.txt.
 
 Required properties:
-- compatible: "axis,artpec6-pcie", "snps,dw-pcie"
+- compatible: "axis,artpec6-pcie", "snps,dw-pcie" for ARTPEC-6 in RC mode;
+	      "axis,artpec6-pcie-ep", "snps,dw-pcie" for ARTPEC-6 in EP mode;
 - reg: base addresses and lengths of the PCIe controller (DBI),
 	the PHY controller, and configuration address space.
 - reg-names: Must include the following entries:
diff --git a/drivers/pci/dwc/Kconfig b/drivers/pci/dwc/Kconfig
index 22ec82fcdea2..e333283fb1ed 100644
--- a/drivers/pci/dwc/Kconfig
+++ b/drivers/pci/dwc/Kconfig
@@ -14,6 +14,36 @@ config PCIE_DW_EP
 	depends on PCI_ENDPOINT
 	select PCIE_DW
 
+config PCIE_ARTPEC6
+	bool "Axis ARTPEC-6 PCIe controller"
+	depends on (PCI && PCI_MSI_IRQ_DOMAIN) || PCI_ENDPOINT
+	depends on MACH_ARTPEC6
+	help
+	  Say Y here to enable PCIe controller support on Axis ARTPEC-6
+	  SoCs.  This PCIe controller uses the DesignWare core.
+
+if PCIE_ARTPEC6
+
+config PCIE_ARTPEC6_HOST
+	bool "Axis ARTPEC-6 Host Mode"
+	depends on PCI
+	depends on PCI_MSI_IRQ_DOMAIN
+	select PCIEPORTBUS
+	select PCIE_DW_HOST
+	help
+	  Enables support for the PCIe controller in the ARTPEC-6 SoC to work in
+	  host mode.
+
+config PCIE_ARTPEC6_EP
+	bool "Axis ARTPEC-6 Endpoint Mode"
+	depends on PCI_ENDPOINT
+	select PCIE_DW_EP
+	help
+	  Enables support for the PCIe controller in the ARTPEC-6 SoC to work in
+	  endpoint mode.
+
+endif
+
 config PCI_DRA7XX
 	bool "TI DRA7xx PCIe controller"
 	depends on SOC_DRA7XX || COMPILE_TEST
@@ -148,17 +178,6 @@ config PCIE_ARMADA_8K
 	  DesignWare hardware and therefore the driver re-uses the
 	  DesignWare core functions to implement the driver.
 
-config PCIE_ARTPEC6
-	bool "Axis ARTPEC-6 PCIe controller"
-	depends on PCI
-	depends on MACH_ARTPEC6
-	depends on PCI_MSI_IRQ_DOMAIN
-	select PCIEPORTBUS
-	select PCIE_DW_HOST
-	help
-	  Say Y here to enable PCIe controller support on Axis ARTPEC-6
-	  SoCs.  This PCIe controller uses the DesignWare core.
-
 config PCIE_KIRIN
 	depends on OF && ARM64
 	bool "HiSilicon Kirin series SoCs PCIe controllers"
diff --git a/drivers/pci/dwc/Makefile b/drivers/pci/dwc/Makefile
index c61be9738cce..ac98242b83a9 100644
--- a/drivers/pci/dwc/Makefile
+++ b/drivers/pci/dwc/Makefile
@@ -2,6 +2,9 @@ obj-$(CONFIG_PCIE_DW) += pcie-designware.o
 obj-$(CONFIG_PCIE_DW_HOST) += pcie-designware-host.o
 obj-$(CONFIG_PCIE_DW_EP) += pcie-designware-ep.o
 obj-$(CONFIG_PCIE_DW_PLAT) += pcie-designware-plat.o
+ifneq ($(filter y,$(CONFIG_PCIE_ARTPEC6_HOST) $(CONFIG_PCIE_ARTPEC6_EP)),)
+        obj-$(CONFIG_PCIE_ARTPEC6) += pcie-artpec6.o
+endif
 ifneq ($(filter y,$(CONFIG_PCI_DRA7XX_HOST) $(CONFIG_PCI_DRA7XX_EP)),)
         obj-$(CONFIG_PCI_DRA7XX) += pci-dra7xx.o
 endif
@@ -12,7 +15,6 @@ obj-$(CONFIG_PCI_KEYSTONE) += pci-keystone-dw.o pci-keystone.o
 obj-$(CONFIG_PCI_LAYERSCAPE) += pci-layerscape.o
 obj-$(CONFIG_PCIE_QCOM) += pcie-qcom.o
 obj-$(CONFIG_PCIE_ARMADA_8K) += pcie-armada8k.o
-obj-$(CONFIG_PCIE_ARTPEC6) += pcie-artpec6.o
 obj-$(CONFIG_PCIE_KIRIN) += pcie-kirin.o
 
 # The following drivers are for devices that use the generic ACPI
diff --git a/drivers/pci/dwc/pcie-artpec6.c b/drivers/pci/dwc/pcie-artpec6.c
index c5d7f98dc6b2..21ea9ffef784 100644
--- a/drivers/pci/dwc/pcie-artpec6.c
+++ b/drivers/pci/dwc/pcie-artpec6.c
@@ -13,6 +13,7 @@
 #include <linux/delay.h>
 #include <linux/kernel.h>
 #include <linux/init.h>
+#include <linux/of_device.h>
 #include <linux/pci.h>
 #include <linux/platform_device.h>
 #include <linux/resource.h>
@@ -30,8 +31,15 @@ struct artpec6_pcie {
 	struct dw_pcie		*pci;
 	struct regmap		*regmap;	/* DT axis,syscon-pcie */
 	void __iomem		*phy_base;	/* DT phy */
+	enum dw_pcie_device_mode mode;
 };
 
+struct artpec_pcie_of_data {
+	enum dw_pcie_device_mode mode;
+};
+
+static const struct of_device_id artpec6_pcie_of_match[];
+
 /* PCIe Port Logic registers (memory-mapped) */
 #define PL_OFFSET			0x700
 #define PCIE_PHY_DEBUG_R0		(PL_OFFSET + 0x28)
@@ -42,6 +50,7 @@ struct artpec6_pcie {
 #define  PCIECFG_DBG_OEN		BIT(24)
 #define  PCIECFG_CORE_RESET_REQ		BIT(21)
 #define  PCIECFG_LTSSM_ENABLE		BIT(20)
+#define  PCIECFG_DEVICE_TYPE_MASK	GENMASK(19, 16)
 #define  PCIECFG_CLKREQ_B		BIT(11)
 #define  PCIECFG_REFCLK_ENABLE		BIT(10)
 #define  PCIECFG_PLL_ENABLE		BIT(9)
@@ -126,6 +135,16 @@ static void artpec6_pcie_init_phy(struct artpec6_pcie *artpec6_pcie)
 	} while (retries && !(val & PHY_COSPLLLOCK));
 }
 
+static void artpec6_pcie_stop_link(struct dw_pcie *pci)
+{
+	struct artpec6_pcie *artpec6_pcie = to_artpec6_pcie(pci);
+	u32 val;
+
+	val = artpec6_pcie_readl(artpec6_pcie, PCIECFG);
+	val &= ~PCIECFG_LTSSM_ENABLE;
+	artpec6_pcie_writel(artpec6_pcie, PCIECFG, val);
+}
+
 static int artpec6_pcie_establish_link(struct dw_pcie *pci)
 {
 	struct artpec6_pcie *artpec6_pcie = to_artpec6_pcie(pci);
@@ -195,6 +214,136 @@ static irqreturn_t artpec6_pcie_msi_handler(int irq, void *arg)
 	return dw_handle_msi_irq(pp);
 }
 
+static void dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar)
+{
+	u32 reg;
+
+	reg = PCI_BASE_ADDRESS_0 + (4 * bar);
+	dw_pcie_dbi_ro_wr_en(pci);
+	dw_pcie_writel_dbi2(pci, reg, 0x0);
+	dw_pcie_writel_dbi(pci, reg, 0x0);
+	dw_pcie_dbi_ro_wr_dis(pci);
+}
+
+static void artpec6_pcie_ep_init(struct dw_pcie_ep *ep)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
+	struct artpec6_pcie *artpec6_pcie = to_artpec6_pcie(pci);
+	enum pci_barno bar;
+
+	artpec6_pcie_assert_core_reset(artpec6_pcie);
+	artpec6_pcie_init_phy(artpec6_pcie);
+	artpec6_pcie_deassert_core_reset(artpec6_pcie);
+
+	for (bar = BAR_0; bar <= BAR_5; bar++)
+		dw_pcie_ep_reset_bar(pci, bar);
+}
+
+static int artpec6_pcie_raise_msi_irq(struct dw_pcie_ep *ep,
+				      u8 interrupt_num)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
+	struct pci_epc *epc = ep->epc;
+	u32 cap_addr, cap_value, cap_id, next_ptr, msg_ctrl, msg_data;
+	u32 msg_addr_lower, msg_addr_upper;
+	u64 msg_addr;
+	bool has_upper;
+	int ret;
+
+	/* Raise MSI per the PCI Local Bus Specification Revision 3.0, 6.8.1. */
+	cap_addr = dw_pcie_readl_dbi(pci, PCI_CAPABILITY_LIST);
+	while (1) {
+		cap_value = dw_pcie_readl_dbi(pci, cap_addr);
+		cap_id = cap_value & GENMASK(7, 0);
+		if (cap_id == PCI_CAP_ID_MSI)
+			break;
+		next_ptr = (cap_value & GENMASK(15, 8)) >> 8;
+		if (next_ptr == 0) {
+			dev_err(pci->dev, "No MSI cap found!\n");
+			return -EINVAL;
+		}
+		cap_addr = next_ptr;
+	}
+	msg_ctrl = (cap_value & GENMASK(31, 16)) >> 16;
+	has_upper = !!(msg_ctrl & BIT(7));
+	msg_addr_lower = dw_pcie_readl_dbi(pci, cap_addr + 0x4);
+	if (has_upper) {
+		msg_addr_upper = dw_pcie_readl_dbi(pci, cap_addr + 0x8);
+		msg_data = dw_pcie_readl_dbi(pci, cap_addr + 0xc) &
+			GENMASK(15, 0);
+	} else {
+		msg_addr_upper = 0;
+		msg_data = dw_pcie_readl_dbi(pci, cap_addr + 0x8) &
+			GENMASK(15, 0);
+	}
+	msg_addr = ((u64) msg_addr_upper) << 32 | msg_addr_lower;
+	ret = epc->ops->map_addr(epc, ep->msi_mem_phys, msg_addr, PAGE_SIZE);
+	if (ret)
+		return ret;
+
+	writel(msg_data | (interrupt_num - 1), ep->msi_mem);
+
+	epc->ops->unmap_addr(epc, ep->msi_mem_phys);
+
+	return 0;
+}
+
+static int artpec6_pcie_raise_irq(struct dw_pcie_ep *ep,
+				  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 artpec6_pcie_raise_msi_irq(ep, interrupt_num);
+	default:
+		dev_err(pci->dev, "UNKNOWN IRQ type\n");
+	}
+
+	return 0;
+}
+
+static struct dw_pcie_ep_ops pcie_ep_ops = {
+	.ep_init = artpec6_pcie_ep_init,
+	.raise_irq = artpec6_pcie_raise_irq,
+};
+
+static int artpec6_add_pcie_ep(struct artpec6_pcie *artpec6_pcie,
+			       struct platform_device *pdev)
+{
+	int ret;
+	struct dw_pcie_ep *ep;
+	struct resource *res;
+	struct device *dev = &pdev->dev;
+	struct dw_pcie *pci = artpec6_pcie->pci;
+
+	ep = &pci->ep;
+	ep->ops = &pcie_ep_ops;
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi2");
+	pci->dbi_base2 = devm_ioremap(dev, res->start, resource_size(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 artpec6_add_pcie_port(struct artpec6_pcie *artpec6_pcie,
 				 struct platform_device *pdev)
 {
@@ -234,6 +383,8 @@ static int artpec6_add_pcie_port(struct artpec6_pcie *artpec6_pcie,
 
 static const struct dw_pcie_ops dw_pcie_ops = {
 	.cpu_addr_fixup = artpec6_pcie_cpu_addr_fixup,
+	.start_link = artpec6_pcie_establish_link,
+	.stop_link = artpec6_pcie_stop_link,
 };
 
 static int artpec6_pcie_probe(struct platform_device *pdev)
@@ -244,6 +395,16 @@ static int artpec6_pcie_probe(struct platform_device *pdev)
 	struct resource *dbi_base;
 	struct resource *phy_base;
 	int ret;
+	const struct of_device_id *match;
+	const struct artpec_pcie_of_data *data;
+	enum dw_pcie_device_mode mode;
+
+	match = of_match_device(artpec6_pcie_of_match, dev);
+	if (!match)
+		return -EINVAL;
+
+	data = (struct artpec_pcie_of_data *)match->data;
+	mode = (enum dw_pcie_device_mode)data->mode;
 
 	artpec6_pcie = devm_kzalloc(dev, sizeof(*artpec6_pcie), GFP_KERNEL);
 	if (!artpec6_pcie)
@@ -257,6 +418,7 @@ static int artpec6_pcie_probe(struct platform_device *pdev)
 	pci->ops = &dw_pcie_ops;
 
 	artpec6_pcie->pci = pci;
+	artpec6_pcie->mode = mode;
 
 	dbi_base = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi");
 	pci->dbi_base = devm_ioremap_resource(dev, dbi_base);
@@ -276,15 +438,47 @@ static int artpec6_pcie_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, artpec6_pcie);
 
-	ret = artpec6_add_pcie_port(artpec6_pcie, pdev);
-	if (ret < 0)
-		return ret;
+	switch (artpec6_pcie->mode) {
+	case DW_PCIE_RC_TYPE:
+		ret = artpec6_add_pcie_port(artpec6_pcie, pdev);
+		if (ret < 0)
+			return ret;
+		break;
+	case DW_PCIE_EP_TYPE: {
+		u32 val;
+
+		val = artpec6_pcie_readl(artpec6_pcie, PCIECFG);
+		val &= ~PCIECFG_DEVICE_TYPE_MASK;
+		artpec6_pcie_writel(artpec6_pcie, PCIECFG, val);
+		ret = artpec6_add_pcie_ep(artpec6_pcie, pdev);
+		if (ret < 0)
+			return ret;
+		break;
+	}
+	default:
+		dev_err(dev, "INVALID device type %d\n", artpec6_pcie->mode);
+	}
 
 	return 0;
 }
 
+static const struct artpec_pcie_of_data artpec6_pcie_rc_of_data = {
+	.mode = DW_PCIE_RC_TYPE,
+};
+
+static const struct artpec_pcie_of_data artpec6_pcie_ep_of_data = {
+	.mode = DW_PCIE_EP_TYPE,
+};
+
 static const struct of_device_id artpec6_pcie_of_match[] = {
-	{ .compatible = "axis,artpec6-pcie", },
+	{
+		.compatible = "axis,artpec6-pcie",
+		.data = &artpec6_pcie_rc_of_data,
+	},
+	{
+		.compatible = "axis,artpec6-pcie-ep",
+		.data = &artpec6_pcie_ep_of_data,
+	},
 	{},
 };
 
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 41+ messages in thread

* Re: [PATCH 08/10] PCI: dwc: artpec6: add support for endpoint mode
  2017-10-13 16:09 ` [PATCH 08/10] PCI: dwc: artpec6: add support for endpoint mode Niklas Cassel
@ 2017-10-16 23:43   ` Bjorn Helgaas
  2017-10-18  8:03       ` Kishon Vijay Abraham I
                       ` (2 more replies)
  2017-10-17 22:24   ` Rob Herring
  2017-10-18  8:46     ` Kishon Vijay Abraham I
  2 siblings, 3 replies; 41+ messages in thread
From: Bjorn Helgaas @ 2017-10-16 23:43 UTC (permalink / raw)
  To: Niklas Cassel
  Cc: Niklas Cassel, Jesper Nilsson, Bjorn Helgaas, Rob Herring,
	Mark Rutland, Kishon Vijay Abraham I, Jingoo Han, Xiaowei Song,
	Peter Robinson, Tomasz Nowicki, Gabriele Paoloni, Duc Dang,
	linux-arm-kernel, linux-pci, devicetree, linux-kernel

On Fri, Oct 13, 2017 at 06:09:11PM +0200, Niklas Cassel wrote:
> Signed-off-by: Niklas Cassel <niklas.cassel@axis.com>
> ---
>  .../devicetree/bindings/pci/axis,artpec6-pcie.txt  |   3 +-
>  drivers/pci/dwc/Kconfig                            |  41 +++--
>  drivers/pci/dwc/Makefile                           |   4 +-
>  drivers/pci/dwc/pcie-artpec6.c                     | 202 ++++++++++++++++++++-
>  4 files changed, 233 insertions(+), 17 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pci/axis,artpec6-pcie.txt b/Documentation/devicetree/bindings/pci/axis,artpec6-pcie.txt
> index 4e4aee4439ea..33eef7ae5a23 100644
> --- a/Documentation/devicetree/bindings/pci/axis,artpec6-pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/axis,artpec6-pcie.txt
> @@ -4,7 +4,8 @@ This PCIe host controller is based on the Synopsys DesignWare PCIe IP
>  and thus inherits all the common properties defined in designware-pcie.txt.
>  
>  Required properties:
> -- compatible: "axis,artpec6-pcie", "snps,dw-pcie"
> +- compatible: "axis,artpec6-pcie", "snps,dw-pcie" for ARTPEC-6 in RC mode;
> +	      "axis,artpec6-pcie-ep", "snps,dw-pcie" for ARTPEC-6 in EP mode;
>  - reg: base addresses and lengths of the PCIe controller (DBI),
>  	the PHY controller, and configuration address space.
>  - reg-names: Must include the following entries:
> diff --git a/drivers/pci/dwc/Kconfig b/drivers/pci/dwc/Kconfig
> index 22ec82fcdea2..e333283fb1ed 100644
> --- a/drivers/pci/dwc/Kconfig
> +++ b/drivers/pci/dwc/Kconfig
> @@ -14,6 +14,36 @@ config PCIE_DW_EP
>  	depends on PCI_ENDPOINT
>  	select PCIE_DW
>  
> +config PCIE_ARTPEC6
> +	bool "Axis ARTPEC-6 PCIe controller"
> +	depends on (PCI && PCI_MSI_IRQ_DOMAIN) || PCI_ENDPOINT
> +	depends on MACH_ARTPEC6
> +	help
> +	  Say Y here to enable PCIe controller support on Axis ARTPEC-6
> +	  SoCs.  This PCIe controller uses the DesignWare core.
> +
> +if PCIE_ARTPEC6
> +
> +config PCIE_ARTPEC6_HOST
> +	bool "Axis ARTPEC-6 Host Mode"
> +	depends on PCI
> +	depends on PCI_MSI_IRQ_DOMAIN
> +	select PCIEPORTBUS
> +	select PCIE_DW_HOST
> +	help
> +	  Enables support for the PCIe controller in the ARTPEC-6 SoC to work in
> +	  host mode.
> +
> +config PCIE_ARTPEC6_EP
> +	bool "Axis ARTPEC-6 Endpoint Mode"
> +	depends on PCI_ENDPOINT
> +	select PCIE_DW_EP
> +	help
> +	  Enables support for the PCIe controller in the ARTPEC-6 SoC to work in
> +	  endpoint mode.
> +
> +endif
> +
>  config PCI_DRA7XX
>  	bool "TI DRA7xx PCIe controller"
>  	depends on SOC_DRA7XX || COMPILE_TEST
> @@ -148,17 +178,6 @@ config PCIE_ARMADA_8K
>  	  DesignWare hardware and therefore the driver re-uses the
>  	  DesignWare core functions to implement the driver.
>  
> -config PCIE_ARTPEC6
> -	bool "Axis ARTPEC-6 PCIe controller"
> -	depends on PCI
> -	depends on MACH_ARTPEC6
> -	depends on PCI_MSI_IRQ_DOMAIN
> -	select PCIEPORTBUS
> -	select PCIE_DW_HOST
> -	help
> -	  Say Y here to enable PCIe controller support on Axis ARTPEC-6
> -	  SoCs.  This PCIe controller uses the DesignWare core.
> -
>  config PCIE_KIRIN
>  	depends on OF && ARM64
>  	bool "HiSilicon Kirin series SoCs PCIe controllers"

> diff --git a/drivers/pci/dwc/Makefile b/drivers/pci/dwc/Makefile
> index c61be9738cce..ac98242b83a9 100644
> --- a/drivers/pci/dwc/Makefile
> +++ b/drivers/pci/dwc/Makefile
> @@ -2,6 +2,9 @@ obj-$(CONFIG_PCIE_DW) += pcie-designware.o
>  obj-$(CONFIG_PCIE_DW_HOST) += pcie-designware-host.o
>  obj-$(CONFIG_PCIE_DW_EP) += pcie-designware-ep.o
>  obj-$(CONFIG_PCIE_DW_PLAT) += pcie-designware-plat.o
> +ifneq ($(filter y,$(CONFIG_PCIE_ARTPEC6_HOST) $(CONFIG_PCIE_ARTPEC6_EP)),)
> +        obj-$(CONFIG_PCIE_ARTPEC6) += pcie-artpec6.o
> +endif

I see you're copying the DRA7XX style here, but I don't really
understand it.  I guess the idea is to build pcie-artpec6.o if either
CONFIG_PCIE_ARTPEC6_HOST or CONFIG_PCIE_ARTPEC6_EP is set (or both).

Is this really the simplest way to express that in Kconfig?  Both the
"if PCIE_ARTPEC6" and this ifneq thing are complicated and relatively
unusual.

>  ifneq ($(filter y,$(CONFIG_PCI_DRA7XX_HOST) $(CONFIG_PCI_DRA7XX_EP)),)
>          obj-$(CONFIG_PCI_DRA7XX) += pci-dra7xx.o
>  endif

> +static void dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar)

Looks funny to have this dw_*() function in pcie-artpec6.c.  Intended?
Or is this something other endpoint drivers could/should share?

> +{
> +	u32 reg;
> +
> +	reg = PCI_BASE_ADDRESS_0 + (4 * bar);
> +	dw_pcie_dbi_ro_wr_en(pci);
> +	dw_pcie_writel_dbi2(pci, reg, 0x0);
> +	dw_pcie_writel_dbi(pci, reg, 0x0);
> +	dw_pcie_dbi_ro_wr_dis(pci);
> +}
> +
> +static void artpec6_pcie_ep_init(struct dw_pcie_ep *ep)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> +	struct artpec6_pcie *artpec6_pcie = to_artpec6_pcie(pci);
> +	enum pci_barno bar;
> +
> +	artpec6_pcie_assert_core_reset(artpec6_pcie);
> +	artpec6_pcie_init_phy(artpec6_pcie);
> +	artpec6_pcie_deassert_core_reset(artpec6_pcie);
> +
> +	for (bar = BAR_0; bar <= BAR_5; bar++)
> +		dw_pcie_ep_reset_bar(pci, bar);
> +}

I naively expected some of this new code to be #ifdef PCIE_ARTPEC6_EP.
But I haven't really internalized the endpoint support, so maybe that
doesn't make sense?

Bjorn

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH 08/10] PCI: dwc: artpec6: add support for endpoint mode
  2017-10-16 23:43   ` Bjorn Helgaas
@ 2017-10-18  8:03       ` Kishon Vijay Abraham I
  2017-10-19  7:59       ` Christoph Hellwig
  2017-10-19 11:40       ` Niklas Cassel
  2 siblings, 0 replies; 41+ messages in thread
From: Kishon Vijay Abraham I @ 2017-10-18  8:03 UTC (permalink / raw)
  To: Bjorn Helgaas, Niklas Cassel
  Cc: Niklas Cassel, Jesper Nilsson, Bjorn Helgaas, Rob Herring,
	Mark Rutland, Jingoo Han, Xiaowei Song, Peter Robinson,
	Tomasz Nowicki, Gabriele Paoloni, Duc Dang, linux-arm-kernel,
	linux-pci, devicetree, linux-kernel

Hi Bjorn,

On Tuesday 17 October 2017 05:13 AM, Bjorn Helgaas wrote:
> On Fri, Oct 13, 2017 at 06:09:11PM +0200, Niklas Cassel wrote:
>> Signed-off-by: Niklas Cassel <niklas.cassel@axis.com>
>> ---
>>  .../devicetree/bindings/pci/axis,artpec6-pcie.txt  |   3 +-
>>  drivers/pci/dwc/Kconfig                            |  41 +++--
>>  drivers/pci/dwc/Makefile                           |   4 +-
>>  drivers/pci/dwc/pcie-artpec6.c                     | 202 ++++++++++++++++++++-
>>  4 files changed, 233 insertions(+), 17 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/pci/axis,artpec6-pcie.txt b/Documentation/devicetree/bindings/pci/axis,artpec6-pcie.txt
>> index 4e4aee4439ea..33eef7ae5a23 100644
>> --- a/Documentation/devicetree/bindings/pci/axis,artpec6-pcie.txt
>> +++ b/Documentation/devicetree/bindings/pci/axis,artpec6-pcie.txt
>> @@ -4,7 +4,8 @@ This PCIe host controller is based on the Synopsys DesignWare PCIe IP
>>  and thus inherits all the common properties defined in designware-pcie.txt.
>>  
>>  Required properties:
>> -- compatible: "axis,artpec6-pcie", "snps,dw-pcie"
>> +- compatible: "axis,artpec6-pcie", "snps,dw-pcie" for ARTPEC-6 in RC mode;
>> +	      "axis,artpec6-pcie-ep", "snps,dw-pcie" for ARTPEC-6 in EP mode;
>>  - reg: base addresses and lengths of the PCIe controller (DBI),
>>  	the PHY controller, and configuration address space.
>>  - reg-names: Must include the following entries:
>> diff --git a/drivers/pci/dwc/Kconfig b/drivers/pci/dwc/Kconfig
>> index 22ec82fcdea2..e333283fb1ed 100644
>> --- a/drivers/pci/dwc/Kconfig
>> +++ b/drivers/pci/dwc/Kconfig
>> @@ -14,6 +14,36 @@ config PCIE_DW_EP
>>  	depends on PCI_ENDPOINT
>>  	select PCIE_DW
>>  
>> +config PCIE_ARTPEC6
>> +	bool "Axis ARTPEC-6 PCIe controller"
>> +	depends on (PCI && PCI_MSI_IRQ_DOMAIN) || PCI_ENDPOINT
>> +	depends on MACH_ARTPEC6
>> +	help
>> +	  Say Y here to enable PCIe controller support on Axis ARTPEC-6
>> +	  SoCs.  This PCIe controller uses the DesignWare core.
>> +
>> +if PCIE_ARTPEC6
>> +
>> +config PCIE_ARTPEC6_HOST
>> +	bool "Axis ARTPEC-6 Host Mode"
>> +	depends on PCI
>> +	depends on PCI_MSI_IRQ_DOMAIN
>> +	select PCIEPORTBUS
>> +	select PCIE_DW_HOST
>> +	help
>> +	  Enables support for the PCIe controller in the ARTPEC-6 SoC to work in
>> +	  host mode.
>> +
>> +config PCIE_ARTPEC6_EP
>> +	bool "Axis ARTPEC-6 Endpoint Mode"
>> +	depends on PCI_ENDPOINT
>> +	select PCIE_DW_EP
>> +	help
>> +	  Enables support for the PCIe controller in the ARTPEC-6 SoC to work in
>> +	  endpoint mode.
>> +
>> +endif
>> +
>>  config PCI_DRA7XX
>>  	bool "TI DRA7xx PCIe controller"
>>  	depends on SOC_DRA7XX || COMPILE_TEST
>> @@ -148,17 +178,6 @@ config PCIE_ARMADA_8K
>>  	  DesignWare hardware and therefore the driver re-uses the
>>  	  DesignWare core functions to implement the driver.
>>  
>> -config PCIE_ARTPEC6
>> -	bool "Axis ARTPEC-6 PCIe controller"
>> -	depends on PCI
>> -	depends on MACH_ARTPEC6
>> -	depends on PCI_MSI_IRQ_DOMAIN
>> -	select PCIEPORTBUS
>> -	select PCIE_DW_HOST
>> -	help
>> -	  Say Y here to enable PCIe controller support on Axis ARTPEC-6
>> -	  SoCs.  This PCIe controller uses the DesignWare core.
>> -
>>  config PCIE_KIRIN
>>  	depends on OF && ARM64
>>  	bool "HiSilicon Kirin series SoCs PCIe controllers"
> 
>> diff --git a/drivers/pci/dwc/Makefile b/drivers/pci/dwc/Makefile
>> index c61be9738cce..ac98242b83a9 100644
>> --- a/drivers/pci/dwc/Makefile
>> +++ b/drivers/pci/dwc/Makefile
>> @@ -2,6 +2,9 @@ obj-$(CONFIG_PCIE_DW) += pcie-designware.o
>>  obj-$(CONFIG_PCIE_DW_HOST) += pcie-designware-host.o
>>  obj-$(CONFIG_PCIE_DW_EP) += pcie-designware-ep.o
>>  obj-$(CONFIG_PCIE_DW_PLAT) += pcie-designware-plat.o
>> +ifneq ($(filter y,$(CONFIG_PCIE_ARTPEC6_HOST) $(CONFIG_PCIE_ARTPEC6_EP)),)
>> +        obj-$(CONFIG_PCIE_ARTPEC6) += pcie-artpec6.o
>> +endif
> 
> I see you're copying the DRA7XX style here, but I don't really
> understand it.  I guess the idea is to build pcie-artpec6.o if either
> CONFIG_PCIE_ARTPEC6_HOST or CONFIG_PCIE_ARTPEC6_EP is set (or both).
> 
> Is this really the simplest way to express that in Kconfig?  Both the
> "if PCIE_ARTPEC6" and this ifneq thing are complicated and relatively
> unusual.
> 
>>  ifneq ($(filter y,$(CONFIG_PCI_DRA7XX_HOST) $(CONFIG_PCI_DRA7XX_EP)),)
>>          obj-$(CONFIG_PCI_DRA7XX) += pci-dra7xx.o
>>  endif
> 
>> +static void dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar)
> 
> Looks funny to have this dw_*() function in pcie-artpec6.c.  Intended?
> Or is this something other endpoint drivers could/should share?

yeah, dw_pcie_ep_reset_bar is already present in pcie-designware-ep.c which
should be made as library function to be used by other drivers. (pci-dra7xx.c
has also added something like above which should also be fixed).

Thanks
Kishon

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH 08/10] PCI: dwc: artpec6: add support for endpoint mode
@ 2017-10-18  8:03       ` Kishon Vijay Abraham I
  0 siblings, 0 replies; 41+ messages in thread
From: Kishon Vijay Abraham I @ 2017-10-18  8:03 UTC (permalink / raw)
  To: Bjorn Helgaas, Niklas Cassel
  Cc: Niklas Cassel, Jesper Nilsson, Bjorn Helgaas, Rob Herring,
	Mark Rutland, Jingoo Han, Xiaowei Song, Peter Robinson,
	Tomasz Nowicki, Gabriele Paoloni, Duc Dang, linux-arm-kernel,
	linux-pci, devicetree, linux-kernel

Hi Bjorn,

On Tuesday 17 October 2017 05:13 AM, Bjorn Helgaas wrote:
> On Fri, Oct 13, 2017 at 06:09:11PM +0200, Niklas Cassel wrote:
>> Signed-off-by: Niklas Cassel <niklas.cassel@axis.com>
>> ---
>>  .../devicetree/bindings/pci/axis,artpec6-pcie.txt  |   3 +-
>>  drivers/pci/dwc/Kconfig                            |  41 +++--
>>  drivers/pci/dwc/Makefile                           |   4 +-
>>  drivers/pci/dwc/pcie-artpec6.c                     | 202 ++++++++++++++++++++-
>>  4 files changed, 233 insertions(+), 17 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/pci/axis,artpec6-pcie.txt b/Documentation/devicetree/bindings/pci/axis,artpec6-pcie.txt
>> index 4e4aee4439ea..33eef7ae5a23 100644
>> --- a/Documentation/devicetree/bindings/pci/axis,artpec6-pcie.txt
>> +++ b/Documentation/devicetree/bindings/pci/axis,artpec6-pcie.txt
>> @@ -4,7 +4,8 @@ This PCIe host controller is based on the Synopsys DesignWare PCIe IP
>>  and thus inherits all the common properties defined in designware-pcie.txt.
>>  
>>  Required properties:
>> -- compatible: "axis,artpec6-pcie", "snps,dw-pcie"
>> +- compatible: "axis,artpec6-pcie", "snps,dw-pcie" for ARTPEC-6 in RC mode;
>> +	      "axis,artpec6-pcie-ep", "snps,dw-pcie" for ARTPEC-6 in EP mode;
>>  - reg: base addresses and lengths of the PCIe controller (DBI),
>>  	the PHY controller, and configuration address space.
>>  - reg-names: Must include the following entries:
>> diff --git a/drivers/pci/dwc/Kconfig b/drivers/pci/dwc/Kconfig
>> index 22ec82fcdea2..e333283fb1ed 100644
>> --- a/drivers/pci/dwc/Kconfig
>> +++ b/drivers/pci/dwc/Kconfig
>> @@ -14,6 +14,36 @@ config PCIE_DW_EP
>>  	depends on PCI_ENDPOINT
>>  	select PCIE_DW
>>  
>> +config PCIE_ARTPEC6
>> +	bool "Axis ARTPEC-6 PCIe controller"
>> +	depends on (PCI && PCI_MSI_IRQ_DOMAIN) || PCI_ENDPOINT
>> +	depends on MACH_ARTPEC6
>> +	help
>> +	  Say Y here to enable PCIe controller support on Axis ARTPEC-6
>> +	  SoCs.  This PCIe controller uses the DesignWare core.
>> +
>> +if PCIE_ARTPEC6
>> +
>> +config PCIE_ARTPEC6_HOST
>> +	bool "Axis ARTPEC-6 Host Mode"
>> +	depends on PCI
>> +	depends on PCI_MSI_IRQ_DOMAIN
>> +	select PCIEPORTBUS
>> +	select PCIE_DW_HOST
>> +	help
>> +	  Enables support for the PCIe controller in the ARTPEC-6 SoC to work in
>> +	  host mode.
>> +
>> +config PCIE_ARTPEC6_EP
>> +	bool "Axis ARTPEC-6 Endpoint Mode"
>> +	depends on PCI_ENDPOINT
>> +	select PCIE_DW_EP
>> +	help
>> +	  Enables support for the PCIe controller in the ARTPEC-6 SoC to work in
>> +	  endpoint mode.
>> +
>> +endif
>> +
>>  config PCI_DRA7XX
>>  	bool "TI DRA7xx PCIe controller"
>>  	depends on SOC_DRA7XX || COMPILE_TEST
>> @@ -148,17 +178,6 @@ config PCIE_ARMADA_8K
>>  	  DesignWare hardware and therefore the driver re-uses the
>>  	  DesignWare core functions to implement the driver.
>>  
>> -config PCIE_ARTPEC6
>> -	bool "Axis ARTPEC-6 PCIe controller"
>> -	depends on PCI
>> -	depends on MACH_ARTPEC6
>> -	depends on PCI_MSI_IRQ_DOMAIN
>> -	select PCIEPORTBUS
>> -	select PCIE_DW_HOST
>> -	help
>> -	  Say Y here to enable PCIe controller support on Axis ARTPEC-6
>> -	  SoCs.  This PCIe controller uses the DesignWare core.
>> -
>>  config PCIE_KIRIN
>>  	depends on OF && ARM64
>>  	bool "HiSilicon Kirin series SoCs PCIe controllers"
> 
>> diff --git a/drivers/pci/dwc/Makefile b/drivers/pci/dwc/Makefile
>> index c61be9738cce..ac98242b83a9 100644
>> --- a/drivers/pci/dwc/Makefile
>> +++ b/drivers/pci/dwc/Makefile
>> @@ -2,6 +2,9 @@ obj-$(CONFIG_PCIE_DW) += pcie-designware.o
>>  obj-$(CONFIG_PCIE_DW_HOST) += pcie-designware-host.o
>>  obj-$(CONFIG_PCIE_DW_EP) += pcie-designware-ep.o
>>  obj-$(CONFIG_PCIE_DW_PLAT) += pcie-designware-plat.o
>> +ifneq ($(filter y,$(CONFIG_PCIE_ARTPEC6_HOST) $(CONFIG_PCIE_ARTPEC6_EP)),)
>> +        obj-$(CONFIG_PCIE_ARTPEC6) += pcie-artpec6.o
>> +endif
> 
> I see you're copying the DRA7XX style here, but I don't really
> understand it.  I guess the idea is to build pcie-artpec6.o if either
> CONFIG_PCIE_ARTPEC6_HOST or CONFIG_PCIE_ARTPEC6_EP is set (or both).
> 
> Is this really the simplest way to express that in Kconfig?  Both the
> "if PCIE_ARTPEC6" and this ifneq thing are complicated and relatively
> unusual.
> 
>>  ifneq ($(filter y,$(CONFIG_PCI_DRA7XX_HOST) $(CONFIG_PCI_DRA7XX_EP)),)
>>          obj-$(CONFIG_PCI_DRA7XX) += pci-dra7xx.o
>>  endif
> 
>> +static void dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar)
> 
> Looks funny to have this dw_*() function in pcie-artpec6.c.  Intended?
> Or is this something other endpoint drivers could/should share?

yeah, dw_pcie_ep_reset_bar is already present in pcie-designware-ep.c which
should be made as library function to be used by other drivers. (pci-dra7xx.c
has also added something like above which should also be fixed).

Thanks
Kishon

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH 08/10] PCI: dwc: artpec6: add support for endpoint mode
  2017-10-18  8:03       ` Kishon Vijay Abraham I
@ 2017-10-18  8:15         ` Niklas Cassel
  -1 siblings, 0 replies; 41+ messages in thread
From: Niklas Cassel @ 2017-10-18  8:15 UTC (permalink / raw)
  To: Kishon Vijay Abraham I, Bjorn Helgaas
  Cc: Jesper Nilsson, Bjorn Helgaas, Rob Herring, Mark Rutland,
	Jingoo Han, Xiaowei Song, Peter Robinson, Tomasz Nowicki,
	Gabriele Paoloni, Duc Dang, linux-arm-kernel, linux-pci,
	devicetree, linux-kernel

On 10/18/2017 10:03 AM, Kishon Vijay Abraham I wrote:
> Hi Bjorn,
> 
> On Tuesday 17 October 2017 05:13 AM, Bjorn Helgaas wrote:
>> On Fri, Oct 13, 2017 at 06:09:11PM +0200, Niklas Cassel wrote:
>>> Signed-off-by: Niklas Cassel <niklas.cassel@axis.com>
>>> ---
>>>  .../devicetree/bindings/pci/axis,artpec6-pcie.txt  |   3 +-
>>>  drivers/pci/dwc/Kconfig                            |  41 +++--
>>>  drivers/pci/dwc/Makefile                           |   4 +-
>>>  drivers/pci/dwc/pcie-artpec6.c                     | 202 ++++++++++++++++++++-
>>>  4 files changed, 233 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/pci/axis,artpec6-pcie.txt b/Documentation/devicetree/bindings/pci/axis,artpec6-pcie.txt
>>> index 4e4aee4439ea..33eef7ae5a23 100644
>>> --- a/Documentation/devicetree/bindings/pci/axis,artpec6-pcie.txt
>>> +++ b/Documentation/devicetree/bindings/pci/axis,artpec6-pcie.txt
>>> @@ -4,7 +4,8 @@ This PCIe host controller is based on the Synopsys DesignWare PCIe IP
>>>  and thus inherits all the common properties defined in designware-pcie.txt.
>>>  
>>>  Required properties:
>>> -- compatible: "axis,artpec6-pcie", "snps,dw-pcie"
>>> +- compatible: "axis,artpec6-pcie", "snps,dw-pcie" for ARTPEC-6 in RC mode;
>>> +	      "axis,artpec6-pcie-ep", "snps,dw-pcie" for ARTPEC-6 in EP mode;
>>>  - reg: base addresses and lengths of the PCIe controller (DBI),
>>>  	the PHY controller, and configuration address space.
>>>  - reg-names: Must include the following entries:
>>> diff --git a/drivers/pci/dwc/Kconfig b/drivers/pci/dwc/Kconfig
>>> index 22ec82fcdea2..e333283fb1ed 100644
>>> --- a/drivers/pci/dwc/Kconfig
>>> +++ b/drivers/pci/dwc/Kconfig
>>> @@ -14,6 +14,36 @@ config PCIE_DW_EP
>>>  	depends on PCI_ENDPOINT
>>>  	select PCIE_DW
>>>  
>>> +config PCIE_ARTPEC6
>>> +	bool "Axis ARTPEC-6 PCIe controller"
>>> +	depends on (PCI && PCI_MSI_IRQ_DOMAIN) || PCI_ENDPOINT
>>> +	depends on MACH_ARTPEC6
>>> +	help
>>> +	  Say Y here to enable PCIe controller support on Axis ARTPEC-6
>>> +	  SoCs.  This PCIe controller uses the DesignWare core.
>>> +
>>> +if PCIE_ARTPEC6
>>> +
>>> +config PCIE_ARTPEC6_HOST
>>> +	bool "Axis ARTPEC-6 Host Mode"
>>> +	depends on PCI
>>> +	depends on PCI_MSI_IRQ_DOMAIN
>>> +	select PCIEPORTBUS
>>> +	select PCIE_DW_HOST
>>> +	help
>>> +	  Enables support for the PCIe controller in the ARTPEC-6 SoC to work in
>>> +	  host mode.
>>> +
>>> +config PCIE_ARTPEC6_EP
>>> +	bool "Axis ARTPEC-6 Endpoint Mode"
>>> +	depends on PCI_ENDPOINT
>>> +	select PCIE_DW_EP
>>> +	help
>>> +	  Enables support for the PCIe controller in the ARTPEC-6 SoC to work in
>>> +	  endpoint mode.
>>> +
>>> +endif
>>> +
>>>  config PCI_DRA7XX
>>>  	bool "TI DRA7xx PCIe controller"
>>>  	depends on SOC_DRA7XX || COMPILE_TEST
>>> @@ -148,17 +178,6 @@ config PCIE_ARMADA_8K
>>>  	  DesignWare hardware and therefore the driver re-uses the
>>>  	  DesignWare core functions to implement the driver.
>>>  
>>> -config PCIE_ARTPEC6
>>> -	bool "Axis ARTPEC-6 PCIe controller"
>>> -	depends on PCI
>>> -	depends on MACH_ARTPEC6
>>> -	depends on PCI_MSI_IRQ_DOMAIN
>>> -	select PCIEPORTBUS
>>> -	select PCIE_DW_HOST
>>> -	help
>>> -	  Say Y here to enable PCIe controller support on Axis ARTPEC-6
>>> -	  SoCs.  This PCIe controller uses the DesignWare core.
>>> -
>>>  config PCIE_KIRIN
>>>  	depends on OF && ARM64
>>>  	bool "HiSilicon Kirin series SoCs PCIe controllers"
>>
>>> diff --git a/drivers/pci/dwc/Makefile b/drivers/pci/dwc/Makefile
>>> index c61be9738cce..ac98242b83a9 100644
>>> --- a/drivers/pci/dwc/Makefile
>>> +++ b/drivers/pci/dwc/Makefile
>>> @@ -2,6 +2,9 @@ obj-$(CONFIG_PCIE_DW) += pcie-designware.o
>>>  obj-$(CONFIG_PCIE_DW_HOST) += pcie-designware-host.o
>>>  obj-$(CONFIG_PCIE_DW_EP) += pcie-designware-ep.o
>>>  obj-$(CONFIG_PCIE_DW_PLAT) += pcie-designware-plat.o
>>> +ifneq ($(filter y,$(CONFIG_PCIE_ARTPEC6_HOST) $(CONFIG_PCIE_ARTPEC6_EP)),)
>>> +        obj-$(CONFIG_PCIE_ARTPEC6) += pcie-artpec6.o
>>> +endif
>>
>> I see you're copying the DRA7XX style here, but I don't really
>> understand it.  I guess the idea is to build pcie-artpec6.o if either
>> CONFIG_PCIE_ARTPEC6_HOST or CONFIG_PCIE_ARTPEC6_EP is set (or both).
>>
>> Is this really the simplest way to express that in Kconfig?  Both the
>> "if PCIE_ARTPEC6" and this ifneq thing are complicated and relatively
>> unusual.
>>
>>>  ifneq ($(filter y,$(CONFIG_PCI_DRA7XX_HOST) $(CONFIG_PCI_DRA7XX_EP)),)
>>>          obj-$(CONFIG_PCI_DRA7XX) += pci-dra7xx.o
>>>  endif
>>
>>> +static void dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar)
>>
>> Looks funny to have this dw_*() function in pcie-artpec6.c.  Intended?
>> Or is this something other endpoint drivers could/should share?
> 
> yeah, dw_pcie_ep_reset_bar is already present in pcie-designware-ep.c which
> should be made as library function to be used by other drivers. (pci-dra7xx.c
> has also added something like above which should also be fixed).
> 

Hello Kishon,

I can remove the static keyword from dw_pcie_ep_reset_bar,
move the declaration to pcie-designware.h,
and make sure that both pcie-artpec6.c and pci-dra7xx.c uses it.

Will do this in the next version of the patch series if you
don't have any objection.


Regards,
Niklas

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH 08/10] PCI: dwc: artpec6: add support for endpoint mode
@ 2017-10-18  8:15         ` Niklas Cassel
  0 siblings, 0 replies; 41+ messages in thread
From: Niklas Cassel @ 2017-10-18  8:15 UTC (permalink / raw)
  To: Kishon Vijay Abraham I, Bjorn Helgaas
  Cc: Jesper Nilsson, Bjorn Helgaas, Rob Herring, Mark Rutland,
	Jingoo Han, Xiaowei Song, Peter Robinson, Tomasz Nowicki,
	Gabriele Paoloni, Duc Dang, linux-arm-kernel, linux-pci,
	devicetree, linux-kernel

On 10/18/2017 10:03 AM, Kishon Vijay Abraham I wrote:
> Hi Bjorn,
> 
> On Tuesday 17 October 2017 05:13 AM, Bjorn Helgaas wrote:
>> On Fri, Oct 13, 2017 at 06:09:11PM +0200, Niklas Cassel wrote:
>>> Signed-off-by: Niklas Cassel <niklas.cassel@axis.com>
>>> ---
>>>  .../devicetree/bindings/pci/axis,artpec6-pcie.txt  |   3 +-
>>>  drivers/pci/dwc/Kconfig                            |  41 +++--
>>>  drivers/pci/dwc/Makefile                           |   4 +-
>>>  drivers/pci/dwc/pcie-artpec6.c                     | 202 ++++++++++++++++++++-
>>>  4 files changed, 233 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/pci/axis,artpec6-pcie.txt b/Documentation/devicetree/bindings/pci/axis,artpec6-pcie.txt
>>> index 4e4aee4439ea..33eef7ae5a23 100644
>>> --- a/Documentation/devicetree/bindings/pci/axis,artpec6-pcie.txt
>>> +++ b/Documentation/devicetree/bindings/pci/axis,artpec6-pcie.txt
>>> @@ -4,7 +4,8 @@ This PCIe host controller is based on the Synopsys DesignWare PCIe IP
>>>  and thus inherits all the common properties defined in designware-pcie.txt.
>>>  
>>>  Required properties:
>>> -- compatible: "axis,artpec6-pcie", "snps,dw-pcie"
>>> +- compatible: "axis,artpec6-pcie", "snps,dw-pcie" for ARTPEC-6 in RC mode;
>>> +	      "axis,artpec6-pcie-ep", "snps,dw-pcie" for ARTPEC-6 in EP mode;
>>>  - reg: base addresses and lengths of the PCIe controller (DBI),
>>>  	the PHY controller, and configuration address space.
>>>  - reg-names: Must include the following entries:
>>> diff --git a/drivers/pci/dwc/Kconfig b/drivers/pci/dwc/Kconfig
>>> index 22ec82fcdea2..e333283fb1ed 100644
>>> --- a/drivers/pci/dwc/Kconfig
>>> +++ b/drivers/pci/dwc/Kconfig
>>> @@ -14,6 +14,36 @@ config PCIE_DW_EP
>>>  	depends on PCI_ENDPOINT
>>>  	select PCIE_DW
>>>  
>>> +config PCIE_ARTPEC6
>>> +	bool "Axis ARTPEC-6 PCIe controller"
>>> +	depends on (PCI && PCI_MSI_IRQ_DOMAIN) || PCI_ENDPOINT
>>> +	depends on MACH_ARTPEC6
>>> +	help
>>> +	  Say Y here to enable PCIe controller support on Axis ARTPEC-6
>>> +	  SoCs.  This PCIe controller uses the DesignWare core.
>>> +
>>> +if PCIE_ARTPEC6
>>> +
>>> +config PCIE_ARTPEC6_HOST
>>> +	bool "Axis ARTPEC-6 Host Mode"
>>> +	depends on PCI
>>> +	depends on PCI_MSI_IRQ_DOMAIN
>>> +	select PCIEPORTBUS
>>> +	select PCIE_DW_HOST
>>> +	help
>>> +	  Enables support for the PCIe controller in the ARTPEC-6 SoC to work in
>>> +	  host mode.
>>> +
>>> +config PCIE_ARTPEC6_EP
>>> +	bool "Axis ARTPEC-6 Endpoint Mode"
>>> +	depends on PCI_ENDPOINT
>>> +	select PCIE_DW_EP
>>> +	help
>>> +	  Enables support for the PCIe controller in the ARTPEC-6 SoC to work in
>>> +	  endpoint mode.
>>> +
>>> +endif
>>> +
>>>  config PCI_DRA7XX
>>>  	bool "TI DRA7xx PCIe controller"
>>>  	depends on SOC_DRA7XX || COMPILE_TEST
>>> @@ -148,17 +178,6 @@ config PCIE_ARMADA_8K
>>>  	  DesignWare hardware and therefore the driver re-uses the
>>>  	  DesignWare core functions to implement the driver.
>>>  
>>> -config PCIE_ARTPEC6
>>> -	bool "Axis ARTPEC-6 PCIe controller"
>>> -	depends on PCI
>>> -	depends on MACH_ARTPEC6
>>> -	depends on PCI_MSI_IRQ_DOMAIN
>>> -	select PCIEPORTBUS
>>> -	select PCIE_DW_HOST
>>> -	help
>>> -	  Say Y here to enable PCIe controller support on Axis ARTPEC-6
>>> -	  SoCs.  This PCIe controller uses the DesignWare core.
>>> -
>>>  config PCIE_KIRIN
>>>  	depends on OF && ARM64
>>>  	bool "HiSilicon Kirin series SoCs PCIe controllers"
>>
>>> diff --git a/drivers/pci/dwc/Makefile b/drivers/pci/dwc/Makefile
>>> index c61be9738cce..ac98242b83a9 100644
>>> --- a/drivers/pci/dwc/Makefile
>>> +++ b/drivers/pci/dwc/Makefile
>>> @@ -2,6 +2,9 @@ obj-$(CONFIG_PCIE_DW) += pcie-designware.o
>>>  obj-$(CONFIG_PCIE_DW_HOST) += pcie-designware-host.o
>>>  obj-$(CONFIG_PCIE_DW_EP) += pcie-designware-ep.o
>>>  obj-$(CONFIG_PCIE_DW_PLAT) += pcie-designware-plat.o
>>> +ifneq ($(filter y,$(CONFIG_PCIE_ARTPEC6_HOST) $(CONFIG_PCIE_ARTPEC6_EP)),)
>>> +        obj-$(CONFIG_PCIE_ARTPEC6) += pcie-artpec6.o
>>> +endif
>>
>> I see you're copying the DRA7XX style here, but I don't really
>> understand it.  I guess the idea is to build pcie-artpec6.o if either
>> CONFIG_PCIE_ARTPEC6_HOST or CONFIG_PCIE_ARTPEC6_EP is set (or both).
>>
>> Is this really the simplest way to express that in Kconfig?  Both the
>> "if PCIE_ARTPEC6" and this ifneq thing are complicated and relatively
>> unusual.
>>
>>>  ifneq ($(filter y,$(CONFIG_PCI_DRA7XX_HOST) $(CONFIG_PCI_DRA7XX_EP)),)
>>>          obj-$(CONFIG_PCI_DRA7XX) += pci-dra7xx.o
>>>  endif
>>
>>> +static void dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar)
>>
>> Looks funny to have this dw_*() function in pcie-artpec6.c.  Intended?
>> Or is this something other endpoint drivers could/should share?
> 
> yeah, dw_pcie_ep_reset_bar is already present in pcie-designware-ep.c which
> should be made as library function to be used by other drivers. (pci-dra7xx.c
> has also added something like above which should also be fixed).
> 

Hello Kishon,

I can remove the static keyword from dw_pcie_ep_reset_bar,
move the declaration to pcie-designware.h,
and make sure that both pcie-artpec6.c and pci-dra7xx.c uses it.

Will do this in the next version of the patch series if you
don't have any objection.


Regards,
Niklas

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH 08/10] PCI: dwc: artpec6: add support for endpoint mode
  2017-10-18  8:15         ` Niklas Cassel
@ 2017-10-18  8:47           ` Kishon Vijay Abraham I
  -1 siblings, 0 replies; 41+ messages in thread
From: Kishon Vijay Abraham I @ 2017-10-18  8:47 UTC (permalink / raw)
  To: Niklas Cassel, Bjorn Helgaas
  Cc: Jesper Nilsson, Bjorn Helgaas, Rob Herring, Mark Rutland,
	Jingoo Han, Xiaowei Song, Peter Robinson, Tomasz Nowicki,
	Gabriele Paoloni, Duc Dang, linux-arm-kernel, linux-pci,
	devicetree, linux-kernel

Niklas,

On Wednesday 18 October 2017 01:45 PM, Niklas Cassel wrote:
> On 10/18/2017 10:03 AM, Kishon Vijay Abraham I wrote:
>> Hi Bjorn,
>>
>> On Tuesday 17 October 2017 05:13 AM, Bjorn Helgaas wrote:
>>> On Fri, Oct 13, 2017 at 06:09:11PM +0200, Niklas Cassel wrote:
>>>> Signed-off-by: Niklas Cassel <niklas.cassel@axis.com>
>>>> ---
>>>>  .../devicetree/bindings/pci/axis,artpec6-pcie.txt  |   3 +-
>>>>  drivers/pci/dwc/Kconfig                            |  41 +++--
>>>>  drivers/pci/dwc/Makefile                           |   4 +-
>>>>  drivers/pci/dwc/pcie-artpec6.c                     | 202 ++++++++++++++++++++-
>>>>  4 files changed, 233 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/pci/axis,artpec6-pcie.txt b/Documentation/devicetree/bindings/pci/axis,artpec6-pcie.txt
>>>> index 4e4aee4439ea..33eef7ae5a23 100644
>>>> --- a/Documentation/devicetree/bindings/pci/axis,artpec6-pcie.txt
>>>> +++ b/Documentation/devicetree/bindings/pci/axis,artpec6-pcie.txt
>>>> @@ -4,7 +4,8 @@ This PCIe host controller is based on the Synopsys DesignWare PCIe IP
>>>>  and thus inherits all the common properties defined in designware-pcie.txt.
>>>>  
>>>>  Required properties:
>>>> -- compatible: "axis,artpec6-pcie", "snps,dw-pcie"
>>>> +- compatible: "axis,artpec6-pcie", "snps,dw-pcie" for ARTPEC-6 in RC mode;
>>>> +	      "axis,artpec6-pcie-ep", "snps,dw-pcie" for ARTPEC-6 in EP mode;
>>>>  - reg: base addresses and lengths of the PCIe controller (DBI),
>>>>  	the PHY controller, and configuration address space.
>>>>  - reg-names: Must include the following entries:
>>>> diff --git a/drivers/pci/dwc/Kconfig b/drivers/pci/dwc/Kconfig
>>>> index 22ec82fcdea2..e333283fb1ed 100644
>>>> --- a/drivers/pci/dwc/Kconfig
>>>> +++ b/drivers/pci/dwc/Kconfig
>>>> @@ -14,6 +14,36 @@ config PCIE_DW_EP
>>>>  	depends on PCI_ENDPOINT
>>>>  	select PCIE_DW
>>>>  
>>>> +config PCIE_ARTPEC6
>>>> +	bool "Axis ARTPEC-6 PCIe controller"
>>>> +	depends on (PCI && PCI_MSI_IRQ_DOMAIN) || PCI_ENDPOINT
>>>> +	depends on MACH_ARTPEC6
>>>> +	help
>>>> +	  Say Y here to enable PCIe controller support on Axis ARTPEC-6
>>>> +	  SoCs.  This PCIe controller uses the DesignWare core.
>>>> +
>>>> +if PCIE_ARTPEC6
>>>> +
>>>> +config PCIE_ARTPEC6_HOST
>>>> +	bool "Axis ARTPEC-6 Host Mode"
>>>> +	depends on PCI
>>>> +	depends on PCI_MSI_IRQ_DOMAIN
>>>> +	select PCIEPORTBUS
>>>> +	select PCIE_DW_HOST
>>>> +	help
>>>> +	  Enables support for the PCIe controller in the ARTPEC-6 SoC to work in
>>>> +	  host mode.
>>>> +
>>>> +config PCIE_ARTPEC6_EP
>>>> +	bool "Axis ARTPEC-6 Endpoint Mode"
>>>> +	depends on PCI_ENDPOINT
>>>> +	select PCIE_DW_EP
>>>> +	help
>>>> +	  Enables support for the PCIe controller in the ARTPEC-6 SoC to work in
>>>> +	  endpoint mode.
>>>> +
>>>> +endif
>>>> +
>>>>  config PCI_DRA7XX
>>>>  	bool "TI DRA7xx PCIe controller"
>>>>  	depends on SOC_DRA7XX || COMPILE_TEST
>>>> @@ -148,17 +178,6 @@ config PCIE_ARMADA_8K
>>>>  	  DesignWare hardware and therefore the driver re-uses the
>>>>  	  DesignWare core functions to implement the driver.
>>>>  
>>>> -config PCIE_ARTPEC6
>>>> -	bool "Axis ARTPEC-6 PCIe controller"
>>>> -	depends on PCI
>>>> -	depends on MACH_ARTPEC6
>>>> -	depends on PCI_MSI_IRQ_DOMAIN
>>>> -	select PCIEPORTBUS
>>>> -	select PCIE_DW_HOST
>>>> -	help
>>>> -	  Say Y here to enable PCIe controller support on Axis ARTPEC-6
>>>> -	  SoCs.  This PCIe controller uses the DesignWare core.
>>>> -
>>>>  config PCIE_KIRIN
>>>>  	depends on OF && ARM64
>>>>  	bool "HiSilicon Kirin series SoCs PCIe controllers"
>>>
>>>> diff --git a/drivers/pci/dwc/Makefile b/drivers/pci/dwc/Makefile
>>>> index c61be9738cce..ac98242b83a9 100644
>>>> --- a/drivers/pci/dwc/Makefile
>>>> +++ b/drivers/pci/dwc/Makefile
>>>> @@ -2,6 +2,9 @@ obj-$(CONFIG_PCIE_DW) += pcie-designware.o
>>>>  obj-$(CONFIG_PCIE_DW_HOST) += pcie-designware-host.o
>>>>  obj-$(CONFIG_PCIE_DW_EP) += pcie-designware-ep.o
>>>>  obj-$(CONFIG_PCIE_DW_PLAT) += pcie-designware-plat.o
>>>> +ifneq ($(filter y,$(CONFIG_PCIE_ARTPEC6_HOST) $(CONFIG_PCIE_ARTPEC6_EP)),)
>>>> +        obj-$(CONFIG_PCIE_ARTPEC6) += pcie-artpec6.o
>>>> +endif
>>>
>>> I see you're copying the DRA7XX style here, but I don't really
>>> understand it.  I guess the idea is to build pcie-artpec6.o if either
>>> CONFIG_PCIE_ARTPEC6_HOST or CONFIG_PCIE_ARTPEC6_EP is set (or both).
>>>
>>> Is this really the simplest way to express that in Kconfig?  Both the
>>> "if PCIE_ARTPEC6" and this ifneq thing are complicated and relatively
>>> unusual.
>>>
>>>>  ifneq ($(filter y,$(CONFIG_PCI_DRA7XX_HOST) $(CONFIG_PCI_DRA7XX_EP)),)
>>>>          obj-$(CONFIG_PCI_DRA7XX) += pci-dra7xx.o
>>>>  endif
>>>
>>>> +static void dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar)
>>>
>>> Looks funny to have this dw_*() function in pcie-artpec6.c.  Intended?
>>> Or is this something other endpoint drivers could/should share?
>>
>> yeah, dw_pcie_ep_reset_bar is already present in pcie-designware-ep.c which
>> should be made as library function to be used by other drivers. (pci-dra7xx.c
>> has also added something like above which should also be fixed).
>>
> 
> Hello Kishon,
> 
> I can remove the static keyword from dw_pcie_ep_reset_bar,
> move the declaration to pcie-designware.h,
> and make sure that both pcie-artpec6.c and pci-dra7xx.c uses it.
> 
> Will do this in the next version of the patch series if you
> don't have any objection.

sure, thanks for doing it :-)

-Kishon

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH 08/10] PCI: dwc: artpec6: add support for endpoint mode
@ 2017-10-18  8:47           ` Kishon Vijay Abraham I
  0 siblings, 0 replies; 41+ messages in thread
From: Kishon Vijay Abraham I @ 2017-10-18  8:47 UTC (permalink / raw)
  To: Niklas Cassel, Bjorn Helgaas
  Cc: Jesper Nilsson, Bjorn Helgaas, Rob Herring, Mark Rutland,
	Jingoo Han, Xiaowei Song, Peter Robinson, Tomasz Nowicki,
	Gabriele Paoloni, Duc Dang, linux-arm-kernel, linux-pci,
	devicetree, linux-kernel

Niklas,

On Wednesday 18 October 2017 01:45 PM, Niklas Cassel wrote:
> On 10/18/2017 10:03 AM, Kishon Vijay Abraham I wrote:
>> Hi Bjorn,
>>
>> On Tuesday 17 October 2017 05:13 AM, Bjorn Helgaas wrote:
>>> On Fri, Oct 13, 2017 at 06:09:11PM +0200, Niklas Cassel wrote:
>>>> Signed-off-by: Niklas Cassel <niklas.cassel@axis.com>
>>>> ---
>>>>  .../devicetree/bindings/pci/axis,artpec6-pcie.txt  |   3 +-
>>>>  drivers/pci/dwc/Kconfig                            |  41 +++--
>>>>  drivers/pci/dwc/Makefile                           |   4 +-
>>>>  drivers/pci/dwc/pcie-artpec6.c                     | 202 ++++++++++++++++++++-
>>>>  4 files changed, 233 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/pci/axis,artpec6-pcie.txt b/Documentation/devicetree/bindings/pci/axis,artpec6-pcie.txt
>>>> index 4e4aee4439ea..33eef7ae5a23 100644
>>>> --- a/Documentation/devicetree/bindings/pci/axis,artpec6-pcie.txt
>>>> +++ b/Documentation/devicetree/bindings/pci/axis,artpec6-pcie.txt
>>>> @@ -4,7 +4,8 @@ This PCIe host controller is based on the Synopsys DesignWare PCIe IP
>>>>  and thus inherits all the common properties defined in designware-pcie.txt.
>>>>  
>>>>  Required properties:
>>>> -- compatible: "axis,artpec6-pcie", "snps,dw-pcie"
>>>> +- compatible: "axis,artpec6-pcie", "snps,dw-pcie" for ARTPEC-6 in RC mode;
>>>> +	      "axis,artpec6-pcie-ep", "snps,dw-pcie" for ARTPEC-6 in EP mode;
>>>>  - reg: base addresses and lengths of the PCIe controller (DBI),
>>>>  	the PHY controller, and configuration address space.
>>>>  - reg-names: Must include the following entries:
>>>> diff --git a/drivers/pci/dwc/Kconfig b/drivers/pci/dwc/Kconfig
>>>> index 22ec82fcdea2..e333283fb1ed 100644
>>>> --- a/drivers/pci/dwc/Kconfig
>>>> +++ b/drivers/pci/dwc/Kconfig
>>>> @@ -14,6 +14,36 @@ config PCIE_DW_EP
>>>>  	depends on PCI_ENDPOINT
>>>>  	select PCIE_DW
>>>>  
>>>> +config PCIE_ARTPEC6
>>>> +	bool "Axis ARTPEC-6 PCIe controller"
>>>> +	depends on (PCI && PCI_MSI_IRQ_DOMAIN) || PCI_ENDPOINT
>>>> +	depends on MACH_ARTPEC6
>>>> +	help
>>>> +	  Say Y here to enable PCIe controller support on Axis ARTPEC-6
>>>> +	  SoCs.  This PCIe controller uses the DesignWare core.
>>>> +
>>>> +if PCIE_ARTPEC6
>>>> +
>>>> +config PCIE_ARTPEC6_HOST
>>>> +	bool "Axis ARTPEC-6 Host Mode"
>>>> +	depends on PCI
>>>> +	depends on PCI_MSI_IRQ_DOMAIN
>>>> +	select PCIEPORTBUS
>>>> +	select PCIE_DW_HOST
>>>> +	help
>>>> +	  Enables support for the PCIe controller in the ARTPEC-6 SoC to work in
>>>> +	  host mode.
>>>> +
>>>> +config PCIE_ARTPEC6_EP
>>>> +	bool "Axis ARTPEC-6 Endpoint Mode"
>>>> +	depends on PCI_ENDPOINT
>>>> +	select PCIE_DW_EP
>>>> +	help
>>>> +	  Enables support for the PCIe controller in the ARTPEC-6 SoC to work in
>>>> +	  endpoint mode.
>>>> +
>>>> +endif
>>>> +
>>>>  config PCI_DRA7XX
>>>>  	bool "TI DRA7xx PCIe controller"
>>>>  	depends on SOC_DRA7XX || COMPILE_TEST
>>>> @@ -148,17 +178,6 @@ config PCIE_ARMADA_8K
>>>>  	  DesignWare hardware and therefore the driver re-uses the
>>>>  	  DesignWare core functions to implement the driver.
>>>>  
>>>> -config PCIE_ARTPEC6
>>>> -	bool "Axis ARTPEC-6 PCIe controller"
>>>> -	depends on PCI
>>>> -	depends on MACH_ARTPEC6
>>>> -	depends on PCI_MSI_IRQ_DOMAIN
>>>> -	select PCIEPORTBUS
>>>> -	select PCIE_DW_HOST
>>>> -	help
>>>> -	  Say Y here to enable PCIe controller support on Axis ARTPEC-6
>>>> -	  SoCs.  This PCIe controller uses the DesignWare core.
>>>> -
>>>>  config PCIE_KIRIN
>>>>  	depends on OF && ARM64
>>>>  	bool "HiSilicon Kirin series SoCs PCIe controllers"
>>>
>>>> diff --git a/drivers/pci/dwc/Makefile b/drivers/pci/dwc/Makefile
>>>> index c61be9738cce..ac98242b83a9 100644
>>>> --- a/drivers/pci/dwc/Makefile
>>>> +++ b/drivers/pci/dwc/Makefile
>>>> @@ -2,6 +2,9 @@ obj-$(CONFIG_PCIE_DW) += pcie-designware.o
>>>>  obj-$(CONFIG_PCIE_DW_HOST) += pcie-designware-host.o
>>>>  obj-$(CONFIG_PCIE_DW_EP) += pcie-designware-ep.o
>>>>  obj-$(CONFIG_PCIE_DW_PLAT) += pcie-designware-plat.o
>>>> +ifneq ($(filter y,$(CONFIG_PCIE_ARTPEC6_HOST) $(CONFIG_PCIE_ARTPEC6_EP)),)
>>>> +        obj-$(CONFIG_PCIE_ARTPEC6) += pcie-artpec6.o
>>>> +endif
>>>
>>> I see you're copying the DRA7XX style here, but I don't really
>>> understand it.  I guess the idea is to build pcie-artpec6.o if either
>>> CONFIG_PCIE_ARTPEC6_HOST or CONFIG_PCIE_ARTPEC6_EP is set (or both).
>>>
>>> Is this really the simplest way to express that in Kconfig?  Both the
>>> "if PCIE_ARTPEC6" and this ifneq thing are complicated and relatively
>>> unusual.
>>>
>>>>  ifneq ($(filter y,$(CONFIG_PCI_DRA7XX_HOST) $(CONFIG_PCI_DRA7XX_EP)),)
>>>>          obj-$(CONFIG_PCI_DRA7XX) += pci-dra7xx.o
>>>>  endif
>>>
>>>> +static void dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar)
>>>
>>> Looks funny to have this dw_*() function in pcie-artpec6.c.  Intended?
>>> Or is this something other endpoint drivers could/should share?
>>
>> yeah, dw_pcie_ep_reset_bar is already present in pcie-designware-ep.c which
>> should be made as library function to be used by other drivers. (pci-dra7xx.c
>> has also added something like above which should also be fixed).
>>
> 
> Hello Kishon,
> 
> I can remove the static keyword from dw_pcie_ep_reset_bar,
> move the declaration to pcie-designware.h,
> and make sure that both pcie-artpec6.c and pci-dra7xx.c uses it.
> 
> Will do this in the next version of the patch series if you
> don't have any objection.

sure, thanks for doing it :-)

-Kishon

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH 08/10] PCI: dwc: artpec6: add support for endpoint mode
@ 2017-10-19  7:59       ` Christoph Hellwig
  0 siblings, 0 replies; 41+ messages in thread
From: Christoph Hellwig @ 2017-10-19  7:59 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Niklas Cassel, Niklas Cassel, Jesper Nilsson, Bjorn Helgaas,
	Rob Herring, Mark Rutland, Kishon Vijay Abraham I, Jingoo Han,
	Xiaowei Song, Peter Robinson, Tomasz Nowicki, Gabriele Paoloni,
	Duc Dang, linux-arm-kernel, linux-pci, devicetree, linux-kernel

On Mon, Oct 16, 2017 at 06:43:26PM -0500, Bjorn Helgaas wrote:
> understand it.  I guess the idea is to build pcie-artpec6.o if either
> CONFIG_PCIE_ARTPEC6_HOST or CONFIG_PCIE_ARTPEC6_EP is set (or both).
> 
> Is this really the simplest way to express that in Kconfig?  Both the
> "if PCIE_ARTPEC6" and this ifneq thing are complicated and relatively
> unusual.

I thĳnk the right style is to make PCIE_ARTPEC6 a hidden kconfig
symbol without help text, and then have PCIE_ARTPEC6_HOST and
PCIE_ARTPEC6_EP select it.

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH 08/10] PCI: dwc: artpec6: add support for endpoint mode
@ 2017-10-19  7:59       ` Christoph Hellwig
  0 siblings, 0 replies; 41+ messages in thread
From: Christoph Hellwig @ 2017-10-19  7:59 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Niklas Cassel, Niklas Cassel, Jesper Nilsson, Bjorn Helgaas,
	Rob Herring, Mark Rutland, Kishon Vijay Abraham I, Jingoo Han,
	Xiaowei Song, Peter Robinson, Tomasz Nowicki, Gabriele Paoloni,
	Duc Dang, linux-arm-kernel-VrBV9hrLPhE,
	linux-pci-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA

On Mon, Oct 16, 2017 at 06:43:26PM -0500, Bjorn Helgaas wrote:
> understand it.  I guess the idea is to build pcie-artpec6.o if either
> CONFIG_PCIE_ARTPEC6_HOST or CONFIG_PCIE_ARTPEC6_EP is set (or both).
> 
> Is this really the simplest way to express that in Kconfig?  Both the
> "if PCIE_ARTPEC6" and this ifneq thing are complicated and relatively
> unusual.

I thĳnk the right style is to make PCIE_ARTPEC6 a hidden kconfig
symbol without help text, and then have PCIE_ARTPEC6_HOST and
PCIE_ARTPEC6_EP select it.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH 08/10] PCI: dwc: artpec6: add support for endpoint mode
  2017-10-19  7:59       ` Christoph Hellwig
@ 2017-10-19 10:57         ` Niklas Cassel
  -1 siblings, 0 replies; 41+ messages in thread
From: Niklas Cassel @ 2017-10-19 10:57 UTC (permalink / raw)
  To: Christoph Hellwig, Bjorn Helgaas
  Cc: Jesper Nilsson, Bjorn Helgaas, Rob Herring, Mark Rutland,
	Kishon Vijay Abraham I, Jingoo Han, Xiaowei Song, Peter Robinson,
	Tomasz Nowicki, Gabriele Paoloni, Duc Dang, linux-arm-kernel,
	linux-pci, devicetree, linux-kernel

On 10/19/2017 09:59 AM, Christoph Hellwig wrote:
> On Mon, Oct 16, 2017 at 06:43:26PM -0500, Bjorn Helgaas wrote:
>> understand it.  I guess the idea is to build pcie-artpec6.o if either
>> CONFIG_PCIE_ARTPEC6_HOST or CONFIG_PCIE_ARTPEC6_EP is set (or both).
>>
>> Is this really the simplest way to express that in Kconfig?  Both the
>> "if PCIE_ARTPEC6" and this ifneq thing are complicated and relatively
>> unusual.
> 
> I thĳnk the right style is to make PCIE_ARTPEC6 a hidden kconfig
> symbol without help text, and then have PCIE_ARTPEC6_HOST and
> PCIE_ARTPEC6_EP select it.
> 

Thanks for the suggestion Christoph!
I will make this change for both pci-dra7xx and pcie-artpec6 in
the next version of the patch series, and then Kishon can ACK/NACK.

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH 08/10] PCI: dwc: artpec6: add support for endpoint mode
@ 2017-10-19 10:57         ` Niklas Cassel
  0 siblings, 0 replies; 41+ messages in thread
From: Niklas Cassel @ 2017-10-19 10:57 UTC (permalink / raw)
  To: Christoph Hellwig, Bjorn Helgaas
  Cc: Jesper Nilsson, Bjorn Helgaas, Rob Herring, Mark Rutland,
	Kishon Vijay Abraham I, Jingoo Han, Xiaowei Song, Peter Robinson,
	Tomasz Nowicki, Gabriele Paoloni, Duc Dang, linux-arm-kernel,
	linux-pci, devicetree, linux-kernel

On 10/19/2017 09:59 AM, Christoph Hellwig wrote:
> On Mon, Oct 16, 2017 at 06:43:26PM -0500, Bjorn Helgaas wrote:
>> understand it.  I guess the idea is to build pcie-artpec6.o if either
>> CONFIG_PCIE_ARTPEC6_HOST or CONFIG_PCIE_ARTPEC6_EP is set (or both).
>>
>> Is this really the simplest way to express that in Kconfig?  Both the
>> "if PCIE_ARTPEC6" and this ifneq thing are complicated and relatively
>> unusual.
> 
> I thĳnk the right style is to make PCIE_ARTPEC6 a hidden kconfig
> symbol without help text, and then have PCIE_ARTPEC6_HOST and
> PCIE_ARTPEC6_EP select it.
> 

Thanks for the suggestion Christoph!
I will make this change for both pci-dra7xx and pcie-artpec6 in
the next version of the patch series, and then Kishon can ACK/NACK.

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH 08/10] PCI: dwc: artpec6: add support for endpoint mode
  2017-10-16 23:43   ` Bjorn Helgaas
@ 2017-10-19 11:40       ` Niklas Cassel
  2017-10-19  7:59       ` Christoph Hellwig
  2017-10-19 11:40       ` Niklas Cassel
  2 siblings, 0 replies; 41+ messages in thread
From: Niklas Cassel @ 2017-10-19 11:40 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Jesper Nilsson, Bjorn Helgaas, Rob Herring, Mark Rutland,
	Kishon Vijay Abraham I, Jingoo Han, Xiaowei Song, Peter Robinson,
	Tomasz Nowicki, Gabriele Paoloni, Duc Dang, linux-arm-kernel,
	linux-pci, devicetree, linux-kernel

Hello Bjorn

On 10/17/2017 01:43 AM, Bjorn Helgaas wrote:
(snip)
> I see you're copying the DRA7XX style here, but I don't really
> understand it.  I guess the idea is to build pcie-artpec6.o if either
> CONFIG_PCIE_ARTPEC6_HOST or CONFIG_PCIE_ARTPEC6_EP is set (or both).
> 
> Is this really the simplest way to express that in Kconfig?  Both the
> "if PCIE_ARTPEC6" and this ifneq thing are complicated and relatively
> unusual.

I will try Christoph's suggestion.

> 
>>  ifneq ($(filter y,$(CONFIG_PCI_DRA7XX_HOST) $(CONFIG_PCI_DRA7XX_EP)),)
>>          obj-$(CONFIG_PCI_DRA7XX) += pci-dra7xx.o
>>  endif
> 
>> +static void dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar)
> 
> Looks funny to have this dw_*() function in pcie-artpec6.c.  Intended?
> Or is this something other endpoint drivers could/should share?

As discussed with Kishon, I make sure that the dw_pcie_ep_reset_bar
in pcie-designware-ep.c is no longer static.

> 
>> +{
>> +	u32 reg;
>> +
>> +	reg = PCI_BASE_ADDRESS_0 + (4 * bar);
>> +	dw_pcie_dbi_ro_wr_en(pci);
>> +	dw_pcie_writel_dbi2(pci, reg, 0x0);
>> +	dw_pcie_writel_dbi(pci, reg, 0x0);
>> +	dw_pcie_dbi_ro_wr_dis(pci);
>> +}
>> +
>> +static void artpec6_pcie_ep_init(struct dw_pcie_ep *ep)
>> +{
>> +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>> +	struct artpec6_pcie *artpec6_pcie = to_artpec6_pcie(pci);
>> +	enum pci_barno bar;
>> +
>> +	artpec6_pcie_assert_core_reset(artpec6_pcie);
>> +	artpec6_pcie_init_phy(artpec6_pcie);
>> +	artpec6_pcie_deassert_core_reset(artpec6_pcie);
>> +
>> +	for (bar = BAR_0; bar <= BAR_5; bar++)
>> +		dw_pcie_ep_reset_bar(pci, bar);
>> +}
> 
> I naively expected some of this new code to be #ifdef PCIE_ARTPEC6_EP.
> But I haven't really internalized the endpoint support, so maybe that
> doesn't make sense?

I copied the style from dra7xx.
But after thinking about it, I agree with you.

If we move dw_pcie_ep_reset_bar and artpec6_pcie_raise_msi_irq
to pcie-designware-ep, the functions that are left are:

artpec6_pcie_stop_link
artpec6_pcie_ep_init
artpec6_pcie_raise_irq
artpec6_add_pcie_ep
the compatible in the of match table

The only function I'm hesitant about is .stop_link,
since it is in dw_pcie_ops, however, right now it
is only used in pcie-designware-ep.c

Would you prefer me to make them #ifdef PCIE_ARTPEC6_EP ?

I can do a similar patch for pci-dra7xx.


Regards,
Niklas

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH 08/10] PCI: dwc: artpec6: add support for endpoint mode
@ 2017-10-19 11:40       ` Niklas Cassel
  0 siblings, 0 replies; 41+ messages in thread
From: Niklas Cassel @ 2017-10-19 11:40 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Jesper Nilsson, Bjorn Helgaas, Rob Herring, Mark Rutland,
	Kishon Vijay Abraham I, Jingoo Han, Xiaowei Song, Peter Robinson,
	Tomasz Nowicki, Gabriele Paoloni, Duc Dang, linux-arm-kernel,
	linux-pci, devicetree, linux-kernel

Hello Bjorn

On 10/17/2017 01:43 AM, Bjorn Helgaas wrote:
(snip)
> I see you're copying the DRA7XX style here, but I don't really
> understand it.  I guess the idea is to build pcie-artpec6.o if either
> CONFIG_PCIE_ARTPEC6_HOST or CONFIG_PCIE_ARTPEC6_EP is set (or both).
> 
> Is this really the simplest way to express that in Kconfig?  Both the
> "if PCIE_ARTPEC6" and this ifneq thing are complicated and relatively
> unusual.

I will try Christoph's suggestion.

> 
>>  ifneq ($(filter y,$(CONFIG_PCI_DRA7XX_HOST) $(CONFIG_PCI_DRA7XX_EP)),)
>>          obj-$(CONFIG_PCI_DRA7XX) += pci-dra7xx.o
>>  endif
> 
>> +static void dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar)
> 
> Looks funny to have this dw_*() function in pcie-artpec6.c.  Intended?
> Or is this something other endpoint drivers could/should share?

As discussed with Kishon, I make sure that the dw_pcie_ep_reset_bar
in pcie-designware-ep.c is no longer static.

> 
>> +{
>> +	u32 reg;
>> +
>> +	reg = PCI_BASE_ADDRESS_0 + (4 * bar);
>> +	dw_pcie_dbi_ro_wr_en(pci);
>> +	dw_pcie_writel_dbi2(pci, reg, 0x0);
>> +	dw_pcie_writel_dbi(pci, reg, 0x0);
>> +	dw_pcie_dbi_ro_wr_dis(pci);
>> +}
>> +
>> +static void artpec6_pcie_ep_init(struct dw_pcie_ep *ep)
>> +{
>> +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>> +	struct artpec6_pcie *artpec6_pcie = to_artpec6_pcie(pci);
>> +	enum pci_barno bar;
>> +
>> +	artpec6_pcie_assert_core_reset(artpec6_pcie);
>> +	artpec6_pcie_init_phy(artpec6_pcie);
>> +	artpec6_pcie_deassert_core_reset(artpec6_pcie);
>> +
>> +	for (bar = BAR_0; bar <= BAR_5; bar++)
>> +		dw_pcie_ep_reset_bar(pci, bar);
>> +}
> 
> I naively expected some of this new code to be #ifdef PCIE_ARTPEC6_EP.
> But I haven't really internalized the endpoint support, so maybe that
> doesn't make sense?

I copied the style from dra7xx.
But after thinking about it, I agree with you.

If we move dw_pcie_ep_reset_bar and artpec6_pcie_raise_msi_irq
to pcie-designware-ep, the functions that are left are:

artpec6_pcie_stop_link
artpec6_pcie_ep_init
artpec6_pcie_raise_irq
artpec6_add_pcie_ep
the compatible in the of match table

The only function I'm hesitant about is .stop_link,
since it is in dw_pcie_ops, however, right now it
is only used in pcie-designware-ep.c

Would you prefer me to make them #ifdef PCIE_ARTPEC6_EP ?

I can do a similar patch for pci-dra7xx.


Regards,
Niklas

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH 08/10] PCI: dwc: artpec6: add support for endpoint mode
  2017-10-13 16:09 ` [PATCH 08/10] PCI: dwc: artpec6: add support for endpoint mode Niklas Cassel
  2017-10-16 23:43   ` Bjorn Helgaas
@ 2017-10-17 22:24   ` Rob Herring
  2017-10-18  8:46     ` Kishon Vijay Abraham I
  2 siblings, 0 replies; 41+ messages in thread
From: Rob Herring @ 2017-10-17 22:24 UTC (permalink / raw)
  To: Niklas Cassel
  Cc: Niklas Cassel, Jesper Nilsson, Bjorn Helgaas, Mark Rutland,
	Kishon Vijay Abraham I, Jingoo Han, Xiaowei Song, Peter Robinson,
	Tomasz Nowicki, Gabriele Paoloni, Duc Dang, linux-arm-kernel,
	linux-pci, devicetree, linux-kernel

On Fri, Oct 13, 2017 at 06:09:11PM +0200, Niklas Cassel wrote:
> Signed-off-by: Niklas Cassel <niklas.cassel@axis.com>
> ---
>  .../devicetree/bindings/pci/axis,artpec6-pcie.txt  |   3 +-

It is preferred to split binding patches. Regardless,

Acked-by: Rob Herring <robh@kernel.org>

>  drivers/pci/dwc/Kconfig                            |  41 +++--
>  drivers/pci/dwc/Makefile                           |   4 +-
>  drivers/pci/dwc/pcie-artpec6.c                     | 202 ++++++++++++++++++++-
>  4 files changed, 233 insertions(+), 17 deletions(-)

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH 08/10] PCI: dwc: artpec6: add support for endpoint mode
@ 2017-10-18  8:46     ` Kishon Vijay Abraham I
  0 siblings, 0 replies; 41+ messages in thread
From: Kishon Vijay Abraham I @ 2017-10-18  8:46 UTC (permalink / raw)
  To: Niklas Cassel, Niklas Cassel, Jesper Nilsson, Bjorn Helgaas,
	Rob Herring, Mark Rutland, Jingoo Han, Xiaowei Song,
	Peter Robinson, Tomasz Nowicki, Gabriele Paoloni, Duc Dang
  Cc: linux-arm-kernel, linux-pci, devicetree, linux-kernel

Hi Niklas,

On Friday 13 October 2017 09:39 PM, Niklas Cassel wrote:
> Signed-off-by: Niklas Cassel <niklas.cassel@axis.com>
> ---
>  .../devicetree/bindings/pci/axis,artpec6-pcie.txt  |   3 +-
>  drivers/pci/dwc/Kconfig                            |  41 +++--
>  drivers/pci/dwc/Makefile                           |   4 +-
>  drivers/pci/dwc/pcie-artpec6.c                     | 202 ++++++++++++++++++++-
>  4 files changed, 233 insertions(+), 17 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pci/axis,artpec6-pcie.txt b/Documentation/devicetree/bindings/pci/axis,artpec6-pcie.txt
> index 4e4aee4439ea..33eef7ae5a23 100644
> --- a/Documentation/devicetree/bindings/pci/axis,artpec6-pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/axis,artpec6-pcie.txt
> @@ -4,7 +4,8 @@ This PCIe host controller is based on the Synopsys DesignWare PCIe IP
>  and thus inherits all the common properties defined in designware-pcie.txt.
>  
>  Required properties:
> -- compatible: "axis,artpec6-pcie", "snps,dw-pcie"
> +- compatible: "axis,artpec6-pcie", "snps,dw-pcie" for ARTPEC-6 in RC mode;
> +	      "axis,artpec6-pcie-ep", "snps,dw-pcie" for ARTPEC-6 in EP mode;

If "snps,dw-pcie" is used for both RC and EP mode, how do we differentiate
between the modes?
>  - reg: base addresses and lengths of the PCIe controller (DBI),
>  	the PHY controller, and configuration address space.
>  - reg-names: Must include the following entries:
> diff --git a/drivers/pci/dwc/Kconfig b/drivers/pci/dwc/Kconfig
> index 22ec82fcdea2..e333283fb1ed 100644
> --- a/drivers/pci/dwc/Kconfig
> +++ b/drivers/pci/dwc/Kconfig
> @@ -14,6 +14,36 @@ config PCIE_DW_EP
>  	depends on PCI_ENDPOINT
>  	select PCIE_DW
>  
> +config PCIE_ARTPEC6
> +	bool "Axis ARTPEC-6 PCIe controller"
> +	depends on (PCI && PCI_MSI_IRQ_DOMAIN) || PCI_ENDPOINT
> +	depends on MACH_ARTPEC6
> +	help
> +	  Say Y here to enable PCIe controller support on Axis ARTPEC-6
> +	  SoCs.  This PCIe controller uses the DesignWare core.
> +
> +if PCIE_ARTPEC6
> +
> +config PCIE_ARTPEC6_HOST
> +	bool "Axis ARTPEC-6 Host Mode"
> +	depends on PCI
> +	depends on PCI_MSI_IRQ_DOMAIN
> +	select PCIEPORTBUS
> +	select PCIE_DW_HOST
> +	help
> +	  Enables support for the PCIe controller in the ARTPEC-6 SoC to work in
> +	  host mode.
> +
> +config PCIE_ARTPEC6_EP
> +	bool "Axis ARTPEC-6 Endpoint Mode"
> +	depends on PCI_ENDPOINT
> +	select PCIE_DW_EP
> +	help
> +	  Enables support for the PCIe controller in the ARTPEC-6 SoC to work in
> +	  endpoint mode.
> +
> +endif
> +
>  config PCI_DRA7XX
>  	bool "TI DRA7xx PCIe controller"
>  	depends on SOC_DRA7XX || COMPILE_TEST
> @@ -148,17 +178,6 @@ config PCIE_ARMADA_8K
>  	  DesignWare hardware and therefore the driver re-uses the
>  	  DesignWare core functions to implement the driver.
>  
> -config PCIE_ARTPEC6
> -	bool "Axis ARTPEC-6 PCIe controller"
> -	depends on PCI
> -	depends on MACH_ARTPEC6
> -	depends on PCI_MSI_IRQ_DOMAIN
> -	select PCIEPORTBUS
> -	select PCIE_DW_HOST
> -	help
> -	  Say Y here to enable PCIe controller support on Axis ARTPEC-6
> -	  SoCs.  This PCIe controller uses the DesignWare core.
> -
>  config PCIE_KIRIN
>  	depends on OF && ARM64
>  	bool "HiSilicon Kirin series SoCs PCIe controllers"
> diff --git a/drivers/pci/dwc/Makefile b/drivers/pci/dwc/Makefile
> index c61be9738cce..ac98242b83a9 100644
> --- a/drivers/pci/dwc/Makefile
> +++ b/drivers/pci/dwc/Makefile
> @@ -2,6 +2,9 @@ obj-$(CONFIG_PCIE_DW) += pcie-designware.o
>  obj-$(CONFIG_PCIE_DW_HOST) += pcie-designware-host.o
>  obj-$(CONFIG_PCIE_DW_EP) += pcie-designware-ep.o
>  obj-$(CONFIG_PCIE_DW_PLAT) += pcie-designware-plat.o
> +ifneq ($(filter y,$(CONFIG_PCIE_ARTPEC6_HOST) $(CONFIG_PCIE_ARTPEC6_EP)),)
> +        obj-$(CONFIG_PCIE_ARTPEC6) += pcie-artpec6.o
> +endif
>  ifneq ($(filter y,$(CONFIG_PCI_DRA7XX_HOST) $(CONFIG_PCI_DRA7XX_EP)),)
>          obj-$(CONFIG_PCI_DRA7XX) += pci-dra7xx.o
>  endif
> @@ -12,7 +15,6 @@ obj-$(CONFIG_PCI_KEYSTONE) += pci-keystone-dw.o pci-keystone.o
>  obj-$(CONFIG_PCI_LAYERSCAPE) += pci-layerscape.o
>  obj-$(CONFIG_PCIE_QCOM) += pcie-qcom.o
>  obj-$(CONFIG_PCIE_ARMADA_8K) += pcie-armada8k.o
> -obj-$(CONFIG_PCIE_ARTPEC6) += pcie-artpec6.o
>  obj-$(CONFIG_PCIE_KIRIN) += pcie-kirin.o
>  
>  # The following drivers are for devices that use the generic ACPI
> diff --git a/drivers/pci/dwc/pcie-artpec6.c b/drivers/pci/dwc/pcie-artpec6.c
> index c5d7f98dc6b2..21ea9ffef784 100644
> --- a/drivers/pci/dwc/pcie-artpec6.c
> +++ b/drivers/pci/dwc/pcie-artpec6.c
> @@ -13,6 +13,7 @@
>  #include <linux/delay.h>
>  #include <linux/kernel.h>
>  #include <linux/init.h>
> +#include <linux/of_device.h>
>  #include <linux/pci.h>
>  #include <linux/platform_device.h>
>  #include <linux/resource.h>
> @@ -30,8 +31,15 @@ struct artpec6_pcie {
>  	struct dw_pcie		*pci;
>  	struct regmap		*regmap;	/* DT axis,syscon-pcie */
>  	void __iomem		*phy_base;	/* DT phy */
> +	enum dw_pcie_device_mode mode;
>  };
>  
> +struct artpec_pcie_of_data {
> +	enum dw_pcie_device_mode mode;
> +};
> +
> +static const struct of_device_id artpec6_pcie_of_match[];
> +
>  /* PCIe Port Logic registers (memory-mapped) */
>  #define PL_OFFSET			0x700
>  #define PCIE_PHY_DEBUG_R0		(PL_OFFSET + 0x28)
> @@ -42,6 +50,7 @@ struct artpec6_pcie {
>  #define  PCIECFG_DBG_OEN		BIT(24)
>  #define  PCIECFG_CORE_RESET_REQ		BIT(21)
>  #define  PCIECFG_LTSSM_ENABLE		BIT(20)
> +#define  PCIECFG_DEVICE_TYPE_MASK	GENMASK(19, 16)
>  #define  PCIECFG_CLKREQ_B		BIT(11)
>  #define  PCIECFG_REFCLK_ENABLE		BIT(10)
>  #define  PCIECFG_PLL_ENABLE		BIT(9)
> @@ -126,6 +135,16 @@ static void artpec6_pcie_init_phy(struct artpec6_pcie *artpec6_pcie)
>  	} while (retries && !(val & PHY_COSPLLLOCK));
>  }
>  
> +static void artpec6_pcie_stop_link(struct dw_pcie *pci)
> +{
> +	struct artpec6_pcie *artpec6_pcie = to_artpec6_pcie(pci);
> +	u32 val;
> +
> +	val = artpec6_pcie_readl(artpec6_pcie, PCIECFG);
> +	val &= ~PCIECFG_LTSSM_ENABLE;
> +	artpec6_pcie_writel(artpec6_pcie, PCIECFG, val);
> +}
> +
>  static int artpec6_pcie_establish_link(struct dw_pcie *pci)
>  {
>  	struct artpec6_pcie *artpec6_pcie = to_artpec6_pcie(pci);
> @@ -195,6 +214,136 @@ static irqreturn_t artpec6_pcie_msi_handler(int irq, void *arg)
>  	return dw_handle_msi_irq(pp);
>  }
>  
> +static void dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar)
> +{
> +	u32 reg;
> +
> +	reg = PCI_BASE_ADDRESS_0 + (4 * bar);
> +	dw_pcie_dbi_ro_wr_en(pci);
> +	dw_pcie_writel_dbi2(pci, reg, 0x0);
> +	dw_pcie_writel_dbi(pci, reg, 0x0);
> +	dw_pcie_dbi_ro_wr_dis(pci);
> +}
> +
> +static void artpec6_pcie_ep_init(struct dw_pcie_ep *ep)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> +	struct artpec6_pcie *artpec6_pcie = to_artpec6_pcie(pci);
> +	enum pci_barno bar;
> +
> +	artpec6_pcie_assert_core_reset(artpec6_pcie);
> +	artpec6_pcie_init_phy(artpec6_pcie);
> +	artpec6_pcie_deassert_core_reset(artpec6_pcie);
> +
> +	for (bar = BAR_0; bar <= BAR_5; bar++)
> +		dw_pcie_ep_reset_bar(pci, bar);
> +}
> +
> +static int artpec6_pcie_raise_msi_irq(struct dw_pcie_ep *ep,
> +				      u8 interrupt_num)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> +	struct pci_epc *epc = ep->epc;
> +	u32 cap_addr, cap_value, cap_id, next_ptr, msg_ctrl, msg_data;
> +	u32 msg_addr_lower, msg_addr_upper;
> +	u64 msg_addr;
> +	bool has_upper;
> +	int ret;
> +
> +	/* Raise MSI per the PCI Local Bus Specification Revision 3.0, 6.8.1. */
> +	cap_addr = dw_pcie_readl_dbi(pci, PCI_CAPABILITY_LIST);
> +	while (1) {
> +		cap_value = dw_pcie_readl_dbi(pci, cap_addr);
> +		cap_id = cap_value & GENMASK(7, 0);
> +		if (cap_id == PCI_CAP_ID_MSI)
> +			break;
> +		next_ptr = (cap_value & GENMASK(15, 8)) >> 8;
> +		if (next_ptr == 0) {
> +			dev_err(pci->dev, "No MSI cap found!\n");
> +			return -EINVAL;
> +		}
> +		cap_addr = next_ptr;
> +	}

The MSI capability always points to fixed offset in DesingWare. So I don't
think this is necessary unless we move it to the generic endpoint layer.
> +	msg_ctrl = (cap_value & GENMASK(31, 16)) >> 16;
> +	has_upper = !!(msg_ctrl & BIT(7));
> +	msg_addr_lower = dw_pcie_readl_dbi(pci, cap_addr + 0x4);
> +	if (has_upper) {
> +		msg_addr_upper = dw_pcie_readl_dbi(pci, cap_addr + 0x8);
> +		msg_data = dw_pcie_readl_dbi(pci, cap_addr + 0xc) &
> +			GENMASK(15, 0);
> +	} else {
> +		msg_addr_upper = 0;
> +		msg_data = dw_pcie_readl_dbi(pci, cap_addr + 0x8) &
> +			GENMASK(15, 0);
> +	}
> +	msg_addr = ((u64) msg_addr_upper) << 32 | msg_addr_lower;
> +	ret = epc->ops->map_addr(epc, ep->msi_mem_phys, msg_addr, PAGE_SIZE);
> +	if (ret)
> +		return ret;
> +
> +	writel(msg_data | (interrupt_num - 1), ep->msi_mem);
> +
> +	epc->ops->unmap_addr(epc, ep->msi_mem_phys);
> +
> +	return 0;
> +}

I think this entire function can also be made as a library function to be used
by other drivers. (looks like only dra7xx has a shortcut to raise MSI irq).

Thanks
Kishon

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH 08/10] PCI: dwc: artpec6: add support for endpoint mode
@ 2017-10-18  8:46     ` Kishon Vijay Abraham I
  0 siblings, 0 replies; 41+ messages in thread
From: Kishon Vijay Abraham I @ 2017-10-18  8:46 UTC (permalink / raw)
  To: Niklas Cassel, Niklas Cassel, Jesper Nilsson, Bjorn Helgaas,
	Rob Herring, Mark Rutland, Jingoo Han, Xiaowei Song,
	Peter Robinson, Tomasz Nowicki, Gabriele Paoloni, Duc Dang
  Cc: linux-arm-kernel-VrBV9hrLPhE, linux-pci-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA

Hi Niklas,

On Friday 13 October 2017 09:39 PM, Niklas Cassel wrote:
> Signed-off-by: Niklas Cassel <niklas.cassel-VrBV9hrLPhE@public.gmane.org>
> ---
>  .../devicetree/bindings/pci/axis,artpec6-pcie.txt  |   3 +-
>  drivers/pci/dwc/Kconfig                            |  41 +++--
>  drivers/pci/dwc/Makefile                           |   4 +-
>  drivers/pci/dwc/pcie-artpec6.c                     | 202 ++++++++++++++++++++-
>  4 files changed, 233 insertions(+), 17 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pci/axis,artpec6-pcie.txt b/Documentation/devicetree/bindings/pci/axis,artpec6-pcie.txt
> index 4e4aee4439ea..33eef7ae5a23 100644
> --- a/Documentation/devicetree/bindings/pci/axis,artpec6-pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/axis,artpec6-pcie.txt
> @@ -4,7 +4,8 @@ This PCIe host controller is based on the Synopsys DesignWare PCIe IP
>  and thus inherits all the common properties defined in designware-pcie.txt.
>  
>  Required properties:
> -- compatible: "axis,artpec6-pcie", "snps,dw-pcie"
> +- compatible: "axis,artpec6-pcie", "snps,dw-pcie" for ARTPEC-6 in RC mode;
> +	      "axis,artpec6-pcie-ep", "snps,dw-pcie" for ARTPEC-6 in EP mode;

If "snps,dw-pcie" is used for both RC and EP mode, how do we differentiate
between the modes?
>  - reg: base addresses and lengths of the PCIe controller (DBI),
>  	the PHY controller, and configuration address space.
>  - reg-names: Must include the following entries:
> diff --git a/drivers/pci/dwc/Kconfig b/drivers/pci/dwc/Kconfig
> index 22ec82fcdea2..e333283fb1ed 100644
> --- a/drivers/pci/dwc/Kconfig
> +++ b/drivers/pci/dwc/Kconfig
> @@ -14,6 +14,36 @@ config PCIE_DW_EP
>  	depends on PCI_ENDPOINT
>  	select PCIE_DW
>  
> +config PCIE_ARTPEC6
> +	bool "Axis ARTPEC-6 PCIe controller"
> +	depends on (PCI && PCI_MSI_IRQ_DOMAIN) || PCI_ENDPOINT
> +	depends on MACH_ARTPEC6
> +	help
> +	  Say Y here to enable PCIe controller support on Axis ARTPEC-6
> +	  SoCs.  This PCIe controller uses the DesignWare core.
> +
> +if PCIE_ARTPEC6
> +
> +config PCIE_ARTPEC6_HOST
> +	bool "Axis ARTPEC-6 Host Mode"
> +	depends on PCI
> +	depends on PCI_MSI_IRQ_DOMAIN
> +	select PCIEPORTBUS
> +	select PCIE_DW_HOST
> +	help
> +	  Enables support for the PCIe controller in the ARTPEC-6 SoC to work in
> +	  host mode.
> +
> +config PCIE_ARTPEC6_EP
> +	bool "Axis ARTPEC-6 Endpoint Mode"
> +	depends on PCI_ENDPOINT
> +	select PCIE_DW_EP
> +	help
> +	  Enables support for the PCIe controller in the ARTPEC-6 SoC to work in
> +	  endpoint mode.
> +
> +endif
> +
>  config PCI_DRA7XX
>  	bool "TI DRA7xx PCIe controller"
>  	depends on SOC_DRA7XX || COMPILE_TEST
> @@ -148,17 +178,6 @@ config PCIE_ARMADA_8K
>  	  DesignWare hardware and therefore the driver re-uses the
>  	  DesignWare core functions to implement the driver.
>  
> -config PCIE_ARTPEC6
> -	bool "Axis ARTPEC-6 PCIe controller"
> -	depends on PCI
> -	depends on MACH_ARTPEC6
> -	depends on PCI_MSI_IRQ_DOMAIN
> -	select PCIEPORTBUS
> -	select PCIE_DW_HOST
> -	help
> -	  Say Y here to enable PCIe controller support on Axis ARTPEC-6
> -	  SoCs.  This PCIe controller uses the DesignWare core.
> -
>  config PCIE_KIRIN
>  	depends on OF && ARM64
>  	bool "HiSilicon Kirin series SoCs PCIe controllers"
> diff --git a/drivers/pci/dwc/Makefile b/drivers/pci/dwc/Makefile
> index c61be9738cce..ac98242b83a9 100644
> --- a/drivers/pci/dwc/Makefile
> +++ b/drivers/pci/dwc/Makefile
> @@ -2,6 +2,9 @@ obj-$(CONFIG_PCIE_DW) += pcie-designware.o
>  obj-$(CONFIG_PCIE_DW_HOST) += pcie-designware-host.o
>  obj-$(CONFIG_PCIE_DW_EP) += pcie-designware-ep.o
>  obj-$(CONFIG_PCIE_DW_PLAT) += pcie-designware-plat.o
> +ifneq ($(filter y,$(CONFIG_PCIE_ARTPEC6_HOST) $(CONFIG_PCIE_ARTPEC6_EP)),)
> +        obj-$(CONFIG_PCIE_ARTPEC6) += pcie-artpec6.o
> +endif
>  ifneq ($(filter y,$(CONFIG_PCI_DRA7XX_HOST) $(CONFIG_PCI_DRA7XX_EP)),)
>          obj-$(CONFIG_PCI_DRA7XX) += pci-dra7xx.o
>  endif
> @@ -12,7 +15,6 @@ obj-$(CONFIG_PCI_KEYSTONE) += pci-keystone-dw.o pci-keystone.o
>  obj-$(CONFIG_PCI_LAYERSCAPE) += pci-layerscape.o
>  obj-$(CONFIG_PCIE_QCOM) += pcie-qcom.o
>  obj-$(CONFIG_PCIE_ARMADA_8K) += pcie-armada8k.o
> -obj-$(CONFIG_PCIE_ARTPEC6) += pcie-artpec6.o
>  obj-$(CONFIG_PCIE_KIRIN) += pcie-kirin.o
>  
>  # The following drivers are for devices that use the generic ACPI
> diff --git a/drivers/pci/dwc/pcie-artpec6.c b/drivers/pci/dwc/pcie-artpec6.c
> index c5d7f98dc6b2..21ea9ffef784 100644
> --- a/drivers/pci/dwc/pcie-artpec6.c
> +++ b/drivers/pci/dwc/pcie-artpec6.c
> @@ -13,6 +13,7 @@
>  #include <linux/delay.h>
>  #include <linux/kernel.h>
>  #include <linux/init.h>
> +#include <linux/of_device.h>
>  #include <linux/pci.h>
>  #include <linux/platform_device.h>
>  #include <linux/resource.h>
> @@ -30,8 +31,15 @@ struct artpec6_pcie {
>  	struct dw_pcie		*pci;
>  	struct regmap		*regmap;	/* DT axis,syscon-pcie */
>  	void __iomem		*phy_base;	/* DT phy */
> +	enum dw_pcie_device_mode mode;
>  };
>  
> +struct artpec_pcie_of_data {
> +	enum dw_pcie_device_mode mode;
> +};
> +
> +static const struct of_device_id artpec6_pcie_of_match[];
> +
>  /* PCIe Port Logic registers (memory-mapped) */
>  #define PL_OFFSET			0x700
>  #define PCIE_PHY_DEBUG_R0		(PL_OFFSET + 0x28)
> @@ -42,6 +50,7 @@ struct artpec6_pcie {
>  #define  PCIECFG_DBG_OEN		BIT(24)
>  #define  PCIECFG_CORE_RESET_REQ		BIT(21)
>  #define  PCIECFG_LTSSM_ENABLE		BIT(20)
> +#define  PCIECFG_DEVICE_TYPE_MASK	GENMASK(19, 16)
>  #define  PCIECFG_CLKREQ_B		BIT(11)
>  #define  PCIECFG_REFCLK_ENABLE		BIT(10)
>  #define  PCIECFG_PLL_ENABLE		BIT(9)
> @@ -126,6 +135,16 @@ static void artpec6_pcie_init_phy(struct artpec6_pcie *artpec6_pcie)
>  	} while (retries && !(val & PHY_COSPLLLOCK));
>  }
>  
> +static void artpec6_pcie_stop_link(struct dw_pcie *pci)
> +{
> +	struct artpec6_pcie *artpec6_pcie = to_artpec6_pcie(pci);
> +	u32 val;
> +
> +	val = artpec6_pcie_readl(artpec6_pcie, PCIECFG);
> +	val &= ~PCIECFG_LTSSM_ENABLE;
> +	artpec6_pcie_writel(artpec6_pcie, PCIECFG, val);
> +}
> +
>  static int artpec6_pcie_establish_link(struct dw_pcie *pci)
>  {
>  	struct artpec6_pcie *artpec6_pcie = to_artpec6_pcie(pci);
> @@ -195,6 +214,136 @@ static irqreturn_t artpec6_pcie_msi_handler(int irq, void *arg)
>  	return dw_handle_msi_irq(pp);
>  }
>  
> +static void dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar)
> +{
> +	u32 reg;
> +
> +	reg = PCI_BASE_ADDRESS_0 + (4 * bar);
> +	dw_pcie_dbi_ro_wr_en(pci);
> +	dw_pcie_writel_dbi2(pci, reg, 0x0);
> +	dw_pcie_writel_dbi(pci, reg, 0x0);
> +	dw_pcie_dbi_ro_wr_dis(pci);
> +}
> +
> +static void artpec6_pcie_ep_init(struct dw_pcie_ep *ep)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> +	struct artpec6_pcie *artpec6_pcie = to_artpec6_pcie(pci);
> +	enum pci_barno bar;
> +
> +	artpec6_pcie_assert_core_reset(artpec6_pcie);
> +	artpec6_pcie_init_phy(artpec6_pcie);
> +	artpec6_pcie_deassert_core_reset(artpec6_pcie);
> +
> +	for (bar = BAR_0; bar <= BAR_5; bar++)
> +		dw_pcie_ep_reset_bar(pci, bar);
> +}
> +
> +static int artpec6_pcie_raise_msi_irq(struct dw_pcie_ep *ep,
> +				      u8 interrupt_num)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> +	struct pci_epc *epc = ep->epc;
> +	u32 cap_addr, cap_value, cap_id, next_ptr, msg_ctrl, msg_data;
> +	u32 msg_addr_lower, msg_addr_upper;
> +	u64 msg_addr;
> +	bool has_upper;
> +	int ret;
> +
> +	/* Raise MSI per the PCI Local Bus Specification Revision 3.0, 6.8.1. */
> +	cap_addr = dw_pcie_readl_dbi(pci, PCI_CAPABILITY_LIST);
> +	while (1) {
> +		cap_value = dw_pcie_readl_dbi(pci, cap_addr);
> +		cap_id = cap_value & GENMASK(7, 0);
> +		if (cap_id == PCI_CAP_ID_MSI)
> +			break;
> +		next_ptr = (cap_value & GENMASK(15, 8)) >> 8;
> +		if (next_ptr == 0) {
> +			dev_err(pci->dev, "No MSI cap found!\n");
> +			return -EINVAL;
> +		}
> +		cap_addr = next_ptr;
> +	}

The MSI capability always points to fixed offset in DesingWare. So I don't
think this is necessary unless we move it to the generic endpoint layer.
> +	msg_ctrl = (cap_value & GENMASK(31, 16)) >> 16;
> +	has_upper = !!(msg_ctrl & BIT(7));
> +	msg_addr_lower = dw_pcie_readl_dbi(pci, cap_addr + 0x4);
> +	if (has_upper) {
> +		msg_addr_upper = dw_pcie_readl_dbi(pci, cap_addr + 0x8);
> +		msg_data = dw_pcie_readl_dbi(pci, cap_addr + 0xc) &
> +			GENMASK(15, 0);
> +	} else {
> +		msg_addr_upper = 0;
> +		msg_data = dw_pcie_readl_dbi(pci, cap_addr + 0x8) &
> +			GENMASK(15, 0);
> +	}
> +	msg_addr = ((u64) msg_addr_upper) << 32 | msg_addr_lower;
> +	ret = epc->ops->map_addr(epc, ep->msi_mem_phys, msg_addr, PAGE_SIZE);
> +	if (ret)
> +		return ret;
> +
> +	writel(msg_data | (interrupt_num - 1), ep->msi_mem);
> +
> +	epc->ops->unmap_addr(epc, ep->msi_mem_phys);
> +
> +	return 0;
> +}

I think this entire function can also be made as a library function to be used
by other drivers. (looks like only dra7xx has a shortcut to raise MSI irq).

Thanks
Kishon
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH 08/10] PCI: dwc: artpec6: add support for endpoint mode
  2017-10-18  8:46     ` Kishon Vijay Abraham I
@ 2017-10-20 10:48       ` Niklas Cassel
  -1 siblings, 0 replies; 41+ messages in thread
From: Niklas Cassel @ 2017-10-20 10:48 UTC (permalink / raw)
  To: Kishon Vijay Abraham I, Jesper Nilsson, Bjorn Helgaas,
	Rob Herring, Mark Rutland, Jingoo Han, Xiaowei Song,
	Peter Robinson, Tomasz Nowicki, Gabriele Paoloni, Duc Dang
  Cc: linux-arm-kernel, linux-pci, devicetree, linux-kernel

On 10/18/2017 10:46 AM, Kishon Vijay Abraham I wrote:
> Hi Niklas,
> 
> On Friday 13 October 2017 09:39 PM, Niklas Cassel wrote:
>> Signed-off-by: Niklas Cassel <niklas.cassel@axis.com>
>> ---
>>  .../devicetree/bindings/pci/axis,artpec6-pcie.txt  |   3 +-
>>  drivers/pci/dwc/Kconfig                            |  41 +++--
>>  drivers/pci/dwc/Makefile                           |   4 +-
>>  drivers/pci/dwc/pcie-artpec6.c                     | 202 ++++++++++++++++++++-
>>  4 files changed, 233 insertions(+), 17 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/pci/axis,artpec6-pcie.txt b/Documentation/devicetree/bindings/pci/axis,artpec6-pcie.txt
>> index 4e4aee4439ea..33eef7ae5a23 100644
>> --- a/Documentation/devicetree/bindings/pci/axis,artpec6-pcie.txt
>> +++ b/Documentation/devicetree/bindings/pci/axis,artpec6-pcie.txt
>> @@ -4,7 +4,8 @@ This PCIe host controller is based on the Synopsys DesignWare PCIe IP
>>  and thus inherits all the common properties defined in designware-pcie.txt.
>>  
>>  Required properties:
>> -- compatible: "axis,artpec6-pcie", "snps,dw-pcie"
>> +- compatible: "axis,artpec6-pcie", "snps,dw-pcie" for ARTPEC-6 in RC mode;
>> +	      "axis,artpec6-pcie-ep", "snps,dw-pcie" for ARTPEC-6 in EP mode;
> 
> If "snps,dw-pcie" is used for both RC and EP mode, how do we differentiate
> between the modes?

Hello Kishon,

Most DWC core based PCIe controllers have the following text in their
DT binding:
"This PCIe host controller is based on the Synopsys DesignWare PCIe IP
and thus inherits all the common properties defined in designware-pcie.txt."

It seems like DRA7xx is one of few DWC core based bindings that does
not include this text.

However, I can see that you've added "EP mode:" and "RC mode:" headings
to designware-pcie.txt.

But still the top of the file says:
Required properties:
- compatible: should contain "snps,dw-pcie" to identify the core.



However, I don't think there's necessarily any contradiction here,
since axis,artpec6-pcie.txt says:

compatible: "axis,artpec6-pcie", "snps,dw-pcie" for ARTPEC-6 in RC mode;
compatible: "axis,artpec6-pcie-ep", "snps,dw-pcie" for ARTPEC-6 in EP mode;

And according to:
https://elinux.org/Device_Tree_Usage#Understanding_the_compatible_Property

The first string in the list specifies the exact device.
The following strings represent other devices that the device is compatible
with.

Their example is:

The compatible property for the MPC8349 serial device should therefore be:
compatible = "fsl,mpc8349-uart", "ns16550".
In this case, fsl,mpc8349-uart specifies the exact device, and ns16550 states
that it is register-level compatible with a National Semiconductor 16550 UART.

The exact device is "axis,artpec6-pcie-ep", but it is register compatible with
"snps,dw-pcie".

But perhaps Rob Herring could correct me if I'm wrong.


Regards,
Niklas

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH 08/10] PCI: dwc: artpec6: add support for endpoint mode
@ 2017-10-20 10:48       ` Niklas Cassel
  0 siblings, 0 replies; 41+ messages in thread
From: Niklas Cassel @ 2017-10-20 10:48 UTC (permalink / raw)
  To: Kishon Vijay Abraham I, Jesper Nilsson, Bjorn Helgaas,
	Rob Herring, Mark Rutland, Jingoo Han, Xiaowei Song,
	Peter Robinson, Tomasz Nowicki, Gabriele Paoloni, Duc Dang
  Cc: linux-arm-kernel, linux-pci, devicetree, linux-kernel

On 10/18/2017 10:46 AM, Kishon Vijay Abraham I wrote:
> Hi Niklas,
> 
> On Friday 13 October 2017 09:39 PM, Niklas Cassel wrote:
>> Signed-off-by: Niklas Cassel <niklas.cassel@axis.com>
>> ---
>>  .../devicetree/bindings/pci/axis,artpec6-pcie.txt  |   3 +-
>>  drivers/pci/dwc/Kconfig                            |  41 +++--
>>  drivers/pci/dwc/Makefile                           |   4 +-
>>  drivers/pci/dwc/pcie-artpec6.c                     | 202 ++++++++++++++++++++-
>>  4 files changed, 233 insertions(+), 17 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/pci/axis,artpec6-pcie.txt b/Documentation/devicetree/bindings/pci/axis,artpec6-pcie.txt
>> index 4e4aee4439ea..33eef7ae5a23 100644
>> --- a/Documentation/devicetree/bindings/pci/axis,artpec6-pcie.txt
>> +++ b/Documentation/devicetree/bindings/pci/axis,artpec6-pcie.txt
>> @@ -4,7 +4,8 @@ This PCIe host controller is based on the Synopsys DesignWare PCIe IP
>>  and thus inherits all the common properties defined in designware-pcie.txt.
>>  
>>  Required properties:
>> -- compatible: "axis,artpec6-pcie", "snps,dw-pcie"
>> +- compatible: "axis,artpec6-pcie", "snps,dw-pcie" for ARTPEC-6 in RC mode;
>> +	      "axis,artpec6-pcie-ep", "snps,dw-pcie" for ARTPEC-6 in EP mode;
> 
> If "snps,dw-pcie" is used for both RC and EP mode, how do we differentiate
> between the modes?

Hello Kishon,

Most DWC core based PCIe controllers have the following text in their
DT binding:
"This PCIe host controller is based on the Synopsys DesignWare PCIe IP
and thus inherits all the common properties defined in designware-pcie.txt."

It seems like DRA7xx is one of few DWC core based bindings that does
not include this text.

However, I can see that you've added "EP mode:" and "RC mode:" headings
to designware-pcie.txt.

But still the top of the file says:
Required properties:
- compatible: should contain "snps,dw-pcie" to identify the core.



However, I don't think there's necessarily any contradiction here,
since axis,artpec6-pcie.txt says:

compatible: "axis,artpec6-pcie", "snps,dw-pcie" for ARTPEC-6 in RC mode;
compatible: "axis,artpec6-pcie-ep", "snps,dw-pcie" for ARTPEC-6 in EP mode;

And according to:
https://elinux.org/Device_Tree_Usage#Understanding_the_compatible_Property

The first string in the list specifies the exact device.
The following strings represent other devices that the device is compatible
with.

Their example is:

The compatible property for the MPC8349 serial device should therefore be:
compatible = "fsl,mpc8349-uart", "ns16550".
In this case, fsl,mpc8349-uart specifies the exact device, and ns16550 states
that it is register-level compatible with a National Semiconductor 16550 UART.

The exact device is "axis,artpec6-pcie-ep", but it is register compatible with
"snps,dw-pcie".

But perhaps Rob Herring could correct me if I'm wrong.


Regards,
Niklas

^ permalink raw reply	[flat|nested] 41+ messages in thread

* [PATCH 09/10] PCI: dwc: make cpu_addr_fixup take struct dw_pcie as argument
  2017-10-13 16:09 ` Niklas Cassel
                   ` (8 preceding siblings ...)
  (?)
@ 2017-10-13 16:09 ` Niklas Cassel
  2017-10-16 12:40     ` Niklas Cassel
  -1 siblings, 1 reply; 41+ messages in thread
From: Niklas Cassel @ 2017-10-13 16:09 UTC (permalink / raw)
  To: Kishon Vijay Abraham I, Bjorn Helgaas, Niklas Cassel,
	Jesper Nilsson, Jingoo Han, Joao Pinto
  Cc: linux-omap, linux-pci, linux-kernel, linux-arm-kernel

There is no need to hard code the cpu to bus fixup address.
By calculating the sum of sizes of config, io and mem,
from device tree, we know how big the PCIe window is.

The bus address has to be inside of this range, so all bits in
the cpu address that are higher than this range, are the ones
that we need to clear to get the local bus address.

Also for ARTPEC-7, hard coding the cpu fixup address is
not possible, since it uses a High Address Bits Look Up Table,
which means that it can, at runtime, map the PCIe window
to an arbitrary address in the 32-bit address space.

This also fixes a bug for ARTPEC-6, where the cpu_fixup_address
previously masked one bit too many. (Another reason why
it should be calculated from device tree.)

Signed-off-by: Niklas Cassel <niklas.cassel@axis.com>
---
 drivers/pci/dwc/pci-dra7xx.c      |  2 +-
 drivers/pci/dwc/pcie-artpec6.c    | 42 +++++++++++++++++++++++++++++++++++----
 drivers/pci/dwc/pcie-designware.c |  2 +-
 drivers/pci/dwc/pcie-designware.h |  2 +-
 4 files changed, 41 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/dwc/pci-dra7xx.c b/drivers/pci/dwc/pci-dra7xx.c
index 34427a6a15af..a93bafc72fa0 100644
--- a/drivers/pci/dwc/pci-dra7xx.c
+++ b/drivers/pci/dwc/pci-dra7xx.c
@@ -109,7 +109,7 @@ static inline void dra7xx_pcie_writel(struct dra7xx_pcie *pcie, u32 offset,
 	writel(value, pcie->base + offset);
 }
 
-static u64 dra7xx_pcie_cpu_addr_fixup(u64 pci_addr)
+static u64 dra7xx_pcie_cpu_addr_fixup(struct dw_pcie *pci, u64 pci_addr)
 {
 	return pci_addr & DRA7XX_CPU_TO_BUS_ADDR;
 }
diff --git a/drivers/pci/dwc/pcie-artpec6.c b/drivers/pci/dwc/pcie-artpec6.c
index 21ea9ffef784..1e113dbea1da 100644
--- a/drivers/pci/dwc/pcie-artpec6.c
+++ b/drivers/pci/dwc/pcie-artpec6.c
@@ -32,6 +32,7 @@ struct artpec6_pcie {
 	struct regmap		*regmap;	/* DT axis,syscon-pcie */
 	void __iomem		*phy_base;	/* DT phy */
 	enum dw_pcie_device_mode mode;
+	u64 cpu_fixup_mask;
 };
 
 struct artpec_pcie_of_data {
@@ -69,8 +70,6 @@ static const struct of_device_id artpec6_pcie_of_match[];
 #define PHY_STATUS			0x118
 #define  PHY_COSPLLLOCK			BIT(0)
 
-#define ARTPEC6_CPU_TO_BUS_ADDR		GENMASK(27, 0)
-
 static u32 artpec6_pcie_readl(struct artpec6_pcie *artpec6_pcie, u32 offset)
 {
 	u32 val;
@@ -84,9 +83,42 @@ static void artpec6_pcie_writel(struct artpec6_pcie *artpec6_pcie, u32 offset, u
 	regmap_write(artpec6_pcie->regmap, offset, val);
 }
 
-static u64 artpec6_pcie_cpu_addr_fixup(u64 pci_addr)
+static void artpec6_pcie_calc_cpu_fixup_mask(struct artpec6_pcie *artpec6_pcie)
 {
-	return pci_addr & ARTPEC6_CPU_TO_BUS_ADDR;
+	struct dw_pcie *pci = artpec6_pcie->pci;
+	struct pcie_port *pp = &pci->pp;
+	struct dw_pcie_ep *ep = &pci->ep;
+	u64 size;
+	u64 mask;
+	int msb;
+
+	switch (artpec6_pcie->mode) {
+	case DW_PCIE_RC_TYPE:
+		size = pp->cfg0_size + pp->cfg1_size + pp->io_size +
+			pp->mem_size;
+		break;
+	case DW_PCIE_EP_TYPE:
+		size = ep->addr_size;
+		break;
+	default:
+		dev_err(pci->dev, "UNKNOWN device type\n");
+		return;
+	}
+	/* Calculate the mask (which can have potential holes). */
+	mask = size - 1;
+	/* Find the mask's msb. */
+	msb = fls64(mask);
+	/* Use the msb to generate a new mask without any holes. */
+	mask = (1ULL << msb) - 1;
+	artpec6_pcie->cpu_fixup_mask = mask;
+	dev_dbg(pci->dev, "Using cpu fixup mask: 0x%llx\n", mask);
+}
+
+static u64 artpec6_pcie_cpu_addr_fixup(struct dw_pcie *pci, u64 pci_addr)
+{
+	struct artpec6_pcie *artpec6_pcie = to_artpec6_pcie(pci);
+
+	return pci_addr & artpec6_pcie->cpu_fixup_mask;
 }
 
 static void artpec6_pcie_init_phy(struct artpec6_pcie *artpec6_pcie)
@@ -190,6 +222,7 @@ static int artpec6_pcie_host_init(struct pcie_port *pp)
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
 	struct artpec6_pcie *artpec6_pcie = to_artpec6_pcie(pci);
 
+	artpec6_pcie_calc_cpu_fixup_mask(artpec6_pcie);
 	artpec6_pcie_assert_core_reset(artpec6_pcie);
 	artpec6_pcie_init_phy(artpec6_pcie);
 	artpec6_pcie_deassert_core_reset(artpec6_pcie);
@@ -231,6 +264,7 @@ static void artpec6_pcie_ep_init(struct dw_pcie_ep *ep)
 	struct artpec6_pcie *artpec6_pcie = to_artpec6_pcie(pci);
 	enum pci_barno bar;
 
+	artpec6_pcie_calc_cpu_fixup_mask(artpec6_pcie);
 	artpec6_pcie_assert_core_reset(artpec6_pcie);
 	artpec6_pcie_init_phy(artpec6_pcie);
 	artpec6_pcie_deassert_core_reset(artpec6_pcie);
diff --git a/drivers/pci/dwc/pcie-designware.c b/drivers/pci/dwc/pcie-designware.c
index 88abdddee2ad..800be7a4f087 100644
--- a/drivers/pci/dwc/pcie-designware.c
+++ b/drivers/pci/dwc/pcie-designware.c
@@ -149,7 +149,7 @@ void dw_pcie_prog_outbound_atu(struct dw_pcie *pci, int index, int type,
 	u32 retries, val;
 
 	if (pci->ops->cpu_addr_fixup)
-		cpu_addr = pci->ops->cpu_addr_fixup(cpu_addr);
+		cpu_addr = pci->ops->cpu_addr_fixup(pci, cpu_addr);
 
 	if (pci->iatu_unroll_enabled) {
 		dw_pcie_prog_outbound_atu_unroll(pci, index, type, cpu_addr,
diff --git a/drivers/pci/dwc/pcie-designware.h b/drivers/pci/dwc/pcie-designware.h
index f56d040afde4..897e190f8e24 100644
--- a/drivers/pci/dwc/pcie-designware.h
+++ b/drivers/pci/dwc/pcie-designware.h
@@ -203,7 +203,7 @@ struct dw_pcie_ep {
 };
 
 struct dw_pcie_ops {
-	u64	(*cpu_addr_fixup)(u64 cpu_addr);
+	u64	(*cpu_addr_fixup)(struct dw_pcie *pcie, u64 cpu_addr);
 	u32	(*read_dbi)(struct dw_pcie *pcie, void __iomem *base, u32 reg,
 			    size_t size);
 	void	(*write_dbi)(struct dw_pcie *pcie, void __iomem *base, u32 reg,
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 41+ messages in thread

* Re: [PATCH 09/10] PCI: dwc: make cpu_addr_fixup take struct dw_pcie as argument
  2017-10-13 16:09 ` [PATCH 09/10] PCI: dwc: make cpu_addr_fixup take struct dw_pcie as argument Niklas Cassel
@ 2017-10-16 12:40     ` Niklas Cassel
  0 siblings, 0 replies; 41+ messages in thread
From: Niklas Cassel @ 2017-10-16 12:40 UTC (permalink / raw)
  To: Kishon Vijay Abraham I, Bjorn Helgaas, Jesper Nilsson, Jingoo Han,
	Joao Pinto
  Cc: linux-omap, linux-pci, linux-kernel, linux-arm-kernel

On 10/13/2017 06:09 PM, Niklas Cassel wrote:
(snip)

Hello Kishon

I just wanted to say that the function
artpec6_pcie_calc_cpu_fixup_mask should work on dra7xx
as well. (I had dra7xx in mind when I wrote it.)
However, I did not want to change pci-dra7xx.c to
also use this function, since your current code is
quite simple.
But if you think that it's a good idea, we could
rename and move artpec6_pcie_calc_cpu_fixup_mask
to pcie-designware.c, and migrate pci-dra7xx
to utilize this function as well, as it is strictly
more correct to use values from device tree, rather
than having a hard coded define in the SoC driver.

Regards,
Niklas

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH 09/10] PCI: dwc: make cpu_addr_fixup take struct dw_pcie as argument
@ 2017-10-16 12:40     ` Niklas Cassel
  0 siblings, 0 replies; 41+ messages in thread
From: Niklas Cassel @ 2017-10-16 12:40 UTC (permalink / raw)
  To: Kishon Vijay Abraham I, Bjorn Helgaas, Jesper Nilsson, Jingoo Han,
	Joao Pinto
  Cc: linux-omap, linux-pci, linux-kernel, linux-arm-kernel

On 10/13/2017 06:09 PM, Niklas Cassel wrote:
(snip)

Hello Kishon

I just wanted to say that the function
artpec6_pcie_calc_cpu_fixup_mask should work on dra7xx
as well. (I had dra7xx in mind when I wrote it.)
However, I did not want to change pci-dra7xx.c to
also use this function, since your current code is
quite simple.
But if you think that it's a good idea, we could
rename and move artpec6_pcie_calc_cpu_fixup_mask
to pcie-designware.c, and migrate pci-dra7xx
to utilize this function as well, as it is strictly
more correct to use values from device tree, rather
than having a hard coded define in the SoC driver.

Regards,
Niklas

^ permalink raw reply	[flat|nested] 41+ messages in thread

* [PATCH 10/10] PCI: dwc: artpec6: add support for the ARTPEC-7 SoC
  2017-10-13 16:09 ` Niklas Cassel
                   ` (9 preceding siblings ...)
  (?)
@ 2017-10-13 16:09 ` Niklas Cassel
  2017-10-17 22:25   ` Rob Herring
  -1 siblings, 1 reply; 41+ messages in thread
From: Niklas Cassel @ 2017-10-13 16:09 UTC (permalink / raw)
  To: Niklas Cassel, Jesper Nilsson, Bjorn Helgaas, Rob Herring,
	Mark Rutland
  Cc: linux-arm-kernel, linux-pci, devicetree, linux-kernel

The ARTPEC-6 SoC and the ARTPEC-7 SoC are very similar.
Unfortunately, some fields in the PCIECFG and PCIESTAT
register have changed.

Signed-off-by: Niklas Cassel <niklas.cassel@axis.com>
---
 .../devicetree/bindings/pci/axis,artpec6-pcie.txt  |   2 +
 drivers/pci/dwc/pcie-artpec6.c                     | 162 ++++++++++++++++++++-
 2 files changed, 161 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/axis,artpec6-pcie.txt b/Documentation/devicetree/bindings/pci/axis,artpec6-pcie.txt
index 33eef7ae5a23..979dc7b6cfe8 100644
--- a/Documentation/devicetree/bindings/pci/axis,artpec6-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/axis,artpec6-pcie.txt
@@ -6,6 +6,8 @@ and thus inherits all the common properties defined in designware-pcie.txt.
 Required properties:
 - compatible: "axis,artpec6-pcie", "snps,dw-pcie" for ARTPEC-6 in RC mode;
 	      "axis,artpec6-pcie-ep", "snps,dw-pcie" for ARTPEC-6 in EP mode;
+	      "axis,artpec7-pcie", "snps,dw-pcie" for ARTPEC-7 in RC mode;
+	      "axis,artpec7-pcie-ep", "snps,dw-pcie" for ARTPEC-7 in EP mode;
 - reg: base addresses and lengths of the PCIe controller (DBI),
 	the PHY controller, and configuration address space.
 - reg-names: Must include the following entries:
diff --git a/drivers/pci/dwc/pcie-artpec6.c b/drivers/pci/dwc/pcie-artpec6.c
index 1e113dbea1da..80f504babbae 100644
--- a/drivers/pci/dwc/pcie-artpec6.c
+++ b/drivers/pci/dwc/pcie-artpec6.c
@@ -27,15 +27,22 @@
 
 #define to_artpec6_pcie(x)	dev_get_drvdata((x)->dev)
 
+enum artpec_pcie_variants {
+	ARTPEC6,
+	ARTPEC7,
+};
+
 struct artpec6_pcie {
 	struct dw_pcie		*pci;
 	struct regmap		*regmap;	/* DT axis,syscon-pcie */
 	void __iomem		*phy_base;	/* DT phy */
+	enum artpec_pcie_variants variant;
 	enum dw_pcie_device_mode mode;
 	u64 cpu_fixup_mask;
 };
 
 struct artpec_pcie_of_data {
+	enum artpec_pcie_variants variant;
 	enum dw_pcie_device_mode mode;
 };
 
@@ -46,6 +53,13 @@ static const struct of_device_id artpec6_pcie_of_match[];
 #define PCIE_PHY_DEBUG_R0		(PL_OFFSET + 0x28)
 #define PCIE_PHY_DEBUG_R1		(PL_OFFSET + 0x2c)
 
+#define ACK_F_ASPM_CTRL_OFF		(PL_OFFSET + 0xc)
+#define ACK_N_FTS_MASK			GENMASK(15, 8)
+#define ACK_N_FTS(x)			(((x) << 8) & ACK_N_FTS_MASK)
+
+#define FAST_TRAINING_SEQ_MASK		GENMASK(7, 0)
+#define FAST_TRAINING_SEQ(x)		(((x) << 0) & FAST_TRAINING_SEQ_MASK)
+
 /* ARTPEC-6 specific registers */
 #define PCIECFG				0x18
 #define  PCIECFG_DBG_OEN		BIT(24)
@@ -60,6 +74,13 @@ static const struct of_device_id artpec6_pcie_of_match[];
 #define  PCIECFG_MODE_TX_DRV_EN		BIT(3)
 #define  PCIECFG_CISRREN		BIT(2)
 #define  PCIECFG_MACRO_ENABLE		BIT(0)
+/* ARTPEC-7 specific fields */
+#define  PCIECFG_REFCLKSEL		BIT(23)
+#define  PCIECFG_NOC_RESET		BIT(3)
+
+#define PCIESTAT			0x1c
+/* ARTPEC-7 specific fields */
+#define  PCIESTAT_EXTREFCLK		BIT(3)
 
 #define NOCCFG				0x40
 #define  NOCCFG_ENABLE_CLK_PCIE		BIT(4)
@@ -70,6 +91,12 @@ static const struct of_device_id artpec6_pcie_of_match[];
 #define PHY_STATUS			0x118
 #define  PHY_COSPLLLOCK			BIT(0)
 
+#define PHY_TX_ASIC_OUT			0x1014
+#define  PHY_TX_ASIC_OUT_TX_ACK		BIT(0)
+
+#define PHY_RX_ASIC_OUT			0x101b
+#define  PHY_RX_ASIC_OUT_ACK		BIT(0)
+
 static u32 artpec6_pcie_readl(struct artpec6_pcie *artpec6_pcie, u32 offset)
 {
 	u32 val;
@@ -121,7 +148,7 @@ static u64 artpec6_pcie_cpu_addr_fixup(struct dw_pcie *pci, u64 pci_addr)
 	return pci_addr & artpec6_pcie->cpu_fixup_mask;
 }
 
-static void artpec6_pcie_init_phy(struct artpec6_pcie *artpec6_pcie)
+static void artpec6_pcie_init_phy_a6(struct artpec6_pcie *artpec6_pcie)
 {
 	u32 val;
 	unsigned int retries;
@@ -167,6 +194,96 @@ static void artpec6_pcie_init_phy(struct artpec6_pcie *artpec6_pcie)
 	} while (retries && !(val & PHY_COSPLLLOCK));
 }
 
+static void artpec6_pcie_init_phy_a7(struct artpec6_pcie *artpec6_pcie)
+{
+	struct dw_pcie *pci = artpec6_pcie->pci;
+	u32 val;
+	u16 phy_status_tx, phy_status_rx;
+	unsigned int retries;
+	bool extrefclk;
+
+	/* Check if external reference clock is connected */
+	val = artpec6_pcie_readl(artpec6_pcie, PCIESTAT);
+	extrefclk = !!(val & PCIESTAT_EXTREFCLK);
+	dev_dbg(pci->dev, "Using reference clock: %s\n",
+		extrefclk ? "external" : "internal");
+
+	val = artpec6_pcie_readl(artpec6_pcie, PCIECFG);
+	val |=  PCIECFG_RISRCREN |	/* Receiver term. 50 Ohm */
+		PCIECFG_PCLK_ENABLE;
+	if (extrefclk)
+		val |= PCIECFG_REFCLKSEL;
+	else
+		val &= ~PCIECFG_REFCLKSEL;
+	artpec6_pcie_writel(artpec6_pcie, PCIECFG, val);
+	usleep_range(10, 20);
+
+	val = artpec6_pcie_readl(artpec6_pcie, NOCCFG);
+	val |= NOCCFG_ENABLE_CLK_PCIE;
+	artpec6_pcie_writel(artpec6_pcie, NOCCFG, val);
+	usleep_range(20, 30);
+
+	val = artpec6_pcie_readl(artpec6_pcie, NOCCFG);
+	val &= ~NOCCFG_POWER_PCIE_IDLEREQ;
+	artpec6_pcie_writel(artpec6_pcie, NOCCFG, val);
+
+	retries = 50;
+	do {
+		usleep_range(1000, 2000);
+		val = artpec6_pcie_readl(artpec6_pcie, NOCCFG);
+		retries--;
+	} while (retries &&
+		(val & (NOCCFG_POWER_PCIE_IDLEACK | NOCCFG_POWER_PCIE_IDLE)));
+
+	retries = 50;
+	do {
+		usleep_range(1000, 2000);
+		phy_status_tx = readw(artpec6_pcie->phy_base + PHY_TX_ASIC_OUT);
+		phy_status_rx = readw(artpec6_pcie->phy_base + PHY_RX_ASIC_OUT);
+		retries--;
+	} while (retries && ((phy_status_tx & PHY_TX_ASIC_OUT_TX_ACK) ||
+				(phy_status_rx & PHY_RX_ASIC_OUT_ACK)));
+}
+
+static void artpec6_pcie_init_phy(struct artpec6_pcie *artpec6_pcie)
+{
+	switch (artpec6_pcie->variant) {
+	case ARTPEC6:
+		artpec6_pcie_init_phy_a6(artpec6_pcie);
+		break;
+	case ARTPEC7:
+		artpec6_pcie_init_phy_a7(artpec6_pcie);
+		break;
+	}
+}
+
+static void artpec6_pcie_set_nfts(struct artpec6_pcie *artpec6_pcie)
+{
+	struct dw_pcie *pci = artpec6_pcie->pci;
+	u32 val;
+
+	if (artpec6_pcie->variant != ARTPEC7)
+		return;
+
+	/*
+	 * Increase the N_FTS (Number of Fast Training Sequences)
+	 * to be transmitted when transitioning from L0s to L0.
+	 */
+	val = dw_pcie_readl_dbi(pci, ACK_F_ASPM_CTRL_OFF);
+	val &= ~ACK_N_FTS_MASK;
+	val |= ACK_N_FTS(180);
+	dw_pcie_writel_dbi(pci, ACK_F_ASPM_CTRL_OFF, val);
+
+	/*
+	 * Set the Number of Fast Training Sequences that the core
+	 * advertises as its N_FTS during Gen2 or Gen3 link training.
+	 */
+	val = dw_pcie_readl_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL);
+	val &= ~FAST_TRAINING_SEQ_MASK;
+	val |= FAST_TRAINING_SEQ(180);
+	dw_pcie_writel_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL, val);
+}
+
 static void artpec6_pcie_stop_link(struct dw_pcie *pci)
 {
 	struct artpec6_pcie *artpec6_pcie = to_artpec6_pcie(pci);
@@ -203,7 +320,14 @@ static void artpec6_pcie_assert_core_reset(struct artpec6_pcie *artpec6_pcie)
 	u32 val;
 
 	val = artpec6_pcie_readl(artpec6_pcie, PCIECFG);
-	val |= PCIECFG_CORE_RESET_REQ;
+	switch (artpec6_pcie->variant) {
+	case ARTPEC6:
+		val |= PCIECFG_CORE_RESET_REQ;
+		break;
+	case ARTPEC7:
+		val &= ~PCIECFG_NOC_RESET;
+		break;
+	}
 	artpec6_pcie_writel(artpec6_pcie, PCIECFG, val);
 }
 
@@ -212,7 +336,14 @@ static void artpec6_pcie_deassert_core_reset(struct artpec6_pcie *artpec6_pcie)
 	u32 val;
 
 	val = artpec6_pcie_readl(artpec6_pcie, PCIECFG);
-	val &= ~PCIECFG_CORE_RESET_REQ;
+	switch (artpec6_pcie->variant) {
+	case ARTPEC6:
+		val &= ~PCIECFG_CORE_RESET_REQ;
+		break;
+	case ARTPEC7:
+		val |= PCIECFG_NOC_RESET;
+		break;
+	}
 	artpec6_pcie_writel(artpec6_pcie, PCIECFG, val);
 	usleep_range(100, 200);
 }
@@ -226,6 +357,7 @@ static int artpec6_pcie_host_init(struct pcie_port *pp)
 	artpec6_pcie_assert_core_reset(artpec6_pcie);
 	artpec6_pcie_init_phy(artpec6_pcie);
 	artpec6_pcie_deassert_core_reset(artpec6_pcie);
+	artpec6_pcie_set_nfts(artpec6_pcie);
 	dw_pcie_setup_rc(pp);
 	artpec6_pcie_establish_link(pci);
 	dw_pcie_wait_for_link(pci);
@@ -268,6 +400,7 @@ static void artpec6_pcie_ep_init(struct dw_pcie_ep *ep)
 	artpec6_pcie_assert_core_reset(artpec6_pcie);
 	artpec6_pcie_init_phy(artpec6_pcie);
 	artpec6_pcie_deassert_core_reset(artpec6_pcie);
+	artpec6_pcie_set_nfts(artpec6_pcie);
 
 	for (bar = BAR_0; bar <= BAR_5; bar++)
 		dw_pcie_ep_reset_bar(pci, bar);
@@ -431,6 +564,7 @@ static int artpec6_pcie_probe(struct platform_device *pdev)
 	int ret;
 	const struct of_device_id *match;
 	const struct artpec_pcie_of_data *data;
+	enum artpec_pcie_variants variant;
 	enum dw_pcie_device_mode mode;
 
 	match = of_match_device(artpec6_pcie_of_match, dev);
@@ -438,6 +572,7 @@ static int artpec6_pcie_probe(struct platform_device *pdev)
 		return -EINVAL;
 
 	data = (struct artpec_pcie_of_data *)match->data;
+	variant = (enum artpec_pcie_variants)data->variant;
 	mode = (enum dw_pcie_device_mode)data->mode;
 
 	artpec6_pcie = devm_kzalloc(dev, sizeof(*artpec6_pcie), GFP_KERNEL);
@@ -452,6 +587,7 @@ static int artpec6_pcie_probe(struct platform_device *pdev)
 	pci->ops = &dw_pcie_ops;
 
 	artpec6_pcie->pci = pci;
+	artpec6_pcie->variant = variant;
 	artpec6_pcie->mode = mode;
 
 	dbi_base = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi");
@@ -497,10 +633,22 @@ static int artpec6_pcie_probe(struct platform_device *pdev)
 }
 
 static const struct artpec_pcie_of_data artpec6_pcie_rc_of_data = {
+	.variant = ARTPEC6,
 	.mode = DW_PCIE_RC_TYPE,
 };
 
 static const struct artpec_pcie_of_data artpec6_pcie_ep_of_data = {
+	.variant = ARTPEC6,
+	.mode = DW_PCIE_EP_TYPE,
+};
+
+static const struct artpec_pcie_of_data artpec7_pcie_rc_of_data = {
+	.variant = ARTPEC7,
+	.mode = DW_PCIE_RC_TYPE,
+};
+
+static const struct artpec_pcie_of_data artpec7_pcie_ep_of_data = {
+	.variant = ARTPEC7,
 	.mode = DW_PCIE_EP_TYPE,
 };
 
@@ -513,6 +661,14 @@ static const struct of_device_id artpec6_pcie_of_match[] = {
 		.compatible = "axis,artpec6-pcie-ep",
 		.data = &artpec6_pcie_ep_of_data,
 	},
+	{
+		.compatible = "axis,artpec7-pcie",
+		.data = &artpec7_pcie_rc_of_data,
+	},
+	{
+		.compatible = "axis,artpec7-pcie-ep",
+		.data = &artpec7_pcie_ep_of_data,
+	},
 	{},
 };
 
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 41+ messages in thread

* Re: [PATCH 10/10] PCI: dwc: artpec6: add support for the ARTPEC-7 SoC
  2017-10-13 16:09 ` [PATCH 10/10] PCI: dwc: artpec6: add support for the ARTPEC-7 SoC Niklas Cassel
@ 2017-10-17 22:25   ` Rob Herring
  0 siblings, 0 replies; 41+ messages in thread
From: Rob Herring @ 2017-10-17 22:25 UTC (permalink / raw)
  To: Niklas Cassel
  Cc: Niklas Cassel, Jesper Nilsson, Bjorn Helgaas, Mark Rutland,
	linux-arm-kernel, linux-pci, devicetree, linux-kernel

On Fri, Oct 13, 2017 at 06:09:13PM +0200, Niklas Cassel wrote:
> The ARTPEC-6 SoC and the ARTPEC-7 SoC are very similar.
> Unfortunately, some fields in the PCIECFG and PCIESTAT
> register have changed.
> 
> Signed-off-by: Niklas Cassel <niklas.cassel@axis.com>
> ---
>  .../devicetree/bindings/pci/axis,artpec6-pcie.txt  |   2 +

Acked-by: Rob Herring <robh@kernel.org>

>  drivers/pci/dwc/pcie-artpec6.c                     | 162 ++++++++++++++++++++-
>  2 files changed, 161 insertions(+), 3 deletions(-)

^ permalink raw reply	[flat|nested] 41+ messages in thread

end of thread, other threads:[~2017-10-20 10:48 UTC | newest]

Thread overview: 41+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-10-13 16:09 [PATCH 00/10] dwc MSI fixes, ARTPEC-6 EP mode support, ARTPEC-7 SoC support Niklas Cassel
2017-10-13 16:09 ` Niklas Cassel
2017-10-13 16:09 ` [PATCH 01/10] PCI: dwc: use DMA-API for allocating MSI data Niklas Cassel
2017-10-13 16:42   ` David Laight
2017-10-16 11:43     ` Niklas Cassel
2017-10-13 16:43   ` Jingoo Han
2017-10-16 12:08     ` Niklas Cassel
2017-10-13 16:47   ` Jingoo Han
2017-10-16 12:11     ` Niklas Cassel
2017-10-16 22:16   ` Bjorn Helgaas
2017-10-13 16:09 ` [PATCH 02/10] PCI: designware-ep: set_msi should only set MMC bits Niklas Cassel
2017-10-16 22:26   ` Bjorn Helgaas
2017-10-13 16:09 ` [PATCH 03/10] PCI: designware-ep: read-only registers need DBI_RO_WR_EN to be writable Niklas Cassel
2017-10-13 16:09 ` [PATCH 04/10] PCI: designware-ep: pre-allocate memory for MSI in dw_pcie_ep_init Niklas Cassel
2017-10-13 16:09 ` [PATCH 05/10] PCI: artpec6: remove unused defines Niklas Cassel
2017-10-13 16:09 ` [PATCH 06/10] PCI: dwc: artpec6: use BIT and GENMASK macros Niklas Cassel
2017-10-13 16:09 ` [PATCH 07/10] PCI: dwc: artpec6: split artpec6_pcie_establish_link to smaller functions Niklas Cassel
2017-10-13 16:09 ` [PATCH 08/10] PCI: dwc: artpec6: add support for endpoint mode Niklas Cassel
2017-10-16 23:43   ` Bjorn Helgaas
2017-10-18  8:03     ` Kishon Vijay Abraham I
2017-10-18  8:03       ` Kishon Vijay Abraham I
2017-10-18  8:15       ` Niklas Cassel
2017-10-18  8:15         ` Niklas Cassel
2017-10-18  8:47         ` Kishon Vijay Abraham I
2017-10-18  8:47           ` Kishon Vijay Abraham I
2017-10-19  7:59     ` Christoph Hellwig
2017-10-19  7:59       ` Christoph Hellwig
2017-10-19 10:57       ` Niklas Cassel
2017-10-19 10:57         ` Niklas Cassel
2017-10-19 11:40     ` Niklas Cassel
2017-10-19 11:40       ` Niklas Cassel
2017-10-17 22:24   ` Rob Herring
2017-10-18  8:46   ` Kishon Vijay Abraham I
2017-10-18  8:46     ` Kishon Vijay Abraham I
2017-10-20 10:48     ` Niklas Cassel
2017-10-20 10:48       ` Niklas Cassel
2017-10-13 16:09 ` [PATCH 09/10] PCI: dwc: make cpu_addr_fixup take struct dw_pcie as argument Niklas Cassel
2017-10-16 12:40   ` Niklas Cassel
2017-10-16 12:40     ` Niklas Cassel
2017-10-13 16:09 ` [PATCH 10/10] PCI: dwc: artpec6: add support for the ARTPEC-7 SoC Niklas Cassel
2017-10-17 22:25   ` Rob Herring

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.