diff mbox

[v3,1/2] mtd: brcmnand: Force 8bit mode before doing nand_scan_ident()

Message ID 20151028001443.GX13239@google.com
State Superseded
Headers show

Commit Message

Brian Norris Oct. 28, 2015, 12:14 a.m. UTC 
On Fri, Oct 23, 2015 at 10:46:12AM +0530, Anup Patel wrote:
> Just like other NAND controllers, the NAND READID command only works
> in 8bit mode for all versions of BRCMNAND controller.
> 
> This patch forces 8bit mode for each NAND CS in brcmnand_init_cs()
> before doing nand_scan_ident() to ensure that BRCMNAND controller
> is in 8bit mode when NAND READID command is issued.
> 
> Signed-off-by: Anup Patel <anup.patel@broadcom.com>
> Reviewed-by: Ray Jui <rjui@broadcom.com>
> Reviewed-by: Scott Branden <sbranden@broadcom.com>
> ---
>  drivers/mtd/nand/brcmnand/brcmnand.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/mtd/nand/brcmnand/brcmnand.c b/drivers/mtd/nand/brcmnand/brcmnand.c
> index 4cba03d..0be8ef9 100644
> --- a/drivers/mtd/nand/brcmnand/brcmnand.c
> +++ b/drivers/mtd/nand/brcmnand/brcmnand.c
> @@ -1888,6 +1888,7 @@ static int brcmnand_init_cs(struct brcmnand_host *host)
>  	struct mtd_info *mtd;
>  	struct nand_chip *chip;
>  	int ret;
> +	u16 cfg_offs;
>  	struct mtd_part_parser_data ppdata = { .of_node = dn };
>  
>  	ret = of_property_read_u32(dn, "reg", &host->cs);
> @@ -1930,6 +1931,14 @@ static int brcmnand_init_cs(struct brcmnand_host *host)
>  
>  	chip->controller = &ctrl->controller;
>  
> +	/*
> +	 * The bootloader might have configured 16bit mode but
> +	 * NAND READID command only works in 8bit mode. We force
> +	 * 8bit mode here to ensure that NAND READID commands works.
> +	 */
> +	cfg_offs = brcmnand_cs_offset(ctrl, host->cs, BRCMNAND_CS_CFG);
> +	nand_writereg(ctrl, cfg_offs, nand_readreg(ctrl, cfg_offs) & ~BIT(23));

