diff mbox

[RFC,2/2] arm: pcibios: move to generic PCI domains

Message ID 1414669490-1217-3-git-send-email-lorenzo.pieralisi@arm.com
State New
Headers show

Commit Message

Lorenzo Pieralisi Oct. 30, 2014, 11:44 a.m. UTC
Most if not all ARM PCI host controller device drivers either ignore the
domain field in the pci_sys_data structure or just increment it every
time a host controller is probed, using it as a domain counter.

Therefore, instead of relying on pci_sys_data to stash the domain number
in a standard location, ARM pcibios code can be moved to the newly
introduced generic PCI domains code, implemented in commits:

commit 41e5c0f81d3e676d671d96a0a1fafb27abfbd9
("of/pci: Add pci_get_new_domain_nr() and of_get_pci_domain_nr()")

commit 670ba0c8883b576d0aec28bd7a838358a4be1
("PCI: Add generic domain handling")

In order to assign a domain number dynamically, the ARM pcibios defines
the function, called by core PCI code:

void pci_bus_assign_domain_nr(...)

that relies on a DT property to define the domain number or falls back to
a counter; its usage replaces the current domain assignment code in PCI
host controllers present in the kernel.

Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Phil Edworthy <phil.edworthy@renesas.com>
Cc: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
Cc: Jingoo Han <jg1.han@samsung.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Mohit Kumar <mohit.kumar@st.com>
Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
---
 arch/arm/Kconfig                   |  3 +++
 arch/arm/include/asm/mach/pci.h    |  3 ---
 arch/arm/include/asm/pci.h         |  7 -------
 arch/arm/kernel/bios32.c           | 26 +++++++++++++++++++++++---
 drivers/pci/host/pci-mvebu.c       | 10 ++--------
 drivers/pci/host/pcie-designware.c |  3 ---
 drivers/pci/host/pcie-rcar.c       |  3 ---
 7 files changed, 28 insertions(+), 27 deletions(-)

Comments

Arnd Bergmann Oct. 30, 2014, 11:55 a.m. UTC | #1
On Thursday 30 October 2014 11:44:48 Lorenzo Pieralisi wrote:
> Most if not all ARM PCI host controller device drivers either ignore the
> domain field in the pci_sys_data structure or just increment it every
> time a host controller is probed, using it as a domain counter.
> 
> Therefore, instead of relying on pci_sys_data to stash the domain number
> in a standard location, ARM pcibios code can be moved to the newly
> introduced generic PCI domains code, implemented in commits:
> 
> commit 41e5c0f81d3e676d671d96a0a1fafb27abfbd9
> ("of/pci: Add pci_get_new_domain_nr() and of_get_pci_domain_nr()")
> 
> commit 670ba0c8883b576d0aec28bd7a838358a4be1
> ("PCI: Add generic domain handling")
> 
> In order to assign a domain number dynamically, the ARM pcibios defines
> the function, called by core PCI code:
> 
> void pci_bus_assign_domain_nr(...)
> 
> that relies on a DT property to define the domain number or falls back to
> a counter; its usage replaces the current domain assignment code in PCI
> host controllers present in the kernel.
> 
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Phil Edworthy <phil.edworthy@renesas.com>
> Cc: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> Cc: Jingoo Han <jg1.han@samsung.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Russell King <linux@arm.linux.org.uk>
> Cc: Mohit Kumar <mohit.kumar@st.com>
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> 

Acked-by: Arnd Bergmann <arnd@arndb.de>

Very nice!
wangyijing Oct. 30, 2014, 12:27 p.m. UTC | #2
On 2014/10/30 19:44, Lorenzo Pieralisi wrote:
> Most if not all ARM PCI host controller device drivers either ignore the
> domain field in the pci_sys_data structure or just increment it every
> time a host controller is probed, using it as a domain counter.
> 
> Therefore, instead of relying on pci_sys_data to stash the domain number
> in a standard location, ARM pcibios code can be moved to the newly
> introduced generic PCI domains code, implemented in commits:
> 
> commit 41e5c0f81d3e676d671d96a0a1fafb27abfbd9
> ("of/pci: Add pci_get_new_domain_nr() and of_get_pci_domain_nr()")
> 
> commit 670ba0c8883b576d0aec28bd7a838358a4be1
> ("PCI: Add generic domain handling")
> 
> In order to assign a domain number dynamically, the ARM pcibios defines
> the function, called by core PCI code:
> 
> void pci_bus_assign_domain_nr(...)
> 
> that relies on a DT property to define the domain number or falls back to
> a counter; its usage replaces the current domain assignment code in PCI
> host controllers present in the kernel.

Nice clean up.

Reviewed-by: Yijing Wang <wangyijing@huawei.com>

> 
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Phil Edworthy <phil.edworthy@renesas.com>
> Cc: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> Cc: Jingoo Han <jg1.han@samsung.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Russell King <linux@arm.linux.org.uk>
> Cc: Mohit Kumar <mohit.kumar@st.com>
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> ---
>  arch/arm/Kconfig                   |  3 +++
>  arch/arm/include/asm/mach/pci.h    |  3 ---
>  arch/arm/include/asm/pci.h         |  7 -------
>  arch/arm/kernel/bios32.c           | 26 +++++++++++++++++++++++---
>  drivers/pci/host/pci-mvebu.c       | 10 ++--------
>  drivers/pci/host/pcie-designware.c |  3 ---
>  drivers/pci/host/pcie-rcar.c       |  3 ---
>  7 files changed, 28 insertions(+), 27 deletions(-)
> 
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 89c4b5c..29544f0 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -1292,6 +1292,9 @@ config PCI_DOMAINS
>  	bool
>  	depends on PCI
>  
> +config PCI_DOMAINS_GENERIC
> +	def_bool PCI_DOMAINS
> +
>  config PCI_NANOENGINE
>  	bool "BSE nanoEngine PCI support"
>  	depends on SA1100_NANOENGINE
> diff --git a/arch/arm/include/asm/mach/pci.h b/arch/arm/include/asm/mach/pci.h
> index 7fc4278..f19f627 100644
> --- a/arch/arm/include/asm/mach/pci.h
> +++ b/arch/arm/include/asm/mach/pci.h
> @@ -44,9 +44,6 @@ struct hw_pci {
>   * Per-controller structure
>   */
>  struct pci_sys_data {
> -#ifdef CONFIG_PCI_DOMAINS
> -	int		domain;
> -#endif
>  	struct list_head node;
>  	int		busnr;		/* primary bus number			*/
>  	u64		mem_offset;	/* bus->cpu memory mapping offset	*/
> diff --git a/arch/arm/include/asm/pci.h b/arch/arm/include/asm/pci.h
> index 7e95d85..585dc33 100644
> --- a/arch/arm/include/asm/pci.h
> +++ b/arch/arm/include/asm/pci.h
> @@ -18,13 +18,6 @@ static inline int pcibios_assign_all_busses(void)
>  }
>  
>  #ifdef CONFIG_PCI_DOMAINS
> -static inline int pci_domain_nr(struct pci_bus *bus)
> -{
> -	struct pci_sys_data *root = bus->sysdata;
> -
> -	return root->domain;
> -}
> -
>  static inline int pci_proc_domain(struct pci_bus *bus)
>  {
>  	return pci_domain_nr(bus);
> diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
> index 17a26c1..d8c2b4e 100644
> --- a/arch/arm/kernel/bios32.c
> +++ b/arch/arm/kernel/bios32.c
> @@ -11,6 +11,8 @@
>  #include <linux/slab.h>
>  #include <linux/init.h>
>  #include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/of_pci.h>
>  
>  #include <asm/mach-types.h>
>  #include <asm/mach/map.h>
> @@ -468,9 +470,6 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw,
>  		if (!sys)
>  			panic("PCI: unable to allocate sys data!");
>  
> -#ifdef CONFIG_PCI_DOMAINS
> -		sys->domain  = hw->domain;
> -#endif
>  		sys->busnr   = busnr;
>  		sys->swizzle = hw->swizzle;
>  		sys->map_irq = hw->map_irq;
> @@ -511,6 +510,27 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw,
>  	}
>  }
>  
> +#ifdef CONFIG_PCI_DOMAINS_GENERIC
> +static bool dt_domain_found;
> +
> +void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
> +{
> +	int domain = of_get_pci_domain_nr(parent->of_node);
> +
> +	if (domain >= 0) {
> +		dt_domain_found = true;
> +	} else if (dt_domain_found == true) {
> +		dev_err(parent, "Node %s is missing \"linux,pci-domain\" property in DT\n",
> +			parent->of_node->full_name);
> +		return;
> +	} else {
> +		domain = pci_get_new_domain_nr();
> +	}
> +
> +	bus->domain_nr = domain;
> +}
> +#endif
> +
>  void pci_common_init_dev(struct device *parent, struct hw_pci *hw)
>  {
>  	struct pci_sys_data *sys;
> diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
> index b1315e1..2b4c1f2 100644
> --- a/drivers/pci/host/pci-mvebu.c
> +++ b/drivers/pci/host/pci-mvebu.c
> @@ -722,17 +722,11 @@ static int mvebu_pcie_setup(int nr, struct pci_sys_data *sys)
>  {
>  	struct mvebu_pcie *pcie = sys_to_pcie(sys);
>  	int i;
> -	int domain = 0;
>  
> -#ifdef CONFIG_PCI_DOMAINS
> -	domain = sys->domain;
> -#endif
> -
> -	snprintf(pcie->mem_name, sizeof(pcie->mem_name), "PCI MEM %04x",
> -		 domain);
> +	snprintf(pcie->mem_name, sizeof(pcie->mem_name), "PCI MEM %04x", nr);
>  	pcie->mem.name = pcie->mem_name;
>  
> -	snprintf(pcie->io_name, sizeof(pcie->io_name), "PCI I/O %04x", domain);
> +	snprintf(pcie->io_name, sizeof(pcie->io_name), "PCI I/O %04x", nr);
>  	pcie->realio.name = pcie->io_name;
>  
>  	if (request_resource(&iomem_resource, &pcie->mem))
> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
> index dfed00a..6790b87 100644
> --- a/drivers/pci/host/pcie-designware.c
> +++ b/drivers/pci/host/pcie-designware.c
> @@ -502,9 +502,6 @@ int __init dw_pcie_host_init(struct pcie_port *pp)
>  	dw_pci.private_data = (void **)&pp;
>  
>  	pci_common_init_dev(pp->dev, &dw_pci);
> -#ifdef CONFIG_PCI_DOMAINS
> -	dw_pci.domain++;
> -#endif
>  
>  	return 0;
>  }
> diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c
> index 61158e0..b6b859e 100644
> --- a/drivers/pci/host/pcie-rcar.c
> +++ b/drivers/pci/host/pcie-rcar.c
> @@ -404,9 +404,6 @@ static void rcar_pcie_enable(struct rcar_pcie *pcie)
>  	rcar_pci.private_data = (void **)&pcie;
>  
>  	pci_common_init_dev(&pdev->dev, &rcar_pci);
> -#ifdef CONFIG_PCI_DOMAINS
> -	rcar_pci.domain++;
> -#endif
>  }
>  
>  static int phy_wait_for_ack(struct rcar_pcie *pcie)
>
Lorenzo Pieralisi Oct. 30, 2014, 4:20 p.m. UTC | #3
On Thu, Oct 30, 2014 at 11:55:31AM +0000, Arnd Bergmann wrote:
> On Thursday 30 October 2014 11:44:48 Lorenzo Pieralisi wrote:
> > Most if not all ARM PCI host controller device drivers either ignore the
> > domain field in the pci_sys_data structure or just increment it every
> > time a host controller is probed, using it as a domain counter.
> > 
> > Therefore, instead of relying on pci_sys_data to stash the domain number
> > in a standard location, ARM pcibios code can be moved to the newly
> > introduced generic PCI domains code, implemented in commits:
> > 
> > commit 41e5c0f81d3e676d671d96a0a1fafb27abfbd9
> > ("of/pci: Add pci_get_new_domain_nr() and of_get_pci_domain_nr()")
> > 
> > commit 670ba0c8883b576d0aec28bd7a838358a4be1
> > ("PCI: Add generic domain handling")
> > 
> > In order to assign a domain number dynamically, the ARM pcibios defines
> > the function, called by core PCI code:
> > 
> > void pci_bus_assign_domain_nr(...)
> > 
> > that relies on a DT property to define the domain number or falls back to
> > a counter; its usage replaces the current domain assignment code in PCI
> > host controllers present in the kernel.
> > 
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: Phil Edworthy <phil.edworthy@renesas.com>
> > Cc: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> > Cc: Jingoo Han <jg1.han@samsung.com>
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: Russell King <linux@arm.linux.org.uk>
> > Cc: Mohit Kumar <mohit.kumar@st.com>
> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > 
> 
> Acked-by: Arnd Bergmann <arnd@arndb.de>
> 
> Very nice!

