[PATCH v3 0/2] NAND support for Broadcom NS2 SoC

[PATCH v3 0/2] NAND support for Broadcom NS2 SoC

[Anup Patel]

We enable NAND support for Broadcom NS2 SoC by reusing existing
BRCMNAND driver.

This patchset applies on-top of "arm64: Simple additions to
NS2 DT" v1 patchset and is available in ns2_nand_v3 branch of
https://github.com/Broadcom/arm64-linux.git.

The patchset is tested on NS2 SVK.

Changes since v2:
 - Dropped patch1 and patch2 because these are already merged
   by MTD maintainer.
 - Avoid using absolute node paths in ns2-svk.dts.

Changes since v1:
 - Dropped patch3 and patch4 because we don't need to reset
   BRCMNAND controller for NS2.
 - Added patch to force 8bit mode before doing nand_scan_ident()
   in brcmnand_init_cs().

Anup Patel (2):
  mtd: brcmnand: Force 8bit mode before doing nand_scan_ident()
  arm64: dts: Add BRCM IPROC NAND DT node for NS2

 arch/arm64/boot/dts/broadcom/ns2-svk.dts | 30 ++++++++++++++++++++----------
 arch/arm64/boot/dts/broadcom/ns2.dtsi    | 14 ++++++++++++++
 drivers/mtd/nand/brcmnand/brcmnand.c     |  9 +++++++++
 3 files changed, 43 insertions(+), 10 deletions(-)

-- 
1.9.1

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

[Anup Patel]

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));
+
 	if (nand_scan_ident(mtd, 1, NULL))
 		return -ENXIO;
 
-- 
1.9.1

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

[Brian Norris]

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(-)

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);
 	}
 
-- 
2.6.0.rc2.230.g3dd15c0

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

[Anup Patel]

> -----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

[PATCH v3 2/2] arm64: dts: Add BRCM IPROC NAND DT node for NS2

[Anup Patel]

The NAND controller on NS2 SoC is compatible with existing
BRCM IPROC NAND driver so let's enable it in NS2 DT and
NS2 SVK DT.

This patch also fixes use of node labels in ns2-svk.dts.

Signed-off-by: Anup Patel <anup.patel@broadcom.com>
Reviewed-by: Ray Jui <rjui@broadcom.com>
Reviewed-by: Scott Branden <sbranden@broadcom.com>
---
 arch/arm64/boot/dts/broadcom/ns2-svk.dts | 30 ++++++++++++++++++++----------
 arch/arm64/boot/dts/broadcom/ns2.dtsi    | 14 ++++++++++++++
 2 files changed, 34 insertions(+), 10 deletions(-)

diff --git a/arch/arm64/boot/dts/broadcom/ns2-svk.dts b/arch/arm64/boot/dts/broadcom/ns2-svk.dts
index e5950d5..6bb3d4d 100644
--- a/arch/arm64/boot/dts/broadcom/ns2-svk.dts
+++ b/arch/arm64/boot/dts/broadcom/ns2-svk.dts
@@ -50,18 +50,28 @@
 		device_type = "memory";
 		reg = <0x000000000 0x80000000 0x00000000 0x40000000>;
 	};
+};
 
-	soc: soc {
-		i2c0: i2c@66080000 {
-			status = "ok";
-		};
+&i2c0 {
+	status = "ok";
+};
 
-		i2c1: i2c@660b0000 {
-			status = "ok";
-		};
+&i2c1 {
+	status = "ok";
+};
+
+&uart3 {
+	status = "ok";
+};
 
-		uart3: serial@66130000 {
-			status = "ok";
-		};
+&nand {
+	nandcs@0 {
+		compatible = "brcm,nandcs";
+		reg = <0>;
+		nand-ecc-mode = "hw";
+		nand-ecc-strength = <8>;
+		nand-ecc-step-size = <512>;
+		#address-cells = <1>;
+		#size-cells = <1>;
 	};
 };
diff --git a/arch/arm64/boot/dts/broadcom/ns2.dtsi b/arch/arm64/boot/dts/broadcom/ns2.dtsi
index f603277..9610822 100644
--- a/arch/arm64/boot/dts/broadcom/ns2.dtsi
+++ b/arch/arm64/boot/dts/broadcom/ns2.dtsi
@@ -212,5 +212,19 @@
 			compatible = "brcm,iproc-rng200";
 			reg = <0x66220000 0x28>;
 		};
+
+		nand: nand@66460000 {
+			compatible = "brcm,nand-iproc", "brcm,brcmnand-v6.1";
+			reg = <0x66460000 0x600>,
+			      <0x67015408 0x600>,
+			      <0x66460f00 0x20>;
+			reg-names = "nand", "iproc-idm", "iproc-ext";
+			interrupts = <GIC_SPI 420 IRQ_TYPE_LEVEL_HIGH>;
+
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			brcm,nand-has-wp;
+		};
 	};
 };
