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.pitc...@atmel.com> > --- > 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(¶ms->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(¶ms->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(¶ms->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(¶ms->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(¶ms->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 = ¶ms->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 = ¶ms->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