Thank you very much, I would be grateful if Jason, Jingoo, and Phil can
give the patch a go on the respective platforms and hopefully I will add
their tested-by tags.

I guess pci_sys_data.add_bus pointer is the next to go :)

Lorenzo
Lorenzo Pieralisi Oct. 30, 2014, 4:21 p.m. UTC | #4
On Thu, Oct 30, 2014 at 12:27:13PM +0000, Yijing Wang wrote:
> On 2014/10/30 19:44, Lorenzo Pieralisi wrote:
> > Most if not all ARM PCI host controller device drivers either ignore the
> > domain field in the pci_sys_data structure or just increment it every
> > time a host controller is probed, using it as a domain counter.
> > 
> > Therefore, instead of relying on pci_sys_data to stash the domain number
> > in a standard location, ARM pcibios code can be moved to the newly
> > introduced generic PCI domains code, implemented in commits:
> > 
> > commit 41e5c0f81d3e676d671d96a0a1fafb27abfbd9
> > ("of/pci: Add pci_get_new_domain_nr() and of_get_pci_domain_nr()")
> > 
> > commit 670ba0c8883b576d0aec28bd7a838358a4be1
> > ("PCI: Add generic domain handling")
> > 
> > In order to assign a domain number dynamically, the ARM pcibios defines
> > the function, called by core PCI code:
> > 
> > void pci_bus_assign_domain_nr(...)
> > 
> > that relies on a DT property to define the domain number or falls back to
> > a counter; its usage replaces the current domain assignment code in PCI
> > host controllers present in the kernel.
> 
> Nice clean up.
> 
> Reviewed-by: Yijing Wang <wangyijing@huawei.com>

Thank you Yijing !
Lorenzo
Phil Edworthy Oct. 31, 2014, 1:43 p.m. UTC | #5
Hi Lorenzo,

On 30 October 2014 11:45, Lorenzo wrote:
> Most if not all ARM PCI host controller device drivers either ignore the
> domain field in the pci_sys_data structure or just increment it every
> time a host controller is probed, using it as a domain counter.
> 
> Therefore, instead of relying on pci_sys_data to stash the domain number
> in a standard location, ARM pcibios code can be moved to the newly
> introduced generic PCI domains code, implemented in commits:
> 
> commit 41e5c0f81d3e676d671d96a0a1fafb27abfbd9
> ("of/pci: Add pci_get_new_domain_nr() and of_get_pci_domain_nr()")
> 
> commit 670ba0c8883b576d0aec28bd7a838358a4be1
> ("PCI: Add generic domain handling")
> 
> In order to assign a domain number dynamically, the ARM pcibios defines
> the function, called by core PCI code:
> 
> void pci_bus_assign_domain_nr(...)
> 
> that relies on a DT property to define the domain number or falls back to
> a counter; its usage replaces the current domain assignment code in PCI
> host controllers present in the kernel.
> 
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Phil Edworthy <phil.edworthy@renesas.com>
> Cc: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> Cc: Jingoo Han <jg1.han@samsung.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Russell King <linux@arm.linux.org.uk>
> Cc: Mohit Kumar <mohit.kumar@st.com>
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>

This patch fixes a current problem with R-Car devices where there is an
internal PCI bridge and an external PCIe bridge on the devices. Both drivers
work independently but need to be on different domains. Just needed to enable
PCI_DOMAINS along with this.
I've done basic testing that the internal PCI and external PCIe work at the
same time.

Acked-by: Phil Edworthy <phil.edworthy@renesas.com>

