DTS: CONFIG_FLASH_BASE_ADDRESS not being generated for SPI based Flash chip

[rgundi]

CONFIG_FLASH_BASE_ADDRESS is not being generated for SPI based Flash chips. It was being generated earlier but looks like one of the recent commits around the DTS for Flash chip has removed this. This is very much required for all Flash chips coz code for DFU uses this in multiple places.

Please reintroduce this for SPI based Flash chips too. It was anyway 0 and hence it should be ok.

[galak]

Can you be more specific or provide a test case in which this isn't happening?

I'm guessing this is on intel_s1000_crb.

[rgundi]

That's correct. Please try building samples/subsys/usb/dfu/ for intel_s1000_crb. This would require XCC tools and hence you may not be able to build. Below is the error message that you get upon building.

/home/rgundi/RajZephyr/zephyr/subsys/usb/class/usb_dfu.c:301: error: ‘CONFIG_FLASH_BASE_ADDRESS’ undeclared here (not in a function)
zephyr/CMakeFiles/zephyr.dir/build.make:453: recipe for target 'zephyr/CMakeFiles/zephyr.dir/subsys/usb/class/usb_dfu.c.obj' failed
make[2]: *** [zephyr/CMakeFiles/zephyr.dir/subsys/usb/class/usb_dfu.c.obj] Error 2
CMakeFiles/Makefile2:540: recipe for target 'zephyr/CMakeFiles/zephyr.dir/all' failed
make[1]: *** [zephyr/CMakeFiles/zephyr.dir/all] Error 2
Makefile:83: recipe for target 'all' failed
make: *** [all] Error 2

[galak]

What would the CONFIG_FLASH_BASE_ADDRESS be for the S1000 as the flash isn't memory map accessible from my understanding on the intel_s1000_crb.

[rgundi]

Since CONFIG_FLASH_BASE_ADDRESS is not applicable for S1000 Flash, i would keep it as 0 to ensure the existing things are not broken.

[galak]

Since CONFIG_FLASH_BASE_ADDRESS is not applicable for S1000 Flash, i would keep it as 0 to ensure the existing things are not broken.

I think having it as 0, means we are hiding issues. 0 is a valid value for CONFIG_FLASH_BASE_ADDRESS, not having the symbol defined is the correct behavior.

[rgundi]

Since CONFIG_FLASH_BASE_ADDRESS is not applicable for S1000 Flash, i would keep it as 0 to ensure the existing things are not broken.

I think having it as 0, means we are hiding issues. 0 is a valid value for CONFIG_FLASH_BASE_ADDRESS, not having the symbol defined is the correct behavior.

No, i don't think so. It really depends upon how CONFIG_FLASH_BASE_ADDRESS is used in the code. For the purpose it is used in usb_dfu.c, having it defined to 0 is the right behavior.

[galak]

No, i don't think so. It really depends upon how CONFIG_FLASH_BASE_ADDRESS is used in the code. For the purpose it is used in usb_dfu.c, having it defined to 0 is the right behavior.

Than I think the usb_dfu.c should be updated to handle CONFIG_FLASH_BASE_ADDRESS not being set.

[galak]

Closing this issue, as stated the usb_dfu.c could should be fixed if the fact that CONFIG_FLASH_BASE_ADDRESS is not defined is an issue. It should not assume a default value exists for flash's that are not memory map accessible.

[rgundi]

I do not quite agree with this, Kumar. 0 is a valid base address and i do not see why it can be an issue if you assume the base address is 0 for a SPI based flash. It definitely doesn't break anything and keeps the code generic (for memory mapped and non-memory mapped flash).

[nashif]

something like that?

diff --git a/subsys/usb/class/usb_dfu.c b/subsys/usb/class/usb_dfu.c
index 22f70a4b69..d37bfe3eda 100644
--- a/subsys/usb/class/usb_dfu.c
+++ b/subsys/usb/class/usb_dfu.c
@@ -63,6 +63,13 @@ LOG_MODULE_REGISTER(usb_dfu);
 #define USB_DFU_MAX_XFER_SIZE          CONFIG_USB_DFU_MAX_XFER_SIZE
 #endif

+
+#ifndef CONFIG_FLASH_BASE_ADDRESS
+#define FLASH_BASE_ADDRESS 0
+#else
+#define FLASH_BASE_ADDRESS CONFIG_FLASH_BASE_ADDRESS
+#endif
+
 static struct k_work dfu_work;

 struct dfu_worker_data_t {
@@ -298,7 +305,7 @@ struct dfu_data_t {
 static struct dfu_data_t dfu_data = {
        .state = appIDLE,
        .status = statusOK,
-       .flash_addr = CONFIG_FLASH_BASE_ADDRESS + FLASH_AREA_IMAGE_1_OFFSET,
+       .flash_addr = FLASH_BASE_ADDRESS + FLASH_AREA_IMAGE_1_OFFSET,
        .flash_upload_size = FLASH_AREA_IMAGE_1_SIZE,
        .alt_setting = 0,
 };
@@ -432,7 +439,7 @@ static int dfu_class_handle_req(struct usb_setup_packet *pSetup,
                case dfuIDLE:
                        LOG_DBG("DFU_DNLOAD start");
                        dfu_reset_counters();
-                       if (dfu_data.flash_addr != CONFIG_FLASH_BASE_ADDRESS
+                       if (dfu_data.flash_addr != FLASH_BASE_ADDRESS
                                                + FLASH_AREA_IMAGE_1_OFFSET) {
                                dfu_data.status = errWRITE;
                                dfu_data.state = dfuERROR;
@@ -636,14 +643,14 @@ static int dfu_custom_handle_req(struct usb_setup_packet *pSetup,
                        switch (pSetup->wValue) {
                        case 0:
                                dfu_data.flash_addr =
-                                       CONFIG_FLASH_BASE_ADDRESS +
+                                       FLASH_BASE_ADDRESS +
                                        FLASH_AREA_IMAGE_0_OFFSET;
                                dfu_data.flash_upload_size =
                                        FLASH_AREA_IMAGE_0_SIZE;
                                break;
                        case 1:
                                dfu_data.flash_addr =
-                                       CONFIG_FLASH_BASE_ADDRESS +
+                                       FLASH_BASE_ADDRESS +
                                        FLASH_AREA_IMAGE_1_OFFSET;
                                dfu_data.flash_upload_size =
                                        FLASH_AREA_IMAGE_1_SIZE;