Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

In Review: Support WeAct mini stm32h7 #144

Merged
merged 3 commits into from
Aug 4, 2023

Conversation

bhav97
Copy link
Contributor

@bhav97 bhav97 commented Sep 5, 2021

I'm trying to add support for WeAct MiniSTM32H7xx

Pending

  • board_self_update
  • board_flash_erase_app
  • board_flash_write
  • board_flash_read
    board_flash_flush
  • board_flash_init
  • board_flash_size
  • board_app_valid

@hathach
Copy link
Member

hathach commented Sep 6, 2021

thank you for your effort along with the pending list. For external flash partition make sure you follow the circuitpython stm32 partition scheme https://github.com/adafruit/circuitpython/tree/main/ports/stm/boards . It will require qspi driver to flash external flash, a new port may take quite a bit of time. I will try to help whenever I could. Though currently I don't have this board and it is hard to get anything delivered to my location due to strict lockdown.

@bhav97
Copy link
Contributor Author

bhav97 commented Sep 6, 2021

I would appreciate any help I can get.

For external flash partition make sure you follow the circuitpython stm32 partition scheme https://github.com/adafruit/circuitpython/tree/main/ports/stm/boards

Looking at the circuitpython repo, I am still quite unsure of what the stm32 partition scheme is and how it would affect tinyuf2. I'm going to try and get the QSPI and SPI memories working first and then continue with the backlog. Ideally I would want all of the memory devices to be available for flash download. (internal + SPI + QSPI)

@bhav97
Copy link
Contributor Author

bhav97 commented Sep 12, 2021

I have added SPI and QSPI W25Qx drivers from the MiniSTM32H7xx repository. Still in early stages but you can somewhat load code into the QSPI memory and jump to it. I tried loading a simply blinky program to the QSPI flash. The interrupts seem to break after jumping to the QSPI code.

  1. Implemented: board_flash_init which initializes the SPI flash and disables memory mapped mode on the QSPI flash to allow writes

  2. Implemented: board_flash_size returns 8MB. (Even though the total flash size if 8MB+8MB+64KB). But everything works at the moment.

  3. Paritally Implemented: board_app_valid checks if the Reset_Handler lies in valid flash memory. Stack pointer verification is still pending.

  4. Partially Implemented: board_flash_read/board_flash_write maps memory into 3 segments

    • QSPI Flash: 0x9000_0000U - 0x907F_FFFF (8MB)
    • SPI Flash: 0x6000_0000U - 0x607F_FFFF (8MB)
    • PFLASH: 0x0801_0000 - 0x0801_FFFF (64KB)

The initial 64KB is for tinyuf2. Programming the PFLASH is still pending since it would require running off of RAM and I'm struggling with getting the board to load code into RAM.

@bhav97
Copy link
Contributor Author

bhav97 commented Oct 9, 2021

Hey @hathach

Could you check if I'm creating uf2 files right? The code is here

@hathach
Copy link
Member

hathach commented Oct 13, 2021

Hey @hathach

Could you check if I'm creating uf2 files right? The code is here

yeah, I think your command is correct, you could try to enable the logging and print out the received address and contents and compare against the bin file to see if it matches. Just fake the actual flashing for testing.
e.g https://github.com/adafruit/tinyuf2/blob/master/ports/stm32f4/board_flash.c#L124

@bhav97
Copy link
Contributor Author

bhav97 commented Oct 14, 2021

Thanks! I finally figured out why the interrupts were not working.

The default SystemInit restores the VTOR value when the application boots. I couldn't get interrupts to work by modifying SystemInit (maybe vector table in QSPI is a limitation?), so I modified the the application linker script to load the vector table into AXISRAM.

The bootloader works for QSPI flash now, if you compile your application with VECT_TAB_SRAM :)

@bhav97
Copy link
Contributor Author

bhav97 commented Oct 17, 2021

I have most things working and implemented now, but there are a few things to test (writing data to SPI flash, self-update and uf2 read). I could finish the remaining work along with the review.

The flash driver code isn't very pretty but it works, so I will leave it as is till I can find a portable working replacement. Also the port doesn't handle PFLASH right now (since my board only has the single sector, single bank variant)

@bhav97 bhav97 marked this pull request as ready for review October 17, 2021 20:15
@DuMaM DuMaM mentioned this pull request Dec 11, 2021
@elpekenin
Copy link

