drivers: flash: spi-nor: add write-block-size for mcuboot

[maass-hamburg]

add write-block-size in dt for spi-nor. The cmake
functions related to mcuboot in the build system
want this prop to be set. It's not meant to be changed.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[de-nordic]

Nack. Software configuration does not go into hardware description.

[maass-hamburg]

@de-nordic where to put it instead? in the mcuboot.cmake, but there the default is already 4 for some options and we get a warning there.

[de-nordic]

@de-nordic where to put it instead? in the mcuboot.cmake, but there the default is already 4 for some options and we get a warning there.

We have this thing for spi nor

zephyr/drivers/flash/Kconfig.nor

Line 80 in c7086c7

config SPI_NOR_FLASH_LAYOUT_PAGE_SIZE

.
This ain't perfect, because you have to have other for qspi.

Flash DTS nodes should only contain flash parameters; they belong to flash drivers and nothing else should use them. I know this is already abused by various subsystems picking directly into DTS nodes, but that is wrong and completely breaks modularity of the system; such approach increases coupling and takes away control over that parameter from the device driver - which should be sole user of it, as it is parameter defined under node it owns.
If there is some logic, in driver, that relies on the parameter the systems that directly pick the param from DTS will be oblivious to that logic and may use it incorrectly.
If MCUboot is allowed to do that, why not other susbystems? Now we have an entanglement where any changes in DTS of a Flash device become breaking change for half of the system.

So it is not even about adding the param, but rather reading it from DTS: because it will be not something you should read directly.

It is very hard to move things around, within modules, once people are used to just peeking whatever they want, because they will oppose taking "works for me" away from them; that is at a cost of freedom of improving driver, as changes will lead to breaking implicit interface, created by bypassing barriers between software modules, even though the improvement would not break the commonly defined interface.

[maass-hamburg]

@de-nordic I understand you, the thing with the Kconfig unfortunately is, that we don't easily know which one to use, as we could have multiple different flashes and drivers on one board and we need it of the one that contains the code partition. Doing that via Kconfigs is going to get ugly soon.

[cfriedt]

I would maybe not make it const and remove the default.

I have a similar pr for erase block size.

Both of those are fine, IMHO, but I might have missed something.

[maass-hamburg]

@cfriedt the spi_nor driver expect it to be 1:

zephyr/drivers/flash/spi_nor.c

Lines 293 to 296 in 9d10d67

	static const struct flash_parameters flash_nor_parameters = {
	.write_block_size = 1,
	.erase_value = 0xff,
	};

[de-nordic]

@de-nordic I understand you, the thing with the Kconfig unfortunately is, that we don't easily know which one to use, as we could have multiple different flashes and drivers on one board and we need it of the one that contains the code partition. Doing that via Kconfigs is going to get ugly soon.

Yet you still do not care, because it fixes your problem, and it is my things that get ugly and entangled.

What we are missing is dedicated way to configure instances of software subsystems that could be shared with run-time and compile time. Something like this #80262, or maybe the other thing that was proposed around generating Settings (which I do not remember what happened with it).

I do not have anything against adding the parameter, I am opposing you using it bypassing the API and picking it directly. You are breaking modularity, information hiding and increasing coupling between external scripts and parameters that belong to drivers. write-block-size is flash parameter, MCUboot does not have such parameter, it has own thing.

I have worked through several instance where change to Flash API Kconfig required a lot of work, because somehow these Kconfigs have been used to control how other subsystems worked directly - like I could not turn on pages layout where I needed it, because suddenly one of subsystems compiled other way, even though I did not wanted it to use it. But it relied on the same Kconfigs that controlled Flash API directly in code.

I know that there is no name-spacing in Kconfig and that may be a problem to clearly show where boundaries are.

Probably what should really happen, because we do not have the zml thing yet, is same approach as with Disk instances in DTS:

mcuboot {
    compatible = "zephyr,mcuboot-software-something";
    slot0_partition = &some_partition_a;
    slot1_partition = &some_partition_b;
    max_align_size = 8;
    logical_sector_size = 4k;
    ...
}

Now you have MCUboot object that defines how MCUboot is visible by anything that uses it out of dts; this becomes de-facto API for MCUboot configuration and can be used by run-time and compile time, because it is dedicated for it.
Note that there is no write-block-size - because it does not matter, MCUboot operates on write alignment property internally! The property should be verified, by for example mcuboot code, against flash API returned values.

The node consists only of things that belong to mcuboot and its behaviour, so it also belongs to imgtool.

[JarmouniA]

The proposed property already exists in a generic flash binding

zephyr/dts/bindings/mtd/soc-nv-flash.yaml

Line 12 in 169cf86

write-block-size:

In my opinion, the external Flash nodes that need the property in order to work with MCUboot should just add the soc-nv-flash compatible and define write-block-size.

[cfriedt]

In my opinion, the external Flash nodes that need the property in order to work with MCUboot should just add the soc-nv-flash compatible and define write-block-size.

yeah, fair enough