On Mon, Oct 18, 2010 at 1:55 PM, Scott Duplichan scott@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@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@notabs.org
Acked-by: Myles Watson mylesgw@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