Add A10/A20 rtc support
Details
Details
Apply patch and build kernel
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| sys/arm/allwinner/files.allwinner | ||
|---|---|---|
| 19 ↗ | (On Diff #13667) | This will likely need to be renamed aw_rtc.c based on changes proposed in D5280 |
| sys/arm/allwinner/sunxi_rtc.c | ||
| 83 ↗ | (On Diff #13667) | Weird alignment here. You should use #define<tab> consistently. |
| 153 ↗ | (On Diff #13667) | This should be set in the attach function, not probe. Also instead of allwinner_soc_family you should just use the compat string. |
| 170 ↗ | (On Diff #13667) | This is not necessary, you can use bus_read_*/bus_write_* on struct resource directly. |
| 187 ↗ | (On Diff #13667) | What is 1000000? |
| 215 ↗ | (On Diff #13667) | return (clock_ct_to_ts(&ct, ts)); |
| 229 ↗ | (On Diff #13667) | Split into two lines. |
| 242 ↗ | (On Diff #13667) | Split into two lines. |
| 249 ↗ | (On Diff #13667) | Why the trailing \ here? |
| 253 ↗ | (On Diff #13667) | Trailing \ again. |
| 257 ↗ | (On Diff #13667) | Split into two lines. |
| 265 ↗ | (On Diff #13667) | Split into two lines. |
| sys/arm/allwinner/sunxi_rtc.c | ||
|---|---|---|
| 218–219 ↗ | (On Diff #13667) | Why do you need a macro for this? It's only used a few times and hides what's happening in the loops. |
| sys/arm/allwinner/files.allwinner | ||
|---|---|---|
| 19 ↗ | (On Diff #13699) | Just flow the current naming structure, will reflect HEAD state |
| sys/arm/allwinner/files.allwinner | ||
|---|---|---|
| 19 ↗ | (On Diff #13699) | I don't understand this comment. |
| sys/arm/allwinner/sunxi_rtc.c | ||
| 50 ↗ | (On Diff #13699) | these should all be #define<tab>FOO |
| 138 ↗ | (On Diff #13699) | You should also have: if (!ofw_bus_status_okay(self)) return (ENXIO); |
| 142–144 ↗ | (On Diff #13699) | Is this needed? the previous check should be enough. |
| 172 ↗ | (On Diff #13699) | This should be: if ((val & LOSC_OSC_SRC) == 0) { |
Comment Actions
ammended new comments, updated to most recent HEAD
| sys/arm/allwinner/files.allwinner | ||
|---|---|---|
| 19 ↗ | (On Diff #13699) | The changes proposed in D5280 are not committed yet, should we add third naming scheme or everything will be renamed in one bunch ? |
| sys/arm/allwinner/sunxi_rtc.c | ||
| 142–144 ↗ | (On Diff #13699) | I can easily imagine errors in device tree scripts. Better to double check. |
| sys/arm/allwinner/sunxi_rtc.c | ||
|---|---|---|
| 142–144 ↗ | (On Diff #13699) | We assume the device tree is correct, if it can get this wrong we have bigger issues. |
| sys/arm/allwinner/aw_rtc.c | ||
|---|---|---|
| 145–148 ↗ | (On Diff #13744) | This check is redundant. The ofw_bus_search_compatible should be all that's required. If not, we have much bigger problems. |
| 219–221 ↗ | (On Diff #13744) | A high quality implementation would delay the right number of nano-seconds to be just before the top of second and then setting the registers. This implementation will cause issues with ntpd which likes to set the time forward or backwards by a few hundred milliseconds sometimes. Though I suppose this is important for only for ntp reboot performance. |
| sys/arm/allwinner/aw_rtc.c | ||
|---|---|---|
| 145–148 ↗ | (On Diff #13744) | Ok, this suddenly became a very good question :) If we look over device tree script we can see the common practice like that: compatible = "allwinner,sun7i-a20-gmac-clk";
...
compatible = "allwinner,sun7i-a20-gmac";
...
compatible = "allwinner,sun6i-a31-hstimer",
"allwinner,sun7i-a20-hstimer";sys/gnu/dts/arm/sun8i-a23-a33.dtsi: compatible = "allwinner,sun7i-a20-pwm"; sys/gnu/dts/arm/sun8i-a23-a33.dtsi: compatible = "allwinner,sun4i-a10-timer"; sys/gnu/dts/arm/sun9i-a80.dtsi: compatible = "allwinner,sun4i-a10-timer"; and many many more ... we can assume that, in one day, during copy paste porting process, compat string compatible = "allwinner,sun4i-a10-rtc"; move on the wrong place. It was especially possible due to the upcoming naming scheme change, and the name and location of this new (aw_rtc) module say nothing against this. Also in the file sys/arm/allwinner/files.allwinner we will have: arm/allwinner/aw_rtc.c standard and this module will be inside every allwinner kernel. Let's look what will happen later, nothing terrible, much bigger problems will never arise, after successful probe and attach this module will silently continue his live but do nothing. If you look to the code you will see that this module will "work" on passive (reserved) bus memory location but don't count time by the lack of hardware. And the user will never know about it, because he will see successfully loaded module and use ntpd or ntpdate. This is the last time when I try to save this poor chunk of the code. |
| 219–221 ↗ | (On Diff #13744) | Usually real time clock has only 1 second resolution and Allwinner SOC is not an exception. This means during reboot cycle we are only able to save time with 1 second precision. And to minimize error we need to round clock to the seconds. For example if we have system clock value like Nsec.600ms we should increment N to have error +400ms that is better then -600ms. |
| sys/arm/allwinner/aw_rtc.c | ||
|---|---|---|
| 145–148 ↗ | (On Diff #13744) | If somebody blindly copies compat strings, they deserve what they get. We don't have this defensive programming anywhere else, and it really serves no useful purpose. We can't protect against all stupidity, nor should we try. |
| 219–221 ↗ | (On Diff #13744) | Reboot is exactly the problem. While a 1 second RTC is common, other implementations try to align top of second better so that when we restart the error is much smaller. I can't recall of the code I did to do that on x86 got committed, or if I got frustrated with the bde nit-picking and punted. It is quite useful for people building time boxes. Not a huge market, granted, but it is an issue none-the-less. But not a blocking issue. A quick survey of the FreeBSD tree shows many other implementations like this one, alas.... |
Comment Actions
excess platform check removed, updated to the most recent HEAD
| sys/arm/allwinner/aw_rtc.c | ||
|---|---|---|
| 219–221 ↗ | (On Diff #13744) | English is not my native language, sorry i do not understand urban acronyms; alas? and bde i think is not borland database engine :) aml8726_rtc_settime, mv_rtc_settime - both, ds3231_settime, ds1307_settime, pcf8563_settime, pcf2123_rtc_settime, ds1553_settime, mc146818_settime, mk48txx_settime - use round method xentimer_settime, ps3_settime - almost do nothing rtas_settime - store time with nanoseconds (IBM!) at91_rtc_settime, lpc_rtc_settime, s390rtc_settime, ds1672_settime, ds1374_settime, ds133x_settime, octeon_rtc_settime, pcrtc_settime, pmu_settime, smu_settime, cuda_settime, sbbc_tod_settime, atrtc_settime - do not bother about nanoseconds, what is may be better, because if we look inside the sys/kern/subr_rtc.c, we found that: the maximum error is calculated inside of clock_register for the specified resolution that is equal to half resolution value. Both inittodr and resettodr then step the time forward for this error. In our case it would 1 sec for the whole reboot cycle. But i am would be happy to do something with it. |
| sys/arm/allwinner/aw_rtc.c | ||
|---|---|---|
| 220–222 ↗ | (On Diff #13839) | alas isn't an acronym. It's is an expression of minor disappointment. bde is Bruce Evans. He's somewhat pedantic and likes to talk issues to death. He makes my concerns raised here look minor and trivial. I just looked, and it looks like I never committed the busy waiting... so sorry for distraction. what's here is fine. |