linux-mtd.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2] mtd: spi-nor: Introduce erase_proto
@ 2021-12-09 10:08 Alexander A Sverdlin
  2021-12-09 10:08 ` [PATCH 2/2] mtd: spi-nor: micron/st: Hardcode erase_proto to 1-1-1 Alexander A Sverdlin
  2021-12-16 20:05 ` [PATCH 1/2] mtd: spi-nor: Introduce erase_proto Pratyush Yadav
  0 siblings, 2 replies; 6+ messages in thread
From: Alexander A Sverdlin @ 2021-12-09 10:08 UTC (permalink / raw)
  To: linux-mtd
  Cc: Alexander Sverdlin, Tudor Ambarus, Pratyush Yadav, Michael Walle,
	Miquel Raynal, Richard Weinberger, Vignesh Raghavendra,
	linux-kernel

From: Alexander Sverdlin <alexander.sverdlin@nokia.com>

I've been looking into non-working erase on mt25qu256a and pinpointed it to
be write_proto 1-4-4 selected from SFDP while the chip only supports 1-1-0
erase.

For now just introduce the separate protocol without functional change and
leave the real fix for the following patch.

Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
---
 drivers/mtd/spi-nor/core.c  | 9 ++++++---
 include/linux/mtd/spi-nor.h | 4 +++-
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 2e21d5a..dcd02ea 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -177,7 +177,7 @@ static int spi_nor_controller_ops_write_reg(struct spi_nor *nor, u8 opcode,
 
 static int spi_nor_controller_ops_erase(struct spi_nor *nor, loff_t offs)
 {
-	if (spi_nor_protocol_is_dtr(nor->write_proto))
+	if (spi_nor_protocol_is_dtr(nor->erase_proto))
 		return -EOPNOTSUPP;
 
 	return nor->controller_ops->erase(nor, offs);
@@ -1186,7 +1186,7 @@ static int spi_nor_erase_chip(struct spi_nor *nor)
 				   SPI_MEM_OP_NO_DUMMY,
 				   SPI_MEM_OP_NO_DATA);
 
-		spi_nor_spimem_setup_op(nor, &op, nor->write_proto);
+		spi_nor_spimem_setup_op(nor, &op, nor->erase_proto);
 
 		ret = spi_mem_exec_op(nor->spimem, &op);
 	} else {
@@ -1331,7 +1331,7 @@ int spi_nor_erase_sector(struct spi_nor *nor, u32 addr)
 				   SPI_MEM_OP_NO_DUMMY,
 				   SPI_MEM_OP_NO_DATA);
 
