[PATCH v3 00/10] mtd: rawnand: brcmnand: driver and doc updates

[PATCH v3 00/10] mtd: rawnand: brcmnand: driver and doc updates

[David Regan]

This patch series is an update from the previous version [1] after
exex_op support and fixes (patch 1 to 4 from the previous version.)

It updates all the BCMBCA SoC to support the nand controller and add
functions to handle BCMBCA specific needs on ECC and Write Protection
usage. The device tree document is also updated accordingly with the new
properties needed by the driver.

In addition there is a bug fix for exec_op helper functions and on-die ECC.

[1] https://lore.kernel.org/lkml/20230606231252.94838-1-william.zhang@broadcom.com/

Changes in v3:
- Update brcm,nand-use-wp description
- Revert the description change to BCM63168 SoC-specific NAND controller
- Updated bcmbca_read_data_bus comment

Changes in v2:
- Revert the new compatible string nand-bcmbca
- Drop the BCM63168 compatible fix to avoid any potential ABI
Incompatibility issue
- Simplify the explanation for brcm,nand-use-wp
- Keep the interrupt name requirement when interrupt number is specified
- Add nand controller node label for 4908 so it is consistent with other
SoC's and can be referenced by board dts file
- Drop the is_param argument to the read data bus function now that we
have the exec_op API to read the parameter page and ONFI data
- Minor cosmetic fixes
- Added patches 8, 9, 10 to patch series

William Zhang (7):
  dt-bindings: mtd: brcmnand: Updates for bcmbca SoCs
  ARM: dts: broadcom: bcmbca: Add NAND controller node
  arm64: dts: broadcom: bcmbca: Add NAND controller node
  mtd: rawnand: brcmnand: Rename bcm63138 nand driver
  mtd: rawnand: brcmnand: Add BCMBCA read data bus interface
  mtd: rawnand: brcmnand: Add support for getting ecc setting from strap
  mtd: rawnand: brcmnand: Support write protection setting from dts

David Regan (3):
  mtd: rawnand: brcmnand: exec_op helper functions return type fixes
  mtd: rawnand: brcmnand: update log level messages
  mtd: rawnand: brcmnand: allow for on-die ecc

 .../bindings/mtd/brcm,brcmnand.yaml           |  37 ++++-
 arch/arm/boot/dts/broadcom/bcm47622.dtsi      |  17 ++
 arch/arm/boot/dts/broadcom/bcm63138.dtsi      |  10 +-
 arch/arm/boot/dts/broadcom/bcm63148.dtsi      |  17 ++
 arch/arm/boot/dts/broadcom/bcm63178.dtsi      |  17 ++
 arch/arm/boot/dts/broadcom/bcm6756.dtsi       |  17 ++
 arch/arm/boot/dts/broadcom/bcm6846.dtsi       |  17 ++
 arch/arm/boot/dts/broadcom/bcm6855.dtsi       |  17 ++
 arch/arm/boot/dts/broadcom/bcm6878.dtsi       |  17 ++
 arch/arm/boot/dts/broadcom/bcm947622.dts      |   4 +
 arch/arm/boot/dts/broadcom/bcm963138.dts      |   4 +
 arch/arm/boot/dts/broadcom/bcm963138dvt.dts   |  12 +-
 arch/arm/boot/dts/broadcom/bcm963148.dts      |   4 +
 arch/arm/boot/dts/broadcom/bcm963178.dts      |   4 +
 arch/arm/boot/dts/broadcom/bcm96756.dts       |   4 +
 arch/arm/boot/dts/broadcom/bcm96846.dts       |   4 +
 arch/arm/boot/dts/broadcom/bcm96855.dts       |   4 +
 arch/arm/boot/dts/broadcom/bcm96878.dts       |   4 +
 .../boot/dts/broadcom/bcmbca/bcm4908.dtsi     |   5 +-
 .../boot/dts/broadcom/bcmbca/bcm4912.dtsi     |  17 ++
 .../boot/dts/broadcom/bcmbca/bcm63146.dtsi    |  17 ++
 .../boot/dts/broadcom/bcmbca/bcm63158.dtsi    |  17 ++
 .../boot/dts/broadcom/bcmbca/bcm6813.dtsi     |  17 ++
 .../boot/dts/broadcom/bcmbca/bcm6856.dtsi     |  17 ++
 .../boot/dts/broadcom/bcmbca/bcm6858.dtsi     |  17 ++
 .../boot/dts/broadcom/bcmbca/bcm94912.dts     |   4 +
 .../boot/dts/broadcom/bcmbca/bcm963146.dts    |   4 +
 .../boot/dts/broadcom/bcmbca/bcm963158.dts    |   4 +
 .../boot/dts/broadcom/bcmbca/bcm96813.dts     |   4 +
 .../boot/dts/broadcom/bcmbca/bcm96856.dts     |   4 +
 .../boot/dts/broadcom/bcmbca/bcm96858.dts     |   4 +
 drivers/mtd/nand/raw/brcmnand/Makefile        |   2 +-
 drivers/mtd/nand/raw/brcmnand/bcm63138_nand.c |  99 ------------
 drivers/mtd/nand/raw/brcmnand/bcmbca_nand.c   | 126 +++++++++++++++
 drivers/mtd/nand/raw/brcmnand/brcmnand.c      | 146 +++++++++++++++---
 drivers/mtd/nand/raw/brcmnand/brcmnand.h      |   2 +
 36 files changed, 578 insertions(+), 138 deletions(-)
 delete mode 100644 drivers/mtd/nand/raw/brcmnand/bcm63138_nand.c
 create mode 100644 drivers/mtd/nand/raw/brcmnand/bcmbca_nand.c

-- 
2.37.3

[PATCH v3 01/10] dt-bindings: mtd: brcmnand: Updates for bcmbca SoCs

[David Regan]

From: William Zhang <william.zhang@broadcom.com>

Update the descriptions to reflect different families of broadband SoC and
use the general name bcmbca for ARM based SoC.

Add brcm,nand-use-wp property to have an option for disabling this
feature on broadband board design that does not use write protection.

Add brcm,nand-ecc-use-strap to get ecc setting from board boot strap for
broadband board designs because they do not specify ecc setting in dts
but rather using the strap setting.

Remove the requirement of interrupts property to reflect the driver
code. Also add myself to the list of maintainers.

Signed-off-by: William Zhang <william.zhang@broadcom.com>
Reviewed-by: David Regan <dregan@broadcom.com>
---
Changes in v3:
- Update brcm,nand-use-wp description
- Revert the description change to BCM63168 SoC-specific NAND controller
---
Changes in v2:
- Revert the new compatible string nand-bcmbca
- Drop the BCM63168 compatible fix to avoid any potential ABI
incompatibility issue
- Simplify the explanation for brcm,nand-use-wp
- Keep the interrupt name requirement when interrupt number is specified
---
 .../bindings/mtd/brcm,brcmnand.yaml           | 37 ++++++++++++++++---
 1 file changed, 32 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/mtd/brcm,brcmnand.yaml b/Documentation/devicetree/bindings/mtd/brcm,brcmnand.yaml
index f57e96374e67..752c6ee98a7d 100644
--- a/Documentation/devicetree/bindings/mtd/brcm,brcmnand.yaml
+++ b/Documentation/devicetree/bindings/mtd/brcm,brcmnand.yaml
@@ -9,6 +9,7 @@ title: Broadcom STB NAND Controller
 maintainers:
   - Brian Norris <computersforpeace@gmail.com>
   - Kamal Dasu <kdasu.kdev@gmail.com>
+  - William Zhang <william.zhang@broadcom.com>
 
 description: |
   The Broadcom Set-Top Box NAND controller supports low-level access to raw NAND
@@ -18,9 +19,10 @@ description: |
   supports basic PROGRAM and READ functions, among other features.
 
   This controller was originally designed for STB SoCs (BCM7xxx) but is now
-  available on a variety of Broadcom SoCs, including some BCM3xxx, BCM63xx, and
-  iProc/Cygnus. Its history includes several similar (but not fully register
-  compatible) versions.
+  available on a variety of Broadcom SoCs, including some BCM3xxx, MIPS based
+  Broadband SoC (BCM63xx), ARM based Broadband SoC (BCMBCA) and iProc/Cygnus.
+  Its history includes several similar (but not fully register compatible)
+  versions.
 
   -- Additional SoC-specific NAND controller properties --
 
@@ -53,7 +55,7 @@ properties:
               - brcm,brcmnand-v7.2
               - brcm,brcmnand-v7.3
           - const: brcm,brcmnand
-      - description: BCM63138 SoC-specific NAND controller
+      - description: BCMBCA SoC-specific NAND controller
         items:
           - const: brcm,nand-bcm63138
           - enum:
@@ -111,6 +113,20 @@ properties:
       earlier versions of this core that include WP
     type: boolean
 
+  brcm,nand-use-wp:
+    description:
+      Use this property to indicate if board design uses
+      controller's write protection feature and connects its
+      NAND_WPb pin to nand chip's WP_L pin. By default the driver
+      uses a module parameter with default value set to enable to
+      control this feature for all boards. Use this dts property to
+      override the default behavior and enable/disable this feature
+      through board dts on a per board basis.
+      Set to 0 if WP pins are not connected and feature is not
+      used. Set to 1 if WP pins are connected and feature is used.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [0, 1]
+
 patternProperties:
   "^nand@[a-f0-9]$":
     type: object
@@ -137,6 +153,16 @@ patternProperties:
           layout.
         $ref: /schemas/types.yaml#/definitions/uint32
 
+      brcm,nand-ecc-use-strap:
+        description:
+          This flag is used by the driver to get the ecc strength and
+          spare area size from the SoC NAND boot strap setting. This
+          is commonly used by the BCMBCA SoC board design. If ecc
+          strength and spare area size are set by nand-ecc-strength
+          and brcm,nand-oob-sector-size in the dts, these settings
+          have precedence and override this flag.
+        $ref: /schemas/types.yaml#/definitions/flag
+
     unevaluatedProperties: false
 
 allOf:
@@ -177,6 +203,8 @@ allOf:
             - const: iproc-idm
             - const: iproc-ext
   - if:
+      required:
+        - interrupts
       properties:
         interrupts:
           minItems: 2
@@ -189,7 +217,6 @@ unevaluatedProperties: false
 required:
   - reg
   - reg-names
-  - interrupts
 
 examples:
   - |
-- 
2.37.3

[PATCH v3 02/10] ARM: dts: broadcom: bcmbca: Add NAND controller node

[David Regan]

From: William Zhang <william.zhang@broadcom.com>

Add support for Broadcom STB NAND controller in BCMBCA ARMv7 chip dts
files.

Signed-off-by: William Zhang <william.zhang@broadcom.com>
Reviewed-by: David Regan <dregan@broadcom.com>
---
Changes in v3: None
---
Changes in v2: None
---
 arch/arm/boot/dts/broadcom/bcm47622.dtsi    | 17 +++++++++++++++++
 arch/arm/boot/dts/broadcom/bcm63138.dtsi    | 10 +++++++++-
 arch/arm/boot/dts/broadcom/bcm63148.dtsi    | 17 +++++++++++++++++
 arch/arm/boot/dts/broadcom/bcm63178.dtsi    | 17 +++++++++++++++++
 arch/arm/boot/dts/broadcom/bcm6756.dtsi     | 17 +++++++++++++++++
 arch/arm/boot/dts/broadcom/bcm6846.dtsi     | 17 +++++++++++++++++
 arch/arm/boot/dts/broadcom/bcm6855.dtsi     | 17 +++++++++++++++++
 arch/arm/boot/dts/broadcom/bcm6878.dtsi     | 17 +++++++++++++++++
 arch/arm/boot/dts/broadcom/bcm947622.dts    |  4 ++++
 arch/arm/boot/dts/broadcom/bcm963138.dts    |  4 ++++
 arch/arm/boot/dts/broadcom/bcm963138dvt.dts | 12 +++++-------
 arch/arm/boot/dts/broadcom/bcm963148.dts    |  4 ++++
 arch/arm/boot/dts/broadcom/bcm963178.dts    |  4 ++++
 arch/arm/boot/dts/broadcom/bcm96756.dts     |  4 ++++
 arch/arm/boot/dts/broadcom/bcm96846.dts     |  4 ++++
 arch/arm/boot/dts/broadcom/bcm96855.dts     |  4 ++++
 arch/arm/boot/dts/broadcom/bcm96878.dts     |  4 ++++
 17 files changed, 165 insertions(+), 8 deletions(-)

diff --git a/arch/arm/boot/dts/broadcom/bcm47622.dtsi b/arch/arm/boot/dts/broadcom/bcm47622.dtsi
index 7cd38de118c3..55ff18043d96 100644
--- a/arch/arm/boot/dts/broadcom/bcm47622.dtsi
+++ b/arch/arm/boot/dts/broadcom/bcm47622.dtsi
@@ -138,6 +138,23 @@ hsspi: spi@1000 {
 			status = "disabled";
 		};
 
+		nand_controller: nand-controller@1800 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			compatible = "brcm,nand-bcm63138", "brcm,brcmnand-v7.1", "brcm,brcmnand";
+			reg = <0x1800 0x600>, <0x2000 0x10>;
+			reg-names = "nand", "nand-int-base";
+			brcm,nand-use-wp = <0>;
+			status = "disabled";
+
+			nandcs: nand@0 {
+				compatible = "brcm,nandcs";
+				reg = <0>;
+				nand-on-flash-bbt;
+				brcm,nand-ecc-use-strap;
+			};
+		};
+
 		uart0: serial@12000 {
 			compatible = "arm,pl011", "arm,primecell";
 			reg = <0x12000 0x1000>;
diff --git a/arch/arm/boot/dts/broadcom/bcm63138.dtsi b/arch/arm/boot/dts/broadcom/bcm63138.dtsi
index 4ef02283612b..3aaee1c6994e 100644
--- a/arch/arm/boot/dts/broadcom/bcm63138.dtsi
+++ b/arch/arm/boot/dts/broadcom/bcm63138.dtsi
@@ -229,7 +229,15 @@ nand_controller: nand-controller@2000 {
 			reg-names = "nand", "nand-int-base";
 			status = "disabled";
 			interrupts = <GIC_SPI 38 IRQ_TYPE_LEVEL_HIGH>;
-			interrupt-names = "nand";
+			interrupt-names = "nand_ctlrdy";
+			brcm,nand-use-wp = <0>;
+
+			nandcs: nand@0 {
+				compatible = "brcm,nandcs";
+				reg = <0>;
+				nand-on-flash-bbt;
+				brcm,nand-ecc-use-strap;
+			};
 		};
 
 		serial@4400 {
diff --git a/arch/arm/boot/dts/broadcom/bcm63148.dtsi b/arch/arm/boot/dts/broadcom/bcm63148.dtsi
index 24431de1810e..6ecd530a7225 100644
--- a/arch/arm/boot/dts/broadcom/bcm63148.dtsi
+++ b/arch/arm/boot/dts/broadcom/bcm63148.dtsi
@@ -119,5 +119,22 @@ hsspi: spi@1000 {
 			num-cs = <8>;
 			status = "disabled";
 		};
+
+		nand_controller: nand-controller@2000 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			compatible = "brcm,nand-bcm63138", "brcm,brcmnand-v7.1", "brcm,brcmnand";
+			reg = <0x2000 0x600>, <0xf0 0x10>;
+			reg-names = "nand", "nand-int-base";
+			brcm,nand-use-wp = <0>;
+			status = "disabled";
+
+			nandcs: nand@0 {
+				compatible = "brcm,nandcs";
+				reg = <0>;
+				nand-on-flash-bbt;
+				brcm,nand-ecc-use-strap;
+			};
+		};
 	};
 };
diff --git a/arch/arm/boot/dts/broadcom/bcm63178.dtsi b/arch/arm/boot/dts/broadcom/bcm63178.dtsi
index 3f9aed96babf..742991fc7544 100644
--- a/arch/arm/boot/dts/broadcom/bcm63178.dtsi
+++ b/arch/arm/boot/dts/broadcom/bcm63178.dtsi
@@ -129,6 +129,23 @@ hsspi: spi@1000 {
 			status = "disabled";
 		};
 
+		nand_controller: nand-controller@1800 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			compatible = "brcm,nand-bcm63138", "brcm,brcmnand-v7.1", "brcm,brcmnand";
+			reg = <0x1800 0x600>, <0x2000 0x10>;
+			reg-names = "nand", "nand-int-base";
+			brcm,nand-use-wp = <0>;
+			status = "disabled";
+
+			nandcs: nand@0 {
+				compatible = "brcm,nandcs";
+				reg = <0>;
+				nand-on-flash-bbt;
+				brcm,nand-ecc-use-strap;
+			};
+		};
+
 		uart0: serial@12000 {
 			compatible = "arm,pl011", "arm,primecell";
 			reg = <0x12000 0x1000>;
diff --git a/arch/arm/boot/dts/broadcom/bcm6756.dtsi b/arch/arm/boot/dts/broadcom/bcm6756.dtsi
index 1d8d957d65dd..0f08b99d93c2 100644
--- a/arch/arm/boot/dts/broadcom/bcm6756.dtsi
+++ b/arch/arm/boot/dts/broadcom/bcm6756.dtsi
@@ -139,6 +139,23 @@ hsspi: spi@1000 {
 			status = "disabled";
 		};
 
+		nand_controller: nand-controller@1800 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			compatible = "brcm,nand-bcm63138", "brcm,brcmnand-v7.1", "brcm,brcmnand";
+			reg = <0x1800 0x600>, <0x2000 0x10>;
+			reg-names = "nand", "nand-int-base";
+			brcm,nand-use-wp = <0>;
+			status = "disabled";
+
+			nandcs: nand@0 {
+				compatible = "brcm,nandcs";
+				reg = <0>;
+				nand-on-flash-bbt;
+				brcm,nand-ecc-use-strap;
+			};
+		};
+
 		uart0: serial@12000 {
 			compatible = "arm,pl011", "arm,primecell";
 			reg = <0x12000 0x1000>;
diff --git a/arch/arm/boot/dts/broadcom/bcm6846.dtsi b/arch/arm/boot/dts/broadcom/bcm6846.dtsi
index cf92cf8c4693..856fd2352cca 100644
--- a/arch/arm/boot/dts/broadcom/bcm6846.dtsi
+++ b/arch/arm/boot/dts/broadcom/bcm6846.dtsi
@@ -119,5 +119,22 @@ hsspi: spi@1000 {
 			num-cs = <8>;
 			status = "disabled";
 		};
+
+		nand_controller: nand-controller@1800 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			compatible = "brcm,nand-bcm63138", "brcm,brcmnand-v7.1", "brcm,brcmnand";
+			reg = <0x1800 0x600>, <0x2000 0x10>;
+			reg-names = "nand", "nand-int-base";
+			brcm,nand-use-wp = <0>;
+			status = "disabled";
+
+			nandcs: nand@0 {
+				compatible = "brcm,nandcs";
+				reg = <0>;
+				nand-on-flash-bbt;
+				brcm,nand-ecc-use-strap;
+			};
+		};
 	};
 };
diff --git a/arch/arm/boot/dts/broadcom/bcm6855.dtsi b/arch/arm/boot/dts/broadcom/bcm6855.dtsi
index 52d6bc89f9f8..ad6f63a92c3a 100644
--- a/arch/arm/boot/dts/broadcom/bcm6855.dtsi
+++ b/arch/arm/boot/dts/broadcom/bcm6855.dtsi
@@ -129,6 +129,23 @@ hsspi: spi@1000 {
 			status = "disabled";
 		};
 
