| Message ID | 20181017144424.17554-1-boris.brezillon@bootlin.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2] mtd: spi-nor: Make SFDP-based 4B_OPCODE support detection works correctly | expand |
In the subject: s/Make/Make sure/ On Wed, 17 Oct 2018 16:44:23 +0200 Boris Brezillon <boris.brezillon@bootlin.com> wrote: > Some flash_info entries have the SPI_NOR_4B_OPCODES to let the core ^ flag set > know that the flash supports 4B opcode. While this solution works fine > for id-based caps detection, it doesn't work that well when relying on > SFDP-based caps detection. Let's add an SNOR_F_4B_OPCODES flag so that > spi_nor_parse_bfpt() can add it when the BFPT_DWORD1_ADDRESS_BYTES > field is set to BFPT_DWORD1_ADDRESS_BYTES_4_ONLY. > > Reported-by: Alexandre Belloni <alexandre.belloni@bootlin.com> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> > --- > drivers/mtd/spi-nor/spi-nor.c | 11 ++++++++--- > include/linux/mtd/spi-nor.h | 1 + > 2 files changed, 9 insertions(+), 3 deletions(-) > > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c > index 9407ca5f9443..85e57e9ea1b5 100644 > --- a/drivers/mtd/spi-nor/spi-nor.c > +++ b/drivers/mtd/spi-nor/spi-nor.c > @@ -2643,6 +2643,7 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor, > break; > > case BFPT_DWORD1_ADDRESS_BYTES_4_ONLY: > + nor->flags |= SNOR_F_4B_OPCODES; > nor->addr_width = 4; > break; > > @@ -3552,7 +3553,7 @@ static int spi_nor_init(struct spi_nor *nor) > > if ((nor->addr_width == 4) && > (JEDEC_MFR(nor->info) != SNOR_MFR_SPANSION) && > - !(nor->info->flags & SPI_NOR_4B_OPCODES)) { > + !(nor->flags & SNOR_F_4B_OPCODES)) { > /* > * If the RESET# pin isn't hooked up properly, or the system > * otherwise doesn't perform a reset command in the boot > @@ -3586,7 +3587,7 @@ void spi_nor_restore(struct spi_nor *nor) > /* restore the addressing mode */ > if ((nor->addr_width == 4) && > (JEDEC_MFR(nor->info) != SNOR_MFR_SPANSION) && > - !(nor->info->flags & SPI_NOR_4B_OPCODES) && > + !(nor->flags & SNOR_F_4B_OPCODES) && > (nor->flags & SNOR_F_BROKEN_RESET)) > set_4byte(nor, nor->info, 0); > } > @@ -3744,11 +3745,15 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, > nor->addr_width = 4; > if (JEDEC_MFR(info) == SNOR_MFR_SPANSION || > info->flags & SPI_NOR_4B_OPCODES) > - spi_nor_set_4byte_opcodes(nor, info); > + nor->flags |= SNOR_F_4B_OPCODES; > } else { > nor->addr_width = 3; > } > > + if (info->addr_width == 4 && We should check nor->addr_width not info->addr_width. > + nor->flags & SNOR_F_4B_OPCODES) > + spi_nor_set_4byte_opcodes(nor, info); > + > if (nor->addr_width > SPI_NOR_MAX_ADDR_WIDTH) { > dev_err(dev, "address width is too large: %u\n", > nor->addr_width); > diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h > index 7f0c7303575e..4ffb165f4f85 100644 > --- a/include/linux/mtd/spi-nor.h > +++ b/include/linux/mtd/spi-nor.h > @@ -236,6 +236,7 @@ enum spi_nor_option_flags { > SNOR_F_READY_XSR_RDY = BIT(4), > SNOR_F_USE_CLSR = BIT(5), > SNOR_F_BROKEN_RESET = BIT(6), > + SNOR_F_4B_OPCODES = BIT(7) Missing comma at the end of the line. I'll send a v2 soon.
Hi Boris, looks good, just a small remark below: Le 17/10/2018 à 16:44, Boris Brezillon a écrit : > Some flash_info entries have the SPI_NOR_4B_OPCODES to let the core > know that the flash supports 4B opcode. While this solution works fine > for id-based caps detection, it doesn't work that well when relying on > SFDP-based caps detection. Let's add an SNOR_F_4B_OPCODES flag so that > spi_nor_parse_bfpt() can add it when the BFPT_DWORD1_ADDRESS_BYTES > field is set to BFPT_DWORD1_ADDRESS_BYTES_4_ONLY. > > Reported-by: Alexandre Belloni <alexandre.belloni@bootlin.com> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> > --- > drivers/mtd/spi-nor/spi-nor.c | 11 ++++++++--- > include/linux/mtd/spi-nor.h | 1 + > 2 files changed, 9 insertions(+), 3 deletions(-) > > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c > index 9407ca5f9443..85e57e9ea1b5 100644 > --- a/drivers/mtd/spi-nor/spi-nor.c > +++ b/drivers/mtd/spi-nor/spi-nor.c > @@ -2643,6 +2643,7 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor, > break; > > case BFPT_DWORD1_ADDRESS_BYTES_4_ONLY: > + nor->flags |= SNOR_F_4B_OPCODES; if spi_nor_parse_sdfp() fails, there is a kind of roll-back operation done in spi_nor_init_params() to set the struct spi_nor *nor back to its previous state. if (spi_nor_parse_sfdp(nor, &sfdp_params)) { nor->addr_width = 0; nor->mtd.erasesize = 0; } else { [...] maybe "nor->flags &= ~SNOR_F_4B_OPCODES;" should be added there. If this roll-back block grows too much, maybe we could introduce a void spi_nor_roll_back_sfdp(struct spi_nor *nor) function. Also it would make the roll back operation more explicit. Best regards, Cyrille > nor->addr_width = 4; > break; > > @@ -3552,7 +3553,7 @@ static int spi_nor_init(struct spi_nor *nor) > > if ((nor->addr_width == 4) && > (JEDEC_MFR(nor->info) != SNOR_MFR_SPANSION) && > - !(nor->info->flags & SPI_NOR_4B_OPCODES)) { > + !(nor->flags & SNOR_F_4B_OPCODES)) { > /* > * If the RESET# pin isn't hooked up properly, or the system > * otherwise doesn't perform a reset command in the boot > @@ -3586,7 +3587,7 @@ void spi_nor_restore(struct spi_nor *nor) > /* restore the addressing mode */ > if ((nor->addr_width == 4) && > (JEDEC_MFR(nor->info) != SNOR_MFR_SPANSION) && > - !(nor->info->flags & SPI_NOR_4B_OPCODES) && > + !(nor->flags & SNOR_F_4B_OPCODES) && > (nor->flags & SNOR_F_BROKEN_RESET)) > set_4byte(nor, nor->info, 0); > } > @@ -3744,11 +3745,15 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, > nor->addr_width = 4; > if (JEDEC_MFR(info) == SNOR_MFR_SPANSION || > info->flags & SPI_NOR_4B_OPCODES) > - spi_nor_set_4byte_opcodes(nor, info); > + nor->flags |= SNOR_F_4B_OPCODES; > } else { > nor->addr_width = 3; > } > > + if (info->addr_width == 4 && > + nor->flags & SNOR_F_4B_OPCODES) > + spi_nor_set_4byte_opcodes(nor, info); > + > if (nor->addr_width > SPI_NOR_MAX_ADDR_WIDTH) { > dev_err(dev, "address width is too large: %u\n", > nor->addr_width); > diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h > index 7f0c7303575e..4ffb165f4f85 100644 > --- a/include/linux/mtd/spi-nor.h > +++ b/include/linux/mtd/spi-nor.h > @@ -236,6 +236,7 @@ enum spi_nor_option_flags { > SNOR_F_READY_XSR_RDY = BIT(4), > SNOR_F_USE_CLSR = BIT(5), > SNOR_F_BROKEN_RESET = BIT(6), > + SNOR_F_4B_OPCODES = BIT(7) > }; > > /** >
On Fri, 19 Oct 2018 09:50:31 +0200 Cyrille Pitchen - M19942 <cyrille.pitchen@microchip.com> wrote: > Hi Boris, > > looks good, just a small remark below: > > Le 17/10/2018 à 16:44, Boris Brezillon a écrit : > > Some flash_info entries have the SPI_NOR_4B_OPCODES to let the core > > know that the flash supports 4B opcode. While this solution works fine > > for id-based caps detection, it doesn't work that well when relying on > > SFDP-based caps detection. Let's add an SNOR_F_4B_OPCODES flag so that > > spi_nor_parse_bfpt() can add it when the BFPT_DWORD1_ADDRESS_BYTES > > field is set to BFPT_DWORD1_ADDRESS_BYTES_4_ONLY. > > > > Reported-by: Alexandre Belloni <alexandre.belloni@bootlin.com> > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> > > --- > > drivers/mtd/spi-nor/spi-nor.c | 11 ++++++++--- > > include/linux/mtd/spi-nor.h | 1 + > > 2 files changed, 9 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c > > index 9407ca5f9443..85e57e9ea1b5 100644 > > --- a/drivers/mtd/spi-nor/spi-nor.c > > +++ b/drivers/mtd/spi-nor/spi-nor.c > > @@ -2643,6 +2643,7 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor, > > break; > > > > case BFPT_DWORD1_ADDRESS_BYTES_4_ONLY: > > + nor->flags |= SNOR_F_4B_OPCODES; > > if spi_nor_parse_sdfp() fails, there is a kind of roll-back operation done > in spi_nor_init_params() to set the struct spi_nor *nor back to its previous > state. > > if (spi_nor_parse_sfdp(nor, &sfdp_params)) { > nor->addr_width = 0; > nor->mtd.erasesize = 0; > } else { > [...] > > maybe "nor->flags &= ~SNOR_F_4B_OPCODES;" should be added there. Actually, it should be if (!(info->flags & SPI_NOR_4B_OPCODES)) nor->flags &= ~SNOR_F_4B_OPCODES; but yes, this is missing. I'll fix that in v3. > If this roll-back block grows too much, maybe we could introduce a > void spi_nor_roll_back_sfdp(struct spi_nor *nor) function. > Also it would make the roll back operation more explicit. I'm wondering why we revert everything when a single bit is bit reported as inconsistent in the SFDP table? I mean, it's not unusual for NOR vendors to make mistake, and we should probably allow vendor/chip specific code to fix the SFDP table at runtime instead of discarding all the useful information we might have extracted. Note that we recently introduced such a ->fixup() hook for the ONFI param page in the raw NAND framework [1]. [1]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/mtd/nand/raw?h=v4.19-rc8&id=00ce4e039ad5bded462931606c3063ff691964b7
Le 19/10/2018 à 10:05, Boris Brezillon a écrit : > On Fri, 19 Oct 2018 09:50:31 +0200 > Cyrille Pitchen - M19942 <cyrille.pitchen@microchip.com> wrote: > >> Hi Boris, >> >> looks good, just a small remark below: >> >> Le 17/10/2018 à 16:44, Boris Brezillon a écrit : >>> Some flash_info entries have the SPI_NOR_4B_OPCODES to let the core >>> know that the flash supports 4B opcode. While this solution works fine >>> for id-based caps detection, it doesn't work that well when relying on >>> SFDP-based caps detection. Let's add an SNOR_F_4B_OPCODES flag so that >>> spi_nor_parse_bfpt() can add it when the BFPT_DWORD1_ADDRESS_BYTES >>> field is set to BFPT_DWORD1_ADDRESS_BYTES_4_ONLY. >>> >>> Reported-by: Alexandre Belloni <alexandre.belloni@bootlin.com> >>> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> >>> --- >>> drivers/mtd/spi-nor/spi-nor.c | 11 ++++++++--- >>> include/linux/mtd/spi-nor.h | 1 + >>> 2 files changed, 9 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c >>> index 9407ca5f9443..85e57e9ea1b5 100644 >>> --- a/drivers/mtd/spi-nor/spi-nor.c >>> +++ b/drivers/mtd/spi-nor/spi-nor.c >>> @@ -2643,6 +2643,7 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor, >>> break; >>> >>> case BFPT_DWORD1_ADDRESS_BYTES_4_ONLY: >>> + nor->flags |= SNOR_F_4B_OPCODES; >> >> if spi_nor_parse_sdfp() fails, there is a kind of roll-back operation done >> in spi_nor_init_params() to set the struct spi_nor *nor back to its previous >> state. >> >> if (spi_nor_parse_sfdp(nor, &sfdp_params)) { >> nor->addr_width = 0; >> nor->mtd.erasesize = 0; >> } else { >> [...] >> >> maybe "nor->flags &= ~SNOR_F_4B_OPCODES;" should be added there. > > Actually, it should be > > if (!(info->flags & SPI_NOR_4B_OPCODES)) > nor->flags &= ~SNOR_F_4B_OPCODES; > > but yes, this is missing. I'll fix that in v3. > >> If this roll-back block grows too much, maybe we could introduce a >> void spi_nor_roll_back_sfdp(struct spi_nor *nor) function. >> Also it would make the roll back operation more explicit. > > I'm wondering why we revert everything when a single bit is bit > reported as inconsistent in the SFDP table? I mean, it's not unusual > for NOR vendors to make mistake, and we should probably allow > vendor/chip specific code to fix the SFDP table at runtime instead of > discarding all the useful information we might have extracted. > I tend to agree with you: it would have been better to keep as much settings as possible. The correct settings of course. The question was how to decide which settings are correct and which are wrong. Some might be wrong even if they're semantically correct. However since we could now use a new fixup mechanism, we can try to statically fix known issues in SFDP table. For unknown issue dynamically found during the parsing, maybe we should keep the roll-back mechanism. When the fix-up is introduced later for the faulty memory part, we can keep the relevant settings and discard wrong settings. Without an explicit fixup, I guess it's hard to know which settings could be kept, so maybe a roll-back operation is more straight forward solution. We just have to cross the fingers and pray for faulty SFDP tables not to be so likely. You know how much I believe in SPI NOR memory manufacturer ;) > Note that we recently introduced such a ->fixup() hook for the ONFI > param page in the raw NAND framework [1]. > > [1]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/mtd/nand/raw?h=v4.19-rc8&id=00ce4e039ad5bded462931606c3063ff691964b7 > > ______________________________________________________ > Linux MTD discussion mailing list > http://lists.infradead.org/mailman/listinfo/linux-mtd/ >
On Fri, 19 Oct 2018 09:50:31 +0200 Cyrille Pitchen - M19942 <cyrille.pitchen@microchip.com> wrote: > Hi Boris, > > looks good, just a small remark below: > > Le 17/10/2018 à 16:44, Boris Brezillon a écrit : > > Some flash_info entries have the SPI_NOR_4B_OPCODES to let the core > > know that the flash supports 4B opcode. While this solution works fine > > for id-based caps detection, it doesn't work that well when relying on > > SFDP-based caps detection. Let's add an SNOR_F_4B_OPCODES flag so that > > spi_nor_parse_bfpt() can add it when the BFPT_DWORD1_ADDRESS_BYTES > > field is set to BFPT_DWORD1_ADDRESS_BYTES_4_ONLY. > > > > Reported-by: Alexandre Belloni <alexandre.belloni@bootlin.com> > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> > > --- > > drivers/mtd/spi-nor/spi-nor.c | 11 ++++++++--- > > include/linux/mtd/spi-nor.h | 1 + > > 2 files changed, 9 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c > > index 9407ca5f9443..85e57e9ea1b5 100644 > > --- a/drivers/mtd/spi-nor/spi-nor.c > > +++ b/drivers/mtd/spi-nor/spi-nor.c > > @@ -2643,6 +2643,7 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor, > > break; > > > > case BFPT_DWORD1_ADDRESS_BYTES_4_ONLY: > > + nor->flags |= SNOR_F_4B_OPCODES; > > if spi_nor_parse_sdfp() fails, there is a kind of roll-back operation done > in spi_nor_init_params() to set the struct spi_nor *nor back to its previous > state. > > if (spi_nor_parse_sfdp(nor, &sfdp_params)) { > nor->addr_width = 0; Looks like the ->addr_width=0; as been lost when adding support for non-uniform erase size. > nor->mtd.erasesize = 0; > } else { > [...] > > maybe "nor->flags &= ~SNOR_F_4B_OPCODES;" should be added there. > If this roll-back block grows too much, maybe we could introduce a > void spi_nor_roll_back_sfdp(struct spi_nor *nor) function. > Also it would make the roll back operation more explicit. > > Best regards, > > Cyrille > > > nor->addr_width = 4; > > break; > > > > @@ -3552,7 +3553,7 @@ static int spi_nor_init(struct spi_nor *nor) > > > > if ((nor->addr_width == 4) && > > (JEDEC_MFR(nor->info) != SNOR_MFR_SPANSION) && > > - !(nor->info->flags & SPI_NOR_4B_OPCODES)) { > > + !(nor->flags & SNOR_F_4B_OPCODES)) { > > /* > > * If the RESET# pin isn't hooked up properly, or the system > > * otherwise doesn't perform a reset command in the boot > > @@ -3586,7 +3587,7 @@ void spi_nor_restore(struct spi_nor *nor) > > /* restore the addressing mode */ > > if ((nor->addr_width == 4) && > > (JEDEC_MFR(nor->info) != SNOR_MFR_SPANSION) && > > - !(nor->info->flags & SPI_NOR_4B_OPCODES) && > > + !(nor->flags & SNOR_F_4B_OPCODES) && > > (nor->flags & SNOR_F_BROKEN_RESET)) > > set_4byte(nor, nor->info, 0); > > } > > @@ -3744,11 +3745,15 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, > > nor->addr_width = 4; > > if (JEDEC_MFR(info) == SNOR_MFR_SPANSION || > > info->flags & SPI_NOR_4B_OPCODES) > > - spi_nor_set_4byte_opcodes(nor, info); > > + nor->flags |= SNOR_F_4B_OPCODES; > > } else { > > nor->addr_width = 3; > > } > > > > + if (info->addr_width == 4 && > > + nor->flags & SNOR_F_4B_OPCODES) > > + spi_nor_set_4byte_opcodes(nor, info); > > + > > if (nor->addr_width > SPI_NOR_MAX_ADDR_WIDTH) { > > dev_err(dev, "address width is too large: %u\n", > > nor->addr_width); > > diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h > > index 7f0c7303575e..4ffb165f4f85 100644 > > --- a/include/linux/mtd/spi-nor.h > > +++ b/include/linux/mtd/spi-nor.h > > @@ -236,6 +236,7 @@ enum spi_nor_option_flags { > > SNOR_F_READY_XSR_RDY = BIT(4), > > SNOR_F_USE_CLSR = BIT(5), > > SNOR_F_BROKEN_RESET = BIT(6), > > + SNOR_F_4B_OPCODES = BIT(7) > > }; > > > > /** > >
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c index 9407ca5f9443..85e57e9ea1b5 100644 --- a/drivers/mtd/spi-nor/spi-nor.c +++ b/drivers/mtd/spi-nor/spi-nor.c @@ -2643,6 +2643,7 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor, break; case BFPT_DWORD1_ADDRESS_BYTES_4_ONLY: + nor->flags |= SNOR_F_4B_OPCODES; nor->addr_width = 4; break; @@ -3552,7 +3553,7 @@ static int spi_nor_init(struct spi_nor *nor) if ((nor->addr_width == 4) && (JEDEC_MFR(nor->info) != SNOR_MFR_SPANSION) && - !(nor->info->flags & SPI_NOR_4B_OPCODES)) { + !(nor->flags & SNOR_F_4B_OPCODES)) { /* * If the RESET# pin isn't hooked up properly, or the system * otherwise doesn't perform a reset command in the boot @@ -3586,7 +3587,7 @@ void spi_nor_restore(struct spi_nor *nor) /* restore the addressing mode */ if ((nor->addr_width == 4) && (JEDEC_MFR(nor->info) != SNOR_MFR_SPANSION) && - !(nor->info->flags & SPI_NOR_4B_OPCODES) && + !(nor->flags & SNOR_F_4B_OPCODES) && (nor->flags & SNOR_F_BROKEN_RESET)) set_4byte(nor, nor->info, 0); } @@ -3744,11 +3745,15 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, nor->addr_width = 4; if (JEDEC_MFR(info) == SNOR_MFR_SPANSION || info->flags & SPI_NOR_4B_OPCODES) - spi_nor_set_4byte_opcodes(nor, info); + nor->flags |= SNOR_F_4B_OPCODES; } else { nor->addr_width = 3; } + if (info->addr_width == 4 && + nor->flags & SNOR_F_4B_OPCODES) + spi_nor_set_4byte_opcodes(nor, info); + if (nor->addr_width > SPI_NOR_MAX_ADDR_WIDTH) { dev_err(dev, "address width is too large: %u\n", nor->addr_width); diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h index 7f0c7303575e..4ffb165f4f85 100644 --- a/include/linux/mtd/spi-nor.h +++ b/include/linux/mtd/spi-nor.h @@ -236,6 +236,7 @@ enum spi_nor_option_flags { SNOR_F_READY_XSR_RDY = BIT(4), SNOR_F_USE_CLSR = BIT(5), SNOR_F_BROKEN_RESET = BIT(6), + SNOR_F_4B_OPCODES = BIT(7) }; /**
Some flash_info entries have the SPI_NOR_4B_OPCODES to let the core know that the flash supports 4B opcode. While this solution works fine for id-based caps detection, it doesn't work that well when relying on SFDP-based caps detection. Let's add an SNOR_F_4B_OPCODES flag so that spi_nor_parse_bfpt() can add it when the BFPT_DWORD1_ADDRESS_BYTES field is set to BFPT_DWORD1_ADDRESS_BYTES_4_ONLY. Reported-by: Alexandre Belloni <alexandre.belloni@bootlin.com> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> --- drivers/mtd/spi-nor/spi-nor.c | 11 ++++++++--- include/linux/mtd/spi-nor.h | 1 + 2 files changed, 9 insertions(+), 3 deletions(-)