| Message ID | 20210727045222.905056-18-tudor.ambarus@microchip.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mtd: spi-nor: Handle ID collisions and clean params init | expand |
On 27/07/21 07:52AM, Tudor Ambarus wrote: > Used to initialize the NOR flags for settings that are not defined > in the JESD216 SFDP standard, thus can not be retrieved when parsing > SFDP. No functional change. I am worried if the order in which these flags are set can cause some subtle bugs. I can see one instance of it with SNOR_F_HAS_LOCK. spi_nor_late_init_params() checks for SNOR_F_HAS_LOCK and if there are no locking ops specified, it sets the default locking ops. This works fine before this patch because the flag is set before the function is called. But now, the flag will be set _after_ the function is called, and so you will never be able to set the default flags. This is one bug I can spot but I fear some others might be hiding somewhere as well. SPI NOR has accumulated a lot of spaghetti code over the years and I certainly felt it when working on my Octal DTR series. It caused an address width selection bug that was not obvious at all, and was not even caught during the rc cycles. I think this series does clean up that spaghetti a lot. But you need to be careful of such bugs. I think you should definitely let this series cook in next for some time so it gets some exposure and hopefully some testing. > > Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> > --- > drivers/mtd/spi-nor/core.c | 88 ++++++++++++++++++++++---------------- > 1 file changed, 52 insertions(+), 36 deletions(-) > > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c > index 1f38fa8ab2fa..6a8617346764 100644 > --- a/drivers/mtd/spi-nor/core.c > +++ b/drivers/mtd/spi-nor/core.c > @@ -2687,6 +2687,56 @@ static void spi_nor_late_init_params(struct spi_nor *nor) > spi_nor_init_default_locking_ops(nor); > } > > +/** > + * spi_nor_nonsfdp_flags_init() - Initialize NOR flags for settings that are not > + * defined in the JESD216 SFDP standard, thus can not be retrieved when parsing > + * SFDP. > + * @nor: pointer to a 'struct spi_nor' > + */ > +static void spi_nor_nonsfdp_flags_init(struct spi_nor *nor) > +{ > + const struct flash_info *info = nor->info; > + struct device_node *np = spi_nor_get_flash_node(nor); > + > + if (of_property_read_bool(np, "broken-flash-reset")) > + nor->flags |= SNOR_F_BROKEN_RESET; > + > + if (info->flags & SPI_NOR_SWP_IS_VOLATILE) > + nor->flags |= SNOR_F_SWP_IS_VOLATILE; > + > + if (info->flags & SPI_NOR_HAS_LOCK) > + nor->flags |= SNOR_F_HAS_LOCK; As mentioned above, this would cause a bug. > + > + if (info->flags & SPI_NOR_HAS_TB) { > + nor->flags |= SNOR_F_HAS_SR_TB; > + if (info->flags & SPI_NOR_TB_SR_BIT6) > + nor->flags |= SNOR_F_HAS_SR_TB_BIT6; > + } > + > + if (info->flags & SPI_NOR_4BIT_BP) { > + nor->flags |= SNOR_F_HAS_4BIT_BP; > + if (info->flags & SPI_NOR_BP3_SR_BIT6) > + nor->flags |= SNOR_F_HAS_SR_BP3_BIT6; > + } > + > + if (info->flags & NO_CHIP_ERASE) > + nor->flags |= SNOR_F_NO_OP_CHIP_ERASE; > + > + if (info->flags & USE_FSR) > + nor->flags |= SNOR_F_USE_FSR; > + > + if (info->flags & USE_CLSR) > + nor->flags |= SNOR_F_USE_CLSR; > + > + /* > + * Make sure the XSR_RDY flag is set before calling > + * spi_nor_wait_till_ready(). Xilinx S3AN share MFR > + * with Atmel SPI NOR. > + */ > + if (info->flags & SPI_NOR_XSR_RDY) > + nor->flags |= SNOR_F_READY_XSR_RDY; > +} > + > /** > * spi_nor_init_params() - Initialize the flash's parameters and settings. > * @nor: pointer to a 'struct spi_nor'. > @@ -2736,6 +2786,8 @@ static int spi_nor_init_params(struct spi_nor *nor) > > spi_nor_late_init_params(nor); > > + spi_nor_nonsfdp_flags_init(nor); > + > return 0; > } > > @@ -3078,7 +3130,6 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, > const struct flash_info *info; > struct device *dev = nor->dev; > struct mtd_info *mtd = &nor->mtd; > - struct device_node *np = spi_nor_get_flash_node(nor); > int ret; > int i; > > @@ -3115,17 +3166,6 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, > > mutex_init(&nor->lock); > > - /* > - * Make sure the XSR_RDY flag is set before calling > - * spi_nor_wait_till_ready(). Xilinx S3AN share MFR > - * with Atmel SPI NOR. > - */ > - if (info->flags & SPI_NOR_XSR_RDY) > - nor->flags |= SNOR_F_READY_XSR_RDY; > - > - if (info->flags & SPI_NOR_HAS_LOCK) > - nor->flags |= SNOR_F_HAS_LOCK; > - > mtd->_write = spi_nor_write; > > /* Init flash parameters based on flash_info struct and SFDP */ > @@ -3147,27 +3187,6 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, > mtd->_get_device = spi_nor_get_device; > mtd->_put_device = spi_nor_put_device; > > - if (info->flags & USE_FSR) > - nor->flags |= SNOR_F_USE_FSR; > - if (info->flags & SPI_NOR_HAS_TB) { > - nor->flags |= SNOR_F_HAS_SR_TB; > - if (info->flags & SPI_NOR_TB_SR_BIT6) > - nor->flags |= SNOR_F_HAS_SR_TB_BIT6; > - } > - > - if (info->flags & NO_CHIP_ERASE) > - nor->flags |= SNOR_F_NO_OP_CHIP_ERASE; > - if (info->flags & USE_CLSR) > - nor->flags |= SNOR_F_USE_CLSR; > - if (info->flags & SPI_NOR_SWP_IS_VOLATILE) > - nor->flags |= SNOR_F_SWP_IS_VOLATILE; > - > - if (info->flags & SPI_NOR_4BIT_BP) { > - nor->flags |= SNOR_F_HAS_4BIT_BP; > - if (info->flags & SPI_NOR_BP3_SR_BIT6) > - nor->flags |= SNOR_F_HAS_SR_BP3_BIT6; > - } > - > if (info->flags & SPI_NOR_NO_ERASE) > mtd->flags |= MTD_NO_ERASE; > > @@ -3175,9 +3194,6 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, > nor->page_size = nor->params->page_size; > mtd->writebufsize = nor->page_size; > > - if (of_property_read_bool(np, "broken-flash-reset")) > - nor->flags |= SNOR_F_BROKEN_RESET; > - As I pointed out above, I think this patch is certainly going in the right direction. We just need to be careful of the bugs that slip through. > /* > * Configure the SPI memory: > * - select op codes for (Fast) Read, Page Program and Sector Erase. > -- > 2.25.1 >
On 8/17/21 1:24 PM, Pratyush Yadav wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On 27/07/21 07:52AM, Tudor Ambarus wrote: >> Used to initialize the NOR flags for settings that are not defined >> in the JESD216 SFDP standard, thus can not be retrieved when parsing >> SFDP. No functional change. > > I am worried if the order in which these flags are set can cause some > subtle bugs. > > I can see one instance of it with SNOR_F_HAS_LOCK. > spi_nor_late_init_params() checks for SNOR_F_HAS_LOCK and if there are > no locking ops specified, it sets the default locking ops. This works > fine before this patch because the flag is set before the function is > called. But now, the flag will be set _after_ the function is called, > and so you will never be able to set the default flags. > > This is one bug I can spot but I fear some others might be hiding > somewhere as well. SPI NOR has accumulated a lot of spaghetti code over > the years and I certainly felt it when working on my Octal DTR series. > It caused an address width selection bug that was not obvious at all, > and was not even caught during the rc cycles. > > I think this series does clean up that spaghetti a lot. But you need to > be careful of such bugs. I think you should definitely let this series > cook in next for some time so it gets some exposure and hopefully some > testing. > >> >> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> >> --- >> drivers/mtd/spi-nor/core.c | 88 ++++++++++++++++++++++---------------- >> 1 file changed, 52 insertions(+), 36 deletions(-) >> >> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c >> index 1f38fa8ab2fa..6a8617346764 100644 >> --- a/drivers/mtd/spi-nor/core.c >> +++ b/drivers/mtd/spi-nor/core.c >> @@ -2687,6 +2687,56 @@ static void spi_nor_late_init_params(struct spi_nor *nor) >> spi_nor_init_default_locking_ops(nor); >> } >> >> +/** >> + * spi_nor_nonsfdp_flags_init() - Initialize NOR flags for settings that are not >> + * defined in the JESD216 SFDP standard, thus can not be retrieved when parsing >> + * SFDP. >> + * @nor: pointer to a 'struct spi_nor' >> + */ >> +static void spi_nor_nonsfdp_flags_init(struct spi_nor *nor) >> +{ >> + const struct flash_info *info = nor->info; >> + struct device_node *np = spi_nor_get_flash_node(nor); >> + >> + if (of_property_read_bool(np, "broken-flash-reset")) >> + nor->flags |= SNOR_F_BROKEN_RESET; >> + >> + if (info->flags & SPI_NOR_SWP_IS_VOLATILE) >> + nor->flags |= SNOR_F_SWP_IS_VOLATILE; >> + >> + if (info->flags & SPI_NOR_HAS_LOCK) >> + nor->flags |= SNOR_F_HAS_LOCK; > > As mentioned above, this would cause a bug. Good catch! > >> + >> + if (info->flags & SPI_NOR_HAS_TB) { >> + nor->flags |= SNOR_F_HAS_SR_TB; >> + if (info->flags & SPI_NOR_TB_SR_BIT6) >> + nor->flags |= SNOR_F_HAS_SR_TB_BIT6; >> + } >> + >> + if (info->flags & SPI_NOR_4BIT_BP) { >> + nor->flags |= SNOR_F_HAS_4BIT_BP; >> + if (info->flags & SPI_NOR_BP3_SR_BIT6) >> + nor->flags |= SNOR_F_HAS_SR_BP3_BIT6; >> + } >> + >> + if (info->flags & NO_CHIP_ERASE) >> + nor->flags |= SNOR_F_NO_OP_CHIP_ERASE; >> + >> + if (info->flags & USE_FSR) >> + nor->flags |= SNOR_F_USE_FSR; >> + >> + if (info->flags & USE_CLSR) >> + nor->flags |= SNOR_F_USE_CLSR; >> + >> + /* >> + * Make sure the XSR_RDY flag is set before calling >> + * spi_nor_wait_till_ready(). Xilinx S3AN share MFR >> + * with Atmel SPI NOR. >> + */ >> + if (info->flags & SPI_NOR_XSR_RDY) >> + nor->flags |= SNOR_F_READY_XSR_RDY; >> +} >> + >> /** >> * spi_nor_init_params() - Initialize the flash's parameters and settings. >> * @nor: pointer to a 'struct spi_nor'. >> @@ -2736,6 +2786,8 @@ static int spi_nor_init_params(struct spi_nor *nor) >> >> spi_nor_late_init_params(nor); >> >> + spi_nor_nonsfdp_flags_init(nor); >> + >> return 0; >> } >> >> @@ -3078,7 +3130,6 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, >> const struct flash_info *info; >> struct device *dev = nor->dev; >> struct mtd_info *mtd = &nor->mtd; >> - struct device_node *np = spi_nor_get_flash_node(nor); >> int ret; >> int i; >> >> @@ -3115,17 +3166,6 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, >> >> mutex_init(&nor->lock); >> >> - /* >> - * Make sure the XSR_RDY flag is set before calling >> - * spi_nor_wait_till_ready(). Xilinx S3AN share MFR >> - * with Atmel SPI NOR. >> - */ >> - if (info->flags & SPI_NOR_XSR_RDY) >> - nor->flags |= SNOR_F_READY_XSR_RDY; >> - >> - if (info->flags & SPI_NOR_HAS_LOCK) >> - nor->flags |= SNOR_F_HAS_LOCK; >> - >> mtd->_write = spi_nor_write; >> >> /* Init flash parameters based on flash_info struct and SFDP */ >> @@ -3147,27 +3187,6 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, >> mtd->_get_device = spi_nor_get_device; >> mtd->_put_device = spi_nor_put_device; >> >> - if (info->flags & USE_FSR) >> - nor->flags |= SNOR_F_USE_FSR; >> - if (info->flags & SPI_NOR_HAS_TB) { >> - nor->flags |= SNOR_F_HAS_SR_TB; >> - if (info->flags & SPI_NOR_TB_SR_BIT6) >> - nor->flags |= SNOR_F_HAS_SR_TB_BIT6; >> - } >> - >> - if (info->flags & NO_CHIP_ERASE) >> - nor->flags |= SNOR_F_NO_OP_CHIP_ERASE; >> - if (info->flags & USE_CLSR) >> - nor->flags |= SNOR_F_USE_CLSR; >> - if (info->flags & SPI_NOR_SWP_IS_VOLATILE) >> - nor->flags |= SNOR_F_SWP_IS_VOLATILE; >> - >> - if (info->flags & SPI_NOR_4BIT_BP) { >> - nor->flags |= SNOR_F_HAS_4BIT_BP; >> - if (info->flags & SPI_NOR_BP3_SR_BIT6) >> - nor->flags |= SNOR_F_HAS_SR_BP3_BIT6; >> - } >> - >> if (info->flags & SPI_NOR_NO_ERASE) >> mtd->flags |= MTD_NO_ERASE; >> >> @@ -3175,9 +3194,6 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, >> nor->page_size = nor->params->page_size; >> mtd->writebufsize = nor->page_size; >> >> - if (of_property_read_bool(np, "broken-flash-reset")) >> - nor->flags |= SNOR_F_BROKEN_RESET; >> - > > As I pointed out above, I think this patch is certainly going in the > right direction. We just need to be careful of the bugs that slip > through. Right, I'll self review all once I'll prepare v3. And I'll redo the testing, this time trying to cover all the flash info flags. Would be great if Michael, Vignesh and others can have a look on the series too. With 2 or 3 persons reviewing and better test coverage, we should be fine. Would be great if we can have these cleaning patches without introducing regressions, but if we introduce it's not a tragedy, we will fix all once reported. Cheers, ta
Am 2021-08-17 12:24, schrieb Pratyush Yadav: > On 27/07/21 07:52AM, Tudor Ambarus wrote: >> Used to initialize the NOR flags for settings that are not defined >> in the JESD216 SFDP standard, thus can not be retrieved when parsing >> SFDP. No functional change. > > I am worried if the order in which these flags are set can cause some > subtle bugs. > > I can see one instance of it with SNOR_F_HAS_LOCK. > spi_nor_late_init_params() checks for SNOR_F_HAS_LOCK and if there are > no locking ops specified, it sets the default locking ops. This works > fine before this patch because the flag is set before the function is > called. But now, the flag will be set _after_ the function is called, > and so you will never be able to set the default flags. Maybe we should just forbid to look at the SNOR_F_ flags in these functions. Instead the information could also be deduced by looking at the SPI_NOR_ flags. -michael
On 22/10/21 01:21PM, Michael Walle wrote: > Am 2021-08-17 12:24, schrieb Pratyush Yadav: > > On 27/07/21 07:52AM, Tudor Ambarus wrote: > > > Used to initialize the NOR flags for settings that are not defined > > > in the JESD216 SFDP standard, thus can not be retrieved when parsing > > > SFDP. No functional change. > > > > I am worried if the order in which these flags are set can cause some > > subtle bugs. > > > > I can see one instance of it with SNOR_F_HAS_LOCK. > > spi_nor_late_init_params() checks for SNOR_F_HAS_LOCK and if there are > > no locking ops specified, it sets the default locking ops. This works > > fine before this patch because the flag is set before the function is > > called. But now, the flag will be set _after_ the function is called, > > and so you will never be able to set the default flags. > > Maybe we should just forbid to look at the SNOR_F_ flags in these > functions. Instead the information could also be deduced by looking at > the SPI_NOR_ flags. TBH I never quite understood why we have two sets of flags in the first place, when the SNOR_F* flags pretty much mirror what SPI_NOR* flags mean. Dunno...
On 10/22/21 3:10 PM, Pratyush Yadav wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On 22/10/21 01:21PM, Michael Walle wrote: >> Am 2021-08-17 12:24, schrieb Pratyush Yadav: >>> On 27/07/21 07:52AM, Tudor Ambarus wrote: >>>> Used to initialize the NOR flags for settings that are not defined >>>> in the JESD216 SFDP standard, thus can not be retrieved when parsing >>>> SFDP. No functional change. >>> >>> I am worried if the order in which these flags are set can cause some >>> subtle bugs. >>> >>> I can see one instance of it with SNOR_F_HAS_LOCK. >>> spi_nor_late_init_params() checks for SNOR_F_HAS_LOCK and if there are >>> no locking ops specified, it sets the default locking ops. This works >>> fine before this patch because the flag is set before the function is >>> called. But now, the flag will be set _after_ the function is called, >>> and so you will never be able to set the default flags. >> >> Maybe we should just forbid to look at the SNOR_F_ flags in these >> functions. Instead the information could also be deduced by looking at >> the SPI_NOR_ flags. not true. > > TBH I never quite understood why we have two sets of flags in the first > place, when the SNOR_F* flags pretty much mirror what SPI_NOR* flags > mean. Dunno... > struct spi_nor { cut const struct flash_info *info; cut } const, which is the way it should be. Every flag that is discovered at SFDP time will fill the SNOR_F correspondent, and only SNOR_F will be used across the core driver.
Am 2021-10-22 14:42, schrieb Tudor.Ambarus@microchip.com: > On 10/22/21 3:10 PM, Pratyush Yadav wrote: >> EXTERNAL EMAIL: Do not click links or open attachments unless you know >> the content is safe >> >> On 22/10/21 01:21PM, Michael Walle wrote: >>> Am 2021-08-17 12:24, schrieb Pratyush Yadav: >>>> On 27/07/21 07:52AM, Tudor Ambarus wrote: >>>>> Used to initialize the NOR flags for settings that are not defined >>>>> in the JESD216 SFDP standard, thus can not be retrieved when >>>>> parsing >>>>> SFDP. No functional change. >>>> >>>> I am worried if the order in which these flags are set can cause >>>> some >>>> subtle bugs. >>>> >>>> I can see one instance of it with SNOR_F_HAS_LOCK. >>>> spi_nor_late_init_params() checks for SNOR_F_HAS_LOCK and if there >>>> are >>>> no locking ops specified, it sets the default locking ops. This >>>> works >>>> fine before this patch because the flag is set before the function >>>> is >>>> called. But now, the flag will be set _after_ the function is >>>> called, >>>> and so you will never be able to set the default flags. >>> >>> Maybe we should just forbid to look at the SNOR_F_ flags in these >>> functions. Instead the information could also be deduced by looking >>> at >>> the SPI_NOR_ flags. > > not true. I guess you mean that some init flash init functions might set it, too. Right? IMHO this goes into the spaghetti code direction again. spi_nor_late_init_params() must not look at the SNOR_F_ flags. There are too much inter-function dependencies and it is really hard to follow the call chain. The locking ops should also go into the (static) flash init which only depends on the SPI_NOR_ flags. If SNOR_F_HAS_LOCK will be added at in an init function, then this code should also take care of setting the correct locking ops. Maybe both can be set together with a helper. -michael
On 10/22/21 3:59 PM, Michael Walle wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > Am 2021-10-22 14:42, schrieb Tudor.Ambarus@microchip.com: >> On 10/22/21 3:10 PM, Pratyush Yadav wrote: >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know >>> the content is safe >>> >>> On 22/10/21 01:21PM, Michael Walle wrote: >>>> Am 2021-08-17 12:24, schrieb Pratyush Yadav: >>>>> On 27/07/21 07:52AM, Tudor Ambarus wrote: >>>>>> Used to initialize the NOR flags for settings that are not defined >>>>>> in the JESD216 SFDP standard, thus can not be retrieved when >>>>>> parsing >>>>>> SFDP. No functional change. >>>>> >>>>> I am worried if the order in which these flags are set can cause >>>>> some >>>>> subtle bugs. >>>>> >>>>> I can see one instance of it with SNOR_F_HAS_LOCK. >>>>> spi_nor_late_init_params() checks for SNOR_F_HAS_LOCK and if there >>>>> are >>>>> no locking ops specified, it sets the default locking ops. This >>>>> works >>>>> fine before this patch because the flag is set before the function >>>>> is >>>>> called. But now, the flag will be set _after_ the function is >>>>> called, >>>>> and so you will never be able to set the default flags. >>>> >>>> Maybe we should just forbid to look at the SNOR_F_ flags in these >>>> functions. Instead the information could also be deduced by looking >>>> at >>>> the SPI_NOR_ flags. >> >> not true. > > I guess you mean that some init flash init functions might set it, too. > Right? IMHO this goes into the spaghetti code direction again. > spi_nor_late_init_params() must not look at the SNOR_F_ flags. There > are too much inter-function dependencies and it is really hard to > follow the call chain. I don't think I understand what you are referring to. The flash_info flags are used just to set their SNOR_F correspondents. Take SNOR_F_4B_OPCODES for example. A flash that can't discover 4b opcodes support when parsing SFDP, will set the SPI_NOR_4B_OPCODES flash_info flag. Flashes that can discover it, will ignore/not set the SPI_NOR_4B_OPCODES flag, and let the SFDP do its thing: the SFDP code will set the SNOR_F_4B_OPCODES flag. The only flags that are used across the SPI NOR core are the nor->flags (SNOR_F). > > The locking ops should also go into the (static) flash init which only > depends on the SPI_NOR_ flags. If SNOR_F_HAS_LOCK will be added at we don't have to specify the locking methods, we have generic ones. A single flag is enough. > in an init function, then this code should also take care of setting what init function? We're just setting some flags. > the correct locking ops. Maybe both can be set together with a helper.
Am 2021-10-22 15:25, schrieb Tudor.Ambarus@microchip.com: > On 10/22/21 3:59 PM, Michael Walle wrote: >> EXTERNAL EMAIL: Do not click links or open attachments unless you know >> the content is safe >> >> Am 2021-10-22 14:42, schrieb Tudor.Ambarus@microchip.com: >>> On 10/22/21 3:10 PM, Pratyush Yadav wrote: >>>> EXTERNAL EMAIL: Do not click links or open attachments unless you >>>> know >>>> the content is safe >>>> >>>> On 22/10/21 01:21PM, Michael Walle wrote: >>>>> Am 2021-08-17 12:24, schrieb Pratyush Yadav: >>>>>> On 27/07/21 07:52AM, Tudor Ambarus wrote: >>>>>>> Used to initialize the NOR flags for settings that are not >>>>>>> defined >>>>>>> in the JESD216 SFDP standard, thus can not be retrieved when >>>>>>> parsing >>>>>>> SFDP. No functional change. >>>>>> >>>>>> I am worried if the order in which these flags are set can cause >>>>>> some >>>>>> subtle bugs. >>>>>> >>>>>> I can see one instance of it with SNOR_F_HAS_LOCK. >>>>>> spi_nor_late_init_params() checks for SNOR_F_HAS_LOCK and if there >>>>>> are >>>>>> no locking ops specified, it sets the default locking ops. This >>>>>> works >>>>>> fine before this patch because the flag is set before the function >>>>>> is >>>>>> called. But now, the flag will be set _after_ the function is >>>>>> called, >>>>>> and so you will never be able to set the default flags. >>>>> >>>>> Maybe we should just forbid to look at the SNOR_F_ flags in these >>>>> functions. Instead the information could also be deduced by looking >>>>> at >>>>> the SPI_NOR_ flags. >>> >>> not true. >> >> I guess you mean that some init flash init functions might set it, >> too. >> Right? IMHO this goes into the spaghetti code direction again. >> spi_nor_late_init_params() must not look at the SNOR_F_ flags. There >> are too much inter-function dependencies and it is really hard to >> follow the call chain. > > I don't think I understand what you are referring to. The flash_info > flags > are used just to set their SNOR_F correspondents. Take > SNOR_F_4B_OPCODES for > example. A flash that can't discover 4b opcodes support when parsing > SFDP, > will set the SPI_NOR_4B_OPCODES flash_info flag. Flashes that can > discover it, > will ignore/not set the SPI_NOR_4B_OPCODES flag, and let the SFDP do > its thing: > the SFDP code will set the SNOR_F_4B_OPCODES flag. The only flags that > are used > across the SPI NOR core are the nor->flags (SNOR_F). Sorry, I wasn't clear enough. I was talking about checking (already set) SNOR_F flags in all these init functions ({flash,manufacturer)->post_sfdp(), (flash,manufacturer)->late_init, ..). If we didn't allow this, then we would avoid all these "subtle bugs" which happen because some of these functions depend on another being called first. I.e. all the called functions within spi_nor_init_params() may only add SNOR_F flags, but must not actually use them. [I see that spi_nor_sfdp_init_params() will remove SNOR_F_4B_OPCODES. Mhh.] Btw. I wonder what is the difference between a member being in "struct spi_nor" and being in "struct spi_nor_flash_parameter". Apparently, it should contain the properties which can be set/changed in the fixups or by parsing the SFPD. But then there are also the flags which are also changed in the fixups. Maybe we should pass the "struct nor" as const and a second parameter "struct spi_nor_flash_parameter *params" which can be updated by the called function. This way we can be sure the functions won't change anything else. I don't suggest to do that right now, just to start a discussion. >> The locking ops should also go into the (static) flash init which only >> depends on the SPI_NOR_ flags. If SNOR_F_HAS_LOCK will be added at > > we don't have to specify the locking methods, we have generic ones. A > single > flag is enough. Hm, I'm not really sure we need that SNOR_F_HAS_LOCK flag at all. "if (nor->params->locking_ops)" should be the same. At least the current code which checks the info->flags for SPI_NOR_HAS_LOCK will set the default locking ops instead of just setting the SNOR_F_HAS_LOCK. >> in an init function, then this code should also take care of setting > > what init function? We're just setting some flags. all the functions called in spi_nor_init_params(). >> the correct locking ops. Maybe both can be set together with a helper. -michael
On 10/24/21 8:05 PM, Michael Walle wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > Am 2021-10-22 15:25, schrieb Tudor.Ambarus@microchip.com: >> On 10/22/21 3:59 PM, Michael Walle wrote: >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know >>> the content is safe >>> >>> Am 2021-10-22 14:42, schrieb Tudor.Ambarus@microchip.com: >>>> On 10/22/21 3:10 PM, Pratyush Yadav wrote: >>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you >>>>> know >>>>> the content is safe >>>>> >>>>> On 22/10/21 01:21PM, Michael Walle wrote: >>>>>> Am 2021-08-17 12:24, schrieb Pratyush Yadav: >>>>>>> On 27/07/21 07:52AM, Tudor Ambarus wrote: >>>>>>>> Used to initialize the NOR flags for settings that are not >>>>>>>> defined >>>>>>>> in the JESD216 SFDP standard, thus can not be retrieved when >>>>>>>> parsing >>>>>>>> SFDP. No functional change. >>>>>>> >>>>>>> I am worried if the order in which these flags are set can cause >>>>>>> some >>>>>>> subtle bugs. >>>>>>> >>>>>>> I can see one instance of it with SNOR_F_HAS_LOCK. >>>>>>> spi_nor_late_init_params() checks for SNOR_F_HAS_LOCK and if there >>>>>>> are >>>>>>> no locking ops specified, it sets the default locking ops. This >>>>>>> works >>>>>>> fine before this patch because the flag is set before the function >>>>>>> is >>>>>>> called. But now, the flag will be set _after_ the function is >>>>>>> called, >>>>>>> and so you will never be able to set the default flags. >>>>>> >>>>>> Maybe we should just forbid to look at the SNOR_F_ flags in these >>>>>> functions. Instead the information could also be deduced by looking >>>>>> at >>>>>> the SPI_NOR_ flags. >>>> >>>> not true. >>> >>> I guess you mean that some init flash init functions might set it, >>> too. >>> Right? IMHO this goes into the spaghetti code direction again. >>> spi_nor_late_init_params() must not look at the SNOR_F_ flags. There >>> are too much inter-function dependencies and it is really hard to >>> follow the call chain. >> >> I don't think I understand what you are referring to. The flash_info >> flags >> are used just to set their SNOR_F correspondents. Take >> SNOR_F_4B_OPCODES for >> example. A flash that can't discover 4b opcodes support when parsing >> SFDP, >> will set the SPI_NOR_4B_OPCODES flash_info flag. Flashes that can >> discover it, >> will ignore/not set the SPI_NOR_4B_OPCODES flag, and let the SFDP do >> its thing: >> the SFDP code will set the SNOR_F_4B_OPCODES flag. The only flags that >> are used >> across the SPI NOR core are the nor->flags (SNOR_F). > > Sorry, I wasn't clear enough. I was talking about checking (already set) > SNOR_F > flags in all these init functions ({flash,manufacturer)->post_sfdp(), > (flash,manufacturer)->late_init, ..). If we didn't allow this, then > we would avoid all these "subtle bugs" which happen because some of > these > functions depend on another being called first. I.e. all the called > functions within spi_nor_init_params() may only add SNOR_F flags, but > must > not actually use them. [I see that spi_nor_sfdp_init_params() will > remove SNOR_F_4B_OPCODES. Mhh.] Keeping the setting of SNOR_F flags based on the flash_info flags as late as we can assures the reader that SNOR_F flags are not used across the init params chain. It also indicates that the flash can't set the SNOR_F capability by parsing SFDP, so it needed explicit flags at declaration. It is clearer who sets and when. > > Btw. I wonder what is the difference between a member being in "struct > spi_nor" > and being in "struct spi_nor_flash_parameter". Apparently, it should > contain the properties which can be set/changed in the fixups or by > parsing the SFPD. But then there are also the flags which are also > changed in the fixups. > > Maybe we should pass the "struct nor" as const and a second parameter > "struct spi_nor_flash_parameter *params" which can be updated by the > called function. This way we can be sure the functions won't change > anything else. I don't suggest to do that right now, just to start > a discussion. I'll let this for other time, maybe a separate thread. > >>> The locking ops should also go into the (static) flash init which only >>> depends on the SPI_NOR_ flags. If SNOR_F_HAS_LOCK will be added at >> >> we don't have to specify the locking methods, we have generic ones. A >> single >> flag is enough. > > Hm, I'm not really sure we need that SNOR_F_HAS_LOCK flag at all. > "if (nor->params->locking_ops)" should be the same. At least the current > code > which checks the info->flags for SPI_NOR_HAS_LOCK will set the default > locking ops instead of just setting the SNOR_F_HAS_LOCK. SNOR_F_HAS_LOCK is kept for consistency. The core driver operates just on SNOR_F flags. Here it's the same idea, I would like to set the locking ops as late in the init chain as possible, to indicate that the locking ops are not needed earlier. > >>> in an init function, then this code should also take care of setting >> >> what init function? We're just setting some flags. > > all the functions called in spi_nor_init_params(). > >>> the correct locking ops. Maybe both can be set together with a helper. > > -michael
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c index 1f38fa8ab2fa..6a8617346764 100644 --- a/drivers/mtd/spi-nor/core.c +++ b/drivers/mtd/spi-nor/core.c @@ -2687,6 +2687,56 @@ static void spi_nor_late_init_params(struct spi_nor *nor) spi_nor_init_default_locking_ops(nor); } +/** + * spi_nor_nonsfdp_flags_init() - Initialize NOR flags for settings that are not + * defined in the JESD216 SFDP standard, thus can not be retrieved when parsing + * SFDP. + * @nor: pointer to a 'struct spi_nor' + */ +static void spi_nor_nonsfdp_flags_init(struct spi_nor *nor) +{ + const struct flash_info *info = nor->info; + struct device_node *np = spi_nor_get_flash_node(nor); + + if (of_property_read_bool(np, "broken-flash-reset")) + nor->flags |= SNOR_F_BROKEN_RESET; + + if (info->flags & SPI_NOR_SWP_IS_VOLATILE) + nor->flags |= SNOR_F_SWP_IS_VOLATILE; + + if (info->flags & SPI_NOR_HAS_LOCK) + nor->flags |= SNOR_F_HAS_LOCK; + + if (info->flags & SPI_NOR_HAS_TB) { + nor->flags |= SNOR_F_HAS_SR_TB; + if (info->flags & SPI_NOR_TB_SR_BIT6) + nor->flags |= SNOR_F_HAS_SR_TB_BIT6; + } + + if (info->flags & SPI_NOR_4BIT_BP) { + nor->flags |= SNOR_F_HAS_4BIT_BP; + if (info->flags & SPI_NOR_BP3_SR_BIT6) + nor->flags |= SNOR_F_HAS_SR_BP3_BIT6; + } + + if (info->flags & NO_CHIP_ERASE) + nor->flags |= SNOR_F_NO_OP_CHIP_ERASE; + + if (info->flags & USE_FSR) + nor->flags |= SNOR_F_USE_FSR; + + if (info->flags & USE_CLSR) + nor->flags |= SNOR_F_USE_CLSR; + + /* + * Make sure the XSR_RDY flag is set before calling + * spi_nor_wait_till_ready(). Xilinx S3AN share MFR + * with Atmel SPI NOR. + */ + if (info->flags & SPI_NOR_XSR_RDY) + nor->flags |= SNOR_F_READY_XSR_RDY; +} + /** * spi_nor_init_params() - Initialize the flash's parameters and settings. * @nor: pointer to a 'struct spi_nor'. @@ -2736,6 +2786,8 @@ static int spi_nor_init_params(struct spi_nor *nor) spi_nor_late_init_params(nor); + spi_nor_nonsfdp_flags_init(nor); + return 0; } @@ -3078,7 +3130,6 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, const struct flash_info *info; struct device *dev = nor->dev; struct mtd_info *mtd = &nor->mtd; - struct device_node *np = spi_nor_get_flash_node(nor); int ret; int i; @@ -3115,17 +3166,6 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, mutex_init(&nor->lock); - /* - * Make sure the XSR_RDY flag is set before calling - * spi_nor_wait_till_ready(). Xilinx S3AN share MFR - * with Atmel SPI NOR. - */ - if (info->flags & SPI_NOR_XSR_RDY) - nor->flags |= SNOR_F_READY_XSR_RDY; - - if (info->flags & SPI_NOR_HAS_LOCK) - nor->flags |= SNOR_F_HAS_LOCK; - mtd->_write = spi_nor_write; /* Init flash parameters based on flash_info struct and SFDP */ @@ -3147,27 +3187,6 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, mtd->_get_device = spi_nor_get_device; mtd->_put_device = spi_nor_put_device; - if (info->flags & USE_FSR) - nor->flags |= SNOR_F_USE_FSR; - if (info->flags & SPI_NOR_HAS_TB) { - nor->flags |= SNOR_F_HAS_SR_TB; - if (info->flags & SPI_NOR_TB_SR_BIT6) - nor->flags |= SNOR_F_HAS_SR_TB_BIT6; - } - - if (info->flags & NO_CHIP_ERASE) - nor->flags |= SNOR_F_NO_OP_CHIP_ERASE; - if (info->flags & USE_CLSR) - nor->flags |= SNOR_F_USE_CLSR; - if (info->flags & SPI_NOR_SWP_IS_VOLATILE) - nor->flags |= SNOR_F_SWP_IS_VOLATILE; - - if (info->flags & SPI_NOR_4BIT_BP) { - nor->flags |= SNOR_F_HAS_4BIT_BP; - if (info->flags & SPI_NOR_BP3_SR_BIT6) - nor->flags |= SNOR_F_HAS_SR_BP3_BIT6; - } - if (info->flags & SPI_NOR_NO_ERASE) mtd->flags |= MTD_NO_ERASE; @@ -3175,9 +3194,6 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, nor->page_size = nor->params->page_size; mtd->writebufsize = nor->page_size; - if (of_property_read_bool(np, "broken-flash-reset")) - nor->flags |= SNOR_F_BROKEN_RESET; - /* * Configure the SPI memory: * - select op codes for (Fast) Read, Page Program and Sector Erase.
Used to initialize the NOR flags for settings that are not defined in the JESD216 SFDP standard, thus can not be retrieved when parsing SFDP. No functional change. Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> --- drivers/mtd/spi-nor/core.c | 88 ++++++++++++++++++++++---------------- 1 file changed, 52 insertions(+), 36 deletions(-)