Linux-RTC Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2 -next] powerpc: kernel/time.c - cleanup warnings
@ 2021-03-23  9:12 He Ying
  2021-03-23 22:18 ` Alexandre Belloni
  0 siblings, 1 reply; 9+ messages in thread
From: He Ying @ 2021-03-23  9:12 UTC (permalink / raw)
  To: mpe, benh, paulus, a.zummo, alexandre.belloni, christophe.leroy,
	npiggin, msuchanek, heying24, tglx, peterz, geert+renesas,
	kernelfans, frederic
  Cc: linuxppc-dev, linux-kernel, linux-rtc

We found these warnings in arch/powerpc/kernel/time.c as follows:
warning: symbol 'decrementer_max' was not declared. Should it be static?
warning: symbol 'rtc_lock' was not declared. Should it be static?
warning: symbol 'dtl_consumer' was not declared. Should it be static?

Declare 'decrementer_max' and 'rtc_lock' in powerpc asm/time.h.
Rename 'rtc_lock' in drviers/rtc/rtc-vr41xx.c to 'vr41xx_rtc_lock' to
avoid the conflict with the variable in powerpc asm/time.h.
Move 'dtl_consumer' definition behind "include <asm/dtl.h>" because it
is declared there.

Reported-by: Hulk Robot <hulkci@huawei.com>
Signed-off-by: He Ying <heying24@huawei.com>
---
v2:
- Instead of including linux/mc146818rtc.h in powerpc kernel/time.c, declare
  rtc_lock in powerpc asm/time.h.

 arch/powerpc/include/asm/time.h |  3 +++
 arch/powerpc/kernel/time.c      |  6 ++----
 drivers/rtc/rtc-vr41xx.c        | 22 +++++++++++-----------
 3 files changed, 16 insertions(+), 15 deletions(-)

diff --git a/arch/powerpc/include/asm/time.h b/arch/powerpc/include/asm/time.h
index 8dd3cdb25338..64a3ef0b4270 100644
--- a/arch/powerpc/include/asm/time.h
+++ b/arch/powerpc/include/asm/time.h
@@ -12,6 +12,7 @@
 #ifdef __KERNEL__
 #include <linux/types.h>
 #include <linux/percpu.h>
+#include <linux/spinlock.h>
 
 #include <asm/processor.h>
 #include <asm/cpu_has_feature.h>
@@ -22,6 +23,8 @@ extern unsigned long tb_ticks_per_jiffy;
 extern unsigned long tb_ticks_per_usec;
 extern unsigned long tb_ticks_per_sec;
 extern struct clock_event_device decrementer_clockevent;
+extern u64 decrementer_max;
+extern spinlock_t rtc_lock;
 
 
 extern void generic_calibrate_decr(void);
diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c
index b67d93a609a2..60b6ac7d3685 100644
--- a/arch/powerpc/kernel/time.c
+++ b/arch/powerpc/kernel/time.c
@@ -150,10 +150,6 @@ bool tb_invalid;
 u64 __cputime_usec_factor;
 EXPORT_SYMBOL(__cputime_usec_factor);
 
-#ifdef CONFIG_PPC_SPLPAR
-void (*dtl_consumer)(struct dtl_entry *, u64);
-#endif
-
 static void calc_cputime_factors(void)
 {
 	struct div_result res;
@@ -179,6 +175,8 @@ static inline unsigned long read_spurr(unsigned long tb)
 
 #include <asm/dtl.h>
 
+void (*dtl_consumer)(struct dtl_entry *, u64);
+
 /*
  * Scan the dispatch trace log and count up the stolen time.
  * Should be called with interrupts disabled.
diff --git a/drivers/rtc/rtc-vr41xx.c b/drivers/rtc/rtc-vr41xx.c
index 5a9f9ad86d32..cc31db058197 100644
--- a/drivers/rtc/rtc-vr41xx.c
+++ b/drivers/rtc/rtc-vr41xx.c
@@ -72,7 +72,7 @@ static void __iomem *rtc2_base;
 
 static unsigned long epoch = 1970;	/* Jan 1 1970 00:00:00 */
 
-static DEFINE_SPINLOCK(rtc_lock);
+static DEFINE_SPINLOCK(vr41xx_rtc_lock);
 static char rtc_name[] = "RTC";
 static unsigned long periodic_count;
 static unsigned int alarm_enabled;
@@ -101,13 +101,13 @@ static inline time64_t read_elapsed_second(void)
 
 static inline void write_elapsed_second(time64_t sec)
 {
-	spin_lock_irq(&rtc_lock);
+	spin_lock_irq(&vr41xx_rtc_lock);
 
 	rtc1_write(ETIMELREG, (uint16_t)(sec << 15));
 	rtc1_write(ETIMEMREG, (uint16_t)(sec >> 1));
 	rtc1_write(ETIMEHREG, (uint16_t)(sec >> 17));
 
-	spin_unlock_irq(&rtc_lock);
+	spin_unlock_irq(&vr41xx_rtc_lock);
 }
 
 static int vr41xx_rtc_read_time(struct device *dev, struct rtc_time *time)
@@ -139,14 +139,14 @@ static int vr41xx_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *wkalrm)
 	unsigned long low, mid, high;
 	struct rtc_time *time = &wkalrm->time;
 
-	spin_lock_irq(&rtc_lock);
+	spin_lock_irq(&vr41xx_rtc_lock);
 
 	low = rtc1_read(ECMPLREG);
 	mid = rtc1_read(ECMPMREG);
 	high = rtc1_read(ECMPHREG);
 	wkalrm->enabled = alarm_enabled;
 
-	spin_unlock_irq(&rtc_lock);
+	spin_unlock_irq(&vr41xx_rtc_lock);
 
 	rtc_time64_to_tm((high << 17) | (mid << 1) | (low >> 15), time);
 
@@ -159,7 +159,7 @@ static int vr41xx_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *wkalrm)
 
 	alarm_sec = rtc_tm_to_time64(&wkalrm->time);
 
-	spin_lock_irq(&rtc_lock);
+	spin_lock_irq(&vr41xx_rtc_lock);
 
 	if (alarm_enabled)
 		disable_irq(aie_irq);
@@ -173,7 +173,7 @@ static int vr41xx_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *wkalrm)
 
 	alarm_enabled = wkalrm->enabled;
 
-	spin_unlock_irq(&rtc_lock);
+	spin_unlock_irq(&vr41xx_rtc_lock);
 
 	return 0;
 }
@@ -202,7 +202,7 @@ static int vr41xx_rtc_ioctl(struct device *dev, unsigned int cmd, unsigned long
 
 static int vr41xx_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled)
 {
-	spin_lock_irq(&rtc_lock);
+	spin_lock_irq(&vr41xx_rtc_lock);
 	if (enabled) {
 		if (!alarm_enabled) {
 			enable_irq(aie_irq);
@@ -214,7 +214,7 @@ static int vr41xx_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled)
 			alarm_enabled = 0;
 		}
 	}
-	spin_unlock_irq(&rtc_lock);
+	spin_unlock_irq(&vr41xx_rtc_lock);
 	return 0;
 }
 
@@ -296,7 +296,7 @@ static int rtc_probe(struct platform_device *pdev)
 	rtc->range_max = (1ULL << 33) - 1;
 	rtc->max_user_freq = MAX_PERIODIC_RATE;
 
-	spin_lock_irq(&rtc_lock);
+	spin_lock_irq(&vr41xx_rtc_lock);
 
 	rtc1_write(ECMPLREG, 0);
 	rtc1_write(ECMPMREG, 0);
@@ -304,7 +304,7 @@ static int rtc_probe(struct platform_device *pdev)
 	rtc1_write(RTCL1LREG, 0);
 	rtc1_write(RTCL1HREG, 0);
 
-	spin_unlock_irq(&rtc_lock);
+	spin_unlock_irq(&vr41xx_rtc_lock);
 
 	aie_irq = platform_get_irq(pdev, 0);
 	if (aie_irq <= 0) {
-- 
2.17.1


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

* Re: [PATCH v2 -next] powerpc: kernel/time.c - cleanup warnings
  2021-03-23  9:12 [PATCH v2 -next] powerpc: kernel/time.c - cleanup warnings He Ying
@ 2021-03-23 22:18 ` Alexandre Belloni
  2021-03-23 23:05   ` Alexandre Belloni
                     ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Alexandre Belloni @ 2021-03-23 22:18 UTC (permalink / raw)
  To: He Ying
  Cc: mpe, benh, paulus, a.zummo, christophe.leroy, npiggin, msuchanek,
	tglx, peterz, geert+renesas, kernelfans, frederic, linuxppc-dev,
	linux-kernel, linux-rtc

Hello,

On 23/03/2021 05:12:57-0400, He Ying wrote:
> We found these warnings in arch/powerpc/kernel/time.c as follows:
> warning: symbol 'decrementer_max' was not declared. Should it be static?
> warning: symbol 'rtc_lock' was not declared. Should it be static?
> warning: symbol 'dtl_consumer' was not declared. Should it be static?
> 
> Declare 'decrementer_max' and 'rtc_lock' in powerpc asm/time.h.
> Rename 'rtc_lock' in drviers/rtc/rtc-vr41xx.c to 'vr41xx_rtc_lock' to
> avoid the conflict with the variable in powerpc asm/time.h.
> Move 'dtl_consumer' definition behind "include <asm/dtl.h>" because it
> is declared there.
> 
> Reported-by: Hulk Robot <hulkci@huawei.com>
> Signed-off-by: He Ying <heying24@huawei.com>
> ---
> v2:
> - Instead of including linux/mc146818rtc.h in powerpc kernel/time.c, declare
>   rtc_lock in powerpc asm/time.h.
> 

V1 was actually the correct thing to do. rtc_lock is there exactly
because chrp and maple are using mc146818 compatible RTCs. This is then
useful because then drivers/char/nvram.c is enabled. The proper fix
would be to scrap all of that and use rtc-cmos for those platforms as
this drives the RTC properly and exposes the NVRAM for the mc146818.

Or at least, if there are no users for the char/nvram driver on those
two platforms, remove the spinlock and stop enabling CONFIG_NVRAM or
more likely rename the symbol as it seems to be abused by both chrp and
powermac.

I'm not completely against the rename in vr41xxx but the fix for the
warnings can and should be contained in arch/powerpc.

>  arch/powerpc/include/asm/time.h |  3 +++
>  arch/powerpc/kernel/time.c      |  6 ++----
>  drivers/rtc/rtc-vr41xx.c        | 22 +++++++++++-----------
>  3 files changed, 16 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/time.h b/arch/powerpc/include/asm/time.h
> index 8dd3cdb25338..64a3ef0b4270 100644
> --- a/arch/powerpc/include/asm/time.h
> +++ b/arch/powerpc/include/asm/time.h
> @@ -12,6 +12,7 @@
>  #ifdef __KERNEL__
>  #include <linux/types.h>
>  #include <linux/percpu.h>
> +#include <linux/spinlock.h>
>  
>  #include <asm/processor.h>
>  #include <asm/cpu_has_feature.h>
> @@ -22,6 +23,8 @@ extern unsigned long tb_ticks_per_jiffy;
>  extern unsigned long tb_ticks_per_usec;
>  extern unsigned long tb_ticks_per_sec;
>  extern struct clock_event_device decrementer_clockevent;
> +extern u64 decrementer_max;
> +extern spinlock_t rtc_lock;
>  
>  
>  extern void generic_calibrate_decr(void);
> diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c
> index b67d93a609a2..60b6ac7d3685 100644
> --- a/arch/powerpc/kernel/time.c
> +++ b/arch/powerpc/kernel/time.c
> @@ -150,10 +150,6 @@ bool tb_invalid;
>  u64 __cputime_usec_factor;
>  EXPORT_SYMBOL(__cputime_usec_factor);
>  
> -#ifdef CONFIG_PPC_SPLPAR
> -void (*dtl_consumer)(struct dtl_entry *, u64);
> -#endif
> -
>  static void calc_cputime_factors(void)
>  {
>  	struct div_result res;
> @@ -179,6 +175,8 @@ static inline unsigned long read_spurr(unsigned long tb)
>  
>  #include <asm/dtl.h>
>  
> +void (*dtl_consumer)(struct dtl_entry *, u64);
> +
>  /*
>   * Scan the dispatch trace log and count up the stolen time.
>   * Should be called with interrupts disabled.
> diff --git a/drivers/rtc/rtc-vr41xx.c b/drivers/rtc/rtc-vr41xx.c
> index 5a9f9ad86d32..cc31db058197 100644
> --- a/drivers/rtc/rtc-vr41xx.c
> +++ b/drivers/rtc/rtc-vr41xx.c
> @@ -72,7 +72,7 @@ static void __iomem *rtc2_base;
>  
>  static unsigned long epoch = 1970;	/* Jan 1 1970 00:00:00 */
>  
> -static DEFINE_SPINLOCK(rtc_lock);
> +static DEFINE_SPINLOCK(vr41xx_rtc_lock);
>  static char rtc_name[] = "RTC";
>  static unsigned long periodic_count;
>  static unsigned int alarm_enabled;
> @@ -101,13 +101,13 @@ static inline time64_t read_elapsed_second(void)
>  
>  static inline void write_elapsed_second(time64_t sec)
>  {
> -	spin_lock_irq(&rtc_lock);
> +	spin_lock_irq(&vr41xx_rtc_lock);
>  
>  	rtc1_write(ETIMELREG, (uint16_t)(sec << 15));
>  	rtc1_write(ETIMEMREG, (uint16_t)(sec >> 1));
>  	rtc1_write(ETIMEHREG, (uint16_t)(sec >> 17));
>  
> -	spin_unlock_irq(&rtc_lock);
> +	spin_unlock_irq(&vr41xx_rtc_lock);
>  }
>  
>  static int vr41xx_rtc_read_time(struct device *dev, struct rtc_time *time)
> @@ -139,14 +139,14 @@ static int vr41xx_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *wkalrm)
>  	unsigned long low, mid, high;
>  	struct rtc_time *time = &wkalrm->time;
>  
> -	spin_lock_irq(&rtc_lock);
> +	spin_lock_irq(&vr41xx_rtc_lock);
>  
>  	low = rtc1_read(ECMPLREG);
>  	mid = rtc1_read(ECMPMREG);
>  	high = rtc1_read(ECMPHREG);
>  	wkalrm->enabled = alarm_enabled;
>  
> -	spin_unlock_irq(&rtc_lock);
> +	spin_unlock_irq(&vr41xx_rtc_lock);
>  
>  	rtc_time64_to_tm((high << 17) | (mid << 1) | (low >> 15), time);
>  
> @@ -159,7 +159,7 @@ static int vr41xx_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *wkalrm)
>  
>  	alarm_sec = rtc_tm_to_time64(&wkalrm->time);
>  
> -	spin_lock_irq(&rtc_lock);
> +	spin_lock_irq(&vr41xx_rtc_lock);
>  
>  	if (alarm_enabled)
>  		disable_irq(aie_irq);
> @@ -173,7 +173,7 @@ static int vr41xx_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *wkalrm)
>  
>  	alarm_enabled = wkalrm->enabled;
>  
> -	spin_unlock_irq(&rtc_lock);
> +	spin_unlock_irq(&vr41xx_rtc_lock);
>  
>  	return 0;
>  }
> @@ -202,7 +202,7 @@ static int vr41xx_rtc_ioctl(struct device *dev, unsigned int cmd, unsigned long
>  
>  static int vr41xx_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled)
>  {
> -	spin_lock_irq(&rtc_lock);
> +	spin_lock_irq(&vr41xx_rtc_lock);
>  	if (enabled) {
>  		if (!alarm_enabled) {
>  			enable_irq(aie_irq);
> @@ -214,7 +214,7 @@ static int vr41xx_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled)
>  			alarm_enabled = 0;
>  		}
>  	}
> -	spin_unlock_irq(&rtc_lock);
> +	spin_unlock_irq(&vr41xx_rtc_lock);
>  	return 0;
>  }
>  
> @@ -296,7 +296,7 @@ static int rtc_probe(struct platform_device *pdev)
>  	rtc->range_max = (1ULL << 33) - 1;
>  	rtc->max_user_freq = MAX_PERIODIC_RATE;
>  
> -	spin_lock_irq(&rtc_lock);
> +	spin_lock_irq(&vr41xx_rtc_lock);
>  
>  	rtc1_write(ECMPLREG, 0);
>  	rtc1_write(ECMPMREG, 0);
> @@ -304,7 +304,7 @@ static int rtc_probe(struct platform_device *pdev)
>  	rtc1_write(RTCL1LREG, 0);
>  	rtc1_write(RTCL1HREG, 0);
>  
> -	spin_unlock_irq(&rtc_lock);
> +	spin_unlock_irq(&vr41xx_rtc_lock);
>  
>  	aie_irq = platform_get_irq(pdev, 0);
>  	if (aie_irq <= 0) {
> -- 
> 2.17.1
> 

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

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

* Re: [PATCH v2 -next] powerpc: kernel/time.c - cleanup warnings
  2021-03-23 22:18 ` Alexandre Belloni
