[PATCH v3 0/3] mtd: nand: Rework/cleanup the Atmel NAND driver

[PATCH v3 0/3] mtd: nand: Rework/cleanup the Atmel NAND driver

[Boris Brezillon]

This is a complete rewrite of the driver whose main purpose is to
support the new DT representation where the NAND controller node is now
really visible in the DT and appears under the EBI bus. With this new
representation, we can add other devices under the EBI bus without
risking pinmuxing conflicts (the NAND controller is under the EBI
bus logic and as such, share some of its pins with other devices
connected on this bus).

Even though the goal of this rework was not necessarily to add new
features, the new driver has been designed with this in mind. With a
clearer separation between the different blocks and different IP
revisions, adding new functionalities should be easier (we already
have plans to support SMC timing configuration so that we no longer
have to rely on the configuration done by the bootloader/bootstrap).

Also note that we no longer have a custom ->cmdfunc() implementation,
which means we can now benefit from new features added in the core
implementation for free (support for new NAND operations for example).

The last thing that we gain with this rework is support for multi-chips
and multi-dies chips, thanks to the clean NAND controller <-> NAND
devices representation.

This new driver has been tested on several platforms (at91sam9261,
at91sam9g45, at91sam9x5, sama5d3 and sama5d4) to make sure it did not
introduce regressions, and it's worth mentioning that old bindings are
still supported (which partly explain the positive diffstat).

Regards,

Boris

Changes since v2:
- add error messages when the NFC reports an error
- drop AVR32 compat code
- add acks
- minor coding style fixes

Changes since v1:
- change function/structure prefixes (asked by Nicolas)
- drop applied patches
- use new GPIO helpers
- set ->chip_delay to 40 as done in the old driver (reported by Nicolas)
- rework read_page to improve perfs
- add a better commit message to patch 2

Boris Brezillon (3):
  mtd: nand: Cleanup/rework the atmel_nand driver
  mtd: nand: atmel: Document the new DT bindings
  mtd: nand: Remove unused chip->write_page() hook

 .../devicetree/bindings/mtd/atmel-nand.txt         |  107 +-
 MAINTAINERS                                        |    2 +-
 drivers/mtd/nand/Kconfig                           |    6 +-
 drivers/mtd/nand/Makefile                          |    2 +-
 drivers/mtd/nand/atmel/Makefile                    |    4 +
 drivers/mtd/nand/atmel/nand-controller.c           | 2198 +++++++++++++++++
 drivers/mtd/nand/atmel/pmecc.c                     | 1020 ++++++++
 drivers/mtd/nand/atmel/pmecc.h                     |   73 +
 drivers/mtd/nand/atmel_nand.c                      | 2479 --------------------
 drivers/mtd/nand/atmel_nand_ecc.h                  |  163 --
 drivers/mtd/nand/atmel_nand_nfc.h                  |  103 -
 drivers/mtd/nand/nand_base.c                       |   12 +-
 include/linux/mtd/nand.h                           |    4 -
 13 files changed, 3411 insertions(+), 2762 deletions(-)
 create mode 100644 drivers/mtd/nand/atmel/Makefile
 create mode 100644 drivers/mtd/nand/atmel/nand-controller.c
 create mode 100644 drivers/mtd/nand/atmel/pmecc.c
 create mode 100644 drivers/mtd/nand/atmel/pmecc.h
 delete mode 100644 drivers/mtd/nand/atmel_nand.c
 delete mode 100644 drivers/mtd/nand/atmel_nand_ecc.h
 delete mode 100644 drivers/mtd/nand/atmel_nand_nfc.h

-- 
2.7.4

[PATCH v3 2/3] mtd: nand: atmel: Document the new DT bindings

[Boris Brezillon]

The old NAND bindings were not exactly describing the hardware topology
and were preventing definitions of several NAND chips under the same
NAND controller.

New bindings address these limitations and should be preferred over the
old ones for new SoCs/boards.
Old bindings are still supported for backward compatibility but are
marked deprecated in the doc.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
Reviewed-by: Nicolas Ferre <nicolas.ferre@microchip.com>
Acked-by: Rob Herring <robh@kernel.org>
---
 .../devicetree/bindings/mtd/atmel-nand.txt         | 107 ++++++++++++++++++++-
 1 file changed, 106 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/mtd/atmel-nand.txt b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
