DTS: flash alignment description #860

nvlsianpu · 2017-07-20T10:17:55Z

Added property for description of
the flash alignment required by write operations.
Thanks to that l-value FLASH_WRITE_BLOCK_SIZE macro
will be generated. It is useful for any component uses
the flash.

Added such a description for nrf52840 SoC.

Signed-off-by: Andrzej Puzdrowski andrzej.puzdrowski@nordicsemi.no

galak

Are we specifying alignment or the write/erase block size? Not sure what the compatible of 'arm' or 'intel' is getting you (and it seems wrong as the compatible should probably be to the flash type on the SoC).

nvlsianpu · 2017-07-20T12:48:09Z

@galak Is https://github.com/zephyrproject-rtos/zephyr/blob/master/dts/arm/atmel/sam4s.dtsi#L27 looks suitable for you?

galak · 2017-07-20T12:54:01Z

As far as the compatible goes, that's probably should be removed. It should really be something like compatible = "atmel,sam-soc-flash".

nvlsianpu · 2017-07-20T13:15:01Z

Then in *.yaml is expected to have such a compatible property:

- compatible:
      (...)
      constraint: "nordic, nrf-soc-flash", "atmel, sam-soc-flash" (...) "vendor, vedor-soc-specific-flash"

Em I right?

d3zd3z · 2017-07-20T13:44:13Z

The compatible value in the yaml may need to be answered by Andy, but it definitely shouldn't be "arm" or "intel". Generally, the flash device hasn't had a compatible value, but lives as a child of the flash controller, and contains a reg field to describe the mapping of the actual flash device. Other devices just have it at the top level, and I guess it is found by name.

I think Andy added a way to find nodes by where they are in the tree, but I don't remember how that happens.

The description should probably be changed to all match write-block-size. At least to me, the block size would include the alignment that the writes need to happen, but also captures that it is the minimum size that the writes must be done at. I think "alignment" was a poor name choice by the mynewt/mcuboot code.

galak · 2017-07-20T15:03:03Z

As David said, we probably should move flash nodes under a flash controller. The NXP dts I think already do this.

Wonder if 'erase-size' as spec in the linux kernel Documentation/devicetree/bindings/mtd/mtd-physmap.txt makes sense for the property name.

mbolivar · 2017-07-20T15:50:19Z

Wonder if 'erase-size' as spec in the linux kernel Documentation/devicetree/bindings/mtd/mtd-physmap.txt makes sense for the property name.

Usually write size (1B-8B) << erase size (sector size), no? The meaning of "alignment" in this context is "write size".

galak · 2017-07-21T14:06:30Z

Should we have both write-size & erase-size as part of the flash node?

mbolivar · 2017-07-21T14:26:49Z

Erase-size is sector size. Not all flash parts have uniform sector sizes. This is being addressed in a separate PR, #816

Edit: the reason for doing it as in 816 is that, if done by hand as nodes, then you'd end up with .dtsi files with hundreds of identical 4K sectors on some parts, which seems unnecessary and doesn't really help with what the user will likely want, which is to query sector information around a given flash offset

galak · 2017-08-03T12:44:09Z

So what's the plan here, are we going to update this or is it needed anymore?

nvlsianpu · 2017-08-07T10:21:09Z

This is still needed (I was on holiday).

@galak wrote:

As David said, we probably should move flash nodes under a flash controller. The NXP dts I think already do this.

Following nxp description I propose similar for nordic:

flash-controller@0x4001E000 {
			compatible = "nxp,nrf52-flash-controller";
			reg = <0x4001E000 0x550>;

			#address-cells = <1>;
			#size-cells = <1>;

			flash0: flash@0 {
				reg = <0x00000000 DT_FLASH_SIZE>;
                                write-block-size = <4>;
			};
		};

@d3zd3z, @galak Do you expect to have such description in nrf52840.dtsi file?

carlescufi · 2017-08-08T10:39:06Z

Adding @agross-linaro here

carlescufi · 2017-08-08T10:40:10Z

@galak, I think @nvlsianpu's latest proposal based on the NXP looks reasonable?

agross-oss · 2017-08-08T15:38:31Z

I am ok with it being inside or outside the flash controller. I don't think we should absolutely mandate it be inside. The main thing is that there is a label that can be used to point to the flash node or partition.

galak · 2017-08-08T15:58:58Z

placing the flash node inside the controller has issues with addressing. Since the flash controller addresses would be for the MMIO register space to program/adjust the controller and the flash itself is at a different location. So its best to have some link instead.

nvlsianpu · 2017-08-09T07:34:12Z

