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

[Carl-Daniel Hailfinger]

Can someone please test this on Geode? Thanks.

On 13.02.2008 01:02, Carl-Daniel Hailfinger wrote:
> 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.

Enable Suspend-to-RAM code based on config option. Revert semantics to
those we had in v2 and before r385.
This causes pm_chipset_init() to be called on Geode LX.

Signed-off-by: Carl-Daniel Hailfinger

Index: LinuxBIOSv3-s3/southbridge/amd/cs5536/cs5536.c
===================================================================
--- LinuxBIOSv3-s3/southbridge/amd/cs5536/cs5536.c	(Revision 590)
+++ LinuxBIOSv3-s3/southbridge/amd/cs5536/cs5536.c	(Arbeitskopie)
@@ -488,8 +488,9 @@
    }
    sb = (struct southbridge_amd_cs5536_dts_config *)dev->device_configuration;

-#if 0
+#ifdef CONFIG_SUSPEND_TO_RAM
    if (!IsS3Resume())
+#endif
    {
    	struct acpi_init *aci = acpi_init_table;
    	for (; aci->ioreg; aci++) {
@@ -498,7 +499,6 @@
    	}
    	pm_chipset_init();
    }
-#endif

/* Set HD IRQ. */
    outl(GPIOL_2_SET, GPIO_IO_BASE + GPIOL_INPUT_ENABLE);
Index: LinuxBIOSv3-s3/device/Kconfig
===================================================================
--- LinuxBIOSv3-s3/device/Kconfig	(Revision 590)
+++ LinuxBIOSv3-s3/device/Kconfig	(Arbeitskopie)
@@ -87,3 +87,13 @@

endmenu

+menu "Power management"
+
+config SUSPEND_TO_RAM
+	bool "Suspend-to-RAM (S3)"
+	depends EXPERIMENTAL && BROKEN
+	help
+	  Enable support for Suspend-to-RAM (S3) functionality.
+
+endmenu
+