x86/rtc: Add option to skip using RTC

PATCH Superseded Rahul Tanwar 3 participants 12 messages Aug 22, 2019 – Aug 26, 2019

Subsystems (3)

Device tree bindings x86 Real time clock (RTC)

lore

Share & tools

lore

Rahul Tanwar

author

2019-08-22 07:44 UTC #0 lore raw

cover x86/rtc: Add option to skip using RTC

Platform init ops get_wallclock()/set_wallclock() are by default setup such that they use MC146818A RTC/CMOS device to set & read time. However, some products might not support RTC. One such example is Intel Atom based Lightning Mountain network processor SOC.

Patch 1: Use status standard property of motorola,mc146818 compatible DT node to determine if RTC is supported. Skip read/write from RTC device only when this node is present and status is disabled.

For non DT machines, do not change anything, proceed same as before.

For DT machines, if this node is not present, proceed same as before.

For DT machines, if this node is present but newly introduced status property is not defined in DTS, proceed same as before.

For DT machines, if this node is present and status property indicates "disabled", then skip read/write from RTC/CMOS device. If status property indicates "okay", proceed same as before.

Patch 2: Update dt bindings document to add status property.

These patches are baselined upon Linux 5.3-rc5 at below Git tree:
git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git x86/core

Rahul Tanwar (2):
x86/rtc: Add option to skip using RTC
dt-bindings: rtc: Add optional status property

 Documentation/devicetree/bindings/rtc/rtc-cmos.txt |  4 ++++
 arch/x86/kernel/rtc.c                              | 17 +++++++++++++++++
 2 files changed, 21 insertions(+)

Signature · 1 line

2.11.0

Rahul Tanwar

author

2019-08-22 07:44 UTC #1 lore raw

PATCH 1/2 x86/rtc: Add option to skip using RTC

Use a newly introduced optional "status" property of "motorola,mc146818" compatible DT node to determine if RTC is supported. Skip read/write from RTC device only when this node is present and status is "disabled". In all other cases, proceed as before.

Suggested-by Alan Cox <[email protected]>

Signed-off-by Rahul Tanwar <[email protected]>

---
 arch/x86/kernel/rtc.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

arch/x86/kernel/rtc.c +17 -0

index 586f718b8e95..f9f760a8e76a 100644
@@ -32,6 +32,11 @@ EXPORT_SYMBOL(cmos_lock);
 DEFINE_SPINLOCK(rtc_lock);
 EXPORT_SYMBOL(rtc_lock);
 
