[PATCH v2 4/8] mtd: spi-nor: core: Introduce method for RDID op
Pratyush Yadav
p.yadav at ti.com
Wed Mar 30 11:49:04 PDT 2022
On 30/03/22 06:53AM, Tudor.Ambarus at microchip.com wrote:
> 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.
Right.
>
> >
> > 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?
Fine by me. It should be fine without the comment. It is not too hard to
see what nor->reg_proto is initialized to.
> I can add a comment if you prefer, specifying that at this point nor->reg_proto is in
> 1-1-1 mode.
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
More information about the linux-mtd
mailing list