drivers: flash: spi nor: Add MultiInstance support

[ashiroji]

Modify the SPI Nor driver to be able to have multiple instances at the same time. This patch is heavily inspired by the at45 driver. It was tested on the nRF5340 DK by using the external spi memory two times.

[Laczen]

@ashiroji, thank you for the proposal. Although I haven't gone trough the complete PR I have two general remarks:

1. I was suspecting changes to the Kconfig file as multi-instance is not easy supported by Kconfig options that are for all instances,
2. The use of ANY_INST_..., instead you should create a structure that fits the instance while using the FOR_EACH macro construct.

[ashiroji]

Hi @Laczen,

1. I was suspecting changes to the Kconfig file as multi-instance is not easy supported by Kconfig options that are for all instances,

Can you please expand more?

2. The use of ANY_INST_..., instead you should create a structure that fits the instance while using the FOR_EACH macro construct.

The modifications I made are inspired by the at45 flash driver without trying to redesign the SPI NOR driver. In the original implementation, there is conditional code that gets compiled (or not) depending the existence of a feature or not (such as dpd). If each driver instance has a fitting structure, the compiler will complain that a data field is missing.

[Laczen]

Hi @Laczen,

1. I was suspecting changes to the Kconfig file as multi-instance is not easy supported by Kconfig options that are for all instances,

Can you please expand more?

As an example the Kconfig has an option: SPI_NOR_SFDP_MINIMAL, this will be enabled globally for all instances and does not match the multi-instance solution.

2. The use of ANY_INST_..., instead you should create a structure that fits the instance while using the FOR_EACH macro construct.

The modifications I made are inspired by the at45 flash driver without trying to redesign the SPI NOR driver. In the original implementation, there is conditional code that gets compiled (or not) depending the existence of a feature or not (such as dpd). If each driver instance has a fitting structure, the compiler will complain that a data field is missing.

Multi-instance support requires supporting a mix of devices that have and don't have a feature, the use of ANY_INST_... could be used to add/eliminate certain parts of the code (or to make it do nothing/not access unavailable elements when it is not used), but it does not seem appropiate to define the structures based upon ANY_INST_.... But I repeat I haven't gone trough the complete PR.

[ashiroji]

Hi @Laczen,

1. I was suspecting changes to the Kconfig file as multi-instance is not easy supported by Kconfig options that are for all instances,

Can you please expand more?

As an example the Kconfig has an option: SPI_NOR_SFDP_MINIMAL, this will be enabled globally for all instances and does not match the multi-instance solution.

I see your point, I did not think about this actually.

2. The use of ANY_INST_..., instead you should create a structure that fits the instance while using the FOR_EACH macro construct.

The modifications I made are inspired by the at45 flash driver without trying to redesign the SPI NOR driver. In the original implementation, there is conditional code that gets compiled (or not) depending the existence of a feature or not (such as dpd). If each driver instance has a fitting structure, the compiler will complain that a data field is missing.

Multi-instance support requires supporting a mix of devices that have and don't have a feature, the use of ANY_INST_... could be used to add/eliminate certain parts of the code (or to make it do nothing/not access unavailable elements when it is not used), but it does not seem appropiate to define the structures based upon ANY_INST_.... But I repeat I haven't gone trough the complete PR.

The general idea I went with, is if an instance has a field, enable the needed data field for all instances and initialise it with proper value from tree (for those who have) and with 0( for those who don't). This is specifically used for deep power down mode parameters. That way the code that uses these parameters will find a valid value even if the feature is not defined.
Please take your time to read the PR and indicate how to improve this

[ashiroji]

@Laczen , does it make sense to transform the Kconfigs to device tree properties? To have the possibility to choose different values for different instances?

[Laczen]

@Laczen , does it make sense to transform the Kconfigs to device tree properties? To have the possibility to choose different values for different instances?

In general yes, but you might leave some default parameters that are used in case they cannot be derived from the dts.

[ashiroji]

@Laczen , does it make sense to transform the Kconfigs to device tree properties? To have the possibility to choose different values for different instances?

In general yes, but you might leave some default parameters that are used in case they cannot be derived from the dts.

I think I can modify the following Kconfigs:
SPI_NOR_SFDP into an enum parameter:

zephyr,spi-nor-sfdp:
type : string
enum:
- "minimal"
- "devicetree"
- "runtime"

SPI_NOR_CS_WAIT_DELAY, SPI_NOR_FLASH_LAYOUT_PAGE_SIZE can be transformed into an int parameter

SPI_NOR_IDLE_IN_DPD into a bool parameter

[Laczen]

@ashiroji, I propose you leave the Kconfig as is, it is not really fit for multi-instance, but it will require so much changes that the PR will grow too much.

Regarding the use of the ANY_INST_... macro: @de-nordic could you comment what should be the best way to handle this: if we have multiple instances of a driver is it acceptable to create one driver config struct with all options enabled or should the driver config struct be split up in several (optional) parts that are added only when required ?

[ashiroji]

@ashiroji, I propose you leave the Kconfig as is, it is not really fit for multi-instance, but it will require so much changes that the PR will grow too much.

Okay.

Regarding the use of the ANY_INST_... macro: @de-nordic could you comment what should be the best way to handle this: if we have multiple instances of a driver is it acceptable to create one driver config struct with all options enabled or should the driver config struct be split up in several (optional) parts that are added only when required ?

I'll be waiting for further comments

[ashiroji]

@nordicjm , @de-nordic , @nashif can you please review the PR?

[de-nordic]

@ashiroji, I propose you leave the Kconfig as is, it is not really fit for multi-instance, but it will require so much changes that the PR will grow too much.

Regarding the use of the ANY_INST_... macro: @de-nordic could you comment what should be the best way to handle this: if we have multiple instances of a driver is it acceptable to create one driver config struct with all options enabled or should the driver config struct be split up in several (optional) parts that are added only when required ?

I think the current solution with enabling struct elements when they are needed for all instances is OK.
If we figure better way to do that, and dynamically add these fields to some buffer we may think it over.

[ashiroji]

Sorry, I guess I did something wrong when I force-pushed the update.
@de-nordic ,
I opened a new PR
#70415

[de-nordic]

Sorry, I guess I did something wrong when I force-pushed the update. @de-nordic , I opened a new PR #70415

Happens.