* [PATCH 0/3] Add OF support to CMT driver - bindings discussion @ 2013-03-01 17:45 ` Bastian Hecht 0 siblings, 0 replies; 26+ messages in thread From: Bastian Hecht @ 2013-03-01 16:53 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 0/3] Add OF support to CMT driver - bindings discussion @ 2013-03-01 17:45 ` Bastian Hecht 0 siblings, 0 replies; 26+ messages in thread From: Bastian Hecht @ 2013-03-01 17:45 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 1/3] clocksource: sh_cmt: Add Device Tree probing 2013-03-01 17:45 ` Bastian Hecht @ 2013-03-01 17:45 ` Bastian Hecht -1 siblings, 0 replies; 26+ messages in thread From: Bastian Hecht @ 2013-03-01 16:45 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 1/3] clocksource: sh_cmt: Add Device Tree probing @ 2013-03-01 17:45 ` Bastian Hecht 0 siblings, 0 replies; 26+ messages in thread From: Bastian Hecht @ 2013-03-01 17:45 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 1/3] clocksource: sh_cmt: Add Device Tree probing 2013-03-01 17:45 ` Bastian Hecht @ 2013-03-04 4:09 ` Paul Mundt -1 siblings, 0 replies; 26+ messages in thread From: Paul Mundt @ 2013-03-04 4:09 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 1/3] clocksource: sh_cmt: Add Device Tree probing @ 2013-03-04 4:09 ` Paul Mundt 0 siblings, 0 replies; 26+ messages in thread From: Paul Mundt @ 2013-03-04 4:09 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/3] clocksource: sh_cmt: Add Device Tree probing 2013-03-04 4:09 ` Paul Mundt @ 2013-03-04 15:51 ` Bastian Hecht -1 siblings, 0 replies; 26+ messages in thread From: Bastian Hecht @ 2013-03-04 15:51 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 1/3] clocksource: sh_cmt: Add Device Tree probing @ 2013-03-04 15:51 ` Bastian Hecht 0 siblings, 0 replies; 26+ messages in thread From: Bastian Hecht @ 2013-03-04 15:51 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/3] clocksource: sh_cmt: Add Device Tree probing 2013-03-01 17:45 ` Bastian Hecht @ 2013-03-04 10:03 ` Mark Rutland -1 siblings, 0 replies; 26+ messages in thread From: Mark Rutland @ 2013-03-04 10:03 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 1/3] clocksource: sh_cmt: Add Device Tree probing @ 2013-03-04 10:03 ` Mark Rutland 0 siblings, 0 replies; 26+ messages in thread From: Mark Rutland @ 2013-03-04 10:03 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/3] clocksource: sh_cmt: Add Device Tree probing 2013-03-04 10:03 ` Mark Rutland @ 2013-03-04 16:29 ` Bastian Hecht -1 siblings, 0 replies; 26+ messages in thread From: Bastian Hecht @ 2013-03-04 16:29 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 1/3] clocksource: sh_cmt: Add Device Tree probing @ 2013-03-04 16:29 ` Bastian Hecht 0 siblings, 0 replies; 26+ messages in thread From: Bastian Hecht @ 2013-03-04 16:29 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/3] clocksource: sh_cmt: Add Device Tree probing 2013-03-04 16:29 ` Bastian Hecht @ 2013-03-05 9:43 ` Mark Rutland -1 siblings, 0 replies; 26+ messages in thread From: Mark Rutland @ 2013-03-05 9:43 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 1/3] clocksource: sh_cmt: Add Device Tree probing @ 2013-03-05 9:43 ` Mark Rutland 0 siblings, 0 replies; 26+ messages in thread From: Mark Rutland @ 2013-03-05 9:43 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/3] clocksource: sh_cmt: Add Device Tree probing 2013-03-05 9:43 ` Mark Rutland @ 2013-03-06 1:06 ` Paul Mundt -1 siblings, 0 replies; 26+ messages in thread From: Paul Mundt @ 2013-03-06 1:06 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 1/3] clocksource: sh_cmt: Add Device Tree probing @ 2013-03-06 1:06 ` Paul Mundt 0 siblings, 0 replies; 26+ messages in thread From: Paul Mundt @ 2013-03-06 1:06 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/3] clocksource: sh_cmt: Add Device Tree probing 2013-03-06 1:06 ` Paul Mundt @ 2013-03-06 13:44 ` Bastian Hecht -1 siblings, 0 replies; 26+ messages in thread From: Bastian Hecht @ 2013-03-06 13:44 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 1/3] clocksource: sh_cmt: Add Device Tree probing @ 2013-03-06 13:44 ` Bastian Hecht 0 siblings, 0 replies; 26+ messages in thread From: Bastian Hecht @ 2013-03-06 13:44 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 3/3] ARM: mach-shmobile: sh73a0: Setup the timer device CMT10 using DT 2013-03-01 17:45 ` Bastian Hecht @ 2013-03-01 17:45 ` Bastian Hecht -1 siblings, 0 replies; 26+ messages in thread From: Bastian Hecht @ 2013-03-01 16:45 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 3/3] ARM: mach-shmobile: sh73a0: Setup the timer device CMT10 using DT @ 2013-03-01 17:45 ` Bastian Hecht 0 siblings, 0 replies; 26+ messages in thread From: Bastian Hecht @ 2013-03-01 17:45 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 3/3] ARM: mach-shmobile: sh73a0: Setup the timer device CMT10 using DT 2013-03-01 17:45 ` Bastian Hecht @ 2013-03-01 19:17 ` Sergei Shtylyov -1 siblings, 0 replies; 26+ messages in thread From: Sergei Shtylyov @ 2013-03-01 18:16 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 3/3] ARM: mach-shmobile: sh73a0: Setup the timer device CMT10 using DT @ 2013-03-01 19:17 ` Sergei Shtylyov 0 siblings, 0 replies; 26+ messages in thread From: Sergei Shtylyov @ 2013-03-01 19:17 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 3/3] ARM: mach-shmobile: sh73a0: Setup the timer device CMT10 using DT 2013-03-01 19:17 ` Sergei Shtylyov @ 2013-03-04 15:46 ` Bastian Hecht -1 siblings, 0 replies; 26+ messages in thread From: Bastian Hecht @ 2013-03-04 15:46 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 3/3] ARM: mach-shmobile: sh73a0: Setup the timer device CMT10 using DT @ 2013-03-04 15:46 ` Bastian Hecht 0 siblings, 0 replies; 26+ messages in thread From: Bastian Hecht @ 2013-03-04 15:46 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 2/3] ARM: mach-shmobile: sh73a0: Add CMT DT name to clock list 2013-03-01 17:45 ` Bastian Hecht @ 2013-03-01 17:45 ` Bastian Hecht -1 siblings, 0 replies; 26+ messages in thread From: Bastian Hecht @ 2013-03-01 16:52 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 2/3] ARM: mach-shmobile: sh73a0: Add CMT DT name to clock list @ 2013-03-01 17:45 ` Bastian Hecht 0 siblings, 0 replies; 26+ messages in thread From: Bastian Hecht @ 2013-03-01 17:45 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
end of thread, other threads:[~2013-03-06 13:44 UTC | newest] Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-03-01 16:53 [PATCH 0/3] Add OF support to CMT driver - bindings discussion Bastian Hecht 2013-03-01 17:45 ` Bastian Hecht 2013-03-01 16:45 ` [PATCH 1/3] clocksource: sh_cmt: Add Device Tree probing Bastian Hecht 2013-03-01 17:45 ` Bastian Hecht 2013-03-04 4:09 ` Paul Mundt 2013-03-04 4:09 ` Paul Mundt 2013-03-04 15:51 ` Bastian Hecht 2013-03-04 15:51 ` Bastian Hecht 2013-03-04 10:03 ` Mark Rutland 2013-03-04 10:03 ` Mark Rutland 2013-03-04 16:29 ` Bastian Hecht 2013-03-04 16:29 ` Bastian Hecht 2013-03-05 9:43 ` Mark Rutland 2013-03-05 9:43 ` Mark Rutland 2013-03-06 1:06 ` Paul Mundt 2013-03-06 1:06 ` Paul Mundt 2013-03-06 13:44 ` Bastian Hecht 2013-03-06 13:44 ` Bastian Hecht 2013-03-01 16:45 ` [PATCH 3/3] ARM: mach-shmobile: sh73a0: Setup the timer device CMT10 using DT Bastian Hecht 2013-03-01 17:45 ` Bastian Hecht 2013-03-01 18:16 ` Sergei Shtylyov 2013-03-01 19:17 ` Sergei Shtylyov 2013-03-04 15:46 ` Bastian Hecht 2013-03-04 15:46 ` Bastian Hecht 2013-03-01 16:52 ` [PATCH 2/3] ARM: mach-shmobile: sh73a0: Add CMT DT name to clock list Bastian Hecht 2013-03-01 17:45 ` Bastian Hecht
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.