ARM: clps711x: Initial DT support / Fixes

This is a partial revert of the patch:
"6597619f9c ARM: clps711x: Add support for SYSCON driver". No reason to make SYSCON driver name unique to that processor.

Signed-off-by Alexander Shiyan <[email protected]>

This is a partial revert of the patch:
"6597619f9c ARM: clps711x: Add support for SYSCON driver". No reason to make SYSCON driver name unique to that processor.

This patch removes support for the fortunet board. This board is not maintained by long time and it seems no one is not using it.

Signed-off-by Alexander Shiyan <[email protected]>

This patch removes incorrect config symbols checking since these symbols not contain "CONFIG_" prefix. Anyway, checking is unneeded here and this patch remove it completely.

Signed-off-by Alexander Shiyan <[email protected]>

The keyboard driver for the CLPS711X platform is missing, so there is no need to call iotable_init() for this memory region.

Signed-off-by Alexander Shiyan <[email protected]>

This adds the clock driver for Cirrus Logic CLPS711X series SoCs using common clock infrastructure.
Designed primarily for migration CLPS711X subarch for multiplatform & DT, for this as the "OF" and "not-OF" calls implemented.
Additionally, patch removes unnecessary symbol CLKDEV_LOOKUP for CLPS711X from main ARM Kconfig since this symbol is selected automatically by COMMON_CLK.

Signed-off-by Alexander Shiyan <[email protected]>

---
 .../devicetree/bindings/clock/clps711x-clock.txt   |  47 +++++++
 arch/arm/Kconfig                                   |   1 -
 drivers/clk/Makefile                               |   1 +
 drivers/clk/clk-clps711x.c                         | 150 +++++++++++++++++++++
 4 files changed, 198 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/clock/clps711x-clock.txt
 create mode 100644 drivers/clk/clk-clps711x.c

Documentation/devicetree/bindings/clock/clps711x-clock.txt new file +21 -0

new file mode 100644
index 0000000..0fb9fff
@@ -0,0 +1,47 @@
+Device Tree Clock bindings for the Cirrus Logic CLPS711X CPUs
+
+=== Clock subsystem ===
+Required properties:
+- compatible: Should be "cirrus,clps711x-clk"
+- reg: Address of the internal register set
+- #clock-cells: Should be <1>
+
+The clock consumer should specify the desired clock by having the clock
+ID in its "clocks" phandle cell. The following table is a full list of
+CLPS711X clocks and IDs.
+
+	Clock		ID
+	------------------
+	dummy		0
+	cpu		1
+	pll		2
+	bus		3
+	timer_hf	4
+	timer_lf	5
+	tc1		6

Documentation/devicetree/bindings/clock/clps711x-clock.txt new file +4 -0

+	clocks = <&clks 6>, <&clks 7>;
+	clock-names = "tc1", "tc2";
+	interrupts = <9>;
+};

arch/arm/Kconfig +0 -1

index ba412e0..dfb1e7f 100644
@@ -370,7 +370,6 @@ config ARCH_CLPS711X
 	bool "Cirrus Logic CLPS711x/EP721x/EP731x-based"
 	select ARCH_REQUIRE_GPIOLIB
 	select AUTO_ZRELADDR
-	select CLKDEV_LOOKUP
 	select CLKSRC_MMIO
 	select COMMON_CLK
 	select CPU_ARM720T

drivers/clk/Makefile +1 -0

index 4038c2b..a7e9b73 100644
@@ -11,6 +11,7 @@ obj-$(CONFIG_COMMON_CLK)	+= clk-composite.o
 
 # SoCs specific
 obj-$(CONFIG_ARCH_BCM2835)	+= clk-bcm2835.o
+obj-$(CONFIG_ARCH_CLPS711X)	+= clk-clps711x.o
 obj-$(CONFIG_ARCH_NOMADIK)	+= clk-nomadik.o
 obj-$(CONFIG_ARCH_HIGHBANK)	+= clk-highbank.o
 obj-$(CONFIG_ARCH_NSPIRE)	+= clk-nspire.o

drivers/clk/clk-clps711x.c new file +38 -0

new file mode 100644
index 0000000..662908c
@@ -0,0 +1,150 @@
+/*
+ *  CLPS711X clk driver
+ *
+ *  Copyright (C) 2013 Alexander Shiyan <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#define pr_fmt(fmt)	"clk-clps711x: " fmt
+
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/clkdev.h>
+#include <linux/io.h>
+#include <linux/ioport.h>
+#include <linux/of_address.h>
+#include <linux/slab.h>
+
+#include <linux/mfd/syscon/clps711x.h>
+
+#define CLPS711X_SYSCON1	(0x0100)
+#define CLPS711X_SYSCON2	(0x1100)
+#define CLPS711X_PLLR		(0xa5a8)
+
+#define CLPS711X_EXT_FREQ	(13000000)
+#define CLPS711X_OSC_FREQ	(3686400)
+
+enum clps711x_clks {
+	dummy, cpu, pll, bus, timer_hf, timer_lf, tc1, tc2, spi, uart, clk_max
+};
+
+static struct {
+	struct clk_onecell_data	clk_data;
+	struct clk		*clks[clk_max];
+} *clps711x_clk;

drivers/clk/clk-clps711x.c new file +8 -0

+
+	clps711x_clk = kzalloc(sizeof(*clps711x_clk), GFP_KERNEL);
+	BUG_ON(!clps711x_clk);
+
+	pllr = ioremap(phys_base + CLPS711X_PLLR, SZ_4);
+	BUG_ON(!pllr);
+
+	syscon1 = ioremap(phys_base + CLPS711X_SYSCON1, SZ_128);

drivers/clk/clk-clps711x.c new file +46 -0

+
+	tmp = readl(syscon2 + SYSFLG_OFFSET);
+	if (tmp & SYSFLG2_CKMODE) {
+		f_cpu = CLPS711X_EXT_FREQ;
+		f_bus = CLPS711X_EXT_FREQ;
+		f_spi = 135400;
+		f_pll = 0;
+	} else {
+		f_cpu = f_pll;
+		if (f_cpu >= 36864000)
+			f_bus = DIV_ROUND_UP(f_cpu, 2);
+		else
+			f_bus = 36864000 / 2;
+		f_spi = DIV_ROUND_CLOSEST(f_cpu, 576);
+	}
+
+	if (tmp & SYSFLG2_CKMODE) {
+		if (readl(syscon2 + SYSCON_OFFSET) & SYSCON2_OSTB)
+			f_timh = DIV_ROUND_CLOSEST(CLPS711X_EXT_FREQ, 26);
+		else
+			f_timh = DIV_ROUND_CLOSEST(CLPS711X_EXT_FREQ, 24);
+	} else
+		f_timh = DIV_ROUND_CLOSEST(f_cpu, 144);
+
+	clps711x_clk->clks[dummy] =
+		clk_register_fixed_rate(NULL, "dummy", NULL, CLK_IS_ROOT, 0);
+	clps711x_clk->clks[cpu] =
+		clk_register_fixed_rate(NULL, "cpu", NULL, CLK_IS_ROOT, f_cpu);
+	clps711x_clk->clks[pll] =
+		clk_register_fixed_rate(NULL, "pll", NULL, CLK_IS_ROOT, f_pll);
+	clps711x_clk->clks[bus] =
+		clk_register_fixed_rate(NULL, "bus", NULL, CLK_IS_ROOT, f_bus);
+	clps711x_clk->clks[timer_hf] =
+		clk_register_fixed_rate(NULL, "timer_hf", NULL, CLK_IS_ROOT,
+					f_timh);
+	clps711x_clk->clks[timer_lf] =
+		clk_register_fixed_factor(NULL, "timer_lf", "timer_hf",
+					  0, 1, 256);
+	clps711x_clk->clks[tc1] =
+		clk_register_mux(NULL, "tc1", tc_parents, 2,
+				 CLK_GET_RATE_NOCACHE, syscon1, 5, 1, 0, NULL);
+	clps711x_clk->clks[tc2] =
+		clk_register_mux(NULL, "tc2", tc_parents, 2,
+				 CLK_GET_RATE_NOCACHE, syscon1, 7, 1, 0, NULL);
+	clps711x_clk->clks[spi] =
+		clk_register_fixed_rate(NULL, "spi", NULL, CLK_IS_ROOT, f_spi);

This adds the clocksource driver for Cirrus Logic CLPS711X series SoCs. Designed primarily for migration CLPS711X subarch for multiplatform & DT, for this as the "OF" and "not-OF" calls implemented.

Signed-off-by Alexander Shiyan <[email protected]>

---
 arch/arm/Kconfig                      |   2 -
 drivers/clocksource/Kconfig           |   6 ++
 drivers/clocksource/Makefile          |   1 +
 drivers/clocksource/clps711x-clksrc.c | 151 ++++++++++++++++++++++++++++++++++
 4 files changed, 158 insertions(+), 2 deletions(-)
 create mode 100644 drivers/clocksource/clps711x-clksrc.c

arch/arm/Kconfig +0 -2

index dfb1e7f..5c60cb7 100644
@@ -370,10 +370,8 @@ config ARCH_CLPS711X
 	bool "Cirrus Logic CLPS711x/EP721x/EP731x-based"
 	select ARCH_REQUIRE_GPIOLIB
 	select AUTO_ZRELADDR
-	select CLKSRC_MMIO
 	select COMMON_CLK
 	select CPU_ARM720T
-	select GENERIC_CLOCKEVENTS
 	select MFD_SYSCON
 	select MULTI_IRQ_HANDLER
 	select SPARSE_IRQ

drivers/clocksource/Kconfig +6 -0

index b7b9b04..bd6dc82 100644
@@ -41,6 +41,12 @@ config VT8500_TIMER
 config CADENCE_TTC_TIMER
 	bool
 
+config CLKSRC_CLPS711X
+	def_bool y if ARCH_CLPS711X
+	select CLKSRC_MMIO
+	select CLKSRC_OF if OF
+	select GENERIC_CLOCKEVENTS
+
 config CLKSRC_NOMADIK_MTU
 	bool
 	depends on (ARCH_NOMADIK || ARCH_U8500)

drivers/clocksource/Makefile +1 -0

index 8b00c5c..da6c102 100644
@@ -12,6 +12,7 @@ obj-$(CONFIG_CLKBLD_I8253)	+= i8253.o
 obj-$(CONFIG_CLKSRC_MMIO)	+= mmio.o
 obj-$(CONFIG_DW_APB_TIMER)	+= dw_apb_timer.o
 obj-$(CONFIG_DW_APB_TIMER_OF)	+= dw_apb_timer_of.o
+obj-$(CONFIG_CLKSRC_CLPS711X)	+= clps711x-clksrc.o
 obj-$(CONFIG_CLKSRC_NOMADIK_MTU)	+= nomadik-mtu.o
 obj-$(CONFIG_CLKSRC_DBX500_PRCMU)	+= clksrc-dbx500-prcmu.o
 obj-$(CONFIG_ARMADA_370_XP_TIMER)	+= time-armada-370-xp.o

drivers/clocksource/clps711x-clksrc.c new file +18 -0

new file mode 100644
index 0000000..1749b0b
@@ -0,0 +1,151 @@
+/*
+ *  CLPS711X clocksource driver
+ *
+ *  Copyright (C) 2013 Alexander Shiyan <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/clk.h>
+#include <linux/clockchips.h>
+#include <linux/clocksource.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/io.h>
+#include <linux/ioport.h>

drivers/clocksource/clps711x-clksrc.c new file +8 -0

+
+static struct clock_event_device clps711x_clockevent = {
+	.name		= "clps711x-clockevent",
+	.rating		= 300,
+	.features	= CLOCK_EVT_FEAT_PERIODIC,
+	.set_mode	= clps711x_clockevent_set_mode,
+};
+

drivers/clocksource/clps711x-clksrc.c new file +18 -0

+	.handler	= clps711x_timer_interrupt,
+};
+
+static void __init _clps711x_clksrc_init(phys_addr_t phys_base, int irq,
+					 struct clk *tc1, struct clk *tc2)
+{
+	unsigned long tc1_rate, tc2_rate;
+	void __iomem *tc2d, *syscon1;
+	u32 tmp;
+
+	BUG_ON(IS_ERR(tc1) || IS_ERR(tc2));
+
+	BUG_ON(!request_mem_region(phys_base + CLPS711X_TC1D, SZ_2, NULL));
+	BUG_ON(!request_mem_region(phys_base + CLPS711X_TC2D, SZ_2, NULL));
+
+	clps711x_clksrc = kzalloc(sizeof(*clps711x_clksrc), GFP_KERNEL);
+	BUG_ON(!clps711x_clksrc);
+

drivers/clocksource/clps711x-clksrc.c new file +8 -0

+
+	tc1_rate = clk_get_rate(tc1);
+	tc2_rate = clk_get_rate(tc2);
+
+	clocksource_mmio_init(clps711x_clksrc->tc1d, "clps711x-clocksource",
+			      tc1_rate, 300, 16, clocksource_mmio_readw_down);
+
+	setup_sched_clock(clps711x_sched_clock_read, 16, tc1_rate);

drivers/clocksource/clps711x-clksrc.c new file +23 -0

+{
+	struct clk *tc1 = clk_get(NULL, "tc1");
+	struct clk *tc2 = clk_get(NULL, "tc2");
+
+	_clps711x_clksrc_init(phys_base, irq, tc1, tc2);
+}
+
+#ifdef CONFIG_CLKSRC_OF
+static void __init clps711x_clksrc_init_dt(struct device_node *np)
+{
+	struct clk *tc1 = of_clk_get_by_name(np, "tc1");
+	struct clk *tc2 = of_clk_get_by_name(np, "tc2");
+	struct resource res_io, res_irq;
+
+	BUG_ON(!of_address_to_resource(np, 0, &res_io));
+
+	BUG_ON(!of_irq_to_resource(np, 0, &res_irq));
+
+	_clps711x_clksrc_init(res_io.start, res_irq.start, tc1, tc2);
+}
+CLOCKSOURCE_OF_DECLARE(clps711x, "cirrus,clps711x-clocksource",
+		       clps711x_clksrc_init_dt);
+#endif

(Added Thomas and John in Cc in case they want to review the patch).

Quoted 3 lines · Alexander Shiyan

This adds the clocksource driver for Cirrus Logic CLPS711X series SoCs. Designed primarily for migration CLPS711X subarch for multiplatform & DT, for this as the "OF" and "not-OF" calls implemented.

When a new driver is submitted, I ask for a more detailed changelog about the driver itself: how it works, any specificities, etc ...

That will help people to understand the code better at least for the review.

Quoted 8 lines

Signed-off-by Alexander Shiyan <[email protected]>

---
 arch/arm/Kconfig                      |   2 -
 drivers/clocksource/Kconfig           |   6 ++
 drivers/clocksource/Makefile          |   1 +
 drivers/clocksource/clps711x-clksrc.c | 151 ++++++++++++++++++++++++++++++++++
 4 files changed, 158 insertions(+), 2 deletions(-)
 create mode 100644 drivers/clocksource/clps711x-clksrc.c

drivers/clocksource/clps711x-clksrc.c line 28

Quoted 3 lines

drivers/clocksource/clps711x-clksrc.c new file +3 -0

+#define CLPS711X_SYSCON1	(0x0100)
+#define CLPS711X_TC1D		(0x0300)
+#define CLPS711X_TC2D		(0x0340)

Alignment.

drivers/clocksource/clps711x-clksrc.c line 33

Quoted 4 lines

drivers/clocksource/clps711x-clksrc.c new file +4 -0

+static struct {
+	void __iomem	*tc1d;
+	int		irq;
+} *clps711x_clksrc;

You don't need to define this structure, the struct clock_event_device already contains a field with irq.

drivers/clocksource/clps711x-clksrc.c line 43

Quoted 4 lines

drivers/clocksource/clps711x-clksrc.c new file +4 -0

+static void clps711x_clockevent_set_mode(enum clock_event_mode mode,
+					 struct clock_event_device *evt)
+{
+	disable_irq(clps711x_clksrc->irq);

Do you really need to disable the interrupt to re-enable it right after ?

Quoted 8 lines

+> + switch (mode) {

drivers/clocksource/clps711x-clksrc.c new file +7 -0

+	case CLOCK_EVT_MODE_PERIODIC:
+		enable_irq(clps711x_clksrc->irq);
+		break;
+	case CLOCK_EVT_MODE_ONESHOT:
+		/* Not supported */
+	case CLOCK_EVT_MODE_SHUTDOWN:
+	case CLOCK_EVT_MODE_UNUSED:

