-
-
Notifications
You must be signed in to change notification settings - Fork 834
puya: add support for PY32F002B, PY32F003, PY32F030, PY32x07x #2000
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
base: main
Are you sure you want to change the base?
Conversation
92cecd5 to
6090ecd
Compare
|
Rebased on |
dragonmux
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We love the reduction in unnamed magic numbers! Very curious that they chose to rework several registers' layouts like that too.. seems very odd. Our main review notes are on some signed-but-should-be-unsigned constants and the other is on a further reduction of magic numbers. With those taken care of we're happy to merge this.
src/target/puya.c
Outdated
| flash_size = 24 * 1024; | ||
| ram_size = 3 * 1024; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please suffix all these constants with U so they are defined unsigned and don't do a signed-unsigned conversion on assignment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed locally; will push together with the other changes once they are done.
src/target/puya.c
Outdated
| #define PUYA_DEV_ID_PY32F002A 0x000 | ||
| /* | ||
| * On PY32F002BF15P an IDCODE value of 0x20220064 was observed. Internet search shows the same value is used on | ||
| * PY32F002BW15. | ||
| */ | ||
| #define PUYA_DEV_ID_PY32F002B 0x064 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please suffix these with U so they are defined unsigned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed locally; will push together with the other changes once they are done.
src/target/puya.c
Outdated
| target_mem32_write32(flash->t, PUYA_FLASH_TS0, eppara0 & 0xffU); | ||
| target_mem32_write32(flash->t, PUYA_FLASH_TS1, (eppara0 >> 16U) & 0x1ffU); | ||
| target_mem32_write32(flash->t, PUYA_FLASH_TS3, (eppara0 >> 8U) & 0xffU); | ||
| target_mem32_write32(flash->t, PUYA_FLASH_TS2P, eppara1 & 0xffU); | ||
| target_mem32_write32(flash->t, PUYA_FLASH_TPS3, (eppara1 >> 16U) & 0x7ffU); | ||
| target_mem32_write32(flash->t, PUYA_FLASH_PERTPE, eppara2 & 0x1ffffU); | ||
| target_mem32_write32(flash->t, PUYA_FLASH_SMERTPE, eppara3 & 0x1ffffU); | ||
| target_mem32_write32(flash->t, PUYA_FLASH_PRGTPE, eppara4 & 0xffffU); | ||
| target_mem32_write32(flash->t, PUYA_FLASH_PRETPE, (eppara4 >> 16U) & 0x3fffU); | ||
| break; | ||
| case PUYA_DEV_ID_PY32F002B: | ||
| target_mem32_write32(flash->t, PUYA_FLASH_TS0, eppara0 & 0x1ffU); | ||
| target_mem32_write32(flash->t, PUYA_FLASH_TS1, (eppara0 >> 18U) & 0x3ffU); | ||
| target_mem32_write32(flash->t, PUYA_FLASH_TS3, (eppara0 >> 9U) & 0x1ffU); | ||
| target_mem32_write32(flash->t, PUYA_FLASH_TS2P, eppara1 & 0x1ffU); | ||
| target_mem32_write32(flash->t, PUYA_FLASH_TPS3, (eppara1 >> 16U) & 0xfffU); | ||
| target_mem32_write32(flash->t, PUYA_FLASH_PERTPE, eppara2 & 0x3ffffU); | ||
| target_mem32_write32(flash->t, PUYA_FLASH_SMERTPE, eppara3 & 0x3ffffU); | ||
| target_mem32_write32(flash->t, PUYA_FLASH_PRGTPE, eppara4 & 0xffffU); | ||
| target_mem32_write32(flash->t, PUYA_FLASH_PRETPE, (eppara4 >> 16U) & 0x3fffU); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the number of magic numbers involved, it might make sense to name them via #defines's.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In progress. While doing so I noticed that the existing code uses additional bits from EPPARA4 for PRETPE (datasheet says 26:16 are PRETPE so the mask should be 0x7ff but the code uses 0x3fff). @ArcaneNibble was this on purpose? If not, would you be willing to test my changes if I fix this to match the datasheet values? I don't have a PY32F002A myself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed locally to use defines named like PY32F002A_EPPARA0_TS0_SHIFT / …_MASK and tested successfully on PY32F002B. Waiting for reply from @ArcaneNibble before pushing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent, let's hope that they get back to you on this query fairly soon then 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's been a week without feedback from @ArcaneNibble so we'll probably need to continue without their assistance.
I've had a closer look at this and it's quite a mess. The PY32F002A Reference Manual says EPPARA4 26:16 (11 bit) contain the PRETPE value but that it's 12 bit wide (11:0). Also FLASH_PRETPE is 14 bit wide (13:0) and must be initialised from the value in EPPARA4. So there are 3 different bit widths documented for what should be the same value. In addition the PY32F002A flash driver (PY32F0xx_20.FLM) contained in the Keil Device Family Pack v1.2.1 (archive.org copy) provided by Puya stores the full upper 16 bits of EPPARA4 into FLASH_PRETPE. So I'm going to assume that the reserved bits are 0 and will keep the 0x3fff mask which matches the documented FLASH_PRETPE size (14 bits) and has been tested before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some hardware tests on a PY32F002A that happened to be on hand:
FLASH_PRETPEcontains 14 settable bits, with the rest being ignored
(gdb) set *(int*)0x40022120 = 0xffffffff
(gdb) x/1xw 0x40022120
0x40022120: 0x00003fff
- the calibration value at
0x1fff0f7c(akaEPPARA4for 24 MHz) contains the value0x12c05dc0. This means that 11 or 12 bits is probably not correct
In conclusion, 14 bits is likely the correct width for the PRETPE calibration value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's excellent - hopefully that gets silbe sorted out on what needs to happen here so this PR can get finished up then 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ArcaneNibble Thanks a lot for spending the time and all the information! Good to know that there are differences between the English and Chinese versions of the datasheet even regarding bit assignments in the registers. I just checked for the PY32F002B and indeed the Chinese version is more consistent. 😕
Agree that we should stick with 14 bits for the PY32F002A. Regarding the chip/driver identification string, until somebody tries this with a PY32F030 we won't know a) what its ID code is and b) whether it will actually work with the current code. So displaying "PY32F002A" would be the best match for now IMO (but I don't feel strongly about it). It also has the highest chance of getting reports from users when PY32F030 happens to work (which means we can ask them for the ID code that was matched).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI: After adding the comment regarding EPPARA4 for PY32F002A I retested mostly pro forma and ran into lots of trouble. Even the previous version which worked fine last week now shows intermittent failures. Of course, the less debug output is provided, the more likely it is to fail, with direct usage of the "serial" interface from GDB failing quite often (so I guess it's timing related in some way).
Configuring the SWD frequency to around 100kHz (actual frequency around 190kHz) did not help.
I also tried not writing the timing parameters in puya_flash_prepare() as both PY32F002Bxx_24.FLM and CMSIS/Flash/PY32F002Bxx/FlashPrg.c in the Puya Keil DFP don't do this either, but the effects are the same.
Guess I need to attach a logic analyser to find out what failed. It'll probably be a while until I get around to that so I will mark this MR as draft and push the current code in case somebody else wants to pick it up in the meantime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, thanks for the review so far!
6090ecd to
8e13ebc
Compare
e2d61a3 to
56284bd
Compare
|
Finally gotten around to working on this again. After rebasing on current master and swapping out the cables there's no more trouble with the PY32F002B chips. Because I was curious about the IDCODEs, I got a bunch more models and even have them working now:
|
The flashing procedure and preparatory steps are very similar across PY32F002A, PY32F002B, PY32F003, PY32F030 and PY32x07x. There are some differences though that need to be handled: - The base address for the Configuration Bytes (factory calibration) is different between some of the families. - On PY32x07x the Configuration Bytes are spaced at 64-bit; for all other families they are spaced at 32-bit. - PY32F002B only supports 24MHz HSI and the corresponding Configuration Bytes are located at what would be the 4MHz entries (HSI_FS=0) for the other models. - On PY32F002B some of the flash timing values are sized differently and EPPARA0 in particular has a different layout because of that. - PY32F002B and PY32x07x do not have flash and RAM size stored at address 0x1fff0ffc and I didn't find obvious candidate so their sizes are hard-coded for now. The structure and values of the DBGMCU IDCODE register are undocumented. We use the DEV_ID / REV_ID split used by the vendor SDK together with IDCODE values we collected ourselves. Most the families (PY32F002A, PY32F003, PY32F030) share the same IDCODE value of 0x60001000 but the ones that need to be handled differently do have their own values. For PY32F002A et al. the EPPARA4 PRETPE mask value was kept the same as before (14 bits). The Reference Manual has conflicting information about it: EPPARA4 26:16 (which are 11 bits) is supposed to contain PRETPE 11:0 (which are 12 bits). FLASH_PRETPE, which is documented as having to be initialised from the EPPARA4 value, has 14 bits for PRETPE. The official Keil Device Family Pack v1.2.1 from Puya uses the full upper 16 bit of EPPARA4 so presumably the reserved bits are simply 0 and using the full 14 bit mask is probably fine. In addition to adjusting the calibration addresses based on the model, we now also write HSI_TRIM (using the corresponding factory calibration value like for the flash registers). While the Reference Manuals do not explicitly whether this is required for programming the flash, it can make a huge difference to the accuracy of the HSI clock (over 13% deviation were measured on one chip) and the vendor software does the same. Tested: - using ST-Link v2 clone directly and via BMDA - sequence: load, power cycle, compare, erase, power cycle, check empty - models: - PY32F002AF15P6 - PY32F002AL15S6 - PY32F002BD15S6 - PY32F002BF15P6 - PY32F003L24D6 - PY32F030F18P6 - PY32F030F38P6 - PY32M070K1BU7-C
56284bd to
efec098
Compare
Detailed description
The PY32F002B series uses different DBGMCU IDCODE values and there does not seem to be a register containing RAM and flash size.
While the procedure for preparing flash access (copying parameters from factory-programmed EPPARAx registers to flash peripheral registers) is the same, the bit allocation inside the registers is slightly different.
The structure and values of the DBGMCU IDCODE register are undocumented but the vendor SDK splits it into DEV_ID and REV_ID fields. We use the DEV_ID fields of the IDCODE values observed on PY32F002AW15U and PY32F002BF15P6 to distinguish between the two families. An internet search shows that at least the PY32F002BW15 uses the same value as the PY32F002BF15P6. The full IDCODE values are retained as comments to make it easier to fix the code later if it turns out the DEV_ID/REV_ID split is incorrect.
Your checklist for this pull request
Closing issues