Patchwork [v4,26/31] mtd: nand: pxa3xx: Introduce multiple page I/O support

login
register
mail settings
Submitter Ezequiel Garcia
Date Nov. 7, 2013, 3:17 p.m.
Message ID <1383837455-30721-27-git-send-email-ezequiel.garcia@free-electrons.com>
Download mbox | patch
Permalink /patch/289443/
State New
Headers show

Comments

Ezequiel Garcia - Nov. 7, 2013, 3:17 p.m.
As preparation work to fully support large pages, this commit adds
the initial infrastructure to support splitted (aka chunked) I/O
operation. This commit adds support for read, and follow-up patches
will add write support.

When a read (aka READ0) command is issued, the driver loops issuing
the same command until all the requested data is transfered, changing
the 'extended' command field as needed.

For instance, if the driver is required to read a 4 KiB page, using a
chunk size of 2 KiB, the transaction is splitted in:
1. Monolithic read, first 2 KiB page chunk is read
2. Last naked read, second and last 2KiB page chunk is read

If ECC is enabled it is calculated on each chunk transfered and added
at a controller-fixed location after the data chunk that must be
spare area.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 drivers/mtd/nand/pxa3xx_nand.c | 182 ++++++++++++++++++++++++++++++++++++++---
 1 file changed, 172 insertions(+), 10 deletions(-)
Ezequiel Garcia - Nov. 7, 2013, 11:45 p.m.
Hello,

I have a concern about the ECC mode and strength determination.

On Thu, Nov 07, 2013 at 12:17:30PM -0300, Ezequiel Garcia wrote:
[..]
> @@ -1152,7 +1289,28 @@ static int armada370_ecc_init(struct pxa3xx_nand_info *info,
>  			      struct nand_ecc_ctrl *ecc,
>  			      int strength, int page_size)
>  {
> -	/* Unimplemented yet */
> +	if (strength == 4 && page_size == 4096) {
> +		info->ecc_bch = 1;
> +		info->chunk_size = 2048;
> +		info->spare_size = 32;
> +		info->ecc_size = 32;
> +		ecc->mode = NAND_ECC_HW;
> +		ecc->size = info->chunk_size;
> +		ecc->layout = &ecc_layout_4KB_bch4bit;
> +		ecc->strength = 16;
> +		return 1;
> +
> +	} else if (strength == 8 && page_size == 4096) {
> +		info->ecc_bch = 1;
> +		info->chunk_size = 1024;
> +		info->spare_size = 0;
> +		info->ecc_size = 32;
> +		ecc->mode = NAND_ECC_HW;
> +		ecc->size = info->chunk_size;
> +		ecc->layout = &ecc_layout_4KB_bch8bit;
> +		ecc->strength = 16;
> +		return 1;
> +	}
>  	return 0;
>  }
>  

The above shows I tried to be very careful (aka paranoid) in the ECC
mode determination. Please note that what I call "ECC mode", is
determined by the chunk->size value only.

As the included documentation explains this controller supports two
different BCH ECC strength: 16-over-1024, or 16-over-2048. Setting the chunk
size to either 1024 or 2048 is what determines which of the above will
actually take effect.

In past mails, we've refered to these two as BCH4 or BCH8, but this is not
really accurate (as Brian has pointed out) so I droped that terms in
favor of the real thing: 16 correctable bits over 1024 data bytes, or
16 correctable bits over 2048 data bytes.

In turn, such BCH mode setting affects heavily the page layout, because
after each transfered chunk (which is either 1024+spare or 2048+spare)
the controller reads and writes a 30B ECC region.

In other words, the kernel cannot arbitrarily change this setting, or
the image won't be readable/writeable any longer.

For this reason, I'm starting to think the above method of picking
the "ECC mode" based solely on the page size or the strength is slightly
fragile (future developers might come and "improve" the driver breaking
images).

Besides: what if a user wants the kernel to use a "higher" than
required strength?

In conclusion: I'm starting to think a better (and long-term safer)
approach is to set the ECC mode in the DT. I know this sucks, but from
my point of view the flash device contains an image that "must" be
read/write with a given ECC mode, and this ECC mode is by no means
discoverable.

Now, let's suppose we want to set this in the DT: what property
would we use? "marvell,nand-ecc-opt = 16-2048"? (a bit odd, no?).

What do you think?
Brian Norris - Nov. 8, 2013, 12:46 a.m.
Hi,

I haven't taken a look at all your changes yet, but I'll get this
comment off my chest while it's still fresh :)

On Thu, Nov 07, 2013 at 08:45:00PM -0300, Ezequiel Garcia wrote:
> Hello,
> 
> I have a concern about the ECC mode and strength determination.
> 
> On Thu, Nov 07, 2013 at 12:17:30PM -0300, Ezequiel Garcia wrote:
> [..]
> > @@ -1152,7 +1289,28 @@ static int armada370_ecc_init(struct pxa3xx_nand_info *info,
> >  			      struct nand_ecc_ctrl *ecc,
> >  			      int strength, int page_size)
> >  {
> > -	/* Unimplemented yet */
> > +	if (strength == 4 && page_size == 4096) {
> > +		info->ecc_bch = 1;
> > +		info->chunk_size = 2048;
> > +		info->spare_size = 32;
> > +		info->ecc_size = 32;
> > +		ecc->mode = NAND_ECC_HW;
> > +		ecc->size = info->chunk_size;
> > +		ecc->layout = &ecc_layout_4KB_bch4bit;
> > +		ecc->strength = 16;
> > +		return 1;
> > +
> > +	} else if (strength == 8 && page_size == 4096) {
> > +		info->ecc_bch = 1;
> > +		info->chunk_size = 1024;
> > +		info->spare_size = 0;
> > +		info->ecc_size = 32;
> > +		ecc->mode = NAND_ECC_HW;
> > +		ecc->size = info->chunk_size;
> > +		ecc->layout = &ecc_layout_4KB_bch8bit;
> > +		ecc->strength = 16;
> > +		return 1;
> > +	}
> >  	return 0;
> >  }
> >  
> 
> The above shows I tried to be very careful (aka paranoid) in the ECC
> mode determination. Please note that what I call "ECC mode", is
> determined by the chunk->size value only.
> 
> As the included documentation explains this controller supports two
> different BCH ECC strength: 16-over-1024, or 16-over-2048. Setting the chunk
> size to either 1024 or 2048 is what determines which of the above will
> actually take effect.
> 
> In past mails, we've refered to these two as BCH4 or BCH8, but this is not
> really accurate (as Brian has pointed out) so I droped that terms in
> favor of the real thing: 16 correctable bits over 1024 data bytes, or
> 16 correctable bits over 2048 data bytes.
> 
> In turn, such BCH mode setting affects heavily the page layout, because
> after each transfered chunk (which is either 1024+spare or 2048+spare)
> the controller reads and writes a 30B ECC region.
> 
> In other words, the kernel cannot arbitrarily change this setting, or
> the image won't be readable/writeable any longer.
> 
> For this reason, I'm starting to think the above method of picking
> the "ECC mode" based solely on the page size or the strength is slightly
> fragile (future developers might come and "improve" the driver breaking
> images).
> 
> Besides: what if a user wants the kernel to use a "higher" than
> required strength?
> 
> In conclusion: I'm starting to think a better (and long-term safer)
> approach is to set the ECC mode in the DT. I know this sucks, but from
> my point of view the flash device contains an image that "must" be
> read/write with a given ECC mode, and this ECC mode is by no means
> discoverable.

I think that's the way to go, in general. We can get by with
"implementation defined behavior" (i.e., make the driver do what makes
sense) for now, but to avoid breakage in the future, the bootloader
needs to specify what ECC configuration is required -- via DT. But your
current patch is still probably fine without it, since you're supporting
new hardware.

> Now, let's suppose we want to set this in the DT: what property
> would we use? "marvell,nand-ecc-opt = 16-2048"? (a bit odd, no?).
> 
> What do you think?

If ECC is purely determined by the chunk size, then why not just:

  marvell,nand-chunk-size = 2048;
  (with a value of 2048, 1024, or 512?)

