[PATCH] mtd: spi-nor: Set address mode in SEMPER flash when SFDP is skipped

[Takahiro Kuwano]

Hi Tudor,

On 10/23/2025 5:21 PM, Tudor Ambarus wrote:
> 
> 
> On 10/21/25 8:41 AM, Takahiro Kuwano wrote:
>> Hi Tudor,
> 
> Hi!
> 
>>
>> On 10/21/2025 3:36 AM, Tudor Ambarus wrote:
>>> Hi, Takahiro,
>>>
>>> On 10/20/25 8:25 AM, tkuw584924 at gmail.com wrote:
>>>> From: Takahiro Kuwano <Takahiro.Kuwano at infineon.com>
>>>>
>>>> nor->addr_mode_nbytes is set during SFDP parse. Infineon SEMPER flash
>>>> family relies on that parameter to read and write registers. To support
>>>> use cases of skipping SFDP, set address mode in device specific setup()
>>>> function.
>>>>
>>>> Tested-by: Hiroyuki Saito <Hiroyuki.Saito2 at infineon.com>
>>>> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano at infineon.com>
>>>> ---
>>>>  drivers/mtd/spi/spi-nor-core.c | 10 ++++++++++
>>>>  1 file changed, 10 insertions(+)
>>>>
>>>> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
>>>> index 6f352c5c0e2..e382a518a34 100644
>>>> --- a/drivers/mtd/spi/spi-nor-core.c
>>>> +++ b/drivers/mtd/spi/spi-nor-core.c
>>>> @@ -3794,6 +3794,16 @@ static int s25_s28_setup(struct spi_nor *nor, const struct flash_info *info,
>>>>  #ifdef CONFIG_SPI_FLASH_BAR
>>>>  	return -ENOTSUPP; /* Bank Address Register is not supported */
>>>>  #endif
>>>> +
>>>> +	/* Setup address mode here, in case SFDP is skipped. */
>>>
>>> Under which conditions is the SFDP skipped?
>>>
>> In u-boot, SFDP is config option and some defconfig do not use it.
> 
> I'd argue this is a good idea. Maybe for the tiny duplicate version
> of the driver could be, but for the main driver, it's not.
> 
> I'd also argue the tiny driver idea was ideal to start with. Instead
> we should have tried to modularize SPI NOR, by vendors, static init
> and SFDP.
> 
>>
>>>> +	if (!nor->addr_mode_nbytes) {
>>>> +		ret = set_4byte(nor, nor->info, 1);
>>>
>>> why do you need this call? Isn't enough the one done in spi_nor_init()?
>>>
>> The addr_mode_nbytes is refereed in spansion_read_any_reg() during
>> setup().
> 
> Right. So you set it twice. Can we set it just once?
> 
> I guess you can feel my disapproval about where the code is leading to if
> we continue like this: unmaintainable code.
> 
Yeah, I will revisit to cleanup and isolation.

Thanks,
Takahiro