Re: [coreboot] [PATCH] MTRR related improvements for AMD family 10h and family 0Fh systems

[Peter Stuge]

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

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