mtd: spi-nor: Add the DDR quad read support

For the DDR Quad read, the dummy cycles maybe 3 or 6 which is less then 8. The dummy cycles is actually 8 for SPI fast/dual/quad read.

This patch makes preparations for the DDR quad read, it fixes the wrong dummy value for both the spi-nor.c and m25p80.c.

Signed-off-by Huang Shijie <[email protected]>

---
This ia actually v3.

For the DDR Quad read, the dummy cycles maybe 3 or 6 which is less then 8. The dummy cycles is actually 8 for SPI fast/dual/quad read.

This patch makes preparations for the DDR quad read, it fixes the wrong dummy value for both the spi-nor.c and m25p80.c.

Signed-off-by Huang Shijie <[email protected]>

---
This ia actually v3.

We need the SPI NOR child node to store some specific features, such as the dummy cycles for the DDR Quad read.

But now, we only have the @dev field in the spi_nor{}. The @dev may points to a spi_device{} for m25p80, while it may points to a platform_deivice{} for the SPI NOR controller, such as fsl_quadspi.c.

It is not convenient for us to get come information from the SPI NOR flash.

This patch adds a new field @np to spi_nor{}, it points to the child node for the SPI NOR flash.

Signed-off-by Huang Shijie <[email protected]>

Quoted 7 lines · Huang Shijie

We need the SPI NOR child node to store some specific features, such as the dummy cycles for the DDR Quad read.

But now, we only have the @dev field in the spi_nor{}. The @dev may points to a spi_device{} for m25p80, while it may points to a platform_deivice{} for the SPI NOR controller, such as fsl_quadspi.c.

Quoted 6 lines

It is not convenient for us to get come information from the SPI NOR flash.

This patch adds a new field @np to spi_nor{}, it points to the child node for the SPI NOR flash.

Signed-off-by Huang Shijie <[email protected]>

Just handle the case where dev->of_node == NULL instead ?

Quoted 11 lines · Marek Vasut

Quoted 10 lines · Huang Shijie

We need the SPI NOR child node to store some specific features, such as the dummy cycles for the DDR Quad read.

But now, we only have the @dev field in the spi_nor{}. The @dev may points to a spi_device{} for m25p80, while it may points to a platform_deivice{} for the SPI NOR controller, such as fsl_quadspi.c.

It is not convenient for us to get come information from the SPI NOR flash.

Quoted 6 lines

Quoted 4 lines

This patch adds a new field @np to spi_nor{}, it points to the child node for the SPI NOR flash.

Signed-off-by Huang Shijie <[email protected]>

Just handle the case where dev->of_node == NULL instead ?

It is not enough.

For the m25p80.c, @dev stands for a child node for the SPI master, and it points to a spi_device{}. Yes, in this case, the dev->of_node is NULL.

But for the fsl_quadspi or other SPI NOR drivers, the @dev stands for the controller itself, the @dev->of_node is a list of the child nodes, so we can _NOT_ know which child node we are working at now.

So it's better to add a new field @np for the spi-nor{} which points the child node we are working at.

Quoted 7 lines · Huang Shijie

For the m25p80.c, @dev stands for a child node for the SPI master, and it points to a spi_device{}. Yes, in this case, the dev->of_node is NULL.

But for the fsl_quadspi or other SPI NOR drivers, the @dev stands for the controller itself, the @dev->of_node is a list of the child nodes, so we can _NOT_ know which child node we are working at now.

Huh ? The dev is being recycled for two different kind of things ?

This patch adds the DDR quad read support by the following:

[1] add SPI_NOR_DDR_QUAD read mode.

[2] add DDR Quad read opcodes:

       SPINOR_OP_READ_1_4_4_D / SPINOR_OP_READ4_1_4_4_D

[3] add set_ddr_quad_mode() to initialize for the DDR quad read. Currently it only works for Spansion NOR.

