[PATCH] fix cpu ht speed display in rs780_gfx.c

[Liu Tao]

Hello,

the original code reads cpu ht speed from HT chain 0's register.
the patch fix it to read the register from the chain which SB chip is on.

Signed-off-by: Liu Tao

Index: src/southbridge/amd/rs780/rs780_gfx.c
===================================================================
--- src/southbridge/amd/rs780/rs780_gfx.c	(revision 5923)
+++ src/southbridge/amd/rs780/rs780_gfx.c	(working copy)
@@ -302,7 +302,7 @@
    unsigned char * bpointer;
    volatile u32 * GpuF0MMReg;
    volatile u32 * pointer;
-	int i;
+	int i, sblk;
    u16 command;
    u32 value;
    u16 deviceid, vendorid;
@@ -453,9 +453,15 @@
    vgainfo.usMinNBVoltage = 0;
    vgainfo.usBootUpNBVoltage = 0x1a;

+	/* SB link */
+	value = pci_read_config32(k8_f0, 0x64);
+	sblk = (value >> 8) & 0x3;
+	printk(BIOS_DEBUG, "SBLK = %d.\n", sblk);
+
+	/* HT speed */
    value = pci_read_config32(nb_dev, 0xd0);
    printk(BIOS_DEBUG, "NB HT speed = %x.\n", value);
-	value = pci_read_config32(k8_f0, 0x88);
+	value = pci_read_config32(k8_f0, 0x88 + (sblk * 0x20));
    printk(BIOS_DEBUG, "CPU HT speed = %x.\n", value);
    vgainfo.ulHTLinkFreq = 100 * 100; /* set HT speed. */

[Warren Turkal]

> From a pure style perspective, the code looks ok. Have you actually

booted this on any boards to see if it is working?

Thanks,
wt

On Sun, Oct 10, 2010 at 8:48 PM, Liu Tao  wrote:
> Hello,
>
> the original code reads cpu ht speed from HT chain 0's register.
> the patch fix it to read the register from the chain which SB chip is on.
>
> Signed-off-by: Liu Tao
> Index: src/southbridge/amd/rs780/rs780_gfx.c
>
> --- src/southbridge/amd/rs780/rs780_gfx.c       (revision 5923)
> +++ src/southbridge/amd/rs780/rs780_gfx.c       (working copy)
> @@ -302,7 +302,7 @@
>        unsigned char * bpointer;
>        volatile u32 * GpuF0MMReg;
>        volatile u32 * pointer;
> int i;
> int i, sblk;
>
> u16 command;
>        u32 value;
>        u16 deviceid, vendorid;
> @@ -453,9 +453,15 @@
>        vgainfo.usMinNBVoltage = 0;
>        vgainfo.usBootUpNBVoltage = 0x1a;
> /* SB link */
> value = pci_read_config32(k8_f0, 0x64);
> sblk = (value >> 8) & 0x3;
> printk(BIOS_DEBUG, "SBLK = %d.\n", sblk);
> /* HT speed */
>
> value = pci_read_config32(nb_dev, 0xd0);
>        printk(BIOS_DEBUG, "NB HT speed = %x.\n", value);
> value = pci_read_config32(k8_f0, 0x88);
> value = pci_read_config32(k8_f0, 0x88 + (sblk * 0x20));
>
> printk(BIOS_DEBUG, "CPU HT speed = %x.\n", value);
>        vgainfo.ulHTLinkFreq = 100 * 100; /* set HT speed. */

[Liu Tao]

Hi Warren,

the patch is tested on real board. I'm porting coreboot to a K8 board
with RS780+SB700 chipset, the RS780 is linked in CPU's HT chain 1,
after patching I get the correct HT_SPEED register value.

On 10/11/10, Warren Turkal  wrote:
> From a pure style perspective, the code looks ok. Have you actually
> booted this on any boards to see if it is working?

[Paul Menzel]

Dear Liu,

Am Montag, den 11.10.2010, 11:48 +0800 schrieb Liu Tao:
> the original code reads cpu ht speed from HT chain 0's register.
> the patch fix it to read the register from the chain which SB chip is on.
>
> Signed-off-by: Liu Tao

Thank you for your patch.
> Index: src/southbridge/amd/rs780/rs780_gfx.c
>
> --- src/southbridge/amd/rs780/rs780_gfx.c	(revision 5923)
> +++ src/southbridge/amd/rs780/rs780_gfx.c	(working copy)
> @@ -302,7 +302,7 @@
>    unsigned char * bpointer;
>    volatile u32 * GpuF0MMReg;
>    volatile u32 * pointer;
> int i;
> int i, sblk;
> u16 command;
> u32 value;
> u16 deviceid, vendorid;
>
> @@ -453,9 +453,15 @@
>    vgainfo.usMinNBVoltage = 0;
>    vgainfo.usBootUpNBVoltage = 0x1a;
> /* SB link */
> value = pci_read_config32(k8_f0, 0x64);
> sblk = (value >> 8) & 0x3;
> printk(BIOS_DEBUG, "SBLK = %d.\n", sblk);

