mtd: nand: force NAND_CMD_READID onto 8-bit bus

Use a repeated read_byte() instead of read_buf(), since for x16 buswidth devices, we need to avoid the upper I/O[16:9] bits. See the following commit for reference:

commit 05f7835975dad6b3b517f9e23415985e648fb875

Author: Uwe Kleine-König <[email protected]>
Date:   Thu Dec 5 22:22:04 2013 +0100

mtd: nand: don't use {read,write}_buf for 8-bit transfers

Now, I think that all barriers to probing ONFI on x16 devices are removed, so remove the check from nand_flash_detect_onfi().

Tested on 8-bit ONFI NAND (Micron MT29F32G08CBADAWP).

Signed-off-by Brian Norris <[email protected]>

The NAND command helpers tend to automatically shift the column address for x16 bus devices, since most commands expect a word address, not a byte address. The Read ID command, however, expects an 8-bit address (i.e., 0x00, 0x20, or 0x40 should not be translated to 0x00, 0x10, or 0x20).

This fixes the column address for a few drivers which imitate the nand_base defaults. Note that I don't touch sh_flctl.c, since it already handles this problem slightly differently (note its comment "READID is always performed using an 8-bit bus").

I have not tested this patch, as I only have x8 parts up for testing at this point. Hopefully that can change soon...

Signed-off-by Brian Norris <[email protected]>

Quoted 14 lines · Brian Norris

The NAND command helpers tend to automatically shift the column address for x16 bus devices, since most commands expect a word address, not a byte address. The Read ID command, however, expects an 8-bit address (i.e., 0x00, 0x20, or 0x40 should not be translated to 0x00, 0x10, or 0x20).

This fixes the column address for a few drivers which imitate the nand_base defaults. Note that I don't touch sh_flctl.c, since it already handles this problem slightly differently (note its comment "READID is always performed using an 8-bit bus").

I have not tested this patch, as I only have x8 parts up for testing at this point. Hopefully that can change soon...

Quoted 1 line

Signed-off-by Brian Norris <[email protected]>

I applied both patches and tested on my AM335x board (omap2-nand driver). Both 8-bit and 16-bit devices get ONFI-probed and pass a nandtest round.

Tested-by Ezequiel Garcia <[email protected]>

[..]

include/linux/mtd/nand.h static inline bool nand_is_slc(struct nand_chip *chip) · line 845

Quoted 11 lines

include/linux/mtd/nand.h +10 -0

+
+/**
+ * Check if the opcode's address should be sent only on the lower 8 bits
+ * @command: opcode to check
+ */
+static inline int nand_opcode_8bits(unsigned int command)
+{
+	return command == NAND_CMD_READID;
+}
+
 #endif /* __LINUX_MTD_NAND_H */

With the introduction of this function, I think all the problems we've discussed.

The solution looks good to me so. Nice job!

From: Brian Norris [mailto:[email protected]]

include/linux/mtd/nand.h +1 -0

index a719686c9cce..c034dc4224cb 100644
@@ -832,4 +832,14 @@ static inline bool nand_is_slc(struct nand_chip *chip)
 {
 	return chip->bits_per_cell == 1;
 }
+

Hi Brian,

Quoted 10 lines

From: Brian Norris [mailto:[email protected]]

include/linux/mtd/nand.h +1 -0

index a719686c9cce..c034dc4224cb 100644
@@ -832,4 +832,14 @@ static inline bool nand_is_slc(struct nand_chip *chip)
 {
 	return chip->bits_per_cell == 1;
 }
+

include/linux/mtd/nand.h static inline bool nand_is_slc(struct nand_chip *chip) · line 845

Quoted 10 lines

include/linux/mtd/nand.h +9 -0

+/**
+ * Check if the opcode's address should be sent only on the lower 8 bits
+ * @command: opcode to check
+ */
+static inline int nand_opcode_8bits(unsigned int command)
+{
+	return command == NAND_CMD_READID;
+}
+
 

Can 'nand_opcode_8bits, made a macro instead of inline function ? #define IS_8BIT_CMD(cmd) (unlikely(cmd == NAND_CMD_READID))

Because 'nand_opcode_8bits' is used in nand_command() and nand_command_lp() which is performance critical code (chip->cmd is called multiple times for fetching page data and OOB). Though we should expect compiler to treat this inline function same as macro here, But just to be doubly sure for future changes also.

Quoted 9 lines

include/linux/mtd/nand.h static inline bool nand_is_slc(struct nand_chip *chip) · line 844

