Patchwork unstable AMD Fam10h boot

login
register
about
Submitter Marc Jones
Date 2009-09-14 00:46:38
Message ID <534e5dc20909131746s27fdac9bube60b3fec2535d8@mail.gmail.com>
Download mbox | patch
Permalink /patch/252/
State Accepted
Headers show

Comments

Marc Jones - 2009-09-14 00:46:38
On Tue, Sep 8, 2009 at 11:02 AM, Marc Jones <marcj303@gmail.com> wrote:
> On Sun, Sep 6, 2009 at 4:32 PM, ron minnich<rminnich@gmail.com> wrote:
>> The way I see it the memory setup and SMP support in CAR are two very
>> different issues.
>
> This bug is totally my fault...
>
> Yes,  Memory setup and SMP CAR are two different issues. The SMP setup
> happens during CAR is to setup microcode, HT and FIDVID prior to the
> PLL reset and memory setup.
>
> All the SMP PCI config space access should be MMIO. It is the first
> thing that is enabled in CPU init in set_pci_mmio_conf_reg().
>
> The bug is that I mixed a mem setup function in with SMP setup by
> using mctGetLogicalCPUID() which uses Get_NB32. As pointed out, the
> GET_NB32 is a cf8/cfc function. The mct code ported from AGESA assumes
> that it is running on the BSP only and uses cf8/cfc..... (historical
> k8 bug I think)
>
> I think that I should change the mct PCI config functions to call the
> coreboot pci_read_config32 functions that handle MMIO vs cfc/cf8
> nicely. This should future proof  mct functions in CAR and a step
> toward SMP memory setup.
>
> Some of that mct code PCI config space code is a little funny (ok, a
> lot funny), so it will take a little care. I should be able work patch
> in a couple of days.


Here is a patch that fixes the cf8 config access. Not complicated like
I initially recalled.  Thanks to Ralf for pointing out the bug.

This needs testing. Anyone?

Signed-off-by: Marc Jones <marcj303@gmail.com>

Thanks,
Marc
Ward Vandewege - 2009-09-14 02:13:43
On Sun, Sep 13, 2009 at 06:46:38PM -0600, Marc Jones wrote:
> Here is a patch that fixes the cf8 config access. Not complicated like
> I initially recalled.  Thanks to Ralf for pointing out the bug.
> 
> This needs testing. Anyone?

We've got a couple H8DME/fam10 boxes coming this week, so I should be able to
test this in a couple days. Will do as soon as we get the hardware.

Thanks,
Ward.
Marc Jones - 2009-09-14 16:45:07
I am not sure what I was thinking last night. This is really simple...
There is no address manipulation to be done before calling the
coreboot pci functions. Thanks Patrick....

Marc
Myles Watson - 2009-09-14 16:49:39
On Mon, Sep 14, 2009 at 10:45 AM, Marc Jones <marcj303@gmail.com> wrote:

> I am not sure what I was thinking last night. This is really simple...
> There is no address manipulation to be done before calling the
> coreboot pci functions. Thanks Patrick....
>

It looks like GetNB and SetNB should die.  Is there a purpose for having the
extra indirection?

Thanks,
Myles
Patrick Georgi - 2009-09-14 16:50:07
Am Montag, den 14.09.2009, 10:45 -0600 schrieb Marc Jones:
> I am not sure what I was thinking last night. This is really simple...
> There is no address manipulation to be done before calling the
> coreboot pci functions. Thanks Patrick....
The board boots much more reliable now, thank you!


Acked-by: Patrick Georgi <patrick.georgi@coresystems.de>
Peter Stuge - 2009-09-14 16:52:47
Myles Watson wrote:
> It looks like GetNB and SetNB should die.

+1


//Peter
Marc Jones - 2009-09-14 16:58:15
On Mon, Sep 14, 2009 at 10:52 AM, Peter Stuge <peter@stuge.se> wrote:
> Myles Watson wrote:
>> It looks like GetNB and SetNB should die.
>
> +1

I guess it should. The only reason to keep it is for easily diffing
against the AMD AGESA reference code, but I am not certain if or when
that will happen again.

Marc
Marc Jones - 2009-09-14 17:00:28
On Mon, Sep 14, 2009 at 10:50 AM, Patrick Georgi <patrick@georgi-clan.de> wrote:
> Am Montag, den 14.09.2009, 10:45 -0600 schrieb Marc Jones:
>> I am not sure what I was thinking last night. This is really simple...
>> There is no address manipulation to be done before calling the
>> coreboot pci functions. Thanks Patrick....
> The board boots much more reliable now, thank you!
>
>
> Acked-by: Patrick Georgi <patrick.georgi@coresystems.de>
>
>

Thanks Patrick.

r4633
Bao, Zheng - 2009-09-15 01:51:57
Are you sure the pci functions will cover the case that the address is
more than 0x100?


Zheng

-----Original Message-----
From: coreboot-bounces@coreboot.org
[mailto:coreboot-bounces@coreboot.org] On Behalf Of Marc Jones
Sent: Tuesday, September 15, 2009 12:45 AM
To: ron minnich
Cc: coreboot@coreboot.org
Subject: Re: [coreboot] unstable AMD Fam10h boot

I am not sure what I was thinking last night. This is really simple...
There is no address manipulation to be done before calling the
coreboot pci functions. Thanks Patrick....

Marc
Marc Jones - 2009-09-15 16:10:38
On Mon, Sep 14, 2009 at 7:51 PM, Bao, Zheng <Zheng.Bao@amd.com> wrote:
> Are you sure the pci functions will cover the case that the address is
> more than 0x100?

It should, unless you know something I don't (bug?). Using the MMIO
config access is the preferred method since it enforces the ordering.
See 2.11 Configuration Space in the BKDG.

Marc

Patch

Use the coreboot pci config read/write functions instead of direct cf8/cfc access. The fam10 pci functions will use mmio and not have  SMP pci access issues.

Signed-off-by: Marc Jones <marcj303@gmail.com↔

Index: coreboot-v2/src/northbridge/amd/amdmct/mct/mct_d.c
===================================================================
--- coreboot-v2.orig/src/northbridge/amd/amdmct/mct/mct_d.c	2009-09-13 18:01:54.000000000 -0600
+++ coreboot-v2/src/northbridge/amd/amdmct/mct/mct_d.c	2009-09-13 18:09:08.000000000 -0600
@@ -2474,10 +2474,8 @@ 
 {
 	u32 addr;
 
-	addr = (dev>>4) | (reg & 0xFF) | ((reg & 0xf00)<<16);
-	outl((1<<31) | (addr & ~3), 0xcf8);
-
-	return inl(0xcfc);
+	addr = ((dev>>4) | (reg & 0xFF) | ((reg & 0xf00)<<16)) & ~3;
+	return pci_read_config32(addr);
 }
 
 
@@ -2485,9 +2483,8 @@ 
 {
 	u32 addr;
 
-	addr = (dev>>4) | (reg & 0xFF) | ((reg & 0xf00)<<16);
-	outl((1<<31) | (addr & ~3), 0xcf8);
-	outl(val, 0xcfc);
+	addr = ((dev>>4) | (reg & 0xFF) | ((reg & 0xf00)<<16)) & ~3;
+	pci_write_config32(addr);
 }