Nordic RTC base driver

[nashif]

Reported by Carles Cufi:

Currently the Nordic RTC hardware peripheral is used directly from 2 different subsystems:

• drivers/timer/nrf_rtc_timer (System Timer)
• subsys/bluetooth/controller/hal/nrf5/cntr.c

Instead we should have a single base drivers/rtc/nrf_rtc.c that is the reused by the 2 subsystems above.

Things to take into account:

• Should we expose a full RTC peripheral, or a single Capture-Compare register per driver instance?
• Is the rtc.h API enough or does it need to be extended?
• Should we instead target the counter.h API, which seems more adequate for a Real Time Counter?

Assumptions

• The RTC functions can be called from any IRQ priority except potential zero-latency IRQ priorities if CONFIG_ZERO_LATENCY_IRQS is configured.
• The RTC interrupt handler itself can be configured to run at any non-zero-latency IRQ priority.
• set_alarm() function first reads current counter value. If (alarm_value >= now) alarm will be triggered in current 32 counter cycle, otherwise it will be set in far future. It means that setting very short alarm needs precautions or even irq_locking() for avoiding preemption. These precautions are not implement in the driver, they should be taken in code layer that uses the driver.

(Imported from Jira ZEP-1808)

[nashif]

by Michał Kruszewski:

Vinayak Kariappa Chettimada I think we should expose full RTC peripheral so that single driver encapsulates all the hardware registers associated with one specific RTCn instance. You can always write some higher layer code that would use RTC driver and split it into single Capture-Compare registers.

[nashif]

by Carles Cufi:

Øyvind Hovdsveen might have a say there too

[nashif]

by Vinayak Kariappa Chettimada:

I agree with Michał Kruszewski , driver should expose full RTC h/w. I am thinking more in terms of instances of RTC and instances of Captures and instances of Compares. As there are many instances of RTC in nRF series and each have different numbers of Capture/Compares, coming up with a model would be nice. Yes, system timer and controller can acquire an RTC (even share) and acquire one or many Capture/Compares from the RTC driver.

[nashif]

by Øyvind Hovdsveen:

Agree, this should be encapsulated so that multiple modules can safely use RTCs, and as a possible further improvement even share an RTC if their usage is compatible (prescaler, shorts, no start/stop/resetting/IRQ priority, possibly other things)

[nashif]

by Carles Cufi:

Vinayak Kariappa Chettimada and Øyvind Hovdsveen from what you are saying the current rtc.h simply doesn't cut it then. We will need to define a specific header for our RTC.

[nashif]

by Michał Kruszewski:

I think handling multiple instances of RTC is quite easy, take a look at my draft rtc_nrf5.c and Kconfig files in attachments. The problem is when it comes to CC registers, TICK event and OVRFLW event. I have some idea but I need your opinions. 1. Handling TICK and OVERFLW can be done with adding callback to current config structure using #ifdef SOC_FAMILY_NRF5 inside it. 2. Handling multiple CC can be done in several ways: a) rtc_api_set_alarm can return alarm descriptor, then in alarm callback it is possible to check which alarm descriptor has triggered an alarm. If there are currently no free CC set alarm can return an error code. b) In addition to alarm_value, set_alarm can also take pointer to callback function, and that function would be called on alarm. c) Adding function for allocating single CC channel. All of the above need changes in include/rtc.h api but I think this is not a big deal cause rtc.h needs to be changed anyway. For example, now there is no way to change prescaler value during runtime. [^rtc_nrf5.c] [^Kconfig]

[nashif]

by Vinayak Kariappa Chettimada:

yes, RTC instances are easy as you have in your draft.

For CC registers, why not have child config structs with interface to CC registers (get, set and ...) and run-time callback function pointer.

rtc_xxx0_config->rtc_xxx0_cc0_config->rtc_xxx0_cc1_config->...->rtc_xxx0_ccN_config->NULL. rtc_xxx1_config->rtc_xxx1_cc0_config->rtc_xxx1_cc1_config->...->rtc_xxx1_ccN_config->NULL. ... rtc_xxxM_config->rtc_xxxM_cc0_config->rtc_xxxM_cc1_config->...->rtc_xxxM_ccN_config->NULL.

Tick and Overflow, I would not worry now (when we need to use it, we define it!). For now, I cant imagine for what I would use. So, you are free to define, have samples/tests applications, let your imagination go wild.

