[PATCH 1/5] mtd: m25p80: fix allocation size

[PATCH 1/5] mtd: m25p80: fix allocation size

[Brian Norris]

This patch fixes two memory errors:

1. During a probe failure (in mtd_device_parse_register?) the command
   buffer would not be freed.

2. The command buffer's size is determined based on the 'fast_read'
   boolean, but the assignment of fast_read is made after this
   allocation. Thus, the buffer may be allocated "too small".

To fix the first, just switch to the devres version of kzalloc.

To fix the second, increase MAX_CMD_SIZE unconditionally. It's not worth
saving a byte to fiddle around with the conditions here.

This problem was reported by Yuhang Wang a while back.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
Reported-by: Yuhang Wang <wangyuhang2014@gmail.com>
Cc: <stable@vger.kernel.org>
---
 drivers/mtd/devices/m25p80.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index 8d6c87be..63a95ac 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -78,7 +78,7 @@
 
 /* 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		5
+#define	MAX_CMD_SIZE		6
 
 #define JEDEC_MFR(_jedec_id)	((_jedec_id) >> 16)
 
@@ -996,15 +996,13 @@ static int m25p_probe(struct spi_device *spi)
 		}
 	}
 
-	flash = kzalloc(sizeof *flash, GFP_KERNEL);
+	flash = devm_kzalloc(&spi->dev, sizeof(*flash), GFP_KERNEL);
 	if (!flash)
 		return -ENOMEM;
-	flash->command = kmalloc(MAX_CMD_SIZE + (flash->fast_read ? 1 : 0),
-					GFP_KERNEL);
-	if (!flash->command) {
-		kfree(flash);
+
+	flash->command = devm_kzalloc(&spi->dev, MAX_CMD_SIZE, GFP_KERNEL);
+	if (!flash->command)
 		return -ENOMEM;
-	}
 
 	flash->spi = spi;
 	mutex_init(&flash->lock);
@@ -1137,14 +1135,10 @@ static int m25p_probe(struct spi_device *spi)
 static int m25p_remove(struct spi_device *spi)
 {
 	struct m25p	*flash = spi_get_drvdata(spi);
-	int		status;
 
 	/* Clean up MTD stuff. */
-	status = mtd_device_unregister(&flash->mtd);
-	if (status == 0) {
-		kfree(flash->command);
-		kfree(flash);
-	}
+	mtd_device_unregister(&flash->mtd);
+
 	return 0;
 }
 
-- 
1.8.4

[PATCH 2/5] mtd: m25p80: remove obsolete FIXME

[Brian Norris]

