[PATCH 0/7] Clocksource changes for Pistachio CPUFreq

[PATCH 0/7] Clocksource changes for Pistachio CPUFreq

[Ezequiel Garcia]

The purpose of this patchset is to support CPUFreq on Pistachio SoC.
However, given Pistachio uses the MIPS GIC clocksource and clockevent
drivers (clocked from the CPU), adding CPUFreq support needs some work.

This patchset changes the MIPS GIC clockevent driver to update
the frequency of the per-cpu clockevents using a clock notifier.

Then, we add a clocksource driver for IMG Pistachio SoC, based on the
general purpose timers. The SoC only provides four timers, so we can't
use them to implement the four clockevents and the clocksource.

However, we can use one of these timers to provide a clocksource and
a sched clock. Given the general purpose timers are clocked from the
peripheral system clock tree, they are not affected by CPU rate changes.

Patches 1 to 3 are just style cleaning and preparation work. Patch 4
adds the clockevent frequency update.

Patches 5 and 6 add the new clocksource driver.

Patch 7 introduces an option to enable the timer based clocksource
on Pistachio.

For CPUFreq to really work, clk driver changes are needed to support
MIPS PLL clock rate change. Patches for this will be posted soon.

This series apply on v4.1-rc4. As always, comments and feedback are welcome!

Ezequiel Garcia (7):
  clocksource: mips-gic: Enable the clock before using it
  clocksource: mips-gic: Add missing error returns checks
  clocksource: mips-gic: Split clocksource and clockevent initialization
  clocksource: mips-gic: Update clockevent frequency on clock rate
    changes
  clocksource: Add Pistachio SoC general purpose timer binding document
  clocksource: Add Pistachio clocksource-only driver
  mips: pistachio: Allow to enable the external timer based clocksource

 .../bindings/timer/img,pistachio-gptimer.txt       |  29 +++
 arch/mips/Kconfig                                  |   1 +
 arch/mips/pistachio/Kconfig                        |  13 ++
 drivers/clocksource/Kconfig                        |   4 +
 drivers/clocksource/Makefile                       |   1 +
 drivers/clocksource/mips-gic-timer.c               |  65 ++++++-
 drivers/clocksource/time-pistachio.c               | 202 +++++++++++++++++++++
 7 files changed, 306 insertions(+), 9 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/timer/img,pistachio-gptimer.txt
 create mode 100644 arch/mips/pistachio/Kconfig
 create mode 100644 drivers/clocksource/time-pistachio.c

-- 
2.3.3

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/7] clocksource: mips-gic: Add missing error returns checks

[Ezequiel Garcia]

This commit adds the required checks on the functions that return
an error. Some of them are not critical, so only a warning is
printed.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@imgtec.com>
---
 drivers/clocksource/mips-gic-timer.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/clocksource/mips-gic-timer.c b/drivers/clocksource/mips-gic-timer.c
index 913585d..c4352f0 100644
--- a/drivers/clocksource/mips-gic-timer.c
+++ b/drivers/clocksource/mips-gic-timer.c
@@ -100,12 +100,18 @@ static struct notifier_block gic_cpu_nb = {
 
 static int gic_clockevent_init(void)
 {
+	int ret;
+
 	if (!cpu_has_counter || !gic_frequency)
 		return -ENXIO;
 
-	setup_percpu_irq(gic_timer_irq, &gic_compare_irqaction);
+	ret = setup_percpu_irq(gic_timer_irq, &gic_compare_irqaction);
+	if (ret < 0)
+		return ret;
 
-	register_cpu_notifier(&gic_cpu_nb);
+	ret = register_cpu_notifier(&gic_cpu_nb);
+	if (ret < 0)
+		pr_warn("GIC: Unable to register CPU notifier\n");
 
 	gic_clockevent_cpu_init(this_cpu_ptr(&gic_clockevent_device));
 
@@ -125,13 +131,17 @@ static struct clocksource gic_clocksource = {
 
 static void __init __gic_clocksource_init(void)
 {
+	int ret;
+
 	/* Set clocksource mask. */
 	gic_clocksource.mask = CLOCKSOURCE_MASK(gic_get_count_width());
 
 	/* Calculate a somewhat reasonable rating value. */
 	gic_clocksource.rating = 200 + gic_frequency / 10000000;
 
-	clocksource_register_hz(&gic_clocksource, gic_frequency);
+	ret = clocksource_register_hz(&gic_clocksource, gic_frequency);
+	if (ret < 0)
+		pr_warn("GIC: Unable to register clocksource\n");
 
 	gic_clockevent_init();
 
-- 
2.3.3

Re: [PATCH 2/7] clocksource: mips-gic: Add missing error returns checks

[Andrew Bresticker]

On Thu, May 21, 2015 at 2:37 PM, Ezequiel Garcia
<ezequiel.garcia-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org> wrote:
> This commit adds the required checks on the functions that return
> an error. Some of them are not critical, so only a warning is
> printed.
>
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>

Reviewed-by: Andrew Bresticker <abrestic-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/7] clocksource: mips-gic: Enable the clock before using it

[Ezequiel Garcia]

For the clock to be used (e.g. get its rate through clk_get_rate)
it should be prepared and enabled first.

Also, while the clock is enabled the driver must hold a reference to it,
so let's remove the call to clk_put.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
---
 drivers/clocksource/mips-gic-timer.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/clocksource/mips-gic-timer.c b/drivers/clocksource/mips-gic-timer.c
index b81ed1a..913585d 100644
--- a/drivers/clocksource/mips-gic-timer.c
+++ b/drivers/clocksource/mips-gic-timer.c
@@ -158,8 +158,13 @@ static void __init gic_clocksource_of_init(struct device_node *node)
 
 	clk = of_clk_get(node, 0);
 	if (!IS_ERR(clk)) {
+		if (clk_prepare_enable(clk) < 0) {
+			pr_err("GIC failed to enable clock\n");
+			clk_put(clk);
+			return;
+		}
+
 		gic_frequency = clk_get_rate(clk);
-		clk_put(clk);
 	} else if (of_property_read_u32(node, "clock-frequency",
 					&gic_frequency)) {
 		pr_err("GIC frequency not specified.\n");
-- 
2.3.3

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/7] clocksource: mips-gic: Enable the clock before using it

[Andrew Bresticker]

On Thu, May 21, 2015 at 2:37 PM, Ezequiel Garcia
<ezequiel.garcia-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org> wrote:
> For the clock to be used (e.g. get its rate through clk_get_rate)
> it should be prepared and enabled first.
>
> Also, while the clock is enabled the driver must hold a reference to it,
> so let's remove the call to clk_put.
>
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>

Reviewed-by: Andrew Bresticker <abrestic-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/7] clocksource: mips-gic: Split clocksource and clockevent initialization

[Ezequiel Garcia]

This is preparation work for the introduction of clockevent frequency
update with a clock notifier. This is only possible when the device
is passed a clk struct, so let's split the legacy and devicetree
initialization.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
---
 drivers/clocksource/mips-gic-timer.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/clocksource/mips-gic-timer.c b/drivers/clocksource/mips-gic-timer.c
index c4352f0..22a4daf 100644
--- a/drivers/clocksource/mips-gic-timer.c
+++ b/drivers/clocksource/mips-gic-timer.c
@@ -142,11 +142,6 @@ static void __init __gic_clocksource_init(void)
 	ret = clocksource_register_hz(&gic_clocksource, gic_frequency);
 	if (ret < 0)
 		pr_warn("GIC: Unable to register clocksource\n");
-
-	gic_clockevent_init();
-
-	/* And finally start the counter */
-	gic_start_count();
 }
 
 void __init gic_clocksource_init(unsigned int frequency)
@@ -156,6 +151,10 @@ void __init gic_clocksource_init(unsigned int frequency)
 		GIC_LOCAL_TO_HWIRQ(GIC_LOCAL_INT_COMPARE);
 
 	__gic_clocksource_init();
+	gic_clockevent_init();
+
+	/* And finally start the counter */
+	gic_start_count();
 }
 
 static void __init gic_clocksource_of_init(struct device_node *node)
@@ -187,6 +186,10 @@ static void __init gic_clocksource_of_init(struct device_node *node)
 	}
 
 	__gic_clocksource_init();
