[PATCH 1/2] include/linux/time.h: define USEC_PER_SEC and friends

[PATCH 1/2] include/linux/time.h: define USEC_PER_SEC and friends

[Ahmad Fatoum]

Linux has them in include/linux/time64.h and they are useful for
making (especially microsecond) use readable such as in read*_poll_timeout.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 include/linux/time.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/linux/time.h b/include/linux/time.h
index 3a1bb50020fd..7903139a65b2 100644
--- a/include/linux/time.h
+++ b/include/linux/time.h
@@ -3,7 +3,13 @@
 
 #include <linux/types.h>
 
+#define MSEC_PER_SEC	1000L
+#define USEC_PER_MSEC	1000L
+#define NSEC_PER_USEC	1000L
+#define NSEC_PER_MSEC	1000000L
+#define USEC_PER_SEC	1000000L
 #define NSEC_PER_SEC	1000000000L
+#define FSEC_PER_SEC	1000000000000000LL
 
 struct timespec {
 	time_t	tv_sec;		/* seconds */
-- 
2.19.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 2/2] net: fec_imx: fix timeout off by *1000 error

[Ahmad Fatoum]

read*_poll_timeout's final timeout parameter is in microseconds,
but the supplied arguments in fec_imx.c were in nanoseconds,
which might lead to barebox getting seemingly stuck in fec_halt
(loops for a thousand seconds instead of one).

I've tested this still works on an i.MX6D by copying a file over
TFTP and verifying the hash is correct.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/net/fec_imx.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/fec_imx.c b/drivers/net/fec_imx.c
index d304b94c567b..642517de6e98 100644
--- a/drivers/net/fec_imx.c
+++ b/drivers/net/fec_imx.c
@@ -61,7 +61,7 @@ static int fec_miibus_read(struct mii_bus *bus, int phyAddr, int regAddr)
 	 * wait for the related interrupt
 	 */
 	if (readl_poll_timeout(fec->regs + FEC_IEVENT, reg,
-			       reg & FEC_IEVENT_MII, MSECOND)) {
+			       reg & FEC_IEVENT_MII, USEC_PER_MSEC)) {
 		dev_err(&fec->edev.dev, "Read MDIO failed...\n");
 		return -1;
 	}
@@ -98,7 +98,7 @@ static int fec_miibus_write(struct mii_bus *bus, int phyAddr,
 	 * wait for the MII interrupt
 	 */
 	if (readl_poll_timeout(fec->regs + FEC_IEVENT, reg,
-			       reg & FEC_IEVENT_MII, MSECOND)) {
+			       reg & FEC_IEVENT_MII, USEC_PER_MSEC)) {
 		dev_err(&fec->edev.dev, "Write MDIO failed...\n");
 		return -1;
 	}
@@ -411,7 +411,7 @@ static void fec_halt(struct eth_device *dev)
 
 	/* wait for graceful stop to register */
 	if (readl_poll_timeout(fec->regs + FEC_IEVENT, reg,
-			       reg & FEC_IEVENT_GRA, SECOND))
+			       reg & FEC_IEVENT_GRA, USEC_PER_SEC))
 		dev_err(&dev->dev, "graceful stop timeout\n");
 
 	/* Disable SmartDMA tasks */
@@ -485,7 +485,7 @@ static int fec_send(struct eth_device *dev, void *eth_data, int data_length)
 	fec_tx_task_enable(fec);
 
 	if (readw_poll_timeout(&fec->tbd_base[fec->tbd_index].status,
-			       status, !(status & FEC_TBD_READY), SECOND))
+			       status, !(status & FEC_TBD_READY), USEC_PER_SEC))
 		dev_err(&dev->dev, "transmission timeout\n");
 
 	dma_unmap_single(fec->dev, dma, data_length, DMA_TO_DEVICE);
@@ -806,7 +806,7 @@ static int fec_probe(struct device_d *dev)
 	/* Reset chip. */
 	writel(FEC_ECNTRL_RESET, fec->regs + FEC_ECNTRL);
 	ret = readl_poll_timeout(fec->regs + FEC_ECNTRL, reg,
-				 !(reg & FEC_ECNTRL_RESET), SECOND);
+				 !(reg & FEC_ECNTRL_RESET), USEC_PER_SEC);
 	if (ret)
 		goto free_gpio;
 
-- 
2.19.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 2/2] net: fec_imx: fix timeout off by *1000 error

[Ahmad Fatoum]

Hello,

I didn't change read*_poll_timeout to take nanoseconds because it is copied
over from Linux.

On 14/12/18 16:23, Ahmad Fatoum wrote:
> read*_poll_timeout's final timeout parameter is in microseconds,
> but the supplied arguments in fec_imx.c were in nanoseconds,
> which might lead to barebox getting seemingly stuck in fec_halt
> (loops for a thousand seconds instead of one).

So, how about dropping the (U|M|)SECOND defines in include/clock.h and replace
them across the tree by the corresponding NSEC_PER_(U|M|)SEC?

I find them more readable and they would avoid such mistakes in future.

Cheers
Ahmad

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/2] include/linux/time.h: define USEC_PER_SEC and friends

[Sascha Hauer]

On Fri, Dec 14, 2018 at 04:23:05PM +0100, Ahmad Fatoum wrote:
> Linux has them in include/linux/time64.h and they are useful for
> making (especially microsecond) use readable such as in read*_poll_timeout.
> 
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  include/linux/time.h | 6 ++++++
>  1 file changed, 6 insertions(+)

Applied, thanks

Sascha

> 
> diff --git a/include/linux/time.h b/include/linux/time.h
> index 3a1bb50020fd..7903139a65b2 100644
> --- a/include/linux/time.h
> +++ b/include/linux/time.h
> @@ -3,7 +3,13 @@
>  
>  #include <linux/types.h>
>  
> +#define MSEC_PER_SEC	1000L
> +#define USEC_PER_MSEC	1000L
> +#define NSEC_PER_USEC	1000L
> +#define NSEC_PER_MSEC	1000000L
> +#define USEC_PER_SEC	1000000L
>  #define NSEC_PER_SEC	1000000000L
> +#define FSEC_PER_SEC	1000000000000000LL
>  
>  struct timespec {
>  	time_t	tv_sec;		/* seconds */
> -- 
> 2.19.1
> 
> 
> _______________________________________________
> barebox mailing list
> barebox@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/barebox
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 2/2] net: fec_imx: fix timeout off by *1000 error

[Sascha Hauer]

On Fri, Dec 14, 2018 at 04:29:02PM +0100, Ahmad Fatoum wrote:
> Hello,
> 
> I didn't change read*_poll_timeout to take nanoseconds because it is copied
> over from Linux.
> 
> On 14/12/18 16:23, Ahmad Fatoum wrote:
> > read*_poll_timeout's final timeout parameter is in microseconds,
> > but the supplied arguments in fec_imx.c were in nanoseconds,
> > which might lead to barebox getting seemingly stuck in fec_halt
> > (loops for a thousand seconds instead of one).
> 
> So, how about dropping the (U|M|)SECOND defines in include/clock.h and replace
> them across the tree by the corresponding NSEC_PER_(U|M|)SEC?
> 
> I find them more readable and they would avoid such mistakes in future.

You are probably right that these are more readable, but still you have
to know that is_timeout() takes nanoseconds and readl_poll_timeout takes
microseconds.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox