[PATCH v2 4/8] mtd: spi-nor: core: Introduce method for RDID op
Tudor.Ambarus at microchip.com
Tudor.Ambarus at microchip.com
Tue Mar 29 23:53:45 PDT 2022
On 3/21/22 19:39, Pratyush Yadav wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On 21/03/22 01:18PM, Tudor.Ambarus at microchip.com wrote:
>> On 3/21/22 14:21, Pratyush Yadav wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On 28/02/22 01:17PM, Tudor Ambarus wrote:
>>>> RDID is used in the core to auto detect the flash, but also by some
>>>> manufacturer drivers that contain flashes that support Octal DTR mode,
>>>> so that they can read the flash ID after the switch to Octal DTR was made
>>>> to test if the switch was successful. Introduce a core method for RDID op
>>>> to avoid code duplication.
>>>>
>>>> Signed-off-by: Tudor Ambarus <tudor.ambarus at microchip.com>
>>>> ---
>>>> drivers/mtd/spi-nor/core.c | 58 ++++++++++++++++++++++++++------------
>>>> drivers/mtd/spi-nor/core.h | 9 ++++++
>>>> 2 files changed, 49 insertions(+), 18 deletions(-)
>>>>
>>>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>>>> index b1d6fa65417d..281e3d25f74c 100644
>>>> --- a/drivers/mtd/spi-nor/core.c
>>>> +++ b/drivers/mtd/spi-nor/core.c
>>>> @@ -369,6 +369,41 @@ int spi_nor_write_disable(struct spi_nor *nor)
>>>> return ret;
>>>> }
>>>>
>>>> +/**
>>>> + * spi_nor_read_id() - Read the JEDEC ID.
>>>> + * @nor: pointer to 'struct spi_nor'.
>>>> + * @naddr: number of address bytes to send. Can be zero if the operation
>>>> + * does not need to send an address.
>>>> + * @ndummy: number of dummy bytes to send after an opcode or address. Can
>>>> + * be zero if the operation does not require dummy bytes.
>>>> + * @id: pointer to a DMA-able buffer where the value of the JEDEC ID
>>>> + * will be written.
>>>> + * @reg_proto: the SPI protocol for register operation.
>>>> + *
>>>> + * Return: 0 on success, -errno otherwise.
>>>> + */
>>>> +int spi_nor_read_id(struct spi_nor *nor, u8 naddr, u8 ndummy, u8 *id,
>>>> + enum spi_nor_protocol reg_proto)
>>>
>>> Nitpick: Could just call it 'proto'.
>>
>> sure, will update
>>
>>>
>>>> +{
>>>> + int ret;
>>>> +
>>>> + if (nor->spimem) {
>>>> + struct spi_mem_op op =
>>>> + SPI_NOR_READID_OP(naddr, ndummy, id, SPI_NOR_MAX_ID_LEN);
>>>> +
>>>> + spi_nor_spimem_setup_op(nor, &op, reg_proto);
>>>> + ret = spi_mem_exec_op(nor->spimem, &op);
>>>> + } else {
>>>> + ret = nor->controller_ops->read_reg(nor, SPINOR_OP_RDID, id,
>>>> + SPI_NOR_MAX_ID_LEN);
>>>> + }
>>>> +
>>>> + if (ret)
>>>> + dev_dbg(nor->dev, "error %d reading JEDEC ID\n", ret);
>>>
>>> I think this message should be in spi_nor_detect(). Let octal DTR enable
>>
>> As of now every SPI NOR operation that return an error also prints a dbg
>> message. I like this because it offers a smaller granularity on the error
>> cause.
>
> Yes, but I think this message would be misleading. If someone sees
> "error reading JEDEC ID", they would think flash detection itself has
> failed, not that we failed to switch to Octal DTR mode.
>
>>
>>> methods print their own, more specific error messages.
>>
>> How about duplicating the error in the octal dtr enable methods if you
>> feel it is worth it?
>
> They should at the very least explain that reading ID failed _after_
> attempting to switch to Octal DTR. But I think it would just be simpler
> if this is not printed here and the caller has the flexibility to
> explain the error.
If the first readID fails, the one that identifies the flash, then the
octal dtr will not be run, thus a single error message. When octal dtr
fails, 2 errors can be printed, one specifying what failed (the read ID
command) and the second where it failed (at the octal dtr enable method).
But I don't care too much, I'll follow your suggestion.
>
>>
>>>
>>>> +
>>>> + return ret;
>>>> +}
>>>> +
>>>> /**
>>>> * spi_nor_read_sr() - Read the Status Register.
>>>> * @nor: pointer to 'struct spi_nor'.
>>>> @@ -1649,28 +1684,15 @@ static const struct flash_info *spi_nor_match_id(struct spi_nor *nor,
>>>> return NULL;
>>>> }
>>>>
>>>> -static const struct flash_info *spi_nor_read_id(struct spi_nor *nor)
>>>> +static const struct flash_info *spi_nor_detect(struct spi_nor *nor)
>>>> {
>>>> const struct flash_info *info;
>>>> u8 *id = nor->bouncebuf;
>>>> int ret;
>>>>
>>>> - if (nor->spimem) {
>>>> - struct spi_mem_op op =
>>>> - SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_RDID, 1),
>>>> - SPI_MEM_OP_NO_ADDR,
>>>> - SPI_MEM_OP_NO_DUMMY,
>>>> - SPI_MEM_OP_DATA_IN(SPI_NOR_MAX_ID_LEN, id, 1));
>>>> -
>>>> - ret = spi_mem_exec_op(nor->spimem, &op);
>>>> - } else {
>>>> - ret = nor->controller_ops->read_reg(nor, SPINOR_OP_RDID, id,
>>>> - SPI_NOR_MAX_ID_LEN);
>>>> - }
>>>> - if (ret) {
>>>> - dev_dbg(nor->dev, "error %d reading JEDEC ID\n", ret);
>>>> + ret = spi_nor_read_id(nor, 0, 0, id, nor->reg_proto);
>>>
>>> Hmm, I wonder if it is better to explicitly use SNOR_PROTO_1_1_1 so
>>> clearly signify that this is intended to use 1S-1S-1S only. What do you
>>> think?
>>
>> I would keep it as it is for now, because it offers flexibility.
>> If we ever gonna determine the protocol at runtime this will come in handy
>> because it will work without touching the code. JESD216 suggests an algorithm
>> that tries to determine the mode depending on the SFDP signature.
>
> I was thinking exactly this but came to the opposite conclusion ;-). I
> think this would imply that other protocols can be used to detect the
> flash which is not true.
It can become true. As you already specified 8d-8d-8d is supported by some flashes
and we can implement hooks for their specific 8d-8d-8d readID command. The logic
will complicate a bit as one has to adjust the hwcaps before issuing the 8d-8d-8d
readID, but it's doable. Otherwise, if the bootloaders pass you the flash in octal
dtr mode, you'll have to disable it, issue readID is 1-1-1 and then re-enable it.
>
> But I have no strong preferences here. Either is fine by me.
I don't have strong preferences either, but it seems that there's room for discussion
on this, so I would keep it for later. Is that fine?
I can add a comment if you prefer, specifying that at this point nor->reg_proto is in
1-1-1 mode.
Cheers,
ta
More information about the linux-mtd
mailing list