[PATCH 1/3] arm: mxs: use timeouts in block reset routines

[PATCH 1/3] arm: mxs: use timeouts in block reset routines

[Wolfram Sang]

These routines can fail, add support for that. Also, put in missing
copyright headers.

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
---
 arch/arm/mach-mxs/common.c |   39 +++++++++++++++++++++++++++++++++++----
 1 file changed, 35 insertions(+), 4 deletions(-)

diff --git a/arch/arm/mach-mxs/common.c b/arch/arm/mach-mxs/common.c
index 3730633..122d883 100644
--- a/arch/arm/mach-mxs/common.c
+++ b/arch/arm/mach-mxs/common.c
@@ -1,8 +1,26 @@
+/*
+ * Freescale i.MXS common code
+ *
+ * Copyright (C) 2012 Wolfram Sang <w.sang@pengutronix.de>
+ *
+ * Based on code from LTIB:
+ * Copyright (C) 2010 Freescale Semiconductor, Inc.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ */
+
 #include <common.h>
 #include <io.h>
+#include <errno.h>
+#include <clock.h>
 #include <mach/mxs.h>
 #include <mach/imx-regs.h>
 
+#define	MXS_IP_RESET_TIMEOUT	(10 * MSECOND)
+
 #define	MXS_BLOCK_SFTRST				(1 << 31)
 #define	MXS_BLOCK_CLKGATE				(1 << 30)
 
@@ -10,7 +28,9 @@ int mxs_reset_block(void __iomem *reg, int just_enable)
 {
 	/* Clear SFTRST */
 	writel(MXS_BLOCK_SFTRST, reg + BIT_CLR);
-	mdelay(1);
+
+	if (wait_on_timeout(MXS_IP_RESET_TIMEOUT, !(readl(reg) & MXS_BLOCK_SFTRST)))
+		goto timeout;
 
 	/* Clear CLKGATE */
 	writel(MXS_BLOCK_CLKGATE, reg + BIT_CLR);
@@ -18,16 +38,27 @@ int mxs_reset_block(void __iomem *reg, int just_enable)
 	if (!just_enable) {
 		/* Set SFTRST */
 		writel(MXS_BLOCK_SFTRST, reg + BIT_SET);
-		mdelay(1);
+
+		/* Wait for CLKGATE being set */
+		if (wait_on_timeout(MXS_IP_RESET_TIMEOUT, readl(reg) & MXS_BLOCK_CLKGATE))
+			goto timeout;
 	}
 
 	/* Clear SFTRST */
 	writel(MXS_BLOCK_SFTRST, reg + BIT_CLR);
-	mdelay(1);
+
+	if (wait_on_timeout(MXS_IP_RESET_TIMEOUT, !(readl(reg) & MXS_BLOCK_SFTRST)))
+		goto timeout;
 
 	/* Clear CLKGATE */
 	writel(MXS_BLOCK_CLKGATE, reg + BIT_CLR);
-	mdelay(1);
+
+	if (wait_on_timeout(MXS_IP_RESET_TIMEOUT, !(readl(reg) & MXS_BLOCK_CLKGATE)))
+		goto timeout;
 
 	return 0;
+
+timeout:
+	printf("MXS: Timeout resetting block via register 0x%p\n", reg);
+	return -ETIMEDOUT;
 }
-- 
1.7.10.4


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

[PATCH 2/3] dma: apbh: check for errors when resetting ip core

[Wolfram Sang]

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
---
 drivers/dma/apbh_dma.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/apbh_dma.c b/drivers/dma/apbh_dma.c
index 363878f..d30b8fb 100644
--- a/drivers/dma/apbh_dma.c
+++ b/drivers/dma/apbh_dma.c
@@ -555,7 +555,9 @@ int mxs_dma_init(void)
 	int ret, channel;
 	u32 val, reg;
 
-	mxs_reset_block(apbh_regs, 0);
+	ret = mxs_reset_block(apbh_regs, 0);
+	if (ret)
+		return ret;
 
 	/* HACK: Get CPUID and determine APBH version */
 	val = readl(0x8001c310) >> 16;
-- 
1.7.10.4


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

Re: [PATCH 2/3] dma: apbh: check for errors when resetting ip core

[Juergen Beisert]

Wolfram Sang wrote:
> Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
> ---
>  drivers/dma/apbh_dma.c |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/dma/apbh_dma.c b/drivers/dma/apbh_dma.c
> index 363878f..d30b8fb 100644
> --- a/drivers/dma/apbh_dma.c
> +++ b/drivers/dma/apbh_dma.c
> @@ -555,7 +555,9 @@ int mxs_dma_init(void)
>  	int ret, channel;
>  	u32 val, reg;
>
> -	mxs_reset_block(apbh_regs, 0);
> +	ret = mxs_reset_block(apbh_regs, 0);
> +	if (ret)
> +		return ret;

