[PATCH] rtc: class: support hctosys from modular RTC drivers

[PATCH] rtc: class: support hctosys from modular RTC drivers

[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

Re: [PATCH] rtc: class: support hctosys from modular RTC drivers

[Alexandre Belloni]

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

Re: [PATCH] rtc: class: support hctosys from modular RTC drivers

[Steve Muckle]

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.

Re: [PATCH] rtc: class: support hctosys from modular RTC drivers

[Alexandre Belloni]

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

Re: [PATCH] rtc: class: support hctosys from modular RTC drivers

[Steve Muckle]

(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?

Re: [PATCH] rtc: class: support hctosys from modular RTC drivers

[Steve Muckle]

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...

Re: [PATCH] rtc: class: support hctosys from modular RTC drivers

[Alexandre Belloni]

On 27/12/2019 12:23:23-0800, Steve Muckle wrote:
> On Fri, Nov 15, 2019 at 5:36 AM Alexandre Belloni <
> alexandre.belloni@bootlin.com> 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?

I've applied this patch for 5.7 after removing the unnecessary
reformatting of the kconfig help.