[RFC 1/4] arm64: amd-seattle: Adding device tree for AMD Seattle platform

From: Suravee Suthikulpanit <***@amd.com>

This patch adds ARM64 support to the generic PCI host driver.

For MSI support, it adds new device tree binding "msi-parent",
which should point to corresponded msi-controller.

Cc: Will Deacon <***@arm.com>
Cc: Liviu Dudau <***@arm.com>
Cc: Bjorn Helgaas <***@google.com>
Cc: Mark Rutland <***@arm.com>
Cc: Catalin Marinas <***@arm.com>
Signed-off-by: Suravee Suthikulpanit <***@amd.com>
---
.../devicetree/bindings/pci/host-generic-pci.txt | 3 +
drivers/pci/host/Kconfig | 2 +-
drivers/pci/host/pci-host-generic.c | 95 ++++++++++++++++++++--
3 files changed, 90 insertions(+), 10 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/host-generic-pci.txt b/Documentation/devicetree/bindings/pci/host-generic-pci.txt
index f0b0436..327e5b1 100644
--- a/Documentation/devicetree/bindings/pci/host-generic-pci.txt
+++ b/Documentation/devicetree/bindings/pci/host-generic-pci.txt
@@ -69,6 +69,9 @@ Practice: Interrupt Mapping' and requires the following properties:

- interrupt-map-mask : <see aforementioned specification>

+Optinal Properties:
+
+- msi-parent : Specify the msi-controller phandle.

Example:

diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
index 90f5cca..44bf523 100644
--- a/drivers/pci/host/Kconfig
+++ b/drivers/pci/host/Kconfig
@@ -50,7 +50,7 @@ config PCI_RCAR_GEN2_PCIE

config PCI_HOST_GENERIC
bool "Generic PCI host controller"
- depends on ARM && OF
+ depends on (ARM || ARM64) && OF
help
Say Y here if you want to support a simple generic PCI host
controller, such as the one emulated by kvmtool.
diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
index 3d2076f..f33c547 100644
--- a/drivers/pci/host/pci-host-generic.c
+++ b/drivers/pci/host/pci-host-generic.c
@@ -42,14 +42,24 @@ struct gen_pci {
struct pci_host_bridge host;
struct gen_pci_cfg_windows cfg;
struct list_head resources;
+ struct device_node *msi_parent;
};

