[PATCH 0/5] arm64, pci: Add ECAM/PCIe support for Cavium ThunderX

[PATCH 0/5] arm64, pci: Add ECAM/PCIe support for Cavium ThunderX

[David Daney]

From: David Daney <david.daney@cavium.com>

The subject pretty much says it all.  The first four patches tweak the
infrastructure a little so that we can get required behavior.  The
final patch adds the drivers.

David Daney (5):
  pci: Add is_pcierc element to struct pci_bus
  gic-its: Allow pci_requester_id to be overridden.
  arm64, pci: Allow RC drivers to supply pcibios_add_device()
    implementation.
  irqchip: gic-v3: Add gic_get_irq_domain() to get the irqdomain of the
    GIC.
  PCI: Add host drivers for Cavium ThunderX processors.

 arch/arm64/include/asm/pci.h        |   3 +
 arch/arm64/kernel/pci.c             |  10 +
 drivers/irqchip/irq-gic-v3-its.c    |  14 +-
 drivers/irqchip/irq-gic-v3.c        |   5 +
 drivers/pci/host/Kconfig            |  12 +
 drivers/pci/host/Makefile           |   2 +
 drivers/pci/host/pcie-thunder-pem.c | 462 ++++++++++++++++++++++++++++++++++++
 drivers/pci/host/pcie-thunder.c     | 422 ++++++++++++++++++++++++++++++++
 drivers/pci/probe.c                 |   2 +
 include/linux/irqchip/arm-gic-v3.h  |   3 +
 include/linux/pci.h                 |   1 +
 11 files changed, 935 insertions(+), 1 deletion(-)
 create mode 100644 drivers/pci/host/pcie-thunder-pem.c
 create mode 100644 drivers/pci/host/pcie-thunder.c

-- 
1.9.1

[PATCH 1/5] pci: Add is_pcierc element to struct pci_bus

[David Daney]

From: David Daney <david.daney@cavium.com>

... and use is to force only_one_child() to return true.

Needed because the ThunderX PCIe RC cannot be identified by existing methods.

Signed-off-by: David Daney <david.daney@cavium.com>
---
 drivers/pci/probe.c | 2 ++
 include/linux/pci.h | 1 +
 2 files changed, 3 insertions(+)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index cefd636..11ec2e7 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1643,6 +1643,8 @@ static int only_one_child(struct pci_bus *bus)
 {
 	struct pci_dev *parent = bus->self;
 
+	if (bus->is_pcierc)
+		return 1;
 	if (!parent || !pci_is_pcie(parent))
 		return 0;
 	if (pci_pcie_type(parent) == PCI_EXP_TYPE_ROOT_PORT)
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 8a0321a..1f1ce73 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -473,6 +473,7 @@ struct pci_bus {
 	struct bin_attribute	*legacy_io; /* legacy I/O for this bus */
 	struct bin_attribute	*legacy_mem; /* legacy mem */
 	unsigned int		is_added:1;
+	unsigned int		is_pcierc:1;
 };
 
 #define to_pci_bus(n)	container_of(n, struct pci_bus, dev)
-- 
1.9.1

[PATCH 2/5] gic-its: Allow pci_requester_id to be overridden.

[David Daney]

From: David Daney <david.daney@cavium.com>

Signed-off-by: David Daney <david.daney@cavium.com>
---
 drivers/irqchip/irq-gic-v3-its.c   | 14 +++++++++++++-
 include/linux/irqchip/arm-gic-v3.h |  2 ++
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 1b7e155..4580970 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -1189,11 +1189,23 @@ static int its_pci_msi_vec_count(struct pci_dev *pdev)
 	return max(msi, msix);
 }
 
+static u32 its_dflt_pci_requester_id(struct pci_dev *pdev, u16 alias)
+{
+	return alias;
+}
+
+static its_pci_requester_id_t its_pci_requester_id = its_dflt_pci_requester_id;
+void set_its_pci_requester_id(its_pci_requester_id_t fn)
+{
+	its_pci_requester_id = fn;
+}
+EXPORT_SYMBOL(set_its_pci_requester_id);
+
 static int its_get_pci_alias(struct pci_dev *pdev, u16 alias, void *data)
 {
 	struct its_pci_alias *dev_alias = data;
 
-	dev_alias->dev_id = alias;
+	dev_alias->dev_id = its_pci_requester_id(pdev, alias);
 	if (pdev != dev_alias->pdev)
 		dev_alias->count += its_pci_msi_vec_count(dev_alias->pdev);
 
diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
index ffbc034..18e3757 100644
--- a/include/linux/irqchip/arm-gic-v3.h
+++ b/include/linux/irqchip/arm-gic-v3.h
@@ -389,6 +389,8 @@ int its_cpu_init(void);
 int its_init(struct device_node *node, struct rdists *rdists,
 	     struct irq_domain *domain);
 
+typedef u32 (*its_pci_requester_id_t)(struct pci_dev *, u16);
+void set_its_pci_requester_id(its_pci_requester_id_t fn);
 #endif
 
 #endif
-- 
1.9.1

[PATCH 2/5] gic-its: Allow pci_requester_id to be overridden.

[Marc Zyngier]

On 15/07/15 17:54, David Daney wrote:
> From: David Daney <david.daney@cavium.com>
> 
> Signed-off-by: David Daney <david.daney@cavium.com>
> ---
>  drivers/irqchip/irq-gic-v3-its.c   | 14 +++++++++++++-
>  include/linux/irqchip/arm-gic-v3.h |  2 ++
>  2 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index 1b7e155..4580970 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -1189,11 +1189,23 @@ static int its_pci_msi_vec_count(struct pci_dev *pdev)
>  	return max(msi, msix);
>  }
>  
> +static u32 its_dflt_pci_requester_id(struct pci_dev *pdev, u16 alias)
> +{
> +	return alias;
> +}
> +
> +static its_pci_requester_id_t its_pci_requester_id = its_dflt_pci_requester_id;
> +void set_its_pci_requester_id(its_pci_requester_id_t fn)
> +{
> +	its_pci_requester_id = fn;
> +}
> +EXPORT_SYMBOL(set_its_pci_requester_id);
> +
>  static int its_get_pci_alias(struct pci_dev *pdev, u16 alias, void *data)
>  {
>  	struct its_pci_alias *dev_alias = data;
>  
> -	dev_alias->dev_id = alias;
> +	dev_alias->dev_id = its_pci_requester_id(pdev, alias);
>  	if (pdev != dev_alias->pdev)
>  		dev_alias->count += its_pci_msi_vec_count(dev_alias->pdev);
>  
> diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
> index ffbc034..18e3757 100644
> --- a/include/linux/irqchip/arm-gic-v3.h
> +++ b/include/linux/irqchip/arm-gic-v3.h
> @@ -389,6 +389,8 @@ int its_cpu_init(void);
>  int its_init(struct device_node *node, struct rdists *rdists,
>  	     struct irq_domain *domain);
>  
> +typedef u32 (*its_pci_requester_id_t)(struct pci_dev *, u16);
> +void set_its_pci_requester_id(its_pci_requester_id_t fn);
>  #endif
>  
>  #endif
> 

This has already been proposed, and I already spoke my mind about that
sort of approach (just in case you had any doubt, this is a NAK).

If you have a new fancy kind of aliasing, please extend the capabilities
of pci_for_each_dma_alias or describe it in terms of topological
information that can be parsed.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH 2/5] gic-its: Allow pci_requester_id to be overridden.

[Pavel Fedin]

 Hello!

> -----Original Message-----
> From: linux-pci-owner at vger.kernel.org [mailto:linux-pci-owner at vger.kernel.org] On Behalf Of David
> Daney
> Sent: Wednesday, July 15, 2015 7:55 PM
> To: linux-arm-kernel at lists.infradead.org; Catalin Marinas; Will Deacon; Bjorn Helgaas; linux-
> pci at vger.kernel.org; Thomas Gleixner; Jason Cooper
> Cc: linux-kernel at vger.kernel.org; Robert Richter; David Daney
> Subject: [PATCH 2/5] gic-its: Allow pci_requester_id to be overridden.
> 
> From: David Daney <david.daney@cavium.com>
> 
> Signed-off-by: David Daney <david.daney@cavium.com>
> ---
>  drivers/irqchip/irq-gic-v3-its.c   | 14 +++++++++++++-
>  include/linux/irqchip/arm-gic-v3.h |  2 ++
>  2 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index 1b7e155..4580970 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -1189,11 +1189,23 @@ static int its_pci_msi_vec_count(struct pci_dev *pdev)
>  	return max(msi, msix);
>  }
> 
> +static u32 its_dflt_pci_requester_id(struct pci_dev *pdev, u16 alias)
> +{
> +	return alias;
> +}
> +
> +static its_pci_requester_id_t its_pci_requester_id = its_dflt_pci_requester_id;
> +void set_its_pci_requester_id(its_pci_requester_id_t fn)
> +{
> +	its_pci_requester_id = fn;
> +}
> +EXPORT_SYMBOL(set_its_pci_requester_id);

 IMHO having a globally defined function is a horrible approach. What if some HW has two different
PCI host controllers, each of them wants to provide different form of IDs? I know, this is just
imaginary, but still...
 What if instead of this function we simply add a field to a struct pci_bus ? Then bus driver would
fill in HW-specific ID bits for every bus.
 I can send a patch if interested.

> +
>  static int its_get_pci_alias(struct pci_dev *pdev, u16 alias, void *data)
>  {
>  	struct its_pci_alias *dev_alias = data;
> 
> -	dev_alias->dev_id = alias;
> +	dev_alias->dev_id = its_pci_requester_id(pdev, alias);
>  	if (pdev != dev_alias->pdev)
>  		dev_alias->count += its_pci_msi_vec_count(dev_alias->pdev);
> 
> diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
> index ffbc034..18e3757 100644
> --- a/include/linux/irqchip/arm-gic-v3.h
> +++ b/include/linux/irqchip/arm-gic-v3.h
> @@ -389,6 +389,8 @@ int its_cpu_init(void);
>  int its_init(struct device_node *node, struct rdists *rdists,
>  	     struct irq_domain *domain);
> 
> +typedef u32 (*its_pci_requester_id_t)(struct pci_dev *, u16);
> +void set_its_pci_requester_id(its_pci_requester_id_t fn);
>  #endif
> 
>  #endif
> --

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

[PATCH 2/5] gic-its: Allow pci_requester_id to be overridden.

[David Daney]

On 08/20/2015 07:11 AM, Pavel Fedin wrote:
>
>   Hello!
>
>> -----Original Message-----
>> From: linux-pci-owner at vger.kernel.org [mailto:linux-pci-owner at vger.kernel.org] On Behalf Of David
>> Daney
>> Sent: Wednesday, July 15, 2015 7:55 PM
>> To: linux-arm-kernel at lists.infradead.org; Catalin Marinas; Will Deacon; Bjorn Helgaas; linux-
>> pci at vger.kernel.org; Thomas Gleixner; Jason Cooper
>> Cc: linux-kernel at vger.kernel.org; Robert Richter; David Daney
>> Subject: [PATCH 2/5] gic-its: Allow pci_requester_id to be overridden.
>>
>> From: David Daney <david.daney@cavium.com>
>>
>> Signed-off-by: David Daney <david.daney@cavium.com>
>> ---
>>   drivers/irqchip/irq-gic-v3-its.c   | 14 +++++++++++++-
>>   include/linux/irqchip/arm-gic-v3.h |  2 ++
>>   2 files changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
>> index 1b7e155..4580970 100644
>> --- a/drivers/irqchip/irq-gic-v3-its.c
>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>> @@ -1189,11 +1189,23 @@ static int its_pci_msi_vec_count(struct pci_dev *pdev)
>>   	return max(msi, msix);
>>   }
>>
>> +static u32 its_dflt_pci_requester_id(struct pci_dev *pdev, u16 alias)
>> +{
>> +	return alias;
>> +}
>> +
>> +static its_pci_requester_id_t its_pci_requester_id = its_dflt_pci_requester_id;
>> +void set_its_pci_requester_id(its_pci_requester_id_t fn)
>> +{
>> +	its_pci_requester_id = fn;
>> +}
>> +EXPORT_SYMBOL(set_its_pci_requester_id);
>
>   IMHO having a globally defined function is a horrible approach. What if some HW has two different
> PCI host controllers, each of them wants to provide different form of IDs? I know, this is just
> imaginary, but still...

It is not imaginary, such systems exist.

>   What if instead of this function we simply add a field to a struct pci_bus ? Then bus driver would
> fill in HW-specific ID bits for every bus.
>   I can send a patch if interested.

There is a lot of work being done in the upstream to extract the proper 
information from the device tree.  We are going to piggyback on that for 
the next versions of the patch.

ACPI will have to be made to work as well.

David Daney




>
>> +
>>   static int its_get_pci_alias(struct pci_dev *pdev, u16 alias, void *data)
>>   {
>>   	struct its_pci_alias *dev_alias = data;
>>
>> -	dev_alias->dev_id = alias;
>> +	dev_alias->dev_id = its_pci_requester_id(pdev, alias);
>>   	if (pdev != dev_alias->pdev)
>>   		dev_alias->count += its_pci_msi_vec_count(dev_alias->pdev);
>>
>> diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
>> index ffbc034..18e3757 100644
>> --- a/include/linux/irqchip/arm-gic-v3.h
>> +++ b/include/linux/irqchip/arm-gic-v3.h
>> @@ -389,6 +389,8 @@ int its_cpu_init(void);
>>   int its_init(struct device_node *node, struct rdists *rdists,
>>   	     struct irq_domain *domain);
>>
>> +typedef u32 (*its_pci_requester_id_t)(struct pci_dev *, u16);
>> +void set_its_pci_requester_id(its_pci_requester_id_t fn);
>>   #endif
>>
>>   #endif
>> --
>
> Kind regards,
> Pavel Fedin
> Expert Engineer
> Samsung Electronics Research center Russia
>
>
>

[PATCH 2/5] gic-its: Allow pci_requester_id to be overridden.

[Pavel Fedin]

 Hello!

> >   What if instead of this function we simply add a field to a struct pci_bus ? Then bus driver
would
> > fill in HW-specific ID bits for every bus.
> >   I can send a patch if interested.
> 
> There is a lot of work being done in the upstream to extract the proper
> information from the device tree.  We are going to piggyback on that for
> the next versions of the patch.

 Where can i take a look at the code?

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

[PATCH 3/5] arm64, pci: Allow RC drivers to supply pcibios_add_device() implementation.

[David Daney]

From: David Daney <david.daney@cavium.com>

The default is to continue doing the what we have done before, but add
a hook so that this can be overridden.

Signed-off-by: David Daney <david.daney@cavium.com>
---
 arch/arm64/include/asm/pci.h |  3 +++
 arch/arm64/kernel/pci.c      | 10 ++++++++++
 2 files changed, 13 insertions(+)

diff --git a/arch/arm64/include/asm/pci.h b/arch/arm64/include/asm/pci.h
index b008a72..ad3fb18 100644
--- a/arch/arm64/include/asm/pci.h
+++ b/arch/arm64/include/asm/pci.h
@@ -37,6 +37,9 @@ static inline int pci_proc_domain(struct pci_bus *bus)
 {
 	return 1;
 }
+
+void set_pcibios_add_device(int (*arg)(struct pci_dev *));
+
 #endif  /* CONFIG_PCI */
 
 #endif  /* __KERNEL__ */
diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
index 4095379..3356023 100644
--- a/arch/arm64/kernel/pci.c
+++ b/arch/arm64/kernel/pci.c
@@ -38,11 +38,21 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
 	return res->start;
 }
 
+static int (*pcibios_add_device_impl)(struct pci_dev *);
+
+void set_pcibios_add_device(int (*arg)(struct pci_dev *))
+{
+	pcibios_add_device_impl = arg;
+}
+
 /*
  * Try to assign the IRQ number from DT when adding a new device
  */
 int pcibios_add_device(struct pci_dev *dev)
 {
+	if (pcibios_add_device_impl)
+		return pcibios_add_device_impl(dev);
+
 	dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
 
 	return 0;
-- 
1.9.1

[PATCH 3/5] arm64, pci: Allow RC drivers to supply pcibios_add_device() implementation.

[Lorenzo Pieralisi]

Hi David,

On Wed, Jul 15, 2015 at 05:54:43PM +0100, David Daney wrote:
> From: David Daney <david.daney@cavium.com>
> 
> The default is to continue doing the what we have done before, but add
> a hook so that this can be overridden.

This commit log is inappropriate. On top of that, it is already
complicated enough to keep ARM and ARM64 in sync, I can't even
imagine what would happen if we added a hook to pcibios_add_device
so that RCs can implement their quirks beyond ARM64 core control.

> Signed-off-by: David Daney <david.daney@cavium.com>
> ---
>  arch/arm64/include/asm/pci.h |  3 +++
>  arch/arm64/kernel/pci.c      | 10 ++++++++++
>  2 files changed, 13 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/pci.h b/arch/arm64/include/asm/pci.h
> index b008a72..ad3fb18 100644
> --- a/arch/arm64/include/asm/pci.h
> +++ b/arch/arm64/include/asm/pci.h
> @@ -37,6 +37,9 @@ static inline int pci_proc_domain(struct pci_bus *bus)
>  {
>  	return 1;
>  }
> +
> +void set_pcibios_add_device(int (*arg)(struct pci_dev *));
> +
>  #endif  /* CONFIG_PCI */
>  
>  #endif  /* __KERNEL__ */
> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> index 4095379..3356023 100644
> --- a/arch/arm64/kernel/pci.c
> +++ b/arch/arm64/kernel/pci.c
> @@ -38,11 +38,21 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
>  	return res->start;
>  }
>  
> +static int (*pcibios_add_device_impl)(struct pci_dev *);
> +
> +void set_pcibios_add_device(int (*arg)(struct pci_dev *))
> +{
> +	pcibios_add_device_impl = arg;
> +}
> +
>  /*
>   * Try to assign the IRQ number from DT when adding a new device
>   */
>  int pcibios_add_device(struct pci_dev *dev)
>  {
> +	if (pcibios_add_device_impl)
> +		return pcibios_add_device_impl(dev);

I am totally against this (and to be honest by reading the other
patches I failed to understand why you even need it), see above.

Thanks,
Lorenzo

> +
>  	dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
>  
>  	return 0;
> -- 
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

[PATCH 3/5] arm64, pci: Allow RC drivers to supply pcibios_add_device() implementation.

[David Daney]

On 07/16/2015 02:04 AM, Lorenzo Pieralisi wrote:
> Hi David,
>
> On Wed, Jul 15, 2015 at 05:54:43PM +0100, David Daney wrote:
>> From: David Daney <david.daney@cavium.com>
>>
[...]
>> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
>> index 4095379..3356023 100644
>> --- a/arch/arm64/kernel/pci.c
>> +++ b/arch/arm64/kernel/pci.c
>> @@ -38,11 +38,21 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
>>   	return res->start;
>>   }
>>
>> +static int (*pcibios_add_device_impl)(struct pci_dev *);
>> +
>> +void set_pcibios_add_device(int (*arg)(struct pci_dev *))
>> +{
>> +	pcibios_add_device_impl = arg;
>> +}
>> +
>>   /*
>>    * Try to assign the IRQ number from DT when adding a new device
>>    */
>>   int pcibios_add_device(struct pci_dev *dev)
>>   {
>> +	if (pcibios_add_device_impl)
>> +		return pcibios_add_device_impl(dev);
>
> I am totally against this (and to be honest by reading the other
> patches I failed to understand why you even need it), see above.
>

It is because ...


> Thanks,
> Lorenzo
>
>> +
>>   	dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);


  ... this is total crap.  But I didn't want to break existing systems.

The PCI RC drivers need a way to configure the legacy virtual-wire 
interrupts, because the existing code doesn't do it.

David Daney

[PATCH 3/5] arm64, pci: Allow RC drivers to supply pcibios_add_device() implementation.

[Lorenzo Pieralisi]

Hi David,

On Thu, Jul 16, 2015 at 06:00:36PM +0100, David Daney wrote:
> On 07/16/2015 02:04 AM, Lorenzo Pieralisi wrote:
> > Hi David,
> >
> > On Wed, Jul 15, 2015 at 05:54:43PM +0100, David Daney wrote:
> >> From: David Daney <david.daney@cavium.com>
> >>
> [...]
> >> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> >> index 4095379..3356023 100644
> >> --- a/arch/arm64/kernel/pci.c
> >> +++ b/arch/arm64/kernel/pci.c
> >> @@ -38,11 +38,21 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
> >>   	return res->start;
> >>   }
> >>
> >> +static int (*pcibios_add_device_impl)(struct pci_dev *);
> >> +
> >> +void set_pcibios_add_device(int (*arg)(struct pci_dev *))
> >> +{
> >> +	pcibios_add_device_impl = arg;
> >> +}
> >> +
> >>   /*
> >>    * Try to assign the IRQ number from DT when adding a new device
> >>    */
> >>   int pcibios_add_device(struct pci_dev *dev)
> >>   {
> >> +	if (pcibios_add_device_impl)
> >> +		return pcibios_add_device_impl(dev);
> >
> > I am totally against this (and to be honest by reading the other
> > patches I failed to understand why you even need it), see above.
> >
> 
> It is because ...
> 
> 
> > Thanks,
> > Lorenzo
> >
> >> +
> >>   	dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
> 
> 
>   ... this is total crap.  But I didn't want to break existing systems.

That's a good aim, but you are still failing to explain the issue properly
I am afraid.

> The PCI RC drivers need a way to configure the legacy virtual-wire 
> interrupts, because the existing code doesn't do it.

Can I ask you please to explain the issue a bit more clearly (and why
the OF API does not work for you ?)

Thank you !
Lorenzo

[PATCH 3/5] arm64, pci: Allow RC drivers to supply pcibios_add_device() implementation.

[David Daney]

On 07/17/2015 04:00 AM, Lorenzo Pieralisi wrote:
> Hi David,
>
> On Thu, Jul 16, 2015 at 06:00:36PM +0100, David Daney wrote:
>> On 07/16/2015 02:04 AM, Lorenzo Pieralisi wrote:
>>> Hi David,
>>>
>>> On Wed, Jul 15, 2015 at 05:54:43PM +0100, David Daney wrote:
>>>> From: David Daney <david.daney@cavium.com>
>>>>
>> [...]
>>>> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
>>>> index 4095379..3356023 100644
>>>> --- a/arch/arm64/kernel/pci.c
>>>> +++ b/arch/arm64/kernel/pci.c
>>>> @@ -38,11 +38,21 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
>>>>    	return res->start;
>>>>    }
>>>>
>>>> +static int (*pcibios_add_device_impl)(struct pci_dev *);
>>>> +
>>>> +void set_pcibios_add_device(int (*arg)(struct pci_dev *))
>>>> +{
>>>> +	pcibios_add_device_impl = arg;
>>>> +}
>>>> +
>>>>    /*
>>>>     * Try to assign the IRQ number from DT when adding a new device
>>>>     */
>>>>    int pcibios_add_device(struct pci_dev *dev)
>>>>    {
>>>> +	if (pcibios_add_device_impl)
>>>> +		return pcibios_add_device_impl(dev);
>>>
>>> I am totally against this (and to be honest by reading the other
>>> patches I failed to understand why you even need it), see above.
>>>
>>
>> It is because ...
>>
>>
>>> Thanks,
>>> Lorenzo
>>>
>>>> +
>>>>    	dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
>>
>>
>>    ... this is total crap.  But I didn't want to break existing systems.
>
> That's a good aim, but you are still failing to explain the issue properly
> I am afraid.
>
>> The PCI RC drivers need a way to configure the legacy virtual-wire
>> interrupts, because the existing code doesn't do it.
>
> Can I ask you please to explain the issue a bit more clearly (and why
> the OF API does not work for you ?)

Several problems:

1) It prints many times to the boot log this string:
     pci 0000:01:0e.2: of_irq_parse_pci() failed with rc=-19

