[LinuxBIOS] [patch 09/10] AMD Geode GX/LX: CS5536 Southbridge

[Stefan Reinauer]

Yes, I love top posting ;)

I agree to these comments and I think we should get this fixed. I am
going to check the patch in as is, but please don't forget to send a fix
later if possible.

Stefan

* Peter Stuge <stuge-linuxbios at cdy.org> [070504 03:50]:
> On Thu, May 03, 2007 at 12:15:57PM -0600, Marc Jones wrote:
> > Index: LinuxBIOSv2/src/include/device/pci_ids.h
> > ===================================================================
> > --- LinuxBIOSv2.orig/src/include/device/pci_ids.h	2007-05-02 15:35:45.000000000 -0600
> > +++ LinuxBIOSv2/src/include/device/pci_ids.h	2007-05-02 15:36:07.000000000 -0600
> > @@ -452,12 +452,13 @@
> >  #define PCI_DEVICE_ID_AMD_AES		0x2082
> >  #define PCI_DEVICE_ID_AMD_CS5536_ISA	0x2090
> >  #define PCI_DEVICE_ID_AMD_CS5536_FLASH	0x2091
> > -#define PCI_DEVICE_ID_AMD_CS5536_IDE	0x2092
> > +#define PCI_DEVICE_ID_AMD_CS5536_IDE_A0	0x2092
> >  #define PCI_DEVICE_ID_AMD_CS5536_AUDIO	0x2093
> >  #define PCI_DEVICE_ID_AMD_CS5536_OHCI	0x2094
> >  #define PCI_DEVICE_ID_AMD_CS5536_EHCI	0x2095
> >  #define PCI_DEVICE_ID_AMD_CS5536_UDC	0x2096
> >  #define PCI_DEVICE_ID_AMD_CS5536_OTG	0x2097
> > +#define PCI_DEVICE_ID_AMD_CS5536_IDE	0x209A
> 
> I would like this to be more future proof, e.g. with _CS5536_A0_IDE
> and _CS5536_B3_IDE. (assuming B3 is the first rev with the new ID)
> 
> Otherwise, the next time the PCI ID is bumped, a new build of old
> working code will break at runtime. That's unneccessary. Better it
> breaks at compile time or not at all..
> 
> 
> > +static void pmChipsetInit(void) {
> ..
> 
> > +	/*	PM_SED*/
> > +	port =	(PMS_IO_BASE + 0x014);
> > +/*	mov		eax, 0x057642	; 100ms, works*/
> > +	val =  0x04601		; /*  5ms*/
> > +	outl(val, port);
> 
> An assembly comment lost in C code. Let's help it find it's way back
> home. :)
> 
> These comments are a bit confusing, maybe just because I don't have
> the data book at hand?
> 
> "100ms, works" but let's use 5ms instead?
> 
> It would be nice to have a better description of the reference
> values here.
> 
> 
> > +	outb( P80_CHIPSET_INIT, 0x80);
> 
> What was the resolution of the POST code output discussion?
> 
> I would prefer if post_code() was used throughout so smart things
> could be added to that function later.
> 
> 
> > +	/* we hope NEVER to be in linuxbios when S3 resumes
> > +	if (! IsS3Resume()) */
> 
> "hope" ? At the very least expand on the problem in the comment.
> 
> 
> > Index: LinuxBIOSv2/src/southbridge/amd/cs5536/cs5536_early_setup.c
> > ===================================================================
> > --- LinuxBIOSv2.orig/src/southbridge/amd/cs5536/cs5536_early_setup.c	2007-05-02 15:35:45.000000000 -0600
> > +++ LinuxBIOSv2/src/southbridge/amd/cs5536/cs5536_early_setup.c	2007-05-02 15:36:07.000000000 -0600
> 
> [..]
> 
> > -static void cs5536_setup_power_bottun(void)
> > +static void cs5536_setup_power_button(void)
> 
> [..]
> 
> > -	; setup GPIO24, it is the external signal for 5536 vsb_work_aux
> > +	/* setup GPIO24, it is the external signal for 5536 vsb_work_aux
> >  	; which controls all voltage rails except Vstandby & Vmem.
> 
> Could this be any GPIO ball or is GPIO24 muxed with another function
> and GPIO24 just serves as a reference here?
> 
> If any GPIO - it would be nice to make this an option.
> 
> If muxed, is there a more relevant signal name that could be used
> instead of GPIO24?
> 
> 
> //Peter
> 
> -- 
> linuxbios mailing list
> linuxbios at linuxbios.org
> http://www.linuxbios.org/mailman/listinfo/linuxbios
> 

-- 
coresystems GmbH • Brahmsstr. 16 • D-79104 Freiburg i. Br.
      Tel.: +49 761 7668825 • Fax: +49 761 7664613
Email: info at coresystems.de  • http://www.coresystems.de/
Registergericht: Amtsgericht Freiburg • HRB 7656
Geschäftsführer: Stefan Reinauer • Ust-IdNr.: DE245674866