mstp10 clock bug

mstp10 clock bug

[Ben Dooks]

William and I have been looking at the clock bug on mstp10_clks
from the sound.git patch which reverts all the clocks [1]

What happens is that when register the node:

> -		mstp10_clks: mstp10_clks@e6150998 {
> -			compatible = "renesas,r8a7790-mstp-clocks", "renesas,cpg-mstp-clocks";
> -			reg = <0 0xe6150998 0 4>, <0 0xe61509a8 0 4>;
> -			clocks = <&p_clk>, <&mstp10_clks R8A7790_CLK_SSI>,
> -				 <&mstp10_clks R8A7790_CLK_SSI>, <&mstp10_clks R8A7790_CLK_SSI>,
> -				 <&mstp10_clks R8A7790_CLK_SSI>, <&mstp10_clks R8A7790_CLK_SSI>,
> -				 <&mstp10_clks R8A7790_CLK_SSI>, <&mstp10_clks R8A7790_CLK_SSI>,
> -				 <&mstp10_clks R8A7790_CLK_SSI>, <&mstp10_clks R8A7790_CLK_SSI>,
> -				 <&mstp10_clks R8A7790_CLK_SSI>;
> -			#clock-cells = <1>;
> -			renesas,clock-indices = <
> -				R8A7790_CLK_SSI R8A7790_CLK_SSI9 R8A7790_CLK_SSI8
> -				R8A7790_CLK_SSI7 R8A7790_CLK_SSI6 R8A7790_CLK_SSI5
> -				R8A7790_CLK_SSI4 R8A7790_CLK_SSI3 R8A7790_CLK_SSI2
> -				R8A7790_CLK_SSI1 R8A7790_CLK_SSI0
> -			>;
> -			clock-output-names > -				"ssi", "ssi9", "ssi8", "ssi7", "ssi6", "ssi5",
> -				"ssi4", "ssi3", "ssi2", "ssi1", "ssi0";
> -		};
>  	};

The ssi5 clock is registered with a parent of ssi5, not ssi as described
in the above node.

This is due to of_clk_get_parent_name() reading the clock-output-names
property and _assuming_ a 1:1 correspondence for the clock index to the
clock-output-names position. In the case of the mstp clocks each of
these nodes has sparse entries, which means the following:

<&mstp10_clks R8A7790_CLK_SSI> R8A7790_CLK_SSI becomes <&mstp10_clks 5>
and the 5th entry in clock-output-names is "ssi5".

This means that the entire machine comes to a halt as the clock layer
tries to create a clock with itself as a parent (never a good idea)

Currently we do not know the best way to fix this.

- we could update the of_clk_get_parent_name() to check the presence of
   renesas,clock-indices which would be a hack.

- allow the clock driver to register a parent name callback, but that
   would assume we never looked up nodes that where not registered yet.

- change all the clock-output-names arrays for rcar dtsi files to have
   null entries where needed, which would be not nice.

- add a new property for mapping numbers to clock indicies and moving
   the renesas,clock-indices to use that (although that would still end
   up causing a number of issues with the clock handling as is)



> https://git.kernel.org/cgit/linux/kernel/git/broonie/sound.git/commit/?h=topic/rcar&idp8d2aa14d1eefef3dd758242787e837485baad7

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

Re: mstp10 clock bug

[Laurent Pinchart]

Hi Ben,

On Wednesday 12 February 2014 19:01:33 Ben Dooks wrote:
> William and I have been looking at the clock bug on mstp10_clks
> from the sound.git patch which reverts all the clocks [1]
> 
> What happens is that when register the node:
> > -		mstp10_clks: mstp10_clks@e6150998 {
> > -			compatible = "renesas,r8a7790-mstp-clocks", "renesas,cpg-
mstp-clocks";
> > -			reg = <0 0xe6150998 0 4>, <0 0xe61509a8 0 4>;
> > -			clocks = <&p_clk>, <&mstp10_clks R8A7790_CLK_SSI>,
> > -				 <&mstp10_clks R8A7790_CLK_SSI>, <&mstp10_clks 
R8A7790_CLK_SSI>,
> > -				 <&mstp10_clks R8A7790_CLK_SSI>, <&mstp10_clks 
R8A7790_CLK_SSI>,
> > -				 <&mstp10_clks R8A7790_CLK_SSI>, <&mstp10_clks 
R8A7790_CLK_SSI>,
> > -				 <&mstp10_clks R8A7790_CLK_SSI>, <&mstp10_clks 
R8A7790_CLK_SSI>,
> > -				 <&mstp10_clks R8A7790_CLK_SSI>;
> > -			#clock-cells = <1>;
> > -			renesas,clock-indices = <
> > -				R8A7790_CLK_SSI R8A7790_CLK_SSI9 R8A7790_CLK_SSI8
> > -				R8A7790_CLK_SSI7 R8A7790_CLK_SSI6 R8A7790_CLK_SSI5
> > -				R8A7790_CLK_SSI4 R8A7790_CLK_SSI3 R8A7790_CLK_SSI2
> > -				R8A7790_CLK_SSI1 R8A7790_CLK_SSI0
> > -			>;
> > -			clock-output-names > > -				"ssi", "ssi9", "ssi8", "ssi7", "ssi6", "ssi5",
> > -				"ssi4", "ssi3", "ssi2", "ssi1", "ssi0";
> > -		};
> > 
> >  	};
> 
> The ssi5 clock is registered with a parent of ssi5, not ssi as described
> in the above node.
> 
> This is due to of_clk_get_parent_name() reading the clock-output-names
> property and _assuming_ a 1:1 correspondence for the clock index to the
> clock-output-names position. In the case of the mstp clocks each of
> these nodes has sparse entries, which means the following:
> 
> <&mstp10_clks R8A7790_CLK_SSI> R8A7790_CLK_SSI becomes <&mstp10_clks 5>
> and the 5th entry in clock-output-names is "ssi5".
> 
> This means that the entire machine comes to a halt as the clock layer
> tries to create a clock with itself as a parent (never a good idea)
> 
> Currently we do not know the best way to fix this.
> 
> - we could update the of_clk_get_parent_name() to check the presence of
>    renesas,clock-indices which would be a hack.
> 
> - allow the clock driver to register a parent name callback, but that
>    would assume we never looked up nodes that where not registered yet.
> 
> - change all the clock-output-names arrays for rcar dtsi files to have
>    null entries where needed, which would be not nice.
> 
> - add a new property for mapping numbers to clock indicies and moving
>    the renesas,clock-indices to use that (although that would still end
>    up causing a number of issues with the clock handling as is)
> 
> > https://git.kernel.org/cgit/linux/kernel/git/broonie/sound.git/commit/?h=t
> > opic/rcar&idp8d2aa14d1eefef3dd758242787e837485baad7

