[PATCH v2 0/2] arm: pcibios: remove pci_sys_data domain

[PATCH v2 0/2] arm: pcibios: remove pci_sys_data domain

[Lorenzo Pieralisi]

This patchset is a v2 of a previous posting:

http://lists.infradead.org/pipermail/linux-arm-kernel/2014-October/298537.html

v1 => v2 changelog

- Removed stale hw_pci domain member
- Reworked pci-mvebu domain handling according to review
- Rebased against 3.18-rc3 and updated the logs
- Dropped RFC status

Original cover letter:
----
This patchset is a first RFC stab at removing the dependency on pci_sys_data
domain field on ARM platforms and by replacing it with generic code that
stashes the domain value in the pci_bus control structure, introduced in

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

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

All the drivers converted (apart from PCIe designware, tested on iMX6SL)
were only compile tested for lack of HW, so along some comments, testing
and verifying that patchset does not break any existing platform are
very appreciated.

Code in drivers/pci/pci-mvebu.c has been changed to add a domain
number to PCI resources by using the nr value coming from the setup
pcibios32 callback, which may not be correct and should be considered
a temporary solution waiting for review comments.

The patchset removes entirely the pci_sys_data.domain field, since its
usage is removed at the same time in the respective host controllers
and ARM is made to select the configuration option
CONFIG_PCI_DOMAINS_GENERIC by default, which compiles a domain_nr in the
pci_bus structure so that the pci_domain_nr() look-up can rely on it.

Lorenzo Pieralisi (2):
  arm: cns3xxx: pci: remove artificial dependency on pci_sys_data domain
  arm: pcibios: move to generic PCI domains

 arch/arm/Kconfig                   |  3 +++
 arch/arm/include/asm/mach/pci.h    |  6 ------
 arch/arm/include/asm/pci.h         |  7 -------
 arch/arm/kernel/bios32.c           | 26 ++++++++++++++++++++++---
 arch/arm/mach-cns3xxx/pcie.c       | 40 ++++++++++++++++----------------------
 drivers/pci/host/pci-mvebu.c       | 15 ++------------
 drivers/pci/host/pcie-designware.c |  3 ---
 drivers/pci/host/pcie-rcar.c       |  3 ---
 8 files changed, 45 insertions(+), 58 deletions(-)

-- 
2.1.2

[PATCH v2 1/2] arm: cns3xxx: pci: remove artificial dependency on pci_sys_data domain

[Lorenzo Pieralisi]

On cns3xxx platforms the PCI controller probing code relies on an
artificial dependency on the domain number to look-up the internal
data structures.

This patch reworks the host controller control data structure and
adds a domain equivalent field named port in it so that the dependency on
pci_sys_data domain field can be eventually removed.

Cc: Krzysztof Halasa <khalasa@piap.pl>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
[lp: added commit log, removed pci_sys_data domain references]
Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
---
 arch/arm/mach-cns3xxx/pcie.c | 40 +++++++++++++++++-----------------------
 1 file changed, 17 insertions(+), 23 deletions(-)

diff --git a/arch/arm/mach-cns3xxx/pcie.c b/arch/arm/mach-cns3xxx/pcie.c
index 45d6bd0..f6bf9f6 100644
--- a/arch/arm/mach-cns3xxx/pcie.c
+++ b/arch/arm/mach-cns3xxx/pcie.c
@@ -30,18 +30,15 @@ struct cns3xxx_pcie {
 	unsigned int irqs[2];
 	struct resource res_io;
 	struct resource res_mem;
-	struct hw_pci hw_pci;
-
+	int port;
 	bool linked;
 };
 
-static struct cns3xxx_pcie cns3xxx_pcie[]; /* forward decl. */
-
 static struct cns3xxx_pcie *sysdata_to_cnspci(void *sysdata)
 {
 	struct pci_sys_data *root = sysdata;
 
-	return &cns3xxx_pcie[root->domain];
+	return root->private_data;
 }
 
 static struct cns3xxx_pcie *pdev_to_cnspci(const struct pci_dev *dev)
@@ -192,13 +189,7 @@ static struct cns3xxx_pcie cns3xxx_pcie[] = {
 			.flags = IORESOURCE_MEM,
 		},
 		.irqs = { IRQ_CNS3XXX_PCIE0_RC, IRQ_CNS3XXX_PCIE0_DEVICE, },