+	gic_clockevent_init();
+
+	/* And finally start the counter */
+	gic_start_count();
 }
 CLOCKSOURCE_OF_DECLARE(mips_gic_timer, "mti,gic-timer",
 		       gic_clocksource_of_init);
-- 
2.3.3

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/7] clocksource: mips-gic: Split clocksource and clockevent initialization

[Andrew Bresticker]

On Thu, May 21, 2015 at 2:37 PM, Ezequiel Garcia
<ezequiel.garcia@imgtec.com> wrote:
> This is preparation work for the introduction of clockevent frequency
> update with a clock notifier. This is only possible when the device
> is passed a clk struct, so let's split the legacy and devicetree
> initialization.
>
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@imgtec.com>
> ---
>  drivers/clocksource/mips-gic-timer.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/clocksource/mips-gic-timer.c b/drivers/clocksource/mips-gic-timer.c
> index c4352f0..22a4daf 100644
> --- a/drivers/clocksource/mips-gic-timer.c
> +++ b/drivers/clocksource/mips-gic-timer.c
> @@ -142,11 +142,6 @@ static void __init __gic_clocksource_init(void)
>         ret = clocksource_register_hz(&gic_clocksource, gic_frequency);
>         if (ret < 0)
>                 pr_warn("GIC: Unable to register clocksource\n");
> -
> -       gic_clockevent_init();
> -
> -       /* And finally start the counter */
> -       gic_start_count();
>  }

Instead of duplicating this bit in both the OF and non-OF paths, maybe
it would be better to do the notifier registration in
gic_clockevent_init(), either by passing around the struct clk or
making it a global?

Re: [PATCH 3/7] clocksource: mips-gic: Split clocksource and clockevent initialization

[Ezequiel Garcia]

On 05/21/2015 07:24 PM, Andrew Bresticker wrote:
> On Thu, May 21, 2015 at 2:37 PM, Ezequiel Garcia
> <ezequiel.garcia-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org> wrote:
>> This is preparation work for the introduction of clockevent frequency
>> update with a clock notifier. This is only possible when the device
>> is passed a clk struct, so let's split the legacy and devicetree
>> initialization.
>>
>> Signed-off-by: Ezequiel Garcia <ezequiel.garcia-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
>> ---
>>  drivers/clocksource/mips-gic-timer.c | 13 ++++++++-----
>>  1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/clocksource/mips-gic-timer.c b/drivers/clocksource/mips-gic-timer.c
>> index c4352f0..22a4daf 100644
>> --- a/drivers/clocksource/mips-gic-timer.c
>> +++ b/drivers/clocksource/mips-gic-timer.c
>> @@ -142,11 +142,6 @@ static void __init __gic_clocksource_init(void)
>>         ret = clocksource_register_hz(&gic_clocksource, gic_frequency);
>>         if (ret < 0)
>>                 pr_warn("GIC: Unable to register clocksource\n");
>> -
>> -       gic_clockevent_init();
>> -
>> -       /* And finally start the counter */
>> -       gic_start_count();
>>  }
> 
> Instead of duplicating this bit in both the OF and non-OF paths, maybe
> it would be better to do the notifier registration in
> gic_clockevent_init(), either by passing around the struct clk or
> making it a global?
> 

Yeah, I had something like that first, but somehow it looked ugly to have:

  gic_clockevent_init(IS_ERR(clk) ? NULL : clk);

Don't have a strong opinion though.
-- 
Ezequiel
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/7] clocksource: mips-gic: Split clocksource and clockevent initialization

[Andrew Bresticker]

On Thu, May 21, 2015 at 3:25 PM, Ezequiel Garcia
<ezequiel.garcia-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org> wrote:
>
>
> On 05/21/2015 07:24 PM, Andrew Bresticker wrote:
>> On Thu, May 21, 2015 at 2:37 PM, Ezequiel Garcia
>> <ezequiel.garcia-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org> wrote:
>>> This is preparation work for the introduction of clockevent frequency
>>> update with a clock notifier. This is only possible when the device
>>> is passed a clk struct, so let's split the legacy and devicetree
>>> initialization.
>>>
>>> Signed-off-by: Ezequiel Garcia <ezequiel.garcia-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
>>> ---
>>>  drivers/clocksource/mips-gic-timer.c | 13 ++++++++-----
>>>  1 file changed, 8 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/clocksource/mips-gic-timer.c b/drivers/clocksource/mips-gic-timer.c
>>> index c4352f0..22a4daf 100644
>>> --- a/drivers/clocksource/mips-gic-timer.c
>>> +++ b/drivers/clocksource/mips-gic-timer.c
>>> @@ -142,11 +142,6 @@ static void __init __gic_clocksource_init(void)
>>>         ret = clocksource_register_hz(&gic_clocksource, gic_frequency);
>>>         if (ret < 0)
>>>                 pr_warn("GIC: Unable to register clocksource\n");
>>> -
>>> -       gic_clockevent_init();
>>> -
>>> -       /* And finally start the counter */
>>> -       gic_start_count();
>>>  }
>>
>> Instead of duplicating this bit in both the OF and non-OF paths, maybe
>> it would be better to do the notifier registration in
>> gic_clockevent_init(), either by passing around the struct clk or
>> making it a global?
>>
>
> Yeah, I had something like that first, but somehow it looked ugly to have:
>
>   gic_clockevent_init(IS_ERR(clk) ? NULL : clk);
>
> Don't have a strong opinion though.

Ah that is a bit ugly.  I'm fine either way though, so:

Reviewed-by: Andrew Bresticker <abrestic-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/7] clocksource: mips-gic: Update clockevent frequency on clock rate changes

[Ezequiel Garcia]

This commit introduces the clockevent frequency update, using
a clock notifier. It will be used to support CPUFreq on platforms
using MIPS GIC based clockevents.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
---
 drivers/clocksource/mips-gic-timer.c | 31 ++++++++++++++++++++++++++++++-
 1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/drivers/clocksource/mips-gic-timer.c b/drivers/clocksource/mips-gic-timer.c
index 22a4daf..71bbd42 100644
--- a/drivers/clocksource/mips-gic-timer.c
+++ b/drivers/clocksource/mips-gic-timer.c
@@ -79,6 +79,13 @@ static void gic_clockevent_cpu_exit(struct clock_event_device *cd)
 	disable_percpu_irq(gic_timer_irq);
 }
 
