| Message ID | 20181129144143.26079-2-boris.brezillon@bootlin.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | mtd: spi-nor: mx25l25635f: Use 4B opcodes | expand |
Hi, Boris, On 11/29/2018 04:41 PM, Boris Brezillon wrote: > Some flash_info entries have the SPI_NOR_4B_OPCODES flag set 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 the SFDP parsing code can set it when appropriate. > > Reported-by: Alexandre Belloni <alexandre.belloni@bootlin.com> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> > Tested-by: Alexandre Belloni <alexandre.belloni@bootlin.com> > Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com> > --- > Changes in v4: > - Set SNOR_F_4B_OPCODES flag outside of the if (mtd->size > 0x1000000) > block > - Do not set SNOR_F_4B_OPCODES when BFPT_DWORD1_ADDRESS_BYTES_4_ONLY, > because 4byte only does not imply 4B opcodes are supported and you got rid of the superfluous check "(JEDEC_MFR(nor->info) != SNOR_MFR_SPANSION)", which is good, thanks. And I guess you should have dropped my rb tag, because I haven't reviewed v4 yet. No worries, just saying ... > > Changes in v3: > - Clear SNOR_F_4B_OPCODES flag when SFDP fails > - Add Alexandre R-b > > Changes in v2: > - Fix the commit message > - Fix the ->addr_width check > - Add a comma at the end of the SNOR_F_4B_OPCODES definition > --- > drivers/mtd/spi-nor/spi-nor.c | 22 ++++++++++++---------- > include/linux/mtd/spi-nor.h | 1 + > 2 files changed, 13 insertions(+), 10 deletions(-) > > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c > index c1d9c2e50bee..98b8d9a778aa 100644 > --- a/drivers/mtd/spi-nor/spi-nor.c > +++ b/drivers/mtd/spi-nor/spi-nor.c > @@ -3311,6 +3311,7 @@ static int spi_nor_init_params(struct spi_nor *nor, > > if (spi_nor_parse_sfdp(nor, &sfdp_params)) { > nor->addr_width = 0; > + nor->flags &= ~SNOR_F_4B_OPCODES; > /* restore previous erase map */ > memcpy(&nor->erase_map, &prev_map, > sizeof(nor->erase_map)); > @@ -3561,9 +3562,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)) { > + if (nor->addr_width == 4 && !(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 > @@ -3595,10 +3594,8 @@ static void spi_nor_resume(struct mtd_info *mtd) > 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_BROKEN_RESET)) > + if (nor->addr_width == 4 && !(nor->flags & SNOR_F_4B_OPCODES) && > + nor->flags & SNOR_F_BROKEN_RESET) > set_4byte(nor, false); > } > EXPORT_SYMBOL_GPL(spi_nor_restore); > @@ -3750,6 +3747,10 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, > if (info->flags & SPI_NOR_NO_FR) > params.hwcaps.mask &= ~SNOR_HWCAPS_READ_FAST; > > + if (info->flags & SPI_NOR_4B_OPCODES || > + (JEDEC_MFR(info) == SNOR_MFR_SPANSION && mtd->size > SZ_16M)) > + nor->flags |= SNOR_F_4B_OPCODES; > + you are potentially overwriting the SNOR_F_4B_OPCODES that may be set in SFDP. I suggest to set SNOR_F_4B_OPCODES flag inside of the "if (mtd->size > 0x1000000)" block. > /* > * Configure the SPI memory: > * - select op codes for (Fast) Read, Page Program and Sector Erase. > @@ -3768,13 +3769,14 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, > } else if (mtd->size > 0x1000000) { > /* enable 4-byte addressing if the device exceeds 16MiB */ > nor->addr_width = 4; > - if (JEDEC_MFR(info) == SNOR_MFR_SPANSION || > - info->flags & SPI_NOR_4B_OPCODES) > - spi_nor_set_4byte_opcodes(nor); > } else { > nor->addr_width = 3; > } > > + if (nor->addr_width == 4 && > + nor->flags & SNOR_F_4B_OPCODES) the conditions fit in a single line Cheers, ta
On Wed, 5 Dec 2018 15:08:49 +0000 <Tudor.Ambarus@microchip.com> wrote: > Hi, Boris, > > On 11/29/2018 04:41 PM, Boris Brezillon wrote: > > Some flash_info entries have the SPI_NOR_4B_OPCODES flag set 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 the SFDP parsing code can set it when appropriate. > > > > Reported-by: Alexandre Belloni <alexandre.belloni@bootlin.com> > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> > > Tested-by: Alexandre Belloni <alexandre.belloni@bootlin.com> > > Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com> > > --- > > Changes in v4: > > - Set SNOR_F_4B_OPCODES flag outside of the if (mtd->size > 0x1000000) > > block > > - Do not set SNOR_F_4B_OPCODES when BFPT_DWORD1_ADDRESS_BYTES_4_ONLY, > > because 4byte only does not imply 4B opcodes are supported > > and you got rid of the superfluous check "(JEDEC_MFR(nor->info) != > SNOR_MFR_SPANSION)", which is good, thanks. > > And I guess you should have dropped my rb tag, because I haven't reviewed v4 > yet. No worries, just saying ... Yes, my bad. > > > > > Changes in v3: > > - Clear SNOR_F_4B_OPCODES flag when SFDP fails > > - Add Alexandre R-b > > > > Changes in v2: > > - Fix the commit message > > - Fix the ->addr_width check > > - Add a comma at the end of the SNOR_F_4B_OPCODES definition > > --- > > drivers/mtd/spi-nor/spi-nor.c | 22 ++++++++++++---------- > > include/linux/mtd/spi-nor.h | 1 + > > 2 files changed, 13 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c > > index c1d9c2e50bee..98b8d9a778aa 100644 > > --- a/drivers/mtd/spi-nor/spi-nor.c > > +++ b/drivers/mtd/spi-nor/spi-nor.c > > @@ -3311,6 +3311,7 @@ static int spi_nor_init_params(struct spi_nor *nor, > > > > if (spi_nor_parse_sfdp(nor, &sfdp_params)) { > > nor->addr_width = 0; > > + nor->flags &= ~SNOR_F_4B_OPCODES; > > /* restore previous erase map */ > > memcpy(&nor->erase_map, &prev_map, > > sizeof(nor->erase_map)); > > @@ -3561,9 +3562,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)) { > > + if (nor->addr_width == 4 && !(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 > > @@ -3595,10 +3594,8 @@ static void spi_nor_resume(struct mtd_info *mtd) > > 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_BROKEN_RESET)) > > + if (nor->addr_width == 4 && !(nor->flags & SNOR_F_4B_OPCODES) && > > + nor->flags & SNOR_F_BROKEN_RESET) > > set_4byte(nor, false); > > } > > EXPORT_SYMBOL_GPL(spi_nor_restore); > > @@ -3750,6 +3747,10 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, > > if (info->flags & SPI_NOR_NO_FR) > > params.hwcaps.mask &= ~SNOR_HWCAPS_READ_FAST; > > > > + if (info->flags & SPI_NOR_4B_OPCODES || > > + (JEDEC_MFR(info) == SNOR_MFR_SPANSION && mtd->size > SZ_16M)) > > + nor->flags |= SNOR_F_4B_OPCODES; > > + > > you are potentially overwriting the SNOR_F_4B_OPCODES that may be set in SFDP. I > suggest to set SNOR_F_4B_OPCODES flag inside of the "if (mtd->size > 0x1000000)" > block. Shouldn't we override this value anyway? I mean, I thought flash_info flags had precedence on the SFDP ones. Also, just because the flash is smaller than 16MB, doesn't mean it does not support 4B opcodes. We probably won't use the 4B opcodes in that case, but still. > > > /* > > * Configure the SPI memory: > > * - select op codes for (Fast) Read, Page Program and Sector Erase. > > @@ -3768,13 +3769,14 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, > > } else if (mtd->size > 0x1000000) { > > /* enable 4-byte addressing if the device exceeds 16MiB */ > > nor->addr_width = 4; > > - if (JEDEC_MFR(info) == SNOR_MFR_SPANSION || > > - info->flags & SPI_NOR_4B_OPCODES) > > - spi_nor_set_4byte_opcodes(nor); > > } else { > > nor->addr_width = 3; > > } > > > > + if (nor->addr_width == 4 && > > + nor->flags & SNOR_F_4B_OPCODES) > > the conditions fit in a single line Will fix that.
On 12/05/2018 05:19 PM, Boris Brezillon wrote: >>> @@ -3750,6 +3747,10 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, >>> if (info->flags & SPI_NOR_NO_FR) >>> params.hwcaps.mask &= ~SNOR_HWCAPS_READ_FAST; >>> >>> + if (info->flags & SPI_NOR_4B_OPCODES || >>> + (JEDEC_MFR(info) == SNOR_MFR_SPANSION && mtd->size > SZ_16M)) >>> + nor->flags |= SNOR_F_4B_OPCODES; >>> + >> you are potentially overwriting the SNOR_F_4B_OPCODES that may be set in SFDP. I >> suggest to set SNOR_F_4B_OPCODES flag inside of the "if (mtd->size > 0x1000000)" >> block. > Shouldn't we override this value anyway? I mean, I thought flash_info > flags had precedence on the SFDP ones. Also, just because the flash is I tend to say that we shouldn't. We have some "JEDEC knows better" attitude in the code: we are overwriting platform ID if we find a different ID in sfdp, we choose addr_width from SFDP even if set in info->addr_width, and we are overwriting all the settings based on flash_info when sfdp parsing succeeds in spi_nor_init_params(). > smaller than 16MB, doesn't mean it does not support 4B opcodes. We > probably won't use the 4B opcodes in that case, but still. > I agree that manufacturers have a sense of humor and this might be possible. But there's no need to use 4B opcodes in this case, so a post_sfdp fixup will help here too. Cheers, ta
On Wed, 5 Dec 2018 15:48:46 +0000 <Tudor.Ambarus@microchip.com> wrote: > On 12/05/2018 05:19 PM, Boris Brezillon wrote: > >>> @@ -3750,6 +3747,10 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, > >>> if (info->flags & SPI_NOR_NO_FR) > >>> params.hwcaps.mask &= ~SNOR_HWCAPS_READ_FAST; > >>> > >>> + if (info->flags & SPI_NOR_4B_OPCODES || > >>> + (JEDEC_MFR(info) == SNOR_MFR_SPANSION && mtd->size > SZ_16M)) > >>> + nor->flags |= SNOR_F_4B_OPCODES; > >>> + > >> you are potentially overwriting the SNOR_F_4B_OPCODES that may be set in SFDP. I > >> suggest to set SNOR_F_4B_OPCODES flag inside of the "if (mtd->size > 0x1000000)" > >> block. > > Shouldn't we override this value anyway? I mean, I thought flash_info > > flags had precedence on the SFDP ones. Also, just because the flash is > > I tend to say that we shouldn't. We have some "JEDEC knows better" attitude in > the code: we are overwriting platform ID if we find a different ID in sfdp, we > choose addr_width from SFDP even if set in info->addr_width, and we are > overwriting all the settings based on flash_info when sfdp parsing succeeds in > spi_nor_init_params(). Given all the "broken SFDP" problems we had, I'm not sure this was the right decision, but that's another topic. For this specific one, I'd really prefer to keep this code as is. Note that the "JEDEC_MFR(info) == SNOR_MFR_SPANSION && mtd->size > SZ_16M" is later moved to a post SFDP fixup hook in my rework, which means we'll anyway override the decision taken by the SFDP parsing. > > > smaller than 16MB, doesn't mean it does not support 4B opcodes. We > > probably won't use the 4B opcodes in that case, but still. > > > > I agree that manufacturers have a sense of humor and this might be possible. But > there's no need to use 4B opcodes in this case, so a post_sfdp fixup will help > here too. Except there's nothing to fix in this case, we just won't use 4B opcodes if we don't need to, that's all.
On 12/05/2018 06:00 PM, Boris Brezillon wrote: > On Wed, 5 Dec 2018 15:48:46 +0000 > <Tudor.Ambarus@microchip.com> wrote: > >> On 12/05/2018 05:19 PM, Boris Brezillon wrote: >>>>> @@ -3750,6 +3747,10 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, >>>>> if (info->flags & SPI_NOR_NO_FR) >>>>> params.hwcaps.mask &= ~SNOR_HWCAPS_READ_FAST; >>>>> >>>>> + if (info->flags & SPI_NOR_4B_OPCODES || >>>>> + (JEDEC_MFR(info) == SNOR_MFR_SPANSION && mtd->size > SZ_16M)) >>>>> + nor->flags |= SNOR_F_4B_OPCODES; >>>>> + >>>> you are potentially overwriting the SNOR_F_4B_OPCODES that may be set in SFDP. I >>>> suggest to set SNOR_F_4B_OPCODES flag inside of the "if (mtd->size > 0x1000000)" >>>> block. >>> Shouldn't we override this value anyway? I mean, I thought flash_info >>> flags had precedence on the SFDP ones. Also, just because the flash is >> >> I tend to say that we shouldn't. We have some "JEDEC knows better" attitude in >> the code: we are overwriting platform ID if we find a different ID in sfdp, we >> choose addr_width from SFDP even if set in info->addr_width, and we are >> overwriting all the settings based on flash_info when sfdp parsing succeeds in >> spi_nor_init_params(). > > Given all the "broken SFDP" problems we had, I'm not sure this was the > right decision, but that's another topic. > > For this specific one, I'd really prefer to keep this code as is. Note > that the "JEDEC_MFR(info) == SNOR_MFR_SPANSION && mtd->size > SZ_16M" > is later moved to a post SFDP fixup hook in my rework, which means we'll > anyway override the decision taken by the SFDP parsing. > >> >>> smaller than 16MB, doesn't mean it does not support 4B opcodes. We >>> probably won't use the 4B opcodes in that case, but still. >>> >> >> I agree that manufacturers have a sense of humor and this might be possible. But >> there's no need to use 4B opcodes in this case, so a post_sfdp fixup will help >> here too. > > Except there's nothing to fix in this case, we just won't use 4B > opcodes if we don't need to, that's all. you'll have an extra byte of address that has a tiny impact on performance on small requests. I see it as a fix, we should do what's best to do. anyway ...
On Wed, 5 Dec 2018 16:26:06 +0000 <Tudor.Ambarus@microchip.com> wrote: > On 12/05/2018 06:00 PM, Boris Brezillon wrote: > > On Wed, 5 Dec 2018 15:48:46 +0000 > > <Tudor.Ambarus@microchip.com> wrote: > > > >> On 12/05/2018 05:19 PM, Boris Brezillon wrote: > >>>>> @@ -3750,6 +3747,10 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, > >>>>> if (info->flags & SPI_NOR_NO_FR) > >>>>> params.hwcaps.mask &= ~SNOR_HWCAPS_READ_FAST; > >>>>> > >>>>> + if (info->flags & SPI_NOR_4B_OPCODES || > >>>>> + (JEDEC_MFR(info) == SNOR_MFR_SPANSION && mtd->size > SZ_16M)) > >>>>> + nor->flags |= SNOR_F_4B_OPCODES; > >>>>> + > >>>> you are potentially overwriting the SNOR_F_4B_OPCODES that may be set in SFDP. I > >>>> suggest to set SNOR_F_4B_OPCODES flag inside of the "if (mtd->size > 0x1000000)" > >>>> block. > >>> Shouldn't we override this value anyway? I mean, I thought flash_info > >>> flags had precedence on the SFDP ones. Also, just because the flash is > >> > >> I tend to say that we shouldn't. We have some "JEDEC knows better" attitude in > >> the code: we are overwriting platform ID if we find a different ID in sfdp, we > >> choose addr_width from SFDP even if set in info->addr_width, and we are > >> overwriting all the settings based on flash_info when sfdp parsing succeeds in > >> spi_nor_init_params(). > > > > Given all the "broken SFDP" problems we had, I'm not sure this was the > > right decision, but that's another topic. > > > > For this specific one, I'd really prefer to keep this code as is. Note > > that the "JEDEC_MFR(info) == SNOR_MFR_SPANSION && mtd->size > SZ_16M" > > is later moved to a post SFDP fixup hook in my rework, which means we'll > > anyway override the decision taken by the SFDP parsing. > > > >> > >>> smaller than 16MB, doesn't mean it does not support 4B opcodes. We > >>> probably won't use the 4B opcodes in that case, but still. > >>> > >> > >> I agree that manufacturers have a sense of humor and this might be possible. But > >> there's no need to use 4B opcodes in this case, so a post_sfdp fixup will help > >> here too. > > > > Except there's nothing to fix in this case, we just won't use 4B > > opcodes if we don't need to, that's all. > > you'll have an extra byte of address that has a tiny impact on performance on > small requests. I see it as a fix, we should do what's best to do. anyway ... No, see the checks that are done in this patch: to use 4B_CODES the flag should be set and the NOR should be larger than 16MB. The flag does not mean "use 4B opcodes", it meas "4B opcodes are supported".
On Wed, 5 Dec 2018 17:32:54 +0100 Boris Brezillon <boris.brezillon@bootlin.com> wrote: > > >>> smaller than 16MB, doesn't mean it does not support 4B opcodes. We > > >>> probably won't use the 4B opcodes in that case, but still. > > >>> > > >> > > >> I agree that manufacturers have a sense of humor and this might be possible. But > > >> there's no need to use 4B opcodes in this case, so a post_sfdp fixup will help > > >> here too. > > > > > > Except there's nothing to fix in this case, we just won't use 4B > > > opcodes if we don't need to, that's all. > > > > you'll have an extra byte of address that has a tiny impact on performance on > > small requests. I see it as a fix, we should do what's best to do. anyway ... > > No, see the checks that are done in this patch: to use 4B_CODES the > flag should be set and the NOR should be larger than 16MB. The flag > does not mean "use 4B opcodes", it meas "4B opMcodes are supported". Maybe I should rename the flag SNOR_F_SUPPORTS_4B_OPCODES to make it clear.
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c index c1d9c2e50bee..98b8d9a778aa 100644 --- a/drivers/mtd/spi-nor/spi-nor.c +++ b/drivers/mtd/spi-nor/spi-nor.c @@ -3311,6 +3311,7 @@ static int spi_nor_init_params(struct spi_nor *nor, if (spi_nor_parse_sfdp(nor, &sfdp_params)) { nor->addr_width = 0; + nor->flags &= ~SNOR_F_4B_OPCODES; /* restore previous erase map */ memcpy(&nor->erase_map, &prev_map, sizeof(nor->erase_map)); @@ -3561,9 +3562,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)) { + if (nor->addr_width == 4 && !(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 @@ -3595,10 +3594,8 @@ static void spi_nor_resume(struct mtd_info *mtd) 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_BROKEN_RESET)) + if (nor->addr_width == 4 && !(nor->flags & SNOR_F_4B_OPCODES) && + nor->flags & SNOR_F_BROKEN_RESET) set_4byte(nor, false); } EXPORT_SYMBOL_GPL(spi_nor_restore); @@ -3750,6 +3747,10 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, if (info->flags & SPI_NOR_NO_FR) params.hwcaps.mask &= ~SNOR_HWCAPS_READ_FAST; + if (info->flags & SPI_NOR_4B_OPCODES || + (JEDEC_MFR(info) == SNOR_MFR_SPANSION && mtd->size > SZ_16M)) + nor->flags |= SNOR_F_4B_OPCODES; + /* * Configure the SPI memory: * - select op codes for (Fast) Read, Page Program and Sector Erase. @@ -3768,13 +3769,14 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, } else if (mtd->size > 0x1000000) { /* enable 4-byte addressing if the device exceeds 16MiB */ nor->addr_width = 4; - if (JEDEC_MFR(info) == SNOR_MFR_SPANSION || - info->flags & SPI_NOR_4B_OPCODES) - spi_nor_set_4byte_opcodes(nor); } else { nor->addr_width = 3; } + if (nor->addr_width == 4 && + nor->flags & SNOR_F_4B_OPCODES) + spi_nor_set_4byte_opcodes(nor); + 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 8b1acf68b7ac..981d628305a2 100644 --- a/include/linux/mtd/spi-nor.h +++ b/include/linux/mtd/spi-nor.h @@ -237,6 +237,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), }; /**