-- 
1.9.1

Re: [PATCH v3 2/2] arm64: dts: Add BRCM IPROC NAND DT node for NS2

[Brian Norris]

On Fri, Oct 23, 2015 at 10:46:13AM +0530, Anup Patel wrote:
> The NAND controller on NS2 SoC is compatible with existing
> BRCM IPROC NAND driver so let's enable it in NS2 DT and
> NS2 SVK DT.
> 
> This patch also fixes use of node labels in ns2-svk.dts.
> 
> Signed-off-by: Anup Patel <anup.patel-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
> Reviewed-by: Ray Jui <rjui-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
> Reviewed-by: Scott Branden <sbranden-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
> ---
>  arch/arm64/boot/dts/broadcom/ns2-svk.dts | 30 ++++++++++++++++++++----------
>  arch/arm64/boot/dts/broadcom/ns2.dtsi    | 14 ++++++++++++++
>  2 files changed, 34 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/broadcom/ns2-svk.dts b/arch/arm64/boot/dts/broadcom/ns2-svk.dts
> index e5950d5..6bb3d4d 100644
> --- a/arch/arm64/boot/dts/broadcom/ns2-svk.dts
> +++ b/arch/arm64/boot/dts/broadcom/ns2-svk.dts
> @@ -50,18 +50,28 @@
>  		device_type = "memory";
>  		reg = <0x000000000 0x80000000 0x00000000 0x40000000>;
>  	};
> +};
>  
> -	soc: soc {
> -		i2c0: i2c@66080000 {
> -			status = "ok";
> -		};
> +&i2c0 {
> +	status = "ok";
> +};
>  
> -		i2c1: i2c@660b0000 {
> -			status = "ok";
> -		};
> +&i2c1 {
> +	status = "ok";
> +};
> +
> +&uart3 {
> +	status = "ok";
> +};
>  
> -		uart3: serial@66130000 {
> -			status = "ok";
> -		};
> +&nand {
> +	nandcs@0 {
> +		compatible = "brcm,nandcs";
> +		reg = <0>;
> +		nand-ecc-mode = "hw";
> +		nand-ecc-strength = <8>;
> +		nand-ecc-step-size = <512>;
> +		#address-cells = <1>;
> +		#size-cells = <1>;
>  	};
>  };
> diff --git a/arch/arm64/boot/dts/broadcom/ns2.dtsi b/arch/arm64/boot/dts/broadcom/ns2.dtsi
> index f603277..9610822 100644
> --- a/arch/arm64/boot/dts/broadcom/ns2.dtsi
> +++ b/arch/arm64/boot/dts/broadcom/ns2.dtsi
> @@ -212,5 +212,19 @@
>  			compatible = "brcm,iproc-rng200";
>  			reg = <0x66220000 0x28>;
>  		};
> +
> +		nand: nand@66460000 {
> +			compatible = "brcm,nand-iproc", "brcm,brcmnand-v6.1";

Technically, the binding says you should also have "brcm,brcmnand" as a
last resort. Otherwise (for the NAND parts):

Reviewed-by: Brian Norris <computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

> +			reg = <0x66460000 0x600>,
> +			      <0x67015408 0x600>,
> +			      <0x66460f00 0x20>;
> +			reg-names = "nand", "iproc-idm", "iproc-ext";
> +			interrupts = <GIC_SPI 420 IRQ_TYPE_LEVEL_HIGH>;
> +
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			brcm,nand-has-wp;
> +		};
>  	};
>  };
> -- 
> 1.9.1
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/2] arm64: dts: Add BRCM IPROC NAND DT node for NS2

[Ray Jui]

On 10/27/2015 5:19 PM, Brian Norris wrote:
> On Fri, Oct 23, 2015 at 10:46:13AM +0530, Anup Patel wrote:
>> The NAND controller on NS2 SoC is compatible with existing
>> BRCM IPROC NAND driver so let's enable it in NS2 DT and
>> NS2 SVK DT.
>>
>> This patch also fixes use of node labels in ns2-svk.dts.
>>
>> Signed-off-by: Anup Patel <anup.patel-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
>> Reviewed-by: Ray Jui <rjui-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
>> Reviewed-by: Scott Branden <sbranden-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
>> ---
>>   arch/arm64/boot/dts/broadcom/ns2-svk.dts | 30 ++++++++++++++++++++----------
>>   arch/arm64/boot/dts/broadcom/ns2.dtsi    | 14 ++++++++++++++
>>   2 files changed, 34 insertions(+), 10 deletions(-)
>>
>> diff --git a/arch/arm64/boot/dts/broadcom/ns2-svk.dts b/arch/arm64/boot/dts/broadcom/ns2-svk.dts
>> index e5950d5..6bb3d4d 100644
>> --- a/arch/arm64/boot/dts/broadcom/ns2-svk.dts
>> +++ b/arch/arm64/boot/dts/broadcom/ns2-svk.dts
>> @@ -50,18 +50,28 @@
>>   		device_type = "memory";
>>   		reg = <0x000000000 0x80000000 0x00000000 0x40000000>;
>>   	};
>> +};
>>
>> -	soc: soc {
>> -		i2c0: i2c@66080000 {
>> -			status = "ok";
>> -		};
>> +&i2c0 {
>> +	status = "ok";
>> +};
>>
>> -		i2c1: i2c@660b0000 {
>> -			status = "ok";
>> -		};
>> +&i2c1 {
>> +	status = "ok";
>> +};
>> +
>> +&uart3 {
>> +	status = "ok";
>> +};
>>
>> -		uart3: serial@66130000 {
>> -			status = "ok";
>> -		};
>> +&nand {
>> +	nandcs@0 {
>> +		compatible = "brcm,nandcs";
>> +		reg = <0>;
>> +		nand-ecc-mode = "hw";
>> +		nand-ecc-strength = <8>;
>> +		nand-ecc-step-size = <512>;
>> +		#address-cells = <1>;
>> +		#size-cells = <1>;
>>   	};
>>   };
>> diff --git a/arch/arm64/boot/dts/broadcom/ns2.dtsi b/arch/arm64/boot/dts/broadcom/ns2.dtsi
>> index f603277..9610822 100644
>> --- a/arch/arm64/boot/dts/broadcom/ns2.dtsi
>> +++ b/arch/arm64/boot/dts/broadcom/ns2.dtsi
>> @@ -212,5 +212,19 @@
>>   			compatible = "brcm,iproc-rng200";
>>   			reg = <0x66220000 0x28>;
>>   		};
>> +
>> +		nand: nand@66460000 {
>> +			compatible = "brcm,nand-iproc", "brcm,brcmnand-v6.1";
>
> Technically, the binding says you should also have "brcm,brcmnand" as a
> last resort. Otherwise (for the NAND parts):
>

I believe Anup was seeing issues when both "brcm,nand-iproc" and 
"brcm,brcmnand" are present.

Note "brcm,nand-iproc" invokes 'iproc_nand_probe', which calls 
'brcmnand_probe' in the end.

"brcm,brcmnand" invokes 'brcmstb_nand_probe', which also calls 
'brcmstb_probe', but without all the prep configuration required for 
"brcm,nand-iproc".

> Reviewed-by: Brian Norris <computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/2] arm64: dts: Add BRCM IPROC NAND DT node for NS2

[Brian Norris]

On Tue, Oct 27, 2015 at 05:25:32PM -0700, Ray Jui wrote:
> On 10/27/2015 5:19 PM, Brian Norris wrote:
> >On Fri, Oct 23, 2015 at 10:46:13AM +0530, Anup Patel wrote:
> >>diff --git a/arch/arm64/boot/dts/broadcom/ns2.dtsi b/arch/arm64/boot/dts/broadcom/ns2.dtsi
> >>index f603277..9610822 100644
> >>--- a/arch/arm64/boot/dts/broadcom/ns2.dtsi
> >>+++ b/arch/arm64/boot/dts/broadcom/ns2.dtsi
> >>@@ -212,5 +212,19 @@
> >>  			compatible = "brcm,iproc-rng200";
> >>  			reg = <0x66220000 0x28>;
> >>  		};
> >>+
> >>+		nand: nand@66460000 {
> >>+			compatible = "brcm,nand-iproc", "brcm,brcmnand-v6.1";
> >
> >Technically, the binding says you should also have "brcm,brcmnand" as a
> >last resort. Otherwise (for the NAND parts):
> >
> 
> I believe Anup was seeing issues when both "brcm,nand-iproc" and
> "brcm,brcmnand" are present.
> 
> Note "brcm,nand-iproc" invokes 'iproc_nand_probe', which calls
> 'brcmnand_probe' in the end.
> 
> "brcm,brcmnand" invokes 'brcmstb_nand_probe', which also calls
> 'brcmstb_probe', but without all the prep configuration required for
> "brcm,nand-iproc".

Ah, I forgot about that problem. That seems like an OF infrastructure
issue that could be fixed. We could lump these drivers back together,
and make sure that "brcm,nand-iproc" gets the priority in the
of_device_id list.

Or we could just relax the DT binding.

But wait, wouldn't cygnus already have that problem? You're using the
binding I suggested in arch/arm/boot/dts/bcm-cygnus.dtsi.

Oh, and I see we hacked this one in drivers/mtd/nand/brcmnand/Makefile:

  # link order matters; don't link the more generic brcmstb_nand.o before the
  # more specific iproc_nand.o, for instance

Brian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/2] arm64: dts: Add BRCM IPROC NAND DT node for NS2

