All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 00/10] ARM: clps711x: Initial DT support / Fixes
@ 2013-07-18 18:34 Alexander Shiyan
  2013-07-18 18:34 ` [PATCH 01/10] ARM: clps711x: Remove the special name for the syscon driver Alexander Shiyan
                   ` (11 more replies)
  0 siblings, 12 replies; 48+ messages in thread
From: Alexander Shiyan @ 2013-07-18 18:34 UTC (permalink / raw)
  To: 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

 .../devicetree/bindings/clock/clps711x-clock.txt   |  47 +++
 .../interrupt-controller/cirrus,clps711x-intc.txt  |  42 +++
 arch/arm/Kconfig                                   |   5 -
 arch/arm/mach-clps711x/Kconfig                     |   8 +-
 arch/arm/mach-clps711x/Makefile                    |   2 +-
 arch/arm/mach-clps711x/board-autcpu12.c            |   9 +-
 arch/arm/mach-clps711x/board-cdb89712.c            |   3 -
 arch/arm/mach-clps711x/board-clep7312.c            |   3 -
 arch/arm/mach-clps711x/board-dt.c                  |  56 ++++
 arch/arm/mach-clps711x/board-edb7211.c             |  20 +-
 arch/arm/mach-clps711x/board-fortunet.c            |  85 -----
 arch/arm/mach-clps711x/board-p720t.c               |   3 -
 arch/arm/mach-clps711x/common.c                    | 345 +--------------------
 arch/arm/mach-clps711x/common.h                    |   3 -
 arch/arm/mach-clps711x/devices.c                   |  12 +-
 drivers/clk/Makefile                               |   1 +
 drivers/clk/clk-clps711x.c                         | 150 +++++++++
 drivers/clocksource/Kconfig                        |   6 +
 drivers/clocksource/Makefile                       |   1 +
 drivers/clocksource/clps711x-clksrc.c              | 151 +++++++++
 drivers/cpuidle/Kconfig                            |   6 +
 drivers/cpuidle/Makefile                           |   1 +
 drivers/cpuidle/cpuidle-clps711x.c                 |  80 +++++
 drivers/irqchip/Kconfig                            |   6 +
 drivers/irqchip/Makefile                           |   1 +
 drivers/irqchip/irq-clps711x.c                     | 277 +++++++++++++++++
 drivers/mfd/syscon.c                               |   3 -
 27 files changed, 851 insertions(+), 475 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/clock/clps711x-clock.txt
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/cirrus,clps711x-intc.txt
 create mode 100644 arch/arm/mach-clps711x/board-dt.c
 delete mode 100644 arch/arm/mach-clps711x/board-fortunet.c
 create mode 100644 drivers/clk/clk-clps711x.c
 create mode 100644 drivers/clocksource/clps711x-clksrc.c
 create mode 100644 drivers/cpuidle/cpuidle-clps711x.c
 create mode 100644 drivers/irqchip/irq-clps711x.c

-- 
1.8.1.5

^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH 01/10] ARM: clps711x: Remove the special name for the syscon driver
  2013-07-18 18:34 [PATCH 00/10] ARM: clps711x: Initial DT support / Fixes Alexander Shiyan
@ 2013-07-18 18:34 ` Alexander Shiyan
  2013-08-14  6:29   ` Olof Johansson
  2013-08-14  6:29   ` Olof Johansson
  2013-07-18 18:34 ` [PATCH 02/10] ARM: clps711x: Drop fortunet board support Alexander Shiyan
                   ` (10 subsequent siblings)
  11 siblings, 2 replies; 48+ messages in thread
From: Alexander Shiyan @ 2013-07-18 18:34 UTC (permalink / raw)
  To: linux-arm-kernel

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 <shc_work@mail.ru>
---
 arch/arm/mach-clps711x/devices.c | 2 +-
 drivers/mfd/syscon.c             | 3 ---
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/arch/arm/mach-clps711x/devices.c b/arch/arm/mach-clps711x/devices.c
index 856b81c..fb77d14 100644
--- a/arch/arm/mach-clps711x/devices.c
+++ b/arch/arm/mach-clps711x/devices.c
@@ -57,7 +57,7 @@ static void __init clps711x_add_syscon(void)
 	unsigned i;
 
 	for (i = 0; i < ARRAY_SIZE(clps711x_syscon_res); i++)
-		platform_device_register_simple("clps711x-syscon", i + 1,
+		platform_device_register_simple("syscon", i + 1,
 						&clps711x_syscon_res[i], 1);
 }
 
diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
index 1a31512..962a6e1 100644
--- a/drivers/mfd/syscon.c
+++ b/drivers/mfd/syscon.c
@@ -159,9 +159,6 @@ static int syscon_probe(struct platform_device *pdev)
 
 static const struct platform_device_id syscon_ids[] = {
 	{ "syscon", },
-#ifdef CONFIG_ARCH_CLPS711X
-	{ "clps711x-syscon", },
-#endif
 	{ }
 };
 
-- 
1.8.1.5

^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [PATCH 02/10] ARM: clps711x: Drop fortunet board support
  2013-07-18 18:34 [PATCH 00/10] ARM: clps711x: Initial DT support / Fixes Alexander Shiyan
  2013-07-18 18:34 ` [PATCH 01/10] ARM: clps711x: Remove the special name for the syscon driver Alexander Shiyan
@ 2013-07-18 18:34 ` Alexander Shiyan
  2013-08-14  6:29   ` Olof Johansson
  2013-07-18 18:34 ` [PATCH 03/10] ARM: clps711x: autcpu12: Remove incorrect config checking Alexander Shiyan
                   ` (9 subsequent siblings)
  11 siblings, 1 reply; 48+ messages in thread
From: Alexander Shiyan @ 2013-07-18 18:34 UTC (permalink / raw)
  To: linux-arm-kernel

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 <shc_work@mail.ru>
---
 arch/arm/mach-clps711x/Kconfig          |  3 --
 arch/arm/mach-clps711x/Makefile         |  1 -
 arch/arm/mach-clps711x/board-fortunet.c | 85 ---------------------------------
 3 files changed, 89 deletions(-)
 delete mode 100644 arch/arm/mach-clps711x/board-fortunet.c

diff --git a/arch/arm/mach-clps711x/Kconfig b/arch/arm/mach-clps711x/Kconfig
index 01ad4d4..bea6295 100644
--- a/arch/arm/mach-clps711x/Kconfig
+++ b/arch/arm/mach-clps711x/Kconfig
@@ -33,9 +33,6 @@ config ARCH_P720T
 	  Say Y here if you intend to run this kernel on the ARM Prospector
 	  720T.
 
-config ARCH_FORTUNET
-	bool "FORTUNET"
-
 config EP72XX_ROM_BOOT
 	bool "EP721x/EP731x ROM boot"
 	help
diff --git a/arch/arm/mach-clps711x/Makefile b/arch/arm/mach-clps711x/Makefile
index f30ed2b..f04151e 100644
--- a/arch/arm/mach-clps711x/Makefile
+++ b/arch/arm/mach-clps711x/Makefile
@@ -10,5 +10,4 @@ obj-$(CONFIG_ARCH_AUTCPU12)	+= board-autcpu12.o
 obj-$(CONFIG_ARCH_CDB89712)	+= board-cdb89712.o
 obj-$(CONFIG_ARCH_CLEP7312)	+= board-clep7312.o
 obj-$(CONFIG_ARCH_EDB7211)	+= board-edb7211.o
-obj-$(CONFIG_ARCH_FORTUNET)	+= board-fortunet.o
 obj-$(CONFIG_ARCH_P720T)	+= board-p720t.o
diff --git a/arch/arm/mach-clps711x/board-fortunet.c b/arch/arm/mach-clps711x/board-fortunet.c
deleted file mode 100644
index b1561e3..0000000
--- a/arch/arm/mach-clps711x/board-fortunet.c
+++ /dev/null
@@ -1,85 +0,0 @@
-/*
- *  linux/arch/arm/mach-clps711x/fortunet.c
- *
- *  Derived from linux/arch/arm/mach-integrator/arch.c
- *
- *  Copyright (C) 2000 Deep Blue Solutions Ltd
- *
- * 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.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software
- * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
- */
-#include <linux/types.h>
-#include <linux/init.h>
-#include <linux/initrd.h>
-
-#include <mach/hardware.h>
-#include <asm/setup.h>
-#include <asm/mach-types.h>
-
-#include <asm/mach/arch.h>
-
-#include <asm/memory.h>
-
-#include "common.h"
-
-struct meminfo memmap = {
-	.nr_banks	= 1,
-	.bank		= {
-		{
-			.start	= 0xC0000000,
-			.size	= 0x01000000,
-		},
-	},
-};
-
-typedef struct tag_IMAGE_PARAMS
-{
-	int	ramdisk_ok;
-	int	ramdisk_address;
-	int	ramdisk_size;
-	int	ram_size;
-	int	extra_param_type;
-	int	extra_param_ptr;
-	int	command_line;
-} IMAGE_PARAMS;
-
-#define IMAGE_PARAMS_PHYS	0xC01F0000
-
-static void __init
-fortunet_fixup(struct tag *tags, char **cmdline, struct meminfo *mi)
-{
-	IMAGE_PARAMS *ip = phys_to_virt(IMAGE_PARAMS_PHYS);
-	*cmdline = phys_to_virt(ip->command_line);
-#ifdef CONFIG_BLK_DEV_INITRD
-	if(ip->ramdisk_ok)
-	{
-		initrd_start = __phys_to_virt(ip->ramdisk_address);
-		initrd_end = initrd_start + ip->ramdisk_size;
-	}
-#endif
-	memmap.bank[0].size = ip->ram_size;
-	*mi = memmap;
-}
-
-MACHINE_START(FORTUNET, "ARM-FortuNet")
-	/* Maintainer: FortuNet Inc. */
-	.nr_irqs	= CLPS711X_NR_IRQS,
-	.fixup		= fortunet_fixup,
-	.map_io		= clps711x_map_io,
-	.init_early	= clps711x_init_early,
-	.init_irq	= clps711x_init_irq,
-	.init_time	= clps711x_timer_init,
-	.handle_irq	= clps711x_handle_irq,
-	.restart	= clps711x_restart,
-MACHINE_END
-- 
1.8.1.5

^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [PATCH 03/10] ARM: clps711x: autcpu12: Remove incorrect config checking
  2013-07-18 18:34 [PATCH 00/10] ARM: clps711x: Initial DT support / Fixes Alexander Shiyan
  2013-07-18 18:34 ` [PATCH 01/10] ARM: clps711x: Remove the special name for the syscon driver Alexander Shiyan
  2013-07-18 18:34 ` [PATCH 02/10] ARM: clps711x: Drop fortunet board support Alexander Shiyan
@ 2013-07-18 18:34 ` Alexander Shiyan
  2013-08-14  6:30   ` Olof Johansson
  2013-07-18 18:34 ` [PATCH 04/10] ARM: clps711x: edb7211: Remove extra iotable_init() call Alexander Shiyan
                   ` (8 subsequent siblings)
  11 siblings, 1 reply; 48+ messages in thread
From: Alexander Shiyan @ 2013-07-18 18:34 UTC (permalink / raw)
  To: linux-arm-kernel

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 <shc_work@mail.ru>
---
 arch/arm/mach-clps711x/board-autcpu12.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/arch/arm/mach-clps711x/board-autcpu12.c b/arch/arm/mach-clps711x/board-autcpu12.c
index 5867aeb..f8d71a8 100644
--- a/arch/arm/mach-clps711x/board-autcpu12.c
+++ b/arch/arm/mach-clps711x/board-autcpu12.c
@@ -259,11 +259,7 @@ static void __init autcpu12_init(void)
 static void __init autcpu12_init_late(void)
 {
 	gpio_request_array(autcpu12_gpios, ARRAY_SIZE(autcpu12_gpios));
-
-	if (IS_ENABLED(MTD_NAND_GPIO) && IS_ENABLED(GPIO_GENERIC_PLATFORM)) {
-		/* We are need both drivers to handle NAND */
-		platform_device_register(&autcpu12_nand_pdev);
-	}
+	platform_device_register(&autcpu12_nand_pdev);
 }
 
 MACHINE_START(AUTCPU12, "autronix autcpu12")
-- 
1.8.1.5

^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [PATCH 04/10] ARM: clps711x: edb7211: Remove extra iotable_init() call
  2013-07-18 18:34 [PATCH 00/10] ARM: clps711x: Initial DT support / Fixes Alexander Shiyan
                   ` (2 preceding siblings ...)
  2013-07-18 18:34 ` [PATCH 03/10] ARM: clps711x: autcpu12: Remove incorrect config checking Alexander Shiyan
@ 2013-07-18 18:34 ` Alexander Shiyan
  2013-08-14  6:30   ` Olof Johansson
  2013-07-18 18:34 ` [PATCH 05/10] ARM: clps711x: Add CLPS711X clk driver Alexander Shiyan
                   ` (7 subsequent siblings)
  11 siblings, 1 reply; 48+ messages in thread
From: Alexander Shiyan @ 2013-07-18 18:34 UTC (permalink / raw)
  To: linux-arm-kernel

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 <shc_work@mail.ru>
---
 arch/arm/mach-clps711x/board-edb7211.c | 17 +----------------
 1 file changed, 1 insertion(+), 16 deletions(-)

diff --git a/arch/arm/mach-clps711x/board-edb7211.c b/arch/arm/mach-clps711x/board-edb7211.c
index 9dfb990..fe6184e 100644
--- a/arch/arm/mach-clps711x/board-edb7211.c
+++ b/arch/arm/mach-clps711x/board-edb7211.c
@@ -126,21 +126,6 @@ static struct gpio edb7211_gpios[] __initconst = {
 	{ EDB7211_LCDBL,	GPIOF_OUT_INIT_LOW,	"LCD BACKLIGHT" },
 };
 
