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

Support for STM32WB55xx #458

Closed
wants to merge 3 commits into from

Conversation

newbrain
Copy link
Contributor

@newbrain newbrain commented Mar 8, 2019

Here is the STM32WB support promised long ago...
What is done:

  • Flashing, erasing, debugging work as expected.
  • Option command improved error checks: number format, right number of parameter.
    Slightly changed behaviour (saner, IMO):
    With no parameters, print the options words. With erase resets them to defaults. With write only the provided words are written in the options, the remaining ones are left untouched instead of being read and rewritten.
  • New CPU2option command: changes the boot address and memory start for CPU2. Probably working only on preproduction HW: @zyp to be checked on final HW!
  • A new erase_page command has been added:
    monitor erase_page <address> [number of pages]
    it is written to be independent of the MCU, it relies only on info from the flash data structure, see discussion at STM32WB support #457

What is not there - and probably cannot be easily implemented (some docs are still not available):

  • The possibility to lock/unlock the chip security (ESE/FSD bits); the final RM does not make any mention of the procedure, and according to it the chips come pre-secured from factory (it was different in an early version I have): the code just leave those alone.
  • A way to flash the radio FW: while the pre-prod HW is "open", for the final one it seems you need to use the bootloader or OTA update (my HW does not have any bootloader, so I can't even try that).

This is a largish commit: though the structure was prepared in #456, still the changes are spread all over, mostly due to the need of parametrize the flash controller base register.

@UweBonnes
Copy link
Contributor

Probably you have a CPU marked "ES". I will try out the PR beginning next week and report back,

Copy link
Contributor

@UweBonnes UweBonnes left a comment

Choose a reason for hiding this comment

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

Unrelated change.

@newbrain
Copy link
Contributor Author

newbrain commented Mar 8, 2019 via email

@UweBonnes
Copy link
Contributor

The metal shield, is plugged, at least on my parts. Look again.

@zyp
Copy link
Contributor

zyp commented Mar 9, 2019

My Nucleo kit arrived today, so I've had the opportunity to test this now.

  • As evidenced by the blinky currently running on my device, erasing, flashing and debugging works fine.
  • Like expected, CPU2opt command can only read; write and erase appears to just be ignored. At this point it's pretty much just an alias for «x/2wx 0x58004080» which doesn't seem to justify its existence.
  • The page erase command works fine, but I propose changing the interface. The mix of start address and number of pages feels inconsistent. Replacing the number of pages with an optional end address would be more consistent and convenient, especially later if it gets ported to devices with several page sizes (e.g. f4).
  • Like the RM says, CPU1 is not allowed to mass erase the chip. The command silently fails.

Other observations:

  • The metal shields are just clipped in place, and even has a label stating «REMOVABLE SHIELD».
  • Loading and decrypting the radio firmware is managed by FUS/RSS, a CPU2 firmware located in the upper 40k of the flash. STM32CubeProgrammer can talk to it through the DFU bootloader. I expect the yet unreleased AN5185 will document how to talk to it, so we can revisit the question of whether it makes sense to add support for loading the radio firmware when it's released.
  • I suspect the procedure to unlock the chip security still might work, but since it involves a full chip erase, it'll also remove the FUS/RSS firmware, which kinda bricks the radio functionality since you won't be able to load the encrypted radio firmware anymore. I'll probably try this at some later point when I have any disposable devices.

@newbrain
Copy link
Contributor Author

Thanks @UweBonnes and @zyp for your comments and info!

Code comments:

  • Like expected, CPU2opt command can only read; write and erase appears to just be ignored. At this point it's pretty much just an alias for «x/2wx 0x58004080» which doesn't seem to justify its existence.

Ok, this is also as expected, in this light I'll remove all the strictly sample dependent stuff (I'll just keep it for my use), including the CPU2options command.

  • The page erase command works fine, but I propose changing the interface. The mix of start address and number of pages feels inconsistent. Replacing the number of pages with an optional end address would be more consistent and convenient, especially later if it gets ported to devices with several page sizes (e.g. f4).