[Ray Jui]

On 10/27/2015 5:39 PM, Brian Norris wrote:
> On Tue, Oct 27, 2015 at 05:25:32PM -0700, Ray Jui wrote:
>> On 10/27/2015 5:19 PM, Brian Norris wrote:
>>> On Fri, Oct 23, 2015 at 10:46:13AM +0530, Anup Patel wrote:
>>>> diff --git a/arch/arm64/boot/dts/broadcom/ns2.dtsi b/arch/arm64/boot/dts/broadcom/ns2.dtsi
>>>> index f603277..9610822 100644
>>>> --- a/arch/arm64/boot/dts/broadcom/ns2.dtsi
>>>> +++ b/arch/arm64/boot/dts/broadcom/ns2.dtsi
>>>> @@ -212,5 +212,19 @@
>>>>   			compatible = "brcm,iproc-rng200";
>>>>   			reg = <0x66220000 0x28>;
>>>>   		};
>>>> +
>>>> +		nand: nand@66460000 {
>>>> +			compatible = "brcm,nand-iproc", "brcm,brcmnand-v6.1";
>>>
>>> Technically, the binding says you should also have "brcm,brcmnand" as a
>>> last resort. Otherwise (for the NAND parts):
>>>
>>
>> I believe Anup was seeing issues when both "brcm,nand-iproc" and
>> "brcm,brcmnand" are present.
>>
>> Note "brcm,nand-iproc" invokes 'iproc_nand_probe', which calls
>> 'brcmnand_probe' in the end.
>>
>> "brcm,brcmnand" invokes 'brcmstb_nand_probe', which also calls
>> 'brcmstb_probe', but without all the prep configuration required for
>> "brcm,nand-iproc".
>
> Ah, I forgot about that problem. That seems like an OF infrastructure
> issue that could be fixed. We could lump these drivers back together,
> and make sure that "brcm,nand-iproc" gets the priority in the
> of_device_id list.
>
> Or we could just relax the DT binding.
>
> But wait, wouldn't cygnus already have that problem? You're using the
> binding I suggested in arch/arm/boot/dts/bcm-cygnus.dtsi.

Interestingly, we do not see this problem with Cygnus or NSP, but only 
on NS2 (arm64 based). There may be a difference between how OF devices 
are instantiated between arm and arm64?

>
> Oh, and I see we hacked this one in drivers/mtd/nand/brcmnand/Makefile:
>
>    # link order matters; don't link the more generic brcmstb_nand.o before the
>    # more specific iproc_nand.o, for instance

Yes, I see that too (after sending out my previous email, :)). Maybe 
Anup can help to elaborate on the problem. I'm now getting a bit 
confused on how the problem can surface on NS2.

But in general, I think it's a good idea to relax the requirement in the 
DT binding document to not require "brcm,brcmnand", in the case when 
"brcm,nand-iproc" and "brcm,nand-bcm63138" are present.

>
> Brian
>

Thanks,

Ray

RE: [PATCH v3 2/2] arm64: dts: Add BRCM IPROC NAND DT node for NS2

[Anup Patel]