@ 2021-03-23 23:05   ` Alexandre Belloni
  2021-03-24  6:14     ` Christophe Leroy
  2021-03-24  1:58   ` heying (H)
  2021-03-24  8:19   ` Geert Uytterhoeven
  2 siblings, 1 reply; 9+ messages in thread
From: Alexandre Belloni @ 2021-03-23 23:05 UTC (permalink / raw)
  To: He Ying
  Cc: mpe, benh, paulus, a.zummo, christophe.leroy, npiggin, msuchanek,
	tglx, peterz, geert+renesas, kernelfans, frederic, linuxppc-dev,
	linux-kernel, linux-rtc

On 23/03/2021 23:18:17+0100, Alexandre Belloni wrote:
> Hello,
> 
> On 23/03/2021 05:12:57-0400, He Ying wrote:
> > We found these warnings in arch/powerpc/kernel/time.c as follows:
> > warning: symbol 'decrementer_max' was not declared. Should it be static?
> > warning: symbol 'rtc_lock' was not declared. Should it be static?
> > warning: symbol 'dtl_consumer' was not declared. Should it be static?
> > 
> > Declare 'decrementer_max' and 'rtc_lock' in powerpc asm/time.h.
> > Rename 'rtc_lock' in drviers/rtc/rtc-vr41xx.c to 'vr41xx_rtc_lock' to
> > avoid the conflict with the variable in powerpc asm/time.h.
> > Move 'dtl_consumer' definition behind "include <asm/dtl.h>" because it
> > is declared there.
> > 
> > Reported-by: Hulk Robot <hulkci@huawei.com>
> > Signed-off-by: He Ying <heying24@huawei.com>
> > ---
> > v2:
> > - Instead of including linux/mc146818rtc.h in powerpc kernel/time.c, declare
> >   rtc_lock in powerpc asm/time.h.
> > 
> 
> V1 was actually the correct thing to do. rtc_lock is there exactly
> because chrp and maple are using mc146818 compatible RTCs. This is then
> useful because then drivers/char/nvram.c is enabled. The proper fix
> would be to scrap all of that and use rtc-cmos for those platforms as
> this drives the RTC properly and exposes the NVRAM for the mc146818.
> 
> Or at least, if there are no users for the char/nvram driver on those
> two platforms, remove the spinlock and stop enabling CONFIG_NVRAM or
> more likely rename the symbol as it seems to be abused by both chrp and
> powermac.
> 

