Hi Marek,

Le 24/08/2015 12:48, Marek Vasut a ?crit :

On Monday, August 24, 2015 at 12:13:58 PM, Cyrille Pitchen wrote:

quoted

The number of dummy cycles used during Fast Read commands can be reduced
to improve transfer performances. Each manufacturer has a dedicated set of
registers to provide the memory with the exact number of dummy cycles it
should expect. Both the memory and the (Q)SPI controller must agree on
this number of dummy cycles.

The number of dummy cycles can be found into the memory datasheet and
mostly depends on the SPI clock frequency, the Fast Read op code and the
Single/Dual Data Rate mode.

Probing JEDEC Serial Flash Discoverable Parameters (SFDP) tables would
only provide the driver with a high enough number of dummy cycles for each
Fast Read command to be used for all clock frequencies: this solution
would not be optimized.

Signed-off-by: Cyrille Pitchen <redacted>

Hi!

quoted

 drivers/mtd/spi-nor/spi-nor.c | 97
++++++++++++++++++++++++++++++++++--------- include/linux/mtd/spi-nor.h  
|  2 +
 2 files changed, 80 insertions(+), 19 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index e2a6029dc056..869e098a6841 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c

@@ -119,24 +119,6 @@ static int read_cr(struct spi_nor *nor)
 }

 /*
- * Dummy Cycle calculation for different type of read.
- * It can be used to support more commands with
- * different dummy cycle requirements.
- */
-static inline int spi_nor_read_dummy_cycles(struct spi_nor *nor)
-{
-	switch (nor->flash_read) {
-	case SPI_NOR_FAST:
-	case SPI_NOR_DUAL:
-	case SPI_NOR_QUAD:
-		return 8;
-	case SPI_NOR_NORMAL:
-		return 0;
-	}
-	return 0;
-}

You can probably just soup up this function so that it sets the
nor->read_dummy, no ?

Actually, this is what the patch does: spi_nor_read_dummy_cycles() was reused
and enhanced few lines below where you've pointed out the 
"switch (nor->flash_read)" block should be move after the else block.

I think when I wrote the code I've chosen to move the definition of this
function instead of adding forward declarations of functions such as read_cr()
or write_sr_cr(), which are now called by micron_set_dummy_cycles().

quoted

-/*
  * Write status register 1 byte
  * Returns negative if error occurred.
  */

@@ -1012,6 +994,81 @@ static int set_quad_mode(struct spi_nor *nor, struct

flash_info *info) }
 }

+static int micron_set_dummy_cycles(struct spi_nor *nor)
+{
+	int ret;
+	u8 val, mask;
+
+	/* read the Volatile Configuration Register (VCR) */

NIT: If this is a sentence, start it with capital letter and end it with 
fullstop :)

done for the next version

quoted

+	ret = nor->read_reg(nor, SPINOR_OP_RD_VCR, &val, 1);
+	if (ret < 0) {
+		dev_err(nor->dev, "error %d reading VCR\n", ret);
+		return ret;
+	}
+
+	write_enable(nor);
+
+	/* update the number of dummy into the VCR */

DTTO

done for the next version

quoted

+	mask = GENMASK(7, 4);
+	val &= ~mask;
+	val |= (nor->read_dummy << 4) & mask;
+	ret = nor->write_reg(nor, SPINOR_OP_WR_VCR, &val, 1, 0);
+	if (ret < 0) {
+		dev_err(nor->dev, "error while writing VCR register\n");
+		return ret;
+	}
+
+	ret = spi_nor_wait_till_ready(nor);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+/*
+ * Dummy Cycle calculation for different type of read.
+ * It can be used to support more commands with
+ * different dummy cycle requirements.
+ */
+static int spi_nor_read_dummy_cycles(struct spi_nor *nor,
+				     const struct flash_info *info)
+{
+	struct device_node *np = nor->dev->of_node;
+	u32 num_dummy_cycles;
+
+	if (np && !of_property_read_u32(np, "m25p,num-dummy-cycles",
+					&num_dummy_cycles)) {
+		nor->read_dummy = num_dummy_cycles;
+
+		/*
+		 * This switch block might be moved after the if...then...else
+		 * statement but it was not tested with all Spansion or Micron
+		 * memories.
+		 * Now the "m25p,num-dummy-cycles" property needs to be
+		 * explicitly set in the device tree so the switch statement is
+		 * executed. This should avoid unwanted side effects and keep
+		 * backward compatibility.
+		 */
+		switch (JEDEC_MFR(info)) {
+		case CFI_MFR_ST:
+			return micron_set_dummy_cycles(nor);
+		default:

If you do have m25p,num-dummy-cycles set for non-micron flash, you have a 
problem here I believe.

quoted

+			break;
+		}
+	} else {

The solution would be to drop this else {} bit here, so that if you fail in
the DT-based configuration, you fall back to this old behavior. What do you 
think please ? :)

Good idea!
I also add a trace for the default case of "switch (JEDEC_MFR(info))":

dev_warn(dev, "can't set the number of dummy cycles\n");

So the user is notified that the driver could not use the value of
"m25p,num-dummy-cycles" from the DT before falling back to the legacy
code.

quoted

+		switch (nor->flash_read) {
+		case SPI_NOR_FAST:
+		case SPI_NOR_DUAL:
+		case SPI_NOR_QUAD:
+			nor->read_dummy = 8;
+		case SPI_NOR_NORMAL:
+			nor->read_dummy = 0;
+		}
+	}
+
+	return 0;
+}

[...]

thanks for the review!

Best regards,

Cyrille