flash0: flash@0 {
				reg = <0x00000000 DT_FLASH_SIZE>;
                                write-block-size = <4>;
			};

flash-controller@0x4001E000 {
			compatible = "nxp,nrf52-flash-controller";
			reg = <0x4001E000 0x550>;

			#address-cells = <1>;
			#size-cells = <1>;

                       flash = <&flash0>;
		};

@galak Is this comply with what you suggested?

galak · 2017-08-09T18:25:34Z

@nvlsianpu looking at this some more and feeling like we should just support ranges properly and place the flash node under the controller.

nvlsianpu · 2017-08-23T09:17:12Z

@galak I've updated the patch. I used flash constrain for bind dtsi description to yaml script.

carlescufi · 2017-08-23T10:20:29Z

@agross-linaro mind taking a look at the updated patch? Thanks!

galak · 2017-08-23T13:45:21Z

dts/arm/nordic/nrf52840.dtsi

@@ -13,8 +13,18 @@
 		};
 	};

-	flash0: flash@0 {
-		reg = <0x00000000 DT_FLASH_SIZE>;
+	flash-controller@0x4001E000 {


nit, remove the 0x in the node name. Should be: flash-controller@4001E000

galak · 2017-08-23T13:45:56Z

dts/arm/nordic/nrf52840.dtsi

+			#address-cells = <1>;
+			#size-cells = <1>;
+
+			flash0: flash {


flash0: flash@0 {

galak

Some minor nits on the dts changes. We need more description on what 'write-block-size' means.

galak · 2017-08-23T13:46:59Z

dts/common/yaml/flash.yaml

+
+  - write-block-size:
+     type: int
+     description: flash write alignment


Can you be more descriptive here.

galak · 2017-08-23T13:50:39Z

dts/arm/nordic/nrf52840.dtsi

+			flash0: flash {
+				compatible = "flash";
+				reg = <0x00000000 DT_FLASH_SIZE>;
+				write-block-size = <4>;


Curious where you expect to end up using write-block-size in actual code. I wonder if such properties belong up with the flash-controller node instead.

e.g. mcuboot, and mcuboot interface (#747) needs to know this value. For me It doesn't mater whether this is flash-controller's property value or flash memory's property (anyway these entities always existing together). But in my opinion flash is right node for this property.

galak · 2017-08-23T13:51:28Z

dts/arm/nordic/nrf52840.dtsi

+			#size-cells = <1>;
+
+			flash0: flash {
+				compatible = "flash";


I think we need a better compatible to describe this class of flash device.

galak · 2017-08-23T15:11:39Z

dts/common/yaml/flash.yaml

@@ -0,0 +1,26 @@
+---
+title: Flash base node description
+id: soc-nv-flash


rename the file from flash.yaml to 'soc-nv-flash.yaml'

Added flash-controller description and moved flash description to it (for coherence). Added property for description of the flash alignment required by write operations. Thanks to that l-value FLASH_WRITE_BLOCK_SIZE macro will be generated. It is useful for any component uses the flash. Signed-off-by: Andrzej Puzdrowski <andrzej.puzdrowski@nordicsemi.no>

nvlsianpu · 2017-08-25T13:47:49Z

@galak , @d3zd3z poke

carlescufi · 2017-08-29T12:07:03Z

@galak @d3zd3z could you take a look at your convenience?

carlescufi · 2017-09-07T19:50:20Z

@galak this has been in review for a while, can you take a look?

The `--rimage-tool=...` parameter was added in March 2022 by the very first map.yml commit (rebased f6d6f1e15022) But `west sign` has been invoked by `west build` (through CMake) since commit fad2da3, almost one year ago. At the time, the ability to sign from west flash was preserved for backwards compatibility. Today, `west flash` does not invoke `west sign` any more which means this `--rimage-tool=...` parameter is now ignored. The CI for more recent, ACE platforms (MTL, LNL, etc.) does not pass any `--rimage-tool=...` at all, see evidence in CI runs of zephyrproject-rtos#860 Removing `--rimage-tool=...` from `.github/data/map.yml` will allow blocking that option in `west flash` (zephyrproject-rtos#860) Now that Zephyr 3.5 has been released, we need to reduce the number of rimage use cases and the corresponding validation complexity and maintenance workload to simplify and accelerate new features like splitting rimage configuration files (zephyrproject-rtos#65411) Signed-off-by: Marc Herbert <marc.herbert@intel.com>

nvlsianpu mentioned this pull request Jul 20, 2017

Preview: storage & boot API for Image manager #747

Merged

galak requested changes Jul 20, 2017

View reviewed changes

galak self-assigned this Jul 20, 2017

carlescufi requested a review from agross-oss August 8, 2017 10:38

nvlsianpu force-pushed the flash_alignment_desc branch 3 times, most recently from e6f560e to 03f2f23 Compare August 23, 2017 08:33

galak reviewed Aug 23, 2017

View reviewed changes

dts/arm/nordic/nrf52840.dtsi Outdated

#address-cells = <1>;

#size-cells = <1>;

flash0: flash {

Copy link

Collaborator

galak Aug 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

flash0: flash@0 {

galak requested changes Aug 23, 2017

View reviewed changes

nvlsianpu force-pushed the flash_alignment_desc branch from 03f2f23 to bcf9058 Compare August 23, 2017 15:07

galak reviewed Aug 23, 2017

View reviewed changes

nvlsianpu force-pushed the flash_alignment_desc branch from bcf9058 to ba26ccf Compare August 23, 2017 15:21

jfischer-no mentioned this pull request Aug 23, 2017

RFC: usb enhancements, composite device, common device descriptor #716

Closed

8 tasks

galak requested a review from d3zd3z August 24, 2017 13:31

carlescufi added the Feature A planned feature with a milestone label Sep 5, 2017

carlescufi added this to the v1.10 milestone Sep 5, 2017

nashif approved these changes Sep 11, 2017

View reviewed changes

carlescufi approved these changes Sep 11, 2017

View reviewed changes

nashif merged commit d868a0b into zephyrproject-rtos:master Sep 11, 2017

nashif modified the milestones: v1.10, v1.10.0 Oct 3, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DTS: flash alignment description #860

DTS: flash alignment description #860

nvlsianpu commented Jul 20, 2017 •

edited

Loading

galak left a comment

nvlsianpu commented Jul 20, 2017

galak commented Jul 20, 2017

nvlsianpu commented Jul 20, 2017

d3zd3z commented Jul 20, 2017

galak commented Jul 20, 2017

mbolivar commented Jul 20, 2017

galak commented Jul 21, 2017

mbolivar commented Jul 21, 2017 •

edited

Loading

galak commented Aug 3, 2017

nvlsianpu commented Aug 7, 2017

carlescufi commented Aug 8, 2017

carlescufi commented Aug 8, 2017

agross-oss commented Aug 8, 2017

galak commented Aug 8, 2017

nvlsianpu commented Aug 9, 2017

galak commented Aug 9, 2017

nvlsianpu commented Aug 23, 2017

carlescufi commented Aug 23, 2017

galak Aug 23, 2017

galak Aug 23, 2017

galak left a comment

galak Aug 23, 2017

galak Aug 23, 2017

nvlsianpu Aug 23, 2017

galak Aug 23, 2017

galak Aug 23, 2017

nvlsianpu commented Aug 25, 2017

carlescufi commented Aug 29, 2017

carlescufi commented Sep 7, 2017

DTS: flash alignment description #860

DTS: flash alignment description #860

Conversation

nvlsianpu commented Jul 20, 2017 • edited Loading

galak left a comment

Choose a reason for hiding this comment

nvlsianpu commented Jul 20, 2017

galak commented Jul 20, 2017

nvlsianpu commented Jul 20, 2017

d3zd3z commented Jul 20, 2017

galak commented Jul 20, 2017

mbolivar commented Jul 20, 2017

galak commented Jul 21, 2017

mbolivar commented Jul 21, 2017 • edited Loading

galak commented Aug 3, 2017

nvlsianpu commented Aug 7, 2017

carlescufi commented Aug 8, 2017

carlescufi commented Aug 8, 2017

agross-oss commented Aug 8, 2017

galak commented Aug 8, 2017

nvlsianpu commented Aug 9, 2017

galak commented Aug 9, 2017

nvlsianpu commented Aug 23, 2017

carlescufi commented Aug 23, 2017

galak Aug 23, 2017

Choose a reason for hiding this comment

galak Aug 23, 2017

Choose a reason for hiding this comment

galak left a comment

Choose a reason for hiding this comment

galak Aug 23, 2017

Choose a reason for hiding this comment

galak Aug 23, 2017

Choose a reason for hiding this comment

nvlsianpu Aug 23, 2017

Choose a reason for hiding this comment

galak Aug 23, 2017

Choose a reason for hiding this comment

galak Aug 23, 2017

Choose a reason for hiding this comment

nvlsianpu commented Aug 25, 2017

carlescufi commented Aug 29, 2017

carlescufi commented Sep 7, 2017

nvlsianpu commented Jul 20, 2017 •

edited

Loading

mbolivar commented Jul 21, 2017 •

edited

Loading