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

[Simon Glass]

Hi Thomas,

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