updating encoding.h and support for deprecated features

[sobuch]

Hi all,
I am looking to reduce custom changes we have in https://github.com/espressif/openocd-esp32, so I would like to open a discussion here about the encoding.h header, generated from https://github.com/riscv/riscv-opcodes

Since the riscv-opcodes repo follows the latest spec versions, I think there is a little disconnect from riscv-openocd. I believe here it may be desirable to keep supporting some deprecated features, since the hardware targets already exist. For example, v0.11 of debug spec is still supported here, but some defines used in the riscv-011 target were removed in riscv-opcodes, so the build breaks if we try updating the encoding header.

Another instance is deprecated N extension (interrupts in user mode), which is implemented for example in esp32c6. Here, riscv-openocd is inconsistent, as CSR registers from this extension were removed, however the extension is still considered elsewhere (see https://github.com/riscv-collab/riscv-openocd/blob/riscv/src/target/riscv/riscv_reg.c#L435)

Any thoughts on this topic? I was thinking maybe a wrapper around the generated encoding header could be added, please see riscv...sobuch:riscv-openocd:custom_encoding_header

[JanMatCodasip]

Hi @sobuch,

your proposed solution sounds OK to me -- that is, having an additional, manually maintained header file with the constants that are no longer part of the generated encoding.h.

The main header file can perhaps be called riscv_encoding.h instead of openocd_encoding.h for clarity.

The main header file could then include two other header files -- generated encoding.h and manually-written encoding_extras.h (or a similar name).

If any of the constants is relevant just for the legacy 0.11 version of RISC-V , I'd move it directly to the 0.11 code and not pollute the global encodings.

Please, include also sufficient comments to make it very clear to code readers

• why the headers encodings are arranged the way they are,
• what is auto-generated code and what is manually written.

Thanks.

[sobuch]

@JanMatCodasip thanks for looking into this and for the suggestions.

I see the header as I implemented it is very unclear, so let me first submit a simple version, only adding the user CSRs - #1220

What made the header messy, is redefining CLIC CSR numbers, but I will try again to get that fix into riscv-opcodes instead.
The encoding.h header can be regenerated and updated after that (including adding those riscv 0.11 defines to the 0.11 code).

[en-sc]

As I've mentioned in #1220, IMHO it's better to use TCL to support deprecated features. I would suggest using riscv expose_csrs command to add the missing CSRs instead. Extra parameters can be added to the command to avoid adding csr_ prefix to the register name.

[JanMatCodasip]

I concur - if all that is needed is to expose additional CSRs, then riscv expose_csrs looks like the right tool for the job.

[sobuch]

@en-sc good point, without the CLIC CSR number changes, which cannot be done with expose_csrs, this seems more reasonable way.

For me, having the defines is useful for future patches, and ustatus should also be considered for example in case something gets implemented to allow caching mstatus (#1219). But I think I will first check whether there are any issues adding the option for expose_csrs not to use the prefix, as that prefix would be my main objection against using expose_csrs for this.

BTW expose_csrs doesnt work well for ustatus (it stays named csr0), seems that the call to init_custom_csr_names should be moved here https://github.com/riscv-collab/riscv-openocd/blob/riscv/src/target/riscv/riscv_reg.c#L184

[aap-sc]

BTW expose_csrs doesnt work well for ustatus (it stays named csr0),

I feel like the fact that we have a register called csr0 is some kind of a mistake/generation artifact.

[aap-sc]

@sobuch the reason that csr0 stays the same is a bug in expose_csr functionality. Take a look here: #1221 . Once this change is applied you should be able to expose USTATUS.

[sobuch]

Thanks for the input guys. I think I was stuck at the idea trying not to add more differences in our fork, but in this case seems better to fix on our side.