| Message ID | 1416413091-13452-4-git-send-email-tomasz.nowicki@linaro.org |
|---|---|
| State | New |
| Headers | show |
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. That's good point, I will readb()/writeb() instead. > > 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. Sure, will do. Regards, Tomasz > >> 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@linaro.org> >> Tested-by: Hanjun Guo <hanjun.guo@linaro.org> >> --- >> 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-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@linaro.org> >> Tested-by: Hanjun Guo <hanjun.guo@linaro.org> >> --- >> 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-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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
We are going to use mmio_config_{} name convention across all architectures. 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@linaro.org> Tested-by: Hanjun Guo <hanjun.guo@linaro.org> --- arch/x86/include/asm/pci.h | 42 +++++++++++++++++++++++++++++++++++++++++ arch/x86/include/asm/pci_x86.h | 43 ------------------------------------------ 2 files changed, 42 insertions(+), 43 deletions(-)