Re: [PATCH]: Third (final?) release of Sun Neptune driver

Matheos Worku
2007-10-05T22:18:36Z

Ingo Oeser wrote:
> Hi David,

Repost since my last post ended up being html
Regards,

matheos

>
> some minor nits.

Some Neptune HW info to explain some of the constants ...

>
> Could this driver be split into more files. 8k lines per file

>> + case 0:
>> + ctrl_reg = ENET_SERDES_0_CTRL_CFG;
>> + test_cfg_reg = ENET_SERDES_0_TEST_CFG;
>> + break;
>> + case 1:
>> + ctrl_reg = ENET_SERDES_1_CTRL_CFG;
>> + test_cfg_reg = ENET_SERDES_1_TEST_CFG;
>> + break;
>> +
>> + default:
>> + return -EINVAL;
>> + }
>
>
> Maybe small table?
>
>> + ctrl_val = (ENET_SERDES_CTRL_SDET_0 |
>> + ENET_SERDES_CTRL_SDET_1 |
>> + ENET_SERDES_CTRL_SDET_2 |
>> + ENET_SERDES_CTRL_SDET_3 |
>> + (0x5 << ENET_SERDES_CTRL_EMPH_0_SHIFT) |
>> + (0x5 << ENET_SERDES_CTRL_EMPH_1_SHIFT) |
>> + (0x5 << ENET_SERDES_CTRL_EMPH_2_SHIFT) |
>> + (0x5 << ENET_SERDES_CTRL_EMPH_3_SHIFT) |
>> + (0x1 << ENET_SERDES_CTRL_LADJ_0_SHIFT) |
>> + (0x1 << ENET_SERDES_CTRL_LADJ_1_SHIFT) |
>> + (0x1 << ENET_SERDES_CTRL_LADJ_2_SHIFT) |
>> + (0x1 << ENET_SERDES_CTRL_LADJ_3_SHIFT));
>> + test_cfg_val = 0;
>> +
>> + if (lp->loopback_mode == LOOPBACK_PHY) {
>> + test_cfg_val |= ((ENET_TEST_MD_PAD_LOOPBACK <<
>> + ENET_SERDES_TEST_MD_0_SHIFT) |
>> + (ENET_TEST_MD_PAD_LOOPBACK <<
>> + ENET_SERDES_TEST_MD_1_SHIFT) |
>> + (ENET_TEST_MD_PAD_LOOPBACK <<
>> + ENET_SERDES_TEST_MD_2_SHIFT) |
>> + (ENET_TEST_MD_PAD_LOOPBACK <<
>> + ENET_SERDES_TEST_MD_3_SHIFT));
>> + }
>> +
>> + nw64(ctrl_reg, ctrl_val);
>> + nw64(test_cfg_reg, test_cfg_val);
>> +
>> + for (i = 0; i < 4; i++) {
>
> why 4? (magic number)

The MAC HW has 4 SERDES lanes. Need to configure each lane.

>
>> + u32 rxtx_ctrl, glue0;
>> + int err;
>> +
>> + err = esr_read_rxtx_ctrl(np, i, &rxtx_ctrl);
>> + if (err)
>> + return err;
>> + err = esr_read_glue0(np, i, &glue0);

>> +
>

>
> [...]
>
>> +static void niu_set_max_burst(struct niu *np, struct tx_ring_info *rp)
>> +{
>> + int mtu = np->dev->mtu;
>> +
>> + rp->max_burst = mtu + 32;
>> + if (rp->max_burst > 4096)
>> + rp->max_burst = 4096;
>
> Why 32 and 4096? (Magic values)

These values were recommended by the HW designers for fair utilization
of DRR feature among TX/RX rings.

>
>> +}
>> +
>
> ... ok, I stop here, since this drivers is damn big :-)
>
> Best Regards
>
> Ingo Oeser