rtc: class: support hctosys from modular RTC drivers
diff mbox series

Message ID 20191106194625.116692-1-smuckle@google.com
State New
Headers show
Series
  • rtc: class: support hctosys from modular RTC drivers
Related show

Commit Message

Steve Muckle Nov. 6, 2019, 7:46 p.m. UTC
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>
---
 drivers/rtc/Kconfig   | 18 ++++++-----
 drivers/rtc/Makefile  |  1 -
 drivers/rtc/class.c   | 63 +++++++++++++++++++++++++++++++++++++++
 drivers/rtc/hctosys.c | 69 -------------------------------------------
 4 files changed, 73 insertions(+), 78 deletions(-)
 delete mode 100644 drivers/rtc/hctosys.c

Comments

Alexandre Belloni Nov. 6, 2019, 11:19 p.m. UTC | #1
Hi,

On 06/11/2019 11:46:25-0800, Steve Muckle wrote:
> 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.
> 

This does not really help distributions because most of them will still
have "rtc0" hardcoded and rtc0 is often the rtc that shouldn't be used.

Can't you move away from HCTOSYS and do the correct thing in userspace
instead of the crap hctosys is doing?

> Signed-off-by: Steve Muckle <smuckle@google.com>
> ---
>  drivers/rtc/Kconfig   | 18 ++++++-----
>  drivers/rtc/Makefile  |  1 -
>  drivers/rtc/class.c   | 63 +++++++++++++++++++++++++++++++++++++++
>  drivers/rtc/hctosys.c | 69 -------------------------------------------
>  4 files changed, 73 insertions(+), 78 deletions(-)
>  delete mode 100644 drivers/rtc/hctosys.c
> 
> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index 1adf9f815652..f663d77deb41 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -35,14 +35,16 @@ config RTC_HCTOSYS_DEVICE
>  	depends on RTC_HCTOSYS
>  	default "rtc0"
>  	help
> -	  The RTC device that will be used to (re)initialize the system
> -	  clock, usually rtc0. Initialization is done when the system
> -	  starts up, and when it resumes from a low power state. This
> -	  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.
> +	  The RTC device that will be used to (re)initialize the system clock,
> +	  usually rtc0. Initialization is done when the driver for the RTC
> +	  device registers, and when it resumes from a low power state. This
> +	  device should record time in UTC, since the kernel won't do timezone
> +	  correction.
> +
> +	  During startup it is useful to initialize the system clock from the
> +	  RTC as early as possible. For this reason the driver for this RTC
> +	  device should be statically linked, or alternately, the kernel module
> +	  for the RTC device driver loaded immediately.
>  
>  	  This clock should be battery-backed, so that it reads the correct
>  	  time when the system boots from a power-off state. Otherwise, your
> diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
> index 4ac8f19fb631..33f932d08262 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..b1ed5f3be223 100644
> --- a/drivers/rtc/class.c
> +++ b/drivers/rtc/class.c
> @@ -34,6 +34,64 @@ 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 +433,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.24.0.rc1.363.gb1bccd3e3d-goog
>
Steve Muckle Nov. 6, 2019, 11:37 p.m. UTC | #2
On 11/6/19 3:19 PM, Alexandre Belloni wrote:
> On 06/11/2019 11:46:25-0800, Steve Muckle wrote:
>> 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.
>>
> 
> This does not really help distributions because most of them will still
> have "rtc0" hardcoded and rtc0 is often the rtc that shouldn't be used.

Just for my own edification, why is that? Is rtc0 normally useless on PC 
for some reason?

On the platforms I'm working with I believe it can be assured that rtc0 
will be the correct rtc. That doesn't help typical distributions though.

What about a kernel parameter to optionally override the rtc hctosys 
device at runtime?

> Can't you move away from HCTOSYS and do the correct thing in userspace
> instead of the crap hctosys is doing?

Yes, I just figured it's a small change, and if hctosys can be made to 
work might as well use that.
Alexandre Belloni Nov. 15, 2019, 1:36 p.m. UTC | #3
On 06/11/2019 15:37:49-0800, Steve Muckle wrote:
> On 11/6/19 3:19 PM, Alexandre Belloni wrote:
> > On 06/11/2019 11:46:25-0800, Steve Muckle wrote:
> > > 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.
> > > 
> > 
> > This does not really help distributions because most of them will still
> > have "rtc0" hardcoded and rtc0 is often the rtc that shouldn't be used.
> 
> Just for my own edification, why is that? Is rtc0 normally useless on PC for
> some reason?
> 

On PC, rtc0 is probably fine which is not the case for other
architectures where rtc0 is the SoC RTC and is often not battery backed.

> On the platforms I'm working with I believe it can be assured that rtc0 will
> be the correct rtc. That doesn't help typical distributions though.
> 
> What about a kernel parameter to optionally override the rtc hctosys device
> at runtime?
> 

What about keeping that in userspace instead which is way easier than
messing with kernel parameters?

> > Can't you move away from HCTOSYS and do the correct thing in userspace
> > instead of the crap hctosys is doing?
> 
> Yes, I just figured it's a small change, and if hctosys can be made to work
> might as well use that.

The fact is that hctosys is more related to time keeping than it is to
the RTC subsytem. It also does a very poor job setting the system time
because adding 0.5s is not the smartest thing to do. The rtc granularity
is indeed 1 second but is can be very precisely set.

Patch
diff mbox series

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 1adf9f815652..f663d77deb41 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -35,14 +35,16 @@  config RTC_HCTOSYS_DEVICE
 	depends on RTC_HCTOSYS
 	default "rtc0"
 	help
-	  The RTC device that will be used to (re)initialize the system
-	  clock, usually rtc0. Initialization is done when the system
-	  starts up, and when it resumes from a low power state. This
-	  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.
+	  The RTC device that will be used to (re)initialize the system clock,
+	  usually rtc0. Initialization is done when the driver for the RTC
+	  device registers, and when it resumes from a low power state. This
+	  device should record time in UTC, since the kernel won't do timezone
+	  correction.
+
+	  During startup it is useful to initialize the system clock from the
+	  RTC as early as possible. For this reason the driver for this RTC
+	  device should be statically linked, or alternately, the kernel module
+	  for the RTC device driver loaded immediately.
 
 	  This clock should be battery-backed, so that it reads the correct
 	  time when the system boots from a power-off state. Otherwise, your
diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
index 4ac8f19fb631..33f932d08262 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..b1ed5f3be223 100644
--- a/drivers/rtc/class.c
+++ b/drivers/rtc/class.c
@@ -34,6 +34,64 @@  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 +433,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);