Re: [coreboot] [PATCH] Avoid hang when 4GB or more DRAM is installed on AMD RS780 UMA systems

[Scott Duplichan]

]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.

You know, it is possible there is no AMD specific code in the new
function amd_setup_var_mtrrs. One change is a fix for a problem that
occurs only if 4GB or more DRAM is installed. The status of the problem
on non-AMD systems is not known. The other change avoids the unanswered
question about whether non-AMD systems really do exclude UMA memory from
the reported memory resource. But based on your i945 comment below, i945
does exclude UMA memory before DRAM is reported. Maybe I will look one
more time at making the AMD code do this. My last attempt didn't work.

]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

That is about right. The code comments I added may be a little misleading.
Here is what item 2 is really trying to do:

2. If there is DRAM above 4GB, then limit the WB variable MTRRs to DRAM
   below TOM. For UMA systems, stop at TOM-uma_memory_size.

]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)

Maybe a similar method would be better for AMD systems. On the other hand,
I believe the i945 does not support DRAM above 4GB, which would explain
why it is immune to the problem of 'hang when 4GB or more DRAM is installed'.
If it can be confirmed that the only coreboot projects supporting DRAM above
4GB are AMD based, then some of the testing concerns can be dropped.

]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.

Not sure I understand the question exactly. RAM between 4GB and TOM2 is
easy on AMD systems. Just set Tom2ForceMemTypeWB (done elsewhere) and
you are done, the DRAM between 4GB and TOM2 becomes WB and memory above
TOM2 continues to default to UC.

Thanks,
Scott

]Patrick