Re: 2.6.12-rc3-mm1 (a-new-10gb-ethernet-driver-by-chelsio-communications.patch)

From: Alexey Dobriyan
Date: Sat Apr 30 2005 - 14:33:44 EST



> --- /dev/null
> +++ 25-akpm/drivers/net/chelsio/common.h

> +#define DIMOF(x) (sizeof(x)/sizeof(x[0]))

ARRAY_SIZE in include/linux/kernel.h

> --- /dev/null
>+++ 25-akpm/drivers/net/chelsio/cpl5_cmd.h

> +struct cpl_tx_pkt {
> + __u16 vlan;

__be16.

> +struct cpl_tx_pkt_lso {
> + __u32 len;

__be32

> + __u16 eth_type_mss;

__be16

> +struct cpl_rx_pkt {
> + __u16 vlan;

_be16 really. You do ntohs(p->vlan).

If any other fields of these structs are in network order I'd mark them as
such.

> --- /dev/null
> +++ 25-akpm/drivers/net/chelsio/cxgb2.c

> +#if BITS_PER_LONG == 64 && !defined(CONFIG_X86_64)
> +# define FMT64 "l"
> +#else
> +# define FMT64 "ll"
> +#endif

FMT64 is unused.

> +#define PCI_DMA_64BIT ~0ULL
> +#define PCI_DMA_32BIT 0xffffffffULL

In include/linux/dma-mapping.h DMA_64BIT_MASK and DMA_32BIT_MASK, respectively.

> +static const char pci_speed[][4] = {
> + "33", "66", "100", "133"
> +};

Unused.

> +static void get_stats(struct net_device *dev, struct ethtool_stats *stats,
> + u64 *data)
> +{

> + *data++ = s->TxOctetsOK;
> + *data++ = s->TxOctetsBad;
> + *data++ = s->TxUnicastFramesOK;
> + *data++ = s->TxMulticastFramesOK;
> + *data++ = s->TxBroadcastFramesOK;
> + *data++ = s->TxPauseFrames;
> + *data++ = s->TxFramesWithDeferredXmissions;
> + *data++ = s->TxLateCollisions;
> + *data++ = s->TxTotalCollisions;
> + *data++ = s->TxFramesAbortedDueToXSCollisions;
> + *data++ = s->TxUnderrun;
> + *data++ = s->TxLengthErrors;
> + *data++ = s->TxInternalMACXmitError;
> + *data++ = s->TxFramesWithExcessiveDeferral;
> + *data++ = s->TxFCSErrors;
> +
> + *data++ = s->RxOctetsOK;
> + *data++ = s->RxOctetsBad;
> + *data++ = s->RxUnicastFramesOK;
> + *data++ = s->RxMulticastFramesOK;
> + *data++ = s->RxBroadcastFramesOK;
> + *data++ = s->RxPauseFrames;
> + *data++ = s->RxFCSErrors;
> + *data++ = s->RxAlignErrors;
> + *data++ = s->RxSymbolErrors;
> + *data++ = s->RxDataErrors;
> + *data++ = s->RxSequenceErrors;
> + *data++ = s->RxRuntErrors;
> + *data++ = s->RxJabberErrors;
> + *data++ = s->RxInternalMACRcvError;
> + *data++ = s->RxInRangeLengthErrors;
> + *data++ = s->RxOutOfRangeLengthField;
> + *data++ = s->RxFrameTooLongErrors;

It seems you want one big memcpy.

> +static int ethtool_ioctl(struct net_device *dev, void *useraddr)

void __user *addr. To make sparse happy.

> +{

> + if (copy_from_user(&cmd, useraddr, sizeof(cmd)))

> +static int __devinit init_one(struct pci_dev *pdev,
> + const struct pci_device_id *ent)
> +{

> + if (!pci_set_dma_mask(pdev, PCI_DMA_64BIT)) {

DMA_64BIT_MASK

> + if (pci_set_consistent_dma_mask(pdev, PCI_DMA_64BIT)) {

DMA_64BIT_MASK

> + } else if ((err = pci_set_dma_mask(pdev, PCI_DMA_32BIT)) != 0) {

DMA_32BIT_MASK

> --- /dev/null
> +++ 25-akpm/drivers/net/chelsio/cxgb2.h

> +struct adapter {
> + u8 *regs;

void __iomem *regs; You do ->regs = ioremap(...);

> --- /dev/null
> +++ 25-akpm/drivers/net/chelsio/espi.c

> +/* 1. Deassert rx_reset_core. */
> +/* 2. Program TRICN_CNFG registers. */
> +/* 3. Deassert rx_reset_link */
> +static int tricn_init(adapter_t *adapter)
> +{

> + /* 1 */
> + timeout=1000;

> + /* 2 */
> + if (sme) {

> + /* 3 */
> + t1_write_reg_4(adapter, A_ESPI_RX_RESET, 0x3);

So move comments where they should be. ;-)

> +/* T2 Init part -- */
> +/* 1. Set T_ESPI_MISCCTRL_ADDR */
> +/* 2. Init ESPI registers. */
> +/* 3. Init TriCN Hard Macro */
> +int t1_espi_init(struct peespi *espi, int mac_type, int nports)
> +{

> --- /dev/null
> +++ 25-akpm/drivers/net/chelsio/mv88x201x.c

> + if (do_enable & LINK_ENABLE_BIT) {
> + led |= LINK_ENABLE_BIT;
> + mdio_write(cphy, 0x1, 0x7, led);
> + } else {
> + led &= ~LINK_ENABLE_BIT;
> + mdio_write(cphy, 0x1, 0x7, led);
> + }

Common mdio_write line.

> --- /dev/null
> +++ 25-akpm/drivers/net/chelsio/pm3393.c

> +static int pm3393_macaddress_set(struct cmac *cmac, u8 ma[6])
> +{
> + u32 val, lo, mid, hi

> + * MAC addr: 00:07:43:00:13:09
> + *
> + * ma[5] = 0x09
> + * ma[4] = 0x13
> + * ma[3] = 0x00
> + * ma[2] = 0x43
> + * ma[1] = 0x07
> + * ma[0] = 0x00
> + *
> + * The PM3393 requires byte swapping and reverse order entry
> + * when programming MAC addresses:
> + *
> + * low_bits[15:0] = ma[1]:ma[0]
> + * mid_bits[31:16] = ma[3]:ma[2]
> + * high_bits[47:32] = ma[5]:ma[4]

> + lo = ((u32) ma[1] << 8) | (u32) ma[0];
> + mid = ((u32) ma[3] << 8) | (u32) ma[2];
> + hi = ((u32) ma[5] << 8) | (u32) ma[4];

This is

lo = (u32) le16_to_cpu((__le16 *) &ma[0]);
mid = (u32) le16_to_cpu((__le16 *) &ma[2]);
hi = (u32) le16_to_cpu((__le16 *) &ma[4]);

Right?

> + /* The following steps are required to properly reset
> + * the PM3393. This information is provided in the
> + * PM3393 datasheet (Issue 2: November 2002)
> + * section 13.1 -- Device Reset.
> + *
> + * The PM3393 has three types of components that are
> + * individually reset:
> + *
> + * DRESETB - Digital circuitry
> + * PL4_ARESETB - PL4 analog circuitry
> + * XAUI_ARESETB - XAUI bus analog circuitry
> + *
> + * Steps to reset PM3393 using RSTB pin:
> + *
> + * 1. Assert RSTB pin low ( write 0 )
> + * 2. Wait at least 1ms to initiate a complete initialization of device.
> + * 3. Wait until all external clocks and REFSEL are stable.
> + * 4. Wait minimum of 1ms. (after external clocks and REFEL are stable)
> + * 5. De-assert RSTB ( write 1 )
> + * 6. Wait until internal timers to expires after ~14ms.
> + * - Allows analog clock synthesizer(PL4CSU) to stabilize to
> + * selected reference frequency before allowing the digital
> + * portion of the device to operate.
> + * 7. Wait at least 200us for XAUI interface to stabilize.
> + * 8. Verify the PM3393 came out of reset successfully.
> + * Set successful reset flag if everything worked else try again
> + * a few more times.

Same comment about comments.

--- /dev/null
+++ 25-akpm/drivers/net/chelsio/regs.h

> +/* Do not edit this file */

OK.

--- /dev/null
+++ 25-akpm/drivers/net/chelsio/subr.c

> +static struct board_info t1_board[] = {
> +
> +{ CHBT_BOARD_N110, 1/*ports#*/,
> + SUPPORTED_10000baseT_Full | SUPPORTED_FIBRE /*caps*/, CHBT_TERM_T1,
> + CHBT_MAC_PM3393, CHBT_PHY_88X2010,
> + 125000000/*clk-core*/, 0/*clk-mc3*/, 0/*clk-mc4*/,
> + 1/*espi-ports*/, 0/*clk-cspi*/, 44/*clk-elmer0*/, 0/*mdien*/,
> + 0/*mdiinv*/, 1/*mdc*/, 0/*phybaseaddr*/, &t1_pm3393_ops,
> + &t1_mv88x201x_ops, &mi1_mdio_ext_ops,
> + "Chelsio N110 1x10GBaseX NIC" },
> +
> +{ CHBT_BOARD_N210, 1/*ports#*/,
> + SUPPORTED_10000baseT_Full | SUPPORTED_FIBRE /*caps*/, CHBT_TERM_T2,
> + CHBT_MAC_PM3393, CHBT_PHY_88X2010,
> + 125000000/*clk-core*/, 0/*clk-mc3*/, 0/*clk-mc4*/,
> + 1/*espi-ports*/, 0/*clk-cspi*/, 44/*clk-elmer0*/, 0/*mdien*/,
> + 0/*mdiinv*/, 1/*mdc*/, 0/*phybaseaddr*/, &t1_pm3393_ops,
> + &t1_mv88x201x_ops, &mi1_mdio_ext_ops,
> + "Chelsio N210 1x10GBaseX NIC" },
> +
> +};

That's what C99 initializers are for:

static struct board_info t1_board[] = {
{
.board = CHBT_BOARD_N110,
.port_number = 1,
.caps = SUPPORTED_10000baseT_Full | SUPPORTED_FIBRE,
.chip_term = CHBT_TERM_T1,
...
}, {
...
},
};

> +const struct board_info *t1_get_board_info(unsigned int board_id)
> +{
> + return board_id < DIMOF(t1_board) ? &t1_board[board_id] : NULL;

ARRAY_SIZE