[PATCH] AMD revF resume support

[Rudolf Marek]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hello,

Following patch adds resume (exit from self refresh) support for AMD K8 revF
CPUs. It handles both type of erratas on those CPUs.

Signed-off-by: Rudolf Marek

Rudolf
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkng2/8ACgkQ3J9wPJqZRNV+EgCfdTvolSY5qH5qn8lqSnOzl4N4
XtkAn1g2PVFNXpW+yyBJsQ+uJUpWSFZv
=I5jd
-----END PGP SIGNATURE-----

[Peter Stuge]

Rudolf Marek wrote:
> Following patch adds resume (exit from self refresh) support for
> AMD K8 revF CPUs. It handles both type of erratas on those CPUs.
>
> Signed-off-by: Rudolf Marek

It looks ok but because this is the more tricky part I think it would
be nice to have one more opinion than mine.

Acked-by: Peter Stuge

[Marc Jones]

On Sun, Apr 12, 2009 at 10:31 PM, Peter Stuge  wrote:
> Rudolf Marek wrote:
> > Following patch adds resume (exit from self refresh) support for
> > AMD K8 revF CPUs. It handles both type of erratas on those CPUs.
> >
> > Signed-off-by: Rudolf Marek
>
> It looks ok but because this is the more tricky part I think it would
> be nice to have one more opinion than mine.
>
> Acked-by: Peter Stuge

Just some minor stuff.
> +++ src/northbridge/amd/amdk8/exit_from_self.c

