[v1,1/2] x86/rtc: Add option to skip using RTC
diff mbox series

Message ID becacc523508b295a52db9f1592e2868e3988e28.1566458029.git.rahul.tanwar@linux.intel.com
State New
Headers show
Series
  • x86/rtc: Add option to skip using RTC
Related show

Commit Message

Tanwar, Rahul Aug. 22, 2019, 7:44 a.m. UTC
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(+)

Comments

Andy Shevchenko Aug. 22, 2019, 9:02 a.m. UTC | #1
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
>
Tanwar, Rahul Aug. 22, 2019, 9:26 a.m. UTC | #2
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
Andy Shevchenko Aug. 22, 2019, 1:04 p.m. UTC | #3
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.
Tanwar, Rahul Aug. 23, 2019, 3:37 a.m. UTC | #4
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

>
>
Andy Shevchenko Aug. 23, 2019, 12:56 p.m. UTC | #5
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;
}
Tanwar, Rahul Aug. 26, 2019, 9:01 a.m. UTC | #6
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
diff mbox series

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.