[PATCH 0/5] NAND support for Broadcom NS2 SoC

[PATCH 0/5] 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" patchset and is available in ns2_nand_v1 branch of 
https://github.com/Broadcom/arm64-linux.git.

The patchset is tested on NS2 SVK.

Anup Patel (5):
  mtd: brcmnand: Fix pointer type-cast in brcmnand_write()
  mtd: nand: Allow MTD_NAND_BRCMNAND to be selected for ARM64
  mtd: brcmnand: Optional DT flag to reset IPROC NAND controller
  Documentation: dt-bindings: Add info about brcm,nand-iproc-reset DT
    flag
  arm64: dts: Add BRCM IPROC NAND DT node for NS2

 .../devicetree/bindings/mtd/brcm,brcmnand.txt         |  4 ++++
 arch/arm64/boot/dts/broadcom/ns2-svk.dts              | 12 ++++++++++++
 arch/arm64/boot/dts/broadcom/ns2.dtsi                 | 15 +++++++++++++++
 drivers/mtd/nand/Kconfig                              |  2 +-
 drivers/mtd/nand/brcmnand/brcmnand.c                  |  4 ++--
 drivers/mtd/nand/brcmnand/iproc_nand.c                | 19 +++++++++++++++++++
 6 files changed, 53 insertions(+), 3 deletions(-)

-- 
1.9.1

[PATCH 1/5] mtd: brcmnand: Fix pointer type-cast in brcmnand_write()

[Anup Patel]

We should always type-cast pointer to "long" or "unsigned long"
because size of pointer is same as machine word size. This will
avoid pointer type-cast issues on both 32bit and 64bit systems.

This patch fixes pointer type-cast issue in brcmnand_write()
as-per above info.

Signed-off-by: Anup Patel <anup.patel@broadcom.com>
Reviewed-by: Vikram Prakash <vikramp@broadcom.com>
Reviewed-by: Ray Jui <rjui@broadcom.com>
Reviewed-by: Scott Branden <sbranden@broadcom.com>
---
 drivers/mtd/nand/brcmnand/brcmnand.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/brcmnand/brcmnand.c b/drivers/mtd/nand/brcmnand/brcmnand.c
