Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date:   Thu, 20 May 2021 17:21:34 +0200
From:   Michael Walle <michael@walle.cc>
To:     Pratyush Yadav <p.yadav@ti.com>
Cc:     linux-mtd@lists.infradead.org, linux-kernel@vger.kernel.org,
        Tudor Ambarus <tudor.ambarus@microchip.com>,
        Miquel Raynal <miquel.raynal@bootlin.com>,
        Richard Weinberger <richard@nod.at>,
        Vignesh Raghavendra <vigneshr@ti.com>
Subject: Re: [PATCH v2] mtd: spi-nor: implement OTP erase for Winbond and
 similar flashes
In-Reply-To: <20210520134940.fpyt52mwpdotrp4a@ti.com>
References: <20210510202056.30000-1-michael@walle.cc>
 <20210520122256.fhkzpqmu7nxwjoqt@ti.com>
 <20a7e9725d54c9566ca06c062c15f015@walle.cc>
 <20210520134940.fpyt52mwpdotrp4a@ti.com>
User-Agent: Roundcube Webmail/1.4.11
Message-ID: <bdf79e0b9cbe291a48ab27b69f15eb04@walle.cc>
Precedence: bulk

Am 2021-05-20 15:49, schrieb Pratyush Yadav:
> On 20/05/21 03:20PM, Michael Walle wrote:
>> Am 2021-05-20 14:22, schrieb Pratyush Yadav:
>> > On 10/05/21 10:20PM, Michael Walle wrote:
..
>> > > +int spi_nor_otp_erase_secr(struct spi_nor *nor, loff_t addr)
>> > > +{
>> > > +	int ret;
>> > > +
>> > > +	ret = spi_nor_write_enable(nor);
>> > > +	if (ret)
>> > > +		return ret;
>> > > +
>> > > +	if (nor->spimem) {
>> > > +		struct spi_mem_op op =
>> > > +			SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_ESECR, 0),
>> > > +				   SPI_MEM_OP_ADDR(3, addr, 0),
>> >
>> > Only 3 address bytes needed? Can the OTP region ever require 4 byte
>> > addressing? For example, say the flash is switched to 4 byte addressing
>> > for the main region. Would it still expect 3 byte addressing for OTP
>> > ops?
>> 
>> It seems you're right. Looking at larger flashes there are sometimes
>> 4 bytes. See for example ch 8.2.44
>> 
>> https://www.winbond.com/resource-files/w25m512jwxiq%20spi%20rev%20c%2012242018.pdf
>> 
>> Thus it seems it should be fixed for the programming and reading,
>> too. Unfortunately, I cannot test any of this.
> 
> I don't think any such flash is supported currently, right? So in that
> case we won't at least introduce any new regressions when making this
> untested change. Whenever someone adds support for one of these 
> flashes,
> we can ask them to test this as well.

yep.

..

>> > > +static int spi_nor_mtd_otp_lock_erase(struct mtd_info *mtd, loff_t
>> > > from,
>> >
>> > spi_nor_mtd_otp_lock_or_erase()? Or would it make it too long?
>> 
>> I'm fine with both, its just that the read/write doesn't have an
>> "or" neither ;)
>> 
>> > Anyway, maybe I am bikeshedding too much, but I don't like that you
>> > combine two completely independent operations into the same function
>> > because they have some common parts. You should make them two separate
>> > functions and see how many of the common parts can be split into
>> > independent subroutines.
>> 
>> Given that the whole boilerplate before and after the erase/lock is
>> exactly the same, even the while loop is the same, I don't see how
>> it can easily be split. Well, you could rename the function to some
>> generic spi_nor_mtd_walk() - which would imply it might also be
>> used for read/write, which is not true - and provide a callback
>> function. But I don't see how this is would make it easier to read.
>> And this is just an implemention local to this module.
> 
> My suggestion was to make two copies of the same code, and then see if
> you can consolidate some in a clean subroutine. If that is not 
> possible,
> then you can just leave the code duplicated in two places. It is not
> that much duplication so it should be fine IMO.
> 
> But I won't press too much on this point. I will leave it to your
> judgement on what works better. Just want to make sure you understand 
> my
> point completely.

I get your point. But I really don't like the code duplication.

>> > > +				      size_t len, bool is_erase)
>> > >  {
>> > >  	struct spi_nor *nor = mtd_to_spi_nor(mtd);
>> > >  	const struct spi_nor_otp_ops *ops = nor->params->otp.ops;
>> > >  	const size_t rlen = spi_nor_otp_region_len(nor);
>> > >  	unsigned int region;
>> > > +	loff_t rstart;
>> > >  	int ret;
>> > >
>> > >  	if (from < 0 || (from + len) > spi_nor_otp_size(nor))
>> > > @@ -337,7 +382,13 @@ static int spi_nor_mtd_otp_lock(struct mtd_info
>> > > *mtd, loff_t from, size_t len)
>> > >
>> > >  	while (len) {
>> > >  		region = spi_nor_otp_offset_to_region(nor, from);
>> > > -		ret = ops->lock(nor, region);
>> > > +
>> > > +		if (is_erase) {
>> > > +			rstart = spi_nor_otp_region_start(nor, region);
>> > > +			ret = ops->erase(nor, rstart);
>> >
>> > This further highlights my point. There are subtle differences between
>> > erase and lock and having them in the same function might not be the
>> > best idea.

Maybe the argument for the locking is wrong. Future will tell. The
start address of a region and the number of a region is actually
equivalent. So maybe ->lock should also take the start address.
But then you'd go from address -> region -> address -> region.

At the moment its modelled after how winbond and macronix flashes
implement these ops.

See here for an old version of the support for macronix(-like)
flashes:
https://lore.kernel.org/linux-mtd/20200911222634.31804-4-michael@walle.cc/

-michael