[PATCH 0/2] mtd: nand: fsl-ifc: fix SRAM initialization for newer controller

[PATCH 0/2] mtd: nand: fsl-ifc: fix SRAM initialization for newer controller

[Kurt Kanzenbach]

Hi,

the current way of initializing the internal SRAM of the IFC controller only
works for older controller versions. Newer versions require a different
method. So, adding support for it.

Thereby, the result of the SRAM initialization should be checked. If it's not
successful, further commands such as read won't work.

Tested on hardware.

Thanks,
Kurt

Kurt Kanzenbach (2):
  mtd: nand: fsl-ifc: check result of SRAM initialization
  mtd: nand: fsl-ifc: fixup SRAM init for newer ctrl versions

 drivers/mtd/nand/raw/fsl_ifc_nand.c | 35 +++++++++++++++++++++++++++++++----
 include/linux/fsl_ifc.h             |  2 ++
 2 files changed, 33 insertions(+), 4 deletions(-)

-- 
2.11.0

[PATCH 1/2] mtd: nand: fsl-ifc: check result of SRAM initialization

[Kurt Kanzenbach]

The SRAM initialization might fail. If that happens further NAND operations
won't be successful. Therefore, the chip init routine should fail if the SRAM
initialization didn't work.

Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
---
 drivers/mtd/nand/raw/fsl_ifc_nand.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/nand/raw/fsl_ifc_nand.c b/drivers/mtd/nand/raw/fsl_ifc_nand.c
index 24f59d0066af..e4f5792dc589 100644
--- a/drivers/mtd/nand/raw/fsl_ifc_nand.c
+++ b/drivers/mtd/nand/raw/fsl_ifc_nand.c
@@ -761,7 +761,7 @@ static const struct nand_controller_ops fsl_ifc_controller_ops = {
 	.attach_chip = fsl_ifc_attach_chip,
 };
 
-static void fsl_ifc_sram_init(struct fsl_ifc_mtd *priv)
+static int fsl_ifc_sram_init(struct fsl_ifc_mtd *priv)
 {
 	struct fsl_ifc_ctrl *ctrl = priv->ctrl;
 	struct fsl_ifc_runtime __iomem *ifc_runtime = ctrl->rregs;
@@ -805,12 +805,16 @@ static void fsl_ifc_sram_init(struct fsl_ifc_mtd *priv)
 	wait_event_timeout(ctrl->nand_wait, ctrl->nand_stat,
 			   msecs_to_jiffies(IFC_TIMEOUT_MSECS));
 
-	if (ctrl->nand_stat != IFC_NAND_EVTER_STAT_OPC)
+	if (ctrl->nand_stat != IFC_NAND_EVTER_STAT_OPC) {
 		pr_err("fsl-ifc: Failed to Initialise SRAM\n");
+		return -ETIMEDOUT;
+	}
 
 	/* Restore CSOR and CSOR_ext */
 	ifc_out32(csor, &ifc_global->csor_cs[cs].csor);
 	ifc_out32(csor_ext, &ifc_global->csor_cs[cs].csor_ext);
+
+	return 0;
 }
 
 static int fsl_ifc_chip_init(struct fsl_ifc_mtd *priv)
@@ -914,8 +918,13 @@ static int fsl_ifc_chip_init(struct fsl_ifc_mtd *priv)
 		chip->ecc.algo = NAND_ECC_HAMMING;
 	}
 