-static struct map_desc edb7211_io_desc[] __initdata = {
-	{	/* Memory-mapped extra keyboard row */
-		.virtual	= IO_ADDRESS(EDB7211_EXTKBD_BASE),
-		.pfn		= __phys_to_pfn(EDB7211_EXTKBD_BASE),
-		.length		= SZ_1M,
-		.type		= MT_DEVICE,
-	},
-};
-
-void __init edb7211_map_io(void)
-{
-	clps711x_map_io();
-	iotable_init(edb7211_io_desc, ARRAY_SIZE(edb7211_io_desc));
-}
-
 /* Reserve screen memory region at the start of main system memory. */
 static void __init edb7211_reserve(void)
 {
@@ -195,7 +180,7 @@ MACHINE_START(EDB7211, "CL-EDB7211 (EP7211 eval board)")
 	.nr_irqs	= CLPS711X_NR_IRQS,
 	.fixup		= fixup_edb7211,
 	.reserve	= edb7211_reserve,
-	.map_io		= edb7211_map_io,
+	.map_io		= clps711x_map_io,
 	.init_early	= clps711x_init_early,
 	.init_irq	= clps711x_init_irq,
 	.init_time	= clps711x_timer_init,
-- 
1.8.1.5

^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [PATCH 05/10] ARM: clps711x: Add CLPS711X clk driver
  2013-07-18 18:34 [PATCH 00/10] ARM: clps711x: Initial DT support / Fixes Alexander Shiyan
                   ` (3 preceding siblings ...)
  2013-07-18 18:34 ` [PATCH 04/10] ARM: clps711x: edb7211: Remove extra iotable_init() call Alexander Shiyan
@ 2013-07-18 18:34 ` Alexander Shiyan
  2013-08-02 10:25   ` Mark Rutland
  2013-07-18 18:34 ` [PATCH 06/10] ARM: clps711x: Add CLPS711X clocksource driver Alexander Shiyan
                   ` (6 subsequent siblings)
  11 siblings, 1 reply; 48+ messages in thread
From: Alexander Shiyan @ 2013-07-18 18:34 UTC (permalink / raw)
  To: linux-arm-kernel

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 <shc_work@mail.ru>
---
 .../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

diff --git a/Documentation/devicetree/bindings/clock/clps711x-clock.txt b/Documentation/devicetree/bindings/clock/clps711x-clock.txt
new file mode 100644
index 0000000..0fb9fff
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/clps711x-clock.txt
@@ -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
+	tc2		7
+	spi		8
+	uart		9
+
+=== Clocksource subsystem ===
+Required properties:
+- compatible: Should be "cirrus,clps711x-clocksource"
+- reg: Address of the internal register set
+- clocks: phandle to the source clocks TC1 and TC2
+- interrupts: The interrupt of the TC2 timer
+
+Example:
+
+clks: clks at 80000000 {
+	compatible = "cirrus,clps711x-clk";
+	reg = <0x80000000 0>;
+	#clock-cells = <1>;
+};
+
+timer {
+	compatible = "cirrus,clps711x-clocksource";
+	reg = <0x80000000 0>;
+	clocks = <&clks 6>, <&clks 7>;
+	clock-names = "tc1", "tc2";
+	interrupts = <9>;
+};
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index ba412e0..dfb1e7f 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -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
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 4038c2b..a7e9b73 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -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
diff --git a/drivers/clk/clk-clps711x.c b/drivers/clk/clk-clps711x.c
new file mode 100644
index 0000000..662908c
--- /dev/null
+++ b/drivers/clk/clk-clps711x.c
@@ -0,0 +1,150 @@
+/*
+ *  CLPS711X clk driver
+ *
+ *  Copyright (C) 2013 Alexander Shiyan <shc_work@mail.ru>
+ *
+ * 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;
+
+static void __init _clps711x_clk_init(phys_addr_t phys_base)
+{
+	const char *tc_parents[] = { "timer_lf", "timer_hf" };
+	u32 tmp, f_cpu, f_pll, f_bus, f_timh, f_spi;
+	void __iomem *pllr, *syscon1, *syscon2;
+
+	BUG_ON(!request_mem_region(phys_base + CLPS711X_PLLR, SZ_4, NULL));
+	BUG_ON(!request_mem_region(phys_base + CLPS711X_SYSCON1, SZ_128, NULL));
+	BUG_ON(!request_mem_region(phys_base + CLPS711X_SYSCON2, SZ_128, NULL));
+
+	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);
+	BUG_ON(!syscon1);
+
+	syscon2 = ioremap(phys_base + CLPS711X_SYSCON2, SZ_128);
+	BUG_ON(!syscon2);
+
+	tmp = readl(pllr) >> 24;
+	if (tmp)
+		f_pll = DIV_ROUND_UP(CLPS711X_OSC_FREQ * tmp, 2);
+	else
+		f_pll = 73728000; /* Old CPUs default value */
+
+	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);
+	clps711x_clk->clks[uart] =
+		clk_register_fixed_factor(NULL, "uart", "bus", 0, 1, 10);
+
+	clk_set_parent(clps711x_clk->clks[tc1], clps711x_clk->clks[timer_lf]);
+	clk_set_parent(clps711x_clk->clks[tc2], clps711x_clk->clks[timer_hf]);
+
+	/* Temporary export clocks for non-DT capable drivers */
+	clk_register_clkdev(clps711x_clk->clks[pll], "pll", NULL);
+	clk_register_clkdev(clps711x_clk->clks[spi], "spi", NULL);
+	clk_register_clkdev(clps711x_clk->clks[uart], "uart", NULL);
+
+	pr_debug("CPU: %u Hz, Timer: %u Hz\n", f_cpu, f_timh);
+}
+
+void __init clps711x_clk_init(phys_addr_t phys_base)
+{
+	_clps711x_clk_init(phys_base);
+
+	clk_register_clkdev(clps711x_clk->clks[tc1], "tc1", NULL);
+	clk_register_clkdev(clps711x_clk->clks[tc2], "tc2", NULL);
+}
+
+#ifdef CONFIG_OF
+static void __init clps711x_clk_init_dt(struct device_node *np)
+{
+	struct resource res;
+
+	BUG_ON(!of_address_to_resource(np, 0, &res));
+
+	_clps711x_clk_init(res.start);
+
+	clps711x_clk->clk_data.clks = clps711x_clk->clks;
+	clps711x_clk->clk_data.clk_num = clk_max;
+	of_clk_add_provider(np, of_clk_src_onecell_get,
+			    &clps711x_clk->clk_data);
+}
+CLK_OF_DECLARE(clps711x, "cirrus,clps711x-clk", clps711x_clk_init_dt);
+#endif
-- 
1.8.1.5

^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [PATCH 06/10] ARM: clps711x: Add CLPS711X clocksource driver
  2013-07-18 18:34 [PATCH 00/10] ARM: clps711x: Initial DT support / Fixes Alexander Shiyan
                   ` (4 preceding siblings ...)
  2013-07-18 18:34 ` [PATCH 05/10] ARM: clps711x: Add CLPS711X clk driver Alexander Shiyan
@ 2013-07-18 18:34 ` Alexander Shiyan
  2013-07-19 14:38   ` Daniel Lezcano
  2013-08-02 10:46   ` Mark Rutland
  2013-07-18 18:34 ` [PATCH 07/10] ARM: clps711x: Add CLPS711X irqchip driver Alexander Shiyan
                   ` (5 subsequent siblings)
  11 siblings, 2 replies; 48+ messages in thread
From: Alexander Shiyan @ 2013-07-18 18:34 UTC (permalink / raw)
  To: linux-arm-kernel

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 <shc_work@mail.ru>
---
 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

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index dfb1e7f..5c60cb7 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -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
diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index b7b9b04..bd6dc82 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -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)
diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index 8b00c5c..da6c102 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -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
diff --git a/drivers/clocksource/clps711x-clksrc.c b/drivers/clocksource/clps711x-clksrc.c
new file mode 100644
index 0000000..1749b0b
--- /dev/null
+++ b/drivers/clocksource/clps711x-clksrc.c
@@ -0,0 +1,151 @@
+/*
+ *  CLPS711X clocksource driver
+ *
+ *  Copyright (C) 2013 Alexander Shiyan <shc_work@mail.ru>
+ *
+ * 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 */
+		break;
+	}
+}
+
+static struct clock_event_device clps711x_clockevent = {
+	.name		= "clps711x-clockevent",
+	.rating		= 300,
+	.features	= CLOCK_EVT_FEAT_PERIODIC,
+	.set_mode	= clps711x_clockevent_set_mode,
+};
+
+static irqreturn_t clps711x_timer_interrupt(int irq, void *dev_id)
+{
+	clps711x_clockevent.event_handler(&clps711x_clockevent);
+
+	return IRQ_HANDLED;
+}
+
+static struct irqaction clps711x_timer_irq = {
+	.name		= "clps711x-timer",
+	.flags		= IRQF_TIMER | IRQF_IRQPOLL,
+	.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);
+
+	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",
+			      tc1_rate, 300, 16, clocksource_mmio_readw_down);
+
+	setup_sched_clock(clps711x_sched_clock_read, 16, tc1_rate);
+
+	/* Set Timer prescaler */
+	writew(DIV_ROUND_CLOSEST(tc2_rate, HZ), tc2d);
+
+	clockevents_config_and_register(&clps711x_clockevent, tc2_rate, 0, 0);
+
+	BUG_ON(setup_irq(clps711x_clksrc->irq, &clps711x_timer_irq));
+}
+
+void __init clps711x_clksrc_init(phys_addr_t phys_base, int irq)
+{
+	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
-- 
1.8.1.5

^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [PATCH 07/10] ARM: clps711x: Add CLPS711X irqchip driver
  2013-07-18 18:34 [PATCH 00/10] ARM: clps711x: Initial DT support / Fixes Alexander Shiyan
                   ` (5 preceding siblings ...)
  2013-07-18 18:34 ` [PATCH 06/10] ARM: clps711x: Add CLPS711X clocksource driver Alexander Shiyan
@ 2013-07-18 18:34 ` Alexander Shiyan
  2013-08-02 10:57   ` Mark Rutland
  2013-07-18 18:34 ` [PATCH 08/10] ARM: clps711x: Add CLPS711X cpuidle driver Alexander Shiyan
                   ` (4 subsequent siblings)
  11 siblings, 1 reply; 48+ messages in thread
From: Alexander Shiyan @ 2013-07-18 18:34 UTC (permalink / raw)
  To: linux-arm-kernel

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 <shc_work@mail.ru>
---
 .../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

diff --git a/Documentation/devicetree/bindings/interrupt-controller/cirrus,clps711x-intc.txt b/Documentation/devicetree/bindings/interrupt-controller/cirrus,clps711x-intc.txt
new file mode 100644
index 0000000..26f8983
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/cirrus,clps711x-intc.txt
@@ -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
+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)
+
+Example:
+
+intc: interrupt-controller {
+	compatible = "cirrus,clps711x-intc";
+	reg = <0x80000000 0>;
+	interrupt-controller;
+	#interrupt-cells = <1>;
+};
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 5c60cb7..6f0d238 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -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.
 
diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 1fea003..c94d00a 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -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
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index e65c41a..a80ff5b 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -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
diff --git a/drivers/irqchip/irq-clps711x.c b/drivers/irqchip/irq-clps711x.c
new file mode 100644
index 0000000..819baa9
--- /dev/null
+++ b/drivers/irqchip/irq-clps711x.c
@@ -0,0 +1,277 @@
+/*
+ *  CLPS711X IRQ driver
+ *
+ *  Copyright (C) 2013 Alexander Shiyan <shc_work@mail.ru>
+ *
+ * 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, },
+	[13]	= { CLPS711X_FLAG_EN, },
+	[14]	= { CLPS711X_FLAG_EN, CLPS711X_UMSEOI, },
+	[15]	= { CLPS711X_FLAG_EN, CLPS711X_SRXEOF, },
+	[16]	= { CLPS711X_FLAG_EN, CLPS711X_KBDEOI, },
+	[17]	= { CLPS711X_FLAG_EN, },
+	[18]	= { CLPS711X_FLAG_EN, },
+	[28]	= { CLPS711X_FLAG_EN, },
+	[29]	= { CLPS711X_FLAG_EN, },
+	[32]	= { CLPS711X_FLAG_FIQ, },
+};
+
+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;
+	}
+	if (!(x & 0xc000)) {
+		x <<= 2;
+		r -= 2;
+	}
+	if (!(x & 0x8000))
+		r--;
+
+	return r;
+}
+
+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;
+
+	if (clps711x_irqs[hw].flags & CLPS711X_FLAG_FIQ) {
+		handler = handle_bad_irq;
+		flags |= IRQF_NOAUTOEN;
+	} else if (clps711x_irqs[hw].phys_eoi) {
+		handler = handle_fasteoi_irq;
+
+		clps711x_intc->eoi[hw] =
+			clps711x_ioremap_one(clps711x_irqs[hw].phys_eoi);
+		if (IS_ERR_OR_NULL(clps711x_intc->eoi[hw]))
+			return PTR_ERR(clps711x_intc->eoi[hw]);
+
+		/* 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);
+}
+
+#ifdef CONFIG_IRQCHIP
+static int __init clps711x_intc_init_dt(struct device_node *np,
+					struct device_node *parent)
+{
+	struct resource res;
+
+	BUG_ON(of_address_to_resource(np, 0, &res));
+
+	_clps711x_intc_init(res.start, np);
+
+	return 0;
+}
+IRQCHIP_DECLARE(clps711x, "cirrus,clps711x-intc", clps711x_intc_init_dt);
+#endif
-- 
1.8.1.5

^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [PATCH 08/10] ARM: clps711x: Add CLPS711X cpuidle driver
  2013-07-18 18:34 [PATCH 00/10] ARM: clps711x: Initial DT support / Fixes Alexander Shiyan
                   ` (6 preceding siblings ...)
  2013-07-18 18:34 ` [PATCH 07/10] ARM: clps711x: Add CLPS711X irqchip driver Alexander Shiyan
@ 2013-07-18 18:34 ` Alexander Shiyan
  2013-07-20 21:42   ` Daniel Lezcano
  2013-08-02 11:10   ` Mark Rutland
  2013-07-18 18:35 ` [PATCH 09/10] ARM: clps711x: Migrate CLPS711X subarch to the new basic drivers Alexander Shiyan
                   ` (3 subsequent siblings)
  11 siblings, 2 replies; 48+ messages in thread