Ok so rtc_lock is not even used by the char/nvram.c driver as it is
completely compiled out.

I guess it is fine having it move to the individual platform as looking
very quickly at the Kconfig, it is not possible to select both
simultaneously. Tentative patch:

8<-----
From dfa59b6f44fdfdefafffa7666aec89e62bbd5c80 Mon Sep 17 00:00:00 2001
From: Alexandre Belloni <alexandre.belloni@bootlin.com>
Date: Wed, 24 Mar 2021 00:00:03 +0100
Subject: [PATCH] powerpc: move rtc_lock to specific platforms

Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
---
 arch/powerpc/kernel/time.c          | 3 ---
 arch/powerpc/platforms/chrp/time.c  | 2 +-
 arch/powerpc/platforms/maple/time.c | 2 ++
 3 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c
index 67feb3524460..d3bb189ea7f4 100644
--- a/arch/powerpc/kernel/time.c
+++ b/arch/powerpc/kernel/time.c
@@ -123,9 +123,6 @@ EXPORT_SYMBOL(tb_ticks_per_usec);
 unsigned long tb_ticks_per_sec;
 EXPORT_SYMBOL(tb_ticks_per_sec);	/* for cputime_t conversions */
 
-DEFINE_SPINLOCK(rtc_lock);
-EXPORT_SYMBOL_GPL(rtc_lock);
-
 static u64 tb_to_ns_scale __read_mostly;
 static unsigned tb_to_ns_shift __read_mostly;
 static u64 boot_tb __read_mostly;