+static void gic_update_frequency(void *data)
+{
+	unsigned long rate = (unsigned long)data;
+
+	clockevents_update_freq(this_cpu_ptr(&gic_clockevent_device), rate);
+}
+
 static int gic_cpu_notifier(struct notifier_block *nb, unsigned long action,
 				void *data)
 {
@@ -94,10 +101,26 @@ static int gic_cpu_notifier(struct notifier_block *nb, unsigned long action,
 	return NOTIFY_OK;
 }
 
+static int gic_clk_notifier(struct notifier_block *nb, unsigned long action,
+			    void *data)
+{
+	struct clk_notifier_data *cnd = data;
+
+	if (action == POST_RATE_CHANGE)
+		on_each_cpu(gic_update_frequency, (void *)cnd->new_rate, 1);
+
+	return NOTIFY_OK;
+}
+
+
 static struct notifier_block gic_cpu_nb = {
 	.notifier_call = gic_cpu_notifier,
 };
 
+static struct notifier_block gic_clk_nb = {
+	.notifier_call = gic_clk_notifier,
+};
+
 static int gic_clockevent_init(void)
 {
 	int ret;
@@ -160,6 +183,7 @@ void __init gic_clocksource_init(unsigned int frequency)
 static void __init gic_clocksource_of_init(struct device_node *node)
 {
 	struct clk *clk;
+	int ret;
 
 	if (WARN_ON(!gic_present || !node->parent ||
 		    !of_device_is_compatible(node->parent, "mti,gic")))
@@ -186,7 +210,12 @@ static void __init gic_clocksource_of_init(struct device_node *node)
 	}
 
 	__gic_clocksource_init();
-	gic_clockevent_init();
+
+	ret = gic_clockevent_init();
+	if (ret < 0 && !IS_ERR(clk)) {
+		if (clk_notifier_register(clk, &gic_clk_nb) < 0)
+			pr_warn("GIC: Unable to register clock notifier\n");
+	}
 
 	/* And finally start the counter */
 	gic_start_count();
-- 
2.3.3

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/7] clocksource: mips-gic: Update clockevent frequency on clock rate changes

[Andrew Bresticker]

On Thu, May 21, 2015 at 2:37 PM, Ezequiel Garcia
<ezequiel.garcia-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org> wrote:
> This commit introduces the clockevent frequency update, using
> a clock notifier. It will be used to support CPUFreq on platforms
> using MIPS GIC based clockevents.
>
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/clocksource/mips-gic-timer.c | 31 ++++++++++++++++++++++++++++++-
>  1 file changed, 30 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clocksource/mips-gic-timer.c b/drivers/clocksource/mips-gic-timer.c
> index 22a4daf..71bbd42 100644
> --- a/drivers/clocksource/mips-gic-timer.c
> +++ b/drivers/clocksource/mips-gic-timer.c
> @@ -79,6 +79,13 @@ static void gic_clockevent_cpu_exit(struct clock_event_device *cd)
>         disable_percpu_irq(gic_timer_irq);
>  }
>
> +static void gic_update_frequency(void *data)
> +{
> +       unsigned long rate = (unsigned long)data;
> +
> +       clockevents_update_freq(this_cpu_ptr(&gic_clockevent_device), rate);
> +}
> +
>  static int gic_cpu_notifier(struct notifier_block *nb, unsigned long action,
>                                 void *data)
>  {
> @@ -94,10 +101,26 @@ static int gic_cpu_notifier(struct notifier_block *nb, unsigned long action,
>         return NOTIFY_OK;
>  }
>
> +static int gic_clk_notifier(struct notifier_block *nb, unsigned long action,
> +                           void *data)
> +{
> +       struct clk_notifier_data *cnd = data;
> +
> +       if (action == POST_RATE_CHANGE)
> +               on_each_cpu(gic_update_frequency, (void *)cnd->new_rate, 1);
> +
> +       return NOTIFY_OK;
> +}
> +
> +
>  static struct notifier_block gic_cpu_nb = {
>         .notifier_call = gic_cpu_notifier,
>  };
>
> +static struct notifier_block gic_clk_nb = {
> +       .notifier_call = gic_clk_notifier,
> +};
> +
>  static int gic_clockevent_init(void)
>  {
>         int ret;
> @@ -160,6 +183,7 @@ void __init gic_clocksource_init(unsigned int frequency)
>  static void __init gic_clocksource_of_init(struct device_node *node)
>  {
>         struct clk *clk;
> +       int ret;
>
>         if (WARN_ON(!gic_present || !node->parent ||
>                     !of_device_is_compatible(node->parent, "mti,gic")))
> @@ -186,7 +210,12 @@ static void __init gic_clocksource_of_init(struct device_node *node)
>         }
>
>         __gic_clocksource_init();
> -       gic_clockevent_init();
> +
> +       ret = gic_clockevent_init();
> +       if (ret < 0 && !IS_ERR(clk)) {

I think you mean ret == 0 here?

> +               if (clk_notifier_register(clk, &gic_clk_nb) < 0)
> +                       pr_warn("GIC: Unable to register clock notifier\n");
> +       }
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/7] clocksource: mips-gic: Update clockevent frequency on clock rate changes

[Ezequiel Garcia]

On 05/21/2015 07:32 PM, Andrew Bresticker wrote:
> On Thu, May 21, 2015 at 2:37 PM, Ezequiel Garcia
> <ezequiel.garcia-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org> wrote:
>> This commit introduces the clockevent frequency update, using
>> a clock notifier. It will be used to support CPUFreq on platforms
>> using MIPS GIC based clockevents.
>>
>> Signed-off-by: Ezequiel Garcia <ezequiel.garcia-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
>> ---
>>  drivers/clocksource/mips-gic-timer.c | 31 ++++++++++++++++++++++++++++++-
>>  1 file changed, 30 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/clocksource/mips-gic-timer.c b/drivers/clocksource/mips-gic-timer.c
>> index 22a4daf..71bbd42 100644
>> --- a/drivers/clocksource/mips-gic-timer.c
>> +++ b/drivers/clocksource/mips-gic-timer.c
>> @@ -79,6 +79,13 @@ static void gic_clockevent_cpu_exit(struct clock_event_device *cd)
>>         disable_percpu_irq(gic_timer_irq);
>>  }
>>
>> +static void gic_update_frequency(void *data)
>> +{
>> +       unsigned long rate = (unsigned long)data;
>> +
>> +       clockevents_update_freq(this_cpu_ptr(&gic_clockevent_device), rate);
>> +}
>> +
>>  static int gic_cpu_notifier(struct notifier_block *nb, unsigned long action,
>>                                 void *data)
>>  {
>> @@ -94,10 +101,26 @@ static int gic_cpu_notifier(struct notifier_block *nb, unsigned long action,
>>         return NOTIFY_OK;
>>  }
>>
>> +static int gic_clk_notifier(struct notifier_block *nb, unsigned long action,
>> +                           void *data)
>> +{
>> +       struct clk_notifier_data *cnd = data;
>> +
>> +       if (action == POST_RATE_CHANGE)
>> +               on_each_cpu(gic_update_frequency, (void *)cnd->new_rate, 1);
>> +
>> +       return NOTIFY_OK;
>> +}
>> +
>> +
>>  static struct notifier_block gic_cpu_nb = {
>>         .notifier_call = gic_cpu_notifier,
>>  };
>>
>> +static struct notifier_block gic_clk_nb = {
>> +       .notifier_call = gic_clk_notifier,
>> +};
>> +
>>  static int gic_clockevent_init(void)
>>  {
>>         int ret;
>> @@ -160,6 +183,7 @@ void __init gic_clocksource_init(unsigned int frequency)
>>  static void __init gic_clocksource_of_init(struct device_node *node)
>>  {
>>         struct clk *clk;
>> +       int ret;
>>
>>         if (WARN_ON(!gic_present || !node->parent ||
>>                     !of_device_is_compatible(node->parent, "mti,gic")))
>> @@ -186,7 +210,12 @@ static void __init gic_clocksource_of_init(struct device_node *node)
>>         }
>>
>>         __gic_clocksource_init();
>> -       gic_clockevent_init();
>> +
>> +       ret = gic_clockevent_init();
>> +       if (ret < 0 && !IS_ERR(clk)) {
> 
> I think you mean ret == 0 here?
> 
>> +               if (clk_notifier_register(clk, &gic_clk_nb) < 0)
>> +                       pr_warn("GIC: Unable to register clock notifier\n");
>> +       }

Ouch, certainly.
-- 
Ezequiel
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 6/7] clocksource: Add Pistachio clocksource-only driver

[Ezequiel Garcia]

The Pistachio SoC provides four general purpose timers, and allow
to implement a clocksource driver.

This driver can be used as a replacement for the MIPS GIC and MIPS R4K
clocksources and sched clocks, which are clocked from the CPU clock.

Given the general purpose timers are clocked from an independent clock,
this new clocksource driver will be useful to introduce CPUFreq support
for Pistachio machines.

Signed-off-by: Govindraj Raja <Govindraj.Raja-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Ezequiel Garcia <ezequiel.garcia-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
---
 drivers/clocksource/Kconfig          |   4 +
 drivers/clocksource/Makefile         |   1 +
 drivers/clocksource/time-pistachio.c | 202 +++++++++++++++++++++++++++++++++++
 3 files changed, 207 insertions(+)
 create mode 100644 drivers/clocksource/time-pistachio.c

diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index 51d7865f..faa16ae 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -106,6 +106,10 @@ config CLKSRC_EFM32
 	  Support to use the timers of EFM32 SoCs as clock source and clock
 	  event device.
 
+config CLKSRC_PISTACHIO
+	bool
+	select CLKSRC_OF
+
 config ARM_ARCH_TIMER
 	bool
 	select CLKSRC_OF if OF
diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index 5b85f6a..9415def 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -42,6 +42,7 @@ obj-$(CONFIG_FSL_FTM_TIMER)	+= fsl_ftm_timer.o
 obj-$(CONFIG_VF_PIT_TIMER)	+= vf_pit_timer.o
 obj-$(CONFIG_CLKSRC_QCOM)	+= qcom-timer.o
 obj-$(CONFIG_MTK_TIMER)		+= mtk_timer.o
+obj-$(CONFIG_CLKSRC_PISTACHIO)	+= time-pistachio.o
 
 obj-$(CONFIG_ARM_ARCH_TIMER)		+= arm_arch_timer.o
 obj-$(CONFIG_ARM_GLOBAL_TIMER)		+= arm_global_timer.o
diff --git a/drivers/clocksource/time-pistachio.c b/drivers/clocksource/time-pistachio.c
new file mode 100644
index 0000000..952d8e6
--- /dev/null
+++ b/drivers/clocksource/time-pistachio.c
@@ -0,0 +1,202 @@
+/*
+ * Pistachio clocksource based on general-purpose timers
+ *
+ * Copyright (C) 2015 Imagination Technologies
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License. See the file "COPYING" in the main directory of this archive
+ * for more details.
+ */
+
+#define pr_fmt(fmt) "%s: " fmt, __func__
+
+#include <linux/clk.h>
+#include <linux/clocksource.h>
+#include <linux/clockchips.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/spinlock.h>
+#include <linux/mfd/syscon.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/sched_clock.h>
+#include <linux/time.h>
+
+/* Top level reg */
+#define	CR_TIMER_CTRL_CFG		0x00
+  #define TIMER_ME_GLOBAL		BIT(0)
+#define	CR_TIMER_REV			0x10
+
+/* Timer specific registers */
+#define TIMER_CFG			0x20
+  #define TIMER_ME_LOCAL		BIT(0)
+#define TIMER_RELOAD_VALUE		0x24
+#define TIMER_CURRENT_VALUE		0x28
+#define TIMER_CURRENT_OVERFLOW_VALUE	0x2C
+#define TIMER_IRQ_STATUS		0x30
+#define TIMER_IRQ_CLEAR			0x34
+#define TIMER_IRQ_MASK			0x38
+
+#define PERIP_TIMER_CONTROL		0x90
+
+/* Timer specific configuration Values */
+#define RELOAD_VALUE	0xffffffff
+
+static void __iomem *timer_base;
+static DEFINE_RAW_SPINLOCK(lock);
+
+static inline u32 gpt_readl(u32 offset, u32 gpt_id)
+{
+	return __raw_readl(timer_base + 0x20 * gpt_id + offset);
+}
+
+static inline void gpt_writel(u32 value, u32 offset, u32 gpt_id)
+{
+	__raw_writel(value, timer_base + 0x20 * gpt_id + offset);
+}
+
+static cycle_t clocksource_read_cycles(struct clocksource *cs)
+{
+	u32 counter, overflw;
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&lock, flags);
+	overflw = gpt_readl(TIMER_CURRENT_OVERFLOW_VALUE, 0);
+	counter = gpt_readl(TIMER_CURRENT_VALUE, 0);
+	raw_spin_unlock_irqrestore(&lock, flags);
+
+	return ~(cycle_t)counter;
+}
+
+static u64 notrace pistachio_read_sched_clock(void)
+{
+	return clocksource_read_cycles(NULL);
+}
+
+static void pistachio_clksrc_enable(int timeridx)
+{
+	u32 val;
+
+	/* Disable GPT local before loading reload value */
+	val = gpt_readl(TIMER_CFG, timeridx);
+	val &= ~TIMER_ME_LOCAL;
+	gpt_writel(val, TIMER_CFG, timeridx);
+
+	gpt_writel(RELOAD_VALUE, TIMER_RELOAD_VALUE, timeridx);
+
+	val = gpt_readl(TIMER_CFG, timeridx);
+	val |= TIMER_ME_LOCAL;
+	gpt_writel(val, TIMER_CFG, timeridx);
+}
+
+static void pistachio_clksrc_disable(int timeridx)
+{
+	u32 val;
+
+	/* Disable GPT local */
+	val = gpt_readl(TIMER_CFG, timeridx);
+	val &= ~TIMER_ME_LOCAL;
+	gpt_writel(val, TIMER_CFG, timeridx);
+}
+
+static int clocksource_enable(struct clocksource *cs)
+{
+	pistachio_clksrc_enable(0);
+	return 0;
+}
+
+static void clocksource_disable(struct clocksource *cs)
+{
+	pistachio_clksrc_disable(0);
+}
+
+/* Desirable clock source for pistachio platform */
+static struct clocksource clocksource_gpt = {
+	.name		= "gptimer",
+	.rating		= 300,
+	.enable		= clocksource_enable,
+	.disable	= clocksource_disable,
+	.read		= clocksource_read_cycles,
+	.mask		= CLOCKSOURCE_MASK(32),
+	.flags		= CLOCK_SOURCE_IS_CONTINUOUS |
+			  CLOCK_SOURCE_SUSPEND_NONSTOP,
+};
+
+static void __init pistachio_clksrc_of_init(struct device_node *node)
+{
+	struct clk *sys_clk, *fast_clk;
+	struct regmap *periph_regs;
+	unsigned long rate;
+	int ret;
+
+	timer_base = of_iomap(node, 0);
+	if (!timer_base) {
+		pr_err("cannot iomap\n");
+		return;
+	}
+
+	/*
+	 * We need early syscon or late clocksource probe for this to work.
+	 */
+	periph_regs = syscon_regmap_lookup_by_phandle(node, "img,cr-periph");
+	if (IS_ERR(periph_regs)) {
+		pr_err("cannot get peripheral regmap (%lu)\n",
+		       PTR_ERR(periph_regs));
+		return;
+	}
+
+	/* Switch to using the fast counter clock */
+	ret = regmap_update_bits(periph_regs, PERIP_TIMER_CONTROL,
+				 0xf, 0x0);
+	if (ret)
+		return;
+
+	sys_clk = of_clk_get_by_name(node, "sys");
+	if (IS_ERR(sys_clk)) {
+		pr_err("clock get failed (%lu)\n", PTR_ERR(sys_clk));
+		return;
+	}
+
+	fast_clk = of_clk_get_by_name(node, "fast");
+	if (IS_ERR(fast_clk)) {
+		pr_err("clock get failed (%lu)\n", PTR_ERR(fast_clk));
+		return;
+	}
+
+	ret = clk_prepare_enable(sys_clk);
+	if (ret < 0) {
+		pr_err("failed to enable clock (%d)\n", ret);
+		return;
+	}
+
+	ret = clk_prepare_enable(fast_clk);
+	if (ret < 0) {
+		pr_err("failed to enable clock (%d)\n", ret);
+		clk_disable_unprepare(sys_clk);
+		return;
+	}
+
+	rate = clk_get_rate(fast_clk);
+
+	/* Disable irq's for clocksource usage */
+	gpt_writel(0, TIMER_IRQ_MASK, 0);
+	gpt_writel(0, TIMER_IRQ_MASK, 1);
+	gpt_writel(0, TIMER_IRQ_MASK, 2);
+	gpt_writel(0, TIMER_IRQ_MASK, 3);
+
+	/* Enable timer block */
+	__raw_writel(TIMER_ME_GLOBAL, timer_base);
+
+	sched_clock_register(pistachio_read_sched_clock, 32, rate);
+	clocksource_register_hz(&clocksource_gpt, rate);
+}
+
+static const struct of_device_id pistachio_clksrc_of_match[] __initconst = {
+	{ .compatible = "img,pistachio-gptimer" },
+	{ },
+};
+CLOCKSOURCE_OF_DECLARE(pistachio_gptimer, "img,pistachio-gptimer",
+		       pistachio_clksrc_of_init);
-- 
2.3.3

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/7] clocksource: Add Pistachio clocksource-only driver

