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

From: Jungseung Lee <js07.lee@samsung.com>
To: Michael Walle <michael@walle.cc>, Tudor.Ambarus@microchip.com
Cc: john.garry@huawei.com, linux-mtd@lists.infradead.org,
	vigneshr@ti.com, js07.lee@gmail.com
Subject: Re: [PATCH v3 2/3] mtd: spi-nor: add 4bit block protection support
Date: Thu, 23 Jan 2020 12:59:54 +0900	[thread overview]
Message-ID: <7004715e639d0d7aadd48ebb82cfaec42e032cec.camel@samsung.com> (raw)
In-Reply-To: <87d1ed5d35cc5cb7addfb60d5dd028c9@walle.cc>

Hi, Michael

2020-01-22 (Wed), 18:14 +0100, Michael Walle:
> Am 2020-01-22 15:31, schrieb Tudor.Ambarus@microchip.com:
> > Hi, Jungseung,
> > 
> > On Wednesday, January 22, 2020 1:42:00 PM EET Jungseung Lee wrote:
> > 
> > cut
> > 
> > > > > > +#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.
> 
> And like the mx25u3232f, but this bit is only OTP programmable! For
> now,
> I'd only add support for reading the TB bit to for this kind of
> flashes,
> to prevent accidentially program this bit. It would be up to the
> board
> manufacturer how to actually set the bit.
> 
> Like having a TB_READ_ONLY flag.
> 
> Its also some kind of asymmetrical because you can only set it one
> way,
> eg. you can OTP the flash to be TB=1. Then you can be sure that the 
> flash
> will always be TB=1. But OTOH you cannot be sure that a TB=0 flash
> will
> always be a TB=0 flash, because you cannot lock that bit.
> 
> Any thoughts?
> 

Actually, I didn't get the chance to take a look at some flash that has
TB bit in configuration register. Currently mainline kernel just care
flashes that has TB bit in SR and SPI_NOR_HAS_TB can be set only such
flashes(as you could see in comment). It seems neccessary to add
another functions and flag for mx25u3232f case.
Is there any flash that has OTP programmable TB bit in SR?

Tudor and me were saying that there is some flash that has not TB bit
in *SR* even if it has BP3 bit in SR. So it seems unnecessary to make
correlated flag like SPI_NOR_SR_TB_BIT6_BP3_BIT5 for convenience.

> -michael
> 
> > > > 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?
> > 
> > Ok, this info should go in the commit message, together with
> > details 
> > about
> > which flashes were tested.
> > 
> > I didn't know that the TB bit can be defined in the Configuration 
> > register.
> > This means that your suggestion with dedicated macros for BP3 and
> > TB is 
> > fine.
> > 
> > I looked a bit over your patches, they are in a pretty good shape.
> > I 
> > saw
> > something that can be improved on patch 2/3, but I didn't manage
> > to 
> > finish the
> > review. Your patches are the first on my TODO list, but now I'm a
> > bit 
> > busy. I
> > hope that I'll come with a complete review by the end of the next
> > week. 
> > I'm
> > going to do tests on few flashes too, to make sure that BP0-2 was
> > not
> > affected.
> > 
> > In the meantime, maybe Michael or John can review/test your
> > patches, 
> > they
> > showed interest in BP0-3 support.
> > 
> > Cheers,
> > ta
> 
> 
Thanks,
Jungseung Lee

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