| Message ID | 1448555221-24257-1-git-send-email-nbd@openwrt.org |
|---|---|
| State | Superseded |
| Headers | show |
Hi Felix, On Thu, Nov 26, 2015 at 05:27:01PM +0100, Felix Fietkau wrote: > This reverts commit c6fc2171b249e73745c497b578b417a2946f1b2f. > > This commit is breaking read access on at least s25fl064k, but also > possibly other Spansion flash chips. This makes no sense. This patch is about Winbond flash, but you're referencing Spansion flash in the commit subject. I suspect you have either some more explaining, or some better investigation to do, because I don't understand how the commit in question could be the problem for you. > Any mtd read seems to succeed, but simply returns a zero-filled buffer. If this is the symptom you're looking at, I doubt you have the right fix. Wild guess: something similar to this patch: http://patchwork.ozlabs.org/patch/547367/ "mtd: spi-nor: wait until lock/unlock operations are ready" Brian > Signed-off-by: Felix Fietkau <nbd@openwrt.org> > --- > drivers/mtd/spi-nor/spi-nor.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c > index 4988390..8b8842e 100644 > --- a/drivers/mtd/spi-nor/spi-nor.c > +++ b/drivers/mtd/spi-nor/spi-nor.c > @@ -1194,14 +1194,13 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode) > mutex_init(&nor->lock); > > /* > - * Atmel, SST, Intel/Numonyx, and others serial NOR tend to power up > - * with the software protection bits set > + * Atmel, SST and Intel/Numonyx serial nor tend to power > + * up with the software protection bits set > */ > > if (JEDEC_MFR(info) == SNOR_MFR_ATMEL || > JEDEC_MFR(info) == SNOR_MFR_INTEL || > - JEDEC_MFR(info) == SNOR_MFR_SST || > - JEDEC_MFR(info) == SNOR_MFR_WINBOND) { > + JEDEC_MFR(info) == SNOR_MFR_SST) { > write_enable(nor); > write_sr(nor, 0); > } > -- > 2.2.2 >
On 2015-11-27 23:27, Brian Norris wrote: > Hi Felix, > > On Thu, Nov 26, 2015 at 05:27:01PM +0100, Felix Fietkau wrote: >> This reverts commit c6fc2171b249e73745c497b578b417a2946f1b2f. >> >> This commit is breaking read access on at least s25fl064k, but also >> possibly other Spansion flash chips. > > This makes no sense. This patch is about Winbond flash, but you're > referencing Spansion flash in the commit subject. I suspect you have > either some more explaining, or some better investigation to do, because > I don't understand how the commit in question could be the problem for > you. Take a look at these Spansion flash ids: { "s25fl004k", INFO(0xef4013, 0, 64 * 1024, 8, SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) }, { "s25fl008k", INFO(0xef4014, 0, 64 * 1024, 16, SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) }, { "s25fl016k", INFO(0xef4015, 0, 64 * 1024, 32, SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) }, { "s25fl064k", INFO(0xef4017, 0, 64 * 1024, 128, SECT_4K) }, Note how they start with 0xef (SNOR_MFR_WINBOND). >> Any mtd read seems to succeed, but simply returns a zero-filled buffer. > > If this is the symptom you're looking at, I doubt you have the right > fix. Wild guess: something similar to this patch: > > http://patchwork.ozlabs.org/patch/547367/ > "mtd: spi-nor: wait until lock/unlock operations are ready" When I have access to a similar device again, I can give this fix a try. If that doesn't work, then maybe we need to add an extra check to exclude Spansion flashes from the codepath that checks for SNOR_MFR_WINBOND. - Felix
On 26 November 2015 at 13:27, Felix Fietkau <nbd@openwrt.org> wrote: > This reverts commit c6fc2171b249e73745c497b578b417a2946f1b2f. > > This commit is breaking read access on at least s25fl064k, but also > possibly other Spansion flash chips. > > Any mtd read seems to succeed, but simply returns a zero-filled buffer. > > Signed-off-by: Felix Fietkau <nbd@openwrt.org> > --- > drivers/mtd/spi-nor/spi-nor.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c > index 4988390..8b8842e 100644 > --- a/drivers/mtd/spi-nor/spi-nor.c > +++ b/drivers/mtd/spi-nor/spi-nor.c > @@ -1194,14 +1194,13 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode) > mutex_init(&nor->lock); > > /* > - * Atmel, SST, Intel/Numonyx, and others serial NOR tend to power up > - * with the software protection bits set > + * Atmel, SST and Intel/Numonyx serial nor tend to power > + * up with the software protection bits set > */ > > if (JEDEC_MFR(info) == SNOR_MFR_ATMEL || > JEDEC_MFR(info) == SNOR_MFR_INTEL || > - JEDEC_MFR(info) == SNOR_MFR_SST || > - JEDEC_MFR(info) == SNOR_MFR_WINBOND) { > + JEDEC_MFR(info) == SNOR_MFR_SST) { > write_enable(nor); > write_sr(nor, 0); > } As Brian mentioned, this looks definitely fishy. IIUC, the above statement is trying to unlock flashes that power-up as locked. But you say your flash is still locked after booting? And moreover, removing the unlock quirk fixes it? I think a more complete description of your problem might help us fix this the right way. Perhaps the above line clearing the status register is wrong?
On 2015-11-28 22:56, Ezequiel Garcia wrote: > On 26 November 2015 at 13:27, Felix Fietkau <nbd@openwrt.org> wrote: >> This reverts commit c6fc2171b249e73745c497b578b417a2946f1b2f. >> >> This commit is breaking read access on at least s25fl064k, but also >> possibly other Spansion flash chips. >> >> Any mtd read seems to succeed, but simply returns a zero-filled buffer. >> >> Signed-off-by: Felix Fietkau <nbd@openwrt.org> >> --- >> drivers/mtd/spi-nor/spi-nor.c | 7 +++---- >> 1 file changed, 3 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c >> index 4988390..8b8842e 100644 >> --- a/drivers/mtd/spi-nor/spi-nor.c >> +++ b/drivers/mtd/spi-nor/spi-nor.c >> @@ -1194,14 +1194,13 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode) >> mutex_init(&nor->lock); >> >> /* >> - * Atmel, SST, Intel/Numonyx, and others serial NOR tend to power up >> - * with the software protection bits set >> + * Atmel, SST and Intel/Numonyx serial nor tend to power >> + * up with the software protection bits set >> */ >> >> if (JEDEC_MFR(info) == SNOR_MFR_ATMEL || >> JEDEC_MFR(info) == SNOR_MFR_INTEL || >> - JEDEC_MFR(info) == SNOR_MFR_SST || >> - JEDEC_MFR(info) == SNOR_MFR_WINBOND) { >> + JEDEC_MFR(info) == SNOR_MFR_SST) { >> write_enable(nor); >> write_sr(nor, 0); >> } > > As Brian mentioned, this looks definitely fishy. > > IIUC, the above statement is trying to unlock flashes that power-up as > locked. But you say your flash is still locked after booting? And > moreover, removing the unlock quirk fixes it? The flash isn't locked when the system boots, reads and writes work just fine. > I think a more complete description of your problem might help us fix > this the right way. Perhaps the above line clearing the status > register is wrong? Without the write_enable/write_sr, the flash is readable and writable. With it, any reads return only null-bytes as data. What other info about the problem do you need? I think it probably works just fine for Winbond flashes, but unfortunately this also affects some Spansion flash chips with IDs that start with 0xef (SNOR_MFR_WINBOND). - Felix
On 28 November 2015 at 19:12, Felix Fietkau <nbd@openwrt.org> wrote: > On 2015-11-28 22:56, Ezequiel Garcia wrote: >> On 26 November 2015 at 13:27, Felix Fietkau <nbd@openwrt.org> wrote: >>> This reverts commit c6fc2171b249e73745c497b578b417a2946f1b2f. >>> >>> This commit is breaking read access on at least s25fl064k, but also >>> possibly other Spansion flash chips. >>> >>> Any mtd read seems to succeed, but simply returns a zero-filled buffer. >>> >>> Signed-off-by: Felix Fietkau <nbd@openwrt.org> >>> --- >>> drivers/mtd/spi-nor/spi-nor.c | 7 +++---- >>> 1 file changed, 3 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c >>> index 4988390..8b8842e 100644 >>> --- a/drivers/mtd/spi-nor/spi-nor.c >>> +++ b/drivers/mtd/spi-nor/spi-nor.c >>> @@ -1194,14 +1194,13 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode) >>> mutex_init(&nor->lock); >>> >>> /* >>> - * Atmel, SST, Intel/Numonyx, and others serial NOR tend to power up >>> - * with the software protection bits set >>> + * Atmel, SST and Intel/Numonyx serial nor tend to power >>> + * up with the software protection bits set >>> */ >>> >>> if (JEDEC_MFR(info) == SNOR_MFR_ATMEL || >>> JEDEC_MFR(info) == SNOR_MFR_INTEL || >>> - JEDEC_MFR(info) == SNOR_MFR_SST || >>> - JEDEC_MFR(info) == SNOR_MFR_WINBOND) { >>> + JEDEC_MFR(info) == SNOR_MFR_SST) { >>> write_enable(nor); >>> write_sr(nor, 0); >>> } >> >> As Brian mentioned, this looks definitely fishy. >> >> IIUC, the above statement is trying to unlock flashes that power-up as >> locked. But you say your flash is still locked after booting? And >> moreover, removing the unlock quirk fixes it? > The flash isn't locked when the system boots, reads and writes work just > fine. > >> I think a more complete description of your problem might help us fix >> this the right way. Perhaps the above line clearing the status >> register is wrong? > Without the write_enable/write_sr, the flash is readable and writable. > With it, any reads return only null-bytes as data. What other info about > the problem do you need? > Well, printing the status register right before a read (i.e. when the problem shows) might help understand what's going on.
On 2015-11-30 15:50, Ezequiel Garcia wrote: > On 28 November 2015 at 19:12, Felix Fietkau <nbd@openwrt.org> wrote: >> On 2015-11-28 22:56, Ezequiel Garcia wrote: >>> On 26 November 2015 at 13:27, Felix Fietkau <nbd@openwrt.org> wrote: >>>> This reverts commit c6fc2171b249e73745c497b578b417a2946f1b2f. >>>> >>>> This commit is breaking read access on at least s25fl064k, but also >>>> possibly other Spansion flash chips. >>>> >>>> Any mtd read seems to succeed, but simply returns a zero-filled buffer. >>>> >>>> Signed-off-by: Felix Fietkau <nbd@openwrt.org> >>>> --- >>>> drivers/mtd/spi-nor/spi-nor.c | 7 +++---- >>>> 1 file changed, 3 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c >>>> index 4988390..8b8842e 100644 >>>> --- a/drivers/mtd/spi-nor/spi-nor.c >>>> +++ b/drivers/mtd/spi-nor/spi-nor.c >>>> @@ -1194,14 +1194,13 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode) >>>> mutex_init(&nor->lock); >>>> >>>> /* >>>> - * Atmel, SST, Intel/Numonyx, and others serial NOR tend to power up >>>> - * with the software protection bits set >>>> + * Atmel, SST and Intel/Numonyx serial nor tend to power >>>> + * up with the software protection bits set >>>> */ >>>> >>>> if (JEDEC_MFR(info) == SNOR_MFR_ATMEL || >>>> JEDEC_MFR(info) == SNOR_MFR_INTEL || >>>> - JEDEC_MFR(info) == SNOR_MFR_SST || >>>> - JEDEC_MFR(info) == SNOR_MFR_WINBOND) { >>>> + JEDEC_MFR(info) == SNOR_MFR_SST) { >>>> write_enable(nor); >>>> write_sr(nor, 0); >>>> } >>> >>> As Brian mentioned, this looks definitely fishy. >>> >>> IIUC, the above statement is trying to unlock flashes that power-up as >>> locked. But you say your flash is still locked after booting? And >>> moreover, removing the unlock quirk fixes it? >> The flash isn't locked when the system boots, reads and writes work just >> fine. >> >>> I think a more complete description of your problem might help us fix >>> this the right way. Perhaps the above line clearing the status >>> register is wrong? >> Without the write_enable/write_sr, the flash is readable and writable. >> With it, any reads return only null-bytes as data. What other info about >> the problem do you need? >> > > Well, printing the status register right before a read (i.e. when the > problem shows) might help understand what's going on. Will do that once I have access to suitable test hardware again. - Felix
Hi, On Sat, Nov 28, 2015 at 09:59:43AM +0100, Felix Fietkau wrote: > On 2015-11-27 23:27, Brian Norris wrote: > > On Thu, Nov 26, 2015 at 05:27:01PM +0100, Felix Fietkau wrote: > >> This reverts commit c6fc2171b249e73745c497b578b417a2946f1b2f. > >> > >> This commit is breaking read access on at least s25fl064k, but also > >> possibly other Spansion flash chips. > > > > This makes no sense. This patch is about Winbond flash, but you're > > referencing Spansion flash in the commit subject. I suspect you have > > either some more explaining, or some better investigation to do, because > > I don't understand how the commit in question could be the problem for > > you. > Take a look at these Spansion flash ids: > > { "s25fl004k", INFO(0xef4013, 0, 64 * 1024, 8, SECT_4K | > SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) }, > { "s25fl008k", INFO(0xef4014, 0, 64 * 1024, 16, SECT_4K | > SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) }, > { "s25fl016k", INFO(0xef4015, 0, 64 * 1024, 32, SECT_4K | > SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) }, > { "s25fl064k", INFO(0xef4017, 0, 64 * 1024, 128, SECT_4K) }, > > Note how they start with 0xef (SNOR_MFR_WINBOND). Ugh, that really sucks :( Maybe we should do the revert [*] now, and fix it for the next release. Maybe we can add something to the table entry to (optionally) specify the manufacturer, so we can specify that even if these look like they're Winbond, they're actually Spansion. However, it seems like the lock/unlock features (e.g., top/bottom; 4K sector ranges) supported vary significantly across not only vendors, but generation. So maybe we just need some more opt-in flags in the table, so we don't have to try to make blanket statements, like "all Winbond flash support flash locking in SR." Brian [*] I have a comment for this patch. I'll reply to the top-level.
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c index 4988390..8b8842e 100644 --- a/drivers/mtd/spi-nor/spi-nor.c +++ b/drivers/mtd/spi-nor/spi-nor.c @@ -1194,14 +1194,13 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode) mutex_init(&nor->lock); /* - * Atmel, SST, Intel/Numonyx, and others serial NOR tend to power up - * with the software protection bits set + * Atmel, SST and Intel/Numonyx serial nor tend to power + * up with the software protection bits set */ if (JEDEC_MFR(info) == SNOR_MFR_ATMEL || JEDEC_MFR(info) == SNOR_MFR_INTEL || - JEDEC_MFR(info) == SNOR_MFR_SST || - JEDEC_MFR(info) == SNOR_MFR_WINBOND) { + JEDEC_MFR(info) == SNOR_MFR_SST) { write_enable(nor); write_sr(nor, 0); }
This reverts commit c6fc2171b249e73745c497b578b417a2946f1b2f. This commit is breaking read access on at least s25fl064k, but also possibly other Spansion flash chips. Any mtd read seems to succeed, but simply returns a zero-filled buffer. Signed-off-by: Felix Fietkau <nbd@openwrt.org> --- drivers/mtd/spi-nor/spi-nor.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)