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

include/drivers: Add RTC support #52618

Conversation

bjarki-andreasen
Copy link
Collaborator

@bjarki-andreasen bjarki-andreasen commented Nov 29, 2022

Introduction

This PR adds an API for real-time clocks.

A real time clock (RTC) contains a gregorian or persian calendar, which keeps track of date and time. These contain alarms which compare with fields of the RTC date and time, like weekday, hour and minute.

Emulated device driver

This PR introduces and emulated RTC device and device driver. It can be added to the devicetree like any other device, and will act just like a real RTC device, except it is unable to retain the clock or calendar between resets, and is most likely less accurate since it uses the system clock to track time.

rtc: rtc {
	status = "okay";
	compatible = "zephyr,rtc-emul";
	alarms-count = <2>;
};

The devices can be configured to support any amount of alarms since they are entirely software, which is useful for emulating the capabilities of any real hardware.

The emulated RTC device has been added to the native_posix board, simply enable CONFIG_RTC to include the drivers for them.

Real device driver

The first real device driver has been created and tested. It supports the ATSAM family of chips. It has been dropped from this PR to avoid delaying the merge of this PR, since it is blocked by another PR unrelated to the RTC API.

RTC API tests

Tests have been added which test the behavior of RTC devices. The tests have been validated against the emulated RTC device driver implemented on the native_posix board, and the ATSAM device driver using a proprietary board.

The API tests will run on any board which has an alias rtc in the boards yaml file, and includes the alias rtc in the boards devicetree.

supported:
  - rtc
aliases {
	rtc = &rtc0;
};

Copy link
Member

@cfriedt cfriedt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks ok to me

include/zephyr/drivers/wakeup-timer.h Outdated Show resolved Hide resolved
@cfriedt
Copy link
Member

cfriedt commented Nov 29, 2022

Should these be in the doxygen Io group?

@finikorg finikorg self-requested a review November 29, 2022 14:10
* @param mask Parameters of the date and time to match
* @param handler Handler invoked once the alarm is triggered
* @param user_data Optional user data which passed to the handler when invoked
* @note Handler and user data will be lost of device resets before alarm is triggered
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing documentation end, does it compile?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does not :) It is a draft to discuss which functions should be in which API

rtcc_time_get_t time_get;
rtcc_alarm_start_t alarm_start;
rtcc_alarm_cancel_t alarm_cancel;
rtcc_timestamp_get_t timestamp_get;
Copy link
Collaborator

@akanisetti akanisetti Nov 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, I am trying to develop an individual RTC driver for MC146818B RTC, available on many x86 boards.
I have checked these header files and rtcc.h will be very useful. But this could have two more APIs which will support READ/WRITE of CMOS battery backed SRAM (256 bytes in my case).
APIs similar to

uint8_t (*read_offset)(const struct device *dev, struct rtc_address *addr) &
int (*write_offset)(const struct device *dev, struct rtc_address *addr, uint8_t value)
and supporting data types,

struct rtc_address {
	/* RAM bank */
	enum rtc_ram_bank rtc_bank;
	/* offset */
	uint8_t offset;
};
 
enum rtc_ram_bank {
	RTC_STD_RAM,
	RTC_EXT_RAM,
};

This enum helps to identify between 2 blocks of SRAM.

This memory could be used by BIOS or for any other services.

Copy link
Collaborator Author

@bjarki-andreasen bjarki-andreasen Nov 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The retention registers are not specific to an RTC module, some I2C attached sensors also allow for retained registers, and some MCUs have internal retention registers and retained SRAM.

We need a tailored API for reading/writing to SRAM, retention registers, and other forms of retained memory, like external EEPROMs.

This functionality is then added to the external RTC node in the devicetree as such:

&i2c2 {
    pinctrl-0 = <&i2c2_scl_ph4 &i2c2_sda_ph5>;
    pinctrl-names = "default";
    status = "okay";
    clock-frequency = <I2C_BITRATE_FAST>;

    /* Implements RTCC API */
    pca8565@2a {
        compatible = "pca8565";
        reg = <0x2a>;

        /* Implements said reg read/write API */
        backup_regs {
            compatible =  "pca8565_backup_regs";
        };	
    };
};

The RTCC API should be limited to setting and getting time. Retained memory needs its own API that can then be implemented by RTC modules that support the feature. Some RTC modules also support watchdog, and we don't want to add that functionality to the rtc or rtcc APIs either, it already has a dedicated API.

This issue #51298 has some more discussion around this. We still don't have an API for performing this, because it is actually quite complex to create a solution that can be used effectively in the devicetree and as a driver API :)

Copy link
Member

@cfriedt cfriedt Nov 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's quite possible that another API and device tree binding already exists that would address this. We don't need to have separate API just for reading and writing at an offset.

E.g. flash.h

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, thank you suggestion. I found that "bbram.h" will serve the purpose for reading/writing to battery backed RAM.

