flash_map: generate content from DTS completely.

[findlayfeng]

This patch add functionality for extract complete flash map content form DTS. This makes DTS proper interface for providing partitioning information without hard-coding flash areas of partitions.

Changes also allow to declare partition nodes inside embedded and external flash memory simultaneously.

Before this PR flash map configuration requires both customized flash map c-structure and preparation of DTS description.

TODO:

• [x] automatic generation of the list of flash devices from dts
• [x] automatic generation of the list of flash area from dts
• [x] align DFU support modules
  • [x] Mcuboot support
  • [x] Flash img support
  • [x] Update zephyr img mgmt port
  • [x] Update usb dfu
  • [x] Update dfu tests
• [x] patch for mcuboot itself https://github.com/JuulLabs-OSS/mcuboot/pull/405
• [x] patch for mcumgr upstream https://github.com/apache/mynewt-mcumgr/pull/20

[codecov-io]

Codecov Report

Merging #8837 into master will increase coverage by 0.12%. The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #8837      +/-   ##
==========================================
+ Coverage   48.74%   48.86%   +0.12%     
==========================================
  Files         317      317              
  Lines       46553    46569      +16     
  Branches    10743    10743              
==========================================
+ Hits        22690    22758      +68     
+ Misses      19351    19267      -84     
- Partials     4512     4544      +32

Impacted Files	Coverage Δ
subsys/net/l2/ethernet/ethernet.c	53.07% <0%> (-0.85%)	:arrow_down:
subsys/net/ip/ipv6.c	56.57% <0%> (-0.8%)	:arrow_down:
subsys/net/ip/net_core.c	62.11% <0%> (-0.63%)	:arrow_down:
subsys/net/ip/utils.c	77.13% <0%> (-0.43%)	:arrow_down:
kernel/init.c	73.43% <0%> (ø)	:arrow_up:
subsys/net/lib/mqtt/mqtt_transport.c	0% <0%> (ø)	:arrow_up:
include/net/net_context.h	73.33% <0%> (ø)	:arrow_up:
subsys/net/ip/net_context.c	59.03% <0%> (+0.05%)	:arrow_up:
subsys/net/ip/net_pkt.c	65.4% <0%> (+0.07%)	:arrow_up:
subsys/net/ip/net_if.c	61.04% <0%> (+0.64%)	:arrow_up:
... and 2 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 36e437f...0fe58aa. Read the comment docs.

[carlescufi]

@nvlsianpu can you take a look?

[nvlsianpu]

@findlayfeng I will provide more comments soon. Nice to see you are still active. Can you rebase this PR onto newest master (and resolve conflicts)? - so I will be able to test it wit newest master.

I have also the question - whether this PR fulfill flowing objective?: "provide information about flash drivers for each image slot (using flash_map subsystem as well). We want to run mcuboot and DFU along with external serial flash. So far reference to the flash driver was hardcoded."

[findlayfeng]

In fact, this is exactly what I am doing. The work on mcuboot has been completed and has not been submitted because the new api has been referenced and the new api has not yet been merged.

[nvlsianpu]

In fact, this is exactly what I am doing. The work on mcuboot has been completed and has not been submitted because the new api has been referenced and the new api has not yet been merged.

Great - I was almost sure thet it does that.

[carlescufi]

Normally proces said that code from ext folder should be added to appropriate external project 1st. In this case changes are atomic (zephyr changes immediately required mcumgr changes), so I would update external project once this PR will be merged. @carlescufi can we do it in such a manner?

But can't we first merge the changes into the external mcumgr repo, and then here? If we don't then we have no way to refer to a SHA or revision we are importing from.

[nvlsianpu]

@carlescufi OK - I would update external project once this PR changes will be approved. Then we can update this PR and merge it immediately. This will be a "transaction".

[carlescufi]

@nvlsianpu sounds good. We should still rebase this first.

[nvlsianpu]

@findlayfeng any chance for update this PR?

[findlayfeng]

@nvlsianpu Today is the last day of the Mid-Autumn Festival holiday in China. Rebase has been completed I am going to sleep now, I will deal with this PR tomorrow.

[nvlsianpu]

Thanks for rebase and fixes - I just have been starting to test how this works (well so far :D).

[nvlsianpu]

@findlayfeng can you put a working example of external flash-controller declaration. I can't build mine with success. here what I have put to overlay file for nrf52840_pca10056:

