7
31
Fork
You've already forked blackmagic
23

Feat: Renesas RA family Option-Setting #1450

Open
krzysztofgawrys wants to merge 31 commits from krzysztofgawrys/fix/renesas-optionsetting-flash into main
pull from: krzysztofgawrys/fix/renesas-optionsetting-flash
merge into: blackmagic-debug:main
blackmagic-debug:main
blackmagic-debug:feature/bmda-remote-comms
blackmagic-debug:ALTracer/feature/aarch64-ident
blackmagic-debug:feature/better-meson-optimisation-handling
blackmagic-debug:feature/am335x-support
blackmagic-debug:feature/esp32-c3-support
blackmagic-debug:feature/cortex-ar-software-breakpoints
blackmagic-debug:feature/unit-testing
blackmagic-debug:feature/windows-usb-serial-interface-naming
blackmagic-debug:fix/bmp-external-spi
blackmagic-debug:ALTracer/feature/bluepillplus-platform
blackmagic-debug:ALTracer/feature/at32f43x-unrdp
blackmagic-debug:feature/const-correctness
blackmagic-debug:ALTracer/feature/fault_handlers
blackmagic-debug:ALTracer/feature/hazard3-ice40-support
blackmagic-debug:fix/ci-cleanup
blackmagic-debug:ALTracer/fix/gdb-10-12-thread
blackmagic-debug:ALTracer/feature/blackpill-f4-adc
blackmagic-debug:ALTracer/fix/cortex-desc-allocfail-report
blackmagic-debug:ALTracer/feature/spi-perf
blackmagic-debug:ALTracer/feature/calibrate_swd
blackmagic-debug:ALTracer/feature/blank-check
blackmagic-debug:feature/avr
blackmagic-debug:v2.0
blackmagic-debug:v1.9
blackmagic-debug:v1.10
blackmagic-debug:v1.8
krzysztofgawrys commented 2023年04月11日 16:30:03 +02:00 (Migrated from github.com)
Copy link

Detailed description

Adds support for Option-Setting Flash memory area for Renesas RA Family

Your checklist for this pull request

  • I've read the Code of Conduct
  • I've read the guidelines for contributing to this repository
  • It builds for hardware native (make PROBE_HOST=native)
  • It builds as BMDA (make PROBE_HOST=hosted)
  • I've tested it to the best of my ability
  • My commit messages provide a useful short description of what the commits do
## Detailed description Adds support for Option-Setting Flash memory area for Renesas RA Family ## Your checklist for this pull request * [x] I've read the [Code of Conduct](https://github.com/blackmagic-debug/blackmagic/blob/main/CODE_OF_CONDUCT.md) * [x] I've read the [guidelines for contributing](https://github.com/blackmagic-debug/blackmagic/blob/main/CONTRIBUTING.md) to this repository * [x] It builds for hardware native (`make PROBE_HOST=native`) * [x] It builds as BMDA (`make PROBE_HOST=hosted`) * [x] I've tested it to the best of my ability * [x] My commit messages provide a useful short description of what the commits do
perigoso (Migrated from github.com) reviewed 2023年04月11日 20:16:17 +02:00
perigoso (Migrated from github.com) left a comment
Copy link

Good work!

(削除) I think the way you implemented this with the hardcoded value for RA4/6 might negatively affect other targets, I was very careful with possible address collisions originally, and read through all existing families, and I want to make sure you have the same confidence here, note that this is just a suspicion I can't back up ATM. (削除ここまで)

Looks like its safe to assume code flash is always under the lowest option-setting address

Good work! ~~I think the way you implemented this with the hardcoded value for RA4/6 might negatively affect other targets, I was very careful with possible address collisions originally, and read through all existing families, and I want to make sure you have the same confidence here, note that this is just a suspicion I can't back up ATM.~~ Looks like its safe to assume code flash is always under the lowest option-setting address
perigoso (Migrated from github.com) commented 2023年04月11日 20:01:25 +02:00
Copy link

This needs better naming, the other flash related macros are tied to the flash type, so no extra context is required, here it's hard to know what this is about

Does this have to be hardcoded like this? what is this tied to, the family, perhaps flash type?

This needs better naming, the other flash related macros are tied to the flash type, so no extra context is required, here it's hard to know what this is about Does this have to be hardcoded like this? what is this tied to, the family, perhaps flash type?
perigoso (Migrated from github.com) commented 2023年04月11日 20:03:29 +02:00
Copy link

(削除) This looks like it breaks parts with flashes that go over that particular option flash adress, I don't remember if thats possible, but I have to assume I chose the original value for areason (削除ここまで) redacted, looks like it's safe

~~This looks like it breaks parts with flashes that go over that particular option flash adress, I don't remember if thats possible, but I have to assume I chose the original value for areason~~ redacted, looks like it's safe
perigoso (Migrated from github.com) commented 2023年04月11日 20:05:57 +02:00
Copy link

if erasing is not allowed why is there a flash_erase method tied to it, we should omit it, I don't know if that's currently allowed, but It should if not, or have a stub, that said retaining the sanity check might still be good

if erasing is not allowed why is there a flash_erase method tied to it, we should omit it, I don't know if that's currently allowed, but It should if not, or have a stub, that said retaining the sanity check might still be good
perigoso (Migrated from github.com) commented 2023年04月11日 20:08:00 +02:00
Copy link

It's starting to look like it might be worth to store this information within the flash struct, in particular because after the rework a while ago we are guaranteed fixed sizes here so a lot of code could be pruned

It's starting to look like it might be worth to store this information within the flash struct, in particular because after the rework a while ago we are guaranteed fixed sizes here so a lot of code could be pruned
perigoso (Migrated from github.com) commented 2023年04月11日 20:10:04 +02:00
Copy link

surely the expected time is documented somehow, it was for most other flash related things, why 200 over 20, was 20 too short?

surely the expected time is documented somehow, it was for most other flash related things, why 200 over 20, was 20 too short?
krzysztofgawrys commented 2023年04月11日 21:32:37 +02:00 (Migrated from github.com)
Copy link

Converted this to draft as there is too many problems with this code.

Converted this to draft as there is too many problems with this code.
perigoso commented 2023年04月11日 22:14:40 +02:00 (Migrated from github.com)
Copy link

Went through all of the families documentation, here is a table with the extracted address spaces

Theres a direct correlation with a lot of the variables, which is somewhat expected

