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

[Marek Vasut]

On 04/16/2017 11:41 PM, 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 and Page Program 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.
> 
> Finally the 'struct spi_nor_flash_parameter', through the optional
> .quad_enable() hook, tells the spi-nor framework how to set the Quad
> Enable (QE) bit of the QSPI memory to enable its Quad SPI features.
> For now the .quad_enable() hook is set with the exact same functions as
> already used before this patch: the right function was and is still
> chosen only based on the memory manufacturer. In further patches, this
> choice will be made also based on the memory part. Indeed, we already
> know that some manufacturers, like Spansion, have updated their procedure
> to set the QE bit with their latest QSPI memories. The new procedure is
> safer but not supported by the oldest memory parts.
> The SFDP table could be one solution to select the relevant procedure but
> this issue will be addressed in further dedicated patches.

This IMO could've been factored out into separate small patch, so that
this patch isn't such a blob of multiple changes.

Otherwise, minor nits below ...

> Signed-off-by: Cyrille Pitchen <cyrille.pitchen@xxxxxxxxx>
> ---
>  drivers/mtd/devices/m25p80.c          |  18 +-
>  drivers/mtd/spi-nor/aspeed-smc.c      |  23 +-
>  drivers/mtd/spi-nor/atmel-quadspi.c   |  83 ++++--
>  drivers/mtd/spi-nor/cadence-quadspi.c |  18 +-
>  drivers/mtd/spi-nor/fsl-quadspi.c     |   6 +-
>  drivers/mtd/spi-nor/hisi-sfc.c        |  31 ++-
>  drivers/mtd/spi-nor/intel-spi.c       |   7 +-
>  drivers/mtd/spi-nor/mtk-quadspi.c     |  15 +-
>  drivers/mtd/spi-nor/nxp-spifi.c       |  22 +-
>  drivers/mtd/spi-nor/spi-nor.c         | 472 +++++++++++++++++++++++++++-------
>  drivers/mtd/spi-nor/stm32-quadspi.c   |  27 +-
>  include/linux/mtd/spi-nor.h           | 158 +++++++++++-
>  12 files changed, 686 insertions(+), 194 deletions(-)
> 
> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> index c4df3b1bded0..34f33f6b88b4 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;

Won't this become something like:

rx_bits = (nor->read_proto >> SNOR_PROTO_INST_SHIFT) & SNOR_PROTO_INST_MASK;

return rx_bits >= 1 ? rx_bits : 0;

And then you don't have to ever touch it once you add new SNOR_PROTO_1_1_x ?

Why do we return 0 by default though ? SPI_NBITS_SINGLE = 1 ... hmmm.

>  	default:
>  		return 0;
> @@ -196,7 +196,11 @@ 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_READ_FAST |
> +			SNOR_HWCAPS_PP,
> +	};
>  	char *flash_name;
>  	int ret;


[...]

> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index 36684ca7aa24..3bb2adff306b 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -150,24 +150,6 @@ static int read_cr(struct spi_nor *nor)
>  }
>  
>  /*
> - * Dummy Cycle calculation for different type of read.
> - * It can be used to support more commands with
> - * different dummy cycle requirements.
> - */
> -static inline int spi_nor_read_dummy_cycles(struct spi_nor *nor)
> -{
> -	switch (nor->flash_read) {
> -	case SPI_NOR_FAST:
> -	case SPI_NOR_DUAL:
> -	case SPI_NOR_QUAD:
> -		return 8;
> -	case SPI_NOR_NORMAL:
> -		return 0;
> -	}
> -	return 0;
> -}
> -
> -/*
>   * Write status register 1 byte
>   * Returns negative if error occurred.
>   */
> @@ -221,6 +203,10 @@ static inline u8 spi_nor_convert_3to4_read(u8 opcode)
>  		{ SPINOR_OP_READ_1_2_2,	SPINOR_OP_READ_1_2_2_4B },
>  		{ SPINOR_OP_READ_1_1_4,	SPINOR_OP_READ_1_1_4_4B },
>  		{ SPINOR_OP_READ_1_4_4,	SPINOR_OP_READ_1_4_4_4B },
> +
> +		{ SPINOR_OP_READ_1_1_1_DTR,	SPINOR_OP_READ_1_1_1_DTR_4B },
> +		{ SPINOR_OP_READ_1_2_2_DTR,	SPINOR_OP_READ_1_2_2_DTR_4B },
> +		{ SPINOR_OP_READ_1_4_4_DTR,	SPINOR_OP_READ_1_4_4_DTR_4B },

If you moved this into separate patch, that patch would be a nice
example for future generations on how to add new protocols ...

>  	};
>  
>  	return spi_nor_convert_opcode(opcode, spi_nor_3to4_read,

[...]

> +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);
> +};
> +
> +

One newline too many here :)

> +static 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;
> +}

[...]

-- 
Best regards,
Marek Vasut