When reviewing the OpenRISC PCI support patch Arnd suggested that
we avoid copying arm64 and riscv asm/pci.h and moving that to be
the new asm-generic/pci.h.
This patch does that by first moving the old pci.h definition
of pci_get_legacy_ide_irq out to the architectures that use it,
this turns out to only be x86.
Next, we create the new pci.h definition.
Note, for the series, I am working on these fixups as part of adding PCI to
OpenRISC. If the series is accepted I would like to merge via the OpenRISC tree
with the OpenRISC PCI support patches to avoid having to coordinate upstream
merges.
Since v2:
- Remove pci_get_legacy_ide_irq from m68k.
Since v1:
- Remove definition of pci_get_legacy_ide_irq on architectures
not using CONFIG_PNP, which eliminated most.
- Add ifdef around PCIBIOS_MIN_MEM for consistency.
Stafford Horne (2):
asm-generic: Remove pci.h copying remaining code to x86
asm-generic: Add new pci.h and use it
arch/alpha/include/asm/pci.h | 1 -
arch/arm64/include/asm/pci.h | 12 +++------
arch/csky/include/asm/pci.h | 24 +++--------------
arch/ia64/include/asm/pci.h | 1 -
arch/m68k/include/asm/pci.h | 2 --
arch/powerpc/include/asm/pci.h | 1 -
arch/riscv/include/asm/pci.h | 25 +++---------------
arch/s390/include/asm/pci.h | 1 -
arch/sparc/include/asm/pci.h | 9 -------
arch/um/include/asm/pci.h | 24 ++---------------
arch/x86/include/asm/pci.h | 6 +++--
arch/xtensa/include/asm/pci.h | 3 ---
include/asm-generic/pci.h | 47 ++++++++++++++++++++++++----------
13 files changed, 49 insertions(+), 107 deletions(-)
--
2.36.1
The asm/pci.h used for many newer architectures share similar
definitions. Move the common parts to asm-generic/pci.h to allow for
sharing code.
Two things to note are:
- isa_dma_bridge_buggy, traditionally this is defined in asm/dma.h but
these architectures avoid creating that file and add the definition
to asm/pci.h.
- ARCH_GENERIC_PCI_MMAP_RESOURCE, csky does not define this so we
undefine it after including asm-generic/pci.h. Why doesn't csky
define it?
- pci_get_legacy_ide_irq, This function is only used on architectures
that support PNP. It is only maintained for arm64, in other
architectures it is removed.
Suggested-by: Arnd Bergmann <[email protected]>
Link: https://lore.kernel.org/lkml/CAK8P3a0JmPeczfmMBE__vn=Jbvf=nkbpVaZCycyv40pZNCJJXQ@mail.gmail.com/
Signed-off-by: Stafford Horne <[email protected]>
---
Second note on isa_dma_bridge_buggy, this is set on x86 but it it also set in
pci/quirks.c. We discussed limiting it only to x86 though as its a general
quick triggered by pci ids I think it will be more tricky than we thought so I
will leave as is. It might be nice to move it out of asm/dma.h and into
asm/pci.h though.
Since v2:
- Nothing
Since v1:
- Remove definition of pci_get_legacy_ide_irq
arch/arm64/include/asm/pci.h | 12 +++---------
arch/csky/include/asm/pci.h | 24 ++++--------------------
arch/riscv/include/asm/pci.h | 25 +++----------------------
arch/um/include/asm/pci.h | 24 ++----------------------
include/asm-generic/pci.h | 36 ++++++++++++++++++++++++++++++++++++
5 files changed, 48 insertions(+), 73 deletions(-)
create mode 100644 include/asm-generic/pci.h
diff --git a/arch/arm64/include/asm/pci.h b/arch/arm64/include/asm/pci.h
index b33ca260e3c9..1180e83712f5 100644
--- a/arch/arm64/include/asm/pci.h
+++ b/arch/arm64/include/asm/pci.h
@@ -9,7 +9,6 @@
#include <asm/io.h>
#define PCIBIOS_MIN_IO 0x1000
-#define PCIBIOS_MIN_MEM 0
/*
* Set to 1 if the kernel should re-assign all PCI bus numbers
@@ -18,9 +17,6 @@
(pci_has_flag(PCI_REASSIGN_ALL_BUS))
#define arch_can_pci_mmap_wc() 1
-#define ARCH_GENERIC_PCI_MMAP_RESOURCE 1
-
-extern int isa_dma_bridge_buggy;
#ifdef CONFIG_PCI
static inline int pci_get_legacy_ide_irq(struct pci_dev *dev, int channel)
@@ -28,11 +24,9 @@ static inline int pci_get_legacy_ide_irq(struct pci_dev *dev, int channel)
/* no legacy IRQ on arm64 */
return -ENODEV;
}
-
-static inline int pci_proc_domain(struct pci_bus *bus)
-{
- return 1;
-}
#endif /* CONFIG_PCI */
+/* Generic PCI */
+#include <asm-generic/pci.h>
+
#endif /* __ASM_PCI_H */
diff --git a/arch/csky/include/asm/pci.h b/arch/csky/include/asm/pci.h
index ebc765b1f78b..44866c1ad461 100644
--- a/arch/csky/include/asm/pci.h
+++ b/arch/csky/include/asm/pci.h
@@ -9,26 +9,10 @@
#include <asm/io.h>
-#define PCIBIOS_MIN_IO 0
-#define PCIBIOS_MIN_MEM 0
+/* Generic PCI */
+#include <asm-generic/pci.h>
-/* C-SKY shim does not initialize PCI bus */
-#define pcibios_assign_all_busses() 1
-
-extern int isa_dma_bridge_buggy;
-
-#ifdef CONFIG_PCI
-static inline int pci_get_legacy_ide_irq(struct pci_dev *dev, int channel)
-{
- /* no legacy IRQ on csky */
- return -ENODEV;
-}
-
-static inline int pci_proc_domain(struct pci_bus *bus)
-{
- /* always show the domain in /proc */
- return 1;
-}
-#endif /* CONFIG_PCI */
+/* csky doesn't use generic pci resource mapping */
+#undef ARCH_GENERIC_PCI_MMAP_RESOURCE
#endif /* __ASM_CSKY_PCI_H */
diff --git a/arch/riscv/include/asm/pci.h b/arch/riscv/include/asm/pci.h
index 7fd52a30e605..12ce8150cfb0 100644
--- a/arch/riscv/include/asm/pci.h
+++ b/arch/riscv/include/asm/pci.h
@@ -12,29 +12,7 @@
#include <asm/io.h>
-#define PCIBIOS_MIN_IO 0
-#define PCIBIOS_MIN_MEM 0
-
-/* RISC-V shim does not initialize PCI bus */
-#define pcibios_assign_all_busses() 1
-
-#define ARCH_GENERIC_PCI_MMAP_RESOURCE 1
-
-extern int isa_dma_bridge_buggy;
-
#ifdef CONFIG_PCI
-static inline int pci_get_legacy_ide_irq(struct pci_dev *dev, int channel)
-{
- /* no legacy IRQ on risc-v */
- return -ENODEV;
-}
-
-static inline int pci_proc_domain(struct pci_bus *bus)
-{
- /* always show the domain in /proc */
- return 1;
-}
-
#ifdef CONFIG_NUMA
static inline int pcibus_to_node(struct pci_bus *bus)
@@ -50,4 +28,7 @@ static inline int pcibus_to_node(struct pci_bus *bus)
#endif /* CONFIG_PCI */
+/* Generic PCI */
+#include <asm-generic/pci.h>
+
#endif /* _ASM_RISCV_PCI_H */
diff --git a/arch/um/include/asm/pci.h b/arch/um/include/asm/pci.h
index da13fd5519ef..34fe4921b5fa 100644
--- a/arch/um/include/asm/pci.h
+++ b/arch/um/include/asm/pci.h
@@ -4,28 +4,8 @@
#include <linux/types.h>
#include <asm/io.h>
-#define PCIBIOS_MIN_IO 0
-#define PCIBIOS_MIN_MEM 0
-
-#define pcibios_assign_all_busses() 1
-
-extern int isa_dma_bridge_buggy;
-
-#ifdef CONFIG_PCI
-static inline int pci_get_legacy_ide_irq(struct pci_dev *dev, int channel)
-{
- /* no legacy IRQs */
- return -ENODEV;
-}
-#endif
-
-#ifdef CONFIG_PCI_DOMAINS
-static inline int pci_proc_domain(struct pci_bus *bus)
-{
- /* always show the domain in /proc */
- return 1;
-}
-#endif /* CONFIG_PCI */
+/* Generic PCI */
+#include <asm-generic/pci.h>
#ifdef CONFIG_PCI_MSI_IRQ_DOMAIN
/*
diff --git a/include/asm-generic/pci.h b/include/asm-generic/pci.h
new file mode 100644
index 000000000000..fbc25741696a
--- /dev/null
+++ b/include/asm-generic/pci.h
@@ -0,0 +1,36 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef __ASM_GENERIC_PCI_H
+#define __ASM_GENERIC_PCI_H
+
+#include <linux/types.h>
+
+#ifndef PCIBIOS_MIN_IO
+#define PCIBIOS_MIN_IO 0
+#endif
+
+#ifndef PCIBIOS_MIN_MEM
+#define PCIBIOS_MIN_MEM 0
+#endif
+
+#ifndef pcibios_assign_all_busses
+/* For bootloaders that do not initialize the PCI bus */
+#define pcibios_assign_all_busses() 1
+#endif
+
+extern int isa_dma_bridge_buggy;
+
+/* Enable generic resource mapping code in drivers/pci/ */
+#define ARCH_GENERIC_PCI_MMAP_RESOURCE
+
+#ifdef CONFIG_PCI
+
+static inline int pci_proc_domain(struct pci_bus *bus)
+{
+ /* always show the domain in /proc */
+ return 1;
+}
+
+#endif /* CONFIG_PCI */
+
+#endif /* __ASM_GENERIC_PCI_H */
--
2.36.1
The generic pci.h header now only provides a definition of
pci_get_legacy_ide_irq which is used by architectures that support PNP.
Of the architectures that use asm-generic/pci.h this is only x86.
This patch removes the old pci.h in order to make room for a new
pci.h to be used by arm64, riscv, openrisc, etc.
The existing code in pci.h is moved out to x86. On other architectures
we clean up any outstanding references.
Suggested-by: Arnd Bergmann <[email protected]>
Link: https://lore.kernel.org/lkml/CAK8P3a0JmPeczfmMBE__vn=Jbvf=nkbpVaZCycyv40pZNCJJXQ@mail.gmail.com/
Signed-off-by: Stafford Horne <[email protected]>
---
Since v2:
- Remove pci_get_legacy_ide_irq in m68k
Since v1:
- Remove pci_get_legacy_ide_irq for most architectures as its not needed.
arch/alpha/include/asm/pci.h | 1 -
arch/ia64/include/asm/pci.h | 1 -
arch/m68k/include/asm/pci.h | 2 --
arch/powerpc/include/asm/pci.h | 1 -
arch/s390/include/asm/pci.h | 1 -
arch/sparc/include/asm/pci.h | 9 ---------
arch/x86/include/asm/pci.h | 6 ++++--
arch/xtensa/include/asm/pci.h | 3 ---
include/asm-generic/pci.h | 17 -----------------
9 files changed, 4 insertions(+), 37 deletions(-)
delete mode 100644 include/asm-generic/pci.h
diff --git a/arch/alpha/include/asm/pci.h b/arch/alpha/include/asm/pci.h
index cf6bc1e64d66..8ac5af0fc4da 100644
--- a/arch/alpha/include/asm/pci.h
+++ b/arch/alpha/include/asm/pci.h
@@ -56,7 +56,6 @@ struct pci_controller {
/* IOMMU controls. */
-/* TODO: integrate with include/asm-generic/pci.h ? */
static inline int pci_get_legacy_ide_irq(struct pci_dev *dev, int channel)
{
return channel ? 15 : 14;
diff --git a/arch/ia64/include/asm/pci.h b/arch/ia64/include/asm/pci.h
index 8c163d1d0189..218412d963c2 100644
--- a/arch/ia64/include/asm/pci.h
+++ b/arch/ia64/include/asm/pci.h
@@ -63,7 +63,6 @@ static inline int pci_proc_domain(struct pci_bus *bus)
return (pci_domain_nr(bus) != 0);
}
-#define HAVE_ARCH_PCI_GET_LEGACY_IDE_IRQ
static inline int pci_get_legacy_ide_irq(struct pci_dev *dev, int channel)
{
return channel ? isa_irq_to_vector(15) : isa_irq_to_vector(14);
diff --git a/arch/m68k/include/asm/pci.h b/arch/m68k/include/asm/pci.h
index 5a4bc223743b..ccdfa0dc8413 100644
--- a/arch/m68k/include/asm/pci.h
+++ b/arch/m68k/include/asm/pci.h
@@ -2,8 +2,6 @@
#ifndef _ASM_M68K_PCI_H
#define _ASM_M68K_PCI_H
-#include <asm-generic/pci.h>
-
#define pcibios_assign_all_busses() 1
#define PCIBIOS_MIN_IO 0x00000100
diff --git a/arch/powerpc/include/asm/pci.h b/arch/powerpc/include/asm/pci.h
index 915d6ee4b40a..f9da506751bb 100644
--- a/arch/powerpc/include/asm/pci.h
+++ b/arch/powerpc/include/asm/pci.h
@@ -39,7 +39,6 @@
#define pcibios_assign_all_busses() \
(pci_has_flag(PCI_REASSIGN_ALL_BUS))
-#define HAVE_ARCH_PCI_GET_LEGACY_IDE_IRQ
static inline int pci_get_legacy_ide_irq(struct pci_dev *dev, int channel)
{
if (ppc_md.pci_get_legacy_ide_irq)
diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h
index fdb9745ee998..5889ddcbc374 100644
--- a/arch/s390/include/asm/pci.h
+++ b/arch/s390/include/asm/pci.h
@@ -6,7 +6,6 @@
#include <linux/mutex.h>
#include <linux/iommu.h>
#include <linux/pci_hotplug.h>
-#include <asm-generic/pci.h>
#include <asm/pci_clp.h>
#include <asm/pci_debug.h>
#include <asm/sclp.h>
diff --git a/arch/sparc/include/asm/pci.h b/arch/sparc/include/asm/pci.h
index 4deddf430e5d..0c58f65bd172 100644
--- a/arch/sparc/include/asm/pci.h
+++ b/arch/sparc/include/asm/pci.h
@@ -40,13 +40,4 @@ static inline int pci_proc_domain(struct pci_bus *bus)
#define get_pci_unmapped_area get_fb_unmapped_area
#endif /* CONFIG_SPARC64 */
-#if defined(CONFIG_SPARC64) || defined(CONFIG_LEON_PCI)
-static inline int pci_get_legacy_ide_irq(struct pci_dev *dev, int channel)
-{
- return PCI_IRQ_NONE;
-}
-#else
-#include <asm-generic/pci.h>
-#endif
-
#endif /* ___ASM_SPARC_PCI_H */
diff --git a/arch/x86/include/asm/pci.h b/arch/x86/include/asm/pci.h
index f3fd5928bcbb..7da27f665cfe 100644
--- a/arch/x86/include/asm/pci.h
+++ b/arch/x86/include/asm/pci.h
@@ -105,8 +105,10 @@ static inline void early_quirks(void) { }
extern void pci_iommu_alloc(void);
-/* generic pci stuff */
-#include <asm-generic/pci.h>
+static inline int pci_get_legacy_ide_irq(struct pci_dev *dev, int channel)
+{
+ return channel ? 15 : 14;
+}
#ifdef CONFIG_NUMA
/* Returns the node based on pci bus */
diff --git a/arch/xtensa/include/asm/pci.h b/arch/xtensa/include/asm/pci.h
index 8e2b48a268db..b56de9635b6c 100644
--- a/arch/xtensa/include/asm/pci.h
+++ b/arch/xtensa/include/asm/pci.h
@@ -43,7 +43,4 @@
#define ARCH_GENERIC_PCI_MMAP_RESOURCE 1
#define arch_can_pci_mmap_io() 1
-/* Generic PCI */
-#include <asm-generic/pci.h>
-
#endif /* _XTENSA_PCI_H */
diff --git a/include/asm-generic/pci.h b/include/asm-generic/pci.h
deleted file mode 100644
index 6bb3cd3d695a..000000000000
--- a/include/asm-generic/pci.h
+++ /dev/null
@@ -1,17 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-/*
- * linux/include/asm-generic/pci.h
- *
- * Copyright (C) 2003 Russell King
- */
-#ifndef _ASM_GENERIC_PCI_H
-#define _ASM_GENERIC_PCI_H
-
-#ifndef HAVE_ARCH_PCI_GET_LEGACY_IDE_IRQ
-static inline int pci_get_legacy_ide_irq(struct pci_dev *dev, int channel)
-{
- return channel ? 15 : 14;
-}
-#endif /* HAVE_ARCH_PCI_GET_LEGACY_IDE_IRQ */
-
-#endif /* _ASM_GENERIC_PCI_H */
--
2.36.1
On Mon, Jul 18, 2022 at 2:41 AM Stafford Horne <[email protected]> wrote:
> The generic pci.h header now only provides a definition of
> pci_get_legacy_ide_irq which is used by architectures that support PNP.
> Of the architectures that use asm-generic/pci.h this is only x86.
>
> This patch removes the old pci.h in order to make room for a new
> pci.h to be used by arm64, riscv, openrisc, etc.
>
> The existing code in pci.h is moved out to x86. On other architectures
> we clean up any outstanding references.
>
> Suggested-by: Arnd Bergmann <[email protected]>
> Link: https://lore.kernel.org/lkml/CAK8P3a0JmPeczfmMBE__vn=Jbvf=nkbpVaZCycyv40pZNCJJXQ@mail.gmail.com/
> Signed-off-by: Stafford Horne <[email protected]>
> ---
> Since v2:
> - Remove pci_get_legacy_ide_irq in m68k
> Since v1:
> - Remove pci_get_legacy_ide_irq for most architectures as its not needed.
> arch/m68k/include/asm/pci.h | 2 --
Acked-by: Geert Uytterhoeven <[email protected]>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On 7/18/22 02:41, Stafford Horne wrote:
> The generic pci.h header now only provides a definition of
> pci_get_legacy_ide_irq which is used by architectures that support PNP.
> Of the architectures that use asm-generic/pci.h this is only x86.
>
> This patch removes the old pci.h in order to make room for a new
> pci.h to be used by arm64, riscv, openrisc, etc.
>
> The existing code in pci.h is moved out to x86. On other architectures
> we clean up any outstanding references.
>
> Suggested-by: Arnd Bergmann <[email protected]>
> Link: https://lore.kernel.org/lkml/CAK8P3a0JmPeczfmMBE__vn=Jbvf=nkbpVaZCycyv40pZNCJJXQ@mail.gmail.com/
> Signed-off-by: Stafford Horne <[email protected]>
> ---
> Since v2:
> - Remove pci_get_legacy_ide_irq in m68k
> Since v1:
> - Remove pci_get_legacy_ide_irq for most architectures as its not needed.
>
> arch/alpha/include/asm/pci.h | 1 -
> arch/ia64/include/asm/pci.h | 1 -
> arch/m68k/include/asm/pci.h | 2 --
> arch/powerpc/include/asm/pci.h | 1 -
> arch/s390/include/asm/pci.h | 1 -
> arch/sparc/include/asm/pci.h | 9 ---------
> arch/x86/include/asm/pci.h | 6 ++++--
> arch/xtensa/include/asm/pci.h | 3 ---
> include/asm-generic/pci.h | 17 -----------------
> 9 files changed, 4 insertions(+), 37 deletions(-)
> delete mode 100644 include/asm-generic/pci.h
>
> diff --git a/arch/alpha/include/asm/pci.h b/arch/alpha/include/asm/pci.h
> index cf6bc1e64d66..8ac5af0fc4da 100644
> --- a/arch/alpha/include/asm/pci.h
> +++ b/arch/alpha/include/asm/pci.h
> @@ -56,7 +56,6 @@ struct pci_controller {
>
> /* IOMMU controls. */
>
> -/* TODO: integrate with include/asm-generic/pci.h ? */
> static inline int pci_get_legacy_ide_irq(struct pci_dev *dev, int channel)
> {
> return channel ? 15 : 14;
> diff --git a/arch/ia64/include/asm/pci.h b/arch/ia64/include/asm/pci.h
> index 8c163d1d0189..218412d963c2 100644
> --- a/arch/ia64/include/asm/pci.h
> +++ b/arch/ia64/include/asm/pci.h
> @@ -63,7 +63,6 @@ static inline int pci_proc_domain(struct pci_bus *bus)
> return (pci_domain_nr(bus) != 0);
> }
>
> -#define HAVE_ARCH_PCI_GET_LEGACY_IDE_IRQ
> static inline int pci_get_legacy_ide_irq(struct pci_dev *dev, int channel)
> {
> return channel ? isa_irq_to_vector(15) : isa_irq_to_vector(14);
> diff --git a/arch/m68k/include/asm/pci.h b/arch/m68k/include/asm/pci.h
> index 5a4bc223743b..ccdfa0dc8413 100644
> --- a/arch/m68k/include/asm/pci.h
> +++ b/arch/m68k/include/asm/pci.h
> @@ -2,8 +2,6 @@
> #ifndef _ASM_M68K_PCI_H
> #define _ASM_M68K_PCI_H
>
> -#include <asm-generic/pci.h>
> -
> #define pcibios_assign_all_busses() 1
>
> #define PCIBIOS_MIN_IO 0x00000100
> diff --git a/arch/powerpc/include/asm/pci.h b/arch/powerpc/include/asm/pci.h
> index 915d6ee4b40a..f9da506751bb 100644
> --- a/arch/powerpc/include/asm/pci.h
> +++ b/arch/powerpc/include/asm/pci.h
> @@ -39,7 +39,6 @@
> #define pcibios_assign_all_busses() \
> (pci_has_flag(PCI_REASSIGN_ALL_BUS))
>
> -#define HAVE_ARCH_PCI_GET_LEGACY_IDE_IRQ
> static inline int pci_get_legacy_ide_irq(struct pci_dev *dev, int channel)
> {
> if (ppc_md.pci_get_legacy_ide_irq)
> diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h
> index fdb9745ee998..5889ddcbc374 100644
> --- a/arch/s390/include/asm/pci.h
> +++ b/arch/s390/include/asm/pci.h
> @@ -6,7 +6,6 @@
> #include <linux/mutex.h>
> #include <linux/iommu.h>
> #include <linux/pci_hotplug.h>
> -#include <asm-generic/pci.h>
> #include <asm/pci_clp.h>
> #include <asm/pci_debug.h>
> #include <asm/sclp.h>
Did not notice any problem for S390.
Acked-by: Pierre Morel <[email protected]>
--
Pierre Morel
IBM Lab Boeblingen
On Sun, 17 Jul 2022 17:41:14 PDT (-0700), [email protected] wrote:
> The asm/pci.h used for many newer architectures share similar
> definitions. Move the common parts to asm-generic/pci.h to allow for
> sharing code.
>
> Two things to note are:
>
> - isa_dma_bridge_buggy, traditionally this is defined in asm/dma.h but
> these architectures avoid creating that file and add the definition
> to asm/pci.h.
> - ARCH_GENERIC_PCI_MMAP_RESOURCE, csky does not define this so we
> undefine it after including asm-generic/pci.h. Why doesn't csky
> define it?
> - pci_get_legacy_ide_irq, This function is only used on architectures
> that support PNP. It is only maintained for arm64, in other
> architectures it is removed.
>
> Suggested-by: Arnd Bergmann <[email protected]>
> Link: https://lore.kernel.org/lkml/CAK8P3a0JmPeczfmMBE__vn=Jbvf=nkbpVaZCycyv40pZNCJJXQ@mail.gmail.com/
> Signed-off-by: Stafford Horne <[email protected]>
> ---
> Second note on isa_dma_bridge_buggy, this is set on x86 but it it also set in
> pci/quirks.c. We discussed limiting it only to x86 though as its a general
> quick triggered by pci ids I think it will be more tricky than we thought so I
> will leave as is. It might be nice to move it out of asm/dma.h and into
> asm/pci.h though.
>
> Since v2:
> - Nothing
> Since v1:
> - Remove definition of pci_get_legacy_ide_irq
>
> arch/arm64/include/asm/pci.h | 12 +++---------
> arch/csky/include/asm/pci.h | 24 ++++--------------------
> arch/riscv/include/asm/pci.h | 25 +++----------------------
> arch/um/include/asm/pci.h | 24 ++----------------------
> include/asm-generic/pci.h | 36 ++++++++++++++++++++++++++++++++++++
> 5 files changed, 48 insertions(+), 73 deletions(-)
> create mode 100644 include/asm-generic/pci.h
>
> diff --git a/arch/arm64/include/asm/pci.h b/arch/arm64/include/asm/pci.h
> index b33ca260e3c9..1180e83712f5 100644
> --- a/arch/arm64/include/asm/pci.h
> +++ b/arch/arm64/include/asm/pci.h
> @@ -9,7 +9,6 @@
> #include <asm/io.h>
>
> #define PCIBIOS_MIN_IO 0x1000
> -#define PCIBIOS_MIN_MEM 0
>
> /*
> * Set to 1 if the kernel should re-assign all PCI bus numbers
> @@ -18,9 +17,6 @@
> (pci_has_flag(PCI_REASSIGN_ALL_BUS))
>
> #define arch_can_pci_mmap_wc() 1
> -#define ARCH_GENERIC_PCI_MMAP_RESOURCE 1
> -
> -extern int isa_dma_bridge_buggy;
>
> #ifdef CONFIG_PCI
> static inline int pci_get_legacy_ide_irq(struct pci_dev *dev, int channel)
> @@ -28,11 +24,9 @@ static inline int pci_get_legacy_ide_irq(struct pci_dev *dev, int channel)
> /* no legacy IRQ on arm64 */
> return -ENODEV;
> }
> -
> -static inline int pci_proc_domain(struct pci_bus *bus)
> -{
> - return 1;
> -}
> #endif /* CONFIG_PCI */
>
> +/* Generic PCI */
> +#include <asm-generic/pci.h>
> +
> #endif /* __ASM_PCI_H */
> diff --git a/arch/csky/include/asm/pci.h b/arch/csky/include/asm/pci.h
> index ebc765b1f78b..44866c1ad461 100644
> --- a/arch/csky/include/asm/pci.h
> +++ b/arch/csky/include/asm/pci.h
> @@ -9,26 +9,10 @@
>
> #include <asm/io.h>
>
> -#define PCIBIOS_MIN_IO 0
> -#define PCIBIOS_MIN_MEM 0
> +/* Generic PCI */
> +#include <asm-generic/pci.h>
>
> -/* C-SKY shim does not initialize PCI bus */
> -#define pcibios_assign_all_busses() 1
> -
> -extern int isa_dma_bridge_buggy;
> -
> -#ifdef CONFIG_PCI
> -static inline int pci_get_legacy_ide_irq(struct pci_dev *dev, int channel)
> -{
> - /* no legacy IRQ on csky */
> - return -ENODEV;
> -}
> -
> -static inline int pci_proc_domain(struct pci_bus *bus)
> -{
> - /* always show the domain in /proc */
> - return 1;
> -}
> -#endif /* CONFIG_PCI */
> +/* csky doesn't use generic pci resource mapping */
> +#undef ARCH_GENERIC_PCI_MMAP_RESOURCE
>
> #endif /* __ASM_CSKY_PCI_H */
> diff --git a/arch/riscv/include/asm/pci.h b/arch/riscv/include/asm/pci.h
> index 7fd52a30e605..12ce8150cfb0 100644
> --- a/arch/riscv/include/asm/pci.h
> +++ b/arch/riscv/include/asm/pci.h
> @@ -12,29 +12,7 @@
>
> #include <asm/io.h>
>
> -#define PCIBIOS_MIN_IO 0
> -#define PCIBIOS_MIN_MEM 0
My for-next changes these in bb356ddb78b2 ("RISC-V: PCI: Avoid handing
out address 0 to devices"). Do you mind either splitting out the
arch/riscv bits or having this in via some sort of shared tag?
> -
> -/* RISC-V shim does not initialize PCI bus */
> -#define pcibios_assign_all_busses() 1
> -
> -#define ARCH_GENERIC_PCI_MMAP_RESOURCE 1
> -
> -extern int isa_dma_bridge_buggy;
> -
> #ifdef CONFIG_PCI
> -static inline int pci_get_legacy_ide_irq(struct pci_dev *dev, int channel)
> -{
> - /* no legacy IRQ on risc-v */
> - return -ENODEV;
> -}
> -
> -static inline int pci_proc_domain(struct pci_bus *bus)
> -{
> - /* always show the domain in /proc */
> - return 1;
> -}
> -
> #ifdef CONFIG_NUMA
>
> static inline int pcibus_to_node(struct pci_bus *bus)
> @@ -50,4 +28,7 @@ static inline int pcibus_to_node(struct pci_bus *bus)
>
> #endif /* CONFIG_PCI */
>
> +/* Generic PCI */
> +#include <asm-generic/pci.h>
> +
> #endif /* _ASM_RISCV_PCI_H */
> diff --git a/arch/um/include/asm/pci.h b/arch/um/include/asm/pci.h
> index da13fd5519ef..34fe4921b5fa 100644
> --- a/arch/um/include/asm/pci.h
> +++ b/arch/um/include/asm/pci.h
> @@ -4,28 +4,8 @@
> #include <linux/types.h>
> #include <asm/io.h>
>
> -#define PCIBIOS_MIN_IO 0
> -#define PCIBIOS_MIN_MEM 0
> -
> -#define pcibios_assign_all_busses() 1
> -
> -extern int isa_dma_bridge_buggy;
> -
> -#ifdef CONFIG_PCI
> -static inline int pci_get_legacy_ide_irq(struct pci_dev *dev, int channel)
> -{
> - /* no legacy IRQs */
> - return -ENODEV;
> -}
> -#endif
> -
> -#ifdef CONFIG_PCI_DOMAINS
> -static inline int pci_proc_domain(struct pci_bus *bus)
> -{
> - /* always show the domain in /proc */
> - return 1;
> -}
> -#endif /* CONFIG_PCI */
> +/* Generic PCI */
> +#include <asm-generic/pci.h>
>
> #ifdef CONFIG_PCI_MSI_IRQ_DOMAIN
> /*
> diff --git a/include/asm-generic/pci.h b/include/asm-generic/pci.h
> new file mode 100644
> index 000000000000..fbc25741696a
> --- /dev/null
> +++ b/include/asm-generic/pci.h
> @@ -0,0 +1,36 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#ifndef __ASM_GENERIC_PCI_H
> +#define __ASM_GENERIC_PCI_H
> +
> +#include <linux/types.h>
> +
> +#ifndef PCIBIOS_MIN_IO
> +#define PCIBIOS_MIN_IO 0
> +#endif
> +
> +#ifndef PCIBIOS_MIN_MEM
> +#define PCIBIOS_MIN_MEM 0
> +#endif
> +
> +#ifndef pcibios_assign_all_busses
> +/* For bootloaders that do not initialize the PCI bus */
> +#define pcibios_assign_all_busses() 1
> +#endif
> +
> +extern int isa_dma_bridge_buggy;
> +
> +/* Enable generic resource mapping code in drivers/pci/ */
> +#define ARCH_GENERIC_PCI_MMAP_RESOURCE
> +
> +#ifdef CONFIG_PCI
> +
> +static inline int pci_proc_domain(struct pci_bus *bus)
> +{
> + /* always show the domain in /proc */
> + return 1;
> +}
> +
> +#endif /* CONFIG_PCI */
> +
> +#endif /* __ASM_GENERIC_PCI_H */
On Tue, Jul 19, 2022 at 08:58:39AM -0700, Palmer Dabbelt wrote:
> On Sun, 17 Jul 2022 17:41:14 PDT (-0700), [email protected] wrote:
> > The asm/pci.h used for many newer architectures share similar
> > definitions. Move the common parts to asm-generic/pci.h to allow for
> > sharing code.
> >
> > Two things to note are:
> >
> > - isa_dma_bridge_buggy, traditionally this is defined in asm/dma.h but
> > these architectures avoid creating that file and add the definition
> > to asm/pci.h.
> > - ARCH_GENERIC_PCI_MMAP_RESOURCE, csky does not define this so we
> > undefine it after including asm-generic/pci.h. Why doesn't csky
> > define it?
> > - pci_get_legacy_ide_irq, This function is only used on architectures
> > that support PNP. It is only maintained for arm64, in other
> > architectures it is removed.
> >
> > Suggested-by: Arnd Bergmann <[email protected]>
> > Link: https://lore.kernel.org/lkml/CAK8P3a0JmPeczfmMBE__vn=Jbvf=nkbpVaZCycyv40pZNCJJXQ@mail.gmail.com/
> > Signed-off-by: Stafford Horne <[email protected]>
> > ---
> > Second note on isa_dma_bridge_buggy, this is set on x86 but it it also set in
> > pci/quirks.c. We discussed limiting it only to x86 though as its a general
> > quick triggered by pci ids I think it will be more tricky than we thought so I
> > will leave as is. It might be nice to move it out of asm/dma.h and into
> > asm/pci.h though.
> >
> > Since v2:
> > - Nothing
> > Since v1:
> > - Remove definition of pci_get_legacy_ide_irq
> >
> > arch/arm64/include/asm/pci.h | 12 +++---------
> > arch/csky/include/asm/pci.h | 24 ++++--------------------
> > arch/riscv/include/asm/pci.h | 25 +++----------------------
> > arch/um/include/asm/pci.h | 24 ++----------------------
> > include/asm-generic/pci.h | 36 ++++++++++++++++++++++++++++++++++++
> > 5 files changed, 48 insertions(+), 73 deletions(-)
> > create mode 100644 include/asm-generic/pci.h
> >
> > diff --git a/arch/arm64/include/asm/pci.h b/arch/arm64/include/asm/pci.h
> > index b33ca260e3c9..1180e83712f5 100644
> > --- a/arch/arm64/include/asm/pci.h
> > +++ b/arch/arm64/include/asm/pci.h
> > @@ -9,7 +9,6 @@
> > #include <asm/io.h>
> >
> > #define PCIBIOS_MIN_IO 0x1000
> > -#define PCIBIOS_MIN_MEM 0
> >
> > /*
> > * Set to 1 if the kernel should re-assign all PCI bus numbers
> > @@ -18,9 +17,6 @@
> > (pci_has_flag(PCI_REASSIGN_ALL_BUS))
> >
> > #define arch_can_pci_mmap_wc() 1
> > -#define ARCH_GENERIC_PCI_MMAP_RESOURCE 1
> > -
> > -extern int isa_dma_bridge_buggy;
> >
> > #ifdef CONFIG_PCI
> > static inline int pci_get_legacy_ide_irq(struct pci_dev *dev, int channel)
> > @@ -28,11 +24,9 @@ static inline int pci_get_legacy_ide_irq(struct pci_dev *dev, int channel)
> > /* no legacy IRQ on arm64 */
> > return -ENODEV;
> > }
> > -
> > -static inline int pci_proc_domain(struct pci_bus *bus)
> > -{
> > - return 1;
> > -}
> > #endif /* CONFIG_PCI */
> >
> > +/* Generic PCI */
> > +#include <asm-generic/pci.h>
> > +
> > #endif /* __ASM_PCI_H */
> > diff --git a/arch/csky/include/asm/pci.h b/arch/csky/include/asm/pci.h
> > index ebc765b1f78b..44866c1ad461 100644
> > --- a/arch/csky/include/asm/pci.h
> > +++ b/arch/csky/include/asm/pci.h
> > @@ -9,26 +9,10 @@
> >
> > #include <asm/io.h>
> >
> > -#define PCIBIOS_MIN_IO 0
> > -#define PCIBIOS_MIN_MEM 0
> > +/* Generic PCI */
> > +#include <asm-generic/pci.h>
> >
> > -/* C-SKY shim does not initialize PCI bus */
> > -#define pcibios_assign_all_busses() 1
> > -
> > -extern int isa_dma_bridge_buggy;
> > -
> > -#ifdef CONFIG_PCI
> > -static inline int pci_get_legacy_ide_irq(struct pci_dev *dev, int channel)
> > -{
> > - /* no legacy IRQ on csky */
> > - return -ENODEV;
> > -}
> > -
> > -static inline int pci_proc_domain(struct pci_bus *bus)
> > -{
> > - /* always show the domain in /proc */
> > - return 1;
> > -}
> > -#endif /* CONFIG_PCI */
> > +/* csky doesn't use generic pci resource mapping */
> > +#undef ARCH_GENERIC_PCI_MMAP_RESOURCE
> >
> > #endif /* __ASM_CSKY_PCI_H */
> > diff --git a/arch/riscv/include/asm/pci.h b/arch/riscv/include/asm/pci.h
> > index 7fd52a30e605..12ce8150cfb0 100644
> > --- a/arch/riscv/include/asm/pci.h
> > +++ b/arch/riscv/include/asm/pci.h
> > @@ -12,29 +12,7 @@
> >
> > #include <asm/io.h>
> >
> > -#define PCIBIOS_MIN_IO 0
> > -#define PCIBIOS_MIN_MEM 0
>
> My for-next changes these in bb356ddb78b2 ("RISC-V: PCI: Avoid handing out
> address 0 to devices"). Do you mind either splitting out the arch/riscv
> bits or having this in via some sort of shared tag?
Hi Palmer,
I replied last on my phone but since it produces HTML multi-part email it got
rejected from a few places and I am not sure if you saw my reply.
It might be a bit hard to separate out the architecture specific bits as it
requires a bit of coordination with the asm-generic/pci.h move.
Some options:
1 I can produce a tag for you to merge into your for-next.
2 I can skip touching riscv at all in this patch and you can take care of it
in a future patch.
3 I could cherry pick your change bb356ddb78b2 ("RISC-V: PCI: Avoid handing out
address 0 to devices") to my for-next branch. It should help avoid conflict.
Seeing that I am still doing small updates here and there I am not sure when I
will be ablt to create a stable tag. So maybe 3 would work best?
-Stafford
On Thu, 21 Jul 2022 15:05:34 PDT (-0700), [email protected] wrote:
> On Tue, Jul 19, 2022 at 08:58:39AM -0700, Palmer Dabbelt wrote:
>> On Sun, 17 Jul 2022 17:41:14 PDT (-0700), [email protected] wrote:
>> > The asm/pci.h used for many newer architectures share similar
>> > definitions. Move the common parts to asm-generic/pci.h to allow for
>> > sharing code.
>> >
>> > Two things to note are:
>> >
>> > - isa_dma_bridge_buggy, traditionally this is defined in asm/dma.h but
>> > these architectures avoid creating that file and add the definition
>> > to asm/pci.h.
>> > - ARCH_GENERIC_PCI_MMAP_RESOURCE, csky does not define this so we
>> > undefine it after including asm-generic/pci.h. Why doesn't csky
>> > define it?
>> > - pci_get_legacy_ide_irq, This function is only used on architectures
>> > that support PNP. It is only maintained for arm64, in other
>> > architectures it is removed.
>> >
>> > Suggested-by: Arnd Bergmann <[email protected]>
>> > Link: https://lore.kernel.org/lkml/CAK8P3a0JmPeczfmMBE__vn=Jbvf=nkbpVaZCycyv40pZNCJJXQ@mail.gmail.com/
>> > Signed-off-by: Stafford Horne <[email protected]>
>> > ---
>> > Second note on isa_dma_bridge_buggy, this is set on x86 but it it also set in
>> > pci/quirks.c. We discussed limiting it only to x86 though as its a general
>> > quick triggered by pci ids I think it will be more tricky than we thought so I
>> > will leave as is. It might be nice to move it out of asm/dma.h and into
>> > asm/pci.h though.
>> >
>> > Since v2:
>> > - Nothing
>> > Since v1:
>> > - Remove definition of pci_get_legacy_ide_irq
>> >
>> > arch/arm64/include/asm/pci.h | 12 +++---------
>> > arch/csky/include/asm/pci.h | 24 ++++--------------------
>> > arch/riscv/include/asm/pci.h | 25 +++----------------------
>> > arch/um/include/asm/pci.h | 24 ++----------------------
>> > include/asm-generic/pci.h | 36 ++++++++++++++++++++++++++++++++++++
>> > 5 files changed, 48 insertions(+), 73 deletions(-)
>> > create mode 100644 include/asm-generic/pci.h
>> >
>> > diff --git a/arch/arm64/include/asm/pci.h b/arch/arm64/include/asm/pci.h
>> > index b33ca260e3c9..1180e83712f5 100644
>> > --- a/arch/arm64/include/asm/pci.h
>> > +++ b/arch/arm64/include/asm/pci.h
>> > @@ -9,7 +9,6 @@
>> > #include <asm/io.h>
>> >
>> > #define PCIBIOS_MIN_IO 0x1000
>> > -#define PCIBIOS_MIN_MEM 0
>> >
>> > /*
>> > * Set to 1 if the kernel should re-assign all PCI bus numbers
>> > @@ -18,9 +17,6 @@
>> > (pci_has_flag(PCI_REASSIGN_ALL_BUS))
>> >
>> > #define arch_can_pci_mmap_wc() 1
>> > -#define ARCH_GENERIC_PCI_MMAP_RESOURCE 1
>> > -
>> > -extern int isa_dma_bridge_buggy;
>> >
>> > #ifdef CONFIG_PCI
>> > static inline int pci_get_legacy_ide_irq(struct pci_dev *dev, int channel)
>> > @@ -28,11 +24,9 @@ static inline int pci_get_legacy_ide_irq(struct pci_dev *dev, int channel)
>> > /* no legacy IRQ on arm64 */
>> > return -ENODEV;
>> > }
>> > -
>> > -static inline int pci_proc_domain(struct pci_bus *bus)
>> > -{
>> > - return 1;
>> > -}
>> > #endif /* CONFIG_PCI */
>> >
>> > +/* Generic PCI */
>> > +#include <asm-generic/pci.h>
>> > +
>> > #endif /* __ASM_PCI_H */
>> > diff --git a/arch/csky/include/asm/pci.h b/arch/csky/include/asm/pci.h
>> > index ebc765b1f78b..44866c1ad461 100644
>> > --- a/arch/csky/include/asm/pci.h
>> > +++ b/arch/csky/include/asm/pci.h
>> > @@ -9,26 +9,10 @@
>> >
>> > #include <asm/io.h>
>> >
>> > -#define PCIBIOS_MIN_IO 0
>> > -#define PCIBIOS_MIN_MEM 0
>> > +/* Generic PCI */
>> > +#include <asm-generic/pci.h>
>> >
>> > -/* C-SKY shim does not initialize PCI bus */
>> > -#define pcibios_assign_all_busses() 1
>> > -
>> > -extern int isa_dma_bridge_buggy;
>> > -
>> > -#ifdef CONFIG_PCI
>> > -static inline int pci_get_legacy_ide_irq(struct pci_dev *dev, int channel)
>> > -{
>> > - /* no legacy IRQ on csky */
>> > - return -ENODEV;
>> > -}
>> > -
>> > -static inline int pci_proc_domain(struct pci_bus *bus)
>> > -{
>> > - /* always show the domain in /proc */
>> > - return 1;
>> > -}
>> > -#endif /* CONFIG_PCI */
>> > +/* csky doesn't use generic pci resource mapping */
>> > +#undef ARCH_GENERIC_PCI_MMAP_RESOURCE
>> >
>> > #endif /* __ASM_CSKY_PCI_H */
>> > diff --git a/arch/riscv/include/asm/pci.h b/arch/riscv/include/asm/pci.h
>> > index 7fd52a30e605..12ce8150cfb0 100644
>> > --- a/arch/riscv/include/asm/pci.h
>> > +++ b/arch/riscv/include/asm/pci.h
>> > @@ -12,29 +12,7 @@
>> >
>> > #include <asm/io.h>
>> >
>> > -#define PCIBIOS_MIN_IO 0
>> > -#define PCIBIOS_MIN_MEM 0
>>
>> My for-next changes these in bb356ddb78b2 ("RISC-V: PCI: Avoid handing out
>> address 0 to devices"). Do you mind either splitting out the arch/riscv
>> bits or having this in via some sort of shared tag?
>
> Hi Palmer,
>
> I replied last on my phone but since it produces HTML multi-part email it got
> rejected from a few places and I am not sure if you saw my reply.
>
> It might be a bit hard to separate out the architecture specific bits as it
> requires a bit of coordination with the asm-generic/pci.h move.
>
> Some options:
> 1 I can produce a tag for you to merge into your for-next.
> 2 I can skip touching riscv at all in this patch and you can take care of it
> in a future patch.
> 3 I could cherry pick your change bb356ddb78b2 ("RISC-V: PCI: Avoid handing out
> address 0 to devices") to my for-next branch. It should help avoid conflict.
>
> Seeing that I am still doing small updates here and there I am not sure when I
> will be ablt to create a stable tag. So maybe 3 would work best?
If you cherry pick it then we're going to both end up with the patch,
which IMO is worse than the conflict (and the patch is pretty far back,
so I don't really want to rebase everything this late).
Since this isn't in yet, would it be OK just landing it as a single
patch on top of 5.19-rc1 that gets merged into whatever tree it ends up
in (ie, yours or Arnd's)? Then as long as the commit is stable when it
lands in for-next I can just merge it down to my tree and sort out the
conflict from there.
If that's not possible then I think the best bet is to just drop the
RISC-V diff, I can deal with it when the rest lands in Linus' tree.
> -Stafford
On Tue, Jul 19, 2022 at 9:59 AM Palmer Dabbelt <[email protected]> wrote:
>
> On Sun, 17 Jul 2022 17:41:14 PDT (-0700), [email protected] wrote:
> > The asm/pci.h used for many newer architectures share similar
> > definitions. Move the common parts to asm-generic/pci.h to allow for
> > sharing code.
> >
> > Two things to note are:
> >
> > - isa_dma_bridge_buggy, traditionally this is defined in asm/dma.h but
> > these architectures avoid creating that file and add the definition
> > to asm/pci.h.
> > - ARCH_GENERIC_PCI_MMAP_RESOURCE, csky does not define this so we
> > undefine it after including asm-generic/pci.h. Why doesn't csky
> > define it?
> > - pci_get_legacy_ide_irq, This function is only used on architectures
> > that support PNP. It is only maintained for arm64, in other
> > architectures it is removed.
> >
> > Suggested-by: Arnd Bergmann <[email protected]>
> > Link: https://lore.kernel.org/lkml/CAK8P3a0JmPeczfmMBE__vn=Jbvf=nkbpVaZCycyv40pZNCJJXQ@mail.gmail.com/
> > Signed-off-by: Stafford Horne <[email protected]>
> > ---
> > Second note on isa_dma_bridge_buggy, this is set on x86 but it it also set in
> > pci/quirks.c. We discussed limiting it only to x86 though as its a general
> > quick triggered by pci ids I think it will be more tricky than we thought so I
> > will leave as is. It might be nice to move it out of asm/dma.h and into
> > asm/pci.h though.
> >
> > Since v2:
> > - Nothing
> > Since v1:
> > - Remove definition of pci_get_legacy_ide_irq
> >
> > arch/arm64/include/asm/pci.h | 12 +++---------
> > arch/csky/include/asm/pci.h | 24 ++++--------------------
> > arch/riscv/include/asm/pci.h | 25 +++----------------------
> > arch/um/include/asm/pci.h | 24 ++----------------------
> > include/asm-generic/pci.h | 36 ++++++++++++++++++++++++++++++++++++
> > 5 files changed, 48 insertions(+), 73 deletions(-)
> > create mode 100644 include/asm-generic/pci.h
> >
> > diff --git a/arch/arm64/include/asm/pci.h b/arch/arm64/include/asm/pci.h
> > index b33ca260e3c9..1180e83712f5 100644
> > --- a/arch/arm64/include/asm/pci.h
> > +++ b/arch/arm64/include/asm/pci.h
> > @@ -9,7 +9,6 @@
> > #include <asm/io.h>
> >
> > #define PCIBIOS_MIN_IO 0x1000
> > -#define PCIBIOS_MIN_MEM 0
> >
> > /*
> > * Set to 1 if the kernel should re-assign all PCI bus numbers
> > @@ -18,9 +17,6 @@
> > (pci_has_flag(PCI_REASSIGN_ALL_BUS))
> >
> > #define arch_can_pci_mmap_wc() 1
> > -#define ARCH_GENERIC_PCI_MMAP_RESOURCE 1
> > -
> > -extern int isa_dma_bridge_buggy;
> >
> > #ifdef CONFIG_PCI
> > static inline int pci_get_legacy_ide_irq(struct pci_dev *dev, int channel)
> > @@ -28,11 +24,9 @@ static inline int pci_get_legacy_ide_irq(struct pci_dev *dev, int channel)
> > /* no legacy IRQ on arm64 */
> > return -ENODEV;
> > }
> > -
> > -static inline int pci_proc_domain(struct pci_bus *bus)
> > -{
> > - return 1;
> > -}
> > #endif /* CONFIG_PCI */
> >
> > +/* Generic PCI */
> > +#include <asm-generic/pci.h>
> > +
> > #endif /* __ASM_PCI_H */
> > diff --git a/arch/csky/include/asm/pci.h b/arch/csky/include/asm/pci.h
> > index ebc765b1f78b..44866c1ad461 100644
> > --- a/arch/csky/include/asm/pci.h
> > +++ b/arch/csky/include/asm/pci.h
> > @@ -9,26 +9,10 @@
> >
> > #include <asm/io.h>
> >
> > -#define PCIBIOS_MIN_IO 0
> > -#define PCIBIOS_MIN_MEM 0
> > +/* Generic PCI */
> > +#include <asm-generic/pci.h>
> >
> > -/* C-SKY shim does not initialize PCI bus */
> > -#define pcibios_assign_all_busses() 1
> > -
> > -extern int isa_dma_bridge_buggy;
> > -
> > -#ifdef CONFIG_PCI
> > -static inline int pci_get_legacy_ide_irq(struct pci_dev *dev, int channel)
> > -{
> > - /* no legacy IRQ on csky */
> > - return -ENODEV;
> > -}
> > -
> > -static inline int pci_proc_domain(struct pci_bus *bus)
> > -{
> > - /* always show the domain in /proc */
> > - return 1;
> > -}
> > -#endif /* CONFIG_PCI */
> > +/* csky doesn't use generic pci resource mapping */
> > +#undef ARCH_GENERIC_PCI_MMAP_RESOURCE
> >
> > #endif /* __ASM_CSKY_PCI_H */
> > diff --git a/arch/riscv/include/asm/pci.h b/arch/riscv/include/asm/pci.h
> > index 7fd52a30e605..12ce8150cfb0 100644
> > --- a/arch/riscv/include/asm/pci.h
> > +++ b/arch/riscv/include/asm/pci.h
> > @@ -12,29 +12,7 @@
> >
> > #include <asm/io.h>
> >
> > -#define PCIBIOS_MIN_IO 0
> > -#define PCIBIOS_MIN_MEM 0
>
> My for-next changes these in bb356ddb78b2 ("RISC-V: PCI: Avoid handing
> out address 0 to devices"). Do you mind either splitting out the
> arch/riscv bits or having this in via some sort of shared tag?
Shouldn't the values not matter here if the IO and mem resources are
described in the DT (and don't use 0)? The values of 4 and 16 look
odd.
Rob
On Fri, Jul 22, 2022 at 1:06 AM Rob Herring <[email protected]> wrote:
> On Tue, Jul 19, 2022 at 9:59 AM Palmer Dabbelt <[email protected]> wrote:
> > On Sun, 17 Jul 2022 17:41:14 PDT (-0700), [email protected] wrote:
> > > diff --git a/arch/riscv/include/asm/pci.h b/arch/riscv/include/asm/pci.h
> > > index 7fd52a30e605..12ce8150cfb0 100644
> > > --- a/arch/riscv/include/asm/pci.h
> > > +++ b/arch/riscv/include/asm/pci.h
> > > @@ -12,29 +12,7 @@
> > >
> > > #include <asm/io.h>
> > >
> > > -#define PCIBIOS_MIN_IO 0
> > > -#define PCIBIOS_MIN_MEM 0
> >
> > My for-next changes these in bb356ddb78b2 ("RISC-V: PCI: Avoid handing
> > out address 0 to devices"). Do you mind either splitting out the
> > arch/riscv bits or having this in via some sort of shared tag?
>
> Shouldn't the values not matter here if the IO and mem resources are
> described in the DT (and don't use 0)? The values of 4 and 16 look
> odd.
I think it's different for the two types: For memory resources, this only
matters if the bus actually contains MMIO address zero. In most cases
the MMIO addresses are the same as the address seen by the CPU
and already nonzero based on the SoC design.
For the I/O port numbers, the port numbers tend to be more dynamic,
but you'd normally have addresses 0 through 0xffff on each PCI host
bridge with memory mapped I/O ports, so this can clearly happen.
Still, it seems better to not address the port zero issue in architecture
specific code but instead do it in the PCI core code. Ideally
we'd just use the 0x1000 minimum, which also helps stay out of
the ISA port numbers that may be used by things like
VGA or SATA adapters in legacy mode. The only reason I can
see for allowed smaller port numbers is for machines that have
a very limited I/O port window and do not have ports over
0x1000 at all.
Arnd
On Thu, 21 Jul 2022 16:06:52 PDT (-0700), Rob Herring wrote:
> On Tue, Jul 19, 2022 at 9:59 AM Palmer Dabbelt <[email protected]> wrote:
>>
>> On Sun, 17 Jul 2022 17:41:14 PDT (-0700), [email protected] wrote:
>> > The asm/pci.h used for many newer architectures share similar
>> > definitions. Move the common parts to asm-generic/pci.h to allow for
>> > sharing code.
>> >
>> > Two things to note are:
>> >
>> > - isa_dma_bridge_buggy, traditionally this is defined in asm/dma.h but
>> > these architectures avoid creating that file and add the definition
>> > to asm/pci.h.
>> > - ARCH_GENERIC_PCI_MMAP_RESOURCE, csky does not define this so we
>> > undefine it after including asm-generic/pci.h. Why doesn't csky
>> > define it?
>> > - pci_get_legacy_ide_irq, This function is only used on architectures
>> > that support PNP. It is only maintained for arm64, in other
>> > architectures it is removed.
>> >
>> > Suggested-by: Arnd Bergmann <[email protected]>
>> > Link: https://lore.kernel.org/lkml/CAK8P3a0JmPeczfmMBE__vn=Jbvf=nkbpVaZCycyv40pZNCJJXQ@mail.gmail.com/
>> > Signed-off-by: Stafford Horne <[email protected]>
>> > ---
>> > Second note on isa_dma_bridge_buggy, this is set on x86 but it it also set in
>> > pci/quirks.c. We discussed limiting it only to x86 though as its a general
>> > quick triggered by pci ids I think it will be more tricky than we thought so I
>> > will leave as is. It might be nice to move it out of asm/dma.h and into
>> > asm/pci.h though.
>> >
>> > Since v2:
>> > - Nothing
>> > Since v1:
>> > - Remove definition of pci_get_legacy_ide_irq
>> >
>> > arch/arm64/include/asm/pci.h | 12 +++---------
>> > arch/csky/include/asm/pci.h | 24 ++++--------------------
>> > arch/riscv/include/asm/pci.h | 25 +++----------------------
>> > arch/um/include/asm/pci.h | 24 ++----------------------
>> > include/asm-generic/pci.h | 36 ++++++++++++++++++++++++++++++++++++
>> > 5 files changed, 48 insertions(+), 73 deletions(-)
>> > create mode 100644 include/asm-generic/pci.h
>> >
>> > diff --git a/arch/arm64/include/asm/pci.h b/arch/arm64/include/asm/pci.h
>> > index b33ca260e3c9..1180e83712f5 100644
>> > --- a/arch/arm64/include/asm/pci.h
>> > +++ b/arch/arm64/include/asm/pci.h
>> > @@ -9,7 +9,6 @@
>> > #include <asm/io.h>
>> >
>> > #define PCIBIOS_MIN_IO 0x1000
>> > -#define PCIBIOS_MIN_MEM 0
>> >
>> > /*
>> > * Set to 1 if the kernel should re-assign all PCI bus numbers
>> > @@ -18,9 +17,6 @@
>> > (pci_has_flag(PCI_REASSIGN_ALL_BUS))
>> >
>> > #define arch_can_pci_mmap_wc() 1
>> > -#define ARCH_GENERIC_PCI_MMAP_RESOURCE 1
>> > -
>> > -extern int isa_dma_bridge_buggy;
>> >
>> > #ifdef CONFIG_PCI
>> > static inline int pci_get_legacy_ide_irq(struct pci_dev *dev, int channel)
>> > @@ -28,11 +24,9 @@ static inline int pci_get_legacy_ide_irq(struct pci_dev *dev, int channel)
>> > /* no legacy IRQ on arm64 */
>> > return -ENODEV;
>> > }
>> > -
>> > -static inline int pci_proc_domain(struct pci_bus *bus)
>> > -{
>> > - return 1;
>> > -}
>> > #endif /* CONFIG_PCI */
>> >
>> > +/* Generic PCI */
>> > +#include <asm-generic/pci.h>
>> > +
>> > #endif /* __ASM_PCI_H */
>> > diff --git a/arch/csky/include/asm/pci.h b/arch/csky/include/asm/pci.h
>> > index ebc765b1f78b..44866c1ad461 100644
>> > --- a/arch/csky/include/asm/pci.h
>> > +++ b/arch/csky/include/asm/pci.h
>> > @@ -9,26 +9,10 @@
>> >
>> > #include <asm/io.h>
>> >
>> > -#define PCIBIOS_MIN_IO 0
>> > -#define PCIBIOS_MIN_MEM 0
>> > +/* Generic PCI */
>> > +#include <asm-generic/pci.h>
>> >
>> > -/* C-SKY shim does not initialize PCI bus */
>> > -#define pcibios_assign_all_busses() 1
>> > -
>> > -extern int isa_dma_bridge_buggy;
>> > -
>> > -#ifdef CONFIG_PCI
>> > -static inline int pci_get_legacy_ide_irq(struct pci_dev *dev, int channel)
>> > -{
>> > - /* no legacy IRQ on csky */
>> > - return -ENODEV;
>> > -}
>> > -
>> > -static inline int pci_proc_domain(struct pci_bus *bus)
>> > -{
>> > - /* always show the domain in /proc */
>> > - return 1;
>> > -}
>> > -#endif /* CONFIG_PCI */
>> > +/* csky doesn't use generic pci resource mapping */
>> > +#undef ARCH_GENERIC_PCI_MMAP_RESOURCE
>> >
>> > #endif /* __ASM_CSKY_PCI_H */
>> > diff --git a/arch/riscv/include/asm/pci.h b/arch/riscv/include/asm/pci.h
>> > index 7fd52a30e605..12ce8150cfb0 100644
>> > --- a/arch/riscv/include/asm/pci.h
>> > +++ b/arch/riscv/include/asm/pci.h
>> > @@ -12,29 +12,7 @@
>> >
>> > #include <asm/io.h>
>> >
>> > -#define PCIBIOS_MIN_IO 0
>> > -#define PCIBIOS_MIN_MEM 0
>>
>> My for-next changes these in bb356ddb78b2 ("RISC-V: PCI: Avoid handing
>> out address 0 to devices"). Do you mind either splitting out the
>> arch/riscv bits or having this in via some sort of shared tag?
>
> Shouldn't the values not matter here if the IO and mem resources are
> described in the DT (and don't use 0)? The values of 4 and 16 look
> odd.
The linked thread has a fairly long discussion
<https://lore.kernel.org/all/[email protected]/>.
I agree it's odd to have this in arch code: "don't hand out address 0"
isn't really a RISC-V constraint (ie, we don't have architecture-defined
limitations on these address spaces) but a constraint that comes from
the generic port I/O functions and some other related PCI/resource code
where the value 0 is a sentinel.
Maybe the right thing to do here is actually to make the default
definitions of these macros non-zero, or to add some sort of ARCH_
flavor of them and move that non-zero requirement closer to where it
comes from? From the look of it any port that uses the generic port I/O
functions and has 0 for these will be broken in the same way.
That said, I'm not really a PCI guy so maybe Bjorn or Maciej has a
better idea?
On Fri, Jul 22, 2022 at 9:27 AM Palmer Dabbelt <[email protected]> wrote:
>
> On Thu, 21 Jul 2022 16:06:52 PDT (-0700), Rob Herring wrote:
> > On Tue, Jul 19, 2022 at 9:59 AM Palmer Dabbelt <[email protected]> wrote:
> >>
> >> On Sun, 17 Jul 2022 17:41:14 PDT (-0700), [email protected] wrote:
> >> > The asm/pci.h used for many newer architectures share similar
> >> > definitions. Move the common parts to asm-generic/pci.h to allow for
> >> > sharing code.
> >> >
> >> > Two things to note are:
> >> >
> >> > - isa_dma_bridge_buggy, traditionally this is defined in asm/dma.h but
> >> > these architectures avoid creating that file and add the definition
> >> > to asm/pci.h.
> >> > - ARCH_GENERIC_PCI_MMAP_RESOURCE, csky does not define this so we
> >> > undefine it after including asm-generic/pci.h. Why doesn't csky
> >> > define it?
> >> > - pci_get_legacy_ide_irq, This function is only used on architectures
> >> > that support PNP. It is only maintained for arm64, in other
> >> > architectures it is removed.
> >> >
> >> > Suggested-by: Arnd Bergmann <[email protected]>
> >> > Link: https://lore.kernel.org/lkml/CAK8P3a0JmPeczfmMBE__vn=Jbvf=nkbpVaZCycyv40pZNCJJXQ@mail.gmail.com/
> >> > Signed-off-by: Stafford Horne <[email protected]>
> >> > ---
> >> > Second note on isa_dma_bridge_buggy, this is set on x86 but it it also set in
> >> > pci/quirks.c. We discussed limiting it only to x86 though as its a general
> >> > quick triggered by pci ids I think it will be more tricky than we thought so I
> >> > will leave as is. It might be nice to move it out of asm/dma.h and into
> >> > asm/pci.h though.
> >> >
> >> > Since v2:
> >> > - Nothing
> >> > Since v1:
> >> > - Remove definition of pci_get_legacy_ide_irq
> >> >
> >> > arch/arm64/include/asm/pci.h | 12 +++---------
> >> > arch/csky/include/asm/pci.h | 24 ++++--------------------
> >> > arch/riscv/include/asm/pci.h | 25 +++----------------------
> >> > arch/um/include/asm/pci.h | 24 ++----------------------
> >> > include/asm-generic/pci.h | 36 ++++++++++++++++++++++++++++++++++++
> >> > 5 files changed, 48 insertions(+), 73 deletions(-)
> >> > create mode 100644 include/asm-generic/pci.h
> >> >
> >> > diff --git a/arch/arm64/include/asm/pci.h b/arch/arm64/include/asm/pci.h
> >> > index b33ca260e3c9..1180e83712f5 100644
> >> > --- a/arch/arm64/include/asm/pci.h
> >> > +++ b/arch/arm64/include/asm/pci.h
> >> > @@ -9,7 +9,6 @@
> >> > #include <asm/io.h>
> >> >
> >> > #define PCIBIOS_MIN_IO 0x1000
> >> > -#define PCIBIOS_MIN_MEM 0
> >> >
> >> > /*
> >> > * Set to 1 if the kernel should re-assign all PCI bus numbers
> >> > @@ -18,9 +17,6 @@
> >> > (pci_has_flag(PCI_REASSIGN_ALL_BUS))
> >> >
> >> > #define arch_can_pci_mmap_wc() 1
> >> > -#define ARCH_GENERIC_PCI_MMAP_RESOURCE 1
> >> > -
> >> > -extern int isa_dma_bridge_buggy;
> >> >
> >> > #ifdef CONFIG_PCI
> >> > static inline int pci_get_legacy_ide_irq(struct pci_dev *dev, int channel)
> >> > @@ -28,11 +24,9 @@ static inline int pci_get_legacy_ide_irq(struct pci_dev *dev, int channel)
> >> > /* no legacy IRQ on arm64 */
> >> > return -ENODEV;
> >> > }
> >> > -
> >> > -static inline int pci_proc_domain(struct pci_bus *bus)
> >> > -{
> >> > - return 1;
> >> > -}
> >> > #endif /* CONFIG_PCI */
> >> >
> >> > +/* Generic PCI */
> >> > +#include <asm-generic/pci.h>
> >> > +
> >> > #endif /* __ASM_PCI_H */
> >> > diff --git a/arch/csky/include/asm/pci.h b/arch/csky/include/asm/pci.h
> >> > index ebc765b1f78b..44866c1ad461 100644
> >> > --- a/arch/csky/include/asm/pci.h
> >> > +++ b/arch/csky/include/asm/pci.h
> >> > @@ -9,26 +9,10 @@
> >> >
> >> > #include <asm/io.h>
> >> >
> >> > -#define PCIBIOS_MIN_IO 0
> >> > -#define PCIBIOS_MIN_MEM 0
> >> > +/* Generic PCI */
> >> > +#include <asm-generic/pci.h>
> >> >
> >> > -/* C-SKY shim does not initialize PCI bus */
> >> > -#define pcibios_assign_all_busses() 1
> >> > -
> >> > -extern int isa_dma_bridge_buggy;
> >> > -
> >> > -#ifdef CONFIG_PCI
> >> > -static inline int pci_get_legacy_ide_irq(struct pci_dev *dev, int channel)
> >> > -{
> >> > - /* no legacy IRQ on csky */
> >> > - return -ENODEV;
> >> > -}
> >> > -
> >> > -static inline int pci_proc_domain(struct pci_bus *bus)
> >> > -{
> >> > - /* always show the domain in /proc */
> >> > - return 1;
> >> > -}
> >> > -#endif /* CONFIG_PCI */
> >> > +/* csky doesn't use generic pci resource mapping */
> >> > +#undef ARCH_GENERIC_PCI_MMAP_RESOURCE
> >> >
> >> > #endif /* __ASM_CSKY_PCI_H */
> >> > diff --git a/arch/riscv/include/asm/pci.h b/arch/riscv/include/asm/pci.h
> >> > index 7fd52a30e605..12ce8150cfb0 100644
> >> > --- a/arch/riscv/include/asm/pci.h
> >> > +++ b/arch/riscv/include/asm/pci.h
> >> > @@ -12,29 +12,7 @@
> >> >
> >> > #include <asm/io.h>
> >> >
> >> > -#define PCIBIOS_MIN_IO 0
> >> > -#define PCIBIOS_MIN_MEM 0
> >>
> >> My for-next changes these in bb356ddb78b2 ("RISC-V: PCI: Avoid handing
> >> out address 0 to devices"). Do you mind either splitting out the
> >> arch/riscv bits or having this in via some sort of shared tag?
> >
> > Shouldn't the values not matter here if the IO and mem resources are
> > described in the DT (and don't use 0)? The values of 4 and 16 look
> > odd.
>
> The linked thread has a fairly long discussion
> <https://lore.kernel.org/all/[email protected]/>.
> I agree it's odd to have this in arch code: "don't hand out address 0"
> isn't really a RISC-V constraint (ie, we don't have architecture-defined
> limitations on these address spaces) but a constraint that comes from
> the generic port I/O functions and some other related PCI/resource code
> where the value 0 is a sentinel.
If you look at arm32, we have a variable for PCIBIOS_MIN_MEM because
pre-DT what platforms required was all over the place. Nothing using
DT needs to set that variable. And arm64 uses 0 without problems. In
all those platforms, none of them have the same restrictions? So it is
still curious to me how PCIBIOS_MIN_MEM matters for Risc-V.
I/O is different as Arnd said, but I'd imagine we could just set the
min to 4 in the generic header and be done with it.
> Maybe the right thing to do here is actually to make the default
> definitions of these macros non-zero, or to add some sort of ARCH_
> flavor of them and move that non-zero requirement closer to where it
> comes from? From the look of it any port that uses the generic port I/O
> functions and has 0 for these will be broken in the same way.
>
> That said, I'm not really a PCI guy so maybe Bjorn or Maciej has a
> better idea?
From fu740:
ranges = <0x81000000 0x0 0x60080000 0x0
0x60080000 0x0 0x10000>, /* I/O */
<0x82000000 0x0 0x60090000 0x0
0x60090000 0x0 0xff70000>, /* mem */
<0x82000000 0x0 0x70000000 0x0
0x70000000 0x0 0x1000000>, /* mem */
<0xc3000000 0x20 0x00000000 0x20
0x00000000 0x20 0x00000000>; /* mem prefetchable */
So again, how does one get a 0 address handed out when that's not even
a valid region according to DT? Is there some legacy stuff that
ignores the bridge windows?
Rob
On Fri, 22 Jul 2022, Rob Herring wrote:
> > Maybe the right thing to do here is actually to make the default
> > definitions of these macros non-zero, or to add some sort of ARCH_
> > flavor of them and move that non-zero requirement closer to where it
> > comes from? From the look of it any port that uses the generic port I/O
> > functions and has 0 for these will be broken in the same way.
> >
> > That said, I'm not really a PCI guy so maybe Bjorn or Maciej has a
> > better idea?
>
> >From fu740:
> ranges = <0x81000000 0x0 0x60080000 0x0
> 0x60080000 0x0 0x10000>, /* I/O */
> <0x82000000 0x0 0x60090000 0x0
> 0x60090000 0x0 0xff70000>, /* mem */
> <0x82000000 0x0 0x70000000 0x0
> 0x70000000 0x0 0x1000000>, /* mem */
> <0xc3000000 0x20 0x00000000 0x20
> 0x00000000 0x20 0x00000000>; /* mem prefetchable */
>
> So again, how does one get a 0 address handed out when that's not even
> a valid region according to DT? Is there some legacy stuff that
> ignores the bridge windows?
It doesn't matter as <asm/pci.h> just sets it as a generic parameter for
the platform, reflecting the limitation of PCI core, which in the course
of the discussion referred was found rather infeasible to remove. The
FU740 does not decode to PCI at 0, but another RISC-V device could. And I
think that DT should faithfully describe hardware and not our software
limitations.
Mind that PCI has originated from the x86 world where decoding low 24-bit
memory space to ISA has been implied (implicitly decoded on PCI systems by
the southbridge) for areas not decoded to DRAM by the memory controller.
So the inability of our PCI core to handle MMIO at 0 did not matter at the
time it was introduced as the value of 0 would never be used for a memory
BAR.
Maciej
On Fri, Jul 22, 2022 at 1:23 PM Maciej W. Rozycki <[email protected]> wrote:
>
> On Fri, 22 Jul 2022, Rob Herring wrote:
>
> > > Maybe the right thing to do here is actually to make the default
> > > definitions of these macros non-zero, or to add some sort of ARCH_
> > > flavor of them and move that non-zero requirement closer to where it
> > > comes from? From the look of it any port that uses the generic port I/O
> > > functions and has 0 for these will be broken in the same way.
> > >
> > > That said, I'm not really a PCI guy so maybe Bjorn or Maciej has a
> > > better idea?
> >
> > >From fu740:
> > ranges = <0x81000000 0x0 0x60080000 0x0
> > 0x60080000 0x0 0x10000>, /* I/O */
> > <0x82000000 0x0 0x60090000 0x0
> > 0x60090000 0x0 0xff70000>, /* mem */
> > <0x82000000 0x0 0x70000000 0x0
> > 0x70000000 0x0 0x1000000>, /* mem */
> > <0xc3000000 0x20 0x00000000 0x20
> > 0x00000000 0x20 0x00000000>; /* mem prefetchable */
> >
> > So again, how does one get a 0 address handed out when that's not even
> > a valid region according to DT? Is there some legacy stuff that
> > ignores the bridge windows?
>
> It doesn't matter as <asm/pci.h> just sets it as a generic parameter for
> the platform, reflecting the limitation of PCI core, which in the course
> of the discussion referred was found rather infeasible to remove. The
> FU740 does not decode to PCI at 0, but another RISC-V device could. And I
> think that DT should faithfully describe hardware and not our software
> limitations.
Let me ask this another way. When would a 0 memory or i/o address ever
work? It doesn't seem this s/w limitation has anything specific to
Risc-V. Given pci_iomap_range() rejects 0, I can't see how it could
ever work. Maybe only for legacy ISA? So should the generic defaults
just be what Risc-V is using instead of 0?
Rob
On Fri, Jul 22, 2022 at 6:36 PM Rob Herring <[email protected]> wrote:
> On Fri, Jul 22, 2022 at 9:27 AM Palmer Dabbelt <[email protected]> wrote:
>
> From fu740:
> ranges = <0x81000000 0x0 0x60080000 0x0
> 0x60080000 0x0 0x10000>, /* I/O */
...
> So again, how does one get a 0 address handed out when that's not even
> a valid region according to DT? Is there some legacy stuff that
> ignores the bridge windows?
The PCI-side port number 0x60080000 gets turned into Linux I/O resource 0,
which I think is what __pci_assign_resource operates on.
The other question is why the platform would want to configure the
PCI bus to have a PCI I/O space window of size 0x10000 at the address
it's mapped into, rather than putting it at address zero. Is this a hardware
bug, a bootloader bug, or just badly set up in the DT?
Putting the PCI address of the I/O space window at port 0 is usually
better because it works with PCI devices and drivers that assume that
port numbers are below 0xfffff, and makes the PCI port number match
the Linux port number.
Arnd
On Fri, Jul 22, 2022 at 1:55 PM Arnd Bergmann <[email protected]> wrote:
>
> On Fri, Jul 22, 2022 at 6:36 PM Rob Herring <[email protected]> wrote:
> > On Fri, Jul 22, 2022 at 9:27 AM Palmer Dabbelt <[email protected]> wrote:
> >
> > From fu740:
> > ranges = <0x81000000 0x0 0x60080000 0x0
> > 0x60080000 0x0 0x10000>, /* I/O */
> ...
> > So again, how does one get a 0 address handed out when that's not even
> > a valid region according to DT? Is there some legacy stuff that
> > ignores the bridge windows?
>
> The PCI-side port number 0x60080000 gets turned into Linux I/O resource 0,
> which I think is what __pci_assign_resource operates on.
>
> The other question is why the platform would want to configure the
> PCI bus to have a PCI I/O space window of size 0x10000 at the address
> it's mapped into, rather than putting it at address zero. Is this a hardware
> bug, a bootloader bug, or just badly set up in the DT?
...putting it at *PCI* address zero, right? Yeah, that looks
suspicious. The core code seems to not use the PCI address, but
various drivers do. Maybe they are miscalculating things and still end
up with 0. If so, we're stuck with that ABI though we could fix it up
in the ranges parsing code and make driver behavior consistent.
In any case, that seems to be a somewhat common occurrence. A somewhat
accurate search (ignore the MBUS_ID ones):
$ git grep -A4 '\sranges =' -- arch/ | grep '0x81000000' | grep -v -E
'0x81000000\s[0x]+\s[0x]+\s'
arch/arm/boot/dts/armada-370.dtsi-
0x81000000 0x1 0 MBUS_ID(0x04, 0xe0) 0 1 0 /* Port 0.0 IO
*/
arch/arm/boot/dts/armada-375.dtsi-
0x81000000 0x1 0 MBUS_ID(0x04, 0xe0) 0 1 0 /* Port 0 IO */
arch/arm/boot/dts/armada-xp-98dx3236.dtsi-
0x81000000 0x1 0 MBUS_ID(0x04, 0xe0) 0 1 0 /* Port 0.0 IO */>;
arch/arm/boot/dts/bcm47622.dtsi: ranges = <0 0x81000000
0x818000>;
arch/arm/boot/dts/dove.dtsi- 0x81000000
0x1 0x0 MBUS_ID(0x04, 0xe0) 0 1 0 /* Port 0.0 I/O */
arch/arm/boot/dts/kirkwood-6192.dtsi-
0x81000000 0x1 0 MBUS_ID(0x04, 0xe0) 0 1 0 /* Port 0.0 IO
*/>;
arch/arm/boot/dts/kirkwood-6281.dtsi-
0x81000000 0x1 0 MBUS_ID(0x04, 0xe0) 0 1 0 /* Port 0.0 IO
*/>;
arch/arm/boot/dts/kirkwood-98dx4122.dtsi-
0x81000000 0x1 0 MBUS_ID(0x04, 0xe0) 0 1 0 /* Port 0.0 IO
*/>;
arch/arm/boot/dts/mt7623.dtsi: ranges = <0x81000000 0
0x1a160000 0 0x1a160000 0 0x00010000
arch/arm/boot/dts/qcom-ipq4019.dtsi: ranges =
<0x81000000 0 0x40200000 0x40200000 0 0x00100000>,
arch/arm/boot/dts/qcom-ipq8064.dtsi: ranges =
<0x81000000 0 0x0fe00000 0x0fe00000 0 0x00100000 /* downstream I/O
*/
arch/arm/boot/dts/qcom-ipq8064.dtsi: ranges =
<0x81000000 0 0x31e00000 0x31e00000 0 0x00100000 /* downstream I/O
*/
arch/arm/boot/dts/qcom-ipq8064.dtsi: ranges =
<0x81000000 0 0x35e00000 0x35e00000 0 0x00100000 /* downstream I/O
*/
arch/arm64/boot/dts/broadcom/bcm4908/bcm4908.dtsi: ranges
= <0x00 0x00 0x81000000 0x4000>;
arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts: ranges =
<0x81000000 0 0xe8000000 0 0xe8000000 0 0x01000000 /* Port 0 IO
*/
arch/arm64/boot/dts/mediatek/mt8192.dtsi-
<0x81000000 0 0x12800000 0x0 0x12800000 0 0x0800000>;
arch/arm64/boot/dts/qcom/ipq6018.dtsi: ranges =
<0x81000000 0 0x20200000 0 0x20200000
arch/arm64/boot/dts/qcom/ipq8074.dtsi: ranges =
<0x81000000 0 0x10200000 0x10200000
arch/arm64/boot/dts/qcom/ipq8074.dtsi: ranges =
<0x81000000 0 0x20200000 0x20200000
arch/arm64/boot/dts/rockchip/rk3399.dtsi-
<0x81000000 0x0 0xfbe00000 0x0 0xfbe00000 0x0 0x100000>;
arch/arm64/boot/dts/toshiba/tmpv7708.dtsi: ranges
= <0x81000000 0 0x40000000 0 0x40000000 0 0x00010000
arch/riscv/boot/dts/sifive/fu740-c000.dtsi: ranges
= <0x81000000 0x0 0x60080000 0x0 0x60080000 0x0 0x10000>, /*
I/O */
> Putting the PCI address of the I/O space window at port 0 is usually
> better because it works with PCI devices and drivers that assume that
> port numbers are below 0xfffff, and makes the PCI port number match
> the Linux port number.
I could make the PCI schema check for this, but I guess technically
any 32-bit PCI I/O address is valid even if 64K is the practical
limit.
Rob
P.S. I really wish I/O space would disappear completely.
On 22 Jul 2022, at 20:55, Arnd Bergmann <[email protected]> wrote:
>
> On Fri, Jul 22, 2022 at 6:36 PM Rob Herring <[email protected]> wrote:
>> On Fri, Jul 22, 2022 at 9:27 AM Palmer Dabbelt <[email protected]> wrote:
>>
>> From fu740:
>> ranges = <0x81000000 0x0 0x60080000 0x0
>> 0x60080000 0x0 0x10000>, /* I/O */
> ...
>> So again, how does one get a 0 address handed out when that's not even
>> a valid region according to DT? Is there some legacy stuff that
>> ignores the bridge windows?
>
> The PCI-side port number 0x60080000 gets turned into Linux I/O resource 0,
> which I think is what __pci_assign_resource operates on.
>
> The other question is why the platform would want to configure the
> PCI bus to have a PCI I/O space window of size 0x10000 at the address
> it's mapped into, rather than putting it at address zero. Is this a hardware
> bug, a bootloader bug, or just badly set up in the DT?
>
> Putting the PCI address of the I/O space window at port 0 is usually
> better because it works with PCI devices and drivers that assume that
> port numbers are below 0xfffff, and makes the PCI port number match
> the Linux port number.
Possibly related is the (harmless) warning spew seen during boot on
FreeBSD. The bridge, and all downstream ones, reset to having I/O
window 0x0-0xfff and memory/prefetch window 0x0-0xfffff (which FreeBSD
tries to allocate, fails because that’s outside the valid ranges above,
so it then just paves over with a sane fresh config). Linux resets the
controller on probe so this is the initial state when enumerating, and
perhaps that’s not handled so gracefully as on FreeBSD (which also
resets in device_attach, the second half of what Linux calls probe)?
Jess
On Fri, 22 Jul 2022, Rob Herring wrote:
> > > So again, how does one get a 0 address handed out when that's not even
> > > a valid region according to DT? Is there some legacy stuff that
> > > ignores the bridge windows?
> >
> > It doesn't matter as <asm/pci.h> just sets it as a generic parameter for
> > the platform, reflecting the limitation of PCI core, which in the course
> > of the discussion referred was found rather infeasible to remove. The
> > FU740 does not decode to PCI at 0, but another RISC-V device could. And I
> > think that DT should faithfully describe hardware and not our software
> > limitations.
>
> Let me ask this another way. When would a 0 memory or i/o address ever
> work? It doesn't seem this s/w limitation has anything specific to
> Risc-V. Given pci_iomap_range() rejects 0, I can't see how it could
> ever work. Maybe only for legacy ISA? So should the generic defaults
> just be what Risc-V is using instead of 0?
Absolutely, cf.:
<https://lore.kernel.org/lkml/[email protected]/>.
Maciej
On Fri, 22 Jul 2022, Rob Herring wrote:
> P.S. I really wish I/O space would disappear completely.
Some systems make it happen already, such as the POWER9 platform and its
PHB4 host bridge (which doesn't handle PCIe TLP I/O read or write commands
at all), however some PCI/e devices do require I/O space, such as IEEE
1284 parallel port adapters. Some PCI devices dating back as far as to
1990s provide dual mapping of their resources via an I/O and a memory BAR
both at a time, so we can choose which to use depending on circumstances.
FWIW,
Maciej