| Message ID | 1357257748.31023.12.camel@pc786-ubu.GNET.GLOBAL.VPN |
|---|---|
| State | Accepted |
| Commit | 972e1b7b450a93589b2a4c709e68f68da059aa5c |
| Headers | show |
On Fri, 2013-01-04 at 13:02 +1300, Austin Boyle wrote: > This patch adds generic support for flash protection on STmicro chips. > On chips with less than 3 protection bits, the unused bits are don't cares > and so can be written anyway. The lock function will only change the > protection bits if it would not unlock other areas. Similarly, the unlock > function will not lock currently unlocked areas. Tested on the m25p64. > > From: Austin Boyle <Austin.Boyle@aviatnet.com> > Signed-off-by: Austin Boyle <Austin.Boyle@aviatnet.com> Pushed to l2-mtd.git, thanks!
Hi, On 01/04/2013 01:02 AM, Austin Boyle wrote: > This patch adds generic support for flash protection on STmicro chips. > On chips with less than 3 protection bits, the unused bits are don't cares > and so can be written anyway. I have two remarks: 1) I believe this introduces incompatibilities with existing bootloaders which do not support this feature. Namely, u-boot is not able (to the best of my knowledge) to treat these bits properly. So as soon as you write something to your SPI nor flash from within linux, u-boot is not able to erase/rewrite those blocks anymore. Wouldn't it make more sense to selectively enable this feature, only if explicity configured to do so (e.g. through its device tree node)? Like what was used for the Spansion's PPB, see: http://lists.infradead.org/pipermail/linux-mtd/2013-January/045536.html > The lock function will only change the > protection bits if it would not unlock other areas. Similarly, the unlock > function will not lock currently unlocked areas. Tested on the m25p64. > > From: Austin Boyle <Austin.Boyle@aviatnet.com> > Signed-off-by: Austin Boyle <Austin.Boyle@aviatnet.com> > --- > diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c > index 4eeeb2d..069e34f 100644 > --- a/drivers/mtd/devices/m25p80.c > +++ b/drivers/mtd/devices/m25p80.c > @@ -565,6 +565,96 @@ time_out: > return ret; > } > > +static int m25p80_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len) > +{ > + struct m25p *flash = mtd_to_m25p(mtd); > + uint32_t offset = ofs; > + uint8_t status_old, status_new; > + int res = 0; > + > + mutex_lock(&flash->lock); > + /* Wait until finished previous command */ > + if (wait_till_ready(flash)) { > + res = 1; > + goto err; > + } > + > + status_old = read_sr(flash); > + > + if (offset < flash->mtd.size-(flash->mtd.size/2)) > + status_new = status_old | SR_BP2 | SR_BP1 | SR_BP0; > + else if (offset < flash->mtd.size-(flash->mtd.size/4)) > + status_new = (status_old & ~SR_BP0) | SR_BP2 | SR_BP1; > + else if (offset < flash->mtd.size-(flash->mtd.size/8)) > + status_new = (status_old & ~SR_BP1) | SR_BP2 | SR_BP0; > + else if (offset < flash->mtd.size-(flash->mtd.size/16)) > + status_new = (status_old & ~(SR_BP0|SR_BP1)) | SR_BP2; > + else if (offset < flash->mtd.size-(flash->mtd.size/32)) > + status_new = (status_old & ~SR_BP2) | SR_BP1 | SR_BP0; > + else if (offset < flash->mtd.size-(flash->mtd.size/64)) > + status_new = (status_old & ~(SR_BP2|SR_BP0)) | SR_BP1; > + else > + status_new = (status_old & ~(SR_BP2|SR_BP1)) | SR_BP0; 2) While I believe this might work on m25p32, m25p64 and m25p128 (i.e. flashes with 64 blocks or more), it looks incorrect for smaller chips (namely our m25p80, with just 16 blocks). There, the 1/64 logic scales down to 1/16, e.g. - 000 means protect nothing - 001 means protect 1/16th (=1 blocks) [m25p64 => 1/64th] - 010 means protect 1/8th (=2 blocks) [m25p64 => 1/32th] - ... - 100 means protect 1/2nd (=8 blocks) - 101,110, 111 mean protect everything and I assume the same goes for chips with fewer sectors. Any comments? Thanks, Gerlando > + > + /* Only modify protection if it will not unlock other areas */ > + if ((status_new&(SR_BP2|SR_BP1|SR_BP0)) > > + (status_old&(SR_BP2|SR_BP1|SR_BP0))) { > + write_enable(flash); > + if (write_sr(flash, status_new) < 0) { > + res = 1; > + goto err; > + } > + } > + > +err: mutex_unlock(&flash->lock); > + return res; > +} > + > +static int m25p80_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len) > +{ > + struct m25p *flash = mtd_to_m25p(mtd); > + uint32_t offset = ofs; > + uint8_t status_old, status_new; > + int res = 0; > + > + mutex_lock(&flash->lock); > + /* Wait until finished previous command */ > + if (wait_till_ready(flash)) { > + res = 1; > + goto err; > + } > + > + status_old = read_sr(flash); > + > + if (offset+len > flash->mtd.size-(flash->mtd.size/64)) > + status_new = status_old & ~(SR_BP2|SR_BP1|SR_BP0); > + else if (offset+len > flash->mtd.size-(flash->mtd.size/32)) > + status_new = (status_old & ~(SR_BP2|SR_BP1)) | SR_BP0; > + else if (offset+len > flash->mtd.size-(flash->mtd.size/16)) > + status_new = (status_old & ~(SR_BP2|SR_BP0)) | SR_BP1; > + else if (offset+len > flash->mtd.size-(flash->mtd.size/8)) > + status_new = (status_old & ~SR_BP2) | SR_BP1 | SR_BP0; > + else if (offset+len > flash->mtd.size-(flash->mtd.size/4)) > + status_new = (status_old & ~(SR_BP0|SR_BP1)) | SR_BP2; > + else if (offset+len > flash->mtd.size-(flash->mtd.size/2)) > + status_new = (status_old & ~SR_BP1) | SR_BP2 | SR_BP0; > + else > + status_new = (status_old & ~SR_BP0) | SR_BP2 | SR_BP1; > + > + /* Only modify protection if it will not lock other areas */ > + if ((status_new&(SR_BP2|SR_BP1|SR_BP0)) < > + (status_old&(SR_BP2|SR_BP1|SR_BP0))) { > + write_enable(flash); > + if (write_sr(flash, status_new) < 0) { > + res = 1; > + goto err; > + } > + } > + > +err: mutex_unlock(&flash->lock); > + return res; > +} > + > /****************************************************************************/ > > /* > @@ -899,6 +989,12 @@ static int m25p_probe(struct spi_device *spi) > flash->mtd._erase = m25p80_erase; > flash->mtd._read = m25p80_read; > > + /* flash protection support for STmicro chips */ > + if (JEDEC_MFR(info->jedec_id) == CFI_MFR_ST) { > + flash->mtd._lock = m25p80_lock; > + flash->mtd._unlock = m25p80_unlock; > + } > + > /* sst flash chips use AAI word program */ > if (JEDEC_MFR(info->jedec_id) == CFI_MFR_SST) > flash->mtd._write = sst_write; > > > ______________________________________________________ > Linux MTD discussion mailing list > http://lists.infradead.org/mailman/listinfo/linux-mtd/ >
Hi, it's me again. In my opinion (and experience) this introduces a pretty serious bug (not to mention the compatibility issues), yet I haven't heard a single word or found a patch applied about it in three months. Am I the only one having this issue? Or maybe I'm just "seeing things"? Thank you, Gerlando P.S. FWIW, the original author of the patch seem to have disappeared. On 11/20/2013 09:04 PM, Gerlando Falauto wrote: > Hi, > > On 01/04/2013 01:02 AM, Austin Boyle wrote: >> This patch adds generic support for flash protection on STmicro chips. >> On chips with less than 3 protection bits, the unused bits are don't >> cares >> and so can be written anyway. > > I have two remarks: > > 1) I believe this introduces incompatibilities with existing bootloaders > which do not support this feature. > Namely, u-boot is not able (to the best of my knowledge) to treat these > bits properly. So as soon as you write something to your SPI nor flash > from within linux, u-boot is not able to erase/rewrite those blocks > anymore. > > Wouldn't it make more sense to selectively enable this feature, only if > explicity configured to do so (e.g. through its device tree node)? > Like what was used for the Spansion's PPB, see: > > http://lists.infradead.org/pipermail/linux-mtd/2013-January/045536.html > > > The lock function will only change the >> protection bits if it would not unlock other areas. Similarly, the unlock >> function will not lock currently unlocked areas. Tested on the m25p64. > > >> From: Austin Boyle <Austin.Boyle@aviatnet.com> >> Signed-off-by: Austin Boyle <Austin.Boyle@aviatnet.com> >> --- >> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c >> index 4eeeb2d..069e34f 100644 >> --- a/drivers/mtd/devices/m25p80.c >> +++ b/drivers/mtd/devices/m25p80.c >> @@ -565,6 +565,96 @@ time_out: >> return ret; >> } >> >> +static int m25p80_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len) >> +{ >> + struct m25p *flash = mtd_to_m25p(mtd); >> + uint32_t offset = ofs; >> + uint8_t status_old, status_new; >> + int res = 0; >> + >> + mutex_lock(&flash->lock); >> + /* Wait until finished previous command */ >> + if (wait_till_ready(flash)) { >> + res = 1; >> + goto err; >> + } >> + >> + status_old = read_sr(flash); >> + >> + if (offset < flash->mtd.size-(flash->mtd.size/2)) >> + status_new = status_old | SR_BP2 | SR_BP1 | SR_BP0; >> + else if (offset < flash->mtd.size-(flash->mtd.size/4)) >> + status_new = (status_old & ~SR_BP0) | SR_BP2 | SR_BP1; >> + else if (offset < flash->mtd.size-(flash->mtd.size/8)) >> + status_new = (status_old & ~SR_BP1) | SR_BP2 | SR_BP0; >> + else if (offset < flash->mtd.size-(flash->mtd.size/16)) >> + status_new = (status_old & ~(SR_BP0|SR_BP1)) | SR_BP2; >> + else if (offset < flash->mtd.size-(flash->mtd.size/32)) >> + status_new = (status_old & ~SR_BP2) | SR_BP1 | SR_BP0; >> + else if (offset < flash->mtd.size-(flash->mtd.size/64)) >> + status_new = (status_old & ~(SR_BP2|SR_BP0)) | SR_BP1; >> + else >> + status_new = (status_old & ~(SR_BP2|SR_BP1)) | SR_BP0; > > 2) While I believe this might work on m25p32, m25p64 and m25p128 (i.e. > flashes with 64 blocks or more), it looks incorrect for smaller chips > (namely our m25p80, with just 16 blocks). There, the 1/64 logic scales > down to 1/16, e.g. > - 000 means protect nothing > - 001 means protect 1/16th (=1 blocks) [m25p64 => 1/64th] > - 010 means protect 1/8th (=2 blocks) [m25p64 => 1/32th] > - ... > - 100 means protect 1/2nd (=8 blocks) > - 101,110, 111 mean protect everything > > and I assume the same goes for chips with fewer sectors. > > Any comments? > > Thanks, > Gerlando > >> + >> + /* Only modify protection if it will not unlock other areas */ >> + if ((status_new&(SR_BP2|SR_BP1|SR_BP0)) > >> + (status_old&(SR_BP2|SR_BP1|SR_BP0))) { >> + write_enable(flash); >> + if (write_sr(flash, status_new) < 0) { >> + res = 1; >> + goto err; >> + } >> + } >> + >> +err: mutex_unlock(&flash->lock); >> + return res; >> +} >> + >> +static int m25p80_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len) >> +{ >> + struct m25p *flash = mtd_to_m25p(mtd); >> + uint32_t offset = ofs; >> + uint8_t status_old, status_new; >> + int res = 0; >> + >> + mutex_lock(&flash->lock); >> + /* Wait until finished previous command */ >> + if (wait_till_ready(flash)) { >> + res = 1; >> + goto err; >> + } >> + >> + status_old = read_sr(flash); >> + >> + if (offset+len > flash->mtd.size-(flash->mtd.size/64)) >> + status_new = status_old & ~(SR_BP2|SR_BP1|SR_BP0); >> + else if (offset+len > flash->mtd.size-(flash->mtd.size/32)) >> + status_new = (status_old & ~(SR_BP2|SR_BP1)) | SR_BP0; >> + else if (offset+len > flash->mtd.size-(flash->mtd.size/16)) >> + status_new = (status_old & ~(SR_BP2|SR_BP0)) | SR_BP1; >> + else if (offset+len > flash->mtd.size-(flash->mtd.size/8)) >> + status_new = (status_old & ~SR_BP2) | SR_BP1 | SR_BP0; >> + else if (offset+len > flash->mtd.size-(flash->mtd.size/4)) >> + status_new = (status_old & ~(SR_BP0|SR_BP1)) | SR_BP2; >> + else if (offset+len > flash->mtd.size-(flash->mtd.size/2)) >> + status_new = (status_old & ~SR_BP1) | SR_BP2 | SR_BP0; >> + else >> + status_new = (status_old & ~SR_BP0) | SR_BP2 | SR_BP1; >> + >> + /* Only modify protection if it will not lock other areas */ >> + if ((status_new&(SR_BP2|SR_BP1|SR_BP0)) < >> + (status_old&(SR_BP2|SR_BP1|SR_BP0))) { >> + write_enable(flash); >> + if (write_sr(flash, status_new) < 0) { >> + res = 1; >> + goto err; >> + } >> + } >> + >> +err: mutex_unlock(&flash->lock); >> + return res; >> +} >> + >> >> /****************************************************************************/ >> >> >> /* >> @@ -899,6 +989,12 @@ static int m25p_probe(struct spi_device *spi) >> flash->mtd._erase = m25p80_erase; >> flash->mtd._read = m25p80_read; >> >> + /* flash protection support for STmicro chips */ >> + if (JEDEC_MFR(info->jedec_id) == CFI_MFR_ST) { >> + flash->mtd._lock = m25p80_lock; >> + flash->mtd._unlock = m25p80_unlock; >> + } >> + >> /* sst flash chips use AAI word program */ >> if (JEDEC_MFR(info->jedec_id) == CFI_MFR_SST) >> flash->mtd._write = sst_write; >> >> >> ______________________________________________________ >> Linux MTD discussion mailing list >> http://lists.infradead.org/mailman/listinfo/linux-mtd/ >> > >
+ Marek, Angus On Thu, Feb 27, 2014 at 03:34:06PM +0100, Gerlando Falauto wrote: > Hi, > > it's me again. > In my opinion (and experience) this introduces a pretty serious bug > (not to mention the compatibility issues), yet I haven't heard a > single word or found a patch applied about it in three months. > Am I the only one having this issue? Or maybe I'm just "seeing things"? I agree that this doesn't look quite like the best implementation. As you note, this feature is not available on *ALL* ST SPI flash. I think it should require yet another device flag in m25p_ids[]... But I don't see why this is a concern for "certain bootloaders". If your bootloader doesn't support locked blocks, then don't run ioctl(MEMLOCK) on the device. Leaving the following context intact for now, since it's old. But please trim your replies and bottom-post in the future. Thanks! Regards, Brian > Thank you, > Gerlando > > P.S. FWIW, the original author of the patch seem to have disappeared. > > On 11/20/2013 09:04 PM, Gerlando Falauto wrote: > >Hi, > > > >On 01/04/2013 01:02 AM, Austin Boyle wrote: > >>This patch adds generic support for flash protection on STmicro chips. > >>On chips with less than 3 protection bits, the unused bits are don't > >>cares > >>and so can be written anyway. > > > >I have two remarks: > > > >1) I believe this introduces incompatibilities with existing bootloaders > >which do not support this feature. > >Namely, u-boot is not able (to the best of my knowledge) to treat these > >bits properly. So as soon as you write something to your SPI nor flash > >from within linux, u-boot is not able to erase/rewrite those blocks > >anymore. > > > >Wouldn't it make more sense to selectively enable this feature, only if > >explicity configured to do so (e.g. through its device tree node)? > >Like what was used for the Spansion's PPB, see: > > > >http://lists.infradead.org/pipermail/linux-mtd/2013-January/045536.html > > > > > The lock function will only change the > >>protection bits if it would not unlock other areas. Similarly, the unlock > >>function will not lock currently unlocked areas. Tested on the m25p64. > > > > >>From: Austin Boyle <Austin.Boyle@aviatnet.com> > >>Signed-off-by: Austin Boyle <Austin.Boyle@aviatnet.com> > >>--- > >>diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c > >>index 4eeeb2d..069e34f 100644 > >>--- a/drivers/mtd/devices/m25p80.c > >>+++ b/drivers/mtd/devices/m25p80.c > >>@@ -565,6 +565,96 @@ time_out: > >> return ret; > >> } > >> > >>+static int m25p80_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len) > >>+{ > >>+ struct m25p *flash = mtd_to_m25p(mtd); > >>+ uint32_t offset = ofs; > >>+ uint8_t status_old, status_new; > >>+ int res = 0; > >>+ > >>+ mutex_lock(&flash->lock); > >>+ /* Wait until finished previous command */ > >>+ if (wait_till_ready(flash)) { > >>+ res = 1; > >>+ goto err; > >>+ } > >>+ > >>+ status_old = read_sr(flash); > >>+ > >>+ if (offset < flash->mtd.size-(flash->mtd.size/2)) > >>+ status_new = status_old | SR_BP2 | SR_BP1 | SR_BP0; > >>+ else if (offset < flash->mtd.size-(flash->mtd.size/4)) > >>+ status_new = (status_old & ~SR_BP0) | SR_BP2 | SR_BP1; > >>+ else if (offset < flash->mtd.size-(flash->mtd.size/8)) > >>+ status_new = (status_old & ~SR_BP1) | SR_BP2 | SR_BP0; > >>+ else if (offset < flash->mtd.size-(flash->mtd.size/16)) > >>+ status_new = (status_old & ~(SR_BP0|SR_BP1)) | SR_BP2; > >>+ else if (offset < flash->mtd.size-(flash->mtd.size/32)) > >>+ status_new = (status_old & ~SR_BP2) | SR_BP1 | SR_BP0; > >>+ else if (offset < flash->mtd.size-(flash->mtd.size/64)) > >>+ status_new = (status_old & ~(SR_BP2|SR_BP0)) | SR_BP1; > >>+ else > >>+ status_new = (status_old & ~(SR_BP2|SR_BP1)) | SR_BP0; > > > >2) While I believe this might work on m25p32, m25p64 and m25p128 (i.e. > >flashes with 64 blocks or more), it looks incorrect for smaller chips > >(namely our m25p80, with just 16 blocks). There, the 1/64 logic scales > >down to 1/16, e.g. > >- 000 means protect nothing > >- 001 means protect 1/16th (=1 blocks) [m25p64 => 1/64th] > >- 010 means protect 1/8th (=2 blocks) [m25p64 => 1/32th] > >- ... > >- 100 means protect 1/2nd (=8 blocks) > >- 101,110, 111 mean protect everything > > > >and I assume the same goes for chips with fewer sectors. > > > >Any comments? > > > >Thanks, > >Gerlando > > > >>+ > >>+ /* Only modify protection if it will not unlock other areas */ > >>+ if ((status_new&(SR_BP2|SR_BP1|SR_BP0)) > > >>+ (status_old&(SR_BP2|SR_BP1|SR_BP0))) { > >>+ write_enable(flash); > >>+ if (write_sr(flash, status_new) < 0) { > >>+ res = 1; > >>+ goto err; > >>+ } > >>+ } > >>+ > >>+err: mutex_unlock(&flash->lock); > >>+ return res; > >>+} > >>+ > >>+static int m25p80_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len) > >>+{ > >>+ struct m25p *flash = mtd_to_m25p(mtd); > >>+ uint32_t offset = ofs; > >>+ uint8_t status_old, status_new; > >>+ int res = 0; > >>+ > >>+ mutex_lock(&flash->lock); > >>+ /* Wait until finished previous command */ > >>+ if (wait_till_ready(flash)) { > >>+ res = 1; > >>+ goto err; > >>+ } > >>+ > >>+ status_old = read_sr(flash); > >>+ > >>+ if (offset+len > flash->mtd.size-(flash->mtd.size/64)) > >>+ status_new = status_old & ~(SR_BP2|SR_BP1|SR_BP0); > >>+ else if (offset+len > flash->mtd.size-(flash->mtd.size/32)) > >>+ status_new = (status_old & ~(SR_BP2|SR_BP1)) | SR_BP0; > >>+ else if (offset+len > flash->mtd.size-(flash->mtd.size/16)) > >>+ status_new = (status_old & ~(SR_BP2|SR_BP0)) | SR_BP1; > >>+ else if (offset+len > flash->mtd.size-(flash->mtd.size/8)) > >>+ status_new = (status_old & ~SR_BP2) | SR_BP1 | SR_BP0; > >>+ else if (offset+len > flash->mtd.size-(flash->mtd.size/4)) > >>+ status_new = (status_old & ~(SR_BP0|SR_BP1)) | SR_BP2; > >>+ else if (offset+len > flash->mtd.size-(flash->mtd.size/2)) > >>+ status_new = (status_old & ~SR_BP1) | SR_BP2 | SR_BP0; > >>+ else > >>+ status_new = (status_old & ~SR_BP0) | SR_BP2 | SR_BP1; > >>+ > >>+ /* Only modify protection if it will not lock other areas */ > >>+ if ((status_new&(SR_BP2|SR_BP1|SR_BP0)) < > >>+ (status_old&(SR_BP2|SR_BP1|SR_BP0))) { > >>+ write_enable(flash); > >>+ if (write_sr(flash, status_new) < 0) { > >>+ res = 1; > >>+ goto err; > >>+ } > >>+ } > >>+ > >>+err: mutex_unlock(&flash->lock); > >>+ return res; > >>+} > >>+ > >> > >>/****************************************************************************/ > >> > >> > >> /* > >>@@ -899,6 +989,12 @@ static int m25p_probe(struct spi_device *spi) > >> flash->mtd._erase = m25p80_erase; > >> flash->mtd._read = m25p80_read; > >> > >>+ /* flash protection support for STmicro chips */ > >>+ if (JEDEC_MFR(info->jedec_id) == CFI_MFR_ST) { > >>+ flash->mtd._lock = m25p80_lock; > >>+ flash->mtd._unlock = m25p80_unlock; > >>+ } > >>+ > >> /* sst flash chips use AAI word program */ > >> if (JEDEC_MFR(info->jedec_id) == CFI_MFR_SST) > >> flash->mtd._write = sst_write; > >> > >>
Different email for Austin? On Wed, Mar 05, 2014 at 12:10:08AM -0800, Brian Norris wrote: > + Marek, Angus > > On Thu, Feb 27, 2014 at 03:34:06PM +0100, Gerlando Falauto wrote: > > Hi, > > > > it's me again. > > In my opinion (and experience) this introduces a pretty serious bug > > (not to mention the compatibility issues), yet I haven't heard a > > single word or found a patch applied about it in three months. > > Am I the only one having this issue? Or maybe I'm just "seeing things"? > > I agree that this doesn't look quite like the best implementation. As > you note, this feature is not available on *ALL* ST SPI flash. I think > it should require yet another device flag in m25p_ids[]... > > But I don't see why this is a concern for "certain bootloaders". If your > bootloader doesn't support locked blocks, then don't run ioctl(MEMLOCK) > on the device. > > Leaving the following context intact for now, since it's old. But please > trim your replies and bottom-post in the future. Thanks! > > Regards, > Brian > > > Thank you, > > Gerlando > > > > P.S. FWIW, the original author of the patch seem to have disappeared. > > > > On 11/20/2013 09:04 PM, Gerlando Falauto wrote: > > >Hi, > > > > > >On 01/04/2013 01:02 AM, Austin Boyle wrote: > > >>This patch adds generic support for flash protection on STmicro chips. > > >>On chips with less than 3 protection bits, the unused bits are don't > > >>cares > > >>and so can be written anyway. > > > > > >I have two remarks: > > > > > >1) I believe this introduces incompatibilities with existing bootloaders > > >which do not support this feature. > > >Namely, u-boot is not able (to the best of my knowledge) to treat these > > >bits properly. So as soon as you write something to your SPI nor flash > > >from within linux, u-boot is not able to erase/rewrite those blocks > > >anymore. > > > > > >Wouldn't it make more sense to selectively enable this feature, only if > > >explicity configured to do so (e.g. through its device tree node)? > > >Like what was used for the Spansion's PPB, see: > > > > > >http://lists.infradead.org/pipermail/linux-mtd/2013-January/045536.html > > > > > > > The lock function will only change the > > >>protection bits if it would not unlock other areas. Similarly, the unlock > > >>function will not lock currently unlocked areas. Tested on the m25p64. > > > > > > >>From: Austin Boyle <Austin.Boyle@aviatnet.com> > > >>Signed-off-by: Austin Boyle <Austin.Boyle@aviatnet.com> > > >>--- > > >>diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c > > >>index 4eeeb2d..069e34f 100644 > > >>--- a/drivers/mtd/devices/m25p80.c > > >>+++ b/drivers/mtd/devices/m25p80.c > > >>@@ -565,6 +565,96 @@ time_out: > > >> return ret; > > >> } > > >> > > >>+static int m25p80_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len) > > >>+{ > > >>+ struct m25p *flash = mtd_to_m25p(mtd); > > >>+ uint32_t offset = ofs; > > >>+ uint8_t status_old, status_new; > > >>+ int res = 0; > > >>+ > > >>+ mutex_lock(&flash->lock); > > >>+ /* Wait until finished previous command */ > > >>+ if (wait_till_ready(flash)) { > > >>+ res = 1; > > >>+ goto err; > > >>+ } > > >>+ > > >>+ status_old = read_sr(flash); > > >>+ > > >>+ if (offset < flash->mtd.size-(flash->mtd.size/2)) > > >>+ status_new = status_old | SR_BP2 | SR_BP1 | SR_BP0; > > >>+ else if (offset < flash->mtd.size-(flash->mtd.size/4)) > > >>+ status_new = (status_old & ~SR_BP0) | SR_BP2 | SR_BP1; > > >>+ else if (offset < flash->mtd.size-(flash->mtd.size/8)) > > >>+ status_new = (status_old & ~SR_BP1) | SR_BP2 | SR_BP0; > > >>+ else if (offset < flash->mtd.size-(flash->mtd.size/16)) > > >>+ status_new = (status_old & ~(SR_BP0|SR_BP1)) | SR_BP2; > > >>+ else if (offset < flash->mtd.size-(flash->mtd.size/32)) > > >>+ status_new = (status_old & ~SR_BP2) | SR_BP1 | SR_BP0; > > >>+ else if (offset < flash->mtd.size-(flash->mtd.size/64)) > > >>+ status_new = (status_old & ~(SR_BP2|SR_BP0)) | SR_BP1; > > >>+ else > > >>+ status_new = (status_old & ~(SR_BP2|SR_BP1)) | SR_BP0; > > > > > >2) While I believe this might work on m25p32, m25p64 and m25p128 (i.e. > > >flashes with 64 blocks or more), it looks incorrect for smaller chips > > >(namely our m25p80, with just 16 blocks). There, the 1/64 logic scales > > >down to 1/16, e.g. > > >- 000 means protect nothing > > >- 001 means protect 1/16th (=1 blocks) [m25p64 => 1/64th] > > >- 010 means protect 1/8th (=2 blocks) [m25p64 => 1/32th] > > >- ... > > >- 100 means protect 1/2nd (=8 blocks) > > >- 101,110, 111 mean protect everything > > > > > >and I assume the same goes for chips with fewer sectors. > > > > > >Any comments? > > > > > >Thanks, > > >Gerlando > > > > > >>+ > > >>+ /* Only modify protection if it will not unlock other areas */ > > >>+ if ((status_new&(SR_BP2|SR_BP1|SR_BP0)) > > > >>+ (status_old&(SR_BP2|SR_BP1|SR_BP0))) { > > >>+ write_enable(flash); > > >>+ if (write_sr(flash, status_new) < 0) { > > >>+ res = 1; > > >>+ goto err; > > >>+ } > > >>+ } > > >>+ > > >>+err: mutex_unlock(&flash->lock); > > >>+ return res; > > >>+} > > >>+ > > >>+static int m25p80_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len) > > >>+{ > > >>+ struct m25p *flash = mtd_to_m25p(mtd); > > >>+ uint32_t offset = ofs; > > >>+ uint8_t status_old, status_new; > > >>+ int res = 0; > > >>+ > > >>+ mutex_lock(&flash->lock); > > >>+ /* Wait until finished previous command */ > > >>+ if (wait_till_ready(flash)) { > > >>+ res = 1; > > >>+ goto err; > > >>+ } > > >>+ > > >>+ status_old = read_sr(flash); > > >>+ > > >>+ if (offset+len > flash->mtd.size-(flash->mtd.size/64)) > > >>+ status_new = status_old & ~(SR_BP2|SR_BP1|SR_BP0); > > >>+ else if (offset+len > flash->mtd.size-(flash->mtd.size/32)) > > >>+ status_new = (status_old & ~(SR_BP2|SR_BP1)) | SR_BP0; > > >>+ else if (offset+len > flash->mtd.size-(flash->mtd.size/16)) > > >>+ status_new = (status_old & ~(SR_BP2|SR_BP0)) | SR_BP1; > > >>+ else if (offset+len > flash->mtd.size-(flash->mtd.size/8)) > > >>+ status_new = (status_old & ~SR_BP2) | SR_BP1 | SR_BP0; > > >>+ else if (offset+len > flash->mtd.size-(flash->mtd.size/4)) > > >>+ status_new = (status_old & ~(SR_BP0|SR_BP1)) | SR_BP2; > > >>+ else if (offset+len > flash->mtd.size-(flash->mtd.size/2)) > > >>+ status_new = (status_old & ~SR_BP1) | SR_BP2 | SR_BP0; > > >>+ else > > >>+ status_new = (status_old & ~SR_BP0) | SR_BP2 | SR_BP1; > > >>+ > > >>+ /* Only modify protection if it will not lock other areas */ > > >>+ if ((status_new&(SR_BP2|SR_BP1|SR_BP0)) < > > >>+ (status_old&(SR_BP2|SR_BP1|SR_BP0))) { > > >>+ write_enable(flash); > > >>+ if (write_sr(flash, status_new) < 0) { > > >>+ res = 1; > > >>+ goto err; > > >>+ } > > >>+ } > > >>+ > > >>+err: mutex_unlock(&flash->lock); > > >>+ return res; > > >>+} > > >>+ > > >> > > >>/****************************************************************************/ > > >> > > >> > > >> /* > > >>@@ -899,6 +989,12 @@ static int m25p_probe(struct spi_device *spi) > > >> flash->mtd._erase = m25p80_erase; > > >> flash->mtd._read = m25p80_read; > > >> > > >>+ /* flash protection support for STmicro chips */ > > >>+ if (JEDEC_MFR(info->jedec_id) == CFI_MFR_ST) { > > >>+ flash->mtd._lock = m25p80_lock; > > >>+ flash->mtd._unlock = m25p80_unlock; > > >>+ } > > >>+ > > >> /* sst flash chips use AAI word program */ > > >> if (JEDEC_MFR(info->jedec_id) == CFI_MFR_SST) > > >> flash->mtd._write = sst_write; > > >> > > >>
Hi all, I am very sorry I missed this discussion, that was my previous work email address. Thanks for finding me Brian! > On Thu, Feb 27, 2014 at 03:34:06PM +0100, Gerlando Falauto wrote: > > >2) While I believe this might work on m25p32, m25p64 and m25p128 (i.e. > > >flashes with 64 blocks or more), it looks incorrect for smaller chips > > >(namely our m25p80, with just 16 blocks). There, the 1/64 logic scales > > >down to 1/16, e.g. Gerlando, I agree that the logic for calculating the protection bits doesn't work for chips with fewer than 64 sectors. I was only testing the m25p64 at the time. I don't think there is an issue with some bootloaders not supporting this feature, it is already optional. It is a good idea to add another flag for flash protection to be explicitly clear which devices support this. (I previously made the assumption that writing to those status bits when unused was harmless, from the datasheets I found they seem to be don't cares.) The following logic for calculating the block protect bits applies to the majority of the STmicro devices that support protection (m25p10, p20, p40, p80, p16, pe16, p32, p64, p128): SR_BPs | Protected (upper) | Unprotected (lower) ===================================================== 0 | 0/n | n - 0/n 1 | 1/n | n - 1/n 2 | 2/n | n - 2/n 3 | 3/n | n - 4/n 4 | 4/n | n - 8/n 5 | 5/n | n - 16/n 6 | 6/n | n - 32/n 7 | 7/n | n - 64/n Where n is number of sectors if less than 64. Some special cases: - m25p64 has 128 sectors but only supports protection to 64 sector resolution. - m25p05 uses SR=1/2 for protect Block Erase, and SR=3 for protect Block Erase, Page Program, Sector Erase. - m25px32 has an additional bit for locking the lower sections. A patch with this implementation follows. Let me know what you think. I have a spreadsheet summarising the block protect bits for the STmicro devices I can share if it will help. Thanks, Austin.
Hi Austin, Brian, thank you for taking care of this. On 03/08/2014 04:03 PM, Austin Boyle wrote: [...] > > I don't think there is an issue with some bootloaders not supporting > this feature, it is already optional. What do you mean exactly by "it is optional"? I agree with you that an explicit ioctl(MEMLOCK) is order for locking to take place. However, this seems to be the default action for the u-boot environment userspace tools. They will issue a MEMUNLOCK/MEMLOCK pair when trying to write some changes to the environment, without even checking the return value. This would of course fail silently when the feature was not implemented (as it was the case before the original patch was applied) and everything was working as expected. Now linux supports this feature, and u-boot doesn't, so as soon as you write something to the flash from userspace, it will be locked and u-boot won't ever be able to write to it again. In my opinion, we're breaking something here (call it userspace API or otherwise). My suggestion would then be to make it an optional feature to be explicitly enabled on the device tree, like Heicho did for CFI flashes: http://lists.infradead.org/pipermail/linux-mtd/2013-January/045536.html Or I guess another way would be to implement the _is_locked() function, so to have the userspace tools check the locking status before unlocking, and only lock it again if was locked in the first place. It wouldn't fix my issue right away (as the userspace tools don't currenctly perform this check), but at least it would provide some way out here without breaking compatibility with the existing u-boot. > It is a good idea to add another flag for flash protection to be > explicitly clear which devices support this. (I previously made the > assumption that writing to those status bits when unused was harmless, > from the datasheets I found they seem to be don't cares.) Agreed. > The following logic for calculating the block protect bits applies to > the majority of the STmicro devices that support protection (m25p10, > p20, p40, p80, p16, pe16, p32, p64, p128): > > SR_BPs| Protected (upper)| Unprotected (lower) > ===================================================== > 0| 0/n| n - 0/n > 1| 1/n| n - 1/n > 2| 2/n| n - 2/n > 3| 3/n| n - 4/n > 4| 4/n| n - 8/n > 5| 5/n| n - 16/n > 6| 6/n| n - 32/n > 7| 7/n| n - 64/n Uhm, I believe it should read like this (unprotected portion is of course "n - protected portion"): SR BPs | Protected portion --------------------------- 0 | 0/n 1 | 1/n 2 | 2/n 3 | min(4,n)/n 4 | min(8,n)/n 5 | min(16,n)/n 6 | min(32,n)/n 7 | min(64,n)/n Or at least that was my understanding. > Where n is number of sectors if less than 64. > > Some special cases: > - m25p64 has 128 sectors but only supports protection to 64 sector > resolution. > - m25p05 uses SR=1/2 for protect Block Erase, and SR=3 for protect Block > Erase, Page Program, Sector Erase. > - m25px32 has an additional bit for locking the lower sections. > > A patch with this implementation follows. Let me know what you think. I > have a spreadsheet summarising the block protect bits for the STmicro > devices I can share if it will help. Could you please share this? Thank you, Gerlando > > Thanks, > Austin.
Hi Gerlando, On Mon, 10 Mar 2014, Gerlando Falauto wrote: > What do you mean exactly by "it is optional"? > I agree with you that an explicit ioctl(MEMLOCK) is order for locking to take > place. However, this seems to be the default action for the u-boot > environment userspace tools. They will issue a MEMUNLOCK/MEMLOCK pair when > trying to write some changes to the environment, without even checking the > return value. This would of course fail silently when the feature was not > implemented (as it was the case before the original patch was applied) and > everything was working as expected. > Now linux supports this feature, and u-boot doesn't, so as soon as you write > something to the flash from userspace, it will be locked and u-boot won't > ever be able to write to it again. > OK I understand your point after checking the code for the u-boot tool. > In my opinion, we're breaking something here (call it userspace API or > otherwise). My suggestion would then be to make it an optional feature to be > explicitly enabled on the device tree, like Heicho did for CFI flashes: > > http://lists.infradead.org/pipermail/linux-mtd/2013-January/045536.html > > Or I guess another way would be to implement the _is_locked() function, so to > have the userspace tools check the locking status before unlocking, and only > lock it again if was locked in the first place. > It wouldn't fix my issue right away (as the userspace tools don't currenctly > perform this check), but at least it would provide some way out here without > breaking compatibility with the existing u-boot. > I prefer this suggestion of implementing _is_locked(). It is a simple change so may as well be done while fixing the lock/unlock code anyway. One could say it is counterintuitive for ioctl(MEMLOCK) not to work without previous configuration, as in the first suggestion. If people are passionate about making the feature configurable it could be done separately once we've fixed the logic with _lock/_unlock and adding _is_locked. I will submit a change to the u-boot env tool to check the locking state if supported and restore the same state after - obviously more correct, regardless of the decision made here. Not ideal but in the meantime if someone needs to ensure the device is unlocked they can run the flash_unlock from mtd-utils after writing u-boot environment variables from the userspace tool. (Call it a user space work around for a user space bug?) > Uhm, I believe it should read like this (unprotected portion is of course "n > - protected portion"): > > SR BPs | Protected portion > --------------------------- > 0 | 0/n > 1 | 1/n > 2 | 2/n > 3 | min(4,n)/n > 4 | min(8,n)/n > 5 | min(16,n)/n > 6 | min(32,n)/n > 7 | min(64,n)/n > Yes sorry that was a typo in the previous email, should have been powers of 2 as above and the patch. Also technically correct about the min macro, as you saw I was just terminating the loop before that condition was met. >> A patch with this implementation follows. Let me know what you think. I >> have a spreadsheet summarising the block protect bits for the STmicro >> devices I can share if it will help. > > Could you please share this? It sounds like you have already done your own research but here is what I have: https://docs.google.com/spreadsheet/ccc?key=0AhKBO-EQCLLkdGR1Q05qLUs2RURsMFA4V2s2X1llY3c&usp=sharing Thanks for your comments on this issue and patch. I will resubmit another version addressing your points soon. Cheers, Austin.
Hi, On 03/13/2014 01:57 PM, Austin Boyle wrote: > > Hi Gerlando, [...] > >> In my opinion, we're breaking something here (call it userspace API or >> otherwise). My suggestion would then be to make it an optional feature to be >> explicitly enabled on the device tree, like Heicho did for CFI flashes: >> >> http://lists.infradead.org/pipermail/linux-mtd/2013-January/045536.html >> >> Or I guess another way would be to implement the _is_locked() function, so to >> have the userspace tools check the locking status before unlocking, and only >> lock it again if was locked in the first place. >> It wouldn't fix my issue right away (as the userspace tools don't currenctly >> perform this check), but at least it would provide some way out here without >> breaking compatibility with the existing u-boot. >> > I prefer this suggestion of implementing _is_locked(). It is a simple > change so may as well be done while fixing the lock/unlock code anyway. I agree, plus it would be useful anyway (btw, shouldn't that be a requirement anyway?). There is one thing I don't understand though. The _lock()/_unlock()/_is_locked() functions will take arbitrary ranges of flash, whereas the BP only allows you to lock predefined portions of it (or, let's say, from a discrete number of predefined starting points up to the end). What should be the expected behavior resp. return value when the specified portion crosses one of the starting points (resp. the current starting point)? > One could say it is counterintuitive for ioctl(MEMLOCK) not to work > without previous configuration, as in the first suggestion. If people are > passionate about making the feature configurable it could be done > separately once we've fixed the logic with _lock/_unlock and > adding _is_locked. Don't know how this falls into the whole device tree logic, but we could also have it default to "enabled", and provide a specific way to explicitly disable the feature. > I will submit a change to the u-boot env tool to check the locking state > if supported and restore the same state after - obviously more > correct, regardless of the decision made here. That would be great, thank you! Back to my point above, I believe it should be put in a way so that no matter what the initial protection status, the whole sequence (check/unlock/lock) should put you back in the same status as before. Not sure if/how this would be at all feasible though, especially if your u-boot environment(s) is(are) stored at very weird locations. > Not ideal but in the meantime if someone needs to ensure the device is > unlocked they can run the flash_unlock from mtd-utils after writing u-boot > environment variables from the userspace tool. (Call it a user space > work around for a user space bug?) Definitely better than having to throw the whole board away ;-) > >> Uhm, I believe it should read like this (unprotected portion is of course "n >> - protected portion"): >> >> SR BPs | Protected portion >> --------------------------- >> 0 | 0/n >> 1 | 1/n >> 2 | 2/n >> 3 | min(4,n)/n >> 4 | min(8,n)/n >> 5 | min(16,n)/n >> 6 | min(32,n)/n >> 7 | min(64,n)/n >> > Yes sorry that was a typo in the previous email, should have been powers > of 2 as above and the patch. Also technically correct about the min macro, > as you saw I was just terminating the loop before that condition was met. > >>> A patch with this implementation follows. Let me know what you think. I >>> have a spreadsheet summarising the block protect bits for the STmicro >>> devices I can share if it will help. >> >> Could you please share this? > It sounds like you have already done your own research but here is what I > have: > https://docs.google.com/spreadsheet/ccc?key=0AhKBO-EQCLLkdGR1Q05qLUs2RURsMFA4V2s2X1llY3c&usp=sharing Thank you... BTW, I had only opened 2-3 datasheets, did not go through the whole family as you did. ;-) > Thanks for your comments on this issue and patch. I will resubmit > another version addressing your points soon. Thank you for taking my complaints upon yourself ;-) Best, Gerlando
diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c index 4eeeb2d..069e34f 100644 --- a/drivers/mtd/devices/m25p80.c +++ b/drivers/mtd/devices/m25p80.c @@ -565,6 +565,96 @@ time_out: return ret; } +static int m25p80_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len) +{ + struct m25p *flash = mtd_to_m25p(mtd); + uint32_t offset = ofs; + uint8_t status_old, status_new; + int res = 0; + + mutex_lock(&flash->lock); + /* Wait until finished previous command */ + if (wait_till_ready(flash)) { + res = 1; + goto err; + } + + status_old = read_sr(flash); + + if (offset < flash->mtd.size-(flash->mtd.size/2)) + status_new = status_old | SR_BP2 | SR_BP1 | SR_BP0; + else if (offset < flash->mtd.size-(flash->mtd.size/4)) + status_new = (status_old & ~SR_BP0) | SR_BP2 | SR_BP1; + else if (offset < flash->mtd.size-(flash->mtd.size/8)) + status_new = (status_old & ~SR_BP1) | SR_BP2 | SR_BP0; + else if (offset < flash->mtd.size-(flash->mtd.size/16)) + status_new = (status_old & ~(SR_BP0|SR_BP1)) | SR_BP2; + else if (offset < flash->mtd.size-(flash->mtd.size/32)) + status_new = (status_old & ~SR_BP2) | SR_BP1 | SR_BP0; + else if (offset < flash->mtd.size-(flash->mtd.size/64)) + status_new = (status_old & ~(SR_BP2|SR_BP0)) | SR_BP1; + else + status_new = (status_old & ~(SR_BP2|SR_BP1)) | SR_BP0; + + /* Only modify protection if it will not unlock other areas */ + if ((status_new&(SR_BP2|SR_BP1|SR_BP0)) > + (status_old&(SR_BP2|SR_BP1|SR_BP0))) { + write_enable(flash); + if (write_sr(flash, status_new) < 0) { + res = 1; + goto err; + } + } + +err: mutex_unlock(&flash->lock); + return res; +} + +static int m25p80_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len) +{ + struct m25p *flash = mtd_to_m25p(mtd); + uint32_t offset = ofs; + uint8_t status_old, status_new; + int res = 0; + + mutex_lock(&flash->lock); + /* Wait until finished previous command */ + if (wait_till_ready(flash)) { + res = 1; + goto err; + } + + status_old = read_sr(flash); + + if (offset+len > flash->mtd.size-(flash->mtd.size/64)) + status_new = status_old & ~(SR_BP2|SR_BP1|SR_BP0); + else if (offset+len > flash->mtd.size-(flash->mtd.size/32)) + status_new = (status_old & ~(SR_BP2|SR_BP1)) | SR_BP0; + else if (offset+len > flash->mtd.size-(flash->mtd.size/16)) + status_new = (status_old & ~(SR_BP2|SR_BP0)) | SR_BP1; + else if (offset+len > flash->mtd.size-(flash->mtd.size/8)) + status_new = (status_old & ~SR_BP2) | SR_BP1 | SR_BP0; + else if (offset+len > flash->mtd.size-(flash->mtd.size/4)) + status_new = (status_old & ~(SR_BP0|SR_BP1)) | SR_BP2; + else if (offset+len > flash->mtd.size-(flash->mtd.size/2)) + status_new = (status_old & ~SR_BP1) | SR_BP2 | SR_BP0; + else + status_new = (status_old & ~SR_BP0) | SR_BP2 | SR_BP1; + + /* Only modify protection if it will not lock other areas */ + if ((status_new&(SR_BP2|SR_BP1|SR_BP0)) < + (status_old&(SR_BP2|SR_BP1|SR_BP0))) { + write_enable(flash); + if (write_sr(flash, status_new) < 0) { + res = 1; + goto err; + } + } + +err: mutex_unlock(&flash->lock); + return res; +} + /****************************************************************************/ /* @@ -899,6 +989,12 @@ static int m25p_probe(struct spi_device *spi) flash->mtd._erase = m25p80_erase; flash->mtd._read = m25p80_read; + /* flash protection support for STmicro chips */ + if (JEDEC_MFR(info->jedec_id) == CFI_MFR_ST) { + flash->mtd._lock = m25p80_lock; + flash->mtd._unlock = m25p80_unlock; + } + /* sst flash chips use AAI word program */ if (JEDEC_MFR(info->jedec_id) == CFI_MFR_SST) flash->mtd._write = sst_write;