[3] about the dummy cycles.
We set the dummy with 8 for DDR quad read by default. The m25p80.c can not support the DDR quad read, but the SPI NOR
controller can set the dummy value in its child DT node, and the SPI NOR framework can parse it out.

Test this patch for Spansion s25fl128s NOR flash.

Signed-off-by Huang Shijie <[email protected]>

This patch adds the DDR quad read support by the following:

[1] add SPI_NOR_DDR_QUAD read mode.

[2] add DDR Quad read opcodes:

       SPINOR_OP_READ_1_4_4_D / SPINOR_OP_READ4_1_4_4_D

[3] add set_ddr_quad_mode() to initialize for the DDR quad read. Currently it only works for Spansion NOR.

[3] about the dummy cycles.
We set the dummy with 8 for DDR quad read by default.

The m25p80.c can not support the DDR quad read, but the SPI NOR controller can set the dummy value in its child DT node, and the SPI NOR framework can parse it out.

Test this patch for Spansion s25fl128s NOR flash.

Signed-off-by Huang Shijie <[email protected]>

---
 drivers/mtd/spi-nor/spi-nor.c |   54 +++++++++++++++++++++++++++++++++++++++-
 include/linux/mtd/spi-nor.h   |    8 ++++-
 2 files changed, 58 insertions(+), 4 deletions(-)

drivers/mtd/spi-nor/spi-nor.c +32 -0

