[sw] Introduce a DIF for rv_timer by mcy · Pull Request #1501 · lowRISC/opentitan

mcy · 2020-02-07T20:23:03Z

Includes a cleanup of rv_timer_test to actually not interrupt immediately.

alphan

Thanks for implementing this @mcy! Please see my comments below. IMHO, the way the word timer is used, or overused, in the spec is a little bit confusing. Here is my understanding:

Each hart can have a timer.
Each timer can have one or more comparators, i.e., mtimecmp.
Each timer, not comparator, can be enabled/disabled individually. (CTRL register)
Within a timer, interrupts of individual comparators, i.e., mtimecmp can be enabled/disabled.

Please let me know if I misunderstood anything.

silvestrst

@mcy looks pretty good, I only went through half of this change, and will try to do the rest at some point this week, or early next week.

silvestrst · 2020-02-19T16:21:11Z

@mcy I am not sure what exactly is happened, but I don't see
#define HART_CFG_ADDR_GAP 0x100

In the latest change that I have reviewed, is github being difficult, or you have removed it?

mcy · 2020-02-19T19:26:31Z

Regarding multiple harts: the specification of OpenTitan's implementation of the timer device is unclear on how multiple harts and multiple comparisons will be handled, especially with how they will be mapped. Right now, there's just one register offset for the sole comparison register.

Also, I think that having to provide the hart separately for each call is a bad idea and makes for an unpleasant API. I think a better approach is to either have the hart be part of the device struct, or to have an "init_hart" function that converts an overall device struct (containing just an MMIO pointer) to a hart-specific struct that most, but not all, DIFs accept.

gkelly

Can you address @alphan's comments please.

mcy · 2020-04-15T17:12:06Z

I've re-written the DIF from the ground up to address most of the comments.

sriyerg · 2020-04-25T04:10:10Z

Is this PR ready to be merged?

mcy · 2020-04-27T14:40:50Z

@sriyerg No. This PR has not been approved yet, and doesn't have unit tests.

alphan

Thanks for updating this @mcy!

lenary

Garret and I did a Design Review of RV_Timer today. There are specific comments below.

In terms of things we would like to see added:

We would like to see the disable/restore API that exists in the UART DIF for interrupts, to allow users to disable interrupts and then restore back to the original state before they disabled any.
We would like typedefs for hart_id and comp_id to typedef uint32_t to dif_rv_timer_hart_id_t and dif_rv_timer_comp_id_t. This promotes readability and understandability of the interface.

lenary · 2020-05-29T13:47:58Z

This is good. We're happy to handle parameters in this way (dynamically, rather than with a _Static_assert), though we know other DIFs are not doing this yet.

lenary · 2020-05-29T13:50:44Z

As with I2C, we're happy that this API allows software to tune the dif_rv_timer_tick_params_t as necessary if there are timing issues on a specific device, but that a programmer can use this function to get good defaults.

lenary · 2020-05-29T13:59:26Z

Given an inability to disarm a timer (beyond disabling the interrupt, which this function doesn't do), we think this should be called dif_rv_timer_set_threshold, and the comment about using UINT64_MAX should be removed, as @alphan points out it is incorrect.

mcy · 2020-06-19T19:28:56Z

(Note: I'm still in the process of going through comments, so I've marked several as resolved, but I won't upload the updated patch until I'm all the way done.)

mcy · 2020-06-29T15:25:12Z

Unit tests added.

gkelly · 2020-07-06T17:10:53Z

@gkelly @lenary, ping for review. Yes I'm pinging myself.

silvestrst · 2020-07-07T08:21:23Z

I was wondering whether it could be a good idea for @hudson-ayers to have a quick look at the .h, just to have another pair of eyes of someone that has done recent work with the timer peripheral in Tock?

hudson-ayers

I went through everything but the tests, and only have a few comments. If any of my comments are obviously ignorant of best practices/standards for DIFs, that is because I first read the "what is a DIF" section of the opentitan docs this morning, so feel free to ignore :)

From the perspective my limited experience using the mtimer in Tock, I think this interface seems very reasonable and that it should be easy to port the existing mtimer code to use this DIF.

alphan

Thanks @mcy! LGTM, mod some nits + a question about irq_reg_for_hart.

alphan · 2020-07-07T19:55:47Z

Do we really need comparators here? Looking at the layout in the tech spec, I would expect the values of RV_TIMER_INTR_{ENABLE0,STATE0,TEST0}_REG_OFFSET to include the address shift due to the number of comparators, wouldn't they?

Yes, unfortunately. The register offsets in the .h file are correct for earlgrey, but not in general: they assume one hart and one comparator per. To be fully general, we need to know the number of comparators, since the IRQ registers are placed after the comparators. I've spoken to @eunchan about switching the order (IRQs, then compartors), but I don't know if that wound up being a thing we were doing or not.

I see, but for an instance with two comparators, I would expect the registers of both of the comparators to be before the irq registers and the values of RV_TIMER_INTR_{ENABLE0,STATE0,TEST0}_REG_OFFSET to be shifted accordingly.

We talked about this offline: It looks like the best solution would be to move irq registers before the comparator registers (since their number is configurable). Without this we don't think there is a great solution:

Adding the offset for comparators here means that if this DIF library is compiled for an rv_timer with N comparators, it can be used for rv_timers with >= N comparators.

Not adding the offset for comparators here means that if this DIF library is compiled for an rv_timer with N comparators, it can be used for rv_timers with 1..N comparators.
I will create an issue to see if we can move the irq registers before the comparator registers.

@mcy, if things stay the same I think it would be a good idea to add a check in init to avoid surprises.

I realized that this can be written to be fully genericaly, so I've gone ahead and updated it as such. =)

