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

[all] interrupt consolidation #3421

Open
tjaychen opened this issue Sep 11, 2020 · 35 comments
Open

[all] interrupt consolidation #3421

tjaychen opened this issue Sep 11, 2020 · 35 comments
Labels
Component:RTL Earlgrey-PROD Triaged Temporary label to triage issues into Earlgrey-PROD Milestones Hotlist:Silicon to be resolved in Silicon Committee IP:gpio IP:i2c IP:uart IP:usbdev Priority:P2 Priority: medium Type:FutureRelease Not relevant to currently planned releases/milestones
Milestone

Comments

@tjaychen
Copy link

tjaychen commented Sep 11, 2020

This is a topic that both @eunchan and @mdhayter raised before.

Right now, all interrupts of a module are fully exposed to the top level. For blocks like UART / I2C, where there will likely be multiple instantiations per design, this greatly increases the number of overall interrupts and connections / logic to rv_plic.

I would like to propose we limit each peripheral to no more than 4 interrupts.. Just at a quick glance, these modules all have a lot

  • GPIO
  • UART
  • I2C
  • USBDEV
  • USBUART

For GPIO, @eunchan has already proposed that there might not be a need for an interrupt per GPIO.
For UART, it might make sense to group all the tx / rx interrupts.
For I2C, it might make sense to group all the tx / rx / error interrupts separately.
Similar story for USB.

Let me know what you guys think.

@tjaychen
Copy link
Author

I should acknowledge that this interrupt consolidation might cause a lot of pain for DV (especially for UART / I2C).
So we think a bit about whether this makes sense.

On bronze for example, there are 34 UART / I2Cs each, so these probably easily scale into 5060 interrupts?

@martin-lueker
Copy link
Contributor

Can we get a cost-benefit analysis on this? As with everything, I'm really concerned about the burden involved in this change. The effort spent on this change should be balanced against the effort on meeting our other M3/M4 goals. I think we have a big challenge ahead of us as it is.

If these types of details are essential design, then we can adapt, but we should retool the schedule to account for them, because I'm sure there are lots of small things like this that we could do to simplify the design. However, making cost-saving--but non-critical-- things like this a requirement without relaxing the schedule does not seem feasable to me right now.

@martin-lueker
Copy link
Contributor

Can we do this at top level? Is there some way to merge interrupts outside the IP but before the PLIC? Just throwing that out there...

@tjaychen
Copy link
Author

i would not want to do this out in the top level. If we choose to do this, it should be contained in the ip. In terms of critical, this is NOT super critical.

I think the main issue (as Eunchan had pointed out), we are running out of interrupt slots on rv_plic, and by having this many interrupts, the rv_plic (which uses a tree-like function to determine priority of interrupts) gets unnecessarily big.

I think a change like this is a really small burden on the design, most of it lands on DV. (Just based on experience going through a similar exercise before elsewhere).

Eunchan let me know if I'm wrong about what you noticed on bronze.

@tjaychen
Copy link
Author

Regarding schedule / resources from WD, sorry I don't have any visibility into that.
So feel free that to discuss that with Dom / Scott on what's realistic for WD.

From Google side, I think it's going to be a sprint for the next 6 months anyways (no it is not going to be a marathon, just long range sprinting).

@tjaychen
Copy link
Author

i've removed i2c from the initial proposed list.
Let me know what others think. It sounds like there's no appetite for such a change. That's fine with me also.
I'll close this issue by EOD if there are no other thoughts.

@martin-lueker
Copy link
Contributor

Hi @tjaychen I agree it is small. However even small things take far longer in this project than I would have expected, and I think Google management would concur.

I see the technical benefit certainly. However I'm inclined to make this a lower priority than getting all V2 related tasks for I2C, PWM, SPI HOST, and pattgen done, as those are all slated for the same DV person. Well get there eventually, it's just a matter of timing.

@martin-lueker
Copy link
Contributor

i've removed i2c from the initial proposed list.
Let me know what others think. It sounds like there's no appetite for such a change. That's fine with me also.
I'll close this issue by EOD if there are no other thoughts.

