lkml.org 
[lkml]   [2019]   [Apr]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    SubjectRe: [PATCH 2/2] mtd: spi-nor: Rework hwcaps selection for the spi-mem case
    Date
    Hi,

    On 04/09/2019 07:26 PM, Vignesh Raghavendra wrote:
    > External E-Mail
    >
    >
    > From: Boris Brezillon <boris.brezillon@bootlin.com>
    >
    > 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@bootlin.com>
    > Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com>
    > ---
    > 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
    >
    \
     
     \ /
      Last update: 2019-04-15 14:00    [W:4.319 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site