Re: [coreboot] [PATCH] KT890 autosetup LDT speeds from AMD NB

[Uwe Hermann]

Hi,

ok, I tested this on my ASUS A8V-E Deluxe, works fine without me having to
change the k8t890_early_setup_car() call.

So with the changes proposed below, and if you add a Signed-off-by, this is

Acked-by: Uwe Hermann

On Sat, Mar 15, 2008 at 07:05:39PM +0100, Uwe Hermann wrote:
> On Sat, Mar 15, 2008 at 02:12:18AM +0100, Rudolf Marek wrote:
> > Following experimental patch will setup KT890 HT automatically. It will find the
> > max width of the link and also it will take the frequency of HT "setuped"
> > already by coreboot (and checks if t can run on it).
> >
> > Uwe, this should fix your board.
>
> Great, will test later.
>
> Please repost with a Signed-off-by though, you forgot it for this patch.
> > Index: src/southbridge/via/k8t890/k8t890_early_car.c
> >
> > --- src/southbridge/via/k8t890/k8t890_early_car.c	(revision 3148)
> > +++ src/southbridge/via/k8t890/k8t890_early_car.c	(working copy)
> > @@ -22,35 +22,69 @@
> > generate PCI reset or LDTSTOP to apply.
> >
> > */
> >
> > -u8 k8t890_early_setup_car(u8 width, u8 speed)
> >
> > +static ldtreg[3] = {0x86, 0xa6, 0xc6};
>
> The values should be documented in a comment, though.
> > +static min(unsigned int a, unsigned int b) {
> > return (a < b) ? a : b ;
> >
> > +}
>
> Please make this
>
> #define MIN(a,b) ((a) < (b) ? (a) : (b))
>   #define MAX(a,b) ((a) > (b) ? (a) : (b))
>
> and add it to stdlib.h where similar stuff (e.g. ARRAY_SIZE) lives.
> It's not k8t890-specific and useful elsewhere.
> > +u8 k8t890_early_setup_HT(void)
>
> Please add a high-level comment of what's going on in this function.
> > {
> > u8 awidth, aspeed;
> > u8 awidth, afreq;
> > u8 cldtwidth_in, cldtwidth_out, vldtwidth_in, vldtwidth_out, ldtnr, width;
> > u8 cldtfreq;
> > u16 vldtcaps;
> > /* check if connected non coherent, initcomplete */
> > if (0x7 == pci_read_config8(PCI_DEV(0, 0x18, 0), 0x98)) {
> > ldtnr = 0;
> > } else if (0x7 == pci_read_config8(PCI_DEV(0, 0x18, 0), 0xb8)) {
> > ldtnr = 1;
> > } else if (0x7 == pci_read_config8(PCI_DEV(0, 0x18, 0), 0xd8)) {
> > ldtnr = 2;
> > }
> > print_debug("LDT width and speed for K8T890 was");
> > print_debug("K8T890f found at LDT ");
>
> K8T890f? Is the "f" intentional?
> > print_debug_hex8(ldtnr);
> > /* get the maximum widths for both sides */
> > cldtwidth_in = pci_read_config8(PCI_DEV(0, 0x18, 0), ldtreg[ldtnr]) & 0x7;
> > cldtwidth_out = (pci_read_config8(PCI_DEV(0, 0x18, 0), ldtreg[ldtnr]) >> 4) & 0x7;
> > vldtwidth_in = pci_read_config8(PCI_DEV(0, 0x0, 0), 0x66) & 0x7;
> > vldtwidth_out = (pci_read_config8(PCI_DEV(0, 0x0, 0), 0x66) >> 4) & 0x7;
> > width = min(min(min(cldtwidth_out, cldtwidth_in), vldtwidth_out), vldtwidth_in);
> > print_debug(" Agreed on width: ");
> > print_debug_hex8(width);
> > awidth = pci_read_config8(PCI_DEV(0, 0x0, 0), 0x67);
> >
> > print_debug_hex8(awidth);
> >
> > aspeed = pci_read_config8(PCI_DEV(0, 0x0, 0), 0x6d);
> >
> > print_debug_hex8(aspeed);
> > /* Update the desired HT LNK to match AMD NB max from VIA NB is 0x1 */
> > width = (width == 0x01) ? 0x11 : 0x00;
> > if ((aspeed == speed) && (((width == 16) && (awidth == 0x11)) ||
> > 		  ((width == 8) && (awidth == 0x00))))
> > return 0;
> > pci_write_config8(PCI_DEV(0, 0x0, 0), 0x67, width);
> > /* Update the desired HT LNK capabilities in NB too. */
> > pci_write_config8(PCI_DEV(0, 0x0, 0), 0x67,
> > 	  (width == 16) ? 0x11 : 0x00);
> > pci_write_config8(PCI_DEV(0, 0x0, 0), 0x6d, speed);
> > /* Get programmed HT freq at base 0x89 */
> > cldtfreq = pci_read_config8(PCI_DEV(0, 0x18, 0), ldtreg[ldtnr] + 3) & 0xf;
> > print_debug(" CPU programmed to HT freq: ");
> > print_debug_hex8(cldtfreq);
> > print_debug(" and will after HT reset: ");
> > print_debug(" VIA HT caps: ");
> > vldtcaps = pci_read_config16(PCI_DEV(0, 0, 0), 0x6e);
> > print_debug_hex16(vldtcaps);
> > awidth = pci_read_config8(PCI_DEV(0, 0x0, 0), 0x67);
> > print_debug_hex8(awidth);
> > if (!(vldtcaps & (1 << cldtfreq ))) {
> > die("Chipset does not support desired HT frequency\n");
> > }
> > aspeed = pci_read_config8(PCI_DEV(0, 0x0, 0), 0x6d);
> > print_debug_hex8(aspeed);
> >
> > afreq = pci_read_config8(PCI_DEV(0, 0x0, 0), 0x6d);
> >
> > pci_write_config8(PCI_DEV(0, 0x0, 0), 0x6d, cldtfreq);
> > print_debug("\n");
> >
> > /* no reset needed */
> >
> > if ((width == awidth) && (afreq == cldtfreq)) {
> > return 0;
> >
> > }
> >
> > return 1;
> >
> > }
> > Index: src/mainboard/asus/a8v-e_se/cache_as_ram_auto.c
> > ===================================================================
> > --- src/mainboard/asus/a8v-e_se/cache_as_ram_auto.c	(revision 3148)
> > +++ src/mainboard/asus/a8v-e_se/cache_as_ram_auto.c	(working copy)
> > @@ -284,20 +284,19 @@
> >    init_timer();
> >    ht_setup_chains_x(sysinfo); /* Init sblnk and sbbusn, nodes, sbdn. */
> > enable_fid_change();
> > init_fidvid_bsp(bsp_apicid);
> > needs_reset = optimize_link_coherent_ht();
> >  needs_reset |= optimize_link_incoherent_ht(sysinfo);
> > needs_reset |= k8t890_early_setup_HT();
>
> Rename this to k8t890_early_setup_ht(), function names should be
> all-lowercase.
> > /* FIXME: Assumes that 1000MHz LDT is selected. */
> > needs_reset |= k8t890_early_setup_car(16, 0x6);
> > if (needs_reset) {
> >   print_debug("ht reset -\r\n");
> >   soft_reset();
> >  }
> > /* the HT settings needs to be OK, because link freq chnage may cause HT disconnect */
> > enable_fid_change();
> > init_fidvid_bsp(bsp_apicid);
> > /* Stop the APs so we can start them later in init. */
> >  allow_all_aps_stop(bsp_apicid);
>
> Looks good otherwise, but I'll test on hardware later and report back.
> Uwe.
>
> http://www.hermann-uwe.de  | http://www.holsham-traders.de
> http://www.crazy-hacks.org | http://www.unmaintained-free-software.org