elpekenin commented Jul 25, 2023

I know there hasn't been any movement out here for 2 years, but i recently bought this board and would like having uf2 on it.

  • Is there something missing on the code? I could try and implement myself
  • Any idea why the firmware worked a couple days back (screen drawing + storage showing up), but it doesnt do either now?
    • Flashed other fimrware via stm32dfu between tests
    • Blue LED (E3) does light both when USB is connected and pressing RST button

Waiting for a STLink to get delivered, can't provide further info for now

@hathach
Copy link
Member

hathach commented Jul 26, 2023

I am not entirely sure as well, PR is still marked as WIP and I kind of forgot about this. @bhav97 do you think this is ready for review and/or merging ?

@bhav97
Copy link
Contributor Author

bhav97 commented Jul 26, 2023

@hathach , @elpekenin
Unfortunately the project I was preparing this for didn't work out and I completely forgot about this WIP-PR.

As for the implementation, all the board_* callouts are available. I believe we can start a review now.

In general I have 2 concerns for this PR

  1. I created the ports/stm32h7 but right now there's only a single board that uses this variant. There are other variants that have a different sector size and my additions may need some work to make life easier for people who might want to extend tinyuf2 support in the future.

  2. The display and QSPI code is straight out of the WeAct repo and is not documented very well


Any idea why the firmware worked a couple days back (screen drawing + storage showing up), but it doesnt do either now?
Flashed other fimrware via stm32dfu between tests
Blue LED (E3) does light both when USB is connected and pressing RST button

I believe stm32dfu doesn't support programming external memory so its possible that tinyuf2 was overwritten. (The MCU variant on the board only has a single flash sector of 128K so its not really possible to program the "internal" flash without overwriting tinyuf2. )
You should be able to load firmware to the "external" QSPI flash and RAM just fine.

@elpekenin
Copy link

elpekenin commented Jul 26, 2023

tinyuf2 was overwritten

Sure, but i mean it doesnt work after flashing the tinyuf2.bin back at it. Which doesnt make any sense in my potato brain

@bhav97
Copy link
Contributor Author

bhav97 commented Jul 26, 2023

That's not good. Does the blue LED stay on? Does double tap bring up the display?

@elpekenin
Copy link

elpekenin commented Jul 26, 2023

That's not good. Does the blue LED stay on? Does double tap bring up the display?

Can double check later but Led blinked on both events (plugging wire and pressing button), screen did nothing (not even backlight i think)

Nothing in double press of either button

@elpekenin
Copy link

Went to the mk file to add -DRAMCODE and give that a try, didnt work either.
But thanks to that i realized there is -flto, disabled it and everything seems to be working as intended again, weird as heck.
Will now try and compile a .uf2 and report whether loading user's code works

ports/stm32h7/boards/mini_stm32h7xx_v11/board.h Outdated Show resolved Hide resolved

# Compiler flags
CFLAGS += \
-flto \

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think LTO is what "broke" code for me (probably gcc-version dependant issue if it worked for you...), do we really need to optimize tinyuf2 code if we aren't using the built-in flash?
Perhaps individual boards using H7xx could opt-in this config, instead of "forcing" its use having it at the MCU level

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good idea, I copied the F4-mkfiles for this port, so there maybe a few more gotchas.
what version of GCC are you compiling with?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

10.3.1 (from ubuntu on WSL2 if that matters)

elpekenin@elPeCenin:~$ arm-none-eabi-gcc --version
arm-none-eabi-gcc (15:10.3-2021.07-4) 10.3.1 20210621 (release)

I cant get the flashed .uf2 to run, i just get back to bootloader every single time, do you happen to know what im doing wrong here? https://github.com/elpekenin/qmk_firmware/blob/579e150c72899e78f06e42aefc27d1b71dcaf7ba/keyboards/handwired/onekey/weact_stm32h750vbt6/ld/STM32H750xB_qspi_tinyuf2.ld. Based this off the .ld on ChibiOS and did also take a look on a .ld on your GitHub

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

arm-none-eabi-gcc (15:10.3-2021.07-4) 10.3.1 20210621 (release)

vs