index 3e7ee99d3949..f6bee57e453a 100644
--- a/Documentation/devicetree/bindings/mtd/atmel-nand.txt
+++ b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
@@ -1,4 +1,109 @@
-Atmel NAND flash
+Atmel NAND flash controller bindings
+
+The NAND flash controller node should be defined under the EBI bus (see
+Documentation/devicetree/bindings/memory-controllers/atmel,ebi.txt).
+One or several NAND devices can be defined under this NAND controller.
+The NAND controller might be connected to an ECC engine.
+
+* NAND controller bindings:
+
+Required properties:
+- compatible: should be one of the following
+	"atmel,at91rm9200-nand-controller"
+	"atmel,at91sam9260-nand-controller"
+	"atmel,at91sam9261-nand-controller"
+	"atmel,at91sam9g45-nand-controller"
+	"atmel,sama5d3-nand-controller"
+- ranges: empty ranges property to forward EBI ranges definitions.
+- #address-cells: should be set to 2.
+- #size-cells: should be set to 1.
+- atmel,nfc-io: phandle to the NFC IO block. Only required for sama5d3
+		controllers.
+- atmel,nfc-sram: phandle to the NFC SRAM block. Only required for sama5d3
+		  controllers.
+
+Optional properties:
+- ecc-engine: phandle to the PMECC block. Only meaningful if the SoC embeds
+	      a PMECC engine.
+
+* NAND device/chip bindings:
+
+Required properties:
+- reg: describes the CS lines assigned to the NAND device. If the NAND device
+       exposes multiple CS lines (multi-dies chips), your reg property will
+       contain X tuples of 3 entries.
+       1st entry: the CS line this NAND chip is connected to
+       2nd entry: the base offset of the memory region assigned to this
+		  device (always 0)
+       3rd entry: the memory region size (always 0x800000)
+
+Optional properties:
+- rb-gpios: the GPIO(s) used to check the Ready/Busy status of the NAND.
+- cs-gpios: the GPIO(s) used to control the CS line.
+- det-gpios: the GPIO used to detect if a Smartmedia Card is present.
+- atmel,rb: an integer identifying the native Ready/Busy pin. Only meaningful
+	    on sama5 SoCs.
+
+All generic properties described in
+Documentation/devicetree/bindings/mtd/{common,nand}.txt also apply to the NAND
+device node, and NAND partitions should be defined under the NAND node as
+described in Documentation/devicetree/bindings/mtd/partition.txt.
+
+* ECC engine (PMECC) bindings:
+
+Required properties:
+- compatible: should be one of the following
+	"atmel,at91sam9g45-pmecc"
+	"atmel,sama5d4-pmecc"
+	"atmel,sama5d2-pmecc"
+- reg: should contain 2 register ranges. The first one is pointing to the PMECC
+       block, and the second one to the PMECC_ERRLOC block.
+
+Example:
+
+	pmecc: ecc-engine at ffffc070 {
+		compatible = "atmel,at91sam9g45-pmecc";
+                reg = <0xffffc070 0x490>,
+                      <0xffffc500 0x100>;
+	};
+
+	ebi: ebi at 10000000 {
+		compatible = "atmel,sama5d3-ebi";
+		#address-cells = <2>;
+		#size-cells = <1>;
+		atmel,smc = <&hsmc>;
+		reg = <0x10000000 0x10000000
+		       0x40000000 0x30000000>;
+		ranges = <0x0 0x0 0x10000000 0x10000000
+			  0x1 0x0 0x40000000 0x10000000
+			  0x2 0x0 0x50000000 0x10000000
+			  0x3 0x0 0x60000000 0x10000000>;
+		clocks = <&mck>;
+
+                nand_controller: nand-controller {
+			compatible = "atmel,sama5d3-nand-controller";
+			atmel,nfc-sram = <&nfc_sram>;
+			atmel,nfc-io = <&nfc_io>;
+			ecc-engine = <&pmecc>;
+			#address-cells = <2>;
+			#size-cells = <1>;
+			ranges;
+
+			nand at 3 {
+				reg = <0x3 0x0 0x800000>;
+				atmel,rb = <0>;
+
+				/*
+				 * Put generic NAND/MTD properties and
+				 * subnodes here.
+				 */
+			};
+		};
+	};
+
+-----------------------------------------------------------------------
+
+Deprecated bindings (should not be used in new device trees):
 
 Required properties:
 - compatible: The possible values are:
-- 
2.7.4

[PATCH v3 3/3] mtd: nand: Remove unused chip->write_page() hook

[Boris Brezillon]