index f374e44..e0bc11a 100644
@@ -73,7 +73,20 @@ static int read_cr(struct spi_nor *nor)
  */
 static inline int spi_nor_read_dummy_cycles(struct spi_nor *nor)
 {
+	u32 dummy;
+
 	switch (nor->flash_read) {
+	case SPI_NOR_DDR_QUAD:
+		/*
+		 * The m25p80.c can not support the DDR quad read.
+		 * We set the dummy cycles to 8 by default. The SPI NOR
+		 * controller driver can set it in its child DT node.
+		 * We parse it out here.
+		 */
+		if (nor->np && !of_property_read_u32(nor->np,
+				"spi-nor,ddr-quad-read-dummy", &dummy)) {
+			return dummy;
+		}
 	case SPI_NOR_FAST:
 	case SPI_NOR_DUAL:
 	case SPI_NOR_QUAD:

@@ -402,6 +415,7 @@ struct flash_info {
 #define	SECT_4K_PMC		0x10	/* SPINOR_OP_BE_4K_PMC works uniformly */
 #define	SPI_NOR_DUAL_READ	0x20    /* Flash supports Dual Read */
 #define	SPI_NOR_QUAD_READ	0x40    /* Flash supports Quad Read */
+#define	SPI_NOR_DDR_QUAD_READ	0x80    /* Flash supports DDR Quad Read */
 };
 
 #define INFO(_jedec_id, _ext_id, _sector_size, _n_sectors, _flags)	\

@@ -846,6 +860,24 @@ static int spansion_quad_enable(struct spi_nor *nor)
 	return 0;
 }
 
+static int set_ddr_quad_mode(struct spi_nor *nor, u32 jedec_id)
+{
+	int status;
+
+	switch (JEDEC_MFR(jedec_id)) {
+	case CFI_MFR_AMD: /* Spansion, actually */
+		status = spansion_quad_enable(nor);
+		if (status) {
+			dev_err(nor->dev,
+				"Spansion DDR quad-read not enabled\n");
+			return status;
+		}
+		return status;
+	default:
+		return -EINVAL;
+	}
+}
+
 static int set_quad_mode(struct spi_nor *nor, u32 jedec_id)
 {
 	int status;

drivers/mtd/spi-nor/spi-nor.c +14 -0

+		}
+		nor->flash_read = SPI_NOR_DDR_QUAD;
+	} else if (mode == SPI_NOR_QUAD && info->flags & SPI_NOR_QUAD_READ) {
 		ret = set_quad_mode(nor, info->jedec_id);
 		if (ret) {
 			dev_err(dev, "quad mode not supported\n");

@@ -1030,6 +1069,14 @@ int spi_nor_scan(struct spi_nor *nor, const struct spi_device_id *id,
 
 	/* Default commands */
 	switch (nor->flash_read) {
+	case SPI_NOR_DDR_QUAD:
+		if (JEDEC_MFR(info->jedec_id) == CFI_MFR_AMD) { /* Spansion */
+			nor->read_opcode = SPINOR_OP_READ_1_4_4_D;
+		} else {
+			dev_err(dev, "DDR Quad Read is not supported.\n");
+			return -EINVAL;
+		}
+		break;
 	case SPI_NOR_QUAD:
 		nor->read_opcode = SPINOR_OP_READ_1_1_4;
 		break;

@@ -1057,6 +1104,9 @@ int spi_nor_scan(struct spi_nor *nor, const struct spi_device_id *id,
 		if (JEDEC_MFR(info->jedec_id) == CFI_MFR_AMD) {
 			/* Dedicated 4-byte command set */
 			switch (nor->flash_read) {
+			case SPI_NOR_DDR_QUAD:
+				nor->read_opcode = SPINOR_OP_READ4_1_4_4_D;
+				break;
 			case SPI_NOR_QUAD:
 				nor->read_opcode = SPINOR_OP_READ4_1_1_4;
 				break;

include/linux/mtd/spi-nor.h +5 -2

index 48fe9fc..d191a6b 100644
@@ -12,10 +12,11 @@
 
 /*
  * Note on opcode nomenclature: some opcodes have a format like
- * SPINOR_OP_FUNCTION{4,}_x_y_z. The numbers x, y, and z stand for the number
+ * SPINOR_OP_FUNCTION{4,}_x_y_z{_D}. The numbers x, y, and z stand for the number
  * of I/O lines used for the opcode, address, and data (respectively). The
  * FUNCTION has an optional suffix of '4', to represent an opcode which
- * requires a 4-byte (32-bit) address.
+ * requires a 4-byte (32-bit) address. The suffix of 'D' stands for the
+ * DDR mode.
  */
 
 /* Flash opcodes. */

@@ -26,6 +27,7 @@
 #define SPINOR_OP_READ_FAST	0x0b	/* Read data bytes (high frequency) */
 #define SPINOR_OP_READ_1_1_2	0x3b	/* Read data bytes (Dual SPI) */
 #define SPINOR_OP_READ_1_1_4	0x6b	/* Read data bytes (Quad SPI) */
+#define SPINOR_OP_READ_1_4_4_D	0xed	/* Read data bytes (DDR Quad SPI) */
 #define SPINOR_OP_PP		0x02	/* Page program (up to 256 bytes) */
 #define SPINOR_OP_BE_4K		0x20	/* Erase 4KiB block */
 #define SPINOR_OP_BE_4K_PMC	0xd7	/* Erase 4KiB block on PMC chips */

@@ -40,6 +42,7 @@
 #define SPINOR_OP_READ4_FAST	0x0c	/* Read data bytes (high frequency) */
 #define SPINOR_OP_READ4_1_1_2	0x3c	/* Read data bytes (Dual SPI) */
 #define SPINOR_OP_READ4_1_1_4	0x6c	/* Read data bytes (Quad SPI) */
+#define SPINOR_OP_READ4_1_4_4_D	0xee	/* Read data bytes (DDR Quad SPI) */
 #define SPINOR_OP_PP_4B		0x12	/* Page program (up to 256 bytes) */
 #define SPINOR_OP_SE_4B		0xdc	/* Sector erase (usually 64KiB) */
 

Quoted 1 line · Huang Shijie

This patch adds the DDR quad read support by the following:

To Mark / linux-spi:

Are DDR modes in the scope of drivers/spi/ at all, so that we could someday wire it up through m25p80.c? Or is 'DDR' such a distortion of the meaning of 'SPI' such that it will be restricted only to SPI NOR dedicated controllers?

Quoted 6 lines

[1] add SPI_NOR_DDR_QUAD read mode.

[2] add DDR Quad read opcodes:

       SPINOR_OP_READ_1_4_4_D / SPINOR_OP_READ4_1_4_4_D

Quoted 5 lines

[3] add set_ddr_quad_mode() to initialize for the DDR quad read. Currently it only works for Spansion NOR.

[3] about the dummy cycles.
We set the dummy with 8 for DDR quad read by default.

Why? That seems wrong. You need to know for sure how many cycles should be used, not just guess a default.

Quoted 4 lines

The m25p80.c can not support the DDR quad read, but the SPI NOR controller can set the dummy value in its child DT node, and the SPI NOR framework can parse it out.

Why does the dummy value belong in device tree? I think this can be handled in software. You might, however, want a few other hardware description parameters in device tree to help you.

So I think spi-nor.c needs to know a few things:

1. Does the hardware/driver support DDR clocking?
2. What granularity of dummy cycles are supported? So m25p80.c needs to communicate that it only supports dummy cycles of multiples of 8, and fsl-quadspi supports single clock cycles.

And spi-nor.c should be able to do the following:

3. Set how many dummy cycles should be used.

4. Write to the configuration register, to choose a Latency Code according to what the flash supports. I see modes that support 3, 6, 7, or 8. We'd probably just go for the fastest mode, which requires 8, right?

So far, none of this seems to require a DT binding, unless there's something I'm missing about your fsl-quadspi controller.

Quoted 2 lines

Test this patch for Spansion s25fl128s NOR flash.

drivers/mtd/spi-nor/spi-nor.c int spi_nor_scan(struct spi_nor *nor, const struct spi_device_id *id, · line 1052

Quoted 4 lines

Signed-off-by Huang Shijie <[email protected]>

---

drivers/mtd/spi-nor/spi-nor.c +2 -0

+	/* DDR Quad/Quad/Dual-read mode takes precedence over fast/normal */
+	if (mode == SPI_NOR_DDR_QUAD && info->flags & SPI_NOR_DDR_QUAD_READ) {

Hmm, I think I should probably take another look at the design of spi-nor.c... Why does spi_nor_scan() take a single 'mode' argument? The driver should be communicating which (multiple) modes it supports, not selecting a single mode. spi-nor.c is the only one which knows what the *flash* supports, so it should be combining knowledge from the controller driver with its own knowledge of the flash.

drivers/mtd/spi-nor/spi-nor.c int spi_nor_scan(struct spi_nor *nor, const struct spi_device_id *id, · line 1056

Quoted 5 lines

drivers/mtd/spi-nor/spi-nor.c +4 -0

+		ret = set_ddr_quad_mode(nor, info->jedec_id);
+		if (ret) {
+			dev_err(dev, "DDR quad mode not supported\n");
+			return ret;

A ramification of my comment above is that we should not be returning an error in a situation like this; we should be able to fall back to another known-supported mode, like SDR QUAD, SDR DUAL, or just plain SPI -- if they're supported by the driver -- and spi-nor.c doesn't currently have enough information to do this.

drivers/mtd/spi-nor/spi-nor.c int spi_nor_scan(struct spi_nor *nor, const struct spi_device_id *id,

Quoted 5 lines

drivers/mtd/spi-nor/spi-nor.c +2 -0

+		}
+		nor->flash_read = SPI_NOR_DDR_QUAD;
 
 /**

So, I'll have to take another hard look at spi-nor.c soon. I think we may need to work on the abstractions here a bit more before I merge any new features like this.

Regards,
Brian

P.S. Is there a good reason we've defined a whole read_xfer/write_xfer API that is completely unused?

Quoted 3 lines · Brian Norris

Quoted 1 line · Huang Shijie

This patch adds the DDR quad read support by the following:

Quoted 6 lines

To Mark / linux-spi:

Are DDR modes in the scope of drivers/spi/ at all, so that we could someday wire it up through m25p80.c? Or is 'DDR' such a distortion of the meaning of 'SPI' such that it will be restricted only to SPI NOR dedicated controllers?

IMHO, the DDR modes can _NOT_ be handled by the driver/spi/*.

The SPI can only handles the byte streams.

But the DDR mode may need to
handle the cycles(such as the dummy cycles could be 7 cycles) which is not byte.

So the DDR mode is handled by the SPI NOR controller now.

Please correct me if I am wrong. :)

Quoted 8 lines

Quoted 8 lines

[1] add SPI_NOR_DDR_QUAD read mode.

[2] add DDR Quad read opcodes:

       SPINOR_OP_READ_1_4_4_D / SPINOR_OP_READ4_1_4_4_D

[3] add set_ddr_quad_mode() to initialize for the DDR quad read. Currently it only works for Spansion NOR.

Quoted 5 lines

Quoted 2 lines

[3] about the dummy cycles.
We set the dummy with 8 for DDR quad read by default.

Why? That seems wrong. You need to know for sure how many cycles should be used, not just guess a default.

Do you mean that if people do not set the DT node for dummy, we should return an -EINVAL immediately?

Quoted 6 lines

Quoted 3 lines

The m25p80.c can not support the DDR quad read, but the SPI NOR controller can set the dummy value in its child DT node, and the SPI NOR framework can parse it out.

Why does the dummy value belong in device tree? I think this can be handled in software.

Quoted 5 lines

You might, however, want a few other hardware description parameters in device tree to help you.

So I think spi-nor.c needs to know a few things:

Quoted 4 lines

1. Does the hardware/driver support DDR clocking?
2. What granularity of dummy cycles are supported? So m25p80.c needs to communicate that it only supports dummy cycles of multiples of 8, and fsl-quadspi supports single clock cycles.

I think you can send patches for these features. I does not clear about:
for what does the spi-nor needs to know the above things.

Quoted 3 lines

And spi-nor.c should be able to do the following:

3. Set how many dummy cycles should be used.

where can we get the number of the cycles?

Quoted 4 lines

4. Write to the configuration register, to choose a Latency Code according to what the flash supports. I see modes that support 3, 6, 7, or 8. We'd probably just go for the fastest mode, which requires 8, right?

not right.

The DDR mode can not work if we set a wrong dummy cycles for the flash.

for some chips, the fastest mode may need 6 cycles, not 8.

Quoted 5 lines

So far, none of this seems to require a DT binding, unless there's something I'm missing about your fsl-quadspi controller.

Quoted 2 lines

Test this patch for Spansion s25fl128s NOR flash.

Quoted 11 lines

drivers/mtd/spi-nor/spi-nor.c int spi_nor_scan(struct spi_nor *nor, const struct spi_device_id *id, · line 1052

Quoted 4 lines

Signed-off-by Huang Shijie <[email protected]>

---

drivers/mtd/spi-nor/spi-nor.c +2 -0

+	/* DDR Quad/Quad/Dual-read mode takes precedence over fast/normal */
+	if (mode == SPI_NOR_DDR_QUAD && info->flags & SPI_NOR_DDR_QUAD_READ) {

Hmm, I think I should probably take another look at the design of spi-nor.c... Why does spi_nor_scan() take a single 'mode' argument? The driver should be communicating which (multiple) modes it supports, not selecting a single mode. spi-nor.c is the only one which knows what the *flash* supports, so it should be combining knowledge from the controller driver with its own knowledge of the flash.

It is okay for me to add multiples modes for the spi_nor_scan(). I added the single mode for spi_nor_scan is because that the fsl-quadspi does not want to support the low speed modes. (Of course, the fsl-quadspi controller does support the low speed modes.)

Quoted 10 lines

drivers/mtd/spi-nor/spi-nor.c int spi_nor_scan(struct spi_nor *nor, const struct spi_device_id *id, · line 1056

Quoted 4 lines

drivers/mtd/spi-nor/spi-nor.c +4 -0

+		ret = set_ddr_quad_mode(nor, info->jedec_id);
+		if (ret) {
+			dev_err(dev, "DDR quad mode not supported\n");
+			return ret;

A ramification of my comment above is that we should not be returning an error in a situation like this; we should be able to fall back to another known-supported mode, like SDR QUAD, SDR DUAL, or just plain SPI -- if they're supported by the driver -- and spi-nor.c doesn't currently have enough information to do this.

ok.

Quoted 9 lines

drivers/mtd/spi-nor/spi-nor.c int spi_nor_scan(struct spi_nor *nor, const struct spi_device_id *id,

Quoted 5 lines

drivers/mtd/spi-nor/spi-nor.c +2 -0

+		}
+		nor->flash_read = SPI_NOR_DDR_QUAD;
 
 /**

So, I'll have to take another hard look at spi-nor.c soon. I think we may need to work on the abstractions here a bit more before I merge any new features like this.

okay. no problem.

Quoted 1 line

Quoted 5 lines

Regards,
Brian

P.S. Is there a good reason we've defined a whole read_xfer/write_xfer API that is completely unused?

These hooks are designed for other SPI NOR drivers, the fsl-quadspi does not use them.

Quoted 8 lines · Huang Shijie

Quoted 7 lines · Brian Norris

Quoted 1 line · Huang Shijie

This patch adds the DDR quad read support by the following:

Are DDR modes in the scope of drivers/spi/ at all, so that we could someday wire it up through m25p80.c? Or is 'DDR' such a distortion of the meaning of 'SPI' such that it will be restricted only to SPI NOR dedicated controllers?

Quoted 1 line

IMHO, the DDR modes can _NOT_ be handled by the driver/spi/*.

I agree to some extent, but I wanted to confirm with the SPI guys that DDR is truly unique to SPI NOR. (I know it doesn't currently support it.)

Quoted 6 lines · Brian Norris

Quoted 1 line · Huang Shijie

IMHO, the DDR modes can _NOT_ be handled by the driver/spi/*.

I agree to some extent, but I wanted to confirm with the SPI guys that DDR is truly unique to SPI NOR. (I know it doesn't currently support it.)

I don't know what DDR is in this context, sorry.

I'm guessing you're
right since it sounds like something to do with extra clocks and this is probably not something used by generic SPI devices at present (if it ends up being widely implemented by sufficiently generic controllers that might change but the trend seems to be to flash specific controllers).

Quoted 9 lines · Mark Brown

Quoted 6 lines · Brian Norris

Quoted 1 line · Huang Shijie

IMHO, the DDR modes can _NOT_ be handled by the driver/spi/*.

I agree to some extent, but I wanted to confirm with the SPI guys that DDR is truly unique to SPI NOR. (I know it doesn't currently support it.)

I don't know what DDR is in this context, sorry.

I think it's just the ability to latch data on both the rising and falling edges of the SPI clock. For SPI flash, it seems to be used for the data portion of the opcode/address/data sequence.

Quoted 6 lines

I'm guessing you're
right since it sounds like something to do with extra clocks and this is probably not something used by generic SPI devices at present (if it ends up being widely implemented by sufficiently generic controllers that might change but the trend seems to be to flash specific controllers).

OK, thanks for chiming in.

Yeah, I suppose it could be wedged in later if drivers/spi/ ever adopts a solution.

On Sat, Aug 2, 2014 at 4:06 AM, Brian Norris

Quoted 3 lines · Mark Brown

Quoted 2 lines

I don't know what DDR is in this context, sorry.

Quoted 6 lines

I think it's just the ability to latch data on both the rising and falling edges of the SPI clock. For SPI flash, it seems to be used for the data portion of the opcode/address/data sequence.

Yeah, I suppose it could be wedged in later if drivers/spi/ ever adopts a solution.

I think this can just be another SPI_* spi_device.mode flag.

Do we need bindings for this in Documentation/devicetree/bindings/spi/spi-bus.txt?
Unlike Quad SPI transfer support, this doesn't need special wiring, so DDR capability is an intrinsic property of the SPI slave, and the mode bit can just be set in the SPI slave driver, without any DT magic?

Quoted 5 lines · Geert Uytterhoeven

On Sat, Aug 2, 2014 at 4:06 AM, Brian Norris

Quoted 3 lines · Mark Brown

Quoted 2 lines

I don't know what DDR is in this context, sorry.

Quoted 8 lines

Quoted 6 lines

I think it's just the ability to latch data on both the rising and falling edges of the SPI clock. For SPI flash, it seems to be used for the data portion of the opcode/address/data sequence.

Yeah, I suppose it could be wedged in later if drivers/spi/ ever adopts a solution.

I think this can just be another SPI_* spi_device.mode flag.

Sounds like it yes - I was wondering if this might be one of the modes with extra clock cycles that I've heard mentioned before which might be a little more fun.

Quoted 5 lines

Do we need bindings for this in Documentation/devicetree/bindings/spi/spi-bus.txt?
Unlike Quad SPI transfer support, this doesn't need special wiring, so DDR capability is an intrinsic property of the SPI slave, and the mode bit can just be set in the SPI slave driver, without any DT magic?

Right, unless we run into things like board design issues causing constraints this is something that can be enabled by the two drivers without needing DT configuration.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 25 lines · Mark Brown

Quoted 5 lines · Geert Uytterhoeven

On Sat, Aug 2, 2014 at 4:06 AM, Brian Norris

Quoted 3 lines · Mark Brown

Quoted 2 lines

I don't know what DDR is in this context, sorry.

Quoted 8 lines

Quoted 6 lines

I think it's just the ability to latch data on both the rising and falling edges of the SPI clock. For SPI flash, it seems to be used for the data portion of the opcode/address/data sequence.

Yeah, I suppose it could be wedged in later if drivers/spi/ ever adopts a solution.

I think this can just be another SPI_* spi_device.mode flag.

Sounds like it yes - I was wondering if this might be one of the modes with extra clock cycles that I've heard mentioned before which might be a little more fun.

Quoted 5 lines

Do we need bindings for this in Documentation/devicetree/bindings/spi/spi-bus.txt?
Unlike Quad SPI transfer support, this doesn't need special wiring, so DDR capability is an intrinsic property of the SPI slave, and the mode bit can just be set in the SPI slave driver, without any DT magic?

Quoted 3 lines

Right, unless we run into things like board design issues causing constraints this is something that can be enabled by the two drivers without needing DT configuration.

All:

we plan resume this work.
I need direction how go ahead further.

I go through this email thread.

The discussion focus on how to get dummy cycle information.

Shijie get it from DT.

Brain suggest get from CFI or map id table if I understand correct.

Accodring to spec
http://www.spansion.com/Support/Datasheets/S25FL128S_256S_00.pdf

Table 8.10

Dummy cycle depend on frequency, read command.

if information saved in driver, it will be huge table.

Quoted 6 lines

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

BTW, does your quadspi controller unconditionally support DDR, or is there any dependency on board/clock configuration? I'm just curious whether you need a DT binding to describe DDR support, or if (as long as the flash supports it, and we get the dummy cycles right) you can always use DDR QUAD.

Quoted 14 lines

Please correct me if I am wrong. :)

Quoted 11 lines

Quoted 10 lines

[1] add SPI_NOR_DDR_QUAD read mode.

[2] add DDR Quad read opcodes:

       SPINOR_OP_READ_1_4_4_D / SPINOR_OP_READ4_1_4_4_D

[3] add set_ddr_quad_mode() to initialize for the DDR quad read. Currently it only works for Spansion NOR.

[3] about the dummy cycles.
We set the dummy with 8 for DDR quad read by default.

Quoted 5 lines

Quoted 2 lines

Why? That seems wrong. You need to know for sure how many cycles should be used, not just guess a default.

Do you mean that if people do not set the DT node for dummy, we should return an -EINVAL immediately?

Possibly. But I actually don't think this belongs in DT at all. See below.

Quoted 6 lines

Quoted 6 lines

Quoted 3 lines

The m25p80.c can not support the DDR quad read, but the SPI NOR controller can set the dummy value in its child DT node, and the SPI NOR framework can parse it out.

Why does the dummy value belong in device tree? I think this can be handled in software.

Can you answer me this question?

Quoted 5 lines

Quoted 5 lines

You might, however, want a few other hardware description parameters in device tree to help you.

So I think spi-nor.c needs to know a few things:

Quoted 7 lines

Quoted 4 lines

1. Does the hardware/driver support DDR clocking?
2. What granularity of dummy cycles are supported? So m25p80.c needs to communicate that it only supports dummy cycles of multiples of 8, and fsl-quadspi supports single clock cycles.

I think you can send patches for these features. I does not clear about:
for what does the spi-nor needs to know the above things.

To properly abstract features between a driver and the spi-nor "library." For example, we need to make sure we don't try to use a command mode with 7 dummy cycles on m25p80.c; right now, each driver has to (implicitly) know the details of dummy cycles when selecting a 'mode' parameter for spi_nor_scan(). spi-nor.c should be selecting this for us, not forcing the driver to make the selection.

Quoted 4 lines

Quoted 3 lines

And spi-nor.c should be able to do the following:

3. Set how many dummy cycles should be used.

where can we get the number of the cycles?

This should be a property of the flash device, just like everything else we detect in spi-nor.c.

Quoted 5 lines

Quoted 5 lines

4. Write to the configuration register, to choose a Latency Code according to what the flash supports. I see modes that support 3, 6, 7, or 8. We'd probably just go for the fastest mode, which requires 8, right?

Quoted 5 lines

not right.

The DDR mode can not work if we set a wrong dummy cycles for the flash.

for some chips, the fastest mode may need 6 cycles, not 8.

OK, but can spi-nor.c detect this instead of putting this in DT? e.g., associate this with the device ID?

Or even better, we can support CFI detection for SPI NOR flash. I notice the datasheet for your Spansion flash [1] has an extensive set of CFI parameters defined, including the dummy cycle information. I think it might be more sustainable to try to support CFI [2] and SFDP [3] for detecting new flash, rather than adding new table entries ad-hoc.

Quoted 18 lines

Quoted 6 lines

So far, none of this seems to require a DT binding, unless there's something I'm missing about your fsl-quadspi controller.

Quoted 2 lines

Test this patch for Spansion s25fl128s NOR flash.

Quoted 11 lines

drivers/mtd/spi-nor/spi-nor.c int spi_nor_scan(struct spi_nor *nor, const struct spi_device_id *id, · line 1052

Quoted 4 lines

Signed-off-by Huang Shijie <[email protected]>

---

drivers/mtd/spi-nor/spi-nor.c +2 -0

+	/* DDR Quad/Quad/Dual-read mode takes precedence over fast/normal */
+	if (mode == SPI_NOR_DDR_QUAD && info->flags & SPI_NOR_DDR_QUAD_READ) {

Hmm, I think I should probably take another look at the design of spi-nor.c... Why does spi_nor_scan() take a single 'mode' argument? The driver should be communicating which (multiple) modes it supports, not selecting a single mode. spi-nor.c is the only one which knows what the *flash* supports, so it should be combining knowledge from the controller driver with its own knowledge of the flash.

Quoted 4 lines

It is okay for me to add multiples modes for the spi_nor_scan(). I added the single mode for spi_nor_scan is because that the fsl-quadspi does not want to support the low speed modes. (Of course, the fsl-quadspi controller does support the low speed modes.)

Right, fsl-quadspi only supports one mode. But m25p80.c is more flexible, and I think we might have broken some of it in the process (e.g., if the SPI controller supports single/dual/quad but the flash only supports single, then we might fail to probe).

I can take a look at this problem if you don't. I'd just recommend that we might take a step back on a few of these issues before blazing ahead with something irrevocable, like a DT binding.

From the datasheet, the chip support the 64K sector erase operation.