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

Thomas Chou
2015-10-22T03:06:36Z

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