Re: [PATCH v2] mtd: spi-nor: Fix the spi_nor_read() when config SPI_STACKED_PARALLEL is enabled

[Marek Vasut <marex@denx.de>]

On 12/16/24 2:58 PM, Abbarapu, Venkatesh wrote:

>>>>> +++ b/drivers/mtd/spi/spi-nor-core.c
>>>>> @@ -1140,7 +1140,7 @@ static int spi_nor_erase(struct mtd_info *mtd,
>>>>> struct
>>>> erase_info *instr)
>>>>>     				nor->spi->flags &= ~SPI_XFER_U_PAGE;
>>>>>     		}
>>>>>     #ifdef CONFIG_SPI_FLASH_BAR
>>>>> -		ret = write_bar(nor, addr);
>>>>> +		ret = write_bar(nor, offset);
>>>>
>>>> This change is really inobvious, the code above likely needs to be
>>>> compiled out if the SPI_STACKED_PARALLEL stuff is disabled ?
>>>
>>> In the spi_nor_erase()
>>> 	offset = addr;
>>> 	if(PARALLEL)
>>> 		offset/=2;
>>> 	so for parallel or single configuration we need to pass "offset" to write_bar()
>>> 	write_bar(nor, offset");
>>
>> The code above likely needs to be compiled out if the SPI_STACKED_PARALLEL
>> stuff is disabled ?
>>
>> [...]
>>
> Already the code here is being checked with the flags SNOR_F_HAS_PARALLEL and SNOR_F_HAS_STACKED. Do you want to add the check SPI_STACKED_PARALLEL apart from these flags?

Yes, to compile the code out completely.

>>>> If I look at this change with 'git show -w' , the change looks like this:
>>>>
>>>> "
>>>>     #ifdef CONFIG_SPI_FLASH_BAR
>>>> +               u32 remain_len;
>>>> +
>>>>                    ret = write_bar(nor, offset);
>>>>                    if (ret < 0)
>>>>                            return log_ret(ret);
>>>> +               remain_len = (SZ_16M * (nor->bank_curr + 1)) - offset;
>>>> +               if (len < remain_len)
>>>> +                       read_len = len;
>>>> +               else
>>>> +                       read_len = remain_len;
>>>>     #endif
>>>> -
>>>> +               if (CONFIG_IS_ENABLED(SPI_STACKED_PARALLEL)) {
>>>>                            if (len < rem_bank_len)
>>>>                                    read_len = len;
>>>>                            else
>>>>                                    read_len = rem_bank_len;
>>>> -
>>>> +               }
>>>>                    if (read_len == 0)
>>>>                            return -EIO; "
>>>>
>>>> Why is there this part of code twice now, ifdeffed out differently in each case ?
>>>>
>>>> "
>>>>                            if (len < rem_bank_len)
>>>>                                    read_len = len;
>>>>                            else
>>>>                                    read_len = rem_bank_len; "
>>>
>>> For parallel/stacked configuration and address width the "rem_bank_len" will vary
>> and as we don't want to disturb the default read functionality added the ifdef
>> separately.
>> What would happen if both SPI_FLASH_BAR and SPI_STACKED_PARALLEL are
>> enabled on a system that only has one SPI NOR attached
>> (non-stacked/parallel) ? I noticed the second "copy" of the code behaves slightly
>> differently in the else branch, so does that mean this would break such setup ?
> 
> If both SPI_FLASH_BAR and SPI_STACKED_PARALLEL are enabled, the "rem_bank_len" manipulation is done under the CONFIG_IS_ENABLED(SPI_STACKED_PARALLEL) code and this won't break any default functionality.
Wouldn't read_len calculation be done twice ?