[RFC] Option to disable mapping genrtc calls to ppc_md calls

[RFC] Option to disable mapping genrtc calls to ppc_md calls

[Mark A. Greer]

[-- Attachment #1: Type: text/plain, Size: 790 bytes --]

All,

I have a platform with an i2c rtc chip.  Since much of the code for an 
rtc driver is already in drivers/char/genrtc.c, I would like to reuse 
that code and directly implement get_rtc_time(), et. al. in the rtc 
driver.  The problem is that include/asm-ppc/rtc.h assumes that 
get_rtc_time(), et. al. should be mapped to ppc_md.get_rtc_time() et. 
al.  To work around this, I made an option to turn off that assumption.  
The patch is included.

There are 2 reasons to not use the ppc_md.get_rtc_time() et. al. interfaces:
1) They are called before the i2c driver is initialized and even loaded 
if its a module.
2) Its ppc-specific.  Implementing get_rtc_time() et. al. directly makes 
it generic across all architectures.

Is there a better way to do this?

Comments welcome.

Mark

[-- Attachment #2: rtc.patch --]
[-- Type: text/plain, Size: 1635 bytes --]

===== drivers/char/Kconfig 1.60 vs edited =====
--- 1.60/drivers/char/Kconfig	2005-01-15 15:31:07 -07:00
+++ edited/drivers/char/Kconfig	2005-01-17 13:49:52 -07:00
@@ -777,6 +777,17 @@
 	  Provides an emulation for RTC_UIE which is required by some programs
 	  and may improve precision of the generic RTC support in some cases.
 
+config GEN_RTC_DISABLE_RTC_MAPPING
+	bool "Disable mapping genrtc interface to ppc-specific calls"
+	depends on GEN_RTC && PPC32
+	default n
+	help
+	  PPC systems typically map genrtc calls to PPC specific routines.
+	  However, this needs to be disabled when using an RTC chip whose
+	  driver implements the genrtc calls.
+
+	  To disable the mapping to PPC specific routines, chose Y here.
+
 config EFI_RTC
 	bool "EFI Real Time Clock Services"
 	depends on IA64
===== include/asm-ppc/rtc.h 1.7 vs edited =====
--- 1.7/include/asm-ppc/rtc.h	2003-09-12 09:26:56 -07:00
+++ edited/include/asm-ppc/rtc.h	2005-01-17 13:52:52 -07:00
@@ -42,6 +42,7 @@
 #define RTC_24H 0x02		/* 24 hour mode - else hours bit 7 means pm */
 #define RTC_DST_EN 0x01	        /* auto switch DST - works f. USA only */
 
+#if !defined(CONFIG_GEN_RTC_DISABLE_RTC_MAPPING)
 static inline unsigned int get_rtc_time(struct rtc_time *time)
 {
 	if (ppc_md.get_rtc_time) {
@@ -91,5 +92,12 @@
 	return -EINVAL;
 }
 
+#else
+extern unsigned int get_rtc_time(struct rtc_time *time);
+extern int set_rtc_time(struct rtc_time *time);
+extern unsigned int get_rtc_ss(void);
+extern int get_rtc_pll(struct rtc_pll_info *pll);
+extern int set_rtc_pll(struct rtc_pll_info *pll);
+#endif
 #endif /* __KERNEL__ */
 #endif /* __ASM_RTC_H__ */

Re: [RFC] Option to disable mapping genrtc calls to ppc_md calls

[Geert Uytterhoeven]

On Mon, 17 Jan 2005, Mark A. Greer wrote:
> I have a platform with an i2c rtc chip.  Since much of the code for an rtc
> driver is already in drivers/char/genrtc.c, I would like to reuse that code
> and directly implement get_rtc_time(), et. al. in the rtc driver.  The problem
> is that include/asm-ppc/rtc.h assumes that get_rtc_time(), et. al. should be
> mapped to ppc_md.get_rtc_time() et. al.  To work around this, I made an option
> to turn off that assumption.  The patch is included.
> 
> There are 2 reasons to not use the ppc_md.get_rtc_time() et. al. interfaces:
> 1) They are called before the i2c driver is initialized and even loaded if its
> a module.

How is this solved by your patch if genrtc is builtin? How is your solution
different from setting ppc_md.get_rtc_time to your get_rtc_time routine?

> 2) Its ppc-specific.  Implementing get_rtc_time() et. al. directly makes it
> generic across all architectures.

... but prevents you from building a kernel that supports both normal RTCs and
your i2c 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

Re: [RFC] Option to disable mapping genrtc calls to ppc_md calls

[Tom Rini]

On Mon, Jan 17, 2005 at 02:10:00PM -0700, Mark A. Greer wrote:

> All,
> 
> I have a platform with an i2c rtc chip.  Since much of the code for an 
> rtc driver is already in drivers/char/genrtc.c, I would like to reuse 
> that code and directly implement get_rtc_time(), et. al. in the rtc 
> driver.  The problem is that include/asm-ppc/rtc.h assumes that 
> get_rtc_time(), et. al. should be mapped to ppc_md.get_rtc_time() et. 
> al.  To work around this, I made an option to turn off that assumption.  
> The patch is included.
> 
> There are 2 reasons to not use the ppc_md.get_rtc_time() et. al. interfaces:
> 1) They are called before the i2c driver is initialized and even loaded 
> if its a module.

But they check if it's set, so they can be assigned later and this is
OK.

> 2) Its ppc-specific.  Implementing get_rtc_time() et. al. directly makes 
> it generic across all architectures.

Guessing, this is for a marvell chipset that's found on MIPS too.

> Is there a better way to do this?

How about we try borrowing the MIPS abstraction and force todc_time,
pmac_time (any others?) to directly define (and EXPORT_SYMBOL)
get_rtc_time / set_rtc_time / etc.

-- 
Tom Rini
http://gate.crashing.org/~trini/

Re: [RFC] Option to disable mapping genrtc calls to ppc_md calls

[Dan Malek]

On Jan 18, 2005, at 11:15 AM, Tom Rini wrote:

