RT1020 and RT1060 OTA #2562

robertc2000 · 2024-01-09T11:55:24Z

No description provided.

cpq · 2024-01-09T12:05:07Z

examples/nxp/rt1020-evk-make-baremetal-builtin/Makefile

-LDFLAGS ?= -T$(LDSCRIPT) -nostdlib -nostartfiles --specs nano.specs -lc -lgcc -Wl,--gc-sections -Wl,-Map=$@.map 
+LDFLAGS ?= -T$(LDSCRIPT) -nostdlib -nostartfiles --specs nano.specs -lc -lgcc -Wl,--gc-sections -Wl,-Map=$@.map
+
+PACK = ./pack             # Utility to pack files into a packed filesystem


Don't repack the UI! None of the embedded projects do. We simply symlink it from the device dashboard, and use a pre-built packed_fs.c. So, ditch this whole section please.

scaprile

RT1060 examples are now running from flash, please rebase and update your code

scaprile

RT1020 examples moved to flash, please rebase and update

scaprile · 2024-01-12T17:53:18Z

Makefile

please don't include this no-change

scaprile · 2024-01-12T17:54:05Z

examples/nxp/rt1060-evk-make-baremetal-builtin/Makefile

No changes, please do not include

scaprile · 2024-01-12T17:58:02Z

examples/nxp/rt1060-evk-make-baremetal-builtin/link.ld

+  .data : { __data_start__ = .; *(.data SORT(.data.*)) *(.iram) __data_end__ = .; } > itcram AT > flash_code
  __etext = LOADADDR(.data);
-  .bss  : { __bss_start__ = .; *(.bss SORT(.bss.*) COMMON) __bss_end__ = .; } > dtcram
+  .bss  : { __bss_start__ = .; *(.bss SORT(.bss.*) COMMON) __bss_end__ = .; } > itcram


Please add a TODO(): separate itcram and go back to using dtcram for data and bss when ota is finished comment so we can remember this in the future, otherwise there is no reason to use itcram

scaprile · 2024-01-12T17:58:34Z

examples/nxp/rt1020-evk-make-baremetal-builtin/Makefile

I'll review these after you rebase

scaprile · 2024-01-12T17:59:44Z

examples/nxp/rt1060-evk-make-baremetal-builtin/link.ld

  flash_hdr(rx)  : ORIGIN = 0x60000000, LENGTH = 8k
  flash_irq(rx)  : ORIGIN = 0x60002000, LENGTH = 1k
-  flash_code(rx) : ORIGIN = 0x60002400, LENGTH = 8183k
+  flash_code(rx) : ORIGIN = 0x60002400, LENGTH = 8179k


Did I overlooked something when I wrote this ? Some OTP/fuse magic ? Please tell me

We use the last sector (4k) of the first half of the flash as a temporary sector in order to do the firmware swap. This is why, the firmware should be 4k less in size (from 8183k to 8179k).

YALMT (Yet Another Linker Magic Trick), this is not portable, what happens if Mongoose is built with MCUXpresso or Keil ? --> OTA does not work. (same with IAR and other tools we do not actively support)
Are we "reserving" room in a similar way in other architectures ? We should not to that.
The cleanest portable way I can think of is to define a constant array of 0xFF when OTA is enabled, even though that will extend Mongoose footprint by 4KB (though that is actually true, it affects flashing time...)

scaprile · 2024-01-12T18:27:54Z

src/device_imxrt.c

+  }
+  MG_DEBUG(("Sector starting at %p erasure: %s", addr, ok ? "ok" : "fail"));
+  return ok;
+}


Regarding this function:

enabling interrupts there is risky if you are erasing sector 0, thing you take care of in other function. I think it is better to handle IRQs at the calling function

that MG_LOG is a) calling for trouble, and b) a nice delay. Like it or not, you are running flash code, the log function is not in RAM. It takes tens of milliseconds.

scaprile · 2024-01-12T18:31:06Z