From: Alexander Shiyan @ 2013-07-18 18:34 UTC (permalink / raw)
  To: linux-arm-kernel

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 <shc_work@mail.ru>
---
 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

diff --git a/drivers/cpuidle/Kconfig b/drivers/cpuidle/Kconfig
index 0e2cd5c..c68a0cf 100644
--- a/drivers/cpuidle/Kconfig
+++ b/drivers/cpuidle/Kconfig
@@ -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
diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile
index 8767a7b..d07d2cb 100644
--- a/drivers/cpuidle/Makefile
+++ b/drivers/cpuidle/Makefile
@@ -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
diff --git a/drivers/cpuidle/cpuidle-clps711x.c b/drivers/cpuidle/cpuidle-clps711x.c
new file mode 100644
index 0000000..9f0129f
--- /dev/null
+++ b/drivers/cpuidle/cpuidle-clps711x.c
@@ -0,0 +1,80 @@
+/*
+ *  CLPS711X CPU idle driver
+ *
+ *  Copyright (C) 2013 Alexander Shiyan <shc_work@mail.ru>
+ *
+ * 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>
+#include <linux/platform_device.h>
+
+static void __iomem *clps711x_halt;
+
+static int clps711x_cpuidle_halt(struct cpuidle_device *dev,
+				 struct cpuidle_driver *drv, int index)
+{
+	writel(1, clps711x_halt);
+	asm volatile ("mov r0, r0");
+	asm volatile ("mov r0, r0");
+
+	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)
+{
+	cpuidle_unregister(&clps711x_idle_driver);
+
+	return 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",
+		.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 <shc_work@mail.ru>");
+MODULE_DESCRIPTION("CLPS711X CPU idle driver");
+MODULE_LICENSE("GPL");
-- 
1.8.1.5

^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [PATCH 09/10] ARM: clps711x: Migrate CLPS711X subarch to the new basic drivers
  2013-07-18 18:34 [PATCH 00/10] ARM: clps711x: Initial DT support / Fixes Alexander Shiyan
                   ` (7 preceding siblings ...)
  2013-07-18 18:34 ` [PATCH 08/10] ARM: clps711x: Add CLPS711X cpuidle driver Alexander Shiyan
@ 2013-07-18 18:35 ` Alexander Shiyan
  2013-07-18 18:35 ` [PATCH 10/10] ARM: clps711x: Add initial DT support Alexander Shiyan
                   ` (2 subsequent siblings)
  11 siblings, 0 replies; 48+ messages in thread
From: Alexander Shiyan @ 2013-07-18 18:35 UTC (permalink / raw)
  To: linux-arm-kernel

This patch remove old code and migrate Cirrus Logic CLPS711X subarch
to the new basic drivers: clk, clocksource, irqchip and cpuidle.
This change brings this subarch for use in multiplatform build.

Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
---
 arch/arm/mach-clps711x/board-autcpu12.c |   3 -
 arch/arm/mach-clps711x/board-cdb89712.c |   3 -
 arch/arm/mach-clps711x/board-clep7312.c |   3 -
 arch/arm/mach-clps711x/board-edb7211.c  |   3 -
 arch/arm/mach-clps711x/board-p720t.c    |   3 -
 arch/arm/mach-clps711x/common.c         | 345 +-------------------------------
 arch/arm/mach-clps711x/common.h         |   3 -
 arch/arm/mach-clps711x/devices.c        |  10 +
 8 files changed, 16 insertions(+), 357 deletions(-)

diff --git a/arch/arm/mach-clps711x/board-autcpu12.c b/arch/arm/mach-clps711x/board-autcpu12.c
index f8d71a8..b1c77ad 100644
--- a/arch/arm/mach-clps711x/board-autcpu12.c
+++ b/arch/arm/mach-clps711x/board-autcpu12.c
@@ -265,14 +265,11 @@ static void __init autcpu12_init_late(void)
 MACHINE_START(AUTCPU12, "autronix autcpu12")
 	/* Maintainer: Thomas Gleixner */
 	.atag_offset	= 0x20000,
-	.nr_irqs	= CLPS711X_NR_IRQS,
 	.map_io		= clps711x_map_io,
-	.init_early	= clps711x_init_early,
 	.init_irq	= clps711x_init_irq,
 	.init_time	= clps711x_timer_init,
 	.init_machine	= autcpu12_init,
 	.init_late	= autcpu12_init_late,
-	.handle_irq	= clps711x_handle_irq,
 	.restart	= clps711x_restart,
 MACHINE_END
 
diff --git a/arch/arm/mach-clps711x/board-cdb89712.c b/arch/arm/mach-clps711x/board-cdb89712.c
index a9e38c6..1ec378c 100644
--- a/arch/arm/mach-clps711x/board-cdb89712.c
+++ b/arch/arm/mach-clps711x/board-cdb89712.c
@@ -139,12 +139,9 @@ static void __init cdb89712_init(void)
 MACHINE_START(CDB89712, "Cirrus-CDB89712")
 	/* Maintainer: Ray Lehtiniemi */
 	.atag_offset	= 0x100,
-	.nr_irqs	= CLPS711X_NR_IRQS,
 	.map_io		= clps711x_map_io,
-	.init_early	= clps711x_init_early,
 	.init_irq	= clps711x_init_irq,
 	.init_time	= clps711x_timer_init,
 	.init_machine	= cdb89712_init,
-	.handle_irq	= clps711x_handle_irq,
 	.restart	= clps711x_restart,
 MACHINE_END
diff --git a/arch/arm/mach-clps711x/board-clep7312.c b/arch/arm/mach-clps711x/board-clep7312.c
index b476424..1f3b403 100644
--- a/arch/arm/mach-clps711x/board-clep7312.c
+++ b/arch/arm/mach-clps711x/board-clep7312.c
@@ -36,12 +36,9 @@ fixup_clep7312(struct tag *tags, char **cmdline, struct meminfo *mi)
 MACHINE_START(CLEP7212, "Cirrus Logic 7212/7312")
 	/* Maintainer: Nobody */
 	.atag_offset	= 0x0100,
-	.nr_irqs	= CLPS711X_NR_IRQS,
 	.fixup		= fixup_clep7312,
 	.map_io		= clps711x_map_io,
-	.init_early	= clps711x_init_early,
 	.init_irq	= clps711x_init_irq,
 	.init_time	= clps711x_timer_init,
-	.handle_irq	= clps711x_handle_irq,
 	.restart	= clps711x_restart,
 MACHINE_END
diff --git a/arch/arm/mach-clps711x/board-edb7211.c b/arch/arm/mach-clps711x/board-edb7211.c
index fe6184e..fa4580f 100644
--- a/arch/arm/mach-clps711x/board-edb7211.c
+++ b/arch/arm/mach-clps711x/board-edb7211.c
@@ -177,15 +177,12 @@ static void __init edb7211_init_late(void)
 MACHINE_START(EDB7211, "CL-EDB7211 (EP7211 eval board)")
 	/* Maintainer: Jon McClintock */
 	.atag_offset	= VIDEORAM_SIZE + 0x100,
-	.nr_irqs	= CLPS711X_NR_IRQS,
 	.fixup		= fixup_edb7211,
 	.reserve	= edb7211_reserve,
 	.map_io		= clps711x_map_io,
-	.init_early	= clps711x_init_early,
 	.init_irq	= clps711x_init_irq,
 	.init_time	= clps711x_timer_init,
 	.init_machine	= edb7211_init,
 	.init_late	= edb7211_init_late,
-	.handle_irq	= clps711x_handle_irq,
 	.restart	= clps711x_restart,
 MACHINE_END
diff --git a/arch/arm/mach-clps711x/board-p720t.c b/arch/arm/mach-clps711x/board-p720t.c
index dd81b06..4a2ec28 100644
--- a/arch/arm/mach-clps711x/board-p720t.c
+++ b/arch/arm/mach-clps711x/board-p720t.c
@@ -363,14 +363,11 @@ static void __init p720t_init_late(void)
 MACHINE_START(P720T, "ARM-Prospector720T")
 	/* Maintainer: ARM Ltd/Deep Blue Solutions Ltd */
 	.atag_offset	= 0x100,
-	.nr_irqs	= CLPS711X_NR_IRQS,
 	.fixup		= fixup_p720t,
 	.map_io		= clps711x_map_io,
-	.init_early	= clps711x_init_early,
 	.init_irq	= clps711x_init_irq,
 	.init_time	= clps711x_timer_init,
 	.init_machine	= p720t_init,
 	.init_late	= p720t_init_late,
-	.handle_irq	= clps711x_handle_irq,
 	.restart	= clps711x_restart,
 MACHINE_END
diff --git a/arch/arm/mach-clps711x/common.c b/arch/arm/mach-clps711x/common.c
index 4ca2f3c..d226609 100644
--- a/arch/arm/mach-clps711x/common.c
+++ b/arch/arm/mach-clps711x/common.c
@@ -22,26 +22,14 @@
 #include <linux/io.h>
 #include <linux/init.h>
 #include <linux/sizes.h>
-#include <linux/interrupt.h>
-#include <linux/irq.h>
-#include <linux/clk.h>
-#include <linux/clkdev.h>
-#include <linux/clockchips.h>
 #include <linux/clocksource.h>
 #include <linux/clk-provider.h>
 
-#include <asm/exception.h>
-#include <asm/mach/irq.h>
 #include <asm/mach/map.h>
-#include <asm/mach/time.h>
-#include <asm/sched_clock.h>
 #include <asm/system_misc.h>
 
 #include <mach/hardware.h>
 
-static struct clk *clk_pll, *clk_bus, *clk_uart, *clk_timerl, *clk_timerh,
-		  *clk_tint, *clk_spi;
-
 /*
  * This maps the generic CLPS711x registers
  */
@@ -59,344 +47,23 @@ void __init clps711x_map_io(void)
 	iotable_init(clps711x_io_desc, ARRAY_SIZE(clps711x_io_desc));
 }
 
