Here is the patch again with an updated comment regarding SMP and a signed-off-by line.
Signed-off-by: Lane Brooks <[EMAIL PROTECTED]> ron minnich wrote: > On 10/2/07, Lane Brooks <[EMAIL PROTECTED]> 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 -- linuxbios mailing list [email protected] http://www.linuxbios.org/mailman/listinfo/linuxbios
