Re: [PATCH] m68k: Remove read_persistent_clock()

[Greg Ungerer]

On 23/04/18 21:47, Arnd Bergmann wrote:
> On Mon, Apr 23, 2018 at 11:47 AM, Geert Uytterhoeven
> <geert@xxxxxxxxxxxxxx> wrote:
> > Hi Arnd,
> >
> > On Mon, Apr 23, 2018 at 11:28 AM, Arnd Bergmann <arnd@xxxxxxxx> wrote:
> > > On Mon, Apr 23, 2018 at 11:07 AM, Geert Uytterhoeven
> > > <geert@xxxxxxxxxxxxxx> wrote:
> > > > On Fri, Apr 20, 2018 at 5:22 PM, Arnd Bergmann <arnd@xxxxxxxx> wrote:
> > > > > On Thu, Apr 19, 2018 at 8:22 AM, Baolin Wang <baolin.wang@xxxxxxxxxx> wrote:
> > > > > > The read_persistent_clock() uses a timespec, which is not year 2038 safe
> > > > > > on 32bit systems. Moreover on m68k architecture, we have implemented generic
> > > > > > RTC drivers that can be used to compensate the system suspend time. So
> > > > > > we can remove the obsolete read_persistent_clock().
> > > > > >
> > > > > > Signed-off-by: Baolin Wang <baolin.wang@xxxxxxxxxx>
> > > > >
> > > > > I'm not sure about this one: it's still possible to turn off
> > > > > CONFIG_RTC_DRV_GENERIC
> > > > > on M68KCLASSIC, and in that case we still need a read_persistent_clock64()
> > > > > implementation. Or we use your patch but make CONFIG_RTC_DRV_GENERIC
> > > > > mandatory.
> > > >
> > > > Typically (in the defconfigs) CONFIG_RTC_DRV_GENERIC is either "m",
> > > > or not set.
> > > >
> > > > And in both cases, a platform-specific RTC class driver may or may not be
> > > > builtin or loaded. We have them for some Amigas (RTC_DRV_MSM6242 or
> > > > RTC_DRV_RP5C01).
> > > >
> > > > I've never been an expert of timekeeping code...
> > >
> > > Some extra background on this:
> > >
> > > read_persistent_clock64/read_persistent_clock is primarily needed when you
> > > have a real time source that is better than the one exposed in the RTC class
> > > driver.  For platforms doing suspend/resume, the timekeeping code will
> > > first try calling read_persistent_clock64() but fall back to the RTC
> > > if that fails.
> > > On only a few architectures like m68k, read_persistent_clock64() falls
> > > back to reading the RTC hardware, which means it will still work even if
> > > the RTC_DRV_GENERIC driver is not loaded. Removing that code will
> > > still work correctly as long as the generic RTC driver does get loaded
> > > before suspend. On platforms without suspend/resume, none of this matters.
> >
> > M68k-with-MMU[*] does not support suspend/resume.
> >
> > [*] Probably the PM dependency should be updated for Coldfire with MMU?
>
> Ok, so for the suspend case on m68k, can can totally ignore
> read_persistent_clock64(): classic doesn't have suspend and
> coldfire doesn't implement mach_hwclock() callbacks, right?

Yep, that is right, no ColdFire platforms use it.

Regards
Greg


> For reference, this is the set of machines implementing mach_hwclk:
>
> arch/m68k/68000/m68328.c:  mach_hwclk = m68328_hwclk;
> arch/m68k/68000/m68EZ328.c:  mach_hwclk = m68328_hwclk;
> arch/m68k/68000/m68VZ328.c:     mach_hwclk = m68328_hwclk;
> arch/m68k/apollo/config.c:      mach_hwclk           = dn_dummy_hwclk; /* */
> arch/m68k/atari/config.c:               mach_hwclk = atari_tt_hwclk;
> arch/m68k/atari/config.c:               mach_hwclk = atari_mste_hwclk;
> arch/m68k/bvme6000/config.c:    mach_hwclk           = bvme6000_hwclk;
> arch/m68k/hp300/config.c:       mach_hwclk           = hp300_hwclk;
> arch/m68k/mac/config.c: mach_hwclk = mac_hwclk;
> arch/m68k/mvme147/config.c:     mach_hwclk              = mvme147_hwclk;
> arch/m68k/mvme16x/config.c:    mach_hwclk           = mvme16x_hwclk;
> arch/m68k/q40/config.c: mach_hwclk = q40_hwclk;
> arch/m68k/sun3/config.c:        mach_hwclk           =  sun3_hwclk;
> arch/m68k/sun3x/config.c:       mach_hwclk           = sun3x_hwclk;
>
> > > The other user of read_persistent_clock() is to set the initial time at boot.
> > > This again can be done by the RTC subsystem if the correct RTC driver
> > > is built-in and CONFIG_RTC_HCTOSYS is set. Alexandre is planning
> > > to remove that option and instead force early user space to load the
> > > RTC driver and then sync it manually using the sysfs knob for it.
> > >
> > > If you have ancient user space that doesn't do this, you might still
> > > rely on read_persistent_clock() to set the boot time.
> >
> > Yeah, we have some ancient userspace (old ramdisk from just after the
> > a.out to ELF switch ;-), which worked fine last time I tried ;-)
> >
> > But this should not be considered a dependency, as most people will
> > run e.g. Debian/ports, which I assume is a modern userspace.
>
> Ok. In that case, a possible cleanup for the old time handling
> could be to move each of the hwclk implementations over to use
> their own RTC driver instead of a mach_hwclk function with
> generic_rtc.
>
> It seems that the mach_set_ss and nach_set_mmss
> callbacks can simply get removed already, they are
> never called. Similarly, the mach_get_rtc_pll() and
> mach_get_rtc_pll() callbacks are only used on q40, and
> could be removed after changing the q40 over to using
> its own RTC driver, or registering the ioctl operation itself.
>
>        Arnd
> --
> To unsubscribe from this list: send the line "unsubscribe linux-m68k" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html