Re: [coreboot] patch for support mainboard pistachio:

[Li, Maggie]

Acked-by: Maggie Li

Best regards
Maggie li

-----Original Message-----
From: Carl-Daniel Hailfinger [mailto:[e-mail]] 
Sent: Friday, December 12, 2008 10:50 AM
To: Li, Maggie
Cc: Bao, Zheng; [e-mail]; ron minnich; Xie, Michael; Huang, FrankR; Feng, Libo; Wang, Qingpei
Subject: Re: [coreboot] patch for support mainboard pistachio:

Hi Maggie,

thanks for the new review. I have changed the patch according to your
suggestions.

Do you think the new patch is OK?

Regards,
Carl-Daniel

On 11.12.2008 09:44, Li, Maggie wrote:
> Hi, Carl-Daniel
>
> I have some suggestion below for you.
>
> Best regards
> Maggie li
>
> -----Original Message-----
> From: Carl-Daniel Hailfinger [mailto:[e-mail]] 
> Sent: Thursday, December 11, 2008 10:13 AM
> To: Li, Maggie
> Cc: ron minnich; Bao, Zheng; [e-mail]; Xie, Michael; Huang, FrankR; Feng, Libo; Wang, Qingpei
> Subject: Re: [coreboot] patch for support mainboard pistachio:
>
> Hi Maggie,
>
> thank you for the fast review!
> I have changed the things you suggested in the hope this will be better.
> Please see below.
>
> Best regards,
> Carl-Daniel Hailfinger
>
> On 10.12.2008 07:56, Li, Maggie wrote:
> > Hi, Carl-Daniel
> >
> > I have some suggestion about your modification. Please see below.
> >
> > Best regards
> > Maggie li
> >
> > -----Original Message-----
> > From: Carl-Daniel Hailfinger [mailto:[e-mail]] 
> > Sent: Wednesday, December 10, 2008 10:19 AM
> > To: ron minnich
> > Cc: Li, Maggie; Bao, Zheng; [e-mail]; Xie, Michael; Huang, FrankR; Feng, Libo; Wang, Qingpei
> > Subject: Re: [coreboot] patch for support mainboard pistachio:
> >
> > Dear Maggie,
> >
> > can you please look at the attached patch and review it? I have tried to
> > improve comments and warn about possible problems in early SB600 setup.
> >
> > Thank you!
> >
> > Signed-off-by: Carl-Daniel Hailfinger
> >
> > The patch is also inlined into this mail for better viewing.
> >
> > Regards,
> > Carl-Daniel Hailfinger
> > Index: LinuxBIOSv2-asus_m2a-vm/src/southbridge/amd/sb600/sb600_early_setup.c
> >
> > --- LinuxBIOSv2-asus_m2a-vm/src/southbridge/amd/sb600/sb600_early_setup.c	(Revision 3808)
> > +++ LinuxBIOSv2-asus_m2a-vm/src/southbridge/amd/sb600/sb600_early_setup.c	(Arbeitskopie)
> > @@ -53,11 +53,15 @@
> >
> > /***************************************
> > Legacy devices are mapped to LPC space.
> >
> > -*	serial port 0
> > +*	Serial port 0
> > KBC Port
> > ACPI Micro-controller port
> >
> > -*	LPC ROM size,
> > +*	LPC ROM size
> > +*	This function does not change port 0x80 decoding.
> > +*	Console output through any port besides 0x3f8 is unsupported.
> > NOTE: Call me ASAP, because I will reset LPC ROM size!
> >
> > +* Reviewed-by: Carl-Daniel Hailfinger
> > +* Reviewed against AMD SB600 Register Reference Manual 3.03, section 3.1 (LPC ISA Bridge)
> >  ***************************************/
> >  static void sb600_lpc_init(void)
> >  {
> > @@ -68,31 +72,38 @@
> >    /* Enable lpc controller */
> >    dev = pci_locate_device(PCI_ID(0x1002, 0x4385), 0);	/* SMBUS controller */
> >    reg32 = pci_read_config32(dev, 0x64);
> > reg32 |= 0x00100000;
> >
> > reg32 |= 1 << 20;
> > pci_write_config32(dev, 0x64, reg32);
> >
> > dev = pci_locate_device(PCI_ID(0x1002, 0x438d), 0);	/* LPC Controller */
> > /* Serial 0 */
> > /* Decode port 0x3f8-0x3ff (Serial 0) */
> >
> > +#warning Serial port decode on LPC is hardcoded to 0x3f8
> >    reg8 = pci_read_config8(dev, 0x44);
> > reg8 |= (1 << 6);
> > reg8 |= 1 << 6;
> > pci_write_config8(dev, 0x44, reg8);
> > /* PS/2 keyboard, ACPI */
> > /* Decode port 0x60-0x67 (PS/2 keyboard, ACPI) */
> >
> > [Li, Maggie] Decode port 60 & 64h (PS/2 keyboard), and port 62 & 66h (ACPI)
>
> Thanks, I changed this.
> > reg8 = pci_read_config8(dev, 0x47);
> >    reg8 |= (1 << 5) | (1 << 6);
> >    pci_write_config8(dev, 0x47, reg8);
> >
> > /* SuperIO, LPC ROM */
> >    reg8 = pci_read_config8(dev, 0x48);
> > reg8 |= (1 << 1) | (1 << 0);	/* enable Super IO config port 2e-2h, 4e-4f */
> > reg8 |= (1 << 3) | (1 << 4);	/* enable for LPC ROM address range1&2, Enable 512KB rom access at 0xFFF80000 - 0xFFFFFFFF  */
> > reg8 |= 1 << 6;		/* enable for RTC I/O range */
> >
> > /* Decode ports 0x2e-0x2f, 0x4e-0x4f (SuperI/O configuration) */
> >
> > reg8 |= (1 << 1) | (1 << 0);
> >
> > /* Decode variable LPC ROM address ranges 1&2 (see register 0x68-0x6b, 0x6c-0x6f) */
> >
> > reg8 |= (1 << 3) | (1 << 4);
> >
> > /* Decode port 0x70-0x73 (RTC) */
> >
> > reg8 |= 1 << 6;
> > pci_write_config8(dev, 0x48, reg8);
> >
> > /* hardware should enable LPC ROM by pin strapes */
> > /*  rom access at 0xFFF80000/0xFFF00000 - 0xFFFFFFFF */
> > /* ROM access at 0xFFF80000/0xFFF00000 - 0xFFFFFFFF */
> > /* See detail in BDG-215SB600-03.pdf page 15. */
> > pci_write_config16(dev, 0x68, 0x000e);	/* enable LPC ROM range, 0xfff8: 512KB, 0xfff0: 1MB; */
> >
> > +#warning If Lpc_Rom strap is enabled, the line below has no effect, and if Lpc_Rom strap is disabled, a write to 0x6a is missing
> > pci_write_config16(dev, 0x68, 0x000e);	/* enable LPC ROM range mirroring below 1 MB */
> >
> > +#warning Due to FWH_*_IDSEL defaults, any FWH ROM chip larger than 512 kB will not work although we enable a range of 1 MB below. LPC ROMs are not affected.
> > +#warning If Lpc_Rom strap is disabled, a write to 0x6e is missing
> >    pci_write_config16(dev, 0x6c, 0xfff0);	/* enable LPC ROM range, 0xfff8: 512KB, 0xfff0: 1MB  */
> > [Li, Maggie] I think the warning about 0x68 and 0x6c is somewhat confusing. If Lpc_Rom strap is disabled, default values for register 0x6a and 0x6e are correct. Do we have to write them? Should the register 0x68, 0x6c be for setting LPC ROM, and register 0x70 for FWH ROM? Should we write some code for setting FWH ROM here?
>
> Yes, the warning was confusing. My apologies.
>
> Will the settings in register 0x68 and 0x6c also affect FWH and SPI? If
> yes, we can simply set them unconditionally.
>
> I have prepared a new patch which assumes 0x68 and 0x6c affect FWH and
> SPI. Setting these registers again (if LPC strap was active) is not a
> problem and it makes the code simpler.
>
> [Li, Maggie] I did a test on my pistachio board to find that 0x68 and 0x6c do affect SPI. I have not a FWH board, so I am not sure whether FWH will be affected.

Thanks for testing! There are only very few RS690 boards for Intel out
there and I think they are no longer in production, so we can ignore
them for now.
> > }
> >
> > @@ -201,26 +212,33 @@
> >    /* P2P Bridge */
> >    dev = pci_locate_device(PCI_ID(0x1002, 0x4384), 0);
> >
> > /* Chip Control: Enable subtractive decoding */
> > byte = pci_read_config8(dev, 0x40);
> > byte |= 1 << 5;
> > pci_write_config8(dev, 0x40, byte);
> >
> > /* Misc Control: Enable subtractive decoding if 0x40 bit 5 is set */
> > byte = pci_read_config8(dev, 0x4B);
> > byte |= 1 << 7;
> > pci_write_config8(dev, 0x4B, byte);
> >
> > /* FIXME: This does not make sense if we want to decode port 0x80 */
>
> I am not sure about this comment. If IO Base and IO Limit are 0xf000,
> nothing will be passed through. Am I wrong?
> [Li, Maggie] The same IO Base and IO Limit here is meaningful when we set the bridge to be subtractive. At early setup stage, we have to make sure that data can go through port 0x80. After resource is allocated, the bridge has its own positive and subtractive window. Any data going to the devices behind the bridge can go through positive window. Data to port 0x80 will go through subtractive window if no other bridge accepts it after four cycles.
>
> I think it is better to remove the comment "/* FIXME: This does not make sense if we want to decode port 0x80 */" now.

