Re: [PATCH v2 1/2] mtd: spi-nor: add Octal DTR support for Macronix flash

[Tudor Ambarus <tudor.ambarus@linaro.org>]

On 8/1/23 08:24, liao jaime wrote:
> Hi Tudor
> 
>>
>>
>>
>> On 7/27/23 10:16, Jaime Liao wrote:
>>> From: JaimeLiao <jaimeliao@mxic.com.tw>
>>>
>>> Create Macronix specify method for enable Octal DTR mode and
>>> set 20 dummy cycles to allow running at the maximum supported
>>> frequency for Macronix Octal flash.
>>
>> 20 dummy cycles is particular for a specific operating frequency.
>>
>> If you test the flash at different speeds, let's say at 30 MHz, and
>> at their highest frequency (133, 200 MHZ?) does the flash work with
>> current settings?
> Yes, datasheet include "Dummy Cycle and Frequency Table" for checking.
> For Macronix spi-nor flash, 20 or 18 dummy cycles should be use at 200MHz.
> It alse means that DC 20 is suit for other frequency(<=200MHz).
> 

You didn't answer my question. Does the flash work at 30 MHz by using 20
dummy cycles?
>>>
>>> For enable Macronix flashes in Octal DTR mode, Configuration
>>> Register2 DOPI bit should be set and DC bit setting for dummy
>>> cycles.
>>
>> Use imperative.
> I didn't get it.

Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
to do frotz", as if you are giving orders to the codebase to change
its behaviour. Please read:
https://www.kernel.org/doc/html/latest/process/submitting-patches.html

