Re: [PATCH v4 4/4] mtd: spi-nor: otp: implement erase for Winbond and similar flashes

[Tudor.Ambarus]

On 6/1/21 4:30 PM, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Am 2021-05-31 10:56, schrieb Tudor.Ambarus@xxxxxxxxxxxxx:
>> On 5/21/21 10:40 PM, Michael Walle wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know
>>> the content is safe
>>>
>>> Winbond flashes with OTP support provide a command to erase the OTP
>>> data. This might come in handy during development.
>>>
>>> This was tested with a Winbond W25Q32JW on a LS1028A SoC with the
>>> NXP FSPI controller.
>>>
>>> Signed-off-by: Michael Walle <michael@xxxxxxxx>
>>> ---
>>>  drivers/mtd/spi-nor/core.c    |  2 +-
>>>  drivers/mtd/spi-nor/core.h    |  4 ++
>>>  drivers/mtd/spi-nor/otp.c     | 83
>>> +++++++++++++++++++++++++++++++++++
>>>  drivers/mtd/spi-nor/winbond.c |  1 +
>>>  4 files changed, 89 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>>> index bd2c7717eb10..9551effb6a44 100644
>>> --- a/drivers/mtd/spi-nor/core.c
>>> +++ b/drivers/mtd/spi-nor/core.c
>>> @@ -1318,7 +1318,7 @@ static u32 spi_nor_convert_addr(struct spi_nor
>>> *nor, loff_t addr)
>>>  /*
>>>   * Initiate the erasure of a single sector
>>>   */
>>> -static int spi_nor_erase_sector(struct spi_nor *nor, u32 addr)
>>> +int spi_nor_erase_sector(struct spi_nor *nor, u32 addr)
>>>  {
>>>         int i;
>>>
>>> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
>>> index 28a2e0be97a3..9398a8738857 100644
>>> --- a/drivers/mtd/spi-nor/core.h
>>> +++ b/drivers/mtd/spi-nor/core.h
>>> @@ -207,6 +207,7 @@ struct spi_nor_otp_organization {
>>>   * @read:      read from the SPI NOR OTP area.
>>>   * @write:     write to the SPI NOR OTP area.
>>>   * @lock:      lock an OTP region.
>>> + * @erase:     erase an OTP region.
>>>   * @is_locked: check if an OTP region of the SPI NOR is locked.
>>>   */
>>>  struct spi_nor_otp_ops {
>>> @@ -214,6 +215,7 @@ struct spi_nor_otp_ops {
>>>         int (*write)(struct spi_nor *nor, loff_t addr, size_t len,
>>>                      const u8 *buf);
>>>         int (*lock)(struct spi_nor *nor, unsigned int region);
>>> +       int (*erase)(struct spi_nor *nor, loff_t addr);
>>
>> maybe better:
>> int (*erase)(struct spi_nor *nor, loff_t addr, size_t len);
> 
> Right now these ops represent what is sent to the hardware. The address
> is what is really sent over the wire. The read and write has the same
> semantics (and limitations, like you can't cross region boundaries etc).
> 
> As there is only one kind of method to access OTP right now, I wouldn't
> want to prematurely optimize it for future ones. It is easy to change
> afterwards if there is need to. And supplying a length right now would
> mean to have a loop and some checks in the erase() op; where it should
> really be "erase one region" and not "erase multiple ones". Mapping
> from multiple to one should be the duty of the otp core code.

Indeed. Duty of the SPI NOR otp core, because I see now that mtd passes
this responsibility with its OTPGETREGIONCOUNT, and how it handles its
otp_info struct.

> 
> So, one could argue about the parameter "unsigned in region" vs.
> "loff_t addr" because both represent the same thing. But
> spi_nor_otp_erase_secr() should really take the address, because
> that is what the opcode takes as an argument. Thus, changing the
> argument of the .erase to "unsigned int region" you couldn't just
> assign spi_nor_otp_erase_secr to .erase but you'd need to have a
> small wrapper which converts the argument.
> 

Ok. Looks good.

> So IMHO future will tell if we have to change this to somewhat
> more generic.
> 
> -michael