From: Alexander Sverdlin <[email protected]>
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 <[email protected]>
---
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
From: Alexander Sverdlin <[email protected]>
This fixes sector erase on mt25qu256aba8e12-1sit.
Looks like others like mt35xu512aba could be affected as well.
Signed-off-by: Alexander Sverdlin <[email protected]>
---
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
Hi Alexander,
On 09/12/21 11:08AM, Alexander A Sverdlin wrote:
> From: Alexander Sverdlin <[email protected]>
>
> 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 <[email protected]>
> ---
> 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.
On 09/12/21 11:08AM, Alexander A Sverdlin wrote:
> From: Alexander Sverdlin <[email protected]>
>
> 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 <[email protected]>
> ---
> 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.
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 <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>> 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/[email protected]/
Hi Tudor!
On 18/07/2022 18:50, [email protected] 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/[email protected]/
>
--
Best regards,
Alexander Sverdlin.