> -----Original Message-----
> From: Ray Jui [mailto:rjui@broadcom.com]
> Sent: 28 October 2015 06:17
> To: Brian Norris
> Cc: Anup Patel; David Woodhouse; Linux MTD; Rob Herring; Pawel Moll; Mark
> Rutland; Catalin Marinas; Will Deacon; Sudeep Holla; Ian Campbell; Kumar Gala;
> 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 2/2] arm64: dts: Add BRCM IPROC NAND DT node for
> NS2
> 
> 
> 
> On 10/27/2015 5:39 PM, Brian Norris wrote:
> > On Tue, Oct 27, 2015 at 05:25:32PM -0700, Ray Jui wrote:
> >> On 10/27/2015 5:19 PM, Brian Norris wrote:
> >>> On Fri, Oct 23, 2015 at 10:46:13AM +0530, Anup Patel wrote:
> >>>> diff --git a/arch/arm64/boot/dts/broadcom/ns2.dtsi
> >>>> b/arch/arm64/boot/dts/broadcom/ns2.dtsi
> >>>> index f603277..9610822 100644
> >>>> --- a/arch/arm64/boot/dts/broadcom/ns2.dtsi
> >>>> +++ b/arch/arm64/boot/dts/broadcom/ns2.dtsi
> >>>> @@ -212,5 +212,19 @@
> >>>>   			compatible = "brcm,iproc-rng200";
> >>>>   			reg = <0x66220000 0x28>;
> >>>>   		};
> >>>> +
> >>>> +		nand: nand@66460000 {
> >>>> +			compatible = "brcm,nand-iproc", "brcm,brcmnand-
> v6.1";
> >>>
> >>> Technically, the binding says you should also have "brcm,brcmnand"
> >>> as a last resort. Otherwise (for the NAND parts):
> >>>
> >>
> >> I believe Anup was seeing issues when both "brcm,nand-iproc" and
> >> "brcm,brcmnand" are present.
> >>
> >> Note "brcm,nand-iproc" invokes 'iproc_nand_probe', which calls
> >> 'brcmnand_probe' in the end.
> >>
> >> "brcm,brcmnand" invokes 'brcmstb_nand_probe', which also calls
> >> 'brcmstb_probe', but without all the prep configuration required for
> >> "brcm,nand-iproc".
> >
> > Ah, I forgot about that problem. That seems like an OF infrastructure
> > issue that could be fixed. We could lump these drivers back together,
> > and make sure that "brcm,nand-iproc" gets the priority in the
> > of_device_id list.
> >
> > Or we could just relax the DT binding.
> >
> > But wait, wouldn't cygnus already have that problem? You're using the
> > binding I suggested in arch/arm/boot/dts/bcm-cygnus.dtsi.
> 
> Interestingly, we do not see this problem with Cygnus or NSP, but only on NS2
> (arm64 based). There may be a difference between how OF devices are
> instantiated between arm and arm64?

Alternately, it could be also about order in-which platform drivers are matched
for newly created OF device.

> 
> >
> > Oh, and I see we hacked this one in drivers/mtd/nand/brcmnand/Makefile:
> >
> >    # link order matters; don't link the more generic brcmstb_nand.o before the
> >    # more specific iproc_nand.o, for instance
> 
> Yes, I see that too (after sending out my previous email, :)). Maybe
> Anup can help to elaborate on the problem. I'm now getting a bit
> confused on how the problem can surface on NS2.

I think for a newly created OF devices the Linux device driver framework will
match the platform drivers in the order in which they are registered by module
init functions. Now the order of module init function calls will be based how
the .initcall section is formed by linker and order in which objects are linked.

IMHO, if multiple platform drivers match given OF device then platform driver
with longest matching compatible string should only be probed. I don't know
how big change this would be for OF framework.

> 
> But in general, I think it's a good idea to relax the requirement in the
> DT binding document to not require "brcm,brcmnand", in the case when
> "brcm,nand-iproc" and "brcm,nand-bcm63138" are present.

Even I think, it will be good to relax the DT bindings requirement for
BRCM NAND driver.

Regards,
Anup

Re: [PATCH v3 2/2] arm64: dts: Add BRCM IPROC NAND DT node for NS2

[Ray Jui]

On 10/28/2015 2:06 AM, Anup Patel wrote:
>
>
>> -----Original Message-----
>> From: Ray Jui [mailto:rjui-dY08KVG/lbpWk0Htik3J/w@public.gmane.org]
>> Sent: 28 October 2015 06:17
>> To: Brian Norris
>> Cc: Anup Patel; David Woodhouse; Linux MTD; Rob Herring; Pawel Moll; Mark
>> Rutland; Catalin Marinas; Will Deacon; Sudeep Holla; Ian Campbell; Kumar Gala;
>> 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 2/2] arm64: dts: Add BRCM IPROC NAND DT node for
>> NS2
>>
>>
>>
>> On 10/27/2015 5:39 PM, Brian Norris wrote:
>>> On Tue, Oct 27, 2015 at 05:25:32PM -0700, Ray Jui wrote:
>>>> On 10/27/2015 5:19 PM, Brian Norris wrote:
>>>>> On Fri, Oct 23, 2015 at 10:46:13AM +0530, Anup Patel wrote:
>>>>>> diff --git a/arch/arm64/boot/dts/broadcom/ns2.dtsi
>>>>>> b/arch/arm64/boot/dts/broadcom/ns2.dtsi
>>>>>> index f603277..9610822 100644
>>>>>> --- a/arch/arm64/boot/dts/broadcom/ns2.dtsi
>>>>>> +++ b/arch/arm64/boot/dts/broadcom/ns2.dtsi
>>>>>> @@ -212,5 +212,19 @@
>>>>>>    			compatible = "brcm,iproc-rng200";
>>>>>>    			reg = <0x66220000 0x28>;
>>>>>>    		};
>>>>>> +
>>>>>> +		nand: nand@66460000 {
>>>>>> +			compatible = "brcm,nand-iproc", "brcm,brcmnand-
>> v6.1";
>>>>>
>>>>> Technically, the binding says you should also have "brcm,brcmnand"
>>>>> as a last resort. Otherwise (for the NAND parts):
>>>>>
>>>>
>>>> I believe Anup was seeing issues when both "brcm,nand-iproc" and
>>>> "brcm,brcmnand" are present.
>>>>
>>>> Note "brcm,nand-iproc" invokes 'iproc_nand_probe', which calls
>>>> 'brcmnand_probe' in the end.
>>>>
>>>> "brcm,brcmnand" invokes 'brcmstb_nand_probe', which also calls
>>>> 'brcmstb_probe', but without all the prep configuration required for
>>>> "brcm,nand-iproc".
>>>
>>> Ah, I forgot about that problem. That seems like an OF infrastructure
>>> issue that could be fixed. We could lump these drivers back together,
>>> and make sure that "brcm,nand-iproc" gets the priority in the
>>> of_device_id list.
>>>
>>> Or we could just relax the DT binding.
>>>
>>> But wait, wouldn't cygnus already have that problem? You're using the
>>> binding I suggested in arch/arm/boot/dts/bcm-cygnus.dtsi.
>>
>> Interestingly, we do not see this problem with Cygnus or NSP, but only on NS2
>> (arm64 based). There may be a difference between how OF devices are
>> instantiated between arm and arm64?
>
> Alternately, it could be also about order in-which platform drivers are matched
> for newly created OF device.
>
>>
>>>
>>> Oh, and I see we hacked this one in drivers/mtd/nand/brcmnand/Makefile:
>>>
>>>     # link order matters; don't link the more generic brcmstb_nand.o before the
>>>     # more specific iproc_nand.o, for instance
>>
>> Yes, I see that too (after sending out my previous email, :)). Maybe
>> Anup can help to elaborate on the problem. I'm now getting a bit
>> confused on how the problem can surface on NS2.
>
> I think for a newly created OF devices the Linux device driver framework will
> match the platform drivers in the order in which they are registered by module
> init functions. Now the order of module init function calls will be based how
> the .initcall section is formed by linker and order in which objects are linked.
>

