Last fixes for v1 #53
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
binutils-esp32ulp allows peripheral registers to be specified either as a full address (e.g. 0x3ff48000) or as a direct ULP address (e.g. 0x120), i.e. the address as seen from the ULP. "direct" addresses are anything from 0x0 to 0x3ff (inclusive). This change ensures that 0x3ff is included in what is treated as a direct ULP address. (See https://github.com/espressif/binutils-esp32ulp/blob/249ec34/gas/config/tc-esp32ulp_esp32.c#L145 for reference to how binutils treats anything larger than DR_REG_MAX_DIRECT (0x3ff) as a full address, so everything less *AND* equal to DR_REG_MAX_DIRECT is therefore a direct ULP address.) This commit contributes to being able to eventually assemble the esp32ulp_ranges.s test from binutils-esp32ulp. It addresses this line: https://github.com/espressif/binutils-esp32ulp/blob/249ec34/gas/testsuite/gas/esp32ulp/esp32/esp32ulp_ranges.s#L136
When register addresses less than or equal to 0x3ff are given to the reg_rd and reg_wr opcodes, these addresses should be used unmodified as the address in the machine instruction. In our implementation we split the 10 bit address field of the machine instruction into two fields, namely "addr" for the lower 8 bits and "periph_sel" for the upper 2 bits. We already had a mechanism for determining the periph_sel part for "full" addresses (e.g. 0x3ff48000), but for direct (ULP) addresses, we always set periph_sel to 0 instead of using the upper 2 bits from the given address. This commit fixes that. Note 1: In binutils-esp32ulp, they don't split the address into these 2 fields but simply put the direct ULP address into a single combined field of 10 bits, which has the same effect. See: https://github.com/espressif/binutils-esp32ulp/blob/249ec34/gas/config/tc-esp32ulp_esp32.c#L145 Note 2: In the "macro approach" in esp-idf for creating ULP code, they also use the split field approach (I assume our implementation is modelled after that) and they also don't handle direct (ULP) addresses correctly (or seemingly at all). See: https://github.com/espressif/esp-idf/blob/9d34a1c/components/ulp/include/esp32/ulp.h#L349 This commit contributes to being able to eventually assemble the esp32ulp_ranges.s test from binutils-esp32ulp. It addresses this line: https://github.com/espressif/binutils-esp32ulp/blob/249ec34/gas/testsuite/gas/esp32ulp/esp32/esp32ulp_ranges.s#L136
There is no need to duplicate the "<= DR_REG_MAG_DIRECT" check in two functions. Now the _soc_reg_to_ulp_periph_sel() function is used only for "full addresses" (as it was some commits ago) and will return a ValueError if used with direct ULP addresses. This commit also masks values correctly when setting the addr and periph_sel fields for "direct ULP addresses", so only the bits we're interested in actually get used (rather than being implicitly trimmed because there aren't more bits available in a field).
Everything necessary to pass previously skipped binutils-esp32ulp tests is now fixed. So we no longer need to skip them. (Tests using unsupported features, such as assembler macros, are still skipped.) Note 1: There is one test, esp32ulp_ranges.s, which requires symbols defined in esp32ulp_globals.s. binutils-esp32ulp joins these during the linking stage in its test scripts. Since we don't separate stages, we simply concatenate the two files before assembly. Note 2: binutils-esp32ulp has a bug related to how absolute symbols defined with .set are interpreted by the JUMP instruction. If a symbol is marked global, the value is taken as-is, but if a symbol is not global, it's value is divided by 4. Since py-esp32-ulp treats the symbol value the same, whether global or not (the believed correct behaviour), we work around the bug in our test script and patch the input files to make the relevant symbols global.
ThomasWaldmann approved these changes Oct 7, 2021
Collaborator
ThomasWaldmann left a comment
ThomasWaldmann left a comment There was a problem hiding this comment.
LGTM, thanks for fixing!
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit. This suggestion is invalid because no changes were made to the code. Suggestions cannot be applied while the pull request is closed. Suggestions cannot be applied while viewing a subset of changes. Only one suggestion per line can be applied in a batch. Add this suggestion to a batch that can be applied as a single commit. Applying suggestions on deleted lines is not supported. You must change the existing code in this line in order to create a valid suggestion. Outdated suggestions cannot be applied. This suggestion has been applied or marked resolved. Suggestions cannot be applied from pending reviews. Suggestions cannot be applied on multi-line comments. Suggestions cannot be applied while the pull request is queued to merge. Suggestion cannot be applied right now. Please check back later.
I have now fixed the last issues for passing binutils-esp32ulp tests.
Those fixes relate to handling peripheral register addresses correctly with the
reg_rdandreg_wropcodes. While the machine instruction needs the "direct ULP address", the assembly opcode can take either direct ULP addresses or full DPORT bus addresses. binutils-esp32ulp internally converts full addresses to direct addresses and thus supports both. We attempted to support both, but had a few bugs related direct ULP addresses. These are now fixed. (More details in the commits)You will notice I added a crude "patching mechanism" in the
02_compat_rtc_tests.shbecause I needed to work around the bug related to global/not-global absolute symbols (as discussed in PR #52). The workaround was to make all symbols used in problem cases global, because that case binutils-esp32ulp handles correctly.When this is merged, I guess we can consider the v1 milestone (#49) reached.