Patchwork AMD MMCONF Support

login
register
about
Submitter Arne Georg Gleditsch
Date 2010-05-26 09:44:52
Message ID <874ohvm1yj.fsf_-_@taniquetil.gledits.ch>
Download mbox | patch
Permalink /patch/1388/
State Superseded
Headers show

Comments

Arne Georg Gleditsch - 2010-05-26 09:44:52
Ed Swierk <eswierk@aristanetworks.com> writes:
> It appears that the write to register 0x78 is attempting to switch the
> LPC device from subtractive decode to positive decode. Presumably the
> hang is due to the LPC device not being configured for some IO or
> memory range. Why you're seeing the hang only when mmconfig is
> enabled, I have no idea.
>
> I'd vote for removing that line, but I have no means to check whether
> that would break any board.

Thanks for checking, Ed.  The following patch implements/fixes PCI
MMCONF support for AMD Fam10 systems, and includes commenting out of the
offending line in the MCP55 setup code.  It also converts a couple of
pci_*_config accesses in amdfam10/early_ht.c to pci_io_*_config, as they
run before MMCONF is ready.  (There's a comment regarding this in the
top of that file, but that was apparently not sufficient to prevent
these calls from sneaking in.)

Signed-off-by: Arne Georg Gleditsch <arne.gleditsch@numascale.com>
Stefan Reinauer - 2010-05-26 11:12:45
On 5/26/10 11:44 AM, Arne Georg Gleditsch wrote:
> @@ -199,7 +199,7 @@ static inline __attribute__((always_inline)) void pci_io_write_config16(device_t
>  static inline __attribute__((always_inline)) void pci_mmio_write_config16(device_t dev, unsigned where, uint16_t value)
>  {
>          unsigned addr;
> -        addr = CONFIG_MMCONF_BASE_ADDRESS | dev | where;
> +        addr = dev | (where & ~1);
>          write16x(addr, value);
>  }
>  #endif
>   
This breaks non-AMD platforms I think.

Stefan
Stefan Reinauer - 2010-05-26 11:16:51
On 5/26/10 11:44 AM, Arne Georg Gleditsch wrote:
> +#define MSR_GS_BASE 0xc0000101
> +
>  #include <arch/mmio_conf.h>
>  
> +
> +static inline __attribute__((always_inline)) void set_gs_base(uint64_t base)
> +{
> +	/* Make sure %gs is a valid descriptor */
> +	__asm__ volatile (
> +		"mov %%ds, %%ax\n\tmov %%ax, %%gs" ::: "eax");
> +	/* Set base */
> +	__asm__ volatile (
> +		"wrmsr" :: "A"(base), "c"(MSR_GS_BASE));
> +}
> +
>   
This also breaks on all non-AMD platforms.

I think we should move all of this code to some AMD specific directory
(northbridge or cpu) before applying these changes.

Comments?
Arne Georg Gleditsch - 2010-05-26 11:34:15
Stefan Reinauer <stefan.reinauer@coresystems.de> writes:
> This also breaks on all non-AMD platforms.

Augh, yes, it would.  Sorry, I though I checked that there were no other
users of this, but I obviously didn't.

> I think we should move all of this code to some AMD specific directory
> (northbridge or cpu) before applying these changes.
>
> Comments?

I guess it's either that or moving the AMD MMCONF range down below the
32-bit barrier.  If we want to do the latter anyway, perhaps we should
just do it now.
Arne Georg Gleditsch - 2010-05-26 11:40:50
Arne Georg Gleditsch <arne.gleditsch@numascale.com> writes:
> Augh, yes, it would.  Sorry, I though I checked that there were no other
> users of this, but I obviously didn't.

(I guess I made this assumption on the basis of the fact that I had to
fix several write8x to be of correct length in the pci_ops_mmconf.c
file...  Is anyone actually using this today?)
Stefan Reinauer - 2010-05-26 13:01:51
On 5/26/10 1:40 PM, Arne Georg Gleditsch wrote:
> Is anyone actually using this today?
>   

ICH7 is using it to set up the virtual channels for Azalia (and
potentially other stuff)

I think there are some uses in some C7 chipset too, but I am not sure.
Stefan Reinauer - 2010-05-26 13:04:12
On 5/26/10 1:34 PM, Arne Georg Gleditsch wrote:
> I guess it's either that or moving the AMD MMCONF range down below the
> 32-bit barrier.  If we want to do the latter anyway, perhaps we should
> just do it now.
>   
That alone won't do it,.. There's the AMD specific MSR and %gs is not
set to MMCONFIG_BASE on other chipsets/cpus.
Not sure what to do about the MSR,... the %gs thing should probably
fixed by setting up %gs in a central place somewhere.

Stefan
Arne Georg Gleditsch - 2010-05-26 13:16:05
Stefan Reinauer <stefan.reinauer@coresystems.de> writes:
> That alone won't do it,.. There's the AMD specific MSR and %gs is not
> set to MMCONFIG_BASE on other chipsets/cpus.
> Not sure what to do about the MSR,... the %gs thing should probably
> fixed by setting up %gs in a central place somewhere.

If we move the MMCONF area into the 32-bit reachable range, we don't
need to mess around with %gs.  The other AMD specfic MSRs are localized
to AMD specfic files, I believe.
Carl-Daniel Hailfinger - 2010-05-26 15:27:47
On 26.05.2010 15:16, Arne Georg Gleditsch wrote:
> Stefan Reinauer <stefan.reinauer@coresystems.de> writes:
>   
>> That alone won't do it,.. There's the AMD specific MSR and %gs is not
>> set to MMCONFIG_BASE on other chipsets/cpus.
>> Not sure what to do about the MSR,... the %gs thing should probably
>> fixed by setting up %gs in a central place somewhere.
>>     
>
> If we move the MMCONF area into the 32-bit reachable range, we don't
> need to mess around with %gs.  The other AMD specfic MSRs are localized
> to AMD specfic files, I believe.
>   

IMHO we want that anyway to support 32bit operating systems.

Regards,
Carl-Daniel
Patrick Georgi - 2010-08-17 08:23:03
Am 26.05.2010 13:12, schrieb Stefan Reinauer:
> On 5/26/10 11:44 AM, Arne Georg Gleditsch wrote:
>> @@ -199,7 +199,7 @@ static inline __attribute__((always_inline)) void pci_io_write_config16(device_t
>>  static inline __attribute__((always_inline)) void pci_mmio_write_config16(device_t dev, unsigned where, uint16_t value)
>>  {
>>          unsigned addr;
>> -        addr = CONFIG_MMCONF_BASE_ADDRESS | dev | where;
>> +        addr = dev | (where & ~1);
>>          write16x(addr, value);
>>  }
>>  #endif
>>   
> This breaks non-AMD platforms I think.
Any solution to that?

This patch is required for a pending board addition, see
http://patchwork.coreboot.org/patch/1387/


Patrick
Arne Georg Gleditsch - 2010-08-17 08:50:49
Patrick Georgi <patrick@georgi-clan.de> writes:
> Am 26.05.2010 13:12, schrieb Stefan Reinauer:
>> This breaks non-AMD platforms I think.
> Any solution to that?
>
> This patch is required for a pending board addition, see
> http://patchwork.coreboot.org/patch/1387/

I intend to rework the MMCONF patch to map the MMCONF area into 32-bit
address space, as well as submit an updated patch for the HP DL165.  I'm
swamped in other stuff right now though; if anyone could give me some
pointers on what the best way to reserve 256M of (256M-aligned) address
space is, that would save me some time.  Either dynamically or
statically (I believe e0000000 - efffffff is often used for this).  We'd
like to hoist any memory behind this, so some method of allocation that
makes this part of the IO hole would be good.

Patch

diff --git a/src/arch/i386/include/arch/mmio_conf.h b/src/arch/i386/include/arch/mmio_conf.h
index df91cb5..79f8373 100644
--- a/src/arch/i386/include/arch/mmio_conf.h
+++ b/src/arch/i386/include/arch/mmio_conf.h
@@ -8,7 +8,7 @@  static inline __attribute__((always_inline)) uint8_t read8x(uint32_t addr)
 {
 	uint8_t value;
         __asm__ volatile (
-                "movb %%gs:(%1), %0\n\t"
+                "movb %%gs:(%1), %%al\n\t"
                 :"=a"(value): "b" (addr)
         );
         return value;
@@ -18,7 +18,7 @@  static inline __attribute__((always_inline)) uint16_t read16x(uint32_t addr)
 {
         uint16_t value;
         __asm__ volatile (
-                "movw %%gs:(%1), %0\n\t"
+                "movw %%gs:(%1), %%ax\n\t"
                 :"=a"(value): "b" (addr)
         );
 
@@ -30,7 +30,7 @@  static inline __attribute__((always_inline)) uint32_t read32x(uint32_t addr)
 {
         uint32_t value;
         __asm__ volatile (
-                "movl %%gs:(%1), %0\n\t"
+                "movl %%gs:(%1), %%eax\n\t"
                 :"=a"(value): "b" (addr)
         );
 
@@ -41,7 +41,7 @@  static inline __attribute__((always_inline)) uint32_t read32x(uint32_t addr)
 static inline __attribute__((always_inline)) void write8x(uint32_t addr, uint8_t value)
 {
         __asm__ volatile (
-                "movb %1, %%gs:(%0)\n\t"
+                "movb %%al, %%gs:(%0)\n\t"
                 :: "b" (addr), "a" (value)
         );
 
@@ -50,7 +50,7 @@  static inline __attribute__((always_inline)) void write8x(uint32_t addr, uint8_t
 static inline __attribute__((always_inline)) void write16x(uint32_t addr, uint16_t value)
 {
         __asm__ volatile (
-                "movw %1, %%gs:(%0)\n\t"
+                "movw %%ax, %%gs:(%0)\n\t"
                 :: "b" (addr), "a" (value)
         );
 
@@ -59,7 +59,7 @@  static inline __attribute__((always_inline)) void write16x(uint32_t addr, uint16
 static inline __attribute__((always_inline)) void write32x(uint32_t addr, uint32_t value)
 {
         __asm__ volatile (
-                "movl %1, %%gs:(%0)\n\t"
+                "movl %%eax, %%gs:(%0)\n\t"
                 :: "b" (addr), "a" (value)
         );
 }
diff --git a/src/arch/i386/include/arch/romcc_io.h b/src/arch/i386/include/arch/romcc_io.h
index 6bc7dfc..dbb522b 100644
--- a/src/arch/i386/include/arch/romcc_io.h
+++ b/src/arch/i386/include/arch/romcc_io.h
@@ -78,7 +78,7 @@  static inline __attribute__((always_inline)) uint8_t pci_io_read_config8(device_
 static inline __attribute__((always_inline)) uint8_t pci_mmio_read_config8(device_t dev, unsigned where)
 {
         unsigned addr;
-        addr = CONFIG_MMCONF_BASE_ADDRESS | dev | where;
+        addr = dev | where;
         return read8x(addr);
 }
 #endif
@@ -107,7 +107,7 @@  static inline __attribute__((always_inline)) uint16_t pci_io_read_config16(devic
 static inline __attribute__((always_inline)) uint16_t pci_mmio_read_config16(device_t dev, unsigned where)
 {
         unsigned addr;
-        addr = CONFIG_MMCONF_BASE_ADDRESS | dev | where;
+        addr = dev | (where & ~1);
         return read16x(addr);
 }
 #endif
@@ -138,7 +138,7 @@  static inline __attribute__((always_inline)) uint32_t pci_io_read_config32(devic
 static inline __attribute__((always_inline)) uint32_t pci_mmio_read_config32(device_t dev, unsigned where)
 {
         unsigned addr;
-        addr = CONFIG_MMCONF_BASE_ADDRESS | dev | where;
+        addr = dev | (where & ~3);
         return read32x(addr);
 }
 #endif
@@ -168,7 +168,7 @@  static inline __attribute__((always_inline)) void pci_io_write_config8(device_t
 static inline __attribute__((always_inline)) void pci_mmio_write_config8(device_t dev, unsigned where, uint8_t value)
 {
         unsigned addr;
-        addr = CONFIG_MMCONF_BASE_ADDRESS | dev | where;
+        addr = dev | where;
         write8x(addr, value);
 }
 #endif
@@ -199,7 +199,7 @@  static inline __attribute__((always_inline)) void pci_io_write_config16(device_t
 static inline __attribute__((always_inline)) void pci_mmio_write_config16(device_t dev, unsigned where, uint16_t value)
 {
         unsigned addr;
-        addr = CONFIG_MMCONF_BASE_ADDRESS | dev | where;
+        addr = dev | (where & ~1);
         write16x(addr, value);
 }
 #endif
@@ -230,7 +230,7 @@  static inline __attribute__((always_inline)) void pci_io_write_config32(device_t
 static inline __attribute__((always_inline)) void pci_mmio_write_config32(device_t dev, unsigned where, uint32_t value)
 {
         unsigned addr;
-        addr = CONFIG_MMCONF_BASE_ADDRESS | dev | where;
+        addr = dev | (where & ~3);
         write32x(addr, value);
 }
 #endif
diff --git a/src/arch/i386/lib/pci_ops_mmconf.c b/src/arch/i386/lib/pci_ops_mmconf.c
index a605708..f459fb2 100644
--- a/src/arch/i386/lib/pci_ops_mmconf.c
+++ b/src/arch/i386/lib/pci_ops_mmconf.c
@@ -12,42 +12,64 @@ 
  * Functions for accessing PCI configuration space with mmconf accesses
  */
 
+#define _ULLx(x) x ## ULL
+#define _ULL(x) _ULLx(x)
+#define PCI_MMIO_BASE _ULL(CONFIG_MMCONF_BASE_ADDRESS)
+
 #define PCI_MMIO_ADDR(SEGBUS, DEVFN, WHERE) ( \
-	CONFIG_MMCONF_BASE_ADDRESS | \
         (((SEGBUS) & 0xFFF) << 20) | \
         (((DEVFN) & 0xFF) << 12) | \
         ((WHERE) & 0xFFF))
 
+#define MSR_GS_BASE 0xc0000101
+
 #include <arch/mmio_conf.h>
 
+
+static inline __attribute__((always_inline)) void set_gs_base(uint64_t base)
+{
+	/* Make sure %gs is a valid descriptor */
+	__asm__ volatile (
+		"mov %%ds, %%ax\n\tmov %%ax, %%gs" ::: "eax");
+	/* Set base */
+	__asm__ volatile (
+		"wrmsr" :: "A"(base), "c"(MSR_GS_BASE));
+}
+
 static uint8_t pci_mmconf_read_config8(struct bus *pbus, int bus, int devfn, int where)
 {
+		set_gs_base(PCI_MMIO_BASE);
 		return (read8x(PCI_MMIO_ADDR(bus, devfn, where)));
 }
 
 static uint16_t pci_mmconf_read_config16(struct bus *pbus, int bus, int devfn, int where)
 {
-                return (read16x(PCI_MMIO_ADDR(bus, devfn, where)));
+		set_gs_base(PCI_MMIO_BASE);
+                return (read16x(PCI_MMIO_ADDR(bus, devfn, where) & ~1));
 }
 
 static uint32_t pci_mmconf_read_config32(struct bus *pbus, int bus, int devfn, int where)
 {
-                return (read32x(PCI_MMIO_ADDR(bus, devfn, where)));
+		set_gs_base(PCI_MMIO_BASE);
+                return (read32x(PCI_MMIO_ADDR(bus, devfn, where) & ~3));
 }
 
 static void  pci_mmconf_write_config8(struct bus *pbus, int bus, int devfn, int where, uint8_t value)
 {
+		set_gs_base(PCI_MMIO_BASE);
                 write8x(PCI_MMIO_ADDR(bus, devfn, where), value);
 }
 
 static void pci_mmconf_write_config16(struct bus *pbus, int bus, int devfn, int where, uint16_t value)
 {
-                write8x(PCI_MMIO_ADDR(bus, devfn, where), value);
+		set_gs_base(PCI_MMIO_BASE);
+                write16x(PCI_MMIO_ADDR(bus, devfn, where) & ~1, value);
 }
 
 static void pci_mmconf_write_config32(struct bus *pbus, int bus, int devfn, int where, uint32_t value)
 {
-                write8x(PCI_MMIO_ADDR(bus, devfn, where), value);
+		set_gs_base(PCI_MMIO_BASE);
+                write32x(PCI_MMIO_ADDR(bus, devfn, where) & ~3, value);
 }
 
 
diff --git a/src/cpu/amd/model_10xxx/init_cpus.c b/src/cpu/amd/model_10xxx/init_cpus.c
index 48a32f8..a0840fa 100644
--- a/src/cpu/amd/model_10xxx/init_cpus.c
+++ b/src/cpu/amd/model_10xxx/init_cpus.c
@@ -57,8 +57,21 @@  static void set_EnableCf8ExtCfg(void)
 static void set_EnableCf8ExtCfg(void) { }
 #endif
 
-/*[39:8] */
-#define PCI_MMIO_BASE 0xfe000000
+static void set_wrap32dis(void) {
+    msr_t msr;
+
+    msr = rdmsr(0xc0010015);
+    msr.lo |= (1<<17);
+
+    wrmsr(0xc0010015, msr);
+
+}
+
+#define _ULLx(x) x ## ULL
+#define _ULL(x) _ULLx(x)
+
+/*[63:0] */
+#define PCI_MMIO_BASE _ULL(CONFIG_MMCONF_BASE_ADDRESS)
 /* because we will use gs to store hi, so need to make sure lo can start
    from 0, So PCI_MMIO_BASE & 0x00ffffff should be equal to 0*/
 
@@ -71,18 +84,17 @@  static void set_pci_mmio_conf_reg(void)
 	// 256 bus per segment, MMIO reg will be 4G , enable MMIO Config space
 	msr.lo |= ((8 + CONFIG_PCI_BUS_SEGN_BITS) << 2) | (1 << 0);
 	msr.hi &= ~(0x0000ffff);
-	msr.hi |= (PCI_MMIO_BASE >> (32 - 8));
-	wrmsr(0xc0010058, msr);	// MMIO Config Base Address Reg
+	msr.hi |= (PCI_MMIO_BASE >> (32));
+	wrmsr(0xc0010058, msr); // MMIO Config Base Address Reg
 
 	//mtrr for that range?
 	// set_var_mtrr_x(7, PCI_MMIO_BASE<<8, PCI_MMIO_BASE>>(32-8), 0x00000000, 0x01, MTRR_TYPE_UNCACHEABLE);
 
 	set_wrap32dis();
 
-	msr.hi = (PCI_MMIO_BASE >> (32 - 8));
+	msr.hi = (PCI_MMIO_BASE >> (32));
 	msr.lo = 0;
-	wrmsr(0xc0000101, msr);	//GS_Base Reg
-
+	wrmsr(0xc0000101, msr); //GS_Base Reg
 #endif
 }
 
diff --git a/src/northbridge/amd/amdfam10/Kconfig b/src/northbridge/amd/amdfam10/Kconfig
index dd893f6..2704273 100644
--- a/src/northbridge/amd/amdfam10/Kconfig
+++ b/src/northbridge/amd/amdfam10/Kconfig
@@ -53,6 +53,16 @@  config HW_MEM_HOLE_SIZE_AUTO_INC
 	default n
 	depends on NORTHBRIDGE_AMD_AMDFAM10
 
+config MMCONF_SUPPORT
+	bool
+	default y
+	depends on NORTHBRIDGE_AMD_AMDFAM10
+
+config MMCONF_BASE_ADDRESS
+	hex
+	default 0xffffff0000000000
+	depends on NORTHBRIDGE_AMD_AMDFAM10
+
 config BOOTBLOCK_NORTHBRIDGE_INIT
         string
         default "northbridge/amd/amdfam10/bootblock.c"
diff --git a/src/northbridge/amd/amdfam10/early_ht.c b/src/northbridge/amd/amdfam10/early_ht.c
index b1c21f2..58f2cdd 100644
--- a/src/northbridge/amd/amdfam10/early_ht.c
+++ b/src/northbridge/amd/amdfam10/early_ht.c
@@ -129,7 +129,7 @@  static void enumerate_ht_chain(void)
 						PCI_HT_CAP_SLAVE_CTRL0 : PCI_HT_CAP_SLAVE_CTRL1;
 
 					do {
-						ctrl = pci_read_config16(devx, pos + ctrl_off);
+						ctrl = pci_io_read_config16(devx, pos + ctrl_off);
 						/* Is this the end of the hypertransport chain? */
 						if (ctrl & (1 << 6)) {
 							goto out;
@@ -144,8 +144,8 @@  static void enumerate_ht_chain(void)
 							 * if its transient
 							 */
 							ctrl |= ((1 << 4) | (1 <<8)); // Link fail + Crc
-							pci_write_config16(devx, pos + ctrl_off, ctrl);
-							ctrl = pci_read_config16(devx, pos + ctrl_off);
+							pci_io_write_config16(devx, pos + ctrl_off, ctrl);
+							ctrl = pci_io_read_config16(devx, pos + ctrl_off);
 							if (ctrl & ((1 << 4) | (1 << 8))) {
 								// can not clear the error
 								break;
diff --git a/src/southbridge/nvidia/mcp55/mcp55_early_setup_car.c b/src/southbridge/nvidia/mcp55/mcp55_early_setup_car.c
index fb84f5a..e01630d 100644
--- a/src/southbridge/nvidia/mcp55/mcp55_early_setup_car.c
+++ b/src/southbridge/nvidia/mcp55/mcp55_early_setup_car.c
@@ -267,8 +267,13 @@  static void mcp55_early_setup(unsigned mcp55_num, unsigned *busn, unsigned *devn
 	RES_PCI_IO, PCI_ADDR(0, 6, 0, 0x74), 0xFFFFFFC0, 0x00000000,
 	RES_PCI_IO, PCI_ADDR(0, 6, 0, 0xC0), 0x00000000, 0xCB8410DE,
 	RES_PCI_IO, PCI_ADDR(0, 6, 0, 0xC4), 0xFFFFFFF8, 0x00000007,
-
-	RES_PCI_IO, PCI_ADDR(0, 1, 0, 0x78), 0xC0FFFFFF, 0x19000000,
+	/* The following operation hangs when performed via MMCFG:
+	     pci_read_config32(romcc): 00010000:0078: 20040000
+	     setup_resource_map_x_offset: 10000, 78: 20040000
+	     pci_write_config32(romcc): 00010000:0078: 19040000
+	     (hang)
+	   Response missing? */
+	/* RES_PCI_IO, PCI_ADDR(0, 1, 0, 0x78), 0xC0FFFFFF, 0x19000000, */
 
 #if MCP55_USE_AZA == 1
 	RES_PCI_IO, PCI_ADDR(0, 6, 1, 0x40), 0x00000000, 0xCB8410DE,