Date:   Mon, 23 Apr 2018 13:47:41 +0200
From:   Arnd Bergmann <arnd@...db.de>
To:     Geert Uytterhoeven <geert@...ux-m68k.org>
Cc:     Baolin Wang <baolin.wang@...aro.org>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Mark Brown <broonie@...nel.org>,
        linux-m68k <linux-m68k@...ts.linux-m68k.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        Finn Thain <fthain@...egraphics.com.au>
Subject: Re: [PATCH] m68k: Remove read_persistent_clock()

On Mon, Apr 23, 2018 at 11:47 AM, Geert Uytterhoeven
<geert@...ux-m68k.org> wrote:
> Hi Arnd,
>
> On Mon, Apr 23, 2018 at 11:28 AM, Arnd Bergmann <arnd@...db.de> wrote:
>> On Mon, Apr 23, 2018 at 11:07 AM, Geert Uytterhoeven
>> <geert@...ux-m68k.org> wrote:
>>> On Fri, Apr 20, 2018 at 5:22 PM, Arnd Bergmann <arnd@...db.de> wrote:
>>>> On Thu, Apr 19, 2018 at 8:22 AM, Baolin Wang <baolin.wang@...aro.org> 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@...aro.org>
>>>>
>>>> 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?

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

Powered by blists - more mailing lists