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

[Jungseung Lee <js07.lee@xxxxxxxxxxx>]

Hi,

2020-03-04 (수), 16:36 +0800, chenxiang (M):
> 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@xxxxxxxxxxxxx>
> > > > 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@xxxxxxxxxxx>
> > > > > > ---
> > > > > > 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) .
> 

So that is the one not having FSR register. If SCUR is used elsewhere
enough, we could add a feature for it later.

Thanks,
> 
> > 
> > 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/