Hi Simon,
On 10/22/2015 10:15 AM, Simon Glass wrote:
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?
We would like to reset sgdma here. But Altera advices that software reset should be used with care to avoid bus lock up. So we wait the sgdma transfer to stop before applying a software reset.
If the sgdma stopped successfully, there is no need to do soft reset. Otherwise, we simply apply a soft reset and go on.
[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?
I will move the code into net ops for those with a single caller.
/* 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()?
The rx dma buffer need to be cache aligned. This is the point that Marek and I have discussed for a while. Though a __algined() modifier might work this way, char rx_buf[PKTSIZE_ALIGN] __aligned(ARCH_DMA_MINALIGN);
I do believe a more explicit and trusted malloc with cache aligned is better.
Thanks a lot for your review.
Best regards, Thomas