| Message ID | 1392339605-20691-24-git-send-email-laurent.pinchart+renesas@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Feb 14, 2014 at 2:00 AM, Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> wrote: > +++ b/Documentation/devicetree/bindings/timer/renesas,cmt.txt > @@ -0,0 +1,75 @@ > +* Renesas R-Car Compare Match Timer (CMT) > + > +The CMT is a multi-channel 16/32/48-bit timer/counter with configurable clock 16-bit is mentioned here ... > +inputs and programmable compare match. > + > +Channels share hadware resources but their counter and compare match value are hardware > +independent. A particular CMT instance can implement only a subset of the > +channels supported by the CMT model. Channels indices start from 0 and are Channel indices > +consecutive. > + > +Required Properties: > + > + - compatible: must contain one of the following. ... why not add "renesas,cmt-16" here (and one extra line in the actual driver), while you're at it? > + - "renesas,cmt-32" for the 32-bit CMT > + (CMT0 on sh7372, sh73a0 and r8a7740) > + - "renesas,cmt-32-fast" for the 32-bit CMT with fast clock support > + (CMT[234] on sh7372, sh73a0 and r8a7740) > + - "renasas,cmt-48" for the 48-bit CMT > + (CMT1 on sh7372, sh73a0 and r8a7740) > + - "renesas,cmt-48-gen2" for the second generation 48-bit CMT > + (CMT[01] on r8a73a4, r8a7790 and r8a7791) Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Fri, Feb 14, 2014 at 01:00:01AM +0000, Laurent Pinchart wrote: > Cc: devicetree@vger.kernel.org > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > --- > .../devicetree/bindings/timer/renesas,cmt.txt | 75 +++++++++++++++ > drivers/clocksource/sh_cmt.c | 104 +++++++++++++++++---- > 2 files changed, 160 insertions(+), 19 deletions(-) > create mode 100644 Documentation/devicetree/bindings/timer/renesas,cmt.txt > > diff --git a/Documentation/devicetree/bindings/timer/renesas,cmt.txt b/Documentation/devicetree/bindings/timer/renesas,cmt.txt > new file mode 100644 > index 0000000..28d4ab5 > --- /dev/null > +++ b/Documentation/devicetree/bindings/timer/renesas,cmt.txt > @@ -0,0 +1,75 @@ > +* Renesas R-Car Compare Match Timer (CMT) > + > +The CMT is a multi-channel 16/32/48-bit timer/counter with configurable clock > +inputs and programmable compare match. > + > +Channels share hadware resources but their counter and compare match value are > +independent. A particular CMT instance can implement only a subset of the > +channels supported by the CMT model. Channels indices start from 0 and are > +consecutive. > + > +Required Properties: > + > + - compatible: must contain one of the following. > + - "renesas,cmt-32" for the 32-bit CMT > + (CMT0 on sh7372, sh73a0 and r8a7740) > + - "renesas,cmt-32-fast" for the 32-bit CMT with fast clock support > + (CMT[234] on sh7372, sh73a0 and r8a7740) > + - "renasas,cmt-48" for the 48-bit CMT > + (CMT1 on sh7372, sh73a0 and r8a7740) > + - "renesas,cmt-48-gen2" for the second generation 48-bit CMT > + (CMT[01] on r8a73a4, r8a7790 and r8a7791) > + > + - reg: base address and length of the registers block for the timer module. > + - interrupt-parent, interrupts: interrupt-specifier for the timer, one per > + channel. It might make more sense to describe the interrupt on the channel subnode. It makes it far clearer which channel has which interrupt. > + - clocks: phandle and clock-specifier pair for the functional clock. > + - clock-names: must be "fck". It would be nice to define the list once: - clocks: A list of phandle + clock-specifier pairs, one for each entry in clock-names. - clock-names: Should contain "fck" for the functional clock. > + > + - #address-cells: must be 1 > + - #size-cells: must be 0 > + > + - renesas,channels-mask: integer bitmask of the channels implemented by the > + timer instance. This is implied by the presence of a subnode. Either remove this or the subnodes. > + > + > +Each channel is described by a sub-node named "channel@<idx>", where <idx> is > +the channel index. > + > +Channels Required Properties: > + > + - reg: the channel index. > + > +Channels Optional Properties: > + > + - clock-source-rating: rating of the timer as a clock source device. > + - clock-event-rating: rating of the timer as a clock event device. This feels like a leak of Linux internals. Why do you need this? > + > + > +Example: R8A7790 (R-Car H2) CMT0 node > + > + CMT0 on R8A7790 implements hardware channels 5 and 6 only and names > + them channels 0 and 1 in the documentation. > + > + cmt0: timer@ffca0000 { > + compatible = "renesas,cmt-48-gen2"; > + reg = <0 0xffca0000 0 0x1004>; > + interrupt-parent = <&gic>; > + interrupts = <0 142 IRQ_TYPE_LEVEL_HIGH>, > + <0 142 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&mstp1_clks R8A7790_CLK_CMT0>; > + > + #address-cells = <1>; > + #size-cells = <0>; > + > + renesas,channels-mask = <0x60>; > + > + channel@0 { > + reg = <0>; > + clock-event-rating = <80>; > + }; > + channel@0 { > + reg = <0>; > + clock-source-rating = <80>; > + }; Aaargh. Use the _real_ channel IDs for the reg proeprties and get rid of the mask. It's pointlessly confusing. Thanks, Mark
Hi Geert, Thank you for the review. On Friday 14 February 2014 10:18:58 Geert Uytterhoeven wrote: > On Fri, Feb 14, 2014 at 2:00 AM, Laurent Pinchart wrote: > > +++ b/Documentation/devicetree/bindings/timer/renesas,cmt.txt > > @@ -0,0 +1,75 @@ > > +* Renesas R-Car Compare Match Timer (CMT) > > + > > +The CMT is a multi-channel 16/32/48-bit timer/counter with configurable > > clock > 16-bit is mentioned here ... > > > +inputs and programmable compare match. > > + > > +Channels share hadware resources but their counter and compare match > > value are > > hardware > > > +independent. A particular CMT instance can implement only a subset of the > > +channels supported by the CMT model. Channels indices start from 0 and > > are > > Channel indices > > > +consecutive. > > + > > +Required Properties: > > + > > + - compatible: must contain one of the following. > > ... why not add "renesas,cmt-16" here (and one extra line in the actual > driver), while you're at it? Because the 16-bit variant is only used on SuperH SoCs, so there's not much point in adding DT bindings for it. > > + - "renesas,cmt-32" for the 32-bit CMT > > + (CMT0 on sh7372, sh73a0 and r8a7740) > > + - "renesas,cmt-32-fast" for the 32-bit CMT with fast clock support > > + (CMT[234] on sh7372, sh73a0 and r8a7740) > > + - "renasas,cmt-48" for the 48-bit CMT > > + (CMT1 on sh7372, sh73a0 and r8a7740) > > + - "renesas,cmt-48-gen2" for the second generation 48-bit CMT > > + (CMT[01] on r8a73a4, r8a7790 and r8a7791)
Hi Mark, Thank you for the review. On Friday 14 February 2014 10:58:22 Mark Rutland wrote: > On Fri, Feb 14, 2014 at 01:00:01AM +0000, Laurent Pinchart wrote: > > Cc: devicetree@vger.kernel.org > > Signed-off-by: Laurent Pinchart > > <laurent.pinchart+renesas@ideasonboard.com> > > --- > > > > .../devicetree/bindings/timer/renesas,cmt.txt | 75 +++++++++++++++ > > drivers/clocksource/sh_cmt.c | 104 +++++++++++++--- > > 2 files changed, 160 insertions(+), 19 deletions(-) > > create mode 100644 > > Documentation/devicetree/bindings/timer/renesas,cmt.txt > > > > diff --git a/Documentation/devicetree/bindings/timer/renesas,cmt.txt > > b/Documentation/devicetree/bindings/timer/renesas,cmt.txt new file mode > > 100644 > > index 0000000..28d4ab5 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/timer/renesas,cmt.txt > > @@ -0,0 +1,75 @@ > > +* Renesas R-Car Compare Match Timer (CMT) > > + > > +The CMT is a multi-channel 16/32/48-bit timer/counter with configurable > > clock > > +inputs and programmable compare match. > > + > > +Channels share hadware resources but their counter and compare match > > value are > > +independent. A particular CMT instance can implement only a subset of the > > +channels supported by the CMT model. Channels indices start from 0 and > > are > > +consecutive. > > + > > +Required Properties: > > + > > + - compatible: must contain one of the following. > > + - "renesas,cmt-32" for the 32-bit CMT > > + (CMT0 on sh7372, sh73a0 and r8a7740) > > + - "renesas,cmt-32-fast" for the 32-bit CMT with fast clock support > > + (CMT[234] on sh7372, sh73a0 and r8a7740) > > + - "renasas,cmt-48" for the 48-bit CMT > > + (CMT1 on sh7372, sh73a0 and r8a7740) > > + - "renesas,cmt-48-gen2" for the second generation 48-bit CMT > > + (CMT[01] on r8a73a4, r8a7790 and r8a7791) > > + > > + - reg: base address and length of the registers block for the timer > > module. > > + - interrupt-parent, interrupts: interrupt-specifier for the timer, one > > per > > + channel. > > It might make more sense to describe the interrupt on the channel > subnode. It makes it far clearer which channel has which interrupt. That's a good point. I'm relying on platform_get_irq() which won't support that usage, but I can switch to of_irq_to_resource() instead. > > + - clocks: phandle and clock-specifier pair for the functional clock. > > + - clock-names: must be "fck". > > It would be nice to define the list once: > > - clocks: A list of phandle + clock-specifier pairs, one for each entry > in clock-names. > - clock-names: Should contain "fck" for the functional clock. OK. > > + > > + - #address-cells: must be 1 > > + - #size-cells: must be 0 > > + > > + - renesas,channels-mask: integer bitmask of the channels implemented by > > the > > + timer instance. > > This is implied by the presence of a subnode. Either remove this or the > subnodes. > > > + > > + > > +Each channel is described by a sub-node named "channel@<idx>", where > > <idx> is +the channel index. > > + > > +Channels Required Properties: > > + > > + - reg: the channel index. > > + > > +Channels Optional Properties: > > + > > + - clock-source-rating: rating of the timer as a clock source device. > > + - clock-event-rating: rating of the timer as a clock event device. > > This feels like a leak of Linux internals. Why do you need this? You're right, it is. The clock source and clock event ratings are currently configured through platform data, I'll need to find a way to compute them in the driver instead. There's still one piece of Linux-specific data I need though, as I need to specify for each channel whether to use it as a clock source device, a clock event device, both of them or none. That's configuration information that needs to be provided somehow. > > + > > + > > +Example: R8A7790 (R-Car H2) CMT0 node > > + > > + CMT0 on R8A7790 implements hardware channels 5 and 6 only and names > > + them channels 0 and 1 in the documentation. > > + > > + cmt0: timer@ffca0000 { > > + compatible = "renesas,cmt-48-gen2"; > > + reg = <0 0xffca0000 0 0x1004>; > > + interrupt-parent = <&gic>; > > + interrupts = <0 142 IRQ_TYPE_LEVEL_HIGH>, > > + <0 142 IRQ_TYPE_LEVEL_HIGH>; > > + clocks = <&mstp1_clks R8A7790_CLK_CMT0>; > > + > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + renesas,channels-mask = <0x60>; > > + > > + channel@0 { > > + reg = <0>; > > + clock-event-rating = <80>; > > + }; > > + channel@0 { > > + reg = <0>; > > + clock-source-rating = <80>; > > + }; > > Aaargh. Use the _real_ channel IDs for the reg proeprties and get rid of > the mask. It's pointlessly confusing. There's two real channel IDs. One of them is the value used in the hardware implementation (5 and 6 in this case, used to compute the channel registers block address) and the other one is the value used throughout the datasheet, 0 and 1 in this case. The later is used by the driver to reference the correct interrupt, which won't be needed anymore when referencing interrupts in the channel subnodes directly. It's also used to print messages to the kernel log and match the channel numbers specified in the datasheets. I could use the hardware channel number instead, but that might become confusing.
On Fri, Feb 14, 2014 at 04:53:08PM +0100, Laurent Pinchart wrote: > On Friday 14 February 2014 10:58:22 Mark Rutland wrote: > > On Fri, Feb 14, 2014 at 01:00:01AM +0000, Laurent Pinchart wrote: > > > +Channels Optional Properties: > > > + > > > + - clock-source-rating: rating of the timer as a clock source device. > > > + - clock-event-rating: rating of the timer as a clock event device. > > > > This feels like a leak of Linux internals. Why do you need this? > > You're right, it is. The clock source and clock event ratings are currently > configured through platform data, I'll need to find a way to compute them in > the driver instead. > > There's still one piece of Linux-specific data I need though, as I need to > specify for each channel whether to use it as a clock source device, a clock > event device, both of them or none. That's configuration information that > needs to be provided somehow. Are all the channels equally capable? We had this problem for the cadence_ttc timer used on Zynq, and decided to just statically allocate the first timer to be the clocksource, and the second to be the clockevent. Also, should the rating really be user configurable?
On Sat, Feb 15, 2014 at 12:53 AM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > On Friday 14 February 2014 10:58:22 Mark Rutland wrote: >> On Fri, Feb 14, 2014 at 01:00:01AM +0000, Laurent Pinchart wrote: >> > +Channels Optional Properties: >> > + >> > + - clock-source-rating: rating of the timer as a clock source device. >> > + - clock-event-rating: rating of the timer as a clock event device. >> >> This feels like a leak of Linux internals. Why do you need this? > > You're right, it is. The clock source and clock event ratings are currently > configured through platform data, I'll need to find a way to compute them in > the driver instead. That would be very good! > There's still one piece of Linux-specific data I need though, as I need to > specify for each channel whether to use it as a clock source device, a clock > event device, both of them or none. That's configuration information that > needs to be provided somehow. I think you can decide clock source or clock event assignment based on number of channels available. If you have only a single channel then both clock event and clock source need to be supported. Otherwise use one channel for clock source and the rest for clock events. This is probably out of scope for this DT conversion, but it would be neat if you somehow could specify the CPU affinity for a channel to tie a clock event to an individual CPU core. This would make a a per-cpu timer unless I'm mistaken. But that's more of a software policy than anything else. Thanks, / magnus
Hi Magnus, On Saturday 15 February 2014 01:01:30 Magnus Damm wrote: > On Sat, Feb 15, 2014 at 12:53 AM, Laurent Pinchart wrote: > > On Friday 14 February 2014 10:58:22 Mark Rutland wrote: > >> On Fri, Feb 14, 2014 at 01:00:01AM +0000, Laurent Pinchart wrote: > >> > +Channels Optional Properties: > >> > + > >> > + - clock-source-rating: rating of the timer as a clock source device. > >> > + - clock-event-rating: rating of the timer as a clock event device. > >> > >> This feels like a leak of Linux internals. Why do you need this? > > > > You're right, it is. The clock source and clock event ratings are > > currently configured through platform data, I'll need to find a way to > > compute them in the driver instead. > > That would be very good! Any pointer would be appreciated :-) How did you compute the various ratings used in platform data all over the place ? > > There's still one piece of Linux-specific data I need though, as I need to > > specify for each channel whether to use it as a clock source device, a > > clock event device, both of them or none. That's configuration > > information that needs to be provided somehow. > > I think you can decide clock source or clock event assignment based on > number of channels available. If you have only a single channel then both > clock event and clock source need to be supported. Otherwise use one channel > for clock source and the rest for clock events. That won't match the current situation. Look at CMT0 in r8a7790 for instance. There's two hardware channels available, and we only use the first one, for clock events only. > This is probably out of scope for this DT conversion, but it would be neat > if you somehow could specify the CPU affinity for a channel to tie a clock > event to an individual CPU core. This would make a a per-cpu timer unless > I'm mistaken. But that's more of a software policy than anything else. Yes, that's a configuration that needs to be specified somewhere. I don't know where though.
Hi Josh, On Friday 14 February 2014 09:59:51 Josh Cartwright wrote: > On Fri, Feb 14, 2014 at 04:53:08PM +0100, Laurent Pinchart wrote: > > On Friday 14 February 2014 10:58:22 Mark Rutland wrote: > > > On Fri, Feb 14, 2014 at 01:00:01AM +0000, Laurent Pinchart wrote: > > > > +Channels Optional Properties: > > > > + > > > > + - clock-source-rating: rating of the timer as a clock source > > > > device. > > > > + - clock-event-rating: rating of the timer as a clock event device. > > > > > > This feels like a leak of Linux internals. Why do you need this? > > > > You're right, it is. The clock source and clock event ratings are > > currently configured through platform data, I'll need to find a way to > > compute them in the driver instead. > > > > There's still one piece of Linux-specific data I need though, as I need to > > specify for each channel whether to use it as a clock source device, a > > clock event device, both of them or none. That's configuration > > information that needs to be provided somehow. > > Are all the channels equally capable? We had this problem for the > cadence_ttc timer used on Zynq, and decided to just statically allocate > the first timer to be the clocksource, and the second to be the > clockevent. No, they're not. The channels can be implemented with different counter widths, different available prescalers and source clocks and different power management features (not all of them are capable to run in all sleep states). > Also, should the rating really be user configurable? Probably not. I suppose the rating should be computed by the driver based on the source clock frequency, prescaler and counter width. Any help there would be very appreciated, I'm pretty new to clock source and clock event devices.
Hi Laurent, On Sat, Feb 15, 2014 at 1:12 AM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Hi Magnus, > > On Saturday 15 February 2014 01:01:30 Magnus Damm wrote: >> On Sat, Feb 15, 2014 at 12:53 AM, Laurent Pinchart wrote: >> > On Friday 14 February 2014 10:58:22 Mark Rutland wrote: >> >> On Fri, Feb 14, 2014 at 01:00:01AM +0000, Laurent Pinchart wrote: >> >> > +Channels Optional Properties: >> >> > + >> >> > + - clock-source-rating: rating of the timer as a clock source device. >> >> > + - clock-event-rating: rating of the timer as a clock event device. >> >> >> >> This feels like a leak of Linux internals. Why do you need this? >> > >> > You're right, it is. The clock source and clock event ratings are >> > currently configured through platform data, I'll need to find a way to >> > compute them in the driver instead. >> >> That would be very good! > > Any pointer would be appreciated :-) How did you compute the various ratings > used in platform data all over the place ? Historically we used the rating to select between CMT and TMU. For clock sources I suppose you also have the jiffy rating to consider. And for the SMP parts we have ARM IP for TWD and arch timers that have their ratings too. So you need to check all the timers on a particular system and consider what you want to have operating by default. The ARM IP timers should be preferred if available. For clock sources the rule is probably the higher resolution the better. >> > There's still one piece of Linux-specific data I need though, as I need to >> > specify for each channel whether to use it as a clock source device, a >> > clock event device, both of them or none. That's configuration >> > information that needs to be provided somehow. >> >> I think you can decide clock source or clock event assignment based on >> number of channels available. If you have only a single channel then both >> clock event and clock source need to be supported. Otherwise use one channel >> for clock source and the rest for clock events. > > That won't match the current situation. Look at CMT0 in r8a7790 for instance. > There's two hardware channels available, and we only use the first one, for > clock events only. You are correct. The reason for that is that the CMT driver today is optimized for combined clock event and clock source operation. Historically the hardware it initially was written for (sh-mobile on the SH arch) only had a single timer channel so combined operation was required for tickless to work. But since you're asking how to allocate channels then I propose checking numbers of channels available and go from there. With that the r8a7790 support can only get better. =) >> This is probably out of scope for this DT conversion, but it would be neat >> if you somehow could specify the CPU affinity for a channel to tie a clock >> event to an individual CPU core. This would make a a per-cpu timer unless >> I'm mistaken. But that's more of a software policy than anything else. > > Yes, that's a configuration that needs to be specified somewhere. I don't know > where though. As long as you have per-channel interrupts described in DT you can probably handle this in a generic way in the driver. Thanks, / magnus
Hi Magnus, On Saturday 15 February 2014 02:22:00 Magnus Damm wrote: > On Sat, Feb 15, 2014 at 1:12 AM, Laurent Pinchart wrote: > > On Saturday 15 February 2014 01:01:30 Magnus Damm wrote: > >> On Sat, Feb 15, 2014 at 12:53 AM, Laurent Pinchart wrote: > >> > On Friday 14 February 2014 10:58:22 Mark Rutland wrote: > >> >> On Fri, Feb 14, 2014 at 01:00:01AM +0000, Laurent Pinchart wrote: > >> >> > +Channels Optional Properties: > >> >> > + > >> >> > + - clock-source-rating: rating of the timer as a clock source > >> >> > device. > >> >> > + - clock-event-rating: rating of the timer as a clock event > >> >> > device. > >> >> > >> >> This feels like a leak of Linux internals. Why do you need this? > >> > > >> > You're right, it is. The clock source and clock event ratings are > >> > currently configured through platform data, I'll need to find a way to > >> > compute them in the driver instead. > >> > >> That would be very good! > > > > Any pointer would be appreciated :-) How did you compute the various > > ratings used in platform data all over the place ? > > Historically we used the rating to select between CMT and TMU. For > clock sources I suppose you also have the jiffy rating to consider. > And for the SMP parts we have ARM IP for TWD and arch timers that have > their ratings too. So you need to check all the timers on a particular > system and consider what you want to have operating by default. The > ARM IP timers should be preferred if available. For clock sources the > rule is probably the higher resolution the better. > > >> > There's still one piece of Linux-specific data I need though, as I need > >> > to specify for each channel whether to use it as a clock source device, > >> > a clock event device, both of them or none. That's configuration > >> > information that needs to be provided somehow. > >> > >> I think you can decide clock source or clock event assignment based on > >> number of channels available. If you have only a single channel then both > >> clock event and clock source need to be supported. Otherwise use one > >> channel for clock source and the rest for clock events. > > > > That won't match the current situation. Look at CMT0 in r8a7790 for > > instance. There's two hardware channels available, and we only use the > > first one, for clock events only. > > You are correct. The reason for that is that the CMT driver today is > optimized for combined clock event and clock source operation. > > Historically the hardware it initially was written for (sh-mobile on > the SH arch) only had a single timer channel so combined operation was > required for tickless to work. But since you're asking how to allocate > channels then I propose checking numbers of channels available and go > from there. With that the r8a7790 support can only get better. =) > > >> This is probably out of scope for this DT conversion, but it would be > >> neat if you somehow could specify the CPU affinity for a channel to tie a > >> clock event to an individual CPU core. This would make a a per-cpu timer > >> unless I'm mistaken. But that's more of a software policy than anything > >> else. > > > > Yes, that's a configuration that needs to be specified somewhere. I don't > > know where though. > > As long as you have per-channel interrupts described in DT you can > probably handle this in a generic way in the driver. But how do we decide whether to use a single timer channel or one channel per CPU ? Will the kernel use one clock event device per CPU automatically ? I have to confess I have no idea how this works.
Hi Laurent, On Mon, Feb 17, 2014 at 10:45 AM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Hi Magnus, > > On Saturday 15 February 2014 02:22:00 Magnus Damm wrote: >> On Sat, Feb 15, 2014 at 1:12 AM, Laurent Pinchart wrote: >> > On Saturday 15 February 2014 01:01:30 Magnus Damm wrote: >> >> On Sat, Feb 15, 2014 at 12:53 AM, Laurent Pinchart wrote: >> >> > On Friday 14 February 2014 10:58:22 Mark Rutland wrote: >> >> >> On Fri, Feb 14, 2014 at 01:00:01AM +0000, Laurent Pinchart wrote: >> >> >> > +Channels Optional Properties: >> >> >> > + >> >> >> > + - clock-source-rating: rating of the timer as a clock source >> >> >> > device. >> >> >> > + - clock-event-rating: rating of the timer as a clock event >> >> >> > device. >> >> >> >> >> >> This feels like a leak of Linux internals. Why do you need this? >> >> > >> >> > You're right, it is. The clock source and clock event ratings are >> >> > currently configured through platform data, I'll need to find a way to >> >> > compute them in the driver instead. >> >> >> >> That would be very good! >> > >> > Any pointer would be appreciated :-) How did you compute the various >> > ratings used in platform data all over the place ? >> >> Historically we used the rating to select between CMT and TMU. For >> clock sources I suppose you also have the jiffy rating to consider. >> And for the SMP parts we have ARM IP for TWD and arch timers that have >> their ratings too. So you need to check all the timers on a particular >> system and consider what you want to have operating by default. The >> ARM IP timers should be preferred if available. For clock sources the >> rule is probably the higher resolution the better. >> >> >> > There's still one piece of Linux-specific data I need though, as I need >> >> > to specify for each channel whether to use it as a clock source device, >> >> > a clock event device, both of them or none. That's configuration >> >> > information that needs to be provided somehow. >> >> >> >> I think you can decide clock source or clock event assignment based on >> >> number of channels available. If you have only a single channel then both >> >> clock event and clock source need to be supported. Otherwise use one >> >> channel for clock source and the rest for clock events. >> > >> > That won't match the current situation. Look at CMT0 in r8a7790 for >> > instance. There's two hardware channels available, and we only use the >> > first one, for clock events only. >> >> You are correct. The reason for that is that the CMT driver today is >> optimized for combined clock event and clock source operation. >> >> Historically the hardware it initially was written for (sh-mobile on >> the SH arch) only had a single timer channel so combined operation was >> required for tickless to work. But since you're asking how to allocate >> channels then I propose checking numbers of channels available and go >> from there. With that the r8a7790 support can only get better. =) >> >> >> This is probably out of scope for this DT conversion, but it would be >> >> neat if you somehow could specify the CPU affinity for a channel to tie a >> >> clock event to an individual CPU core. This would make a a per-cpu timer >> >> unless I'm mistaken. But that's more of a software policy than anything >> >> else. >> > >> > Yes, that's a configuration that needs to be specified somewhere. I don't >> > know where though. >> >> As long as you have per-channel interrupts described in DT you can >> probably handle this in a generic way in the driver. > > But how do we decide whether to use a single timer channel or one channel per > CPU ? Will the kernel use one clock event device per CPU automatically ? I > have to confess I have no idea how this works. I guess that's the tricky bit about timer support, it is a mix of hardware description and software configuration. So it sounds to me that we need some kind of software configuration interface. But it can probably be considered when/if we add such kind of support to the driver. Probably out of scope for now. Regardless it seems to me that the hardware description in DT doesn't need to care about this. Cheers, / magnus
Hi Magnus, On Monday 17 February 2014 10:48:55 Magnus Damm wrote: > On Mon, Feb 17, 2014 at 10:45 AM, Laurent Pinchart wrote: > > On Saturday 15 February 2014 02:22:00 Magnus Damm wrote: > >> On Sat, Feb 15, 2014 at 1:12 AM, Laurent Pinchart wrote: > >> > On Saturday 15 February 2014 01:01:30 Magnus Damm wrote: > >> >> On Sat, Feb 15, 2014 at 12:53 AM, Laurent Pinchart wrote: > >> >> > On Friday 14 February 2014 10:58:22 Mark Rutland wrote: > >> >> >> On Fri, Feb 14, 2014 at 01:00:01AM +0000, Laurent Pinchart wrote: > >> >> >> > +Channels Optional Properties: > >> >> >> > + > >> >> >> > + - clock-source-rating: rating of the timer as a clock source > >> >> >> > device. > >> >> >> > + - clock-event-rating: rating of the timer as a clock event > >> >> >> > device. > >> >> >> > >> >> >> This feels like a leak of Linux internals. Why do you need this? > >> >> > > >> >> > You're right, it is. The clock source and clock event ratings are > >> >> > currently configured through platform data, I'll need to find a way > >> >> > to compute them in the driver instead. > >> >> > >> >> That would be very good! > >> > > >> > Any pointer would be appreciated :-) How did you compute the various > >> > ratings used in platform data all over the place ? > >> > >> Historically we used the rating to select between CMT and TMU. For > >> clock sources I suppose you also have the jiffy rating to consider. > >> And for the SMP parts we have ARM IP for TWD and arch timers that have > >> their ratings too. So you need to check all the timers on a particular > >> system and consider what you want to have operating by default. The > >> ARM IP timers should be preferred if available. For clock sources the > >> rule is probably the higher resolution the better. > >> > >> >> > There's still one piece of Linux-specific data I need though, as I > >> >> > need to specify for each channel whether to use it as a clock source > >> >> > device, a clock event device, both of them or none. That's > >> >> > configuration information that needs to be provided somehow. > >> >> > >> >> I think you can decide clock source or clock event assignment based on > >> >> number of channels available. If you have only a single channel then > >> >> both clock event and clock source need to be supported. Otherwise use > >> >> one channel for clock source and the rest for clock events. > >> > > >> > That won't match the current situation. Look at CMT0 in r8a7790 for > >> > instance. There's two hardware channels available, and we only use the > >> > first one, for clock events only. > >> > >> You are correct. The reason for that is that the CMT driver today is > >> optimized for combined clock event and clock source operation. > >> > >> Historically the hardware it initially was written for (sh-mobile on > >> the SH arch) only had a single timer channel so combined operation was > >> required for tickless to work. But since you're asking how to allocate > >> channels then I propose checking numbers of channels available and go > >> from there. With that the r8a7790 support can only get better. =) > >> > >> >> This is probably out of scope for this DT conversion, but it would be > >> >> neat if you somehow could specify the CPU affinity for a channel to > >> >> tie a clock event to an individual CPU core. This would make a a per- > >> >> cpu timer unless I'm mistaken. But that's more of a software policy > >> >> than anything else. > >> > > >> > Yes, that's a configuration that needs to be specified somewhere. I > >> > don't know where though. > >> > >> As long as you have per-channel interrupts described in DT you can > >> probably handle this in a generic way in the driver. > > > > But how do we decide whether to use a single timer channel or one channel > > per CPU ? Will the kernel use one clock event device per CPU > > automatically ? I have to confess I have no idea how this works. > > I guess that's the tricky bit about timer support, it is a mix of > hardware description and software configuration. So it sounds to me > that we need some kind of software configuration interface. But it can > probably be considered when/if we add such kind of support to the > driver. Probably out of scope for now. > > Regardless it seems to me that the hardware description in DT doesn't > need to care about this. I'll revisit that later. Per-CPU timers is not a high priority for now, so I'll just return -EAGAIN :-) Nonetheless, specifying which timer channel to use as a clock source and which channel to use as a clock event device might need to be specified in DT (or somewhere else, but I'm not sure what other options we have here).
Hi Laurent, On Tue, Feb 18, 2014 at 6:43 AM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Hi Magnus, > > On Monday 17 February 2014 10:48:55 Magnus Damm wrote: >> On Mon, Feb 17, 2014 at 10:45 AM, Laurent Pinchart wrote: >> > On Saturday 15 February 2014 02:22:00 Magnus Damm wrote: >> >> On Sat, Feb 15, 2014 at 1:12 AM, Laurent Pinchart wrote: >> >> > On Saturday 15 February 2014 01:01:30 Magnus Damm wrote: >> >> >> On Sat, Feb 15, 2014 at 12:53 AM, Laurent Pinchart wrote: >> >> >> > On Friday 14 February 2014 10:58:22 Mark Rutland wrote: >> >> >> >> On Fri, Feb 14, 2014 at 01:00:01AM +0000, Laurent Pinchart wrote: >> >> >> >> > +Channels Optional Properties: >> >> >> >> > + >> >> >> >> > + - clock-source-rating: rating of the timer as a clock source >> >> >> >> > device. >> >> >> >> > + - clock-event-rating: rating of the timer as a clock event >> >> >> >> > device. >> >> >> >> >> >> >> >> This feels like a leak of Linux internals. Why do you need this? >> >> >> > >> >> >> > You're right, it is. The clock source and clock event ratings are >> >> >> > currently configured through platform data, I'll need to find a way >> >> >> > to compute them in the driver instead. >> >> >> >> >> >> That would be very good! >> >> > >> >> > Any pointer would be appreciated :-) How did you compute the various >> >> > ratings used in platform data all over the place ? >> >> >> >> Historically we used the rating to select between CMT and TMU. For >> >> clock sources I suppose you also have the jiffy rating to consider. >> >> And for the SMP parts we have ARM IP for TWD and arch timers that have >> >> their ratings too. So you need to check all the timers on a particular >> >> system and consider what you want to have operating by default. The >> >> ARM IP timers should be preferred if available. For clock sources the >> >> rule is probably the higher resolution the better. >> >> >> >> >> > There's still one piece of Linux-specific data I need though, as I >> >> >> > need to specify for each channel whether to use it as a clock source >> >> >> > device, a clock event device, both of them or none. That's >> >> >> > configuration information that needs to be provided somehow. >> >> >> >> >> >> I think you can decide clock source or clock event assignment based on >> >> >> number of channels available. If you have only a single channel then >> >> >> both clock event and clock source need to be supported. Otherwise use >> >> >> one channel for clock source and the rest for clock events. >> >> > >> >> > That won't match the current situation. Look at CMT0 in r8a7790 for >> >> > instance. There's two hardware channels available, and we only use the >> >> > first one, for clock events only. >> >> >> >> You are correct. The reason for that is that the CMT driver today is >> >> optimized for combined clock event and clock source operation. >> >> >> >> Historically the hardware it initially was written for (sh-mobile on >> >> the SH arch) only had a single timer channel so combined operation was >> >> required for tickless to work. But since you're asking how to allocate >> >> channels then I propose checking numbers of channels available and go >> >> from there. With that the r8a7790 support can only get better. =) >> >> >> >> >> This is probably out of scope for this DT conversion, but it would be >> >> >> neat if you somehow could specify the CPU affinity for a channel to >> >> >> tie a clock event to an individual CPU core. This would make a a per- >> >> >> cpu timer unless I'm mistaken. But that's more of a software policy >> >> >> than anything else. >> >> > >> >> > Yes, that's a configuration that needs to be specified somewhere. I >> >> > don't know where though. >> >> >> >> As long as you have per-channel interrupts described in DT you can >> >> probably handle this in a generic way in the driver. >> > >> > But how do we decide whether to use a single timer channel or one channel >> > per CPU ? Will the kernel use one clock event device per CPU >> > automatically ? I have to confess I have no idea how this works. >> >> I guess that's the tricky bit about timer support, it is a mix of >> hardware description and software configuration. So it sounds to me >> that we need some kind of software configuration interface. But it can >> probably be considered when/if we add such kind of support to the >> driver. Probably out of scope for now. >> >> Regardless it seems to me that the hardware description in DT doesn't >> need to care about this. > > I'll revisit that later. Per-CPU timers is not a high priority for now, so > I'll just return -EAGAIN :-) Thanks, that's fine. > Nonetheless, specifying which timer channel to use as a clock source and which > channel to use as a clock event device might need to be specified in DT (or > somewhere else, but I'm not sure what other options we have here). I disagree about the need for specifying clock source or clock event channel in DT. Since per-cpu timers is out of scope for now then why don't we simply just let the driver automatically allocate the during run-time? In case one CMT channel exists: Use that for both clock source and clock event. In case more than one CMT channel exists: Use one separate channel for clock source and one separate channel for clock event. That would cover our existing use case, no? Thanks, / magnus
Hi Magnus, On Tuesday 18 February 2014 09:51:30 Magnus Damm wrote: > On Tue, Feb 18, 2014 at 6:43 AM, Laurent Pinchart wrote: > > On Monday 17 February 2014 10:48:55 Magnus Damm wrote: > >> On Mon, Feb 17, 2014 at 10:45 AM, Laurent Pinchart wrote: > >> > On Saturday 15 February 2014 02:22:00 Magnus Damm wrote: > >> >> On Sat, Feb 15, 2014 at 1:12 AM, Laurent Pinchart wrote: > >> >> > On Saturday 15 February 2014 01:01:30 Magnus Damm wrote: > >> >> >> On Sat, Feb 15, 2014 at 12:53 AM, Laurent Pinchart wrote: [snip] > >> >> >> > There's still one piece of Linux-specific data I need though, as > >> >> >> > I need to specify for each channel whether to use it as a clock > >> >> >> > source device, a clock event device, both of them or none. That's > >> >> >> > configuration information that needs to be provided somehow. > >> >> >> > >> >> >> I think you can decide clock source or clock event assignment based > >> >> >> on number of channels available. If you have only a single channel > >> >> >> then both clock event and clock source need to be supported. > >> >> >> Otherwise use one channel for clock source and the rest for clock > >> >> >> events. > >> >> > > >> >> > That won't match the current situation. Look at CMT0 in r8a7790 for > >> >> > instance. There's two hardware channels available, and we only use > >> >> > the first one, for clock events only. > >> >> > >> >> You are correct. The reason for that is that the CMT driver today is > >> >> optimized for combined clock event and clock source operation. > >> >> > >> >> Historically the hardware it initially was written for (sh-mobile on > >> >> the SH arch) only had a single timer channel so combined operation was > >> >> required for tickless to work. But since you're asking how to allocate > >> >> channels then I propose checking numbers of channels available and go > >> >> from there. With that the r8a7790 support can only get better. =) [snip] > > Nonetheless, specifying which timer channel to use as a clock source and > > which channel to use as a clock event device might need to be specified > > in DT (or somewhere else, but I'm not sure what other options we have > > here). > > I disagree about the need for specifying clock source or clock event channel > in DT. Since per-cpu timers is out of scope for now then why don't we > simply just let the driver automatically allocate the during run-time? > > In case one CMT channel exists: > Use that for both clock source and clock event. > > In case more than one CMT channel exists: > Use one separate channel for clock source and one separate channel for clock > event. > > That would cover our existing use case, no? One of the issues with that approach is that the decision on what to use a channel for will be taken locally from a timer point of view, without a global system-wide view. When more than one timer is available for a given platform (several CMT instances for instance, or CMT + TMU) the driver will decide on how to configure each instance without taking the other timers into account. Beside leading to creating more clock sources and clock event devices than today (is that a problem ?), couldn't it also lead to taking sub-optimal decisions ? You also mentioned that only a subset of channels have the ability to run during sleep states. I suppose this needs to be taken into account as well. Could you please elaborate on our requirements in that area ?
diff --git a/Documentation/devicetree/bindings/timer/renesas,cmt.txt b/Documentation/devicetree/bindings/timer/renesas,cmt.txt new file mode 100644 index 0000000..28d4ab5 --- /dev/null +++ b/Documentation/devicetree/bindings/timer/renesas,cmt.txt @@ -0,0 +1,75 @@ +* Renesas R-Car Compare Match Timer (CMT) + +The CMT is a multi-channel 16/32/48-bit timer/counter with configurable clock +inputs and programmable compare match. + +Channels share hadware resources but their counter and compare match value are +independent. A particular CMT instance can implement only a subset of the +channels supported by the CMT model. Channels indices start from 0 and are +consecutive. + +Required Properties: + + - compatible: must contain one of the following. + - "renesas,cmt-32" for the 32-bit CMT + (CMT0 on sh7372, sh73a0 and r8a7740) + - "renesas,cmt-32-fast" for the 32-bit CMT with fast clock support + (CMT[234] on sh7372, sh73a0 and r8a7740) + - "renasas,cmt-48" for the 48-bit CMT + (CMT1 on sh7372, sh73a0 and r8a7740) + - "renesas,cmt-48-gen2" for the second generation 48-bit CMT + (CMT[01] on r8a73a4, r8a7790 and r8a7791) + + - reg: base address and length of the registers block for the timer module. + - interrupt-parent, interrupts: interrupt-specifier for the timer, one per + channel. + - clocks: phandle and clock-specifier pair for the functional clock. + - clock-names: must be "fck". + + - #address-cells: must be 1 + - #size-cells: must be 0 + + - renesas,channels-mask: integer bitmask of the channels implemented by the + timer instance. + + +Each channel is described by a sub-node named "channel@<idx>", where <idx> is +the channel index. + +Channels Required Properties: + + - reg: the channel index. + +Channels Optional Properties: + + - clock-source-rating: rating of the timer as a clock source device. + - clock-event-rating: rating of the timer as a clock event device. + + +Example: R8A7790 (R-Car H2) CMT0 node + + CMT0 on R8A7790 implements hardware channels 5 and 6 only and names + them channels 0 and 1 in the documentation. + + cmt0: timer@ffca0000 { + compatible = "renesas,cmt-48-gen2"; + reg = <0 0xffca0000 0 0x1004>; + interrupt-parent = <&gic>; + interrupts = <0 142 IRQ_TYPE_LEVEL_HIGH>, + <0 142 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&mstp1_clks R8A7790_CLK_CMT0>; + + #address-cells = <1>; + #size-cells = <0>; + + renesas,channels-mask = <0x60>; + + channel@0 { + reg = <0>; + clock-event-rating = <80>; + }; + channel@0 { + reg = <0>; + clock-source-rating = <80>; + }; + }; diff --git a/drivers/clocksource/sh_cmt.c b/drivers/clocksource/sh_cmt.c index fe9694b..8e46c40 100644 --- a/drivers/clocksource/sh_cmt.c +++ b/drivers/clocksource/sh_cmt.c @@ -24,6 +24,7 @@ #include <linux/ioport.h> #include <linux/irq.h> #include <linux/module.h> +#include <linux/of.h> #include <linux/platform_device.h> #include <linux/pm_domain.h> #include <linux/pm_runtime.h> @@ -918,10 +919,72 @@ static int sh_cmt_map_memory(struct sh_cmt_device *cmt) return 0; } +static const struct platform_device_id sh_cmt_id_table[] = { + { "sh-cmt-16", (kernel_ulong_t)&sh_cmt_info[SH_CMT_16BIT] }, + { "sh-cmt-32", (kernel_ulong_t)&sh_cmt_info[SH_CMT_32BIT] }, + { "sh-cmt-32-fast", (kernel_ulong_t)&sh_cmt_info[SH_CMT_32BIT_FAST] }, + { "sh-cmt-48", (kernel_ulong_t)&sh_cmt_info[SH_CMT_48BIT] }, + { "sh-cmt-48-gen2", (kernel_ulong_t)&sh_cmt_info[SH_CMT_48BIT_GEN2] }, + { } +}; +MODULE_DEVICE_TABLE(platform, sh_cmt_id_table); + +static const struct of_device_id sh_cmt_of_table[] = { + { .compatible = "renesas,cmt-32", .data = &sh_cmt_info[SH_CMT_32BIT] }, + { .compatible = "renesas,cmt-32-fast", .data = &sh_cmt_info[SH_CMT_32BIT_FAST] }, + { .compatible = "renasas,cmt-48", .data = &sh_cmt_info[SH_CMT_48BIT] }, + { .compatible = "renesas,cmt-48-gen2", .data = &sh_cmt_info[SH_CMT_48BIT_GEN2] }, + { } +}; +MODULE_DEVICE_TABLE(of, sh_cmt_of_table); + +static struct sh_timer_channel_config * +sh_cmt_parse_dt(struct sh_cmt_device *cmt) +{ + struct sh_timer_channel_config *channels; + struct sh_timer_channel_config *channel; + struct device_node *np = cmt->pdev->dev.of_node; + struct device_node *child; + int ret; + + cmt->num_channels = of_get_child_count(np); + if (cmt->num_channels == 0) + return ERR_PTR(-EINVAL); + + ret = of_property_read_u32(np, "renesas,channels-mask", + &cmt->hw_channels); + if (ret < 0) + return ERR_PTR(-EINVAL); + + channels = devm_kzalloc(&cmt->pdev->dev, sizeof(*channels), GFP_KERNEL); + if (channels == NULL) + return ERR_PTR(-ENOMEM); + + channel = channels; + + for_each_child_of_node(np, child) { + u32 val; + + ret = of_property_read_u32(child, "reg", &channel->index); + if (ret < 0) + return ERR_PTR(-EINVAL); + + ret = of_property_read_u32(child, "clock-source-rating", &val); + if (ret == 0) + channel->clocksource_rating = val; + ret = of_property_read_u32(child, "clock-event-rating", &val); + if (ret == 0) + channel->clockevent_rating = val; + + channel++; + } + + return channels; +} + static int sh_cmt_setup(struct sh_cmt_device *cmt, struct platform_device *pdev) { - struct sh_timer_config *cfg = pdev->dev.platform_data; - const struct platform_device_id *id = pdev->id_entry; + const struct sh_timer_channel_config *ch_cfg; unsigned int mask; unsigned int i; int ret; @@ -929,13 +992,28 @@ static int sh_cmt_setup(struct sh_cmt_device *cmt, struct platform_device *pdev) memset(cmt, 0, sizeof(*cmt)); cmt->pdev = pdev; - if (!cfg) { + if (IS_ENABLED(CONFIG_OF) && pdev->dev.of_node) { + const struct of_device_id *id; + + id = of_match_node(sh_cmt_of_table, pdev->dev.of_node); + cmt->info = id->data; + + ch_cfg = sh_cmt_parse_dt(cmt); + if (IS_ERR(ch_cfg)) + return PTR_ERR(ch_cfg); + } else if (pdev->dev.platform_data) { + struct sh_timer_config *cfg = pdev->dev.platform_data; + const struct platform_device_id *id = pdev->id_entry; + + cmt->info = (const struct sh_cmt_info *)id->driver_data; + cmt->num_channels = cfg->num_channels; + cmt->hw_channels = ~cfg->channels_mask; + ch_cfg = cfg->channels; + } else { dev_err(&cmt->pdev->dev, "missing platform data\n"); return -ENXIO; } - cmt->info = (const struct sh_cmt_info *)id->driver_data; - /* Get hold of clock. */ cmt->clk = clk_get(&cmt->pdev->dev, NULL); if (IS_ERR(cmt->clk)) { @@ -953,9 +1031,6 @@ static int sh_cmt_setup(struct sh_cmt_device *cmt, struct platform_device *pdev) goto err_clk_unprepare; /* Allocate and setup the channels. */ - cmt->num_channels = cfg->num_channels; - cmt->hw_channels = ~cfg->channels_mask; - cmt->channels = kzalloc(cmt->num_channels * sizeof(*cmt->channels), GFP_KERNEL); if (cmt->channels == NULL) { @@ -966,7 +1041,7 @@ static int sh_cmt_setup(struct sh_cmt_device *cmt, struct platform_device *pdev) for (i = 0, mask = cmt->hw_channels; i < cmt->num_channels; ++i) { unsigned int hwidx = ffs(mask) - 1; - ret = sh_cmt_setup_channel(&cmt->channels[i], &cfg->channels[i], + ret = sh_cmt_setup_channel(&cmt->channels[i], &ch_cfg[i], hwidx, cmt); if (ret < 0) goto err_unmap; @@ -1032,21 +1107,12 @@ static int sh_cmt_remove(struct platform_device *pdev) return -EBUSY; /* cannot unregister clockevent and clocksource */ } -static const struct platform_device_id sh_cmt_id_table[] = { - { "sh-cmt-16", (kernel_ulong_t)&sh_cmt_info[SH_CMT_16BIT] }, - { "sh-cmt-32", (kernel_ulong_t)&sh_cmt_info[SH_CMT_32BIT] }, - { "sh-cmt-32-fast", (kernel_ulong_t)&sh_cmt_info[SH_CMT_32BIT_FAST] }, - { "sh-cmt-48", (kernel_ulong_t)&sh_cmt_info[SH_CMT_48BIT] }, - { "sh-cmt-48-gen2", (kernel_ulong_t)&sh_cmt_info[SH_CMT_48BIT_GEN2] }, - { } -}; -MODULE_DEVICE_TABLE(platform, sh_cmt_id_table); - static struct platform_driver sh_cmt_device_driver = { .probe = sh_cmt_probe, .remove = sh_cmt_remove, .driver = { .name = "sh_cmt", + .of_match_table = of_match_ptr(sh_cmt_of_table), }, .id_table = sh_cmt_id_table, };
Cc: devicetree@vger.kernel.org Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> --- .../devicetree/bindings/timer/renesas,cmt.txt | 75 +++++++++++++++ drivers/clocksource/sh_cmt.c | 104 +++++++++++++++++---- 2 files changed, 160 insertions(+), 19 deletions(-) create mode 100644 Documentation/devicetree/bindings/timer/renesas,cmt.txt