[LinuxBIOS] [Marc.Jones at AMD.com: Re: r369 - in LinuxBIOSv3/southbridge: amd/cs5536 intel/i82371eb]

[Uwe Hermann]

[I think this was only sent to me, not the list, so I'm forwarding...]

----- Forwarded message from Marc Jones <Marc.Jones at AMD.com> -----

From: Marc Jones <Marc.Jones at AMD.com>
To: Uwe Hermann <uwe at hermann-uwe.de>
Subject: Re: [LinuxBIOS] r369 - in LinuxBIOSv3/southbridge: amd/cs5536
 intel/i82371eb
Date: Wed, 27 Jun 2007 23:21:22 -0600

Just a couple of comments.

Uwe Hermann wrote:

>On Wed, Jun 27, 2007 at 03:59:18PM -0700, ron minnich wrote:
> 
>
>>>This construct is all over the file. There is a #define PM_WKXD 0x034
>>>so please change the code to use it and remove the comment.
>>>     
>>>
>>I like the comment above the code block. I did change the constant to the 
>>name.
>>   
>>
>
>Yep, that's ok, IMHO. I wouldn't drop the comment.
>
>
> 
>
>>>>+#define RTC_CENTURY 0x32
>>>>+#define RTC_DOMA     0x3D
>>>>+#define RTC_MONA     0x3E
>>>>       
>>>>
>>>Perhaps these should go into some .h?
>>>     
>>>
>>but where?
>>   
>>
>
>Good question. Are these Geode-specific values or generic RTC values?
>
> 
>
They are not generic(legacy) and not Geode specific. How's that for an 
answer ;)
These are fairly common extensions to modern the RTC.

> 
>
>>>I would love to see the POST code mess unified before there are ten
>>>boards in v3 all using their own slightly different code and define
>>>style.
>>>     
>>>
>>we're trying. But marc did such a great job with post that I don't
>>want to lose it.
>>   
>>
>
>Sure, but for consistency I propose two rules for POST codes:
>
>- All POST codes are emitted _only_ by post_code(), no other functions,
>  macros, outb's, or whatever.
>
>- All POST codes are #defines in post_code.h, every #define is called
>  POST_FOO (replace FOO with something which makes sense).
>
>I.e., the only allowed form is post_code(POST_FOO). In addition we'll
>have think more about which numbers we use in which situations, but
>as a first step I think the above rules will improve the situation a lot.
>
>
> 
>
>>This LX port is going to go in several stages. For now, we want code
>>in the repo that builds. My laptop disk is dying and I want it in the
>>repo and off my disk .... there's a lot of urgency.
>>   
>>
>
>Send it as patch to the list and it's archived for eternity :)
>
>
> 
>
>>Index: southbridge/amd/cs5536/cs5536.c
>>===================================================================
>>--- southbridge/amd/cs5536/cs5536.c	(revision 390)
>>+++ southbridge/amd/cs5536/cs5536.c	(working copy)
>>@@ -117,21 +117,21 @@
>>	/*      PM_WKXD */
>>	/*      Make sure bits[3:0]=0000b to clear the */
>>	/*      saved Sx state */
>>-	port = (PMS_IO_BASE + 0x034);
>>+	port = (PMS_IO_BASE + PM_WKXD);
>>	val = 0x0A0;		/*  5ms */
>>	outl(val, port);
>>
>>	/*      PM_WKD */
>>-	port = (PMS_IO_BASE + 0x030);
>>+	port = (PMS_IO_BASE + PM_WKD);
>>	outl(val, port);
>>
>>	/*      PM_SED */
>>-	port = (PMS_IO_BASE + 0x014);
>>+	port = (PMS_IO_BASE + PM_SED);
>>	val = 0x04601;		/*  5ms, # of 3.57954MHz clock edges */
>>	outl(val, port);
>>
>>	/*      PM_SIDD */
>>-	port = (PMS_IO_BASE + 0x020);
>>+	port = (PMS_IO_BASE + PM_SIDD);
>>	val = 0x08C02;		/*  10ms, # of 3.57954MHz clock edges */
>>	outl(val, port);
>>}
>>@@ -519,7 +519,7 @@
>>	post_code(P80_CHIPSET_INIT);
>>	dev = dev_find_device(PCI_VENDOR_ID_AMD, 
>>	PCI_DEVICE_ID_AMD_CS5536_ISA, 0);
>>	if (! dev) {
>>-		printk(BIOS_ERR, "%s: Could not find the south bridge!\n", 
>>__FUNCTION));
>>+		printk(BIOS_ERR, "%s: Could not find the south bridge!\n", 
>>__FUNCTION__);
>>		return;
>>	}
>>	sb = (struct southbridge_amd_cs5536_config 
>>	*)dev->device_configuration;
>>@@ -540,8 +540,10 @@
>>	outl(GPIOL_2_SET, GPIO_IO_BASE + GPIOL_INPUT_ENABLE);
>>	outl(GPIOL_2_SET, GPIO_IO_BASE + GPIOL_IN_AUX1_SELECT);
>>
>>-	/*      Allow IO read and writes during a ATA DMA operation. */
>>-	/*       This could be done in the HD rom but do it here for easier 
>>debugging. */
>>   
>>
>
>Can someone explain this, I'm slightly confused. "Can be done in HD
>rom"? Huh?
>
> 
>
If there were a 5536 IDE ROM it could do the setup. Since there isn't I 
would remove the comment.

> 
>
>>+	/* Allow IO read and writes during a ATA DMA operation.
>>+	  * This could be done in the HD rom but 
>>+	  * do it here for easier debugging.
>>+	  */
>>   
>>
>
>The last three lines of the comment are indented with one space too
>much, but other than that this patch is fine:
>
>Acked-by: Uwe Hermann <uwe at hermann-uwe.de>
>
>
>Uwe.
> 
>

----- End forwarded message -----

Uwe.
-- 
http://www.hermann-uwe.de  | http://www.holsham-traders.de
http://www.crazy-hacks.org | http://www.unmaintained-free-software.org
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://www.coreboot.org/pipermail/coreboot/attachments/20070628/0e44eb18/attachment.sig>