Brian
Ezequiel Garcia - Nov. 8, 2013, 3:21 a.m.
On Thu, Nov 07, 2013 at 04:46:00PM -0800, Brian Norris wrote:
> On Thu, Nov 07, 2013 at 08:45:00PM -0300, Ezequiel Garcia wrote:
> > Hello,
> > 
> > I have a concern about the ECC mode and strength determination.
> > 
> > On Thu, Nov 07, 2013 at 12:17:30PM -0300, Ezequiel Garcia wrote:
> > [..]
> > > @@ -1152,7 +1289,28 @@ static int armada370_ecc_init(struct pxa3xx_nand_info *info,
> > >  			      struct nand_ecc_ctrl *ecc,
> > >  			      int strength, int page_size)
> > >  {
> > > -	/* Unimplemented yet */
> > > +	if (strength == 4 && page_size == 4096) {
> > > +		info->ecc_bch = 1;
> > > +		info->chunk_size = 2048;
> > > +		info->spare_size = 32;
> > > +		info->ecc_size = 32;
> > > +		ecc->mode = NAND_ECC_HW;
> > > +		ecc->size = info->chunk_size;
> > > +		ecc->layout = &ecc_layout_4KB_bch4bit;
> > > +		ecc->strength = 16;
> > > +		return 1;
> > > +
> > > +	} else if (strength == 8 && page_size == 4096) {
> > > +		info->ecc_bch = 1;
> > > +		info->chunk_size = 1024;
> > > +		info->spare_size = 0;
> > > +		info->ecc_size = 32;
> > > +		ecc->mode = NAND_ECC_HW;
> > > +		ecc->size = info->chunk_size;
> > > +		ecc->layout = &ecc_layout_4KB_bch8bit;
> > > +		ecc->strength = 16;
> > > +		return 1;
> > > +	}
> > >  	return 0;
> > >  }
> > >  
> > 
> > The above shows I tried to be very careful (aka paranoid) in the ECC
> > mode determination. Please note that what I call "ECC mode", is
> > determined by the chunk->size value only.
> > 
> > As the included documentation explains this controller supports two
> > different BCH ECC strength: 16-over-1024, or 16-over-2048. Setting the chunk
> > size to either 1024 or 2048 is what determines which of the above will
> > actually take effect.
> > 
> > In past mails, we've refered to these two as BCH4 or BCH8, but this is not
> > really accurate (as Brian has pointed out) so I droped that terms in
> > favor of the real thing: 16 correctable bits over 1024 data bytes, or
> > 16 correctable bits over 2048 data bytes.
> > 
> > In turn, such BCH mode setting affects heavily the page layout, because
> > after each transfered chunk (which is either 1024+spare or 2048+spare)
> > the controller reads and writes a 30B ECC region.
> > 
> > In other words, the kernel cannot arbitrarily change this setting, or
> > the image won't be readable/writeable any longer.
> > 
> > For this reason, I'm starting to think the above method of picking
> > the "ECC mode" based solely on the page size or the strength is slightly
> > fragile (future developers might come and "improve" the driver breaking
> > images).
> > 
> > Besides: what if a user wants the kernel to use a "higher" than
> > required strength?
> > 
> > In conclusion: I'm starting to think a better (and long-term safer)
> > approach is to set the ECC mode in the DT. I know this sucks, but from
> > my point of view the flash device contains an image that "must" be
> > read/write with a given ECC mode, and this ECC mode is by no means
> > discoverable.
> 
> I think that's the way to go, in general. We can get by with
> "implementation defined behavior" (i.e., make the driver do what makes
> sense) for now, but to avoid breakage in the future, the bootloader
> needs to specify what ECC configuration is required -- via DT. But your
> current patch is still probably fine without it, since you're supporting
> new hardware.
> 

That sounds good. The ECC mode is currently "selected" in the driver
to be the one that "makes sense". In follow up patches we'll add a
proper DT binding, so this ECC mode can be set by the user.

The latter is an improvement over the former (adding more flexibility),
so there won't be any regressions.

Just to confirm: I consider this patchset to be OK as it is, and I expect
it to either get merged or receive some request for changes.

Oh, and by the way: we __really__ need someone to test for regressions on PXA.
Ezequiel Garcia - Nov. 14, 2013, 3:40 p.m.
On Thu, Nov 07, 2013 at 04:46:00PM -0800, Brian Norris wrote:
> 
> I haven't taken a look at all your changes yet, but I'll get this

Any comments on the v4?

I don't think there are any open issues to reply, but maybe my memory
is failing, so feel free to refresh it :-)

I'll re-submit the series as soon as v3.13-rc1 is out.

Patch

diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index 59efd72..51d90cc 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -103,6 +103,8 @@ 
 #define NDCB0_ST_ROW_EN         (0x1 << 26)
 #define NDCB0_AUTO_RS		(0x1 << 25)
 #define NDCB0_CSEL		(0x1 << 24)