Hi Tim,
Please don't get me wrong, I think there is technical merit. We should leave I2C on the list. I just don't want to break momentum for a small change, and I want to see us finish the critical stuff before making non-critical changes.

If you think it is important and we should do this, then please keep I2C on the list. We'll get to it when we can! Though I think we do need to prioritize all changes from here on out.

I wonder if @igk8 and @tunghoang290780 weigh in with their opinions.

@eunchan
Copy link
Contributor

eunchan commented Sep 11, 2020

I expect with fully implemented designs in top_earlgrey, rv_plic could be 5 times bigger than current, which is around 50kGE ?
But personally, I think the IPs need normally one or two interrupts unless the IP is time sensitive thing (I don't see anything other than alert_handler has that kind of time critical events..). One for normal operation events, the other is for error notification (not through alert system). Then we could maintain or even reduce the rv_plic IP size.

From SW perspective, I envision that SW will have one more wrapper ISR that reads Interrupt Status register from IP and call the sub ISR.

@sriyerg
Copy link
Contributor

sriyerg commented Sep 11, 2020

This does increase DV effort. One thing that could mitigate it is if we could standardize the intermediate aggregation across ALL comportable IPs.

@martin-lueker
Copy link
Contributor

martin-lueker commented Sep 11, 2020

@eunchan I agree. When we decide this is needed, we probably could do this with just two interrupts, if we really wanted to economize. I'm trying to think if there is some clean way we could recommend for mapping all the existing interrupts to these priority categories. Some special registers for mapping out the status. If we were to do this could this be done with some "interrupt sub-status" register, that enumerates all the original interrupt status conditions that we want to advertize?

Such a generalization probably wouldn't reduce the DV burden though.

@martin-lueker
Copy link
Contributor

On a procedural note: Does this count as a port change from the point of view of the lifecycle review process? It would change the hjson. Does this mean we would want a D1/V1/D2/V2 review all the way through again? I suppose the reviews could be merged...

@tjaychen
Copy link
Author

closing the issue.
It seems in general the project needs to trend forward. So until this breaks, no point looking back i guess.

@sjgitty
Copy link
Contributor

sjgitty commented Sep 15, 2020

I want to leave this open as it will continue to grow to be a bigger issue. It can be a big change to DV but if it blows up our area we won't have much choice. @eunchan can you give a full accounting here of where all of the interrupts are coming from? Then we can poll the different designers and see if we expect more growth. Also, if we're near a tipping point, it might just be the case that we only need to slim down a few that are multiply instantiated.

As for D transitions, I think it would drop a design from D2 back to D1 with a change like this, but not farther back, and I think it would recover pretty quickly.

@sjgitty sjgitty reopened this Sep 15, 2020
@GregAC GregAC removed the Triaging:MultipleBlocks Issue is relevant for the triage of multiple HW blocks label Mar 2, 2023
@msfschaffner msfschaffner added the Earlgrey-PROD Candidate Temporary label to triage issues into Earlgrey-PROD Milestones label Oct 6, 2023
@msfschaffner msfschaffner added the Hotlist:Silicon to be resolved in Silicon Committee label Nov 6, 2023
@msfschaffner msfschaffner added this to the Earlgrey-PROD.M2 milestone Nov 6, 2023
@msfschaffner
Copy link
Contributor

I don't think this is something that is within scope for PROD due to the added churn.
Moving to backlog instead.

CC @moidx @GregAC @vogelpi

@msfschaffner msfschaffner added Earlgrey-PROD Triaged Temporary label to triage issues into Earlgrey-PROD Milestones and removed Earlgrey-PROD Candidate Temporary label to triage issues into Earlgrey-PROD Milestones labels Dec 21, 2023
This was referenced Feb 12, 2024
This was referenced Feb 23, 2024
This was referenced Feb 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component:RTL Earlgrey-PROD Triaged Temporary label to triage issues into Earlgrey-PROD Milestones Hotlist:Silicon to be resolved in Silicon Committee IP:gpio IP:i2c IP:uart IP:usbdev Priority:P2 Priority: medium Type:FutureRelease Not relevant to currently planned releases/milestones
Projects
None yet
Development

No branches or pull requests