The RTC driver topic

[Giovanni]

Ok, now it works (rev. 3790). Added lock for threadsafe and interruptsafe usage.
Also I think that disabling write access to BKP register is pointless. Write protection needed only during startup phase to protect from parasitic write. Lets programmer to think about safe writing to BKP.

I don't agree on this one, the purpose is also to protect it from crashes or other malfunctions. The access should enabled when required and disabled soon after.

Giovanni

[Giovanni]

I think the best course of action is to study the ST RTC test application and use the same protocol for accessing the registers, I will give it a try tomorrow.

Hopefully the RTCv2 in L1, F2 and F4 is less problematic.

Giovanni

[barthess]

Code: Select all

static void rtc_lld_acquire(void) {

  /* Waits registers write completion.*/
BEGIN:
  while ((RTC->CRL & RTC_CRL_RTOFF) == 0)
    ;
  chSysLock();
  if ((RTC->CRL & RTC_CRL_RTOFF) == 0){
    chSysUnlock();
    goto BEGIN;
  }

That strange check realized to prevent writing in RTC registers from other places of program. I have observe deadlocks during debug in "while ((RTC->CRL & RTC_CRL_RTOFF) == 0)" cycle when playing with interrupts without that check. Any way I do not know about

You cannot use chSysLock() and chSysUnlock() from the initialization function

that is my fault. But we must realize exclusive writing access to RTC registers.

Code: Select all

  /* All interrupts initially disabled.*/
  if (RTC->CRH != 0){
    rtc_lld_acquire();
    RTC->CRH = 0;
    rtc_lld_release();

IMO not too redundant because rtc_lld_acquire(); take additional time to run, but during init phase we can ignore it.

Code: Select all

    RTC->CRL &= ~(RTC_CRL_OWF | RTC_CRL_ALRF | RTC_CRL_SECF);
    RTC->CRH &= ~(RTC_CRH_OWIE | RTC_CRH_ALRIE | RTC_CRH_SECIE);

just to be safe in case when ST add some new flags to that registers in next MCU families. My be you are right about CRH, but CRL flags is better to clear in personal way.

I don't agree on this one, the purpose is also to protect it from crashes or other malfunctions.

You are right, we must be as safe as possible.

[barthess]

Also I propose to replace

Code: Select all

PWR->CR |= PWR_CR_DBP;

and its counterpart by more readable macros, something like bkp_lld_write_enable()

[Giovanni]

I also noticed in the ST code that there is a function to wait bus synchronization before reading RTC registers, we should implement that as well.

I agree that some abstraction is desirable instead that toggling bits into registers so we will keep the acquire/release mechanism integrating it with the wait.

Giovanni

[barthess]

I also noticed in the ST code that there is a function to wait bus synchronization before reading RTC registers, we should implement that as well.

Agree.

[Giovanni]

Hi barthess,

I studied the problem some more and my conclusions are:

1) The PWR->PVD should be left enabled. Safety considerations are true but the RM specifies that it must stay enabled if the RTC is clocked by HSE. I don't want to have two different behaviors depending on the selected clock source so better keep it enabled like you did.
2) Waiting for write completion is only required for PRL, CNT, ALR registers, the CR register is not part of the backup domain and can always be written.
3) Waiting for APB1 synchronization is only required for DIV, CNT, ALR registers, the CR register is not part of the backup domain and can always be read.

I modified the driver according to this and also made some minor optimizations, can you review it when you have some time? the driver looks thin now, I like it (and I will like it even more after confirmation it works ).

The RTCv2 driver will have to wait after 2.4.0, after the RTCv1 driver is OK I will freeze this version.

Giovanni

[barthess]

it must stay enabled if the RTC is clocked by HSE. I don't want to have two different behaviors depending on the selected clock source

I forgot about it when discuss write protection of BKP registers.
As workaround we can add chDbgCheck about it in bkp_lld_writeprotect() macro.

can you review it when you have some time?

Will do it on this weekend.

[barthess]

Hi Giovanni,
After your changes driver is unable to set callback just because it can not write to CR without RTOFF bit. Problem stable reproduces on testahl program. As I understand, we always must poll RTOFF after changing registers requiring setting of CNF bit. So, the most clear and straightforward way is polling that bit before every writing access. Also I turn polling functions to macros. Now they look better in my opinion.

The second problem is closely related to the first. The driver is not thread safe.
http://rghost.net/35894400/image.png

Yes, it is hardly probable but probable situation. We can not ignore it because ChibiOS is not Windows. So we must realize exclusive write access to rtc. What about exclusion with semaphores/mutexes like in other drivers? Or simple macros with locks?

About disabling write access to BKP registers. Let it be in current state. Developer always can use it if he want.

[Giovanni]

I see, so even the access to CR must be preceded by a wait. Macros are OK too but consider that small static functions are always inlined so there is no effective gain unless you work with -O0.

About locks, probably it is a good idea for the rtcSetCallback() function but then we will have to introduce an "I" variant too in order to allos it from ISR context, I will do this tomorrow in the HLL, hopefully it will be the last iteration, I want to begin tests for 2.4.0 release now.

Giovanni