+static const struct of_device_id of_cmos_match[] = {
+        { .compatible = "motorola,mc146818" },
+        {}
+};
+
 /*
  * In order to set the CMOS clock precisely, set_rtc_mmss has to be
  * called 500 ms after the second nowtime has started, because when

@@ -42,9 +47,14 @@ EXPORT_SYMBOL(rtc_lock);
 int mach_set_rtc_mmss(const struct timespec64 *now)
 {
 	unsigned long long nowtime = now->tv_sec;
+	struct device_node *node;
 	struct rtc_time tm;
 	int retval = 0;
 
+	node = of_find_matching_node(NULL, of_cmos_match);
+	if (node && !of_device_is_available(node))
+		return -EINVAL;
+
 	rtc_time64_to_tm(nowtime, &tm);
 	if (!rtc_valid_tm(&tm)) {
 		retval = mc146818_set_time(&tm);

@@ -63,8 +73,15 @@ int mach_set_rtc_mmss(const struct timespec64 *now)
 void mach_get_cmos_time(struct timespec64 *now)
 {
 	unsigned int status, year, mon, day, hour, min, sec, century = 0;
+	struct device_node *node;
 	unsigned long flags;
 
+	node = of_find_matching_node(NULL, of_cmos_match);
+	if (node && !of_device_is_available(node)) {
+		now->tv_sec = now->tv_nsec = 0;
+		return;
+	}
+
 	/*
 	 * If pm_trace abused the RTC as storage, set the timespec to 0,
 	 * which tells the caller that this RTC value is unusable.

Signature · 1 line

2.11.0

Andy Shevchenko

2019-08-22 09:02 UTC #2 lore raw

Quoted 4 lines · Rahul Tanwar

Use a newly introduced optional "status" property of "motorola,mc146818" compatible DT node to determine if RTC is supported. Skip read/write from RTC device only when this node is present and status is "disabled". In all other cases, proceed as before.

Can't we rather update ->get_wallclock() and ->set_wallclock() based on this?

Quoted 56 lines

Suggested-by Alan Cox <[email protected]>

Signed-off-by Rahul Tanwar <[email protected]>

---
 arch/x86/kernel/rtc.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

arch/x86/kernel/rtc.c +17 -0

index 586f718b8e95..f9f760a8e76a 100644
@@ -32,6 +32,11 @@ EXPORT_SYMBOL(cmos_lock);
 DEFINE_SPINLOCK(rtc_lock);
 EXPORT_SYMBOL(rtc_lock);
 
+static const struct of_device_id of_cmos_match[] = {
+        { .compatible = "motorola,mc146818" },
+        {}
+};
+
 /*
  * In order to set the CMOS clock precisely, set_rtc_mmss has to be
  * called 500 ms after the second nowtime has started, because when

@@ -42,9 +47,14 @@ EXPORT_SYMBOL(rtc_lock);
 int mach_set_rtc_mmss(const struct timespec64 *now)
 {
 	unsigned long long nowtime = now->tv_sec;
+	struct device_node *node;
 	struct rtc_time tm;
 	int retval = 0;
 
+	node = of_find_matching_node(NULL, of_cmos_match);
+	if (node && !of_device_is_available(node))
+		return -EINVAL;
+
 	rtc_time64_to_tm(nowtime, &tm);
 	if (!rtc_valid_tm(&tm)) {
 		retval = mc146818_set_time(&tm);

@@ -63,8 +73,15 @@ int mach_set_rtc_mmss(const struct timespec64 *now)
 void mach_get_cmos_time(struct timespec64 *now)
 {
 	unsigned int status, year, mon, day, hour, min, sec, century = 0;
+	struct device_node *node;
 	unsigned long flags;
 
+	node = of_find_matching_node(NULL, of_cmos_match);
+	if (node && !of_device_is_available(node)) {
+		now->tv_sec = now->tv_nsec = 0;
+		return;
+	}
+
 	/*
 	 * If pm_trace abused the RTC as storage, set the timespec to 0,
 	 * which tells the caller that this RTC value is unusable.

Signature · 1 line

2.11.0

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Tanwar, Rahul

author

2019-08-22 09:26 UTC #3 lore raw

Hi Andy,

Quoted 6 lines · Andy Shevchenko

Quoted 4 lines · Rahul Tanwar
Use a newly introduced optional "status" property of "motorola,mc146818" compatible DT node to determine if RTC is supported. Skip read/write from RTC device only when this node is present and status is "disabled". In all other cases, proceed as before.
Can't we rather update ->get_wallclock() and ->set_wallclock() based on this?

get_wallclock() and set_wallclock() are function pointers of platform_ops

which are initialized to mach_get_cmos_time() and mach_set_rtc_mmss()

at init time. Since adding a new platform to override these functions is

discouraged, so the only way is to modify RTC get/set functions.

Regards,

Rahul

Andy Shevchenko

2019-08-22 13:04 UTC #4 lore raw

Quoted 9 lines · Rahul

Quoted 6 lines · Andy Shevchenko
Quoted 4 lines · Rahul Tanwar
Use a newly introduced optional "status" property of "motorola,mc146818" compatible DT node to determine if RTC is supported. Skip read/write from RTC device only when this node is present and status is "disabled". In all other cases, proceed as before.
Can't we rather update ->get_wallclock() and ->set_wallclock() based on this?

Quoted 7 lines

get_wallclock() and set_wallclock() are function pointers of platform_ops
which are initialized to mach_get_cmos_time() and mach_set_rtc_mmss()
at init time. Since adding a new platform to override these functions is
discouraged, so the only way is to modify RTC get/set functions.

Shouldn't it be platform agnostic code?
So, my point is, instead of hacking two functions, perhaps better to avoid them at all.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Tanwar, Rahul

author

2019-08-23 03:37 UTC #5 lore raw

Hi Andy,

Quoted 11 lines · Andy Shevchenko

Quoted 10 lines · Rahul
Quoted 6 lines · Andy Shevchenko
Quoted 4 lines · Rahul Tanwar
Use a newly introduced optional "status" property of "motorola,mc146818" compatible DT node to determine if RTC is supported. Skip read/write from RTC device only when this node is present and status is "disabled". In all other cases, proceed as before.
Can't we rather update ->get_wallclock() and ->set_wallclock() based on this?
get_wallclock() and set_wallclock() are function pointers of platform_ops

Quoted 8 lines

Quoted 5 lines
which are initialized to mach_get_cmos_time() and mach_set_rtc_mmss()
at init time. Since adding a new platform to override these functions is
discouraged, so the only way is to modify RTC get/set functions.
Shouldn't it be platform agnostic code?
So, my point is, instead of hacking two functions, perhaps better to avoid them at all.

Sorry, i could not understand your point. The changes are platform

agnostic i.e. it doesn't break existing use cases. Are you recommending

to add a new platform and make changes there ?

Regards,

Rahul

Quoted 2 lines

Andy Shevchenko

2019-08-23 12:56 UTC #6 lore raw

Quoted 21 lines · Rahul

Quoted 11 lines · Andy Shevchenko
Quoted 10 lines · Rahul
Quoted 6 lines · Andy Shevchenko
Quoted 4 lines · Rahul Tanwar
Use a newly introduced optional "status" property of "motorola,mc146818" compatible DT node to determine if RTC is supported. Skip read/write from RTC device only when this node is present and status is "disabled". In all other cases, proceed as before.
Can't we rather update ->get_wallclock() and ->set_wallclock() based on this?
get_wallclock() and set_wallclock() are function pointers of platform_ops
Quoted 8 lines
Quoted 5 lines
which are initialized to mach_get_cmos_time() and mach_set_rtc_mmss()
at init time. Since adding a new platform to override these functions is
discouraged, so the only way is to modify RTC get/set functions.
Shouldn't it be platform agnostic code?
So, my point is, instead of hacking two functions, perhaps better to avoid them at all.

Quoted 5 lines

Sorry, i could not understand your point. The changes are platform
agnostic i.e. it doesn't break existing use cases. Are you recommending
to add a new platform and make changes there ?

Nope, I propose to do something like

void __init foo()

{
	if (platform has RTC)
		return;

	set_wallclock = noop;
	get_wallclock = noop;
}

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Tanwar, Rahul

author

2019-08-26 09:01 UTC #7 lore raw

Hi Andy,

Quoted 17 lines · Andy Shevchenko

Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
get_wallclock() and set_wallclock() are function pointers of platform_ops
which are initialized to mach_get_cmos_time() and mach_set_rtc_mmss()
at init time. Since adding a new platform to override these functions is
discouraged, so the only way is to modify RTC get/set functions.
Quoted 8 lines
Quoted 3 lines
Shouldn't it be platform agnostic code?
So, my point is, instead of hacking two functions, perhaps better to avoid them at all.
Sorry, i could not understand your point. The changes are platform
agnostic i.e. it doesn't break existing use cases. Are you recommending
to add a new platform and make changes there ?
Nope, I propose to do something like

Quoted 8 lines

void __init foo()

{
	if (platform has RTC)
		return;

	set_wallclock = noop;
	get_wallclock = noop;
}

Thanks. I will work out a V2 patch as per your suggestion

and send out for review again.

Regards,

Rahul

Rahul Tanwar

author

2019-08-22 07:44 UTC #8 lore raw

PATCH 2/2 dt-bindings: rtc: Add optional status property

In reply to Rahul Tanwar

Some products may not support MC146818 RTC CMOS device. Introduce a optional 'status' standard property for RTC-CMOS to indicate if the MC146818 RTC device is available (status="okay") or not (status="disabled")

Signed-off-by Rahul Tanwar <[email protected]>

---
 Documentation/devicetree/bindings/rtc/rtc-cmos.txt | 4 ++++
 1 file changed, 4 insertions(+)

Documentation/devicetree/bindings/rtc/rtc-cmos.txt +4 -0

index b94b35f3600b..fea4fe0ed4b9 100644
@@ -11,6 +11,9 @@ Optional properties:
     called "Register B".
   - freq-reg : Contains the initial value of the frequency register also
     called "Regsiter A".
+  - status: indicates the operational status of the device.
+    Value must be either "disabled" or "okay".
+
 
 "Register A" and "B" are usually initialized by the firmware (BIOS for
 instance). If this is not done, it can be performed by the driver.

@@ -24,4 +27,5 @@ ISA Example:
 	         ctrl-reg = <2>;
 	         freq-reg = <0x26>;
 	         reg = <1 0x70 2>;
+		 status = "okay";
 	 };

Signature · 1 line

2.11.0

Andy Shevchenko

2019-08-22 08:56 UTC #9 lore raw

Re: [PATCH v1 2/2] dt-bindings: rtc: Add optional status property

Quoted 3 lines · Rahul Tanwar

Some products may not support MC146818 RTC CMOS device. Introduce a optional 'status' standard property for RTC-CMOS to indicate if the MC146818 RTC device is available (status="okay") or not (status="disabled")

This needs to be converted to YAML

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Alexandre Belloni

maintainer

2019-08-22 09:09 UTC #10 lore raw

Re: [PATCH v1 2/2] dt-bindings: rtc: Add optional status property

Quoted 7 lines · Andy Shevchenko

Quoted 3 lines · Rahul Tanwar
Some products may not support MC146818 RTC CMOS device. Introduce a optional 'status' standard property for RTC-CMOS to indicate if the MC146818 RTC device is available (status="okay") or not (status="disabled")
This needs to be converted to YAML

Well, I think the status property doesn't even need to be documented because it is simply the generic behaviour.

Signature · 3 lines

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

Tanwar, Rahul

author

2019-08-22 09:38 UTC #11 lore raw

Re: [PATCH v1 2/2] dt-bindings: rtc: Add optional status property

Quoted 9 lines · Alexandre Belloni

Quoted 6 lines · Andy Shevchenko
Quoted 3 lines · Rahul Tanwar
Some products may not support MC146818 RTC CMOS device. Introduce a optional 'status' standard property for RTC-CMOS to indicate if the MC146818 RTC device is available (status="okay") or not (status="disabled")
This needs to be converted to YAML
Well, I think the status property doesn't even need to be documented because it is simply the generic behaviour.

Thanks for your comments. I now realize it. I will omit posting this again.

Regards,

Rahul