...
> if (dcl & DCL_DimmEccEn) {
> 	uint32_t mnc;

make u32

...
> 		     ".align 64\n\t"
> 		     "out  %%al, (%%dx) \n\t"
> 		     "andb %1, %%al\n\t"
> 		     "out %%al, (%%dx)\n\t"

The controller might allow this but "out %%eax" would be more correct
with the more recent pci specs.

...
> if (loops >= TIMEOUT_LOOPS) {
> 	printk_debug(" failed\n");

You might want to know which controller is was on when it failed (or passes).

...
> for (i = 0; i < controllers; i++) {
> dqs_restore_MC_NVRAM((ctrl + i)->f2);
> sysinfo->mem_trained[i] = 1;	// mem was trained

I assume that somewhere in the path there is a nvram valid check?
> if (!sysinfo->ctrl_present[i])
> 	continue;
> /* Skip everything if I don't have any memory on this controller */
> if (sysinfo->meminfo[i].dimm_mask == 0x00)
> 	continue;

I don't think that the two "continues" do anything here at the end.
Should they be above the nvram and mem_trained code?
> +++ src/northbridge/amd/amdk8/raminit_f.c
> +#include "exit_from_self.c"

Just add the function here since it is always included.
> int s = acpi_is_wakeup_early();

better variable name?
> +++ src/northbridge/amd/amdk8/raminit_f_dqs.c
> +#ifdef S3_NVRAM_EARLY
> +int s3_save_nvram_early(u32 dword, int size, int  nvram_pos);
> +int s3_load_nvram_early(int size, u32 *old_dword, int nvram_pos);
> +#else
> +int s3_save_nvram_early(u32 dword, int size, int  nvram_pos) {
> +}
>
> -#if MEM_TRAIN_SEQ == 0
> +int s3_load_nvram_early(int size, u32 *old_dword, int nvram_pos) {
> +}
> +#endif

It would be better to stub this in the file that has
s3_save_nvram_early and add the prototype a .h.
> +static int load_index_to_pos(unsigned int dev, int size, int index, int nvram_pos) {
> u32 old_dword = pci_read_config32_index_wait(dev, 0x98, index);
> nvram_pos = s3_load_nvram_early(size, &old_dword, nvram_pos);

What happens when this is s3_load_nvram_early doesn't do anything?
> +static int dqs_load_MC_NVRAM_ch(unsigned int dev, int ch, int pos) {

Does this play nice with the mainboard nvram map?
> +#if HAVE_ACPI_RESUME
> dword = 0x21132113;

Can you comment on this setting?

Thanks,
Marc

[Rudolf Marek]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hello,
> > +++ src/northbridge/amd/amdk8/exit_from_self.c
>
> ...
> > if (dcl & DCL_DimmEccEn) {
> > 	uint32_t mnc;
>
> make u32

OK this was copied perhaps.
> ...
> > 		     ".align 64\n\t"
> > 		     "out  %%al, (%%dx) \n\t"
> > 		     "andb %1, %%al\n\t"
> > 		     "out %%al, (%%dx)\n\t"
>
> The controller might allow this but "out %%eax" would be more correct
> with the more recent pci specs.

Ok I will fix that
> ...
> > if (loops >= TIMEOUT_LOOPS) {
> > 	printk_debug(" failed\n");
>
> You might want to know which controller is was on when it failed (or passes).

OK.
> ...
> > for (i = 0; i < controllers; i++) {
> > dqs_restore_MC_NVRAM((ctrl + i)->f2);
> > sysinfo->mem_trained[i] = 1;	// mem was trained
>
> I assume that somewhere in the path there is a nvram valid check?

Well frankly I just did what DQS is doing to be code compatible with that.
> > if (!sysinfo->ctrl_present[i])
> > 	continue;
> > /* Skip everything if I don't have any memory on this controller */
> > if (sysinfo->meminfo[i].dimm_mask == 0x00)
> > 	continue;
>
> I don't think that the two "continues" do anything here at the end.
> Should they be above the nvram and mem_trained code?

Whoops it should be down there fixed.
> > +++ src/northbridge/amd/amdk8/raminit_f.c
> > +#include "exit_from_self.c"
>
> Just add the function here since it is always included.
> > int s = acpi_is_wakeup_early();
>
> better variable name?

Yes fixed.
> > +++ src/northbridge/amd/amdk8/raminit_f_dqs.c
> > +#ifdef S3_NVRAM_EARLY
> > +int s3_save_nvram_early(u32 dword, int size, int  nvram_pos);
> > +int s3_load_nvram_early(int size, u32 *old_dword, int nvram_pos);
> > +#else
> > +int s3_save_nvram_early(u32 dword, int size, int  nvram_pos) {
> > +}
> >
> > -#if MEM_TRAIN_SEQ == 0
> > +int s3_load_nvram_early(int size, u32 *old_dword, int nvram_pos) {
> > +}
> > +#endif
>
> It would be better to stub this in the file that has
> s3_save_nvram_early and add the prototype a .h.

Hmm v2 CAR is such mess. I would prefer to have it like this at least for now.
> > +static int load_index_to_pos(unsigned int dev, int size, int index, int nvram_pos) {
> > u32 old_dword = pci_read_config32_index_wait(dev, 0x98, index);
> > nvram_pos = s3_load_nvram_early(size, &old_dword, nvram_pos);
>
> What happens when this is s3_load_nvram_early doesn't do anything?

I will burn in hell ;)  I will put die in this funcs as it is used just when
doing resume.
> > +static int dqs_load_MC_NVRAM_ch(unsigned int dev, int ch, int pos) {
> Does this play nice with the mainboard nvram map?

It is an extra memory in mine case. not a cmos one. If one needs to use CMOS
then it must play well.
> > +#if HAVE_ACPI_RESUME
> > dword = 0x21132113;
>
> Can you comment on this setting?

Hmm I just added the response to S3 according the BKDG. You suspect anything is
wrong?

Committed revision 4102

Thanks,
Rudolf
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAknjhckACgkQ3J9wPJqZRNX67wCfai8sJM/OlpCbPCNSAqwdF+lz
r3UAnRgBjHdvsrulbB7sp3zfyQplHy/5
=2WML
-----END PGP SIGNATURE-----

[Marc Jones]

On Mon, Apr 13, 2009 at 12:34 PM, Rudolf Marek  wrote:
> > > +#if HAVE_ACPI_RESUME
> > > dword = 0x21132113;
> >
> > Can you comment on this setting?
>
> Hmm I just added the response to S3 according the BKDG. You suspect anything is
> wrong?

Ok, Just wondered where the value came from.
Looks good to me.

Thanks,
Marc