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

[Myles Watson]

On Mon, Oct 18, 2010 at 11:29 AM, Scott Duplichan  wrote:
> 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
> 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