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

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

[tkuw584924]

From: Takahiro Kuwano <Takahiro.Kuwano@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@infineon.com>
Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@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. */
+	if (!nor->addr_mode_nbytes) {
+		ret = set_4byte(nor, nor->info, 1);
+		if (ret)
+			return ret;
+
+		nor->addr_mode_nbytes = 4;
+	}
+
 	/*
 	 * S25FS256T has multiple sector architecture options, with selection of
 	 * count and location of 128KB and 64KB sectors. This driver supports
-- 
2.34.1

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

[Tudor Ambarus]

Hi, Takahiro,

On 10/20/25 8:25 AM, tkuw584924@gmail.com wrote:
> From: Takahiro Kuwano <Takahiro.Kuwano@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@infineon.com>
> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@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?

> +	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()?

Cheers,
ta> +		if (ret)
> +			return ret;
> +
> +		nor->addr_mode_nbytes = 4;
> +	}
> +
>  	/*
>  	 * S25FS256T has multiple sector architecture options, with selection of
>  	 * count and location of 128KB and 64KB sectors. This driver supports

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

[Takahiro Kuwano]

Hi Tudor,

On 10/21/2025 3:36 AM, Tudor Ambarus wrote:
> Hi, Takahiro,
> 
> On 10/20/25 8:25 AM, tkuw584924@gmail.com wrote:
>> From: Takahiro Kuwano <Takahiro.Kuwano@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@infineon.com>
>> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@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.

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

> Cheers,
> ta> +		if (ret)
>> +			return ret;
>> +
>> +		nor->addr_mode_nbytes = 4;
>> +	}
>> +
>>  	/*
>>  	 * S25FS256T has multiple sector architecture options, with selection of
>>  	 * count and location of 128KB and 64KB sectors. This driver supports

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

[Tudor Ambarus]

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@gmail.com wrote:
>>> From: Takahiro Kuwano <Takahiro.Kuwano@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@infineon.com>
>>> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@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.

Cheers,
ta

> 
>> Cheers,
>> ta> +		if (ret)
>>> +			return ret;
>>> +
>>> +		nor->addr_mode_nbytes = 4;
>>> +	}
>>> +
>>>  	/*
>>>  	 * S25FS256T has multiple sector architecture options, with selection of
>>>  	 * count and location of 128KB and 64KB sectors. This driver supports
> 

Re: [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@gmail.com wrote:
>>>> From: Takahiro Kuwano <Takahiro.Kuwano@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@infineon.com>
>>>> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@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