Re: [coreboot] [PATCH] Enable Suspend-to-RAM code based on config option

[Carl-Daniel Hailfinger]

The patch is correct if you compare it with latest v3 code, but
incorrect if you compare it to v2 because v3 has inverted logic. See below.

Ron? This one is for you.

On 12.02.2008 19:26, Carl-Daniel Hailfinger wrote:
> Enable Suspend-to-RAM code based on Kconfig option.
> Does not work yet, so the Kconfig option depends on BROKEN.
>
> Signed-off-by: Carl-Daniel Hailfinger
> Index: LinuxBIOSv3-if0crud/southbridge/amd/cs5536/cs5536.c
>
> --- LinuxBIOSv3-if0crud/southbridge/amd/cs5536/cs5536.c	(Revision 587)
> +++ LinuxBIOSv3-if0crud/southbridge/amd/cs5536/cs5536.c	(Arbeitskopie)
> @@ -488,7 +488,7 @@
>    }
>    sb = (struct southbridge_amd_cs5536_dts_config *)dev->device_configuration;
>
> -#if 0
> +#ifdef CONFIG_SUSPEND_TO_RAM
>    if (!IsS3Resume())
>    {
>    	struct acpi_init *aci = acpi_init_table;

Looking at the equivalent v2 code and the history of these lines in v3,
I found a logic inversion committed in
> r385 | rminnich | 2007-06-27 22:52:49 +0200 (Mi, 27 Jun 2007) | 9 lines
>
> Trivial fixes for some typos, and a major fix for an unitialized
> variable.
>
> Add a license to dts.
>
> Signed-off-by: Ronald G. Minnich 
> Acked-by: Stefan Reinauer 
>  void chipsetinit(void)
>  {
>    struct device *dev;
>    msr_t msr;
>    u32 msrnum;
> struct southbridge_amd_cs5536_config *sb =
>    (struct southbridge_amd_cs5536_config *)dev->device_configuration;
>
> struct southbridge_amd_cs5536_config *sb;
> struct msrinit *csi;
>
> 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));
> return;
> }
> sb = (struct southbridge_amd_cs5536_config *)dev->device_configuration;
> /* we hope NEVER to be in linuxbios when S3 resumes
>
> +#if 0
>       if (! IsS3Resume()) */

In the old (before r385) version, the code below was always executed.
In the new version, the code below is never executed.
v2 behaves like the code before r385.
> {
>    	struct acpiinit *aci = acpi_init_table;
> @@ -531,6 +534,7 @@
> pm_chipset_init();
>
> }
> +#endif
>
> /* set hd IRQ */
>    outl(GPIOL_2_SET, GPIO_IO_BASE + GPIOL_INPUT_ENABLE);

I believe we have to revert the semantic change.

Regards,
Carl-Daniel