Re: [PATCH 2/4] drivers: mtd: spi-nand: try a regular dirmap if creating a dirmap for continuous reading fails

[Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu>]

On 05.08.2025 18:30, Miquel Raynal wrote:
> On 04/08/2025 at 22:21:30 +03, Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu> wrote:
>
>> Continuous reading may result in multiple flash pages reading in one
>> operation. Typically only one flash page has read/written (a little bit
>> more than 2-4 Kb), but continuous reading requires the spi-controller
>> to read up to 512 Kb in one operation without togling CS in beetween.
>>
>> Roughly speaking spi-controllers can be divided on 2 categories:
>>  * spi-controllers without dirmap acceleration support
>>  * spi-controllers with dirmap acceleration support
>>
>> Usually, first of them have no issues with large reading support.
>> Second group often supports acceleration of single page only reading.
>> Thus enabling of continuous reading can break flash reading.
> I would be more conservative, I believe it is very implementation
> dependent; many controller drivers do not even advertise a max size.
>
> I agree though that controllers with dirmap support may express
> limitations such as the mapped size which may lead to splitting
> operations into smaller chunks, possibly leading to CS changes which
> would break the continuous read mode on the chip's side.
>
>> This patch tries to create dirmap for continuous reading first and
>> fallback to regular reading if spi-controller refuses to create it.
> no '-'                              ^
>
>> Signed-off-by: Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu>
>> ---
>>  drivers/mtd/nand/spi/core.c | 29 +++++++++++++++++++++++++----
>>  1 file changed, 25 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
>> index b42c42ec58a4..ff6a1e2fcfdc 100644
>> --- a/drivers/mtd/nand/spi/core.c
>> +++ b/drivers/mtd/nand/spi/core.c
>> @@ -1114,11 +1114,32 @@ static int spinand_create_dirmap(struct spinand_device *spinand,
>>  
>>  	spinand->dirmaps[plane].wdesc = desc;
>>  
>> -	if (spinand->cont_read_possible)
>> +	desc = NULL;
>> +	if (spinand->cont_read_possible) {
>> +		/*
>> +		 * spi-controllers may return an error if info.length is
>> +		 * too large
>> +		 */
>>  		info.length = nanddev_eraseblock_size(nand);
>> -	info.op_tmpl = *spinand->op_templates.read_cache;
>> -	desc = devm_spi_mem_dirmap_create(&spinand->spimem->spi->dev,
>> -					  spinand->spimem, &info);
>> +		info.op_tmpl = *spinand->op_templates.read_cache;
>> +		desc = devm_spi_mem_dirmap_create(&spinand->spimem->spi->dev,
>> +						  spinand->spimem, &info);
>> +	}
>> +
>> +	if (IS_ERR_OR_NULL(desc)) {
> Here if the problem is continuous reading, I expect an error and not a
> NULL pointer.
NULL is possible if flash does not supports continuous reading
>
>> +		/*
>> +		 * continuous reading is not supported by flash or
> Not by the flash, here if we get an error, it is the spi controller
> (still without '-' ;) ) that fails (please fix the comment).

we can go here in 2 cases:
1) spinand->cont_read_possible is false (flash does not supports
continuous reading)

2) spi controller returns an error (spi controller does not like
continuous reading)

>
>> +		 * its spi-controller, try regular reading
>> +		 */
>> +		spinand->cont_read_possible = false;
>> +
>> +		info.length = nanddev_page_size(nand) +
>> +			      nanddev_per_page_oobsize(nand);
>> +		info.op_tmpl = *spinand->op_templates.read_cache;
>> +		desc = devm_spi_mem_dirmap_create(&spinand->spimem->spi->dev,
>> +						  spinand->spimem, &info);
>> +	}
>> +
>>  	if (IS_ERR(desc))
>>  		return PTR_ERR(desc);