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

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