[flashrom] [RFC] National Semiconductor DP83815 NIC patch

[Carl-Daniel Hailfinger]

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 at 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

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