[U-Boot] [PATCH v6] net: ll_temac: Add LL TEMAC driver to u-boot

[Wolfgang Denk]

Dear Stephan Linz,

In message <1322419397-26326-2-git-send-email-linz at li-pro.net> you wrote:
> Xilinx LocalLink Tri-Mode Ether MAC driver can be
> used by Xilinx Microblaze or Xilinx ppc405/440 in
> SDMA and FIFO mode. DCR or XPS bus can be used.
> 
> The driver uses and requires MII and PHYLIB.
> 
> Signed-off-by: Stephan Linz <linz at li-pro.net>
...

> +static inline void xps_ll_temac_check_status(struct temac_reg *regs, u32 mask)
> +{
> +	unsigned timeout = 2000;
> +	while (timeout && (!(in_be32(&regs->rdy) & mask)))
> +		timeout--;

This has been asked before:  what exactly is the limit for the timeout
here?  2000 loops though that loop is not exactly a known value.
Please add some udelay() here (which is also good for triggering the
watchdog, should you have one running).


> +	if (!timeout)
> +		printf("%s: Timeout\n", __func__);

Why is this function void when you are know that you might have to
handle error situations (like a timeout)?

> +static void xps_ll_temac_hostif_set(struct eth_device *dev, u8 phy_addr,
> +					u8 reg_addr, u16 phy_data)
> +{
> +	struct temac_reg *regs = (struct temac_reg *)dev->iobase;
> +
> +	out_be32(&regs->lsw, (phy_data & LSW_REGDAT_MASK));
> +	out_be32(&regs->ctl, CTL_WEN | TEMAC_MIIMWD);
> +	out_be32(&regs->lsw,
> +		((phy_addr << LSW_PHYAD_POS) & LSW_PHYAD_MASK) |
> +		(reg_addr & LSW_REGAD_MASK));
> +	out_be32(&regs->ctl, CTL_WEN | TEMAC_MIIMAI);
> +	xps_ll_temac_check_status(regs, RSE_MIIM_WR);
> +}

So what happens here if we have a timeout in
xps_ll_temac_check_status()?  SHould such an error not be handled?

Ditto for all following functions.

> + * Undirect hostif read to ll_temac.

"Undirect"??  Or "Indirect" ?

"read to"??  Or "read from" ?

Please fix globally.

> +#ifdef DEBUG
> +static inline void read_phy_reg(struct eth_device *dev, u8 phy_addr)
> +{
> +	int j, result;
> +	debug("phy%d ", phy_addr);

Please always have one blank line between declarations and code.
Please fix globally.


> +	if (ll_temac->ctrlreset)
> +		if (ll_temac->ctrlreset(dev))
> +			return -1;

Braces needed for multiline statements.  Please fix globally.

> +/* halt device */
> +static void ll_temac_halt(struct eth_device *dev)
> +{
> +#ifdef ETH_HALTING
> +	struct ll_temac *ll_temac = dev->priv;
> +
> +	/* Disable Receiver */
> +	xps_ll_temac_indirect_set(dev, TEMAC_RCW0, 0);
> +
> +	/* Disable Transmitter */
> +	xps_ll_temac_indirect_set(dev, TEMAC_TC, 0);
> +
> +	if (ll_temac->ctrlhalt)
> +		ll_temac->ctrlhalt(dev);
> +#endif
> +}

ETH_HALTING is permanently undef'ed, so all this is dead code.  Please
remove.

> +static int ll_temac_bus_reset(struct mii_dev *bus)
> +{
> +	debug("Just bus reset\n");
> +	return 0;
> +}

Can we remove this function?  It does not appear to perform any useful
operation?

> + * FIXME: This part should going up to arch/powerpc -- but where?

s/going/go/ ?

> +/* Check for TX and RX channel errrors. */

s/rrr/rr/ ?

> +int ll_temac_halt_sdma(struct eth_device *dev)
> +{
> +	unsigned timeout = 2000;

See comments above.

> +	if (!timeout) {
> +		printf("%s: Timeout\n", __func__);
> +		return 1;
> +	}

Interesting - here you return an error code, above you don't :-(

...
> +
> +int ll_temac_send_sdma(struct eth_device *dev,
> +				volatile void *buffer, int length)
> +{
> +	unsigned timeout = 2000;
...
> +	if (!timeout)
> +		printf("%s: Timeout\n", __func__);
> +
> +	return 0;

Same issues again.  Please fix timout and error handling globally.

> +
> +#if defined(CONFIG_XILINX_440) || defined(CONFIG_XILINX_405)
> +/* Check for TX and RX channel errrors. */
> +static inline int ll_temac_dmac_error(struct eth_device *dev)
> +{
> +	int err;
> +	struct ll_temac *ll_temac = dev->priv;
> +	unsigned dmac_ctrl = ll_temac->ctrladdr;
> +
> +	err = mifdcr_xilinx(dmac_ctrl + TX_CHNL_STS) & CHNL_STS_ERROR;
> +	err |= mifdcr_xilinx(dmac_ctrl + RX_CHNL_STS) & CHNL_STS_ERROR;
> +	return err;
> +}
> +
> +void ll_temac_init_dmac(struct eth_device *dev)
> +{
> +	struct ll_temac *ll_temac = dev->priv;
> +	unsigned dmac_ctrl = ll_temac->ctrladdr;
> +	struct cdmac_bd *rx_dp = ll_temac->rx_dp;
> +	struct cdmac_bd *tx_dp = ll_temac->tx_dp;
> +
> +	memset(tx_dp, 0, sizeof(*tx_dp));
> +	memset(rx_dp, 0, sizeof(*rx_dp));
> +
> +	/* from LL TEMAC (Rx) */
> +	rx_dp->phys_buf_p = ll_temac->rx_bp;
> +
> +	rx_dp->next_p = rx_dp;
> +	rx_dp->buf_len = PKTSIZE_ALIGN;
> +	flush_cache((u32)rx_dp, sizeof(*tx_dp));
> +	flush_cache((u32)rx_dp->phys_buf_p, PKTSIZE_ALIGN);
> +
> +	/* setup first fd */
> +	mitdcr_xilinx(dmac_ctrl + RX_CURDESC_PTR, (u32)rx_dp);
> +	mitdcr_xilinx(dmac_ctrl + RX_TAILDESC_PTR, (u32)rx_dp);
> +	mitdcr_xilinx(dmac_ctrl + RX_NXTDESC_PTR, (u32)rx_dp);
> +
> +	/* to LL TEMAC (Tx) */
> +	tx_dp->phys_buf_p = ll_temac->tx_bp;
> +	tx_dp->next_p = tx_dp;
> +
> +	flush_cache((u32)tx_dp, sizeof(*tx_dp));
> +	mitdcr_xilinx(dmac_ctrl + TX_CURDESC_PTR, (u32)rx_dp);
> +}
> +
> +int ll_temac_halt_dmac(struct eth_device *dev)
> +{
> +	unsigned timeout = 2000;
> +	struct ll_temac *ll_temac = dev->priv;
> +	unsigned dmac_ctrl = ll_temac->ctrladdr;
> +
> +	/*
> +	 * Soft reset the DMA
> +	 *
> +	 * Quote from MPMC documentation: Writing a 1 to this field
> +	 * forces the DMA engine to shutdown and reset itself. After
> +	 * setting this bit, software must poll it until the bit is
> +	 * cleared by the DMA. This indicates that the reset process
> +	 * is done and the pipeline has been flushed.
> +	 */
> +	mitdcr_xilinx(dmac_ctrl + DMA_CONTROL_REG, DMA_CONTROL_RESET);
> +	while (timeout && (mifdcr_xilinx(dmac_ctrl + DMA_CONTROL_REG)
> +					& DMA_CONTROL_RESET))
> +		timeout--;
> +
> +	if (!timeout) {
> +		printf("%s: Timeout\n", __func__);
> +		return 1;
> +	}
> +
> +	return 0;
> +}
> +
> +int ll_temac_reset_dmac(struct eth_device *dev)
> +{
> +	u32 r;
> +	struct ll_temac *ll_temac = dev->priv;
> +	unsigned dmac_ctrl = ll_temac->ctrladdr;
> +
> +	/* Soft reset the DMA.  */
> +	if (ll_temac_halt_dmac(dev))
> +		return 1;
> +
> +	/* Now clear the interrupts.  */
> +	r = mifdcr_xilinx(dmac_ctrl + TX_CHNL_CTRL);
> +	r &= ~CHNL_CTRL_IRQ_MASK;
> +	mitdcr_xilinx(dmac_ctrl + TX_CHNL_CTRL, r);
> +
> +	r = mifdcr_xilinx(dmac_ctrl + RX_CHNL_CTRL);
> +	r &= ~CHNL_CTRL_IRQ_MASK;
> +	mitdcr_xilinx(dmac_ctrl + RX_CHNL_CTRL, r);
> +
> +	/* Now ACK pending IRQs.  */
> +	mitdcr_xilinx(dmac_ctrl + TX_IRQ_REG, IRQ_REG_IRQ_MASK);
> +	mitdcr_xilinx(dmac_ctrl + RX_IRQ_REG, IRQ_REG_IRQ_MASK);
> +
> +	/* Set tail-ptr mode, disable errors for both channels.  */
> +	mitdcr_xilinx(dmac_ctrl + DMA_CONTROL_REG,
> +			/* Enable use of tail pointer register */
> +			DMA_CONTROL_TPE |
> +			/* Disable error when 2 or 4 bit coalesce cnt overfl */
> +			DMA_CONTROL_RXOCEID |
> +			/* Disable error when 2 or 4 bit coalesce cnt overfl */
> +			DMA_CONTROL_TXOCEID);
> +
> +	return 0;
> +}
> +
> +int ll_temac_recv_dmac(struct eth_device *dev)
> +{
> +	int length;
> +	struct ll_temac *ll_temac = dev->priv;
> +	unsigned dmac_ctrl = ll_temac->ctrladdr;
> +	struct cdmac_bd *rx_dp = ll_temac->rx_dp;

Umm.... this code is more or less exactly the same as for
ll_temac_recv_sdma().  Can we please avoid this duplication of code?

Ditto for the other duplicated functions.


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Q: What do you get when you cross an ethernet with an income statement?
A: A local area networth.