-static void int1_mask(struct irq_data *d)
-{
-	u32 intmr1;
-
-	intmr1 = clps_readl(INTMR1);
-	intmr1 &= ~(1 << d->irq);
-	clps_writel(intmr1, INTMR1);
-}
-
-static void int1_eoi(struct irq_data *d)
-{
-	switch (d->irq) {
-	case IRQ_CSINT:  clps_writel(0, COEOI);  break;
-	case IRQ_TC1OI:  clps_writel(0, TC1EOI); break;
-	case IRQ_TC2OI:  clps_writel(0, TC2EOI); break;
-	case IRQ_RTCMI:  clps_writel(0, RTCEOI); break;
-	case IRQ_TINT:   clps_writel(0, TEOI);   break;
-	case IRQ_UMSINT: clps_writel(0, UMSEOI); break;
-	}
-}
-
-static void int1_unmask(struct irq_data *d)
-{
-	u32 intmr1;
-
-	intmr1 = clps_readl(INTMR1);
-	intmr1 |= 1 << d->irq;
-	clps_writel(intmr1, INTMR1);
-}
-
-static struct irq_chip int1_chip = {
-	.name		= "Interrupt Vector 1",
-	.irq_eoi	= int1_eoi,
-	.irq_mask	= int1_mask,
-	.irq_unmask	= int1_unmask,
-};
-
-static void int2_mask(struct irq_data *d)
-{
-	u32 intmr2;
-
-	intmr2 = clps_readl(INTMR2);
-	intmr2 &= ~(1 << (d->irq - 16));
-	clps_writel(intmr2, INTMR2);
-}
-
-static void int2_eoi(struct irq_data *d)
-{
-	switch (d->irq) {
-	case IRQ_KBDINT: clps_writel(0, KBDEOI); break;
-	}
-}
-
-static void int2_unmask(struct irq_data *d)
-{
-	u32 intmr2;
-
-	intmr2 = clps_readl(INTMR2);
-	intmr2 |= 1 << (d->irq - 16);
-	clps_writel(intmr2, INTMR2);
-}
-
-static struct irq_chip int2_chip = {
-	.name		= "Interrupt Vector 2",
-	.irq_eoi	= int2_eoi,
-	.irq_mask	= int2_mask,
-	.irq_unmask	= int2_unmask,
-};
-
-static void int3_mask(struct irq_data *d)
-{
-	u32 intmr3;
-
-	intmr3 = clps_readl(INTMR3);
-	intmr3 &= ~(1 << (d->irq - 32));
-	clps_writel(intmr3, INTMR3);
-}
-
-static void int3_unmask(struct irq_data *d)
-{
-	u32 intmr3;
-
-	intmr3 = clps_readl(INTMR3);
-	intmr3 |= 1 << (d->irq - 32);
-	clps_writel(intmr3, INTMR3);
-}
-
-static struct irq_chip int3_chip = {
-	.name		= "Interrupt Vector 3",
-	.irq_mask	= int3_mask,
-	.irq_unmask	= int3_unmask,
-};
-
-static struct {
-	int			nr;
-	struct irq_chip		*chip;
-	irq_flow_handler_t	handle;
-} clps711x_irqdescs[] __initdata = {
-	{ IRQ_CSINT,	&int1_chip,	handle_fasteoi_irq,	},
-	{ IRQ_EINT1,	&int1_chip,	handle_level_irq,	},
-	{ IRQ_EINT2,	&int1_chip,	handle_level_irq,	},
-	{ IRQ_EINT3,	&int1_chip,	handle_level_irq,	},
-	{ IRQ_TC1OI,	&int1_chip,	handle_fasteoi_irq,	},
-	{ IRQ_TC2OI,	&int1_chip,	handle_fasteoi_irq,	},
-	{ IRQ_RTCMI,	&int1_chip,	handle_fasteoi_irq,	},
-	{ IRQ_TINT,	&int1_chip,	handle_fasteoi_irq,	},
-	{ IRQ_UTXINT1,	&int1_chip,	handle_level_irq,	},
-	{ IRQ_URXINT1,	&int1_chip,	handle_level_irq,	},
-	{ IRQ_UMSINT,	&int1_chip,	handle_fasteoi_irq,	},
-	{ IRQ_SSEOTI,	&int1_chip,	handle_level_irq,	},
-	{ IRQ_KBDINT,	&int2_chip,	handle_fasteoi_irq,	},
-	{ IRQ_SS2RX,	&int2_chip,	handle_level_irq,	},
-	{ IRQ_SS2TX,	&int2_chip,	handle_level_irq,	},
-	{ IRQ_UTXINT2,	&int2_chip,	handle_level_irq,	},
-	{ IRQ_URXINT2,	&int2_chip,	handle_level_irq,	},
-};
-
 void __init clps711x_init_irq(void)
 {
-	unsigned int i;
-
-	/* Disable interrupts */
-	clps_writel(0, INTMR1);
-	clps_writel(0, INTMR2);
-	clps_writel(0, INTMR3);
-
-	/* Clear down any pending interrupts */
-	clps_writel(0, BLEOI);
-	clps_writel(0, MCEOI);
-	clps_writel(0, COEOI);
-	clps_writel(0, TC1EOI);
-	clps_writel(0, TC2EOI);
-	clps_writel(0, RTCEOI);
-	clps_writel(0, TEOI);
-	clps_writel(0, UMSEOI);
-	clps_writel(0, KBDEOI);
-	clps_writel(0, SRXEOF);
-	clps_writel(0xffffffff, DAISR);
-
-	for (i = 0; i < ARRAY_SIZE(clps711x_irqdescs); i++) {
-		irq_set_chip_and_handler(clps711x_irqdescs[i].nr,
-					 clps711x_irqdescs[i].chip,
-					 clps711x_irqdescs[i].handle);
-		set_irq_flags(clps711x_irqdescs[i].nr,
-			      IRQF_VALID | IRQF_PROBE);
-	}
-
-	if (IS_ENABLED(CONFIG_FIQ)) {
-		init_FIQ(0);
-		irq_set_chip_and_handler(IRQ_DAIINT, &int3_chip,
-					 handle_bad_irq);
-		set_irq_flags(IRQ_DAIINT,
-			      IRQF_VALID | IRQF_PROBE | IRQF_NOAUTOEN);
-	}
-}
-
-static inline u32 fls16(u32 x)
-{
-	u32 r = 15;
+	extern void clps711x_intc_init(phys_addr_t);
 
-	if (!(x & 0xff00)) {
-		x <<= 8;
-		r -= 8;
-	}
-	if (!(x & 0xf000)) {
-		x <<= 4;
-		r -= 4;
-	}
-	if (!(x & 0xc000)) {
-		x <<= 2;
-		r -= 2;
-	}
-	if (!(x & 0x8000))
-		r--;
-
-	return r;
-}
-
-asmlinkage void __exception_irq_entry clps711x_handle_irq(struct pt_regs *regs)
-{
-	do {
-		u32 irqstat;
-		void __iomem *base = CLPS711X_VIRT_BASE;
-
-		irqstat = readw_relaxed(base + INTSR1) &
-			  readw_relaxed(base + INTMR1);
-		if (irqstat)
-			handle_IRQ(fls16(irqstat), regs);
-
-		irqstat = readw_relaxed(base + INTSR2) &
-			  readw_relaxed(base + INTMR2);
-		if (irqstat) {
-			handle_IRQ(fls16(irqstat) + 16, regs);
-			continue;
-		}
-
-		break;
-	} while (1);
-}
-
-static u32 notrace clps711x_sched_clock_read(void)
-{
-	return ~readw_relaxed(CLPS711X_VIRT_BASE + TC1D);
-}
-
-static void clps711x_clockevent_set_mode(enum clock_event_mode mode,
-					 struct clock_event_device *evt)
-{
-	disable_irq(IRQ_TC2OI);
-
-	switch (mode) {
-	case CLOCK_EVT_MODE_PERIODIC:
-		enable_irq(IRQ_TC2OI);
-		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 */
-		break;
-	}
-}
-
-static struct clock_event_device clockevent_clps711x = {
-	.name		= "clps711x-clockevent",
-	.rating		= 300,
-	.features	= CLOCK_EVT_FEAT_PERIODIC,
-	.set_mode	= clps711x_clockevent_set_mode,
-};
-
-static irqreturn_t clps711x_timer_interrupt(int irq, void *dev_id)
-{
-	clockevent_clps711x.event_handler(&clockevent_clps711x);
-
-	return IRQ_HANDLED;
-}
-
-static struct irqaction clps711x_timer_irq = {
-	.name		= "clps711x-timer",
-	.flags		= IRQF_TIMER | IRQF_IRQPOLL,
-	.handler	= clps711x_timer_interrupt,
-};
-
-static void add_fixed_clk(struct clk *clk, const char *name, int rate)
-{
-	clk = clk_register_fixed_rate(NULL, name, NULL, CLK_IS_ROOT, rate);
-	clk_register_clkdev(clk, name, NULL);
+	clps711x_intc_init(CLPS711X_PHYS_BASE);
 }
 
 void __init clps711x_timer_init(void)
 {
-	int osc, ext, pll, cpu, bus, timl, timh, uart, spi;
-	u32 tmp;
-
-	osc = 3686400;
-	ext = 13000000;
-
-	tmp = clps_readl(PLLR) >> 24;
-	if (tmp)
-		pll = (osc * tmp) / 2;
-	else
-		pll = 73728000; /* Default value */
-
-	tmp = clps_readl(SYSFLG2);
-	if (tmp & SYSFLG2_CKMODE) {
-		cpu = ext;
-		bus = cpu;
-		spi = 135400;
-		pll = 0;
-	} else {
-		cpu = pll;
-		if (cpu >= 36864000)
-			bus = cpu / 2;
-		else
-			bus = 36864000 / 2;
-		spi = cpu / 576;
-	}
-
-	uart = bus / 10;
-
-	if (tmp & SYSFLG2_CKMODE) {
-		tmp = clps_readl(SYSCON2);
-		if (tmp & SYSCON2_OSTB)
-			timh = ext / 26;
-		else
-			timh = 541440;
-	} else
-		timh = DIV_ROUND_CLOSEST(cpu, 144);
-
-	timl = DIV_ROUND_CLOSEST(timh, 256);
-
-	/* All clocks are fixed */
-	add_fixed_clk(clk_pll, "pll", pll);
-	add_fixed_clk(clk_bus, "bus", bus);
-	add_fixed_clk(clk_uart, "uart", uart);
-	add_fixed_clk(clk_timerl, "timer_lf", timl);
-	add_fixed_clk(clk_timerh, "timer_hf", timh);
-	add_fixed_clk(clk_tint, "tint", 64);
-	add_fixed_clk(clk_spi, "spi", spi);
-
-	pr_info("CPU frequency set at %i Hz.\n", cpu);
+	extern void clps711x_clk_init(phys_addr_t);
+	extern void clps711x_clksrc_init(phys_addr_t, int);
 
-	/* Start Timer1 in free running mode (Low frequency) */
-	tmp = clps_readl(SYSCON1) & ~(SYSCON1_TC1S | SYSCON1_TC1M);
-	clps_writel(tmp, SYSCON1);
-
-	setup_sched_clock(clps711x_sched_clock_read, 16, timl);
-
-	clocksource_mmio_init(CLPS711X_VIRT_BASE + TC1D,
-			      "clps711x_clocksource", timl, 300, 16,
-			      clocksource_mmio_readw_down);
-
-	/* Set Timer2 prescaler */
-	clps_writew(DIV_ROUND_CLOSEST(timh, HZ), TC2D);
-
-	/* Start Timer2 in prescale mode (High frequency)*/
-	tmp = clps_readl(SYSCON1) | SYSCON1_TC2M | SYSCON1_TC2S;
-	clps_writel(tmp, SYSCON1);
-
-	clockevents_config_and_register(&clockevent_clps711x, timh, 0, 0);
-
-	setup_irq(IRQ_TC2OI, &clps711x_timer_irq);
+	clps711x_clk_init(CLPS711X_PHYS_BASE);
+	clps711x_clksrc_init(CLPS711X_PHYS_BASE, IRQ_TC2OI);
 }
 
 void clps711x_restart(enum reboot_mode mode, const char *cmd)
 {
 	soft_restart(0);
 }
-
-static void clps711x_idle(void)
-{
-	clps_writel(1, HALT);
-	asm("mov r0, r0");
-	asm("mov r0, r0");
-}
-
-void __init clps711x_init_early(void)
-{
-	arm_pm_idle = clps711x_idle;
-}
diff --git a/arch/arm/mach-clps711x/common.h b/arch/arm/mach-clps711x/common.h
index 9a6767b..77f3130 100644
--- a/arch/arm/mach-clps711x/common.h
+++ b/arch/arm/mach-clps711x/common.h
@@ -6,13 +6,10 @@
 
 #include <linux/reboot.h>
 
-#define CLPS711X_NR_IRQS	(33)
 #define CLPS711X_NR_GPIO	(4 * 8 + 3)
 #define CLPS711X_GPIO(prt, bit)	((prt) * 8 + (bit))
 
 extern void clps711x_map_io(void);
 extern void clps711x_init_irq(void);
 extern void clps711x_timer_init(void);
-extern void clps711x_handle_irq(struct pt_regs *regs);
 extern void clps711x_restart(enum reboot_mode mode, const char *cmd);
-extern void clps711x_init_early(void);
diff --git a/arch/arm/mach-clps711x/devices.c b/arch/arm/mach-clps711x/devices.c
index fb77d14..aa24dc4 100644
--- a/arch/arm/mach-clps711x/devices.c
+++ b/arch/arm/mach-clps711x/devices.c
@@ -14,6 +14,15 @@
 
 #include <mach/hardware.h>
 
+static const struct resource clps711x_cpuidle_res __initconst =
+	DEFINE_RES_MEM(CLPS711X_PHYS_BASE + HALT, SZ_4);
+
+static void __init clps711x_add_cpuidle(void)
+{
+	platform_device_register_simple("clps711x-cpuidle", PLATFORM_DEVID_NONE,
+					&clps711x_cpuidle_res, 1);
+}
+
 static const phys_addr_t clps711x_gpios[][2] __initconst = {
 	{ PADR, PADDR },
 	{ PBDR, PBDDR },
@@ -63,6 +72,7 @@ static void __init clps711x_add_syscon(void)
 
 void __init clps711x_devices_init(void)
 {
+	clps711x_add_cpuidle();
 	clps711x_add_gpio();
 	clps711x_add_syscon();
 }
-- 
1.8.1.5

^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [PATCH 10/10] ARM: clps711x: Add initial DT support
  2013-07-18 18:34 [PATCH 00/10] ARM: clps711x: Initial DT support / Fixes Alexander Shiyan
                   ` (8 preceding siblings ...)
  2013-07-18 18:35 ` [PATCH 09/10] ARM: clps711x: Migrate CLPS711X subarch to the new basic drivers Alexander Shiyan
@ 2013-07-18 18:35 ` Alexander Shiyan
  2013-08-02 11:15   ` Mark Rutland
  2013-08-03  3:54 ` [PATCH 00/10] ARM: clps711x: Initial DT support / Fixes Alexander Shiyan
  2013-08-14  6:31 ` Olof Johansson
  11 siblings, 1 reply; 48+ messages in thread
From: Alexander Shiyan @ 2013-07-18 18:35 UTC (permalink / raw)
  To: 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 <shc_work@mail.ru>
---
 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

diff --git a/arch/arm/mach-clps711x/Kconfig b/arch/arm/mach-clps711x/Kconfig
index bea6295..8ef38c6 100644
--- a/arch/arm/mach-clps711x/Kconfig
+++ b/arch/arm/mach-clps711x/Kconfig
@@ -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
diff --git a/arch/arm/mach-clps711x/Makefile b/arch/arm/mach-clps711x/Makefile
index f04151e..77d3a76 100644
--- a/arch/arm/mach-clps711x/Makefile
+++ b/arch/arm/mach-clps711x/Makefile
@@ -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
diff --git a/arch/arm/mach-clps711x/board-dt.c b/arch/arm/mach-clps711x/board-dt.c
new file mode 100644
index 0000000..5b6f3f1
--- /dev/null
+++ b/arch/arm/mach-clps711x/board-dt.c
@@ -0,0 +1,56 @@
+/*
+ *  CLPS711X Generic DT board
+ *
+ *  Author: Alexander Shiyan <shc_work@mail.ru>, 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>
+
+#include <mach/hardware.h>
+
+#include "common.h"
+
+void __init clps711x_timer_init_dt(void)
+{
+	of_clk_init(NULL);
+	clocksource_of_init();
+}
+
+static struct of_dev_auxdata clps711x_auxdata_lookup[] __initdata = {
+	OF_DEV_AUXDATA("cirrus,clps711x-syscon", CLPS711X_PHYS_BASE + SYSCON1,
+		       "syscon.1", NULL),
+	OF_DEV_AUXDATA("cirrus,clps711x-syscon", CLPS711X_PHYS_BASE + SYSCON2,
+		       "syscon.2", NULL),
+	OF_DEV_AUXDATA("cirrus,clps711x-syscon", CLPS711X_PHYS_BASE + SYSCON3,
+		       "syscon.3", NULL),
+	{ }
+};
+
+static void __init clps711x_init_dt(void)
+{
+	of_platform_populate(NULL, of_default_bus_match_table,
+			     clps711x_auxdata_lookup, NULL);
+}
+
+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
-- 
1.8.1.5

^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [PATCH 06/10] ARM: clps711x: Add CLPS711X clocksource driver
  2013-07-18 18:34 ` [PATCH 06/10] ARM: clps711x: Add CLPS711X clocksource driver Alexander Shiyan
@ 2013-07-19 14:38   ` Daniel Lezcano
  2013-07-20  3:44     ` Alexander Shiyan
  2013-08-02 10:46   ` Mark Rutland
  1 sibling, 1 reply; 48+ messages in thread
From: Daniel Lezcano @ 2013-07-19 14:38 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Alex,

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

On 07/18/2013 08:34 PM, Alexander Shiyan wrote:
> 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.

> Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
> ---
>  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
> 
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index dfb1e7f..5c60cb7 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -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
> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> index b7b9b04..bd6dc82 100644
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig
> @@ -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)
> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> index 8b00c5c..da6c102 100644
> --- a/drivers/clocksource/Makefile
> +++ b/drivers/clocksource/Makefile
> @@ -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
> diff --git a/drivers/clocksource/clps711x-clksrc.c b/drivers/clocksource/clps711x-clksrc.c
> new file mode 100644
> index 0000000..1749b0b
> --- /dev/null
> +++ b/drivers/clocksource/clps711x-clksrc.c
> @@ -0,0 +1,151 @@
> +/*
> + *  CLPS711X clocksource driver
> + *
> + *  Copyright (C) 2013 Alexander Shiyan <shc_work@mail.ru>
> + *
> + * 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)

Alignment.

> +
> +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.

> +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);

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

> +> +	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 */
> +		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 ?

> +
> +static struct clock_event_device clps711x_clockevent = {
> +	.name		= "clps711x-clockevent",
> +	.rating		= 300,
> +	.features	= CLOCK_EVT_FEAT_PERIODIC,
> +	.set_mode	= clps711x_clockevent_set_mode,
> +};
> +
> +static irqreturn_t clps711x_timer_interrupt(int irq, void *dev_id)
> +{
> +	clps711x_clockevent.event_handler(&clps711x_clockevent);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static struct irqaction clps711x_timer_irq = {
> +	.name		= "clps711x-timer",
> +	.flags		= IRQF_TIMER | IRQF_IRQPOLL,

Why do you need IRQF_IRQPOOL ?

> +	.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);
> +
> +	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;

	clps711x_clockevent.irq = irq;

	clps711x_clockevent.cpumask = ?

> +	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 ?

> +
> +	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);
> +
> +	/* Set Timer prescaler */
> +	writew(DIV_ROUND_CLOSEST(tc2_rate, HZ), tc2d);
> +
> +	clockevents_config_and_register(&clps711x_clockevent, tc2_rate, 0, 0);
> +
> +	BUG_ON(setup_irq(clps711x_clksrc->irq, &clps711x_timer_irq));
> +}
> +
> +void __init clps711x_clksrc_init(phys_addr_t phys_base, int irq)

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

