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

Add debug clock tap port to all default designs #1697

Merged
merged 10 commits into from
Jan 11, 2024
Merged

Add debug clock tap port to all default designs #1697

merged 10 commits into from
Jan 11, 2024

Conversation

jerryz123
Copy link
Contributor

@jerryz123 jerryz123 commented Dec 15, 2023

Related PRs / Issues:
Resolves #1687

Type of change:

  • Bug fix
  • New feature
  • Other enhancement

Impact:

  • RTL change
  • Software change (RISC-V software)
  • Build system change
  • Other

Contributor Checklist:

  • Did you set main as the base branch?
  • Is this PR's title suitable for inclusion in the changelog and have you added a changelog:<topic> label?
  • Did you state the type-of-change/impact?
  • Did you delete any extraneous prints/debugging code?
  • Did you mark the PR with a changelog: label?
  • (If applicable) Did you update the conda .conda-lock.yml file if you updated the conda requirements file?
  • (If applicable) Did you add documentation for the feature?
  • (If applicable) Did you add a test demonstrating the PR?
  • (If applicable) Did you mark the PR as Please Backport?

@T-K-233
Copy link
Member

T-K-233 commented Dec 18, 2023

(Also reminder to change the default ClockDivider for debug clock)

@jerryz123
Copy link
Contributor Author

(Also reminder to change the default ClockDivider for debug clock)

Sorry, I don't follow?

@T-K-233
Copy link
Member

T-K-233 commented Dec 18, 2023

(Also reminder to change the default ClockDivider for debug clock)

Sorry, I don't follow?

I haven't looked into the issue, but I think we have two type of clock dividers:

The first type will disable the clock when DIV is set to 0, and the second one will passthrough the clock with DIV=0.

By default the clock debug pad is using the first one, and should be changed to use the second one.

@jerryz123
Copy link
Contributor Author

The clock debug tap is currently set up to use a fixed divider with a configurable >1 ratio. I couldn't think of a use case for dynamically setting the division ratio for this debug tap.

@T-K-233
Copy link
Member

T-K-233 commented Dec 18, 2023

The clock debug tap is currently set up to use a fixed divider with a configurable >1 ratio. I couldn't think of a use case for dynamically setting the division ratio for this debug tap.

I would recommend to instead make the divider an MMIO device within the PRCI controller region. Most IOs only support <200MHz output speed, and having an MMIO clock divider would be handy for debugging fast clock signals like PLL, which can reach GHz level.

I believe most of the chips we have taped out are implemented this way, so it would be nice if this is the default configuration.

@jerryz123
Copy link
Contributor Author

The default config uses a static division of 16, although it can be changed at hardware elaboration time.

Is there a use case for dynamically setting the ratio of the debug clock tap at runtime? I figured making it a fixed static divisor would make it easier to interpret, reduce complexity in the system, and avoid polluting the memory map

@T-K-233
Copy link
Member

T-K-233 commented Dec 18, 2023

For example, during the initial stage of chip bringup, we want to set the divider to be pass-through, feed in 100 Hz clock, and be able to get 100 Hz clock at the debug clock pad to verify basic signs of life.
Then, when feeding clock > 200MHz to the chip, we normally set the divider to be /2 or /10 to keep the debug clock frequency within range.
When testing PLL, we then need a larger division, normally /50 or /100, to again keep the debug output frequency within range.

@T-K-233
Copy link
Member

T-K-233 commented Dec 18, 2023

I find this is a pretty good reference design. It comes very handy and straight-forward when using this feature.
(Section 7.2.15 Clock-out capability)

@jerryz123
Copy link
Contributor Author

Wouldn't it be safer to just set a fixed division ratio? So in slow bring up, you'd expect 100MHz/16, then once you switch to the fast clock, you expect ~1GHz/16.

This requires no software manipulation of the debug clock, and the clock is always slower than the max supported frequency of the IOs

@jerryz123
Copy link
Contributor Author

If 16 is not sufficient as a static division ratio, we can change it to a much higher ratio, like 64 or 128.

The reason why I'm pushing for a simpler debug clock is to avoid situations like Argo, where the debug clock is broken for whatever reason. In Argo I made tbe debug clock programmable, but it doesn't seem to work.

@T-K-233
Copy link
Member

T-K-233 commented Dec 18, 2023

We can verify the design with FPGA prototyping.

I still insist that software-configurable clock divider is a must-have feature and helps the bringup process enormously.

@jerryz123
Copy link
Contributor Author

What information could be extracted from a software programmable debug divider, that could not be extracted from a statically divided debug clock?

If there's a scenario, I'm happy to make the change. But otherwise, I'd prefer to implement to simplest system that provides all the desired debug information.

@jerryz123
Copy link
Contributor Author

Note that this only affects the division FROM the sbus clock to the debug out.You can still dynamically set a division ratio for the sbus clock from the pll clock. I'm just trying to argue against having two programmable dividers in series, that seems too complex with little inherent gain

@T-K-233
Copy link
Member

T-K-233 commented Dec 18, 2023

There are occasions where we don't want to decrease the speed the system is running, and just probe the clock with a slower output ratio. Having the divider after the mux allows us to agilely adapt to probing the internal with different frequencies, as shown in the diagram
image

Furthermore, having the division ratio be /16 is hard to work with and will bring a lot of unnecessary trouble on the bench. (Imaging looking at the scope showing a 3.1192 MHz signal when you feed the system a 50 MHz clock...)

For implementation, we already have most of the system there in the previous chips. Just a bit bug to be resolved. I am afraid that switching to the design without the divisor will be more of a change, and is not worthy for the reliableness it offers.

I am willing to help and do validation on FPGA for the clock divider design.

@jerryz123
Copy link
Contributor Author

Ok, I concede. I can change this such that the debug clock is its own independent "domain" with its own programmable divider and mux.

The tradeoff is that this may not precisely match the sbus clock. But it sounds like that isn't that useful anyways?

Is this acceptable?

@T-K-233
Copy link
Member

T-K-233 commented Dec 18, 2023

Thanks! Is there anything I can help?

@jerryz123
Copy link
Contributor Author

Once I fix this (tomorrow T the earliest), can you check that the divider works as intended in RTL simulation?

@jerryz123
Copy link
Contributor Author

@T-K-233 I've updated this PR.
Check the ChipLikeRocketConfig, which should generate the divider/mux for the clock tap.

100004: Tile allClocks_tileClockGroup_tile_0 clock gate                                                                                                                                                                                       
110000: Tile allClocks_tileClockGroup_tile_0 reset control                                                                                                                                                                                    
120000: Clock domain allClocks_uncore divider                                                                                                                                                                                                 
120004: Clock domain allClocks_tileClockGroup_tile_0 divider                                                                                                                                                                                  
120008: Clock domain allClocks_clockTapNode_clock_tap divider                                                                                                                                                                                 
130000: Clock domain allClocks_uncore clock mux                                                                                                                                                                                               
130004: Clock domain allClocks_tileClockGroup_tile_0 clock mux                                                                                                                                                                                
130008: Clock domain allClocks_clockTapNode_clock_tap clock mux 

I believe the initial state for the clock tap should be the slow clock.

Copy link
Member

@T-K-233 T-K-233 left a comment

Choose a reason for hiding this comment

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

tyty

@jerryz123 jerryz123 merged commit 8073ecb into main Jan 11, 2024
52 checks passed
@jerryz123 jerryz123 deleted the clock_tap branch January 11, 2024 19:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add debug clock probes to default design
3 participants