clk: ti: dra7: fix parent for gmac_clkctrl

Message ID 20191109142017.10851-1-grygorii.strashko@ti.com
State Superseded
Headers show
Series
  • clk: ti: dra7: fix parent for gmac_clkctrl
Related show

Commit Message

Grygorii Strashko Nov. 9, 2019, 2:20 p.m.
The parent clk for gmac clk ctrl has to be gmac_main_clk (125MHz) instead
of dpll_gmac_ck (1GHz). This is caused incorrect CPSW MDIO operation.
Hence, fix it.

Fixes: commit dffa9051d546 ('clk: ti: dra7: add new clkctrl data')
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>

---
 drivers/clk/ti/clk-7xx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.17.1

Comments

Tony Lindgren Nov. 11, 2019, 5:12 p.m. | #1
* Grygorii Strashko <grygorii.strashko@ti.com> [191109 14:21]:
> The parent clk for gmac clk ctrl has to be gmac_main_clk (125MHz) instead

> of dpll_gmac_ck (1GHz). This is caused incorrect CPSW MDIO operation.

> Hence, fix it.

> 

> Fixes: commit dffa9051d546 ('clk: ti: dra7: add new clkctrl data')

> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>


Hmm is there a mux for the source though?

In that case using assigned-clocks and assigned-clock-parents in
addition to a related clk-7xx.c fix would be needed.

Regards,

Tony

> ---

>  drivers/clk/ti/clk-7xx.c | 2 +-

>  1 file changed, 1 insertion(+), 1 deletion(-)

> 

> diff --git a/drivers/clk/ti/clk-7xx.c b/drivers/clk/ti/clk-7xx.c

> index 9dd6185a4b4e..66e4b2b9ec60 100644

> --- a/drivers/clk/ti/clk-7xx.c

> +++ b/drivers/clk/ti/clk-7xx.c

> @@ -405,7 +405,7 @@ static const struct omap_clkctrl_bit_data dra7_gmac_bit_data[] __initconst = {

>  };

>  

>  static const struct omap_clkctrl_reg_data dra7_gmac_clkctrl_regs[] __initconst = {

> -	{ DRA7_GMAC_GMAC_CLKCTRL, dra7_gmac_bit_data, CLKF_SW_SUP, "dpll_gmac_ck" },

> +	{ DRA7_GMAC_GMAC_CLKCTRL, dra7_gmac_bit_data, CLKF_SW_SUP, "gmac_main_clk" },

>  	{ 0 },

>  };

>  

> -- 

> 2.17.1

>
Grygorii Strashko Nov. 13, 2019, 10:02 a.m. | #2
On 11/11/2019 19:12, Tony Lindgren wrote:
> * Grygorii Strashko <grygorii.strashko@ti.com> [191109 14:21]:

>> The parent clk for gmac clk ctrl has to be gmac_main_clk (125MHz) instead

>> of dpll_gmac_ck (1GHz). This is caused incorrect CPSW MDIO operation.

>> Hence, fix it.

>>

>> Fixes: commit dffa9051d546 ('clk: ti: dra7: add new clkctrl data')

>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>

> 

> Hmm is there a mux for the source though?


