[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 <scott at notabs.org>

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