+		nand_controller: nand-controller@1800 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			compatible = "brcm,nand-bcm63138", "brcm,brcmnand-v7.1", "brcm,brcmnand";
+			reg = <0x1800 0x600>, <0x2000 0x10>;
+			reg-names = "nand", "nand-int-base";
+			brcm,nand-use-wp = <0>;
+			status = "disabled";
+
+			nandcs: nand@0 {
+				compatible = "brcm,nandcs";
+				reg = <0>;
+				nand-on-flash-bbt;
+				brcm,nand-ecc-use-strap;
+			};
+		};
+
 		uart0: serial@12000 {
 			compatible = "arm,pl011", "arm,primecell";
 			reg = <0x12000 0x1000>;
diff --git a/arch/arm/boot/dts/broadcom/bcm6878.dtsi b/arch/arm/boot/dts/broadcom/bcm6878.dtsi
index 2c5d706bac7e..540aac1b82f9 100644
--- a/arch/arm/boot/dts/broadcom/bcm6878.dtsi
+++ b/arch/arm/boot/dts/broadcom/bcm6878.dtsi
@@ -120,6 +120,23 @@ hsspi: spi@1000 {
 			status = "disabled";
 		};
 
+		nand_controller: nand-controller@1800 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			compatible = "brcm,nand-bcm63138", "brcm,brcmnand-v7.1", "brcm,brcmnand";
+			reg = <0x1800 0x600>, <0x2000 0x10>;
+			reg-names = "nand", "nand-int-base";
+			brcm,nand-use-wp = <0>;
+			status = "disabled";
+
+			nandcs: nand@0 {
+				compatible = "brcm,nandcs";
+				reg = <0>;
+				nand-on-flash-bbt;
+				brcm,nand-ecc-use-strap;
+			};
+		};
+
 		uart0: serial@12000 {
 			compatible = "arm,pl011", "arm,primecell";
 			reg = <0x12000 0x1000>;
diff --git a/arch/arm/boot/dts/broadcom/bcm947622.dts b/arch/arm/boot/dts/broadcom/bcm947622.dts
index 93b8ce22678d..22e3c4508e1a 100644
--- a/arch/arm/boot/dts/broadcom/bcm947622.dts
+++ b/arch/arm/boot/dts/broadcom/bcm947622.dts
@@ -32,3 +32,7 @@ &uart0 {
 &hsspi {
 	status = "okay";
 };
+
+&nand_controller {
+	status = "okay";
+};
diff --git a/arch/arm/boot/dts/broadcom/bcm963138.dts b/arch/arm/boot/dts/broadcom/bcm963138.dts
index 1b405c249213..450289d47dc7 100644
--- a/arch/arm/boot/dts/broadcom/bcm963138.dts
+++ b/arch/arm/boot/dts/broadcom/bcm963138.dts
@@ -29,3 +29,7 @@ &serial0 {
 &hsspi {
 	status = "okay";
 };
+
+&nand_controller {
+	status = "okay";
+};
diff --git a/arch/arm/boot/dts/broadcom/bcm963138dvt.dts b/arch/arm/boot/dts/broadcom/bcm963138dvt.dts
index b5af61853a07..22107d14a120 100644
--- a/arch/arm/boot/dts/broadcom/bcm963138dvt.dts
+++ b/arch/arm/boot/dts/broadcom/bcm963138dvt.dts
@@ -33,14 +33,12 @@ &serial1 {
 
 &nand_controller {
 	status = "okay";
+};
 
-	nand@0 {
-		compatible = "brcm,nandcs";
-		reg = <0>;
-		nand-ecc-strength = <4>;
-		nand-ecc-step-size = <512>;
-		brcm,nand-oob-sectors-size = <16>;
-	};
+&nandcs {
+	nand-ecc-strength = <4>;
+	nand-ecc-step-size = <512>;
+	brcm,nand-oob-sector-size = <16>;
 };
 
 &ahci {
diff --git a/arch/arm/boot/dts/broadcom/bcm963148.dts b/arch/arm/boot/dts/broadcom/bcm963148.dts
index 1f5d6d783f09..aa08b473c7cd 100644
--- a/arch/arm/boot/dts/broadcom/bcm963148.dts
+++ b/arch/arm/boot/dts/broadcom/bcm963148.dts
@@ -32,3 +32,7 @@ &uart0 {
 &hsspi {
 	status = "okay";
 };
+
+&nand_controller {
+	status = "okay";
+};
diff --git a/arch/arm/boot/dts/broadcom/bcm963178.dts b/arch/arm/boot/dts/broadcom/bcm963178.dts
index d036e99dd8d1..c0f504ac43a4 100644
--- a/arch/arm/boot/dts/broadcom/bcm963178.dts
+++ b/arch/arm/boot/dts/broadcom/bcm963178.dts
@@ -32,3 +32,7 @@ &uart0 {
 &hsspi {
 	status = "okay";
 };
+
+&nand_controller {
+	status = "okay";
+};
diff --git a/arch/arm/boot/dts/broadcom/bcm96756.dts b/arch/arm/boot/dts/broadcom/bcm96756.dts
index 8b104f3fb14a..2ce998f2b84f 100644
--- a/arch/arm/boot/dts/broadcom/bcm96756.dts
+++ b/arch/arm/boot/dts/broadcom/bcm96756.dts
@@ -32,3 +32,7 @@ &uart0 {
 &hsspi {
 	status = "okay";
 };
+
+&nand_controller {
+	status = "okay";
+};
diff --git a/arch/arm/boot/dts/broadcom/bcm96846.dts b/arch/arm/boot/dts/broadcom/bcm96846.dts
index 55852c229608..f4b9a07370ee 100644
--- a/arch/arm/boot/dts/broadcom/bcm96846.dts
+++ b/arch/arm/boot/dts/broadcom/bcm96846.dts
@@ -32,3 +32,7 @@ &uart0 {
 &hsspi {
 	status = "okay";
 };
+
+&nand_controller {
+	status = "okay";
+};
diff --git a/arch/arm/boot/dts/broadcom/bcm96855.dts b/arch/arm/boot/dts/broadcom/bcm96855.dts
index 2ad880af2104..5c94063bceaf 100644
--- a/arch/arm/boot/dts/broadcom/bcm96855.dts
+++ b/arch/arm/boot/dts/broadcom/bcm96855.dts
@@ -32,3 +32,7 @@ &uart0 {
 &hsspi {
 	status = "okay";
 };
+
+&nand_controller {
+	status = "okay";
+};
diff --git a/arch/arm/boot/dts/broadcom/bcm96878.dts b/arch/arm/boot/dts/broadcom/bcm96878.dts
index b7af8ade7a9d..910f7e125bad 100644
--- a/arch/arm/boot/dts/broadcom/bcm96878.dts
+++ b/arch/arm/boot/dts/broadcom/bcm96878.dts
@@ -32,3 +32,7 @@ &uart0 {
 &hsspi {
 	status = "okay";
 };
+
+&nand_controller {
+	status = "okay";
+};
-- 
2.37.3

[PATCH v3 03/10] arm64: dts: broadcom: bcmbca: Add NAND controller node

[David Regan]

From: William Zhang <william.zhang@broadcom.com>

Add support for Broadcom STB NAND controller in BCMBCA ARMv8 chip dts
files.

Signed-off-by: William Zhang <william.zhang@broadcom.com>
Reviewed-by: David Regan <dregan@broadcom.com>
---
Changes in v3: None
---
Changes in v2:
- Add nand controller node label for 4908 so it is consistent with other
SoCs and can be referenced by board dts file
---
 .../arm64/boot/dts/broadcom/bcmbca/bcm4908.dtsi |  5 ++++-
 .../arm64/boot/dts/broadcom/bcmbca/bcm4912.dtsi | 17 +++++++++++++++++
 .../boot/dts/broadcom/bcmbca/bcm63146.dtsi      | 17 +++++++++++++++++
 .../boot/dts/broadcom/bcmbca/bcm63158.dtsi      | 17 +++++++++++++++++
 .../arm64/boot/dts/broadcom/bcmbca/bcm6813.dtsi | 17 +++++++++++++++++
 .../arm64/boot/dts/broadcom/bcmbca/bcm6856.dtsi | 17 +++++++++++++++++
 .../arm64/boot/dts/broadcom/bcmbca/bcm6858.dtsi | 17 +++++++++++++++++
 .../arm64/boot/dts/broadcom/bcmbca/bcm94912.dts |  4 ++++
 .../boot/dts/broadcom/bcmbca/bcm963146.dts      |  4 ++++
 .../boot/dts/broadcom/bcmbca/bcm963158.dts      |  4 ++++
 .../arm64/boot/dts/broadcom/bcmbca/bcm96813.dts |  4 ++++
 .../arm64/boot/dts/broadcom/bcmbca/bcm96856.dts |  4 ++++
 .../arm64/boot/dts/broadcom/bcmbca/bcm96858.dts |  4 ++++
 13 files changed, 130 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/broadcom/bcmbca/bcm4908.dtsi b/arch/arm64/boot/dts/broadcom/bcmbca/bcm4908.dtsi
index 2f124b027bbf..03a516d02501 100644
--- a/arch/arm64/boot/dts/broadcom/bcmbca/bcm4908.dtsi
+++ b/arch/arm64/boot/dts/broadcom/bcmbca/bcm4908.dtsi
@@ -589,7 +589,7 @@ hsspi: spi@1000 {
 			status = "disabled";
 		};
 
-		nand-controller@1800 {
+		nand_controller: nand-controller@1800 {
 			#address-cells = <1>;
 			#size-cells = <0>;
 			compatible = "brcm,nand-bcm63138", "brcm,brcmnand-v7.1", "brcm,brcmnand";
@@ -597,11 +597,14 @@ nand-controller@1800 {
 			reg-names = "nand", "nand-int-base";
 			interrupts = <GIC_SPI 37 IRQ_TYPE_LEVEL_HIGH>;
 			interrupt-names = "nand_ctlrdy";
+			brcm,nand-use-wp = <0>;
 			status = "okay";
 
 			nandcs: nand@0 {
 				compatible = "brcm,nandcs";
 				reg = <0>;
+				nand-on-flash-bbt;
+				brcm,nand-ecc-use-strap;
 			};
 		};
 
diff --git a/arch/arm64/boot/dts/broadcom/bcmbca/bcm4912.dtsi b/arch/arm64/boot/dts/broadcom/bcmbca/bcm4912.dtsi
index d658c81f7285..92a829542467 100644
--- a/arch/arm64/boot/dts/broadcom/bcmbca/bcm4912.dtsi
+++ b/arch/arm64/boot/dts/broadcom/bcmbca/bcm4912.dtsi
@@ -138,6 +138,23 @@ hsspi: spi@1000 {
 			status = "disabled";
 		};
 
+		nand_controller: nand-controller@1800 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			compatible = "brcm,nand-bcm63138", "brcm,brcmnand-v7.1", "brcm,brcmnand";
+			reg = <0x1800 0x600>, <0x2000 0x10>;
+			reg-names = "nand", "nand-int-base";
+			brcm,nand-use-wp = <0>;
+			status = "disabled";
+
+			nandcs: nand@0 {
+				compatible = "brcm,nandcs";
+				reg = <0>;
+				nand-on-flash-bbt;
+				brcm,nand-ecc-use-strap;
+			};
+		};
+
 		uart0: serial@12000 {
 			compatible = "arm,pl011", "arm,primecell";
 			reg = <0x12000 0x1000>;
diff --git a/arch/arm64/boot/dts/broadcom/bcmbca/bcm63146.dtsi b/arch/arm64/boot/dts/broadcom/bcmbca/bcm63146.dtsi
index 4f474d47022e..2283fd81ece6 100644
--- a/arch/arm64/boot/dts/broadcom/bcmbca/bcm63146.dtsi
+++ b/arch/arm64/boot/dts/broadcom/bcmbca/bcm63146.dtsi
@@ -119,6 +119,23 @@ hsspi: spi@1000 {
 			status = "disabled";
 		};
 
+		nand_controller: nand-controller@1800 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			compatible = "brcm,nand-bcm63138", "brcm,brcmnand-v7.1", "brcm,brcmnand";
+			reg = <0x1800 0x600>, <0x2000 0x10>;
+			reg-names = "nand", "nand-int-base";
+			brcm,nand-use-wp = <0>;
+			status = "disabled";
+
+			nandcs: nand@0 {
+				compatible = "brcm,nandcs";
+				reg = <0>;
+				nand-on-flash-bbt;
+				brcm,nand-ecc-use-strap;
+			};
+		};
+
 		uart0: serial@12000 {
 			compatible = "arm,pl011", "arm,primecell";
 			reg = <0x12000 0x1000>;
diff --git a/arch/arm64/boot/dts/broadcom/bcmbca/bcm63158.dtsi b/arch/arm64/boot/dts/broadcom/bcmbca/bcm63158.dtsi
index 909f254dc47d..c70177066a15 100644
--- a/arch/arm64/boot/dts/broadcom/bcmbca/bcm63158.dtsi
+++ b/arch/arm64/boot/dts/broadcom/bcmbca/bcm63158.dtsi
@@ -137,6 +137,23 @@ hsspi: spi@1000 {
 			status = "disabled";
 		};
 
+		nand_controller: nand-controller@1800 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			compatible = "brcm,nand-bcm63138", "brcm,brcmnand-v7.1", "brcm,brcmnand";
+			reg = <0x1800 0x600>, <0x2000 0x10>;
+			reg-names = "nand", "nand-int-base";
+			brcm,nand-use-wp = <0>;
+			status = "disabled";
+
+			nandcs: nand@0 {
+				compatible = "brcm,nandcs";
+				reg = <0>;
+				nand-on-flash-bbt;
+				brcm,nand-ecc-use-strap;
+			};
+		};
+
 		uart0: serial@12000 {
 			compatible = "arm,pl011", "arm,primecell";
 			reg = <0x12000 0x1000>;
diff --git a/arch/arm64/boot/dts/broadcom/bcmbca/bcm6813.dtsi b/arch/arm64/boot/dts/broadcom/bcmbca/bcm6813.dtsi
index 685ae32951c9..09115e3af694 100644
--- a/arch/arm64/boot/dts/broadcom/bcmbca/bcm6813.dtsi
+++ b/arch/arm64/boot/dts/broadcom/bcmbca/bcm6813.dtsi
@@ -138,6 +138,23 @@ hsspi: spi@1000 {
 			status = "disabled";
 		};
 
+		nand_controller: nand-controller@1800 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			compatible = "brcm,nand-bcm63138", "brcm,brcmnand-v7.1", "brcm,brcmnand";
+			reg = <0x1800 0x600>, <0x2000 0x10>;
+			reg-names = "nand", "nand-int-base";
+			brcm,nand-use-wp = <0>;
+			status = "disabled";
+
+			nandcs: nand@0 {
+				compatible = "brcm,nandcs";
+				reg = <0>;
+				nand-on-flash-bbt;
+				brcm,nand-ecc-use-strap;
+			};
+		};
+
 		uart0: serial@12000 {
 			compatible = "arm,pl011", "arm,primecell";
 			reg = <0x12000 0x1000>;
diff --git a/arch/arm64/boot/dts/broadcom/bcmbca/bcm6856.dtsi b/arch/arm64/boot/dts/broadcom/bcmbca/bcm6856.dtsi
index 820553ce541b..03ed65b35dbc 100644
--- a/arch/arm64/boot/dts/broadcom/bcmbca/bcm6856.dtsi
+++ b/arch/arm64/boot/dts/broadcom/bcmbca/bcm6856.dtsi
@@ -119,5 +119,22 @@ hsspi: spi@1000 {
 			num-cs = <8>;
 			status = "disabled";
 		};
+
+		nand_controller: nand-controller@1800 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			compatible = "brcm,nand-bcm63138", "brcm,brcmnand-v7.1", "brcm,brcmnand";
+			reg = <0x1800 0x600>, <0x2000 0x10>;
+			reg-names = "nand", "nand-int-base";
+			brcm,nand-use-wp = <0>;
+			status = "disabled";
+
+			nandcs: nand@0 {
+				compatible = "brcm,nandcs";
+				reg = <0>;
+				nand-on-flash-bbt;
+				brcm,nand-ecc-use-strap;
+			};
+		};
 	};
 };
diff --git a/arch/arm64/boot/dts/broadcom/bcmbca/bcm6858.dtsi b/arch/arm64/boot/dts/broadcom/bcmbca/bcm6858.dtsi
index 0eb93c298297..36f1fc06fcec 100644
--- a/arch/arm64/boot/dts/broadcom/bcmbca/bcm6858.dtsi
+++ b/arch/arm64/boot/dts/broadcom/bcmbca/bcm6858.dtsi
@@ -156,5 +156,22 @@ hsspi: spi@1000 {
 			num-cs = <8>;
 			status = "disabled";
 		};
+
+		nand_controller: nand-controller@1800 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			compatible = "brcm,nand-bcm63138", "brcm,brcmnand-v7.1", "brcm,brcmnand";
+			reg = <0x1800 0x600>, <0x2000 0x10>;
+			reg-names = "nand", "nand-int-base";
+			brcm,nand-use-wp = <0>;
+			status = "disabled";
+
+			nandcs: nand@0 {
+				compatible = "brcm,nandcs";
+				reg = <0>;
+				nand-on-flash-bbt;
+				brcm,nand-ecc-use-strap;
+			};
+		};
 	};
 };
diff --git a/arch/arm64/boot/dts/broadcom/bcmbca/bcm94912.dts b/arch/arm64/boot/dts/broadcom/bcmbca/bcm94912.dts
index e69cd683211a..4d1ea501e384 100644
--- a/arch/arm64/boot/dts/broadcom/bcmbca/bcm94912.dts
+++ b/arch/arm64/boot/dts/broadcom/bcmbca/bcm94912.dts
@@ -32,3 +32,7 @@ &uart0 {
 &hsspi {
 	status = "okay";
 };
+
+&nand_controller {
+	status = "okay";
+};
diff --git a/arch/arm64/boot/dts/broadcom/bcmbca/bcm963146.dts b/arch/arm64/boot/dts/broadcom/bcmbca/bcm963146.dts
index db2c82d6dfd8..810b5a23da7b 100644
--- a/arch/arm64/boot/dts/broadcom/bcmbca/bcm963146.dts
+++ b/arch/arm64/boot/dts/broadcom/bcmbca/bcm963146.dts
@@ -32,3 +32,7 @@ &uart0 {
 &hsspi {
 	status = "okay";
 };
+
+&nand_controller {
+	status = "okay";
+};
diff --git a/arch/arm64/boot/dts/broadcom/bcmbca/bcm963158.dts b/arch/arm64/boot/dts/broadcom/bcmbca/bcm963158.dts
index 25c12bc63545..3aaae5dbb568 100644
--- a/arch/arm64/boot/dts/broadcom/bcmbca/bcm963158.dts
+++ b/arch/arm64/boot/dts/broadcom/bcmbca/bcm963158.dts
@@ -32,3 +32,7 @@ &uart0 {
 &hsspi {
 	status = "okay";
 };
+
+&nand_controller {
+	status = "okay";
+};
diff --git a/arch/arm64/boot/dts/broadcom/bcmbca/bcm96813.dts b/arch/arm64/boot/dts/broadcom/bcmbca/bcm96813.dts
index faba21f03120..6b167cc2af76 100644
--- a/arch/arm64/boot/dts/broadcom/bcmbca/bcm96813.dts
+++ b/arch/arm64/boot/dts/broadcom/bcmbca/bcm96813.dts
@@ -32,3 +32,7 @@ &uart0 {
 &hsspi {
 	status = "okay";
 };
+
+&nand_controller {
+	status = "okay";
+};
diff --git a/arch/arm64/boot/dts/broadcom/bcmbca/bcm96856.dts b/arch/arm64/boot/dts/broadcom/bcmbca/bcm96856.dts
index 9808331eede2..d598cd618b57 100644
--- a/arch/arm64/boot/dts/broadcom/bcmbca/bcm96856.dts
+++ b/arch/arm64/boot/dts/broadcom/bcmbca/bcm96856.dts
@@ -32,3 +32,7 @@ &uart0 {
 &hsspi {
 	status = "okay";
 };
+
+&nand_controller {
+	status = "okay";
+};
diff --git a/arch/arm64/boot/dts/broadcom/bcmbca/bcm96858.dts b/arch/arm64/boot/dts/broadcom/bcmbca/bcm96858.dts
index 1f561c8e13b0..e50ddbf6f58c 100644
--- a/arch/arm64/boot/dts/broadcom/bcmbca/bcm96858.dts
+++ b/arch/arm64/boot/dts/broadcom/bcmbca/bcm96858.dts
@@ -32,3 +32,7 @@ &uart0 {
 &hsspi {
 	status = "okay";
 };
+
+&nand_controller {
+	status = "okay";
+};
-- 
2.37.3

[PATCH v3 04/10] mtd: rawnand: brcmnand: Rename bcm63138 nand driver

[David Regan]

From: William Zhang <william.zhang@broadcom.com>

In preparing to support multiple BCMBCA SoCs, rename bcm63138 to bcmbca
in the driver code and driver file name.

Signed-off-by: William Zhang <william.zhang@broadcom.com>
Reviewed-by: David Regan <dregan@broadcom.com>
Acked-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
Changes in v3: None
---
Changes in v2: None
---
 drivers/mtd/nand/raw/brcmnand/Makefile        |  2 +-
 drivers/mtd/nand/raw/brcmnand/bcm63138_nand.c | 99 -------------------
 drivers/mtd/nand/raw/brcmnand/bcmbca_nand.c   | 99 +++++++++++++++++++
 3 files changed, 100 insertions(+), 100 deletions(-)
 delete mode 100644 drivers/mtd/nand/raw/brcmnand/bcm63138_nand.c
 create mode 100644 drivers/mtd/nand/raw/brcmnand/bcmbca_nand.c

diff --git a/drivers/mtd/nand/raw/brcmnand/Makefile b/drivers/mtd/nand/raw/brcmnand/Makefile
index 9907e3ec4bb2..0536568c6467 100644
--- a/drivers/mtd/nand/raw/brcmnand/Makefile
+++ b/drivers/mtd/nand/raw/brcmnand/Makefile
@@ -2,7 +2,7 @@
 # link order matters; don't link the more generic brcmstb_nand.o before the
 # more specific iproc_nand.o, for instance
 obj-$(CONFIG_MTD_NAND_BRCMNAND_IPROC)	+= iproc_nand.o
-obj-$(CONFIG_MTD_NAND_BRCMNAND_BCMBCA)	+= bcm63138_nand.o
+obj-$(CONFIG_MTD_NAND_BRCMNAND_BCMBCA)	+= bcmbca_nand.o
 obj-$(CONFIG_MTD_NAND_BRCMNAND_BCM63XX)	+= bcm6368_nand.o
 obj-$(CONFIG_MTD_NAND_BRCMNAND_BRCMSTB)	+= brcmstb_nand.o
 obj-$(CONFIG_MTD_NAND_BRCMNAND)		+= brcmnand.o
diff --git a/drivers/mtd/nand/raw/brcmnand/bcm63138_nand.c b/drivers/mtd/nand/raw/brcmnand/bcm63138_nand.c
deleted file mode 100644
index 968c5b674b08..000000000000
--- a/drivers/mtd/nand/raw/brcmnand/bcm63138_nand.c
+++ /dev/null
@@ -1,99 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-only
-/*
- * Copyright © 2015 Broadcom Corporation
- */
-
-#include <linux/device.h>
-#include <linux/io.h>
-#include <linux/ioport.h>
-#include <linux/module.h>
-#include <linux/of.h>
-#include <linux/of_address.h>
-#include <linux/platform_device.h>
-#include <linux/slab.h>
-
-#include "brcmnand.h"
-
-struct bcm63138_nand_soc {
-	struct brcmnand_soc soc;
-	void __iomem *base;
-};
-
-#define BCM63138_NAND_INT_STATUS		0x00
-#define BCM63138_NAND_INT_EN			0x04
-
-enum {
-	BCM63138_CTLRDY		= BIT(4),
-};
-
-static bool bcm63138_nand_intc_ack(struct brcmnand_soc *soc)
-{
-	struct bcm63138_nand_soc *priv =
-			container_of(soc, struct bcm63138_nand_soc, soc);
-	void __iomem *mmio = priv->base + BCM63138_NAND_INT_STATUS;
-	u32 val = brcmnand_readl(mmio);
-
-	if (val & BCM63138_CTLRDY) {
-		brcmnand_writel(val & ~BCM63138_CTLRDY, mmio);
-		return true;
-	}
-
-	return false;
-}
-
-static void bcm63138_nand_intc_set(struct brcmnand_soc *soc, bool en)
-{
-	struct bcm63138_nand_soc *priv =
-			container_of(soc, struct bcm63138_nand_soc, soc);
-	void __iomem *mmio = priv->base + BCM63138_NAND_INT_EN;
-	u32 val = brcmnand_readl(mmio);
-
-	if (en)
-		val |= BCM63138_CTLRDY;
-	else
-		val &= ~BCM63138_CTLRDY;
-
-	brcmnand_writel(val, mmio);
-}
-
-static int bcm63138_nand_probe(struct platform_device *pdev)
-{
-	struct device *dev = &pdev->dev;
-	struct bcm63138_nand_soc *priv;
-	struct brcmnand_soc *soc;
-
-	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
-	if (!priv)
-		return -ENOMEM;
-	soc = &priv->soc;
-
-	priv->base = devm_platform_ioremap_resource_byname(pdev, "nand-int-base");
-	if (IS_ERR(priv->base))
-		return PTR_ERR(priv->base);
-
-	soc->ctlrdy_ack = bcm63138_nand_intc_ack;
-	soc->ctlrdy_set_enabled = bcm63138_nand_intc_set;
-
-	return brcmnand_probe(pdev, soc);
-}
-
-static const struct of_device_id bcm63138_nand_of_match[] = {
-	{ .compatible = "brcm,nand-bcm63138" },
-	{},
-};
-MODULE_DEVICE_TABLE(of, bcm63138_nand_of_match);
-
-static struct platform_driver bcm63138_nand_driver = {
-	.probe			= bcm63138_nand_probe,
-	.remove_new		= brcmnand_remove,
-	.driver = {
-		.name		= "bcm63138_nand",
-		.pm		= &brcmnand_pm_ops,
-		.of_match_table	= bcm63138_nand_of_match,
-	}
-};
-module_platform_driver(bcm63138_nand_driver);
-
-MODULE_LICENSE("GPL v2");
-MODULE_AUTHOR("Brian Norris");
-MODULE_DESCRIPTION("NAND driver for BCM63138");
diff --git a/drivers/mtd/nand/raw/brcmnand/bcmbca_nand.c b/drivers/mtd/nand/raw/brcmnand/bcmbca_nand.c
new file mode 100644
index 000000000000..3e2f3b79788d
--- /dev/null
+++ b/drivers/mtd/nand/raw/brcmnand/bcmbca_nand.c
@@ -0,0 +1,99 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright © 2015 Broadcom Corporation
+ */
+
+#include <linux/device.h>
+#include <linux/io.h>
+#include <linux/ioport.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#include "brcmnand.h"
+
+struct bcmbca_nand_soc {
+	struct brcmnand_soc soc;
+	void __iomem *base;
+};
+
+#define BCMBCA_NAND_INT_STATUS		0x00
+#define BCMBCA_NAND_INT_EN			0x04
+
+enum {
+	BCMBCA_CTLRDY		= BIT(4),
+};
+
+static bool bcmbca_nand_intc_ack(struct brcmnand_soc *soc)
+{
+	struct bcmbca_nand_soc *priv =
+			container_of(soc, struct bcmbca_nand_soc, soc);
+	void __iomem *mmio = priv->base + BCMBCA_NAND_INT_STATUS;
+	u32 val = brcmnand_readl(mmio);
+
+	if (val & BCMBCA_CTLRDY) {
+		brcmnand_writel(val & ~BCMBCA_CTLRDY, mmio);
+		return true;
+	}
+
+	return false;
+}
+
+static void bcmbca_nand_intc_set(struct brcmnand_soc *soc, bool en)
+{
+	struct bcmbca_nand_soc *priv =
+			container_of(soc, struct bcmbca_nand_soc, soc);
+	void __iomem *mmio = priv->base + BCMBCA_NAND_INT_EN;
+	u32 val = brcmnand_readl(mmio);
+
+	if (en)
+		val |= BCMBCA_CTLRDY;
+	else
+		val &= ~BCMBCA_CTLRDY;
+
+	brcmnand_writel(val, mmio);
+}
+
+static int bcmbca_nand_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct bcmbca_nand_soc *priv;
+	struct brcmnand_soc *soc;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+	soc = &priv->soc;
+
+	priv->base = devm_platform_ioremap_resource_byname(pdev, "nand-int-base");
+	if (IS_ERR(priv->base))
+		return PTR_ERR(priv->base);
+
+	soc->ctlrdy_ack = bcmbca_nand_intc_ack;
+	soc->ctlrdy_set_enabled = bcmbca_nand_intc_set;
+
+	return brcmnand_probe(pdev, soc);
+}
+
+static const struct of_device_id bcmbca_nand_of_match[] = {
+	{ .compatible = "brcm,nand-bcm63138" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, bcmbca_nand_of_match);
+
+static struct platform_driver bcmbca_nand_driver = {
+	.probe			= bcmbca_nand_probe,
+	.remove_new		= brcmnand_remove,
+	.driver = {
+		.name		= "bcmbca_nand",
+		.pm		= &brcmnand_pm_ops,
+		.of_match_table	= bcmbca_nand_of_match,
+	}
+};
+module_platform_driver(bcmbca_nand_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Brian Norris");
+MODULE_DESCRIPTION("NAND driver for BCMBCA");
-- 
2.37.3

[PATCH v3 05/10] mtd: rawnand: brcmnand: Add BCMBCA read data bus interface

[David Regan]

From: William Zhang <william.zhang@broadcom.com>

The BCMBCA broadband SoC integrates the NAND controller differently than
STB, iProc and other SoCs.  It has different endianness for NAND cache
data.

Add a SoC read data bus shim for BCMBCA to meet the specific SoC need
and performance improvement using the optimized memcpy function on NAND
cache memory.

Signed-off-by: William Zhang <william.zhang@broadcom.com>
Reviewed-by: David Regan <dregan@broadcom.com>
---
Changes in v3:
- Updated bcmbca_read_data_bus comment
---
Changes in v2:
- Drop the is_param argument to the read data bus function now that we
have the exec_op API to read the parameter page and ONFI data
- Remove be32_to_cpu from brcmnand_read_data_bus
---
 drivers/mtd/nand/raw/brcmnand/bcmbca_nand.c | 27 +++++++++++++++++++++
 drivers/mtd/nand/raw/brcmnand/brcmnand.c    | 20 ++++++++++++---
 drivers/mtd/nand/raw/brcmnand/brcmnand.h    |  2 ++
 3 files changed, 46 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/nand/raw/brcmnand/bcmbca_nand.c b/drivers/mtd/nand/raw/brcmnand/bcmbca_nand.c
index 3e2f3b79788d..7ad3e7a98f97 100644
--- a/drivers/mtd/nand/raw/brcmnand/bcmbca_nand.c
+++ b/drivers/mtd/nand/raw/brcmnand/bcmbca_nand.c
@@ -26,6 +26,18 @@ enum {
 	BCMBCA_CTLRDY		= BIT(4),
 };
 
+#if defined(CONFIG_ARM64)
+#define ALIGN_REQ		8
+#else
+#define ALIGN_REQ		4
+#endif
+
+static inline bool bcmbca_nand_is_buf_aligned(void *flash_cache,  void *buffer)
+{
+	return IS_ALIGNED((uintptr_t)buffer, ALIGN_REQ) &&
+				IS_ALIGNED((uintptr_t)flash_cache, ALIGN_REQ);
+}
+
 static bool bcmbca_nand_intc_ack(struct brcmnand_soc *soc)
 {
 	struct bcmbca_nand_soc *priv =
@@ -56,6 +68,20 @@ static void bcmbca_nand_intc_set(struct brcmnand_soc *soc, bool en)
 	brcmnand_writel(val, mmio);
 }
 
+static void bcmbca_read_data_bus(struct brcmnand_soc *soc,
+				 void __iomem *flash_cache,  u32 *buffer, int fc_words)
+{
+	/*
+	 * memcpy can do unaligned aligned access depending on source
+	 * and dest address, which is incompatible with nand cache. Fallback
+	 * to the memcpy_fromio in such case
+	 */
+	if (bcmbca_nand_is_buf_aligned((void *)flash_cache, buffer))
+		memcpy((void *)buffer, (void *)flash_cache, fc_words * 4);
+	else
+		memcpy_fromio((void *)buffer, flash_cache, fc_words * 4);
+}
+
 static int bcmbca_nand_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -73,6 +99,7 @@ static int bcmbca_nand_probe(struct platform_device *pdev)
 
 	soc->ctlrdy_ack = bcmbca_nand_intc_ack;
 	soc->ctlrdy_set_enabled = bcmbca_nand_intc_set;
+	soc->read_data_bus = bcmbca_read_data_bus;
 
 	return brcmnand_probe(pdev, soc);
 }
diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
index 8faca43ae1ff..73fdf7ce21aa 100644
--- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
+++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
@@ -851,6 +851,20 @@ static inline u32 edu_readl(struct brcmnand_controller *ctrl,
 	return brcmnand_readl(ctrl->edu_base + offs);
 }
 
+static inline void brcmnand_read_data_bus(struct brcmnand_controller *ctrl,
+					  void __iomem *flash_cache, u32 *buffer, int fc_words)
+{
+	struct brcmnand_soc *soc = ctrl->soc;
+	int i;
+
+	if (soc->read_data_bus) {
+		soc->read_data_bus(soc, flash_cache, buffer, fc_words);
+	} else {
+		for (i = 0; i < fc_words; i++)
+			buffer[i] = brcmnand_read_fc(ctrl, i);
+	}
+}
+
 static void brcmnand_clear_ecc_addr(struct brcmnand_controller *ctrl)
 {
 
@@ -1975,7 +1989,7 @@ static int brcmnand_read_by_pio(struct mtd_info *mtd, struct nand_chip *chip,
 {
 	struct brcmnand_host *host = nand_get_controller_data(chip);
 	struct brcmnand_controller *ctrl = host->ctrl;
-	int i, j, ret = 0;
+	int i, ret = 0;
 
 	brcmnand_clear_ecc_addr(ctrl);
 
@@ -1988,8 +2002,8 @@ static int brcmnand_read_by_pio(struct mtd_info *mtd, struct nand_chip *chip,
 		if (likely(buf)) {
 			brcmnand_soc_data_bus_prepare(ctrl->soc, false);
 
-			for (j = 0; j < FC_WORDS; j++, buf++)
-				*buf = brcmnand_read_fc(ctrl, j);
+			brcmnand_read_data_bus(ctrl, ctrl->nand_fc, buf, FC_WORDS);
+			buf += FC_WORDS;
 
 			brcmnand_soc_data_bus_unprepare(ctrl->soc, false);
 		}
diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.h b/drivers/mtd/nand/raw/brcmnand/brcmnand.h
index 928114c0be5e..7261a69989fe 100644
--- a/drivers/mtd/nand/raw/brcmnand/brcmnand.h
+++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.h
@@ -24,6 +24,8 @@ struct brcmnand_soc {
 	void (*ctlrdy_set_enabled)(struct brcmnand_soc *soc, bool en);
 	void (*prepare_data_bus)(struct brcmnand_soc *soc, bool prepare,
 				 bool is_param);
+	void (*read_data_bus)(struct brcmnand_soc *soc, void __iomem *flash_cache,
+				  u32 *buffer, int fc_words);
 	const struct brcmnand_io_ops *ops;
 };
 
-- 
2.37.3

[PATCH v3 06/10] mtd: rawnand: brcmnand: Add support for getting ecc setting from strap

[David Regan]

From: William Zhang <william.zhang@broadcom.com>

BCMBCA broadband SoC based board design does not specify ecc setting in
dts but rather use the SoC NAND strap info to obtain the ecc strength
and spare area size setting. Add brcm,nand-ecc-use-strap dts propety for
this purpose and update driver to support this option.

The generic nand ecc settings still take precedence over this flag. For
example, if nand-ecc-strength is set in the dts, the driver ignores the
strap setting and falls back to original behavior. This makes sure that
the existing BCMBCA board dts still works the old way even the strap
flag is set in the BCMBCA chip dtsi.

Signed-off-by: William Zhang <william.zhang@broadcom.com>
Reviewed-by: David Regan <dregan@broadcom.com>
---
Changes in v3: None
---
Changes in v2:
- Minor cosmetic fixes
---
 drivers/mtd/nand/raw/brcmnand/brcmnand.c | 83 ++++++++++++++++++++++--
 1 file changed, 76 insertions(+), 7 deletions(-)

diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
index 73fdf7ce21aa..869ea64e9189 100644
--- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
+++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
@@ -1038,6 +1038,19 @@ static inline int brcmnand_sector_1k_shift(struct brcmnand_controller *ctrl)
 		return -1;
 }
 
+static int brcmnand_get_sector_size_1k(struct brcmnand_host *host)
+{
+	struct brcmnand_controller *ctrl = host->ctrl;
+	int shift = brcmnand_sector_1k_shift(ctrl);
+	u16 acc_control_offs = brcmnand_cs_offset(ctrl, host->cs,
+						  BRCMNAND_CS_ACC_CONTROL);
+
+	if (shift < 0)
+		return 0;
+
+	return (nand_readreg(ctrl, acc_control_offs) >> shift) & 0x1;
+}
+
 static void brcmnand_set_sector_size_1k(struct brcmnand_host *host, int val)
 {
 	struct brcmnand_controller *ctrl = host->ctrl;
@@ -1055,6 +1068,38 @@ static void brcmnand_set_sector_size_1k(struct brcmnand_host *host, int val)
 	nand_writereg(ctrl, acc_control_offs, tmp);
 }
 
+static int brcmnand_get_spare_size(struct brcmnand_host *host)
+{
+	struct brcmnand_controller *ctrl = host->ctrl;
+	u16 acc_control_offs = brcmnand_cs_offset(ctrl, host->cs,
+						  BRCMNAND_CS_ACC_CONTROL);
+	u32 acc = nand_readreg(ctrl, acc_control_offs);
+
+	return (acc & brcmnand_spare_area_mask(ctrl));
+}
+
+static int brcmnand_get_ecc_strength(struct brcmnand_host *host)
+{
+	struct brcmnand_controller *ctrl = host->ctrl;
+	u16 acc_control_offs = brcmnand_cs_offset(ctrl, host->cs,
+						  BRCMNAND_CS_ACC_CONTROL);
+	int sector_size_1k = brcmnand_get_sector_size_1k(host);
+	int spare_area_size, ecc_level, ecc_strength;
+	u32 acc;
+
+	spare_area_size = brcmnand_get_spare_size(host);
+	acc = nand_readreg(ctrl, acc_control_offs);
+	ecc_level = (acc & brcmnand_ecc_level_mask(ctrl)) >> ctrl->ecc_level_shift;
+	if (sector_size_1k)
+		ecc_strength = ecc_level * 2;
+	else if (spare_area_size == 16 && ecc_level == 15)
+		ecc_strength = 1; /* hamming */
+	else
+		ecc_strength = ecc_level;
+
+	return ecc_strength;
+}
+
 /***********************************************************************
  * CS_NAND_SELECT
  ***********************************************************************/
@@ -2622,19 +2667,43 @@ static int brcmnand_setup_dev(struct brcmnand_host *host)
 		nanddev_get_memorg(&chip->base);
 	struct brcmnand_controller *ctrl = host->ctrl;
 	struct brcmnand_cfg *cfg = &host->hwcfg;
-	char msg[128];
+	struct device_node *np = nand_get_flash_node(chip);
 	u32 offs, tmp, oob_sector;
-	int ret;
+	int ret, sector_size_1k = 0;
+	bool use_strap = false;
+	char msg[128];
 
 	memset(cfg, 0, sizeof(*cfg));
+	use_strap = of_property_read_bool(np, "brcm,nand-ecc-use-strap");
 
-	ret = of_property_read_u32(nand_get_flash_node(chip),
-				   "brcm,nand-oob-sector-size",
+	/*
+	 * Set ECC size and strength based on hw configuration from strap
+	 * if device tree does not specify them and use strap property is set
+	 * If ecc strength is set in dts, don't use strap setting.
+	 */
+	if (chip->ecc.strength)
+		use_strap = 0;
+
+	if (use_strap) {
+		chip->ecc.strength = brcmnand_get_ecc_strength(host);
+		sector_size_1k = brcmnand_get_sector_size_1k(host);
+		if (chip->ecc.size == 0) {
+			if (sector_size_1k < 0)
+				chip->ecc.size = 512;
+			else
+				chip->ecc.size = 512 << sector_size_1k;
+		}
+	}
+
+	ret = of_property_read_u32(np, "brcm,nand-oob-sector-size",
 				   &oob_sector);
 	if (ret) {
-		/* Use detected size */
-		cfg->spare_area_size = mtd->oobsize /
-					(mtd->writesize >> FC_SHIFT);
+		if (use_strap)
+			cfg->spare_area_size = brcmnand_get_spare_size(host);
+		else
+			/* Use detected size */
+			cfg->spare_area_size = mtd->oobsize /
+						(mtd->writesize >> FC_SHIFT);
 	} else {
 		cfg->spare_area_size = oob_sector;
 	}
-- 
2.37.3

[PATCH v3 07/10] mtd: rawnand: brcmnand: Support write protection setting from dts

[David Regan]

From: William Zhang <william.zhang@broadcom.com>

The write protection feature is controlled by the module parameter wp_on
with default set to enabled. But not all the board use this feature
especially in BCMBCA broadband board. And module parameter is not
sufficient as different board can have different option.  Add a device
tree property and allow this feature to be configured through the board
dts on per board basis.

Signed-off-by: William Zhang <william.zhang@broadcom.com>
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
Reviewed-by: Kamal Dasu <kamal.dasu@broadcom.com>
Reviewed-by: David Regan <dregan@broadcom.com>
---
Changes in v3: None
---
Changes in v2: None
---
 drivers/mtd/nand/raw/brcmnand/brcmnand.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
index 869ea64e9189..9a904c7c6dad 100644
--- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
+++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
@@ -3081,7 +3081,7 @@ int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc)
 	struct brcmnand_controller *ctrl;
 	struct brcmnand_host *host;
 	struct resource *res;
-	int ret;
+	int ret, wp_dt;
 
 	if (dn && !of_match_node(brcmnand_of_match, dn))
 		return -ENODEV;
@@ -3218,6 +3218,12 @@ int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc)
 	/* Disable XOR addressing */
 	brcmnand_rmw_reg(ctrl, BRCMNAND_CS_XOR, 0xff, 0, 0);
 
+	/* Not all boards support write protect (WP), check DT property */
+	if (of_property_read_u32(dn, "brcm,nand-use-wp", &wp_dt) == 0) {
+		if (wp_dt >= 0 && wp_dt <= 2)
+			wp_on = wp_dt;
+	}
+
 	if (ctrl->features & BRCMNAND_HAS_WP) {
 		/* Permanently disable write protection */
 		if (wp_on == 2)
-- 
2.37.3

[PATCH v3 08/10] mtd: rawnand: brcmnand: exec_op helper functions return type fixes

[David Regan]

fix return type for exec_op reset and status detect helper functions

Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: http://lists.infradead.org/pipermail/linux-mtd/2023-December/102423.html
Fixes: 3cc4718fa644 ("mtd: rawnand: brcmnand: exec_op implementation")
Signed-off-by: David Regan <dregan@broadcom.com>
Reviewed-by: William Zhang <william.zhang@broadcom.com>
---
Changes in v3: None
---
Changes in v2:
- Added to patch series
---
 drivers/mtd/nand/raw/brcmnand/brcmnand.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
index 9a904c7c6dad..6b5d76eff0ec 100644
--- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
+++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
@@ -625,7 +625,7 @@ enum {
 /* Only for v7.2 */
 #define	ACC_CONTROL_ECC_EXT_SHIFT		13
 
-static u8 brcmnand_status(struct brcmnand_host *host);
+static int brcmnand_status(struct brcmnand_host *host);
 
 static inline bool brcmnand_non_mmio_ops(struct brcmnand_controller *ctrl)
 {
@@ -1749,7 +1749,7 @@ static int brcmnand_waitfunc(struct nand_chip *chip)
 				 INTFC_FLASH_STATUS;
 }
 
-static u8 brcmnand_status(struct brcmnand_host *host)
+static int brcmnand_status(struct brcmnand_host *host)
 {
 	struct nand_chip *chip = &host->chip;
 	struct mtd_info *mtd = nand_to_mtd(chip);
@@ -1760,7 +1760,7 @@ static u8 brcmnand_status(struct brcmnand_host *host)
 	return brcmnand_waitfunc(chip);
 }
 
-static u8 brcmnand_reset(struct brcmnand_host *host)
+static int brcmnand_reset(struct brcmnand_host *host)
 {
 	struct nand_chip *chip = &host->chip;
 
@@ -2492,11 +2492,14 @@ static int brcmnand_exec_op(struct nand_chip *chip,
 
 	if (brcmnand_op_is_status(op)) {
 		status = op->instrs[1].ctx.data.buf.in;
-		*status = brcmnand_status(host);
+		ret = brcmnand_status(host);
+		if (ret < 0)
+			return ret;
+
+		*status = ret & 0xFF;
 
 		return 0;
-	}
-	else if (brcmnand_op_is_reset(op)) {
+	} else if (brcmnand_op_is_reset(op)) {
 		ret = brcmnand_reset(host);
 		if (ret < 0)
 			return ret;
-- 
2.37.3

[PATCH v3 09/10] mtd: rawnand: brcmnand: update log level messages

[David Regan]

Update log level messages so that more critical messages
can be seen.

Signed-off-by: David Regan <dregan@broadcom.com>
Reviewed-by: William Zhang <william.zhang@broadcom.com>
---
Changes in v3: None
---
Changes in v2:
- Added to patch series
---
 drivers/mtd/nand/raw/brcmnand/brcmnand.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
index 6b5d76eff0ec..a4e311b6798c 100644
--- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
+++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
@@ -1143,7 +1143,7 @@ static int bcmnand_ctrl_poll_status(struct brcmnand_host *host,
 	if ((val & mask) == expected_val)
 		return 0;
 
-	dev_warn(ctrl->dev, "timeout on status poll (expected %x got %x)\n",
+	dev_err(ctrl->dev, "timeout on status poll (expected %x got %x)\n",
 		 expected_val, val & mask);
 
 	return -ETIMEDOUT;
@@ -2196,7 +2196,7 @@ static int brcmnand_read(struct mtd_info *mtd, struct nand_chip *chip,
 				return err;
 		}
 
-		dev_dbg(ctrl->dev, "uncorrectable error at 0x%llx\n",
+		dev_err(ctrl->dev, "uncorrectable error at 0x%llx\n",
 			(unsigned long long)err_addr);
 		mtd->ecc_stats.failed++;
 		/* NAND layer expects zero on ECC errors */
@@ -2211,7 +2211,7 @@ static int brcmnand_read(struct mtd_info *mtd, struct nand_chip *chip,
 			err = brcmnand_read_by_pio(mtd, chip, addr, trans, buf,
 						   oob, &err_addr);
 
-		dev_dbg(ctrl->dev, "corrected error at 0x%llx\n",
+		dev_info(ctrl->dev, "corrected error at 0x%llx\n",
 			(unsigned long long)err_addr);
 		mtd->ecc_stats.corrected += corrected;
 		/* Always exceed the software-imposed threshold */
-- 
2.37.3

[PATCH v3 10/10] mtd: rawnand: brcmnand: allow for on-die ecc

[David Regan]

Allow settings for on-die ecc such that if on-die ECC is selected
don't error out but require ECC strap setting of zero

Signed-off-by: David Regan <dregan@broadcom.com>
Reviewed-by: William Zhang <william.zhang@broadcom.com>
---
Changes in v3: None
---
Changes in v2:
- Added to patch series
---
 drivers/mtd/nand/raw/brcmnand/brcmnand.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
index a4e311b6798c..42526f3250c9 100644
--- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
+++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
@@ -2727,9 +2727,11 @@ static int brcmnand_setup_dev(struct brcmnand_host *host)
 	cfg->blk_adr_bytes = get_blk_adr_bytes(mtd->size, mtd->writesize);
 
 	if (chip->ecc.engine_type != NAND_ECC_ENGINE_TYPE_ON_HOST) {
-		dev_err(ctrl->dev, "only HW ECC supported; selected: %d\n",
-			chip->ecc.engine_type);
-		return -EINVAL;
+		if (chip->ecc.strength) {
+			dev_err(ctrl->dev, "ERROR!!! HW ECC must be set to zero for non-hardware ECC; selected: %d\n",
+				chip->ecc.strength);
+			return -EINVAL;
+		}
 	}
 
 	if (chip->ecc.algo == NAND_ECC_ALGO_UNKNOWN) {
@@ -2797,7 +2799,11 @@ static int brcmnand_setup_dev(struct brcmnand_host *host)
 	if (ret)
 		return ret;
 
-	brcmnand_set_ecc_enabled(host, 1);
+	if (chip->ecc.engine_type == NAND_ECC_ENGINE_TYPE_ON_DIE) {
+		dev_dbg(ctrl->dev, "Disable HW ECC for on-die ECC\n");
+		brcmnand_set_ecc_enabled(host, 0);
+	} else
+		brcmnand_set_ecc_enabled(host, 1);
 
 	brcmnand_print_cfg(host, msg, cfg);
 	dev_info(ctrl->dev, "detected %s\n", msg);
-- 
2.37.3

Re: [PATCH v3 01/10] dt-bindings: mtd: brcmnand: Updates for bcmbca SoCs

[Conor Dooley]

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

Hey,

On Tue, Jan 23, 2024 at 07:04:49PM -0800, David Regan wrote:
> From: William Zhang <william.zhang@broadcom.com>
> 
> Update the descriptions to reflect different families of broadband SoC and
> use the general name bcmbca for ARM based SoC.
> 
> Add brcm,nand-use-wp property to have an option for disabling this
> feature on broadband board design that does not use write protection.
> 
> Add brcm,nand-ecc-use-strap to get ecc setting from board boot strap for
> broadband board designs because they do not specify ecc setting in dts
> but rather using the strap setting.
> 
> Remove the requirement of interrupts property to reflect the driver
> code. Also add myself to the list of maintainers.
> 
> Signed-off-by: William Zhang <william.zhang@broadcom.com>
> Reviewed-by: David Regan <dregan@broadcom.com>
> ---
> Changes in v3:
> - Update brcm,nand-use-wp description
> - Revert the description change to BCM63168 SoC-specific NAND controller
> ---
> Changes in v2:
> - Revert the new compatible string nand-bcmbca
> - Drop the BCM63168 compatible fix to avoid any potential ABI
> incompatibility issue
> - Simplify the explanation for brcm,nand-use-wp
> - Keep the interrupt name requirement when interrupt number is specified
> ---
>  .../bindings/mtd/brcm,brcmnand.yaml           | 37 ++++++++++++++++---
>  1 file changed, 32 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/brcm,brcmnand.yaml b/Documentation/devicetree/bindings/mtd/brcm,brcmnand.yaml
> index f57e96374e67..752c6ee98a7d 100644
> --- a/Documentation/devicetree/bindings/mtd/brcm,brcmnand.yaml
> +++ b/Documentation/devicetree/bindings/mtd/brcm,brcmnand.yaml
> @@ -9,6 +9,7 @@ title: Broadcom STB NAND Controller
>  maintainers:
>    - Brian Norris <computersforpeace@gmail.com>
>    - Kamal Dasu <kdasu.kdev@gmail.com>
> +  - William Zhang <william.zhang@broadcom.com>
>  
>  description: |
>    The Broadcom Set-Top Box NAND controller supports low-level access to raw NAND
> @@ -18,9 +19,10 @@ description: |
>    supports basic PROGRAM and READ functions, among other features.
>  
>    This controller was originally designed for STB SoCs (BCM7xxx) but is now
> -  available on a variety of Broadcom SoCs, including some BCM3xxx, BCM63xx, and
> -  iProc/Cygnus. Its history includes several similar (but not fully register
> -  compatible) versions.
> +  available on a variety of Broadcom SoCs, including some BCM3xxx, MIPS based
> +  Broadband SoC (BCM63xx), ARM based Broadband SoC (BCMBCA) and iProc/Cygnus.
> +  Its history includes several similar (but not fully register compatible)
> +  versions.
>  
>    -- Additional SoC-specific NAND controller properties --
>  
> @@ -53,7 +55,7 @@ properties:
>                - brcm,brcmnand-v7.2
>                - brcm,brcmnand-v7.3
>            - const: brcm,brcmnand
> -      - description: BCM63138 SoC-specific NAND controller
> +      - description: BCMBCA SoC-specific NAND controller
>          items:
>            - const: brcm,nand-bcm63138
>            - enum:
> @@ -111,6 +113,20 @@ properties:
>        earlier versions of this core that include WP
>      type: boolean
>  
> +  brcm,nand-use-wp:
> +    description:
> +      Use this property to indicate if board design uses
> +      controller's write protection feature and connects its
> +      NAND_WPb pin to nand chip's WP_L pin.

> By default the driver
> +      uses a module parameter with default value set to enable to
> +      control this feature for all boards. Use this dts property to
> +      override the default behavior and enable/disable this feature
> +      through board dts on a per board basis.

None of this information about module parameters is relevant in the
binding, as it should be independent of the implementation of one
particular operating system.

If the module parameter is not provided, what does the driver do?
If "wp_on" is the module parameter, then the default is to enable the
write protection feature. If that's correct, it seems to me that the
property should be called something like "brcm,no-wp". This property
would be a boolean that indicates that the NAND_WPb and WP_L pins are
not connected & nothing more. Clearly if the module param tries to
enable WP and the DT property indicates that it is not supported you
can decline to enable the feature, but that is up to the drivers in
the OS to decide.


> +      Set to 0 if WP pins are not connected and feature is not
> +      used. Set to 1 if WP pins are connected and feature is used.

As it stands, this property is firmly in the "software policy" side
of things, as it is being used as an override for a module parameter,
particularly given you have properties for each direction. Whether or
not the feature is to be used by the operating system is not relevant to
the binding.

> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [0, 1]
> +
>  patternProperties:
>    "^nand@[a-f0-9]$":
>      type: object
> @@ -137,6 +153,16 @@ patternProperties:
>            layout.
>          $ref: /schemas/types.yaml#/definitions/uint32
>  
> +      brcm,nand-ecc-use-strap:
> +        description:
> +          This flag is used by the driver to get the ecc strength and
> +          spare area size from the SoC NAND boot strap setting. This
> +          is commonly used by the BCMBCA SoC board design.

Quoting from v1, as I didn't get to it in time:

| > This property I'm not all that sure about either. If the specific
| > properties that you mention here are not set in the DT what happens at
| > the moment?
| > 
| In that case, the ecc strength and oob size come from ONFI or nand id 
| decoding.  But that is usually the minimum requirement and user can 
| choose to use stronger ecc as long as there is enough oob spare area in 
| the nand chip.
| 
| > I suppose what I am getting at is why are the bootstrap settings not
| > always used in the absence of specific values provided in the DT?
| > 
| This is because the STB, iProc and other chip and their board design may 
| not have or use the strap setting. But for BCMBCA SoC and board 
| reference design, we always use the strap setting.

My main question here I suppose is why would you need this property at
all? If the specific properties are provided (nand-ecc-strength etc)
then use them, and if they are not use the strap settings?

If there's no properties and no strap settings, the this property does
not help. If there's properties and strap settings, but properties are
wrong, then you just have some devicetrees that incorrectly describe the
hardware and need to be fixed.

> If ecc
> +          strength and spare area size are set by nand-ecc-strength
> +          and brcm,nand-oob-sector-size in the dts, these settings
> +          have precedence and override this flag.

Again, IMO, this is not for the binding to decide. That is a decision
for the operating system to make.

I am confused!


> +        $ref: /schemas/types.yaml#/definitions/flag
> +
>      unevaluatedProperties: false
>  
>  allOf:
> @@ -177,6 +203,8 @@ allOf:
>              - const: iproc-idm
>              - const: iproc-ext
>    - if:
> +      required:
> +        - interrupts
>        properties:
>          interrupts:
>            minItems: 2
> @@ -189,7 +217,6 @@ unevaluatedProperties: false
>  required:
>    - reg
>    - reg-names
> -  - interrupts
>  
>  examples:
>    - |
> -- 
> 2.37.3
> 

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

Re: [PATCH v3 01/10] dt-bindings: mtd: brcmnand: Updates for bcmbca SoCs

[Conor Dooley]

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

On Tue, Jan 23, 2024 at 07:04:49PM -0800, David Regan wrote:
> From: William Zhang <william.zhang@broadcom.com>
> 
> Update the descriptions to reflect different families of broadband SoC and
> use the general name bcmbca for ARM based SoC.
> 
> Add brcm,nand-use-wp property to have an option for disabling this
> feature on broadband board design that does not use write protection.
> 
> Add brcm,nand-ecc-use-strap to get ecc setting from board boot strap for
> broadband board designs because they do not specify ecc setting in dts
> but rather using the strap setting.
> 
> Remove the requirement of interrupts property to reflect the driver
> code. Also add myself to the list of maintainers.
> 
> Signed-off-by: William Zhang <william.zhang@broadcom.com>
> Reviewed-by: David Regan <dregan@broadcom.com>

Oh, and the patch is missing your signoff David.

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

Re: [PATCH v3 02/10] ARM: dts: broadcom: bcmbca: Add NAND controller node

[Miquel Raynal]

Hi David,

dregan@broadcom.com wrote on Tue, 23 Jan 2024 19:04:50 -0800:

> From: William Zhang <william.zhang@broadcom.com>
> 
> Add support for Broadcom STB NAND controller in BCMBCA ARMv7 chip dts
> files.
> 
> Signed-off-by: William Zhang <william.zhang@broadcom.com>
> Reviewed-by: David Regan <dregan@broadcom.com>
> ---
> Changes in v3: None
> ---
> Changes in v2: None
> ---
>  arch/arm/boot/dts/broadcom/bcm47622.dtsi    | 17 +++++++++++++++++
>  arch/arm/boot/dts/broadcom/bcm63138.dtsi    | 10 +++++++++-
>  arch/arm/boot/dts/broadcom/bcm63148.dtsi    | 17 +++++++++++++++++
>  arch/arm/boot/dts/broadcom/bcm63178.dtsi    | 17 +++++++++++++++++
>  arch/arm/boot/dts/broadcom/bcm6756.dtsi     | 17 +++++++++++++++++
>  arch/arm/boot/dts/broadcom/bcm6846.dtsi     | 17 +++++++++++++++++
>  arch/arm/boot/dts/broadcom/bcm6855.dtsi     | 17 +++++++++++++++++
>  arch/arm/boot/dts/broadcom/bcm6878.dtsi     | 17 +++++++++++++++++
>  arch/arm/boot/dts/broadcom/bcm947622.dts    |  4 ++++
>  arch/arm/boot/dts/broadcom/bcm963138.dts    |  4 ++++
>  arch/arm/boot/dts/broadcom/bcm963138dvt.dts | 12 +++++-------
>  arch/arm/boot/dts/broadcom/bcm963148.dts    |  4 ++++
>  arch/arm/boot/dts/broadcom/bcm963178.dts    |  4 ++++
>  arch/arm/boot/dts/broadcom/bcm96756.dts     |  4 ++++
>  arch/arm/boot/dts/broadcom/bcm96846.dts     |  4 ++++
>  arch/arm/boot/dts/broadcom/bcm96855.dts     |  4 ++++
>  arch/arm/boot/dts/broadcom/bcm96878.dts     |  4 ++++
>  17 files changed, 165 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/broadcom/bcm47622.dtsi b/arch/arm/boot/dts/broadcom/bcm47622.dtsi
> index 7cd38de118c3..55ff18043d96 100644
> --- a/arch/arm/boot/dts/broadcom/bcm47622.dtsi
> +++ b/arch/arm/boot/dts/broadcom/bcm47622.dtsi
> @@ -138,6 +138,23 @@ hsspi: spi@1000 {
>  			status = "disabled";
>  		};
>  
> +		nand_controller: nand-controller@1800 {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			compatible = "brcm,nand-bcm63138", "brcm,brcmnand-v7.1", "brcm,brcmnand";
> +			reg = <0x1800 0x600>, <0x2000 0x10>;
> +			reg-names = "nand", "nand-int-base";
> +			brcm,nand-use-wp = <0>;
> +			status = "disabled";
> +
> +			nandcs: nand@0 {
> +				compatible = "brcm,nandcs";
> +				reg = <0>;
> +				nand-on-flash-bbt;
> +				brcm,nand-ecc-use-strap;

Describing the NAND chip in a SoC DTSI does not look relevant to me.
Even more if you add something like this nand-ecc-use-strap setting
which is very board dependent.

Same applies to your arm64 DT patch.

Thanks,
Miquèl

Re: [PATCH v3 06/10] mtd: rawnand: brcmnand: Add support for getting ecc setting from strap

[Miquel Raynal]

Hi David,

> @@ -2622,19 +2667,43 @@ static int brcmnand_setup_dev(struct brcmnand_host *host)
>  		nanddev_get_memorg(&chip->base);
>  	struct brcmnand_controller *ctrl = host->ctrl;
>  	struct brcmnand_cfg *cfg = &host->hwcfg;
> -	char msg[128];
> +	struct device_node *np = nand_get_flash_node(chip);
>  	u32 offs, tmp, oob_sector;
> -	int ret;
> +	int ret, sector_size_1k = 0;
> +	bool use_strap = false;
> +	char msg[128];
>  
>  	memset(cfg, 0, sizeof(*cfg));
> +	use_strap = of_property_read_bool(np, "brcm,nand-ecc-use-strap");
>  
> -	ret = of_property_read_u32(nand_get_flash_node(chip),
> -				   "brcm,nand-oob-sector-size",
> +	/*
> +	 * Set ECC size and strength based on hw configuration from strap
> +	 * if device tree does not specify them and use strap property is set
> +	 * If ecc strength is set in dts, don't use strap setting.
> +	 */

You would have to use the strap settings only if the property is set.
If not property is set, the default from the core should apply I guess.

> +	if (chip->ecc.strength)
> +		use_strap = 0;
> +
> +	if (use_strap) {
> +		chip->ecc.strength = brcmnand_get_ecc_strength(host);
> +		sector_size_1k = brcmnand_get_sector_size_1k(host);
> +		if (chip->ecc.size == 0) {
> +			if (sector_size_1k < 0)
> +				chip->ecc.size = 512;
> +			else
> +				chip->ecc.size = 512 << sector_size_1k;
> +		}
> +	}
> +
> +	ret = of_property_read_u32(np, "brcm,nand-oob-sector-size",
>  				   &oob_sector);
>  	if (ret) {
> -		/* Use detected size */
> -		cfg->spare_area_size = mtd->oobsize /
> -					(mtd->writesize >> FC_SHIFT);
> +		if (use_strap)
> +			cfg->spare_area_size = brcmnand_get_spare_size(host);
> +		else
> +			/* Use detected size */
> +			cfg->spare_area_size = mtd->oobsize /
> +						(mtd->writesize >> FC_SHIFT);
>  	} else {
>  		cfg->spare_area_size = oob_sector;
>  	}


Thanks,
Miquèl

Re: [PATCH v3 01/10] dt-bindings: mtd: brcmnand: Updates for bcmbca SoCs

[Miquel Raynal]

Hi David,

dregan@broadcom.com wrote on Tue, 23 Jan 2024 19:04:49 -0800:

> From: William Zhang <william.zhang@broadcom.com>
> 
> Update the descriptions to reflect different families of broadband SoC and
> use the general name bcmbca for ARM based SoC.
> 
> Add brcm,nand-use-wp property to have an option for disabling this
> feature on broadband board design that does not use write protection.
> 
> Add brcm,nand-ecc-use-strap to get ecc setting from board boot strap for
> broadband board designs because they do not specify ecc setting in dts
> but rather using the strap setting.
> 
> Remove the requirement of interrupts property to reflect the driver
> code. Also add myself to the list of maintainers.
> 
> Signed-off-by: William Zhang <william.zhang@broadcom.com>
> Reviewed-by: David Regan <dregan@broadcom.com>

Please split this into three commits. dt-bindings are hard enough to
think again, mixing changes is definitely not a good idea.

Thanks,
Miquèl

Re: [PATCH v3 08/10] mtd: rawnand: brcmnand: exec_op helper functions return type fixes

[Miquel Raynal]

Hi David,

dregan@broadcom.com wrote on Tue, 23 Jan 2024 19:04:56 -0800:

> fix return type for exec_op reset and status detect helper functions

Style, please.

Fix								--> .

> 
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: http://lists.infradead.org/pipermail/linux-mtd/2023-December/102423.html
> Fixes: 3cc4718fa644 ("mtd: rawnand: brcmnand: exec_op implementation")
> Signed-off-by: David Regan <dregan@broadcom.com>
> Reviewed-by: William Zhang <william.zhang@broadcom.com>
> ---
> Changes in v3: None
> ---
> Changes in v2:
> - Added to patch series
> ---
>  drivers/mtd/nand/raw/brcmnand/brcmnand.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> index 9a904c7c6dad..6b5d76eff0ec 100644
> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> @@ -625,7 +625,7 @@ enum {
>  /* Only for v7.2 */
>  #define	ACC_CONTROL_ECC_EXT_SHIFT		13
>  
> -static u8 brcmnand_status(struct brcmnand_host *host);
> +static int brcmnand_status(struct brcmnand_host *host);
>  
>  static inline bool brcmnand_non_mmio_ops(struct brcmnand_controller *ctrl)
>  {
> @@ -1749,7 +1749,7 @@ static int brcmnand_waitfunc(struct nand_chip *chip)
>  				 INTFC_FLASH_STATUS;
>  }
>  
> -static u8 brcmnand_status(struct brcmnand_host *host)
> +static int brcmnand_status(struct brcmnand_host *host)
>  {
>  	struct nand_chip *chip = &host->chip;
>  	struct mtd_info *mtd = nand_to_mtd(chip);
> @@ -1760,7 +1760,7 @@ static u8 brcmnand_status(struct brcmnand_host *host)
>  	return brcmnand_waitfunc(chip);
>  }
>  
> -static u8 brcmnand_reset(struct brcmnand_host *host)
> +static int brcmnand_reset(struct brcmnand_host *host)
>  {
>  	struct nand_chip *chip = &host->chip;
>  
> @@ -2492,11 +2492,14 @@ static int brcmnand_exec_op(struct nand_chip *chip,
>  
>  	if (brcmnand_op_is_status(op)) {
>  		status = op->instrs[1].ctx.data.buf.in;
> -		*status = brcmnand_status(host);
> +		ret = brcmnand_status(host);
> +		if (ret < 0)
> +			return ret;
> +
> +		*status = ret & 0xFF;
>  
>  		return 0;
> -	}
> -	else if (brcmnand_op_is_reset(op)) {
> +	} else if (brcmnand_op_is_reset(op)) {
>  		ret = brcmnand_reset(host);
>  		if (ret < 0)
>  			return ret;


Thanks,
Miquèl

Re: [PATCH v3 09/10] mtd: rawnand: brcmnand: update log level messages

[Miquel Raynal]

Hi David,

dregan@broadcom.com wrote on Tue, 23 Jan 2024 19:04:57 -0800:

> Update log level messages so that more critical messages
> can be seen.
> 
> Signed-off-by: David Regan <dregan@broadcom.com>
> Reviewed-by: William Zhang <william.zhang@broadcom.com>
> ---
> Changes in v3: None
> ---
> Changes in v2:
> - Added to patch series
> ---
>  drivers/mtd/nand/raw/brcmnand/brcmnand.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> index 6b5d76eff0ec..a4e311b6798c 100644
> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> @@ -1143,7 +1143,7 @@ static int bcmnand_ctrl_poll_status(struct brcmnand_host *host,
>  	if ((val & mask) == expected_val)
>  		return 0;
>  
> -	dev_warn(ctrl->dev, "timeout on status poll (expected %x got %x)\n",
> +	dev_err(ctrl->dev, "timeout on status poll (expected %x got %x)\n",

I don't see the point but if you want.

>  		 expected_val, val & mask);
>  
>  	return -ETIMEDOUT;
> @@ -2196,7 +2196,7 @@ static int brcmnand_read(struct mtd_info *mtd, struct nand_chip *chip,
>  				return err;
>  		}
>  
> -		dev_dbg(ctrl->dev, "uncorrectable error at 0x%llx\n",
> +		dev_err(ctrl->dev, "uncorrectable error at 0x%llx\n",

Upper layer will complain, you can keep this at the debug level.

>  			(unsigned long long)err_addr);
>  		mtd->ecc_stats.failed++;
>  		/* NAND layer expects zero on ECC errors */
> @@ -2211,7 +2211,7 @@ static int brcmnand_read(struct mtd_info *mtd, struct nand_chip *chip,
>  			err = brcmnand_read_by_pio(mtd, chip, addr, trans, buf,
>  						   oob, &err_addr);
>  
> -		dev_dbg(ctrl->dev, "corrected error at 0x%llx\n",
> +		dev_info(ctrl->dev, "corrected error at 0x%llx\n",

Definitely not! Way too verbose. Please keep this one as it is.

>  			(unsigned long long)err_addr);
>  		mtd->ecc_stats.corrected += corrected;
>  		/* Always exceed the software-imposed threshold */


Thanks,
Miquèl

Re: [PATCH v3 10/10] mtd: rawnand: brcmnand: allow for on-die ecc

[Miquel Raynal]

Hi David,

dregan@broadcom.com wrote on Tue, 23 Jan 2024 19:04:58 -0800:

> Allow settings for on-die ecc such that if on-die ECC is selected
> don't error out but require ECC strap setting of zero
> 
> Signed-off-by: David Regan <dregan@broadcom.com>
> Reviewed-by: William Zhang <william.zhang@broadcom.com>
> ---
> Changes in v3: None
> ---
> Changes in v2:
> - Added to patch series
> ---
>  drivers/mtd/nand/raw/brcmnand/brcmnand.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> index a4e311b6798c..42526f3250c9 100644
> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> @@ -2727,9 +2727,11 @@ static int brcmnand_setup_dev(struct brcmnand_host *host)
>  	cfg->blk_adr_bytes = get_blk_adr_bytes(mtd->size, mtd->writesize);
>  
>  	if (chip->ecc.engine_type != NAND_ECC_ENGINE_TYPE_ON_HOST) {
> -		dev_err(ctrl->dev, "only HW ECC supported; selected: %d\n",
> -			chip->ecc.engine_type);
> -		return -EINVAL;
> +		if (chip->ecc.strength) {
> +			dev_err(ctrl->dev, "ERROR!!! HW ECC must be set to zero for non-hardware ECC; selected: %d\n",
> +				chip->ecc.strength);

Can you use a more formal string? Also clarify it because I don't
really understand what it leads to.

> +			return -EINVAL;
> +		}
>  	}
>  
>  	if (chip->ecc.algo == NAND_ECC_ALGO_UNKNOWN) {
> @@ -2797,7 +2799,11 @@ static int brcmnand_setup_dev(struct brcmnand_host *host)
>  	if (ret)
>  		return ret;
>  
> -	brcmnand_set_ecc_enabled(host, 1);
> +	if (chip->ecc.engine_type == NAND_ECC_ENGINE_TYPE_ON_DIE) {
> +		dev_dbg(ctrl->dev, "Disable HW ECC for on-die ECC\n");

Not needed.

> +		brcmnand_set_ecc_enabled(host, 0);
> +	} else
> +		brcmnand_set_ecc_enabled(host, 1);

Style is wrong, but otherwise I think ECC should be kept disabled while
not in active use, so I am a bit surprised by this line.

>  
>  	brcmnand_print_cfg(host, msg, cfg);
>  	dev_info(ctrl->dev, "detected %s\n", msg);


Thanks,
Miquèl

Re: [PATCH v3 01/10] dt-bindings: mtd: brcmnand: Updates for bcmbca SoCs

[William Zhang]

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

Hi Conor,

On 1/24/24 09:24, Conor Dooley wrote:
> Hey,
> 
> On Tue, Jan 23, 2024 at 07:04:49PM -0800, David Regan wrote:
>> From: William Zhang <william.zhang@broadcom.com>
>>
>> Update the descriptions to reflect different families of broadband SoC and
>> use the general name bcmbca for ARM based SoC.
>>
>> Add brcm,nand-use-wp property to have an option for disabling this
>> feature on broadband board design that does not use write protection.
>>
>> Add brcm,nand-ecc-use-strap to get ecc setting from board boot strap for
>> broadband board designs because they do not specify ecc setting in dts
>> but rather using the strap setting.
>>
>> Remove the requirement of interrupts property to reflect the driver
>> code. Also add myself to the list of maintainers.
>>
>> Signed-off-by: William Zhang <william.zhang@broadcom.com>
>> Reviewed-by: David Regan <dregan@broadcom.com>
>> ---
>> Changes in v3:
>> - Update brcm,nand-use-wp description
>> - Revert the description change to BCM63168 SoC-specific NAND controller
>> ---
>> Changes in v2:
>> - Revert the new compatible string nand-bcmbca
>> - Drop the BCM63168 compatible fix to avoid any potential ABI
>> incompatibility issue
>> - Simplify the explanation for brcm,nand-use-wp
>> - Keep the interrupt name requirement when interrupt number is specified
>> ---
>>   .../bindings/mtd/brcm,brcmnand.yaml           | 37 ++++++++++++++++---
>>   1 file changed, 32 insertions(+), 5 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/mtd/brcm,brcmnand.yaml b/Documentation/devicetree/bindings/mtd/brcm,brcmnand.yaml
>> index f57e96374e67..752c6ee98a7d 100644
>> --- a/Documentation/devicetree/bindings/mtd/brcm,brcmnand.yaml
>> +++ b/Documentation/devicetree/bindings/mtd/brcm,brcmnand.yaml
>> @@ -9,6 +9,7 @@ title: Broadcom STB NAND Controller
>>   maintainers:
>>     - Brian Norris <computersforpeace@gmail.com>
>>     - Kamal Dasu <kdasu.kdev@gmail.com>
>> +  - William Zhang <william.zhang@broadcom.com>
>>   
>>   description: |
>>     The Broadcom Set-Top Box NAND controller supports low-level access to raw NAND
>> @@ -18,9 +19,10 @@ description: |
>>     supports basic PROGRAM and READ functions, among other features.
>>   
>>     This controller was originally designed for STB SoCs (BCM7xxx) but is now
>> -  available on a variety of Broadcom SoCs, including some BCM3xxx, BCM63xx, and
>> -  iProc/Cygnus. Its history includes several similar (but not fully register
>> -  compatible) versions.
>> +  available on a variety of Broadcom SoCs, including some BCM3xxx, MIPS based
>> +  Broadband SoC (BCM63xx), ARM based Broadband SoC (BCMBCA) and iProc/Cygnus.
>> +  Its history includes several similar (but not fully register compatible)
>> +  versions.
>>   
>>     -- Additional SoC-specific NAND controller properties --
>>   
>> @@ -53,7 +55,7 @@ properties:
>>                 - brcm,brcmnand-v7.2
>>                 - brcm,brcmnand-v7.3
>>             - const: brcm,brcmnand
>> -      - description: BCM63138 SoC-specific NAND controller
>> +      - description: BCMBCA SoC-specific NAND controller
>>           items:
>>             - const: brcm,nand-bcm63138
>>             - enum:
>> @@ -111,6 +113,20 @@ properties:
>>         earlier versions of this core that include WP
>>       type: boolean
>>   
>> +  brcm,nand-use-wp:
>> +    description:
>> +      Use this property to indicate if board design uses
>> +      controller's write protection feature and connects its
>> +      NAND_WPb pin to nand chip's WP_L pin.
> 
>> By default the driver
>> +      uses a module parameter with default value set to enable to
>> +      control this feature for all boards. Use this dts property to
>> +      override the default behavior and enable/disable this feature
>> +      through board dts on a per board basis.
> 
> None of this information about module parameters is relevant in the
> binding, as it should be independent of the implementation of one
> particular operating system.
> 
Agree this is OS related stuff. I was trying to make it more clean by 
explaining how it is implemented in linux. And if we were to implement 
the driver for another OS, there will be at least a default value for 
wp_on. I will remove any mention of module parameter from this doc.

> If the module parameter is not provided, what does the driver do?
> If "wp_on" is the module parameter, then the default is to enable the
> write protection feature. If that's correct, it seems to me that the
> property should be called something like "brcm,no-wp". This property
> would be a boolean that indicates that the NAND_WPb and WP_L pins are
> not connected & nothing more. Clearly if the module param tries to
> enable WP and the DT property indicates that it is not supported you
> can decline to enable the feature, but that is up to the drivers in
> the OS to decide.
> 
If I were to implement this from day one, I certainly will choose the 
same approach as you suggested here.  But there is existing 
brcm,nand-has-wp property for other purpose and now if we have 
brcm,no-wp, it will be more confusing with that property IMHO. I prefer 
to keep use the "has" for feature availability and "use" for feature 
being used or not.

And the reason I keep it as int is because there could be a potential 
value of 2 for another mode that the current driver could set the wp_on 
to. I don't see it is being used in BCMBCA product but I am not sure 
about other SoC family. So I don't want to completely close the door 
here just in case.

> 
>> +      Set to 0 if WP pins are not connected and feature is not
>> +      used. Set to 1 if WP pins are connected and feature is used.
> 
> As it stands, this property is firmly in the "software policy" side
> of things, as it is being used as an override for a module parameter,
> particularly given you have properties for each direction. Whether or
> not the feature is to be used by the operating system is not relevant to
> the binding.
> 
I don't understand why this is a software policy.  This is the board 
design choice although it does override the driver default value. But it 
is still board or device setting and describe the hardware. OS has to 
follow this hardware configuration and set up accordingly.

>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    enum: [0, 1]
>> +
>>   patternProperties:
>>     "^nand@[a-f0-9]$":
>>       type: object
>> @@ -137,6 +153,16 @@ patternProperties:
>>             layout.
>>           $ref: /schemas/types.yaml#/definitions/uint32
>>   
>> +      brcm,nand-ecc-use-strap:
>> +        description:
>> +          This flag is used by the driver to get the ecc strength and
>> +          spare area size from the SoC NAND boot strap setting. This
>> +          is commonly used by the BCMBCA SoC board design.
> 
> Quoting from v1, as I didn't get to it in time:
> 
> | > This property I'm not all that sure about either. If the specific
> | > properties that you mention here are not set in the DT what happens at
> | > the moment?
> | >
> | In that case, the ecc strength and oob size come from ONFI or nand id
> | decoding.  But that is usually the minimum requirement and user can
> | choose to use stronger ecc as long as there is enough oob spare area in
> | the nand chip.
> |
> | > I suppose what I am getting at is why are the bootstrap settings not
> | > always used in the absence of specific values provided in the DT?
> | >
> | This is because the STB, iProc and other chip and their board design may
> | not have or use the strap setting. But for BCMBCA SoC and board
> | reference design, we always use the strap setting.
> 
> My main question here I suppose is why would you need this property at
> all? If the specific properties are provided (nand-ecc-strength etc)
> then use them, and if they are not use the strap settings?If nand-ecc-strength does not exist, the current driver implementation 
use the auto detected ecc strength from the NAND chip ONFI parameter 
which is okay. But for BCABCM SoC and our reference board design, we 
don't use nand-ecc-strength and don't want to use the auto detected 
value(as they are typically minimum requirement) but rather use strap 
setting so I need a third choice.

> If there's no properties and no strap settings, the this property does
> not help. If there's properties and strap settings, but properties are
> wrong, then you just have some devicetrees that incorrectly describe the
> hardware and need to be fixed.
> 
True but manually setting nand-ecc-strength can be error prone like 
copy/paste error. The hardware strap setting does not involve such edit 
in the dts file so no error can happen.

>> If ecc
>> +          strength and spare area size are set by nand-ecc-strength
>> +          and brcm,nand-oob-sector-size in the dts, these settings
>> +          have precedence and override this flag.
> 
> Again, IMO, this is not for the binding to decide. That is a decision
> for the operating system to make.
> 
Again this is just for historic reason and BCMBCA as late player does 
not want to break any existing dts setting.  So I thought it is useful 
to explain here.  I can remove this text if you don't think it should be 
in the binding document.


> I am confused!
> 
> 
>> +        $ref: /schemas/types.yaml#/definitions/flag
>> +
>>       unevaluatedProperties: false
>>   
>>   allOf:
>> @@ -177,6 +203,8 @@ allOf:
>>               - const: iproc-idm
>>               - const: iproc-ext
>>     - if:
>> +      required:
>> +        - interrupts
>>         properties:
>>           interrupts:
>>             minItems: 2
>> @@ -189,7 +217,6 @@ unevaluatedProperties: false
>>   required:
>>     - reg
>>     - reg-names
>> -  - interrupts
>>   
>>   examples:
>>     - |
>> -- 
>> 2.37.3
>>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4212 bytes --]

Re: [PATCH v3 02/10] ARM: dts: broadcom: bcmbca: Add NAND controller node

[William Zhang]

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

Hi Miquel,

On 1/24/24 09:30, Miquel Raynal wrote:
> Hi David,
> 
> dregan@broadcom.com wrote on Tue, 23 Jan 2024 19:04:50 -0800:
> 
>> From: William Zhang <william.zhang@broadcom.com>
>>
>> Add support for Broadcom STB NAND controller in BCMBCA ARMv7 chip dts
>> files.
>>
>> Signed-off-by: William Zhang <william.zhang@broadcom.com>
>> Reviewed-by: David Regan <dregan@broadcom.com>
>> ---
>> Changes in v3: None
>> ---
>> Changes in v2: None
>> ---
>>   arch/arm/boot/dts/broadcom/bcm47622.dtsi    | 17 +++++++++++++++++
>>   arch/arm/boot/dts/broadcom/bcm63138.dtsi    | 10 +++++++++-
>>   arch/arm/boot/dts/broadcom/bcm63148.dtsi    | 17 +++++++++++++++++
>>   arch/arm/boot/dts/broadcom/bcm63178.dtsi    | 17 +++++++++++++++++
>>   arch/arm/boot/dts/broadcom/bcm6756.dtsi     | 17 +++++++++++++++++
>>   arch/arm/boot/dts/broadcom/bcm6846.dtsi     | 17 +++++++++++++++++
>>   arch/arm/boot/dts/broadcom/bcm6855.dtsi     | 17 +++++++++++++++++
>>   arch/arm/boot/dts/broadcom/bcm6878.dtsi     | 17 +++++++++++++++++
>>   arch/arm/boot/dts/broadcom/bcm947622.dts    |  4 ++++
>>   arch/arm/boot/dts/broadcom/bcm963138.dts    |  4 ++++
>>   arch/arm/boot/dts/broadcom/bcm963138dvt.dts | 12 +++++-------
>>   arch/arm/boot/dts/broadcom/bcm963148.dts    |  4 ++++
>>   arch/arm/boot/dts/broadcom/bcm963178.dts    |  4 ++++
>>   arch/arm/boot/dts/broadcom/bcm96756.dts     |  4 ++++
>>   arch/arm/boot/dts/broadcom/bcm96846.dts     |  4 ++++
>>   arch/arm/boot/dts/broadcom/bcm96855.dts     |  4 ++++
>>   arch/arm/boot/dts/broadcom/bcm96878.dts     |  4 ++++
>>   17 files changed, 165 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/arm/boot/dts/broadcom/bcm47622.dtsi b/arch/arm/boot/dts/broadcom/bcm47622.dtsi
>> index 7cd38de118c3..55ff18043d96 100644
>> --- a/arch/arm/boot/dts/broadcom/bcm47622.dtsi
>> +++ b/arch/arm/boot/dts/broadcom/bcm47622.dtsi
>> @@ -138,6 +138,23 @@ hsspi: spi@1000 {
>>   			status = "disabled";
>>   		};
>>   
>> +		nand_controller: nand-controller@1800 {
>> +			#address-cells = <1>;
>> +			#size-cells = <0>;
>> +			compatible = "brcm,nand-bcm63138", "brcm,brcmnand-v7.1", "brcm,brcmnand";
>> +			reg = <0x1800 0x600>, <0x2000 0x10>;
>> +			reg-names = "nand", "nand-int-base";
>> +			brcm,nand-use-wp = <0>;
>> +			status = "disabled";
>> +
>> +			nandcs: nand@0 {
>> +				compatible = "brcm,nandcs";
>> +				reg = <0>;
>> +				nand-on-flash-bbt;
>> +				brcm,nand-ecc-use-strap;
> 
> Describing the NAND chip in a SoC DTSI does not look relevant to me.
> Even more if you add something like this nand-ecc-use-strap setting
> which is very board dependent.
> 
I am not sure if I understand you comments correctly but are you 
suggesting to put this whole nand controller node into each board dts? 
We have other ip block nodes like SPI, uart in this same soc dtsi file 
too.  For all the bcmbca soc dtsi I am updating here(and its board 
design), we always use the strap to for ecc setting.  So I thought it 
should be okay to put brcm,nand-ecc-use-strap in the default dtsi file. 
For any board that uses the raw nand nand-ecc property, the board dts 
can do so and override the brcm,nand-ecc-use-strap setting.

> Same applies to your arm64 DT patch.
> 
> Thanks,
> Miquèl

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4212 bytes --]

Re: [PATCH v3 06/10] mtd: rawnand: brcmnand: Add support for getting ecc setting from strap

[William Zhang]

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

Hi Miquel,

On 1/24/24 09:32, Miquel Raynal wrote:
> Hi David,
> 
>> @@ -2622,19 +2667,43 @@ static int brcmnand_setup_dev(struct brcmnand_host *host)
>>   		nanddev_get_memorg(&chip->base);
>>   	struct brcmnand_controller *ctrl = host->ctrl;
>>   	struct brcmnand_cfg *cfg = &host->hwcfg;
>> -	char msg[128];
>> +	struct device_node *np = nand_get_flash_node(chip);
>>   	u32 offs, tmp, oob_sector;
>> -	int ret;
>> +	int ret, sector_size_1k = 0;
>> +	bool use_strap = false;
>> +	char msg[128];
>>   
>>   	memset(cfg, 0, sizeof(*cfg));
>> +	use_strap = of_property_read_bool(np, "brcm,nand-ecc-use-strap");
>>   
>> -	ret = of_property_read_u32(nand_get_flash_node(chip),
>> -				   "brcm,nand-oob-sector-size",
>> +	/*
>> +	 * Set ECC size and strength based on hw configuration from strap
>> +	 * if device tree does not specify them and use strap property is set
>> +	 * If ecc strength is set in dts, don't use strap setting.
>> +	 */
> 
> You would have to use the strap settings only if the property is set.
> If not property is set, the default from the core should apply I guess.
> 
Maybe the comment confuse you... I will make it more clear.  But the 
logic here is exactly what you said.  If no properties are set, the core 
value is already set in chip->ecc.strength and chip->ecc.size and it 
clear the use_strap flag too.

>> +	if (chip->ecc.strength)
>> +		use_strap = 0;
>> +
>> +	if (use_strap) {
>> +		chip->ecc.strength = brcmnand_get_ecc_strength(host);
>> +		sector_size_1k = brcmnand_get_sector_size_1k(host);
>> +		if (chip->ecc.size == 0) {
>> +			if (sector_size_1k < 0)
>> +				chip->ecc.size = 512;
>> +			else
>> +				chip->ecc.size = 512 << sector_size_1k;
>> +		}
>> +	}
>> +
>> +	ret = of_property_read_u32(np, "brcm,nand-oob-sector-size",
>>   				   &oob_sector);
>>   	if (ret) {
>> -		/* Use detected size */
>> -		cfg->spare_area_size = mtd->oobsize /
>> -					(mtd->writesize >> FC_SHIFT);
>> +		if (use_strap)
>> +			cfg->spare_area_size = brcmnand_get_spare_size(host);
>> +		else
>> +			/* Use detected size */
>> +			cfg->spare_area_size = mtd->oobsize /
>> +						(mtd->writesize >> FC_SHIFT);
>>   	} else {
>>   		cfg->spare_area_size = oob_sector;
>>   	}
> 
> 
> Thanks,
> Miquèl

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4212 bytes --]

Re: [PATCH v3 01/10] dt-bindings: mtd: brcmnand: Updates for bcmbca SoCs

[William Zhang]

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



On 1/24/24 09:34, Miquel Raynal wrote:
> Hi David,
> 
> dregan@broadcom.com wrote on Tue, 23 Jan 2024 19:04:49 -0800:
> 
>> From: William Zhang <william.zhang@broadcom.com>
>>
>> Update the descriptions to reflect different families of broadband SoC and
>> use the general name bcmbca for ARM based SoC.
>>
>> Add brcm,nand-use-wp property to have an option for disabling this
>> feature on broadband board design that does not use write protection.
>>
>> Add brcm,nand-ecc-use-strap to get ecc setting from board boot strap for
>> broadband board designs because they do not specify ecc setting in dts
>> but rather using the strap setting.
>>
>> Remove the requirement of interrupts property to reflect the driver
>> code. Also add myself to the list of maintainers.
>>
>> Signed-off-by: William Zhang <william.zhang@broadcom.com>
>> Reviewed-by: David Regan <dregan@broadcom.com>
> 
> Please split this into three commits. dt-bindings are hard enough to
> think again, mixing changes is definitely not a good idea.
> 
Will do

> Thanks,
> Miquèl

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4212 bytes --]

Re: [PATCH v3 02/10] ARM: dts: broadcom: bcmbca: Add NAND controller node

[Florian Fainelli]

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



On 1/24/2024 7:09 PM, William Zhang wrote:
> Hi Miquel,
> 
> On 1/24/24 09:30, Miquel Raynal wrote:
>> Hi David,
>>
>> dregan@broadcom.com wrote on Tue, 23 Jan 2024 19:04:50 -0800:
>>
>>> From: William Zhang <william.zhang@broadcom.com>
>>>
>>> Add support for Broadcom STB NAND controller in BCMBCA ARMv7 chip dts
>>> files.
>>>
>>> Signed-off-by: William Zhang <william.zhang@broadcom.com>
>>> Reviewed-by: David Regan <dregan@broadcom.com>
>>> ---
>>> Changes in v3: None
>>> ---
>>> Changes in v2: None
>>> ---
>>>   arch/arm/boot/dts/broadcom/bcm47622.dtsi    | 17 +++++++++++++++++
>>>   arch/arm/boot/dts/broadcom/bcm63138.dtsi    | 10 +++++++++-
>>>   arch/arm/boot/dts/broadcom/bcm63148.dtsi    | 17 +++++++++++++++++
>>>   arch/arm/boot/dts/broadcom/bcm63178.dtsi    | 17 +++++++++++++++++
>>>   arch/arm/boot/dts/broadcom/bcm6756.dtsi     | 17 +++++++++++++++++
>>>   arch/arm/boot/dts/broadcom/bcm6846.dtsi     | 17 +++++++++++++++++
>>>   arch/arm/boot/dts/broadcom/bcm6855.dtsi     | 17 +++++++++++++++++
>>>   arch/arm/boot/dts/broadcom/bcm6878.dtsi     | 17 +++++++++++++++++
>>>   arch/arm/boot/dts/broadcom/bcm947622.dts    |  4 ++++
>>>   arch/arm/boot/dts/broadcom/bcm963138.dts    |  4 ++++
>>>   arch/arm/boot/dts/broadcom/bcm963138dvt.dts | 12 +++++-------
>>>   arch/arm/boot/dts/broadcom/bcm963148.dts    |  4 ++++
>>>   arch/arm/boot/dts/broadcom/bcm963178.dts    |  4 ++++
>>>   arch/arm/boot/dts/broadcom/bcm96756.dts     |  4 ++++
>>>   arch/arm/boot/dts/broadcom/bcm96846.dts     |  4 ++++
>>>   arch/arm/boot/dts/broadcom/bcm96855.dts     |  4 ++++
>>>   arch/arm/boot/dts/broadcom/bcm96878.dts     |  4 ++++
>>>   17 files changed, 165 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/arch/arm/boot/dts/broadcom/bcm47622.dtsi 
>>> b/arch/arm/boot/dts/broadcom/bcm47622.dtsi
>>> index 7cd38de118c3..55ff18043d96 100644
>>> --- a/arch/arm/boot/dts/broadcom/bcm47622.dtsi
>>> +++ b/arch/arm/boot/dts/broadcom/bcm47622.dtsi
>>> @@ -138,6 +138,23 @@ hsspi: spi@1000 {
>>>               status = "disabled";
>>>           };
>>> +        nand_controller: nand-controller@1800 {
>>> +            #address-cells = <1>;
>>> +            #size-cells = <0>;
>>> +            compatible = "brcm,nand-bcm63138", "brcm,brcmnand-v7.1", 
>>> "brcm,brcmnand";
>>> +            reg = <0x1800 0x600>, <0x2000 0x10>;
>>> +            reg-names = "nand", "nand-int-base";
>>> +            brcm,nand-use-wp = <0>;
>>> +            status = "disabled";
>>> +
>>> +            nandcs: nand@0 {
>>> +                compatible = "brcm,nandcs";
>>> +                reg = <0>;
>>> +                nand-on-flash-bbt;
>>> +                brcm,nand-ecc-use-strap;
>>
>> Describing the NAND chip in a SoC DTSI does not look relevant to me.
>> Even more if you add something like this nand-ecc-use-strap setting
>> which is very board dependent.
>>
> I am not sure if I understand you comments correctly but are you 
> suggesting to put this whole nand controller node into each board dts? 
> We have other ip block nodes like SPI, uart in this same soc dtsi file 
> too.  For all the bcmbca soc dtsi I am updating here(and its board 
> design), we always use the strap to for ecc setting.  So I thought it 
> should be okay to put brcm,nand-ecc-use-strap in the default dtsi file. 
> For any board that uses the raw nand nand-ecc property, the board dts 
> can do so and override the brcm,nand-ecc-use-strap setting.

I read Miquel's comment as meaning that the nandcs aka the NAND 
chip/flash part description should be in the board .dts file, while the 
controller itself can remain in the .dtsi file with its status = 
"disabled" property.

Are there customer boards, that is non reference boards that might chose 
a different chip select number and/or not use the strap settings?
-- 
Florian

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

Re: [PATCH v3 02/10] ARM: dts: broadcom: bcmbca: Add NAND controller node

[William Zhang]

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



On 1/24/24 19:34, Florian Fainelli wrote:
> 
> 
> On 1/24/2024 7:09 PM, William Zhang wrote:
>> Hi Miquel,
>>
>> On 1/24/24 09:30, Miquel Raynal wrote:
>>> Hi David,
>>>
>>> dregan@broadcom.com wrote on Tue, 23 Jan 2024 19:04:50 -0800:
>>>
>>>> From: William Zhang <william.zhang@broadcom.com>
>>>>
>>>> Add support for Broadcom STB NAND controller in BCMBCA ARMv7 chip dts
>>>> files.
>>>>
>>>> Signed-off-by: William Zhang <william.zhang@broadcom.com>
>>>> Reviewed-by: David Regan <dregan@broadcom.com>
>>>> ---
>>>> Changes in v3: None
>>>> ---
>>>> Changes in v2: None
>>>> ---
>>>>   arch/arm/boot/dts/broadcom/bcm47622.dtsi    | 17 +++++++++++++++++
>>>>   arch/arm/boot/dts/broadcom/bcm63138.dtsi    | 10 +++++++++-
>>>>   arch/arm/boot/dts/broadcom/bcm63148.dtsi    | 17 +++++++++++++++++
>>>>   arch/arm/boot/dts/broadcom/bcm63178.dtsi    | 17 +++++++++++++++++
>>>>   arch/arm/boot/dts/broadcom/bcm6756.dtsi     | 17 +++++++++++++++++
>>>>   arch/arm/boot/dts/broadcom/bcm6846.dtsi     | 17 +++++++++++++++++
>>>>   arch/arm/boot/dts/broadcom/bcm6855.dtsi     | 17 +++++++++++++++++
>>>>   arch/arm/boot/dts/broadcom/bcm6878.dtsi     | 17 +++++++++++++++++
>>>>   arch/arm/boot/dts/broadcom/bcm947622.dts    |  4 ++++
>>>>   arch/arm/boot/dts/broadcom/bcm963138.dts    |  4 ++++
>>>>   arch/arm/boot/dts/broadcom/bcm963138dvt.dts | 12 +++++-------
>>>>   arch/arm/boot/dts/broadcom/bcm963148.dts    |  4 ++++
>>>>   arch/arm/boot/dts/broadcom/bcm963178.dts    |  4 ++++
>>>>   arch/arm/boot/dts/broadcom/bcm96756.dts     |  4 ++++
>>>>   arch/arm/boot/dts/broadcom/bcm96846.dts     |  4 ++++
>>>>   arch/arm/boot/dts/broadcom/bcm96855.dts     |  4 ++++
>>>>   arch/arm/boot/dts/broadcom/bcm96878.dts     |  4 ++++
>>>>   17 files changed, 165 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/arch/arm/boot/dts/broadcom/bcm47622.dtsi 
>>>> b/arch/arm/boot/dts/broadcom/bcm47622.dtsi
>>>> index 7cd38de118c3..55ff18043d96 100644
>>>> --- a/arch/arm/boot/dts/broadcom/bcm47622.dtsi
>>>> +++ b/arch/arm/boot/dts/broadcom/bcm47622.dtsi
>>>> @@ -138,6 +138,23 @@ hsspi: spi@1000 {
>>>>               status = "disabled";
>>>>           };
>>>> +        nand_controller: nand-controller@1800 {
>>>> +            #address-cells = <1>;
>>>> +            #size-cells = <0>;
>>>> +            compatible = "brcm,nand-bcm63138", 
>>>> "brcm,brcmnand-v7.1", "brcm,brcmnand";
>>>> +            reg = <0x1800 0x600>, <0x2000 0x10>;
>>>> +            reg-names = "nand", "nand-int-base";
>>>> +            brcm,nand-use-wp = <0>;
>>>> +            status = "disabled";
>>>> +
>>>> +            nandcs: nand@0 {
>>>> +                compatible = "brcm,nandcs";
>>>> +                reg = <0>;
>>>> +                nand-on-flash-bbt;
>>>> +                brcm,nand-ecc-use-strap;
>>>
>>> Describing the NAND chip in a SoC DTSI does not look relevant to me.
>>> Even more if you add something like this nand-ecc-use-strap setting
>>> which is very board dependent.
>>>
>> I am not sure if I understand you comments correctly but are you 
>> suggesting to put this whole nand controller node into each board dts? 
>> We have other ip block nodes like SPI, uart in this same soc dtsi file 
>> too.  For all the bcmbca soc dtsi I am updating here(and its board 
>> design), we always use the strap to for ecc setting.  So I thought it 
>> should be okay to put brcm,nand-ecc-use-strap in the default dtsi 
>> file. For any board that uses the raw nand nand-ecc property, the 
>> board dts can do so and override the brcm,nand-ecc-use-strap setting.
> 
> I read Miquel's comment as meaning that the nandcs aka the NAND 
> chip/flash part description should be in the board .dts file, while the 
> controller itself can remain in the .dtsi file with its status = 
> "disabled" property.
> 
> Are there customer boards, that is non reference boards that might chose 
> a different chip select number and/or not use the strap settings?
In BCMBCA SoC, there is only one cs and customer design also have to use 
strap for the bootrom to boot up properly.  They can override it with 
dts in linux but I don't think any customer would do that.

Maybe the nand-on-flash-bbt could be possible item that customer may 
have to set it differently if they don't follow our reference software 
design.

I will move the nand-on-flash-bbt to the board dts but I would like to 
keep the other default nandcs settings in SoC.dsti if that is not too 
out of the conventional rule and Miquel is okay with it.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4212 bytes --]

Re: [PATCH v3 02/10] ARM: dts: broadcom: bcmbca: Add NAND controller node

[Miquel Raynal]

Hi William,

> >>>> +        nand_controller: nand-controller@1800 {
> >>>> +            #address-cells = <1>;
> >>>> +            #size-cells = <0>;
> >>>> +            compatible = "brcm,nand-bcm63138", >>>> "brcm,brcmnand-v7.1", "brcm,brcmnand";
> >>>> +            reg = <0x1800 0x600>, <0x2000 0x10>;
> >>>> +            reg-names = "nand", "nand-int-base";
> >>>> +            brcm,nand-use-wp = <0>;
> >>>> +            status = "disabled";
> >>>> +
> >>>> +            nandcs: nand@0 {
> >>>> +                compatible = "brcm,nandcs";
> >>>> +                reg = <0>;
> >>>> +                nand-on-flash-bbt;
> >>>> +                brcm,nand-ecc-use-strap;  
> >>>
> >>> Describing the NAND chip in a SoC DTSI does not look relevant to me.
> >>> Even more if you add something like this nand-ecc-use-strap setting
> >>> which is very board dependent.
> >>>  
> >> I am not sure if I understand you comments correctly but are you >> suggesting to put this whole nand controller node into each board dts? >> We have other ip block nodes like SPI, uart in this same soc dtsi file >> too.  For all the bcmbca soc dtsi I am updating here(and its board >> design), we always use the strap to for ecc setting.  So I thought it >> should be okay to put brcm,nand-ecc-use-strap in the default dtsi >> file. For any board that uses the raw nand nand-ecc property, the >> board dts can do so and override the brcm,nand-ecc-use-strap setting.  
> > 
> > I read Miquel's comment as meaning that the nandcs aka the NAND > chip/flash part description should be in the board .dts file, while the > controller itself can remain in the .dtsi file with its status = > "disabled" property.
> > 
> > Are there customer boards, that is non reference boards that might chose > a different chip select number and/or not use the strap settings?  
> In BCMBCA SoC, there is only one cs and customer design also have to use strap for the bootrom to boot up properly.  They can override it with dts in linux but I don't think any customer would do that.
> 
> Maybe the nand-on-flash-bbt could be possible item that customer may have to set it differently if they don't follow our reference software design.
> 
> I will move the nand-on-flash-bbt to the board dts but I would like to keep the other default nandcs settings in SoC.dsti if that is not too out of the conventional rule and Miquel is okay with it.

I think there is a global misunderstanding regarding the use of the
nand-ecc-* properties. These are not the default. The default is the OS
choice and depends on the NAND capabilities. The OS will always try to
match the closest ECC settings offered by the engine, based on the NAND
chip requirements which are discoverable. If you want to maximize your
strength, it is also possible to tell the OS with a dedicated (generic)
property. And only if you want something different, you may use these
properties, but they should be the exception rather than the rule.

Overriding this with a strap is a bad hardware design on commercial
products IMO. I am totally fine with the idea of a strap to choose
the ECC configuration for development boards/evaluation kits, but once
you've decided which setting you want you cannot change it for the
lifetime of your project (or with a lot of difficulties) so I don't see
the point of such a strap. So really, I don't like the idea of defining
by default a variable which asks for an override of the defaults, even
though many of your customers might want to use that.

So, anything that is design dependent (the chip CS, ECC
configuration, etc) should go into the board DTS, and what is SoC
related hardware (like the definition of the NAND controller) should
stay in the DTSI, as properly clarified by Florian.

Thanks,
Miquèl

Re: [PATCH v3 02/10] ARM: dts: broadcom: bcmbca: Add NAND controller node

[William Zhang]

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

Hi Miquel,

On 1/25/24 01:20, Miquel Raynal wrote:
> Hi William,
> 
>>>>>> +        nand_controller: nand-controller@1800 {
>>>>>> +            #address-cells = <1>;
>>>>>> +            #size-cells = <0>;
>>>>>> +            compatible = "brcm,nand-bcm63138", >>>> "brcm,brcmnand-v7.1", "brcm,brcmnand";
>>>>>> +            reg = <0x1800 0x600>, <0x2000 0x10>;
>>>>>> +            reg-names = "nand", "nand-int-base";
>>>>>> +            brcm,nand-use-wp = <0>;
>>>>>> +            status = "disabled";
>>>>>> +
>>>>>> +            nandcs: nand@0 {
>>>>>> +                compatible = "brcm,nandcs";
>>>>>> +                reg = <0>;
>>>>>> +                nand-on-flash-bbt;
>>>>>> +                brcm,nand-ecc-use-strap;
>>>>>
>>>>> Describing the NAND chip in a SoC DTSI does not look relevant to me.
>>>>> Even more if you add something like this nand-ecc-use-strap setting
>>>>> which is very board dependent.
>>>>>   
>>>> I am not sure if I understand you comments correctly but are you >> suggesting to put this whole nand controller node into each board dts? >> We have other ip block nodes like SPI, uart in this same soc dtsi file >> too.  For all the bcmbca soc dtsi I am updating here(and its board >> design), we always use the strap to for ecc setting.  So I thought it >> should be okay to put brcm,nand-ecc-use-strap in the default dtsi >> file. For any board that uses the raw nand nand-ecc property, the >> board dts can do so and override the brcm,nand-ecc-use-strap setting.
>>>
>>> I read Miquel's comment as meaning that the nandcs aka the NAND > chip/flash part description should be in the board .dts file, while the > controller itself can remain in the .dtsi file with its status = > "disabled" property.
>>>
>>> Are there customer boards, that is non reference boards that might chose > a different chip select number and/or not use the strap settings?
>> In BCMBCA SoC, there is only one cs and customer design also have to use strap for the bootrom to boot up properly.  They can override it with dts in linux but I don't think any customer would do that.
>>
>> Maybe the nand-on-flash-bbt could be possible item that customer may have to set it differently if they don't follow our reference software design.
>>
>> I will move the nand-on-flash-bbt to the board dts but I would like to keep the other default nandcs settings in SoC.dsti if that is not too out of the conventional rule and Miquel is okay with it.
> 
> I think there is a global misunderstanding regarding the use of the
> nand-ecc-* properties. These are not the default. The default is the OS
> choice and depends on the NAND capabilities. The OS will always try to
> match the closest ECC settings offered by the engine, based on the NAND
> chip requirements which are discoverable. If you want to maximize your
> strength, it is also possible to tell the OS with a dedicated (generic)
This is the nand-ecc-* property, right?

> property. And only if you want something different, you may use these
> properties, but they should be the exception rather than the rule.
> 
> Overriding this with a strap is a bad hardware design on commercial
> products IMO. I am totally fine with the idea of a strap to choose
> the ECC configuration for development boards/evaluation kits, but once
> you've decided which setting you want you cannot change it for the
> lifetime of your project (or with a lot of difficulties) so I don't see
> the point of such a strap. So really, I don't like the idea of defining
> by default a variable which asks for an override of the defaults, even
> though many of your customers might want to use that.
Correct, no change to strap is possible on real product because they are 
always through soldered down resister and no dip switch/jumper for ecc 
strap. But as the SoC requires, it is part of bootstrap each product has 
to set and that's how bootloader get the ecc setting as it does not have 
the access to the dts and the capability to auto select the ecc setting.

Most of the time, customer will set strap to match the OS auto selected 
ecc setting but there are times customer want more strength so yes they 
can use nand-ecc-* to override but it has to match the strap setting. 
Then I think it make sense and much easier for customer to just use 
strap to override and reduce the any manual setting error in dts. It 
will cause many trouble down the road if the edit does not match strap 
setting.  Not saying this is for everyone but definitively a good 
feature for our product and it reduces ecc setting error in case of 
overriding OS default selection.

> 
> So, anything that is design dependent (the chip CS, ECC
> configuration, etc) should go into the board DTS, and what is SoC
> related hardware (like the definition of the NAND controller) should
> stay in the DTSI, as properly clarified by Florian.
> 
Okay I will move nand-on-flash-bbt and brcm,nand-ecc-use-strap from soc 
dtsi to board dts but leave the default nandcs node with compatible and 
reg = 0 in the dtsi as they are not design dependent and board dts can 
conveniently reference the nandcs node.

> Thanks,
> Miquèl

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4212 bytes --]

Re: [PATCH v3 09/10] mtd: rawnand: brcmnand: update log level messages

[David Regan]

On Wed, Jan 24, 2024 at 9:37 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> Hi David,
>
> dregan@broadcom.com wrote on Tue, 23 Jan 2024 19:04:57 -0800:
>
> > Update log level messages so that more critical messages
> > can be seen.
> >
> > Signed-off-by: David Regan <dregan@broadcom.com>
> > Reviewed-by: William Zhang <william.zhang@broadcom.com>
> > ---
> > Changes in v3: None
> > ---
> > Changes in v2:
> > - Added to patch series
> > ---
> >  drivers/mtd/nand/raw/brcmnand/brcmnand.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> > index 6b5d76eff0ec..a4e311b6798c 100644
> > --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> > +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> > @@ -1143,7 +1143,7 @@ static int bcmnand_ctrl_poll_status(struct brcmnand_host *host,
> >       if ((val & mask) == expected_val)
> >               return 0;
> >
> > -     dev_warn(ctrl->dev, "timeout on status poll (expected %x got %x)\n",
> > +     dev_err(ctrl->dev, "timeout on status poll (expected %x got %x)\n",
>
> I don't see the point but if you want.
>
> >                expected_val, val & mask);
> >
> >       return -ETIMEDOUT;
> > @@ -2196,7 +2196,7 @@ static int brcmnand_read(struct mtd_info *mtd, struct nand_chip *chip,
> >                               return err;
> >               }
> >
> > -             dev_dbg(ctrl->dev, "uncorrectable error at 0x%llx\n",
> > +             dev_err(ctrl->dev, "uncorrectable error at 0x%llx\n",
>
> Upper layer will complain, you can keep this at the debug level.

I've discovered that the upper layer will complain, but typically
with information that is maybe not useful for debugging.

>
> >                       (unsigned long long)err_addr);
> >               mtd->ecc_stats.failed++;
> >               /* NAND layer expects zero on ECC errors */
> > @@ -2211,7 +2211,7 @@ static int brcmnand_read(struct mtd_info *mtd, struct nand_chip *chip,
> >                       err = brcmnand_read_by_pio(mtd, chip, addr, trans, buf,
> >                                                  oob, &err_addr);
> >
> > -             dev_dbg(ctrl->dev, "corrected error at 0x%llx\n",
> > +             dev_info(ctrl->dev, "corrected error at 0x%llx\n",
>
> Definitely not! Way too verbose. Please keep this one as it is.

ok

>
> >                       (unsigned long long)err_addr);
> >               mtd->ecc_stats.corrected += corrected;
> >               /* Always exceed the software-imposed threshold */
>
>
> Thanks,
> Miquèl

Thanks!

-Dave

Re: [PATCH v3 10/10] mtd: rawnand: brcmnand: allow for on-die ecc

[David Regan]

Hi Miquèl,

On Wed, Jan 24, 2024 at 9:40 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> Hi David,
>
> dregan@broadcom.com wrote on Tue, 23 Jan 2024 19:04:58 -0800:
>
> > Allow settings for on-die ecc such that if on-die ECC is selected
> > don't error out but require ECC strap setting of zero
> >
> > Signed-off-by: David Regan <dregan@broadcom.com>
> > Reviewed-by: William Zhang <william.zhang@broadcom.com>
> > ---
> > Changes in v3: None
> > ---
> > Changes in v2:
> > - Added to patch series
> > ---
> >  drivers/mtd/nand/raw/brcmnand/brcmnand.c | 14 ++++++++++----
> >  1 file changed, 10 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> > index a4e311b6798c..42526f3250c9 100644
> > --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> > +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> > @@ -2727,9 +2727,11 @@ static int brcmnand_setup_dev(struct brcmnand_host *host)
> >       cfg->blk_adr_bytes = get_blk_adr_bytes(mtd->size, mtd->writesize);
> >
> >       if (chip->ecc.engine_type != NAND_ECC_ENGINE_TYPE_ON_HOST) {
> > -             dev_err(ctrl->dev, "only HW ECC supported; selected: %d\n",
> > -                     chip->ecc.engine_type);
> > -             return -EINVAL;
> > +             if (chip->ecc.strength) {
> > +                     dev_err(ctrl->dev, "ERROR!!! HW ECC must be set to zero for non-hardware ECC; selected: %d\n",
> > +                             chip->ecc.strength);
>
> Can you use a more formal string? Also clarify it because I don't
> really understand what it leads to.

How about:

dev_err(ctrl->dev, "HW ECC set to %d, must be zero for on-die ECC\n",

>
> > +                     return -EINVAL;
> > +             }
> >       }
> >
> >       if (chip->ecc.algo == NAND_ECC_ALGO_UNKNOWN) {
> > @@ -2797,7 +2799,11 @@ static int brcmnand_setup_dev(struct brcmnand_host *host)
> >       if (ret)
> >               return ret;
> >
> > -     brcmnand_set_ecc_enabled(host, 1);
> > +     if (chip->ecc.engine_type == NAND_ECC_ENGINE_TYPE_ON_DIE) {
> > +             dev_dbg(ctrl->dev, "Disable HW ECC for on-die ECC\n");
>
> Not needed.

Will remove.

>
> > +             brcmnand_set_ecc_enabled(host, 0);
> > +     } else
> > +             brcmnand_set_ecc_enabled(host, 1);
>
> Style is wrong, but otherwise I think ECC should be kept disabled while
> not in active use, so I am a bit surprised by this line.

This is a double check to turn on/off our hardware ECC.

>
> >
> >       brcmnand_print_cfg(host, msg, cfg);
> >       dev_info(ctrl->dev, "detected %s\n", msg);
>
>
> Thanks,
> Miquèl

Thanks!

-Dave

Re: [PATCH v3 01/10] dt-bindings: mtd: brcmnand: Updates for bcmbca SoCs

[Conor Dooley]

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


On Wed, Jan 24, 2024 at 07:01:48PM -0800, William Zhang wrote:
> On 1/24/24 09:24, Conor Dooley wrote:
> > On Tue, Jan 23, 2024 at 07:04:49PM -0800, David Regan wrote:
> > > From: William Zhang <william.zhang@broadcom.com>
> > > 
> > > Update the descriptions to reflect different families of broadband SoC and
> > > use the general name bcmbca for ARM based SoC.
> > > 
> > > Add brcm,nand-use-wp property to have an option for disabling this
> > > feature on broadband board design that does not use write protection.
> > > 
> > > Add brcm,nand-ecc-use-strap to get ecc setting from board boot strap for
> > > broadband board designs because they do not specify ecc setting in dts
> > > but rather using the strap setting.
> > > 
> > > Remove the requirement of interrupts property to reflect the driver
> > > code. Also add myself to the list of maintainers.
> > > 
> > > Signed-off-by: William Zhang <william.zhang@broadcom.com>
> > > Reviewed-by: David Regan <dregan@broadcom.com>
> > > ---
> > > Changes in v3:
> > > - Update brcm,nand-use-wp description
> > > - Revert the description change to BCM63168 SoC-specific NAND controller
> > > ---
> > > Changes in v2:
> > > - Revert the new compatible string nand-bcmbca
> > > - Drop the BCM63168 compatible fix to avoid any potential ABI
> > > incompatibility issue
> > > - Simplify the explanation for brcm,nand-use-wp
> > > - Keep the interrupt name requirement when interrupt number is specified
> > > ---
> > >   .../bindings/mtd/brcm,brcmnand.yaml           | 37 ++++++++++++++++---
> > >   1 file changed, 32 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/mtd/brcm,brcmnand.yaml b/Documentation/devicetree/bindings/mtd/brcm,brcmnand.yaml
> > > index f57e96374e67..752c6ee98a7d 100644
> > > --- a/Documentation/devicetree/bindings/mtd/brcm,brcmnand.yaml
> > > +++ b/Documentation/devicetree/bindings/mtd/brcm,brcmnand.yaml
> > > @@ -9,6 +9,7 @@ title: Broadcom STB NAND Controller
> > >   maintainers:
> > >     - Brian Norris <computersforpeace@gmail.com>
> > >     - Kamal Dasu <kdasu.kdev@gmail.com>
> > > +  - William Zhang <william.zhang@broadcom.com>
> > >   description: |
> > >     The Broadcom Set-Top Box NAND controller supports low-level access to raw NAND
> > > @@ -18,9 +19,10 @@ description: |
> > >     supports basic PROGRAM and READ functions, among other features.
> > >     This controller was originally designed for STB SoCs (BCM7xxx) but is now
> > > -  available on a variety of Broadcom SoCs, including some BCM3xxx, BCM63xx, and
> > > -  iProc/Cygnus. Its history includes several similar (but not fully register
> > > -  compatible) versions.
> > > +  available on a variety of Broadcom SoCs, including some BCM3xxx, MIPS based
> > > +  Broadband SoC (BCM63xx), ARM based Broadband SoC (BCMBCA) and iProc/Cygnus.
> > > +  Its history includes several similar (but not fully register compatible)
> > > +  versions.
> > >     -- Additional SoC-specific NAND controller properties --
> > > @@ -53,7 +55,7 @@ properties:
> > >                 - brcm,brcmnand-v7.2
> > >                 - brcm,brcmnand-v7.3
> > >             - const: brcm,brcmnand
> > > -      - description: BCM63138 SoC-specific NAND controller
> > > +      - description: BCMBCA SoC-specific NAND controller
> > >           items:
> > >             - const: brcm,nand-bcm63138
> > >             - enum:
> > > @@ -111,6 +113,20 @@ properties:
> > >         earlier versions of this core that include WP
> > >       type: boolean
> > > +  brcm,nand-use-wp:
> > > +    description:
> > > +      Use this property to indicate if board design uses
> > > +      controller's write protection feature and connects its
> > > +      NAND_WPb pin to nand chip's WP_L pin.
> > 
> > > By default the driver
> > > +      uses a module parameter with default value set to enable to
> > > +      control this feature for all boards. Use this dts property to
> > > +      override the default behavior and enable/disable this feature
> > > +      through board dts on a per board basis.
> > 
> > None of this information about module parameters is relevant in the
> > binding, as it should be independent of the implementation of one
> > particular operating system.
> > 
> Agree this is OS related stuff. I was trying to make it more clean by
> explaining how it is implemented in linux. And if we were to implement the
> driver for another OS, there will be at least a default value for wp_on. I
> will remove any mention of module parameter from this doc.
> 
> > If the module parameter is not provided, what does the driver do?
> > If "wp_on" is the module parameter, then the default is to enable the
> > write protection feature. If that's correct, it seems to me that the
> > property should be called something like "brcm,no-wp". This property
> > would be a boolean that indicates that the NAND_WPb and WP_L pins are
> > not connected & nothing more. Clearly if the module param tries to
> > enable WP and the DT property indicates that it is not supported you
> > can decline to enable the feature, but that is up to the drivers in
> > the OS to decide.
> > 
> If I were to implement this from day one, I certainly will choose the same
> approach as you suggested here.  But there is existing brcm,nand-has-wp
> property for other purpose and now if we have brcm,no-wp, it will be more
> confusing with that property IMHO. I prefer to keep use the "has" for
> feature availability and "use" for feature being used or not.

"brcm,wp-not-connected" then, since I want it to become a boolean and it
would better communicate what the problem is than "brcm,dont-use-wp"
would.

> And the reason I keep it as int is because there could be a potential value
> of 2 for another mode that the current driver could set the wp_on to.

Can you explain, in driver independent terms, what this other mode has
to do with how the WP is connected?
Either you've got the standard scenario where apparently "NAND_WPb" and
"WP_L" are connected and another situation where they are not.
Is there another pin configuration in addition to that, that this 3rd
mode represents?

> I
> don't see it is being used in BCMBCA product but I am not sure about other
> SoC family. So I don't want to completely close the door here just in case.

If you ever need it, you could have a "brcm,wp-in-wacky-configuration"
property that indicates that the hardware is configured in that way
(providing that this hardware configuration is not just "NAND_WPb" and
"WP_L" pins connected. The property can very easily be made mutually
exclusive with "brcm,wp-not-connected" iff it ever comes to be.

> > > +      Set to 0 if WP pins are not connected and feature is not
> > > +      used. Set to 1 if WP pins are connected and feature is used.
> > 
> > As it stands, this property is firmly in the "software policy" side
> > of things, as it is being used as an override for a module parameter,
> > particularly given you have properties for each direction. Whether or
> > not the feature is to be used by the operating system is not relevant to
> > the binding.
> > 
> I don't understand why this is a software policy.  This is the board design
> choice although it does override the driver default value. But it is still
> board or device setting and describe the hardware. OS has to follow this
> hardware configuration and set up accordingly.

The software policy side of things is that you are instructing the
software what to do with "use" nature of it and the force enable &
disable options for the property. A boolean property that tells software
that the wp is not connected is all you need, as far as I can tell.
That should be sufficient information for the operating system to know
whether or not it can use the wp.

> > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > +    enum: [0, 1]
> > > +
> > >   patternProperties:
> > >     "^nand@[a-f0-9]$":
> > >       type: object
> > > @@ -137,6 +153,16 @@ patternProperties:
> > >             layout.
> > >           $ref: /schemas/types.yaml#/definitions/uint32
> > > +      brcm,nand-ecc-use-strap:
> > > +        description:
> > > +          This flag is used by the driver to get the ecc strength and
> > > +          spare area size from the SoC NAND boot strap setting. This
> > > +          is commonly used by the BCMBCA SoC board design.
> > 
> > Quoting from v1, as I didn't get to it in time:
> > 
> > | > This property I'm not all that sure about either. If the specific
> > | > properties that you mention here are not set in the DT what happens at
> > | > the moment?
> > | >
> > | In that case, the ecc strength and oob size come from ONFI or nand id
> > | decoding.  But that is usually the minimum requirement and user can
> > | choose to use stronger ecc as long as there is enough oob spare area in
> > | the nand chip.
> > |
> > | > I suppose what I am getting at is why are the bootstrap settings not
> > | > always used in the absence of specific values provided in the DT?
> > | >
> > | This is because the STB, iProc and other chip and their board design may
> > | not have or use the strap setting. But for BCMBCA SoC and board
> > | reference design, we always use the strap setting.
> > 
> > My main question here I suppose is why would you need this property at
> > all? If the specific properties are provided (nand-ecc-strength etc)
> > then use them, and if they are not use the strap settings?If
> > nand-ecc-strength does not exist, the current driver implementation
> use the auto detected ecc strength from the NAND chip ONFI parameter which
> is okay. But for BCABCM SoC and our reference board design, we don't use
> nand-ecc-strength and don't want to use the auto detected value(as they are
> typically minimum requirement) but rather use strap setting so I need a
> third choice.

That seems reasonable. Please limit the property though to the SoCs
where it is relevant if you had not done so already.
Thanks for the explanation.

> > If there's no properties and no strap settings, the this property does
> > not help. If there's properties and strap settings, but properties are
> > wrong, then you just have some devicetrees that incorrectly describe the
> > hardware and need to be fixed.
> > 
> True but manually setting nand-ecc-strength can be error prone like
> copy/paste error. The hardware strap setting does not involve such edit in
> the dts file so no error can happen.

Copy paste errors are not really an argument here, you could use that as
an excuse for anything DT property in the world.. You could copy paste
the wrong thing into the strap, and idk about you, but on the systems I
have it is far easier to change the DT passed to the kernel than what I
assume is an early bootloader stage? (I tried googling for "broadcom
strap" but there was nothing relevant)

> > > If ecc
> > > +          strength and spare area size are set by nand-ecc-strength
> > > +          and brcm,nand-oob-sector-size in the dts, these settings
> > > +          have precedence and override this flag.
> > 
> > Again, IMO, this is not for the binding to decide. That is a decision
> > for the operating system to make.
> > 
> Again this is just for historic reason and BCMBCA as late player does not
> want to break any existing dts setting.  So I thought it is useful to
> explain here.  I can remove this text if you don't think it should be in the
> binding document.

I don't, at least not in that form. I think it is reasonable to explain
why these values are better though.

Thanks,
Conor.

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

Re: [PATCH v3 01/10] dt-bindings: mtd: brcmnand: Updates for bcmbca SoCs

[William Zhang]

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



On 1/25/24 11:51, Conor Dooley wrote:
> 
> On Wed, Jan 24, 2024 at 07:01:48PM -0800, William Zhang wrote:
>> On 1/24/24 09:24, Conor Dooley wrote:
>>> On Tue, Jan 23, 2024 at 07:04:49PM -0800, David Regan wrote:
>>>> From: William Zhang <william.zhang@broadcom.com>
>>>>
>>>> Update the descriptions to reflect different families of broadband SoC and
>>>> use the general name bcmbca for ARM based SoC.
>>>>
>>>> Add brcm,nand-use-wp property to have an option for disabling this
>>>> feature on broadband board design that does not use write protection.
>>>>
>>>> Add brcm,nand-ecc-use-strap to get ecc setting from board boot strap for
>>>> broadband board designs because they do not specify ecc setting in dts
>>>> but rather using the strap setting.
>>>>
>>>> Remove the requirement of interrupts property to reflect the driver
>>>> code. Also add myself to the list of maintainers.
>>>>
>>>> Signed-off-by: William Zhang <william.zhang@broadcom.com>
>>>> Reviewed-by: David Regan <dregan@broadcom.com>
>>>> ---
>>>> Changes in v3:
>>>> - Update brcm,nand-use-wp description
>>>> - Revert the description change to BCM63168 SoC-specific NAND controller
>>>> ---
>>>> Changes in v2:
>>>> - Revert the new compatible string nand-bcmbca
>>>> - Drop the BCM63168 compatible fix to avoid any potential ABI
>>>> incompatibility issue
>>>> - Simplify the explanation for brcm,nand-use-wp
>>>> - Keep the interrupt name requirement when interrupt number is specified
>>>> ---
>>>>    .../bindings/mtd/brcm,brcmnand.yaml           | 37 ++++++++++++++++---
>>>>    1 file changed, 32 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/mtd/brcm,brcmnand.yaml b/Documentation/devicetree/bindings/mtd/brcm,brcmnand.yaml
>>>> index f57e96374e67..752c6ee98a7d 100644
>>>> --- a/Documentation/devicetree/bindings/mtd/brcm,brcmnand.yaml
>>>> +++ b/Documentation/devicetree/bindings/mtd/brcm,brcmnand.yaml
>>>> @@ -9,6 +9,7 @@ title: Broadcom STB NAND Controller
>>>>    maintainers:
>>>>      - Brian Norris <computersforpeace@gmail.com>
>>>>      - Kamal Dasu <kdasu.kdev@gmail.com>
>>>> +  - William Zhang <william.zhang@broadcom.com>
>>>>    description: |
>>>>      The Broadcom Set-Top Box NAND controller supports low-level access to raw NAND
>>>> @@ -18,9 +19,10 @@ description: |
>>>>      supports basic PROGRAM and READ functions, among other features.
>>>>      This controller was originally designed for STB SoCs (BCM7xxx) but is now
>>>> -  available on a variety of Broadcom SoCs, including some BCM3xxx, BCM63xx, and
>>>> -  iProc/Cygnus. Its history includes several similar (but not fully register
>>>> -  compatible) versions.
>>>> +  available on a variety of Broadcom SoCs, including some BCM3xxx, MIPS based
>>>> +  Broadband SoC (BCM63xx), ARM based Broadband SoC (BCMBCA) and iProc/Cygnus.
>>>> +  Its history includes several similar (but not fully register compatible)
>>>> +  versions.
>>>>      -- Additional SoC-specific NAND controller properties --
>>>> @@ -53,7 +55,7 @@ properties:
>>>>                  - brcm,brcmnand-v7.2
>>>>                  - brcm,brcmnand-v7.3
>>>>              - const: brcm,brcmnand
>>>> -      - description: BCM63138 SoC-specific NAND controller
>>>> +      - description: BCMBCA SoC-specific NAND controller
>>>>            items:
>>>>              - const: brcm,nand-bcm63138
>>>>              - enum:
>>>> @@ -111,6 +113,20 @@ properties:
>>>>          earlier versions of this core that include WP
>>>>        type: boolean
>>>> +  brcm,nand-use-wp:
>>>> +    description:
>>>> +      Use this property to indicate if board design uses
>>>> +      controller's write protection feature and connects its
>>>> +      NAND_WPb pin to nand chip's WP_L pin.
>>>
>>>> By default the driver
>>>> +      uses a module parameter with default value set to enable to
>>>> +      control this feature for all boards. Use this dts property to
>>>> +      override the default behavior and enable/disable this feature
>>>> +      through board dts on a per board basis.
>>>
>>> None of this information about module parameters is relevant in the
>>> binding, as it should be independent of the implementation of one
>>> particular operating system.
>>>
>> Agree this is OS related stuff. I was trying to make it more clean by
>> explaining how it is implemented in linux. And if we were to implement the
>> driver for another OS, there will be at least a default value for wp_on. I
>> will remove any mention of module parameter from this doc.
>>
>>> If the module parameter is not provided, what does the driver do?
>>> If "wp_on" is the module parameter, then the default is to enable the
>>> write protection feature. If that's correct, it seems to me that the
>>> property should be called something like "brcm,no-wp". This property
>>> would be a boolean that indicates that the NAND_WPb and WP_L pins are
>>> not connected & nothing more. Clearly if the module param tries to
>>> enable WP and the DT property indicates that it is not supported you
>>> can decline to enable the feature, but that is up to the drivers in
>>> the OS to decide.
>>>
>> If I were to implement this from day one, I certainly will choose the same
>> approach as you suggested here.  But there is existing brcm,nand-has-wp
>> property for other purpose and now if we have brcm,no-wp, it will be more
>> confusing with that property IMHO. I prefer to keep use the "has" for
>> feature availability and "use" for feature being used or not.
> 
> "brcm,wp-not-connected" then, since I want it to become a boolean and it
> would better communicate what the problem is than "brcm,dont-use-wp"
> would.
> 
>> And the reason I keep it as int is because there could be a potential value
>> of 2 for another mode that the current driver could set the wp_on to.
> 
> Can you explain, in driver independent terms, what this other mode has
> to do with how the WP is connected?
> Either you've got the standard scenario where apparently "NAND_WPb" and
> "WP_L" are connected and another situation where they are not.
> Is there another pin configuration in addition to that, that this 3rd
> mode represents?
> 
The 3rd mode is WP pin connected but wp feature is disabled in the 
controller.

>> I
>> don't see it is being used in BCMBCA product but I am not sure about other
>> SoC family. So I don't want to completely close the door here just in case.
> 
> If you ever need it, you could have a "brcm,wp-in-wacky-configuration"
> property that indicates that the hardware is configured in that way
> (providing that this hardware configuration is not just "NAND_WPb" and
> "WP_L" pins connected. The property can very easily be made mutually
> exclusive with "brcm,wp-not-connected" iff it ever comes to be.
Yes we could have a new property but if we can have a single int 
property to cover all, IMHO it is better to just have one property as 
these three modes are related. I would really like to have it as an int 
property to keep things simple.

But if you think this is absolutely against the dts rule,  I will switch 
to the "brcm,wp-not-connected" boolean property as you suggested.

> 
>>>> +      Set to 0 if WP pins are not connected and feature is not
>>>> +      used. Set to 1 if WP pins are connected and feature is used.
>>>
>>> As it stands, this property is firmly in the "software policy" side
>>> of things, as it is being used as an override for a module parameter,
>>> particularly given you have properties for each direction. Whether or
>>> not the feature is to be used by the operating system is not relevant to
>>> the binding.
>>>
>> I don't understand why this is a software policy.  This is the board design
>> choice although it does override the driver default value. But it is still
>> board or device setting and describe the hardware. OS has to follow this
>> hardware configuration and set up accordingly.
> 
> The software policy side of things is that you are instructing the
> software what to do with "use" nature of it and the force enable &
> disable options for the property. A boolean property that tells software
> that the wp is not connected is all you need, as far as I can tell.
> That should be sufficient information for the operating system to know
> whether or not it can use the wp.
>

>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>> +    enum: [0, 1]
>>>> +
>>>>    patternProperties:
>>>>      "^nand@[a-f0-9]$":
>>>>        type: object
>>>> @@ -137,6 +153,16 @@ patternProperties:
>>>>              layout.
>>>>            $ref: /schemas/types.yaml#/definitions/uint32
>>>> +      brcm,nand-ecc-use-strap:
>>>> +        description:
>>>> +          This flag is used by the driver to get the ecc strength and
>>>> +          spare area size from the SoC NAND boot strap setting. This
>>>> +          is commonly used by the BCMBCA SoC board design.
>>>
>>> Quoting from v1, as I didn't get to it in time:
>>>
>>> | > This property I'm not all that sure about either. If the specific
>>> | > properties that you mention here are not set in the DT what happens at
>>> | > the moment?
>>> | >
>>> | In that case, the ecc strength and oob size come from ONFI or nand id
>>> | decoding.  But that is usually the minimum requirement and user can
>>> | choose to use stronger ecc as long as there is enough oob spare area in
>>> | the nand chip.
>>> |
>>> | > I suppose what I am getting at is why are the bootstrap settings not
>>> | > always used in the absence of specific values provided in the DT?
>>> | >
>>> | This is because the STB, iProc and other chip and their board design may
>>> | not have or use the strap setting. But for BCMBCA SoC and board
>>> | reference design, we always use the strap setting.
>>>
>>> My main question here I suppose is why would you need this property at
>>> all? If the specific properties are provided (nand-ecc-strength etc)
>>> then use them, and if they are not use the strap settings?If
>>> nand-ecc-strength does not exist, the current driver implementation
>> use the auto detected ecc strength from the NAND chip ONFI parameter which
>> is okay. But for BCABCM SoC and our reference board design, we don't use
>> nand-ecc-strength and don't want to use the auto detected value(as they are
>> typically minimum requirement) but rather use strap setting so I need a
>> third choice.
> 
> That seems reasonable. Please limit the property though to the SoCs
> where it is relevant if you had not done so already.
Yes it is only for BCMBCA SoC that I am updating in this patch series.

> Thanks for the explanation.
> 
>>> If there's no properties and no strap settings, the this property does
>>> not help. If there's properties and strap settings, but properties are
>>> wrong, then you just have some devicetrees that incorrectly describe the
>>> hardware and need to be fixed.
>>>
>> True but manually setting nand-ecc-strength can be error prone like
>> copy/paste error. The hardware strap setting does not involve such edit in
>> the dts file so no error can happen.
> 
> Copy paste errors are not really an argument here, you could use that as
> an excuse for anything DT property in the world.. You could copy paste
> the wrong thing into the strap, and idk about you, but on the systems I
> have it is far easier to change the DT passed to the kernel than what I
> assume is an early bootloader stage? (I tried googling for "broadcom
> strap" but there was nothing relevant)
> 
Agree, the strap could be set incorrectly in the hardware but then the 
board likely won't be able to boot to linux for linux developer to work 
on. Changing dts is easy but obtaining the ecc setting from hardware 
strap automatically is no-brainer as it require no change in dts or any 
manual setting, assuming hardware is correct.

>>>> If ecc
>>>> +          strength and spare area size are set by nand-ecc-strength
>>>> +          and brcm,nand-oob-sector-size in the dts, these settings
>>>> +          have precedence and override this flag.
>>>
>>> Again, IMO, this is not for the binding to decide. That is a decision
>>> for the operating system to make.
>>>
>> Again this is just for historic reason and BCMBCA as late player does not
>> want to break any existing dts setting.  So I thought it is useful to
>> explain here.  I can remove this text if you don't think it should be in the
>> binding document.
> 
> I don't, at least not in that form. I think it is reasonable to explain
> why these values are better though.
I understand the decision is for OS/driver to make. If we were to write 
another driver for other OS, the same precedence will apply too so the 
binding works the same way across all the OS. So I am not sure what 
better explanation or form I can put up here. I am open to any 
suggestion or I just delete it.

> 
> Thanks,
> Conor.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4212 bytes --]

Re: [PATCH v3 10/10] mtd: rawnand: brcmnand: allow for on-die ecc

[Miquel Raynal]

Hi David,

dregan@broadcom.com wrote on Thu, 25 Jan 2024 11:47:46 -0800:

> Hi Miquèl,
> 
> On Wed, Jan 24, 2024 at 9:40 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >
> > Hi David,
> >
> > dregan@broadcom.com wrote on Tue, 23 Jan 2024 19:04:58 -0800:
> >  
> > > Allow settings for on-die ecc such that if on-die ECC is selected
> > > don't error out but require ECC strap setting of zero
> > >
> > > Signed-off-by: David Regan <dregan@broadcom.com>
> > > Reviewed-by: William Zhang <william.zhang@broadcom.com>
> > > ---
> > > Changes in v3: None
> > > ---
> > > Changes in v2:
> > > - Added to patch series
> > > ---
> > >  drivers/mtd/nand/raw/brcmnand/brcmnand.c | 14 ++++++++++----
> > >  1 file changed, 10 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> > > index a4e311b6798c..42526f3250c9 100644
> > > --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> > > +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> > > @@ -2727,9 +2727,11 @@ static int brcmnand_setup_dev(struct brcmnand_host *host)
> > >       cfg->blk_adr_bytes = get_blk_adr_bytes(mtd->size, mtd->writesize);
> > >
> > >       if (chip->ecc.engine_type != NAND_ECC_ENGINE_TYPE_ON_HOST) {
> > > -             dev_err(ctrl->dev, "only HW ECC supported; selected: %d\n",
> > > -                     chip->ecc.engine_type);
> > > -             return -EINVAL;
> > > +             if (chip->ecc.strength) {
> > > +                     dev_err(ctrl->dev, "ERROR!!! HW ECC must be set to zero for non-hardware ECC; selected: %d\n",
> > > +                             chip->ecc.strength);  
> >
> > Can you use a more formal string? Also clarify it because I don't
> > really understand what it leads to.  
> 
> How about:
> 
> dev_err(ctrl->dev, "HW ECC set to %d, must be zero for on-die ECC\n",

Actually I am wondering how legitimate this is. Just don't enable the
on host ECC engine if it's not in use. No need to check the core's
choice.

> 
> >  
> > > +                     return -EINVAL;
> > > +             }
> > >       }
> > >
> > >       if (chip->ecc.algo == NAND_ECC_ALGO_UNKNOWN) {
> > > @@ -2797,7 +2799,11 @@ static int brcmnand_setup_dev(struct brcmnand_host *host)
> > >       if (ret)
> > >               return ret;
> > >
> > > -     brcmnand_set_ecc_enabled(host, 1);
> > > +     if (chip->ecc.engine_type == NAND_ECC_ENGINE_TYPE_ON_DIE) {
> > > +             dev_dbg(ctrl->dev, "Disable HW ECC for on-die ECC\n");  
> >
> > Not needed.  
> 
> Will remove.
> 
> >  
> > > +             brcmnand_set_ecc_enabled(host, 0);
> > > +     } else
> > > +             brcmnand_set_ecc_enabled(host, 1);  
> >
> > Style is wrong, but otherwise I think ECC should be kept disabled while
> > not in active use, so I am a bit surprised by this line.  
> 
> This is a double check to turn on/off our hardware ECC.

I expect the engine to be always disabled. Enable it only when you
need (may require an additional patch before this one).

Thanks,
Miquèl

Re: [PATCH v3 01/10] dt-bindings: mtd: brcmnand: Updates for bcmbca SoCs

[Conor Dooley]

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

On Thu, Jan 25, 2024 at 05:55:29PM -0800, William Zhang wrote:
> On 1/25/24 11:51, Conor Dooley wrote:
> > On Wed, Jan 24, 2024 at 07:01:48PM -0800, William Zhang wrote:
> > > On 1/24/24 09:24, Conor Dooley wrote:
> > > > On Tue, Jan 23, 2024 at 07:04:49PM -0800, David Regan wrote:
> > > > > From: William Zhang <william.zhang@broadcom.com>

> > > And the reason I keep it as int is because there could be a potential value
> > > of 2 for another mode that the current driver could set the wp_on to.
> > 
> > Can you explain, in driver independent terms, what this other mode has
> > to do with how the WP is connected?
> > Either you've got the standard scenario where apparently "NAND_WPb" and
> > "WP_L" are connected and another situation where they are not.
> > Is there another pin configuration in addition to that, that this 3rd
> > mode represents?
> > 
> The 3rd mode is WP pin connected but wp feature is disabled in the
> controller.

What does "disabled" mean? The controller itself is not capable of using
the WP pin because of hardware modifications? Or there's a bit in a
register that disables it and that bit has been set by software?

If it is a hardware difference for that controller, why does it not have
a dedicated compatible? If it's a software decision, then it shouldn't
be in the DT in the first place ;)

We've gone back here a bunch trying to figure stuff out, and you give me
a vague one sentence answer. Please.

> > > I
> > > don't see it is being used in BCMBCA product but I am not sure about other
> > > SoC family. So I don't want to completely close the door here just in case.
> > 
> > If you ever need it, you could have a "brcm,wp-in-wacky-configuration"
> > property that indicates that the hardware is configured in that way
> > (providing that this hardware configuration is not just "NAND_WPb" and
> > "WP_L" pins connected. The property can very easily be made mutually
> > exclusive with "brcm,wp-not-connected" iff it ever comes to be.
> Yes we could have a new property but if we can have a single int property to
> cover all, IMHO it is better to just have one property as these three modes
> are related. I would really like to have it as an int property to keep
> things simple.

It is "better" because it is easier for you perhaps, but ripe for abuse.

> But if you think this is absolutely against the dts rule,  I will switch to
> the "brcm,wp-not-connected" boolean property as you suggested.

I'll answer this when you describe the mode better, right now I cannot
really comment, but I have not yet seen a justification for the non-flag
version of the property. Even if there's a justification for documenting
that other mode in the DT, I'm likely to still think boolean properties
are a better fit.

> > > > > If ecc
> > > > > +          strength and spare area size are set by nand-ecc-strength
> > > > > +          and brcm,nand-oob-sector-size in the dts, these settings
> > > > > +          have precedence and override this flag.
> > > > 
> > > > Again, IMO, this is not for the binding to decide. That is a decision
> > > > for the operating system to make.
> > > > 
> > > Again this is just for historic reason and BCMBCA as late player does not
> > > want to break any existing dts setting.  So I thought it is useful to
> > > explain here.  I can remove this text if you don't think it should be in the
> > > binding document.
> > 
> > I don't, at least not in that form. I think it is reasonable to explain
> > why these values are better though.
> I understand the decision is for OS/driver to make. If we were to write
> another driver for other OS, the same precedence will apply too so the
> binding works the same way across all the OS. So I am not sure what better
> explanation or form I can put up here. I am open to any suggestion or I just
> delete it.

I would just delete it tbh.

Thanks,
Conor.

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

Re: [PATCH v3 01/10] dt-bindings: mtd: brcmnand: Updates for bcmbca SoCs

[William Zhang]

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



On 1/26/24 08:46, Conor Dooley wrote:
> On Thu, Jan 25, 2024 at 05:55:29PM -0800, William Zhang wrote:
>> On 1/25/24 11:51, Conor Dooley wrote:
>>> On Wed, Jan 24, 2024 at 07:01:48PM -0800, William Zhang wrote:
>>>> On 1/24/24 09:24, Conor Dooley wrote:
>>>>> On Tue, Jan 23, 2024 at 07:04:49PM -0800, David Regan wrote:
>>>>>> From: William Zhang <william.zhang@broadcom.com>
> 
>>>> And the reason I keep it as int is because there could be a potential value
>>>> of 2 for another mode that the current driver could set the wp_on to.
>>>
>>> Can you explain, in driver independent terms, what this other mode has
>>> to do with how the WP is connected?
>>> Either you've got the standard scenario where apparently "NAND_WPb" and
>>> "WP_L" are connected and another situation where they are not.
>>> Is there another pin configuration in addition to that, that this 3rd
>>> mode represents?
>>>
>> The 3rd mode is WP pin connected but wp feature is disabled in the
>> controller.
> 
> What does "disabled" mean? The controller itself is not capable of using
> the WP pin because of hardware modifications? Or there's a bit in a
> register that disables it and that bit has been set by software?
> 
Yes that is a bit in the controller reg to disable the feature by the 
driver even the chip have the WP pin connected.

> If it is a hardware difference for that controller, why does it not have
> a dedicated compatible? If it's a software decision, then it shouldn't
> be in the DT in the first place ;)
> 
Agree it is more on the software for that particular mode.

> We've gone back here a bunch trying to figure stuff out, and you give me
> a vague one sentence answer. Please.
> 
>>>> I
>>>> don't see it is being used in BCMBCA product but I am not sure about other
>>>> SoC family. So I don't want to completely close the door here just in case.
>>>
>>> If you ever need it, you could have a "brcm,wp-in-wacky-configuration"
>>> property that indicates that the hardware is configured in that way
>>> (providing that this hardware configuration is not just "NAND_WPb" and
>>> "WP_L" pins connected. The property can very easily be made mutually
>>> exclusive with "brcm,wp-not-connected" iff it ever comes to be.
>> Yes we could have a new property but if we can have a single int property to
>> cover all, IMHO it is better to just have one property as these three modes
>> are related. I would really like to have it as an int property to keep
>> things simple.
> 
> It is "better" because it is easier for you perhaps, but ripe for abuse.
> 
Well if binding only say 3 possible value, you can not use this property 
for other value of course. DTS binding check will fail. But agree there 
is no check on this in the driver side for any integer property.

>> But if you think this is absolutely against the dts rule,  I will switch to
>> the "brcm,wp-not-connected" boolean property as you suggested.
> 
> I'll answer this when you describe the mode better, right now I cannot
> really comment, but I have not yet seen a justification for the non-flag
> version of the property. Even if there's a justification for documenting
> that other mode in the DT, I'm likely to still think boolean properties
> are a better fit.
> 
That's fine. I will just change to the boolean.

>>>>>> If ecc
>>>>>> +          strength and spare area size are set by nand-ecc-strength
>>>>>> +          and brcm,nand-oob-sector-size in the dts, these settings
>>>>>> +          have precedence and override this flag.
>>>>>
>>>>> Again, IMO, this is not for the binding to decide. That is a decision
>>>>> for the operating system to make.
>>>>>
>>>> Again this is just for historic reason and BCMBCA as late player does not
>>>> want to break any existing dts setting.  So I thought it is useful to
>>>> explain here.  I can remove this text if you don't think it should be in the
>>>> binding document.
>>>
>>> I don't, at least not in that form. I think it is reasonable to explain
>>> why these values are better though.
>> I understand the decision is for OS/driver to make. If we were to write
>> another driver for other OS, the same precedence will apply too so the
>> binding works the same way across all the OS. So I am not sure what better
>> explanation or form I can put up here. I am open to any suggestion or I just
>> delete it.
> 
> I would just delete it tbh.
Will delete.

> 
> Thanks,
> Conor.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4212 bytes --]

Re: [PATCH v3 10/10] mtd: rawnand: brcmnand: allow for on-die ecc

[David Regan]

Hi Miquèl,

On Thu, Jan 25, 2024 at 10:19 PM Miquel Raynal
<miquel.raynal@bootlin.com> wrote:
>
> Hi David,
>
> dregan@broadcom.com wrote on Thu, 25 Jan 2024 11:47:46 -0800:
>
> > Hi Miquèl,
> >
> > On Wed, Jan 24, 2024 at 9:40 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > >
> > > Hi David,
> > >
> > > dregan@broadcom.com wrote on Tue, 23 Jan 2024 19:04:58 -0800:
> > >
> > > > Allow settings for on-die ecc such that if on-die ECC is selected
> > > > don't error out but require ECC strap setting of zero
> > > >
> > > > Signed-off-by: David Regan <dregan@broadcom.com>
> > > > Reviewed-by: William Zhang <william.zhang@broadcom.com>
> > > > ---
> > > > Changes in v3: None
> > > > ---
> > > > Changes in v2:
> > > > - Added to patch series
> > > > ---
> > > >  drivers/mtd/nand/raw/brcmnand/brcmnand.c | 14 ++++++++++----
> > > >  1 file changed, 10 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> > > > index a4e311b6798c..42526f3250c9 100644
> > > > --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> > > > +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> > > > @@ -2727,9 +2727,11 @@ static int brcmnand_setup_dev(struct brcmnand_host *host)
> > > >       cfg->blk_adr_bytes = get_blk_adr_bytes(mtd->size, mtd->writesize);
> > > >
> > > >       if (chip->ecc.engine_type != NAND_ECC_ENGINE_TYPE_ON_HOST) {
> > > > -             dev_err(ctrl->dev, "only HW ECC supported; selected: %d\n",
> > > > -                     chip->ecc.engine_type);
> > > > -             return -EINVAL;
> > > > +             if (chip->ecc.strength) {
> > > > +                     dev_err(ctrl->dev, "ERROR!!! HW ECC must be set to zero for non-hardware ECC; selected: %d\n",
> > > > +                             chip->ecc.strength);
> > >
> > > Can you use a more formal string? Also clarify it because I don't
> > > really understand what it leads to.
> >
> > How about:
> >
> > dev_err(ctrl->dev, "HW ECC set to %d, must be zero for on-die ECC\n",
>
> Actually I am wondering how legitimate this is. Just don't enable the
> on host ECC engine if it's not in use. No need to check the core's
> choice.

Our chip ECC engine will either be on if it's needed or off if it's not.
Either I can do that in one place or put checks in before each
read/write to turn on/off the ECC engine, which seems a lot more
work and changes and possible issues/problems.
Turning it on/off as needed has not been explicitly tested and
could cause unforeseen consequences. This
is a minimal change which should have minimal impact.

>
> >
> > >
> > > > +                     return -EINVAL;
> > > > +             }
> > > >       }
> > > >
> > > >       if (chip->ecc.algo == NAND_ECC_ALGO_UNKNOWN) {
> > > > @@ -2797,7 +2799,11 @@ static int brcmnand_setup_dev(struct brcmnand_host *host)
> > > >       if (ret)
> > > >               return ret;
> > > >
> > > > -     brcmnand_set_ecc_enabled(host, 1);
> > > > +     if (chip->ecc.engine_type == NAND_ECC_ENGINE_TYPE_ON_DIE) {
> > > > +             dev_dbg(ctrl->dev, "Disable HW ECC for on-die ECC\n");
> > >
> > > Not needed.
> >
> > Will remove.
> >
> > >
> > > > +             brcmnand_set_ecc_enabled(host, 0);
> > > > +     } else
> > > > +             brcmnand_set_ecc_enabled(host, 1);
> > >
> > > Style is wrong, but otherwise I think ECC should be kept disabled while
> > > not in active use, so I am a bit surprised by this line.
> >
> > This is a double check to turn on/off our hardware ECC.
>
> I expect the engine to be always disabled. Enable it only when you
> need (may require an additional patch before this one).

We are already turning on the ECC enable at this point,
this is just adding the option to turn it off if the NAND chip
itself will be doing the ECC instead of our controller.

>
> Thanks,
> Miquèl

Thanks!

-Dave

Re: [PATCH v3 10/10] mtd: rawnand: brcmnand: allow for on-die ecc

[Miquel Raynal]

Hi David,

dregan@broadcom.com wrote on Fri, 26 Jan 2024 11:57:39 -0800:

> Hi Miquèl,
> 
> On Thu, Jan 25, 2024 at 10:19 PM Miquel Raynal
> <miquel.raynal@bootlin.com> wrote:
> >
> > Hi David,
> >
> > dregan@broadcom.com wrote on Thu, 25 Jan 2024 11:47:46 -0800:
> >  
> > > Hi Miquèl,
> > >
> > > On Wed, Jan 24, 2024 at 9:40 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:  
> > > >
> > > > Hi David,
> > > >
> > > > dregan@broadcom.com wrote on Tue, 23 Jan 2024 19:04:58 -0800:
> > > >  
> > > > > Allow settings for on-die ecc such that if on-die ECC is selected
> > > > > don't error out but require ECC strap setting of zero
> > > > >
> > > > > Signed-off-by: David Regan <dregan@broadcom.com>
> > > > > Reviewed-by: William Zhang <william.zhang@broadcom.com>
> > > > > ---
> > > > > Changes in v3: None
> > > > > ---
> > > > > Changes in v2:
> > > > > - Added to patch series
> > > > > ---
> > > > >  drivers/mtd/nand/raw/brcmnand/brcmnand.c | 14 ++++++++++----
> > > > >  1 file changed, 10 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> > > > > index a4e311b6798c..42526f3250c9 100644
> > > > > --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> > > > > +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> > > > > @@ -2727,9 +2727,11 @@ static int brcmnand_setup_dev(struct brcmnand_host *host)
> > > > >       cfg->blk_adr_bytes = get_blk_adr_bytes(mtd->size, mtd->writesize);
> > > > >
> > > > >       if (chip->ecc.engine_type != NAND_ECC_ENGINE_TYPE_ON_HOST) {
> > > > > -             dev_err(ctrl->dev, "only HW ECC supported; selected: %d\n",
> > > > > -                     chip->ecc.engine_type);
> > > > > -             return -EINVAL;
> > > > > +             if (chip->ecc.strength) {
> > > > > +                     dev_err(ctrl->dev, "ERROR!!! HW ECC must be set to zero for non-hardware ECC; selected: %d\n",
> > > > > +                             chip->ecc.strength);  
> > > >
> > > > Can you use a more formal string? Also clarify it because I don't
> > > > really understand what it leads to.  
> > >
> > > How about:
> > >
> > > dev_err(ctrl->dev, "HW ECC set to %d, must be zero for on-die ECC\n",  
> >
> > Actually I am wondering how legitimate this is. Just don't enable the
> > on host ECC engine if it's not in use. No need to check the core's
> > choice.  
> 
> Our chip ECC engine will either be on if it's needed or off if it's not.
> Either I can do that in one place or put checks in before each
> read/write to turn on/off the ECC engine, which seems a lot more
> work and changes and possible issues/problems.
> Turning it on/off as needed has not been explicitly tested and
> could cause unforeseen consequences. This
> is a minimal change which should have minimal impact.
> 
> >  
> > >  
> > > >  
> > > > > +                     return -EINVAL;
> > > > > +             }
> > > > >       }
> > > > >
> > > > >       if (chip->ecc.algo == NAND_ECC_ALGO_UNKNOWN) {
> > > > > @@ -2797,7 +2799,11 @@ static int brcmnand_setup_dev(struct brcmnand_host *host)
> > > > >       if (ret)
> > > > >               return ret;
> > > > >
> > > > > -     brcmnand_set_ecc_enabled(host, 1);
> > > > > +     if (chip->ecc.engine_type == NAND_ECC_ENGINE_TYPE_ON_DIE) {
> > > > > +             dev_dbg(ctrl->dev, "Disable HW ECC for on-die ECC\n");  
> > > >
> > > > Not needed.  
> > >
> > > Will remove.
> > >  
> > > >  
> > > > > +             brcmnand_set_ecc_enabled(host, 0);
> > > > > +     } else
> > > > > +             brcmnand_set_ecc_enabled(host, 1);  
> > > >
> > > > Style is wrong, but otherwise I think ECC should be kept disabled while
> > > > not in active use, so I am a bit surprised by this line.  
> > >
> > > This is a double check to turn on/off our hardware ECC.  
> >
> > I expect the engine to be always disabled. Enable it only when you
> > need (may require an additional patch before this one).  
> 
> We are already turning on the ECC enable at this point,
> this is just adding the option to turn it off if the NAND chip
> itself will be doing the ECC instead of our controller.

Sorry if I have not been clear.

This sequence:
- init
- enable hw ECC engine
Is broken.

It *cannot* work as any operation going through exec_op now may
perform page reads which should be unmodified by the ECC engine. You
driver *must* follow the following sequence:

- init and disable (or keep disabled) the hw ECC engine
- when you perform a page operation with correction you need to
	- enable the engine
	- perform the operation
	- disable the engine

Thanks,
Miquèl

Re: [PATCH v3 10/10] mtd: rawnand: brcmnand: allow for on-die ecc

[William Zhang]

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

Hi Miquel,

On 1/29/24 02:52, Miquel Raynal wrote:
> Hi David,
> 
> dregan@broadcom.com wrote on Fri, 26 Jan 2024 11:57:39 -0800:
> 
>> Hi Miquèl,
>>
>> On Thu, Jan 25, 2024 at 10:19 PM Miquel Raynal
>> <miquel.raynal@bootlin.com> wrote:
>>>
>>> Hi David,
>>>
>>> dregan@broadcom.com wrote on Thu, 25 Jan 2024 11:47:46 -0800:
>>>   
>>>> Hi Miquèl,
>>>>
>>>> On Wed, Jan 24, 2024 at 9:40 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>>>>>
>>>>> Hi David,
>>>>>
>>>>> dregan@broadcom.com wrote on Tue, 23 Jan 2024 19:04:58 -0800:
>>>>>   
>>>>>> Allow settings for on-die ecc such that if on-die ECC is selected
>>>>>> don't error out but require ECC strap setting of zero
>>>>>>
>>>>>> Signed-off-by: David Regan <dregan@broadcom.com>
>>>>>> Reviewed-by: William Zhang <william.zhang@broadcom.com>
>>>>>> ---
>>>>>> Changes in v3: None
>>>>>> ---
>>>>>> Changes in v2:
>>>>>> - Added to patch series
>>>>>> ---
>>>>>>   drivers/mtd/nand/raw/brcmnand/brcmnand.c | 14 ++++++++++----
>>>>>>   1 file changed, 10 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
>>>>>> index a4e311b6798c..42526f3250c9 100644
>>>>>> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
>>>>>> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
>>>>>> @@ -2727,9 +2727,11 @@ static int brcmnand_setup_dev(struct brcmnand_host *host)
>>>>>>        cfg->blk_adr_bytes = get_blk_adr_bytes(mtd->size, mtd->writesize);
>>>>>>
>>>>>>        if (chip->ecc.engine_type != NAND_ECC_ENGINE_TYPE_ON_HOST) {
>>>>>> -             dev_err(ctrl->dev, "only HW ECC supported; selected: %d\n",
>>>>>> -                     chip->ecc.engine_type);
>>>>>> -             return -EINVAL;
>>>>>> +             if (chip->ecc.strength) {
>>>>>> +                     dev_err(ctrl->dev, "ERROR!!! HW ECC must be set to zero for non-hardware ECC; selected: %d\n",
>>>>>> +                             chip->ecc.strength);
>>>>>
>>>>> Can you use a more formal string? Also clarify it because I don't
>>>>> really understand what it leads to.
>>>>
>>>> How about:
>>>>
>>>> dev_err(ctrl->dev, "HW ECC set to %d, must be zero for on-die ECC\n",
>>>
>>> Actually I am wondering how legitimate this is. Just don't enable the
>>> on host ECC engine if it's not in use. No need to check the core's
>>> choice.
>>
>> Our chip ECC engine will either be on if it's needed or off if it's not.
>> Either I can do that in one place or put checks in before each
>> read/write to turn on/off the ECC engine, which seems a lot more
>> work and changes and possible issues/problems.
>> Turning it on/off as needed has not been explicitly tested and
>> could cause unforeseen consequences. This
>> is a minimal change which should have minimal impact.
>>
>>>   
>>>>   
>>>>>   
>>>>>> +                     return -EINVAL;
>>>>>> +             }
>>>>>>        }
>>>>>>
>>>>>>        if (chip->ecc.algo == NAND_ECC_ALGO_UNKNOWN) {
>>>>>> @@ -2797,7 +2799,11 @@ static int brcmnand_setup_dev(struct brcmnand_host *host)
>>>>>>        if (ret)
>>>>>>                return ret;
>>>>>>
>>>>>> -     brcmnand_set_ecc_enabled(host, 1);
>>>>>> +     if (chip->ecc.engine_type == NAND_ECC_ENGINE_TYPE_ON_DIE) {
>>>>>> +             dev_dbg(ctrl->dev, "Disable HW ECC for on-die ECC\n");
>>>>>
>>>>> Not needed.
>>>>
>>>> Will remove.
>>>>   
>>>>>   
>>>>>> +             brcmnand_set_ecc_enabled(host, 0);
>>>>>> +     } else
>>>>>> +             brcmnand_set_ecc_enabled(host, 1);
>>>>>
>>>>> Style is wrong, but otherwise I think ECC should be kept disabled while
>>>>> not in active use, so I am a bit surprised by this line.
>>>>
>>>> This is a double check to turn on/off our hardware ECC.
>>>
>>> I expect the engine to be always disabled. Enable it only when you
>>> need (may require an additional patch before this one).
>>
>> We are already turning on the ECC enable at this point,
>> this is just adding the option to turn it off if the NAND chip
>> itself will be doing the ECC instead of our controller.
> 
> Sorry if I have not been clear.
> 
> This sequence:
> - init
> - enable hw ECC engine
> Is broken.
> 
ECC engine is not enabled for all the cases. Here we only intended to 
enable it for the nand chip that is set to use 
NAND_ECC_ENGINE_TYPE_ON_HOST. The logic here should better change to:
if (chip->ecc.engine_type == NAND_ECC_ENGINE_TYPE_ON_HOST)
     brcmnand_set_ecc_enabled(host, 1);
else
     brcmnand_set_ecc_enabled(host, 0);

> It *cannot* work as any operation going through exec_op now may
> perform page reads which should be unmodified by the ECC engine. You > driver *must* follow the following sequence:
> - init and disable (or keep disabled) the hw ECC engine
> - when you perform a page operation with correction you need to
> 	- enable the engine
> 	- perform the operation
> 	- disable the engine
> Maybe I am missing something here but are you saying the exec_op can 
have different ecc type for page read/write at run time on the same nand 
chip? I don't see the op instr structure has the ecc type field and 
thought it is only bind to the nand chip and won't change at run time. 
So looks to me the init time setting to the engine based on 
ecc.engine_type should be sufficient.

What you described here can work for the hw.ecc read path (ecc.read_page 
= brcmnand_read_page) which always assumes ecc is enabled. Although it 
is probably not too bad with these two extra operation, it would be 
better if we don't have to add anything as our current code does. For 
the brcmnand_read_page_raw,  we currently disable the engine and then 
re-enable it(but we need to fix it to only enable it with hw ecc engine 
type).  So it is just opposite of you logic but works the same with no 
impact on the most performance critical path.


> Thanks,
> Miquèl

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4212 bytes --]

Re: [PATCH v3 10/10] mtd: rawnand: brcmnand: allow for on-die ecc

[Miquel Raynal]

Hi William,

william.zhang@broadcom.com wrote on Tue, 30 Jan 2024 00:11:32 -0800:

> Hi Miquel,
> 
> On 1/29/24 02:52, Miquel Raynal wrote:
> > Hi David,
> > 
> > dregan@broadcom.com wrote on Fri, 26 Jan 2024 11:57:39 -0800:
> >   
> >> Hi Miquèl,
> >>
> >> On Thu, Jan 25, 2024 at 10:19 PM Miquel Raynal
> >> <miquel.raynal@bootlin.com> wrote:  
> >>>
> >>> Hi David,
> >>>
> >>> dregan@broadcom.com wrote on Thu, 25 Jan 2024 11:47:46 -0800:  
> >>>   >>>> Hi Miquèl,  
> >>>>
> >>>> On Wed, Jan 24, 2024 at 9:40 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:  
> >>>>>
> >>>>> Hi David,
> >>>>>
> >>>>> dregan@broadcom.com wrote on Tue, 23 Jan 2024 19:04:58 -0800:  
> >>>>>   >>>>>> Allow settings for on-die ecc such that if on-die ECC is selected  
> >>>>>> don't error out but require ECC strap setting of zero
> >>>>>>
> >>>>>> Signed-off-by: David Regan <dregan@broadcom.com>
> >>>>>> Reviewed-by: William Zhang <william.zhang@broadcom.com>
> >>>>>> ---
> >>>>>> Changes in v3: None
> >>>>>> ---
> >>>>>> Changes in v2:
> >>>>>> - Added to patch series
> >>>>>> ---
> >>>>>>   drivers/mtd/nand/raw/brcmnand/brcmnand.c | 14 ++++++++++----
> >>>>>>   1 file changed, 10 insertions(+), 4 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> >>>>>> index a4e311b6798c..42526f3250c9 100644
> >>>>>> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> >>>>>> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> >>>>>> @@ -2727,9 +2727,11 @@ static int brcmnand_setup_dev(struct brcmnand_host *host)
> >>>>>>        cfg->blk_adr_bytes = get_blk_adr_bytes(mtd->size, mtd->writesize);
> >>>>>>
> >>>>>>        if (chip->ecc.engine_type != NAND_ECC_ENGINE_TYPE_ON_HOST) {
> >>>>>> -             dev_err(ctrl->dev, "only HW ECC supported; selected: %d\n",
> >>>>>> -                     chip->ecc.engine_type);
> >>>>>> -             return -EINVAL;
> >>>>>> +             if (chip->ecc.strength) {
> >>>>>> +                     dev_err(ctrl->dev, "ERROR!!! HW ECC must be set to zero for non-hardware ECC; selected: %d\n",
> >>>>>> +                             chip->ecc.strength);  
> >>>>>
> >>>>> Can you use a more formal string? Also clarify it because I don't
> >>>>> really understand what it leads to.  
> >>>>
> >>>> How about:
> >>>>
> >>>> dev_err(ctrl->dev, "HW ECC set to %d, must be zero for on-die ECC\n",  
> >>>
> >>> Actually I am wondering how legitimate this is. Just don't enable the
> >>> on host ECC engine if it's not in use. No need to check the core's
> >>> choice.  
> >>
> >> Our chip ECC engine will either be on if it's needed or off if it's not.
> >> Either I can do that in one place or put checks in before each
> >> read/write to turn on/off the ECC engine, which seems a lot more
> >> work and changes and possible issues/problems.
> >> Turning it on/off as needed has not been explicitly tested and
> >> could cause unforeseen consequences. This
> >> is a minimal change which should have minimal impact.
> >>  
> >>>   >>>>   >>>>>   >>>>>> +                     return -EINVAL;  
> >>>>>> +             }
> >>>>>>        }
> >>>>>>
> >>>>>>        if (chip->ecc.algo == NAND_ECC_ALGO_UNKNOWN) {
> >>>>>> @@ -2797,7 +2799,11 @@ static int brcmnand_setup_dev(struct brcmnand_host *host)
> >>>>>>        if (ret)
> >>>>>>                return ret;
> >>>>>>
> >>>>>> -     brcmnand_set_ecc_enabled(host, 1);
> >>>>>> +     if (chip->ecc.engine_type == NAND_ECC_ENGINE_TYPE_ON_DIE) {
> >>>>>> +             dev_dbg(ctrl->dev, "Disable HW ECC for on-die ECC\n");  
> >>>>>
> >>>>> Not needed.  
> >>>>
> >>>> Will remove.  
> >>>>   >>>>>   >>>>>> +             brcmnand_set_ecc_enabled(host, 0);  
> >>>>>> +     } else
> >>>>>> +             brcmnand_set_ecc_enabled(host, 1);  
> >>>>>
> >>>>> Style is wrong, but otherwise I think ECC should be kept disabled while
> >>>>> not in active use, so I am a bit surprised by this line.  
> >>>>
> >>>> This is a double check to turn on/off our hardware ECC.  
> >>>
> >>> I expect the engine to be always disabled. Enable it only when you
> >>> need (may require an additional patch before this one).  
> >>
> >> We are already turning on the ECC enable at this point,
> >> this is just adding the option to turn it off if the NAND chip
> >> itself will be doing the ECC instead of our controller.  
> > 
> > Sorry if I have not been clear.
> > 
> > This sequence:
> > - init
> > - enable hw ECC engine
> > Is broken.
> >   
> ECC engine is not enabled for all the cases. Here we only intended to enable it for the nand chip that is set to use NAND_ECC_ENGINE_TYPE_ON_HOST. The logic here should better change to:
> if (chip->ecc.engine_type == NAND_ECC_ENGINE_TYPE_ON_HOST)
>      brcmnand_set_ecc_enabled(host, 1);
> else
>      brcmnand_set_ecc_enabled(host, 0);
> 
> > It *cannot* work as any operation going through exec_op now may
> > perform page reads which should be unmodified by the ECC engine. You > driver *must* follow the following sequence:
> > - init and disable (or keep disabled) the hw ECC engine
> > - when you perform a page operation with correction you need to
> > 	- enable the engine
> > 	- perform the operation
> > 	- disable the engine
> > Maybe I am missing something here but are you saying the exec_op can have different ecc type for page read/write at run time on the same nand chip? I don't see the op instr structure has the ecc type field and thought it is only bind to the nand chip and won't change at run time. So looks to me the init time setting to the engine based on ecc.engine_type should be sufficient.  
> 
> What you described here can work for the hw.ecc read path (ecc.read_page = brcmnand_read_page) which always assumes ecc is enabled. Although it is probably not too bad with these two extra operation, it would be better if we don't have to add anything as our current code does. For the brcmnand_read_page_raw,  we currently disable the engine and then re-enable it(but we need to fix it to only enable it with hw ecc engine type).  So it is just opposite of you logic but works the same with no impact on the most performance critical path.

This is not "my" logic, this is the "core's" logic. I am saying: your
approach is broken because that is not how the API is supposed to work,
but it mostly works in the standard case.

Thanks,
Miquèl

Re: [PATCH v3 10/10] mtd: rawnand: brcmnand: allow for on-die ecc

[David Regan]

Hi Miquel,

On Tue, Jan 30, 2024 at 3:01 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> Hi William,
>
> william.zhang@broadcom.com wrote on Tue, 30 Jan 2024 00:11:32 -0800:
>
> > Hi Miquel,
> >
> > On 1/29/24 02:52, Miquel Raynal wrote:
> > > Hi David,
> > >
> > > dregan@broadcom.com wrote on Fri, 26 Jan 2024 11:57:39 -0800:
> > >
> > >> Hi Miquèl,
> > >>
> > >> On Thu, Jan 25, 2024 at 10:19 PM Miquel Raynal
> > >> <miquel.raynal@bootlin.com> wrote:
> > >>>
> > >>> Hi David,
> > >>>
> > >>> dregan@broadcom.com wrote on Thu, 25 Jan 2024 11:47:46 -0800:
> > >>>   >>>> Hi Miquèl,
> > >>>>
> > >>>> On Wed, Jan 24, 2024 at 9:40 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > >>>>>
> > >>>>> Hi David,
> > >>>>>
> > >>>>> dregan@broadcom.com wrote on Tue, 23 Jan 2024 19:04:58 -0800:
> > >>>>>   >>>>>> Allow settings for on-die ecc such that if on-die ECC is selected
> > >>>>>> don't error out but require ECC strap setting of zero
> > >>>>>>
> > >>>>>> Signed-off-by: David Regan <dregan@broadcom.com>
> > >>>>>> Reviewed-by: William Zhang <william.zhang@broadcom.com>
> > >>>>>> ---
> > >>>>>> Changes in v3: None
> > >>>>>> ---
> > >>>>>> Changes in v2:
> > >>>>>> - Added to patch series
> > >>>>>> ---
> > >>>>>>   drivers/mtd/nand/raw/brcmnand/brcmnand.c | 14 ++++++++++----
> > >>>>>>   1 file changed, 10 insertions(+), 4 deletions(-)
> > >>>>>>
> > >>>>>> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> > >>>>>> index a4e311b6798c..42526f3250c9 100644
> > >>>>>> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> > >>>>>> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> > >>>>>> @@ -2727,9 +2727,11 @@ static int brcmnand_setup_dev(struct brcmnand_host *host)
> > >>>>>>        cfg->blk_adr_bytes = get_blk_adr_bytes(mtd->size, mtd->writesize);
> > >>>>>>
> > >>>>>>        if (chip->ecc.engine_type != NAND_ECC_ENGINE_TYPE_ON_HOST) {
> > >>>>>> -             dev_err(ctrl->dev, "only HW ECC supported; selected: %d\n",
> > >>>>>> -                     chip->ecc.engine_type);
> > >>>>>> -             return -EINVAL;
> > >>>>>> +             if (chip->ecc.strength) {
> > >>>>>> +                     dev_err(ctrl->dev, "ERROR!!! HW ECC must be set to zero for non-hardware ECC; selected: %d\n",
> > >>>>>> +                             chip->ecc.strength);
> > >>>>>
> > >>>>> Can you use a more formal string? Also clarify it because I don't
> > >>>>> really understand what it leads to.
> > >>>>
> > >>>> How about:
> > >>>>
> > >>>> dev_err(ctrl->dev, "HW ECC set to %d, must be zero for on-die ECC\n",
> > >>>
> > >>> Actually I am wondering how legitimate this is. Just don't enable the
> > >>> on host ECC engine if it's not in use. No need to check the core's
> > >>> choice.
> > >>
> > >> Our chip ECC engine will either be on if it's needed or off if it's not.
> > >> Either I can do that in one place or put checks in before each
> > >> read/write to turn on/off the ECC engine, which seems a lot more
> > >> work and changes and possible issues/problems.
> > >> Turning it on/off as needed has not been explicitly tested and
> > >> could cause unforeseen consequences. This
> > >> is a minimal change which should have minimal impact.
> > >>
> > >>>   >>>>   >>>>>   >>>>>> +                     return -EINVAL;
> > >>>>>> +             }
> > >>>>>>        }
> > >>>>>>
> > >>>>>>        if (chip->ecc.algo == NAND_ECC_ALGO_UNKNOWN) {
> > >>>>>> @@ -2797,7 +2799,11 @@ static int brcmnand_setup_dev(struct brcmnand_host *host)
> > >>>>>>        if (ret)
> > >>>>>>                return ret;
> > >>>>>>
> > >>>>>> -     brcmnand_set_ecc_enabled(host, 1);
> > >>>>>> +     if (chip->ecc.engine_type == NAND_ECC_ENGINE_TYPE_ON_DIE) {
> > >>>>>> +             dev_dbg(ctrl->dev, "Disable HW ECC for on-die ECC\n");
> > >>>>>
> > >>>>> Not needed.
> > >>>>
> > >>>> Will remove.
> > >>>>   >>>>>   >>>>>> +             brcmnand_set_ecc_enabled(host, 0);
> > >>>>>> +     } else
> > >>>>>> +             brcmnand_set_ecc_enabled(host, 1);
> > >>>>>
> > >>>>> Style is wrong, but otherwise I think ECC should be kept disabled while
> > >>>>> not in active use, so I am a bit surprised by this line.
> > >>>>
> > >>>> This is a double check to turn on/off our hardware ECC.
> > >>>
> > >>> I expect the engine to be always disabled. Enable it only when you
> > >>> need (may require an additional patch before this one).
> > >>
> > >> We are already turning on the ECC enable at this point,
> > >> this is just adding the option to turn it off if the NAND chip
> > >> itself will be doing the ECC instead of our controller.
> > >
> > > Sorry if I have not been clear.
> > >
> > > This sequence:
> > > - init
> > > - enable hw ECC engine
> > > Is broken.
> > >
> > ECC engine is not enabled for all the cases. Here we only intended to enable it for the nand chip that is set to use NAND_ECC_ENGINE_TYPE_ON_HOST. The logic here should better change to:
> > if (chip->ecc.engine_type == NAND_ECC_ENGINE_TYPE_ON_HOST)
> >      brcmnand_set_ecc_enabled(host, 1);
> > else
> >      brcmnand_set_ecc_enabled(host, 0);
> >
> > > It *cannot* work as any operation going through exec_op now may
> > > perform page reads which should be unmodified by the ECC engine. You > driver *must* follow the following sequence:
> > > - init and disable (or keep disabled) the hw ECC engine
> > > - when you perform a page operation with correction you need to
> > >     - enable the engine
> > >     - perform the operation
> > >     - disable the engine
> > > Maybe I am missing something here but are you saying the exec_op can have different ecc type for page read/write at run time on the same nand chip? I don't see the op instr structure has the ecc type field and thought it is only bind to the nand chip and won't change at run time. So looks to me the init time setting to the engine based on ecc.engine_type should be sufficient.
> >
> > What you described here can work for the hw.ecc read path (ecc.read_page = brcmnand_read_page) which always assumes ecc is enabled. Although it is probably not too bad with these two extra operation, it would be better if we don't have to add anything as our current code does. For the brcmnand_read_page_raw,  we currently disable the engine and then re-enable it(but we need to fix it to only enable it with hw ecc engine type).  So it is just opposite of you logic but works the same with no impact on the most performance critical path.
>
> This is not "my" logic, this is the "core's" logic. I am saying: your
> approach is broken because that is not how the API is supposed to work,
> but it mostly works in the standard case.

In the interest of minimizing register writes, would it be acceptable to
enable/disable ECC at the beginning of a standard
path transfer but not, after the transfer, turn off the ECC? This should not
affect other standard path operations nor affect the exec_op path as those
are low level transfers which our ECC engine would not touch and the NAND
device driver should be responsible for turning on/off its own ECC.

>
> Thanks,
> Miquèl

Thanks!

-Dave

Re: [PATCH v3 10/10] mtd: rawnand: brcmnand: allow for on-die ecc

[Miquel Raynal]

Hi David,

dregan@broadcom.com wrote on Tue, 30 Jan 2024 07:26:02 -0800:

> Hi Miquel,
> 
> On Tue, Jan 30, 2024 at 3:01 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >
> > Hi William,
> >
> > william.zhang@broadcom.com wrote on Tue, 30 Jan 2024 00:11:32 -0800:
> >  
> > > Hi Miquel,
> > >
> > > On 1/29/24 02:52, Miquel Raynal wrote:  
> > > > Hi David,
> > > >
> > > > dregan@broadcom.com wrote on Fri, 26 Jan 2024 11:57:39 -0800:
> > > >  
> > > >> Hi Miquèl,
> > > >>
> > > >> On Thu, Jan 25, 2024 at 10:19 PM Miquel Raynal
> > > >> <miquel.raynal@bootlin.com> wrote:  
> > > >>>
> > > >>> Hi David,
> > > >>>
> > > >>> dregan@broadcom.com wrote on Thu, 25 Jan 2024 11:47:46 -0800:  
> > > >>>   >>>> Hi Miquèl,  
> > > >>>>
> > > >>>> On Wed, Jan 24, 2024 at 9:40 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:  
> > > >>>>>
> > > >>>>> Hi David,
> > > >>>>>
> > > >>>>> dregan@broadcom.com wrote on Tue, 23 Jan 2024 19:04:58 -0800:  
> > > >>>>>   >>>>>> Allow settings for on-die ecc such that if on-die ECC is selected  
> > > >>>>>> don't error out but require ECC strap setting of zero
> > > >>>>>>
> > > >>>>>> Signed-off-by: David Regan <dregan@broadcom.com>
> > > >>>>>> Reviewed-by: William Zhang <william.zhang@broadcom.com>
> > > >>>>>> ---
> > > >>>>>> Changes in v3: None
> > > >>>>>> ---
> > > >>>>>> Changes in v2:
> > > >>>>>> - Added to patch series
> > > >>>>>> ---
> > > >>>>>>   drivers/mtd/nand/raw/brcmnand/brcmnand.c | 14 ++++++++++----
> > > >>>>>>   1 file changed, 10 insertions(+), 4 deletions(-)
> > > >>>>>>
> > > >>>>>> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> > > >>>>>> index a4e311b6798c..42526f3250c9 100644
> > > >>>>>> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> > > >>>>>> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> > > >>>>>> @@ -2727,9 +2727,11 @@ static int brcmnand_setup_dev(struct brcmnand_host *host)
> > > >>>>>>        cfg->blk_adr_bytes = get_blk_adr_bytes(mtd->size, mtd->writesize);
> > > >>>>>>
> > > >>>>>>        if (chip->ecc.engine_type != NAND_ECC_ENGINE_TYPE_ON_HOST) {
> > > >>>>>> -             dev_err(ctrl->dev, "only HW ECC supported; selected: %d\n",
> > > >>>>>> -                     chip->ecc.engine_type);
> > > >>>>>> -             return -EINVAL;
> > > >>>>>> +             if (chip->ecc.strength) {
> > > >>>>>> +                     dev_err(ctrl->dev, "ERROR!!! HW ECC must be set to zero for non-hardware ECC; selected: %d\n",
> > > >>>>>> +                             chip->ecc.strength);  
> > > >>>>>
> > > >>>>> Can you use a more formal string? Also clarify it because I don't
> > > >>>>> really understand what it leads to.  
> > > >>>>
> > > >>>> How about:
> > > >>>>
> > > >>>> dev_err(ctrl->dev, "HW ECC set to %d, must be zero for on-die ECC\n",  
> > > >>>
> > > >>> Actually I am wondering how legitimate this is. Just don't enable the
> > > >>> on host ECC engine if it's not in use. No need to check the core's
> > > >>> choice.  
> > > >>
> > > >> Our chip ECC engine will either be on if it's needed or off if it's not.
> > > >> Either I can do that in one place or put checks in before each
> > > >> read/write to turn on/off the ECC engine, which seems a lot more
> > > >> work and changes and possible issues/problems.
> > > >> Turning it on/off as needed has not been explicitly tested and
> > > >> could cause unforeseen consequences. This
> > > >> is a minimal change which should have minimal impact.
> > > >>  
> > > >>>   >>>>   >>>>>   >>>>>> +                     return -EINVAL;  
> > > >>>>>> +             }
> > > >>>>>>        }
> > > >>>>>>
> > > >>>>>>        if (chip->ecc.algo == NAND_ECC_ALGO_UNKNOWN) {
> > > >>>>>> @@ -2797,7 +2799,11 @@ static int brcmnand_setup_dev(struct brcmnand_host *host)
> > > >>>>>>        if (ret)
> > > >>>>>>                return ret;
> > > >>>>>>
> > > >>>>>> -     brcmnand_set_ecc_enabled(host, 1);
> > > >>>>>> +     if (chip->ecc.engine_type == NAND_ECC_ENGINE_TYPE_ON_DIE) {
> > > >>>>>> +             dev_dbg(ctrl->dev, "Disable HW ECC for on-die ECC\n");  
> > > >>>>>
> > > >>>>> Not needed.  
> > > >>>>
> > > >>>> Will remove.  
> > > >>>>   >>>>>   >>>>>> +             brcmnand_set_ecc_enabled(host, 0);  
> > > >>>>>> +     } else
> > > >>>>>> +             brcmnand_set_ecc_enabled(host, 1);  
> > > >>>>>
> > > >>>>> Style is wrong, but otherwise I think ECC should be kept disabled while
> > > >>>>> not in active use, so I am a bit surprised by this line.  
> > > >>>>
> > > >>>> This is a double check to turn on/off our hardware ECC.  
> > > >>>
> > > >>> I expect the engine to be always disabled. Enable it only when you
> > > >>> need (may require an additional patch before this one).  
> > > >>
> > > >> We are already turning on the ECC enable at this point,
> > > >> this is just adding the option to turn it off if the NAND chip
> > > >> itself will be doing the ECC instead of our controller.  
> > > >
> > > > Sorry if I have not been clear.
> > > >
> > > > This sequence:
> > > > - init
> > > > - enable hw ECC engine
> > > > Is broken.
> > > >  
> > > ECC engine is not enabled for all the cases. Here we only intended to enable it for the nand chip that is set to use NAND_ECC_ENGINE_TYPE_ON_HOST. The logic here should better change to:
> > > if (chip->ecc.engine_type == NAND_ECC_ENGINE_TYPE_ON_HOST)
> > >      brcmnand_set_ecc_enabled(host, 1);
> > > else
> > >      brcmnand_set_ecc_enabled(host, 0);
> > >  
> > > > It *cannot* work as any operation going through exec_op now may
> > > > perform page reads which should be unmodified by the ECC engine. You > driver *must* follow the following sequence:
> > > > - init and disable (or keep disabled) the hw ECC engine
> > > > - when you perform a page operation with correction you need to
> > > >     - enable the engine
> > > >     - perform the operation
> > > >     - disable the engine
> > > > Maybe I am missing something here but are you saying the exec_op can have different ecc type for page read/write at run time on the same nand chip? I don't see the op instr structure has the ecc type field and thought it is only bind to the nand chip and won't change at run time. So looks to me the init time setting to the engine based on ecc.engine_type should be sufficient.  
> > >
> > > What you described here can work for the hw.ecc read path (ecc.read_page = brcmnand_read_page) which always assumes ecc is enabled. Although it is probably not too bad with these two extra operation, it would be better if we don't have to add anything as our current code does. For the brcmnand_read_page_raw,  we currently disable the engine and then re-enable it(but we need to fix it to only enable it with hw ecc engine type).  So it is just opposite of you logic but works the same with no impact on the most performance critical path.  
> >
> > This is not "my" logic, this is the "core's" logic. I am saying: your
> > approach is broken because that is not how the API is supposed to work,
> > but it mostly works in the standard case.  
> 
> In the interest of minimizing register writes, would it be acceptable to
> enable/disable ECC at the beginning of a standard
> path transfer but not, after the transfer, turn off the ECC? This should not
> affect other standard path operations nor affect the exec_op path as those
> are low level transfers which our ECC engine would not touch and the NAND
> device driver should be responsible for turning on/off its own ECC.

Do you have legitimate concerns about this register write taking way
more time than I could expect? Because compared to the transfer of a
NAND page + tR/tPROG it should not be noticeable. I don't see how you
could even measure such impact actually, unless the register write does
way more than usual. I'm fine with the above idea if you show me it has
an interest.

Thanks,
Miquèl

Re: [PATCH v3 10/10] mtd: rawnand: brcmnand: allow for on-die ecc

[William Zhang]

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

Hi Miquel,

On 1/30/24 10:55, Miquel Raynal wrote:
> Hi David,
> 
> dregan@broadcom.com wrote on Tue, 30 Jan 2024 07:26:02 -0800:
> 
>> Hi Miquel,
>>
>> On Tue, Jan 30, 2024 at 3:01 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>>>
>>> Hi William,
>>>
>>> william.zhang@broadcom.com wrote on Tue, 30 Jan 2024 00:11:32 -0800:
>>>   
>>>> Hi Miquel,
>>>>
>>>> On 1/29/24 02:52, Miquel Raynal wrote:
>>>>> Hi David,
>>>>>
>>>>> dregan@broadcom.com wrote on Fri, 26 Jan 2024 11:57:39 -0800:
>>>>>   
>>>>>> Hi Miquèl,
>>>>>>
>>>>>> On Thu, Jan 25, 2024 at 10:19 PM Miquel Raynal
>>>>>> <miquel.raynal@bootlin.com> wrote:
>>>>>>>
>>>>>>> Hi David,
>>>>>>>
>>>>>>> dregan@broadcom.com wrote on Thu, 25 Jan 2024 11:47:46 -0800:
>>>>>>>    >>>> Hi Miquèl,
>>>>>>>>
>>>>>>>> On Wed, Jan 24, 2024 at 9:40 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>>>>>>>>>
>>>>>>>>> Hi David,
>>>>>>>>>
>>>>>>>>> dregan@broadcom.com wrote on Tue, 23 Jan 2024 19:04:58 -0800:
>>>>>>>>>    >>>>>> Allow settings for on-die ecc such that if on-die ECC is selected
>>>>>>>>>> don't error out but require ECC strap setting of zero
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: David Regan <dregan@broadcom.com>
>>>>>>>>>> Reviewed-by: William Zhang <william.zhang@broadcom.com>
>>>>>>>>>> ---
>>>>>>>>>> Changes in v3: None
>>>>>>>>>> ---
>>>>>>>>>> Changes in v2:
>>>>>>>>>> - Added to patch series
>>>>>>>>>> ---
>>>>>>>>>>    drivers/mtd/nand/raw/brcmnand/brcmnand.c | 14 ++++++++++----
>>>>>>>>>>    1 file changed, 10 insertions(+), 4 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
>>>>>>>>>> index a4e311b6798c..42526f3250c9 100644
>>>>>>>>>> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
>>>>>>>>>> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
>>>>>>>>>> @@ -2727,9 +2727,11 @@ static int brcmnand_setup_dev(struct brcmnand_host *host)
>>>>>>>>>>         cfg->blk_adr_bytes = get_blk_adr_bytes(mtd->size, mtd->writesize);
>>>>>>>>>>
>>>>>>>>>>         if (chip->ecc.engine_type != NAND_ECC_ENGINE_TYPE_ON_HOST) {
>>>>>>>>>> -             dev_err(ctrl->dev, "only HW ECC supported; selected: %d\n",
>>>>>>>>>> -                     chip->ecc.engine_type);
>>>>>>>>>> -             return -EINVAL;
>>>>>>>>>> +             if (chip->ecc.strength) {
>>>>>>>>>> +                     dev_err(ctrl->dev, "ERROR!!! HW ECC must be set to zero for non-hardware ECC; selected: %d\n",
>>>>>>>>>> +                             chip->ecc.strength);
>>>>>>>>>
>>>>>>>>> Can you use a more formal string? Also clarify it because I don't
>>>>>>>>> really understand what it leads to.
>>>>>>>>
>>>>>>>> How about:
>>>>>>>>
>>>>>>>> dev_err(ctrl->dev, "HW ECC set to %d, must be zero for on-die ECC\n",
>>>>>>>
>>>>>>> Actually I am wondering how legitimate this is. Just don't enable the
>>>>>>> on host ECC engine if it's not in use. No need to check the core's
>>>>>>> choice.
>>>>>>
>>>>>> Our chip ECC engine will either be on if it's needed or off if it's not.
>>>>>> Either I can do that in one place or put checks in before each
>>>>>> read/write to turn on/off the ECC engine, which seems a lot more
>>>>>> work and changes and possible issues/problems.
>>>>>> Turning it on/off as needed has not been explicitly tested and
>>>>>> could cause unforeseen consequences. This
>>>>>> is a minimal change which should have minimal impact.
>>>>>>   
>>>>>>>    >>>>   >>>>>   >>>>>> +                     return -EINVAL;
>>>>>>>>>> +             }
>>>>>>>>>>         }
>>>>>>>>>>
>>>>>>>>>>         if (chip->ecc.algo == NAND_ECC_ALGO_UNKNOWN) {
>>>>>>>>>> @@ -2797,7 +2799,11 @@ static int brcmnand_setup_dev(struct brcmnand_host *host)
>>>>>>>>>>         if (ret)
>>>>>>>>>>                 return ret;
>>>>>>>>>>
>>>>>>>>>> -     brcmnand_set_ecc_enabled(host, 1);
>>>>>>>>>> +     if (chip->ecc.engine_type == NAND_ECC_ENGINE_TYPE_ON_DIE) {
>>>>>>>>>> +             dev_dbg(ctrl->dev, "Disable HW ECC for on-die ECC\n");
>>>>>>>>>
>>>>>>>>> Not needed.
>>>>>>>>
>>>>>>>> Will remove.
>>>>>>>>    >>>>>   >>>>>> +             brcmnand_set_ecc_enabled(host, 0);
>>>>>>>>>> +     } else
>>>>>>>>>> +             brcmnand_set_ecc_enabled(host, 1);
>>>>>>>>>
>>>>>>>>> Style is wrong, but otherwise I think ECC should be kept disabled while
>>>>>>>>> not in active use, so I am a bit surprised by this line.
>>>>>>>>
>>>>>>>> This is a double check to turn on/off our hardware ECC.
>>>>>>>
>>>>>>> I expect the engine to be always disabled. Enable it only when you
>>>>>>> need (may require an additional patch before this one).
>>>>>>
>>>>>> We are already turning on the ECC enable at this point,
>>>>>> this is just adding the option to turn it off if the NAND chip
>>>>>> itself will be doing the ECC instead of our controller.
>>>>>
>>>>> Sorry if I have not been clear.
>>>>>
>>>>> This sequence:
>>>>> - init
>>>>> - enable hw ECC engine
>>>>> Is broken.
>>>>>   
>>>> ECC engine is not enabled for all the cases. Here we only intended to enable it for the nand chip that is set to use NAND_ECC_ENGINE_TYPE_ON_HOST. The logic here should better change to:
>>>> if (chip->ecc.engine_type == NAND_ECC_ENGINE_TYPE_ON_HOST)
>>>>       brcmnand_set_ecc_enabled(host, 1);
>>>> else
>>>>       brcmnand_set_ecc_enabled(host, 0);
>>>>   
>>>>> It *cannot* work as any operation going through exec_op now may
>>>>> perform page reads which should be unmodified by the ECC engine. You > driver *must* follow the following sequence:
>>>>> - init and disable (or keep disabled) the hw ECC engine
>>>>> - when you perform a page operation with correction you need to
>>>>>      - enable the engine
>>>>>      - perform the operation
>>>>>      - disable the engine
>>>>> Maybe I am missing something here but are you saying the exec_op can have different ecc type for page read/write at run time on the same nand chip? I don't see the op instr structure has the ecc type field and thought it is only bind to the nand chip and won't change at run time. So looks to me the init time setting to the engine based on ecc.engine_type should be sufficient.
>>>>
>>>> What you described here can work for the hw.ecc read path (ecc.read_page = brcmnand_read_page) which always assumes ecc is enabled. Although it is probably not too bad with these two extra operation, it would be better if we don't have to add anything as our current code does. For the brcmnand_read_page_raw,  we currently disable the engine and then re-enable it(but we need to fix it to only enable it with hw ecc engine type).  So it is just opposite of you logic but works the same with no impact on the most performance critical path.
>>>
>>> This is not "my" logic, this is the "core's" logic. I am saying: your
>>> approach is broken because that is not how the API is supposed to work,
>>> but it mostly works in the standard case.
>>
>> In the interest of minimizing register writes, would it be acceptable to
>> enable/disable ECC at the beginning of a standard
>> path transfer but not, after the transfer, turn off the ECC? This should not
>> affect other standard path operations nor affect the exec_op path as those
>> are low level transfers which our ECC engine would not touch and the NAND
>> device driver should be responsible for turning on/off its own ECC.
> 
> Do you have legitimate concerns about this register write taking way
> more time than I could expect? Because compared to the transfer of a
> NAND page + tR/tPROG it should not be noticeable. I don't see how you
> could even measure such impact actually, unless the register write does
> way more than usual. I'm fine with the above idea if you show me it has
> an interest.
> 
Dave did the mtd_speed test and we can see we get consistently ~35KB/s 
slower with the extra enable and disable ecc engine calls in ecc read 
page path.

With the change:
[   28.148355] mtd_speedtest:   page read speed is 9857 KiB/s
[   31.754258] mtd_speedtest: 2 page read speed is 9865 KiB/s
Without the change
[   56.444735] mtd_speedtest:   page read speed is 9892 KiB/s
[   60.042262] mtd_speedtest: 2 page read speed is 9897 KiB/s

Although it is only less than 1% drop, it is still something. I 
understand the procedure you laid out above is the preferred way but 
with our driver fully control the chip ecc read/write page, ecc 
read_raw/write_raw page function and exec_op path, I don't see where it 
may not work. What are the non-standard cases that can break it?

If the driver started with preferred procedure,  we will never have this 
conversation:) But unfortunately it is not the case and now if we want 
to revert this but lose performance, I just don't feel like to swallow 
this if there is no real world case that breaks this code.

> Thanks,
> Miquèl

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4212 bytes --]

Re: [PATCH v3 10/10] mtd: rawnand: brcmnand: allow for on-die ecc

[Miquel Raynal]

Hi William,

> >>>>>>>> This is a double check to turn on/off our hardware ECC.  
> >>>>>>>
> >>>>>>> I expect the engine to be always disabled. Enable it only when you
> >>>>>>> need (may require an additional patch before this one).  
> >>>>>>
> >>>>>> We are already turning on the ECC enable at this point,
> >>>>>> this is just adding the option to turn it off if the NAND chip
> >>>>>> itself will be doing the ECC instead of our controller.  
> >>>>>
> >>>>> Sorry if I have not been clear.
> >>>>>
> >>>>> This sequence:
> >>>>> - init
> >>>>> - enable hw ECC engine
> >>>>> Is broken.  
> >>>>>   >>>> ECC engine is not enabled for all the cases. Here we only intended to enable it for the nand chip that is set to use NAND_ECC_ENGINE_TYPE_ON_HOST. The logic here should better change to:  
> >>>> if (chip->ecc.engine_type == NAND_ECC_ENGINE_TYPE_ON_HOST)
> >>>>       brcmnand_set_ecc_enabled(host, 1);
> >>>> else
> >>>>       brcmnand_set_ecc_enabled(host, 0);  
> >>>>   >>>>> It *cannot* work as any operation going through exec_op now may  
> >>>>> perform page reads which should be unmodified by the ECC engine. You > driver *must* follow the following sequence:
> >>>>> - init and disable (or keep disabled) the hw ECC engine
> >>>>> - when you perform a page operation with correction you need to
> >>>>>      - enable the engine
> >>>>>      - perform the operation
> >>>>>      - disable the engine
> >>>>> Maybe I am missing something here but are you saying the exec_op can have different ecc type for page read/write at run time on the same nand chip? I don't see the op instr structure has the ecc type field and thought it is only bind to the nand chip and won't change at run time. So looks to me the init time setting to the engine based on ecc.engine_type should be sufficient.  
> >>>>
> >>>> What you described here can work for the hw.ecc read path (ecc.read_page = brcmnand_read_page) which always assumes ecc is enabled. Although it is probably not too bad with these two extra operation, it would be better if we don't have to add anything as our current code does. For the brcmnand_read_page_raw,  we currently disable the engine and then re-enable it(but we need to fix it to only enable it with hw ecc engine type).  So it is just opposite of you logic but works the same with no impact on the most performance critical path.  
> >>>
> >>> This is not "my" logic, this is the "core's" logic. I am saying: your
> >>> approach is broken because that is not how the API is supposed to work,
> >>> but it mostly works in the standard case.  
> >>
> >> In the interest of minimizing register writes, would it be acceptable to
> >> enable/disable ECC at the beginning of a standard
> >> path transfer but not, after the transfer, turn off the ECC? This should not
> >> affect other standard path operations nor affect the exec_op path as those
> >> are low level transfers which our ECC engine would not touch and the NAND
> >> device driver should be responsible for turning on/off its own ECC.  
> > 
> > Do you have legitimate concerns about this register write taking way
> > more time than I could expect? Because compared to the transfer of a
> > NAND page + tR/tPROG it should not be noticeable. I don't see how you
> > could even measure such impact actually, unless the register write does
> > way more than usual. I'm fine with the above idea if you show me it has
> > an interest.
> >   
> Dave did the mtd_speed test and we can see we get consistently ~35KB/s slower with the extra enable and disable ecc engine calls in ecc read page path.
> 
> With the change:
> [   28.148355] mtd_speedtest:   page read speed is 9857 KiB/s
> [   31.754258] mtd_speedtest: 2 page read speed is 9865 KiB/s
> Without the change
> [   56.444735] mtd_speedtest:   page read speed is 9892 KiB/s
> [   60.042262] mtd_speedtest: 2 page read speed is 9897 KiB/s

I believe if you repeat this 10 times you'll get totally different
results. I don't think this test on a non RT machine is precise enough
so that a unique 35kiB difference can be interpreted as being
significant.

> Although it is only less than 1% drop, it is still something. I understand the procedure you laid out above is the preferred way but with our driver fully control the chip ecc read/write page, ecc read_raw/write_raw page function and exec_op path, I don't see where it may not work.

I just told you, the exec_op path runs with ECC enabled. I don't know
how this controller works. Now if you don't care and are 100% sure this
is working and future proof, just keep it like this.

Cheers,
Miquèl

Re: [PATCH v3 10/10] mtd: rawnand: brcmnand: allow for on-die ecc

[William Zhang]

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



On 2/1/24 00:25, Miquel Raynal wrote:
> Hi William,
> 
>>>>>>>>>> This is a double check to turn on/off our hardware ECC.
>>>>>>>>>
>>>>>>>>> I expect the engine to be always disabled. Enable it only when you
>>>>>>>>> need (may require an additional patch before this one).
>>>>>>>>
>>>>>>>> We are already turning on the ECC enable at this point,
>>>>>>>> this is just adding the option to turn it off if the NAND chip
>>>>>>>> itself will be doing the ECC instead of our controller.
>>>>>>>
>>>>>>> Sorry if I have not been clear.
>>>>>>>
>>>>>>> This sequence:
>>>>>>> - init
>>>>>>> - enable hw ECC engine
>>>>>>> Is broken.
>>>>>>>    >>>> ECC engine is not enabled for all the cases. Here we only intended to enable it for the nand chip that is set to use NAND_ECC_ENGINE_TYPE_ON_HOST. The logic here should better change to:
>>>>>> if (chip->ecc.engine_type == NAND_ECC_ENGINE_TYPE_ON_HOST)
>>>>>>        brcmnand_set_ecc_enabled(host, 1);
>>>>>> else
>>>>>>        brcmnand_set_ecc_enabled(host, 0);
>>>>>>    >>>>> It *cannot* work as any operation going through exec_op now may
>>>>>>> perform page reads which should be unmodified by the ECC engine. You > driver *must* follow the following sequence:
>>>>>>> - init and disable (or keep disabled) the hw ECC engine
>>>>>>> - when you perform a page operation with correction you need to
>>>>>>>       - enable the engine
>>>>>>>       - perform the operation
>>>>>>>       - disable the engine
>>>>>>> Maybe I am missing something here but are you saying the exec_op can have different ecc type for page read/write at run time on the same nand chip? I don't see the op instr structure has the ecc type field and thought it is only bind to the nand chip and won't change at run time. So looks to me the init time setting to the engine based on ecc.engine_type should be sufficient.
>>>>>>
>>>>>> What you described here can work for the hw.ecc read path (ecc.read_page = brcmnand_read_page) which always assumes ecc is enabled. Although it is probably not too bad with these two extra operation, it would be better if we don't have to add anything as our current code does. For the brcmnand_read_page_raw,  we currently disable the engine and then re-enable it(but we need to fix it to only enable it with hw ecc engine type).  So it is just opposite of you logic but works the same with no impact on the most performance critical path.
>>>>>
>>>>> This is not "my" logic, this is the "core's" logic. I am saying: your
>>>>> approach is broken because that is not how the API is supposed to work,
>>>>> but it mostly works in the standard case.
>>>>
>>>> In the interest of minimizing register writes, would it be acceptable to
>>>> enable/disable ECC at the beginning of a standard
>>>> path transfer but not, after the transfer, turn off the ECC? This should not
>>>> affect other standard path operations nor affect the exec_op path as those
>>>> are low level transfers which our ECC engine would not touch and the NAND
>>>> device driver should be responsible for turning on/off its own ECC.
>>>
>>> Do you have legitimate concerns about this register write taking way
>>> more time than I could expect? Because compared to the transfer of a
>>> NAND page + tR/tPROG it should not be noticeable. I don't see how you
>>> could even measure such impact actually, unless the register write does
>>> way more than usual. I'm fine with the above idea if you show me it has
>>> an interest.
>>>    
>> Dave did the mtd_speed test and we can see we get consistently ~35KB/s slower with the extra enable and disable ecc engine calls in ecc read page path.
>>
>> With the change:
>> [   28.148355] mtd_speedtest:   page read speed is 9857 KiB/s
>> [   31.754258] mtd_speedtest: 2 page read speed is 9865 KiB/s
>> Without the change
>> [   56.444735] mtd_speedtest:   page read speed is 9892 KiB/s
>> [   60.042262] mtd_speedtest: 2 page read speed is 9897 KiB/s
> 
> I believe if you repeat this 10 times you'll get totally different
> results. I don't think this test on a non RT machine is precise enough
> so that a unique 35kiB difference can be interpreted as being
> significant.
> 
No, it is very consistent. It is not RT system but a bare minimum setup 
with just linux kernel, mtd/nand drivers and a shell. We don't run any 
apps and extra drivers. So the system is pretty idle while we run mtd 
speed test.

>> Although it is only less than 1% drop, it is still something. I understand the procedure you laid out above is the preferred way but with our driver fully control the chip ecc read/write page, ecc read_raw/write_raw page function and exec_op path, I don't see where it may not work.
> 
> I just told you, the exec_op path runs with ECC enabled. I don't know
> how this controller works. Now if you don't care and are 100% sure this
> is working and future proof, just keep it like this.
> 
This is something I don't get.  The patch basically only enable the ecc 
when NAND_ECC_ENGINE_TYPE_ON_HOST is set. But in this case exec_op does 
not do page read/write. Even page read/write go through exec_op, we do 
want the engine to do the ecc checking. So need to have it enabled. 
Other op like parameter read does not go through controller's ecc engine 
so it does not matter if ecc is on or not.

If NAND_ECC_ENGINE_TYPE_ON_DIE is set, this patch disable the controller 
engine and let NAND chip do the work as it requires.  So exec_op path 
run with ecc disabled when page read/write also go through exec_op path. 
  I don't see any issue with this either.

I am not trying to make your job hard but I want to understand if there 
is really any issue here.

> Cheers,
> Miquèl

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4212 bytes --]

Re: [PATCH v3 10/10] mtd: rawnand: brcmnand: allow for on-die ecc

[David Regan]

Hi Miquèl,

On Thu, Feb 1, 2024 at 12:26 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> Hi William,
>
> > >>>>>>>> This is a double check to turn on/off our hardware ECC.
> > >>>>>>>
> > >>>>>>> I expect the engine to be always disabled. Enable it only when you
> > >>>>>>> need (may require an additional patch before this one).
> > >>>>>>
> > >>>>>> We are already turning on the ECC enable at this point,
> > >>>>>> this is just adding the option to turn it off if the NAND chip
> > >>>>>> itself will be doing the ECC instead of our controller.
> > >>>>>
> > >>>>> Sorry if I have not been clear.
> > >>>>>
> > >>>>> This sequence:
> > >>>>> - init
> > >>>>> - enable hw ECC engine
> > >>>>> Is broken.
> > >>>>>   >>>> ECC engine is not enabled for all the cases. Here we only intended to enable it for the nand chip that is set to use NAND_ECC_ENGINE_TYPE_ON_HOST. The logic here should better change to:
> > >>>> if (chip->ecc.engine_type == NAND_ECC_ENGINE_TYPE_ON_HOST)
> > >>>>       brcmnand_set_ecc_enabled(host, 1);
> > >>>> else
> > >>>>       brcmnand_set_ecc_enabled(host, 0);
> > >>>>   >>>>> It *cannot* work as any operation going through exec_op now may
> > >>>>> perform page reads which should be unmodified by the ECC engine. You > driver *must* follow the following sequence:
> > >>>>> - init and disable (or keep disabled) the hw ECC engine
> > >>>>> - when you perform a page operation with correction you need to
> > >>>>>      - enable the engine
> > >>>>>      - perform the operation
> > >>>>>      - disable the engine
> > >>>>> Maybe I am missing something here but are you saying the exec_op can have different ecc type for page read/write at run time on the same nand chip? I don't see the op instr structure has the ecc type field and thought it is only bind to the nand chip and won't change at run time. So looks to me the init time setting to the engine based on ecc.engine_type should be sufficient.
> > >>>>
> > >>>> What you described here can work for the hw.ecc read path (ecc.read_page = brcmnand_read_page) which always assumes ecc is enabled. Although it is probably not too bad with these two extra operation, it would be better if we don't have to add anything as our current code does. For the brcmnand_read_page_raw,  we currently disable the engine and then re-enable it(but we need to fix it to only enable it with hw ecc engine type).  So it is just opposite of you logic but works the same with no impact on the most performance critical path.
> > >>>
> > >>> This is not "my" logic, this is the "core's" logic. I am saying: your
> > >>> approach is broken because that is not how the API is supposed to work,
> > >>> but it mostly works in the standard case.
> > >>
> > >> In the interest of minimizing register writes, would it be acceptable to
> > >> enable/disable ECC at the beginning of a standard
> > >> path transfer but not, after the transfer, turn off the ECC? This should not
> > >> affect other standard path operations nor affect the exec_op path as those
> > >> are low level transfers which our ECC engine would not touch and the NAND
> > >> device driver should be responsible for turning on/off its own ECC.
> > >
> > > Do you have legitimate concerns about this register write taking way
> > > more time than I could expect? Because compared to the transfer of a
> > > NAND page + tR/tPROG it should not be noticeable. I don't see how you
> > > could even measure such impact actually, unless the register write does
> > > way more than usual. I'm fine with the above idea if you show me it has
> > > an interest.
> > >
> > Dave did the mtd_speed test and we can see we get consistently ~35KB/s slower with the extra enable and disable ecc engine calls in ecc read page path.
> >
> > With the change:
> > [   28.148355] mtd_speedtest:   page read speed is 9857 KiB/s
> > [   31.754258] mtd_speedtest: 2 page read speed is 9865 KiB/s
> > Without the change
> > [   56.444735] mtd_speedtest:   page read speed is 9892 KiB/s
> > [   60.042262] mtd_speedtest: 2 page read speed is 9897 KiB/s
>
> I believe if you repeat this 10 times you'll get totally different
> results. I don't think this test on a non RT machine is precise enough
> so that a unique 35kiB difference can be interpreted as being
> significant.
>
> > Although it is only less than 1% drop, it is still something. I understand the procedure you laid out above is the preferred way but with our driver fully control the chip ecc read/write page, ecc read_raw/write_raw page function and exec_op path, I don't see where it may not work.
>
> I just told you, the exec_op path runs with ECC enabled. I don't know
> how this controller works. Now if you don't care and are 100% sure this
> is working and future proof, just keep it like this.

Thank you for all your help, it appears this will need further rework
and testing. Since it's not critical to this series I will most likely
pull this patch and take all your good information to prepare for
a future update.

>
> Cheers,
> Miquèl

Thanks!

-Dave