In this case the user faces a "MXS: Timeout resetting block via register ...". 
Do you think this message is helpful to give the user a pointer *where* the 
failure happens?

jbe

-- 
Pengutronix e.K.                              | Juergen Beisert             |
Linux Solutions for Science and Industry      | http://www.pengutronix.de/  |

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

Re: [PATCH 2/3] dma: apbh: check for errors when resetting ip core

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 1118 bytes --]

On Wed, Oct 31, 2012 at 09:29:56AM +0100, Juergen Beisert wrote:
> Wolfram Sang wrote:
> > Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
> > ---
> >  drivers/dma/apbh_dma.c |    4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/dma/apbh_dma.c b/drivers/dma/apbh_dma.c
> > index 363878f..d30b8fb 100644
> > --- a/drivers/dma/apbh_dma.c
> > +++ b/drivers/dma/apbh_dma.c
> > @@ -555,7 +555,9 @@ int mxs_dma_init(void)
> >  	int ret, channel;
> >  	u32 val, reg;
> >
> > -	mxs_reset_block(apbh_regs, 0);
> > +	ret = mxs_reset_block(apbh_regs, 0);
> > +	if (ret)
> > +		return ret;
> 
> In this case the user faces a "MXS: Timeout resetting block via register ...". 
> Do you think this message is helpful to give the user a pointer *where* the 
> failure happens?

Yes, since it points to the IP core which was used here. Which again,
makes clear which driver was trying to reset the IP core.

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 149 bytes --]

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

Re: [PATCH 2/3] dma: apbh: check for errors when resetting ip core

[Juergen Beisert]

Wolfram Sang wrote:
> On Wed, Oct 31, 2012 at 09:29:56AM +0100, Juergen Beisert wrote:
> > Wolfram Sang wrote:
> > > Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
> > > ---
> > >  drivers/dma/apbh_dma.c |    4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/dma/apbh_dma.c b/drivers/dma/apbh_dma.c
> > > index 363878f..d30b8fb 100644
> > > --- a/drivers/dma/apbh_dma.c
> > > +++ b/drivers/dma/apbh_dma.c
> > > @@ -555,7 +555,9 @@ int mxs_dma_init(void)
> > >  	int ret, channel;
> > >  	u32 val, reg;
> > >
> > > -	mxs_reset_block(apbh_regs, 0);
> > > +	ret = mxs_reset_block(apbh_regs, 0);
> > > +	if (ret)
> > > +		return ret;
> >
> > In this case the user faces a "MXS: Timeout resetting block via register
> > ...". Do you think this message is helpful to give the user a pointer
> > *where* the failure happens?
>
> Yes, since it points to the IP core which was used here. Which again,
> makes clear which driver was trying to reset the IP core.

You mean the reported register offset points to the corresponding IP core?

Just my 2cents: a generic routine should report a failure to the caller. And 
the caller should output a valuable failure message which makes clear what 
the caller had tried to do by calling the generic routine. I think such a 
message would be more helpful. Your generic message is for developers only. 
But also Barebox has more users than developers.

jbe

-- 
Pengutronix e.K.                              | Juergen Beisert             |
Linux Solutions for Science and Industry      | http://www.pengutronix.de/  |

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

Re: [PATCH 2/3] dma: apbh: check for errors when resetting ip core

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 1640 bytes --]

On Wed, Oct 31, 2012 at 09:48:31AM +0100, Juergen Beisert wrote:
> Wolfram Sang wrote:
> > On Wed, Oct 31, 2012 at 09:29:56AM +0100, Juergen Beisert wrote:
> > > Wolfram Sang wrote:
> > > > Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
> > > > ---
> > > >  drivers/dma/apbh_dma.c |    4 +++-
> > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/dma/apbh_dma.c b/drivers/dma/apbh_dma.c
> > > > index 363878f..d30b8fb 100644
> > > > --- a/drivers/dma/apbh_dma.c
> > > > +++ b/drivers/dma/apbh_dma.c
> > > > @@ -555,7 +555,9 @@ int mxs_dma_init(void)
> > > >  	int ret, channel;
> > > >  	u32 val, reg;
> > > >
> > > > -	mxs_reset_block(apbh_regs, 0);
> > > > +	ret = mxs_reset_block(apbh_regs, 0);
> > > > +	if (ret)
> > > > +		return ret;
> > >
> > > In this case the user faces a "MXS: Timeout resetting block via register
> > > ...". Do you think this message is helpful to give the user a pointer
> > > *where* the failure happens?
> >
> > Yes, since it points to the IP core which was used here. Which again,
> > makes clear which driver was trying to reset the IP core.
> 
> You mean the reported register offset points to the corresponding IP core?

