Add EFI RTC support
ClosedPublic
Actions

Authored by andrew on Oct 13 2017, 9:31 AM.

Details

Reviewers

kib
imp

Commits

rS326066: Add a driver for the EFI RTC. This uses the EFI Runtime Services to query

Summary

Add a driver to read teh system time from the EFI rtc. This allows arm64
to use this to get the time on boot.

Test Plan

So far only tested on ThunderX

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

andrew created this revision.Oct 13 2017, 9:31 AM

Harbormaster completed remote builds in B12018: Diff 33935.Oct 13 2017, 9:31 AM

kib added inline comments.Oct 13 2017, 9:47 AM

sys/dev/efidev/efirtc.c
48 ↗	(On Diff #33935)	Does this line require some external code to create the efirtc device ? I mean that this seems to prevent the driver to work on amd64, while I think it is desirable to have the ability to use it on x86.
67 ↗	(On Diff #33935)	Uneeded empty line. Perhaps, if using this on amd64, driver should check the efirt state and not attach blindly. On amd64, efirt was changed to load even if efi table is not present. Also efirt should order its initialization after the efirt init.
76 ↗	(On Diff #33935)	Same.
sys/modules/efirt/Makefile
9 ↗	(On Diff #33935)	Perhaps group all human-readable sources first, and put the XXX_if.h generated files on its own separate SRCS line.

Be stricter when we use the driver
Update style base on feedback from kib

sys/dev/efidev/efirtc.c
48 ↗	(On Diff #33935)	It should add it as a child of nexus, however I haven't tried it on x86.
67 ↗	(On Diff #33935)	How do you mean? The efirt driver will be loaded at SI_SUB_VM_CONF where this will be later in SI_SUB_DRIVERS. If they have been loaded at boot they will be in order. I'm not sure how they interace when loaded as a module, however clock_register doesn't seem to allow a new device to be used after boot.

this looks good to my eye... Stretch goal would be to allow adding clocks at runtime, but it's unclear what that would mean or how useful that would be given our current use cases....

This revision is now accepted and ready to land.Nov 16 2017, 8:40 PM

kib accepted this revision.Nov 17 2017, 3:28 PM

I'm building a new kernel to test this on a SoftIron OverDrive 1000 now.

In D12650#273064, @emaste wrote:

I'm building a new kernel to test this on a SoftIron OverDrive 1000 now.

I believe that the patch was already tested on arm64, IMO amd64 would be much more interesting.

In D12650#273067, @kib wrote:

In D12650#273064, @emaste wrote:

I'm building a new kernel to test this on a SoftIron OverDrive 1000 now.

I believe that the patch was already tested on arm64, IMO amd64 would be much more interesting.

Also useful would be something with U-Boot that returns an error when querying the time.

Attempting to set the time on OD1000:

root@od1000:~ # date 1711171600
timeout stopping cpus
panic: invalid fpcurthread
cpuid = 2
time = 1510952400
KDB: stack backtrace:
db_trace_self() at db_trace_self_wrapper+0x28
         pc = 0xffff00000060d8f8  lr = 0xffff000000086eec
         sp = 0xffff0000e64e93e0  fp = 0xffff0000e64e95f0

db_trace_self_wrapper() at vpanic+0x184
         pc = 0xffff000000086eec  lr = 0xffff000000316a48
         sp = 0xffff0000e64e9600  fp = 0xffff0000e64e9680

vpanic() at kassert_panic+0x158
         pc = 0xffff000000316a48  lr = 0xffff0000003168c0
         sp = 0xffff0000e64e9690  fp = 0xffff0000e64e9750

kassert_panic() at fpu_kern_enter+0x68
         pc = 0xffff0000003168c0  lr = 0xffff000000628974
         sp = 0xffff0000e64e9760  fp = 0xffff0000e64e9780

fpu_kern_enter() at efi_enter+0x70
         pc = 0xffff000000628974  lr = 0xffff0000000ca26c
         sp = 0xffff0000e64e9790  fp = 0xffff0000e64e97b0

efi_enter() at efi_set_time+0x20
         pc = 0xffff0000000ca26c  lr = 0xffff0000000ca2dc
         sp = 0xffff0000e64e97c0  fp = 0xffff0000e64e97e0

efi_set_time() at efirtc_settime+0x4c
         pc = 0xffff0000000ca2dc  lr = 0xffff0000000ca9c4
         sp = 0xffff0000e64e97f0  fp = 0xffff0000e64e9830

efirtc_settime() at settime_task_func+0xc0
         pc = 0xffff0000000ca9c4  lr = 0xffff0000003603e8
         sp = 0xffff0000e64e9840  fp = 0xffff0000e64e9860

settime_task_func() at taskqueue_run_locked+0x104
         pc = 0xffff0000003603e8  lr = 0xffff000000369144
         sp = 0xffff0000e64e9870  fp = 0xffff0000e64e98e0

taskqueue_run_locked() at taskqueue_thread_loop+0x9c
         pc = 0xffff000000369144  lr = 0xffff000000369fac
         sp = 0xffff0000e64e98f0  fp = 0xffff0000e64e9910

taskqueue_thread_loop() at fork_exit+0x7c
         pc = 0xffff000000369fac  lr = 0xffff0000002dacfc
         sp = 0xffff0000e64e9920  fp = 0xffff0000e64e9950

fork_exit() at fork_trampoline+0x10
         pc = 0xffff0000002dacfc  lr = 0xffff00000062701c
         sp = 0xffff0000e64e9960  fp = 0x0000000000000000

KDB: enter: panic
[ thread pid 0 tid 100008 ]
Stopped at      0
db>

I believe that the patch was already tested on arm64, IMO amd64 would be much more interesting.

Yes, but I think we can reasonably do that later on; this can be committed for arm64 first.

Tested successfully on SoftIron OverDrive 1000 after applying fpu_kern_enter fix from @andrew via IRC

emaste added inline comments.Nov 20 2017, 4:21 PM

sys/dev/efidev/efirtc.c
127 ↗	(On Diff #35326)	what about tm_tz and tm_dst?

Closed by commit rS326066: Add a driver for the EFI RTC. This uses the EFI Runtime Services to query (authored by andrew). · Explain WhyNov 21 2017, 5:23 PM

This revision was automatically updated to reflect the committed changes.

andrew added inline comments.Nov 21 2017, 5:26 PM

sys/dev/efidev/efirtc.c
127 ↗	(On Diff #35326)	I'm not sure how best to handle them. I expect we are given UTC here. SDhould we be adjusting the time sent to the firmware based on the time zone, or just zeroing these?

emaste added inline comments.Nov 21 2017, 5:31 PM

sys/dev/efidev/efirtc.c
127 ↗	(On Diff #35326)	Good question, probably we want to store UTC in the rtc, but we should at least not write uninitialized values to tm_tz and tm_dst :-)