diff --git a/arch/powerpc/platforms/chrp/time.c b/arch/powerpc/platforms/chrp/time.c
index acde7bbe0716..ea90c15f5edd 100644
--- a/arch/powerpc/platforms/chrp/time.c
+++ b/arch/powerpc/platforms/chrp/time.c
@@ -30,7 +30,7 @@
 
 #include <platforms/chrp/chrp.h>
 
-extern spinlock_t rtc_lock;
+DEFINE_SPINLOCK(rtc_lock);
 
 #define NVRAM_AS0  0x74
 #define NVRAM_AS1  0x75
diff --git a/arch/powerpc/platforms/maple/time.c b/arch/powerpc/platforms/maple/time.c
index 78209bb7629c..ddda02010d86 100644
--- a/arch/powerpc/platforms/maple/time.c
+++ b/arch/powerpc/platforms/maple/time.c
@@ -34,6 +34,8 @@
 #define DBG(x...)
 #endif
 
+DEFINE_SPINLOCK(rtc_lock);
+
 static int maple_rtc_addr;
 
 static int maple_clock_read(int addr)
-- 
2.25.1


> I'm not completely against the rename in vr41xxx but the fix for the
> warnings can and should be contained in arch/powerpc.
> 
> >  arch/powerpc/include/asm/time.h |  3 +++
> >  arch/powerpc/kernel/time.c      |  6 ++----
> >  drivers/rtc/rtc-vr41xx.c        | 22 +++++++++++-----------
> >  3 files changed, 16 insertions(+), 15 deletions(-)
> > 
> > diff --git a/arch/powerpc/include/asm/time.h b/arch/powerpc/include/asm/time.h
> > index 8dd3cdb25338..64a3ef0b4270 100644
> > --- a/arch/powerpc/include/asm/time.h
> > +++ b/arch/powerpc/include/asm/time.h
> > @@ -12,6 +12,7 @@
> >  #ifdef __KERNEL__
> >  #include <linux/types.h>
> >  #include <linux/percpu.h>
> > +#include <linux/spinlock.h>
> >  
> >  #include <asm/processor.h>
> >  #include <asm/cpu_has_feature.h>
> > @@ -22,6 +23,8 @@ extern unsigned long tb_ticks_per_jiffy;
> >  extern unsigned long tb_ticks_per_usec;
> >  extern unsigned long tb_ticks_per_sec;
> >  extern struct clock_event_device decrementer_clockevent;
> > +extern u64 decrementer_max;
> > +extern spinlock_t rtc_lock;
> >  
> >  
> >  extern void generic_calibrate_decr(void);
> > diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c
> > index b67d93a609a2..60b6ac7d3685 100644
> > --- a/arch/powerpc/kernel/time.c
> > +++ b/arch/powerpc/kernel/time.c
> > @@ -150,10 +150,6 @@ bool tb_invalid;
> >  u64 __cputime_usec_factor;
> >  EXPORT_SYMBOL(__cputime_usec_factor);
> >  
> > -#ifdef CONFIG_PPC_SPLPAR
> > -void (*dtl_consumer)(struct dtl_entry *, u64);
> > -#endif
> > -
> >  static void calc_cputime_factors(void)
> >  {
> >  	struct div_result res;
> > @@ -179,6 +175,8 @@ static inline unsigned long read_spurr(unsigned long tb)
> >  
> >  #include <asm/dtl.h>
> >  
> > +void (*dtl_consumer)(struct dtl_entry *, u64);
> > +
> >  /*
> >   * Scan the dispatch trace log and count up the stolen time.
> >   * Should be called with interrupts disabled.
> > diff --git a/drivers/rtc/rtc-vr41xx.c b/drivers/rtc/rtc-vr41xx.c
> > index 5a9f9ad86d32..cc31db058197 100644
> > --- a/drivers/rtc/rtc-vr41xx.c
> > +++ b/drivers/rtc/rtc-vr41xx.c
> > @@ -72,7 +72,7 @@ static void __iomem *rtc2_base;
> >  
> >  static unsigned long epoch = 1970;	/* Jan 1 1970 00:00:00 */
> >  
> > -static DEFINE_SPINLOCK(rtc_lock);
> > +static DEFINE_SPINLOCK(vr41xx_rtc_lock);
> >  static char rtc_name[] = "RTC";
> >  static unsigned long periodic_count;
> >  static unsigned int alarm_enabled;
> > @@ -101,13 +101,13 @@ static inline time64_t read_elapsed_second(void)
> >  
> >  static inline void write_elapsed_second(time64_t sec)
> >  {
> > -	spin_lock_irq(&rtc_lock);
> > +	spin_lock_irq(&vr41xx_rtc_lock);
> >  
> >  	rtc1_write(ETIMELREG, (uint16_t)(sec << 15));
> >  	rtc1_write(ETIMEMREG, (uint16_t)(sec >> 1));
> >  	rtc1_write(ETIMEHREG, (uint16_t)(sec >> 17));
> >  
> > -	spin_unlock_irq(&rtc_lock);
> > +	spin_unlock_irq(&vr41xx_rtc_lock);
> >  }
> >  
> >  static int vr41xx_rtc_read_time(struct device *dev, struct rtc_time *time)
> > @@ -139,14 +139,14 @@ static int vr41xx_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *wkalrm)
> >  	unsigned long low, mid, high;
> >  	struct rtc_time *time = &wkalrm->time;
> >  
> > -	spin_lock_irq(&rtc_lock);
> > +	spin_lock_irq(&vr41xx_rtc_lock);
> >  
> >  	low = rtc1_read(ECMPLREG);
> >  	mid = rtc1_read(ECMPMREG);
> >  	high = rtc1_read(ECMPHREG);
> >  	wkalrm->enabled = alarm_enabled;
> >  
> > -	spin_unlock_irq(&rtc_lock);
> > +	spin_unlock_irq(&vr41xx_rtc_lock);
> >  
> >  	rtc_time64_to_tm((high << 17) | (mid << 1) | (low >> 15), time);
> >  
> > @@ -159,7 +159,7 @@ static int vr41xx_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *wkalrm)
> >  
> >  	alarm_sec = rtc_tm_to_time64(&wkalrm->time);
> >  
> > -	spin_lock_irq(&rtc_lock);
> > +	spin_lock_irq(&vr41xx_rtc_lock);
> >  
> >  	if (alarm_enabled)
> >  		disable_irq(aie_irq);
> > @@ -173,7 +173,7 @@ static int vr41xx_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *wkalrm)
> >  
> >  	alarm_enabled = wkalrm->enabled;
> >  
> > -	spin_unlock_irq(&rtc_lock);
> > +	spin_unlock_irq(&vr41xx_rtc_lock);
> >  
> >  	return 0;
> >  }
> > @@ -202,7 +202,7 @@ static int vr41xx_rtc_ioctl(struct device *dev, unsigned int cmd, unsigned long
> >  
> >  static int vr41xx_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled)
> >  {
> > -	spin_lock_irq(&rtc_lock);
> > +	spin_lock_irq(&vr41xx_rtc_lock);
> >  	if (enabled) {
> >  		if (!alarm_enabled) {
> >  			enable_irq(aie_irq);
> > @@ -214,7 +214,7 @@ static int vr41xx_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled)
> >  			alarm_enabled = 0;
> >  		}
> >  	}
> > -	spin_unlock_irq(&rtc_lock);
> > +	spin_unlock_irq(&vr41xx_rtc_lock);
> >  	return 0;
> >  }
> >  
> > @@ -296,7 +296,7 @@ static int rtc_probe(struct platform_device *pdev)
> >  	rtc->range_max = (1ULL << 33) - 1;
> >  	rtc->max_user_freq = MAX_PERIODIC_RATE;
> >  
> > -	spin_lock_irq(&rtc_lock);
> > +	spin_lock_irq(&vr41xx_rtc_lock);
> >  
> >  	rtc1_write(ECMPLREG, 0);
> >  	rtc1_write(ECMPMREG, 0);
> > @@ -304,7 +304,7 @@ static int rtc_probe(struct platform_device *pdev)
> >  	rtc1_write(RTCL1LREG, 0);
> >  	rtc1_write(RTCL1HREG, 0);
> >  
> > -	spin_unlock_irq(&rtc_lock);
> > +	spin_unlock_irq(&vr41xx_rtc_lock);
> >  
> >  	aie_irq = platform_get_irq(pdev, 0);
> >  	if (aie_irq <= 0) {
> > -- 
> > 2.17.1
> > 
> 
> -- 
> Alexandre Belloni, co-owner and COO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

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

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

* Re: [PATCH v2 -next] powerpc: kernel/time.c - cleanup warnings
  2021-03-23 22:18 ` Alexandre Belloni
  2021-03-23 23:05   ` Alexandre Belloni
@ 2021-03-24  1:58   ` heying (H)
  2021-03-24  8:19   ` Geert Uytterhoeven
  2 siblings, 0 replies; 9+ messages in thread
From: heying (H) @ 2021-03-24  1:58 UTC (permalink / raw)
  To: Alexandre Belloni
  Cc: mpe, benh, paulus, a.zummo, christophe.leroy, npiggin, msuchanek,
	tglx, peterz, geert+renesas, kernelfans, frederic, linuxppc-dev,
	linux-kernel, linux-rtc

Dear,


在 2021/3/24 6:18, Alexandre Belloni 写道:
> Hello,
>
> On 23/03/2021 05:12:57-0400, He Ying wrote:
>> We found these warnings in arch/powerpc/kernel/time.c as follows:
>> warning: symbol 'decrementer_max' was not declared. Should it be static?
>> warning: symbol 'rtc_lock' was not declared. Should it be static?
>> warning: symbol 'dtl_consumer' was not declared. Should it be static?
>>
>> Declare 'decrementer_max' and 'rtc_lock' in powerpc asm/time.h.
>> Rename 'rtc_lock' in drviers/rtc/rtc-vr41xx.c to 'vr41xx_rtc_lock' to
>> avoid the conflict with the variable in powerpc asm/time.h.
>> Move 'dtl_consumer' definition behind "include <asm/dtl.h>" because it
>> is declared there.
>>
>> Reported-by: Hulk Robot <hulkci@huawei.com>
>> Signed-off-by: He Ying <heying24@huawei.com>
>> ---
>> v2:
>> - Instead of including linux/mc146818rtc.h in powerpc kernel/time.c, declare
>>    rtc_lock in powerpc asm/time.h.
>>
> V1 was actually the correct thing to do. rtc_lock is there exactly
> because chrp and maple are using mc146818 compatible RTCs. This is then
> useful because then drivers/char/nvram.c is enabled. The proper fix
> would be to scrap all of that and use rtc-cmos for those platforms as
> this drives the RTC properly and exposes the NVRAM for the mc146818.

Do you mean that 'rtc_lock' declared in linux/mc146818rtc.h points to

same thing as that defined in powerpc kernel/time.c? And you think V1

was correct? Oh, I should have added you to my patch V1 senders:)

