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

Myles Watson mylesgw at gmail.com
Mon Oct 18 22:10:15 CEST 2010


On Mon, Oct 18, 2010 at 1:55 PM, Scott Duplichan <scott at notabs.org> 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 <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?
Yes.  And it takes me a comparatively long time to find errors in it.
That was why I suggested moving the complexity somewhere else.

> 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.
Yep.

> To really improve it, we could consider moving this into a C function
> then calling it from cache_as_ram.inc.
I don't think we have to go that far.  All the values are known at
compile time, so I was picturing some check like:
#if (CONFIG_MMCONF_BASE_ADDRESS && (( CONFIG_MMCONF_BUS_NUMBER * 0x100000)-1)
#error "CONFIG_MMCONF_BASE_ADDRESS not aligned correctly"
#endif

Note that I didn't look to see if it's really 1 MB per bus.  You could
insert the correct size there.

> Signed-off-by: Scott Duplichan <scott at notabs.org>
Acked-by: Myles Watson <mylesgw at gmail.com>

>
> 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)
These could all be <= checks.  There's no real reason not to let
someone say they want to have 14 busses supported, is there?

#elif (CONFIG_MMCONF_BUS_NUMBER <= 4)

Thanks,
Myles




More information about the coreboot mailing list