[coreboot] [PATCH] AMD F10h: set MMCONF bus count according to configured value

Scott Duplichan scott at notabs.org
Mon Oct 18 21:55:54 CEST 2010


> For AMD family 10h processors, msr c0010058 is always programmed
> for 256 buses, even if fewer are configured. This patch lets msr
> c0010058 programming use the configured bus count, CONFIG_MMCONF_BUS_NUMBER.
>
> Tested with the mahogany_fam10 project. Does the assembler have
> a compile time operator for highest set bit or base 2 log? That
> would sure simplify this patch.
>
>
> Signed-off-by: Scott Duplichan <scott at notabs.org>
>
> Index: src/cpu/amd/car/cache_as_ram.inc
> ===================================================================
> --- src/cpu/amd/car/cache_as_ram.inc    (revision 5965)
> +++ src/cpu/amd/car/cache_as_ram.inc    (working copy)
> @@ -136,8 +136,26 @@
>        movl    $MSR_MCFG_BASE, %ecx
>        rdmsr
>        andl    $(~(0xfff00000 | (0xf << 2))), %eax
> -       orl     $((CONFIG_MMCONF_BASE_ADDRESS & 0xfff00000)), %eax
> -       orl     $((8 << 2) | (1 << 0)), %eax
I don't see how the logic below ever lets you get (8<<2)  The largest
I see is 8.

> +       orl     $((CONFIG_MMCONF_BASE_ADDRESS & 0xfff00000) | (1 << 0)), %eax
> +   #if (CONFIG_MMCONF_BUS_NUMBER == 2)
> +       orl     $1, %eax
Doesn't this just set the enable bit?

> +   #elif (CONFIG_MMCONF_BUS_NUMBER == 4)
> +       orl     $2, %eax
> +   #elif (CONFIG_MMCONF_BUS_NUMBER == 8)
> +       orl     $3, %eax
> +   #elif (CONFIG_MMCONF_BUS_NUMBER == 16)
> +       orl     $4, %eax
> +   #elif (CONFIG_MMCONF_BUS_NUMBER == 32)
> +       orl     $5, %eax
> +   #elif (CONFIG_MMCONF_BUS_NUMBER == 64)
> +       orl     $6, %eax
> +   #elif (CONFIG_MMCONF_BUS_NUMBER == 128)
> +       orl     $7, %eax
> +   #elif (CONFIG_MMCONF_BUS_NUMBER == 256)
> +       orl     $8, %eax
> +   #else
> +       #error "unsupported MMCONF_BUS_NUMBER value"
> +   #endif
>        andl    $(~(0x0000ffff)), %edx
>        orl     $(CONFIG_MMCONF_BASE_ADDRESS >> 32), %edx
>        wrmsr

Could you move the ugly logic into Kconfig or a header file?  Should
there be a check with the base address alignment & size to make sure
it's a valid combination?

> Index: src/cpu/amd/car/cache_as_ram.inc
> ===================================================================
> --- src/cpu/amd/car/cache_as_ram.inc    (revision 5965)
> +++ src/cpu/amd/car/cache_as_ram.inc    (working copy)
> @@ -136,8 +136,26 @@
>        movl    $MSR_MCFG_BASE, %ecx
>        rdmsr
>        andl    $(~(0xfff00000 | (0xf << 2))), %eax
> -       orl     $((CONFIG_MMCONF_BASE_ADDRESS & 0xfff00000)), %eax
> -       orl     $((8 << 2) | (1 << 0)), %eax
> +       orl     $((CONFIG_AMD_MMCONF_REG_VAL << 2) | (1 << 0)), %eax

Thanks,
Myles



Thank you Myles. Here is a corrected version. It also avoids the unnessary
read of the register before programming it. It is limited to base address
values below 4GB.

As far as moving the uglyness, isn't asm code about as ugly as it gets
anyway? A check for aligment, etc could be added. But it didn't exist
before, and the purpose of this patch is to fix the basic problem of
incorrect register programming.

To really improve it, we could consider moving this into a C function
then calling it from cache_as_ram.inc. I hate to see too much effort
get put into asm language coding.

Thanks,
Scott

Signed-off-by: Scott Duplichan <scott at notabs.org>

Index: src/cpu/amd/car/cache_as_ram.inc
===================================================================
--- src/cpu/amd/car/cache_as_ram.inc    (revision 5965)
+++ src/cpu/amd/car/cache_as_ram.inc    (working copy)
@@ -133,13 +133,29 @@

 #if CONFIG_MMCONF_SUPPORT
        /* Set MMIO config space BAR. */
+       movl    $0, %edx
+       movl    $(CONFIG_MMCONF_BASE_ADDRESS | (1 << 0)), %eax
+   #if (CONFIG_MMCONF_BUS_NUMBER == 2)
+       orl     $(1 << 2), %eax
+   #elif (CONFIG_MMCONF_BUS_NUMBER == 4)
+       orl     $(2 << 2), %eax
+   #elif (CONFIG_MMCONF_BUS_NUMBER == 8)
+       orl     $(3 << 2), %eax
+   #elif (CONFIG_MMCONF_BUS_NUMBER == 16)
+       orl     $(4 << 2), %eax
+   #elif (CONFIG_MMCONF_BUS_NUMBER == 32)
+       orl     $(5 << 2), %eax
+   #elif (CONFIG_MMCONF_BUS_NUMBER == 64)
+       orl     $(6 << 2), %eax
+   #elif (CONFIG_MMCONF_BUS_NUMBER == 128)
+       orl     $(7 << 2), %eax
+   #elif (CONFIG_MMCONF_BUS_NUMBER == 256)
+       orl     $(8 << 2), %eax
+   #else
+       #error "unsupported MMCONF_BUS_NUMBER value"
+   #endif
+
        movl    $MSR_MCFG_BASE, %ecx
-       rdmsr
-       andl    $(~(0xfff00000 | (0xf << 2))), %eax
-       orl     $((CONFIG_MMCONF_BASE_ADDRESS & 0xfff00000)), %eax
-       orl     $((8 << 2) | (1 << 0)), %eax
-       andl    $(~(0x0000ffff)), %edx
-       orl     $(CONFIG_MMCONF_BASE_ADDRESS >> 32), %edx
        wrmsr
 #endif






More information about the coreboot mailing list