| Message ID | 20181031144504.19405-1-boris.brezillon@bootlin.com |
|---|---|
| State | Superseded |
| Delegated to: | Boris Brezillon |
| Headers | show |
| Series | [v3,1/2] mtd: spi-nor: Make sure SFDP-based 4B_OPCODE support detection works correctly | expand |
On 10/31/2018 04:45 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 > 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> > Tested-by: Alexandre Belloni <alexandre.belloni@bootlin.com> > --- > 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 | 12 +++++++++--- > include/linux/mtd/spi-nor.h | 1 + > 2 files changed, 10 insertions(+), 3 deletions(-) > > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c > index 3e54e31889c7..798915b5c2b0 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; > > @@ -3252,6 +3253,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)); > @@ -3554,7 +3556,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 > @@ -3588,7 +3590,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); > } > @@ -3746,11 +3748,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 (nor->addr_width == 4 && Shouldn't SNOR_F_4B_OPCODES already imply nor->addr_width == 4? When SNOR_F_4B_OPCODES comes from bfpt, addr_width is set to 4. For the id-based caps detection, when mtd->size > 0x1000000, we set nor->addr_width = 4 too. The only uncovered case would be when } else if (info->addr_width) { nor->addr_width = info->addr_width; but this can be solved by reordering the else if cases. if (nor->addr_width) { /* already configured from SFDP */ } else if (mtd->size > 0x1000000) { ... } else if (info->addr_width) { nor->addr_width = info->addr_width; } else { nor->addr_width = 3; } What do you think? > + 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..5bc75110d5ea 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 Thu, 8 Nov 2018 16:55:29 +0000 <Tudor.Ambarus@microchip.com> wrote: > On 10/31/2018 04:45 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 > > 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> > > Tested-by: Alexandre Belloni <alexandre.belloni@bootlin.com> > > --- > > 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 | 12 +++++++++--- > > include/linux/mtd/spi-nor.h | 1 + > > 2 files changed, 10 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c > > index 3e54e31889c7..798915b5c2b0 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; > > > > @@ -3252,6 +3253,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)); > > @@ -3554,7 +3556,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 > > @@ -3588,7 +3590,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); > > } > > @@ -3746,11 +3748,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 (nor->addr_width == 4 && > > Shouldn't SNOR_F_4B_OPCODES already imply nor->addr_width == 4? Can't we have NORs supporting 4B opcodes but are less than 16MB? In this case SNOR_F_4B_OPCODES would be set and ->addr_width would be 3. > When SNOR_F_4B_OPCODES comes from bfpt, addr_width is set to 4. For the id-based > caps detection, when mtd->size > 0x1000000, we set nor->addr_width = 4 too. > > The only uncovered case would be when > } else if (info->addr_width) { > nor->addr_width = info->addr_width; > > but this can be solved by reordering the else if cases. > > if (nor->addr_width) { > /* already configured from SFDP */ > } else if (mtd->size > 0x1000000) { > ... > } else if (info->addr_width) { > nor->addr_width = info->addr_width; > } else { > nor->addr_width = 3; > } > > What do you think? I'd rather not change that in this patch, but feel free to propose a patch on top of mine to simplify the logic if you think it's needed. > > > + 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..5bc75110d5ea 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 11/08/2018 07:08 PM, Boris Brezillon wrote: > On Thu, 8 Nov 2018 16:55:29 +0000 > <Tudor.Ambarus@microchip.com> wrote: > >> On 10/31/2018 04:45 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 >>> 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> >>> Tested-by: Alexandre Belloni <alexandre.belloni@bootlin.com> >>> --- >>> 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 | 12 +++++++++--- >>> include/linux/mtd/spi-nor.h | 1 + >>> 2 files changed, 10 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c >>> index 3e54e31889c7..798915b5c2b0 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; >>> >>> @@ -3252,6 +3253,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)); >>> @@ -3554,7 +3556,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 >>> @@ -3588,7 +3590,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); >>> } >>> @@ -3746,11 +3748,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 (nor->addr_width == 4 && >> >> Shouldn't SNOR_F_4B_OPCODES already imply nor->addr_width == 4? > > Can't we have NORs supporting 4B opcodes but are less than 16MB? In > this case SNOR_F_4B_OPCODES would be set and ->addr_width would be 3. I guess not, it wouldn't make sense, but who knows ... :) The 4-byte opcodes indicate that 4-bytes of address follow the instruction. And 4-byte addresses make sense for spi memories that exceed the 16MB density. > >> When SNOR_F_4B_OPCODES comes from bfpt, addr_width is set to 4. For the id-based >> caps detection, when mtd->size > 0x1000000, we set nor->addr_width = 4 too. >> >> The only uncovered case would be when >> } else if (info->addr_width) { >> nor->addr_width = info->addr_width; >> >> but this can be solved by reordering the else if cases. >> >> if (nor->addr_width) { >> /* already configured from SFDP */ >> } else if (mtd->size > 0x1000000) { >> ... >> } else if (info->addr_width) { >> nor->addr_width = info->addr_width; >> } else { >> nor->addr_width = 3; >> } >> >> What do you think? > > I'd rather not change that in this patch, but feel free to propose > a patch on top of mine to simplify the logic if you think it's > needed. > yeah, it can be made in a separate patch. Cheers, ta
On 10/31/2018 04:45 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 > 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> > Tested-by: Alexandre Belloni <alexandre.belloni@bootlin.com> Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com> > --- > 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 | 12 +++++++++--- > include/linux/mtd/spi-nor.h | 1 + > 2 files changed, 10 insertions(+), 3 deletions(-) > > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c > index 3e54e31889c7..798915b5c2b0 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; > > @@ -3252,6 +3253,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)); > @@ -3554,7 +3556,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 > @@ -3588,7 +3590,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); > } > @@ -3746,11 +3748,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 (nor->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..5bc75110d5ea 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, 9 Nov 2018 09:57:13 +0000 <Tudor.Ambarus@microchip.com> wrote: > On 11/08/2018 07:08 PM, Boris Brezillon wrote: > > On Thu, 8 Nov 2018 16:55:29 +0000 > > <Tudor.Ambarus@microchip.com> wrote: > > > >> On 10/31/2018 04:45 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 > >>> 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> > >>> Tested-by: Alexandre Belloni <alexandre.belloni@bootlin.com> > >>> --- > >>> 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 | 12 +++++++++--- > >>> include/linux/mtd/spi-nor.h | 1 + > >>> 2 files changed, 10 insertions(+), 3 deletions(-) > >>> > >>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c > >>> index 3e54e31889c7..798915b5c2b0 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; > >>> > >>> @@ -3252,6 +3253,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)); > >>> @@ -3554,7 +3556,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 > >>> @@ -3588,7 +3590,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); > >>> } > >>> @@ -3746,11 +3748,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 (nor->addr_width == 4 && > >> > >> Shouldn't SNOR_F_4B_OPCODES already imply nor->addr_width == 4? > > > > Can't we have NORs supporting 4B opcodes but are less than 16MB? In > > this case SNOR_F_4B_OPCODES would be set and ->addr_width would be 3. > > I guess not, it wouldn't make sense, but who knows ... :) Don't under-estimate flash manufacturers :-). > > The 4-byte opcodes indicate that 4-bytes of address follow the instruction. And > 4-byte addresses make sense for spi memories that exceed the 16MB density. I'm pretty sure they use the same controller for <=16MB and >16MB chips, so I wouldn't be surprised if some of their modern <=16MB chips support 4B opcodes even if it's not needed.
On Fri, 9 Nov 2018 09:57:13 +0000 <Tudor.Ambarus@microchip.com> wrote: > >> When SNOR_F_4B_OPCODES comes from bfpt, addr_width is set to 4. For the id-based > >> caps detection, when mtd->size > 0x1000000, we set nor->addr_width = 4 too. > >> > >> The only uncovered case would be when > >> } else if (info->addr_width) { > >> nor->addr_width = info->addr_width; > >> > >> but this can be solved by reordering the else if cases. > >> > >> if (nor->addr_width) { > >> /* already configured from SFDP */ > >> } else if (mtd->size > 0x1000000) { > >> ... > >> } else if (info->addr_width) { > >> nor->addr_width = info->addr_width; > >> } else { > >> nor->addr_width = 3; > >> } > >> > >> What do you think? > > > > I'd rather not change that in this patch, but feel free to propose > > a patch on top of mine to simplify the logic if you think it's > > needed. > > > > yeah, it can be made in a separate patch. If you're okay with the current version, can you add a R-b? Thanks, Boris
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c index 3e54e31889c7..798915b5c2b0 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; @@ -3252,6 +3253,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)); @@ -3554,7 +3556,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 @@ -3588,7 +3590,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); } @@ -3746,11 +3748,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 (nor->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..5bc75110d5ea 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), }; /**