src/device.h

+#define MG_FLASH_OFFSET 0x60000000  // Offset to memory mapped flash
+#define MG_FLASH_CODE_OFFSET 0x2000 // Offset to code from start of flash


Why not use mg_flash_start() to handle this ? (same in device_imxrt.c)

Ditto. mg_flash_* API was added to fully describe flash layout. We must not have this extra stuff.

scaprile · 2024-01-12T18:32:15Z

src/device_imxrt.c

+MG_IRAM void *mg_flash_start(void) {
+  return (void *) 0x0;
+}


What if mg_flash_start() returns 0x60000000 ?

Yes, you are right. Initially, there was the problem that the flash is offset by this value (0x60000000). So, there appeared two cases: when you wanted to write (using the ROM API), you had to translate this address by subtracting 0x60000000 from it. The other case was when you wanted to read and it this case you just accessed it at 0x60000000 (no subtraction required).

What I wanted to do by using that macro was to avoid over-complicating myself in the mg_flash_write / mg_flash_erase functions with having to worry about translating the write address when using the ROM API, so I added that macro which worked for any case (for flashes that require no translation, like ST, it's value is 0 and changes nothing)

Anyway, I realise now that it can work even without adding the macro. We'll stick to the original mg_flash_* API like Sergey commented above. I'll make the address translations (for ROM API calls to write/erase) in the mg_flash_write/erase functions and leave the rest of the code untouched.

scaprile · 2024-01-12T18:33:04Z

src/ota_flash.c

  if (s_size) {
    size_t size = s_addr - base;
-    uint32_t crc32 = mg_crc32(0, base, s_size);
+    uint32_t crc32 = mg_crc32(0, base + MG_FLASH_OFFSET, s_size);


Is this macro really needed ? (see other files, I don't in which order GH will present this to you...)

Solved, see above

scaprile · 2024-01-12T18:36:03Z

src/ota_flash.c

    (void) tmpsector;
    for (ofs = 0; ofs < max; ofs += ss) {
      // mg_flash_erase(tmpsector);
-      mg_flash_write(tmpsector, partition1 + ofs, ss);
+      mg_flash_write(tmpsector, partition1 + ofs + MG_FLASH_OFFSET, ss);
      // mg_flash_erase(partition1 + ofs);
-      mg_flash_write(partition1 + ofs, partition2 + ofs, ss);
+      mg_flash_write(partition1 + ofs, partition2 + ofs + MG_FLASH_OFFSET, ss);
      // mg_flash_erase(partition2 + ofs);
-      mg_flash_write(partition2 + ofs, tmpsector, ss);
+      mg_flash_write(partition2 + ofs, tmpsector + MG_FLASH_OFFSET, ss);
    }
    mg_device_reset();
  }


Did you actually try to swap partitions and succeed ?
Log calls in mg_flash_erase() will eventually mean you are calling flash that has been erased...

Before we start swapping the partitions, we disable the logs, so the macro has no effect.

The macro always has an effect, code has been compiled in with log enabled so that macro generated some code.
Does that RAM code actually check for the log level and then skips jumping to the log function, whatever it is ? (The user can change it)
Then that is cool and quite clever.
Bear in mind that all those strings are quite likely using valuable RAM space, nevetheless. (for the same reason)

Yes, the macro will first check the log level value and then decides if it the logging function will be executed. In the case of MG_LL_NONE (we set logging to this value in mg_ota_boot, right before swapping), no logging function will be executed.

cpq · 2024-01-13T11:33:48Z

examples/nxp/rt1020-evk-make-baremetal-builtin/main.c

-      mg_http_reply(c, 404, "", "Not Found\n");
-    }
-  }
+#ifdef MQTT_DASHBOARD


Not in this PR:
We have those no-op stubs defined for may projects that don't have / don't use real hardware.
I suggest to move these no-op stubs to the mqtt-dashboard/net.c itself - that would be a default implementation, no-op. Wrap it into conditionals to provide an ability to redefine.