family code end option start option size option content option 2 start option2 size option content data flash data size factory flash factory size factory flash content sramhs sramhs size sram0 sram0 size stdby sram stdby sram size type pnr loc FRT start FRT size
ra2l1 0x00040000 0x01010010 36 0:4 - Access Window Setting Register 8:36 - OCD/Serial Programmer ID Setting Register - - 0x40100000 0x40102000 - - - - 0x20000000 0x20008000 - - MF4 FL1 - -
ra2e1 0x00020000 0x01010010 36 0:4 - Access Window Setting Register 8:36 - OCD/Serial Programmer ID Setting Register - - 0x40100000 0x40101000 - - - - 0x20004000 0x20008000 - - MF4 FL1 - -
ra2e2 0x00010000 0x01010010 36 0:4 - Access Window Setting Register 8:36 - OCD/Serial Programmer ID Setting Register - - 0x40100000 0x40100800 - - - - 0x20004000 0x20006000 - - MF4 FL1 - -
ra2a1 0x00040000 0x01010008 44 0:4 - Access Window Setting Control Register (AWSC) 8:12 - Access Window Setting Register (AWS) 16:44 - OCD/Serial Programmer ID Setting Register (OSIS) - - 0x40100000 0x40102000 - - 0x20000000 0x20008000 - - MF3 FRT 0x407FB19C 4
ra4m1 0x00040000 0x01010008 44 0:4 - Access Window Setting Control Register (AWSC) 8:12 - Access Window Setting Register (AWS) 16:44 - OCD/Serial Programmer ID Setting Register (OSIS) - - 0x40100000 0x40102000 - - - - 0x20000000 0x20008000 - - MF3 FRT 0x407FB19C 4
ra4m2 0x00080000 0x0100A100 512 MCU settings - - 0x08000000 0x08002000 0x010080F0 196 0:16 - Part Numbering Register 160:176 - UIDR 192 - MCUVE - - 0x20000000 0x20020000 0x28000000 0x28000400 RV40 FL2 - -
ra4m3 0x00100000 0x0100A100 512 MCU settings - - 0x08000000 0x08002000 0x010080F0 196 0:16 - Part Numbering Register 160:176 - UIDR 192 - MCUVE - - 0x20000000 0x20020000 0x28000000 0x28000400 RV40 FL2 - -
ra4e1 0x00080000 0x0100A100 512 MCU settings - - 0x08000000 0x08002000 0x010080F0 196 0:16 - Part Numbering Register 160:176 - UIDR 192 - MCUVE - - 0x20000000 0x20020000 0x28000000 0x28000400 RV40 FL2 - -
ra4e2 0x00020000 0x0100A100 512 MCU settings - - 0x08000000 0x08001000 0x010080F0 196 0:16 - Part Numbering Register 160:176 - UIDR 192 - MCUVE - - 0x20000000 0x2000A000 0x28000000 0x28000400 RV40 FL2 - -
ra4w1 0x00080000 0x01010008 44 0:4 - Access Window Setting Control Register (AWSC) 8:12 - Access Window Setting Register (AWS) 16:44 - OCD/Serial Programmer ID Setting Register (OSIS) - - 0x40100000 0x40102000 - - - - 0x20000000 0x20018000 - - MF3 FRT 0x407FB19C 4
ra6m1 0x00080000 0x01007000 4096 0:45 - Factory MCU Information Flash Root Table content The purpose of the remainder is unclear 0x0100A150 24 0:12 - OCD/Serial Programmer ID Setting Register (OSIS) 20:24 - Access Window Setting Register (AWS) 0x40100000 0x40102000 - - 0x1FFE0000 0x20000000 0x20000000 0x20020000 0x200FE000 0x20100000 RV40 FRT 0x407FB17C 36 (0:32 - temp cal)
ra6m2 0x00100000 0x01007000 4096 0:45 - Factory MCU Information Flash Root Table content The purpose of the remainder is unclear 0x0100A150 24 0:12 - OCD/Serial Programmer ID Setting Register (OSIS) 20:24 - Access Window Setting Register (AWS) 0x40100000 0x40108000 - - 0x1FFE0000 0x20000000 0x20000000 0x20040000 0x200FE000 0x20100000 RV40 FRT 0x407FB17C 36 (0:32 - temp cal)
ra6m4 0x00280000 0x0100A100 512 MCU settings - - 0x08000000 0x08002000 0x010080F0 196 0:16 - Part Numbering Register 160:176 - UIDR 192 - MCUVE - - 0x20000000 0x20040000 0x28000000 0x28000400 RV40 FL2 - -
ra6m5 0x00300000 0x0100A100 512 MCU settings - - 0x08000000 0x08002000 0x010080F0 196 0:16 - Part Numbering Register 160:176 - UIDR 192 - MCUVE - - 0x20000000 0x20080000 0x28000000 0x28000400 RV40 FL2 - -
ra6e1 0x00280000 0x0100A100 512 MCU settings - - 0x08000000 0x08002000 0x010080F0 196 0:16 - Part Numbering Register 160:176 - UIDR 192 - MCUVE - - 0x20000000 0x20040000 0x28000000 0x28000400 RV40 FL2 - -
ra6e2 0x00040000 0x0100A100 512 MCU settings - - 0x08000000 0x08001000 0x010080F0 196 0:16 - Part Numbering Register 160:176 - UIDR 192 - MCUVE - - 0x20000000 0x2000A000 0x28000000 0x28000400 RV40 FL2 - -
ra6t1 0x00080000 0x01007000 4096 0:45 - Factory MCU Information Flash Root Table content The purpose of the remainder is unclear 0x0100A150 24 0:12 - OCD/Serial Programmer ID Setting Register (OSIS) 20:24 - Access Window Setting Register (AWS) 0x40100000 0x40102000 - - 0x1FFE0000 0x1FFF0000 - - - - RV40 FRT 0x407FB17C 36 (0:32 - temp cal)
ra6t2 0x00080000 0x0100A100 512 MCU settings - - 0x08000000 0x08004000 0x010080F0 196 0:16 - Part Numbering Register 160:176 - UIDR 192 - MCUVE - - 0x20000000 0x20010000 0x28000000 0x28000400 RV40 FL2 - -

The references:
https://www.renesas.com/us/en/document/mah/ra6e2-group-users-manual-hardware?r=25411266
https://www.renesas.com/us/en/document/man/ra6m5-group-users-manual-hardware?r=1493931
https://www.renesas.com/us/en/document/mah/renesas-ra6m2-group-users-manual-hardware?r=1054161
https://www.renesas.com/us/en/document/man/ra6m4-group-user-s-manual-hardware?r=1333976
https://www.renesas.com/us/en/document/man/ra4m2-group-users-manual-hardware?r=1469026
https://www.renesas.com/us/en/document/mah/renesas-ra6m1-group-users-manual-hardware?r=1054156
https://www.renesas.com/us/en/document/mah/ra6e1-group-users-manual-hardware?r=1521986
https://www.renesas.com/us/en/document/mah/ra6t2-group-users-manual-hardware?r=1542426
https://www.renesas.com/us/en/document/mat/renesas-ra6t1-group-users-manual-hardware?r=1326271
https://www.renesas.com/us/en/document/man/renesas-ra4w1-group-users-manual-hardware?r=1054151
https://www.renesas.com/us/en/document/mah/renesas-ra4m1-group-users-manual-hardware?r=1054146
https://www.renesas.com/us/en/document/man/ra4m3-group-users-manual-hardware?r=1400806
https://www.renesas.com/us/en/document/mah/ra4e2-group-users-manual-hardware?r=25411256
https://www.renesas.com/us/en/document/mah/ra4e1-group-users-manual-hardware?r=1521991
https://www.renesas.com/us/en/document/mah/ra2l1-group-users-manual-hardware?r=1398061
https://www.renesas.com/us/en/document/man/ra2e1-group-users-manual-hardware?r=1469021
https://www.renesas.com/us/en/document/mah/ra2e2-group-users-manual-hardware?r=1532431
https://www.renesas.com/us/en/document/mah/renesas-ra2a1-group-users-manual-hardware?r=1054141
https://www.renesas.com/us/en/document/mah/ra6m3-group-users-manual-hardware