Yes, what you said is my understanding as well, but then here is the 
mystery. This is the link order in brcmnand/Makefile:

1 # link order matters; don't link the more generic brcmstb_nand.o 
before the
2 # more specific iproc_nand.o, for instance
3 obj-$(CONFIG_MTD_NAND_BRCMNAND) += iproc_nand.o
4 obj-$(CONFIG_MTD_NAND_BRCMNAND) += bcm63138_nand.o
5 obj-$(CONFIG_MTD_NAND_BRCMNAND) += brcmstb_nand.o
6 obj-$(CONFIG_MTD_NAND_BRCMNAND) += brcmnand.o

Based on the order above, probe from iproc_nand should always be called 
first if a matching compatible string is found. If so, then why having 
both compatible strings "brcm,brcmnand" and "brcm,nand-iproc" causes 
issues for NS2 (I remember it broke smoketest in the past when you 
submitted the change)? I'm not saying we should have "brcm,brcmnand" for 
iProc devices, but I don't get why it would cause any issue.

Does the order of the compatible string matter when they are assigned to 
the same 'compatible' property like this?

compatible = "brcm,nand-iproc", "brcm,brcmnand-v6.1",    "brcm,brcmnand";


> IMHO, if multiple platform drivers match given OF device then platform driver
> with longest matching compatible string should only be probed. I don't know
> how big change this would be for OF framework.
>
>>
>> But in general, I think it's a good idea to relax the requirement in the
>> DT binding document to not require "brcm,brcmnand", in the case when
>> "brcm,nand-iproc" and "brcm,nand-bcm63138" are present.
>
> Even I think, it will be good to relax the DT bindings requirement for
> BRCM NAND driver.
>
> Regards,
> Anup
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/2] arm64: dts: Add BRCM IPROC NAND DT node for NS2

[Florian Fainelli]

