| Message ID | 1376302946-11534-3-git-send-email-s.hauer@pengutronix.de |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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 {
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(-)