I think @alphan has filed this as an issue - #2740.

alphan · 2020-07-07T20:05:11Z

If you agree with my above comment, I don't think you need irq_reg_for_hart. reg_for_hart(..., RV_TIMER_INTR_STATE0_REG_OFFSET) should suffice. Applies to other usages with the appropriate RV_TIMER_*_REG_OFFSET offset.

lenary

I don't think there are major hurdles here - we can address any minor comments in follow-ups, which will be much easier to review.

lenary · 2020-07-06T17:39:59Z

We know we need to revisit the "reset on init" issue, so keep this for the moment and we can decide for real later.

Yup. I think this behavior is consistent with the code we have today, and it shouldn't be hard to change if we change our minds.

Signed-off-by: Miguel Young de la Sota <mcyoung@google.com>

alphan · 2020-07-08T14:56:53Z

+static ptrdiff_t irq_reg_for_hart(uint32_t hart_id, uint32_t comparators,
+                                  ptrdiff_t reg_offset) {
+  // Note that it is completely valid for this value to be negative: if this
+  // library is built for hardware with a large number of compartors, this value
+  // is necessarially negative when used with hardware with a small number of
+  // comparators.
+  ptrdiff_t extra_comparator_offset =
+      sizeof(uint64_t) * (comparators - kComparatorsInReggenHeader);
+  return reg_for_hart(hart_id, reg_offset) + extra_comparator_offset;
+}


Yes, this should work. The comment for the config struct is clear, as long as its fields are set correctly for the rv_timer instance being used, this eliminates the need for recompilation.

mcy · 2020-07-08T15:38:52Z

See #2741 for comments deferred to followups.

lenary · 2020-07-08T15:50:52Z

Ok, I think we have enough consensus to land this. Anyone think not?

mcy requested review from gkelly, moidx and sriyerg as code owners February 7, 2020 20:23

mcy requested review from alphan, jon-flatley, lenary and silvestrst February 7, 2020 20:23

mcy force-pushed the rv-timer-dif branch 3 times, most recently from 91614b6 to 5ae1934 Compare February 11, 2020 15:32

alphan reviewed Feb 11, 2020

View reviewed changes

mcy force-pushed the rv-timer-dif branch 4 times, most recently from 0d4daf6 to 9a17829 Compare February 12, 2020 17:31

silvestrst reviewed Feb 19, 2020

View reviewed changes

mcy force-pushed the rv-timer-dif branch 2 times, most recently from c4e956f to 7d6f818 Compare February 19, 2020 19:33

gkelly suggested changes Feb 25, 2020

View reviewed changes

Comment thread sw/device/lib/dif/dif_rv_timer.c Outdated

Comment thread sw/device/lib/dif/dif_rv_timer.c Outdated

Comment thread sw/device/lib/dif/dif_rv_timer.c Outdated

mcy force-pushed the rv-timer-dif branch from 7d6f818 to 8541712 Compare April 15, 2020 17:11

mcy force-pushed the rv-timer-dif branch from 8541712 to 7902c75 Compare April 15, 2020 17:13

alphan suggested changes May 12, 2020

View reviewed changes

lenary added the Component:Software Issue related to Software label May 20, 2020

lenary suggested changes May 29, 2020

View reviewed changes

mcy force-pushed the rv-timer-dif branch from 7902c75 to 580db82 Compare June 29, 2020 15:24

mcy requested review from alphan, gkelly, lenary and silvestrst June 29, 2020 15:24

mcy force-pushed the rv-timer-dif branch from 580db82 to 0a10200 Compare July 6, 2020 15:42

hudson-ayers reviewed Jul 7, 2020

View reviewed changes

Comment thread sw/device/lib/dif/dif_rv_timer.h Outdated

Comment thread sw/device/lib/dif/dif_rv_timer.h Outdated

Comment thread sw/device/lib/dif/dif_rv_timer.h Outdated

alphan approved these changes Jul 7, 2020

View reviewed changes

lenary approved these changes Jul 8, 2020

View reviewed changes

mcy requested a review from alphan July 8, 2020 14:15

mcy force-pushed the rv-timer-dif branch 2 times, most recently from f4f9d79 to 34ed2c8 Compare July 8, 2020 14:42

[sw] Introduce a DIF for rv_timer

291e9ed

Signed-off-by: Miguel Young de la Sota <mcyoung@google.com>

mcy force-pushed the rv-timer-dif branch from 34ed2c8 to 291e9ed Compare July 8, 2020 14:47

alphan approved these changes Jul 8, 2020

View reviewed changes

alphan mentioned this pull request Jul 8, 2020

Question about rv_timer register layout #2740

Closed

gkelly approved these changes Jul 8, 2020

View reviewed changes

mcy merged commit 6146c67 into lowRISC:master Jul 8, 2020

Conversation

mcy commented Feb 7, 2020

Uh oh!

alphan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

silvestrst left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

silvestrst commented Feb 19, 2020

Uh oh!

mcy commented Feb 19, 2020

Uh oh!

gkelly left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mcy commented Apr 15, 2020

Uh oh!

sriyerg commented Apr 25, 2020

Uh oh!

mcy commented Apr 27, 2020

Uh oh!

alphan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lenary left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcy commented Jun 19, 2020

Uh oh!

mcy commented Jun 29, 2020

Uh oh!

gkelly commented Jul 6, 2020

Uh oh!

silvestrst commented Jul 7, 2020

Uh oh!

hudson-ayers left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alphan left a comment

mcy Jul 8, 2020 •

edited

Loading

alphan Jul 8, 2020 •

edited

Loading

lenary Jul 8, 2020 •

edited

Loading