I've submitted a second version of the SSI clocks patches that fixes the issue 
(http://www.spinics.net/lists/linux-sh/msg27013.html). I've just pinged Mike 
Turquette and have CC'ed you.

As explained in the cover letter, the problem comes from the fact that the 
meaning of the DT "clocks" property depends on whether the property is located 
in a clock provider node or a clock consumer node. Let's discuss the issue in 
the other mail thread.

-- 
Regards,

Laurent Pinchart

Re: mstp10 clock bug

[Ben Dooks]

On 13/02/14 14:51, Laurent Pinchart wrote:
> Hi Ben,
>
> On Wednesday 12 February 2014 19:01:33 Ben Dooks wrote:
>> William and I have been looking at the clock bug on mstp10_clks
>> from the sound.git patch which reverts all the clocks [1]
>>
>> What happens is that when register the node:
>>> -		mstp10_clks: mstp10_clks@e6150998 {
>>> -			compatible = "renesas,r8a7790-mstp-clocks", "renesas,cpg-
> mstp-clocks";
>>> -			reg = <0 0xe6150998 0 4>, <0 0xe61509a8 0 4>;
>>> -			clocks = <&p_clk>, <&mstp10_clks R8A7790_CLK_SSI>,
>>> -				 <&mstp10_clks R8A7790_CLK_SSI>, <&mstp10_clks
> R8A7790_CLK_SSI>,
>>> -				 <&mstp10_clks R8A7790_CLK_SSI>, <&mstp10_clks
> R8A7790_CLK_SSI>,
>>> -				 <&mstp10_clks R8A7790_CLK_SSI>, <&mstp10_clks
> R8A7790_CLK_SSI>,
>>> -				 <&mstp10_clks R8A7790_CLK_SSI>, <&mstp10_clks
> R8A7790_CLK_SSI>,
>>> -				 <&mstp10_clks R8A7790_CLK_SSI>;
>>> -			#clock-cells = <1>;
>>> -			renesas,clock-indices = <
>>> -				R8A7790_CLK_SSI R8A7790_CLK_SSI9 R8A7790_CLK_SSI8
>>> -				R8A7790_CLK_SSI7 R8A7790_CLK_SSI6 R8A7790_CLK_SSI5
>>> -				R8A7790_CLK_SSI4 R8A7790_CLK_SSI3 R8A7790_CLK_SSI2
>>> -				R8A7790_CLK_SSI1 R8A7790_CLK_SSI0
>>> -			>;
>>> -			clock-output-names >>> -				"ssi", "ssi9", "ssi8", "ssi7", "ssi6", "ssi5",
>>> -				"ssi4", "ssi3", "ssi2", "ssi1", "ssi0";
>>> -		};
>>>
>>>   	};
>>
>> The ssi5 clock is registered with a parent of ssi5, not ssi as described
>> in the above node.
>>
>> This is due to of_clk_get_parent_name() reading the clock-output-names
>> property and _assuming_ a 1:1 correspondence for the clock index to the
>> clock-output-names position. In the case of the mstp clocks each of
>> these nodes has sparse entries, which means the following:
>>
>> <&mstp10_clks R8A7790_CLK_SSI> R8A7790_CLK_SSI becomes <&mstp10_clks 5>
>> and the 5th entry in clock-output-names is "ssi5".
>>
>> This means that the entire machine comes to a halt as the clock layer
>> tries to create a clock with itself as a parent (never a good idea)
>>
>> Currently we do not know the best way to fix this.
>>
>> - we could update the of_clk_get_parent_name() to check the presence of
>>     renesas,clock-indices which would be a hack.
>>
>> - allow the clock driver to register a parent name callback, but that
>>     would assume we never looked up nodes that where not registered yet.
>>
>> - change all the clock-output-names arrays for rcar dtsi files to have
>>     null entries where needed, which would be not nice.
>>
>> - add a new property for mapping numbers to clock indicies and moving
>>     the renesas,clock-indices to use that (although that would still end
>>     up causing a number of issues with the clock handling as is)
>>
>>> https://git.kernel.org/cgit/linux/kernel/git/broonie/sound.git/commit/?h=t
>>> opic/rcar&idp8d2aa14d1eefef3dd758242787e837485baad7
>
> I've submitted a second version of the SSI clocks patches that fixes the issue
> (http://www.spinics.net/lists/linux-sh/msg27013.html). I've just pinged Mike
> Turquette and have CC'ed you.
>
> As explained in the cover letter, the problem comes from the fact that the
> meaning of the DT "clocks" property depends on whether the property is located
> in a clock provider node or a clock consumer node. Let's discuss the issue in
> the other mail thread.

Ok, I've got a 2-3 patch series that fixes the problem by changing the
renesas,clock-indicies to clock-indicies and making the code looking
up the name use this to work out the array index of clock-output-names.





-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

Re: mstp10 clock bug

[Laurent Pinchart]

Hi Ben,

On Thursday 13 February 2014 17:43:19 Ben Dooks wrote:
> On 13/02/14 14:51, Laurent Pinchart wrote:
> > On Wednesday 12 February 2014 19:01:33 Ben Dooks wrote:
> >> William and I have been looking at the clock bug on mstp10_clks
> >> from the sound.git patch which reverts all the clocks [1]
> >> 
> >> What happens is that when register the node:
> >>> -		mstp10_clks: mstp10_clks@e6150998 {
> >>> -			compatible = "renesas,r8a7790-mstp-clocks", "renesas,cpg-
> > 
> > mstp-clocks";
> > 
> >>> -			reg = <0 0xe6150998 0 4>, <0 0xe61509a8 0 4>;
> >>> -			clocks = <&p_clk>, <&mstp10_clks R8A7790_CLK_SSI>,
> >>> -				 <&mstp10_clks R8A7790_CLK_SSI>, <&mstp10_clks
> > 
> > R8A7790_CLK_SSI>,
> > 
> >>> -				 <&mstp10_clks R8A7790_CLK_SSI>, <&mstp10_clks
> > 
> > R8A7790_CLK_SSI>,
> > 
> >>> -				 <&mstp10_clks R8A7790_CLK_SSI>, <&mstp10_clks
> > 
> > R8A7790_CLK_SSI>,
> > 
> >>> -				 <&mstp10_clks R8A7790_CLK_SSI>, <&mstp10_clks
> > 
> > R8A7790_CLK_SSI>,
> > 
> >>> -				 <&mstp10_clks R8A7790_CLK_SSI>;
> >>> -			#clock-cells = <1>;
> >>> -			renesas,clock-indices = <
> >>> -				R8A7790_CLK_SSI R8A7790_CLK_SSI9 R8A7790_CLK_SSI8
> >>> -				R8A7790_CLK_SSI7 R8A7790_CLK_SSI6 R8A7790_CLK_SSI5
> >>> -				R8A7790_CLK_SSI4 R8A7790_CLK_SSI3 R8A7790_CLK_SSI2
> >>> -				R8A7790_CLK_SSI1 R8A7790_CLK_SSI0
> >>> -			>;
> >>> -			clock-output-names > >>> -				"ssi", "ssi9", "ssi8", "ssi7", "ssi6", "ssi5",
> >>> -				"ssi4", "ssi3", "ssi2", "ssi1", "ssi0";
> >>> -		};
> >>> 
> >>>   	};
> >> 
> >> The ssi5 clock is registered with a parent of ssi5, not ssi as described
> >> in the above node.
> >> 
> >> This is due to of_clk_get_parent_name() reading the clock-output-names
> >> property and _assuming_ a 1:1 correspondence for the clock index to the
> >> clock-output-names position. In the case of the mstp clocks each of
> >> these nodes has sparse entries, which means the following:
> >> 
> >> <&mstp10_clks R8A7790_CLK_SSI> R8A7790_CLK_SSI becomes <&mstp10_clks 5>
> >> and the 5th entry in clock-output-names is "ssi5".
> >> 
> >> This means that the entire machine comes to a halt as the clock layer
> >> tries to create a clock with itself as a parent (never a good idea)
> >> 
> >> Currently we do not know the best way to fix this.
> >> 
> >> - we could update the of_clk_get_parent_name() to check the presence of
> >>   renesas,clock-indices which would be a hack.
> >> 
> >> - allow the clock driver to register a parent name callback, but that
> >>   would assume we never looked up nodes that where not registered yet.
> >> 
> >> - change all the clock-output-names arrays for rcar dtsi files to have
> >>   null entries where needed, which would be not nice.
> >> 
> >> - add a new property for mapping numbers to clock indicies and moving
> >>   the renesas,clock-indices to use that (although that would still end
> >>   up causing a number of issues with the clock handling as is)
> >>> 
> >>> https://git.kernel.org/cgit/linux/kernel/git/broonie/sound.git/commit/?h
> >>> =topic/rcar&idp8d2aa14d1eefef3dd758242787e837485baad7
> > 
> > I've submitted a second version of the SSI clocks patches that fixes the
> > issue (http://www.spinics.net/lists/linux-sh/msg27013.html). I've just
> > pinged Mike Turquette and have CC'ed you.
> > 
> > As explained in the cover letter, the problem comes from the fact that the
> > meaning of the DT "clocks" property depends on whether the property is
> > located in a clock provider node or a clock consumer node. Let's discuss
> > the issue in the other mail thread.
> 
> Ok, I've got a 2-3 patch series that fixes the problem by changing the
> renesas,clock-indicies to clock-indicies and making the code looking
> up the name use this to work out the array index of clock-output-names.

Thank you. I'll let Mike comment on that, I have no strong preference.

-- 
Regards,

Laurent Pinchart