On Mon, Feb 15, 2016 at 10:37:36AM +0100, Christian Ehrhardt wrote: > > Hi, > > before I post the next iteration of the patch, I'd like > some more advice on some of the points you commented (inline):
> > > > Odd that I219 advertises itself as I217 > > Hm, I didn't look at this code in detail at all. It is one of those > places where the intel code does not seem to make a difference between > I217/I218 and I219. > > However, given that stuff works, this is apparently how it is... Right. > > The latest Intel code in FreeBSD passes size as an argument to > > this function. Should ours do the same? > > Which function are you talking about? We do pass size to > em_read_ich8_data above already, em-7.5.2 does not have a size > argument to e1000_read_flash_data32_ich8lan which is the equivalent > of the function below. https://svnweb.freebsd.org/base/head/sys/dev/e1000/ which is "em(4) 7.6.1; igb(4) 2.5.3" according to the logs. /** * e1000_read_flash_data_ich8lan - Read byte or word from NVM * @hw: pointer to the HW structure * @offset: The offset (in bytes) of the byte or word to read. * @size: Size of data to read, 1=byte 2=word * @data: Pointer to the word to store the value read. * * Reads a byte or word from the NVM using the flash access registers. **/ static s32 e1000_read_flash_data_ich8lan(struct e1000_hw *hw, u32 offset, u8 size, u16 *data) I wouldn't worry about supporting the other sizes there for now. Intel appears to have added it in a later version. > > > @@ -9833,6 +10123,82 @@ em_write_ich8_data(struct em_hw *hw, uin > > > } > > > > This function is only called by em_verify_write_ich8_dword() which > > itself isn't called. Any reason to keep either of them? > > I can remove the eeprom write functions if you prefer? > > The idea was that we could keep the code that is already ported > until someone (else) finds the time to add the missing bits to > the erase flash bank and function and to the eeprom update code above > (remember the XXX comment...). I'd prefer them to be removed if they are unused to not further grow the ramdisk/kernel size with unused code. Why would we want to write to the eeprom/nvm? > > > > In the Intel code in FreeBSD e1000_read_flash_dword_ich8lan does the > > offset shift instead of the caller and checks if data is NULL. > > > > That would make this function seem less pointless. > > Yes, I figured that but didn't want to change the interface for > existing callers of em_read_ich8_word and these two should have > a consistent interface! The way it was done in the initial diff works for now. > Remaining open questions: > - What about the unused eeprom write code? remove it > - Should we change the interface for em_read_ich8_{byte,word,dword} > to match the intel code or should we stick to the current openbsd > calling convention? leave as is > - Do we need to investigate the phy type issue (I217 vs. I219) further? no, that it matches and works is proof that it is fine.