On 28/10/15 09:08, Ray Jui wrote:
> 
> 
> On 10/28/2015 2:06 AM, Anup Patel wrote:
>>
>>
>>> -----Original Message-----
>>> From: Ray Jui [mailto:rjui@broadcom.com]
>>> Sent: 28 October 2015 06:17
>>> To: Brian Norris
>>> Cc: Anup Patel; David Woodhouse; Linux MTD; Rob Herring; Pawel Moll;
>>> Mark
>>> Rutland; Catalin Marinas; Will Deacon; Sudeep Holla; Ian Campbell;
>>> Kumar Gala;
>>> 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 2/2] arm64: dts: Add BRCM IPROC NAND DT node for
>>> NS2
>>>
>>>
>>>
>>> On 10/27/2015 5:39 PM, Brian Norris wrote:
>>>> On Tue, Oct 27, 2015 at 05:25:32PM -0700, Ray Jui wrote:
>>>>> On 10/27/2015 5:19 PM, Brian Norris wrote:
>>>>>> On Fri, Oct 23, 2015 at 10:46:13AM +0530, Anup Patel wrote:
>>>>>>> diff --git a/arch/arm64/boot/dts/broadcom/ns2.dtsi
>>>>>>> b/arch/arm64/boot/dts/broadcom/ns2.dtsi
>>>>>>> index f603277..9610822 100644
>>>>>>> --- a/arch/arm64/boot/dts/broadcom/ns2.dtsi
>>>>>>> +++ b/arch/arm64/boot/dts/broadcom/ns2.dtsi
>>>>>>> @@ -212,5 +212,19 @@
>>>>>>>                compatible = "brcm,iproc-rng200";
>>>>>>>                reg = <0x66220000 0x28>;
>>>>>>>            };
>>>>>>> +
>>>>>>> +        nand: nand@66460000 {
>>>>>>> +            compatible = "brcm,nand-iproc", "brcm,brcmnand-
>>> v6.1";
>>>>>>
>>>>>> Technically, the binding says you should also have "brcm,brcmnand"
>>>>>> as a last resort. Otherwise (for the NAND parts):
>>>>>>
>>>>>
>>>>> I believe Anup was seeing issues when both "brcm,nand-iproc" and
>>>>> "brcm,brcmnand" are present.
>>>>>
>>>>> Note "brcm,nand-iproc" invokes 'iproc_nand_probe', which calls
>>>>> 'brcmnand_probe' in the end.
>>>>>
>>>>> "brcm,brcmnand" invokes 'brcmstb_nand_probe', which also calls
>>>>> 'brcmstb_probe', but without all the prep configuration required for
>>>>> "brcm,nand-iproc".
>>>>
>>>> Ah, I forgot about that problem. That seems like an OF infrastructure
>>>> issue that could be fixed. We could lump these drivers back together,
>>>> and make sure that "brcm,nand-iproc" gets the priority in the
>>>> of_device_id list.
>>>>
>>>> Or we could just relax the DT binding.
>>>>
>>>> But wait, wouldn't cygnus already have that problem? You're using the
>>>> binding I suggested in arch/arm/boot/dts/bcm-cygnus.dtsi.
>>>
>>> Interestingly, we do not see this problem with Cygnus or NSP, but
>>> only on NS2
>>> (arm64 based). There may be a difference between how OF devices are
>>> instantiated between arm and arm64?
>>
>> Alternately, it could be also about order in-which platform drivers
>> are matched
>> for newly created OF device.
>>
>>>
>>>>
>>>> Oh, and I see we hacked this one in drivers/mtd/nand/brcmnand/Makefile:
>>>>
>>>>     # link order matters; don't link the more generic brcmstb_nand.o
>>>> before the
>>>>     # more specific iproc_nand.o, for instance
>>>
>>> Yes, I see that too (after sending out my previous email, :)). Maybe
>>> Anup can help to elaborate on the problem. I'm now getting a bit
>>> confused on how the problem can surface on NS2.
>>
>> I think for a newly created OF devices the Linux device driver
>> framework will
>> match the platform drivers in the order in which they are registered
>> by module
>> init functions. Now the order of module init function calls will be
>> based how
>> the .initcall section is formed by linker and order in which objects
>> are linked.
>>
> 
> Yes, what you said is my understanding as well, but then here is the
> mystery. This is the link order in brcmnand/Makefile:
> 
> 1 # link order matters; don't link the more generic brcmstb_nand.o
> before the
> 2 # more specific iproc_nand.o, for instance
> 3 obj-$(CONFIG_MTD_NAND_BRCMNAND) += iproc_nand.o
> 4 obj-$(CONFIG_MTD_NAND_BRCMNAND) += bcm63138_nand.o
> 5 obj-$(CONFIG_MTD_NAND_BRCMNAND) += brcmstb_nand.o
> 6 obj-$(CONFIG_MTD_NAND_BRCMNAND) += brcmnand.o
> 
> Based on the order above, probe from iproc_nand should always be called
> first if a matching compatible string is found. If so, then why having
> both compatible strings "brcm,brcmnand" and "brcm,nand-iproc" causes
> issues for NS2 (I remember it broke smoketest in the past when you
> submitted the change)? I'm not saying we should have "brcm,brcmnand" for
> iProc devices, but I don't get why it would cause any issue.
> 
> Does the order of the compatible string matter when they are assigned to
> the same 'compatible' property like this?
> 
> compatible = "brcm,nand-iproc", "brcm,brcmnand-v6.1",    "brcm,brcmnand";

It is possible that by the time this was designed and tested, and the
baseline you are now using, there have been changes in how probing is
done and resolved. I have not been able to pin point which commit would
be responsible for that, but it would not surprise me that something
changed in that area, especially with all the on-going on-demand probing
business happening.
-- 
Florian