>
> Or at least, if there are no users for the char/nvram driver on those
> two platforms, remove the spinlock and stop enabling CONFIG_NVRAM or
> more likely rename the symbol as it seems to be abused by both chrp and
> powermac.
>
> I'm not completely against the rename in vr41xxx but the fix for the
> warnings can and should be contained in arch/powerpc.

Yes, I agree with you. But I have no choice because there is a compiling 
error.

Maybe there's a better way.

So, what about my patch V1? Should I resend it and add you to senders?


Thanks.


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

* Re: [PATCH v2 -next] powerpc: kernel/time.c - cleanup warnings
  2021-03-23 23:05   ` Alexandre Belloni
@ 2021-03-24  6:14     ` Christophe Leroy
  2021-03-24  6:22       ` Christophe Leroy
  0 siblings, 1 reply; 9+ messages in thread
From: Christophe Leroy @ 2021-03-24  6:14 UTC (permalink / raw)
  To: Alexandre Belloni, He Ying
  Cc: mpe, benh, paulus, a.zummo, npiggin, msuchanek, tglx, peterz,
	geert+renesas, kernelfans, frederic, linuxppc-dev, linux-kernel,
	linux-rtc



Le 24/03/2021 à 00:05, Alexandre Belloni a écrit :
> On 23/03/2021 23:18:17+0100, Alexandre Belloni wrote:
>> Hello,
>>
>> On 23/03/2021 05:12:57-0400, He Ying wrote:
>>> We found these warnings in arch/powerpc/kernel/time.c as follows:
>>> warning: symbol 'decrementer_max' was not declared. Should it be static?
>>> warning: symbol 'rtc_lock' was not declared. Should it be static?
>>> warning: symbol 'dtl_consumer' was not declared. Should it be static?
>>>
>>> Declare 'decrementer_max' and 'rtc_lock' in powerpc asm/time.h.
>>> Rename 'rtc_lock' in drviers/rtc/rtc-vr41xx.c to 'vr41xx_rtc_lock' to
>>> avoid the conflict with the variable in powerpc asm/time.h.
>>> Move 'dtl_consumer' definition behind "include <asm/dtl.h>" because it
>>> is declared there.
>>>
>>> Reported-by: Hulk Robot <hulkci@huawei.com>
>>> Signed-off-by: He Ying <heying24@huawei.com>
>>> ---
>>> v2:
>>> - Instead of including linux/mc146818rtc.h in powerpc kernel/time.c, declare
>>>    rtc_lock in powerpc asm/time.h.
>>>
>>
>> V1 was actually the correct thing to do. rtc_lock is there exactly
>> because chrp and maple are using mc146818 compatible RTCs. This is then
>> useful because then drivers/char/nvram.c is enabled. The proper fix
>> would be to scrap all of that and use rtc-cmos for those platforms as
>> this drives the RTC properly and exposes the NVRAM for the mc146818.
>>
>> Or at least, if there are no users for the char/nvram driver on those
>> two platforms, remove the spinlock and stop enabling CONFIG_NVRAM or
>> more likely rename the symbol as it seems to be abused by both chrp and
>> powermac.
>>
> 
> Ok so rtc_lock is not even used by the char/nvram.c driver as it is
> completely compiled out.
> 
> I guess it is fine having it move to the individual platform as looking
> very quickly at the Kconfig, it is not possible to select both
> simultaneously. Tentative patch:
> 

Looking at it once more, it looks like including linux/mc146818rtc.h is the thing to do, at least 
for now. Several platforms are defining the rtc_lock exactly the same way as powerpc does, and 
including mc146818rtc.h

I think that to get it clean, this change should go in a dedicated patch and do a bit more and 
explain exactly what is being do and why. I'll try to draft something for it.

He Y., can you make a version v3 of your patch excluding the rtc_lock change ?

Christophe

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

* Re: [PATCH v2 -next] powerpc: kernel/time.c - cleanup warnings
  2021-03-24  6:14     ` Christophe Leroy