[Thomas Gleixner]

On Thu, 21 May 2015, Ezequiel Garcia wrote:
> +static cycle_t clocksource_read_cycles(struct clocksource *cs)
> +{
> +	u32 counter, overflw;
> +	unsigned long flags;
> +
> +	raw_spin_lock_irqsave(&lock, flags);

Hmm. Is that lock really necessary to read that counter? The
clocksource is global. And if its actually used for timekeeping, the
lock can get heavy contended.

> +	overflw = gpt_readl(TIMER_CURRENT_OVERFLOW_VALUE, 0);
> +	counter = gpt_readl(TIMER_CURRENT_VALUE, 0);
> +	raw_spin_unlock_irqrestore(&lock, flags);
> +
> +	return ~(cycle_t)counter;
> +}

Thanks,

	tglx

Re: [PATCH 6/7] clocksource: Add Pistachio clocksource-only driver

[Ezequiel Garcia]

On 05/21/2015 07:00 PM, Thomas Gleixner wrote:
> On Thu, 21 May 2015, Ezequiel Garcia wrote:
>> +static cycle_t clocksource_read_cycles(struct clocksource *cs)
>> +{
>> +	u32 counter, overflw;
>> +	unsigned long flags;
>> +
>> +	raw_spin_lock_irqsave(&lock, flags);
> 
> Hmm. Is that lock really necessary to read that counter? The
> clocksource is global. And if its actually used for timekeeping, the
> lock can get heavy contended.
> 

Yup, it is really (and sadly) necessary. The kernel hangs up completely
without it when the counter is accesed by more than one CPU.

Apparently, those two timer registers overflow and counter must be read
atomically.

>> +	overflw = gpt_readl(TIMER_CURRENT_OVERFLOW_VALUE, 0);
>> +	counter = gpt_readl(TIMER_CURRENT_VALUE, 0);
>> +	raw_spin_unlock_irqrestore(&lock, flags);
>> +
>> +	return ~(cycle_t)counter;
>> +}
> 
> Thanks,
> 
> 	tglx
> 