+#define NDCB0_EXT_CMD_TYPE_MASK	(0x7 << 29)
+#define NDCB0_EXT_CMD_TYPE(x)	(((x) << 29) & NDCB0_EXT_CMD_TYPE_MASK)
 #define NDCB0_CMD_TYPE_MASK	(0x7 << 21)
 #define NDCB0_CMD_TYPE(x)	(((x) << 21) & NDCB0_CMD_TYPE_MASK)
 #define NDCB0_NC		(0x1 << 20)
@@ -113,6 +115,14 @@ 
 #define NDCB0_CMD1_MASK		(0xff)
 #define NDCB0_ADDR_CYC_SHIFT	(16)
 
+#define EXT_CMD_TYPE_DISPATCH	6 /* Command dispatch */
+#define EXT_CMD_TYPE_NAKED_RW	5 /* Naked read or Naked write */
+#define EXT_CMD_TYPE_READ	4 /* Read */
+#define EXT_CMD_TYPE_DISP_WR	4 /* Command dispatch with write */
+#define EXT_CMD_TYPE_FINAL	3 /* Final command */
+#define EXT_CMD_TYPE_LAST_RW	1 /* Last naked read/write */
+#define EXT_CMD_TYPE_MONO	0 /* Monolithic read/write */
+
 /* macros for registers read/write */
 #define nand_writel(info, off, val)	\
 	__raw_writel((val), (info)->mmio_base + (off))