> +{
> +	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
> 


-- 
 <http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH 06/10] ARM: clps711x: Add CLPS711X clocksource driver
  2013-07-19 14:38   ` Daniel Lezcano
@ 2013-07-20  3:44     ` Alexander Shiyan
  2013-07-20 21:48       ` Daniel Lezcano
  0 siblings, 1 reply; 48+ messages in thread
From: Alexander Shiyan @ 2013-07-20  3:44 UTC (permalink / raw)
  To: linux-arm-kernel

On Fri, 19 Jul 2013 16:38:54 +0200
Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> (Added Thomas and John in Cc in case they want to review the patch).
> 
> On 07/18/2013 08:34 PM, Alexander Shiyan wrote:
> > 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.

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.

> > Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
> > ---
> >  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
[...]
> > +#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.

> > +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.

[...]
> > +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.

> > +> +	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 */
> > +		break;
> > +	}
> > +}
> 
> I am not expert in interrupts, but it is possible this interrupt could
> be shared ?

Timer interrupt? No.

> 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.

[...]
> > +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.

[...]
> > +	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?

[...]
> > +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.

[...]

Thanks.

-- 
Alexander Shiyan <shc_work@mail.ru>

^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH 08/10] ARM: clps711x: Add CLPS711X cpuidle driver
  2013-07-18 18:34 ` [PATCH 08/10] ARM: clps711x: Add CLPS711X cpuidle driver Alexander Shiyan
@ 2013-07-20 21:42   ` Daniel Lezcano
  2013-07-21  4:11     ` Alexander Shiyan
  2013-08-02 11:10   ` Mark Rutland
  1 sibling, 1 reply; 48+ messages in thread
From: Daniel Lezcano @ 2013-07-20 21:42 UTC (permalink / raw)
  To: linux-arm-kernel

On 07/18/2013 08:34 PM, Alexander Shiyan wrote:
> 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 <shc_work@mail.ru>
> ---
>  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
> 
> diff --git a/drivers/cpuidle/Kconfig b/drivers/cpuidle/Kconfig
> index 0e2cd5c..c68a0cf 100644
> --- a/drivers/cpuidle/Kconfig
> +++ b/drivers/cpuidle/Kconfig
> @@ -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
> diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile
> index 8767a7b..d07d2cb 100644
> --- a/drivers/cpuidle/Makefile
> +++ b/drivers/cpuidle/Makefile
> @@ -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
> diff --git a/drivers/cpuidle/cpuidle-clps711x.c b/drivers/cpuidle/cpuidle-clps711x.c
> new file mode 100644
> index 0000000..9f0129f
> --- /dev/null
> +++ b/drivers/cpuidle/cpuidle-clps711x.c
> @@ -0,0 +1,80 @@
> +/*
> + *  CLPS711X CPU idle driver
> + *
> + *  Copyright (C) 2013 Alexander Shiyan <shc_work@mail.ru>
> + *
> + * 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>
> +#include <linux/platform_device.h>
> +
> +static void __iomem *clps711x_halt;
> +
> +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) ?

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

Why are needed these two volatile ?

> +	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)
> +{
> +	cpuidle_unregister(&clps711x_idle_driver);
> +
> +	return 0;
> +}

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

> +
> +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",
> +		.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

> +
> +MODULE_AUTHOR("Alexander Shiyan <shc_work@mail.ru>");
> +MODULE_DESCRIPTION("CLPS711X CPU idle driver");
> +MODULE_LICENSE("GPL");




-- 
 <http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH 06/10] ARM: clps711x: Add CLPS711X clocksource driver
  2013-07-20  3:44     ` Alexander Shiyan
@ 2013-07-20 21:48       ` Daniel Lezcano
  2013-07-21  4:30         ` Alexander Shiyan
  0 siblings, 1 reply; 48+ messages in thread
From: Daniel Lezcano @ 2013-07-20 21:48 UTC (permalink / raw)
  To: linux-arm-kernel

On 07/20/2013 05:44 AM, Alexander Shiyan wrote:
> On Fri, 19 Jul 2013 16:38:54 +0200
> Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>> (Added Thomas and John in Cc in case they want to review the patch).
>>
>> On 07/18/2013 08:34 PM, Alexander Shiyan wrote:
>>> 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.
> 
> 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.

I understand there is nothing extra in the code but I was talking about
some specificity of the driver if there are (eg. timer must be disabled
with disable/enable irq).

>>> Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
>>> ---
>>>  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
> [...]
>>> +#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.

Ok.

>>> +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.
> 
> [...]
>>> +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.
> 
>>> +> +	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 */
>>> +		break;
>>> +	}
>>> +}
>>
>> I am not expert in interrupts, but it is possible this interrupt could
>> be shared ?
> 
> Timer interrupt? No.
> 
>> 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.

Ok.

> [...]
>>> +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.

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

> [...]
>>> +	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?

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

> [...]
>>> +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.

Ok, thanks.


-- 
 <http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH 08/10] ARM: clps711x: Add CLPS711X cpuidle driver
  2013-07-20 21:42   ` Daniel Lezcano
@ 2013-07-21  4:11     ` Alexander Shiyan
  2013-07-21  8:32       ` Daniel Lezcano
  2013-07-22 11:40       ` Russell King - ARM Linux
  0 siblings, 2 replies; 48+ messages in thread
From: Alexander Shiyan @ 2013-07-21  4:11 UTC (permalink / raw)
  To: linux-arm-kernel

On Sat, 20 Jul 2013 23:42:11 +0200
Daniel Lezcano <daniel.lezcano@linaro.org> wrote:

> On 07/18/2013 08:34 PM, Alexander Shiyan wrote:
> > 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 <shc_work@mail.ru>
> > ---
> >  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
[...]
> > +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."

> > +	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.

[...]
> > +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.

> > +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",
> > +		.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?

[...]

-- 
Alexander Shiyan <shc_work@mail.ru>

^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH 06/10] ARM: clps711x: Add CLPS711X clocksource driver
  2013-07-20 21:48       ` Daniel Lezcano
@ 2013-07-21  4:30         ` Alexander Shiyan
  0 siblings, 0 replies; 48+ messages in thread
From: Alexander Shiyan @ 2013-07-21  4:30 UTC (permalink / raw)
  To: linux-arm-kernel

On Sat, 20 Jul 2013 23:48:30 +0200
Daniel Lezcano <daniel.lezcano@linaro.org> wrote:

> On 07/20/2013 05:44 AM, Alexander Shiyan wrote:
> > On Fri, 19 Jul 2013 16:38:54 +0200
> > Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> >> (Added Thomas and John in Cc in case they want to review the patch).
> >>
> >> On 07/18/2013 08:34 PM, Alexander Shiyan wrote:
> >>> 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.
[...]
> >>> +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.
> 
> So IIUC, IRQPOLL is to switch to polling mode when a lot of interrupts
> occur, right ?

I'm also not an expert on these flags, but realized appointment just like you.

> >>> +	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?
> 
> I prefer to encapsulate readl/writel calls inside a small function with
> an explicit name, comments are still welcome.

Ok.

[...]

-- 
Alexander Shiyan <shc_work@mail.ru>

^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH 08/10] ARM: clps711x: Add CLPS711X cpuidle driver
  2013-07-21  4:11     ` Alexander Shiyan
@ 2013-07-21  8:32       ` Daniel Lezcano
  2013-07-21 10:04         ` Alexander Shiyan
  2013-07-22 11:40       ` Russell King - ARM Linux
  1 sibling, 1 reply; 48+ messages in thread
From: Daniel Lezcano @ 2013-07-21  8:32 UTC (permalink / raw)
  To: linux-arm-kernel

On 07/21/2013 06:11 AM, Alexander Shiyan wrote:
> On Sat, 20 Jul 2013 23:42:11 +0200
> Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> 
>> On 07/18/2013 08:34 PM, Alexander Shiyan wrote:
>>> 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 <shc_work@mail.ru>
>>> ---
>>>  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
> [...]
>>> +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."

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.

>>> +	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.

Ok.

> [...]
>>> +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.
> 
>>> +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",
>>> +		.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?

I like the approach :)

Thanks
  -- Daniel


-- 
 <http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH 08/10] ARM: clps711x: Add CLPS711X cpuidle driver
  2013-07-21  8:32       ` Daniel Lezcano
@ 2013-07-21 10:04         ` Alexander Shiyan
  0 siblings, 0 replies; 48+ messages in thread
From: Alexander Shiyan @ 2013-07-21 10:04 UTC (permalink / raw)
  To: linux-arm-kernel

On Sun, 21 Jul 2013 10:32:31 +0200
Daniel Lezcano <daniel.lezcano@linaro.org> wrote:

> On 07/21/2013 06:11 AM, Alexander Shiyan wrote:
> > On Sat, 20 Jul 2013 23:42:11 +0200
> > Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> > 
> >> On 07/18/2013 08:34 PM, Alexander Shiyan wrote:
> >>> 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 <shc_work@mail.ru>
> >>> ---
> >>>  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
> > [...]
> >>> +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."
> 
> 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.

Currently, this is exactly what is done :)
But I really would like to further add a second mode (STDBY),
which allow to use all PM features.
At this time add this mode is too hard, because there are some barriers
for this, which will be resolved when all drivers for this platform will
contains DT-support.

[...]

-- 
Alexander Shiyan <shc_work@mail.ru>

^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH 08/10] ARM: clps711x: Add CLPS711X cpuidle driver
  2013-07-21  4:11     ` Alexander Shiyan
  2013-07-21  8:32       ` Daniel Lezcano
@ 2013-07-22 11:40       ` Russell King - ARM Linux
  1 sibling, 0 replies; 48+ messages in thread
From: Russell King - ARM Linux @ 2013-07-22 11:40 UTC (permalink / raw)
  To: linux-arm-kernel

On Sun, Jul 21, 2013 at 08:11:38AM +0400, Alexander Shiyan wrote:
> > > +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."
> 
> > > +	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.

However, bear in mind that this is not how you insert two nops after
something; the compiler is free to add anything it likes before,
between separate asm() statements, or after them, even if they're
marked volatile.

^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH 05/10] ARM: clps711x: Add CLPS711X clk driver
  2013-07-18 18:34 ` [PATCH 05/10] ARM: clps711x: Add CLPS711X clk driver Alexander Shiyan
