Hi Venkatesh,
On Fri, 9 Aug 2024 at 03:39, Venkatesh Yadav Abbarapu
<venkatesh.abbar...@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.abbar...@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