Went through all of the families documentation, here is a table with the extracted address spaces Theres a direct correlation with a lot of the variables, which is somewhat expected | family | code end | option start | option size | option content | option 2 start | option2 size | option content | data flash | data size | factory flash | factory size | factory flash content | sramhs | sramhs size | sram0 | sram0 size | stdby sram | stdby sram size | type | pnr loc | FRT start | FRT size | |--------|------------|--------------|-------------|----------------------------------------------------------------------------------------------------------------------------------------------------------|----------------|--------------|------------------------------------------------------------------------------------------------------|------------|------------|---------------|--------------|-----------------------------------------------------------|------------|-------------|------------|------------|------------|-----------------|------|---------|------------|----------------------| | ra2l1 | 0x00040000 | 0x01010010 | 36 | 0:4 - Access Window Setting Register 8:36 - OCD/Serial Programmer ID Setting Register | - | - | | 0x40100000 | 0x40102000 | - | - | | - | - | 0x20000000 | 0x20008000 | - | - | MF4 | FL1 | - | - | | ra2e1 | 0x00020000 | 0x01010010 | 36 | 0:4 - Access Window Setting Register 8:36 - OCD/Serial Programmer ID Setting Register | - | - | | 0x40100000 | 0x40101000 | - | - | | - | - | 0x20004000 | 0x20008000 | - | - | MF4 | FL1 | - | - | | ra2e2 | 0x00010000 | 0x01010010 | 36 | 0:4 - Access Window Setting Register 8:36 - OCD/Serial Programmer ID Setting Register | - | - | | 0x40100000 | 0x40100800 | - | - | | - | - | 0x20004000 | 0x20006000 | - | - | MF4 | FL1 | - | - | | ra2a1 | 0x00040000 | 0x01010008 | 44 | 0:4 - Access Window Setting Control Register (AWSC) 8:12 - Access Window Setting Register (AWS) 16:44 - OCD/Serial Programmer ID Setting Register (OSIS) | - | - | | 0x40100000 | 0x40102000 | - | - | | | | 0x20000000 | 0x20008000 | - | - | MF3 | FRT | 0x407FB19C | 4 | | ra4m1 | 0x00040000 | 0x01010008 | 44 | 0:4 - Access Window Setting Control Register (AWSC) 8:12 - Access Window Setting Register (AWS) 16:44 - OCD/Serial Programmer ID Setting Register (OSIS) | - | - | | 0x40100000 | 0x40102000 | - | - | | - | - | 0x20000000 | 0x20008000 | - | - | MF3 | FRT | 0x407FB19C | 4 | | ra4m2 | 0x00080000 | 0x0100A100 | 512 | MCU settings | - | - | | 0x08000000 | 0x08002000 | 0x010080F0 | 196 | 0:16 - Part Numbering Register 160:176 - UIDR 192 - MCUVE | - | - | 0x20000000 | 0x20020000 | 0x28000000 | 0x28000400 | RV40 | FL2 | - | - | | ra4m3 | 0x00100000 | 0x0100A100 | 512 | MCU settings | - | - | | 0x08000000 | 0x08002000 | 0x010080F0 | 196 | 0:16 - Part Numbering Register 160:176 - UIDR 192 - MCUVE | - | - | 0x20000000 | 0x20020000 | 0x28000000 | 0x28000400 | RV40 | FL2 | - | - | | ra4e1 | 0x00080000 | 0x0100A100 | 512 | MCU settings | - | - | | 0x08000000 | 0x08002000 | 0x010080F0 | 196 | 0:16 - Part Numbering Register 160:176 - UIDR 192 - MCUVE | - | - | 0x20000000 | 0x20020000 | 0x28000000 | 0x28000400 | RV40 | FL2 | - | - | | ra4e2 | 0x00020000 | 0x0100A100 | 512 | MCU settings | - | - | | 0x08000000 | 0x08001000 | 0x010080F0 | 196 | 0:16 - Part Numbering Register 160:176 - UIDR 192 - MCUVE | - | - | 0x20000000 | 0x2000A000 | 0x28000000 | 0x28000400 | RV40 | FL2 | - | - | | ra4w1 | 0x00080000 | 0x01010008 | 44 | 0:4 - Access Window Setting Control Register (AWSC) 8:12 - Access Window Setting Register (AWS) 16:44 - OCD/Serial Programmer ID Setting Register (OSIS) | - | - | | 0x40100000 | 0x40102000 | - | - | | - | - | 0x20000000 | 0x20018000 | - | - | MF3 | FRT | 0x407FB19C | 4 | | ra6m1 | 0x00080000 | 0x01007000 | 4096 | 0:45 - Factory MCU Information Flash Root Table content The purpose of the remainder is unclear | 0x0100A150 | 24 | 0:12 - OCD/Serial Programmer ID Setting Register (OSIS) 20:24 - Access Window Setting Register (AWS) | 0x40100000 | 0x40102000 | - | - | | 0x1FFE0000 | 0x20000000 | 0x20000000 | 0x20020000 | 0x200FE000 | 0x20100000 | RV40 | FRT | 0x407FB17C | 36 (0:32 - temp cal) | | ra6m2 | 0x00100000 | 0x01007000 | 4096 | 0:45 - Factory MCU Information Flash Root Table content The purpose of the remainder is unclear | 0x0100A150 | 24 | 0:12 - OCD/Serial Programmer ID Setting Register (OSIS) 20:24 - Access Window Setting Register (AWS) | 0x40100000 | 0x40108000 | - | - | | 0x1FFE0000 | 0x20000000 | 0x20000000 | 0x20040000 | 0x200FE000 | 0x20100000 | RV40 | FRT | 0x407FB17C | 36 (0:32 - temp cal) | | ra6m4 | 0x00280000 | 0x0100A100 | 512 | MCU settings | - | - | | 0x08000000 | 0x08002000 | 0x010080F0 | 196 | 0:16 - Part Numbering Register 160:176 - UIDR 192 - MCUVE | - | - | 0x20000000 | 0x20040000 | 0x28000000 | 0x28000400 | RV40 | FL2 | - | - | | ra6m5 | 0x00300000 | 0x0100A100 | 512 | MCU settings | - | - | | 0x08000000 | 0x08002000 | 0x010080F0 | 196 | 0:16 - Part Numbering Register 160:176 - UIDR 192 - MCUVE | - | - | 0x20000000 | 0x20080000 | 0x28000000 | 0x28000400 | RV40 | FL2 | - | - | | ra6e1 | 0x00280000 | 0x0100A100 | 512 | MCU settings | - | - | | 0x08000000 | 0x08002000 | 0x010080F0 | 196 | 0:16 - Part Numbering Register 160:176 - UIDR 192 - MCUVE | - | - | 0x20000000 | 0x20040000 | 0x28000000 | 0x28000400 | RV40 | FL2 | - | - | | ra6e2 | 0x00040000 | 0x0100A100 | 512 | MCU settings | - | - | | 0x08000000 | 0x08001000 | 0x010080F0 | 196 | 0:16 - Part Numbering Register 160:176 - UIDR 192 - MCUVE | - | - | 0x20000000 | 0x2000A000 | 0x28000000 | 0x28000400 | RV40 | FL2 | - | - | | ra6t1 | 0x00080000 | 0x01007000 | 4096 | 0:45 - Factory MCU Information Flash Root Table content The purpose of the remainder is unclear | 0x0100A150 | 24 | 0:12 - OCD/Serial Programmer ID Setting Register (OSIS) 20:24 - Access Window Setting Register (AWS) | 0x40100000 | 0x40102000 | - | - | | 0x1FFE0000 | 0x1FFF0000 | - | - | - | - | RV40 | FRT | 0x407FB17C | 36 (0:32 - temp cal) | | ra6t2 | 0x00080000 | 0x0100A100 | 512 | MCU settings | - | - | | 0x08000000 | 0x08004000 | 0x010080F0 | 196 | 0:16 - Part Numbering Register 160:176 - UIDR 192 - MCUVE | - | - | 0x20000000 | 0x20010000 | 0x28000000 | 0x28000400 | RV40 | FL2 | - | - | The references: https://www.renesas.com/us/en/document/mah/ra6e2-group-users-manual-hardware?r=25411266 https://www.renesas.com/us/en/document/man/ra6m5-group-users-manual-hardware?r=1493931 https://www.renesas.com/us/en/document/mah/renesas-ra6m2-group-users-manual-hardware?r=1054161 https://www.renesas.com/us/en/document/man/ra6m4-group-user-s-manual-hardware?r=1333976 https://www.renesas.com/us/en/document/man/ra4m2-group-users-manual-hardware?r=1469026 https://www.renesas.com/us/en/document/mah/renesas-ra6m1-group-users-manual-hardware?r=1054156 https://www.renesas.com/us/en/document/mah/ra6e1-group-users-manual-hardware?r=1521986 https://www.renesas.com/us/en/document/mah/ra6t2-group-users-manual-hardware?r=1542426 https://www.renesas.com/us/en/document/mat/renesas-ra6t1-group-users-manual-hardware?r=1326271 https://www.renesas.com/us/en/document/man/renesas-ra4w1-group-users-manual-hardware?r=1054151 https://www.renesas.com/us/en/document/mah/renesas-ra4m1-group-users-manual-hardware?r=1054146 https://www.renesas.com/us/en/document/man/ra4m3-group-users-manual-hardware?r=1400806 https://www.renesas.com/us/en/document/mah/ra4e2-group-users-manual-hardware?r=25411256 https://www.renesas.com/us/en/document/mah/ra4e1-group-users-manual-hardware?r=1521991 https://www.renesas.com/us/en/document/mah/ra2l1-group-users-manual-hardware?r=1398061 https://www.renesas.com/us/en/document/man/ra2e1-group-users-manual-hardware?r=1469021 https://www.renesas.com/us/en/document/mah/ra2e2-group-users-manual-hardware?r=1532431 https://www.renesas.com/us/en/document/mah/renesas-ra2a1-group-users-manual-hardware?r=1054141 https://www.renesas.com/us/en/document/mah/ra6m3-group-users-manual-hardware
perigoso commented 2023年04月11日 23:41:15 +02:00 (Migrated from github.com)
Copy link

