Hi,
I have a couple of Netgear FA311 network cards, which have DP83815DVNG chips on board, and a space to mount a 32-pin boot ROM socket. Looking at the datasheet for the chip I have created this patch.
The datasheet suggests that reads and writes must be done 32 bits at a time, and only at 32 bit offsets. This seems strange for chips which have an 8 bit data bus.
I don't think this code is ready for inclusion in flashrom yet, but if anyone has a card with this chip and can test it out then give it a go. I'm pretty sure it won't just work though. It might even destroy your computer/house/planet.
I can't test it yet because the cards don't have a 32pin socket mounted on them. If and when I can add one then I can do some tests.
This is my first 'large' patch so let me know what I have got wrong. :)
Signed-off-by: Andrew Morgan ziltro@ziltro.com
On 04.06.2010 05:07, Andrew Morgan wrote:
I have a couple of Netgear FA311 network cards, which have DP83815DVNG chips on board, and a space to mount a 32-pin boot ROM socket. Looking at the datasheet for the chip I have created this patch.
The datasheet suggests that reads and writes must be done 32 bits at a time, and only at 32 bit offsets. This seems strange for chips which have an 8 bit data bus.
I don't think this code is ready for inclusion in flashrom yet, but if anyone has a card with this chip and can test it out then give it a go. I'm pretty sure it won't just work though. It might even destroy your computer/house/planet.
I can't test it yet because the cards don't have a 32pin socket mounted on them. If and when I can add one then I can do some tests.
This is my first 'large' patch so let me know what I have got wrong. :)
Signed-off-by: Andrew Morgan ziltro@ziltro.com
Oh, it looks really nice.
In fact, we could commit this instantly except for two reasons: 1. It's untested, and completely untested drivers should default to no in the Makefile. Once we have the first successful test, we can change it to yes. 2. The INB/OUTB for data contradicts the datasheet (I know, the datasheet might very well be wrong), so a comment would be appreciated in those places, e.g. "The datasheet says this register can only be accessed with full 32 bit width, but that would make 8 bit writes impossible. Due to that, we assume the meaning was garbled in translation."
Hm. It is entirely possible that the datasheet means the register access mode is 32 bit. The contents of the data register might only care about the lowest 8 bits. That would also mean your driver could work...
Change those two things, and you get an ack from me and I'll commit.
Regards, Carl-Daniel
On 04/06/10 15:13, Carl-Daniel Hailfinger wrote:
Oh, it looks really nice. In fact, we could commit this instantly except for two reasons:
- It's untested, and completely untested drivers should default to no
in the Makefile. Once we have the first successful test, we can change it to yes. 2. The INB/OUTB for data contradicts the datasheet (I know, the datasheet might very well be wrong), so a comment would be appreciated in those places, e.g. "The datasheet says this register can only be accessed with full 32 bit width, but that would make 8 bit writes impossible. Due to that, we assume the meaning was garbled in translation."
Hm. It is entirely possible that the datasheet means the register access mode is 32 bit. The contents of the data register might only care about the lowest 8 bits. That would also mean your driver could work...
Change those two things, and you get an ack from me and I'll commit.
Regards, Carl-Daniel
Thanks for looking at the previous patch! New patch attached... This is now for DP83815/DP83816 (same PCI ID) and DP83820 (same flash access). I disabled compilation by default, and added the comment in both places. I was hoping that now I have had a chance to solder a boot ROM socket to the card I could test the code and make it work. I have done some tests and it looks like this is very close, in fact it may well be that this code works but my soldering is no good! I've been getting some good data read & written, and some random data appearing. Probing seems to work fine. Erasing seems to work too, but read and/or write sometimes get random data. I believe a dry solder joint would explain this behavior.
Signed-off-by: Andrew Morganziltro@ziltro.com
On 07.06.2010 22:53, Andrew Morgan wrote:
On 04/06/10 15:13, Carl-Daniel Hailfinger wrote:
Oh, it looks really nice. In fact, we could commit this instantly except for two reasons:
- It's untested, and completely untested drivers should default to no
in the Makefile. Once we have the first successful test, we can change it to yes. 2. The INB/OUTB for data contradicts the datasheet (I know, the datasheet might very well be wrong), so a comment would be appreciated in those places, e.g. "The datasheet says this register can only be accessed with full 32 bit width, but that would make 8 bit writes impossible. Due to that, we assume the meaning was garbled in translation."
Hm. It is entirely possible that the datasheet means the register access mode is 32 bit. The contents of the data register might only care about the lowest 8 bits. That would also mean your driver could work...
Change those two things, and you get an ack from me and I'll commit.
Regards, Carl-Daniel
Thanks for looking at the previous patch! New patch attached... This is now for DP83815/DP83816 (same PCI ID) and DP83820 (same flash access). I disabled compilation by default, and added the comment in both places. I was hoping that now I have had a chance to solder a boot ROM socket to the card I could test the code and make it work. I have done some tests and it looks like this is very close, in fact it may well be that this code works but my soldering is no good! I've been getting some good data read & written, and some random data appearing. Probing seems to work fine. Erasing seems to work too, but read and/or write sometimes get random data. I believe a dry solder joint would explain this behavior.
Signed-off-by: Andrew Morganziltro@ziltro.com
- /*
- The datasheet says this register can only be
- accessed with full 32 bit width, but that would make 8 bit writes
- impossible. Due to that, we assume the meaning was garbled in translation.
- */
Can you reformat this comment (and the other identical comment) a bit? Please make sure to take care of the 80 column limit, start each line with an asterisk and align the asterisks, similar to this:
/* * comment * more comment * even more comment */
Ah yes, and while I know I suggested this wording, I'd like to rephrase it a bit. Sorry.
"The datasheet requires 32 bit accesses to this register, but it seems that requirement might only apply if the register is memory mapped. Bit 8-31 of this register are apparently don't care, and if this register is I/O port mapped 8 bit accesses to the lowest byte of the register seem to work fine. Due to that, we ignore the advice in the data sheet."
Note to you: It would be an interesting experiment to replace the OUTB with an OUTL, and compare write/read reliability if the high 24 bits are 1 or 0. This is not needed before I commit, but hey... maybe it helps you and the soldering is OK after all.
Sorry for the nitpicks in the second review round. I think the patch looks really good, and plan to apply it later tonight.
Regards, Carl-Daniel
On 07/06/10 22:19, Carl-Daniel Hailfinger wrote:
Can you reformat this comment (and the other identical comment) a bit? Please make sure to take care of the 80 column limit, start each line with an asterisk and align the asterisks, similar to this:
/* * comment * more comment * even more comment */
Ah yes, and while I know I suggested this wording, I'd like to rephrase it a bit. Sorry.
"The datasheet requires 32 bit accesses to this register, but it seems that requirement might only apply if the register is memory mapped. Bit 8-31 of this register are apparently don't care, and if this register is I/O port mapped 8 bit accesses to the lowest byte of the register seem to work fine. Due to that, we ignore the advice in the data sheet."
Note to you: It would be an interesting experiment to replace the OUTB with an OUTL, and compare write/read reliability if the high 24 bits are 1 or 0. This is not needed before I commit, but hey... maybe it helps you and the soldering is OK after all.
Sorry for the nitpicks in the second review round. I think the patch looks really good, and plan to apply it later tonight.
Regards, Carl-Daniel
Here's the next patch, hopefully it is ok. I did try to think of more descriptive text for the comment, then got busy testing the code out instead.
I have removed one line:
max_rom_decode.parallel = 65536;
As it shouldn't have been there anyway. I don't believe this is a requirement as the NIC chip has more address lines than are required for 64K and there are tracks on the PCB leading to the boot ROM socket for those address lines.
I had a quick try replacing OUTB with OUTL and that doesn't seem to work, even probe doesn't work then, lots of "probe_jedec_common: id1 0xff, id2 0xff".
Patch attached.
On 08.06.2010 00:12, Andrew Morgan wrote:
On 07/06/10 22:19, Carl-Daniel Hailfinger wrote:
Can you reformat this comment (and the other identical comment) a bit? Please make sure to take care of the 80 column limit, start each line with an asterisk and align the asterisks, similar to this:
/* * comment * more comment * even more comment */Ah yes, and while I know I suggested this wording, I'd like to rephrase it a bit. Sorry.
"The datasheet requires 32 bit accesses to this register, but it seems that requirement might only apply if the register is memory mapped. Bit 8-31 of this register are apparently don't care, and if this register is I/O port mapped 8 bit accesses to the lowest byte of the register seem to work fine. Due to that, we ignore the advice in the data sheet."
Note to you: It would be an interesting experiment to replace the OUTB with an OUTL, and compare write/read reliability if the high 24 bits are 1 or 0. This is not needed before I commit, but hey... maybe it helps you and the soldering is OK after all.
Sorry for the nitpicks in the second review round. I think the patch looks really good, and plan to apply it later tonight.
Regards, Carl-Daniel
Here's the next patch, hopefully it is ok. I did try to think of more descriptive text for the comment, then got busy testing the code out instead.
I have removed one line:
max_rom_decode.parallel = 65536;
As it shouldn't have been there anyway. I don't believe this is a requirement as the NIC chip has more address lines than are required for 64K and there are tracks on the PCB leading to the boot ROM socket for those address lines.
Are you sure? AFAICS your code can't support more than 64 kB because it truncates the address to 16 bits. Due to that, it should definitely set max_rom_decode.parallel. You can try changing the address mask, and if that give you good readbacks, you can still increase the size. However, in the end every programmer with parallel flash has to set this limit to make flashing safe for users.
I had a quick try replacing OUTB with OUTL and that doesn't seem to work, even probe doesn't work then, lots of "probe_jedec_common: id1 0xff, id2 0xff".
Ah interesting, so it is probably just a MMIO access limitation.
Patch attached.
Acked-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net and committed in r1039.
I noticed you forgot the Signed-off-by statement in your latest patch, but since there were only very small changes, I reused the Signed-off-by statement in the commit log. It would be cool if you could respond to your latest patch with a signoff, though (no need to resend the patch itself).
TODO: Please send a patch which sets max_rom_decode.parallel to a size which makes sense (i.e. 65536 with the current code) and please add printing of the programmer PCI devices to print.c and print_wiki.c. It would be cool if you could add some info to the man page as well. Just copy and paste from an existing programmer there.
Regards, Carl-Daniel
On 07/06/10 23:47, Carl-Daniel Hailfinger wrote:
Are you sure? AFAICS your code can't support more than 64 kB because it truncates the address to 16 bits. Due to that, it should definitely set max_rom_decode.parallel. You can try changing the address mask, and if that give you good readbacks, you can still increase the size. However, in the end every programmer with parallel flash has to set this limit to make flashing safe for users.
Good point. :) I have set it to 128K now, see comment in patch. I hope the comment is ok.
TODO: Please send a patch which sets max_rom_decode.parallel to a size which makes sense (i.e. 65536 with the current code) and please add printing of the programmer PCI devices to print.c and print_wiki.c. It would be cool if you could add some info to the man page as well. Just copy and paste from an existing programmer there.
Done with the exception of the man page, as CONFIG_NICNATSEMI is off by default it wouldn't make sense to be in the man page yet, and I don't really know the syntax. I could probably just copy '...nicrealtek...' like I have done in other places though... ;)
print.c doesn't pad the PCI bus IDs: (0020/0022) National Semiconductor DP83815/DP83816 [100b:20] (untested) National Semiconductor DP83820 [100b:22] (untested)
The attached patch adds nicnatsemi to print.c and print_wiki.c, changes the address mask to use MA0-MA16 and sets the maximum decode size to 128KB.
Signed-off-by Andrew Morgan ziltro@ziltro.com
On 08/06/10 01:29, Andrew Morgan wrote:
I have set it to 128K now, see comment in patch. I hope the comment is ok.
The 128KB comment might not be needed any more. It seems that 128KB is the correct maximum size. Using a multimeter I found that the voltage on A16 does change during a read. The other thing I found is that the network card uses 3.3v for the boot ROM, not 5v! As I have been doing all my tests with a 5v±10% 256KB flash chip, and one of the address lines was floating, I'm not surprised there was random data being read. (I am surprised that erase seemed to work.)
So now all I need to do is find a 128KB 3.3v DIP32 parallel flash chip...
Hi Andrew, This probably isn't too important, Most (all?) parallel flash can jive with TTL levels (ie. >= 2V is high, <= 0,8V is low, in-between is garbage).
eg. this datasheet (29F002) - see the Vil and Vih specifications. http://www.datasheetcatalog.org/datasheets/150/489768_DS.pdf
However, it is essential that VCC be 5V, give or take. 3,3V will cause errors. Are you saying Vcc is 3,3V? Or just the data/addr lines?
-Mark
On Mon, Jun 7, 2010 at 7:29 PM, Andrew Morgan ziltro@ziltro.com wrote:
On 08/06/10 01:29, Andrew Morgan wrote:
I have set it to 128K now, see comment in patch. I hope the comment is ok.
The 128KB comment might not be needed any more. It seems that 128KB is the correct maximum size. Using a multimeter I found that the voltage on A16 does change during a read. The other thing I found is that the network card uses 3.3v for the boot ROM, not 5v! As I have been doing all my tests with a 5v±10% 256KB flash chip, and one of the address lines was floating, I'm not surprised there was random data being read. (I am surprised that erase seemed to work.)
So now all I need to do is find a 128KB 3.3v DIP32 parallel flash chip...
--
Andrew.
flashrom mailing list flashrom@flashrom.org http://www.flashrom.org/mailman/listinfo/flashrom
On 09/06/10 02:24, Marko Kraljevic wrote:
Hi Andrew, This probably isn't too important, Most (all?) parallel flash can jive with TTL levels (ie.>= 2V is high,<= 0,8V is low, in-between is garbage).
eg. this datasheet (29F002) - see the Vil and Vih specifications. http://www.datasheetcatalog.org/datasheets/150/489768_DS.pdf
However, it is essential that VCC be 5V, give or take. 3,3V will cause errors. Are you saying Vcc is 3,3V? Or just the data/addr lines?
Vcc is 3.3v! I wonder if that's why they didn't put a socket on the board... :)
Hi Andrew,
thanks for your patch.
On 08.06.2010 02:29, Andrew Morgan wrote:
On 07/06/10 23:47, Carl-Daniel Hailfinger wrote:
Are you sure? AFAICS your code can't support more than 64 kB because it truncates the address to 16 bits. Due to that, it should definitely set max_rom_decode.parallel. You can try changing the address mask, and if that give you good readbacks, you can still increase the size. However, in the end every programmer with parallel flash has to set this limit to make flashing safe for users.
Good point. :) I have set it to 128K now, see comment in patch. I hope the comment is ok.
TODO: Please send a patch which sets max_rom_decode.parallel to a size which makes sense (i.e. 65536 with the current code) and please add printing of the programmer PCI devices to print.c and print_wiki.c. It would be cool if you could add some info to the man page as well. Just copy and paste from an existing programmer there.
Done with the exception of the man page, as CONFIG_NICNATSEMI is off by default it wouldn't make sense to be in the man page yet, and I don't really know the syntax. I could probably just copy '...nicrealtek...' like I have done in other places though... ;)
print.c doesn't pad the PCI bus IDs: (0020/0022) National Semiconductor DP83815/DP83816 [100b:20] (untested) National Semiconductor DP83820 [100b:22] (untested)
The attached patch adds nicnatsemi to print.c and print_wiki.c, changes the address mask to use MA0-MA16 and sets the maximum decode size to 128KB.
Signed-off-by Andrew Morgan ziltro@ziltro.com
Acked-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net and committed in r1095.
Regards, Carl-Daniel
On 07/06/10 23:12, Andrew Morgan wrote:
Patch attached.
Forgot to add:
Signed-off-by: Andrew Morgan ziltro@ziltro.com
Hoping to get another patch with further changes soon.
-
Andrew Morgan -
Carl-Daniel Hailfinger -
Marko Kraljevic