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

[Marc Jones]

Stefan Reinauer wrote:
> 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

I will try to address as many as I can this week.
Thanks to Stefan, Uwe, Peter, and everyone else for the review.
Marc
> Peter Stuge  [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