Improvements for mec16xx #507

purdeaandrei · 2024-01-07T15:25:56Z

Please see individual commits for details and commit messages.
Let me know if you'd want to split this up as separate PRs.
In short:

Arc is halted correctly
Fixed bug with the write subcommand failing
Added support for accessing eeprom
Added erase-flash subcommand

software/glasgow/applet/program/mec16xx/__init__.py

whitequark

Congratulations on a fantastic first-time contribution! Your PR is excellent and I look forward to working with you more in the future. All I have is some minor stylistic changes.

Would you like to become the code owner for this applet?

purdeaandrei · 2024-01-14T09:06:13Z

Ready for re-review.

Note that this patchset now includes the commit from #514 (as first in the patch set), since it's a dependency
I have squashed some changes into the previously reviewed commits.

Commits with completely new code, at the end of the patch set are:

32a9cc8
a8afa09
3bd5160
a425aa9
7e726b9
3df2adf

I think with this I consider the program-mec16xx applet complete.

purdeaandrei · 2024-01-14T09:16:11Z

Also added: e565831

This commit fixes two issues: 1) Fixes the error "TypeError: __init__() got an unexpected keyword argument 'halted'" When this code was written, the previous version of bitstruct/Bitfield ignored incorrect fields, and as such this resulted in writing zero into that register. At some point in time bitstruct/Bitfield was improved, and that exposed this bug. 2) While the original intent of the code, to set STATUS32.H may or may not work to some degree, it is not the correct way to force the ARC to be halted, instead the DEBUG register should be used. The openocd code in this very repository also does the same.

…ew operation This fixes the write command failing on MEC1633. The write command used to first successfully execute an erase, but after that the actual writing of the data failed. It seems that at least the MEC1633 requires the embedded flash controller to go back to manually the STANDBY state before executing the next command. This is also explicitly spelled out in the MEC1632 and MEC1618i datasheets, that the controller must go back to standby when switching to a different mode. This is also especially useful in case of calling these in repl mode

…sh_command To be used later. This commit contains no functional changes.

…tandby mode.

…tandby mode after every operation. Since now we go into standby mode at the beginning of every operation, this just adds extra paranoia. Also on program, wait for not busy before going back to standby mode. The datasheets say that in standby mode the fifos are flushed. It's not clear but that might mean that data that's still in there is thrown away, so this is just some paranoia code to make sure that doesn't happen.

Emergency erase requires a power cycle after it has been executed, at least on some chips. This is explicitly called out in the MEC1632 datasheet for example. The warning might help a user to avoid thinking that something is broken, if they try a read/write command after emergency-erase.

…elp strings Also got rid of EC firmware mentions, cause flash might not necessarily contain only EC firmware, so we should not make any assumptions about what it contains in our documentation.

purdeaandrei · 2024-01-14T10:28:34Z

Rebased, typos fixed

Both the read-flash and write-flash commands don't hardcode flash size anymore. write-flash uses the size of the file specified, while read-flash forces the user to specify a flash size using the -s argument. The help text has also been extended to describe typical flash sizes observed on mec16xx devices.

The main issue with it was that the polarity of VTR_POR and VCC_POR was wrong, due to the datasheet contradicting itself. This change also makes sure that the erase completes before the coroutine ends, and that reasonable state is restored after. Also rename the internal function name, since it doesn't only erase flash anymore, since on some MECs it also erases eeprom.

…complete operations with --force

This makes burst write of the flash more correct, by checking the Data_Full flag, like the datasheet suggests. This has not been observed to be necessary at the moment, but may become and issue in the future if we get jtag access to be faster. This will make writes slower, but will ensure that no data is lost.

Burst mode is now off by default, because commends from the previous flash read code indicate that it didn't work right, at least for some MEC16xx variants. Lenovo's ThinkShiled branded MEC1633s appear to work correctly with burst mode

…have been tested

whitequark

Thank you for this excellent contribution!

purdeaandrei requested a review from whitequark as a code owner January 7, 2024 15:25

purdeaandrei changed the title ~~F improvements for mec16xx~~ Improvements for mec16xx Jan 7, 2024