-- 
Ezequiel
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/7] clocksource: Add Pistachio clocksource-only driver

[Thomas Gleixner]

On Thu, 21 May 2015, Ezequiel Garcia wrote:
> On 05/21/2015 07:00 PM, Thomas Gleixner wrote:
> > On Thu, 21 May 2015, Ezequiel Garcia wrote:
> >> +static cycle_t clocksource_read_cycles(struct clocksource *cs)
> >> +{
> >> +	u32 counter, overflw;
> >> +	unsigned long flags;
> >> +
> >> +	raw_spin_lock_irqsave(&lock, flags);
> > 
> > Hmm. Is that lock really necessary to read that counter? The
> > clocksource is global. And if its actually used for timekeeping, the
> > lock can get heavy contended.
> > 
> 
> Yup, it is really (and sadly) necessary. The kernel hangs up completely
> without it when the counter is accesed by more than one CPU.
> 
> Apparently, those two timer registers overflow and counter must be read
> atomically.

Welcome to the wonderful world of useless timer hardware.

	tglx
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/7] clocksource: Add Pistachio clocksource-only driver

[Andrew Bresticker]

On Thu, May 21, 2015 at 2:41 PM, Ezequiel Garcia
<ezequiel.garcia@imgtec.com> wrote:
> The Pistachio SoC provides four general purpose timers, and allow
> to implement a clocksource driver.
>
> This driver can be used as a replacement for the MIPS GIC and MIPS R4K
> clocksources and sched clocks, which are clocked from the CPU clock.
>
> Given the general purpose timers are clocked from an independent clock,
> this new clocksource driver will be useful to introduce CPUFreq support
> for Pistachio machines.
>
> Signed-off-by: Govindraj Raja <Govindraj.Raja@imgtec.com>
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@imgtec.com>

> --- /dev/null
> +++ b/drivers/clocksource/time-pistachio.c

> @@ -0,0 +1,202 @@
> +/*
> + * Pistachio clocksource based on general-purpose timers
> + *
> + * Copyright (C) 2015 Imagination Technologies
> + *
> + * This file is subject to the terms and conditions of the GNU General Public
> + * License. See the file "COPYING" in the main directory of this archive
> + * for more details.
> + */
> +
> +#define pr_fmt(fmt) "%s: " fmt, __func__
> +
> +#include <linux/clk.h>
> +#include <linux/clocksource.h>
> +#include <linux/clockchips.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/spinlock.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/sched_clock.h>
> +#include <linux/time.h>
> +
> +/* Top level reg */
> +#define        CR_TIMER_CTRL_CFG               0x00
> +  #define TIMER_ME_GLOBAL              BIT(0)
> +#define        CR_TIMER_REV                    0x10
> +
> +/* Timer specific registers */
> +#define TIMER_CFG                      0x20
> +  #define TIMER_ME_LOCAL               BIT(0)
> +#define TIMER_RELOAD_VALUE             0x24
> +#define TIMER_CURRENT_VALUE            0x28
> +#define TIMER_CURRENT_OVERFLOW_VALUE   0x2C
> +#define TIMER_IRQ_STATUS               0x30
> +#define TIMER_IRQ_CLEAR                        0x34
> +#define TIMER_IRQ_MASK                 0x38

nit: the spacing in these two sets of #defines is inconsistent and the
space at the beginning of the line looks a little weird to me, maybe
just do something like this:

#define REGISTER ...
#define  REGISTER_FIELD ...

> +
> +#define PERIP_TIMER_CONTROL            0x90
> +
> +/* Timer specific configuration Values */
> +#define RELOAD_VALUE   0xffffffff
> +
> +static void __iomem *timer_base;
> +static DEFINE_RAW_SPINLOCK(lock);
> +
> +static inline u32 gpt_readl(u32 offset, u32 gpt_id)
> +{
> +       return __raw_readl(timer_base + 0x20 * gpt_id + offset);
> +}
> +
> +static inline void gpt_writel(u32 value, u32 offset, u32 gpt_id)
> +{
> +       __raw_writel(value, timer_base + 0x20 * gpt_id + offset);
> +}

Why raw iomem accessors?

> +static cycle_t clocksource_read_cycles(struct clocksource *cs)
> +{
> +       u32 counter, overflw;
> +       unsigned long flags;
> +
> +       raw_spin_lock_irqsave(&lock, flags);
> +       overflw = gpt_readl(TIMER_CURRENT_OVERFLOW_VALUE, 0);
> +       counter = gpt_readl(TIMER_CURRENT_VALUE, 0);
> +       raw_spin_unlock_irqrestore(&lock, flags);
> +
> +       return ~(cycle_t)counter;
> +}
> +
> +static u64 notrace pistachio_read_sched_clock(void)
> +{
> +       return clocksource_read_cycles(NULL);
> +}
> +
> +static void pistachio_clksrc_enable(int timeridx)
> +{
> +       u32 val;
> +
> +       /* Disable GPT local before loading reload value */
> +       val = gpt_readl(TIMER_CFG, timeridx);
> +       val &= ~TIMER_ME_LOCAL;
> +       gpt_writel(val, TIMER_CFG, timeridx);
> +
> +       gpt_writel(RELOAD_VALUE, TIMER_RELOAD_VALUE, timeridx);
> +
> +       val = gpt_readl(TIMER_CFG, timeridx);
> +       val |= TIMER_ME_LOCAL;
> +       gpt_writel(val, TIMER_CFG, timeridx);
> +}
> +
> +static void pistachio_clksrc_disable(int timeridx)
> +{
> +       u32 val;
> +
> +       /* Disable GPT local */
> +       val = gpt_readl(TIMER_CFG, timeridx);
> +       val &= ~TIMER_ME_LOCAL;
> +       gpt_writel(val, TIMER_CFG, timeridx);
> +}
> +
> +static int clocksource_enable(struct clocksource *cs)
> +{
> +       pistachio_clksrc_enable(0);
> +       return 0;
> +}
> +
> +static void clocksource_disable(struct clocksource *cs)
> +{
> +       pistachio_clksrc_disable(0);
> +}
> +
> +/* Desirable clock source for pistachio platform */
> +static struct clocksource clocksource_gpt = {
> +       .name           = "gptimer",
> +       .rating         = 300,
> +       .enable         = clocksource_enable,
> +       .disable        = clocksource_disable,
> +       .read           = clocksource_read_cycles,

nit: these names are rather generic sounding, maybe add a "pistachio" prefix?

> +       .mask           = CLOCKSOURCE_MASK(32),
> +       .flags          = CLOCK_SOURCE_IS_CONTINUOUS |
> +                         CLOCK_SOURCE_SUSPEND_NONSTOP,
> +};
> +
> +static void __init pistachio_clksrc_of_init(struct device_node *node)
> +{
> +       struct clk *sys_clk, *fast_clk;
> +       struct regmap *periph_regs;
> +       unsigned long rate;
> +       int ret;
> +
> +       timer_base = of_iomap(node, 0);
> +       if (!timer_base) {
> +               pr_err("cannot iomap\n");
> +               return;
> +       }
> +
> +       /*
> +        * We need early syscon or late clocksource probe for this to work.
> +        */

I don't think this is true... if the syscon hasn't probed yet, then
syscon_regmap_lookup_by_phandle will probe it for us.

> +       periph_regs = syscon_regmap_lookup_by_phandle(node, "img,cr-periph");
> +       if (IS_ERR(periph_regs)) {
> +               pr_err("cannot get peripheral regmap (%lu)\n",
> +                      PTR_ERR(periph_regs));
> +               return;
> +       }
> +
> +       /* Switch to using the fast counter clock */
> +       ret = regmap_update_bits(periph_regs, PERIP_TIMER_CONTROL,
> +                                0xf, 0x0);
> +       if (ret)
> +               return;
> +
> +       sys_clk = of_clk_get_by_name(node, "sys");
> +       if (IS_ERR(sys_clk)) {
> +               pr_err("clock get failed (%lu)\n", PTR_ERR(sys_clk));
> +               return;
> +       }
> +
> +       fast_clk = of_clk_get_by_name(node, "fast");
> +       if (IS_ERR(fast_clk)) {
> +               pr_err("clock get failed (%lu)\n", PTR_ERR(fast_clk));
> +               return;
> +       }
> +
> +       ret = clk_prepare_enable(sys_clk);
> +       if (ret < 0) {
> +               pr_err("failed to enable clock (%d)\n", ret);
> +               return;
> +       }
> +
> +       ret = clk_prepare_enable(fast_clk);
> +       if (ret < 0) {
> +               pr_err("failed to enable clock (%d)\n", ret);
> +               clk_disable_unprepare(sys_clk);
> +               return;
> +       }
> +
> +       rate = clk_get_rate(fast_clk);
> +
> +       /* Disable irq's for clocksource usage */
> +       gpt_writel(0, TIMER_IRQ_MASK, 0);
> +       gpt_writel(0, TIMER_IRQ_MASK, 1);
> +       gpt_writel(0, TIMER_IRQ_MASK, 2);
> +       gpt_writel(0, TIMER_IRQ_MASK, 3);
> +
> +       /* Enable timer block */
> +       __raw_writel(TIMER_ME_GLOBAL, timer_base);
> +
> +       sched_clock_register(pistachio_read_sched_clock, 32, rate);
> +       clocksource_register_hz(&clocksource_gpt, rate);
> +}
> +
> +static const struct of_device_id pistachio_clksrc_of_match[] __initconst = {
> +       { .compatible = "img,pistachio-gptimer" },
> +       { },
> +};