index fddb795..4cba03d 100644
--- a/drivers/mtd/nand/brcmnand/brcmnand.c
+++ b/drivers/mtd/nand/brcmnand/brcmnand.c
@@ -1544,9 +1544,9 @@ static int brcmnand_write(struct mtd_info *mtd, struct nand_chip *chip,
 
 	dev_dbg(ctrl->dev, "write %llx <- %p\n", (unsigned long long)addr, buf);
 
-	if (unlikely((u32)buf & 0x03)) {
+	if (unlikely((unsigned long)buf & 0x03)) {
 		dev_warn(ctrl->dev, "unaligned buffer: %p\n", buf);
-		buf = (u32 *)((u32)buf & ~0x03);
+		buf = (u32 *)((unsigned long)buf & ~0x03);
 	}
 
 	brcmnand_wp(mtd, 0);
-- 
1.9.1

[PATCH 2/5] mtd: nand: Allow MTD_NAND_BRCMNAND to be selected for ARM64

[Anup Patel]

The BRCM NAND driver can be re-used for Broadcom ARM64 SoCs hence
this patch updates Kconfig to allow selection of MTD_NAND_BRCMNAND
for ARM64.

Signed-off-by: Anup Patel <anup.patel@broadcom.com>
Reviewed-by: Vikram Prakash <vikramp@broadcom.com>
Reviewed-by: Ray Jui <rjui@broadcom.com>
Reviewed-by: Pramod KUMAR <pramodku@broadcom.com>
Reviewed-by: Scott Branden <sbranden@broadcom.com>
---
 drivers/mtd/nand/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
index 3324281..a1b5819 100644
--- a/drivers/mtd/nand/Kconfig
+++ b/drivers/mtd/nand/Kconfig
@@ -393,7 +393,7 @@ config MTD_NAND_GPMI_NAND
 
 config MTD_NAND_BRCMNAND
 	tristate "Broadcom STB NAND controller"
-	depends on ARM || MIPS
+	depends on ARM || ARM64 || MIPS
 	help
 	  Enables the Broadcom NAND controller driver. The controller was
 	  originally designed for Set-Top Box but is used on various BCM7xxx,
-- 
1.9.1

[PATCH 3/5] mtd: brcmnand: Optional DT flag to reset IPROC NAND controller

[Anup Patel]

The BRCM NAND controller on NS2 SoC requires a reset to
cleanup previously configured NAND controller state.

This patch adds optional boolean device tree flag named
"brcm,nand-iproc-reset". If this flag is present in NAND
controller DT node then BRCM IPROC NAND driver will reset
the NAND controller before any commands are issued.

Signed-off-by: Anup Patel <anup.patel@broadcom.com>
Reviewed-by: Pramod KUMAR <pramodku@broadcom.com>
Reviewed-by: Ray Jui <rjui@broadcom.com>
Reviewed-by: Scott Branden <sbranden@broadcom.com>
---
 drivers/mtd/nand/brcmnand/iproc_nand.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/mtd/nand/brcmnand/iproc_nand.c b/drivers/mtd/nand/brcmnand/iproc_nand.c
index 683495c..d837207 100644
--- a/drivers/mtd/nand/brcmnand/iproc_nand.c
+++ b/drivers/mtd/nand/brcmnand/iproc_nand.c
@@ -11,6 +11,7 @@
  * GNU General Public License for more details.
  */
 
+#include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/io.h>
 #include <linux/ioport.h>
@@ -35,6 +36,10 @@ struct iproc_nand_soc_priv {
 #define IPROC_NAND_APB_LE_MODE             BIT(24)
 #define IPROC_NAND_INT_CTRL_READ_ENABLE    BIT(6)
 
+#define IPROC_NAND_RESET_OFFSET            0x3f8
+#define IPROC_NAND_RESET_BIT_SHIFT         0
+#define IPROC_NAND_RESET_BIT               BIT(IPROC_NAND_RESET_BIT_SHIFT)
+
 static bool iproc_nand_intc_ack(struct brcmnand_soc *soc)
 {
 	struct iproc_nand_soc_priv *priv = soc->priv;
@@ -93,6 +98,7 @@ static void iproc_nand_apb_access(struct brcmnand_soc *soc, bool prepare)
 
 static int iproc_nand_probe(struct platform_device *pdev)
 {
+	u32 reset;
 	struct device *dev = &pdev->dev;
 	struct iproc_nand_soc_priv *priv;
 	struct brcmnand_soc *soc;
@@ -124,6 +130,19 @@ static int iproc_nand_probe(struct platform_device *pdev)
 	soc->ctlrdy_set_enabled = iproc_nand_intc_set;
 	soc->prepare_data_bus = iproc_nand_apb_access;
 
+	if (of_property_read_bool(dev->of_node, "brcm,nand-iproc-reset")) {
+		/* Put controller in reset and wait 10 usecs */
+		reset = readl(priv->idm_base + IPROC_NAND_RESET_OFFSET);
+		reset |= IPROC_NAND_RESET_BIT;
+		writel(reset, priv->idm_base + IPROC_NAND_RESET_OFFSET);
+		udelay(10);
+		/* Bring controller out of reset and wait 10 usecs */
+		reset = readl(priv->idm_base + IPROC_NAND_RESET_OFFSET);
+		reset &= ~IPROC_NAND_RESET_BIT;
+		writel(reset, priv->idm_base + IPROC_NAND_RESET_OFFSET);
+		udelay(10);
+	}
+
 	return brcmnand_probe(pdev, soc);
 }
 
-- 
1.9.1

[PATCH 4/5] Documentation: dt-bindings: Add info about brcm,nand-iproc-reset DT flag

[Anup Patel]

This patch updates the BRCM NAND controller DT bindings documentation
to add info about newly added optional flag "brcm,nand-iproc-reset".

Signed-off-by: Anup Patel <anup.patel@broadcom.com>
Reviewed-by: Pramod KUMAR <pramodku@broadcom.com>
Reviewed-by: Ray Jui <rjui@broadcom.com>
Reviewed-by: Scott Branden <sbranden@broadcom.com>
---
 Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt b/Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt
index 4ff7128..19b7a3c 100644
--- a/Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt
+++ b/Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt
@@ -78,6 +78,10 @@ we define additional 'compatible' properties and associated register resources w
        for interrupt status/ack.
      - reg-names: (required) a list of the names corresponding to the previous
        register ranges. Should contain "iproc-idm" and "iproc-ext".
+     - brcm,nand-iproc-reset: (optional) Some of the Broadcom IPROC SoCs
+       require NAND controller to be resetted for cleanup of previously
+       configured NAND controller state. This optional flag resets the
+       NAND controller once before any NAND commands are issued.
 
 
 * NAND chip-select
-- 
1.9.1

[PATCH 5/5] 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.

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 | 12 ++++++++++++
 arch/arm64/boot/dts/broadcom/ns2.dtsi    | 15 +++++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/arch/arm64/boot/dts/broadcom/ns2-svk.dts b/arch/arm64/boot/dts/broadcom/ns2-svk.dts
index e5950d5..a754160 100644
--- a/arch/arm64/boot/dts/broadcom/ns2-svk.dts
+++ b/arch/arm64/boot/dts/broadcom/ns2-svk.dts
@@ -63,5 +63,17 @@
 		uart3: serial@66130000 {
 			status = "ok";
 		};
+
+		nand: nand@66460000 {
+			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..55c3c5a 100644
--- a/arch/arm64/boot/dts/broadcom/ns2.dtsi
+++ b/arch/arm64/boot/dts/broadcom/ns2.dtsi
@@ -212,5 +212,20 @@
 			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-iproc-reset;
+			brcm,nand-has-wp;
+		};
 	};
 };
-- 
1.9.1

Re: [PATCH 3/5] mtd: brcmnand: Optional DT flag to reset IPROC NAND controller

[Brian Norris]

+ Rafal (to extend this mighty CC list)

On Fri, Oct 02, 2015 at 11:26:44PM +0530, Anup Patel wrote:
> The BRCM NAND controller on NS2 SoC requires a reset to
> cleanup previously configured NAND controller state.
> 
> This patch adds optional boolean device tree flag named
> "brcm,nand-iproc-reset". If this flag is present in NAND
> controller DT node then BRCM IPROC NAND driver will reset
> the NAND controller before any commands are issued.

Is there a reason not to do this reset unconditionally? I recall this
came up in discussion previously, when the OpenWRT folks were trying to
integrate with BCMA, where this reset was one of the few differences
between the platform-device-based driver (i.e., this one) and the BCMA
based driver. Might it help simplify things a bit if we just did the
same thing everywhere?

Brian

> Signed-off-by: Anup Patel <anup.patel@broadcom.com>
> Reviewed-by: Pramod KUMAR <pramodku@broadcom.com>
> Reviewed-by: Ray Jui <rjui@broadcom.com>
> Reviewed-by: Scott Branden <sbranden@broadcom.com>
> ---
>  drivers/mtd/nand/brcmnand/iproc_nand.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/mtd/nand/brcmnand/iproc_nand.c b/drivers/mtd/nand/brcmnand/iproc_nand.c
> index 683495c..d837207 100644
> --- a/drivers/mtd/nand/brcmnand/iproc_nand.c
> +++ b/drivers/mtd/nand/brcmnand/iproc_nand.c
> @@ -11,6 +11,7 @@
>   * GNU General Public License for more details.
>   */
>  
> +#include <linux/delay.h>
>  #include <linux/device.h>
>  #include <linux/io.h>
>  #include <linux/ioport.h>
> @@ -35,6 +36,10 @@ struct iproc_nand_soc_priv {
>  #define IPROC_NAND_APB_LE_MODE             BIT(24)
>  #define IPROC_NAND_INT_CTRL_READ_ENABLE    BIT(6)
>  
> +#define IPROC_NAND_RESET_OFFSET            0x3f8
> +#define IPROC_NAND_RESET_BIT_SHIFT         0
> +#define IPROC_NAND_RESET_BIT               BIT(IPROC_NAND_RESET_BIT_SHIFT)
> +
>  static bool iproc_nand_intc_ack(struct brcmnand_soc *soc)
>  {
>  	struct iproc_nand_soc_priv *priv = soc->priv;
> @@ -93,6 +98,7 @@ static void iproc_nand_apb_access(struct brcmnand_soc *soc, bool prepare)
>  
>  static int iproc_nand_probe(struct platform_device *pdev)
>  {
> +	u32 reset;
>  	struct device *dev = &pdev->dev;
>  	struct iproc_nand_soc_priv *priv;
>  	struct brcmnand_soc *soc;
> @@ -124,6 +130,19 @@ static int iproc_nand_probe(struct platform_device *pdev)
>  	soc->ctlrdy_set_enabled = iproc_nand_intc_set;
>  	soc->prepare_data_bus = iproc_nand_apb_access;
>  
> +	if (of_property_read_bool(dev->of_node, "brcm,nand-iproc-reset")) {
> +		/* Put controller in reset and wait 10 usecs */
> +		reset = readl(priv->idm_base + IPROC_NAND_RESET_OFFSET);
> +		reset |= IPROC_NAND_RESET_BIT;
> +		writel(reset, priv->idm_base + IPROC_NAND_RESET_OFFSET);
> +		udelay(10);
> +		/* Bring controller out of reset and wait 10 usecs */
> +		reset = readl(priv->idm_base + IPROC_NAND_RESET_OFFSET);
> +		reset &= ~IPROC_NAND_RESET_BIT;
> +		writel(reset, priv->idm_base + IPROC_NAND_RESET_OFFSET);
> +		udelay(10);
> +	}
> +
>  	return brcmnand_probe(pdev, soc);
>  }
>  
> -- 
> 1.9.1
> 

RE: [PATCH 3/5] mtd: brcmnand: Optional DT flag to reset IPROC NAND controller

[Anup Patel]

> -----Original Message-----
> From: Brian Norris [mailto:computersforpeace@gmail.com]
> Sent: 05 October 2015 03:20
> To: Anup Patel
> Cc: linux-arm-kernel@lists.infradead.org; Rob Herring; Pawel Moll; Mark
> Rutland; Ian Campbell; Kumar Gala; Catalin Marinas; Will Deacon; David
> Woodhouse; Ray Jui; Scott Branden; Florian Fainelli; Pramod Kumar; Vikram
> Prakash; Sandeep Tripathy; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; linux-mtd@lists.infradead.org; bcm-kernel-feedback-
> list; Rafal Milecki
> Subject: Re: [PATCH 3/5] mtd: brcmnand: Optional DT flag to reset IPROC NAND
> controller
> 
> + Rafal (to extend this mighty CC list)
> 
> On Fri, Oct 02, 2015 at 11:26:44PM +0530, Anup Patel wrote:
> > The BRCM NAND controller on NS2 SoC requires a reset to cleanup
> > previously configured NAND controller state.
> >
> > This patch adds optional boolean device tree flag named
> > "brcm,nand-iproc-reset". If this flag is present in NAND controller DT
> > node then BRCM IPROC NAND driver will reset the NAND controller before
> > any commands are issued.
> 
> Is there a reason not to do this reset unconditionally? I recall this came up in
> discussion previously, when the OpenWRT folks were trying to integrate with
> BCMA, where this reset was one of the few differences between the platform-
> device-based driver (i.e., this one) and the BCMA based driver. Might it help
> simplify things a bit if we just did the same thing everywhere?

This driver is currently shared by Cygnus and NS2.

We had similar suggestion when this patch was reviewed
internally in Broadcom.

The rationale for adding optional DT flag is as follows:
1. The NAND controller reset is currently required for NS2 only so
that it is in sane state before any NAND commands are issued. We
are not sure if Cygnus and all future iProc SoCs will require NAND
controller reset.
2. The NAND controller reset in probe would certainly increase
Linux boot time so for certain iProc SoCs we might choose avoid
NAND controller reset to reduce boot time if possible.

Regards
Anup

> 
> Brian
> 
> > Signed-off-by: Anup Patel <anup.patel@broadcom.com>
> > Reviewed-by: Pramod KUMAR <pramodku@broadcom.com>
> > Reviewed-by: Ray Jui <rjui@broadcom.com>
> > Reviewed-by: Scott Branden <sbranden@broadcom.com>
> > ---
> >  drivers/mtd/nand/brcmnand/iproc_nand.c | 19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> >
> > diff --git a/drivers/mtd/nand/brcmnand/iproc_nand.c
> > b/drivers/mtd/nand/brcmnand/iproc_nand.c
> > index 683495c..d837207 100644
> > --- a/drivers/mtd/nand/brcmnand/iproc_nand.c
> > +++ b/drivers/mtd/nand/brcmnand/iproc_nand.c
> > @@ -11,6 +11,7 @@
> >   * GNU General Public License for more details.
> >   */
> >
> > +#include <linux/delay.h>
> >  #include <linux/device.h>
> >  #include <linux/io.h>
> >  #include <linux/ioport.h>
> > @@ -35,6 +36,10 @@ struct iproc_nand_soc_priv {
> >  #define IPROC_NAND_APB_LE_MODE             BIT(24)
> >  #define IPROC_NAND_INT_CTRL_READ_ENABLE    BIT(6)
> >
> > +#define IPROC_NAND_RESET_OFFSET            0x3f8
> > +#define IPROC_NAND_RESET_BIT_SHIFT         0
> > +#define IPROC_NAND_RESET_BIT
> BIT(IPROC_NAND_RESET_BIT_SHIFT)
> > +
> >  static bool iproc_nand_intc_ack(struct brcmnand_soc *soc)  {
> >  	struct iproc_nand_soc_priv *priv = soc->priv; @@ -93,6 +98,7 @@
> > static void iproc_nand_apb_access(struct brcmnand_soc *soc, bool
> > prepare)
> >
> >  static int iproc_nand_probe(struct platform_device *pdev)  {
> > +	u32 reset;
> >  	struct device *dev = &pdev->dev;
> >  	struct iproc_nand_soc_priv *priv;
> >  	struct brcmnand_soc *soc;
> > @@ -124,6 +130,19 @@ static int iproc_nand_probe(struct platform_device
> *pdev)
> >  	soc->ctlrdy_set_enabled = iproc_nand_intc_set;
> >  	soc->prepare_data_bus = iproc_nand_apb_access;
> >
> > +	if (of_property_read_bool(dev->of_node, "brcm,nand-iproc-reset")) {
> > +		/* Put controller in reset and wait 10 usecs */
> > +		reset = readl(priv->idm_base + IPROC_NAND_RESET_OFFSET);
> > +		reset |= IPROC_NAND_RESET_BIT;
> > +		writel(reset, priv->idm_base + IPROC_NAND_RESET_OFFSET);
> > +		udelay(10);
> > +		/* Bring controller out of reset and wait 10 usecs */
> > +		reset = readl(priv->idm_base + IPROC_NAND_RESET_OFFSET);
> > +		reset &= ~IPROC_NAND_RESET_BIT;
> > +		writel(reset, priv->idm_base + IPROC_NAND_RESET_OFFSET);
> > +		udelay(10);
> > +	}
> > +
> >  	return brcmnand_probe(pdev, soc);
> >  }
> >
> > --
> > 1.9.1
> >

Re: [PATCH 3/5] mtd: brcmnand: Optional DT flag to reset IPROC NAND controller

[Brian Norris]

Hi Anup,

On Mon, Oct 05, 2015 at 06:27:16AM +0000, Anup Patel wrote:
> > -----Original Message-----
> > From: Brian Norris [mailto:computersforpeace@gmail.com]
> > Sent: 05 October 2015 03:20
> > To: Anup Patel
> > Cc: linux-arm-kernel@lists.infradead.org; Rob Herring; Pawel Moll; Mark
> > Rutland; Ian Campbell; Kumar Gala; Catalin Marinas; Will Deacon; David
> > Woodhouse; Ray Jui; Scott Branden; Florian Fainelli; Pramod Kumar; Vikram
> > Prakash; Sandeep Tripathy; devicetree@vger.kernel.org; linux-
> > kernel@vger.kernel.org; linux-mtd@lists.infradead.org; bcm-kernel-feedback-
> > list; Rafal Milecki
> > Subject: Re: [PATCH 3/5] mtd: brcmnand: Optional DT flag to reset IPROC NAND
> > controller
> > 
> > + Rafal (to extend this mighty CC list)
> > 
> > On Fri, Oct 02, 2015 at 11:26:44PM +0530, Anup Patel wrote:
> > > The BRCM NAND controller on NS2 SoC requires a reset to cleanup
> > > previously configured NAND controller state.
> > >
> > > This patch adds optional boolean device tree flag named
> > > "brcm,nand-iproc-reset". If this flag is present in NAND controller DT
> > > node then BRCM IPROC NAND driver will reset the NAND controller before
> > > any commands are issued.
> > 
> > Is there a reason not to do this reset unconditionally? I recall this came up in
> > discussion previously, when the OpenWRT folks were trying to integrate with
> > BCMA, where this reset was one of the few differences between the platform-
> > device-based driver (i.e., this one) and the BCMA based driver. Might it help
> > simplify things a bit if we just did the same thing everywhere?
> 
> This driver is currently shared by Cygnus and NS2.
> 
> We had similar suggestion when this patch was reviewed
> internally in Broadcom.
> 
> The rationale for adding optional DT flag is as follows:
> 1. The NAND controller reset is currently required for NS2 only so
> that it is in sane state before any NAND commands are issued. We
> are not sure if Cygnus and all future iProc SoCs will require NAND
> controller reset.

I'm not sure this is a very strong reason. It seems fairly reasonable in
general to reset a HW block before using it.

> 2. The NAND controller reset in probe would certainly increase
> Linux boot time so for certain iProc SoCs we might choose avoid
> NAND controller reset to reduce boot time if possible.

I recall this reason being mentioned before. I believe this only happens
because the brcmnand driver doesn't yet handle configuring the timing
registers, so iProc is implicitly relying on the bootloader to configure
the NAND timings. Perhaps it's time that we fix that. I'd rather not add
extra DT properties unless we actually need to [1]. And having proper
timing configuration in the Linux driver will help improve speeds for
all users (whose timings may not be configured in the bootloader).

I actually had some preliminary work to do some timing configuration
according to the new timing information from nand_base.c/nand_timing.c.
Unfortunately, I didn't complete this, and I'm no longer working at
Broadcom, so I don't exactly have access to the HW docs for all the NAND
controller revisions, nor do I have access to as much HW for testing...

Brian

[1] If we really do need a device tree differentiation, perhaps it would
be better to just differentiate the compatible string than to have
individual boolean properties. e.g.:

	compatible = "brcm,iproc-nand-ns2", ...;

Re: [PATCH 3/5] mtd: brcmnand: Optional DT flag to reset IPROC NAND controller

[Scott Branden]

Hi Brian,

On 15-10-06 06:41 AM, Brian Norris wrote:

>>>
>>> Is there a reason not to do this reset unconditionally? I recall this came up in
>>> discussion previously, when the OpenWRT folks were trying to integrate with
>>> BCMA, where this reset was one of the few differences between the platform-
>>> device-based driver (i.e., this one) and the BCMA based driver. Might it help
>>> simplify things a bit if we just did the same thing everywhere?
>>
>> This driver is currently shared by Cygnus and NS2.
>>
>> We had similar suggestion when this patch was reviewed
>> internally in Broadcom.
>>
>> The rationale for adding optional DT flag is as follows:
>> 1. The NAND controller reset is currently required for NS2 only so
>> that it is in sane state before any NAND commands are issued. We
>> are not sure if Cygnus and all future iProc SoCs will require NAND
>> controller reset.
>
> I'm not sure this is a very strong reason. It seems fairly reasonable in
> general to reset a HW block before using it.

Efficient Boot time is a very strong reason for needing this actually. 
We use the NAND controller in the bootROM, boot1/BL1, u-boot/UEFI, and 
then Kernel stage.  By properly initializing the controller once we do 
not need to reset it 4 different times.

>
>> 2. The NAND controller reset in probe would certainly increase
>> Linux boot time so for certain iProc SoCs we might choose avoid
>> NAND controller reset to reduce boot time if possible.
>
> I recall this reason being mentioned before. I believe this only happens
> because the brcmnand driver doesn't yet handle configuring the timing
> registers, so iProc is implicitly relying on the bootloader to configure
> the NAND timings. Perhaps it's time that we fix that. I'd rather not add
> extra DT properties unless we actually need to [1]. And having proper
> timing configuration in the Linux driver will help improve speeds for
> all users (whose timings may not be configured in the bootloader).

This is the very reason we need the optional reset property.  We need to 
have timings configured by the linux driver or not.  Yes, in some cases 
we will be relying on earlier boot stages to configure some of the hardware.

>
> I actually had some preliminary work to do some timing configuration
> according to the new timing information from nand_base.c/nand_timing.c.
> Unfortunately, I didn't complete this, and I'm no longer working at
> Broadcom, so I don't exactly have access to the HW docs for all the NAND
> controller revisions, nor do I have access to as much HW for testing...
>
> Brian
>
> [1] If we really do need a device tree differentiation, perhaps it would
> be better to just differentiate the compatible string than to have
> individual boolean properties. e.g.:
>
> 	compatible = "brcm,iproc-nand-ns2", ...;
>
As described above - the option is not SoC specific.  It is system 
specific.  In some systems we may wish to reset the NAND controller in 
linux.  In some we may wish to rely on initialization that has already 
been done to speed up boot times.



Regards,
  Scott

Re: [PATCH 3/5] mtd: brcmnand: Optional DT flag to reset IPROC NAND controller

[Florian Fainelli]

On 06/10/15 15:25, Scott Branden wrote:
> Hi Brian,
> 
> On 15-10-06 06:41 AM, Brian Norris wrote:
> 
>>>>
>>>> Is there a reason not to do this reset unconditionally? I recall
>>>> this came up in
>>>> discussion previously, when the OpenWRT folks were trying to
>>>> integrate with
>>>> BCMA, where this reset was one of the few differences between the
>>>> platform-
>>>> device-based driver (i.e., this one) and the BCMA based driver.
>>>> Might it help
>>>> simplify things a bit if we just did the same thing everywhere?
>>>
>>> This driver is currently shared by Cygnus and NS2.
>>>
>>> We had similar suggestion when this patch was reviewed
>>> internally in Broadcom.
>>>
>>> The rationale for adding optional DT flag is as follows:
>>> 1. The NAND controller reset is currently required for NS2 only so
>>> that it is in sane state before any NAND commands are issued. We
>>> are not sure if Cygnus and all future iProc SoCs will require NAND
>>> controller reset.
>>
>> I'm not sure this is a very strong reason. It seems fairly reasonable in
>> general to reset a HW block before using it.
> 
> Efficient Boot time is a very strong reason for needing this actually.
> We use the NAND controller in the bootROM, boot1/BL1, u-boot/UEFI, and
> then Kernel stage.  By properly initializing the controller once we do
> not need to reset it 4 different times.

This could be used as a reverse argument, issuing a reset will increase
the boot time.

> 
>>
>>> 2. The NAND controller reset in probe would certainly increase
>>> Linux boot time so for certain iProc SoCs we might choose avoid
>>> NAND controller reset to reduce boot time if possible.
>>
>> I recall this reason being mentioned before. I believe this only happens
>> because the brcmnand driver doesn't yet handle configuring the timing
>> registers, so iProc is implicitly relying on the bootloader to configure
>> the NAND timings. Perhaps it's time that we fix that. I'd rather not add
>> extra DT properties unless we actually need to [1]. And having proper
>> timing configuration in the Linux driver will help improve speeds for
>> all users (whose timings may not be configured in the bootloader).
> 
> This is the very reason we need the optional reset property.  We need to
> have timings configured by the linux driver or not.  Yes, in some cases
> we will be relying on earlier boot stages to configure some of the
> hardware.

Then instead of adding a "reset flag" to Device Tree, another approach
could be to put the desired or currently configured exhaustive list of
NAND timings in Device Tree, and based on that you could have this:

- the NAND controller driver finds that these timings match the current
configuration, you are good to go

- the NAND controller drivers finds a difference in how current timings
are configured vs. desired timings, and issues a controller reset, prior
to applying new timing configuration

- no timings are configured, reset the controller and use existing
auto-detection capabilities like ONFI modes

Typically you would put the desired timings instead of the currently
configured timings though..

> 
>>
>> I actually had some preliminary work to do some timing configuration
>> according to the new timing information from nand_base.c/nand_timing.c.
>> Unfortunately, I didn't complete this, and I'm no longer working at
>> Broadcom, so I don't exactly have access to the HW docs for all the NAND
>> controller revisions, nor do I have access to as much HW for testing...
>>
>> Brian
>>
>> [1] If we really do need a device tree differentiation, perhaps it would
>> be better to just differentiate the compatible string than to have
>> individual boolean properties. e.g.:
>>
>>     compatible = "brcm,iproc-nand-ns2", ...;
>>
> As described above - the option is not SoC specific.  It is system
> specific.  In some systems we may wish to reset the NAND controller in
> linux.  In some we may wish to rely on initialization that has already
> been done to speed up boot times.

It seems to me like having this property is fine as long as you are
describing that the controller *needs* a reset to operate properly, it
does not strike me as a particularly well suited property if its side
effect and main usage is to keep or wipe-out existing NAND timings.
-- 
Florian

RE: [PATCH 3/5] mtd: brcmnand: Optional DT flag to reset IPROC NAND controller

[Anup Patel]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 6142 bytes --]



> -----Original Message-----
> From: Florian Fainelli [mailto:f.fainelli@gmail.com]
> Sent: 07 October 2015 04:51
> To: Scott Branden; Brian Norris; Anup Patel
> Cc: linux-arm-kernel@lists.infradead.org; Rob Herring; Pawel Moll; Mark
> Rutland; Ian Campbell; Kumar Gala; Catalin Marinas; Will Deacon; David
> Woodhouse; Ray Jui; Florian Fainelli; Pramod Kumar; Vikram Prakash; Sandeep
> Tripathy; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
> mtd@lists.infradead.org; bcm-kernel-feedback-list; Rafal Milecki
> Subject: Re: [PATCH 3/5] mtd: brcmnand: Optional DT flag to reset IPROC NAND
> controller
> 
> On 06/10/15 15:25, Scott Branden wrote:
> > Hi Brian,
> >
> > On 15-10-06 06:41 AM, Brian Norris wrote:
> >
> >>>>
> >>>> Is there a reason not to do this reset unconditionally? I recall
> >>>> this came up in discussion previously, when the OpenWRT folks were
> >>>> trying to integrate with BCMA, where this reset was one of the few
> >>>> differences between the
> >>>> platform-
> >>>> device-based driver (i.e., this one) and the BCMA based driver.
> >>>> Might it help
> >>>> simplify things a bit if we just did the same thing everywhere?
> >>>
> >>> This driver is currently shared by Cygnus and NS2.
> >>>
> >>> We had similar suggestion when this patch was reviewed internally in
> >>> Broadcom.
> >>>
> >>> The rationale for adding optional DT flag is as follows:
> >>> 1. The NAND controller reset is currently required for NS2 only so
> >>> that it is in sane state before any NAND commands are issued. We are
> >>> not sure if Cygnus and all future iProc SoCs will require NAND
> >>> controller reset.
> >>
> >> I'm not sure this is a very strong reason. It seems fairly reasonable
> >> in general to reset a HW block before using it.
> >
> > Efficient Boot time is a very strong reason for needing this actually.
> > We use the NAND controller in the bootROM, boot1/BL1, u-boot/UEFI, and
> > then Kernel stage.  By properly initializing the controller once we do
> > not need to reset it 4 different times.
> 
> This could be used as a reverse argument, issuing a reset will increase the boot
> time.
> 
> >
> >>
> >>> 2. The NAND controller reset in probe would certainly increase Linux
> >>> boot time so for certain iProc SoCs we might choose avoid NAND
> >>> controller reset to reduce boot time if possible.
> >>
> >> I recall this reason being mentioned before. I believe this only
> >> happens because the brcmnand driver doesn't yet handle configuring
> >> the timing registers, so iProc is implicitly relying on the
> >> bootloader to configure the NAND timings. Perhaps it's time that we
> >> fix that. I'd rather not add extra DT properties unless we actually
> >> need to [1]. And having proper timing configuration in the Linux
> >> driver will help improve speeds for all users (whose timings may not be
> configured in the bootloader).
> >
> > This is the very reason we need the optional reset property.  We need
> > to have timings configured by the linux driver or not.  Yes, in some
> > cases we will be relying on earlier boot stages to configure some of
> > the hardware.
> 
> Then instead of adding a "reset flag" to Device Tree, another approach could be
> to put the desired or currently configured exhaustive list of NAND timings in
> Device Tree, and based on that you could have this:
> 
> - the NAND controller driver finds that these timings match the current
> configuration, you are good to go
> 
> - the NAND controller drivers finds a difference in how current timings are
> configured vs. desired timings, and issues a controller reset, prior to applying
> new timing configuration

To add to this ...

The mechanism to reset is BRCM NAND controller is SOC specific so the
SoC independent BRCM NAND driver (i.e. brcmnand.c) does not know how
to reset the NAND controller.

For iProc SoC family, the NAND controller reset is through IDM register
space which is only iomap'ed by iproc_nand.c.

We might end-up having one more SoC specific callback which will be
Provided by iproc_nand.c to brcmnand.c.

> 
> - no timings are configured, reset the controller and use existing auto-detection
> capabilities like ONFI modes
> 
> Typically you would put the desired timings instead of the currently configured
> timings though..

Overall, it would good to support timing parameters through DT or ONFI but
for now have we can rely on reset and auto-devid configuration.

> 
> >
> >>
> >> I actually had some preliminary work to do some timing configuration
> >> according to the new timing information from nand_base.c/nand_timing.c.
> >> Unfortunately, I didn't complete this, and I'm no longer working at
> >> Broadcom, so I don't exactly have access to the HW docs for all the
> >> NAND controller revisions, nor do I have access to as much HW for testing...
> >>
> >> Brian
> >>
> >> [1] If we really do need a device tree differentiation, perhaps it
> >> would be better to just differentiate the compatible string than to
> >> have individual boolean properties. e.g.:
> >>
> >>     compatible = "brcm,iproc-nand-ns2", ...;
> >>
> > As described above - the option is not SoC specific.  It is system
> > specific.  In some systems we may wish to reset the NAND controller in
> > linux.  In some we may wish to rely on initialization that has already
> > been done to speed up boot times.
> 
> It seems to me like having this property is fine as long as you are describing that
> the controller *needs* a reset to operate properly, it does not strike me as a
> particularly well suited property if its side effect and main usage is to keep or
> wipe-out existing NAND timings.

IMHO, having SoC specific compatible string for NS2 is like saying
NAND controller on NS2 is different from other iProc SoCs whereas
Having optional DT flags for quirks/work-arounds (e.g. NAND controller
reset) is like saying NAND controller on NS2 same as other iProc SoCs
but some additional programming is required. 

--
Anup
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 1/5] mtd: brcmnand: Fix pointer type-cast in brcmnand_write()

[Brian Norris]

On Fri, Oct 02, 2015 at 11:26:42PM +0530, Anup Patel wrote:
> We should always type-cast pointer to "long" or "unsigned long"
> because size of pointer is same as machine word size. This will
> avoid pointer type-cast issues on both 32bit and 64bit systems.
> 
> This patch fixes pointer type-cast issue in brcmnand_write()
> as-per above info.
> 
> Signed-off-by: Anup Patel <anup.patel@broadcom.com>
> Reviewed-by: Vikram Prakash <vikramp@broadcom.com>
> Reviewed-by: Ray Jui <rjui@broadcom.com>
> Reviewed-by: Scott Branden <sbranden@broadcom.com>
> ---
>  drivers/mtd/nand/brcmnand/brcmnand.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/nand/brcmnand/brcmnand.c b/drivers/mtd/nand/brcmnand/brcmnand.c
> index fddb795..4cba03d 100644
> --- a/drivers/mtd/nand/brcmnand/brcmnand.c
> +++ b/drivers/mtd/nand/brcmnand/brcmnand.c
> @@ -1544,9 +1544,9 @@ static int brcmnand_write(struct mtd_info *mtd, struct nand_chip *chip,
>  
>  	dev_dbg(ctrl->dev, "write %llx <- %p\n", (unsigned long long)addr, buf);
>  
> -	if (unlikely((u32)buf & 0x03)) {
> +	if (unlikely((unsigned long)buf & 0x03)) {
>  		dev_warn(ctrl->dev, "unaligned buffer: %p\n", buf);
> -		buf = (u32 *)((u32)buf & ~0x03);
> +		buf = (u32 *)((unsigned long)buf & ~0x03);
>  	}

Hmm, this block isn't really good. We should probably work to remove
this. Not sure why I left that in, actually. At a minimum, it'd probably
good to make this noisier, so if we see this in practice, we can work to
fix those cases.

But anyway, the patch is good (though 'uintptr_t' might be slightly
better), so pushed to l2-mtd.git.

Brian

>  
>  	brcmnand_wp(mtd, 0);
> -- 
> 1.9.1
> 

Re: [PATCH 2/5] mtd: nand: Allow MTD_NAND_BRCMNAND to be selected for ARM64

[Brian Norris]

On Fri, Oct 02, 2015 at 11:26:43PM +0530, Anup Patel wrote:
> The BRCM NAND driver can be re-used for Broadcom ARM64 SoCs hence
> this patch updates Kconfig to allow selection of MTD_NAND_BRCMNAND
> for ARM64.
> 
> Signed-off-by: Anup Patel <anup.patel@broadcom.com>
> Reviewed-by: Vikram Prakash <vikramp@broadcom.com>
> Reviewed-by: Ray Jui <rjui@broadcom.com>
> Reviewed-by: Pramod KUMAR <pramodku@broadcom.com>
> Reviewed-by: Scott Branden <sbranden@broadcom.com>

Pushed to l2-mtd.git

Re: [PATCH 3/5] mtd: brcmnand: Optional DT flag to reset IPROC NAND controller

[Brian Norris]

Hi Anup,

On Wed, Oct 07, 2015 at 03:33:50AM +0000, Anup Patel wrote:
> > -----Original Message-----
> > From: Florian Fainelli [mailto:f.fainelli@gmail.com]
> > 
> > On 06/10/15 15:25, Scott Branden wrote:
> > 
> > Then instead of adding a "reset flag" to Device Tree, another approach could be
> > to put the desired or currently configured exhaustive list of NAND timings in
> > Device Tree, and based on that you could have this:
> > 
> > - the NAND controller driver finds that these timings match the current
> > configuration, you are good to go
> > 
> > - the NAND controller drivers finds a difference in how current timings are
> > configured vs. desired timings, and issues a controller reset, prior to applying
> > new timing configuration
> 
> To add to this ...
> 
> The mechanism to reset is BRCM NAND controller is SOC specific so the
> SoC independent BRCM NAND driver (i.e. brcmnand.c) does not know how
> to reset the NAND controller.
> 
> For iProc SoC family, the NAND controller reset is through IDM register
> space which is only iomap'ed by iproc_nand.c.
> 
> We might end-up having one more SoC specific callback which will be
> Provided by iproc_nand.c to brcmnand.c.
> 
> > 
> > - no timings are configured, reset the controller and use existing auto-detection
> > capabilities like ONFI modes
> > 
> > Typically you would put the desired timings instead of the currently configured
> > timings though..
> 
> Overall, it would good to support timing parameters through DT or ONFI but
> for now have we can rely on reset and auto-devid configuration.

I don't want to support a DT property that is only used as a workaround
for the right solution. That means the property may quickly become
obsolete, yet we have to support it forever.


> > >>     compatible = "brcm,iproc-nand-ns2", ...;
> > >>
> > > As described above - the option is not SoC specific.  It is system
> > > specific.  In some systems we may wish to reset the NAND controller in
> > > linux.  In some we may wish to rely on initialization that has already
> > > been done to speed up boot times.
> > 
> > It seems to me like having this property is fine as long as you are describing that
> > the controller *needs* a reset to operate properly, it does not strike me as a
> > particularly well suited property if its side effect and main usage is to keep or
> > wipe-out existing NAND timings.
> 
> IMHO, having SoC specific compatible string for NS2 is like saying
> NAND controller on NS2 is different from other iProc SoCs whereas
> Having optional DT flags for quirks/work-arounds (e.g. NAND controller
> reset) is like saying NAND controller on NS2 same as other iProc SoCs
> but some additional programming is required. 

OK... so what is the reason that you have to reset the controller on NS2
and not Cygnus? Is it a SoC difference (i.e., compatible string)?
Firmware/bootloader difference? So far, all statements have been
non-specific, AFAICT.

Brian

Re: [PATCH 3/5] mtd: brcmnand: Optional DT flag to reset IPROC NAND controller

[Josh Cartwright]

[-- Attachment #1: Type: text/plain, Size: 1492 bytes --]

On Wed, Oct 07, 2015 at 03:33:50AM +0000, Anup Patel wrote:
> From: Florian Fainelli [mailto:f.fainelli@gmail.com]
> > On 06/10/15 15:25, Scott Branden wrote:
[..]
> > Then instead of adding a "reset flag" to Device Tree, another approach could be
> > to put the desired or currently configured exhaustive list of NAND timings in
> > Device Tree, and based on that you could have this:
> > 
> > - the NAND controller driver finds that these timings match the current
> > configuration, you are good to go
> > 
> > - the NAND controller drivers finds a difference in how current timings are
> > configured vs. desired timings, and issues a controller reset, prior to applying
> > new timing configuration
>
> To add to this ...
>
> The mechanism to reset is BRCM NAND controller is SOC specific so the
> SoC independent BRCM NAND driver (i.e. brcmnand.c) does not know how
> to reset the NAND controller.
>
> For iProc SoC family, the NAND controller reset is through IDM register
> space which is only iomap'ed by iproc_nand.c.
>
> We might end-up having one more SoC specific callback which will be
> Provided by iproc_nand.c to brcmnand.c.

Not that I'm familiar with these SoCs, but I did want to chime in and
make sure you are aware of the existing reset_controller_dev
abstraction, which is intended to solve exactly this problem.  Including
a reset_control_get_optional() that might fit your use case.  See
include/linux/reset{,-controller}.h.

  Josh

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 3/5] mtd: brcmnand: Optional DT flag to reset IPROC NAND controller

[Florian Fainelli]

On 12/10/15 14:54, Josh Cartwright wrote:
> On Wed, Oct 07, 2015 at 03:33:50AM +0000, Anup Patel wrote:
>> From: Florian Fainelli [mailto:f.fainelli@gmail.com]
>>> On 06/10/15 15:25, Scott Branden wrote:
> [..]
>>> Then instead of adding a "reset flag" to Device Tree, another approach could be
>>> to put the desired or currently configured exhaustive list of NAND timings in
>>> Device Tree, and based on that you could have this:
>>>
>>> - the NAND controller driver finds that these timings match the current
>>> configuration, you are good to go
>>>
>>> - the NAND controller drivers finds a difference in how current timings are
>>> configured vs. desired timings, and issues a controller reset, prior to applying
>>> new timing configuration
>>
>> To add to this ...
>>
>> The mechanism to reset is BRCM NAND controller is SOC specific so the
>> SoC independent BRCM NAND driver (i.e. brcmnand.c) does not know how
>> to reset the NAND controller.
>>
>> For iProc SoC family, the NAND controller reset is through IDM register
>> space which is only iomap'ed by iproc_nand.c.
>>
>> We might end-up having one more SoC specific callback which will be
>> Provided by iproc_nand.c to brcmnand.c.
> 
> Not that I'm familiar with these SoCs, but I did want to chime in and
> make sure you are aware of the existing reset_controller_dev
> abstraction, which is intended to solve exactly this problem.  Including
> a reset_control_get_optional() that might fit your use case.  See
> include/linux/reset{,-controller}.h.

I almost suggested that, and then looked more closely at where this
reset register is located, and it happens to be in the NAND controller
itself (IPROC IDM which is the iProc SHIM to the NAND controller), so
coming up with a reset controller driver and a reset controller consumer
for that simple use case sounds both unnecessary and complex.

The core of the discussion is about disguising this NAND controller
reset as a way to preserve previously configured NAND timings, which is
at best a hack and an unstated dependency with the firmware.
-- 
Florian

RE: [PATCH 3/5] mtd: brcmnand: Optional DT flag to reset IPROC NAND controller

[Anup Patel]

Hi Brian,

> -----Original Message-----
> From: Brian Norris [mailto:computersforpeace@gmail.com]
> Sent: 13 October 2015 02:58
> To: Anup Patel
> Cc: Florian Fainelli; Scott Branden; linux-arm-kernel@lists.infradead.org; Rob
> Herring; Pawel Moll; Mark Rutland; Ian Campbell; Kumar Gala; Catalin Marinas;
> Will Deacon; David Woodhouse; Ray Jui; Pramod Kumar; Vikram Prakash;
> Sandeep Tripathy; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> linux-mtd@lists.infradead.org; bcm-kernel-feedback-list; Rafal Milecki
> Subject: Re: [PATCH 3/5] mtd: brcmnand: Optional DT flag to reset IPROC NAND
> controller
> 
> Hi Anup,
> 
> On Wed, Oct 07, 2015 at 03:33:50AM +0000, Anup Patel wrote:
> > > -----Original Message-----
> > > From: Florian Fainelli [mailto:f.fainelli@gmail.com]
> > >
> > > On 06/10/15 15:25, Scott Branden wrote:
> > >
> > > Then instead of adding a "reset flag" to Device Tree, another
> > > approach could be to put the desired or currently configured
> > > exhaustive list of NAND timings in Device Tree, and based on that you could
> have this:
> > >
> > > - the NAND controller driver finds that these timings match the
> > > current configuration, you are good to go
> > >
> > > - the NAND controller drivers finds a difference in how current
> > > timings are configured vs. desired timings, and issues a controller
> > > reset, prior to applying new timing configuration
> >
> > To add to this ...
> >
> > The mechanism to reset is BRCM NAND controller is SOC specific so the
> > SoC independent BRCM NAND driver (i.e. brcmnand.c) does not know how
> > to reset the NAND controller.
> >
> > For iProc SoC family, the NAND controller reset is through IDM
> > register space which is only iomap'ed by iproc_nand.c.
> >
> > We might end-up having one more SoC specific callback which will be
> > Provided by iproc_nand.c to brcmnand.c.
> >
> > >
> > > - no timings are configured, reset the controller and use existing
> > > auto-detection capabilities like ONFI modes
> > >
> > > Typically you would put the desired timings instead of the currently
> > > configured timings though..
> >
> > Overall, it would good to support timing parameters through DT or ONFI
> > but for now have we can rely on reset and auto-devid configuration.
> 
> I don't want to support a DT property that is only used as a workaround for the
> right solution. That means the property may quickly become obsolete, yet we
> have to support it forever.
> 
> 
> > > >>     compatible = "brcm,iproc-nand-ns2", ...;
> > > >>
> > > > As described above - the option is not SoC specific.  It is system
> > > > specific.  In some systems we may wish to reset the NAND
> > > > controller in linux.  In some we may wish to rely on
> > > > initialization that has already been done to speed up boot times.
> > >
> > > It seems to me like having this property is fine as long as you are
> > > describing that the controller *needs* a reset to operate properly,
> > > it does not strike me as a particularly well suited property if its
> > > side effect and main usage is to keep or wipe-out existing NAND timings.
> >
> > IMHO, having SoC specific compatible string for NS2 is like saying
> > NAND controller on NS2 is different from other iProc SoCs whereas
> > Having optional DT flags for quirks/work-arounds (e.g. NAND controller
> > reset) is like saying NAND controller on NS2 same as other iProc SoCs
> > but some additional programming is required.
> 
> OK... so what is the reason that you have to reset the controller on NS2 and not
> Cygnus? Is it a SoC difference (i.e., compatible string)?
> Firmware/bootloader difference? So far, all statements have been non-specific,
> AFAICT.
> 

On NS2 SVK, we have 16bit NAND chip whereas on all Cygnus SVKs we mostly
have 8bit NAND chip.

The bootloader on NS2 touches NAND controller and configures it to 16bit mode.
When we reach BRCMNAND driver probing on NS2, the BRCMNAND controller is
already in 16bit mode so NAND READID command does not work. On Cygnus,
we mostly have 8bit NAND chip so BRCMNAND controller is always in 8bit mode
so we don't see any issue with NAND READID command.

We really don't require to reset BRCNNAND controller on NS2 to get NAND
READID command working. Instead, we can simply force 8bit mode before
we do nand_scan_ident() for each CS. This will be a much simpler fix for all
versions of BRCMNAND because NAND READID command will only work
in 8bit mode irrespective to BRCMNAND version (NAND controllers from
other vendors might also have similar issue with NAND READID command).

I will send a revised patchset which will fix brcmnand_init_cs() as-per above.

Best Regards,
Anup