[flashrom] [PATCH] nicintel_eeprom: New programmer for Intel Nics eeproms

[Stefan Tauner]

On Mon, 12 May 2014 11:52:24 +0200
Ricardo Ribalda Delgado <ricardo.ribalda at gmail.com> wrote:

> On Sun, May 11, 2014 at 11:14 PM, Stefan Tauner
> <stefan.tauner at alumni.tuwien.ac.at> wrote:
> > On Thu,  5 Sep 2013 15:45:07 +0200
> > Ricardo Ribalda Delgado <ricardo.ribalda at gmail.com> wrote:
> >
> >
> > This probably means that we want to copy parts of the copyright lines
> > from those files here. I don't care too much about this, but
> > Carl-Daniel probably does and it would be the right thing™ to do. The
> > least effort would be to copy them all... Carl-Daniel, what do you
> > think? For now I have only added myself for the opaque programmer
> > implementation in ichspi.c and my changes today.
> 
> That depends how you handle copyright on your project. The "based on"
> declaration it is usually enough.
> The author line it is generally used to specify who to cc on a bug/patch.

It's totally OK for me, but Carl-Daniel is rather picky about these
things.

> >> +
> >> +/* EEPROM/Flash Control & Data Register */
> >> +#define EECD 0x10
> >
> > Where does the D in EECD comes from? My datasheet denotes it EEC only.
> 
> To be consistent with the names of the other parts of flashrom. Just
> in case in the future you want to add a big define for all the intel
> cards. The kernel does this:
> 
> driver/ethernet/intel/e1000/e1000_hw.h

Ah! I did not notice that the registers of the older cards are almost
the same (although the supported eeproms are different).
I'll use the names of the respective datasheet nevertheless in this
case.

> >> +const struct dev_entry nics_intel_ee[] = {
> >> +     {PCI_VENDOR_ID_INTEL, 0x150e, OK, "Intel", "82580 Quad/Dual Gigabit Ethernet LAN Controller"},
> >> +     {PCI_VENDOR_ID_INTEL, UNPROG_DEVICE, OK, "Intel", "Unprogrammed 82580 Quad/Dual Gigabit Ethernet LAN Controller"},
> >
> > Should we include more than this? First of all the devices that were
> > tested by others, but also untested devices that should actually work.
> 
> I would recommend only adding tested devices, but it is your call.

That would require the user to recompile flashrom without gain. We use
the OK, NT etc. enum values to indicate if the device has been tested. I
have added the other device IDs mentioned in the datasheet.

> >> +     {0},
> >> +};
> >> +
> >> +static int nicintel_ee_probe(struct flashctx *flash)
> >> +{
> >> +     if (nicintel_pci->device_id == UNPROG_DEVICE)
> >> +             flash->chip->total_size = 16;
> >
> > Why is this a special case? AFAICS this default ID is given if no
> > EEPROM is attached although I guess one could store exactly this ID in
> > the EEPROM too? UNPROG seems to indicate that this is also set for
> > empty EEPROMs. Isn't this dead code in case no EEPROM is attached due o
> > the check for EE_PRES in the init function? And why doesn't the probe
> > fail but defaults to 16kB instead? I seem to neither understand Intel's
> > auto-detection of the EEPROM size nor your interpretation.
> 
> The intel card recognizes the eeprom size based on the eeprom content:
> Word 0x12, bit 13:10
> 
> A device shows itself as UNPROG_DEVICE if the eeprom content is wrong or empty.
> 
> In that case we have to default to the smallest size supported by the
> card :  16 Kbytes

OK, thanks, that makes sense. I have added an appropriate comments to
the probe function and a reference to 3.3.1.5. OTOH, I am not convinced
yet, that an unset EE_PRES bit is a complete show stopper yet. The
description in 3.3.1.5 is either unrelated to EE_PRES or at least one
of the descriptions (EE_PRES or 3.3.15) is wrong, because they
contradict themselves IMHO.

While viewing the datasheet I have also noticed 4.7 which seems to have
been ignored so far. I'll assume our module works nevertheless, but I
have added a FIXME comment.

> 
> As fas as I know it is impossible to auto detect the size of an
> eeprom. (unless you write to it)

At least for those in questions this seems to be true.

> >> +#define RETRIES_EERD 10000000
> >
> > How did you obtain all retry values?
> 
> Trial and error.

Fair enough but it should be documented :)

> >> +     /* Automatic restore of EECD on shutdown is not possible because EECD
> >> +      * does not only contain EE_REQ but other bits with side effects as well.
> >> +      * Those other bits must be left untouched.
> >> +      */
> >
> > This is actually not true. We can of course store the state of the bits
> > we want to restore and use that in a shutdown function that only
> > restores these bits. I have implemented this in my patch for the bit
> > banging output pins/bits and EE_REQ.
> 
> Then please also do the same on nicintel_spi.c :P

Well, the comment in nicintel_spi.c is referring to the
pci_*r*mmio_writel functions that tidy up after themselves. The
"semi-automatic" approach using registered shutdown functions works of
course (and that's what implemented there too already). The only
difference is that they unconditionally disable flash access there
while we restored exactly the original state of the bits in question.

> >
> >> +     if (dev->device_id != UNPROG_DEVICE) {
> >
> > And why this distinction again?
> 
> When the device does not have an eeprom/checksum is wrong, the EE_PRES
> bit is not reliable

Understood, thank you.

> I have made some trivial changes on the code and added the manpage entry.

I had to revert a number of them again. We do use a line length of 112
characters, and I personally detest c89 variable declarations w/o good
reason but prefer tighter scopes. Some of them made sense nevertheless,
thanks.

> I have also tested the code on my board and it looks ok. I have sended
> the patch with git send-email

Great, thanks. As the first EEPROM-accessing flashrom module it
deserves its own section in the manpage and I also want to warn about
possible breakage if EEPROM values become invalid for the NIC. I'll add
a dedicated section to the manpage and commit it sometime this week if
Carl-Daniel does not object. Thanks for all your efforts and patience.
-- 
Kind regards/Mit freundlichen Grüßen, Stefan Tauner