This table doesn't appear to be used anywhere.

> +CLOCKSOURCE_OF_DECLARE(pistachio_gptimer, "img,pistachio-gptimer",
> +                      pistachio_clksrc_of_init);
> --
> 2.3.3
>

Re: [PATCH 6/7] clocksource: Add Pistachio clocksource-only driver

[Ezequiel Garcia]

On 05/22/2015 01:48 PM, Andrew Bresticker wrote:
> On Thu, May 21, 2015 at 2:41 PM, Ezequiel Garcia
> <ezequiel.garcia-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org> wrote:
>> The Pistachio SoC provides four general purpose timers, and allow
>> to implement a clocksource driver.
>>
>> This driver can be used as a replacement for the MIPS GIC and MIPS R4K
>> clocksources and sched clocks, which are clocked from the CPU clock.
>>
>> Given the general purpose timers are clocked from an independent clock,
>> this new clocksource driver will be useful to introduce CPUFreq support
>> for Pistachio machines.
>>
>> Signed-off-by: Govindraj Raja <Govindraj.Raja-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
>> Signed-off-by: Ezequiel Garcia <ezequiel.garcia-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
> 
>> --- /dev/null
>> +++ b/drivers/clocksource/time-pistachio.c
> 
>> @@ -0,0 +1,202 @@
>> +/*
>> + * Pistachio clocksource based on general-purpose timers
>> + *
>> + * Copyright (C) 2015 Imagination Technologies
>> + *
>> + * This file is subject to the terms and conditions of the GNU General Public
>> + * License. See the file "COPYING" in the main directory of this archive
>> + * for more details.
>> + */
>> +
>> +#define pr_fmt(fmt) "%s: " fmt, __func__
>> +
>> +#include <linux/clk.h>
>> +#include <linux/clocksource.h>
>> +#include <linux/clockchips.h>
>> +#include <linux/delay.h>
>> +#include <linux/err.h>
>> +#include <linux/init.h>
>> +#include <linux/spinlock.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +#include <linux/sched_clock.h>
>> +#include <linux/time.h>
>> +
>> +/* Top level reg */
>> +#define        CR_TIMER_CTRL_CFG               0x00
>> +  #define TIMER_ME_GLOBAL              BIT(0)
>> +#define        CR_TIMER_REV                    0x10
>> +
>> +/* Timer specific registers */
>> +#define TIMER_CFG                      0x20
>> +  #define TIMER_ME_LOCAL               BIT(0)
>> +#define TIMER_RELOAD_VALUE             0x24
>> +#define TIMER_CURRENT_VALUE            0x28
>> +#define TIMER_CURRENT_OVERFLOW_VALUE   0x2C
>> +#define TIMER_IRQ_STATUS               0x30
>> +#define TIMER_IRQ_CLEAR                        0x34
>> +#define TIMER_IRQ_MASK                 0x38
> 
> nit: the spacing in these two sets of #defines is inconsistent and the
> space at the beginning of the line looks a little weird to me, maybe
> just do something like this:
> 
> #define REGISTER ...
> #define  REGISTER_FIELD ...
> 

Done.

>> +
>> +#define PERIP_TIMER_CONTROL            0x90
>> +
>> +/* Timer specific configuration Values */
>> +#define RELOAD_VALUE   0xffffffff
>> +
>> +static void __iomem *timer_base;
>> +static DEFINE_RAW_SPINLOCK(lock);
>> +
>> +static inline u32 gpt_readl(u32 offset, u32 gpt_id)
>> +{
>> +       return __raw_readl(timer_base + 0x20 * gpt_id + offset);
>> +}
>> +
>> +static inline void gpt_writel(u32 value, u32 offset, u32 gpt_id)
>> +{
>> +       __raw_writel(value, timer_base + 0x20 * gpt_id + offset);
>> +}
> 
> Why raw iomem accessors?
> 

For no good reason, it was a blind copy-paste. I'll change that to
standard accessors.

