Am 10.11.2010 18:43, schrieb Scott Duplichan:
Another possibility is to go back to the idea of one x86_setup_var_mtrrs function. A new flag could be passed to x86_setup_var_mtrrs that tells tells the function that it ius running on an AMD system.
In that case, you could as well keep the separate AMD function around. My main "issue" was that there's vendor specific code in a (supposedly) generic file, esp. when there's already an amd_mtrr.c file. But I guess it doesn't matter that much, and when you're using static variables of mtrr.c, then it might actually be the right place.
It is a little frustrating to have to worry about the possibility of a code change breaking non-AMD UMA operation when I have no way to test it. For example, I suspect the UMA logic for > 4GB systems is not working for all systems. While I can confirm this for AMD systems, I cannot confirm it for non-AMD systems. So I assume I must use one method or another to preserve the existing logic for non-AMD use, just in case it really does work for UMA systems with more than 4GB DRAM.
What's the highlevel algorithm there?
From what I understand, the following happens - please correct me here:
You look up the resources like the "generic" mtrr code, and then there are two differences to the generic code path: 1. (4gb-tom2 .. 4gb) is left uncached (because of Tom2ForceMemTypeWB) 2. UMA is put at (4gb-umasize .. 4gb) and left uncached
Maybe we could do that with a weak function: First, the generic code in x86_setup_var_mtrrs determines the set of memory regions, then calls a weak function (if implemented), and only then makes the call to enable_var_mtrr.
The AMD code would then provide that function and handle the special cases (eg. UMA and that TOM2 space). It wouldn't require those other functions as it's given the var_state struct to work on.
On i945, UMA is done by providing a fixed resource. I don't think any other changes were necessary (see src/northbridge/intel/i945/northbridge.c)
As for the TOM2 stuff: Do you _have_ to mark it uncached? Depending on the situation, it might be less work to just mark it WB in the MTRRs (if that provides a full power-of-two region). But that's a potential optimization to look into separately.
Patrick