-	if (ctrl->version >= FSL_IFC_VERSION_1_1_0)
-		fsl_ifc_sram_init(priv);
+	if (ctrl->version >= FSL_IFC_VERSION_1_1_0) {
+		int ret;
+
+		ret = fsl_ifc_sram_init(priv);
+		if (ret)
+			return ret;
+	}
 
 	/*
 	 * As IFC version 2.0.0 has 16KB of internal SRAM as compared to older
-- 
2.11.0

[PATCH 2/2] mtd: nand: fsl-ifc: fixup SRAM init for newer ctrl versions

[Kurt Kanzenbach]

Newer versions of the IFC controller use a different method of initializing the
internal SRAM: Instead of reading from flash, a bit in the NAND configuration
register has to be set in order to trigger the self-initializing process.

Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
---
 drivers/mtd/nand/raw/fsl_ifc_nand.c | 18 ++++++++++++++++++
 include/linux/fsl_ifc.h             |  2 ++
 2 files changed, 20 insertions(+)

diff --git a/drivers/mtd/nand/raw/fsl_ifc_nand.c b/drivers/mtd/nand/raw/fsl_ifc_nand.c
index e4f5792dc589..384d5e12b05c 100644
--- a/drivers/mtd/nand/raw/fsl_ifc_nand.c
+++ b/drivers/mtd/nand/raw/fsl_ifc_nand.c
@@ -30,6 +30,7 @@
 #include <linux/mtd/partitions.h>
 #include <linux/mtd/nand_ecc.h>
 #include <linux/fsl_ifc.h>
+#include <linux/iopoll.h>
 
 #define ERR_BYTE		0xFF /* Value returned for read
 					bytes when read failed	*/
@@ -769,6 +770,23 @@ static int fsl_ifc_sram_init(struct fsl_ifc_mtd *priv)
 	uint32_t csor = 0, csor_8k = 0, csor_ext = 0;
 	uint32_t cs = priv->bank;
 
+	if (ctrl->version > FSL_IFC_VERSION_1_1_0) {
+		u32 ncfgr, status;
+		int ret;
+
+		/* Trigger auto initialization */
+		ncfgr = ifc_in32(&ifc_runtime->ifc_nand.ncfgr);
+		ifc_out32(ncfgr | IFC_NAND_NCFGR_SRAM_INIT_EN, &ifc_runtime->ifc_nand.ncfgr);
+
+		/* Wait until done */
+		ret = readx_poll_timeout(ifc_in32, &ifc_runtime->ifc_nand.ncfgr,
+					 status, !(status & IFC_NAND_NCFGR_SRAM_INIT_EN),
+					 10, 1000);
+		if (ret)
+			dev_err(priv->dev, "Failed to initialize SRAM!\n");
+		return ret;
+	}
+
 	/* Save CSOR and CSOR_ext */
 	csor = ifc_in32(&ifc_global->csor_cs[cs].csor);
 	csor_ext = ifc_in32(&ifc_global->csor_cs[cs].csor_ext);
diff --git a/include/linux/fsl_ifc.h b/include/linux/fsl_ifc.h
index 3fdfede2f0f3..5f343b796ad9 100644
--- a/include/linux/fsl_ifc.h
+++ b/include/linux/fsl_ifc.h
@@ -274,6 +274,8 @@
  */
 /* Auto Boot Mode */
 #define IFC_NAND_NCFGR_BOOT		0x80000000
+/* SRAM Initialization */
+#define IFC_NAND_NCFGR_SRAM_INIT_EN	0x20000000
 /* Addressing Mode-ROW0+n/COL0 */
 #define IFC_NAND_NCFGR_ADDR_MODE_RC0	0x00000000
 /* Addressing Mode-ROW0+n/COL0+n */
-- 
2.11.0

Re: [PATCH 2/2] mtd: nand: fsl-ifc: fixup SRAM init for newer ctrl versions

[Miquel Raynal]

Hi Kurt,

Subject prefix should be "mtd: rawnand: fsl_ifc:".

Kurt Kanzenbach <kurt@linutronix.de> wrote on Mon,  6 Aug 2018 11:21:37
+0200:

