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

[Jungseung Lee <js07.lee@samsung.com>]

Hi, Tudor,

2020-01-18 (Sat), 00:06 +0900, 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?
> 

Hmm.. I again have summarized the case of using 4bit block protection.

* Not supporting TB on status register.

- BP3(BIT5)
  SPI_NOR_HAS_BP3

- BP3(BIT6)
  SPI_NOR_HAS_BP3 | SPI_NOR_BP3_SR_BIT6

* Supporting TB on status register.

- TB(BIT6), BP3(BIT5)
  SPI_NOR_HAS_TB | SPI_NOR_TB_SR_BIT6 | SPI_NOR_HAS_BP3
  vs
  SPI_NOR_HAS_TB | SPI_NOR_HAS_BP3 | SPI_NOR_SR_TB_BIT6_BP3_BIT5
    
- TB(BIT5), BP3(BIT6)
  SPI_NOR_HAS_TB | SPI_NOR_HAS_BP3 | SPI_NOR_BP3_SR_BIT6
  vs
  SPI_NOR_HAS_TB | SPI_NOR_HAS_BP3 | SPI_NOR_SR_TB_BIT5_BP3_BIT6  
    
SPI_NOR_BP3_SR_BIT6 and SPI_NOR_TB_SR_BIT6 must exist becuase it maybe
used. It would be ok to add some definitions(as you mentioned) to get
same value with SPI_NOR_BP3_SR_BIT6 or SPI_NOR_TB_SR_BIT6 for
readabilty.

But I think there is nothing looks so cool. Any suggestion?

> > Cheers,
> > ta
> > 
> 
> 
Thanks,
Jungseung Lee


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