| Submitter | Scott |
|---|---|
| Date | 2010-10-18 19:55:54 |
| Message ID | <9FD1800B7F5D47C4895E30C234D5B344@m3a78> |
| Download | mbox | patch |
| Permalink | /patch/2140/ |
| State | Superseded |
| Headers | show |
Comments
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
Patch
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