[U-Boot] [PATCH 1/5] spi_sf: Skip the erasing of protected sectors

[Otavio Salvador]

On Thu, Sep 3, 2015 at 6:35 AM, Jagan Teki <jteki at openedev.com> wrote:
> On 2 September 2015 at 20:11, Otavio Salvador <otavio at ossystems.com.br> wrote:
>> 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.
>>
>> Introduce the CONFIG_SPI_NOR_PROTECTION option that can be selected
>> by systems that want to protect selected regions of SPI NOR flash.
>>
>> Signed-off-by: Otavio Salvador <otavio at ossystems.com.br>
>> ---
>>
>>  README                     |  4 ++
>>  drivers/mtd/spi/sf_ops.c   | 91 ++++++++++++++++++++++++++++++++++++++++++++--
>>  drivers/mtd/spi/sf_probe.c |  2 +
>>  3 files changed, 94 insertions(+), 3 deletions(-)
>>
>> diff --git a/README b/README
>> index 1acc355..18d61a6 100644
>> --- a/README
>> +++ b/README
>> @@ -2751,6 +2751,10 @@ CBFS (Coreboot Filesystem) support
>>                 Timeout for waiting until spi transfer completed.
>>                 default: (CONFIG_SYS_HZ/100)     /* 10 ms */
>>
>> +               CONFIG_SPI_NOR_PROTECTION
>> +               Enable the built-in protection mechanism provided by the
>> +               BP2, BP1 and BP0 bits from the status register.
>
> Does these bits specific for micron flash?

They seem to be generic. I tested this on two part numbers from
different vendors: Micron M25P32 and SST25VF032B. In this case the
protection bits (BP) have the same meaning/layout.

If you prefer we can call this option CONFIG_SPI_NOR_PROTECTION_STM in
case we have SPI NOR flashes with a different BP bits structure as
done in the kernel.

>> +
>>  - FPGA Support: CONFIG_FPGA
>>
>>                 Enables FPGA subsystem.
>> diff --git a/drivers/mtd/spi/sf_ops.c b/drivers/mtd/spi/sf_ops.c
>> index 900ec1f..a7a07f9 100644
>> --- a/drivers/mtd/spi/sf_ops.c
>> +++ b/drivers/mtd/spi/sf_ops.c
>> @@ -264,6 +264,84 @@ int spi_flash_write_common(struct spi_flash *flash, const u8 *cmd,
>>         return ret;
>>  }
>>
>> +#ifdef CONFIG_SPI_NOR_PROTECTION
>> +static int is_protected(struct spi_flash *flash, u32 offset)
>> +{
>> +       int total_sector, ret, protected_sector;
>> +       bool *protected;
>> +       u8 sr;
>> +
>> +       total_sector = flash->size / flash->erase_size;
>> +       protected = calloc(1, total_sector);
>> +       if (!protected)
>> +               return -ENOMEM;
>> +
>> +       ret = spi_flash_cmd_read_status(flash, &sr);
>> +       if (ret < 0)
>> +               goto free_protected;
>> +
>> +       /* Extract the protection bits: BP2, BP1 and BP0 */
>> +       sr = (sr & 0x1c) >> 2;
>
> Does this masking specific to particular flash part or it's generic
> way, please elaborate.

Seems to be generic.

>> +
>> +       switch (sr) {
>> +       case 0:
>> +               protected_sector = 0;
>> +       break;
>> +
>> +       case 1:
>> +               protected_sector = total_sector / 64;
>> +       break;
>> +
>> +       case 2:
>> +               protected_sector = total_sector / 32;
>> +       break;
>> +
>> +       case 3:
>> +               protected_sector = total_sector / 16;
>> +       break;
>> +
>> +       case 4:
>> +               protected_sector = total_sector / 8;
>> +       break;
>> +
>> +       case 5:
>> +               protected_sector = total_sector / 4;
>> +       break;
>> +
>> +       case 6:
>> +               protected_sector = total_sector / 2;
>> +       break;
>> +
>> +       case 7:
>> +               protected_sector = total_sector;
>> +       break;
>> +
>> +       default:
>> +               ret = -EINVAL;
>> +               goto free_protected;
>> +       }
>> +
>> +       while (protected_sector) {
>> +               protected[total_sector - protected_sector] = 1;
>> +               protected_sector--;
>> +       }
>> +
>> +       if (protected[offset/flash->erase_size])
>> +               return 1;
>> +       else
>> +               return 0;
>> +
>> +free_protected:
>> +       free(protected);
>> +       return ret;
>> +}
>> +#else
>> +static int is_protected(struct spi_flash *flash, u32 offset)
>> +{
>> +       return 0;
>> +}
>> +#endif
>> +
>>  int spi_flash_cmd_erase_ops(struct spi_flash *flash, u32 offset, size_t len)
>>  {
>>         u32 erase_size, erase_addr;
>> @@ -294,10 +372,17 @@ int spi_flash_cmd_erase_ops(struct spi_flash *flash, u32 offset, size_t len)
>>                 debug("SF: erase %2x %2x %2x %2x (%x)\n", cmd[0], cmd[1],
>>                       cmd[2], cmd[3], erase_addr);
>>
>> -               ret = spi_flash_write_common(flash, cmd, sizeof(cmd), NULL, 0);
>> -               if (ret < 0) {
>> -                       debug("SF: erase failed\n");
>> +               if (is_protected(flash, offset) > 0) {
>> +                       printf("Skipping to erase protected offset 0x%x\n",
>> +                              offset);
>>                         break;
>> +               } else {
>> +                       ret = spi_flash_write_common(flash, cmd, sizeof(cmd),
>> +                                                    NULL, 0);
>> +                       if (ret < 0) {
>> +                               debug("SF: erase failed\n");
>> +                               break;
>> +                       }
>
> So this seems to be locking sector area which is been protected, why
> it's needed on erase? why can't we do it while probe? any side

We do not want to erase a sector that is protected, so that's why we
skip it during the erase process.

> effects. and also do we have any way to unlock the protected sectors?
> does that really a need?

Yes, we can lock and unlock sectors using the
spi_flash_cmd_write_status() function. Please see patch 5/5.


-- 
Otavio Salvador                             O.S. Systems
http://www.ossystems.com.br        http://code.ossystems.com.br
Mobile: +55 (53) 9981-7854            Mobile: +1 (347) 903-9750