2014-03-10 13:56 GMT+01:00 Stefan Hajnoczi <stefa...@redhat.com>:
>> @@ -409,6 +468,9 @@ set_ctrl(E1000State *s, int index, uint32_t val)
>> {
>> /* RST is self clearing */
>> s->mac_reg[CTRL] = val & ~E1000_CTRL_RST;
>> + if (val & E1000_CTRL_RST) {
>> + e1000_reset(s);
>> + }
>
> Maybe this should be a separate commit. Is this a bugfix?
Honoring resets is required for the semaphores used in recent cards, IIRC.
> Where does this template come from and what is the relationship between
> the EEPROM and the flash? Does the device have both EEPROM and flash?
As far as I can tell, same kind of data, different storage medium. And it comes
from the documentation :-)
> How are the BARs numbered on the real device? Do all models have flash?
The driver always use 1 for flash, even though not all devices have
flash. Didn't
quite understand how the driver tells the difference.
> We cannot change the BARs, it would break migration between old and new
> QEMU.
1 is required for flash in the driver, it's hardcoded.
>> @@ -542,9 +587,9 @@
>> #define E1000_EEPROM_SWDPIN0 0x0001 /* SWDPIN 0 EEPROM Value */
>> #define E1000_EEPROM_LED_LOGIC 0x0020 /* Led Logic Word */
>> #define E1000_EEPROM_RW_REG_DATA 16 /* Offset to data in EEPROM
>> read/write registers */
>> -#define E1000_EEPROM_RW_REG_DONE 0x10 /* Offset to READ/WRITE done bit */
>> +#define E1000_EEPROM_RW_REG_DONE 0x2 /* Offset to READ/WRITE done bit */
>
> Please put a change like this in a separate commit and explain the
> reasoning.
Bugfix. The old data are wrong (according to the doc and the driver and the
fact that they don't work with the device that need them :-)
>> #define E1000_EEPROM_RW_REG_START 1 /* First bit for telling part to
>> start operation */
>> -#define E1000_EEPROM_RW_ADDR_SHIFT 8 /* Shift to the address bits */
>> +#define E1000_EEPROM_RW_ADDR_SHIFT 2 /* Shift to the address bits */
>
> This can go together with the E1000_EEPROM_RW_REG_DONE change.
Same answer.
> I don't think you can use C bitfields since the bit-ordering is up to
> the compiler. It may or may not match the hardware register layout!
> Instead, use uint16_t and define bitmask constants so you can use
> bitwise-AND/OR.
Copy/paste from the e1000e driver... works for it :-)
Anyway, I probably won't be able to update the patch, I've already spent
a lot more time on it than I should have :-/
Cordially,
--
Romain Dolbeau