@ 2013-08-02 10:25   ` Mark Rutland
  0 siblings, 0 replies; 48+ messages in thread
From: Mark Rutland @ 2013-08-02 10:25 UTC (permalink / raw)
  To: linux-arm-kernel

On Thu, Jul 18, 2013 at 07:34:56PM +0100, Alexander Shiyan wrote:
> 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 <shc_work@mail.ru>
> ---
>  .../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
> 
> diff --git a/Documentation/devicetree/bindings/clock/clps711x-clock.txt b/Documentation/devicetree/bindings/clock/clps711x-clock.txt
> new file mode 100644
> index 0000000..0fb9fff
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/clps711x-clock.txt
> @@ -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
> +	tc2		7
> +	spi		8
> +	uart		9
> +
> +=== Clocksource subsystem ===
> +Required properties:
> +- compatible: Should be "cirrus,clps711x-clocksource"
> +- reg: Address of the internal register set
> +- clocks: phandle to the source clocks TC1 and TC2
> +- interrupts: The interrupt of the TC2 timer

That sounds odd. What's the interrupt generated by?

> +
> +Example:
> +
> +clks: clks at 80000000 {
> +	compatible = "cirrus,clps711x-clk";
> +	reg = <0x80000000 0>;

Zero length?

> +	#clock-cells = <1>;
> +};
> +
> +timer {
> +	compatible = "cirrus,clps711x-clocksource";
> +	reg = <0x80000000 0>;

Zero length again?

> +	clocks = <&clks 6>, <&clks 7>;
> +	clock-names = "tc1", "tc2";
> +	interrupts = <9>;
> +};
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index ba412e0..dfb1e7f 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -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
> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> index 4038c2b..a7e9b73 100644
> --- a/drivers/clk/Makefile
> +++ b/drivers/clk/Makefile
> @@ -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
> diff --git a/drivers/clk/clk-clps711x.c b/drivers/clk/clk-clps711x.c
> new file mode 100644
> index 0000000..662908c
> --- /dev/null
> +++ b/drivers/clk/clk-clps711x.c
> @@ -0,0 +1,150 @@
> +/*
> + *  CLPS711X clk driver
> + *
> + *  Copyright (C) 2013 Alexander Shiyan <shc_work@mail.ru>
> + *
> + * 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;
> +
> +static void __init _clps711x_clk_init(phys_addr_t phys_base)
> +{
> +	const char *tc_parents[] = { "timer_lf", "timer_hf" };
> +	u32 tmp, f_cpu, f_pll, f_bus, f_timh, f_spi;
> +	void __iomem *pllr, *syscon1, *syscon2;
> +
> +	BUG_ON(!request_mem_region(phys_base + CLPS711X_PLLR, SZ_4, NULL));
> +	BUG_ON(!request_mem_region(phys_base + CLPS711X_SYSCON1, SZ_128, NULL));
> +	BUG_ON(!request_mem_region(phys_base + CLPS711X_SYSCON2, SZ_128, NULL));

This is a wider issue than just this driver, but is BUG_ON appropriate
here? Is a pr_warn("Could not initialise [some clocks], it may not be
possible to use the machine") and a return not good enough?

Also, doesn't that leak an allocated resource? Surely there's a better
way to check that the window given to you was big enough (e.g. make this
take a resource or device_node, and check the size on that).

> +
> +	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);
> +	BUG_ON(!syscon1);
> +
> +	syscon2 = ioremap(phys_base + CLPS711X_SYSCON2, SZ_128);
> +	BUG_ON(!syscon2);
> +
> +	tmp = readl(pllr) >> 24;
> +	if (tmp)
> +		f_pll = DIV_ROUND_UP(CLPS711X_OSC_FREQ * tmp, 2);
> +	else
> +		f_pll = 73728000; /* Old CPUs default value */

A comment explaining what the value of ((readl(pllr) >> 24) represents
would be nice.

> +
> +	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);
> +	clps711x_clk->clks[uart] =
> +		clk_register_fixed_factor(NULL, "uart", "bus", 0, 1, 10);
> +
> +	clk_set_parent(clps711x_clk->clks[tc1], clps711x_clk->clks[timer_lf]);
> +	clk_set_parent(clps711x_clk->clks[tc2], clps711x_clk->clks[timer_hf]);
> +
> +	/* Temporary export clocks for non-DT capable drivers */
> +	clk_register_clkdev(clps711x_clk->clks[pll], "pll", NULL);
> +	clk_register_clkdev(clps711x_clk->clks[spi], "spi", NULL);
> +	clk_register_clkdev(clps711x_clk->clks[uart], "uart", NULL);

If you've booted with dt, do those drivers get probed from platform
data, and use these clocks?

I have a concern that if we allow that, we're forced to keep platform
data for those devices to maintain backwards compatibility, as otherwise
we require dt changes later (adding those devices and their clock
properties) to keep those devices functioning, rather than only
requiring dt changes later to add new device support.

> +
> +	pr_debug("CPU: %u Hz, Timer: %u Hz\n", f_cpu, f_timh);
> +}
> +
> +void __init clps711x_clk_init(phys_addr_t phys_base)
> +{

This could take a resource and pass it on...

> +	_clps711x_clk_init(phys_base);
> +
> +	clk_register_clkdev(clps711x_clk->clks[tc1], "tc1", NULL);
> +	clk_register_clkdev(clps711x_clk->clks[tc2], "tc2", NULL);
> +}

Thanks,
Mark.

^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH 06/10] ARM: clps711x: Add CLPS711X clocksource driver
  2013-07-18 18:34 ` [PATCH 06/10] ARM: clps711x: Add CLPS711X clocksource driver Alexander Shiyan
  2013-07-19 14:38   ` Daniel Lezcano
@ 2013-08-02 10:46   ` Mark Rutland
  2013-08-04 12:31     ` Alexander Shiyan
  1 sibling, 1 reply; 48+ messages in thread
From: Mark Rutland @ 2013-08-02 10:46 UTC (permalink / raw)
  To: linux-arm-kernel

On Thu, Jul 18, 2013 at 07:34:57PM +0100, Alexander Shiyan wrote:
> 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 <shc_work@mail.ru>
> ---

[...]

> diff --git a/drivers/clocksource/clps711x-clksrc.c b/drivers/clocksource/clps711x-clksrc.c
> new file mode 100644
> index 0000000..1749b0b
> --- /dev/null
> +++ b/drivers/clocksource/clps711x-clksrc.c
> @@ -0,0 +1,151 @@
> +/*
> + *  CLPS711X clocksource driver
> + *
> + *  Copyright (C) 2013 Alexander Shiyan <shc_work@mail.ru>
> + *
> + * 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 */
> +		break;
> +	}
> +}
> +
> +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)?

> +
> +static irqreturn_t clps711x_timer_interrupt(int irq, void *dev_id)
> +{
> +	clps711x_clockevent.event_handler(&clps711x_clockevent);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static struct irqaction clps711x_timer_irq = {
> +	.name		= "clps711x-timer",
> +	.flags		= IRQF_TIMER | IRQF_IRQPOLL,
> +	.handler	= clps711x_timer_interrupt,
> +};

Is there any reason you need to use irqaction and can't use
{request,free}_irq?

> +
> +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));

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...

I don't think the BUGs are necessary, and more robust failure handling
would be preferable.

> +
> +	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",
> +			      tc1_rate, 300, 16, clocksource_mmio_readw_down);
> +
> +	setup_sched_clock(clps711x_sched_clock_read, 16, tc1_rate);
> +
> +	/* Set Timer prescaler */
> +	writew(DIV_ROUND_CLOSEST(tc2_rate, HZ), tc2d);
> +
> +	clockevents_config_and_register(&clps711x_clockevent, tc2_rate, 0, 0);
> +
> +	BUG_ON(setup_irq(clps711x_clksrc->irq, &clps711x_timer_irq));

I think the last two actions should be reordered, what if upon
registering the timer the clockevents layer decides to program the
timer? You don't have your interrupt set up, yet you'll try to disable
and enable it...

Thanks,
Mark.

^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH 07/10] ARM: clps711x: Add CLPS711X irqchip driver
  2013-07-18 18:34 ` [PATCH 07/10] ARM: clps711x: Add CLPS711X irqchip driver Alexander Shiyan
@ 2013-08-02 10:57   ` Mark Rutland
  2013-08-02 15:55     ` Alexander Shiyan
  0 siblings, 1 reply; 48+ messages in thread
From: Mark Rutland @ 2013-08-02 10:57 UTC (permalink / raw)
  To: linux-arm-kernel

On Thu, Jul 18, 2013 at 07:34:58PM +0100, Alexander Shiyan wrote:
> 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 <shc_work@mail.ru>
> ---
>  .../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
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/cirrus,clps711x-intc.txt b/Documentation/devicetree/bindings/interrupt-controller/cirrus,clps711x-intc.txt
> new file mode 100644
> index 0000000..26f8983
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/cirrus,clps711x-intc.txt
> @@ -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
> +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?

