ONFI patch

ONFI patch

[David Mosberger-Tang]

[-- Attachment #1.1: Type: text/plain, Size: 940 bytes --]

I would like to propose a patch along the attached to make some of the ONFI
support routines work on 16-bit devices.  It does the following:

   - Always call nand_flash_detect_onfi(), even for known chips.  Without
   this, onfi_version stays at 0 and nand_onfi_get_features and
   nand_onfi_set_features will always fail with EINVAL.
   - For the READID, GET_FEATURES, and SET_FEATURES commands, the column
   address is always 1 byte, regardless of bus-width.
   - nand_onfi_set_features(), nand_onfi_get_features(), and
   nand_flash_detect_onfi() need to use 8-bit transfers, regardless of
   bus-width.

I see that the linux-mtd tree has a NAND_BUSWIDTH_AUTO features, but I
don't see how that's supposed to help for, say, nand_onfi-set_features() or
nand_onfi_get_features() as nand_set_defaults() will eventually set the
16-bit transfer routines for 16-bit devices.

The patch is relative to linux-mtd GIT tree.

Thanks,

  --david

[-- Attachment #1.2: Type: text/html, Size: 1161 bytes --]

[-- Attachment #2: onfi-fixes.diff.txt --]
[-- Type: text/plain, Size: 3889 bytes --]

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 42c6392..31decd1 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -507,6 +507,32 @@ void nand_wait_ready(struct mtd_info *mtd)
 }
 EXPORT_SYMBOL_GPL(nand_wait_ready);
 
+static int
+set_column (struct mtd_info *mtd, struct nand_chip *chip,
+	    unsigned int command, int column,
+	    unsigned int column_width, int ctrl)
+{
+	switch (command) {
+	case NAND_CMD_READID:
+	case NAND_CMD_GET_FEATURES:
+	case NAND_CMD_SET_FEATURES:
+		chip->cmd_ctrl(mtd, column, ctrl);
+		ctrl &= ~NAND_CTRL_CHANGE;
+		break;
+
+	default:
+		/* Adjust columns for 16 bit buswidth */
+		if (chip->options & NAND_BUSWIDTH_16)
+			column >>= 1;
+		chip->cmd_ctrl(mtd, column, ctrl);
+		ctrl &= ~NAND_CTRL_CHANGE;
+		if (column_width > 8)
+			chip->cmd_ctrl(mtd, column >> 8, ctrl);
+		break;
+	}
+	return ctrl;
+}
+
 /**
  * nand_command - [DEFAULT] Send command to NAND device
  * @mtd: MTD device structure
@@ -546,13 +572,8 @@ static void nand_command(struct mtd_info *mtd, unsigned int command,
 	/* Address cycle, when necessary */
 	ctrl = NAND_CTRL_ALE | NAND_CTRL_CHANGE;
 	/* Serially input address */
-	if (column != -1) {
-		/* Adjust columns for 16 bit buswidth */
-		if (chip->options & NAND_BUSWIDTH_16)
-			column >>= 1;
-		chip->cmd_ctrl(mtd, column, ctrl);
-		ctrl &= ~NAND_CTRL_CHANGE;
-	}
+	if (column != -1)
+		ctrl = set_column(mtd, chip, command, column, 8, ctrl);
 	if (page_addr != -1) {
 		chip->cmd_ctrl(mtd, page_addr, ctrl);
 		ctrl &= ~NAND_CTRL_CHANGE;
@@ -638,14 +659,9 @@ static void nand_command_lp(struct mtd_info *mtd, unsigned int command,
 		int ctrl = NAND_CTRL_CHANGE | NAND_NCE | NAND_ALE;
 
 		/* Serially input address */
-		if (column != -1) {
-			/* Adjust columns for 16 bit buswidth */
-			if (chip->options & NAND_BUSWIDTH_16)
-				column >>= 1;
-			chip->cmd_ctrl(mtd, column, ctrl);
-			ctrl &= ~NAND_CTRL_CHANGE;
-			chip->cmd_ctrl(mtd, column >> 8, ctrl);
-		}
+		if (column != -1)
+			ctrl = set_column (mtd, chip, command, column, 16,
+					   ctrl);
 		if (page_addr != -1) {
 			chip->cmd_ctrl(mtd, page_addr, ctrl);
 			chip->cmd_ctrl(mtd, page_addr >> 8,
@@ -2737,7 +2753,7 @@ static int nand_onfi_set_features(struct mtd_info *mtd, struct nand_chip *chip,
 		return -EINVAL;
 
 	chip->cmdfunc(mtd, NAND_CMD_SET_FEATURES, addr, -1);
-	chip->write_buf(mtd, subfeature_param, ONFI_SUBFEATURE_PARAM_LEN);
+	nand_write_buf(mtd, subfeature_param, ONFI_SUBFEATURE_PARAM_LEN);
 	status = chip->waitfunc(mtd, chip);
 	if (status & NAND_STATUS_FAIL)
 		return -EIO;
@@ -2761,7 +2777,7 @@ static int nand_onfi_get_features(struct mtd_info *mtd, struct nand_chip *chip,
 	memset(subfeature_param, 0, ONFI_SUBFEATURE_PARAM_LEN);
 
 	chip->cmdfunc(mtd, NAND_CMD_GET_FEATURES, addr, -1);
-	chip->read_buf(mtd, subfeature_param, ONFI_SUBFEATURE_PARAM_LEN);
+	nand_read_buf(mtd, subfeature_param, ONFI_SUBFEATURE_PARAM_LEN);
 	return 0;
 }
 
@@ -2882,7 +2898,8 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip,
 
 	chip->cmdfunc(mtd, NAND_CMD_PARAM, 0, -1);
 	for (i = 0; i < 3; i++) {
-		chip->read_buf(mtd, (uint8_t *)p, sizeof(*p));
+		/* Must read with 8-bit transfers even on 16-bit devices: */
+		nand_read_buf(mtd, (uint8_t *)p, sizeof (*p));
 		if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) ==
 				le16_to_cpu(p->crc)) {
 			pr_info("ONFI param page %d valid\n", i);
@@ -3227,11 +3244,9 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
 			break;
 
 	chip->onfi_version = 0;
-	if (!type->name || !type->pagesize) {
-		/* Check is chip is ONFI compliant */
-		if (nand_flash_detect_onfi(mtd, chip, &busw))
-			goto ident_done;
-	}
+	/* Check is chip is ONFI compliant */
+	if (nand_flash_detect_onfi(mtd, chip, &busw))
+		goto ident_done;
 
 	if (!type->name)
 		return ERR_PTR(-ENODEV);

RE : ONFI patch

[Matthieu Castet]

> I would like to propose a patch along the attached to make some of
> the ONFI support routines work on 16-bit devices.  It does the
> following:
> 
>  *   Always call nand_flash_detect_onfi(), even for known chips.
> Without this, onfi_version stays at 0 and nand_onfi_get_features and
> nand_onfi_set_features will always fail with EINVAL.

Why do you call it for flash we identify as small page ?
On which flash it is not working ?

>  *   For the READID, GET_FEATURES, and SET_FEATURES commands, the
> column address is always 1 byte, regardless of bus-width.

This is ugly.

> I see that the linux-mtd tree has a NAND_BUSWIDTH_AUTO features, but
> I don't see how that's supposed to help for, say,
Did you look at the omap example ?

> nand_onfi-set_features() or nand_onfi_get_features() as
> nand_set_defaults() will eventually set the 16-bit transfer routines
> for 16-bit devices.
If that's a problem of nand_onfi_get/set_features why don't you fix them to use only 8 bits transfert ?

Isn't there a patch already submitted for doing that ?


Matthieu

Re: RE : ONFI patch

[Matthieu CASTET]

David Mosberger-Tang a écrit :
> Matthieu,
> 
> On Sat, Mar 16, 2013 at 12:21 PM, Matthieu Castet
> <matthieu.castet@parrot.com <mailto:matthieu.castet@parrot.com>> wrote:

>  
> 
>     > I see that the linux-mtd tree has a NAND_BUSWIDTH_AUTO features, but
>     > I don't see how that's supposed to help for, say,
>     Did you look at the omap example ?
> 
> 
> I don't see any users of NAND_BUSWIDTH_AUTO in the linux-mtd GIT tree.
>  Am I missing something?


Yes the patch was pending. You can find it here :
http://article.gmane.org/gmane.linux.ports.arm.omap/93502/match=
>  
> 
>     > nand_onfi-set_features() or nand_onfi_get_features() as
>     > nand_set_defaults() will eventually set the 16-bit transfer routines
>     > for 16-bit devices.
>     If that's a problem of nand_onfi_get/set_features why don't you fix
>     them to use only 8 bits transfert ?
> 
> 
> That's what my patch does.
>  
> 
>     Isn't there a patch already submitted for doing that ?
> 
> 
> I don't know.  I joined the mailing list only recently.  Can you point
> me to such a patch?
> 
http://thread.gmane.org/gmane.linux.drivers.mtd/45578/focus=45610

But it only fix the nand_onfi_get_features and I don't think it fix the column
shift.

BTW you can't use nand_write_buf/nand_read_buf in common code. Some controller
overide chip->read_buf/chip->write_buf with their version and don't expect
nand_write_buf/nand_read_buf to be called.


Matthieu

Re: RE : ONFI patch

[Matthieu CASTET]

David Mosberger-Tang a écrit :
> Matthieu,
> 

> 
> 
> I was afraid that might be the case.  Since there is no write_byte
> callback, would it be OK if we convert the subfeature_param to 16 bits
> and then write it using the write_buf callback?  If so, I can code that up.
Yes may be that's the best solution

And while we have a 16 bits version of nand_onfi_set_features we send :
chip->cmdfunc(mtd, NAND_CMD_SET_FEATURES, addr<<1, -1);


Matthieu