Messages in this thread Patch in this message | | | From | Pratyush Yadav <> | Subject | Re: [RFC PATCH v1 6/6] mtd: spi-nor: introduce support for displaying deprecation message | Date | Wed, 17 Apr 2024 17:52:48 +0200 |
| |
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) >
-- Regards, Pratyush Yadav
| |