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

[Marek Vasut <marek.vasut@gmail.com>]

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@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(&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