drivers/clocksource/clps711x-clksrc.c line 57

Quoted 5 lines

drivers/clocksource/clps711x-clksrc.c new file +5 -0

+	case CLOCK_EVT_MODE_RESUME:
+		/* Left event sources disabled, no more interrupts appear */
+		break;
+	}
+}

I am not expert in interrupts, but it is possible this interrupt could be shared ?

There isn't a register to enable/disable the timer ?

drivers/clocksource/clps711x-clksrc.c line 75

Quoted 3 lines

drivers/clocksource/clps711x-clksrc.c new file +3 -0

+static struct irqaction clps711x_timer_irq = {
+	.name		= "clps711x-timer",
+	.flags		= IRQF_TIMER | IRQF_IRQPOLL,

Why do you need IRQF_IRQPOOL ?

drivers/clocksource/clps711x-clksrc.c line 110

Quoted 6 lines

drivers/clocksource/clps711x-clksrc.c new file +6 -0

+	tmp = readl(syscon1);
+	/* TC1 in free running mode */
+	tmp &= ~SYSCON1_TC1M;
+	/* TC2 in prescale mode */
+	tmp |= SYSCON1_TC2M;
+	writel(tmp, syscon1);

Could you encapsulate these calls inside a static inline function with a more detailed comment please ?

Quoted 1 line

+void __init clps711x_clksrc_init(phys_addr_t phys_base, int irq)

Is this function called somewhere ? I don't see the function definition ?

Quoted 10 lines · On Fri, 19 Jul 2013 16:38:54 +0200 Daniel Lezcano <[email protected]>…

(Added Thomas and John in Cc in case they want to review the patch).

Quoted 3 lines · Alexander Shiyan

This adds the clocksource driver for Cirrus Logic CLPS711X series SoCs. Designed primarily for migration CLPS711X subarch for multiplatform & DT, for this as the "OF" and "not-OF" calls implemented.

When a new driver is submitted, I ask for a more detailed changelog about the driver itself: how it works, any specificities, etc ...

Quoted 1 line

That will help people to understand the code better at least for the review.

Basically, this series is a port of the CLPS711X subsystems in places specially designed for this purpose. Additionally added to the standard description of the subsystem for use with DT.
As for specs, I'm a little confused by this question. In this case, I'm doing is not anything new for this CPU. The CPU has two timers and I use both to provide the correct time sources.

Quoted 8 lines

Quoted 8 lines

Signed-off-by Alexander Shiyan <[email protected]>

---
 arch/arm/Kconfig                      |   2 -
 drivers/clocksource/Kconfig           |   6 ++
 drivers/clocksource/Makefile          |   1 +
 drivers/clocksource/clps711x-clksrc.c | 151 ++++++++++++++++++++++++++++++++++
 4 files changed, 158 insertions(+), 2 deletions(-)
 create mode 100644 drivers/clocksource/clps711x-clksrc.c

[...]

Quoted 5 lines

drivers/clocksource/clps711x-clksrc.c line 28

Quoted 3 lines

drivers/clocksource/clps711x-clksrc.c new file +3 -0

+#define CLPS711X_SYSCON1	(0x0100)
+#define CLPS711X_TC1D		(0x0300)
+#define CLPS711X_TC2D		(0x0340)

Alignment.

Alignment? Do not see any problems.
Wrong alignment in this case is the result of quote.

Quoted 7 lines

drivers/clocksource/clps711x-clksrc.c line 33

Quoted 4 lines

drivers/clocksource/clps711x-clksrc.c new file +4 -0

+static struct {
+	void __iomem	*tc1d;
+	int		irq;
+} *clps711x_clksrc;

You don't need to define this structure, the struct clock_event_device already contains a field with irq.

Thank you, I will know.

[...]

Quoted 6 lines

drivers/clocksource/clps711x-clksrc.c line 43

Quoted 4 lines

drivers/clocksource/clps711x-clksrc.c new file +4 -0

+static void clps711x_clockevent_set_mode(enum clock_event_mode mode,
+					 struct clock_event_device *evt)
+{
+	disable_irq(clps711x_clksrc->irq);

Do you really need to disable the interrupt to re-enable it right after ?

That was the original implementation. Perhaps the best solution is to disable interrupt by CLOCK_EVT_MODE_SHUTDOWN and enable it back by CLOCK_EVT_MODE_RESUME.

Quoted 8 lines

Quoted 8 lines

+> + switch (mode) {

drivers/clocksource/clps711x-clksrc.c new file +7 -0

+	case CLOCK_EVT_MODE_PERIODIC:
+		enable_irq(clps711x_clksrc->irq);
+		break;
+	case CLOCK_EVT_MODE_ONESHOT:
+		/* Not supported */
+	case CLOCK_EVT_MODE_SHUTDOWN:
+	case CLOCK_EVT_MODE_UNUSED:

Quoted 8 lines

drivers/clocksource/clps711x-clksrc.c line 57

Quoted 5 lines

drivers/clocksource/clps711x-clksrc.c new file +5 -0

+	case CLOCK_EVT_MODE_RESUME:
+		/* Left event sources disabled, no more interrupts appear */
+		break;
+	}
+}

I am not expert in interrupts, but it is possible this interrupt could be shared ?

Timer interrupt? No.

Quoted 1 line

There isn't a register to enable/disable the timer ?

Unfortunately, no. The timer is running always, except STDBY CPU state, so we can control it only by enable/disable interrupt.

[...]

Quoted 5 lines

drivers/clocksource/clps711x-clksrc.c line 75

Quoted 3 lines

drivers/clocksource/clps711x-clksrc.c new file +3 -0

+static struct irqaction clps711x_timer_irq = {
+	.name		= "clps711x-timer",
+	.flags		= IRQF_TIMER | IRQF_IRQPOLL,

Why do you need IRQF_IRQPOOL ?

linux/interrupt.h:
* IRQF_IRQPOLL - Interrupt is used for polling (only the interrupt that is
* registered first in an shared interrupt is considered for
* performance reasons)

I do not see need this flag, but in fact, this flag is traditionally used in almost all subsystems of this type.

[...]

Quoted 9 lines

drivers/clocksource/clps711x-clksrc.c line 110

Quoted 6 lines

drivers/clocksource/clps711x-clksrc.c new file +6 -0

+	tmp = readl(syscon1);
+	/* TC1 in free running mode */
+	tmp &= ~SYSCON1_TC1M;
+	/* TC2 in prescale mode */
+	tmp |= SYSCON1_TC2M;
+	writel(tmp, syscon1);

Could you encapsulate these calls inside a static inline function with a more detailed comment please ?

Why static inline?
There is a comment line for each bit, You think this is not enough?

[...]

Quoted 3 lines

Quoted 1 line

+void __init clps711x_clksrc_init(phys_addr_t phys_base, int irq)

Is this function called somewhere ? I don't see the function definition ?

It is defined in the patch [9/10] for non-DT case.

Quoted 8 lines · Alexander Shiyan

Quoted 6 lines · On Fri, 19 Jul 2013 16:38:54 +0200 Daniel Lezcano <[email protected]>…

(Added Thomas and John in Cc in case they want to review the patch).

Quoted 3 lines · Alexander Shiyan

This adds the clocksource driver for Cirrus Logic CLPS711X series SoCs. Designed primarily for migration CLPS711X subarch for multiplatform & DT, for this as the "OF" and "not-OF" calls implemented.

Quoted 11 lines

Quoted 5 lines

drivers/clocksource/clps711x-clksrc.c line 75

Quoted 3 lines

drivers/clocksource/clps711x-clksrc.c new file +3 -0

+static struct irqaction clps711x_timer_irq = {
+	.name		= "clps711x-timer",
+	.flags		= IRQF_TIMER | IRQF_IRQPOLL,

Why do you need IRQF_IRQPOOL ?

linux/interrupt.h:
* IRQF_IRQPOLL - Interrupt is used for polling (only the interrupt that is
* registered first in an shared interrupt is considered for
* performance reasons)

Quoted 2 lines

I do not see need this flag, but in fact, this flag is traditionally used in almost all subsystems of this type.

So IIUC, IRQPOLL is to switch to polling mode when a lot of interrupts occur, right ?

Quoted 10 lines

Quoted 9 lines

drivers/clocksource/clps711x-clksrc.c line 110

Quoted 6 lines

drivers/clocksource/clps711x-clksrc.c new file +6 -0

+	tmp = readl(syscon1);
+	/* TC1 in free running mode */
+	tmp &= ~SYSCON1_TC1M;
+	/* TC2 in prescale mode */
+	tmp |= SYSCON1_TC2M;
+	writel(tmp, syscon1);

Could you encapsulate these calls inside a static inline function with a more detailed comment please ?

Quoted 2 lines

Why static inline?
There is a comment line for each bit, You think this is not enough?

I prefer to encapsulate readl/writel calls inside a small function with an explicit name, comments are still welcome.

This adds the clocksource driver for Cirrus Logic CLPS711X series SoCs. Designed primarily for migration CLPS711X subarch for multiplatform & DT, for this as the "OF" and "not-OF" calls implemented.

Signed-off-by Alexander Shiyan <[email protected]>

---

drivers/clocksource/clps711x-clksrc.c new file +54 -0

new file mode 100644
index 0000000..1749b0b
@@ -0,0 +1,151 @@
+/*
+ *  CLPS711X clocksource driver
+ *
+ *  Copyright (C) 2013 Alexander Shiyan <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/clk.h>
+#include <linux/clockchips.h>
+#include <linux/clocksource.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/io.h>
+#include <linux/ioport.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/sched_clock.h>
+#include <linux/slab.h>
+
+#include <linux/mfd/syscon/clps711x.h>
+
+#define CLPS711X_SYSCON1	(0x0100)
+#define CLPS711X_TC1D		(0x0300)
+#define CLPS711X_TC2D		(0x0340)
+
+static struct {
+	void __iomem	*tc1d;
+	int		irq;
+} *clps711x_clksrc;
+
+static u32 notrace clps711x_sched_clock_read(void)
+{
+	return ~readw(clps711x_clksrc->tc1d);
+}
+
+static void clps711x_clockevent_set_mode(enum clock_event_mode mode,
+					 struct clock_event_device *evt)
+{
+	disable_irq(clps711x_clksrc->irq);
+
+	switch (mode) {
+	case CLOCK_EVT_MODE_PERIODIC:
+		enable_irq(clps711x_clksrc->irq);
+		break;
+	case CLOCK_EVT_MODE_ONESHOT:
+		/* Not supported */
+	case CLOCK_EVT_MODE_SHUTDOWN:
+	case CLOCK_EVT_MODE_UNUSED:
+	case CLOCK_EVT_MODE_RESUME:
+		/* Left event sources disabled, no more interrupts appear */

drivers/clocksource/clps711x-clksrc.c new file +29 -0

+
+	BUG_ON(!request_mem_region(phys_base + CLPS711X_TC1D, SZ_2, NULL));
+	BUG_ON(!request_mem_region(phys_base + CLPS711X_TC2D, SZ_2, NULL));
+
+	clps711x_clksrc = kzalloc(sizeof(*clps711x_clksrc), GFP_KERNEL);
+	BUG_ON(!clps711x_clksrc);
+
+	clps711x_clksrc->tc1d = ioremap(phys_base + CLPS711X_TC1D, SZ_2);
+	BUG_ON(!clps711x_clksrc->tc1d);
+
+	tc2d = ioremap(phys_base + CLPS711X_TC2D, SZ_2);
+	BUG_ON(!tc2d);
+
+	syscon1 = ioremap(phys_base + CLPS711X_SYSCON1, SZ_4);
+	BUG_ON(!syscon1);
+
+	clps711x_clksrc->irq = irq;
+
+	tmp = readl(syscon1);
+	/* TC1 in free running mode */
+	tmp &= ~SYSCON1_TC1M;
+	/* TC2 in prescale mode */
+	tmp |= SYSCON1_TC2M;
+	writel(tmp, syscon1);
+
+	tc1_rate = clk_get_rate(tc1);
+	tc2_rate = clk_get_rate(tc2);
+
+	clocksource_mmio_init(clps711x_clksrc->tc1d, "clps711x-clocksource",

Quoted 6 lines · Alexander Shiyan

This adds the clocksource driver for Cirrus Logic CLPS711X series SoCs. Designed primarily for migration CLPS711X subarch for multiplatform & DT, for this as the "OF" and "not-OF" calls implemented.

Signed-off-by Alexander Shiyan <[email protected]>

---

[...]

drivers/clocksource/clps711x-clksrc.c line 64

Quoted 6 lines

drivers/clocksource/clps711x-clksrc.c new file +6 -0

+static struct clock_event_device clps711x_clockevent = {
+	.name		= "clps711x-clockevent",
+	.rating		= 300,
+	.features	= CLOCK_EVT_FEAT_PERIODIC,
+	.set_mode	= clps711x_clockevent_set_mode,
+};

This seems to be a global clockevent, or a CPU0-only clockevent. Please set the cpumask to clarify this, or core clockevent code will scream at you (see clockevents_register_device).

I assume this doesn't stop in low power states (CLOCK_EVT_FEAT_C3STOP)?

Quoted 4 lines

drivers/clocksource/clps711x-clksrc.c new file +4 -0

+static void __init _clps711x_clksrc_init(phys_addr_t phys_base, int irq,
+					 struct clk *tc1, struct clk *tc2)
+{
+	unsigned long tc1_rate, tc2_rate;

drivers/clocksource/clps711x-clksrc.c line 86

Quoted 4 lines

drivers/clocksource/clps711x-clksrc.c new file +4 -0

+	void __iomem *tc2d, *syscon1;
+	u32 tmp;
+
+	BUG_ON(IS_ERR(tc1) || IS_ERR(tc2));

Is this the only timer possible in the SoCs it'll be used in? You might not be capable of initialising this timer, but another timer may be able to keep the system running...

Quoted 21 lines · On Fri, 2 Aug 2013 11:46:35 +0100 Mark Rutland <[email protected]> wrote:

Quoted 6 lines · Alexander Shiyan

This adds the clocksource driver for Cirrus Logic CLPS711X series SoCs. Designed primarily for migration CLPS711X subarch for multiplatform & DT, for this as the "OF" and "not-OF" calls implemented.

Signed-off-by Alexander Shiyan <[email protected]>

---

[...]

drivers/clocksource/clps711x-clksrc.c line 64

Quoted 6 lines

drivers/clocksource/clps711x-clksrc.c new file +6 -0

+static struct clock_event_device clps711x_clockevent = {
+	.name		= "clps711x-clockevent",
+	.rating		= 300,
+	.features	= CLOCK_EVT_FEAT_PERIODIC,
+	.set_mode	= clps711x_clockevent_set_mode,
+};

This seems to be a global clockevent, or a CPU0-only clockevent. Please set the cpumask to clarify this, or core clockevent code will scream at you (see clockevents_register_device).

Quoted 1 line

I assume this doesn't stop in low power states (CLOCK_EVT_FEAT_C3STOP)?

This feature is marked as "x86(64) specific" in the header.

[...]

Quoted 4 lines

Quoted 4 lines

drivers/clocksource/clps711x-clksrc.c new file +4 -0

+static void __init _clps711x_clksrc_init(phys_addr_t phys_base, int irq,
+					 struct clk *tc1, struct clk *tc2)
+{
+	unsigned long tc1_rate, tc2_rate;

Quoted 8 lines

drivers/clocksource/clps711x-clksrc.c line 86

Quoted 4 lines

drivers/clocksource/clps711x-clksrc.c new file +4 -0

+	void __iomem *tc2d, *syscon1;
+	u32 tmp;
+
+	BUG_ON(IS_ERR(tc1) || IS_ERR(tc2));

Is this the only timer possible in the SoCs it'll be used in? You might not be capable of initialising this timer, but another timer may be able to keep the system running...

This CPU have two HW timers only. Both are used here.

This adds the irqchip driver for Cirrus Logic CLPS711X series SoCs. Designed primarily for migration CLPS711X subarch for multiplatform & DT, for this as the "OF" and "not-OF" calls implemented.

Signed-off-by Alexander Shiyan <[email protected]>

---
 .../interrupt-controller/cirrus,clps711x-intc.txt  |  42 ++++
 arch/arm/Kconfig                                   |   2 -
 drivers/irqchip/Kconfig                            |   6 +
 drivers/irqchip/Makefile                           |   1 +
 drivers/irqchip/irq-clps711x.c                     | 277 +++++++++++++++++++++
 5 files changed, 326 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/cirrus,clps711x-intc.txt
 create mode 100644 drivers/irqchip/irq-clps711x.c

Documentation/devicetree/bindings/interrupt-controller/cirrus,clps711x-intc.txt new file +23 -0

new file mode 100644
index 0000000..26f8983
@@ -0,0 +1,42 @@
+Cirrus Logic CLPS711X Interrupt Controller
+
+Required properties:
+
+- compatible: Should be "cirrus,clps711x-intc"
+- reg: Specifies base physical address of the registers set
+- interrupt-controller: Identifies the node as an interrupt controller
+- #interrupt-cells: Specifies the number of cells needed to encode an
+  interrupt source. The value shall be 1.
+
+The interrupt sources are as follows:
+ID     Name    Description
+---------------------------
+1:     BLINT   Battery low (FIQ)
+3:     MCINT   Media changed (FIQ)
+4:     CSINT   CODEC sound
+5:     EINT1   External 1
+6:     EINT2   External 2
+7:     EINT3   External 3
+8:     TC1OI   TC1 under flow
+9:     TC2OI   TC2 under flow
+10:    RTCMI   RTC compare match
+11:    TINT    64Hz tick

Documentation/devicetree/bindings/interrupt-controller/cirrus,clps711x-intc.txt new file +3 -0

+       interrupt-controller;
+       #interrupt-cells = <1>;
+};

arch/arm/Kconfig +0 -2

index 5c60cb7..6f0d238 100644
@@ -373,8 +373,6 @@ config ARCH_CLPS711X
        select COMMON_CLK
        select CPU_ARM720T
        select MFD_SYSCON
-       select MULTI_IRQ_HANDLER
-       select SPARSE_IRQ
        help
          Support for Cirrus Logic 711x/721x/731x based boards.
 

drivers/irqchip/Kconfig +6 -0

index 1fea003..c94d00a 100644
@@ -30,6 +30,12 @@ config ARM_VIC_NR
          The maximum number of VICs available in the system, for
          power management.
 
+config CLPS711X_IRQCHIP
+       def_bool y if ARCH_CLPS711X
+       select IRQ_DOMAIN
+       select MULTI_IRQ_HANDLER
+       select SPARSE_IRQ
+
 config ORION_IRQCHIP
        bool
        select IRQ_DOMAIN

drivers/irqchip/Makefile +1 -0

index e65c41a..a80ff5b 100644
@@ -8,6 +8,7 @@ obj-$(CONFIG_ARCH_S3C24XX)              += irq-s3c24xx.o
 obj-$(CONFIG_METAG)                    += irq-metag-ext.o
 obj-$(CONFIG_METAG_PERFCOUNTER_IRQS)   += irq-metag.o
 obj-$(CONFIG_ARCH_MOXART)              += irq-moxart.o
+obj-$(CONFIG_CLPS711X_IRQCHIP)         += irq-clps711x.o
 obj-$(CONFIG_ORION_IRQCHIP)            += irq-orion.o
 obj-$(CONFIG_ARCH_SUNXI)               += irq-sun4i.o
 obj-$(CONFIG_ARCH_SPEAR3XX)            += spear-shirq.o

drivers/irqchip/irq-clps711x.c new file +57 -0

new file mode 100644
index 0000000..819baa9
@@ -0,0 +1,277 @@
+/*
+ *  CLPS711X IRQ driver
+ *
+ *  Copyright (C) 2013 Alexander Shiyan <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/io.h>
+#include <linux/irq.h>
+#include <linux/irqdomain.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/slab.h>
+
+#include <asm/exception.h>
+#include <asm/mach/irq.h>
+
+#include "irqchip.h"
+
+#define CLPS711X_INTSR1        (0x0240)
+#define CLPS711X_INTMR1        (0x0280)
+#define CLPS711X_BLEOI (0x0600)
+#define CLPS711X_MCEOI (0x0640)
+#define CLPS711X_TEOI  (0x0680)
+#define CLPS711X_TC1EOI        (0x06c0)
+#define CLPS711X_TC2EOI        (0x0700)
+#define CLPS711X_RTCEOI        (0x0740)
+#define CLPS711X_UMSEOI        (0x0780)
+#define CLPS711X_COEOI (0x07c0)
+#define CLPS711X_INTSR2        (0x1240)
+#define CLPS711X_INTMR2        (0x1280)
+#define CLPS711X_SRXEOF        (0x1600)
+#define CLPS711X_KBDEOI        (0x1700)
+#define CLPS711X_INTSR3        (0x2240)
+#define CLPS711X_INTMR3        (0x2280)
+
+static const struct {
+#define CLPS711X_FLAG_EN       (1 << 0)
+#define CLPS711X_FLAG_FIQ      (1 << 1)
+       unsigned int    flags;
+       phys_addr_t     phys_eoi;
+} clps711x_irqs[] __initconst = {
+       [1]     = { CLPS711X_FLAG_FIQ, CLPS711X_BLEOI, },
+       [3]     = { CLPS711X_FLAG_FIQ, CLPS711X_MCEOI, },
+       [4]     = { CLPS711X_FLAG_EN, CLPS711X_COEOI, },
+       [5]     = { CLPS711X_FLAG_EN, },
+       [6]     = { CLPS711X_FLAG_EN, },
+       [7]     = { CLPS711X_FLAG_EN, },
+       [8]     = { CLPS711X_FLAG_EN, CLPS711X_TC1EOI, },
+       [9]     = { CLPS711X_FLAG_EN, CLPS711X_TC2EOI, },
+       [10]    = { CLPS711X_FLAG_EN, CLPS711X_RTCEOI, },
+       [11]    = { CLPS711X_FLAG_EN, CLPS711X_TEOI, },
+       [12]    = { CLPS711X_FLAG_EN, },

drivers/irqchip/irq-clps711x.c new file +21 -0

+
+static struct {
+       struct irq_domain       *domain;
+       phys_addr_t             phys_base;
+       void __iomem            *intmr[3];
+       void __iomem            *intsr[3];
+       void __iomem            *eoi[ARRAY_SIZE(clps711x_irqs)];
+} *clps711x_intc;
+
+
+static inline u32 fls16(u32 x)
+{
+       u32 r = 15;
+
+       if (!(x & 0xff00)) {
+               x <<= 8;
+               r -= 8;
+       }
+       if (!(x & 0xf000)) {
+               x <<= 4;
+               r -= 4;

drivers/irqchip/irq-clps711x.c new file +87 -0

+
+static asmlinkage void __exception_irq_entry
+clps711x_handle_irq(struct pt_regs *regs)
+{
+       do {
+               u32 irqnr, irqstat;
+
+               irqstat = readw(clps711x_intc->intmr[0]) &
+                         readw(clps711x_intc->intsr[0]);
+               if (irqstat) {
+                       irqnr = irq_find_mapping(clps711x_intc->domain,
+                                                fls16(irqstat));
+                       handle_IRQ(irqnr, regs);
+               }
+
+               irqstat = readw(clps711x_intc->intmr[1]) &
+                         readw(clps711x_intc->intsr[1]);
+               if (irqstat) {
+                       irqnr = irq_find_mapping(clps711x_intc->domain,
+                                                fls16(irqstat) + 16);
+                       handle_IRQ(irqnr, regs);
+                       continue;
+               }
+
+               break;
+       } while (1);
+}
+
+static void clps711x_intc_eoi(struct irq_data *d)
+{
+       irq_hw_number_t hwirq = irqd_to_hwirq(d);
+
+       writel(0, clps711x_intc->eoi[hwirq]);
+}
+
+static void clps711x_intc_mask(struct irq_data *d)
+{
+       irq_hw_number_t hwirq = irqd_to_hwirq(d);
+       void __iomem *intmr = clps711x_intc->intmr[hwirq / 16];
+       u32 tmp;
+
+       tmp = readl(intmr);
+       tmp &= ~(1 << (hwirq % 16));
+       writel(tmp, intmr);
+}
+
+static void clps711x_intc_unmask(struct irq_data *d)
+{
+       irq_hw_number_t hwirq = irqd_to_hwirq(d);
+       void __iomem *intmr = clps711x_intc->intmr[hwirq / 16];
+       u32 tmp;
+
+       tmp = readl(intmr);
+       tmp |= 1 << (hwirq % 16);
+       writel(tmp, intmr);
+}
+
+static struct irq_chip clps711x_intc_chip = {
+       .name           = "clps711x-intc",
+       .irq_eoi        = clps711x_intc_eoi,
+       .irq_mask       = clps711x_intc_mask,
+       .irq_unmask     = clps711x_intc_unmask,
+};
+
+static void __iomem __init *clps711x_ioremap_one(phys_addr_t reg)
+{
+       void __iomem *ret;
+
+       if (!request_mem_region(clps711x_intc->phys_base + reg, SZ_4, NULL))
+               return ERR_PTR(-EBUSY);
+
+       ret = ioremap(clps711x_intc->phys_base + reg, SZ_4);
+       if (!ret)
+               return ERR_PTR(-ENOMEM);
+
+       return ret;
+}
+
+static int __init clps711x_intc_irq_map(struct irq_domain *h, unsigned int virq,
+                                       irq_hw_number_t hw)
+{
+       irq_flow_handler_t handler = handle_level_irq;
+       unsigned int flags = IRQF_VALID | IRQF_PROBE;
+
+       if (!clps711x_irqs[hw].flags)
+               return 0;
+

drivers/irqchip/irq-clps711x.c new file +67 -0

+
+               /* Clear down pending interrupt */
+               writel(0, clps711x_intc->eoi[hw]);
+       }
+
+       irq_set_chip_and_handler(virq, &clps711x_intc_chip, handler);
+       set_irq_flags(virq, flags);
+
+       return 0;
+}
+
+static struct irq_domain_ops clps711x_intc_ops = {
+       .map    = clps711x_intc_irq_map,
+       .xlate  = irq_domain_xlate_onecell,
+};
+
+static void __init _clps711x_intc_init(phys_addr_t phys_base,
+                                      struct device_node *np)
+{
+       clps711x_intc = kzalloc(sizeof(*clps711x_intc), GFP_KERNEL);
+       BUG_ON(!clps711x_intc);
+
+       clps711x_intc->phys_base = phys_base;
+
+       clps711x_intc->intsr[0] = clps711x_ioremap_one(CLPS711X_INTSR1);
+       BUG_ON(IS_ERR_OR_NULL(clps711x_intc->intsr[0]));
+
+       clps711x_intc->intmr[0] = clps711x_ioremap_one(CLPS711X_INTMR1);
+       BUG_ON(IS_ERR_OR_NULL(clps711x_intc->intmr[0]));
+
+       clps711x_intc->intsr[1] = clps711x_ioremap_one(CLPS711X_INTSR2);
+       BUG_ON(IS_ERR_OR_NULL(clps711x_intc->intsr[1]));
+
+       clps711x_intc->intmr[1] = clps711x_ioremap_one(CLPS711X_INTMR2);
+       BUG_ON(IS_ERR_OR_NULL(clps711x_intc->intmr[1]));
+
+       clps711x_intc->intsr[2] = clps711x_ioremap_one(CLPS711X_INTSR3);
+       BUG_ON(IS_ERR_OR_NULL(clps711x_intc->intsr[2]));
+
+       clps711x_intc->intmr[2] = clps711x_ioremap_one(CLPS711X_INTMR3);
+       BUG_ON(IS_ERR_OR_NULL(clps711x_intc->intmr[2]));
+
+       /* Disable interrupts */
+       writel(0, clps711x_intc->intmr[0]);
+       writel(0, clps711x_intc->intmr[1]);
+       writel(0, clps711x_intc->intmr[2]);
+
+       BUG_ON(IS_ERR_VALUE(irq_alloc_descs(-1, 0, ARRAY_SIZE(clps711x_irqs),
+                                           numa_node_id())));
+
+       clps711x_intc->domain =
+               irq_domain_add_legacy(np, ARRAY_SIZE(clps711x_irqs),
+                                     0, 0, &clps711x_intc_ops, NULL);
+       BUG_ON(!clps711x_intc->domain);
+
+       irq_set_default_host(clps711x_intc->domain);
+       set_handle_irq(clps711x_handle_irq);
+
+#ifdef CONFIG_FIQ
+       init_FIQ(0);
+#endif
+}
+
+void __init clps711x_intc_init(phys_addr_t phys_base)
+{
+       _clps711x_intc_init(phys_base, NULL);
+}

Quoted 44 lines · Alexander Shiyan

This adds the irqchip driver for Cirrus Logic CLPS711X series SoCs. Designed primarily for migration CLPS711X subarch for multiplatform & DT, for this as the "OF" and "not-OF" calls implemented.

Signed-off-by Alexander Shiyan <[email protected]>

---
 .../interrupt-controller/cirrus,clps711x-intc.txt  |  42 ++++
 arch/arm/Kconfig                                   |   2 -
 drivers/irqchip/Kconfig                            |   6 +
 drivers/irqchip/Makefile                           |   1 +
 drivers/irqchip/irq-clps711x.c                     | 277 +++++++++++++++++++++
 5 files changed, 326 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/cirrus,clps711x-intc.txt
 create mode 100644 drivers/irqchip/irq-clps711x.c

Documentation/devicetree/bindings/interrupt-controller/cirrus,clps711x-intc.txt new file +23 -0

new file mode 100644
index 0000000..26f8983
@@ -0,0 +1,42 @@
+Cirrus Logic CLPS711X Interrupt Controller
+
+Required properties:
+
+- compatible: Should be "cirrus,clps711x-intc"
+- reg: Specifies base physical address of the registers set
+- interrupt-controller: Identifies the node as an interrupt controller
+- #interrupt-cells: Specifies the number of cells needed to encode an
+  interrupt source. The value shall be 1.
+
+The interrupt sources are as follows:
+ID     Name    Description
+---------------------------
+1:     BLINT   Battery low (FIQ)
+3:     MCINT   Media changed (FIQ)
+4:     CSINT   CODEC sound
+5:     EINT1   External 1
+6:     EINT2   External 2
+7:     EINT3   External 3
+8:     TC1OI   TC1 under flow
+9:     TC2OI   TC2 under flow
+10:    RTCMI   RTC compare match
+11:    TINT    64Hz tick

Documentation/devicetree/bindings/interrupt-controller/cirrus,clps711x-intc.txt line 33

Quoted 10 lines

Documentation/devicetree/bindings/interrupt-controller/cirrus,clps711x-intc.txt new file +10 -0

+12:    UTXINT1 UART1 transmit FIFO half empty
+13:    URXINT1 UART1 receive FIFO half full
+14:    UMSINT  UART1 modem status changed
+15:    SSEOTI  SSI1 end of transfer
+16:    KBDINT  Keyboard
+17:    SS2RX   SSI2 receive FIFO half or greater full
+18:    SS2TX   SSI2 transmit FIFO less than half empty
+28:    UTXINT2 UART2 transmit FIFO half empty
+29:    URXINT2 UART2 receive FIFO half full
+32:    DAIINT  DAI interface (FIQ)

Surely that's specific to the SoC the interrupt controller is used in, not the interrupt controller IP itself?

Quoted 21 lines

Documentation/devicetree/bindings/interrupt-controller/cirrus,clps711x-intc.txt new file +21 -0

+static const struct {
+#define CLPS711X_FLAG_EN       (1 << 0)
+#define CLPS711X_FLAG_FIQ      (1 << 1)
+       unsigned int    flags;
+       phys_addr_t     phys_eoi;
+} clps711x_irqs[] __initconst = {
+       [1]     = { CLPS711X_FLAG_FIQ, CLPS711X_BLEOI, },
+       [3]     = { CLPS711X_FLAG_FIQ, CLPS711X_MCEOI, },
+       [4]     = { CLPS711X_FLAG_EN, CLPS711X_COEOI, },
+       [5]     = { CLPS711X_FLAG_EN, },
+       [6]     = { CLPS711X_FLAG_EN, },
+       [7]     = { CLPS711X_FLAG_EN, },
+       [8]     = { CLPS711X_FLAG_EN, CLPS711X_TC1EOI, },
+       [9]     = { CLPS711X_FLAG_EN, CLPS711X_TC2EOI, },
+       [10]    = { CLPS711X_FLAG_EN, CLPS711X_RTCEOI, },
+       [11]    = { CLPS711X_FLAG_EN, CLPS711X_TEOI, },
+       [12]    = { CLPS711X_FLAG_EN, },
+       [13]    = { CLPS711X_FLAG_EN, },
+       [14]    = { CLPS711X_FLAG_EN, CLPS711X_UMSEOI, },
+       [15]    = { CLPS711X_FLAG_EN, CLPS711X_SRXEOF, },
+       [16]    = { CLPS711X_FLAG_EN, CLPS711X_KBDEOI, },

Documentation/devicetree/bindings/interrupt-controller/cirrus,clps711x-intc.txt

Quoted 6 lines

Documentation/devicetree/bindings/interrupt-controller/cirrus,clps711x-intc.txt new file +6 -0

+       [17]    = { CLPS711X_FLAG_EN, },
+       [18]    = { CLPS711X_FLAG_EN, },
+       [28]    = { CLPS711X_FLAG_EN, },
+       [29]    = { CLPS711X_FLAG_EN, },
+       [32]    = { CLPS711X_FLAG_FIQ, },
+};

Isn't that SoC specific?

Quoted 28 lines · On Fri, 2 Aug 2013 11:57:54 +0100 Mark Rutland <[email protected]> wrote:

Quoted 13 lines · Alexander Shiyan

Documentation/devicetree/bindings/interrupt-controller/cirrus,clps711x-intc.txt new file +13 -0

+The interrupt sources are as follows:
+ID     Name    Description
+---------------------------
+1:     BLINT   Battery low (FIQ)
+3:     MCINT   Media changed (FIQ)
+4:     CSINT   CODEC sound
+5:     EINT1   External 1
+6:     EINT2   External 2
+7:     EINT3   External 3
+8:     TC1OI   TC1 under flow
+9:     TC2OI   TC2 under flow
+10:    RTCMI   RTC compare match
+11:    TINT    64Hz tick

Documentation/devicetree/bindings/interrupt-controller/cirrus,clps711x-intc.txt line 33

Quoted 10 lines

Documentation/devicetree/bindings/interrupt-controller/cirrus,clps711x-intc.txt new file +10 -0

+12:    UTXINT1 UART1 transmit FIFO half empty
+13:    URXINT1 UART1 receive FIFO half full
+14:    UMSINT  UART1 modem status changed
+15:    SSEOTI  SSI1 end of transfer
+16:    KBDINT  Keyboard
+17:    SS2RX   SSI2 receive FIFO half or greater full
+18:    SS2TX   SSI2 transmit FIFO less than half empty
+28:    UTXINT2 UART2 transmit FIFO half empty
+29:    URXINT2 UART2 receive FIFO half full
+32:    DAIINT  DAI interface (FIQ)

Surely that's specific to the SoC the interrupt controller is used in, not the interrupt controller IP itself?

[...]

Quoted 21 lines

Quoted 21 lines

drivers/irqchip/irq-clps711x.c new file +21 -0

+static const struct {
+#define CLPS711X_FLAG_EN       (1 << 0)
+#define CLPS711X_FLAG_FIQ      (1 << 1)
+       unsigned int    flags;
+       phys_addr_t     phys_eoi;
+} clps711x_irqs[] __initconst = {
+       [1]     = { CLPS711X_FLAG_FIQ, CLPS711X_BLEOI, },
+       [3]     = { CLPS711X_FLAG_FIQ, CLPS711X_MCEOI, },
+       [4]     = { CLPS711X_FLAG_EN, CLPS711X_COEOI, },
+       [5]     = { CLPS711X_FLAG_EN, },
+       [6]     = { CLPS711X_FLAG_EN, },
+       [7]     = { CLPS711X_FLAG_EN, },
+       [8]     = { CLPS711X_FLAG_EN, CLPS711X_TC1EOI, },
+       [9]     = { CLPS711X_FLAG_EN, CLPS711X_TC2EOI, },
+       [10]    = { CLPS711X_FLAG_EN, CLPS711X_RTCEOI, },
+       [11]    = { CLPS711X_FLAG_EN, CLPS711X_TEOI, },
+       [12]    = { CLPS711X_FLAG_EN, },
+       [13]    = { CLPS711X_FLAG_EN, },
+       [14]    = { CLPS711X_FLAG_EN, CLPS711X_UMSEOI, },
+       [15]    = { CLPS711X_FLAG_EN, CLPS711X_SRXEOF, },
+       [16]    = { CLPS711X_FLAG_EN, CLPS711X_KBDEOI, },

Quoted 8 lines

drivers/irqchip/irq-clps711x.c line 67

Quoted 6 lines

drivers/irqchip/irq-clps711x.c new file +6 -0

+       [17]    = { CLPS711X_FLAG_EN, },
+       [18]    = { CLPS711X_FLAG_EN, },
+       [28]    = { CLPS711X_FLAG_EN, },
+       [29]    = { CLPS711X_FLAG_EN, },
+       [32]    = { CLPS711X_FLAG_FIQ, },
+};

Isn't that SoC specific?

I absolutely do not understand the last two comments. You are not difficult to describe them in other words?

Quoted 8 lines

drivers/irqchip/irq-clps711x.c new file +8 -0

+static void __iomem __init *clps711x_ioremap_one(phys_addr_t reg)
+{
+       void __iomem *ret;
+
+       if (!request_mem_region(clps711x_intc->phys_base + reg, SZ_4, NULL))
+               return ERR_PTR(-EBUSY);
+
+       ret = ioremap(clps711x_intc->phys_base + reg, SZ_4);

drivers/irqchip/irq-clps711x.c line 174

Quoted 4 lines

drivers/irqchip/irq-clps711x.c new file +4 -0

+       if (!ret)
+               return ERR_PTR(-ENOMEM);
+
+       return ret;

Another unrelated comment: Doing repeated ioremap() and request_mem_region calls is rather wasteful as every single call will consume resources. Better do a single ioremap in the probe function and just add the offsets later.

On Sat, 3 Aug 2013 21:45:41 +0200
Arnd Bergmann <[email protected]> wrote:

[...]

Quoted 8 lines

Quoted 8 lines

drivers/irqchip/irq-clps711x.c new file +8 -0

+static void __iomem __init *clps711x_ioremap_one(phys_addr_t reg)
+{
+       void __iomem *ret;
+
+       if (!request_mem_region(clps711x_intc->phys_base + reg, SZ_4, NULL))
+               return ERR_PTR(-EBUSY);
+
+       ret = ioremap(clps711x_intc->phys_base + reg, SZ_4);

Quoted 8 lines

drivers/irqchip/irq-clps711x.c line 174

Quoted 4 lines

drivers/irqchip/irq-clps711x.c new file +4 -0

+       if (!ret)
+               return ERR_PTR(-ENOMEM);
+
+       return ret;

Another unrelated comment: Doing repeated ioremap() and request_mem_region calls is rather wasteful as every single call will consume resources. Better do a single ioremap in the probe function and just add the offsets later.

Registers are not arranged linearly for each submodule, so for example there are registers for LCD, Timers, PWM, UART etc. between INTMR1 and INTMR2. Single request cannot be used here, since it block access to these holes from drivers.

Quoted 21 lines · Alexander Shiyan

On Sat, 3 Aug 2013 21:45:41 +0200
Arnd Bergmann <[email protected]> wrote:

[...]

Quoted 16 lines

drivers/irqchip/irq-clps711x.c line 174

Quoted 12 lines

drivers/irqchip/irq-clps711x.c new file +12 -0

+static void __iomem __init *clps711x_ioremap_one(phys_addr_t reg)
+{
+       void __iomem *ret;
+
+       if (!request_mem_region(clps711x_intc->phys_base + reg, SZ_4, NULL))
+               return ERR_PTR(-EBUSY);
+
+       ret = ioremap(clps711x_intc->phys_base + reg, SZ_4);
+       if (!ret)
+               return ERR_PTR(-ENOMEM);
+
+       return ret;

Another unrelated comment: Doing repeated ioremap() and request_mem_region calls is rather wasteful as every single call will consume resources. Better do a single ioremap in the probe function and just add the offsets later.

Quoted 3 lines

Registers are not arranged linearly for each submodule, so for example there are registers for LCD, Timers, PWM, UART etc. between INTMR1 and INTMR2. Single request cannot be used here, since it block access to these holes from drivers.

Well, you could still have a single ioremap() but separate request_mem_region() calls then. Each ioremap() actually installs a full page anyway.

Alternatively, you could use the syscon driver to provide access to all the registers.

Quoted 22 lines · Arnd Bergmann

Quoted 21 lines · Alexander Shiyan

On Sat, 3 Aug 2013 21:45:41 +0200
Arnd Bergmann <[email protected]> wrote:

[...]

Quoted 16 lines

Review

Quoted 12 lines

(quoted diff) +12 -0

+static void __iomem __init *clps711x_ioremap_one(phys_addr_t reg)
+{
+       void __iomem *ret;
+
+       if (!request_mem_region(clps711x_intc->phys_base + reg, SZ_4, NULL))
+               return ERR_PTR(-EBUSY);
+
+       ret = ioremap(clps711x_intc->phys_base + reg, SZ_4);
+       if (!ret)
+               return ERR_PTR(-ENOMEM);
+
+       return ret;

Another unrelated comment: Doing repeated ioremap() and request_mem_region calls is rather wasteful as every single call will consume resources. Better do a single ioremap in the probe function and just add the offsets later.

Quoted 8 lines

Quoted 3 lines

Registers are not arranged linearly for each submodule, so for example there are registers for LCD, Timers, PWM, UART etc. between INTMR1 and INTMR2. Single request cannot be used here, since it block access to these holes from drivers.

Well, you could still have a single ioremap() but separate request_mem_region() calls then. Each ioremap() actually installs a full page anyway.

Alternatively, you could use the syscon driver to provide access to all the registers.

There is another problem, which is that you have overlapping register ranges in the DT if the interrupt controller device node contains all the registers including those that are used by the other drivers. Using the syscon driver would solve that, too.

Rethinking the whole situation, I wonder if the EOI registers could be accessed by the individual drivers instead, if they are in their register sets anyway.

Handling them in the irqchip driver probably seemed like a logical solution when the code was initially written, but if you move that access into the device drivers, you can probably remove the irqchip driver entirely and just use the generic irqchip implementation.

Quoted 4 lines · On Sun, 4 Aug 2013 22:57:58 +0200 Arnd Bergmann <[email protected]> wrote:

Handling them in the irqchip driver probably seemed like a logical solution when the code was initially written, but if you move that access into the device drivers, you can probably remove the irqchip driver entirely and just use the generic irqchip implementation.

ioremap for a whole set of CPU registers is called from map_io, so ioremap returns already mapped address, that is, it's just converting physical to virtual addresses.

Quoted 8 lines · Alexander Shiyan

Quoted 4 lines

Handling them in the irqchip driver probably seemed like a logical solution when the code was initially written, but if you move that access into the device drivers, you can probably remove the irqchip driver entirely and just use the generic irqchip implementation.

ioremap for a whole set of CPU registers is called from map_io, so ioremap returns already mapped address, that is, it's just converting physical to virtual addresses.

I see, that does make things better at least.

It would be nice to change the DT binding in some way so the register ranges are non-overlapping, but I'm not going to insist if the other reviewers are ok with the small inconsistency here.

Do you have any reason for not using the syscon driver for these registers?

Quoted 17 lines · On Mon, 5 Aug 2013 21:26:07 +0200 Arnd Bergmann <[email protected]> wrote:

Quoted 8 lines · Alexander Shiyan

Quoted 4 lines

Handling them in the irqchip driver probably seemed like a logical solution when the code was initially written, but if you move that access into the device drivers, you can probably remove the irqchip driver entirely and just use the generic irqchip implementation.

ioremap for a whole set of CPU registers is called from map_io, so ioremap returns already mapped address, that is, it's just converting physical to virtual addresses.

I see, that does make things better at least.

It would be nice to change the DT binding in some way so the register ranges are non-overlapping, but I'm not going to insist if the other reviewers are ok with the small inconsistency here.

Quoted 2 lines

Do you have any reason for not using the syscon driver for these registers?

I plan to use syscon for some registers to ensure the correct sequence of read-modify-write. For all other registers, the access to which is not needed from the different subsystems, it does not make sense and seriously degrade performance due to locks.

As alternative, I can completely remove clps711x_ioremap_one() and simply duplicate ioremap() for the entire range set of registers
(without request_mem_region). The functionality of driver will remain the same. Will it be better?

Quoted 23 lines · Alexander Shiyan

Quoted 19 lines · On Mon, 5 Aug 2013 21:26:07 +0200 Arnd Bergmann <[email protected]> wrote:

Quoted 8 lines · Alexander Shiyan

Quoted 4 lines

Handling them in the irqchip driver probably seemed like a logical solution when the code was initially written, but if you move that access into the device drivers, you can probably remove the irqchip driver entirely and just use the generic irqchip implementation.

ioremap for a whole set of CPU registers is called from map_io, so ioremap returns already mapped address, that is, it's just converting physical to virtual addresses.

I see, that does make things better at least.

It would be nice to change the DT binding in some way so the register ranges are non-overlapping, but I'm not going to insist if the other reviewers are ok with the small inconsistency here.

Do you have any reason for not using the syscon driver for these registers?

Quoted 4 lines

I plan to use syscon for some registers to ensure the correct sequence of read-modify-write. For all other registers, the access to which is not needed from the different subsystems, it does not make sense and seriously degrade performance due to locks.

Have you measure this? MMIO accesses are typically really slow already, they can be multiple orders of magnitude slower than the locks. Also, note that spinlocks get optimized out on uniprocessor machines.

The main reason why syscon was introduced is to handle MMIO register sets that span multiple devices within a small range, which is exactly what you have here. You could simply have a single ioremap in the syscon driver and refer to register offsets from drivers using the syscon binding. You could then actually describe all the EIO registers in a way that

Quoted 4 lines

As alternative, I can completely remove clps711x_ioremap_one() and simply duplicate ioremap() for the entire range set of registers
(without request_mem_region). The functionality of driver will remain the same. Will it be better?

That wouldn't change the binding in any way, so I don't see a reason to prefer one or the other (ioremap or clps711x_ioremap_one), especially since you explained that the registers already come with a static boot-time mapping.

Quoted 3 lines · Alexander Shiyan

Quoted 1 line · On Fri, 2 Aug 2013 11:57:54 +0100 Mark Rutland <[email protected]> wrote:

On Thu, Jul 18, 2013 at 07:34:58PM +0100, Alexander Shiyan wrote:

Quoted 5 lines

Quoted 2 lines

Isn't that SoC specific?

I absolutely do not understand the last two comments. You are not difficult to describe them in other words?

The point that Mark was making is that the both the binding and the driver should be written to only specify what the interrupt controller itself looks like, not how any of the lines are connected or configured.

For instance, if the same interrupt controller is used in another SoC
(ep93xx?) but that soc has an i2c controller connected to IRQ 4 rather than
the sound, the driver should still be usable unmodified.

This adds the cpuidle driver for Cirrus Logic CLPS711X series SoCs. Designed primarily for migration CLPS711X subarch for multiplatform & DT.

Signed-off-by Alexander Shiyan <[email protected]>

---
 drivers/cpuidle/Kconfig            |  6 +++
 drivers/cpuidle/Makefile           |  1 +
 drivers/cpuidle/cpuidle-clps711x.c | 80 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 87 insertions(+)
 create mode 100644 drivers/cpuidle/cpuidle-clps711x.c

drivers/cpuidle/Kconfig +6 -0

index 0e2cd5c..c68a0cf 100644
@@ -36,6 +36,12 @@ config CPU_IDLE_CALXEDA
 	help
 	  Select this to enable cpuidle on Calxeda processors.
 
+config CPU_IDLE_CLPS711X
+	bool "CPU Idle Driver for CLPS711X processors"
+	depends on ARCH_CLPS711X
+	help
+	  Select this to enable cpuidle on Cirrus Logic CLPS711X processors.
+
 config CPU_IDLE_ZYNQ
 	bool "CPU Idle Driver for Xilinx Zynq processors"
 	depends on ARCH_ZYNQ

drivers/cpuidle/Makefile +1 -0

index 8767a7b..d07d2cb 100644
@@ -6,5 +6,6 @@ obj-y += cpuidle.o driver.o governor.o sysfs.o governors/
 obj-$(CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED) += coupled.o
 
 obj-$(CONFIG_CPU_IDLE_CALXEDA) += cpuidle-calxeda.o
+obj-$(CONFIG_CPU_IDLE_CLPS711X)	+= cpuidle-clps711x.o
 obj-$(CONFIG_ARCH_KIRKWOOD) += cpuidle-kirkwood.o
 obj-$(CONFIG_CPU_IDLE_ZYNQ) += cpuidle-zynq.o

drivers/cpuidle/cpuidle-clps711x.c new file +14 -0

new file mode 100644
index 0000000..9f0129f
@@ -0,0 +1,80 @@
+/*
+ *  CLPS711X CPU idle driver
+ *
+ *  Copyright (C) 2013 Alexander Shiyan <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/cpuidle.h>
+#include <linux/err.h>
+#include <linux/io.h>

drivers/cpuidle/cpuidle-clps711x.c new file +23 -0

+	return index;
+}
+
+static struct cpuidle_driver clps711x_idle_driver = {
+	.name			= "clps711x_idle",
+	.owner			= THIS_MODULE,
+	.state_count		= 1,
+	.states[0]	= {
+		.name		= "HALT",
+		.desc		= "CLPS711X WFI",
+		.enter		= clps711x_cpuidle_halt,
+		.exit_latency	= 1,
+	},
+};
+
+static int __init clps711x_cpuidle_probe(struct platform_device *pdev)
+{
+	struct resource *res;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	clps711x_halt = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(clps711x_halt))
+		return PTR_ERR(clps711x_halt);

drivers/cpuidle/cpuidle-clps711x.c new file +6 -0

+
+static const struct of_device_id clps711x_cpuidle_dt_ids[] = {
+	{ .compatible = "cirrus,clps711x-cpuidle", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, clps711x_cpuidle_dt_ids);

drivers/cpuidle/cpuidle-clps711x.c new file +4 -0

+
+MODULE_AUTHOR("Alexander Shiyan <[email protected]>");
+MODULE_DESCRIPTION("CLPS711X CPU idle driver");
+MODULE_LICENSE("GPL");

Quoted 10 lines · Alexander Shiyan

This adds the cpuidle driver for Cirrus Logic CLPS711X series SoCs. Designed primarily for migration CLPS711X subarch for multiplatform & DT.

Signed-off-by Alexander Shiyan <[email protected]>

---
 drivers/cpuidle/Kconfig            |  6 +++
 drivers/cpuidle/Makefile           |  1 +
 drivers/cpuidle/cpuidle-clps711x.c | 80 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 87 insertions(+)
 create mode 100644 drivers/cpuidle/cpuidle-clps711x.c

drivers/cpuidle/cpuidle-clps711x.c line 24

Quoted 4 lines

drivers/cpuidle/cpuidle-clps711x.c new file +4 -0

+static int clps711x_cpuidle_halt(struct cpuidle_device *dev,
+				 struct cpuidle_driver *drv, int index)
+{
+	writel(1, clps711x_halt);

In what the 'clps711x_halt' differs from the usual WFI (cpu_do_idle) ?

drivers/cpuidle/cpuidle-clps711x.c line 26

Quoted 2 lines

drivers/cpuidle/cpuidle-clps711x.c new file +2 -0

+	asm volatile ("mov r0, r0");
+	asm volatile ("mov r0, r0");

Why are needed these two volatile ?

drivers/cpuidle/cpuidle-clps711x.c line 60

Quoted 6 lines

drivers/cpuidle/cpuidle-clps711x.c new file +6 -0

+static int clps711x_cpuidle_remove(struct platform_device *pdev)
+{
+	cpuidle_unregister(&clps711x_idle_driver);
+
+	return 0;
+}

The driver is not compiled as module, will this function be called ?

Quoted 9 lines

drivers/cpuidle/cpuidle-clps711x.c new file +9 -0

+static const struct of_device_id clps711x_cpuidle_dt_ids[] = {
+	{ .compatible = "cirrus,clps711x-cpuidle", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, clps711x_cpuidle_dt_ids);
+
+static struct platform_driver clps711x_cpuidle_driver = {
+	.driver	= {
+		.name		= "clps711x-cpuidle",

drivers/cpuidle/cpuidle-clps711x.c line 76

Quoted 6 lines

drivers/cpuidle/cpuidle-clps711x.c new file +6 -0

+		.owner		= THIS_MODULE,
+		.of_match_table	= clps711x_cpuidle_dt_ids,
+	},
+	.remove	= clps711x_cpuidle_remove,
+};
+module_platform_driver_probe(clps711x_cpuidle_driver, clps711x_cpuidle_probe);

+1

Quoted 11 lines · On Sat, 20 Jul 2013 23:42:11 +0200 Daniel Lezcano <[email protected]>…

Quoted 10 lines · Alexander Shiyan

This adds the cpuidle driver for Cirrus Logic CLPS711X series SoCs. Designed primarily for migration CLPS711X subarch for multiplatform & DT.

Signed-off-by Alexander Shiyan <[email protected]>

---
 drivers/cpuidle/Kconfig            |  6 +++
 drivers/cpuidle/Makefile           |  1 +
 drivers/cpuidle/cpuidle-clps711x.c | 80 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 87 insertions(+)
 create mode 100644 drivers/cpuidle/cpuidle-clps711x.c

[...]

Quoted 6 lines

drivers/cpuidle/cpuidle-clps711x.c line 24

Quoted 4 lines

drivers/cpuidle/cpuidle-clps711x.c new file +4 -0

+static int clps711x_cpuidle_halt(struct cpuidle_device *dev,
+				 struct cpuidle_driver *drv, int index)
+{
+	writel(1, clps711x_halt);

In what the 'clps711x_halt' differs from the usual WFI (cpu_do_idle) ?

AFAIK, ARM720T does not implement the WFI instruction. "HALT" register in CLPS711X do the same:
"A write to this location will put the system into the Idle State by halting the clock to the processor until an interrupt is generated."

Quoted 4 lines

drivers/cpuidle/cpuidle-clps711x.c line 26

Quoted 2 lines

drivers/cpuidle/cpuidle-clps711x.c new file +2 -0

+	asm volatile ("mov r0, r0");
+	asm volatile ("mov r0, r0");

Why are needed these two volatile ?

Two NOP instructions necessary following the enable or disable of the MMU. Documentation not say anything about using this for "HALT", maybe it's the remnants of the old code. I will remove it.

[...]

Quoted 8 lines

drivers/cpuidle/cpuidle-clps711x.c line 60

Quoted 6 lines

drivers/cpuidle/cpuidle-clps711x.c new file +6 -0

+static int clps711x_cpuidle_remove(struct platform_device *pdev)
+{
+	cpuidle_unregister(&clps711x_idle_driver);
+
+	return 0;
+}

The driver is not compiled as module, will this function be called ?

You're right. I will remove it.

Quoted 9 lines

Quoted 9 lines

drivers/cpuidle/cpuidle-clps711x.c new file +9 -0

+static const struct of_device_id clps711x_cpuidle_dt_ids[] = {
+	{ .compatible = "cirrus,clps711x-cpuidle", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, clps711x_cpuidle_dt_ids);
+
+static struct platform_driver clps711x_cpuidle_driver = {
+	.driver	= {
+		.name		= "clps711x-cpuidle",

Quoted 8 lines

drivers/cpuidle/cpuidle-clps711x.c line 76

Quoted 6 lines

drivers/cpuidle/cpuidle-clps711x.c new file +6 -0

+		.owner		= THIS_MODULE,
+		.of_match_table	= clps711x_cpuidle_dt_ids,
+	},
+	.remove	= clps711x_cpuidle_remove,
+};
+module_platform_driver_probe(clps711x_cpuidle_driver, clps711x_cpuidle_probe);

+1

??? What here?

Quoted 22 lines · Alexander Shiyan

Quoted 11 lines · On Sat, 20 Jul 2013 23:42:11 +0200 Daniel Lezcano <[email protected]>…

Quoted 10 lines · Alexander Shiyan

This adds the cpuidle driver for Cirrus Logic CLPS711X series SoCs. Designed primarily for migration CLPS711X subarch for multiplatform & DT.

Signed-off-by Alexander Shiyan <[email protected]>

---
 drivers/cpuidle/Kconfig            |  6 +++
 drivers/cpuidle/Makefile           |  1 +
 drivers/cpuidle/cpuidle-clps711x.c | 80 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 87 insertions(+)
 create mode 100644 drivers/cpuidle/cpuidle-clps711x.c

[...]

Quoted 6 lines

drivers/cpuidle/cpuidle-clps711x.c line 24

Quoted 4 lines

drivers/cpuidle/cpuidle-clps711x.c new file +4 -0

+static int clps711x_cpuidle_halt(struct cpuidle_device *dev,
+				 struct cpuidle_driver *drv, int index)
+{
+	writel(1, clps711x_halt);

In what the 'clps711x_halt' differs from the usual WFI (cpu_do_idle) ?

Quoted 4 lines

AFAIK, ARM720T does not implement the WFI instruction. "HALT" register in CLPS711X do the same:
"A write to this location will put the system into the Idle State by halting the clock to the processor until an interrupt is generated."

I am wondering if you are not using more states, may be you can consider to add a clps711x idle function for the 'arm_pm_idle' callback instead of creating a new driver.

Quoted 6 lines

drivers/cpuidle/cpuidle-clps711x.c line 24

Quoted 4 lines

drivers/cpuidle/cpuidle-clps711x.c new file +4 -0

+static int clps711x_cpuidle_halt(struct cpuidle_device *dev,
+				 struct cpuidle_driver *drv, int index)
+{
+	writel(1, clps711x_halt);

In what the 'clps711x_halt' differs from the usual WFI (cpu_do_idle) ?

AFAIK, ARM720T does not implement the WFI instruction. "HALT" register in CLPS711X do the same:
"A write to this location will put the system into the Idle State by halting the clock to the processor until an interrupt is generated."

Quoted 4 lines

drivers/cpuidle/cpuidle-clps711x.c line 26

Quoted 2 lines

drivers/cpuidle/cpuidle-clps711x.c new file +2 -0

+	asm volatile ("mov r0, r0");
+	asm volatile ("mov r0, r0");

Why are needed these two volatile ?

Two NOP instructions necessary following the enable or disable of the MMU. Documentation not say anything about using this for "HALT", maybe it's the remnants of the old code. I will remove it.

This adds the cpuidle driver for Cirrus Logic CLPS711X series SoCs. Designed primarily for migration CLPS711X subarch for multiplatform & DT.

Signed-off-by Alexander Shiyan <[email protected]>

---
 drivers/cpuidle/Kconfig            |  6 +++
 drivers/cpuidle/Makefile           |  1 +
 drivers/cpuidle/cpuidle-clps711x.c | 80 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 87 insertions(+)
 create mode 100644 drivers/cpuidle/cpuidle-clps711x.c

drivers/cpuidle/Kconfig +6 -0

index 0e2cd5c..c68a0cf 100644
@@ -36,6 +36,12 @@ config CPU_IDLE_CALXEDA
 	help
 	  Select this to enable cpuidle on Calxeda processors.
 
+config CPU_IDLE_CLPS711X
+	bool "CPU Idle Driver for CLPS711X processors"
+	depends on ARCH_CLPS711X
+	help
+	  Select this to enable cpuidle on Cirrus Logic CLPS711X processors.
+
 config CPU_IDLE_ZYNQ
 	bool "CPU Idle Driver for Xilinx Zynq processors"
 	depends on ARCH_ZYNQ

drivers/cpuidle/Makefile +1 -0

index 8767a7b..d07d2cb 100644
@@ -6,5 +6,6 @@ obj-y += cpuidle.o driver.o governor.o sysfs.o governors/
 obj-$(CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED) += coupled.o
 
 obj-$(CONFIG_CPU_IDLE_CALXEDA) += cpuidle-calxeda.o
+obj-$(CONFIG_CPU_IDLE_CLPS711X)	+= cpuidle-clps711x.o
 obj-$(CONFIG_ARCH_KIRKWOOD) += cpuidle-kirkwood.o
 obj-$(CONFIG_CPU_IDLE_ZYNQ) += cpuidle-zynq.o

drivers/cpuidle/cpuidle-clps711x.c new file +16 -0

new file mode 100644
index 0000000..9f0129f
@@ -0,0 +1,80 @@
+/*
+ *  CLPS711X CPU idle driver
+ *
+ *  Copyright (C) 2013 Alexander Shiyan <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/cpuidle.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>

drivers/cpuidle/cpuidle-clps711x.c new file +29 -0

+
+	return index;
+}
+
+static struct cpuidle_driver clps711x_idle_driver = {
+	.name			= "clps711x_idle",
+	.owner			= THIS_MODULE,
+	.state_count		= 1,
+	.states[0]	= {
+		.name		= "HALT",
+		.desc		= "CLPS711X WFI",
+		.enter		= clps711x_cpuidle_halt,
+		.exit_latency	= 1,
+	},
+};
+
+static int __init clps711x_cpuidle_probe(struct platform_device *pdev)
+{
+	struct resource *res;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	clps711x_halt = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(clps711x_halt))
+		return PTR_ERR(clps711x_halt);
+
+	return cpuidle_register(&clps711x_idle_driver, NULL);
+}
+
+static int clps711x_cpuidle_remove(struct platform_device *pdev)

drivers/cpuidle/cpuidle-clps711x.c new file +15 -0

+MODULE_DEVICE_TABLE(of, clps711x_cpuidle_dt_ids);
+
+static struct platform_driver clps711x_cpuidle_driver = {
+	.driver	= {
+		.name		= "clps711x-cpuidle",
+		.owner		= THIS_MODULE,
+		.of_match_table	= clps711x_cpuidle_dt_ids,
+	},
+	.remove	= clps711x_cpuidle_remove,
+};
+module_platform_driver_probe(clps711x_cpuidle_driver, clps711x_cpuidle_probe);
+
+MODULE_AUTHOR("Alexander Shiyan <[email protected]>");
+MODULE_DESCRIPTION("CLPS711X CPU idle driver");
+MODULE_LICENSE("GPL");

Signature · 7 lines

1.8.1.5


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

This patch adds initial support for Cirrus Logic CLPS711X based platforms using the device tree for discovery.
The basic devices is not yet DT-capable, so the CLPS711X DT option is marked as experimental.

Signed-off-by Alexander Shiyan <[email protected]>

---
 arch/arm/mach-clps711x/Kconfig    |  7 +++++
 arch/arm/mach-clps711x/Makefile   |  1 +
 arch/arm/mach-clps711x/board-dt.c | 56 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 64 insertions(+)
 create mode 100644 arch/arm/mach-clps711x/board-dt.c

arch/arm/mach-clps711x/Kconfig +7 -0

index bea6295..8ef38c6 100644
@@ -33,6 +33,13 @@ config ARCH_P720T
 	  Say Y here if you intend to run this kernel on the ARM Prospector
 	  720T.
 
+config CLPS711X_DT
+	bool "Support CLPS711X platforms from device tree (Very Experimental!)"
+	depends on USE_OF
+	help
+	  Include support for Cirrus Logic CLPS711X based platforms
+	  using the device tree for discovery.
+
 config EP72XX_ROM_BOOT
 	bool "EP721x/EP731x ROM boot"
 	help

arch/arm/mach-clps711x/Makefile +1 -0

index f04151e..77d3a76 100644
@@ -11,3 +11,4 @@ obj-$(CONFIG_ARCH_CDB89712)	+= board-cdb89712.o
 obj-$(CONFIG_ARCH_CLEP7312)	+= board-clep7312.o
 obj-$(CONFIG_ARCH_EDB7211)	+= board-edb7211.o
 obj-$(CONFIG_ARCH_P720T)	+= board-p720t.o
+obj-$(CONFIG_CLPS711X_DT)	+= board-dt.o

arch/arm/mach-clps711x/board-dt.c new file +17 -0

new file mode 100644
index 0000000..5b6f3f1
@@ -0,0 +1,56 @@
+/*
+ *  CLPS711X Generic DT board
+ *
+ *  Author: Alexander Shiyan <[email protected]>, 2013
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/clocksource.h>
+#include <linux/clk-provider.h>
+#include <linux/init.h>
+#include <linux/of_platform.h>
+
+#include <asm/mach/arch.h>

arch/arm/mach-clps711x/board-dt.c new file +13 -0

+
+static const char *clps711x_dt_compat[] __initdata = {
+	"cirrus,clps711x",
+	NULL
+};
+
+DT_MACHINE_START(CLPS711X_DT, "Cirrus Logic CLPS711X (Device Tree Support)")
+	.dt_compat	= clps711x_dt_compat,
+	.map_io		= clps711x_map_io,
+	.init_time	= clps711x_timer_init,
+	.init_machine	= clps711x_init_dt,
+	.restart	= clps711x_restart,
+MACHINE_END

Signature · 7 lines

1.8.1.5


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Here is next patchset for the Cirrus Logic CLPS711X target CPUs. The first four patches is a small fixes. The rest is a attempt to add DT support for this subarch.
As long as DT support is incomplete (no peripherals), I did not add DTS yet. This is the next step.

Alexander Shiyan (10):
ARM: clps711x: Remove the special name for the syscon driver
ARM: clps711x: Drop fortunet board support
ARM: clps711x: autcpu12: Remove incorrect config checking
ARM: clps711x: edb7211: Remove extra iotable_init() call
ARM: clps711x: Add CLPS711X clk driver
ARM: clps711x: Add CLPS711X clocksource driver
ARM: clps711x: Add CLPS711X irqchip driver
ARM: clps711x: Add CLPS711X cpuidle driver
ARM: clps711x: Migrate CLPS711X subarch to the new basic drivers
ARM: clps711x: Add initial DT support

Quoted 6 lines · On Thu, 18 Jul 2013 22:34:51 +0400 Alexander Shiyan <[email protected]> wrote:

Here is next patchset for the Cirrus Logic CLPS711X target CPUs. The first four patches is a small fixes. The rest is a attempt to add DT support for this subarch.
As long as DT support is incomplete (no peripherals), I did not add DTS yet. This is the next step.

Quoted 11 lines

Alexander Shiyan (10):
ARM: clps711x: Remove the special name for the syscon driver
ARM: clps711x: Drop fortunet board support
ARM: clps711x: autcpu12: Remove incorrect config checking
ARM: clps711x: edb7211: Remove extra iotable_init() call
ARM: clps711x: Add CLPS711X clk driver
ARM: clps711x: Add CLPS711X clocksource driver
ARM: clps711x: Add CLPS711X irqchip driver
ARM: clps711x: Add CLPS711X cpuidle driver
ARM: clps711x: Migrate CLPS711X subarch to the new basic drivers
ARM: clps711x: Add initial DT support

[...]

If there is no objection about "fixes" part of patchset, can these parts (1/10-4/10) be applied?
Thanks.

Quoted 20 lines · Alexander Shiyan

Quoted 6 lines · On Thu, 18 Jul 2013 22:34:51 +0400 Alexander Shiyan <[email protected]> wrote:

Here is next patchset for the Cirrus Logic CLPS711X target CPUs. The first four patches is a small fixes. The rest is a attempt to add DT support for this subarch.
As long as DT support is incomplete (no peripherals), I did not add DTS yet. This is the next step.

Quoted 11 lines

Alexander Shiyan (10):
ARM: clps711x: Remove the special name for the syscon driver
ARM: clps711x: Drop fortunet board support
ARM: clps711x: autcpu12: Remove incorrect config checking
ARM: clps711x: edb7211: Remove extra iotable_init() call
ARM: clps711x: Add CLPS711X clk driver
ARM: clps711x: Add CLPS711X clocksource driver
ARM: clps711x: Add CLPS711X irqchip driver
ARM: clps711x: Add CLPS711X cpuidle driver
ARM: clps711x: Migrate CLPS711X subarch to the new basic drivers
ARM: clps711x: Add initial DT support

Quoted 5 lines

[...]

If there is no objection about "fixes" part of patchset, can these parts (1/10-4/10) be applied?
Thanks.

Definitely, I'll apply them to fixes-non-critical for 3.12.

Btw, Kevin Hilman is stepping in to help out with arm-soc maintenance, so please email arm at kenrel.org, since it's an alias that we are all on.

Also, I don't know if I've asked this of you before, but for these "smaller" platforms that don't see quite so much activity, it's easy for us to forget who wants us to apply patches and who stages branches. So emails like these are appreciated so we know if you want them applied, or if we'll get a pull request later. Clarifying in the patch posting if they are for review or for applying works too.

Quoted 32 lines · On Tue, 13 Aug 2013 23:29:00 -0700 Olof Johansson <[email protected]> wrote:

Quoted 20 lines · Alexander Shiyan

Quoted 6 lines · On Thu, 18 Jul 2013 22:34:51 +0400 Alexander Shiyan <[email protected]> wrote:

Here is next patchset for the Cirrus Logic CLPS711X target CPUs. The first four patches is a small fixes. The rest is a attempt to add DT support for this subarch.
As long as DT support is incomplete (no peripherals), I did not add DTS yet. This is the next step.

Quoted 11 lines

Alexander Shiyan (10):
ARM: clps711x: Remove the special name for the syscon driver
ARM: clps711x: Drop fortunet board support
ARM: clps711x: autcpu12: Remove incorrect config checking
ARM: clps711x: edb7211: Remove extra iotable_init() call
ARM: clps711x: Add CLPS711X clk driver
ARM: clps711x: Add CLPS711X clocksource driver
ARM: clps711x: Add CLPS711X irqchip driver
ARM: clps711x: Add CLPS711X cpuidle driver
ARM: clps711x: Migrate CLPS711X subarch to the new basic drivers
ARM: clps711x: Add initial DT support

Quoted 5 lines

[...]

If there is no objection about "fixes" part of patchset, can these parts (1/10-4/10) be applied?
Thanks.

Definitely, I'll apply them to fixes-non-critical for 3.12.

Btw, Kevin Hilman is stepping in to help out with arm-soc maintenance, so please email arm at kenrel.org, since it's an alias that we are all on.

Quoted 6 lines

Also, I don't know if I've asked this of you before, but for these "smaller" platforms that don't see quite so much activity, it's easy for us to forget who wants us to apply patches and who stages branches. So emails like these are appreciated so we know if you want them applied, or if we'll get a pull request later. Clarifying in the patch posting if they are for review or for applying works too.

Do you mean always create CC to "arm at ..." for parts [0/xx] or after review period? Thnaks.

Here is next patchset for the Cirrus Logic CLPS711X target CPUs. The first four patches is a small fixes. The rest is a attempt to add DT support for this subarch.
As long as DT support is incomplete (no peripherals), I did not add DTS yet. This is the next step.