@ 2021-03-24  6:22       ` Christophe Leroy
  2021-03-24  8:29         ` heying (H)
  0 siblings, 1 reply; 9+ messages in thread
From: Christophe Leroy @ 2021-03-24  6:22 UTC (permalink / raw)
  To: Alexandre Belloni, He Ying
  Cc: mpe, benh, paulus, a.zummo, npiggin, msuchanek, tglx, peterz,
	geert+renesas, kernelfans, frederic, linuxppc-dev, linux-kernel,
	linux-rtc



Le 24/03/2021 à 07:14, Christophe Leroy a écrit :
> 
> 
> Le 24/03/2021 à 00:05, Alexandre Belloni a écrit :
>> On 23/03/2021 23:18:17+0100, Alexandre Belloni wrote:
>>> Hello,
>>>
>>> On 23/03/2021 05:12:57-0400, He Ying wrote:
>>>> We found these warnings in arch/powerpc/kernel/time.c as follows:
>>>> warning: symbol 'decrementer_max' was not declared. Should it be static?
>>>> warning: symbol 'rtc_lock' was not declared. Should it be static?
>>>> warning: symbol 'dtl_consumer' was not declared. Should it be static?
>>>>
>>>> Declare 'decrementer_max' and 'rtc_lock' in powerpc asm/time.h.
>>>> Rename 'rtc_lock' in drviers/rtc/rtc-vr41xx.c to 'vr41xx_rtc_lock' to
>>>> avoid the conflict with the variable in powerpc asm/time.h.
>>>> Move 'dtl_consumer' definition behind "include <asm/dtl.h>" because it
>>>> is declared there.
>>>>
>>>> Reported-by: Hulk Robot <hulkci@huawei.com>
>>>> Signed-off-by: He Ying <heying24@huawei.com>
>>>> ---
>>>> v2:
>>>> - Instead of including linux/mc146818rtc.h in powerpc kernel/time.c, declare
>>>>    rtc_lock in powerpc asm/time.h.
>>>>
>>>
>>> V1 was actually the correct thing to do. rtc_lock is there exactly
>>> because chrp and maple are using mc146818 compatible RTCs. This is then
>>> useful because then drivers/char/nvram.c is enabled. The proper fix
>>> would be to scrap all of that and use rtc-cmos for those platforms as
>>> this drives the RTC properly and exposes the NVRAM for the mc146818.
>>>
>>> Or at least, if there are no users for the char/nvram driver on those
>>> two platforms, remove the spinlock and stop enabling CONFIG_NVRAM or
>>> more likely rename the symbol as it seems to be abused by both chrp and
>>> powermac.
>>>
>>
>> Ok so rtc_lock is not even used by the char/nvram.c driver as it is
>> completely compiled out.
>>
>> I guess it is fine having it move to the individual platform as looking
>> very quickly at the Kconfig, it is not possible to select both
>> simultaneously. Tentative patch:
>>
> 
> Looking at it once more, it looks like including linux/mc146818rtc.h is the thing to do, at least 
> for now. Several platforms are defining the rtc_lock exactly the same way as powerpc does, and 
> including mc146818rtc.h
> 
> I think that to get it clean, this change should go in a dedicated patch and do a bit more and 
> explain exactly what is being do and why. I'll try to draft something for it.
> 
> He Y., can you make a version v3 of your patch excluding the rtc_lock change ?
> 

