Re: [PATCH 7/8] x86/mrst: add vrtc driver which serves as a wall clock device

[Feng Tang <feng.tang@intel.com>]

Hi Thomas,

Thanks for the great comments!


On Tue, 11 May 2010 22:57:44 +0800
Thomas Gleixner <tglx@linutronix.de> wrote:

> > +extern int vrtc_set_mmss(unsigned long nowtime);
> > +extern void vrtc_set_base(void __iomem *base);
> > +
> > +#define MRST_VRTC_PGOFFSET  (0xc00)
> > +
> > +#else
> 
>   Errm. That's a MRST specific header and nothing outside of MRST is
>   using it. So why the #ifdef CONFIG_X86_MRST and the inline functions
>   in the #else path ?

My bad not mentioning there is another rtc-mrst.c which will sit in drivers/rtc,
it will use some of the functions listed here. It will be posted later.

vrtc.c/rtc-mrst.c is similar with the rtc.c/rtc-cmos.c in general x86 PCs, as
drivers/rtc may not always be enabled in kernel, vrtc.c need sit in arch/x86
to provide the get/set_time service, while rtc-mrst.c will serve general rtc
subsystem

> >  void __init mrst_rtc_init(void)
> >  {
> > +	unsigned long rtc_paddr;
> > +	void __iomem *virt_base;
> > +
> >  	sfi_table_parse(SFI_SIG_MRTC, NULL, NULL, sfi_parse_mrtc);
> > +	if (!sfi_mrtc_num)
> > +		return;
> > +
> > +	rtc_paddr = sfi_mrtc_array[0].phys_addr;
> > +
> > +	/* vRTC's register address may not be page aligned */
> > +	set_fixmap_nocache(FIX_LNW_VRTC, rtc_paddr);
> 
> Why do we need a fixmap for that ? There is no need to setup RTC that
> early. The first call is from timekeeping_init() 

Actually when to init the vrtc register is a big problem for me, vrtc
need be inited before timekeeping_init(), and I thought better to put it
somewhere in setup_arch(), as it is architecture specific, and ioremap
is not working at that time. Also that's the reason I created a new
wallclock_init func for x86_platforms, I could not find a better way
to do the vrtc init.

> 
> Also this RTC init code should be in vrtc.c
I agree I should move this init code to vrtc.c, but still think it should
be called in the setup_arch() than in start_kernel()

> 
> > +
> > +static unsigned char __iomem *vrtc_virt_base;
> > +
> > +void vrtc_set_base(void __iomem *base)
> > +{
> > +	vrtc_virt_base = base;
> > +}
> > +
> > +unsigned char vrtc_cmos_read(unsigned char reg)
> > +{
> > +	unsigned char retval;
> > +
> > +	/* vRTC's registers range from 0x0 to 0xD */
> > +	if (reg > 0xd || !vrtc_virt_base)
> > +		return 0xff;
> > +
> > +	lock_cmos_prefix(reg);
> 
>   This lock_cmos magic should just die. I have no idea why something
>   wants or wanted to access the RTC from an NMI.

I will try to reuse the rtc_lock defined in rtc.c whose get/set_time
service won't be called with vrtc's at the same time.

> > +	/* vRTC YEAR reg contains the offset to 1960 */
> > +	year += 1960;
> > +
> > +	printk(KERN_INFO "vRTC: sec: %d min: %d hour: %d day: %d "
> > +		"mon: %d year: %d\n", sec, min, hour, mday, mon,
> > year);
> 
>   Please remove the debug noise

Will make it a pr_debug.

Thanks,
Feng