Re: [PATCH 13/23] e1000: gather hardware bit tweaks.
From: Jeff Garzik <hidden>
Date: 2006-09-19 19:36:48
Kok, Auke wrote:
Several hardware bits were set all over the driver and have been consolidated into a single function.
quoted hunk ↗ jump to hunk
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.