> +
> +Example:
> +
> +intc: interrupt-controller {
> +       compatible = "cirrus,clps711x-intc";
> +       reg = <0x80000000 0>;

Zero length?

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

[...]

> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 5c60cb7..6f0d238 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -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.
> 
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index 1fea003..c94d00a 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -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
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index e65c41a..a80ff5b 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -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
> diff --git a/drivers/irqchip/irq-clps711x.c b/drivers/irqchip/irq-clps711x.c
> new file mode 100644
> index 0000000..819baa9
> --- /dev/null
> +++ b/drivers/irqchip/irq-clps711x.c
> @@ -0,0 +1,277 @@
> +/*
> + *  CLPS711X IRQ driver
> + *
> + *  Copyright (C) 2013 Alexander Shiyan <shc_work@mail.ru>
> + *
> + * 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, },
> +       [13]    = { CLPS711X_FLAG_EN, },
> +       [14]    = { CLPS711X_FLAG_EN, CLPS711X_UMSEOI, },
> +       [15]    = { CLPS711X_FLAG_EN, CLPS711X_SRXEOF, },
> +       [16]    = { CLPS711X_FLAG_EN, CLPS711X_KBDEOI, },
> +       [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?

> +
> +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;
> +       }
> +       if (!(x & 0xc000)) {
> +               x <<= 2;
> +               r -= 2;
> +       }
> +       if (!(x & 0x8000))
> +               r--;
> +
> +       return r;
> +}

Why hand-roll this? There's already a generic fls in bitops.h (see
include/asm-generic/bitops/fls.h).

> +
> +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;
> +
> +       if (clps711x_irqs[hw].flags & CLPS711X_FLAG_FIQ) {
> +               handler = handle_bad_irq;
> +               flags |= IRQF_NOAUTOEN;
> +       } else if (clps711x_irqs[hw].phys_eoi) {
> +               handler = handle_fasteoi_irq;
> +
> +               clps711x_intc->eoi[hw] =
> +                       clps711x_ioremap_one(clps711x_irqs[hw].phys_eoi);
> +               if (IS_ERR_OR_NULL(clps711x_intc->eoi[hw]))
> +                       return PTR_ERR(clps711x_intc->eoi[hw]);

Wrong. What if it's NULL?

> +
> +               /* 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);
> +}
> +
> +#ifdef CONFIG_IRQCHIP
> +static int __init clps711x_intc_init_dt(struct device_node *np,
> +                                       struct device_node *parent)
> +{
> +       struct resource res;
> +
> +       BUG_ON(of_address_to_resource(np, 0, &res));
> +
> +       _clps711x_intc_init(res.start, np);

Why not pass the whole resource?

> +
> +       return 0;
> +}
> +IRQCHIP_DECLARE(clps711x, "cirrus,clps711x-intc", clps711x_intc_init_dt);
> +#endif

Thanks,
Mark.

^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH 08/10] ARM: clps711x: Add CLPS711X cpuidle driver
  2013-07-18 18:34 ` [PATCH 08/10] ARM: clps711x: Add CLPS711X cpuidle driver Alexander Shiyan
  2013-07-20 21:42   ` Daniel Lezcano
@ 2013-08-02 11:10   ` Mark Rutland
  1 sibling, 0 replies; 48+ messages in thread
From: Mark Rutland @ 2013-08-02 11:10 UTC (permalink / raw)
  To: linux-arm-kernel

On Thu, Jul 18, 2013 at 07:34:59PM +0100, Alexander Shiyan wrote:
> 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 <shc_work@mail.ru>
> ---
>  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
> 
> diff --git a/drivers/cpuidle/Kconfig b/drivers/cpuidle/Kconfig
> index 0e2cd5c..c68a0cf 100644
> --- a/drivers/cpuidle/Kconfig
> +++ b/drivers/cpuidle/Kconfig
> @@ -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
> diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile
> index 8767a7b..d07d2cb 100644
> --- a/drivers/cpuidle/Makefile
> +++ b/drivers/cpuidle/Makefile
> @@ -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
> diff --git a/drivers/cpuidle/cpuidle-clps711x.c b/drivers/cpuidle/cpuidle-clps711x.c
> new file mode 100644
> index 0000000..9f0129f
> --- /dev/null
> +++ b/drivers/cpuidle/cpuidle-clps711x.c
> @@ -0,0 +1,80 @@
> +/*
> + *  CLPS711X CPU idle driver
> + *
> + *  Copyright (C) 2013 Alexander Shiyan <shc_work@mail.ru>
> + *
> + * 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>
> +#include <linux/platform_device.h>
> +
> +static void __iomem *clps711x_halt;
> +
> +static int clps711x_cpuidle_halt(struct cpuidle_device *dev,
> +				 struct cpuidle_driver *drv, int index)
> +{
> +	writel(1, clps711x_halt);
> +	asm volatile ("mov r0, r0");
> +	asm volatile ("mov r0, r0");

Huh?

> +
> +	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)
> +{
> +	cpuidle_unregister(&clps711x_idle_driver);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id clps711x_cpuidle_dt_ids[] = {
> +	{ .compatible = "cirrus,clps711x-cpuidle", },
> +	{ }
> +};

I don't like this -- cpuidle is a Linux abstraction, not a real device.
Additionally you haven't provided a binding.

What is the device being poked actually called? Is it a register in a
larger power management unit?

Thanks,
Mark.

> +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 <shc_work@mail.ru>");
> +MODULE_DESCRIPTION("CLPS711X CPU idle driver");
> +MODULE_LICENSE("GPL");
> -- 
> 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
> 

^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH 10/10] ARM: clps711x: Add initial DT support
  2013-07-18 18:35 ` [PATCH 10/10] ARM: clps711x: Add initial DT support Alexander Shiyan
@ 2013-08-02 11:15   ` Mark Rutland
  0 siblings, 0 replies; 48+ messages in thread
From: Mark Rutland @ 2013-08-02 11:15 UTC (permalink / raw)
  To: linux-arm-kernel

On Thu, Jul 18, 2013 at 07:35:01PM +0100, Alexander Shiyan wrote:
> 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 <shc_work@mail.ru>
> ---
>  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
> 
> diff --git a/arch/arm/mach-clps711x/Kconfig b/arch/arm/mach-clps711x/Kconfig
> index bea6295..8ef38c6 100644
> --- a/arch/arm/mach-clps711x/Kconfig
> +++ b/arch/arm/mach-clps711x/Kconfig
> @@ -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
> diff --git a/arch/arm/mach-clps711x/Makefile b/arch/arm/mach-clps711x/Makefile
> index f04151e..77d3a76 100644
> --- a/arch/arm/mach-clps711x/Makefile
> +++ b/arch/arm/mach-clps711x/Makefile
> @@ -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
> diff --git a/arch/arm/mach-clps711x/board-dt.c b/arch/arm/mach-clps711x/board-dt.c
> new file mode 100644
> index 0000000..5b6f3f1
> --- /dev/null
> +++ b/arch/arm/mach-clps711x/board-dt.c
> @@ -0,0 +1,56 @@
> +/*
> + *  CLPS711X Generic DT board
> + *
> + *  Author: Alexander Shiyan <shc_work@mail.ru>, 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>
> +
> +#include <mach/hardware.h>
> +
> +#include "common.h"
> +
> +void __init clps711x_timer_init_dt(void)
> +{
> +	of_clk_init(NULL);
> +	clocksource_of_init();
> +}

Doesn't generic code do that now if you don't provide this callback?

> +
> +static struct of_dev_auxdata clps711x_auxdata_lookup[] __initdata = {
> +	OF_DEV_AUXDATA("cirrus,clps711x-syscon", CLPS711X_PHYS_BASE + SYSCON1,
> +		       "syscon.1", NULL),
> +	OF_DEV_AUXDATA("cirrus,clps711x-syscon", CLPS711X_PHYS_BASE + SYSCON2,
> +		       "syscon.2", NULL),
> +	OF_DEV_AUXDATA("cirrus,clps711x-syscon", CLPS711X_PHYS_BASE + SYSCON3,
> +		       "syscon.3", NULL),
> +	{ }
> +};

Is there any reason these can't sit in the syscon driver? What are they
used for?

> +
> +static void __init clps711x_init_dt(void)
> +{
> +	of_platform_populate(NULL, of_default_bus_match_table,
> +			     clps711x_auxdata_lookup, NULL);
> +}

Depending on the answer to the above, you may not need this.

Thanks,
Mark.

> +
> +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
> -- 
> 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
> 

^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH 07/10] ARM: clps711x: Add CLPS711X irqchip driver
  2013-08-02 10:57   ` Mark Rutland
@ 2013-08-02 15:55     ` Alexander Shiyan
  2013-08-03 19:45       ` Arnd Bergmann
  0 siblings, 1 reply; 48+ messages in thread
From: Alexander Shiyan @ 2013-08-02 15:55 UTC (permalink / raw)
  To: linux-arm-kernel

On Fri, 2 Aug 2013 11:57:54 +0100
Mark Rutland <mark.rutland@arm.com> wrote:

> On Thu, Jul 18, 2013 at 07:34:58PM +0100, Alexander Shiyan wrote:
> > 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 <shc_work@mail.ru>
> > ---
> >  .../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
> > 
> > diff --git a/Documentation/devicetree/bindings/interrupt-controller/cirrus,clps711x-intc.txt b/Documentation/devicetree/bindings/interrupt-controller/cirrus,clps711x-intc.txt
> > new file mode 100644
> > index 0000000..26f8983
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/interrupt-controller/cirrus,clps711x-intc.txt
> > @@ -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
> > +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?

[...]
> > +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, },
> > +       [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?
Thanks.

-- 
Alexander Shiyan <shc_work@mail.ru>

^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH 00/10] ARM: clps711x: Initial DT support / Fixes
  2013-07-18 18:34 [PATCH 00/10] ARM: clps711x: Initial DT support / Fixes Alexander Shiyan
                   ` (9 preceding siblings ...)
  2013-07-18 18:35 ` [PATCH 10/10] ARM: clps711x: Add initial DT support Alexander Shiyan
@ 2013-08-03  3:54 ` Alexander Shiyan
  2013-08-14  6:29   ` Olof Johansson
  2013-08-14  6:31 ` Olof Johansson
  11 siblings, 1 reply; 48+ messages in thread
From: Alexander Shiyan @ 2013-08-03  3:54 UTC (permalink / raw)
  To: linux-arm-kernel

On Thu, 18 Jul 2013 22:34:51 +0400
Alexander Shiyan <shc_work@mail.ru> 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.
> 
> 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.

-- 
Alexander Shiyan <shc_work@mail.ru>

^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH 07/10] ARM: clps711x: Add CLPS711X irqchip driver
  2013-08-02 15:55     ` Alexander Shiyan
@ 2013-08-03 19:45       ` Arnd Bergmann
  2013-08-04  4:50         ` Alexander Shiyan
  2013-08-05 16:36         ` H Hartley Sweeten
  0 siblings, 2 replies; 48+ messages in thread
From: Arnd Bergmann @ 2013-08-03 19:45 UTC (permalink / raw)
  To: linux-arm-kernel

On Friday 02 August 2013, Alexander Shiyan wrote:
> On Fri, 2 Aug 2013 11:57:54 +0100
> Mark Rutland <mark.rutland@arm.com> wrote:
> > On Thu, Jul 18, 2013 at 07:34:58PM +0100, Alexander Shiyan wrote:

> > > +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
> > > +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?
> 
> [...]
> > > +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, },
> > > +       [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?

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.

Unfortunately, the EOI register offset cannot be computed from the
interrupt number. What Mark was getting at here is that it could be
done in a more generic way if you define the interrupt specifier to
take three cells instead of one, and pass the flag and EOI number
along with the interrupt number, e.g.

	serial at abcd0000 {
		reg = <0xabcd0000 10000>;
		interrupts = <12 0 0>, <13 0 0>, <14 1 6>;
	}

Where '1' signifies the use of an EOI register, and '6' is the index of that register,
so you can compute the actual register as (0x600 + i * 0x40).

I don't actually think that would be much of an improvement though, as the controller
can be considered 'complex', so I'm also fine with your approach. Maybe Mark has some
further thoughts on this.

On Thursday 18 July 2013, Alexander Shiyan wrote:
> +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.

	Arnd

^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH 07/10] ARM: clps711x: Add CLPS711X irqchip driver
  2013-08-03 19:45       ` Arnd Bergmann
@ 2013-08-04  4:50         ` Alexander Shiyan
  2013-08-04 20:46           ` Arnd Bergmann
  2013-08-05 16:36         ` H Hartley Sweeten
  1 sibling, 1 reply; 48+ messages in thread
From: Alexander Shiyan @ 2013-08-04  4:50 UTC (permalink / raw)
  To: linux-arm-kernel

On Sat, 3 Aug 2013 21:45:41 +0200
Arnd Bergmann <arnd@arndb.de> wrote:

[...]
> > +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.

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.
Thanks.

-- 
Alexander Shiyan <shc_work@mail.ru>

^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH 06/10] ARM: clps711x: Add CLPS711X clocksource driver
  2013-08-02 10:46   ` Mark Rutland
@ 2013-08-04 12:31     ` Alexander Shiyan
  2013-08-05 17:02       ` Mark Rutland
  0 siblings, 1 reply; 48+ messages in thread
From: Alexander Shiyan @ 2013-08-04 12:31 UTC (permalink / raw)
  To: linux-arm-kernel

On Fri, 2 Aug 2013 11:46:35 +0100
Mark Rutland <mark.rutland@arm.com> wrote:

> On Thu, Jul 18, 2013 at 07:34:57PM +0100, Alexander Shiyan wrote:
> > 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 <shc_work@mail.ru>
> > ---
> 
> [...]

> > +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)?

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

[...]
> > +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));
> 
> 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.

-- 
Alexander Shiyan <shc_work@mail.ru>

^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH 07/10] ARM: clps711x: Add CLPS711X irqchip driver
  2013-08-04  4:50         ` Alexander Shiyan
@ 2013-08-04 20:46           ` Arnd Bergmann
  2013-08-04 20:57             ` Arnd Bergmann
  0 siblings, 1 reply; 48+ messages in thread
From: Arnd Bergmann @ 2013-08-04 20:46 UTC (permalink / raw)
  To: linux-arm-kernel

On Sunday 04 August 2013, Alexander Shiyan wrote:
> On Sat, 3 Aug 2013 21:45:41 +0200
> Arnd Bergmann <arnd@arndb.de> wrote:
> 
> [...]
> > > +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.
> 
> 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.

	Arnd

^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH 07/10] ARM: clps711x: Add CLPS711X irqchip driver
  2013-08-04 20:46           ` Arnd Bergmann
@ 2013-08-04 20:57             ` Arnd Bergmann
  2013-08-05 18:16               ` Alexander Shiyan
  0 siblings, 1 reply; 48+ messages in thread
From: Arnd Bergmann @ 2013-08-04 20:57 UTC (permalink / raw)
  To: linux-arm-kernel

On Sunday 04 August 2013, Arnd Bergmann wrote:
> On Sunday 04 August 2013, Alexander Shiyan wrote:
> > On Sat, 3 Aug 2013 21:45:41 +0200
> > Arnd Bergmann <arnd@arndb.de> wrote:
> > 
> > [...]
> > > > +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.
> > 
> > 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.

	Arnd

^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH 07/10] ARM: clps711x: Add CLPS711X irqchip driver
  2013-08-03 19:45       ` Arnd Bergmann
  2013-08-04  4:50         ` Alexander Shiyan
@ 2013-08-05 16:36         ` H Hartley Sweeten
  1 sibling, 0 replies; 48+ messages in thread
From: H Hartley Sweeten @ 2013-08-05 16:36 UTC (permalink / raw)
  To: linux-arm-kernel

On Saturday, August 03, 2013 12:46 PM, Arnd Bergmann wrote:
> On Friday 02 August 2013, Alexander Shiyan wrote:
>> On Fri, 2 Aug 2013 11:57:54 +0100
>> Mark Rutland <mark.rutland@arm.com> wrote:
>>> On Thu, Jul 18, 2013 at 07:34:58PM +0100, Alexander Shiyan wrote:

<snip>

>>> 
>>> 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.

FWIW, the cpls711x and ep93xx are completely different.

I'm not sure, but I think the cpls711x is a Cirrus EP73xx series processor 
(CPU_ARM720T). The EP93xx series processor is a CPU_ARM920T.

Regards,
Hartley

^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH 06/10] ARM: clps711x: Add CLPS711X clocksource driver
  2013-08-04 12:31     ` Alexander Shiyan
@ 2013-08-05 17:02       ` Mark Rutland
  0 siblings, 0 replies; 48+ messages in thread
From: Mark Rutland @ 2013-08-05 17:02 UTC (permalink / raw)
  To: linux-arm-kernel

On Sun, Aug 04, 2013 at 01:31:13PM +0100, Alexander Shiyan wrote:
> On Fri, 2 Aug 2013 11:46:35 +0100
> Mark Rutland <mark.rutland@arm.com> wrote:
> 
> > On Thu, Jul 18, 2013 at 07:34:57PM +0100, Alexander Shiyan wrote:
> > > 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 <shc_work@mail.ru>
> > > ---
> > 
> > [...]
> 
> > > +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)?
> 
> This feature is marked as "x86(64) specific" in the header.

While the misfeature was at one point x86-specific, it's rather generic
now.

If your device doesn't stop when the CPU is put into a low power state,
then that's far better and there's nothing to worry about. I just
thought I'd check now to save any future time spent debugging odd stalls
:)

> 
> [...]
> > > +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));
> > 
> > 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.

Ok. So there clocks aren't likely to be used in futures SoCs with other
clocks?

Thanks,
Mark.

^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH 07/10] ARM: clps711x: Add CLPS711X irqchip driver
  2013-08-04 20:57             ` Arnd Bergmann
@ 2013-08-05 18:16               ` Alexander Shiyan
  2013-08-05 19:26                 ` Arnd Bergmann
  0 siblings, 1 reply; 48+ messages in thread
From: Alexander Shiyan @ 2013-08-05 18:16 UTC (permalink / raw)
  To: linux-arm-kernel

On Sun, 4 Aug 2013 22:57:58 +0200
Arnd Bergmann <arnd@arndb.de> wrote:

> On Sunday 04 August 2013, Arnd Bergmann wrote:
> > On Sunday 04 August 2013, Alexander Shiyan wrote:
> > > On Sat, 3 Aug 2013 21:45:41 +0200
> > > Arnd Bergmann <arnd@arndb.de> wrote:
> > > 
> > > [...]
> > > > > +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.
> > > 
> > > 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.

Unfortunately, no.

> 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.

-- 
Alexander Shiyan <shc_work@mail.ru>

^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH 07/10] ARM: clps711x: Add CLPS711X irqchip driver
  2013-08-05 18:16               ` Alexander Shiyan
@ 2013-08-05 19:26                 ` Arnd Bergmann
  2013-08-05 20:42                   ` Alexander Shiyan
  0 siblings, 1 reply; 48+ messages in thread
From: Arnd Bergmann @ 2013-08-05 19:26 UTC (permalink / raw)
  To: linux-arm-kernel

On Monday 05 August 2013, Alexander Shiyan wrote:
> On Sun, 4 Aug 2013 22:57:58 +0200
> Arnd Bergmann <arnd@arndb.de> 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.

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?

	Arnd

^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH 07/10] ARM: clps711x: Add CLPS711X irqchip driver
  2013-08-05 19:26                 ` Arnd Bergmann
@ 2013-08-05 20:42                   ` Alexander Shiyan
  2013-08-05 21:00                     ` Arnd Bergmann
  0 siblings, 1 reply; 48+ messages in thread
From: Alexander Shiyan @ 2013-08-05 20:42 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, 5 Aug 2013 21:26:07 +0200
Arnd Bergmann <arnd@arndb.de> wrote:

> On Monday 05 August 2013, Alexander Shiyan 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.
> 
> 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?

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?

-- 
Alexander Shiyan <shc_work@mail.ru>

^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH 07/10] ARM: clps711x: Add CLPS711X irqchip driver
  2013-08-05 20:42                   ` Alexander Shiyan
@ 2013-08-05 21:00                     ` Arnd Bergmann
  2013-08-06 15:25                       ` Alexander Shiyan
  0 siblings, 1 reply; 48+ messages in thread
From: Arnd Bergmann @ 2013-08-05 21:00 UTC (permalink / raw)
  To: linux-arm-kernel

On Monday 05 August 2013, Alexander Shiyan wrote:
> On Mon, 5 Aug 2013 21:26:07 +0200
> Arnd Bergmann <arnd@arndb.de> wrote:
> 
> > On Monday 05 August 2013, Alexander Shiyan 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.
> > 
> > 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?
> 
> 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 

> 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.

	Arnd

^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH 07/10] ARM: clps711x: Add CLPS711X irqchip driver
  2013-08-05 21:00                     ` Arnd Bergmann
@ 2013-08-06 15:25                       ` Alexander Shiyan
  0 siblings, 0 replies; 48+ messages in thread
From: Alexander Shiyan @ 2013-08-06 15:25 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, 5 Aug 2013 23:00:22 +0200
Arnd Bergmann <arnd@arndb.de> wrote:

> On Monday 05 August 2013, Alexander Shiyan wrote:
> > On Mon, 5 Aug 2013 21:26:07 +0200
> > Arnd Bergmann <arnd@arndb.de> wrote:
> > 
> > > On Monday 05 August 2013, Alexander Shiyan 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.
> > > 
> > > 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?
> > 
> > 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.

No, I just keep in mind that any access to any register in regmap (SYSCON)
will block access to the registers of the other modules, and it will increase
the interrupt latency.

> 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 
> 
> > 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.

What can you say about the removal "reg" property from the bindings and hard
define the physical address in the driver?

In any case, I'll make a second version of the patch, and since a lot of
comments, I will divide each functional part in the individual patches.
So I want to ask again, please apply "fixes" part of the series (1/10-4/10).
Thanks.

-- 
Alexander Shiyan <shc_work@mail.ru>

^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH 00/10] ARM: clps711x: Initial DT support / Fixes
  2013-08-03  3:54 ` [PATCH 00/10] ARM: clps711x: Initial DT support / Fixes Alexander Shiyan
@ 2013-08-14  6:29   ` Olof Johansson
  2013-08-17  4:32     ` Alexander Shiyan
  0 siblings, 1 reply; 48+ messages in thread
From: Olof Johansson @ 2013-08-14  6:29 UTC (permalink / raw)
  To: linux-arm-kernel

On Sat, Aug 03, 2013 at 07:54:42AM +0400, Alexander Shiyan wrote:
> On Thu, 18 Jul 2013 22:34:51 +0400
> Alexander Shiyan <shc_work@mail.ru> 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.
> > 
> > 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.

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.


-Olof

^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH 01/10] ARM: clps711x: Remove the special name for the syscon driver
  2013-07-18 18:34 ` [PATCH 01/10] ARM: clps711x: Remove the special name for the syscon driver Alexander Shiyan
@ 2013-08-14  6:29   ` Olof Johansson
  2013-08-14  6:29   ` Olof Johansson
  1 sibling, 0 replies; 48+ messages in thread
From: Olof Johansson @ 2013-08-14  6:29 UTC (permalink / raw)
  To: linux-arm-kernel

On Thu, Jul 18, 2013 at 10:34:52PM +0400, Alexander Shiyan wrote:
> 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 <shc_work@mail.ru>

Applied, thanks.

-Olof

^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH 01/10] ARM: clps711x: Remove the special name for the syscon driver
  2013-07-18 18:34 ` [PATCH 01/10] ARM: clps711x: Remove the special name for the syscon driver Alexander Shiyan
  2013-08-14  6:29   ` Olof Johansson
@ 2013-08-14  6:29   ` Olof Johansson
  1 sibling, 0 replies; 48+ messages in thread
From: Olof Johansson @ 2013-08-14  6:29 UTC (permalink / raw)
  To: linux-arm-kernel

On Thu, Jul 18, 2013 at 10:34:52PM +0400, Alexander Shiyan wrote:
> 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.

Applied, thanks.


-Olof

^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH 02/10] ARM: clps711x: Drop fortunet board support
  2013-07-18 18:34 ` [PATCH 02/10] ARM: clps711x: Drop fortunet board support Alexander Shiyan
@ 2013-08-14  6:29   ` Olof Johansson
  0 siblings, 0 replies; 48+ messages in thread
From: Olof Johansson @ 2013-08-14  6:29 UTC (permalink / raw)
  To: linux-arm-kernel

On Thu, Jul 18, 2013 at 10:34:53PM +0400, Alexander Shiyan wrote:
> 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 <shc_work@mail.ru>

Applied, thanks.

-Olof

^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH 03/10] ARM: clps711x: autcpu12: Remove incorrect config checking
  2013-07-18 18:34 ` [PATCH 03/10] ARM: clps711x: autcpu12: Remove incorrect config checking Alexander Shiyan
@ 2013-08-14  6:30   ` Olof Johansson
  0 siblings, 0 replies; 48+ messages in thread
From: Olof Johansson @ 2013-08-14  6:30 UTC (permalink / raw)
  To: linux-arm-kernel

On Thu, Jul 18, 2013 at 10:34:54PM +0400, Alexander Shiyan wrote:
> 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 <shc_work@mail.ru>

Applied, thanks.


-Olof

^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH 04/10] ARM: clps711x: edb7211: Remove extra iotable_init() call
  2013-07-18 18:34 ` [PATCH 04/10] ARM: clps711x: edb7211: Remove extra iotable_init() call Alexander Shiyan
@ 2013-08-14  6:30   ` Olof Johansson
  0 siblings, 0 replies; 48+ messages in thread
From: Olof Johansson @ 2013-08-14  6:30 UTC (permalink / raw)
  To: linux-arm-kernel

On Thu, Jul 18, 2013 at 10:34:55PM +0400, Alexander Shiyan wrote:
> 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 <shc_work@mail.ru>


Applied, thanks.


-Olof

^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH 00/10] ARM: clps711x: Initial DT support / Fixes
  2013-07-18 18:34 [PATCH 00/10] ARM: clps711x: Initial DT support / Fixes Alexander Shiyan
                   ` (10 preceding siblings ...)
  2013-08-03  3:54 ` [PATCH 00/10] ARM: clps711x: Initial DT support / Fixes Alexander Shiyan
@ 2013-08-14  6:31 ` Olof Johansson
  11 siblings, 0 replies; 48+ messages in thread
From: Olof Johansson @ 2013-08-14  6:31 UTC (permalink / raw)
  To: linux-arm-kernel

On Thu, Jul 18, 2013 at 10:34:51PM +0400, Alexander Shiyan 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.

Actually, an initial fragment DTS/DTSI that just adds a probe for the
machine compatible (combined with DT_MACHINE.* in the platform code)
is a great start, so don't hold back on adding DTS files/contents for
that reason.


-Olof

^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH 00/10] ARM: clps711x: Initial DT support / Fixes
  2013-08-14  6:29   ` Olof Johansson
@ 2013-08-17  4:32     ` Alexander Shiyan
  2013-08-22  5:00       ` Olof Johansson
  0 siblings, 1 reply; 48+ messages in thread
From: Alexander Shiyan @ 2013-08-17  4:32 UTC (permalink / raw)
  To: linux-arm-kernel

On Tue, 13 Aug 2013 23:29:00 -0700
Olof Johansson <olof@lixom.net> wrote:

> On Sat, Aug 03, 2013 at 07:54:42AM +0400, Alexander Shiyan wrote:
> > On Thu, 18 Jul 2013 22:34:51 +0400
> > Alexander Shiyan <shc_work@mail.ru> 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.
> > > 
> > > 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.
> 
> 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.

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

-- 
Alexander Shiyan <shc_work@mail.ru>

^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH 00/10] ARM: clps711x: Initial DT support / Fixes
  2013-08-17  4:32     ` Alexander Shiyan
@ 2013-08-22  5:00       ` Olof Johansson
  0 siblings, 0 replies; 48+ messages in thread
From: Olof Johansson @ 2013-08-22  5:00 UTC (permalink / raw)
  To: linux-arm-kernel

On Sat, Aug 17, 2013 at 08:32:30AM +0400, Alexander Shiyan wrote:
> On Tue, 13 Aug 2013 23:29:00 -0700
> Olof Johansson <olof@lixom.net> wrote:
> 
> > On Sat, Aug 03, 2013 at 07:54:42AM +0400, Alexander Shiyan wrote:
> > > On Thu, 18 Jul 2013 22:34:51 +0400
> > > Alexander Shiyan <shc_work@mail.ru> 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.
> > > > 
> > > > 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.
> > 
> > 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.
> 
> Do you mean always create CC to "arm at ..." for parts [0/xx] or after review period?
> Thnaks.

Hi,

arm at kernel.org for when you want them applied. I try to avoid getting
too many patches posted there just for review, it's the alias we try
to keep mostly for platform maintainers to email us pull requests and
patches to apply to.

You can cc us on our individual addresses if you want for the reviews. We might
not always respond in a timely manner depending on how backlogged we are on
email, so remind us if needed.


-Olof

^ permalink raw reply	[flat|nested] 48+ messages in thread

end of thread, other threads:[~2013-08-22  5:00 UTC | newest]

Thread overview: 48+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-07-18 18:34 [PATCH 00/10] ARM: clps711x: Initial DT support / Fixes Alexander Shiyan
2013-07-18 18:34 ` [PATCH 01/10] ARM: clps711x: Remove the special name for the syscon driver Alexander Shiyan
2013-08-14  6:29   ` Olof Johansson
2013-08-14  6:29   ` Olof Johansson
2013-07-18 18:34 ` [PATCH 02/10] ARM: clps711x: Drop fortunet board support Alexander Shiyan
2013-08-14  6:29   ` Olof Johansson
2013-07-18 18:34 ` [PATCH 03/10] ARM: clps711x: autcpu12: Remove incorrect config checking Alexander Shiyan
2013-08-14  6:30   ` Olof Johansson
2013-07-18 18:34 ` [PATCH 04/10] ARM: clps711x: edb7211: Remove extra iotable_init() call Alexander Shiyan
2013-08-14  6:30   ` Olof Johansson
2013-07-18 18:34 ` [PATCH 05/10] ARM: clps711x: Add CLPS711X clk driver Alexander Shiyan
2013-08-02 10:25   ` Mark Rutland
2013-07-18 18:34 ` [PATCH 06/10] ARM: clps711x: Add CLPS711X clocksource driver Alexander Shiyan
2013-07-19 14:38   ` Daniel Lezcano
2013-07-20  3:44     ` Alexander Shiyan
2013-07-20 21:48       ` Daniel Lezcano
2013-07-21  4:30         ` Alexander Shiyan
2013-08-02 10:46   ` Mark Rutland
2013-08-04 12:31     ` Alexander Shiyan
2013-08-05 17:02       ` Mark Rutland
2013-07-18 18:34 ` [PATCH 07/10] ARM: clps711x: Add CLPS711X irqchip driver Alexander Shiyan
2013-08-02 10:57   ` Mark Rutland
2013-08-02 15:55     ` Alexander Shiyan
2013-08-03 19:45       ` Arnd Bergmann
2013-08-04  4:50         ` Alexander Shiyan
2013-08-04 20:46           ` Arnd Bergmann
2013-08-04 20:57             ` Arnd Bergmann
2013-08-05 18:16               ` Alexander Shiyan
2013-08-05 19:26                 ` Arnd Bergmann
2013-08-05 20:42                   ` Alexander Shiyan
2013-08-05 21:00                     ` Arnd Bergmann
2013-08-06 15:25                       ` Alexander Shiyan
2013-08-05 16:36         ` H Hartley Sweeten
2013-07-18 18:34 ` [PATCH 08/10] ARM: clps711x: Add CLPS711X cpuidle driver Alexander Shiyan
2013-07-20 21:42   ` Daniel Lezcano
2013-07-21  4:11     ` Alexander Shiyan
2013-07-21  8:32       ` Daniel Lezcano
2013-07-21 10:04         ` Alexander Shiyan
2013-07-22 11:40       ` Russell King - ARM Linux
2013-08-02 11:10   ` Mark Rutland
2013-07-18 18:35 ` [PATCH 09/10] ARM: clps711x: Migrate CLPS711X subarch to the new basic drivers Alexander Shiyan
2013-07-18 18:35 ` [PATCH 10/10] ARM: clps711x: Add initial DT support Alexander Shiyan
2013-08-02 11:15   ` Mark Rutland
2013-08-03  3:54 ` [PATCH 00/10] ARM: clps711x: Initial DT support / Fixes Alexander Shiyan
2013-08-14  6:29   ` Olof Johansson
2013-08-17  4:32     ` Alexander Shiyan
2013-08-22  5:00       ` Olof Johansson
2013-08-14  6:31 ` Olof Johansson

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.