* [PATCH 1/2] mtd: spi-nor: core: Advance erase after the erase cmd has been completed
@ 2021-02-05 13:52 Tudor Ambarus
2021-02-05 13:52 ` [PATCH 2/2] mtd: spi-nor: core: Add dbg msg for spi_nor_erase_multi_sectors() Tudor Ambarus
2021-02-08 11:41 ` [PATCH 1/2] mtd: spi-nor: core: Advance erase after the erase cmd has been completed Pratyush Yadav
0 siblings, 2 replies; 6+ messages in thread
From: Tudor Ambarus @ 2021-02-05 13:52 UTC (permalink / raw)
To: Takahiro.Kuwano, miquel.raynal, richard, vigneshr
Cc: linux-mtd, linux-kernel, Tudor Ambarus
Wait for the erase cmd to complete and then advance the erase.
Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
drivers/mtd/spi-nor/core.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 0522304f52fa..bcaa161bc7db 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -1618,12 +1618,12 @@ static int spi_nor_erase_multi_sectors(struct spi_nor *nor, u64 addr, u32 len)
if (ret)
goto destroy_erase_cmd_list;
- addr += cmd->size;
- cmd->count--;
-
ret = spi_nor_wait_till_ready(nor);
if (ret)
goto destroy_erase_cmd_list;
+
+ addr += cmd->size;
+ cmd->count--;
}
list_del(&cmd->list);
kfree(cmd);
@@ -1704,12 +1704,12 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
if (ret)
goto erase_err;
- addr += mtd->erasesize;
- len -= mtd->erasesize;
-
ret = spi_nor_wait_till_ready(nor);
if (ret)
goto erase_err;
+
+ addr += mtd->erasesize;
+ len -= mtd->erasesize;
}
/* erase multiple sectors */
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] mtd: spi-nor: core: Add dbg msg for spi_nor_erase_multi_sectors()
2021-02-05 13:52 [PATCH 1/2] mtd: spi-nor: core: Advance erase after the erase cmd has been completed Tudor Ambarus
@ 2021-02-05 13:52 ` Tudor Ambarus
2021-02-08 11:41 ` Pratyush Yadav
2021-02-08 11:41 ` [PATCH 1/2] mtd: spi-nor: core: Advance erase after the erase cmd has been completed Pratyush Yadav
1 sibling, 1 reply; 6+ messages in thread
From: Tudor Ambarus @ 2021-02-05 13:52 UTC (permalink / raw)
To: Takahiro.Kuwano, miquel.raynal, richard, vigneshr
Cc: linux-mtd, linux-kernel, Tudor Ambarus
Useful when debugging non-uniform erase.
Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
drivers/mtd/spi-nor/core.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index bcaa161bc7db..7401c60b53e6 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -1622,6 +1622,9 @@ static int spi_nor_erase_multi_sectors(struct spi_nor *nor, u64 addr, u32 len)
if (ret)
goto destroy_erase_cmd_list;
+ dev_dbg(nor->dev, "erase_cmd->size = 0x%08x, erase_cmd->opcode = 0x%02x, erase_cmd->count = %d\n",
+ cmd->size, cmd->opcode, cmd->count);
+
addr += cmd->size;
cmd->count--;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] mtd: spi-nor: core: Advance erase after the erase cmd has been completed
2021-02-05 13:52 [PATCH 1/2] mtd: spi-nor: core: Advance erase after the erase cmd has been completed Tudor Ambarus
2021-02-05 13:52 ` [PATCH 2/2] mtd: spi-nor: core: Add dbg msg for spi_nor_erase_multi_sectors() Tudor Ambarus
@ 2021-02-08 11:41 ` Pratyush Yadav
2021-02-08 12:14 ` Tudor.Ambarus
1 sibling, 1 reply; 6+ messages in thread
From: Pratyush Yadav @ 2021-02-08 11:41 UTC (permalink / raw)
To: Tudor Ambarus
Cc: Takahiro.Kuwano, miquel.raynal, richard, vigneshr, linux-mtd,
linux-kernel
On 05/02/21 03:52PM, Tudor Ambarus wrote:
> Wait for the erase cmd to complete and then advance the erase.
>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---
> drivers/mtd/spi-nor/core.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 0522304f52fa..bcaa161bc7db 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -1618,12 +1618,12 @@ static int spi_nor_erase_multi_sectors(struct spi_nor *nor, u64 addr, u32 len)
> if (ret)
> goto destroy_erase_cmd_list;
>
> - addr += cmd->size;
> - cmd->count--;
> -
> ret = spi_nor_wait_till_ready(nor);
> if (ret)
> goto destroy_erase_cmd_list;
> +
> + addr += cmd->size;
> + cmd->count--;
> }
> list_del(&cmd->list);
> kfree(cmd);
> @@ -1704,12 +1704,12 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
> if (ret)
> goto erase_err;
>
> - addr += mtd->erasesize;
> - len -= mtd->erasesize;
> -
> ret = spi_nor_wait_till_ready(nor);
> if (ret)
> goto erase_err;
> +
> + addr += mtd->erasesize;
> + len -= mtd->erasesize;
Do these changes have any practical benefit? IMO they are worth doing
even if there is none but I'm curious what prompted this patch.
Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
> }
>
> /* erase multiple sectors */
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] mtd: spi-nor: core: Add dbg msg for spi_nor_erase_multi_sectors()
2021-02-05 13:52 ` [PATCH 2/2] mtd: spi-nor: core: Add dbg msg for spi_nor_erase_multi_sectors() Tudor Ambarus
@ 2021-02-08 11:41 ` Pratyush Yadav
2021-02-08 11:59 ` Tudor.Ambarus
0 siblings, 1 reply; 6+ messages in thread
From: Pratyush Yadav @ 2021-02-08 11:41 UTC (permalink / raw)
To: Tudor Ambarus
Cc: Takahiro.Kuwano, miquel.raynal, richard, vigneshr, linux-mtd,
linux-kernel
On 05/02/21 03:52PM, Tudor Ambarus wrote:
> Useful when debugging non-uniform erase.
>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---
> drivers/mtd/spi-nor/core.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index bcaa161bc7db..7401c60b53e6 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -1622,6 +1622,9 @@ static int spi_nor_erase_multi_sectors(struct spi_nor *nor, u64 addr, u32 len)
> if (ret)
> goto destroy_erase_cmd_list;
>
> + dev_dbg(nor->dev, "erase_cmd->size = 0x%08x, erase_cmd->opcode = 0x%02x, erase_cmd->count = %d\n",
> + cmd->size, cmd->opcode, cmd->count);
> +
I don't like the position of this debug message. This prints cmd->count
_after_ the erase is done but _before_ cmd->count is updated. It might
end up giving some wrong or misleading information. Can you either move
it before the start of the erase or after all the bookkeeping is done?
> addr += cmd->size;
> cmd->count--;
> }
> --
> 2.25.1
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] mtd: spi-nor: core: Add dbg msg for spi_nor_erase_multi_sectors()
2021-02-08 11:41 ` Pratyush Yadav
@ 2021-02-08 11:59 ` Tudor.Ambarus
0 siblings, 0 replies; 6+ messages in thread
From: Tudor.Ambarus @ 2021-02-08 11:59 UTC (permalink / raw)
To: p.yadav
Cc: Takahiro.Kuwano, miquel.raynal, richard, vigneshr, linux-mtd,
linux-kernel
On 2/8/21 1:41 PM, Pratyush Yadav wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On 05/02/21 03:52PM, Tudor Ambarus wrote:
>> Useful when debugging non-uniform erase.
>>
>> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
>> ---
>> drivers/mtd/spi-nor/core.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>> index bcaa161bc7db..7401c60b53e6 100644
>> --- a/drivers/mtd/spi-nor/core.c
>> +++ b/drivers/mtd/spi-nor/core.c
>> @@ -1622,6 +1622,9 @@ static int spi_nor_erase_multi_sectors(struct spi_nor *nor, u64 addr, u32 len)
>> if (ret)
>> goto destroy_erase_cmd_list;
>>
>> + dev_dbg(nor->dev, "erase_cmd->size = 0x%08x, erase_cmd->opcode = 0x%02x, erase_cmd->count = %d\n",
>> + cmd->size, cmd->opcode, cmd->count);
>> +
>
> I don't like the position of this debug message. This prints cmd->count
> _after_ the erase is done but _before_ cmd->count is updated. It might
oh, yes.
> end up giving some wrong or misleading information. Can you either move
> it before the start of the erase or after all the bookkeeping is done?
Before the start of the erase sounds good, but still inside the while,
so that we catch each executed command. And maybe I'll change dev_dbg
to dev_vdbg. I'll made my mind before v2.
Cheers,
ta
>
>> addr += cmd->size;
>> cmd->count--;
>> }
>> --
>> 2.25.1
>
> --
> Regards,
> Pratyush Yadav
> Texas Instruments Inc.
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] mtd: spi-nor: core: Advance erase after the erase cmd has been completed
2021-02-08 11:41 ` [PATCH 1/2] mtd: spi-nor: core: Advance erase after the erase cmd has been completed Pratyush Yadav
@ 2021-02-08 12:14 ` Tudor.Ambarus
0 siblings, 0 replies; 6+ messages in thread
From: Tudor.Ambarus @ 2021-02-08 12:14 UTC (permalink / raw)
To: p.yadav
Cc: Takahiro.Kuwano, miquel.raynal, richard, vigneshr, linux-mtd,
linux-kernel
On 2/8/21 1:41 PM, Pratyush Yadav wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On 05/02/21 03:52PM, Tudor Ambarus wrote:
>> Wait for the erase cmd to complete and then advance the erase.
>>
>> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
>> ---
>> drivers/mtd/spi-nor/core.c | 12 ++++++------
>> 1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>> index 0522304f52fa..bcaa161bc7db 100644
>> --- a/drivers/mtd/spi-nor/core.c
>> +++ b/drivers/mtd/spi-nor/core.c
>> @@ -1618,12 +1618,12 @@ static int spi_nor_erase_multi_sectors(struct spi_nor *nor, u64 addr, u32 len)
>> if (ret)
>> goto destroy_erase_cmd_list;
>>
>> - addr += cmd->size;
>> - cmd->count--;
>> -
>> ret = spi_nor_wait_till_ready(nor);
>> if (ret)
>> goto destroy_erase_cmd_list;
>> +
>> + addr += cmd->size;
>> + cmd->count--;
>> }
>> list_del(&cmd->list);
>> kfree(cmd);
>> @@ -1704,12 +1704,12 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
>> if (ret)
>> goto erase_err;
>>
>> - addr += mtd->erasesize;
>> - len -= mtd->erasesize;
>> -
>> ret = spi_nor_wait_till_ready(nor);
>> if (ret)
>> goto erase_err;
>> +
>> + addr += mtd->erasesize;
>> + len -= mtd->erasesize;
>
> Do these changes have any practical benefit? IMO they are worth doing
> even if there is none but I'm curious what prompted this patch.
I saw these when reviewing Takahiro's patches. Addr and len were gratuitously
updated even when the wait failed. We'll avoid 2 extra ops on the error path.
Plus, having them updated before the wait can be misleading for someone that
tracks them down with some debug messages. I find the code better structured,
and the code will make more sense when it is read, if using this patch.
>
> Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
Thanks,
ta
>
>> }
>>
>> /* erase multiple sectors */
>
> --
> Regards,
> Pratyush Yadav
> Texas Instruments Inc.
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-02-08 12:30 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-05 13:52 [PATCH 1/2] mtd: spi-nor: core: Advance erase after the erase cmd has been completed Tudor Ambarus
2021-02-05 13:52 ` [PATCH 2/2] mtd: spi-nor: core: Add dbg msg for spi_nor_erase_multi_sectors() Tudor Ambarus
2021-02-08 11:41 ` Pratyush Yadav
2021-02-08 11:59 ` Tudor.Ambarus
2021-02-08 11:41 ` [PATCH 1/2] mtd: spi-nor: core: Advance erase after the erase cmd has been completed Pratyush Yadav
2021-02-08 12:14 ` Tudor.Ambarus
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).