-		spi_nor_spimem_setup_op(nor, &op, nor->write_proto);
+		spi_nor_spimem_setup_op(nor, &op, nor->erase_proto);
 
 		return spi_mem_exec_op(nor->spimem, &op);
 	} else if (nor->controller_ops->erase) {
@@ -2727,6 +2727,9 @@ static void spi_nor_late_init_params(struct spi_nor *nor)
 	 */
 	if (nor->flags & SNOR_F_HAS_LOCK && !nor->params->locking_ops)
 		spi_nor_init_default_locking_ops(nor);
+
+	if (!nor->erase_proto)
+		nor->erase_proto = nor->write_proto;
 }
 
 /**
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index fc90fce..23a901b 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -381,7 +381,8 @@ struct spi_nor_flash_parameter;
  * @cmd_ext_type:	the command opcode extension type for DTR mode.
  * @read_proto:		the SPI protocol for read operations
  * @write_proto:	the SPI protocol for write operations
- * @reg_proto:		the SPI protocol for read_reg/write_reg/erase operations
+ * @reg_proto:		the SPI protocol for read_reg/write_reg operations
+ * @erase_proto:	the SPI protocol for erase operations
  * @sfdp:		the SFDP data of the flash
  * @controller_ops:	SPI NOR controller driver specific operations.
  * @params:		[FLASH-SPECIFIC] SPI NOR flash parameters and settings.
@@ -408,6 +409,7 @@ struct spi_nor {
 	enum spi_nor_protocol	read_proto;
 	enum spi_nor_protocol	write_proto;
 	enum spi_nor_protocol	reg_proto;
+	enum spi_nor_protocol	erase_proto;
 	bool			sst_write_second;
 	u32			flags;
 	enum spi_nor_cmd_ext	cmd_ext_type;
-- 
2.10.2


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

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH 2/2] mtd: spi-nor: micron/st: Hardcode erase_proto to 1-1-1
  2021-12-09 10:08 [PATCH 1/2] mtd: spi-nor: Introduce erase_proto Alexander A Sverdlin
@ 2021-12-09 10:08 ` Alexander A Sverdlin
  2021-12-16 20:07   ` Pratyush Yadav
  2021-12-16 20:05 ` [PATCH 1/2] mtd: spi-nor: Introduce erase_proto Pratyush Yadav
  1 sibling, 1 reply; 6+ messages in thread
From: Alexander A Sverdlin @ 2021-12-09 10:08 UTC (permalink / raw)
  To: linux-mtd
  Cc: Alexander Sverdlin, Tudor Ambarus, Pratyush Yadav, Michael Walle,
	Miquel Raynal, Richard Weinberger, Vignesh Raghavendra,
	linux-kernel

From: Alexander Sverdlin <alexander.sverdlin@nokia.com>

This fixes sector erase on mt25qu256aba8e12-1sit.
Looks like others like mt35xu512aba could be affected as well.

Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
---
 drivers/mtd/spi-nor/micron-st.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/mtd/spi-nor/micron-st.c b/drivers/mtd/spi-nor/micron-st.c
index 2f3054b..058bbb7 100644
--- a/drivers/mtd/spi-nor/micron-st.c
+++ b/drivers/mtd/spi-nor/micron-st.c
@@ -267,6 +267,12 @@ static void micron_st_default_init(struct spi_nor *nor)
 	nor->flags &= ~SNOR_F_HAS_16BIT_SR;
 	nor->params->quad_enable = NULL;
 	nor->params->set_4byte_addr_mode = st_micron_set_4byte_addr_mode;
+
+	/*
+	 * mt25qu doesn't support all possible write protocols for erase, only
+	 * 1-1-0, 2-2-0, 4-4-0.
+	 */
+	nor->erase_proto = SNOR_PROTO_1_1_1;
 }
 
 static const struct spi_nor_fixups micron_st_fixups = {
-- 
2.10.2


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

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] mtd: spi-nor: Introduce erase_proto
  2021-12-09 10:08 [PATCH 1/2] mtd: spi-nor: Introduce erase_proto Alexander A Sverdlin
  2021-12-09 10:08 ` [PATCH 2/2] mtd: spi-nor: micron/st: Hardcode erase_proto to 1-1-1 Alexander A Sverdlin
@ 2021-12-16 20:05 ` Pratyush Yadav
  2022-07-18 16:50   ` Tudor.Ambarus
  1 sibling, 1 reply; 6+ messages in thread
From: Pratyush Yadav @ 2021-12-16 20:05 UTC (permalink / raw)
  To: Alexander A Sverdlin
  Cc: linux-mtd, Tudor Ambarus, Michael Walle, Miquel Raynal,
	Richard Weinberger, Vignesh Raghavendra, linux-kernel

Hi Alexander,

On 09/12/21 11:08AM, Alexander A Sverdlin wrote:
> From: Alexander Sverdlin <alexander.sverdlin@nokia.com>
> 
> I've been looking into non-working erase on mt25qu256a and pinpointed it to
> be write_proto 1-4-4 selected from SFDP while the chip only supports 1-1-0
> erase.
> 
> For now just introduce the separate protocol without functional change and
> leave the real fix for the following patch.
> 
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
> ---
>  drivers/mtd/spi-nor/core.c  | 9 ++++++---
>  include/linux/mtd/spi-nor.h | 4 +++-
>  2 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 2e21d5a..dcd02ea 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -177,7 +177,7 @@ static int spi_nor_controller_ops_write_reg(struct spi_nor *nor, u8 opcode,
>  
>  static int spi_nor_controller_ops_erase(struct spi_nor *nor, loff_t offs)
>  {
> -	if (spi_nor_protocol_is_dtr(nor->write_proto))
> +	if (spi_nor_protocol_is_dtr(nor->erase_proto))
>  		return -EOPNOTSUPP;
>  
>  	return nor->controller_ops->erase(nor, offs);
> @@ -1186,7 +1186,7 @@ static int spi_nor_erase_chip(struct spi_nor *nor)
>  				   SPI_MEM_OP_NO_DUMMY,
>  				   SPI_MEM_OP_NO_DATA);
>  
> -		spi_nor_spimem_setup_op(nor, &op, nor->write_proto);
> +		spi_nor_spimem_setup_op(nor, &op, nor->erase_proto);
>  
>  		ret = spi_mem_exec_op(nor->spimem, &op);
>  	} else {
> @@ -1331,7 +1331,7 @@ int spi_nor_erase_sector(struct spi_nor *nor, u32 addr)
>  				   SPI_MEM_OP_NO_DUMMY,
>  				   SPI_MEM_OP_NO_DATA);
>  
> -		spi_nor_spimem_setup_op(nor, &op, nor->write_proto);
> +		spi_nor_spimem_setup_op(nor, &op, nor->erase_proto);
>  
>  		return spi_mem_exec_op(nor->spimem, &op);
>  	} else if (nor->controller_ops->erase) {
> @@ -2727,6 +2727,9 @@ static void spi_nor_late_init_params(struct spi_nor *nor)
>  	 */
>  	if (nor->flags & SNOR_F_HAS_LOCK && !nor->params->locking_ops)
>  		spi_nor_init_default_locking_ops(nor);
> +
> +	if (!nor->erase_proto)
> +		nor->erase_proto = nor->write_proto;

I get that you are trying to not break any existing flashes with this, 
but I don't quite like it. We should keep the same initialization flow 
with erase_proto as with write_proto, read_proto, etc. That is, 
initialize it to SNOR_PROTO_1_1_1 in spi_nor_scan() and then let the 
initialization procedure change it as needed.

The problem with this is of course that it could break some flashes by 
selecting the wrong erase. I would expect _most_ flashes to use 
erase_proto as 1-1-1 but I of course haven't went and looked at every 
single flash to point out the exceptions.

I would like to hear from others if they think it is okay to do this.

>  }
>  
>  /**

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] mtd: spi-nor: micron/st: Hardcode erase_proto to 1-1-1
  2021-12-09 10:08 ` [PATCH 2/2] mtd: spi-nor: micron/st: Hardcode erase_proto to 1-1-1 Alexander A Sverdlin
