Hello Stefan Need help on this?
Cheers On Tue, May 13, 2014 at 12:26 AM, Stefan Tauner <[email protected]> wrote: > On Mon, 12 May 2014 11:52:24 +0200 > Ricardo Ribalda Delgado <[email protected]> wrote: > >> On Sun, May 11, 2014 at 11:14 PM, Stefan Tauner >> <[email protected]> wrote: >> > On Thu, 5 Sep 2013 15:45:07 +0200 >> > Ricardo Ribalda Delgado <[email protected]> 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 _______________________________________________ flashrom mailing list [email protected] http://www.flashrom.org/mailman/listinfo/flashrom