Using built-in specs.
COLLECT_GCC=arm-none-eabi-gcc
COLLECT_LTO_WRAPPER=~/Workspace/toolchains/gcc-arm-none-eabi-10.3-2021.10/bin/../lib/gcc/arm-none-eabi/10.3.1/lto-wrapper
Target: arm-none-eabi
Configured with:
Thread model: single
Supported LTO compression algorithms: zlib
gcc version 10.3.1 20210824 (release) (GNU Arm Embedded Toolchain 10.3-2021.10)

Definitely seems like something LTO -related is different between our toolchains.


I'm not sure how qmk is laid out in memory, but in your ld, can you try creating 2 memory regions?
Something like

    flash0 : org = 0x90000000, len = 0x300
    flash1 : org = 0x90000300, len = 8m-0x300

to explicitly ensure that the vector table comes first

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The build should output a .map file to show you the final memory layout. The vector table should have its out input section to ensure it is placed correctly.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like we should move this to Discord or at least an issue, to stop cluttering the PR (sorry), please answer this message in any way and @ me into it. Unless you think it's better staying here, that is.

The map file looks correct(ish)? Idk, im as noob as you can be with ld stuff....
https://gist.github.com/elpekenin/95eab3c7831187b86f29b983050e5a4e (line 2551)

@bhav97
Copy link
Contributor Author

bhav97 commented Jul 27, 2023

@hathach I have rebased over the latest changes, cleaned up the code and history

@bhav97 bhav97 changed the title WIP: Support WeAct mini stm32h7 In Review: Support WeAct mini stm32h7 Jul 27, 2023
@hathach
Copy link
Member

hathach commented Jul 28, 2023

@hathach I have rebased over the latest changes, cleaned up the code and history

superb thank you very much, I will review this as soon as I could. There is no problem with H7 having only this as board. We can refactor this later on when new board is added. Thank you very much for updating the PR.

Comment on lines +126 to +143
#if BOARD_QSPI_FLASH_EN
// addr += QSPI_BASE_ADDR;
if (IS_QSPI_ADDR(addr))
{
(void) W25qxx_Read(data, addr - QSPI_BASE_ADDR, len);
return;
}
#endif

#if BOARD_AXISRAM_EN
if (IS_AXISRAM_ADDR(addr) && IS_AXISRAM_ADDR(addr + len - 1))
{
memcpy(data, (void *) addr, len);
return;
}
#endif // BOARD_AXISRAM_EN

if (IS_PFLASH_ADDR(addr))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing SPI check here, only doing QSPI/RAM/InternalFlash
I found this while playing around with the code to try and get my .uf2 to run...
Will try and double-check for similar missing pieces as soon as i get this working...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

board_flash_read is used to provide data for the CURRENT.uf2, but it's not really possible to target multiple memories for a readout. (maybe if we readout internal/QSPI/SPI flash and then concatenate all data into CURRENT.uf2)
Internally within the port it's used to read the vector table, and the SPI flash cannot contain executables, so I left it out of board_flash_read.

Copy link
Member

@hathach hathach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all changes look great, this PR definitely needs lots of work. I actualy don't have the hardware to test with. Therefore I could only go through changes only and so far so good. There is only minor requests

  • suppress warnings in default handler
  • rename the board from mini_stm32hxx_v11 to mini_stm32h750_v11. WeAct has 2 version, though the H743 variant will likely to use on-chip flash for firmware instead of off-chip spi/qspi one. They are different mcus anyway, so it is better to specify the name.

Indeed, there maybe some work to also add on-chip flash support for mcu such as h743, however, I think this is too good to merge now provided it works (since I couldn't test). And we could worry about other MCU later on.


__attribute__((optimize("O0")))
void fault_handler(sFrame *fr)
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add (void) fr; to suppress unused warning

@hathach
Copy link
Member

hathach commented Aug 4, 2023

@bhav97 if you are busy, I could make the review changes myself so that we could finally merge this :)

@bhav97
Copy link
Contributor Author

bhav97 commented Aug 4, 2023

@hathach I have made the requested changes but the underlying issue from #342 is still present. I'll open another PR when I figure it out.

Thank you :)

Copy link
Member

@hathach hathach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perfect, thank you very much for your work and patient. No worry about the other issue, this PR already done an amazing lots of works. We can always fix it later, and/or add support for on-flash, refactor etc...

@hathach hathach merged commit e88c6d6 into adafruit:master Aug 4, 2023
101 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants