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 + orl $((CONFIG_MMCONF_BASE_ADDRESS & 0xfff00000) | (1 << 0)), %eax + #if (CONFIG_MMCONF_BUS_NUMBER == 2) + orl $1, %eax + #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
On Mon, Oct 18, 2010 at 11:29 AM, 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)), %eaxorl $((8 << 2) | (1 << 0)), %eax
orl $((CONFIG_AMD_MMCONF_REG_VAL << 2) | (1 << 0)), %eax
Thanks, Myles
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)), %eaxorl $((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@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
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
On 18.10.2010, at 10:44, Myles Watson mylesgw@gmail.com wrote:
On Mon, Oct 18, 2010 at 11:29 AM, Scott Duplichan scott@notabs.org wrote:
- #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?
I don't think that we should move ugly logic into Kconfig. Kconfig is for configuration, code is for logic.
Stefan
On Mon, Oct 18, 2010 at 8:06 PM, Stefan Reinauer stefan.reinauer@coresystems.de wrote:
On 18.10.2010, at 10:44, Myles Watson mylesgw@gmail.com wrote:
On Mon, Oct 18, 2010 at 11:29 AM, Scott Duplichan scott@notabs.org wrote:
- #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?
I don't think that we should move ugly logic into Kconfig. Kconfig is for configuration, code is for logic.
Agreed.
Thanks, Myles
"Scott Duplichan" scott@notabs.org writes:
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.
How about just renaming CONFIG_MMCONF_BUS_NUMBER to ...BUS_BITS? There's only 8 discrete values it can take anyway, and this would also make it harder to pick an unsupported value for BUS_NUMBER.
-----Original Message----- From: coreboot-bounces@coreboot.org [mailto:coreboot-bounces@coreboot.org] On Behalf Of Arne Georg Gleditsch Sent: Tuesday, October 19, 2010 01:51 AM To: Scott Duplichan Cc: 'Coreboot' Subject: Re: [coreboot] [PATCH] AMD F10h: set MMCONF bus count according toconfigured value
]"Scott Duplichan" scott@notabs.org writes: ] ]> 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. ] ]How about just renaming CONFIG_MMCONF_BUS_NUMBER to ...BUS_BITS? ]There's only 8 discrete values it can take anyway, and this would also ]make it harder to pick an unsupported value for BUS_NUMBER. ] ]-- ] Arne.
Hello Arne,
Thanks for the suggestion. It would certainly eliminate the awkward logic. On the other hand, if anyone is overriding the kconfig value CONFIG_MMCONF_BUS_NUMBER locally, they would have to update their override.
Last night, I thought I would just learn about gas macros and do it that way. It was more difficult than I thought. I cannot even find examples of gas macros that use arguments. A C style macro seemed possible, but I found when gas processes it the needed ?: does not work. So I ended up with a gas macro that does not use arguments. The macro allows the awkwark logic and additional error checking to be placed in an existing include file. The new error checking was tested by forcing a few failing cases:
Signed-off-by: Scott Duplichan scott@notabs.org
Index: src/include/cpu/amd/mtrr.h =================================================================== --- src/include/cpu/amd/mtrr.h (revision 5975) +++ src/include/cpu/amd/mtrr.h (working copy) @@ -32,6 +32,44 @@ #define TOP_MEM_MASK 0x007fffff #define TOP_MEM_MASK_KB (TOP_MEM_MASK >> 10)
+ +#if defined(ASSEMBLY) + +.macro SET_C0010058 + #if (CONFIG_MMCONF_BASE_ADDRESS > 0xFFFFFFFF) + #error MMCONF_BASE_ADDRESS too big + #elif (CONFIG_MMCONF_BASE_ADDRESS & 0xFFFFF) + #error MMCONF_BASE_ADDRESS not 1MB aligned + #endif + movl $0, %edx + movl $((CONFIG_MMCONF_BASE_ADDRESS) | (1 << 0)), %eax + #if (CONFIG_MMCONF_BUS_NUMBER == 1) + #elif (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 "bad MMCONF_BUS_NUMBER value" + #endif + movl $(0xc0010058), %ecx + wrmsr +.endm + +#endif + + #if !defined(__PRE_RAM__) && !defined(ASSEMBLY) void amd_setup_mtrrs(void); #endif Index: src/cpu/amd/car/cache_as_ram.inc =================================================================== --- src/cpu/amd/car/cache_as_ram.inc (revision 5975) +++ src/cpu/amd/car/cache_as_ram.inc (working copy) @@ -132,15 +132,7 @@ wrmsr
#if CONFIG_MMCONF_SUPPORT - /* Set MMIO config space BAR. */ - 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 + SET_C0010058 #endif
CAR_FAM10_out_post_errata:
Last night, I thought I would just learn about gas macros and do it that way. It was more difficult than I thought. I cannot even find examples of gas macros that use arguments. A C style macro seemed possible, but I found when gas processes it the needed ?: does not work. So I ended up with a gas macro that does not use arguments. The macro allows the awkwark logic and additional error checking to be placed in an existing include file. The new error checking was tested by forcing a few failing cases:
Signed-off-by: Scott Duplichan scott@notabs.org
I'm sorry that I wasn't more clear. I think it was better before. The code looks exactly the same, but it's in a different file, which wasn't my intent at all.
I like the other version much better. I hope that your new knowledge of gas macros will come in handy in the future :)
Thanks, Myles
Hi Scott,
On 19.10.2010 19:47, Scott Duplichan wrote:
Last night, I thought I would just learn about gas macros and do it that way. It was more difficult than I thought. I cannot even find examples of gas macros that use arguments.
Take a look at src/cpu/amd/car/cache_as_ram.inc, specifically extractmask and simplemask.
Regards, Carl-Daniel
-----Original Message----- From: coreboot-bounces@coreboot.org [mailto:coreboot-bounces@coreboot.org] On Behalf Of Carl-Daniel Hailfinger Sent: Tuesday, October 19, 2010 01:25 PM To: Scott Duplichan Cc: 'Arne Georg Gleditsch'; 'Coreboot' Subject: Re: [coreboot] [PATCH] AMD F10h: set MMCONF bus count according toconfigured value
]Hi Scott, ] ]On 19.10.2010 19:47, Scott Duplichan wrote: ]> Last night, I thought I would just learn about gas macros and do it ]> that way. It was more difficult than I thought. I cannot even find ]> examples of gas macros that use arguments. ] ]Take a look at src/cpu/amd/car/cache_as_ram.inc, specifically ]extractmask and simplemask. ] ]Regards, ]Carl-Daniel ] ]-- ]http://www.hailfinger.org/
Thanks Carl-Daniel.
Hello Myles,
I am not so happy with the new one either. What I really wanted was a macro called highestSetBit. That way, the reader can guess what macro invocation highestSetBit (CONFIG_MMCONF_BUS_NUMBER) does without actually digging it out. But I could not find a way to make that work. Another idea was to find highestSetBit at runtime, if it could be done a short and readable code sequence. This would be possible with family 15h, where instructions such as lzcnt are supported. But family 10h does not have this instruction.
Now that Carl-Daniel has showed how to do gas macros, how about I add a highestSetBit macro to amd/mtrr.h? Then everything else can be inlined?
Thanks, Scott
On Tue, Oct 19, 2010 at 12:34 PM, Scott Duplichan scott@notabs.org wrote:
-----Original Message----- From: coreboot-bounces@coreboot.org [mailto:coreboot-bounces@coreboot.org] On Behalf Of Carl-Daniel Hailfinger Sent: Tuesday, October 19, 2010 01:25 PM To: Scott Duplichan Cc: 'Arne Georg Gleditsch'; 'Coreboot' Subject: Re: [coreboot] [PATCH] AMD F10h: set MMCONF bus count according toconfigured value
]Hi Scott, ] ]On 19.10.2010 19:47, Scott Duplichan wrote: ]> Last night, I thought I would just learn about gas macros and do it ]> that way. It was more difficult than I thought. I cannot even find ]> examples of gas macros that use arguments. ] ]Take a look at src/cpu/amd/car/cache_as_ram.inc, specifically ]extractmask and simplemask. ] ]Regards, ]Carl-Daniel ] ]-- ]http://www.hailfinger.org/
Thanks Carl-Daniel.
Hello Myles,
I am not so happy with the new one either. What I really wanted was a macro called highestSetBit. That way, the reader can guess what macro invocation highestSetBit (CONFIG_MMCONF_BUS_NUMBER) does without actually digging it out. But I could not find a way to make that work. Another idea was to find highestSetBit at runtime, if it could be done a short and readable code sequence. This would be possible with family 15h, where instructions such as lzcnt are supported. But family 10h does not have this instruction.
Now that Carl-Daniel has showed how to do gas macros, how about I add a highestSetBit macro to amd/mtrr.h? Then everything else can be inlined?
I would say it isn't worth the effort, but it's up to you. The problem with macros in assembly is that it's unclear which registers they clobber. That was why I suggested a pre-processor macro, but it's not that important.
Thanks, Myles
]I would say it isn't worth the effort, but it's up to you. The ]problem with macros in assembly is that it's unclear which registers ]they clobber. That was why I suggested a pre-processor macro, but ]it's not that important. ] ]Thanks, ]Myles
Hello Myles,
Good point. Best would be a 'macro' that allows writing: movl highestSetBit (busn), %eax But that is not possible apparently. How about then, back to inlined code with the extra error checks:
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 5975) +++ src/cpu/amd/car/cache_as_ram.inc (working copy) @@ -132,15 +132,35 @@ wrmsr
#if CONFIG_MMCONF_SUPPORT - /* Set MMIO config space BAR. */ - 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 + #if (CONFIG_MMCONF_BASE_ADDRESS > 0xFFFFFFFF) + #error "MMCONF_BASE_ADDRESS too big" + #elif (CONFIG_MMCONF_BASE_ADDRESS & 0xFFFFF) + #error "MMCONF_BASE_ADDRESS not 1MB aligned" + #endif + movl $0, %edx + movl $((CONFIG_MMCONF_BASE_ADDRESS) | (1 << 0)), %eax + #if (CONFIG_MMCONF_BUS_NUMBER == 1) + #elif (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 "bad MMCONF_BUS_NUMBER value" + #endif + movl $(0xc0010058), %ecx + wrmsr #endif
CAR_FAM10_out_post_errata:
Good point. Best would be a 'macro' that allows writing: movl highestSetBit (busn), %eax But that is not possible apparently. How about then, back to inlined code with the extra error checks:
Signed-off-by: Scott Duplichan <scott@notabs.org
Acked-by: Myles Watson mylesgw@gmail.com
Thanks, Myles
Scott Duplichan wrote:
back to inlined code with the extra error checks:
Signed-off-by: Scott Duplichan <scott@notabs.org
Acked-by: Peter Stuge peter@stuge.se
-
Arne Georg Gleditsch -
Carl-Daniel Hailfinger -
Myles Watson -
Peter Stuge -
Scott Duplichan -
Stefan Reinauer