[PATCH v1 0/2] x86/rtc: Add option to skip using RTC

[PATCH v1 0/2] x86/rtc: Add option to skip using RTC

[Rahul Tanwar]

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(+)

-- 
2.11.0

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

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

Suggested-by: Alan Cox <alan@linux.intel.com>
Signed-off-by: Rahul Tanwar <rahul.tanwar@linux.intel.com>
---
 arch/x86/kernel/rtc.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/arch/x86/kernel/rtc.c b/arch/x86/kernel/rtc.c
index 586f718b8e95..f9f760a8e76a 100644
--- a/arch/x86/kernel/rtc.c
+++ b/arch/x86/kernel/rtc.c
@@ -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.
-- 
2.11.0

Re: [PATCH v1 1/2] x86/rtc: Add option to skip using RTC

[Andy Shevchenko]

On Thu, Aug 22, 2019 at 03:44:03PM +0800, Rahul Tanwar wrote:
> 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?

> Suggested-by: Alan Cox <alan@linux.intel.com>
> Signed-off-by: Rahul Tanwar <rahul.tanwar@linux.intel.com>
> ---
>  arch/x86/kernel/rtc.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/arch/x86/kernel/rtc.c b/arch/x86/kernel/rtc.c
> index 586f718b8e95..f9f760a8e76a 100644
> --- a/arch/x86/kernel/rtc.c
> +++ b/arch/x86/kernel/rtc.c
> @@ -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.
> -- 
> 2.11.0
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/2] x86/rtc: Add option to skip using RTC

[Tanwar, Rahul]

Hi Andy,

On 22/8/2019 5:02 PM, Andy Shevchenko wrote:
> On Thu, Aug 22, 2019 at 03:44:03PM +0800, Rahul Tanwar wrote:
>> 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

Re: [PATCH v1 1/2] x86/rtc: Add option to skip using RTC

[Andy Shevchenko]

On Thu, Aug 22, 2019 at 05:26:33PM +0800, Tanwar, Rahul wrote:
> On 22/8/2019 5:02 PM, Andy Shevchenko wrote:
> > On Thu, Aug 22, 2019 at 03:44:03PM +0800, Rahul Tanwar wrote:
> > > 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.

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


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/2] x86/rtc: Add option to skip using RTC

[Tanwar, Rahul]

Hi Andy,

On 22/8/2019 9:04 PM, Andy Shevchenko wrote:
> On Thu, Aug 22, 2019 at 05:26:33PM +0800, Tanwar, Rahul wrote:
>> On 22/8/2019 5:02 PM, Andy Shevchenko wrote:
>>> On Thu, Aug 22, 2019 at 03:44:03PM +0800, Rahul Tanwar wrote:
>>>> 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.
> 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

>
>

Re: [PATCH v1 1/2] x86/rtc: Add option to skip using RTC

[Andy Shevchenko]

On Fri, Aug 23, 2019 at 11:37:38AM +0800, Tanwar, Rahul wrote:
> On 22/8/2019 9:04 PM, Andy Shevchenko wrote:
> > On Thu, Aug 22, 2019 at 05:26:33PM +0800, Tanwar, Rahul wrote:
> > > On 22/8/2019 5:02 PM, Andy Shevchenko wrote:
> > > > On Thu, Aug 22, 2019 at 03:44:03PM +0800, Rahul Tanwar wrote:
> > > > > 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.
> > 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

void __init foo()
{
	if (platform has RTC)
		return;

	set_wallclock = noop;
	get_wallclock = noop;
}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/2] x86/rtc: Add option to skip using RTC

[Tanwar, Rahul]

Hi Andy,

On 23/8/2019 8:56 PM, Andy Shevchenko wrote:
>>>> 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.
>> 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;
> }

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

and send out for review again.

Regards,

Rahul

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

[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 <rahul.tanwar@linux.intel.com>
---
 Documentation/devicetree/bindings/rtc/rtc-cmos.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/rtc/rtc-cmos.txt b/Documentation/devicetree/bindings/rtc/rtc-cmos.txt
index b94b35f3600b..fea4fe0ed4b9 100644
--- a/Documentation/devicetree/bindings/rtc/rtc-cmos.txt
+++ b/Documentation/devicetree/bindings/rtc/rtc-cmos.txt
@@ -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";
 	 };
-- 
2.11.0

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

[Andy Shevchenko]

On Thu, Aug 22, 2019 at 03:44:04PM +0800, Rahul Tanwar wrote:
> 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

-- 
With Best Regards,
Andy Shevchenko

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

[Alexandre Belloni]

On 22/08/2019 11:56:59+0300, Andy Shevchenko wrote:
> On Thu, Aug 22, 2019 at 03:44:04PM +0800, Rahul Tanwar wrote:
> > 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.

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

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

[Tanwar, Rahul]

On 22/8/2019 5:09 PM, Alexandre Belloni wrote:
> On 22/08/2019 11:56:59+0300, Andy Shevchenko wrote:
>> On Thu, Aug 22, 2019 at 03:44:04PM +0800, Rahul Tanwar wrote:
>>> 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