[riot-notifications] [RIOT-OS/RIOT] stm32f{2, 4, 7}: Initial flashpage support (#15420)

benpicco notifications at github.com
Wed Jan 27 23:36:54 CET 2021


@benpicco commented on this pull request.

Looks good!
I think you can get rid of an `#ifdef` block to make the code simpler - and in turn please add a comment explaining that register magic blocks :wink: 

> @@ -118,8 +126,19 @@ static void _erase_page(void *page_addr)
     pn = (uint8_t)page;
 #endif
     CNTRL_REG &= ~FLASH_CR_PNB;
+#if defined(CPU_FAM_STM32F4) || defined(CPU_FAM_STM32F7)
+    if (FLASHPAGE_DUAL_BANK && (pn > (FLASHPAGE_NUMOF / 2 - 1))) {
+        pn = pn - (FLASHPAGE_NUMOF / 2);
+        CNTRL_REG |= FLASH_CR_SNB_4 | (uint32_t)(pn << FLASH_CR_PNB_Pos);
+    }
+    else {
+        CNTRL_REG |= (uint32_t)(pn << FLASH_CR_PNB_Pos);
+    }
+#else

Does this case only cover STM32F2 or all STM32 variants other than F4 & F7?

> @@ -118,8 +126,19 @@ static void _erase_page(void *page_addr)
     pn = (uint8_t)page;
 #endif
     CNTRL_REG &= ~FLASH_CR_PNB;
+#if defined(CPU_FAM_STM32F4) || defined(CPU_FAM_STM32F7)

```suggestion
```

> @@ -118,8 +126,19 @@ static void _erase_page(void *page_addr)
     pn = (uint8_t)page;
 #endif
     CNTRL_REG &= ~FLASH_CR_PNB;
+#if defined(CPU_FAM_STM32F4) || defined(CPU_FAM_STM32F7)
+    if (FLASHPAGE_DUAL_BANK && (pn > (FLASHPAGE_NUMOF / 2 - 1))) {

```suggestion
    if (IS_USED(FLASHPAGE_DUAL_BANK) && (pn > (FLASHPAGE_NUMOF / 2 - 1))) {
```

> +#else
     CNTRL_REG |= (uint32_t)(pn << FLASH_CR_PNB_Pos);
+#endif

```suggestion
```

We can get rid of that `#ifdef`, makes it less fragile when adding new STM32 siblings.

> @@ -130,9 +149,24 @@ static void _erase_page(void *page_addr)
     /* wait as long as device is busy */
     _wait_for_pending_operations();
 
+#ifdef FLASH_ACR_DCEN

Please add a note what this does. 

> +#ifdef FLASH_ACR_ICEN
+    if (FLASH->ACR & FLASH_ACR_ICEN) {

Same here

> +#if defined(CPU_FAM_STM32F2) || defined(CPU_FAM_STM32F4) || \
+    defined(CPU_FAM_STM32F7)

```suggestion
#if defined(FLASH_CR_PSIZE_1)
```

Would this also work?

> +#ifdef FLASH_ACR_DCEN
+    if (FLASH->ACR & FLASH_ACR_DCEN) {
+        FLASH->ACR &= ~FLASH_ACR_DCEN;
+    }
+#endif
+#ifdef FLASH_ACR_ICEN
+    if (FLASH->ACR & FLASH_ACR_ICEN) {
+        FLASH->ACR &= ~FLASH_ACR_ICEN;
+    }
+#endif

What do those blocks do?

> +#ifdef FLASH_ACR_DCEN
+    FLASH->ACR |= FLASH_ACR_DCRST;
+    FLASH->ACR |= FLASH_ACR_DCEN;
+#endif
+#ifdef FLASH_ACR_ICEN
+    FLASH->ACR |= FLASH_ACR_ICRST;
+    FLASH->ACR |= FLASH_ACR_ICEN;
+#endif

same here

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/RIOT-OS/RIOT/pull/15420#pullrequestreview-577815599
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20210127/b17faa3b/attachment.htm>


More information about the notifications mailing list