diff mbox

[v21,01/13] clocksource: arm_arch_timer: introduce two functions to get the frequency from mmio and sysreg.

Message ID 20170206185015.12296-2-fu.wei@linaro.org (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

fu.wei@linaro.org Feb. 6, 2017, 6:50 p.m. UTC 
From: Fu Wei <fu.wei@linaro.org>

The patch introduce two new functions: arch_timer_get_sysreg_freq and
arch_timer_get_mmio_freq, and applys them in arch_timer_detect_rate.
These will be used for getting the frequency from mmio and sysreg to
prepare for reworking counter frequency detection.

Signed-off-by: Fu Wei <fu.wei@linaro.org>
---
 drivers/clocksource/arm_arch_timer.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

Comments

Mark Rutland March 17, 2017, 6:05 p.m. UTC | #1 
On Tue, Feb 07, 2017 at 02:50:03AM +0800, fu.wei@linaro.org wrote:
> +static u32 arch_timer_get_sysreg_freq(void)
> +{
> +	/*
> +	 * Try to get the frequency from the CNTFRQ of sysreg.
> +	 */
> +	return arch_timer_get_cntfrq();
> +}

We already have arch_timer_get_cntfrq(), so I don't see the point in
this wrapper.

> +static u32 arch_timer_get_mmio_freq(void __iomem *cntbase)
> +{
> +	/*
> +	 * Try to get the frequency from the CNTFRQ of timer frame registers.
> +	 * Note: please verify cntbase in caller.
> +	 */
> +	return readl_relaxed(cntbase + CNTFRQ);
> +}

Wrapping the MMIO read makes sense if we're going to do this in more
than one place, so I'm happy with this wrapper.

If you can s/arch_timer_get_mmio_freq/arch_timer_get_cntfrq/, and drop
the comments, then this looks fine to me.

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
fu.wei@linaro.org March 20, 2017, 7:36 a.m. UTC | #2 
Hi Mark,

On 18 March 2017 at 02:05, Mark Rutland <mark.rutland@arm.com> wrote:
> On Tue, Feb 07, 2017 at 02:50:03AM +0800, fu.wei@linaro.org wrote:
>> +static u32 arch_timer_get_sysreg_freq(void)
>> +{
>> +     /*
>> +      * Try to get the frequency from the CNTFRQ of sysreg.
>> +      */
>> +     return arch_timer_get_cntfrq();
>> +}
>
> We already have arch_timer_get_cntfrq(), so I don't see the point in
> this wrapper.
>
>> +static u32 arch_timer_get_mmio_freq(void __iomem *cntbase)
>> +{
>> +     /*
>> +      * Try to get the frequency from the CNTFRQ of timer frame registers.
>> +      * Note: please verify cntbase in caller.
>> +      */
>> +     return readl_relaxed(cntbase + CNTFRQ);
>> +}
>
> Wrapping the MMIO read makes sense if we're going to do this in more
> than one place, so I'm happy with this wrapper.
>
> If you can s/arch_timer_get_mmio_freq/arch_timer_get_cntfrq/, and drop

sorry, May I guess that is
"s/arch_timer_get_mmio_freq/arch_timer_get_mmio_cntfrq/"
or
"s/arch_timer_get_mmio_freq/arch_timer_mem_get_cntfrq/"

which one do you prefer? :-)

> the comments, then this looks fine to me.
>
> Thanks,
> Mark.
fu.wei@linaro.org March 20, 2017, 9:43 a.m. UTC | #3 
Hi Mark,

On 20 March 2017 at 15:36, Fu Wei <fu.wei@linaro.org> wrote:
> Hi Mark,
>
> On 18 March 2017 at 02:05, Mark Rutland <mark.rutland@arm.com> wrote:
>> On Tue, Feb 07, 2017 at 02:50:03AM +0800, fu.wei@linaro.org wrote:
>>> +static u32 arch_timer_get_sysreg_freq(void)
>>> +{
>>> +     /*
>>> +      * Try to get the frequency from the CNTFRQ of sysreg.
>>> +      */
>>> +     return arch_timer_get_cntfrq();
>>> +}
>>
>> We already have arch_timer_get_cntfrq(), so I don't see the point in
>> this wrapper.
>>
>>> +static u32 arch_timer_get_mmio_freq(void __iomem *cntbase)
>>> +{
>>> +     /*
>>> +      * Try to get the frequency from the CNTFRQ of timer frame registers.
>>> +      * Note: please verify cntbase in caller.
>>> +      */
>>> +     return readl_relaxed(cntbase + CNTFRQ);
>>> +}
>>
>> Wrapping the MMIO read makes sense if we're going to do this in more
>> than one place, so I'm happy with this wrapper.
>>
>> If you can s/arch_timer_get_mmio_freq/arch_timer_get_cntfrq/, and drop
>
> sorry, May I guess that is
> "s/arch_timer_get_mmio_freq/arch_timer_get_mmio_cntfrq/"
> or
> "s/arch_timer_get_mmio_freq/arch_timer_mem_get_cntfrq/"
>
> which one do you prefer? :-)

keeping using arch_timer_get_cntfrq();  for per-CPU arch timer, then

+static u32 arch_timer_mem_get_cntfrq(void __iomem *cntbase)
+{
+       return readl_relaxed(cntbase + CNTFRQ);
+}
+

Is that OK for you?

