| Message ID | 20201003153235.29762-3-michael@walle.cc |
|---|---|
| State | Changes Requested |
| Delegated to: | Ambarus Tudor |
| Headers | show |
| Series | mtd: spi-nor: keep lock bits if they are non-volatile | expand |
On 10/3/20 6:32 PM, Michael Walle wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > This is considered bad for the following reasons: > (1) We only support the block protection with BPn bits for write > protection. Not all SST parts support this. > (2) Newly added flash chip will automatically inherit the "has > locking" support and thus needs to explicitly tested. Better > be opt-in instead of opt-out. > (3) There are already supported flashes which doesn't support > the locking scheme. So I assume this wasn't properly tested > before adding that chip; which enforces my previous argument > that locking support should be an opt-in. > > Remove the global flag and add individual flags to all flashes > which supports BP locking. In particular the following flashes > don't support the BP scheme: > - SST26VF016B > - SST26WF016B > - SST26VF064B > > Signed-off-by: Michael Walle <michael@walle.cc> Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com> > --- > changes since v4: > - none > > changes since v3/v2/v1: > - there was no such version because this patch was bundled with another > patch > > changes since RFC: > - none > > drivers/mtd/spi-nor/sst.c | 30 ++++++++++++------------------ > 1 file changed, 12 insertions(+), 18 deletions(-) > > diff --git a/drivers/mtd/spi-nor/sst.c b/drivers/mtd/spi-nor/sst.c > index e0af6d25d573..8b169fa4102a 100644 > --- a/drivers/mtd/spi-nor/sst.c > +++ b/drivers/mtd/spi-nor/sst.c > @@ -11,26 +11,26 @@ > static const struct flash_info sst_parts[] = { > /* SST -- large erase sizes are "overlays", "sectors" are 4K */ > { "sst25vf040b", INFO(0xbf258d, 0, 64 * 1024, 8, > - SECT_4K | SST_WRITE) }, > + SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) }, https://ww1.microchip.com/downloads/en/DeviceDoc/DS20005051D.pdf BP GENMASK(2, 5), volatile, after power up BP0-2 set to 1, BP3 set to 0. BP3 is "don't care". SR.BPL BIT(7) volatile, default to 0. When 1 - BPs are read-only bits, when 0 - BPs are r/w > { "sst25vf080b", INFO(0xbf258e, 0, 64 * 1024, 16, > - SECT_4K | SST_WRITE) }, > + SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) }, https://ww1.microchip.com/downloads/en/DeviceDoc/20005045C.pdf same as in sst25vf040b > { "sst25vf016b", INFO(0xbf2541, 0, 64 * 1024, 32, > - SECT_4K | SST_WRITE) }, > + SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) }, https://ww1.microchip.com/downloads/en/DeviceDoc/20005044C.pdf same as in sst25vf040b > { "sst25vf032b", INFO(0xbf254a, 0, 64 * 1024, 64, > - SECT_4K | SST_WRITE) }, > - { "sst25vf064c", INFO(0xbf254b, 0, 64 * 1024, 128, SECT_4K) }, > + SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) }, https://ww1.microchip.com/downloads/en/DeviceDoc/20005071B.pdf same as in sst25vf040b > + { "sst25vf064c", INFO(0xbf254b, 0, 64 * 1024, 128, SECT_4K | SPI_NOR_HAS_LOCK) }, https://ww1.microchip.com/downloads/en/devicedoc/20005036c.pdf BP GENMASK(2, 5), volatile, after power up all set to 1. SR.BPL BIT(7) volatile, default to 0. When 1 - BPs are read-only bits, when 0 - BPs are r/w > { "sst25wf512", INFO(0xbf2501, 0, 64 * 1024, 1, > - SECT_4K | SST_WRITE) }, > + SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) }, https://ww1.microchip.com/downloads/en/DeviceDoc/20005016C.pdf BP GENMASK(2, 4), volatile, after power up all set to 1. SR.BPL BIT(7) volatile, default to 0. When 1 - BPs are read-only bits, when 0 - BPs are r/w > { "sst25wf010", INFO(0xbf2502, 0, 64 * 1024, 2, > - SECT_4K | SST_WRITE) }, > + SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) }, same datasheet and characteristics as in sst25wf512 > { "sst25wf020", INFO(0xbf2503, 0, 64 * 1024, 4, > - SECT_4K | SST_WRITE) }, > - { "sst25wf020a", INFO(0x621612, 0, 64 * 1024, 4, SECT_4K) }, > - { "sst25wf040b", INFO(0x621613, 0, 64 * 1024, 8, SECT_4K) }, > + SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) }, same datasheet and characteristics as in sst25wf512 > + { "sst25wf020a", INFO(0x621612, 0, 64 * 1024, 4, SECT_4K | SPI_NOR_HAS_LOCK) }, https://ww1.microchip.com/downloads/en/DeviceDoc/20005139F.pdf BP0 BIT(2), BP1 BIT(3), TB BIT(5), BPL BIT(7) all non-volatile SR.BPL BIT(7): when 1 - BPs are read-only bits, when 0 - BPs are r/w > + { "sst25wf040b", INFO(0x621613, 0, 64 * 1024, 8, SECT_4K | SPI_NOR_HAS_LOCK) }, https://ww1.microchip.com/downloads/en/DeviceDoc/20005193D.pdf BP0 BIT(2), BP1 BIT(3), BP2 BIT(4), TB BIT(5), BPL BIT(7) all non-volatile SR.BPL BIT(7): when 1 - BPs are read-only bits, when 0 - BPs are r/w > { "sst25wf040", INFO(0xbf2504, 0, 64 * 1024, 8, > - SECT_4K | SST_WRITE) }, > + SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) }, same datasheet and characteristics as in sst25wf512 > { "sst25wf080", INFO(0xbf2505, 0, 64 * 1024, 16, > - SECT_4K | SST_WRITE) }, > + SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) }, https://ww1.microchip.com/downloads/en/DeviceDoc/25024A.pdf same as in sst25vf040b > { "sst26wf016b", INFO(0xbf2651, 0, 64 * 1024, 32, https://ww1.microchip.com/downloads/en/DeviceDoc/20005013D.pdf OK. sst26wf016b, SST26VF016B, SST26VF064B support individual block protection and global unlock command. > SECT_4K | SPI_NOR_DUAL_READ | > SPI_NOR_QUAD_READ) }, > @@ -127,11 +127,6 @@ static int sst_write(struct mtd_info *mtd, loff_t to, size_t len, > return ret; > } > > -static void sst_default_init(struct spi_nor *nor) > -{ > - nor->flags |= SNOR_F_HAS_LOCK; > -} > - > static void sst_post_sfdp_fixups(struct spi_nor *nor) > { > if (nor->info->flags & SST_WRITE) > @@ -139,7 +134,6 @@ static void sst_post_sfdp_fixups(struct spi_nor *nor) > } > > static const struct spi_nor_fixups sst_fixups = { > - .default_init = sst_default_init, > .post_sfdp = sst_post_sfdp_fixups, > }; > > -- > 2.20.1 >
diff --git a/drivers/mtd/spi-nor/sst.c b/drivers/mtd/spi-nor/sst.c index e0af6d25d573..8b169fa4102a 100644 --- a/drivers/mtd/spi-nor/sst.c +++ b/drivers/mtd/spi-nor/sst.c @@ -11,26 +11,26 @@ static const struct flash_info sst_parts[] = { /* SST -- large erase sizes are "overlays", "sectors" are 4K */ { "sst25vf040b", INFO(0xbf258d, 0, 64 * 1024, 8, - SECT_4K | SST_WRITE) }, + SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) }, { "sst25vf080b", INFO(0xbf258e, 0, 64 * 1024, 16, - SECT_4K | SST_WRITE) }, + SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) }, { "sst25vf016b", INFO(0xbf2541, 0, 64 * 1024, 32, - SECT_4K | SST_WRITE) }, + SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) }, { "sst25vf032b", INFO(0xbf254a, 0, 64 * 1024, 64, - SECT_4K | SST_WRITE) }, - { "sst25vf064c", INFO(0xbf254b, 0, 64 * 1024, 128, SECT_4K) }, + SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) }, + { "sst25vf064c", INFO(0xbf254b, 0, 64 * 1024, 128, SECT_4K | SPI_NOR_HAS_LOCK) }, { "sst25wf512", INFO(0xbf2501, 0, 64 * 1024, 1, - SECT_4K | SST_WRITE) }, + SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) }, { "sst25wf010", INFO(0xbf2502, 0, 64 * 1024, 2, - SECT_4K | SST_WRITE) }, + SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) }, { "sst25wf020", INFO(0xbf2503, 0, 64 * 1024, 4, - SECT_4K | SST_WRITE) }, - { "sst25wf020a", INFO(0x621612, 0, 64 * 1024, 4, SECT_4K) }, - { "sst25wf040b", INFO(0x621613, 0, 64 * 1024, 8, SECT_4K) }, + SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) }, + { "sst25wf020a", INFO(0x621612, 0, 64 * 1024, 4, SECT_4K | SPI_NOR_HAS_LOCK) }, + { "sst25wf040b", INFO(0x621613, 0, 64 * 1024, 8, SECT_4K | SPI_NOR_HAS_LOCK) }, { "sst25wf040", INFO(0xbf2504, 0, 64 * 1024, 8, - SECT_4K | SST_WRITE) }, + SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) }, { "sst25wf080", INFO(0xbf2505, 0, 64 * 1024, 16, - SECT_4K | SST_WRITE) }, + SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK) }, { "sst26wf016b", INFO(0xbf2651, 0, 64 * 1024, 32, SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) }, @@ -127,11 +127,6 @@ static int sst_write(struct mtd_info *mtd, loff_t to, size_t len, return ret; } -static void sst_default_init(struct spi_nor *nor) -{ - nor->flags |= SNOR_F_HAS_LOCK; -} - static void sst_post_sfdp_fixups(struct spi_nor *nor) { if (nor->info->flags & SST_WRITE) @@ -139,7 +134,6 @@ static void sst_post_sfdp_fixups(struct spi_nor *nor) } static const struct spi_nor_fixups sst_fixups = { - .default_init = sst_default_init, .post_sfdp = sst_post_sfdp_fixups, };
This is considered bad for the following reasons: (1) We only support the block protection with BPn bits for write protection. Not all SST parts support this. (2) Newly added flash chip will automatically inherit the "has locking" support and thus needs to explicitly tested. Better be opt-in instead of opt-out. (3) There are already supported flashes which doesn't support the locking scheme. So I assume this wasn't properly tested before adding that chip; which enforces my previous argument that locking support should be an opt-in. Remove the global flag and add individual flags to all flashes which supports BP locking. In particular the following flashes don't support the BP scheme: - SST26VF016B - SST26WF016B - SST26VF064B Signed-off-by: Michael Walle <michael@walle.cc> --- changes since v4: - none changes since v3/v2/v1: - there was no such version because this patch was bundled with another patch changes since RFC: - none drivers/mtd/spi-nor/sst.c | 30 ++++++++++++------------------ 1 file changed, 12 insertions(+), 18 deletions(-)