[coreboot] [PATCH] AMD revF resume support

[Marc Jones]

On Sun, Apr 12, 2009 at 10:31 PM, Peter Stuge <peter at stuge.se> 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 <r.marek at assembler.cz>
>
> 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 <peter at stuge.se>


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


-- 
http://marcjonesconsulting.com