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

[Miquel Raynal]

Hi Pratyush,

> > @@ -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.

Mmmh that's right, I wanted a slightly more optimal path but the static
branch doesn't play well here if it's a global parameter.

> 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?

I believe using static branches here would have been relevant with a
single chip use-case, but I don't think it is worth using if we have
more than one flash. It is an almost tepid path, at most, so I'll just
check ->set_cont_read presence.

> > +		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.

It is a parameter for the spi controller driver. It may lead to an
error if the size is too big for what the controller is capable of,
which is not the case for any of the existing controllers which are
all capable of mapping a block at least (source: me reading spi code).

the info parameter being per-nand it's fine to tune it like that, but I
agree the if() conditional is currently wrong in the case of dual
devices.

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

TBH the read path might be considered hot, but not hot enough to
justify too complex optimizations schemes, definitely.

Thanks a lot!
Miquèl