DT_FLASH_AREA replacement with new dts macros #25056

galak · 2020-05-07T05:39:00Z

No description provided.

zephyrbot · 2020-05-07T05:41:49Z

All checks are passing now.

checkpatch (informational only, not a failure)

-:774: WARNING:CONSTANT_COMPARISON: Comparisons should place the constant on the right side of the test
#774: FILE: tests/deprecated/dts/src/main.c:13:
+	BUILD_ASSERT(DT_FLASH_AREA_MCUBOOT_ID == FLASH_AREA_ID(mcuboot),

-:777: WARNING:CONSTANT_COMPARISON: Comparisons should place the constant on the right side of the test
#777: FILE: tests/deprecated/dts/src/main.c:16:
+	BUILD_ASSERT(DT_FLASH_AREA_STORAGE_ID == FLASH_AREA_ID(storage),

-:779: WARNING:CONSTANT_COMPARISON: Comparisons should place the constant on the right side of the test
#779: FILE: tests/deprecated/dts/src/main.c:18:
+	BUILD_ASSERT(DT_FLASH_AREA_IMAGE_0_ID == FLASH_AREA_ID(image_0),

-:781: WARNING:CONSTANT_COMPARISON: Comparisons should place the constant on the right side of the test
#781: FILE: tests/deprecated/dts/src/main.c:20:
+	BUILD_ASSERT(DT_FLASH_AREA_IMAGE_1_ID == FLASH_AREA_ID(image_1),

- total: 0 errors, 4 warnings, 912 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

Your patch has style problems, please review.

NOTE: Ignored message types: AVOID_EXTERNS BRACES CONFIG_EXPERIMENTAL CONST_STRUCT DATE_TIME FILE_PATH_CHANGES MINMAX NETWORKING_BLOCK_COMMENT_STYLE PRINTK_WITHOUT_KERN_LEVEL SPDX_LICENSE_TAG SPLIT_STRING VOLATILE

NOTE: If any of the errors are false positives, please report
      them to the maintainers.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

galak · 2020-05-07T05:42:00Z

This is based off of #24522.

There's a known issue with samples/subsys/fs/littlefs on nrf52840dk_nrf52840 failing. This is because the nrf52840dk_nrf52840 has 2 fixed-partition nodes. We need to decide if partitions should be seen as per flash device or more global.

pabigot · 2020-05-07T10:48:57Z

There's a known issue with samples/subsys/fs/littlefs on nrf52840dk_nrf52840 failing.

I wasn't aware of that issue. That's a very ugly failure message.

This is because the nrf52840dk_nrf52840 has 2 fixed-partition nodes. We need to decide if partitions should be seen as per flash device or more global.

Aha. Now I finally see what may be underlying the discussion at #24522 (comment)

Flash partitions identified by label property should be global; i.e. the name should be sufficient to identify the containing device and region within the device. If the numeric ID is local to the containing device it shouldn't matter.

Edit: The upstream binding seems to support my requirement above.

de-nordic · 2020-05-07T12:54:26Z

There's a known issue with samples/subsys/fs/littlefs on nrf52840dk_nrf52840 failing.

I have seen that in original PR for FLASH_AREA. I have posted workaround PR here:
#25062

Setup node.compats right after we create the Node. This allows access to the compats information in _bus_node. Signed-off-by: Kumar Gala <[email protected]>

If we have a fixed-partition on a flash device that is for example on a spi controller we will not get a binding match currently. This is because we expect a match between both the compatible and the fact that fixed-partition node is a decendant of the spi bus. To address this we treat fixed-partitions as if they are on no bus. This has the effect of causing a binding match as well as ensuring that when we process the fixed-partition node we will do anything special to it because of the bus it happens to be under (for example SPI CS_GPIO processing). Signed-off-by: Kumar Gala <[email protected]>

allow the old generator to act as it did. Signed-off-by: Kumar Gala <[email protected]>

Rework how write_child_functions to match how we do the code for DT_FOREACH_OKAY_INST. Signed-off-by: Kumar Gala <[email protected]>

Add DT_NODE_BY_FIXED_PARTITION_LABEL that given a "label" in any fixed-partitions map will return the node_id for that partition node. Add DT_NODE_HAS_FIXED_PARTITION_LABEL that will test if a given fixed-partitions "label" is valid. Add DT_FIXED_PARTITION_ID that will return an unique ordinal value for the partition give a node_id to the partition. Signed-off-by: Kumar Gala <[email protected]>

Add macros that we'll utilize instead of DT_FLASH_AREA_ Signed-off-by: Kumar Gala <[email protected]>

Convert to use new DTS macros to populate default flash map Signed-off-by: Kumar Gala <[email protected]>

Convert with a combo of scripts and by hand fixups: git grep -l DT_FLASH_AREA_.*_ID | \ xargs sed -i -r 's/DT_FLASH_AREA_(.*)_ID/FLASH_AREA_ID(\L\1)/' git grep -l DT_FLASH_AREA_.*_OFFSET | \ xargs sed -i -r 's/DT_FLASH_AREA_(.*)_OFFSET/FLASH_AREA_OFFSET(\L\1)/' git grep -l DT_FLASH_AREA_.*_SIZE | \ xargs sed -i -r 's/DT_FLASH_AREA_(.*)_SIZE/FLASH_AREA_SIZE(\L\1)/' Signed-off-by: Kumar Gala <[email protected]>

hakonfam · 2020-05-12T10:13:24Z

include/storage/flash_map.h

We should allow users to provide their own definition of these symbols. This could be the default definition included, but (like with flash_map_default.c) it should be possible to override this through a kconfig value.

Currently any dowstream with their own partition definitions is required to maintain a patch of this file.

My initial thoughts for solving this were to in the future extend this macro to take a second parameter identifying the provider of partition information. The default behavior would be as if that parameter were fixed_partition which would through magic pass to the current solution, but if it were say partition_mgr would route to a different macro or possibly ultimately a function.

The thought is that we do need label uniqueness within a given provider of flash region managers. Attempting to enforce it across all region managers isn't reasonable IMO, and limiting applications to a single region manager (devicetree or other) is also not reasonable.

A way to resolve the conflict is to pair the label with a provider, using the existing devicetree provider as the default.

This was consulted with me. @mbolivar-nordic This is the simple way we can inject our partitions info.

OK, then there must be something like a kconfig option passed in as the second argument to avoid having to patch the file.

I'm fine with this as long as it allows us to not need patches in any of the upstream source files.

nvlsianpu · 2020-05-12T13:19:26Z

west.yml

upgrade sha

Update mcumgr/mcuboot for changes to DT_FLASH_AREA -> FLASH_AREA macros. Signed-off-by: Kumar Gala <[email protected]>

Set LEGACY_DEVICETREE_MACROS to default to no since we are deprecating the old macro style and all in tree users are now converted. Signed-off-by: Kumar Gala <[email protected]>

This this covers two small aspects of DTS functionality: first ensuring the legacy generation script continues to generally function and second that FLASH_AREA id's are the same between the old and new generator. This test will be removed when the legacy generator is removed. Signed-off-by: Kumar Gala <[email protected]>

zephyrbot added area: Settings Settings subsystem area: File System area: Storage Storage subsystem area: API Changes to public APIs area: Samples Samples area: Tests Issues related to a particular existing or missing test area: Documentation labels May 7, 2020

galak requested review from hakonfam, mbolivar-nordic and pabigot May 7, 2020 05:42

galak added the area: Devicetree label May 7, 2020

mbolivar-nordic requested a review from de-nordic May 7, 2020 05:45

galak force-pushed the kumar-dt-flash branch from e3e428c to 57424da Compare May 7, 2020 05:47

galak requested a review from nvlsianpu May 7, 2020 05:48

galak force-pushed the kumar-dt-flash branch from 57424da to 215d836 Compare May 7, 2020 11:31

galak force-pushed the kumar-dt-flash branch from 215d836 to 612f47a Compare May 7, 2020 22:59

galak marked this pull request as ready for review May 7, 2020 22:59

galak requested review from albertofloyd, anangl, bwitherspoon, carlescufi, dcpleung, erwango, finikorg, franciscomunoz and henrikbrixandersen as code owners May 7, 2020 22:59

carlescufi requested review from de-nordic and removed request for alexanderwachter, andrewboie, nandojve and pgielda May 11, 2020 14:41

galak added 8 commits May 11, 2020 20:49

edtlib: Initialize node.compats earlier

e289052

Setup node.compats right after we create the Node. This allows access to the compats information in _bus_node. Signed-off-by: Kumar Gala <[email protected]>

edtlib: add a flag for old fixed-partitions behavior

18ec88c

allow the old generator to act as it did. Signed-off-by: Kumar Gala <[email protected]>

gen_defines: Rework implementation of write_child_functions

ba3be40

Rework how write_child_functions to match how we do the code for DT_FOREACH_OKAY_INST. Signed-off-by: Kumar Gala <[email protected]>

storage: flash_map: Add macros to get FLASH_AREA info

1c27fcf

Add macros that we'll utilize instead of DT_FLASH_AREA_ Signed-off-by: Kumar Gala <[email protected]>

storage: flash_map_default: Convert to new dts macros

5ab6218

Convert to use new DTS macros to populate default flash map Signed-off-by: Kumar Gala <[email protected]>

galak force-pushed the kumar-dt-flash branch from 5e31fef to 10d6b3d Compare May 12, 2020 01:51

hakonfam requested changes May 12, 2020

View reviewed changes

nvlsianpu requested review from mbolivar-nordic and nvlsianpu May 12, 2020 10:20

hakonfam approved these changes May 12, 2020

View reviewed changes

nvlsianpu approved these changes May 12, 2020

View reviewed changes

nvlsianpu mentioned this pull request May 12, 2020

zephyr: Change DT_FLASH_AREA_ macros to FLASH_AREA_ mcu-tools/mcuboot#734

Merged

nvlsianpu reviewed May 12, 2020

View reviewed changes

west.yml Outdated

Copy link

Contributor

nvlsianpu May 12, 2020

Choose a reason for hiding this comment

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

upgrade sha

galak added 3 commits May 12, 2020 08:24

west.yml: Update mcumgr/mcuboot to pickup changes to match new dts

80f6478

Update mcumgr/mcuboot for changes to DT_FLASH_AREA -> FLASH_AREA macros. Signed-off-by: Kumar Gala <[email protected]>

Kconfig: Default to 'n' for LEGACY_DEVICETREE_MACROS

a84ba4f

Set LEGACY_DEVICETREE_MACROS to default to no since we are deprecating the old macro style and all in tree users are now converted. Signed-off-by: Kumar Gala <[email protected]>

galak force-pushed the kumar-dt-flash branch from 10d6b3d to 1b131eb Compare May 12, 2020 13:25

galak removed the DNM This PR should not be merged (Do Not Merge) label May 12, 2020

mbolivar-nordic approved these changes May 12, 2020

View reviewed changes

carlescufi approved these changes May 13, 2020

View reviewed changes

carlescufi merged commit 9fa3048 into zephyrproject-rtos:master May 13, 2020

otavio mentioned this pull request May 15, 2020

lib: updatehub: Add missing include for FLASH_AREA #25333

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

DT_FLASH_AREA replacement with new dts macros #25056

DT_FLASH_AREA replacement with new dts macros #25056

Uh oh!

galak commented May 7, 2020

Uh oh!

zephyrbot commented May 7, 2020 •

edited

Loading

Uh oh!

galak commented May 7, 2020

Uh oh!

pabigot commented May 7, 2020 •

edited

Loading

Uh oh!

de-nordic commented May 7, 2020

Uh oh!

hakonfam May 12, 2020

Uh oh!

pabigot May 12, 2020

Uh oh!

nvlsianpu May 12, 2020 •

edited by hakonfam

Loading

Uh oh!

hakonfam May 12, 2020

Uh oh!

nvlsianpu May 12, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

DT_FLASH_AREA replacement with new dts macros #25056

DT_FLASH_AREA replacement with new dts macros #25056

Uh oh!

Conversation

galak commented May 7, 2020

Uh oh!

zephyrbot commented May 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

checkpatch (informational only, not a failure)

Uh oh!

galak commented May 7, 2020

Uh oh!

pabigot commented May 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

de-nordic commented May 7, 2020

Uh oh!

hakonfam May 12, 2020

Choose a reason for hiding this comment

Uh oh!

pabigot May 12, 2020

Choose a reason for hiding this comment

Uh oh!

nvlsianpu May 12, 2020 • edited by hakonfam Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hakonfam May 12, 2020

Choose a reason for hiding this comment

Uh oh!

nvlsianpu May 12, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

zephyrbot commented May 7, 2020 •

edited

Loading

pabigot commented May 7, 2020 •

edited

Loading

nvlsianpu May 12, 2020 •

edited by hakonfam

Loading