[PATCHv3 0/2] m25p80: QUAD read support + cleanup.

[PATCHv3 0/2] m25p80: QUAD read support + cleanup.

[Sourav Poddar]

Patch series does the following:
1. Cleanup the m25p80 driver to convert bool check for
   read into an enum. This will help adding more read 
   commands into the driver easily.

2. Add quad read support for spansion and macronix flash devices.

Sourav Poddar (2):
  drivers: mtd: m25p80: convert "bool" read type into an enum
  drivers: mtd: m25p80: Add quad read support.

 drivers/mtd/devices/m25p80.c |  220 ++++++++++++++++++++++++++++++++++++++----
 1 files changed, 202 insertions(+), 18 deletions(-)

[PATCH 1/2] drivers: mtd: m25p80: convert "bool" read check into an enum

[Sourav Poddar]

This is a cleanup prior to adding quad read support. This will fecilitate 
easy addition of more read commands check under an enum rather that defining a 
seperate bool for it.

Signed-off-by: Sourav Poddar <sourav.poddar@ti.com>
Suggested-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/mtd/devices/m25p80.c |   71 +++++++++++++++++++++++++++++++++--------
 1 files changed, 57 insertions(+), 14 deletions(-)

diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index 7eda71d..cfafdce 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -84,6 +84,11 @@
 
 /****************************************************************************/
 
+enum read_type {
+	M25P80_NORMAL = 0,
+	M25P80_FAST,
+};
+
 struct m25p {
 	struct spi_device	*spi;
 	struct mutex		lock;
@@ -94,7 +99,7 @@ struct m25p {
 	u8			read_opcode;
 	u8			program_opcode;
 	u8			*command;
-	bool			fast_read;
+	enum read_type		flash_read;
 };
 
 static inline struct m25p *mtd_to_m25p(struct mtd_info *mtd)
@@ -350,6 +355,24 @@ static int m25p80_erase(struct mtd_info *mtd, struct erase_info *instr)
 }
 
 /*
+ * Dummy Cycle calculation for different type of read.
+ * It can be used to support more commands with
+ * different dummy cycle requirement.
+ */
+static inline int m25p80_dummy_cycles_read(struct m25p *flash)
+{
+	switch (flash->flash_read) {
+	case M25P80_FAST:
+		return 1;
+	case M25P80_NORMAL:
+		return 0;
+	default:
+		dev_err(&flash->spi->dev, "No valid read type supported");
+		return -1;
+	}
+}
+
+/*
  * Read an address range from the flash chip.  The address range
  * may be any size provided it is within the physical boundaries.
  */