Yes, sure. Otherwise the error message would be useless :)

> But also Barebox has more users than developers.

There is nothing a user could do in this case except asking a developer
what could have happened? And that's the best option here.

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 149 bytes --]

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

Re: [PATCH 2/3] dma: apbh: check for errors when resetting ip core

[Juergen Beisert]

Wolfram Sang wrote:
> On Wed, Oct 31, 2012 at 09:48:31AM +0100, Juergen Beisert wrote:
> > Wolfram Sang wrote:
> > > On Wed, Oct 31, 2012 at 09:29:56AM +0100, Juergen Beisert wrote:
> > > > Wolfram Sang wrote:
> > > > > Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
> > > > > ---
> > > > >  drivers/dma/apbh_dma.c |    4 +++-
> > > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/dma/apbh_dma.c b/drivers/dma/apbh_dma.c
> > > > > index 363878f..d30b8fb 100644
> > > > > --- a/drivers/dma/apbh_dma.c
> > > > > +++ b/drivers/dma/apbh_dma.c
> > > > > @@ -555,7 +555,9 @@ int mxs_dma_init(void)
> > > > >  	int ret, channel;
> > > > >  	u32 val, reg;
> > > > >
> > > > > -	mxs_reset_block(apbh_regs, 0);
> > > > > +	ret = mxs_reset_block(apbh_regs, 0);
> > > > > +	if (ret)
> > > > > +		return ret;
> > > >
> > > > In this case the user faces a "MXS: Timeout resetting block via
> > > > register ...". Do you think this message is helpful to give the user
> > > > a pointer *where* the failure happens?
> > >
> > > Yes, since it points to the IP core which was used here. Which again,
> > > makes clear which driver was trying to reset the IP core.
> >
> > You mean the reported register offset points to the corresponding IP
> > core?
>
> Yes, sure. Otherwise the error message would be useless :)
>
> > But also Barebox has more users than developers.
>
> There is nothing a user could do in this case except asking a developer
> what could have happened? And that's the best option here.

"MXS: Timeout resetting block via register <friesel>. Please ask the developer 
for a solution"

:))

jbe

-- 
Pengutronix e.K.                              | Juergen Beisert             |
Linux Solutions for Science and Industry      | http://www.pengutronix.de/  |

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

Re: [PATCH 2/3] dma: apbh: check for errors when resetting ip core

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 823 bytes --]


> > There is nothing a user could do in this case except asking a developer
> > what could have happened? And that's the best option here.
> 
> "MXS: Timeout resetting block via register <friesel>. Please ask the developer 
> for a solution"

"You can do this by writing an email. Further information can be found
here: http://www.wikihow.com/Write-an-Email-to-a-Friend"

Geez, but now we need to explain every browser on every platform...
And that should be applied to every error message, oh noes!

Jürgen, I certainly support your wish to have proper error messages.
Yet, I also think there is a line somewhere to not state too obvious
things?

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 149 bytes --]

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

[PATCH 3/3] mtd: nand: mxs: check for errors when resetting ip core

[Wolfram Sang]

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
---
 drivers/mtd/nand/nand_mxs.c |   12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/nand/nand_mxs.c b/drivers/mtd/nand/nand_mxs.c
index 8aeb14d..906d695 100644
--- a/drivers/mtd/nand/nand_mxs.c
+++ b/drivers/mtd/nand/nand_mxs.c
@@ -1039,9 +1039,13 @@ static int mxs_nand_scan_bbt(struct mtd_info *mtd)
 	struct mxs_nand_info *nand_info = nand->priv;
 	void __iomem *bch_regs = (void __iomem *)MXS_BCH_BASE;
 	uint32_t tmp;
+	int ret;
 
 	/* Reset BCH. Don't use SFTRST on MX23 due to Errata #2847 */
-	mxs_reset_block(bch_regs + BCH_CTRL, nand_info->version == GPMI_VERSION_TYPE_MX23);
+	ret = mxs_reset_block(bch_regs + BCH_CTRL,
+				nand_info->version == GPMI_VERSION_TYPE_MX23);
+	if (ret)
+		return ret;
 
 	/* Configure layout 0 */
 	tmp = (mxs_nand_ecc_chunk_cnt(mtd->writesize) - 1)
