Skip to content

Commit 9d375c2

Browse files
committed
Peer review fixes (thanks Copilot)
hal/stm32g4.c: - hal_flash_write unaligned path: rebase the aligned doubleword on (address + i), not the initial address; index as dst[0]/dst[1]. The old form skipped to the wrong DW once i advanced past 8 with an unaligned starting address. - hal_flash_erase: end_address was address + len - 1 paired with a strict less-than, which dropped the last page when len straddled a page boundary by one byte. Use address + len. - flash_clear_errors / EOP clear at end of hal_flash_write: FLASH_SR bits are W1C. Read-modify-write via |= can clear unrelated W1C bits that happen to be set. Write the mask directly. - uart_write: cast through uint8_t before promoting to uint32_t so high-bit chars do not sign-extend into TDR. hal/stm32g4.h: - DMB/ISB/DSB: add the "memory" clobber so the compiler also treats them as scheduling barriers, not just hardware-level fences. hal/stm32g4.ld: - Rename the output section .edidx to .ARM.exidx to match the input-section pattern and standard Cortex-M linker scripts.
1 parent 9209278 commit 9d375c2

3 files changed

Lines changed: 25 additions & 20 deletions

File tree

hal/stm32g4.c

Lines changed: 18 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -36,10 +36,13 @@ static RAMFUNCTION void flash_wait_complete(void)
3636

3737
static void RAMFUNCTION flash_clear_errors(void)
3838
{
39-
FLASH_SR |= (FLASH_SR_OPERR | FLASH_SR_PROGERR | FLASH_SR_WRPERR |
40-
FLASH_SR_PGAERR | FLASH_SR_SIZERR | FLASH_SR_PGSERR |
41-
FLASH_SR_MISERR | FLASH_SR_FASTERR | FLASH_SR_RDERR |
42-
FLASH_SR_OPTVERR);
39+
/* FLASH_SR error/EOP bits are write-1-to-clear. Assign the mask
40+
* directly so a read-modify-write doesn't accidentally clear any
41+
* other W1C bits that happen to be set. */
42+
FLASH_SR = (FLASH_SR_OPERR | FLASH_SR_PROGERR | FLASH_SR_WRPERR |
43+
FLASH_SR_PGAERR | FLASH_SR_SIZERR | FLASH_SR_PGSERR |
44+
FLASH_SR_MISERR | FLASH_SR_FASTERR | FLASH_SR_RDERR |
45+
FLASH_SR_OPTVERR);
4346
}
4447

4548
int RAMFUNCTION hal_flash_write(uint32_t address, const uint8_t *data, int len)
@@ -63,21 +66,21 @@ int RAMFUNCTION hal_flash_write(uint32_t address, const uint8_t *data, int len)
6366
} else {
6467
uint32_t val[2];
6568
uint8_t *vbytes = (uint8_t *)(val);
66-
int off = (address + i) - (((address + i) >> 3) << 3);
67-
uint32_t base_addr = address & (~0x07); /* aligned to 64 bit */
68-
int u32_idx = (i >> 2);
69+
uint32_t base_addr = (address + i) & ~0x7u; /* DW we touch */
70+
int off = (address + i) & 0x7; /* byte in DW */
6971
dst = (uint32_t *)(base_addr);
70-
val[0] = dst[u32_idx];
71-
val[1] = dst[u32_idx + 1];
72+
val[0] = dst[0];
73+
val[1] = dst[1];
7274
while ((off < 8) && (i < len))
7375
vbytes[off++] = data[i++];
74-
dst[u32_idx] = val[0];
75-
dst[u32_idx + 1] = val[1];
76+
flash_wait_complete();
77+
dst[0] = val[0];
78+
dst[1] = val[1];
7679
flash_wait_complete();
7780
}
7881
}
79-
if ((FLASH_SR & FLASH_SR_EOP) == FLASH_SR_EOP)
80-
FLASH_SR |= FLASH_SR_EOP;
82+
/* W1C: assign directly; no RMW. Harmless if EOP isn't set. */
83+
FLASH_SR = FLASH_SR_EOP;
8184
FLASH_CR &= ~FLASH_CR_PG;
8285
return 0;
8386
}
@@ -113,7 +116,7 @@ int RAMFUNCTION hal_flash_erase(uint32_t address, int len)
113116
if (len == 0)
114117
return -1;
115118
address -= FLASHMEM_ADDRESS_SPACE;
116-
end_address = address + len - 1;
119+
end_address = address + len;
117120
for (p = address; p < end_address; p += FLASH_PAGE_SIZE) {
118121
flash_wait_complete();
119122
flash_clear_errors();
@@ -258,7 +261,7 @@ void uart_write(const char *buf, unsigned int len)
258261
for (i = 0; i < len; i++) {
259262
while ((LPUART1_ISR & USART_ISR_TXE) == 0)
260263
;
261-
LPUART1_TDR = (uint32_t)buf[i];
264+
LPUART1_TDR = (uint32_t)(uint8_t)buf[i];
262265
}
263266
}
264267

hal/stm32g4.h

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -30,10 +30,12 @@
3030

3131
#include <stdint.h>
3232

33-
/* Assembly helpers */
34-
#define DMB() __asm__ volatile ("dmb")
35-
#define ISB() __asm__ volatile ("isb")
36-
#define DSB() __asm__ volatile ("dsb")
33+
/* Assembly helpers. The "memory" clobber tells the compiler not to
34+
* reorder loads/stores across the barrier; without it the asm only
35+
* orders hardware accesses, not the compiler's own scheduling. */
36+
#define DMB() __asm__ volatile ("dmb" ::: "memory")
37+
#define ISB() __asm__ volatile ("isb" ::: "memory")
38+
#define DSB() __asm__ volatile ("dsb" ::: "memory")
3739

3840

3941
/* -------- RCC (AHB1 + 0x1000 = 0x40021000) -------- RM0440 - 7.4 -------- */

hal/stm32g4.ld

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ SECTIONS
1616
_end_text = .;
1717
} > FLASH
1818

19-
.edidx :
19+
.ARM.exidx :
2020
{
2121
. = ALIGN(4);
2222
*(.ARM.exidx*)

0 commit comments

Comments
 (0)