[nashif]

by Michał Kruszewski:

Tick? As product specification says: for tick-less RTOS.

[nashif]

by Øyvind Hovdsveen:

I believe Vinayak is referring to how to forward the TICK and OVRFLW events, see http://infocenter.nordicsemi.com/topic/com.nordic.infocenter.nrf52832.ps.v1.1/rtc.html?cp=2_2_0_24_9#topic

[nashif]

by Vinayak Kariappa Chettimada:

Michał Kruszewski We are not using TICK in Zephyr, we are using capture/compare instead for system timer.

[nashif]

by Michał Kruszewski:

But we could use TICK, wouldn't it consume less power? For sure one more CC register would be available for the application.

[nashif]

by Øyvind Hovdsveen:

You would still have to use CC registers to wake up after tickless sleep, I think, so we would still need one CC register (at least if tickless sleep was configured). And the prescaler would have to be set so the ticks were slow enough, making the RTC have very poor resolution (resolution would be sys tick frequency). But yes, I think it's possible, the question is if it's desirable.

[nashif]

by Michał Kruszewski:

I attach files with my 'almost' finished work. It requires some minor rtc api change. I would like to ask you for your opinions. Carles Cufi Øyvind Hovdsveen Vinayak Kariappa Chettimada Wojciech Bober [^rtc.h] [^Kconfig] [^Kconfig.nrf5] [^rtc_nrf5.c]

[nashif]

by Øyvind Hovdsveen:

Looks like a good start to me.

I haven't looked much at the API stuff, but here's a few comments from the implementation:

• Why not allow any prescaler? (now only dividing by powers of 2 are allowed)
• There is a race condition in rtc_nrf5_read(), if the counter wraps after reading the data->counter_32 or if the can be ISR is blocked while this function is called.
• Wraparound of the 32-bit counter value is not handled in cc_event_handler()
• If the alarm is set at more than RTC_MASK/2 from now but less than RTC_MASK (which is allowed), the alarm will trigger immediately, as the interrupt will be pended immediately.
• If the user sets an alarm too close or in the "near" past, it will immediately trigger the callback. Is that expected/accepted?

Also, could it be better to not trigger the COMPARE event when we are not in the correct "upper 8 bits" of the 32-bit counter value? We could for instance iterate all alarms on the overflow event and set the CC registers when closer. Functionality or power wise I don't think it matters much, but there could be a wish to implement this in a way so that the HW event also can be used and trusted (e.g. for PPI use in some HW near real-time modules).

[nashif]

by Michał Kruszewski:

Øyvind Hovdsveen Vinayak Kariappa Chettimada I am fixing rtc driver and there is one issue I am not sure how I should resolve. What should I do if value that needs to be set to CC register is less than the min delta that guarantees generating CC event? Should I set the interrupt pending or set CC value for the closest value that will trigger compare event?

[nashif]

by Øyvind Hovdsveen:

As briefly discussed there are several options (in no particular order, also including your mentioned options):

• Do nothing: The CC value is "after wraparound".
• Set the event and interrupt pending immediately and accept that the callback may be given too early.
• Set the interrupt pending immediately but check in the handler whether the CC time has passed. If not, loop until it has passed before giving the callback.
• Set the CC value to the closest value that will trigger the event (guarantee triggering but perhaps a little late).
• Return an error code if the CC cannot be set (the time has passed or is too close in the future) and let the caller deal with the corner cases.
• Set the interrupt pending immediately and also pass information that the callback may be too early.

I don't know the requirement for the RTC module in the Zephyr project. My personal preference (which may not be aligned with Zephyr's philosophy) is to guarantee the triggering of the alarm either on-time or not at all, and also with a possible future improvement be able to use the HW events (with the accuracy expected). Hence I would prefer neither setting the interrupt pending or setting the CC to a later time. I would return an error code if the timeout could not be guaranteed, and in that case also guarantee that the timeout does NOT come.

[nashif]

by Vinayak Kariappa Chettimada:

I set to next CC value that would guarantee. See https://github.com/zephyrproject-rtos/zephyr/blob/master/subsys/bluetooth/controller/ticker/ticker.c#L1041

But again this is for soft-real time requirement.

[nashif]

by Øyvind Hovdsveen:

Triggering too late is OK I think, as long as the user of the alarm later can check if it was too late, if it cares about that.

