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

[Peter Stuge]

On Wed, Jun 27, 2007 at 09:14:27PM +0200, [e-mail] 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.
> +#define RTC_CENTURY 0x32
> +#define RTC_DOMA	0x3D
> +#define RTC_MONA	0x3E

Perhaps these should go into some .h?
> /* 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?
> +/**
> 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.
> /* 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 ?
> 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.
> /* 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. :\
> /*      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 ?
> +#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

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

[ron minnich]

Thanks again for the comments, I will try to get them in.

Please bear in mind that a lot of the code that we are finding issues
with is almost straight from
V2 and went through this process already, so it's surprising to me how
many problems we're seeing :-)

ron

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

[Peter Stuge]

On Wed, Jun 27, 2007 at 03:30:41PM -0700, ron minnich wrote:
> Please bear in mind that a lot of the code that we are finding
> issues with is almost straight from V2

Yup.
> and went through this process already,

Not really.
> so it's surprising to me how many problems we're seeing :-)

I'm not at all surprised. :) The LX patches from AMD were committed
to v2 REALLY quickly, the review then brought up some issues that
never got addressed and now we're getting them again.

//Peter

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

[Stefan Reinauer]

* Peter Stuge  [070628 00:50]:
> > so it's surprising to me how many problems we're seeing :-)
>
> I'm not at all surprised. :) The LX patches from AMD were committed
> to v2 REALLY quickly, the review then brought up some issues that
> never got addressed and now we're getting them again.

So what do you suggest we do in 4 days we have to get LBv3 working?
Except we could answer with "please send your comments in patch form"?
;-)

We are trying to fix the rough picture, and we run into danger to forget
about the small stuff once we have it working. How do we proceed?

Can anyone help? Uwe? Peter? Anyone else on the list?

Stefan

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

[ron minnich]

Peter, I did the best I can to respond to the comments. I can not fix
everything just yet.

The 0x100 I don't know about. Marc?

On 6/27/07, Peter Stuge  wrote:
> On Wed, Jun 27, 2007 at 09:14:27PM +0200, [e-mail] 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.
> > +#define RTC_CENTURY 0x32
> > +#define RTC_DOMA     0x3D
> > +#define RTC_MONA     0x3E
>
> Perhaps these should go into some .h?

but where?
> Maybe there should be a default: here to catch problems in the dts?

I just don't know how to handle it. Marc, what is that 7 6 5 4 in the
case in cs5536.h for the
addr =
bit?
> The comment is obviously wrong for USB. :)

I think I fixed that.
> >         /* ; EECP=50h, IST=01h, ASPC=1 */
> >         *(bar + HCCPARAMS) = 0x00005012;
>
> Don't know if these exist as defines, but they should.

Marc?
> #define 0x7C ?

Uh, Marc :-)?
> 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.
> > /*      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..

fixed.
> > msrnum = ATA_SB_GLD_MSR_ERR;
> > msr = rdmsr(msrnum);
> > msr.lo &= ~0x100;
> > wrmsr(msrnum, msr);
>
> #define 0x100 ?

Marc :-)?
> > +#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?

no, I will be reviving it.

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.

thanks

ron

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

[Uwe Hermann]

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?
> > 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?
> /* 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.

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

[ron minnich]

This patch is commited 391. There are a number of issues which we need
to resolve, and
Marc Jones is going to take a look at it now.

Thanks everyone, your patience with this process is greatly
appreciated, as are your comments!

Please, don't let us off the hook :-)

ron

[LinuxBIOS] [[e-mail]: 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  -----

From: Marc Jones 
To: Uwe Hermann 
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.

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

Uwe.

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

[Marc Jones]

Peter Stuge wrote:
> On Wed, Jun 27, 2007 at 09:14:27PM +0200, [e-mail] 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