diff mbox series

[v3,1/1] spi-nor: always respect write-protect input

Message ID 20190427062313.24258-1-jonas@norrbonn.se
State New, archived
Delegated to: Ambarus Tudor
Headers show
Series [v3,1/1] spi-nor: always respect write-protect input | expand

Commit Message

Jonas Bonn April 27, 2019, 6:23 a.m. UTC 
The status register bit SRWD (status register write disable) is
described in many words in the datasheets but effectively boils down to:

i) if set, respect WP# when trying to change protection bits;
ii) if unset, ignore WP# when trying to change protection bits

In short, the bit determines whether the WP# signal is honored or not.

This protection bit is used in a couple of different ways:

i)  Some designs attach WP# directly to ground.  At first boot, they
write to the NOR and call flash_lock; this results in the BP0-2 block
protection bits and SRWD being set.  As WP# is permanently grounded,
this block protection cannot be undone and the NOR is effectively
read-only.

ii)  Some designs can control WP#, thus allowing the protection of the
SRWD bit itself to be managed.  When the hardware pulls WP# low, SRWD
and the BP[0-2] block protection bits cannot be changed; when hardware
sets WP# high, the block protection bits can be freely changed,
_irregardless_ of the state of SRWD.

iii)  In the third configuration WP# is pulled high internally, thereby
rendering the SRWD bit ineffective.  In this configuration, the BP[0-2]
block protection bits can always be freely modified; this puts the
writability of the NOR flash exclusively under software control.

Linux currently sets SRWD when flash_lock is invoked.  This prevents
further modification of the BP[0-2] bits and the SRWD bit itself, _if_
WP# is in play; if WP# is not in play, this setting has no effect.  This
behaviour is rational.

Linux, however, clears SRWD when the flash_unlock is invoked to clear
all BP[0-2] bits.  If WP# is low, this will fail and SRWD and the block
protection bits will remain unchanged.  If WP# is high, changing the
BP[0-2] bits will succeed irregardless of the state of SRWD, so clearing
SRWD has no meaningful effect here.

There is, however, another scenario where clearing SRWD when calling
flash_unlock causes unwanted behaviour.  If the BPNV bit is set, the
BP[0-2] bits revert to "fully protected" at reset.  In this
configuration, only someone who can control WP# is able to call
flash_unlock and clear the block protection bits.  In this
configuration, it is desirable that SRWD is not ever cleared so that the
WP# always has full control over the writability of the BP[0-2] bits.
This allows the NOR flash to be made writable _only_ by someone who has
control over the WP# signal; for others, the NOR is read-only.

Given the above, this patch removes the clearing of the SRWD bit from
the flash_unlock path.  With this, Linux only ever sets the SRWD; it
will never clear it.  This should be compatible with all three
configuration outlined above:

i)  If WP# is permanently grounded, SRWD is not clearable anyway.
ii)  If WP# is controllable, SRWD is moot because WP# takes over its
role
iii)  If WP# is floating and thereby pulled permanently high, SRWD has
no effect on the writability of the block protection bits.

Tested on a Cypress s25fl512s.

Signed-off-by: Jonas Bonn <jonas@norrbonn.se>
---

Hi Tudor,

I think this problem is simpler than we think.  Just removing the clear
of SRWD in the flash_unlock path is sufficient to cover all the various
cases, as far as I can tell.

Best regards,
Jonas


 drivers/mtd/spi-nor/spi-nor.c | 4 ----
 1 file changed, 4 deletions(-)
diff mbox series

Patch

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index fae147452aff..bc3317f2bc7c 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -1313,10 +1313,6 @@  static int stm_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
 
 	status_new = (status_old & ~mask & ~SR_TB) | val;
 
-	/* Don't protect status register if we're fully unlocked */
-	if (lock_len == 0)
-		status_new &= ~SR_SRWD;
-
 	if (!use_top)
 		status_new |= SR_TB;