drivers: add snvs srtc support #1700
Conversation
| * When cpu/bus runs at low freq, we may never get same value | ||
| * during two consecutive read, so only compare the second value. | ||
| */ | ||
| } while ((val1 >> CNT_TO_SECS_SHIFT) != (val2 >> CNT_TO_SECS_SHIFT)); |
There was a problem hiding this comment.
Why do we read it twice and compare the result?
There was a problem hiding this comment.
Because the value is in two registers, first read low, then read high, low may overflow. The two reads is to keep high consistent and handle low register overflow.
core/drivers/imx_snvs.c
Outdated
| REGISTER_TIME_SOURCE(snvs_srtc_time_source) | ||
|
|
||
| /* Needs to be invoked before service_init */ | ||
| int snvs_srtc_enable(bool enable) |
There was a problem hiding this comment.
Return type should be TEE_Result
core/drivers/imx_snvs.c
Outdated
| } | ||
| } | ||
| if (bytes) { | ||
| FMSG("%s: 0x%02X\n", __func__, |
There was a problem hiding this comment.
\n isn't needed
To avoid the suspicious cast you could replace the X", with " PRIX16
core/drivers/imx_snvs.c
Outdated
| } | ||
| if (bytes) { | ||
| FMSG("%s: 0x%02X\n", __func__, | ||
| (int)acc & ((1 << (bytes * 8)) - 1)); |
There was a problem hiding this comment.
Please use the BIT32() macro instead of 1 << whatever, perhaps GENMASK_32() is an even better choice here.
Using signed types and bit operations is balancing on the border of undefined behavior.
There was a problem hiding this comment.
Fix in V2. This piece code is reused from arm cnt part.
core/include/drivers/imx_snvs.h
Outdated
| * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE | ||
| * POSSIBILITY OF SUCH DAMAGE. | ||
| */ | ||
| #ifndef IMX_SNVS_H |
There was a problem hiding this comment.
Please use __DRIVERS_IMX_SNVS_H as guard instead.
core/include/drivers/imx_snvs.h
Outdated
|
|
||
| #include <types_ext.h> | ||
|
|
||
| int snvs_srtc_enable(bool enable); |
There was a problem hiding this comment.
Calling this function as snvs_srtc_enable(false) seems a bit confusing.
I'd prefer snvc_srtc_enable(void) and snvc_srtc_dissable(void).
It it's to be only one function, perhaps snvc_srtc_init(bool enable) or snvc_srtc_set_status(bool enable)
There was a problem hiding this comment.
when SRTC starts, it seems disabling it is not a good idea. I'll refine this part to make it only have enabling function.
|
@jenswi-linaro Review updated. |
core/drivers/imx_snvs.c
Outdated
| } | ||
| if (bytes) { | ||
| FMSG("%s: 0x%02" PRIX16, __func__, | ||
| (int)acc & GENMASK_32(bytes * 8, 0)); |
There was a problem hiding this comment.
Please drop the unneeded cast (int) also
|
Updated with int removed. |
|
|
Introduce i.MX SNVS SRTC support. The SRTC works with 32.768KHz. The SRTC is in SNVS_LP domain. The SNVS_LP is a data storage subsystem with enhanced security capabilities. Its purpose is to store and protect system data, regardless of the main system power state. SNVS_LP is in the always-powered-up domain, which is a separate power domain with its own power supply. When the chip power supply domain loses power, SNVS_LP continues to operate normally. Since OP-TEE does not care about calendar time, there is no need to update calendar time, we only need to read the counter and get out the time. The plat_prng_add_jitter_entropy is reused from tee_time_arm_cntpct.c. Signed-off-by: Peng Fan <peng.fan@nxp.com> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
|
tags applied. |
Introduce i.MX SNVS SRTC support. The SRTC works with 32.768KHz.
The SRTC is in SNVS_LP domain. The SNVS_LP is a data storage
subsystem with enhanced security capabilities. Its purpose is to store
and protect system data, regardless of the main system power state.
SNVS_LP is in the always-powered-up domain, which is a separate power
domain with its own power supply. When the chip power supply domain
loses power, SNVS_LP continues to operate normally.
Since OP-TEE does not care about calendar time, there is no need
to update calendar time, we only need to read the counter and
get out the time.
The plat_prng_add_jitter_entropy is reused from tee_time_arm_cntpct.c.
This driver works on i.MX7D-SDB with CFG_IMX_SNVS=y and CFG_SECURE_TIME_SOURCE_REE=n and invoke "snvs_srtc_enable(true)" in plat_cpu_reset_late. In linux side, needs to first disable SNVS SRTC in dts.
There are two issues #1352 #1673 discussing SRTC support. Currently, the snvs srtc only support rtc counter reading and no set time. Moving SRTC to OP-TEE will break the linux to use SRTC, so needs to add SIP/OEM to support Linux driver could set some SNVS SRTC register to trigger alarm and wakeup system.
Signed-off-by: Peng Fan peng.fan@nxp.com