Thanks
Phil
> ---
>  arch/arm/Kconfig                   |  3 +++
>  arch/arm/include/asm/mach/pci.h    |  3 ---
>  arch/arm/include/asm/pci.h         |  7 -------
>  arch/arm/kernel/bios32.c           | 26 +++++++++++++++++++++++---
>  drivers/pci/host/pci-mvebu.c       | 10 ++--------
>  drivers/pci/host/pcie-designware.c |  3 ---
>  drivers/pci/host/pcie-rcar.c       |  3 ---
>  7 files changed, 28 insertions(+), 27 deletions(-)
> 
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 89c4b5c..29544f0 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -1292,6 +1292,9 @@ config PCI_DOMAINS
>  	bool
>  	depends on PCI
> 
> +config PCI_DOMAINS_GENERIC
> +	def_bool PCI_DOMAINS
> +
>  config PCI_NANOENGINE
>  	bool "BSE nanoEngine PCI support"
>  	depends on SA1100_NANOENGINE
> diff --git a/arch/arm/include/asm/mach/pci.h
> b/arch/arm/include/asm/mach/pci.h
> index 7fc4278..f19f627 100644
> --- a/arch/arm/include/asm/mach/pci.h
> +++ b/arch/arm/include/asm/mach/pci.h
> @@ -44,9 +44,6 @@ struct hw_pci {
>   * Per-controller structure
>   */
>  struct pci_sys_data {
> -#ifdef CONFIG_PCI_DOMAINS
> -	int		domain;
> -#endif
>  	struct list_head node;
>  	int		busnr;		/* primary bus number			*/
>  	u64		mem_offset;	/* bus->cpu memory mapping offset	*/
> diff --git a/arch/arm/include/asm/pci.h b/arch/arm/include/asm/pci.h
> index 7e95d85..585dc33 100644
> --- a/arch/arm/include/asm/pci.h
> +++ b/arch/arm/include/asm/pci.h
> @@ -18,13 +18,6 @@ static inline int pcibios_assign_all_busses(void)
>  }
> 
>  #ifdef CONFIG_PCI_DOMAINS
> -static inline int pci_domain_nr(struct pci_bus *bus)
> -{
> -	struct pci_sys_data *root = bus->sysdata;
> -
> -	return root->domain;
> -}
> -
>  static inline int pci_proc_domain(struct pci_bus *bus)
>  {
>  	return pci_domain_nr(bus);
> diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
> index 17a26c1..d8c2b4e 100644
> --- a/arch/arm/kernel/bios32.c
> +++ b/arch/arm/kernel/bios32.c
> @@ -11,6 +11,8 @@
>  #include <linux/slab.h>
>  #include <linux/init.h>
>  #include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/of_pci.h>
> 
>  #include <asm/mach-types.h>
>  #include <asm/mach/map.h>
> @@ -468,9 +470,6 @@ static void pcibios_init_hw(struct device *parent, struct
> hw_pci *hw,
>  		if (!sys)
>  			panic("PCI: unable to allocate sys data!");
> 
> -#ifdef CONFIG_PCI_DOMAINS
> -		sys->domain  = hw->domain;
> -#endif
>  		sys->busnr   = busnr;
>  		sys->swizzle = hw->swizzle;
>  		sys->map_irq = hw->map_irq;
> @@ -511,6 +510,27 @@ static void pcibios_init_hw(struct device *parent, struct
> hw_pci *hw,
>  	}
>  }
> 
> +#ifdef CONFIG_PCI_DOMAINS_GENERIC
> +static bool dt_domain_found;
> +
> +void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
> +{
> +	int domain = of_get_pci_domain_nr(parent->of_node);
> +
> +	if (domain >= 0) {
> +		dt_domain_found = true;
> +	} else if (dt_domain_found == true) {
> +		dev_err(parent, "Node %s is missing \"linux,pci-domain\"
> property in DT\n",
> +			parent->of_node->full_name);
> +		return;
> +	} else {
> +		domain = pci_get_new_domain_nr();
> +	}
> +
> +	bus->domain_nr = domain;
> +}
> +#endif
> +
>  void pci_common_init_dev(struct device *parent, struct hw_pci *hw)
>  {
>  	struct pci_sys_data *sys;
> diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
> index b1315e1..2b4c1f2 100644
> --- a/drivers/pci/host/pci-mvebu.c
> +++ b/drivers/pci/host/pci-mvebu.c
> @@ -722,17 +722,11 @@ static int mvebu_pcie_setup(int nr, struct pci_sys_data
> *sys)
>  {
>  	struct mvebu_pcie *pcie = sys_to_pcie(sys);
>  	int i;
> -	int domain = 0;
> 
> -#ifdef CONFIG_PCI_DOMAINS
> -	domain = sys->domain;
> -#endif
> -
> -	snprintf(pcie->mem_name, sizeof(pcie->mem_name), "PCI MEM %04x",
> -		 domain);
> +	snprintf(pcie->mem_name, sizeof(pcie->mem_name), "PCI MEM %04x",
> nr);
>  	pcie->mem.name = pcie->mem_name;
> 
> -	snprintf(pcie->io_name, sizeof(pcie->io_name), "PCI I/O %04x",
> domain);
> +	snprintf(pcie->io_name, sizeof(pcie->io_name), "PCI I/O %04x", nr);
>  	pcie->realio.name = pcie->io_name;
> 
>  	if (request_resource(&iomem_resource, &pcie->mem))
> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-
> designware.c
> index dfed00a..6790b87 100644
> --- a/drivers/pci/host/pcie-designware.c
> +++ b/drivers/pci/host/pcie-designware.c
> @@ -502,9 +502,6 @@ int __init dw_pcie_host_init(struct pcie_port *pp)
>  	dw_pci.private_data = (void **)&pp;
> 
>  	pci_common_init_dev(pp->dev, &dw_pci);
> -#ifdef CONFIG_PCI_DOMAINS
> -	dw_pci.domain++;
> -#endif
> 
>  	return 0;
>  }
> diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c
> index 61158e0..b6b859e 100644
> --- a/drivers/pci/host/pcie-rcar.c
> +++ b/drivers/pci/host/pcie-rcar.c
> @@ -404,9 +404,6 @@ static void rcar_pcie_enable(struct rcar_pcie *pcie)
>  	rcar_pci.private_data = (void **)&pcie;
> 
>  	pci_common_init_dev(&pdev->dev, &rcar_pci);
> -#ifdef CONFIG_PCI_DOMAINS
> -	rcar_pci.domain++;
> -#endif
>  }
> 
>  static int phy_wait_for_ack(struct rcar_pcie *pcie)
> --
> 2.1.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas Oct. 31, 2014, 4:37 p.m. UTC | #6
On Fri, Oct 31, 2014 at 7:43 AM, Phil Edworthy
<phil.edworthy@renesas.com> wrote:
> Hi Lorenzo,
>
> On 30 October 2014 11:45, Lorenzo wrote:
>> Most if not all ARM PCI host controller device drivers either ignore the
>> domain field in the pci_sys_data structure or just increment it every
>> time a host controller is probed, using it as a domain counter.
>>
>> Therefore, instead of relying on pci_sys_data to stash the domain number
>> in a standard location, ARM pcibios code can be moved to the newly
>> introduced generic PCI domains code, implemented in commits:
>>
>> commit 41e5c0f81d3e676d671d96a0a1fafb27abfbd9
>> ("of/pci: Add pci_get_new_domain_nr() and of_get_pci_domain_nr()")
>>
>> commit 670ba0c8883b576d0aec28bd7a838358a4be1
>> ("PCI: Add generic domain handling")
>>
>> In order to assign a domain number dynamically, the ARM pcibios defines
>> the function, called by core PCI code:
>>
>> void pci_bus_assign_domain_nr(...)
>>
>> that relies on a DT property to define the domain number or falls back to
>> a counter; its usage replaces the current domain assignment code in PCI
>> host controllers present in the kernel.
>>
>> Cc: Arnd Bergmann <arnd@arndb.de>
>> Cc: Phil Edworthy <phil.edworthy@renesas.com>
>> Cc: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
>> Cc: Jingoo Han <jg1.han@samsung.com>
>> Cc: Bjorn Helgaas <bhelgaas@google.com>
>> Cc: Russell King <linux@arm.linux.org.uk>
>> Cc: Mohit Kumar <mohit.kumar@st.com>
>> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>
> This patch fixes a current problem with R-Car devices where there is an
> internal PCI bridge and an external PCIe bridge on the devices. Both drivers
> work independently but need to be on different domains. Just needed to enable
> PCI_DOMAINS along with this.
> I've done basic testing that the internal PCI and external PCIe work at the
> same time.

Hi Phil,

Thanks for testing this.  Can you give me some more guidance on where
you'd like to see this merged?  Until your comment about this fixing a
current problem on R-Car, I probably would have considered this to be
a cleanup and enhancement and hence material for v3.19.  But if R-Car
is actually broken and this fixes it, maybe this should go in for
v3.18 instead.

If it is currently broken, is there a point where it broke?  I assume
it used to work at one time.  If there's a commit that broke it, it
would be nice to reference that in the changelog and explain exactly
what was broken and how this fixes it.

Bjorn
Phil Edworthy Oct. 31, 2014, 5:04 p.m. UTC | #7
Hi Bjorn,

On 31 October 2014 16:37, Bjorn wrote:
> On Fri, Oct 31, 2014 at 7:43 AM, Phil Edworthy

> <phil.edworthy@renesas.com> wrote:

> > Hi Lorenzo,

> >

> > On 30 October 2014 11:45, Lorenzo wrote:

> >> Most if not all ARM PCI host controller device drivers either ignore the

> >> domain field in the pci_sys_data structure or just increment it every

> >> time a host controller is probed, using it as a domain counter.

> >>

> >> Therefore, instead of relying on pci_sys_data to stash the domain number

> >> in a standard location, ARM pcibios code can be moved to the newly

> >> introduced generic PCI domains code, implemented in commits:

> >>

> >> commit 41e5c0f81d3e676d671d96a0a1fafb27abfbd9

> >> ("of/pci: Add pci_get_new_domain_nr() and of_get_pci_domain_nr()")

> >>

> >> commit 670ba0c8883b576d0aec28bd7a838358a4be1

> >> ("PCI: Add generic domain handling")

> >>

> >> In order to assign a domain number dynamically, the ARM pcibios defines

> >> the function, called by core PCI code:

> >>

> >> void pci_bus_assign_domain_nr(...)

> >>

> >> that relies on a DT property to define the domain number or falls back to

> >> a counter; its usage replaces the current domain assignment code in PCI

> >> host controllers present in the kernel.

> >>

> >> Cc: Arnd Bergmann <arnd@arndb.de>

> >> Cc: Phil Edworthy <phil.edworthy@renesas.com>

> >> Cc: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>

> >> Cc: Jingoo Han <jg1.han@samsung.com>

> >> Cc: Bjorn Helgaas <bhelgaas@google.com>

> >> Cc: Russell King <linux@arm.linux.org.uk>

> >> Cc: Mohit Kumar <mohit.kumar@st.com>

> >> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>

> >

> > This patch fixes a current problem with R-Car devices where there is an

> > internal PCI bridge and an external PCIe bridge on the devices. Both drivers

> > work independently but need to be on different domains. Just needed to

> enable

> > PCI_DOMAINS along with this.

> > I've done basic testing that the internal PCI and external PCIe work at the

> > same time.

> 

> Hi Phil,

> 

> Thanks for testing this.  Can you give me some more guidance on where

> you'd like to see this merged?  Until your comment about this fixing a

> current problem on R-Car, I probably would have considered this to be

> a cleanup and enhancement and hence material for v3.19.  But if R-Car

> is actually broken and this fixes it, maybe this should go in for

> v3.18 instead.

I don’t think its urgent as most of our customers use LTSI kernels, e.g. v3.10.
Renesas typically provide out-of-tree BSPs with upstream code back ported, along
with other patches. Simon Horman (Renesas maintainer, cc'd) generally handles
the back ports, so I'll defer to his opinion.

> If it is currently broken, is there a point where it broke?  I assume

> it used to work at one time.  If there's a commit that broke it, it

> would be nice to reference that in the changelog and explain exactly

> what was broken and how this fixes it.

Both the internal PCI and external PCIe drivers work on their own, but have
never worked at the same time. I think it was just unfortunate timing when I
added the PCIe driver. At that time, the internal PCI driver didn’t have the
relevant DT nodes for the board I was using so I didn't see any conflicts.

Thanks
Phil
Jingoo Han Nov. 3, 2014, 1:18 a.m. UTC | #8
On Thursday, October 30, 2014 8:45 PM, Lorenzo Pieralisi wrote:
> 
> Most if not all ARM PCI host controller device drivers either ignore the
> domain field in the pci_sys_data structure or just increment it every
> time a host controller is probed, using it as a domain counter.
> 
> Therefore, instead of relying on pci_sys_data to stash the domain number
> in a standard location, ARM pcibios code can be moved to the newly
> introduced generic PCI domains code, implemented in commits:
> 
> commit 41e5c0f81d3e676d671d96a0a1fafb27abfbd9
> ("of/pci: Add pci_get_new_domain_nr() and of_get_pci_domain_nr()")
> 
> commit 670ba0c8883b576d0aec28bd7a838358a4be1
> ("PCI: Add generic domain handling")
> 
> In order to assign a domain number dynamically, the ARM pcibios defines
> the function, called by core PCI code:
> 
> void pci_bus_assign_domain_nr(...)
> 
> that relies on a DT property to define the domain number or falls back to
> a counter; its usage replaces the current domain assignment code in PCI
> host controllers present in the kernel.
> 
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Phil Edworthy <phil.edworthy@renesas.com>
> Cc: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> Cc: Jingoo Han <jg1.han@samsung.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Russell King <linux@arm.linux.org.uk>
> Cc: Mohit Kumar <mohit.kumar@st.com>
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>

(+CC Richard Zhu, Lucas Stach, Kishon Vijay Abraham I, Murali Karicheri)

Acked-by: Jingoo Han <jg1.han@samsung.com>

For a long time, I have wanted the patch like this. Personally,
I want to increase the build coverage and keep the same binary
regardless of config options as possible; thus, I don't like
#ifdef guards such as '#ifdef CONFIG_PCI_DOMAINS'. When I submitted
the first patch for 'pcie-designware.c', I was not able to find
a way to remove '#ifdef CONFIG_PCI_DOMAINS' from 'pcie-designware.c'.

This patch looks good and I think that this patch will not make
the problem. However, I didn't test this patch with Exynos platform,
due to some reasons. So, I hope that other engineers test this patch
on other SoC platforms related to designware PCIe.

To Mohit Kumar (ST SPEAR13XX), Richard Zhu/Lucas Stach (IMX6)
    Kishon Vijay Abraham I (TI DRA7XX), Murali Karicheri (TI KEYSTONE)

Would you test this patch and share the result?
It would be very helpful. Thank you.

Best regards,
Jingoo Han

> ---
>  arch/arm/Kconfig                   |  3 +++
>  arch/arm/include/asm/mach/pci.h    |  3 ---
>  arch/arm/include/asm/pci.h         |  7 -------
>  arch/arm/kernel/bios32.c           | 26 +++++++++++++++++++++++---
>  drivers/pci/host/pci-mvebu.c       | 10 ++--------
>  drivers/pci/host/pcie-designware.c |  3 ---
>  drivers/pci/host/pcie-rcar.c       |  3 ---
>  7 files changed, 28 insertions(+), 27 deletions(-)
> 
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 89c4b5c..29544f0 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -1292,6 +1292,9 @@ config PCI_DOMAINS
>  	bool
>  	depends on PCI
> 
> +config PCI_DOMAINS_GENERIC
> +	def_bool PCI_DOMAINS
> +
>  config PCI_NANOENGINE
>  	bool "BSE nanoEngine PCI support"
>  	depends on SA1100_NANOENGINE
> diff --git a/arch/arm/include/asm/mach/pci.h b/arch/arm/include/asm/mach/pci.h
> index 7fc4278..f19f627 100644
> --- a/arch/arm/include/asm/mach/pci.h
> +++ b/arch/arm/include/asm/mach/pci.h
> @@ -44,9 +44,6 @@ struct hw_pci {
>   * Per-controller structure
>   */
>  struct pci_sys_data {
> -#ifdef CONFIG_PCI_DOMAINS
> -	int		domain;
> -#endif
>  	struct list_head node;
>  	int		busnr;		/* primary bus number			*/
>  	u64		mem_offset;	/* bus->cpu memory mapping offset	*/
> diff --git a/arch/arm/include/asm/pci.h b/arch/arm/include/asm/pci.h
> index 7e95d85..585dc33 100644
> --- a/arch/arm/include/asm/pci.h
> +++ b/arch/arm/include/asm/pci.h
> @@ -18,13 +18,6 @@ static inline int pcibios_assign_all_busses(void)
>  }
> 
>  #ifdef CONFIG_PCI_DOMAINS
> -static inline int pci_domain_nr(struct pci_bus *bus)
> -{
> -	struct pci_sys_data *root = bus->sysdata;
> -
> -	return root->domain;
> -}
> -
>  static inline int pci_proc_domain(struct pci_bus *bus)
>  {
>  	return pci_domain_nr(bus);
> diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
> index 17a26c1..d8c2b4e 100644
> --- a/arch/arm/kernel/bios32.c
> +++ b/arch/arm/kernel/bios32.c
> @@ -11,6 +11,8 @@
>  #include <linux/slab.h>
>  #include <linux/init.h>
>  #include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/of_pci.h>
> 
>  #include <asm/mach-types.h>
>  #include <asm/mach/map.h>
> @@ -468,9 +470,6 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw,
>  		if (!sys)
>  			panic("PCI: unable to allocate sys data!");
> 
> -#ifdef CONFIG_PCI_DOMAINS
> -		sys->domain  = hw->domain;
> -#endif
>  		sys->busnr   = busnr;
>  		sys->swizzle = hw->swizzle;
>  		sys->map_irq = hw->map_irq;
> @@ -511,6 +510,27 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw,
>  	}
>  }
> 
> +#ifdef CONFIG_PCI_DOMAINS_GENERIC
> +static bool dt_domain_found;
> +
> +void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
> +{
> +	int domain = of_get_pci_domain_nr(parent->of_node);
> +
> +	if (domain >= 0) {
> +		dt_domain_found = true;
> +	} else if (dt_domain_found == true) {
> +		dev_err(parent, "Node %s is missing \"linux,pci-domain\" property in DT\n",
> +			parent->of_node->full_name);
> +		return;
> +	} else {
> +		domain = pci_get_new_domain_nr();
> +	}
> +
> +	bus->domain_nr = domain;
> +}
> +#endif
> +
>  void pci_common_init_dev(struct device *parent, struct hw_pci *hw)
>  {
>  	struct pci_sys_data *sys;
> diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
> index b1315e1..2b4c1f2 100644
> --- a/drivers/pci/host/pci-mvebu.c
> +++ b/drivers/pci/host/pci-mvebu.c
> @@ -722,17 +722,11 @@ static int mvebu_pcie_setup(int nr, struct pci_sys_data *sys)
>  {
>  	struct mvebu_pcie *pcie = sys_to_pcie(sys);
>  	int i;
> -	int domain = 0;
> 
> -#ifdef CONFIG_PCI_DOMAINS
> -	domain = sys->domain;
> -#endif
> -
> -	snprintf(pcie->mem_name, sizeof(pcie->mem_name), "PCI MEM %04x",
> -		 domain);
> +	snprintf(pcie->mem_name, sizeof(pcie->mem_name), "PCI MEM %04x", nr);
>  	pcie->mem.name = pcie->mem_name;
> 
> -	snprintf(pcie->io_name, sizeof(pcie->io_name), "PCI I/O %04x", domain);
> +	snprintf(pcie->io_name, sizeof(pcie->io_name), "PCI I/O %04x", nr);
>  	pcie->realio.name = pcie->io_name;
> 
>  	if (request_resource(&iomem_resource, &pcie->mem))
> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
> index dfed00a..6790b87 100644
> --- a/drivers/pci/host/pcie-designware.c
> +++ b/drivers/pci/host/pcie-designware.c
> @@ -502,9 +502,6 @@ int __init dw_pcie_host_init(struct pcie_port *pp)
>  	dw_pci.private_data = (void **)&pp;
> 
>  	pci_common_init_dev(pp->dev, &dw_pci);
> -#ifdef CONFIG_PCI_DOMAINS
> -	dw_pci.domain++;
> -#endif
> 
>  	return 0;
>  }
> diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c
> index 61158e0..b6b859e 100644
> --- a/drivers/pci/host/pcie-rcar.c
> +++ b/drivers/pci/host/pcie-rcar.c
> @@ -404,9 +404,6 @@ static void rcar_pcie_enable(struct rcar_pcie *pcie)
>  	rcar_pci.private_data = (void **)&pcie;
> 
>  	pci_common_init_dev(&pdev->dev, &rcar_pci);
> -#ifdef CONFIG_PCI_DOMAINS
> -	rcar_pci.domain++;
> -#endif
>  }
> 
>  static int phy_wait_for_ack(struct rcar_pcie *pcie)
> --
> 2.1.2
Murali Karicheri Nov. 3, 2014, 2:36 a.m. UTC | #9
>-----Original Message-----
>From: Jingoo Han [mailto:jg1.han@samsung.com]
>Sent: Sunday, November 02, 2014 8:18 PM
>To: 'Lorenzo Pieralisi'
>Cc: linux-arm-kernel@lists.infradead.org; linux-pci@vger.kernel.org; liviu.dudau@arm.com;
>'Arnd Bergmann'; 'Phil Edworthy'; 'Jason Gunthorpe'; 'Bjorn Helgaas'; 'Russell King'; 'Mohit
>Kumar'; 'Richard Zhu'; 'Lucas Stach'; KISHON VIJAY ABRAHAM; Karicheri, Muralidharan;
>'Jingoo Han'
>Subject: Re: [RFC PATCH 2/2] arm: pcibios: move to generic PCI domains
>
>On Thursday, October 30, 2014 8:45 PM, Lorenzo Pieralisi wrote:
>>
>> Most if not all ARM PCI host controller device drivers either ignore
>> the domain field in the pci_sys_data structure or just increment it
>> every time a host controller is probed, using it as a domain counter.
>>
>> Therefore, instead of relying on pci_sys_data to stash the domain
>> number in a standard location, ARM pcibios code can be moved to the
>> newly introduced generic PCI domains code, implemented in commits:
>>
>> commit 41e5c0f81d3e676d671d96a0a1fafb27abfbd9
>> ("of/pci: Add pci_get_new_domain_nr() and of_get_pci_domain_nr()")
>>
>> commit 670ba0c8883b576d0aec28bd7a838358a4be1
>> ("PCI: Add generic domain handling")
>>
>> In order to assign a domain number dynamically, the ARM pcibios
>> defines the function, called by core PCI code:
>>
>> void pci_bus_assign_domain_nr(...)
>>
>> that relies on a DT property to define the domain number or falls back
>> to a counter; its usage replaces the current domain assignment code in
>> PCI host controllers present in the kernel.
>>
>> Cc: Arnd Bergmann <arnd@arndb.de>
>> Cc: Phil Edworthy <phil.edworthy@renesas.com>
>> Cc: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
>> Cc: Jingoo Han <jg1.han@samsung.com>
>> Cc: Bjorn Helgaas <bhelgaas@google.com>
>> Cc: Russell King <linux@arm.linux.org.uk>
>> Cc: Mohit Kumar <mohit.kumar@st.com>
>> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>
>(+CC Richard Zhu, Lucas Stach, Kishon Vijay Abraham I, Murali Karicheri)
>
>Acked-by: Jingoo Han <jg1.han@samsung.com>
>
>For a long time, I have wanted the patch like this. Personally, I want to increase the build
>coverage and keep the same binary regardless of config options as possible; thus, I don't
>like #ifdef guards such as '#ifdef CONFIG_PCI_DOMAINS'. When I submitted the first patch
>for 'pcie-designware.c', I was not able to find a way to remove '#ifdef
>CONFIG_PCI_DOMAINS' from 'pcie-designware.c'.
>
>This patch looks good and I think that this patch will not make the problem. However, I
>didn't test this patch with Exynos platform, due to some reasons. So, I hope that other
>engineers test this patch on other SoC platforms related to designware PCIe.
>
>To Mohit Kumar (ST SPEAR13XX), Richard Zhu/Lucas Stach (IMX6)
>    Kishon Vijay Abraham I (TI DRA7XX), Murali Karicheri (TI KEYSTONE)
>
Jingoo,
I will be able to try this sometime after 11/9 as I am on travel.

Regards,
Murali

>Would you test this patch and share the result?
>It would be very helpful. Thank you.
>
>Best regards,
>Jingoo Han
>
>> ---
>>  arch/arm/Kconfig                   |  3 +++
>>  arch/arm/include/asm/mach/pci.h    |  3 ---
>>  arch/arm/include/asm/pci.h         |  7 -------
>>  arch/arm/kernel/bios32.c           | 26 +++++++++++++++++++++++---
>>  drivers/pci/host/pci-mvebu.c       | 10 ++--------
>>  drivers/pci/host/pcie-designware.c |  3 ---
>>  drivers/pci/host/pcie-rcar.c       |  3 ---
>>  7 files changed, 28 insertions(+), 27 deletions(-)
>>
>> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index
>> 89c4b5c..29544f0 100644
>> --- a/arch/arm/Kconfig
>> +++ b/arch/arm/Kconfig
>> @@ -1292,6 +1292,9 @@ config PCI_DOMAINS
>>  	bool
>>  	depends on PCI
>>
>> +config PCI_DOMAINS_GENERIC
>> +	def_bool PCI_DOMAINS
>> +
>>  config PCI_NANOENGINE
>>  	bool "BSE nanoEngine PCI support"
>>  	depends on SA1100_NANOENGINE
>> diff --git a/arch/arm/include/asm/mach/pci.h
>> b/arch/arm/include/asm/mach/pci.h index 7fc4278..f19f627 100644
>> --- a/arch/arm/include/asm/mach/pci.h
>> +++ b/arch/arm/include/asm/mach/pci.h
>> @@ -44,9 +44,6 @@ struct hw_pci {
>>   * Per-controller structure
>>   */
>>  struct pci_sys_data {
>> -#ifdef CONFIG_PCI_DOMAINS
>> -	int		domain;
>> -#endif
>>  	struct list_head node;
>>  	int		busnr;		/* primary bus number			*/
>>  	u64		mem_offset;	/* bus->cpu memory mapping offset	*/
>> diff --git a/arch/arm/include/asm/pci.h b/arch/arm/include/asm/pci.h
>> index 7e95d85..585dc33 100644
>> --- a/arch/arm/include/asm/pci.h
>> +++ b/arch/arm/include/asm/pci.h
>> @@ -18,13 +18,6 @@ static inline int pcibios_assign_all_busses(void)
>> }
>>
>>  #ifdef CONFIG_PCI_DOMAINS
>> -static inline int pci_domain_nr(struct pci_bus *bus) -{
>> -	struct pci_sys_data *root = bus->sysdata;
>> -
>> -	return root->domain;
>> -}
>> -
>>  static inline int pci_proc_domain(struct pci_bus *bus)  {
>>  	return pci_domain_nr(bus);
>> diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c index
>> 17a26c1..d8c2b4e 100644
>> --- a/arch/arm/kernel/bios32.c
>> +++ b/arch/arm/kernel/bios32.c
>> @@ -11,6 +11,8 @@
>>  #include <linux/slab.h>
>>  #include <linux/init.h>
>>  #include <linux/io.h>
>> +#include <linux/of.h>
>> +#include <linux/of_pci.h>
>>
>>  #include <asm/mach-types.h>
>>  #include <asm/mach/map.h>
>> @@ -468,9 +470,6 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci
>*hw,
>>  		if (!sys)
>>  			panic("PCI: unable to allocate sys data!");
>>
>> -#ifdef CONFIG_PCI_DOMAINS
>> -		sys->domain  = hw->domain;
>> -#endif
>>  		sys->busnr   = busnr;
>>  		sys->swizzle = hw->swizzle;
>>  		sys->map_irq = hw->map_irq;
>> @@ -511,6 +510,27 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci
>*hw,
>>  	}
>>  }
>>
>> +#ifdef CONFIG_PCI_DOMAINS_GENERIC
>> +static bool dt_domain_found;
>> +
>> +void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device
>> +*parent) {
>> +	int domain = of_get_pci_domain_nr(parent->of_node);
>> +
>> +	if (domain >= 0) {
>> +		dt_domain_found = true;
>> +	} else if (dt_domain_found == true) {
>> +		dev_err(parent, "Node %s is missing \"linux,pci-domain\" property in DT\n",
>> +			parent->of_node->full_name);
>> +		return;
>> +	} else {
>> +		domain = pci_get_new_domain_nr();
>> +	}
>> +
>> +	bus->domain_nr = domain;
>> +}
>> +#endif
>> +
>>  void pci_common_init_dev(struct device *parent, struct hw_pci *hw)  {
>>  	struct pci_sys_data *sys;
>> diff --git a/drivers/pci/host/pci-mvebu.c
>> b/drivers/pci/host/pci-mvebu.c index b1315e1..2b4c1f2 100644
>> --- a/drivers/pci/host/pci-mvebu.c
>> +++ b/drivers/pci/host/pci-mvebu.c
>> @@ -722,17 +722,11 @@ static int mvebu_pcie_setup(int nr, struct
>> pci_sys_data *sys)  {
>>  	struct mvebu_pcie *pcie = sys_to_pcie(sys);
>>  	int i;
>> -	int domain = 0;
>>
>> -#ifdef CONFIG_PCI_DOMAINS
>> -	domain = sys->domain;
>> -#endif
>> -
>> -	snprintf(pcie->mem_name, sizeof(pcie->mem_name), "PCI MEM %04x",
>> -		 domain);
>> +	snprintf(pcie->mem_name, sizeof(pcie->mem_name), "PCI MEM %04x",
>> +nr);
>>  	pcie->mem.name = pcie->mem_name;
>>
>> -	snprintf(pcie->io_name, sizeof(pcie->io_name), "PCI I/O %04x", domain);
>> +	snprintf(pcie->io_name, sizeof(pcie->io_name), "PCI I/O %04x", nr);
>>  	pcie->realio.name = pcie->io_name;
>>
>>  	if (request_resource(&iomem_resource, &pcie->mem)) diff --git
>> a/drivers/pci/host/pcie-designware.c
>> b/drivers/pci/host/pcie-designware.c
>> index dfed00a..6790b87 100644
>> --- a/drivers/pci/host/pcie-designware.c
>> +++ b/drivers/pci/host/pcie-designware.c
>> @@ -502,9 +502,6 @@ int __init dw_pcie_host_init(struct pcie_port *pp)
>>  	dw_pci.private_data = (void **)&pp;
>>
>>  	pci_common_init_dev(pp->dev, &dw_pci); -#ifdef CONFIG_PCI_DOMAINS
>> -	dw_pci.domain++;
>> -#endif
>>
>>  	return 0;
>>  }
>> diff --git a/drivers/pci/host/pcie-rcar.c
>> b/drivers/pci/host/pcie-rcar.c index 61158e0..b6b859e 100644
>> --- a/drivers/pci/host/pcie-rcar.c
>> +++ b/drivers/pci/host/pcie-rcar.c
>> @@ -404,9 +404,6 @@ static void rcar_pcie_enable(struct rcar_pcie *pcie)
>>  	rcar_pci.private_data = (void **)&pcie;
>>
>>  	pci_common_init_dev(&pdev->dev, &rcar_pci); -#ifdef
>> CONFIG_PCI_DOMAINS
>> -	rcar_pci.domain++;
>> -#endif
>>  }
>>
>>  static int phy_wait_for_ack(struct rcar_pcie *pcie)
>> --
>> 2.1.2
wangyijing Nov. 3, 2014, 3:48 a.m. UTC | #10
On 2014/10/30 19:44, Lorenzo Pieralisi wrote:
> Most if not all ARM PCI host controller device drivers either ignore the
> domain field in the pci_sys_data structure or just increment it every
> time a host controller is probed, using it as a domain counter.
> 
> Therefore, instead of relying on pci_sys_data to stash the domain number
> in a standard location, ARM pcibios code can be moved to the newly
> introduced generic PCI domains code, implemented in commits:
> 
> commit 41e5c0f81d3e676d671d96a0a1fafb27abfbd9
> ("of/pci: Add pci_get_new_domain_nr() and of_get_pci_domain_nr()")
> 
> commit 670ba0c8883b576d0aec28bd7a838358a4be1
> ("PCI: Add generic domain handling")
> 
> In order to assign a domain number dynamically, the ARM pcibios defines
> the function, called by core PCI code:
> 
> void pci_bus_assign_domain_nr(...)
> 
> that relies on a DT property to define the domain number or falls back to
> a counter; its usage replaces the current domain assignment code in PCI
> host controllers present in the kernel.
> 
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Phil Edworthy <phil.edworthy@renesas.com>
> Cc: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> Cc: Jingoo Han <jg1.han@samsung.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Russell King <linux@arm.linux.org.uk>
> Cc: Mohit Kumar <mohit.kumar@st.com>
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> ---
>  arch/arm/Kconfig                   |  3 +++
>  arch/arm/include/asm/mach/pci.h    |  3 ---
>  arch/arm/include/asm/pci.h         |  7 -------
>  arch/arm/kernel/bios32.c           | 26 +++++++++++++++++++++++---
>  drivers/pci/host/pci-mvebu.c       | 10 ++--------
>  drivers/pci/host/pcie-designware.c |  3 ---
>  drivers/pci/host/pcie-rcar.c       |  3 ---
>  7 files changed, 28 insertions(+), 27 deletions(-)
> 
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 89c4b5c..29544f0 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -1292,6 +1292,9 @@ config PCI_DOMAINS
>  	bool
>  	depends on PCI
>  
> +config PCI_DOMAINS_GENERIC
> +	def_bool PCI_DOMAINS
> +
>  config PCI_NANOENGINE
>  	bool "BSE nanoEngine PCI support"
>  	depends on SA1100_NANOENGINE
> diff --git a/arch/arm/include/asm/mach/pci.h b/arch/arm/include/asm/mach/pci.h
> index 7fc4278..f19f627 100644
> --- a/arch/arm/include/asm/mach/pci.h
> +++ b/arch/arm/include/asm/mach/pci.h
> @@ -44,9 +44,6 @@ struct hw_pci {
>   * Per-controller structure
>   */

domain in hw_pci also should be clean up, right ?


struct hw_pci {
#ifdef CONFIG_PCI_DOMAINS
	int		domain;
#endif
	struct pci_ops	*ops;
	int		nr_controllers;
	void		**private_data;


>  struct pci_sys_data {
> -#ifdef CONFIG_PCI_DOMAINS
> -	int		domain;
> -#endif
>  	struct list_head node;
>  	int		busnr;		/* primary bus number			*/
>  	u64		mem_offset;	/* bus->cpu memory mapping offset	*/
> diff --git a/arch/arm/include/asm/pci.h b/arch/arm/include/asm/pci.h
> index 7e95d85..585dc33 100644
> --- a/arch/arm/include/asm/pci.h
> +++ b/arch/arm/include/asm/pci.h
> @@ -18,13 +18,6 @@ static inline int pcibios_assign_all_busses(void)
>  }
>  
>  #ifdef CONFIG_PCI_DOMAINS
> -static inline int pci_domain_nr(struct pci_bus *bus)
> -{
> -	struct pci_sys_data *root = bus->sysdata;
> -
> -	return root->domain;
> -}
> -
>  static inline int pci_proc_domain(struct pci_bus *bus)
>  {
>  	return pci_domain_nr(bus);
> diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
> index 17a26c1..d8c2b4e 100644
> --- a/arch/arm/kernel/bios32.c
> +++ b/arch/arm/kernel/bios32.c
> @@ -11,6 +11,8 @@
>  #include <linux/slab.h>
>  #include <linux/init.h>
>  #include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/of_pci.h>
>  
>  #include <asm/mach-types.h>
>  #include <asm/mach/map.h>
> @@ -468,9 +470,6 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw,
>  		if (!sys)
>  			panic("PCI: unable to allocate sys data!");
>  
> -#ifdef CONFIG_PCI_DOMAINS
> -		sys->domain  = hw->domain;
> -#endif
>  		sys->busnr   = busnr;
>  		sys->swizzle = hw->swizzle;
>  		sys->map_irq = hw->map_irq;
> @@ -511,6 +510,27 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw,
>  	}
>  }
>  
> +#ifdef CONFIG_PCI_DOMAINS_GENERIC
> +static bool dt_domain_found;
> +
> +void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
> +{
> +	int domain = of_get_pci_domain_nr(parent->of_node);
> +
> +	if (domain >= 0) {
> +		dt_domain_found = true;
> +	} else if (dt_domain_found == true) {
> +		dev_err(parent, "Node %s is missing \"linux,pci-domain\" property in DT\n",
> +			parent->of_node->full_name);
> +		return;
> +	} else {
> +		domain = pci_get_new_domain_nr();
> +	}
> +
> +	bus->domain_nr = domain;
> +}
> +#endif
> +
>  void pci_common_init_dev(struct device *parent, struct hw_pci *hw)
>  {
>  	struct pci_sys_data *sys;
> diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
> index b1315e1..2b4c1f2 100644
> --- a/drivers/pci/host/pci-mvebu.c
> +++ b/drivers/pci/host/pci-mvebu.c
> @@ -722,17 +722,11 @@ static int mvebu_pcie_setup(int nr, struct pci_sys_data *sys)
>  {
>  	struct mvebu_pcie *pcie = sys_to_pcie(sys);
>  	int i;
> -	int domain = 0;
>  
> -#ifdef CONFIG_PCI_DOMAINS
> -	domain = sys->domain;
> -#endif
> -
> -	snprintf(pcie->mem_name, sizeof(pcie->mem_name), "PCI MEM %04x",
> -		 domain);
> +	snprintf(pcie->mem_name, sizeof(pcie->mem_name), "PCI MEM %04x", nr);
>  	pcie->mem.name = pcie->mem_name;
>  
> -	snprintf(pcie->io_name, sizeof(pcie->io_name), "PCI I/O %04x", domain);
> +	snprintf(pcie->io_name, sizeof(pcie->io_name), "PCI I/O %04x", nr);
>  	pcie->realio.name = pcie->io_name;
>  
>  	if (request_resource(&iomem_resource, &pcie->mem))
> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
> index dfed00a..6790b87 100644
> --- a/drivers/pci/host/pcie-designware.c
> +++ b/drivers/pci/host/pcie-designware.c
> @@ -502,9 +502,6 @@ int __init dw_pcie_host_init(struct pcie_port *pp)
>  	dw_pci.private_data = (void **)&pp;
>  
>  	pci_common_init_dev(pp->dev, &dw_pci);
> -#ifdef CONFIG_PCI_DOMAINS
> -	dw_pci.domain++;
> -#endif
>  
>  	return 0;
>  }
> diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c
> index 61158e0..b6b859e 100644
> --- a/drivers/pci/host/pcie-rcar.c
> +++ b/drivers/pci/host/pcie-rcar.c
> @@ -404,9 +404,6 @@ static void rcar_pcie_enable(struct rcar_pcie *pcie)
>  	rcar_pci.private_data = (void **)&pcie;
>  
>  	pci_common_init_dev(&pdev->dev, &rcar_pci);
> -#ifdef CONFIG_PCI_DOMAINS
> -	rcar_pci.domain++;
> -#endif
>  }
>  
>  static int phy_wait_for_ack(struct rcar_pcie *pcie)
>
Lorenzo Pieralisi Nov. 3, 2014, 10:49 a.m. UTC | #11
On Mon, Nov 03, 2014 at 03:48:23AM +0000, Yijing Wang wrote:
> On 2014/10/30 19:44, Lorenzo Pieralisi wrote:

[...]

> > diff --git a/arch/arm/include/asm/mach/pci.h b/arch/arm/include/asm/mach/pci.h
> > index 7fc4278..f19f627 100644
> > --- a/arch/arm/include/asm/mach/pci.h
> > +++ b/arch/arm/include/asm/mach/pci.h
> > @@ -44,9 +44,6 @@ struct hw_pci {
> >   * Per-controller structure
> >   */
> 
> domain in hw_pci also should be clean up, right ?
> 
> 
> struct hw_pci {
> #ifdef CONFIG_PCI_DOMAINS
> 	int		domain;
> #endif
> 	struct pci_ops	*ops;
> 	int		nr_controllers;
> 	void		**private_data;

Yes, thanks for spotting that.

Lorenzo
Lorenzo Pieralisi Nov. 3, 2014, 11:06 a.m. UTC | #12
Hi Phil,

On Fri, Oct 31, 2014 at 01:43:49PM +0000, Phil Edworthy wrote:
> Hi Lorenzo,
> 
> On 30 October 2014 11:45, Lorenzo wrote:
> > Most if not all ARM PCI host controller device drivers either ignore the
> > domain field in the pci_sys_data structure or just increment it every
> > time a host controller is probed, using it as a domain counter.
> > 
> > Therefore, instead of relying on pci_sys_data to stash the domain number
> > in a standard location, ARM pcibios code can be moved to the newly
> > introduced generic PCI domains code, implemented in commits:
> > 
> > commit 41e5c0f81d3e676d671d96a0a1fafb27abfbd9
> > ("of/pci: Add pci_get_new_domain_nr() and of_get_pci_domain_nr()")
> > 
> > commit 670ba0c8883b576d0aec28bd7a838358a4be1
> > ("PCI: Add generic domain handling")
> > 
> > In order to assign a domain number dynamically, the ARM pcibios defines
> > the function, called by core PCI code:
> > 
> > void pci_bus_assign_domain_nr(...)
> > 
> > that relies on a DT property to define the domain number or falls back to
> > a counter; its usage replaces the current domain assignment code in PCI
> > host controllers present in the kernel.
> > 
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: Phil Edworthy <phil.edworthy@renesas.com>
> > Cc: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> > Cc: Jingoo Han <jg1.han@samsung.com>
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: Russell King <linux@arm.linux.org.uk>
> > Cc: Mohit Kumar <mohit.kumar@st.com>
> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> 
> This patch fixes a current problem with R-Car devices where there is an
> internal PCI bridge and an external PCIe bridge on the devices. Both drivers
> work independently but need to be on different domains. Just needed to enable
> PCI_DOMAINS along with this.
> I've done basic testing that the internal PCI and external PCIe work at the
> same time.
> 
> Acked-by: Phil Edworthy <phil.edworthy@renesas.com>

Thank you very much.

Lorenzo
Lorenzo Pieralisi Nov. 3, 2014, 11:23 a.m. UTC | #13
On Mon, Nov 03, 2014 at 01:18:12AM +0000, Jingoo Han wrote:
> On Thursday, October 30, 2014 8:45 PM, Lorenzo Pieralisi wrote:
> >
> > Most if not all ARM PCI host controller device drivers either ignore the
> > domain field in the pci_sys_data structure or just increment it every
> > time a host controller is probed, using it as a domain counter.
> >
> > Therefore, instead of relying on pci_sys_data to stash the domain number
> > in a standard location, ARM pcibios code can be moved to the newly
> > introduced generic PCI domains code, implemented in commits:
> >
> > commit 41e5c0f81d3e676d671d96a0a1fafb27abfbd9
> > ("of/pci: Add pci_get_new_domain_nr() and of_get_pci_domain_nr()")
> >
> > commit 670ba0c8883b576d0aec28bd7a838358a4be1
> > ("PCI: Add generic domain handling")
> >
> > In order to assign a domain number dynamically, the ARM pcibios defines
> > the function, called by core PCI code:
> >
> > void pci_bus_assign_domain_nr(...)
> >
> > that relies on a DT property to define the domain number or falls back to
> > a counter; its usage replaces the current domain assignment code in PCI
> > host controllers present in the kernel.
> >
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: Phil Edworthy <phil.edworthy@renesas.com>
> > Cc: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> > Cc: Jingoo Han <jg1.han@samsung.com>
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: Russell King <linux@arm.linux.org.uk>
> > Cc: Mohit Kumar <mohit.kumar@st.com>
> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> 
> (+CC Richard Zhu, Lucas Stach, Kishon Vijay Abraham I, Murali Karicheri)
> 
> Acked-by: Jingoo Han <jg1.han@samsung.com>

Thank you Jingoo.

> For a long time, I have wanted the patch like this. Personally,
> I want to increase the build coverage and keep the same binary
> regardless of config options as possible; thus, I don't like
> #ifdef guards such as '#ifdef CONFIG_PCI_DOMAINS'. When I submitted
> the first patch for 'pcie-designware.c', I was not able to find
> a way to remove '#ifdef CONFIG_PCI_DOMAINS' from 'pcie-designware.c'.
> 
> This patch looks good and I think that this patch will not make
> the problem. However, I didn't test this patch with Exynos platform,
> due to some reasons. So, I hope that other engineers test this patch
> on other SoC platforms related to designware PCIe.
> 
> To Mohit Kumar (ST SPEAR13XX), Richard Zhu/Lucas Stach (IMX6)
>     Kishon Vijay Abraham I (TI DRA7XX), Murali Karicheri (TI KEYSTONE)
> 
> Would you test this patch and share the result?
> It would be very helpful. Thank you.

I am happy to wait for a week or two before asking to add this code to -next,
it would be definitely great to get more coverage on pcie-designware.

Thanks,
Lorenzo

> Best regards,
> Jingoo Han
> 
> > ---
> >  arch/arm/Kconfig                   |  3 +++
> >  arch/arm/include/asm/mach/pci.h    |  3 ---
> >  arch/arm/include/asm/pci.h         |  7 -------
> >  arch/arm/kernel/bios32.c           | 26 +++++++++++++++++++++++---
> >  drivers/pci/host/pci-mvebu.c       | 10 ++--------
> >  drivers/pci/host/pcie-designware.c |  3 ---
> >  drivers/pci/host/pcie-rcar.c       |  3 ---
> >  7 files changed, 28 insertions(+), 27 deletions(-)
> >
> > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> > index 89c4b5c..29544f0 100644
> > --- a/arch/arm/Kconfig
> > +++ b/arch/arm/Kconfig
> > @@ -1292,6 +1292,9 @@ config PCI_DOMAINS
> >       bool
> >       depends on PCI
> >
> > +config PCI_DOMAINS_GENERIC
> > +     def_bool PCI_DOMAINS
> > +
> >  config PCI_NANOENGINE
> >       bool "BSE nanoEngine PCI support"
> >       depends on SA1100_NANOENGINE
> > diff --git a/arch/arm/include/asm/mach/pci.h b/arch/arm/include/asm/mach/pci.h
> > index 7fc4278..f19f627 100644
> > --- a/arch/arm/include/asm/mach/pci.h
> > +++ b/arch/arm/include/asm/mach/pci.h
> > @@ -44,9 +44,6 @@ struct hw_pci {
> >   * Per-controller structure
> >   */
> >  struct pci_sys_data {
> > -#ifdef CONFIG_PCI_DOMAINS
> > -     int             domain;
> > -#endif
> >       struct list_head node;
> >       int             busnr;          /* primary bus number                   */
> >       u64             mem_offset;     /* bus->cpu memory mapping offset       */
> > diff --git a/arch/arm/include/asm/pci.h b/arch/arm/include/asm/pci.h
> > index 7e95d85..585dc33 100644
> > --- a/arch/arm/include/asm/pci.h
> > +++ b/arch/arm/include/asm/pci.h
> > @@ -18,13 +18,6 @@ static inline int pcibios_assign_all_busses(void)
> >  }
> >
> >  #ifdef CONFIG_PCI_DOMAINS
> > -static inline int pci_domain_nr(struct pci_bus *bus)
> > -{
> > -     struct pci_sys_data *root = bus->sysdata;
> > -
> > -     return root->domain;
> > -}
> > -
> >  static inline int pci_proc_domain(struct pci_bus *bus)
> >  {
> >       return pci_domain_nr(bus);
> > diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
> > index 17a26c1..d8c2b4e 100644
> > --- a/arch/arm/kernel/bios32.c
> > +++ b/arch/arm/kernel/bios32.c
> > @@ -11,6 +11,8 @@
> >  #include <linux/slab.h>
> >  #include <linux/init.h>
> >  #include <linux/io.h>
> > +#include <linux/of.h>
> > +#include <linux/of_pci.h>
> >
> >  #include <asm/mach-types.h>
> >  #include <asm/mach/map.h>
> > @@ -468,9 +470,6 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw,
> >               if (!sys)
> >                       panic("PCI: unable to allocate sys data!");
> >
> > -#ifdef CONFIG_PCI_DOMAINS
> > -             sys->domain  = hw->domain;
> > -#endif
> >               sys->busnr   = busnr;
> >               sys->swizzle = hw->swizzle;
> >               sys->map_irq = hw->map_irq;
> > @@ -511,6 +510,27 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw,
> >       }
> >  }
> >
> > +#ifdef CONFIG_PCI_DOMAINS_GENERIC
> > +static bool dt_domain_found;
> > +
> > +void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
> > +{
> > +     int domain = of_get_pci_domain_nr(parent->of_node);
> > +
> > +     if (domain >= 0) {
> > +             dt_domain_found = true;
> > +     } else if (dt_domain_found == true) {
> > +             dev_err(parent, "Node %s is missing \"linux,pci-domain\" property in DT\n",
> > +                     parent->of_node->full_name);
> > +             return;
> > +     } else {
> > +             domain = pci_get_new_domain_nr();
> > +     }
> > +
> > +     bus->domain_nr = domain;
> > +}
> > +#endif
> > +
> >  void pci_common_init_dev(struct device *parent, struct hw_pci *hw)
> >  {
> >       struct pci_sys_data *sys;
> > diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
> > index b1315e1..2b4c1f2 100644
> > --- a/drivers/pci/host/pci-mvebu.c
> > +++ b/drivers/pci/host/pci-mvebu.c
> > @@ -722,17 +722,11 @@ static int mvebu_pcie_setup(int nr, struct pci_sys_data *sys)
> >  {
> >       struct mvebu_pcie *pcie = sys_to_pcie(sys);
> >       int i;
> > -     int domain = 0;
> >
> > -#ifdef CONFIG_PCI_DOMAINS
> > -     domain = sys->domain;
> > -#endif
> > -
> > -     snprintf(pcie->mem_name, sizeof(pcie->mem_name), "PCI MEM %04x",
> > -              domain);
> > +     snprintf(pcie->mem_name, sizeof(pcie->mem_name), "PCI MEM %04x", nr);
> >       pcie->mem.name = pcie->mem_name;
> >
> > -     snprintf(pcie->io_name, sizeof(pcie->io_name), "PCI I/O %04x", domain);
> > +     snprintf(pcie->io_name, sizeof(pcie->io_name), "PCI I/O %04x", nr);
> >       pcie->realio.name = pcie->io_name;
> >
> >       if (request_resource(&iomem_resource, &pcie->mem))
> > diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
> > index dfed00a..6790b87 100644
> > --- a/drivers/pci/host/pcie-designware.c
> > +++ b/drivers/pci/host/pcie-designware.c
> > @@ -502,9 +502,6 @@ int __init dw_pcie_host_init(struct pcie_port *pp)
> >       dw_pci.private_data = (void **)&pp;
> >
> >       pci_common_init_dev(pp->dev, &dw_pci);
> > -#ifdef CONFIG_PCI_DOMAINS
> > -     dw_pci.domain++;
> > -#endif
> >
> >       return 0;
> >  }
> > diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c
> > index 61158e0..b6b859e 100644
> > --- a/drivers/pci/host/pcie-rcar.c
> > +++ b/drivers/pci/host/pcie-rcar.c
> > @@ -404,9 +404,6 @@ static void rcar_pcie_enable(struct rcar_pcie *pcie)
> >       rcar_pci.private_data = (void **)&pcie;
> >
> >       pci_common_init_dev(&pdev->dev, &rcar_pci);
> > -#ifdef CONFIG_PCI_DOMAINS
> > -     rcar_pci.domain++;
> > -#endif
> >  }
> >
> >  static int phy_wait_for_ack(struct rcar_pcie *pcie)
> > --
> > 2.1.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
Lucas Stach Nov. 3, 2014, 11:33 a.m. UTC | #14
Am Montag, den 03.11.2014, 11:23 +0000 schrieb Lorenzo Pieralisi:
> On Mon, Nov 03, 2014 at 01:18:12AM +0000, Jingoo Han wrote:
> > On Thursday, October 30, 2014 8:45 PM, Lorenzo Pieralisi wrote:
> > >
> > > Most if not all ARM PCI host controller device drivers either ignore the
> > > domain field in the pci_sys_data structure or just increment it every
> > > time a host controller is probed, using it as a domain counter.
> > >
> > > Therefore, instead of relying on pci_sys_data to stash the domain number
> > > in a standard location, ARM pcibios code can be moved to the newly
> > > introduced generic PCI domains code, implemented in commits:
> > >
> > > commit 41e5c0f81d3e676d671d96a0a1fafb27abfbd9
> > > ("of/pci: Add pci_get_new_domain_nr() and of_get_pci_domain_nr()")
> > >
> > > commit 670ba0c8883b576d0aec28bd7a838358a4be1
> > > ("PCI: Add generic domain handling")
> > >
> > > In order to assign a domain number dynamically, the ARM pcibios defines
> > > the function, called by core PCI code:
> > >
> > > void pci_bus_assign_domain_nr(...)
> > >
> > > that relies on a DT property to define the domain number or falls back to
> > > a counter; its usage replaces the current domain assignment code in PCI
> > > host controllers present in the kernel.
> > >
> > > Cc: Arnd Bergmann <arnd@arndb.de>
> > > Cc: Phil Edworthy <phil.edworthy@renesas.com>
> > > Cc: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> > > Cc: Jingoo Han <jg1.han@samsung.com>
> > > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > > Cc: Russell King <linux@arm.linux.org.uk>
> > > Cc: Mohit Kumar <mohit.kumar@st.com>
> > > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > 
> > (+CC Richard Zhu, Lucas Stach, Kishon Vijay Abraham I, Murali Karicheri)
> > 
> > Acked-by: Jingoo Han <jg1.han@samsung.com>
> 
> Thank you Jingoo.
> 
> > For a long time, I have wanted the patch like this. Personally,
> > I want to increase the build coverage and keep the same binary
> > regardless of config options as possible; thus, I don't like
> > #ifdef guards such as '#ifdef CONFIG_PCI_DOMAINS'. When I submitted
> > the first patch for 'pcie-designware.c', I was not able to find
> > a way to remove '#ifdef CONFIG_PCI_DOMAINS' from 'pcie-designware.c'.
> > 
> > This patch looks good and I think that this patch will not make
> > the problem. However, I didn't test this patch with Exynos platform,
> > due to some reasons. So, I hope that other engineers test this patch
> > on other SoC platforms related to designware PCIe.
> > 
> > To Mohit Kumar (ST SPEAR13XX), Richard Zhu/Lucas Stach (IMX6)
> >     Kishon Vijay Abraham I (TI DRA7XX), Murali Karicheri (TI KEYSTONE)
> > 
> > Would you test this patch and share the result?
> > It would be very helpful. Thank you.
> 
> I am happy to wait for a week or two before asking to add this code to -next,
> it would be definitely great to get more coverage on pcie-designware.
> 
I've just tested this on i.MX6 on top of 3.18-rc3. Given that we only
use a single domain there this patch should obviously not break
anything. Testing backs this claim, so:

Tested-by: Lucas Stach <l.stach@pengutronix.de>

I also looked through the patch briefly and it seems sane to me, so this
is also:

Acked-by: Lucas Stach <l.stach@pengutronix.de>

I don't like the fact that a proper binding documentation is missing for
the "linux,pci-domain" property, but this isn't the fault of this
patchset.

Regards,
Lucas
Jingoo Han Nov. 3, 2014, 12:13 p.m. UTC | #15
On Monday, November 03, 2014 8:34 PM, Lucas Stach wrote:
> Am Montag, den 03.11.2014, 11:23 +0000 schrieb Lorenzo Pieralisi:
> > On Mon, Nov 03, 2014 at 01:18:12AM +0000, Jingoo Han wrote:
> > > On Thursday, October 30, 2014 8:45 PM, Lorenzo Pieralisi wrote:
> > > >
> > > > Most if not all ARM PCI host controller device drivers either ignore the
> > > > domain field in the pci_sys_data structure or just increment it every
> > > > time a host controller is probed, using it as a domain counter.
> > > >
> > > > Therefore, instead of relying on pci_sys_data to stash the domain number
> > > > in a standard location, ARM pcibios code can be moved to the newly
> > > > introduced generic PCI domains code, implemented in commits:
> > > >
> > > > commit 41e5c0f81d3e676d671d96a0a1fafb27abfbd9
> > > > ("of/pci: Add pci_get_new_domain_nr() and of_get_pci_domain_nr()")
> > > >
> > > > commit 670ba0c8883b576d0aec28bd7a838358a4be1
> > > > ("PCI: Add generic domain handling")
> > > >
> > > > In order to assign a domain number dynamically, the ARM pcibios defines
> > > > the function, called by core PCI code:
> > > >
> > > > void pci_bus_assign_domain_nr(...)
> > > >
> > > > that relies on a DT property to define the domain number or falls back to
> > > > a counter; its usage replaces the current domain assignment code in PCI
> > > > host controllers present in the kernel.
> > > >
> > > > Cc: Arnd Bergmann <arnd@arndb.de>
> > > > Cc: Phil Edworthy <phil.edworthy@renesas.com>
> > > > Cc: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> > > > Cc: Jingoo Han <jg1.han@samsung.com>
> > > > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > > > Cc: Russell King <linux@arm.linux.org.uk>
> > > > Cc: Mohit Kumar <mohit.kumar@st.com>
> > > > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > >
> > > (+CC Richard Zhu, Lucas Stach, Kishon Vijay Abraham I, Murali Karicheri)
> > >
> > > Acked-by: Jingoo Han <jg1.han@samsung.com>
> >
> > Thank you Jingoo.
> >
> > > For a long time, I have wanted the patch like this. Personally,
> > > I want to increase the build coverage and keep the same binary
> > > regardless of config options as possible; thus, I don't like
> > > #ifdef guards such as '#ifdef CONFIG_PCI_DOMAINS'. When I submitted
> > > the first patch for 'pcie-designware.c', I was not able to find
> > > a way to remove '#ifdef CONFIG_PCI_DOMAINS' from 'pcie-designware.c'.
> > >
> > > This patch looks good and I think that this patch will not make
> > > the problem. However, I didn't test this patch with Exynos platform,
> > > due to some reasons. So, I hope that other engineers test this patch
> > > on other SoC platforms related to designware PCIe.
> > >
> > > To Mohit Kumar (ST SPEAR13XX), Richard Zhu/Lucas Stach (IMX6)
> > >     Kishon Vijay Abraham I (TI DRA7XX), Murali Karicheri (TI KEYSTONE)
> > >
> > > Would you test this patch and share the result?
> > > It would be very helpful. Thank you.
> >
> > I am happy to wait for a week or two before asking to add this code to -next,
> > it would be definitely great to get more coverage on pcie-designware.
> >
> I've just tested this on i.MX6 on top of 3.18-rc3. Given that we only
> use a single domain there this patch should obviously not break
> anything. Testing backs this claim, so:
> 
> Tested-by: Lucas Stach <l.stach@pengutronix.de>

Thank you for your testing!
In fact, Exynos uses multi domain structures.

> 
> I also looked through the patch briefly and it seems sane to me, so this
> is also:
> 
> Acked-by: Lucas Stach <l.stach@pengutronix.de>
> 
> I don't like the fact that a proper binding documentation is missing for
> the "linux,pci-domain" property, but this isn't the fault of this
> patchset.

I agree with your opinion. I also thought that the binding documentation
for the "linux,pci-domain" property is needed.

> 
> Regards,
> Lucas
> 
> --
> Pengutronix e.K.             | Lucas Stach                 |
> Industrial Linux Solutions   | http://www.pengutronix.de/  |
Simon Horman Nov. 3, 2014, 11:26 p.m. UTC | #16
On Fri, Oct 31, 2014 at 05:04:49PM +0000, Phil Edworthy wrote:
> Hi Bjorn,
> 
> On 31 October 2014 16:37, Bjorn wrote:
> > On Fri, Oct 31, 2014 at 7:43 AM, Phil Edworthy
> > <phil.edworthy@renesas.com> wrote:
> > > Hi Lorenzo,
> > >
> > > On 30 October 2014 11:45, Lorenzo wrote:
> > >> Most if not all ARM PCI host controller device drivers either ignore the
> > >> domain field in the pci_sys_data structure or just increment it every
> > >> time a host controller is probed, using it as a domain counter.
> > >>
> > >> Therefore, instead of relying on pci_sys_data to stash the domain number
> > >> in a standard location, ARM pcibios code can be moved to the newly
> > >> introduced generic PCI domains code, implemented in commits:
> > >>
> > >> commit 41e5c0f81d3e676d671d96a0a1fafb27abfbd9
> > >> ("of/pci: Add pci_get_new_domain_nr() and of_get_pci_domain_nr()")
> > >>
> > >> commit 670ba0c8883b576d0aec28bd7a838358a4be1
> > >> ("PCI: Add generic domain handling")
> > >>
> > >> In order to assign a domain number dynamically, the ARM pcibios defines
> > >> the function, called by core PCI code:
> > >>
> > >> void pci_bus_assign_domain_nr(...)
> > >>
> > >> that relies on a DT property to define the domain number or falls back to
> > >> a counter; its usage replaces the current domain assignment code in PCI
> > >> host controllers present in the kernel.
> > >>
> > >> Cc: Arnd Bergmann <arnd@arndb.de>
> > >> Cc: Phil Edworthy <phil.edworthy@renesas.com>
> > >> Cc: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> > >> Cc: Jingoo Han <jg1.han@samsung.com>
> > >> Cc: Bjorn Helgaas <bhelgaas@google.com>
> > >> Cc: Russell King <linux@arm.linux.org.uk>
> > >> Cc: Mohit Kumar <mohit.kumar@st.com>
> > >> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > >
> > > This patch fixes a current problem with R-Car devices where there is an
> > > internal PCI bridge and an external PCIe bridge on the devices. Both drivers
> > > work independently but need to be on different domains. Just needed to
> > enable
> > > PCI_DOMAINS along with this.
> > > I've done basic testing that the internal PCI and external PCIe work at the
> > > same time.
> > 
> > Hi Phil,
> > 
> > Thanks for testing this.  Can you give me some more guidance on where
> > you'd like to see this merged?  Until your comment about this fixing a
> > current problem on R-Car, I probably would have considered this to be
> > a cleanup and enhancement and hence material for v3.19.  But if R-Car
> > is actually broken and this fixes it, maybe this should go in for
> > v3.18 instead.
> I don’t think its urgent as most of our customers use LTSI kernels, e.g. v3.10.
> Renesas typically provide out-of-tree BSPs with upstream code back ported, along
> with other patches. Simon Horman (Renesas maintainer, cc'd) generally handles
> the back ports, so I'll defer to his opinion.

Hi Phil,

from my point of view it would be best if this went into -stable if it is a
fix. However, as you point out, from a customer point of view it shouldn't
be a big deal as they should get backports via our LTSI kernel regardless of
if the patch goes through stable or not.

> > If it is currently broken, is there a point where it broke?  I assume
> > it used to work at one time.  If there's a commit that broke it, it
> > would be nice to reference that in the changelog and explain exactly
> > what was broken and how this fixes it.
> Both the internal PCI and external PCIe drivers work on their own, but have
> never worked at the same time. I think it was just unfortunate timing when I
> added the PCIe driver. At that time, the internal PCI driver didn’t have the
> relevant DT nodes for the board I was using so I didn't see any conflicts.
> 
> Thanks
> Phil
Liviu Dudau Nov. 4, 2014, 11:44 a.m. UTC | #17
On Mon, Nov 03, 2014 at 11:26:25PM +0000, Simon Horman wrote:
> On Fri, Oct 31, 2014 at 05:04:49PM +0000, Phil Edworthy wrote:
> > Hi Bjorn,
> > 
> > On 31 October 2014 16:37, Bjorn wrote:
> > > On Fri, Oct 31, 2014 at 7:43 AM, Phil Edworthy
> > > <phil.edworthy@renesas.com> wrote:
> > > > Hi Lorenzo,
> > > >
> > > > On 30 October 2014 11:45, Lorenzo wrote:
> > > >> Most if not all ARM PCI host controller device drivers either ignore the
> > > >> domain field in the pci_sys_data structure or just increment it every
> > > >> time a host controller is probed, using it as a domain counter.
> > > >>
> > > >> Therefore, instead of relying on pci_sys_data to stash the domain number
> > > >> in a standard location, ARM pcibios code can be moved to the newly
> > > >> introduced generic PCI domains code, implemented in commits:
> > > >>
> > > >> commit 41e5c0f81d3e676d671d96a0a1fafb27abfbd9
> > > >> ("of/pci: Add pci_get_new_domain_nr() and of_get_pci_domain_nr()")
> > > >>
> > > >> commit 670ba0c8883b576d0aec28bd7a838358a4be1
> > > >> ("PCI: Add generic domain handling")
> > > >>
> > > >> In order to assign a domain number dynamically, the ARM pcibios defines
> > > >> the function, called by core PCI code:
> > > >>
> > > >> void pci_bus_assign_domain_nr(...)
> > > >>
> > > >> that relies on a DT property to define the domain number or falls back to
> > > >> a counter; its usage replaces the current domain assignment code in PCI
> > > >> host controllers present in the kernel.
> > > >>
> > > >> Cc: Arnd Bergmann <arnd@arndb.de>
> > > >> Cc: Phil Edworthy <phil.edworthy@renesas.com>
> > > >> Cc: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> > > >> Cc: Jingoo Han <jg1.han@samsung.com>
> > > >> Cc: Bjorn Helgaas <bhelgaas@google.com>
> > > >> Cc: Russell King <linux@arm.linux.org.uk>
> > > >> Cc: Mohit Kumar <mohit.kumar@st.com>
> > > >> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > > >
> > > > This patch fixes a current problem with R-Car devices where there is an
> > > > internal PCI bridge and an external PCIe bridge on the devices. Both drivers
> > > > work independently but need to be on different domains. Just needed to
> > > enable
> > > > PCI_DOMAINS along with this.
> > > > I've done basic testing that the internal PCI and external PCIe work at the
> > > > same time.
> > > 
> > > Hi Phil,
> > > 
> > > Thanks for testing this.  Can you give me some more guidance on where
> > > you'd like to see this merged?  Until your comment about this fixing a
> > > current problem on R-Car, I probably would have considered this to be
> > > a cleanup and enhancement and hence material for v3.19.  But if R-Car
> > > is actually broken and this fixes it, maybe this should go in for
> > > v3.18 instead.
> > I don’t think its urgent as most of our customers use LTSI kernels, e.g. v3.10.
> > Renesas typically provide out-of-tree BSPs with upstream code back ported, along
> > with other patches. Simon Horman (Renesas maintainer, cc'd) generally handles
> > the back ports, so I'll defer to his opinion.
> 
> Hi Phil,
> 
> from my point of view it would be best if this went into -stable if it is a
> fix. However, as you point out, from a customer point of view it shouldn't
> be a big deal as they should get backports via our LTSI kernel regardless of
> if the patch goes through stable or not.

Unfortunately this patch depends on a series that only got in in v3.18-rc1, so
I don't think -stable is a workable target.

Best regards,
Liviu

> 
> > > If it is currently broken, is there a point where it broke?  I assume
> > > it used to work at one time.  If there's a commit that broke it, it
> > > would be nice to reference that in the changelog and explain exactly
> > > what was broken and how this fixes it.
> > Both the internal PCI and external PCIe drivers work on their own, but have
> > never worked at the same time. I think it was just unfortunate timing when I
> > added the PCIe driver. At that time, the internal PCI driver didn’t have the
> > relevant DT nodes for the board I was using so I didn't see any conflicts.
> > 
> > Thanks
> > Phil
>
diff mbox

Patch

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 89c4b5c..29544f0 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -1292,6 +1292,9 @@  config PCI_DOMAINS
 	bool
 	depends on PCI
 
+config PCI_DOMAINS_GENERIC
+	def_bool PCI_DOMAINS
+
 config PCI_NANOENGINE
 	bool "BSE nanoEngine PCI support"
 	depends on SA1100_NANOENGINE
diff --git a/arch/arm/include/asm/mach/pci.h b/arch/arm/include/asm/mach/pci.h
index 7fc4278..f19f627 100644
--- a/arch/arm/include/asm/mach/pci.h
+++ b/arch/arm/include/asm/mach/pci.h
@@ -44,9 +44,6 @@  struct hw_pci {
  * Per-controller structure
  */
 struct pci_sys_data {
-#ifdef CONFIG_PCI_DOMAINS
-	int		domain;
-#endif
 	struct list_head node;
 	int		busnr;		/* primary bus number			*/
 	u64		mem_offset;	/* bus->cpu memory mapping offset	*/
diff --git a/arch/arm/include/asm/pci.h b/arch/arm/include/asm/pci.h
index 7e95d85..585dc33 100644
--- a/arch/arm/include/asm/pci.h
+++ b/arch/arm/include/asm/pci.h
@@ -18,13 +18,6 @@  static inline int pcibios_assign_all_busses(void)
 }
 
 #ifdef CONFIG_PCI_DOMAINS
-static inline int pci_domain_nr(struct pci_bus *bus)
-{
-	struct pci_sys_data *root = bus->sysdata;
-
-	return root->domain;
-}
-
 static inline int pci_proc_domain(struct pci_bus *bus)
 {
 	return pci_domain_nr(bus);
diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
index 17a26c1..d8c2b4e 100644
--- a/arch/arm/kernel/bios32.c
+++ b/arch/arm/kernel/bios32.c
@@ -11,6 +11,8 @@ 
 #include <linux/slab.h>
 #include <linux/init.h>
 #include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_pci.h>
 
 #include <asm/mach-types.h>
 #include <asm/mach/map.h>
@@ -468,9 +470,6 @@  static void pcibios_init_hw(struct device *parent, struct hw_pci *hw,
 		if (!sys)
 			panic("PCI: unable to allocate sys data!");
 
-#ifdef CONFIG_PCI_DOMAINS
-		sys->domain  = hw->domain;
-#endif
 		sys->busnr   = busnr;
 		sys->swizzle = hw->swizzle;
 		sys->map_irq = hw->map_irq;
@@ -511,6 +510,27 @@  static void pcibios_init_hw(struct device *parent, struct hw_pci *hw,
 	}
 }
 
+#ifdef CONFIG_PCI_DOMAINS_GENERIC
+static bool dt_domain_found;
+
+void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
+{
+	int domain = of_get_pci_domain_nr(parent->of_node);
+
+	if (domain >= 0) {
+		dt_domain_found = true;
+	} else if (dt_domain_found == true) {
+		dev_err(parent, "Node %s is missing \"linux,pci-domain\" property in DT\n",
+			parent->of_node->full_name);
+		return;
+	} else {
+		domain = pci_get_new_domain_nr();
+	}
+
+	bus->domain_nr = domain;
+}
+#endif
+
 void pci_common_init_dev(struct device *parent, struct hw_pci *hw)
 {
 	struct pci_sys_data *sys;
diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
index b1315e1..2b4c1f2 100644
--- a/drivers/pci/host/pci-mvebu.c
+++ b/drivers/pci/host/pci-mvebu.c
@@ -722,17 +722,11 @@  static int mvebu_pcie_setup(int nr, struct pci_sys_data *sys)
 {
 	struct mvebu_pcie *pcie = sys_to_pcie(sys);
 	int i;
-	int domain = 0;
 
-#ifdef CONFIG_PCI_DOMAINS
-	domain = sys->domain;
-#endif
-
-	snprintf(pcie->mem_name, sizeof(pcie->mem_name), "PCI MEM %04x",
-		 domain);
+	snprintf(pcie->mem_name, sizeof(pcie->mem_name), "PCI MEM %04x", nr);
 	pcie->mem.name = pcie->mem_name;
 
-	snprintf(pcie->io_name, sizeof(pcie->io_name), "PCI I/O %04x", domain);
+	snprintf(pcie->io_name, sizeof(pcie->io_name), "PCI I/O %04x", nr);
 	pcie->realio.name = pcie->io_name;
 
 	if (request_resource(&iomem_resource, &pcie->mem))
diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
index dfed00a..6790b87 100644
--- a/drivers/pci/host/pcie-designware.c
+++ b/drivers/pci/host/pcie-designware.c
@@ -502,9 +502,6 @@  int __init dw_pcie_host_init(struct pcie_port *pp)
 	dw_pci.private_data = (void **)&pp;
 
 	pci_common_init_dev(pp->dev, &dw_pci);
-#ifdef CONFIG_PCI_DOMAINS
-	dw_pci.domain++;
-#endif
 
 	return 0;
 }
diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c
index 61158e0..b6b859e 100644
--- a/drivers/pci/host/pcie-rcar.c
+++ b/drivers/pci/host/pcie-rcar.c
@@ -404,9 +404,6 @@  static void rcar_pcie_enable(struct rcar_pcie *pcie)
 	rcar_pci.private_data = (void **)&pcie;
 
 	pci_common_init_dev(&pdev->dev, &rcar_pci);
-#ifdef CONFIG_PCI_DOMAINS
-	rcar_pci.domain++;
-#endif
 }
 
 static int phy_wait_for_ack(struct rcar_pcie *pcie)