/ {
    flash-controller2@7001E000 {
            compatible = "turbo,turbo-flash-controller";
            reg = <0x7001E000 0x550>;

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

            label="EXT_FLASH_DRV_NAME";

            flash1: flash@0 {
                compatible = "serial-flash";
                label = "SERIAL_FLASH";
                erase-block-size = <4096>;
                write-block-size = <4>;
            };
    };
};

&flash1 {
    /*
     * For more information, see:
     * http://docs.zephyrproject.org/latest/devices/dts/flash_partitions.html
     */
    partitions {
        compatible = "fixed-partitions";
        #address-cells = <1>;
        #size-cells = <1>;

        external_partition: partition@0 {
            label = "external";
            reg = <0x000000000 0x00002000>;
        };
    };
};

BTW - I had added binding for extract turbo-flash-controlle as well.

[findlayfeng]

/ {
    model = "xxxx";
    compatible = "nordic,nrf51822";

    chosen {
        zephyr,flash = &flash0;
        zephyr,flash_map = &flash0, &flash1;
    };

    flash1: serial-flash@0 {
        compatible = "serial-flash";
        reg = <0 DT_FLASH_SIZE>;
        label = "FLASH_1";

        partitions {
            compatible = "fixed-partitions";
            #address-cells = <1>;
            #size-cells = <1>;

            slot1_partition: partition@0 {
                label = "image-1";
                reg = <0x00000000 0x30000>;
            };

            storage_partition: partition@30000 {
                label = "storage";
                reg = <0x00030000 0>;
            };
        };
    };

};

&flash0 {
    /*
     * For more information, see:
     * http://docs.zephyrproject.org/devices/dts/flash_partitions.html
     */
    partitions {
        compatible = "fixed-partitions";
        #address-cells = <1>;
        #size-cells = <1>;

        boot_partition: partition@0 {
            label = "mcuboot";
            reg = <0x00000000 0xC000>;
        };
        slot0_partition: partition@C000 {
            label = "image-0";
            reg = <0x0000C000 0x30000>;
        };

        scratch_partition: partition@3c000 {
            label = "image-scratch";
            reg = <0x0003c000 0x4000>;
        };

    };
};

[findlayfeng]

Important is

chosen {
        zephyr,flash = &flash0;
        zephyr,flash_map = &flash0, &flash1;
    };

If zephyr, flash_map has no value Automatic use of flash So the old configuration does not need to be modified

Here is just a reference to the device based on label The definition of the controller I think is the change related to flash

[nvlsianpu]

@findlayfeng I see now that this PR introduce outstanding functionality required for all external flash devices support (SPI, QSPI. I2C and parallel as well). There is nothing very specific for only on of these interfaces. So I recommend to change terms used elsewhere (as keyword etc.) in this PR from SPI/Serial to External. for example: compatible = "serialexternal-flash";

cc @galak ^^

[findlayfeng]

@nvlsianpu I don't think so, because the direction that follows should be a bus-based device configuration. Just like rtc Based on this, I think the current description is appropriate. Dependent on the partition tag instead of "serial-flash" or something else If you want to introduce devices from other buses, I think you should increase the configuration.

[nvlsianpu]

@findlayfeng So your thought is that this functionality is dedicated to seria flash devices, right?

[findlayfeng]

@nvlsianpu I mean you can add a dts configuration file like dts/bindings/mtd/parallel-flash.yaml Because I am thinking about adding !include spi-device.yaml to dts/bindings/mtd/serial-flash.yaml

[nvlsianpu]

@erwango Use the last commit form my fork https://github.com/nvlsianpu/zephyr/tree/fix_flash_map_sandbox for generated flash_map test.

[nvlsianpu]

Seems interesting but it would be great to have a way to test this for the review.

@erwango Can you elaborate more - Did you think about example which lists each flash_areas with it's properties?

[galak]

Is there an example of what the DT is suppose to look like? Not sure I follow what's this is trying to do with generating a list of flash devices, etc.

[findlayfeng]

@galak This PR is not generating a list of flash devices, Is to give a list of flash devices and scan the defined partitions on these devices

[nvlsianpu]

@findlayfeng I can send example of DT description requested by @galak onto your commits - It might help with further discussion. Can I?

[findlayfeng]

@findlayfeng I can send example of DT description requested by @galak onto your commits - It might help with further discussion. Can I?

yep

[nvlsianpu]

@findlayfeng can you act for this PR resolution. For instance you can prepare a expected by this patch external memory DT description for a board (for instance nrf52840_pca10056 has SPI/qspi die on the pcb)?