Yes, thanks! I added your explanation as comment to the code.
If you agree to the new patch, it would be great if you could give an
Acked-by: statement.

Improve comments in early SB600 setup, handle non-LPC strapping and
document verification against the data sheets.

Signed-off-by: Carl-Daniel Hailfinger

Index: LinuxBIOSv2-asus_m2a-vm/src/southbridge/amd/sb600/sb600_early_setup.c
===================================================================
--- LinuxBIOSv2-asus_m2a-vm/src/southbridge/amd/sb600/sb600_early_setup.c	(revision 3808)
+++ LinuxBIOSv2-asus_m2a-vm/src/southbridge/amd/sb600/sb600_early_setup.c	(working copy)
@@ -53,11 +53,17 @@

/***************************************
 * Legacy devices are mapped to LPC space.
-*	serial port 0
+*	Serial port 0
 *	KBC Port
 *	ACPI Micro-controller port
-*	LPC ROM size,
+*	LPC ROM size
+*	This function does not change port 0x80 decoding.
+*	Console output through any port besides 0x3f8 is unsupported.
+*	If you use FWH ROMs, you have to setup IDSEL.
 * NOTE: Call me ASAP, because I will reset LPC ROM size!
+* Reviewed-by: Carl-Daniel Hailfinger
+* Reviewed against AMD SB600 Register Reference Manual rev. 3.03, section 3.1
+* 	(LPC ISA Bridge)
 ***************************************/
 static void sb600_lpc_init(void)
 {
@@ -68,32 +74,42 @@
    /* Enable lpc controller */
    dev = pci_locate_device(PCI_ID(0x1002, 0x4385), 0);	/* SMBUS controller */
    reg32 = pci_read_config32(dev, 0x64);
-	reg32 |= 0x00100000;
+	reg32 |= 1 << 20;
    pci_write_config32(dev, 0x64, reg32);

dev = pci_locate_device(PCI_ID(0x1002, 0x438d), 0);	/* LPC Controller */
-	/* Serial 0 */
+	/* Decode port 0x3f8-0x3ff (Serial 0) */
+#warning Serial port decode on LPC is hardcoded to 0x3f8
    reg8 = pci_read_config8(dev, 0x44);
-	reg8 |= (1 << 6);
+	reg8 |= 1 << 6;
    pci_write_config8(dev, 0x44, reg8);

-	/* PS/2 keyboard, ACPI */
+	/* Decode port 0x60 & 0x64 (PS/2 keyboard) and port 0x62 & 0x66 (ACPI)*/
    reg8 = pci_read_config8(dev, 0x47);
    reg8 |= (1 << 5) | (1 << 6);
    pci_write_config8(dev, 0x47, reg8);

/* SuperIO, LPC ROM */
    reg8 = pci_read_config8(dev, 0x48);
-	reg8 |= (1 << 1) | (1 << 0);	/* enable Super IO config port 2e-2h, 4e-4f */
-	reg8 |= (1 << 3) | (1 << 4);	/* enable for LPC ROM address range1&2, Enable 512KB rom access at 0xFFF80000 - 0xFFFFFFFF  */
-	reg8 |= 1 << 6;		/* enable for RTC I/O range */
+	/* Decode ports 0x2e-0x2f, 0x4e-0x4f (SuperI/O configuration) */
+	reg8 |= (1 << 1) | (1 << 0);
+	/* Decode variable LPC ROM address ranges 1&2 (see register 0x68-0x6b, 0x6c-0x6f) */
+	reg8 |= (1 << 3) | (1 << 4);
+	/* Decode port 0x70-0x73 (RTC) */
+	reg8 |= 1 << 6;
    pci_write_config8(dev, 0x48, reg8);

-	/* hardware should enable LPC ROM by pin strapes */
-	/*  rom access at 0xFFF80000/0xFFF00000 - 0xFFFFFFFF */
+	/* hardware should enable LPC ROM by pin straps */
+	/* ROM access at 0xFFF80000/0xFFF00000 - 0xFFFFFFFF */
    /* See detail in BDG-215SB600-03.pdf page 15. */
-	pci_write_config16(dev, 0x68, 0x000e);	/* enable LPC ROM range, 0xfff8: 512KB, 0xfff0: 1MB; */
-	pci_write_config16(dev, 0x6c, 0xfff0);	/* enable LPC ROM range, 0xfff8: 512KB, 0xfff0: 1MB  */
+	/* enable LPC ROM range mirroring start 0x000e(0000) */
+	pci_write_config16(dev, 0x68, 0x000e);
+	/* enable LPC ROM range mirroring end   0x000f(ffff) */
+	pci_write_config16(dev, 0x6a, 0x000f);
+	/* enable LPC ROM range start, 0xfff8(0000): 512KB, 0xfff0(0000): 1MB */
+	pci_write_config16(dev, 0x6c, 0xfff0);
+	/* enable LPC ROM range end at 0xffff(ffff) */
+	pci_write_config16(dev, 0x6e, 0xffff);
 }

