| Message ID | 20180806092137.9287-3-kurt@linutronix.de |
|---|---|
| State | Changes Requested |
| Delegated to: | Miquel Raynal |
| Headers | show |
| Series | mtd: nand: fsl-ifc: fix SRAM initialization for newer controller | expand |
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
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
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
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
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 */
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(+)