linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2] dt-bindings: mtd: spi-nor: document new flag
@ 2019-12-14 19:19 Michael Walle
  2019-12-14 19:19 ` [PATCH 2/2] mtd: spi-nor: add option to keep lock bits Michael Walle
  2019-12-16  8:54 ` [PATCH 1/2] dt-bindings: mtd: spi-nor: document new flag Vignesh Raghavendra
  0 siblings, 2 replies; 6+ messages in thread
From: Michael Walle @ 2019-12-14 19:19 UTC (permalink / raw)
  To: linux-mtd, devicetree, linux-kernel
  Cc: Miquel Raynal, Richard Weinberger, Vignesh Raghavendra,
	Rob Herring, Mark Rutland, Tudor Ambarus, Michael Walle

Document the new flag "no-unlock".

Signed-off-by: Michael Walle <michael@walle.cc>
---
Does the property need a prefix? I couldn't find any hint. If so, what
should it be? "m25p," or "spi-nor," ?

 Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt
index f03be904d3c2..2d305c893ed7 100644
--- a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt
+++ b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt
@@ -78,6 +78,12 @@ Optional properties:
 		   cannot reboot properly if the flash is left in the "wrong"
 		   state. This boolean flag can be used on such systems, to
 		   denote the absence of a reliable reset mechanism.
+- no-unlock : By default, linux unlocks the whole flash because there
+		   are legacy flash devices which are locked by default
+		   after reset. Set this flag if you don't want linux to
+		   unlock the whole flash automatically. In this case you
+		   can control the non-volatile bits by the
+		   flash_lock/flash_unlock tools.
 
 Example:
 
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH 2/2] mtd: spi-nor: add option to keep lock bits
  2019-12-14 19:19 [PATCH 1/2] dt-bindings: mtd: spi-nor: document new flag Michael Walle
@ 2019-12-14 19:19 ` Michael Walle
  2019-12-16  8:54 ` [PATCH 1/2] dt-bindings: mtd: spi-nor: document new flag Vignesh Raghavendra
  1 sibling, 0 replies; 6+ messages in thread
From: Michael Walle @ 2019-12-14 19:19 UTC (permalink / raw)
  To: linux-mtd, devicetree, linux-kernel
  Cc: Miquel Raynal, Richard Weinberger, Vignesh Raghavendra,
	Rob Herring, Mark Rutland, Tudor Ambarus, Michael Walle