@@ -360,6 +383,7 @@ static int m25p80_read(struct mtd_info *mtd, loff_t from, size_t len,
 	struct spi_transfer t[2];
 	struct spi_message m;
 	uint8_t opcode;
+	int dummy;
 
 	pr_debug("%s: %s from 0x%08x, len %zd\n", dev_name(&flash->spi->dev),
 			__func__, (u32)from, len);
@@ -367,8 +391,14 @@ static int m25p80_read(struct mtd_info *mtd, loff_t from, size_t len,
 	spi_message_init(&m);
 	memset(t, 0, (sizeof t));
 
+	dummy =  m25p80_dummy_cycles_read(flash);
+	if (dummy < 0) {
+		dev_err(&flash->spi->dev, "No valid read command supported");
+		return -EINVAL;
+	}
+
 	t[0].tx_buf = flash->command;
-	t[0].len = m25p_cmdsz(flash) + (flash->fast_read ? 1 : 0);
+	t[0].len = m25p_cmdsz(flash) + dummy;
 	spi_message_add_tail(&t[0], &m);
 
 	t[1].rx_buf = buf;
@@ -391,8 +421,7 @@ static int m25p80_read(struct mtd_info *mtd, loff_t from, size_t len,
 
 	spi_sync(flash->spi, &m);
 
-	*retlen = m.actual_length - m25p_cmdsz(flash) -
-			(flash->fast_read ? 1 : 0);
+	*retlen = m.actual_length - m25p_cmdsz(flash) - dummy;
 
 	mutex_unlock(&flash->lock);
 
@@ -1051,22 +1080,31 @@ static int m25p_probe(struct spi_device *spi)
 	flash->page_size = info->page_size;
 	flash->mtd.writebufsize = flash->page_size;
 
-	if (np)
+	if (np) {
 		/* If we were instantiated by DT, use it */
-		flash->fast_read = of_property_read_bool(np, "m25p,fast-read");
-	else
+		if (of_property_read_bool(np, "m25p,fast-read"))
+			flash->flash_read = M25P80_FAST;
+	} else {
 		/* If we weren't instantiated by DT, default to fast-read */
-		flash->fast_read = true;
+		flash->flash_read = M25P80_FAST;
+	}
 
 	/* Some devices cannot do fast-read, no matter what DT tells us */
 	if (info->flags & M25P_NO_FR)
-		flash->fast_read = false;
+		flash->flash_read = M25P80_NORMAL;
 
 	/* Default commands */
-	if (flash->fast_read)
+	switch (flash->flash_read) {
+	case M25P80_FAST:
 		flash->read_opcode = OPCODE_FAST_READ;
-	else
+		break;
+	case M25P80_NORMAL:
 		flash->read_opcode = OPCODE_NORM_READ;
+		break;
+	default:
+		dev_err(&flash->spi->dev, "No Read opcode defined");
+		return -EINVAL;
+	}
 
 	flash->program_opcode = OPCODE_PP;
 
@@ -1077,9 +1115,14 @@ static int m25p_probe(struct spi_device *spi)
 		flash->addr_width = 4;
 		if (JEDEC_MFR(info->jedec_id) == CFI_MFR_AMD) {
 			/* Dedicated 4-byte command set */
-			flash->read_opcode = flash->fast_read ?
-				OPCODE_FAST_READ_4B :
-				OPCODE_NORM_READ_4B;
+			switch (flash->flash_read) {
+			case M25P80_FAST:
+				flash->read_opcode = OPCODE_FAST_READ_4B;
+				break;
+			case M25P80_NORMAL:
+				flash->read_opcode = OPCODE_NORM_READ_4B;
+				break;
+			}
 			flash->program_opcode = OPCODE_PP_4B;
 			/* No small sector erase for 4-byte command set */
 			flash->erase_opcode = OPCODE_SE_4B;
-- 
1.7.1

[PATCHv3 2/2] drivers: mtd: m25p80: Add quad read support.

[Sourav Poddar]

Some flash also support quad read mode.
Adding support for adding quad mode in m25p80
for spansion and macronix flash.

Signed-off-by: Sourav Poddar <sourav.poddar@ti.com>
---
v2->v3:
Re arrange the code based on the cleanup done as
first patch of the series.
 drivers/mtd/devices/m25p80.c |  149 ++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 145 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index cfafdce..7460fb3 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -41,6 +41,7 @@
 #define	OPCODE_WRSR		0x01	/* Write status register 1 byte */
 #define	OPCODE_NORM_READ	0x03	/* Read data bytes (low frequency) */
 #define	OPCODE_FAST_READ	0x0b	/* Read data bytes (high frequency) */
+#define	OPCODE_QUAD_READ        0x6b    /* Read data bytes */
 #define	OPCODE_PP		0x02	/* Page program (up to 256 bytes) */
 #define	OPCODE_BE_4K		0x20	/* Erase 4KiB block */
 #define	OPCODE_BE_4K_PMC	0xd7	/* Erase 4KiB block on PMC chips */
@@ -48,10 +49,12 @@
 #define	OPCODE_CHIP_ERASE	0xc7	/* Erase whole flash chip */
 #define	OPCODE_SE		0xd8	/* Sector erase (usually 64KiB) */
 #define	OPCODE_RDID		0x9f	/* Read JEDEC ID */
+#define	OPCODE_RDCR             0x35    /* Read configuration register */
 
 /* 4-byte address opcodes - used on Spansion and some Macronix flashes. */
 #define	OPCODE_NORM_READ_4B	0x13	/* Read data bytes (low frequency) */
 #define	OPCODE_FAST_READ_4B	0x0c	/* Read data bytes (high frequency) */
+#define	OPCODE_QUAD_READ_4B	0x6c    /* Read data bytes */
 #define	OPCODE_PP_4B		0x12	/* Page program (up to 256 bytes) */
 #define	OPCODE_SE_4B		0xdc	/* Sector erase (usually 64KiB) */
 
@@ -76,6 +79,11 @@
 #define	SR_BP2			0x10	/* Block protect 2 */
 #define	SR_SRWD			0x80	/* SR write protect */
 
+#define SR_QUAD_EN_MX           0x40    /* Macronix Quad I/O */
+
+/* Configuration Register bits. */
+#define CR_QUAD_EN_SPAN		0x2     /* Spansion Quad I/O */
+
 /* Define max times to check status register before we give up. */
 #define	MAX_READY_WAIT_JIFFIES	(40 * HZ)	/* M25P16 specs 40s max chip erase */
 #define	MAX_CMD_SIZE		6
@@ -87,6 +95,7 @@
 enum read_type {
 	M25P80_NORMAL = 0,
 	M25P80_FAST,
+	M25P80_QUAD,
 };
 
 struct m25p {
@@ -136,6 +145,26 @@ static int read_sr(struct m25p *flash)
 }
 
 /*
+ * Read configuration register, returning its value in the
+ * location. Return the configuration register value.
+ * Returns negative if error occured.
+ */
+static int read_cr(struct m25p *flash)
+{
+	u8 code = OPCODE_RDCR;
+	int ret;
+	u8 val;
+
+	ret = spi_write_then_read(flash->spi, &code, 1, &val, 1);
+	if (ret < 0) {
+		dev_err(&flash->spi->dev, "error %d reading CR\n", ret);
+		return ret;
+	}
+
+	return val;
+}
+
+/*
  * Write status register 1 byte
  * Returns negative if error occurred.
  */
@@ -225,6 +254,95 @@ static int wait_till_ready(struct m25p *flash)
 }
 
 /*
+ * Write status Register and configuration register with 2 bytes
+ * The first byte will be written to the status register, while the
+ * second byte will be written to the configuration register.
+ * Return negative if error occured.
+ */
+static int write_sr_cr(struct m25p *flash, u16 val)
+{
+	flash->command[0] = OPCODE_WRSR;
+	flash->command[1] = val & 0xff;
+	flash->command[2] = (val >> 8);
+
+	return spi_write(flash->spi, flash->command, 3);
+}
+
+static int macronix_quad_enable(struct m25p *flash)
+{
+	int ret, val;
+	u8 cmd[2];
+	cmd[0] = OPCODE_WRSR;
+
+	val = read_sr(flash);
+	cmd[1] = val | SR_QUAD_EN_MX;
+	write_enable(flash);
+
+	spi_write(flash->spi, &cmd, 2);
+
+	if (wait_till_ready(flash))
+		return 1;
+
+	ret = read_sr(flash);
+	if (!(ret > 0 && (ret & SR_QUAD_EN_MX))) {
+		dev_err(&flash->spi->dev,
+			"Macronix Quad bit not set");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int spansion_quad_enable(struct m25p *flash)
+{
+	int ret;
+	int quad_en = CR_QUAD_EN_SPAN << 8;
+
+	write_enable(flash);
+
+	ret = write_sr_cr(flash, quad_en);
+	if (ret < 0) {
+		dev_err(&flash->spi->dev,
+			"error while writing configuration register");
+		return -EINVAL;
+	}
+
+	/* read back and check it */
+	ret = read_cr(flash);
+	if (!(ret > 0 && (ret & CR_QUAD_EN_SPAN))) {
+		dev_err(&flash->spi->dev,
+			"Spansion Quad bit not set");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static inline int set_quad_mode(struct m25p *flash, u32 jedec_id)
+{
+	int status;
+
+	switch (JEDEC_MFR(jedec_id)) {
+	case CFI_MFR_MACRONIX:
+		status = macronix_quad_enable(flash);
+		if (status) {
+			dev_err(&flash->spi->dev,
+				"Macronix quad-read not enabled");
+			return -EINVAL;
+		}
+		return status;
+	default:
+		status = spansion_quad_enable(flash);
+		if (status) {
+			dev_err(&flash->spi->dev,
+				"Spansion quad-read not enabled");
+			return -EINVAL;
+		}
+		return status;
+	}
+}
+
+/*
  * Erase the whole flash memory
  *
  * Returns 0 if successful, non-zero otherwise.
@@ -363,6 +481,7 @@ static inline int m25p80_dummy_cycles_read(struct m25p *flash)
 {
 	switch (flash->flash_read) {
 	case M25P80_FAST:
+	case M25P80_QUAD:
 		return 1;
 	case M25P80_NORMAL:
 		return 0;
@@ -397,6 +516,7 @@ static int m25p80_read(struct mtd_info *mtd, loff_t from, size_t len,
 		return -EINVAL;
 	}
 
+
 	t[0].tx_buf = flash->command;
 	t[0].len = m25p_cmdsz(flash) + dummy;
 	spi_message_add_tail(&t[0], &m);
@@ -727,6 +847,7 @@ struct flash_info {
 #define	SST_WRITE	0x04		/* use SST byte programming */
 #define	M25P_NO_FR	0x08		/* Can't do fastread */
 #define	SECT_4K_PMC	0x10		/* OPCODE_BE_4K_PMC works uniformly */
+#define	M25P80_QUAD_READ	0x20    /* Flash supports Quad Read */
 };
 
 #define INFO(_jedec_id, _ext_id, _sector_size, _n_sectors, _flags)	\
@@ -804,7 +925,7 @@ static const struct spi_device_id m25p_ids[] = {
 	{ "mx25l12855e", INFO(0xc22618, 0, 64 * 1024, 256, 0) },
 	{ "mx25l25635e", INFO(0xc22019, 0, 64 * 1024, 512, 0) },
 	{ "mx25l25655e", INFO(0xc22619, 0, 64 * 1024, 512, 0) },
-	{ "mx66l51235l", INFO(0xc2201a, 0, 64 * 1024, 1024, 0) },
+	{ "mx66l51235l", INFO(0xc2201a, 0, 64 * 1024, 1024, M25P80_QUAD_READ) },
 
 	/* Micron */
 	{ "n25q064",     INFO(0x20ba17, 0, 64 * 1024,  128, 0) },
@@ -824,7 +945,7 @@ static const struct spi_device_id m25p_ids[] = {
 	{ "s25sl032p",  INFO(0x010215, 0x4d00,  64 * 1024,  64, 0) },
 	{ "s25sl064p",  INFO(0x010216, 0x4d00,  64 * 1024, 128, 0) },
 	{ "s25fl256s0", INFO(0x010219, 0x4d00, 256 * 1024, 128, 0) },
-	{ "s25fl256s1", INFO(0x010219, 0x4d01,  64 * 1024, 512, 0) },
+	{ "s25fl256s1", INFO(0x010219, 0x4d01,  64 * 1024, 512, M25P80_QUAD_READ) },
 	{ "s25fl512s",  INFO(0x010220, 0x4d00, 256 * 1024, 256, 0) },
 	{ "s70fl01gs",  INFO(0x010221, 0x4d00, 256 * 1024, 256, 0) },
 	{ "s25sl12800", INFO(0x012018, 0x0300, 256 * 1024,  64, 0) },
@@ -966,6 +1087,7 @@ static int m25p_probe(struct spi_device *spi)
 	unsigned			i;
 	struct mtd_part_parser_data	ppdata;
 	struct device_node *np = spi->dev.of_node;
+	int ret;
 
 	/* Platform data helps sort out which chip type we have, as
 	 * well as how this board partitions it.  If we don't have
@@ -1080,6 +1202,15 @@ static int m25p_probe(struct spi_device *spi)
 	flash->page_size = info->page_size;
 	flash->mtd.writebufsize = flash->page_size;
 
+	if (spi->mode & SPI_RX_QUAD && info->flags & M25P80_QUAD_READ) {
+		ret = set_quad_mode(flash, info->jedec_id);
+		if (ret) {
+			dev_err(&flash->spi->dev, "quad mode not supported\n");
+			return ret;
+		}
+		flash->flash_read = M25P80_QUAD;
+	}
+
 	if (np) {
 		/* If we were instantiated by DT, use it */
 		if (of_property_read_bool(np, "m25p,fast-read"))
@@ -1089,12 +1220,19 @@ static int m25p_probe(struct spi_device *spi)
 		flash->flash_read = M25P80_FAST;
 	}
 
-	/* Some devices cannot do fast-read, no matter what DT tells us */
-	if (info->flags & M25P_NO_FR)
+	/*
+	 * Some devices cannot do fast-read, no matter what DT tells us
+	 * Some device might support Quad read also. So, If fast read and quad
+	 * read both are not supported, default to NORMAL mode.
+	 */
+	if ((info->flags & M25P_NO_FR) || !(info->flags & M25P80_QUAD_READ))
 		flash->flash_read = M25P80_NORMAL;
 
 	/* Default commands */
 	switch (flash->flash_read) {
+	case M25P80_QUAD:
+		flash->read_opcode = OPCODE_QUAD_READ;
+		break;
 	case M25P80_FAST:
 		flash->read_opcode = OPCODE_FAST_READ;
 		break;
@@ -1116,6 +1254,9 @@ static int m25p_probe(struct spi_device *spi)
 		if (JEDEC_MFR(info->jedec_id) == CFI_MFR_AMD) {
 			/* Dedicated 4-byte command set */
 			switch (flash->flash_read) {
+			case M25P80_QUAD:
+				flash->read_opcode = OPCODE_QUAD_READ;
+				break;
 			case M25P80_FAST:
 				flash->read_opcode = OPCODE_FAST_READ_4B;
 				break;
-- 
1.7.1

Re: [PATCH 1/2] drivers: mtd: m25p80: convert "bool" read check into an enum

[Marek Vasut]

Dear Sourav Poddar,

> This is a cleanup prior to adding quad read support. This will fecilitate
> easy addition of more read commands check under an enum rather that
> defining a seperate bool for it.
> 
> Signed-off-by: Sourav Poddar <sourav.poddar@ti.com>
> Suggested-by: Brian Norris <computersforpeace@gmail.com>
> ---
>  drivers/mtd/devices/m25p80.c |   71
> +++++++++++++++++++++++++++++++++-------- 1 files changed, 57
> insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> index 7eda71d..cfafdce 100644
> --- a/drivers/mtd/devices/m25p80.c
> +++ b/drivers/mtd/devices/m25p80.c
> @@ -84,6 +84,11 @@
> 
>  /*************************************************************************
> ***/
> 
> +enum read_type {
> +	M25P80_NORMAL = 0,
> +	M25P80_FAST,
> +};
> +
>  struct m25p {
>  	struct spi_device	*spi;
>  	struct mutex		lock;
> @@ -94,7 +99,7 @@ struct m25p {
>  	u8			read_opcode;
>  	u8			program_opcode;
>  	u8			*command;
> -	bool			fast_read;
> +	enum read_type		flash_read;
>  };
> 
>  static inline struct m25p *mtd_to_m25p(struct mtd_info *mtd)
> @@ -350,6 +355,24 @@ static int m25p80_erase(struct mtd_info *mtd, struct
> erase_info *instr) }
> 
>  /*
> + * Dummy Cycle calculation for different type of read.
> + * It can be used to support more commands with
> + * different dummy cycle requirement.
> + */
> +static inline int m25p80_dummy_cycles_read(struct m25p *flash)
> +{
> +	switch (flash->flash_read) {
> +	case M25P80_FAST:
> +		return 1;
> +	case M25P80_NORMAL:
> +		return 0;
> +	default:
> +		dev_err(&flash->spi->dev, "No valid read type supported");

Does dev_err() insert newlines automatically?

Re: [PATCHv3 2/2] drivers: mtd: m25p80: Add quad read support.

[Marek Vasut]

Dear Sourav Poddar,
[...]

Reviewed-by: Marek Vasut <marex@denx.de>

Best regards,
Marek Vasut

Re: [PATCH 1/2] drivers: mtd: m25p80: convert "bool" read check into an enum

[Brian Norris]

On Wed, Nov 06, 2013 at 09:24:37PM +0100, Marek Vasut wrote:
> Dear Sourav Poddar,
> 
> > This is a cleanup prior to adding quad read support. This will fecilitate
> > easy addition of more read commands check under an enum rather that
> > defining a seperate bool for it.
> > 
> > Signed-off-by: Sourav Poddar <sourav.poddar@ti.com>
> > Suggested-by: Brian Norris <computersforpeace@gmail.com>
> > ---
> >  drivers/mtd/devices/m25p80.c |   71
> > +++++++++++++++++++++++++++++++++-------- 1 files changed, 57
> > insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> > index 7eda71d..cfafdce 100644
> > --- a/drivers/mtd/devices/m25p80.c
> > +++ b/drivers/mtd/devices/m25p80.c
> > @@ -84,6 +84,11 @@
> > 
> >  /*************************************************************************
> > ***/
> > 
> > +enum read_type {
> > +	M25P80_NORMAL = 0,
> > +	M25P80_FAST,
> > +};
> > +
> >  struct m25p {
> >  	struct spi_device	*spi;
> >  	struct mutex		lock;
> > @@ -94,7 +99,7 @@ struct m25p {
> >  	u8			read_opcode;
> >  	u8			program_opcode;
> >  	u8			*command;
> > -	bool			fast_read;
> > +	enum read_type		flash_read;
> >  };
> > 
> >  static inline struct m25p *mtd_to_m25p(struct mtd_info *mtd)
> > @@ -350,6 +355,24 @@ static int m25p80_erase(struct mtd_info *mtd, struct
> > erase_info *instr) }
> > 
> >  /*
> > + * Dummy Cycle calculation for different type of read.
> > + * It can be used to support more commands with
> > + * different dummy cycle requirement.
> > + */
> > +static inline int m25p80_dummy_cycles_read(struct m25p *flash)
> > +{
> > +	switch (flash->flash_read) {
> > +	case M25P80_FAST:
> > +		return 1;
> > +	case M25P80_NORMAL:
> > +		return 0;
> > +	default:
> > +		dev_err(&flash->spi->dev, "No valid read type supported");
> 
> Does dev_err() insert newlines automatically?

Not sure if this is a rhetorical question, but I'll answer: no,
dev_err() does not insert newlines automatically. The string parameter
should contain the '\n'.

This pair of patches looks good otherwise. I'll see if others have
reviews to make, and I can just fixup the newlines if that's the only
change to make.

Thanks,
Brian

Re: [PATCH 1/2] drivers: mtd: m25p80: convert "bool" read check into an enum

[Sourav Poddar]

On Thursday 07 November 2013 01:36 PM, Brian Norris wrote:
> On Wed, Nov 06, 2013 at 09:24:37PM +0100, Marek Vasut wrote:
>> Dear Sourav Poddar,
>>
>>> This is a cleanup prior to adding quad read support. This will fecilitate
>>> easy addition of more read commands check under an enum rather that
>>> defining a seperate bool for it.
>>>
>>> Signed-off-by: Sourav Poddar<sourav.poddar@ti.com>
>>> Suggested-by: Brian Norris<computersforpeace@gmail.com>
>>> ---
>>>   drivers/mtd/devices/m25p80.c |   71
>>> +++++++++++++++++++++++++++++++++-------- 1 files changed, 57
>>> insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
>>> index 7eda71d..cfafdce 100644
>>> --- a/drivers/mtd/devices/m25p80.c
>>> +++ b/drivers/mtd/devices/m25p80.c
>>> @@ -84,6 +84,11 @@
>>>
>>>   /*************************************************************************
>>> ***/
>>>
>>> +enum read_type {
>>> +	M25P80_NORMAL = 0,
>>> +	M25P80_FAST,
>>> +};
>>> +
>>>   struct m25p {
>>>   	struct spi_device	*spi;
>>>   	struct mutex		lock;
>>> @@ -94,7 +99,7 @@ struct m25p {
>>>   	u8			read_opcode;
>>>   	u8			program_opcode;
>>>   	u8			*command;
>>> -	bool			fast_read;
>>> +	enum read_type		flash_read;
>>>   };
>>>
>>>   static inline struct m25p *mtd_to_m25p(struct mtd_info *mtd)
>>> @@ -350,6 +355,24 @@ static int m25p80_erase(struct mtd_info *mtd, struct
>>> erase_info *instr) }
>>>
>>>   /*
>>> + * Dummy Cycle calculation for different type of read.
>>> + * It can be used to support more commands with
>>> + * different dummy cycle requirement.
>>> + */
>>> +static inline int m25p80_dummy_cycles_read(struct m25p *flash)
>>> +{
>>> +	switch (flash->flash_read) {
>>> +	case M25P80_FAST:
>>> +		return 1;
>>> +	case M25P80_NORMAL:
>>> +		return 0;
>>> +	default:
>>> +		dev_err(&flash->spi->dev, "No valid read type supported");
>> Does dev_err() insert newlines automatically?
> Not sure if this is a rhetorical question, but I'll answer: no,
> dev_err() does not insert newlines automatically. The string parameter
> should contain the '\n'.
>
> This pair of patches looks good otherwise. I'll see if others have
> reviews to make, and I can just fixup the newlines if that's the only
> change to make.
>
Thanks a lot Brian.
> Thanks,
> Brian

Re: [PATCHv3 2/2] drivers: mtd: m25p80: Add quad read support.

[Sourav Poddar]

On Thursday 07 November 2013 01:56 AM, Marek Vasut wrote:
> Dear Sourav Poddar,
> [...]
>
> Reviewed-by: Marek Vasut<marex@denx.de>
>
Thanks!
> Best regards,
> Marek Vasut

Re: [PATCH 1/2] drivers: mtd: m25p80: convert "bool" read check into an enum

[Marek Vasut]

Hi Brian,

> On Wed, Nov 06, 2013 at 09:24:37PM +0100, Marek Vasut wrote:
> > Dear Sourav Poddar,
> > 
> > > This is a cleanup prior to adding quad read support. This will
> > > fecilitate easy addition of more read commands check under an enum
> > > rather that defining a seperate bool for it.
> > > 
> > > Signed-off-by: Sourav Poddar <sourav.poddar@ti.com>
> > > Suggested-by: Brian Norris <computersforpeace@gmail.com>
> > > ---
> > > 
> > >  drivers/mtd/devices/m25p80.c |   71
> > > 
> > > +++++++++++++++++++++++++++++++++-------- 1 files changed, 57
> > > insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/drivers/mtd/devices/m25p80.c
> > > b/drivers/mtd/devices/m25p80.c index 7eda71d..cfafdce 100644
> > > --- a/drivers/mtd/devices/m25p80.c
> > > +++ b/drivers/mtd/devices/m25p80.c
> > > @@ -84,6 +84,11 @@
> > > 
> > >  /*********************************************************************
> > >  ****
> > > 
> > > ***/
> > > 
> > > +enum read_type {
> > > +	M25P80_NORMAL = 0,
> > > +	M25P80_FAST,
> > > +};
> > > +
> > > 
> > >  struct m25p {
> > >  
> > >  	struct spi_device	*spi;
> > >  	struct mutex		lock;
> > > 
> > > @@ -94,7 +99,7 @@ struct m25p {
> > > 
> > >  	u8			read_opcode;
> > >  	u8			program_opcode;
> > >  	u8			*command;
> > > 
> > > -	bool			fast_read;
> > > +	enum read_type		flash_read;
> > > 
> > >  };
> > >  
> > >  static inline struct m25p *mtd_to_m25p(struct mtd_info *mtd)
> > > 
> > > @@ -350,6 +355,24 @@ static int m25p80_erase(struct mtd_info *mtd,
> > > struct erase_info *instr) }
> > > 
> > >  /*
> > > 
> > > + * Dummy Cycle calculation for different type of read.
> > > + * It can be used to support more commands with
> > > + * different dummy cycle requirement.
> > > + */
> > > +static inline int m25p80_dummy_cycles_read(struct m25p *flash)
> > > +{
> > > +	switch (flash->flash_read) {
> > > +	case M25P80_FAST:
> > > +		return 1;
> > > +	case M25P80_NORMAL:
> > > +		return 0;
> > > +	default:
> > > +		dev_err(&flash->spi->dev, "No valid read type supported");
> > 
> > Does dev_err() insert newlines automatically?
> 
> Not sure if this is a rhetorical question, but I'll answer: no,
> dev_err() does not insert newlines automatically. The string parameter
> should contain the '\n'.
> 
> This pair of patches looks good otherwise. I'll see if others have
> reviews to make, and I can just fixup the newlines if that's the only
> change to make.

Yep, I think the patches are good otherwise. Thanks!
 
> Thanks,
> Brian

Re: [PATCHv3 0/2] m25p80: QUAD read support + cleanup.

[Brian Norris]

+ Huang

On Wed, Nov 06, 2013 at 08:05:33PM +0530, Sourav Poddar wrote:
> Patch series does the following:
> 1. Cleanup the m25p80 driver to convert bool check for
>    read into an enum. This will help adding more read 
>    commands into the driver easily.
> 
> 2. Add quad read support for spansion and macronix flash devices.

Do we have any testing results? I know that some QSPI controllers still
need some more work to be able to support this, but has someone tested
this current patch set with a "true" SPI controller on mainline?

Brian

Re: [PATCHv3 0/2] m25p80: QUAD read support + cleanup.

[Sourav Poddar]

Hi Brian,
On Thursday 07 November 2013 11:59 PM, Brian Norris wrote:
> + Huang
>
> On Wed, Nov 06, 2013 at 08:05:33PM +0530, Sourav Poddar wrote:
>> Patch series does the following:
>> 1. Cleanup the m25p80 driver to convert bool check for
>>     read into an enum. This will help adding more read
>>     commands into the driver easily.
>>
>> 2. Add quad read support for spansion and macronix flash devices.
> Do we have any testing results? I know that some QSPI controllers still
> need some more work to be able to support this, but has someone tested
> this current patch set with a "true" SPI controller on mainline?
>
I have tested this with a 3.12-rc6  based internal kernel(as dt patches 
are not in).
  You can check the controller at drivers/spi/spi-ti-qspi.c. From driver 
perspective, there
is an additional patch[1] required, it was pulled in by Mark, and I can 
see it in his tree.
Testing details:
flash_erase the entire chip
mtd write the flash with a particular pattern
mtd read the flash
diff the write and the read value.

[1]:
Add dual/quad read mode bit flag for the master controller.
These check will be used in the spi framework to determine
whether the master controller can do dual/quad read respectively.

Signed-off-by: Sourav Poddar <sourav.poddar@ti.com>
---
v1->v2
Added dual mode bit also.
  drivers/spi/spi-ti-qspi.c |    2 +-
  1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/spi/spi-ti-qspi.c b/drivers/spi/spi-ti-qspi.c
index e12d962..7a45c3e 100644
--- a/drivers/spi/spi-ti-qspi.c
+++ b/drivers/spi/spi-ti-qspi.c
@@ -472,7 +472,7 @@ static int ti_qspi_probe(struct platform_device *pdev)
      if (!master)
          return -ENOMEM;

-    master->mode_bits = SPI_CPOL | SPI_CPHA;
+    master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_RX_DUAL | SPI_RX_QUAD;

      master->bus_num = -1;
      master->flags = SPI_MASTER_HALF_DUPLEX;

> Bri
> an

Re: [PATCHv3 0/2] m25p80: QUAD read support + cleanup.

[Sourav Poddar]

On Friday 08 November 2013 09:54 AM, Sourav Poddar wrote:
> Hi Brian,
> On Thursday 07 November 2013 11:59 PM, Brian Norris wrote:
>> + Huang
>>
>> On Wed, Nov 06, 2013 at 08:05:33PM +0530, Sourav Poddar wrote:
>>> Patch series does the following:
>>> 1. Cleanup the m25p80 driver to convert bool check for
>>>     read into an enum. This will help adding more read
>>>     commands into the driver easily.
>>>
>>> 2. Add quad read support for spansion and macronix flash devices.
>> Do we have any testing results? I know that some QSPI controllers still
>> need some more work to be able to support this, but has someone tested
>> this current patch set with a "true" SPI controller on mainline?
>>
> I have tested this with a 3.12-rc6  based internal kernel(as dt 
> patches are not in).
>  You can check the controller at drivers/spi/spi-ti-qspi.c. From 
> driver perspective, there
> is an additional patch[1] required, it was pulled in by Mark, and I 
> can see it in his tree.
> Testing details:
> flash_erase the entire chip
> mtd write the flash with a particular pattern
> mtd read the flash
> diff the write and the read value.
>
Just to add more info:
I have tested this on
1. DRA7xx having Spansion S25FL256S flash
2. AM43xx having Macronix MX66L51235F
> [1]:
> Add dual/quad read mode bit flag for the master controller.
> These check will be used in the spi framework to determine
> whether the master controller can do dual/quad read respectively.
>
> Signed-off-by: Sourav Poddar <sourav.poddar@ti.com>
> ---
> v1->v2
> Added dual mode bit also.
>  drivers/spi/spi-ti-qspi.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/spi/spi-ti-qspi.c b/drivers/spi/spi-ti-qspi.c
> index e12d962..7a45c3e 100644
> --- a/drivers/spi/spi-ti-qspi.c
> +++ b/drivers/spi/spi-ti-qspi.c
> @@ -472,7 +472,7 @@ static int ti_qspi_probe(struct platform_device 
> *pdev)
>      if (!master)
>          return -ENOMEM;
>
> -    master->mode_bits = SPI_CPOL | SPI_CPHA;
> +    master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_RX_DUAL | SPI_RX_QUAD;
>
>      master->bus_num = -1;
>      master->flags = SPI_MASTER_HALF_DUPLEX;
>
>> Bri
>> an
>

Re: [PATCH 1/2] drivers: mtd: m25p80: convert "bool" read check into an enum

[Brian Norris]

On Wed, Nov 06, 2013 at 08:05:34PM +0530, Sourav Poddar wrote:
> This is a cleanup prior to adding quad read support. This will fecilitate 
> easy addition of more read commands check under an enum rather that defining a 
> seperate bool for it.
> 
> Signed-off-by: Sourav Poddar <sourav.poddar@ti.com>
> Suggested-by: Brian Norris <computersforpeace@gmail.com>
> ---
>  drivers/mtd/devices/m25p80.c |   71 +++++++++++++++++++++++++++++++++--------
>  1 files changed, 57 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> index 7eda71d..cfafdce 100644
> --- a/drivers/mtd/devices/m25p80.c
> +++ b/drivers/mtd/devices/m25p80.c
> @@ -84,6 +84,11 @@
>  
>  /****************************************************************************/
>  
> +enum read_type {
> +	M25P80_NORMAL = 0,
> +	M25P80_FAST,
> +};
> +
>  struct m25p {
>  	struct spi_device	*spi;
>  	struct mutex		lock;
> @@ -94,7 +99,7 @@ struct m25p {
>  	u8			read_opcode;
>  	u8			program_opcode;
>  	u8			*command;
> -	bool			fast_read;
> +	enum read_type		flash_read;
>  };
>  
>  static inline struct m25p *mtd_to_m25p(struct mtd_info *mtd)
> @@ -350,6 +355,24 @@ static int m25p80_erase(struct mtd_info *mtd, struct erase_info *instr)
>  }
>  
>  /*
> + * Dummy Cycle calculation for different type of read.
> + * It can be used to support more commands with
> + * different dummy cycle requirement.

I changed:

s/requirement/requirements/

> + */
> +static inline int m25p80_dummy_cycles_read(struct m25p *flash)
> +{
> +	switch (flash->flash_read) {
> +	case M25P80_FAST:
> +		return 1;
> +	case M25P80_NORMAL:
> +		return 0;
> +	default:
> +		dev_err(&flash->spi->dev, "No valid read type supported");

Added '\n' to the dev_err() strings (in multiple places).

> +		return -1;
> +	}
> +}
> +
> +/*
>   * Read an address range from the flash chip.  The address range
>   * may be any size provided it is within the physical boundaries.
>   */

...

And pushed to l2-mtd.git. Thanks!

Brian

Re: [PATCH 1/2] drivers: mtd: m25p80: convert "bool" read check into an enum

[Sourav Poddar]

On Friday 08 November 2013 11:36 PM, Brian Norris wrote:
> On Wed, Nov 06, 2013 at 08:05:34PM +0530, Sourav Poddar wrote:
>> This is a cleanup prior to adding quad read support. This will fecilitate
>> easy addition of more read commands check under an enum rather that defining a
>> seperate bool for it.
>>
>> Signed-off-by: Sourav Poddar<sourav.poddar@ti.com>
>> Suggested-by: Brian Norris<computersforpeace@gmail.com>
>> ---
>>   drivers/mtd/devices/m25p80.c |   71 +++++++++++++++++++++++++++++++++--------
>>   1 files changed, 57 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
>> index 7eda71d..cfafdce 100644
>> --- a/drivers/mtd/devices/m25p80.c
>> +++ b/drivers/mtd/devices/m25p80.c
>> @@ -84,6 +84,11 @@
>>
>>   /****************************************************************************/
>>
>> +enum read_type {
>> +	M25P80_NORMAL = 0,
>> +	M25P80_FAST,
>> +};
>> +
>>   struct m25p {
>>   	struct spi_device	*spi;
>>   	struct mutex		lock;
>> @@ -94,7 +99,7 @@ struct m25p {
>>   	u8			read_opcode;
>>   	u8			program_opcode;
>>   	u8			*command;
>> -	bool			fast_read;
>> +	enum read_type		flash_read;
>>   };
>>
>>   static inline struct m25p *mtd_to_m25p(struct mtd_info *mtd)
>> @@ -350,6 +355,24 @@ static int m25p80_erase(struct mtd_info *mtd, struct erase_info *instr)
>>   }
>>
>>   /*
>> + * Dummy Cycle calculation for different type of read.
>> + * It can be used to support more commands with
>> + * different dummy cycle requirement.
> I changed:
>
> s/requirement/requirements/
>
>> + */
>> +static inline int m25p80_dummy_cycles_read(struct m25p *flash)
>> +{
>> +	switch (flash->flash_read) {
>> +	case M25P80_FAST:
>> +		return 1;
>> +	case M25P80_NORMAL:
>> +		return 0;
>> +	default:
>> +		dev_err(&flash->spi->dev, "No valid read type supported");
> Added '\n' to the dev_err() strings (in multiple places).
>
>> +		return -1;
>> +	}
>> +}
>> +
>> +/*
>>    * Read an address range from the flash chip.  The address range
>>    * may be any size provided it is within the physical boundaries.
>>    */
> ...
>
> And pushed to l2-mtd.git. Thanks!
>
> Brian
Thanks Brian!
Just curious, is 2nd patch of this series not pushed for purpose? Its
reviewed by Marek and you said in one of your reply that its look good
to you too.

Re: [PATCHv3 2/2] drivers: mtd: m25p80: Add quad read support.

[Brian Norris]

On Wed, Nov 06, 2013 at 08:05:35PM +0530, Sourav Poddar wrote:
> Some flash also support quad read mode.
> Adding support for adding quad mode in m25p80
> for spansion and macronix flash.
> 
> Signed-off-by: Sourav Poddar <sourav.poddar@ti.com>
> ---
> v2->v3:
> Re arrange the code based on the cleanup done as
> first patch of the series.
>  drivers/mtd/devices/m25p80.c |  149 ++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 145 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> index cfafdce..7460fb3 100644
> --- a/drivers/mtd/devices/m25p80.c
> +++ b/drivers/mtd/devices/m25p80.c
> +static inline int set_quad_mode(struct m25p *flash, u32 jedec_id)

I would drop the inline here. This isn't a critical path function, nor
is it particularly small.

> +{
> +	int status;
> +
> +	switch (JEDEC_MFR(jedec_id)) {
> +	case CFI_MFR_MACRONIX:
> +		status = macronix_quad_enable(flash);
> +		if (status) {
> +			dev_err(&flash->spi->dev,
> +				"Macronix quad-read not enabled");
> +			return -EINVAL;
> +		}
> +		return status;
> +	default:
> +		status = spansion_quad_enable(flash);
> +		if (status) {
> +			dev_err(&flash->spi->dev,
> +				"Spansion quad-read not enabled");
> +			return -EINVAL;
> +		}
> +		return status;
> +	}
> +}
> +
> +/*
>   * Erase the whole flash memory
>   *
>   * Returns 0 if successful, non-zero otherwise.

...

> @@ -397,6 +516,7 @@ static int m25p80_read(struct mtd_info *mtd, loff_t from, size_t len,
>  		return -EINVAL;
>  	}
>  
> +

Extraneous new line?

>  	t[0].tx_buf = flash->command;
>  	t[0].len = m25p_cmdsz(flash) + dummy;
>  	spi_message_add_tail(&t[0], &m);
> @@ -727,6 +847,7 @@ struct flash_info {
>  #define	SST_WRITE	0x04		/* use SST byte programming */
>  #define	M25P_NO_FR	0x08		/* Can't do fastread */
>  #define	SECT_4K_PMC	0x10		/* OPCODE_BE_4K_PMC works uniformly */
> +#define	M25P80_QUAD_READ	0x20    /* Flash supports Quad Read */
>  };
>  
>  #define INFO(_jedec_id, _ext_id, _sector_size, _n_sectors, _flags)	\

...

> @@ -1089,12 +1220,19 @@ static int m25p_probe(struct spi_device *spi)
>  		flash->flash_read = M25P80_FAST;
>  	}
>  
> -	/* Some devices cannot do fast-read, no matter what DT tells us */
> -	if (info->flags & M25P_NO_FR)
> +	/*
> +	 * Some devices cannot do fast-read, no matter what DT tells us
> +	 * Some device might support Quad read also. So, If fast read and quad
> +	 * read both are not supported, default to NORMAL mode.
> +	 */
> +	if ((info->flags & M25P_NO_FR) || !(info->flags & M25P80_QUAD_READ))
>  		flash->flash_read = M25P80_NORMAL;
>  

Did I miss something earlier, or did you just sneak this hunk into v3?
This is incorrect. It effectively drops all support for FAST_READ in
devices that don't support QUAD_READ, due the second clause of your ||.

Similarly, I just realized you placed the QUAD_READ check before the
fast/normal checks, which means that fast/normal-read will often
override quad-read. This also is not what we want.

Since I already started fixing up your patch, I'll just send out the v4
and let you guys take a look at it.

Brian