>
>> the comments, then this looks fine to me.
>>
>> Thanks,
>> Mark.
>
>
>
> --
> Best regards,
>
> Fu Wei
> Software Engineer
> Red Hat
Mark Rutland March 20, 2017, 10:41 a.m. UTC | #4 
On Mon, Mar 20, 2017 at 05:43:29PM +0800, Fu Wei wrote:
> On 20 March 2017 at 15:36, Fu Wei <fu.wei@linaro.org> wrote:
> > On 18 March 2017 at 02:05, Mark Rutland <mark.rutland@arm.com> wrote:
> >> On Tue, Feb 07, 2017 at 02:50:03AM +0800, fu.wei@linaro.org wrote:

> >>> +static u32 arch_timer_get_mmio_freq(void __iomem *cntbase)
> >>> +{
> >>> +     /*
> >>> +      * Try to get the frequency from the CNTFRQ of timer frame registers.
> >>> +      * Note: please verify cntbase in caller.
> >>> +      */
> >>> +     return readl_relaxed(cntbase + CNTFRQ);
> >>> +}
> >>
> >> Wrapping the MMIO read makes sense if we're going to do this in more
> >> than one place, so I'm happy with this wrapper.
> >>
> >> If you can s/arch_timer_get_mmio_freq/arch_timer_get_cntfrq/, and drop
> >
> > sorry, May I guess that is
> > "s/arch_timer_get_mmio_freq/arch_timer_get_mmio_cntfrq/"
> > or
> > "s/arch_timer_get_mmio_freq/arch_timer_mem_get_cntfrq/"
> >
> > which one do you prefer? :-)
> 
> keeping using arch_timer_get_cntfrq();  for per-CPU arch timer, then
> 
> +static u32 arch_timer_mem_get_cntfrq(void __iomem *cntbase)
> +{
> +       return readl_relaxed(cntbase + CNTFRQ);
> +}
> +

That looks perfect to me.

Sorry for the confusion above!

Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
fu.wei@linaro.org March 20, 2017, 11:09 a.m. UTC | #5 
Hi Mark,

On 20 March 2017 at 18:41, Mark Rutland <mark.rutland@arm.com> wrote:
> On Mon, Mar 20, 2017 at 05:43:29PM +0800, Fu Wei wrote:
>> On 20 March 2017 at 15:36, Fu Wei <fu.wei@linaro.org> wrote:
>> > On 18 March 2017 at 02:05, Mark Rutland <mark.rutland@arm.com> wrote:
>> >> On Tue, Feb 07, 2017 at 02:50:03AM +0800, fu.wei@linaro.org wrote:
>
>> >>> +static u32 arch_timer_get_mmio_freq(void __iomem *cntbase)
>> >>> +{
>> >>> +     /*
>> >>> +      * Try to get the frequency from the CNTFRQ of timer frame registers.
>> >>> +      * Note: please verify cntbase in caller.
>> >>> +      */
>> >>> +     return readl_relaxed(cntbase + CNTFRQ);
>> >>> +}
>> >>
>> >> Wrapping the MMIO read makes sense if we're going to do this in more
>> >> than one place, so I'm happy with this wrapper.
>> >>
>> >> If you can s/arch_timer_get_mmio_freq/arch_timer_get_cntfrq/, and drop
>> >
>> > sorry, May I guess that is
>> > "s/arch_timer_get_mmio_freq/arch_timer_get_mmio_cntfrq/"
>> > or
>> > "s/arch_timer_get_mmio_freq/arch_timer_mem_get_cntfrq/"
>> >
>> > which one do you prefer? :-)
>>
>> keeping using arch_timer_get_cntfrq();  for per-CPU arch timer, then
>>
>> +static u32 arch_timer_mem_get_cntfrq(void __iomem *cntbase)
>> +{
>> +       return readl_relaxed(cntbase + CNTFRQ);
>> +}
>> +
>
> That looks perfect to me.
>
> Sorry for the confusion above!

Great, thanks , doing this way :-)

>
> Mark.
diff mbox

Patch

diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 46a1709..1d273d6 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -554,6 +554,23 @@  static int arch_timer_starting_cpu(unsigned int cpu)
 	return 0;
 }
 
+static u32 arch_timer_get_sysreg_freq(void)
+{
+	/*
+	 * Try to get the frequency from the CNTFRQ of sysreg.
+	 */
+	return arch_timer_get_cntfrq();
+}
+
+static u32 arch_timer_get_mmio_freq(void __iomem *cntbase)
+{
+	/*
+	 * Try to get the frequency from the CNTFRQ of timer frame registers.
+	 * Note: please verify cntbase in caller.
+	 */
+	return readl_relaxed(cntbase + CNTFRQ);
+}
+
 static void
 arch_timer_detect_rate(void __iomem *cntbase, struct device_node *np)
 {
@@ -568,9 +585,9 @@  arch_timer_detect_rate(void __iomem *cntbase, struct device_node *np)
 	if (!acpi_disabled ||
 	    of_property_read_u32(np, "clock-frequency", &arch_timer_rate)) {
 		if (cntbase)
-			arch_timer_rate = readl_relaxed(cntbase + CNTFRQ);
+			arch_timer_rate = arch_timer_get_mmio_freq(cntbase);
 		else
-			arch_timer_rate = arch_timer_get_cntfrq();
+			arch_timer_rate = arch_timer_get_sysreg_freq();
 	}
 
 	/* Check the timer frequency. */