> On Mon, Jan 17, 2005 at 02:10:00PM -0700, Mark A. Greer wrote:

>> There are 2 reasons to not use the ppc_md.get_rtc_time() et. al. 
>> interfaces:
>> 1) They are called before the i2c driver is initialized and even 
>> loaded
>> if its a module.

There are three reasons.  You don't want to use an I2c rtc clock at
all in these functions because they get can get called from the
clock interrupt to update the time in the rtc.  If it does happen to 
work,
it creates long latencies in the timer interrupt.  If the i2c requires 
an
interrupt, they system will crash or hang.

A system using an I2C RTC should find some way to access the
clock from application space as a standard I2C device and manage
time/clock from the application, not from the kernel.

	-- Dan

Re: [RFC] Option to disable mapping genrtc calls to ppc_md calls

[Tolunay Orkun]

Dan Malek wrote:
> 
> On Jan 18, 2005, at 11:15 AM, Tom Rini wrote:
> 
>> On Mon, Jan 17, 2005 at 02:10:00PM -0700, Mark A. Greer wrote:
> 
> 
>>> There are 2 reasons to not use the ppc_md.get_rtc_time() et. al. 
>>> interfaces:
>>> 1) They are called before the i2c driver is initialized and even loaded
>>> if its a module.
> 
> 
> There are three reasons.  You don't want to use an I2c rtc clock at
> all in these functions because they get can get called from the
> clock interrupt to update the time in the rtc.  If it does happen to work,
> it creates long latencies in the timer interrupt.  If the i2c requires an
> interrupt, they system will crash or hang.
> 
> A system using an I2C RTC should find some way to access the
> clock from application space as a standard I2C device and manage
> time/clock from the application, not from the kernel.

This is exactly what I've done for our PPC405GP based Linux port 
(2.4.25). I've written a clone of hwclock utility using /dev/i2c0 to 
access Dallas RTC chip with I2C interface. I setthe system clock from 
RTC within /etc/rcS (Busybox). It is less than perfect as early logs 
have the date wrong but it was the direction of least resistance.

Still, I think there should be a better standardized RTC interface to 
help deal with I2C/SMBus based RTC chips.

Re: [RFC] Option to disable mapping genrtc calls to ppc_md calls

[Tom Rini]

On Tue, Jan 18, 2005 at 11:25:33AM -0500, Dan Malek wrote:
> 
> On Jan 18, 2005, at 11:15 AM, Tom Rini wrote:
> 
> >On Mon, Jan 17, 2005 at 02:10:00PM -0700, Mark A. Greer wrote:
> 
> >>There are 2 reasons to not use the ppc_md.get_rtc_time() et. al. 
> >>interfaces:
> >>1) They are called before the i2c driver is initialized and even 
> >>loaded
> >>if its a module.
> 
> There are three reasons.  You don't want to use an I2c rtc clock at
> all in these functions because they get can get called from the
> clock interrupt to update the time in the rtc.  If it does happen to 
> work,
> it creates long latencies in the timer interrupt.  If the i2c requires 
> an
> interrupt, they system will crash or hang.

I think one of us wasn't clear.  I'm not arguing for nuking
ppc_md.{get,set}_rtc_time(), I'm arguing for nuking
get_rtc_time()/set_rtc_time() inlines from <asm-ppc/rtc.h> (which are
used by drivers/char/genrtc.c) in favor of todc_time et al providing the
functions for genrtc.  So all of the other places we use
ppc_md.{get,set}_rtc_time() are unchanged.

-- 
Tom Rini
http://gate.crashing.org/~trini/

Re: [RFC] Option to disable mapping genrtc calls to ppc_md calls

[Tom Rini]

On Tue, Jan 18, 2005 at 11:39:46AM -0600, Tolunay Orkun wrote:
> Dan Malek wrote:
> >
> >On Jan 18, 2005, at 11:15 AM, Tom Rini wrote:
> >
> >>On Mon, Jan 17, 2005 at 02:10:00PM -0700, Mark A. Greer wrote:
> >
> >
> >>>There are 2 reasons to not use the ppc_md.get_rtc_time() et. al. 
> >>>interfaces:
> >>>1) They are called before the i2c driver is initialized and even loaded
> >>>if its a module.
> >
> >
> >There are three reasons.  You don't want to use an I2c rtc clock at
> >all in these functions because they get can get called from the
> >clock interrupt to update the time in the rtc.  If it does happen to work,
> >it creates long latencies in the timer interrupt.  If the i2c requires an
> >interrupt, they system will crash or hang.
> >
> >A system using an I2C RTC should find some way to access the
> >clock from application space as a standard I2C device and manage
> >time/clock from the application, not from the kernel.
> 
> This is exactly what I've done for our PPC405GP based Linux port 
> (2.4.25). I've written a clone of hwclock utility using /dev/i2c0 to 
> access Dallas RTC chip with I2C interface. I setthe system clock from 
> RTC within /etc/rcS (Busybox). It is less than perfect as early logs 
> have the date wrong but it was the direction of least resistance.
> 
> Still, I think there should be a better standardized RTC interface to 
> help deal with I2C/SMBus based RTC chips.

There kind of is now, arch/arm/common/rtctime.c (thought of for I2C RTCs
and has alarm support).

-- 
Tom Rini
http://gate.crashing.org/~trini/

Re: [RFC] Option to disable mapping genrtc calls to ppc_md calls

[Mark A. Greer]

Geert Uytterhoeven wrote:

>On Mon, 17 Jan 2005, Mark A. Greer wrote:
>  
>
>><snip>
>>
>>There are 2 reasons to not use the ppc_md.get_rtc_time() et. al. interfaces:
>>1) They are called before the i2c driver is initialized and even loaded if its
>>a module.
>>    
>>
>
>How is this solved by your patch if genrtc is builtin?
>

It solved because the rtc interface isn't called until you do an hwclock 
presumably in a startup script.

> How is your solution
>different from setting ppc_md.get_rtc_time to your get_rtc_time routine?
>