>> +static cycle_t clocksource_read_cycles(struct clocksource *cs)
>> +{
>> +       u32 counter, overflw;
>> +       unsigned long flags;
>> +
>> +       raw_spin_lock_irqsave(&lock, flags);
>> +       overflw = gpt_readl(TIMER_CURRENT_OVERFLOW_VALUE, 0);
>> +       counter = gpt_readl(TIMER_CURRENT_VALUE, 0);
>> +       raw_spin_unlock_irqrestore(&lock, flags);
>> +
>> +       return ~(cycle_t)counter;
>> +}
>> +
>> +static u64 notrace pistachio_read_sched_clock(void)
>> +{
>> +       return clocksource_read_cycles(NULL);
>> +}
>> +
>> +static void pistachio_clksrc_enable(int timeridx)
>> +{
>> +       u32 val;
>> +
>> +       /* Disable GPT local before loading reload value */
>> +       val = gpt_readl(TIMER_CFG, timeridx);
>> +       val &= ~TIMER_ME_LOCAL;
>> +       gpt_writel(val, TIMER_CFG, timeridx);
>> +
>> +       gpt_writel(RELOAD_VALUE, TIMER_RELOAD_VALUE, timeridx);
>> +
>> +       val = gpt_readl(TIMER_CFG, timeridx);
>> +       val |= TIMER_ME_LOCAL;
>> +       gpt_writel(val, TIMER_CFG, timeridx);
>> +}
>> +
>> +static void pistachio_clksrc_disable(int timeridx)
>> +{
>> +       u32 val;
>> +
>> +       /* Disable GPT local */
>> +       val = gpt_readl(TIMER_CFG, timeridx);
>> +       val &= ~TIMER_ME_LOCAL;
>> +       gpt_writel(val, TIMER_CFG, timeridx);
>> +}
>> +
>> +static int clocksource_enable(struct clocksource *cs)
>> +{
>> +       pistachio_clksrc_enable(0);
>> +       return 0;
>> +}
>> +
>> +static void clocksource_disable(struct clocksource *cs)
>> +{
>> +       pistachio_clksrc_disable(0);
>> +}
>> +
>> +/* Desirable clock source for pistachio platform */
>> +static struct clocksource clocksource_gpt = {
>> +       .name           = "gptimer",
>> +       .rating         = 300,
>> +       .enable         = clocksource_enable,
>> +       .disable        = clocksource_disable,
>> +       .read           = clocksource_read_cycles,
> 
> nit: these names are rather generic sounding, maybe add a "pistachio" prefix?
> 

Sure.

>> +       .mask           = CLOCKSOURCE_MASK(32),
>> +       .flags          = CLOCK_SOURCE_IS_CONTINUOUS |
>> +                         CLOCK_SOURCE_SUSPEND_NONSTOP,
>> +};
>> +
>> +static void __init pistachio_clksrc_of_init(struct device_node *node)
>> +{
>> +       struct clk *sys_clk, *fast_clk;
>> +       struct regmap *periph_regs;
>> +       unsigned long rate;
>> +       int ret;
>> +
>> +       timer_base = of_iomap(node, 0);
>> +       if (!timer_base) {
>> +               pr_err("cannot iomap\n");
>> +               return;
>> +       }
>> +
>> +       /*
>> +        * We need early syscon or late clocksource probe for this to work.
>> +        */
> 
> I don't think this is true... if the syscon hasn't probed yet, then
> syscon_regmap_lookup_by_phandle will probe it for us.
> 

Ah, that comment is a left over from early experiments.

>> +       periph_regs = syscon_regmap_lookup_by_phandle(node, "img,cr-periph");
>> +       if (IS_ERR(periph_regs)) {
>> +               pr_err("cannot get peripheral regmap (%lu)\n",
>> +                      PTR_ERR(periph_regs));
>> +               return;
>> +       }
>> +
>> +       /* Switch to using the fast counter clock */
>> +       ret = regmap_update_bits(periph_regs, PERIP_TIMER_CONTROL,
>> +                                0xf, 0x0);
>> +       if (ret)
>> +               return;
>> +
>> +       sys_clk = of_clk_get_by_name(node, "sys");
>> +       if (IS_ERR(sys_clk)) {
>> +               pr_err("clock get failed (%lu)\n", PTR_ERR(sys_clk));
>> +               return;
>> +       }
>> +
>> +       fast_clk = of_clk_get_by_name(node, "fast");
>> +       if (IS_ERR(fast_clk)) {
>> +               pr_err("clock get failed (%lu)\n", PTR_ERR(fast_clk));
>> +               return;
>> +       }
>> +
>> +       ret = clk_prepare_enable(sys_clk);
>> +       if (ret < 0) {
>> +               pr_err("failed to enable clock (%d)\n", ret);
>> +               return;
>> +       }
>> +
>> +       ret = clk_prepare_enable(fast_clk);
>> +       if (ret < 0) {
>> +               pr_err("failed to enable clock (%d)\n", ret);
>> +               clk_disable_unprepare(sys_clk);
>> +               return;
>> +       }
>> +
>> +       rate = clk_get_rate(fast_clk);
>> +
>> +       /* Disable irq's for clocksource usage */
>> +       gpt_writel(0, TIMER_IRQ_MASK, 0);
>> +       gpt_writel(0, TIMER_IRQ_MASK, 1);
>> +       gpt_writel(0, TIMER_IRQ_MASK, 2);
>> +       gpt_writel(0, TIMER_IRQ_MASK, 3);
>> +
>> +       /* Enable timer block */
>> +       __raw_writel(TIMER_ME_GLOBAL, timer_base);
>> +
>> +       sched_clock_register(pistachio_read_sched_clock, 32, rate);
>> +       clocksource_register_hz(&clocksource_gpt, rate);
>> +}
>> +
>> +static const struct of_device_id pistachio_clksrc_of_match[] __initconst = {
>> +       { .compatible = "img,pistachio-gptimer" },
>> +       { },
>> +};
> 
> This table doesn't appear to be used anywhere.
> 

Left over from same experiments.

>> +CLOCKSOURCE_OF_DECLARE(pistachio_gptimer, "img,pistachio-gptimer",
>> +                      pistachio_clksrc_of_init);
>> --
>> 2.3.3
>>

Thanks for the review!
-- 
Ezequiel
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 5/7] clocksource: Add Pistachio SoC general purpose timer binding document

[Ezequiel Garcia]

Add a device-tree binding document for the clocksource driver provided
by Pistachio SoC general purpose timers.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@imgtec.com>
---
 .../bindings/timer/img,pistachio-gptimer.txt       | 29 ++++++++++++++++++++++
 1 file changed, 29 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/timer/img,pistachio-gptimer.txt

diff --git a/Documentation/devicetree/bindings/timer/img,pistachio-gptimer.txt b/Documentation/devicetree/bindings/timer/img,pistachio-gptimer.txt
new file mode 100644
index 0000000..f1b7c08
--- /dev/null
+++ b/Documentation/devicetree/bindings/timer/img,pistachio-gptimer.txt
@@ -0,0 +1,29 @@
+* Pistachio general-purpose timer based clocksource
+
+Required properties:
+ - compatible: "img,pistachio-gptimer".
+ - reg: Address range of the timer registers.
+ - interrupts: An interrupt for each of the four timers
+ - clocks : Should contain a clock specifier for each entry in clock-names
+ - clock-names : Should contain the following entries:
+                "sys", interface clock
+                "slow", slow counter clock
+                "fast", fast counter clock
+ - img,cr-periph: Must contain a phandle to the peripheral control
+		  syscon node.
+
+Example:
+	timer: timer@18102000 {
+		compatible = "img,pistachio-gptimer";
+		reg = <0x18102000 0x100>;
+		interrupts = <GIC_SHARED 60 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SHARED 61 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SHARED 62 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SHARED 63 IRQ_TYPE_LEVEL_HIGH>;
+		clocks = <&clk_periph PERIPH_CLK_COUNTER_FAST>,
+		         <&clk_periph PERIPH_CLK_COUNTER_SLOW>,
+			 <&cr_periph SYS_CLK_TIMER>;
+		clock-names = "fast", "slow", "sys";
+		img,cr-periph = <&cr_periph>;
+	};
+
-- 
2.3.3

Re: [PATCH 5/7] clocksource: Add Pistachio SoC general purpose timer binding document

[Andrew Bresticker]

On Thu, May 21, 2015 at 2:41 PM, Ezequiel Garcia
<ezequiel.garcia@imgtec.com> wrote:
> Add a device-tree binding document for the clocksource driver provided
> by Pistachio SoC general purpose timers.
>
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@imgtec.com>

Reviewed-by: Andrew Bresticker <abrestic@chromium.org>

[PATCH 7/7] mips: pistachio: Allow to enable the external timer based clocksource

[Ezequiel Garcia]

This commit introduces a new config, so the user can choose to enable
the General Purpose Timer based clocksource. This option is required
to have CPUFreq support.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@imgtec.com>
---
 arch/mips/Kconfig           |  1 +
 arch/mips/pistachio/Kconfig | 13 +++++++++++++
 2 files changed, 14 insertions(+)
 create mode 100644 arch/mips/pistachio/Kconfig

diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
index f501665..91f6ca0 100644
--- a/arch/mips/Kconfig
+++ b/arch/mips/Kconfig
@@ -934,6 +934,7 @@ source "arch/mips/jazz/Kconfig"
 source "arch/mips/jz4740/Kconfig"
 source "arch/mips/lantiq/Kconfig"
 source "arch/mips/lasat/Kconfig"
+source "arch/mips/pistachio/Kconfig"
 source "arch/mips/pmcs-msp71xx/Kconfig"
 source "arch/mips/ralink/Kconfig"
 source "arch/mips/sgi-ip27/Kconfig"
diff --git a/arch/mips/pistachio/Kconfig b/arch/mips/pistachio/Kconfig
new file mode 100644
index 0000000..97731ea
--- /dev/null
+++ b/arch/mips/pistachio/Kconfig
@@ -0,0 +1,13 @@
+config PISTACHIO_GPTIMER_CLKSRC
+	bool "Enable General Purpose Timer based clocksource"
+	depends on MACH_PISTACHIO
+	select CLKSRC_PISTACHIO
+	select MIPS_EXTERNAL_TIMER
+	help
+	  This option enables a clocksource driver based on a Pistachio
+	  SoC General Purpose external timer.
+
+	  If you want to enable the CPUFreq, you need to enable
+	  this option.
+
+	  If you don't want to enable CPUFreq, you can leave this disabled.
-- 
2.3.3

