Hi Thomas, On 21 October 2015 at 07:05, Thomas Chou <[email protected]> wrote: > > Convert altera_tse to driver model and phylib. > > Signed-off-by: Thomas Chou <[email protected]> > --- > v2 > remove volatile and use I/O accessors as suggested by Marek. > use virt_to_phys() to get dma address. > add free_pkt() ops. > add timeout to loop. > allocate cache aligned rx buffer. > refactor sgdma calls. > > configs/nios2-generic_defconfig | 2 + > doc/device-tree-bindings/net/altera_tse.txt | 112 +++ > drivers/net/Kconfig | 9 + > drivers/net/altera_tse.c | 1150 > +++++++++------------------ > drivers/net/altera_tse.h | 277 +------ > include/configs/nios2-generic.h | 8 + > 6 files changed, 526 insertions(+), 1032 deletions(-) > create mode 100644 doc/device-tree-bindings/net/altera_tse.txt >
This is a very nice piece of work. Reviewed-by: Simon Glass <[email protected]> [snip] > -static void tse_eth_reset(struct eth_device *dev) > +static void tse_eth_reset(struct altera_tse_priv *priv) > { > - /* stop sgdmas, disable tse receive */ > - struct altera_tse_priv *priv = dev->priv; > - volatile struct alt_tse_mac *mac_dev = priv->mac_dev; > - volatile struct alt_sgdma_registers *rx_sgdma = priv->sgdma_rx; > - volatile struct alt_sgdma_registers *tx_sgdma = priv->sgdma_tx; > - int counter; > - volatile struct alt_sgdma_descriptor *rx_desc = > - (volatile struct alt_sgdma_descriptor *)&priv->rx_desc[0]; > + struct alt_tse_mac *mac_dev = priv->mac_dev; > + struct alt_sgdma_registers *rx_sgdma = priv->sgdma_rx; > + struct alt_sgdma_registers *tx_sgdma = priv->sgdma_tx; > + struct alt_sgdma_descriptor *rx_desc = priv->rx_desc; > + unsigned int status; > + int ret; > + ulong ctime; > > /* clear rx desc & wait for sgdma to complete */ > rx_desc->descriptor_control = 0; > - rx_sgdma->control = 0; > - counter = 0; > - while (rx_sgdma->status & ALT_SGDMA_STATUS_BUSY_MSK) { > - if (counter++ > ALT_TSE_SGDMA_BUSY_WATCHDOG_CNTR) > - break; > - } > - > - if (counter >= ALT_TSE_SGDMA_BUSY_WATCHDOG_CNTR) { > - debug("Timeout waiting for rx sgdma!\n"); > - rx_sgdma->control = ALT_SGDMA_CONTROL_SOFTWARERESET_MSK; > - rx_sgdma->control = ALT_SGDMA_CONTROL_SOFTWARERESET_MSK; > - } > + writel(0, &rx_sgdma->control); > + ret = alt_sgdma_wait_transfer(rx_sgdma); > + if (ret == -ETIMEDOUT) > + writel(ALT_SGDMA_CONTROL_SOFTWARERESET_MSK, > + &rx_sgdma->control); > + > + writel(0, &tx_sgdma->control); > + ret = alt_sgdma_wait_transfer(tx_sgdma); > + if (ret == -ETIMEDOUT) > + writel(ALT_SGDMA_CONTROL_SOFTWARERESET_MSK, > + &tx_sgdma->control); > > - counter = 0; > - tx_sgdma->control = 0; > - while (tx_sgdma->status & ALT_SGDMA_STATUS_BUSY_MSK) { > - if (counter++ > ALT_TSE_SGDMA_BUSY_WATCHDOG_CNTR) > - break; It looks like this function can fail in various ways - should it not return an error? [snip] > +static int altera_tse_free_pkt(struct udevice *dev, uchar *packet, > + int length) > { These functions seem to just call other functions. Perhaps you might consider moving the code here a follow-on patch? > -static int tse_eth_init(struct eth_device *dev, bd_t * bd) > -{ > - int dat; > - struct altera_tse_priv *priv = dev->priv; > - volatile struct alt_tse_mac *mac_dev = priv->mac_dev; > - volatile struct alt_sgdma_descriptor *tx_desc = priv->tx_desc; > - volatile struct alt_sgdma_descriptor *rx_desc = priv->rx_desc; > - volatile struct alt_sgdma_descriptor *rx_desc_cur = > - (volatile struct alt_sgdma_descriptor *)&rx_desc[0]; > + /* > + * decode regs, assume address-cells and size-cells are both one. > + * there are multiple reg tuples, and they need to match with > + * reg-names. > + */ > + list = fdt_getprop(blob, node, "reg-names", &len); > + if (!list) > + return -ENOENT; > + end = list + len; > + cell = fdt_getprop(blob, node, "reg", &len); > + if (!cell) > + return -ENOENT; > + idx = 0; > + while (list < end) { > + addr = fdt_translate_address((void *)blob, > + node, cell + idx); > + size = fdt_addr_to_cpu(cell[idx + 1]); > + base = ioremap(addr, size); > + len = strlen(list); > + if (strcmp(list, "control_port") == 0) > + priv->mac_dev = base; > + else if (strcmp(list, "rx_csr") == 0) > + priv->sgdma_rx = base; > + else if (strcmp(list, "tx_csr") == 0) > + priv->sgdma_tx = base; > + else if (strcmp(list, "s1") == 0) > + desc_mem = base; > + idx += 2; > + list += (len + 1); > + } > + /* decode fifo depth */ > + priv->rx_fifo_depth = fdtdec_get_int(blob, node, > + "rx-fifo-depth", 0); > + priv->tx_fifo_depth = fdtdec_get_int(blob, node, > + "tx-fifo-depth", 0); > + /* decode phy */ > + addr = fdtdec_get_int(blob, node, > + "phy-handle", 0); > + addr = fdt_node_offset_by_phandle(blob, addr); > + priv->phyaddr = fdtdec_get_int(blob, addr, > + "reg", 0); > + /* init desc */ > + len = sizeof(struct alt_sgdma_descriptor) * 4; > + if (!desc_mem) { > + desc_mem = dma_alloc_coherent(len, &addr); > + if (!desc_mem) > + return -ENOMEM; > + } > + memset(desc_mem, 0, len); > + priv->tx_desc = desc_mem; > + priv->rx_desc = priv->tx_desc + 2; > + /* allocate recv packet buffer */ > + priv->rx_buf = malloc_cache_aligned(PKTSIZE_ALIGN); Can you just use uint8_t rx_buf[PKTSIZE_ALIGN] in this 'priv' structure? Why do a separate malloc()? Regards, Simon _______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
