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

[otp_ctrl,doc] Fix some links to otp_ctrl documentation #26070

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

Conversation

rswarbrick
Copy link
Contributor

These were broken when the block became templated. There are still a couple of hits for "ip/otp_ctrl" as an absolute path in the repository:

  • hw/syn/tools/dc/testsynth.tcl

    (I don't have a DC licence and I'm not sure how this gets run, so can't really fix it)

  • util/design/data/otp_ctrl_img.c.tpl

    This doesn't look quite right, but the generation is tied in with the Bazel SW build. I'm not entirely sure how the link should work.

@matutem: Would you mind glancing through this and checking that the renames (and explanatory comments) match how you think it should work?

@engdoreis: As someone with much more knowledge of the SW world, do you know how the otp_ctrl_img.c.tpl file gets used? Does it make more sense for the comment to point at the otp_ctrl template, or should this be some relative path?

@matutem
Copy link
Contributor

matutem commented Jan 31, 2025

I suggest you make this change in otp_ctrl_img-c-tpl:
// AUTOGENERATED. Do not edit this file by hand.
-// See the util/design/data hw/ip/otp_ctrl/data README for details.
+// See util/design/README.md section "OTP Preload Image Generator" for details.

As for the synthesis tcl script, it may only support earlgrey? If so, change the path from
../../../ip/otp_ctrl
to
../../../top_earlgrey/ip_autogen/otp_ctrl

If the script knew the topname it may be able to generate the right path.

Copy link
Contributor

@matutem matutem left a comment

Choose a reason for hiding this comment

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

Thanks Rupert. These look good. I have a couple suggestions about the missing paths you mention.
Also, I think it may be best to be able to refer to the html generated from the markup since it provides more anchors. This may be possible when the docs contain all tops (or do they already?). Perhaps we should add an issue to replace reference to md files to references to the generated html if and when docs are multi-top?

Please at least make the change in otp_ctrl_img.c.tpl, otherwise LGTM

These were broken when the block became templated.

Signed-off-by: Rupert Swarbrick <[email protected]>
@rswarbrick
Copy link
Contributor Author

CHANGE AUTHORIZED: hw/ip/sram_ctrl/rtl/sram_ctrl.sv
CHANGE AUTHORIZED: hw/top_earlgrey/ip_autogen/otp_ctrl/rtl/otp_ctrl.sv
CHANGE AUTHORIZED: hw/top_earlgrey/ip_autogen/otp_ctrl/rtl/otp_ctrl_scrmbl.sv
CHANGE AUTHORIZED: hw/top_earlgrey/ip_autogen/otp_ctrl/rtl/otp_ctrl_token_const.sv

The changes to these files lie in comments in each case, so cause no risk to the design.

@rswarbrick
Copy link
Contributor Author

@matutem: I think the docs already contain all tops. I'm not quite sure where this replacement would be relevant though. Can you point to an example?

@matutem
Copy link
Contributor

matutem commented Jan 31, 2025

@rswarbrick my relevant comment is #26070 (comment)
I see you made the otp_ctrl_img.c.tpl change, thanks.

Regarding the synthesis tcl file, we can make it either top_earlgrey-specific, or consult if the flow can take a topname parameter in case it actually makes sense. Another option is to move that script to hw/top_earlgrey/syn. This probably falls outside the scope of this PR, so could you file an issue for it?

In any case, I insist this LGTM :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component:Doc Documentation issue IP:otp_ctrl
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants