Re: [coreboot] [PATCH] consolidate K8M890 VGA code - take2

[Myles Watson]

2010/3/6 Rudolf Marek
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Hi,
>
> Following patch changes the K8M890 VGA handling. It reverts the framebuffer
> size
> to option based (similar what Uwe did) and also it uses GFXUMA to handle
> the
> high_tables_start offset from memory top.
>
> To satisfy the CMOS option users (Hi, libv! ;) I added also a possibility
> to do
> that through CMOS.
>
> Signed-off-by: Rudolf Marek

+choice
+    prompt "Framebuffer size"
+    default K8M890_VIDEO_MB_32MB
+    depends on SOUTHBRIDGE_VIA_K8T890_VGA_EN
+
+config K8M890_VIDEO_MB_32MB
+    bool "32MB"
+config K8M890_VIDEO_MB_64MB
+    bool "64MB"
+config K8M890_VIDEO_MB_128MB
+    bool "128MB"
+config K8M890_VIDEO_MB_256MB
+    bool "256MB"
+config K8M890_VIDEO_MB_CMOS
+    bool "Use CMOS option"

Shouldn't there be a config K8M890_VIDEO_MB_OFF choice?

+endchoice
+
+config VIDEO_MB
+    int
+    default 0   if K8M890_VIDEO_MB_OFF

I didn't see any comments documenting the meaning of fbbits.  At first I
thought it  was log2 of the MB of memory....

+        uma_memory_size = 4 << (fbbits + 20);
+    } else {
+        uma_memory_size = (CONFIG_VIDEO_MB << 20);
     }

And this definition looks a little different, but maybe it's equivalent?

+    fbbits = ((log2(uma_memory_size >> 20) - 2) << 4);
+    printk_info("K8M890: Using a %dMB framebuffer.\n", (unsigned int)
(uma_memory_size >> 20));

It seems like clarity would be good, since it's a user option.

With a few minor things addressed:

Acked-by: Myles Watson

Thanks,
Myles