[LinuxBIOS] r369 - in LinuxBIOSv3/southbridge: amd/cs5536 intel/i82371eb

Marc Jones Marc.Jones at AMD.com
Thu Jun 28 01:10:43 CEST 2007



Peter Stuge wrote:

>On Wed, Jun 27, 2007 at 09:14:27PM +0200, svn at openbios.org wrote:
>  
>
>>+	/*      PM_WKXD */
>>+	/*      Make sure bits[3:0]=0000b to clear the */
>>+	/*      saved Sx state */
>>+	port = (PMS_IO_BASE + 0x034);
>>+	val = 0x0A0;		/*  5ms */
>>+	outl(val, port);
>>    
>>
>
>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.
>
>
>  
>
yes

>>+#define RTC_CENTURY 0x32
>>+#define RTC_DOMA	0x3D
>>+#define RTC_MONA	0x3E
>>    
>>
>
>Perhaps these should go into some .h?
>
>  
>

Yes, I think that this could go into the 5536.h. Note that this is 
defining cmos tokens so I guess that the geode cmos map should be 
updated to match.

>  
>
>>+	/* COM1 */
>>+	if (sb->com1_enable) {
>>+		/* Set the address */
>>+		switch (sb->com1_address) {
>>+		case 0x3F8:
>>+		case 0x3E8:
>>+		case 0x2F8:
>>+		case 0x2E8:
>>    
>>
>
>Maybe there should be a default: here to catch problems in the dts?
>
>
>
>
>  
>
I might not be clear on what you mean by default but the dts does have a 
default value.

>
>  
>
>>+/** 
>>+ * Depending on settings in the config struct, enable COM1 or COM2 or both. 
>>    
>>
>
>  
>
>>+ * If the enable is NOT set, the UARTS are explicitly disabled, which is required 
>>+ * if (e.g.) there is a superio attached that does COM1 or COM2.
>>+  * @param southbridge config structure 
>>+  */
>>+static void enable_USB_port4(struct southbridge_amd_cs5536_config *sb)
>>    
>>
>
>The comment is obviously wrong for USB. :)
>
>
>  
>
>>+		/* ; EECP=50h, IST=01h, ASPC=1 */
>>+		*(bar + HCCPARAMS) = 0x00005012;
>>    
>>
>
>Don't know if these exist as defines, but they should.
>
>
>  
>
yes.

>>+	/* Disable virtual PCI UDC and OTG headers */
>>+	dev = dev_find_device(PCI_VENDOR_ID_AMD, 
>>+			PCI_DEVICE_ID_AMD_CS5536_UDC, 0);
>>+	if (dev) {
>>+		pci_write_config32(dev, 0x7C, 0xDEADBEEF);
>>+	}
>>+
>>+	dev = dev_find_device(PCI_VENDOR_ID_AMD, 
>>+			PCI_DEVICE_ID_AMD_CS5536_OTG, 0);
>>+	if (dev) {
>>+		pci_write_config32(dev, 0x7C, 0xDEADBEEF);
>>+	}
>>    
>>
>
>#define 0x7C ?
>
>
>  
>
yes.

>>+	post_code(P80_CHIPSET_INIT);
>>    
>>
>
>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.
>
>
>  
>
Ron, Stephan, Jordan, and I discussed this today. Nothing was resolved 
but we recognized that we need a consitant scheme.

>>+	/* we hope NEVER to be in linuxbios when S3 resumes
>>+	   if (! IsS3Resume()) */
>>    
>>
>
>This comment is still bad. Marc and/or Jordan and I talked about it
>on IRC a while ago and it's not as bad as it sounds but the comment
>should really change. I don't remember the details anymore. :\
>
>
>  
>
I would like this line removed. There are a number of items that need to 
be done before S3 resume can be tackled in LB.


>>+	/*      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. */
>>    
>>
>
>long lines..
>
>
>  
>
>>+	msrnum = ATA_SB_GLD_MSR_ERR;
>>+	msr = rdmsr(msrnum);
>>+	msr.lo &= ~0x100;
>>+	wrmsr(msrnum, msr);
>>    
>>
>
>#define 0x100 ?
>
>
>  
>
yes

>>+#warning Add back in unwanted VPCI support
>>+#if 0
>>+	/* disable unwanted virtual PCI devices */
>>+	for (i = 0; (i < MAX_UNWANTED_VPCI) && (0 != sb->unwanted_vpci[i]); i++) {
>>+		printk(BIOS_DEBUG, "Disabling VPCI device: 0x%08X\n",
>>+			     sb->unwanted_vpci[i]);
>>+		outl(sb->unwanted_vpci[i] + 0x7C, 0xCF8);
>>    
>>
>
>Here's 0x7c again. Same value? #define ?
>
>
>  
>
>>+		outl(0xDEADBEEF, 0xCFC);
>>+	}
>>+#endif
>>    
>>
>
>Should all of this code just be deleted?
>
>
>Other than that this is becoming pretty beautiful code!! :)
>
>
>//Peter
>
>  
>


So, I will make a patch of the defines tonight once Ron has his other 
changes in.
Marc







More information about the coreboot mailing list