It arch independent which was the whole motivation for doing it this 
way.  However, it does rely on a startup script to do a 'hwclock 
--hctosys' which happen after driver initialization.  From what I can 
tell sysvinit used to do the hwclock but doesn't anymore so you need a 
script.  The mvl userland has a startup script that does this; others 
probably do too.  Note that using a startup script to do a hwclock is 
pretty standard AFAICT.

>>2) Its ppc-specific.  Implementing get_rtc_time() et. al. directly makes it
>>generic across all architectures.
>>    
>>
>
>... but prevents you from building a kernel that supports both normal RTCs and
>your i2c RTC.
>

Well, yes but you aren't going to be able to do this and be 
arch-agnostic.  If you don't care about running on anything but ppc then 
you can to it the way i2c rtc's have been done before and either kludge 
into the i2c driver during early startup or setup ppc_md.get_rtc_time() 
once the i2c/rtc driver(s) are initialized.  This patch will not 
interfere with that unless you deliberately set that option.

I think the real question you should be ask is are the 
ppc_md.get_rtc_time() et. al. calls really necessary?  Or to put it 
another way, should we be grilling the people who are submitting 
ppc-only solutions and not the ones submitting generic solutions?  :)

Mark

Re: [RFC] Option to disable mapping genrtc calls to ppc_md calls

[Eugene Surovegin]

On Tue, Jan 18, 2005 at 11:25:33AM -0500, Dan Malek wrote:
> There are three reasons.  You don't want to use an I2c rtc clock at
> all in these functions because they get can get called from the
> clock interrupt to update the time in the rtc.  If it does happen to 
> work,
> it creates long latencies in the timer interrupt.  If the i2c requires 
> an
> interrupt, they system will crash or hang.
> 
> A system using an I2C RTC should find some way to access the
> clock from application space as a standard I2C device and manage
> time/clock from the application, not from the kernel.

Well, it was discussed numerous times before with solutions how to use 
i2c based RTC as well.

I use i2c RTC which requires interrupt and guess what, my kernel 
doesn't crash when timer_interrupt calls ppc_md.set_rtc_time.

It's just a matter of writing correct i2c RTC driver.

Dan, please, stop spreading "i2c-RTC" FUD :).

--
Eugene

Re: [RFC] Option to disable mapping genrtc calls to ppc_md calls

[Mark A. Greer]

Tom Rini wrote:

>On Mon, Jan 17, 2005 at 02:10:00PM -0700, Mark A. Greer wrote:
>
>  
>
>><snip>
>>
>>There are 2 reasons to not use the ppc_md.get_rtc_time() et. al. interfaces:
>>1) They are called before the i2c driver is initialized and even loaded 
>>if its a module.
>>    
>>
>
>But they check if it's set, so they can be assigned later and this is
>OK.
>

But looking at ppc_md.<anything> automatically makes it ppc only.  
Pretty much any of the rtc chips that we use on ppc platforms could 
appear on almost any other platform with a different processor 
architecture.  So the question is, why do we keep adding ppc-only 
support for rtc chips?  Why are we not putting our effort into making 
code that works on all architectures?  Its easy enough to do...

>>2) Its ppc-specific.  Implementing get_rtc_time() et. al. directly makes 
>>it generic across all architectures.
>>    
>>
>
>Guessing, this is for a marvell chipset that's found on MIPS too.
>

That's an example but like I said above rtc chips are not tied to any 
particular processor architecture.

>>Is there a better way to do this?
>>    
>>
>
>How about we try borrowing the MIPS abstraction and force todc_time,
>pmac_time (any others?) to directly define (and EXPORT_SYMBOL)
>get_rtc_time / set_rtc_time / etc.
>

Yep, MIPS has a solution...and so does ARM...and so does PPC.  This is 
sort of my point.

If we really want to do it right then someone needs to architect a 
generic solution.  What I have done is generic but does not handle the 
case that Geert mentioned when you have one kernel binary and several 
possible rtc chips.  In the meantime, what I have done works fine for 
all but that case.

Mark

Re: [RFC] Option to disable mapping genrtc calls to ppc_md calls

[Mark A. Greer]

Tom Rini wrote:

>On Tue, Jan 18, 2005 at 11:25:33AM -0500, Dan Malek wrote:
>  
>
>>On Jan 18, 2005, at 11:15 AM, Tom Rini wrote:
>>
>>    
>>
>>>On Mon, Jan 17, 2005 at 02:10:00PM -0700, Mark A. Greer wrote:
>>>      
>>>
>>>>There are 2 reasons to not use the ppc_md.get_rtc_time() et. al. 
>>>>interfaces:
>>>>1) They are called before the i2c driver is initialized and even 
>>>>loaded
>>>>if its a module.
>>>>        
>>>>
>>There are three reasons.  You don't want to use an I2c rtc clock at
>>all in these functions because they get can get called from the
>>clock interrupt to update the time in the rtc.  If it does happen to 
>>work,
>>it creates long latencies in the timer interrupt.  If the i2c requires 
>>an
>>interrupt, they system will crash or hang.
>>    
>>
>
>I think one of us wasn't clear.  I'm not arguing for nuking
>ppc_md.{get,set}_rtc_time(), I'm arguing for nuking
>get_rtc_time()/set_rtc_time() inlines from <asm-ppc/rtc.h> (which are
>used by drivers/char/genrtc.c) in favor of todc_time et al providing the
>functions for genrtc.  So all of the other places we use
>ppc_md.{get,set}_rtc_time() are unchanged.
>  
>

Ahh.  Okay, that's good but it should be done in drivers/rtc or 
something like and not just another arch specific solution.  I want to 
stress again that rtc chips typically have no care in the world what 
processor happens to be stuck on the board that they are also stuck on.  
There is no reason our software should either.

Mark

Re: [RFC] Option to disable mapping genrtc calls to ppc_md calls

[Eugene Surovegin]