After reading quite a bit, and re-learning these MCUs, I'm questioning if the option-memory is something we should expose like this, as a flash block, and I'm guessing it was the reason I didn't include these memories on the initial implementation...
These memories are very special purpose, they contain reserved registers, read only registers, and registers with bits that cannot be set after clearing, that's a lot of potential for undesired behavior, and not at all what a user expects when dealing with a flash memory, potentially bricking devices in the worst cases.
The way I would maybe tackle this is with something like is being done on other MCUs like stm32 or sam3U, with dedicated commands to work with the options, that said I think the project would need some work, providing some sort of framework to do that, otherwise we end up with a lot of very specific and hard to maintain code like we already do...

After reading quite a bit, and re-learning these MCUs, I'm questioning if the option-memory is something we should expose like this, as a flash block, and I'm guessing it was the reason I didn't include these memories on the initial implementation... These memories are very special purpose, they contain reserved registers, read only registers, and registers with bits that cannot be set after clearing, that's a lot of potential for undesired behavior, and not at all what a user expects when dealing with a flash memory, potentially bricking devices in the worst cases. The way I would maybe tackle this is with something like is being done on other MCUs like stm32 or sam3U, with dedicated commands to work with the options, that said I think the project would need some work, providing some sort of framework to do that, otherwise we end up with a lot of very specific and hard to maintain code like we already do...
perigoso (Migrated from github.com) requested changes 2023年04月11日 23:44:46 +02:00
perigoso (Migrated from github.com) left a comment
Copy link

With my last comment in consideration, let's refactor this PR, reduce the scope. Let's add the necessary functionality to work with those pages, but not expose them as flashes to the user

With my last comment in consideration, let's refactor this PR, reduce the scope. Let's add the necessary functionality to work with those pages, but not expose them as flashes to the user
krzysztofgawrys commented 2023年04月12日 15:06:13 +02:00 (Migrated from github.com)
Copy link

I do not agree we should hide that memory from user, there are option settings that are essential for weird MCU startup configurations, I can agree on making part of this option-memory read-only in driver, with additional option to make them writable.

I do not agree we should hide that memory from user, there are option settings that are essential for weird MCU startup configurations, I can agree on making part of this option-memory read-only in driver, with additional option to make them writable.
perigoso commented 2023年04月12日 16:25:16 +02:00 (Migrated from github.com)
Copy link

What I'm suggesting is not hiding this from the user, just not expose it to them as part of the regular flash, but as a options command like on stm32 or sam3x, your suggestion also works, would be more akin to the kinetis security bytes

What I'm suggesting is not hiding this from the user, just not expose it to them as part of the regular flash, but as a options command like on stm32 or sam3x, your suggestion also works, would be more akin to the kinetis security bytes
krzysztofgawrys commented 2023年04月12日 23:21:28 +02:00 (Migrated from github.com)
Copy link

So, just tested something, e2 Studio produces load with option-settings included and that is understandable as IWDT, WDT, HOCO and Voltage Detection are part of this settings, it finds out we need to add option-setting flash like I did in this PR, otherwise gdb doesn't know what to do with this sections and fails immediately

(gdb) load
Loading section .option_setting_ofs, size 0x10 lma 0x100a100
Load failed

my aproach is that I will expose that flash but ignore writes for "one time programable" registers

So, just tested something, e2 Studio produces load with option-settings included and that is understandable as IWDT, WDT, HOCO and Voltage Detection are part of this settings, it finds out we need to add option-setting flash like I did in this PR, otherwise gdb doesn't know what to do with this sections and fails immediately > (gdb) load > Loading section .option_setting_ofs, size 0x10 lma 0x100a100 > Load failed my aproach is that I will expose that flash but ignore writes for "one time programable" registers
krzysztofgawrys (Migrated from github.com) reviewed 2023年04月12日 23:25:12 +02:00
krzysztofgawrys (Migrated from github.com) commented 2023年04月12日 23:25:11 +02:00
Copy link

not allowed to not add erase handler in driver, it will crash when trying to flash memory

#0 0x0000000000000000 in ?? ()
#1 0x000055555557a732 in target_flash_erase (t=0x5555555a9630, addr=16818432, len=64) at target/target_flash.c:149
#2 0x0000555555565320 in handle_v_packet (plen=29, packet=0x5555555a3630 "vFlashErase:0100a100,00000040") at gdb_main.c:624
#3 gdb_main_loop (tc=, pbuf=0x5555555a3630 "vFlashErase:0100a100,00000040", pbuf_size=, size=29, in_syscall=) at gdb_main.c:339
#4 0x000055555555c04b in bmp_poll_loop () at main.c:68
#5 main (argc=, argv=) at main.c:84

not allowed to not add erase handler in driver, it will crash when trying to flash memory > #0 0x0000000000000000 in ?? () > #1 0x000055555557a732 in target_flash_erase (t=0x5555555a9630, addr=16818432, len=64) at target/target_flash.c:149 > #2 0x0000555555565320 in handle_v_packet (plen=29, packet=0x5555555a3630 <pbuf> "vFlashErase:0100a100,00000040") at gdb_main.c:624 > #3 gdb_main_loop (tc=<optimized out>, pbuf=0x5555555a3630 <pbuf> "vFlashErase:0100a100,00000040", pbuf_size=<optimized out>, size=29, in_syscall=<optimized out>) at gdb_main.c:339 > #4 0x000055555555c04b in bmp_poll_loop () at main.c:68 > #5 main (argc=<optimized out>, argv=<optimized out>) at main.c:84
krzysztofgawrys (Migrated from github.com) reviewed 2023年04月12日 23:44:56 +02:00
krzysztofgawrys (Migrated from github.com) commented 2023年04月12日 23:44:56 +02:00
Copy link

@dragonmux do we have stub for such situations?

@dragonmux do we have stub for such situations?

We do not - you could modify target_flash_erase() to check the pointer for NULL before calling it (treating nullptr as returning true).

One note of feedback about the check done in the snippet above, because RENESAS_OF_START + RENESAS_OF_SIZE gives an address that is more than the address range of the region, you want the second comparison to just be addr < , not addr <=

We do not - you could modify `target_flash_erase()` to check the pointer for NULL before calling it (treating nullptr as returning `true`). One note of feedback about the check done in the snippet above, because `RENESAS_OF_START + RENESAS_OF_SIZE` gives an address that is more than the address range of the region, you want the second comparison to just be `addr < `, not `addr <= `
krzysztofgawrys commented 2023年04月14日 14:17:12 +02:00 (Migrated from github.com)
Copy link

here is compiled map is option-setting registers locations (excluding registers in code flash for some MCU series), red marked are registers with OTP, it seems we can allow write to all of option-setting flash except for three ranges
image

