[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <a82d1ce203383af149ed77c5fd6c8985@walle.cc>
Date: Thu, 01 Oct 2020 00:38:42 +0200
From: Michael Walle <michael@...le.cc>
To: Tudor.Ambarus@...rochip.com
Cc: linux-mtd@...ts.infradead.org, linux-kernel@...r.kernel.org,
vigneshr@...com, richard@....at, boris.brezillon@...labora.com,
miquel.raynal@...tlin.com
Subject: Re: [PATCH v3] mtd: spi-nor: keep lock bits if they are non-volatile
Hi Tudor,
>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>> index cc68ea84318e..fd1c36d70a13 100644
>> --- a/drivers/mtd/spi-nor/core.c
>> +++ b/drivers/mtd/spi-nor/core.c
>> @@ -2916,20 +2916,38 @@ static int spi_nor_quad_enable(struct spi_nor
>> *nor)
>> }
>>
>> /**
>> - * spi_nor_unlock_all() - Unlocks the entire flash memory array.
>> + * spi_nor_global_unprotect() - Perform a global unprotect of the
>> memory area.
>> * @nor: pointer to a 'struct spi_nor'.
>> *
>> * Some SPI NOR flashes are write protected by default after a
>> power-on reset
>> * cycle, in order to avoid inadvertent writes during power-up.
>> Backward
>> * compatibility imposes to unlock the entire flash memory array at
>> power-up
>> - * by default.
>> + * by default. Do it only for flashes where the block protection bits
>> + * are volatile, this is indicated by SNOR_F_NEED_UNPROTECT.
>> + *
>> + * We cannot use spi_nor_unlock(nor->params.size) here because there
>> are
>> + * legacy devices (eg. AT25DF041A) which need a "global unprotect"
>> command.
>> + * This is done by writing 0b0x0000xx to the status register. This
>> will also
>> + * work for all other flashes which have these bits mapped to BP0 to
>> BP3.
>> + * The top most bit is ususally some kind of lock bit for the block
>> + * protection bits.
>> */
>> -static int spi_nor_unlock_all(struct spi_nor *nor)
>> +static int spi_nor_global_unprotect(struct spi_nor *nor)
>> {
>> - if (nor->flags & SNOR_F_HAS_LOCK)
>> - return spi_nor_unlock(&nor->mtd, 0, nor->params->size);
>> + int ret;
>>
>> - return 0;
>> + dev_dbg(nor->dev, "unprotecting entire flash\n");
>> + ret = spi_nor_read_sr(nor, nor->bouncebuf);
>> + if (ret)
>> + return ret;
>> +
>> + nor->bouncebuf[0] &= ~SR_GLOBAL_UNPROTECT_MASK;
>> +
>> + /*
>> + * Don't use spi_nor_write_sr1_and_check() because writing the
>> status
>> + * register might fail if the flash is hardware write protected.
>> + */
>> + return spi_nor_write_sr(nor, nor->bouncebuf, 1);
>> }
>
> This won't work for all the flashes. You use a GENMASK(5, 2) to clear
> the Status Register even for BP0-2 flashes and you end up clearing
> BIT(5)
> which can lead to side effects.
>
> We should instead introduce a nor->params->locking_ops->global_unlock()
> hook
> for the flashes that have special opcodes that unlock all the flash
> blocks,
> or for the flashes that deviate from the "clear just your BP bits"
> rule.
Wouldn't it make more sense to just set params->locking_ops for these
flashes
to different functions? or even provide a spi_nor_global_unprotect_ops
in
core.c and these flashes will just set them. there is no individual
sector
range lock for these chips. just a lock all or nothing.
If it is more common and not just one vendor it might also make sense to
add
a seperate flag which will then set the locking ops to
spi_nor_global_unprotect_ops, also it seems that there have to be two
writes
to the status register, one to clear SPRL and then one to clear the BP
bits.
See for example [1] Table 9-2. I'll have to go through the datasheets
again
to see what flashes just have a global (un)protect.
[1] https://www.adestotech.com/wp-content/uploads/doc3668.pdf
-michael
Powered by blists - more mailing lists