Here is the patch again with an updated comment regarding SMP and a
signed-off-by line.
Signed-off-by: Lane Brooks <lbrooks(a)mit.edu>
ron minnich wrote:
On 10/2/07, Lane Brooks <lbrooks(a)mit.edu>
wrote:
Attached is a patch that enables AMD Geode CS5536
chipset support. I
have tested it successfully on a MSM800 board from digital logic. It
does, however, have a few issues that I would like some feedback on.
In my discussions with Marc Jones, Geode systems write protect the BIOS
via RCONFs (cache settings similar to MTRRs). Unlocking requires
changing MSR 0x1808 top byte to 0x22. Reading and writing to the msr,
however, requires instrucitons rdmsr/wrmsr, which are ring0 privileged
instructions so only the kernel can do the read/write. So my patch uses
the msr kernel module to access these instructions from user space using
the /dev/cpu/0/msr device.
My questions are:
- I do not think this is portable beyond linux. Is that an issue?
it's not, but it is not really an issue at present.
- My code assumes the msr kernel module is
already loaded. Is there a
way to force a kernel module to load from the C code? My code does die
gracefully with a message reminding them to load the kernel module if
things fail.
it is possible I think to have udev help with this, but I am not sure.
Graceful failure is certainly a good start.
- It seems like there should be a way to revert
the msr back after
flashing is completed to put the bios back in write protect mode. Is
there a cleanup mechanism available? Something like disable_flash...
I thought that was in the structure of flashrom. Now that I look, it
seems like we lost it!
I propose this at the end of flashrom:
board_flash_disable(lb_vendor, lb_part);
chipset_flash_disable(chipset);
but we'll need to change some things to make this all work. We need a
penable struct * to use for the disable; no point in searching each
time we touch a chip. or not?
one comment on your patch: you use /dev/cpu/0/msr. This is great for a
single-cpu machine, and will always be fine on geode. Lots of times,
people use one piece of flashrom to write another. Imagine some future
hacker, in a year or two, who has copied your code for some SMP
system, not understanding why sometimes flashrom fails (i.e. they set
CPU0 msr but end up running on cpu1). We had this very problem when we
were getting graphics going (and, earlier, SMP going). I suggest a
comment as to why the /dev/cpu/0/msr is always safe on geode, but
future hackers beware on SMP. Or something like that.
That's up to you, however :-)
If you had a signed-off-by line, I would ack and commit for you :-)
ron
ron