Regarding the APIs in rtcc.h, could there be a possibility to add an API to get current alarm time like rtcc_get_alarm.

Copy link
Collaborator Author

@bjarki-andreasen bjarki-andreasen Dec 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, thank you suggestion. I found that "bbram.h" will serve the purpose for reading/writing to battery backed RAM.

Regarding the APIs in rtcc.h, could there be a possibility to add an API to get current alarm time like rtcc_get_alarm.

We can definitely add that. Can you think of a use case for getting the timer configuration? The Raspberry Pi Pico SDK doesn't provide an alarm_get https://raspberrypi.github.io/pico-sdk-doxygen/group__hardware__rtc.html And what should happen if the timer is not configured, should we return an error code maybe?

@carlescufi
Copy link
Member

carlescufi commented Nov 29, 2022

Architecture WG:

  • History of the RTC API in Zephyr:
  • @bjarki-trackunit presents the differences between this PR and drivers: rtc: define RTC API driver #41287
    • Supports:
      • rtc.h: Simpler counter API, with promise to work on power down
      • rtcc.h: Calendar-based API
      • wakeup_timer.h: Simple timer that wakes up the system
    • The reason for the 3 APIs is the huge difference in functionality across RTC models and MCU models
  • @stephanosio thinks is a good idea to split up the APIs from the counter one, given the different nature of these devices
  • @cfriedt: Part of the reason why the RTC API became relevant again, is because Stephanos, Keith Packard & I discovered a layering violation in the libc. Currently, PicoLibC (and newlib, FWIR) call into posix functions for clocks. However, we can't have the libc relying on POSIX functions. We need an actual hardware API for a clock that includes calendar functionality. So the proper layering would be libc -> Zephyr RTC(c) API, and then the POSIX subsystem should also ideally reference the Zephyr RTC(c) API
  • @stephanosio mentions that we might need to unify time handling in a single place in Zephyr
  • @nashif agrees with @stephanosio's comments on both splitting up and unifying the time handling in Zephyr

@bjarki-andreasen
Copy link
Collaborator Author

bjarki-andreasen commented Dec 1, 2022

@cfriedt

This following idea makes it possible to access rtc and rtcc alarms and timestamps, from the devicetree, making it possible to create a time and wakeup subsystem, which will utilize the specified hardware to keep time and possibly configure alarms for wakeup.

/* Chosen nodes used to specifiy hardware to be used for time subsystem */
chosen {
    zephyr,rtcc = &rtcc0;
    zephyr,wakeup-timer = &wup0;
    zephyr,rtcc-alarm = &rtcc0_alarm0;
};

/* RTCC block with 2 alarms and 1 timestamp accessible using node labels */
rtcc0: rtcc0@40000000 {
    compatible = "dummy,rtcc";
    reg = <0x40000000 0x028>;    

    alarms {
        #address-cells = <1>;
        #size-cells = <0>;

        rtcc0_alarm0: rtcc_alarm@0 {
            reg = <0x0>;
        };
        rtcc0_alarm1: rtcc_alarm@1 {
            reg = <0x1>;
        }; 
    };

    timestamps {
        rtcc0_timestamp0: timestamp@0 {
            reg = <0x0>;
        };
    };
};

/* Wakeup timer block accessible using node label */
wup0: wup@40000020 {
    compatible = "dummy,wup";
    reg = <0x40000020 0x008>;
};

From the applications perspective, the alarms and timestamps address is used to specify the id of alarm to use, similar to how an I2C slave address is specified. This uses the id field in the APIs.

@galak What do you think of this idea?

@carlescufi carlescufi requested review from cfriedt, finikorg and akanisetti and removed request for akanisetti December 5, 2022 14:24
@galak
Copy link
Collaborator

galak commented Dec 13, 2022

Dev-Review (Dec 13, 2022): discussed devicetree bits, decided to drop for now as there isn't a need and can bring back if needed in future.

@bjarki-andreasen
Copy link
Collaborator Author

bjarki-andreasen commented Dec 13, 2022

  • Add set/get config API to alarms
  • Add get triggered API to alarms
  • Add enable/disable API to alarms
  • Remove devicetree handles for alarms

@bjarki-andreasen bjarki-andreasen force-pushed the drivers_add_rtc_rtcc_wup_tmr branch 3 times, most recently from 659a515 to 05e6fd4 Compare December 18, 2022 07:38
@bjarki-andreasen bjarki-andreasen marked this pull request as ready for review December 18, 2022 08:17
@bjarki-andreasen
Copy link
Collaborator Author

Update

The three APIs have been updated based on the feedback from the dev-review (Dec 13 2022). They are now in a finalized state, ready for thorough review. If and once the APIs have been accepted, creating tests and drivers can begin.

(const struct rtc_driver_api *)dev->api;

