Clock divisor/parent settings changes

Clock divisor/parent settings changes

[William Towle]

Patches (3x) to ensure we see:
    sh_mobile_sdhi ee100000.sd: mmc0 base at 0xee100000 clock rate 97 MHz
    sh_mobile_sdhi ee140000.sd: mmc1 base at 0xee140000 clock rate 48 MHz

Attached:
    [PATCH 1/3] clk: rcar-h2: fix sd0/sd1 divisor table
    [PATCH 2/3] ARM: shmobile: r8a7790: specify multiple parents for
    [PATCH 3/3] clk: shmobile: handle multiple parent clocks for cpg

[PATCH 1/3] clk: rcar-h2: fix sd0/sd1 divisor table

[William Towle]

The clk_div_table for cpg_sd01_div_table[] concurs with the manual
but not with values found in the device itself (which are also the
same as the ones in arch/arm/mach-shmobile/clock-r8a7790.c).

Update the clk-rcar-gen2.c driver to have the same table as the one
used by the mach-shmobile driver which work once further issues are
fixed in the clk-rcar-gen2.c driver.

Part of the fix for the following error where the driver reports the
output as 1MHz but is really 97.5MHz:
    sh_mobile_sdhi ee100000.sd: mmc0 base at 0xee100000 clock rate 1 MHz

[ben.dooks@codethink.co.uk: updated patch description]
Signed-off-by: William Towle <william.towle@codethink.co.uk>
Reviewed-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
 drivers/clk/shmobile/clk-rcar-gen2.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/clk/shmobile/clk-rcar-gen2.c b/drivers/clk/shmobile/clk-rcar-gen2.c
index a59ec21..df4a1e6 100644
--- a/drivers/clk/shmobile/clk-rcar-gen2.c
+++ b/drivers/clk/shmobile/clk-rcar-gen2.c
@@ -170,6 +170,8 @@ static const struct clk_div_table cpg_sdh_div_table[] = {
 };
 
 static const struct clk_div_table cpg_sd01_div_table[] = {
+	{  0,  2 }, {  1,  3 }, {  2,  4 }, {  3,  6 },
+	{  4,  8 },
 	{  5, 12 }, {  6, 16 }, {  7, 18 }, {  8, 24 },
 	{ 10, 36 }, { 11, 48 }, { 12, 10 }, {  0,  0 },
 };
-- 
1.7.10.4

Re: [PATCH 1/3] clk: rcar-h2: fix sd0/sd1 divisor table

[Laurent Pinchart]

Hi William,

Thank you for the patch.

On Tuesday 04 February 2014 18:17:36 William Towle wrote:
> The clk_div_table for cpg_sd01_div_table[] concurs with the manual
> but not with values found in the device itself (which are also the
> same as the ones in arch/arm/mach-shmobile/clock-r8a7790.c).
> 
> Update the clk-rcar-gen2.c driver to have the same table as the one
> used by the mach-shmobile driver which work once further issues are
> fixed in the clk-rcar-gen2.c driver.
> 
> Part of the fix for the following error where the driver reports the
> output as 1MHz but is really 97.5MHz:
>     sh_mobile_sdhi ee100000.sd: mmc0 base at 0xee100000 clock rate 1 MHz
> 
> [ben.dooks@codethink.co.uk: updated patch description]
> Signed-off-by: William Towle <william.towle@codethink.co.uk>
> Reviewed-by: Ben Dooks <ben.dooks@codethink.co.uk>
> ---
>  drivers/clk/shmobile/clk-rcar-gen2.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/clk/shmobile/clk-rcar-gen2.c
> b/drivers/clk/shmobile/clk-rcar-gen2.c index a59ec21..df4a1e6 100644
> --- a/drivers/clk/shmobile/clk-rcar-gen2.c
> +++ b/drivers/clk/shmobile/clk-rcar-gen2.c
> @@ -170,6 +170,8 @@ static const struct clk_div_table cpg_sdh_div_table[] > { };
> 
>  static const struct clk_div_table cpg_sd01_div_table[] = {
> +	{  0,  2 }, {  1,  3 }, {  2,  4 }, {  3,  6 },
> +	{  4,  8 },
>  	{  5, 12 }, {  6, 16 }, {  7, 18 }, {  8, 24 },
>  	{ 10, 36 }, { 11, 48 }, { 12, 10 }, {  0,  0 },

With this applied the only difference between the sdh and sd0/1 dividers 
tables would be the { 12, 10 } entry, available for sd0/1 only. Given that the 
hardware does not match the documentation, could you check whether that entry 
is supported by sdh as well ? If so we could merge the two tables. Otherwise 
this patch looks good, could you please just reformat the table to avoid the 
mostly empty line in the middle ?

>  };

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/3] clk: rcar-h2: fix sd0/sd1 divisor table

