Linux-mtd Archive on lore.kernel.org
 help / color / Atom feed
From: <Tudor.Ambarus@microchip.com>
To: <p.yadav@ti.com>, <miquel.raynal@bootlin.com>, <richard@nod.at>,
	<vigneshr@ti.com>, <broonie@kernel.org>,
	<Nicolas.Ferre@microchip.com>, <alexandre.belloni@bootlin.com>,
	<Ludovic.Desroches@microchip.com>, <matthias.bgg@gmail.com>,
	<michal.simek@xilinx.com>, <linux-mtd@lists.infradead.org>,
	<linux-kernel@vger.kernel.org>, <linux-spi@vger.kernel.org>,
	<linux-arm-kernel@lists.infradead.org>,
	<linux-mediatek@lists.infradead.org>
Cc: boris.brezillon@collabora.com, nsekhar@ti.com
Subject: Re: [PATCH v10 05/17] mtd: spi-nor: add support for DTR protocol
Date: Tue, 7 Jul 2020 17:37:26 +0000
Message-ID: <fbb3d7e7-75ed-dbf6-a975-2ae871bc9fbf@microchip.com> (raw)
In-Reply-To: <20200623183030.26591-6-p.yadav@ti.com>

Hi, Pratyush,

On 6/23/20 9:30 PM, Pratyush Yadav wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Double Transfer Rate (DTR) is SPI protocol in which data is transferred
> on each clock edge as opposed to on each clock cycle. Make
> framework-level changes to allow supporting flashes in DTR mode.
> 
> Right now, mixed DTR modes are not supported. So, for example a mode
> like 4S-4D-4D will not work. All phases need to be either DTR or STR.
> 
> Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
> ---
>  drivers/mtd/spi-nor/core.c  | 305 ++++++++++++++++++++++++++++-------- 
>  drivers/mtd/spi-nor/core.h  |   6 +
>  drivers/mtd/spi-nor/sfdp.c  |   9 +-
>  include/linux/mtd/spi-nor.h |  51 ++++--
>  4 files changed, 295 insertions(+), 76 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 0369d98b2d12..22a3832b83a6 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -40,6 +40,76 @@
> 
>  #define SPI_NOR_MAX_ADDR_WIDTH 4
> 
> +/**
> + * spi_nor_get_cmd_ext() - Get the command opcode extension based on the
> + *                        extension type.
> + * @nor:               pointer to a 'struct spi_nor'
> + * @op:                        pointer to the 'struct spi_mem_op' whose properties
> + *                     need to be initialized.
> + *
> + * Right now, only "repeat" and "invert" are supported.
> + *
> + * Return: The opcode extension.
> + */
> +static u8 spi_nor_get_cmd_ext(const struct spi_nor *nor,
> +                             const struct spi_mem_op *op)
> +{
> +       switch (nor->cmd_ext_type) {
> +       case SPI_NOR_EXT_INVERT:
> +               return ~op->cmd.opcode;
> +
> +       case SPI_NOR_EXT_REPEAT:
> +               return op->cmd.opcode;
> +
> +       default:
> +               dev_err(nor->dev, "Unknown command extension type\n");
> +               return 0;
> +       }
> +}
> +
> +/**
> + * spi_nor_spimem_setup_op() - Set up common properties of a spi-mem op.
> + * @nor:               pointer to a 'struct spi_nor'
> + * @op:                        pointer to the 'struct spi_mem_op' whose properties
> + *                     need to be initialized.
> + * @proto:             the protocol from which the properties need to be set.
> + */
> +void spi_nor_spimem_setup_op(const struct spi_nor *nor,
> +                            struct spi_mem_op *op,
> +                            const enum spi_nor_protocol proto)

There's not much to set for the REG operations.

