hal/armv7a/imx6ull: add pctl for RTC config#751
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces the necessary infrastructure to manage the Secure Non-Volatile Storage (SNVS) Real-Time Clock (RTC) on the i.MX6ULL platform. It enables privileged software to programmatically control the RTC's state, addressing the security requirement that SNVS components reside in a secure domain. The changes involve memory mapping, register definitions, and a new platform control interface for RTC operations. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
The pull request introduces support for the i.MX6ULL's Secure Non-Volatile Storage (SNVS) and its Real-Time Clock (RTC). This involves mapping SNVS registers during system initialization, defining SNVS register offsets, adding SNVS register access to the platform's common structure, and implementing a function to control the RTC's enable/disable state. This RTC control is exposed through the hal_platformctl interface. A review comment notes that the newly introduced pctl_rtc_set_state enum in imx6ull.h is currently unused and should either be removed or refactored for better clarity.
| /* SNVS domain */ | ||
| enum { | ||
| pctl_rtc_set_state | ||
| }; |
There was a problem hiding this comment.
The new enum pctl_rtc_set_state is defined but not used anywhere in the codebase. It seems to add confusion without providing functionality. If the intention is to define constants for enabling/disabling the RTC, it would be better to define them explicitly (e.g., RTC_STATE_DISABLE = 0, RTC_STATE_ENABLE = 1). As it is, this enum should probably be removed to avoid confusion.
SEN-92
Description
Since SNVS is in secure domain, only privileged software can change its state. Adding basic pctl
for secure RTC enabling/disabling was necessary
Tested and working, but please review if adding snvs registers to kernel page tables is done properly.
Motivation and Context
Types of changes
How Has This Been Tested?
Checklist:
Special treatment