Scott Duplichan wrote: > The email subject line is changed to "MTRR related improvements for > AMD family 10h and family 0Fh systems" because the patch corrects > some additional problems.
Is looking really good. > MTRR related improvements for AMD family 10h and family 0Fh systems .. > -- Add above4gb flag argument to function x86_setup_var_mtrrs. Clearing > this flag causes x86_setup_var_mtrrs() to omit MTRR ranges for > DRAM above 4GB. AMD systems use this option to conserve MTRRs. This is a bit odd, but I say let's fix that in the future. > -- Improve white space consistency for mtrr.c by using tabs in place > of spaces. Personally I would strongly prefer to have all whitespace changes in a separate patch. > Signed-off-by: Scott Duplichan <[email protected]> Almost ack, but there are some things.. > +++ src/cpu/amd/mtrr/amd_mtrr.c (working copy) > @@ -146,13 +150,21 @@ > /* Setup TOP_MEM */ > msr.hi = state.mmio_basek >> 22; > msr.lo = state.mmio_basek << 10; > + > + // If UMA graphics is enabled, the frame buffer memory has been deducted > + // from the size of memory below 4GB. When setting TOM, include UMA DRAM. > + #if CONFIG_GFXUMA == 1 > + msr.lo += uma_memory_size; > + #endif > wrmsr(TOP_MEM, msr); I'd prefer plain C style comments /* */ that also used tabs, and were less than 80 chars. > +++ src/cpu/x86/mtrr/mtrr.c (working copy) > @@ -263,7 +264,10 @@ > (type==MTRR_TYPE_UNCACHEABLE)?"UC": > ((type==MTRR_TYPE_WRBACK)?"WB":"Other") > ); > - set_var_mtrr(reg++, range_startk, sizek, type, address_bits); > + > + // if range is above 4GB, MTRR is needed only if above4gb flag is set > + if (range_startk < 0x100000000ull / 1024 || above4gb) > + set_var_mtrr(reg++, range_startk, sizek, type, > address_bits); > range_startk += sizek; > range_sizek -= sizek; Please make indentation for comments match the associated code. This is an issue with many parts of the patch. > +++ src/northbridge/amd/amdmct/wrappers/mcti.h (working copy) > @@ -42,11 +42,12 @@ > //#define SYSTEM_TYPE MOBILE > #endif > > -/*---------------------------------------------------------------------------- > -COMMENT OUT ALL BUT 1 > -----------------------------------------------------------------------------*/ > +/*--------------------------------------------------------------------------*/ > +#if (CONFIG_GFXUMA) > +#define UMA_SUPPORT 1 /*Supported */ > +#else > #define UMA_SUPPORT 0 /*Not supported */ > -//#define UMA_SUPPORT 1 /*Supported */ > +#endif It seems that the above could simply be removed, and any code that currently checks UMA_SUPPORT could instead check CONFIG_GFXUMA directly. If you strongly prefer to make that a separate change then at least change the above to simply #define UMA_SUPPORT CONFIG_GFXUMA //Peter -- coreboot mailing list: [email protected] http://www.coreboot.org/mailman/listinfo/coreboot