> +{
> +       u8 ext;
> +
> +       op->cmd.buswidth = spi_nor_get_protocol_inst_nbits(proto);
> +
> +       if (op->addr.nbytes)
> +               op->addr.buswidth = spi_nor_get_protocol_addr_nbits(proto);
> +
> +       if (op->dummy.nbytes)
> +               op->dummy.buswidth = spi_nor_get_protocol_addr_nbits(proto);
> +
> +       if (op->data.nbytes)
> +               op->data.buswidth = spi_nor_get_protocol_data_nbits(proto);

How about getting rid of the above and

> +
> +       if (spi_nor_protocol_is_dtr(proto)) {

introduce a spi_nor_spimem_setup_dtr_op() just for the body of this if?

> +               /*
> +                * spi-mem supports mixed DTR modes, but right now we can only
> +                * have all phases either DTR or STR. IOW, spi-mem can have
nit: SPIMEM
> +                * something like 4S-4D-4D, but spi-nor can't. So, set all 4
nit: SPI NOR
> +                * phases to either DTR or STR.
> +                */
> +               op->cmd.dtr = op->addr.dtr = op->dummy.dtr
> +                              = op->data.dtr = true;
> +
> +               /* 2 bytes per clock cycle in DTR mode. */
> +               op->dummy.nbytes *= 2;
> +
> +               ext = spi_nor_get_cmd_ext(nor, op);
> +               op->cmd.opcode = (op->cmd.opcode << 8) | ext;
> +               op->cmd.nbytes = 2;
> +       }
> +}
> +
>  /**
>   * spi_nor_spimem_bounce() - check if a bounce buffer is needed for the data
>   *                           transfer
> @@ -104,14 +174,12 @@ static ssize_t spi_nor_spimem_read_data(struct spi_nor *nor, loff_t from,
>         ssize_t nbytes;
>         int error;
> 
> -       /* get transfer protocols. */
> -       op.cmd.buswidth = spi_nor_get_protocol_inst_nbits(nor->read_proto);
> -       op.addr.buswidth = spi_nor_get_protocol_addr_nbits(nor->read_proto);
> -       op.dummy.buswidth = op.addr.buswidth;
> -       op.data.buswidth = spi_nor_get_protocol_data_nbits(nor->read_proto);
> +       spi_nor_spimem_setup_op(nor, &op, nor->read_proto);

Here we would keep the code as it were.
> 
>         /* convert the dummy cycles to the number of bytes */
>         op.dummy.nbytes = (nor->read_dummy * op.dummy.buswidth) / 8;
> +       if (spi_nor_protocol_is_dtr(nor->read_proto))
> +               op.dummy.nbytes *= 2;

And replace these 2 lines with:
	if (spi_nor_protocol_is_dtr(nor->read_proto))
		spi_nor_spimem_setup_dtr_op(nor, &op, nor->read_proto)
> 
>         usebouncebuf = spi_nor_spimem_bounce(nor, &op);
> 
> @@ -169,13 +237,11 @@ static ssize_t spi_nor_spimem_write_data(struct spi_nor *nor, loff_t to,
>         ssize_t nbytes;
>         int error;
> 
> -       op.cmd.buswidth = spi_nor_get_protocol_inst_nbits(nor->write_proto);
> -       op.addr.buswidth = spi_nor_get_protocol_addr_nbits(nor->write_proto);
> -       op.data.buswidth = spi_nor_get_protocol_data_nbits(nor->write_proto);
> -
>         if (nor->program_opcode == SPINOR_OP_AAI_WP && nor->sst_write_second)
>                 op.addr.nbytes = 0;
> 
> +       spi_nor_spimem_setup_op(nor, &op, nor->write_proto);
> +
>         if (spi_nor_spimem_bounce(nor, &op))
>                 memcpy(nor->bouncebuf, buf, op.data.nbytes);
> 
> @@ -227,10 +293,16 @@ int spi_nor_write_enable(struct spi_nor *nor)
>                                    SPI_MEM_OP_NO_DUMMY,
>                                    SPI_MEM_OP_NO_DATA);
> 
> +               spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);

For the reg operation we can get rid of the extra checks that were in
spi_nor_spimem_setup_op and simply do:

		if (spi_nor_protocol_is_dtr(proto))
			spi_nor_spimem_setup_dtr_op()