if (api->counter_set == NULL) {
return -ENOSYS;
Copy link
Member

@cfriedt cfriedt Dec 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like all of the API calls are optional. Is that intentional?

If any API calls are optional, it should be explicitly called out (but there should be a very good reason for that).

Otherwise, we should remove all of the NULL checks. Attempting to execute NULL will naturally cause exceptions to be thrown / crash code.

Bjarki Arge Andreasen added 5 commits April 3, 2023 21:08
This commit adds the rtc.h header file which contains
API functions for real-time-clocks, which are low power
devices which track and represent broken-down time.

It also changes one line of doxygen documentation in the
maxim_ds3132.h file to place it in its own group.

The handlers for use of the API from userspace is also
added with this commit.

The API is split into one mandatory section, setting and
getting time, and three optional sections, alarms, update
event callback, and clock calibration.

Signed-off-by: Bjarki Arge Andreasen <baa@trackunit.com>
This test suite adds tests for the following:

- Setting and getting time
- Validating time is incrementing correctly
- Validating behavior of alarms with callback disabled
- Validating behavior of alarms with callback enabled
- Validating update callback

The test suite uses the devicetree  alias rtc to find
the device to test.

Signed-off-by: Bjarki Arge Andreasen <baa@trackunit.com>
The emulated RTC device driver is used to emulate a real
RTC device. Note that it is not a replacement for the
native_rtc module, which is used to control simulated time,
get time from the host system, etc.

Signed-off-by: Bjarki Arge Andreasen <baa@trackunit.com>
Added initial maintainers entry for RTC. Will be
expanded with collaborators before merged.

Signed-off-by: Bjarki Arge Andreasen <baa@trackunit.com>
This test suite validates the RTC API helper
functions, like currently, and exclusively, the
rtc_time_to_tm() helper function.

It also validates that the struct rtc_time is
member to member compatible with the struct tm
from the tm_sec to and including the tm_isdts
member.

Signed-off-by: Bjarki Arge Andreasen <baa@trackunit.com>
Copy link
Collaborator

@nordicjm nordicjm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor comments/fixes

between broken-down time and the unix timestamp within the RTC
drivers, which internally used the broken-down time representation.

The disadvantages of this approach where that hardware counters can
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
The disadvantages of this approach where that hardware counters can
The disadvantages of this approach were that hardware counters could

# SPDX-License-Identifier: Apache-2.0

menuconfig RTC
bool "RTC driver support"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would put "real time clock" in a help field here, just so that it is obvious to someone browsing Kconfig options without having to dive into the documentation

*/
typedef int (*rtc_api_get_calibration)(const struct device *dev, int32_t *calibration);

/**
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think this is missing here:

/**
 * @cond INTERNAL_HIDDEN
 *
 * For internal driver use only, skip these in public documentation.
 */

rtc_api_get_calibration get_calibration;
#endif /* CONFIG_RTC_CALIBRATION */
};

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And from here:

/** @endcond */

properties:
alarms-count:
type: int
required: true
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really required? Is a better solution not to have it default to 0?

console to be manually compared. The user must review the set and
gotten values to ensure they are valid.

By default, only the mandatory Setting and getting time is enabled
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
By default, only the mandatory Setting and getting time is enabled
By default, only the mandatory setting and getting of time is enabled

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why this is showing outdated? I opened all the commits just as I started reviewing this, github bug? If so/comment no longer applies, ignore

Comment on lines 30 to 31
``CONFIG_RTC_ALARM``, ``CONFIG_RTC_UPDATE`` and
``CONFIG_RTC_CALIBRATION``.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should use correct references

:kconfig:option:`<name>`

Comment on lines +84 to +85
``CONFIG_RTC_ALARM``, ``CONFIG_RTC_UPDATE`` and
``CONFIG_RTC_CALIBRATION``.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As before, use

:kconfig:option:`<name>`

syntax

contains the device tree alias ``rtc``, the following command can be used
for reference:

::
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like these parts should be using .. code-block:: bash ?

tests:
drivers.rtc.rtc_api:
tags: drivers rtc api helpers
platform_allow: native_posix
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is probably going to need to also run on at least variations of all architectures, e.g. mips, arm, etc. and if they have different endian options.

@nordicjm
Copy link
Collaborator

nordicjm commented Apr 4, 2023

Also needs release notes update

Comment on lines +9 to +24
config RTC_ALARM
bool "RTC driver alarm support"
help
This is an option which enables driver support for RTC alarms.

config RTC_UPDATE
bool "RTC driver update event callback support"
help
This is an option which enables driver support for the RTC
update event callback.

config RTC_CALIBRATION
bool "RTC driver clock calibration support"
help
This is an option which enables driver support for RTC clock
calibration.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these really necessary? At some point, configuration options get too fine-grained.

@cfriedt
Copy link
Member

cfriedt commented Apr 9, 2023

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Base OS Base OS Library (lib/os) area: Devicetree Binding PR modifies or adds a Device Tree binding area: Documentation area: native port Host native arch port (native_sim) area: Process platform: Microchip SAM Microchip SAM Platform (formerly Atmel SAM)
Projects
None yet
Development

Successfully merging this pull request may close these issues.