diff mbox series

[U-Boot,v4,07/13] net: ftgmac100: handle timeouts when transmitting

Message ID 20181016092252.11664-8-clg@kaod.org
State Superseded
Delegated to: Joe Hershberger
Headers show
Series Support for the Faraday ftgmac100 controller | expand

Commit Message

Cédric Le Goater Oct. 16, 2018, 9:22 a.m. UTC 
Signed-off-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: Joel Stanley <joel@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(+)

Comments

Joe Hershberger Oct. 22, 2018, 7:55 p.m. UTC | #1 
Hi Cedric,

On Tue, Oct 16, 2018 at 4:32 AM Cédric Le Goater <clg@kaod.org> wrote:
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> Reviewed-by: Joel Stanley <joel@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;

[ ... ]
}

> +       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
Cédric Le Goater Oct. 29, 2018, 6:04 a.m. UTC | #2 
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 <clg@kaod.org> wrote:
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> Reviewed-by: Joel Stanley <joel@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
Joe Hershberger Oct. 29, 2018, 7:26 p.m. UTC | #3 
On Mon, Oct 29, 2018 at 1:04 AM Cédric Le Goater <clg@kaod.org> wrote:
>
> 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 <clg@kaod.org> wrote:
> >>
> >> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> >> Reviewed-by: Joel Stanley <joel@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.

I agree... spread the word to people with hardware who can make the
change and test. :)

> Sending a v5 with the proposed change.

Looks great, thanks!

>
> 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
diff mbox series

Patch

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)
+{
+	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 */