Message-ID: <bec5b899-fbd2-1c29-611c-654f17e63dbf@microchip.com>
Date:   Thu, 1 Oct 2020 07:07:42 +0000
From:   <Tudor.Ambarus@...rochip.com>
To:     <michael@...le.cc>
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

On 10/1/20 1:38 AM, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hi Tudor,

Hi, Michael,

> 
>>> 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.

I like the idea of having all locking related functions placed in a single
place, thus the global_unlock() should be inside locking_ops struct.
You can update params->locking_ops. If we have vendor specific locking_ops
we can set them via:
	nor->manufacturer->fixups->default_init(nor);
or if they are flash specific, with a smaller granularity, via:
	nor->info->fixups->default_init(nor); 

> 
> 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

I don't like that we have so many flags, so I try to avoid introducing new
ones as best as I can. Checking for null pointer should suffice.

> writes
> to the status register, one to clear SPRL and then one to clear the BP
> bits.

SPR is pretty common, also available in Winbond and Micron. I see that
it is a non-volatile bit for these vendors. NV bits are of concern 
because you can wear them down with multiple set/clear ops. We have to
think how we'll deal with NV bits in general in SPI NOR.

> 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.

Yes, sorry for that, my bad. I can help if you want, I can go through
datasheets to.

Let's not introduce the global unlock ops commands yet, let's just
solve the backward compatibility issues with the existing code. We can
add new opcodes later on.

Also, I can scratch a patch with the global_unlock() hook if you prefer,
so that we can talk on code.

Cheers,
ta

> 
> [1] https://www.adestotech.com/wp-content/uploads/doc3668.pdf
> 
> -michael

Powered by blists - more mailing lists