Re: [PATCH] mtd: spi-nor: winbond: add support for W25Q512NW-IQ/IN
From: Jae Hyun Yoo
Date: Fri Jul 15 2022 - 19:15:56 EST
Hi,
On 7/15/2022 4:03 PM, Michael Walle wrote:
Hi,
Am 2022-07-16 00:35, schrieb Jae Hyun Yoo:
On 7/15/2022 1:15 PM, Jae Hyun Yoo wrote:
On 7/14/2022 7:30 AM, Jae Hyun Yoo wrote:
On 7/14/2022 7:21 AM, Michael Walle wrote:
Am 2022-07-14 16:16, schrieb Michael Walle:
Am 2022-07-14 15:47, schrieb Jae Hyun Yoo:
On 7/14/2022 12:41 AM, Michael Walle wrote:
What does "doesn't boot at all" mean? Are there any kernel startup
messages?
I'm sharing the error messages below.
Thanks.
[ 0.748594] spi-nor spi0.0: w25q512nwq (65536 Kbytes)
[ 0.865216] spi-aspeed-smc 1e620000.spi: CE0 read buswidth:4
[0x406c0741]
[ 0.872833] ------------[ cut here ]------------
[ 0.877984] WARNING: CPU: 1 PID: 1 at drivers/mtd/mtdcore.c:583
add_mtd_device+0x28c/0x53c
[ 0.887237] CPU: 1 PID: 1 Comm: swapper/0 Not tainted
5.15.43-AUTOINC-dirty-23801a6 #1
Could you please try it on the latest (vanilla) linux-next?
or spi-nor/next [1] as there are quite a lot of changes. The
patches shall be based on that.
Okay. Let me try that. I tested it using 5.15.43 with back-ported
spi-nor patches from the latest. I'll back-port more changes from
the spi-nor/next and will test the INFO(0xef6020, 0, 0, 0) setting
again.
I tested the setting again after cherry picking all SPI relating changes
from the 'for-next' branch of
git://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi repository.
No luck! It's still making the same warning dump at 'add_mtd_device'
since 'mtd->erasesize' is checked as 0.
I traced it further to check if the erasesize is properly parsed from
the sfdp and checked that erase map seems parsed and initialized
correctly in 'spi_nor_parse_bfpt' but problem is, a target
mtd->erasesize is not properly selected in 'spi_nor_select_erase' since
the 'wanted_size' variable is initialized as sector size of info table
so a selected target mtd->erasesize is also 0 so looks like it's the
reason why it can't initialize mtd device if we use
INFO(0xef6020, 0, 0, 0).
Also, checked that the mtd->erasesize is set to 4096 if I enable
CONFIG_MTD_SPI_NOR_USE_4K_SECTORS so the SPI flash can be initialized
with the INFO(0xef6020, 0, 0, 0) setting but, it should cover even when
the configuration is not enabled. I think, this patch should go as it
is. The erasesize selecting issue could be fixed using a separate
patch.
Are you still sure that the INFO(0xef6020, 0, 0, 0) works in the
latest spi-next?
I also tried to fix the issue and made a fix like below.
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 502967c76c5f..f8a020f80a56 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -2117,7 +2117,7 @@ spi_nor_select_uniform_erase(struct
spi_nor_erase_map *map,
* If the current erase size is the one, stop here:
* we have found the right uniform Sector Erase command.
*/
- if (tested_erase->size == wanted_size) {
+ if (wanted_size && tested_erase->size == wanted_size) {
erase = tested_erase;
break;
}
Tested that it makes the INFO(0xef6020, 0, 0, 0) setting work and a
selected mtd->erasesize is 65536 which is what I expected for this
device.
Not sure if it's a right fix or not. Please review and let me know if
it's good to submit or not.
Ahh, I think I know whats going wrong here. Thanks!
4bait will set the erase size to 0 if there is no corresponding
opcode for the 4byte erase. So you'll end up with
et[0]: 4096 - 21h
et[1]: 0 - FFh
et[2]: 65536 - DCh
et[3]: --
And spi_nor_select_uniform_erase() will select et[1].
Could you try the following:
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index ce5d69317d46..a2c8de250e01 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -2113,6 +2113,10 @@ spi_nor_select_uniform_erase(struct
spi_nor_erase_map *map,
tested_erase = &map->erase_type[i];
+ /* Skip masked erase types. */
+ if (!tested_erase->size)
+ continue;
Yes, it also works. Do you want me to update this patch with adding this
fix? Or is it good to go as is so that the INFO table can be replaced
with SNOR_ID3 later after the fix is merged?
Thanks,
Jae
+
/*
* If the current erase size is the one, stop here:
* we have found the right uniform Sector Erase command.
-michael