/* what is its usage? */
@@ -201,26 +217,36 @@
    /* P2P Bridge */
    dev = pci_locate_device(PCI_ID(0x1002, 0x4384), 0);

+	/* Chip Control: Enable subtractive decoding */
    byte = pci_read_config8(dev, 0x40);
    byte |= 1 << 5;
    pci_write_config8(dev, 0x40, byte);

+	/* Misc Control: Enable subtractive decoding if 0x40 bit 5 is set */
    byte = pci_read_config8(dev, 0x4B);
    byte |= 1 << 7;
    pci_write_config8(dev, 0x4B, byte);

+	/* The same IO Base and IO Limit here is meaningful because we set the
+	 * bridge to be subtractive. During early setup stage, we have to make
+	 * sure that data can go through port 0x80.
+	 */
+	/* IO Base: 0xf000 */
    byte = pci_read_config8(dev, 0x1C);
    byte |= 0xF << 4;
    pci_write_config8(dev, 0x1C, byte);

+	/* IO Limit: 0xf000 */
    byte = pci_read_config8(dev, 0x1D);
    byte |= 0xF << 4;
    pci_write_config8(dev, 0x1D, byte);

+	/* PCI Command: Enable IO response */
    byte = pci_read_config8(dev, 0x04);
    byte |= 1 << 0;
    pci_write_config8(dev, 0x04, byte);

+	/* LPC controller */
    dev = pci_locate_device(PCI_ID(0x1002, 0x438D), 0);

byte = pci_read_config8(dev, 0x4A);
@@ -234,13 +260,13 @@
    device_t dev;
    u32 reg32;

-	/* enable lpc controller */
+	/* Enable LPC controller */
    dev = pci_locate_device(PCI_ID(0x1002, 0x4385), 0);
    reg32 = pci_read_config32(dev, 0x64);
    reg32 |= 0x00100000;	/* lpcEnable */
    pci_write_config32(dev, 0x64, reg32);

-	/* enable prot80 LPC decode in pci function 3 configuration space. */
+	/* Enable port 80 LPC decode in pci function 3 configuration space. */
    dev = pci_locate_device(PCI_ID(0x1002, 0x438d), 0);
    byte = pci_read_config8(dev, 0x4a);
    byte |= 1 << 5;		/* enable port 80 */