Quoted 6 lines

include/linux/mtd/nand.h +5 -0

+static inline int nand_opcode_8bits(unsigned int command)
+{
+	return command == NAND_CMD_READID;
+}
+

Can 'nand_opcode_8bits, made a macro instead of inline function ? #define IS_8BIT_CMD(cmd) (unlikely(cmd == NAND_CMD_READID))

Quoted 5 lines

Because 'nand_opcode_8bits' is used in nand_command() and nand_command_lp() which is performance critical code (chip->cmd is called multiple times for fetching page data and OOB). Though we should expect compiler to treat this inline function same as macro here, But just to be doubly sure for future changes also.

I'm not a compiler guru, but I'd be very surprised if the compiler would make a difference here, given the function is static, inline and small. Besides, an inline function is more readable and type-aware. I'd say it's the Right Thing to do.

Nevertheless, I did a small check on my platform (handbuilt ARM GCC 4.7.2) and both macro and inline is compiled into a "cmp" instruction. The unlikely doesn't seem to have any effect.

Quoted 15 lines

Quoted 9 lines

include/linux/mtd/nand.h static inline bool nand_is_slc(struct nand_chip *chip) · line 844

Quoted 6 lines

include/linux/mtd/nand.h +5 -0

+static inline int nand_opcode_8bits(unsigned int command)
+{
+	return command == NAND_CMD_READID;
+}
+

Can 'nand_opcode_8bits, made a macro instead of inline function ? #define IS_8BIT_CMD(cmd) (unlikely(cmd == NAND_CMD_READID))

Quoted 5 lines

Because 'nand_opcode_8bits' is used in nand_command() and nand_command_lp() which is performance critical code (chip->cmd is called multiple times for fetching page data and OOB). Though we should expect compiler to treat this inline function same as macro here, But just to be doubly sure for future changes also.

Quoted 9 lines

I'm not a compiler guru, but I'd be very surprised if the compiler would make a difference here, given the function is static, inline and small. Besides, an inline function is more readable and type-aware. I'd say it's the Right Thing to do.

Nevertheless, I did a small check on my platform (handbuilt ARM GCC 4.7.2) and both macro and inline is compiled into a "cmp" instruction. The unlikely doesn't seem to have any effect.

Please check the JMP instruction just after CMP instruction.. In-case of (unlikely(x == y)) JMP/JE will be used to branch when x == y. where as in-case of (likely(x == y)) JMP/JNE will be used to branch x != y.

(this is my understanding, however may be for small branches this has
no effect because both paths can be fetched simultaneously into pipeline).

From: Brian Norris [mailto:[email protected]]

Let's stop the nonsense then :)

Though its redundant, as Ezequiel has already tested these patches on AM335x using both x8 and x16 Micron devices. But still I re-tested them on different OMAP boards.

Tested-by Pekon Gupta <[email protected]>

Quoted 23 lines · Pekon

Quoted 10 lines

From: Brian Norris [mailto:[email protected]]

include/linux/mtd/nand.h +1 -0

index a719686c9cce..c034dc4224cb 100644
@@ -832,4 +832,14 @@ static inline bool nand_is_slc(struct nand_chip *chip)
 {
 	return chip->bits_per_cell == 1;
 }
+

include/linux/mtd/nand.h static inline bool nand_is_slc(struct nand_chip *chip) · line 845

Quoted 10 lines

include/linux/mtd/nand.h +9 -0

+/**
+ * Check if the opcode's address should be sent only on the lower 8 bits
+ * @command: opcode to check
+ */
+static inline int nand_opcode_8bits(unsigned int command)
+{
+	return command == NAND_CMD_READID;
+}
+
 

Can 'nand_opcode_8bits, made a macro instead of inline function ? #define IS_8BIT_CMD(cmd) (unlikely(cmd == NAND_CMD_READID))

Quoted 3 lines

Because 'nand_opcode_8bits' is used in nand_command() and nand_command_lp() which is performance critical code (chip->cmd is called multiple times for fetching page data and OOB).

Quoted 3 lines

Though we should expect compiler to treat this inline function same as macro here, But just to be doubly sure for future changes also.

I'm not a compiler guru, but I'd be very surprised if the compiler would make a difference here, given the function is static, inline and small. Besides, an inline function is more readable and type-aware. I'd say it's the Right Thing to do.

Nevertheless, I did a small check on my platform (handbuilt ARM GCC 4.7.2) and both macro and inline is compiled into a "cmp" instruction. The unlikely doesn't seem to have any effect.