[U-Boot,v5,17/18] spi: Add SPI NOR protection mechanism

From: Fabio Estevam <fabio.estevam@freescale.com>

Many SPI flashes have protection bits (BP2, BP1 and BP0) in the
status register that can protect selected regions of the SPI NOR.

Take these bits into account when performing erase operations,
making sure that the protected areas are skipped.

Tested on a mx6qsabresd:

=> sf probe
SF: Detected M25P32 with page size 256 Bytes, erase size 64 KiB, total 4 MiB
=> sf protect lock  0x3f0000 0x10000
=> sf erase 0x3f0000 0x10000
offset 0x3f0000 is protected and cannot be erased
SF: 65536 bytes @ 0x3f0000 Erased: ERROR
=> sf protect unlock  0x3f0000 0x10000
=> sf erase 0x3f0000 0x10000
SF: 65536 bytes @ 0x3f0000 Erased: OK

Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
Changes since v4:
- Only apply lock operations if the SPI NOR flash ID matches STMicro (Jagan)

 common/cmd_sf.c               | 35 +++++++++++++++++++++++++
 drivers/mtd/spi/sf-uclass.c   |  8 ++++++
 drivers/mtd/spi/sf_internal.h |  8 ++++++
 drivers/mtd/spi/sf_ops.c      | 25 ++++++++++++++++++
 drivers/mtd/spi/sf_probe.c    | 61 +++++++++++++++++++++++++++++++++++++++++++
 include/spi_flash.h           | 41 +++++++++++++++++++++++++++++
 6 files changed, 178 insertions(+)

Hi Fabio,

