| Message ID | 20210503154545.152714-1-juergh@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | [SRU,F/raspi] rtc: class: support hctosys from modular RTC drivers | expand |
On 03/05/2021 11:45, Juerg Haefliger wrote: > From: Steve Muckle <smuckle@google.com> > > BugLink: https://bugs.launchpad.net/bugs/1926911 > > Due to distribution constraints it may not be possible to statically > compile the required RTC driver into the kernel. > > Expand RTC_HCTOSYS support to cover all RTC devices (statically compiled > or not) by checking at the end of RTC device registration whether the > time should be synced. > > Signed-off-by: Steve Muckle <smuckle@google.com> > Link: https://lore.kernel.org/r/20191106194625.116692-1-smuckle@google.com > Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com> > (cherry picked from commit f9b2a4d6a5f18e0aaf715206a056565c56889d9f) > Signed-off-by: Juerg Haefliger <juergh@canonical.com> > --- > drivers/rtc/Kconfig | 3 -- > drivers/rtc/Makefile | 1 - > drivers/rtc/class.c | 61 ++++++++++++++++++++++++++++++++++++++ > drivers/rtc/hctosys.c | 69 ------------------------------------------- > 4 files changed, 61 insertions(+), 73 deletions(-) > delete mode 100644 drivers/rtc/hctosys.c > Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> Best regards, Krzysztof
On 03/05/2021 16:45, Juerg Haefliger wrote: > From: Steve Muckle <smuckle@google.com> > > BugLink: https://bugs.launchpad.net/bugs/1926911 > > Due to distribution constraints it may not be possible to statically > compile the required RTC driver into the kernel. > > Expand RTC_HCTOSYS support to cover all RTC devices (statically compiled > or not) by checking at the end of RTC device registration whether the > time should be synced. > > Signed-off-by: Steve Muckle <smuckle@google.com> > Link: https://lore.kernel.org/r/20191106194625.116692-1-smuckle@google.com > Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com> > (cherry picked from commit f9b2a4d6a5f18e0aaf715206a056565c56889d9f) > Signed-off-by: Juerg Haefliger <juergh@canonical.com> > --- > drivers/rtc/Kconfig | 3 -- > drivers/rtc/Makefile | 1 - > drivers/rtc/class.c | 61 ++++++++++++++++++++++++++++++++++++++ > drivers/rtc/hctosys.c | 69 ------------------------------------------- > 4 files changed, 61 insertions(+), 73 deletions(-) > delete mode 100644 drivers/rtc/hctosys.c > > diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig > index 778cb0e63055..d9daa6ec45ff 100644 > --- a/drivers/rtc/Kconfig > +++ b/drivers/rtc/Kconfig > @@ -41,9 +41,6 @@ config RTC_HCTOSYS_DEVICE > device should record time in UTC, since the kernel won't do > timezone correction. > > - The driver for this RTC device must be loaded before late_initcall > - functions run, so it must usually be statically linked. > - > This clock should be battery-backed, so that it reads the correct > time when the system boots from a power-off state. Otherwise, your > system will need an external clock source (like an NTP server). > diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile > index a20943eafaab..9918b0429383 100644 > --- a/drivers/rtc/Makefile > +++ b/drivers/rtc/Makefile > @@ -6,7 +6,6 @@ > ccflags-$(CONFIG_RTC_DEBUG) := -DDEBUG > > obj-$(CONFIG_RTC_LIB) += lib.o > -obj-$(CONFIG_RTC_HCTOSYS) += hctosys.o > obj-$(CONFIG_RTC_SYSTOHC) += systohc.o > obj-$(CONFIG_RTC_CLASS) += rtc-core.o > obj-$(CONFIG_RTC_MC146818_LIB) += rtc-mc146818-lib.o > diff --git a/drivers/rtc/class.c b/drivers/rtc/class.c > index 9458e6d6686a..8793b2b8cf9d 100644 > --- a/drivers/rtc/class.c > +++ b/drivers/rtc/class.c > @@ -34,6 +34,62 @@ static void rtc_device_release(struct device *dev) > #ifdef CONFIG_RTC_HCTOSYS_DEVICE > /* Result of the last RTC to system clock attempt. */ > int rtc_hctosys_ret = -ENODEV; > + > +/* IMPORTANT: the RTC only stores whole seconds. It is arbitrary > + * whether it stores the most close value or the value with partial > + * seconds truncated. However, it is important that we use it to store > + * the truncated value. This is because otherwise it is necessary, > + * in an rtc sync function, to read both xtime.tv_sec and > + * xtime.tv_nsec. On some processors (i.e. ARM), an atomic read > + * of >32bits is not possible. So storing the most close value would > + * slow down the sync API. So here we have the truncated value and > + * the best guess is to add 0.5s. > + */ > + > +static int rtc_hctosys(void) > +{ > + int err = -ENODEV; > + struct rtc_time tm; > + struct timespec64 tv64 = { > + .tv_nsec = NSEC_PER_SEC >> 1, > + }; > + struct rtc_device *rtc = rtc_class_open(CONFIG_RTC_HCTOSYS_DEVICE); > + > + if (!rtc) { > + pr_info("unable to open rtc device (%s)\n", > + CONFIG_RTC_HCTOSYS_DEVICE); > + goto err_open; > + } > + > + err = rtc_read_time(rtc, &tm); > + if (err) { > + dev_err(rtc->dev.parent, > + "hctosys: unable to read the hardware clock\n"); > + goto err_read; > + } > + > + tv64.tv_sec = rtc_tm_to_time64(&tm); > + > +#if BITS_PER_LONG == 32 > + if (tv64.tv_sec > INT_MAX) { > + err = -ERANGE; > + goto err_read; > + } > +#endif > + > + err = do_settimeofday64(&tv64); > + > + dev_info(rtc->dev.parent, "setting system clock to %ptR UTC (%lld)\n", > + &tm, (long long)tv64.tv_sec); > + > +err_read: > + rtc_class_close(rtc); > + > +err_open: > + rtc_hctosys_ret = err; > + > + return err; > +} > #endif > > #if defined(CONFIG_PM_SLEEP) && defined(CONFIG_RTC_HCTOSYS_DEVICE) > @@ -375,6 +431,11 @@ int __rtc_register_device(struct module *owner, struct rtc_device *rtc) > dev_info(rtc->dev.parent, "registered as %s\n", > dev_name(&rtc->dev)); > > +#ifdef CONFIG_RTC_HCTOSYS_DEVICE > + if (!strcmp(dev_name(&rtc->dev), CONFIG_RTC_HCTOSYS_DEVICE)) > + rtc_hctosys(); > +#endif > + > return 0; > } > EXPORT_SYMBOL_GPL(__rtc_register_device); > diff --git a/drivers/rtc/hctosys.c b/drivers/rtc/hctosys.c > deleted file mode 100644 > index a74d0d890600..000000000000 > --- a/drivers/rtc/hctosys.c > +++ /dev/null > @@ -1,69 +0,0 @@ > -// SPDX-License-Identifier: GPL-2.0 > -/* > - * RTC subsystem, initialize system time on startup > - * > - * Copyright (C) 2005 Tower Technologies > - * Author: Alessandro Zummo <a.zummo@towertech.it> > - */ > - > -#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > - > -#include <linux/rtc.h> > - > -/* IMPORTANT: the RTC only stores whole seconds. It is arbitrary > - * whether it stores the most close value or the value with partial > - * seconds truncated. However, it is important that we use it to store > - * the truncated value. This is because otherwise it is necessary, > - * in an rtc sync function, to read both xtime.tv_sec and > - * xtime.tv_nsec. On some processors (i.e. ARM), an atomic read > - * of >32bits is not possible. So storing the most close value would > - * slow down the sync API. So here we have the truncated value and > - * the best guess is to add 0.5s. > - */ > - > -static int __init rtc_hctosys(void) > -{ > - int err = -ENODEV; > - struct rtc_time tm; > - struct timespec64 tv64 = { > - .tv_nsec = NSEC_PER_SEC >> 1, > - }; > - struct rtc_device *rtc = rtc_class_open(CONFIG_RTC_HCTOSYS_DEVICE); > - > - if (!rtc) { > - pr_info("unable to open rtc device (%s)\n", > - CONFIG_RTC_HCTOSYS_DEVICE); > - goto err_open; > - } > - > - err = rtc_read_time(rtc, &tm); > - if (err) { > - dev_err(rtc->dev.parent, > - "hctosys: unable to read the hardware clock\n"); > - goto err_read; > - } > - > - tv64.tv_sec = rtc_tm_to_time64(&tm); > - > -#if BITS_PER_LONG == 32 > - if (tv64.tv_sec > INT_MAX) { > - err = -ERANGE; > - goto err_read; > - } > -#endif > - > - err = do_settimeofday64(&tv64); > - > - dev_info(rtc->dev.parent, "setting system clock to %ptR UTC (%lld)\n", > - &tm, (long long)tv64.tv_sec); > - > -err_read: > - rtc_class_close(rtc); > - > -err_open: > - rtc_hctosys_ret = err; > - > - return err; > -} > - > -late_initcall(rtc_hctosys); > Yep, LGTM Acked-by: Colin Ian King <colin.king@canonical.com>
Hi, This will introduce a regression in certain configurations as mentioned on https://github.com/systemd/systemd/issues/17737 I had a tentative patch to kernel to only advance time in rtc_hctosys only if it is newer than the one returned by ktime_get_real_ts64. However that went nowhere. If this is cherry picked, imho we must change config CONFIG_RTC_HCTOSYS or at least set it to something sane because currently on raspi it is set to CONFIG_RTC_HCTOSYS_DEVICE="rtc0" which always points at the non-battery backed RTC from the SOC. Does above make any sense? On Mon, May 3, 2021 at 4:46 PM Juerg Haefliger <juerg.haefliger@canonical.com> wrote: > > From: Steve Muckle <smuckle@google.com> > > BugLink: https://bugs.launchpad.net/bugs/1926911 > > Due to distribution constraints it may not be possible to statically > compile the required RTC driver into the kernel. > > Expand RTC_HCTOSYS support to cover all RTC devices (statically compiled > or not) by checking at the end of RTC device registration whether the > time should be synced. > > Signed-off-by: Steve Muckle <smuckle@google.com> > Link: https://lore.kernel.org/r/20191106194625.116692-1-smuckle@google.com > Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com> > (cherry picked from commit f9b2a4d6a5f18e0aaf715206a056565c56889d9f) > Signed-off-by: Juerg Haefliger <juergh@canonical.com> > --- > drivers/rtc/Kconfig | 3 -- > drivers/rtc/Makefile | 1 - > drivers/rtc/class.c | 61 ++++++++++++++++++++++++++++++++++++++ > drivers/rtc/hctosys.c | 69 ------------------------------------------- > 4 files changed, 61 insertions(+), 73 deletions(-) > delete mode 100644 drivers/rtc/hctosys.c > > diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig > index 778cb0e63055..d9daa6ec45ff 100644 > --- a/drivers/rtc/Kconfig > +++ b/drivers/rtc/Kconfig > @@ -41,9 +41,6 @@ config RTC_HCTOSYS_DEVICE > device should record time in UTC, since the kernel won't do > timezone correction. > > - The driver for this RTC device must be loaded before late_initcall > - functions run, so it must usually be statically linked. > - > This clock should be battery-backed, so that it reads the correct > time when the system boots from a power-off state. Otherwise, your > system will need an external clock source (like an NTP server). > diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile > index a20943eafaab..9918b0429383 100644 > --- a/drivers/rtc/Makefile > +++ b/drivers/rtc/Makefile > @@ -6,7 +6,6 @@ > ccflags-$(CONFIG_RTC_DEBUG) := -DDEBUG > > obj-$(CONFIG_RTC_LIB) += lib.o > -obj-$(CONFIG_RTC_HCTOSYS) += hctosys.o > obj-$(CONFIG_RTC_SYSTOHC) += systohc.o > obj-$(CONFIG_RTC_CLASS) += rtc-core.o > obj-$(CONFIG_RTC_MC146818_LIB) += rtc-mc146818-lib.o > diff --git a/drivers/rtc/class.c b/drivers/rtc/class.c > index 9458e6d6686a..8793b2b8cf9d 100644 > --- a/drivers/rtc/class.c > +++ b/drivers/rtc/class.c > @@ -34,6 +34,62 @@ static void rtc_device_release(struct device *dev) > #ifdef CONFIG_RTC_HCTOSYS_DEVICE > /* Result of the last RTC to system clock attempt. */ > int rtc_hctosys_ret = -ENODEV; > + > +/* IMPORTANT: the RTC only stores whole seconds. It is arbitrary > + * whether it stores the most close value or the value with partial > + * seconds truncated. However, it is important that we use it to store > + * the truncated value. This is because otherwise it is necessary, > + * in an rtc sync function, to read both xtime.tv_sec and > + * xtime.tv_nsec. On some processors (i.e. ARM), an atomic read > + * of >32bits is not possible. So storing the most close value would > + * slow down the sync API. So here we have the truncated value and > + * the best guess is to add 0.5s. > + */ > + > +static int rtc_hctosys(void) > +{ > + int err = -ENODEV; > + struct rtc_time tm; > + struct timespec64 tv64 = { > + .tv_nsec = NSEC_PER_SEC >> 1, > + }; > + struct rtc_device *rtc = rtc_class_open(CONFIG_RTC_HCTOSYS_DEVICE); > + > + if (!rtc) { > + pr_info("unable to open rtc device (%s)\n", > + CONFIG_RTC_HCTOSYS_DEVICE); > + goto err_open; > + } > + > + err = rtc_read_time(rtc, &tm); > + if (err) { > + dev_err(rtc->dev.parent, > + "hctosys: unable to read the hardware clock\n"); > + goto err_read; > + } > + > + tv64.tv_sec = rtc_tm_to_time64(&tm); > + > +#if BITS_PER_LONG == 32 > + if (tv64.tv_sec > INT_MAX) { > + err = -ERANGE; > + goto err_read; > + } > +#endif > + > + err = do_settimeofday64(&tv64); > + > + dev_info(rtc->dev.parent, "setting system clock to %ptR UTC (%lld)\n", > + &tm, (long long)tv64.tv_sec); > + > +err_read: > + rtc_class_close(rtc); > + > +err_open: > + rtc_hctosys_ret = err; > + > + return err; > +} > #endif > > #if defined(CONFIG_PM_SLEEP) && defined(CONFIG_RTC_HCTOSYS_DEVICE) > @@ -375,6 +431,11 @@ int __rtc_register_device(struct module *owner, struct rtc_device *rtc) > dev_info(rtc->dev.parent, "registered as %s\n", > dev_name(&rtc->dev)); > > +#ifdef CONFIG_RTC_HCTOSYS_DEVICE > + if (!strcmp(dev_name(&rtc->dev), CONFIG_RTC_HCTOSYS_DEVICE)) > + rtc_hctosys(); > +#endif > + > return 0; > } > EXPORT_SYMBOL_GPL(__rtc_register_device); > diff --git a/drivers/rtc/hctosys.c b/drivers/rtc/hctosys.c > deleted file mode 100644 > index a74d0d890600..000000000000 > --- a/drivers/rtc/hctosys.c > +++ /dev/null > @@ -1,69 +0,0 @@ > -// SPDX-License-Identifier: GPL-2.0 > -/* > - * RTC subsystem, initialize system time on startup > - * > - * Copyright (C) 2005 Tower Technologies > - * Author: Alessandro Zummo <a.zummo@towertech.it> > - */ > - > -#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > - > -#include <linux/rtc.h> > - > -/* IMPORTANT: the RTC only stores whole seconds. It is arbitrary > - * whether it stores the most close value or the value with partial > - * seconds truncated. However, it is important that we use it to store > - * the truncated value. This is because otherwise it is necessary, > - * in an rtc sync function, to read both xtime.tv_sec and > - * xtime.tv_nsec. On some processors (i.e. ARM), an atomic read > - * of >32bits is not possible. So storing the most close value would > - * slow down the sync API. So here we have the truncated value and > - * the best guess is to add 0.5s. > - */ > - > -static int __init rtc_hctosys(void) > -{ > - int err = -ENODEV; > - struct rtc_time tm; > - struct timespec64 tv64 = { > - .tv_nsec = NSEC_PER_SEC >> 1, > - }; > - struct rtc_device *rtc = rtc_class_open(CONFIG_RTC_HCTOSYS_DEVICE); > - > - if (!rtc) { > - pr_info("unable to open rtc device (%s)\n", > - CONFIG_RTC_HCTOSYS_DEVICE); > - goto err_open; > - } > - > - err = rtc_read_time(rtc, &tm); > - if (err) { > - dev_err(rtc->dev.parent, > - "hctosys: unable to read the hardware clock\n"); > - goto err_read; > - } > - > - tv64.tv_sec = rtc_tm_to_time64(&tm); > - > -#if BITS_PER_LONG == 32 > - if (tv64.tv_sec > INT_MAX) { > - err = -ERANGE; > - goto err_read; > - } > -#endif > - > - err = do_settimeofday64(&tv64); > - > - dev_info(rtc->dev.parent, "setting system clock to %ptR UTC (%lld)\n", > - &tm, (long long)tv64.tv_sec); > - > -err_read: > - rtc_class_close(rtc); > - > -err_open: > - rtc_hctosys_ret = err; > - > - return err; > -} > - > -late_initcall(rtc_hctosys); > -- > 2.27.0 > > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team
See previous discussion about this attached as emails. On Tue, May 4, 2021 at 6:22 PM Dimitri John Ledkov <dimitri.ledkov@canonical.com> wrote: > > Hi, > > This will introduce a regression in certain configurations as > mentioned on https://github.com/systemd/systemd/issues/17737 > > I had a tentative patch to kernel to only advance time in rtc_hctosys > only if it is newer than the one returned by ktime_get_real_ts64. > > However that went nowhere. > > If this is cherry picked, imho we must change config > CONFIG_RTC_HCTOSYS or at least set it to something sane because > currently on raspi it is set to CONFIG_RTC_HCTOSYS_DEVICE="rtc0" which > always points at the non-battery backed RTC from the SOC. > > Does above make any sense? > > > On Mon, May 3, 2021 at 4:46 PM Juerg Haefliger > <juerg.haefliger@canonical.com> wrote: > > > > From: Steve Muckle <smuckle@google.com> > > > > BugLink: https://bugs.launchpad.net/bugs/1926911 > > > > Due to distribution constraints it may not be possible to statically > > compile the required RTC driver into the kernel. > > > > Expand RTC_HCTOSYS support to cover all RTC devices (statically compiled > > or not) by checking at the end of RTC device registration whether the > > time should be synced. > > > > Signed-off-by: Steve Muckle <smuckle@google.com> > > Link: https://lore.kernel.org/r/20191106194625.116692-1-smuckle@google.com > > Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com> > > (cherry picked from commit f9b2a4d6a5f18e0aaf715206a056565c56889d9f) > > Signed-off-by: Juerg Haefliger <juergh@canonical.com> > > --- > > drivers/rtc/Kconfig | 3 -- > > drivers/rtc/Makefile | 1 - > > drivers/rtc/class.c | 61 ++++++++++++++++++++++++++++++++++++++ > > drivers/rtc/hctosys.c | 69 ------------------------------------------- > > 4 files changed, 61 insertions(+), 73 deletions(-) > > delete mode 100644 drivers/rtc/hctosys.c > > > > diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig > > index 778cb0e63055..d9daa6ec45ff 100644 > > --- a/drivers/rtc/Kconfig > > +++ b/drivers/rtc/Kconfig > > @@ -41,9 +41,6 @@ config RTC_HCTOSYS_DEVICE > > device should record time in UTC, since the kernel won't do > > timezone correction. > > > > - The driver for this RTC device must be loaded before late_initcall > > - functions run, so it must usually be statically linked. > > - > > This clock should be battery-backed, so that it reads the correct > > time when the system boots from a power-off state. Otherwise, your > > system will need an external clock source (like an NTP server). > > diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile > > index a20943eafaab..9918b0429383 100644 > > --- a/drivers/rtc/Makefile > > +++ b/drivers/rtc/Makefile > > @@ -6,7 +6,6 @@ > > ccflags-$(CONFIG_RTC_DEBUG) := -DDEBUG > > > > obj-$(CONFIG_RTC_LIB) += lib.o > > -obj-$(CONFIG_RTC_HCTOSYS) += hctosys.o > > obj-$(CONFIG_RTC_SYSTOHC) += systohc.o > > obj-$(CONFIG_RTC_CLASS) += rtc-core.o > > obj-$(CONFIG_RTC_MC146818_LIB) += rtc-mc146818-lib.o > > diff --git a/drivers/rtc/class.c b/drivers/rtc/class.c > > index 9458e6d6686a..8793b2b8cf9d 100644 > > --- a/drivers/rtc/class.c > > +++ b/drivers/rtc/class.c > > @@ -34,6 +34,62 @@ static void rtc_device_release(struct device *dev) > > #ifdef CONFIG_RTC_HCTOSYS_DEVICE > > /* Result of the last RTC to system clock attempt. */ > > int rtc_hctosys_ret = -ENODEV; > > + > > +/* IMPORTANT: the RTC only stores whole seconds. It is arbitrary > > + * whether it stores the most close value or the value with partial > > + * seconds truncated. However, it is important that we use it to store > > + * the truncated value. This is because otherwise it is necessary, > > + * in an rtc sync function, to read both xtime.tv_sec and > > + * xtime.tv_nsec. On some processors (i.e. ARM), an atomic read > > + * of >32bits is not possible. So storing the most close value would > > + * slow down the sync API. So here we have the truncated value and > > + * the best guess is to add 0.5s. > > + */ > > + > > +static int rtc_hctosys(void) > > +{ > > + int err = -ENODEV; > > + struct rtc_time tm; > > + struct timespec64 tv64 = { > > + .tv_nsec = NSEC_PER_SEC >> 1, > > + }; > > + struct rtc_device *rtc = rtc_class_open(CONFIG_RTC_HCTOSYS_DEVICE); > > + > > + if (!rtc) { > > + pr_info("unable to open rtc device (%s)\n", > > + CONFIG_RTC_HCTOSYS_DEVICE); > > + goto err_open; > > + } > > + > > + err = rtc_read_time(rtc, &tm); > > + if (err) { > > + dev_err(rtc->dev.parent, > > + "hctosys: unable to read the hardware clock\n"); > > + goto err_read; > > + } > > + > > + tv64.tv_sec = rtc_tm_to_time64(&tm); > > + > > +#if BITS_PER_LONG == 32 > > + if (tv64.tv_sec > INT_MAX) { > > + err = -ERANGE; > > + goto err_read; > > + } > > +#endif > > + > > + err = do_settimeofday64(&tv64); > > + > > + dev_info(rtc->dev.parent, "setting system clock to %ptR UTC (%lld)\n", > > + &tm, (long long)tv64.tv_sec); > > + > > +err_read: > > + rtc_class_close(rtc); > > + > > +err_open: > > + rtc_hctosys_ret = err; > > + > > + return err; > > +} > > #endif > > > > #if defined(CONFIG_PM_SLEEP) && defined(CONFIG_RTC_HCTOSYS_DEVICE) > > @@ -375,6 +431,11 @@ int __rtc_register_device(struct module *owner, struct rtc_device *rtc) > > dev_info(rtc->dev.parent, "registered as %s\n", > > dev_name(&rtc->dev)); > > > > +#ifdef CONFIG_RTC_HCTOSYS_DEVICE > > + if (!strcmp(dev_name(&rtc->dev), CONFIG_RTC_HCTOSYS_DEVICE)) > > + rtc_hctosys(); > > +#endif > > + > > return 0; > > } > > EXPORT_SYMBOL_GPL(__rtc_register_device); > > diff --git a/drivers/rtc/hctosys.c b/drivers/rtc/hctosys.c > > deleted file mode 100644 > > index a74d0d890600..000000000000 > > --- a/drivers/rtc/hctosys.c > > +++ /dev/null > > @@ -1,69 +0,0 @@ > > -// SPDX-License-Identifier: GPL-2.0 > > -/* > > - * RTC subsystem, initialize system time on startup > > - * > > - * Copyright (C) 2005 Tower Technologies > > - * Author: Alessandro Zummo <a.zummo@towertech.it> > > - */ > > - > > -#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > > - > > -#include <linux/rtc.h> > > - > > -/* IMPORTANT: the RTC only stores whole seconds. It is arbitrary > > - * whether it stores the most close value or the value with partial > > - * seconds truncated. However, it is important that we use it to store > > - * the truncated value. This is because otherwise it is necessary, > > - * in an rtc sync function, to read both xtime.tv_sec and > > - * xtime.tv_nsec. On some processors (i.e. ARM), an atomic read > > - * of >32bits is not possible. So storing the most close value would > > - * slow down the sync API. So here we have the truncated value and > > - * the best guess is to add 0.5s. > > - */ > > - > > -static int __init rtc_hctosys(void) > > -{ > > - int err = -ENODEV; > > - struct rtc_time tm; > > - struct timespec64 tv64 = { > > - .tv_nsec = NSEC_PER_SEC >> 1, > > - }; > > - struct rtc_device *rtc = rtc_class_open(CONFIG_RTC_HCTOSYS_DEVICE); > > - > > - if (!rtc) { > > - pr_info("unable to open rtc device (%s)\n", > > - CONFIG_RTC_HCTOSYS_DEVICE); > > - goto err_open; > > - } > > - > > - err = rtc_read_time(rtc, &tm); > > - if (err) { > > - dev_err(rtc->dev.parent, > > - "hctosys: unable to read the hardware clock\n"); > > - goto err_read; > > - } > > - > > - tv64.tv_sec = rtc_tm_to_time64(&tm); > > - > > -#if BITS_PER_LONG == 32 > > - if (tv64.tv_sec > INT_MAX) { > > - err = -ERANGE; > > - goto err_read; > > - } > > -#endif > > - > > - err = do_settimeofday64(&tv64); > > - > > - dev_info(rtc->dev.parent, "setting system clock to %ptR UTC (%lld)\n", > > - &tm, (long long)tv64.tv_sec); > > - > > -err_read: > > - rtc_class_close(rtc); > > - > > -err_open: > > - rtc_hctosys_ret = err; > > - > > - return err; > > -} > > - > > -late_initcall(rtc_hctosys); > > -- > > 2.27.0 > > > > > > -- > > kernel-team mailing list > > kernel-team@lists.ubuntu.com > > https://lists.ubuntu.com/mailman/listinfo/kernel-team > > > > -- > Regards, > > Dimitri.
On 04/05/2021 13:22, Dimitri John Ledkov wrote: > Hi, > > This will introduce a regression in certain configurations as > mentioned on https://github.com/systemd/systemd/issues/17737 > > I had a tentative patch to kernel to only advance time in rtc_hctosys > only if it is newer than the one returned by ktime_get_real_ts64. That's the mainline commit, so change for it (e.g. do not set the system clock for modules) should go via upstream. > However that went nowhere. > > If this is cherry picked, imho we must change config > CONFIG_RTC_HCTOSYS or at least set it to something sane because > currently on raspi it is set to CONFIG_RTC_HCTOSYS_DEVICE="rtc0" which > always points at the non-battery backed RTC from the SOC. RPi does not have a RTC, so whoever adds it via overlay, probably chooses one with a battery. Otherwise there is no much point of having RTC... Best regards, Krzysztof
On Tue, May 4, 2021 at 6:39 PM Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> wrote: > > On 04/05/2021 13:22, Dimitri John Ledkov wrote: > > Hi, > > > > This will introduce a regression in certain configurations as > > mentioned on https://github.com/systemd/systemd/issues/17737 > > > > I had a tentative patch to kernel to only advance time in rtc_hctosys > > only if it is newer than the one returned by ktime_get_real_ts64. > > That's the mainline commit, so change for it (e.g. do not set the system > clock for modules) should go via upstream. > > > However that went nowhere. > > > > If this is cherry picked, imho we must change config > > CONFIG_RTC_HCTOSYS or at least set it to something sane because > > currently on raspi it is set to CONFIG_RTC_HCTOSYS_DEVICE="rtc0" which > > always points at the non-battery backed RTC from the SOC. > > RPi does not have a RTC, so whoever adds it via overlay, probably > chooses one with a battery. Otherwise there is no much point of having > RTC... That. I just don't know which RTC are available as rpi-hats and which dtb overlay they use and which rtc name they end up with. If the properly working & battery backed RTC is added via overlay dtb and it ends up having rtc0 name, everything is fine. I just don't know if the overlays remove all other rtc nodes and like take over the rtc0 name. @juerg do rpi RTCs with batteries end up having rtc0 kernel name? or do they end up with something else? Also we must include them into the initrd as modules for UC20 cause otherwise it would be odd if the clock jumps after pivot to root & after NTP got network time.
On 04/05/2021 13:42, Dimitri John Ledkov wrote: >>> >>> If this is cherry picked, imho we must change config >>> CONFIG_RTC_HCTOSYS or at least set it to something sane because >>> currently on raspi it is set to CONFIG_RTC_HCTOSYS_DEVICE="rtc0" which >>> always points at the non-battery backed RTC from the SOC. >> >> RPi does not have a RTC, so whoever adds it via overlay, probably >> chooses one with a battery. Otherwise there is no much point of having >> RTC... > > That. I just don't know which RTC are available as rpi-hats and which > dtb overlay they use and which rtc name they end up with. > > If the properly working & battery backed RTC is added via overlay dtb > and it ends up having rtc0 name, everything is fine. I just don't know > if the overlays remove all other rtc nodes and like take over the rtc0 > name. The first RTC registered will always have rtc0. Regardless of its properties (e.g. with or without battery). Best regards, Krzysztof
On Tue, 4 May 2021 18:42:41 +0100 Dimitri John Ledkov <dimitri.ledkov@canonical.com> wrote: > On Tue, May 4, 2021 at 6:39 PM Krzysztof Kozlowski > <krzysztof.kozlowski@canonical.com> wrote: > > > > On 04/05/2021 13:22, Dimitri John Ledkov wrote: > > > Hi, > > > > > > This will introduce a regression in certain configurations as > > > mentioned on https://github.com/systemd/systemd/issues/17737 > > > > > > I had a tentative patch to kernel to only advance time in rtc_hctosys > > > only if it is newer than the one returned by ktime_get_real_ts64. > > > > That's the mainline commit, so change for it (e.g. do not set the system > > clock for modules) should go via upstream. > > > > > However that went nowhere. > > > > > > If this is cherry picked, imho we must change config > > > CONFIG_RTC_HCTOSYS or at least set it to something sane because > > > currently on raspi it is set to CONFIG_RTC_HCTOSYS_DEVICE="rtc0" which > > > always points at the non-battery backed RTC from the SOC. > > > > RPi does not have a RTC, so whoever adds it via overlay, probably > > chooses one with a battery. Otherwise there is no much point of having > > RTC... > > That. I just don't know which RTC are available as rpi-hats and which > dtb overlay they use and which rtc name they end up with. The first to load will get rtc0 and that's what the kernel will use ('rtc0' is set as a CONFIG option). > If the properly working & battery backed RTC is added via overlay dtb > and it ends up having rtc0 name, everything is fine. I just don't know > if the overlays remove all other rtc nodes and like take over the rtc0 > name. You can have multiple RTCs. If someone chooses to do that (why?) then whichever is probed first ends up being rtc0. I don't think we need to handle the case of multiple RTCs and try to determine which one to use. > @juerg do rpi RTCs with batteries end up having rtc0 kernel name? The first to load. I don't believe the driver knows if an RTC is battery backed or not. > or > do they end up with something else? Also we must include them into the > initrd as modules for UC20 cause otherwise it would be odd if the > clock jumps after pivot to root & after NTP got network time. Yes. But the same is true for server, isn't it? Adding the module is a separate step and not under the kernels control. The question is if we should/need to include all RTCs currently supported by the Pi or just add them as needed. ....Juerg
Applied to focal:raspi/master-next. Thanks. From the ensuing discussion, I gather that there are no more RTC config changes required ? -rtg On 5/3/21 9:45 AM, Juerg Haefliger wrote: > From: Steve Muckle <smuckle@google.com> > > BugLink: https://bugs.launchpad.net/bugs/1926911 > > Due to distribution constraints it may not be possible to statically > compile the required RTC driver into the kernel. > > Expand RTC_HCTOSYS support to cover all RTC devices (statically compiled > or not) by checking at the end of RTC device registration whether the > time should be synced. > > Signed-off-by: Steve Muckle <smuckle@google.com> > Link: https://lore.kernel.org/r/20191106194625.116692-1-smuckle@google.com > Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com> > (cherry picked from commit f9b2a4d6a5f18e0aaf715206a056565c56889d9f) > Signed-off-by: Juerg Haefliger <juergh@canonical.com> > --- > drivers/rtc/Kconfig | 3 -- > drivers/rtc/Makefile | 1 - > drivers/rtc/class.c | 61 ++++++++++++++++++++++++++++++++++++++ > drivers/rtc/hctosys.c | 69 ------------------------------------------- > 4 files changed, 61 insertions(+), 73 deletions(-) > delete mode 100644 drivers/rtc/hctosys.c > > diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig > index 778cb0e63055..d9daa6ec45ff 100644 > --- a/drivers/rtc/Kconfig > +++ b/drivers/rtc/Kconfig > @@ -41,9 +41,6 @@ config RTC_HCTOSYS_DEVICE > device should record time in UTC, since the kernel won't do > timezone correction. > > - The driver for this RTC device must be loaded before late_initcall > - functions run, so it must usually be statically linked. > - > This clock should be battery-backed, so that it reads the correct > time when the system boots from a power-off state. Otherwise, your > system will need an external clock source (like an NTP server). > diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile > index a20943eafaab..9918b0429383 100644 > --- a/drivers/rtc/Makefile > +++ b/drivers/rtc/Makefile > @@ -6,7 +6,6 @@ > ccflags-$(CONFIG_RTC_DEBUG) := -DDEBUG > > obj-$(CONFIG_RTC_LIB) += lib.o > -obj-$(CONFIG_RTC_HCTOSYS) += hctosys.o > obj-$(CONFIG_RTC_SYSTOHC) += systohc.o > obj-$(CONFIG_RTC_CLASS) += rtc-core.o > obj-$(CONFIG_RTC_MC146818_LIB) += rtc-mc146818-lib.o > diff --git a/drivers/rtc/class.c b/drivers/rtc/class.c > index 9458e6d6686a..8793b2b8cf9d 100644 > --- a/drivers/rtc/class.c > +++ b/drivers/rtc/class.c > @@ -34,6 +34,62 @@ static void rtc_device_release(struct device *dev) > #ifdef CONFIG_RTC_HCTOSYS_DEVICE > /* Result of the last RTC to system clock attempt. */ > int rtc_hctosys_ret = -ENODEV; > + > +/* IMPORTANT: the RTC only stores whole seconds. It is arbitrary > + * whether it stores the most close value or the value with partial > + * seconds truncated. However, it is important that we use it to store > + * the truncated value. This is because otherwise it is necessary, > + * in an rtc sync function, to read both xtime.tv_sec and > + * xtime.tv_nsec. On some processors (i.e. ARM), an atomic read > + * of >32bits is not possible. So storing the most close value would > + * slow down the sync API. So here we have the truncated value and > + * the best guess is to add 0.5s. > + */ > + > +static int rtc_hctosys(void) > +{ > + int err = -ENODEV; > + struct rtc_time tm; > + struct timespec64 tv64 = { > + .tv_nsec = NSEC_PER_SEC >> 1, > + }; > + struct rtc_device *rtc = rtc_class_open(CONFIG_RTC_HCTOSYS_DEVICE); > + > + if (!rtc) { > + pr_info("unable to open rtc device (%s)\n", > + CONFIG_RTC_HCTOSYS_DEVICE); > + goto err_open; > + } > + > + err = rtc_read_time(rtc, &tm); > + if (err) { > + dev_err(rtc->dev.parent, > + "hctosys: unable to read the hardware clock\n"); > + goto err_read; > + } > + > + tv64.tv_sec = rtc_tm_to_time64(&tm); > + > +#if BITS_PER_LONG == 32 > + if (tv64.tv_sec > INT_MAX) { > + err = -ERANGE; > + goto err_read; > + } > +#endif > + > + err = do_settimeofday64(&tv64); > + > + dev_info(rtc->dev.parent, "setting system clock to %ptR UTC (%lld)\n", > + &tm, (long long)tv64.tv_sec); > + > +err_read: > + rtc_class_close(rtc); > + > +err_open: > + rtc_hctosys_ret = err; > + > + return err; > +} > #endif > > #if defined(CONFIG_PM_SLEEP) && defined(CONFIG_RTC_HCTOSYS_DEVICE) > @@ -375,6 +431,11 @@ int __rtc_register_device(struct module *owner, struct rtc_device *rtc) > dev_info(rtc->dev.parent, "registered as %s\n", > dev_name(&rtc->dev)); > > +#ifdef CONFIG_RTC_HCTOSYS_DEVICE > + if (!strcmp(dev_name(&rtc->dev), CONFIG_RTC_HCTOSYS_DEVICE)) > + rtc_hctosys(); > +#endif > + > return 0; > } > EXPORT_SYMBOL_GPL(__rtc_register_device); > diff --git a/drivers/rtc/hctosys.c b/drivers/rtc/hctosys.c > deleted file mode 100644 > index a74d0d890600..000000000000 > --- a/drivers/rtc/hctosys.c > +++ /dev/null > @@ -1,69 +0,0 @@ > -// SPDX-License-Identifier: GPL-2.0 > -/* > - * RTC subsystem, initialize system time on startup > - * > - * Copyright (C) 2005 Tower Technologies > - * Author: Alessandro Zummo <a.zummo@towertech.it> > - */ > - > -#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > - > -#include <linux/rtc.h> > - > -/* IMPORTANT: the RTC only stores whole seconds. It is arbitrary > - * whether it stores the most close value or the value with partial > - * seconds truncated. However, it is important that we use it to store > - * the truncated value. This is because otherwise it is necessary, > - * in an rtc sync function, to read both xtime.tv_sec and > - * xtime.tv_nsec. On some processors (i.e. ARM), an atomic read > - * of >32bits is not possible. So storing the most close value would > - * slow down the sync API. So here we have the truncated value and > - * the best guess is to add 0.5s. > - */ > - > -static int __init rtc_hctosys(void) > -{ > - int err = -ENODEV; > - struct rtc_time tm; > - struct timespec64 tv64 = { > - .tv_nsec = NSEC_PER_SEC >> 1, > - }; > - struct rtc_device *rtc = rtc_class_open(CONFIG_RTC_HCTOSYS_DEVICE); > - > - if (!rtc) { > - pr_info("unable to open rtc device (%s)\n", > - CONFIG_RTC_HCTOSYS_DEVICE); > - goto err_open; > - } > - > - err = rtc_read_time(rtc, &tm); > - if (err) { > - dev_err(rtc->dev.parent, > - "hctosys: unable to read the hardware clock\n"); > - goto err_read; > - } > - > - tv64.tv_sec = rtc_tm_to_time64(&tm); > - > -#if BITS_PER_LONG == 32 > - if (tv64.tv_sec > INT_MAX) { > - err = -ERANGE; > - goto err_read; > - } > -#endif > - > - err = do_settimeofday64(&tv64); > - > - dev_info(rtc->dev.parent, "setting system clock to %ptR UTC (%lld)\n", > - &tm, (long long)tv64.tv_sec); > - > -err_read: > - rtc_class_close(rtc); > - > -err_open: > - rtc_hctosys_ret = err; > - > - return err; > -} > - > -late_initcall(rtc_hctosys); >
diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig index 778cb0e63055..d9daa6ec45ff 100644 --- a/drivers/rtc/Kconfig +++ b/drivers/rtc/Kconfig @@ -41,9 +41,6 @@ config RTC_HCTOSYS_DEVICE device should record time in UTC, since the kernel won't do timezone correction. - The driver for this RTC device must be loaded before late_initcall - functions run, so it must usually be statically linked. - This clock should be battery-backed, so that it reads the correct time when the system boots from a power-off state. Otherwise, your system will need an external clock source (like an NTP server). diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile index a20943eafaab..9918b0429383 100644 --- a/drivers/rtc/Makefile +++ b/drivers/rtc/Makefile @@ -6,7 +6,6 @@ ccflags-$(CONFIG_RTC_DEBUG) := -DDEBUG obj-$(CONFIG_RTC_LIB) += lib.o -obj-$(CONFIG_RTC_HCTOSYS) += hctosys.o obj-$(CONFIG_RTC_SYSTOHC) += systohc.o obj-$(CONFIG_RTC_CLASS) += rtc-core.o obj-$(CONFIG_RTC_MC146818_LIB) += rtc-mc146818-lib.o diff --git a/drivers/rtc/class.c b/drivers/rtc/class.c index 9458e6d6686a..8793b2b8cf9d 100644 --- a/drivers/rtc/class.c +++ b/drivers/rtc/class.c @@ -34,6 +34,62 @@ static void rtc_device_release(struct device *dev) #ifdef CONFIG_RTC_HCTOSYS_DEVICE /* Result of the last RTC to system clock attempt. */ int rtc_hctosys_ret = -ENODEV; + +/* IMPORTANT: the RTC only stores whole seconds. It is arbitrary + * whether it stores the most close value or the value with partial + * seconds truncated. However, it is important that we use it to store + * the truncated value. This is because otherwise it is necessary, + * in an rtc sync function, to read both xtime.tv_sec and + * xtime.tv_nsec. On some processors (i.e. ARM), an atomic read + * of >32bits is not possible. So storing the most close value would + * slow down the sync API. So here we have the truncated value and + * the best guess is to add 0.5s. + */ + +static int rtc_hctosys(void) +{ + int err = -ENODEV; + struct rtc_time tm; + struct timespec64 tv64 = { + .tv_nsec = NSEC_PER_SEC >> 1, + }; + struct rtc_device *rtc = rtc_class_open(CONFIG_RTC_HCTOSYS_DEVICE); + + if (!rtc) { + pr_info("unable to open rtc device (%s)\n", + CONFIG_RTC_HCTOSYS_DEVICE); + goto err_open; + } + + err = rtc_read_time(rtc, &tm); + if (err) { + dev_err(rtc->dev.parent, + "hctosys: unable to read the hardware clock\n"); + goto err_read; + } + + tv64.tv_sec = rtc_tm_to_time64(&tm); + +#if BITS_PER_LONG == 32 + if (tv64.tv_sec > INT_MAX) { + err = -ERANGE; + goto err_read; + } +#endif + + err = do_settimeofday64(&tv64); + + dev_info(rtc->dev.parent, "setting system clock to %ptR UTC (%lld)\n", + &tm, (long long)tv64.tv_sec); + +err_read: + rtc_class_close(rtc); + +err_open: + rtc_hctosys_ret = err; + + return err; +} #endif #if defined(CONFIG_PM_SLEEP) && defined(CONFIG_RTC_HCTOSYS_DEVICE) @@ -375,6 +431,11 @@ int __rtc_register_device(struct module *owner, struct rtc_device *rtc) dev_info(rtc->dev.parent, "registered as %s\n", dev_name(&rtc->dev)); +#ifdef CONFIG_RTC_HCTOSYS_DEVICE + if (!strcmp(dev_name(&rtc->dev), CONFIG_RTC_HCTOSYS_DEVICE)) + rtc_hctosys(); +#endif + return 0; } EXPORT_SYMBOL_GPL(__rtc_register_device); diff --git a/drivers/rtc/hctosys.c b/drivers/rtc/hctosys.c deleted file mode 100644 index a74d0d890600..000000000000 --- a/drivers/rtc/hctosys.c +++ /dev/null @@ -1,69 +0,0 @@ -// SPDX-License-Identifier: GPL-2.0 -/* - * RTC subsystem, initialize system time on startup - * - * Copyright (C) 2005 Tower Technologies - * Author: Alessandro Zummo <a.zummo@towertech.it> - */ - -#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt - -#include <linux/rtc.h> - -/* IMPORTANT: the RTC only stores whole seconds. It is arbitrary - * whether it stores the most close value or the value with partial - * seconds truncated. However, it is important that we use it to store - * the truncated value. This is because otherwise it is necessary, - * in an rtc sync function, to read both xtime.tv_sec and - * xtime.tv_nsec. On some processors (i.e. ARM), an atomic read - * of >32bits is not possible. So storing the most close value would - * slow down the sync API. So here we have the truncated value and - * the best guess is to add 0.5s. - */ - -static int __init rtc_hctosys(void) -{ - int err = -ENODEV; - struct rtc_time tm; - struct timespec64 tv64 = { - .tv_nsec = NSEC_PER_SEC >> 1, - }; - struct rtc_device *rtc = rtc_class_open(CONFIG_RTC_HCTOSYS_DEVICE); - - if (!rtc) { - pr_info("unable to open rtc device (%s)\n", - CONFIG_RTC_HCTOSYS_DEVICE); - goto err_open; - } - - err = rtc_read_time(rtc, &tm); - if (err) { - dev_err(rtc->dev.parent, - "hctosys: unable to read the hardware clock\n"); - goto err_read; - } - - tv64.tv_sec = rtc_tm_to_time64(&tm); - -#if BITS_PER_LONG == 32 - if (tv64.tv_sec > INT_MAX) { - err = -ERANGE; - goto err_read; - } -#endif - - err = do_settimeofday64(&tv64); - - dev_info(rtc->dev.parent, "setting system clock to %ptR UTC (%lld)\n", - &tm, (long long)tv64.tv_sec); - -err_read: - rtc_class_close(rtc); - -err_open: - rtc_hctosys_ret = err; - - return err; -} - -late_initcall(rtc_hctosys);