On Tue, Jan 18, 2005 at 11:40:50AM -0700, Mark A. Greer wrote:
> However, it does rely on a startup script to do a 'hwclock 
> --hctosys' which happen after driver initialization.  From what I can 
> tell sysvinit used to do the hwclock but doesn't anymore so you need a 
> script.  The mvl userland has a startup script that does this; others 
> probably do too.  Note that using a startup script to do a hwclock is 
> pretty standard AFAICT.

You can always call do_settimeofday from your i2c driver 
initialization. No scripts will be required and system behavior will 
be similar to the case when ppc_md.get_rtc_time is called during init 
to get correct time from RTC.

--
Eugene

Re: [RFC] Option to disable mapping genrtc calls to ppc_md calls

[Tom Rini]

On Tue, Jan 18, 2005 at 11:55:54AM -0700, Mark A. Greer wrote:
> Tom Rini wrote:
[snip]
> >>Is there a better way to do this?
> >>   
> >>
> >
> >How about we try borrowing the MIPS abstraction and force todc_time,
> >pmac_time (any others?) to directly define (and EXPORT_SYMBOL)
> >get_rtc_time / set_rtc_time / etc.
> >
> 
> Yep, MIPS has a solution...and so does ARM...and so does PPC.  This is 
> sort of my point.

And my point was to use someone elses solution, 'cuz that's how we go
from N to N-1 to 1 :)

> If we really want to do it right then someone needs to architect a 
> generic solution.  What I have done is generic but does not handle the 
> case that Geert mentioned when you have one kernel binary and several 
> possible rtc chips.  In the meantime, what I have done works fine for 
> all but that case.

I guess there's two points:
- How does your solution differ from what MIPS does, and probably ARM
  does of saying the backend (todc_time, i2c-foo) provides
  get_rtc_time/set_rtc_time?
- I horribly briefly talked with rmk about this a long time ago, and I
  think he has the generic solution, siting in arch/arm/common/rtctime.c
  (sure it would need to be moved to drivers/char/something, but..).
- I lied, #3 how does ARM, which I think lets you select multiple
  boards, and thus probably end up with multiple rtc chip choices, deal
  with it.

-- 
Tom Rini
http://gate.crashing.org/~trini/

Re: [RFC] Option to disable mapping genrtc calls to ppc_md calls

[Tom Rini]

On Tue, Jan 18, 2005 at 11:58:25AM -0700, Mark A. Greer wrote:
> Tom Rini wrote:
> 
> >On Tue, Jan 18, 2005 at 11:25:33AM -0500, Dan Malek wrote:
> > 
> >
> >>On Jan 18, 2005, at 11:15 AM, Tom Rini wrote:
> >>
> >>   
> >>
> >>>On Mon, Jan 17, 2005 at 02:10:00PM -0700, Mark A. Greer wrote:
> >>>     
> >>>
> >>>>There are 2 reasons to not use the ppc_md.get_rtc_time() et. al. 
> >>>>interfaces:
> >>>>1) They are called before the i2c driver is initialized and even 
> >>>>loaded
> >>>>if its a module.
> >>>>       
> >>>>
> >>There are three reasons.  You don't want to use an I2c rtc clock at
> >>all in these functions because they get can get called from the
> >>clock interrupt to update the time in the rtc.  If it does happen to 
> >>work,
> >>it creates long latencies in the timer interrupt.  If the i2c requires 
> >>an
> >>interrupt, they system will crash or hang.
> >>   
> >>
> >
> >I think one of us wasn't clear.  I'm not arguing for nuking
> >ppc_md.{get,set}_rtc_time(), I'm arguing for nuking
> >get_rtc_time()/set_rtc_time() inlines from <asm-ppc/rtc.h> (which are
> >used by drivers/char/genrtc.c) in favor of todc_time et al providing the
> >functions for genrtc.  So all of the other places we use
> >ppc_md.{get,set}_rtc_time() are unchanged.
> 
> Ahh.  Okay, that's good but it should be done in drivers/rtc or 
> something like and not just another arch specific solution.

It's not an arch specific solution today.  It's just that no one that's
written an rtc chip library (*cough* todc_time.c *cough*) has placed one
in drivers/char/ and let the chips (or the file) be selected.  genrtc.c
already says "someone else tell me how to get the time".  I kinda sorta
think arch/arm/common/rtctime.c does too, except it has hooks for alarm.

-- 
Tom Rini
http://gate.crashing.org/~trini/

Re: [RFC] Option to disable mapping genrtc calls to ppc_md calls

[Mark A. Greer]

Tom Rini wrote:

>On Tue, Jan 18, 2005 at 11:55:54AM -0700, Mark A. Greer wrote:
>  
>
>>Tom Rini wrote:
>>    
>>
>[snip]
>  
>
>>>>Is there a better way to do this?
>>>>  
>>>>
>>>>        
>>>>
>>>How about we try borrowing the MIPS abstraction and force todc_time,
>>>pmac_time (any others?) to directly define (and EXPORT_SYMBOL)
>>>get_rtc_time / set_rtc_time / etc.
>>>
>>>      
>>>
>>Yep, MIPS has a solution...and so does ARM...and so does PPC.  This is 
>>sort of my point.
>>    
>>
>
>And my point was to use someone elses solution, 'cuz that's how we go
>from N to N-1 to 1 :)
>

If effort is going to be put into this then why not just go from N 
directly to 1?  BTW, I am not volunteering.  ;)

>
>  
>
>>If we really want to do it right then someone needs to architect a 
>>generic solution.  What I have done is generic but does not handle the 
>>case that Geert mentioned when you have one kernel binary and several 
>>possible rtc chips.  In the meantime, what I have done works fine for 
>>all but that case.
>>    
>>
>
>I guess there's two points:
>- How does your solution differ from what MIPS does, and probably ARM
>  does of saying the backend (todc_time, i2c-foo) provides
>  get_rtc_time/set_rtc_time?
>
First, I want to make sure we all on the same page.  There are really 
two issues being discussed and I think we're all swinging back and forth 
between the two.

Issue 1) -  My patch:

