[flashrom] [RFC] National Semiconductor DP83815 NIC patch

Carl-Daniel Hailfinger c-d.hailfinger.devel.2006 at gmx.net
Mon Jun 7 23:19:56 CEST 2010


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:
>> 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
>>
>>    
> 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 Morgan<ziltro at 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

-- 
http://www.hailfinger.org/





More information about the flashrom mailing list