| Message ID | 1362159932-18533-2-git-send-email-hechtb+renesas@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
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. -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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), } };
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