Re: [PATCH 23/27] clocksource: sh_cmt: Add DT support

[Laurent Pinchart]

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@xxxxxxxxxxxxxxx
> > Signed-off-by: Laurent Pinchart
> > <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
> > ---
> > 
> >  .../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.

-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/