Re: [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.