Re: [PATCH 3/6] x86, acpi, pci: Move PCI config space accessors.

Tomasz Nowicki <tomasz.nowicki@xxxxxxxxxx> · Tue, 17 Feb 2015 14:03:01 +0100

On 11.12.2014 00:17, Bjorn Helgaas wrote:
On Wed, Nov 19, 2014 at 05:04:48PM +0100, Tomasz Nowicki wrote:
We are going to use mmio_config_{} name convention across all architectures.

mmio_config_*() are workarounds for an AMD Fam10h defect [1].  I would prefer
not to extend these names to other architectures, because they should be
able to use readb()/writeb()/etc. directly, as we did on x86 before
3320ad994afb ("x86: Work around mmio config space quirk on AMD Fam10h").

In fact, it would be nice if we could use readb()/writeb()/etc. on x86, and
only use mmio_config_*() when we're on AMD Fam10h.  Maybe there could be a
"struct pci_raw_ops pci_mmcfg_amd_fam10h" that used mmio_config_*(), and we
could set raw_pci_ext_ops to those ops instead of the default ones when
we're on AMD Fam10h.  Then x86 should be able to use the generic
readb()-based ops on most platforms.

While I do agree we should use readb()/writeb()... methods, I am not 
sure there is a nice way to use mmio_config_*() exclusively for AMD 
Fam10h. For x86, right now, we have three PCI config accessors sets: 
mmconfig_32.c, mmconfig_64.c, numachip.c and each are different. So one 
pci_mmcfg_amd_fam10h pci_raw_ops is not enough. I am thinking of having 
additional structure (integrated with "struct pci_mmcfg_region") that 
keeps MMIO accessors where readb()/writeb() would be the default one. 
For AMD Fam10h case we could tweak it as necessary. What do you thing Bjorn?

Tomasz


Bjorn

[1] 3320ad994afb ("x86: Work around mmio config space quirk on AMD Fam10h")

Somewhere in this process, I'd like to update the AMD Fam10h comment to fix
the typos and reference the spec that talks about this, i.e., the "BIOS and
Kernel Developer's Guide (BKDG) For AMD Family 10h Processors," rev 3.48,
sec 2.11.1, "MMIO Configuration Coding Requirements."

That can be a separate patch since it's only cosmetic.

Currently it belongs to asm/pci_x86.h header which should be included
only for x86 specific files. From now on, those accessors are in asm/pci.h
header which can be included in non-architecture code much easier.

Signed-off-by: Tomasz Nowicki <tomasz.nowicki@xxxxxxxxxx>
Tested-by: Hanjun Guo <hanjun.guo@xxxxxxxxxx>
---
  arch/x86/include/asm/pci.h     | 42 +++++++++++++++++++++++++++++++++++++++++
  arch/x86/include/asm/pci_x86.h | 43 ------------------------------------------
  2 files changed, 42 insertions(+), 43 deletions(-)

diff --git a/arch/x86/include/asm/pci.h b/arch/x86/include/asm/pci.h
index 0892ea0..5ba3720 100644
--- a/arch/x86/include/asm/pci.h
+++ b/arch/x86/include/asm/pci.h
@@ -71,6 +71,48 @@ void pcibios_set_master(struct pci_dev *dev);
  struct irq_routing_table *pcibios_get_irq_routing_table(void);
  int pcibios_set_irq_routing(struct pci_dev *dev, int pin, int irq);

+/*
+ * AMD Fam10h CPUs are buggy, and cannot access MMIO config space
+ * on their northbrige except through the * %eax register. As such, you MUST
+ * NOT use normal IOMEM accesses, you need to only use the magic mmio-config
+ * accessor functions.
+ * In fact just use pci_config_*, nothing else please.
+ */
+static inline unsigned char mmio_config_readb(void __iomem *pos)
+{
+	u8 val;
+	asm volatile("movb (%1),%%al" : "=a" (val) : "r" (pos));
+	return val;
+}
+
+static inline unsigned short mmio_config_readw(void __iomem *pos)
+{
+	u16 val;
+	asm volatile("movw (%1),%%ax" : "=a" (val) : "r" (pos));
+	return val;
+}
+
+static inline unsigned int mmio_config_readl(void __iomem *pos)
+{
+	u32 val;
+	asm volatile("movl (%1),%%eax" : "=a" (val) : "r" (pos));
+	return val;
+}
+
+static inline void mmio_config_writeb(void __iomem *pos, u8 val)
+{
+	asm volatile("movb %%al,(%1)" : : "a" (val), "r" (pos) : "memory");
+}
+
+static inline void mmio_config_writew(void __iomem *pos, u16 val)
+{
+	asm volatile("movw %%ax,(%1)" : : "a" (val), "r" (pos) : "memory");
+}
+
+static inline void mmio_config_writel(void __iomem *pos, u32 val)
+{
+	asm volatile("movl %%eax,(%1)" : : "a" (val), "r" (pos) : "memory");
+}

  #define HAVE_PCI_MMAP
  extern int pci_mmap_page_range(struct pci_dev *dev, struct vm_area_struct *vma,
diff --git a/arch/x86/include/asm/pci_x86.h b/arch/x86/include/asm/pci_x86.h
index caba141..42e7332 100644
--- a/arch/x86/include/asm/pci_x86.h
+++ b/arch/x86/include/asm/pci_x86.h
@@ -121,49 +121,6 @@ extern int __init pcibios_init(void);
  extern int pci_legacy_init(void);
  extern void pcibios_fixup_irqs(void);

-/*
- * AMD Fam10h CPUs are buggy, and cannot access MMIO config space
- * on their northbrige except through the * %eax register. As such, you MUST
- * NOT use normal IOMEM accesses, you need to only use the magic mmio-config
- * accessor functions.
- * In fact just use pci_config_*, nothing else please.
- */
-static inline unsigned char mmio_config_readb(void __iomem *pos)
-{
-	u8 val;
-	asm volatile("movb (%1),%%al" : "=a" (val) : "r" (pos));
-	return val;
-}
-
-static inline unsigned short mmio_config_readw(void __iomem *pos)
-{
-	u16 val;
-	asm volatile("movw (%1),%%ax" : "=a" (val) : "r" (pos));
-	return val;
-}
-
-static inline unsigned int mmio_config_readl(void __iomem *pos)
-{
-	u32 val;
-	asm volatile("movl (%1),%%eax" : "=a" (val) : "r" (pos));
-	return val;
-}
-
-static inline void mmio_config_writeb(void __iomem *pos, u8 val)
-{
-	asm volatile("movb %%al,(%1)" : : "a" (val), "r" (pos) : "memory");
-}
-
-static inline void mmio_config_writew(void __iomem *pos, u16 val)
-{
-	asm volatile("movw %%ax,(%1)" : : "a" (val), "r" (pos) : "memory");
-}
-
-static inline void mmio_config_writel(void __iomem *pos, u32 val)
-{
-	asm volatile("movl %%eax,(%1)" : : "a" (val), "r" (pos) : "memory");
-}
-
  #ifdef CONFIG_PCI
  # ifdef CONFIG_ACPI
  #  define x86_default_pci_init		pci_acpi_init
--
1.9.1

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







