On Tue, May 31, 2016 at 1:42 PM, Trent Piepho <tpiepho@xxxxxxxxxxxxxx> wrote:
> On Tue, 2016-05-31 at 10:09 -0700, Andrey Smirnov wrote:
>> Consolidate all code taking care on CSR offset differences for i210
>> chips into a single place in the driver and integrate that
>> funcionality into E1000_{READ,WRITE}_REG macros. This way we can get
>> rid of all those
>>
>> if (hw->mac_type == e1000_igb) {
>> ....
>> } else {
>> ....
>> }
>>
>> snippets sprinkled all across the driver code.
>>
>> +
>> +static inline uint32_t e1000_true_offset(struct e1000_hw *hw, uint32_t reg)
>> +{
>
> Any reason this needs to be inlined? gcc space vs size optimization
> choice usually does the right thing.
No reason, just a leftover of copy and paste from header file (I
initially put those functions there). Will fix in v2.
>
>> + if (hw->mac_type == e1000_igb) {
>> + unsigned int i;
>> +
>> + const struct e1000_fixup_table fixup_table[] = {
>> + { E1000_EEWR, E1000_I210_EEWR },
>> + { E1000_PHY_CTRL, E1000_I210_PHY_CTRL },
>> + { E1000_EEMNGCTL, E1000_I210_EEMNGCTL },
>> + };
>> +
>> + for (i = 0; i < ARRAY_SIZE(fixup_table); i++) {
>> + if (fixup_table[i].orig == reg)
>> + return fixup_table[i].fixed;
>> + }
>
> Looping through the table on each reg access seems a bit costly. What
> if the registers with different addresses had a flag bit in their
> definition? Then you could check the bit and only translate those that
> need translating. It would also document in the register definition
> that the register got translated.
>
> #define I210_ALT 0x100000 /* register has alternate addr on I210 */
> #define E1000_EEWR (0x0102C | I210_ALT)/* EEPROM Write Register - RW */
>
> if (reg & I210_ALT) {
> reg &= ~I210_ALT;
> if (hw->mac_type == e1000_igb) {
> /* look up alternate. note a case statement can be faster... */
> case (reg) {
> case E1000_EEWR: reg = E1000_I210_EEWR; break;
> default: ;
> }
> }
>
Sure, sounds like a good idea, will do in v2.
Thanks,
Andrey
_______________________________________________
barebox mailing list
barebox@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/barebox