Finally, I think there is not enough changes to justify a separate patch.

So you can send a V3 based on your V1. In addition to the changes you had in V1, please remove the 
declaration of rfc_lock in arch/powerpc/platforms/chrp/chrp.h

Christophe

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

* Re: [PATCH v2 -next] powerpc: kernel/time.c - cleanup warnings
  2021-03-23 22:18 ` Alexandre Belloni
  2021-03-23 23:05   ` Alexandre Belloni
  2021-03-24  1:58   ` heying (H)
@ 2021-03-24  8:19   ` Geert Uytterhoeven
  2021-03-24  9:09     ` Alexandre Belloni
  2 siblings, 1 reply; 9+ messages in thread
From: Geert Uytterhoeven @ 2021-03-24  8:19 UTC (permalink / raw)
  To: Alexandre Belloni
  Cc: He Ying, Michael Ellerman, Benjamin Herrenschmidt,
	Paul Mackerras, Alessandro Zummo, Christophe Leroy,
	Nicholas Piggin, Michal Suchanek, Thomas Gleixner,
	Peter Zijlstra, Geert Uytterhoeven, kernelfans, frederic,
	linuxppc-dev, Linux Kernel Mailing List, linux-rtc

Hi Alexandre,

On Tue, Mar 23, 2021 at 11:18 PM Alexandre Belloni
<alexandre.belloni@bootlin.com> wrote:
> On 23/03/2021 05:12:57-0400, He Ying wrote:
> > We found these warnings in arch/powerpc/kernel/time.c as follows:
> > warning: symbol 'decrementer_max' was not declared. Should it be static?
> > warning: symbol 'rtc_lock' was not declared. Should it be static?
> > warning: symbol 'dtl_consumer' was not declared. Should it be static?
> >
> > Declare 'decrementer_max' and 'rtc_lock' in powerpc asm/time.h.
> > Rename 'rtc_lock' in drviers/rtc/rtc-vr41xx.c to 'vr41xx_rtc_lock' to
> > avoid the conflict with the variable in powerpc asm/time.h.
> > Move 'dtl_consumer' definition behind "include <asm/dtl.h>" because it
> > is declared there.
> >
> > Reported-by: Hulk Robot <hulkci@huawei.com>
> > Signed-off-by: He Ying <heying24@huawei.com>
> > ---
> > v2:
> > - Instead of including linux/mc146818rtc.h in powerpc kernel/time.c, declare
> >   rtc_lock in powerpc asm/time.h.
> >
>
> V1 was actually the correct thing to do. rtc_lock is there exactly
> because chrp and maple are using mc146818 compatible RTCs. This is then
> useful because then drivers/char/nvram.c is enabled. The proper fix
> would be to scrap all of that and use rtc-cmos for those platforms as
> this drives the RTC properly and exposes the NVRAM for the mc146818.
>
> Or at least, if there are no users for the char/nvram driver on those
> two platforms, remove the spinlock and stop enabling CONFIG_NVRAM or
> more likely rename the symbol as it seems to be abused by both chrp and
> powermac.

IIRC, on CHRP LongTrail, NVRAM was inherited from CHRP's Mac ancestry,
not from CHRP's PC ancestry, and thus NVRAM is not the one in the
mc146818-compatible RTC.

http://users.telenet.be/geertu/Linux/PPC/DeviceTree.html confirms that,
showing that nvram is a different device node than rtc.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

* Re: [PATCH v2 -next] powerpc: kernel/time.c - cleanup warnings
  2021-03-24  6:22       ` Christophe Leroy
