Re: [PATCH 13/23] e1000: gather hardware bit tweaks.

[Jeff Garzik <jgarzik@pobox.com>]

Kok, Auke wrote:
> Several hardware bits were set all over the driver and have been
> consolidated into a single function.

> diff --git a/drivers/net/e1000/e1000_hw.c b/drivers/net/e1000/e1000_hw.c
> index 9422864..a143b49 100644
> --- a/drivers/net/e1000/e1000_hw.c
> +++ b/drivers/net/e1000/e1000_hw.c
> @@ -61,6 +61,7 @@ static int32_t e1000_id_led_init(struct 
>  static int32_t e1000_init_lcd_from_nvm_config_region(struct e1000_hw *hw, uint32_t cnf_base_addr, uint32_t cnf_size);
>  static int32_t e1000_init_lcd_from_nvm(struct e1000_hw *hw);
>  static void e1000_init_rx_addrs(struct e1000_hw *hw);
> +static void e1000_initialize_hardware_bits(struct e1000_hw *hw);
>  static boolean_t e1000_is_onboard_nvm_eeprom(struct e1000_hw *hw);
>  static int32_t e1000_kumeran_lock_loss_workaround(struct e1000_hw *hw);
>  static int32_t e1000_mng_enable_host_if(struct e1000_hw *hw);
> @@ -716,6 +717,110 @@ e1000_reset_hw(struct e1000_hw *hw)
>  }
>  
>  /******************************************************************************
> + *
> + * Initialize a number of hardware-dependent bits
> + *
> + * hw: Struct containing variables accessed by shared code
> + *
> + *****************************************************************************/
> +static void
> +e1000_initialize_hardware_bits(struct e1000_hw *hw)
> +{
> +    if ((hw->mac_type >= e1000_82571) && (!hw->initialize_hw_bits_disable)) {
> +        /* Settings common to all silicon */
> +        uint32_t reg_ctrl, reg_ctrl_ext;
> +        uint32_t reg_tarc0, reg_tarc1;
> +        uint32_t reg_tctl;
> +        uint32_t reg_txdctl, reg_txdctl1;
> +
> +        reg_tarc0 = E1000_READ_REG(hw, TARC0);
> +        reg_tarc0 &= ~0x78000000;           /* Clear bits 30, 29, 28, and 27 */
> +
> +        reg_txdctl = E1000_READ_REG(hw, TXDCTL);
> +        reg_txdctl |= E1000_TXDCTL_COUNT_DESC;       /* Set bit 22 */
> +        E1000_WRITE_REG(hw, TXDCTL, reg_txdctl);
> +
> +        reg_txdctl1 = E1000_READ_REG(hw, TXDCTL1);
> +        reg_txdctl1 |= E1000_TXDCTL_COUNT_DESC;      /* Set bit 22 */
> +        E1000_WRITE_REG(hw, TXDCTL1, reg_txdctl1);
> +
> +        switch (hw->mac_type) {
> +            case e1000_82571:
> +            case e1000_82572:
> +                reg_tarc1 = E1000_READ_REG(hw, TARC1);
> +                reg_tctl = E1000_READ_REG(hw, TCTL);
> +
> +                /* Set the phy Tx compatible mode bits */
> +                reg_tarc1 &= ~0x60000000;   /* Clear bits 30 and 29 */
> +
> +                reg_tarc0 |= 0x07800000;    /* Set TARC0 bits 23-26 */
> +                reg_tarc1 |= 0x07000000;    /* Set TARC1 bits 24-26 */
> +
> +                if (reg_tctl & E1000_TCTL_MULR)
> +                    reg_tarc1 &= ~0x10000000;   /* Clear bit 28 if MULR is 1b */
> +                else
> +                    reg_tarc1 |= 0x10000000;    /* Set bit 28 if MULR is 0b */
> +
> +                E1000_WRITE_REG(hw, TARC1, reg_tarc1);
> +                break;
> +            case e1000_82573:
> +                reg_ctrl_ext = E1000_READ_REG(hw, CTRL_EXT);
> +                reg_ctrl = E1000_READ_REG(hw, CTRL);
> +
> +                reg_ctrl_ext &= ~0x00800000;    /* Clear bit 23 */
> +                reg_ctrl_ext |= 0x00400000;     /* Set bit 22 */
> +                reg_ctrl &= ~0x20000000;        /* Clear bit 29 */
> +
> +                E1000_WRITE_REG(hw, CTRL_EXT, reg_ctrl_ext);
> +                E1000_WRITE_REG(hw, CTRL, reg_ctrl);
> +                break;
> +            case e1000_80003es2lan:
> +                if ((hw->media_type == e1000_media_type_fiber) ||
> +                    (hw->media_type == e1000_media_type_internal_serdes)) {
> +                    reg_tarc0 &= ~0x00100000;   /* Clear bit 20 */
> +                }
> +
> +                reg_tctl = E1000_READ_REG(hw, TCTL);
> +                reg_tarc1 = E1000_READ_REG(hw, TARC1);
> +                if (reg_tctl & E1000_TCTL_MULR)
> +                    reg_tarc1 &= ~0x10000000;   /* Clear bit 28 if MULR is 1b */
> +                else
> +                    reg_tarc1 |= 0x10000000;    /* Set bit 28 if MULR is 0b */
> +
> +                E1000_WRITE_REG(hw, TARC1, reg_tarc1);
> +                break;
> +            case e1000_ich8lan:
> +                if ((hw->revision_id < 3) ||
> +                    ((hw->device_id != E1000_DEV_ID_ICH8_IGP_M_AMT) &&
> +                     (hw->device_id != E1000_DEV_ID_ICH8_IGP_M)))
> +                    reg_tarc0 |= 0x30000000;    /* Set TARC0 bits 29 and 28 */
> +                reg_ctrl_ext = E1000_READ_REG(hw, CTRL_EXT);
> +                reg_ctrl_ext |= 0x00400000;     /* Set bit 22 */
> +                E1000_WRITE_REG(hw, CTRL_EXT, reg_ctrl_ext);
> +
> +                reg_tarc0 |= 0x0d800000;    /* Set TARC0 bits 23, 24, 26, 27 */
> +
> +                reg_tarc1 = E1000_READ_REG(hw, TARC1);
> +                reg_tctl = E1000_READ_REG(hw, TCTL);
> +
> +                if (reg_tctl & E1000_TCTL_MULR)
> +                    reg_tarc1 &= ~0x10000000;   /* Clear bit 28 if MULR is 1b */
> +                else
> +                    reg_tarc1 |= 0x10000000;    /* Set bit 28 if MULR is 0b */
> +
> +                reg_tarc1 |= 0x45000000;        /* Set bit 24, 26 and 30 */
> +
> +                E1000_WRITE_REG(hw, TARC1, reg_tarc1);
> +                break;
> +            default:
> +                break;
> +        }

Overall this is a positive change.  However, NAK'd for two superficial 
reasons:

1) the comments are completely useless.  We can see from reading the 
code what the comments tell us.  What the comments DON'T tell us are (a) 
a description of the bit being set, and (b) why that bit is being set.

2) the easy-for-humans-to-read notation for bit setting is

	(1 << n)

not a hex constant.