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

[spi_device] Check whether clock inverter can be removed #13437

Closed
msfschaffner opened this issue Jun 28, 2022 · 5 comments
Closed

[spi_device] Check whether clock inverter can be removed #13437

msfschaffner opened this issue Jun 28, 2022 · 5 comments
Assignees
Labels
Component:RTL Earlgrey-PROD Candidate Temporary label to triage issues into Earlgrey-PROD Milestones IP:spi_device Milestone:D3 Priority:P2 Priority: medium Type:Task Tasks, to-do list.

Comments

@msfschaffner
Copy link
Contributor

msfschaffner commented Jun 28, 2022

As detailed in @tjaychen's slide deck here, inverters on the clock network can cause significant skew, which in turn can result in very long build times or even infeasible configurations. Ideally, we would want to use negedge flops instead of inverting the clock explicitly.

@eunchan can you check whether that would be feasible in case of spi_dev?

CC @tjaychen @a-will

@msfschaffner msfschaffner added this to the Project: M3 milestone Jun 28, 2022
@msfschaffner msfschaffner changed the title [spi_device] Check whether clock inverter can be reviewed [spi_device] Check whether clock inverter can be removed Jun 28, 2022
@tjaychen
Copy link

i'm not sure this is worth doing now, unless we find major issues related to this.

@a-will
Copy link
Contributor

a-will commented Jun 28, 2022

Just to clarify a bit, behavioral logic on clock signals can be a source of greater skew, uncertainty, and routing congestion in FPGAs (due to going in and out of the fabric, instead of using the dedicated resources). At these low speeds and by itself, that one inverter is probably not a major problem. If we wanted to go at full speed on the FPGA, then it might be an obstacle.

The biggest source of skew was actually the use of a global clock buffer after the inverter. This created an inherent imbalance of multiple nanoseconds between clk_spi_out and clk_spi_in, leading to hold time problems.

@tjaychen
Copy link

that sounds good. Thanks for clarifying Alex.

@weicaiyang
I would say if we do want to make a change like this, it would be nice to do it after DV is stable.
That way if we do make a change, we can be reasonably certain we did not break anything.

cindychip added a commit to cindychip/opentitan that referenced this issue Jul 11, 2022
This PR copies the context from PR lowRISC#13417. The PR lowRISC#13437 has some
merging conflict.

Signed-off-by: Cindy Chen <[email protected]>
cindychip added a commit that referenced this issue Jul 12, 2022
This PR copies the context from PR #13417. The PR #13437 has some
merging conflict.

Signed-off-by: Cindy Chen <[email protected]>
@andreaskurth
Copy link
Contributor

Triaged for spi_device. Assigning to M2.5 with Priority:P4 Priority: propose to move to backlog because I think this is worth keeping in mind when analyzing synthesis/implementation results for tapeout, but unless we see a need there, we should not fix this for M2.5. If we see there's no need, we can defer to Type:Icebox Changes deferred to future milestones / Type:FutureRelease Not relevant to currently planned releases/milestones .

@andreaskurth andreaskurth added Priority:P4 Priority: propose to move to backlog Triaged and removed Priority:P2 Priority: medium labels Feb 24, 2023
@hcallahan-lowrisc
Copy link
Contributor

estimate 8
remaining 2023-03-19 8

@msfschaffner msfschaffner added the Earlgrey-PROD Candidate Temporary label to triage issues into Earlgrey-PROD Milestones label Oct 6, 2023
@msfschaffner msfschaffner added Priority:P2 Priority: medium and removed Priority:P4 Priority: propose to move to backlog labels Dec 4, 2023
@msfschaffner msfschaffner closed this as not planned Won't fix, can't repro, duplicate, stale Jan 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component:RTL Earlgrey-PROD Candidate Temporary label to triage issues into Earlgrey-PROD Milestones IP:spi_device Milestone:D3 Priority:P2 Priority: medium Type:Task Tasks, to-do list.
Projects
None yet
Development

No branches or pull requests

7 participants