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

[topgen] Fix broken pinmux table link #21696

Merged
merged 1 commit into from
Feb 27, 2024

Conversation

msfschaffner
Copy link
Contributor

@msfschaffner msfschaffner commented Feb 26, 2024

This is a short term fix and addresses #18585 and #19425.
Longer term, we should align how top-level documentation is generated, see #12848.

@msfschaffner msfschaffner self-assigned this Feb 26, 2024
@msfschaffner msfschaffner changed the title [top/pads] Switch CC pins to 5V tolerant pad cells [topgen] Fix broken pinmux table link Feb 26, 2024
@msfschaffner msfschaffner marked this pull request as ready for review February 26, 2024 23:17
This was referenced Feb 26, 2024
a-will
a-will previously approved these changes Feb 26, 2024
| CW340 | 4 | 47 | 14 | 13 | 74 | [Pinout Table](./pinout_cw340.md) |
| Target Name | #IO Banks | #Muxed Pads | #Direct Pads | #Manual Pads | #Total Pads | Pinout / Pinmux Tables |
|:-------------:|:-----------:|:-------------:|:--------------:|:--------------:|:-------------:|:---------------------------------------------------------------------------:|
| ASIC | 4 | 47 | 14 | 10 | 71 | [Pinout Table](../../../top_earlgrey/ip/pinmux/doc/autogen/pinout_asic.md) |
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm... actually, is this the right number of directory changes? It looks to me like this new link wouldn't resolve.

Also, did you find a broken link on the website? It was working for me.

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like this would break the top-level documentation here: https://opentitan.org/book/hw/top_earlgrey/ip/pinmux/doc/autogen/targets.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are two scripts, I believe. One that generates top-level docs, and one for the IP itself. The latter is broken:
https://opentitan.org/book/hw/ip/pinmux/doc/programmers_guide.html#top_earlgrey-pinmux-targets

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I see now that this table is reused here:
https://opentitan.org/book/hw/top_earlgrey/ip/pinmux/doc/autogen/targets.html

That was not how this was initially intended.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm feeling like the top-level documentation is already fine, but it's the IP-level that is broken (which is referenced in the issue you linked). Should we consider eliminating the top-level-specific pin-out table from the IP docs instead?

## Pinout and Pinmux Mapping
The tables below summarize the pinout and pinmux connectivity for certain top-level designs.
### Top Earlgrey
{{#include ../../../top_earlgrey/ip/pinmux/doc/autogen/targets.md}}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that may be the easier fix. Let me do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated - PTAL

@a-will a-will self-requested a review February 26, 2024 23:33
@a-will a-will dismissed their stale review February 26, 2024 23:40

Prematurely clicked approve. Some outstanding discussion items. :)

Removes the top-dependent tables from the IP documentation and refers to
the top-level specific docs.

Signed-off-by: Michael Schaffner <[email protected]>
@msfschaffner msfschaffner merged commit cb32cee into lowRISC:master Feb 27, 2024
19 of 25 checks passed
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.

2 participants