[PATCH v3 00/15] pxa3xx-nand patches to support mvebu builds

[PATCH v3 00/15] pxa3xx-nand patches to support mvebu builds

[Ezequiel Garcia]

This patchset is part of the work I'm doing to enable the pxa3xx-nand
driver to support the NAND controller in Armada 370/XP. While this is
work in progress, here's a subset of patches I consider good enough
for mainline.

Since pxa3xx has a mach-specific DMA API, I'm including a few patches to
ifdef such pxa-specific code in non PXA/MMP platforms.
This is only an ugly workaround and should be removed once the specific
DMA API is replaced by dmaengine.

Daniel Mack is already working on that, so it's expected this removal
will happen soon.

Other than that, the rest of the patchset is just assorted cleanups
and minor fixes.

This patchset is based in today's Artem's l2-mtd master branch.

This series has been tested by Daniel Mack in pxa3xx boards
and no regressions have been reported (thanks Daniel!).

If at all possible and if no regressions are reported I'd like to see
this queued for v3.12.

Changes from v2:

  * Remove ifdef CONFIG_OF from the driver as suggested by Brian Norris.

  * Rework patch 01/15: "mtd: nand: pxa3xx: Introduce 'marvell,armada370...",
    according to the above change.

Changes from v1:

  * Use __maybe_unused instead of inline keyword for a symbol
    that's maybe unused. Suggested by Brian Norris.

  * As Daniel Mack reported, the PXA nand controller (NFCv1)
    lacks an NDBC3 register and it's access is undefined.
    This means we need a way to distinguish between the two
    controllers (NFCv1 and NFCv2).

    We introduce a new 'marvell,armada370-nand' compatible
    string to distinguish the newer controller (NFCv2).

Ezequiel Garcia (15):
  mtd: nand: pxa3xx: Remove unneeded ifdef CONFIG_OF
  mtd: nand: pxa3xx: Introduce 'marvell,armada370-nand' compatible
    string
  mtd: nand: pxa3xx: Handle ECC and DMA enable/disable properly
  mtd: nand: pxa3xx: Allow to set/clear the 'spare enable' field
  mtd: nand: pxa3xx: Support command buffer #3
  mtd: nand: pxa3xx: Use 'length override' in ONFI paramater page read
  mtd: nand: pxa3xx: Add a local loop variable
  mtd: nand: pxa3xx: Remove hardcoded mtd name
  mtd: nand: pxa3xx: Remove uneeded internal cmdset
  mtd: nand: pxa3xx: Move cached registers to info structure
  mtd: nand: pxa3xx: Make dma code dependent on dma capable platforms
  mtd: nand: pxa3xx: Add __maybe_unused keyword to enable_int()
  mtd: nand: pxa3xx: Allow devices with no dma resources
  mtd: nand: pxa3xx: Increase data buffer size
  mtd: nand: Allow to build pxa3xx_nand on Orion platforms

 drivers/mtd/nand/Kconfig                      |   2 +-
 drivers/mtd/nand/pxa3xx_nand.c                | 269 ++++++++++++++++----------
 include/linux/platform_data/mtd-nand-pxa3xx.h |  13 --
 3 files changed, 164 insertions(+), 120 deletions(-)

-- 
1.8.1.5

[PATCH v3 01/15] mtd: nand: pxa3xx: Remove unneeded ifdef CONFIG_OF

[Ezequiel Garcia]

There's no need to enclose this code within idef CONFIG_OF,
because the OF framework provides no-op stubs if CONFIG_OF=n.

Cc: devicetree@vger.kernel.org
Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 drivers/mtd/nand/pxa3xx_nand.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index 501e380..02f214d 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -1190,7 +1190,6 @@ static int pxa3xx_nand_remove(struct platform_device *pdev)
 	return 0;
 }
 
-#ifdef CONFIG_OF
 static struct of_device_id pxa3xx_nand_dt_ids[] = {
 	{ .compatible = "marvell,pxa3xx-nand" },
 	{}
@@ -1221,12 +1220,6 @@ static int pxa3xx_nand_probe_dt(struct platform_device *pdev)
 
 	return 0;
 }
-#else
-static inline int pxa3xx_nand_probe_dt(struct platform_device *pdev)
-{
-	return 0;
-}
-#endif
 
 static int pxa3xx_nand_probe(struct platform_device *pdev)
 {
-- 
1.8.1.5

[PATCH v3 01/15] mtd: nand: pxa3xx: Remove unneeded ifdef CONFIG_OF

[Ezequiel Garcia]

There's no need to enclose this code within idef CONFIG_OF,
because the OF framework provides no-op stubs if CONFIG_OF=n.

Cc: devicetree@vger.kernel.org
Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 drivers/mtd/nand/pxa3xx_nand.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index 501e380..02f214d 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -1190,7 +1190,6 @@ static int pxa3xx_nand_remove(struct platform_device *pdev)
 	return 0;
 }
 
-#ifdef CONFIG_OF
 static struct of_device_id pxa3xx_nand_dt_ids[] = {
 	{ .compatible = "marvell,pxa3xx-nand" },
 	{}
@@ -1221,12 +1220,6 @@ static int pxa3xx_nand_probe_dt(struct platform_device *pdev)
 
 	return 0;
 }
