[PATCH 1/3] clocksource: sh_cmt: Add Device Tree probing

[PATCH 1/3] clocksource: sh_cmt: Add Device Tree probing

[Bastian Hecht]

We add the capabilty to probe SH CMT timer devices using Device Tree
setup.

We can deduce former platform data by the device IDs and channel
IDs of our timer instances, so we choose this more intuitive info as our
DT properties.

Signed-off-by: Bastian Hecht <hechtb+renesas@gmail.com>
---
 .../bindings/timer/renesas,sh-cmt-timer.txt        |   21 +++++
 drivers/clocksource/sh_cmt.c                       |   94 +++++++++++++++++---
 2 files changed, 102 insertions(+), 13 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/timer/renesas,sh-cmt-timer.txt

diff --git a/Documentation/devicetree/bindings/timer/renesas,sh-cmt-timer.txt b/Documentation/devicetree/bindings/timer/renesas,sh-cmt-timer.txt
new file mode 100644
index 0000000..e5ad808
--- /dev/null
+++ b/Documentation/devicetree/bindings/timer/renesas,sh-cmt-timer.txt
@@ -0,0 +1,21 @@
+* Renesas SH Mobile Compare Match Timer
+
+Required properties:
+- compatible : Should be "renesas,cmt"
+- reg : Address and length of the register set for the device
+- interrupts : Timer IRQ
+- renesas,timer-device-id : The ID of the timer device
+- renesas,timer-channel-id : The channel ID of the timer device
+
+Example for CMT10 of the R8A7740 SoC:
+
+	cmt@0xe6138010 {
+		compatible = "renesas,cmt";
+		interrupt-parent = <&intca>;
+		reg = <0xe6138010 0xc>;
+		interrupts = <0x0b00>;
+		renesas,timer-device-id = <1>;
+		renesas,timer-channel-id = <0>;
+		renesas,clocksource-rating = <150>;
+		renesas,clockevent-rating = <150>;
+	};
diff --git a/drivers/clocksource/sh_cmt.c b/drivers/clocksource/sh_cmt.c
index b72b724..9a7a7d4 100644
--- a/drivers/clocksource/sh_cmt.c
+++ b/drivers/clocksource/sh_cmt.c
@@ -38,6 +38,8 @@
 struct sh_cmt_priv {
 	void __iomem *mapbase;
 	struct clk *clk;
+	long channel_offset;
+	int timer_bit;
 	unsigned long width; /* 16 or 32 bit version of hardware block */
 	unsigned long overflow_bit;
 	unsigned long clear_bits;
@@ -109,9 +111,7 @@ static void sh_cmt_write32(void __iomem *base, unsigned long offs,
 
 static inline unsigned long sh_cmt_read_cmstr(struct sh_cmt_priv *p)
 {
-	struct sh_timer_config *cfg = p->pdev->dev.platform_data;
-
-	return p->read_control(p->mapbase - cfg->channel_offset, 0);
+	return p->read_control(p->mapbase - p->channel_offset, 0);
 }
 
 static inline unsigned long sh_cmt_read_cmcsr(struct sh_cmt_priv *p)
@@ -127,9 +127,7 @@ static inline unsigned long sh_cmt_read_cmcnt(struct sh_cmt_priv *p)
 static inline void sh_cmt_write_cmstr(struct sh_cmt_priv *p,
 				      unsigned long value)
 {
-	struct sh_timer_config *cfg = p->pdev->dev.platform_data;
-
-	p->write_control(p->mapbase - cfg->channel_offset, 0, value);
+	p->write_control(p->mapbase - p->channel_offset, 0, value);
 }
 
 static inline void sh_cmt_write_cmcsr(struct sh_cmt_priv *p,
@@ -176,7 +174,6 @@ static DEFINE_RAW_SPINLOCK(sh_cmt_lock);
 
 static void sh_cmt_start_stop_ch(struct sh_cmt_priv *p, int start)
 {
-	struct sh_timer_config *cfg = p->pdev->dev.platform_data;
 	unsigned long flags, value;
 
 	/* start stop register shared by multiple timer channels */
@@ -184,9 +181,9 @@ static void sh_cmt_start_stop_ch(struct sh_cmt_priv *p, int start)
 	value = sh_cmt_read_cmstr(p);
 
 	if (start)
-		value |= 1 << cfg->timer_bit;
+		value |= 1 << p->timer_bit;
 	else
-		value &= ~(1 << cfg->timer_bit);
+		value &= ~(1 << p->timer_bit);
 
 	sh_cmt_write_cmstr(p, value);
 	raw_spin_unlock_irqrestore(&sh_cmt_lock, flags);
@@ -673,9 +670,71 @@ static int sh_cmt_register(struct sh_cmt_priv *p, char *name,
 	return 0;
 }
 
-static int sh_cmt_setup(struct sh_cmt_priv *p, struct platform_device *pdev)
+#ifdef CONFIG_OF
+static const struct of_device_id of_sh_cmt_match[] = {
+	{ .compatible = "renesas,cmt" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, of_sh_cmt_match);
+
+static const int sh_timer_offset_multiplier[] = { 0x60, 0x10, 0x40, 0x40 };
+
+static struct sh_timer_config *sh_cmt_parse_dt(struct device *dev)
+{
+	struct sh_timer_config *cfg;
+	struct device_node *np = dev->of_node;
+	const __be32 *prop;
+	int timer_id, channel_id;
+
+	cfg = devm_kzalloc(dev, sizeof(struct sh_timer_config), GFP_KERNEL);
+	if (!cfg) {
+		dev_err(dev, "failed to allocate DT config data\n");
+		return NULL;
+	}
+
+	prop = of_get_property(np, "renesas,timer-device-id", NULL);
+	if (!prop) {
+		dev_err(dev, "device id missing\n");
+		return NULL;
+	}
+	timer_id = be32_to_cpup(prop);
+
+	prop = of_get_property(np, "renesas,timer-channel-id", NULL);
+	if (!prop) {
+		dev_err(dev, "channel id missing\n");
+		return NULL;
+	}
+	channel_id = be32_to_cpup(prop);
+
+	if (timer_id >= ARRAY_SIZE(sh_timer_offset_multiplier)) {
+		dev_err(dev, "unsupported timer id\n");
+		return NULL;
+	}
+
+	cfg->channel_offset = sh_timer_offset_multiplier[timer_id] *
+							(channel_id + 1);
+	cfg->timer_bit = channel_id;
+
+	prop = of_get_property(np, "renesas,clocksource-rating", NULL);
+	if (prop)
+		cfg->clocksource_rating = be32_to_cpup(prop);
+
+	prop = of_get_property(np, "renesas,clockevent-rating", NULL);
+	if (prop)
+		cfg->clockevent_rating = be32_to_cpup(prop);
+
+	return cfg;
+}
+#else
+static struct sh_timer_config *sh_cmt_parse_dt(struct device *dev)
+{
+	return NULL;
+}
+#endif /* CONFIG_OF */
+
+static int sh_cmt_setup(struct sh_cmt_priv *p, struct platform_device *pdev,
+						struct sh_timer_config *cfg)
 {
-	struct sh_timer_config *cfg = pdev->dev.platform_data;
 	struct resource *res;
 	int irq, ret;
 	ret = -ENXIO;
@@ -762,6 +821,9 @@ static int sh_cmt_setup(struct sh_cmt_priv *p, struct platform_device *pdev)
 		goto err2;
 	}
 
+	p->channel_offset = cfg->channel_offset;
+	p->timer_bit = cfg->timer_bit;
+
 	platform_set_drvdata(pdev, p);
 
 	return 0;
@@ -777,7 +839,7 @@ err0:
 static int sh_cmt_probe(struct platform_device *pdev)
 {
 	struct sh_cmt_priv *p = platform_get_drvdata(pdev);
-	struct sh_timer_config *cfg = pdev->dev.platform_data;
+	struct sh_timer_config *cfg;
 	int ret;
 
 	if (!is_early_platform_device(pdev)) {
@@ -785,6 +847,11 @@ static int sh_cmt_probe(struct platform_device *pdev)
 		pm_runtime_enable(&pdev->dev);
 	}
 
+	if (pdev->dev.of_node)
+		cfg = sh_cmt_parse_dt(&pdev->dev);
+	else
+		cfg = pdev->dev.platform_data;
+
 	if (p) {
 		dev_info(&pdev->dev, "kept as earlytimer\n");
 		goto out;
@@ -796,7 +863,7 @@ static int sh_cmt_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	}
 
-	ret = sh_cmt_setup(p, pdev);
+	ret = sh_cmt_setup(p, pdev, cfg);
 	if (ret) {
 		kfree(p);
 		pm_runtime_idle(&pdev->dev);
@@ -824,6 +891,7 @@ static struct platform_driver sh_cmt_device_driver = {
 	.remove		= sh_cmt_remove,
 	.driver		= {
 		.name	= "sh_cmt",
+		.of_match_table = of_match_ptr(of_sh_cmt_match),
 	}
 };
 
-- 
1.7.9.5

[PATCH 3/3] ARM: mach-shmobile: sh73a0: Setup the timer device CMT10 using DT

[Bastian Hecht]

We can now use the Device Tree for bringing up our timer device CMT10.
We move it out of the DT devices list into the early_devices list by
the non-reference board code. And we add the device to the
kzm9g-reference .dts file.

Not-yet-signed-off-by: Bastian Hecht <hechtb+renesas@gmail.com>
---
I wonder if this should go to arch/arm/boot/dts/sh73a0.dtsi.

The moving to the early_devices is a bit hackish - I will cook up a better
patch when I know into which .dts(i) file things go.

 arch/arm/boot/dts/sh73a0-kzm9g-reference.dts |   11 +++++++++++
 arch/arm/mach-shmobile/setup-sh73a0.c        |    2 +-
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/sh73a0-kzm9g-reference.dts b/arch/arm/boot/dts/sh73a0-kzm9g-reference.dts
index 7fad4b9..fef12f0 100644
--- a/arch/arm/boot/dts/sh73a0-kzm9g-reference.dts
+++ b/arch/arm/boot/dts/sh73a0-kzm9g-reference.dts
@@ -69,6 +69,17 @@
 		toshiba,mmc-wrprotect-disable;
 		toshiba,mmc-has-idle-wait;
 	};
+
+	cmt@0xe6138010 {
+		compatible = "renesas,cmt";
+		reg = <0xe6138010 0xc>;
+		interrupt-parent = <&gic>;
+		interrupts = <0 65 0x4>;
+		renesas,timer-device-id = <1>;
+		renesas,timer-channel-id = <0>;
+		renesas,clocksource-rating = <125>;
+		renesas,clockevent-rating = <125>;
+	};
 };
 
 &mmcif {
diff --git a/arch/arm/mach-shmobile/setup-sh73a0.c b/arch/arm/mach-shmobile/setup-sh73a0.c
index 6259e07..b7fdec0 100644
--- a/arch/arm/mach-shmobile/setup-sh73a0.c
+++ b/arch/arm/mach-shmobile/setup-sh73a0.c
@@ -913,10 +913,10 @@ static struct platform_device *sh73a0_devices_dt[] __initdata = {
 	&scif6_device,
 	&scif7_device,
 	&scif8_device,
-	&cmt10_device,
 };
 
 static struct platform_device *sh73a0_early_devices[] __initdata = {
+	&cmt10_device,
 	&tmu00_device,
 	&tmu01_device,
 };
-- 
1.7.9.5

[PATCH 2/3] ARM: mach-shmobile: sh73a0: Add CMT DT name to clock list

[Bastian Hecht]

This adds temporarily the alternative device name to the clock list
that is used when booting via Device Tree setup.

Signed-off-by: Bastian Hecht <hechtb+renesas@gmail.com>
---
 arch/arm/mach-shmobile/clock-sh73a0.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/mach-shmobile/clock-sh73a0.c b/arch/arm/mach-shmobile/clock-sh73a0.c
index 71843dd..4aed113 100644
--- a/arch/arm/mach-shmobile/clock-sh73a0.c
+++ b/arch/arm/mach-shmobile/clock-sh73a0.c
@@ -575,6 +575,7 @@ static struct clk_lookup lookups[] = {
 	CLKDEV_DEV_ID("sh-sci.4", &mstp_clks[MSTP200]), /* SCIFA4 */
 	CLKDEV_DEV_ID("sh-sci.6", &mstp_clks[MSTP331]), /* SCIFA6 */
 	CLKDEV_DEV_ID("sh_cmt.10", &mstp_clks[MSTP329]), /* CMT10 */
+	CLKDEV_DEV_ID("e6138010.cmt", &mstp_clks[MSTP329]), /* CMT10 */
 	CLKDEV_DEV_ID("sh_fsi2", &mstp_clks[MSTP328]), /* FSI */
 	CLKDEV_DEV_ID("sh_irda.0", &mstp_clks[MSTP325]), /* IrDA */
 	CLKDEV_DEV_ID("i2c-sh_mobile.1", &mstp_clks[MSTP323]), /* I2C1 */
-- 
1.7.9.5

[PATCH 0/3] Add OF support to CMT driver - bindings discussion

[Bastian Hecht]

Hello,

here in the CMT driver we currently can derive all neccessary hardware info from the device ID and channel ID (there are multiple CMT instances built into one SoC with multiple channels each). This seems like a very intuituve description to me.

If multiple register layouts appear we can add an (optional) regtype property and some parts of the driver need to be rewritten anyway. But the descriptiveness of device ID and channel ID should stay I think.

Will this work?

In the patchset I have added the dts specification to sh73a0-kzm9g-reference.dts to test it. Does this belong to sh73a0.dtsi instead? I wonder because the clock is in the setup code, not board code.

Thanks,

 Bastian


Bastian Hecht (3):
  clocksource: sh_cmt: Add Device Tree probing
  ARM: mach-shmobile: sh73a0: Add CMT DT name to clock list
  ARM: mach-shmobile: sh73a0: Setup the timer device CMT10 using DT

 arch/arm/boot/dts/sh73a0-kzm9g-reference.dts |   11 ++++
 arch/arm/mach-shmobile/clock-sh73a0.c        |    1 +
 arch/arm/mach-shmobile/setup-sh73a0.c        |    1 -
 drivers/clocksource/sh_cmt.c                 |   87 ++++++++++++++++++++++----
 4 files changed, 86 insertions(+), 14 deletions(-)

-- 
1.7.9.5

[PATCH 0/3] Add OF support to CMT driver - bindings discussion

[Bastian Hecht]

Hello,

here in the CMT driver we currently can derive all neccessary hardware info from the device ID and channel ID (there are multiple CMT instances built into one SoC with multiple channels each). This seems like a very intuituve description to me.

If multiple register layouts appear we can add an (optional) regtype property and some parts of the driver need to be rewritten anyway. But the descriptiveness of device ID and channel ID should stay I think.

Will this work?

In the patchset I have added the dts specification to sh73a0-kzm9g-reference.dts to test it. Does this belong to sh73a0.dtsi instead? I wonder because the clock is in the setup code, not board code.

Thanks,

 Bastian


Bastian Hecht (3):
  clocksource: sh_cmt: Add Device Tree probing
  ARM: mach-shmobile: sh73a0: Add CMT DT name to clock list
  ARM: mach-shmobile: sh73a0: Setup the timer device CMT10 using DT

 arch/arm/boot/dts/sh73a0-kzm9g-reference.dts |   11 ++++
 arch/arm/mach-shmobile/clock-sh73a0.c        |    1 +
 arch/arm/mach-shmobile/setup-sh73a0.c        |    1 -
 drivers/clocksource/sh_cmt.c                 |   87 ++++++++++++++++++++++----
 4 files changed, 86 insertions(+), 14 deletions(-)

-- 
1.7.9.5

[PATCH 1/3] clocksource: sh_cmt: Add Device Tree probing

[Bastian Hecht]

We add the capabilty to probe SH CMT timer devices using Device Tree
setup.

We can deduce former platform data by the device IDs and channel
IDs of our timer instances, so we choose this more intuitive info as our
DT properties.

Signed-off-by: Bastian Hecht <hechtb+renesas@gmail.com>
---
 .../bindings/timer/renesas,sh-cmt-timer.txt        |   21 +++++
 drivers/clocksource/sh_cmt.c                       |   94 +++++++++++++++++---
 2 files changed, 102 insertions(+), 13 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/timer/renesas,sh-cmt-timer.txt

diff --git a/Documentation/devicetree/bindings/timer/renesas,sh-cmt-timer.txt b/Documentation/devicetree/bindings/timer/renesas,sh-cmt-timer.txt
new file mode 100644
index 0000000..e5ad808
--- /dev/null
+++ b/Documentation/devicetree/bindings/timer/renesas,sh-cmt-timer.txt
@@ -0,0 +1,21 @@
+* Renesas SH Mobile Compare Match Timer
+
+Required properties:
+- compatible : Should be "renesas,cmt"
+- reg : Address and length of the register set for the device
+- interrupts : Timer IRQ
+- renesas,timer-device-id : The ID of the timer device
+- renesas,timer-channel-id : The channel ID of the timer device
+
+Example for CMT10 of the R8A7740 SoC:
+
+	cmt at 0xe6138010 {
+		compatible = "renesas,cmt";
+		interrupt-parent = <&intca>;
+		reg = <0xe6138010 0xc>;
+		interrupts = <0x0b00>;
+		renesas,timer-device-id = <1>;
+		renesas,timer-channel-id = <0>;
+		renesas,clocksource-rating = <150>;
+		renesas,clockevent-rating = <150>;
+	};
diff --git a/drivers/clocksource/sh_cmt.c b/drivers/clocksource/sh_cmt.c
index b72b724..9a7a7d4 100644
--- a/drivers/clocksource/sh_cmt.c
+++ b/drivers/clocksource/sh_cmt.c
@@ -38,6 +38,8 @@
 struct sh_cmt_priv {
 	void __iomem *mapbase;
 	struct clk *clk;
+	long channel_offset;
+	int timer_bit;
 	unsigned long width; /* 16 or 32 bit version of hardware block */
 	unsigned long overflow_bit;
 	unsigned long clear_bits;
@@ -109,9 +111,7 @@ static void sh_cmt_write32(void __iomem *base, unsigned long offs,
 
 static inline unsigned long sh_cmt_read_cmstr(struct sh_cmt_priv *p)
 {
-	struct sh_timer_config *cfg = p->pdev->dev.platform_data;
-
-	return p->read_control(p->mapbase - cfg->channel_offset, 0);
+	return p->read_control(p->mapbase - p->channel_offset, 0);
 }
 
 static inline unsigned long sh_cmt_read_cmcsr(struct sh_cmt_priv *p)
@@ -127,9 +127,7 @@ static inline unsigned long sh_cmt_read_cmcnt(struct sh_cmt_priv *p)
 static inline void sh_cmt_write_cmstr(struct sh_cmt_priv *p,
 				      unsigned long value)
 {
-	struct sh_timer_config *cfg = p->pdev->dev.platform_data;
-
-	p->write_control(p->mapbase - cfg->channel_offset, 0, value);
+	p->write_control(p->mapbase - p->channel_offset, 0, value);
 }
 
 static inline void sh_cmt_write_cmcsr(struct sh_cmt_priv *p,
@@ -176,7 +174,6 @@ static DEFINE_RAW_SPINLOCK(sh_cmt_lock);
 
 static void sh_cmt_start_stop_ch(struct sh_cmt_priv *p, int start)
 {
-	struct sh_timer_config *cfg = p->pdev->dev.platform_data;
 	unsigned long flags, value;
 
 	/* start stop register shared by multiple timer channels */
@@ -184,9 +181,9 @@ static void sh_cmt_start_stop_ch(struct sh_cmt_priv *p, int start)
 	value = sh_cmt_read_cmstr(p);
 
 	if (start)
-		value |= 1 << cfg->timer_bit;
+		value |= 1 << p->timer_bit;
 	else
-		value &= ~(1 << cfg->timer_bit);
+		value &= ~(1 << p->timer_bit);
 
 	sh_cmt_write_cmstr(p, value);
 	raw_spin_unlock_irqrestore(&sh_cmt_lock, flags);
@@ -673,9 +670,71 @@ static int sh_cmt_register(struct sh_cmt_priv *p, char *name,
 	return 0;
 }
 
-static int sh_cmt_setup(struct sh_cmt_priv *p, struct platform_device *pdev)
+#ifdef CONFIG_OF
+static const struct of_device_id of_sh_cmt_match[] = {
+	{ .compatible = "renesas,cmt" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, of_sh_cmt_match);
+
+static const int sh_timer_offset_multiplier[] = { 0x60, 0x10, 0x40, 0x40 };
+
+static struct sh_timer_config *sh_cmt_parse_dt(struct device *dev)
+{
+	struct sh_timer_config *cfg;
+	struct device_node *np = dev->of_node;
+	const __be32 *prop;
+	int timer_id, channel_id;
+
+	cfg = devm_kzalloc(dev, sizeof(struct sh_timer_config), GFP_KERNEL);
+	if (!cfg) {
+		dev_err(dev, "failed to allocate DT config data\n");
+		return NULL;
+	}
+
+	prop = of_get_property(np, "renesas,timer-device-id", NULL);
+	if (!prop) {
+		dev_err(dev, "device id missing\n");
+		return NULL;
+	}
+	timer_id = be32_to_cpup(prop);
+
+	prop = of_get_property(np, "renesas,timer-channel-id", NULL);
+	if (!prop) {
+		dev_err(dev, "channel id missing\n");
+		return NULL;
+	}
+	channel_id = be32_to_cpup(prop);
+
+	if (timer_id >= ARRAY_SIZE(sh_timer_offset_multiplier)) {
+		dev_err(dev, "unsupported timer id\n");
+		return NULL;
+	}
+
+	cfg->channel_offset = sh_timer_offset_multiplier[timer_id] *
+							(channel_id + 1);
+	cfg->timer_bit = channel_id;
+
+	prop = of_get_property(np, "renesas,clocksource-rating", NULL);
+	if (prop)
+		cfg->clocksource_rating = be32_to_cpup(prop);
+
+	prop = of_get_property(np, "renesas,clockevent-rating", NULL);
+	if (prop)
+		cfg->clockevent_rating = be32_to_cpup(prop);
+
+	return cfg;
+}
+#else
+static struct sh_timer_config *sh_cmt_parse_dt(struct device *dev)
+{
+	return NULL;
+}
+#endif /* CONFIG_OF */
+
+static int sh_cmt_setup(struct sh_cmt_priv *p, struct platform_device *pdev,
+						struct sh_timer_config *cfg)
 {
-	struct sh_timer_config *cfg = pdev->dev.platform_data;
 	struct resource *res;
 	int irq, ret;
 	ret = -ENXIO;
@@ -762,6 +821,9 @@ static int sh_cmt_setup(struct sh_cmt_priv *p, struct platform_device *pdev)
 		goto err2;
 	}
 
+	p->channel_offset = cfg->channel_offset;
+	p->timer_bit = cfg->timer_bit;
+
 	platform_set_drvdata(pdev, p);
 
 	return 0;
@@ -777,7 +839,7 @@ err0:
 static int sh_cmt_probe(struct platform_device *pdev)
 {
 	struct sh_cmt_priv *p = platform_get_drvdata(pdev);
-	struct sh_timer_config *cfg = pdev->dev.platform_data;
+	struct sh_timer_config *cfg;
 	int ret;
 
 	if (!is_early_platform_device(pdev)) {
@@ -785,6 +847,11 @@ static int sh_cmt_probe(struct platform_device *pdev)
 		pm_runtime_enable(&pdev->dev);
 	}
 
+	if (pdev->dev.of_node)
+		cfg = sh_cmt_parse_dt(&pdev->dev);
+	else
+		cfg = pdev->dev.platform_data;
+
 	if (p) {
 		dev_info(&pdev->dev, "kept as earlytimer\n");
 		goto out;
@@ -796,7 +863,7 @@ static int sh_cmt_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	}
 
-	ret = sh_cmt_setup(p, pdev);
+	ret = sh_cmt_setup(p, pdev, cfg);
 	if (ret) {
 		kfree(p);
 		pm_runtime_idle(&pdev->dev);
@@ -824,6 +891,7 @@ static struct platform_driver sh_cmt_device_driver = {
 	.remove		= sh_cmt_remove,
 	.driver		= {
 		.name	= "sh_cmt",
+		.of_match_table = of_match_ptr(of_sh_cmt_match),
 	}
 };
 
-- 
1.7.9.5

[PATCH 2/3] ARM: mach-shmobile: sh73a0: Add CMT DT name to clock list

[Bastian Hecht]

This adds temporarily the alternative device name to the clock list
that is used when booting via Device Tree setup.

Signed-off-by: Bastian Hecht <hechtb+renesas@gmail.com>
---
 arch/arm/mach-shmobile/clock-sh73a0.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/mach-shmobile/clock-sh73a0.c b/arch/arm/mach-shmobile/clock-sh73a0.c
index 71843dd..4aed113 100644
--- a/arch/arm/mach-shmobile/clock-sh73a0.c
+++ b/arch/arm/mach-shmobile/clock-sh73a0.c
@@ -575,6 +575,7 @@ static struct clk_lookup lookups[] = {
 	CLKDEV_DEV_ID("sh-sci.4", &mstp_clks[MSTP200]), /* SCIFA4 */
 	CLKDEV_DEV_ID("sh-sci.6", &mstp_clks[MSTP331]), /* SCIFA6 */
 	CLKDEV_DEV_ID("sh_cmt.10", &mstp_clks[MSTP329]), /* CMT10 */
+	CLKDEV_DEV_ID("e6138010.cmt", &mstp_clks[MSTP329]), /* CMT10 */
 	CLKDEV_DEV_ID("sh_fsi2", &mstp_clks[MSTP328]), /* FSI */
 	CLKDEV_DEV_ID("sh_irda.0", &mstp_clks[MSTP325]), /* IrDA */
 	CLKDEV_DEV_ID("i2c-sh_mobile.1", &mstp_clks[MSTP323]), /* I2C1 */
-- 
1.7.9.5

[PATCH 3/3] ARM: mach-shmobile: sh73a0: Setup the timer device CMT10 using DT

[Bastian Hecht]

We can now use the Device Tree for bringing up our timer device CMT10.
We move it out of the DT devices list into the early_devices list by
the non-reference board code. And we add the device to the
kzm9g-reference .dts file.

Not-yet-signed-off-by: Bastian Hecht <hechtb+renesas@gmail.com>
---
I wonder if this should go to arch/arm/boot/dts/sh73a0.dtsi.

The moving to the early_devices is a bit hackish - I will cook up a better
patch when I know into which .dts(i) file things go.

 arch/arm/boot/dts/sh73a0-kzm9g-reference.dts |   11 +++++++++++
 arch/arm/mach-shmobile/setup-sh73a0.c        |    2 +-
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/sh73a0-kzm9g-reference.dts b/arch/arm/boot/dts/sh73a0-kzm9g-reference.dts
index 7fad4b9..fef12f0 100644
--- a/arch/arm/boot/dts/sh73a0-kzm9g-reference.dts
+++ b/arch/arm/boot/dts/sh73a0-kzm9g-reference.dts
@@ -69,6 +69,17 @@
 		toshiba,mmc-wrprotect-disable;
 		toshiba,mmc-has-idle-wait;
 	};
+
+	cmt at 0xe6138010 {
+		compatible = "renesas,cmt";
+		reg = <0xe6138010 0xc>;
+		interrupt-parent = <&gic>;
+		interrupts = <0 65 0x4>;
+		renesas,timer-device-id = <1>;
+		renesas,timer-channel-id = <0>;
+		renesas,clocksource-rating = <125>;
+		renesas,clockevent-rating = <125>;
+	};
 };
 
 &mmcif {
diff --git a/arch/arm/mach-shmobile/setup-sh73a0.c b/arch/arm/mach-shmobile/setup-sh73a0.c
index 6259e07..b7fdec0 100644
--- a/arch/arm/mach-shmobile/setup-sh73a0.c
+++ b/arch/arm/mach-shmobile/setup-sh73a0.c
@@ -913,10 +913,10 @@ static struct platform_device *sh73a0_devices_dt[] __initdata = {
 	&scif6_device,
 	&scif7_device,
 	&scif8_device,
-	&cmt10_device,
 };
 
 static struct platform_device *sh73a0_early_devices[] __initdata = {
+	&cmt10_device,
 	&tmu00_device,
 	&tmu01_device,
 };
-- 
1.7.9.5

Re: [PATCH 3/3] ARM: mach-shmobile: sh73a0: Setup the timer device CMT10 using DT

[Sergei Shtylyov]

Hello.

On 03/01/2013 08:45 PM, Bastian Hecht wrote:

> We can now use the Device Tree for bringing up our timer device CMT10.
> We move it out of the DT devices list into the early_devices list by
> the non-reference board code. And we add the device to the
> kzm9g-reference .dts file.
>
> Not-yet-signed-off-by: Bastian Hecht<hechtb+renesas@gmail.com>
> ---
> I wonder if this should go to arch/arm/boot/dts/sh73a0.dtsi.
>
> The moving to the early_devices is a bit hackish - I will cook up a better
> patch when I know into which .dts(i) file things go.
>
>   arch/arm/boot/dts/sh73a0-kzm9g-reference.dts |   11 +++++++++++
>   arch/arm/mach-shmobile/setup-sh73a0.c        |    2 +-
>   2 files changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/boot/dts/sh73a0-kzm9g-reference.dts b/arch/arm/boot/dts/sh73a0-kzm9g-reference.dts
> index 7fad4b9..fef12f0 100644
> --- a/arch/arm/boot/dts/sh73a0-kzm9g-reference.dts
> +++ b/arch/arm/boot/dts/sh73a0-kzm9g-reference.dts
> @@ -69,6 +69,17 @@
>   		toshiba,mmc-wrprotect-disable;
>   		toshiba,mmc-has-idle-wait;
>   	};
> +
> +	cmt@0xe6138010 {
>    

     "0x" shouldn't be there, remove it please.

WBR, Sergei

[PATCH 3/3] ARM: mach-shmobile: sh73a0: Setup the timer device CMT10 using DT

[Sergei Shtylyov]

Hello.

On 03/01/2013 08:45 PM, Bastian Hecht wrote:

> We can now use the Device Tree for bringing up our timer device CMT10.
> We move it out of the DT devices list into the early_devices list by
> the non-reference board code. And we add the device to the
> kzm9g-reference .dts file.
>
> Not-yet-signed-off-by: Bastian Hecht<hechtb+renesas@gmail.com>
> ---
> I wonder if this should go to arch/arm/boot/dts/sh73a0.dtsi.
>
> The moving to the early_devices is a bit hackish - I will cook up a better
> patch when I know into which .dts(i) file things go.
>
>   arch/arm/boot/dts/sh73a0-kzm9g-reference.dts |   11 +++++++++++
>   arch/arm/mach-shmobile/setup-sh73a0.c        |    2 +-
>   2 files changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/boot/dts/sh73a0-kzm9g-reference.dts b/arch/arm/boot/dts/sh73a0-kzm9g-reference.dts
> index 7fad4b9..fef12f0 100644
> --- a/arch/arm/boot/dts/sh73a0-kzm9g-reference.dts
> +++ b/arch/arm/boot/dts/sh73a0-kzm9g-reference.dts
> @@ -69,6 +69,17 @@
>   		toshiba,mmc-wrprotect-disable;
>   		toshiba,mmc-has-idle-wait;
>   	};
> +
> +	cmt at 0xe6138010 {
>    

     "0x" shouldn't be there, remove it please.

WBR, Sergei

Re: [PATCH 1/3] clocksource: sh_cmt: Add Device Tree probing

[Paul Mundt]

On Fri, Mar 01, 2013 at 11:45:30AM -0600, Bastian Hecht wrote:
> We add the capabilty to probe SH CMT timer devices using Device Tree
> setup.
> 
> We can deduce former platform data by the device IDs and channel
> IDs of our timer instances, so we choose this more intuitive info as our
> DT properties.
> 
I wonder if it makes more sense to attempt to create a more generalized
binding, as what you have here is pretty much uniform for all of
CMT/TMU/MTU2. Perhaps having a general sh-timer binding would be cleaner,
while letting each timer plug the compatible string it cares about.

[PATCH 1/3] clocksource: sh_cmt: Add Device Tree probing

[Paul Mundt]

On Fri, Mar 01, 2013 at 11:45:30AM -0600, Bastian Hecht wrote:
> We add the capabilty to probe SH CMT timer devices using Device Tree
> setup.
> 
> We can deduce former platform data by the device IDs and channel
> IDs of our timer instances, so we choose this more intuitive info as our
> DT properties.
> 
I wonder if it makes more sense to attempt to create a more generalized
binding, as what you have here is pretty much uniform for all of
CMT/TMU/MTU2. Perhaps having a general sh-timer binding would be cleaner,
while letting each timer plug the compatible string it cares about.

Re: [PATCH 1/3] clocksource: sh_cmt: Add Device Tree probing

[Mark Rutland]

Hello,

I have a couple of comments on the binding and the way it's parsed.

On Fri, Mar 01, 2013 at 05:45:30PM +0000, Bastian Hecht wrote:
> We add the capabilty to probe SH CMT timer devices using Device Tree
> setup.
> 
> We can deduce former platform data by the device IDs and channel
> IDs of our timer instances, so we choose this more intuitive info as our
> DT properties.
> 
> Signed-off-by: Bastian Hecht <hechtb+renesas@gmail.com>
> ---
>  .../bindings/timer/renesas,sh-cmt-timer.txt        |   21 +++++
>  drivers/clocksource/sh_cmt.c                       |   94 +++++++++++++++++---
>  2 files changed, 102 insertions(+), 13 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/timer/renesas,sh-cmt-timer.txt
> 
> diff --git a/Documentation/devicetree/bindings/timer/renesas,sh-cmt-timer.txt b/Documentation/devicetree/bindings/timer/renesas,sh-cmt-timer.txt
> new file mode 100644
> index 0000000..e5ad808
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/timer/renesas,sh-cmt-timer.txt
> @@ -0,0 +1,21 @@
> +* Renesas SH Mobile Compare Match Timer
> +
> +Required properties:
> +- compatible : Should be "renesas,cmt"
> +- reg : Address and length of the register set for the device
> +- interrupts : Timer IRQ
> +- renesas,timer-device-id : The ID of the timer device
> +- renesas,timer-channel-id : The channel ID of the timer device

I'm not familiar with this hardware. Could you give me a basic idea of how it's
structured?

Does the device have a single timer that this describes, or does the device
have multiple timers, and this selects which one to use?

> +
> +Example for CMT10 of the R8A7740 SoC:
> +
> +	cmt@0xe6138010 {
> +		compatible = "renesas,cmt";
> +		interrupt-parent = <&intca>;
> +		reg = <0xe6138010 0xc>;
> +		interrupts = <0x0b00>;
> +		renesas,timer-device-id = <1>;
> +		renesas,timer-channel-id = <0>;
> +		renesas,clocksource-rating = <150>;
> +		renesas,clockevent-rating = <150>;

I'm not sure these last two are describing the hardware as such, they look like
Linux implementation details. Do these really need to be in the binding?

[...]

> +static struct sh_timer_config *sh_cmt_parse_dt(struct device *dev)
> +{
> +	struct sh_timer_config *cfg;
> +	struct device_node *np = dev->of_node;
> +	const __be32 *prop;
> +	int timer_id, channel_id;
> +
> +	cfg = devm_kzalloc(dev, sizeof(struct sh_timer_config), GFP_KERNEL);
> +	if (!cfg) {
> +		dev_err(dev, "failed to allocate DT config data\n");
> +		return NULL;
> +	}
> +
> +	prop = of_get_property(np, "renesas,timer-device-id", NULL);
> +	if (!prop) {
> +		dev_err(dev, "device id missing\n");
> +		return NULL;
> +	}
> +	timer_id = be32_to_cpup(prop);

You can use of_property_read_u32 here, e.g.

if (!of_property_read_u32(np, "renesas,timer-device-id", &timer_id) {
	dev_err(dev, "device id missing\n");
	return NULL;
}

> +
> +	prop = of_get_property(np, "renesas,timer-channel-id", NULL);
> +	if (!prop) {
> +		dev_err(dev, "channel id missing\n");
> +		return NULL;
> +	}
> +	channel_id = be32_to_cpup(prop);

You can use of_property_read_u32 here too.

I assume there's a sensible range of channel_id values that could be checked
before it's used in a calculation below.

> +
> +	if (timer_id >= ARRAY_SIZE(sh_timer_offset_multiplier)) {
> +		dev_err(dev, "unsupported timer id\n");
> +		return NULL;
> +	}
> +
> +	cfg->channel_offset = sh_timer_offset_multiplier[timer_id] *
> +							(channel_id + 1);
> +	cfg->timer_bit = channel_id;
> +
> +	prop = of_get_property(np, "renesas,clocksource-rating", NULL);
> +	if (prop)
> +		cfg->clocksource_rating = be32_to_cpup(prop);

You can use of_property_read_u32 here too.

> +
> +	prop = of_get_property(np, "renesas,clockevent-rating", NULL);
> +	if (prop)
> +		cfg->clockevent_rating = be32_to_cpup(prop);

And here.

[...]

Thanks,
Mark.

[PATCH 1/3] clocksource: sh_cmt: Add Device Tree probing

[Mark Rutland]

Hello,

I have a couple of comments on the binding and the way it's parsed.

On Fri, Mar 01, 2013 at 05:45:30PM +0000, Bastian Hecht wrote:
> We add the capabilty to probe SH CMT timer devices using Device Tree
> setup.
> 
> We can deduce former platform data by the device IDs and channel
> IDs of our timer instances, so we choose this more intuitive info as our
> DT properties.
> 
> Signed-off-by: Bastian Hecht <hechtb+renesas@gmail.com>
> ---
>  .../bindings/timer/renesas,sh-cmt-timer.txt        |   21 +++++
>  drivers/clocksource/sh_cmt.c                       |   94 +++++++++++++++++---
>  2 files changed, 102 insertions(+), 13 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/timer/renesas,sh-cmt-timer.txt
> 
> diff --git a/Documentation/devicetree/bindings/timer/renesas,sh-cmt-timer.txt b/Documentation/devicetree/bindings/timer/renesas,sh-cmt-timer.txt
> new file mode 100644
> index 0000000..e5ad808
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/timer/renesas,sh-cmt-timer.txt
> @@ -0,0 +1,21 @@
> +* Renesas SH Mobile Compare Match Timer
> +
> +Required properties:
> +- compatible : Should be "renesas,cmt"
> +- reg : Address and length of the register set for the device
> +- interrupts : Timer IRQ
> +- renesas,timer-device-id : The ID of the timer device
> +- renesas,timer-channel-id : The channel ID of the timer device

I'm not familiar with this hardware. Could you give me a basic idea of how it's
structured?

Does the device have a single timer that this describes, or does the device
have multiple timers, and this selects which one to use?

> +
> +Example for CMT10 of the R8A7740 SoC:
> +
> +	cmt at 0xe6138010 {
> +		compatible = "renesas,cmt";
> +		interrupt-parent = <&intca>;
> +		reg = <0xe6138010 0xc>;
> +		interrupts = <0x0b00>;
> +		renesas,timer-device-id = <1>;
> +		renesas,timer-channel-id = <0>;
> +		renesas,clocksource-rating = <150>;
> +		renesas,clockevent-rating = <150>;

I'm not sure these last two are describing the hardware as such, they look like
Linux implementation details. Do these really need to be in the binding?

[...]

> +static struct sh_timer_config *sh_cmt_parse_dt(struct device *dev)
> +{
> +	struct sh_timer_config *cfg;
> +	struct device_node *np = dev->of_node;
> +	const __be32 *prop;
> +	int timer_id, channel_id;
> +
> +	cfg = devm_kzalloc(dev, sizeof(struct sh_timer_config), GFP_KERNEL);
> +	if (!cfg) {
> +		dev_err(dev, "failed to allocate DT config data\n");
> +		return NULL;
> +	}
> +
> +	prop = of_get_property(np, "renesas,timer-device-id", NULL);
> +	if (!prop) {
> +		dev_err(dev, "device id missing\n");
> +		return NULL;
> +	}
> +	timer_id = be32_to_cpup(prop);

You can use of_property_read_u32 here, e.g.

if (!of_property_read_u32(np, "renesas,timer-device-id", &timer_id) {
	dev_err(dev, "device id missing\n");
	return NULL;
}

> +
> +	prop = of_get_property(np, "renesas,timer-channel-id", NULL);
> +	if (!prop) {
> +		dev_err(dev, "channel id missing\n");
> +		return NULL;
> +	}
> +	channel_id = be32_to_cpup(prop);

You can use of_property_read_u32 here too.

I assume there's a sensible range of channel_id values that could be checked
before it's used in a calculation below.

> +
> +	if (timer_id >= ARRAY_SIZE(sh_timer_offset_multiplier)) {
> +		dev_err(dev, "unsupported timer id\n");
> +		return NULL;
> +	}
> +
> +	cfg->channel_offset = sh_timer_offset_multiplier[timer_id] *
> +							(channel_id + 1);
> +	cfg->timer_bit = channel_id;
> +
> +	prop = of_get_property(np, "renesas,clocksource-rating", NULL);
> +	if (prop)
> +		cfg->clocksource_rating = be32_to_cpup(prop);

You can use of_property_read_u32 here too.

> +
> +	prop = of_get_property(np, "renesas,clockevent-rating", NULL);
> +	if (prop)
> +		cfg->clockevent_rating = be32_to_cpup(prop);

And here.

[...]

Thanks,
Mark.

Re: [PATCH 3/3] ARM: mach-shmobile: sh73a0: Setup the timer device CMT10 using DT

[Bastian Hecht]

Hi,

2013/3/1 Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>:
> Hello.
>
>
> On 03/01/2013 08:45 PM, Bastian Hecht wrote:
>
>> We can now use the Device Tree for bringing up our timer device CMT10.
>> We move it out of the DT devices list into the early_devices list by
>> the non-reference board code. And we add the device to the
>> kzm9g-reference .dts file.
>>
>> Not-yet-signed-off-by: Bastian Hecht<hechtb+renesas@gmail.com>
>> ---
>> I wonder if this should go to arch/arm/boot/dts/sh73a0.dtsi.
>>
>> The moving to the early_devices is a bit hackish - I will cook up a better
>> patch when I know into which .dts(i) file things go.
>>
>>   arch/arm/boot/dts/sh73a0-kzm9g-reference.dts |   11 +++++++++++
>>   arch/arm/mach-shmobile/setup-sh73a0.c        |    2 +-
>>   2 files changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/boot/dts/sh73a0-kzm9g-reference.dts
>> b/arch/arm/boot/dts/sh73a0-kzm9g-reference.dts
>> index 7fad4b9..fef12f0 100644
>> --- a/arch/arm/boot/dts/sh73a0-kzm9g-reference.dts
>> +++ b/arch/arm/boot/dts/sh73a0-kzm9g-reference.dts
>> @@ -69,6 +69,17 @@
>>                 toshiba,mmc-wrprotect-disable;
>>                 toshiba,mmc-has-idle-wait;
>>         };
>> +
>> +       cmt@0xe6138010 {
>>
>
>
>     "0x" shouldn't be there, remove it please.
>
> WBR, Sergei
>

Thanks for pointing out this one. Fixed it in another patchset as well.

Bastian

[PATCH 3/3] ARM: mach-shmobile: sh73a0: Setup the timer device CMT10 using DT

[Bastian Hecht]

Hi,

2013/3/1 Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>:
> Hello.
>
>
> On 03/01/2013 08:45 PM, Bastian Hecht wrote:
>
>> We can now use the Device Tree for bringing up our timer device CMT10.
>> We move it out of the DT devices list into the early_devices list by
>> the non-reference board code. And we add the device to the
>> kzm9g-reference .dts file.
>>
>> Not-yet-signed-off-by: Bastian Hecht<hechtb+renesas@gmail.com>
>> ---
>> I wonder if this should go to arch/arm/boot/dts/sh73a0.dtsi.
>>
>> The moving to the early_devices is a bit hackish - I will cook up a better
>> patch when I know into which .dts(i) file things go.
>>
>>   arch/arm/boot/dts/sh73a0-kzm9g-reference.dts |   11 +++++++++++
>>   arch/arm/mach-shmobile/setup-sh73a0.c        |    2 +-
>>   2 files changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/boot/dts/sh73a0-kzm9g-reference.dts
>> b/arch/arm/boot/dts/sh73a0-kzm9g-reference.dts
>> index 7fad4b9..fef12f0 100644
>> --- a/arch/arm/boot/dts/sh73a0-kzm9g-reference.dts
>> +++ b/arch/arm/boot/dts/sh73a0-kzm9g-reference.dts
>> @@ -69,6 +69,17 @@
>>                 toshiba,mmc-wrprotect-disable;
>>                 toshiba,mmc-has-idle-wait;
>>         };
>> +
>> +       cmt at 0xe6138010 {
>>
>
>
>     "0x" shouldn't be there, remove it please.
>
> WBR, Sergei
>

Thanks for pointing out this one. Fixed it in another patchset as well.

Bastian

Re: [PATCH 1/3] clocksource: sh_cmt: Add Device Tree probing

[Bastian Hecht]

Hi Paul,

2013/3/4 Paul Mundt <lethal@linux-sh.org>:
> On Fri, Mar 01, 2013 at 11:45:30AM -0600, Bastian Hecht wrote:
>> We add the capabilty to probe SH CMT timer devices using Device Tree
>> setup.
>>
>> We can deduce former platform data by the device IDs and channel
>> IDs of our timer instances, so we choose this more intuitive info as our
>> DT properties.
>>
> I wonder if it makes more sense to attempt to create a more generalized
> binding, as what you have here is pretty much uniform for all of
> CMT/TMU/MTU2. Perhaps having a general sh-timer binding would be cleaner,
> while letting each timer plug the compatible string it cares about.

Yes that sounds good. I'll have a look at the various timers and see
if I can come up with something good.

Thanks,

 Bastian

[PATCH 1/3] clocksource: sh_cmt: Add Device Tree probing

[Bastian Hecht]

Hi Paul,

2013/3/4 Paul Mundt <lethal@linux-sh.org>:
> On Fri, Mar 01, 2013 at 11:45:30AM -0600, Bastian Hecht wrote:
>> We add the capabilty to probe SH CMT timer devices using Device Tree
>> setup.
>>
>> We can deduce former platform data by the device IDs and channel
>> IDs of our timer instances, so we choose this more intuitive info as our
>> DT properties.
>>
> I wonder if it makes more sense to attempt to create a more generalized
> binding, as what you have here is pretty much uniform for all of
> CMT/TMU/MTU2. Perhaps having a general sh-timer binding would be cleaner,
> while letting each timer plug the compatible string it cares about.

Yes that sounds good. I'll have a look at the various timers and see
if I can come up with something good.

Thanks,

 Bastian

Re: [PATCH 1/3] clocksource: sh_cmt: Add Device Tree probing

[Bastian Hecht]

Hi Mark,

2013/3/4 Mark Rutland <mark.rutland@arm.com>:
> Hello,
>
> I have a couple of comments on the binding and the way it's parsed.

Thank you - appreciated!

> On Fri, Mar 01, 2013 at 05:45:30PM +0000, Bastian Hecht wrote:
>> We add the capabilty to probe SH CMT timer devices using Device Tree
>> setup.
>>
>> We can deduce former platform data by the device IDs and channel
>> IDs of our timer instances, so we choose this more intuitive info as our
>> DT properties.
>>
>> Signed-off-by: Bastian Hecht <hechtb+renesas@gmail.com>
>> ---
>>  .../bindings/timer/renesas,sh-cmt-timer.txt        |   21 +++++
>>  drivers/clocksource/sh_cmt.c                       |   94 +++++++++++++++++---
>>  2 files changed, 102 insertions(+), 13 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/timer/renesas,sh-cmt-timer.txt
>>
>> diff --git a/Documentation/devicetree/bindings/timer/renesas,sh-cmt-timer.txt b/Documentation/devicetree/bindings/timer/renesas,sh-cmt-timer.txt
>> new file mode 100644
>> index 0000000..e5ad808
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/timer/renesas,sh-cmt-timer.txt
>> @@ -0,0 +1,21 @@
>> +* Renesas SH Mobile Compare Match Timer
>> +
>> +Required properties:
>> +- compatible : Should be "renesas,cmt"
>> +- reg : Address and length of the register set for the device
>> +- interrupts : Timer IRQ
>> +- renesas,timer-device-id : The ID of the timer device
>> +- renesas,timer-channel-id : The channel ID of the timer device
>
> I'm not familiar with this hardware. Could you give me a basic idea of how it's
> structured?
>
> Does the device have a single timer that this describes, or does the device
> have multiple timers, and this selects which one to use?

The CMT timer is built into various SoCs from Renesas, often multiple
instances of it. Each timer instance has 6 channels. You can select if
they are driven by the main clock or the real time clock (the SoCs
include an SH core for data processing) and can use different subscale
modes. It features free running mode and can fire events. The clock
has up to 48 bits to count. It has a DMA channel and wires enabling
the device to be a wakeup source. Hmm don't know what to add more.
It's a timer - you can check for overflows, read the counter and so
on.

>> +
>> +Example for CMT10 of the R8A7740 SoC:
>> +
>> +     cmt@0xe6138010 {
>> +             compatible = "renesas,cmt";
>> +             interrupt-parent = <&intca>;
>> +             reg = <0xe6138010 0xc>;
>> +             interrupts = <0x0b00>;
>> +             renesas,timer-device-id = <1>;
>> +             renesas,timer-channel-id = <0>;
>> +             renesas,clocksource-rating = <150>;
>> +             renesas,clockevent-rating = <150>;
>
> I'm not sure these last two are describing the hardware as such, they look like
> Linux implementation details. Do these really need to be in the binding?

That's true - they don't describe the hardware. It's a policy how the
device should be used. Here: Register it as clocksource *and*
eventsource. I don't know if there is a use case when we want only 1
of them.

> [...]
>
>> +static struct sh_timer_config *sh_cmt_parse_dt(struct device *dev)
>> +{
>> +     struct sh_timer_config *cfg;
>> +     struct device_node *np = dev->of_node;
>> +     const __be32 *prop;
>> +     int timer_id, channel_id;
>> +
>> +     cfg = devm_kzalloc(dev, sizeof(struct sh_timer_config), GFP_KERNEL);
>> +     if (!cfg) {
>> +             dev_err(dev, "failed to allocate DT config data\n");
>> +             return NULL;
>> +     }
>> +
>> +     prop = of_get_property(np, "renesas,timer-device-id", NULL);
>> +     if (!prop) {
>> +             dev_err(dev, "device id missing\n");
>> +             return NULL;
>> +     }
>> +     timer_id = be32_to_cpup(prop);
>
> You can use of_property_read_u32 here, e.g.

Oh yes nice! I will convert these calls.

> if (!of_property_read_u32(np, "renesas,timer-device-id", &timer_id) {
>         dev_err(dev, "device id missing\n");
>         return NULL;
> }
>
>> +
>> +     prop = of_get_property(np, "renesas,timer-channel-id", NULL);
>> +     if (!prop) {
>> +             dev_err(dev, "channel id missing\n");
>> +             return NULL;
>> +     }
>> +     channel_id = be32_to_cpup(prop);
>
> You can use of_property_read_u32 here too.
>
> I assume there's a sensible range of channel_id values that could be checked
> before it's used in a calculation below.

Oh true. Jeeez - this very much looks like exploitable code. If
someone manages to change the boot config...
Whatever - I'll add a check.

>> +
>> +     if (timer_id >= ARRAY_SIZE(sh_timer_offset_multiplier)) {
>> +             dev_err(dev, "unsupported timer id\n");
>> +             return NULL;
>> +     }
>> +
>> +     cfg->channel_offset = sh_timer_offset_multiplier[timer_id] *
>> +                                                     (channel_id + 1);
>> +     cfg->timer_bit = channel_id;
>> +
>> +     prop = of_get_property(np, "renesas,clocksource-rating", NULL);
>> +     if (prop)
>> +             cfg->clocksource_rating = be32_to_cpup(prop);
>
> You can use of_property_read_u32 here too.

Will do.

>> +
>> +     prop = of_get_property(np, "renesas,clockevent-rating", NULL);
>> +     if (prop)
>> +             cfg->clockevent_rating = be32_to_cpup(prop);
>
> And here.

Dito.

> [...]
>
> Thanks,
> Mark.

Thanks for this productive review!

Bastian

[PATCH 1/3] clocksource: sh_cmt: Add Device Tree probing

[Bastian Hecht]

Hi Mark,

2013/3/4 Mark Rutland <mark.rutland@arm.com>:
> Hello,
>
> I have a couple of comments on the binding and the way it's parsed.

Thank you - appreciated!

> On Fri, Mar 01, 2013 at 05:45:30PM +0000, Bastian Hecht wrote:
>> We add the capabilty to probe SH CMT timer devices using Device Tree
>> setup.
>>
>> We can deduce former platform data by the device IDs and channel
>> IDs of our timer instances, so we choose this more intuitive info as our
>> DT properties.
>>
>> Signed-off-by: Bastian Hecht <hechtb+renesas@gmail.com>
>> ---
>>  .../bindings/timer/renesas,sh-cmt-timer.txt        |   21 +++++
>>  drivers/clocksource/sh_cmt.c                       |   94 +++++++++++++++++---
>>  2 files changed, 102 insertions(+), 13 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/timer/renesas,sh-cmt-timer.txt
>>
>> diff --git a/Documentation/devicetree/bindings/timer/renesas,sh-cmt-timer.txt b/Documentation/devicetree/bindings/timer/renesas,sh-cmt-timer.txt
>> new file mode 100644
>> index 0000000..e5ad808
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/timer/renesas,sh-cmt-timer.txt
>> @@ -0,0 +1,21 @@
>> +* Renesas SH Mobile Compare Match Timer
>> +
>> +Required properties:
>> +- compatible : Should be "renesas,cmt"
>> +- reg : Address and length of the register set for the device
>> +- interrupts : Timer IRQ
>> +- renesas,timer-device-id : The ID of the timer device
>> +- renesas,timer-channel-id : The channel ID of the timer device
>
> I'm not familiar with this hardware. Could you give me a basic idea of how it's
> structured?
>
> Does the device have a single timer that this describes, or does the device
> have multiple timers, and this selects which one to use?

The CMT timer is built into various SoCs from Renesas, often multiple
instances of it. Each timer instance has 6 channels. You can select if
they are driven by the main clock or the real time clock (the SoCs
include an SH core for data processing) and can use different subscale
modes. It features free running mode and can fire events. The clock
has up to 48 bits to count. It has a DMA channel and wires enabling
the device to be a wakeup source. Hmm don't know what to add more.
It's a timer - you can check for overflows, read the counter and so
on.

>> +
>> +Example for CMT10 of the R8A7740 SoC:
>> +
>> +     cmt at 0xe6138010 {
>> +             compatible = "renesas,cmt";
>> +             interrupt-parent = <&intca>;
>> +             reg = <0xe6138010 0xc>;
>> +             interrupts = <0x0b00>;
>> +             renesas,timer-device-id = <1>;
>> +             renesas,timer-channel-id = <0>;
>> +             renesas,clocksource-rating = <150>;
>> +             renesas,clockevent-rating = <150>;
>
> I'm not sure these last two are describing the hardware as such, they look like
> Linux implementation details. Do these really need to be in the binding?

That's true - they don't describe the hardware. It's a policy how the
device should be used. Here: Register it as clocksource *and*
eventsource. I don't know if there is a use case when we want only 1
of them.

> [...]
>
>> +static struct sh_timer_config *sh_cmt_parse_dt(struct device *dev)
>> +{
>> +     struct sh_timer_config *cfg;
>> +     struct device_node *np = dev->of_node;
>> +     const __be32 *prop;
>> +     int timer_id, channel_id;
>> +
>> +     cfg = devm_kzalloc(dev, sizeof(struct sh_timer_config), GFP_KERNEL);
>> +     if (!cfg) {
>> +             dev_err(dev, "failed to allocate DT config data\n");
>> +             return NULL;
>> +     }
>> +
>> +     prop = of_get_property(np, "renesas,timer-device-id", NULL);
>> +     if (!prop) {
>> +             dev_err(dev, "device id missing\n");
>> +             return NULL;
>> +     }
>> +     timer_id = be32_to_cpup(prop);
>
> You can use of_property_read_u32 here, e.g.

Oh yes nice! I will convert these calls.

> if (!of_property_read_u32(np, "renesas,timer-device-id", &timer_id) {
>         dev_err(dev, "device id missing\n");
>         return NULL;
> }
>
>> +
>> +     prop = of_get_property(np, "renesas,timer-channel-id", NULL);
>> +     if (!prop) {
>> +             dev_err(dev, "channel id missing\n");
>> +             return NULL;
>> +     }
>> +     channel_id = be32_to_cpup(prop);
>
> You can use of_property_read_u32 here too.
>
> I assume there's a sensible range of channel_id values that could be checked
> before it's used in a calculation below.

Oh true. Jeeez - this very much looks like exploitable code. If
someone manages to change the boot config...
Whatever - I'll add a check.

>> +
>> +     if (timer_id >= ARRAY_SIZE(sh_timer_offset_multiplier)) {
>> +             dev_err(dev, "unsupported timer id\n");
>> +             return NULL;
>> +     }
>> +
>> +     cfg->channel_offset = sh_timer_offset_multiplier[timer_id] *
>> +                                                     (channel_id + 1);
>> +     cfg->timer_bit = channel_id;
>> +
>> +     prop = of_get_property(np, "renesas,clocksource-rating", NULL);
>> +     if (prop)
>> +             cfg->clocksource_rating = be32_to_cpup(prop);
>
> You can use of_property_read_u32 here too.

Will do.

>> +
>> +     prop = of_get_property(np, "renesas,clockevent-rating", NULL);
>> +     if (prop)
>> +             cfg->clockevent_rating = be32_to_cpup(prop);
>
> And here.

Dito.

> [...]
>
> Thanks,
> Mark.

Thanks for this productive review!

Bastian

Re: [PATCH 1/3] clocksource: sh_cmt: Add Device Tree probing

[Mark Rutland]

Hi again,

> >> diff --git a/Documentation/devicetree/bindings/timer/renesas,sh-cmt-timer.txt b/Documentation/devicetree/bindings/timer/renesas,sh-cmt-timer.txt
> >> new file mode 100644
> >> index 0000000..e5ad808
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/timer/renesas,sh-cmt-timer.txt
> >> @@ -0,0 +1,21 @@
> >> +* Renesas SH Mobile Compare Match Timer
> >> +
> >> +Required properties:
> >> +- compatible : Should be "renesas,cmt"
> >> +- reg : Address and length of the register set for the device
> >> +- interrupts : Timer IRQ
> >> +- renesas,timer-device-id : The ID of the timer device
> >> +- renesas,timer-channel-id : The channel ID of the timer device
> >
> > I'm not familiar with this hardware. Could you give me a basic idea of how it's
> > structured?
> >
> > Does the device have a single timer that this describes, or does the device
> > have multiple timers, and this selects which one to use?
> 
> The CMT timer is built into various SoCs from Renesas, often multiple
> instances of it. Each timer instance has 6 channels. You can select if
> they are driven by the main clock or the real time clock (the SoCs
> include an SH core for data processing) and can use different subscale
> modes. It features free running mode and can fire events. The clock
> has up to 48 bits to count. It has a DMA channel and wires enabling
> the device to be a wakeup source. Hmm don't know what to add more.
> It's a timer - you can check for overflows, read the counter and so
> on.

Unless I've misunderstood, couldn't Linux choose one timer to use completely
arbitrarily?

Why do we need to describe which one to use?

> 
> >> +
> >> +Example for CMT10 of the R8A7740 SoC:
> >> +
> >> +     cmt@0xe6138010 {
> >> +             compatible = "renesas,cmt";
> >> +             interrupt-parent = <&intca>;
> >> +             reg = <0xe6138010 0xc>;
> >> +             interrupts = <0x0b00>;
> >> +             renesas,timer-device-id = <1>;
> >> +             renesas,timer-channel-id = <0>;
> >> +             renesas,clocksource-rating = <150>;
> >> +             renesas,clockevent-rating = <150>;
> >
> > I'm not sure these last two are describing the hardware as such, they look like
> > Linux implementation details. Do these really need to be in the binding?
> 
> That's true - they don't describe the hardware. It's a policy how the
> device should be used. Here: Register it as clocksource *and*
> eventsource. I don't know if there is a use case when we want only 1
> of them.

I don't think it's a good idea to expose something like this through the
binding, as it's strongly tied to the current Linux implementation. If we can
always use the device as both, we may as well (other drivers already do). If
the quality of the clocksource / timer is variable enough that it's worth
describing, maybe there's a better way of describing this variability
specifically?

> 
> > [...]
> >
> >> +static struct sh_timer_config *sh_cmt_parse_dt(struct device *dev)
> >> +{
> >> +     struct sh_timer_config *cfg;
> >> +     struct device_node *np = dev->of_node;
> >> +     const __be32 *prop;
> >> +     int timer_id, channel_id;
> >> +
> >> +     cfg = devm_kzalloc(dev, sizeof(struct sh_timer_config), GFP_KERNEL);
> >> +     if (!cfg) {
> >> +             dev_err(dev, "failed to allocate DT config data\n");
> >> +             return NULL;
> >> +     }
> >> +
> >> +     prop = of_get_property(np, "renesas,timer-device-id", NULL);
> >> +     if (!prop) {
> >> +             dev_err(dev, "device id missing\n");
> >> +             return NULL;
> >> +     }
> >> +     timer_id = be32_to_cpup(prop);
> >
> > You can use of_property_read_u32 here, e.g.
> 
> Oh yes nice! I will convert these calls.
> 
> > if (!of_property_read_u32(np, "renesas,timer-device-id", &timer_id) {
> >         dev_err(dev, "device id missing\n");
> >         return NULL;
> > }

I've just realised some of the variables being assigned to aren't u32s.  While
it'd still be nicer to use of_property_read_u32, you may still need a temporary
variable.

> >
> >> +
> >> +     prop = of_get_property(np, "renesas,timer-channel-id", NULL);
> >> +     if (!prop) {
> >> +             dev_err(dev, "channel id missing\n");
> >> +             return NULL;
> >> +     }
> >> +     channel_id = be32_to_cpup(prop);
> >
> > You can use of_property_read_u32 here too.
> >
> > I assume there's a sensible range of channel_id values that could be checked
> > before it's used in a calculation below.
> 
> Oh true. Jeeez - this very much looks like exploitable code. If
> someone manages to change the boot config...
> Whatever - I'll add a check.

What I meant was it'd be nice to warn the user that the dt doesn't look right
so it's easier to debug when something blows up unexpectedly.

[...]

Thanks,
Mark.

[PATCH 1/3] clocksource: sh_cmt: Add Device Tree probing

[Mark Rutland]

Hi again,

> >> diff --git a/Documentation/devicetree/bindings/timer/renesas,sh-cmt-timer.txt b/Documentation/devicetree/bindings/timer/renesas,sh-cmt-timer.txt
> >> new file mode 100644
> >> index 0000000..e5ad808
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/timer/renesas,sh-cmt-timer.txt
> >> @@ -0,0 +1,21 @@
> >> +* Renesas SH Mobile Compare Match Timer
> >> +
> >> +Required properties:
> >> +- compatible : Should be "renesas,cmt"
> >> +- reg : Address and length of the register set for the device
> >> +- interrupts : Timer IRQ
> >> +- renesas,timer-device-id : The ID of the timer device
> >> +- renesas,timer-channel-id : The channel ID of the timer device
> >
> > I'm not familiar with this hardware. Could you give me a basic idea of how it's
> > structured?
> >
> > Does the device have a single timer that this describes, or does the device
> > have multiple timers, and this selects which one to use?
> 
> The CMT timer is built into various SoCs from Renesas, often multiple
> instances of it. Each timer instance has 6 channels. You can select if
> they are driven by the main clock or the real time clock (the SoCs
> include an SH core for data processing) and can use different subscale
> modes. It features free running mode and can fire events. The clock
> has up to 48 bits to count. It has a DMA channel and wires enabling
> the device to be a wakeup source. Hmm don't know what to add more.
> It's a timer - you can check for overflows, read the counter and so
> on.

Unless I've misunderstood, couldn't Linux choose one timer to use completely
arbitrarily?

Why do we need to describe which one to use?

> 
> >> +
> >> +Example for CMT10 of the R8A7740 SoC:
> >> +
> >> +     cmt at 0xe6138010 {
> >> +             compatible = "renesas,cmt";
> >> +             interrupt-parent = <&intca>;
> >> +             reg = <0xe6138010 0xc>;
> >> +             interrupts = <0x0b00>;
> >> +             renesas,timer-device-id = <1>;
> >> +             renesas,timer-channel-id = <0>;
> >> +             renesas,clocksource-rating = <150>;
> >> +             renesas,clockevent-rating = <150>;
> >
> > I'm not sure these last two are describing the hardware as such, they look like
> > Linux implementation details. Do these really need to be in the binding?
> 
> That's true - they don't describe the hardware. It's a policy how the
> device should be used. Here: Register it as clocksource *and*
> eventsource. I don't know if there is a use case when we want only 1
> of them.

I don't think it's a good idea to expose something like this through the
binding, as it's strongly tied to the current Linux implementation. If we can
always use the device as both, we may as well (other drivers already do). If
the quality of the clocksource / timer is variable enough that it's worth
describing, maybe there's a better way of describing this variability
specifically?

> 
> > [...]
> >
> >> +static struct sh_timer_config *sh_cmt_parse_dt(struct device *dev)
> >> +{
> >> +     struct sh_timer_config *cfg;
> >> +     struct device_node *np = dev->of_node;
> >> +     const __be32 *prop;
> >> +     int timer_id, channel_id;
> >> +
> >> +     cfg = devm_kzalloc(dev, sizeof(struct sh_timer_config), GFP_KERNEL);
> >> +     if (!cfg) {
> >> +             dev_err(dev, "failed to allocate DT config data\n");
> >> +             return NULL;
> >> +     }
> >> +
> >> +     prop = of_get_property(np, "renesas,timer-device-id", NULL);
> >> +     if (!prop) {
> >> +             dev_err(dev, "device id missing\n");
> >> +             return NULL;
> >> +     }
> >> +     timer_id = be32_to_cpup(prop);
> >
> > You can use of_property_read_u32 here, e.g.
> 
> Oh yes nice! I will convert these calls.
> 
> > if (!of_property_read_u32(np, "renesas,timer-device-id", &timer_id) {
> >         dev_err(dev, "device id missing\n");
> >         return NULL;
> > }

I've just realised some of the variables being assigned to aren't u32s.  While
it'd still be nicer to use of_property_read_u32, you may still need a temporary
variable.

> >
> >> +
> >> +     prop = of_get_property(np, "renesas,timer-channel-id", NULL);
> >> +     if (!prop) {
> >> +             dev_err(dev, "channel id missing\n");
> >> +             return NULL;
> >> +     }
> >> +     channel_id = be32_to_cpup(prop);
> >
> > You can use of_property_read_u32 here too.
> >
> > I assume there's a sensible range of channel_id values that could be checked
> > before it's used in a calculation below.
> 
> Oh true. Jeeez - this very much looks like exploitable code. If
> someone manages to change the boot config...
> Whatever - I'll add a check.

What I meant was it'd be nice to warn the user that the dt doesn't look right
so it's easier to debug when something blows up unexpectedly.

[...]

Thanks,
Mark.

Re: [PATCH 1/3] clocksource: sh_cmt: Add Device Tree probing

[Paul Mundt]

On Tue, Mar 05, 2013 at 09:43:05AM +0000, Mark Rutland wrote:
> Hi again,
> 
> > >> diff --git a/Documentation/devicetree/bindings/timer/renesas,sh-cmt-timer.txt b/Documentation/devicetree/bindings/timer/renesas,sh-cmt-timer.txt
> > >> new file mode 100644
> > >> index 0000000..e5ad808
> > >> --- /dev/null
> > >> +++ b/Documentation/devicetree/bindings/timer/renesas,sh-cmt-timer.txt
> > >> @@ -0,0 +1,21 @@
> > >> +* Renesas SH Mobile Compare Match Timer
> > >> +
> > >> +Required properties:
> > >> +- compatible : Should be "renesas,cmt"
> > >> +- reg : Address and length of the register set for the device
> > >> +- interrupts : Timer IRQ
> > >> +- renesas,timer-device-id : The ID of the timer device
> > >> +- renesas,timer-channel-id : The channel ID of the timer device
> > >
> > > I'm not familiar with this hardware. Could you give me a basic idea of how it's
> > > structured?
> > >
> > > Does the device have a single timer that this describes, or does the device
> > > have multiple timers, and this selects which one to use?
> > 
> > The CMT timer is built into various SoCs from Renesas, often multiple
> > instances of it. Each timer instance has 6 channels. You can select if
> > they are driven by the main clock or the real time clock (the SoCs
> > include an SH core for data processing) and can use different subscale
> > modes. It features free running mode and can fire events. The clock
> > has up to 48 bits to count. It has a DMA channel and wires enabling
> > the device to be a wakeup source. Hmm don't know what to add more.
> > It's a timer - you can check for overflows, read the counter and so
> > on.
> 
> Unless I've misunderstood, couldn't Linux choose one timer to use completely
> arbitrarily?
> 
> Why do we need to describe which one to use?
> 
Many of these CPUs have multiple timer blocks (CMT, TMU, MTU2) that sit
in different power domains, so there is often a desire to prioritize one
over another, as it will determine which sleep states are available to
us. Beyond that, many of these channels have special behavioural
constraints (only certain channels being useable by the PWM block for
example, as well as mux issues (certain channels will be tied up with
functions we care less about than others).

We have traditionally registered all of the available timer channels as
platform devices, and then left it to the clocksource/event rating to
determine which to use for what. This was based on the assumption that
later on we would do some work on the clockevents layer to dynamically
allocate the other channels that weren't being used for anything, but
this never happened, and most users managed to get away with simply
adopting hrtimers instead.

Perhaps the rating is not a detail we want to expose, but we are still
going to need some facility for defining whether a given timer is
suitable for a clocksource/clockevent or not. We also can't really encode
the rating information in the driver itself, as the placement of the
block is wholly SoC dependent.

[PATCH 1/3] clocksource: sh_cmt: Add Device Tree probing

[Paul Mundt]

On Tue, Mar 05, 2013 at 09:43:05AM +0000, Mark Rutland wrote:
> Hi again,
> 
> > >> diff --git a/Documentation/devicetree/bindings/timer/renesas,sh-cmt-timer.txt b/Documentation/devicetree/bindings/timer/renesas,sh-cmt-timer.txt
> > >> new file mode 100644
> > >> index 0000000..e5ad808
> > >> --- /dev/null
> > >> +++ b/Documentation/devicetree/bindings/timer/renesas,sh-cmt-timer.txt
> > >> @@ -0,0 +1,21 @@
> > >> +* Renesas SH Mobile Compare Match Timer
> > >> +
> > >> +Required properties:
> > >> +- compatible : Should be "renesas,cmt"
> > >> +- reg : Address and length of the register set for the device
> > >> +- interrupts : Timer IRQ
> > >> +- renesas,timer-device-id : The ID of the timer device
> > >> +- renesas,timer-channel-id : The channel ID of the timer device
> > >
> > > I'm not familiar with this hardware. Could you give me a basic idea of how it's
> > > structured?
> > >
> > > Does the device have a single timer that this describes, or does the device
> > > have multiple timers, and this selects which one to use?
> > 
> > The CMT timer is built into various SoCs from Renesas, often multiple
> > instances of it. Each timer instance has 6 channels. You can select if
> > they are driven by the main clock or the real time clock (the SoCs
> > include an SH core for data processing) and can use different subscale
> > modes. It features free running mode and can fire events. The clock
> > has up to 48 bits to count. It has a DMA channel and wires enabling
> > the device to be a wakeup source. Hmm don't know what to add more.
> > It's a timer - you can check for overflows, read the counter and so
> > on.
> 
> Unless I've misunderstood, couldn't Linux choose one timer to use completely
> arbitrarily?
> 
> Why do we need to describe which one to use?
> 
Many of these CPUs have multiple timer blocks (CMT, TMU, MTU2) that sit
in different power domains, so there is often a desire to prioritize one
over another, as it will determine which sleep states are available to
us. Beyond that, many of these channels have special behavioural
constraints (only certain channels being useable by the PWM block for
example, as well as mux issues (certain channels will be tied up with
functions we care less about than others).

We have traditionally registered all of the available timer channels as
platform devices, and then left it to the clocksource/event rating to
determine which to use for what. This was based on the assumption that
later on we would do some work on the clockevents layer to dynamically
allocate the other channels that weren't being used for anything, but
this never happened, and most users managed to get away with simply
adopting hrtimers instead.

Perhaps the rating is not a detail we want to expose, but we are still
going to need some facility for defining whether a given timer is
suitable for a clocksource/clockevent or not. We also can't really encode
the rating information in the driver itself, as the placement of the
block is wholly SoC dependent.

Re: [PATCH 1/3] clocksource: sh_cmt: Add Device Tree probing

[Bastian Hecht]

Hi Paul, hi Mark,

2013/3/6 Paul Mundt <lethal@linux-sh.org>:
> On Tue, Mar 05, 2013 at 09:43:05AM +0000, Mark Rutland wrote:
>> Hi again,
>>
>> > >> diff --git a/Documentation/devicetree/bindings/timer/renesas,sh-cmt-timer.txt b/Documentation/devicetree/bindings/timer/renesas,sh-cmt-timer.txt
>> > >> new file mode 100644
>> > >> index 0000000..e5ad808
>> > >> --- /dev/null
>> > >> +++ b/Documentation/devicetree/bindings/timer/renesas,sh-cmt-timer.txt
>> > >> @@ -0,0 +1,21 @@
>> > >> +* Renesas SH Mobile Compare Match Timer
>> > >> +
>> > >> +Required properties:
>> > >> +- compatible : Should be "renesas,cmt"
>> > >> +- reg : Address and length of the register set for the device
>> > >> +- interrupts : Timer IRQ
>> > >> +- renesas,timer-device-id : The ID of the timer device
>> > >> +- renesas,timer-channel-id : The channel ID of the timer device
>> > >
>> > > I'm not familiar with this hardware. Could you give me a basic idea of how it's
>> > > structured?
>> > >
>> > > Does the device have a single timer that this describes, or does the device
>> > > have multiple timers, and this selects which one to use?
>> >
>> > The CMT timer is built into various SoCs from Renesas, often multiple
>> > instances of it. Each timer instance has 6 channels. You can select if
>> > they are driven by the main clock or the real time clock (the SoCs
>> > include an SH core for data processing) and can use different subscale
>> > modes. It features free running mode and can fire events. The clock
>> > has up to 48 bits to count. It has a DMA channel and wires enabling
>> > the device to be a wakeup source. Hmm don't know what to add more.
>> > It's a timer - you can check for overflows, read the counter and so
>> > on.
>>
>> Unless I've misunderstood, couldn't Linux choose one timer to use completely
>> arbitrarily?
>>
>> Why do we need to describe which one to use?
>>
> Many of these CPUs have multiple timer blocks (CMT, TMU, MTU2) that sit
> in different power domains, so there is often a desire to prioritize one
> over another, as it will determine which sleep states are available to
> us. Beyond that, many of these channels have special behavioural
> constraints (only certain channels being useable by the PWM block for
> example, as well as mux issues (certain channels will be tied up with
> functions we care less about than others).
>
> We have traditionally registered all of the available timer channels as
> platform devices, and then left it to the clocksource/event rating to
> determine which to use for what. This was based on the assumption that
> later on we would do some work on the clockevents layer to dynamically
> allocate the other channels that weren't being used for anything, but
> this never happened, and most users managed to get away with simply
> adopting hrtimers instead.
>
> Perhaps the rating is not a detail we want to expose, but we are still
> going to need some facility for defining whether a given timer is
> suitable for a clocksource/clockevent or not. We also can't really encode
> the rating information in the driver itself, as the placement of the
> block is wholly SoC dependent.

Thanks for the explanation. Given this situation I wonder if we can
add the optional properties:
renesas,source-quality : The viability to use this device as a free
running clock
renesas,event-quality : Th viability to use this device as an event generator

Maybe let it run from 0-10 or from 0-255 and explain the situation in
the bindings.
I mean the environmental circumstances of the device are not really a
property of the device itself, but it doesn't need much imagination to
see it like that.

Thanks,

 Bastian

[PATCH 1/3] clocksource: sh_cmt: Add Device Tree probing

[Bastian Hecht]

Hi Paul, hi Mark,

2013/3/6 Paul Mundt <lethal@linux-sh.org>:
> On Tue, Mar 05, 2013 at 09:43:05AM +0000, Mark Rutland wrote:
>> Hi again,
>>
>> > >> diff --git a/Documentation/devicetree/bindings/timer/renesas,sh-cmt-timer.txt b/Documentation/devicetree/bindings/timer/renesas,sh-cmt-timer.txt
>> > >> new file mode 100644
>> > >> index 0000000..e5ad808
>> > >> --- /dev/null
>> > >> +++ b/Documentation/devicetree/bindings/timer/renesas,sh-cmt-timer.txt
>> > >> @@ -0,0 +1,21 @@
>> > >> +* Renesas SH Mobile Compare Match Timer
>> > >> +
>> > >> +Required properties:
>> > >> +- compatible : Should be "renesas,cmt"
>> > >> +- reg : Address and length of the register set for the device
>> > >> +- interrupts : Timer IRQ
>> > >> +- renesas,timer-device-id : The ID of the timer device
>> > >> +- renesas,timer-channel-id : The channel ID of the timer device
>> > >
>> > > I'm not familiar with this hardware. Could you give me a basic idea of how it's
>> > > structured?
>> > >
>> > > Does the device have a single timer that this describes, or does the device
>> > > have multiple timers, and this selects which one to use?
>> >
>> > The CMT timer is built into various SoCs from Renesas, often multiple
>> > instances of it. Each timer instance has 6 channels. You can select if
>> > they are driven by the main clock or the real time clock (the SoCs
>> > include an SH core for data processing) and can use different subscale
>> > modes. It features free running mode and can fire events. The clock
>> > has up to 48 bits to count. It has a DMA channel and wires enabling
>> > the device to be a wakeup source. Hmm don't know what to add more.
>> > It's a timer - you can check for overflows, read the counter and so
>> > on.
>>
>> Unless I've misunderstood, couldn't Linux choose one timer to use completely
>> arbitrarily?
>>
>> Why do we need to describe which one to use?
>>
> Many of these CPUs have multiple timer blocks (CMT, TMU, MTU2) that sit
> in different power domains, so there is often a desire to prioritize one
> over another, as it will determine which sleep states are available to
> us. Beyond that, many of these channels have special behavioural
> constraints (only certain channels being useable by the PWM block for
> example, as well as mux issues (certain channels will be tied up with
> functions we care less about than others).
>
> We have traditionally registered all of the available timer channels as
> platform devices, and then left it to the clocksource/event rating to
> determine which to use for what. This was based on the assumption that
> later on we would do some work on the clockevents layer to dynamically
> allocate the other channels that weren't being used for anything, but
> this never happened, and most users managed to get away with simply
> adopting hrtimers instead.
>
> Perhaps the rating is not a detail we want to expose, but we are still
> going to need some facility for defining whether a given timer is
> suitable for a clocksource/clockevent or not. We also can't really encode
> the rating information in the driver itself, as the placement of the
> block is wholly SoC dependent.

Thanks for the explanation. Given this situation I wonder if we can
add the optional properties:
renesas,source-quality : The viability to use this device as a free
running clock
renesas,event-quality : Th viability to use this device as an event generator

Maybe let it run from 0-10 or from 0-255 and explain the situation in
the bindings.
I mean the environmental circumstances of the device are not really a
property of the device itself, but it doesn't need much imagination to
see it like that.

Thanks,

 Bastian