Re: [PATCH 2/2] mtd: spi-nor: Rework hwcaps selection for the spi-mem case

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On 04/09/2019 07:26 PM, Vignesh Raghavendra wrote:
> External E-Mail
> 
> 
> From: Boris Brezillon <boris.brezillon@xxxxxxxxxxx>
> 
> The spi-mem layer provides a spi_mem_supports_op() function to check
> whether a specific operation is supported by the controller or not.
> This is much more accurate than the hwcaps selection logic based on
> SPI_{RX,TX}_ flags.
> 
> Rework the hwcaps selection logic to use spi_mem_supports_op() when
> nor->spimem != NULL.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxx>
> Signed-off-by: Vignesh Raghavendra <vigneshr@xxxxxx>
> ---
> Chagnes wrt RFC:
> Fix checkpatch issues
> Rebase onto latest
> 
>  drivers/mtd/spi-nor/spi-nor.c | 164 ++++++++++++++++++++++++++--------
>  include/linux/mtd/spi-nor.h   |  14 +++
>  2 files changed, 142 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index 03c8c346c9ae..d48ae3c9661d 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -2852,6 +2852,110 @@ static int spi_nor_read_sfdp(struct spi_nor *nor, u32 addr,
>  	return ret;
>  }
>  
> +static int spi_nor_spimem_check_readop(struct spi_nor *nor,
> +				       const struct spi_nor_read_command *read)
> +{
> +	struct spi_mem_op op = SPI_MEM_OP(SPI_MEM_OP_CMD(read->opcode, 1),
> +					  SPI_MEM_OP_ADDR(3, 0, 1),
> +					  SPI_MEM_OP_DUMMY(0, 1),
> +					  SPI_MEM_OP_DATA_IN(0, NULL, 1));
> +
> +	op.cmd.buswidth = spi_nor_get_protocol_inst_nbits(read->proto);
> +	op.addr.buswidth = spi_nor_get_protocol_addr_nbits(read->proto);
> +	op.data.buswidth = spi_nor_get_protocol_data_nbits(read->proto);
> +	op.dummy.buswidth = op.addr.buswidth;
> +	op.dummy.nbytes = (read->num_mode_clocks + read->num_wait_states) *
> +			  op.dummy.buswidth / 8;
> +
> +	/*
> +	 * First test with 3 address bytes. The opcode itself might already
> +	 * be a 4B addressing opcode but we don't care, because SPI controller
> +	 * implementation should not check the opcode, but just the sequence.
> +	 */
> +	if (!spi_mem_supports_op(nor->spimem, &op))
> +		return -ENOTSUPP;
> +
> +	/* Now test with 4 address bytes. */

shouldn't this be verified only when the previous check failed?

> +	op.addr.nbytes = 4;
> +	if (!spi_mem_supports_op(nor->spimem, &op))
> +		return -ENOTSUPP;
> +
> +	return 0;
> +}
> +
> +static int spi_nor_spimem_check_progop(struct spi_nor *nor,

s/progop/pp?

> +				       const struct spi_nor_pp_command *pp)
> +{
> +	struct spi_mem_op op = SPI_MEM_OP(SPI_MEM_OP_CMD(pp->opcode, 1),
> +					  SPI_MEM_OP_ADDR(3, 0, 1),
> +					  SPI_MEM_OP_NO_DUMMY,
> +					  SPI_MEM_OP_DATA_OUT(0, NULL, 1));
> +
> +	op.cmd.buswidth = spi_nor_get_protocol_inst_nbits(pp->proto);
> +	op.addr.buswidth = spi_nor_get_protocol_addr_nbits(pp->proto);
> +	op.data.buswidth = spi_nor_get_protocol_data_nbits(pp->proto);
> +
maybe we can introduce a spi_nor_spimem_check_op() for the block from below to
reduce code duplication

> +	/*
> +	 * First test with 3 address bytes. The opcode itself might already
> +	 * be a 4B addressing opcode but we don't care, because SPI controller
> +	 * implementation should not check the opcode, but just the sequence.
> +	 */
> +	if (!spi_mem_supports_op(nor->spimem, &op))
> +		return -ENOTSUPP;
> +
> +	/* Now test with 4 address bytes. */
> +	op.addr.nbytes = 4;
> +	if (!spi_mem_supports_op(nor->spimem, &op))
> +		return -ENOTSUPP;
> +
> +	return 0;
> +}
> +
> +static void
> +spi_nor_spimem_adjust_hwcaps(struct spi_nor *nor,
> +			     const struct spi_nor_flash_parameter *params,
> +			     u32 *hwcaps)
> +{
> +	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;
> +
> +	/* Start with read commands. */
> +	for (cap = 0; cap < 32; cap++) {

maybe a macro for 32?

> +		int idx;
> +
> +		if (!(*hwcaps & BIT(cap)))
> +			continue;
> +
> +		idx = spi_nor_hwcaps_read2cmd(BIT(cap));
> +		if (idx < 0)
> +			continue;
> +
> +		if (spi_nor_spimem_check_readop(nor, &params->reads[idx]))
> +			*hwcaps &= ~BIT(cap);
> +	}
> +
> +	/* Now check program commands. */
> +	for (cap = 0; cap < 32; cap++) {

can't we use a single loop for both read & pp?

> +		int idx;
> +
> +		if (!(*hwcaps & BIT(cap)))
> +			continue;
> +
> +		idx = spi_nor_hwcaps_pp2cmd(BIT(cap));
> +		if (idx < 0)
> +			continue;
> +
> +		if (spi_nor_spimem_check_progop(nor,
> +						&params->page_programs[idx]))
> +			*hwcaps &= ~BIT(cap);
> +	}
> +}
> +
>  /**
>   * spi_nor_read_sfdp_dma_unsafe() - read Serial Flash Discoverable Parameters.
>   * @nor:	pointer to a 'struct spi_nor'
> @@ -4259,16 +4363,25 @@ static int spi_nor_setup(struct spi_nor *nor,
>  	 */
>  	shared_mask = hwcaps->mask & params->hwcaps.mask;
>  
> -	/* SPI n-n-n protocols 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 n-n-n protocols are not supported yet.\n");
> -		shared_mask &= ~ignored_mask;
> +	if (nor->spimem) {
> +		/*
> +		 * When called from spi_nor_probe(), all caps are set and we
> +		 * need to discard some of them based on what the SPI
> +		 * controller actually supports (using spi_mem_supports_op()).
> +		 */
> +		spi_nor_spimem_adjust_hwcaps(nor, params, &shared_mask);
> +	} else {
> +		/*
> +		 * SPI n-n-n protocols are not supported when the SPI
> +		 * controller directly implements the spi_nor interface.
> +		 * Yet another reason to switch to spi-mem.
> +		 */
> +		ignored_mask = SNOR_HWCAPS_X_X_X;
> +		if (shared_mask & ignored_mask) {
> +			dev_dbg(nor->dev,
> +				"SPI n-n-n protocols are not supported.\n");
> +			shared_mask &= ~ignored_mask;
> +		}
>  	}
>  
>  	/* Select the (Fast) Read command. */
> @@ -4591,11 +4704,11 @@ static int spi_nor_probe(struct spi_mem *spimem)
>  	struct spi_device *spi = spimem->spi;
>  	struct flash_platform_data *data;
>  	struct spi_nor *nor;
> -	struct spi_nor_hwcaps hwcaps = {
> -		.mask = SNOR_HWCAPS_READ |
> -			SNOR_HWCAPS_READ_FAST |
> -			SNOR_HWCAPS_PP,
> -	};
> +	/*
> +	 * Enable all caps by default. The core will mask them after
> +	 * checking what's really supported using spi_mem_supports_op().
> +	 */
> +	struct spi_nor_hwcaps hwcaps = { .mask = SNOR_HWCAPS_ALL };

