Hello Joe,
On 10/22/18 9:55 PM, Joe Hershberger wrote:
> Hi Cedric,
>
> On Tue, Oct 16, 2018 at 4:32 AM Cédric Le Goater <c...@kaod.org> wrote:
>>
>> Signed-off-by: Cédric Le Goater <c...@kaod.org>
>> Reviewed-by: Joel Stanley <j...@jms.id.au>
>> ---
>>
>> Changes since v3 :
>>
>> - introduced a ftgmac100_wait_for_txdone() function similar to the
>> wait_for_bit_*() macros.
>>
>> drivers/net/ftgmac100.c | 44 +++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 44 insertions(+)
>>
>> diff --git a/drivers/net/ftgmac100.c b/drivers/net/ftgmac100.c
>> index bf8600814690..9adfe109ebc2 100644
>> --- a/drivers/net/ftgmac100.c
>> +++ b/drivers/net/ftgmac100.c
>> @@ -14,6 +14,7 @@
>> #include <dm.h>
>> #include <miiphy.h>
>> #include <net.h>
>> +#include <wait_bit.h>
>> #include <linux/io.h>
>> #include <linux/iopoll.h>
>>
>> @@ -28,6 +29,9 @@
>> /* PKTBUFSTX/PKTBUFSRX must both be power of 2 */
>> #define PKTBUFSTX 4 /* must be power of 2 */
>>
>> +/* Timeout for transmit */
>> +#define FTGMAC100_TX_TIMEOUT_MS 1000
>> +
>> /* Timeout for a mdio read/write operation */
>> #define FTGMAC100_MDIO_TIMEOUT_USEC 10000
>>
>> @@ -401,6 +405,41 @@ static int ftgmac100_recv(struct udevice *dev, int
>> flags, uchar **packetp)
>> return rxlen;
>> }
>>
>> +/*
>> + * The wait_for_bit_*() macros require a register value. This define a
>> + * similar routine which loops on the in-memory transmit descriptor to
>> + * wait for the MAC to clear the DMA_OWN bit.
>> + */
>> +static int ftgmac100_wait_for_txdone(struct ftgmac100_txdes *txdes,
>> + const unsigned int timeout_ms,
>> + const bool breakable)
>> +{
>
> I was hoping to see something like this instead:
>
> static u32 ftgmac100_read_txdesc(void *desc)
> {
> struct ftgmac100_txdes *txdes = desc;
> ulong des_start = (ulong)txdes;
> ulong des_end = des_start + roundup(sizeof(*txdes),
> ARCH_DMA_MINALIGN);
>
> invalidate_dcache_range(des_start, des_end);
>
> return txdes->txdes0;
> }
>
> BUILD_WAIT_FOR_BIT(ftgmac100_txdone, u32, ftgmac100_read_txdesc)
>
> [ ... ]
>
> ftgmac100_send( ... )
> {
> [ ... ]
>
> rc = wait_for_bit_ftgmac100_txdone(curr_des,
> FTGMAC100_TXDES0_TXDMA_OWN, false, FTGMAC100_TX_TIMEOUT_MS, true);
> if (rc)
> return rc;
>
> [ ... ]
> }
Yes, this is much better. A few other drivers could make use of a similar
macro.
Sending a v5 with the proposed change.
Thanks,
C.
>> + ulong des_start = (ulong)txdes;
>> + ulong des_end = des_start + roundup(sizeof(*txdes),
>> ARCH_DMA_MINALIGN);
>> + ulong start = get_timer(0);
>> +
>> + while (1) {
>> + invalidate_dcache_range(des_start, des_end);
>> +
>> + if (!(txdes->txdes0 & FTGMAC100_TXDES0_TXDMA_OWN))
>> + return 0;
>> +
>> + if (get_timer(start) > timeout_ms)
>> + break;
>> +
>> + if (breakable && ctrlc()) {
>> + puts("Abort\n");
>> + return -EINTR;
>> + }
>> +
>> + udelay(1);
>> + WATCHDOG_RESET();
>> + }
>> +
>> + dev_err(dev, "transmit timeout\n");
>> + return -ETIMEDOUT;
>> +}
>> +
>> /*
>> * Send a data block via Ethernet
>> */
>> @@ -414,6 +453,7 @@ static int ftgmac100_send(struct udevice *dev, void
>> *packet, int length)
>> roundup(sizeof(*curr_des), ARCH_DMA_MINALIGN);
>> ulong data_start;
>> ulong data_end;
>> + int rc;
>>
>> invalidate_dcache_range(des_start, des_end);
>>
>> @@ -446,6 +486,10 @@ static int ftgmac100_send(struct udevice *dev, void
>> *packet, int length)
>> /* Start transmit */
>> writel(1, &ftgmac100->txpd);
>>
>> + rc = ftgmac100_wait_for_txdone(curr_des, FTGMAC100_TX_TIMEOUT_MS,
>> true);
>> + if (rc)
>> + return rc;
>> +
>> debug("%s(): packet sent\n", __func__);
>>
>> /* Move to next descriptor */
>> --
>> 2.17.2
>>
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot@lists.denx.de
>> https://lists.denx.de/listinfo/u-boot
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