mqtt-dashboard/net.c:

#ifndef ENABLE_CUSTOM_HAL bool hal_gpio_write(....) { MG_DEBUG(("pin: .., val:...", ...)); } ... #endif

cpq · 2024-01-13T11:39:10Z

src/device_imxrt.c

+
+#if MG_DEVICE == MG_DEVICE_RT1020 || MG_DEVICE == MG_DEVICE_RT1060
+
+#include <flexspi.h>  /**************************************************************************************************************************/


This include looks dangerous to me.
Generally, we do not include any device-specific files from an SDK. For two reasons:

To be independent from any 3rd party source

To protect from any possible clashes

cpq · 2024-01-13T11:39:27Z

src/device_imxrt.c

+// these are defined in the device CMSIS .h file, so examples will have their own, we should use a different name or #undef
+// hence, the example flexspi.h does not have these macros, that is NXP code carrying their license
+// Mongoose does not include any CMSIS file but these would go into mongoose.h and the user including mongoose.h and the device .h both definitions would clash


Ditto - I do not feel easy about that include.

cpq · 2024-01-13T11:41:04Z

src/device_imxrt.c

+typedef struct {
+  uint32_t version;
+  int (*init)(uint32_t instance, flexspi_nor_config_t *config);
+  int (*program)(uint32_t instance, flexspi_nor_config_t *config,
+                      uint32_t dst_addr, const uint32_t *src);
+  uint32_t reserved;
+  int (*erase)(uint32_t instance, flexspi_nor_config_t *config,
+                    uint32_t start, uint32_t lengthInBytes);
+  uint32_t reserved2;
+  int (*update_lut)(uint32_t instance, uint32_t seqIndex,
+                         const uint32_t *lutBase, uint32_t seqNumber);
+  int (*xfer)(uint32_t instance, char *xfer);
+  void (*clear_cache)(uint32_t instance);
+} flexspi_nor_driver_interface_t;
+#elif MG_DEVICE == MG_DEVICE_RT1060
+typedef struct {
+  uint32_t version;
+  int (*init)(uint32_t instance, flexspi_nor_config_t *config);
+  int (*program)(uint32_t instance, flexspi_nor_config_t *config,
+                      uint32_t dst_addr, const uint32_t *src);
+  int (*erase_all)(uint32_t instance, flexspi_nor_config_t *config);
+  int (*erase)(uint32_t instance, flexspi_nor_config_t *config,
+                    uint32_t start, uint32_t lengthInBytes);
+  int (*read)(uint32_t instance, flexspi_nor_config_t *config,
+                   uint32_t *dst, uint32_t addr, uint32_t lengthInBytes);


Yes.
We should redefine our own structure.
The same way we do e.g. for ST Ethernet driver, or ST H5/H7 devices for flashing - we do not rely on any SDK sources, but define addresses/structures ourselves.

cpq · 2024-01-13T11:44:27Z

mongoose.h

 #define MG_DEVICE MG_DEVICE_NONE
 #endif

+#if MG_DEVICE == MG_DEVICE_RT1020 || MG_DEVICE == MG_DEVICE_RT1060


Why do we need that?

Flash layout is supposed to be described by the mg_flash_* API. Having a define which kinda does the same is confusing. I am not sure we should have this.

Solved, see explanation for original reasoning above

cpq · 2024-01-13T11:45:25Z

src/device.h

+#define MG_FLASH_OFFSET 0x60000000  // Offset to memory mapped flash
+#define MG_FLASH_CODE_OFFSET 0x2000 // Offset to code from start of flash


Ditto. mg_flash_* API was added to fully describe flash layout. We must not have this extra stuff.

scaprile · 2024-01-15T12:33:09Z

examples/nxp/rt1020-evk-make-baremetal-builtin/flash_image.c

Please go back to the original file and don't add unnecessary changes, that will be reverted back when someone runs clang-format and f-starting goes before h-starting again...

scaprile · 2024-01-15T12:39:20Z

src/device_imxrt.c

+static inline void flash_wait(void) {
+  while ((*((volatile uint32_t *)(0x402A8000 + 0xE0)) & MG_BIT(1)) == 0) spin(1);
+}


If they did not want to wait inside the ROM code to return quickly and be able to do something else, they could have just documented that...

scaprile · 2024-01-15T12:43:38Z

src/device_imxrt.c

+  // Note: If we overwrite the flash irq section of the image, we must also
+  // make sure interrupts are disabled and are not reenabled until we write
+  // this sector with another irq table.


In fact, once we change this, new IRQs will be pointing to somewhere in the new image, so actually we can't reenable IRQs until the new image is able to run. I guess that means booting again, so that crude reality might simplify your handling of interrupts in the code.
Once this sector has been erased, there is no going back, bridges are burned, we either flash the whole thing and reboot or we are bricked...

scaprile · 2024-01-15T12:47:14Z

src/device_imxrt_flexspi.h

We need to de-vendor-ize this file, and cesanta-ize / mongoose-ize it; make it our own. Most comments are plainly useless and that comment formatting is not our own
Should we also add IMXRT to the MGprefix ? @cpq ? (Should we acknowledge the source ?)

Now, with this file in mongoose.h, our baremetal examples don't need their flexspi.h file, flash_image.c can be reworked to use these built-in flexspi settings.

scaprile · 2024-01-18T20:51:56Z

Managed to get rid of flexspi.h, please rebase

robertc2000 requested review from cpq and scaprile January 9, 2024 11:55

cpq reviewed Jan 9, 2024

View reviewed changes

robertc2000 force-pushed the imxflash-2 branch 3 times, most recently from 7a6456b to 6e072ed Compare January 10, 2024 11:27

scaprile reviewed Jan 11, 2024

View reviewed changes

robertc2000 force-pushed the imxflash-2 branch 2 times, most recently from a5629f2 to f750cf3 Compare January 12, 2024 13:07

scaprile self-requested a review January 12, 2024 17:24

scaprile reviewed Jan 12, 2024

View reviewed changes

cpq requested changes Jan 13, 2024

View reviewed changes

robertc2000 force-pushed the imxflash-2 branch 4 times, most recently from 984d9e8 to cc10443 Compare January 15, 2024 12:16

scaprile reviewed Jan 15, 2024

View reviewed changes

robertc2000 force-pushed the imxflash-2 branch 8 times, most recently from b4685e5 to e28a611 Compare January 18, 2024 08:36

RT1020 and RT1060 OTA

8500afb

robertc2000 force-pushed the imxflash-2 branch from e28a611 to 8500afb Compare January 19, 2024 07:49

cpq approved these changes Jan 19, 2024

View reviewed changes

robertc2000 merged commit a1c5995 into master Jan 19, 2024

scaprile deleted the imxflash-2 branch January 22, 2024 21:45

		#define MG_FLASH_OFFSET 0x60000000 // Offset to memory mapped flash
		#define MG_FLASH_CODE_OFFSET 0x2000 // Offset to code from start of flash


		#if MG_DEVICE == MG_DEVICE_RT1020 \|\| MG_DEVICE == MG_DEVICE_RT1060

		#include <flexspi.h> /**************************************************************************************************************************/

RT1020 and RT1060 OTA #2562

RT1020 and RT1060 OTA #2562

Uh oh!

Conversation

robertc2000 commented Jan 9, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scaprile left a comment

Choose a reason for hiding this comment

Uh oh!

scaprile left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robertc2000 Jan 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scaprile Jan 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scaprile commented Jan 18, 2024

Uh oh!

Reviewers

Assignees

robertc2000 Jan 15, 2024 •

edited

Loading

scaprile Jan 15, 2024 •

edited

Loading