@@ -1124,7 +1128,7 @@ int mxs_nand_alloc_buffers(struct mxs_nand_info *nand_info)
 int mxs_nand_hw_init(struct mxs_nand_info *info)
 {
 	void __iomem *gpmi_regs = (void *)MXS_GPMI_BASE;
-	int i = 0;
+	int i = 0, ret;
 	u32 val;
 
 	info->desc = malloc(sizeof(struct mxs_dma_desc *) *
@@ -1145,7 +1149,9 @@ int mxs_nand_hw_init(struct mxs_nand_info *info)
 	imx_enable_nandclk();
 
 	/* Reset the GPMI block. */
-	mxs_reset_block(gpmi_regs + GPMI_CTRL0, 0);
+	ret = mxs_reset_block(gpmi_regs + GPMI_CTRL0, 0);
+	if (ret)
+		return ret;
 
 	/*
 	 * Choose NAND mode, set IRQ polarity, disable write protection and
-- 
1.7.10.4


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

Re: [PATCH 1/3] arm: mxs: use timeouts in block reset routines

[Sascha Hauer]

On Tue, Oct 30, 2012 at 03:21:13PM +0100, Wolfram Sang wrote:
> These routines can fail, add support for that. Also, put in missing
> copyright headers.
> 
> Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>

Applied, thanks

Sascha

> ---
>  arch/arm/mach-mxs/common.c |   39 +++++++++++++++++++++++++++++++++++----
>  1 file changed, 35 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/mach-mxs/common.c b/arch/arm/mach-mxs/common.c
> index 3730633..122d883 100644
> --- a/arch/arm/mach-mxs/common.c
> +++ b/arch/arm/mach-mxs/common.c
> @@ -1,8 +1,26 @@
> +/*
> + * Freescale i.MXS common code
> + *
> + * Copyright (C) 2012 Wolfram Sang <w.sang@pengutronix.de>
> + *
> + * Based on code from LTIB:
> + * Copyright (C) 2010 Freescale Semiconductor, Inc.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + */
> +
>  #include <common.h>
>  #include <io.h>
> +#include <errno.h>
> +#include <clock.h>
>  #include <mach/mxs.h>
>  #include <mach/imx-regs.h>
>  
> +#define	MXS_IP_RESET_TIMEOUT	(10 * MSECOND)
> +
>  #define	MXS_BLOCK_SFTRST				(1 << 31)
>  #define	MXS_BLOCK_CLKGATE				(1 << 30)
>  
> @@ -10,7 +28,9 @@ int mxs_reset_block(void __iomem *reg, int just_enable)
>  {
>  	/* Clear SFTRST */
>  	writel(MXS_BLOCK_SFTRST, reg + BIT_CLR);
> -	mdelay(1);
> +
> +	if (wait_on_timeout(MXS_IP_RESET_TIMEOUT, !(readl(reg) & MXS_BLOCK_SFTRST)))
> +		goto timeout;
>  
>  	/* Clear CLKGATE */
>  	writel(MXS_BLOCK_CLKGATE, reg + BIT_CLR);
> @@ -18,16 +38,27 @@ int mxs_reset_block(void __iomem *reg, int just_enable)
>  	if (!just_enable) {
>  		/* Set SFTRST */
>  		writel(MXS_BLOCK_SFTRST, reg + BIT_SET);
> -		mdelay(1);
> +
> +		/* Wait for CLKGATE being set */
> +		if (wait_on_timeout(MXS_IP_RESET_TIMEOUT, readl(reg) & MXS_BLOCK_CLKGATE))
> +			goto timeout;
>  	}
>  
>  	/* Clear SFTRST */
>  	writel(MXS_BLOCK_SFTRST, reg + BIT_CLR);
> -	mdelay(1);
> +
> +	if (wait_on_timeout(MXS_IP_RESET_TIMEOUT, !(readl(reg) & MXS_BLOCK_SFTRST)))
> +		goto timeout;
>  
>  	/* Clear CLKGATE */
>  	writel(MXS_BLOCK_CLKGATE, reg + BIT_CLR);
> -	mdelay(1);
> +
> +	if (wait_on_timeout(MXS_IP_RESET_TIMEOUT, !(readl(reg) & MXS_BLOCK_CLKGATE)))
> +		goto timeout;
>  
>  	return 0;
> +
> +timeout:
> +	printf("MXS: Timeout resetting block via register 0x%p\n", reg);
> +	return -ETIMEDOUT;
>  }
> -- 
> 1.7.10.4
> 
> 
> _______________________________________________
> 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