Could you make that message more elaborate please. Like »SB link = …«?
> /* HT speed */
> value = pci_read_config32(nb_dev, 0xd0);
> printk(BIOS_DEBUG, "NB HT speed = %x.\n", value);
> value = pci_read_config32(k8_f0, 0x88);
> value = pci_read_config32(k8_f0, 0x88 + (sblk * 0x20));
> printk(BIOS_DEBUG, "CPU HT speed = %x.\n", value);
> vgainfo.ulHTLinkFreq = 100 * 100; /* set HT speed. */

Thanks,

Paul

[Warren Turkal]

On Mon, Oct 11, 2010 at 1:05 AM, Paul Menzel
 wrote:
> > printk(BIOS_DEBUG, "SBLK = %d.\n", sblk);
>
> Could you make that message more elaborate please. Like »SB link = …«?

A more descriptive name for the sblk variable would be nice also.

Thanks,
wt

[Myles Watson]

On Sun, Oct 10, 2010 at 9:48 PM, Liu Tao  wrote:
> Hello,
>
> the original code reads cpu ht speed from HT chain 0's register.
> the patch fix it to read the register from the chain which SB chip is on.
>
> Signed-off-by: Liu Tao

Acked-by: Myles Watson

Thanks,
Myles

[Uwe Hermann]

On Mon, Oct 11, 2010 at 09:17:06AM -0600, Myles Watson wrote:
> On Sun, Oct 10, 2010 at 9:48 PM, Liu Tao  wrote:
> > Hello,
> >
> > the original code reads cpu ht speed from HT chain 0's register.
> > the patch fix it to read the register from the chain which SB chip is on.
> >
> > Signed-off-by: Liu Tao
>
> Acked-by: Myles Watson

Thanks, committed as r5958 with small changes as suggested:

- Renamed sblk to sblink (the name of the register bits as per
   BIOS + Kernel Developer's Guide for AMD Athlon 64 & AMD Opteron Processors,
   chapter 3.3.8.

- Made sblink an u32 instead of int.

- Mention full name/description of sblink in a comment:
   "HyperTransport I/O Hub Link ID"

Uwe.

[Uwe Hermann]

On Sun, Oct 17, 2010 at 11:39:46PM +0200, Uwe Hermann wrote:
> Renamed sblk to sblink (the name of the register bits as per
> BIOS + Kernel Developer's Guide for AMD Athlon 64 & AMD Opteron Processors,
> chapter 3.3.8.

Hm, I noticed that the K8/Fam10h code uses "sblk" as function/variable name
pretty consistently, but I can't seem to find that spelling in any of
the AMD manuals. Should we rename "sblk" to "sblink" in the whole tree?

Uwe.

[Myles Watson]

> On Sun, Oct 17, 2010 at 11:39:46PM +0200, Uwe Hermann wrote:
> > Renamed sblk to sblink (the name of the register bits as per
> > BIOS + Kernel Developer's Guide for AMD Athlon 64 & AMD Opteron
>
> Processors,
> > chapter 3.3.8.
>
> Hm, I noticed that the K8/Fam10h code uses "sblk" as function/variable
> name
> pretty consistently, but I can't seem to find that spelling in any of
> the AMD manuals. Should we rename "sblk" to "sblink" in the whole tree?

I'm not sure sblink is that much better of a name than sblk.  I'm assuming
that the AMD reference code that the folks from AMD use has sblk in it.

Thanks,
Myles

[Uwe Hermann]

On Mon, Oct 18, 2010 at 10:01:14AM -0600, Myles Watson wrote:
> > On Sun, Oct 17, 2010 at 11:39:46PM +0200, Uwe Hermann wrote:
> > > Renamed sblk to sblink (the name of the register bits as per
> > > BIOS + Kernel Developer's Guide for AMD Athlon 64 & AMD Opteron
> >
> > Processors,
> > > chapter 3.3.8.
> >
> > Hm, I noticed that the K8/Fam10h code uses "sblk" as function/variable
> > name
> > pretty consistently, but I can't seem to find that spelling in any of
> > the AMD manuals. Should we rename "sblk" to "sblink" in the whole tree?
>
> I'm not sure sblink is that much better of a name than sblk.  I'm assuming
> that the AMD reference code that the folks from AMD use has sblk in it.

Ok, reverted to sblk for now. If we ever change it, we should change all
occurences anyway.

Uwe.

[Liu Tao]

The name of sblink seems more clear, but for most cases, the name of sblk
only appears in K8/Fam10 codes, so it didn't cause much confusions.

A more consistent way to get HT southbridge link in sb700 chipset is to include
the <cpu/amd/amdk8_sysconf.h> or <cpu/amd/amdfam10_sysconf.h>, and
use global variable sysconf.sblk. For now, fam10 codes set sysconf.sblk during
early romstage, but K8 codes didn't. Maybe we should fix K8 coeds to set
sysconf.sblk earlyer, and use it in later device drivers.

On Mon, Oct 18, 2010 at 6:01 AM, Uwe Hermann  wrote:
> On Sun, Oct 17, 2010 at 11:39:46PM +0200, Uwe Hermann wrote:
> > - Renamed sblk to sblink (the name of the register bits as per
> >    BIOS + Kernel Developer's Guide for AMD Athlon 64 & AMD Opteron Processors,
> >    chapter 3.3.8.
>
> Hm, I noticed that the K8/Fam10h code uses "sblk" as function/variable name
> pretty consistently, but I can't seem to find that spelling in any of
> the AMD manuals. Should we rename "sblk" to "sblink" in the whole tree?