> 
>>>
>>> Signed-off-by: JaimeLiao <jaimeliao@mxic.com.tw>
>>> Co-developed-by: Tudor Ambarus <tudor.ambarus@linaro.org>
>>> ---
>>>  drivers/mtd/spi-nor/macronix.c | 97 ++++++++++++++++++++++++++++++++++
>>>  1 file changed, 97 insertions(+)
>>>
>>> diff --git a/drivers/mtd/spi-nor/macronix.c b/drivers/mtd/spi-nor/macronix.c
>>> index 04888258e891..b397fd274868 100644
>>> --- a/drivers/mtd/spi-nor/macronix.c
>>> +++ b/drivers/mtd/spi-nor/macronix.c
>>> @@ -8,6 +8,22 @@
>>>
>>>  #include "core.h"
>>>
>>> +#define SPINOR_OP_MXIC_RD_ANY_REG    0x71            /* Read volatile configuration register 2 */
>>> +#define SPINOR_OP_MXIC_WR_ANY_REG    0x72            /* Write volatile configuration register 2 */
>>> +#define SPINOR_REG_MXIC_CR2_MODE     0x00000000      /* For setting octal DTR mode */
>>> +#define SPINOR_REG_MXIC_CR2_DC               0x00000300      /* For setting dummy cycles */
>>> +#define SPINOR_REG_MXIC_OPI_DTR_EN   0x2             /* Enable Octal DTR */
>>> +#define SPINOR_REG_MXIC_SPI_EN               0x0             /* Enable SPI */
>>> +#define SPINOR_REG_MXIC_DC_20                0x0             /* Setting dummy cycles to 20 */
>>> +#define SPINOR_REG_MXIC_ADDR_BYTES   4               /* Fixed R/W volatile address bytes to 4 */
>>> +
>>> +/* Macronix SPI NOR flash operations. */
>>> +#define MXIC_NOR_WR_ANY_REG_OP(naddr, addr, ndata, buf)              \
>>> +     SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_MXIC_WR_ANY_REG, 0),                \
>>> +                SPI_MEM_OP_ADDR(naddr, addr, 0),                     \
>>> +                SPI_MEM_OP_NO_DUMMY,                                 \
>>> +                SPI_MEM_OP_DATA_OUT(ndata, buf, 0))
>>> +
>>>  static int
>>>  mx25l25635_post_bfpt_fixups(struct spi_nor *nor,
>>>                           const struct sfdp_parameter_header *bfpt_header,
>>> @@ -105,9 +121,90 @@ static const struct flash_info macronix_nor_parts[] = {
>>>               FIXUP_FLAGS(SPI_NOR_4B_OPCODES) },
>>>  };
>>>
>>> +static int macronix_nor_octal_dtr_en(struct spi_nor *nor)
>>> +{
>>> +     struct spi_mem_op op;
>>> +     u8 *buf = nor->bouncebuf, i;
>>> +     int ret;
>>> +
>>> +     /* Use 20 dummy cycles for memory array reads. */
>>> +     buf[0] = SPINOR_REG_MXIC_DC_20;
>>> +     op = (struct spi_mem_op)
>>> +             MXIC_NOR_WR_ANY_REG_OP(SPINOR_REG_MXIC_ADDR_BYTES,
>>> +                                    SPINOR_REG_MXIC_CR2_DC, 1, buf);
>>> +
>>> +     ret = spi_nor_write_any_volatile_reg(nor, &op, nor->reg_proto);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     /* Set the octal and DTR enable bits. */
>>> +     buf[0] = SPINOR_REG_MXIC_OPI_DTR_EN;
>>> +     op = (struct spi_mem_op)
>>> +             MXIC_NOR_WR_ANY_REG_OP(SPINOR_REG_MXIC_ADDR_BYTES,
>>> +                                    SPINOR_REG_MXIC_CR2_MODE, 1, buf);
>>> +     ret = spi_nor_write_any_volatile_reg(nor, &op, nor->reg_proto);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     /* Read flash ID to make sure the switch was successful. */
>>> +     ret = spi_nor_read_id(nor, 4, 4, buf, SNOR_PROTO_8_8_8_DTR);
>>> +     if (ret) {
>>> +             dev_dbg(nor->dev, "error %d reading JEDEC ID after enabling 8D-8D-8D mode\n", ret);
>>> +             return ret;
>>> +     }
>>> +
>>> +     /* Macronix SPI-NOR flash 8D-8D-8D read ID would get 6 bytes data A-A-B-B-C-C */
>>> +     for (i = 0; i < nor->info->id_len; i++)
>>> +             if (buf[i * 2] != nor->info->id[i])
>>> +                     return -EINVAL;
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static int macronix_nor_octal_dtr_dis(struct spi_nor *nor)
>>> +{
>>> +     struct spi_mem_op op;
>>> +     u8 *buf = nor->bouncebuf;
>>> +     int ret;
>>> +
>>> +     /*
>>> +      * The register is 1-byte wide, but 1-byte transactions are not
>>> +      * allowed in 8D-8D-8D mode. Since there is no register at the
>>> +      * next location, just initialize the value to 0 and let the
>>> +      * transaction go on.
>>> +      */
>>> +     buf[0] = SPINOR_REG_MXIC_SPI_EN;
>>> +     buf[1] = 0x0;
>>> +     op = (struct spi_mem_op)
>>> +             MXIC_NOR_WR_ANY_REG_OP(SPINOR_REG_MXIC_ADDR_BYTES,
>>> +                                    SPINOR_REG_MXIC_CR2_MODE, 2, buf);
>>> +     ret = spi_nor_write_any_volatile_reg(nor, &op, SNOR_PROTO_8_8_8_DTR);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     /* Read flash ID to make sure the switch was successful. */
>>> +     ret = spi_nor_read_id(nor, 0, 0, buf, SNOR_PROTO_1_1_1);
>>> +     if (ret) {
>>> +             dev_dbg(nor->dev, "error %d reading JEDEC ID after disabling 8D-8D-8D mode\n", ret);
>>> +             return ret;
>>> +     }
>>> +
>>> +     if (memcmp(buf, nor->info->id, nor->info->id_len))
>>> +             return -EINVAL;
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static int macronix_nor_octal_dtr_enable(struct spi_nor *nor, bool enable)
>>> +{
>>> +     return enable ? macronix_nor_octal_dtr_en(nor) :
>>> +                     macronix_nor_octal_dtr_dis(nor);
>>> +}
>>> +
>>>  static void macronix_nor_default_init(struct spi_nor *nor)
>>>  {
>>>       nor->params->quad_enable = spi_nor_sr1_bit6_quad_enable;
>>> +     nor->params->octal_dtr_enable = macronix_nor_octal_dtr_enable;
>>
>> this must be done in the late_init hook, I'd like to get rid of the
>> default_init hook.
> nor->params->quad_enable should be move in late_init or
> nor->params->octal_dtr_enable only?

quad_enable should be parsed from sfdp, thus we have to get rid of
setting quad_enable in a dedicated hook. Remove it entirely, but in a
dedicated patch. Of course, I expect you test the change and verify
that quad still works on all the affected flashes.

octal_dtr_enable should be set in late_init as we don't retrieve the
octal dtr enable from SFDP yet.
> 
>>>  }
>>>
>>>  static void macronix_nor_late_init(struct spi_nor *nor)
> 
> Thanks
> Jaime

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/