> Newer versions of the IFC controller use a different method of initializing the
> internal SRAM: Instead of reading from flash, a bit in the NAND configuration
> register has to be set in order to trigger the self-initializing process.
> 
> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
> ---
>  drivers/mtd/nand/raw/fsl_ifc_nand.c | 18 ++++++++++++++++++
>  include/linux/fsl_ifc.h             |  2 ++
>  2 files changed, 20 insertions(+)
> 
> diff --git a/drivers/mtd/nand/raw/fsl_ifc_nand.c b/drivers/mtd/nand/raw/fsl_ifc_nand.c
> index e4f5792dc589..384d5e12b05c 100644
> --- a/drivers/mtd/nand/raw/fsl_ifc_nand.c
> +++ b/drivers/mtd/nand/raw/fsl_ifc_nand.c
> @@ -30,6 +30,7 @@
>  #include <linux/mtd/partitions.h>
>  #include <linux/mtd/nand_ecc.h>
>  #include <linux/fsl_ifc.h>
> +#include <linux/iopoll.h>
>  
>  #define ERR_BYTE		0xFF /* Value returned for read
>  					bytes when read failed	*/
> @@ -769,6 +770,23 @@ static int fsl_ifc_sram_init(struct fsl_ifc_mtd *priv)
>  	uint32_t csor = 0, csor_8k = 0, csor_ext = 0;
>  	uint32_t cs = priv->bank;
>  
> +	if (ctrl->version > FSL_IFC_VERSION_1_1_0) {

This is redundant and fsl_ifc_sram_init() is called only if
"ctrl->version > FSL_FC_VERSION_1_1_0".

So this means this function has never worked?

If this is the case, there should be at least a Fixes: tag.

Maybe it would be cleaner to always call fsl_ifc_sram_init() from the
probe(), and just exit with a "return 0" here if the version is old?
(I'll let you choose the way you prefer).

> +		u32 ncfgr, status;
> +		int ret;
> +
> +		/* Trigger auto initialization */
> +		ncfgr = ifc_in32(&ifc_runtime->ifc_nand.ncfgr);
> +		ifc_out32(ncfgr | IFC_NAND_NCFGR_SRAM_INIT_EN, &ifc_runtime->ifc_nand.ncfgr);
> +
> +		/* Wait until done */
> +		ret = readx_poll_timeout(ifc_in32, &ifc_runtime->ifc_nand.ncfgr,
> +					 status, !(status & IFC_NAND_NCFGR_SRAM_INIT_EN),
> +					 10, 1000);

Nit: I always prefer when delays/timeouts are defined (and may be
reused).

> +		if (ret)
> +			dev_err(priv->dev, "Failed to initialize SRAM!\n");

Space

> +		return ret;
> +	}
> +
>  	/* Save CSOR and CSOR_ext */
>  	csor = ifc_in32(&ifc_global->csor_cs[cs].csor);
>  	csor_ext = ifc_in32(&ifc_global->csor_cs[cs].csor_ext);
> diff --git a/include/linux/fsl_ifc.h b/include/linux/fsl_ifc.h
> index 3fdfede2f0f3..5f343b796ad9 100644
> --- a/include/linux/fsl_ifc.h
> +++ b/include/linux/fsl_ifc.h
> @@ -274,6 +274,8 @@
>   */
>  /* Auto Boot Mode */
>  #define IFC_NAND_NCFGR_BOOT		0x80000000
> +/* SRAM Initialization */
> +#define IFC_NAND_NCFGR_SRAM_INIT_EN	0x20000000
>  /* Addressing Mode-ROW0+n/COL0 */
>  #define IFC_NAND_NCFGR_ADDR_MODE_RC0	0x00000000
>  /* Addressing Mode-ROW0+n/COL0+n */


Thanks,
Miquèl

Re: [PATCH 2/2] mtd: nand: fsl-ifc: fixup SRAM init for newer ctrl versions

[Kurt Kanzenbach]

Hi Miquel,

On Wed, Aug 08, 2018 at 11:48:32AM +0200, Miquel Raynal wrote:
> Hi Kurt,
>
> Subject prefix should be "mtd: rawnand: fsl_ifc:".

okay, noted.

>
> Kurt Kanzenbach <kurt@linutronix.de> wrote on Mon,  6 Aug 2018 11:21:37
> +0200:
>
> > Newer versions of the IFC controller use a different method of initializing the
> > internal SRAM: Instead of reading from flash, a bit in the NAND configuration
> > register has to be set in order to trigger the self-initializing process.
> >
> > Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
> > ---
> >  drivers/mtd/nand/raw/fsl_ifc_nand.c | 18 ++++++++++++++++++
> >  include/linux/fsl_ifc.h             |  2 ++
> >  2 files changed, 20 insertions(+)
> >
> > diff --git a/drivers/mtd/nand/raw/fsl_ifc_nand.c b/drivers/mtd/nand/raw/fsl_ifc_nand.c
> > index e4f5792dc589..384d5e12b05c 100644
> > --- a/drivers/mtd/nand/raw/fsl_ifc_nand.c
> > +++ b/drivers/mtd/nand/raw/fsl_ifc_nand.c
> > @@ -30,6 +30,7 @@
> >  #include <linux/mtd/partitions.h>
> >  #include <linux/mtd/nand_ecc.h>
> >  #include <linux/fsl_ifc.h>
> > +#include <linux/iopoll.h>
> >
> >  #define ERR_BYTE		0xFF /* Value returned for read
> >  					bytes when read failed	*/
> > @@ -769,6 +770,23 @@ static int fsl_ifc_sram_init(struct fsl_ifc_mtd *priv)
> >  	uint32_t csor = 0, csor_8k = 0, csor_ext = 0;
> >  	uint32_t cs = priv->bank;
> >
> > +	if (ctrl->version > FSL_IFC_VERSION_1_1_0) {
>
> This is redundant and fsl_ifc_sram_init() is called only if
> "ctrl->version > FSL_FC_VERSION_1_1_0".

No, it's not. It's called when ctrl->version >=
FSL_IFC_VERSION_1_1_0. Therefore, this check is needed.

>
> So this means this function has never worked?

It did work for e.g. IFC controller in version 1.1.0.

However, it worked for the newer versions by accident, because U-Boot
already initialized the SRAM correctly. If you boot without NAND
initialization in U-Boot, then you'll hit the issue.

>
> If this is the case, there should be at least a Fixes: tag.
>
> Maybe it would be cleaner to always call fsl_ifc_sram_init() from the
> probe(), and just exit with a "return 0" here if the version is old?
> (I'll let you choose the way you prefer).

Sounds like a good idea. Otherwise we have to check the version twice.

>
> > +		u32 ncfgr, status;
> > +		int ret;
> > +
> > +		/* Trigger auto initialization */
> > +		ncfgr = ifc_in32(&ifc_runtime->ifc_nand.ncfgr);
> > +		ifc_out32(ncfgr | IFC_NAND_NCFGR_SRAM_INIT_EN, &ifc_runtime->ifc_nand.ncfgr);
> > +
> > +		/* Wait until done */
> > +		ret = readx_poll_timeout(ifc_in32, &ifc_runtime->ifc_nand.ncfgr,
> > +					 status, !(status & IFC_NAND_NCFGR_SRAM_INIT_EN),
> > +					 10, 1000);
>
> Nit: I always prefer when delays/timeouts are defined (and may be
> reused).

Me too. I've missed that there is already a timeout constant
IFC_TIMEOUT_MSECS (500). As it's huge, I'll add a second one.

>
> > +		if (ret)
> > +			dev_err(priv->dev, "Failed to initialize SRAM!\n");
>
> Space

okay.

Thanks,
Kurt

>
> > +		return ret;
> > +	}
> > +
> >  	/* Save CSOR and CSOR_ext */
> >  	csor = ifc_in32(&ifc_global->csor_cs[cs].csor);
> >  	csor_ext = ifc_in32(&ifc_global->csor_cs[cs].csor_ext);
> > diff --git a/include/linux/fsl_ifc.h b/include/linux/fsl_ifc.h
> > index 3fdfede2f0f3..5f343b796ad9 100644
> > --- a/include/linux/fsl_ifc.h
> > +++ b/include/linux/fsl_ifc.h
> > @@ -274,6 +274,8 @@
> >   */
> >  /* Auto Boot Mode */
> >  #define IFC_NAND_NCFGR_BOOT		0x80000000
> > +/* SRAM Initialization */
> > +#define IFC_NAND_NCFGR_SRAM_INIT_EN	0x20000000
> >  /* Addressing Mode-ROW0+n/COL0 */
> >  #define IFC_NAND_NCFGR_ADDR_MODE_RC0	0x00000000
> >  /* Addressing Mode-ROW0+n/COL0+n */
>
>
> Thanks,
> Miquèl

Re: [PATCH 2/2] mtd: nand: fsl-ifc: fixup SRAM init for newer ctrl versions

[Miquel Raynal]

Hi Kurt,


> > > @@ -769,6 +770,23 @@ static int fsl_ifc_sram_init(struct fsl_ifc_mtd *priv)
> > >  	uint32_t csor = 0, csor_8k = 0, csor_ext = 0;
> > >  	uint32_t cs = priv->bank;
> > >
> > > +	if (ctrl->version > FSL_IFC_VERSION_1_1_0) {  
> >
> > This is redundant and fsl_ifc_sram_init() is called only if
> > "ctrl->version > FSL_FC_VERSION_1_1_0".  
> 
> No, it's not. It's called when ctrl->version >=
> FSL_IFC_VERSION_1_1_0. Therefore, this check is needed.

Oh right, I missed the "=".

> 
> >
> > So this means this function has never worked?  
> 
> It did work for e.g. IFC controller in version 1.1.0.
> 
> However, it worked for the newer versions by accident, because U-Boot
> already initialized the SRAM correctly. If you boot without NAND
> initialization in U-Boot, then you'll hit the issue.
> 
> >
> > If this is the case, there should be at least a Fixes: tag.
> >
> > Maybe it would be cleaner to always call fsl_ifc_sram_init() from the
> > probe(), and just exit with a "return 0" here if the version is old?
> > (I'll let you choose the way you prefer).  
> 
> Sounds like a good idea. Otherwise we have to check the version twice.
> 
> >  
> > > +		u32 ncfgr, status;
> > > +		int ret;
> > > +
> > > +		/* Trigger auto initialization */
> > > +		ncfgr = ifc_in32(&ifc_runtime->ifc_nand.ncfgr);
> > > +		ifc_out32(ncfgr | IFC_NAND_NCFGR_SRAM_INIT_EN, &ifc_runtime->ifc_nand.ncfgr);
> > > +
> > > +		/* Wait until done */
> > > +		ret = readx_poll_timeout(ifc_in32, &ifc_runtime->ifc_nand.ncfgr,
> > > +					 status, !(status & IFC_NAND_NCFGR_SRAM_INIT_EN),
> > > +					 10, 1000);  
> >
> > Nit: I always prefer when delays/timeouts are defined (and may be
> > reused).  
> 
> Me too. I've missed that there is already a timeout constant
> IFC_TIMEOUT_MSECS (500). As it's huge, I'll add a second one.

Well, I'm not bothered with huge timeouts, it's in the error path so we
don't really care.

Thanks,
Miquèl

Re: [PATCH 2/2] mtd: nand: fsl-ifc: fixup SRAM init for newer ctrl versions

[Kurt Kanzenbach]

Hi Miquel,

On Wed, Aug 08, 2018 at 02:33:52PM +0200, Miquel Raynal wrote:
> Hi Kurt,
>
>
> > > > +		u32 ncfgr, status;
> > > > +		int ret;
> > > > +
> > > > +		/* Trigger auto initialization */
> > > > +		ncfgr = ifc_in32(&ifc_runtime->ifc_nand.ncfgr);
> > > > +		ifc_out32(ncfgr | IFC_NAND_NCFGR_SRAM_INIT_EN, &ifc_runtime->ifc_nand.ncfgr);
> > > > +
> > > > +		/* Wait until done */
> > > > +		ret = readx_poll_timeout(ifc_in32, &ifc_runtime->ifc_nand.ncfgr,
> > > > +					 status, !(status & IFC_NAND_NCFGR_SRAM_INIT_EN),
> > > > +					 10, 1000);
> > >
> > > Nit: I always prefer when delays/timeouts are defined (and may be
> > > reused).
> >
> > Me too. I've missed that there is already a timeout constant
> > IFC_TIMEOUT_MSECS (500). As it's huge, I'll add a second one.
>
> Well, I'm not bothered with huge timeouts, it's in the error path so we
> don't really care.

okay. I'll send a v2 next week addressing your comments.

Thanks,
Kurt