> +
>                 ret = spi_mem_exec_op(nor->spimem, &op);
>         } else {
> -               ret = nor->controller_ops->write_reg(nor, SPINOR_OP_WREN,
> -                                                    NULL, 0);
> +               if (spi_nor_protocol_is_dtr(nor->reg_proto))
> +                       ret = -ENOTSUPP;
> +               else
> +                       ret = nor->controller_ops->write_reg(nor,
> +                                                            SPINOR_OP_WREN,
> +                                                            NULL, 0);

Would you introduce helpers for the controller ops, like Boris
did in the following patch?
https://patchwork.ozlabs.org/project/linux-mtd/patch/20181012084825.23697-10-boris.brezillon@bootlin.com/

How about spi_nor_controller_ops_read_reg()
and spi_nor_controller_ops_write_reg() instead?

cut

> @@ -1144,7 +1291,11 @@ static int spi_nor_erase_sector(struct spi_nor *nor, u32 addr)
>                                    SPI_MEM_OP_NO_DUMMY,
>                                    SPI_MEM_OP_NO_DATA);
> 
> +               spi_nor_spimem_setup_op(nor, &op, nor->write_proto);
> +
>                 return spi_mem_exec_op(nor->spimem, &op);
> +       } else if (spi_nor_protocol_is_dtr(nor->write_proto)) {
> +               return -ENOTSUPP;
>         } else if (nor->controller_ops->erase) {
>                 return nor->controller_ops->erase(nor, addr);
>         }

here you would need a helper: spi_nor_controller_ops_erase()

cut

> @@ -2368,12 +2517,16 @@ spi_nor_spimem_adjust_hwcaps(struct spi_nor *nor, u32 *hwcaps)
>         struct spi_nor_flash_parameter *params = nor->params;
>         unsigned int cap;
> 
> -       /* DTR modes are not supported yet, mask them all. */
> -       *hwcaps &= ~SNOR_HWCAPS_DTR;
> -
>         /* X-X-X modes are not supported yet, mask them all. */
>         *hwcaps &= ~SNOR_HWCAPS_X_X_X;
> 
> +       /*
> +        * If the reset line is broken, we do not want to enter a stateful
> +        * mode.
> +        */
> +       if (nor->flags & SNOR_F_BROKEN_RESET)
> +               *hwcaps &= ~(SNOR_HWCAPS_X_X_X | SNOR_HWCAPS_X_X_X_DTR);

A dedicated reset line is not enough for flashes that keep their state
in non-volatile bits. Since we can't protect from unexpected crashes in
the non volatile state case, we should enter these modes only with an
explicit request, i.e. an optional DT property: "update-nonvolatile-state",
or something similar.

For the volatile state case, we can parse the SFDP SCCR map, save if we
can enter stateful modes in a volatile way, and if yes allow the entering.

Do the flashes that you played with define the SFDP SCCR map?

