Patchwork fix cpu ht speed display in rs780_gfx.c

login
register
about
Submitter Liu Tao
Date 2010-10-11 03:48:14
Message ID <AANLkTikoXsMWNBr=PpUBV5RYqupuBxb-5rikZ=pFo-Be@mail.gmail.com>
Download mbox | patch
Permalink /patch/2090/
State New
Headers show

Comments

Liu Tao - 2010-10-11 03:48:14
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 <liutao1980@gmail.com>
Warren Turkal - 2010-10-11 05:56:18
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 <liutao1980@gmail.com> 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 <liutao1980@gmail.com>
>
> 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. */
>
>
> --
> Regards,
> Liu Tao
>
> --
> coreboot mailing list: coreboot@coreboot.org
> http://www.coreboot.org/mailman/listinfo/coreboot
>
Paul Menzel - 2010-10-11 08:05:32
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 <liutao1980@gmail.com>

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 - 2010-10-11 09:54:50
On Mon, Oct 11, 2010 at 1:05 AM, Paul Menzel
<paulepanter@users.sourceforge.net> 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 - 2010-10-11 15:17:06
On Sun, Oct 10, 2010 at 9:48 PM, Liu Tao <liutao1980@gmail.com> 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 <liutao1980@gmail.com>
Acked-by: Myles Watson <mylesgw@gmail.com>

Thanks,
Myles
Liu Tao - 2010-10-15 02:34:22
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 <wt@penguintechs.org> wrote:
> From a pure style perspective, the code looks ok. Have you actually
> booted this on any boards to see if it is working?
Uwe Hermann - 2010-10-17 21:39:46
On Mon, Oct 11, 2010 at 09:17:06AM -0600, Myles Watson wrote:
> On Sun, Oct 10, 2010 at 9:48 PM, Liu Tao <liutao1980@gmail.com> 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 <liutao1980@gmail.com>
> Acked-by: Myles Watson <mylesgw@gmail.com>

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 - 2010-10-17 22:01:26
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 - 2010-10-18 16:01:14
> 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
Liu Tao - 2010-10-20 01:13:17
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 <uwe@hermann-uwe.de> 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?
Uwe Hermann - 2010-10-20 20:21:49
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.

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