@ 2021-12-16 20:07   ` Pratyush Yadav
  0 siblings, 0 replies; 6+ messages in thread
From: Pratyush Yadav @ 2021-12-16 20:07 UTC (permalink / raw)
  To: Alexander A Sverdlin
  Cc: linux-mtd, Tudor Ambarus, Michael Walle, Miquel Raynal,
	Richard Weinberger, Vignesh Raghavendra, linux-kernel

On 09/12/21 11:08AM, Alexander A Sverdlin wrote:
> From: Alexander Sverdlin <alexander.sverdlin@nokia.com>
> 
> This fixes sector erase on mt25qu256aba8e12-1sit.
> Looks like others like mt35xu512aba could be affected as well.

Indeed. mt35xu512aba would need to set erase_proto to 8D-8D-8D mode.

> 
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
> ---
>  drivers/mtd/spi-nor/micron-st.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/mtd/spi-nor/micron-st.c b/drivers/mtd/spi-nor/micron-st.c
> index 2f3054b..058bbb7 100644
> --- a/drivers/mtd/spi-nor/micron-st.c
> +++ b/drivers/mtd/spi-nor/micron-st.c
> @@ -267,6 +267,12 @@ static void micron_st_default_init(struct spi_nor *nor)
>  	nor->flags &= ~SNOR_F_HAS_16BIT_SR;
>  	nor->params->quad_enable = NULL;
>  	nor->params->set_4byte_addr_mode = st_micron_set_4byte_addr_mode;
> +
> +	/*
> +	 * mt25qu doesn't support all possible write protocols for erase, only
> +	 * 1-1-0, 2-2-0, 4-4-0.
> +	 */
> +	nor->erase_proto = SNOR_PROTO_1_1_1;

If this is only a mt25qu thing, why do it for all Micron flashes? 
Anyway, _if_ you do as I suggest in patch 1, this won't be needed.

>  }
>  
>  static const struct spi_nor_fixups micron_st_fixups = {
> -- 
> 2.10.2
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] mtd: spi-nor: Introduce erase_proto
  2021-12-16 20:05 ` [PATCH 1/2] mtd: spi-nor: Introduce erase_proto Pratyush Yadav
