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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
Add a device-tree binding document for the clocksource driver provided
by Pistachio SoC general purpose timers.
Signed-off-by: Ezequiel Garcia <[email protected]>
---
.../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
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 <[email protected]>
Signed-off-by: Ezequiel Garcia <[email protected]>
---
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
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 <[email protected]>
---
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
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
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
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
On Thu, May 21, 2015 at 2:37 PM, Ezequiel Garcia
<[email protected]> 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 <[email protected]>
Reviewed-by: Andrew Bresticker <[email protected]>
On Thu, May 21, 2015 at 2:37 PM, Ezequiel Garcia
<[email protected]> 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 <[email protected]>
Reviewed-by: Andrew Bresticker <[email protected]>
On Thu, May 21, 2015 at 2:37 PM, Ezequiel Garcia
<[email protected]> 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 <[email protected]>
> ---
> 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?
On 05/21/2015 07:24 PM, Andrew Bresticker wrote:
> On Thu, May 21, 2015 at 2:37 PM, Ezequiel Garcia
> <[email protected]> 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 <[email protected]>
>> ---
>> 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
On 05/21/2015 07:32 PM, Andrew Bresticker wrote:
> On Thu, May 21, 2015 at 2:37 PM, Ezequiel Garcia
> <[email protected]> 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 <[email protected]>
>> ---
>> 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
On Thu, May 21, 2015 at 2:37 PM, Ezequiel Garcia
<[email protected]> 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 <[email protected]>
> ---
> 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");
> + }
On Thu, May 21, 2015 at 2:41 PM, Ezequiel Garcia
<[email protected]> wrote:
> Add a device-tree binding document for the clocksource driver provided
> by Pistachio SoC general purpose timers.
>
> Signed-off-by: Ezequiel Garcia <[email protected]>
Reviewed-by: Andrew Bresticker <[email protected]>
On Thu, May 21, 2015 at 2:41 PM, Ezequiel Garcia
<[email protected]> 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 <[email protected]>
> Signed-off-by: Ezequiel Garcia <[email protected]>
> --- /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
>
On Thu, May 21, 2015 at 2:43 PM, Ezequiel Garcia
<[email protected]> 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 <[email protected]>
> ---
> 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?
On Thu, May 21, 2015 at 3:25 PM, Ezequiel Garcia
<[email protected]> wrote:
>
>
> On 05/21/2015 07:24 PM, Andrew Bresticker wrote:
>> On Thu, May 21, 2015 at 2:37 PM, Ezequiel Garcia
>> <[email protected]> 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 <[email protected]>
>>> ---
>>> 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 <[email protected]>
> -----Original Message-----
> From: [email protected] [mailto:[email protected]] On Behalf Of
> Andrew Bresticker
> Sent: 22 May 2015 17:50
> To: Ezequiel Garcia
> Cc: [email protected]; Linux-MIPS; Daniel Lezcano;
> [email protected]; 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
> <[email protected]> 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 <[email protected]>
> > ---
> > 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
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On 05/22/2015 01:58 PM, James Hartley wrote:
>
>
>> -----Original Message-----
>> From: [email protected] [mailto:[email protected]] On Behalf Of
>> Andrew Bresticker
>> Sent: 22 May 2015 17:50
>> To: Ezequiel Garcia
>> Cc: [email protected]; Linux-MIPS; Daniel Lezcano;
>> [email protected]; 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
>> <[email protected]> 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 <[email protected]>
>>> ---
>>> 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
On 05/22/2015 01:48 PM, Andrew Bresticker wrote:
> On Thu, May 21, 2015 at 2:41 PM, Ezequiel Garcia
> <[email protected]> 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 <[email protected]>
>> Signed-off-by: Ezequiel Garcia <[email protected]>
>
>> --- /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