| Message ID | 20190731090315.26798-7-tudor.ambarus@microchip.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Ambarus Tudor |
| Headers | show |
| Series | mtd: spi-nor: move manuf out of the core - batch 1 | expand |
Hi Tudor, On 31-Jul-19 2:33 PM, Tudor.Ambarus@microchip.com wrote: > From: Boris Brezillon <boris.brezillon@bootlin.com> > > Move the locking hooks in a separate struct so that we have just > one field to update when we change the locking implementation. > > stm_locking_ops, the legacy locking operations, can be overwritten > later on by implementing manufacturer specific default_init() hooks. > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> > [tudor.ambarus@microchip.com: use ->default_init() hook] > Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> [...] > @@ -1782,7 +1788,7 @@ static int spi_nor_is_locked(struct mtd_info *mtd, loff_t ofs, uint64_t len) > if (ret) > return ret; > > - ret = nor->flash_is_locked(nor, ofs, len); > + ret = nor->locking_ops->is_locked(nor, ofs, len); > > spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_LOCK); > return ret; > @@ -4805,6 +4811,10 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, > nor->quad_enable = spansion_quad_enable; > nor->set_4byte = spansion_set_4byte; > > + /* Default locking operations. */ > + if (info->flags & SPI_NOR_HAS_LOCK) > + nor->locking_ops = &stm_locking_ops; > + This condition is different than how lock/unlock ops are populated today. We would need to add SPI_NOR_HAS_LOCK to all SNOR_MFR_ST and SNOR_MFR_MICRON entries to be backward compatible or keep the condition as is. > /* Init flash parameters based on flash_info struct and SFDP */ > spi_nor_init_params(nor, ¶ms); > > @@ -4819,21 +4829,6 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, > mtd->_read = spi_nor_read; > mtd->_resume = spi_nor_resume; > > - /* NOR protection support for STmicro/Micron chips and similar */ > - if (JEDEC_MFR(info) == SNOR_MFR_ST || > - JEDEC_MFR(info) == SNOR_MFR_MICRON || > - info->flags & SPI_NOR_HAS_LOCK) { > - nor->flash_lock = stm_lock; > - nor->flash_unlock = stm_unlock; > - nor->flash_is_locked = stm_is_locked; > - } > - [...] > diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h > index a434ab7a53e6..bd68ec5a10e7 100644 > --- a/include/linux/mtd/spi-nor.h > +++ b/include/linux/mtd/spi-nor.h > @@ -425,9 +425,23 @@ struct spi_nor { > int (*set_4byte)(struct spi_nor *nor, bool enable); > int (*clear_sr_bp)(struct spi_nor *nor); > > + const struct spi_nor_locking_ops *locking_ops; > + Also, to be consistent, document this new member. > void *priv; > }; > > +/** > + * struct spi_nor_locking_ops - SPI NOR locking methods > + * @lock: lock a region of the SPI NOR > + * @unlock: unlock a region of the SPI NOR > + * @is_locked: check if a region of the SPI NOR is completely locked > + */ > +struct spi_nor_locking_ops { > + int (*lock)(struct spi_nor *nor, loff_t ofs, uint64_t len); > + int (*unlock)(struct spi_nor *nor, loff_t ofs, uint64_t len); > + int (*is_locked)(struct spi_nor *nor, loff_t ofs, uint64_t len); checkpatch does not like uint64_t. Please changes these to size_t Regards Vignesh > +}; > + > static u64 __maybe_unused > spi_nor_region_is_last(const struct spi_nor_erase_region *region) > { >
On 08/04/2019 05:36 PM, Vignesh Raghavendra wrote: > External E-Mail > > > Hi Tudor, > > On 31-Jul-19 2:33 PM, Tudor.Ambarus@microchip.com wrote: >> From: Boris Brezillon <boris.brezillon@bootlin.com> >> >> Move the locking hooks in a separate struct so that we have just >> one field to update when we change the locking implementation. >> >> stm_locking_ops, the legacy locking operations, can be overwritten >> later on by implementing manufacturer specific default_init() hooks. >> >> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> >> [tudor.ambarus@microchip.com: use ->default_init() hook] >> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> > > [...] > >> @@ -1782,7 +1788,7 @@ static int spi_nor_is_locked(struct mtd_info *mtd, loff_t ofs, uint64_t len) >> if (ret) >> return ret; >> >> - ret = nor->flash_is_locked(nor, ofs, len); >> + ret = nor->locking_ops->is_locked(nor, ofs, len); >> >> spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_LOCK); >> return ret; >> @@ -4805,6 +4811,10 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, >> nor->quad_enable = spansion_quad_enable; >> nor->set_4byte = spansion_set_4byte; >> >> + /* Default locking operations. */ >> + if (info->flags & SPI_NOR_HAS_LOCK) >> + nor->locking_ops = &stm_locking_ops; >> + > > This condition is different than how lock/unlock ops are populated > today. We would need to add SPI_NOR_HAS_LOCK to all SNOR_MFR_ST and > SNOR_MFR_MICRON entries to be backward compatible or keep the condition > as is. Will do, thanks! > >> /* Init flash parameters based on flash_info struct and SFDP */ >> spi_nor_init_params(nor, ¶ms); >> >> @@ -4819,21 +4829,6 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, >> mtd->_read = spi_nor_read; >> mtd->_resume = spi_nor_resume; >> >> - /* NOR protection support for STmicro/Micron chips and similar */ >> - if (JEDEC_MFR(info) == SNOR_MFR_ST || >> - JEDEC_MFR(info) == SNOR_MFR_MICRON || >> - info->flags & SPI_NOR_HAS_LOCK) { >> - nor->flash_lock = stm_lock; >> - nor->flash_unlock = stm_unlock; >> - nor->flash_is_locked = stm_is_locked; >> - } >> - > > [...] > >> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h >> index a434ab7a53e6..bd68ec5a10e7 100644 >> --- a/include/linux/mtd/spi-nor.h >> +++ b/include/linux/mtd/spi-nor.h >> @@ -425,9 +425,23 @@ struct spi_nor { >> int (*set_4byte)(struct spi_nor *nor, bool enable); >> int (*clear_sr_bp)(struct spi_nor *nor); >> >> + const struct spi_nor_locking_ops *locking_ops; >> + > > Also, to be consistent, document this new member. Will do. > > >> void *priv; >> }; >> >> +/** >> + * struct spi_nor_locking_ops - SPI NOR locking methods >> + * @lock: lock a region of the SPI NOR >> + * @unlock: unlock a region of the SPI NOR >> + * @is_locked: check if a region of the SPI NOR is completely locked >> + */ >> +struct spi_nor_locking_ops { >> + int (*lock)(struct spi_nor *nor, loff_t ofs, uint64_t len); >> + int (*unlock)(struct spi_nor *nor, loff_t ofs, uint64_t len); >> + int (*is_locked)(struct spi_nor *nor, loff_t ofs, uint64_t len); > > checkpatch does not like uint64_t. Please changes these to size_t This respects what struct mtd_info is expecting: int (*_lock) (struct mtd_info *mtd, loff_t ofs, uint64_t len); int (*_unlock) (struct mtd_info *mtd, loff_t ofs, uint64_t len); int (*_is_locked) (struct mtd_info *mtd, loff_t ofs, uint64_t len); I haven't seen the warnings, would you mind pasting them? ./scripts/checkpatch.pl --strict 6-7-mtd-spi-nor-Rework-the-SPI-NOR-lock-unlock-logic.patch total: 0 errors, 0 warnings, 0 checks, 102 lines checked 6-7-mtd-spi-nor-Rework-the-SPI-NOR-lock-unlock-logic.patch has no obvious style problems and is ready for submission. Cheers, ta > > Regards > Vignesh > > >> +}; >> + >> static u64 __maybe_unused >> spi_nor_region_is_last(const struct spi_nor_erase_region *region) >> { >>
On 05/08/19 1:30 PM, Tudor.Ambarus@microchip.com wrote: >> >> On 31-Jul-19 2:33 PM, Tudor.Ambarus@microchip.com wrote: >>> From: Boris Brezillon <boris.brezillon@bootlin.com> >>> >>> Move the locking hooks in a separate struct so that we have just >>> one field to update when we change the locking implementation. >>> >>> stm_locking_ops, the legacy locking operations, can be overwritten >>> later on by implementing manufacturer specific default_init() hooks. >>> >>> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> >>> [tudor.ambarus@microchip.com: use ->default_init() hook] >>> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> >> >> [...] [...] >>> >>> +/** >>> + * struct spi_nor_locking_ops - SPI NOR locking methods >>> + * @lock: lock a region of the SPI NOR >>> + * @unlock: unlock a region of the SPI NOR >>> + * @is_locked: check if a region of the SPI NOR is completely locked >>> + */ >>> +struct spi_nor_locking_ops { >>> + int (*lock)(struct spi_nor *nor, loff_t ofs, uint64_t len); >>> + int (*unlock)(struct spi_nor *nor, loff_t ofs, uint64_t len); >>> + int (*is_locked)(struct spi_nor *nor, loff_t ofs, uint64_t len); >> >> checkpatch does not like uint64_t. Please changes these to size_t > > This respects what struct mtd_info is expecting: > > int (*_lock) (struct mtd_info *mtd, loff_t ofs, uint64_t len); > int (*_unlock) (struct mtd_info *mtd, loff_t ofs, uint64_t len); > int (*_is_locked) (struct mtd_info *mtd, loff_t ofs, uint64_t len); > > I haven't seen the warnings, would you mind pasting them? > ./scripts/checkpatch.pl --strict 6-7-mtd-spi-nor-Rework-the-SPI-NOR-lock-unlock-logic.patch > total: 0 errors, 0 warnings, 0 checks, 102 lines checked > > 6-7-mtd-spi-nor-Rework-the-SPI-NOR-lock-unlock-logic.patch has no obvious style problems and is ready for submission. > Hmm, seems to be emitted only for certain type of declarations. Not sure whats the pattern here. Warning is something like: CHECK: Prefer kernel type 'u64' over 'uint64_t' from: https://elixir.bootlin.com/linux/latest/source/scripts/checkpatch.pl#L5906
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c index e35aae88d38b..95ca5dd96403 100644 --- a/drivers/mtd/spi-nor/spi-nor.c +++ b/drivers/mtd/spi-nor/spi-nor.c @@ -1743,6 +1743,12 @@ static int stm_is_locked(struct spi_nor *nor, loff_t ofs, uint64_t len) return stm_is_locked_sr(nor, ofs, len, status); } +static const struct spi_nor_locking_ops stm_locking_ops = { + .lock = stm_lock, + .unlock = stm_unlock, + .is_locked = stm_is_locked, +}; + static int spi_nor_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len) { struct spi_nor *nor = mtd_to_spi_nor(mtd); @@ -1752,7 +1758,7 @@ static int spi_nor_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len) if (ret) return ret; - ret = nor->flash_lock(nor, ofs, len); + ret = nor->locking_ops->lock(nor, ofs, len); spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_UNLOCK); return ret; @@ -1767,7 +1773,7 @@ static int spi_nor_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len) if (ret) return ret; - ret = nor->flash_unlock(nor, ofs, len); + ret = nor->locking_ops->unlock(nor, ofs, len); spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_LOCK); return ret; @@ -1782,7 +1788,7 @@ static int spi_nor_is_locked(struct mtd_info *mtd, loff_t ofs, uint64_t len) if (ret) return ret; - ret = nor->flash_is_locked(nor, ofs, len); + ret = nor->locking_ops->is_locked(nor, ofs, len); spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_LOCK); return ret; @@ -4805,6 +4811,10 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, nor->quad_enable = spansion_quad_enable; nor->set_4byte = spansion_set_4byte; + /* Default locking operations. */ + if (info->flags & SPI_NOR_HAS_LOCK) + nor->locking_ops = &stm_locking_ops; + /* Init flash parameters based on flash_info struct and SFDP */ spi_nor_init_params(nor, ¶ms); @@ -4819,21 +4829,6 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, mtd->_read = spi_nor_read; mtd->_resume = spi_nor_resume; - /* NOR protection support for STmicro/Micron chips and similar */ - if (JEDEC_MFR(info) == SNOR_MFR_ST || - JEDEC_MFR(info) == SNOR_MFR_MICRON || - info->flags & SPI_NOR_HAS_LOCK) { - nor->flash_lock = stm_lock; - nor->flash_unlock = stm_unlock; - nor->flash_is_locked = stm_is_locked; - } - - if (nor->flash_lock && nor->flash_unlock && nor->flash_is_locked) { - mtd->_lock = spi_nor_lock; - mtd->_unlock = spi_nor_unlock; - mtd->_is_locked = spi_nor_is_locked; - } - /* sst nor chips use AAI word program */ if (info->flags & SST_WRITE) mtd->_write = sst_write; @@ -4874,6 +4869,12 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, if (info->flags & SPI_NOR_NO_FR) params.hwcaps.mask &= ~SNOR_HWCAPS_READ_FAST; + if (nor->locking_ops) { + mtd->_lock = spi_nor_lock; + mtd->_unlock = spi_nor_unlock; + mtd->_is_locked = spi_nor_is_locked; + } + /* * Configure the SPI memory: * - select op codes for (Fast) Read, Page Program and Sector Erase. diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h index a434ab7a53e6..bd68ec5a10e7 100644 --- a/include/linux/mtd/spi-nor.h +++ b/include/linux/mtd/spi-nor.h @@ -425,9 +425,23 @@ struct spi_nor { int (*set_4byte)(struct spi_nor *nor, bool enable); int (*clear_sr_bp)(struct spi_nor *nor); + const struct spi_nor_locking_ops *locking_ops; + void *priv; }; +/** + * struct spi_nor_locking_ops - SPI NOR locking methods + * @lock: lock a region of the SPI NOR + * @unlock: unlock a region of the SPI NOR + * @is_locked: check if a region of the SPI NOR is completely locked + */ +struct spi_nor_locking_ops { + int (*lock)(struct spi_nor *nor, loff_t ofs, uint64_t len); + int (*unlock)(struct spi_nor *nor, loff_t ofs, uint64_t len); + int (*is_locked)(struct spi_nor *nor, loff_t ofs, uint64_t len); +}; + static u64 __maybe_unused spi_nor_region_is_last(const struct spi_nor_erase_region *region) {