+#ifdef CONFIG_ARM64
+#define bus_to_gen_pci(b) \
+ ((struct gen_pci *)b->sysdata)
+#else
+#define bus_to_gen_pci(b) \
+ ((struct gen_pci *) \
+ (((struct pci_sys_data *) \
+ (bus->sysdata))->private_data))
+#endif
+
static void __iomem *gen_pci_map_cfg_bus_cam(struct pci_bus *bus,
unsigned int devfn,
int where)
{
- struct pci_sys_data *sys = bus->sysdata;
- struct gen_pci *pci = sys->private_data;
+ struct gen_pci *pci = bus_to_gen_pci(bus);
resource_size_t idx = bus->number - pci->cfg.bus_range.start;

return pci->cfg.win[idx] + ((devfn << 8) | where);
@@ -64,8 +74,7 @@ static void __iomem *gen_pci_map_cfg_bus_ecam(struct pci_bus *bus,
unsigned int devfn,
int where)
{
- struct pci_sys_data *sys = bus->sysdata;
- struct gen_pci *pci = sys->private_data;
+ struct gen_pci *pci = bus_to_gen_pci(bus);
resource_size_t idx = bus->number - pci->cfg.bus_range.start;

return pci->cfg.win[idx] + ((devfn << 12) | where);
@@ -80,8 +89,7 @@ static int gen_pci_config_read(struct pci_bus *bus, unsigned int devfn,
int where, int size, u32 *val)
{
void __iomem *addr;
- struct pci_sys_data *sys = bus->sysdata;
- struct gen_pci *pci = sys->private_data;
+ struct gen_pci *pci = bus_to_gen_pci(bus);

addr = pci->cfg.ops->map_bus(bus, devfn, where);

@@ -103,8 +111,7 @@ static int gen_pci_config_write(struct pci_bus *bus, unsigned int devfn,
int where, int size, u32 val)
{
void __iomem *addr;
- struct pci_sys_data *sys = bus->sysdata;
- struct gen_pci *pci = sys->private_data;
+ struct gen_pci *pci = bus_to_gen_pci(bus);

addr = pci->cfg.ops->map_bus(bus, devfn, where);

@@ -144,8 +151,11 @@ static int gen_pci_calc_io_offset(struct device *dev,
resource_size_t *offset)
{
static atomic_t wins = ATOMIC_INIT(0);
- int err, idx, max_win;
+ int idx, max_win;
unsigned int window;
+#ifndef CONFIG_ARM64
+ int err;
+#endif

if (!PAGE_ALIGNED(range->cpu_addr))
return -EINVAL;
@@ -156,9 +166,12 @@ static int gen_pci_calc_io_offset(struct device *dev,
return -ENOSPC;

window = (idx - 1) * SZ_64K;
+
+#ifndef CONFIG_ARM64
err = pci_ioremap_io(window, range->cpu_addr);
if (err)
return err;
+#endif

of_pci_range_to_resource(range, dev->of_node, res);
res->start = window;
@@ -310,12 +323,58 @@ static int gen_pci_parse_map_cfg_windows(struct gen_pci *pci)
return 0;
}

+#ifndef CONFIG_ARM64
static int gen_pci_setup(int nr, struct pci_sys_data *sys)
{
struct gen_pci *pci = sys->private_data;
list_splice_init(&pci->resources, &sys->resources);
return 1;
}
+#endif
+
+#ifdef CONFIG_ARM64
+struct pci_bus *gen_scan_root_bus(struct device *parent, int bus,
+ struct pci_ops *ops, void *sysdata,
+ struct list_head *resources)
+{
+ struct pci_host_bridge_window *window;
+ bool found = false;
+ struct pci_bus *b;
+ int max;
+ struct gen_pci *pci = sysdata;
+
+ list_for_each_entry(window, resources, list)
+ if (window->res->flags & IORESOURCE_BUS) {
+ found = true;
+ break;
+ }
+
+ b = pci_create_root_bus(parent, bus, ops, sysdata, resources);
+ if (!b)
+ return NULL;
+
+ /* TODO:
+ * This is probably should be done in the core pci driver somewhere
+ */
+ if (pci->msi_parent)
+ b->msi = of_pci_find_msi_chip_by_node(pci->msi_parent);
+
+ if (!found) {
+ dev_info(&b->dev,
+ "No busn resource found for root bus, will use [bus %02x-ff]\n",
+ bus);
+ pci_bus_insert_busn_res(b, bus, 255);
+ }
+
+ max = pci_scan_child_bus(b);
+
+ if (!found)
+ pci_bus_update_busn_res_end(b, max);
+
+ pci_bus_add_devices(b);
+ return b;
+}
+#endif

static int gen_pci_probe(struct platform_device *pdev)
{
@@ -326,6 +385,7 @@ static int gen_pci_probe(struct platform_device *pdev)
struct device *dev = &pdev->dev;
struct device_node *np = dev->of_node;
struct gen_pci *pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);
+#ifndef CONFIG_ARM64
struct hw_pci hw = {
.nr_controllers = 1,
.private_data = (void **)&pci,
@@ -333,6 +393,7 @@ static int gen_pci_probe(struct platform_device *pdev)
.map_irq = of_irq_parse_and_map_pci,
.ops = &gen_pci_ops,
};
+#endif

if (!pci)
return -ENOMEM;
@@ -368,8 +429,24 @@ static int gen_pci_probe(struct platform_device *pdev)
gen_pci_release_of_pci_ranges(pci);
return err;
}
+#ifdef CONFIG_ARM64

+#ifdef CONFIG_PCI_MSI
+ pci->msi_parent = of_parse_phandle(np, "msi-parent", 0);
+ if (!pci->msi_parent) {
+ dev_err(&pdev->dev, "Failed to allocate msi-parent.\n");
+ return -EINVAL;
+ }
+#endif
+
+ if (!gen_scan_root_bus(&pdev->dev, pci->cfg.bus_range.start,
+ &gen_pci_ops, pci, &pci->resources)) {
+ dev_err(&pdev->dev, "failed to enable PCIe ports\n");
+ return -ENODEV;
+ }
+#else
pci_common_init_dev(dev, &hw);
+#endif /* CONFIG_ARM64 */
return 0;
}

--
1.9.3

Arnd Bergmann

2014-09-29 14:36:30 UTC

Post by s***@amd.com
+
+#ifdef CONFIG_ARM64
+struct pci_bus *gen_scan_root_bus(struct device *parent, int bus,
+ struct pci_ops *ops, void *sysdata,
+ struct list_head *resources)
+{

I don't see anything ARM64 specific in this, the function only uses
the newly added generic helpers.

Post by s***@amd.com
static int gen_pci_probe(struct platform_device *pdev)
{
@@ -326,6 +385,7 @@ static int gen_pci_probe(struct platform_device *pdev)
struct device *dev = &pdev->dev;
struct device_node *np = dev->of_node;
struct gen_pci *pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);
+#ifndef CONFIG_ARM64
struct hw_pci hw = {
.nr_controllers = 1,
.private_data = (void **)&pci,
@@ -333,6 +393,7 @@ static int gen_pci_probe(struct platform_device *pdev)
.map_irq = of_irq_parse_and_map_pci,
.ops = &gen_pci_ops,
};
+#endif

Same here, I'd suggest marking this "#ifdef CONFIG_ARM" instead, as hw_pci
is an arm32 specific data structure.

Post by s***@amd.com
if (!pci)
return -ENOMEM;
@@ -368,8 +429,24 @@ static int gen_pci_probe(struct platform_device *pdev)
gen_pci_release_of_pci_ranges(pci);
return err;
}
+#ifdef CONFIG_ARM64
+#ifdef CONFIG_PCI_MSI
+ pci->msi_parent = of_parse_phandle(np, "msi-parent", 0);
+ if (!pci->msi_parent) {
+ dev_err(&pdev->dev, "Failed to allocate msi-parent.\n");
+ return -EINVAL;
+ }
+#endif

We probably want to enable MSI unconditionally on ARM64, so the #ifdef is
not necessary here. However, I don't think that a missing msi-parent should
be a fatal error here: we can still continue without MSI support if this
case.

Post by s***@amd.com
+ if (!gen_scan_root_bus(&pdev->dev, pci->cfg.bus_range.start,
+ &gen_pci_ops, pci, &pci->resources)) {
+ dev_err(&pdev->dev, "failed to enable PCIe ports\n");
+ return -ENODEV;
+ }
+#else
pci_common_init_dev(dev, &hw);
+#endif /* CONFIG_ARM64 */

Again, just make the pci_common_init_dev() call #ifdef CONFIG_ARM, and move
the generic case after it, outside of the #ifdef.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Lorenzo Pieralisi

2014-09-30 12:03:44 UTC

Post by s***@amd.com
+
+#ifdef CONFIG_ARM64
+struct pci_bus *gen_scan_root_bus(struct device *parent, int bus,
+ struct pci_ops *ops, void *sysdata,
+ struct list_head *resources)
+{

I don't see anything ARM64 specific in this, the function only uses
the newly added generic helpers.

I do not see it either, it is just a copy'n'paste of common PCI code,
plus msi management.

Same here, I'd suggest marking this "#ifdef CONFIG_ARM" instead, as hw_pci
is an arm32 specific data structure.

I do not think we need hw struct at all, see below, we can write code so
that we do not rely on ARM32 PCI bios, I will have a stab at that and
post the resulting code.

Again, just make the pci_common_init_dev() call #ifdef CONFIG_ARM, and move
the generic case after it, outside of the #ifdef.

I went through the code quickly but I think we can (and should) remove
this quite ugly ifdeffery altogether. Most of the functionality in
pci_common_init_dev() can be implemented through the common PCI API (and this
would make this driver arch agnostic as it should be), I will go through ARM32
PCI bios code to check what is executed in detail in pci_common_init_dev() and
make sure that we follow those initialization steps in the resulting probe code
for this PCI generic host controller driver.

Lorenzo

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

Arnd Bergmann

2014-09-30 12:31:44 UTC

Same here, I'd suggest marking this "#ifdef CONFIG_ARM" instead, as hw_pci
is an arm32 specific data structure.

I do not think we need hw struct at all, see below, we can write code so
that we do not rely on ARM32 PCI bios, I will have a stab at that and
post the resulting code.

That would of course be best. I think it needs some rework of the
arm32 PCI code though, or you'd still have to create pci_sys_data
manually, and that is currently allocated by pcibios_init_hw.

Again, just make the pci_common_init_dev() call #ifdef CONFIG_ARM, and move
the generic case after it, outside of the #ifdef.

These are the functions I found that refer to pci_sys_data on arm32:

pcibios_add_bus
pcibios_remove_bus
pcibios_align_resource
pci_mmap_page_range
pci_domain_nr
pci_proc_domain

This is not as bad as I had feared, but we still have to ensure that
any caller of these functions will work with both the generic PCI support
and the arm32 specific drivers that today use hw_pci.

My idea for dealing with this was to convert all host drivers in
drivers/pci/host to the generic PCI code and never build the arm32
bios32 code when CONFIG_ARCH_MULTIPLATFORM is set. Unfortunately that
requires either doing them all at once or coming up with a migration
strategy so we don't break things in the process.

Note that arch/arm/mach-cns3xxx/pcie.c also belongs in the drivers/pci/host
category and should probably be moved there. The integrator and versatile
PCI drivers are currently not used with MULTIPLATFORM but will at some point
and we can convert them as we get there.

Arnd

Lorenzo Pieralisi

2014-09-30 16:12:41 UTC

Same here, I'd suggest marking this "#ifdef CONFIG_ARM" instead, as hw_pci
is an arm32 specific data structure.

I do not think we need hw struct at all, see below, we can write code so
that we do not rely on ARM32 PCI bios, I will have a stab at that and
post the resulting code.

That would of course be best. I think it needs some rework of the
arm32 PCI code though, or you'd still have to create pci_sys_data
manually, and that is currently allocated by pcibios_init_hw.

Right, as far as I can see, creating a pci_sys_data struct
that's all we would need. "Problem" is that it does not exist on ARM64
so to avoid ifdeffery we have to declare a struct with the same
fields (ie only pci_sys_data.private_data is used by this driver -
apart from arm32 specific functions usage) that is passed to the PCI layer
and stored in the bus.sysdata, but that's extremely ugly (and we won't
need this when the arm32 conversion is completed).

Again, just make the pci_common_init_dev() call #ifdef CONFIG_ARM, and move
the generic case after it, outside of the #ifdef.

pcibios_add_bus
pcibios_remove_bus
pcibios_align_resource
pci_mmap_page_range
pci_domain_nr
pci_proc_domain
This is not as bad as I had feared, but we still have to ensure that
any caller of these functions will work with both the generic PCI support
and the arm32 specific drivers that today use hw_pci.
My idea for dealing with this was to convert all host drivers in
drivers/pci/host to the generic PCI code and never build the arm32
bios32 code when CONFIG_ARCH_MULTIPLATFORM is set. Unfortunately that
requires either doing them all at once or coming up with a migration
strategy so we don't break things in the process.

That makes sense. Related to the migration strategy, thoughts
appreciated. Declaring a static pci_sys_data (with some ifdef around it)
seems a horrible hack to me. Calling pci_common_init() only if CONFIG_ARM
is rather horrible too, but we can probably live with that.

I do not see anything else as possible solution at the moment unless
we go the whole nine yards and do what you suggest above, might take a
little while though.

Probably leaving pci_common_init() call (and related hw_pci struct, and
related ifdeffery to differentiate between different sysdata layouts for ARM
and ARM64) is the fastest path but I still think it is not nice at all.

Thanks,
Lorenzo

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

Liviu Dudau

2014-09-30 16:42:56 UTC

Post by s***@amd.com
static int gen_pci_probe(struct platform_device *pdev)
{
@@ -326,6 +385,7 @@ static int gen_pci_probe(struct platform_=

device *pdev)

Post by s***@amd.com
struct device *dev =3D &pdev->dev;
struct device_node *np =3D dev->of_node;
struct gen_pci *pci =3D devm_kzalloc(dev, sizeof(*pci=

), GFP_KERNEL);

Post by s***@amd.com
+#ifndef CONFIG_ARM64
struct hw_pci hw =3D {
.nr_controllers =3D 1,
.private_data =3D (void **)&pci,
@@ -333,6 +393,7 @@ static int gen_pci_probe(struct platform_=

device *pdev)

Post by s***@amd.com
.map_irq =3D of_irq_parse_and_map_pci,
.ops =3D &gen_pci_ops,
};
+#endif
=20

=20
Same here, I'd suggest marking this "#ifdef CONFIG_ARM" instead=

, as hw_pci

is an arm32 specific data structure.

=20
I do not think we need hw struct at all, see below, we can write =

code so

that we do not rely on ARM32 PCI bios, I will have a stab at that=

and

post the resulting code.

=20
That would of course be best. I think it needs some rework of the
arm32 PCI code though, or you'd still have to create pci_sys_data
manually, and that is currently allocated by pcibios_init_hw.

I don't see why we need to involve the arm32 code here at all. A host b=
ridge can
be fully functional with the generic code without having to use any of =
the
arm32 code (unless I'm missing something here).

Post by Lorenzo Pieralisi
=20
Right, as far as I can see, creating a pci_sys_data struct
that's all we would need. "Problem" is that it does not exist on ARM6=

Post by Lorenzo Pieralisi
so to avoid ifdeffery we have to declare a struct with the same
fields (ie only pci_sys_data.private_data is used by this driver -
apart from arm32 specific functions usage) that is passed to the PCI =

layer

Post by Lorenzo Pieralisi
and stored in the bus.sysdata, but that's extremely ugly (and we won'=

Post by Lorenzo Pieralisi
need this when the arm32 conversion is completed).
=20

Post by s***@amd.com
+ if (!gen_scan_root_bus(&pdev->dev, pci->cfg.bus_range=

=2Estart,

Post by s***@amd.com
+ &gen_pci_ops, pci, &pci->resou=

rces)) {

Post by s***@amd.com
+ dev_err(&pdev->dev, "failed to enable PCIe po=

rts\n");

Post by s***@amd.com
+ return -ENODEV;
+ }
+#else
pci_common_init_dev(dev, &hw);
+#endif /* CONFIG_ARM64 */
=20

=20
Again, just make the pci_common_init_dev() call #ifdef CONFIG_A=

RM, and move

the generic case after it, outside of the #ifdef.

=20
I went through the code quickly but I think we can (and should) r=

emove

this quite ugly ifdeffery altogether. Most of the functionality i=

pci_common_init_dev() can be implemented through the common PCI A=

PI (and this

would make this driver arch agnostic as it should be), I will go =

through ARM32

PCI bios code to check what is executed in detail in pci_common_i=

nit_dev() and

make sure that we follow those initialization steps in the result=

ing probe code

for this PCI generic host controller driver.

=20
These are the functions I found that refer to pci_sys_data on arm32=
=20
pcibios_add_bus
pcibios_remove_bus
pcibios_align_resource
pci_mmap_page_range
pci_domain_nr
pci_proc_domain
=20
This is not as bad as I had feared, but we still have to ensure tha=

Post by Arnd Bergmann
any caller of these functions will work with both the generic PCI s=

upport

Post by Arnd Bergmann
and the arm32 specific drivers that today use hw_pci.
=20
My idea for dealing with this was to convert all host drivers in
drivers/pci/host to the generic PCI code and never build the arm32
bios32 code when CONFIG_ARCH_MULTIPLATFORM is set. Unfortunately th=

Post by Arnd Bergmann
requires either doing them all at once or coming up with a migratio=

Post by Arnd Bergmann
strategy so we don't break things in the process.

=20
That makes sense. Related to the migration strategy, thoughts
appreciated. Declaring a static pci_sys_data (with some ifdef around =

it)

Post by Lorenzo Pieralisi
seems a horrible hack to me. Calling pci_common_init() only if CONFIG=

_ARM

Post by Lorenzo Pieralisi
is rather horrible too, but we can probably live with that.
=20
I do not see anything else as possible solution at the moment unless
we go the whole nine yards and do what you suggest above, might take =

Post by Lorenzo Pieralisi
little while though.
=20
Probably leaving pci_common_init() call (and related hw_pci struct, a=

Post by Lorenzo Pieralisi
related ifdeffery to differentiate between different sysdata layouts =

for ARM

Post by Lorenzo Pieralisi
and ARM64) is the fastest path but I still think it is not nice at al=

l.

Rob Herring found the conversion of mach-integrator/pci_v3.c to the gen=
eric
framework quite painless. We might have to go through a lot of testing,=
but I don't
see the process to be too horrendous.

That being said, I think we first need to make sure we have all the fea=
tures
needed by the host bridge drivers in place before we start the conversi=
on. MSI
support is amongst them.

Best regards,
Liviu

Post by Lorenzo Pieralisi
=20
Thanks,
Lorenzo

--=20
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
=C2=AF\_(=E3=83=84)_/=C2=AF

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

Lorenzo Pieralisi

2014-09-30 17:35:40 UTC

Same here, I'd suggest marking this "#ifdef CONFIG_ARM" instead, as hw_pci
is an arm32 specific data structure.

I do not think we need hw struct at all, see below, we can write code so
that we do not rely on ARM32 PCI bios, I will have a stab at that and
post the resulting code.

That would of course be best. I think it needs some rework of the
arm32 PCI code though, or you'd still have to create pci_sys_data
manually, and that is currently allocated by pcibios_init_hw.

I don't see why we need to involve the arm32 code here at all. A host bridge can
be fully functional with the generic code without having to use any of the
arm32 code (unless I'm missing something here).

Ok so I can remove the pci_common_init() call, use the common PCI API and
everything will work as expected, even if there is a list of functions (see
below) that *require* pci_sys_data to exist (and that's allocated in arm32
pcibios code in pcibios_init_hw(), called from pci_common_init()) ?

I like the idea but I think that's optimistic, or at least we did not
trigger the code paths that can cause issues.

Post by Lorenzo Pieralisi
Right, as far as I can see, creating a pci_sys_data struct
that's all we would need. "Problem" is that it does not exist on ARM64
so to avoid ifdeffery we have to declare a struct with the same
fields (ie only pci_sys_data.private_data is used by this driver -
apart from arm32 specific functions usage) that is passed to the PCI layer
and stored in the bus.sysdata, but that's extremely ugly (and we won't
need this when the arm32 conversion is completed).

Again, just make the pci_common_init_dev() call #ifdef CONFIG_ARM, and move
the generic case after it, outside of the #ifdef.

That makes sense. Related to the migration strategy, thoughts
appreciated. Declaring a static pci_sys_data (with some ifdef around it)
seems a horrible hack to me. Calling pci_common_init() only if CONFIG_ARM
is rather horrible too, but we can probably live with that.
I do not see anything else as possible solution at the moment unless
we go the whole nine yards and do what you suggest above, might take a
little while though.
Probably leaving pci_common_init() call (and related hw_pci struct, and
related ifdeffery to differentiate between different sysdata layouts for ARM
and ARM64) is the fastest path but I still think it is not nice at all.

Rob Herring found the conversion of mach-integrator/pci_v3.c to the generic
framework quite painless. We might have to go through a lot of testing, but I don't
see the process to be too horrendous.

See my comments above, I have not said that the conversion is complicated,
what I am saying is that I am not sure we can get rid of pcibios code calls
yet, as Arnd pointed out.

Lorenzo

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

Liviu Dudau

2014-09-30 17:48:21 UTC

Post by s***@amd.com
static int gen_pci_probe(struct platform_device *pdev)
{
@@ -326,6 +385,7 @@ static int gen_pci_probe(struct platf=

orm_device *pdev)

Post by s***@amd.com
struct device *dev =3D &pdev->dev;
struct device_node *np =3D dev->of_node;
struct gen_pci *pci =3D devm_kzalloc(dev, sizeof(=

*pci), GFP_KERNEL);

Post by s***@amd.com
+#ifndef CONFIG_ARM64
struct hw_pci hw =3D {
.nr_controllers =3D 1,
.private_data =3D (void **)&pci,
@@ -333,6 +393,7 @@ static int gen_pci_probe(struct platf=

orm_device *pdev)

Post by s***@amd.com
.map_irq =3D of_irq_parse_and_map_=

pci,

Post by s***@amd.com
.ops =3D &gen_pci_ops,
};
+#endif
=20

=20
Same here, I'd suggest marking this "#ifdef CONFIG_ARM" ins=

tead, as hw_pci

is an arm32 specific data structure.

=20
I do not think we need hw struct at all, see below, we can wr=

ite code so

that we do not rely on ARM32 PCI bios, I will have a stab at =

that and

post the resulting code.

=20
That would of course be best. I think it needs some rework of t=

arm32 PCI code though, or you'd still have to create pci_sys_da=

manually, and that is currently allocated by pcibios_init_hw.

=20
I don't see why we need to involve the arm32 code here at all. A ho=

st bridge can

be fully functional with the generic code without having to use any=

of the

arm32 code (unless I'm missing something here).

=20
Ok so I can remove the pci_common_init() call, use the common PCI API=

and

everything will work as expected, even if there is a list of function=

s (see

below) that *require* pci_sys_data to exist (and that's allocated in =

arm32

pcibios code in pcibios_init_hw(), called from pci_common_init()) ?

See bellow my comments on those functions.

=20
I like the idea but I think that's optimistic, or at least we did not
trigger the code paths that can cause issues.
=20

Post by Lorenzo Pieralisi
Right, as far as I can see, creating a pci_sys_data struct
that's all we would need. "Problem" is that it does not exist on =

ARM64

Post by Lorenzo Pieralisi
so to avoid ifdeffery we have to declare a struct with the same
fields (ie only pci_sys_data.private_data is used by this driver =

Post by Lorenzo Pieralisi
apart from arm32 specific functions usage) that is passed to the =

PCI layer

Post by Lorenzo Pieralisi
and stored in the bus.sysdata, but that's extremely ugly (and we =

won't

Post by Lorenzo Pieralisi
need this when the arm32 conversion is completed).
=20

Post by s***@amd.com
+ if (!gen_scan_root_bus(&pdev->dev, pci->cfg.bus_r=

ange.start,

Post by s***@amd.com
+ &gen_pci_ops, pci, &pci->r=

esources)) {

Post by s***@amd.com
+ dev_err(&pdev->dev, "failed to enable PCI=

e ports\n");

Post by s***@amd.com
+ return -ENODEV;
+ }
+#else
pci_common_init_dev(dev, &hw);
+#endif /* CONFIG_ARM64 */
=20

=20
Again, just make the pci_common_init_dev() call #ifdef CONF=

IG_ARM, and move

the generic case after it, outside of the #ifdef.

=20
I went through the code quickly but I think we can (and shoul=

d) remove

this quite ugly ifdeffery altogether. Most of the functionali=

ty in

pci_common_init_dev() can be implemented through the common P=

CI API (and this

would make this driver arch agnostic as it should be), I will=

go through ARM32

PCI bios code to check what is executed in detail in pci_comm=

on_init_dev() and

make sure that we follow those initialization steps in the re=

sulting probe code

for this PCI generic host controller driver.

=20
These are the functions I found that refer to pci_sys_data on a=
=20
pcibios_add_bus
pcibios_remove_bus

These are only needed if you want to do per HB processing of the bus

pcibios_align_resource

mvebu is the only user of this function.

pci_mmap_page_range

This is only needed when mapping a PCI resource to userspace. Is that y=
our case here?

pci_domain_nr
pci_proc_domain

We have equivalent functionality in the generic patches for those.

Best regards,
Liviu

=20
This is not as bad as I had feared, but we still have to ensure=

that

any caller of these functions will work with both the generic P=

CI support

and the arm32 specific drivers that today use hw_pci.
=20
My idea for dealing with this was to convert all host drivers i=

drivers/pci/host to the generic PCI code and never build the ar=

m32

bios32 code when CONFIG_ARCH_MULTIPLATFORM is set. Unfortunatel=

y that

requires either doing them all at once or coming up with a migr=

ation

strategy so we don't break things in the process.

=20
That makes sense. Related to the migration strategy, thoughts
appreciated. Declaring a static pci_sys_data (with some ifdef aro=

und it)

Post by Lorenzo Pieralisi
seems a horrible hack to me. Calling pci_common_init() only if CO=

NFIG_ARM

Post by Lorenzo Pieralisi
is rather horrible too, but we can probably live with that.
=20
I do not see anything else as possible solution at the moment unl=

ess

Post by Lorenzo Pieralisi
we go the whole nine yards and do what you suggest above, might t=

ake a

Post by Lorenzo Pieralisi
little while though.
=20
Probably leaving pci_common_init() call (and related hw_pci struc=

t, and

Post by Lorenzo Pieralisi
related ifdeffery to differentiate between different sysdata layo=

uts for ARM

Post by Lorenzo Pieralisi
and ARM64) is the fastest path but I still think it is not nice a=

t all.

=20
Rob Herring found the conversion of mach-integrator/pci_v3.c to the=

generic

framework quite painless. We might have to go through a lot of test=

ing, but I don't

see the process to be too horrendous.

=20
See my comments above, I have not said that the conversion is complic=

ated,

what I am saying is that I am not sure we can get rid of pcibios code=

calls

yet, as Arnd pointed out.
=20
Lorenzo

Arnd Bergmann

2014-09-30 18:54:41 UTC

Post by Arnd Bergmann
pcibios_add_bus
pcibios_remove_bus

These are only needed if you want to do per HB processing of the bus

Post by Arnd Bergmann
pcibios_align_resource

mvebu is the only user of this function.

Post by Arnd Bergmann
pci_mmap_page_range

This is only needed when mapping a PCI resource to userspace. Is that your case here?

Post by Arnd Bergmann
pci_domain_nr
pci_proc_domain

We have equivalent functionality in the generic patches for those.

We clearly don't need those functions for the new drivers, but that's not
the point. The problem is that when you build a kernel that has both
a traditional host bridge driver and a new one in it, you always get those
functions and they get called from the PCI core, with incorrect arguments.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Arnd Bergmann

2014-09-30 20:01:14 UTC

Post by Arnd Bergmann
pcibios_add_bus
pcibios_remove_bus

These are only needed if you want to do per HB processing of the bus

Post by Arnd Bergmann
pcibios_align_resource

mvebu is the only user of this function.

Post by Arnd Bergmann
pci_mmap_page_range

This is only needed when mapping a PCI resource to userspace. Is that your case here?

Post by Arnd Bergmann
pci_domain_nr
pci_proc_domain

We have equivalent functionality in the generic patches for those.

FWIW, the last time we discussed this, I think I had suggested that the
functions that are currently architecture specific and have a generic
__weak fallback could become function pointers in a per-host structure
passed to pci_scan_root_bus, either a new structure or an extended
struct pci_ops. Something along these lines:

diff --git a/arch/arm/include/asm/mach/pci.h b/arch/arm/include/asm/mach/pci.h
index 7fc42784becb..3da32fc631d0 100644
--- a/arch/arm/include/asm/mach/pci.h
+++ b/arch/arm/include/asm/mach/pci.h
@@ -36,7 +36,6 @@ struct hw_pci {
resource_size_t start,
resource_size_t size,
resource_size_t align);
- void (*add_bus)(struct pci_bus *bus);
void (*remove_bus)(struct pci_bus *bus);
};

@@ -65,7 +64,6 @@ struct pci_sys_data {
resource_size_t start,
resource_size_t size,
resource_size_t align);
- void (*add_bus)(struct pci_bus *bus);
void (*remove_bus)(struct pci_bus *bus);
void *private_data; /* platform controller private data */
};
diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
index 17a26c17f7f5..3cbcf8dc41e4 100644
--- a/arch/arm/kernel/bios32.c
+++ b/arch/arm/kernel/bios32.c
@@ -360,13 +360,6 @@ void pcibios_fixup_bus(struct pci_bus *bus)
}
EXPORT_SYMBOL(pcibios_fixup_bus);

-void pcibios_add_bus(struct pci_bus *bus)
-{
- struct pci_sys_data *sys = bus->sysdata;
- if (sys->add_bus)
- sys->add_bus(bus);
-}
-
void pcibios_remove_bus(struct pci_bus *bus)
{
struct pci_sys_data *sys = bus->sysdata;
@@ -475,7 +468,6 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw,
sys->swizzle = hw->swizzle;
sys->map_irq = hw->map_irq;
sys->align_resource = hw->align_resource;
- sys->add_bus = hw->add_bus;
sys->remove_bus = hw->remove_bus;
INIT_LIST_HEAD(&sys->resources);

diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
index b1315e197ffb..c9a0ee0429e8 100644
--- a/drivers/pci/host/pci-mvebu.c
+++ b/drivers/pci/host/pci-mvebu.c
@@ -716,6 +716,7 @@ static int mvebu_pcie_rd_conf(struct pci_bus *bus, u32 devfn, int where,
static struct pci_ops mvebu_pcie_ops = {
.read = mvebu_pcie_rd_conf,
.write = mvebu_pcie_wr_conf,
+ .add_bus = mvebu_pcie_add_bus,
};

static int mvebu_pcie_setup(int nr, struct pci_sys_data *sys)
@@ -823,7 +824,6 @@ static void mvebu_pcie_enable(struct mvebu_pcie *pcie)
hw.map_irq = of_irq_parse_and_map_pci;
hw.ops = &mvebu_pcie_ops;
hw.align_resource = mvebu_pcie_align_resource;
- hw.add_bus = mvebu_pcie_add_bus;

pci_common_init(&hw);
}
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index a63a47a70846..be6d56358320 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1885,6 +1885,8 @@ int __weak pcibios_root_bridge_prepare(struct pci_host_bridge *bridge)

void __weak pcibios_add_bus(struct pci_bus *bus)
{
+ if (bus->ops && bus->ops->add_bus)
+ bus->ops->add_bus(bus);
}

void __weak pcibios_remove_bus(struct pci_bus *bus)

Arnd

Liviu Dudau

2014-10-01 08:46:26 UTC

These are the functions I found that refer to pci_sys_dat=
=20
pcibios_add_bus
pcibios_remove_bus

=20
These are only needed if you want to do per HB processing of the =

bus

=20

pcibios_align_resource

=20
mvebu is the only user of this function.
=20

pci_mmap_page_range

=20
This is only needed when mapping a PCI resource to userspace. Is =

that your case here?

=20

pci_domain_nr
pci_proc_domain

=20
We have equivalent functionality in the generic patches for those=

=2E

=20

=20
We clearly don't need those functions for the new drivers, but that=

's not

the point. The problem is that when you build a kernel that has bot=

a traditional host bridge driver and a new one in it, you always ge=

t those

functions and they get called from the PCI core, with incorrect arg=

uments.

=20
FWIW, the last time we discussed this, I think I had suggested that t=

functions that are currently architecture specific and have a generic
__weak fallback could become function pointers in a per-host structur=

passed to pci_scan_root_bus, either a new structure or an extended

Agree to the general idea. But have a look why host drivers need the ad=
d_bus ops:
to add MSI information into the bus!! If we take care of the MSI in the=
generic
code there is less of a need for this function at all.

Best regards,
Liviu

=20
diff --git a/arch/arm/include/asm/mach/pci.h b/arch/arm/include/asm/m=

ach/pci.h

index 7fc42784becb..3da32fc631d0 100644
--- a/arch/arm/include/asm/mach/pci.h
+++ b/arch/arm/include/asm/mach/pci.h
@@ -36,7 +36,6 @@ struct hw_pci {
resource_size_t start,
resource_size_t size,
resource_size_t align);
- void (*add_bus)(struct pci_bus *bus);
void (*remove_bus)(struct pci_bus *bus);
};
=20
@@ -65,7 +64,6 @@ struct pci_sys_data {
resource_size_t start,
resource_size_t size,
resource_size_t align);
- void (*add_bus)(struct pci_bus *bus);
void (*remove_bus)(struct pci_bus *bus);
void *private_data; /* platform controller private data */
};
diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
index 17a26c17f7f5..3cbcf8dc41e4 100644
--- a/arch/arm/kernel/bios32.c
+++ b/arch/arm/kernel/bios32.c
@@ -360,13 +360,6 @@ void pcibios_fixup_bus(struct pci_bus *bus)
}
EXPORT_SYMBOL(pcibios_fixup_bus);
=20
-void pcibios_add_bus(struct pci_bus *bus)
-{
- struct pci_sys_data *sys =3D bus->sysdata;
- if (sys->add_bus)
- sys->add_bus(bus);
-}
-
void pcibios_remove_bus(struct pci_bus *bus)
{
struct pci_sys_data *sys =3D bus->sysdata;
@@ -475,7 +468,6 @@ static void pcibios_init_hw(struct device *parent=

, struct hw_pci *hw,

sys->swizzle =3D hw->swizzle;
sys->map_irq =3D hw->map_irq;
sys->align_resource =3D hw->align_resource;
- sys->add_bus =3D hw->add_bus;
sys->remove_bus =3D hw->remove_bus;
INIT_LIST_HEAD(&sys->resources);
=20
diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mveb=

u.c

index b1315e197ffb..c9a0ee0429e8 100644
--- a/drivers/pci/host/pci-mvebu.c
+++ b/drivers/pci/host/pci-mvebu.c
@@ -716,6 +716,7 @@ static int mvebu_pcie_rd_conf(struct pci_bus *bus=

, u32 devfn, int where,

static struct pci_ops mvebu_pcie_ops =3D {
.read =3D mvebu_pcie_rd_conf,
.write =3D mvebu_pcie_wr_conf,
+ .add_bus =3D mvebu_pcie_add_bus,
};
=20
static int mvebu_pcie_setup(int nr, struct pci_sys_data *sys)
@@ -823,7 +824,6 @@ static void mvebu_pcie_enable(struct mvebu_pcie *=

pcie)

hw.map_irq =3D of_irq_parse_and_map_pci;
hw.ops =3D &mvebu_pcie_ops;
hw.align_resource =3D mvebu_pcie_align_resource;
- hw.add_bus =3D mvebu_pcie_add_bus;
=20
pci_common_init(&hw);
}
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index a63a47a70846..be6d56358320 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1885,6 +1885,8 @@ int __weak pcibios_root_bridge_prepare(struct p=

ci_host_bridge *bridge)

=20
void __weak pcibios_add_bus(struct pci_bus *bus)
{
+ if (bus->ops && bus->ops->add_bus)
+ bus->ops->add_bus(bus); =09
}
=20
void __weak pcibios_remove_bus(struct pci_bus *bus)
=20
Arnd
=20
=20

Arnd Bergmann

2014-10-01 09:38:45 UTC

Post by Arnd Bergmann
pcibios_add_bus
pcibios_remove_bus

These are only needed if you want to do per HB processing of the bus

Post by Arnd Bergmann
pcibios_align_resource

mvebu is the only user of this function.

Post by Arnd Bergmann
pci_mmap_page_range

This is only needed when mapping a PCI resource to userspace. Is that your case here?

Post by Arnd Bergmann
pci_domain_nr
pci_proc_domain

We have equivalent functionality in the generic patches for those.

to add MSI information into the bus!! If we take care of the MSI in the generic
code there is less of a need for this function at all.

Right, if we can eliminate the need for some or all of the functions above,
we don't have to abstract them any more.

pcibios_remove_bus can just go away entirely, we don't have a single driver
on ARM that implements it. pcibios_add_bus as you say is just used for MSI
at the moment, and we could get rid of it by just moving the msi_chip
reference from pci_bus into pci_host_bridge.

The arm32 implementations of pci_domain_nr/pci_proc_domain can probably be
removed if we change the arm32 pcibios_init_hw function to call the new
interfaces that set the domain number.

pci_mmap_page_range could either get generalized some more in an attempt
to have a __weak default implementation that works on ARM, or it could
be changed to lose the dependency on pci_sys_data instead. In either
case, the change would involve using the generic pci_host_bridge_window
list.

pcibios_align_resource should probably be per host, and we could move
that into a pointer in pci_host_bridge, something like this:

diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
index b7c3a5ea1fca..d9cb6c916d54 100644
--- a/drivers/pci/setup-res.c
+++ b/drivers/pci/setup-res.c
@@ -200,11 +200,15 @@ static int pci_revert_fw_address(struct resource *res, struct pci_dev *dev,
static int __pci_assign_resource(struct pci_bus *bus, struct pci_dev *dev,
int resno, resource_size_t size, resource_size_t align)
{
+ struct pci_host_bridge *host = find_pci_host_bridge(bus);
+ resource_size_t (*alignf)(void *, const struct resource *,
+ resource_size_t, resource_size_t),
struct resource *res = dev->resource + resno;
resource_size_t min;
int ret;

min = (res->flags & IORESOURCE_IO) ? PCIBIOS_MIN_IO : PCIBIOS_MIN_MEM;
+ alignf = host->align_resource ?: pcibios_align_resource;

/*
* First, try exact prefetching match. Even if a 64-bit
@@ -215,7 +219,7 @@ static int __pci_assign_resource(struct pci_bus *bus, struct pci_dev *dev,
*/
ret = pci_bus_alloc_resource(bus, res, size, align, min,
IORESOURCE_PREFETCH | IORESOURCE_MEM_64,
- pcibios_align_resource, dev);
+ alignf, dev);
if (ret == 0)
return 0;

@@ -227,7 +231,7 @@ static int __pci_assign_resource(struct pci_bus *bus, struct pci_dev *dev,
(IORESOURCE_PREFETCH | IORESOURCE_MEM_64)) {
ret = pci_bus_alloc_resource(bus, res, size, align, min,
IORESOURCE_PREFETCH,
- pcibios_align_resource, dev);
+ alignf, dev);
if (ret == 0)
return 0;
}
@@ -240,7 +244,7 @@ static int __pci_assign_resource(struct pci_bus *bus, struct pci_dev *dev,
*/
if (res->flags & (IORESOURCE_PREFETCH | IORESOURCE_MEM_64))
ret = pci_bus_alloc_resource(bus, res, size, align, min, 0,
- pcibios_align_resource, dev);
+ alignf, dev);

return ret;
}

If we decide constantly calling find_pci_host_bridge() is too expensive, we can
be more clever about it.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Lorenzo Pieralisi

2014-10-07 12:06:59 UTC

On Wed, Oct 01, 2014 at 10:38:45AM +0100, Arnd Bergmann wrote:

[...]

Post by Arnd Bergmann
pci_mmap_page_range could either get generalized some more in an attempt
to have a __weak default implementation that works on ARM, or it could
be changed to lose the dependency on pci_sys_data instead. In either
case, the change would involve using the generic pci_host_bridge_window
list.

On ARM pci_mmap_page_range requires pci_sys_data to retrieve its
mem_offset parameter. I had a look, and I do not understand *why*
it is required in that function, so I am asking. That function
is basically used to map PCI resources to userspace, IIUC, through
/proc or /sysfs file mappings. As far as I understand those mappings
expect VMA pgoff to be the CPU address when files representing resources
are mmapped from /proc and 0 when mmapped from /sys (I mean from
userspace, then VMA pgoff should be updated by the kernel to map the
resource).

Question is: why pci_mmap_page_range() should apply an additional
shift to the VMA pgoff based on pci_sys_data.mem_offset, which represents
the offset from cpu->bus offset. I do not understand that. PowerPC
does not seem to apply that fix-up (in PowerPC __pci_mmap_make_offset there
is commented out code which prevents the pci_mem_offset shift to be
applied). I think it all boils down to what the userspace interface is
expecting when the memory areas are mmapped, if anyone has comments on
this that is appreciated.

Thanks,
Lorenzo

Arnd Bergmann

2014-10-07 13:52:27 UTC

Applying the mem_offset is certainly the more intuitive way, since
that lets you read the PCI BAR values from a device and access the
device with the appropriate offsets.

Post by Lorenzo Pieralisi
Question is: why pci_mmap_page_range() should apply an additional
shift to the VMA pgoff based on pci_sys_data.mem_offset, which represents
the offset from cpu->bus offset. I do not understand that. PowerPC
does not seem to apply that fix-up (in PowerPC __pci_mmap_make_offset there
is commented out code which prevents the pci_mem_offset shift to be
applied). I think it all boils down to what the userspace interface is
expecting when the memory areas are mmapped, if anyone has comments on
this that is appreciated.

The important part is certainly that whatever transformation is done
by pci_resource_to_user() gets undone by __pci_mmap_make_offset().

In case of PowerPC and Microblaze, the mem_offset handling is commented
out in both, to work around X11 trying to use the same values on
/dev/mem. However, they do have the respective fixup for io_offset.

sparc applies the offset in both places for both io_offset and mem_offset.
xtensa applies only io_offset in __pci_mmap_make_offset but neither
in pci_resource_to_user. This probably works because the mem_offset is
always zero there.
mips applies a different fixup (for 36-bit addressing), but not the
mem_offset.

Every other architecture applies no offset here, neither in __pci_mmap_make_offset/pci_mmap_page_range nor in pci_resource_to_user

The only hint I could find for how the ARM version came to be is
from the historic kernel tree git log for linux-2.5.42, which added
the current code as

2002/10/13 11:05:47+01:00 rmk
[ARM] Update pcibios_enable_device, supply pci_mmap_page_range()
Update pcibios_enable_device to only enable requested resources,
mainly for IDE. Supply a pci_mmap_page_range() function to allow
user space to mmap PCI regions.

At that point, only two platforms had a nonzero mem_offset:
footbridge/dc21285 and integrator/pci_v3. Both were using VGA,
and presumably used this to make X work. (rmk might remember
details).

The code at the time matched what powerpc and sparc did, but then
both implemented pci_resource_to_user() in order for libpciaccess
to work correctly (bcea1db16b for sparc, 463ce0e103f for powerpc),
and later powerpc changed it again to not apply the offset in
pci_resource_to_user or pci_mmap_page_range in 396a1a5832ae.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Lorenzo Pieralisi

2014-10-07 14:47:50 UTC

Applying the mem_offset is certainly the more intuitive way, since
that lets you read the PCI BAR values from a device and access the
device with the appropriate offsets.

Ok, but I am referring to this snippet (drivers/pci/pci-sysfs.c):

/* pci_mmap_page_range() expects the same kind of entry as coming
* from /proc/bus/pci/ which is a "user visible" value. If this is
* different from the resource itself, arch will do necessary fixup.
*/
pci_resource_to_user(pdev, i, res, &start, &end);

--> Here start represents a CPU physical address, if pci_resource_to_user()
does not fix it up, correct ?

vma->vm_pgoff += start >> PAGE_SHIFT;

[...]

return pci_mmap_page_range(...);

pci_mmap_page_range() applies (mem_offset >> PAGE_SHIFT) to pgoff in the
ARM implemention.

Is not there a mismatch here on platforms where mem_offset != 0 ?

The important part is certainly that whatever transformation is done
by pci_resource_to_user() gets undone by __pci_mmap_make_offset().

Exactly, it does not seem to be the case above, that's why I asked.

Post by Arnd Bergmann
In case of PowerPC and Microblaze, the mem_offset handling is commented
out in both, to work around X11 trying to use the same values on
/dev/mem. However, they do have the respective fixup for io_offset.
sparc applies the offset in both places for both io_offset and mem_offset.
xtensa applies only io_offset in __pci_mmap_make_offset but neither
in pci_resource_to_user. This probably works because the mem_offset is
always zero there.
mips applies a different fixup (for 36-bit addressing), but not the
mem_offset.
Every other architecture applies no offset here, neither in __pci_mmap_make_offset/pci_mmap_page_range nor in pci_resource_to_user
The only hint I could find for how the ARM version came to be is
from the historic kernel tree git log for linux-2.5.42, which added
the current code as
2002/10/13 11:05:47+01:00 rmk
[ARM] Update pcibios_enable_device, supply pci_mmap_page_range()
Update pcibios_enable_device to only enable requested resources,
mainly for IDE. Supply a pci_mmap_page_range() function to allow
user space to mmap PCI regions.
footbridge/dc21285 and integrator/pci_v3. Both were using VGA,
and presumably used this to make X work. (rmk might remember
details).

I think that, as I mentioned, it boils down to what the userspace
interface (proc/sys and they seem to differ) is supposed to be passed
from userspace processes upon mmap.

Post by Arnd Bergmann
The code at the time matched what powerpc and sparc did, but then
both implemented pci_resource_to_user() in order for libpciaccess
to work correctly (bcea1db16b for sparc, 463ce0e103f for powerpc),
and later powerpc changed it again to not apply the offset in
pci_resource_to_user or pci_mmap_page_range in 396a1a5832ae.

I will keep investigating, thank you for your help, any further comments
appreciated.

Lorenzo

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

Arnd Bergmann

2014-10-07 21:39:47 UTC

Applying the mem_offset is certainly the more intuitive way, since
that lets you read the PCI BAR values from a device and access the
device with the appropriate offsets.

/* pci_mmap_page_range() expects the same kind of entry as coming
* from /proc/bus/pci/ which is a "user visible" value. If this is
* different from the resource itself, arch will do necessary fixup.
*/
pci_resource_to_user(pdev, i, res, &start, &end);
--> Here start represents a CPU physical address, if pci_resource_to_user()
does not fix it up, correct ?
vma->vm_pgoff += start >> PAGE_SHIFT;
[...]
return pci_mmap_page_range(...);
pci_mmap_page_range() applies (mem_offset >> PAGE_SHIFT) to pgoff in the
ARM implemention.
Is not there a mismatch here on platforms where mem_offset != 0 ?

Yes, I think that's right: ARM never gained its own pci_resource_to_user()
implementation, presumably because nobody ran into this problem and
debugged it all the way.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Lorenzo Pieralisi

2014-10-08 10:19:43 UTC

Applying the mem_offset is certainly the more intuitive way, since
that lets you read the PCI BAR values from a device and access the
device with the appropriate offsets.

Yes, I think that's right: ARM never gained its own pci_resource_to_user()
implementation, presumably because nobody ran into this problem and
debugged it all the way.

Ok. So, unless I am missing something, on platform with mem_offset != 0
/proc and /sys interfaces for remapping PCI resources can't work (IIUC
the proc interface expects the user to pass in the resource address as
seen from /proc/bus/pci/devices - which are not BAR values. Even if the
user passed the BAR value to mmap, pci_mmap_fits() in proc_bus_pci_mmap()
would fail since it compares the pgoff to resource values, which are not
BAR values).

As things stand I think we can safely remove the mem_offset (and
pci_sys_data dependency) from pci_mmap_page_range(). I do not think
we can break userspace in any way, basically because it can't work at
the moment, again, happy to be corrected if I am wrong, please shout.

Or we can add mem_offset to the host bridge (after all architectures like
PowerPC and Microblaze have a pci_mem_offset variable in their host
controllers), but still, this removes pci_sys_data dependency but does
not solve the pci_mmap_page_range() issue.

Lorenzo

Arnd Bergmann

2014-10-08 14:47:43 UTC

Post by Lorenzo Pieralisi
Ok. So, unless I am missing something, on platform with mem_offset != 0
/proc and /sys interfaces for remapping PCI resources can't work (IIUC
the proc interface expects the user to pass in the resource address as
seen from /proc/bus/pci/devices - which are not BAR values. Even if the
user passed the BAR value to mmap, pci_mmap_fits() in proc_bus_pci_mmap()
would fail since it compares the pgoff to resource values, which are not
BAR values).

I think you are right for the sysfs interface, that one can't possibly
work because of the incorrect address computation.

For the /procfs interface, I think it can work as long as the offsets
used there are coming from the config space dump in /proc/bus/pci/*
rather than from the /sys/bus/pci/devices/*/resource file.

Post by Lorenzo Pieralisi
As things stand I think we can safely remove the mem_offset (and
pci_sys_data dependency) from pci_mmap_page_range(). I do not think
we can break userspace in any way, basically because it can't work at
the moment, again, happy to be corrected if I am wrong, please shout.

Please look at the procfs interface again. That one can be defined
in two ways (either like sparc and arm, or like powerpc and microblaze)
but either one should be able to work with user space that expects
that interface and break with user space that expects the other one.

Post by Lorenzo Pieralisi
Or we can add mem_offset to the host bridge (after all architectures like
PowerPC and Microblaze have a pci_mem_offset variable in their host
controllers), but still, this removes pci_sys_data dependency but does
not solve the pci_mmap_page_range() issue.

The host bridge already stores the mem_offset in terms of the resource
list, so we could readily use that, except that it might break the
powerpc hack if that is still in use.

Arnd

Lorenzo Pieralisi

2014-10-09 09:04:20 UTC

I think you are right for the sysfs interface, that one can't possibly
work because of the incorrect address computation.
For the /procfs interface, I think it can work as long as the offsets
used there are coming from the config space dump in /proc/bus/pci/*
rather than from the /sys/bus/pci/devices/*/resource file.

I agree as long as pci_mmap_page_range() is concerned, but I am
referring to the pci_mmap_fits() implementation here:

start = vma->vm_pgoff;
size = ((pci_resource_len(pdev, resno) - 1) >> PAGE_SHIFT) + 1;
pci_start = (mmap_api == PCI_MMAP_PROCFS) ?
pci_resource_start(pdev, resno) >> PAGE_SHIFT : 0;
if (start >= pci_start && start < pci_start + size &&
start + nr <= pci_start + size)
return 1;
return 0;

pci_mmap_fits(), when mapping from procfs always check the offset against
resources, which are fixed-up addresses. If we passed the values dumped
from the device config space (as pci_mmap_page_range() expects on arm) IMHO
the check above would fail (always referring to platforms where
mem_offset != 0).

Last changes where introduced by commit 8c05cd08a, whose commit log adds
to my confusion:

"[...] I think what we want here is for pci_start to be 0 when mmap_api ==
PCI_MMAP_PROCFS.[...]"

But that's not what the code does.

I will try to grab an integrator board to give it a go.

The host bridge already stores the mem_offset in terms of the resource
list, so we could readily use that, except that it might break the
powerpc hack if that is still in use.

Well, yes, I am not saying it can't be done by using the resources list,
I am just trying to understand if that's really useful.

Thank you !
Lorenzo

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

Arnd Bergmann

2014-10-09 10:51:43 UTC

Post by Arnd Bergmann
Please look at the procfs interface again. That one can be defined
in two ways (either like sparc and arm, or like powerpc and microblaze)
but either one should be able to work with user space that expects
that interface and break with user space that expects the other one.

I agree as long as pci_mmap_page_range() is concerned, but I am
start = vma->vm_pgoff;
size = ((pci_resource_len(pdev, resno) - 1) >> PAGE_SHIFT) + 1;
pci_start = (mmap_api == PCI_MMAP_PROCFS) ?
pci_resource_start(pdev, resno) >> PAGE_SHIFT : 0;
if (start >= pci_start && start < pci_start + size &&
start + nr <= pci_start + size)
return 1;
return 0;
pci_mmap_fits(), when mapping from procfs always check the offset against
resources, which are fixed-up addresses. If we passed the values dumped
from the device config space (as pci_mmap_page_range() expects on arm) IMHO
the check above would fail (always referring to platforms where
mem_offset != 0).

Ah, I see it now too.

Post by Lorenzo Pieralisi
Last changes where introduced by commit 8c05cd08a, whose commit log adds
"[...] I think what we want here is for pci_start to be 0 when mmap_api ==
PCI_MMAP_PROCFS.[...]"
But that's not what the code does.

My best guess is that this is a typo and that Darrick meant PCI_MMAP_SYSFS
in the changelog, which is the same thing that the code does. It's also
the sensible thing to do.

This probably means that the procfs interface is now also broken on
sparc.

Post by Lorenzo Pieralisi
I will try to grab an integrator board to give it a go.

Ok, good idea.

The host bridge already stores the mem_offset in terms of the resource
list, so we could readily use that, except that it might break the
powerpc hack if that is still in use.

Well, yes, I am not saying it can't be done by using the resources list,
I am just trying to understand if that's really useful.

The PCI core tries to be ready for PCI host bridges that have multiple
discontiguous memory spaces with different offsets, although I don't know
of anybody has that. However if we decide to implement a generic
pci_mmap_page_range that tries to take the offset into account, we should
use the resource list in the host bridge because it can tell us the correct
offsets.

However, given what you found, the procfs interface being broken since
2010 on both architectures (arm32 and sparc) that try to honor the offset,
we should probably go back to your previous suggestion of removing
the offset handling, which would make it possible to use the procfs
interface and the sysfs interface on all architectures.

Would you be able to prepare a patch that does this and circulate that
with the sparc, powerpc and microblaze maintainers as well as Darrick
and Martin who were involved with the pci_mmap_fits change?

Arnd

Lorenzo Pieralisi

2014-10-10 13:58:04 UTC

On Thu, Oct 09, 2014 at 11:51:43AM +0100, Arnd Bergmann wrote:

[...]

My best guess is that this is a typo and that Darrick meant PCI_MMAP_SYSFS
in the changelog, which is the same thing that the code does. It's also
the sensible thing to do.
This probably means that the procfs interface is now also broken on
sparc.

Post by Lorenzo Pieralisi
I will try to grab an integrator board to give it a go.

Ok, good idea.

Grabbed, tested it, my theory was correct, I can't map PCI resources
to userspace. Actually, if I pass resource offset as a fixed-up address, mmap
succeeds through proc, but it does not mmap the resource, it maps
the resource + mem_offset that happens to be RAM :D for the PCI slot I am
using.

I am testing on an oldish (3.16) kernel since I am having trouble with
mainline PCI and my network adapter on integrator, but I do not see why this
is a problem, this bug has been there forever.

By removing mem_offset from pci_mmap_page_range() everything works fine,
both proc and sys mappings are ok.

The host bridge already stores the mem_offset in terms of the resource
list, so we could readily use that, except that it might break the
powerpc hack if that is still in use.

Well, yes, I am not saying it can't be done by using the resources list,
I am just trying to understand if that's really useful.

The PCI core tries to be ready for PCI host bridges that have multiple
discontiguous memory spaces with different offsets, although I don't know
of anybody has that. However if we decide to implement a generic
pci_mmap_page_range that tries to take the offset into account, we should
use the resource list in the host bridge because it can tell us the correct
offsets.
However, given what you found, the procfs interface being broken since
2010 on both architectures (arm32 and sparc) that try to honor the offset,
we should probably go back to your previous suggestion of removing
the offset handling, which would make it possible to use the procfs
interface and the sysfs interface on all architectures.
Would you be able to prepare a patch that does this and circulate that
with the sparc, powerpc and microblaze maintainers as well as Darrick
and Martin who were involved with the pci_mmap_fits change?

Yes, but let's step back a second. I think that the proc interface
should expect an offset as passed to the user in /proc/bus/pci/devices,
and there the resource is exposed through pci_resource_to_user().

Hence, the pci_mmap_fits() should check the offset against the
resource filtered through pci_resource_to_user(), job done, patch
is trivial, and does what pci_resource_to_user() was meant for IMHO.

Then we have to decide what to do with arm32 code:

1) we remove mem_offset from pci_mmap_page_range() (and rely on default
pci_resource_to_user())

