[LinuxBIOS] [PATCH] K8T890 V-link fine-tune

[Rudolf Marek]

Hello,

Attached patch fine-tunes the V-link bus between K8T890 and VT8237R and set it 
to 8X transfer rate (up to 1066 MB/s) similar code placed here would be needed 
for VT8237A/S etc. Using VIA recommended values despite they are for K8T890CF, 
this is K8T890CE (still dont know what is exactly different).

This patch enables the parity error reporting on V-Link, so it enables NMI 
generation for the SERR# errors. The NMI may not be generated, maybe port 61h
needs some tuning too.

Signed-off-by: Rudolf Marek

Btw there is FID/VID patch pending too.

Rudolf

[Uwe Hermann]

On Mon, Nov 12, 2007 at 11:24:37PM +0100, Rudolf Marek wrote:
> Hello,
>
> Attached patch fine-tunes the V-link bus between K8T890 and VT8237R and set 
> it to 8X transfer rate (up to 1066 MB/s) similar code placed here would be 
> needed for VT8237A/S etc. Using VIA recommended values despite they are for 
> K8T890CF, this is K8T890CE (still dont know what is exactly different).
>
> This patch enables the parity error reporting on V-Link, so it enables NMI 
> generation for the SERR# errors. The NMI may not be generated, maybe port 
> 61h
> needs some tuning too.
>
> Signed-off-by: Rudolf Marek

Thanks, r2958 with some changes, see below.
> +#include <device/device.h>
> +#include <device/pci.h>
> +#include <device/pci_ops.h>
> +#include <device/pci_ids.h>
> +#include <console/console.h>
> +#include <cpu/x86/msr.h>
> +#include <cpu/amd/mtrr.h>

I dropped

#include <device/pci_ops.h>
  #include <cpu/x86/msr.h>
  #include <cpu/amd/mtrr.h>

