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

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

Hi,

在 2020/3/4 13:20, Jungseung Lee 写道:
> Hi,
>
> 2020-02-11 (화), 15:52 +0800, chenxiang (M):
>> 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:
>>
> Could you dump sr register value?
> But if you are using old kernel or the flash doesn't have FSR error
> bits, erase/program operation failure couldn't be detected.
>
> refer this one.
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v5.6-rc4&id=20ccb993f29bd6ad17699dd0b349db086e3ca719

The kernel i used is 5.5-rc4, and it includes the above patch.
I also checked the datasheet of mx25u12835f flash, and it doesn't have 
FSR register, but it has other register SCUR(Security Reigster, 
bit6&bit5 are
E_FAIL/P_FAIL which i think the function of them are like the bit5&bit4 
of FSR register) .


>
> I am planning to post new patch for 4bit Block protection soon. I would
> appreciate it if you check there too.

It's my pleasure to check it.

>
> Thanks,
>
>> 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
>>>
> https://protect2.fireeye.com/url?k=81f52bac-dc2196e8-81f4a0e3-0cc47a31381a-cfeff2fb749261ae&u=http://lists.infradead.org/mailman/listinfo/linux-mtd/
>>> .
>>>
>>
>>
>
> .
>



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