[U-Boot] [PATCH v2] net: convert altera_tse to driver model and phylib

Simon Glass
2015-10-22T02:15:03Z

Hi Thomas,

On 21 October 2015 at 07:05, Thomas Chou <thomas@wytron.com.tw> wrote:
>
> Convert altera_tse to driver model and phylib.
>
> Signed-off-by: Thomas Chou <thomas@wytron.com.tw>
> ---
> 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 <sjg@chromium.org>

[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