@ 2021-03-24  8:29         ` heying (H)
  0 siblings, 0 replies; 9+ messages in thread
From: heying (H) @ 2021-03-24  8:29 UTC (permalink / raw)
  To: Christophe Leroy, Alexandre Belloni
  Cc: mpe, benh, paulus, a.zummo, npiggin, msuchanek, tglx, peterz,
	geert+renesas, kernelfans, frederic, linuxppc-dev, linux-kernel,
	linux-rtc

Dear,


在 2021/3/24 14:22, Christophe Leroy 写道:
>
>
> Le 24/03/2021 à 07:14, Christophe Leroy a écrit :
>>
>>
>> Le 24/03/2021 à 00:05, Alexandre Belloni a écrit :
>>> On 23/03/2021 23:18:17+0100, Alexandre Belloni wrote:
>>>> Hello,
>>>>
>>>> On 23/03/2021 05:12:57-0400, He Ying wrote:
>>>>> We found these warnings in arch/powerpc/kernel/time.c as follows:
>>>>> warning: symbol 'decrementer_max' was not declared. Should it be 
>>>>> static?
>>>>> warning: symbol 'rtc_lock' was not declared. Should it be static?
>>>>> warning: symbol 'dtl_consumer' was not declared. Should it be static?
>>>>>
>>>>> Declare 'decrementer_max' and 'rtc_lock' in powerpc asm/time.h.
>>>>> Rename 'rtc_lock' in drviers/rtc/rtc-vr41xx.c to 'vr41xx_rtc_lock' to
>>>>> avoid the conflict with the variable in powerpc asm/time.h.
>>>>> Move 'dtl_consumer' definition behind "include <asm/dtl.h>" 
>>>>> because it
>>>>> is declared there.
>>>>>
>>>>> Reported-by: Hulk Robot <hulkci@huawei.com>
>>>>> Signed-off-by: He Ying <heying24@huawei.com>
>>>>> ---
>>>>> v2:
>>>>> - Instead of including linux/mc146818rtc.h in powerpc 
>>>>> kernel/time.c, declare
>>>>>    rtc_lock in powerpc asm/time.h.
>>>>>
>>>>
>>>> V1 was actually the correct thing to do. rtc_lock is there exactly
>>>> because chrp and maple are using mc146818 compatible RTCs. This is 
>>>> then
>>>> useful because then drivers/char/nvram.c is enabled. The proper fix
>>>> would be to scrap all of that and use rtc-cmos for those platforms as
>>>> this drives the RTC properly and exposes the NVRAM for the mc146818.
>>>>
>>>> Or at least, if there are no users for the char/nvram driver on those
>>>> two platforms, remove the spinlock and stop enabling CONFIG_NVRAM or
>>>> more likely rename the symbol as it seems to be abused by both chrp 
>>>> and
>>>> powermac.
>>>>
>>>
>>> Ok so rtc_lock is not even used by the char/nvram.c driver as it is
>>> completely compiled out.
>>>
>>> I guess it is fine having it move to the individual platform as looking
>>> very quickly at the Kconfig, it is not possible to select both
>>> simultaneously. Tentative patch:
>>>
>>
>> Looking at it once more, it looks like including linux/mc146818rtc.h 
>> is the thing to do, at least for now. Several platforms are defining 
>> the rtc_lock exactly the same way as powerpc does, and including 
>> mc146818rtc.h
>>
>> I think that to get it clean, this change should go in a dedicated 
>> patch and do a bit more and explain exactly what is being do and why. 
>> I'll try to draft something for it.
>>
>> He Y., can you make a version v3 of your patch excluding the rtc_lock 
>> change ?
>>
>
> Finally, I think there is not enough changes to justify a separate patch.
>
> So you can send a V3 based on your V1. In addition to the changes you 
> had in V1, please remove the declaration of rfc_lock in 
> arch/powerpc/platforms/chrp/chrp.h

So glad to hear that. I'll do that and send my V3.


Thanks.


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

* Re: [PATCH v2 -next] powerpc: kernel/time.c - cleanup warnings
  2021-03-24  8:19   ` Geert Uytterhoeven
@ 2021-03-24  9:09     ` Alexandre Belloni
  0 siblings, 0 replies; 9+ messages in thread
From: Alexandre Belloni @ 2021-03-24  9:09 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: He Ying, Michael Ellerman, Benjamin Herrenschmidt,
	Paul Mackerras, Alessandro Zummo, Christophe Leroy,
	Nicholas Piggin, Michal Suchanek, Thomas Gleixner,
	Peter Zijlstra, Geert Uytterhoeven, kernelfans, frederic,
	linuxppc-dev, Linux Kernel Mailing List, linux-rtc

On 24/03/2021 09:19:58+0100, Geert Uytterhoeven wrote:
> Hi Alexandre,
> 
> On Tue, Mar 23, 2021 at 11:18 PM Alexandre Belloni
> <alexandre.belloni@bootlin.com> wrote:
> > On 23/03/2021 05:12:57-0400, He Ying wrote:
> > > We found these warnings in arch/powerpc/kernel/time.c as follows:
> > > warning: symbol 'decrementer_max' was not declared. Should it be static?
> > > warning: symbol 'rtc_lock' was not declared. Should it be static?
> > > warning: symbol 'dtl_consumer' was not declared. Should it be static?
> > >
> > > Declare 'decrementer_max' and 'rtc_lock' in powerpc asm/time.h.
> > > Rename 'rtc_lock' in drviers/rtc/rtc-vr41xx.c to 'vr41xx_rtc_lock' to
> > > avoid the conflict with the variable in powerpc asm/time.h.
> > > Move 'dtl_consumer' definition behind "include <asm/dtl.h>" because it
> > > is declared there.
> > >
> > > Reported-by: Hulk Robot <hulkci@huawei.com>
> > > Signed-off-by: He Ying <heying24@huawei.com>
> > > ---
> > > v2:
> > > - Instead of including linux/mc146818rtc.h in powerpc kernel/time.c, declare
> > >   rtc_lock in powerpc asm/time.h.
> > >
> >
> > V1 was actually the correct thing to do. rtc_lock is there exactly
> > because chrp and maple are using mc146818 compatible RTCs. This is then
> > useful because then drivers/char/nvram.c is enabled. The proper fix
> > would be to scrap all of that and use rtc-cmos for those platforms as
> > this drives the RTC properly and exposes the NVRAM for the mc146818.
> >
> > Or at least, if there are no users for the char/nvram driver on those
> > two platforms, remove the spinlock and stop enabling CONFIG_NVRAM or
> > more likely rename the symbol as it seems to be abused by both chrp and
> > powermac.
> 
> IIRC, on CHRP LongTrail, NVRAM was inherited from CHRP's Mac ancestry,
> not from CHRP's PC ancestry, and thus NVRAM is not the one in the
> mc146818-compatible RTC.
> 
> http://users.telenet.be/geertu/Linux/PPC/DeviceTree.html confirms that,
> showing that nvram is a different device node than rtc.
> 

Yes, what I missed was the ifdefery in drivers/char/nvram.c that makes
it a completely different driver on both platforms. I tend to forget
about that as reading this driver is not a pleasant experience. I would
really like to get rid of the x86 part which would in turn allow to
remove the global rtc_lock spinlock on all architectures.

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

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

end of thread, back to index

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-23  9:12 [PATCH v2 -next] powerpc: kernel/time.c - cleanup warnings He Ying
2021-03-23 22:18 ` Alexandre Belloni
2021-03-23 23:05   ` Alexandre Belloni
2021-03-24  6:14     ` Christophe Leroy
2021-03-24  6:22       ` Christophe Leroy
2021-03-24  8:29         ` heying (H)
2021-03-24  1:58   ` heying (H)
2021-03-24  8:19   ` Geert Uytterhoeven
2021-03-24  9:09     ` Alexandre Belloni

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