Re: [RFC][PATCH 3/3] Try to convert non-trivial clocksources to clocksource_register_hz

[Jim Cromie]

On Fri, Apr 30, 2010 at 9:36 PM, John Stultz <johnstul@xxxxxxxxxx> wrote:
> NOT FOR INCLUSION!
> NOT FOR INCLUSION!
>
> I've already gone through and converted the rest of the clocksources
> to use clocksource_register_hz, and I'll be hopefully pushing those
> to arch maintainers for 2.6.36-2.6.37.
>
> However, in going through all the clocksources, I hit a few
> non-trivial conversions and wanted to bring them up on the list
> early so they can be handled soon.
>
> The following patch tries to convert the non-trival clocksources
> to use clocksource_register_hz/khz. It is likely broken. I will
> need arch maintainer help to figure out the best way to resovle
> these clocksources.
>
> This patch requires the previous "Add clocksource_register_hz/khz
> interface" patch to build.
>
> So any help maintainers can provide in finding solutions to
> break the mult/shift assumptions in the arch code would be
> greatly appreciated!
>
> thanks
> -john
>
> NOT FOR INCLUSION!
> NOT FOR INCLUSION!
>  drivers/clocksource/scx200_hrt.c    |   19 ++++++-------------
>  drivers/clocksource/sh_cmt.c        |    4 ++--
>  drivers/clocksource/sh_tmu.c        |    4 ++--
>  kernel/time/jiffies.c               |    3 ++-
>  7 files changed, 35 insertions(+), 29 deletions(-)
>

> diff --git a/drivers/clocksource/scx200_hrt.c b/drivers/clocksource/scx200_hrt.c
> index 27f4d96..24f884c 100644
> --- a/drivers/clocksource/scx200_hrt.c
> +++ b/drivers/clocksource/scx200_hrt.c
> @@ -49,9 +49,6 @@ static cycle_t read_hrt(struct clocksource *cs)
>        return (cycle_t) inl(scx200_cb_base + SCx200_TIMER_OFFSET);
>  }
>
> -#define HRT_SHIFT_1    22
> -#define HRT_SHIFT_27   26
> -
>  static struct clocksource cs_hrt = {
>        .name           = "scx200_hrt",
>        .rating         = 250,
> @@ -63,6 +60,7 @@ static struct clocksource cs_hrt = {
>
>  static int __init init_hrt_clocksource(void)
>  {
> +       u32 freq;
>        /* Make sure scx200 has initialized the configuration block */
>        if (!scx200_cb_present())
>                return -ENODEV;
> @@ -79,19 +77,14 @@ static int __init init_hrt_clocksource(void)
>        outb(HR_TMEN | (mhz27 ? HR_TMCLKSEL : 0),
>             scx200_cb_base + SCx200_TMCNFG_OFFSET);
>
> -       if (mhz27) {
> -               cs_hrt.shift = HRT_SHIFT_27;
> -               cs_hrt.mult = clocksource_hz2mult((HRT_FREQ + ppm) * 27,
> -                                                 cs_hrt.shift);
> -       } else {
> -               cs_hrt.shift = HRT_SHIFT_1;
> -               cs_hrt.mult = clocksource_hz2mult(HRT_FREQ + ppm,
> -                                                 cs_hrt.shift);
> -       }
> +       freq = (HRT_FREQ + ppm);
> +       if (mhz27)
> +               freq *= 27;
> +
>        printk(KERN_INFO "enabling scx200 high-res timer (%s MHz +%d ppm)\n",
>                mhz27 ? "27":"1", ppm);
>
> -       return clocksource_register(&cs_hrt);
> +       return clocksource_register_hz(&cs_hrt, freq);
>  }
>
>  module_init(init_hrt_clocksource);


hi John,

On casual inspection, this looks good, however
I wont be able to try it on the hardware til early June earliest.

I have a few qs, (idle musings really)

- scx200_hrt cannot be rmmod'd, due iirc to the clocksource design.
This is not a problem, but I wonder if it might be added later, and what
it might mean to the drivers (in a new module_exit())

- HRT_SHIFT_1/_27 macros are removed, and now calculated in clocksource code.
I chose a 1:16 shift ratio for the 1:27 input clocks.  I suppose I could have
used 1:32, I dont recall thinking of it or trying it then.
What would/should your new code do ?

- when writing scx200_hrt, I chose to adjust by ppm, not +/- hz
partly cuz it was easier to describe in a 1-line mod-desc, and partly
cuz 1/27ppm vernier-knob is overkill on a $0.25 crystal.
The new API name (with _hz suffix) suggests that I could change
the mod-param-name, and add it in after the 1:27 multiplier.

<OT - out of scope in this thread>

on 5/3, slashdot had a synopsis of this item:
http://queue.acm.org/detail.cfm?id=1773943
It proposes a different split of duty between kernel and NTP daemon,
but oddly doesnt mention PTP.
Does this or PTP make any sense in Linux ?

thanks
-jimc
--
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/