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

Fix esp RISC-V configs #1204

Open
wants to merge 1 commit into
base: riscv
Choose a base branch
from
Open

Conversation

sobuch
Copy link
Contributor

@sobuch sobuch commented Jan 13, 2025

Seems to be broken since 6a27d9f, with the following error:

tcl//target/esp_common.cfg:69: Error: Command/target: riscv Exists
Traceback (most recent call last):
  File "tcl//target/esp32c6.cfg", line 142, in script
    create_esp_target riscv
  File "tcl//target/esp_common.cfg", line 79, in create_esp_target
    create_openocd_targets
  File "tcl//target/esp_common.cfg", line 69, in create_openocd_targets
    {target create} riscv riscv -chain-position riscv.cpu -coreid 0 -rtos hwthread

@aap-sc
Copy link
Collaborator

aap-sc commented Jan 13, 2025

@sobuch are you sure that 6a27d9f is the right commit? Looking at the diff alone it does not look like this configuration is affected.

@aap-sc
Copy link
Collaborator

aap-sc commented Jan 13, 2025

Looking at the diff alone it does not look like this configuration is affected.

actually... Maybe this line is to blame:

# .../riscv/startup.tcl
proc riscv {cmd args} {
	tailcall "riscv $cmd" {*}$args
}

@en-sc , given that you are the author of this specific code - could you take a look please?

Fixes tcl configs to enable esp RISC-V boards

Signed-off-by: Samuel Obuch <[email protected]>
@sobuch sobuch force-pushed the esp-riscv-cfg-fix branch from 16f6f1e to fb16cb5 Compare January 13, 2025 16:35
Copy link
Collaborator

@MarekVCodasip MarekVCodasip left a comment

Choose a reason for hiding this comment

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

Checked visually only and looks good. (I don't have the hardware to test it.)

@TommyMurphyTM1234
Copy link
Collaborator

Looking at the diff alone it does not look like this configuration is affected.

actually... Maybe this line is to blame:

# .../riscv/startup.tcl
proc riscv {cmd args} {
	tailcall "riscv $cmd" {*}$args
}

@en-sc , given that you are the author of this specific code - could you take a look please?

@en-sc, any update on this? Why is the procedure named riscv? Doesn't seem that descriptive? What is the rationale for it?

@sobuch
Copy link
Contributor Author

sobuch commented Jan 24, 2025

@TommyMurphyTM1234 startup.tcl can be probably discussed separately from this PR (though I didnt observe any issues when I tried removing this procedure), I believe its better in any case to decouple the target type from its name in our configs. The targets were only named "riscv" here to use the riscv target instead of custom per-chip targets (that we have in our repo).

@sobuch
Copy link
Contributor Author

sobuch commented Jan 24, 2025

Hi @MarekVCodasip, if you would like some esp hardware for testing OpenOCD, it probably can be arranged

Copy link
Collaborator

@en-sc en-sc left a comment

Choose a reason for hiding this comment

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

@sobuch, sorry for taking so long to reply.

IMHO this is a valuable patch regardless of startup.tcl.

The reason is -- by convention target name includes a dot.
https://openocd.org/doc-release/html/CPU-Configuration.html#Target-Configuration

Moreover, naming the target riscv results in commands like riscv riscv ..., which seem strange.

@TommyMurphyTM1234, @aap-sc, as for the issue with startup.tcl:
proc riscv was added to support deprecation of RISC-V-specific command in TCL.
Please see https://sourceforge.net/p/openocd/tickets/434/

However, there is an issue with applying this approach to riscv ... commands: many such commands can be used like <target_name> riscv .... This use case is not covered by proc riscv.

@en-sc
Copy link
Collaborator

en-sc commented Jan 24, 2025

See also #1213

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