| Message ID | 20181207092637.18687-35-boris.brezillon@bootlin.com |
|---|---|
| State | Superseded |
| Delegated to: | Ambarus Tudor |
| Headers | show |
| Series | mtd: spi-nor: Move manufacturer/SFDP code out of the core | expand |
Hello Boris, some minor issues inline: On 07/12/2018 10:26, Boris Brezillon wrote: > Experience has proven that SFDP tables are sometimes wrong, and the > parsing that is done by the core can lead ton erroneous flash config > which can sometimes be harmful. > > This leaves us 2 options: > > 1/ set the SPI_NOR_SKIP_SFDP flag and completely ignore SFDP parsing > 2/ fix SFDP tables at runtime > > While #1 should always work, it might implies extra work if most of the > SFDP is correct. #2 has the benefit of keeping the generic SFDP parsing > logic almost untouched while allowing SPI NOR manufacturer drivers to > fix the broken bits. > > Add three new hooks to do that. > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> > --- > drivers/mtd/spi-nor/internals.h | 7 ++++ > drivers/mtd/spi-nor/sfdp.c | 57 +++++++++++++++++++++++++++++++-- > 2 files changed, 62 insertions(+), 2 deletions(-) > > diff --git a/drivers/mtd/spi-nor/internals.h b/drivers/mtd/spi-nor/internals.h > index ae3eb40d7241..da71145995d9 100644 > --- a/drivers/mtd/spi-nor/internals.h > +++ b/drivers/mtd/spi-nor/internals.h > @@ -188,6 +188,8 @@ struct sfdp_bfpt { > > /** > * struct spi_nor_fixups - SPI NOR fixup hooks > + * @sfdp_hdr: SFDP header fixups > + * @sfdp_hdr: SFDP parameter headers fixups ^^^^^^^^ Should be sfdp_param_hdrs > * @post_bfpt: called after the BFPT table has been parsed > * @post_sfdp: called after SFDP has been parsed (is also called for SPI NORs > * that do not support RDSFDP). Typically used to tweak various > @@ -199,6 +201,11 @@ struct sfdp_bfpt { > * table is broken or not available. > */ > struct spi_nor_fixups { > + int (*sfdp_hdr)(struct spi_nor *nor, > + struct sfdp_header *hdr); > + int (*sfdp_param_hdrs)(struct spi_nor *nor, > + struct sfdp_header *hdr, > + struct sfdp_parameter_header *param_hdrs); > int (*post_bfpt)(struct spi_nor *nor, > const struct sfdp_parameter_header *bfpt_header, > const struct sfdp_bfpt *bfpt, > diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c > index 36343e3e6be0..a8cd070e4ea2 100644 > --- a/drivers/mtd/spi-nor/sfdp.c > +++ b/drivers/mtd/spi-nor/sfdp.c > @@ -534,8 +534,8 @@ static const struct sfdp_bfpt_erase sfdp_bfpt_erases[] = { > static int > spi_nor_post_bfpt_fixups(struct spi_nor *nor, > const struct sfdp_parameter_header *bfpt_header, > - const struct sfdp_bfpt *bfpt, > - struct spi_nor_flash_parameter *params) > + const struct sfdp_bfpt *bfpt, > + struct spi_nor_flash_parameter *params) That's where checkpatch.pl would warn you > { > int ret; > > @@ -748,6 +748,50 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor, > return spi_nor_post_bfpt_fixups(nor, bfpt_header, &bfpt, params); > } > > +static int spi_nor_sfdp_hdr_fixups(struct spi_nor *nor, > + struct sfdp_header *hdr) > +{ > + int ret; > + > + if (nor->manufacturer && nor->manufacturer->fixups && > + nor->manufacturer->fixups->sfdp_hdr) { > + ret = nor->manufacturer->fixups->sfdp_hdr(nor, hdr); > + if (ret) > + return ret; > + } > + > + if (nor->info->fixups && nor->info->fixups->sfdp_hdr) { > + ret = nor->info->fixups->sfdp_hdr(nor, hdr); > + if (ret) > + return ret; > + } > + > + return 0; > +} > + > +static int > +spi_nor_sfdp_param_hdrs_fixups(struct spi_nor *nor, struct sfdp_header *hdr, > + struct sfdp_parameter_header *param_hdrs) > +{ > + int ret; > + > + if (nor->manufacturer && nor->manufacturer->fixups && > + nor->manufacturer->fixups->sfdp_param_hdrs) { > + ret = nor->manufacturer->fixups->sfdp_param_hdrs(nor, hdr, > + param_hdrs); > + if (ret) > + return ret; > + } > + > + if (nor->info->fixups && nor->info->fixups->sfdp_hdr) { > + ret = nor->info->fixups->sfdp_param_hdrs(nor, hdr, param_hdrs); > + if (ret) > + return ret; > + } > + > + return 0; > +} > + > /** > * spi_nor_parse_sfdp() - parse the Serial Flash Discoverable Parameters. > * @nor: pointer to a 'struct spi_nor' > @@ -777,6 +821,10 @@ int spi_nor_parse_sfdp(struct spi_nor *nor, > if (err < 0) > return err; > > + err = spi_nor_sfdp_hdr_fixups(nor, &header); > + if (err) > + return err; > + > /* Check the SFDP header version. */ > if (le32_to_cpu(header.signature) != SFDP_SIGNATURE || > header.major != SFDP_JESD216_MAJOR) > @@ -815,6 +863,11 @@ int spi_nor_parse_sfdp(struct spi_nor *nor, > dev_err(dev, "failed to read SFDP parameter headers\n"); > goto exit; > } > + > + err = spi_nor_sfdp_param_hdrs_fixups(nor, &header, > + param_headers); > + if (err) > + return err; > } > > /* >
On Fri, 7 Dec 2018 16:29:41 +0000 "Sverdlin, Alexander (Nokia - DE/Ulm)" <alexander.sverdlin@nokia.com> wrote: > > > > /** > > * struct spi_nor_fixups - SPI NOR fixup hooks > > + * @sfdp_hdr: SFDP header fixups > > + * @sfdp_hdr: SFDP parameter headers fixups > ^^^^^^^^ > Should be sfdp_param_hdrs Oops. > > > * @post_bfpt: called after the BFPT table has been parsed > > * @post_sfdp: called after SFDP has been parsed (is also called for SPI NORs > > * that do not support RDSFDP). Typically used to tweak various > > @@ -199,6 +201,11 @@ struct sfdp_bfpt { > > * table is broken or not available. > > */ > > struct spi_nor_fixups { > > + int (*sfdp_hdr)(struct spi_nor *nor, > > + struct sfdp_header *hdr); > > + int (*sfdp_param_hdrs)(struct spi_nor *nor, > > + struct sfdp_header *hdr, > > + struct sfdp_parameter_header *param_hdrs); > > int (*post_bfpt)(struct spi_nor *nor, > > const struct sfdp_parameter_header *bfpt_header, > > const struct sfdp_bfpt *bfpt, > > diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c > > index 36343e3e6be0..a8cd070e4ea2 100644 > > --- a/drivers/mtd/spi-nor/sfdp.c > > +++ b/drivers/mtd/spi-nor/sfdp.c > > @@ -534,8 +534,8 @@ static const struct sfdp_bfpt_erase sfdp_bfpt_erases[] = { > > static int > > spi_nor_post_bfpt_fixups(struct spi_nor *nor, > > const struct sfdp_parameter_header *bfpt_header, > > - const struct sfdp_bfpt *bfpt, > > - struct spi_nor_flash_parameter *params) > > + const struct sfdp_bfpt *bfpt, > > + struct spi_nor_flash_parameter *params) > > That's where checkpatch.pl would warn you Didn't run checkpatch on this series (I tend to not run checkpatch on big RFC series where I'm expecting feedback on the general approach). Anyway, this patch should be dropped since those hooks are not used, and I don't think we should add new fixup hooks if we don't have a user.
diff --git a/drivers/mtd/spi-nor/internals.h b/drivers/mtd/spi-nor/internals.h index ae3eb40d7241..da71145995d9 100644 --- a/drivers/mtd/spi-nor/internals.h +++ b/drivers/mtd/spi-nor/internals.h @@ -188,6 +188,8 @@ struct sfdp_bfpt { /** * struct spi_nor_fixups - SPI NOR fixup hooks + * @sfdp_hdr: SFDP header fixups + * @sfdp_hdr: SFDP parameter headers fixups * @post_bfpt: called after the BFPT table has been parsed * @post_sfdp: called after SFDP has been parsed (is also called for SPI NORs * that do not support RDSFDP). Typically used to tweak various @@ -199,6 +201,11 @@ struct sfdp_bfpt { * table is broken or not available. */ struct spi_nor_fixups { + int (*sfdp_hdr)(struct spi_nor *nor, + struct sfdp_header *hdr); + int (*sfdp_param_hdrs)(struct spi_nor *nor, + struct sfdp_header *hdr, + struct sfdp_parameter_header *param_hdrs); int (*post_bfpt)(struct spi_nor *nor, const struct sfdp_parameter_header *bfpt_header, const struct sfdp_bfpt *bfpt, diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c index 36343e3e6be0..a8cd070e4ea2 100644 --- a/drivers/mtd/spi-nor/sfdp.c +++ b/drivers/mtd/spi-nor/sfdp.c @@ -534,8 +534,8 @@ static const struct sfdp_bfpt_erase sfdp_bfpt_erases[] = { static int spi_nor_post_bfpt_fixups(struct spi_nor *nor, const struct sfdp_parameter_header *bfpt_header, - const struct sfdp_bfpt *bfpt, - struct spi_nor_flash_parameter *params) + const struct sfdp_bfpt *bfpt, + struct spi_nor_flash_parameter *params) { int ret; @@ -748,6 +748,50 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor, return spi_nor_post_bfpt_fixups(nor, bfpt_header, &bfpt, params); } +static int spi_nor_sfdp_hdr_fixups(struct spi_nor *nor, + struct sfdp_header *hdr) +{ + int ret; + + if (nor->manufacturer && nor->manufacturer->fixups && + nor->manufacturer->fixups->sfdp_hdr) { + ret = nor->manufacturer->fixups->sfdp_hdr(nor, hdr); + if (ret) + return ret; + } + + if (nor->info->fixups && nor->info->fixups->sfdp_hdr) { + ret = nor->info->fixups->sfdp_hdr(nor, hdr); + if (ret) + return ret; + } + + return 0; +} + +static int +spi_nor_sfdp_param_hdrs_fixups(struct spi_nor *nor, struct sfdp_header *hdr, + struct sfdp_parameter_header *param_hdrs) +{ + int ret; + + if (nor->manufacturer && nor->manufacturer->fixups && + nor->manufacturer->fixups->sfdp_param_hdrs) { + ret = nor->manufacturer->fixups->sfdp_param_hdrs(nor, hdr, + param_hdrs); + if (ret) + return ret; + } + + if (nor->info->fixups && nor->info->fixups->sfdp_hdr) { + ret = nor->info->fixups->sfdp_param_hdrs(nor, hdr, param_hdrs); + if (ret) + return ret; + } + + return 0; +} + /** * spi_nor_parse_sfdp() - parse the Serial Flash Discoverable Parameters. * @nor: pointer to a 'struct spi_nor' @@ -777,6 +821,10 @@ int spi_nor_parse_sfdp(struct spi_nor *nor, if (err < 0) return err; + err = spi_nor_sfdp_hdr_fixups(nor, &header); + if (err) + return err; + /* Check the SFDP header version. */ if (le32_to_cpu(header.signature) != SFDP_SIGNATURE || header.major != SFDP_JESD216_MAJOR) @@ -815,6 +863,11 @@ int spi_nor_parse_sfdp(struct spi_nor *nor, dev_err(dev, "failed to read SFDP parameter headers\n"); goto exit; } + + err = spi_nor_sfdp_param_hdrs_fixups(nor, &header, + param_headers); + if (err) + return err; } /*
Experience has proven that SFDP tables are sometimes wrong, and the parsing that is done by the core can lead ton erroneous flash config which can sometimes be harmful. This leaves us 2 options: 1/ set the SPI_NOR_SKIP_SFDP flag and completely ignore SFDP parsing 2/ fix SFDP tables at runtime While #1 should always work, it might implies extra work if most of the SFDP is correct. #2 has the benefit of keeping the generic SFDP parsing logic almost untouched while allowing SPI NOR manufacturer drivers to fix the broken bits. Add three new hooks to do that. Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> --- drivers/mtd/spi-nor/internals.h | 7 ++++ drivers/mtd/spi-nor/sfdp.c | 57 +++++++++++++++++++++++++++++++-- 2 files changed, 62 insertions(+), 2 deletions(-)