as they're not needed.
> +static void error_enable(struct device *dev)
> +{
> /*
> * bit0 - Enable V-link parity error reporting in 0x50 bit0 (RWC)
> * bit6 - Parity Error/SERR# Report Through V-Link to SB
> * bit7 - Parity Error/SERR# Report Through NMI
> */
> pci_write_config8(dev, 0x58, 0x81);
>
> +}
>
> +static struct device_operations error_ops = {
> .read_resources = pci_dev_read_resources,
> .set_resources = pci_dev_set_resources,
> .enable_resources = pci_dev_enable_resources,
> .enable = error_enable,
> .ops_pci = 0,
>
> +};
>
> +static const struct pci_driver northbridge_driver __pci_driver = {
> .ops = &error_ops,
> .vendor = PCI_VENDOR_ID_VIA,
> .device = PCI_DEVICE_ID_VIA_K8T890CE_1,

This should probably be renamed to PCI_DEVICE_ID_VIA_K8T890CE_ERROR later.
> +};
> Index: src/southbridge/via/k8t890/k8t890_ctrl.c
> ===================================================================
> --- src/southbridge/via/k8t890/k8t890_ctrl.c	(revision 2953)
> +++ src/southbridge/via/k8t890/k8t890_ctrl.c	(working copy)
> @@ -23,6 +23,48 @@
>  #include <device/pci_ids.h>
>  #include <console/console.h>
>
> +static void ctrl_init(struct device *dev)
> +{
> u8 reg;
> device_t devsb = dev_find_device(PCI_VENDOR_ID_VIA,
> 			 PCI_DEVICE_ID_VIA_VT8237R_LPC, 0);
> /* Setup the V-Link for VT8237R, 8X mode */
> if (devsb) {

Do we even need this?

The
        .init = ctrl_init,

and the

static const struct pci_driver northbridge_driver __pci_driver = {
            .ops = &ctrl_ops,
            .vendor = PCI_VENDOR_ID_VIA,
            .device = PCI_DEVICE_ID_VIA_K8T890CE_7,
    };

will only call this function for the PCI_DEVICE_ID_VIA_K8T890CE_7 device,
no need for explicit checks. Are the registers accessed from
PCI_DEVICE_ID_VIA_K8T890CE_7 only?

Also, why check for VT8237R's LPC device in the K8T890 code? Drop it?
> /* for K8T890CF VIA recommends what is in VIA column, AW is award 8X
> *						REG	DEF	AW	VIA-8X	VIA-4X
> * NB V-Link Manual Driving Control strobe	0xb5	0x46	0x46	0x88	0x88
> * NB V-Link Manual Driving Control - Data	0xb6	0x46	0x46	0x88	0x88
> * NB V-Link Receiving Strobe Delay		0xb7	0x02	0x02	0x61	0x01
> * NB V-Link Compensation Control bit4,0 (b5,b6)0xb4	0x10	0x10	0x11	0x11
> * SB V-Link Strobe Drive Control 		0xb9	0x00	0xa5	0x98	0x98
> * SB V-Link Data drive Control????		0xba	0x00	0xbb	0x77	0x77
> * SB V-Link Receive Strobe Delay????		0xbb	0x04	0x11	0x11	0x11
> * SB V-Link Compensation Control bit0 (use b9)	0xb8	0x00	0x01	0x01 	0x01
> * V-Link CKG Control				0xb0	0x05	0x05	0x06	0x03
> * V-Link CKG Control				0xb1	0x05	0x05	0x01	0x03
> */
> pci_write_config8(dev, 0xb5, 0x88);
> pci_write_config8(dev, 0xb6, 0x88);
> pci_write_config8(dev, 0xb7, 0x61);
> reg = pci_read_config8(dev, 0xb4);
> reg |= 0x11;
> pci_write_config8(dev, 0xb4, reg);
> pci_write_config8(dev, 0xb9, 0x98);
> pci_write_config8(dev, 0xba, 0x77);
> pci_write_config8(dev, 0xbb, 0x11);
> reg = pci_read_config8(dev, 0xb8);
> reg |= 0x1;
> pci_write_config8(dev, 0xb8, reg);
> pci_write_config8(dev, 0xb0, 0x06);
> pci_write_config8(dev, 0xb1, 0x01);
> /* Program V-link 8X 16bit full duplex, parity enabled */
> pci_write_config8(dev, 0x48, 0xa3);
> }

I've refactored this a bit, there should be no semantic changes though.

Uwe.

[Rudolf Marek]

Uwe Hermann wrote:
> On Mon, Nov 12, 2007 at 11:24:37PM +0100, Rudolf Marek wrote:
> > Hello,
> >
> > Attached patch fine-tunes the V-link bus between K8T890 and VT8237R and set 
> > it to 8X transfer rate (up to 1066 MB/s) similar code placed here would be 
> > needed for VT8237A/S etc. Using VIA recommended values despite they are for 
> > K8T890CF, this is K8T890CE (still dont know what is exactly different).
> >
> > This patch enables the parity error reporting on V-Link, so it enables NMI 
> > generation for the SERR# errors. The NMI may not be generated, maybe port 
> > 61h
> > needs some tuning too.
> >
> > Signed-off-by: Rudolf Marek
>
> Thanks, r2958 with some changes, see below.
> > +#include <device/device.h>
> > +#include <device/pci.h>
> > +#include <device/pci_ops.h>
> > +#include <device/pci_ids.h>
> > +#include <console/console.h>
> > +#include <cpu/x86/msr.h>
> > +#include <cpu/amd/mtrr.h>
>
> I dropped
>
> #include <device/pci_ops.h>
>   #include <cpu/x86/msr.h>
>   #include <cpu/amd/mtrr.h>
>
> as they're not needed.

hmm aha cut and paste ;) Thanks.
> > +static void error_enable(struct device *dev)
> > +{
> > /*
> > * bit0 - Enable V-link parity error reporting in 0x50 bit0 (RWC)
> > * bit6 - Parity Error/SERR# Report Through V-Link to SB
> > * bit7 - Parity Error/SERR# Report Through NMI
> > */
> > pci_write_config8(dev, 0x58, 0x81);
> >
> > +}
> >
> > +static struct device_operations error_ops = {
> > .read_resources = pci_dev_read_resources,
> > .set_resources = pci_dev_set_resources,
> > .enable_resources = pci_dev_enable_resources,
> > .enable = error_enable,
> > .ops_pci = 0,
> >
> > +};
> >
> > +static const struct pci_driver northbridge_driver __pci_driver = {
> > .ops = &error_ops,
> > .vendor = PCI_VENDOR_ID_VIA,
> > .device = PCI_DEVICE_ID_VIA_K8T890CE_1,
>
> This should probably be renamed to PCI_DEVICE_ID_VIA_K8T890CE_ERROR later.
> > +};
> > Index: src/southbridge/via/k8t890/k8t890_ctrl.c
> > ===================================================================
> > --- src/southbridge/via/k8t890/k8t890_ctrl.c	(revision 2953)
> > +++ src/southbridge/via/k8t890/k8t890_ctrl.c	(working copy)
> > @@ -23,6 +23,48 @@
> >  #include <device/pci_ids.h>
> >  #include <console/console.h>
> >
> > +static void ctrl_init(struct device *dev)
> > +{
> > u8 reg;
> > device_t devsb = dev_find_device(PCI_VENDOR_ID_VIA,
> > 			 PCI_DEVICE_ID_VIA_VT8237R_LPC, 0);
> > /* Setup the V-Link for VT8237R, 8X mode */
> > if (devsb) {
>
> Do we even need this?
>
> The
>         .init = ctrl_init,
>
> and the
>
> static const struct pci_driver northbridge_driver __pci_driver = {
>            .ops = &ctrl_ops,
>            .vendor = PCI_VENDOR_ID_VIA,
>            .device = PCI_DEVICE_ID_VIA_K8T890CE_7,
>    };
>
> will only call this function for the PCI_DEVICE_ID_VIA_K8T890CE_7 device,
> no need for explicit checks. Are the registers accessed from
> PCI_DEVICE_ID_VIA_K8T890CE_7 only?
>
> Also, why check for VT8237R's LPC device in the K8T890 code? Drop it?

We check because all those values are specific for the SB & NB combination,
for other SBs this should be extended for VT8237A VT8237S VT8237 (without plus 
R) and VT8251 too.
> I've refactored this a bit, there should be no semantic changes though.

Well I really need it like it was, because I can add those numbers for other via 
SB... Image there some kind of case statement with different PCIids or just 
block with the find_pci_device...

Rudolf

[Uwe Hermann]

On Tue, Nov 13, 2007 at 11:04:49PM +0100, Rudolf Marek wrote:
> > Also, why check for VT8237R's LPC device in the K8T890 code? Drop it?
>
> We check because all those values are specific for the SB & NB combination,
> for other SBs this should be extended for VT8237A VT8237S VT8237 (without 
> plus R) and VT8251 too.

Ah, ok, makes sense. I added a comment in the code to reflect that.

Uwe.