> +
>         for (cap = 0; cap < sizeof(*hwcaps) * BITS_PER_BYTE; cap++) {
>                 int rdidx, ppidx;
> 
> @@ -2628,7 +2781,7 @@ static int spi_nor_default_setup(struct spi_nor *nor,
>                  * controller directly implements the spi_nor interface.
>                  * Yet another reason to switch to spi-mem.
>                  */
> -               ignored_mask = SNOR_HWCAPS_X_X_X;
> +               ignored_mask = SNOR_HWCAPS_X_X_X | SNOR_HWCAPS_X_X_X_DTR;
>                 if (shared_mask & ignored_mask) {
>                         dev_dbg(nor->dev,
>                                 "SPI n-n-n protocols are not supported.\n");
> @@ -2774,11 +2927,25 @@ static void spi_nor_info_init_params(struct spi_nor *nor)
>                                           SNOR_PROTO_1_1_8);
>         }
> 
> +       if (info->flags & SPI_NOR_OCTAL_DTR_READ) {

Why do we need this flag? Can't we determine if the flash supports
octal DTR by parsing SFDP?

> +               params->hwcaps.mask |= SNOR_HWCAPS_READ_8_8_8_DTR;
> +               spi_nor_set_read_settings(&params->reads[SNOR_CMD_READ_8_8_8_DTR],
> +                                         0, 20, SPINOR_OP_READ_FAST,
> +                                         SNOR_PROTO_8_8_8_DTR);
> +       }
> +
>         /* Page Program settings. */
>         params->hwcaps.mask |= SNOR_HWCAPS_PP;
>         spi_nor_set_pp_settings(&params->page_programs[SNOR_CMD_PP],
>                                 SPINOR_OP_PP, SNOR_PROTO_1_1_1);
> 
> +       /*
> +        * Since xSPI Page Program opcode is backward compatible with
> +        * Legacy SPI, use Legacy SPI opcode there as well.
> +        */
> +       spi_nor_set_pp_settings(&params->page_programs[SNOR_CMD_PP_8_8_8_DTR],
> +                               SPINOR_OP_PP, SNOR_PROTO_8_8_8_DTR);
> +

This looks fishy. You haven't updated the hwcaps.mask, these pp settings never
get selected?

>         /*
>          * Sector Erase settings. Sort Erase Types in ascending order, with the
>          * smallest erase size starting at BIT(0).
> @@ -2886,7 +3053,8 @@ static int spi_nor_init_params(struct spi_nor *nor)
> 
>         spi_nor_manufacturer_init_params(nor);
> 
> -       if ((nor->info->flags & (SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)) &&
> +       if ((nor->info->flags & (SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
> +                                SPI_NOR_OCTAL_READ | SPI_NOR_OCTAL_DTR_READ)) &&
>             !(nor->info->flags & SPI_NOR_SKIP_SFDP))
>                 spi_nor_sfdp_init_params(nor);
> 
> @@ -2948,7 +3116,9 @@ static int spi_nor_init(struct spi_nor *nor)
>                 return err;
>         }
> 
> -       if (nor->addr_width == 4 && !(nor->flags & SNOR_F_4B_OPCODES)) {
> +       if (nor->addr_width == 4 &&
> +           !(nor->info->flags & SPI_NOR_OCTAL_DTR_READ) &&

Why is the Octal DTR read exempted?

> +           !(nor->flags & SNOR_F_4B_OPCODES)) {
>                 /*
>                  * If the RESET# pin isn't hooked up properly, or the system
>                  * otherwise doesn't perform a reset command in the boot
> @@ -3007,6 +3177,9 @@ static int spi_nor_set_addr_width(struct spi_nor *nor)
>  {
>         if (nor->addr_width) {
>                 /* already configured from SFDP */
> +       } else if (spi_nor_protocol_is_dtr(nor->read_proto)) {
> +                /* Always use 4-byte addresses in DTR mode. */
> +               nor->addr_width = 4;

Why? DTR with 3 byte addr width should be possible too.

>         } else if (nor->info->addr_width) {
>                 nor->addr_width = nor->info->addr_width;
>         } else if (nor->mtd.size > 0x1000000) {

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

  reply index

Thread overview: 39+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-23 18:30 [PATCH v10 00/17] mtd: spi-nor: add xSPI Octal DTR support Pratyush Yadav
2020-06-23 18:30 ` [PATCH v10 01/17] spi: spi-mem: allow specifying whether an op is DTR or not Pratyush Yadav
2020-07-08 16:22   ` Tudor.Ambarus
2020-07-13  3:55   ` Tudor.Ambarus
2020-06-23 18:30 ` [PATCH v10 02/17] spi: spi-mem: allow specifying a command's extension Pratyush Yadav
2020-07-13  6:15   ` Tudor.Ambarus
2020-06-23 18:30 ` [PATCH v10 03/17] spi: atmel-quadspi: reject DTR ops Pratyush Yadav
2020-07-13  6:19   ` Tudor.Ambarus
2020-06-23 18:30 ` [PATCH v10 04/17] spi: spi-mtk-nor: " Pratyush Yadav
2020-07-13  6:24   ` Tudor.Ambarus
2020-06-23 18:30 ` [PATCH v10 05/17] mtd: spi-nor: add support for DTR protocol Pratyush Yadav
2020-07-07 17:37   ` Tudor.Ambarus [this message]
2020-07-21 11:29     ` Pratyush Yadav
2020-06-23 18:30 ` [PATCH v10 06/17] mtd: spi-nor: sfdp: get command opcode extension type from BFPT Pratyush Yadav
2020-07-07 17:53   ` Tudor.Ambarus
2020-06-23 18:30 ` [PATCH v10 07/17] mtd: spi-nor: sfdp: parse xSPI Profile 1.0 table Pratyush Yadav
2020-07-08 16:01   ` Tudor.Ambarus
2020-07-20 16:38     ` Pratyush Yadav
2020-06-23 18:30 ` [PATCH v10 08/17] mtd: spi-nor: core: use dummy cycle and address width info from SFDP Pratyush Yadav
2020-07-08 16:03   ` Tudor.Ambarus
2020-07-20 16:24     ` Pratyush Yadav
2020-06-23 18:30 ` [PATCH v10 09/17] mtd: spi-nor: core: do 2 byte reads for SR and FSR in DTR mode Pratyush Yadav
2020-06-23 18:30 ` [PATCH v10 10/17] mtd: spi-nor: core: enable octal DTR mode when possible Pratyush Yadav
2020-06-23 18:30 ` [PATCH v10 11/17] mtd: spi-nor: sfdp: do not make invalid quad enable fatal Pratyush Yadav
2020-07-13  9:33   ` Tudor.Ambarus
2020-06-23 18:30 ` [PATCH v10 12/17] mtd: spi-nor: sfdp: detect Soft Reset sequence support from BFPT Pratyush Yadav
2020-07-08 16:08   ` Tudor.Ambarus
2020-07-20 16:21     ` Pratyush Yadav
2020-06-23 18:30 ` [PATCH v10 13/17] mtd: spi-nor: core: perform a Soft Reset on shutdown Pratyush Yadav
2020-07-08 16:10   ` Tudor.Ambarus
2020-07-20 16:11     ` Pratyush Yadav
2020-06-23 18:30 ` [PATCH v10 14/17] mtd: spi-nor: core: disable Octal DTR mode on suspend Pratyush Yadav
2020-06-23 18:30 ` [PATCH v10 15/17] mtd: spi-nor: core: expose spi_nor_default_setup() in core.h Pratyush Yadav
2020-06-23 18:30 ` [PATCH v10 16/17] mtd: spi-nor: spansion: add support for Cypress Semper flash Pratyush Yadav
2020-06-23 18:30 ` [PATCH v10 17/17] mtd: spi-nor: micron-st: allow using MT35XU512ABA in Octal DTR mode Pratyush Yadav
2020-07-13  6:34 ` [PATCH v10 00/17] mtd: spi-nor: add xSPI Octal DTR support Tudor.Ambarus
2020-07-14 19:19   ` Mark Brown
2020-07-15  3:40     ` Tudor.Ambarus
2020-07-14 16:40 ` 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=fbb3d7e7-75ed-dbf6-a975-2ae871bc9fbf@microchip.com \
    --to=tudor.ambarus@microchip.com \
    --cc=Ludovic.Desroches@microchip.com \
    --cc=Nicolas.Ferre@microchip.com \
    --cc=alexandre.belloni@bootlin.com \
    --cc=boris.brezillon@collabora.com \
    --cc=broonie@kernel.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mediatek@lists.infradead.org \
    --cc=linux-mtd@lists.infradead.org \
    --cc=linux-spi@vger.kernel.org \
    --cc=matthias.bgg@gmail.com \
    --cc=michal.simek@xilinx.com \
    --cc=miquel.raynal@bootlin.com \
    --cc=nsekhar@ti.com \
    --cc=p.yadav@ti.com \
    --cc=richard@nod.at \
    --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

Linux-mtd Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-mtd/0 linux-mtd/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-mtd linux-mtd/ https://lore.kernel.org/linux-mtd \
		linux-mtd@lists.infradead.org
	public-inbox-index linux-mtd

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-mtd


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git