Not sure what do you mean here :(

fck clock for CPSW and MDIO is "gmac_main_clk" which is 125MHz and
that what need to be passed to drivers and enabled through the clock tree.
The TI specific PM is handled by gmac_clkctrl DRA7_GMAC_GMAC_CLKCTRL 0
which required sysc programming and child modules dosn't need to even know that.

So, this patch is simply correct clock tree for dra7:
dpll_gmac_ck -> .... -> gmac_main_clk -> gmac_clkctrl DRA7_GMAC_GMAC_CLKCTRL 0

Seems MDIO dt also need to be fixed to use:
gmac_main_clk as "fck". I'll try and send patch.

By the way, the patch follows am3/am4 (am4_cpsw_125mhz_clkctrl_regs,
  am3_cpsw_125mhz_clkctrl_regs)


> 

> In that case using assigned-clocks and assigned-clock-parents in

> addition to a related clk-7xx.c fix would be needed.

> 

> Regards,

> 

> Tony

> 

>> ---

>>   drivers/clk/ti/clk-7xx.c | 2 +-

>>   1 file changed, 1 insertion(+), 1 deletion(-)

>>

>> diff --git a/drivers/clk/ti/clk-7xx.c b/drivers/clk/ti/clk-7xx.c

>> index 9dd6185a4b4e..66e4b2b9ec60 100644

>> --- a/drivers/clk/ti/clk-7xx.c

>> +++ b/drivers/clk/ti/clk-7xx.c

>> @@ -405,7 +405,7 @@ static const struct omap_clkctrl_bit_data dra7_gmac_bit_data[] __initconst = {

>>   };

>>   

>>   static const struct omap_clkctrl_reg_data dra7_gmac_clkctrl_regs[] __initconst = {

>> -	{ DRA7_GMAC_GMAC_CLKCTRL, dra7_gmac_bit_data, CLKF_SW_SUP, "dpll_gmac_ck" },

>> +	{ DRA7_GMAC_GMAC_CLKCTRL, dra7_gmac_bit_data, CLKF_SW_SUP, "gmac_main_clk" },

>>   	{ 0 },

>>   };

>>   

>> -- 

>> 2.17.1

>>


-- 
Best regards,
grygorii
Tony Lindgren Nov. 13, 2019, 3:23 p.m. | #3
* Grygorii Strashko <grygorii.strashko@ti.com> [191113 10:02]:
> 

> 

> On 11/11/2019 19:12, Tony Lindgren wrote:

> > * Grygorii Strashko <grygorii.strashko@ti.com> [191109 14:21]:

> > > The parent clk for gmac clk ctrl has to be gmac_main_clk (125MHz) instead

> > > of dpll_gmac_ck (1GHz). This is caused incorrect CPSW MDIO operation.

> > > Hence, fix it.

> > > 

> > > Fixes: commit dffa9051d546 ('clk: ti: dra7: add new clkctrl data')

> > > Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>

> > 

> > Hmm is there a mux for the source though?

> 

> Not sure what do you mean here :(

> 

> fck clock for CPSW and MDIO is "gmac_main_clk" which is 125MHz and

> that what need to be passed to drivers and enabled through the clock tree.

> The TI specific PM is handled by gmac_clkctrl DRA7_GMAC_GMAC_CLKCTRL 0

> which required sysc programming and child modules dosn't need to even know that.


OK

> So, this patch is simply correct clock tree for dra7:

> dpll_gmac_ck -> .... -> gmac_main_clk -> gmac_clkctrl DRA7_GMAC_GMAC_CLKCTRL 0


So I guess there's no mux clock for DRA7_GMAC_GMAC_CLKCTRL 0.

What I meant is maybe check also that no top level mux is needed
similar to what we have for these configured with assigned-clocks:

$ git grep -C3 assigned-clock arch/arm/boot/dts/dra7*

> Seems MDIO dt also need to be fixed to use:

> gmac_main_clk as "fck". I'll try and send patch.


OK

> By the way, the patch follows am3/am4 (am4_cpsw_125mhz_clkctrl_regs,

>  am3_cpsw_125mhz_clkctrl_regs)


OK

Tony
Grygorii Strashko Nov. 13, 2019, 3:35 p.m. | #4
On 13/11/2019 17:23, Tony Lindgren wrote:
> * Grygorii Strashko <grygorii.strashko@ti.com> [191113 10:02]:

>>

>>

>> On 11/11/2019 19:12, Tony Lindgren wrote:

>>> * Grygorii Strashko <grygorii.strashko@ti.com> [191109 14:21]:

>>>> The parent clk for gmac clk ctrl has to be gmac_main_clk (125MHz) instead

>>>> of dpll_gmac_ck (1GHz). This is caused incorrect CPSW MDIO operation.

>>>> Hence, fix it.

>>>>

>>>> Fixes: commit dffa9051d546 ('clk: ti: dra7: add new clkctrl data')

>>>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>

>>>

>>> Hmm is there a mux for the source though?

>>

>> Not sure what do you mean here :(

>>

>> fck clock for CPSW and MDIO is "gmac_main_clk" which is 125MHz and

>> that what need to be passed to drivers and enabled through the clock tree.

>> The TI specific PM is handled by gmac_clkctrl DRA7_GMAC_GMAC_CLKCTRL 0

>> which required sysc programming and child modules dosn't need to even know that.

> 

> OK

> 

>> So, this patch is simply correct clock tree for dra7:

>> dpll_gmac_ck -> .... -> gmac_main_clk -> gmac_clkctrl DRA7_GMAC_GMAC_CLKCTRL 0

> 

> So I guess there's no mux clock for DRA7_GMAC_GMAC_CLKCTRL 0.

> 

> What I meant is maybe check also that no top level mux is needed

> similar to what we have for these configured with assigned-clocks:

> 

> $ git grep -C3 assigned-clock arch/arm/boot/dts/dra7*


No. No muxes here.

[...]

-- 
Best regards,
grygorii
Tony Lindgren Nov. 13, 2019, 3:40 p.m. | #5
* Grygorii Strashko <grygorii.strashko@ti.com> [191113 15:36]:
> 

> 

> On 13/11/2019 17:23, Tony Lindgren wrote:

> > * Grygorii Strashko <grygorii.strashko@ti.com> [191113 10:02]:

> > > 

> > > 

> > > On 11/11/2019 19:12, Tony Lindgren wrote:

> > > > * Grygorii Strashko <grygorii.strashko@ti.com> [191109 14:21]:

> > > > > The parent clk for gmac clk ctrl has to be gmac_main_clk (125MHz) instead

> > > > > of dpll_gmac_ck (1GHz). This is caused incorrect CPSW MDIO operation.

> > > > > Hence, fix it.

> > > > > 

> > > > > Fixes: commit dffa9051d546 ('clk: ti: dra7: add new clkctrl data')

> > > > > Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>

> > > > 

> > > > Hmm is there a mux for the source though?

> > > 

> > > Not sure what do you mean here :(

> > > 

> > > fck clock for CPSW and MDIO is "gmac_main_clk" which is 125MHz and

> > > that what need to be passed to drivers and enabled through the clock tree.

> > > The TI specific PM is handled by gmac_clkctrl DRA7_GMAC_GMAC_CLKCTRL 0

> > > which required sysc programming and child modules dosn't need to even know that.

> > 

> > OK

> > 

> > > So, this patch is simply correct clock tree for dra7:

> > > dpll_gmac_ck -> .... -> gmac_main_clk -> gmac_clkctrl DRA7_GMAC_GMAC_CLKCTRL 0

> > 

> > So I guess there's no mux clock for DRA7_GMAC_GMAC_CLKCTRL 0.

> > 

> > What I meant is maybe check also that no top level mux is needed

> > similar to what we have for these configured with assigned-clocks:

> > 

> > $ git grep -C3 assigned-clock arch/arm/boot/dts/dra7*

> 

> No. No muxes here.


OK thanks for checking.

Tony
Stephen Boyd Dec. 18, 2019, 5:46 p.m. | #6
Quoting Grygorii Strashko (2019-11-09 06:20:17)
> The parent clk for gmac clk ctrl has to be gmac_main_clk (125MHz) instead

> of dpll_gmac_ck (1GHz). This is caused incorrect CPSW MDIO operation.

> Hence, fix it.

> 

> Fixes: commit dffa9051d546 ('clk: ti: dra7: add new clkctrl data')


There shouldn't be 'commit' in a Fixes: line.

Patch

diff --git a/drivers/clk/ti/clk-7xx.c b/drivers/clk/ti/clk-7xx.c
index 9dd6185a4b4e..66e4b2b9ec60 100644
--- a/drivers/clk/ti/clk-7xx.c
+++ b/drivers/clk/ti/clk-7xx.c
@@ -405,7 +405,7 @@  static const struct omap_clkctrl_bit_data dra7_gmac_bit_data[] __initconst = {
 };
 
 static const struct omap_clkctrl_reg_data dra7_gmac_clkctrl_regs[] __initconst = {
-	{ DRA7_GMAC_GMAC_CLKCTRL, dra7_gmac_bit_data, CLKF_SW_SUP, "dpll_gmac_ck" },
+	{ DRA7_GMAC_GMAC_CLKCTRL, dra7_gmac_bit_data, CLKF_SW_SUP, "gmac_main_clk" },
 	{ 0 },
 };