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

From: Tudor.Ambarus
Date: Mon Apr 15 2019 - 07:59:08 EST

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
>