I had to write some support for an ST m41t00 rtc w/ an i2c interface.  I 
could have made it ppc only or generic with the same amount of effort so 
I chose the generic one.  The gereric one I chose was to use the code in 
genrtc and interface directly to the bottom of that code b/c that's 
where things become arch-specific.  However, that is where I collided 
with asm-ppc/rtc.h, hence the patch.

What I did is generic because genrtc.c is generic, the rtc "driver" is 
generic, and you can plug in any generic i2c algo/adapter driver 
underneath the entire thing.

Issue 2) - What should the *real* rtc architecture be?

RMK's solution may be fine, I'd have to look.  I think a discussion like 
this is good but I know I don't have the time right now to do it.

This is the one I think you, Tom, are talking about.  That's good but 
just understand that my patch has nothing to do with a generic solution 
for all rtc's.  I'm just trying to get this one to work (issue 1).

>- I horribly briefly talked with rmk about this a long time ago, and I
>  think he has the generic solution, siting in arch/arm/common/rtctime.c
>  (sure it would need to be moved to drivers/char/something, but..).
>

Yep, if it isn't in the right place, it doesn't help (for now anyway).

>- I lied, #3 how does ARM, which I think lets you select multiple
>  boards, and thus probably end up with multiple rtc chip choices, deal
>  with it.
>

Yep, ARM has  a reasonable solution but its ARM only and I'm not trying 
to rewrite anything at this point (see above).

Mark

Re: [RFC] Option to disable mapping genrtc calls to ppc_md calls

[Mark A. Greer]

Tom Rini wrote:

>On Tue, Jan 18, 2005 at 11:58:25AM -0700, Mark A. Greer wrote:
>  
>
>>Tom Rini wrote:
>>
>>    
>>
>>>I think one of us wasn't clear.  I'm not arguing for nuking
>>>ppc_md.{get,set}_rtc_time(), I'm arguing for nuking
>>>get_rtc_time()/set_rtc_time() inlines from <asm-ppc/rtc.h> (which are
>>>used by drivers/char/genrtc.c) in favor of todc_time et al providing the
>>>functions for genrtc.  So all of the other places we use
>>>ppc_md.{get,set}_rtc_time() are unchanged.
>>>      
>>>
>>Ahh.  Okay, that's good but it should be done in drivers/rtc or 
>>something like and not just another arch specific solution.
>>    
>>
>
>It's not an arch specific solution today.
>

Okay, I see what you mean and yes moving that up (or something 
equivalent) would be a good idea.

That's not what my patch is about though.  My patch is just so I can 
provide a generic solution with what is there today.  Its not 
redesigning anything, its just getting asm-ppc/rtc.h out of the way so I 
can make my own, generic get_rtc_time(), etc.

>  It's just that no one that's
>written an rtc chip library (*cough* todc_time.c *cough*)
>

Heh!

> has placed one
>in drivers/char/ and let the chips (or the file) be selected.  genrtc.c
>already says "someone else tell me how to get the time".  I kinda sorta
>think arch/arm/common/rtctime.c does too, except it has hooks for alarm.
>

Yeah, its probably time for a better solution to what's there today...  
Maybe I can get to it but it won't be for a while.

Mark

Re: [RFC] Option to disable mapping genrtc calls to ppc_md calls

[Tom Rini]

On Tue, Jan 18, 2005 at 12:43:12PM -0700, Mark A. Greer wrote:
> Tom Rini wrote:
> 
> >On Tue, Jan 18, 2005 at 11:58:25AM -0700, Mark A. Greer wrote:
> > 
> >
> >>Tom Rini wrote:
> >>
> >>   
> >>
> >>>I think one of us wasn't clear.  I'm not arguing for nuking
> >>>ppc_md.{get,set}_rtc_time(), I'm arguing for nuking
> >>>get_rtc_time()/set_rtc_time() inlines from <asm-ppc/rtc.h> (which are
> >>>used by drivers/char/genrtc.c) in favor of todc_time et al providing the
> >>>functions for genrtc.  So all of the other places we use
> >>>ppc_md.{get,set}_rtc_time() are unchanged.
> >>>     
> >>>
> >>Ahh.  Okay, that's good but it should be done in drivers/rtc or 
> >>something like and not just another arch specific solution.
> >>   
> >>
> >
> >It's not an arch specific solution today.
> >
> 
> Okay, I see what you mean and yes moving that up (or something 
> equivalent) would be a good idea.
> 
> That's not what my patch is about though.  My patch is just so I can 
> provide a generic solution with what is there today.  Its not 
> redesigning anything, its just getting asm-ppc/rtc.h out of the way so I 
> can make my own, generic get_rtc_time(), etc.

I guess the problem I have is you're not providing a generic solution
(it won't work on PPC_MULTIPLATFORM), you're providing a different hook
for your board(s) :)

Taking a look at arch/arm/common/rtctime.c again finally, it solves this
problem rather elegantly.  The board registers with the driver the
rtc_ops to get at the chip.

So until we get around to stealing arch/arm/common/rtctime.c, I'd rather
you used something like:
- arch/ppc/Kconfig (or where ever you add the board bits)
  config PPC_DIFFERENT_GENRTC_HOOKS
  default y if BOARD_A || BOARD_B
- include/asm-ppc/rtc.h : what you had, basically.

-- 
Tom Rini
http://gate.crashing.org/~trini/

Re: [RFC] Option to disable mapping genrtc calls to ppc_md calls

[Mark A. Greer]

Tom Rini wrote:

>
>I guess the problem I have is you're not providing a generic solution
>(it won't work on PPC_MULTIPLATFORM), you're providing a different hook
>for your board(s) :)
>

Tom, again, [I think that] you are thinking the patch I submitted is 
trying to solve something that it isn't.  I think we both agree that the 
current rtc architecture (or lack thereof) needs to be improved.  And 
maybe the ARM way is the way to go.  That's fine but that's not what the 
patch I submitted is trying to do.

The patch I submittted is only trying to be the most generic it can be 
with the rtc architecture that is there today.  The patch will work on 
any architecture, even on PPC/MULTIPLATFORM.  It is not only for my 
boards, its for any board on any architecture.  Also, it isn't a 
different hook, its the same hook asm-ppc/rtc.c uses.  The only thing it 
doesn't work for is one kernel binary supporting more than one rtc 
chip.  So its about as generic as you can get given the rtc architecture 
that is there today.

