Re: [PATCH 08/21] e1000: Consolidate register offset fixups

[Andrey Smirnov <andrew.smirnov@gmail.com>]

On Tue, May 31, 2016 at 1:42 PM, Trent Piepho <tpiepho@kymetacorp.com> 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@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox