Re: [PATCH] NTP: Add a CONFIG_RTC_SYSTOHC configuration

[John Stultz]

On 12/12/2012 01:04 PM, Jason Gunthorpe wrote:
> On Wed, Dec 12, 2012 at 11:40:26AM -0800, John Stultz wrote:
>
> > > The option also overrides the call to any platform update_persistent_clock
> > > so that the RTC subsystem completely and generically replaces this
> > > functionality.
> > >
> > > Tested on ARM kirkwood and PPC405
> > So I'm initially mixed on this, as it feels a little redundant (esp
> > given it may override a higher precision arch-specific function).
> > But from the perspective of this being a generic RTC function,
> > instead of an per-arch implementation, it does seem reasonable.
> It isn't really redundant. The kernel still has two RTC subsystems,
> 'class RTC' and various platform specific
> things. update_persisent_clock is the entry for the platform specific
> RTC, while rtc_update_persistent_clock is the entry for class RTC.
>
> The issue is on platforms like PPC where both co-exist. On PPC
> update_persisent_clock just calls through a function pointer
> (set_rtc_time) to the machine description to do whatever that mach
> needs. Currently all the PPC mach's that use class RTC drivers via DTS
> set set_rtc_time to null. Only the machs that implement a non class
> RTC driver provide an implementation.
>
> So it is an either/or case - either rtc_update_persistent_clock works
> or update_persistent_clock does, never both.
Right.  I just want to make sure that we reduce the duplication between 
the two paths (and ensure sane defaults, so users don't have to go 
hunting for the right config for things to work properly). The other 
complication is that in some cases, the arch specific path is much finer 
grained then the RTC layer's second granularity, so we need to ensure we 
prefer the arch specific path in those cases.

> > Hrm. So on architectures that support GENERIC_CMOS_UPDATE already,
> > this creates a duplicated code path that is slightly different. I'd
> > like to avoid this if possible.  Since you're plugging
> > rtc_update_persistent_clock into the GENERIC_CMOS_UPDATE code, we
> > can't just have this option depend on !GENERIC_CMOS_UPDATE.
> It isn't duplicate, we need to keep update_persistent_clock to support
> non-class RTC machines.
>
> I thought about this:
>
>          if (abs(now.tv_nsec - (NSEC_PER_SEC / 2)) <= tick_nsec / 2) {
> 	        fail = -1;
> #ifdef CONFIG_RTC_SYSTOHC
> 	        fail = rtc_update_persistent_clock(now);
> #endif
> #ifdef CONFIG_GENERIC_CMOS_UPDATE
>                  if (fail)
>                           fail = update_persistent_clock(now);
> #endif
>         }
>
> Try the RTC function first, then fall back to the legacy platform
> function if it didn't work.
>
> That seems better to me, do you agree?

I do, although again, in the case where the arch specific implementation 
is "better", we end up losing granularity (s390 is the specific example 
I'm thinking of), since this prefers the RTC implementation over the 
arch specific one.  So maybe I'd suggest switching it so we prefer the 
arch specific one, and then remove the arch specific implementations 
where they are inferior to the RTC one.

> > So this might need a slightly deeper rework?
> > I'd suggest the following:
> > 1) Convert GENERIC_CMOS_UPDATE into SUPPORTS_UPDATE_PERSISTENT_CLOCK.
> > 2) Add RTC_SYSTOHC but make it depend on !SUPPORTS_UPDATE_PERSISTENT_CLOCK
> This would only work for ARM, not PPC..
>
> Ultimately I suspect the clean up needed is to convert more things to
> class rtc drivers and remove update_persistent_clock.
> ppc is pretty close already, and I think ARM is already there.
As long as we have a clear iterative path forward, with a solution for 
the cases where the arch method is actually preferred, I think it sounds 
like a reasonable approach.

thanks
-john
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/