| Message ID | 20240412134405.381832-7-mwalle@kernel.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | mtd: spi-nor: spring cleaning | expand |
On 4/12/24 14:44, Michael Walle wrote:
> Therefore, introduce a (temporary) flag to display a message on boot if
Fine by me!
On Fri, Apr 12 2024, Michael Walle wrote: > SPI-NOR will automatically detect the attached flash device most of the > time. We cannot easily find out if boards are using a given flash. > Therefore, introduce a (temporary) flag to display a message on boot if Why temporary? There will always be a need to deprecate one flash or another. Might as well keep the flag around. Also, this patch/series does not add any users of the deprecated flag. If you have some flashes in mind, it would be good to add them to the patch/series. I like the idea in general. Do you think we should also print a rough date for the deprecation as well? > support for a given flash device is scheduled to be removed in the > future. > > Signed-off-by: Michael Walle <mwalle@kernel.org> > --- > drivers/mtd/spi-nor/core.c | 12 ++++++++++++ > drivers/mtd/spi-nor/core.h | 1 + > 2 files changed, 13 insertions(+) > > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c > index 58d310427d35..a294eef2e34a 100644 > --- a/drivers/mtd/spi-nor/core.c > +++ b/drivers/mtd/spi-nor/core.c > @@ -3312,6 +3312,7 @@ static const struct flash_info *spi_nor_get_flash_info(struct spi_nor *nor, > const char *name) > { > const struct flash_info *jinfo = NULL, *info = NULL; > + const char *deprecated = NULL; > > if (name) > info = spi_nor_match_name(nor, name); > @@ -3326,6 +3327,17 @@ static const struct flash_info *spi_nor_get_flash_info(struct spi_nor *nor, > return jinfo; > } > > + if (info && (info->flags & SPI_NOR_DEPRECATED)) > + deprecated = info->name; > + else if (jinfo && (jinfo->flags & SPI_NOR_DEPRECATED)) > + deprecated = jinfo->name; > + > + if (deprecated) > + pr_warn("Your board or device tree is using a SPI NOR flash (%s) with\n" > + "deprecated driver support. It will be removed in future kernel\n" Nit: "removed in a future kernel version" > + "version. If you feel this shouldn't be the case, please contact\n" > + "us at linux-mtd@lists.infradead.org\n", deprecated); > + Hmm, this isn't so nice. I'd suggest doing something like: /* * If caller has specified name of flash model that can normally be * ... */ info = jinfo ?: info; if (info->flags & SPI_NOR_DEPRECATED) pr_warn(...); return info; > /* > * If caller has specified name of flash model that can normally be > * detected using JEDEC, let's verify it. > diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h > index 8552e31b1b07..0317d8e253f4 100644 > --- a/drivers/mtd/spi-nor/core.h > +++ b/drivers/mtd/spi-nor/core.h > @@ -524,6 +524,7 @@ struct flash_info { > #define SPI_NOR_NO_ERASE BIT(6) > #define SPI_NOR_QUAD_PP BIT(8) > #define SPI_NOR_RWW BIT(9) > +#define SPI_NOR_DEPRECATED BIT(15) If you do agree with my suggestion of making it permanent, would it make more sense to make it BIT(10) instead. Or BIT(9) once you move up the others because we no longer have BIT(7). > > u8 no_sfdp_flags; > #define SPI_NOR_SKIP_SFDP BIT(0)
Hi, On Wed Apr 17, 2024 at 4:36 PM CEST, Pratyush Yadav wrote: > On Fri, Apr 12 2024, Michael Walle wrote: > > > SPI-NOR will automatically detect the attached flash device most of the > > time. We cannot easily find out if boards are using a given flash. > > Therefore, introduce a (temporary) flag to display a message on boot if > > Why temporary? There will always be a need to deprecate one flash or > another. Might as well keep the flag around. Mh, yes I agree. That also means that this flag will not have any users (most) of the time (hopefully ;) ). > Also, this patch/series does not add any users of the deprecated flag. > If you have some flashes in mind, it would be good to add them to the > patch/series. This is just an RFC to see if whether you Tudor agree with me :) But I was about to add it to the evervision/cypress FRAMs. > I like the idea in general. Do you think we should also print a rough > date for the deprecation as well? Might make sense, any suggestions? > > support for a given flash device is scheduled to be removed in the > > future. > > > > Signed-off-by: Michael Walle <mwalle@kernel.org> > > --- > > drivers/mtd/spi-nor/core.c | 12 ++++++++++++ > > drivers/mtd/spi-nor/core.h | 1 + > > 2 files changed, 13 insertions(+) > > > > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c > > index 58d310427d35..a294eef2e34a 100644 > > --- a/drivers/mtd/spi-nor/core.c > > +++ b/drivers/mtd/spi-nor/core.c > > @@ -3312,6 +3312,7 @@ static const struct flash_info *spi_nor_get_flash_info(struct spi_nor *nor, > > const char *name) > > { > > const struct flash_info *jinfo = NULL, *info = NULL; > > + const char *deprecated = NULL; > > > > if (name) > > info = spi_nor_match_name(nor, name); > > @@ -3326,6 +3327,17 @@ static const struct flash_info *spi_nor_get_flash_info(struct spi_nor *nor, > > return jinfo; > > } > > > > + if (info && (info->flags & SPI_NOR_DEPRECATED)) > > + deprecated = info->name; > > + else if (jinfo && (jinfo->flags & SPI_NOR_DEPRECATED)) > > + deprecated = jinfo->name; > > + > > + if (deprecated) > > + pr_warn("Your board or device tree is using a SPI NOR flash (%s) with\n" > > + "deprecated driver support. It will be removed in future kernel\n" > > Nit: "removed in a future kernel version" > > > + "version. If you feel this shouldn't be the case, please contact\n" > > + "us at linux-mtd@lists.infradead.org\n", deprecated); > > + > > Hmm, this isn't so nice. I'd suggest doing something like: > > /* > * If caller has specified name of flash model that can normally be > * ... > */ > info = jinfo ?: info; > > if (info->flags & SPI_NOR_DEPRECATED) > pr_warn(...); Actually, I had that, *but* I was thinking we might only check the detected flash and not the one specified in the device tree. But thinking about that again, I guess it makes sense because: - that's the actually used flash driver - having jinfo != info will print that other warning, thus this case is hopefully very unlikely. > > return info; > > > /* > > * If caller has specified name of flash model that can normally be > > * detected using JEDEC, let's verify it. > > diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h > > index 8552e31b1b07..0317d8e253f4 100644 > > --- a/drivers/mtd/spi-nor/core.h > > +++ b/drivers/mtd/spi-nor/core.h > > @@ -524,6 +524,7 @@ struct flash_info { > > #define SPI_NOR_NO_ERASE BIT(6) > > #define SPI_NOR_QUAD_PP BIT(8) > > #define SPI_NOR_RWW BIT(9) > > +#define SPI_NOR_DEPRECATED BIT(15) > > If you do agree with my suggestion of making it permanent, would it make > more sense to make it BIT(10) instead. Or BIT(9) once you move up the > others because we no longer have BIT(7). Or just BIT(7) and avoid any code churn :) -michael > > > > > u8 no_sfdp_flags; > > #define SPI_NOR_SKIP_SFDP BIT(0)
On Wed, Apr 17 2024, Michael Walle wrote: > Hi, > > On Wed Apr 17, 2024 at 4:36 PM CEST, Pratyush Yadav wrote: >> On Fri, Apr 12 2024, Michael Walle wrote: >> >> > SPI-NOR will automatically detect the attached flash device most of the >> > time. We cannot easily find out if boards are using a given flash. >> > Therefore, introduce a (temporary) flag to display a message on boot if >> >> Why temporary? There will always be a need to deprecate one flash or >> another. Might as well keep the flag around. > > Mh, yes I agree. That also means that this flag will not have any > users (most) of the time (hopefully ;) ). > >> Also, this patch/series does not add any users of the deprecated flag. >> If you have some flashes in mind, it would be good to add them to the >> patch/series. > > This is just an RFC to see if whether you Tudor agree with me :) But > I was about to add it to the evervision/cypress FRAMs. > >> I like the idea in general. Do you think we should also print a rough >> date for the deprecation as well? > > Might make sense, any suggestions? How about a simple string to flash_info? /** * struct flash_info - SPI NOR flash_info entry. * @id: pointer to struct spi_nor_id or NULL, which means "no ID" (mostly * older chips). * @name: (obsolete) the name of the flash. Do not set it for new additions. * @size: the size of the flash in bytes. * @deprecation_date: The date after which the support for this flash will be * removed. * [...] */ struct flash_info { char *name; const struct spi_nor_id *id; char *deprecation_date; [...] } And then in everspin.c for example, { .name = "mr25h128", .size = SZ_16K, .sector_size = SZ_16K, .addr_nbytes = 2, .flags = SPI_NOR_NO_ERASE | SPI_NOR_NO_FR, .deprecation_date = "2025-01-01", }, { And in spi_nor_get_flash_info() (changed some wording of the message): info = jinfo ?: info; if (info->deprecation_date) pr_warn("Your board or device tree is using a SPI NOR flash (%s) with\n" "deprecated driver support. It can be removed in any kernel\n" "version after %s. If you feel this shouldn't be the case, please contact\n" "us at linux-mtd@lists.infradead.org\n", info->name, info->deprecation_date); return info; This would also remove the need for SPI_NOR_DEPRECATED. But it would make the flash_info 4 or 8 bytes larger. What do you think? > >> > support for a given flash device is scheduled to be removed in the >> > future. >> > >> > Signed-off-by: Michael Walle <mwalle@kernel.org> >> > --- >> > drivers/mtd/spi-nor/core.c | 12 ++++++++++++ >> > drivers/mtd/spi-nor/core.h | 1 + >> > 2 files changed, 13 insertions(+) >> > >> > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c >> > index 58d310427d35..a294eef2e34a 100644 >> > --- a/drivers/mtd/spi-nor/core.c >> > +++ b/drivers/mtd/spi-nor/core.c >> > @@ -3312,6 +3312,7 @@ static const struct flash_info *spi_nor_get_flash_info(struct spi_nor *nor, >> > const char *name) >> > { >> > const struct flash_info *jinfo = NULL, *info = NULL; >> > + const char *deprecated = NULL; >> > >> > if (name) >> > info = spi_nor_match_name(nor, name); >> > @@ -3326,6 +3327,17 @@ static const struct flash_info *spi_nor_get_flash_info(struct spi_nor *nor, >> > return jinfo; >> > } >> > >> > + if (info && (info->flags & SPI_NOR_DEPRECATED)) >> > + deprecated = info->name; >> > + else if (jinfo && (jinfo->flags & SPI_NOR_DEPRECATED)) >> > + deprecated = jinfo->name; >> > + >> > + if (deprecated) >> > + pr_warn("Your board or device tree is using a SPI NOR flash (%s) with\n" >> > + "deprecated driver support. It will be removed in future kernel\n" >> >> Nit: "removed in a future kernel version" >> >> > + "version. If you feel this shouldn't be the case, please contact\n" >> > + "us at linux-mtd@lists.infradead.org\n", deprecated); >> > + >> >> Hmm, this isn't so nice. I'd suggest doing something like: >> >> /* >> * If caller has specified name of flash model that can normally be >> * ... >> */ >> info = jinfo ?: info; >> >> if (info->flags & SPI_NOR_DEPRECATED) >> pr_warn(...); > > Actually, I had that, *but* I was thinking we might only check the > detected flash and not the one specified in the device tree. But > thinking about that again, I guess it makes sense because: > - that's the actually used flash driver > - having jinfo != info will print that other warning, thus this > case is hopefully very unlikely. > >> >> return info; >> >> > /* >> > * If caller has specified name of flash model that can normally be >> > * detected using JEDEC, let's verify it. >> > diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h >> > index 8552e31b1b07..0317d8e253f4 100644 >> > --- a/drivers/mtd/spi-nor/core.h >> > +++ b/drivers/mtd/spi-nor/core.h >> > @@ -524,6 +524,7 @@ struct flash_info { >> > #define SPI_NOR_NO_ERASE BIT(6) >> > #define SPI_NOR_QUAD_PP BIT(8) >> > #define SPI_NOR_RWW BIT(9) >> > +#define SPI_NOR_DEPRECATED BIT(15) >> >> If you do agree with my suggestion of making it permanent, would it make >> more sense to make it BIT(10) instead. Or BIT(9) once you move up the >> others because we no longer have BIT(7). > > Or just BIT(7) and avoid any code churn :) Yep, that works. > > -michael > >> >> > >> > u8 no_sfdp_flags; >> > #define SPI_NOR_SKIP_SFDP BIT(0) >
Hi, On Wed Apr 17, 2024 at 5:52 PM CEST, Pratyush Yadav wrote: > On Wed, Apr 17 2024, Michael Walle wrote: > > On Wed Apr 17, 2024 at 4:36 PM CEST, Pratyush Yadav wrote: > >> On Fri, Apr 12 2024, Michael Walle wrote: > >> > >> > SPI-NOR will automatically detect the attached flash device most of the > >> > time. We cannot easily find out if boards are using a given flash. > >> > Therefore, introduce a (temporary) flag to display a message on boot if > >> > >> Why temporary? There will always be a need to deprecate one flash or > >> another. Might as well keep the flag around. > > > > Mh, yes I agree. That also means that this flag will not have any > > users (most) of the time (hopefully ;) ). > > > >> Also, this patch/series does not add any users of the deprecated flag. > >> If you have some flashes in mind, it would be good to add them to the > >> patch/series. > > > > This is just an RFC to see if whether you Tudor agree with me :) But > > I was about to add it to the evervision/cypress FRAMs. > > > >> I like the idea in general. Do you think we should also print a rough > >> date for the deprecation as well? > > > > Might make sense, any suggestions? > > How about a simple string to flash_info? Ahh, I was rather asking if you already had a time frame in mind. Besides that, should it be a date or a (future) kernel version? Roughly about two/three kernel releases? > /** > * struct flash_info - SPI NOR flash_info entry. > * @id: pointer to struct spi_nor_id or NULL, which means "no ID" (mostly > * older chips). > * @name: (obsolete) the name of the flash. Do not set it for new additions. > * @size: the size of the flash in bytes. > * @deprecation_date: The date after which the support for this flash will be > * removed. > * [...] > */ > struct flash_info { > char *name; > const struct spi_nor_id *id; > char *deprecation_date; > [...] > } > > And then in everspin.c for example, > > { > .name = "mr25h128", > .size = SZ_16K, > .sector_size = SZ_16K, > .addr_nbytes = 2, > .flags = SPI_NOR_NO_ERASE | SPI_NOR_NO_FR, > .deprecation_date = "2025-01-01", > }, { > > And in spi_nor_get_flash_info() (changed some wording of the message): > > info = jinfo ?: info; > > if (info->deprecation_date) > pr_warn("Your board or device tree is using a SPI NOR flash (%s) with\n" > "deprecated driver support. It can be removed in any kernel\n" > "version after %s. If you feel this shouldn't be the case, please contact\n" > "us at linux-mtd@lists.infradead.org\n", info->name, > info->deprecation_date); > > return info; > > This would also remove the need for SPI_NOR_DEPRECATED. But it would > make the flash_info 4 or 8 bytes larger. > > What do you think? Looks good. -michael
On Thu, Apr 18 2024, Michael Walle wrote: > Hi, > > On Wed Apr 17, 2024 at 5:52 PM CEST, Pratyush Yadav wrote: >> On Wed, Apr 17 2024, Michael Walle wrote: >> > On Wed Apr 17, 2024 at 4:36 PM CEST, Pratyush Yadav wrote: [...] >> >> I like the idea in general. Do you think we should also print a rough >> >> date for the deprecation as well? >> > >> > Might make sense, any suggestions? >> >> How about a simple string to flash_info? > > Ahh, I was rather asking if you already had a time frame in mind. > > Besides that, should it be a date or a (future) kernel version? > Roughly about two/three kernel releases? I think kernel version is better. I don't base this on any real data, only on gut feeling, but I think three kernel releases is a good time. Should add up to about 6-7 months. [...]
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c index 58d310427d35..a294eef2e34a 100644 --- a/drivers/mtd/spi-nor/core.c +++ b/drivers/mtd/spi-nor/core.c @@ -3312,6 +3312,7 @@ static const struct flash_info *spi_nor_get_flash_info(struct spi_nor *nor, const char *name) { const struct flash_info *jinfo = NULL, *info = NULL; + const char *deprecated = NULL; if (name) info = spi_nor_match_name(nor, name); @@ -3326,6 +3327,17 @@ static const struct flash_info *spi_nor_get_flash_info(struct spi_nor *nor, return jinfo; } + if (info && (info->flags & SPI_NOR_DEPRECATED)) + deprecated = info->name; + else if (jinfo && (jinfo->flags & SPI_NOR_DEPRECATED)) + deprecated = jinfo->name; + + if (deprecated) + pr_warn("Your board or device tree is using a SPI NOR flash (%s) with\n" + "deprecated driver support. It will be removed in future kernel\n" + "version. If you feel this shouldn't be the case, please contact\n" + "us at linux-mtd@lists.infradead.org\n", deprecated); + /* * If caller has specified name of flash model that can normally be * detected using JEDEC, let's verify it. diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h index 8552e31b1b07..0317d8e253f4 100644 --- a/drivers/mtd/spi-nor/core.h +++ b/drivers/mtd/spi-nor/core.h @@ -524,6 +524,7 @@ struct flash_info { #define SPI_NOR_NO_ERASE BIT(6) #define SPI_NOR_QUAD_PP BIT(8) #define SPI_NOR_RWW BIT(9) +#define SPI_NOR_DEPRECATED BIT(15) u8 no_sfdp_flags; #define SPI_NOR_SKIP_SFDP BIT(0)
SPI-NOR will automatically detect the attached flash device most of the time. We cannot easily find out if boards are using a given flash. Therefore, introduce a (temporary) flag to display a message on boot if support for a given flash device is scheduled to be removed in the future. Signed-off-by: Michael Walle <mwalle@kernel.org> --- drivers/mtd/spi-nor/core.c | 12 ++++++++++++ drivers/mtd/spi-nor/core.h | 1 + 2 files changed, 13 insertions(+)