Message-ID: <AM4PR0701MB213041BDD681BFF384B59839FE900@AM4PR0701MB2130.eurprd07.prod.outlook.com>
Date:   Tue, 20 Dec 2016 19:36:08 +0000
From:   "Krzeminski, Marcin (Nokia - PL/Wroclaw)" 
        <marcin.krzeminski@...ia.com>
To:     Cyrille Pitchen <cyrille.pitchen@...el.com>,
        "computersforpeace@...il.com" <computersforpeace@...il.com>,
        "marek.vasut@...il.com" <marek.vasut@...il.com>,
        "boris.brezillon@...e-electrons.com" 
        <boris.brezillon@...e-electrons.com>,
        "richard@....at" <richard@....at>,
        "linux-mtd@...ts.infradead.org" <linux-mtd@...ts.infradead.org>
CC:     "nicolas.ferre@...el.com" <nicolas.ferre@...el.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "mar.krzeminski@...il.com" <mar.krzeminski@...il.com>
Subject: Odp.: [PATCH v4 4/8] mtd: spi-nor: add support of SPI protocols like
 SPI 1-2-2 and SPI 1-4-4


> Hi Marcin,
>
> Le 13/12/2016 à 10:46, Krzeminski, Marcin (Nokia - PL/Wroclaw) a écrit :
> > Cyrille,
> > 
> >> -----Original Message-----
> >> From: linux-mtd [mailto:linux-mtd-bounces@...ts.infradead.org] On Behalf
> >> Of Cyrille Pitchen
> >> Sent: Monday, November 21, 2016 3:16 PM
> >> To: computersforpeace@...il.com; marek.vasut@...il.com;
> >> boris.brezillon@...e-electrons.com; richard@....at; linux-
> >> mtd@...ts.infradead.org
> >> Cc: Cyrille Pitchen <cyrille.pitchen@...el.com>; nicolas.ferre@...el.com;
> >> linux-kernel@...r.kernel.org
> >> Subject: [PATCH v4 4/8] mtd: spi-nor: add support of SPI protocols like SPI 1-
> >> 2-2 and SPI 1-4-4
> >>
> >> This patch changes the prototype of spi_nor_scan(): its 3rd parameter is
> >> replaced by a const struct spi_nor_modes pointer, which tells the spi-nor
> >> framework about which SPI protocols are supported by the SPI controller.
> >>
> >> Besides, this patch also introduces a new spi_nor_basic_flash_parameter
> >> structure telling the spi-nor framework about the SPI protocols supported by
> >> the SPI memory and the needed op codes to use these SPI protocols.
> >>
> >> Currently the struct spi_nor_basic_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 SPI protocols supported by both the (Q)SPI memory and
> >> controller hence selecting the relevant op codes for (Fast) Read, Page
> >> Program and Sector Erase operations.
> >>
> >> The spi_nor_basic_flash_parameter structure 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 spi_nor_basic_flash_parameter structure, 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@...el.com>
> >> ---
> >>  drivers/mtd/devices/m25p80.c          |  17 ++-
> >>  drivers/mtd/spi-nor/atmel-quadspi.c   |  83 ++++++----
> >>  drivers/mtd/spi-nor/cadence-quadspi.c |  18 ++-
> >>  drivers/mtd/spi-nor/fsl-quadspi.c     |   8 +-
> >>  drivers/mtd/spi-nor/hisi-sfc.c        |  32 +++-
> >>  drivers/mtd/spi-nor/mtk-quadspi.c     |  16 +-
> >>  drivers/mtd/spi-nor/nxp-spifi.c       |  21 +--
> >>  drivers/mtd/spi-nor/spi-nor.c         | 280 +++++++++++++++++++++++++++-
> >> ------
> [...]
> >> +static int spi_nor_setup(struct spi_nor *nor, const struct flash_info *info,
> >> +                     const struct spi_nor_basic_flash_parameter
> >> *params,
> >> +                     const struct spi_nor_modes *modes)
> >>  {
> >> +    bool enable_quad_io;
> >> +    u32 rd_modes, wr_modes, mask;
> >> +    const struct spi_nor_erase_type *erase_type = NULL;
> >> +    const struct spi_nor_read *read;
> >> +    int rd_pindex, wr_pindex, i, err = 0;
> >> +    u8 erase_size = SNOR_ERASE_64K;
> > 
> > Erase size could be configurable, then user can chose best sector size that match his use case on multi-sized flash.
> >
>
> The purpose of this patch is only to add support to more SPI protocols.
> About the sector erase operations, spi_nor_init_params() and
> spi_nor_setup() are written so the resulting configuration is the same as
> before the patch. Currently only 64K and 4K sector erase operations are
> supported.
>
> The only difference introduced by this patch is when neither the 64K Sector
> Erase nor the 4K Sector Erase operations are supported, for instance when
> the memory supports only 256K Sector Erase operations, spi_nor_setup() can
> now select another size as a fallback according to the supported sizes
> given in params->erase_types[i].
> With this patch only, ie without the SFDP patch, spi_nor_init_params()
> assumes that at least the 64K Sector Erase operations are supported and
> depending on info->flags, the 4K Sector Erase operations might be supported
> too. The current spi-nor framework makes the very same assumption.

The is no assumption in current spi-nor framework that flash need to support 64KiB
erase size (eg. s25fl256s0 entry).

The reason for this comment is my S25FS512S. As we have already talked
on #mtd this one reports in JESD216 tables that it support 4KiB erase size,
64KiB and 256KiB. Unfortunately it does not support 64KiB. Moreover, opcode
for 64KiB erase and 456KiB erase is reported as the same. Probably to
distinguish which sector flash version it is, we need to know JEDEC ID
(as in older families eg s25fl256s0 and s25fl256s1).

If you just do: u8 erase_size = info->sector_size it should work.
Question is what are your plans for flash_info.
Looking at this case, and number of dies in chip erase it is not possible to get rid
of it completely.

>
> Also before and after this patch, the spi-nor framework still assumes a
> uniform sector erase size. I know this is not true for all memories but
> this bug should be fixed by another dedicated patch.

Sure, this is not my expectation.
>
> I would like a smooth transition between the current spi-nor framework and
> a new one changing the 3rd argument of spi_nor_scan().
> Changing too many things in a single patch would complicate the review.
> Once the "4byte address instruction set" series will be accepted, I plan to
> send this patch as a standalone patch. Then later the SFDP changes and so on.
>
> I'm splitting the SFDP series to ease its review.

Very good idea, this patch is really hard to read.
>
> >> +
> >> +    /* 2-2-2 or 4-4-4 modes are not supported yet. */
> >> +    mask = (SNOR_MODE_2_2_2 | SNOR_MODE_4_4_4);
> >> +    rd_modes = modes->rd_modes & ~mask;
> >> +    wr_modes = modes->wr_modes & ~mask;
> >> +
> >> +    /* Setup read operation. */
> >> +    rd_pindex = fls(params->rd_modes & rd_modes) - 1;
> >> +    if (spi_nor_pindex2proto(rd_pindex, &nor->read_proto)) {
> >> +            dev_err(nor->dev, "invalid (fast) read\n");
> >> +            return -EINVAL;
> >> +    }
> >> +    read = &params->reads[rd_pindex];
> >> +    nor->read_opcode = read->opcode;
> >> +    nor->read_dummy = read->num_mode_clocks + read-
> >>> num_wait_states;
> > 
> > I would vote that num_mode_clocks, num_wait_states and mode value will be available for the driver.
> > There are some QSPi controller that are aware of that. It generally should not hurt, but might help in the future.
> >
>
> I thought about that but finally I've chosen to hide the mode/wait_states
> values and only provide the sum in nor->read_dummy, so for all SPI
> controller drivers the nor->read_dummy value as the exact same meaning as
> before this patch.
>
> Indeed, the *only* purpose of the mode cycle value is during some Fast Read
> operations (mainly Fast Read 1-2-2 or Fast Read 1-4-4) for asking the QSPI
> memory to enter/leave it's 0-2-2 or 0-4-4 mode.
>
> "0-x-x mode" meaning that the next SPI command sent on the bus to the
> memory skips the now implicit Fast Read op code and starts directly to the
> address cycles. Hence entering/leaving those 0-x-x modes is statefull and
> like using the 4byte address mode, many bootloaders don't support that mode.
>
> The performance improvement provided by the 0-x-x modes is only relevant
> for a eXecution In Place usage of the QSPI memory, where very few bytes are
> read in a single SPI command (a I-cache line). However XIP is out of the
> scope of the spi-nor framework, which in most usage, reads at least a full
> page (>= 256 bytes).
>
> Also whatever the actual number of mode cycle is, I recommend to always set
> at least the very first *byte* of dummy data to the 0xFF value.
> Indeed, according to the SFDP specification, this value works with every
> manufacturer to prevent their memory from entering a 0-x-x mode.
>
> For instance, this is what I did in atmel_qspi_read().
> I also plan to patch m25p80 so this driver sets all dummy cycles to 0xFF:
> currently the dummy bytes are uninitialized.
>
> Other QSPI controller drivers aware of the mode cycles should do the same.
>

Sound reasonable :)
Maybe a nice comment about this expectations from driver
could help to avoid some problems in the feature.
>
> >> +
> >> +    /* Set page program op code and protocol. */
> >> +    wr_pindex = fls(params->wr_modes & wr_modes) - 1;
> >> +    if (spi_nor_pindex2proto(wr_pindex, &nor->write_proto)) {
> >> +            dev_err(nor->dev, "invalid page program\n");
> >> +            return -EINVAL;
> >> +    }
> >> +    nor->program_opcode = params->page_programs[wr_pindex];
> >> +
> >> +    /* Set sector erase op code and size. */
> >> +    erase_type = &params->erase_types[0];
> >> +#ifdef CONFIG_MTD_SPI_NOR_USE_4K_SECTORS
> >> +    erase_size = SNOR_ERASE_4K;
> >> +#endif
> >> +    for (i = 0; i < SNOR_MAX_ERASE_TYPES; ++i) {
> >> +            if (params->erase_types[i].size == erase_size) {
> >> +                    erase_type = &params->erase_types[i];
> >> +                    break;
> >> +            } else if (!erase_type->size && params->erase_types[i].size)
> >> {
> >> +                    erase_type = &params->erase_types[i];
> >> +            }
> >> +    }
> >> +    nor->erase_opcode = erase_type->opcode;
> >> +    nor->mtd.erasesize = (1 << erase_type->size);
> >> +
> >> +
> >> +    enable_quad_io = (SNOR_PROTO_DATA_FROM_PROTO(nor-
> >>> read_proto) == 4 ||
> >> +                      SNOR_PROTO_DATA_FROM_PROTO(nor-
> >>> write_proto) == 4);
> >> +
> >> +    /* Enable Quad I/O if needed. */
> >> +    if (enable_quad_io && params->enable_quad_io) {
> >> +            err = params->enable_quad_io(nor);
> >> +            if (err) {
> >> +                    dev_err(nor->dev,
> >> +                            "failed to enable the Quad I/O mode\n");
> >> +                    return err;
> >> +            }
> >> +    }
> >> +
> >> +    dev_dbg(nor->dev,
> >> +            "(Fast) Read:  opcode=%02Xh, protocol=%03x, mode=%u,
> >> wait=%u\n",
> >> +            nor->read_opcode, nor->read_proto,
> >> +            read->num_mode_clocks, read->num_wait_states);
> >> +    dev_dbg(nor->dev,
> >> +            "Page Program: opcode=%02Xh, protocol=%03x\n",
> >> +            nor->program_opcode, nor->write_proto);
> >> +    dev_dbg(nor->dev,
> >> +            "Sector Erase: opcode=%02Xh, protocol=%03x, sector
> >> size=%u\n",
> >> +            nor->erase_opcode, nor->reg_proto, (u32)nor-
> >>> mtd.erasesize);
> > 
> > At the end above debugs can be a bit misleading, because later opcodes could be replaced in
> > set_4byte function.
> >
>
> Indeed, I agree with you: I will remove those dev_dbg() on the next version.
>
>
> > Thanks,
> > Marcin
> > 
>
> Thanks for the review :)

Unfortunately that all just a small comments not a decent review :)

Thanks,
Marcin

>
> Best regards,
>
> Cyrille
>

Powered by blists - more mailing lists