Re: [PATCH v5 1/6] mtd: spi-nor: introduce more SPI protocols and the Dual Transfer Mode

From: Marek Vasut
Date: Thu Apr 06 2017 - 20:18:51 EST

On 03/23/2017 12:33 AM, Cyrille Pitchen wrote:
> This patch changes the prototype of spi_nor_scan(): its 3rd parameter
> is replaced by a 'struct spi_nor_hwcaps' pointer, which tells the spi-nor
> framework about the actual hardware capabilities supported by the SPI
> controller and its driver.
>
> Besides, this patch also introduces a new 'struct spi_nor_flash_parameter'
> telling the spi-nor framework about the hardware capabilities supported by
> the SPI flash memory and the associated settings required to use those
> hardware caps.
>
> Currently the 'struct spi_nor_flash_parameter' is filled with legacy
> values but a later patch will allow to fill it dynamically by reading the
> JESD216 Serial Flash Discoverable Parameter (SFDP) tables from the SPI
> memory.
>
> With both structures, the spi-nor framework can now compute the best
> match between hardware caps supported by both the (Q)SPI memory and
> controller hence selecting the relevant SPI protocols and op codes for
> (Fast) Read, Page Program and Sector Erase operations.
>
> The 'struct spi_nor_flash_parameter' also provides the spi-nor framework
> with the number of dummy cycles to be used with each Fast Read commands
> and the erase block size associated to the erase block op codes.
>
> Finally the 'struct spi_nor_flash_parameter', through the optional
> .enable_quad_io() hook, tells the spi-nor framework how to set the Quad
> Enable (QE) bit of the QSPI memory to enable its Quad SPI features.
>
> Signed-off-by: Cyrille Pitchen <cyrille.pitchen@xxxxxxxxx>
> ---
> drivers/mtd/devices/m25p80.c | 16 +-
> drivers/mtd/spi-nor/aspeed-smc.c | 23 +-
> drivers/mtd/spi-nor/atmel-quadspi.c | 80 +++---
> drivers/mtd/spi-nor/cadence-quadspi.c | 18 +-
> drivers/mtd/spi-nor/fsl-quadspi.c | 8 +-
> drivers/mtd/spi-nor/hisi-sfc.c | 31 ++-
> drivers/mtd/spi-nor/intel-spi.c | 7 +-
> drivers/mtd/spi-nor/mtk-quadspi.c | 16 +-
> drivers/mtd/spi-nor/nxp-spifi.c | 22 +-
> drivers/mtd/spi-nor/spi-nor.c | 441 +++++++++++++++++++++++++++-------
> include/linux/mtd/spi-nor.h | 158 +++++++++++-
> 11 files changed, 643 insertions(+), 177 deletions(-)
>
> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> index c4df3b1bded0..68986a26c8fe 100644
> --- a/drivers/mtd/devices/m25p80.c
> +++ b/drivers/mtd/devices/m25p80.c
> @@ -111,10 +111,10 @@ static ssize_t m25p80_write(struct spi_nor *nor, loff_t to, size_t len,
>
> static inline unsigned int m25p80_rx_nbits(struct spi_nor *nor)
> {
> - switch (nor->flash_read) {
> - case SPI_NOR_DUAL:
> + switch (nor->read_proto) {
> + case SNOR_PROTO_1_1_2:
> return 2;
> - case SPI_NOR_QUAD:
> + case SNOR_PROTO_1_1_4:
> return 4;
> default:
> return 0;
> @@ -196,7 +196,9 @@ static int m25p_probe(struct spi_device *spi)
> struct flash_platform_data *data;
> struct m25p *flash;
> struct spi_nor *nor;
> - enum read_mode mode = SPI_NOR_NORMAL;
> + struct spi_nor_hwcaps hwcaps = {
> + .mask = (SNOR_HWCAPS_READ | SNOR_HWCAPS_PP),

Drop the unneeded parenthesis.

> + };
> char *flash_name;
> int ret;
>
> @@ -222,9 +224,9 @@ static int m25p_probe(struct spi_device *spi)
> flash->spi = spi;
>
> if (spi->mode & SPI_RX_QUAD)
> - mode = SPI_NOR_QUAD;
> + hwcaps.mask |= SNOR_HWCAPS_READ_1_1_4;
> else if (spi->mode & SPI_RX_DUAL)
> - mode = SPI_NOR_DUAL;
> + hwcaps.mask |= SNOR_HWCAPS_READ_1_1_2;
>
> if (data && data->name)
> nor->mtd.name = data->name;
> @@ -241,7 +243,7 @@ static int m25p_probe(struct spi_device *spi)
> else
> flash_name = spi->modalias;
>
> - ret = spi_nor_scan(nor, flash_name, mode);
> + ret = spi_nor_scan(nor, flash_name, &hwcaps);
> if (ret)
> return ret;
>
> diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c
> index 56051d30f000..723026d9cf0c 100644
> --- a/drivers/mtd/spi-nor/aspeed-smc.c
> +++ b/drivers/mtd/spi-nor/aspeed-smc.c
> @@ -585,14 +585,12 @@ static int aspeed_smc_chip_setup_finish(struct aspeed_smc_chip *chip)
> * TODO: Adjust clocks if fast read is supported and interpret
> * SPI-NOR flags to adjust controller settings.
> */
> - switch (chip->nor.flash_read) {
> - case SPI_NOR_NORMAL:
> - cmd = CONTROL_COMMAND_MODE_NORMAL;
> - break;
> - case SPI_NOR_FAST:
> - cmd = CONTROL_COMMAND_MODE_FREAD;
> - break;
> - default:
> + if (chip->nor.read_proto == SNOR_PROTO_1_1_1) {
> + if (chip->nor.read_dummy == 0)
> + cmd = CONTROL_COMMAND_MODE_NORMAL;
> + else
> + cmd = CONTROL_COMMAND_MODE_FREAD;
> + } else {
> dev_err(chip->nor.dev, "unsupported SPI read mode\n");
> return -EINVAL;
> }
> @@ -608,6 +606,11 @@ static int aspeed_smc_chip_setup_finish(struct aspeed_smc_chip *chip)
> static int aspeed_smc_setup_flash(struct aspeed_smc_controller *controller,
> struct device_node *np, struct resource *r)
> {
> + struct spi_nor_hwcaps hwcaps = {
> + .mask = (SNOR_HWCAPS_READ |
> + SNOR_HWCAPS_READ_FAST |
> + SNOR_HWCAPS_PP),

Drop the extra parenthesis ... shouldn't the structure be const ?

> + };
> const struct aspeed_smc_info *info = controller->info;
> struct device *dev = controller->dev;
> struct device_node *child;
> @@ -671,11 +674,11 @@ static int aspeed_smc_setup_flash(struct aspeed_smc_controller *controller,
> break;
>
> /*
> - * TODO: Add support for SPI_NOR_QUAD and SPI_NOR_DUAL
> + * TODO: Add support for Dual and Quad SPI protocols
> * attach when board support is present as determined
> * by of property.
> */
> - ret = spi_nor_scan(nor, NULL, SPI_NOR_NORMAL);
> + ret = spi_nor_scan(nor, NULL, &hwcaps);
> if (ret)
> break;


[...]

> +struct spi_nor_flash_parameter {
> + u64 size;
> + u32 page_size;
> +
> + struct spi_nor_hwcaps hwcaps;
> + struct spi_nor_read_command reads[SNOR_CMD_READ_MAX];
> + struct spi_nor_pp_command page_programs[SNOR_CMD_PP_MAX];
> +
> + int (*quad_enable)(struct spi_nor *nor);

This callback should be added by a separate patch, there's WAY too much
crap in this patch.

> +};
> +
> +
> +static inline void
> +spi_nor_set_read_settings(struct spi_nor_read_command *read,
> + u8 num_mode_clocks,
> + u8 num_wait_states,
> + u8 opcode,
> + enum spi_nor_protocol proto)
> +{
> + read->num_mode_clocks = num_mode_clocks;
> + read->num_wait_states = num_wait_states;
> + read->opcode = opcode;
> + read->proto = proto;
> +}
> +
> +static inline void

Drop the inline , the compiler will decide. Fix globally.

> +spi_nor_set_pp_settings(struct spi_nor_pp_command *pp,
> + u8 opcode,
> + enum spi_nor_protocol proto)
> +{
> + pp->opcode = opcode;
> + pp->proto = proto;
> +}
> +
> +static int spi_nor_init_params(struct spi_nor *nor,
> + const struct flash_info *info,
> + struct spi_nor_flash_parameter *params)
> +{
> + /* Set legacy flash parameters as default. */
> + memset(params, 0, sizeof(*params));
> +
> + /* Set SPI NOR sizes. */
> + params->size = info->sector_size * info->n_sectors;
> + params->page_size = info->page_size;
> +
> + /* (Fast) Read settings. */
> + params->hwcaps.mask |= SNOR_HWCAPS_READ;
> + spi_nor_set_read_settings(&params->reads[SNOR_CMD_READ],
> + 0, 0, SPINOR_OP_READ,
> + SNOR_PROTO_1_1_1);

Newline

> + if (!(info->flags & SPI_NOR_NO_FR)) {
> + params->hwcaps.mask |= SNOR_HWCAPS_READ_FAST;
> + spi_nor_set_read_settings(&params->reads[SNOR_CMD_READ_FAST],
> + 0, 8, SPINOR_OP_READ_FAST,
> + SNOR_PROTO_1_1_1);
> + }

Newline

> + if (info->flags & SPI_NOR_DUAL_READ) {
> + params->hwcaps.mask |= SNOR_HWCAPS_READ_1_1_2;
> + spi_nor_set_read_settings(&params->reads[SNOR_CMD_READ_1_1_2],
> + 0, 8, SPINOR_OP_READ_1_1_2,
> + SNOR_PROTO_1_1_2);
> + }

Newline ... this is really hard to read as it is.

> + if (info->flags & SPI_NOR_QUAD_READ) {
> + params->hwcaps.mask |= SNOR_HWCAPS_READ_1_1_4;
> + spi_nor_set_read_settings(&params->reads[SNOR_CMD_READ_1_1_4],
> + 0, 8, SPINOR_OP_READ_1_1_4,
> + SNOR_PROTO_1_1_4);
> + }
> +
> + /* 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);
> +
> + /* Select the procedure to set the Quad Enable bit. */
> + if (params->hwcaps.mask & (SNOR_HWCAPS_READ_QUAD |
> + SNOR_HWCAPS_PP_QUAD)) {
> + switch (JEDEC_MFR(info)) {
> + case SNOR_MFR_MACRONIX:
> + params->quad_enable = macronix_quad_enable;
> + break;
> +
> + case SNOR_MFR_MICRON:
> + break;
> +
> + default:

Are you sure this is a good idea ?

> + params->quad_enable = spansion_quad_enable;
> + break;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int spi_nor_hwcaps2cmd(u32 hwcaps)

const u32 hwcaps ...

> {
> + switch (hwcaps) {
> + case SNOR_HWCAPS_READ: return SNOR_CMD_READ;

You can do this as a table lookup or array indexing and it would be
checkpatch clean.

> + case SNOR_HWCAPS_READ_FAST: return SNOR_CMD_READ_FAST;
> + case SNOR_HWCAPS_READ_1_1_1_DTR: return SNOR_CMD_READ_1_1_1_DTR;
> + case SNOR_HWCAPS_READ_1_1_2: return SNOR_CMD_READ_1_1_2;
> + case SNOR_HWCAPS_READ_1_2_2: return SNOR_CMD_READ_1_2_2;
> + case SNOR_HWCAPS_READ_2_2_2: return SNOR_CMD_READ_2_2_2;
> + case SNOR_HWCAPS_READ_1_2_2_DTR: return SNOR_CMD_READ_1_2_2_DTR;
> + case SNOR_HWCAPS_READ_1_1_4: return SNOR_CMD_READ_1_1_4;
> + case SNOR_HWCAPS_READ_1_4_4: return SNOR_CMD_READ_1_4_4;
> + case SNOR_HWCAPS_READ_4_4_4: return SNOR_CMD_READ_4_4_4;
> + case SNOR_HWCAPS_READ_1_4_4_DTR: return SNOR_CMD_READ_1_4_4_DTR;
> + case SNOR_HWCAPS_READ_1_1_8: return SNOR_CMD_READ_1_1_8;
> + case SNOR_HWCAPS_READ_1_8_8: return SNOR_CMD_READ_1_8_8;
> + case SNOR_HWCAPS_READ_8_8_8: return SNOR_CMD_READ_8_8_8;
> + case SNOR_HWCAPS_READ_1_8_8_DTR: return SNOR_CMD_READ_1_8_8_DTR;
> +
> + case SNOR_HWCAPS_PP: return SNOR_CMD_PP;
> + case SNOR_HWCAPS_PP_1_1_4: return SNOR_CMD_PP_1_1_4;
> + case SNOR_HWCAPS_PP_1_4_4: return SNOR_CMD_PP_1_4_4;
> + case SNOR_HWCAPS_PP_4_4_4: return SNOR_CMD_PP_4_4_4;
> + case SNOR_HWCAPS_PP_1_1_8: return SNOR_CMD_PP_1_1_8;
> + case SNOR_HWCAPS_PP_1_8_8: return SNOR_CMD_PP_1_8_8;
> + case SNOR_HWCAPS_PP_8_8_8: return SNOR_CMD_PP_8_8_8;
> + }
> +
> + return -EINVAL;
> +}
> +
> +static int spi_nor_select_read(struct spi_nor *nor,
> + const struct spi_nor_flash_parameter *params,
> + u32 shared_hwcaps)
> +{
> + int cmd, best_match = fls(shared_hwcaps & SNOR_HWCAPS_READ_MASK) - 1;
> + const struct spi_nor_read_command *read;
> +
> + if (best_match < 0)
> + return -EINVAL;
> +
> + cmd = spi_nor_hwcaps2cmd(BIT(best_match));

How does this work? Do we assume that hwcaps2cmd is always given a value
with 1-bit set ? That's quite wasteful IMO.

> + if (cmd < 0)
> + return -EINVAL;
> +
> + read = &params->reads[cmd];
> + nor->read_opcode = read->opcode;
> + nor->read_proto = read->proto;
> +
> + /*
> + * In the spi-nor framework, we don't need to make the difference
> + * between mode clock cycles and wait state clock cycles.
> + * Indeed, the value of the mode clock cycles is used by a QSPI
> + * flash memory to know whether it should enter or leave its 0-4-4
> + * (Continuous Read / XIP) mode.

0-4-4 ?

> + * eXecution In Place is out of the scope of the mtd sub-system.
> + * Hence we choose to merge both mode and wait state clock cycles
> + * into the so called dummy clock cycles.
> + */
> + nor->read_dummy = read->num_mode_clocks + read->num_wait_states;
> + return 0;
> +}
> +
> +static int spi_nor_select_pp(struct spi_nor *nor,
> + const struct spi_nor_flash_parameter *params,
> + u32 shared_hwcaps)
> +{
> + int cmd, best_match = fls(shared_hwcaps & SNOR_HWCAPS_PP_MASK) - 1;
> + const struct spi_nor_pp_command *pp;
> +
> + if (best_match < 0)
> + return -EINVAL;
> +
> + cmd = spi_nor_hwcaps2cmd(BIT(best_match));
> + if (cmd < 0)
> + return -EINVAL;
> +
> + pp = &params->page_programs[cmd];
> + nor->program_opcode = pp->opcode;
> + nor->write_proto = pp->proto;
> + return 0;
> +}
> +
> +static int spi_nor_select_erase(struct spi_nor *nor,
> + const struct flash_info *info)
> +{
> + struct mtd_info *mtd = &nor->mtd;
> +
> +#ifdef CONFIG_MTD_SPI_NOR_USE_4K_SECTORS
> + /* prefer "small sector" erase if possible */
> + if (info->flags & SECT_4K) {
> + nor->erase_opcode = SPINOR_OP_BE_4K;
> + mtd->erasesize = 4096;
> + } else if (info->flags & SECT_4K_PMC) {
> + nor->erase_opcode = SPINOR_OP_BE_4K_PMC;
> + mtd->erasesize = 4096;
> + } else
> +#endif
> + {
> + nor->erase_opcode = SPINOR_OP_SE;
> + mtd->erasesize = info->sector_size;
> + }
> + return 0;
> +}
> +
> +static int spi_nor_setup(struct spi_nor *nor, const struct flash_info *info,
> + const struct spi_nor_flash_parameter *params,
> + const struct spi_nor_hwcaps *hwcaps)
> +{
> + u32 ignored_mask, shared_mask;
> + bool enable_quad_io;
> + int err;
> +
> + /*
> + * Keep only the hardware capabilities supported by both the SPI
> + * controller and the SPI flash memory.
> + */
> + shared_mask = hwcaps->mask & params->hwcaps.mask;
> +
> + /* SPI protocol classes N-N-N are not supported yet. */
> + ignored_mask = (SNOR_HWCAPS_READ_2_2_2 |
> + SNOR_HWCAPS_READ_4_4_4 |
> + SNOR_HWCAPS_READ_8_8_8 |
> + SNOR_HWCAPS_PP_4_4_4 |
> + SNOR_HWCAPS_PP_8_8_8);
> + if (shared_mask & ignored_mask) {
> + dev_dbg(nor->dev,
> + "SPI protocol classes N-N-N are not supported yet.\n");
> + shared_mask &= ~ignored_mask;
> + }
> +
> + /* Select the (Fast) Read command. */
> + err = spi_nor_select_read(nor, params, shared_mask);
> + if (err) {
> + dev_err(nor->dev, "invalid (fast) read\n");

What does this information tell me, as an end user ? If I see this error
message, what sort of conclusion can I derive from it ? I have
no idea ...

> + return err;
> + }
> +
> + /* Select the Page Program command. */
> + err = spi_nor_select_pp(nor, params, shared_mask);
> + if (err) {
> + dev_err(nor->dev, "invalid page program\n");
> + return err;
> + }
> +
> + /* Select the Sector Erase command. */
> + err = spi_nor_select_erase(nor, info);
> + if (err) {
> + dev_err(nor->dev, "invalid sector/block erase\n");
> + return err;
> + }
> +
> + /* Enable Quad I/O if needed. */
> + enable_quad_io = (spi_nor_get_protocol_width(nor->read_proto) == 4 ||
> + spi_nor_get_protocol_width(nor->write_proto) == 4);

What if read_proto != write_proto ? Also, this is awful code ... fix it.

> + if (enable_quad_io && params->quad_enable)
> + nor->flash_quad_enable = params->quad_enable;
> + else
> + nor->flash_quad_enable = NULL;
> +
> + return 0;
> +}
> +
> +int spi_nor_scan(struct spi_nor *nor, const char *name,
> + const struct spi_nor_hwcaps *hwcaps)
> +{
> + struct spi_nor_flash_parameter params;
> const struct flash_info *info = NULL;
> struct device *dev = nor->dev;
> struct mtd_info *mtd = &nor->mtd;
> @@ -1548,6 +1824,11 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
> if (ret)
> return ret;
>
> + /* Reset SPI protocol for all commands */
> + nor->reg_proto = SNOR_PROTO_1_1_1;
> + nor->read_proto = SNOR_PROTO_1_1_1;
> + nor->write_proto = SNOR_PROTO_1_1_1;
> +
> if (name)
> info = spi_nor_match_id(name);
> /* Try to auto-detect if chip name wasn't specified or not found */
[...]

--
Best regards,
Marek Vasut