Re: [coreboot] [PATCH 4/6] Support for the Intel 945 northbridge

[Stefan Reinauer]

Myles Watson wrote:
> > Support for the Intel 945 northbridge.
>
> Sorry I'm late.  I know it's already in, but I was in the middle of
> reviewing it.  I prefixed my comments with //, I hope that helps you find
> them faster.

Ok, trying to address the issues:
> > Index: src/northbridge/intel/i945/early_init.c
> >
> > --- src/northbridge/intel/i945/early_init.c	(revision 0)
> > +++ src/northbridge/intel/i945/early_init.c	(revision 0)
> >
> > +static int i945_silicon_revision(void)
> > +{
> > return pci_read_config8(PCI_DEV(0, 0x00, 0), 8);
>
> //could use PCI_CLASS_REVISION here.

ok.
> > +}
> >
> > +static void i945_detect_chipset(void)
>
> //I would prefer a name that makes it clear that this function only prints
> information.
> > /* Set C0000-FFFFF to access RAM on both reads and writes */
> > pci_write_config8(PCI_DEV(0, 0x00, 0), PAM0, 0x30);
> > pci_write_config8(PCI_DEV(0, 0x00, 0), PAM1, 0x33);
> > pci_write_config8(PCI_DEV(0, 0x00, 0), PAM2, 0x33);
> > pci_write_config8(PCI_DEV(0, 0x00, 0), PAM3, 0x33);
> > pci_write_config8(PCI_DEV(0, 0x00, 0), PAM4, 0x33);
> > pci_write_config8(PCI_DEV(0, 0x00, 0), PAM5, 0x33);
> > pci_write_config8(PCI_DEV(0, 0x00, 0), PAM6, 0x33);
> > pci_write_config8(PCI_DEV(0, 0x00, 0), TOLUD, 0x40);	/* 1G XXX
>
> dynamic! */
> //Care to elaborate?