Also, this is not without precedent.  drivers/char/ds1302.c, for 
example, is a complete parallel as what I'm doing.  The only difference 
is that it [essentially] duplicated the code in genrtc and its not an 
i2c device.

>
>Taking a look at arch/arm/common/rtctime.c again finally, it solves this
>problem rather elegantly.  The board registers with the driver the
>rtc_ops to get at the chip.
>

That's nice...  :)

>
>So until we get around to stealing arch/arm/common/rtctime.c, I'd rather
>you used something like:
>- arch/ppc/Kconfig (or where ever you add the board bits)
>  config PPC_DIFFERENT_GENRTC_HOOKS
>  default y if BOARD_A || BOARD_B
>

Why?  There is no reason do add in the 'if BOARD_A' stuff.  I've added a 
simple option, if you want to use the m41t00 rtc--this is what I'm doing 
this for--select i2c and the correct i2c algo/adapter, select the m41t00 
i2c client, and [if on ppc] select the option to turn off 
include/asm-ppc/rtc.h.  If its too hard to remember, it can be easily 
captured in a arch/ppc/configs/xxx_defconfig file.  :)

>- include/asm-ppc/rtc.h : what you had, basically.
>
>  
>

Mark

Re: [RFC] Option to disable mapping genrtc calls to ppc_md calls

[Benjamin Herrenschmidt]

On Mon, 2005-01-17 at 14:10 -0700, Mark A. Greer wrote:
> All,
> 
> I have a platform with an i2c rtc chip.  Since much of the code for an 
> rtc driver is already in drivers/char/genrtc.c, I would like to reuse 
> that code and directly implement get_rtc_time(), et. al. in the rtc 
> driver.  The problem is that include/asm-ppc/rtc.h assumes that 
> get_rtc_time(), et. al. should be mapped to ppc_md.get_rtc_time() et. 
> al.  To work around this, I made an option to turn off that assumption.  
> The patch is included.
> 
> There are 2 reasons to not use the ppc_md.get_rtc_time() et. al. interfaces:
> 1) They are called before the i2c driver is initialized and even loaded 
> if its a module.
> 2) Its ppc-specific.  Implementing get_rtc_time() et. al. directly makes 
> it generic across all architectures.
> 
> Is there a better way to do this?

The patch is going backward since it disables the ability to have
runtime selection of the RTC chip.

</me deletes some sarcastic comments about embedded companies not
caring about anything but gross hacks for every board>

Re: [RFC] Option to disable mapping genrtc calls to ppc_md calls

[Benjamin Herrenschmidt]

On Tue, 2005-01-18 at 11:25 -0500, Dan Malek wrote:
> On Jan 18, 2005, at 11:15 AM, Tom Rini wrote:
> 
> > On Mon, Jan 17, 2005 at 02:10:00PM -0700, Mark A. Greer wrote:
> 
> >> There are 2 reasons to not use the ppc_md.get_rtc_time() et. al. 
> >> interfaces:
> >> 1) They are called before the i2c driver is initialized and even 
> >> loaded
> >> if its a module.
> 
> There are three reasons.  You don't want to use an I2c rtc clock at
> all in these functions because they get can get called from the
> clock interrupt to update the time in the rtc.  If it does happen to 
> work,
> it creates long latencies in the timer interrupt.  If the i2c requires 
> an
> interrupt, they system will crash or hang.

I have the same problem and I think the solution is to fix the clock
interrupt to not do this at interrupt time, but delay to a work queue
instead.

> A system using an I2C RTC should find some way to access the
> clock from application space as a standard I2C device and manage
> time/clock from the application, not from the kernel.

gack ?

Ben.

Re: [RFC] Option to disable mapping genrtc calls to ppc_md calls

[Tom Rini]

On Thu, Jan 20, 2005 at 01:52:51PM -0700, Mark A. Greer wrote:
> Tom Rini wrote:
> 
> >
> >I guess the problem I have is you're not providing a generic solution
> >(it won't work on PPC_MULTIPLATFORM), you're providing a different hook
> >for your board(s) :)
> 
> Tom, again, [I think that] you are thinking the patch I submitted is 
> trying to solve something that it isn't.  I think we both agree that the 
> current rtc architecture (or lack thereof) needs to be improved.  And 
> maybe the ARM way is the way to go.  That's fine but that's not what the 
> patch I submitted is trying to do.

So you're saying that using the hook directly isn't a different hook?  I
suppose we can agree to disagree, since it's not relevant really :)

> The patch I submittted is only trying to be the most generic it can be 
> with the rtc architecture that is there today.  The patch will work on 
> any architecture, even on PPC/MULTIPLATFORM.  It is not only for my 

It can't work on PPC/MULTIPLATFORM as that's a kernel with multiple rtc
chips in one binary (pmac, todc for prep, chrp?).

> >So until we get around to stealing arch/arm/common/rtctime.c, I'd rather
> >you used something like:
> >- arch/ppc/Kconfig (or where ever you add the board bits)
> > config PPC_DIFFERENT_GENRTC_HOOKS
> > default y if BOARD_A || BOARD_B
> 
> Why?  There is no reason do add in the 'if BOARD_A' stuff.  I've added a 
> simple option, if you want to use the m41t00 rtc--this is what I'm doing 
> this for--select i2c and the correct i2c algo/adapter, select the m41t00 
> i2c client, and [if on ppc] select the option to turn off 
> include/asm-ppc/rtc.h.  If its too hard to remember, it can be easily 
> captured in a arch/ppc/configs/xxx_defconfig file.  :)

Why ask the user for something you know he wants?  If they've selected
GENRTC and are using the board, they'll probably want GENRTC to work.
It's exactly what you described except the user won't have to know about
anything going on.

-- 
Tom Rini
http://gate.crashing.org/~trini/

Re: [RFC] Option to disable mapping genrtc calls to ppc_md calls