[nashif]

by Michał Kruszewski:

Øyvind Hovdsveen If you really care if it is on time, you can always implement the following in callback: key = irq_lock(); now = rtc_read(dev); if (now >= set_value + optional_acceptable_difference) { my_alarm_is_late(); } irq_unlock(key); Reading counter value is one of the mechanisms that can be used to check if alarm is late and it is already in api. Situation, where alarm is generated 2 ticks too late does not differ from situation where alarm is generated on time but your callback gets preempted for 2 ticks. And in either case you can get to know that alarm is late by reading counter value.

I opt for setting CC register value to the closest value that can guarantee event triggering.

[nashif]

by Øyvind Hovdsveen:

OK, agree.

Situation, where alarm is generated 2 ticks too late does not differ from situation where alarm is generated on time but your callback gets preempted for 2 ticks.

That is true only if we don't use the RTC HW event for anything. But that we don't do now, so that we can fix in the future if it should become necessary. So I'm OK with setting the CC register value to the closest value that can guarantee event triggering.

I think it would be wise to document that the event may be delayed if set too close. Two RTC ticks is at least 61us, and can be tremendously long if the prescaler is used.

A side question, how is it handled when the alarm is set after the timeout, which can happen for instance if the alarm_set call is being called on time but is preempted?

[nashif]

by Michał Kruszewski:

bq. That is true only if we don't use the RTC HW event for anything. But that we don't do now, so that we can fix in the future if it should become necessary. So I'm OK with setting the CC register value to the closest value that can guarantee event triggering.

Do you mean PPI? If yes, then I would like us to think about it now because PPI is the next peripheral I would like to take care of.

bq. I think it would be wise to document that the event may be delayed if set too close. Two RTC ticks is at least 61us, and can be tremendously long if the prescaler is used.

Sure

bq. A side question, how is it handled when the alarm is set after the timeout, which can happen for instance if the alarm_set call is being called on time but is preempted?

Same story here, set it for closest possible?

[nashif]

by Øyvind Hovdsveen:

Do you mean PPI? If yes, then I would like us to think about it now because PPI is the next peripheral I would like to take care of. Yes, I meant via PPI.

A side question, how is it handled when the alarm is set after the timeout, which can happen for instance if the alarm_set call is being called on time but is preempted?

Same story here, set it for closest possible?

If we set it to the closest value in the future when the requested value is in the past, should we then do that for "all" values in the past, not only when we are very close? E.g. if "now" is 1000, and someone tries to set an alarm for 500, how do we deal with that? Same as if someone tried to set it for 999 or 1001?

If not, how do we deal with values obviously in the past? Where do we draw the line?

I think this is difficult to distinguish, and is the reason for one of the previous suggestions (Return an error code if the CC cannot be set, same if we are too close or if the requested value is in the past)

[nashif]

by Michał Kruszewski:

I thought in the following way: set_alarm_fun(alarm_val){ now = rtc_read(); if (alarm_val >= now) { alarm will be triggered in this cycle of counter; } else { alarm will be triggered in next cycle of counter; }

[nashif]

by Michał Kruszewski:

Øyvind Hovdsveen Vinayak Kariappa Chettimada I attach pdf file so that you can check how I see it. [^rtc_alarm.pdf]

[nashif]

by Øyvind Hovdsveen:

I am a bit confused with your pseudocode. I assume "now" is also the time returned by rtc_read()? and that "read+delta" is really "read_delta"? And that the else block is to handle wraparounds?

Also I think the case where alarm < now is missing for the non-wrap case.

I would suggest abstracting away the wraparound calculations using subtraction between the (32-bit) counter values, the result cast to int32 and checked for difference to 0. Then it is only a matter of checking if "now" is past the deadline of setting the alarm or not.

Another question: Can the alarm be set from any IRQ priority? Or can it be called only from a subset of IRQ priorities or only from thread context? Which priority is the callback given in? Depending on this I think we can also get rid of the irq_lock().

[nashif]

by Michał Kruszewski:

I will implement my idea and then we will make changes. It will be better way to exchange our thoughts.

[nashif]

by Michał Kruszewski:

Øyvind Hovdsveen I have updated header and source files. [^rtc_nrf5.c] [^rtc.h]

[nashif]

by Øyvind Hovdsveen:

I have looked at the files (would it be possible to leave the original files next time, or perhaps just upload a patch? It is useful to see the changes done, not just the end result), and I think most of my issues stated above is solved :)