const struct spi_nor_hwcaps hwcaps = { .mask = SNOR_HWCAPS_ALL };

Cheers,
ta

>  	char *flash_name;
>  	int ret;
>  
> @@ -4624,27 +4737,6 @@ static int spi_nor_probe(struct spi_mem *spimem)
>  
>  	spi_mem_set_drvdata(spimem, nor);
>  
> -	if (spi->mode & SPI_RX_OCTAL) {
> -		hwcaps.mask |= SNOR_HWCAPS_READ_1_1_8;
> -
> -		if (spi->mode & SPI_TX_OCTAL)
> -			hwcaps.mask |= (SNOR_HWCAPS_READ_1_8_8 |
> -					SNOR_HWCAPS_PP_1_1_8 |
> -					SNOR_HWCAPS_PP_1_8_8);
> -	} else if (spi->mode & SPI_RX_QUAD) {
> -		hwcaps.mask |= SNOR_HWCAPS_READ_1_1_4;
> -
> -		if (spi->mode & SPI_TX_QUAD)
> -			hwcaps.mask |= (SNOR_HWCAPS_READ_1_4_4 |
> -					SNOR_HWCAPS_PP_1_1_4 |
> -					SNOR_HWCAPS_PP_1_4_4);
> -	} else if (spi->mode & SPI_RX_DUAL) {
> -		hwcaps.mask |= SNOR_HWCAPS_READ_1_1_2;
> -
> -		if (spi->mode & SPI_TX_DUAL)
> -			hwcaps.mask |= SNOR_HWCAPS_READ_1_2_2;
> -	}
> -
>  	if (data && data->name)
>  		nor->mtd.name = data->name;
>  
> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index ac16745f5ef8..1d56c437f553 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -523,6 +523,20 @@ struct spi_nor_hwcaps {
>  #define SNOR_HWCAPS_PP_1_8_8	BIT(21)
>  #define SNOR_HWCAPS_PP_8_8_8	BIT(22)
>  
> +#define SNOR_HWCAPS_X_X_X	(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)
> +
> +#define SNOR_HWCAPS_DTR		(SNOR_HWCAPS_READ_1_1_1_DTR |	\
> +				 SNOR_HWCAPS_READ_1_2_2_DTR |	\
> +				 SNOR_HWCAPS_READ_1_4_4_DTR |	\
> +				 SNOR_HWCAPS_READ_1_8_8_DTR)
> +
> +#define SNOR_HWCAPS_ALL		(SNOR_HWCAPS_READ_MASK |	\
> +				 SNOR_HWCAPS_PP_MASK)
> +
>  /**
>   * spi_nor_scan() - scan the SPI NOR
>   * @nor:	the spi_nor structure
> 
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/



[Index of Archives]     [LARTC]     [Bugtraq]     [Yosemite Forum]     [Photo]

  Powered by Linux