Re: [PATCH 4/9] mtd: spi-nand: Add continuous read support

[Pratyush Yadav <pratyush@kernel.org>]

Hi,

On Wed, Aug 21 2024, Miquel Raynal wrote:

> A regular page read consist in:
> - Asking one page of content from the NAND array to be loaded in the
>   chip's SRAM,
> - Waiting for the operation to be done,
> - Retrieving the data (I/O phase) from the chip's SRAM.
>
> When reading several sequential pages, the above operation is repeated
> over and over. There is however a way to optimize these accesses, by
> enabling continuous reads. The feature requires the NAND chip to have a
> second internal SRAM area plus a bit of additional internal logic to
> trigger another internal transfer between the NAND array and the second
> SRAM area while the I/O phase is ongoing. Once the first I/O phase is
> done, the host can continue reading more data, continuously, as the chip
> will automatically switch to the second SRAM content (which has already
> been loaded) and in turns trigger the next load into the first SRAM area
> again.
>
> From an instruction perspective, the command op-codes are different, but
> the same cycles are required. The only difference is that after a
> continuous read (which is stopped by a CS deassert), the host must
> observe a delay of tRST. However, because there is no guarantee in Linux
> regarding the actual state of the CS pin after a transfer (in order to
> speed-up the next transfer if targeting the same device), it was
> necessary to manually end the continuous read with a configuration
> register write operation.
>
> Continuous reads have two main drawbacks:
> * They only work on full pages (column address ignored)
> * Only the main data area is pulled, out-of-band bytes are not
>   accessible. Said otherwise, the feature can only be useful with on-die
>   ECC engines.
>
> Performance wise, measures have been performed on a Zynq platform using
> Macronix SPI-NAND controller with a Macronix chip (based on the
> flash_speed tool modified for testing sequential reads):
> - 1-1-1 mode: performances improved from +3% (2-pages) up to +10% after
>               a dozen pages.
> - 1-1-4 mode: performances improved from +15% (2-pages) up to +40% after
>               a dozen pages.
>
> This series is based on a previous work from Macronix engineer Jaime
> Liao.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
> diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
> index 9042a092687c..964c9035fdc8 100644
> --- a/drivers/mtd/nand/spi/core.c
> +++ b/drivers/mtd/nand/spi/core.c
[...]
> +
> +static DEFINE_STATIC_KEY_FALSE(cont_read_supported);
> +
> +static void spinand_cont_read_init(struct spinand_device *spinand)
> +{
> +	struct nand_device *nand = spinand_to_nand(spinand);
> +	enum nand_ecc_engine_type engine_type = nand->ecc.ctx.conf.engine_type;
> +
> +	/* OOBs cannot be retrieved so external/on-host ECC engine won't work */
> +	if (spinand->set_cont_read &&
> +	    (engine_type == NAND_ECC_ENGINE_TYPE_ON_DIE ||
> +	     engine_type == NAND_ECC_ENGINE_TYPE_NONE)) {
> +		static_branch_enable(&cont_read_supported);
> +	}
> +}
> +
> +static bool spinand_use_cont_read(struct mtd_info *mtd, loff_t from,
> +				  struct mtd_oob_ops *ops)
> +{
> +	struct nand_device *nand = mtd_to_nanddev(mtd);
> +	struct nand_pos start_pos, end_pos;
> +
> +	/* OOBs won't be retrieved */
> +	if (ops->ooblen || ops->oobbuf)
> +		return false;
> +
> +	nanddev_offs_to_pos(nand, from, &start_pos);
> +	nanddev_offs_to_pos(nand, from + ops->len - 1, &end_pos);
> +
> +	/*
> +	 * Continuous reads never cross LUN boundaries. Some devices don't
> +	 * support crossing planes boundaries. Some devices don't even support
> +	 * crossing blocks boundaries. The common case being to read through UBI,
> +	 * we will very rarely read two consequent blocks or more, so it is safer
> +	 * and easier (can be improved) to only enable continuous reads when
> +	 * reading within the same erase block.
> +	 */
> +	if (start_pos.target != end_pos.target ||
> +	    start_pos.plane != end_pos.plane ||
> +	    start_pos.eraseblock != end_pos.eraseblock)
> +		return false;
> +
> +	return start_pos.page < end_pos.page;
> +}
> +
>  static int spinand_mtd_read(struct mtd_info *mtd, loff_t from,
>  			    struct mtd_oob_ops *ops)
>  {
> @@ -684,7 +830,11 @@ static int spinand_mtd_read(struct mtd_info *mtd, loff_t from,
>  
>  	old_stats = mtd->ecc_stats;
>  
> -	ret = spinand_mtd_regular_page_read(mtd, from, ops, &max_bitflips);
> +	if (static_branch_unlikely(&cont_read_supported) &&
> +	    spinand_use_cont_read(mtd, from, ops))

This looks a bit odd. If your system has two NAND devices, one with
continuous read support and one without, you will enable this static
branch and then attempt to use continuous read for both, right? I think
spinand_use_cont_read() should have a check for spinand->set_cont_read,
otherwise you end up calling a NULL function pointer for the flash
without continuous read.

This would reduce the cost of checking set_cont_read in the hot path if
there are no flashes with continuous read. When you do have at least
one, you would have to pay it every time. I suppose you can do some sort
of double branch where you take the respective static branch if _all_ or
_none_ of the flashes have continuous read, and then the heterogeneous
setups do the check at runtime. But is spinand_mtd_read() even hot
enough to warrant such optimizations?

> +		ret = spinand_mtd_continuous_page_read(mtd, from, ops, &max_bitflips);
> +	else
> +		ret = spinand_mtd_regular_page_read(mtd, from, ops, &max_bitflips);
>  
>  	if (ops->stats) {
>  		ops->stats->uncorrectable_errors +=
> @@ -874,6 +1024,9 @@ static int spinand_create_dirmap(struct spinand_device *spinand,
>  	};
>  	struct spi_mem_dirmap_desc *desc;
>  
> +	if (static_branch_unlikely(&cont_read_supported))
> +		info.length = nanddev_eraseblock_size(nand);

Same here. With a heterogeneous setup, you will set length to eraseblock
size even for the non-continuous-read flash. Though from a quick look
info.length doesn't seem to be used anywhere meaningful.

In general, a static branch looks misused here. This isn't even a hot
path where you'd care about performance.

> + /* The plane number is passed in MSB just above the column address
> */ info.offset = plane << fls(nand->memorg.pagesize);
>  
[...]

-- 
Regards,
Pratyush Yadav

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/