If the call to the alarm_set() is delayed past the alarm time (for instance by IRQ), the alarm will be set in the far future and must be checked by the caller.

Personally I'm not a fan of disabling interrupts as this gives no guarantee for timing, since if CONFIG_ZERO_LATENCY_IRQS is configured, the code inside irq_lock() can still be interrupted.

When doing {{rtc->CC[i] = alarm_val & RTC_MASK;}} in {{set_alarm()}}, there is no check following it to see if the CC was set in time. It think it would be good to guarantee it in case the rtc_read() was done late enough that the value returned is OK when the the read was done, but not anymore after the if checks. This is especially the case when CONFIG_ZERO_LATENCY_IRQS is configured. For example something like this:

rtc->CC[i] = alarm_val & RTC_MASK;
now = rtc_read(dev)
while ((int32_t)(alarm_val - now) < RTC_MIN_DELTA
    && !RTC_CC_EVENT(i))
{
    alarm_val = (now + RTC_MIN_DELTA) & RTC_MASK;
    rtc->CC[i] = alarm_val & RTC_MASK;
    now = rtc_read(dev);
}
//It the event happened, set the CC to the past and handle it in the interrupt hander
if(RTC_CC_EVENT(i))
{
    rtc->CC[i] = now;
}
nrf_rtc_int_enable(rtc, int_mask);
irq_unlock(key);

(This code should also be able to replace all the code after key = irq_lock() depending on the requirements. If you do that, all late set alarms will trigger at first opportunity. If that is not desirable and the alarm should be set in the far future instead, the code must be modified)

[nashif]

by Michał Kruszewski:

We still miss the functionality for handling situations where alarm was set too late due to interruption. In such case user may want to cancel the alarm and set it for the new value. It will require to add rtc_cancel_alarm() function to RTC api and return some kind of alarm descriptor from rtc_set_alarm() so that alarms can be distinguished. What is more complicated is how to inform the user that his alarm was set for the past value and will be triggered in next RTC cycle or that his alarm will be delayed due to RTC_MIN_DELTA. One of the solutions is to return additional information from rtc_set_alarm() function, via pointer, that alarm was set in the past value or that it will be delayed.

[nashif]

by Øyvind Hovdsveen:

Could we simply return one of the existing error codes when setting the alarm too late, and clean up (cancel the alarm) internally? Then the user can just call the rtc_set_alarm() again. This would be the same as bullet point 5 in my comment from 22/Mar/17 3:28 PM:

• Return an error code if the CC cannot be set (the time has passed or is too close in the future) and let the caller deal with the corner cases.

This of course changes the third assumption:

• set_alarm() function first reads current counter value. If (alarm_value >= now) alarm will be triggered in current 32 counter cycle, otherwise it will be set in far future. It means that setting very short alarm needs precautions or even irq_locking() for avoiding preemption. These precautions are not implement in the driver, they should be taken in code layer that uses the driver. The precautions are then taken in the driver, and the timer is never set in the far future (the only valid timeouts can be from "now + margin for CC" to "now + UINT32_MAX/2"). Hopefully this would require no API change.

[nashif]

by Michał Kruszewski:

Øyvind Hovdsveen Carles Cufi have suggested to target counter.h API. I wanted to make some (I hope final) fixes but I am not sure what API to focus, rtc.h or counter.h? I have checked counter.h and I must agree with Carles Cufi that counter.h suits nordic RTC (real time COUNTER) peripheral more than zephyr rtc.h API that is designed for peripherals that can track real time with its internal registers., I guess?

[nashif]

by Øyvind Hovdsveen:

Michał Kruszewski I don't know counter.h very well, but from the looks of it I agree it seems to be a better fit for the Nordic RTC. To me it doesn't seem like rtc.h has any requirements to any special functionality in the underlying HW either and should be possible to implement on top of the Nordic RTC, but I agree the counter.h is a closer fit.

[nashif]

by Michał Kruszewski:

Carles Cufi Vinayak Kariappa Chettimada Øyvind Hovdsveen I have created initial pull request: https://github.com/zephyrproject-rtos/zephyr/pull/395 . Can you check it in free time?

[nashif]

Related to GH-685