| Message ID | CAKbvp-NuFWzCCT+eo-WO47kazNuHbcA4KxhjDLYGR9p9Jh9FRg@mail.gmail.com |
|---|---|
| State | Rejected |
| Headers | show |
On Saturday, August 16, 2014 at 08:14:05 PM, Jeremy Garff wrote: > This is my first submission to the linux-mtd list. I found Found yes > and fixed Fixed no ;-) > a issue related to the i.MX6 when using a N25Q00A spi-nor as the boot > device. The following is a patch and more detailed description. As > this is my first submission to this list, so please let me know you > have any questions or concerns. Well, the problem is that the SPI NOR ends in undefined state when you reset the board. The problem is not in Linux, the problem is in your board. You should use the RESET-OUTPUT pin of your MX6 to implement proper reset sequencing for the SPI NOR. This patch would not work in case you were doing a PageProgram operation and I came around and mashed the 'reset' button on your board. The SPI NOR would be somewhere in the middle of the PP and your MX6 would simply not boot. So the solution here is to fix your hardware ;-) btw this is a common issue, we should really document this before more people get hurt. btw2 Spansion does much better job, their SPI NORs have RESET pin, so you don't need to implement logic to power cycle the SPI NOR. Best regards, Marek Vasut
On Thu, Sep 25, 2014 at 11:09 AM, Marek Vasut <marex@denx.de> wrote: > On Saturday, August 16, 2014 at 08:14:05 PM, Jeremy Garff wrote: >> This is my first submission to the linux-mtd list. I found > > Found yes > >> and fixed > > Fixed no ;-) > >> a issue related to the i.MX6 when using a N25Q00A spi-nor as the boot >> device. The following is a patch and more detailed description. As >> this is my first submission to this list, so please let me know you >> have any questions or concerns. > > Well, the problem is that the SPI NOR ends in undefined state when you reset the > board. The problem is not in Linux, the problem is in your board. You should use > the RESET-OUTPUT pin of your MX6 to implement proper reset sequencing for the > SPI NOR. There is no reset pin on this particular SPI NOR device, therein lies the problem. http://www.micron.com/-/media/documents/products/data%20sheet/nor%20flash/serial%20nor/n25q/n25q_1gb_1_8v_65nm.pdf > > This patch would not work in case you were doing a PageProgram operation and I > came around and mashed the 'reset' button on your board. The SPI NOR would be > somewhere in the middle of the PP and your MX6 would simply not boot. > It's actually worse than this. _Any_ time the reset button is pushed after linux initializes the device, the system won't boot. Not just during a page program. Obviously this problem is outside the scope of my fix, but it does however fix the case where the user initiates the reboot. > So the solution here is to fix your hardware ;-) > Easier said than done. In many circumstances, hardware fixes like the one you suggest are outside the control of the developer and/or incur additional board and part costs. In my opinion, the real fix for this issue is in the Freescale boot ROM, which should do a soft reset of the SPI NOR prior to any reads. Unfortunately it doesn't do this, and can't be changed except by Freescale. > btw this is a common issue, we should really document this before more people > get hurt. > What you are effectively saying is that you simply can't and shouldn't use this part with a i.MX6. But why not implement the fix if the following are true? - It's as you say, a common issue. - A hardware fix may be outside the developers control or parts are fielded. - The fix simply puts the part in a consistent state on reboot/shutdown and seems reasonable. Thanks, Jeremy
On Friday, September 26, 2014 at 09:46:10 PM, Jeremy Garff wrote: [...] > > This patch would not work in case you were doing a PageProgram operation > > and I came around and mashed the 'reset' button on your board. The SPI > > NOR would be somewhere in the middle of the PP and your MX6 would simply > > not boot. > > It's actually worse than this. _Any_ time the reset button is pushed > after linux initializes the device, the system won't boot. Not just > during a page program. Obviously this problem is outside the scope of > my fix, but it does however fix the case where the user initiates the > reboot. That's just papering over bugs. > > So the solution here is to fix your hardware ;-) > > Easier said than done. In many circumstances, hardware fixes like the > one you suggest are outside the control of the developer and/or incur > additional board and part costs. Again, this doesn't justify papering over bugs. It's a plain and simple hardware bug. > In my opinion, the real fix for this issue is in the Freescale boot > ROM, which should do a soft reset of the SPI NOR prior to any reads. > Unfortunately it doesn't do this, and can't be changed except by > Freescale. In case the SPI NOR is completely stuck, the only way to bring it into defined state is to power-cycle it or toggle it's reset pin. Software reset would again be just a plaster as the SPI NOR might ignore it. > > btw this is a common issue, we should really document this before more > > people get hurt. > > What you are effectively saying is that you simply can't and shouldn't > use this part with a i.MX6. Does MX6 not have an Reset Out ? I recall it does have one. > But why not implement the fix if the > following are true? > > - It's as you say, a common issue. > - A hardware fix may be outside the developers control or parts are > fielded. - The fix simply puts the part in a consistent state on > reboot/shutdown and seems reasonable. Because this just hides the bug ; the real solution is to make hardware which has correct reset routing, the software in this case cannot solve the problem. Best regards, Marek Vasut
diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c index ed7e0a1b..ca7caad 100644 --- a/drivers/mtd/devices/m25p80.c +++ b/drivers/mtd/devices/m25p80.c @@ -244,6 +244,14 @@ static int m25p_remove(struct spi_device *spi) return mtd_device_unregister(&flash->mtd); } +static void m25p_shutdown(struct spi_device *spi) +{ + struct m25p *flash = spi_get_drvdata(spi); + struct spi_nor *nor = &flash->spi_nor; + + if (nor->shutdown) + nor->shutdown(nor); +} static struct spi_driver m25p80_driver = { .driver = { @@ -253,6 +261,7 @@ static struct spi_driver m25p80_driver = { .id_table = spi_nor_ids, .probe = m25p_probe, .remove = m25p_remove, + .shutdown = m25p_shutdown, /* REVISIT: many of these chips have deep power-down modes, which * should clearly be entered on suspend() to minimize power use. diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c index b5ad6be..d7b5aa8 100644 --- a/drivers/mtd/spi-nor/spi-nor.c +++ b/drivers/mtd/spi-nor/spi-nor.c @@ -448,6 +448,7 @@ struct flash_info { #define SPI_NOR_DUAL_READ 0x20 /* Flash supports Dual Read */ #define SPI_NOR_QUAD_READ 0x40 /* Flash supports Quad Read */ #define USE_FSR 0x80 /* use flag status register */ +#define USE_SHUTDOWN_RESET 0x100 /* needs reset on shutdown */ }; #define INFO(_jedec_id, _ext_id, _sector_size, _n_sectors, _flags) \ @@ -536,7 +537,7 @@ const struct spi_device_id spi_nor_ids[] = { { "n25q256a", INFO(0x20ba19, 0, 64 * 1024, 512, SECT_4K) }, { "n25q512a", INFO(0x20bb20, 0, 64 * 1024, 1024, SECT_4K) }, { "n25q512ax3", INFO(0x20ba20, 0, 64 * 1024, 1024, USE_FSR) }, - { "n25q00", INFO(0x20ba21, 0, 64 * 1024, 2048, USE_FSR) }, + { "n25q00", INFO(0x20ba21, 0, 64 * 1024, 2048, USE_FSR | USE_SHUTDOWN_RESET) }, /* PMC */ { "pm25lv512", INFO(0, 0, 32 * 1024, 2, SECT_4K_PMC) }, @@ -916,6 +917,17 @@ static int spi_nor_check(struct spi_nor *nor) return 0; } +static void spi_nor_shutdown(struct spi_nor *nor) +{ + // Wait for any final commands + (void)wait_till_ready(nor); + + nor->write_reg(nor, SPINOR_OP_RSTEN, NULL, 0, 0); + nor->write_reg(nor, SPINOR_OP_RSTMEM, NULL, 0, 0); + + (void)read_sr(nor); // Force the previous async commands to complete +} + int spi_nor_scan(struct spi_nor *nor, const struct spi_device_id *id, enum read_mode mode) { @@ -1018,6 +1030,9 @@ int spi_nor_scan(struct spi_nor *nor, const struct spi_device_id *id, nor->wait_till_ready == spi_nor_wait_till_ready) nor->wait_till_ready = spi_nor_wait_till_fsr_ready; + if (info->flags & USE_SHUTDOWN_RESET) + nor->shutdown = spi_nor_shutdown; + /* prefer "small sector" erase if possible */ if (info->flags & SECT_4K) { nor->erase_opcode = SPINOR_OP_BE_4K; diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h index 9e6294f..b7cce8a 100644 --- a/include/linux/mtd/spi-nor.h +++ b/include/linux/mtd/spi-nor.h @@ -36,6 +36,10 @@ #define SPINOR_OP_RDCR 0x35 /* Read configuration register */ #define SPINOR_OP_RDFSR 0x70 /* Read flag status register */ +/* Reset opcodes */ +#define SPINOR_OP_RSTEN 0x66 /* Reset enable command */ +#define SPINOR_OP_RSTMEM 0x99 /* Reset device, follows reset enable */ + /* 4-byte address opcodes - used on Spansion and some Macronix flashes. */ #define SPINOR_OP_READ4 0x13 /* Read data bytes (low frequency) */ #define SPINOR_OP_READ4_FAST 0x0c /* Read data bytes (high frequency) */ @@ -180,6 +184,7 @@ struct spi_nor { void (*write)(struct spi_nor *nor, loff_t to, size_t len, size_t *retlen, const u_char *write_buf); int (*erase)(struct spi_nor *nor, loff_t offs); + void (*shutdown)(struct spi_nor *nor); void *priv; };