On Sun, 25 Aug 2019 13:19:57 +0000 <[email protected]> wrote:
> On 08/25/2019 04:09 PM, Boris Brezillon wrote: > > On Fri, 23 Aug 2019 15:53:41 +0000 > > <[email protected]> wrote: > > > >> From: Tudor Ambarus <[email protected]> > >> > >> All flash parameters and settings should reside inside > >> 'struct spi_nor_flash_parameter'. Move clear_sr_bp() from > >> 'struct spi_nor' to 'struct spi_nor_flash_parameter'. > >> > >> Rename clear_sr_bp()/disable_block_protection() to better indicate > >> what the function does. > >> > >> Signed-off-by: Tudor Ambarus <[email protected]> > >> --- > >> drivers/mtd/spi-nor/spi-nor.c | 47 > >> +++++++++++++++++++++++++++++++++---------- > >> include/linux/mtd/spi-nor.h | 5 ++--- > >> 2 files changed, 38 insertions(+), 14 deletions(-) > >> > >> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c > >> index 6bd104c29cd9..15b0b1148bf3 100644 > >> --- a/drivers/mtd/spi-nor/spi-nor.c > >> +++ b/drivers/mtd/spi-nor/spi-nor.c > >> @@ -4477,20 +4477,45 @@ static int spi_nor_quad_enable(struct spi_nor *nor) > >> return nor->params.quad_enable(nor); > >> } > >> > >> +/** > >> + * spi_nor_disable_block_protection() - Disable the write block protection > >> + * during power-up. > >> + * @nor: pointer to a 'struct spi_nor' > >> + * > >> + * Some spi-nor flashes are write protected by default after a power-on > >> reset > >> + * cycle, in order to avoid inadvertend writes during power-up. Backward > >> + * compatibility imposes to disable the write block protection at power-up > >> + * by default. > >> + * > >> + * Return: 0 on success, -errno otherwise. > >> + */ > >> +static int spi_nor_disable_block_protection(struct spi_nor *nor) > >> +{ > >> + if (!nor->params.disable_block_protection) > >> + return 0; > >> + > >> + /* > >> + * In case of the legacy quad enable requirements are set, if the > >> + * configuration register Quad Enable bit is one, only the the > >> + * Write Status (01h) command with two data bytes may be used to clear > >> + * the block protection bits. > >> + */ > >> + if (nor->params.quad_enable == spansion_quad_enable) > >> + nor->params.disable_block_protection = > >> + spi_nor_spansion_clear_sr_bp; > > > > Hm, doesn't look right to adjust the function pointer just before > > calling it. Can't we move that logic earlier (when doing the > > default/manufacturer specific init)? Also, as I said in one of my > > No, we can't move it earlier to ->default_init() because the pointer to > quad_enable() function can be modified later on, when parsing SFDP. This > should > stay here, after the quad_enable() method is known, so after the > spi_nor_init_params() call. > > > > previous emails, I'd prefer to have this hook moved to > > spi_nor_locking_ops and just have a flag to reflect when block > > protection can be disabled. > > yes, I agree, will move. That won't work: ->locking_ops is const, meaning that you can't update individual fields (which I consider a good thing). As I see it, the locking scheme is a package that describe how to lock/unlock blocks, be it individually, by groups or globally. I really hope we can take this decision on a per-manufacturer basis, but I fear it might actually be a per-chip thing. > > > > >> + > >> + return nor->params.disable_block_protection(nor); > >> +} > >> + > >> static int spi_nor_init(struct spi_nor *nor) > >> { > >> int err; > >> > >> - if (nor->clear_sr_bp) { > >> - if (nor->quad_enable == spansion_quad_enable) > >> - nor->clear_sr_bp = spi_nor_spansion_clear_sr_bp; > >> - > >> - err = nor->clear_sr_bp(nor); > >> - if (err) { > >> - dev_err(nor->dev, > >> - "fail to clear block protection bits\n"); > >> - return err; > >> - } > >> + err = spi_nor_disable_block_protection(nor); > >> + if (err) { > >> + dev_err(nor->dev, > >> + "fail to unlock the flash at init (err = %d)\n", err); > >> + return err; > >> } > >> > >> err = spi_nor_quad_enable(nor); > >> @@ -4635,7 +4660,7 @@ int spi_nor_scan(struct spi_nor *nor, const char > >> *name, > >> JEDEC_MFR(nor->info) == SNOR_MFR_INTEL || > >> JEDEC_MFR(nor->info) == SNOR_MFR_SST || > >> nor->info->flags & SPI_NOR_HAS_LOCK) > >> - nor->clear_sr_bp = spi_nor_clear_sr_bp; > >> + nor->params.disable_block_protection = spi_nor_clear_sr_bp; > >> > >> /* Parse the Serial Flash Discoverable Parameters table. */ > >> ret = spi_nor_init_params(nor); > >> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h > >> index 17787238f0e9..399ac34a529d 100644 > >> --- a/include/linux/mtd/spi-nor.h > >> +++ b/include/linux/mtd/spi-nor.h > >> @@ -480,6 +480,7 @@ struct spi_nor; > >> * @page_programs: page program capabilities ordered by priority: > >> the > >> * higher index in the array, the higher priority. > >> * @quad_enable: enables SPI NOR quad mode. > >> + * @disable_block_protection: disables block protection during power-up. > >> */ > >> struct spi_nor_flash_parameter { > >> u64 size; > >> @@ -490,6 +491,7 @@ struct spi_nor_flash_parameter { > >> struct spi_nor_pp_command page_programs[SNOR_CMD_PP_MAX]; > >> > >> int (*quad_enable)(struct spi_nor *nor); > >> + int (*disable_block_protection)(struct spi_nor *nor); > >> }; > >> > >> /** > >> @@ -535,8 +537,6 @@ struct flash_info; > >> * @flash_unlock: [FLASH-SPECIFIC] unlock a region of the SPI NOR > >> * @flash_is_locked: [FLASH-SPECIFIC] check if a region of the SPI > >> NOR is > >> * completely locked > >> - * @clear_sr_bp: [FLASH-SPECIFIC] clears the Block Protection Bits from > >> - * the SPI NOR Status Register. > >> * @params: [FLASH-SPECIFIC] SPI-NOR flash parameters and > >> settings. > >> * The structure includes legacy flash parameters and > >> * settings that can be overwritten by the > >> spi_nor_fixups > >> @@ -578,7 +578,6 @@ struct spi_nor { > >> int (*flash_lock)(struct spi_nor *nor, loff_t ofs, uint64_t len); > >> int (*flash_unlock)(struct spi_nor *nor, loff_t ofs, uint64_t len); > >> int (*flash_is_locked)(struct spi_nor *nor, loff_t ofs, uint64_t len); > >> - int (*clear_sr_bp)(struct spi_nor *nor); > >> struct spi_nor_flash_parameter params; > >> > >> void *priv; > > > >
