* [PATCH] m25p80 / fast read
@ 2013-08-12 10:22 Sascha Hauer
2013-08-12 10:22 ` [PATCH 1/4] mtd: m25p80: Pass flags through CAT25_INFO macro Sascha Hauer
` (6 more replies)
0 siblings, 7 replies; 13+ messages in thread
From: Sascha Hauer @ 2013-08-12 10:22 UTC (permalink / raw)
To: linux-mtd; +Cc: linux-kernel, Marek Vasut, Artem Bityutskiy, kernel
The following adds support for the mr25h10 chip to to the m25p80
driver.
This driver currently has the problem that it unconditionally uses
the fast_read command once it's enabled in Kconfig which of course
leads to problems with multiboard kernels. This series adds a flag
to the chip specific data to solve this. Also some unnecessary ifdefs
are removed.
Sascha
----------------------------------------------------------------
Markus Niebel (1):
mtd: m25p80: add support for mr25h10
Sascha Hauer (3):
mtd: m25p80: Pass flags through CAT25_INFO macro
mtd: m25p80: make CONFIG_M25PXX_USE_FAST_READ safe to enable
mtd: m25p80: remove unnecessary ifdef
drivers/mtd/devices/m25p80.c | 23 +++++++++++++----------
1 file changed, 13 insertions(+), 10 deletions(-)
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/4] mtd: m25p80: Pass flags through CAT25_INFO macro
2013-08-12 10:22 [PATCH] m25p80 / fast read Sascha Hauer
@ 2013-08-12 10:22 ` Sascha Hauer
2013-08-12 20:07 ` Marek Vasut
2013-08-12 10:22 ` [PATCH 2/4] mtd: m25p80: make CONFIG_M25PXX_USE_FAST_READ safe to enable Sascha Hauer
` (5 subsequent siblings)
6 siblings, 1 reply; 13+ messages in thread
From: Sascha Hauer @ 2013-08-12 10:22 UTC (permalink / raw)
To: linux-mtd
Cc: linux-kernel, Marek Vasut, Artem Bityutskiy, kernel, Sascha Hauer
The flags may have to be overwritten, so add them to the CAT25_INFO
macro.
Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
drivers/mtd/devices/m25p80.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index 2f3d2a5..75d4391 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -694,13 +694,13 @@ struct flash_info {
.flags = (_flags), \
})
-#define CAT25_INFO(_sector_size, _n_sectors, _page_size, _addr_width) \
+#define CAT25_INFO(_sector_size, _n_sectors, _page_size, _addr_width, _flags) \
((kernel_ulong_t)&(struct flash_info) { \
.sector_size = (_sector_size), \
.n_sectors = (_n_sectors), \
.page_size = (_page_size), \
.addr_width = (_addr_width), \
- .flags = M25P_NO_ERASE, \
+ .flags = (_flags), \
})
/* NOTE: double check command sets and memory organization when you add
@@ -732,7 +732,7 @@ static const struct spi_device_id m25p_ids[] = {
{ "en25qh256", INFO(0x1c7019, 0, 64 * 1024, 512, 0) },
/* Everspin */
- { "mr25h256", CAT25_INFO( 32 * 1024, 1, 256, 2) },
+ { "mr25h256", CAT25_INFO( 32 * 1024, 1, 256, 2, M25P_NO_ERASE) },
/* GigaDevice */
{ "gd25q32", INFO(0xc84016, 0, 64 * 1024, 64, SECT_4K) },
@@ -846,11 +846,11 @@ static const struct spi_device_id m25p_ids[] = {
{ "w25q256", INFO(0xef4019, 0, 64 * 1024, 512, SECT_4K) },
/* Catalyst / On Semiconductor -- non-JEDEC */
- { "cat25c11", CAT25_INFO( 16, 8, 16, 1) },
- { "cat25c03", CAT25_INFO( 32, 8, 16, 2) },
- { "cat25c09", CAT25_INFO( 128, 8, 32, 2) },
- { "cat25c17", CAT25_INFO( 256, 8, 32, 2) },
- { "cat25128", CAT25_INFO(2048, 8, 64, 2) },
+ { "cat25c11", CAT25_INFO( 16, 8, 16, 1, M25P_NO_ERASE) },
+ { "cat25c03", CAT25_INFO( 32, 8, 16, 2, M25P_NO_ERASE) },
+ { "cat25c09", CAT25_INFO( 128, 8, 32, 2, M25P_NO_ERASE) },
+ { "cat25c17", CAT25_INFO( 256, 8, 32, 2, M25P_NO_ERASE) },
+ { "cat25128", CAT25_INFO(2048, 8, 64, 2, M25P_NO_ERASE) },
{ },
};
MODULE_DEVICE_TABLE(spi, m25p_ids);
--
1.8.4.rc1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/4] mtd: m25p80: make CONFIG_M25PXX_USE_FAST_READ safe to enable
2013-08-12 10:22 [PATCH] m25p80 / fast read Sascha Hauer
2013-08-12 10:22 ` [PATCH 1/4] mtd: m25p80: Pass flags through CAT25_INFO macro Sascha Hauer
@ 2013-08-12 10:22 ` Sascha Hauer
2013-08-17 20:17 ` Brian Norris
2013-08-12 10:22 ` [PATCH 3/4] mtd: m25p80: add support for mr25h10 Sascha Hauer
` (4 subsequent siblings)
6 siblings, 1 reply; 13+ messages in thread
From: Sascha Hauer @ 2013-08-12 10:22 UTC (permalink / raw)
To: linux-mtd
Cc: linux-kernel, Marek Vasut, Artem Bityutskiy, kernel, Sascha Hauer
This patch adds a flag to struct flash_info indicating that
fast_read is not supported. This now gives the following logic
when determing whether to enable fastread:
1) enable fast_read if device node contains m25p,fast-read
2) enable fast_read unconditionally if forced in Kconfig
3) Disable fast_read if the chip does not support it
This makes enabling CONFIG_M25PXX_USE_FAST_READ a safe option
since we no longer enable the fast_read option unconditionally.
For now fast_read is disabled for the everspin mr25h256 and the
catalyst devices. Others may need the flag aswell.
Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
drivers/mtd/devices/m25p80.c | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index 75d4391..8e30b07 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -682,6 +682,7 @@ struct flash_info {
#define SECT_4K 0x01 /* OPCODE_BE_4K works uniformly */
#define M25P_NO_ERASE 0x02 /* No erase command needed */
#define SST_WRITE 0x04 /* use SST byte programming */
+#define M25P_NO_FR 0x08 /* Can't do fastread */
};
#define INFO(_jedec_id, _ext_id, _sector_size, _n_sectors, _flags) \
@@ -732,7 +733,7 @@ static const struct spi_device_id m25p_ids[] = {
{ "en25qh256", INFO(0x1c7019, 0, 64 * 1024, 512, 0) },
/* Everspin */
- { "mr25h256", CAT25_INFO( 32 * 1024, 1, 256, 2, M25P_NO_ERASE) },
+ { "mr25h256", CAT25_INFO( 32 * 1024, 1, 256, 2, M25P_NO_ERASE | M25P_NO_FR) },
/* GigaDevice */
{ "gd25q32", INFO(0xc84016, 0, 64 * 1024, 64, SECT_4K) },
@@ -846,11 +847,11 @@ static const struct spi_device_id m25p_ids[] = {
{ "w25q256", INFO(0xef4019, 0, 64 * 1024, 512, SECT_4K) },
/* Catalyst / On Semiconductor -- non-JEDEC */
- { "cat25c11", CAT25_INFO( 16, 8, 16, 1, M25P_NO_ERASE) },
- { "cat25c03", CAT25_INFO( 32, 8, 16, 2, M25P_NO_ERASE) },
- { "cat25c09", CAT25_INFO( 128, 8, 32, 2, M25P_NO_ERASE) },
- { "cat25c17", CAT25_INFO( 256, 8, 32, 2, M25P_NO_ERASE) },
- { "cat25128", CAT25_INFO(2048, 8, 64, 2, M25P_NO_ERASE) },
+ { "cat25c11", CAT25_INFO( 16, 8, 16, 1, M25P_NO_ERASE | M25P_NO_FR) },
+ { "cat25c03", CAT25_INFO( 32, 8, 16, 2, M25P_NO_ERASE | M25P_NO_FR) },
+ { "cat25c09", CAT25_INFO( 128, 8, 32, 2, M25P_NO_ERASE | M25P_NO_FR) },
+ { "cat25c17", CAT25_INFO( 256, 8, 32, 2, M25P_NO_ERASE | M25P_NO_FR) },
+ { "cat25128", CAT25_INFO(2048, 8, 64, 2, M25P_NO_ERASE | M25P_NO_FR) },
{ },
};
MODULE_DEVICE_TABLE(spi, m25p_ids);
@@ -1037,6 +1038,9 @@ static int m25p_probe(struct spi_device *spi)
flash->fast_read = true;
#endif
+ if (info->flags & M25P_NO_FR)
+ flash->fast_read = false;
+
if (info->addr_width)
flash->addr_width = info->addr_width;
else {
--
1.8.4.rc1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 3/4] mtd: m25p80: add support for mr25h10
2013-08-12 10:22 [PATCH] m25p80 / fast read Sascha Hauer
2013-08-12 10:22 ` [PATCH 1/4] mtd: m25p80: Pass flags through CAT25_INFO macro Sascha Hauer
2013-08-12 10:22 ` [PATCH 2/4] mtd: m25p80: make CONFIG_M25PXX_USE_FAST_READ safe to enable Sascha Hauer
@ 2013-08-12 10:22 ` Sascha Hauer
2013-08-12 10:22 ` [PATCH 4/4] mtd: m25p80: remove unnecessary ifdef Sascha Hauer
` (3 subsequent siblings)
6 siblings, 0 replies; 13+ messages in thread
From: Sascha Hauer @ 2013-08-12 10:22 UTC (permalink / raw)
To: linux-mtd
Cc: linux-kernel, Marek Vasut, Artem Bityutskiy, kernel,
Markus Niebel, Sascha Hauer
From: Markus Niebel <Markus.Niebel@tqs.de>
This adds support for the Everspin mr25h10 MRAM chip to the m25p80
driver.
Signed-off-by: Markus Niebel <Markus.Niebel@tqs.de>
Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
drivers/mtd/devices/m25p80.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index 8e30b07..d179188 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -734,6 +734,7 @@ static const struct spi_device_id m25p_ids[] = {
/* 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) },
/* GigaDevice */
{ "gd25q32", INFO(0xc84016, 0, 64 * 1024, 64, SECT_4K) },
--
1.8.4.rc1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 4/4] mtd: m25p80: remove unnecessary ifdef
2013-08-12 10:22 [PATCH] m25p80 / fast read Sascha Hauer
` (2 preceding siblings ...)
2013-08-12 10:22 ` [PATCH 3/4] mtd: m25p80: add support for mr25h10 Sascha Hauer
@ 2013-08-12 10:22 ` Sascha Hauer
2013-08-20 4:27 ` Brian Norris
2013-08-12 20:13 ` [PATCH] m25p80 / fast read Marek Vasut
` (2 subsequent siblings)
6 siblings, 1 reply; 13+ messages in thread
From: Sascha Hauer @ 2013-08-12 10:22 UTC (permalink / raw)
To: linux-mtd
Cc: linux-kernel, Marek Vasut, Artem Bityutskiy, kernel, Sascha Hauer
of_property_read_bool properly compiles away, no need to ifdef this
for non DT builds.
Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
drivers/mtd/devices/m25p80.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index d179188..f21af0e 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -1030,10 +1030,8 @@ static int m25p_probe(struct spi_device *spi)
flash->mtd.writebufsize = flash->page_size;
flash->fast_read = false;
-#ifdef CONFIG_OF
if (np && of_property_read_bool(np, "m25p,fast-read"))
flash->fast_read = true;
-#endif
#ifdef CONFIG_M25PXX_USE_FAST_READ
flash->fast_read = true;
--
1.8.4.rc1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/4] mtd: m25p80: Pass flags through CAT25_INFO macro
2013-08-12 10:22 ` [PATCH 1/4] mtd: m25p80: Pass flags through CAT25_INFO macro Sascha Hauer
@ 2013-08-12 20:07 ` Marek Vasut
0 siblings, 0 replies; 13+ messages in thread
From: Marek Vasut @ 2013-08-12 20:07 UTC (permalink / raw)
To: Sascha Hauer; +Cc: linux-mtd, linux-kernel, Artem Bityutskiy, kernel
Dear Sascha Hauer,
> The flags may have to be overwritten, so add them to the CAT25_INFO
> macro.
>
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
> drivers/mtd/devices/m25p80.c | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> index 2f3d2a5..75d4391 100644
> --- a/drivers/mtd/devices/m25p80.c
> +++ b/drivers/mtd/devices/m25p80.c
> @@ -694,13 +694,13 @@ struct flash_info {
> .flags = (_flags), \
> })
>
> -#define CAT25_INFO(_sector_size, _n_sectors, _page_size, _addr_width)
\
> +#define CAT25_INFO(_sector_size, _n_sectors, _page_size, _addr_width,
> _flags) \ ((kernel_ulong_t)&(struct flash_info) {
\
> .sector_size = (_sector_size), \
> .n_sectors = (_n_sectors), \
> .page_size = (_page_size), \
> .addr_width = (_addr_width), \
> - .flags = M25P_NO_ERASE, \
> + .flags = (_flags), \
> })
>
> /* NOTE: double check command sets and memory organization when you add
> @@ -732,7 +732,7 @@ static const struct spi_device_id m25p_ids[] = {
> { "en25qh256", INFO(0x1c7019, 0, 64 * 1024, 512, 0) },
>
> /* Everspin */
> - { "mr25h256", CAT25_INFO( 32 * 1024, 1, 256, 2) },
> + { "mr25h256", CAT25_INFO( 32 * 1024, 1, 256, 2, M25P_NO_ERASE) },
>
> /* GigaDevice */
> { "gd25q32", INFO(0xc84016, 0, 64 * 1024, 64, SECT_4K) },
> @@ -846,11 +846,11 @@ static const struct spi_device_id m25p_ids[] = {
> { "w25q256", INFO(0xef4019, 0, 64 * 1024, 512, SECT_4K) },
>
> /* Catalyst / On Semiconductor -- non-JEDEC */
> - { "cat25c11", CAT25_INFO( 16, 8, 16, 1) },
> - { "cat25c03", CAT25_INFO( 32, 8, 16, 2) },
> - { "cat25c09", CAT25_INFO( 128, 8, 32, 2) },
> - { "cat25c17", CAT25_INFO( 256, 8, 32, 2) },
> - { "cat25128", CAT25_INFO(2048, 8, 64, 2) },
> + { "cat25c11", CAT25_INFO( 16, 8, 16, 1, M25P_NO_ERASE) },
> + { "cat25c03", CAT25_INFO( 32, 8, 16, 2, M25P_NO_ERASE) },
> + { "cat25c09", CAT25_INFO( 128, 8, 32, 2, M25P_NO_ERASE) },
> + { "cat25c17", CAT25_INFO( 256, 8, 32, 2, M25P_NO_ERASE) },
> + { "cat25128", CAT25_INFO(2048, 8, 64, 2, M25P_NO_ERASE) },
> { },
> };
> MODULE_DEVICE_TABLE(spi, m25p_ids);
I can't say I like the growing macro magic (voodoo?) in this file, but your
patch doesn't break anything and I don't feel like reworking this, so:
Acked-by: Marek Vasut <marex@denx.de>
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] m25p80 / fast read
2013-08-12 10:22 [PATCH] m25p80 / fast read Sascha Hauer
` (3 preceding siblings ...)
2013-08-12 10:22 ` [PATCH 4/4] mtd: m25p80: remove unnecessary ifdef Sascha Hauer
@ 2013-08-12 20:13 ` Marek Vasut
2013-08-16 9:53 ` Sascha Hauer
2013-08-16 13:54 ` Artem Bityutskiy
6 siblings, 0 replies; 13+ messages in thread
From: Marek Vasut @ 2013-08-12 20:13 UTC (permalink / raw)
To: Sascha Hauer; +Cc: linux-mtd, linux-kernel, Artem Bityutskiy, kernel
Dear Sascha Hauer,
> The following adds support for the mr25h10 chip to to the m25p80
> driver.
>
> This driver currently has the problem that it unconditionally uses
> the fast_read command once it's enabled in Kconfig which of course
> leads to problems with multiboard kernels. This series adds a flag
> to the chip specific data to solve this. Also some unnecessary ifdefs
> are removed.
>
> Sascha
Went through the whole series,
Acked-by: Marek Vasut <marex@denx.de>
Thanks!
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] m25p80 / fast read
2013-08-12 10:22 [PATCH] m25p80 / fast read Sascha Hauer
` (4 preceding siblings ...)
2013-08-12 20:13 ` [PATCH] m25p80 / fast read Marek Vasut
@ 2013-08-16 9:53 ` Sascha Hauer
2013-08-16 13:54 ` Artem Bityutskiy
6 siblings, 0 replies; 13+ messages in thread
From: Sascha Hauer @ 2013-08-16 9:53 UTC (permalink / raw)
To: Artem Bityutskiy; +Cc: linux-mtd, Marek Vasut, linux-kernel, kernel
Artem,
Any chance you take this for the next merge window?
Thanks
Sascha
On Mon, Aug 12, 2013 at 12:22:22PM +0200, Sascha Hauer wrote:
> The following adds support for the mr25h10 chip to to the m25p80
> driver.
>
> This driver currently has the problem that it unconditionally uses
> the fast_read command once it's enabled in Kconfig which of course
> leads to problems with multiboard kernels. This series adds a flag
> to the chip specific data to solve this. Also some unnecessary ifdefs
> are removed.
>
> Sascha
>
> ----------------------------------------------------------------
> Markus Niebel (1):
> mtd: m25p80: add support for mr25h10
>
> Sascha Hauer (3):
> mtd: m25p80: Pass flags through CAT25_INFO macro
> mtd: m25p80: make CONFIG_M25PXX_USE_FAST_READ safe to enable
> mtd: m25p80: remove unnecessary ifdef
>
> drivers/mtd/devices/m25p80.c | 23 +++++++++++++----------
> 1 file changed, 13 insertions(+), 10 deletions(-)
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] m25p80 / fast read
2013-08-12 10:22 [PATCH] m25p80 / fast read Sascha Hauer
` (5 preceding siblings ...)
2013-08-16 9:53 ` Sascha Hauer
@ 2013-08-16 13:54 ` Artem Bityutskiy
6 siblings, 0 replies; 13+ messages in thread
From: Artem Bityutskiy @ 2013-08-16 13:54 UTC (permalink / raw)
To: Sascha Hauer; +Cc: linux-mtd, Marek Vasut, linux-kernel, kernel
On Mon, 2013-08-12 at 12:22 +0200, Sascha Hauer wrote:
> The following adds support for the mr25h10 chip to to the m25p80
> driver.
>
> This driver currently has the problem that it unconditionally uses
> the fast_read command once it's enabled in Kconfig which of course
> leads to problems with multiboard kernels. This series adds a flag
> to the chip specific data to solve this. Also some unnecessary ifdefs
> are removed.
Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
I'll let Biran take a look, and tomorrow apply, unless he applies
himself or has objections.
--
Best Regards,
Artem Bityutskiy
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/4] mtd: m25p80: make CONFIG_M25PXX_USE_FAST_READ safe to enable
2013-08-12 10:22 ` [PATCH 2/4] mtd: m25p80: make CONFIG_M25PXX_USE_FAST_READ safe to enable Sascha Hauer
@ 2013-08-17 20:17 ` Brian Norris
2013-08-19 8:42 ` Sascha Hauer
0 siblings, 1 reply; 13+ messages in thread
From: Brian Norris @ 2013-08-17 20:17 UTC (permalink / raw)
To: Sascha Hauer
Cc: linux-mtd, Marek Vasut, Artem Bityutskiy, linux-kernel, kernel
On Mon, Aug 12, 2013 at 12:22:24PM +0200, Sascha Hauer wrote:
> This patch adds a flag to struct flash_info indicating that
> fast_read is not supported. This now gives the following logic
> when determing whether to enable fastread:
>
> 1) enable fast_read if device node contains m25p,fast-read
> 2) enable fast_read unconditionally if forced in Kconfig
> 3) Disable fast_read if the chip does not support it
This logic is either unclear or incorrect.
> This makes enabling CONFIG_M25PXX_USE_FAST_READ a safe option
> since we no longer enable the fast_read option unconditionally.
This statement seems to contradict 2 above, depending on the reading
(how can 2 enable "unconditionally", yet CONFIG_..._FAST_READ "no longer
enable[s] ... unconditionally"?).
The problem I have with this description is that it is assuming that
1, 2, and 3 are applied sequentially, so that later items in the
sequence have higher precedence. So it's describing code ordering, not
really logic. And statement 3 weakens the "unconditionally" of 2.
And to avoid simply complaining, I propose an alternative explanation:
If the flash chip does not support fast_read, then disable it.
Otherwise:
1) enable fast_read if device node contains m25p,fast-read
2) enable fast_read if forced in Kconfig
If we correct this description, then:
Acked-by: Brian Norris <computersforpeace@gmail.com>
I can edit the patch and push the whole thing if this is acceptable.
One related question (not required for this series): do we even need
Kconfig M25PXX_USE_FAST_READ any more? Are there any SPI controllers
that can't use FAST_READ? Or perhaps if they have a slow clock, it's
preferable to use normal read?
If there are no restrictions from the controller side, I think this
NO_FR flag gives enough information to determine everything at runtime,
not compile-time.
Brian
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/4] mtd: m25p80: make CONFIG_M25PXX_USE_FAST_READ safe to enable
2013-08-17 20:17 ` Brian Norris
@ 2013-08-19 8:42 ` Sascha Hauer
2013-08-20 4:20 ` Brian Norris
0 siblings, 1 reply; 13+ messages in thread
From: Sascha Hauer @ 2013-08-19 8:42 UTC (permalink / raw)
To: Brian Norris
Cc: linux-mtd, Marek Vasut, Artem Bityutskiy, linux-kernel, kernel
On Sat, Aug 17, 2013 at 01:17:02PM -0700, Brian Norris wrote:
> On Mon, Aug 12, 2013 at 12:22:24PM +0200, Sascha Hauer wrote:
> > This patch adds a flag to struct flash_info indicating that
> > fast_read is not supported. This now gives the following logic
> > when determing whether to enable fastread:
> >
> > 1) enable fast_read if device node contains m25p,fast-read
> > 2) enable fast_read unconditionally if forced in Kconfig
> > 3) Disable fast_read if the chip does not support it
>
> This logic is either unclear or incorrect.
>
> > This makes enabling CONFIG_M25PXX_USE_FAST_READ a safe option
> > since we no longer enable the fast_read option unconditionally.
>
> This statement seems to contradict 2 above, depending on the reading
> (how can 2 enable "unconditionally", yet CONFIG_..._FAST_READ "no longer
> enable[s] ... unconditionally"?).
>
> The problem I have with this description is that it is assuming that
> 1, 2, and 3 are applied sequentially, so that later items in the
> sequence have higher precedence. So it's describing code ordering, not
> really logic. And statement 3 weakens the "unconditionally" of 2.
>
> And to avoid simply complaining, I propose an alternative explanation:
>
> If the flash chip does not support fast_read, then disable it.
> Otherwise:
> 1) enable fast_read if device node contains m25p,fast-read
> 2) enable fast_read if forced in Kconfig
>
> If we correct this description, then:
>
> Acked-by: Brian Norris <computersforpeace@gmail.com>
>
> I can edit the patch and push the whole thing if this is acceptable.
Yes, that would be great. Your explanation sounds better than mine.
>
> One related question (not required for this series): do we even need
> Kconfig M25PXX_USE_FAST_READ any more? Are there any SPI controllers
> that can't use FAST_READ? Or perhaps if they have a slow clock, it's
> preferable to use normal read?
>
> If there are no restrictions from the controller side, I think this
> NO_FR flag gives enough information to determine everything at runtime,
> not compile-time.
This M25PXX_USE_FAST_READ is a no-go for multiplatform Kernels and
should be removed. I have no idea though how we can do this without
risking regressions since we have no idea who intentionally disabled
this option. Maybe we just have to find out by removing it and waiting
for people to complain^B^B^Bsend patches.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/4] mtd: m25p80: make CONFIG_M25PXX_USE_FAST_READ safe to enable
2013-08-19 8:42 ` Sascha Hauer
@ 2013-08-20 4:20 ` Brian Norris
0 siblings, 0 replies; 13+ messages in thread
From: Brian Norris @ 2013-08-20 4:20 UTC (permalink / raw)
To: Sascha Hauer
Cc: linux-mtd, Marek Vasut, Artem Bityutskiy, linux-kernel, kernel
On Mon, Aug 19, 2013 at 10:42:26AM +0200, Sascha Hauer wrote:
> On Sat, Aug 17, 2013 at 01:17:02PM -0700, Brian Norris wrote:
> > On Mon, Aug 12, 2013 at 12:22:24PM +0200, Sascha Hauer wrote:
> > > This patch adds a flag to struct flash_info indicating that
> > > fast_read is not supported. This now gives the following logic
> > > when determing whether to enable fastread:
> > >
> > > 1) enable fast_read if device node contains m25p,fast-read
> > > 2) enable fast_read unconditionally if forced in Kconfig
> > > 3) Disable fast_read if the chip does not support it
> >
> > This logic is either unclear or incorrect.
> >
> > > This makes enabling CONFIG_M25PXX_USE_FAST_READ a safe option
> > > since we no longer enable the fast_read option unconditionally.
> >
> > This statement seems to contradict 2 above, depending on the reading
> > (how can 2 enable "unconditionally", yet CONFIG_..._FAST_READ "no longer
> > enable[s] ... unconditionally"?).
> >
> > The problem I have with this description is that it is assuming that
> > 1, 2, and 3 are applied sequentially, so that later items in the
> > sequence have higher precedence. So it's describing code ordering, not
> > really logic. And statement 3 weakens the "unconditionally" of 2.
> >
> > And to avoid simply complaining, I propose an alternative explanation:
> >
> > If the flash chip does not support fast_read, then disable it.
> > Otherwise:
> > 1) enable fast_read if device node contains m25p,fast-read
> > 2) enable fast_read if forced in Kconfig
> >
> > If we correct this description, then:
> >
> > Acked-by: Brian Norris <computersforpeace@gmail.com>
> >
> > I can edit the patch and push the whole thing if this is acceptable.
>
> Yes, that would be great. Your explanation sounds better than mine.
Can you incorporate this description and resend based on l2-mtd.git?
Your patch currently doesn't apply.
http://git.infradead.org/l2-mtd.git
> >
> > One related question (not required for this series): do we even need
> > Kconfig M25PXX_USE_FAST_READ any more? Are there any SPI controllers
> > that can't use FAST_READ? Or perhaps if they have a slow clock, it's
> > preferable to use normal read?
> >
> > If there are no restrictions from the controller side, I think this
> > NO_FR flag gives enough information to determine everything at runtime,
> > not compile-time.
>
> This M25PXX_USE_FAST_READ is a no-go for multiplatform Kernels and
> should be removed. I have no idea though how we can do this without
> risking regressions since we have no idea who intentionally disabled
> this option. Maybe we just have to find out by removing it and waiting
> for people to complain^B^B^Bsend patches.
I don't think removal would be too bad. And yes, we can just wait for
complaints :) I may send a patch after your series.
Thanks,
Brian
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 4/4] mtd: m25p80: remove unnecessary ifdef
2013-08-12 10:22 ` [PATCH 4/4] mtd: m25p80: remove unnecessary ifdef Sascha Hauer
@ 2013-08-20 4:27 ` Brian Norris
0 siblings, 0 replies; 13+ messages in thread
From: Brian Norris @ 2013-08-20 4:27 UTC (permalink / raw)
To: Sascha Hauer
Cc: linux-mtd, Marek Vasut, Artem Bityutskiy, linux-kernel, kernel
On Mon, Aug 12, 2013 at 12:22:26PM +0200, Sascha Hauer wrote:
> of_property_read_bool properly compiles away, no need to ifdef this
> for non DT builds.
>
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
Pushed only this one to l2-mtd.git, since it's independent.
Thanks,
Brian
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2013-08-20 4:27 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-08-12 10:22 [PATCH] m25p80 / fast read Sascha Hauer
2013-08-12 10:22 ` [PATCH 1/4] mtd: m25p80: Pass flags through CAT25_INFO macro Sascha Hauer
2013-08-12 20:07 ` Marek Vasut
2013-08-12 10:22 ` [PATCH 2/4] mtd: m25p80: make CONFIG_M25PXX_USE_FAST_READ safe to enable Sascha Hauer
2013-08-17 20:17 ` Brian Norris
2013-08-19 8:42 ` Sascha Hauer
2013-08-20 4:20 ` Brian Norris
2013-08-12 10:22 ` [PATCH 3/4] mtd: m25p80: add support for mr25h10 Sascha Hauer
2013-08-12 10:22 ` [PATCH 4/4] mtd: m25p80: remove unnecessary ifdef Sascha Hauer
2013-08-20 4:27 ` Brian Norris
2013-08-12 20:13 ` [PATCH] m25p80 / fast read Marek Vasut
2013-08-16 9:53 ` Sascha Hauer
2013-08-16 13:54 ` Artem Bityutskiy
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).