Dear Stephan Linz,
In message <1322419397-26326-2-git-send-email-l...@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 <l...@li-pro.net>
...
> +static inline void xps_ll_temac_check_status(struct temac_reg *regs, u32
> mask)
> +{
> + unsigned timeout = 2000;
> + while (timeout && (!(in_be32(®s->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(®s->lsw, (phy_data & LSW_REGDAT_MASK));
> + out_be32(®s->ctl, CTL_WEN | TEMAC_MIIMWD);
> + out_be32(®s->lsw,
> + ((phy_addr << LSW_PHYAD_POS) & LSW_PHYAD_MASK) |
> + (reg_addr & LSW_REGAD_MASK));
> + out_be32(®s->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: w...@denx.de
Q: What do you get when you cross an ethernet with an income statement?
A: A local area networth.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