The last/only user of the chip->write_page() hook (the Atmel NAND
controller driver) has been reworked and is no longer specifying a custom
->write_page() implementation.
Drop this hook before someone else start abusing it.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/mtd/nand/nand_base.c | 12 +++++-------
 include/linux/mtd/nand.h     |  4 ----
 2 files changed, 5 insertions(+), 11 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index b0524f8accb6..a28be471ebe0 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -2637,7 +2637,7 @@ static int nand_write_page_syndrome(struct mtd_info *mtd,
 }
 
 /**
- * nand_write_page - [REPLACEABLE] write one page
+ * nand_write_page - write one page
  * @mtd: MTD device structure
  * @chip: NAND chip descriptor
  * @offset: address offset within the page
@@ -2840,9 +2840,10 @@ static int nand_do_write_ops(struct mtd_info *mtd, loff_t to,
 			/* We still need to erase leftover OOB data */
 			memset(chip->oob_poi, 0xff, mtd->oobsize);
 		}
-		ret = chip->write_page(mtd, chip, column, bytes, wbuf,
-					oob_required, page, cached,
-					(ops->mode == MTD_OPS_RAW));
+
+		ret = nand_write_page(mtd, chip, column, bytes, wbuf,
+				      oob_required, page, cached,
+				      (ops->mode == MTD_OPS_RAW));
 		if (ret)
 			break;
 
@@ -4663,9 +4664,6 @@ int nand_scan_tail(struct mtd_info *mtd)
 		}
 	}
 