@ 2022-07-18 16:50   ` Tudor.Ambarus
  2022-07-25 14:54     ` Alexander Sverdlin
  0 siblings, 1 reply; 6+ messages in thread
From: Tudor.Ambarus @ 2022-07-18 16:50 UTC (permalink / raw)
  To: p.yadav, alexander.sverdlin
  Cc: linux-mtd, michael, miquel.raynal, richard, vigneshr, linux-kernel

On 12/16/21 22:05, Pratyush Yadav wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hi Alexander,
> 
> On 09/12/21 11:08AM, Alexander A Sverdlin wrote:
>> From: Alexander Sverdlin <alexander.sverdlin@nokia.com>
>>
>> I've been looking into non-working erase on mt25qu256a and pinpointed it to
>> be write_proto 1-4-4 selected from SFDP while the chip only supports 1-1-0
>> erase.
>>
>> For now just introduce the separate protocol without functional change and
>> leave the real fix for the following patch.
>>
>> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
>> ---
>>  drivers/mtd/spi-nor/core.c  | 9 ++++++---
>>  include/linux/mtd/spi-nor.h | 4 +++-
>>  2 files changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>> index 2e21d5a..dcd02ea 100644
>> --- a/drivers/mtd/spi-nor/core.c
>> +++ b/drivers/mtd/spi-nor/core.c
>> @@ -177,7 +177,7 @@ static int spi_nor_controller_ops_write_reg(struct spi_nor *nor, u8 opcode,
>>
>>  static int spi_nor_controller_ops_erase(struct spi_nor *nor, loff_t offs)
>>  {
>> -     if (spi_nor_protocol_is_dtr(nor->write_proto))
>> +     if (spi_nor_protocol_is_dtr(nor->erase_proto))
>>               return -EOPNOTSUPP;
>>
>>       return nor->controller_ops->erase(nor, offs);
>> @@ -1186,7 +1186,7 @@ static int spi_nor_erase_chip(struct spi_nor *nor)
>>                                  SPI_MEM_OP_NO_DUMMY,
>>                                  SPI_MEM_OP_NO_DATA);
>>
>> -             spi_nor_spimem_setup_op(nor, &op, nor->write_proto);
>> +             spi_nor_spimem_setup_op(nor, &op, nor->erase_proto);
>>
>>               ret = spi_mem_exec_op(nor->spimem, &op);
>>       } else {
>> @@ -1331,7 +1331,7 @@ int spi_nor_erase_sector(struct spi_nor *nor, u32 addr)
>>                                  SPI_MEM_OP_NO_DUMMY,
>>                                  SPI_MEM_OP_NO_DATA);
>>
>> -             spi_nor_spimem_setup_op(nor, &op, nor->write_proto);
>> +             spi_nor_spimem_setup_op(nor, &op, nor->erase_proto);
>>
>>               return spi_mem_exec_op(nor->spimem, &op);
>>       } else if (nor->controller_ops->erase) {
>> @@ -2727,6 +2727,9 @@ static void spi_nor_late_init_params(struct spi_nor *nor)
>>        */
>>       if (nor->flags & SNOR_F_HAS_LOCK && !nor->params->locking_ops)
>>               spi_nor_init_default_locking_ops(nor);
>> +
>> +     if (!nor->erase_proto)
>> +             nor->erase_proto = nor->write_proto;
> 
> I get that you are trying to not break any existing flashes with this,
> but I don't quite like it. We should keep the same initialization flow
> with erase_proto as with write_proto, read_proto, etc. That is,
> initialize it to SNOR_PROTO_1_1_1 in spi_nor_scan() and then let the
> initialization procedure change it as needed.
> 
> The problem with this is of course that it could break some flashes by
> selecting the wrong erase. I would expect _most_ flashes to use
> erase_proto as 1-1-1 but I of course haven't went and looked at every
> single flash to point out the exceptions.
> 
> I would like to hear from others if they think it is okay to do this.
> 

Doesn't [1] solve Alexander's problem? Alexander, would you please test
Patrice's patch and provide a Tested-by tag if everything is ok?

Thanks,
ta

[1] https://patchwork.ozlabs.org/project/linux-mtd/patch/20220629133013.3382393-1-patrice.chotard@foss.st.com/

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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] mtd: spi-nor: Introduce erase_proto
  2022-07-18 16:50   ` Tudor.Ambarus
@ 2022-07-25 14:54     ` Alexander Sverdlin
  0 siblings, 0 replies; 6+ messages in thread