@@ -206,8 +216,8 @@  struct pxa3xx_nand_info {
 	int			use_spare;	/* use spare ? */
 	int			is_ready;
 
-	unsigned int		fifo_size;	/* max. data size in the FIFO */
 	unsigned int		data_size;	/* data to be read from FIFO */
+	unsigned int		chunk_size;	/* split commands chunk size */
 	unsigned int		oob_size;
 	unsigned int		spare_size;
 	unsigned int		ecc_size;
@@ -271,6 +281,31 @@  static struct nand_bbt_descr bbt_mirror_descr = {
 	.pattern = bbt_mirror_pattern
 };
 
+static struct nand_ecclayout ecc_layout_4KB_bch4bit = {
+	.eccbytes = 64,
+	.eccpos = {
+		32,  33,  34,  35,  36,  37,  38,  39,
+		40,  41,  42,  43,  44,  45,  46,  47,
+		48,  49,  50,  51,  52,  53,  54,  55,
+		56,  57,  58,  59,  60,  61,  62,  63,
+		96,  97,  98,  99,  100, 101, 102, 103,
+		104, 105, 106, 107, 108, 109, 110, 111,
+		112, 113, 114, 115, 116, 117, 118, 119,
+		120, 121, 122, 123, 124, 125, 126, 127},
+	/* Bootrom looks in bytes 0 & 5 for bad blocks */
+	.oobfree = { {6, 26}, { 64, 32} }
+};
+
+static struct nand_ecclayout ecc_layout_4KB_bch8bit = {
+	.eccbytes = 128,
+	.eccpos = {
+		32,  33,  34,  35,  36,  37,  38,  39,
+		40,  41,  42,  43,  44,  45,  46,  47,
+		48,  49,  50,  51,  52,  53,  54,  55,
+		56,  57,  58,  59,  60,  61,  62,  63},
+	.oobfree = { }
+};
+
 /* Define a default flash type setting serve as flash detecting only */
 #define DEFAULT_FLASH_TYPE (&builtin_flash_types[0])
 
@@ -433,7 +468,7 @@  static void disable_int(struct pxa3xx_nand_info *info, uint32_t int_mask)
 
 static void handle_data_pio(struct pxa3xx_nand_info *info)
 {
-	unsigned int do_bytes = min(info->data_size, info->fifo_size);
+	unsigned int do_bytes = min(info->data_size, info->chunk_size);
 
 	switch (info->state) {
 	case STATE_PIO_WRITING:
@@ -670,7 +705,7 @@  static void prepare_start_command(struct pxa3xx_nand_info *info, int command)
 }
 
 static int prepare_set_command(struct pxa3xx_nand_info *info, int command,
-		uint16_t column, int page_addr)
+		int ext_cmd_type, uint16_t column, int page_addr)
 {
 	int addr_cycle, exec_cmd;
 	struct pxa3xx_nand_host *host;
@@ -703,9 +738,20 @@  static int prepare_set_command(struct pxa3xx_nand_info *info, int command,
 		if (command == NAND_CMD_READOOB)
 			info->buf_start += mtd->writesize;
 
-		/* Second command setting for large pages */
-		if (mtd->writesize >= PAGE_CHUNK_SIZE)
+		/*
+		 * Multiple page read needs an 'extended command type' field,
+		 * which is either naked-read or last-read according to the
+		 * state.
+		 */
+		if (mtd->writesize == PAGE_CHUNK_SIZE) {
 			info->ndcb0 |= NDCB0_DBC | (NAND_CMD_READSTART << 8);
+		} else if (mtd->writesize > PAGE_CHUNK_SIZE) {
+			info->ndcb0 |= NDCB0_DBC | (NAND_CMD_READSTART << 8)
+					| NDCB0_LEN_OVRD
+					| NDCB0_EXT_CMD_TYPE(ext_cmd_type);
+			info->ndcb3 = info->chunk_size +
+				      info->oob_size;
+		}
 
 		set_command_address(info, mtd->writesize, column, page_addr);
 		break;
@@ -821,7 +867,8 @@  static void pxa3xx_nand_cmdfunc(struct mtd_info *mtd, unsigned command,
 	prepare_start_command(info, command);
 
 	info->state = STATE_PREPARED;
-	exec_cmd = prepare_set_command(info, command, column, page_addr);
+	exec_cmd = prepare_set_command(info, command, 0, column, page_addr);
+
 	if (exec_cmd) {
 		init_completion(&info->cmd_complete);
 		init_completion(&info->dev_ready);
@@ -839,6 +886,93 @@  static void pxa3xx_nand_cmdfunc(struct mtd_info *mtd, unsigned command,
 	info->state = STATE_IDLE;
 }
 
+static void armada370_nand_cmdfunc(struct mtd_info *mtd,
+				   const unsigned command,
+				   int column, int page_addr)
+{
+	struct pxa3xx_nand_host *host = mtd->priv;
+	struct pxa3xx_nand_info *info = host->info_data;
+	int ret, exec_cmd, ext_cmd_type;
+
+	/*
+	 * if this is a x16 device then convert the input
+	 * "byte" address into a "word" address appropriate
+	 * for indexing a word-oriented device
+	 */
+	if (info->reg_ndcr & NDCR_DWIDTH_M)
+		column /= 2;
+
+	/*
+	 * There may be different NAND chip hooked to
+	 * different chip select, so check whether
+	 * chip select has been changed, if yes, reset the timing
+	 */
+	if (info->cs != host->cs) {
+		info->cs = host->cs;
+		nand_writel(info, NDTR0CS0, info->ndtr0cs0);
+		nand_writel(info, NDTR1CS0, info->ndtr1cs0);
+	}
+
+	/* Select the extended command for the first command */
+	switch (command) {
+	case NAND_CMD_READ0:
+	case NAND_CMD_READOOB:
+		ext_cmd_type = EXT_CMD_TYPE_MONO;
+		break;
+	default:
+		ext_cmd_type = 0;
+	}
+
+	prepare_start_command(info, command);
+
+	/*
+	 * Prepare the "is ready" completion before starting a command
+	 * transaction sequence. If the command is not executed the
+	 * completion will be completed, see below.
+	 *
+	 * We can do that inside the loop because the command variable
+	 * is invariant and thus so is the exec_cmd.
+	 */
+	info->is_ready = 0;
+	init_completion(&info->dev_ready);
+	do {
+		info->state = STATE_PREPARED;
+		exec_cmd = prepare_set_command(info, command, ext_cmd_type,
+					       column, page_addr);
+		if (!exec_cmd) {
+			info->is_ready = 1;
+			complete(&info->dev_ready);
+			break;
+		}
+
+		init_completion(&info->cmd_complete);
+		pxa3xx_nand_start(info);
+
+		ret = wait_for_completion_timeout(&info->cmd_complete,
+				CHIP_DELAY_TIMEOUT);
+		if (!ret) {
+			dev_err(&info->pdev->dev, "Wait time out!!!\n");
+			/* Stop State Machine for next command cycle */
+			pxa3xx_nand_stop(info);
+			break;
+		}
+
+		/* Check if the sequence is complete */
+		if (info->data_size == 0)
+			break;
+
+		if (command == NAND_CMD_READ0 || command == NAND_CMD_READOOB) {
+			/* Last read: issue a 'last naked read' */
+			if (info->data_size == info->chunk_size)
+				ext_cmd_type = EXT_CMD_TYPE_LAST_RW;
+			else
+				ext_cmd_type = EXT_CMD_TYPE_NAKED_RW;
+		}
+	} while (1);
+
+	info->state = STATE_IDLE;
+}
+
 static int pxa3xx_nand_write_page_hwecc(struct mtd_info *mtd,
 		struct nand_chip *chip, const uint8_t *buf, int oob_required)
 {
@@ -1020,13 +1154,14 @@  static int pxa3xx_nand_detect_config(struct pxa3xx_nand_info *info)
 
 	if (ndcr & NDCR_PAGE_SZ) {
 		/* Controller's FIFO size */
-		info->fifo_size = 2048;
+		info->chunk_size = 2048;
 		host->read_id_bytes = 4;
 	} else {
-		info->fifo_size = 512;
+		info->chunk_size = 512;
 		host->read_id_bytes = 2;
 	}
 
+	/* Set an initial chunk size */
 	info->reg_ndcr = ndcr & ~NDCR_INT_MASK;
 	info->ndtr0cs0 = nand_readl(info, NDTR0CS0);
 	info->ndtr1cs0 = nand_readl(info, NDTR1CS0);
@@ -1130,6 +1265,7 @@  static int pxa_ecc_init(struct pxa3xx_nand_info *info,
 	 * is used with non-ONFI compliant devices.
 	 */
 	if (page_size == 2048) {
+		info->chunk_size = 2048;
 		info->spare_size = 40;
 		info->ecc_size = 24;
 		ecc->mode = NAND_ECC_HW;
@@ -1138,6 +1274,7 @@  static int pxa_ecc_init(struct pxa3xx_nand_info *info,
 		return 1;
 
 	} else if (page_size == 512) {
+		info->chunk_size = 512;
 		info->spare_size = 8;
 		info->ecc_size = 8;
 		ecc->mode = NAND_ECC_HW;
@@ -1152,7 +1289,28 @@  static int armada370_ecc_init(struct pxa3xx_nand_info *info,
 			      struct nand_ecc_ctrl *ecc,
 			      int strength, int page_size)
 {
-	/* Unimplemented yet */
+	if (strength == 4 && page_size == 4096) {
+		info->ecc_bch = 1;
+		info->chunk_size = 2048;
+		info->spare_size = 32;
+		info->ecc_size = 32;
+		ecc->mode = NAND_ECC_HW;
+		ecc->size = info->chunk_size;
+		ecc->layout = &ecc_layout_4KB_bch4bit;
+		ecc->strength = 16;
+		return 1;
+
+	} else if (strength == 8 && page_size == 4096) {
+		info->ecc_bch = 1;
+		info->chunk_size = 1024;
+		info->spare_size = 0;
+		info->ecc_size = 32;
+		ecc->mode = NAND_ECC_HW;
+		ecc->size = info->chunk_size;
+		ecc->layout = &ecc_layout_4KB_bch8bit;
+		ecc->strength = 16;
+		return 1;
+	}
 	return 0;
 }
 
@@ -1320,12 +1478,16 @@  static int alloc_nand_resource(struct platform_device *pdev)
 		chip->controller        = &info->controller;
 		chip->waitfunc		= pxa3xx_nand_waitfunc;
 		chip->select_chip	= pxa3xx_nand_select_chip;
-		chip->cmdfunc		= pxa3xx_nand_cmdfunc;
 		chip->read_word		= pxa3xx_nand_read_word;
 		chip->read_byte		= pxa3xx_nand_read_byte;
 		chip->read_buf		= pxa3xx_nand_read_buf;
 		chip->write_buf		= pxa3xx_nand_write_buf;
 		chip->options		|= NAND_NO_SUBPAGE_WRITE;
+
+		if (info->variant == PXA3XX_NAND_VARIANT_ARMADA370)
+			chip->cmdfunc = armada370_nand_cmdfunc;
+		else
+			chip->cmdfunc = pxa3xx_nand_cmdfunc;
 	}
 
 	spin_lock_init(&chip->controller->lock);