On Wed, Sep 8, 2010 at 9:00 AM, Arne Georg Gleditsch arne.gleditsch@numascale.com wrote:
Myles Watson mylesgw@gmail.com writes:
I'm confused why you wouldn't add the resource from the northbridge code. Is there a reason to have it be a mainboard resource?
Not really, but there's no existing infrastructure for having add_northbridge_resources be called except by way of add_mainboard_resources. As far as I can tell. I think this may warrant changing, but preferrably as a separate step.
+config MMCONF_SUPPORT
- bool
- default y
- depends on NORTHBRIDGE_AMD_AMDFAM10
You could use select for MMCONF_SUPPORT. I think it should be selected in the northbridge or in the mainboard, but not both.
MMCONF_SUPPORT is selected in the northbridge, MMCONF_SUPPORT_DEFAULT is selected in the mainboard config to actually activate the functionality. This was the way I read the existing code, perhaps this two-level approach is not be required? Either way, I think you want to select this on a mainboard-by-mainboard basis, at least initially. There is a potential for breakage, like the one we experienced with the nvidia southbridge.
OK.
Instead of adding the reserved area directly to the coreboot tables, you should add it before resource allocation and let the rest happen automatically.
I'm sorry, I'm not at all familiar with the resource allocation framework. I tried to model this on the corresponding code for relevant Intel mainboards. How would you change it, precisely?
Fair question. It's been a long time since I've thought about MMCONF, but here are a couple of ways that I think it could be done:
I. Use a hard-coded address for MMCONF only for pre-RAM 1. Add a resource to read_resources in the northbridge that gives the size but isn't fixed 2. The resource allocator will find a good place for it 3. In set resource update the address so that future accesses work
II. Use a hard-coded fixed address always 1. Add a fixed resource to read_resources in the northbridge 2. The resource allocator will avoid it
Either way, if the area needs to end up reserved in the coreboot tables, then the code should live there. If we need a new resource flag that says create an entry for this, then I think that would be the way to go. I nominate IORESOURCE_RESERVE.
I started a patch that applied over yours, but I think discussing the way to go in higher-level terms is the right first step.
Much of your patch is unrelated to this discussion. If we split the bug fixes into a separate patch and get that committed I'd be happy to help make this work.
Thanks, Myles