2) For a RC with no device_node it does nothing (in addition to printing 
the annoying message).


>
> Thank you !
> Lorenzo
>

[PATCH 3/5] arm64, pci: Allow RC drivers to supply pcibios_add_device() implementation.

[Mark Rutland]

On Fri, Jul 17, 2015 at 05:38:45PM +0100, David Daney wrote:
> On 07/17/2015 04:00 AM, Lorenzo Pieralisi wrote:
> > Hi David,
> >
> > On Thu, Jul 16, 2015 at 06:00:36PM +0100, David Daney wrote:
> >> On 07/16/2015 02:04 AM, Lorenzo Pieralisi wrote:
> >>> Hi David,
> >>>
> >>> On Wed, Jul 15, 2015 at 05:54:43PM +0100, David Daney wrote:
> >>>> From: David Daney <david.daney@cavium.com>
> >>>>
> >> [...]
> >>>> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> >>>> index 4095379..3356023 100644
> >>>> --- a/arch/arm64/kernel/pci.c
> >>>> +++ b/arch/arm64/kernel/pci.c
> >>>> @@ -38,11 +38,21 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
> >>>>    	return res->start;
> >>>>    }
> >>>>
> >>>> +static int (*pcibios_add_device_impl)(struct pci_dev *);
> >>>> +
> >>>> +void set_pcibios_add_device(int (*arg)(struct pci_dev *))
> >>>> +{
> >>>> +	pcibios_add_device_impl = arg;
> >>>> +}
> >>>> +
> >>>>    /*
> >>>>     * Try to assign the IRQ number from DT when adding a new device
> >>>>     */
> >>>>    int pcibios_add_device(struct pci_dev *dev)
> >>>>    {
> >>>> +	if (pcibios_add_device_impl)
> >>>> +		return pcibios_add_device_impl(dev);
> >>>
> >>> I am totally against this (and to be honest by reading the other
> >>> patches I failed to understand why you even need it), see above.
> >>>
> >>
> >> It is because ...
> >>
> >>
> >>> Thanks,
> >>> Lorenzo
> >>>
> >>>> +
> >>>>    	dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
> >>
> >>
> >>    ... this is total crap.  But I didn't want to break existing systems.
> >
> > That's a good aim, but you are still failing to explain the issue properly
> > I am afraid.
> >
> >> The PCI RC drivers need a way to configure the legacy virtual-wire
> >> interrupts, because the existing code doesn't do it.
> >
> > Can I ask you please to explain the issue a bit more clearly (and why
> > the OF API does not work for you ?)
> 
> Several problems:
> 
> 1) It prints many times to the boot log this string:
>      pci 0000:01:0e.2: of_irq_parse_pci() failed with rc=-19

which happens because...?

That's -ENODEV. As far as I can tell, that only gets printed if the
device doesn't have a wired interrupt (the interrupt pin field in config
space read as zero).

So that's beningn, but annoying?

Surely we can clean that up.

> 2) For a RC with no device_node it does nothing (in addition to printing 
> the annoying message).

Your root complex has a device_node if it is described in DT.

Is the problem that this doesn't work with ACPI?

Thanks,
Mark.

[PATCH 4/5] irqchip: gic-v3: Add gic_get_irq_domain() to get the irqdomain of the GIC.

[David Daney]

From: David Daney <david.daney@cavium.com>

Needed to map SPI interrupt sources.

Signed-off-by: David Daney <david.daney@cavium.com>
---
 drivers/irqchip/irq-gic-v3.c       | 5 +++++
 include/linux/irqchip/arm-gic-v3.h | 1 +
 2 files changed, 6 insertions(+)

diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index c52f7ba..0019fed 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -58,6 +58,11 @@ static struct gic_chip_data gic_data __read_mostly;
 /* Our default, arbitrary priority value. Linux only uses one anyway. */
 #define DEFAULT_PMR_VALUE	0xf0
 