-#else
-static inline int pxa3xx_nand_probe_dt(struct platform_device *pdev)
-{
-	return 0;
-}
-#endif
 
 static int pxa3xx_nand_probe(struct platform_device *pdev)
 {
-- 
1.8.1.5

[PATCH v3 02/15] mtd: nand: pxa3xx: Introduce 'marvell, armada370-nand' compatible string

[Ezequiel Garcia]

This driver supports NFCv1 (as found in PXA SoC) and NFCv2 (as found in
Armada 370/XP SoC). As both controller has a few differences, a way of
distinguishing between the two is needed.

This commit introduces a new compatible string 'marvell,armada370-nand'
and assigns a compatible data of type enum pxa3xx_nand_variant to allow
such distinction.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
Tested-by: Daniel Mack <zonque@gmail.com>
---
 drivers/mtd/nand/pxa3xx_nand.c | 31 ++++++++++++++++++++++++++++++-
 1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index 02f214d..55eb153 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -123,6 +123,11 @@ enum {
 	STATE_READY,
 };
 
+enum pxa3xx_nand_variant {
+	PXA3XX_NAND_VARIANT_PXA,
+	PXA3XX_NAND_VARIANT_ARMADA370,
+};
+
 struct pxa3xx_nand_host {
 	struct nand_chip	chip;
 	struct pxa3xx_nand_cmdset *cmdset;
@@ -171,6 +176,12 @@ struct pxa3xx_nand_info {
 	struct pxa3xx_nand_host *host[NUM_CHIP_SELECT];
 	unsigned int		state;
 
+	/*
+	 * This driver supports NFCv1 (as found in PXA SoC)
+	 * and NFCv2 (as found in Armada 370/XP SoC).
+	 */
+	enum pxa3xx_nand_variant variant;
+
 	int			cs;
 	int			use_ecc;	/* use HW ECC ? */
 	int			use_dma;	/* use DMA ? */
@@ -1191,11 +1202,28 @@ static int pxa3xx_nand_remove(struct platform_device *pdev)
 }
 
 static struct of_device_id pxa3xx_nand_dt_ids[] = {
-	{ .compatible = "marvell,pxa3xx-nand" },
+	{
+		.compatible = "marvell,pxa3xx-nand",
+		.data       = (void *)PXA3XX_NAND_VARIANT_PXA,
+	},
+	{
+		.compatible = "marvell,armada370-nand",
+		.data       = (void *)PXA3XX_NAND_VARIANT_ARMADA370,
+	},
 	{}
 };
 MODULE_DEVICE_TABLE(of, pxa3xx_nand_dt_ids);
 
+static enum pxa3xx_nand_variant
+pxa3xx_nand_get_variant(struct platform_device *pdev)
+{
+	const struct of_device_id *of_id =
+			of_match_device(pxa3xx_nand_dt_ids, &pdev->dev);
+	if (!of_id)
+		return PXA3XX_NAND_VARIANT_PXA;
+	return (enum pxa3xx_nand_variant)of_id->data;
+}
+
 static int pxa3xx_nand_probe_dt(struct platform_device *pdev)
 {
 	struct pxa3xx_nand_platform_data *pdata;
@@ -1245,6 +1273,7 @@ static int pxa3xx_nand_probe(struct platform_device *pdev)
 	}
 
 	info = platform_get_drvdata(pdev);
+	info->variant = pxa3xx_nand_get_variant(pdev);
 	probe_success = 0;
 	for (cs = 0; cs < pdata->num_cs; cs++) {
 		info->cs = cs;
-- 
1.8.1.5

[PATCH v3 03/15] mtd: nand: pxa3xx: Handle ECC and DMA enable/disable properly

[Ezequiel Garcia]

When ECC is not selected, the ECC enable bit must be cleared
in the NAND control register. Same applies to DMA.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
Tested-by: Daniel Mack <zonque@gmail.com>
---
 drivers/mtd/nand/pxa3xx_nand.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index 55eb153..dfff327 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -314,8 +314,17 @@ static void pxa3xx_nand_start(struct pxa3xx_nand_info *info)
 	uint32_t ndcr;
 
 	ndcr = host->reg_ndcr;
-	ndcr |= info->use_ecc ? NDCR_ECC_EN : 0;
-	ndcr |= info->use_dma ? NDCR_DMA_EN : 0;
+
+	if (info->use_ecc)
+		ndcr |= NDCR_ECC_EN;
+	else
+		ndcr &= ~NDCR_ECC_EN;
+
+	if (info->use_dma)
+		ndcr |= NDCR_DMA_EN;
+	else
+		ndcr &= ~NDCR_DMA_EN;
+
 	ndcr |= NDCR_ND_RUN;
 
 	/* clear status bits and run */
-- 
1.8.1.5

[PATCH v3 04/15] mtd: nand: pxa3xx: Allow to set/clear the 'spare enable' field

[Ezequiel Garcia]

Some commands (such as the ONFI parameter page read) need to
clear the 'spare enable' bit. This commit allows to set/clear
depending on the prepared command, instead of having it always
set.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
Tested-by: Daniel Mack <zonque@gmail.com>
---
 drivers/mtd/nand/pxa3xx_nand.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index dfff327..3a3e042 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -185,6 +185,7 @@ struct pxa3xx_nand_info {
 	int			cs;
 	int			use_ecc;	/* use HW ECC ? */
 	int			use_dma;	/* use DMA ? */
+	int			use_spare;	/* use spare ? */
 	int			is_ready;
 
 	unsigned int		page_size;	/* page size of attached chip */
@@ -325,6 +326,11 @@ static void pxa3xx_nand_start(struct pxa3xx_nand_info *info)
 	else
 		ndcr &= ~NDCR_DMA_EN;
 
+	if (info->use_spare)
+		ndcr |= NDCR_SPARE_EN;
+	else
+		ndcr &= ~NDCR_SPARE_EN;
+
 	ndcr |= NDCR_ND_RUN;
 
 	/* clear status bits and run */
@@ -526,6 +532,7 @@ static int prepare_command_pool(struct pxa3xx_nand_info *info, int command,
 	info->buf_count		= 0;
 	info->oob_size		= 0;
 	info->use_ecc		= 0;
+	info->use_spare		= 1;
 	info->use_dma		= (use_dma) ? 1 : 0;
 	info->is_ready		= 0;
 	info->retcode		= ERR_NONE;
-- 
1.8.1.5

[PATCH v3 05/15] mtd: nand: pxa3xx: Support command buffer #3

[Ezequiel Garcia]

Some newer controllers support a fourth command buffer. This additional
command buffer allows to set an arbitrary length count, using the
NDCB3.NDLENCNT field, to perform non-standard length operations
such as the ONFI parameter page read.

In controllers without this register, the operation has no effect.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
Tested-by: Daniel Mack <zonque@gmail.com>
---
 drivers/mtd/nand/pxa3xx_nand.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index 3a3e042..83a7187 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -197,6 +197,7 @@ struct pxa3xx_nand_info {
 	uint32_t		ndcb0;
 	uint32_t		ndcb1;
 	uint32_t		ndcb2;
+	uint32_t		ndcb3;
 };
 
 static bool use_dma = 1;
@@ -496,6 +497,10 @@ static irqreturn_t pxa3xx_nand_irq(int irq, void *devid)
 		nand_writel(info, NDCB0, info->ndcb0);
 		nand_writel(info, NDCB0, info->ndcb1);
 		nand_writel(info, NDCB0, info->ndcb2);
+
+		/* NDCB3 register is available in NFCv2 (Armada 370/XP SoC) */
+		if (info->variant == PXA3XX_NAND_VARIANT_ARMADA370)
+			nand_writel(info, NDCB0, info->ndcb3);
 	}
 
 	/* clear NDSR to let the controller exit the IRQ */
@@ -554,6 +559,7 @@ static int prepare_command_pool(struct pxa3xx_nand_info *info, int command,
 	default:
 		info->ndcb1 = 0;
 		info->ndcb2 = 0;
+		info->ndcb3 = 0;
 		break;
 	}
 
-- 
1.8.1.5

[PATCH v3 06/15] mtd: nand: pxa3xx: Use 'length override' in ONFI paramater page read

[Ezequiel Garcia]

The ONFI command 'parameter page read' needs a non-standard length.
Therefore, we enable the 'length override' field in NDCB0 and set
a non-zero 'length count' in NDCB3.

Additionally, the 'spare enable' bit must be disabled for any command
that sets a non-zero 'length count' in NDCB3.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
Tested-by: Daniel Mack <zonque@gmail.com>
---
 drivers/mtd/nand/pxa3xx_nand.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index 83a7187..30630b2 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -80,6 +80,7 @@
 #define NDSR_RDDREQ		(0x1 << 1)
 #define NDSR_WRCMDREQ		(0x1)
 
+#define NDCB0_LEN_OVRD		(0x1 << 28)
 #define NDCB0_ST_ROW_EN         (0x1 << 26)
 #define NDCB0_AUTO_RS		(0x1 << 25)
 #define NDCB0_CSEL		(0x1 << 24)
@@ -553,6 +554,9 @@ static int prepare_command_pool(struct pxa3xx_nand_info *info, int command,
 	case NAND_CMD_READOOB:
 		pxa3xx_set_datasize(info);
 		break;
+	case NAND_CMD_PARAM:
+		info->use_spare = 0;
+		break;
 	case NAND_CMD_SEQIN:
 		exec_cmd = 0;
 		break;
@@ -628,8 +632,10 @@ static int prepare_command_pool(struct pxa3xx_nand_info *info, int command,
 		info->buf_count = 256;
 		info->ndcb0 |= NDCB0_CMD_TYPE(0)
 				| NDCB0_ADDR_CYC(1)
+				| NDCB0_LEN_OVRD
 				| cmd;
 		info->ndcb1 = (column & 0xFF);
+		info->ndcb3 = 256;
 		info->data_size = 256;
 		break;
 
-- 
1.8.1.5

[PATCH v3 07/15] mtd: nand: pxa3xx: Add a local loop variable

[Ezequiel Garcia]

This is just a cosmetic change, to make the code more readable.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
Tested-by: Daniel Mack <zonque@gmail.com>
---
 drivers/mtd/nand/pxa3xx_nand.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index 30630b2..b34fa25 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -1304,8 +1304,9 @@ static int pxa3xx_nand_probe(struct platform_device *pdev)
 	info->variant = pxa3xx_nand_get_variant(pdev);
 	probe_success = 0;
 	for (cs = 0; cs < pdata->num_cs; cs++) {
+		struct mtd_info *mtd = info->host[cs]->mtd;
 		info->cs = cs;
-		ret = pxa3xx_nand_scan(info->host[cs]->mtd);
+		ret = pxa3xx_nand_scan(mtd);
 		if (ret) {
 			dev_warn(&pdev->dev, "failed to scan nand at cs %d\n",
 				cs);
@@ -1313,7 +1314,7 @@ static int pxa3xx_nand_probe(struct platform_device *pdev)
 		}
 
 		ppdata.of_node = pdev->dev.of_node;
-		ret = mtd_device_parse_register(info->host[cs]->mtd, NULL,
+		ret = mtd_device_parse_register(mtd, NULL,
 						&ppdata, pdata->parts[cs],
 						pdata->nr_parts[cs]);
 		if (!ret)
-- 
1.8.1.5

[PATCH v3 08/15] mtd: nand: pxa3xx: Remove hardcoded mtd name

[Ezequiel Garcia]

There's no advantage in using a hardcoded name for the mtd device.
Instead use the provided by the platform_device.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
Tested-by: Daniel Mack <zonque@gmail.com>
---
 drivers/mtd/nand/pxa3xx_nand.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index b34fa25..f59c687 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -244,8 +244,6 @@ static struct pxa3xx_nand_flash builtin_flash_types[] = {
 /* Define a default flash type setting serve as flash detecting only */
 #define DEFAULT_FLASH_TYPE (&builtin_flash_types[0])
 
-const char *mtd_names[] = {"pxa3xx_nand-0", "pxa3xx_nand-1", NULL};
-
 #define NDTR0_tCH(c)	(min((c), 7) << 19)
 #define NDTR0_tCS(c)	(min((c), 7) << 16)
 #define NDTR0_tWH(c)	(min((c), 7) << 11)
@@ -1082,8 +1080,6 @@ KEEP_CONFIG:
 		host->row_addr_cycles = 3;
 	else
 		host->row_addr_cycles = 2;
-
-	mtd->name = mtd_names[0];
 	return nand_scan_tail(mtd);
 }
 
@@ -1305,6 +1301,8 @@ static int pxa3xx_nand_probe(struct platform_device *pdev)
 	probe_success = 0;
 	for (cs = 0; cs < pdata->num_cs; cs++) {
 		struct mtd_info *mtd = info->host[cs]->mtd;
+
+		mtd->name = pdev->name;
 		info->cs = cs;
 		ret = pxa3xx_nand_scan(mtd);
 		if (ret) {
-- 
1.8.1.5

[PATCH v3 09/15] mtd: nand: pxa3xx: Remove uneeded internal cmdset

[Ezequiel Garcia]

Use the defined macros for NAND command instead of using a constant
internal structure. This commit is only a cleanup, there's no
functionality modification.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
Tested-by: Daniel Mack <zonque@gmail.com>
---
 drivers/mtd/nand/pxa3xx_nand.c                | 63 ++++++++-------------------
 include/linux/platform_data/mtd-nand-pxa3xx.h | 13 ------
 2 files changed, 17 insertions(+), 59 deletions(-)

diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index f59c687..47a8641 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -131,7 +131,6 @@ enum pxa3xx_nand_variant {
 
 struct pxa3xx_nand_host {
 	struct nand_chip	chip;
-	struct pxa3xx_nand_cmdset *cmdset;
 	struct mtd_info         *mtd;
 	void			*info_data;
 
@@ -205,23 +204,6 @@ static bool use_dma = 1;
 module_param(use_dma, bool, 0444);
 MODULE_PARM_DESC(use_dma, "enable DMA for data transferring to/from NAND HW");
 
-/*
- * Default NAND flash controller configuration setup by the
- * bootloader. This configuration is used only when pdata->keep_config is set
- */
-static struct pxa3xx_nand_cmdset default_cmdset = {
-	.read1		= 0x3000,
-	.read2		= 0x0050,
-	.program	= 0x1080,
-	.read_status	= 0x0070,
-	.read_id	= 0x0090,
-	.erase		= 0xD060,
-	.reset		= 0x00FF,
-	.lock		= 0x002A,
-	.unlock		= 0x2423,
-	.lock_status	= 0x007A,
-};
-
 static struct pxa3xx_nand_timing timing[] = {
 	{ 40, 80, 60, 100, 80, 100, 90000, 400, 40, },
 	{ 10,  0, 20,  40, 30,  40, 11123, 110, 10, },
@@ -521,7 +503,6 @@ static inline int is_buf_blank(uint8_t *buf, size_t len)
 static int prepare_command_pool(struct pxa3xx_nand_info *info, int command,
 		uint16_t column, int page_addr)
 {
-	uint16_t cmd;
 	int addr_cycle, exec_cmd;
 	struct pxa3xx_nand_host *host;
 	struct mtd_info *mtd;
@@ -571,21 +552,17 @@ static int prepare_command_pool(struct pxa3xx_nand_info *info, int command,
 	switch (command) {
 	case NAND_CMD_READOOB:
 	case NAND_CMD_READ0:
-		cmd = host->cmdset->read1;
+		info->buf_start = column;
+		info->ndcb0 |= NDCB0_CMD_TYPE(0)
+				| addr_cycle
+				| NAND_CMD_READ0;
+
 		if (command == NAND_CMD_READOOB)
-			info->buf_start = mtd->writesize + column;
-		else
-			info->buf_start = column;
+			info->buf_start += mtd->writesize;
 
-		if (unlikely(host->page_size < PAGE_CHUNK_SIZE))
-			info->ndcb0 |= NDCB0_CMD_TYPE(0)
-					| addr_cycle
-					| (cmd & NDCB0_CMD1_MASK);
-		else
-			info->ndcb0 |= NDCB0_CMD_TYPE(0)
-					| NDCB0_DBC
-					| addr_cycle
-					| cmd;
+		/* Second command setting for large pages */
+		if (host->page_size >= PAGE_CHUNK_SIZE)
+			info->ndcb0 |= NDCB0_DBC | (NAND_CMD_READSTART << 8);
 
 	case NAND_CMD_SEQIN:
 		/* small page addr setting */
@@ -616,62 +593,58 @@ static int prepare_command_pool(struct pxa3xx_nand_info *info, int command,
 			break;
 		}
 
-		cmd = host->cmdset->program;
 		info->ndcb0 |= NDCB0_CMD_TYPE(0x1)
 				| NDCB0_AUTO_RS
 				| NDCB0_ST_ROW_EN
 				| NDCB0_DBC
-				| cmd
+				| (NAND_CMD_PAGEPROG << 8)
+				| NAND_CMD_SEQIN
 				| addr_cycle;
 		break;
 
 	case NAND_CMD_PARAM:
-		cmd = NAND_CMD_PARAM;
 		info->buf_count = 256;
 		info->ndcb0 |= NDCB0_CMD_TYPE(0)
 				| NDCB0_ADDR_CYC(1)
 				| NDCB0_LEN_OVRD
-				| cmd;
+				| command;
 		info->ndcb1 = (column & 0xFF);
 		info->ndcb3 = 256;
 		info->data_size = 256;
 		break;
 
 	case NAND_CMD_READID:
-		cmd = host->cmdset->read_id;
 		info->buf_count = host->read_id_bytes;
 		info->ndcb0 |= NDCB0_CMD_TYPE(3)
 				| NDCB0_ADDR_CYC(1)
-				| cmd;
+				| command;
 		info->ndcb1 = (column & 0xFF);
 
 		info->data_size = 8;
 		break;
 	case NAND_CMD_STATUS:
-		cmd = host->cmdset->read_status;
 		info->buf_count = 1;
 		info->ndcb0 |= NDCB0_CMD_TYPE(4)
 				| NDCB0_ADDR_CYC(1)
-				| cmd;
+				| command;
 
 		info->data_size = 8;
 		break;
 
 	case NAND_CMD_ERASE1:
-		cmd = host->cmdset->erase;
 		info->ndcb0 |= NDCB0_CMD_TYPE(2)
 				| NDCB0_AUTO_RS
 				| NDCB0_ADDR_CYC(3)
 				| NDCB0_DBC
-				| cmd;
+				| (NAND_CMD_ERASE2 << 8)
+				| NAND_CMD_ERASE1;
 		info->ndcb1 = page_addr;
 		info->ndcb2 = 0;
 
 		break;
 	case NAND_CMD_RESET:
-		cmd = host->cmdset->reset;
 		info->ndcb0 |= NDCB0_CMD_TYPE(5)
-				| cmd;
+				| command;
 
 		break;
 
@@ -867,7 +840,6 @@ static int pxa3xx_nand_config_flash(struct pxa3xx_nand_info *info,
 	}
 
 	/* calculate flash information */
-	host->cmdset = &default_cmdset;
 	host->page_size = f->page_size;
 	host->read_id_bytes = (f->page_size == 2048) ? 4 : 2;
 
@@ -913,7 +885,6 @@ static int pxa3xx_nand_detect_config(struct pxa3xx_nand_info *info)
 	}
 
 	host->reg_ndcr = ndcr & ~NDCR_INT_MASK;
-	host->cmdset = &default_cmdset;
 
 	host->ndtr0cs0 = nand_readl(info, NDTR0CS0);
 	host->ndtr1cs0 = nand_readl(info, NDTR1CS0);
diff --git a/include/linux/platform_data/mtd-nand-pxa3xx.h b/include/linux/platform_data/mtd-nand-pxa3xx.h
index c42f39f..ffb8019 100644
--- a/include/linux/platform_data/mtd-nand-pxa3xx.h
+++ b/include/linux/platform_data/mtd-nand-pxa3xx.h
@@ -16,19 +16,6 @@ struct pxa3xx_nand_timing {
 	unsigned int	tAR;  /* ND_ALE low to ND_nRE low delay */
 };
 
-struct pxa3xx_nand_cmdset {
-	uint16_t	read1;
-	uint16_t	read2;
-	uint16_t	program;
-	uint16_t	read_status;
-	uint16_t	read_id;
-	uint16_t	erase;
-	uint16_t	reset;
-	uint16_t	lock;
-	uint16_t	unlock;
-	uint16_t	lock_status;
-};
-
 struct pxa3xx_nand_flash {
 	char		*name;
 	uint32_t	chip_id;
-- 
1.8.1.5

[PATCH v3 10/15] mtd: nand: pxa3xx: Move cached registers to info structure

[Ezequiel Garcia]

This registers are not per-chip (aka host) but controller-wise,
so it's better to store them in the global 'info' structure.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
Tested-by: Daniel Mack <zonque@gmail.com>
---
 drivers/mtd/nand/pxa3xx_nand.c | 36 +++++++++++++++++-------------------
 1 file changed, 17 insertions(+), 19 deletions(-)

diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index 47a8641..ae441ff 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -144,10 +144,6 @@ struct pxa3xx_nand_host {
 	unsigned int		row_addr_cycles;
 	size_t			read_id_bytes;
 
-	/* cached register value */
-	uint32_t		reg_ndcr;
-	uint32_t		ndtr0cs0;
-	uint32_t		ndtr1cs0;
 };
 
 struct pxa3xx_nand_info {
@@ -193,6 +189,11 @@ struct pxa3xx_nand_info {
 	unsigned int		oob_size;
 	int 			retcode;
 
+	/* cached register value */
+	uint32_t		reg_ndcr;
+	uint32_t		ndtr0cs0;
+	uint32_t		ndtr1cs0;
+
 	/* generated NDCBx register values */
 	uint32_t		ndcb0;
 	uint32_t		ndcb1;
@@ -258,8 +259,8 @@ static void pxa3xx_nand_set_timing(struct pxa3xx_nand_host *host,
 		NDTR1_tWHR(ns2cycle(t->tWHR, nand_clk)) |
 		NDTR1_tAR(ns2cycle(t->tAR, nand_clk));
 
-	host->ndtr0cs0 = ndtr0;
-	host->ndtr1cs0 = ndtr1;
+	info->ndtr0cs0 = ndtr0;
+	info->ndtr1cs0 = ndtr1;
 	nand_writel(info, NDTR0CS0, ndtr0);
 	nand_writel(info, NDTR1CS0, ndtr1);
 }
@@ -267,7 +268,7 @@ static void pxa3xx_nand_set_timing(struct pxa3xx_nand_host *host,
 static void pxa3xx_set_datasize(struct pxa3xx_nand_info *info)
 {
 	struct pxa3xx_nand_host *host = info->host[info->cs];
-	int oob_enable = host->reg_ndcr & NDCR_SPARE_EN;
+	int oob_enable = info->reg_ndcr & NDCR_SPARE_EN;
 
 	info->data_size = host->page_size;
 	if (!oob_enable) {
@@ -293,10 +294,9 @@ static void pxa3xx_set_datasize(struct pxa3xx_nand_info *info)
  */
 static void pxa3xx_nand_start(struct pxa3xx_nand_info *info)
 {
-	struct pxa3xx_nand_host *host = info->host[info->cs];
 	uint32_t ndcr;
 
-	ndcr = host->reg_ndcr;
+	ndcr = info->reg_ndcr;
 
 	if (info->use_ecc)
 		ndcr |= NDCR_ECC_EN;
@@ -674,7 +674,7 @@ static void pxa3xx_nand_cmdfunc(struct mtd_info *mtd, unsigned command,
 	 * "byte" address into a "word" address appropriate
 	 * for indexing a word-oriented device
 	 */
-	if (host->reg_ndcr & NDCR_DWIDTH_M)
+	if (info->reg_ndcr & NDCR_DWIDTH_M)
 		column /= 2;
 
 	/*
@@ -684,8 +684,8 @@ static void pxa3xx_nand_cmdfunc(struct mtd_info *mtd, unsigned command,
 	 */
 	if (info->cs != host->cs) {
 		info->cs = host->cs;
-		nand_writel(info, NDTR0CS0, host->ndtr0cs0);
-		nand_writel(info, NDTR1CS0, host->ndtr1cs0);
+		nand_writel(info, NDTR0CS0, info->ndtr0cs0);
+		nand_writel(info, NDTR1CS0, info->ndtr1cs0);
 	}
 
 	info->state = STATE_PREPARED;
@@ -861,7 +861,7 @@ static int pxa3xx_nand_config_flash(struct pxa3xx_nand_info *info,
 	ndcr |= NDCR_RD_ID_CNT(host->read_id_bytes);
 	ndcr |= NDCR_SPARE_EN; /* enable spare by default */
 
-	host->reg_ndcr = ndcr;
+	info->reg_ndcr = ndcr;
 
 	pxa3xx_nand_set_timing(host, f->timing);
 	return 0;
@@ -884,11 +884,9 @@ static int pxa3xx_nand_detect_config(struct pxa3xx_nand_info *info)
 		host->read_id_bytes = 2;
 	}
 
-	host->reg_ndcr = ndcr & ~NDCR_INT_MASK;
-
-	host->ndtr0cs0 = nand_readl(info, NDTR0CS0);
-	host->ndtr1cs0 = nand_readl(info, NDTR1CS0);
-
+	info->reg_ndcr = ndcr & ~NDCR_INT_MASK;
+	info->ndtr0cs0 = nand_readl(info, NDTR0CS0);
+	info->ndtr1cs0 = nand_readl(info, NDTR1CS0);
 	return 0;
 }
 
@@ -1035,7 +1033,7 @@ KEEP_CONFIG:
 	chip->ecc.size = host->page_size;
 	chip->ecc.strength = 1;
 
-	if (host->reg_ndcr & NDCR_DWIDTH_M)
+	if (info->reg_ndcr & NDCR_DWIDTH_M)
 		chip->options |= NAND_BUSWIDTH_16;
 
 	if (nand_scan_ident(mtd, 1, def))
-- 
1.8.1.5

[PATCH v3 11/15] mtd: nand: pxa3xx: Make dma code dependent on dma capable platforms

[Ezequiel Garcia]

This patch adds a macro ARCH_HAS_DMA to compile-out arch specific
dma code, namely pxa_request_dma() and pxa_free_dma(). These symbols
are available only in pxa, which makes impossible to build the driver in
other platforms than ARCH_PXA.

In order to handle non-dma capable platforms, we implement a fallbacks that
allocate buffers as if 'use_dma=false', putting the dma related code
under the ARCH_HAS_DMA conditional.

Please note that the correct way to handle this is to migrate the
dma code to use of the mmp_pdma dmaengine driver. However, currently
this is not possible because the two dmaengine drivers can't work together.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
Tested-by: Daniel Mack <zonque@gmail.com>
---
 drivers/mtd/nand/pxa3xx_nand.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index ae441ff..d8c133e 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -25,7 +25,14 @@
 #include <linux/of.h>
 #include <linux/of_device.h>
 
+#if defined(CONFIG_ARCH_PXA) || defined(CONFIG_ARCH_MMP)
+#define ARCH_HAS_DMA
+#endif
+
+#ifdef ARCH_HAS_DMA
 #include <mach/dma.h>
+#endif
+
 #include <linux/platform_data/mtd-nand-pxa3xx.h>
 
 #define	CHIP_DELAY_TIMEOUT	(2 * HZ/10)
@@ -381,6 +388,7 @@ static void handle_data_pio(struct pxa3xx_nand_info *info)
 	}
 }
 
+#ifdef ARCH_HAS_DMA
 static void start_data_dma(struct pxa3xx_nand_info *info)
 {
 	struct pxa_dma_desc *desc = info->data_desc;
@@ -427,6 +435,10 @@ static void pxa3xx_nand_data_dma_irq(int channel, void *data)
 	enable_int(info, NDCR_INT_MASK);
 	nand_writel(info, NDSR, NDSR_WRDREQ | NDSR_RDDREQ);
 }
+#else
+static void start_data_dma(struct pxa3xx_nand_info *info)
+{}
+#endif
 
 static irqreturn_t pxa3xx_nand_irq(int irq, void *devid)
 {
@@ -896,6 +908,7 @@ static int pxa3xx_nand_detect_config(struct pxa3xx_nand_info *info)
  */
 #define MAX_BUFF_SIZE	PAGE_SIZE
 
+#ifdef ARCH_HAS_DMA
 static int pxa3xx_nand_init_buff(struct pxa3xx_nand_info *info)
 {
 	struct platform_device *pdev = info->pdev;
@@ -941,6 +954,20 @@ static void pxa3xx_nand_free_buff(struct pxa3xx_nand_info *info)
 		kfree(info->data_buff);
 	}
 }
+#else
+static int pxa3xx_nand_init_buff(struct pxa3xx_nand_info *info)
+{
+	info->data_buff = kmalloc(MAX_BUFF_SIZE, GFP_KERNEL);
+	if (info->data_buff == NULL)
+		return -ENOMEM;
+	return 0;
+}
+
+static void pxa3xx_nand_free_buff(struct pxa3xx_nand_info *info)
+{
+	kfree(info->data_buff);
+}
+#endif
 
 static int pxa3xx_nand_sensing(struct pxa3xx_nand_info *info)
 {
@@ -1249,6 +1276,13 @@ static int pxa3xx_nand_probe(struct platform_device *pdev)
 	struct pxa3xx_nand_info *info;
 	int ret, cs, probe_success;
 
+#ifndef ARCH_HAS_DMA
+	if (use_dma) {
+		use_dma = 0;
+		dev_warn(&pdev->dev,
+			 "This platform can't do DMA on this device\n");
+	}
+#endif
 	ret = pxa3xx_nand_probe_dt(pdev);
 	if (ret)
 		return ret;
-- 
1.8.1.5

[PATCH v3 12/15] mtd: nand: pxa3xx: Add __maybe_unused keyword to enable_int()

[Ezequiel Garcia]

Now that we have added ARCH_HAS_DMA conditional the function
enable_int() may be unused. Declare it as __maybe_unused,
in order to remove the following warning, when the function is not used:

drivers/mtd/nand//pxa3xx_nand.c:343:24: warning: 'enable_int' defined
but not used [-Wunused-function]

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
Tested-by: Daniel Mack <zonque@gmail.com>
---
 drivers/mtd/nand/pxa3xx_nand.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index d8c133e..dca1747 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -348,7 +348,8 @@ static void pxa3xx_nand_stop(struct pxa3xx_nand_info *info)
 	nand_writel(info, NDSR, NDSR_MASK);
 }
 
-static void enable_int(struct pxa3xx_nand_info *info, uint32_t int_mask)
+static void __maybe_unused
+enable_int(struct pxa3xx_nand_info *info, uint32_t int_mask)
 {
 	uint32_t ndcr;
 
-- 
1.8.1.5

[PATCH v3 13/15] mtd: nand: pxa3xx: Allow devices with no dma resources

[Ezequiel Garcia]

When use_dma=0 there's no point in requesting resources for dma,
since they won't be used anyway. Therefore we remove that requirement,
therefore allowing devices without dma to pass the driver probe.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
Tested-by: Daniel Mack <zonque@gmail.com>
---
 drivers/mtd/nand/pxa3xx_nand.c | 51 +++++++++++++++++++++++-------------------
 1 file changed, 28 insertions(+), 23 deletions(-)

diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index 0f94c81..3c1d061 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -1131,30 +1131,35 @@ static int alloc_nand_resource(struct platform_device *pdev)
 	if (ret < 0)
 		return ret;
 
-	/*
-	 * This is a dirty hack to make this driver work from devicetree
-	 * bindings. It can be removed once we have a prober DMA controller
-	 * framework for DT.
-	 */
-	if (pdev->dev.of_node && of_machine_is_compatible("marvell,pxa3xx")) {
-		info->drcmr_dat = 97;
-		info->drcmr_cmd = 99;
-	} else {
-		r = platform_get_resource(pdev, IORESOURCE_DMA, 0);
-		if (r == NULL) {
-			dev_err(&pdev->dev, "no resource defined for data DMA\n");
-			ret = -ENXIO;
-			goto fail_disable_clk;
-		}
-		info->drcmr_dat = r->start;
-
-		r = platform_get_resource(pdev, IORESOURCE_DMA, 1);
-		if (r == NULL) {
-			dev_err(&pdev->dev, "no resource defined for command DMA\n");
-			ret = -ENXIO;
-			goto fail_disable_clk;
+	if (use_dma) {
+		/*
+		 * This is a dirty hack to make this driver work from
+		 * devicetree bindings. It can be removed once we have
+		 * a prober DMA controller framework for DT.
+		 */
+		if (pdev->dev.of_node &&
+		    of_machine_is_compatible("marvell,pxa3xx")) {
+			info->drcmr_dat = 97;
+			info->drcmr_cmd = 99;
+		} else {
+			r = platform_get_resource(pdev, IORESOURCE_DMA, 0);
+			if (r == NULL) {
+				dev_err(&pdev->dev,
+					"no resource defined for data DMA\n");
+				ret = -ENXIO;
+				goto fail_disable_clk;
+			}
+			info->drcmr_dat = r->start;
+
+			r = platform_get_resource(pdev, IORESOURCE_DMA, 1);
+			if (r == NULL) {
+				dev_err(&pdev->dev,
+					"no resource defined for cmd DMA\n");
+				ret = -ENXIO;
+				goto fail_disable_clk;
+			}
+			info->drcmr_cmd = r->start;
 		}
-		info->drcmr_cmd = r->start;
 	}
 
 	irq = platform_get_irq(pdev, 0);
-- 
1.8.1.5

[PATCH v3 14/15] mtd: nand: pxa3xx: Increase data buffer size

[Ezequiel Garcia]

Devices with 4 KiB pages (plus OOB data) can be detected, so we increase
the data buffer size. A better solution would be to allocate a buffer
depending on the detected page size, but that's not possible given
we need the buffer prior to the device detection.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
Tested-by: Daniel Mack <zonque@gmail.com>
---
 drivers/mtd/nand/pxa3xx_nand.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index 3c1d061..49c4f92 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -906,7 +906,7 @@ static int pxa3xx_nand_detect_config(struct pxa3xx_nand_info *info)
  * is: 2048 + 64 = 2112 bytes, allocate a page here for both the
  * data buffer and the DMA descriptor
  */
-#define MAX_BUFF_SIZE	PAGE_SIZE
+#define MAX_BUFF_SIZE	(PAGE_SIZE*2)
 
 #ifdef ARCH_HAS_DMA
 static int pxa3xx_nand_init_buff(struct pxa3xx_nand_info *info)
-- 
1.8.1.5

[PATCH v3 15/15] mtd: nand: Allow to build pxa3xx_nand on Orion platforms

[Ezequiel Garcia]

The Armada 370 and Armada XP SoC families, selected by PLAT_ORION,
have a Nand Flash Controller (NFC) IP very similar to the one present
in PXA platforms. Therefore, we want to build this driver on PLAT_ORION.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
Tested-by: Daniel Mack <zonque@gmail.com>
---
 drivers/mtd/nand/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
index 5ef8f5e..86c135b 100644
--- a/drivers/mtd/nand/Kconfig
+++ b/drivers/mtd/nand/Kconfig
@@ -354,7 +354,7 @@ config MTD_NAND_ATMEL
 
 config MTD_NAND_PXA3xx
 	tristate "Support for NAND flash devices on PXA3xx"
-	depends on PXA3xx || ARCH_MMP
+	depends on PXA3xx || ARCH_MMP || PLAT_ORION
 	help
 	  This enables the driver for the NAND flash device found on
 	  PXA3xx processors
-- 
1.8.1.5

Re: [PATCH v3 05/15] mtd: nand: pxa3xx: Support command buffer #3

[Brian Norris]

On Sat, Aug 10, 2013 at 04:34:55PM -0300, Ezequiel Garcia wrote:
> Some newer controllers support a fourth command buffer. This additional
> command buffer allows to set an arbitrary length count, using the
> NDCB3.NDLENCNT field, to perform non-standard length operations
> such as the ONFI parameter page read.
> 
> In controllers without this register, the operation has no effect.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> Tested-by: Daniel Mack <zonque@gmail.com>
> ---
>  drivers/mtd/nand/pxa3xx_nand.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
> index 3a3e042..83a7187 100644
> --- a/drivers/mtd/nand/pxa3xx_nand.c
> +++ b/drivers/mtd/nand/pxa3xx_nand.c
> @@ -197,6 +197,7 @@ struct pxa3xx_nand_info {
>  	uint32_t		ndcb0;
>  	uint32_t		ndcb1;
>  	uint32_t		ndcb2;
> +	uint32_t		ndcb3;
>  };
>  
>  static bool use_dma = 1;
> @@ -496,6 +497,10 @@ static irqreturn_t pxa3xx_nand_irq(int irq, void *devid)
>  		nand_writel(info, NDCB0, info->ndcb0);
>  		nand_writel(info, NDCB0, info->ndcb1);
>  		nand_writel(info, NDCB0, info->ndcb2);
> +
> +		/* NDCB3 register is available in NFCv2 (Armada 370/XP SoC) */
> +		if (info->variant == PXA3XX_NAND_VARIANT_ARMADA370)
> +			nand_writel(info, NDCB0, info->ndcb3);

Pardon my unfamiliarity with your hardware, but why do you have the
following three register definitions if you're only using NDCB0?

#define NDCB0           (0x48) /* Command Buffer0 */
#define NDCB1           (0x4C) /* Command Buffer1 */
#define NDCB2           (0x50) /* Command Buffer2 */

>  	}
>  
>  	/* clear NDSR to let the controller exit the IRQ */
> @@ -554,6 +559,7 @@ static int prepare_command_pool(struct pxa3xx_nand_info *info, int command,
>  	default:
>  		info->ndcb1 = 0;
>  		info->ndcb2 = 0;
> +		info->ndcb3 = 0;
>  		break;
>  	}
>  

Brian

Re: [PATCH v3 05/15] mtd: nand: pxa3xx: Support command buffer #3

[Thomas Petazzoni]

Dear Ezequiel Garcia,

On Sat, 10 Aug 2013 16:34:55 -0300, Ezequiel Garcia wrote:
> Some newer controllers support a fourth command buffer. This additional
> command buffer allows to set an arbitrary length count, using the
> NDCB3.NDLENCNT field, to perform non-standard length operations
> such as the ONFI parameter page read.
> 
> In controllers without this register, the operation has no effect.

Are you sure this is true? I thought you had this statement in earlier
revisions of your patch set, but one of the comment was precisely that
this patch was breaking platforms that did not have this register, and
this lead you to introduce the separate compatible string.

I must admit, I'm also a bit confused by the existing code:

 		nand_writel(info, NDCB0, info->ndcb0);
 		nand_writel(info, NDCB0, info->ndcb1);
 		nand_writel(info, NDCB0, info->ndcb2);

but it's probably because I don't know much about NAND and the
registers of this controller.

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH v3 05/15] mtd: nand: pxa3xx: Support command buffer #3

[Brian Norris]

On Sat, Aug 10, 2013 at 11:48:14PM +0200, Thomas Petazzoni wrote:
> On Sat, 10 Aug 2013 16:34:55 -0300, Ezequiel Garcia wrote:
> > Some newer controllers support a fourth command buffer. This additional
> > command buffer allows to set an arbitrary length count, using the
> > NDCB3.NDLENCNT field, to perform non-standard length operations
> > such as the ONFI parameter page read.
> > 
> > In controllers without this register, the operation has no effect.
> 
> Are you sure this is true? I thought you had this statement in earlier
> revisions of your patch set, but one of the comment was precisely that
> this patch was breaking platforms that did not have this register, and
> this lead you to introduce the separate compatible string.

It appears as if he didn't change the commit message properly. He does
now protect the fourth command buffer (?) with this:

+		/* NDCB3 register is available in NFCv2 (Armada 370/XP SoC) */
+		if (info->variant == PXA3XX_NAND_VARIANT_ARMADA370)
+			nand_writel(info, NDCB0, info->ndcb3);

> I must admit, I'm also a bit confused by the existing code:
> 
>  		nand_writel(info, NDCB0, info->ndcb0);
>  		nand_writel(info, NDCB0, info->ndcb1);
>  		nand_writel(info, NDCB0, info->ndcb2);
> 
> but it's probably because I don't know much about NAND and the
> registers of this controller.

Brian

Re: [PATCH v3 14/15] mtd: nand: pxa3xx: Increase data buffer size

[Brian Norris]

On Sat, Aug 10, 2013 at 04:35:04PM -0300, Ezequiel Garcia wrote:
> Devices with 4 KiB pages (plus OOB data) can be detected, so we increase
> the data buffer size. A better solution would be to allocate a buffer
> depending on the detected page size, but that's not possible given
> we need the buffer prior to the device detection.

Another possibility (for future work, maybe) is that you allocate a
smaller buffer that fits what you need for "device detection" (I presume
it's just for ONFI parameter page commands?), then reallocate the buffer
between nand_scan_ident() and nand_scan_tail() according to page size.
There are some other similar buffers throughout NAND that use a fixed
NAND_MAX_OOBSIZE and NAND_MAX_PAGESIZE.

> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> Tested-by: Daniel Mack <zonque@gmail.com>

...

Brian

Re: [PATCH v3 05/15] mtd: nand: pxa3xx: Support command buffer #3

[Ezequiel Garcia]

On Sat, Aug 10, 2013 at 07:33:18PM -0700, Brian Norris wrote:
> On Sat, Aug 10, 2013 at 11:48:14PM +0200, Thomas Petazzoni wrote:
> > On Sat, 10 Aug 2013 16:34:55 -0300, Ezequiel Garcia wrote:
> > > Some newer controllers support a fourth command buffer. This additional
> > > command buffer allows to set an arbitrary length count, using the
> > > NDCB3.NDLENCNT field, to perform non-standard length operations
> > > such as the ONFI parameter page read.
> > > 
> > > In controllers without this register, the operation has no effect.
> > 
> > Are you sure this is true? I thought you had this statement in earlier
> > revisions of your patch set, but one of the comment was precisely that
> > this patch was breaking platforms that did not have this register, and
> > this lead you to introduce the separate compatible string.
> 
> It appears as if he didn't change the commit message properly. He does
> now protect the fourth command buffer (?) with this:
> 
> +		/* NDCB3 register is available in NFCv2 (Armada 370/XP SoC) */
> +		if (info->variant == PXA3XX_NAND_VARIANT_ARMADA370)
> +			nand_writel(info, NDCB0, info->ndcb3);
> 

Indeed, I just forgot to update the commit message.

I'll prepare a new version to fix this.

> > I must admit, I'm also a bit confused by the existing code:
> > 
> >  		nand_writel(info, NDCB0, info->ndcb0);
> >  		nand_writel(info, NDCB0, info->ndcb1);
> >  		nand_writel(info, NDCB0, info->ndcb2);
> > 

In fact, I got confused as well when I first saw it.

However this behavior is specified in the Armada 370 (I can't find it in the public
PXA specs), which says that software must write either 12 or 16 bytes directly to
NDCB0, four bytes at a time to load the commands in NDCB0, NDCB1, NDCB2, (and optionally
NDCB3).

Writes to NDCB1, NDCB2 and NDCB3 are ignored but each NDCBx register can be read.

I'll add this information as a comment to the above confusing sequence
in the next soon to come version.
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Re: [PATCH v3 14/15] mtd: nand: pxa3xx: Increase data buffer size

[Ezequiel Garcia]

On Sat, Aug 10, 2013 at 08:30:45PM -0700, Brian Norris wrote:
> On Sat, Aug 10, 2013 at 04:35:04PM -0300, Ezequiel Garcia wrote:
> > Devices with 4 KiB pages (plus OOB data) can be detected, so we increase
> > the data buffer size. A better solution would be to allocate a buffer
> > depending on the detected page size, but that's not possible given
> > we need the buffer prior to the device detection.
> 
> Another possibility (for future work, maybe) is that you allocate a
> smaller buffer that fits what you need for "device detection" (I presume
> it's just for ONFI parameter page commands?), then reallocate the buffer
> between nand_scan_ident() and nand_scan_tail() according to page size.
> There are some other similar buffers throughout NAND that use a fixed
> NAND_MAX_OOBSIZE and NAND_MAX_PAGESIZE.
> 

That sounds better. I spent a considerable amount of time trying to find
a better solution.

I'll drop this one from the next version and I'll re-work following your
idea.

Thanks,
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com