From: Alexander Sverdlin @ 2022-07-25 14:54 UTC (permalink / raw)
  To: Tudor.Ambarus, p.yadav
  Cc: linux-mtd, michael, miquel.raynal, richard, vigneshr, linux-kernel

Hi Tudor!

On 18/07/2022 18:50, Tudor.Ambarus@microchip.com wrote:
>>> @@ -2727,6 +2727,9 @@ static void spi_nor_late_init_params(struct spi_nor *nor)
>>>        */
>>>       if (nor->flags & SNOR_F_HAS_LOCK && !nor->params->locking_ops)
>>>               spi_nor_init_default_locking_ops(nor);
>>> +
>>> +     if (!nor->erase_proto)
>>> +             nor->erase_proto = nor->write_proto;
>> I get that you are trying to not break any existing flashes with this,
>> but I don't quite like it. We should keep the same initialization flow
>> with erase_proto as with write_proto, read_proto, etc. That is,
>> initialize it to SNOR_PROTO_1_1_1 in spi_nor_scan() and then let the
>> initialization procedure change it as needed.
>>
>> The problem with this is of course that it could break some flashes by
>> selecting the wrong erase. I would expect _most_ flashes to use
>> erase_proto as 1-1-1 but I of course haven't went and looked at every
>> single flash to point out the exceptions.
>>
>> I would like to hear from others if they think it is okay to do this.
>>
> Doesn't [1] solve Alexander's problem? Alexander, would you please test
> Patrice's patch and provide a Tested-by tag if everything is ok?

Yes, looks good, provided the Tested-by tag.

> Thanks,
> ta
> 
> [1] https://patchwork.ozlabs.org/project/linux-mtd/patch/20220629133013.3382393-1-patrice.chotard@foss.st.com/
> 

-- 
Best regards,
Alexander Sverdlin.

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

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-07-25 14:55 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-09 10:08 [PATCH 1/2] mtd: spi-nor: Introduce erase_proto Alexander A Sverdlin
2021-12-09 10:08 ` [PATCH 2/2] mtd: spi-nor: micron/st: Hardcode erase_proto to 1-1-1 Alexander A Sverdlin
2021-12-16 20:07   ` Pratyush Yadav
2021-12-16 20:05 ` [PATCH 1/2] mtd: spi-nor: Introduce erase_proto Pratyush Yadav
2022-07-18 16:50   ` Tudor.Ambarus
2022-07-25 14:54     ` Alexander Sverdlin

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).