[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <f788aaf1-71a6-85c3-1e57-2c9c0099ea83@microchip.com>
Date: Thu, 3 Jun 2021 05:08:29 +0000
From: <Tudor.Ambarus@...rochip.com>
To: <michael@...le.cc>
CC: <linux-mtd@...ts.infradead.org>, <linux-kernel@...r.kernel.org>,
<p.yadav@...com>, <miquel.raynal@...tlin.com>, <richard@....at>,
<vigneshr@...com>
Subject: Re: [PATCH v4 4/4] mtd: spi-nor: otp: implement erase for Winbond and
similar flashes
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@...rochip.com:
>> 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@...le.cc>
>>> ---
>>> 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
Powered by blists - more mailing lists