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

[Thomas Chou <thomas@wytron.com.tw>]

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