[PATCH] Istanbul support

[Arne Georg Gleditsch]

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 <[e-mail]>

-- 
								Arne.

[Bao, Zheng]

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 <[e-mail]>
> -----Original Message-----
> From: [e-mail]
> [mailto:[e-mail]]
> On Behalf Of Arne Georg Gleditsch
> Sent: Friday, March 05, 2010 10:41 PM
> To: [e-mail]
> 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 <[e-mail]>

[Arne Georg Gleditsch]

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);	// 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.

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.

[Arne Georg Gleditsch]

Arne Georg Gleditsch <[e-mail]> 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.

[Stefan Reinauer]

> Arne Georg Gleditsch <[e-mail]> 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


-- 
coresystems GmbH • Brahmsstr. 16 • D-79104 Freiburg i. Br.
      Tel.: [phone] • Fax: [phone]
Email: [e-mail]  • http://www.coresystems.de/
Registergericht: Amtsgericht Freiburg • HRB 7656
Geschäftsführer: Stefan Reinauer • Ust-IdNr.: DE245674866

[Arne Georg Gleditsch]

Stefan Reinauer <[e-mail]> 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).

-- 
							Arne.

[Carl-Daniel Hailfinger]

On 13.04.2010 09:28, Arne Georg Gleditsch wrote:
> Stefan Reinauer <[e-mail]> 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

-- 
http://www.hailfinger.org/

[Arne Georg Gleditsch]

Carl-Daniel Hailfinger <[e-mail]> writes:
> On 13.04.2010 09:28, Arne Georg Gleditsch wrote:
> > Stefan Reinauer <[e-mail]> 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.)

-- 
							Arne.

[Ed Swierk]

On Mon, Apr 12, 2010 at 3:28 AM, Arne Georg Gleditsch
<[e-mail]> 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

[Joseph Smith]

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

-- 
Thanks,
Joseph Smith
Set-Top-Linux
www.settoplinux.org

[Arne Georg Gleditsch]

Ed Swierk <[e-mail]> 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?

-- 
							Arne.

[Ed Swierk]

On Tue, Apr 13, 2010 at 12:35 AM, Arne Georg Gleditsch
<[e-mail]> 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

[Carl-Daniel Hailfinger]

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

-- 
http://www.hailfinger.org/

[Ed Swierk]

On Tue, Apr 13, 2010 at 12:35 AM, Arne Georg Gleditsch
<[e-mail]> 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

[PATCH] AMD MMCONF Support

[Arne Georg Gleditsch]

Ed Swierk <[e-mail]> 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 <[e-mail]>

-- 
							Arne.

[PATCH] AMD MMCONF Support

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

Stefan

-- 
coresystems GmbH • Brahmsstr. 16 • D-79104 Freiburg i. Br.
      Tel.: [phone] • Fax: [phone]
Email: [e-mail]  • http://www.coresystems.de/
Registergericht: Amtsgericht Freiburg • HRB 7656
Geschäftsführer: Stefan Reinauer • Ust-IdNr.: DE245674866

[PATCH] AMD MMCONF Support

[Patrick Georgi]

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

[PATCH] AMD MMCONF Support

[Arne Georg Gleditsch]

Patrick Georgi <[e-mail]> 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.

[PATCH] AMD MMCONF Support

[Arne Georg Gleditsch]

Arne Georg Gleditsch <[e-mail]> 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 <[e-mail]>

-- 
							Arne.

[PATCH] AMD MMCONF Support

[Myles Watson]

On Wed, Sep 8, 2010 at 7:53 AM, Arne Georg Gleditsch
<[e-mail]> wrote:
> Arne Georg Gleditsch <[e-mail]> 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

[PATCH] AMD MMCONF Support

[Arne Georg Gleditsch]

Myles Watson <[e-mail]> 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?

-- 
							Arne.

[PATCH] AMD MMCONF Support

[Myles Watson]

On Wed, Sep 8, 2010 at 9:00 AM, Arne Georg Gleditsch
<[e-mail]> wrote:
> Myles Watson <[e-mail]> 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

[PATCH] AMD MMCONF Support

[Myles Watson]

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 <[e-mail]>

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

[PATCH] AMD MMCONF Support

[Arne Georg Gleditsch]

Myles Watson <[e-mail]> 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.

[PATCH] AMD MMCONF Support

[Arne Georg Gleditsch]

Arne Georg Gleditsch <[e-mail]> 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 <[e-mail]>

-- 
							Arne.

[PATCH] AMD MMCONF Support

[Myles Watson]