The FIXME and NOTE have already been fixed (we have FAST_READ support).

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/mtd/devices/m25p80.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index 63a95ac..7c4cbad 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -367,10 +367,6 @@ static int m25p80_read(struct mtd_info *mtd, loff_t from, size_t len,
 	spi_message_init(&m);
 	memset(t, 0, (sizeof t));
 
-	/* NOTE:
-	 * OPCODE_FAST_READ (if available) is faster.
-	 * Should add 1 byte DUMMY_BYTE.
-	 */
 	t[0].tx_buf = flash->command;
 	t[0].len = m25p_cmdsz(flash) + (flash->fast_read ? 1 : 0);
 	spi_message_add_tail(&t[0], &m);
@@ -388,11 +384,6 @@ static int m25p80_read(struct mtd_info *mtd, loff_t from, size_t len,
 		return 1;
 	}
 
-	/* FIXME switch to OPCODE_FAST_READ.  It's required for higher
-	 * clocks; and at this writing, every chip this driver handles
-	 * supports that opcode.
-	 */
-
 	/* Set up the write data buffer. */
 	opcode = flash->read_opcode;
 	flash->command[0] = opcode;
-- 
1.8.4

Re: [PATCH 2/5] mtd: m25p80: remove obsolete FIXME

[Sourav Poddar]

On Thursday 24 October 2013 08:28 AM, Brian Norris wrote:
> The FIXME and NOTE have already been fixed (we have FAST_READ support).
>
> Signed-off-by: Brian Norris<computersforpeace@gmail.com>
Reviewed-by: Sourav Poddar <sourav.poddar@ti.com>
> ---
>   drivers/mtd/devices/m25p80.c | 9 ---------
>   1 file changed, 9 deletions(-)
>
> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> index 63a95ac..7c4cbad 100644
> --- a/drivers/mtd/devices/m25p80.c
> +++ b/drivers/mtd/devices/m25p80.c
> @@ -367,10 +367,6 @@ static int m25p80_read(struct mtd_info *mtd, loff_t from, size_t len,
>   	spi_message_init(&m);
>   	memset(t, 0, (sizeof t));
>
> -	/* NOTE:
> -	 * OPCODE_FAST_READ (if available) is faster.
> -	 * Should add 1 byte DUMMY_BYTE.
> -	 */
>   	t[0].tx_buf = flash->command;
>   	t[0].len = m25p_cmdsz(flash) + (flash->fast_read ? 1 : 0);
>   	spi_message_add_tail(&t[0],&m);
> @@ -388,11 +384,6 @@ static int m25p80_read(struct mtd_info *mtd, loff_t from, size_t len,
>   		return 1;
>   	}
>
> -	/* FIXME switch to OPCODE_FAST_READ.  It's required for higher
> -	 * clocks; and at this writing, every chip this driver handles
> -	 * supports that opcode.
> -	 */
> -
>   	/* Set up the write data buffer. */
>   	opcode = flash->read_opcode;
>   	flash->command[0] = opcode;

Re: [PATCH 2/5] mtd: m25p80: remove obsolete FIXME

[Marek Vasut]

Hi Brian,

> The FIXME and NOTE have already been fixed (we have FAST_READ support).
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> ---
>  drivers/mtd/devices/m25p80.c | 9 ---------
>  1 file changed, 9 deletions(-)
> 
> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> index 63a95ac..7c4cbad 100644
> --- a/drivers/mtd/devices/m25p80.c
> +++ b/drivers/mtd/devices/m25p80.c
> @@ -367,10 +367,6 @@ static int m25p80_read(struct mtd_info *mtd, loff_t
> from, size_t len, spi_message_init(&m);
>  	memset(t, 0, (sizeof t));
> 
> -	/* NOTE:
> -	 * OPCODE_FAST_READ (if available) is faster.
> -	 * Should add 1 byte DUMMY_BYTE.
> -	 */
>  	t[0].tx_buf = flash->command;
>  	t[0].len = m25p_cmdsz(flash) + (flash->fast_read ? 1 : 0);
>  	spi_message_add_tail(&t[0], &m);
> @@ -388,11 +384,6 @@ static int m25p80_read(struct mtd_info *mtd, loff_t
> from, size_t len, return 1;
>  	}
> 
> -	/* FIXME switch to OPCODE_FAST_READ.  It's required for higher
> -	 * clocks; and at this writing, every chip this driver handles
> -	 * supports that opcode.
> -	 */
> -
>  	/* Set up the write data buffer. */
>  	opcode = flash->read_opcode;
>  	flash->command[0] = opcode;

Makes great share of sense ;-)

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

Best regards,
Marek Vasut

[PATCH 3/5] mtd: m25p80: re-align ID entries

[Brian Norris]

No change in the table data.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/mtd/devices/m25p80.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index 7c4cbad..7e3ec7a 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -740,19 +740,19 @@ static const struct spi_device_id m25p_ids[] = {
 	{ "at45db081d", INFO(0x1f2500, 0, 64 * 1024, 16, SECT_4K) },
 
 	/* EON -- en25xxx */
-	{ "en25f32", INFO(0x1c3116, 0, 64 * 1024,  64, SECT_4K) },
-	{ "en25p32", INFO(0x1c2016, 0, 64 * 1024,  64, 0) },
-	{ "en25q32b", INFO(0x1c3016, 0, 64 * 1024,  64, 0) },
-	{ "en25p64", INFO(0x1c2017, 0, 64 * 1024, 128, 0) },
-	{ "en25q64", INFO(0x1c3017, 0, 64 * 1024, 128, SECT_4K) },
-	{ "en25qh256", INFO(0x1c7019, 0, 64 * 1024, 512, 0) },
+	{ "en25f32",    INFO(0x1c3116, 0, 64 * 1024,   64, SECT_4K) },
+	{ "en25p32",    INFO(0x1c2016, 0, 64 * 1024,   64, 0) },
+	{ "en25q32b",   INFO(0x1c3016, 0, 64 * 1024,   64, 0) },
+	{ "en25p64",    INFO(0x1c2017, 0, 64 * 1024,  128, 0) },
+	{ "en25q64",    INFO(0x1c3017, 0, 64 * 1024,  128, SECT_4K) },
+	{ "en25qh256",  INFO(0x1c7019, 0, 64 * 1024,  512, 0) },
 
 	/* ESMT */
 	{ "f25l32pa", INFO(0x8c2016, 0, 64 * 1024, 64, SECT_4K) },
 
 	/* Everspin */
-	{ "mr25h256", CAT25_INFO(  32 * 1024, 1, 256, 2, M25P_NO_ERASE | M25P_NO_FR) },
-	{ "mr25h10", CAT25_INFO(128 * 1024, 1, 256, 3, M25P_NO_ERASE | M25P_NO_FR) },
+	{ "mr25h256", CAT25_INFO( 32 * 1024, 1, 256, 2, M25P_NO_ERASE | M25P_NO_FR) },
+	{ "mr25h10",  CAT25_INFO(128 * 1024, 1, 256, 3, M25P_NO_ERASE | M25P_NO_FR) },
 
 	/* GigaDevice */
 	{ "gd25q32", INFO(0xc84016, 0, 64 * 1024,  64, SECT_4K) },
@@ -777,16 +777,16 @@ static const struct spi_device_id m25p_ids[] = {
 	{ "mx66l51235l", INFO(0xc2201a, 0, 64 * 1024, 1024, 0) },
 
 	/* Micron */
-	{ "n25q064",  INFO(0x20ba17, 0, 64 * 1024, 128, 0) },
-	{ "n25q128a11",  INFO(0x20bb18, 0, 64 * 1024, 256, 0) },
-	{ "n25q128a13",  INFO(0x20ba18, 0, 64 * 1024, 256, 0) },
-	{ "n25q256a", INFO(0x20ba19, 0, 64 * 1024, 512, SECT_4K) },
-	{ "n25q512a", INFO(0x20bb20, 0, 64 * 1024, 1024, SECT_4K) },
+	{ "n25q064",     INFO(0x20ba17, 0, 64 * 1024,  128, 0) },
+	{ "n25q128a11",  INFO(0x20bb18, 0, 64 * 1024,  256, 0) },
+	{ "n25q128a13",  INFO(0x20ba18, 0, 64 * 1024,  256, 0) },
+	{ "n25q256a",    INFO(0x20ba19, 0, 64 * 1024,  512, SECT_4K) },
+	{ "n25q512a",    INFO(0x20bb20, 0, 64 * 1024, 1024, SECT_4K) },
 
 	/* PMC */
-	{ "pm25lv512", INFO(0, 0, 32 * 1024, 2, SECT_4K_PMC) },
-	{ "pm25lv010", INFO(0, 0, 32 * 1024, 4, SECT_4K_PMC) },
-	{ "pm25lq032", INFO(0x7f9d46, 0, 64 * 1024,  64, SECT_4K) },
+	{ "pm25lv512",   INFO(0,        0, 32 * 1024,    2, SECT_4K_PMC) },
+	{ "pm25lv010",   INFO(0,        0, 32 * 1024,    4, SECT_4K_PMC) },
+	{ "pm25lq032",   INFO(0x7f9d46, 0, 64 * 1024,   64, SECT_4K) },
 
 	/* Spansion -- single (large) sector size only, at least
 	 * for the chips listed here (without boot sectors).
-- 
1.8.4

Re: [PATCH 3/5] mtd: m25p80: re-align ID entries

[Sourav Poddar]

On Thursday 24 October 2013 08:28 AM, Brian Norris wrote:
> No change in the table data.
>
> Signed-off-by: Brian Norris<computersforpeace@gmail.com>
Reviewed-by: Sourav Poddar <sourav.poddar@ti.com>
> ---
>   drivers/mtd/devices/m25p80.c | 32 ++++++++++++++++----------------
>   1 file changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> index 7c4cbad..7e3ec7a 100644
> --- a/drivers/mtd/devices/m25p80.c
> +++ b/drivers/mtd/devices/m25p80.c
> @@ -740,19 +740,19 @@ static const struct spi_device_id m25p_ids[] = {
>   	{ "at45db081d", INFO(0x1f2500, 0, 64 * 1024, 16, SECT_4K) },
>
>   	/* EON -- en25xxx */
> -	{ "en25f32", INFO(0x1c3116, 0, 64 * 1024,  64, SECT_4K) },
> -	{ "en25p32", INFO(0x1c2016, 0, 64 * 1024,  64, 0) },
> -	{ "en25q32b", INFO(0x1c3016, 0, 64 * 1024,  64, 0) },
> -	{ "en25p64", INFO(0x1c2017, 0, 64 * 1024, 128, 0) },
> -	{ "en25q64", INFO(0x1c3017, 0, 64 * 1024, 128, SECT_4K) },
> -	{ "en25qh256", INFO(0x1c7019, 0, 64 * 1024, 512, 0) },
> +	{ "en25f32",    INFO(0x1c3116, 0, 64 * 1024,   64, SECT_4K) },
> +	{ "en25p32",    INFO(0x1c2016, 0, 64 * 1024,   64, 0) },
> +	{ "en25q32b",   INFO(0x1c3016, 0, 64 * 1024,   64, 0) },
> +	{ "en25p64",    INFO(0x1c2017, 0, 64 * 1024,  128, 0) },
> +	{ "en25q64",    INFO(0x1c3017, 0, 64 * 1024,  128, SECT_4K) },
> +	{ "en25qh256",  INFO(0x1c7019, 0, 64 * 1024,  512, 0) },
>
>   	/* ESMT */
>   	{ "f25l32pa", INFO(0x8c2016, 0, 64 * 1024, 64, SECT_4K) },
>
>   	/* Everspin */
> -	{ "mr25h256", CAT25_INFO(  32 * 1024, 1, 256, 2, M25P_NO_ERASE | M25P_NO_FR) },
> -	{ "mr25h10", CAT25_INFO(128 * 1024, 1, 256, 3, M25P_NO_ERASE | M25P_NO_FR) },
> +	{ "mr25h256", CAT25_INFO( 32 * 1024, 1, 256, 2, M25P_NO_ERASE | M25P_NO_FR) },
> +	{ "mr25h10",  CAT25_INFO(128 * 1024, 1, 256, 3, M25P_NO_ERASE | M25P_NO_FR) },
>
>   	/* GigaDevice */
>   	{ "gd25q32", INFO(0xc84016, 0, 64 * 1024,  64, SECT_4K) },
> @@ -777,16 +777,16 @@ static const struct spi_device_id m25p_ids[] = {
>   	{ "mx66l51235l", INFO(0xc2201a, 0, 64 * 1024, 1024, 0) },
>
>   	/* Micron */
> -	{ "n25q064",  INFO(0x20ba17, 0, 64 * 1024, 128, 0) },
> -	{ "n25q128a11",  INFO(0x20bb18, 0, 64 * 1024, 256, 0) },
> -	{ "n25q128a13",  INFO(0x20ba18, 0, 64 * 1024, 256, 0) },
> -	{ "n25q256a", INFO(0x20ba19, 0, 64 * 1024, 512, SECT_4K) },
> -	{ "n25q512a", INFO(0x20bb20, 0, 64 * 1024, 1024, SECT_4K) },
> +	{ "n25q064",     INFO(0x20ba17, 0, 64 * 1024,  128, 0) },
> +	{ "n25q128a11",  INFO(0x20bb18, 0, 64 * 1024,  256, 0) },
> +	{ "n25q128a13",  INFO(0x20ba18, 0, 64 * 1024,  256, 0) },
> +	{ "n25q256a",    INFO(0x20ba19, 0, 64 * 1024,  512, SECT_4K) },
> +	{ "n25q512a",    INFO(0x20bb20, 0, 64 * 1024, 1024, SECT_4K) },
>
>   	/* PMC */
> -	{ "pm25lv512", INFO(0, 0, 32 * 1024, 2, SECT_4K_PMC) },
> -	{ "pm25lv010", INFO(0, 0, 32 * 1024, 4, SECT_4K_PMC) },
> -	{ "pm25lq032", INFO(0x7f9d46, 0, 64 * 1024,  64, SECT_4K) },
> +	{ "pm25lv512",   INFO(0,        0, 32 * 1024,    2, SECT_4K_PMC) },
> +	{ "pm25lv010",   INFO(0,        0, 32 * 1024,    4, SECT_4K_PMC) },
> +	{ "pm25lq032",   INFO(0x7f9d46, 0, 64 * 1024,   64, SECT_4K) },
>
>   	/* Spansion -- single (large) sector size only, at least
>   	 * for the chips listed here (without boot sectors).

[PATCH 4/5] mtd: m25p80: remove M25PXX_USE_FAST_READ Kconfig

[Brian Norris]

Remove the compile-time option for FAST_READ, since we have run-time
support for detecting it.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/mtd/devices/Kconfig  |  7 -------
 drivers/mtd/devices/m25p80.c | 11 ++++++-----
 2 files changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/mtd/devices/Kconfig b/drivers/mtd/devices/Kconfig
index 74ab4b7..0128138 100644
--- a/drivers/mtd/devices/Kconfig
+++ b/drivers/mtd/devices/Kconfig
@@ -95,13 +95,6 @@ config MTD_M25P80
 	  if you want to specify device partitioning or to use a device which
 	  doesn't support the JEDEC ID instruction.
 
-config M25PXX_USE_FAST_READ
-	bool "Use FAST_READ OPCode allowing SPI CLK >= 50MHz"
-	depends on MTD_M25P80
-	default y
-	help
-	  This option enables FAST_READ access supported by ST M25Pxx.
-
 config MTD_SPEAR_SMI
 	tristate "SPEAR MTD NOR Support through SMI controller"
 	depends on PLAT_SPEAR
diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index 7e3ec7a..d6c5c57 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -1055,13 +1055,14 @@ static int m25p_probe(struct spi_device *spi)
 	flash->page_size = info->page_size;
 	flash->mtd.writebufsize = flash->page_size;
 
-	flash->fast_read = false;
-	if (np && of_property_read_bool(np, "m25p,fast-read"))
+	if (np)
+		/* If we were instantiated by DT, use it */
+		flash->fast_read = of_property_read_bool(np, "m25p,fast-read");
+	else
+		/* If we weren't instantiated by DT, default to fast-read */
 		flash->fast_read = true;
 
-#ifdef CONFIG_M25PXX_USE_FAST_READ
-	flash->fast_read = true;
-#endif
+	/* Some devices cannot do fast-read, no matter what DT tells us */
 	if (info->flags & M25P_NO_FR)
 		flash->fast_read = false;
 
-- 
1.8.4

Re: [PATCH 4/5] mtd: m25p80: remove M25PXX_USE_FAST_READ Kconfig

[Sourav Poddar]

Hi Brian,
On Thursday 24 October 2013 08:28 AM, Brian Norris wrote:
> Remove the compile-time option for FAST_READ, since we have run-time
> support for detecting it.
>
> Signed-off-by: Brian Norris<computersforpeace@gmail.com>
> ---
>   drivers/mtd/devices/Kconfig  |  7 -------
>   drivers/mtd/devices/m25p80.c | 11 ++++++-----
>   2 files changed, 6 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/mtd/devices/Kconfig b/drivers/mtd/devices/Kconfig
> index 74ab4b7..0128138 100644
> --- a/drivers/mtd/devices/Kconfig
> +++ b/drivers/mtd/devices/Kconfig
> @@ -95,13 +95,6 @@ config MTD_M25P80
>   	  if you want to specify device partitioning or to use a device which
>   	  doesn't support the JEDEC ID instruction.
>
> -config M25PXX_USE_FAST_READ
> -	bool "Use FAST_READ OPCode allowing SPI CLK>= 50MHz"
> -	depends on MTD_M25P80
> -	default y
> -	help
> -	  This option enables FAST_READ access supported by ST M25Pxx.
> -
>   config MTD_SPEAR_SMI
>   	tristate "SPEAR MTD NOR Support through SMI controller"
>   	depends on PLAT_SPEAR
> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> index 7e3ec7a..d6c5c57 100644
> --- a/drivers/mtd/devices/m25p80.c
> +++ b/drivers/mtd/devices/m25p80.c
> @@ -1055,13 +1055,14 @@ static int m25p_probe(struct spi_device *spi)
>   	flash->page_size = info->page_size;
>   	flash->mtd.writebufsize = flash->page_size;
>
> -	flash->fast_read = false;
> -	if (np&&  of_property_read_bool(np, "m25p,fast-read"))
> +	if (np)
> +		/* If we were instantiated by DT, use it */
> +		flash->fast_read = of_property_read_bool(np, "m25p,fast-read");
> +	else
> +		/* If we weren't instantiated by DT, default to fast-read */
>   		flash->fast_read = true;
>
This comment is in sync with my quad read mode support patch on the mtd 
list.

Here, you are defaulting the fast read to be true. Once I add quad mode
on top of this, I will set flash->quad_read = true. So, we will have both
fast and quad read set(which will not be correct). So, it is necessary 
to default to fast read ?
> -#ifdef CONFIG_M25PXX_USE_FAST_READ
> -	flash->fast_read = true;
> -#endif
> +	/* Some devices cannot do fast-read, no matter what DT tells us */
>   	if (info->flags&  M25P_NO_FR)
>   		flash->fast_read = false;
>

Re: [PATCH 4/5] mtd: m25p80: remove M25PXX_USE_FAST_READ Kconfig

[Sourav Poddar]

On Thursday 24 October 2013 02:37 PM, Sourav Poddar wrote:
> Hi Brian,
> On Thursday 24 October 2013 08:28 AM, Brian Norris wrote:
>> Remove the compile-time option for FAST_READ, since we have run-time
>> support for detecting it.
>>
>> Signed-off-by: Brian Norris<computersforpeace@gmail.com>
>> ---
>>   drivers/mtd/devices/Kconfig  |  7 -------
>>   drivers/mtd/devices/m25p80.c | 11 ++++++-----
>>   2 files changed, 6 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/mtd/devices/Kconfig b/drivers/mtd/devices/Kconfig
>> index 74ab4b7..0128138 100644
>> --- a/drivers/mtd/devices/Kconfig
>> +++ b/drivers/mtd/devices/Kconfig
>> @@ -95,13 +95,6 @@ config MTD_M25P80
>>         if you want to specify device partitioning or to use a device 
>> which
>>         doesn't support the JEDEC ID instruction.
>>
>> -config M25PXX_USE_FAST_READ
>> -    bool "Use FAST_READ OPCode allowing SPI CLK>= 50MHz"
>> -    depends on MTD_M25P80
>> -    default y
>> -    help
>> -      This option enables FAST_READ access supported by ST M25Pxx.
>> -
>>   config MTD_SPEAR_SMI
>>       tristate "SPEAR MTD NOR Support through SMI controller"
>>       depends on PLAT_SPEAR
>> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
>> index 7e3ec7a..d6c5c57 100644
>> --- a/drivers/mtd/devices/m25p80.c
>> +++ b/drivers/mtd/devices/m25p80.c
>> @@ -1055,13 +1055,14 @@ static int m25p_probe(struct spi_device *spi)
>>       flash->page_size = info->page_size;
>>       flash->mtd.writebufsize = flash->page_size;
>>
>> -    flash->fast_read = false;
>> -    if (np&&  of_property_read_bool(np, "m25p,fast-read"))
>> +    if (np)
>> +        /* If we were instantiated by DT, use it */
>> +        flash->fast_read = of_property_read_bool(np, "m25p,fast-read");
>> +    else
>> +        /* If we weren't instantiated by DT, default to fast-read */
>>           flash->fast_read = true;
>>
> This comment is in sync with my quad read mode support patch on the 
> mtd list.
>
> Here, you are defaulting the fast read to be true. Once I add quad mode
> on top of this, I will set flash->quad_read = true. So, we will have both
> fast and quad read set(which will not be correct). So, it is necessary 
> to default to fast read ?
Though, we will hit this scenario only for a non dt case. For dt case, 
things will be fine.
>> -#ifdef CONFIG_M25PXX_USE_FAST_READ
>> -    flash->fast_read = true;
>> -#endif
>> +    /* Some devices cannot do fast-read, no matter what DT tells us */
>>       if (info->flags&  M25P_NO_FR)
>>           flash->fast_read = false;
>>
>

Re: [PATCH 4/5] mtd: m25p80: remove M25PXX_USE_FAST_READ Kconfig

[Brian Norris]

On Thu, Oct 24, 2013 at 02:42:53PM +0530, Sourav Poddar wrote:
> On Thursday 24 October 2013 02:37 PM, Sourav Poddar wrote:
> >On Thursday 24 October 2013 08:28 AM, Brian Norris wrote:
> >>--- a/drivers/mtd/devices/m25p80.c
> >>+++ b/drivers/mtd/devices/m25p80.c
> >>@@ -1055,13 +1055,14 @@ static int m25p_probe(struct spi_device *spi)
> >>      flash->page_size = info->page_size;
> >>      flash->mtd.writebufsize = flash->page_size;
> >>
> >>-    flash->fast_read = false;
> >>-    if (np&&  of_property_read_bool(np, "m25p,fast-read"))
> >>+    if (np)
> >>+        /* If we were instantiated by DT, use it */
> >>+        flash->fast_read = of_property_read_bool(np, "m25p,fast-read");
> >>+    else
> >>+        /* If we weren't instantiated by DT, default to fast-read */
> >>          flash->fast_read = true;
> >>
> >This comment is in sync with my quad read mode support patch on
> >the mtd list.
> >
> >Here, you are defaulting the fast read to be true.

It was already default true for non-DT case where
M25PXX_USE_FAST_READ=y. I believe my code is purely a refactoring of the
code, assuming M25PXX_USE_FAST_READ is always enabled (and removing the
Kconfig).

> >Once I add quad mode
> >on top of this, I will set flash->quad_read = true. So, we will have both
> >fast and quad read set(which will not be correct). So, it is
> >necessary to default to fast read ?

I think my patch is a correct refactoring by itself. Any problem your
quad read patch has with it would still be a problem without this patch.

So, without quad-read support, we should default to fast-read unless the
chip doesn't support it. I think this is reasonable.

Now, once you add quad-read, you need to have quad-read override
fast-read.

> Though, we will hit this scenario only for a non dt case. For dt
> case, things will be fine.

Yes, but we need to make sure things make sense for all cases.

> >>-#ifdef CONFIG_M25PXX_USE_FAST_READ
> >>-    flash->fast_read = true;
> >>-#endif
> >>+    /* Some devices cannot do fast-read, no matter what DT tells us */
> >>      if (info->flags&  M25P_NO_FR)
> >>          flash->fast_read = false;
> >>
> >

I just realized this: fast-read and quad-read are mutually exclusive, so
the field that is currently 'bool m25p.fast_read' could easily just become
an 'enum m25p.read_type' (with values M25P_READ_NORMAL, M25P_READ_FAST,
M25P_READ_QUAD). That may or may not help your quad read patch.

I can delay this patch until others have looked at it, if you'd like.
You can resubmit your quad read patch without this patch 4. (But I think
this patch will help clean up your patch slightly.)

Brian

Re: [PATCH 4/5] mtd: m25p80: remove M25PXX_USE_FAST_READ Kconfig

[Sourav Poddar]

On Thursday 24 October 2013 11:09 PM, Brian Norris wrote:
> On Thu, Oct 24, 2013 at 02:42:53PM +0530, Sourav Poddar wrote:
>> On Thursday 24 October 2013 02:37 PM, Sourav Poddar wrote:
>>> On Thursday 24 October 2013 08:28 AM, Brian Norris wrote:
>>>> --- a/drivers/mtd/devices/m25p80.c
>>>> +++ b/drivers/mtd/devices/m25p80.c
>>>> @@ -1055,13 +1055,14 @@ static int m25p_probe(struct spi_device *spi)
>>>>       flash->page_size = info->page_size;
>>>>       flash->mtd.writebufsize = flash->page_size;
>>>>
>>>> -    flash->fast_read = false;
>>>> -    if (np&&   of_property_read_bool(np, "m25p,fast-read"))
>>>> +    if (np)
>>>> +        /* If we were instantiated by DT, use it */
>>>> +        flash->fast_read = of_property_read_bool(np, "m25p,fast-read");
>>>> +    else
>>>> +        /* If we weren't instantiated by DT, default to fast-read */
>>>>           flash->fast_read = true;
>>>>
>>> This comment is in sync with my quad read mode support patch on
>>> the mtd list.
>>>
>>> Here, you are defaulting the fast read to be true.
> It was already default true for non-DT case where
> M25PXX_USE_FAST_READ=y. I believe my code is purely a refactoring of the
> code, assuming M25PXX_USE_FAST_READ is always enabled (and removing the
> Kconfig).
>
>>> Once I add quad mode
>>> on top of this, I will set flash->quad_read = true. So, we will have both
>>> fast and quad read set(which will not be correct). So, it is
>>> necessary to default to fast read ?
> I think my patch is a correct refactoring by itself. Any problem your
> quad read patch has with it would still be a problem without this patch.
>
True, my bad I didn't realise that for internal testing I disable
fast read option in menuconfig.
> So, without quad-read support, we should default to fast-read unless the
> chip doesn't support it. I think this is reasonable.
>
> Now, once you add quad-read, you need to have quad-read override
> fast-read.
>
>> Though, we will hit this scenario only for a non dt case. For dt
>> case, things will be fine.
> Yes, but we need to make sure things make sense for all cases.
>
Yes.
>>>> -#ifdef CONFIG_M25PXX_USE_FAST_READ
>>>> -    flash->fast_read = true;
>>>> -#endif
>>>> +    /* Some devices cannot do fast-read, no matter what DT tells us */
>>>>       if (info->flags&   M25P_NO_FR)
>>>>           flash->fast_read = false;
>>>>
> I just realized this: fast-read and quad-read are mutually exclusive, so
> the field that is currently 'bool m25p.fast_read' could easily just become
correct.
> an 'enum m25p.read_type' (with values M25P_READ_NORMAL, M25P_READ_FAST,
> M25P_READ_QUAD). That may or may not help your quad read patch.
>
> I can delay this patch until others have looked at it, if you'd like.
> You can resubmit your quad read patch without this patch 4. (But I think
> this patch will help clean up your patch slightly.)
Ok. I will include this patch and try to cleanup.
> Brian

Re: [PATCH 4/5] mtd: m25p80: remove M25PXX_USE_FAST_READ Kconfig

[Brian Norris]

On Wed, Oct 23, 2013 at 07:58:22PM -0700, Brian Norris wrote:
> Remove the compile-time option for FAST_READ, since we have run-time
> support for detecting it.
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>

Pushed to l2-mtd.git. Comments are still welcome.

FYI Huang: Sourav's new quad read patch applies on top of this patch.

Brian

Re: [PATCH 4/5] mtd: m25p80: remove M25PXX_USE_FAST_READ Kconfig

[Marek Vasut]

Hi Brian,

> Remove the compile-time option for FAST_READ, since we have run-time
> support for detecting it.
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> ---
>  drivers/mtd/devices/Kconfig  |  7 -------
>  drivers/mtd/devices/m25p80.c | 11 ++++++-----
>  2 files changed, 6 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/mtd/devices/Kconfig b/drivers/mtd/devices/Kconfig
> index 74ab4b7..0128138 100644
> --- a/drivers/mtd/devices/Kconfig
> +++ b/drivers/mtd/devices/Kconfig
> @@ -95,13 +95,6 @@ config MTD_M25P80
>  	  if you want to specify device partitioning or to use a device which
>  	  doesn't support the JEDEC ID instruction.
> 
> -config M25PXX_USE_FAST_READ
> -	bool "Use FAST_READ OPCode allowing SPI CLK >= 50MHz"
> -	depends on MTD_M25P80
> -	default y
> -	help
> -	  This option enables FAST_READ access supported by ST M25Pxx.
> -
>  config MTD_SPEAR_SMI
>  	tristate "SPEAR MTD NOR Support through SMI controller"
>  	depends on PLAT_SPEAR
> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> index 7e3ec7a..d6c5c57 100644
> --- a/drivers/mtd/devices/m25p80.c
> +++ b/drivers/mtd/devices/m25p80.c
> @@ -1055,13 +1055,14 @@ static int m25p_probe(struct spi_device *spi)
>  	flash->page_size = info->page_size;
>  	flash->mtd.writebufsize = flash->page_size;
> 
> -	flash->fast_read = false;
> -	if (np && of_property_read_bool(np, "m25p,fast-read"))
> +	if (np)
> +		/* If we were instantiated by DT, use it */
> +		flash->fast_read = of_property_read_bool(np, "m25p,fast-read");
> +	else
> +		/* If we weren't instantiated by DT, default to fast-read */
>  		flash->fast_read = true;

We should default to FALSE , unless explicitly requested by DT, am I wrong? 
Otherwise this might break the old chips.

[...]

Best regards,
Marek Vasut

Re: [PATCH 4/5] mtd: m25p80: remove M25PXX_USE_FAST_READ Kconfig

[Brian Norris]

I see I never responded to this one.

On Sun, Oct 27, 2013 at 05:32:42PM +0100, Marek Vasut wrote:
> Hi Brian,
> 
> > Remove the compile-time option for FAST_READ, since we have run-time
> > support for detecting it.
> > 
> > Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> > ---
> >  drivers/mtd/devices/Kconfig  |  7 -------
> >  drivers/mtd/devices/m25p80.c | 11 ++++++-----
> >  2 files changed, 6 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/mtd/devices/Kconfig b/drivers/mtd/devices/Kconfig
> > index 74ab4b7..0128138 100644
> > --- a/drivers/mtd/devices/Kconfig
> > +++ b/drivers/mtd/devices/Kconfig
> > @@ -95,13 +95,6 @@ config MTD_M25P80
> >  	  if you want to specify device partitioning or to use a device which
> >  	  doesn't support the JEDEC ID instruction.
> > 
> > -config M25PXX_USE_FAST_READ
> > -	bool "Use FAST_READ OPCode allowing SPI CLK >= 50MHz"
> > -	depends on MTD_M25P80
> > -	default y
> > -	help
> > -	  This option enables FAST_READ access supported by ST M25Pxx.
> > -
> >  config MTD_SPEAR_SMI
> >  	tristate "SPEAR MTD NOR Support through SMI controller"
> >  	depends on PLAT_SPEAR
> > diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> > index 7e3ec7a..d6c5c57 100644
> > --- a/drivers/mtd/devices/m25p80.c
> > +++ b/drivers/mtd/devices/m25p80.c
> > @@ -1055,13 +1055,14 @@ static int m25p_probe(struct spi_device *spi)
> >  	flash->page_size = info->page_size;
> >  	flash->mtd.writebufsize = flash->page_size;
> > 
> > -	flash->fast_read = false;
> > -	if (np && of_property_read_bool(np, "m25p,fast-read"))
> > +	if (np)
> > +		/* If we were instantiated by DT, use it */
> > +		flash->fast_read = of_property_read_bool(np, "m25p,fast-read");
> > +	else
> > +		/* If we weren't instantiated by DT, default to fast-read */
> >  		flash->fast_read = true;
> 
> We should default to FALSE , unless explicitly requested by DT, am I wrong? 
> Otherwise this might break the old chips.

I believe my patch is simply a refactoring of the existing code with the
assumption that M25PXX_USE_FAST_READ=y. (In my experience, everyone has
it set to y. Perhaps I'm wrong.)

Now, it's unclear to me what this Kconfig was used for. It's the wrong
approach for controlling fast read on a per-controller or
per-flash-device basis, as it provides a blunt hammer to disable it for
ALL systems which might use the same kernel (think multiplatform
kernels). And now we have a better alternative: the M25P_NO_FR flag for
flash which do not support fast-read.

However, if we come across SPI controllers which can't handle this, then
we might have a problem. Such a situation would really suggest that we
need to identify whether a SPI controller supports fast-read, not just
the flash device.

(Side note: IMO, given the fact that we have to have an ID table for
these flash anyway, the DT property simply provides redundant
information. In the case of the quad-read patch, we were able to
identify this early to avoid an unnecessary DT binding. But in this
case, we also have an indication of controller support for quad I/O...)

If you still object to this patch, I can drop the patch from l2-mtd.git
for now. Then Sourav will need to rebase his patch again.

Brian

Re: [PATCH 4/5] mtd: m25p80: remove M25PXX_USE_FAST_READ Kconfig

[Marek Vasut]

Dear Brian Norris,

> I see I never responded to this one.
> 
> On Sun, Oct 27, 2013 at 05:32:42PM +0100, Marek Vasut wrote:
> > Hi Brian,
> > 
> > > Remove the compile-time option for FAST_READ, since we have run-time
> > > support for detecting it.
> > > 
> > > Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> > > ---
> > > 
> > >  drivers/mtd/devices/Kconfig  |  7 -------
> > >  drivers/mtd/devices/m25p80.c | 11 ++++++-----
> > >  2 files changed, 6 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/drivers/mtd/devices/Kconfig b/drivers/mtd/devices/Kconfig
> > > index 74ab4b7..0128138 100644
> > > --- a/drivers/mtd/devices/Kconfig
> > > +++ b/drivers/mtd/devices/Kconfig
> > > @@ -95,13 +95,6 @@ config MTD_M25P80
> > > 
> > >  	  if you want to specify device partitioning or to use a device which
> > >  	  doesn't support the JEDEC ID instruction.
> > > 
> > > -config M25PXX_USE_FAST_READ
> > > -	bool "Use FAST_READ OPCode allowing SPI CLK >= 50MHz"
> > > -	depends on MTD_M25P80
> > > -	default y
> > > -	help
> > > -	  This option enables FAST_READ access supported by ST M25Pxx.
> > > -
> > > 
> > >  config MTD_SPEAR_SMI
> > >  
> > >  	tristate "SPEAR MTD NOR Support through SMI controller"
> > >  	depends on PLAT_SPEAR
> > > 
> > > diff --git a/drivers/mtd/devices/m25p80.c
> > > b/drivers/mtd/devices/m25p80.c index 7e3ec7a..d6c5c57 100644
> > > --- a/drivers/mtd/devices/m25p80.c
> > > +++ b/drivers/mtd/devices/m25p80.c
> > > @@ -1055,13 +1055,14 @@ static int m25p_probe(struct spi_device *spi)
> > > 
> > >  	flash->page_size = info->page_size;
> > >  	flash->mtd.writebufsize = flash->page_size;
> > > 
> > > -	flash->fast_read = false;
> > > -	if (np && of_property_read_bool(np, "m25p,fast-read"))
> > > +	if (np)
> > > +		/* If we were instantiated by DT, use it */
> > > +		flash->fast_read = of_property_read_bool(np, "m25p,fast-read");
> > > +	else
> > > +		/* If we weren't instantiated by DT, default to fast-read */
> > > 
> > >  		flash->fast_read = true;
> > 
> > We should default to FALSE , unless explicitly requested by DT, am I
> > wrong? Otherwise this might break the old chips.
> 
> I believe my patch is simply a refactoring of the existing code with the
> assumption that M25PXX_USE_FAST_READ=y. (In my experience, everyone has
> it set to y. Perhaps I'm wrong.)

OK, this is where we diverged. I always set FAST_READ to =n and only enabled it 
via this DT prop if I was dead sure the chip could do it.

> Now, it's unclear to me what this Kconfig was used for. It's the wrong
> approach for controlling fast read on a per-controller or
> per-flash-device basis, as it provides a blunt hammer to disable it for
> ALL systems which might use the same kernel (think multiplatform
> kernels). And now we have a better alternative: the M25P_NO_FR flag for
> flash which do not support fast-read.

This Kconfig was entirely wrong. I suspect it was there from the old times to 
force-enable the fast read and was meant for simple systems with one SPI NOR. 
Multiplatform kernels weren't taken into consideration here, the thing was added 
5 years ago.

commit 2230b76b3838a37167f80487c694d8691248da9f
Date:   Fri Apr 25 12:07:32 2008 +0800

    [MTD] m25p80: add FAST_READ access support to M25Pxx

> However, if we come across SPI controllers which can't handle this, then
> we might have a problem. Such a situation would really suggest that we
> need to identify whether a SPI controller supports fast-read, not just
> the flash device.

The FAST_READ is entirely flash-device thing, it has nothing to do with the 
controller. I wonder why there's this 50 MHz limit in the kernel config at all. 
My understanding of FAST_READ is that after you send FAST_READ opcode, you can 
read all you want until CS is toggled, only then you can send another opcode. 
The "slow" READ opcode on the other hand has to be sent for each block.

> (Side note: IMO, given the fact that we have to have an ID table for
> these flash anyway, the DT property simply provides redundant
> information.

Side note: I wonder how we should handle exotic chips like the N25Q256A which 
recycle the ID for different chips with different capabilities though. We might 
need to have some DT props.

> In the case of the quad-read patch, we were able to
> identify this early to avoid an unnecessary DT binding. But in this
> case, we also have an indication of controller support for quad I/O...)

Yes. I see your point why the fast-read might be redundant.

> If you still object to this patch, I can drop the patch from l2-mtd.git
> for now. Then Sourav will need to rebase his patch again.
> 
> Brian

Re: [PATCH 4/5] mtd: m25p80: remove M25PXX_USE_FAST_READ Kconfig

[Sourav Poddar]

On Thursday 31 October 2013 02:51 PM, Marek Vasut wrote:
> Dear Brian Norris,
>
>> I see I never responded to this one.
>>
>> On Sun, Oct 27, 2013 at 05:32:42PM +0100, Marek Vasut wrote:
>>> Hi Brian,
>>>
>>>> Remove the compile-time option for FAST_READ, since we have run-time
>>>> support for detecting it.
>>>>
>>>> Signed-off-by: Brian Norris<computersforpeace@gmail.com>
>>>> ---
>>>>
>>>>   drivers/mtd/devices/Kconfig  |  7 -------
>>>>   drivers/mtd/devices/m25p80.c | 11 ++++++-----
>>>>   2 files changed, 6 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/drivers/mtd/devices/Kconfig b/drivers/mtd/devices/Kconfig
>>>> index 74ab4b7..0128138 100644
>>>> --- a/drivers/mtd/devices/Kconfig
>>>> +++ b/drivers/mtd/devices/Kconfig
>>>> @@ -95,13 +95,6 @@ config MTD_M25P80
>>>>
>>>>   	  if you want to specify device partitioning or to use a device which
>>>>   	  doesn't support the JEDEC ID instruction.
>>>>
>>>> -config M25PXX_USE_FAST_READ
>>>> -	bool "Use FAST_READ OPCode allowing SPI CLK>= 50MHz"
>>>> -	depends on MTD_M25P80
>>>> -	default y
>>>> -	help
>>>> -	  This option enables FAST_READ access supported by ST M25Pxx.
>>>> -
>>>>
>>>>   config MTD_SPEAR_SMI
>>>>
>>>>   	tristate "SPEAR MTD NOR Support through SMI controller"
>>>>   	depends on PLAT_SPEAR
>>>>
>>>> diff --git a/drivers/mtd/devices/m25p80.c
>>>> b/drivers/mtd/devices/m25p80.c index 7e3ec7a..d6c5c57 100644
>>>> --- a/drivers/mtd/devices/m25p80.c
>>>> +++ b/drivers/mtd/devices/m25p80.c
>>>> @@ -1055,13 +1055,14 @@ static int m25p_probe(struct spi_device *spi)
>>>>
>>>>   	flash->page_size = info->page_size;
>>>>   	flash->mtd.writebufsize = flash->page_size;
>>>>
>>>> -	flash->fast_read = false;
>>>> -	if (np&&  of_property_read_bool(np, "m25p,fast-read"))
>>>> +	if (np)
>>>> +		/* If we were instantiated by DT, use it */
>>>> +		flash->fast_read = of_property_read_bool(np, "m25p,fast-read");
>>>> +	else
>>>> +		/* If we weren't instantiated by DT, default to fast-read */
>>>>
>>>>   		flash->fast_read = true;
>>> We should default to FALSE , unless explicitly requested by DT, am I
>>> wrong? Otherwise this might break the old chips.
>> I believe my patch is simply a refactoring of the existing code with the
>> assumption that M25PXX_USE_FAST_READ=y. (In my experience, everyone has
>> it set to y. Perhaps I'm wrong.)
> OK, this is where we diverged. I always set FAST_READ to =n and only enabled it
> via this DT prop if I was dead sure the chip could do it.
>
>> Now, it's unclear to me what this Kconfig was used for. It's the wrong
>> approach for controlling fast read on a per-controller or
>> per-flash-device basis, as it provides a blunt hammer to disable it for
>> ALL systems which might use the same kernel (think multiplatform
>> kernels). And now we have a better alternative: the M25P_NO_FR flag for
>> flash which do not support fast-read.
> This Kconfig was entirely wrong. I suspect it was there from the old times to
> force-enable the fast read and was meant for simple systems with one SPI NOR.
> Multiplatform kernels weren't taken into consideration here, the thing was added
> 5 years ago.
>
> commit 2230b76b3838a37167f80487c694d8691248da9f
> Date:   Fri Apr 25 12:07:32 2008 +0800
>
>      [MTD] m25p80: add FAST_READ access support to M25Pxx
>
I tried to work around this in my quad patch, where based on some
check, I have tried to assign opcode to quad or fast read. By default, I 
have
kept the command to NORMAL read.
>> However, if we come across SPI controllers which can't handle this, then
>> we might have a problem. Such a situation would really suggest that we
>> need to identify whether a SPI controller supports fast-read, not just
>> the flash device.
> The FAST_READ is entirely flash-device thing, it has nothing to do with the
> controller. I wonder why there's this 50 MHz limit in the kernel config at all.
> My understanding of FAST_READ is that after you send FAST_READ opcode, you can
> read all you want until CS is toggled, only then you can send another opcode.
> The "slow" READ opcode on the other hand has to be sent for each block.
>
I dont think resending commands after each block comes into picture for
slow read. What differnece I see between NORMAL(slow) ad fast read is
just the dummy bytes.
>> (Side note: IMO, given the fact that we have to have an ID table for
>> these flash anyway, the DT property simply provides redundant
>> information.
> Side note: I wonder how we should handle exotic chips like the N25Q256A which
> recycle the ID for different chips with different capabilities though. We might
> need to have some DT props.
>
>> In the case of the quad-read patch, we were able to
>> identify this early to avoid an unnecessary DT binding. But in this
>> case, we also have an indication of controller support for quad I/O...)
> Yes. I see your point why the fast-read might be redundant.
>
>> If you still object to this patch, I can drop the patch from l2-mtd.git
>> for now. Then Sourav will need to rebase his patch again.
>>
>> Brian

Re: [PATCH 4/5] mtd: m25p80: remove M25PXX_USE_FAST_READ Kconfig

[Brian Norris]

Hi Marek,

On Thu, Oct 31, 2013 at 10:21:42AM +0100, Marek Vasut wrote:
> Dear Brian Norris,
> 
> > On Sun, Oct 27, 2013 at 05:32:42PM +0100, Marek Vasut wrote:
> > > Hi Brian,
> > > 
> > > > Remove the compile-time option for FAST_READ, since we have run-time
> > > > support for detecting it.
> > > > 
> > > > Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> > > > ---
> > > > 
> > > >  drivers/mtd/devices/Kconfig  |  7 -------
> > > >  drivers/mtd/devices/m25p80.c | 11 ++++++-----
> > > >  2 files changed, 6 insertions(+), 12 deletions(-)
> > > > 
> > > > diff --git a/drivers/mtd/devices/m25p80.c
> > > > b/drivers/mtd/devices/m25p80.c index 7e3ec7a..d6c5c57 100644
> > > > --- a/drivers/mtd/devices/m25p80.c
> > > > +++ b/drivers/mtd/devices/m25p80.c
> > > > @@ -1055,13 +1055,14 @@ static int m25p_probe(struct spi_device *spi)
> > > > 
> > > >  	flash->page_size = info->page_size;
> > > >  	flash->mtd.writebufsize = flash->page_size;
> > > > 
> > > > -	flash->fast_read = false;
> > > > -	if (np && of_property_read_bool(np, "m25p,fast-read"))
> > > > +	if (np)
> > > > +		/* If we were instantiated by DT, use it */
> > > > +		flash->fast_read = of_property_read_bool(np, "m25p,fast-read");
> > > > +	else
> > > > +		/* If we weren't instantiated by DT, default to fast-read */
> > > > 
> > > >  		flash->fast_read = true;
> > > 
> > > We should default to FALSE , unless explicitly requested by DT, am I
> > > wrong? Otherwise this might break the old chips.
> > 
> > I believe my patch is simply a refactoring of the existing code with the
> > assumption that M25PXX_USE_FAST_READ=y.

Ah, my statement was wrong: for DT-instantiated devices, my patch
"defaulted" as if M25PXX_USE_FAST_READ=n; for devices without a DT node,
it defaulted as if M25PXX_USE_FAST_READ=y.

> > (In my experience, everyone has
> > it set to y. Perhaps I'm wrong.)
> 
> OK, this is where we diverged. I always set FAST_READ to =n and only enabled it 
> via this DT prop if I was dead sure the chip could do it.

Right, that actually makes sense, and your mode is not affected by my
patch. For DT-enabled devices, we default to the DT property.

The only mode that has less flexibility is for platform devices
(non-DT), where we only use normal read for devices that can't handle
FAST READ.

> > Now, it's unclear to me what this Kconfig was used for. It's the wrong
> > approach for controlling fast read on a per-controller or
> > per-flash-device basis, as it provides a blunt hammer to disable it for
> > ALL systems which might use the same kernel (think multiplatform
> > kernels). And now we have a better alternative: the M25P_NO_FR flag for
> > flash which do not support fast-read.
> 
> This Kconfig was entirely wrong. I suspect it was there from the old times to 
> force-enable the fast read and was meant for simple systems with one SPI NOR. 
> Multiplatform kernels weren't taken into consideration here, the thing was added 
> 5 years ago.

Sure, I agree.

> commit 2230b76b3838a37167f80487c694d8691248da9f
> Date:   Fri Apr 25 12:07:32 2008 +0800
> 
>     [MTD] m25p80: add FAST_READ access support to M25Pxx
> 
> > However, if we come across SPI controllers which can't handle this, then
> > we might have a problem. Such a situation would really suggest that we
> > need to identify whether a SPI controller supports fast-read, not just
> > the flash device.
> 
> The FAST_READ is entirely flash-device thing, it has nothing to do with the 
> controller. I wonder why there's this 50 MHz limit in the kernel config at all. 

I have a Spansion S25FL128S datasheet which says:

  "The maximum operating clock frequency for the READ command is 50
  MHz."

And:

  "The maximum operating clock frequency for FAST READ command is 133
  MHz."

But this does suggest, as you say, that the controller provides no
limitation on using FAST READ. However, the extra dummy cycles would be
a *slight* penalty for using FAST READ instead of (normal) READ when run
at a frequency under which either would suffice. But this likely isn't a
significant problem.

> My understanding of FAST_READ is that after you send FAST_READ opcode, you can 
> read all you want until CS is toggled, only then you can send another opcode. 
> The "slow" READ opcode on the other hand has to be sent for each block.

As Sourav said in his response, I don't believe this is true. AIUI, the
only differences are the dummy cycles and the maximum clock frequency.

> > (Side note: IMO, given the fact that we have to have an ID table for
> > these flash anyway, the DT property simply provides redundant
> > information.
> 
> Side note: I wonder how we should handle exotic chips like the N25Q256A which 
> recycle the ID for different chips with different capabilities though. We might 
> need to have some DT props.

Cross that bridge when/if we come to it, I guess? :) Do you know of any
current problems with this device?

> > In the case of the quad-read patch, we were able to
> > identify this early to avoid an unnecessary DT binding. But in this
> > case, we also have an indication of controller support for quad I/O...)
> 
> Yes. I see your point why the fast-read might be redundant.
> 
> > If you still object to this patch, I can drop the patch from l2-mtd.git
> > for now. Then Sourav will need to rebase his patch again.

So what do you think? I feel like my current patch is the right way to
go, but I think I could update the description to be more verbose, since
there are obviously a few other issues coming into play here.

Brian

Re: [PATCH 4/5] mtd: m25p80: remove M25PXX_USE_FAST_READ Kconfig

[Marek Vasut]

Hi Brian,

> Hi Marek,
> 
> On Thu, Oct 31, 2013 at 10:21:42AM +0100, Marek Vasut wrote:
> > Dear Brian Norris,
> > 
> > > On Sun, Oct 27, 2013 at 05:32:42PM +0100, Marek Vasut wrote:
> > > > Hi Brian,
> > > > 
> > > > > Remove the compile-time option for FAST_READ, since we have
> > > > > run-time support for detecting it.
> > > > > 
> > > > > Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> > > > > ---
> > > > > 
> > > > >  drivers/mtd/devices/Kconfig  |  7 -------
> > > > >  drivers/mtd/devices/m25p80.c | 11 ++++++-----
> > > > >  2 files changed, 6 insertions(+), 12 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/mtd/devices/m25p80.c
> > > > > b/drivers/mtd/devices/m25p80.c index 7e3ec7a..d6c5c57 100644
> > > > > --- a/drivers/mtd/devices/m25p80.c
> > > > > +++ b/drivers/mtd/devices/m25p80.c
> > > > > @@ -1055,13 +1055,14 @@ static int m25p_probe(struct spi_device
> > > > > *spi)
> > > > > 
> > > > >  	flash->page_size = info->page_size;
> > > > >  	flash->mtd.writebufsize = flash->page_size;
> > > > > 
> > > > > -	flash->fast_read = false;
> > > > > -	if (np && of_property_read_bool(np, "m25p,fast-read"))
> > > > > +	if (np)
> > > > > +		/* If we were instantiated by DT, use it */
> > > > > +		flash->fast_read = of_property_read_bool(np, "m25p,fast-
read");
> > > > > +	else
> > > > > +		/* If we weren't instantiated by DT, default to fast-
read */
> > > > > 
> > > > >  		flash->fast_read = true;
> > > > 
> > > > We should default to FALSE , unless explicitly requested by DT, am I
> > > > wrong? Otherwise this might break the old chips.
> > > 
> > > I believe my patch is simply a refactoring of the existing code with
> > > the assumption that M25PXX_USE_FAST_READ=y.
> 
> Ah, my statement was wrong: for DT-instantiated devices, my patch
> "defaulted" as if M25PXX_USE_FAST_READ=n; for devices without a DT node,
> it defaulted as if M25PXX_USE_FAST_READ=y.

I'd say old devices (which are not DT capable even) might actually be less able 
to support the FAST READ opcode. But I think we're reaching a point where we 
cannot clearly tell and either way we will break some devices here. If that's 
the case, let's do it ;-)

> > > (In my experience, everyone has
> > > it set to y. Perhaps I'm wrong.)
> > 
> > OK, this is where we diverged. I always set FAST_READ to =n and only
> > enabled it via this DT prop if I was dead sure the chip could do it.
> 
> Right, that actually makes sense, and your mode is not affected by my
> patch. For DT-enabled devices, we default to the DT property.
> 
> The only mode that has less flexibility is for platform devices
> (non-DT), where we only use normal read for devices that can't handle
> FAST READ.

And there we should disable it by default to play safe, no ?

> > > Now, it's unclear to me what this Kconfig was used for. It's the wrong
> > > approach for controlling fast read on a per-controller or
> > > per-flash-device basis, as it provides a blunt hammer to disable it for
> > > ALL systems which might use the same kernel (think multiplatform
> > > kernels). And now we have a better alternative: the M25P_NO_FR flag for
> > > flash which do not support fast-read.
> > 
> > This Kconfig was entirely wrong. I suspect it was there from the old
> > times to force-enable the fast read and was meant for simple systems
> > with one SPI NOR. Multiplatform kernels weren't taken into consideration
> > here, the thing was added 5 years ago.
> 
> Sure, I agree.
> 
> > commit 2230b76b3838a37167f80487c694d8691248da9f
> > Date:   Fri Apr 25 12:07:32 2008 +0800
> > 
> >     [MTD] m25p80: add FAST_READ access support to M25Pxx
> > > 
> > > However, if we come across SPI controllers which can't handle this,
> > > then we might have a problem. Such a situation would really suggest
> > > that we need to identify whether a SPI controller supports fast-read,
> > > not just the flash device.
> > 
> > The FAST_READ is entirely flash-device thing, it has nothing to do with
> > the controller. I wonder why there's this 50 MHz limit in the kernel
> > config at all.
> 
> I have a Spansion S25FL128S datasheet which says:
> 
>   "The maximum operating clock frequency for the READ command is 50
>   MHz."
> 
> And:
> 
>   "The maximum operating clock frequency for FAST READ command is 133
>   MHz."
> 
> But this does suggest, as you say, that the controller provides no
> limitation on using FAST READ. However, the extra dummy cycles would be
> a *slight* penalty for using FAST READ instead of (normal) READ when run
> at a frequency under which either would suffice. But this likely isn't a
> significant problem.

Certainly. All we need to know is whether or not does the chip support FAST READ 
and if so, we can use it.

> > My understanding of FAST_READ is that after you send FAST_READ opcode,
> > you can read all you want until CS is toggled, only then you can send
> > another opcode. The "slow" READ opcode on the other hand has to be sent
> > for each block.
> 
> As Sourav said in his response, I don't believe this is true. AIUI, the
> only differences are the dummy cycles and the maximum clock frequency.

Aka. "slow" READ can also read as many bytes as seen fit ?

> > > (Side note: IMO, given the fact that we have to have an ID table for
> > > these flash anyway, the DT property simply provides redundant
> > > information.
> > 
> > Side note: I wonder how we should handle exotic chips like the N25Q256A
> > which recycle the ID for different chips with different capabilities
> > though. We might need to have some DT props.
> 
> Cross that bridge when/if we come to it, I guess? :) Do you know of any
> current problems with this device?

This particular one? Not in this context, but there are many otherwise.

> > > In the case of the quad-read patch, we were able to
> > > identify this early to avoid an unnecessary DT binding. But in this
> > > case, we also have an indication of controller support for quad I/O...)
> > 
> > Yes. I see your point why the fast-read might be redundant.
> > 
> > > If you still object to this patch, I can drop the patch from l2-mtd.git
> > > for now. Then Sourav will need to rebase his patch again.
> 
> So what do you think? I feel like my current patch is the right way to
> go, but I think I could update the description to be more verbose, since
> there are obviously a few other issues coming into play here.

Yes, I won't hold it any longer. If someone complains, we will know where the 
wind blows from anyway and then we can fix his platform properly.

Re: [PATCH 4/5] mtd: m25p80: remove M25PXX_USE_FAST_READ Kconfig

[Brian Norris]

Hi Marek,

On Fri, Nov 01, 2013 at 01:26:15PM +0100, Marek Vasut wrote:
> Hi Brian,
> 
> > Hi Marek,
> > 
> > On Thu, Oct 31, 2013 at 10:21:42AM +0100, Marek Vasut wrote:
> > > Dear Brian Norris,
> > > 
> > > > On Sun, Oct 27, 2013 at 05:32:42PM +0100, Marek Vasut wrote:
> > > > > Hi Brian,
> > > > > 
> > > > > > --- a/drivers/mtd/devices/m25p80.c
> > > > > > +++ b/drivers/mtd/devices/m25p80.c
> > > > > > @@ -1055,13 +1055,14 @@ static int m25p_probe(struct spi_device *spi)
> > > > > > 
> > > > > >  	flash->page_size = info->page_size;
> > > > > >  	flash->mtd.writebufsize = flash->page_size;
> > > > > > 
> > > > > > -	flash->fast_read = false;
> > > > > > -	if (np && of_property_read_bool(np, "m25p,fast-read"))
> > > > > > +	if (np)
> > > > > > +		/* If we were instantiated by DT, use it */
> > > > > > +		flash->fast_read = of_property_read_bool(np, "m25p,fast-read");
> > > > > > +	else
> > > > > > +		/* If we weren't instantiated by DT, default to fast-read */
> > > > > > 
> > > > > >  		flash->fast_read = true;
> > > > > 
> > > > > We should default to FALSE , unless explicitly requested by DT, am I
> > > > > wrong? Otherwise this might break the old chips.
> > > > 
> > > > I believe my patch is simply a refactoring of the existing code with
> > > > the assumption that M25PXX_USE_FAST_READ=y.
> > 
> > Ah, my statement was wrong: for DT-instantiated devices, my patch
> > "defaulted" as if M25PXX_USE_FAST_READ=n; for devices without a DT node,
> > it defaulted as if M25PXX_USE_FAST_READ=y.
> 
> I'd say old devices (which are not DT capable even) might actually be less able 
> to support the FAST READ opcode. But I think we're reaching a point where we 
> cannot clearly tell and either way we will break some devices here. If that's 
> the case, let's do it ;-)

I agree, let's just do it.

> > > > (In my experience, everyone has
> > > > it set to y. Perhaps I'm wrong.)
> > > 
> > > OK, this is where we diverged. I always set FAST_READ to =n and only
> > > enabled it via this DT prop if I was dead sure the chip could do it.
> > 
> > Right, that actually makes sense, and your mode is not affected by my
> > patch. For DT-enabled devices, we default to the DT property.
> > 
> > The only mode that has less flexibility is for platform devices
> > (non-DT), where we only use normal read for devices that can't handle
> > FAST READ.
> 
> And there we should disable it by default to play safe, no ?

Unless the device table M25P_NO_FR flag is imprecisely-used (which it
may be), I don't think we need a "play-it-safe" option here. The current
code seems to have the right level of precision.

> > > My understanding of FAST_READ is that after you send FAST_READ opcode,
> > > you can read all you want until CS is toggled, only then you can send
> > > another opcode. The "slow" READ opcode on the other hand has to be sent
> > > for each block.
> > 
> > As Sourav said in his response, I don't believe this is true. AIUI, the
> > only differences are the dummy cycles and the maximum clock frequency.
> 
> Aka. "slow" READ can also read as many bytes as seen fit ?

Yes, according to the data sheet I read.

> > > > If you still object to this patch, I can drop the patch from l2-mtd.git
> > > > for now. Then Sourav will need to rebase his patch again.
> > 
> > So what do you think? I feel like my current patch is the right way to
> > go, but I think I could update the description to be more verbose, since
> > there are obviously a few other issues coming into play here.
> 
> Yes, I won't hold it any longer. If someone complains, we will know where the 
> wind blows from anyway and then we can fix his platform properly.

Sounds good. I'm updating the commit to include the following
description (no code changes, so no need to rebase, Sourav):

------------------ BEGIN DESCRIPTION ------------------
Remove the compile-time option for FAST_READ, since we have run-time
support for detecting it. This refactors the logic for enabling
fast-read, such that for DT-enabled devices, we honor the 
"m25p,fast-read" property but for non-DT devices, we default to using 
FAST_READ whenever the flash device supports it according to our
m25p_ids[] table.

Normal READ and FAST_READ differ only in the following:

  * FAST_READ supports SPI higher clock frequencies [1]

  * number of dummy cycles; FAST_READ requires 8 dummy cycles (whereas 
    READ requires 0) to allow the flash sufficient setup time, even when
    running at higher clock speeds

Thus, for flash chips which support FAST_READ, there is otherwise no
limiting reason why we cannot use the FAST_READ opcode instead of READ.
It simply allows the SPI controller to run at higher clock rates. So 
theoretically, nobody should be needing the compile-time option anyway.

  [1] I have a Spansion S25FL128S datasheet which says:

    "The maximum operating clock frequency for the READ command is 50
    MHz."

  And:

    "The maximum operating clock frequency for FAST READ command is 133
    MHz."
-------------------- END DESCRIPTION ------------------

Brian

Re: [PATCH 4/5] mtd: m25p80: remove M25PXX_USE_FAST_READ Kconfig

[Marek Vasut]

Dear Brian Norris,

> Hi Marek,
> 
> On Fri, Nov 01, 2013 at 01:26:15PM +0100, Marek Vasut wrote:
> > Hi Brian,
> > 
> > > Hi Marek,
> > > 
> > > On Thu, Oct 31, 2013 at 10:21:42AM +0100, Marek Vasut wrote:
> > > > Dear Brian Norris,
> > > > 
> > > > > On Sun, Oct 27, 2013 at 05:32:42PM +0100, Marek Vasut wrote:
> > > > > > Hi Brian,
> > > > > > 
> > > > > > > --- a/drivers/mtd/devices/m25p80.c
> > > > > > > +++ b/drivers/mtd/devices/m25p80.c
> > > > > > > @@ -1055,13 +1055,14 @@ static int m25p_probe(struct spi_device
> > > > > > > *spi)
> > > > > > > 
> > > > > > >  	flash->page_size = info->page_size;
> > > > > > >  	flash->mtd.writebufsize = flash->page_size;
> > > > > > > 
> > > > > > > -	flash->fast_read = false;
> > > > > > > -	if (np && of_property_read_bool(np, "m25p,fast-read"))
> > > > > > > +	if (np)
> > > > > > > +		/* If we were instantiated by DT, use it */
> > > > > > > +		flash->fast_read = of_property_read_bool(np,
> > > > > > > "m25p,fast-read"); +	else
> > > > > > > +		/* If we weren't instantiated by DT, default to fast-
read */
> > > > > > > 
> > > > > > >  		flash->fast_read = true;
> > > > > > 
> > > > > > We should default to FALSE , unless explicitly requested by DT,
> > > > > > am I wrong? Otherwise this might break the old chips.
> > > > > 
> > > > > I believe my patch is simply a refactoring of the existing code
> > > > > with the assumption that M25PXX_USE_FAST_READ=y.
> > > 
> > > Ah, my statement was wrong: for DT-instantiated devices, my patch
> > > "defaulted" as if M25PXX_USE_FAST_READ=n; for devices without a DT
> > > node, it defaulted as if M25PXX_USE_FAST_READ=y.
> > 
> > I'd say old devices (which are not DT capable even) might actually be
> > less able to support the FAST READ opcode. But I think we're reaching a
> > point where we cannot clearly tell and either way we will break some
> > devices here. If that's the case, let's do it ;-)
> 
> I agree, let's just do it.
> 
> > > > > (In my experience, everyone has
> > > > > it set to y. Perhaps I'm wrong.)
> > > > 
> > > > OK, this is where we diverged. I always set FAST_READ to =n and only
> > > > enabled it via this DT prop if I was dead sure the chip could do it.
> > > 
> > > Right, that actually makes sense, and your mode is not affected by my
> > > patch. For DT-enabled devices, we default to the DT property.
> > > 
> > > The only mode that has less flexibility is for platform devices
> > > (non-DT), where we only use normal read for devices that can't handle
> > > FAST READ.
> > 
> > And there we should disable it by default to play safe, no ?
> 
> Unless the device table M25P_NO_FR flag is imprecisely-used (which it
> may be), I don't think we need a "play-it-safe" option here. The current
> code seems to have the right level of precision.

Yes, I found this NO_FR flag just yesterday (shame on me!) when I was searching 
for some strange behavior on one of my boards here. I think we're good.

> > > > My understanding of FAST_READ is that after you send FAST_READ
> > > > opcode, you can read all you want until CS is toggled, only then you
> > > > can send another opcode. The "slow" READ opcode on the other hand
> > > > has to be sent for each block.
> > > 
> > > As Sourav said in his response, I don't believe this is true. AIUI, the
> > > only differences are the dummy cycles and the maximum clock frequency.
> > 
> > Aka. "slow" READ can also read as many bytes as seen fit ?
> 
> Yes, according to the data sheet I read.
> 
> > > > > If you still object to this patch, I can drop the patch from
> > > > > l2-mtd.git for now. Then Sourav will need to rebase his patch
> > > > > again.
> > > 
> > > So what do you think? I feel like my current patch is the right way to
> > > go, but I think I could update the description to be more verbose,
> > > since there are obviously a few other issues coming into play here.
> > 
> > Yes, I won't hold it any longer. If someone complains, we will know where
> > the wind blows from anyway and then we can fix his platform properly.
> 
> Sounds good. I'm updating the commit to include the following
> description (no code changes, so no need to rebase, Sourav):

Cool, thanks!

> ------------------ BEGIN DESCRIPTION ------------------
> Remove the compile-time option for FAST_READ, since we have run-time
> support for detecting it. This refactors the logic for enabling
> fast-read, such that for DT-enabled devices, we honor the
> "m25p,fast-read" property but for non-DT devices, we default to using
> FAST_READ whenever the flash device supports it according to our
> m25p_ids[] table.
> 
> Normal READ and FAST_READ differ only in the following:
> 
>   * FAST_READ supports SPI higher clock frequencies [1]
> 
>   * number of dummy cycles; FAST_READ requires 8 dummy cycles (whereas
>     READ requires 0) to allow the flash sufficient setup time, even when
>     running at higher clock speeds
> 
> Thus, for flash chips which support FAST_READ, there is otherwise no
> limiting reason why we cannot use the FAST_READ opcode instead of READ.
> It simply allows the SPI controller to run at higher clock rates. So
> theoretically, nobody should be needing the compile-time option anyway.
> 
>   [1] I have a Spansion S25FL128S datasheet which says:
> 
>     "The maximum operating clock frequency for the READ command is 50
>     MHz."
> 
>   And:
> 
>     "The maximum operating clock frequency for FAST READ command is 133
>     MHz."
> -------------------- END DESCRIPTION ------------------
> 
> Brian

[PATCH 5/5] mtd: m25p80: remove 'disabled' device check

[Brian Norris]

It seems like the following commit was never necessary

    commit 5f949137952020214cd167093dd7be448f21c079
    Author: Shaohui Xie <Shaohui.Xie@freescale.com>
    Date:   Fri Oct 14 15:49:00 2011 +0800

        mtd: m25p80: don't probe device which has status of 'disabled'

because it duplicates the code in of_platform_device_create_pdata()
which ensures that 'disabled' nodes are never instantiated.

Also, drop the __maybe_unused.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
Cc: Grant Likely <grant.likely@linaro.org>
Cc: Rob Herring <rob.herring@calxeda.com>
Cc: <devicetree@vger.kernel.org>
---
 drivers/mtd/devices/m25p80.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index d6c5c57..a1dc49a 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -935,12 +935,7 @@ static int m25p_probe(struct spi_device *spi)
 	struct flash_info		*info;
 	unsigned			i;
 	struct mtd_part_parser_data	ppdata;
-	struct device_node __maybe_unused *np = spi->dev.of_node;
-
-#ifdef CONFIG_MTD_OF_PARTS
-	if (!of_device_is_available(np))
-		return -ENODEV;
-#endif
+	struct device_node *np = spi->dev.of_node;
 
 	/* Platform data helps sort out which chip type we have, as
 	 * well as how this board partitions it.  If we don't have
-- 
1.8.4

Re: [PATCH 5/5] mtd: m25p80: remove 'disabled' device check

[Sourav Poddar]

On Thursday 24 October 2013 08:28 AM, Brian Norris wrote:
> It seems like the following commit was never necessary
>
>      commit 5f949137952020214cd167093dd7be448f21c079
>      Author: Shaohui Xie<Shaohui.Xie@freescale.com>
>      Date:   Fri Oct 14 15:49:00 2011 +0800
>
>          mtd: m25p80: don't probe device which has status of 'disabled'
>
> because it duplicates the code in of_platform_device_create_pdata()
> which ensures that 'disabled' nodes are never instantiated.
>
> Also, drop the __maybe_unused.
>
> Signed-off-by: Brian Norris<computersforpeace@gmail.com>
> Cc: Grant Likely<grant.likely@linaro.org>
> Cc: Rob Herring<rob.herring@calxeda.com>
> Cc:<devicetree@vger.kernel.org>
Reviewed-by: Sourav Poddar <sourav.poddar@ti.com>
> ---
>   drivers/mtd/devices/m25p80.c | 7 +------
>   1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> index d6c5c57..a1dc49a 100644
> --- a/drivers/mtd/devices/m25p80.c
> +++ b/drivers/mtd/devices/m25p80.c
> @@ -935,12 +935,7 @@ static int m25p_probe(struct spi_device *spi)
>   	struct flash_info		*info;
>   	unsigned			i;
>   	struct mtd_part_parser_data	ppdata;
> -	struct device_node __maybe_unused *np = spi->dev.of_node;
> -
> -#ifdef CONFIG_MTD_OF_PARTS
> -	if (!of_device_is_available(np))
> -		return -ENODEV;
> -#endif
> +	struct device_node *np = spi->dev.of_node;
>
>   	/* Platform data helps sort out which chip type we have, as
>   	 * well as how this board partitions it.  If we don't have

Re: [PATCH 5/5] mtd: m25p80: remove 'disabled' device check

[Grant Likely]

On Wed, 23 Oct 2013 19:58:23 -0700, Brian Norris <computersforpeace@gmail.com> wrote:
> It seems like the following commit was never necessary
> 
>     commit 5f949137952020214cd167093dd7be448f21c079
>     Author: Shaohui Xie <Shaohui.Xie@freescale.com>
>     Date:   Fri Oct 14 15:49:00 2011 +0800
> 
>         mtd: m25p80: don't probe device which has status of 'disabled'
> 
> because it duplicates the code in of_platform_device_create_pdata()
> which ensures that 'disabled' nodes are never instantiated.
> 
> Also, drop the __maybe_unused.

Looks reasonable.

Reviewed-by: Grant Likely <grant.likely@linaro.org>

> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> Cc: Grant Likely <grant.likely@linaro.org>
> Cc: Rob Herring <rob.herring@calxeda.com>
> Cc: <devicetree@vger.kernel.org>
> ---
>  drivers/mtd/devices/m25p80.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> index d6c5c57..a1dc49a 100644
> --- a/drivers/mtd/devices/m25p80.c
> +++ b/drivers/mtd/devices/m25p80.c
> @@ -935,12 +935,7 @@ static int m25p_probe(struct spi_device *spi)
>  	struct flash_info		*info;
>  	unsigned			i;
>  	struct mtd_part_parser_data	ppdata;
> -	struct device_node __maybe_unused *np = spi->dev.of_node;
> -
> -#ifdef CONFIG_MTD_OF_PARTS
> -	if (!of_device_is_available(np))
> -		return -ENODEV;
> -#endif
> +	struct device_node *np = spi->dev.of_node;
>  
>  	/* Platform data helps sort out which chip type we have, as
>  	 * well as how this board partitions it.  If we don't have
> -- 
> 1.8.4
> 

Re: [PATCH 5/5] mtd: m25p80: remove 'disabled' device check

[Brian Norris]

On Wed, Oct 23, 2013 at 07:58:23PM -0700, Brian Norris wrote:
> It seems like the following commit was never necessary
> 
>     commit 5f949137952020214cd167093dd7be448f21c079
>     Author: Shaohui Xie <Shaohui.Xie@freescale.com>
>     Date:   Fri Oct 14 15:49:00 2011 +0800
> 
>         mtd: m25p80: don't probe device which has status of 'disabled'
> 
> because it duplicates the code in of_platform_device_create_pdata()
> which ensures that 'disabled' nodes are never instantiated.
> 
> Also, drop the __maybe_unused.
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> Cc: Grant Likely <grant.likely@linaro.org>
> Cc: Rob Herring <rob.herring@calxeda.com>
> Cc: <devicetree@vger.kernel.org>

Pushed to l2-mtd.git.

Brian

Re: [PATCH 1/5] mtd: m25p80: fix allocation size

[Sourav Poddar]

On Thursday 24 October 2013 08:28 AM, Brian Norris wrote:
> This patch fixes two memory errors:
>
> 1. During a probe failure (in mtd_device_parse_register?) the command
>     buffer would not be freed.
>
> 2. The command buffer's size is determined based on the 'fast_read'
>     boolean, but the assignment of fast_read is made after this
>     allocation. Thus, the buffer may be allocated "too small".
>
> To fix the first, just switch to the devres version of kzalloc.
>
> To fix the second, increase MAX_CMD_SIZE unconditionally. It's not worth
> saving a byte to fiddle around with the conditions here.
>
> This problem was reported by Yuhang Wang a while back.
>
> Signed-off-by: Brian Norris<computersforpeace@gmail.com>
> Reported-by: Yuhang Wang<wangyuhang2014@gmail.com>
> Cc:<stable@vger.kernel.org>
Reviewed-by: Sourav Poddar <sourav.poddar@ti.com>
> ---
>   drivers/mtd/devices/m25p80.c | 20 +++++++-------------
>   1 file changed, 7 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> index 8d6c87be..63a95ac 100644
> --- a/drivers/mtd/devices/m25p80.c
> +++ b/drivers/mtd/devices/m25p80.c
> @@ -78,7 +78,7 @@
>
>   /* 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		5
> +#define	MAX_CMD_SIZE		6
>
>   #define JEDEC_MFR(_jedec_id)	((_jedec_id)>>  16)
>
> @@ -996,15 +996,13 @@ static int m25p_probe(struct spi_device *spi)
>   		}
>   	}
>
> -	flash = kzalloc(sizeof *flash, GFP_KERNEL);
> +	flash = devm_kzalloc(&spi->dev, sizeof(*flash), GFP_KERNEL);
>   	if (!flash)
>   		return -ENOMEM;
> -	flash->command = kmalloc(MAX_CMD_SIZE + (flash->fast_read ? 1 : 0),
> -					GFP_KERNEL);
> -	if (!flash->command) {
> -		kfree(flash);
> +
> +	flash->command = devm_kzalloc(&spi->dev, MAX_CMD_SIZE, GFP_KERNEL);
> +	if (!flash->command)
>   		return -ENOMEM;
> -	}
>
>   	flash->spi = spi;
>   	mutex_init(&flash->lock);
> @@ -1137,14 +1135,10 @@ static int m25p_probe(struct spi_device *spi)
>   static int m25p_remove(struct spi_device *spi)
>   {
>   	struct m25p	*flash = spi_get_drvdata(spi);
> -	int		status;
>
>   	/* Clean up MTD stuff. */
> -	status = mtd_device_unregister(&flash->mtd);
> -	if (status == 0) {
> -		kfree(flash->command);
> -		kfree(flash);
> -	}
> +	mtd_device_unregister(&flash->mtd);
> +
>   	return 0;
>   }
>

Re: [PATCH 1/5] mtd: m25p80: fix allocation size

[Brian Norris]

On Wed, Oct 23, 2013 at 07:58:19PM -0700, Brian Norris wrote:
> This patch fixes two memory errors:
> 
> 1. During a probe failure (in mtd_device_parse_register?) the command
>    buffer would not be freed.
> 
> 2. The command buffer's size is determined based on the 'fast_read'
>    boolean, but the assignment of fast_read is made after this
>    allocation. Thus, the buffer may be allocated "too small".
> 
> To fix the first, just switch to the devres version of kzalloc.
> 
> To fix the second, increase MAX_CMD_SIZE unconditionally. It's not worth
> saving a byte to fiddle around with the conditions here.
> 
> This problem was reported by Yuhang Wang a while back.
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> Reported-by: Yuhang Wang <wangyuhang2014@gmail.com>
> Cc: <stable@vger.kernel.org>

I pushed patches 1, 2, and 3 to l2-mtd.git (for Sourav's sake). I'll
wait a little while on the others. Comments are still welcome on the
whole series, though.

Brian

Re: [PATCH 1/5] mtd: m25p80: fix allocation size

[Sourav Poddar]

On Thursday 24 October 2013 10:47 PM, Brian Norris wrote:
> On Wed, Oct 23, 2013 at 07:58:19PM -0700, Brian Norris wrote:
>> This patch fixes two memory errors:
>>
>> 1. During a probe failure (in mtd_device_parse_register?) the command
>>     buffer would not be freed.
>>
>> 2. The command buffer's size is determined based on the 'fast_read'
>>     boolean, but the assignment of fast_read is made after this
>>     allocation. Thus, the buffer may be allocated "too small".
>>
>> To fix the first, just switch to the devres version of kzalloc.
>>
>> To fix the second, increase MAX_CMD_SIZE unconditionally. It's not worth
>> saving a byte to fiddle around with the conditions here.
>>
>> This problem was reported by Yuhang Wang a while back.
>>
>> Signed-off-by: Brian Norris<computersforpeace@gmail.com>
>> Reported-by: Yuhang Wang<wangyuhang2014@gmail.com>
>> Cc:<stable@vger.kernel.org>
> I pushed patches 1, 2, and 3 to l2-mtd.git (for Sourav's sake).
Thanks!
> I'll
> wait a little while on the others. Comments are still welcome on the
> whole series, though.
>
> Brian

Re: [PATCH 1/5] mtd: m25p80: fix allocation size

[Marek Vasut]

Hi Brian,

[...]

> @@ -1137,14 +1135,10 @@ static int m25p_probe(struct spi_device *spi)
>  static int m25p_remove(struct spi_device *spi)
>  {
>  	struct m25p	*flash = spi_get_drvdata(spi);
> -	int		status;
> 
>  	/* Clean up MTD stuff. */
> -	status = mtd_device_unregister(&flash->mtd);
> -	if (status == 0) {
> -		kfree(flash->command);
> -		kfree(flash);
> -	}
> +	mtd_device_unregister(&flash->mtd);
> +
>  	return 0;
>  }

I wonder if we shouldn't return "status" in here in case mtd_device_unregister() 
failed.

Best regards,
Marek Vasut

Re: [PATCH 1/5] mtd: m25p80: fix allocation size

[Brian Norris]

On Sun, Oct 27, 2013 at 9:30 AM, Marek Vasut <marex@denx.de> wrote:
> Hi Brian,
>
> [...]
>
>> @@ -1137,14 +1135,10 @@ static int m25p_probe(struct spi_device *spi)
>>  static int m25p_remove(struct spi_device *spi)
>>  {
>>       struct m25p     *flash = spi_get_drvdata(spi);
>> -     int             status;
>>
>>       /* Clean up MTD stuff. */
>> -     status = mtd_device_unregister(&flash->mtd);
>> -     if (status == 0) {
>> -             kfree(flash->command);
>> -             kfree(flash);
>> -     }
>> +     mtd_device_unregister(&flash->mtd);
>> +
>>       return 0;
>>  }
>
> I wonder if we shouldn't return "status" in here in case mtd_device_unregister()
> failed.

Sure, I thought of that earlier actually. I may send a trivial
follow-up patch for this.

Thanks,
Brian

Re: [PATCH 1/5] mtd: m25p80: fix allocation size

[Marek Vasut]

Hi Brian,

> On Sun, Oct 27, 2013 at 9:30 AM, Marek Vasut <marex@denx.de> wrote:
> > Hi Brian,
> > 
> > [...]
> > 
> >> @@ -1137,14 +1135,10 @@ static int m25p_probe(struct spi_device *spi)
> >> 
> >>  static int m25p_remove(struct spi_device *spi)
> >>  {
> >>  
> >>       struct m25p     *flash = spi_get_drvdata(spi);
> >> 
> >> -     int             status;
> >> 
> >>       /* Clean up MTD stuff. */
> >> 
> >> -     status = mtd_device_unregister(&flash->mtd);
> >> -     if (status == 0) {
> >> -             kfree(flash->command);
> >> -             kfree(flash);
> >> -     }
> >> +     mtd_device_unregister(&flash->mtd);
> >> +
> >> 
> >>       return 0;
> >>  
> >>  }
> > 
> > I wonder if we shouldn't return "status" in here in case
> > mtd_device_unregister() failed.
> 
> Sure, I thought of that earlier actually. I may send a trivial
> follow-up patch for this.

Sounds good, certainly better than mixing two things into one patch. Thanks!

Best regards,
Marek Vasut