or

2) we provide pci_resource_to_user() for arm32 which does the CPU->bus
conversion for us (and leave mem_offset as-is in pci_mmap_range())

Thoughts ?

Thanks,
Lorenzo

Arnd Bergmann

2014-10-10 18:31:26 UTC

My best guess is that this is a typo and that Darrick meant PCI_MMAP_SYSFS
in the changelog, which is the same thing that the code does. It's also
the sensible thing to do.
This probably means that the procfs interface is now also broken on
sparc.

Post by Lorenzo Pieralisi
I will try to grab an integrator board to give it a go.

Ok, good idea.

Grabbed, tested it, my theory was correct, I can't map PCI resources
to userspace. Actually, if I pass resource offset as a fixed-up address, mmap
succeeds through proc, but it does not mmap the resource, it maps
the resource + mem_offset that happens to be RAM :D for the PCI slot I am
using.
I am testing on an oldish (3.16) kernel since I am having trouble with
mainline PCI and my network adapter on integrator, but I do not see why this
is a problem, this bug has been there forever.

I would guess that almost the only users of the sysfs and procfs
interfaces are Xorg drivers, you certainly don't need it to get
a network adapter working.

Post by Lorenzo Pieralisi
By removing mem_offset from pci_mmap_page_range() everything works fine,
both proc and sys mappings are ok.

Ok, thanks for confirming!

Post by Arnd Bergmann
However, given what you found, the procfs interface being broken since
2010 on both architectures (arm32 and sparc) that try to honor the offset,
we should probably go back to your previous suggestion of removing
the offset handling, which would make it possible to use the procfs
interface and the sysfs interface on all architectures.
Would you be able to prepare a patch that does this and circulate that
with the sparc, powerpc and microblaze maintainers as well as Darrick
and Martin who were involved with the pci_mmap_fits change?

My point was that there is no reason why sparc and powerpc should
do this differently. At the moment they do and sparc is broken
as you proved. We can either fix sparc to restore the old behavior
by adding pci_resource_to_user to pci_mmap_fits, or by making it
do what powerpc does, essentially removing the memory space handling
from pci_resource_to_user.

Whatever we do for sparc is probably what we need to do on ARM as well,
except that ARM has been broken for a longer time than sparc.

Post by Lorenzo Pieralisi
1) we remove mem_offset from pci_mmap_page_range() (and rely on default
pci_resource_to_user())
or
2) we provide pci_resource_to_user() for arm32 which does the CPU->bus
conversion for us (and leave mem_offset as-is in pci_mmap_range())

I'd vote for 1) to get it in line with the only working architectures
that currently use a nonzero offset, but Russell needs to have the final
word on this, and I still think we have to involve the sparc and powerpc
maintainers as well, hoping to find a common solution for everybody.

Making a user space interface behave differently based on the CPU
architecture is a bad idea.

Arnd

Lorenzo Pieralisi

2014-10-13 09:36:26 UTC

My best guess is that this is a typo and that Darrick meant PCI_MMAP_SYSFS
in the changelog, which is the same thing that the code does. It's also
the sensible thing to do.
This probably means that the procfs interface is now also broken on
sparc.

Post by Lorenzo Pieralisi
I will try to grab an integrator board to give it a go.

Ok, good idea.

Grabbed, tested it, my theory was correct, I can't map PCI resources
to userspace. Actually, if I pass resource offset as a fixed-up address, mmap
succeeds through proc, but it does not mmap the resource, it maps
the resource + mem_offset that happens to be RAM :D for the PCI slot I am
using.
I am testing on an oldish (3.16) kernel since I am having trouble with
mainline PCI and my network adapter on integrator, but I do not see why this
is a problem, this bug has been there forever.

I would guess that almost the only users of the sysfs and procfs
interfaces are Xorg drivers, you certainly don't need it to get
a network adapter working.

The issue I am facing is not related to the PCI mmap implementation,
that's certainly broken but does not stop me from using the board.

[...]

Post by Lorenzo Pieralisi
By removing mem_offset from pci_mmap_page_range() everything works fine,
both proc and sys mappings are ok.

Ok, thanks for confirming!

My point was that there is no reason why sparc and powerpc should
do this differently. At the moment they do and sparc is broken
as you proved. We can either fix sparc to restore the old behavior
by adding pci_resource_to_user to pci_mmap_fits, or by making it
do what powerpc does, essentially removing the memory space handling
from pci_resource_to_user.
Whatever we do for sparc is probably what we need to do on ARM as well,
except that ARM has been broken for a longer time than sparc.

I'd vote for 1) to get it in line with the only working architectures
that currently use a nonzero offset, but Russell needs to have the final
word on this, and I still think we have to involve the sparc and powerpc
maintainers as well, hoping to find a common solution for everybody.
Making a user space interface behave differently based on the CPU
architecture is a bad idea.

I agree with you, I will put together a patchset and copy all people
who should be involved.

Lorenzo

Lorenzo Pieralisi

2014-10-22 15:59:14 UTC

On Wed, Oct 01, 2014 at 10:38:45AM +0100, Arnd Bergmann wrote:

[...]

Post by Arnd Bergmann
The arm32 implementations of pci_domain_nr/pci_proc_domain can probably be
removed if we change the arm32 pcibios_init_hw function to call the new
interfaces that set the domain number.

I wished, but it is a bit more complicated than I thought unfortunately,
mostly because some drivers, eg cns3xxx set the domain numbers
statically in pci_sys_data and this sets a chain of dependency that is
not easy to untangle. I think cns3xxx is the only legacy driver that "uses"
the domain number (in pci_sys_data) in a way that clashes with the
generic domain_nr implementation, I need to give it more thought.

I need to repost my series, but I *think* we can consider the dependency on
pci_sys_data gone in pci_mmap_page_range().

Post by Arnd Bergmann
pcibios_align_resource should probably be per host, and we could move

Yes, and that's likely to be true for add_bus too. I wonder what's the
best course of action. Putting together all the bits and pieces required
to remove PCI bios dependency from this patch can take a while, I wonder
whether we should aim for merging this driver (rebased on top of my port to the
new parse ranges API) with the ARM/ARM64 ifdeffery and clean it up later
or aim for the whole thing at once, I am just worried it can take us a while.

Lorenzo