Traditionally, linux unlocks the whole flash because there are legacy
devices which has the write protections bits set by default at startup.
If you actually want to use the flash protection bits, eg. because there
is a read-only part for a bootloader, this automatic unlocking is
harmful. If there is no hardware write protection in place (usually
called WP#), a startup of the kernel just discards this protection.

Introduce a new device tree flag to indicate that the flash should not
be automatically unlocked.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/mtd/spi-nor/spi-nor.c | 5 ++++-
 include/linux/mtd/spi-nor.h   | 1 +
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index f4afe123e9dc..d0bec0adf2f8 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -4910,7 +4910,7 @@ static int spi_nor_quad_enable(struct spi_nor *nor)
  */
 static int spi_nor_unlock_all(struct spi_nor *nor)
 {
-	if (nor->flags & SNOR_F_HAS_LOCK)
+	if (nor->flags & SNOR_F_HAS_LOCK && !(nor->flags & SNOR_F_NO_UNLOCK))
 		return spi_nor_unlock(&nor->mtd, 0, nor->params.size);
 
 	return 0;
@@ -5159,6 +5159,9 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
 	if (of_property_read_bool(np, "broken-flash-reset"))
 		nor->flags |= SNOR_F_BROKEN_RESET;
 
+	if (of_property_read_bool(np, "no-unlock"))
+		nor->flags |= SNOR_F_NO_UNLOCK;
+
 	/*
 	 * Configure the SPI memory:
 	 * - select op codes for (Fast) Read, Page Program and Sector Erase.
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index 5a4623fc586b..4cba5dda2d38 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -244,6 +244,7 @@ enum spi_nor_option_flags {
 	SNOR_F_HAS_LOCK		= BIT(8),
 	SNOR_F_HAS_16BIT_SR	= BIT(9),
 	SNOR_F_NO_READ_CR	= BIT(10),
+	SNOR_F_NO_UNLOCK	= BIT(11),
 
 };
 
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] dt-bindings: mtd: spi-nor: document new flag
  2019-12-14 19:19 [PATCH 1/2] dt-bindings: mtd: spi-nor: document new flag Michael Walle
  2019-12-14 19:19 ` [PATCH 2/2] mtd: spi-nor: add option to keep lock bits Michael Walle
@ 2019-12-16  8:54 ` Vignesh Raghavendra
  2019-12-16 10:29   ` Michael Walle
  1 sibling, 1 reply; 6+ messages in thread
From: Vignesh Raghavendra @ 2019-12-16  8:54 UTC (permalink / raw)
  To: Michael Walle, linux-mtd, devicetree, linux-kernel
  Cc: Miquel Raynal, Richard Weinberger, Rob Herring, Mark Rutland,
	Tudor Ambarus

Hi,

On 15/12/19 12:49 am, Michael Walle wrote:
> Document the new flag "no-unlock".
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
> Does the property need a prefix? I couldn't find any hint. If so, what
> should it be? "m25p," or "spi-nor," ?
> 
>  Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt
> index f03be904d3c2..2d305c893ed7 100644
> --- a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt
> +++ b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt
> @@ -78,6 +78,12 @@ Optional properties:
>  		   cannot reboot properly if the flash is left in the "wrong"
>  		   state. This boolean flag can be used on such systems, to
>  		   denote the absence of a reliable reset mechanism.
> +- no-unlock : By default, linux unlocks the whole flash because there
> +		   are legacy flash devices which are locked by default
> +		   after reset. Set this flag if you don't want linux to
> +		   unlock the whole flash automatically. In this case you
> +		   can control the non-volatile bits by the
> +		   flash_lock/flash_unlock tools.
>  

Current SPI NOR framework unconditionally unlocks entire flash which
I agree is not the best thing to do, but I don't think we need
new DT property here. MTD cmdline partitions and DT partitions already 
provide a way to specify that a partition should remain locked[1][2]

SPI NOR framework should instead set MTD_POWERUP_LOCK flags in mtd->flags
for flash devices that power up with lock bits set. And MTD core will 
take care of unlocking flash regions while taking into account partition
flags defined by user as part of MTD partitions defined in DT or
via cmdline args.

So that change should to be set MTD_POWERUP_LOCK for
in SPI NOR core. Can you check below[3] (untested) diff helps?
This should prevent unlocking partitions that are to remain locked 
as specified in DT/cmdline 

[1] Documentation/devicetree/bindings/mtd/partition.txt
[2] drivers/mtd/parsers/cmdlinepart.c (see "lk" parameter)

[3]

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 1082b6bb1393..6adb950849f6 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -4914,23 +4914,6 @@ static int spi_nor_quad_enable(struct spi_nor *nor)
 	return nor->params.quad_enable(nor);
 }
 
-/**
- * spi_nor_unlock_all() - Unlocks the entire flash memory array.
- * @nor:	pointer to a 'struct spi_nor'.
- *
- * Some SPI NOR flashes are write protected by default after a power-on reset
- * cycle, in order to avoid inadvertent writes during power-up. Backward
- * compatibility imposes to unlock the entire flash memory array at power-up
- * by default.
- */
-static int spi_nor_unlock_all(struct spi_nor *nor)
-{
-	if (nor->flags & SNOR_F_HAS_LOCK)
-		return spi_nor_unlock(&nor->mtd, 0, nor->params.size);
-
-	return 0;
-}
-
 static int spi_nor_init(struct spi_nor *nor)
 {
 	int err;
@@ -4941,11 +4924,11 @@ static int spi_nor_init(struct spi_nor *nor)
 		return err;
 	}
 
-	err = spi_nor_unlock_all(nor);
-	if (err) {
-		dev_dbg(nor->dev, "Failed to unlock the entire flash memory array\n");
-		return err;
-	}
+	/*
+	 * Flashes may power up locked. Set this flag so that MTD core
+	 * takes care of unlocking partitions as required.
+	 */
+	nor->mtd.flags |= MTD_POWERUP_LOCK;
 
 	if (nor->addr_width == 4 && !(nor->flags & SNOR_F_4B_OPCODES)) {
 		/*




-- 
Regards
Vignesh

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] dt-bindings: mtd: spi-nor: document new flag
  2019-12-16  8:54 ` [PATCH 1/2] dt-bindings: mtd: spi-nor: document new flag Vignesh Raghavendra
@ 2019-12-16 10:29   ` Michael Walle
  2019-12-19  5:33     ` Vignesh Raghavendra
  0 siblings, 1 reply; 6+ messages in thread
From: Michael Walle @ 2019-12-16 10:29 UTC (permalink / raw)
  To: Vignesh Raghavendra
  Cc: linux-mtd, devicetree, linux-kernel, Miquel Raynal,
	Richard Weinberger, Rob Herring, Mark Rutland, Tudor Ambarus

Hi,

Am 2019-12-16 09:54, schrieb Vignesh Raghavendra:
> Hi,
> 
> On 15/12/19 12:49 am, Michael Walle wrote:
>> Document the new flag "no-unlock".
>> 
>> Signed-off-by: Michael Walle <michael@walle.cc>
>> ---
>> Does the property need a prefix? I couldn't find any hint. If so, what
>> should it be? "m25p," or "spi-nor," ?
>> 
>>  Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt | 6 ++++++
>>  1 file changed, 6 insertions(+)
>> 
>> diff --git a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt 
>> b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt
>> index f03be904d3c2..2d305c893ed7 100644
>> --- a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt
>> +++ b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt
>> @@ -78,6 +78,12 @@ Optional properties:
>>  		   cannot reboot properly if the flash is left in the "wrong"
>>  		   state. This boolean flag can be used on such systems, to
>>  		   denote the absence of a reliable reset mechanism.
>> +- no-unlock : By default, linux unlocks the whole flash because there
>> +		   are legacy flash devices which are locked by default
>> +		   after reset. Set this flag if you don't want linux to
>> +		   unlock the whole flash automatically. In this case you
>> +		   can control the non-volatile bits by the
>> +		   flash_lock/flash_unlock tools.
>> 
> 
> Current SPI NOR framework unconditionally unlocks entire flash which
> I agree is not the best thing to do, but I don't think we need
> new DT property here. MTD cmdline partitions and DT partitions already
> provide a way to specify that a partition should remain locked[1][2]

I know that the MTD layer has the same kind of unlocking. But that
unlocking is done on a per mtd partition basis. Eg. consider something
like the following

  mtd1 bootloader  (locked)
  mtd2 firmware    (locked)
  mtd3 kernel
  mtd4 environment

Further assume, that the end of mtd2 aligns with one of the possible
locking areas which are supported by the flash chip. Eg. the first 
quarter.

The mtd layer would do two (or four, if "lock" property is set) unlock()
calls, one for mtd1 and one for mtd2.

My point here is, that the mtd partitions doesn't always map to the
locking regions of the SPI flash (at least if the are not merged 
together).

> SPI NOR framework should instead set MTD_POWERUP_LOCK flags in 
> mtd->flags
> for flash devices that power up with lock bits set. And MTD core will
> take care of unlocking flash regions while taking into account 
> partition
> flags defined by user as part of MTD partitions defined in DT or
> via cmdline args.
> 
> So that change should to be set MTD_POWERUP_LOCK for
> in SPI NOR core. Can you check below[3] (untested) diff helps?
> This should prevent unlocking partitions that are to remain locked
> as specified in DT/cmdline

As this change may help my use-case, unlocking is skipped because the
partitions are marked as read only; I fear that the old behaviour will
change. See above.

Mhh. thinking more about it, doesn't the calls also wear out the
non-volatile bits in the NOR flash?

In any case, I'll try your suggestion.

-michael

> 
> [1] Documentation/devicetree/bindings/mtd/partition.txt
> [2] drivers/mtd/parsers/cmdlinepart.c (see "lk" parameter)
> 
> [3]
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c 
> b/drivers/mtd/spi-nor/spi-nor.c
> index 1082b6bb1393..6adb950849f6 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -4914,23 +4914,6 @@ static int spi_nor_quad_enable(struct spi_nor 
> *nor)
>  	return nor->params.quad_enable(nor);
>  }
> 
> -/**
> - * spi_nor_unlock_all() - Unlocks the entire flash memory array.
> - * @nor:	pointer to a 'struct spi_nor'.
> - *
> - * Some SPI NOR flashes are write protected by default after a 
> power-on reset
> - * cycle, in order to avoid inadvertent writes during power-up. 
> Backward
> - * compatibility imposes to unlock the entire flash memory array at 
> power-up
> - * by default.
> - */
> -static int spi_nor_unlock_all(struct spi_nor *nor)
> -{
> -	if (nor->flags & SNOR_F_HAS_LOCK)
> -		return spi_nor_unlock(&nor->mtd, 0, nor->params.size);
> -
> -	return 0;
> -}
> -
>  static int spi_nor_init(struct spi_nor *nor)
>  {
>  	int err;
> @@ -4941,11 +4924,11 @@ static int spi_nor_init(struct spi_nor *nor)
>  		return err;
>  	}
> 
> -	err = spi_nor_unlock_all(nor);
> -	if (err) {
> -		dev_dbg(nor->dev, "Failed to unlock the entire flash memory 
> array\n");
> -		return err;
> -	}
> +	/*
> +	 * Flashes may power up locked. Set this flag so that MTD core
> +	 * takes care of unlocking partitions as required.
> +	 */
> +	nor->mtd.flags |= MTD_POWERUP_LOCK;
> 
>  	if (nor->addr_width == 4 && !(nor->flags & SNOR_F_4B_OPCODES)) {
>  		/*

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] dt-bindings: mtd: spi-nor: document new flag
  2019-12-16 10:29   ` Michael Walle
@ 2019-12-19  5:33     ` Vignesh Raghavendra
  2019-12-20 12:46       ` Michael Walle
  0 siblings, 1 reply; 6+ messages in thread
From: Vignesh Raghavendra @ 2019-12-19  5:33 UTC (permalink / raw)
  To: Michael Walle, Tudor Ambarus
  Cc: linux-mtd, devicetree, linux-kernel, Miquel Raynal,
	Richard Weinberger, Rob Herring, Mark Rutland

Hi Michael,

[...]
>>> +- no-unlock : By default, linux unlocks the whole flash because there
>>> +           are legacy flash devices which are locked by default
>>> +           after reset. Set this flag if you don't want linux to
>>> +           unlock the whole flash automatically. In this case you
>>> +           can control the non-volatile bits by the
>>> +           flash_lock/flash_unlock tools.
>>>
>>
>> Current SPI NOR framework unconditionally unlocks entire flash which
>> I agree is not the best thing to do, but I don't think we need
>> new DT property here. MTD cmdline partitions and DT partitions already
>> provide a way to specify that a partition should remain locked[1][2]
> 
> I know that the MTD layer has the same kind of unlocking. But that
> unlocking is done on a per mtd partition basis. Eg. consider something
> like the following
> 
>  mtd1 bootloader  (locked)
>  mtd2 firmware    (locked)
>  mtd3 kernel
>  mtd4 environment
> 
> Further assume, that the end of mtd2 aligns with one of the possible
> locking areas which are supported by the flash chip. Eg. the first quarter.
> 
> The mtd layer would do two (or four, if "lock" property is set) unlock()
> calls, one for mtd1 and one for mtd2.
> 


> My point here is, that the mtd partitions doesn't always map to the
> locking regions of the SPI flash (at least if the are not merged together).
> 

You are right! This will be an issue if existing partitions are not
aligned to locking regions.

I take my comments back... But I am not sure if a new DT property is the
needed. This does not describe HW and is specific to Linux SPI NOR
stack. How about a module parameter instead?
Module parameter won't provide per flash granularity in controlling
unlocking behavior. But I don't think that matters.

Tudor,

You had a patch doing something similar. Does module param sound good to
you?


-- 
Regards
Vignesh

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] dt-bindings: mtd: spi-nor: document new flag
  2019-12-19  5:33     ` Vignesh Raghavendra
@ 2019-12-20 12:46       ` Michael Walle
  0 siblings, 0 replies; 6+ messages in thread
From: Michael Walle @ 2019-12-20 12:46 UTC (permalink / raw)
  To: Vignesh Raghavendra
  Cc: Tudor Ambarus, linux-mtd, devicetree, linux-kernel,
	Miquel Raynal, Richard Weinberger, Rob Herring, Mark Rutland

Hi Vignesh,

Am 2019-12-19 06:33, schrieb Vignesh Raghavendra:
> Hi Michael,
> 
> [...]
>>>> +- no-unlock : By default, linux unlocks the whole flash because 
>>>> there
>>>> +           are legacy flash devices which are locked by default
>>>> +           after reset. Set this flag if you don't want linux to
>>>> +           unlock the whole flash automatically. In this case you
>>>> +           can control the non-volatile bits by the
>>>> +           flash_lock/flash_unlock tools.
>>>> 
>>> 
>>> Current SPI NOR framework unconditionally unlocks entire flash which
>>> I agree is not the best thing to do, but I don't think we need
>>> new DT property here. MTD cmdline partitions and DT partitions 
>>> already
>>> provide a way to specify that a partition should remain locked[1][2]
>> 
>> I know that the MTD layer has the same kind of unlocking. But that
>> unlocking is done on a per mtd partition basis. Eg. consider something
>> like the following
>> 
>>  mtd1 bootloader  (locked)
>>  mtd2 firmware    (locked)
>>  mtd3 kernel
>>  mtd4 environment
>> 
>> Further assume, that the end of mtd2 aligns with one of the possible
>> locking areas which are supported by the flash chip. Eg. the first 
>> quarter.
>> 
>> The mtd layer would do two (or four, if "lock" property is set) 
>> unlock()
>> calls, one for mtd1 and one for mtd2.
>> 
> 
> 
>> My point here is, that the mtd partitions doesn't always map to the
>> locking regions of the SPI flash (at least if the are not merged 
>> together).
>> 
> 
> You are right! This will be an issue if existing partitions are not
> aligned to locking regions.
> 
> I take my comments back... But I am not sure if a new DT property is 
> the
> needed. This does not describe HW and is specific to Linux SPI NOR
> stack. How about a module parameter instead?
> Module parameter won't provide per flash granularity in controlling
> unlocking behavior. But I don't think that matters.

I don't argue against having a kernel parameter, but just wanting to 
point
out another alternative (which might be controversial):

  - What is the purpose of this unlock_all() at all. Apparently there are
    some flashes which have the protection bits set. Either at startup
    (and then they are non-volatile) or they come in that state out of 
the
    factory. The latter makes little sense IMHO.

  - Actually, all newer flashes we've used have these bits non-volatile 
and
    are unlocked by default.

So can't we have a whitelist (ie. a new flag in the spi_nor_ids) which
supresses the unlock if they haven't any block protections bit set
according to the manual? Because in this case the unlocking makes never
sense IMHO.

-michael

> 
> Tudor,
> 
> You had a patch doing something similar. Does module param sound good 
> to
> you?

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2019-12-20 12:46 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-14 19:19 [PATCH 1/2] dt-bindings: mtd: spi-nor: document new flag Michael Walle
2019-12-14 19:19 ` [PATCH 2/2] mtd: spi-nor: add option to keep lock bits Michael Walle
2019-12-16  8:54 ` [PATCH 1/2] dt-bindings: mtd: spi-nor: document new flag Vignesh Raghavendra
2019-12-16 10:29   ` Michael Walle
2019-12-19  5:33     ` Vignesh Raghavendra
2019-12-20 12:46       ` Michael Walle

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).