Re: [patch 02/23] GTOD: persistent clock support, core

[David Brownell <david-b@pacbell.net>]

> Implement generic timekeeping suspend/resume accounting by introducing 
> the read_persistent_clock() interface. This is an arch specific 
> function that returns the seconds since the epoch using the arch 
> defined battery backed clock.

I remain unclear what's expected to happen when there IS no such
architcture-defined clock ... but where the system itself still
has one, e.g. a board may access one through I2C or SPI once IRQs
are working normally.

You'll recall that I had pointed out that the drivers/rtc framework
provides CONFIG_RTC_HCTOSYS, which already unifies quite a lot of
the "persistent" clocks in the way you described above, but without
that nasty requirement of working without IRQs enabled.


> +/**
> + * read_persistent_clock -  Return time in seconds from the persistent clock.
> + *
> + * Weak dummy function for arches that do not yet support it.
> + * Returns seconds from epoch using the battery backed persistent clock.
> + * Returns zero if unsupported.
> + *
> + *  XXX - Do be sure to remove it once all arches implement it.

But not all architectures **CAN** support this notion ...


> + */
> +unsigned long __attribute__((weak)) read_persistent_clock(void)
> +{
> +       return 0;
> +}
> +
>
>  /* 
>   * timekeeping_init - Initializes the clocksource and common timekeeping values
>   */
>  void __init timekeeping_init(void)
>  {
>         unsigned long flags;
> +       unsigned long sec = read_persistent_clock();

... and timekeeping_init() is called before I2C or SPI could be used,
since IRQs aren't enabled yet and accessing those busses can't be
done in general without IRQs enabled.


> @@ -774,13 +801,23 @@ static int timekeeping_suspended;
>  static int timekeeping_resume(struct sys_device *dev)
>  {
>         unsigned long flags;
> +       unsigned long now = read_persistent_clock();

Again: sys_device resume() is called with IRQs disabled, which
prevents access to many systems' persistent clocks.  In fact,
after posting this example patch

  http://marc.theaimsgroup.com/?l=linux-kernel&m=115600629813751&w=2

I never heard anything more from you on this issue.  Given this
particular patch (in $SUBJECT) should I assume you're going to
just ignore the issues whereby RTCs ("persistent clocks") can't
always meet the no-IRQs-needed assumptions being made here?  Or
address isssues like using pointer-to-function instead of using
linker tricks?

  http://marc.theaimsgroup.com/?l=linux-kernel&m=115600629825461&w=2

Those class suspend/resume hooks are now merged to kernel.org, by
the way, so that example patch is now pretty much deployable.

- Dave