On Thu, Sep 9, 2010 at 1:31 AM, Arne Georg Gleditsch
<[e-mail]> wrote:
> Arne Georg Gleditsch <[e-mail]> 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 <[e-mail]>
> Acked-by: Myles Watson <[e-mail]>

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

[PATCH] AMD MMCONF Support

[Myles Watson]

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

[PATCH] AMD MMCONF Support

[Arne Georg Gleditsch]

Myles Watson <[e-mail]> 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: 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.

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.

[PATCH] AMD MMCONF Support

[Arne Georg Gleditsch]

Arne Georg Gleditsch <[e-mail]> 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: 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.
>
> 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.

[PATCH] AMD MMCONF Support

[Arne Georg Gleditsch]

Arne Georg Gleditsch <[e-mail]> 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 <[e-mail]>

-- 
							Arne.

[PATCH] AMD MMCONF Support

[Myles Watson]

> Arne Georg Gleditsch <[e-mail]> 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 <[e-mail]>

Acked-by: Myles Watson <[e-mail]>

Thanks for tracking it down!

Myles

[PATCH] AMD MMCONF Support

[Myles Watson]

On Mon, Sep 13, 2010 at 6:52 AM, Myles Watson <[e-mail]> wrote:
> > Arne Georg Gleditsch <[e-mail]> 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 <[e-mail]>
>
> Acked-by: Myles Watson <[e-mail]>

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

[PATCH] AMD MMCONF Support

[Arne Georg Gleditsch]

Myles Watson <[e-mail]> 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.

[PATCH] AMD MMCONF Support

[Myles Watson]

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

[PATCH] AMD MMCONF Support

[Myles Watson]

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

[PATCH] AMD MMCONF Support

[Stefan Reinauer]

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?

-- 
coresystems GmbH • Brahmsstr. 16 • D-79104 Freiburg i. Br.
      Tel.: [phone] • Fax: [phone]
Email: [e-mail]  • http://www.coresystems.de/
Registergericht: Amtsgericht Freiburg • HRB 7656
Geschäftsführer: Stefan Reinauer • Ust-IdNr.: DE245674866

[PATCH] AMD MMCONF Support

[Arne Georg Gleditsch]

Stefan Reinauer <[e-mail]> 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.

[PATCH] AMD MMCONF Support

[Arne Georg Gleditsch]

Arne Georg Gleditsch <[e-mail]> 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?)

-- 
							Arne.

[PATCH] AMD MMCONF Support

[Stefan Reinauer]

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.


-- 
coresystems GmbH • Brahmsstr. 16 • D-79104 Freiburg i. Br.
      Tel.: [phone] • Fax: [phone]
Email: [e-mail]  • http://www.coresystems.de/
Registergericht: Amtsgericht Freiburg • HRB 7656
Geschäftsführer: Stefan Reinauer • Ust-IdNr.: DE245674866

[PATCH] AMD MMCONF Support

[Stefan Reinauer]

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

-- 
coresystems GmbH • Brahmsstr. 16 • D-79104 Freiburg i. Br.
      Tel.: [phone] • Fax: [phone]
Email: [e-mail]  • http://www.coresystems.de/
Registergericht: Amtsgericht Freiburg • HRB 7656
Geschäftsführer: Stefan Reinauer • Ust-IdNr.: DE245674866

[PATCH] AMD MMCONF Support

[Arne Georg Gleditsch]

Stefan Reinauer <[e-mail]> 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.

-- 
							Arne.

[PATCH] AMD MMCONF Support

[Carl-Daniel Hailfinger]

On 26.05.2010 15:16, Arne Georg Gleditsch wrote:
> Stefan Reinauer <[e-mail]> 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

-- 
http://www.hailfinger.org/

[Bao, Zheng]

Acked-by: Zheng Bao <[e-mail]>
> -----Original Message-----
> From: [e-mail]
> [mailto:[e-mail]]
> On Behalf Of Arne Georg Gleditsch
> Sent: Friday, March 05, 2010 10:41 PM
> To: [e-mail]
> 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 <[e-mail]>

[Bao, Zheng]

r5200.
> -----Original Message-----
> From: [e-mail]
> [mailto:[e-mail]]
> On Behalf Of Bao, Zheng
> Sent: Wednesday, March 10, 2010 11:39 AM
> To: Arne Georg Gleditsch
> Cc: [e-mail]
> Subject: Re: [coreboot] [PATCH] Istanbul support
>
> Acked-by: Zheng Bao <[e-mail]>
> > -----Original Message-----
> > From: [e-mail]
> > [mailto:[e-mail]]
> > On Behalf Of Arne Georg Gleditsch
> > Sent: Friday, March 05, 2010 10:41 PM
> > To: [e-mail]
> > 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 <[e-mail]>