Post by Arnd Bergmann
diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
index b7c3a5ea1fca..d9cb6c916d54 100644
--- a/drivers/pci/setup-res.c
+++ b/drivers/pci/setup-res.c
@@ -200,11 +200,15 @@ static int pci_revert_fw_address(struct resource *res, struct pci_dev *dev,
static int __pci_assign_resource(struct pci_bus *bus, struct pci_dev *dev,
int resno, resource_size_t size, resource_size_t align)
{
+ struct pci_host_bridge *host = find_pci_host_bridge(bus);
+ resource_size_t (*alignf)(void *, const struct resource *,
+ resource_size_t, resource_size_t),
struct resource *res = dev->resource + resno;
resource_size_t min;
int ret;
min = (res->flags & IORESOURCE_IO) ? PCIBIOS_MIN_IO : PCIBIOS_MIN_MEM;
+ alignf = host->align_resource ?: pcibios_align_resource;
/*
* First, try exact prefetching match. Even if a 64-bit
@@ -215,7 +219,7 @@ static int __pci_assign_resource(struct pci_bus *bus, struct pci_dev *dev,
*/
ret = pci_bus_alloc_resource(bus, res, size, align, min,
IORESOURCE_PREFETCH | IORESOURCE_MEM_64,
- pcibios_align_resource, dev);
+ alignf, dev);
if (ret == 0)
return 0;
@@ -227,7 +231,7 @@ static int __pci_assign_resource(struct pci_bus *bus, struct pci_dev *dev,
(IORESOURCE_PREFETCH | IORESOURCE_MEM_64)) {
ret = pci_bus_alloc_resource(bus, res, size, align, min,
IORESOURCE_PREFETCH,
- pcibios_align_resource, dev);
+ alignf, dev);
if (ret == 0)
return 0;
}
@@ -240,7 +244,7 @@ static int __pci_assign_resource(struct pci_bus *bus, struct pci_dev *dev,
*/
if (res->flags & (IORESOURCE_PREFETCH | IORESOURCE_MEM_64))
ret = pci_bus_alloc_resource(bus, res, size, align, min, 0,
- pcibios_align_resource, dev);
+ alignf, dev);
return ret;
}
If we decide constantly calling find_pci_host_bridge() is too expensive, we can
be more clever about it.
Arnd

Bjorn Helgaas

2014-10-22 16:49:14 UTC

On Wed, Oct 22, 2014 at 9:59 AM, Lorenzo Pieralisi

... I wonder what's the
best course of action. Putting together all the bits and pieces required
to remove PCI bios dependency from this patch can take a while, I wonder
whether we should aim for merging this driver (rebased on top of my port to the
new parse ranges API) with the ARM/ARM64 ifdeffery and clean it up later
or aim for the whole thing at once, I am just worried it can take us a while.

I haven't looked at your patches, but "the whole thing at once" is
never *my* goal. A gradual cleanup is just fine with me.

Bjorn

Arnd Bergmann

2014-10-22 20:52:19 UTC

Just had a look at that driver, shouldn't be too hard to change, see below.

Signed-off-by: Arnd Bergmann <***@arndb.de>

diff --git a/arch/arm/mach-cns3xxx/pcie.c b/arch/arm/mach-cns3xxx/pcie.c
index 45d6bd09e6ef..aa4b9d7c52fd 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,10 @@ 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 +309,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 +329,9 @@ 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);
+ hw_pci->domain = i;
+ private_data = &cns3xxx_pcie[i];
+ pci_common_init(&hw_pci);
}

pci_assign_unassigned_resources();

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

Liviu Dudau

2014-10-23 09:13:09 UTC

=20
[...]
=20

The arm32 implementations of pci_domain_nr/pci_proc_domain can pr=

obably be

removed if we change the arm32 pcibios_init_hw function to call t=

he new

interfaces that set the domain number.

=20
I wished, but it is a bit more complicated than I thought unfortuna=

tely,

mostly because some drivers, eg cns3xxx set the domain numbers
statically in pci_sys_data and this sets a chain of dependency that=

not easy to untangle. I think cns3xxx is the only legacy driver tha=

t "uses"

the domain number (in pci_sys_data) in a way that clashes with the
generic domain_nr implementation, I need to give it more thought.

=20
Just had a look at that driver, shouldn't be too hard to change, see =

below.

I like this!

One thing though ...

=20
=20
diff --git a/arch/arm/mach-cns3xxx/pcie.c b/arch/arm/mach-cns3xxx/pci=

e.c

index 45d6bd09e6ef..aa4b9d7c52fd 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;
};
=20
-static struct cns3xxx_pcie cns3xxx_pcie[]; /* forward decl. */
-
static struct cns3xxx_pcie *sysdata_to_cnspci(void *sysdata)
{
struct pci_sys_data *root =3D sysdata;
=20
- return &cns3xxx_pcie[root->domain];
+ return root->private_data;
}
=20
static struct cns3xxx_pcie *pdev_to_cnspci(const struct pci_dev *dev=

)

@@ -192,13 +189,7 @@ static struct cns3xxx_pcie cns3xxx_pcie[] =3D {
.flags =3D IORESOURCE_MEM,
},
.irqs =3D { IRQ_CNS3XXX_PCIE0_RC, IRQ_CNS3XXX_PCIE0_DEVICE, },
- .hw_pci =3D {
- .domain =3D 0,
- .nr_controllers =3D 1,
- .ops =3D &cns3xxx_pcie_ops,
- .setup =3D cns3xxx_pci_setup,
- .map_irq =3D cns3xxx_pcie_map_irq,
- },
+ .port =3D 0,
},
[1] =3D {
.host_regs =3D (void __iomem *)CNS3XXX_PCIE1_HOST_BASE_VIRT,
@@ -217,19 +208,13 @@ static struct cns3xxx_pcie cns3xxx_pcie[] =3D {
.flags =3D IORESOURCE_MEM,
},
.irqs =3D { IRQ_CNS3XXX_PCIE1_RC, IRQ_CNS3XXX_PCIE1_DEVICE, },
- .hw_pci =3D {
- .domain =3D 1,
- .nr_controllers =3D 1,
- .ops =3D &cns3xxx_pcie_ops,
- .setup =3D cns3xxx_pci_setup,
- .map_irq =3D cns3xxx_pcie_map_irq,
- },
+ .port =3D 1,
},
};
=20
static void __init cns3xxx_pcie_check_link(struct cns3xxx_pcie *cnsp=

ci)