Can we get a new enum for cfg bits? Unfortunately, I never managed that
in brcmnand_set_cfg(); just magic numbers :( But I'd like to stop that
if we're going to have to touch these bits outside of
brcmnand_set_cfg().

> +
>  	if (nand_scan_ident(mtd, 1, NULL))
>  		return -ENXIO;
>  

How about the following, as a preparatory patch? Only compile tested.

From c5423a86dbfa33b550d2b170bda3c12ecf4d5313 Mon Sep 17 00:00:00 2001
From: Brian Norris <computersforpeace@gmail.com>
Date: Tue, 27 Oct 2015 17:12:13 -0700
Subject: [PATCH] mtd: brcmnand: factor out CFG and CFG_EXT bitfields

These used magic numbers. Shame on me.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
Cc: Anup Patel <anup.patel@broadcom.com>
---
 drivers/mtd/nand/brcmnand/brcmnand.c | 38 +++++++++++++++++++++++++++++-------
 1 file changed, 31 insertions(+), 7 deletions(-)

Comments

Anup Patel Oct. 28, 2015, 9:13 a.m. UTC | #1 
> -----Original Message-----
> From: Brian Norris [mailto:computersforpeace@gmail.com]
> Sent: 28 October 2015 05:45
> To: Anup Patel
> Cc: David Woodhouse; Linux MTD; Rob Herring; Pawel Moll; Mark Rutland;
> Catalin Marinas; Will Deacon; Sudeep Holla; Ian Campbell; Kumar Gala; Ray Jui;
> Scott Branden; Florian Fainelli; Pramod Kumar; Vikram Prakash; Sandeep
> Tripathy; Linux ARM Kernel; Device Tree; Linux Kernel; bcm-kernel-feedback-list
> Subject: Re: [PATCH v3 1/2] mtd: brcmnand: Force 8bit mode before doing
> nand_scan_ident()
> 
> On Fri, Oct 23, 2015 at 10:46:12AM +0530, Anup Patel wrote:
> > Just like other NAND controllers, the NAND READID command only works
> > in 8bit mode for all versions of BRCMNAND controller.
> >
> > This patch forces 8bit mode for each NAND CS in brcmnand_init_cs()
> > before doing nand_scan_ident() to ensure that BRCMNAND controller is
> > in 8bit mode when NAND READID command is issued.
> >
> > Signed-off-by: Anup Patel <anup.patel@broadcom.com>
> > Reviewed-by: Ray Jui <rjui@broadcom.com>
> > Reviewed-by: Scott Branden <sbranden@broadcom.com>
> > ---
> >  drivers/mtd/nand/brcmnand/brcmnand.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/drivers/mtd/nand/brcmnand/brcmnand.c
> > b/drivers/mtd/nand/brcmnand/brcmnand.c
> > index 4cba03d..0be8ef9 100644
> > --- a/drivers/mtd/nand/brcmnand/brcmnand.c
> > +++ b/drivers/mtd/nand/brcmnand/brcmnand.c
> > @@ -1888,6 +1888,7 @@ static int brcmnand_init_cs(struct brcmnand_host
> *host)
> >  	struct mtd_info *mtd;
> >  	struct nand_chip *chip;
> >  	int ret;
> > +	u16 cfg_offs;
> >  	struct mtd_part_parser_data ppdata = { .of_node = dn };
> >
> >  	ret = of_property_read_u32(dn, "reg", &host->cs); @@ -1930,6
> > +1931,14 @@ static int brcmnand_init_cs(struct brcmnand_host *host)
> >
> >  	chip->controller = &ctrl->controller;
> >
> > +	/*
> > +	 * The bootloader might have configured 16bit mode but
> > +	 * NAND READID command only works in 8bit mode. We force
> > +	 * 8bit mode here to ensure that NAND READID commands works.
> > +	 */
> > +	cfg_offs = brcmnand_cs_offset(ctrl, host->cs, BRCMNAND_CS_CFG);
> > +	nand_writereg(ctrl, cfg_offs, nand_readreg(ctrl, cfg_offs) &
> > +~BIT(23));
> 
> Can we get a new enum for cfg bits? Unfortunately, I never managed that in
> brcmnand_set_cfg(); just magic numbers :( But I'd like to stop that if we're going
> to have to touch these bits outside of brcmnand_set_cfg().

Even I felt the need to have enum for cfg bits, just like other parts of the
BRCM NAND driver.

> 
> > +
> >  	if (nand_scan_ident(mtd, 1, NULL))
> >  		return -ENXIO;
> >
> 
> How about the following, as a preparatory patch? Only compile tested.
> 
> From c5423a86dbfa33b550d2b170bda3c12ecf4d5313 Mon Sep 17 00:00:00
> 2001
> From: Brian Norris <computersforpeace@gmail.com>
> Date: Tue, 27 Oct 2015 17:12:13 -0700
> Subject: [PATCH] mtd: brcmnand: factor out CFG and CFG_EXT bitfields
> 
> These used magic numbers. Shame on me.
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> Cc: Anup Patel <anup.patel@broadcom.com>
> ---
>  drivers/mtd/nand/brcmnand/brcmnand.c | 38
> +++++++++++++++++++++++++++++-------
>  1 file changed, 31 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/mtd/nand/brcmnand/brcmnand.c
> b/drivers/mtd/nand/brcmnand/brcmnand.c
> index d694d876631e..c93fbc3869ee 100644
> --- a/drivers/mtd/nand/brcmnand/brcmnand.c
> +++ b/drivers/mtd/nand/brcmnand/brcmnand.c
> @@ -344,6 +344,28 @@ static const u8 brcmnand_cs_offsets_cs0[] = {
>  	[BRCMNAND_CS_TIMING2]		= 0x14,
>  };
> 
> +/*
> + * Bitfields for the CFG and CFG_EXT registers. Pre-v7.1 controllers
> +only had
> + * one config register, but once the bitfields overflowed, newer
> +controllers
> + * (v7.1 and newer) added a CFG_EXT register and shuffled a few fields around.
> + */
> +enum {
> +	CFG_BLK_ADR_BYTES_SHIFT		= 8,
> +	CFG_COL_ADR_BYTES_SHIFT		= 12,
> +	CFG_FUL_ADR_BYTES_SHIFT		= 16,
> +	CFG_BUS_WIDTH_SHIFT		= 23,
> +	CFG_BUS_WIDTH			=
> BIT(CFG_BUS_WIDTH_SHIFT),
> +	CFG_DEVICE_SIZE_SHIFT		= 24,
> +
> +	/* Only for pre-v7.1 (with no CFG_EXT register) */
> +	CFG_PAGE_SIZE_SHIFT		= 20,
> +	CFG_BLK_SIZE_SHIFT		= 28,
> +
> +	/* Only for v7.1+ (with CFG_EXT register) */
> +	CFG_EXT_PAGE_SIZE_SHIFT		= 0,
> +	CFG_EXT_BLK_SIZE_SHIFT		= 4,
> +};
> +
>  /* BRCMNAND_INTFC_STATUS */
>  enum {
>  	INTFC_FLASH_STATUS		= GENMASK(7, 0),
> @@ -1710,17 +1732,19 @@ static int brcmnand_set_cfg(struct brcmnand_host
> *host,
>  	}
>  	device_size = fls64(cfg->device_size) -
> fls64(BRCMNAND_MIN_DEVSIZE);
> 
> -	tmp = (cfg->blk_adr_bytes << 8) |
> -		(cfg->col_adr_bytes << 12) |
> -		(cfg->ful_adr_bytes << 16) |
> -		(!!(cfg->device_width == 16) << 23) |
> -		(device_size << 24);
> +	tmp = (cfg->blk_adr_bytes << CFG_BLK_ADR_BYTES_SHIFT) |
> +		(cfg->col_adr_bytes << CFG_BLK_ADR_BYTES_SHIFT) |
> +		(cfg->ful_adr_bytes << CFG_FUL_ADR_BYTES_SHIFT) |
> +		(!!(cfg->device_width == 16) << CFG_BUS_WIDTH_SHIFT) |
> +		(device_size << CFG_DEVICE_SIZE_SHIFT);
>  	if (cfg_offs == cfg_ext_offs) {
> -		tmp |= (page_size << 20) | (block_size << 28);
> +		tmp |= (page_size << CFG_PAGE_SIZE_SHIFT) |
> +		       (block_size << CFG_BLK_SIZE_SHIFT);
>  		nand_writereg(ctrl, cfg_offs, tmp);
>  	} else {
>  		nand_writereg(ctrl, cfg_offs, tmp);
> -		tmp = page_size | (block_size << 4);
> +		tmp = (page_size << CFG_EXT_PAGE_SIZE_SHIFT) |
> +		      (block_size << CFG_EXT_BLK_SIZE_SHIFT);
>  		nand_writereg(ctrl, cfg_ext_offs, tmp);
>  	}

Your patch looks good to me. I will base this patch upon your
patch and test it at my end.

I will send a revised patchset which will include your patch (preserving
your authorship) and tested on NS2 SVK.

Thanks,
Anup
diff mbox

Patch

diff --git a/drivers/mtd/nand/brcmnand/brcmnand.c b/drivers/mtd/nand/brcmnand/brcmnand.c
index d694d876631e..c93fbc3869ee 100644
--- a/drivers/mtd/nand/brcmnand/brcmnand.c
+++ b/drivers/mtd/nand/brcmnand/brcmnand.c
@@ -344,6 +344,28 @@  static const u8 brcmnand_cs_offsets_cs0[] = {
 	[BRCMNAND_CS_TIMING2]		= 0x14,
 };
 
+/*
+ * Bitfields for the CFG and CFG_EXT registers. Pre-v7.1 controllers only had
+ * one config register, but once the bitfields overflowed, newer controllers
+ * (v7.1 and newer) added a CFG_EXT register and shuffled a few fields around.
+ */
+enum {
+	CFG_BLK_ADR_BYTES_SHIFT		= 8,
+	CFG_COL_ADR_BYTES_SHIFT		= 12,
+	CFG_FUL_ADR_BYTES_SHIFT		= 16,
+	CFG_BUS_WIDTH_SHIFT		= 23,
+	CFG_BUS_WIDTH			= BIT(CFG_BUS_WIDTH_SHIFT),
+	CFG_DEVICE_SIZE_SHIFT		= 24,
+
+	/* Only for pre-v7.1 (with no CFG_EXT register) */
+	CFG_PAGE_SIZE_SHIFT		= 20,
+	CFG_BLK_SIZE_SHIFT		= 28,
+
+	/* Only for v7.1+ (with CFG_EXT register) */
+	CFG_EXT_PAGE_SIZE_SHIFT		= 0,
+	CFG_EXT_BLK_SIZE_SHIFT		= 4,
+};
+
 /* BRCMNAND_INTFC_STATUS */
 enum {
 	INTFC_FLASH_STATUS		= GENMASK(7, 0),
@@ -1710,17 +1732,19 @@  static int brcmnand_set_cfg(struct brcmnand_host *host,
 	}
 	device_size = fls64(cfg->device_size) - fls64(BRCMNAND_MIN_DEVSIZE);
 
-	tmp = (cfg->blk_adr_bytes << 8) |
-		(cfg->col_adr_bytes << 12) |
-		(cfg->ful_adr_bytes << 16) |
-		(!!(cfg->device_width == 16) << 23) |
-		(device_size << 24);
+	tmp = (cfg->blk_adr_bytes << CFG_BLK_ADR_BYTES_SHIFT) |
+		(cfg->col_adr_bytes << CFG_BLK_ADR_BYTES_SHIFT) |
+		(cfg->ful_adr_bytes << CFG_FUL_ADR_BYTES_SHIFT) |
+		(!!(cfg->device_width == 16) << CFG_BUS_WIDTH_SHIFT) |
+		(device_size << CFG_DEVICE_SIZE_SHIFT);
 	if (cfg_offs == cfg_ext_offs) {
-		tmp |= (page_size << 20) | (block_size << 28);
+		tmp |= (page_size << CFG_PAGE_SIZE_SHIFT) |
+		       (block_size << CFG_BLK_SIZE_SHIFT);
 		nand_writereg(ctrl, cfg_offs, tmp);
 	} else {
 		nand_writereg(ctrl, cfg_offs, tmp);
-		tmp = page_size | (block_size << 4);
+		tmp = (page_size << CFG_EXT_PAGE_SIZE_SHIFT) |
+		      (block_size << CFG_EXT_BLK_SIZE_SHIFT);
 		nand_writereg(ctrl, cfg_ext_offs, tmp);
 	}