Hi Mark,
On 17 January 2017 at 01:50, Mark Rutland <mark.rutland@xxxxxxx> wrote:
> On Wed, Dec 21, 2016 at 02:45:54PM +0800, fu.wei@xxxxxxxxxx wrote:
>> From: Fu Wei <fu.wei@xxxxxxxxxx>
>>
>> Currently, the counter frequency detection call(arch_timer_detect_rate)
>> combines all the ways to get counter frequency: device-tree property,
>> system coprocessor register, MMIO timer. But in the most of use cases,
>> we don't need all the ways to try:
>> For example, reading device-tree property will be needed only when
>> system boot with device-tree, getting frequency from MMIO timer register
>> will beneeded only when we init MMIO timer.
>>
>> This patch separates paths to determine frequency:
>> Separate out device-tree code, keep them in device-tree init function.
>
> Splitting these out makes sense to me.
OK , will do
>
>> Separate out the MMIO frequency and the sysreg frequency detection call,
>> and use the appropriate one for the counter.
>
>> Signed-off-by: Fu Wei <fu.wei@xxxxxxxxxx>
>> Tested-by: Xiongfeng Wang <wangxiongfeng2@xxxxxxxxxx>
>> ---
>> drivers/clocksource/arm_arch_timer.c | 49 +++++++++++++++++++++++-------------
>> 1 file changed, 31 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
>> index c7b4482..9a1f138 100644
>> --- a/drivers/clocksource/arm_arch_timer.c
>> +++ b/drivers/clocksource/arm_arch_timer.c
>> @@ -488,27 +488,31 @@ static int arch_timer_starting_cpu(unsigned int cpu)
>> return 0;
>> }
>>
>> -static void
>> -arch_timer_detect_rate(void __iomem *cntbase, struct device_node *np)
>> +static void arch_timer_detect_rate(void)
>> {
>> - /* Who has more than one independent system counter? */
>> - if (arch_timer_rate)
>> - return;
>> + /*
>> + * Try to get the timer frequency from
>> + * cntfrq_el0(system coprocessor register).
>> + */
>> + if (!arch_timer_rate)
>> + arch_timer_rate = arch_timer_get_cntfrq();
>> +
>> + /* Check the timer frequency. */
>> + if (!arch_timer_rate)
>> + pr_warn("frequency not available\n");
>> +}
>>
>> +static void arch_timer_mem_detect_rate(void __iomem *cntbase)
>> +{
>> /*
>> - * Try to determine the frequency from the device tree or CNTFRQ,
>> - * if ACPI is enabled, get the frequency from CNTFRQ ONLY.
>> + * Try to determine the frequency from
>> + * CNTFRQ in memory-mapped timer.
>> */
>> - if (!acpi_disabled ||
>> - of_property_read_u32(np, "clock-frequency", &arch_timer_rate)) {
>> - if (cntbase)
>> - arch_timer_rate = readl_relaxed(cntbase + CNTFRQ);
>> - else
>> - arch_timer_rate = arch_timer_get_cntfrq();
>> - }
>> + if (!arch_timer_rate)
>> + arch_timer_rate = readl_relaxed(cntbase + CNTFRQ);
>>
>> /* Check the timer frequency. */
>> - if (arch_timer_rate == 0)
>> + if (!arch_timer_rate)
I think you mean this one, this is for keeping consistency with
arch_timer_detect_rate.
>> pr_warn("frequency not available\n");
>> }
>
> There's a subtle change in behaviour here. Previously for ACPI we'd only
> ever use the sysreg CNTFRQ value for arch_timer_rate, whereas now we
> might use the MMIO timer rate. Maybe that's not a big deal, but I will
> need to think.
>
> Generally, the logic to determine the rate is fairly gnarly regardless.
>
> It would be nice if we could split the MMIO and sysreg rates entirely,
Yes, I am doing this way,
For sysreg rates,
static void arch_timer_detect_rate(void)
{
/*
* Try to get the timer frequency from
* cntfrq_el0(system coprocessor register).
*/
if (!arch_timer_rate)
arch_timer_rate = arch_timer_get_cntfrq();
/* Check the timer frequency. */
if (!arch_timer_rate)
pr_warn("frequency not available\n");
}
For MMIO timer,
static void arch_timer_mem_detect_rate(void __iomem *cntbase)
{
/*
* Try to determine the frequency from
* CNTFRQ in memory-mapped timer.
*/
if (!arch_timer_rate)
arch_timer_rate = readl_relaxed(cntbase + CNTFRQ);
/* Check the timer frequency. */
if (!arch_timer_rate)
pr_warn("frequency not available\n");
}
in arch_time_*_init, only call arch_timer_detect_rate,
in arch_timer_mem_init, only call arch_timer_mem_detect_rate.
But you are right, this is fairly gnarly regardless.
> and kill the implicit relationship between the two, or at least make one
> canonical and warn if the two differ.
So I think maybe we can do this:
static void __arch_timer_determine_rate(u32 rate)
{
/* Check the timer frequency. */
if (!arch_timer_rate)
if (rate)
arch_timer_rate = rate;
else
pr_warn("frequency not available\n");
else if (arch_timer_rate != rate)
pr_warn("got different frequency, keep original.\n");
}
static void arch_timer_detect_rate(void)
{
/*
* Try to get the timer frequency from
* cntfrq_el0(system coprocessor register).
*/
__arch_timer_determine_rate(arch_timer_get_cntfrq());
}
static void arch_timer_mem_detect_rate(void __iomem *cntbase)
{
/*
* Try to get the timer frequency from
* CNTFRQ in the MMIO timer.
*/
__arch_timer_determine_rate(readl_relaxed(cntbase + CNTFRQ));
}
any thought?
>
> Thanks,
> Mark.
--
Best regards,
Fu Wei
Software Engineer
Red Hat
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
