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

[Ricardo Ribalda Delgado]

Hello Stefan

  Need help on this?

Cheers

On Tue, May 13, 2014 at 12:26 AM, Stefan Tauner
<stefan.tauner at alumni.tuwien.ac.at> wrote:
> 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



-- 
Ricardo Ribalda