Re: [PATCH 1/6] clocksource: sh_cmt: compute rate before registration again

[John Stultz]

On Mon, Feb 6, 2017 at 1:11 PM, Nicolai Stange <nicstange@xxxxxxxxx> wrote:
> With the upcoming NTP correction related rate adjustments to be implemented
> in the clockevents core, the latter needs to get informed about every rate
> change of a clockevent device made after its registration.
>
> Currently, sh_cmt violates this requirement in that it registers its
> clockevent device with a dummy rate and sets its final ->mult and ->shift
> values from its ->set_state_oneshot() and ->set_state_periodic() functions
> respectively.
>
> This patch moves the setting of the clockevent device's ->mult and ->shift
> values to before its registration.
>
> Note that there has been some back and forth regarding this question with
> respect to the clocksource also provided by this driver:
>   commit f4d7c3565c16 ("clocksource: sh_cmt: compute mult and shift before
>                         registration")
> moves the rate determination from the clocksource's ->enable() function to
> before its registration. OTOH, the later
>   commit 3593f5fe40a1 ("clocksource: sh_cmt: __clocksource_updatefreq_hz()
>                         update")
> basically reverts this, saying
>   "Without this patch the old code uses clocksource_register() together
>    with a hack that assumes a never changing clock rate."
>
> However, I checked all current sh_cmt users in arch/sh as well as in
> arch/arm/mach-shmobile carefully and right now, none of them changes any
> rate in any clock tree relevant to sh_cmt after their respective
> time_init(). Since all sh_cmt instances are created after time_init(), none
> of them should ever observe any clock rate changes.
>
> What's more, both, a clocksource as well as a clockevent device, can
> immediately get selected for use at their registration and thus, enabled
> at this point already. So it's probably safer to assume a "never changing
> clock rate" here.
>
> - Move the struct sh_cmt_channel's ->rate member to struct sh_cmt_device:
>   it's a property of the underlying clock which is in turn specific to
>   the sh_cmt_device.
> - Determine the ->rate value in sh_cmt_setup() at device probing rather
>   than at first usage.
> - Set the clockevent device's ->mult and ->shift values right before its
>   registration.
> - Although not strictly necessary for the upcoming clockevent core changes,
>   set the clocksource's rate at its registration for consistency.

Magnus: Do you have any objection to the above?  I know we reworked
this a few times in the past.

thanks
-john