Linux-RTC Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] rtc: class: support hctosys from modular RTC drivers
@ 2019-11-06 19:46 Steve Muckle
  2019-11-06 23:19 ` Alexandre Belloni
  0 siblings, 1 reply; 6+ messages in thread
From: Steve Muckle @ 2019-11-06 19:46 UTC (permalink / raw)
  To: Alessandro Zummo, Alexandre Belloni
  Cc: linux-kernel, linux-rtc, kernel-team, Steve Muckle

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

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] rtc: class: support hctosys from modular RTC drivers
  2019-11-06 19:46 [PATCH] rtc: class: support hctosys from modular RTC drivers Steve Muckle
@ 2019-11-06 23:19 ` Alexandre Belloni
  2019-11-06 23:37   ` Steve Muckle
  0 siblings, 1 reply; 6+ messages in thread
From: Alexandre Belloni @ 2019-11-06 23:19 UTC (permalink / raw)
  To: Steve Muckle; +Cc: Alessandro Zummo, linux-kernel, linux-rtc, kernel-team

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
> 

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] rtc: class: support hctosys from modular RTC drivers
  2019-11-06 23:19 ` Alexandre Belloni
@ 2019-11-06 23:37   ` Steve Muckle
  2019-11-15 13:36     ` Alexandre Belloni
  0 siblings, 1 reply; 6+ messages in thread
From: Steve Muckle @ 2019-11-06 23:37 UTC (permalink / raw)
  To: Alexandre Belloni; +Cc: Alessandro Zummo, linux-kernel, linux-rtc, kernel-team

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.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] rtc: class: support hctosys from modular RTC drivers
  2019-11-06 23:37   ` Steve Muckle
@ 2019-11-15 13:36     ` Alexandre Belloni
  2019-12-27 20:26       ` Steve Muckle
  0 siblings, 1 reply; 6+ messages in thread
From: Alexandre Belloni @ 2019-11-15 13:36 UTC (permalink / raw)
  To: Steve Muckle; +Cc: Alessandro Zummo, linux-kernel, linux-rtc, kernel-team

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.


-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] rtc: class: support hctosys from modular RTC drivers
  2019-11-15 13:36     ` Alexandre Belloni
@ 2019-12-27 20:26       ` Steve Muckle
  2020-01-31 18:35         ` Steve Muckle
  0 siblings, 1 reply; 6+ messages in thread
From: Steve Muckle @ 2019-12-27 20:26 UTC (permalink / raw)
  To: Alexandre Belloni; +Cc: Alessandro Zummo, linux-kernel, linux-rtc, kernel-team

(accidentally sent as HTML, resending in text)

On 11/15/19 5:36 AM, Alexandre Belloni wrote:
> 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?

This should ideally happen before file systems are mounted so I don't 
see many alternatives for communicating which RTC should be used. 
Android uses the kernel command line for userspace parameters as well 
and that's an option but that defeats part of the value of doing it in 
userspace IMO. There's also device tree but I'm not sure this belongs there.

Hctosys is also saving and restoring the system time on suspend/resume. 
It seems more efficient to me to do this (which happens very frequently 
on an Android device) in the kernel as opposed to in userspace.

If I set the initial system time from the rtc in userspace but continue 
to rely on the hctosys suspend/resume code, as it stands there will be a 
window after the rtc driver is loaded but before the system time is set 
where if suspend is entered, the correct time in the rtc will be lost.
>>> 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.
No argument with that, but millions of devices successfully rely on it 
today. AFAICT this simple patch doesn't make anything worse. Together 
with a change to support a kernel parameter for runtime rtc selection, 
it should allow RTC drivers to be modularized on many systems. Can it be 
adopted as a stopgap measure?

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] rtc: class: support hctosys from modular RTC drivers
  2019-12-27 20:26       ` Steve Muckle
@ 2020-01-31 18:35         ` Steve Muckle
  0 siblings, 0 replies; 6+ messages in thread
From: Steve Muckle @ 2020-01-31 18:35 UTC (permalink / raw)
  To: Alexandre Belloni; +Cc: Alessandro Zummo, linux-kernel, linux-rtc, kernel-team

On 12/27/19 12:26 PM, Steve Muckle wrote:
> On 11/15/19 5:36 AM, Alexandre Belloni wrote:
>> 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?
> 
> This should ideally happen before file systems are mounted so I don't
> see many alternatives for communicating which RTC should be used.
> Android uses the kernel command line for userspace parameters as well
> and that's an option but that defeats part of the value of doing it in
> userspace IMO. There's also device tree but I'm not sure this belongs there.
> 
> Hctosys is also saving and restoring the system time on suspend/resume.
> It seems more efficient to me to do this (which happens very frequently
> on an Android device) in the kernel as opposed to in userspace.
> 
> If I set the initial system time from the rtc in userspace but continue
> to rely on the hctosys suspend/resume code, as it stands there will be a
> window after the rtc driver is loaded but before the system time is set
> where if suspend is entered, the correct time in the rtc will be lost.
 >
>>>> 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.
 >
> No argument with that, but millions of devices successfully rely on it
> today. AFAICT this simple patch doesn't make anything worse. Together
> with a change to support a kernel parameter for runtime rtc selection,
> it should allow RTC drivers to be modularized on many systems. Can it be
> adopted as a stopgap measure?

Hi Alexandre, ping...

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-06 19:46 [PATCH] rtc: class: support hctosys from modular RTC drivers Steve Muckle
2019-11-06 23:19 ` Alexandre Belloni
2019-11-06 23:37   ` Steve Muckle
2019-11-15 13:36     ` Alexandre Belloni
2019-12-27 20:26       ` Steve Muckle
2020-01-31 18:35         ` Steve Muckle

Linux-RTC Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-rtc/0 linux-rtc/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-rtc linux-rtc/ https://lore.kernel.org/linux-rtc \
		linux-rtc@vger.kernel.org
	public-inbox-index linux-rtc

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-rtc


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git