Re: [PATCH] mtd: spi-nor: Fix the read issue when addr_width is 4byte

[Marek Vasut <marek.vasut@mailbox.org>]

On 12/5/24 8:35 AM, Tudor Ambarus wrote:
> + Marek
> 
> use get_maintainers script please
> 
> On 11/18/24 9:05 AM, Venkatesh Yadav Abbarapu wrote:
>> Fix the read issue for 4byte address width by passing the entire
>> length to the read function, split the memory of 16MB size banks
>> only when the address width is 3byte. Also update the size when
>> the configuration is stacked.
>>
>> Fixes: 5d40b3d384 ("mtd: spi-nor: Add parallel and stacked memories support")
>> Signed-off-by: Venkatesh Yadav Abbarapu <venkatesh.abbarapu@amd.com>
>> ---
>>   drivers/mtd/spi/spi-nor-core.c | 24 +++++++++++++++---------
>>   1 file changed, 15 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
>> index ec841fb13b..c37a9bce74 100644
>> --- a/drivers/mtd/spi/spi-nor-core.c
>> +++ b/drivers/mtd/spi/spi-nor-core.c
>> @@ -1580,6 +1580,7 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len,
>>   	loff_t offset = from;
>>   	u32 read_len = 0;
>>   	u32 rem_bank_len = 0;
>> +	u32 stack_shift = 0;
>>   	u8 bank;
>>   	bool is_ofst_odd = false;
>>   
>> @@ -1593,18 +1594,20 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len,
>>   	}
>>   
>>   	while (len) {
>> -		bank = (u32)from / SZ_16M;
>> -		if (nor->flags & SNOR_F_HAS_PARALLEL)
>> -			bank /= 2;
>> -
>> -		rem_bank_len = SZ_16M * (bank + 1);
>> -		if (nor->flags & SNOR_F_HAS_PARALLEL)
>> -			rem_bank_len *= 2;
>> -		rem_bank_len -= from;
>> -
>> +		if (nor->addr_width == 3) {
> 
> No, this looks bad because you enter the body of this for all the
> flashes with addr_width 3, not just for the parallel/stacked ones.
> 
> Instead you should do whatever you do here just for your
> parallel/stacked cases.
> 
>> +			bank = (u32)from / SZ_16M;
>> +			if (nor->flags & SNOR_F_HAS_PARALLEL)
>> +				bank /= 2;
>> +
>> +			rem_bank_len = SZ_16M * (bank + 1);
>> +			if (nor->flags & SNOR_F_HAS_PARALLEL)
>> +				rem_bank_len *= 2;
>> +			rem_bank_len -= from;
>> +		}
>>   		offset = from;
>>   
>>   		if (nor->flags & SNOR_F_HAS_STACKED) {
>> +			stack_shift = 1;
>>   			if (offset >= (mtd->size / 2)) {
>>   				offset = offset - (mtd->size / 2);
>>   				nor->spi->flags |= SPI_XFER_U_PAGE;
>> @@ -1616,6 +1619,9 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len,
>>   		if (nor->flags & SNOR_F_HAS_PARALLEL)
>>   			offset /= 2;
>>   
>> +		if (nor->addr_width == 4)
>> +			rem_bank_len = (mtd->size >> stack_shift) - offset;
> 
> same here.
> 
> If we still want to keep this support until everything is implemented
> above SPI NOR, other option is to duplicate core methods and introduce
> dedicated methods for the parallel/stacked cases. You could keep the
> parallel/stacked methods in dedicated file as well.
> 
> The advantage is that you won't interfere with current SPI NOR support
> and that you won't break any flashes anymore. Then we can easily remove
> everything once proper support is implemented above SPI NOR. The
> disadvantage is that we duplicate code ... Marek?
I can imagine the duplication can be solved by factoring out the SPI NOR 
code into a library design, where each callback implementation could be 
assembled from building blocks, thus avoiding code duplication. However, 
this seems like a huge change for the current 2025.01 release cycle, we 
are already in rc4.

For 2025.01 , either this stacked stuff has to be carefully ifdeffed 
out, so it can be enabled only for platforms which need it and disabled 
for all others to avoid breaking them ... or reverted outright .