Re: [SPAM] Re: [PATCH v3 2/3] mtd: spi-nor: add 4bit block protection support

From: Vignesh Raghavendra <vigneshr@ti.com>
To: Michael Walle <michael@walle.cc>
Cc: js07.lee@gmail.com, linux-mtd@lists.infradead.org,
	tudor.ambarus@microchip.com, Jungseung Lee <js07.lee@samsung.com>
Subject: Re: [SPAM] Re: [PATCH v3 2/3] mtd: spi-nor: add 4bit block protection support
Date: Mon, 3 Feb 2020 23:01:56 +0530	[thread overview]
Message-ID: <62b66868-5a18-4fc4-f18e-6b4c39fdb65d@ti.com> (raw)
In-Reply-To: <9966c63930b47103e78873fb82dc16f2@walle.cc>

Hi,

On 2/3/2020 8:08 PM, Michael Walle wrote:
> Am 2020-02-03 14:56, schrieb Vignesh Raghavendra:

[...]
>>
>> Flash devices have variable sector size, 64KB, 128KB or 256KB... While
>> mapping of number of sectors locked to BP bits is dependent on rules 1
>> to 3 you mentioned below, the size or area of flash protected depends on
>> sector size.
> 
> Is there any flash device which has another sector size != 64KiB for the
> BP bits?

I cannot seem to find BP scheme for Micron mt35x series but they all 
have 128KB sectors with 4 bit block protection scheme:
https://www.micron.com/-/media/client/global/documents/products/data-sheet/nor-flash/serial-nor/mt35x_xccela/die-rev-a/xccela_flash_memory_data_sheet_brief.pdf

Another example would be s25fl512s with 256KB sector with 3 bit BP:
https://www.cypress.com/file/177971/download

> 
>> So, the current formula in spi-nor.c (ignoring TB and other boilerplate):
>>
>> pow = ilog2(mtd->size) - ilog2(lock_len);
>> val = mask - (pow << shift);
>>
>> This works only for devices with 64KB sector size as 8MB flash with 64KB
>> sector size would have 128 sectors (BP0-2 => 0b111 => 2^7).
> 
> It also only works with flashes >= 4MiB. See fix below. And IMHO this is
> exactly
> the same "problem" the flashes with 4 BP bits have.
> 

Yes agree, but that patch is assumes sector size to be 64KB and 
once generalized would mostly be equivalent to below formula.

>> A more generic formula would be:
>>
>> Find n where 2^(n - 1) = len/sector-size
>> OR 2^ (n - 1) = len * n_sectors / mtd->size
>>
>> Which solves to:
>>
>> pow = ilog2(mtd->size) - ilog2(lock_len);
>> val = ilog2(nor->n_sectors) + 1 - pow;
>>
>> I see this is what Jungseung has tried to implement here.  Please
>> correct me if I got this wrong.
>>
>> This, combined with point (3) below should provide a generic
>> implementation that should support a wide variety of flashes.
>>
>> Of course, there are always exceptions and they need to be handled using
>> custom hooks.
>>
>> I don't have the patch that you shared with Jungseung. I would greatly
>> appreciate, if you and Jungseung could work on patch with above logic as
>> well as fixes to handle overflow case?
> 
> https://lore.kernel.org/linux-mtd/20200123170130.8289-1-michael@walle.cc/
> 
> As I said, that should work for both 3 and 4 bits. But be aware that this
> is an RFC and I've just tested it in in userspace, like the calculation
> of the bits and transferred that into the driver. So one would actually
> have to test that. But apparently no one had a even applied it.
> 

Sorry, I haven't looked at that patch. But I think better alternative would
be extend this patch to support overflow case.

Would it be possible to test this patch on flashes <32MBit, if you have
access to?

Regards
Vignesh

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/