Re: [PATCH 7/7] mips: pistachio: Allow to enable the external timer based clocksource

[Andrew Bresticker]

On Thu, May 21, 2015 at 2:43 PM, Ezequiel Garcia
<ezequiel.garcia-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org> wrote:
> This commit introduces a new config, so the user can choose to enable
> the General Purpose Timer based clocksource. This option is required
> to have CPUFreq support.
>
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
> ---
>  arch/mips/Kconfig           |  1 +
>  arch/mips/pistachio/Kconfig | 13 +++++++++++++
>  2 files changed, 14 insertions(+)
>  create mode 100644 arch/mips/pistachio/Kconfig
>
> diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
> index f501665..91f6ca0 100644
> --- a/arch/mips/Kconfig
> +++ b/arch/mips/Kconfig
> @@ -934,6 +934,7 @@ source "arch/mips/jazz/Kconfig"
>  source "arch/mips/jz4740/Kconfig"
>  source "arch/mips/lantiq/Kconfig"
>  source "arch/mips/lasat/Kconfig"
> +source "arch/mips/pistachio/Kconfig"
>  source "arch/mips/pmcs-msp71xx/Kconfig"
>  source "arch/mips/ralink/Kconfig"
>  source "arch/mips/sgi-ip27/Kconfig"
> diff --git a/arch/mips/pistachio/Kconfig b/arch/mips/pistachio/Kconfig
> new file mode 100644
> index 0000000..97731ea
> --- /dev/null
> +++ b/arch/mips/pistachio/Kconfig
> @@ -0,0 +1,13 @@
> +config PISTACHIO_GPTIMER_CLKSRC
> +       bool "Enable General Purpose Timer based clocksource"
> +       depends on MACH_PISTACHIO
> +       select CLKSRC_PISTACHIO
> +       select MIPS_EXTERNAL_TIMER

Why not just select these in the MACH_PISTACHIO Kconfig entry?  Is
there any harm in always having the Pistachio GPT enabled?
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 7/7] mips: pistachio: Allow to enable the external timer based clocksource

[James Hartley]

> -----Original Message-----
> From: abrestic@google.com [mailto:abrestic@google.com] On Behalf Of
> Andrew Bresticker
> Sent: 22 May 2015 17:50
> To: Ezequiel Garcia
> Cc: linux-kernel@vger.kernel.org; Linux-MIPS; Daniel Lezcano;
> devicetree@vger.kernel.org; James Hartley; James Hogan; Thomas Gleixner;
> Damien Horsley; Govindraj Raja
> Subject: Re: [PATCH 7/7] mips: pistachio: Allow to enable the external timer
> based clocksource
> 
> On Thu, May 21, 2015 at 2:43 PM, Ezequiel Garcia
> <ezequiel.garcia@imgtec.com> wrote:
> > This commit introduces a new config, so the user can choose to enable
> > the General Purpose Timer based clocksource. This option is required
> > to have CPUFreq support.
> >
> > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@imgtec.com>
> > ---
> >  arch/mips/Kconfig           |  1 +
> >  arch/mips/pistachio/Kconfig | 13 +++++++++++++
> >  2 files changed, 14 insertions(+)
> >  create mode 100644 arch/mips/pistachio/Kconfig
> >
> > diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig index
> > f501665..91f6ca0 100644
> > --- a/arch/mips/Kconfig
> > +++ b/arch/mips/Kconfig
> > @@ -934,6 +934,7 @@ source "arch/mips/jazz/Kconfig"
> >  source "arch/mips/jz4740/Kconfig"
> >  source "arch/mips/lantiq/Kconfig"
> >  source "arch/mips/lasat/Kconfig"
> > +source "arch/mips/pistachio/Kconfig"
> >  source "arch/mips/pmcs-msp71xx/Kconfig"
> >  source "arch/mips/ralink/Kconfig"
> >  source "arch/mips/sgi-ip27/Kconfig"
> > diff --git a/arch/mips/pistachio/Kconfig b/arch/mips/pistachio/Kconfig
> > new file mode 100644 index 0000000..97731ea
> > --- /dev/null
> > +++ b/arch/mips/pistachio/Kconfig
> > @@ -0,0 +1,13 @@
> > +config PISTACHIO_GPTIMER_CLKSRC
> > +       bool "Enable General Purpose Timer based clocksource"
> > +       depends on MACH_PISTACHIO
> > +       select CLKSRC_PISTACHIO
> > +       select MIPS_EXTERNAL_TIMER
> 
> Why not just select these in the MACH_PISTACHIO Kconfig entry?  Is there any
> harm in always having the Pistachio GPT enabled?

It does mean that there are less GPT's available for other users, and whilst I'm not aware of any use cases that currently require all 4, perhaps having the flexibility is worth it.

James

Re: [PATCH 7/7] mips: pistachio: Allow to enable the external timer based clocksource

[Ezequiel Garcia]

On 05/22/2015 01:58 PM, James Hartley wrote:
> 
> 
>> -----Original Message-----
>> From: abrestic@google.com [mailto:abrestic@google.com] On Behalf Of
>> Andrew Bresticker
>> Sent: 22 May 2015 17:50
>> To: Ezequiel Garcia
>> Cc: linux-kernel@vger.kernel.org; Linux-MIPS; Daniel Lezcano;
>> devicetree@vger.kernel.org; James Hartley; James Hogan; Thomas Gleixner;
>> Damien Horsley; Govindraj Raja
>> Subject: Re: [PATCH 7/7] mips: pistachio: Allow to enable the external timer
>> based clocksource
>>
>> On Thu, May 21, 2015 at 2:43 PM, Ezequiel Garcia
>> <ezequiel.garcia@imgtec.com> wrote:
>>> This commit introduces a new config, so the user can choose to enable
>>> the General Purpose Timer based clocksource. This option is required
>>> to have CPUFreq support.
>>>
>>> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@imgtec.com>
>>> ---
>>>  arch/mips/Kconfig           |  1 +
>>>  arch/mips/pistachio/Kconfig | 13 +++++++++++++
>>>  2 files changed, 14 insertions(+)
>>>  create mode 100644 arch/mips/pistachio/Kconfig
>>>
>>> diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig index
>>> f501665..91f6ca0 100644
>>> --- a/arch/mips/Kconfig
>>> +++ b/arch/mips/Kconfig
>>> @@ -934,6 +934,7 @@ source "arch/mips/jazz/Kconfig"
>>>  source "arch/mips/jz4740/Kconfig"
>>>  source "arch/mips/lantiq/Kconfig"
>>>  source "arch/mips/lasat/Kconfig"
>>> +source "arch/mips/pistachio/Kconfig"
>>>  source "arch/mips/pmcs-msp71xx/Kconfig"
>>>  source "arch/mips/ralink/Kconfig"
>>>  source "arch/mips/sgi-ip27/Kconfig"
>>> diff --git a/arch/mips/pistachio/Kconfig b/arch/mips/pistachio/Kconfig
>>> new file mode 100644 index 0000000..97731ea
>>> --- /dev/null
>>> +++ b/arch/mips/pistachio/Kconfig
>>> @@ -0,0 +1,13 @@
>>> +config PISTACHIO_GPTIMER_CLKSRC
>>> +       bool "Enable General Purpose Timer based clocksource"
>>> +       depends on MACH_PISTACHIO
>>> +       select CLKSRC_PISTACHIO
>>> +       select MIPS_EXTERNAL_TIMER
>>
>> Why not just select these in the MACH_PISTACHIO Kconfig entry?  Is there any
>> harm in always having the Pistachio GPT enabled?
> 
> It does mean that there are less GPT's available for other users, and whilst I'm not aware of any use cases that currently require all 4, perhaps having the flexibility is worth it.
> 

And also, this is only useful if the user wants CPUFreq. Otherwise we
have MIPS GIC and R4K for clockevents and clocksource. Not sure why we'd
want another one.

-- 
Ezequiel