[PATCH v10 05/17] mtd: spi-nor: add support for DTR protocol
Tudor.Ambarus at microchip.com
Tudor.Ambarus at microchip.com
Tue Jul 7 13:37:26 EDT 2020
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 at 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(¶ms->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(¶ms->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(¶ms->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
More information about the linux-mtd
mailing list