[Mark A. Greer]

Tom Rini wrote:

>So you're saying that using the hook directly isn't a different hook?  I
>suppose we can agree to disagree, since it's not relevant really :)
>

Well, I could have not used genrtc at all and duplicated the code like 
others have (e.g., ds1302 and ip27-rtc.c) but I chose to reuse code 
that's already there and debugged.  Chosing to interface to genrtc 
instead of ppc_md.xxx is chosing to have it work on a wider variety of 
platforms over flexibility within one particular type of platform.  Pros 
and cons to each.

>It can't work on PPC/MULTIPLATFORM as that's a kernel with multiple rtc
>chips in one binary (pmac, todc for prep, chrp?).
>

The point I was making is there there is nothing precluding a kernel 
that runs on several boards from using it as long as that's the rtc chip 
on all the platforms  Its already clear that if there are different rtc 
chips, it won't work.

>
>Why ask the user for something you know he wants?  If they've selected
>GENRTC and are using the board, they'll probably want GENRTC to work.
>It's exactly what you described except the user won't have to know about
>anything going on.
>

If that's the way you want to go then it should depend on having the rtc 
"driver" selected and rtc.h should look something like:

#if defined(CONFIG_I2C_<rtc_driver_a>) || defined(CONFIG_<rtc_driver_b> ...

For now, it would only have to be #ifdef CONFIG_xxx_M41T00

That's fine with me.

Re: [RFC] Option to disable mapping genrtc calls to ppc_md calls

[Tom Rini]

On Thu, Jan 20, 2005 at 04:21:14PM -0700, Mark A. Greer wrote:
> Tom Rini wrote:
> 
> >Why ask the user for something you know he wants?  If they've selected
> >GENRTC and are using the board, they'll probably want GENRTC to work.
> >It's exactly what you described except the user won't have to know about
> >anything going on.
> 
> If that's the way you want to go then it should depend on having the rtc 
> "driver" selected and rtc.h should look something like:

I don't know how we talked past eachother, but it would look exactly
like it did in your patch.  The only thing is that you don't ask the
user, you make use of what the user told you already.  This is a nice
new feature of the 2.6 kconfig system.  This is what I'm asking you to
change from your first patch.

-- 
Tom Rini
http://gate.crashing.org/~trini/

Re: [RFC] Option to disable mapping genrtc calls to ppc_md calls

[Mark A. Greer]

Benjamin Herrenschmidt wrote:

>
>The patch is going backward since it disables the ability to have
>runtime selection of the RTC chip.
>

Its only "backwards" from a narrow PPC point of view.  Without a major 
rewrite of the rtc infrastructure, these are the choices:

a) Make a ppc-only solution using ppc_md.xxx.
b) Make a generic solution either by writing an entire driver 
duplicating code that's in genrtc (e.g., ds1302) or interface to the 
bottom of genrtc (e.g., my rtc code).  The only limitation is that you 
can't select different rtc chips at runtime.

Choosing a) give you more flexibility within PPC but is PPC only; 
choosing b) is generic but assumes its the only rtc chip that will be 
used by whatever kernel binary its put in.

I chose b) and to reuse the genrtc code.  In a sane world, reusing code 
is considered a good thing...

Its obvious that you and Tom prefer a).  That's fine but if I switch to 
a), I know the first comment I'll get when I post the driver to lmkl 
will be, "Why would you make this ppc-specific when you could have made 
it generic?"  Will you and Tom then defend that decision for me?

Also, this is not board-specific as you and Tom have tried to suggest.  
Assuming I change the #ifdef in rtc.h to remove the option as I think 
Tom and I are agreeing upon, you select the i2c algo/adapter, the 
i2c/rtc client and its there.  Where are the "gross hacks for every board"?

Mark

Re: [RFC] Option to disable mapping genrtc calls to ppc_md calls

[Mark A. Greer]

Tom Rini wrote:

>I don't know how we talked past eachother, but it would look exactly
>like it did in your patch.  The only thing is that you don't ask the
>user, you make use of what the user told you already.  This is a nice
>new feature of the 2.6 kconfig system.  This is what I'm asking you to
>change from your first patch.
>

I don't know how we talked past each other either but as I wrote that 
last email, I realized that was what you had already suggested.  My bad 
& my apologies.

Mark

Re: [RFC] Option to disable mapping genrtc calls to ppc_md calls

[Benjamin Herrenschmidt]

On Thu, 2005-01-20 at 16:54 -0700, Mark A. Greer wrote:

> Choosing a) give you more flexibility within PPC but is PPC only; 
> choosing b) is generic but assumes its the only rtc chip that will be 
> used by whatever kernel binary its put in.
> 
> I chose b) and to reuse the genrtc code.  In a sane world, reusing code 
> is considered a good thing...
> 
> Its obvious that you and Tom prefer a).  That's fine but if I switch to 
> a), I know the first comment I'll get when I post the driver to lmkl 
> will be, "Why would you make this ppc-specific when you could have made 
> it generic?"  Will you and Tom then defend that decision for me?
> 
> Also, this is not board-specific as you and Tom have tried to suggest.  
> Assuming I change the #ifdef in rtc.h to remove the option as I think 
> Tom and I are agreeing upon, you select the i2c algo/adapter, the 
> i2c/rtc client and its there.  Where are the "gross hacks for every board"?

Because it makes things like CONFIG_PPC_MULTIPLATFORM impossible, which
means you end up with a CONFIG_* mess.

I consider that more important than re-using code.

In any case, as I wrote, the proper solution is to update genrtc to
define rtc_ops so that you get both a) and b), it shouldn't be hard to
update the archs using it.

Ben.

Re: [RFC] Option to disable mapping genrtc calls to ppc_md calls

[Mark A. Greer]

Benjamin Herrenschmidt wrote:

>On Thu, 2005-01-20 at 16:54 -0700, Mark A. Greer wrote:
>
>  
>
>>Choosing a) give you more flexibility within PPC but is PPC only; 
>>choosing b) is generic but assumes its the only rtc chip that will be 
>>used by whatever kernel binary its put in.
>>
>>I chose b) and to reuse the genrtc code.  In a sane world, reusing code 
>>is considered a good thing...
>>
>>Its obvious that you and Tom prefer a).  That's fine but if I switch to 
>>a), I know the first comment I'll get when I post the driver to lmkl 
>>will be, "Why would you make this ppc-specific when you could have made 
>>it generic?"  Will you and Tom then defend that decision for me?
>>
>>Also, this is not board-specific as you and Tom have tried to suggest.  
>>Assuming I change the #ifdef in rtc.h to remove the option as I think 
>>Tom and I are agreeing upon, you select the i2c algo/adapter, the 
>>i2c/rtc client and its there.  Where are the "gross hacks for every board"?
>>    
>>
>
>Because it makes things like CONFIG_PPC_MULTIPLATFORM impossible, which
>means you end up with a CONFIG_* mess.
>

/me feels the anger...   :)

>
>I consider that more important than re-using code.
>

Okay, it shall be so.

>
>In any case, as I wrote, the proper solution is to update genrtc to
>define rtc_ops so that you get both a) and b), it shouldn't be hard to
>update the archs using it.
>  
>

Yes but as I wrote, I don't have time right now.

Mark

Re: [RFC] Option to disable mapping genrtc calls to ppc_md calls

[Benjamin Herrenschmidt]

On Thu, 2005-01-20 at 17:09 -0700, Mark A. Greer wrote:

> Yes but as I wrote, I don't have time right now.

 ... which is exactly my rant ... embedded companies never have time
to do the right thing...

Ben.

Re: [RFC] Option to disable mapping genrtc calls to ppc_md calls

[Geert Uytterhoeven]

On Fri, 21 Jan 2005, Benjamin Herrenschmidt wrote:
> On Thu, 2005-01-20 at 17:09 -0700, Mark A. Greer wrote:
> > Yes but as I wrote, I don't have time right now.
> 
>  ... which is exactly my rant ... embedded companies never have time
> to do the right thing...

Because they have strict (paid for) deadlines, unlike the `we do it for
fun'-crowd led by Linus ;-)

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

Re: [RFC] Option to disable mapping genrtc calls to ppc_md calls

[Christoph Hellwig]

On Thu, Jan 20, 2005 at 05:09:45PM -0700, Mark A. Greer wrote:
> >In any case, as I wrote, the proper solution is to update genrtc to
> >define rtc_ops so that you get both a) and b), it shouldn't be hard to
> >update the archs using it.
> > 
> >
> 
> Yes but as I wrote, I don't have time right now.

You could have easily implemented it in the time you wasted arguing
here.

Re: [RFC] Option to disable mapping genrtc calls to ppc_md calls

[Corey Minyard]

Geert Uytterhoeven wrote:

>On Fri, 21 Jan 2005, Benjamin Herrenschmidt wrote:
>  
>
>>On Thu, 2005-01-20 at 17:09 -0700, Mark A. Greer wrote:
>>    
>>
>>>Yes but as I wrote, I don't have time right now.
>>>      
>>>
>> ... which is exactly my rant ... embedded companies never have time
>>to do the right thing...
>>    
>>
>
>Because they have strict (paid for) deadlines, unlike the `we do it for
>fun'-crowd led by Linus ;-)
>  
>
[begin soapbox]

I disagree with this attitude.  I believe there is a balance to be 
struck between doing
things right and meeting deadlines, and I believe the embedded hardware 
companies
are way too far off on the meeting deadlines side of things.  From what 
I can tell, they
tend to be penny-wise and pound-foolish.  (They do things that save them 
a little bit
of money in one area and cost a lot in another area.)

This is, of course, a generalization.  I see some good things happening 
in some areas.

IMHO, this is due to the fact that these are mostly run by hardware 
engineers
and software is an afterthought.  For instance, I have dealt with one 
company who
used a non-standard interface where a standard one was available.  They 
probably
saved 1-2 dollars per board.  This change probably cost $150,000 in 
software costs.
There is no way they will sell close to 75,000 boards.  Penny-wise, 
pound-foolish.
And they have 10-20 years of support for this non-standard hack.

The more consistent you make things, the lower the software cost.  When 
you follow
standards, it lowers software costs.  Software is *expensive* to create; 
the more
you can reuse what is there, the better off you are.  It seems to me 
that the PPC
hardware vendors have spent 10s if not 100s of millions of dollars in 
software costs
that were really kind of pointless.  If things were more standard and 
consistent,
things would work just as well (and almost certainly better) and the 
cost would be
much lower.  Then us software folks who work on embedded linux could work on
making things better instead of chasing new board ports.

As I mentioned, there is a balance here.  If you are selling, 10 million 
units, saving a
dollar per unit at the cost of $150,000 in software is well worth it.  
If you can get a 50%
improvement in performance, it's probably worth it.  But to gain no 
benefit, cost
yourself a lot of money, and reduce your reliability (new software is 
generally less
reliable than old), well, that sounds silly to me.

[end soapbox]

After saying this, I don't know how to fix the PPC world.  But every 
customization
that requires software changes is one more reason for people to use x86 
(where
new boards "just work" but can be tuned for performance) and not PPC (where
new boards are pain and suffering to get working and there is no time to 
tune them
for performance).

-Corey

Re: [RFC] Option to disable mapping genrtc calls to ppc_md calls

[Benjamin Herrenschmidt]

On Fri, 2005-01-21 at 10:14 +0100, Geert Uytterhoeven wrote:
> On Fri, 21 Jan 2005, Benjamin Herrenschmidt wrote:
> > On Thu, 2005-01-20 at 17:09 -0700, Mark A. Greer wrote:
> > > Yes but as I wrote, I don't have time right now.
> > 
> >  ... which is exactly my rant ... embedded companies never have time
> > to do the right thing...
> 
> Because they have strict (paid for) deadlines, unlike the `we do it for
> fun'-crowd led by Linus ;-)

Then they should include the time of doing stuffs properly in the
initial sizing  ...