From: Yicong Yang <yangyicong@hisilicon.com>
To: Pratyush Yadav <p.yadav@ti.com>
Cc: <tudor.ambarus@microchip.com>, <broonie@kernel.org>,
<miquel.raynal@bootlin.com>, <richard@nod.at>, <vigneshr@ti.com>,
<linux-mtd@lists.infradead.org>, <linux-spi@vger.kernel.org>,
<john.garry@huawei.com>, <linuxarm@openeuler.org>,
<prime.zeng@huawei.com>
Subject: Re: [PATCH 1/2] mtd: spi-nor: check 4-byte address support when parsing 4bait
Date: Mon, 1 Feb 2021 20:55:23 +0800 [thread overview]
Message-ID: <28c3dacb-e222-8d5e-b795-947b555551bf@hisilicon.com> (raw)
In-Reply-To: <20210129111649.jypytpp7dkywthwh@ti.com>
Hi Pratyush,
Thanks for the comments. Some replies below.
On 2021/1/29 19:16, Pratyush Yadav wrote:
> Hi Yicong,
>
> On 27/01/21 05:40PM, Yicong Yang wrote:
>> The spi-nor core will convert the address mode to 4-btye,
>> without checking whether 4-byte address is supported or not.
>> For example, the 16M s25fs128s1 can work under both 3-byte
>> and 4-byte address and provides a 4bait table. The spi-nor
>> will drive the flash under 4-byte address mode after parsing
>> the 4bait and will cause it unusable on platforms doesn't
>> support 4-byte.
>
> Another problem caused by 4BAIT parser prematurely selecting the address
> width. See [0].
>
> Let's fix this 4BAIT problem once and for all. Instead of setting
> nor->addr_width to 4 in spi_nor_parse_4bait(), just set SNOR_F_HAS_4BAIT
> (which is already being done). Then in spi_nor_default_setup(), use this
> information when negotiating the read/write/program commands with the
> controller to determine the correct address width to use.
The idea is great. ideally we should check the buswidth and commands in
spi_nor_default_setup() and finally set address width in
spi_nor_set_addr_width().
>
> This refactor is easier said than done. We don't associate address width
> information with a command. Just protocol, opcode, and dummy cycles
> (only for read commands). A new mechanism needs to be added where we
> associate a set of supported addresses with a command and then the
> command negotiation can use all this information to arrive at the
> optimal set of commands.
>
> With this in mind, it would be great if you can come up with a patch to
> add such a mechanism. But I would also be OK with this fix with the
> condition that it is clearly marked as a temporary fix, and mentions
> what should ideally be done.
>
i'd like to get this issue fixed first to make the flash work properly,
if possible. then to do the refactor as it may take me sometime.
>> Add checking of 4-byte address support when parsing the 4bait
>> table, stop converting the address mode if it's not supported.
>>
>> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
>> ---
>> drivers/mtd/spi-nor/sfdp.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 48 insertions(+)
>>
>> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
>> index 6ee7719..fdafc9b 100644
>> --- a/drivers/mtd/spi-nor/sfdp.c
>> +++ b/drivers/mtd/spi-nor/sfdp.c
>> @@ -940,6 +940,27 @@ static int spi_nor_parse_smpt(struct spi_nor *nor,
>> return ret;
>> }
>>
>> +static int spi_nor_spimem_check_4byte_addr(struct spi_nor *nor,
>> + const struct spi_nor_read_command *read)
>> +{
>> + struct spi_mem_op op = SPI_MEM_OP(SPI_MEM_OP_CMD(read->opcode, 1),
>> + SPI_MEM_OP_ADDR(4, 0, 1),
>> + SPI_MEM_OP_DUMMY(0, 1),
>> + SPI_MEM_OP_DATA_IN(0, NULL, 1));
>
> Set buswidths to 0 here...
ok.
>
>> +
>> + op.cmd.buswidth = spi_nor_get_protocol_inst_nbits(read->proto);
>> + op.addr.buswidth = spi_nor_get_protocol_addr_nbits(read->proto);
>> + op.data.buswidth = spi_nor_get_protocol_data_nbits(read->proto);
>> + op.dummy.buswidth = op.addr.buswidth;
>
> ... and use spi_nor_spimem_setup_op() to do this. It will also take care
> of setting up DTR ops.
will change to the new function, i didn't notice it. :)
Thanks,
Yicong
>
>> + op.dummy.nbytes = (read->num_mode_clocks + read->num_wait_states) *
>> + op.dummy.buswidth / 8;
>> +
>> + if (!spi_mem_supports_op(nor->spimem, &op))
>> + return -EOPNOTSUPP;
>> +
>> + return 0;
>> +}
>> +
>> /**
>> * spi_nor_parse_4bait() - parse the 4-Byte Address Instruction Table
>> * @nor: pointer to a 'struct spi_nor'.
>> @@ -1061,6 +1082,33 @@ static int spi_nor_parse_4bait(struct spi_nor *nor,
>> goto out;
>>
>> /*
>> + * Check whether the 4-byte address is supported before converting
>> + * the instruction set to 4-byte.
>> + */
>> + if (nor->spimem) {
>> + bool support = false;
>> +
>> + for (i = 0; i < SNOR_CMD_READ_MAX; i++) {
>> + struct spi_nor_read_command read_cmd;
>> +
>> + memcpy(&read_cmd, ¶ms->reads[i], sizeof(read_cmd));
>> +
>> + read_cmd.opcode = spi_nor_convert_3to4_read(read_cmd.opcode);
>> + if (!spi_nor_spimem_check_4byte_addr(nor, &read_cmd)) {
>> + support = true;
>> + break;
>> + }
>> + }
>> +
>> + /*
>> + * No supported 4-byte instruction is found, stop parsing the
>> + * 4bait table.
>> + */
>> + if (!support)
>> + goto out;
>> + }
>> +
>> + /*
>> * Discard all operations from the 4-byte instruction set which are
>> * not supported by this memory.
>> */
>
> [0] https://lore.kernel.org/linux-mtd/20201212115817.5122-1-vigneshr@ti.com/
>
next prev parent reply other threads:[~2021-02-01 12:56 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-01-27 9:40 [PATCH 0/2] Add check of 4-byte when parsing SFDP 4bait table Yicong Yang
2021-01-27 9:40 ` [PATCH 1/2] mtd: spi-nor: check 4-byte address support when parsing 4bait Yicong Yang
[not found] ` <20210129111649.jypytpp7dkywthwh@ti.com>
2021-02-01 12:55 ` Yicong Yang [this message]
2021-01-27 9:40 ` [PATCH 2/2] spi: hisi-sfc-v3xx: add address mode check Yicong Yang
2021-01-27 17:15 ` (subset) [PATCH 0/2] Add check of 4-byte when parsing SFDP 4bait table Mark Brown
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=28c3dacb-e222-8d5e-b795-947b555551bf@hisilicon.com \
--to=yangyicong@hisilicon.com \
--cc=broonie@kernel.org \
--cc=john.garry@huawei.com \
--cc=linux-mtd@lists.infradead.org \
--cc=linux-spi@vger.kernel.org \
--cc=linuxarm@openeuler.org \
--cc=miquel.raynal@bootlin.com \
--cc=p.yadav@ti.com \
--cc=prime.zeng@huawei.com \
--cc=richard@nod.at \
--cc=tudor.ambarus@microchip.com \
--cc=vigneshr@ti.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).