-		.hw_pci = {
-			.domain = 0,
-			.nr_controllers = 1,
-			.ops = &cns3xxx_pcie_ops,
-			.setup = cns3xxx_pci_setup,
-			.map_irq = cns3xxx_pcie_map_irq,
-		},
+		.port = 0,
 	},
 	[1] = {
 		.host_regs = (void __iomem *)CNS3XXX_PCIE1_HOST_BASE_VIRT,
@@ -217,19 +208,13 @@ static struct cns3xxx_pcie cns3xxx_pcie[] = {
 			.flags = IORESOURCE_MEM,
 		},
 		.irqs = { IRQ_CNS3XXX_PCIE1_RC, IRQ_CNS3XXX_PCIE1_DEVICE, },
-		.hw_pci = {
-			.domain = 1,
-			.nr_controllers = 1,
-			.ops = &cns3xxx_pcie_ops,
-			.setup = cns3xxx_pci_setup,
-			.map_irq = cns3xxx_pcie_map_irq,
-		},
+		.port = 1,
 	},
 };
 
 static void __init cns3xxx_pcie_check_link(struct cns3xxx_pcie *cnspci)
 {
-	int port = cnspci->hw_pci.domain;
+	int port = cnspci->port;
 	u32 reg;
 	unsigned long time;
 
@@ -260,9 +245,9 @@ static void __init cns3xxx_pcie_check_link(struct cns3xxx_pcie *cnspci)
 
 static void __init cns3xxx_pcie_hw_init(struct cns3xxx_pcie *cnspci)
 {
-	int port = cnspci->hw_pci.domain;
+	int port = cnspci->port;
 	struct pci_sys_data sd = {
-		.domain = port,
+		.private_data = cnspci,
 	};
 	struct pci_bus bus = {
 		.number = 0,
@@ -323,6 +308,14 @@ static int cns3xxx_pcie_abort_handler(unsigned long addr, unsigned int fsr,
 void __init cns3xxx_pcie_init_late(void)
 {
 	int i;
+	void *private_data;
+	struct hw_pci hw_pci = {
+	       .nr_controllers = 1,
+	       .ops = &cns3xxx_pcie_ops,
+	       .setup = cns3xxx_pci_setup,
+	       .map_irq = cns3xxx_pcie_map_irq,
+	       .private_data = &private_data,
+	};
 
 	pcibios_min_io = 0;
 	pcibios_min_mem = 0;
@@ -335,7 +328,8 @@ void __init cns3xxx_pcie_init_late(void)
 		cns3xxx_pwr_soft_rst(0x1 << PM_SOFT_RST_REG_OFFST_PCIE(i));
 		cns3xxx_pcie_check_link(&cns3xxx_pcie[i]);
 		cns3xxx_pcie_hw_init(&cns3xxx_pcie[i]);
-		pci_common_init(&cns3xxx_pcie[i].hw_pci);
+		private_data = &cns3xxx_pcie[i];
+		pci_common_init(&hw_pci);
 	}
 
 	pci_assign_unassigned_resources();
-- 
2.1.2

[PATCH v2 1/2] arm: cns3xxx: pci: remove artificial dependency on pci_sys_data domain

[Krzysztof Hałasa]

Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> writes:

> On cns3xxx platforms the PCI controller probing code relies on an
> artificial dependency on the domain number to look-up the internal
> data structures.
>
> This patch reworks the host controller control data structure and
> adds a domain equivalent field named port in it so that the dependency on
> pci_sys_data domain field can be eventually removed.

Looks sane.
Acked-by: Krzysztof Ha?asa <khalasa@piap.pl>
-- 
Krzysztof Halasa

Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

[PATCH v2 2/2] arm: pcibios: move to generic PCI domains

[Lorenzo Pieralisi]

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>
Tested-by: Lucas Stach <l.stach@pengutronix.de>
Acked-by: Lucas Stach <l.stach@pengutronix.de>
Acked-by: Jingoo Han <jg1.han@samsung.com>
Acked-by: Phil Edworthy <phil.edworthy@renesas.com>
Acked-by: Arnd Bergmann <arnd@arndb.de>
Reviewed-by: Yijing Wang <wangyijing@huawei.com>
---
 arch/arm/Kconfig                   |  3 +++
 arch/arm/include/asm/mach/pci.h    |  6 ------
 arch/arm/include/asm/pci.h         |  7 -------
 arch/arm/kernel/bios32.c           | 26 +++++++++++++++++++++++---
 drivers/pci/host/pci-mvebu.c       | 15 ++-------------
 drivers/pci/host/pcie-designware.c |  3 ---
 drivers/pci/host/pcie-rcar.c       |  3 ---
 7 files changed, 28 insertions(+), 35 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..112b770 100644
--- a/arch/arm/include/asm/mach/pci.h
+++ b/arch/arm/include/asm/mach/pci.h
@@ -19,9 +19,6 @@ struct pci_bus;
 struct device;
 
 struct hw_pci {
-#ifdef CONFIG_PCI_DOMAINS
-	int		domain;
-#endif
 	struct pci_ops	*ops;
 	int		nr_controllers;
 	void		**private_data;
@@ -44,9 +41,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..dc2ed4d 100644
--- a/drivers/pci/host/pci-mvebu.c
+++ b/drivers/pci/host/pci-mvebu.c
@@ -101,9 +101,7 @@ struct mvebu_pcie {
 	struct mvebu_pcie_port *ports;
 	struct msi_chip *msi;
 	struct resource io;
-	char io_name[30];
 	struct resource realio;
-	char mem_name[30];
 	struct resource mem;
 	struct resource busn;
 	int nports;
@@ -722,18 +720,9 @@ 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);
-	pcie->mem.name = pcie->mem_name;
-
-	snprintf(pcie->io_name, sizeof(pcie->io_name), "PCI I/O %04x", domain);
-	pcie->realio.name = pcie->io_name;
+	pcie->mem.name = "PCI MEM";
+	pcie->realio.name = "PCI I/O";
 
 	if (request_resource(&iomem_resource, &pcie->mem))
 		return 0;
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

[PATCH v2 2/2] arm: pcibios: move to generic PCI domains

[Rob Herring]

On Thu, Nov 6, 2014 at 9:32 AM, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> 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.

I realize you are just copying the arm64 version, but as we've agreed
the DT domain handling should be common and what the error cases are,
I've got some comments on the current implementation.

[...]

> +#ifdef CONFIG_PCI_DOMAINS_GENERIC
> +static bool dt_domain_found;

This can be moved into the function.

> +
> +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;

No error other than a printk? Do we want to set domain_nr to an
illegal value or something?

> +       } else {
> +               domain = pci_get_new_domain_nr();
> +       }
> +
> +       bus->domain_nr = domain;
> +}

This doesn't handle the case where the 1st node(s) probed does not
have a domain prop and a later one does. I think something like this
should work:

static int use_dt_domains = -1;
int domain = of_get_pci_domain_nr(parent->of_node);

if (domain >= 0 && use_dt_domains) {
        use_dt_domains = 1;
} else if (domain < 0 && use_dt_domain != 1) {
        use_dt_domains = 0;
        domain = pci_get_new_domain_nr();
} else {
        dev_err(parent, "Node %s has inconsistent \"linux,pci-domain\"
property in DT\n",
                 parent->of_node->full_name);
        domain = -1;  // Not sure if -1 would be illegal or not????
}

bus->domain_nr = domain;


Rob

[PATCH v2 2/2] arm: pcibios: move to generic PCI domains

[Lorenzo Pieralisi]

On Fri, Nov 07, 2014 at 04:07:30PM +0000, Rob Herring wrote:
> On Thu, Nov 6, 2014 at 9:32 AM, Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com> 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.
> 
> I realize you are just copying the arm64 version, but as we've agreed
> the DT domain handling should be common and what the error cases are,
> I've got some comments on the current implementation.

So this means that either I patch arm64 code with your implementation
below and I duplicate the code for arm too, or I have to factor out an
implementation based on your code below and add it somewhere in common
kernel code (drivers/pci/pci.c ? drivers/pci/of.c ?), where, it has to be
seen.

> 
> [...]
> 
> > +#ifdef CONFIG_PCI_DOMAINS_GENERIC
> > +static bool dt_domain_found;
> 
> This can be moved into the function.

Yes, I noticed while putting the patch together, I was more concerned
about the removal of pci_sys_data.domain than the code parsing the domain
value, which I considered agreed upon.

> > +
> > +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;
> 
> No error other than a printk? Do we want to set domain_nr to an
> illegal value or something?

See above.

> > +       } else {
> > +               domain = pci_get_new_domain_nr();
> > +       }
> > +
> > +       bus->domain_nr = domain;
> > +}
> 
> This doesn't handle the case where the 1st node(s) probed does not
> have a domain prop and a later one does. I think something like this
> should work:

Well, a boolean won't do, your code below looks ok, I can add a trivial enum
to make use_dt_domains usage clearer (but I think a comment to your code will
do):

enum {
	PCI_DOMAIN_INVALID,
	PCI_DOMAIN_DT,
	PCI_DOMAIN_DEFAULT
};

> static int use_dt_domains = -1;
> int domain = of_get_pci_domain_nr(parent->of_node);
> 
> if (domain >= 0 && use_dt_domains) {
>         use_dt_domains = 1;
> } else if (domain < 0 && use_dt_domain != 1) {
>         use_dt_domains = 0;
>         domain = pci_get_new_domain_nr();
> } else {
>         dev_err(parent, "Node %s has inconsistent \"linux,pci-domain\"
> property in DT\n",
>                  parent->of_node->full_name);
>         domain = -1;  // Not sure if -1 would be illegal or not????

I have to check, that's a valid point, we can't certainly leave it as it is.

Thanks,
Lorenzo