-	if (!chip->write_page)
-		chip->write_page = nand_write_page;
-
 	/*
 	 * Check ECC mode, default to software if 3byte/512byte hardware ECC is
 	 * selected and we have 256 byte pagesize fallback to software ECC
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index 9591e0fbe5bd..37f02cbb7a06 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -822,7 +822,6 @@ nand_get_sdr_timings(const struct nand_data_interface *conf)
  * @errstat:		[OPTIONAL] hardware specific function to perform
  *			additional error status checks (determine if errors are
  *			correctable).
- * @write_page:		[REPLACEABLE] High-level page write function
  */
 
 struct nand_chip {
@@ -847,9 +846,6 @@ struct nand_chip {
 	int (*scan_bbt)(struct mtd_info *mtd);
 	int (*errstat)(struct mtd_info *mtd, struct nand_chip *this, int state,
 			int status, int page);
-	int (*write_page)(struct mtd_info *mtd, struct nand_chip *chip,
-			uint32_t offset, int data_len, const uint8_t *buf,
-			int oob_required, int page, int cached, int raw);
 	int (*onfi_set_features)(struct mtd_info *mtd, struct nand_chip *chip,
 			int feature_addr, uint8_t *subfeature_para);
 	int (*onfi_get_features)(struct mtd_info *mtd, struct nand_chip *chip,
-- 
2.7.4

[PATCH v3 3/3] mtd: nand: Remove unused chip->write_page() hook

[Masahiro Yamada]

2017-03-16 17:02 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> The last/only user of the chip->write_page() hook (the Atmel NAND
> controller driver) has been reworked and is no longer specifying a custom
> ->write_page() implementation.
> Drop this hook before someone else start abusing it.
>
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>

Reviewed-by: Masahiro Yamada <yamada.masahiro@socionext.com>


-- 
Best Regards
Masahiro Yamada

[PATCH v3 0/3] mtd: nand: Rework/cleanup the Atmel NAND driver

[Boris Brezillon]

On Thu, 16 Mar 2017 09:02:39 +0100
Boris Brezillon <boris.brezillon@free-electrons.com> wrote:

> This is a complete rewrite of the driver whose main purpose is to
> support the new DT representation where the NAND controller node is now
> really visible in the DT and appears under the EBI bus. With this new
> representation, we can add other devices under the EBI bus without
> risking pinmuxing conflicts (the NAND controller is under the EBI
> bus logic and as such, share some of its pins with other devices
> connected on this bus).
> 
> Even though the goal of this rework was not necessarily to add new
> features, the new driver has been designed with this in mind. With a
> clearer separation between the different blocks and different IP
> revisions, adding new functionalities should be easier (we already
> have plans to support SMC timing configuration so that we no longer
> have to rely on the configuration done by the bootloader/bootstrap).
> 
> Also note that we no longer have a custom ->cmdfunc() implementation,
> which means we can now benefit from new features added in the core
> implementation for free (support for new NAND operations for example).
> 
> The last thing that we gain with this rework is support for multi-chips
> and multi-dies chips, thanks to the clean NAND controller <-> NAND
> devices representation.
> 
> This new driver has been tested on several platforms (at91sam9261,
> at91sam9g45, at91sam9x5, sama5d3 and sama5d4) to make sure it did not
> introduce regressions, and it's worth mentioning that old bindings are
> still supported (which partly explain the positive diffstat).

Applied the whole series.

> 
> Regards,
> 
> Boris
> 
> Changes since v2:
> - add error messages when the NFC reports an error
> - drop AVR32 compat code
> - add acks
> - minor coding style fixes
> 
> Changes since v1:
> - change function/structure prefixes (asked by Nicolas)
> - drop applied patches
> - use new GPIO helpers
> - set ->chip_delay to 40 as done in the old driver (reported by Nicolas)
> - rework read_page to improve perfs
> - add a better commit message to patch 2
> 
> Boris Brezillon (3):
>   mtd: nand: Cleanup/rework the atmel_nand driver
>   mtd: nand: atmel: Document the new DT bindings
>   mtd: nand: Remove unused chip->write_page() hook
> 
>  .../devicetree/bindings/mtd/atmel-nand.txt         |  107 +-
>  MAINTAINERS                                        |    2 +-
>  drivers/mtd/nand/Kconfig                           |    6 +-
>  drivers/mtd/nand/Makefile                          |    2 +-
>  drivers/mtd/nand/atmel/Makefile                    |    4 +
>  drivers/mtd/nand/atmel/nand-controller.c           | 2198 +++++++++++++++++
>  drivers/mtd/nand/atmel/pmecc.c                     | 1020 ++++++++
>  drivers/mtd/nand/atmel/pmecc.h                     |   73 +
>  drivers/mtd/nand/atmel_nand.c                      | 2479 --------------------
>  drivers/mtd/nand/atmel_nand_ecc.h                  |  163 --
>  drivers/mtd/nand/atmel_nand_nfc.h                  |  103 -
>  drivers/mtd/nand/nand_base.c                       |   12 +-
>  include/linux/mtd/nand.h                           |    4 -
>  13 files changed, 3411 insertions(+), 2762 deletions(-)
>  create mode 100644 drivers/mtd/nand/atmel/Makefile
>  create mode 100644 drivers/mtd/nand/atmel/nand-controller.c
>  create mode 100644 drivers/mtd/nand/atmel/pmecc.c
>  create mode 100644 drivers/mtd/nand/atmel/pmecc.h
>  delete mode 100644 drivers/mtd/nand/atmel_nand.c
>  delete mode 100644 drivers/mtd/nand/atmel_nand_ecc.h
>  delete mode 100644 drivers/mtd/nand/atmel_nand_nfc.h
> 

[PATCH v3 1/3] mtd: nand: Cleanup/rework the atmel_nand driver

[Brian Norris]

On Thu, Mar 16, 2017 at 09:02:40AM +0100, Boris Brezillon wrote:

> diff --git a/drivers/mtd/nand/atmel/nand-controller.c b/drivers/mtd/nand/atmel/nand-controller.c
> new file mode 100644
> index 000000000000..f71b9e5d7d9d
> --- /dev/null
> +++ b/drivers/mtd/nand/atmel/nand-controller.c
> @@ -0,0 +1,2198 @@
[...]

> +static int
> +atmel_hsmc_nand_controller_legacy_init(struct atmel_hsmc_nand_controller *nc)
> +{
> +	struct regmap_config regmap_conf = {
> +		.reg_bits = 32,
> +		.val_bits = 32,
> +		.reg_stride = 4,
> +		.val_bits = 32,

You assigned val_bits twice. Is that just a harmless mistake, or did you
mean to set something else?

(sparse and other tools complain about this)

> +	};


Brian

[PATCH v3 1/3] mtd: nand: Cleanup/rework the atmel_nand driver

[Boris Brezillon]

On Mon, 24 Apr 2017 18:09:30 -0700
Brian Norris <computersforpeace@gmail.com> wrote:

> On Thu, Mar 16, 2017 at 09:02:40AM +0100, Boris Brezillon wrote:
> 
> > diff --git a/drivers/mtd/nand/atmel/nand-controller.c b/drivers/mtd/nand/atmel/nand-controller.c
> > new file mode 100644
> > index 000000000000..f71b9e5d7d9d
> > --- /dev/null
> > +++ b/drivers/mtd/nand/atmel/nand-controller.c
> > @@ -0,0 +1,2198 @@  
> [...]
> 
> > +static int
> > +atmel_hsmc_nand_controller_legacy_init(struct atmel_hsmc_nand_controller *nc)
> > +{
> > +	struct regmap_config regmap_conf = {
> > +		.reg_bits = 32,
> > +		.val_bits = 32,
> > +		.reg_stride = 4,
> > +		.val_bits = 32,  
> 
> You assigned val_bits twice. Is that just a harmless mistake, or did you
> mean to set something else?

Nope, I didn't mean to set another field to 32. I'll just apply the fix
you provided.

Thanks,

Boris