Re: [PATCH 06/17] pci: Add generic pcibios_{fixup_bus,align_resource}
On Thu, 08 Jun 2017 01:35:29 PDT (-0700), Arnd Bergmann wrote: > On Thu, Jun 8, 2017 at 10:12 AM, Christoph Hellwig wrote: >> On Wed, Jun 07, 2017 at 09:19:49AM +0200, Geert Uytterhoeven wrote: >>> CC pci folks >> >> Ok, replying with pci folks in Cc then :) >> >> Weak symbols have (rightly) gotten a bad reputation, so maybe >> we should approach this without them. > > Agreed, I would almost never recommend using a __weak symbol, > but they are already widely used in the PCI subsystem, so I suggested > using them here for consistency. > > We have a struct pci_host_bridge now, and we should eventually > move most of the 38 pci specific weak per-architecture symbols > into per-host driver callbacks, but that I think that would be too much > to ask for when adding an architecture port. I agree :) >> It seems we have a large number of emptry pcibios_fixup_bus calls >> alreayd, so I think we should simply have the architectures >> that do define it define a Kconfig or header symbol and not call >> it at all otherwise. > > I would argue that most of them should not be per-architecture > in the first place, the current state is mostly an artifact of the > times when each architecture had just one PCI implementation. > > The ones that have multiple implementations (arm, powerpc, ...) > tend to actually override the weak functions with their own > per-host multiplexers again. > >> For the ones that exist as lot just seem to call pci_read_bridge_bases >> and/or pcibios_fixup_device_resources in one form or another, >> and I wonder why we even need the arch indirection for that. >> >> Similarly for pcibios_align_resource: a lot of architetures seem >> to have a noop, and the once that don't mostly seem copy and >> paste code, so we should again have a symbol for architectures >> to opt into it, and we probably should have a generic helper >> for the VGA window mirroring code instead of duplicating it multiple >> times. > > I now remember that we already have a host_bridge->align_resource > callback pointer, so the generic function should definitely try > to use that. > > We could just use the version from arch/mips/pci/pci-generic.c > and remove that in the process. I'm not sure about why mips calls > pci_read_bridge_bases() in pcibios_fixup_bus() though, or whether > this make sense to put in the generic version. I'm splitting this off into another patch set and sending it to a bunch of PCI people.
Re: [PATCH 06/17] pci: Add generic pcibios_{fixup_bus,align_resource}
On Wed, 07 Jun 2017 01:01:57 PDT (-0700), Arnd Bergmann wrote: > On Wed, Jun 7, 2017 at 9:19 AM, Geert Uytterhoeven > wrote: >> CC pci folks >> >> On Wed, Jun 7, 2017 at 12:59 AM, Palmer Dabbelt wrote: >>> While upstreaming the RISC-V port, it was pointed out that multiple >>> architectures (arc, arm64, cris, microblaze, sh, tile) have copied the >>> mostly empty versions of at least one of these functions. This defines >>> weakly bound versions of the common functions so other architetures can >>> use them. >>> >>> Signed-off-by: Palmer Dabbelt > > Thanks a lot for taking care of this! No problem. > >>> diff --git a/drivers/pci/bios.c b/drivers/pci/bios.c >>> new file mode 100644 >>> index ..ffe34c024aa8 >>> --- /dev/null >>> +++ b/drivers/pci/bios.c >>> @@ -0,0 +1,42 @@ >>> + >>> +/* This file contains weakly bound functions that implement pcibios >>> functions >>> + * that some architectures have copied verbatim. >>> + */ > > Instead of adding a new file, I would suggest adding the two functions next > to their callers, in probe.c and setup-res.c, respectively. I've split this out into another patch set.
Re: [PATCH 06/17] pci: Add generic pcibios_{fixup_bus,align_resource}
On Thu, Jun 8, 2017 at 10:12 AM, Christoph Hellwig wrote: > On Wed, Jun 07, 2017 at 09:19:49AM +0200, Geert Uytterhoeven wrote: >> CC pci folks > > Ok, replying with pci folks in Cc then :) > > Weak symbols have (rightly) gotten a bad reputation, so maybe > we should approach this without them. Agreed, I would almost never recommend using a __weak symbol, but they are already widely used in the PCI subsystem, so I suggested using them here for consistency. We have a struct pci_host_bridge now, and we should eventually move most of the 38 pci specific weak per-architecture symbols into per-host driver callbacks, but that I think that would be too much to ask for when adding an architecture port. > It seems we have a large number of emptry pcibios_fixup_bus calls > alreayd, so I think we should simply have the architectures > that do define it define a Kconfig or header symbol and not call > it at all otherwise. I would argue that most of them should not be per-architecture in the first place, the current state is mostly an artifact of the times when each architecture had just one PCI implementation. The ones that have multiple implementations (arm, powerpc, ...) tend to actually override the weak functions with their own per-host multiplexers again. > For the ones that exist as lot just seem to call pci_read_bridge_bases > and/or pcibios_fixup_device_resources in one form or another, > and I wonder why we even need the arch indirection for that. > > Similarly for pcibios_align_resource: a lot of architetures seem > to have a noop, and the once that don't mostly seem copy and > paste code, so we should again have a symbol for architectures > to opt into it, and we probably should have a generic helper > for the VGA window mirroring code instead of duplicating it multiple > times. I now remember that we already have a host_bridge->align_resource callback pointer, so the generic function should definitely try to use that. We could just use the version from arch/mips/pci/pci-generic.c and remove that in the process. I'm not sure about why mips calls pci_read_bridge_bases() in pcibios_fixup_bus() though, or whether this make sense to put in the generic version. Arnd
Re: [PATCH 06/17] pci: Add generic pcibios_{fixup_bus,align_resource}
On Wed, Jun 07, 2017 at 09:19:49AM +0200, Geert Uytterhoeven wrote: > CC pci folks Ok, replying with pci folks in Cc then :) Weak symbols have (rightly) gotten a bad reputation, so maybe we should approach this without them. It seems we have a large number of emptry pcibios_fixup_bus calls alreayd, so I think we should simply have the architectures that do define it define a Kconfig or header symbol and not call it at all otherwise. For the ones that exist as lot just seem to call pci_read_bridge_bases and/or pcibios_fixup_device_resources in one form or another, and I wonder why we even need the arch indirection for that. Similarly for pcibios_align_resource: a lot of architetures seem to have a noop, and the once that don't mostly seem copy and paste code, so we should again have a symbol for architectures to opt into it, and we probably should have a generic helper for the VGA window mirroring code instead of duplicating it multiple times.
Re: [PATCH 06/17] pci: Add generic pcibios_{fixup_bus,align_resource}
On Wed, Jun 7, 2017 at 9:19 AM, Geert Uytterhoeven wrote: > CC pci folks > > On Wed, Jun 7, 2017 at 12:59 AM, Palmer Dabbelt wrote: >> While upstreaming the RISC-V port, it was pointed out that multiple >> architectures (arc, arm64, cris, microblaze, sh, tile) have copied the >> mostly empty versions of at least one of these functions. This defines >> weakly bound versions of the common functions so other architetures can >> use them. >> >> Signed-off-by: Palmer Dabbelt Thanks a lot for taking care of this! >> diff --git a/drivers/pci/bios.c b/drivers/pci/bios.c >> new file mode 100644 >> index ..ffe34c024aa8 >> --- /dev/null >> +++ b/drivers/pci/bios.c >> @@ -0,0 +1,42 @@ >> + >> +/* This file contains weakly bound functions that implement pcibios >> functions >> + * that some architectures have copied verbatim. >> + */ Instead of adding a new file, I would suggest adding the two functions next to their callers, in probe.c and setup-res.c, respectively. Arnd
Re: [PATCH 06/17] pci: Add generic pcibios_{fixup_bus,align_resource}
CC pci folks
On Wed, Jun 7, 2017 at 12:59 AM, Palmer Dabbelt wrote:
> While upstreaming the RISC-V port, it was pointed out that multiple
> architectures (arc, arm64, cris, microblaze, sh, tile) have copied the
> mostly empty versions of at least one of these functions. This defines
> weakly bound versions of the common functions so other architetures can
> use them.
>
> Signed-off-by: Palmer Dabbelt
> ---
> drivers/pci/Makefile | 2 +-
> drivers/pci/bios.c | 42 ++
> 2 files changed, 43 insertions(+), 1 deletion(-)
> create mode 100644 drivers/pci/bios.c
>
> diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
> index a29d9ec05d13..fa7040915194 100644
> --- a/drivers/pci/Makefile
> +++ b/drivers/pci/Makefile
> @@ -4,7 +4,7 @@
>
> obj-y += access.o bus.o probe.o host-bridge.o remove.o pci.o \
> pci-driver.o search.o pci-sysfs.o rom.o setup-res.o \
> - irq.o vpd.o setup-bus.o vc.o mmap.o
> + irq.o vpd.o setup-bus.o vc.o mmap.o bios.o
> obj-$(CONFIG_PROC_FS) += proc.o
> obj-$(CONFIG_SYSFS) += slot.o
>
> diff --git a/drivers/pci/bios.c b/drivers/pci/bios.c
> new file mode 100644
> index ..ffe34c024aa8
> --- /dev/null
> +++ b/drivers/pci/bios.c
> @@ -0,0 +1,42 @@
> +/*
> + * Code borrowed from arch/arm64/kernel/pci.c
> + * which borrowed from powerpc/kernel/pci-common.c
> + * which borrowed from arch/alpha/kernel/pci.c
> + *
> + * Extruded from code written by
> + * Dave Rusling (david.rusl...@reo.mts.dec.com)
> + * David Mosberger (dav...@cs.arizona.edu)
> + * Copyright (C) 1999 Andrea Arcangeli
> + * Copyright (C) 2000 Ivan Kokshaysky
> + * Copyright (C) 2003 Anton Blanchard , IBM
> + * Copyright (C) 2014 ARM Ltd.
> + * Copyright (C) 2017 SiFive
> + *
> + * 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.
> + */
> +
> +/* This file contains weakly bound functions that implement pcibios functions
> + * that some architectures have copied verbatim.
> + */
> +
> +#include
> +
> +/*
> + * Called after each bus is probed, but before its children are examined
> + */
> +__attribute__ ((weak))
> +void pcibios_fixup_bus(struct pci_bus *bus)
> +{
> + /* nothing to do, expected to be removed in the future */
> +}
> +/*
> + * We don't have to worry about legacy ISA devices, so nothing to do here
> + */
> +__attribute__ ((weak))
> +resource_size_t pcibios_align_resource(void *data, const struct resource
> *res,
> + resource_size_t size, resource_size_t
> align)
> +{
> + return res->start;
> +}
> --
> 2.13.0
[PATCH 06/17] pci: Add generic pcibios_{fixup_bus,align_resource}
While upstreaming the RISC-V port, it was pointed out that multiple
architectures (arc, arm64, cris, microblaze, sh, tile) have copied the
mostly empty versions of at least one of these functions. This defines
weakly bound versions of the common functions so other architetures can
use them.
Signed-off-by: Palmer Dabbelt
---
drivers/pci/Makefile | 2 +-
drivers/pci/bios.c | 42 ++
2 files changed, 43 insertions(+), 1 deletion(-)
create mode 100644 drivers/pci/bios.c
diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
index a29d9ec05d13..fa7040915194 100644
--- a/drivers/pci/Makefile
+++ b/drivers/pci/Makefile
@@ -4,7 +4,7 @@
obj-y += access.o bus.o probe.o host-bridge.o remove.o pci.o \
pci-driver.o search.o pci-sysfs.o rom.o setup-res.o \
- irq.o vpd.o setup-bus.o vc.o mmap.o
+ irq.o vpd.o setup-bus.o vc.o mmap.o bios.o
obj-$(CONFIG_PROC_FS) += proc.o
obj-$(CONFIG_SYSFS) += slot.o
diff --git a/drivers/pci/bios.c b/drivers/pci/bios.c
new file mode 100644
index ..ffe34c024aa8
--- /dev/null
+++ b/drivers/pci/bios.c
@@ -0,0 +1,42 @@
+/*
+ * Code borrowed from arch/arm64/kernel/pci.c
+ * which borrowed from powerpc/kernel/pci-common.c
+ * which borrowed from arch/alpha/kernel/pci.c
+ *
+ * Extruded from code written by
+ * Dave Rusling (david.rusl...@reo.mts.dec.com)
+ * David Mosberger (dav...@cs.arizona.edu)
+ * Copyright (C) 1999 Andrea Arcangeli
+ * Copyright (C) 2000 Ivan Kokshaysky
+ * Copyright (C) 2003 Anton Blanchard , IBM
+ * Copyright (C) 2014 ARM Ltd.
+ * Copyright (C) 2017 SiFive
+ *
+ * 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.
+ */
+
+/* This file contains weakly bound functions that implement pcibios functions
+ * that some architectures have copied verbatim.
+ */
+
+#include
+
+/*
+ * Called after each bus is probed, but before its children are examined
+ */
+__attribute__ ((weak))
+void pcibios_fixup_bus(struct pci_bus *bus)
+{
+ /* nothing to do, expected to be removed in the future */
+}
+/*
+ * We don't have to worry about legacy ISA devices, so nothing to do here
+ */
+__attribute__ ((weak))
+resource_size_t pcibios_align_resource(void *data, const struct resource *res,
+ resource_size_t size, resource_size_t
align)
+{
+ return res->start;
+}
--
2.13.0
