Hi,
The following patch implements Opteron Fam 10 rev D (aka Istanbul) support for coreboot. I have not updated MAX_CPUS for all fam10 mainboards, but it might make sense to multiply those by 1.5.
Signed-off-by: Arne Georg Gleditsch arne.gleditsch@numascale.com
index ac62981..067560d 100644 --- a/src/cpu/amd/quadcore/quadcore.c +++ b/src/cpu/amd/quadcore/quadcore.c @@ -69,11 +72,10 @@ static void real_start_other_core(u32 nodeid, u32 cores)
if(cores > 1) { dword = pci_read_config32(NODE_PCI(nodeid, 0), 0x168); - dword |= (1 << 0); // core2 - if(cores > 2) { // core3 - dword |= (1 << 1); + for (i = 0; i < cores - 1; i++) { + dword |= 1 << i; + pci_write_config32(NODE_PCI(nodeid, 0), 0x168, dword); } - pci_write_config32(NODE_PCI(nodeid, 0), 0x168, dword); } }
I assume the line pci_write_config32(NODE_PCI(nodeid, 0), 0x168, dword); should be put outside the loop.
Everything seems to be fine. I don't have Istanbul to test. I have read every changes and they all look good.
Signed-off-by: Zheng Bao zheng.bao@amd.com
-----Original Message----- From: coreboot-bounces@coreboot.org
[mailto:coreboot-bounces@coreboot.org]
On Behalf Of Arne Georg Gleditsch Sent: Friday, March 05, 2010 10:41 PM To: coreboot@coreboot.org Subject: [coreboot] [PATCH] Istanbul support
Hi,
The following patch implements Opteron Fam 10 rev D (aka Istanbul) support for coreboot. I have not updated MAX_CPUS for all fam10 mainboards, but it might make sense to multiply those by 1.5.
Signed-off-by: Arne Georg Gleditsch arne.gleditsch@numascale.com
-- Arne.
Acked-by: Zheng Bao zheng.bao@amd.com
-----Original Message----- From: coreboot-bounces@coreboot.org
[mailto:coreboot-bounces@coreboot.org]
On Behalf Of Arne Georg Gleditsch Sent: Friday, March 05, 2010 10:41 PM To: coreboot@coreboot.org Subject: [coreboot] [PATCH] Istanbul support
Hi,
The following patch implements Opteron Fam 10 rev D (aka Istanbul) support for coreboot. I have not updated MAX_CPUS for all fam10 mainboards, but it might make sense to multiply those by 1.5.
Signed-off-by: Arne Georg Gleditsch arne.gleditsch@numascale.com
-- Arne.
r5200.
-----Original Message----- From: coreboot-bounces@coreboot.org
[mailto:coreboot-bounces@coreboot.org]
On Behalf Of Bao, Zheng Sent: Wednesday, March 10, 2010 11:39 AM To: Arne Georg Gleditsch Cc: coreboot@coreboot.org Subject: Re: [coreboot] [PATCH] Istanbul support
Acked-by: Zheng Bao zheng.bao@amd.com
-----Original Message----- From: coreboot-bounces@coreboot.org
[mailto:coreboot-bounces@coreboot.org]
On Behalf Of Arne Georg Gleditsch Sent: Friday, March 05, 2010 10:41 PM To: coreboot@coreboot.org Subject: [coreboot] [PATCH] Istanbul support
Hi,
The following patch implements Opteron Fam 10 rev D (aka Istanbul) support for coreboot. I have not updated MAX_CPUS for all fam10 mainboards, but it might make sense to multiply those by 1.5.
Signed-off-by: Arne Georg Gleditsch arne.gleditsch@numascale.com
-- Arne.
-- coreboot mailing list: coreboot@coreboot.org http://www.coreboot.org/mailman/listinfo/coreboot
On 10. mars 2010 04:30, Bao, Zheng wrote:
index ac62981..067560d 100644 --- a/src/cpu/amd/quadcore/quadcore.c +++ b/src/cpu/amd/quadcore/quadcore.c @@ -69,11 +72,10 @@ static void real_start_other_core(u32 nodeid, u32 cores)
if(cores > 1) { dword = pci_read_config32(NODE_PCI(nodeid, 0), 0x168);
dword |= (1 << 0); // core2if(cores > 2) { // core3dword |= (1 << 1);
for (i = 0; i < cores - 1; i++) {dword |= 1 << i;pci_write_config32(NODE_PCI(nodeid, 0), 0x168,dword); }
pci_write_config32(NODE_PCI(nodeid, 0), 0x168, dword);}
I assume the line pci_write_config32(NODE_PCI(nodeid, 0), 0x168, dword); should be put outside the loop.
Yes, well, I originally had that as "dword |= (0xf >> (5-cores); pci_write_config32(..)" or something. However, after reducing the loglevel below "debug" and encountering hangs that seemed to be conflict issues with the core startup, I added the loop to be able to add delays between startup of the individual cores during debugging. As it stands, the write could just as well be outside the loop.
Regarding hangs, I suspect this might be a PCI config race condition between the cores. I tried to adjust my configuration to use MMCONFIG to address this. It didn't seem to be supported out of the box, and I didn't have the time to look into it further at the moment. I still intend to do so, but it would be nice to know if anyone is using MMCONFIG with fam10 configurations. Is it supposed to work?
Arne Georg Gleditsch arne.gleditsch@numascale.com writes:
Regarding hangs, I suspect this might be a PCI config race condition between the cores. I tried to adjust my configuration to use MMCONFIG to address this. It didn't seem to be supported out of the box, and I didn't have the time to look into it further at the moment. I still intend to do so, but it would be nice to know if anyone is using MMCONFIG with fam10 configurations. Is it supposed to work?
Having looked at it some more, it is hard to see how mmconf support can possibly be functional at the moment. I've made some progress on getting it to work on my s2912 test rig; I'm appending the patch I'm currently running. (This is against r5200.)
My only remaining real issue is that parts of the nvidia mcp55 init code will not run properly using mmconf. The offending line is
RES_PCI_IO, PCI_ADDR(0, 1, 0, 0x78), 0xC0FFFFFF, 0x19000000,
which causes the operations
pci_read_config32: 00010000:0078: 20040000 pci_write_config32: 00010000:0078: 19040000
the second of which never returns when executed using mmconf. I'm speculating that this might be related to missing HT responses or something due to bus reconfiguration. As far as I can tell the device being targeted here is 10de:0364 (ISA bridge: nVidia Corporation MCP55 LPC Bridge).
Anoyone familiar with the mcp55 who can shed some light on what this write is supposed to accomplish and perhaps also on why it succeeds using the IO config mechanism when mmconf fails?
Arne Georg Gleditsch arne.gleditsch@numascale.com writes:
Regarding hangs, I suspect this might be a PCI config race condition between the cores. I tried to adjust my configuration to use MMCONFIG to address this. It didn't seem to be supported out of the box, and I didn't have the time to look into it further at the moment. I still intend to do so, but it would be nice to know if anyone is using MMCONFIG with fam10 configurations. Is it supposed to work?
Having looked at it some more, it is hard to see how mmconf support can possibly be functional at the moment. I've made some progress on getting it to work on my s2912 test rig; I'm appending the patch I'm currently running. (This is against r5200.)
Nice... !
I've been wondering before... why this odd approach with read8x and gs:... instead of just doing normal read8/16/32 ?
Where's %gs set up for romcc_io usage?
Stefan
On Mon, Apr 12, 2010 at 3:28 AM, Arne Georg Gleditsch arne.gleditsch@numascale.com wrote:
My only remaining real issue is that parts of the nvidia mcp55 init code will not run properly using mmconf. The offending line is
RES_PCI_IO, PCI_ADDR(0, 1, 0, 0x78), 0xC0FFFFFF, 0x19000000,
which causes the operations
pci_read_config32: 00010000:0078: 20040000 pci_write_config32: 00010000:0078: 19040000
the second of which never returns when executed using mmconf. I'm speculating that this might be related to missing HT responses or something due to bus reconfiguration. As far as I can tell the device being targeted here is 10de:0364 (ISA bridge: nVidia Corporation MCP55 LPC Bridge).
Anoyone familiar with the mcp55 who can shed some light on what this write is supposed to accomplish and perhaps also on why it succeeds using the IO config mechanism when mmconf fails?
That write has always caused a hang on Arista boards, even using plain IO accesses. I have no idea what it's supposed to do, but we've left it commented out with no apparent ill effects.
(Which reminds me that I need to port our patches to a modern version of Coreboot one of these days...)
--Ed
(Which reminds me that I need to port our patches to a modern version of Coreboot one of these days...)
--Ed
Yes, Ed alot has changed, it would be great to see.
Stefan Reinauer stepan@coresystems.de writes:
Nice... !
I've been wondering before... why this odd approach with read8x and gs:... instead of just doing normal read8/16/32 ?
Primarily to enable mapping in 64-bit space, I suppose. I guess we could map AMD fam10 mmio config space into 32 bit-space as well, the main reason not to is that it consumes quite a lot of space.
Where's %gs set up for romcc_io usage?
src/cpu/amd/model_10xxx/init_cpus.c, set_pci_mmio_conf_reg. I added explicit per-invocation assignment for post-car since the base is lost whenever %gs is changed. (Which happened once on transition to post-car and which I assume can happen arbitrarily when we're executing option roms).
Ed Swierk eswierk@aristanetworks.com writes:
Anoyone familiar with the mcp55 who can shed some light on what this write is supposed to accomplish and perhaps also on why it succeeds using the IO config mechanism when mmconf fails?
That write has always caused a hang on Arista boards, even using plain IO accesses. I have no idea what it's supposed to do, but we've left it commented out with no apparent ill effects.
That's a valuable data point. I'm also running with it disabled, no apparent problems here either.
I'd like to push this mmconf patch upstream in some shape or form, so if anyone would like to chime in how to approach this I'd be glad. I could just wrap the offending line with "#if !CONFIG_MMCONF_SUPPORT_DEFAULT" or something, if that's acceptable. If we can't figure out what it affects, however, perhaps we're just as well off removing it?
On Tue, Apr 13, 2010 at 12:35 AM, Arne Georg Gleditsch arne.gleditsch@numascale.com wrote:
I'd like to push this mmconf patch upstream in some shape or form, so if anyone would like to chime in how to approach this I'd be glad. I could just wrap the offending line with "#if !CONFIG_MMCONF_SUPPORT_DEFAULT" or something, if that's acceptable. If we can't figure out what it affects, however, perhaps we're just as well off removing it?
The logic in mcp55_early_setup_car.c seems at least partially based on ck804_early_setup_car.c. Maybe the write to 0x78 is just a leftover copy-and-paste scrap?
--Ed
On 13.04.2010 09:28, Arne Georg Gleditsch wrote:
Stefan Reinauer stepan@coresystems.de writes:
Nice... !
I've been wondering before... why this odd approach with read8x and gs:... instead of just doing normal read8/16/32 ?
Primarily to enable mapping in 64-bit space, I suppose. I guess we could map AMD fam10 mmio config space into 32 bit-space as well, the main reason not to is that it consumes quite a lot of space.
Mh. This also means that any OS using MMCONFIG (either directly or through ACPI) has to be 64 bit. Among others, this will break some Linux Live CDs (which are 32bit, although there are 64bit variants out there for some of them). Oh, and Windows might be affected, too.
Regards, Carl-Daniel
Hi Ed, hi Yinghai,
On 13.04.2010 10:32, Ed Swierk wrote:
The logic in mcp55_early_setup_car.c seems at least partially based on ck804_early_setup_car.c. Maybe the write to 0x78 is just a leftover copy-and-paste scrap?
Didn't you have access to the MCP55 docs? Not sure if you still have them, but if you do, this might be something that can be chcked easily against the docs.
Regards, Carl-Daniel
On Tue, Apr 13, 2010 at 12:35 AM, Arne Georg Gleditsch arne.gleditsch@numascale.com wrote:
That's a valuable data point. I'm also running with it disabled, no apparent problems here either.
I'd like to push this mmconf patch upstream in some shape or form, so if anyone would like to chime in how to approach this I'd be glad. I could just wrap the offending line with "#if !CONFIG_MMCONF_SUPPORT_DEFAULT" or something, if that's acceptable. If we can't figure out what it affects, however, perhaps we're just as well off removing it?
It appears that the write to register 0x78 is attempting to switch the LPC device from subtractive decode to positive decode. Presumably the hang is due to the LPC device not being configured for some IO or memory range. Why you're seeing the hang only when mmconfig is enabled, I have no idea.
I'd vote for removing that line, but I have no means to check whether that would break any board.
--Ed
Ed Swierk eswierk@aristanetworks.com writes:
It appears that the write to register 0x78 is attempting to switch the LPC device from subtractive decode to positive decode. Presumably the hang is due to the LPC device not being configured for some IO or memory range. Why you're seeing the hang only when mmconfig is enabled, I have no idea.
I'd vote for removing that line, but I have no means to check whether that would break any board.
Thanks for checking, Ed. The following patch implements/fixes PCI MMCONF support for AMD Fam10 systems, and includes commenting out of the offending line in the MCP55 setup code. It also converts a couple of pci_*_config accesses in amdfam10/early_ht.c to pci_io_*_config, as they run before MMCONF is ready. (There's a comment regarding this in the top of that file, but that was apparently not sufficient to prevent these calls from sneaking in.)
Signed-off-by: Arne Georg Gleditsch arne.gleditsch@numascale.com
Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net writes:
On 13.04.2010 09:28, Arne Georg Gleditsch wrote:
Stefan Reinauer stepan@coresystems.de writes:
Nice... !
I've been wondering before... why this odd approach with read8x and gs:... instead of just doing normal read8/16/32 ?
Primarily to enable mapping in 64-bit space, I suppose. I guess we could map AMD fam10 mmio config space into 32 bit-space as well, the main reason not to is that it consumes quite a lot of space.
Mh. This also means that any OS using MMCONFIG (either directly or through ACPI) has to be 64 bit. Among others, this will break some Linux Live CDs (which are 32bit, although there are 64bit variants out there for some of them). Oh, and Windows might be affected, too.
This is true. We could map the MMCONFIG area into 32-bit address space, if we wanted. That would remote quite a bit of complexity. Addressing all 256 PCI buses requires 256M, and I see there are vendors who are using 0xe0000000 - 0xf0000000 for this. We can configure the window to be smaller according to the actual number of buses present, but I'm not sure it's worth it.
I'm not exacly sure where to hook into the resource allocation framework to register such a range, but the actual changes ought to be fairly small.
As far as I know, we're not communicating any information about the the MMCONFIG area we've set up to the OS at present, so it's not really a pressing issue. (I suppose we do want to do so, however.)
On 5/26/10 11:44 AM, Arne Georg Gleditsch wrote:
@@ -199,7 +199,7 @@ static inline __attribute__((always_inline)) void pci_io_write_config16(device_t static inline __attribute__((always_inline)) void pci_mmio_write_config16(device_t dev, unsigned where, uint16_t value) { unsigned addr;
addr = CONFIG_MMCONF_BASE_ADDRESS | dev | where;
addr = dev | (where & ~1); write16x(addr, value);} #endif
This breaks non-AMD platforms I think.
Stefan
On 5/26/10 11:44 AM, Arne Georg Gleditsch wrote:
+#define MSR_GS_BASE 0xc0000101
#include <arch/mmio_conf.h>
+static inline __attribute__((always_inline)) void set_gs_base(uint64_t base) +{
- /* Make sure %gs is a valid descriptor */
- __asm__ volatile (
"mov %%ds, %%ax\n\tmov %%ax, %%gs" ::: "eax");- /* Set base */
- __asm__ volatile (
"wrmsr" :: "A"(base), "c"(MSR_GS_BASE));+}
This also breaks on all non-AMD platforms.
I think we should move all of this code to some AMD specific directory (northbridge or cpu) before applying these changes.
Comments?
Stefan Reinauer stefan.reinauer@coresystems.de writes:
This also breaks on all non-AMD platforms.
Augh, yes, it would. Sorry, I though I checked that there were no other users of this, but I obviously didn't.
I think we should move all of this code to some AMD specific directory (northbridge or cpu) before applying these changes.
Comments?
I guess it's either that or moving the AMD MMCONF range down below the 32-bit barrier. If we want to do the latter anyway, perhaps we should just do it now.
Arne Georg Gleditsch arne.gleditsch@numascale.com writes:
Augh, yes, it would. Sorry, I though I checked that there were no other users of this, but I obviously didn't.
(I guess I made this assumption on the basis of the fact that I had to fix several write8x to be of correct length in the pci_ops_mmconf.c file... Is anyone actually using this today?)
On 5/26/10 1:40 PM, Arne Georg Gleditsch wrote:
Is anyone actually using this today?
ICH7 is using it to set up the virtual channels for Azalia (and potentially other stuff)
I think there are some uses in some C7 chipset too, but I am not sure.
On 5/26/10 1:34 PM, Arne Georg Gleditsch wrote:
I guess it's either that or moving the AMD MMCONF range down below the 32-bit barrier. If we want to do the latter anyway, perhaps we should just do it now.
That alone won't do it,.. There's the AMD specific MSR and %gs is not set to MMCONFIG_BASE on other chipsets/cpus. Not sure what to do about the MSR,... the %gs thing should probably fixed by setting up %gs in a central place somewhere.
Stefan
Stefan Reinauer stefan.reinauer@coresystems.de writes:
That alone won't do it,.. There's the AMD specific MSR and %gs is not set to MMCONFIG_BASE on other chipsets/cpus. Not sure what to do about the MSR,... the %gs thing should probably fixed by setting up %gs in a central place somewhere.
If we move the MMCONF area into the 32-bit reachable range, we don't need to mess around with %gs. The other AMD specfic MSRs are localized to AMD specfic files, I believe.
On 26.05.2010 15:16, Arne Georg Gleditsch wrote:
Stefan Reinauer stefan.reinauer@coresystems.de writes:
That alone won't do it,.. There's the AMD specific MSR and %gs is not set to MMCONFIG_BASE on other chipsets/cpus. Not sure what to do about the MSR,... the %gs thing should probably fixed by setting up %gs in a central place somewhere.
If we move the MMCONF area into the 32-bit reachable range, we don't need to mess around with %gs. The other AMD specfic MSRs are localized to AMD specfic files, I believe.
IMHO we want that anyway to support 32bit operating systems.
Regards, Carl-Daniel
Am 26.05.2010 13:12, schrieb Stefan Reinauer:
On 5/26/10 11:44 AM, Arne Georg Gleditsch wrote:
@@ -199,7 +199,7 @@ static inline __attribute__((always_inline)) void pci_io_write_config16(device_t static inline __attribute__((always_inline)) void pci_mmio_write_config16(device_t dev, unsigned where, uint16_t value) { unsigned addr;
addr = CONFIG_MMCONF_BASE_ADDRESS | dev | where;
addr = dev | (where & ~1); write16x(addr, value);} #endif
This breaks non-AMD platforms I think.
Any solution to that?
This patch is required for a pending board addition, see http://patchwork.coreboot.org/patch/1387/
Patrick
Patrick Georgi patrick@georgi-clan.de writes:
Am 26.05.2010 13:12, schrieb Stefan Reinauer:
This breaks non-AMD platforms I think.
Any solution to that?
This patch is required for a pending board addition, see http://patchwork.coreboot.org/patch/1387/
I intend to rework the MMCONF patch to map the MMCONF area into 32-bit address space, as well as submit an updated patch for the HP DL165. I'm swamped in other stuff right now though; if anyone could give me some pointers on what the best way to reserve 256M of (256M-aligned) address space is, that would save me some time. Either dynamically or statically (I believe e0000000 - efffffff is often used for this). We'd like to hoist any memory behind this, so some method of allocation that makes this part of the IO hole would be good.
Arne Georg Gleditsch arne.gleditsch@numascale.com writes:
I intend to rework the MMCONF patch to map the MMCONF area into 32-bit address space, [..]
Please find appended. This patch gets rid of the %gs magic altogether, fixes a few alignment wrinkles and sets up and registers the MMCONF area for AMD Fam10h CPUs (where selected by mainboard configuration). It removes a bit of code that proved troublesome in MMCONF setups from mcp55_early_setup_car.c, as per earlier discussion.
(The way the patch hooks add_northbridge_resources via s2912_fam10/mainboard.c replicates a well-used pattern that perhaps ought to be simplified. HAVE_NORTHBRIDGE_RESOURCES would remove otherwise empty add_mainboard_resources from a lot of mainboard.c files.)
Signed-off-by: Arne Georg Gleditsch arne.gleditsch@numascale.com
On Wed, Sep 8, 2010 at 7:53 AM, Arne Georg Gleditsch arne.gleditsch@numascale.com wrote:
Arne Georg Gleditsch arne.gleditsch@numascale.com writes:
I intend to rework the MMCONF patch to map the MMCONF area into 32-bit address space, [..]
Please find appended. This patch gets rid of the %gs magic altogether, fixes a few alignment wrinkles and sets up and registers the MMCONF area for AMD Fam10h CPUs (where selected by mainboard configuration). It removes a bit of code that proved troublesome in MMCONF setups from mcp55_early_setup_car.c, as per earlier discussion.
(The way the patch hooks add_northbridge_resources via s2912_fam10/mainboard.c replicates a well-used pattern that perhaps ought to be simplified. HAVE_NORTHBRIDGE_RESOURCES would remove otherwise empty add_mainboard_resources from a lot of mainboard.c files.)
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?
+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.
Instead of adding the reserved area directly to the coreboot tables, you should add it before resource allocation and let the rest happen automatically.
Thanks, Myles
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.
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?
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
Here's a first stab at what I mean. It compiles, but I don't have the board to test.
Signed-off-by: Myles Watson mylesgw@gmail.com
Patrick and Stefan,
Could you comment. I'd like it even better if the resources were added earlier, but I didn't want to change too much at once.
Thanks, Myles
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?
I'd forgotten that this was the way it was done. I agree that you did it the same way that it had been done before. It's often hard to review a patch without reviewing the mechanisms it uses.
Thanks, Myles
Myles Watson mylesgw@gmail.com writes:
Here's a first stab at what I mean. It compiles, but I don't have the board to test.
Ok, so building on that, here's an updated version of the first patch. The resource is still fixed, but the gratuitous call through add_mainboard_resource is gone and the coreboot table manipulation is removed from the northbridge code.
Arne Georg Gleditsch arne.gleditsch@numascale.com writes:
Ok, so building on that, here's an updated version of the first patch. The resource is still fixed, but the gratuitous call through add_mainboard_resource is gone and the coreboot table manipulation is removed from the northbridge code.
For the record, this version is also
Signed-off-by: Arne Georg Gleditsch arne.gleditsch@numascale.com
On Thu, Sep 9, 2010 at 1:31 AM, Arne Georg Gleditsch arne.gleditsch@numascale.com wrote:
Arne Georg Gleditsch arne.gleditsch@numascale.com writes:
Ok, so building on that, here's an updated version of the first patch. The resource is still fixed, but the gratuitous call through add_mainboard_resource is gone and the coreboot table manipulation is removed from the northbridge code.
For the record, this version is also
Signed-off-by: Arne Georg Gleditsch arne.gleditsch@numascale.com
Acked-by: Myles Watson mylesgw@gmail.com
Rev 5795. Rev 5796.
I split it into two to make it easier to understand. I almost split it again so that your bug fixes were separate from the board-specific code.
Thanks, Myles
- - RES_PCI_IO, PCI_ADDR(0, 1, 0, 0x78), 0xC0FFFFFF, 0x19000000, + /* The following operation hangs when performed via MMCFG: + pci_read_config32(romcc): 00010000:0078: 20040000 + setup_resource_map_x_offset: 10000, 78: 20040000 + pci_write_config32(romcc): 00010000:0078: 19040000 + (hang) + Response missing? */ + /* RES_PCI_IO, PCI_ADDR(0, 1, 0, 0x78), 0xC0FFFFFF, 0x19000000, */
I forgot to ask if you'd tried setting the SyncOnWdError bit (20) in function 3, register 0x44. That could help further debug this problem. For me it caused a reboot instead of a hang when there was a response missing. Bit 21 could also be helpful.
Since we don't have the chipset documentation, it makes me a little worried to leave out one of the settings. Maybe we could use non MMCONF writes for that setting.
Thanks, Myles
Myles Watson mylesgw@gmail.com writes:
- RES_PCI_IO, PCI_ADDR(0, 1, 0, 0x78), 0xC0FFFFFF, 0x19000000,
- /* The following operation hangs when performed via MMCFG:
pci_read_config32(romcc): 00010000:0078: 20040000setup_resource_map_x_offset: 10000, 78: 20040000pci_write_config32(romcc): 00010000:0078: 19040000(hang)Response missing? */- /* RES_PCI_IO, PCI_ADDR(0, 1, 0, 0x78), 0xC0FFFFFF, 0x19000000, */
I forgot to ask if you'd tried setting the SyncOnWdError bit (20) in function 3, register 0x44. That could help further debug this problem. For me it caused a reboot instead of a hang when there was a response missing. Bit 21 could also be helpful.
I can have a look. I won't get a chance until Monday, though.
Since we don't have the chipset documentation, it makes me a little worried to leave out one of the settings. Maybe we could use non MMCONF writes for that setting.
We could fall back to that, if we want to keep it. FWIW, Ed Swierk had some input on the semantics of this register the last time this came up: http://thread.gmane.org/gmane.linux.bios/57708/focus=59900
Arne Georg Gleditsch arne.gleditsch@numascale.com writes:
- RES_PCI_IO, PCI_ADDR(0, 1, 0, 0x78), 0xC0FFFFFF, 0x19000000,
- /* The following operation hangs when performed via MMCFG:
pci_read_config32(romcc): 00010000:0078: 20040000setup_resource_map_x_offset: 10000, 78: 20040000pci_write_config32(romcc): 00010000:0078: 19040000(hang)Response missing? */- /* RES_PCI_IO, PCI_ADDR(0, 1, 0, 0x78), 0xC0FFFFFF, 0x19000000, */
I forgot to ask if you'd tried setting the SyncOnWdError bit (20) in function 3, register 0x44. That could help further debug this problem. For me it caused a reboot instead of a hang when there was a response missing. Bit 21 could also be helpful.
I can have a look. I won't get a chance until Monday, though.
Ah, I've just stumbled across something. src/mainboard/tyan/s2912_fam10/romstage.c:sio_setup does:
byte = pci_read_config8(PCI_DEV(0, MCP55_DEVN_BASE+1 , 0), 0x7b); byte |= 0x20; pci_write_config8(PCI_DEV(0, MCP55_DEVN_BASE+1 , 0), 0x7b, byte);
It appears that the offending RES_PCI_IO line is not so much hanging the system as disabling the serial console. Furthermore, the reason this does not happen when using pio-based config space accesses seems to be that several functions, for instance precisely sio_setup, are using pci_read/write_config before init_cpus has run and thus begore the MMCONF BAR is set up. This means these accesses are effectively ignored when MMCONF is enabled. Some of these are apparently required for the RES_PCI_IO in question to be set without affecting the serial output.
Rather than hunt around and try to change all config accesses that might run before init_cpus to explicit pio-accesses, I've moved the BAR init to run as early as possible, in cache_as_ram. With this change, I can re-insert the RES_PCI_IO wihtout ill effects.
We could fall back to that, if we want to keep it. FWIW, Ed Swierk had some input on the semantics of this register the last time this came up: http://thread.gmane.org/gmane.linux.bios/57708/focus=59900
(The correct link to Ed's post is http://thread.gmane.org/gmane.linux.bios/57708/focus=58810)
Arne Georg Gleditsch arne.gleditsch@numascale.com writes:
With this change, I can re-insert the RES_PCI_IO wihtout ill effects.
Move initialization of MMCONF BAR to cache_as_ram setup phase, in order to make sure MMCONF is set up before use. Otherwise, PCI config accesses run before init_cpus() will be lost if MMCONF is enabled (unless explicitly done as port-based accesses).
This obsoletes removal of RES_PCI_IO, PCI_ADDR(0, 1, 0, 0x78) in mcp55_early_setup, so reinsert.
Signed-off-by: Arne Georg Gleditsch arne.gleditsch@numascale.com
Arne Georg Gleditsch arne.gleditsch@numascale.com writes:
With this change, I can re-insert the RES_PCI_IO wihtout ill effects.
Move initialization of MMCONF BAR to cache_as_ram setup phase, in order to make sure MMCONF is set up before use. Otherwise, PCI config accesses run before init_cpus() will be lost if MMCONF is enabled (unless explicitly done as port-based accesses).
This obsoletes removal of RES_PCI_IO, PCI_ADDR(0, 1, 0, 0x78) in mcp55_early_setup, so reinsert.
Signed-off-by: Arne Georg Gleditsch arne.gleditsch@numascale.com
Acked-by: Myles Watson mylesgw@gmail.com
Thanks for tracking it down!
Myles
On Mon, Sep 13, 2010 at 6:52 AM, Myles Watson mylesgw@gmail.com wrote:
Arne Georg Gleditsch arne.gleditsch@numascale.com writes:
With this change, I can re-insert the RES_PCI_IO wihtout ill effects.
Move initialization of MMCONF BAR to cache_as_ram setup phase, in order to make sure MMCONF is set up before use. Otherwise, PCI config accesses run before init_cpus() will be lost if MMCONF is enabled (unless explicitly done as port-based accesses).
This obsoletes removal of RES_PCI_IO, PCI_ADDR(0, 1, 0, 0x78) in mcp55_early_setup, so reinsert.
Signed-off-by: Arne Georg Gleditsch arne.gleditsch@numascale.com
Acked-by: Myles Watson mylesgw@gmail.com
Rev 5810.
Arne: Could you help me understand MMCONF_SUPPORT and MMCONF_SUPPORT_DEFAULT? It looks like the area gets reserved for MMCONF_SUPPORT, even if MMCONF_SUPPORT_DEFAULT isn't selected.
Thanks, Myles
Thanks, Myles
Myles Watson mylesgw@gmail.com writes:
Rev 5810.
Thanks!
Arne: Could you help me understand MMCONF_SUPPORT and MMCONF_SUPPORT_DEFAULT? It looks like the area gets reserved for MMCONF_SUPPORT, even if MMCONF_SUPPORT_DEFAULT isn't selected.
My understanding of the code as it was before I started messing with it, is that MMCONF_SUPPORT is intended to indicate whether the facility is available at all, and MMCONF_SUPPORT_DEFAULT toggles the actual use of it (by Coreboot). Both src/northbridge/amd/amdfam10/northbridge.c and src/northbridge/intel/i945/northbridge.c contains code like this:
#if CONFIG_MMCONF_SUPPORT_DEFAULT .ops_pci_bus = &pci_ops_mmconf, #else .ops_pci_bus = &pci_cf8_conf1, #endif
which, alongside romcc_io.h's
#if CONFIG_MMCONF_SUPPORT_DEFAULT return pci_mmio_read_config32(dev, where); #else return pci_io_read_config32(dev, where); #endif
is more or less the only use for MMCONF_SUPPORT_DEFAULT as far as I can see. I'll leave it to others to determine if this actually makes sense.
(Having SUPPORT without SUPPORT_DEFAULT gives you an MCFG area that you can communicate to the OS (at least potentially), as well as the option of explicitly doing individual config accesses via mmio instead of ports. I don't think anyone is doing either of these today.)
Arne: Could you help me understand MMCONF_SUPPORT and MMCONF_SUPPORT_DEFAULT? It looks like the area gets reserved for MMCONF_SUPPORT, even if MMCONF_SUPPORT_DEFAULT isn't selected.
My understanding of the code as it was before I started messing with it, is that MMCONF_SUPPORT is intended to indicate whether the facility is available at all, and MMCONF_SUPPORT_DEFAULT toggles the actual use of it (by Coreboot). Both src/northbridge/amd/amdfam10/northbridge.c and src/northbridge/intel/i945/northbridge.c contains code like this:
#if CONFIG_MMCONF_SUPPORT_DEFAULT .ops_pci_bus = &pci_ops_mmconf, #else .ops_pci_bus = &pci_cf8_conf1, #endif
which, alongside romcc_io.h's
#if CONFIG_MMCONF_SUPPORT_DEFAULT return pci_mmio_read_config32(dev, where); #else return pci_io_read_config32(dev, where); #endif
is more or less the only use for MMCONF_SUPPORT_DEFAULT as far as I can see. I'll leave it to others to determine if this actually makes sense.
(Having SUPPORT without SUPPORT_DEFAULT gives you an MCFG area that you can communicate to the OS (at least potentially), as well as the option of explicitly doing individual config accesses via mmio instead of ports. I don't think anyone is doing either of these today.)
Thanks for the explanation. It looks like the trouble with reserving a region for MMCONF is related to broken UMA code.
Thanks, Myles
-
Arne Georg Gleditsch -
Bao, Zheng -
Carl-Daniel Hailfinger -
Ed Swierk -
Joseph Smith -
Myles Watson -
Patrick Georgi -
Stefan Reinauer -
Stefan Reinauer