Re: [PATCH v2 4/8] mtd: spi-nor: core: Introduce method for RDID op

[<Tudor.Ambarus@microchip.com>]

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@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@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
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/