Re: [PATCH v3 14/19] ARM: shmobile: r8a7779: Add TMU devices to DT

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Simon,

On Monday 16 June 2014 17:47:07 Simon Horman wrote:
> On Sat, Jun 14, 2014 at 06:23:36PM +0200, Laurent Pinchart wrote:
> > Add the TMU0, TMU1 and TMU2 counters to the r8a7779 device tree and make
> > them disabled by default.
> > 
> > Signed-off-by: Laurent Pinchart
> > <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> > 
> >  arch/arm/boot/dts/r8a7779.dtsi | 42 +++++++++++++++++++++++++++++++++++++
> >  1 file changed, 42 insertions(+)
> > 
> > diff --git a/arch/arm/boot/dts/r8a7779.dtsi
> > b/arch/arm/boot/dts/r8a7779.dtsi index 94e2fc8..bf716ce 100644
> > --- a/arch/arm/boot/dts/r8a7779.dtsi
> > +++ b/arch/arm/boot/dts/r8a7779.dtsi
> > @@ -266,6 +266,48 @@
> >  		reg = <0xffc48000 0x38>;
> >  	};
> > 
> > +	tmu0: timer@ffd80000 {
> > +		compatible = "renesas,tmu";
> > +		reg = <0xffd80000 0x30>;
> > +		interrupts = <0 40 IRQ_TYPE_LEVEL_HIGH>,
> > +			     <0 41 IRQ_TYPE_LEVEL_HIGH>,
> > +			     <0 42 IRQ_TYPE_LEVEL_HIGH>;
> > +		clocks = <&mstp0_clks R8A7779_CLK_TMU0>;
> > +		clock-names = "fck";
> > +
> > +		#renesas,channels = <3>;
> > +
> > +		status = "disabled";
> > +	};
> > +
> > +	tmu1: timer@ffd81000 {
> > +		compatible = "renesas,tmu";
> > +		reg = <0xffd81000 0x30>;
> > +		interrupts = <0 44 IRQ_TYPE_LEVEL_HIGH>,
> > +			     <0 45 IRQ_TYPE_LEVEL_HIGH>,
> > +			     <0 46 IRQ_TYPE_LEVEL_HIGH>;
> > +		clocks = <&mstp0_clks R8A7779_CLK_TMU1>;
> > +		clock-names = "fck";
> > +
> > +		#renesas,channels = <3>;
> > +
> > +		status = "disabled";
> > +	};
> > +
> > +	tmu2: timer@ffd82000 {
> > +		compatible = "renesas,tmu";
> > +		reg = <0xffd82000 0x30>;
> > +		interrupts = <0 48 IRQ_TYPE_LEVEL_HIGH>,
> > +			     <0 49 IRQ_TYPE_LEVEL_HIGH>,
> > +			     <0 50 IRQ_TYPE_LEVEL_HIGH>;
> > +		clocks = <&mstp0_clks R8A7779_CLK_TMU2>;
> > +		clock-names = "fck";
> > +
> > +		#renesas,channels = <3>;
> > +
> > +		status = "disabled";
> > +	};
> > +
> >  	sata: sata@fc600000 {
> >  		compatible = "renesas,rcar-sata";
> >  		reg = <0xfc600000 0x2000>;
> 
> There are no interrupt-parents in in the nodes above nodes
> although the documentation of the binding has one in the example.
> Is it just an oversight in this patch?

Not really.

The interrupt-parent property is required but can be inherited. Furthermore, 
the interrupts-extended property can be used to replace the interrupts and 
interrupt-parent properties. This should all be documented in the DT bindings, 
but not by duplicating the full explanation in dozens of different flavours in 
all bindings. We need standard wordings for common properties such as 
interrupts, clocks or reg.

> I believe that the IRQ numbers are not correct.
> 
> In setup-r8a7779.c I see gic_iid(0x40). But above I see 40.
> I think the correct value is 0x40 - 32 = 32. Likewise
> for the other IRQs of tmu0. And looking at the documentaiton
> this seems true for tmu1 and tmu2 too.
>
> I have successfully booted a marzen board with the following incremental
> patch to the DT nodes.

You're right, sorry about that. I'll fix the interrupt numbers and resubmit. 
The interrupt-parent is specified at the top level so there's no need to add 
it to the TMU nodes.

> diff --git a/arch/arm/boot/dts/r8a7779.dtsi b/arch/arm/boot/dts/r8a7779.dtsi
> index bf716ce..81714ce 100644
> --- a/arch/arm/boot/dts/r8a7779.dtsi
> +++ b/arch/arm/boot/dts/r8a7779.dtsi
> @@ -269,9 +269,10 @@
>  	tmu0: timer@ffd80000 {
>  		compatible = "renesas,tmu";
>  		reg = <0xffd80000 0x30>;
> -		interrupts = <0 40 IRQ_TYPE_LEVEL_HIGH>,
> -			     <0 41 IRQ_TYPE_LEVEL_HIGH>,
> -			     <0 42 IRQ_TYPE_LEVEL_HIGH>;
> +		interrupt-parent = <&gic>;
> +		interrupts = <0 32 IRQ_TYPE_LEVEL_HIGH>,
> +			     <0 33 IRQ_TYPE_LEVEL_HIGH>,
> +			     <0 34 IRQ_TYPE_LEVEL_HIGH>;
>  		clocks = <&mstp0_clks R8A7779_CLK_TMU0>;
>  		clock-names = "fck";
> 
> @@ -283,9 +284,10 @@
>  	tmu1: timer@ffd81000 {
>  		compatible = "renesas,tmu";
>  		reg = <0xffd81000 0x30>;
> -		interrupts = <0 44 IRQ_TYPE_LEVEL_HIGH>,
> -			     <0 45 IRQ_TYPE_LEVEL_HIGH>,
> -			     <0 46 IRQ_TYPE_LEVEL_HIGH>;
> +		interrupt-parent = <&gic>;
> +		interrupts = <0 35 IRQ_TYPE_LEVEL_HIGH>,
> +			     <0 36 IRQ_TYPE_LEVEL_HIGH>,
> +			     <0 37 IRQ_TYPE_LEVEL_HIGH>;

If I'm not mistaken the interrupts should be 36, 37 and 38.

>  		clocks = <&mstp0_clks R8A7779_CLK_TMU1>;
>  		clock-names = "fck";
> 
> @@ -296,10 +298,11 @@
> 
>  	tmu2: timer@ffd82000 {
>  		compatible = "renesas,tmu";
> +		interrupt-parent = <&gic>;
>  		reg = <0xffd82000 0x30>;
> -		interrupts = <0 48 IRQ_TYPE_LEVEL_HIGH>,
> -			     <0 49 IRQ_TYPE_LEVEL_HIGH>,
> -			     <0 50 IRQ_TYPE_LEVEL_HIGH>;
> +		interrupts = <0 38 IRQ_TYPE_LEVEL_HIGH>,
> +			     <0 39 IRQ_TYPE_LEVEL_HIGH>,
> +			     <0 40 IRQ_TYPE_LEVEL_HIGH>;

And 40, 41 and 42 here.

>  		clocks = <&mstp0_clks R8A7779_CLK_TMU2>;
>  		clock-names = "fck";

-- 
Regards,

Laurent Pinchart