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

Make cavstool code more shareable #78753

Merged

Conversation

jsarha
Copy link

@jsarha jsarha commented Sep 20, 2024

The cavstool.py has a lot of useful but complex code in it to access DSP memory, but it looks to be grown quite hard to maintain. This PR makes some critical changes there to make some of the functions in cavstool.py shareable from outside the module to avoid growing it even more or forking the code. Much more could be done to that direction, but since I do not understand too well what it is doing, I am proceeding in baby steps and doing only what I need.

The motivation to these changes is to allow debug_stream.py [1] from SOF side to access its debug window slot directly using cavstool.py functions.

[1] https://github.com/thesofproject/sof/blob/main/tools/debug_stream/debug_stream.py

@jsarha
Copy link
Author

jsarha commented Sep 20, 2024

The PR requiring this change is here: thesofproject/sof#9498

Copy link
Collaborator

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

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

Thanks @jsarha . Looks good for the most part, a few minor comments inline, please check.

soc/intel/intel_adsp/tools/cavstool.py Outdated Show resolved Hide resolved
soc/intel/intel_adsp/tools/cavstool.py Outdated Show resolved Hide resolved
kv2019i
kv2019i previously approved these changes Sep 24, 2024
Copy link
Collaborator

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

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

Good for me now! You do need to address the CI compliance check fails before this can be merged.

@jsarha
Copy link
Author

jsarha commented Sep 24, 2024

Fix couple of pylint issues for CI compliance checks.

Jyri Sarha added 4 commits September 25, 2024 12:40
Do not force argsparse code to all modules importing cavstool.py. The
commit moves argparse code into a separate function, and calls it from
'if __name__ == "__main__":'. Also adds the argsparse call to to
acetool.py that shares cavstool code with the argument parsing.

Signed-off-by: Jyri Sarha <jyri.sarha@linux.intel.com>
map_reg() depends on args global variable for knowing it should
load a new firmware or just stand by for logging or Zephyr
shell. The map_regs() code is the very first step to access the
DSP memory, it nees to be shareable if the code is to be accessed
from another python module.

Signed-off-by: Jyri Sarha <jyri.sarha@linux.intel.com>
…ed()

The fw_is_alive() depends on 'dsp' global variable which is assigned
from map_regs() return value. To make fw_is_alive() and
wait_fw_entered(), that calls fw_is_alive(), callable from another
module, the 'dsp' variable needs to be passed as an argument.

Signed-off-by: Jyri Sarha <jyri.sarha@linux.intel.com>
Add debug_slot_offset() function for getting a debug slot offset by
the slot number.

Signed-off-by: Jyri Sarha <jyri.sarha@linux.intel.com>
@jsarha
Copy link
Author

jsarha commented Sep 25, 2024

Fix couple of pylint issues for CI compliance checks.

And one more.

Copy link
Contributor

@andyross andyross left a comment

Choose a reason for hiding this comment

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

Seems clean enough, though I absolutely agree that this has grown beyond the original organization and maybe a rewrite might be worth thinking about. Note that MediaTek DSPs have a broadly similar MMIO interface to device SRAM, and it would be good for them to evolve in the direction of sharing the same host-side log code.

@fabiobaltieri fabiobaltieri merged commit 80e629c into zephyrproject-rtos:main Oct 3, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform: Intel ADSP Intel Audio platforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants