[PATCH v2] fsl_ifc: Support all 8 IFC chip selects

[PATCH v2] fsl_ifc: Support all 8 IFC chip selects

[Aaron Sierra]

Freescale's QorIQ T Series processors support 8 IFC chip selects
within a memory map backward compatible with previous P Series
processors which supported only 4 chip selects.

Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
---
 drivers/memory/fsl_ifc.c        |  2 +-
 drivers/mtd/nand/fsl_ifc_nand.c | 17 ++++++++++-------
 include/linux/fsl_ifc.h         | 34 +++++++++++++++++++++++++---------
 3 files changed, 36 insertions(+), 17 deletions(-)

diff --git a/drivers/memory/fsl_ifc.c b/drivers/memory/fsl_ifc.c
index 3d5d792..a539dc2 100644
--- a/drivers/memory/fsl_ifc.c
+++ b/drivers/memory/fsl_ifc.c
@@ -61,7 +61,7 @@ int fsl_ifc_find(phys_addr_t addr_base)
 	if (!fsl_ifc_ctrl_dev || !fsl_ifc_ctrl_dev->regs)
 		return -ENODEV;
 
-	for (i = 0; i < ARRAY_SIZE(fsl_ifc_ctrl_dev->regs->cspr_cs); i++) {
+	for (i = 0; i < fsl_ifc_bank_count(fsl_ifc_ctrl_dev->regs); i++) {
 		u32 cspr = in_be32(&fsl_ifc_ctrl_dev->regs->cspr_cs[i].cspr);
 		if (cspr & CSPR_V && (cspr & CSPR_BA) ==
 				convert_ifc_address(addr_base))
diff --git a/drivers/mtd/nand/fsl_ifc_nand.c b/drivers/mtd/nand/fsl_ifc_nand.c
index 2338124..f7b7077 100644
--- a/drivers/mtd/nand/fsl_ifc_nand.c
+++ b/drivers/mtd/nand/fsl_ifc_nand.c
@@ -31,7 +31,6 @@
 #include <linux/mtd/nand_ecc.h>
 #include <linux/fsl_ifc.h>
 
-#define FSL_IFC_V1_1_0	0x01010000
 #define ERR_BYTE		0xFF /* Value returned for read
 					bytes when read failed	*/
 #define IFC_TIMEOUT_MSECS	500  /* Maximum number of mSecs to wait
@@ -54,7 +53,7 @@ struct fsl_ifc_mtd {
 /* overview of the fsl ifc controller */
 struct fsl_ifc_nand_ctrl {
 	struct nand_hw_control controller;
-	struct fsl_ifc_mtd *chips[FSL_IFC_BANK_COUNT];
+	struct fsl_ifc_mtd *chips[FSL_IFC_BANK_COUNT_MAX];
 
 	void __iomem *addr;	/* Address of assigned IFC buffer	*/
 	unsigned int page;	/* Last page written to / read from	*/
@@ -877,7 +876,7 @@ static int fsl_ifc_chip_init(struct fsl_ifc_mtd *priv)
 	struct fsl_ifc_regs __iomem *ifc = ctrl->regs;
 	struct nand_chip *chip = &priv->chip;
 	struct nand_ecclayout *layout;
-	u32 csor, ver;
+	u32 csor;
 
 	/* Fill in fsl_ifc_mtd structure */
 	priv->mtd.priv = chip;
@@ -984,8 +983,7 @@ static int fsl_ifc_chip_init(struct fsl_ifc_mtd *priv)
 		chip->ecc.mode = NAND_ECC_SOFT;
 	}
 
-	ver = ioread32be(&ifc->ifc_rev);
-	if (ver == FSL_IFC_V1_1_0)
+	if (fsl_ifc_version(ifc) == FSL_IFC_VERSION_1_1_0)
 		fsl_ifc_sram_init(priv);
 
 	return 0;
@@ -1044,13 +1042,18 @@ static int fsl_ifc_nand_probe(struct platform_device *dev)
 		return ret;
 	}
 
+	dev_info(&dev->dev, "IFC version %d.%d, %d banks\n",
+		fsl_ifc_version(ifc) >> 24,
+		(fsl_ifc_version(ifc) >> 16) & 0xf,
+		fsl_ifc_bank_count(ifc));
+
 	/* find which chip select it is connected to */
-	for (bank = 0; bank < FSL_IFC_BANK_COUNT; bank++) {
+	for (bank = 0; bank < fsl_ifc_bank_count(ifc); bank++) {
 		if (match_bank(ifc, bank, res.start))
 			break;
 	}
 
-	if (bank >= FSL_IFC_BANK_COUNT) {
+	if (bank >= fsl_ifc_bank_count(ifc)) {
 		dev_err(&dev->dev, "%s: address did not match any chip selects\n",
 			__func__);
 		return -ENODEV;
diff --git a/include/linux/fsl_ifc.h b/include/linux/fsl_ifc.h
index 84d60cb..7a92773 100644
--- a/include/linux/fsl_ifc.h
+++ b/include/linux/fsl_ifc.h
@@ -29,7 +29,16 @@
 #include <linux/of_platform.h>
 #include <linux/interrupt.h>
 
-#define FSL_IFC_BANK_COUNT 4
+/*
+ * The actual number of banks implemented depends on the IFC version
+ *    - IFC version 1.0 implements 4 banks.
+ *    - IFC version 1.1 onward implements 8 banks.
+ */
+#define FSL_IFC_BANK_COUNT_MAX 8
+
+#define FSL_IFC_VERSION_MASK	0x0F0F0000
+#define FSL_IFC_VERSION_1_0_0	0x01000000
+#define FSL_IFC_VERSION_1_1_0	0x01010000
 
 /*
  * CSPR - Chip Select Property Register
@@ -775,24 +784,24 @@ struct fsl_ifc_regs {
 		__be32 cspr_ext;
 		__be32 cspr;
 		u32 res2;
-	} cspr_cs[FSL_IFC_BANK_COUNT];
-	u32 res3[0x19];
+	} cspr_cs[FSL_IFC_BANK_COUNT_MAX];
+	u32 res3[0xd];
 	struct {
 		__be32 amask;
 		u32 res4[0x2];
-	} amask_cs[FSL_IFC_BANK_COUNT];
-	u32 res5[0x18];
+	} amask_cs[FSL_IFC_BANK_COUNT_MAX];
+	u32 res5[0xc];
 	struct {
 		__be32 csor;
 		__be32 csor_ext;
 		u32 res6;
-	} csor_cs[FSL_IFC_BANK_COUNT];
-	u32 res7[0x18];
+	} csor_cs[FSL_IFC_BANK_COUNT_MAX];
+	u32 res7[0xc];
 	struct {
 		__be32 ftim[4];
 		u32 res8[0x8];
-	} ftim_cs[FSL_IFC_BANK_COUNT];
-	u32 res9[0x60];
+	} ftim_cs[FSL_IFC_BANK_COUNT_MAX];
+	u32 res9[0x30];
 	__be32 rb_stat;
 	u32 res10[0x2];
 	__be32 ifc_gcr;
@@ -834,5 +843,12 @@ struct fsl_ifc_ctrl {
 
 extern struct fsl_ifc_ctrl *fsl_ifc_ctrl_dev;
 
+static inline u32 fsl_ifc_version(struct fsl_ifc_regs *regs) {
+	return ioread32be(&regs->ifc_rev) & FSL_IFC_VERSION_MASK;
+}
+
+static inline int fsl_ifc_bank_count(struct fsl_ifc_regs *regs) {
+	return (fsl_ifc_version(regs) == FSL_IFC_VERSION_1_0_0) ? 4 : 8;
+}
 
 #endif /* __ASM_FSL_IFC_H */
-- 
1.9.1

Re: [PATCH v2] fsl_ifc: Support all 8 IFC chip selects

[Scott Wood]

On Tue, 2014-08-26 at 12:31 -0500, Aaron Sierra wrote:
> Freescale's QorIQ T Series processors support 8 IFC chip selects
> within a memory map backward compatible with previous P Series
> processors which supported only 4 chip selects.
> 
> Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
> ---
>  drivers/memory/fsl_ifc.c        |  2 +-
>  drivers/mtd/nand/fsl_ifc_nand.c | 17 ++++++++++-------
>  include/linux/fsl_ifc.h         | 34 +++++++++++++++++++++++++---------
>  3 files changed, 36 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/memory/fsl_ifc.c b/drivers/memory/fsl_ifc.c
> index 3d5d792..a539dc2 100644
> --- a/drivers/memory/fsl_ifc.c
> +++ b/drivers/memory/fsl_ifc.c
> @@ -61,7 +61,7 @@ int fsl_ifc_find(phys_addr_t addr_base)
>  	if (!fsl_ifc_ctrl_dev || !fsl_ifc_ctrl_dev->regs)
>  		return -ENODEV;
>  
> -	for (i = 0; i < ARRAY_SIZE(fsl_ifc_ctrl_dev->regs->cspr_cs); i++) {
> +	for (i = 0; i < fsl_ifc_bank_count(fsl_ifc_ctrl_dev->regs); i++) {
>  		u32 cspr = in_be32(&fsl_ifc_ctrl_dev->regs->cspr_cs[i].cspr);
>  		if (cspr & CSPR_V && (cspr & CSPR_BA) ==
>  				convert_ifc_address(addr_base))
> diff --git a/drivers/mtd/nand/fsl_ifc_nand.c b/drivers/mtd/nand/fsl_ifc_nand.c
> index 2338124..f7b7077 100644
> --- a/drivers/mtd/nand/fsl_ifc_nand.c
> +++ b/drivers/mtd/nand/fsl_ifc_nand.c
> @@ -31,7 +31,6 @@
>  #include <linux/mtd/nand_ecc.h>
>  #include <linux/fsl_ifc.h>
>  
> -#define FSL_IFC_V1_1_0	0x01010000
>  #define ERR_BYTE		0xFF /* Value returned for read
>  					bytes when read failed	*/
>  #define IFC_TIMEOUT_MSECS	500  /* Maximum number of mSecs to wait
> @@ -54,7 +53,7 @@ struct fsl_ifc_mtd {
>  /* overview of the fsl ifc controller */
>  struct fsl_ifc_nand_ctrl {
>  	struct nand_hw_control controller;
> -	struct fsl_ifc_mtd *chips[FSL_IFC_BANK_COUNT];
> +	struct fsl_ifc_mtd *chips[FSL_IFC_BANK_COUNT_MAX];

FSL_IFC_MAX_BANKS would be more concise.  I'm not sure we really need to
rename this, though.

> @@ -834,5 +843,12 @@ struct fsl_ifc_ctrl {
>  
>  extern struct fsl_ifc_ctrl *fsl_ifc_ctrl_dev;
>  
> +static inline u32 fsl_ifc_version(struct fsl_ifc_regs *regs) {
> +	return ioread32be(&regs->ifc_rev) & FSL_IFC_VERSION_MASK;
> +}
> +
> +static inline int fsl_ifc_bank_count(struct fsl_ifc_regs *regs) {
> +	return (fsl_ifc_version(regs) == FSL_IFC_VERSION_1_0_0) ? 4 : 8;
> +}

Whitespace

Do we really need the bank count here, as opposed to just checking it in
probe()?  I also don't really care for reading the registers over and
over, even though it's not performance critical.

The reserved bits of the version register are defined as zero for
current versions -- I think just comparing ifc_rev to the version
constant, as is currently done, is fine.

Also, please send the patch to the mtd list and maintainer.

-Scott

Re: [PATCH v2] fsl_ifc: Support all 8 IFC chip selects

[Aaron Sierra]

----- Original Message -----
> From: "Scott Wood" <scottwood@freescale.com>
> Sent: Tuesday, August 26, 2014 3:48:51 PM
> 
> On Tue, 2014-08-26 at 12:31 -0500, Aaron Sierra wrote:
> > Freescale's QorIQ T Series processors support 8 IFC chip selects
> > within a memory map backward compatible with previous P Series
> > processors which supported only 4 chip selects.
> > 
> > Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
> > ---
> >  drivers/memory/fsl_ifc.c        |  2 +-
> >  drivers/mtd/nand/fsl_ifc_nand.c | 17 ++++++++++-------
> >  include/linux/fsl_ifc.h         | 34 +++++++++++++++++++++++++---------
> >  3 files changed, 36 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/memory/fsl_ifc.c b/drivers/memory/fsl_ifc.c
> > index 3d5d792..a539dc2 100644
> > --- a/drivers/memory/fsl_ifc.c
> > +++ b/drivers/memory/fsl_ifc.c
> > @@ -61,7 +61,7 @@ int fsl_ifc_find(phys_addr_t addr_base)
> >  	if (!fsl_ifc_ctrl_dev || !fsl_ifc_ctrl_dev->regs)
> >  		return -ENODEV;
> >  
> > -	for (i = 0; i < ARRAY_SIZE(fsl_ifc_ctrl_dev->regs->cspr_cs); i++) {
> > +	for (i = 0; i < fsl_ifc_bank_count(fsl_ifc_ctrl_dev->regs); i++) {
> >  		u32 cspr = in_be32(&fsl_ifc_ctrl_dev->regs->cspr_cs[i].cspr);
> >  		if (cspr & CSPR_V && (cspr & CSPR_BA) ==
> >  				convert_ifc_address(addr_base))
> > diff --git a/drivers/mtd/nand/fsl_ifc_nand.c
> > b/drivers/mtd/nand/fsl_ifc_nand.c
> > index 2338124..f7b7077 100644
> > --- a/drivers/mtd/nand/fsl_ifc_nand.c
> > +++ b/drivers/mtd/nand/fsl_ifc_nand.c
> > @@ -31,7 +31,6 @@
> >  #include <linux/mtd/nand_ecc.h>
> >  #include <linux/fsl_ifc.h>
> >  
> > -#define FSL_IFC_V1_1_0	0x01010000
> >  #define ERR_BYTE		0xFF /* Value returned for read
> >  					bytes when read failed	*/
> >  #define IFC_TIMEOUT_MSECS	500  /* Maximum number of mSecs to wait
> > @@ -54,7 +53,7 @@ struct fsl_ifc_mtd {
> >  /* overview of the fsl ifc controller */
> >  struct fsl_ifc_nand_ctrl {
> >  	struct nand_hw_control controller;
> > -	struct fsl_ifc_mtd *chips[FSL_IFC_BANK_COUNT];
> > +	struct fsl_ifc_mtd *chips[FSL_IFC_BANK_COUNT_MAX];
> 
> FSL_IFC_MAX_BANKS would be more concise.  I'm not sure we really need to
> rename this, though.

I renamed it to be sure that I found all of the places it was used and I
wanted to make clear that the FSL_IFC_BANK_COUNT doesn't refer to the
implemented number of banks. I agree that with the comment immediately above
the definition, renaming the define is redundant.

> > @@ -834,5 +843,12 @@ struct fsl_ifc_ctrl {
> >  
> >  extern struct fsl_ifc_ctrl *fsl_ifc_ctrl_dev;
> >  
> > +static inline u32 fsl_ifc_version(struct fsl_ifc_regs *regs) {
> > +	return ioread32be(&regs->ifc_rev) & FSL_IFC_VERSION_MASK;
> > +}
> > +
> > +static inline int fsl_ifc_bank_count(struct fsl_ifc_regs *regs) {
> > +	return (fsl_ifc_version(regs) == FSL_IFC_VERSION_1_0_0) ? 4 : 8;
> > +}
> 
> Whitespace

Oops.

> Do we really need the bank count here, as opposed to just checking it in
> probe()?  I also don't really care for reading the registers over and
> over, even though it's not performance critical.

The bank count is used in fsl_ifc_nand.c and fsl_ifc.c, so I thought it
was a good idea to have the version to bank count mapping defined in one
place rather than two.

> The reserved bits of the version register are defined as zero for
> current versions -- I think just comparing ifc_rev to the version
> constant, as is currently done, is fine.

I wasn't sure because the manuals I have only say that reserved values
are zero at reset.

> Also, please send the patch to the mtd list and maintainer.

Ok.

-Aaron

Re: [PATCH v2] fsl_ifc: Support all 8 IFC chip selects

[Scott Wood]

On Tue, 2014-08-26 at 16:34 -0500, Aaron Sierra wrote:
> > > +static inline u32 fsl_ifc_version(struct fsl_ifc_regs *regs) {
> > > +	return ioread32be(&regs->ifc_rev) & FSL_IFC_VERSION_MASK;
> > > +}
> > > +
> > > +static inline int fsl_ifc_bank_count(struct fsl_ifc_regs *regs) {
> > > +	return (fsl_ifc_version(regs) == FSL_IFC_VERSION_1_0_0) ? 4 : 8;
> > > +}
> > 
> > Whitespace
> 
> Oops.
> 
> > Do we really need the bank count here, as opposed to just checking it in
> > probe()?  I also don't really care for reading the registers over and
> > over, even though it's not performance critical.
> 
> The bank count is used in fsl_ifc_nand.c and fsl_ifc.c, so I thought it
> was a good idea to have the version to bank count mapping defined in one
> place rather than two.

OK.

> > The reserved bits of the version register are defined as zero for
> > current versions -- I think just comparing ifc_rev to the version
> > constant, as is currently done, is fine.
> 
> I wasn't sure because the manuals I have only say that reserved values
> are zero at reset.

Well, we already have a version check that assumes they're zero, but if
you want to be especially safe I guess that's OK. :-)

-Scott