[findlayfeng]

@nvlsianpu The board I use is not a development board. I need to prepare a board that is in the boards.

[findlayfeng]

Dt has changed a bit, I need to confirm the change

[carlescufi]

@findlayfeng have you added QSPI commits here by mistake?

[findlayfeng]

Is an example that relies on qspi's flash @carlescufi

[nvlsianpu]

@findlayfeng Thanks for adding the qspi example! For sure it is required (by the contribution process itself) to isolate changes. So please remove most QSPI code form here and keep it exclusively in #11278.

For presentation of this PR contribution I would keep here only changes you added in files: dts/arm/nordic/nrf52840.dtsi dts/bindings/flash_controller/nordic,nrf-qspi-flash-controller.yaml soc/arm/nordic_nrf/nrf52/dts_fixup.h so only these required to generate DT structures for external controller and its qspi memory and c- macros - this will show what is purpose precisely.

[findlayfeng]

@nvlsianpu Sorry, I am doing other work recently. What do you mean by splitting PR so that they are no longer dependent on it? My original idea was to make #11278 first merged. If you remove the example, the first commit (64707cc) is also the common dependency of the two PRs.

[findlayfeng]

@nvlsianpu Update example using an area of soc flash ^_^

[nvlsianpu]

I removed commit samples: flash_map: add sample for nrf52840_pca10056 as it was not my intention to ask about it. I keep in backup here: https://github.com/nvlsianpu/zephyr/tree/fix_flash_map.

[zephyrbot]

All checks are passing now.

Review history of this comment for details about previous failed status. Note that some checks might have not completed yet.

[nvlsianpu]

@lemrey Can you test this along with mcuboot? On pca10040 or pca10056. Looks like PR is ready I'm on vacation" so can't test this.

[lemrey]

@nvlsianpu sure, I can check this out today.

[lemrey]

@findlayfeng @nvlsianpu there is an issue with the slots: in loader.c : <wrn> mcuboot: Cannot upgrade: slots are not compatible Tested on both nrf52840_pca10059 and nrf52_pca10040, I might investigate further today.

[findlayfeng]

Mcuboot needs some modifications

[nvlsianpu]

Works fine for me. So strange.

@findlayfeng

Mcuboot needs some modifications

What changes?

[nvlsianpu]

^^ we discovered than on my build machne we have got in zephyr/include/generated/generated_dts_board.h

#define FLASH_AREA_1        IMAGE_0
#define FLASH_AREA_2        IMAGE_1

while on @lemrey machine he has got

#define FLASH_AREA_0        IMAGE_0 
#define FLASH_AREA_4        IMAGE_1

Question is why this behaves diferently.

[nvlsianpu]

mcuboot should work with https://github.com/nvlsianpu/mcuboot/tree/allow-zephyr-dynamic-flash_map

[findlayfeng]

^^ we discovered than on my build machne we have got in zephyr/include/generated/generated_dts_board.h

#define FLASH_AREA_1      IMAGE_0
#define FLASH_AREA_2      IMAGE_1

while on @lemrey machine he has got

#define FLASH_AREA_0        IMAGE_0 
#define FLASH_AREA_4        IMAGE_1

Question is why this behaves diferently.

The numbers are assigned sequentially, Decided on the script to find the order of this area, Therefore, you should not use a fixed value when using it.

[nvlsianpu]

@lemrey @carlescufi @nashif @findlayfeng @ioannisg I tweaks this PR by edit commit messages. I tested it against mcuboot successfully as well. Also opened required PR to mcuboot and mcumgr up-steram.

Pleas review in your convenience.

[lemrey]

@nvlsianpu please double-check serial recovery.

[nvlsianpu]

@lemrey Thanks. I investigated and spot that mcuboot needs some more tweaks to be compatible with this patch. Need to rework flash_area_sector_from_off function.

[carlescufi]

@lemrey @nvlsianpu @erwango Is this ready go in?

[nvlsianpu]

@lemrey Can you send me your generated_dts_board.h file?

[nvlsianpu]

^^ pleas also check whether disabling progressive erasing mitigates the issue.

[lemrey]

The MCUBoot hex image generated on my machine works on @nvlsianpu and not mine. We suspect it is related to my setup, given that this also happens on master for me; it'd be great though if somebody else could check this.

[nvlsianpu]

High level idea is to allow partitions in external flash storage and embedded simultaneously. This is step required by suport external memory for mcuboot for instance.