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

["chenxiang (M)" <chenxiang66@hisilicon.com>]

Hi Jungseung Lee,

在 2020/1/17 23:06, Jungseung Lee 写道:
> Hi, Tudor,
>
> On Tue, Jan 14, 2020 at 7:49 PM <Tudor.Ambarus@microchip.com> wrote:
>> Hi, Jungseung,
>>
>> Thanks for working on this.
>>
>> On Monday, January 13, 2020 7:59:06 AM EET Jungseung Lee wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
>>> content is safe
>>>
>>> Currently, we are supporting block protection only for
>>> flash chips with 3 block protection bits in the SR register.
>>> This patch enables block protection support for some flash with
>>> 4 block protection bits(bp0-3).
>> Some? Isn't this generic for all the flashes that support BP0-3?
>>
> This one would be a generic solution to support BP0-3 on Status Register.
> >From my study, this covers all the flashes listed on spi-nor.c that
> have BP0-3 bit on SR.
> It looks like I have to change this description.
>
> Note that it is NOT for some flashes that have BP0-3 in another register.
> As you know, just like SPI_NOR_HAS_TB did.
>
>>> Signed-off-by: Jungseung Lee <js07.lee@samsung.com>
>>> ---
>>> v3 :
>>>    Fix wrong ofs calculation on v2 patch
>>> v2 :
>>>    Add sample table portion about 4bit block protection on the comment
>>>    Trivial coding style change
>>>
>>>   drivers/mtd/spi-nor/spi-nor.c | 127 +++++++++++++++++++++++++++++-----
>>>   include/linux/mtd/spi-nor.h   |   8 +++
>>>   2 files changed, 119 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
>>> index e3da6a8654a8..7e8af6c4fdfa 100644
>>> --- a/drivers/mtd/spi-nor/spi-nor.c
>>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>>> @@ -238,6 +238,14 @@ struct flash_info {
>>>                                           * status register. Must be used
>>> with * SPI_NOR_HAS_TB.
>>>                                           */
>>> +#define SPI_NOR_HAS_BP3                BIT(17) /*
>>> +                                        * Flash SR has 4 bit fields (BP0-3)
>>> +                                        * for block protection.
>>> +                                        */
>>> +#define SPI_NOR_BP3_SR_BIT6    BIT(18) /*
>>> +                                        * BP3 is bit 6 of status register.
>>> +                                        * Must be used with
>> Are we safe to replace SPI_NOR_TB_SR_BIT6 and SPI_NOR_BP3_SR_BIT6 with a
>> SPI_NOR_SR_TB_BIT6_BP3_BIT5? Or maybe with a SPI_NOR_SR_BP3_BIT6_TB_BIT5, how
>> is more convenient?
>>
> Let's think about some flash in which BP0-3 exists in the status
> register but TB exists in another register.
> for example, mx25u12835f.
> I haven't tested yet, but according to the datasheet, I think this
> patch can support 4bit block protection for the flash.
>
> In order to embrace the case, how about letting them as It is.
> Is there any suggestion?

In my board(hisilicon D06) there is  a mx25u12835f Flash, and i enable 
SPI_NOR_HAS_LOCK and SPI_NOR_HAS_BP3 which is not enabled in spi-nor.c, 
find it seems not work:

Euler:~ # ls /dev/mtd0
/dev/mtd0
Euler:~ # ./flash_lock -i /dev/mtd0
Device: /dev/mtd0
Start: 0
Len: 0x1000000
Lock status: unlocked
Return code: 0
Euler:~ #
Euler:~ # ./flash_lock /dev/mtd0
flash_lock: error!: could not lock device: /dev/mtd0

             error 5 (Input/output error)
Euler:~ # ./flash_lock -i /dev/mtd0
Device: /dev/mtd0
Start: 0
Len: 0x1000000
Lock status: locked
Return code: 1
Euler:~ #

I find that in function spi_nor_write_sr_and_check(), if spi_nor has 
16bit SR, spi_nor_write_16bit_sr_and_check() will be called, if not 
spi_nor_write_sr1_and_check() will be called.
For mx25u12835f, it just has 8bit SR, but SNOR_F_HAS_16BIT_SR is set as 
default, so it causes above issue;
Even hack the code to not set flag SNOR_F_HAS_16BIT_SR, it seems that 
lock is still not valided as Program/Erase still success even if it is 
locked:

Euler:~ # ./flash_lock -i /dev/mtd0
Device: /dev/mtd0
Start: 0
Len: 0x1000000
Lock status: unlocked
Return code: 0
Euler:~ #
Euler:~ # ./mtd_debug erase /dev/mtd0 0xe40000 4096
Erased 4096 bytes from address 0x00e40000 in flash
Euler:~ #
Euler:~ # ./flash_lock /dev/mtd0
Euler:~ #
Euler:~ # ./flash_lock -i /dev/mtd0
Device: /dev/mtd0
Start: 0
Len: 0x1000000
Lock status: locked
Return code: 1
Euler:~ #
Euler:~ # ./mtd_debug erase /dev/mtd0 0xe40000 4096
Erased 4096 bytes from address 0x00e40000 in flash
Euler:~ # ./flash_lock -i /dev/mtd0
Device: /dev/mtd0
Start: 0
Len: 0x1000000
Lock status: locked
Return code: 1
Euler:~ #
Euler:~ #


Thanks,
chenxiang


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



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