x86/mrst: add vrtc driver which serves as a wall clock device
in [Kernel]
|
Prev: [tip:perf/core] perf, x86: Pass enable bit mask to __x86_pmu_enable_event()
Next: [PATCH] trivial: inode uses a mutex instead of a semaphore.
From: H. Peter Anvin on 10 May 2010 14:30 On 05/10/2010 02:17 AM, Feng Tang wrote: > > Thank you all for the comments. please review this follow-on patch. > > - Feng That doesn't move it to common code, though. I'd rather see existing common style used, then merged (centralized) and *then* the style corrected. -hpa -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Feng Tang on 10 May 2010 22:20 On Tue, 11 May 2010 02:22:15 +0800 "H. Peter Anvin" <hpa(a)zytor.com> wrote: > On 05/10/2010 02:17 AM, Feng Tang wrote: > > > > Thank you all for the comments. please review this follow-on patch. > > > > - Feng > > That doesn't move it to common code, though. I'd rather see existing > common style used, then merged (centralized) and *then* the style > corrected. > > -hpa Hi Peter, The reason I didn't move it to rtc common code is, this vrtc.c sits in arch/x86/kernel/ and better not to depend on drivers/rtc, as drivers/rtc may not be always enabled in kernel configuration. I also have another general driver for vrtc which will be in drivers/rtc/, just like general x86 kernel which has a rtc.c in arch/ and a rtc-cmos.c in drivers/rtc, I will clean my code up and try to move these funcs to a common code. Thanks, Feng -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Thomas Gleixner on 11 May 2010 11:00 On Fri, 7 May 2010, Jacob Pan wrote: > --- /dev/null > +++ b/arch/x86/include/asm/vrtc.h > @@ -0,0 +1,27 @@ > +#ifndef _MRST_VRTC_H > +#define _MRST_VRTC_H > + > +#ifdef CONFIG_X86_MRST > +extern unsigned char vrtc_cmos_read(unsigned char reg); > +extern void vrtc_cmos_write(unsigned char val, unsigned char reg); > +extern unsigned long vrtc_get_time(void); > +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 ? > +static inline unsigned char vrtc_cmos_read(unsigned char reg) > +{ > + return 0xff; > +} > + > /** > * the clockevent devices on Moorestown/Medfield can be APBT or LAPIC clock, > @@ -268,7 +269,24 @@ void __init mrst_time_init(void) > > 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() Also this RTC init code should be in vrtc.c > + > +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. > + retval = __raw_readb(vrtc_virt_base + (reg << 2)); > + lock_cmos_suffix(reg); > + return retval; > +} > +EXPORT_SYMBOL(vrtc_cmos_read); > + > +void vrtc_cmos_write(unsigned char val, unsigned char reg) > +{ > + if (reg > 0xd || !vrtc_virt_base) > + return; > + > + lock_cmos_prefix(reg); > + __raw_writeb(val, vrtc_virt_base + (reg << 2)); > + lock_cmos_suffix(reg); > +} > +EXPORT_SYMBOL(vrtc_cmos_write); > + > +unsigned long vrtc_get_time(void) > +{ > + u8 sec, min, hour, mday, mon; > + u32 year; > + > + while ((vrtc_cmos_read(RTC_FREQ_SELECT) & RTC_UIP)) > + cpu_relax(); > + > + sec = vrtc_cmos_read(RTC_SECONDS); > + min = vrtc_cmos_read(RTC_MINUTES); > + hour = vrtc_cmos_read(RTC_HOURS); > + mday = vrtc_cmos_read(RTC_DAY_OF_MONTH); > + mon = vrtc_cmos_read(RTC_MONTH); > + year = vrtc_cmos_read(RTC_YEAR); > + > + /* 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 Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Feng Tang on 11 May 2010 22:30 Hi Thomas, Thanks for the great comments! On Tue, 11 May 2010 22:57:44 +0800 Thomas Gleixner <tglx(a)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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Thomas Gleixner on 16 May 2010 18:50
On Fri, 14 May 2010, Jacob Pan wrote: > From: Feng Tang <feng.tang(a)intel.com> > > Moorestown platform doesn't have a m146818 RTC device like traditional > x86 PC, but a firmware emulated virtual RTC device(vrtc), which provides > some basic RTC functions like get/set time. vrtc serves as the only > wall clock device on Moorestown platform. NAK for the very same reason as [5/8] Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |