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: Thomas Gleixner on 17 May 2010 05:20 Feng, [ Cc'ed John ] On Wed, 12 May 2010, Feng Tang wrote: > > > 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. There is no particular reason why we need to read it in timekeeping_init(). Nothing in the kernel needs the correct wall time at that point. So we can safely move the setting of xtime to rtc wall clock time to a separate timekeeping_late_init() function. John ??? > > 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() I do not :) > > > + 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. Please don't create artifical dependencies. Use a separate vrtc_lock to serialize the access to vrtc. 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 18 May 2010 02:20 Hi Thomas, On Mon, 17 May 2010 17:15:55 +0800 Thomas Gleixner <tglx(a)linutronix.de> wrote: > > > > 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. > > There is no particular reason why we need to read it in > timekeeping_init(). Nothing in the kernel needs the correct wall time > at that point. So we can safely move the setting of xtime to rtc wall > clock time to a separate timekeeping_late_init() function. > > John ??? > Yeah, good suggestion, if xtime init is moved to a later time in kernel init flow, then vrtc's init function can be set a arch_initcall() > > > > + 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. > > Please don't create artifical dependencies. Use a separate vrtc_lock > to serialize the access to vrtc. I just checked the code, when wall clock's get/set_time service is called, it is always protected by rtc_lock(code in arch/x86/kernel/rtc.c), then no need to add the lock for each individual register read/write operation. I will submit a v2 vrtc patch. Thanks, Feng > > 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: Thomas Gleixner on 18 May 2010 03:40 On Tue, 18 May 2010, Feng Tang wrote: > Hi Thomas, > > > On Mon, 17 May 2010 17:15:55 +0800 > Thomas Gleixner <tglx(a)linutronix.de> wrote: > > > > > > > > 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. > > > > There is no particular reason why we need to read it in > > timekeeping_init(). Nothing in the kernel needs the correct wall time > > at that point. So we can safely move the setting of xtime to rtc wall > > clock time to a separate timekeeping_late_init() function. > > > > John ??? > > > Yeah, good suggestion, if xtime init is moved to a later time in kernel > init flow, then vrtc's init function can be set a arch_initcall() John was away yesterday, so we have to wait for his answer, but I don't expect a nono from him. > > > > > + 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. > > > > Please don't create artifical dependencies. Use a separate vrtc_lock > > to serialize the access to vrtc. > I just checked the code, when wall clock's get/set_time service is called, > it is always protected by rtc_lock(code in arch/x86/kernel/rtc.c), then > no need to add the lock for each individual register read/write operation. Grr. Yes, missed that the code is called under rtc_lock already. So you can drop the locking in your code completely. 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: john stultz on 18 May 2010 16:50 On Mon, 2010-05-17 at 11:15 +0200, Thomas Gleixner wrote: > > > There is no particular reason why we need to read it in > timekeeping_init(). Nothing in the kernel needs the correct wall time > at that point. So we can safely move the setting of xtime to rtc wall > clock time to a separate timekeeping_late_init() function. > > John ??? No big objections here. Still would like to keep the amount of time that the kernel is up without xtime being initialized to a minimum. However the generic RTC code already have this issue since some of them require interrupts to be enabled to do a read, so pushing it off into a _late_init() function is probably just a short term fix until we figure out how to get the generic RTC code working better with the timekeeping code. Does the delayed init required by vrtc cause any trouble with suspend/resume? thanks -john -- 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 18 May 2010 17:10
On Tue, 18 May 2010, john stultz wrote: > On Mon, 2010-05-17 at 11:15 +0200, Thomas Gleixner wrote: > > > > > > There is no particular reason why we need to read it in > > timekeeping_init(). Nothing in the kernel needs the correct wall time > > at that point. So we can safely move the setting of xtime to rtc wall > > clock time to a separate timekeeping_late_init() function. > > > > John ??? > > No big objections here. Still would like to keep the amount of time that > the kernel is up without xtime being initialized to a minimum. However Why? Nothing _IS_ depending on xtime at this point. We just should have it ready when we mount the first file system, but even that is not a big issue aside of some stupid warnings. > the generic RTC code already have this issue since some of them require > interrupts to be enabled to do a read, so pushing it off into a > _late_init() function is probably just a short term fix until we figure > out how to get the generic RTC code working better with the timekeeping > code. Well, you need to wait until the driver is loaded and the hardware accessible which might take some time when I2C, SPI or similar stuff is involved. No way to get this stuff accesible early. So really the question is, why do we want wall time initialized early? All we care about until we hit user space is clock monotonic and working timekeeping in general. NTP is not an issue either before we hit user space. So what's your concern ? > Does the delayed init required by vrtc cause any trouble with > suspend/resume? No, suspend/resume does not go through that early code. It has everything setup (mappings, etc) 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/ |