| Message ID | 20220228134505.203270-3-tudor.ambarus@microchip.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mtd: spi-nor: Handle ID collisions | expand |
Am 2022-02-28 14:45, schrieb Tudor Ambarus: > A typical differentiator between flashes whose ID collide is whether > they > support SFDP or not. For such a collision there will be a single > flash_info entry where the developer should specify: > 1/ PARSE_SFDP - so that the flash that supports SFDP to initialize its > parameters by parsing the SFDP tables > 2/ any of the no_sfdp_flags less SPI_NOR_SKIP_SFDP, to initialize the > flash parameters via the static no_sfdp_flags for the flash that > doesn't support SFDP. > Use the logic the above to handle ID collisions between SFDP & non-SFDP > flashes. > > Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> > --- > drivers/mtd/spi-nor/core.c | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c > index fbf3278ba29a..aef00151c116 100644 > --- a/drivers/mtd/spi-nor/core.c > +++ b/drivers/mtd/spi-nor/core.c > @@ -2639,8 +2639,17 @@ static int spi_nor_init_params(struct spi_nor > *nor) > if (nor->info->parse_sfdp) { > ret = spi_nor_parse_sfdp(nor); Can we return -ENOENT here if sfdp isn't supported, so we can differentiate between "no sfdp present" and other errors? > if (ret) { > - dev_err(nor->dev, "BFPT parsing failed. Please consider using > SPI_NOR_SKIP_SFDP when declaring the flash\n"); > - return ret; > + /* > + * Handle ID collisions between flashes that support > + * SFDP and flashes that don't. Initialize parameters > + * for the flash that doesn't support SFDP. > + */ > + if (nor->info->no_sfdp_flags & ~SPI_NOR_SKIP_SFDP) { Shouldn't this be if (!(nor->info->no_sfdp_flags & SPI_NOR_SKIP_SFDP)) > + spi_nor_no_sfdp_init_params(nor); > + } else { > + dev_err(nor->dev, "BFPT parsing failed. Please consider using > SPI_NOR_SKIP_SFDP when declaring the flash\n"); > + return ret; > + } > } > } else if (nor->info->no_sfdp_flags & SPI_NOR_SKIP_SFDP) { > spi_nor_no_sfdp_init_params(nor); -michael
On 3/1/22 23:52, Michael Walle wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > Am 2022-02-28 14:45, schrieb Tudor Ambarus: >> A typical differentiator between flashes whose ID collide is whether >> they >> support SFDP or not. For such a collision there will be a single >> flash_info entry where the developer should specify: >> 1/ PARSE_SFDP - so that the flash that supports SFDP to initialize its >> parameters by parsing the SFDP tables >> 2/ any of the no_sfdp_flags less SPI_NOR_SKIP_SFDP, to initialize the >> flash parameters via the static no_sfdp_flags for the flash that >> doesn't support SFDP. >> Use the logic the above to handle ID collisions between SFDP & non-SFDP >> flashes. >> >> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> >> --- >> drivers/mtd/spi-nor/core.c | 13 +++++++++++-- >> 1 file changed, 11 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c >> index fbf3278ba29a..aef00151c116 100644 >> --- a/drivers/mtd/spi-nor/core.c >> +++ b/drivers/mtd/spi-nor/core.c >> @@ -2639,8 +2639,17 @@ static int spi_nor_init_params(struct spi_nor >> *nor) >> if (nor->info->parse_sfdp) { >> ret = spi_nor_parse_sfdp(nor); > > Can we return -ENOENT here if sfdp isn't supported, so we > can differentiate between "no sfdp present" and other errors? I'll take a look. > >> if (ret) { >> - dev_err(nor->dev, "BFPT parsing failed. Please consider using >> SPI_NOR_SKIP_SFDP when declaring the flash\n"); >> - return ret; >> + /* >> + * Handle ID collisions between flashes that support >> + * SFDP and flashes that don't. Initialize parameters >> + * for the flash that doesn't support SFDP. >> + */ >> + if (nor->info->no_sfdp_flags & ~SPI_NOR_SKIP_SFDP) { > > Shouldn't this be > if (!(nor->info->no_sfdp_flags & SPI_NOR_SKIP_SFDP)) No, because this will be true when no_sfdp_flags is zero, and the method from below will be called. I would like to call it when any of the no_sfdp_flags are defined, less the SPI_NOR_SKIP_SFDP flag. So when one declares a flash like: + { "mx25l3205d", INFO(0xc22016, 0, 64 * 1024, 64) + /* ID collision with mx25l3233f. */ + PARSE_SFDP + NO_SFDP_FLAGS(SECT_4K) First we will try to retrieve the flash params from SFDP. If SFDP fails, then we'll init the flash based on the no_sfdp_flags. If SFDP succeeds the no_sfdp_flags is ignored. Cheers, ta
Am 2022-03-03 15:41, schrieb Tudor.Ambarus@microchip.com: > On 3/1/22 23:52, Michael Walle wrote: >> EXTERNAL EMAIL: Do not click links or open attachments unless you know >> the content is safe >> >> Am 2022-02-28 14:45, schrieb Tudor Ambarus: >>> A typical differentiator between flashes whose ID collide is whether >>> they >>> support SFDP or not. For such a collision there will be a single >>> flash_info entry where the developer should specify: >>> 1/ PARSE_SFDP - so that the flash that supports SFDP to initialize >>> its >>> parameters by parsing the SFDP tables >>> 2/ any of the no_sfdp_flags less SPI_NOR_SKIP_SFDP, to initialize the >>> flash parameters via the static no_sfdp_flags for the flash that >>> doesn't support SFDP. >>> Use the logic the above to handle ID collisions between SFDP & >>> non-SFDP >>> flashes. >>> >>> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> >>> --- >>> drivers/mtd/spi-nor/core.c | 13 +++++++++++-- >>> 1 file changed, 11 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c >>> index fbf3278ba29a..aef00151c116 100644 >>> --- a/drivers/mtd/spi-nor/core.c >>> +++ b/drivers/mtd/spi-nor/core.c >>> @@ -2639,8 +2639,17 @@ static int spi_nor_init_params(struct spi_nor >>> *nor) >>> if (nor->info->parse_sfdp) { >>> ret = spi_nor_parse_sfdp(nor); >> >> Can we return -ENOENT here if sfdp isn't supported, so we >> can differentiate between "no sfdp present" and other errors? > > I'll take a look. > >> >>> if (ret) { >>> - dev_err(nor->dev, "BFPT parsing failed. Please >>> consider using >>> SPI_NOR_SKIP_SFDP when declaring the flash\n"); >>> - return ret; >>> + /* >>> + * Handle ID collisions between flashes that >>> support >>> + * SFDP and flashes that don't. Initialize >>> parameters >>> + * for the flash that doesn't support SFDP. >>> + */ >>> + if (nor->info->no_sfdp_flags & >>> ~SPI_NOR_SKIP_SFDP) { >> >> Shouldn't this be >> if (!(nor->info->no_sfdp_flags & SPI_NOR_SKIP_SFDP)) Ahh I misread that for the "skip no sftp handling". But doesn't render my point below invalid. > No, because this will be true when no_sfdp_flags is zero, and the > method > from below will be called. I would like to call it when any of the > no_sfdp_flags are defined, less the SPI_NOR_SKIP_SFDP flag. You should add a comment then. > So when one > declares a flash like: > + { "mx25l3205d", INFO(0xc22016, 0, 64 * 1024, 64) > + /* ID collision with mx25l3233f. */ > + PARSE_SFDP > + NO_SFDP_FLAGS(SECT_4K) But what about + { "differentflash", INFO(0xc22016, 0, 64 * 1024, 64) + /* ID collision with mx25l3233f. */ + PARSE_SFDP Thats also valid, no? Why is having 4k sectors special? FWIW, the function not only handles no_sfdp_flags but also erase related things. So in summary, the nosfdp handling is always called when parsing fails (that is when there is no SFDP, not due to read errors or similar). I don't see why that shouldn't be the case. -michael > First we will try to retrieve the flash params from SFDP. If SFDP > fails, > then we'll init the flash based on the no_sfdp_flags. If SFDP succeeds > the no_sfdp_flags is ignored.
On 3/3/22 16:51, Michael Walle wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > Am 2022-03-03 15:41, schrieb Tudor.Ambarus@microchip.com: >> On 3/1/22 23:52, Michael Walle wrote: >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know >>> the content is safe >>> >>> Am 2022-02-28 14:45, schrieb Tudor Ambarus: >>>> A typical differentiator between flashes whose ID collide is whether >>>> they >>>> support SFDP or not. For such a collision there will be a single >>>> flash_info entry where the developer should specify: >>>> 1/ PARSE_SFDP - so that the flash that supports SFDP to initialize >>>> its >>>> parameters by parsing the SFDP tables >>>> 2/ any of the no_sfdp_flags less SPI_NOR_SKIP_SFDP, to initialize the >>>> flash parameters via the static no_sfdp_flags for the flash that >>>> doesn't support SFDP. >>>> Use the logic the above to handle ID collisions between SFDP & >>>> non-SFDP >>>> flashes. >>>> >>>> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> >>>> --- >>>> drivers/mtd/spi-nor/core.c | 13 +++++++++++-- >>>> 1 file changed, 11 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c >>>> index fbf3278ba29a..aef00151c116 100644 >>>> --- a/drivers/mtd/spi-nor/core.c >>>> +++ b/drivers/mtd/spi-nor/core.c >>>> @@ -2639,8 +2639,17 @@ static int spi_nor_init_params(struct spi_nor >>>> *nor) >>>> if (nor->info->parse_sfdp) { >>>> ret = spi_nor_parse_sfdp(nor); >>> >>> Can we return -ENOENT here if sfdp isn't supported, so we >>> can differentiate between "no sfdp present" and other errors? >> >> I'll take a look. >> >>> >>>> if (ret) { >>>> - dev_err(nor->dev, "BFPT parsing failed. Please >>>> consider using >>>> SPI_NOR_SKIP_SFDP when declaring the flash\n"); >>>> - return ret; >>>> + /* >>>> + * Handle ID collisions between flashes that >>>> support >>>> + * SFDP and flashes that don't. Initialize >>>> parameters >>>> + * for the flash that doesn't support SFDP. >>>> + */ >>>> + if (nor->info->no_sfdp_flags & >>>> ~SPI_NOR_SKIP_SFDP) { >>> >>> Shouldn't this be >>> if (!(nor->info->no_sfdp_flags & SPI_NOR_SKIP_SFDP)) > > Ahh I misread that for the "skip no sftp handling". But doesn't render > my point below invalid. > >> No, because this will be true when no_sfdp_flags is zero, and the >> method >> from below will be called. I would like to call it when any of the >> no_sfdp_flags are defined, less the SPI_NOR_SKIP_SFDP flag. > > You should add a comment then. > >> So when one >> declares a flash like: >> + { "mx25l3205d", INFO(0xc22016, 0, 64 * 1024, 64) >> + /* ID collision with mx25l3233f. */ >> + PARSE_SFDP >> + NO_SFDP_FLAGS(SECT_4K) > > But what about > + { "differentflash", INFO(0xc22016, 0, 64 * 1024, 64) > + /* ID collision with mx25l3233f. */ > + PARSE_SFDP > > Thats also valid, no? Why is having 4k sectors special? FWIW, the no, because just the first entry with the 0xc22016 ID will be hit, the second one will be ignored. We use a single flash entry for all the flashes that collide. PARSE_SFDP together with any of the: #define SECT_4K BIT(1) #define SECT_4K_PMC BIT(2) #define SPI_NOR_DUAL_READ BIT(3) #define SPI_NOR_QUAD_READ BIT(4) #define SPI_NOR_OCTAL_READ BIT(5) #define SPI_NOR_OCTAL_DTR_READ BIT(6) #define SPI_NOR_OCTAL_DTR_PP BIT(7) suggests that we'd like to differentiate between a flash that supports SFDP and can discover its params via SFDP (no_sfdp_flags will be ignored), and a flash that doesn't support SFDP and it's forced to initialize the params via the no_sfdp_flags. > function not only handles no_sfdp_flags but also erase related > things. > > So in summary, the nosfdp handling is always called when parsing > fails (that is when there is no SFDP, not due to read errors or > similar). I don't see why that shouldn't be the case. > > -michael > >> First we will try to retrieve the flash params from SFDP. If SFDP >> fails, >> then we'll init the flash based on the no_sfdp_flags. If SFDP succeeds >> the no_sfdp_flags is ignored.
Am 2022-03-03 16:25, schrieb Tudor.Ambarus@microchip.com: > On 3/3/22 16:51, Michael Walle wrote: >> EXTERNAL EMAIL: Do not click links or open attachments unless you know >> the content is safe >> >> Am 2022-03-03 15:41, schrieb Tudor.Ambarus@microchip.com: >>> On 3/1/22 23:52, Michael Walle wrote: >>>> EXTERNAL EMAIL: Do not click links or open attachments unless you >>>> know >>>> the content is safe >>>> >>>> Am 2022-02-28 14:45, schrieb Tudor Ambarus: >>>>> A typical differentiator between flashes whose ID collide is >>>>> whether >>>>> they >>>>> support SFDP or not. For such a collision there will be a single >>>>> flash_info entry where the developer should specify: >>>>> 1/ PARSE_SFDP - so that the flash that supports SFDP to initialize >>>>> its >>>>> parameters by parsing the SFDP tables >>>>> 2/ any of the no_sfdp_flags less SPI_NOR_SKIP_SFDP, to initialize >>>>> the >>>>> flash parameters via the static no_sfdp_flags for the flash that >>>>> doesn't support SFDP. >>>>> Use the logic the above to handle ID collisions between SFDP & >>>>> non-SFDP >>>>> flashes. >>>>> >>>>> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> >>>>> --- >>>>> drivers/mtd/spi-nor/core.c | 13 +++++++++++-- >>>>> 1 file changed, 11 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/drivers/mtd/spi-nor/core.c >>>>> b/drivers/mtd/spi-nor/core.c >>>>> index fbf3278ba29a..aef00151c116 100644 >>>>> --- a/drivers/mtd/spi-nor/core.c >>>>> +++ b/drivers/mtd/spi-nor/core.c >>>>> @@ -2639,8 +2639,17 @@ static int spi_nor_init_params(struct >>>>> spi_nor >>>>> *nor) >>>>> if (nor->info->parse_sfdp) { >>>>> ret = spi_nor_parse_sfdp(nor); >>>> >>>> Can we return -ENOENT here if sfdp isn't supported, so we >>>> can differentiate between "no sfdp present" and other errors? >>> >>> I'll take a look. >>> >>>> >>>>> if (ret) { >>>>> - dev_err(nor->dev, "BFPT parsing failed. >>>>> Please >>>>> consider using >>>>> SPI_NOR_SKIP_SFDP when declaring the flash\n"); >>>>> - return ret; >>>>> + /* >>>>> + * Handle ID collisions between flashes that >>>>> support >>>>> + * SFDP and flashes that don't. Initialize >>>>> parameters >>>>> + * for the flash that doesn't support SFDP. >>>>> + */ >>>>> + if (nor->info->no_sfdp_flags & >>>>> ~SPI_NOR_SKIP_SFDP) { >>>> >>>> Shouldn't this be >>>> if (!(nor->info->no_sfdp_flags & SPI_NOR_SKIP_SFDP)) >> >> Ahh I misread that for the "skip no sftp handling". But doesn't render >> my point below invalid. >> >>> No, because this will be true when no_sfdp_flags is zero, and the >>> method >>> from below will be called. I would like to call it when any of the >>> no_sfdp_flags are defined, less the SPI_NOR_SKIP_SFDP flag. >> >> You should add a comment then. >> >>> So when one >>> declares a flash like: >>> + { "mx25l3205d", INFO(0xc22016, 0, 64 * 1024, 64) >>> + /* ID collision with mx25l3233f. */ >>> + PARSE_SFDP >>> + NO_SFDP_FLAGS(SECT_4K) >> >> But what about >> + { "differentflash", INFO(0xc22016, 0, 64 * 1024, 64) >> + /* ID collision with mx25l3233f. */ >> + PARSE_SFDP >> >> Thats also valid, no? Why is having 4k sectors special? FWIW, the > > no, because just the first entry with the 0xc22016 ID will be hit, the > second one will be ignored. We use a single flash entry for all the > flashes that collide. Not in addition but instead of yours, of course. > PARSE_SFDP together with any of the: > #define SECT_4K BIT(1) > #define SECT_4K_PMC BIT(2) > #define SPI_NOR_DUAL_READ BIT(3) > #define SPI_NOR_QUAD_READ BIT(4) > #define SPI_NOR_OCTAL_READ BIT(5) > #define SPI_NOR_OCTAL_DTR_READ BIT(6) > #define SPI_NOR_OCTAL_DTR_PP BIT(7) > > suggests that we'd like to differentiate between a flash that supports > SFDP and can discover its params via SFDP (no_sfdp_flags will be > ignored), > and a flash that doesn't support SFDP and it's forced to initialize the > params via the no_sfdp_flags. I get that, but what if I have a flash, which doesn't have 4k sectors but ordinary 64k sectors (to stick to your example). In general, what if I have a flash where none of the above flags are set. You only call that function if there are any no_sfdp flags set, but they are all optional, no? Who is setting the erase opcode for flashes with that ID but without SFDP, then? >> function not only handles no_sfdp_flags but also erase related >> things. >> >> So in summary, the nosfdp handling is always called when parsing >> fails (that is when there is no SFDP, not due to read errors or >> similar). I don't see why that shouldn't be the case. >> >> -michael >> >>> First we will try to retrieve the flash params from SFDP. If SFDP >>> fails, >>> then we'll init the flash based on the no_sfdp_flags. If SFDP >>> succeeds >>> the no_sfdp_flags is ignored.
On 3/3/22 17:42, Michael Walle wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > Am 2022-03-03 16:25, schrieb Tudor.Ambarus@microchip.com: >> On 3/3/22 16:51, Michael Walle wrote: >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know >>> the content is safe >>> >>> Am 2022-03-03 15:41, schrieb Tudor.Ambarus@microchip.com: >>>> On 3/1/22 23:52, Michael Walle wrote: >>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you >>>>> know >>>>> the content is safe >>>>> >>>>> Am 2022-02-28 14:45, schrieb Tudor Ambarus: >>>>>> A typical differentiator between flashes whose ID collide is >>>>>> whether >>>>>> they >>>>>> support SFDP or not. For such a collision there will be a single >>>>>> flash_info entry where the developer should specify: >>>>>> 1/ PARSE_SFDP - so that the flash that supports SFDP to initialize >>>>>> its >>>>>> parameters by parsing the SFDP tables >>>>>> 2/ any of the no_sfdp_flags less SPI_NOR_SKIP_SFDP, to initialize >>>>>> the >>>>>> flash parameters via the static no_sfdp_flags for the flash that >>>>>> doesn't support SFDP. >>>>>> Use the logic the above to handle ID collisions between SFDP & >>>>>> non-SFDP >>>>>> flashes. >>>>>> >>>>>> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> >>>>>> --- >>>>>> drivers/mtd/spi-nor/core.c | 13 +++++++++++-- >>>>>> 1 file changed, 11 insertions(+), 2 deletions(-) >>>>>> >>>>>> diff --git a/drivers/mtd/spi-nor/core.c >>>>>> b/drivers/mtd/spi-nor/core.c >>>>>> index fbf3278ba29a..aef00151c116 100644 >>>>>> --- a/drivers/mtd/spi-nor/core.c >>>>>> +++ b/drivers/mtd/spi-nor/core.c >>>>>> @@ -2639,8 +2639,17 @@ static int spi_nor_init_params(struct >>>>>> spi_nor >>>>>> *nor) >>>>>> if (nor->info->parse_sfdp) { >>>>>> ret = spi_nor_parse_sfdp(nor); >>>>> >>>>> Can we return -ENOENT here if sfdp isn't supported, so we >>>>> can differentiate between "no sfdp present" and other errors? >>>> >>>> I'll take a look. >>>> >>>>> >>>>>> if (ret) { >>>>>> - dev_err(nor->dev, "BFPT parsing failed. >>>>>> Please >>>>>> consider using >>>>>> SPI_NOR_SKIP_SFDP when declaring the flash\n"); >>>>>> - return ret; >>>>>> + /* >>>>>> + * Handle ID collisions between flashes that >>>>>> support >>>>>> + * SFDP and flashes that don't. Initialize >>>>>> parameters >>>>>> + * for the flash that doesn't support SFDP. >>>>>> + */ >>>>>> + if (nor->info->no_sfdp_flags & >>>>>> ~SPI_NOR_SKIP_SFDP) { >>>>> >>>>> Shouldn't this be >>>>> if (!(nor->info->no_sfdp_flags & SPI_NOR_SKIP_SFDP)) >>> >>> Ahh I misread that for the "skip no sftp handling". But doesn't render >>> my point below invalid. >>> >>>> No, because this will be true when no_sfdp_flags is zero, and the >>>> method >>>> from below will be called. I would like to call it when any of the >>>> no_sfdp_flags are defined, less the SPI_NOR_SKIP_SFDP flag. >>> >>> You should add a comment then. >>> >>>> So when one >>>> declares a flash like: >>>> + { "mx25l3205d", INFO(0xc22016, 0, 64 * 1024, 64) >>>> + /* ID collision with mx25l3233f. */ >>>> + PARSE_SFDP >>>> + NO_SFDP_FLAGS(SECT_4K) >>> >>> But what about >>> + { "differentflash", INFO(0xc22016, 0, 64 * 1024, 64) >>> + /* ID collision with mx25l3233f. */ >>> + PARSE_SFDP >>> >>> Thats also valid, no? Why is having 4k sectors special? FWIW, the >> >> no, because just the first entry with the 0xc22016 ID will be hit, the >> second one will be ignored. We use a single flash entry for all the >> flashes that collide. > > Not in addition but instead of yours, of course. > >> PARSE_SFDP together with any of the: >> #define SECT_4K BIT(1) >> #define SECT_4K_PMC BIT(2) >> #define SPI_NOR_DUAL_READ BIT(3) >> #define SPI_NOR_QUAD_READ BIT(4) >> #define SPI_NOR_OCTAL_READ BIT(5) >> #define SPI_NOR_OCTAL_DTR_READ BIT(6) >> #define SPI_NOR_OCTAL_DTR_PP BIT(7) >> >> suggests that we'd like to differentiate between a flash that supports >> SFDP and can discover its params via SFDP (no_sfdp_flags will be >> ignored), >> and a flash that doesn't support SFDP and it's forced to initialize the >> params via the no_sfdp_flags. > > I get that, but what if I have a flash, which doesn't have 4k > sectors but ordinary 64k sectors (to stick to your example). In > general, what if I have a flash where none of the above flags > are set. You only call that function if there are any no_sfdp flags > set, but they are all optional, no? Who is setting the erase opcode > for flashes with that ID but without SFDP, then? this use case is not handled in this proposal, indeed. I'm not sure if there are such flashes though. Can't think of an example. We search for a flash that operates only in 1-1-1 and can't do 4k erase. Is that a NOR? But if you feel we should care about this case too, I'll address it, probably I will be forced to introduce a new flag or a bool, meh. > >>> function not only handles no_sfdp_flags but also erase related >>> things. >>> >>> So in summary, the nosfdp handling is always called when parsing >>> fails (that is when there is no SFDP, not due to read errors or >>> similar). I don't see why that shouldn't be the case. >>> >>> -michael >>> >>>> First we will try to retrieve the flash params from SFDP. If SFDP >>>> fails, >>>> then we'll init the flash based on the no_sfdp_flags. If SFDP >>>> succeeds >>>> the no_sfdp_flags is ignored. > > -- > -michael
Am 2022-03-03 17:03, schrieb Tudor.Ambarus@microchip.com: .. >>>>> So when one >>>>> declares a flash like: >>>>> + { "mx25l3205d", INFO(0xc22016, 0, 64 * 1024, 64) >>>>> + /* ID collision with mx25l3233f. */ >>>>> + PARSE_SFDP >>>>> + NO_SFDP_FLAGS(SECT_4K) >>>> >>>> But what about >>>> + { "differentflash", INFO(0xc22016, 0, 64 * 1024, 64) >>>> + /* ID collision with mx25l3233f. */ >>>> + PARSE_SFDP >>>> >>>> Thats also valid, no? Why is having 4k sectors special? FWIW, the >>> >>> no, because just the first entry with the 0xc22016 ID will be hit, >>> the >>> second one will be ignored. We use a single flash entry for all the >>> flashes that collide. >> >> Not in addition but instead of yours, of course. >> >>> PARSE_SFDP together with any of the: >>> #define SECT_4K BIT(1) >>> #define SECT_4K_PMC BIT(2) >>> #define SPI_NOR_DUAL_READ BIT(3) >>> #define SPI_NOR_QUAD_READ BIT(4) >>> #define SPI_NOR_OCTAL_READ BIT(5) >>> #define SPI_NOR_OCTAL_DTR_READ BIT(6) >>> #define SPI_NOR_OCTAL_DTR_PP BIT(7) >>> >>> suggests that we'd like to differentiate between a flash that >>> supports >>> SFDP and can discover its params via SFDP (no_sfdp_flags will be >>> ignored), >>> and a flash that doesn't support SFDP and it's forced to initialize >>> the >>> params via the no_sfdp_flags. >> >> I get that, but what if I have a flash, which doesn't have 4k >> sectors but ordinary 64k sectors (to stick to your example). In >> general, what if I have a flash where none of the above flags >> are set. You only call that function if there are any no_sfdp flags >> set, but they are all optional, no? Who is setting the erase opcode >> for flashes with that ID but without SFDP, then? > > this use case is not handled in this proposal, indeed. I'm not sure > if there are such flashes though. Can't think of an example. We search > for a flash that operates only in 1-1-1 and can't do 4k erase. > Is that a NOR? But if you feel we should care about this case too, > I'll address it, probably I will be forced to introduce a new flag or > a bool, meh. 1-1-1 with 64k sectors would be the very basic spi nor flash no? In any case, no I don't care that much about this, it was just not that easy to understand why you did it this way. So we should really add a comment that we assume here that at least one nosfdp flag is set. -michael
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c index fbf3278ba29a..aef00151c116 100644 --- a/drivers/mtd/spi-nor/core.c +++ b/drivers/mtd/spi-nor/core.c @@ -2639,8 +2639,17 @@ static int spi_nor_init_params(struct spi_nor *nor) if (nor->info->parse_sfdp) { ret = spi_nor_parse_sfdp(nor); if (ret) { - dev_err(nor->dev, "BFPT parsing failed. Please consider using SPI_NOR_SKIP_SFDP when declaring the flash\n"); - return ret; + /* + * Handle ID collisions between flashes that support + * SFDP and flashes that don't. Initialize parameters + * for the flash that doesn't support SFDP. + */ + if (nor->info->no_sfdp_flags & ~SPI_NOR_SKIP_SFDP) { + spi_nor_no_sfdp_init_params(nor); + } else { + dev_err(nor->dev, "BFPT parsing failed. Please consider using SPI_NOR_SKIP_SFDP when declaring the flash\n"); + return ret; + } } } else if (nor->info->no_sfdp_flags & SPI_NOR_SKIP_SFDP) { spi_nor_no_sfdp_init_params(nor);
A typical differentiator between flashes whose ID collide is whether they support SFDP or not. For such a collision there will be a single flash_info entry where the developer should specify: 1/ PARSE_SFDP - so that the flash that supports SFDP to initialize its parameters by parsing the SFDP tables 2/ any of the no_sfdp_flags less SPI_NOR_SKIP_SFDP, to initialize the flash parameters via the static no_sfdp_flags for the flash that doesn't support SFDP. Use the logic the above to handle ID collisions between SFDP & non-SFDP flashes. Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> --- drivers/mtd/spi-nor/core.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)