The L4 itself has different page sizes, and I agree with the proposal (I'm changing the code now). Thanks.

Unrelated change.

I think I have many, some is formatting (I don't like to see hex numbers with a mix of upper and lower case), and some is more substantial (option handling).
I will revert the option handling if you thinks it's "too much"; it's much less warranted now, as I'm removing the CPU2options command, though I maintain that better error checking is an added value.

The page erase function is just missing a "static", so as to be accessible from other targets
Please let me know whether that's ok, or it's better to move it in, say, command.c: it's needed here to manually erase the chip leaving the radio FW alone, as @zyp confirms.

Other:

The metal shield, is plugged, at least on my parts. Look again.

The metal shield is, in fact, removable. No label on mine, though.
I did not examine it very closely, and mistook the clips for solder (we were told at the WS that the shield was permanently fixed).
The chip is marked ES32WB55, as expected.
WB55

  • Loading and decrypting the radio firmware is managed by FUS/RSS, a CPU2 firmware located in the upper 40k of the flash. STM32CubeProgrammer can talk to it through the DFU bootloader. I expect the yet unreleased AN5185 will document how to talk to it, so we can revisit the question of whether it makes sense to add support for loading the radio firmware when it's released.

Yes, I'll check my boards, but I suspect I haven't got it. But I can freely load everywhere in flash (ESE == 0).

@UweBonnes
Copy link
Contributor

Perhaps one "no-code change only cleanup commit" and one commit introducing the new feature. I think that works out clearest what changed. But no hard rules on that, only a proposal. What about putting and exporting the page erase function in command.c?

@zyp
Copy link
Contributor

zyp commented Mar 11, 2019

I second putting cosmetic changes in a separate commit, makes the revision history easier to browse later. A PR can contain more than one commit, so even if you split up unrelated changes, you can still submit them all at once.

Today I tried the command for editing option bytes and couldn't get it to write anything, so either I'm not understanding how to use it correctly or it doesn't work properly. Poking at the flash registers directly to make the same changes worked, so it's not that I'm not permitted to make them.

I also just had to try turning off the security to see if it was possible, and have apparently bricked the device harder than I expected to. The aforementioned full chip erase apparently went through and the ESE bit at 0x1fff8000 reads 0, but the ESE bit in FLASH_OPTR remains at 1 and everything else still appears locked. Since I still can't read the top area of the flash, I can't say for sure whether I erased it, but STM32CubeProgrammer doesn't manage to connect to FUS anymore.

In other words, I've now got a device where I apparently lost the ability to load the encrypted firmware binaries, and I still didn't gain the ability to load my own code for CPU2. I don't recommend attempting the same. :)

@UweBonnes
Copy link
Contributor

Option bits and register only get into sync after a RESET. Did you issue a reset?

B.t.w. what are FUS/RSS you talk about ? Fuses? Relative Signal strength?

@zyp
Copy link
Contributor

zyp commented Mar 11, 2019

Yes, I've tried both the traditional power on reset and the OBL_LAUNCH bit in FLASH_CR. I can change other stuff in the same register like RDP just fine and have it take effect, but FLASH_OPTR.ESE reads as 1 no matter what.

RSS and FUS are respectively Root Security Services and Firmware Update Service. Like I wrote in my first comment it's a preinstalled firmware stored at the top 40k of the flash, responsible for loading and decrypting the encrypted CPU2 firmware binaries.

@UweBonnes
Copy link
Contributor

Sorry for tedious question: Di you change FSD? And perhaps: Do you have a Stlink with original firmware? Can STM32CubeProgrammer perhaps help you?

@zyp
Copy link
Contributor

zyp commented Mar 11, 2019

I tried setting FSD but FLASH_SFR still appears to be locked.

Anyway, I'm not worried about recovering the device. I went into this fully expecting to lose the ability to load encrypted firmwares, I was just hoping to get the ability to run my own code on CPU2 in return. :)

@newbrain
Copy link
Contributor Author

So it seems that what was described as a silicon bug in ES, is still there but as some kind of protection?

I'll split the commit as suggested. What's the preference for the erase page command?
I'd put it in the clean up one, while introducing it for regular L4.

I'll check the options code.

@newbrain
Copy link
Contributor Author

I did not have much time to work on this as I am abroad (but I brought the Nucleo).
So, here is a short update:

  • I have found the cause of the option command failing: bad initialization of FPEC_BASE in attach function, so the SR is not in a clean state, and option writing will fail.
  • A workaround is issuing a load command before using option, this will leave SR in good state.
  • I hope I'll be able to update the PR tomorrow.

@newbrain
Copy link
Contributor Author

newbrain commented Mar 16, 2019

Split in 3 commits:

  • Adding the erase page command to command.c and .h.
    Note, the command is still per target (as are e.g. the mass erase ones).
  • Improve option command handling, some cleanup. Prepare data structures (changed flags).
  • Add the actual STM32WB55 support.

This has been tested only with a WB55 Nucleo:

  • The option command problem is resolved. Still it's strange that SR signals an error right after POR (same as L4, as it would seem)
  • Flashing and debugging work.
  • The erase page command works.

#include "target_internal.h"
#include "cortexm.h"

static bool stm32l4_cmd_erase_mass(target *t);
Copy link
Contributor

Choose a reason for hiding this comment

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

Unneeded shuffling around unchanged code.
\

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I had moved that without noticing and forgot to reset the position, no reason to do that.

@@ -109,19 +114,20 @@ static int stm32l4_flash_write(struct target_flash *f,
#define OR_DBANK (1 << 22)

#define DBGMCU_CR(dbgmcureg) (dbgmcureg + 0x04)
#define DBGMCU_CR_DBG_SLEEP (0x1U << 0U)
Copy link
Contributor

Choose a reason for hiding this comment

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

Whitespace changes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But...they are misaligned, why keep them that way?

Copy link
Contributor

Choose a reason for hiding this comment

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

Style versus functional change

@UweBonnes
Copy link
Contributor

Ferderico, sorry if I am picky. You still have unrelated and whitespace changes. git log --check also shows whitespace errors. Otherwise thanks for your persistance so long.

@newbrain
Copy link
Contributor Author

It's your duty and privilege to be picky, and for me an opportunity to learn (first time I contribute to a real project).
Thanks for pointing me to git log --check.
I got better in handling branches, commits, rebasing and even cherry picking but git is complex.
The whitespace errors are my fault: as said, I'm abroad and missed one editor setting.
For some of the unrelated changes, see the comments.

@UweBonnes
Copy link
Contributor

I only commented some places, so look around for more issues.

A command to erase one or more pages in flash in command.c.
To use it, include command.h and add monitor_cmd_erase_page
to the target command list.
Better error handling in option command.
Erase page cmd integrated.
Changes in the info structure for coming WB55 support.
The support is implemented completely in stm32l4.c.
Major changes are related to the different flash controller address, and
some special handling for options.
@newbrain
Copy link
Contributor Author

Style versus functional change

I see and know the difference, of course.
If a source file is not touched no reason to do that.
Given the heavy edits here, why not have a easier on the eyes (== easier to maintain) code?
One might object that the added value is low, still, I find it non-negative.
(BTW: this has always been the official policy at work, so it's a bit of an automatism for me)

Anyhow, if the policy is not to have this kind of edits, I'm reverting them.
Thanks for your patience.

@newbrain
Copy link
Contributor Author

Otherwise thanks for your persistance so long.

Hah! You won't get rid of me so easily.

@zyp
Copy link
Contributor

zyp commented Mar 17, 2019

IMO any improvements should be acceptable. The only argument against style changes is that it increases the number of edited lines that has to be reviewed. I'd say just put them in their own commit bundled in the same PR so the style and the functional changes can be reviewed separately. A style only commit is easy to review to check that it's actually only style changes.

If you use a Git GUI with line by line staging (e.g. sourcetree/gitx/gitg/etc…) it's a fairly easy process to turn a bunch of changes into a neatly grouped set of commits. At work I often create like five-six commits at a time because I've been making edits all over the place and while it made sense to work on them at the same time, they are independent enough they should be reviewed separately.

@UweBonnes
Copy link
Contributor

Newbrain, can you resolve the conflicts?

@newbrain
Copy link
Contributor Author

newbrain commented Jul 19, 2019 via email

@UweBonnes
Copy link
Contributor

Please try with #507. Please also look at the comments around the flash region command.

@akamensky
Copy link

Is this still planned to go ahead at any time? I may need to work with WB55 soon-ish. Would love to use BMP for that if possible.

@UweBonnes
Copy link
Contributor

As you hopefully know, BMP is a community effort. What about following the steps from https://tighten.co/blog/adding-commits-to-a-pull-request and cloning newbrain:STM32WBsupport and test and report back? And best rebase to master to update the PR.

@akamensky
Copy link

@UweBonnes, sure, I will see if I can repeat the same. I think I was just mostly checking with PR author here to see if he is still interested in completing his contribution here.

@zyp
Copy link
Contributor

zyp commented Mar 7, 2020

@akamensky If you're just looking for something to use, the PR as it is is working. Just check it out and build it.

@UweBonnes
Copy link
Contributor

I would appreciated a rebase PR.

@akamensky
Copy link

akamensky commented Mar 9, 2020

I am trying to rebase the PR on my fork, however I don't think I understand code well enough to resolve a conflict in src/target/stm32l4.c:stm32l4_cmd_option. If I were to provide a completely valid changeset in rebased code, I'd need to either dig deeper into the code (to understand the context of the changes) or (I think simpler here) do my best effort and provide a PR for review, which will need to be done with extra scrutiny to said function.

UPD:

There are conflicts all over in stm32l4.c in final commit. Considering I am not familiar with this code at all I would rather not submit anything that would mess up existing cpu functionalities.

Moreover I am not sure why this even had to be added in stm32l4.c as this is entirely different series probably deserving its own target perhaps?

@UweBonnes UweBonnes mentioned this pull request Mar 9, 2020
UweBonnes added a commit to UweBonnes/blackmagic that referenced this pull request Mar 9, 2020
- Stripped down from erase_page
- G4 option handling probably missing.
@UweBonnes
Copy link
Contributor

You can try
https://github.com/UweBonnes/blackmagic/tree/stm32wb55
with Newbrains effort merged into git head. Tests, feedback and additions welcome!

@newbrain
Copy link
Contributor Author

newbrain commented Mar 9, 2020

I must apologize for not taking care of this myself (after more or less promising I'd do).
What I thought was a momentary spike in my workload transformed in the daily routine, and I have much less spare time (and energy) to dedicate to this.

Thanks once again for your excellent work.

UweBonnes added a commit to UweBonnes/blackmagic that referenced this pull request Mar 10, 2020
- Stripped down from erase_page
- G4 option handling probably missing.
UweBonnes added a commit to UweBonnes/blackmagic that referenced this pull request Jun 16, 2020
- Stripped down from erase_page
- G4 option handling probably missing.
@DrZlo13 DrZlo13 mentioned this pull request Oct 28, 2021
@esden
Copy link
Member

esden commented Dec 15, 2021

What is the status on this PR? I see it is currently conflicting HEAD so it would need to be at least rebased? What needs to be still done on this?

@zyp
Copy link
Contributor

zyp commented Dec 15, 2021

As far as I can see, this was rebased and merged in #942, so both this and #457 can be closed.

@esden
Copy link
Member

esden commented Dec 15, 2021

Thank you very much @zyp! :)

@esden esden closed this Dec 15, 2021
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.

5 participants