I think that's a bug. I'll give it a go and try removing it.
> > printk_debug(" done.\r\n");
> > /* Wait for MCH BAR to come up */
> > printk_debug("Waiting for MCHBAR to come up...");
> > if ((pci_read_config8(PCI_DEV(0, 0x0f, 0), 0xe6) & 0x2) == 0x00) {
>
> /* Bit 49 of CAPID0 */
> > do {
> > 	reg8 = *(volatile u8 *)0xfed40000;
> > } while (!(reg8 & 0x80));
> > }
>
> //Consistency with \r\n ...
> > printk_debug("ok\r\n");
> >
> > +}
> >
> > +static void i945_setup_egress_port(void)
> > +{
> > u32 reg32;
> > u32 timeout;
>
> //and just \n

Which one is the right one to use these days?
> > printk_debug("Wait for VC1 negotiation ...");
> > /* Wait for VC1 negotiation pending */
> > timeout = 0x7ffff;
> > while ((DMIBAR16(DMIVC1RSTS) & (1 << 1)) && --timeout) ;
> > if (!timeout)
> > printk_debug("timeout!\n");
> > else
> > printk_debug("done..\n");
>
> //Do we need the #if 1 ?

Good question. I am not sure when ASPM should be enabled and when it
should not. At some point this should be a better #if or if.
> > +#if 1
> > /* Enable Active State Power Management (ASPM) L0 state */
> > reg32 = DMIBAR32(DMILCAP);
> > reg32 &= ~(7 << 12);
> > reg32 |= (2 << 12);
> > reg32 &= ~(7 << 15);
> > reg32 |= (2 << 15);
> > DMIBAR32(DMILCAP) = reg32;
> > reg32 = DMIBAR32(DMICC);
> > reg32 &= 0x00ffffff;
> > reg32 &= ~(3 << 0);
> > reg32 |= (1 << 0);
> > DMIBAR32(DMICC) = reg32;
> > if (0) {
> > DMIBAR32(DMILCTL) |= (3 << 0);
> > }
> >
> > +#endif
> > /* Last but not least, some additional steps */
> > reg32 = MCHBAR32(FSBSNPCTL);
> > reg32 &= ~(0xff << 2);
> > reg32 |= (0xaa << 2);
> > MCHBAR32(FSBSNPCTL) = reg32;
> > DMIBAR32(0x2c) = 0x86000040;
> > reg32 = DMIBAR32(0x204);
> > reg32 &= ~0x3ff;
>
> //should be #if DMIX == 4?
> > +#if 1
> > reg32 |= 0x13f;		/* for x4 DMI only */
> >
> > +#else
> > reg32 |= 0x1e4; /* for x2 DMI only */
> >
> > +#endif
> > DMIBAR32(0x204) = reg32;

Not sure. And set this in auto.c?
> // I think this should be named disabled, since it doesn't disable it.
> > +static int sdram_capabilities_MEM4G_disable(void)
> > +{
> > u8 reg8;
> > reg8 = pci_read_config8(PCI_DEV(0, 0x00, 0), 0xe5);
> > reg8 &= (1 << 0);
> > return (reg8 != 0);
> >
> > +}

agreed
> > +static void sdram_detect_smallest_refresh(struct sys_info * sysinfo)
> > +{
> > int i;
> > sysinfo->refresh = 0;
> > for (i=0; i<2*DIMM_SOCKETS; i++) {
> > int refresh;
> > if (sysinfo->dimm[i] == SYSINFO_DIMM_NOT_POPULATED)
> > 	continue;
> > refresh = spd_read_byte(DIMM_SPD_BASE + i, SPD_REFRESH) &
>
> ~(1 << 7);
> > /* 15.6us */
> > if (!refresh)
> > 	continue;
> > /* Refresh is slower than 15.6us, use 15.6us */
> > if (refresh > 2)
> > 	continue;
> > if (refresh == 2) {
> > 	sysinfo->refresh = 1;
> > 	break;
> > }
>
> // It looks like there is only one unsupported value (1) could say that here
> //	If that value means 3.9 us:
> //		die("DDR-II module has unsupported refresh value (3.9
> us)\n");
> > die("DDR-II module has unsupported refresh value\n");

I never got any of those 3.9us modules, so I can't tell for sure that
assumption is true. Even if it is, I doubt knowing it's 3.9 is of much
value.
> > +static void sdram_program_memory_frequency(struct sys_info *sysinfo)
> > +{
> > u32 clkcfg;
> > u8 reg8;
> > printk_debug ("Setting Memory Frequency... ");
> > clkcfg = MCHBAR32(CLKCFG);
> > printk_debug("CLKCFG=0x%08x, ", clkcfg);
> > clkcfg &= ~( (1 << 12) | (1 << 7) | ( 7 << 4) );
> > if (sysinfo->mvco4x) {
> > printk_debug("MVCO 4x, ");
>
> // You just did this - why twice
> > clkcfg &= ~(1 << 12);
> > }

Good catch. Let me check.
> > printk_debug("RAM initialization finished.\r\n");
> > sdram_setup_processor_side();
> >
> > +}
>
> //Since this is CPU specific, should it go with the CPU files?

While the name is misleading, it really sets up stuff in the chipset,
not in the CPU. It's not CPU specific, it just sets up parts of the
northbridge that cope with the CPU.
> > printk_debug("%dM UMA\n", uma_size >> 10);
> > tomk -= uma_size;
> > }
> > /* The following needs to be 2 lines, otherwise the second
> > * number is always 0
> > */
>
> // Did you try since tomk is unsigned long?
> //	printk_info("Available memory: %ldK (%ldM)\n", tomk, (tomk >> 10));

I tried lots of stuff, but I am not sure I tried that. Will retry.
> > printk_info("Available memory: %dK", tomk);
> > printk_info(" (%dM)\n", (tomk >> 10));
> > tolmk = tomk;
> > /* Report the memory regions */
> > ram_resource(dev, 3, 0, 640);
> > ram_resource(dev, 4, 768, (tolmk - 768));
> > if (tomk > 4 * 1024 * 1024) {
> > ram_resource(dev, 5, 4096 * 1024, tomk - 4 * 1024 * 1024);
> > }
> > assign_resources(&dev->link[0]);
> >
> > +}

Stefan