+struct irq_domain *gic_get_irq_domain(void)
+{
+	return gic_data.domain;
+}
+
 static inline unsigned int gic_irq(struct irq_data *d)
 {
 	return d->hwirq;
diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
index 18e3757..5992224 100644
--- a/include/linux/irqchip/arm-gic-v3.h
+++ b/include/linux/irqchip/arm-gic-v3.h
@@ -391,6 +391,7 @@ int its_init(struct device_node *node, struct rdists *rdists,
 
 typedef u32 (*its_pci_requester_id_t)(struct pci_dev *, u16);
 void set_its_pci_requester_id(its_pci_requester_id_t fn);
+struct irq_domain *gic_get_irq_domain(void);
 #endif
 
 #endif
-- 
1.9.1

[PATCH 4/5] irqchip: gic-v3: Add gic_get_irq_domain() to get the irqdomain of the GIC.

[Marc Zyngier]

On 15/07/15 17:54, David Daney wrote:
> From: David Daney <david.daney@cavium.com>
> 
> Needed to map SPI interrupt sources.
> 
> Signed-off-by: David Daney <david.daney@cavium.com>
> ---
>  drivers/irqchip/irq-gic-v3.c       | 5 +++++
>  include/linux/irqchip/arm-gic-v3.h | 1 +
>  2 files changed, 6 insertions(+)
> 
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index c52f7ba..0019fed 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -58,6 +58,11 @@ static struct gic_chip_data gic_data __read_mostly;
>  /* Our default, arbitrary priority value. Linux only uses one anyway. */
>  #define DEFAULT_PMR_VALUE	0xf0
>  
> +struct irq_domain *gic_get_irq_domain(void)
> +{
> +	return gic_data.domain;
> +}
> +
>  static inline unsigned int gic_irq(struct irq_data *d)
>  {
>  	return d->hwirq;
> diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
> index 18e3757..5992224 100644
> --- a/include/linux/irqchip/arm-gic-v3.h
> +++ b/include/linux/irqchip/arm-gic-v3.h
> @@ -391,6 +391,7 @@ int its_init(struct device_node *node, struct rdists *rdists,
>  
>  typedef u32 (*its_pci_requester_id_t)(struct pci_dev *, u16);
>  void set_its_pci_requester_id(its_pci_requester_id_t fn);
> +struct irq_domain *gic_get_irq_domain(void);
>  #endif
>  
>  #endif
> 

Hmmmffff... You need the domain for SPIs??

What is wrong with putting these interrupts in your device tree?

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH 4/5] irqchip: gic-v3: Add gic_get_irq_domain() to get the irqdomain of the GIC.

[David Daney]

On 07/15/2015 10:12 AM, Marc Zyngier wrote:
> On 15/07/15 17:54, David Daney wrote:
>> From: David Daney <david.daney@cavium.com>
>>
>> Needed to map SPI interrupt sources.
>>
>> Signed-off-by: David Daney <david.daney@cavium.com>
>> ---
>>   drivers/irqchip/irq-gic-v3.c       | 5 +++++
>>   include/linux/irqchip/arm-gic-v3.h | 1 +
>>   2 files changed, 6 insertions(+)
>>
>> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
>> index c52f7ba..0019fed 100644
>> --- a/drivers/irqchip/irq-gic-v3.c
>> +++ b/drivers/irqchip/irq-gic-v3.c
>> @@ -58,6 +58,11 @@ static struct gic_chip_data gic_data __read_mostly;
>>   /* Our default, arbitrary priority value. Linux only uses one anyway. */
>>   #define DEFAULT_PMR_VALUE	0xf0
>>
>> +struct irq_domain *gic_get_irq_domain(void)
>> +{
>> +	return gic_data.domain;
>> +}
>> +
>>   static inline unsigned int gic_irq(struct irq_data *d)
>>   {
>>   	return d->hwirq;
>> diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
>> index 18e3757..5992224 100644
>> --- a/include/linux/irqchip/arm-gic-v3.h
>> +++ b/include/linux/irqchip/arm-gic-v3.h
>> @@ -391,6 +391,7 @@ int its_init(struct device_node *node, struct rdists *rdists,
>>
>>   typedef u32 (*its_pci_requester_id_t)(struct pci_dev *, u16);
>>   void set_its_pci_requester_id(its_pci_requester_id_t fn);
>> +struct irq_domain *gic_get_irq_domain(void);
>>   #endif
>>
>>   #endif
>>
>
> Hmmmffff... You need the domain for SPIs??
>
> What is wrong with putting these interrupts in your device tree?
>

There is no device tree node for ECAM based "PCIe" devices, they are 
discovered in the PCI bus scan, yet they still need to use SPI interrupts.

We need a way to be able to map these.

David Daney



> 	M.
>

[PATCH 4/5] irqchip: gic-v3: Add gic_get_irq_domain() to get the irqdomain of the GIC.

[Marc Zyngier]

On 15/07/15 19:57, David Daney wrote:
> On 07/15/2015 10:12 AM, Marc Zyngier wrote:
>> On 15/07/15 17:54, David Daney wrote:
>>> From: David Daney <david.daney@cavium.com>
>>>
>>> Needed to map SPI interrupt sources.
>>>
>>> Signed-off-by: David Daney <david.daney@cavium.com>
>>> ---
>>>   drivers/irqchip/irq-gic-v3.c       | 5 +++++
>>>   include/linux/irqchip/arm-gic-v3.h | 1 +
>>>   2 files changed, 6 insertions(+)
>>>
>>> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
>>> index c52f7ba..0019fed 100644
>>> --- a/drivers/irqchip/irq-gic-v3.c
>>> +++ b/drivers/irqchip/irq-gic-v3.c
>>> @@ -58,6 +58,11 @@ static struct gic_chip_data gic_data __read_mostly;
>>>   /* Our default, arbitrary priority value. Linux only uses one anyway. */
>>>   #define DEFAULT_PMR_VALUE	0xf0
>>>
>>> +struct irq_domain *gic_get_irq_domain(void)
>>> +{
>>> +	return gic_data.domain;
>>> +}
>>> +
>>>   static inline unsigned int gic_irq(struct irq_data *d)
>>>   {
>>>   	return d->hwirq;
>>> diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
>>> index 18e3757..5992224 100644
>>> --- a/include/linux/irqchip/arm-gic-v3.h
>>> +++ b/include/linux/irqchip/arm-gic-v3.h
>>> @@ -391,6 +391,7 @@ int its_init(struct device_node *node, struct rdists *rdists,
>>>
>>>   typedef u32 (*its_pci_requester_id_t)(struct pci_dev *, u16);
>>>   void set_its_pci_requester_id(its_pci_requester_id_t fn);
>>> +struct irq_domain *gic_get_irq_domain(void);
>>>   #endif
>>>
>>>   #endif
>>>
>>
>> Hmmmffff... You need the domain for SPIs??
>>
>> What is wrong with putting these interrupts in your device tree?
>>
> 
> There is no device tree node for ECAM based "PCIe" devices, they are 
> discovered in the PCI bus scan, yet they still need to use SPI interrupts.

What is different between these and the normal legacy interrupts that
are normally expressed in the RC node using an interrupt map? Sorry if
I'm only showing my ignorance here, but I'd like to understand the exact
nature of the problem.

> We need a way to be able to map these.

However you're going to map them, it will not be by just blindly
exporting random irqdomains from an unsuspecting interrupt controller.

Patch 5 has established that you're using "virtual wire" SPIs, so we
need to work on exposing that with the normal kernel abstraction, and
not by messing with the internals of the GIC.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH 4/5] irqchip: gic-v3: Add gic_get_irq_domain() to get the irqdomain of the GIC.

[David Daney]

On 07/16/2015 12:38 AM, Marc Zyngier wrote:
> On 15/07/15 19:57, David Daney wrote:
>> On 07/15/2015 10:12 AM, Marc Zyngier wrote:
>>> On 15/07/15 17:54, David Daney wrote:
>>>> From: David Daney <david.daney@cavium.com>
>>>>
>>>> Needed to map SPI interrupt sources.
>>>>
>>>> Signed-off-by: David Daney <david.daney@cavium.com>
>>>> ---
>>>>    drivers/irqchip/irq-gic-v3.c       | 5 +++++
>>>>    include/linux/irqchip/arm-gic-v3.h | 1 +
>>>>    2 files changed, 6 insertions(+)
>>>>
>>>> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
>>>> index c52f7ba..0019fed 100644
>>>> --- a/drivers/irqchip/irq-gic-v3.c
>>>> +++ b/drivers/irqchip/irq-gic-v3.c
>>>> @@ -58,6 +58,11 @@ static struct gic_chip_data gic_data __read_mostly;
>>>>    /* Our default, arbitrary priority value. Linux only uses one anyway. */
>>>>    #define DEFAULT_PMR_VALUE	0xf0
>>>>
>>>> +struct irq_domain *gic_get_irq_domain(void)
>>>> +{
>>>> +	return gic_data.domain;
>>>> +}
>>>> +
>>>>    static inline unsigned int gic_irq(struct irq_data *d)
>>>>    {
>>>>    	return d->hwirq;
>>>> diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
>>>> index 18e3757..5992224 100644
>>>> --- a/include/linux/irqchip/arm-gic-v3.h
>>>> +++ b/include/linux/irqchip/arm-gic-v3.h
>>>> @@ -391,6 +391,7 @@ int its_init(struct device_node *node, struct rdists *rdists,
>>>>
>>>>    typedef u32 (*its_pci_requester_id_t)(struct pci_dev *, u16);
>>>>    void set_its_pci_requester_id(its_pci_requester_id_t fn);
>>>> +struct irq_domain *gic_get_irq_domain(void);
>>>>    #endif
[...]
>
>> We need a way to be able to map these.
>
> However you're going to map them, it will not be by just blindly
> exporting random irqdomains from an unsuspecting interrupt controller.

There is nothing random about it.  The ARM architects specified that 
there is exactly One True GIC in a system.  If we need to do something 
with the GIC, it is not a "random ... unsuspecting interrupt 
controller", it is *The* GIC.

>
> Patch 5 has established that you're using "virtual wire" SPIs, so we
> need to work on exposing that with the normal kernel abstraction, and
> not by messing with the internals of the GIC.
>

Agreed.

The MSI system has pci_enable_msix()/pci_disable_msix().

I would propose something like:

struct gic_spi_entry {
	int spi   /* SPI number */
	int irq;  /* kernel irq number mapped to the spi*/
	u32 msg;  /* message to be written */
	u64 assert_addr;
	u64 deassert_addr;
};

/* Fill in the SPI processing information */
int gic_map_spi(int spi, struct gic_spi_entry *data);

David Daney



> Thanks,
>
> 	M.
>

[PATCH 4/5] irqchip: gic-v3: Add gic_get_irq_domain() to get the irqdomain of the GIC.

[Marc Zyngier]

On 16/07/15 17:50, David Daney wrote:
> On 07/16/2015 12:38 AM, Marc Zyngier wrote:
>> On 15/07/15 19:57, David Daney wrote:
>>> On 07/15/2015 10:12 AM, Marc Zyngier wrote:
>>>> On 15/07/15 17:54, David Daney wrote:
>>>>> From: David Daney <david.daney@cavium.com>
>>>>>
>>>>> Needed to map SPI interrupt sources.
>>>>>
>>>>> Signed-off-by: David Daney <david.daney@cavium.com>
>>>>> ---
>>>>>    drivers/irqchip/irq-gic-v3.c       | 5 +++++
>>>>>    include/linux/irqchip/arm-gic-v3.h | 1 +
>>>>>    2 files changed, 6 insertions(+)
>>>>>
>>>>> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
>>>>> index c52f7ba..0019fed 100644
>>>>> --- a/drivers/irqchip/irq-gic-v3.c
>>>>> +++ b/drivers/irqchip/irq-gic-v3.c
>>>>> @@ -58,6 +58,11 @@ static struct gic_chip_data gic_data __read_mostly;
>>>>>    /* Our default, arbitrary priority value. Linux only uses one anyway. */
>>>>>    #define DEFAULT_PMR_VALUE	0xf0
>>>>>
>>>>> +struct irq_domain *gic_get_irq_domain(void)
>>>>> +{
>>>>> +	return gic_data.domain;
>>>>> +}
>>>>> +
>>>>>    static inline unsigned int gic_irq(struct irq_data *d)
>>>>>    {
>>>>>    	return d->hwirq;
>>>>> diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
>>>>> index 18e3757..5992224 100644
>>>>> --- a/include/linux/irqchip/arm-gic-v3.h
>>>>> +++ b/include/linux/irqchip/arm-gic-v3.h
>>>>> @@ -391,6 +391,7 @@ int its_init(struct device_node *node, struct rdists *rdists,
>>>>>
>>>>>    typedef u32 (*its_pci_requester_id_t)(struct pci_dev *, u16);
>>>>>    void set_its_pci_requester_id(its_pci_requester_id_t fn);
>>>>> +struct irq_domain *gic_get_irq_domain(void);
>>>>>    #endif
> [...]
>>
>>> We need a way to be able to map these.
>>
>> However you're going to map them, it will not be by just blindly
>> exporting random irqdomains from an unsuspecting interrupt controller.
> 
> There is nothing random about it.  The ARM architects specified that 
> there is exactly One True GIC in a system.  If we need to do something 
> with the GIC, it is not a "random ... unsuspecting interrupt 
> controller", it is *The* GIC.

Indeed. And The One True GIC Driver will not be butchered just because
you can, thank you very much.

>>
>> Patch 5 has established that you're using "virtual wire" SPIs, so we
>> need to work on exposing that with the normal kernel abstraction, and
>> not by messing with the internals of the GIC.
>>
> 
> Agreed.
> 
> The MSI system has pci_enable_msix()/pci_disable_msix().
> 
> I would propose something like:
> 
> struct gic_spi_entry {
> 	int spi   /* SPI number */
> 	int irq;  /* kernel irq number mapped to the spi*/
> 	u32 msg;  /* message to be written */
> 	u64 assert_addr;
> 	u64 deassert_addr;
> };
> 
> /* Fill in the SPI processing information */
> int gic_map_spi(int spi, struct gic_spi_entry *data);

Neither.

The way to do it is to make this a *separate* IRQ domain stacked onto
the SPI domain. No funky hook on the side. If it doesn't go through the
normal kernel API, it doesn't reach the GIC.

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH 4/5] irqchip: gic-v3: Add gic_get_irq_domain() to get the irqdomain of the GIC.

[David Daney]

On 07/16/2015 10:09 AM, Marc Zyngier wrote:
> On 16/07/15 17:50, David Daney wrote:
[...]
>>> Patch 5 has established that you're using "virtual wire" SPIs, so we
>>> need to work on exposing that with the normal kernel abstraction, and
>>> not by messing with the internals of the GIC.
>>>
>>
>> Agreed.
>>
>> The MSI system has pci_enable_msix()/pci_disable_msix().
>>
>> I would propose something like:
>>
>> struct gic_spi_entry {
>> 	int spi   /* SPI number */
>> 	int irq;  /* kernel irq number mapped to the spi*/
>> 	u32 msg;  /* message to be written */
>> 	u64 assert_addr;
>> 	u64 deassert_addr;
>> };
>>
>> /* Fill in the SPI processing information */
>> int gic_map_spi(int spi, struct gic_spi_entry *data);
>
> Neither.
>
> The way to do it is to make this a *separate* IRQ domain stacked onto
> the SPI domain. No funky hook on the side. If it doesn't go through the
> normal kernel API, it doesn't reach the GIC.

Yes, the irqdomain does handle mapping SPI -> irq, and the message can 
be derived from the SPI.  However, the irqdomain infrastructure cannot 
supply values for either assert_addr or deassert_addr.

Those are needed in order to use SPI.  How would you suggest that they 
be obtained?


David Daney


>
> 	M.
>

[PATCH 4/5] irqchip: gic-v3: Add gic_get_irq_domain() to get the irqdomain of the GIC.

[Marc Zyngier]

On 16/07/15 18:14, David Daney wrote:
> On 07/16/2015 10:09 AM, Marc Zyngier wrote:
>> On 16/07/15 17:50, David Daney wrote:
> [...]
>>>> Patch 5 has established that you're using "virtual wire" SPIs, so we
>>>> need to work on exposing that with the normal kernel abstraction, and
>>>> not by messing with the internals of the GIC.
>>>>
>>>
>>> Agreed.
>>>
>>> The MSI system has pci_enable_msix()/pci_disable_msix().
>>>
>>> I would propose something like:
>>>
>>> struct gic_spi_entry {
>>> 	int spi   /* SPI number */
>>> 	int irq;  /* kernel irq number mapped to the spi*/
>>> 	u32 msg;  /* message to be written */
>>> 	u64 assert_addr;
>>> 	u64 deassert_addr;
>>> };
>>>
>>> /* Fill in the SPI processing information */
>>> int gic_map_spi(int spi, struct gic_spi_entry *data);
>>
>> Neither.
>>
>> The way to do it is to make this a *separate* IRQ domain stacked onto
>> the SPI domain. No funky hook on the side. If it doesn't go through the
>> normal kernel API, it doesn't reach the GIC.
> 
> Yes, the irqdomain does handle mapping SPI -> irq, and the message can 
> be derived from the SPI.  However, the irqdomain infrastructure cannot 
> supply values for either assert_addr or deassert_addr.

This is why I suggested earlier (in my reply to patch 5) that you have a
look at the series I posted a couple of days ago to implement non-PCI
MSI support. This would allow you to compose the domains as such:

platform-MSI -> message-SPI -> GIC

You'd end up with a msi_msg containing the GICD_SETSPI_NSR doorbell, and
the SPI as a payload.

> Those are needed in order to use SPI.  How would you suggest that they 
> be obtained?

Two possibilities: either you derive GICD_CLRSPI_NSR by adding 8 to the
doorbell you got from the msi_msg structure (ugly, but limited to your
own code), or you extend msi_msg to cater for this case.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH 4/5] irqchip: gic-v3: Add gic_get_irq_domain() to get the irqdomain of the GIC.

[Marc Zyngier]

On Thu, 16 Jul 2015 18:32:28 +0100
Marc Zyngier <marc.zyngier@arm.com> wrote:

> On 16/07/15 18:14, David Daney wrote:
> > On 07/16/2015 10:09 AM, Marc Zyngier wrote:
> >> On 16/07/15 17:50, David Daney wrote:
> > [...]
> >>>> Patch 5 has established that you're using "virtual wire" SPIs, so we
> >>>> need to work on exposing that with the normal kernel abstraction, and
> >>>> not by messing with the internals of the GIC.
> >>>>
> >>>
> >>> Agreed.
> >>>
> >>> The MSI system has pci_enable_msix()/pci_disable_msix().
> >>>
> >>> I would propose something like:
> >>>
> >>> struct gic_spi_entry {
> >>> 	int spi   /* SPI number */
> >>> 	int irq;  /* kernel irq number mapped to the spi*/
> >>> 	u32 msg;  /* message to be written */
> >>> 	u64 assert_addr;
> >>> 	u64 deassert_addr;
> >>> };
> >>>
> >>> /* Fill in the SPI processing information */
> >>> int gic_map_spi(int spi, struct gic_spi_entry *data);
> >>
> >> Neither.
> >>
> >> The way to do it is to make this a *separate* IRQ domain stacked onto
> >> the SPI domain. No funky hook on the side. If it doesn't go through the
> >> normal kernel API, it doesn't reach the GIC.
> > 
> > Yes, the irqdomain does handle mapping SPI -> irq, and the message can 
> > be derived from the SPI.  However, the irqdomain infrastructure cannot 
> > supply values for either assert_addr or deassert_addr.
> 
> This is why I suggested earlier (in my reply to patch 5) that you have a
> look at the series I posted a couple of days ago to implement non-PCI
> MSI support. This would allow you to compose the domains as such:
> 
> platform-MSI -> message-SPI -> GIC
> 
> You'd end up with a msi_msg containing the GICD_SETSPI_NSR doorbell, and
> the SPI as a payload.
> 
> > Those are needed in order to use SPI.  How would you suggest that they 
> > be obtained?
> 
> Two possibilities: either you derive GICD_CLRSPI_NSR by adding 8 to the
> doorbell you got from the msi_msg structure (ugly, but limited to your
> own code), or you extend msi_msg to cater for this case.

A simpler alternative to the above would be to move all this to
firmware, and only expose a *wired* SPI to the kernel. This would have
the advantage to use the existing infrastructure for both DT and ACPI.

As a side note, that was the intended use of these registers - to
provide a "virtual wire" interface that remains mostly invisible to
software.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny.

[PATCH 5/5] PCI: Add host drivers for Cavium ThunderX processors.

[David Daney]

From: David Daney <david.daney@cavium.com>

Signed-off-by: David Daney <david.daney@cavium.com>
---
 drivers/pci/host/Kconfig            |  12 +
 drivers/pci/host/Makefile           |   2 +
 drivers/pci/host/pcie-thunder-pem.c | 462 ++++++++++++++++++++++++++++++++++++
 drivers/pci/host/pcie-thunder.c     | 422 ++++++++++++++++++++++++++++++++
 4 files changed, 898 insertions(+)
 create mode 100644 drivers/pci/host/pcie-thunder-pem.c
 create mode 100644 drivers/pci/host/pcie-thunder.c

diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
index c132bdd..06e26ad 100644
--- a/drivers/pci/host/Kconfig
+++ b/drivers/pci/host/Kconfig
@@ -145,4 +145,16 @@ config PCIE_IPROC_BCMA
 	  Say Y here if you want to use the Broadcom iProc PCIe controller
 	  through the BCMA bus interface
 
+config PCI_THUNDER_PEM
+	bool
+
+config PCI_THUNDER
+	bool "Thunder PCIe host controller"
+	depends on ARM64 || COMPILE_TEST
+	depends on OF_PCI
+	select PCI_MSI
+	select PCI_THUNDER_PEM
+	help
+	  Say Y here if you want internal PCI support on Thunder SoC.
+
 endmenu
diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
index 140d66f..a355155 100644
--- a/drivers/pci/host/Makefile
+++ b/drivers/pci/host/Makefile
@@ -17,3 +17,5 @@ obj-$(CONFIG_PCI_VERSATILE) += pci-versatile.o
 obj-$(CONFIG_PCIE_IPROC) += pcie-iproc.o
 obj-$(CONFIG_PCIE_IPROC_PLATFORM) += pcie-iproc-platform.o
 obj-$(CONFIG_PCIE_IPROC_BCMA) += pcie-iproc-bcma.o
+obj-$(CONFIG_PCI_THUNDER) += pcie-thunder.o
+obj-$(CONFIG_PCI_THUNDER_PEM) += pcie-thunder-pem.o
diff --git a/drivers/pci/host/pcie-thunder-pem.c b/drivers/pci/host/pcie-thunder-pem.c
new file mode 100644
index 0000000..7861a8a
--- /dev/null
+++ b/drivers/pci/host/pcie-thunder-pem.c
@@ -0,0 +1,462 @@
+/*
+ * PCIe host controller driver for Cavium Thunder SOC
+ *
+ * Copyright (C) 2014,2015 Cavium Inc.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ */
+/* #define DEBUG 1 */
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/delay.h>
+#include <linux/irq.h>
+#include <linux/pci.h>
+#include <linux/irqdomain.h>
+#include <linux/msi.h>
+#include <linux/irqchip/arm-gic-v3.h>
+
+#define THUNDER_SLI_S2M_REG_ACC_BASE	0x874001000000ull
+
+#define THUNDER_GIC			0x801000000000ull
+#define THUNDER_GICD_SETSPI_NSR		0x801000000040ull
+#define THUNDER_GICD_CLRSPI_NSR		0x801000000048ull
+
+#define THUNDER_GSER_PCIE_MASK		0x01
+
+#define PEM_CTL_STATUS	0x000
+#define PEM_RD_CFG	0x030
+#define P2N_BAR0_START	0x080
+#define P2N_BAR1_START	0x088
+#define P2N_BAR2_START	0x090
+#define BAR_CTL		0x0a8
+#define BAR2_MASK	0x0b0
+#define BAR1_INDEX	0x100
+#define PEM_CFG		0x410
+#define PEM_ON		0x420
+
+struct thunder_pem {
+	struct list_head list; /* on thunder_pem_buses */
+	bool		connected;
+	unsigned int	id;
+	unsigned int	sli;
+	unsigned int	sli_group;
+	unsigned int	node;
+	u64		sli_window_base;
+	void __iomem	*bar0;
+	void __iomem	*bar4;
+	void __iomem	*sli_s2m;
+	void __iomem	*cfgregion;
+	struct pci_bus	*bus;
+	int		vwire_irqs[4];
+	u32		vwire_data[4];
+};
+
+static LIST_HEAD(thunder_pem_buses);
+
+static struct pci_device_id thunder_pem_pci_table[] = {
+	{PCI_VENDOR_ID_CAVIUM, 0xa020, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0},
+	{0,}
+};
+MODULE_DEVICE_TABLE(pci, thunder_pem_pci_table);
+
+enum slix_s2m_ctype {
+	CTYPE_MEMORY	= 0,
+	CTYPE_CONFIG	= 1,
+	CTYPE_IO	= 2
+};
+
+static u64 slix_s2m_reg_val(unsigned mac, enum slix_s2m_ctype ctype,
+			    bool merge, bool relaxed, bool snoop, u32 ba_msb)
+{
+	u64 v;
+
+	v = (u64)(mac % 3) << 49;
+	v |= (u64)ctype << 53;
+	if (!merge)
+		v |= 1ull << 48;
+	if (relaxed)
+		v |= 5ull << 40;
+	if (!snoop)
+		v |= 5ull << 41;
+	v |= (u64)ba_msb;
+
+	return v;
+}
+
+static u32 thunder_pcierc_config_read(struct thunder_pem *pem, u32 reg, int size)
+{
+	unsigned int val;
+
+	writeq(reg & ~3u, pem->bar0 + PEM_RD_CFG);
+	val = readq(pem->bar0 + PEM_RD_CFG) >> 32;
+
+	if (size == 1)
+		val = (val >> (8 * (reg & 3))) & 0xff;
+	else if (size == 2)
+		val = (val >> (8 * (reg & 3))) & 0xffff;
+
+	return val;
+}
+
+static int thunder_pem_read_config(struct pci_bus *bus, unsigned int devfn,
+				   int reg, int size, u32 *val)
+{
+	void __iomem *addr;
+	struct thunder_pem *pem = bus->sysdata;
+	unsigned int busnr = bus->number;
+
+	if (busnr > 255 || devfn > 255 || reg > 4095)
+		return PCIBIOS_DEVICE_NOT_FOUND;
+
+	addr = pem->cfgregion + ((busnr << 24)  | (devfn << 16) | reg);
+
+	switch (size) {
+	case 1:
+		*val = readb(addr);
+		break;
+	case 2:
+		*val = readw(addr);
+		break;
+	case 4:
+		*val = readl(addr);
+		break;
+	default:
+		return PCIBIOS_BAD_REGISTER_NUMBER;
+	}
+
+	return PCIBIOS_SUCCESSFUL;
+}
+
+static int thunder_pem_write_config(struct pci_bus *bus, unsigned int devfn,
+				    int reg, int size, u32 val)
+{
+	void __iomem *addr;
+	struct thunder_pem *pem = bus->sysdata;
+	unsigned int busnr = bus->number;
+
+	if (busnr > 255 || devfn > 255 || reg > 4095)
+		return PCIBIOS_DEVICE_NOT_FOUND;
+
+	addr = pem->cfgregion + ((busnr << 24)  | (devfn << 16) | reg);
+
+	switch (size) {
+	case 1:
+		writeb(val, addr);
+		break;
+	case 2:
+		writew(val, addr);
+		break;
+	case 4:
+		writel(val, addr);
+		break;
+	default:
+		return PCIBIOS_BAD_REGISTER_NUMBER;
+	}
+
+	return PCIBIOS_SUCCESSFUL;
+}
+
+static struct pci_ops thunder_pem_ops = {
+	.read	= thunder_pem_read_config,
+	.write	= thunder_pem_write_config,
+};
+
+static struct thunder_pem *thunder_pem_from_dev(struct pci_dev *dev)
+{
+	struct thunder_pem *pem;
+	struct pci_bus *bus = dev->bus;
+
+	while (!pci_is_root_bus(bus))
+		bus = bus->parent;
+
+	list_for_each_entry(pem, &thunder_pem_buses, list) {
+		if (pem->bus == bus)
+			return pem;
+	}
+	return NULL;
+}
+
+int thunder_pem_requester_id(struct pci_dev *dev)
+{
+	struct thunder_pem *pem = thunder_pem_from_dev(dev);
+
+	if (!pem)
+		return -ENODEV;
+
+	if (pem->id < 3)
+		return ((1 << 16) |
+			((dev)->bus->number << 8) |
+			(dev)->devfn);
+
+	if (pem->id < 6)
+		return ((3 << 16) |
+			((dev)->bus->number << 8) |
+			(dev)->devfn);
+
+	if (pem->id < 9)
+		return ((1 << 19) | (1 << 16) |
+			((dev)->bus->number << 8) |
+			(dev)->devfn);
+
+	if (pem->id < 12)
+		return ((1 << 19) |
+			(3 << 16) |
+			((dev)->bus->number << 8) |
+			(dev)->devfn);
+	return -ENODEV;
+}
+
+static int thunder_pem_pcibios_add_device(struct pci_dev *dev)
+{
+	struct thunder_pem *pem;
+	u8 pin;
+
+	pem = thunder_pem_from_dev(dev);
+	if (!pem)
+		return 0;
+
+	pci_read_config_byte(dev, PCI_INTERRUPT_PIN, &pin);
+
+	/* Cope with illegal. */
+	if (pin > 4)
+		pin = 1;
+
+	dev->irq = pin > 0 ? pem->vwire_irqs[pin - 1] : 0;
+
+	if (pin)
+		dev_dbg(&dev->dev, "assigning IRQ %02d\n", dev->irq);
+
+	pci_write_config_byte(dev, PCI_INTERRUPT_LINE, dev->irq);
+
+	return 0;
+}
+
+static int thunder_pem_pci_probe(struct pci_dev *pdev,
+				 const struct pci_device_id *ent)
+{
+	struct thunder_pem *pem;
+	resource_size_t bar0_start;
+	u64 regval;
+	u64 sliaddr, pciaddr;
+	u32 cfgval;
+	int primary_bus;
+	int i;
+	int ret = 0;
+	struct resource *res;
+	LIST_HEAD(resources);
+
+	set_pcibios_add_device(thunder_pem_pcibios_add_device);
+
+	pem = devm_kzalloc(&pdev->dev, sizeof(*pem), GFP_KERNEL);
+	if (!pem)
+		return -ENOMEM;
+
+	pci_set_drvdata(pdev, pem);
+
+	bar0_start = pci_resource_start(pdev, 0);
+	pem->node = (bar0_start >> 44) & 3;
+	pem->id = ((bar0_start >> 24) & 7) + (6 * pem->node);
+	pem->sli = pem->id % 3;
+	pem->sli_group = (pem->id / 3) % 2;
+	pem->sli_window_base = 0x880000000000ull | (((u64)pem->node) << 44) | ((u64)pem->sli_group << 40);
+	pem->sli_window_base += 0x4000000000 * pem->sli;
+
+	ret = pci_enable_device_mem(pdev);
+	if (ret)
+		goto out;
+
+	pem->bar0 = pcim_iomap(pdev, 0, 0x100000);
+	if (!pem->bar0) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	pem->bar4 = pcim_iomap(pdev, 4, 0x100000);
+	if (!pem->bar0) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	sliaddr = THUNDER_SLI_S2M_REG_ACC_BASE | ((u64)pem->node << 44) | ((u64)pem->sli_group << 36);
+
+	regval = readq(pem->bar0 + PEM_ON);
+	if (!(regval & 1)) {
+		dev_notice(&pdev->dev, "PEM%u_ON not set, skipping...\n", pem->id);
+		goto out;
+	}
+
+	regval = readq(pem->bar0 + PEM_CTL_STATUS);
+	regval |= 0x10; /* Set Link Enable bit */
+	writeq(regval, pem->bar0 + PEM_CTL_STATUS);
+
+	udelay(1000);
+
+	cfgval = thunder_pcierc_config_read(pem, 32 * 4, 4); /* PCIERC_CFG032 */
+
+	if (((cfgval >> 29 & 0x1) == 0x0) || ((cfgval >> 27 & 0x1) == 0x1)) {
+		dev_notice(&pdev->dev, "PEM%u Link Timeout, skipping...\n", pem->id);
+		goto out;
+	}
+
+	pem->sli_s2m = devm_ioremap(&pdev->dev, sliaddr, 0x1000);
+	if (!pem->sli_s2m) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	pem->cfgregion = devm_ioremap(&pdev->dev, pem->sli_window_base, 0x100000000ull);
+	if (!pem->cfgregion) {
+		ret = -ENOMEM;
+		goto out;
+	}
+	regval = slix_s2m_reg_val(pem->sli, CTYPE_CONFIG, false, false, false, 0);
+	writeq(regval, pem->sli_s2m + 0x10 * ((0x40 * pem->sli) + 0));
+
+	cfgval = thunder_pcierc_config_read(pem, 6 * 4, 4); /* PCIERC_CFG006 */
+	primary_bus = (cfgval >> 8) & 0xff;
+
+	res = kzalloc(sizeof(*res), GFP_KERNEL);
+	if (!res) {
+		ret = -ENOMEM;
+		goto out;
+	}
+	res->start = primary_bus;
+	res->end = 255;
+	res->flags = IORESOURCE_BUS;
+	pci_add_resource(&resources, res);
+
+
+	res = kzalloc(sizeof(*res), GFP_KERNEL);
+	if (!res) {
+		ret = -ENOMEM;
+		goto out;
+	}
+	res->start = 0x100000 * pem->id;
+	res->end = res->start + 0x100000 - 1;
+	res->flags = IORESOURCE_IO;
+	pci_add_resource(&resources, res);
+	regval = slix_s2m_reg_val(pem->sli, CTYPE_IO, false, false, false, 0);
+	writeq(regval, pem->sli_s2m + 0x10 * ((0x40 * pem->sli) + 1));
+
+	res = kzalloc(sizeof(*res), GFP_KERNEL);
+	if (!res) {
+		ret = -ENOMEM;
+		goto out;
+	}
+	pciaddr = 0x10000000ull;
+	res->start = pem->sli_window_base + 0x1000000000ull + pciaddr;
+	res->end = res->start + 0x1000000000ull - pciaddr - 1;
+	res->flags = IORESOURCE_MEM;
+	pci_add_resource_offset(&resources, res, res->start - pciaddr);
+	for (i = 0; i < 16; i++) {
+		regval = slix_s2m_reg_val(pem->sli, CTYPE_MEMORY, false, false, false, i);
+		writeq(regval, pem->sli_s2m + 0x10 * ((0x40 * pem->sli) + (0x10 + i)));
+	}
+
+	res = kzalloc(sizeof(*res), GFP_KERNEL);
+	if (!res) {
+		ret = -ENOMEM;
+		goto out;
+	}
+	pciaddr = 0x1000000000ull;
+	res->start = pem->sli_window_base + 0x1000000000ull + pciaddr;
+	res->end = res->start + 0x1000000000ull - 1;
+	res->flags = IORESOURCE_MEM | IORESOURCE_PREFETCH;
+	pci_add_resource_offset(&resources, res, res->start - pciaddr);
+	for (i = 0; i < 16; i++) {
+		regval = slix_s2m_reg_val(pem->sli, CTYPE_MEMORY, true, true, true, i + 0x10);
+		writeq(regval, pem->sli_s2m + 0x10 * ((0x40 * pem->sli) + (0x20 + i)));
+	}
+
+	writeq(0, pem->bar0 + P2N_BAR0_START);
+	writeq(0, pem->bar0 + P2N_BAR1_START);
+	writeq(0, pem->bar0 + P2N_BAR2_START);
+
+	regval = 0x10;	/* BAR_CTL[BAR1_SIZ] = 1 (64MB) */
+	regval |= 0x8;	/* BAR_CTL[BAR2_ENB] = 1 */
+	writeq(regval, pem->bar0 + BAR_CTL);
+
+	/* 1st 4MB region -> GIC registers so 32-bit MSI can reach the GIC. */
+	regval = (THUNDER_GIC + (((u64)pem->node) << 44)) >> 18;
+	/* BAR1_INDEX[ADDR_V] = 1 */
+	regval |= 1;
+	writeq(regval, pem->bar0 + BAR1_INDEX);
+	/* Remaining regions linear mapping to physical address space */
+	for (i = 1; i < 16; i++) {
+		regval = (i << 4) | 1;
+		writeq(regval, pem->bar0 + BAR1_INDEX + 8 * i);
+	}
+
+	pem->bus = pci_create_root_bus(&pdev->dev, primary_bus, &thunder_pem_ops, pem, &resources);
+	if (!pem->bus) {
+		ret = -ENODEV;
+		goto err_root_bus;
+	}
+	pem->bus->is_pcierc = 1;
+	list_add_tail(&pem->list, &thunder_pem_buses);
+
+	for (i = 0; i < 3; i++) {
+		pem->vwire_data[i] = 40 + 4 * pem->id + i;
+		pem->vwire_irqs[i] = irq_create_mapping(gic_get_irq_domain(), pem->vwire_data[i]);
+		if (!pem->vwire_irqs[i]) {
+			dev_err(&pdev->dev, "Error: No irq mapping for %u\n", pem->vwire_data[i]);
+			continue;
+		}
+		irq_set_irq_type(pem->vwire_irqs[i], IRQ_TYPE_LEVEL_HIGH);
+
+		writeq(THUNDER_GICD_SETSPI_NSR,	pem->bar4 + 0 + (i + 2) * 32);
+		writeq(pem->vwire_data[i],	pem->bar4 + 8 + (i + 2) * 32);
+		writeq(THUNDER_GICD_CLRSPI_NSR,	pem->bar4 + 16 + (i + 2) * 32);
+		writeq(pem->vwire_data[i],	pem->bar4 + 24 + (i + 2) * 32);
+	}
+	ret = pci_read_config_dword(pdev, 44 * 4, &cfgval);
+	if (WARN_ON(ret))
+		goto err_free_root_bus;
+	cfgval &= ~0x40000000; /* Clear FUNM */
+	cfgval |= 0x80000000;  /* Set MSIXEN */
+	pci_write_config_dword(pdev, 44 * 4, cfgval);
+	pem->bus->msi = pdev->bus->msi;
+
+	pci_scan_child_bus(pem->bus);
+	pci_bus_add_devices(pem->bus);
+	pci_assign_unassigned_root_bus_resources(pem->bus);
+
+	return 0;
+
+err_free_root_bus:
+	pci_remove_root_bus(pem->bus);
+err_root_bus:
+	pci_free_resource_list(&resources);
+out:
+	return ret;
+}
+
+static void thunder_pem_pci_remove(struct pci_dev *pdev)
+{
+}
+
+static struct pci_driver thunder_pem_driver = {
+	.name		= "thunder_pem",
+	.id_table	= thunder_pem_pci_table,
+	.probe		= thunder_pem_pci_probe,
+	.remove		= thunder_pem_pci_remove
+};
+
+static int __init thunder_pcie_init(void)
+{
+	int ret;
+
+	ret = pci_register_driver(&thunder_pem_driver);
+
+	return ret;
+}
+module_init(thunder_pcie_init);
+
+static void __exit thunder_pcie_exit(void)
+{
+	pci_unregister_driver(&thunder_pem_driver);
+}
+module_exit(thunder_pcie_exit);
diff --git a/drivers/pci/host/pcie-thunder.c b/drivers/pci/host/pcie-thunder.c
new file mode 100644
index 0000000..4abab5a
--- /dev/null
+++ b/drivers/pci/host/pcie-thunder.c
@@ -0,0 +1,422 @@
+/*
+ * PCIe host controller driver for Cavium Thunder SOC
+ *
+ * Copyright (C) 2014, 2015 Cavium Inc.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_irq.h>
+#include <linux/of_pci.h>
+#include <linux/pci.h>
+#include <linux/platform_device.h>
+#include <linux/msi.h>
+#include <linux/irqchip/arm-gic-v3.h>
+
+#define PCI_DEVICE_ID_THUNDER_BRIDGE	0xa002
+
+#define THUNDER_PCIE_BUS_SHIFT		20
+#define THUNDER_PCIE_DEV_SHIFT		15
+#define THUNDER_PCIE_FUNC_SHIFT		12
+
+#define THUNDER_ECAM0_CFG_BASE		0x848000000000
+#define THUNDER_ECAM1_CFG_BASE		0x849000000000
+#define THUNDER_ECAM2_CFG_BASE		0x84a000000000
+#define THUNDER_ECAM3_CFG_BASE		0x84b000000000
+#define THUNDER_ECAM4_CFG_BASE		0x948000000000
+#define THUNDER_ECAM5_CFG_BASE		0x949000000000
+#define THUNDER_ECAM6_CFG_BASE		0x94a000000000
+#define THUNDER_ECAM7_CFG_BASE		0x94b000000000
+
+struct thunder_pcie {
+	struct device_node	*node;
+	struct device		*dev;
+	void __iomem		*cfg_base;
+	struct msi_controller	*msi;
+	int			ecam;
+	bool			valid;
+};
+
+int thunder_pem_requester_id(struct pci_dev *dev);
+
+static atomic_t thunder_pcie_ecam_probed;
+
+static u32 pci_requester_id_ecam(struct pci_dev *dev)
+{
+	return (((pci_domain_nr(dev->bus) >> 2) << 19) |
+		((pci_domain_nr(dev->bus) % 4) << 16) |
+		(dev->bus->number << 8) | dev->devfn);
+}
+
+static u32 thunder_pci_requester_id(struct pci_dev *dev, u16 alias)
+{
+	int ret;
+
+	ret = thunder_pem_requester_id(dev);
+	if (ret >= 0)
+		return (u32)ret;
+
+	return pci_requester_id_ecam(dev);
+}
+
+/*
+ * This bridge is just for the sake of supporting ARI for
+ * downstream devices. No resources are attached to it.
+ * Copy upstream root bus resources to bridge which aide in
+ * resource claiming for downstream devices
+ */
+static void pci_bridge_resource_fixup(struct pci_dev *dev)
+{
+	struct pci_bus *bus;
+	int resno;
+
+	bus = dev->subordinate;
+	for (resno = 0; resno < PCI_BRIDGE_RESOURCE_NUM; resno++) {
+		bus->resource[resno] = pci_bus_resource_n(bus->parent,
+			PCI_BRIDGE_RESOURCE_NUM + resno);
+	}
+
+	for (resno = PCI_BRIDGE_RESOURCES;
+		resno <= PCI_BRIDGE_RESOURCE_END; resno++) {
+		dev->resource[resno].start = dev->resource[resno].end = 0;
+		dev->resource[resno].flags = 0;
+	}
+}
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_CAVIUM, PCI_DEVICE_ID_THUNDER_BRIDGE,
+			pci_bridge_resource_fixup);
+
+/*
+ * All PCIe devices in Thunder have fixed resources, shouldn't be reassigned.
+ * Also claim the device's valid resources to set 'res->parent' hierarchy.
+ */
+static void pci_dev_resource_fixup(struct pci_dev *dev)
+{
+	struct resource *res;
+	int resno;
+
+	/*
+	 * If the ECAM is not yet probed, we must be in a virtual
+	 * machine.  In that case, don't mark things as
+	 * IORESOURCE_PCI_FIXED
+	 */
+	if (!atomic_read(&thunder_pcie_ecam_probed))
+		return;
+
+	for (resno = 0; resno < PCI_NUM_RESOURCES; resno++)
+		dev->resource[resno].flags |= IORESOURCE_PCI_FIXED;
+
+	for (resno = 0; resno < PCI_BRIDGE_RESOURCES; resno++) {
+		res = &dev->resource[resno];
+		if (res->parent || !(res->flags & IORESOURCE_MEM))
+			continue;
+		pci_claim_resource(dev, resno);
+	}
+}
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_CAVIUM, PCI_ANY_ID,
+			pci_dev_resource_fixup);
+
+static void __iomem *thunder_pcie_get_cfg_addr(struct thunder_pcie *pcie,
+					       unsigned int busnr,
+					       unsigned int devfn, int reg)
+{
+	return  pcie->cfg_base +
+		((busnr << THUNDER_PCIE_BUS_SHIFT)
+		 | (PCI_SLOT(devfn) << THUNDER_PCIE_DEV_SHIFT)
+		 | (PCI_FUNC(devfn) << THUNDER_PCIE_FUNC_SHIFT)) + reg;
+}
+
+static int thunder_pcie_read_config(struct pci_bus *bus, unsigned int devfn,
+				int reg, int size, u32 *val)
+{
+	struct thunder_pcie *pcie = bus->sysdata;
+	void __iomem *addr;
+	unsigned int busnr = bus->number;
+
+	if (busnr > 255 || devfn > 255 || reg > 4095)
+		return PCIBIOS_DEVICE_NOT_FOUND;
+
+	addr = thunder_pcie_get_cfg_addr(pcie, busnr, devfn, reg);
+
+	switch (size) {
+	case 1:
+		*val = readb(addr);
+		break;
+	case 2:
+		*val = readw(addr);
+		break;
+	case 4:
+		*val = readl(addr);
+		break;
+	default:
+		return PCIBIOS_BAD_REGISTER_NUMBER;
+	}
+
+	return PCIBIOS_SUCCESSFUL;
+}
+
+static int thunder_pcie_write_config(struct pci_bus *bus, unsigned int devfn,
+				  int reg, int size, u32 val)
+{
+	struct thunder_pcie *pcie = bus->sysdata;
+	void __iomem *addr;
+	unsigned int busnr = bus->number;
+
+	if (busnr > 255 || devfn > 255 || reg > 4095)
+		return PCIBIOS_DEVICE_NOT_FOUND;
+
+	addr = thunder_pcie_get_cfg_addr(pcie, busnr, devfn, reg);
+
+	switch (size) {
+	case 1:
+		writeb(val, addr);
+		break;
+	case 2:
+		writew(val, addr);
+		break;
+	case 4:
+		writel(val, addr);
+		break;
+	default:
+		return PCIBIOS_BAD_REGISTER_NUMBER;
+	}
+
+	return PCIBIOS_SUCCESSFUL;
+}
+
+static struct pci_ops thunder_pcie_ops = {
+	.read	= thunder_pcie_read_config,
+	.write	= thunder_pcie_write_config,
+};
+
+static int thunder_pcie_msi_enable(struct thunder_pcie *pcie,
+					struct pci_bus *bus)
+{
+	struct device_node *msi_node;
+
+	msi_node = of_parse_phandle(pcie->node, "msi-parent", 0);
+	if (!msi_node)
+		return -ENODEV;
+
+	pcie->msi = of_pci_find_msi_chip_by_node(msi_node);
+	if (!pcie->msi)
+		return -ENODEV;
+
+	pcie->msi->dev = pcie->dev;
+	bus->msi = pcie->msi;
+
+	return 0;
+}
+
+static void thunder_pcie_config(struct thunder_pcie *pcie, u64 addr)
+{
+	atomic_set(&thunder_pcie_ecam_probed, 1);
+	set_its_pci_requester_id(thunder_pci_requester_id);
+
+	pcie->valid = true;
+
+	switch (addr) {
+	case THUNDER_ECAM0_CFG_BASE:
+		pcie->ecam = 0;
+		break;
+	case THUNDER_ECAM1_CFG_BASE:
+		pcie->ecam = 1;
+		break;
+	case THUNDER_ECAM2_CFG_BASE:
+		pcie->ecam = 2;
+		break;
+	case THUNDER_ECAM3_CFG_BASE:
+		pcie->ecam = 3;
+		break;
+	case THUNDER_ECAM4_CFG_BASE:
+		pcie->ecam = 4;
+		break;
+	case THUNDER_ECAM5_CFG_BASE:
+		pcie->ecam = 5;
+		break;
+	case THUNDER_ECAM6_CFG_BASE:
+		pcie->ecam = 6;
+		break;
+	case THUNDER_ECAM7_CFG_BASE:
+		pcie->ecam = 7;
+		break;
+	default:
+		pcie->valid = false;
+		break;
+	}
+}
+
+static int thunder_pcie_probe(struct platform_device *pdev)
+{
+	struct thunder_pcie *pcie;
+	struct resource *cfg_base;
+	struct pci_bus *bus;
+	int ret = 0;
+	LIST_HEAD(res);
+
+	pcie = devm_kzalloc(&pdev->dev, sizeof(*pcie), GFP_KERNEL);
+	if (!pcie)
+		return -ENOMEM;
+
+	pcie->node = of_node_get(pdev->dev.of_node);
+	pcie->dev = &pdev->dev;
+
+	/* Get controller's configuration space range */
+	cfg_base = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+
+	thunder_pcie_config(pcie, cfg_base->start);
+
+	pcie->cfg_base = devm_ioremap_resource(&pdev->dev, cfg_base);
+	if (IS_ERR(pcie->cfg_base)) {
+		ret = PTR_ERR(pcie->cfg_base);
+		goto err_ioremap;
+	}
+
+	dev_info(&pdev->dev, "ECAM%d CFG BASE 0x%llx\n",
+		 pcie->ecam, (u64)cfg_base->start);
+
+	ret = of_pci_get_host_bridge_resources(pdev->dev.of_node,
+					0, 255, &res, NULL);
+	if (ret)
+		goto err_root_bus;
+
+	bus = pci_create_root_bus(&pdev->dev, 0, &thunder_pcie_ops, pcie, &res);
+	if (!bus) {
+		ret = -ENODEV;
+		goto err_root_bus;
+	}
+
+	/* Set reference to MSI chip */
+	ret = thunder_pcie_msi_enable(pcie, bus);
+	if (ret) {
+		dev_err(&pdev->dev,
+			"Unable to set reference to MSI chip: ret=%d\n", ret);
+		goto err_msi;
+	}
+
+	platform_set_drvdata(pdev, pcie);
+
+	pci_scan_child_bus(bus);
+	pci_bus_add_devices(bus);
+
+	return 0;
+err_msi:
+	pci_remove_root_bus(bus);
+err_root_bus:
+	pci_free_resource_list(&res);
+err_ioremap:
+	of_node_put(pcie->node);
+	return ret;
+}
+
+static const struct of_device_id thunder_pcie_of_match[] = {
+	{ .compatible = "cavium,thunder-pcie", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, thunder_pcie_of_match);
+
+static struct platform_driver thunder_pcie_driver = {
+	.driver = {
+		.name = "thunder-pcie",
+		.owner = THIS_MODULE,
+		.of_match_table = thunder_pcie_of_match,
+	},
+	.probe = thunder_pcie_probe,
+};
+module_platform_driver(thunder_pcie_driver);
+
+#ifdef CONFIG_ACPI
+
+static int
+thunder_mmcfg_read_config(struct pci_mmcfg_region *cfg, unsigned int busnr,
+			unsigned int devfn, int reg, int len, u32 *value)
+{
+	struct thunder_pcie *pcie = cfg->data;
+	void __iomem *addr;
+
+	if (!pcie->valid) {
+		/* Not support for now */
+		pr_err("RC PEM not supported !!!\n");
+		return PCIBIOS_DEVICE_NOT_FOUND;
+	}
+
+	addr = thunder_pcie_get_cfg_addr(pcie, busnr, devfn, reg);
+
+	switch (len) {
+	case 1:
+		*value = readb(addr);
+		break;
+	case 2:
+		*value = readw(addr);
+		break;
+	case 4:
+		*value = readl(addr);
+		break;
+	default:
+		return PCIBIOS_BAD_REGISTER_NUMBER;
+	}
+
+	return PCIBIOS_SUCCESSFUL;
+}
+
+static int thunder_mmcfg_write_config(struct pci_mmcfg_region *cfg,
+		unsigned int busnr, unsigned int devfn, int reg, int len,
+		u32 value) {
+	struct thunder_pcie *pcie = cfg->data;
+	void __iomem *addr;
+
+	if (!pcie->valid) {
+		/* Not support for now */
+		pr_err("RC PEM not supported !!!\n");
+		return PCIBIOS_DEVICE_NOT_FOUND;
+	}
+
+	addr = thunder_pcie_get_cfg_addr(pcie, busnr, devfn, reg);
+
+	switch (len) {
+	case 1:
+		writeb(value, addr);
+		break;
+	case 2:
+		writew(value, addr);
+		break;
+	case 4:
+		writel(value, addr);
+		break;
+	default:
+		return PCIBIOS_BAD_REGISTER_NUMBER;
+	}
+
+	return PCIBIOS_SUCCESSFUL;
+}
+
+static int thunder_acpi_mcfg_fixup(struct acpi_pci_root *root,
+				   struct pci_mmcfg_region *cfg)
+{
+	struct thunder_pcie *pcie;
+
+	pcie = kzalloc(sizeof(*pcie), GFP_KERNEL);
+	if (!pcie)
+		return -ENOMEM;
+
+	pcie->dev = &root->device->dev;
+
+	thunder_pcie_config(pcie, cfg->address);
+
+	pcie->cfg_base = cfg->virt;
+	cfg->data = pcie;
+	cfg->read = thunder_mmcfg_read_config;
+	cfg->write = thunder_mmcfg_write_config;
+
+	return 0;
+}
+DECLARE_ACPI_MCFG_FIXUP("CAVIUM", "THUNDERX", thunder_acpi_mcfg_fixup);
+#endif
+
+MODULE_AUTHOR("Sunil Goutham");
+MODULE_DESCRIPTION("Cavium Thunder ECAM host controller driver");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

[PATCH 5/5] PCI: Add host drivers for Cavium ThunderX processors.

[Mark Rutland]

Hi,

As mentioned in my reply to the cover letter, a DT binding document is
necessary for this.

It looks like many of the properties of your root complex which should
be described (e.g. physical addresses, master IDs as visible to MSI
controllers) are blindly assumed by this driver, and I expect those to
be explicit in the DT. I suspect that means you also need to reconsider
your ACPI description, which needs to express the same information.

Please Cc me on subsequent postings of both the binding and driver.

On Wed, Jul 15, 2015 at 05:54:45PM +0100, David Daney wrote:
> From: David Daney <david.daney@cavium.com>
>
> Signed-off-by: David Daney <david.daney@cavium.com>
> ---
>  drivers/pci/host/Kconfig            |  12 +
>  drivers/pci/host/Makefile           |   2 +
>  drivers/pci/host/pcie-thunder-pem.c | 462 ++++++++++++++++++++++++++++++++++++
>  drivers/pci/host/pcie-thunder.c     | 422 ++++++++++++++++++++++++++++++++
>  4 files changed, 898 insertions(+)
>  create mode 100644 drivers/pci/host/pcie-thunder-pem.c
>  create mode 100644 drivers/pci/host/pcie-thunder.c
>
> diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
> index c132bdd..06e26ad 100644
> --- a/drivers/pci/host/Kconfig
> +++ b/drivers/pci/host/Kconfig
> @@ -145,4 +145,16 @@ config PCIE_IPROC_BCMA
>           Say Y here if you want to use the Broadcom iProc PCIe controller
>           through the BCMA bus interface
>
> +config PCI_THUNDER_PEM
> +       bool
> +
> +config PCI_THUNDER
> +       bool "Thunder PCIe host controller"
> +       depends on ARM64 || COMPILE_TEST
> +       depends on OF_PCI
> +       select PCI_MSI
> +       select PCI_THUNDER_PEM
> +       help
> +         Say Y here if you want internal PCI support on Thunder SoC.
> +
>  endmenu
> diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
> index 140d66f..a355155 100644
> --- a/drivers/pci/host/Makefile
> +++ b/drivers/pci/host/Makefile
> @@ -17,3 +17,5 @@ obj-$(CONFIG_PCI_VERSATILE) += pci-versatile.o
>  obj-$(CONFIG_PCIE_IPROC) += pcie-iproc.o
>  obj-$(CONFIG_PCIE_IPROC_PLATFORM) += pcie-iproc-platform.o
>  obj-$(CONFIG_PCIE_IPROC_BCMA) += pcie-iproc-bcma.o
> +obj-$(CONFIG_PCI_THUNDER) += pcie-thunder.o
> +obj-$(CONFIG_PCI_THUNDER_PEM) += pcie-thunder-pem.o
> diff --git a/drivers/pci/host/pcie-thunder-pem.c b/drivers/pci/host/pcie-thunder-pem.c
> new file mode 100644
> index 0000000..7861a8a
> --- /dev/null
> +++ b/drivers/pci/host/pcie-thunder-pem.c
> @@ -0,0 +1,462 @@
> +/*
> + * PCIe host controller driver for Cavium Thunder SOC
> + *
> + * Copyright (C) 2014,2015 Cavium Inc.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + */
> +/* #define DEBUG 1 */
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/delay.h>
> +#include <linux/irq.h>
> +#include <linux/pci.h>
> +#include <linux/irqdomain.h>
> +#include <linux/msi.h>
> +#include <linux/irqchip/arm-gic-v3.h>

This looks very suspicious.

> +
> +#define THUNDER_SLI_S2M_REG_ACC_BASE   0x874001000000ull
> +
> +#define THUNDER_GIC                    0x801000000000ull
> +#define THUNDER_GICD_SETSPI_NSR                0x801000000040ull
> +#define THUNDER_GICD_CLRSPI_NSR                0x801000000048ull

Are these physical addresses related to your GIC?

Given that this is a driver for a "Thunder PCIe host controller", and
not a GIC, this driver has no business poking those registers. If you
need something to happen at the GIC, you must go via the irqchip
infrastructure in order to do so.

Additionally, there shouldn't be any hard-coded physical addresses in
this driver. They should all come from DT (or ACPI). That applies to
_all_ physical addresses in this driver.

> +int thunder_pem_requester_id(struct pci_dev *dev);
> +
> +static atomic_t thunder_pcie_ecam_probed;
> +
> +static u32 pci_requester_id_ecam(struct pci_dev *dev)
> +{
> +       return (((pci_domain_nr(dev->bus) >> 2) << 19) |
> +               ((pci_domain_nr(dev->bus) % 4) << 16) |
> +               (dev->bus->number << 8) | dev->devfn);
> +}
> +
> +static u32 thunder_pci_requester_id(struct pci_dev *dev, u16 alias)
> +{
> +       int ret;
> +
> +       ret = thunder_pem_requester_id(dev);
> +       if (ret >= 0)
> +               return (u32)ret;
> +
> +       return pci_requester_id_ecam(dev);
> +}

Master IDs should be described in IORT with ACPI, and there's ongoing
discussion regarding what to do for DT [1] (I'll be posting updates
shortly).

This shouldn't live in driver code where it's non-standard and hidden
from other subsystems which need it (e.g. KVM).

> +/*
> + * All PCIe devices in Thunder have fixed resources, shouldn't be reassigned.

If this is the case, what's stopping you using the generic PCIe driver
that we already have?

Also isn't pci-probe-only sufficient?

> + * Also claim the device's valid resources to set 'res->parent' hierarchy.
> + */
> +static void pci_dev_resource_fixup(struct pci_dev *dev)
> +{
> +       struct resource *res;
> +       int resno;
> +
> +       /*
> +        * If the ECAM is not yet probed, we must be in a virtual
> +        * machine.  In that case, don't mark things as
> +        * IORESOURCE_PCI_FIXED
> +        */

You always set thunder_pcie_ecam_probed when the driver probes, and I
can't see what that has to do w.r.t. physical vs virtual machines.

What am I missing?

> +       if (!atomic_read(&thunder_pcie_ecam_probed))
> +               return;
> +
> +       for (resno = 0; resno < PCI_NUM_RESOURCES; resno++)
> +               dev->resource[resno].flags |= IORESOURCE_PCI_FIXED;
> +
> +       for (resno = 0; resno < PCI_BRIDGE_RESOURCES; resno++) {
> +               res = &dev->resource[resno];
> +               if (res->parent || !(res->flags & IORESOURCE_MEM))
> +                       continue;
> +               pci_claim_resource(dev, resno);
> +       }
> +}
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_CAVIUM, PCI_ANY_ID,
> +                       pci_dev_resource_fixup);

Thanks,
Mark.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/354997.html

[PATCH 5/5] PCI: Add host drivers for Cavium ThunderX processors.

[Marc Zyngier]

On 15/07/15 17:54, David Daney wrote:
> From: David Daney <david.daney@cavium.com>
> 
> Signed-off-by: David Daney <david.daney@cavium.com>
> ---
>  drivers/pci/host/Kconfig            |  12 +
>  drivers/pci/host/Makefile           |   2 +
>  drivers/pci/host/pcie-thunder-pem.c | 462 ++++++++++++++++++++++++++++++++++++
>  drivers/pci/host/pcie-thunder.c     | 422 ++++++++++++++++++++++++++++++++
>  4 files changed, 898 insertions(+)
>  create mode 100644 drivers/pci/host/pcie-thunder-pem.c
>  create mode 100644 drivers/pci/host/pcie-thunder.c
> 
> diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
> index c132bdd..06e26ad 100644
> --- a/drivers/pci/host/Kconfig
> +++ b/drivers/pci/host/Kconfig
> @@ -145,4 +145,16 @@ config PCIE_IPROC_BCMA
>  	  Say Y here if you want to use the Broadcom iProc PCIe controller
>  	  through the BCMA bus interface
>  
> +config PCI_THUNDER_PEM
> +	bool
> +
> +config PCI_THUNDER
> +	bool "Thunder PCIe host controller"
> +	depends on ARM64 || COMPILE_TEST
> +	depends on OF_PCI
> +	select PCI_MSI
> +	select PCI_THUNDER_PEM
> +	help
> +	  Say Y here if you want internal PCI support on Thunder SoC.
> +
>  endmenu
> diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
> index 140d66f..a355155 100644
> --- a/drivers/pci/host/Makefile
> +++ b/drivers/pci/host/Makefile
> @@ -17,3 +17,5 @@ obj-$(CONFIG_PCI_VERSATILE) += pci-versatile.o
>  obj-$(CONFIG_PCIE_IPROC) += pcie-iproc.o
>  obj-$(CONFIG_PCIE_IPROC_PLATFORM) += pcie-iproc-platform.o
>  obj-$(CONFIG_PCIE_IPROC_BCMA) += pcie-iproc-bcma.o
> +obj-$(CONFIG_PCI_THUNDER) += pcie-thunder.o
> +obj-$(CONFIG_PCI_THUNDER_PEM) += pcie-thunder-pem.o
> diff --git a/drivers/pci/host/pcie-thunder-pem.c b/drivers/pci/host/pcie-thunder-pem.c
> new file mode 100644
> index 0000000..7861a8a
> --- /dev/null
> +++ b/drivers/pci/host/pcie-thunder-pem.c
> @@ -0,0 +1,462 @@
> +/*
> + * PCIe host controller driver for Cavium Thunder SOC
> + *
> + * Copyright (C) 2014,2015 Cavium Inc.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + */
> +/* #define DEBUG 1 */
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/delay.h>
> +#include <linux/irq.h>
> +#include <linux/pci.h>
> +#include <linux/irqdomain.h>
> +#include <linux/msi.h>
> +#include <linux/irqchip/arm-gic-v3.h>
> +
> +#define THUNDER_SLI_S2M_REG_ACC_BASE	0x874001000000ull
> +
> +#define THUNDER_GIC			0x801000000000ull
> +#define THUNDER_GICD_SETSPI_NSR		0x801000000040ull
> +#define THUNDER_GICD_CLRSPI_NSR		0x801000000048ull

Right. Now I know why you want the SPI domain. If you want virtual wire
SPIs, this is not going to happen this way. Implement a stacked irqchip
on top of the GIC, and don't mess with the GIC in your driver.

The whole approach is doomed.

Also, you'll need to be able to describe which SPIs are MSI only, and
this will require a change in the binding.

> +
> +#define THUNDER_GSER_PCIE_MASK		0x01
> +
> +#define PEM_CTL_STATUS	0x000
> +#define PEM_RD_CFG	0x030
> +#define P2N_BAR0_START	0x080
> +#define P2N_BAR1_START	0x088
> +#define P2N_BAR2_START	0x090
> +#define BAR_CTL		0x0a8
> +#define BAR2_MASK	0x0b0
> +#define BAR1_INDEX	0x100
> +#define PEM_CFG		0x410
> +#define PEM_ON		0x420
> +
> +struct thunder_pem {
> +	struct list_head list; /* on thunder_pem_buses */
> +	bool		connected;
> +	unsigned int	id;
> +	unsigned int	sli;
> +	unsigned int	sli_group;
> +	unsigned int	node;
> +	u64		sli_window_base;
> +	void __iomem	*bar0;
> +	void __iomem	*bar4;
> +	void __iomem	*sli_s2m;
> +	void __iomem	*cfgregion;
> +	struct pci_bus	*bus;
> +	int		vwire_irqs[4];
> +	u32		vwire_data[4];
> +};
> +
> +static LIST_HEAD(thunder_pem_buses);
> +
> +static struct pci_device_id thunder_pem_pci_table[] = {
> +	{PCI_VENDOR_ID_CAVIUM, 0xa020, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0},
> +	{0,}
> +};
> +MODULE_DEVICE_TABLE(pci, thunder_pem_pci_table);
> +
> +enum slix_s2m_ctype {
> +	CTYPE_MEMORY	= 0,
> +	CTYPE_CONFIG	= 1,
> +	CTYPE_IO	= 2
> +};
> +
> +static u64 slix_s2m_reg_val(unsigned mac, enum slix_s2m_ctype ctype,
> +			    bool merge, bool relaxed, bool snoop, u32 ba_msb)
> +{
> +	u64 v;
> +
> +	v = (u64)(mac % 3) << 49;
> +	v |= (u64)ctype << 53;
> +	if (!merge)
> +		v |= 1ull << 48;
> +	if (relaxed)
> +		v |= 5ull << 40;
> +	if (!snoop)
> +		v |= 5ull << 41;
> +	v |= (u64)ba_msb;
> +
> +	return v;
> +}
> +
> +static u32 thunder_pcierc_config_read(struct thunder_pem *pem, u32 reg, int size)
> +{
> +	unsigned int val;
> +
> +	writeq(reg & ~3u, pem->bar0 + PEM_RD_CFG);
> +	val = readq(pem->bar0 + PEM_RD_CFG) >> 32;
> +
> +	if (size == 1)
> +		val = (val >> (8 * (reg & 3))) & 0xff;
> +	else if (size == 2)
> +		val = (val >> (8 * (reg & 3))) & 0xffff;
> +
> +	return val;
> +}
> +
> +static int thunder_pem_read_config(struct pci_bus *bus, unsigned int devfn,
> +				   int reg, int size, u32 *val)
> +{
> +	void __iomem *addr;
> +	struct thunder_pem *pem = bus->sysdata;
> +	unsigned int busnr = bus->number;
> +
> +	if (busnr > 255 || devfn > 255 || reg > 4095)
> +		return PCIBIOS_DEVICE_NOT_FOUND;
> +
> +	addr = pem->cfgregion + ((busnr << 24)  | (devfn << 16) | reg);
> +
> +	switch (size) {
> +	case 1:
> +		*val = readb(addr);
> +		break;
> +	case 2:
> +		*val = readw(addr);
> +		break;
> +	case 4:
> +		*val = readl(addr);
> +		break;
> +	default:
> +		return PCIBIOS_BAD_REGISTER_NUMBER;
> +	}
> +
> +	return PCIBIOS_SUCCESSFUL;
> +}
> +
> +static int thunder_pem_write_config(struct pci_bus *bus, unsigned int devfn,
> +				    int reg, int size, u32 val)
> +{
> +	void __iomem *addr;
> +	struct thunder_pem *pem = bus->sysdata;
> +	unsigned int busnr = bus->number;
> +
> +	if (busnr > 255 || devfn > 255 || reg > 4095)
> +		return PCIBIOS_DEVICE_NOT_FOUND;
> +
> +	addr = pem->cfgregion + ((busnr << 24)  | (devfn << 16) | reg);
> +
> +	switch (size) {
> +	case 1:
> +		writeb(val, addr);
> +		break;
> +	case 2:
> +		writew(val, addr);
> +		break;
> +	case 4:
> +		writel(val, addr);
> +		break;
> +	default:
> +		return PCIBIOS_BAD_REGISTER_NUMBER;
> +	}
> +
> +	return PCIBIOS_SUCCESSFUL;
> +}
> +
> +static struct pci_ops thunder_pem_ops = {
> +	.read	= thunder_pem_read_config,
> +	.write	= thunder_pem_write_config,
> +};
> +
> +static struct thunder_pem *thunder_pem_from_dev(struct pci_dev *dev)
> +{
> +	struct thunder_pem *pem;
> +	struct pci_bus *bus = dev->bus;
> +
> +	while (!pci_is_root_bus(bus))
> +		bus = bus->parent;
> +
> +	list_for_each_entry(pem, &thunder_pem_buses, list) {
> +		if (pem->bus == bus)
> +			return pem;
> +	}
> +	return NULL;
> +}
> +
> +int thunder_pem_requester_id(struct pci_dev *dev)
> +{
> +	struct thunder_pem *pem = thunder_pem_from_dev(dev);
> +
> +	if (!pem)
> +		return -ENODEV;
> +
> +	if (pem->id < 3)
> +		return ((1 << 16) |
> +			((dev)->bus->number << 8) |
> +			(dev)->devfn);
> +
> +	if (pem->id < 6)
> +		return ((3 << 16) |
> +			((dev)->bus->number << 8) |
> +			(dev)->devfn);
> +
> +	if (pem->id < 9)
> +		return ((1 << 19) | (1 << 16) |
> +			((dev)->bus->number << 8) |
> +			(dev)->devfn);
> +
> +	if (pem->id < 12)
> +		return ((1 << 19) |
> +			(3 << 16) |
> +			((dev)->bus->number << 8) |
> +			(dev)->devfn);
> +	return -ENODEV;
> +}
> +
> +static int thunder_pem_pcibios_add_device(struct pci_dev *dev)
> +{
> +	struct thunder_pem *pem;
> +	u8 pin;
> +
> +	pem = thunder_pem_from_dev(dev);
> +	if (!pem)
> +		return 0;
> +
> +	pci_read_config_byte(dev, PCI_INTERRUPT_PIN, &pin);
> +
> +	/* Cope with illegal. */
> +	if (pin > 4)
> +		pin = 1;
> +
> +	dev->irq = pin > 0 ? pem->vwire_irqs[pin - 1] : 0;
> +
> +	if (pin)
> +		dev_dbg(&dev->dev, "assigning IRQ %02d\n", dev->irq);
> +
> +	pci_write_config_byte(dev, PCI_INTERRUPT_LINE, dev->irq);
> +
> +	return 0;
> +}
> +
> +static int thunder_pem_pci_probe(struct pci_dev *pdev,
> +				 const struct pci_device_id *ent)
> +{
> +	struct thunder_pem *pem;
> +	resource_size_t bar0_start;
> +	u64 regval;
> +	u64 sliaddr, pciaddr;
> +	u32 cfgval;
> +	int primary_bus;
> +	int i;
> +	int ret = 0;
> +	struct resource *res;
> +	LIST_HEAD(resources);
> +
> +	set_pcibios_add_device(thunder_pem_pcibios_add_device);
> +
> +	pem = devm_kzalloc(&pdev->dev, sizeof(*pem), GFP_KERNEL);
> +	if (!pem)
> +		return -ENOMEM;
> +
> +	pci_set_drvdata(pdev, pem);
> +
> +	bar0_start = pci_resource_start(pdev, 0);
> +	pem->node = (bar0_start >> 44) & 3;
> +	pem->id = ((bar0_start >> 24) & 7) + (6 * pem->node);
> +	pem->sli = pem->id % 3;
> +	pem->sli_group = (pem->id / 3) % 2;
> +	pem->sli_window_base = 0x880000000000ull | (((u64)pem->node) << 44) | ((u64)pem->sli_group << 40);
> +	pem->sli_window_base += 0x4000000000 * pem->sli;
> +
> +	ret = pci_enable_device_mem(pdev);
> +	if (ret)
> +		goto out;
> +
> +	pem->bar0 = pcim_iomap(pdev, 0, 0x100000);
> +	if (!pem->bar0) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	pem->bar4 = pcim_iomap(pdev, 4, 0x100000);
> +	if (!pem->bar0) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	sliaddr = THUNDER_SLI_S2M_REG_ACC_BASE | ((u64)pem->node << 44) | ((u64)pem->sli_group << 36);
> +
> +	regval = readq(pem->bar0 + PEM_ON);
> +	if (!(regval & 1)) {
> +		dev_notice(&pdev->dev, "PEM%u_ON not set, skipping...\n", pem->id);
> +		goto out;
> +	}
> +
> +	regval = readq(pem->bar0 + PEM_CTL_STATUS);
> +	regval |= 0x10; /* Set Link Enable bit */
> +	writeq(regval, pem->bar0 + PEM_CTL_STATUS);
> +
> +	udelay(1000);
> +
> +	cfgval = thunder_pcierc_config_read(pem, 32 * 4, 4); /* PCIERC_CFG032 */
> +
> +	if (((cfgval >> 29 & 0x1) == 0x0) || ((cfgval >> 27 & 0x1) == 0x1)) {
> +		dev_notice(&pdev->dev, "PEM%u Link Timeout, skipping...\n", pem->id);
> +		goto out;
> +	}
> +
> +	pem->sli_s2m = devm_ioremap(&pdev->dev, sliaddr, 0x1000);
> +	if (!pem->sli_s2m) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	pem->cfgregion = devm_ioremap(&pdev->dev, pem->sli_window_base, 0x100000000ull);
> +	if (!pem->cfgregion) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +	regval = slix_s2m_reg_val(pem->sli, CTYPE_CONFIG, false, false, false, 0);
> +	writeq(regval, pem->sli_s2m + 0x10 * ((0x40 * pem->sli) + 0));
> +
> +	cfgval = thunder_pcierc_config_read(pem, 6 * 4, 4); /* PCIERC_CFG006 */
> +	primary_bus = (cfgval >> 8) & 0xff;
> +
> +	res = kzalloc(sizeof(*res), GFP_KERNEL);
> +	if (!res) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +	res->start = primary_bus;
> +	res->end = 255;
> +	res->flags = IORESOURCE_BUS;
> +	pci_add_resource(&resources, res);
> +
> +
> +	res = kzalloc(sizeof(*res), GFP_KERNEL);
> +	if (!res) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +	res->start = 0x100000 * pem->id;
> +	res->end = res->start + 0x100000 - 1;
> +	res->flags = IORESOURCE_IO;
> +	pci_add_resource(&resources, res);
> +	regval = slix_s2m_reg_val(pem->sli, CTYPE_IO, false, false, false, 0);
> +	writeq(regval, pem->sli_s2m + 0x10 * ((0x40 * pem->sli) + 1));
> +
> +	res = kzalloc(sizeof(*res), GFP_KERNEL);
> +	if (!res) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +	pciaddr = 0x10000000ull;
> +	res->start = pem->sli_window_base + 0x1000000000ull + pciaddr;
> +	res->end = res->start + 0x1000000000ull - pciaddr - 1;
> +	res->flags = IORESOURCE_MEM;
> +	pci_add_resource_offset(&resources, res, res->start - pciaddr);
> +	for (i = 0; i < 16; i++) {
> +		regval = slix_s2m_reg_val(pem->sli, CTYPE_MEMORY, false, false, false, i);
> +		writeq(regval, pem->sli_s2m + 0x10 * ((0x40 * pem->sli) + (0x10 + i)));
> +	}
> +
> +	res = kzalloc(sizeof(*res), GFP_KERNEL);
> +	if (!res) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +	pciaddr = 0x1000000000ull;
> +	res->start = pem->sli_window_base + 0x1000000000ull + pciaddr;
> +	res->end = res->start + 0x1000000000ull - 1;
> +	res->flags = IORESOURCE_MEM | IORESOURCE_PREFETCH;
> +	pci_add_resource_offset(&resources, res, res->start - pciaddr);
> +	for (i = 0; i < 16; i++) {
> +		regval = slix_s2m_reg_val(pem->sli, CTYPE_MEMORY, true, true, true, i + 0x10);
> +		writeq(regval, pem->sli_s2m + 0x10 * ((0x40 * pem->sli) + (0x20 + i)));
> +	}
> +
> +	writeq(0, pem->bar0 + P2N_BAR0_START);
> +	writeq(0, pem->bar0 + P2N_BAR1_START);
> +	writeq(0, pem->bar0 + P2N_BAR2_START);
> +
> +	regval = 0x10;	/* BAR_CTL[BAR1_SIZ] = 1 (64MB) */
> +	regval |= 0x8;	/* BAR_CTL[BAR2_ENB] = 1 */
> +	writeq(regval, pem->bar0 + BAR_CTL);
> +
> +	/* 1st 4MB region -> GIC registers so 32-bit MSI can reach the GIC. */
> +	regval = (THUNDER_GIC + (((u64)pem->node) << 44)) >> 18;
> +	/* BAR1_INDEX[ADDR_V] = 1 */
> +	regval |= 1;
> +	writeq(regval, pem->bar0 + BAR1_INDEX);
> +	/* Remaining regions linear mapping to physical address space */
> +	for (i = 1; i < 16; i++) {
> +		regval = (i << 4) | 1;
> +		writeq(regval, pem->bar0 + BAR1_INDEX + 8 * i);
> +	}
> +
> +	pem->bus = pci_create_root_bus(&pdev->dev, primary_bus, &thunder_pem_ops, pem, &resources);
> +	if (!pem->bus) {
> +		ret = -ENODEV;
> +		goto err_root_bus;
> +	}
> +	pem->bus->is_pcierc = 1;
> +	list_add_tail(&pem->list, &thunder_pem_buses);
> +
> +	for (i = 0; i < 3; i++) {
> +		pem->vwire_data[i] = 40 + 4 * pem->id + i;
> +		pem->vwire_irqs[i] = irq_create_mapping(gic_get_irq_domain(), pem->vwire_data[i]);
> +		if (!pem->vwire_irqs[i]) {
> +			dev_err(&pdev->dev, "Error: No irq mapping for %u\n", pem->vwire_data[i]);
> +			continue;
> +		}
> +		irq_set_irq_type(pem->vwire_irqs[i], IRQ_TYPE_LEVEL_HIGH);
> +
> +		writeq(THUNDER_GICD_SETSPI_NSR,	pem->bar4 + 0 + (i + 2) * 32);
> +		writeq(pem->vwire_data[i],	pem->bar4 + 8 + (i + 2) * 32);
> +		writeq(THUNDER_GICD_CLRSPI_NSR,	pem->bar4 + 16 + (i + 2) * 32);
> +		writeq(pem->vwire_data[i],	pem->bar4 + 24 + (i + 2) * 32);

Yeah, just as I thought. Poor man's MSIs. I've posted a series that
would work for that kind of ugly hack:

http://lwn.net/Articles/651071/

I'm feeling a bit sick now...

> +	}
> +	ret = pci_read_config_dword(pdev, 44 * 4, &cfgval);
> +	if (WARN_ON(ret))
> +		goto err_free_root_bus;
> +	cfgval &= ~0x40000000; /* Clear FUNM */
> +	cfgval |= 0x80000000;  /* Set MSIXEN */
> +	pci_write_config_dword(pdev, 44 * 4, cfgval);
> +	pem->bus->msi = pdev->bus->msi;
> +
> +	pci_scan_child_bus(pem->bus);
> +	pci_bus_add_devices(pem->bus);
> +	pci_assign_unassigned_root_bus_resources(pem->bus);
> +
> +	return 0;
> +
> +err_free_root_bus:
> +	pci_remove_root_bus(pem->bus);
> +err_root_bus:
> +	pci_free_resource_list(&resources);
> +out:
> +	return ret;
> +}
> +
> +static void thunder_pem_pci_remove(struct pci_dev *pdev)
> +{
> +}
> +
> +static struct pci_driver thunder_pem_driver = {
> +	.name		= "thunder_pem",
> +	.id_table	= thunder_pem_pci_table,
> +	.probe		= thunder_pem_pci_probe,
> +	.remove		= thunder_pem_pci_remove
> +};
> +
> +static int __init thunder_pcie_init(void)
> +{
> +	int ret;
> +
> +	ret = pci_register_driver(&thunder_pem_driver);
> +
> +	return ret;
> +}
> +module_init(thunder_pcie_init);
> +
> +static void __exit thunder_pcie_exit(void)
> +{
> +	pci_unregister_driver(&thunder_pem_driver);
> +}
> +module_exit(thunder_pcie_exit);
> diff --git a/drivers/pci/host/pcie-thunder.c b/drivers/pci/host/pcie-thunder.c
> new file mode 100644
> index 0000000..4abab5a
> --- /dev/null
> +++ b/drivers/pci/host/pcie-thunder.c
> @@ -0,0 +1,422 @@
> +/*
> + * PCIe host controller driver for Cavium Thunder SOC
> + *
> + * Copyright (C) 2014, 2015 Cavium Inc.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_pci.h>
> +#include <linux/pci.h>
> +#include <linux/platform_device.h>
> +#include <linux/msi.h>
> +#include <linux/irqchip/arm-gic-v3.h>
> +
> +#define PCI_DEVICE_ID_THUNDER_BRIDGE	0xa002
> +
> +#define THUNDER_PCIE_BUS_SHIFT		20
> +#define THUNDER_PCIE_DEV_SHIFT		15
> +#define THUNDER_PCIE_FUNC_SHIFT		12
> +
> +#define THUNDER_ECAM0_CFG_BASE		0x848000000000
> +#define THUNDER_ECAM1_CFG_BASE		0x849000000000
> +#define THUNDER_ECAM2_CFG_BASE		0x84a000000000
> +#define THUNDER_ECAM3_CFG_BASE		0x84b000000000
> +#define THUNDER_ECAM4_CFG_BASE		0x948000000000
> +#define THUNDER_ECAM5_CFG_BASE		0x949000000000
> +#define THUNDER_ECAM6_CFG_BASE		0x94a000000000
> +#define THUNDER_ECAM7_CFG_BASE		0x94b000000000
> +
> +struct thunder_pcie {
> +	struct device_node	*node;
> +	struct device		*dev;
> +	void __iomem		*cfg_base;
> +	struct msi_controller	*msi;
> +	int			ecam;
> +	bool			valid;
> +};
> +
> +int thunder_pem_requester_id(struct pci_dev *dev);
> +
> +static atomic_t thunder_pcie_ecam_probed;
> +
> +static u32 pci_requester_id_ecam(struct pci_dev *dev)
> +{
> +	return (((pci_domain_nr(dev->bus) >> 2) << 19) |
> +		((pci_domain_nr(dev->bus) % 4) << 16) |
> +		(dev->bus->number << 8) | dev->devfn);
> +}
> +
> +static u32 thunder_pci_requester_id(struct pci_dev *dev, u16 alias)
> +{
> +	int ret;
> +
> +	ret = thunder_pem_requester_id(dev);
> +	if (ret >= 0)
> +		return (u32)ret;
> +
> +	return pci_requester_id_ecam(dev);
> +}
> +
> +/*
> + * This bridge is just for the sake of supporting ARI for
> + * downstream devices. No resources are attached to it.
> + * Copy upstream root bus resources to bridge which aide in
> + * resource claiming for downstream devices
> + */
> +static void pci_bridge_resource_fixup(struct pci_dev *dev)
> +{
> +	struct pci_bus *bus;
> +	int resno;
> +
> +	bus = dev->subordinate;
> +	for (resno = 0; resno < PCI_BRIDGE_RESOURCE_NUM; resno++) {
> +		bus->resource[resno] = pci_bus_resource_n(bus->parent,
> +			PCI_BRIDGE_RESOURCE_NUM + resno);
> +	}
> +
> +	for (resno = PCI_BRIDGE_RESOURCES;
> +		resno <= PCI_BRIDGE_RESOURCE_END; resno++) {
> +		dev->resource[resno].start = dev->resource[resno].end = 0;
> +		dev->resource[resno].flags = 0;
> +	}
> +}
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_CAVIUM, PCI_DEVICE_ID_THUNDER_BRIDGE,
> +			pci_bridge_resource_fixup);
> +
> +/*
> + * All PCIe devices in Thunder have fixed resources, shouldn't be reassigned.
> + * Also claim the device's valid resources to set 'res->parent' hierarchy.
> + */

What's wrong with probe_only?

> +static void pci_dev_resource_fixup(struct pci_dev *dev)
> +{
> +	struct resource *res;
> +	int resno;
> +
> +	/*
> +	 * If the ECAM is not yet probed, we must be in a virtual
> +	 * machine.  In that case, don't mark things as

What makes you think that?

> +	 * IORESOURCE_PCI_FIXED
> +	 */
> +	if (!atomic_read(&thunder_pcie_ecam_probed))
> +		return;
> +
> +	for (resno = 0; resno < PCI_NUM_RESOURCES; resno++)
> +		dev->resource[resno].flags |= IORESOURCE_PCI_FIXED;
> +
> +	for (resno = 0; resno < PCI_BRIDGE_RESOURCES; resno++) {
> +		res = &dev->resource[resno];
> +		if (res->parent || !(res->flags & IORESOURCE_MEM))
> +			continue;
> +		pci_claim_resource(dev, resno);
> +	}
> +}
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_CAVIUM, PCI_ANY_ID,
> +			pci_dev_resource_fixup);
> +
> +static void __iomem *thunder_pcie_get_cfg_addr(struct thunder_pcie *pcie,
> +					       unsigned int busnr,
> +					       unsigned int devfn, int reg)
> +{
> +	return  pcie->cfg_base +
> +		((busnr << THUNDER_PCIE_BUS_SHIFT)
> +		 | (PCI_SLOT(devfn) << THUNDER_PCIE_DEV_SHIFT)
> +		 | (PCI_FUNC(devfn) << THUNDER_PCIE_FUNC_SHIFT)) + reg;
> +}
> +
> +static int thunder_pcie_read_config(struct pci_bus *bus, unsigned int devfn,
> +				int reg, int size, u32 *val)
> +{
> +	struct thunder_pcie *pcie = bus->sysdata;
> +	void __iomem *addr;
> +	unsigned int busnr = bus->number;
> +
> +	if (busnr > 255 || devfn > 255 || reg > 4095)
> +		return PCIBIOS_DEVICE_NOT_FOUND;
> +
> +	addr = thunder_pcie_get_cfg_addr(pcie, busnr, devfn, reg);
> +
> +	switch (size) {
> +	case 1:
> +		*val = readb(addr);
> +		break;
> +	case 2:
> +		*val = readw(addr);
> +		break;
> +	case 4:
> +		*val = readl(addr);
> +		break;
> +	default:
> +		return PCIBIOS_BAD_REGISTER_NUMBER;
> +	}
> +
> +	return PCIBIOS_SUCCESSFUL;
> +}
> +
> +static int thunder_pcie_write_config(struct pci_bus *bus, unsigned int devfn,
> +				  int reg, int size, u32 val)
> +{
> +	struct thunder_pcie *pcie = bus->sysdata;
> +	void __iomem *addr;
> +	unsigned int busnr = bus->number;
> +
> +	if (busnr > 255 || devfn > 255 || reg > 4095)
> +		return PCIBIOS_DEVICE_NOT_FOUND;
> +
> +	addr = thunder_pcie_get_cfg_addr(pcie, busnr, devfn, reg);
> +
> +	switch (size) {
> +	case 1:
> +		writeb(val, addr);
> +		break;
> +	case 2:
> +		writew(val, addr);
> +		break;
> +	case 4:
> +		writel(val, addr);
> +		break;
> +	default:
> +		return PCIBIOS_BAD_REGISTER_NUMBER;
> +	}
> +
> +	return PCIBIOS_SUCCESSFUL;
> +}
> +
> +static struct pci_ops thunder_pcie_ops = {
> +	.read	= thunder_pcie_read_config,
> +	.write	= thunder_pcie_write_config,
> +};
> +
> +static int thunder_pcie_msi_enable(struct thunder_pcie *pcie,
> +					struct pci_bus *bus)
> +{
> +	struct device_node *msi_node;
> +
> +	msi_node = of_parse_phandle(pcie->node, "msi-parent", 0);
> +	if (!msi_node)
> +		return -ENODEV;
> +
> +	pcie->msi = of_pci_find_msi_chip_by_node(msi_node);
> +	if (!pcie->msi)
> +		return -ENODEV;
> +
> +	pcie->msi->dev = pcie->dev;
> +	bus->msi = pcie->msi;
> +
> +	return 0;
> +}
> +
> +static void thunder_pcie_config(struct thunder_pcie *pcie, u64 addr)
> +{
> +	atomic_set(&thunder_pcie_ecam_probed, 1);
> +	set_its_pci_requester_id(thunder_pci_requester_id);
> +
> +	pcie->valid = true;
> +
> +	switch (addr) {
> +	case THUNDER_ECAM0_CFG_BASE:
> +		pcie->ecam = 0;
> +		break;
> +	case THUNDER_ECAM1_CFG_BASE:
> +		pcie->ecam = 1;
> +		break;
> +	case THUNDER_ECAM2_CFG_BASE:
> +		pcie->ecam = 2;
> +		break;
> +	case THUNDER_ECAM3_CFG_BASE:
> +		pcie->ecam = 3;
> +		break;
> +	case THUNDER_ECAM4_CFG_BASE:
> +		pcie->ecam = 4;
> +		break;
> +	case THUNDER_ECAM5_CFG_BASE:
> +		pcie->ecam = 5;
> +		break;
> +	case THUNDER_ECAM6_CFG_BASE:
> +		pcie->ecam = 6;
> +		break;
> +	case THUNDER_ECAM7_CFG_BASE:
> +		pcie->ecam = 7;
> +		break;
> +	default:
> +		pcie->valid = false;
> +		break;
> +	}
> +}
> +
> +static int thunder_pcie_probe(struct platform_device *pdev)
> +{
> +	struct thunder_pcie *pcie;
> +	struct resource *cfg_base;
> +	struct pci_bus *bus;
> +	int ret = 0;
> +	LIST_HEAD(res);
> +
> +	pcie = devm_kzalloc(&pdev->dev, sizeof(*pcie), GFP_KERNEL);
> +	if (!pcie)
> +		return -ENOMEM;
> +
> +	pcie->node = of_node_get(pdev->dev.of_node);
> +	pcie->dev = &pdev->dev;
> +
> +	/* Get controller's configuration space range */
> +	cfg_base = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +
> +	thunder_pcie_config(pcie, cfg_base->start);
> +
> +	pcie->cfg_base = devm_ioremap_resource(&pdev->dev, cfg_base);
> +	if (IS_ERR(pcie->cfg_base)) {
> +		ret = PTR_ERR(pcie->cfg_base);
> +		goto err_ioremap;
> +	}
> +
> +	dev_info(&pdev->dev, "ECAM%d CFG BASE 0x%llx\n",
> +		 pcie->ecam, (u64)cfg_base->start);
> +
> +	ret = of_pci_get_host_bridge_resources(pdev->dev.of_node,
> +					0, 255, &res, NULL);
> +	if (ret)
> +		goto err_root_bus;
> +
> +	bus = pci_create_root_bus(&pdev->dev, 0, &thunder_pcie_ops, pcie, &res);
> +	if (!bus) {
> +		ret = -ENODEV;
> +		goto err_root_bus;
> +	}
> +
> +	/* Set reference to MSI chip */
> +	ret = thunder_pcie_msi_enable(pcie, bus);
> +	if (ret) {
> +		dev_err(&pdev->dev,
> +			"Unable to set reference to MSI chip: ret=%d\n", ret);
> +		goto err_msi;
> +	}
> +
> +	platform_set_drvdata(pdev, pcie);
> +
> +	pci_scan_child_bus(bus);
> +	pci_bus_add_devices(bus);
> +
> +	return 0;
> +err_msi:
> +	pci_remove_root_bus(bus);
> +err_root_bus:
> +	pci_free_resource_list(&res);
> +err_ioremap:
> +	of_node_put(pcie->node);
> +	return ret;
> +}
> +
> +static const struct of_device_id thunder_pcie_of_match[] = {
> +	{ .compatible = "cavium,thunder-pcie", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, thunder_pcie_of_match);
> +
> +static struct platform_driver thunder_pcie_driver = {
> +	.driver = {
> +		.name = "thunder-pcie",
> +		.owner = THIS_MODULE,
> +		.of_match_table = thunder_pcie_of_match,
> +	},
> +	.probe = thunder_pcie_probe,
> +};
> +module_platform_driver(thunder_pcie_driver);
> +
> +#ifdef CONFIG_ACPI
> +
> +static int
> +thunder_mmcfg_read_config(struct pci_mmcfg_region *cfg, unsigned int busnr,
> +			unsigned int devfn, int reg, int len, u32 *value)
> +{
> +	struct thunder_pcie *pcie = cfg->data;
> +	void __iomem *addr;
> +
> +	if (!pcie->valid) {
> +		/* Not support for now */
> +		pr_err("RC PEM not supported !!!\n");
> +		return PCIBIOS_DEVICE_NOT_FOUND;
> +	}
> +
> +	addr = thunder_pcie_get_cfg_addr(pcie, busnr, devfn, reg);
> +
> +	switch (len) {
> +	case 1:
> +		*value = readb(addr);
> +		break;
> +	case 2:
> +		*value = readw(addr);
> +		break;
> +	case 4:
> +		*value = readl(addr);
> +		break;
> +	default:
> +		return PCIBIOS_BAD_REGISTER_NUMBER;
> +	}
> +
> +	return PCIBIOS_SUCCESSFUL;
> +}
> +
> +static int thunder_mmcfg_write_config(struct pci_mmcfg_region *cfg,
> +		unsigned int busnr, unsigned int devfn, int reg, int len,
> +		u32 value) {
> +	struct thunder_pcie *pcie = cfg->data;
> +	void __iomem *addr;
> +
> +	if (!pcie->valid) {
> +		/* Not support for now */
> +		pr_err("RC PEM not supported !!!\n");
> +		return PCIBIOS_DEVICE_NOT_FOUND;
> +	}
> +
> +	addr = thunder_pcie_get_cfg_addr(pcie, busnr, devfn, reg);
> +
> +	switch (len) {
> +	case 1:
> +		writeb(value, addr);
> +		break;
> +	case 2:
> +		writew(value, addr);
> +		break;
> +	case 4:
> +		writel(value, addr);
> +		break;
> +	default:
> +		return PCIBIOS_BAD_REGISTER_NUMBER;
> +	}
> +
> +	return PCIBIOS_SUCCESSFUL;
> +}
> +
> +static int thunder_acpi_mcfg_fixup(struct acpi_pci_root *root,
> +				   struct pci_mmcfg_region *cfg)
> +{
> +	struct thunder_pcie *pcie;
> +
> +	pcie = kzalloc(sizeof(*pcie), GFP_KERNEL);
> +	if (!pcie)
> +		return -ENOMEM;
> +
> +	pcie->dev = &root->device->dev;
> +
> +	thunder_pcie_config(pcie, cfg->address);
> +
> +	pcie->cfg_base = cfg->virt;
> +	cfg->data = pcie;
> +	cfg->read = thunder_mmcfg_read_config;
> +	cfg->write = thunder_mmcfg_write_config;
> +
> +	return 0;
> +}
> +DECLARE_ACPI_MCFG_FIXUP("CAVIUM", "THUNDERX", thunder_acpi_mcfg_fixup);

Why do we have different behaviours between DT and ACPI?

> +#endif
> +
> +MODULE_AUTHOR("Sunil Goutham");
> +MODULE_DESCRIPTION("Cavium Thunder ECAM host controller driver");
> +MODULE_LICENSE("GPL v2");
> 

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH 5/5] PCI: Add host drivers for Cavium ThunderX processors.

[Paul Bolle]

On wo, 2015-07-15 at 09:54 -0700, David Daney wrote:
> --- a/drivers/pci/host/Kconfig
> +++ b/drivers/pci/host/Kconfig

> +config PCI_THUNDER_PEM
> +	bool

Do you expect other symbols to select this symbol in the future? Because
at this moment PCI_THUNDER_PEM is merely an alias for PCI_THUNDER.

> +config PCI_THUNDER
> +	bool "Thunder PCIe host controller"
> +	depends on ARM64 || COMPILE_TEST
> +	depends on OF_PCI
> +	select PCI_MSI
> +	select PCI_THUNDER_PEM
> +	help
> +	  Say Y here if you want internal PCI support on Thunder SoC.

> --- a/drivers/pci/host/Makefile
> +++ b/drivers/pci/host/Makefile

> +obj-$(CONFIG_PCI_THUNDER) += pcie-thunder.o
> +obj-$(CONFIG_PCI_THUNDER_PEM) += pcie-thunder-pem.o

> --- /dev/null
> +++ b/drivers/pci/host/pcie-thunder-pem.c

> +#include <linux/module.h>

> +MODULE_DEVICE_TABLE(pci, thunder_pem_pci_table);

> +static int __init thunder_pcie_init(void)
> +{
> +	int ret;
> +
> +	ret = pci_register_driver(&thunder_pem_driver);
> +
> +	return ret;
> +}
> +module_init(thunder_pcie_init);
> +
> +static void __exit thunder_pcie_exit(void)
> +{
> +	pci_unregister_driver(&thunder_pem_driver);
> +}
> +module_exit(thunder_pcie_exit);

> --- /dev/null
> +++ b/drivers/pci/host/pcie-thunder.c

> +#include <linux/module.h>

> +MODULE_DEVICE_TABLE(of, thunder_pcie_of_match);
> +
> +static struct platform_driver thunder_pcie_driver = {
> +	.driver = {
> +		[...]
> +		.owner = THIS_MODULE,
> +		[...]
> +};
> +module_platform_driver(thunder_pcie_driver);

> +MODULE_AUTHOR("Sunil Goutham");
> +MODULE_DESCRIPTION("Cavium Thunder ECAM host controller driver");
> +MODULE_LICENSE("GPL v2");

This patch adds two bool Kconfig symbols and two files that can only be
built-in if these symbols are set, right?

But the code uses various constructs that are only useful for modular
code (MODULE_DEVICE_TABLE, __exit, module_exit, THIS_MODULE, and the
three MODULE_* macros) or have built-in alternatives (module_init and
module_platform_driver).

Was it your intention to make this code built-in or modular?

Thanks,


Paul Bolle

[PATCH 5/5] PCI: Add host drivers for Cavium ThunderX processors.

[David Daney]

On 07/16/2015 01:31 AM, Paul Bolle wrote:
> On wo, 2015-07-15 at 09:54 -0700, David Daney wrote:
>> --- a/drivers/pci/host/Kconfig
>> +++ b/drivers/pci/host/Kconfig
>
>> +config PCI_THUNDER_PEM
>> +	bool
>
> Do you expect other symbols to select this symbol in the future?

No, ...

> Because
> at this moment PCI_THUNDER_PEM is merely an alias for PCI_THUNDER.
>

... the next version of the patch will uncouple them.  They are separate 
drivers and should be selectable independently.

Thanks,
David Daney

[PATCH 5/5] PCI: Add host drivers for Cavium ThunderX processors.

[Lorenzo Pieralisi]

Hi David,

On Wed, Jul 15, 2015 at 05:54:45PM +0100, David Daney wrote:
> From: David Daney <david.daney@cavium.com>
> 

-ENOCOMMITLOG

> Signed-off-by: David Daney <david.daney@cavium.com>
> ---
>  drivers/pci/host/Kconfig            |  12 +
>  drivers/pci/host/Makefile           |   2 +
>  drivers/pci/host/pcie-thunder-pem.c | 462 ++++++++++++++++++++++++++++++++++++
>  drivers/pci/host/pcie-thunder.c     | 422 ++++++++++++++++++++++++++++++++
>  4 files changed, 898 insertions(+)
>  create mode 100644 drivers/pci/host/pcie-thunder-pem.c
>  create mode 100644 drivers/pci/host/pcie-thunder.c
> 
> diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
> index c132bdd..06e26ad 100644
> --- a/drivers/pci/host/Kconfig
> +++ b/drivers/pci/host/Kconfig
> @@ -145,4 +145,16 @@ config PCIE_IPROC_BCMA
>           Say Y here if you want to use the Broadcom iProc PCIe controller
>           through the BCMA bus interface
> 
> +config PCI_THUNDER_PEM
> +       bool
> +
> +config PCI_THUNDER
> +       bool "Thunder PCIe host controller"
> +       depends on ARM64 || COMPILE_TEST
> +       depends on OF_PCI
> +       select PCI_MSI
> +       select PCI_THUNDER_PEM
> +       help
> +         Say Y here if you want internal PCI support on Thunder SoC.
> +
>  endmenu
> diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
> index 140d66f..a355155 100644
> --- a/drivers/pci/host/Makefile
> +++ b/drivers/pci/host/Makefile
> @@ -17,3 +17,5 @@ obj-$(CONFIG_PCI_VERSATILE) += pci-versatile.o
>  obj-$(CONFIG_PCIE_IPROC) += pcie-iproc.o
>  obj-$(CONFIG_PCIE_IPROC_PLATFORM) += pcie-iproc-platform.o
>  obj-$(CONFIG_PCIE_IPROC_BCMA) += pcie-iproc-bcma.o
> +obj-$(CONFIG_PCI_THUNDER) += pcie-thunder.o
> +obj-$(CONFIG_PCI_THUNDER_PEM) += pcie-thunder-pem.o
> diff --git a/drivers/pci/host/pcie-thunder-pem.c b/drivers/pci/host/pcie-thunder-pem.c
> new file mode 100644
> index 0000000..7861a8a
> --- /dev/null
> +++ b/drivers/pci/host/pcie-thunder-pem.c
> @@ -0,0 +1,462 @@
> +/*
> + * PCIe host controller driver for Cavium Thunder SOC
> + *
> + * Copyright (C) 2014,2015 Cavium Inc.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + */
> +/* #define DEBUG 1 */
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/delay.h>
> +#include <linux/irq.h>
> +#include <linux/pci.h>
> +#include <linux/irqdomain.h>
> +#include <linux/msi.h>
> +#include <linux/irqchip/arm-gic-v3.h>
> +
> +#define THUNDER_SLI_S2M_REG_ACC_BASE   0x874001000000ull
> +
> +#define THUNDER_GIC                    0x801000000000ull
> +#define THUNDER_GICD_SETSPI_NSR                0x801000000040ull
> +#define THUNDER_GICD_CLRSPI_NSR                0x801000000048ull
> +
> +#define THUNDER_GSER_PCIE_MASK         0x01
> +
> +#define PEM_CTL_STATUS 0x000
> +#define PEM_RD_CFG     0x030
> +#define P2N_BAR0_START 0x080
> +#define P2N_BAR1_START 0x088
> +#define P2N_BAR2_START 0x090
> +#define BAR_CTL                0x0a8
> +#define BAR2_MASK      0x0b0
> +#define BAR1_INDEX     0x100
> +#define PEM_CFG                0x410
> +#define PEM_ON         0x420
> +
> +struct thunder_pem {
> +       struct list_head list; /* on thunder_pem_buses */
> +       bool            connected;
> +       unsigned int    id;
> +       unsigned int    sli;
> +       unsigned int    sli_group;
> +       unsigned int    node;
> +       u64             sli_window_base;
> +       void __iomem    *bar0;
> +       void __iomem    *bar4;
> +       void __iomem    *sli_s2m;
> +       void __iomem    *cfgregion;
> +       struct pci_bus  *bus;
> +       int             vwire_irqs[4];
> +       u32             vwire_data[4];
> +};
> +
> +static LIST_HEAD(thunder_pem_buses);
> +
> +static struct pci_device_id thunder_pem_pci_table[] = {
> +       {PCI_VENDOR_ID_CAVIUM, 0xa020, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0},
> +       {0,}
> +};
> +MODULE_DEVICE_TABLE(pci, thunder_pem_pci_table);
> +
> +enum slix_s2m_ctype {
> +       CTYPE_MEMORY    = 0,
> +       CTYPE_CONFIG    = 1,
> +       CTYPE_IO        = 2
> +};
> +
> +static u64 slix_s2m_reg_val(unsigned mac, enum slix_s2m_ctype ctype,
> +                           bool merge, bool relaxed, bool snoop, u32 ba_msb)
> +{
> +       u64 v;
> +
> +       v = (u64)(mac % 3) << 49;
> +       v |= (u64)ctype << 53;
> +       if (!merge)
> +               v |= 1ull << 48;
> +       if (relaxed)
> +               v |= 5ull << 40;
> +       if (!snoop)
> +               v |= 5ull << 41;
> +       v |= (u64)ba_msb;
> +
> +       return v;
> +}
> +
> +static u32 thunder_pcierc_config_read(struct thunder_pem *pem, u32 reg, int size)
> +{
> +       unsigned int val;
> +
> +       writeq(reg & ~3u, pem->bar0 + PEM_RD_CFG);
> +       val = readq(pem->bar0 + PEM_RD_CFG) >> 32;
> +
> +       if (size == 1)
> +               val = (val >> (8 * (reg & 3))) & 0xff;
> +       else if (size == 2)
> +               val = (val >> (8 * (reg & 3))) & 0xffff;
> +
> +       return val;
> +}
> +
> +static int thunder_pem_read_config(struct pci_bus *bus, unsigned int devfn,
> +                                  int reg, int size, u32 *val)
> +{
> +       void __iomem *addr;
> +       struct thunder_pem *pem = bus->sysdata;
> +       unsigned int busnr = bus->number;
> +
> +       if (busnr > 255 || devfn > 255 || reg > 4095)
> +               return PCIBIOS_DEVICE_NOT_FOUND;
> +
> +       addr = pem->cfgregion + ((busnr << 24)  | (devfn << 16) | reg);
> +
> +       switch (size) {
> +       case 1:
> +               *val = readb(addr);
> +               break;
> +       case 2:
> +               *val = readw(addr);
> +               break;
> +       case 4:
> +               *val = readl(addr);
> +               break;
> +       default:
> +               return PCIBIOS_BAD_REGISTER_NUMBER;
> +       }
> +
> +       return PCIBIOS_SUCCESSFUL;
> +}
> +
> +static int thunder_pem_write_config(struct pci_bus *bus, unsigned int devfn,
> +                                   int reg, int size, u32 val)
> +{
> +       void __iomem *addr;
> +       struct thunder_pem *pem = bus->sysdata;
> +       unsigned int busnr = bus->number;
> +
> +       if (busnr > 255 || devfn > 255 || reg > 4095)
> +               return PCIBIOS_DEVICE_NOT_FOUND;
> +
> +       addr = pem->cfgregion + ((busnr << 24)  | (devfn << 16) | reg);
> +
> +       switch (size) {
> +       case 1:
> +               writeb(val, addr);
> +               break;
> +       case 2:
> +               writew(val, addr);
> +               break;
> +       case 4:
> +               writel(val, addr);
> +               break;
> +       default:
> +               return PCIBIOS_BAD_REGISTER_NUMBER;
> +       }
> +
> +       return PCIBIOS_SUCCESSFUL;
> +}
> +
> +static struct pci_ops thunder_pem_ops = {
> +       .read   = thunder_pem_read_config,
> +       .write  = thunder_pem_write_config,
> +};

Why can't you use generic PCI config accessors (struct gen_pci_cfg_bus_ops) ?

> +static struct thunder_pem *thunder_pem_from_dev(struct pci_dev *dev)
> +{
> +       struct thunder_pem *pem;
> +       struct pci_bus *bus = dev->bus;
> +
> +       while (!pci_is_root_bus(bus))
> +               bus = bus->parent;
> +
> +       list_for_each_entry(pem, &thunder_pem_buses, list) {
> +               if (pem->bus == bus)
> +                       return pem;
> +       }
> +       return NULL;
> +}
> +
> +int thunder_pem_requester_id(struct pci_dev *dev)
> +{
> +       struct thunder_pem *pem = thunder_pem_from_dev(dev);
> +
> +       if (!pem)
> +               return -ENODEV;
> +
> +       if (pem->id < 3)
> +               return ((1 << 16) |
> +                       ((dev)->bus->number << 8) |
> +                       (dev)->devfn);
> +
> +       if (pem->id < 6)
> +               return ((3 << 16) |
> +                       ((dev)->bus->number << 8) |
> +                       (dev)->devfn);
> +
> +       if (pem->id < 9)
> +               return ((1 << 19) | (1 << 16) |
> +                       ((dev)->bus->number << 8) |
> +                       (dev)->devfn);
> +
> +       if (pem->id < 12)
> +               return ((1 << 19) |
> +                       (3 << 16) |
> +                       ((dev)->bus->number << 8) |
> +                       (dev)->devfn);
> +       return -ENODEV;
> +}
> +
> +static int thunder_pem_pcibios_add_device(struct pci_dev *dev)
> +{
> +       struct thunder_pem *pem;
> +       u8 pin;
> +
> +       pem = thunder_pem_from_dev(dev);
> +       if (!pem)
> +               return 0;
> +
> +       pci_read_config_byte(dev, PCI_INTERRUPT_PIN, &pin);
> +
> +       /* Cope with illegal. */
> +       if (pin > 4)
> +               pin = 1;
> +
> +       dev->irq = pin > 0 ? pem->vwire_irqs[pin - 1] : 0;
> +
> +       if (pin)
> +               dev_dbg(&dev->dev, "assigning IRQ %02d\n", dev->irq);
> +
> +       pci_write_config_byte(dev, PCI_INTERRUPT_LINE, dev->irq);

Suspiciously similar to pci_fixup_irqs, so use that + proper DT
bindings and remove this function (and drop patch 3).

> +
> +       return 0;
> +}
> +
> +static int thunder_pem_pci_probe(struct pci_dev *pdev,
> +                                const struct pci_device_id *ent)
> +{
> +       struct thunder_pem *pem;
> +       resource_size_t bar0_start;
> +       u64 regval;
> +       u64 sliaddr, pciaddr;
> +       u32 cfgval;
> +       int primary_bus;
> +       int i;
> +       int ret = 0;
> +       struct resource *res;
> +       LIST_HEAD(resources);
> +
> +       set_pcibios_add_device(thunder_pem_pcibios_add_device);

This has to go, see above.

> +
> +       pem = devm_kzalloc(&pdev->dev, sizeof(*pem), GFP_KERNEL);
> +       if (!pem)
> +               return -ENOMEM;
> +
> +       pci_set_drvdata(pdev, pem);
> +
> +       bar0_start = pci_resource_start(pdev, 0);
> +       pem->node = (bar0_start >> 44) & 3;
> +       pem->id = ((bar0_start >> 24) & 7) + (6 * pem->node);
> +       pem->sli = pem->id % 3;
> +       pem->sli_group = (pem->id / 3) % 2;
> +       pem->sli_window_base = 0x880000000000ull | (((u64)pem->node) << 44) | ((u64)pem->sli_group << 40);
> +       pem->sli_window_base += 0x4000000000 * pem->sli;
> +
> +       ret = pci_enable_device_mem(pdev);
> +       if (ret)
> +               goto out;
> +
> +       pem->bar0 = pcim_iomap(pdev, 0, 0x100000);
> +       if (!pem->bar0) {
> +               ret = -ENOMEM;
> +               goto out;
> +       }
> +
> +       pem->bar4 = pcim_iomap(pdev, 4, 0x100000);
> +       if (!pem->bar0) {
> +               ret = -ENOMEM;
> +               goto out;
> +       }
> +
> +       sliaddr = THUNDER_SLI_S2M_REG_ACC_BASE | ((u64)pem->node << 44) | ((u64)pem->sli_group << 36);
> +
> +       regval = readq(pem->bar0 + PEM_ON);
> +       if (!(regval & 1)) {
> +               dev_notice(&pdev->dev, "PEM%u_ON not set, skipping...\n", pem->id);
> +               goto out;
> +       }
> +
> +       regval = readq(pem->bar0 + PEM_CTL_STATUS);
> +       regval |= 0x10; /* Set Link Enable bit */
> +       writeq(regval, pem->bar0 + PEM_CTL_STATUS);
> +
> +       udelay(1000);
> +
> +       cfgval = thunder_pcierc_config_read(pem, 32 * 4, 4); /* PCIERC_CFG032 */
> +
> +       if (((cfgval >> 29 & 0x1) == 0x0) || ((cfgval >> 27 & 0x1) == 0x1)) {
> +               dev_notice(&pdev->dev, "PEM%u Link Timeout, skipping...\n", pem->id);
> +               goto out;
> +       }
> +
> +       pem->sli_s2m = devm_ioremap(&pdev->dev, sliaddr, 0x1000);
> +       if (!pem->sli_s2m) {
> +               ret = -ENOMEM;
> +               goto out;
> +       }

I would expect most of the configuration above can be moved to FW so
that you can reuse the PCI generic host in the kernel. And all these
hardcoded magic numbers are quite irritating to start with.

> +
> +       pem->cfgregion = devm_ioremap(&pdev->dev, pem->sli_window_base, 0x100000000ull);
> +       if (!pem->cfgregion) {
> +               ret = -ENOMEM;
> +               goto out;
> +       }
> +       regval = slix_s2m_reg_val(pem->sli, CTYPE_CONFIG, false, false, false, 0);
> +       writeq(regval, pem->sli_s2m + 0x10 * ((0x40 * pem->sli) + 0));
> +
> +       cfgval = thunder_pcierc_config_read(pem, 6 * 4, 4); /* PCIERC_CFG006 */
> +       primary_bus = (cfgval >> 8) & 0xff;
> +
> +       res = kzalloc(sizeof(*res), GFP_KERNEL);
> +       if (!res) {
> +               ret = -ENOMEM;
> +               goto out;
> +       }
> +       res->start = primary_bus;
> +       res->end = 255;
> +       res->flags = IORESOURCE_BUS;
> +       pci_add_resource(&resources, res);
> +
> +
> +       res = kzalloc(sizeof(*res), GFP_KERNEL);
> +       if (!res) {
> +               ret = -ENOMEM;
> +               goto out;

Leaking memory, but that's not the (only) point. These resources should be
parsed from DT (of_pci_get_host_bridge_resources(), why it is done in
the driver below and not here is beyond my understanding, please
explain).

> +       }
> +       res->start = 0x100000 * pem->id;
> +       res->end = res->start + 0x100000 - 1;
> +       res->flags = IORESOURCE_IO;
> +       pci_add_resource(&resources, res);
> +       regval = slix_s2m_reg_val(pem->sli, CTYPE_IO, false, false, false, 0);
> +       writeq(regval, pem->sli_s2m + 0x10 * ((0x40 * pem->sli) + 1));
> +
> +       res = kzalloc(sizeof(*res), GFP_KERNEL);
> +       if (!res) {
> +               ret = -ENOMEM;
> +               goto out;

Ditto

> +       }
> +       pciaddr = 0x10000000ull;
> +       res->start = pem->sli_window_base + 0x1000000000ull + pciaddr;
> +       res->end = res->start + 0x1000000000ull - pciaddr - 1;
> +       res->flags = IORESOURCE_MEM;
> +       pci_add_resource_offset(&resources, res, res->start - pciaddr);
> +       for (i = 0; i < 16; i++) {
> +               regval = slix_s2m_reg_val(pem->sli, CTYPE_MEMORY, false, false, false, i);
> +               writeq(regval, pem->sli_s2m + 0x10 * ((0x40 * pem->sli) + (0x10 + i)));
> +       }
> +
> +       res = kzalloc(sizeof(*res), GFP_KERNEL);
> +       if (!res) {
> +               ret = -ENOMEM;
> +               goto out;

Ditto

> +       }
> +       pciaddr = 0x1000000000ull;
> +       res->start = pem->sli_window_base + 0x1000000000ull + pciaddr;
> +       res->end = res->start + 0x1000000000ull - 1;
> +       res->flags = IORESOURCE_MEM | IORESOURCE_PREFETCH;
> +       pci_add_resource_offset(&resources, res, res->start - pciaddr);
> +       for (i = 0; i < 16; i++) {
> +               regval = slix_s2m_reg_val(pem->sli, CTYPE_MEMORY, true, true, true, i + 0x10);
> +               writeq(regval, pem->sli_s2m + 0x10 * ((0x40 * pem->sli) + (0x20 + i)));
> +       }
> +
> +       writeq(0, pem->bar0 + P2N_BAR0_START);
> +       writeq(0, pem->bar0 + P2N_BAR1_START);
> +       writeq(0, pem->bar0 + P2N_BAR2_START);
> +
> +       regval = 0x10;  /* BAR_CTL[BAR1_SIZ] = 1 (64MB) */
> +       regval |= 0x8;  /* BAR_CTL[BAR2_ENB] = 1 */
> +       writeq(regval, pem->bar0 + BAR_CTL);
> +
> +       /* 1st 4MB region -> GIC registers so 32-bit MSI can reach the GIC. */
> +       regval = (THUNDER_GIC + (((u64)pem->node) << 44)) >> 18;
> +       /* BAR1_INDEX[ADDR_V] = 1 */
> +       regval |= 1;
> +       writeq(regval, pem->bar0 + BAR1_INDEX);
> +       /* Remaining regions linear mapping to physical address space */
> +       for (i = 1; i < 16; i++) {
> +               regval = (i << 4) | 1;
> +               writeq(regval, pem->bar0 + BAR1_INDEX + 8 * i);
> +       }
> +
> +       pem->bus = pci_create_root_bus(&pdev->dev, primary_bus, &thunder_pem_ops, pem, &resources);
> +       if (!pem->bus) {
> +               ret = -ENODEV;
> +               goto err_root_bus;
> +       }
> +       pem->bus->is_pcierc = 1;
> +       list_add_tail(&pem->list, &thunder_pem_buses);
> +
> +       for (i = 0; i < 3; i++) {
> +               pem->vwire_data[i] = 40 + 4 * pem->id + i;
> +               pem->vwire_irqs[i] = irq_create_mapping(gic_get_irq_domain(), pem->vwire_data[i]);
> +               if (!pem->vwire_irqs[i]) {
> +                       dev_err(&pdev->dev, "Error: No irq mapping for %u\n", pem->vwire_data[i]);
> +                       continue;
> +               }
> +               irq_set_irq_type(pem->vwire_irqs[i], IRQ_TYPE_LEVEL_HIGH);
> +
> +               writeq(THUNDER_GICD_SETSPI_NSR, pem->bar4 + 0 + (i + 2) * 32);
> +               writeq(pem->vwire_data[i],      pem->bar4 + 8 + (i + 2) * 32);
> +               writeq(THUNDER_GICD_CLRSPI_NSR, pem->bar4 + 16 + (i + 2) * 32);
> +               writeq(pem->vwire_data[i],      pem->bar4 + 24 + (i + 2) * 32);
> +       }
> +       ret = pci_read_config_dword(pdev, 44 * 4, &cfgval);
> +       if (WARN_ON(ret))
> +               goto err_free_root_bus;
> +       cfgval &= ~0x40000000; /* Clear FUNM */
> +       cfgval |= 0x80000000;  /* Set MSIXEN */
> +       pci_write_config_dword(pdev, 44 * 4, cfgval);
> +       pem->bus->msi = pdev->bus->msi;
> +
> +       pci_scan_child_bus(pem->bus);
> +       pci_bus_add_devices(pem->bus);
> +       pci_assign_unassigned_root_bus_resources(pem->bus);
> +
> +       return 0;
> +
> +err_free_root_bus:
> +       pci_remove_root_bus(pem->bus);
> +err_root_bus:
> +       pci_free_resource_list(&resources);
> +out:
> +       return ret;
> +}
> +
> +static void thunder_pem_pci_remove(struct pci_dev *pdev)
> +{
> +}
> +
> +static struct pci_driver thunder_pem_driver = {
> +       .name           = "thunder_pem",
> +       .id_table       = thunder_pem_pci_table,
> +       .probe          = thunder_pem_pci_probe,
> +       .remove         = thunder_pem_pci_remove
> +};
> +
> +static int __init thunder_pcie_init(void)
> +{
> +       int ret;
> +
> +       ret = pci_register_driver(&thunder_pem_driver);
> +
> +       return ret;
> +}
> +module_init(thunder_pcie_init);
> +
> +static void __exit thunder_pcie_exit(void)
> +{
> +       pci_unregister_driver(&thunder_pem_driver);
> +}
> +module_exit(thunder_pcie_exit);
> diff --git a/drivers/pci/host/pcie-thunder.c b/drivers/pci/host/pcie-thunder.c
> new file mode 100644
> index 0000000..4abab5a
> --- /dev/null
> +++ b/drivers/pci/host/pcie-thunder.c
> @@ -0,0 +1,422 @@
> +/*
> + * PCIe host controller driver for Cavium Thunder SOC
> + *
> + * Copyright (C) 2014, 2015 Cavium Inc.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_pci.h>
> +#include <linux/pci.h>
> +#include <linux/platform_device.h>
> +#include <linux/msi.h>
> +#include <linux/irqchip/arm-gic-v3.h>
> +
> +#define PCI_DEVICE_ID_THUNDER_BRIDGE   0xa002
> +
> +#define THUNDER_PCIE_BUS_SHIFT         20
> +#define THUNDER_PCIE_DEV_SHIFT         15
> +#define THUNDER_PCIE_FUNC_SHIFT                12

What's THUNDER specific in this ?

> +#define THUNDER_ECAM0_CFG_BASE         0x848000000000
> +#define THUNDER_ECAM1_CFG_BASE         0x849000000000
> +#define THUNDER_ECAM2_CFG_BASE         0x84a000000000
> +#define THUNDER_ECAM3_CFG_BASE         0x84b000000000
> +#define THUNDER_ECAM4_CFG_BASE         0x948000000000
> +#define THUNDER_ECAM5_CFG_BASE         0x949000000000
> +#define THUNDER_ECAM6_CFG_BASE         0x94a000000000
> +#define THUNDER_ECAM7_CFG_BASE         0x94b000000000

You are not telling me you are hardcoding config base addresses
here, are you ?

> +struct thunder_pcie {
> +       struct device_node      *node;
> +       struct device           *dev;
> +       void __iomem            *cfg_base;
> +       struct msi_controller   *msi;
> +       int                     ecam;
> +       bool                    valid;
> +};
> +
> +int thunder_pem_requester_id(struct pci_dev *dev);
> +
> +static atomic_t thunder_pcie_ecam_probed;
> +
> +static u32 pci_requester_id_ecam(struct pci_dev *dev)
> +{
> +       return (((pci_domain_nr(dev->bus) >> 2) << 19) |
> +               ((pci_domain_nr(dev->bus) % 4) << 16) |
> +               (dev->bus->number << 8) | dev->devfn);
> +}
> +
> +static u32 thunder_pci_requester_id(struct pci_dev *dev, u16 alias)
> +{
> +       int ret;
> +
> +       ret = thunder_pem_requester_id(dev);
> +       if (ret >= 0)
> +               return (u32)ret;
> +
> +       return pci_requester_id_ecam(dev);
> +}
> +
> +/*
> + * This bridge is just for the sake of supporting ARI for
> + * downstream devices. No resources are attached to it.
> + * Copy upstream root bus resources to bridge which aide in
> + * resource claiming for downstream devices
> + */
> +static void pci_bridge_resource_fixup(struct pci_dev *dev)
> +{
> +       struct pci_bus *bus;
> +       int resno;
> +
> +       bus = dev->subordinate;
> +       for (resno = 0; resno < PCI_BRIDGE_RESOURCE_NUM; resno++) {
> +               bus->resource[resno] = pci_bus_resource_n(bus->parent,
> +                       PCI_BRIDGE_RESOURCE_NUM + resno);
> +       }
> +
> +       for (resno = PCI_BRIDGE_RESOURCES;
> +               resno <= PCI_BRIDGE_RESOURCE_END; resno++) {
> +               dev->resource[resno].start = dev->resource[resno].end = 0;
> +               dev->resource[resno].flags = 0;
> +       }
> +}
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_CAVIUM, PCI_DEVICE_ID_THUNDER_BRIDGE,
> +                       pci_bridge_resource_fixup);

Wouldn't pci_read_bridge_bases() do what you need here ? It will be
called from core code. If it does not please explain to me what
you want to achieve, thank you.

> + * All PCIe devices in Thunder have fixed resources, shouldn't be reassigned.
> + * Also claim the device's valid resources to set 'res->parent' hierarchy.
> + */
> +static void pci_dev_resource_fixup(struct pci_dev *dev)
> +{
> +       struct resource *res;
> +       int resno;
> +
> +       /*
> +        * If the ECAM is not yet probed, we must be in a virtual
> +        * machine.  In that case, don't mark things as
> +        * IORESOURCE_PCI_FIXED
> +        */
> +       if (!atomic_read(&thunder_pcie_ecam_probed))
> +               return;

I do not understand the logic here, and to be frank that seems just
a royal hack to me regardless.

> +       for (resno = 0; resno < PCI_NUM_RESOURCES; resno++)
> +               dev->resource[resno].flags |= IORESOURCE_PCI_FIXED;
> +
> +       for (resno = 0; resno < PCI_BRIDGE_RESOURCES; resno++) {
> +               res = &dev->resource[resno];
> +               if (res->parent || !(res->flags & IORESOURCE_MEM))
> +                       continue;
> +               pci_claim_resource(dev, resno);
> +       }
> +}
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_CAVIUM, PCI_ANY_ID,
> +                       pci_dev_resource_fixup);
> +
> +static void __iomem *thunder_pcie_get_cfg_addr(struct thunder_pcie *pcie,
> +                                              unsigned int busnr,
> +                                              unsigned int devfn, int reg)
> +{
> +       return  pcie->cfg_base +
> +               ((busnr << THUNDER_PCIE_BUS_SHIFT)
> +                | (PCI_SLOT(devfn) << THUNDER_PCIE_DEV_SHIFT)
> +                | (PCI_FUNC(devfn) << THUNDER_PCIE_FUNC_SHIFT)) + reg;
> +}
> +
> +static int thunder_pcie_read_config(struct pci_bus *bus, unsigned int devfn,
> +                               int reg, int size, u32 *val)
> +{
> +       struct thunder_pcie *pcie = bus->sysdata;
> +       void __iomem *addr;
> +       unsigned int busnr = bus->number;
> +
> +       if (busnr > 255 || devfn > 255 || reg > 4095)
> +               return PCIBIOS_DEVICE_NOT_FOUND;
> +
> +       addr = thunder_pcie_get_cfg_addr(pcie, busnr, devfn, reg);
> +
> +       switch (size) {
> +       case 1:
> +               *val = readb(addr);
> +               break;
> +       case 2:
> +               *val = readw(addr);
> +               break;
> +       case 4:
> +               *val = readl(addr);
> +               break;
> +       default:
> +               return PCIBIOS_BAD_REGISTER_NUMBER;
> +       }
> +
> +       return PCIBIOS_SUCCESSFUL;
> +}
> +
> +static int thunder_pcie_write_config(struct pci_bus *bus, unsigned int devfn,
> +                                 int reg, int size, u32 val)
> +{
> +       struct thunder_pcie *pcie = bus->sysdata;
> +       void __iomem *addr;
> +       unsigned int busnr = bus->number;
> +
> +       if (busnr > 255 || devfn > 255 || reg > 4095)
> +               return PCIBIOS_DEVICE_NOT_FOUND;
> +
> +       addr = thunder_pcie_get_cfg_addr(pcie, busnr, devfn, reg);
> +
> +       switch (size) {
> +       case 1:
> +               writeb(val, addr);
> +               break;
> +       case 2:
> +               writew(val, addr);
> +               break;
> +       case 4:
> +               writel(val, addr);
> +               break;
> +       default:
> +               return PCIBIOS_BAD_REGISTER_NUMBER;
> +       }
> +
> +       return PCIBIOS_SUCCESSFUL;
> +}
> +
> +static struct pci_ops thunder_pcie_ops = {
> +       .read   = thunder_pcie_read_config,
> +       .write  = thunder_pcie_write_config,
> +};

I have already comment on this above.

> +static int thunder_pcie_msi_enable(struct thunder_pcie *pcie,
> +                                       struct pci_bus *bus)
> +{
> +       struct device_node *msi_node;
> +
> +       msi_node = of_parse_phandle(pcie->node, "msi-parent", 0);
> +       if (!msi_node)
> +               return -ENODEV;
> +
> +       pcie->msi = of_pci_find_msi_chip_by_node(msi_node);
> +       if (!pcie->msi)
> +               return -ENODEV;
> +
> +       pcie->msi->dev = pcie->dev;
> +       bus->msi = pcie->msi;
> +
> +       return 0;
> +}
> +
> +static void thunder_pcie_config(struct thunder_pcie *pcie, u64 addr)
> +{
> +       atomic_set(&thunder_pcie_ecam_probed, 1);
> +       set_its_pci_requester_id(thunder_pci_requester_id);
> +
> +       pcie->valid = true;
> +
> +       switch (addr) {
> +       case THUNDER_ECAM0_CFG_BASE:
> +               pcie->ecam = 0;
> +               break;
> +       case THUNDER_ECAM1_CFG_BASE:
> +               pcie->ecam = 1;
> +               break;
> +       case THUNDER_ECAM2_CFG_BASE:
> +               pcie->ecam = 2;
> +               break;
> +       case THUNDER_ECAM3_CFG_BASE:
> +               pcie->ecam = 3;
> +               break;
> +       case THUNDER_ECAM4_CFG_BASE:
> +               pcie->ecam = 4;
> +               break;
> +       case THUNDER_ECAM5_CFG_BASE:
> +               pcie->ecam = 5;
> +               break;
> +       case THUNDER_ECAM6_CFG_BASE:
> +               pcie->ecam = 6;
> +               break;
> +       case THUNDER_ECAM7_CFG_BASE:
> +               pcie->ecam = 7;
> +               break;
> +       default:
> +               pcie->valid = false;
> +               break;
> +       }

My eyes. You are hardcoding config addresses to check their validity
and just to set a number (ecam) to use in a dev_info call ? I'll pretend I
have not seen this code, please remove it.

> +}
> +
> +static int thunder_pcie_probe(struct platform_device *pdev)
> +{
> +       struct thunder_pcie *pcie;
> +       struct resource *cfg_base;
> +       struct pci_bus *bus;
> +       int ret = 0;
> +       LIST_HEAD(res);
> +
> +       pcie = devm_kzalloc(&pdev->dev, sizeof(*pcie), GFP_KERNEL);
> +       if (!pcie)
> +               return -ENOMEM;
> +
> +       pcie->node = of_node_get(pdev->dev.of_node);
> +       pcie->dev = &pdev->dev;
> +
> +       /* Get controller's configuration space range */
> +       cfg_base = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +
> +       thunder_pcie_config(pcie, cfg_base->start);
> +
> +       pcie->cfg_base = devm_ioremap_resource(&pdev->dev, cfg_base);
> +       if (IS_ERR(pcie->cfg_base)) {
> +               ret = PTR_ERR(pcie->cfg_base);
> +               goto err_ioremap;
> +       }
> +
> +       dev_info(&pdev->dev, "ECAM%d CFG BASE 0x%llx\n",
> +                pcie->ecam, (u64)cfg_base->start);
> +
> +       ret = of_pci_get_host_bridge_resources(pdev->dev.of_node,
> +                                       0, 255, &res, NULL);
> +       if (ret)
> +               goto err_root_bus;
> +
> +       bus = pci_create_root_bus(&pdev->dev, 0, &thunder_pcie_ops, pcie, &res);
> +       if (!bus) {
> +               ret = -ENODEV;
> +               goto err_root_bus;
> +       }
> +
> +       /* Set reference to MSI chip */
> +       ret = thunder_pcie_msi_enable(pcie, bus);
> +       if (ret) {
> +               dev_err(&pdev->dev,
> +                       "Unable to set reference to MSI chip: ret=%d\n", ret);
> +               goto err_msi;
> +       }
> +
> +       platform_set_drvdata(pdev, pcie);
> +
> +       pci_scan_child_bus(bus);
> +       pci_bus_add_devices(bus);
> +
> +       return 0;
> +err_msi:
> +       pci_remove_root_bus(bus);
> +err_root_bus:
> +       pci_free_resource_list(&res);
> +err_ioremap:
> +       of_node_put(pcie->node);
> +       return ret;
> +}
> +
> +static const struct of_device_id thunder_pcie_of_match[] = {
> +       { .compatible = "cavium,thunder-pcie", },
> +       {},
> +};
> +MODULE_DEVICE_TABLE(of, thunder_pcie_of_match);
> +
> +static struct platform_driver thunder_pcie_driver = {
> +       .driver = {
> +               .name = "thunder-pcie",
> +               .owner = THIS_MODULE,
> +               .of_match_table = thunder_pcie_of_match,
> +       },
> +       .probe = thunder_pcie_probe,
> +};
> +module_platform_driver(thunder_pcie_driver);
> +
> +#ifdef CONFIG_ACPI
> +
> +static int
> +thunder_mmcfg_read_config(struct pci_mmcfg_region *cfg, unsigned int busnr,
> +                       unsigned int devfn, int reg, int len, u32 *value)
> +{
> +       struct thunder_pcie *pcie = cfg->data;
> +       void __iomem *addr;
> +
> +       if (!pcie->valid) {
> +               /* Not support for now */
> +               pr_err("RC PEM not supported !!!\n");
> +               return PCIBIOS_DEVICE_NOT_FOUND;
> +       }
> +
> +       addr = thunder_pcie_get_cfg_addr(pcie, busnr, devfn, reg);
> +
> +       switch (len) {
> +       case 1:
> +               *value = readb(addr);
> +               break;
> +       case 2:
> +               *value = readw(addr);
> +               break;
> +       case 4:
> +               *value = readl(addr);
> +               break;
> +       default:
> +               return PCIBIOS_BAD_REGISTER_NUMBER;
> +       }
> +
> +       return PCIBIOS_SUCCESSFUL;
> +}
> +
> +static int thunder_mmcfg_write_config(struct pci_mmcfg_region *cfg,
> +               unsigned int busnr, unsigned int devfn, int reg, int len,
> +               u32 value) {
> +       struct thunder_pcie *pcie = cfg->data;
> +       void __iomem *addr;
> +
> +       if (!pcie->valid) {
> +               /* Not support for now */
> +               pr_err("RC PEM not supported !!!\n");
> +               return PCIBIOS_DEVICE_NOT_FOUND;
> +       }
> +
> +       addr = thunder_pcie_get_cfg_addr(pcie, busnr, devfn, reg);
> +
> +       switch (len) {
> +       case 1:
> +               writeb(value, addr);
> +               break;
> +       case 2:
> +               writew(value, addr);
> +               break;
> +       case 4:
> +               writel(value, addr);
> +               break;
> +       default:
> +               return PCIBIOS_BAD_REGISTER_NUMBER;
> +       }
> +
> +       return PCIBIOS_SUCCESSFUL;
> +}

Again these duplicate config accessors here.

> +static int thunder_acpi_mcfg_fixup(struct acpi_pci_root *root,
> +                                  struct pci_mmcfg_region *cfg)
> +{
> +       struct thunder_pcie *pcie;
> +
> +       pcie = kzalloc(sizeof(*pcie), GFP_KERNEL);
> +       if (!pcie)
> +               return -ENOMEM;
> +
> +       pcie->dev = &root->device->dev;
> +
> +       thunder_pcie_config(pcie, cfg->address);
> +
> +       pcie->cfg_base = cfg->virt;
> +       cfg->data = pcie;
> +       cfg->read = thunder_mmcfg_read_config;
> +       cfg->write = thunder_mmcfg_write_config;
> +
> +       return 0;
> +}
> +DECLARE_ACPI_MCFG_FIXUP("CAVIUM", "THUNDERX", thunder_acpi_mcfg_fixup);
> +#endif

I must have missed MCFG support for ARM64. if this patch depends on
something else you have to tell us, otherwise reviewing it is close
to useless.

Thanks,
Lorenzo

[PATCH 5/5] PCI: Add host drivers for Cavium ThunderX processors.

[Arnd Bergmann]

On Wednesday 15 July 2015 09:54:45 David Daney wrote:
> From: David Daney <david.daney@cavium.com>
> 
> Signed-off-by: David Daney <david.daney@cavium.com>

Please describe here in what way is this incompatible with SBSA.
>From earlier discussions I had expected ThunderX to be SBSA compliant,
so I'm a bit surprised to see a PCI hack now.

> +#define THUNDER_SLI_S2M_REG_ACC_BASE	0x874001000000ull
> +
> +#define THUNDER_GIC			0x801000000000ull
> +#define THUNDER_GICD_SETSPI_NSR		0x801000000040ull
> +#define THUNDER_GICD_CLRSPI_NSR		0x801000000048ull
> +

All I/O addresses must come from DT.

> +#define THUNDER_GSER_PCIE_MASK		0x01
> +
> +#define PEM_CTL_STATUS	0x000
> +#define PEM_RD_CFG	0x030
> +#define P2N_BAR0_START	0x080
> +#define P2N_BAR1_START	0x088
> +#define P2N_BAR2_START	0x090
> +#define BAR_CTL		0x0a8
> +#define BAR2_MASK	0x0b0
> +#define BAR1_INDEX	0x100
> +#define PEM_CFG		0x410
> +#define PEM_ON		0x420
> +
> +struct thunder_pem {
> +	struct list_head list; /* on thunder_pem_buses */
> +	bool		connected;
> +	unsigned int	id;
> +	unsigned int	sli;
> +	unsigned int	sli_group;
> +	unsigned int	node;
> +	u64		sli_window_base;
> +	void __iomem	*bar0;
> +	void __iomem	*bar4;
> +	void __iomem	*sli_s2m;
> +	void __iomem	*cfgregion;
> +	struct pci_bus	*bus;
> +	int		vwire_irqs[4];
> +	u32		vwire_data[4];
> +};
> +
> +static LIST_HEAD(thunder_pem_buses);

Please kill off global lists like this and use the driver
model abstractions we have.

> +static u64 slix_s2m_reg_val(unsigned mac, enum slix_s2m_ctype ctype,
> +			    bool merge, bool relaxed, bool snoop, u32 ba_msb)
> +{
> +	u64 v;
> +
> +	v = (u64)(mac % 3) << 49;
> +	v |= (u64)ctype << 53;
> +	if (!merge)
> +		v |= 1ull << 48;
> +	if (relaxed)
> +		v |= 5ull << 40;
> +	if (!snoop)
> +		v |= 5ull << 41;
> +	v |= (u64)ba_msb;
> +
> +	return v;
> +}

Please add a comment to explain why these registers have to be
configured at runtime. Are you working around broken bootloaders,
or does the configuration depend on the connected PCI devices?

> +static int thunder_pem_read_config(struct pci_bus *bus, unsigned int devfn,
> +				   int reg, int size, u32 *val)
> +{
> +	void __iomem *addr;
> +	struct thunder_pem *pem = bus->sysdata;
> +	unsigned int busnr = bus->number;
> +
> +	if (busnr > 255 || devfn > 255 || reg > 4095)
> +		return PCIBIOS_DEVICE_NOT_FOUND;
> +
> +	addr = pem->cfgregion + ((busnr << 24)  | (devfn << 16) | reg);

This looks like ECAM. Could you use the standard accessors?

> +
> +static int thunder_pem_pci_probe(struct pci_dev *pdev,
> +				 const struct pci_device_id *ent)
> +{
> +	struct thunder_pem *pem;
> +	resource_size_t bar0_start;
> +	u64 regval;
> +	u64 sliaddr, pciaddr;
> +	u32 cfgval;
> +	int primary_bus;
> +	int i;
> +	int ret = 0;
> +	struct resource *res;
> +	LIST_HEAD(resources);
> +
> +	set_pcibios_add_device(thunder_pem_pcibios_add_device);

This looks awful. I don't know who introduced the set_pcibios_add_device
interface, but we should not have something like that. In particular,
it seem wrong for a PCI device driver to add that.

What exactly is the pem? Is that an incompatible replacement of the
normal PCIe port devices?

> +
> +	bar0_start = pci_resource_start(pdev, 0);
> +	pem->node = (bar0_start >> 44) & 3;
> +	pem->id = ((bar0_start >> 24) & 7) + (6 * pem->node);
> +	pem->sli = pem->id % 3;
> +	pem->sli_group = (pem->id / 3) % 2;
> +	pem->sli_window_base = 0x880000000000ull | (((u64)pem->node) << 44) | ((u64)pem->sli_group << 40);
> +	pem->sli_window_base += 0x4000000000 * pem->sli;

What is this computation?

> +	udelay(1000);

This is awfully long for a delay. Please try to avoid it or at least turn it
into a sleeping wait.

> +	res->start = primary_bus;
> +	res->end = 255;
> +	res->flags = IORESOURCE_BUS;
> +	pci_add_resource(&resources, res);

What is this about? You have a PCI device that itself has 255 buses underneath it?

> +static int __init thunder_pcie_init(void)
> +{
> +	int ret;
> +
> +	ret = pci_register_driver(&thunder_pem_driver);
> +
> +	return ret;
> +}
> +module_init(thunder_pcie_init);
> +
> +static void __exit thunder_pcie_exit(void)
> +{
> +	pci_unregister_driver(&thunder_pem_driver);
> +}

module_pci_driver() ?

> +
> +#define PCI_DEVICE_ID_THUNDER_BRIDGE	0xa002
> +
> +#define THUNDER_PCIE_BUS_SHIFT		20
> +#define THUNDER_PCIE_DEV_SHIFT		15
> +#define THUNDER_PCIE_FUNC_SHIFT		12

ECAM?

> +#define THUNDER_ECAM0_CFG_BASE		0x848000000000
> +#define THUNDER_ECAM1_CFG_BASE		0x849000000000
> +#define THUNDER_ECAM2_CFG_BASE		0x84a000000000
> +#define THUNDER_ECAM3_CFG_BASE		0x84b000000000
> +#define THUNDER_ECAM4_CFG_BASE		0x948000000000
> +#define THUNDER_ECAM5_CFG_BASE		0x949000000000
> +#define THUNDER_ECAM6_CFG_BASE		0x94a000000000
> +#define THUNDER_ECAM7_CFG_BASE		0x94b000000000

-> DT

> +/*
> + * All PCIe devices in Thunder have fixed resources, shouldn't be reassigned.
> + * Also claim the device's valid resources to set 'res->parent' hierarchy.
> + */
> +static void pci_dev_resource_fixup(struct pci_dev *dev)
> +{
> +	struct resource *res;
> +	int resno;
> +
> +	/*
> +	 * If the ECAM is not yet probed, we must be in a virtual
> +	 * machine.  In that case, don't mark things as
> +	 * IORESOURCE_PCI_FIXED
> +	 */
> +	if (!atomic_read(&thunder_pcie_ecam_probed))
> +		return;
> +
> +	for (resno = 0; resno < PCI_NUM_RESOURCES; resno++)
> +		dev->resource[resno].flags |= IORESOURCE_PCI_FIXED;

Just list the resources as fixed in DT and kill this function.

If that doesn't work, it's a bug in the PCI core code.

> +
> +static int thunder_pcie_read_config(struct pci_bus *bus, unsigned int devfn,
> +				int reg, int size, u32 *val)
> +{
> +	struct thunder_pcie *pcie = bus->sysdata;
> +	void __iomem *addr;
> +	unsigned int busnr = bus->number;
> +
> +	if (busnr > 255 || devfn > 255 || reg > 4095)
> +		return PCIBIOS_DEVICE_NOT_FOUND;
> +
> +	addr = thunder_pcie_get_cfg_addr(pcie, busnr, devfn, reg);

ECAM again?

> +static int thunder_pcie_msi_enable(struct thunder_pcie *pcie,
> +					struct pci_bus *bus)
> +{
> +	struct device_node *msi_node;
> +
> +	msi_node = of_parse_phandle(pcie->node, "msi-parent", 0);
> +	if (!msi_node)
> +		return -ENODEV;
> +
> +	pcie->msi = of_pci_find_msi_chip_by_node(msi_node);
> +	if (!pcie->msi)
> +		return -ENODEV;
> +
> +	pcie->msi->dev = pcie->dev;
> +	bus->msi = pcie->msi;
> +
> +	return 0;
> +}

This should really be done in the core code, so we don't have to duplicate
it to each driver.

> +
> +static void thunder_pcie_config(struct thunder_pcie *pcie, u64 addr)
> +{
> +	atomic_set(&thunder_pcie_ecam_probed, 1);
> +	set_its_pci_requester_id(thunder_pci_requester_id);
> +
> +	pcie->valid = true;
> +
> +	switch (addr) {
> +	case THUNDER_ECAM0_CFG_BASE:
> +		pcie->ecam = 0;
> +		break;
> +	case THUNDER_ECAM1_CFG_BASE:
> +		pcie->ecam = 1;
> +		break;
> +	case THUNDER_ECAM2_CFG_BASE:
> +		pcie->ecam = 2;
> +		break;
> +	case THUNDER_ECAM3_CFG_BASE:
> +		pcie->ecam = 3;
> +		break;
> +	case THUNDER_ECAM4_CFG_BASE:
> +		pcie->ecam = 4;
> +		break;
> +	case THUNDER_ECAM5_CFG_BASE:
> +		pcie->ecam = 5;
> +		break;
> +	case THUNDER_ECAM6_CFG_BASE:
> +		pcie->ecam = 6;
> +		break;
> +	case THUNDER_ECAM7_CFG_BASE:
> +		pcie->ecam = 7;
> +		break;
> +	default:
> +		pcie->valid = false;
> +		break;
> +	}
> +}

You don't even seem to use the "ecam" numbers, and the "valid" part
looks pointless: if the device is listed by firmware, you should
assume that it is valid.

> +#ifdef CONFIG_ACPI
> +
> +static int
> +thunder_mmcfg_read_config(struct pci_mmcfg_region *cfg, unsigned int busnr,
> +			unsigned int devfn, int reg, int len, u32 *value)
> +{

Just drop the ACPI part: if the device is not compatible with the
normal ACPI probing method and the PCI definition and with SBSA,
then we can't really support it on ACPI based systems.

	Arnd

[PATCH 0/5] arm64, pci: Add ECAM/PCIe support for Cavium ThunderX

[Mark Rutland]

Hi,

On Wed, Jul 15, 2015 at 05:54:40PM +0100, David Daney wrote:
> From: David Daney <david.daney@cavium.com>
> 
> The subject pretty much says it all.  The first four patches tweak the
> infrastructure a little so that we can get required behavior.  The
> final patch adds the drivers.
> 
> David Daney (5):
>   pci: Add is_pcierc element to struct pci_bus
>   gic-its: Allow pci_requester_id to be overridden.
>   arm64, pci: Allow RC drivers to supply pcibios_add_device()
>     implementation.
>   irqchip: gic-v3: Add gic_get_irq_domain() to get the irqdomain of the
>     GIC.
>   PCI: Add host drivers for Cavium ThunderX processors.
> 
>  arch/arm64/include/asm/pci.h        |   3 +
>  arch/arm64/kernel/pci.c             |  10 +
>  drivers/irqchip/irq-gic-v3-its.c    |  14 +-
>  drivers/irqchip/irq-gic-v3.c        |   5 +
>  drivers/pci/host/Kconfig            |  12 +
>  drivers/pci/host/Makefile           |   2 +
>  drivers/pci/host/pcie-thunder-pem.c | 462 ++++++++++++++++++++++++++++++++++++
>  drivers/pci/host/pcie-thunder.c     | 422 ++++++++++++++++++++++++++++++++
>  drivers/pci/probe.c                 |   2 +
>  include/linux/irqchip/arm-gic-v3.h  |   3 +
>  include/linux/pci.h                 |   1 +
>  11 files changed, 935 insertions(+), 1 deletion(-)

I note that the driver attempts to probe with DT, yet there is no DT
binding in this patch, nor has the devicetree mailing list been placed
on Cc.

A DT binding document is mandatory for anything probing via DT.

Please write one, and submit it (as its own patch) with the next version
of this series as per
Documentation/devicetree/bindings/submitting-patches.txt.

Thanks,
Mark.

[PATCH 0/5] arm64, pci: Add ECAM/PCIe support for Cavium ThunderX

[Will Deacon]

On Wed, Jul 15, 2015 at 06:07:01PM +0100, Mark Rutland wrote:
> On Wed, Jul 15, 2015 at 05:54:40PM +0100, David Daney wrote:
> > From: David Daney <david.daney@cavium.com>
> > 
> > The subject pretty much says it all.  The first four patches tweak the
> > infrastructure a little so that we can get required behavior.  The
> > final patch adds the drivers.
> > 
> > David Daney (5):
> >   pci: Add is_pcierc element to struct pci_bus
> >   gic-its: Allow pci_requester_id to be overridden.
> >   arm64, pci: Allow RC drivers to supply pcibios_add_device()
> >     implementation.
> >   irqchip: gic-v3: Add gic_get_irq_domain() to get the irqdomain of the
> >     GIC.
> >   PCI: Add host drivers for Cavium ThunderX processors.
> > 
> >  arch/arm64/include/asm/pci.h        |   3 +
> >  arch/arm64/kernel/pci.c             |  10 +
> >  drivers/irqchip/irq-gic-v3-its.c    |  14 +-
> >  drivers/irqchip/irq-gic-v3.c        |   5 +
> >  drivers/pci/host/Kconfig            |  12 +
> >  drivers/pci/host/Makefile           |   2 +
> >  drivers/pci/host/pcie-thunder-pem.c | 462 ++++++++++++++++++++++++++++++++++++
> >  drivers/pci/host/pcie-thunder.c     | 422 ++++++++++++++++++++++++++++++++
> >  drivers/pci/probe.c                 |   2 +
> >  include/linux/irqchip/arm-gic-v3.h  |   3 +
> >  include/linux/pci.h                 |   1 +
> >  11 files changed, 935 insertions(+), 1 deletion(-)
> 
> I note that the driver attempts to probe with DT, yet there is no DT
> binding in this patch, nor has the devicetree mailing list been placed
> on Cc.
> 
> A DT binding document is mandatory for anything probing via DT.
> 
> Please write one, and submit it (as its own patch) with the next version
> of this series as per
> Documentation/devicetree/bindings/submitting-patches.txt.

Furthermore, the RequesterID -> DeviceID transformation should be described
in device-tree as an offset on each PCI host controller node, rather than
computed dynamically by picking apart the (software-assigned) PCI domain
ID.

For ACPI, IORT should be used.

Will

[PATCH 0/5] arm64, pci: Add ECAM/PCIe support for Cavium ThunderX

[Thomas Gleixner]

On Wed, 15 Jul 2015, David Daney wrote:

> From: David Daney <david.daney@cavium.com>
> 
> The subject pretty much says it all.  The first four patches tweak the
> infrastructure a little so that we can get required behavior.  The

Just that you avoid to describe what the required behaviour is.

Can you please provide a proper description how this looks like from
the HW side and how you want to map that into software?

Thanks,

	tglx