On 3 November 2015 at 05:08, Fabio Estevam <festevam@gmail.com> wrote:
> From: Fabio Estevam <fabio.estevam@freescale.com>
>
> Many SPI flashes have protection bits (BP2, BP1 and BP0) in the
> status register that can protect selected regions of the SPI NOR.
>
> Take these bits into account when performing erase operations,
> making sure that the protected areas are skipped.
>
> Tested on a mx6qsabresd:
>
> => sf probe
> SF: Detected M25P32 with page size 256 Bytes, erase size 64 KiB, total 4 MiB
> => sf protect lock  0x3f0000 0x10000
> => sf erase 0x3f0000 0x10000
> offset 0x3f0000 is protected and cannot be erased
> SF: 65536 bytes @ 0x3f0000 Erased: ERROR
> => sf protect unlock  0x3f0000 0x10000
> => sf erase 0x3f0000 0x10000
> SF: 65536 bytes @ 0x3f0000 Erased: OK
>
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> ---
> Changes since v4:
> - Only apply lock operations if the SPI NOR flash ID matches STMicro (Jagan)
>
>  common/cmd_sf.c               | 35 +++++++++++++++++++++++++
>  drivers/mtd/spi/sf-uclass.c   |  8 ++++++
>  drivers/mtd/spi/sf_internal.h |  8 ++++++
>  drivers/mtd/spi/sf_ops.c      | 25 ++++++++++++++++++
>  drivers/mtd/spi/sf_probe.c    | 61 +++++++++++++++++++++++++++++++++++++++++++
>  include/spi_flash.h           | 41 +++++++++++++++++++++++++++++
>  6 files changed, 178 insertions(+)
>
> diff --git a/common/cmd_sf.c b/common/cmd_sf.c
> index ac7f5df..42862d9 100644
> --- a/common/cmd_sf.c
> +++ b/common/cmd_sf.c
> @@ -348,6 +348,37 @@ static int do_spi_flash_erase(int argc, char * const argv[])
>         return ret == 0 ? 0 : 1;
>  }
>
> +static int do_spi_protect(int argc, char * const argv[])
> +{
> +       int ret = 0;
> +       loff_t start, len;
> +       bool prot = false;
> +
> +       if (argc != 4)
> +               return -1;
> +
> +       if (!str2off(argv[2], &start)) {
> +               puts("start sector is not a valid number\n");
> +               return 1;
> +       }
> +
> +       if (!str2off(argv[3], &len)) {
> +               puts("len is not a valid number\n");
> +               return 1;
> +       }
> +
> +       if (strcmp(argv[1], "lock") == 0)
> +               prot = true;
> +       else if (strcmp(argv[1], "unlock") == 0)
> +               prot = false;
> +       else
> +               return -1;  /* Unknown parameter */
> +
> +       ret = spi_flash_protect(flash, start, len, prot);
> +
> +       return ret == 0 ? 0 : 1;
> +}
> +
>  #ifdef CONFIG_CMD_SF_TEST
>  enum {
>         STAGE_ERASE,
> @@ -540,6 +571,8 @@ static int do_spi_flash(cmd_tbl_t *cmdtp, int flag, int argc,
>                 ret = do_spi_flash_read_write(argc, argv);
>         else if (strcmp(cmd, "erase") == 0)
>                 ret = do_spi_flash_erase(argc, argv);
> +       else if (strcmp(cmd, "protect") == 0)
> +               ret = do_spi_protect(argc, argv);
>  #ifdef CONFIG_CMD_SF_TEST
>         else if (!strcmp(cmd, "test"))
>                 ret = do_spi_flash_test(argc, argv);
> @@ -579,5 +612,7 @@ U_BOOT_CMD(
>         "sf update addr offset|partition len    - erase and write `len' bytes from memory\n"
>         "                                         at `addr' to flash at `offset'\n"
>         "                                         or to start of mtd `partition'\n"
> +       "sf protect lock/unlock sector len      - protect/unprotect 'len' bytes starting\n"
> +       "                                         at address 'sector'\n"
>         SF_TEST_HELP
>  );
> diff --git a/drivers/mtd/spi/sf-uclass.c b/drivers/mtd/spi/sf-uclass.c
> index 350e21a..7663885 100644
> --- a/drivers/mtd/spi/sf-uclass.c
> +++ b/drivers/mtd/spi/sf-uclass.c
> @@ -27,6 +27,14 @@ int spi_flash_erase_dm(struct udevice *dev, u32 offset, size_t len)
>         return sf_get_ops(dev)->erase(dev, offset, len);
>  }
>
> +int spi_flash_protect_dm(struct udevice *dev, u32 offset, size_t len, bool prot)
> +{
> +       if (prot)
> +               return sf_get_ops(dev)->lock(dev, offset, len);
> +       else
> +               return sf_get_ops(dev)->unlock(dev, offset, len);
> +}
> +
>  /*
>   * TODO(sjg@chromium.org): This is an old-style function. We should remove
>   * it when all SPI flash drivers use dm
> diff --git a/drivers/mtd/spi/sf_internal.h b/drivers/mtd/spi/sf_internal.h
> index adfcd89..db46aa8 100644
> --- a/drivers/mtd/spi/sf_internal.h
> +++ b/drivers/mtd/spi/sf_internal.h
> @@ -177,6 +177,10 @@ int spi_flash_cmd_read_status(struct spi_flash *flash, u8 *rs);
>  /* Program the status register */
>  int spi_flash_cmd_write_status(struct spi_flash *flash, u8 ws);
>
> +int stm_is_locked(struct spi_flash *nor, loff_t ofs, u32 len);
> +int stm_lock(struct spi_flash *nor, u32 ofs, u32 len);
> +int stm_unlock(struct spi_flash *nor, u32 ofs, u32 len);
> +
>  /* Read the config register */
>  int spi_flash_cmd_read_config(struct spi_flash *flash, u8 *rc);
>
> @@ -231,6 +235,10 @@ int spi_flash_read_common(struct spi_flash *flash, const u8 *cmd,
>  int spi_flash_cmd_read_ops(struct spi_flash *flash, u32 offset,
>                 size_t len, void *data);
>
> +int spi_flash_cmd_lock_ops(struct spi_flash *flash, u32 offset, size_t len);
> +int spi_flash_cmd_unlock_ops(struct spi_flash *flash, u32 offset, size_t len);
> +int spi_flash_cmd_is_locked_ops(struct spi_flash *flash, u32 offset, size_t len);
> +
>  #ifdef CONFIG_SPI_FLASH_MTD
>  int spi_flash_mtd_register(struct spi_flash *flash);
>  void spi_flash_mtd_unregister(void);
> diff --git a/drivers/mtd/spi/sf_ops.c b/drivers/mtd/spi/sf_ops.c
> index 4ada13f..99fc7f9 100644
> --- a/drivers/mtd/spi/sf_ops.c
> +++ b/drivers/mtd/spi/sf_ops.c
> @@ -268,6 +268,11 @@ int spi_flash_cmd_erase_ops(struct spi_flash *flash, u32 offset, size_t len)
>                 return -1;
>         }
>
> +       if (flash->is_locked(flash, offset, len) > 0) {
> +               printf("offset 0x%x is protected and cannot be erased\n", offset);
> +               return -EINVAL;
> +       }
> +
>         cmd[0] = flash->erase_cmd;
>         while (len) {
>                 erase_addr = offset;
> @@ -310,6 +315,11 @@ int spi_flash_cmd_write_ops(struct spi_flash *flash, u32 offset,
>
>         page_size = flash->page_size;
>
> +       if (flash->is_locked(flash, offset, len) > 0) {
> +               printf("offset 0x%x is protected and cannot be written\n", offset);
> +               return -EINVAL;
> +       }

This flash lock check related to non-dm lock ops so it's look not good
to me as we assigned lock ops for both dm and non-dm cases.

> +
>         cmd[0] = flash->write_cmd;
>         for (actual = 0; actual < len; actual += chunk_len) {
>                 write_addr = offset;
> @@ -348,6 +358,21 @@ int spi_flash_cmd_write_ops(struct spi_flash *flash, u32 offset,
>         return ret;
>  }
>
> +int spi_flash_cmd_lock_ops(struct spi_flash *flash, u32 offset, size_t len)
> +{
> +       return stm_lock(flash, offset, len);
> +}
> +
> +int spi_flash_cmd_unlock_ops(struct spi_flash *flash, u32 offset, size_t len)
> +{
> +       return stm_unlock(flash, offset, len);
> +}
> +
> +int spi_flash_cmd_is_locked_ops(struct spi_flash *flash, u32 offset, size_t len)
> +{
> +       return stm_is_locked(flash, offset, len);
> +}
> +
>  int spi_flash_read_common(struct spi_flash *flash, const u8 *cmd,
>                 size_t cmd_len, void *data, size_t data_len)
>  {
> diff --git a/drivers/mtd/spi/sf_probe.c b/drivers/mtd/spi/sf_probe.c
> index c000c53..3626433 100644
> --- a/drivers/mtd/spi/sf_probe.c
> +++ b/drivers/mtd/spi/sf_probe.c
> @@ -130,6 +130,22 @@ bank_end:
>  }
>  #endif
>
> +int static is_stm(struct spi_slave *spi, struct spi_flash *flash)
> +{
> +       u8 idcode[5];
> +       int ret;
> +
> +       /* Read the ID codes */
> +       ret = spi_flash_cmd(spi, CMD_READ_ID, idcode, sizeof(idcode));
> +       if (ret < 0)
> +               return ret;

This is duplicate we have already got the idcode.

> +
> +       if (idcode[0] == SPI_FLASH_CFI_MFR_STMICRO)
> +               return 1;
> +       else
> +               return 0;
> +}
> +
>  static int spi_flash_validate_params(struct spi_slave *spi, u8 *idcode,
>                                      struct spi_flash *flash)
>  {
> @@ -180,6 +196,13 @@ static int spi_flash_validate_params(struct spi_slave *spi, u8 *idcode,
>  #endif
>         flash->erase = spi_flash_cmd_erase_ops;
>         flash->read = spi_flash_cmd_read_ops;
> +#if defined(CONFIG_SPI_FLASH_STMICRO)
> +       if (is_stm(spi, flash) > 0) {
> +               flash->lock = spi_flash_cmd_lock_ops;
> +               flash->unlock = spi_flash_cmd_unlock_ops;
> +               flash->is_locked = spi_flash_cmd_is_locked_ops;
> +       }
> +#endif
>  #endif
>
>         /* Compute the flash size */
> @@ -482,6 +505,39 @@ int spi_flash_std_erase(struct udevice *dev, u32 offset, size_t len)
>         return spi_flash_cmd_erase_ops(flash, offset, len);
>  }
>
> +int spi_flash_std_lock(struct udevice *dev, u32 offset, size_t len)
> +{
> +       struct spi_flash *flash = dev_get_uclass_priv(dev);
> +       struct spi_slave *spi = dev_get_parent_priv(dev);
> +
> +       if (is_stm(spi, flash) <= 0)
> +               return -EOPNOTSUPP;
> +
> +       return spi_flash_cmd_lock_ops(flash, offset, len);
> +}
> +
> +int spi_flash_std_unlock(struct udevice *dev, u32 offset, size_t len)
> +{
> +       struct spi_flash *flash = dev_get_uclass_priv(dev);
> +       struct spi_slave *spi = dev_get_parent_priv(dev);
> +
> +       if (is_stm(spi, flash) <= 0)
> +               return -EOPNOTSUPP;
> +
> +       return spi_flash_cmd_unlock_ops(flash, offset, len);
> +}
> +
> +int spi_flash_std_is_locked(struct udevice *dev, u32 offset, size_t len)
> +{
> +       struct spi_flash *flash = dev_get_uclass_priv(dev);
> +       struct spi_slave *spi = dev_get_parent_priv(dev);
> +
> +       if (is_stm(spi, flash) <= 0)
> +               return -EOPNOTSUPP;
> +
> +       return spi_flash_cmd_is_locked_ops(flash, offset, len);
> +}
> +
>  int spi_flash_std_probe(struct udevice *dev)
>  {
>         struct spi_slave *slave = dev_get_parent_priv(dev);
> @@ -498,6 +554,11 @@ static const struct dm_spi_flash_ops spi_flash_std_ops = {
>         .read = spi_flash_std_read,
>         .write = spi_flash_std_write,
>         .erase = spi_flash_std_erase,
> +#if defined(CONFIG_SPI_FLASH_STMICRO)
> +       .lock = spi_flash_std_lock,
> +       .unlock = spi_flash_std_unlock,
> +       .is_locked = spi_flash_std_is_locked,
> +#endif
>  };
>
>  static const struct udevice_id spi_flash_std_ids[] = {
> diff --git a/include/spi_flash.h b/include/spi_flash.h
> index 4312d3d..23f30a5 100644
> --- a/include/spi_flash.h
> +++ b/include/spi_flash.h
> @@ -104,6 +104,9 @@ struct spi_flash {
>                         const void *buf);
>         int (*erase)(struct spi_flash *flash, u32 offset, size_t len);
>  #endif
> +       int (*lock)(struct spi_flash *flash, u32 offset, size_t len);
> +       int (*unlock)(struct spi_flash *flash, u32 offset, size_t len);
> +       int (*is_locked)(struct spi_flash *flash, u32 offset, size_t len);
>  };
>
>  struct dm_spi_flash_ops {
> @@ -111,6 +114,9 @@ struct dm_spi_flash_ops {
>         int (*write)(struct udevice *dev, u32 offset, size_t len,
>                      const void *buf);
>         int (*erase)(struct udevice *dev, u32 offset, size_t len);
> +       int (*lock)(struct udevice *dev, u32 offset, size_t len);
> +       int (*unlock)(struct udevice *dev, u32 offset, size_t len);
> +       int (*is_locked)(struct udevice *dev, u32 offset, size_t len);
>  };
>
>  /* Access the serial operations for a device */
> @@ -152,6 +158,20 @@ int spi_flash_write_dm(struct udevice *dev, u32 offset, size_t len,
>   */
>  int spi_flash_erase_dm(struct udevice *dev, u32 offset, size_t len);
>
> +/**
> + * spi_flash_protect_dm() - Protect/unprotect blocks of the SPI flash
> + *
> + * Note that @len must be a muiltiple of the flash sector size.
> + *
> + * @dev:       SPI flash device
> + * @offset:    Offset into device in bytes to start erasing
> + * @len:       Number of bytes to erase
> + * @prot:      True: lock the block or False: unlock the block
> + * @return 0 if OK, -ve on error
> + */
> +int spi_flash_protect_dm(struct udevice *dev, u32 offset, size_t len,
> +                        bool prot);
> +
>  int spi_flash_probe_bus_cs(unsigned int busnum, unsigned int cs,
>                            unsigned int max_hz, unsigned int spi_mode,
>                            struct udevice **devp);
> @@ -183,6 +203,15 @@ static inline int spi_flash_erase(struct spi_flash *flash, u32 offset,
>         return spi_flash_erase_dm(flash->dev, offset, len);
>  }
>
> +static inline int spi_flash_protect(struct spi_flash *flash, loff_t ofs,
> +                                   u32 len, bool prot)
> +{
> +       if (!flash->lock)
> +               return -EOPNOTSUPP;
> +
> +       return spi_flash_protect_dm(flash->dev, ofs, len, prot);
> +}
> +
>  struct sandbox_state;
>
>  int sandbox_sf_bind_emul(struct sandbox_state *state, int busnum, int cs,
> @@ -225,6 +254,18 @@ static inline int spi_flash_erase(struct spi_flash *flash, u32 offset,
>  {
>         return flash->erase(flash, offset, len);
>  }
> +
> +static inline int spi_flash_protect(struct spi_flash *flash, u32 ofs, u32 len,
> +                                   bool prot)
> +{
> +       if (!flash->lock)
> +               return -EOPNOTSUPP;
> +
> +       if (prot)
> +               return flash->lock(flash, ofs, len);
> +       else
> +               return flash->unlock(flash, ofs, len);
> +}
>  #endif
>
>  void spi_boot(void) __noreturn;
> --
> 1.9.1
>

I will come back for this patch changes what I thought, probably lock
ops shouldn't be separate for dm and non-dm as we have different ops
based on the flash itself.

Jagan,

On Tue, Nov 3, 2015 at 12:51 PM, Jagan Teki <jteki@openedev.com> wrote:

>> +       if (flash->is_locked(flash, offset, len) > 0) {
>> +               printf("offset 0x%x is protected and cannot be written\n", offset);
>> +               return -EINVAL;
>> +       }
>
> This flash lock check related to non-dm lock ops so it's look not good
> to me as we assigned lock ops for both dm and non-dm cases.

This is getting totally ridiculous: you have been seeing this patch
series so many times and now you say this is not OK and you don't give
me any clue as to what you would like me to change.

That's why I proposed earlier that you take the non-SPI patches first
and then we can rework the SPI flash pieces.

> I will come back for this patch changes what I thought, probably lock
> ops shouldn't be separate for dm and non-dm as we have different ops
> based on the flash itself.

I wish we can make progress on this. Not sure how to proceed though.

Fabio,

On 3 November 2015 at 20:29, Fabio Estevam <festevam@gmail.com> wrote:
> Jagan,
>
> On Tue, Nov 3, 2015 at 12:51 PM, Jagan Teki <jteki@openedev.com> wrote:
>
>>> +       if (flash->is_locked(flash, offset, len) > 0) {
>>> +               printf("offset 0x%x is protected and cannot be written\n", offset);
>>> +               return -EINVAL;
>>> +       }
>>
>> This flash lock check related to non-dm lock ops so it's look not good
>> to me as we assigned lock ops for both dm and non-dm cases.
>
> This is getting totally ridiculous: you have been seeing this patch
> series so many times and now you say this is not OK and you don't give
> me any clue as to what you would like me to change.

Nothing is worse than before or now - as spi-flash code is configured
both the dm and non-dm cases and it's been thinking job as I have
planning for proper design[1] to go ahead. I understand you sent
multiple versions but I already told that "please don't send the next
version" until the current version discussion finishes.

[1] http://git.denx.de/?p=u-boot/u-boot-spi.git;a=shortlog;h=refs/heads/next-spi-nor

>
> That's why I proposed earlier that you take the non-SPI patches first
> and then we can rework the SPI flash pieces.
>
>> I will come back for this patch changes what I thought, probably lock
>> ops shouldn't be separate for dm and non-dm as we have different ops
>> based on the flash itself.
>
> I wish we can make progress on this. Not sure how to proceed though.

thanks!

Jagan,

On Tue, Nov 3, 2015 at 1:11 PM, Jagan Teki <jteki@openedev.com> wrote:

> Nothing is worse than before or now - as spi-flash code is configured
> both the dm and non-dm cases and it's been thinking job as I have
> planning for proper design[1] to go ahead. I understand you sent
> multiple versions but I already told that "please don't send the next
> version" until the current version discussion finishes.

How do I know that you finished reviewing a specific version and I am
ready to send another one?

Anyway, let's focus on the remaining technical issues.

What exactly do you want me to change?

You say: "This flash lock check related to non-dm lock ops so it's look not good
to me as we assigned lock ops for both dm and non-dm cases."

How can I make it look good then?

On Tue, Nov 03, 2015 at 08:21:32PM +0530, Jagan Teki wrote:

[snip]
> I will come back for this patch changes what I thought, probably lock
> ops shouldn't be separate for dm and non-dm as we have different ops
> based on the flash itself.

As we work on consolidation and moving everything over to DM we'll have
funny cases like this.  But we shouldn't let that stop us from making
forward progress either.  The code itself looks reasonable enough today
as a starting point, so lets pick it up and move from there, thanks!

On Tue, Nov 3, 2015 at 1:20 PM, Tom Rini <trini@konsulko.com> wrote:
> On Tue, Nov 03, 2015 at 08:21:32PM +0530, Jagan Teki wrote:
>
> [snip]
>> I will come back for this patch changes what I thought, probably lock
>> ops shouldn't be separate for dm and non-dm as we have different ops
>> based on the flash itself.
>
> As we work on consolidation and moving everything over to DM we'll have
> funny cases like this.  But we shouldn't let that stop us from making
> forward progress either.  The code itself looks reasonable enough today
> as a starting point, so lets pick it up and move from there, thanks!

Yes, we can always make incremental patches to improve things.

Thanks, Tom!