{
- int port =3D cnspci->hw_pci.domain;
+ int port =3D cnspci->port;
u32 reg;
unsigned long time;
=20
@@ -260,9 +245,10 @@ static void __init cns3xxx_pcie_check_link(struc=

t cns3xxx_pcie *cnspci)

=20
static void __init cns3xxx_pcie_hw_init(struct cns3xxx_pcie *cnspci)
{
- int port =3D cnspci->hw_pci.domain;
+ int port =3D cnspci->port;
struct pci_sys_data sd =3D {
.domain =3D port,
+ .private_data =3D cnspci,
};
struct pci_bus bus =3D {
.number =3D 0,
@@ -323,6 +309,14 @@ static int cns3xxx_pcie_abort_handler(unsigned l=

ong addr, unsigned int fsr,

void __init cns3xxx_pcie_init_late(void)
{
int i;
+ void *private_data;
+ struct hw_pci hw_pci =3D {
+ .nr_controllers =3D 1,
+ .ops =3D &cns3xxx_pcie_ops,
+ .setup =3D cns3xxx_pci_setup,
+ .map_irq =3D cns3xxx_pcie_map_irq,
+ .private_data =3D &private_data,
+ };
=20
pcibios_min_io =3D 0;
pcibios_min_mem =3D 0;
@@ -335,7 +329,9 @@ 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);
+ hw_pci->domain =3D i;
+ private_data =3D &cns3xxx_pcie[i];

Is this dance with pointers absolutely necessary? Does gcc though dishe=
s at you
for doing hw_pci->private_data =3D &cns3xxx_pcie[i] directly?

Best regards,
Liviu

+ pci_common_init(&hw_pci);
}
=20
pci_assign_unassigned_resources();
=20
=20
=20
=20

Lorenzo Pieralisi

2014-10-23 11:27:31 UTC

Just had a look at that driver, shouldn't be too hard to change, see below.

I like this!
One thing though ...

I like it too, it is one way of removing the artificial domain dependency
from this driver.

I think that by removing that, we could switch to CONFIG_PCI_DOMAINS_GENERIC
on ARM32. I will remove the dependency in drivers/pci/host/pci-mvebu.c
introduced by commit 2613ba48. pci_sys_data.domain is always 0 in that
driver so its usefulness is doubtful, comments welcome, copied Jason in
if he has comments.

[...]

Post by Arnd Bergmann
@@ -323,6 +309,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 +329,9 @@ 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);
+ hw_pci->domain = i;

+ hw_pci.domain = i;

I will remove this since if we move to generic domains it is useless to
pass the value through hw_pci.

Post by Arnd Bergmann
+ private_data = &cns3xxx_pcie[i];

Is this dance with pointers absolutely necessary? Does gcc though dishes at you
for doing hw_pci->private_data = &cns3xxx_pcie[i] directly?

You can't, hw_pci.private_data is void **.

Lorenzo

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

Jason Gunthorpe

2014-10-23 16:52:06 UTC

Post by Lorenzo Pieralisi
I think that by removing that, we could switch to CONFIG_PCI_DOMAINS_GENERIC
on ARM32. I will remove the dependency in drivers/pci/host/pci-mvebu.c
introduced by commit 2613ba48. pci_sys_data.domain is always 0 in that
driver so its usefulness is doubtful, comments welcome, copied Jason in
if he has comments.

pcie-mvebu is like all the other new drivers, each top level DT node
that introduces the interface should have a unique domain number. It
would be very strange (and currently unsupported by the driver) to
ever have more than 1 mvebu top level node in any DT.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Arnd Bergmann

2014-10-23 13:33:16 UTC

Post by Arnd Bergmann
@@ -335,7 +329,9 @@ 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);
+ hw_pci->domain = i;
+ private_data = &cns3xxx_pcie[i];

Is this dance with pointers absolutely necessary? Does gcc though dishes at you
for doing hw_pci->private_data = &cns3xxx_pcie[i] directly?

hw_pci->private_data is an array of pointers to private_data for each
host controller instance within the domain. There is only one entry
here, but you still the the correct type, so that would be

hw_pci->private_data = (void **)&&cns3xxx_pcie[i];

which is even more confusing and ugly than what I wrote. If you have
a better idea, I'm all for it. Maybe it's clearer to write like this
(taken from rcar driver)?

void *hw_private[1];
hw_pci.private_data = hw_private;

for each host {
...
hw_private[0] = &cns3xxx_pcie[i];
pci_common_init_dev(&hw_pci);
}

Note that all 'modern' controllers always use nr_controllers=1, so we
only need a single private_data pointer per domain, and the entire
hw_pci interface is a bit pointless.

The platforms that currently require it are iop13xx, dove, mv78xx0
and orion5x. We have plans to remove the last three platforms in
the next merge window or two, once all users are able to migrate to
mach-mvebu. Once that happens, we could probably move the entire
hw_pci logic that deals with multiple hosts per domain into the
iop13xx pci driver if we want to. A less intrusive simplification
would be to convert all 'multiplatform'-aware host controllers to
use pci_common_init_dev() and then take hw_pci out of that.

See below for a sample patch I just did. It duplicates the code from
pci_common_init_dev/pci_common_init because we know that all users
of pci_common_init_dev are modern and only pass a single host bridge.
The new pci_common_init_dev is simpler than the old one but should
do the exact same thing for all current users, with the addition
of propagating the return value.

pci_init_single() is the new internal helper and we should be able to
convert all existing users of pci_common_init_dev() to use that directly
and no longer define hw_pci at all.

I've converted two drivers to give an example, but the conversions
should be done in follow-up patches really, and the pci_common_init_dev
function removed after all users are moved over.

The new pci_init_single() is also rather simple, and it should just
converge with what we do for arm64 over time.

Arnd

---
arch/arm/include/asm/mach/pci.h | 20 ++++---
arch/arm/kernel/bios32.c | 103 ++++++++++++++++++++++++++++++++++--
drivers/pci/host/pci-host-generic.c | 53 ++++++++-----------
drivers/pci/host/pci-mvebu.c | 44 +++++++--------
4 files changed, 157 insertions(+), 63 deletions(-)

diff --git a/arch/arm/include/asm/mach/pci.h b/arch/arm/include/asm/mach/pci.h
index 7fc42784becb..fe7e13759ec0 100644
--- a/arch/arm/include/asm/mach/pci.h
+++ b/arch/arm/include/asm/mach/pci.h
@@ -73,16 +73,22 @@ struct pci_sys_data {
/*
* Call this with your hw_pci struct to initialise the PCI system.
*/
-void pci_common_init_dev(struct device *, struct hw_pci *);
+void pci_common_init(struct hw_pci *);

/*
- * Compatibility wrapper for older platforms that do not care about
- * passing the parent device.
+ * Used by modern platforms, only one host allowed.
*/
-static inline void pci_common_init(struct hw_pci *hw)
-{
- pci_common_init_dev(NULL, hw);
-}
+int pci_common_init_dev(struct device *, struct hw_pci *);
+
+/*
+ * Replaces pci_common_init_dev for drivers that want to do the
+ * initialization simpler and avoid defining hw_pci
+ */
+int pci_init_single(struct device *parent,
+ struct pci_sys_data *sys,
+ struct pci_bus *(*scan)(int nr, struct pci_sys_data *),
+ struct pci_ops *ops);
+

/*
* Setup early fixed I/O mapping.
diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
index 17a26c17f7f5..bccc8703e575 100644
--- a/arch/arm/kernel/bios32.c
+++ b/arch/arm/kernel/bios32.c
@@ -456,8 +456,7 @@ static int pcibios_init_resources(int busnr, struct pci_sys_data *sys)
return 0;
}

-static void pcibios_init_hw(struct device *parent, struct hw_pci *hw,
- struct list_head *head)
+static void pcibios_init_hw(struct hw_pci *hw, struct list_head *head)
{
struct pci_sys_data *sys = NULL;
int ret;
@@ -494,7 +493,7 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw,
if (hw->scan)
sys->bus = hw->scan(nr, sys);
else
- sys->bus = pci_scan_root_bus(parent, sys->busnr,
+ sys->bus = pci_scan_root_bus(NULL, sys->busnr,
hw->ops, sys, &sys->resources);

if (!sys->bus)
@@ -511,7 +510,7 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw,
}
}

-void pci_common_init_dev(struct device *parent, struct hw_pci *hw)
+void pci_common_init(struct hw_pci *hw)
{
struct pci_sys_data *sys;
LIST_HEAD(head);
@@ -519,7 +518,7 @@ void pci_common_init_dev(struct device *parent, struct hw_pci *hw)
pci_add_flags(PCI_REASSIGN_ALL_RSRC);
if (hw->preinit)
hw->preinit();
- pcibios_init_hw(parent, hw, &head);
+ pcibios_init_hw(hw, &head);
if (hw->postinit)
hw->postinit();

@@ -559,6 +558,100 @@ void pci_common_init_dev(struct device *parent, struct hw_pci *hw)
}
}

+int pci_init_single(struct device *parent,
+ struct pci_sys_data *sys,
+ struct pci_bus *(*scan)(int nr, struct pci_sys_data *),
+ struct pci_ops *ops)
+{
+ int ret;
+ struct pci_bus *bus;
+
+ ret = pcibios_init_resources(0, sys);
+ if (ret)
+ return ret;
+
+ if (scan)
+ bus = scan(0, sys);
+ else
+ bus = pci_scan_root_bus(parent, 0, ops, sys, &sys->resources);
+
+ if (!bus) {
+ dev_err(parent, "PCI: unable to scan bus!");
+ return -ENXIO;
+ }
+ sys->bus = bus;
+
+ pci_fixup_irqs(pcibios_swizzle, pcibios_map_irq);
+
+ if (!pci_has_flag(PCI_PROBE_ONLY)) {
+ /*
+ * Size the bridge windows.
+ */
+ pci_bus_size_bridges(bus);
+
+ /*
+ * Assign resources.
+ */
+ pci_bus_assign_resources(bus);
+ }
+
+ /*
+ * Tell drivers about devices found.
+ */
+ pci_bus_add_devices(bus);
+
+ /* Configure PCI Express settings */
+ if (bus && !pci_has_flag(PCI_PROBE_ONLY)) {
+ struct pci_bus *child;
+
+ list_for_each_entry(child, &bus->children, node)
+ pcie_bus_configure_settings(child);
+ }
+
+ return 0;
+}
+
+int pci_common_init_dev(struct device *parent, struct hw_pci *hw)
+{
+ struct pci_sys_data *sys;
+ int ret;
+
+ if (hw->nr_controllers != 1 ||
+ hw->preinit || hw->postinit)
+ return -EINVAL;
+
+ sys = kzalloc(sizeof(struct pci_sys_data), GFP_KERNEL);
+ if (!sys)
+ return -ENOMEM;
+
+#ifdef CONFIG_PCI_DOMAINS
+ sys->domain = hw->domain;
+#endif
+ sys->swizzle = hw->swizzle;
+ sys->map_irq = hw->map_irq;
+ sys->align_resource = hw->align_resource;
+ sys->add_bus = hw->add_bus;
+ sys->remove_bus = hw->remove_bus;
+ INIT_LIST_HEAD(&sys->resources);
+
+ if (hw->private_data)
+ sys->private_data = hw->private_data[0];
+
+ pci_add_flags(PCI_REASSIGN_ALL_RSRC);
+ ret = hw->setup(0, sys);
+ if (ret == 0)
+ ret = -ENXIO;
+ if (ret < 0)
+ return ret;
+
+ ret = pcibios_init_sysdata(parent, sys, hw->scan, hw->ops);
+ if (ret)
+ /* FIXME: undo ->setup */
+ kfree(sys);
+
+ return ret;
+}
+
#ifndef CONFIG_PCI_HOST_ITE8152
void pcibios_set_master(struct pci_dev *dev)
{
diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
index 3d2076f59911..3542a7b740e5 100644
--- a/drivers/pci/host/pci-host-generic.c
+++ b/drivers/pci/host/pci-host-generic.c
@@ -40,16 +40,20 @@ struct gen_pci_cfg_windows {

struct gen_pci {
struct pci_host_bridge host;
+ struct pci_sys_data sys;
struct gen_pci_cfg_windows cfg;
- struct list_head resources;
};

+static inline struct gen_pci *gen_pci_from_sys(struct pci_sys_data *sys)
+{
+ return container_of(sys, struct gen_pci, sys);
+}
+
static void __iomem *gen_pci_map_cfg_bus_cam(struct pci_bus *bus,
unsigned int devfn,
int where)
{
- struct pci_sys_data *sys = bus->sysdata;
- struct gen_pci *pci = sys->private_data;
+ struct gen_pci *pci = gen_pci_from_sys(bus->sysdata);
resource_size_t idx = bus->number - pci->cfg.bus_range.start;

return pci->cfg.win[idx] + ((devfn << 8) | where);
@@ -64,8 +68,7 @@ static void __iomem *gen_pci_map_cfg_bus_ecam(struct pci_bus *bus,
unsigned int devfn,
int where)
{
- struct pci_sys_data *sys = bus->sysdata;
- struct gen_pci *pci = sys->private_data;
+ struct gen_pci *pci = gen_pci_from_sys(bus->sysdata);
resource_size_t idx = bus->number - pci->cfg.bus_range.start;

return pci->cfg.win[idx] + ((devfn << 12) | where);
@@ -80,8 +83,7 @@ static int gen_pci_config_read(struct pci_bus *bus, unsigned int devfn,
int where, int size, u32 *val)
{
void __iomem *addr;
- struct pci_sys_data *sys = bus->sysdata;
- struct gen_pci *pci = sys->private_data;
+ struct gen_pci *pci = gen_pci_from_sys(bus->sysdata);

addr = pci->cfg.ops->map_bus(bus, devfn, where);

@@ -103,8 +105,7 @@ static int gen_pci_config_write(struct pci_bus *bus, unsigned int devfn,
int where, int size, u32 val)
{
void __iomem *addr;
- struct pci_sys_data *sys = bus->sysdata;
- struct gen_pci *pci = sys->private_data;
+ struct gen_pci *pci = gen_pci_from_sys(bus->sysdata);

addr = pci->cfg.ops->map_bus(bus, devfn, where);

@@ -181,10 +182,10 @@ static void gen_pci_release_of_pci_ranges(struct gen_pci *pci)
{
struct pci_host_bridge_window *win;

- list_for_each_entry(win, &pci->resources, list)
+ list_for_each_entry(win, &pci->sys.resources, list)
release_resource(win->res);

- pci_free_resource_list(&pci->resources);
+ pci_free_resource_list(&pci->sys.resources);
}

static int gen_pci_parse_request_of_pci_ranges(struct gen_pci *pci)
@@ -237,7 +238,7 @@ static int gen_pci_parse_request_of_pci_ranges(struct gen_pci *pci)
if (err)
goto out_release_res;

- pci_add_resource_offset(&pci->resources, res, offset);
+ pci_add_resource_offset(&pci->sys.resources, res, offset);
}

if (!res_valid) {
@@ -306,17 +307,10 @@ static int gen_pci_parse_map_cfg_windows(struct gen_pci *pci)
}

/* Register bus resource */
- pci_add_resource(&pci->resources, bus_range);
+ pci_add_resource(&pci->sys.resources, bus_range);
return 0;
}

-static int gen_pci_setup(int nr, struct pci_sys_data *sys)
-{
- struct gen_pci *pci = sys->private_data;
- list_splice_init(&pci->resources, &sys->resources);
- return 1;
-}
-
static int gen_pci_probe(struct platform_device *pdev)
{
int err;
@@ -326,17 +320,12 @@ static int gen_pci_probe(struct platform_device *pdev)
struct device *dev = &pdev->dev;
struct device_node *np = dev->of_node;
struct gen_pci *pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);
- struct hw_pci hw = {
- .nr_controllers = 1,
- .private_data = (void **)&pci,
- .setup = gen_pci_setup,
- .map_irq = of_irq_parse_and_map_pci,
- .ops = &gen_pci_ops,
- };

if (!pci)
return -ENOMEM;

+ pci->sys.map_irq = of_irq_parse_and_map_pci,
+
type = of_get_property(np, "device_type", NULL);
if (!type || strcmp(type, "pci")) {
dev_err(dev, "invalid \"device_type\" %s\n", type);
@@ -355,7 +344,7 @@ static int gen_pci_probe(struct platform_device *pdev)
pci->cfg.ops = of_id->data;
pci->host.dev.parent = dev;
INIT_LIST_HEAD(&pci->host.windows);
- INIT_LIST_HEAD(&pci->resources);
+ INIT_LIST_HEAD(&pci->sys.resources);

/* Parse our PCI ranges and request their resources */
err = gen_pci_parse_request_of_pci_ranges(pci);
@@ -369,8 +358,12 @@ static int gen_pci_probe(struct platform_device *pdev)
return err;
}

- pci_common_init_dev(dev, &hw);
- return 0;
+ pci_add_flags(PCI_REASSIGN_ALL_RSRC);
+ err = pci_init_single(dev, &pci->sys, NULL, &gen_pci_ops);
+ if (err)
+ gen_pci_release_of_pci_ranges(pci);
+
+ return err;
}

static struct platform_driver gen_pci_driver = {
diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
index b1315e197ffb..e1381c0699be 100644
--- a/drivers/pci/host/pci-mvebu.c
+++ b/drivers/pci/host/pci-mvebu.c
@@ -99,6 +99,7 @@ struct mvebu_pcie_port;
struct mvebu_pcie {
struct platform_device *pdev;
struct mvebu_pcie_port *ports;
+ struct pci_sys_data sysdata;
struct msi_chip *msi;
struct resource io;
char io_name[30];
@@ -611,7 +612,7 @@ static int mvebu_sw_pci_bridge_write(struct mvebu_pcie_port *port,

static inline struct mvebu_pcie *sys_to_pcie(struct pci_sys_data *sys)
{
- return sys->private_data;
+ return container_of(sys, struct mvebu_pcie, sysdata);
}

static struct mvebu_pcie_port *mvebu_pcie_find_port(struct mvebu_pcie *pcie,
@@ -718,11 +719,26 @@ static struct pci_ops mvebu_pcie_ops = {
.write = mvebu_pcie_wr_conf,
};

-static int mvebu_pcie_setup(int nr, struct pci_sys_data *sys)
+/* FIXME: move the code around to avoid these */
+static struct pci_bus *mvebu_pcie_scan_bus(int nr, struct pci_sys_data *sys);
+static void mvebu_pcie_add_bus(struct pci_bus *bus);
+static resource_size_t mvebu_pcie_align_resource(struct pci_dev *dev,
+ const struct resource *res,
+ resource_size_t start,
+ resource_size_t size,
+ resource_size_t align);
+
+static int mvebu_pcie_enable(struct mvebu_pcie *pcie)
{
- struct mvebu_pcie *pcie = sys_to_pcie(sys);
int i;
int domain = 0;
+ struct pci_sys_data *sys = &pcie->sysdata;
+
+ pcie->sysdata = (struct pci_sys_data) {
+ .map_irq = of_irq_parse_and_map_pci,
+ .align_resource = mvebu_pcie_align_resource,
+ .add_bus = mvebu_pcie_add_bus,
+ };

#ifdef CONFIG_PCI_DOMAINS
domain = sys->domain;
@@ -738,11 +754,13 @@ static int mvebu_pcie_setup(int nr, struct pci_sys_data *sys)
if (request_resource(&iomem_resource, &pcie->mem))
return 0;

+ INIT_LIST_HEAD(&sys->resources);
if (resource_size(&pcie->realio) != 0) {
if (request_resource(&ioport_resource, &pcie->realio)) {
release_resource(&pcie->mem);
return 0;
}
+
pci_add_resource_offset(&sys->resources, &pcie->realio,
sys->io_offset);
}
@@ -756,7 +774,9 @@ static int mvebu_pcie_setup(int nr, struct pci_sys_data *sys)
mvebu_pcie_setup_hw(port);
}

- return 1;
+ pci_add_flags(PCI_REASSIGN_ALL_RSRC);
+ return pci_init_single(&pcie->pdev->dev, &pcie->sysdata,
+ mvebu_pcie_scan_bus, &mvebu_pcie_ops);
}

static struct pci_bus *mvebu_pcie_scan_bus(int nr, struct pci_sys_data *sys)
@@ -810,24 +830,6 @@ static resource_size_t mvebu_pcie_align_resource(struct pci_dev *dev,
return start;
}

-static void mvebu_pcie_enable(struct mvebu_pcie *pcie)
-{
- struct hw_pci hw;
-
- memset(&hw, 0, sizeof(hw));
-
- hw.nr_controllers = 1;
- hw.private_data = (void **)&pcie;
- hw.setup = mvebu_pcie_setup;
- hw.scan = mvebu_pcie_scan_bus;
- hw.map_irq = of_irq_parse_and_map_pci;
- hw.ops = &mvebu_pcie_ops;
- hw.align_resource = mvebu_pcie_align_resource;
- hw.add_bus = mvebu_pcie_add_bus;
-
- pci_common_init(&hw);
-}
-
/*
* Looks up the list of register addresses encoded into the reg =
* <...> property for one that matches the given port/lane. Once
@@ -1066,9 +1068,7 @@ static int mvebu_pcie_probe(struct platform_device *pdev)
pci_ioremap_io(i, pcie->io.start + i);

mvebu_pcie_msi_enable(pcie);
- mvebu_pcie_enable(pcie);
-
- return 0;
+ return mvebu_pcie_enable(pcie);
}

static const struct of_device_id mvebu_pcie_of_match_table[] = {

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

Liviu Dudau

2014-10-24 10:04:20 UTC

Post by Arnd Bergmann
@@ -335,7 +329,9 @@ void __init cns3xxx_pcie_init_late(void)
cns3xxx_pwr_soft_rst(0x1 << PM_SOFT_RST_REG_OFFST_P=

CIE(i));

Post by Arnd Bergmann
cns3xxx_pcie_check_link(&cns3xxx_pcie[i]);
cns3xxx_pcie_hw_init(&cns3xxx_pcie[i]);
- pci_common_init(&cns3xxx_pcie[i].hw_pci);
+ hw_pci->domain =3D i;
+ private_data =3D &cns3xxx_pcie[i];

Is this dance with pointers absolutely necessary? Does gcc though d=

ishes at you

for doing hw_pci->private_data =3D &cns3xxx_pcie[i] directly?

=20
hw_pci->private_data is an array of pointers to private_data for each
host controller instance within the domain. There is only one entry
here, but you still the the correct type, so that would be
=20
hw_pci->private_data =3D (void **)&&cns3xxx_pcie[i];
=20
which is even more confusing and ugly than what I wrote. If you have
a better idea, I'm all for it. Maybe it's clearer to write like this
(taken from rcar driver)?

I was not questioning the implementation (and I do like it) I was just
wondering if it is to work around some quirk of gcc. As I did not had
the time to test it I was hoping that if there is a gcc requirement to
phrase the code that way you might provide me with an answer.

Best regards,
Liviu

Post by Arnd Bergmann
=20
void *hw_private[1];
hw_pci.private_data =3D hw_private;
=20
for each host {
...
hw_private[0] =3D &cns3xxx_pcie[i];
pci_common_init_dev(&hw_pci);
}
=20
Note that all 'modern' controllers always use nr_controllers=3D1, so =

Post by Arnd Bergmann
only need a single private_data pointer per domain, and the entire
hw_pci interface is a bit pointless.
=20
The platforms that currently require it are iop13xx, dove, mv78xx0
and orion5x. We have plans to remove the last three platforms in
the next merge window or two, once all users are able to migrate to
mach-mvebu. Once that happens, we could probably move the entire
hw_pci logic that deals with multiple hosts per domain into the
iop13xx pci driver if we want to. A less intrusive simplification
would be to convert all 'multiplatform'-aware host controllers to
use pci_common_init_dev() and then take hw_pci out of that.
=20
See below for a sample patch I just did. It duplicates the code from
pci_common_init_dev/pci_common_init because we know that all users
of pci_common_init_dev are modern and only pass a single host bridge.
The new pci_common_init_dev is simpler than the old one but should
do the exact same thing for all current users, with the addition
of propagating the return value.
=20
pci_init_single() is the new internal helper and we should be able to
convert all existing users of pci_common_init_dev() to use that direc=

tly

Post by Arnd Bergmann
and no longer define hw_pci at all.
=20
I've converted two drivers to give an example, but the conversions
should be done in follow-up patches really, and the pci_common_init_d=

Post by Arnd Bergmann
function removed after all users are moved over.
=20
The new pci_init_single() is also rather simple, and it should just
converge with what we do for arm64 over time.
=20
Arnd
=20
---
arch/arm/include/asm/mach/pci.h | 20 ++++---
arch/arm/kernel/bios32.c | 103 ++++++++++++++++++++++++++=

++++++++--

Post by Arnd Bergmann
drivers/pci/host/pci-host-generic.c | 53 ++++++++-----------
drivers/pci/host/pci-mvebu.c | 44 +++++++--------
4 files changed, 157 insertions(+), 63 deletions(-)
=20
diff --git a/arch/arm/include/asm/mach/pci.h b/arch/arm/include/asm/m=

ach/pci.h

Post by Arnd Bergmann
index 7fc42784becb..fe7e13759ec0 100644
--- a/arch/arm/include/asm/mach/pci.h
+++ b/arch/arm/include/asm/mach/pci.h
@@ -73,16 +73,22 @@ struct pci_sys_data {
/*
* Call this with your hw_pci struct to initialise the PCI system.
*/
-void pci_common_init_dev(struct device *, struct hw_pci *);
+void pci_common_init(struct hw_pci *);
=20
/*
- * Compatibility wrapper for older platforms that do not care about
- * passing the parent device.
+ * Used by modern platforms, only one host allowed.
*/
-static inline void pci_common_init(struct hw_pci *hw)
-{
- pci_common_init_dev(NULL, hw);
-}
+int pci_common_init_dev(struct device *, struct hw_pci *);
+
+/*
+ * Replaces pci_common_init_dev for drivers that want to do the
+ * initialization simpler and avoid defining hw_pci
+ */
+int pci_init_single(struct device *parent,
+ struct pci_sys_data *sys,
+ struct pci_bus *(*scan)(int nr, struct pci_sys_da=

ta *),

Post by Arnd Bergmann
+ struct pci_ops *ops);
+
=20
/*
* Setup early fixed I/O mapping.
diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
index 17a26c17f7f5..bccc8703e575 100644
--- a/arch/arm/kernel/bios32.c
+++ b/arch/arm/kernel/bios32.c
@@ -456,8 +456,7 @@ static int pcibios_init_resources(int busnr, stru=

ct pci_sys_data *sys)

Post by Arnd Bergmann
return 0;
}
=20
-static void pcibios_init_hw(struct device *parent, struct hw_pci *hw=

Post by Arnd Bergmann
- struct list_head *head)
+static void pcibios_init_hw(struct hw_pci *hw, struct list_head *hea=

Post by Arnd Bergmann
{
struct pci_sys_data *sys =3D NULL;
int ret;
@@ -494,7 +493,7 @@ static void pcibios_init_hw(struct device *parent=

, struct hw_pci *hw,

Post by Arnd Bergmann
if (hw->scan)
sys->bus =3D hw->scan(nr, sys);
else
- sys->bus =3D pci_scan_root_bus(parent=

, sys->busnr,

Post by Arnd Bergmann
+ sys->bus =3D pci_scan_root_bus(NULL, =

sys->busnr,

Post by Arnd Bergmann
hw->ops, sys, &sys->r=

esources);

Post by Arnd Bergmann
=20
if (!sys->bus)
@@ -511,7 +510,7 @@ static void pcibios_init_hw(struct device *parent=

, struct hw_pci *hw,

Post by Arnd Bergmann
}
}
=20
-void pci_common_init_dev(struct device *parent, struct hw_pci *hw)
+void pci_common_init(struct hw_pci *hw)
{
struct pci_sys_data *sys;
LIST_HEAD(head);
@@ -519,7 +518,7 @@ void pci_common_init_dev(struct device *parent, s=

truct hw_pci *hw)

Post by Arnd Bergmann
pci_add_flags(PCI_REASSIGN_ALL_RSRC);
if (hw->preinit)
hw->preinit();
- pcibios_init_hw(parent, hw, &head);
+ pcibios_init_hw(hw, &head);
if (hw->postinit)
hw->postinit();
=20
@@ -559,6 +558,100 @@ void pci_common_init_dev(struct device *parent,=

struct hw_pci *hw)

Post by Arnd Bergmann
}
}
=20
+int pci_init_single(struct device *parent,
+ struct pci_sys_data *sys,
+ struct pci_bus *(*scan)(int nr, struct pci_sys_da=

ta *),

Post by Arnd Bergmann
+ struct pci_ops *ops)
+{
+ int ret;
+ struct pci_bus *bus;
+
+ ret =3D pcibios_init_resources(0, sys);
+ if (ret)
+ return ret;
+
+ if (scan)
+ bus =3D scan(0, sys);
+ else
+ bus =3D pci_scan_root_bus(parent, 0, ops, sys, &sys->=

resources);

Post by Arnd Bergmann
+
+ if (!bus) {
+ dev_err(parent, "PCI: unable to scan bus!");
+ return -ENXIO;
+ }
+ sys->bus =3D bus;
+
+ pci_fixup_irqs(pcibios_swizzle, pcibios_map_irq);
+
+ if (!pci_has_flag(PCI_PROBE_ONLY)) {
+ /*
+ * Size the bridge windows.
+ */
+ pci_bus_size_bridges(bus);
+
+ /*
+ * Assign resources.
+ */
+ pci_bus_assign_resources(bus);
+ }
+
+ /*
+ * Tell drivers about devices found.
+ */
+ pci_bus_add_devices(bus);
+
+ /* Configure PCI Express settings */
+ if (bus && !pci_has_flag(PCI_PROBE_ONLY)) {
+ struct pci_bus *child;
+
+ list_for_each_entry(child, &bus->children, node)
+ pcie_bus_configure_settings(child);
+ }
+
+ return 0;
+}
+
+int pci_common_init_dev(struct device *parent, struct hw_pci *hw)
+{
+ struct pci_sys_data *sys;
+ int ret;
+
+ if (hw->nr_controllers !=3D 1 ||
+ hw->preinit || hw->postinit)
+ return -EINVAL;
+
+ sys =3D kzalloc(sizeof(struct pci_sys_data), GFP_KERNEL);
+ if (!sys)
+ return -ENOMEM;
+
+#ifdef CONFIG_PCI_DOMAINS
+ sys->domain =3D hw->domain;
+#endif
+ sys->swizzle =3D hw->swizzle;
+ sys->map_irq =3D hw->map_irq;
+ sys->align_resource =3D hw->align_resource;
+ sys->add_bus =3D hw->add_bus;
+ sys->remove_bus =3D hw->remove_bus;
+ INIT_LIST_HEAD(&sys->resources);
+
+ if (hw->private_data)
+ sys->private_data =3D hw->private_data[0];
+
+ pci_add_flags(PCI_REASSIGN_ALL_RSRC);
+ ret =3D hw->setup(0, sys);
+ if (ret =3D=3D 0)
+ ret =3D -ENXIO;
+ if (ret < 0)
+ return ret;
+
+ ret =3D pcibios_init_sysdata(parent, sys, hw->scan, hw->ops);
+ if (ret)
+ /* FIXME: undo ->setup */
+ kfree(sys);
+
+ return ret;
+}
+
#ifndef CONFIG_PCI_HOST_ITE8152
void pcibios_set_master(struct pci_dev *dev)
{
diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/p=

ci-host-generic.c

Post by Arnd Bergmann
index 3d2076f59911..3542a7b740e5 100644
--- a/drivers/pci/host/pci-host-generic.c
+++ b/drivers/pci/host/pci-host-generic.c
@@ -40,16 +40,20 @@ struct gen_pci_cfg_windows {
=20
struct gen_pci {
struct pci_host_bridge host;
+ struct pci_sys_data sys;
struct gen_pci_cfg_windows cfg;
- struct list_head resources;
};
=20
+static inline struct gen_pci *gen_pci_from_sys(struct pci_sys_data *=

sys)

Post by Arnd Bergmann
+{
+ return container_of(sys, struct gen_pci, sys);
+}
+
static void __iomem *gen_pci_map_cfg_bus_cam(struct pci_bus *bus,
unsigned int devfn,
int where)
{
- struct pci_sys_data *sys =3D bus->sysdata;
- struct gen_pci *pci =3D sys->private_data;
+ struct gen_pci *pci =3D gen_pci_from_sys(bus->sysdata);
resource_size_t idx =3D bus->number - pci->cfg.bus_range.star=

Post by Arnd Bergmann
=20
return pci->cfg.win[idx] + ((devfn << 8) | where);
@@ -64,8 +68,7 @@ static void __iomem *gen_pci_map_cfg_bus_ecam(struc=

t pci_bus *bus,

Post by Arnd Bergmann
unsigned int devfn,
int where)
{
- struct pci_sys_data *sys =3D bus->sysdata;
- struct gen_pci *pci =3D sys->private_data;
+ struct gen_pci *pci =3D gen_pci_from_sys(bus->sysdata);
resource_size_t idx =3D bus->number - pci->cfg.bus_range.star=

Post by Arnd Bergmann
=20
return pci->cfg.win[idx] + ((devfn << 12) | where);
@@ -80,8 +83,7 @@ static int gen_pci_config_read(struct pci_bus *bus,=

unsigned int devfn,

Post by Arnd Bergmann
int where, int size, u32 *val)
{
void __iomem *addr;
- struct pci_sys_data *sys =3D bus->sysdata;
- struct gen_pci *pci =3D sys->private_data;
+ struct gen_pci *pci =3D gen_pci_from_sys(bus->sysdata);
=20
addr =3D pci->cfg.ops->map_bus(bus, devfn, where);
=20
@@ -103,8 +105,7 @@ static int gen_pci_config_write(struct pci_bus *b=

us, unsigned int devfn,

Post by Arnd Bergmann
int where, int size, u32 val)
{
void __iomem *addr;
- struct pci_sys_data *sys =3D bus->sysdata;
- struct gen_pci *pci =3D sys->private_data;
+ struct gen_pci *pci =3D gen_pci_from_sys(bus->sysdata);
=20
addr =3D pci->cfg.ops->map_bus(bus, devfn, where);
=20
@@ -181,10 +182,10 @@ static void gen_pci_release_of_pci_ranges(struc=

t gen_pci *pci)

Post by Arnd Bergmann
{
struct pci_host_bridge_window *win;
=20
- list_for_each_entry(win, &pci->resources, list)
+ list_for_each_entry(win, &pci->sys.resources, list)
release_resource(win->res);
=20
- pci_free_resource_list(&pci->resources);
+ pci_free_resource_list(&pci->sys.resources);
}
=20
static int gen_pci_parse_request_of_pci_ranges(struct gen_pci *pci)
@@ -237,7 +238,7 @@ static int gen_pci_parse_request_of_pci_ranges(st=

ruct gen_pci *pci)

Post by Arnd Bergmann
if (err)
goto out_release_res;
=20
- pci_add_resource_offset(&pci->resources, res, offset)=

;

Post by Arnd Bergmann
+ pci_add_resource_offset(&pci->sys.resources, res, off=

set);

Post by Arnd Bergmann
}
=20
if (!res_valid) {
@@ -306,17 +307,10 @@ static int gen_pci_parse_map_cfg_windows(struct=

gen_pci *pci)

Post by Arnd Bergmann
}
=20
/* Register bus resource */
- pci_add_resource(&pci->resources, bus_range);
+ pci_add_resource(&pci->sys.resources, bus_range);
return 0;
}
=20
-static int gen_pci_setup(int nr, struct pci_sys_data *sys)
-{
- struct gen_pci *pci =3D sys->private_data;
- list_splice_init(&pci->resources, &sys->resources);
- return 1;
-}
-
static int gen_pci_probe(struct platform_device *pdev)
{
int err;
@@ -326,17 +320,12 @@ static int gen_pci_probe(struct platform_device=

*pdev)

Post by Arnd Bergmann
struct device *dev =3D &pdev->dev;
struct device_node *np =3D dev->of_node;
struct gen_pci *pci =3D devm_kzalloc(dev, sizeof(*pci), GFP_K=

ERNEL);

Post by Arnd Bergmann
- struct hw_pci hw =3D {
- .nr_controllers =3D 1,
- .private_data =3D (void **)&pci,
- .setup =3D gen_pci_setup,
- .map_irq =3D of_irq_parse_and_map_pci,
- .ops =3D &gen_pci_ops,
- };
=20
if (!pci)
return -ENOMEM;
=20
+ pci->sys.map_irq =3D of_irq_parse_and_map_pci,
+
type =3D of_get_property(np, "device_type", NULL);
if (!type || strcmp(type, "pci")) {
dev_err(dev, "invalid \"device_type\" %s\n", type);
@@ -355,7 +344,7 @@ static int gen_pci_probe(struct platform_device *=

pdev)

Post by Arnd Bergmann
pci->cfg.ops =3D of_id->data;
pci->host.dev.parent =3D dev;
INIT_LIST_HEAD(&pci->host.windows);
- INIT_LIST_HEAD(&pci->resources);
+ INIT_LIST_HEAD(&pci->sys.resources);
=20
/* Parse our PCI ranges and request their resources */
err =3D gen_pci_parse_request_of_pci_ranges(pci);
@@ -369,8 +358,12 @@ static int gen_pci_probe(struct platform_device =

*pdev)

Post by Arnd Bergmann
return err;
}
=20
- pci_common_init_dev(dev, &hw);
- return 0;
+ pci_add_flags(PCI_REASSIGN_ALL_RSRC);
+ err =3D pci_init_single(dev, &pci->sys, NULL, &gen_pci_ops);
+ if (err)
+ gen_pci_release_of_pci_ranges(pci);
+
+ return err;
}
=20
static struct platform_driver gen_pci_driver =3D {
diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mveb=

u.c

Post by Arnd Bergmann
index b1315e197ffb..e1381c0699be 100644
--- a/drivers/pci/host/pci-mvebu.c
+++ b/drivers/pci/host/pci-mvebu.c
@@ -99,6 +99,7 @@ struct mvebu_pcie_port;
struct mvebu_pcie {
struct platform_device *pdev;
struct mvebu_pcie_port *ports;
+ struct pci_sys_data sysdata;
struct msi_chip *msi;
struct resource io;
char io_name[30];
@@ -611,7 +612,7 @@ static int mvebu_sw_pci_bridge_write(struct mvebu=

_pcie_port *port,

Post by Arnd Bergmann
=20
static inline struct mvebu_pcie *sys_to_pcie(struct pci_sys_data *sy=

Post by Arnd Bergmann
{
- return sys->private_data;
+ return container_of(sys, struct mvebu_pcie, sysdata);
}
=20
static struct mvebu_pcie_port *mvebu_pcie_find_port(struct mvebu_pci=

e *pcie,

Post by Arnd Bergmann
@@ -718,11 +719,26 @@ static struct pci_ops mvebu_pcie_ops =3D {
.write =3D mvebu_pcie_wr_conf,
};
=20
-static int mvebu_pcie_setup(int nr, struct pci_sys_data *sys)
+/* FIXME: move the code around to avoid these */
+static struct pci_bus *mvebu_pcie_scan_bus(int nr, struct pci_sys_da=

ta *sys);

Post by Arnd Bergmann
+static void mvebu_pcie_add_bus(struct pci_bus *bus);
+static resource_size_t mvebu_pcie_align_resource(struct pci_dev *dev=

Post by Arnd Bergmann
+ const struct resourc=

e *res,

Post by Arnd Bergmann
+ resource_size_t star=

Post by Arnd Bergmann
+ resource_size_t size=

Post by Arnd Bergmann
+ resource_size_t alig=

n);

Post by Arnd Bergmann
+
+static int mvebu_pcie_enable(struct mvebu_pcie *pcie)
{
- struct mvebu_pcie *pcie =3D sys_to_pcie(sys);
int i;
int domain =3D 0;
+ struct pci_sys_data *sys =3D &pcie->sysdata;
+
+ pcie->sysdata =3D (struct pci_sys_data) {
+ .map_irq =3D of_irq_parse_and_map_pci,
+ .align_resource =3D mvebu_pcie_align_resource,
+ .add_bus =3D mvebu_pcie_add_bus,
+ };
=20
#ifdef CONFIG_PCI_DOMAINS
domain =3D sys->domain;
@@ -738,11 +754,13 @@ static int mvebu_pcie_setup(int nr, struct pci_=

sys_data *sys)

Post by Arnd Bergmann
if (request_resource(&iomem_resource, &pcie->mem))
return 0;
=20
+ INIT_LIST_HEAD(&sys->resources);
if (resource_size(&pcie->realio) !=3D 0) {
if (request_resource(&ioport_resource, &pcie->realio)=

) {

Post by Arnd Bergmann
release_resource(&pcie->mem);
return 0;
}
+
pci_add_resource_offset(&sys->resources, &pcie->reali=

Post by Arnd Bergmann
sys->io_offset);
}
@@ -756,7 +774,9 @@ static int mvebu_pcie_setup(int nr, struct pci_sy=

s_data *sys)

Post by Arnd Bergmann
mvebu_pcie_setup_hw(port);
}
=20
- return 1;
+ pci_add_flags(PCI_REASSIGN_ALL_RSRC);
+ return pci_init_single(&pcie->pdev->dev, &pcie->sysdata,
+ mvebu_pcie_scan_bus, &mvebu_pcie_ops);
}
=20
static struct pci_bus *mvebu_pcie_scan_bus(int nr, struct pci_sys_da=

ta *sys)

Post by Arnd Bergmann
@@ -810,24 +830,6 @@ static resource_size_t mvebu_pcie_align_resource=

(struct pci_dev *dev,

Post by Arnd Bergmann
return start;
}
=20
-static void mvebu_pcie_enable(struct mvebu_pcie *pcie)
-{
- struct hw_pci hw;
-
- memset(&hw, 0, sizeof(hw));
-
- hw.nr_controllers =3D 1;
- hw.private_data =3D (void **)&pcie;
- hw.setup =3D mvebu_pcie_setup;
- hw.scan =3D mvebu_pcie_scan_bus;
- hw.map_irq =3D of_irq_parse_and_map_pci;
- hw.ops =3D &mvebu_pcie_ops;
- hw.align_resource =3D mvebu_pcie_align_resource;
- hw.add_bus =3D mvebu_pcie_add_bus;
-
- pci_common_init(&hw);
-}
-
/*
* Looks up the list of register addresses encoded into the reg =3D
* <...> property for one that matches the given port/lane. Once
@@ -1066,9 +1068,7 @@ static int mvebu_pcie_probe(struct platform_dev=

ice *pdev)

Post by Arnd Bergmann
pci_ioremap_io(i, pcie->io.start + i);
=20
mvebu_pcie_msi_enable(pcie);
- mvebu_pcie_enable(pcie);
-
- return 0;
+ return mvebu_pcie_enable(pcie);
}
=20
static const struct of_device_id mvebu_pcie_of_match_table[] =3D {
=20
=20
=20

Sunil Kovvuri

2014-09-29 19:19:01 UTC

How does the res->parent for devices of non-"PCI_PROBE_ONLY" controller
gets set with this patch ?

Post by s***@amd.com
This patch adds ARM64 support to the generic PCI host driver.
For MSI support, it adds new device tree binding "msi-parent",
which should point to corresponded msi-controller.
---
.../devicetree/bindings/pci/host-generic-pci.txt | 3 +
drivers/pci/host/Kconfig | 2 +-
drivers/pci/host/pci-host-generic.c | 95 ++++++++++++++++++++--
3 files changed, 90 insertions(+), 10 deletions(-)
diff --git a/Documentation/devicetree/bindings/pci/host-generic-pci.txt b/Documentation/devicetree/bindings/pci/host-generic-pci.txt
index f0b0436..327e5b1 100644
--- a/Documentation/devicetree/bindings/pci/host-generic-pci.txt
+++ b/Documentation/devicetree/bindings/pci/host-generic-pci.txt
- interrupt-map-mask : <see aforementioned specification>
+
+- msi-parent : Specify the msi-controller phandle.
diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
index 90f5cca..44bf523 100644
--- a/drivers/pci/host/Kconfig
+++ b/drivers/pci/host/Kconfig
@@ -50,7 +50,7 @@ config PCI_RCAR_GEN2_PCIE
config PCI_HOST_GENERIC
bool "Generic PCI host controller"
- depends on ARM && OF
+ depends on (ARM || ARM64) && OF
help
Say Y here if you want to support a simple generic PCI host
controller, such as the one emulated by kvmtool.
diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
index 3d2076f..f33c547 100644
--- a/drivers/pci/host/pci-host-generic.c
+++ b/drivers/pci/host/pci-host-generic.c
@@ -42,14 +42,24 @@ struct gen_pci {
struct pci_host_bridge host;
struct gen_pci_cfg_windows cfg;
struct list_head resources;
+ struct device_node *msi_parent;
};
+#ifdef CONFIG_ARM64
+#define bus_to_gen_pci(b) \
+ ((struct gen_pci *)b->sysdata)
+#else
+#define bus_to_gen_pci(b) \
+ ((struct gen_pci *) \
+ (((struct pci_sys_data *) \
+ (bus->sysdata))->private_data))
+#endif
+
static void __iomem *gen_pci_map_cfg_bus_cam(struct pci_bus *bus,
unsigned int devfn,
int where)
{
- struct pci_sys_data *sys = bus->sysdata;
- struct gen_pci *pci = sys->private_data;
+ struct gen_pci *pci = bus_to_gen_pci(bus);
resource_size_t idx = bus->number - pci->cfg.bus_range.start;
return pci->cfg.win[idx] + ((devfn << 8) | where);
@@ -64,8 +74,7 @@ static void __iomem *gen_pci_map_cfg_bus_ecam(struct pci_bus *bus,
unsigned int devfn,
int where)
{
- struct pci_sys_data *sys = bus->sysdata;
- struct gen_pci *pci = sys->private_data;
+ struct gen_pci *pci = bus_to_gen_pci(bus);
resource_size_t idx = bus->number - pci->cfg.bus_range.start;
return pci->cfg.win[idx] + ((devfn << 12) | where);
@@ -80,8 +89,7 @@ static int gen_pci_config_read(struct pci_bus *bus, unsigned int devfn,
int where, int size, u32 *val)
{
void __iomem *addr;
- struct pci_sys_data *sys = bus->sysdata;
- struct gen_pci *pci = sys->private_data;
+ struct gen_pci *pci = bus_to_gen_pci(bus);
addr = pci->cfg.ops->map_bus(bus, devfn, where);
@@ -103,8 +111,7 @@ static int gen_pci_config_write(struct pci_bus *bus, unsigned int devfn,
int where, int size, u32 val)
{
void __iomem *addr;
- struct pci_sys_data *sys = bus->sysdata;
- struct gen_pci *pci = sys->private_data;
+ struct gen_pci *pci = bus_to_gen_pci(bus);
addr = pci->cfg.ops->map_bus(bus, devfn, where);
@@ -144,8 +151,11 @@ static int gen_pci_calc_io_offset(struct device *dev,
resource_size_t *offset)
{
static atomic_t wins = ATOMIC_INIT(0);
- int err, idx, max_win;
+ int idx, max_win;
unsigned int window;
+#ifndef CONFIG_ARM64
+ int err;
+#endif
if (!PAGE_ALIGNED(range->cpu_addr))
return -EINVAL;
@@ -156,9 +166,12 @@ static int gen_pci_calc_io_offset(struct device *dev,
return -ENOSPC;
window = (idx - 1) * SZ_64K;
+
+#ifndef CONFIG_ARM64
err = pci_ioremap_io(window, range->cpu_addr);
if (err)
return err;
+#endif
of_pci_range_to_resource(range, dev->of_node, res);
res->start = window;
@@ -310,12 +323,58 @@ static int gen_pci_parse_map_cfg_windows(struct gen_pci *pci)
return 0;
}
+#ifndef CONFIG_ARM64
static int gen_pci_setup(int nr, struct pci_sys_data *sys)
{
struct gen_pci *pci = sys->private_data;
list_splice_init(&pci->resources, &sys->resources);
return 1;
}
+#endif
+
+#ifdef CONFIG_ARM64
+struct pci_bus *gen_scan_root_bus(struct device *parent, int bus,
+ struct pci_ops *ops, void *sysdata,
+ struct list_head *resources)
+{
+ struct pci_host_bridge_window *window;
+ bool found = false;
+ struct pci_bus *b;
+ int max;
+ struct gen_pci *pci = sysdata;
+
+ list_for_each_entry(window, resources, list)
+ if (window->res->flags & IORESOURCE_BUS) {
+ found = true;
+ break;
+ }
+
+ b = pci_create_root_bus(parent, bus, ops, sysdata, resources);
+ if (!b)
+ return NULL;
+
+ * This is probably should be done in the core pci driver somewhere
+ */
+ if (pci->msi_parent)
+ b->msi = of_pci_find_msi_chip_by_node(pci->msi_parent);
+
+ if (!found) {
+ dev_info(&b->dev,
+ "No busn resource found for root bus, will use [bus %02x-ff]\n",
+ bus);
+ pci_bus_insert_busn_res(b, bus, 255);
+ }
+
+ max = pci_scan_child_bus(b);
+
+ if (!found)
+ pci_bus_update_busn_res_end(b, max);
+
+ pci_bus_add_devices(b);
+ return b;
+}
+#endif
static int gen_pci_probe(struct platform_device *pdev)
{
@@ -326,6 +385,7 @@ static int gen_pci_probe(struct platform_device *pdev)
struct device *dev = &pdev->dev;
struct device_node *np = dev->of_node;
struct gen_pci *pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);
+#ifndef CONFIG_ARM64
struct hw_pci hw = {
.nr_controllers = 1,
.private_data = (void **)&pci,
@@ -333,6 +393,7 @@ static int gen_pci_probe(struct platform_device *pdev)
.map_irq = of_irq_parse_and_map_pci,
.ops = &gen_pci_ops,
};
+#endif
if (!pci)
return -ENOMEM;
@@ -368,8 +429,24 @@ static int gen_pci_probe(struct platform_device *pdev)
gen_pci_release_of_pci_ranges(pci);
return err;
}
+#ifdef CONFIG_ARM64
+#ifdef CONFIG_PCI_MSI
+ pci->msi_parent = of_parse_phandle(np, "msi-parent", 0);
+ if (!pci->msi_parent) {
+ dev_err(&pdev->dev, "Failed to allocate msi-parent.\n");
+ return -EINVAL;
+ }
+#endif
+
+ if (!gen_scan_root_bus(&pdev->dev, pci->cfg.bus_range.start,
+ &gen_pci_ops, pci, &pci->resources)) {
+ dev_err(&pdev->dev, "failed to enable PCIe ports\n");
+ return -ENODEV;
+ }
+#else
pci_common_init_dev(dev, &hw);
+#endif /* CONFIG_ARM64 */
return 0;
}
--
1.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
More majordomo info at http://vger.kernel.org/majordomo-info.html

s***@amd.com

2014-09-28 20:53:29 UTC

From: Suravee Suthikulpanit <***@amd.com>

When specify PCI_PROBE_ONLY, the resource parent does not get assigned.
Therefore, pci_enable_resources() return error saying that
"BAR x not claimed".

Note: This same logic is also used in the arch/arm/kernel/bios32.c

Cc: Liviu Dudau <***@arm.com>
Cc: Bjorn Helgaas <***@google.com>
Cc: Will Deacon <***@arm.com>
Cc: Catalin Marinas <***@arm.com>
Signed-off-by: Suravee Suthikulpanit <***@amd.com>
---
arch/arm64/kernel/pci.c | 8 ++++++++
1 file changed, 8 insertions(+)

diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
index ce5836c..7fd4d2b 100644
--- a/arch/arm64/kernel/pci.c
+++ b/arch/arm64/kernel/pci.c
@@ -68,3 +68,11 @@ void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
bus->domain_nr = domain;
}
#endif
+
+int pcibios_enable_device(struct pci_dev *dev, int mask)
+{
+ if (pci_has_flag(PCI_PROBE_ONLY))
+ return 0;
+
+ return pci_enable_resources(dev, mask);
+}

--
1.9.3

Arnd Bergmann

2014-09-29 14:38:09 UTC

Post by s***@amd.com
diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
index ce5836c..7fd4d2b 100644
--- a/arch/arm64/kernel/pci.c
+++ b/arch/arm64/kernel/pci.c
@@ -68,3 +68,11 @@ void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
bus->domain_nr = domain;
}
#endif
+
+int pcibios_enable_device(struct pci_dev *dev, int mask)
+{
+ if (pci_has_flag(PCI_PROBE_ONLY))
+ return 0;
+
+ return pci_enable_resources(dev, mask);
+}

This doesn't look arm64 specific to me, and there is already a generic
pcibios_enable_device() function in drivers/pci/pci.c. Would it
be possible to move the check for PCI_PROBE_ONLY there without breaking
other architectures?

If that works, please do so.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Bjorn Helgaas

2014-09-29 18:17:29 UTC

Post by s***@amd.com
When specify PCI_PROBE_ONLY, the resource parent does not get assigned.
Therefore, pci_enable_resources() return error saying that
"BAR x not claimed".
Note: This same logic is also used in the arch/arm/kernel/bios32.c
---
arch/arm64/kernel/pci.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
index ce5836c..7fd4d2b 100644
--- a/arch/arm64/kernel/pci.c
+++ b/arch/arm64/kernel/pci.c
@@ -68,3 +68,11 @@ void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
bus->domain_nr = domain;
}
#endif
+
+int pcibios_enable_device(struct pci_dev *dev, int mask)
+{
+ if (pci_has_flag(PCI_PROBE_ONLY))
+ return 0;
+
+ return pci_enable_resources(dev, mask);

I had thought of "PCI_PROBE_ONLY" as the "look but don't touch" flag,
i.e., never change any BAR or bridge window assignments. But I guess
the current usage is more general than that: we also use it to

- avoid pci_enable_resources(), which only turns on IO/MEM bits in
the command register
- avoid pcie_bus_configure_settings(), which programs MPS and MRRS
- avoid pci_read_bridge_bases(), which really just *reads* bridge windows
- avoid pcibios_reserve_legacy_regions(), which doesn't touch any
PCI registers
- enables pci_claim_resource()

This seems like more than necessary, but I don't know all the history.
In particular, I don't know why PCI_PROBE_ONLY should make a
difference to things like claiming resources.

Post by s***@amd.com
+}
--
1.9.3
b

s***@public.gmane.org

2014-09-28 20:53:30 UTC

From: Suravee Suthikulpanit <Suravee.Suthikulpanit-***@public.gmane.org>

ARM GICv2m specification extends GICv2 to support MSI(-X) with
a new set of register frame. This patch introduces support for
the non-secure GICv2m register frame. Currently, GICV2m is available
in certain version of GIC-400.

The patch introduces a new property in ARM gic binding, the v2m subnode.
It is optional.

Cc: Marc Zyngier <Marc.Zyngier-***@public.gmane.org>
Cc: Mark Rutland <Mark.Rutland-***@public.gmane.org>
Cc: Thomas Gleixner <tglx-***@public.gmane.org>
Cc: Jason Cooper <jason-***@public.gmane.org>
Cc: Catalin Marinas <Catalin.Marinas-***@public.gmane.org>
Cc: Will Deacon <Will.Deacon-***@public.gmane.org>
Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit-***@public.gmane.org>
---
Documentation/devicetree/bindings/arm/gic.txt | 53 +++++
arch/arm64/Kconfig | 1 +
drivers/irqchip/Kconfig | 5 +
drivers/irqchip/Makefile | 1 +
drivers/irqchip/irq-gic-v2m.c | 301 ++++++++++++++++++++++++++
drivers/irqchip/irq-gic.c | 82 ++++---
drivers/irqchip/irq-gic.h | 54 +++++
7 files changed, 467 insertions(+), 30 deletions(-)
create mode 100644 drivers/irqchip/irq-gic-v2m.c
create mode 100644 drivers/irqchip/irq-gic.h

diff --git a/Documentation/devicetree/bindings/arm/gic.txt b/Documentation/devicetree/bindings/arm/gic.txt
index c7d2fa1..ebf976a 100644
--- a/Documentation/devicetree/bindings/arm/gic.txt
+++ b/Documentation/devicetree/bindings/arm/gic.txt
@@ -96,3 +96,56 @@ Example:
<0x2c006000 0x2000>;
interrupts = <1 9 0xf04>;
};
+
+
+* GICv2m extension for MSI/MSI-x support (Optional)
+
+Certain revisions of GIC-400 supports MSI/MSI-x via V2M register frame(s).
+This is enabled by specifying v2m sub-node(s).
+
+Required properties:
+
+- compatible : The value here should contain "arm,gic-v2m-frame".
+
+- msi-controller : Identifies the node as an MSI controller.
+
+- reg : GICv2m MSI interface register base and size
+
+Optional properties:
+
+- arm,msi-base-spi : When the MSI_TYPER register contains an incorrect
+ value, this property should contain the SPI base of
+ the MSI frame, overriding the HW value.
+
+- arm,msi-num-spis : When the MSI_TYPER register contains an incorrect
+ value, this property should contain the number of
+ SPIs assigned to the frame, overriding the HW value.
+
+Example:
+
+ interrupt-***@e1101000 {
+ compatible = "arm,gic-400";
+ #interrupt-cells = <3>;
+ #address-cells = <2>;
+ #size-cells = <2>;
+ interrupt-controller;
+ interrupts = <1 8 0xf04>;
+ ranges = <0 0 0 0xe1100000 0 0x100000>;
+ reg = <0x0 0xe1110000 0 0x01000>,
+ <0x0 0xe112f000 0 0x02000>,
+ <0x0 0xe1140000 0 0x10000>,
+ <0x0 0xe1160000 0 0x10000>;
+ v2m0: ***@0x8000 {
+ compatible = "arm,gic-v2m-frame";
+ msi-controller;
+ reg = <0x0 0x80000 0 0x1000>;
+ };
+
+ ....
+
+ v2mN: ***@0x9000 {
+ compatible = "arm,gic-v2m-frame";
+ msi-controller;
+ reg = <0x0 0x90000 0 0x1000>;
+ };
+ };
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 6b01df9..01927e5 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -12,6 +12,7 @@ config ARM64
select ARM_ARCH_TIMER
select ARM_GIC
select AUDIT_ARCH_COMPAT_GENERIC
+ select ARM_GIC_V2M
select ARM_GIC_V3
select BUILDTIME_EXTABLE_SORT
select CLONE_BACKWARDS
diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index b8632bf..61d18d9 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -7,6 +7,11 @@ config ARM_GIC
select IRQ_DOMAIN
select MULTI_IRQ_HANDLER

+config ARM_GIC_V2M
+ bool
+ depends on ARM_GIC
+ depends on PCI && PCI_MSI
+
config GIC_NON_BANKED
bool

diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 73052ba..3bda951 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -17,6 +17,7 @@ obj-$(CONFIG_ARCH_SUNXI) += irq-sun4i.o
obj-$(CONFIG_ARCH_SUNXI) += irq-sunxi-nmi.o
obj-$(CONFIG_ARCH_SPEAR3XX) += spear-shirq.o
obj-$(CONFIG_ARM_GIC) += irq-gic.o irq-gic-common.o
+obj-$(CONFIG_ARM_GIC_V2M) += irq-gic-v2m.o
obj-$(CONFIG_ARM_GIC_V3) += irq-gic-v3.o irq-gic-common.o
obj-$(CONFIG_ARM_NVIC) += irq-nvic.o
obj-$(CONFIG_ARM_VIC) += irq-vic.o
diff --git a/drivers/irqchip/irq-gic-v2m.c b/drivers/irqchip/irq-gic-v2m.c
new file mode 100644
index 0000000..725b8a4
--- /dev/null
+++ b/drivers/irqchip/irq-gic-v2m.c
@@ -0,0 +1,301 @@
+/*
+ * ARM GIC v2m MSI(-X) support
+ * Support for Message Signaled Interrupts for systems that
+ * implement ARM Generic Interrupt Controller: GICv2m.
+ *
+ * Copyright (C) 2014 Advanced Micro Devices, Inc.
+ * Authors: Suravee Suthikulpanit <suravee.suthikulpanit-***@public.gmane.org>
+ * Harish Kasiviswanathan <harish.kasiviswanathan-***@public.gmane.org>
+ * Brandon Anderson <brandon.anderson-***@public.gmane.org>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published
+ * by the Free Software Foundation.
+ */
+
+#define pr_fmt(fmt) "GICv2m: " fmt
+
+#include <linux/bitmap.h>
+#include <linux/irq.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/of_pci.h>
+#include <linux/pci.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+
+#include "irqchip.h"
+#include "irq-gic.h"
+#include "irq-gic-common.h"
+
+/*
+* MSI_TYPER:
+* [31:26] Reserved
+* [25:16] lowest SPI assigned to MSI
+* [15:10] Reserved
+* [9:0] Numer of SPIs assigned to MSI
+*/
+#define V2M_MSI_TYPER 0x008
+#define V2M_MSI_TYPER_BASE_SHIFT 16
+#define V2M_MSI_TYPER_BASE_MASK 0x3FF
+#define V2M_MSI_TYPER_NUM_MASK 0x3FF
+#define V2M_MSI_SETSPI_NS 0x040
+#define V2M_MIN_SPI 32
+#define V2M_MAX_SPI 1019
+
+#define V2M_MSI_TYPER_BASE_SPI(x) \
+ (((x) >> V2M_MSI_TYPER_BASE_SHIFT) & V2M_MSI_TYPER_BASE_MASK)
+
+#define V2M_MSI_TYPER_NUM_SPI(x) ((x) & V2M_MSI_TYPER_NUM_MASK)
+
+/*
+ * alloc_msi_irq - Allocate MSIs from available MSI bitmap.
+ * @data: Pointer to v2m_data
+ * @nvec: Number of interrupts to allocate
+ * @irq: Pointer to the allocated irq
+ *
+ * Allocates interrupts only if the contiguous range of MSIs
+ * with specified nvec are available. Otherwise return the number
+ * of available interrupts. If none are available, then returns -ENOENT.
+ */
+static int alloc_msi_irq(struct v2m_data *data, int nvec, int *irq)
+{
+ int size = data->nr_spis;
+ int next = size, i = nvec, ret;
+
+ /* We should never allocate more than available nr_spis */
+ if (i >= size)
+ i = size;
+
+ spin_lock(&data->msi_cnt_lock);
+
+ for (; i > 0; i--) {
+ next = bitmap_find_next_zero_area(data->bm,
+ size, 0, i, 0);
+ if (next < size)
+ break;
+ }
+
+ if (i != nvec) {
+ ret = i ? : -ENOENT;
+ } else {
+ bitmap_set(data->bm, next, nvec);
+ *irq = data->spi_start + next;
+ ret = 0;
+ }
+
+ spin_unlock(&data->msi_cnt_lock);
+
+ return ret;
+}
+
+static void gicv2m_teardown_msi_irq(struct msi_chip *chip, unsigned int irq)
+{
+ int pos;
+ struct v2m_data *data = container_of(chip, struct v2m_data, msi_chip);
+
+ spin_lock(&data->msi_cnt_lock);
+
+ pos = irq - data->spi_start;
+ if (pos >= 0 && pos < data->nr_spis)
+ bitmap_clear(data->bm, pos, 1);
+
+ spin_unlock(&data->msi_cnt_lock);
+}
+
+bool gicv2m_check_msi_range(struct gic_chip_data *gic, irq_hw_number_t hw)
+{
+ struct v2m_data *v2m = NULL;
+
+ list_for_each_entry(v2m, &gic->v2m_list, list) {
+ if (hw >= v2m->spi_start &&
+ hw < v2m->spi_start + v2m->nr_spis)
+ return true;
+ }
+ return false;
+}
+
+static int gicv2m_setup_msi_irq(struct msi_chip *chip,
+ struct pci_dev *pdev,
+ struct msi_desc *desc)
+{
+ int irq = 0, avail;
+ struct msi_msg msg;
+ phys_addr_t addr;
+ struct v2m_data *data = container_of(chip, struct v2m_data, msi_chip);
+
+ if (!desc) {
+ dev_err(&pdev->dev,
+ "MSI setup failed. Invalid msi descriptor\n");
+ return -EINVAL;
+ }
+
+ avail = alloc_msi_irq(data, 1, &irq);
+ if (avail != 0) {
+ dev_err(&pdev->dev,
+ "MSI setup failed. Cannnot allocate IRQ\n");
+ return -ENOSPC;
+ }
+
+ irq_set_chip_data(irq, chip);
+ irq_set_msi_desc(irq, desc);
+ irq_set_irq_type(irq, IRQ_TYPE_EDGE_RISING);
+
+ addr = data->res.start + V2M_MSI_SETSPI_NS;
+ msg.address_hi = (u32)(addr >> 32);
+ msg.address_lo = (u32)(addr);
+ msg.data = irq;
+ write_msi_msg(irq, &msg);
+
+ return 0;
+}
+
+static void gicv2m_mask_irq(struct irq_data *d)
+{
+ gic_mask_irq(d);
+ if (d->msi_desc)
+ mask_msi_irq(d);
+}
+
+static void gicv2m_unmask_irq(struct irq_data *d)
+{
+ gic_unmask_irq(d);
+ if (d->msi_desc)
+ unmask_msi_irq(d);
+}
+
+static bool is_msi_spi_valid(u32 base, u32 num)
+{
+ if (base < V2M_MIN_SPI) {
+ pr_err("Invalid MSI base SPI (base:%u)\n", base);
+ return false;
+ }
+
+ if ((num == 0) || (base + num > V2M_MAX_SPI)) {
+ pr_err("Number of SPIs (%u) exceed maximum (%u)\n",
+ num, V2M_MAX_SPI - V2M_MIN_SPI + 1);
+ return false;
+ }
+
+ return true;
+}
+
+static int __init
+gicv2m_init_one(struct device_node *node, struct v2m_data **v,
+ struct gic_chip_data *gic)
+{
+ int ret;
+ struct v2m_data *v2m = NULL;
+
+ *v = kzalloc(sizeof(struct v2m_data), GFP_KERNEL);
+ if (!*v) {
+ pr_err("Failed to allocate struct v2m_data.\n");
+ return -ENOMEM;
+ }
+
+ v2m = *v;
+ v2m->gic = gic;
+ v2m->msi_chip.owner = THIS_MODULE;
+ v2m->msi_chip.of_node = node;
+ v2m->msi_chip.setup_irq = gicv2m_setup_msi_irq;
+ v2m->msi_chip.teardown_irq = gicv2m_teardown_msi_irq;
+ ret = of_address_to_resource(node, 0, &v2m->res);
+ if (ret) {
+ pr_err("Failed to allocate v2m resource.\n");
+ goto err_out;
+ }
+
+ v2m->base = ioremap(v2m->res.start, resource_size(&v2m->res));
+ if (!v2m->base) {
+ pr_err("Failed to map GICv2m resource\n");
+ ret = -EINVAL;
+ goto err_out;
+ }
+
+ ret = of_pci_msi_chip_add(&v2m->msi_chip);
+ if (ret) {
+ pr_info("Failed to add msi_chip.\n");
+ goto err_out;
+ }
+
+ if (!of_property_read_u32(node, "arm,msi-base-spi", &v2m->spi_start) &&
+ !of_property_read_u32(node, "arm,msi-num-spis", &v2m->nr_spis)) {
+ pr_info("Overriding V2M MSI_TYPER (base:%u, num:%u)\n",
+ v2m->spi_start, v2m->nr_spis);
+ } else {
+ u32 typer = readl_relaxed(v2m->base + V2M_MSI_TYPER);
+
+ v2m->spi_start = V2M_MSI_TYPER_BASE_SPI(typer);
+ v2m->nr_spis = V2M_MSI_TYPER_NUM_SPI(typer);
+ }
+
+ if (!is_msi_spi_valid(v2m->spi_start, v2m->nr_spis)) {
+ ret = -EINVAL;
+ goto err_out;
+ }
+
+ v2m->bm = kzalloc(sizeof(long) * BITS_TO_LONGS(v2m->nr_spis),
+ GFP_KERNEL);
+ if (!v2m->bm) {
+ pr_err("Failed to allocate MSI bitmap\n");
+ ret = -ENOMEM;
+ goto err_out;
+ }
+
+ spin_lock_init(&v2m->msi_cnt_lock);
+
+ pr_info("Node %s: range[%#lx:%#lx], SPI[%d:%d]\n", node->name,
+ (unsigned long)v2m->res.start, (unsigned long)v2m->res.end,
+ v2m->spi_start, (v2m->spi_start + v2m->nr_spis));
+
+ return 0;
+err_out:
+ of_pci_msi_chip_remove(&v2m->msi_chip);
+ if (v2m->base)
+ iounmap(v2m->base);
+ kfree(v2m);
+ return ret;
+}
+
+int __init gicv2m_of_init(struct device_node *node,
+ struct gic_chip_data *gic,
+ struct irq_chip *v2m_chip)
+{
+ int ret = 0;
+ struct v2m_data *v2m;
+ struct device_node *child = NULL;
+
+ INIT_LIST_HEAD(&gic->v2m_list);
+
+ v2m_chip->irq_mask = gicv2m_mask_irq;
+ v2m_chip->irq_unmask = gicv2m_unmask_irq;
+
+ for (;;) {
+ child = of_get_next_child(node, child);
+ if (!child)
+ break;
+
+ if (!of_device_is_compatible(child, "arm,gic-v2m-frame"))
+ continue;
+
+ if (!of_find_property(child, "msi-controller", NULL))
+ continue;
+
+ ret = gicv2m_init_one(child, &v2m, gic);
+ if (ret) {
+ of_node_put(node);
+ break;
+ }
+
+ list_add_tail(&v2m->list, &gic->v2m_list);
+ }
+
+ if (ret && list_empty(&gic->v2m_list)) {
+ pr_warn("Warning: Failed to enable GICv2m support.\n");
+ return -EINVAL;
+ }
+
+ return ret;
+}
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index dda6dbc..63fa02d 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -46,30 +46,9 @@
#include <asm/smp_plat.h>

#include "irq-gic-common.h"
+#include "irq-gic.h"
#include "irqchip.h"

-union gic_base {
- void __iomem *common_base;
- void __percpu * __iomem *percpu_base;
-};
-
-struct gic_chip_data {
- union gic_base dist_base;
- union gic_base cpu_base;
-#ifdef CONFIG_CPU_PM
- u32 saved_spi_enable[DIV_ROUND_UP(1020, 32)];
- u32 saved_spi_conf[DIV_ROUND_UP(1020, 16)];
- u32 saved_spi_target[DIV_ROUND_UP(1020, 4)];
- u32 __percpu *saved_ppi_enable;
- u32 __percpu *saved_ppi_conf;
-#endif
- struct irq_domain *domain;
- unsigned int gic_irqs;
-#ifdef CONFIG_GIC_NON_BANKED
- void __iomem *(*get_base)(union gic_base *);
-#endif
-};
-
static DEFINE_RAW_SPINLOCK(irq_controller_lock);

/*
@@ -131,15 +110,36 @@ static inline void gic_set_base_accessor(struct gic_chip_data *data,
#define gic_set_base_accessor(d, f)
#endif

+static inline
+struct gic_chip_data *irq_data_get_gic_chip_data(struct irq_data *d)
+{
+ struct gic_chip_data *gic_data;
+ struct msi_chip *mchip;
+ struct v2m_data *v2mdat;
+
+ /*
+ * For MSI, irq_data.chip_data points to struct msi_chip.
+ * For non-MSI, irq_data.chip_data points to struct gic_chip_data.
+ */
+ if (d->msi_desc) {
+ mchip = irq_data_get_irq_chip_data(d);
+ v2mdat = container_of(mchip, struct v2m_data, msi_chip);
+ gic_data = v2mdat->gic;
+ } else {
+ gic_data = irq_data_get_irq_chip_data(d);
+ }
+ return gic_data;
+}
+
static inline void __iomem *gic_dist_base(struct irq_data *d)
{
- struct gic_chip_data *gic_data = irq_data_get_irq_chip_data(d);
+ struct gic_chip_data *gic_data = irq_data_get_gic_chip_data(d);
return gic_data_dist_base(gic_data);
}

static inline void __iomem *gic_cpu_base(struct irq_data *d)
{
- struct gic_chip_data *gic_data = irq_data_get_irq_chip_data(d);
+ struct gic_chip_data *gic_data = irq_data_get_gic_chip_data(d);
return gic_data_cpu_base(gic_data);
}

@@ -151,7 +151,7 @@ static inline unsigned int gic_irq(struct irq_data *d)
/*
* Routines to acknowledge, disable and enable interrupts
*/
-static void gic_mask_irq(struct irq_data *d)
+void gic_mask_irq(struct irq_data *d)
{
u32 mask = 1 << (gic_irq(d) % 32);

@@ -162,7 +162,7 @@ static void gic_mask_irq(struct irq_data *d)
raw_spin_unlock(&irq_controller_lock);
}

-static void gic_unmask_irq(struct irq_data *d)
+void gic_unmask_irq(struct irq_data *d)
{
u32 mask = 1 << (gic_irq(d) % 32);

@@ -325,6 +325,15 @@ static struct irq_chip gic_chip = {
.irq_set_wake = gic_set_wake,
};

+static struct irq_chip v2m_chip = {
+ .name = "GICv2m",
+ .irq_eoi = gic_eoi_irq,
+ .irq_set_type = gic_set_type,
+#ifdef CONFIG_SMP
+ .irq_set_affinity = gic_set_affinity,
+#endif
+};
+
void __init gic_cascade_irq(unsigned int gic_nr, unsigned int irq)
{
if (gic_nr >= MAX_GIC_NR)
@@ -767,19 +776,29 @@ void __init gic_init_physaddr(struct device_node *node)
static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq,
irq_hw_number_t hw)
{
+ struct gic_chip_data *gic = d->host_data;
+
+ irq_set_chip_data(irq, gic);
+
if (hw < 32) {
+ /* PPIs */
irq_set_percpu_devid(irq);
irq_set_chip_and_handler(irq, &gic_chip,
handle_percpu_devid_irq);
set_irq_flags(irq, IRQF_VALID | IRQF_NOAUTOEN);
} else {
- irq_set_chip_and_handler(irq, &gic_chip,
- handle_fasteoi_irq);
+ /* SPIs */
set_irq_flags(irq, IRQF_VALID | IRQF_PROBE);

- gic_routable_irq_domain_ops->map(d, irq, hw);
+ if (!gicv2m_check_msi_range(gic, hw)) {
+ irq_set_chip_and_handler(irq, &gic_chip,
+ handle_fasteoi_irq);
+ gic_routable_irq_domain_ops->map(d, irq, hw);
+ } else {
+ irq_set_chip_and_handler(irq, &v2m_chip,
+ handle_fasteoi_irq);
+ }
}
- irq_set_chip_data(irq, d->host_data);
return 0;
}

@@ -1010,6 +1029,9 @@ gic_of_init(struct device_node *node, struct device_node *parent)
if (of_property_read_u32(node, "cpu-offset", &percpu_offset))
percpu_offset = 0;

+ if (IS_ENABLED(CONFIG_ARM_GIC_V2M))
+ gicv2m_of_init(node, &gic_data[gic_cnt], &v2m_chip);
+
gic_init_bases(gic_cnt, -1, dist_base, cpu_base, percpu_offset, node);
if (!gic_cnt)
gic_init_physaddr(node);
diff --git a/drivers/irqchip/irq-gic.h b/drivers/irqchip/irq-gic.h
new file mode 100644
index 0000000..3021665
--- /dev/null
+++ b/drivers/irqchip/irq-gic.h
@@ -0,0 +1,54 @@
+#ifndef _IRQ_GIC_H_
+#define _IRQ_GIC_H_
+
+#include <linux/msi.h>
+#include <linux/pci.h>
+
+union gic_base {
+ void __iomem *common_base;
+ void __percpu * __iomem *percpu_base;
+};
+
+struct gic_chip_data;
+
+struct v2m_data {
+#ifdef CONFIG_ARM_GIC_V2M
+ struct list_head list;
+ spinlock_t msi_cnt_lock;
+ struct msi_chip msi_chip;
+ struct resource res; /* GICv2m resource */
+ void __iomem *base; /* GICv2m virt address */
+ unsigned int spi_start; /* The SPI number that MSIs start */
+ unsigned int nr_spis; /* The number of SPIs for MSIs */
+ unsigned long *bm; /* MSI vector bitmap */
+ struct gic_chip_data *gic;
+#endif
+};
+
+struct gic_chip_data {
+ union gic_base dist_base;
+ union gic_base cpu_base;
+#ifdef CONFIG_CPU_PM
+ u32 saved_spi_enable[DIV_ROUND_UP(1020, 32)];
+ u32 saved_spi_conf[DIV_ROUND_UP(1020, 16)];
+ u32 saved_spi_target[DIV_ROUND_UP(1020, 4)];
+ u32 __percpu *saved_ppi_enable;
+ u32 __percpu *saved_ppi_conf;
+#endif
+ struct irq_domain *domain;
+ unsigned int gic_irqs;
+#ifdef CONFIG_GIC_NON_BANKED
+ void __iomem *(*get_base)(union gic_base *);
+#endif
+#ifdef CONFIG_ARM_GIC_V2M
+ struct list_head v2m_list;
+#endif
+};
+
+void gic_mask_irq(struct irq_data *d);
+void gic_unmask_irq(struct irq_data *d);
+int gicv2m_of_init(struct device_node *node, struct gic_chip_data *gic,
+ struct irq_chip *v2m_chip) __init;
+bool gicv2m_check_msi_range(struct gic_chip_data *gic, irq_hw_number_t hw);
+
+#endif /* _IRQ_GIC_H_ */

Suravee Suthikulpanit

2014-09-28 21:35:19 UTC

Jason/Thomas,

This patch comes from:
[V8 2/2] irqchip: gicv2m: Add supports for ARM GICv2m MSI(-X)
(https://lkml.org/lkml/2014/9/20/113)

It has been slightly modified to remove the multi-MSI supports for now
(I am waiting to discuss with Marc after he returned from vacation.),
and will be submitted separately.

Since this patch is independent from the multi-MSI stuff. Please let me
know if you would consider taking this separately.

Thanks,

Suravee

Post by s***@public.gmane.org
ARM GICv2m specification extends GICv2 to support MSI(-X) with
a new set of register frame. This patch introduces support for
the non-secure GICv2m register frame. Currently, GICV2m is available
in certain version of GIC-400.
The patch introduces a new property in ARM gic binding, the v2m subnode.
It is optional.
---
Documentation/devicetree/bindings/arm/gic.txt | 53 +++++
arch/arm64/Kconfig | 1 +
drivers/irqchip/Kconfig | 5 +
drivers/irqchip/Makefile | 1 +
drivers/irqchip/irq-gic-v2m.c | 301 ++++++++++++++++++++++++++
drivers/irqchip/irq-gic.c | 82 ++++---
drivers/irqchip/irq-gic.h | 54 +++++
7 files changed, 467 insertions(+), 30 deletions(-)
create mode 100644 drivers/irqchip/irq-gic-v2m.c
create mode 100644 drivers/irqchip/irq-gic.h
diff --git a/Documentation/devicetree/bindings/arm/gic.txt b/Documentation/devicetree/bindings/arm/gic.txt
index c7d2fa1..ebf976a 100644
--- a/Documentation/devicetree/bindings/arm/gic.txt
+++ b/Documentation/devicetree/bindings/arm/gic.txt
<0x2c006000 0x2000>;
interrupts = <1 9 0xf04>;
};
+
+
+* GICv2m extension for MSI/MSI-x support (Optional)
+
+Certain revisions of GIC-400 supports MSI/MSI-x via V2M register frame(s).
+This is enabled by specifying v2m sub-node(s).
+
+
+- compatible : The value here should contain "arm,gic-v2m-frame".
+
+- msi-controller : Identifies the node as an MSI controller.
+
+- reg : GICv2m MSI interface register base and size
+
+
+- arm,msi-base-spi : When the MSI_TYPER register contains an incorrect
+ value, this property should contain the SPI base of
+ the MSI frame, overriding the HW value.
+
+- arm,msi-num-spis : When the MSI_TYPER register contains an incorrect
+ value, this property should contain the number of
+ SPIs assigned to the frame, overriding the HW value.
+
+
+ compatible = "arm,gic-400";
+ #interrupt-cells = <3>;
+ #address-cells = <2>;
+ #size-cells = <2>;
+ interrupt-controller;
+ interrupts = <1 8 0xf04>;
+ ranges = <0 0 0 0xe1100000 0 0x100000>;
+ reg = <0x0 0xe1110000 0 0x01000>,
+ <0x0 0xe112f000 0 0x02000>,
+ <0x0 0xe1140000 0 0x10000>,
+ <0x0 0xe1160000 0 0x10000>;
+ compatible = "arm,gic-v2m-frame";
+ msi-controller;
+ reg = <0x0 0x80000 0 0x1000>;
+ };
+
+ ....
+
+ compatible = "arm,gic-v2m-frame";
+ msi-controller;
+ reg = <0x0 0x90000 0 0x1000>;
+ };
+ };
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 6b01df9..01927e5 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -12,6 +12,7 @@ config ARM64
select ARM_ARCH_TIMER
select ARM_GIC
select AUDIT_ARCH_COMPAT_GENERIC
+ select ARM_GIC_V2M
select ARM_GIC_V3
select BUILDTIME_EXTABLE_SORT
select CLONE_BACKWARDS
diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index b8632bf..61d18d9 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -7,6 +7,11 @@ config ARM_GIC
select IRQ_DOMAIN
select MULTI_IRQ_HANDLER
+config ARM_GIC_V2M
+ bool
+ depends on ARM_GIC
+ depends on PCI && PCI_MSI
+
config GIC_NON_BANKED
bool
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 73052ba..3bda951 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -17,6 +17,7 @@ obj-$(CONFIG_ARCH_SUNXI) += irq-sun4i.o
obj-$(CONFIG_ARCH_SUNXI) += irq-sunxi-nmi.o
obj-$(CONFIG_ARCH_SPEAR3XX) += spear-shirq.o
obj-$(CONFIG_ARM_GIC) += irq-gic.o irq-gic-common.o
+obj-$(CONFIG_ARM_GIC_V2M) += irq-gic-v2m.o
obj-$(CONFIG_ARM_GIC_V3) += irq-gic-v3.o irq-gic-common.o
obj-$(CONFIG_ARM_NVIC) += irq-nvic.o
obj-$(CONFIG_ARM_VIC) += irq-vic.o
diff --git a/drivers/irqchip/irq-gic-v2m.c b/drivers/irqchip/irq-gic-v2m.c
new file mode 100644
index 0000000..725b8a4
--- /dev/null
+++ b/drivers/irqchip/irq-gic-v2m.c
@@ -0,0 +1,301 @@
+/*
+ * ARM GIC v2m MSI(-X) support
+ * Support for Message Signaled Interrupts for systems that
+ * implement ARM Generic Interrupt Controller: GICv2m.
+ *
+ * Copyright (C) 2014 Advanced Micro Devices, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published
+ * by the Free Software Foundation.
+ */
+
+#define pr_fmt(fmt) "GICv2m: " fmt
+
+#include <linux/bitmap.h>
+#include <linux/irq.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/of_pci.h>
+#include <linux/pci.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+
+#include "irqchip.h"
+#include "irq-gic.h"
+#include "irq-gic-common.h"
+
+/*
+* [31:26] Reserved
+* [25:16] lowest SPI assigned to MSI
+* [15:10] Reserved
+* [9:0] Numer of SPIs assigned to MSI
+*/
+#define V2M_MSI_TYPER 0x008
+#define V2M_MSI_TYPER_BASE_SHIFT 16
+#define V2M_MSI_TYPER_BASE_MASK 0x3FF
+#define V2M_MSI_TYPER_NUM_MASK 0x3FF
+#define V2M_MSI_SETSPI_NS 0x040
+#define V2M_MIN_SPI 32
+#define V2M_MAX_SPI 1019
+
+#define V2M_MSI_TYPER_BASE_SPI(x) \
+ (((x) >> V2M_MSI_TYPER_BASE_SHIFT) & V2M_MSI_TYPER_BASE_MASK)
+
+#define V2M_MSI_TYPER_NUM_SPI(x) ((x) & V2M_MSI_TYPER_NUM_MASK)
+
+/*
+ * alloc_msi_irq - Allocate MSIs from available MSI bitmap.
+ *
+ * Allocates interrupts only if the contiguous range of MSIs
+ * with specified nvec are available. Otherwise return the number
+ * of available interrupts. If none are available, then returns -ENOENT.
+ */
+static int alloc_msi_irq(struct v2m_data *data, int nvec, int *irq)
+{
+ int size = data->nr_spis;
+ int next = size, i = nvec, ret;
+
+ /* We should never allocate more than available nr_spis */
+ if (i >= size)
+ i = size;
+
+ spin_lock(&data->msi_cnt_lock);
+
+ for (; i > 0; i--) {
+ next = bitmap_find_next_zero_area(data->bm,
+ size, 0, i, 0);
+ if (next < size)
+ break;
+ }
+
+ if (i != nvec) {
+ ret = i ? : -ENOENT;
+ } else {
+ bitmap_set(data->bm, next, nvec);
+ *irq = data->spi_start + next;
+ ret = 0;
+ }
+
+ spin_unlock(&data->msi_cnt_lock);
+
+ return ret;
+}
+
+static void gicv2m_teardown_msi_irq(struct msi_chip *chip, unsigned int irq)
+{
+ int pos;
+ struct v2m_data *data = container_of(chip, struct v2m_data, msi_chip);
+
+ spin_lock(&data->msi_cnt_lock);
+
+ pos = irq - data->spi_start;
+ if (pos >= 0 && pos < data->nr_spis)
+ bitmap_clear(data->bm, pos, 1);
+
+ spin_unlock(&data->msi_cnt_lock);
+}
+
+bool gicv2m_check_msi_range(struct gic_chip_data *gic, irq_hw_number_t hw)
+{
+ struct v2m_data *v2m = NULL;
+
+ list_for_each_entry(v2m, &gic->v2m_list, list) {
+ if (hw >= v2m->spi_start &&
+ hw < v2m->spi_start + v2m->nr_spis)
+ return true;
+ }
+ return false;
+}
+
+static int gicv2m_setup_msi_irq(struct msi_chip *chip,
+ struct pci_dev *pdev,
+ struct msi_desc *desc)
+{
+ int irq = 0, avail;
+ struct msi_msg msg;
+ phys_addr_t addr;
+ struct v2m_data *data = container_of(chip, struct v2m_data, msi_chip);
+
+ if (!desc) {
+ dev_err(&pdev->dev,
+ "MSI setup failed. Invalid msi descriptor\n");
+ return -EINVAL;
+ }
+
+ avail = alloc_msi_irq(data, 1, &irq);
+ if (avail != 0) {
+ dev_err(&pdev->dev,
+ "MSI setup failed. Cannnot allocate IRQ\n");
+ return -ENOSPC;
+ }
+
+ irq_set_chip_data(irq, chip);
+ irq_set_msi_desc(irq, desc);
+ irq_set_irq_type(irq, IRQ_TYPE_EDGE_RISING);
+
+ addr = data->res.start + V2M_MSI_SETSPI_NS;
+ msg.address_hi = (u32)(addr >> 32);
+ msg.address_lo = (u32)(addr);
+ msg.data = irq;
+ write_msi_msg(irq, &msg);
+
+ return 0;
+}
+
+static void gicv2m_mask_irq(struct irq_data *d)
+{
+ gic_mask_irq(d);
+ if (d->msi_desc)
+ mask_msi_irq(d);
+}
+
+static void gicv2m_unmask_irq(struct irq_data *d)
+{
+ gic_unmask_irq(d);
+ if (d->msi_desc)
+ unmask_msi_irq(d);
+}
+
+static bool is_msi_spi_valid(u32 base, u32 num)
+{
+ if (base < V2M_MIN_SPI) {
+ pr_err("Invalid MSI base SPI (base:%u)\n", base);
+ return false;
+ }
+
+ if ((num == 0) || (base + num > V2M_MAX_SPI)) {
+ pr_err("Number of SPIs (%u) exceed maximum (%u)\n",
+ num, V2M_MAX_SPI - V2M_MIN_SPI + 1);
+ return false;
+ }
+
+ return true;
+}
+
+static int __init
+gicv2m_init_one(struct device_node *node, struct v2m_data **v,
+ struct gic_chip_data *gic)
+{
+ int ret;
+ struct v2m_data *v2m = NULL;
+
+ *v = kzalloc(sizeof(struct v2m_data), GFP_KERNEL);
+ if (!*v) {
+ pr_err("Failed to allocate struct v2m_data.\n");
+ return -ENOMEM;
+ }
+
+ v2m = *v;
+ v2m->gic = gic;
+ v2m->msi_chip.owner = THIS_MODULE;
+ v2m->msi_chip.of_node = node;
+ v2m->msi_chip.setup_irq = gicv2m_setup_msi_irq;
+ v2m->msi_chip.teardown_irq = gicv2m_teardown_msi_irq;
+ ret = of_address_to_resource(node, 0, &v2m->res);
+ if (ret) {
+ pr_err("Failed to allocate v2m resource.\n");
+ goto err_out;
+ }
+
+ v2m->base = ioremap(v2m->res.start, resource_size(&v2m->res));
+ if (!v2m->base) {
+ pr_err("Failed to map GICv2m resource\n");
+ ret = -EINVAL;
+ goto err_out;
+ }
+
+ ret = of_pci_msi_chip_add(&v2m->msi_chip);
+ if (ret) {
+ pr_info("Failed to add msi_chip.\n");
+ goto err_out;
+ }
+
+ if (!of_property_read_u32(node, "arm,msi-base-spi", &v2m->spi_start) &&
+ !of_property_read_u32(node, "arm,msi-num-spis", &v2m->nr_spis)) {
+ pr_info("Overriding V2M MSI_TYPER (base:%u, num:%u)\n",
+ v2m->spi_start, v2m->nr_spis);
+ } else {
+ u32 typer = readl_relaxed(v2m->base + V2M_MSI_TYPER);
+
+ v2m->spi_start = V2M_MSI_TYPER_BASE_SPI(typer);
+ v2m->nr_spis = V2M_MSI_TYPER_NUM_SPI(typer);
+ }
+
+ if (!is_msi_spi_valid(v2m->spi_start, v2m->nr_spis)) {
+ ret = -EINVAL;
+ goto err_out;
+ }
+
+ v2m->bm = kzalloc(sizeof(long) * BITS_TO_LONGS(v2m->nr_spis),
+ GFP_KERNEL);
+ if (!v2m->bm) {
+ pr_err("Failed to allocate MSI bitmap\n");
+ ret = -ENOMEM;
+ goto err_out;
+ }
+
+ spin_lock_init(&v2m->msi_cnt_lock);
+
+ pr_info("Node %s: range[%#lx:%#lx], SPI[%d:%d]\n", node->name,
+ (unsigned long)v2m->res.start, (unsigned long)v2m->res.end,
+ v2m->spi_start, (v2m->spi_start + v2m->nr_spis));
+
+ return 0;
+ of_pci_msi_chip_remove(&v2m->msi_chip);
+ if (v2m->base)
+ iounmap(v2m->base);
+ kfree(v2m);
+ return ret;
+}
+
+int __init gicv2m_of_init(struct device_node *node,
+ struct gic_chip_data *gic,
+ struct irq_chip *v2m_chip)
+{
+ int ret = 0;
+ struct v2m_data *v2m;
+ struct device_node *child = NULL;
+
+ INIT_LIST_HEAD(&gic->v2m_list);
+
+ v2m_chip->irq_mask = gicv2m_mask_irq;
+ v2m_chip->irq_unmask = gicv2m_unmask_irq;
+
+ for (;;) {
+ child = of_get_next_child(node, child);
+ if (!child)
+ break;
+
+ if (!of_device_is_compatible(child, "arm,gic-v2m-frame"))
+ continue;
+
+ if (!of_find_property(child, "msi-controller", NULL))
+ continue;
+
+ ret = gicv2m_init_one(child, &v2m, gic);
+ if (ret) {
+ of_node_put(node);
+ break;
+ }
+
+ list_add_tail(&v2m->list, &gic->v2m_list);
+ }
+
+ if (ret && list_empty(&gic->v2m_list)) {
+ pr_warn("Warning: Failed to enable GICv2m support.\n");
+ return -EINVAL;
+ }
+
+ return ret;
+}
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index dda6dbc..63fa02d 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -46,30 +46,9 @@
#include <asm/smp_plat.h>
#include "irq-gic-common.h"
+#include "irq-gic.h"
#include "irqchip.h"
-union gic_base {
- void __iomem *common_base;
- void __percpu * __iomem *percpu_base;
-};
-
-struct gic_chip_data {
- union gic_base dist_base;
- union gic_base cpu_base;
-#ifdef CONFIG_CPU_PM
- u32 saved_spi_enable[DIV_ROUND_UP(1020, 32)];
- u32 saved_spi_conf[DIV_ROUND_UP(1020, 16)];
- u32 saved_spi_target[DIV_ROUND_UP(1020, 4)];
- u32 __percpu *saved_ppi_enable;
- u32 __percpu *saved_ppi_conf;
-#endif
- struct irq_domain *domain;
- unsigned int gic_irqs;
-#ifdef CONFIG_GIC_NON_BANKED
- void __iomem *(*get_base)(union gic_base *);
-#endif
-};
-
static DEFINE_RAW_SPINLOCK(irq_controller_lock);
/*
@@ -131,15 +110,36 @@ static inline void gic_set_base_accessor(struct gic_chip_data *data,
#define gic_set_base_accessor(d, f)
#endif
+static inline
+struct gic_chip_data *irq_data_get_gic_chip_data(struct irq_data *d)
+{
+ struct gic_chip_data *gic_data;
+ struct msi_chip *mchip;
+ struct v2m_data *v2mdat;
+
+ /*
+ * For MSI, irq_data.chip_data points to struct msi_chip.
+ * For non-MSI, irq_data.chip_data points to struct gic_chip_data.
+ */
+ if (d->msi_desc) {
+ mchip = irq_data_get_irq_chip_data(d);
+ v2mdat = container_of(mchip, struct v2m_data, msi_chip);
+ gic_data = v2mdat->gic;
+ } else {
+ gic_data = irq_data_get_irq_chip_data(d);
+ }
+ return gic_data;
+}
+
static inline void __iomem *gic_dist_base(struct irq_data *d)
{
- struct gic_chip_data *gic_data = irq_data_get_irq_chip_data(d);
+ struct gic_chip_data *gic_data = irq_data_get_gic_chip_data(d);
return gic_data_dist_base(gic_data);
}
static inline void __iomem *gic_cpu_base(struct irq_data *d)
{
- struct gic_chip_data *gic_data = irq_data_get_irq_chip_data(d);
+ struct gic_chip_data *gic_data = irq_data_get_gic_chip_data(d);
return gic_data_cpu_base(gic_data);
}
@@ -151,7 +151,7 @@ static inline unsigned int gic_irq(struct irq_data *d)
/*
* Routines to acknowledge, disable and enable interrupts
*/
-static void gic_mask_irq(struct irq_data *d)
+void gic_mask_irq(struct irq_data *d)
{
u32 mask = 1 << (gic_irq(d) % 32);
@@ -162,7 +162,7 @@ static void gic_mask_irq(struct irq_data *d)
raw_spin_unlock(&irq_controller_lock);
}
-static void gic_unmask_irq(struct irq_data *d)
+void gic_unmask_irq(struct irq_data *d)
{
u32 mask = 1 << (gic_irq(d) % 32);
@@ -325,6 +325,15 @@ static struct irq_chip gic_chip = {
.irq_set_wake = gic_set_wake,
};
+static struct irq_chip v2m_chip = {
+ .name = "GICv2m",
+ .irq_eoi = gic_eoi_irq,
+ .irq_set_type = gic_set_type,
+#ifdef CONFIG_SMP
+ .irq_set_affinity = gic_set_affinity,
+#endif
+};
+
void __init gic_cascade_irq(unsigned int gic_nr, unsigned int irq)
{
if (gic_nr >= MAX_GIC_NR)
@@ -767,19 +776,29 @@ void __init gic_init_physaddr(struct device_node *node)
static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq,
irq_hw_number_t hw)
{
+ struct gic_chip_data *gic = d->host_data;
+
+ irq_set_chip_data(irq, gic);
+
if (hw < 32) {
+ /* PPIs */
irq_set_percpu_devid(irq);
irq_set_chip_and_handler(irq, &gic_chip,
handle_percpu_devid_irq);
set_irq_flags(irq, IRQF_VALID | IRQF_NOAUTOEN);
} else {
- irq_set_chip_and_handler(irq, &gic_chip,
- handle_fasteoi_irq);
+ /* SPIs */
set_irq_flags(irq, IRQF_VALID | IRQF_PROBE);
- gic_routable_irq_domain_ops->map(d, irq, hw);
+ if (!gicv2m_check_msi_range(gic, hw)) {
+ irq_set_chip_and_handler(irq, &gic_chip,
+ handle_fasteoi_irq);
+ gic_routable_irq_domain_ops->map(d, irq, hw);
+ } else {
+ irq_set_chip_and_handler(irq, &v2m_chip,
+ handle_fasteoi_irq);
+ }
}
- irq_set_chip_data(irq, d->host_data);
return 0;
}
@@ -1010,6 +1029,9 @@ gic_of_init(struct device_node *node, struct device_node *parent)
if (of_property_read_u32(node, "cpu-offset", &percpu_offset))
percpu_offset = 0;
+ if (IS_ENABLED(CONFIG_ARM_GIC_V2M))
+ gicv2m_of_init(node, &gic_data[gic_cnt], &v2m_chip);
+
gic_init_bases(gic_cnt, -1, dist_base, cpu_base, percpu_offset, node);
if (!gic_cnt)
gic_init_physaddr(node);
diff --git a/drivers/irqchip/irq-gic.h b/drivers/irqchip/irq-gic.h
new file mode 100644
index 0000000..3021665
--- /dev/null
+++ b/drivers/irqchip/irq-gic.h
@@ -0,0 +1,54 @@
+#ifndef _IRQ_GIC_H_
+#define _IRQ_GIC_H_
+
+#include <linux/msi.h>
+#include <linux/pci.h>
+
+union gic_base {
+ void __iomem *common_base;
+ void __percpu * __iomem *percpu_base;
+};
+
+struct gic_chip_data;
+
+struct v2m_data {
+#ifdef CONFIG_ARM_GIC_V2M
+ struct list_head list;
+ spinlock_t msi_cnt_lock;
+ struct msi_chip msi_chip;
+ struct resource res; /* GICv2m resource */
+ void __iomem *base; /* GICv2m virt address */
+ unsigned int spi_start; /* The SPI number that MSIs start */
+ unsigned int nr_spis; /* The number of SPIs for MSIs */
+ unsigned long *bm; /* MSI vector bitmap */
+ struct gic_chip_data *gic;
+#endif
+};
+
+struct gic_chip_data {
+ union gic_base dist_base;
+ union gic_base cpu_base;
+#ifdef CONFIG_CPU_PM
+ u32 saved_spi_enable[DIV_ROUND_UP(1020, 32)];
+ u32 saved_spi_conf[DIV_ROUND_UP(1020, 16)];
+ u32 saved_spi_target[DIV_ROUND_UP(1020, 4)];
+ u32 __percpu *saved_ppi_enable;
+ u32 __percpu *saved_ppi_conf;
+#endif
+ struct irq_domain *domain;
+ unsigned int gic_irqs;
+#ifdef CONFIG_GIC_NON_BANKED
+ void __iomem *(*get_base)(union gic_base *);
+#endif
+#ifdef CONFIG_ARM_GIC_V2M
+ struct list_head v2m_list;
+#endif
+};
+
+void gic_mask_irq(struct irq_data *d);
+void gic_unmask_irq(struct irq_data *d);
+int gicv2m_of_init(struct device_node *node, struct gic_chip_data *gic,
+ struct irq_chip *v2m_chip) __init;
+bool gicv2m_check_msi_range(struct gic_chip_data *gic, irq_hw_number_t hw);
+
+#endif /* _IRQ_GIC_H_ */

Thomas Gleixner

2014-09-29 14:23:08 UTC

Post by Suravee Suthikulpanit

Post by Suravee Suthikulpanit
Jason/Thomas,
[V8 2/2] irqchip: gicv2m: Add supports for ARM GICv2m MSI(-X)
(https://lkml.org/lkml/2014/9/20/113)
It has been slightly modified to remove the multi-MSI supports for now
(I am waiting to discuss with Marc after he returned from vacation.), and will
be submitted separately.
Since this patch is independent from the multi-MSI stuff. Please let me know
if you would consider taking this separately.

Not without an explicit reviewed/acked from Marc for the GIC part and
a reviewed/acked from the DT folks.

Post by s***@public.gmane.org
+static inline
+struct gic_chip_data *irq_data_get_gic_chip_data(struct irq_data *d)
+{
+ struct gic_chip_data *gic_data;
+ struct msi_chip *mchip;
+ struct v2m_data *v2mdat;
+
+ /*
+ * For MSI, irq_data.chip_data points to struct msi_chip.
+ * For non-MSI, irq_data.chip_data points to struct gic_chip_data.
+ */
+ if (d->msi_desc) {
+ mchip = irq_data_get_irq_chip_data(d);
+ v2mdat = container_of(mchip, struct v2m_data, msi_chip);
+ gic_data = v2mdat->gic;
+ } else {
+ gic_data = irq_data_get_irq_chip_data(d);
+ }
+ return gic_data;
+}

For heavens sake, why are you insisting on duct-taping that into the
GIC proper instead of coming up with a proper layering?

https://lkml.org/lkml/2014/8/27/228
https://lkml.org/lkml/2014/8/26/707

Thanks,

tglx

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

Arnd Bergmann

2014-09-29 14:42:18 UTC

+ compatible = "arm,gic-400";
+ #interrupt-cells = >;
+ #address-cells = <2>;
+ #size-cells = <2>;
+ interrupt-controller;
+ interrupts = <1 8 0xf04>;
+ ranges = <0 0 0 0xe1100000 0 0x100000>;
+ reg = <0x0 0xe1110000 0 0x01000>,
+ <0x0 0xe112f000 0 0x02000>,
+ <0x0 0xe1140000 0 0x10000>,
+ <0x0 0xe1160000 0 0x10000>;
+ compatible = "arm,gic-v2m-frame";
+ msi-controller;
+ reg = <0x0 0x80000 0 0x1000>;
+ };
+
+ ....
+
+ compatible = "arm,gic-v2m-frame";
+ msi-controller;
+ reg = <0x0 0x90000 0 0x1000>;
+ };
+ };

Could this just be modeled as a separate msi-controller node
outside of the GIC?

Instead of the arm,msi-base-spi/arm,msi-num-spis properties, how
about using regular "interrupts"/"interrupt-parent" properties listing
the exact interrupts? That would also make it more flexible in
case the same layout is used with a parent other than the GIC.

ARnd
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Mark Rutland

2014-10-10 13:45:34 UTC