[Ben Dooks]

On 05/02/14 10:56, Laurent Pinchart wrote:
> Hi William,
>
> Thank you for the patch.
>
> On Tuesday 04 February 2014 18:17:36 William Towle wrote:
>> The clk_div_table for cpg_sd01_div_table[] concurs with the manual
>> but not with values found in the device itself (which are also the
>> same as the ones in arch/arm/mach-shmobile/clock-r8a7790.c).
>>
>> Update the clk-rcar-gen2.c driver to have the same table as the one
>> used by the mach-shmobile driver which work once further issues are
>> fixed in the clk-rcar-gen2.c driver.
>>
>> Part of the fix for the following error where the driver reports the
>> output as 1MHz but is really 97.5MHz:
>>      sh_mobile_sdhi ee100000.sd: mmc0 base at 0xee100000 clock rate 1 MHz
>>
>> [ben.dooks@codethink.co.uk: updated patch description]
>> Signed-off-by: William Towle <william.towle@codethink.co.uk>
>> Reviewed-by: Ben Dooks <ben.dooks@codethink.co.uk>
>> ---
>>   drivers/clk/shmobile/clk-rcar-gen2.c |    2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/clk/shmobile/clk-rcar-gen2.c
>> b/drivers/clk/shmobile/clk-rcar-gen2.c index a59ec21..df4a1e6 100644
>> --- a/drivers/clk/shmobile/clk-rcar-gen2.c
>> +++ b/drivers/clk/shmobile/clk-rcar-gen2.c
>> @@ -170,6 +170,8 @@ static const struct clk_div_table cpg_sdh_div_table[] >> { };
>>
>>   static const struct clk_div_table cpg_sd01_div_table[] = {
>> +	{  0,  2 }, {  1,  3 }, {  2,  4 }, {  3,  6 },
>> +	{  4,  8 },
>>   	{  5, 12 }, {  6, 16 }, {  7, 18 }, {  8, 24 },
>>   	{ 10, 36 }, { 11, 48 }, { 12, 10 }, {  0,  0 },
>
> With this applied the only difference between the sdh and sd0/1 dividers
> tables would be the { 12, 10 } entry, available for sd0/1 only. Given that the
> hardware does not match the documentation, could you check whether that entry
> is supported by sdh as well ? If so we could merge the two tables. Otherwise
> this patch looks good, could you please just reformat the table to avoid the
> mostly empty line in the middle ?

I would like feedback from Renesas on this issue if possible. I can
have a quick try at setting the clock value to 10 in u-boot and scope
it out and see what happens.

Magnus or Morimoto-san, is there a chance this could be reviewed by
someone in Renesas who has knowledge of the hardware block?

[PS, added Kuninori Morimoto to this[

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

Re: [PATCH 1/3] clk: rcar-h2: fix sd0/sd1 divisor table

[Ben Dooks]

On 05/02/14 11:55, Ben Dooks wrote:
> On 05/02/14 10:56, Laurent Pinchart wrote:
>> Hi William,
>>
>> Thank you for the patch.
>>
>> On Tuesday 04 February 2014 18:17:36 William Towle wrote:
>>> The clk_div_table for cpg_sd01_div_table[] concurs with the manual
>>> but not with values found in the device itself (which are also the
>>> same as the ones in arch/arm/mach-shmobile/clock-r8a7790.c).
>>>
>>> Update the clk-rcar-gen2.c driver to have the same table as the one
>>> used by the mach-shmobile driver which work once further issues are
>>> fixed in the clk-rcar-gen2.c driver.
>>>
>>> Part of the fix for the following error where the driver reports the
>>> output as 1MHz but is really 97.5MHz:
>>>      sh_mobile_sdhi ee100000.sd: mmc0 base at 0xee100000 clock rate 1
>>> MHz
>>>
>>> [ben.dooks@codethink.co.uk: updated patch description]
>>> Signed-off-by: William Towle <william.towle@codethink.co.uk>
>>> Reviewed-by: Ben Dooks <ben.dooks@codethink.co.uk>
>>> ---
>>>   drivers/clk/shmobile/clk-rcar-gen2.c |    2 ++
>>>   1 file changed, 2 insertions(+)
>>>
>>> diff --git a/drivers/clk/shmobile/clk-rcar-gen2.c
>>> b/drivers/clk/shmobile/clk-rcar-gen2.c index a59ec21..df4a1e6 100644
>>> --- a/drivers/clk/shmobile/clk-rcar-gen2.c
>>> +++ b/drivers/clk/shmobile/clk-rcar-gen2.c
>>> @@ -170,6 +170,8 @@ static const struct clk_div_table
>>> cpg_sdh_div_table[] >>> { };
>>>
>>>   static const struct clk_div_table cpg_sd01_div_table[] = {
>>> +    {  0,  2 }, {  1,  3 }, {  2,  4 }, {  3,  6 },
>>> +    {  4,  8 },
>>>       {  5, 12 }, {  6, 16 }, {  7, 18 }, {  8, 24 },
>>>       { 10, 36 }, { 11, 48 }, { 12, 10 }, {  0,  0 },
>>
>> With this applied the only difference between the sdh and sd0/1 dividers
>> tables would be the { 12, 10 } entry, available for sd0/1 only. Given
>> that the
>> hardware does not match the documentation, could you check whether
>> that entry
>> is supported by sdh as well ? If so we could merge the two tables.
>> Otherwise
>> this patch looks good, could you please just reformat the table to
>> avoid the
>> mostly empty line in the middle ?
>
> I would like feedback from Renesas on this issue if possible. I can
> have a quick try at setting the clock value to 10 in u-boot and scope
> it out and see what happens.
>
> Magnus or Morimoto-san, is there a chance this could be reviewed by
> someone in Renesas who has knowledge of the hardware block?
>
> [PS, added Kuninori Morimoto to this[

I got William to do a quick test with the following u-boot command
	mw.l 0xE6150074 0xCCC

sdhi0 showed 156MHz output, and it seemed to work. So there is a
distinct possibility that the sdh clock also supports setting 12
for a /10

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

Re: [PATCH 1/3] clk: rcar-h2: fix sd0/sd1 divisor table

[Laurent Pinchart]

Hi Ben,

On Wednesday 05 February 2014 12:41:10 Ben Dooks wrote:
> On 05/02/14 11:55, Ben Dooks wrote:
> > On 05/02/14 10:56, Laurent Pinchart wrote:
> >> On Tuesday 04 February 2014 18:17:36 William Towle wrote:
> >>> The clk_div_table for cpg_sd01_div_table[] concurs with the manual
> >>> but not with values found in the device itself (which are also the
> >>> same as the ones in arch/arm/mach-shmobile/clock-r8a7790.c).
> >>> 
> >>> Update the clk-rcar-gen2.c driver to have the same table as the one
> >>> used by the mach-shmobile driver which work once further issues are
> >>> fixed in the clk-rcar-gen2.c driver.
> >>> 
> >>> Part of the fix for the following error where the driver reports the
> >>> 
> >>> output as 1MHz but is really 97.5MHz:
> >>>      sh_mobile_sdhi ee100000.sd: mmc0 base at 0xee100000 clock rate 1
> >>> MHz
> >>> 
> >>> [ben.dooks@codethink.co.uk: updated patch description]
> >>> Signed-off-by: William Towle <william.towle@codethink.co.uk>
> >>> Reviewed-by: Ben Dooks <ben.dooks@codethink.co.uk>
> >>> ---
> >>> 
> >>>   drivers/clk/shmobile/clk-rcar-gen2.c |    2 ++
> >>>   1 file changed, 2 insertions(+)
> >>> 
> >>> diff --git a/drivers/clk/shmobile/clk-rcar-gen2.c
> >>> b/drivers/clk/shmobile/clk-rcar-gen2.c index a59ec21..df4a1e6 100644
> >>> --- a/drivers/clk/shmobile/clk-rcar-gen2.c
> >>> +++ b/drivers/clk/shmobile/clk-rcar-gen2.c
> >>> @@ -170,6 +170,8 @@ static const struct clk_div_table
> >>> cpg_sdh_div_table[] > >>> { };
> >>> 
> >>>   static const struct clk_div_table cpg_sd01_div_table[] = {
> >>> 
> >>> +    {  0,  2 }, {  1,  3 }, {  2,  4 }, {  3,  6 },
> >>> +    {  4,  8 },
> >>> 
> >>>       {  5, 12 }, {  6, 16 }, {  7, 18 }, {  8, 24 },
> >>>       { 10, 36 }, { 11, 48 }, { 12, 10 }, {  0,  0 },
> >> 
> >> With this applied the only difference between the sdh and sd0/1 dividers
> >> tables would be the { 12, 10 } entry, available for sd0/1 only. Given
> >> that the hardware does not match the documentation, could you check
> >> whether that entry is supported by sdh as well ? If so we could merge the
> >> two tables. Otherwise this patch looks good, could you please just
> >> reformat the table to avoid the mostly empty line in the middle ?
> > 
> > I would like feedback from Renesas on this issue if possible. I can
> > have a quick try at setting the clock value to 10 in u-boot and scope
> > it out and see what happens.
> > 
> > Magnus or Morimoto-san, is there a chance this could be reviewed by
> > someone in Renesas who has knowledge of the hardware block?
> > 
> > [PS, added Kuninori Morimoto to this[
> 
> I got William to do a quick test with the following u-boot command
> 	mw.l 0xE6150074 0xCCC
> 
> sdhi0 showed 156MHz output, and it seemed to work. So there is a
> distinct possibility that the sdh clock also supports setting 12
> for a /10

Thank you for trying it out. I'd like feedback from Renesas as well, assuming 
we don't get a "don't do that or it will cause the universe to collapse - 
woops, too late" nack, let's respin this patch and merge the two tables 
instead.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/3] clk: rcar-h2: fix sd0/sd1 divisor table

[Kuninori Morimoto]

Hi all

> > >>> The clk_div_table for cpg_sd01_div_table[] concurs with the manual
> > >>> but not with values found in the device itself (which are also the
> > >>> same as the ones in arch/arm/mach-shmobile/clock-r8a7790.c).
> > >>> 
> > >>> Update the clk-rcar-gen2.c driver to have the same table as the one
> > >>> used by the mach-shmobile driver which work once further issues are
> > >>> fixed in the clk-rcar-gen2.c driver.
> > >>> 
> > >>> Part of the fix for the following error where the driver reports the
> > >>> 
> > >>> output as 1MHz but is really 97.5MHz:
> > >>>      sh_mobile_sdhi ee100000.sd: mmc0 base at 0xee100000 clock rate 1
> > >>> MHz
> > >>> 
> > >>> [ben.dooks@codethink.co.uk: updated patch description]
> > >>> Signed-off-by: William Towle <william.towle@codethink.co.uk>
> > >>> Reviewed-by: Ben Dooks <ben.dooks@codethink.co.uk>
(snip)
> > >>>   static const struct clk_div_table cpg_sd01_div_table[] = {
> > >>> +    {  0,  2 }, {  1,  3 }, {  2,  4 }, {  3,  6 },
> > >>> +    {  4,  8 },
> > >>> 
> > >>>       {  5, 12 }, {  6, 16 }, {  7, 18 }, {  8, 24 },
> > >>>       { 10, 36 }, { 11, 48 }, { 12, 10 }, {  0,  0 },
(snip)
> > > I would like feedback from Renesas on this issue if possible. I can
> > > have a quick try at setting the clock value to 10 in u-boot and scope
> > > it out and see what happens.
> > > 
> > > Magnus or Morimoto-san, is there a chance this could be reviewed by
> > > someone in Renesas who has knowledge of the hardware block?
> > > 
> > > [PS, added Kuninori Morimoto to this[
> > 
> > I got William to do a quick test with the following u-boot command
> > 	mw.l 0xE6150074 0xCCC
> > 
> > sdhi0 showed 156MHz output, and it seemed to work. So there is a
> > distinct possibility that the sdh clock also supports setting 12
> > for a /10
> 
> Thank you for trying it out. I'd like feedback from Renesas as well, assuming 
> we don't get a "don't do that or it will cause the universe to collapse - 
> woops, too late" nack, let's respin this patch and merge the two tables 
> instead.

I ask it to Renesas HW team.
Please wait

Best regards
---
Kuninori Morimoto

Re: [PATCH 1/3] clk: rcar-h2: fix sd0/sd1 divisor table

[Kuninori Morimoto]

Hi William, Ben, Laurent

> > > >>>   static const struct clk_div_table cpg_sd01_div_table[] = {
> > > >>> +    {  0,  2 }, {  1,  3 }, {  2,  4 }, {  3,  6 },
> > > >>> +    {  4,  8 },
> > > >>> 
> > > >>>       {  5, 12 }, {  6, 16 }, {  7, 18 }, {  8, 24 },
> > > >>>       { 10, 36 }, { 11, 48 }, { 12, 10 }, {  0,  0 },

According to HW team, datasheet is correct.
Some HW has above un-documented implementation indeed,
but these are not supported.
But, 0x0100 (x1/8) on SD0FC/SD1FC is now supported and documented in
latest datasheet.

> > > sdhi0 showed 156MHz output, and it seemed to work. So there is a
> > > distinct possibility that the sdh clock also supports setting 12
> > > for a /10

According to HW there,
SDHFC will be stopped if you set 0xC.

Best regards
---
Kuninori Morimoto

Re: [PATCH 1/3] clk: rcar-h2: fix sd0/sd1 divisor table

[Ben Dooks]

On 07/02/14 06:43, Kuninori Morimoto wrote:
> Hi William, Ben, Laurent
>
>>>>>>>    static const struct clk_div_table cpg_sd01_div_table[] = {
>>>>>>> +    {  0,  2 }, {  1,  3 }, {  2,  4 }, {  3,  6 },
>>>>>>> +    {  4,  8 },
>>>>>>>
>>>>>>>        {  5, 12 }, {  6, 16 }, {  7, 18 }, {  8, 24 },
>>>>>>>        { 10, 36 }, { 11, 48 }, { 12, 10 }, {  0,  0 },
>
> According to HW team, datasheet is correct.
> Some HW has above un-documented implementation indeed,
> but these are not supported.
> But, 0x0100 (x1/8) on SD0FC/SD1FC is now supported and documented in
> latest datasheet.

Thanks, we do not have a copy of that so cannot comment.

>>>> sdhi0 showed 156MHz output, and it seemed to work. So there is a
>>>> distinct possibility that the sdh clock also supports setting 12
>>>> for a /10
>
> According to HW there,
> SDHFC will be stopped if you set 0xC.

We'll look at re-working this patch and getting it re-sent.

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

[PATCH 2/3] ARM: shmobile: r8a7790: specify multiple parents for cpg_clks

[William Towle]

The current drivers/clk/shmobile/clk-rcar-gen2.c uses the
extal_clk reference for the parent of all the clocks that
it registers. However the lb, qspi, sdh, sd0 and sd1 clocks
are all parented to either pll1 or pll1_div2 which means
that the clock rates are incorrect.

This is part of the fix that corrects the SDHI0 clock
rate error where it reports 1MHz instead of 97.5:
    sh_mobile_sdhi ee100000.sd: mmc0 base at 0xee100000 clock rate 1 MHz

Notes:
- May require cross-merge with clk-rcar-gen2.c fix
- Also not clear which clock "z" is to fix it.

[ben.dooks@codethink.co.uk: updated patch description]
Signed-off-by: William Towle <william.towle@codethink.co.uk>
Reviewed-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
 arch/arm/boot/dts/r8a7790.dtsi |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/r8a7790.dtsi b/arch/arm/boot/dts/r8a7790.dtsi
index ff55c6e..242e6e2 100644
--- a/arch/arm/boot/dts/r8a7790.dtsi
+++ b/arch/arm/boot/dts/r8a7790.dtsi
@@ -446,7 +446,13 @@
 			compatible = "renesas,r8a7790-cpg-clocks",
 				     "renesas,rcar-gen2-cpg-clocks";
 			reg = <0 0xe6150000 0 0x1000>;
-			clocks = <&extal_clk>;
+			clocks = <&extal_clk>, <&extal_clk>, <&extal_clk>, <&extal_clk>,
+				<&cpg_clocks R8A7790_CLK_PLL1>,
+				<&pll1_div2_clk>,
+				<&cpg_clocks R8A7790_CLK_PLL1>,
+				<&cpg_clocks R8A7790_CLK_PLL1>,
+				<&cpg_clocks R8A7790_CLK_PLL1>
+				/* not known for "z" ...,<> */;
 			#clock-cells = <1>;
 			clock-output-names = "main", "pll0", "pll1", "pll3",
 					     "lb", "qspi", "sdh", "sd0", "sd1",
-- 
1.7.10.4

Re: [Linux-kernel] [PATCH 2/3] ARM: shmobile: r8a7790: specify multiple parents for cpg_clks

[Ben Dooks]

On 04/02/14 18:17, William Towle wrote:
> The current drivers/clk/shmobile/clk-rcar-gen2.c uses the
> extal_clk reference for the parent of all the clocks that
> it registers. However the lb, qspi, sdh, sd0 and sd1 clocks
> are all parented to either pll1 or pll1_div2 which means
> that the clock rates are incorrect.
>
> This is part of the fix that corrects the SDHI0 clock
> rate error where it reports 1MHz instead of 97.5:
>      sh_mobile_sdhi ee100000.sd: mmc0 base at 0xee100000 clock rate 1 MHz
>
> Notes:
> - May require cross-merge with clk-rcar-gen2.c fix
> - Also not clear which clock "z" is to fix it.

Laurent, if you could give us an idea of how to fix this then
it would be helpful to get this patch fully fixed.

>
> [ben.dooks@codethink.co.uk: updated patch description]
> Signed-off-by: William Towle <william.towle@codethink.co.uk>
> Reviewed-by: Ben Dooks <ben.dooks@codethink.co.uk>
> ---
>   arch/arm/boot/dts/r8a7790.dtsi |    8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/boot/dts/r8a7790.dtsi b/arch/arm/boot/dts/r8a7790.dtsi
> index ff55c6e..242e6e2 100644
> --- a/arch/arm/boot/dts/r8a7790.dtsi
> +++ b/arch/arm/boot/dts/r8a7790.dtsi
> @@ -446,7 +446,13 @@
>   			compatible = "renesas,r8a7790-cpg-clocks",
>   				     "renesas,rcar-gen2-cpg-clocks";
>   			reg = <0 0xe6150000 0 0x1000>;
> -			clocks = <&extal_clk>;
> +			clocks = <&extal_clk>, <&extal_clk>, <&extal_clk>, <&extal_clk>,
> +				<&cpg_clocks R8A7790_CLK_PLL1>,
> +				<&pll1_div2_clk>,
> +				<&cpg_clocks R8A7790_CLK_PLL1>,
> +				<&cpg_clocks R8A7790_CLK_PLL1>,
> +				<&cpg_clocks R8A7790_CLK_PLL1>

Should we add a pll1_clk node, or leave this as is?

> +				/* not known for "z" ...,<> */;
>   			#clock-cells = <1>;
>   			clock-output-names = "main", "pll0", "pll1", "pll3",
>   					     "lb", "qspi", "sdh", "sd0", "sd1",
>


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

Re: [Linux-kernel] [PATCH 2/3] ARM: shmobile: r8a7790: specify multiple parents for cpg_clks

[Laurent Pinchart]

Hi Ben,

On Wednesday 05 February 2014 09:15:31 Ben Dooks wrote:
> On 04/02/14 18:17, William Towle wrote:
> > The current drivers/clk/shmobile/clk-rcar-gen2.c uses the
> > extal_clk reference for the parent of all the clocks that
> > it registers. However the lb, qspi, sdh, sd0 and sd1 clocks
> > are all parented to either pll1 or pll1_div2 which means
> > that the clock rates are incorrect.
> > 
> > This is part of the fix that corrects the SDHI0 clock
> > 
> > rate error where it reports 1MHz instead of 97.5:
> >      sh_mobile_sdhi ee100000.sd: mmc0 base at 0xee100000 clock rate 1 MHz
> > 
> > Notes:
> > - May require cross-merge with clk-rcar-gen2.c fix
> > - Also not clear which clock "z" is to fix it.
> 
> Laurent, if you could give us an idea of how to fix this then
> it would be helpful to get this patch fully fixed.

I've already sent a patch that fixes this issue.

"clk: shmobile: rcar-gen2: Fix clock parent all non-PLL clocks"

(http://www.spinics.net/lists/linux-sh/msg27275.html)

I've just pinged Mike to ask him to pick it up for v3.14.

> > [ben.dooks@codethink.co.uk: updated patch description]
> > Signed-off-by: William Towle <william.towle@codethink.co.uk>
> > Reviewed-by: Ben Dooks <ben.dooks@codethink.co.uk>
> > ---
> > 
> >   arch/arm/boot/dts/r8a7790.dtsi |    8 +++++++-
> >   1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm/boot/dts/r8a7790.dtsi
> > b/arch/arm/boot/dts/r8a7790.dtsi index ff55c6e..242e6e2 100644
> > --- a/arch/arm/boot/dts/r8a7790.dtsi
> > +++ b/arch/arm/boot/dts/r8a7790.dtsi
> > @@ -446,7 +446,13 @@
> > 
> >   			compatible = "renesas,r8a7790-cpg-clocks",
> >   			
> >   				     "renesas,rcar-gen2-cpg-clocks";
> >   			
> >   			reg = <0 0xe6150000 0 0x1000>;
> > 
> > -			clocks = <&extal_clk>;
> > +			clocks = <&extal_clk>, <&extal_clk>, <&extal_clk>, 
<&extal_clk>,
> > +				<&cpg_clocks R8A7790_CLK_PLL1>,
> > +				<&pll1_div2_clk>,
> > +				<&cpg_clocks R8A7790_CLK_PLL1>,
> > +				<&cpg_clocks R8A7790_CLK_PLL1>,
> > +				<&cpg_clocks R8A7790_CLK_PLL1>
> 
> Should we add a pll1_clk node, or leave this as is?
> 
> > +				/* not known for "z" ...,<> */;
> > 
> >   			#clock-cells = <1>;
> >   			clock-output-names = "main", "pll0", "pll1", "pll3",
> >   			
> >   					     "lb", "qspi", "sdh", "sd0", "sd1",

-- 
Regards,

Laurent Pinchart

Re: [Linux-kernel] [PATCH 2/3] ARM: shmobile: r8a7790: specify multiple parents for cpg_clks

[Ben Dooks]

On 05/02/14 10:32, Laurent Pinchart wrote:
> Hi Ben,
>
> On Wednesday 05 February 2014 09:15:31 Ben Dooks wrote:
>> On 04/02/14 18:17, William Towle wrote:
>>> The current drivers/clk/shmobile/clk-rcar-gen2.c uses the
>>> extal_clk reference for the parent of all the clocks that
>>> it registers. However the lb, qspi, sdh, sd0 and sd1 clocks
>>> are all parented to either pll1 or pll1_div2 which means
>>> that the clock rates are incorrect.
>>>
>>> This is part of the fix that corrects the SDHI0 clock
>>>
>>> rate error where it reports 1MHz instead of 97.5:
>>>       sh_mobile_sdhi ee100000.sd: mmc0 base at 0xee100000 clock rate 1 MHz
>>>
>>> Notes:
>>> - May require cross-merge with clk-rcar-gen2.c fix
>>> - Also not clear which clock "z" is to fix it.
>>
>> Laurent, if you could give us an idea of how to fix this then
>> it would be helpful to get this patch fully fixed.
>
> I've already sent a patch that fixes this issue.
>
> "clk: shmobile: rcar-gen2: Fix clock parent all non-PLL clocks"
>
> (http://www.spinics.net/lists/linux-sh/msg27275.html)
>
> I've just pinged Mike to ask him to pick it up for v3.14.

I just saw and commented on it. I think the DT is the nicer way
of actually doing this, especially if the driver may get re-used
in future.

There's also an issue with SDHI0/1 divider table which has been
posted too.

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

Re: [Linux-kernel] [PATCH 2/3] ARM: shmobile: r8a7790: specify multiple parents for cpg_clks

[Laurent Pinchart]

Hi Ben,

On Wednesday 05 February 2014 10:36:56 Ben Dooks wrote:
> On 05/02/14 10:32, Laurent Pinchart wrote:
> > On Wednesday 05 February 2014 09:15:31 Ben Dooks wrote:
> >> On 04/02/14 18:17, William Towle wrote:
> >>> The current drivers/clk/shmobile/clk-rcar-gen2.c uses the
> >>> extal_clk reference for the parent of all the clocks that
> >>> it registers. However the lb, qspi, sdh, sd0 and sd1 clocks
> >>> are all parented to either pll1 or pll1_div2 which means
> >>> that the clock rates are incorrect.
> >>> 
> >>> This is part of the fix that corrects the SDHI0 clock
> >>> 
> >>> rate error where it reports 1MHz instead of 97.5:
> >>>       sh_mobile_sdhi ee100000.sd: mmc0 base at 0xee100000 clock rate 1
> >>>       MHz
> >>> 
> >>> Notes:
> >>> - May require cross-merge with clk-rcar-gen2.c fix
> >>> - Also not clear which clock "z" is to fix it.
> >> 
> >> Laurent, if you could give us an idea of how to fix this then
> >> it would be helpful to get this patch fully fixed.
> > 
> > I've already sent a patch that fixes this issue.
> > 
> > "clk: shmobile: rcar-gen2: Fix clock parent all non-PLL clocks"
> > 
> > (http://www.spinics.net/lists/linux-sh/msg27275.html)
> > 
> > I've just pinged Mike to ask him to pick it up for v3.14.
> 
> I just saw and commented on it. I think the DT is the nicer way
> of actually doing this, especially if the driver may get re-used
> in future.

I've replied to that in the other mail thread, we can continue the discussion 
there.

> There's also an issue with SDHI0/1 divider table which has been posted too.

I've replied to that patch as well.

-- 
Regards,

Laurent Pinchart

[PATCH 3/3] clk: shmobile: handle multiple parent clocks for cpg clocks

[William Towle]

The current action on registering the cpg_clocks is to have
them all registered with a single parent clock. This is not
the correct action, as some of the clocks such as the SDHI
and QSPI are actually parented to PLL1 or PLL1/2.

This also requires a change to the device-tree files to make
sure that appropriate parent clocks are assigned.

[ben.dooks@codethink.co.uk: updated patch description]
Signed-off-by: William Towle <william.towle@codethink.co.uk>
Reviewed-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
 drivers/clk/shmobile/clk-rcar-gen2.c |   11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/shmobile/clk-rcar-gen2.c b/drivers/clk/shmobile/clk-rcar-gen2.c
index df4a1e6..57bc2e4 100644
--- a/drivers/clk/shmobile/clk-rcar-gen2.c
+++ b/drivers/clk/shmobile/clk-rcar-gen2.c
@@ -185,7 +185,7 @@ static u32 cpg_mode __initdata;
 static struct clk * __init
 rcar_gen2_cpg_register_clock(struct device_node *np, struct rcar_gen2_cpg *cpg,
 			     const struct cpg_pll_config *config,
-			     const char *name)
+			     const char *name, int index)
 {
 	const struct clk_div_table *table = NULL;
 	const char *parent_name = "main";
@@ -193,8 +193,13 @@ rcar_gen2_cpg_register_clock(struct device_node *np, struct rcar_gen2_cpg *cpg,
 	unsigned int mult = 1;
 	unsigned int div = 1;
 
+	parent_name = of_clk_get_parent_name(np, index);
+	if (!parent_name) {
+		pr_err("no parent set for clocks array\n");
+		return ERR_PTR(-ENOENT);
+	}
+
 	if (!strcmp(name, "main")) {
-		parent_name = of_clk_get_parent_name(np, 0);
 		div = config->extal_div;
 	} else if (!strcmp(name, "pll0")) {
 		/* PLL0 is a configurable multiplier clock. Register it as a
@@ -279,7 +284,7 @@ static void __init rcar_gen2_cpg_clocks_init(struct device_node *np)
 		of_property_read_string_index(np, "clock-output-names", i,
 					      &name);
 
-		clk = rcar_gen2_cpg_register_clock(np, cpg, config, name);
+		clk = rcar_gen2_cpg_register_clock(np, cpg, config, name, i);
 		if (IS_ERR(clk))
 			pr_err("%s: failed to register %s %s clock (%ld)\n",
 			       __func__, np->name, name, PTR_ERR(clk));
-- 
1.7.10.4