Re: [PATCH v3 2/2] arm64: dts: Add BRCM IPROC NAND DT node for NS2

[Brian Norris]

On Wed, Oct 28, 2015 at 09:08:02AM -0700, Ray Jui wrote:
> On 10/28/2015 2:06 AM, Anup Patel wrote:
> >
> >I think for a newly created OF devices the Linux device driver framework will
> >match the platform drivers in the order in which they are registered by module
> >init functions. Now the order of module init function calls will be based how
> >the .initcall section is formed by linker and order in which objects are linked.
> >
> 
> Yes, what you said is my understanding as well, but then here is the
> mystery. This is the link order in brcmnand/Makefile:
> 
> 1 # link order matters; don't link the more generic brcmstb_nand.o
> before the
> 2 # more specific iproc_nand.o, for instance
> 3 obj-$(CONFIG_MTD_NAND_BRCMNAND) += iproc_nand.o
> 4 obj-$(CONFIG_MTD_NAND_BRCMNAND) += bcm63138_nand.o
> 5 obj-$(CONFIG_MTD_NAND_BRCMNAND) += brcmstb_nand.o
> 6 obj-$(CONFIG_MTD_NAND_BRCMNAND) += brcmnand.o
> 
> Based on the order above, probe from iproc_nand should always be
> called first if a matching compatible string is found. If so, then
> why having both compatible strings "brcm,brcmnand" and
> "brcm,nand-iproc" causes issues for NS2 (I remember it broke
> smoketest in the past when you submitted the change)? I'm not saying
> we should have "brcm,brcmnand" for iProc devices, but I don't get
> why it would cause any issue.

FWIW, the above hack doesn't do anything if these are built as modules,
AFAICT. So I guess udev's (or similar) module rules would be another
point of failure here? Not that any of us probably care too much about
this driver as a module, but just throwing it out there...

I have a feeling we'll have to solve this locally, by avoiding having
"independent" drivers handling similar compatible properties, as I
expect (despite our expectation that compatible ordering should matter)
this problem will not be solved any time soon in the generic
infrastructure.

Or we can just use a hack (as Anup is doing) to leave off the
"brcm,brcmnand" compatibility property. Unless someone has brilliant
ideas, I guess we go with Anup's hack for now.

Brian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/2] arm64: dts: Add BRCM IPROC NAND DT node for NS2

[Ray Jui]

On 10/30/2015 11:49 AM, Brian Norris wrote:
> On Wed, Oct 28, 2015 at 09:08:02AM -0700, Ray Jui wrote:
>> On 10/28/2015 2:06 AM, Anup Patel wrote:
>>>
>>> I think for a newly created OF devices the Linux device driver framework will
>>> match the platform drivers in the order in which they are registered by module
>>> init functions. Now the order of module init function calls will be based how
>>> the .initcall section is formed by linker and order in which objects are linked.
>>>
>>
>> Yes, what you said is my understanding as well, but then here is the
>> mystery. This is the link order in brcmnand/Makefile:
>>
>> 1 # link order matters; don't link the more generic brcmstb_nand.o
>> before the
>> 2 # more specific iproc_nand.o, for instance
>> 3 obj-$(CONFIG_MTD_NAND_BRCMNAND) += iproc_nand.o
>> 4 obj-$(CONFIG_MTD_NAND_BRCMNAND) += bcm63138_nand.o
>> 5 obj-$(CONFIG_MTD_NAND_BRCMNAND) += brcmstb_nand.o
>> 6 obj-$(CONFIG_MTD_NAND_BRCMNAND) += brcmnand.o
>>
>> Based on the order above, probe from iproc_nand should always be
>> called first if a matching compatible string is found. If so, then
>> why having both compatible strings "brcm,brcmnand" and
>> "brcm,nand-iproc" causes issues for NS2 (I remember it broke
>> smoketest in the past when you submitted the change)? I'm not saying
>> we should have "brcm,brcmnand" for iProc devices, but I don't get
>> why it would cause any issue.
>
> FWIW, the above hack doesn't do anything if these are built as modules,
> AFAICT. So I guess udev's (or similar) module rules would be another
> point of failure here? Not that any of us probably care too much about
> this driver as a module, but just throwing it out there...
>
> I have a feeling we'll have to solve this locally, by avoiding having
> "independent" drivers handling similar compatible properties, as I
> expect (despite our expectation that compatible ordering should matter)
> this problem will not be solved any time soon in the generic
> infrastructure.
>
> Or we can just use a hack (as Anup is doing) to leave off the
> "brcm,brcmnand" compatibility property. Unless someone has brilliant
> ideas, I guess we go with Anup's hack for now.

I'm fine with that, :)

Ray

>
> Brian
>