Hi Venkatesh,
On Fri, 9 Aug 2024 at 03:39, Venkatesh Yadav Abbarapu venkatesh.abbarapu@amd.com wrote:
Many SPI flashes have status/OTP bit for TOP or BOTTOM selection in the status register that can protect selected regions of the SPI NOR.
Take this bit into consideration while locking the region. The command "sf lockinfo" shows whether the flash is TOP or BOTTOM protected, based on this info the "offset" is provided to the "sf protect lock" command.
Versal> sf erase 0 100000 SF: 1048576 bytes @ 0x0 Erased: OK Versal>sf lockinfo Flash is BOTTOM protected Versal> sf protect lock 0 20000 Versal> sf erase 0 20000 ERROR: flash area is locked Versal> sf protect unlock 0 20000 Versal> sf erase 0 20000 SF: 131072 bytes @ 0x0 Erased: OK
Signed-off-by: Venkatesh Yadav Abbarapu venkatesh.abbarapu@amd.com
cmd/sf.c | 30 +++++++++++++++++++++++++++++- drivers/mtd/spi/spi-nor-core.c | 16 ++++++++++++++++ include/linux/mtd/spi-nor.h | 1 + include/spi.h | 3 +++ include/spi_flash.h | 8 ++++++++ 5 files changed, 57 insertions(+), 1 deletion(-)
Please can you add to doc/usage/cmd/sf.c and extend the test in test/dm/sf.c ? Let me know if you need help with the test.
diff --git a/cmd/sf.c b/cmd/sf.c index f43a2e08b3..62afa91057 100644 --- a/cmd/sf.c +++ b/cmd/sf.c @@ -413,6 +413,30 @@ static int do_spi_protect(int argc, char *const argv[]) return ret == 0 ? 0 : 1; }
+static int do_spi_lock_info(int argc, char *const argv[]) +{
int ret;if (argc != 1)return CMD_RET_USAGE;
Shouldn't be needed, see below.
ret = spi_flash_lock_info(flash);if (ret < 0) {printf("Flash info is not set\n");return CMD_RET_FAILURE;}if (ret == BOTTOM_PROTECT) {printf("Flash is BOTTOM protected\n");return CMD_RET_SUCCESS;} else if (ret == TOP_PROTECT) {printf("Flash is TOP protected\n");return CMD_RET_SUCCESS;
return 0 is better than CMD_RET_SUCCESS, to avoid confusion that success might be anything other than 0.
}return CMD_RET_FAILURE;+}
enum { STAGE_ERASE, STAGE_CHECK, @@ -607,6 +631,8 @@ static int do_spi_flash(struct cmd_tbl *cmdtp, int flag, int argc, ret = do_spi_flash_erase(argc, argv); else if (IS_ENABLED(CONFIG_SPI_FLASH_LOCK) && strcmp(cmd, "protect") == 0) ret = do_spi_protect(argc, argv);
else if (IS_ENABLED(CONFIG_SPI_FLASH_LOCK) && strcmp(cmd, "lockinfo") == 0)ret = do_spi_lock_info(argc, argv);
Eek this is a mess. Before adding your feature, please create a patch to refactor this code to use U_BOOT_CMD_WITH_SUBCMDS() like other commands with subcommands.
else if (IS_ENABLED(CONFIG_CMD_SF_TEST) && !strcmp(cmd, "test")) ret = do_spi_flash_test(argc, argv); else@@ -632,7 +658,9 @@ U_BOOT_LONGHELP(sf, " or to start of mtd `partition'\n" #ifdef CONFIG_SPI_FLASH_LOCK "sf protect lock/unlock sector len - protect/unprotect 'len' bytes starting\n"
" at address 'sector'"
" at address 'sector'\n""sf lockinfo - shows whether the flash locking is top or bottom\n"" protected\n"#endif #ifdef CONFIG_CMD_SF_TEST "\nsf test offset len - run a very basic destructive test" diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c index aea611fef5..2114be1e61 100644 --- a/drivers/mtd/spi/spi-nor-core.c +++ b/drivers/mtd/spi/spi-nor-core.c @@ -1410,6 +1410,21 @@ static int stm_is_unlocked(struct spi_nor *nor, loff_t ofs, uint64_t len)
return stm_is_unlocked_sr(nor, ofs, len, status);}
+static bool stm_lock_info(struct spi_nor *nor) +{
int status;status = read_sr(nor);if (status < 0)return status;if (status & SR_TB)return BOTTOM_PROTECT;return TOP_PROTECT;+}
#endif /* CONFIG_SPI_FLASH_STMICRO */ #endif
@@ -4145,6 +4160,7 @@ int spi_nor_scan(struct spi_nor *nor) nor->flash_lock = stm_lock; nor->flash_unlock = stm_unlock; nor->flash_is_unlocked = stm_is_unlocked;
nor->flash_lock_info = stm_lock_info; }#endif
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h index d1dbf3eadb..48d39a7052 100644 --- a/include/linux/mtd/spi-nor.h +++ b/include/linux/mtd/spi-nor.h @@ -580,6 +580,7 @@ struct spi_nor { int (*quad_enable)(struct spi_nor *nor); int (*octal_dtr_enable)(struct spi_nor *nor); int (*ready)(struct spi_nor *nor);
bool (*flash_lock_info)(struct spi_nor *nor);
Are we trying to keep this in sync with Linux mtd? It seems that Linux has split these ops into a separate struct. We should really be using an MTD uclass here, I suspect. But that is another discussion...
We have these members:
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_unlocked)(struct spi_nor *nor, loff_t ofs, uint64_t len); int (*quad_enable)(struct spi_nor *nor); int (*octal_dtr_enable)(struct spi_nor *nor); int (*ready)(struct spi_nor *nor);
It seems to me that we need to sort this out a bit one day. But for now (and your patch), how about a lock_info() call which fills in a new 'struct spi_nor_info' with all the useful info? Then you can put your bool in that struct and we can extend it to other things as needed.
struct { struct spi_mem_dirmap_desc *rdesc;diff --git a/include/spi.h b/include/spi.h index 7e38cc2a2a..46d27f7564 100644 --- a/include/spi.h +++ b/include/spi.h @@ -38,6 +38,9 @@
#define SPI_DEFAULT_WORDLEN 8
+#define BOTTOM_PROTECT 1 +#define TOP_PROTECT 0
/**
- struct dm_spi_bus - SPI bus info
diff --git a/include/spi_flash.h b/include/spi_flash.h index 10d19fd4b1..b8a6456fe4 100644 --- a/include/spi_flash.h +++ b/include/spi_flash.h @@ -202,4 +202,12 @@ static inline int spi_flash_protect(struct spi_flash *flash, u32 ofs, u32 len, return flash->flash_unlock(flash, ofs, len); }
+static inline int spi_flash_lock_info(struct spi_flash *flash) +{
if (!flash->flash_lock_info)return -EOPNOTSUPP;return flash->flash_lock_info(flash);+}
#endif /* _SPI_FLASH_H_ */
2.17.1
Regards, SImon