here is compiled map is option-setting registers locations (excluding registers in code flash for some MCU series), red marked are registers with OTP, it seems we can allow write to all of option-setting flash except for three ranges ![image](https://user-images.githubusercontent.com/3679311/232041014-eb9be472-10f5-44fc-ab0b-709529e94d9e.png)
perigoso (Migrated from github.com) requested changes 2023年04月14日 19:22:53 +02:00
perigoso (Migrated from github.com) commented 2023年04月14日 18:20:00 +02:00
Copy link

Don't use a macro/magic number, just set them to 0 and use size > 0 to check for it's existence

Don't use a macro/magic number, just set them to `0` and use `size > 0` to check for it's existence
perigoso (Migrated from github.com) commented 2023年04月14日 18:31:04 +02:00
Copy link
} renesas_flash_type_e;

see typedef-and-structure-enumeration-and-union-naming (yes, my original code is wrong too)

```suggestion } renesas_flash_type_e; ``` see [typedef-and-structure-enumeration-and-union-naming](https://github.com/blackmagic-debug/blackmagic/blob/main/src/target/HACKING.md#typedef-and-structure-enumeration-and-union-naming) (yes, my original code is wrong too)
perigoso (Migrated from github.com) commented 2023年04月14日 18:43:00 +02:00
Copy link
typedef enum {
	RENENESAS_LOCATION_FIXED1,
	RENENESAS_LOCATION_FIXED2,
	RENENESAS_LOCATION_FRT,
} renesas_location_e;

I think a more general naming here is better, as this affects more than the PNR location.
Same name style correction as above,.

```suggestion typedef enum { RENENESAS_LOCATION_FIXED1, RENENESAS_LOCATION_FIXED2, RENENESAS_LOCATION_FRT, } renesas_location_e; ``` I think a more general naming here is better, as this affects more than the PNR location. Same name style correction as above,.
perigoso (Migrated from github.com) commented 2023年04月14日 18:48:39 +02:00
Copy link

Can't we merge this into a single struct? I don't see advantage in having both renesas_ra_family_s and renesas_ra_family_details_s

Can't we merge this into a single struct? I don't see advantage in having both `renesas_ra_family_s` and `renesas_ra_family_details_s`
perigoso (Migrated from github.com) commented 2023年04月14日 18:56:42 +02:00
Copy link

This is a bit of a misnomer from Renesas (and me on the table), this is not much diferent from the other Option-Setting, from what I gather, with that said we can merge factory_flash_start/size into option_start/size_2, I think it wont conflict and we store less superfluous data

This is a bit of a misnomer from Renesas (and me on the table), this is not much diferent from the other Option-Setting, from what I gather, with that said we can merge `factory_flash_start/size` into `option_start/size_2`, I think it wont conflict and we store less superfluous data
perigoso (Migrated from github.com) commented 2023年04月14日 19:00:57 +02:00
Copy link
	for (size_t i = 0; i < ARRAY_LENGTH(renesas_ra_family); ++i) {

use ARRAY_LENGTH() (aka (sizeof(arr) / sizeof(arr[0]))) here to prevent mistakes, it's defined in general.h, and size_t for the type

```suggestion for (size_t i = 0; i < ARRAY_LENGTH(renesas_ra_family); ++i) { ``` use `ARRAY_LENGTH()` (aka `(sizeof(arr) / sizeof(arr[0]))`) here to prevent mistakes, it's defined in `general.h`, and `size_t` for the type
perigoso (Migrated from github.com) commented 2023年04月14日 19:09:20 +02:00
Copy link

replace _end addresses with size, I missed that In the table I shared, we might be able to store only 16b for sizes at least on some variables

replace `_end` addresses with size, I missed that In the table I shared, we might be able to store only 16b for sizes at least on some variables
@ -676,3 +798,1 @@
case PNR_SERIES_RA2A1:
case PNR_SERIES_RA4M1:
case PNR_SERIES_RA4W1:
switch (priv_storage->details.flash_type) {
perigoso (Migrated from github.com) commented 2023年04月14日 19:21:37 +02:00
Copy link

You can merge these two, AFAIR, the driver will be the same when time comes to implement it, and the extra debug info is not useful as we have already have the family, better so save memory and use 1 string for both

You can merge these two, AFAIR, the driver will be the same when time comes to implement it, and the extra debug info is not useful as we have already have the family, better so save memory and use 1 string for both
@ -884,2 +913,2 @@
default:
if (series != priv_storage->details.series) {
DEBUG_WARN("Found renesas chip (%.*s) with pnr location RENESAS_FIXED2_PNR and unsupported Part ID %x "
perigoso (Migrated from github.com) commented 2023年04月14日 19:19:36 +02:00
Copy link

Check that you got a valid family, we can't know that we have every one mapped

Check that you got a valid family, we can't know that we have every one mapped
perigoso (Migrated from github.com) commented 2023年04月14日 18:13:44 +02:00
Copy link

This breaks the algorithm, for this target it doesn't matter, but for a theoretical target that has contiguous flashes it could cause trouble, please do this check just before flash_prepare(), and do the address math and continue instead of doing a early return:

		/* Terminate flash operations if we're not in the same target flash */
		if (f != active_flash) {
			ret &= flash_done(active_flash);
			active_flash = f;
		}
		if (f->erase != NULL) {
			const target_addr_t local_start_addr = addr & ~(f->blocksize - 1U);
			const target_addr_t local_end_addr = local_start_addr + f->blocksize;
			if (!flash_prepare(f))
				return false;
			ret &= f->erase(f, local_start_addr, f->blocksize);
			if (!ret) {
				DEBUG_WARN("Erase failed at %" PRIx32 "\n", local_start_addr);
				break;
			}
			len -= MIN(local_end_addr - addr, len);
			addr = local_end_addr;
		} else {
			/* This flash does not implement an erase function, skip it */
			len -= MIN(f->length, len);
			addr = f->start + f->length;
		}
This breaks the algorithm, for this target it doesn't matter, but for a theoretical target that has contiguous flashes it could cause trouble, please do this check just before flash_prepare(), and do the address math and continue instead of doing a early return: ```suggestion /* Terminate flash operations if we're not in the same target flash */ if (f != active_flash) { ret &= flash_done(active_flash); active_flash = f; } if (f->erase != NULL) { const target_addr_t local_start_addr = addr & ~(f->blocksize - 1U); const target_addr_t local_end_addr = local_start_addr + f->blocksize; if (!flash_prepare(f)) return false; ret &= f->erase(f, local_start_addr, f->blocksize); if (!ret) { DEBUG_WARN("Erase failed at %" PRIx32 "\n", local_start_addr); break; } len -= MIN(local_end_addr - addr, len); addr = local_end_addr; } else { /* This flash does not implement an erase function, skip it */ len -= MIN(f->length, len); addr = f->start + f->length; } ```
perigoso commented 2023年04月14日 19:23:28 +02:00 (Migrated from github.com)
Copy link

Looking better :) good work

Looking better :) good work
krzysztofgawrys commented 2023年04月14日 20:27:41 +02:00 (Migrated from github.com)
Copy link

@perigoso @dragonmux

what did I broke?

*** stack smashing detected ***: terminated

Thread 1 "blackmagic" received signal SIGABRT, Aborted.
__GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
50 ../sysdeps/unix/sysv/linux/raise.c: No such file or directory.
(gdb) bt
#0 __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
#1 0x00007ffff7db4859 in __GI_abort () at abort.c:79
#2 0x00007ffff7e1f26e in __libc_message (action=action@entry=do_abort, fmt=fmt@entry=0x7ffff7f4908f "*** %s ***: terminated\n") at ../sysdeps/posix/libc_fatal.c:155
#3 0x00007ffff7ec1aba in __GI___fortify_fail (msg=msg@entry=0x7ffff7f49077 "stack smashing detected") at fortify_fail.c:26
#4 0x00007ffff7ec1a86 in __stack_chk_fail () at stack_chk_fail.c:24
#5 0x000055555557b97f in target_mem_read8 (t=t@entry=0x5555555ac630, addr=addr@entry=1082122256) at target/target.c:533
#6 0x000055555557a4c5 in renesas_rv40_error_check (t=t@entry=0x5555555ac630, error_bits=error_bits@entry=16384) at target/renesas.c:554
#7 0x000055555557a889 in renesas_rv40_prepare (f=0x5555555ac980) at target/renesas.c:610
#8 0x000055555557bf8e in flash_prepare (f=0x5555555ac980) at target/target_flash.c:93
#9 flash_prepare (f=0x5555555ac980) at target/target_flash.c:86
#10 target_flash_erase (t=0x5555555ac630, addr=0, len=32768) at target/target_flash.c:150
#11 0x0000555555565baf in handle_v_packet (plen=29, packet=0x5555555a6050 "vFlashErase:00000000,00008000") at gdb_main.c:624
#12 gdb_main_loop (tc=, pbuf=0x5555555a6050 "vFlashErase:00000000,00008000", pbuf_size=, size=29, in_syscall=) at gdb_main.c:339
#13 0x000055555555c07e in bmp_poll_loop () at main.c:68
#14 main (argc=, argv=) at main.c:84

@perigoso @dragonmux what did I broke? > *** stack smashing detected ***: terminated > > Thread 1 "blackmagic" received signal SIGABRT, Aborted. > __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50 > 50 ../sysdeps/unix/sysv/linux/raise.c: No such file or directory. > (gdb) bt > #0 __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50 > #1 0x00007ffff7db4859 in __GI_abort () at abort.c:79 > #2 0x00007ffff7e1f26e in __libc_message (action=action@entry=do_abort, fmt=fmt@entry=0x7ffff7f4908f "*** %s ***: terminated\n") at ../sysdeps/posix/libc_fatal.c:155 > #3 0x00007ffff7ec1aba in __GI___fortify_fail (msg=msg@entry=0x7ffff7f49077 "stack smashing detected") at fortify_fail.c:26 > #4 0x00007ffff7ec1a86 in __stack_chk_fail () at stack_chk_fail.c:24 > #5 0x000055555557b97f in target_mem_read8 (t=t@entry=0x5555555ac630, addr=addr@entry=1082122256) at target/target.c:533 > #6 0x000055555557a4c5 in renesas_rv40_error_check (t=t@entry=0x5555555ac630, error_bits=error_bits@entry=16384) at target/renesas.c:554 > #7 0x000055555557a889 in renesas_rv40_prepare (f=0x5555555ac980) at target/renesas.c:610 > #8 0x000055555557bf8e in flash_prepare (f=0x5555555ac980) at target/target_flash.c:93 > #9 flash_prepare (f=0x5555555ac980) at target/target_flash.c:86 > #10 target_flash_erase (t=0x5555555ac630, addr=0, len=32768) at target/target_flash.c:150 > #11 0x0000555555565baf in handle_v_packet (plen=29, packet=0x5555555a6050 <pbuf> "vFlashErase:00000000,00008000") at gdb_main.c:624 > #12 gdb_main_loop (tc=<optimized out>, pbuf=0x5555555a6050 <pbuf> "vFlashErase:00000000,00008000", pbuf_size=<optimized out>, size=29, in_syscall=<optimized out>) at gdb_main.c:339 > #13 0x000055555555c07e in bmp_poll_loop () at main.c:68 > #14 main (argc=<optimized out>, argv=<optimized out>) at main.c:84

Enable ASAN with make PROBE_HOST=hosted ASAN=1 ENABLE_DEBUG=1 and re-run to find out - from just "stack smashing" we can't know, but ASAN will trap the moment w/e bad thing happened happens and print proper stack traces and diagnostics on the bad write

Enable ASAN with `make PROBE_HOST=hosted ASAN=1 ENABLE_DEBUG=1` and re-run to find out - from just "stack smashing" we can't know, but ASAN will trap the moment w/e bad thing happened happens and print proper stack traces and diagnostics on the bad write
krzysztofgawrys commented 2023年04月16日 00:33:53 +02:00 (Migrated from github.com)
Copy link

@dragonmux here is report with ASAN enabled, strangely it shows DEBUG macro, it seems not related to what I'm doing in this PR, because when i commented out this whole debug section of send_recv the bug went away.
I think this deserves its own Issue.

==5469==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffd067a9871 at pc 0x562643bb0335 bp 0x7ffd067a96c0 sp 0x7ffd067a96b0
READ of size 1 at 0x7ffd067a9871 thread T0
 #0 0x562643bb0334 in send_recv platforms/hosted/bmp_libusb.c:437
 #1 0x562643bb2cfc in read_retry platforms/hosted/stlinkv2.c:355
 #2 0x562643bb2cfc in stlink_mem_read platforms/hosted/stlinkv2.c:871
 #3 0x562643b810a8 in adiv5_mem_read platforms/hosted/platform.c:642
 #4 0x562643b98ac8 in target_mem_read8 target/target.c:532
 #5 0x562643b95518 in renesas_rv40_error_check target/renesas.c:534
 #6 0x562643b9599d in renesas_rv40_prepare target/renesas.c:590
 #7 0x562643b99cd6 in flash_prepare target/target_flash.c:93
 #8 0x562643b99cd6 in flash_prepare target/target_flash.c:86
 #9 0x562643b99cd6 in target_flash_erase target/target_flash.c:147
 #10 0x562643b6d400 in handle_v_packet /home/kgawrys/blackmagic/src/gdb_main.c:624
 #11 0x562643b6d400 in gdb_main_loop /home/kgawrys/blackmagic/src/gdb_main.c:339
 #12 0x562643b56f70 in bmp_poll_loop /home/kgawrys/blackmagic/src/main.c:68
 #13 0x562643b56f70 in main /home/kgawrys/blackmagic/src/main.c:84
 #14 0x7f055ecc6082 in __libc_start_main ../csu/libc-start.c:308
 #15 0x562643b5781d in _start (/home/kgawrys/blackmagic/src/blackmagic+0x3181d)
Address 0x7ffd067a9871 is located in stack of thread T0 at offset 33 in frame
 #0 0x562643b989f3 in target_mem_read8 target/target.c:529
 This frame has 1 object(s):
 [32, 33) 'result' (line 530) <== Memory access at offset 33 overflows this variable
HINT: this may be a false positive if your program uses some custom stack unwind mechanism, swapcontext or vfork
 (longjmp and C++ exceptions *are* supported)
SUMMARY: AddressSanitizer: stack-buffer-overflow platforms/hosted/bmp_libusb.c:437 in send_recv
Shadow bytes around the buggy address:
 0x100020ced2b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
 0x100020ced2c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
 0x100020ced2d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
 0x100020ced2e0: 00 00 00 00 00 00 00 00 00 00 00 00 f1 f1 f1 f1
 0x100020ced2f0: 00 00 f3 f3 00 00 00 00 00 00 00 00 00 00 00 00
=>0x100020ced300: 00 00 00 00 00 00 00 00 00 00 f1 f1 f1 f1[01]f3
 0x100020ced310: f3 f3 00 00 00 00 00 00 00 00 00 00 00 00 f1 f1
 0x100020ced320: f1 f1 04 f3 f3 f3 00 00 00 00 00 00 00 00 00 00
 0x100020ced330: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
 0x100020ced340: 00 00 00 00 00 00 f1 f1 f1 f1 f1 f1 01 f2 04 f2
 0x100020ced350: 04 f2 04 f2 04 f2 04 f2 04 f2 04 f2 04 f2 04 f2
Shadow byte legend (one shadow byte represents 8 application bytes):
 Addressable: 00
 Partially addressable: 01 02 03 04 05 06 07
 Heap left redzone: fa
 Freed heap region: fd
 Stack left redzone: f1
 Stack mid redzone: f2
 Stack right redzone: f3
 Stack after return: f5
 Stack use after scope: f8
 Global redzone: f9
 Global init order: f6
 Poisoned by user: f7
 Container overflow: fc
 Array cookie: ac
 Intra object redzone: bb
 ASan internal: fe
 Left alloca redzone: ca
 Right alloca redzone: cb
 Shadow gap: cc
==5469==ABORTING
@dragonmux here is report with ASAN enabled, strangely it shows DEBUG macro, it seems not related to what I'm doing in this PR, because when i commented out this whole debug section of `send_recv` the bug went away. I think this deserves its own Issue. ```================================================================= ==5469==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffd067a9871 at pc 0x562643bb0335 bp 0x7ffd067a96c0 sp 0x7ffd067a96b0 READ of size 1 at 0x7ffd067a9871 thread T0 #0 0x562643bb0334 in send_recv platforms/hosted/bmp_libusb.c:437 #1 0x562643bb2cfc in read_retry platforms/hosted/stlinkv2.c:355 #2 0x562643bb2cfc in stlink_mem_read platforms/hosted/stlinkv2.c:871 #3 0x562643b810a8 in adiv5_mem_read platforms/hosted/platform.c:642 #4 0x562643b98ac8 in target_mem_read8 target/target.c:532 #5 0x562643b95518 in renesas_rv40_error_check target/renesas.c:534 #6 0x562643b9599d in renesas_rv40_prepare target/renesas.c:590 #7 0x562643b99cd6 in flash_prepare target/target_flash.c:93 #8 0x562643b99cd6 in flash_prepare target/target_flash.c:86 #9 0x562643b99cd6 in target_flash_erase target/target_flash.c:147 #10 0x562643b6d400 in handle_v_packet /home/kgawrys/blackmagic/src/gdb_main.c:624 #11 0x562643b6d400 in gdb_main_loop /home/kgawrys/blackmagic/src/gdb_main.c:339 #12 0x562643b56f70 in bmp_poll_loop /home/kgawrys/blackmagic/src/main.c:68 #13 0x562643b56f70 in main /home/kgawrys/blackmagic/src/main.c:84 #14 0x7f055ecc6082 in __libc_start_main ../csu/libc-start.c:308 #15 0x562643b5781d in _start (/home/kgawrys/blackmagic/src/blackmagic+0x3181d) Address 0x7ffd067a9871 is located in stack of thread T0 at offset 33 in frame #0 0x562643b989f3 in target_mem_read8 target/target.c:529 This frame has 1 object(s): [32, 33) 'result' (line 530) <== Memory access at offset 33 overflows this variable HINT: this may be a false positive if your program uses some custom stack unwind mechanism, swapcontext or vfork (longjmp and C++ exceptions *are* supported) SUMMARY: AddressSanitizer: stack-buffer-overflow platforms/hosted/bmp_libusb.c:437 in send_recv Shadow bytes around the buggy address: 0x100020ced2b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x100020ced2c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x100020ced2d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x100020ced2e0: 00 00 00 00 00 00 00 00 00 00 00 00 f1 f1 f1 f1 0x100020ced2f0: 00 00 f3 f3 00 00 00 00 00 00 00 00 00 00 00 00 =>0x100020ced300: 00 00 00 00 00 00 00 00 00 00 f1 f1 f1 f1[01]f3 0x100020ced310: f3 f3 00 00 00 00 00 00 00 00 00 00 00 00 f1 f1 0x100020ced320: f1 f1 04 f3 f3 f3 00 00 00 00 00 00 00 00 00 00 0x100020ced330: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x100020ced340: 00 00 00 00 00 00 f1 f1 f1 f1 f1 f1 01 f2 04 f2 0x100020ced350: 04 f2 04 f2 04 f2 04 f2 04 f2 04 f2 04 f2 04 f2 Shadow byte legend (one shadow byte represents 8 application bytes): Addressable: 00 Partially addressable: 01 02 03 04 05 06 07 Heap left redzone: fa Freed heap region: fd Stack left redzone: f1 Stack mid redzone: f2 Stack right redzone: f3 Stack after return: f5 Stack use after scope: f8 Global redzone: f9 Global init order: f6 Poisoned by user: f7 Container overflow: fc Array cookie: ac Intra object redzone: bb ASan internal: fe Left alloca redzone: ca Right alloca redzone: cb Shadow gap: cc ==5469==ABORTING

Agreed, that should be its own separate issue as a bug in BMDA and the ST-Link backend. If you could open a new issue for that and also test and see if it reproduces with feature/improved-debug-logging (in case we've accidentally fixed it.. but we doubt that) that'd be fantastic.

Agreed, that should be its own separate issue as a bug in BMDA and the ST-Link backend. If you could open a new issue for that and also test and see if it reproduces with feature/improved-debug-logging (in case we've accidentally fixed it.. but we doubt that) that'd be fantastic.
krzysztofgawrys (Migrated from github.com) reviewed 2023年04月17日 01:33:46 +02:00
krzysztofgawrys (Migrated from github.com) commented 2023年04月17日 01:33:45 +02:00
Copy link

20ms was way too short, I ended up on 100ms, tested 50 gave me errors, 75 was stable, but I could only test RA4M2, not sure what other series would behave like, also this timeout should be as high as we feel good with it, this is just timeout, there is RV40_FSTATR_RDY check afterwards anyway, so we are not making things go slow.

20ms was way too short, I ended up on 100ms, tested 50 gave me errors, 75 was stable, but I could only test RA4M2, not sure what other series would behave like, also this timeout should be as high as we feel good with it, this is just timeout, there is `RV40_FSTATR_RDY` check afterwards anyway, so we are not making things go slow.
krzysztofgawrys (Migrated from github.com) reviewed 2023年04月17日 01:35:05 +02:00
krzysztofgawrys (Migrated from github.com) commented 2023年04月17日 01:35:05 +02:00
Copy link

you solution is working, using it

you solution is working, using it
perigoso (Migrated from github.com) reviewed 2023年04月17日 11:45:01 +02:00
perigoso (Migrated from github.com) commented 2023年04月17日 11:45:01 +02:00
Copy link

Theres a mistake here on my part I forgot to edit out, in the else block, we should do some more arithmetic for correctness, It's a bit pedantic but I want to make sure this is correct (if we start at the middle of the flash, we want to subtract only what we theoretically erased, not the full size anyway)

		} else {
			/* This flash does not implement an erase function, skip it */
			len -= MIN((f->start + f->length) - addr, len);
			addr = f->start + f->length;
		}
Theres a mistake here on my part I forgot to edit out, in the else block, we should do some more arithmetic for correctness, It's a bit pedantic but I want to make sure this is correct (if we start at the middle of the flash, we want to subtract only what we theoretically erased, not the full size anyway) ```c } else { /* This flash does not implement an erase function, skip it */ len -= MIN((f->start + f->length) - addr, len); addr = f->start + f->length; } ```
perigoso (Migrated from github.com) reviewed 2023年04月17日 11:48:27 +02:00
@ -281,16 +378,18 @@ typedef enum {
#define RV40_FCPSR_ESUSPMD 1U
perigoso (Migrated from github.com) commented 2023年04月17日 11:48:27 +02:00
Copy link

now that renesas_family_s also holds the renesas_pnr_series_e we are storing redundant information, we can remove series here and refactor the code to use the details

now that `renesas_family_s` also holds the `renesas_pnr_series_e` we are storing redundant information, we can remove `series` here and refactor the code to use the `details`
krzysztofgawrys (Migrated from github.com) reviewed 2023年04月17日 12:08:19 +02:00
@ -281,16 +378,18 @@ typedef enum {
#define RV40_FCPSR_ESUSPMD 1U
krzysztofgawrys (Migrated from github.com) commented 2023年04月17日 12:08:18 +02:00
Copy link

agreed

agreed
krzysztofgawrys (Migrated from github.com) reviewed 2023年04月17日 13:13:02 +02:00
krzysztofgawrys (Migrated from github.com) commented 2023年04月17日 13:13:02 +02:00
Copy link

factory flash contains registers for FIXED2_PNR, UID and MCUVER on some MCUs, other do have FRT, I think this could be also redone to get address of there registers from family table and do not keep them as defines

factory flash contains registers for FIXED2_PNR, UID and MCUVER on some MCUs, other do have FRT, I think this could be also redone to get address of there registers from family table and do not keep them as defines
krzysztofgawrys (Migrated from github.com) reviewed 2023年04月17日 13:21:26 +02:00
krzysztofgawrys (Migrated from github.com) commented 2023年04月17日 13:21:25 +02:00
Copy link

factory flash aligns only with FIXED2 registers

factory flash aligns only with FIXED2 registers
perigoso commented 2023年05月03日 11:40:30 +02:00 (Migrated from github.com)
Copy link

Sorry I left you hanging @krzysztofgawrys, I have been very busy with work, I will try to look at this soon

Sorry I left you hanging @krzysztofgawrys, I have been very busy with work, I will try to look at this soon
perigoso (Migrated from github.com) reviewed 2023年05月04日 00:52:43 +02:00
perigoso (Migrated from github.com) left a comment
Copy link

This is generally looking good, @dragonmux might be the time you start to take a look at this as I don't have much to add

I will note that the commits are a bit of a mess and need to be cleaned up before we consider merging

This is generally looking good, @dragonmux might be the time you start to take a look at this as I don't have much to add I will note that the commits are a bit of a mess and need to be cleaned up before we consider merging
perigoso (Migrated from github.com) commented 2023年05月04日 00:45:33 +02:00
Copy link

Are you planning on implementing this?

Are you planning on implementing this?
perigoso (Migrated from github.com) commented 2023年05月04日 00:45:59 +02:00
Copy link

variables are being used

variables are being used

Well, before we dive into reviewing the code, the one thing that need fixing from our side is the PR needs rebasing against main and the contribution guidelines on commit messages followed

You can do this by running git rebase -i main and marking every commit 'r' instead of 'pick' for rewording. That'll also get rid of the merge commits

Well, before we dive into reviewing the code, the one thing that need fixing from our side is the PR needs rebasing against main and the contribution guidelines on commit messages followed You can do this by running `git rebase -i main` and marking every commit 'r' instead of 'pick' for rewording. That'll also get rid of the merge commits
dragonmux left a comment
Copy link

First off, thank you for the contribution! This is looking decent but there are a few things that need addressing in addition to Perigoso's outstanding review comments before we can merge this.

First off, thank you for the contribution! This is looking decent but there are a few things that need addressing in addition to Perigoso's outstanding review comments before we can merge this.
@ -94,0 +132,4 @@
/* For details please check https://github.com/blackmagic-debug/blackmagic/pull/1450#issuecomment-1508416224 */
renesas_restricted_setting_s renesas_restricted_setting[] = {
{0x0100a130, 0x0100a168},

The order things appear in this structure result in a lot of packing fields being inserted - for example after each uint16_t. Please reorder the fields to put the size information after the start information to reduce the Flash footprint it causes. See the -Wpacked compiler flag, or clang-tidy's lints for more information.

The order things appear in this structure result in a lot of packing fields being inserted - for example after each uint16_t. Please reorder the fields to put the size information after the start information to reduce the Flash footprint it causes. See the `-Wpacked` compiler flag, or clang-tidy's lints for more information.

It would be better for this to construct details on when it's actually needed and not on every call by doing:

	return (renesas_family_s){0};
It would be better for this to construct `details` on when it's actually needed and not on every call by doing: ```suggestion return (renesas_family_s){0}; ```

This branch of the if statement will always execute on the first loop through where the if condition doesn't match, making this loop always execute just once - drop the else. Also, please put braces on the for body (but not the if)

This branch of the if statement will always execute on the first loop through where the if condition doesn't match, making this loop always execute just once - drop the else. Also, please put braces on the for body (but not the if)

if priv_storage is always allocated in the probe function and is already NULL checked there, it cannot be NULL here, drop the check.

if priv_storage is always allocated in the probe function and is already NULL checked there, it cannot be NULL here, drop the check.

Same comment about impossible NULL as above

Same comment about impossible NULL as above

This looks like it's worth lifting into its own function so you can say const uint16_t block_size = renesas_rv40_block_size(f->blocksize, addr); or so - this will improve codegen and const-correctness

This looks like it's worth lifting into its own function so you can say `const uint16_t block_size = renesas_rv40_block_size(f->blocksize, addr);` or so - this will improve codegen and `const`-correctness

Drop the braces on this if and the one in the while loop - they're unnecessary.

Drop the braces on this if and the one in the while loop - they're unnecessary.

PRIx16 doesn't do anything special here, please stick to using %x.

`PRIx16` doesn't do anything special here, please stick to using `%x`.

arglen here should be a size_t as this is the result of strlen()

arglen here should be a size_t as this is the result of strlen()

Please avoid VLAs. uint8_t data[16U]; and then when you need the array length, use the ARRAY_LENGTH() macro

Please avoid VLAs. `uint8_t data[16U];` and then when you need the array length, use the `ARRAY_LENGTH()` macro

i += 4U should be a part of this loop statement, not in the body of the loop. All constants used here should have the L suffix dropped especially as that might not even result in the right integer type (like in the uint8_t assignment above, where -Wconversion would warn about it truncating):

		for (size_t i = 0; i < ARRAY_LENGTH(data); i += 4U) {
`i += 4U` should be a part of this loop statement, not in the body of the loop. All constants used here should have the L suffix dropped especially as that might not even result in the right integer type (like in the uint8_t assignment above, where `-Wconversion` would warn about it truncating): ```suggestion for (size_t i = 0; i < ARRAY_LENGTH(data); i += 4U) { ```

Same comment as above about the loop increment value. Additionally, drop the braces from the loop and use read_le2() from the buffer_utils.h header for constructing the values to pass to target_mem_write16().

Same comment as above about the loop increment value. Additionally, drop the braces from the loop and use `read_le2()` from the buffer_utils.h header for constructing the values to pass to `target_mem_write16()`.

This file is missing the final newline, please add it (that is what the "no entry" icon is about in the GitHub interface)

This file is missing the final newline, please add it (that is what the "no entry" icon is about in the GitHub interface)
This pull request has changes conflicting with the target branch.
  • src/target/renesas_ra.c
  • src/target/target_flash.c
View command line instructions

Manual merge helper

Use this merge commit message when completing the merge manually.

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin krzysztofgawrys/fix/renesas-optionsetting-flash:krzysztofgawrys/fix/renesas-optionsetting-flash
git switch krzysztofgawrys/fix/renesas-optionsetting-flash
Sign in to join this conversation.
No reviewers
Labels
Clear labels
BMD App
Black Magic Debug App (aka. PC hosted) (not firmware)
BMP Firmware
Black Magic Probe Firmware (not PC hosted software)
Bug
Confirmed bug
Build system
Build system
Can't reproduce
Maintainers can't reproduce this problem
CI
Continuous Integration System
Contribution wanted
User contributions welcome
Documentation
Project documentation
Draft
Work in progress draft
Duplicate
This issue or pull request already exists
Enhancement
General project improvement
Feedback wanted
Requires additional submitter feedback
Foreign Host Board
Non Native hardware to runing Black Magic firmware on
GDB
Issue/PR related to GDB
Good first issue
Good for newcommers
HwIssue Mitigation
Solving or mitigating a Hardware issue in Software
Information Needed
Maintainers need more information
NativeHardware
Official Black Magic Debug Hardware
New Host Board
New hardware to run Black Magic firmware on
New Target
New debug target
Off Topic
Something that does not involve the project in any way
Potential Bug
A potential, unconfirmed or very special circumstance bug
Regression
Bug caused by a regression
User Interest Needed
More user interest required before consideration
User Testing Needed
Looking for user testing reports
Won't fix
Outside of the project scope or works as intended
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
blackmagic-debug/blackmagic!1450
Reference in a new issue
blackmagic-debug/blackmagic
No description provided.
Delete branch "krzysztofgawrys/fix/renesas-optionsetting-flash"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?