[PATCH 0/3] clk: ti: add CLK_SET_RATE_PARENT support for clkctrl

[PATCH 0/3] clk: ti: add CLK_SET_RATE_PARENT support for clkctrl

[Tero Kristo]

Hi,

This patch adds clock rate propagation support for clkctrl clocks. This
is needed on am33xx beaglebone black device at least, otherwise the
display does not work. Similar problem appears to be present on am43xx
but haven't heard of any reports of such so far, maybe Jyri can confirm
this?

-Tero

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCH 1/3] clk: ti: clkctrl: add support for CLK_SET_RATE_PARENT flag

[Tero Kristo]

Certain clkctrl clocks, notably the display ones, use the
CLK_SET_RATE_PARENT feature extensively. Add support for this flag
to the clkctrl clocks.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
Reported-by: Jyri Sarha <jsarha@ti.com>
---
 drivers/clk/ti/clkctrl.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/clk/ti/clkctrl.c b/drivers/clk/ti/clkctrl.c
index afa0d6b..421b0539 100644
--- a/drivers/clk/ti/clkctrl.c
+++ b/drivers/clk/ti/clkctrl.c
@@ -537,6 +537,8 @@ static void __init _ti_omap4_clkctrl_setup(struct device_node *node)
 		init.parent_names = &reg_data->parent;
 		init.num_parents = 1;
 		init.flags = 0;
+		if (reg_data->flags & CLKF_SET_RATE_PARENT)
+			init.flags |= CLK_SET_RATE_PARENT;
 		init.name = kasprintf(GFP_KERNEL, "%s:%s:%04x:%d",
 				      node->parent->name, node->name,
 				      reg_data->offset, 0);
-- 
1.9.1

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCH 2/3] clk: ti: am33xx: add set-rate-parent support for display clkctrl clock

[Tero Kristo]

Display driver assumes it can use clk_set_rate for the display clock
via set-rate-parent mechanism, so add the flag for this to it.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
Reported-by: Jyri Sarha <jsarha@ti.com>
---
 drivers/clk/ti/clk-33xx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/ti/clk-33xx.c b/drivers/clk/ti/clk-33xx.c
index 612491a..12e0a2d 100644
--- a/drivers/clk/ti/clk-33xx.c
+++ b/drivers/clk/ti/clk-33xx.c
@@ -45,7 +45,7 @@
 
 static const struct omap_clkctrl_reg_data am3_l4_per_clkctrl_regs[] __initconst = {
 	{ AM3_CPGMAC0_CLKCTRL, NULL, CLKF_SW_SUP, "cpsw_125mhz_gclk", "cpsw_125mhz_clkdm" },
-	{ AM3_LCDC_CLKCTRL, NULL, CLKF_SW_SUP, "lcd_gclk", "lcdc_clkdm" },
+	{ AM3_LCDC_CLKCTRL, NULL, CLKF_SW_SUP | CLKF_SET_RATE_PARENT, "lcd_gclk", "lcdc_clkdm" },
 	{ AM3_USB_OTG_HS_CLKCTRL, NULL, CLKF_SW_SUP, "usbotg_fck", "l3s_clkdm" },
 	{ AM3_TPTC0_CLKCTRL, NULL, CLKF_SW_SUP, "l3_gclk", "l3_clkdm" },
 	{ AM3_EMIF_CLKCTRL, NULL, CLKF_SW_SUP, "dpll_ddr_m2_div2_ck", "l3_clkdm" },
-- 
1.9.1

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCH 3/3] clk: ti: am43xx: add set-rate-parent support for display clkctrl clock

[Tero Kristo]

Display driver assumes it can use clk_set_rate for the display clock
via set-rate-parent mechanism, so add the flag for this to id.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
---
 drivers/clk/ti/clk-43xx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/ti/clk-43xx.c b/drivers/clk/ti/clk-43xx.c
index 2b7c2e0..63c5ddb 100644
--- a/drivers/clk/ti/clk-43xx.c
+++ b/drivers/clk/ti/clk-43xx.c
@@ -187,7 +187,7 @@
 	{ AM4_OCP2SCP0_CLKCTRL, NULL, CLKF_SW_SUP, "l4ls_gclk" },
 	{ AM4_OCP2SCP1_CLKCTRL, NULL, CLKF_SW_SUP, "l4ls_gclk" },
 	{ AM4_EMIF_CLKCTRL, NULL, CLKF_SW_SUP, "dpll_ddr_m2_ck", "emif_clkdm" },
-	{ AM4_DSS_CORE_CLKCTRL, NULL, CLKF_SW_SUP, "disp_clk", "dss_clkdm" },
+	{ AM4_DSS_CORE_CLKCTRL, NULL, CLKF_SW_SUP | CLKF_SET_RATE_PARENT, "disp_clk", "dss_clkdm" },
 	{ AM4_CPGMAC0_CLKCTRL, NULL, CLKF_SW_SUP, "cpsw_125mhz_gclk", "cpsw_125mhz_clkdm" },
 	{ 0 },
 };
-- 
1.9.1

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH 0/3] clk: ti: add CLK_SET_RATE_PARENT support for clkctrl

[Tony Lindgren]

Hi,

* Tero Kristo <t-kristo@ti.com> [180226 15:11]:
> This patch adds clock rate propagation support for clkctrl clocks. This
> is needed on am33xx beaglebone black device at least, otherwise the
> display does not work. Similar problem appears to be present on am43xx
> but haven't heard of any reports of such so far, maybe Jyri can confirm
> this?

I think I was also hitting this with the system timers with ti-sysc
where set_parent() would fail unless the dts has the following for
timers:

clocks = <&l4_wkup_clkctrl OMAP4_TIMER1_CLKCTRL 24>;

So bit 24 instead of 0.

Hmm so should we have all the timers use bit 0 in the dtsi?
Or default to bit 24 for all of them?

Regards,

Tony

Re: [PATCH 0/3] clk: ti: add CLK_SET_RATE_PARENT support for clkctrl

[Tero Kristo]

On 27/02/18 00:05, Tony Lindgren wrote:
> Hi,
> 
> * Tero Kristo <t-kristo@ti.com> [180226 15:11]:
>> This patch adds clock rate propagation support for clkctrl clocks. This
>> is needed on am33xx beaglebone black device at least, otherwise the
>> display does not work. Similar problem appears to be present on am43xx
>> but haven't heard of any reports of such so far, maybe Jyri can confirm
>> this?
> 
> I think I was also hitting this with the system timers with ti-sysc
> where set_parent() would fail unless the dts has the following for
> timers:
> 
> clocks = <&l4_wkup_clkctrl OMAP4_TIMER1_CLKCTRL 24>;
> 
> So bit 24 instead of 0.
> 
> Hmm so should we have all the timers use bit 0 in the dtsi?
> Or default to bit 24 for all of them?

Who is going to control the clkctrl clock for the timers if you just 
control the opt clock? Also, ain't the bit 24 the clksel mux setting? 
Tweaking that would seem wrong...

If you were facing problems with setting the clock source, we could 
maybe just apply the CLK_SET_RATE_PARENT globally to all clkctrl clocks 
(modify the patch #1 in this series to always set the flag, instead of 
clecking against the CLKF_xyz flag.) Any thoughts?

-Tero
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH 0/3] clk: ti: add CLK_SET_RATE_PARENT support for clkctrl

[Tony Lindgren]

* Tero Kristo <t-kristo@ti.com> [180227 06:35]:
> On 27/02/18 00:05, Tony Lindgren wrote:
> > Hi,
> > 
> > * Tero Kristo <t-kristo@ti.com> [180226 15:11]:
> > > This patch adds clock rate propagation support for clkctrl clocks. This
> > > is needed on am33xx beaglebone black device at least, otherwise the
> > > display does not work. Similar problem appears to be present on am43xx
> > > but haven't heard of any reports of such so far, maybe Jyri can confirm
> > > this?
> > 
> > I think I was also hitting this with the system timers with ti-sysc
> > where set_parent() would fail unless the dts has the following for
> > timers:
> > 
> > clocks = <&l4_wkup_clkctrl OMAP4_TIMER1_CLKCTRL 24>;
> > 
> > So bit 24 instead of 0.
> > 
> > Hmm so should we have all the timers use bit 0 in the dtsi?
> > Or default to bit 24 for all of them?
> 
> Who is going to control the clkctrl clock for the timers if you just control
> the opt clock? Also, ain't the bit 24 the clksel mux setting? Tweaking that
> would seem wrong...

Yeah OK.

> If you were facing problems with setting the clock source, we could maybe
> just apply the CLK_SET_RATE_PARENT globally to all clkctrl clocks (modify
> the patch #1 in this series to always set the flag, instead of clecking
> against the CLKF_xyz flag.) Any thoughts?

Sounds good to me if that keeps omap_dm_timer_init_one() working
for clk_set_parent() for configuring timer via dts :)

I think there's a reserved range for the parent clocks that is
different from the opt clocks in the clkctrl registers so it
can be maybe checked that way?

Regards,

Tony

Re: [PATCH 0/3] clk: ti: add CLK_SET_RATE_PARENT support for clkctrl

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [180227 16:43]:
> * Tero Kristo <t-kristo@ti.com> [180227 06:35]:
> > On 27/02/18 00:05, Tony Lindgren wrote:
> > > Hmm so should we have all the timers use bit 0 in the dtsi?
> > > Or default to bit 24 for all of them?
> > 
> > Who is going to control the clkctrl clock for the timers if you just control
> > the opt clock? Also, ain't the bit 24 the clksel mux setting? Tweaking that
> > would seem wrong...
> 
> Yeah OK.

$ git grep TIMER arch/arm/boot/dts/* | grep CLKCTRL

And that shows timer1 using bit 24 for omap4, omap5 and dra7
dtsi files.

So shouldn't that then be just bit 0 instead of bit 24 for
those? And then we let omap_dm_timer_init_one() reparent it?

Regards,

Tony

Re: [PATCH 0/3] clk: ti: add CLK_SET_RATE_PARENT support for clkctrl

[Tero Kristo]

On 27/02/18 18:48, Tony Lindgren wrote:
> * Tony Lindgren <tony@atomide.com> [180227 16:43]:
>> * Tero Kristo <t-kristo@ti.com> [180227 06:35]:
>>> On 27/02/18 00:05, Tony Lindgren wrote:
>>>> Hmm so should we have all the timers use bit 0 in the dtsi?
>>>> Or default to bit 24 for all of them?
>>>
>>> Who is going to control the clkctrl clock for the timers if you just control
>>> the opt clock? Also, ain't the bit 24 the clksel mux setting? Tweaking that
>>> would seem wrong...
>>
>> Yeah OK.
> 
> $ git grep TIMER arch/arm/boot/dts/* | grep CLKCTRL
> 
> And that shows timer1 using bit 24 for omap4, omap5 and dra7
> dtsi files.
> 
> So shouldn't that then be just bit 0 instead of bit 24 for
> those? And then we let omap_dm_timer_init_one() reparent it?

Actually, that fck setting is because of this patch:

138f7ca78f5a0677f591fdf23d0309c2f4774bf7
ARM: OMAP2+: timer: add support for fetching fck handle from DT

So, you need to provide the clock handle at bit offset 24.

The main clkctrl clock is still handled via hwmod core, or via the 
interconnect driver.

-Tero
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH 0/3] clk: ti: add CLK_SET_RATE_PARENT support for clkctrl

[Jyri Sarha]

On 26/02/18 17:10, Tero Kristo wrote:
> Hi,
> 
> This patch adds clock rate propagation support for clkctrl clocks. This
> is needed on am33xx beaglebone black device at least, otherwise the
> display does not work. Similar problem appears to be present on am43xx
> but haven't heard of any reports of such so far, maybe Jyri can confirm
> this?
> 
> -Tero
> 
> --
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
> 

Tested-by: Jyri Sarha <jsarha@ti.com>

Displays on am335x-evm, Beaglebone-black, and Beaglebone with tfp410
dvi-cape work with these patches.

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH 0/3] clk: ti: add CLK_SET_RATE_PARENT support for clkctrl

[Tony Lindgren]

* Tero Kristo <t-kristo@ti.com> [180228 05:38]:
> On 27/02/18 18:48, Tony Lindgren wrote:
> > * Tony Lindgren <tony@atomide.com> [180227 16:43]:
> > > * Tero Kristo <t-kristo@ti.com> [180227 06:35]:
> > > > On 27/02/18 00:05, Tony Lindgren wrote:
> > > > > Hmm so should we have all the timers use bit 0 in the dtsi?
> > > > > Or default to bit 24 for all of them?
> > > > 
> > > > Who is going to control the clkctrl clock for the timers if you just control
> > > > the opt clock? Also, ain't the bit 24 the clksel mux setting? Tweaking that
> > > > would seem wrong...
> > > 
> > > Yeah OK.
> > 
> > $ git grep TIMER arch/arm/boot/dts/* | grep CLKCTRL
> > 
> > And that shows timer1 using bit 24 for omap4, omap5 and dra7
> > dtsi files.
> > 
> > So shouldn't that then be just bit 0 instead of bit 24 for
> > those? And then we let omap_dm_timer_init_one() reparent it?
> 
> Actually, that fck setting is because of this patch:
> 
> 138f7ca78f5a0677f591fdf23d0309c2f4774bf7
> ARM: OMAP2+: timer: add support for fetching fck handle from DT
> 
> So, you need to provide the clock handle at bit offset 24.
> 
> The main clkctrl clock is still handled via hwmod core, or via the
> interconnect driver.

Yeah but in timer.c we just need to enable the module and it's
clock and reparent if needed. There's nothing else to manage
there, omap_dm_timer_init_one() just queries the rate.

So I now think something is is wrong. For the interconnect target
module we need to provide the module clock (bit 0) in the dts,
not the source mux clock (bit 24).

Then omap_dm_timer_init_one() can configure the source clock
which is bit 24. My guess is that 138f7ca78f is really a
workaround for set_parent() not working properly for clkctrl
clock :)

Then a board can specify it's desired source clock for system
timer(s) by configure "assigned-lcok-parents" and set it to
32KiHz clock or SYS_CLK.

Regards,

Tony

Re: [PATCH 0/3] clk: ti: add CLK_SET_RATE_PARENT support for clkctrl

[Tero Kristo]

On 28/02/18 23:58, Tony Lindgren wrote:
> * Tero Kristo <t-kristo@ti.com> [180228 05:38]:
>> On 27/02/18 18:48, Tony Lindgren wrote:
>>> * Tony Lindgren <tony@atomide.com> [180227 16:43]:
>>>> * Tero Kristo <t-kristo@ti.com> [180227 06:35]:
>>>>> On 27/02/18 00:05, Tony Lindgren wrote:
>>>>>> Hmm so should we have all the timers use bit 0 in the dtsi?
>>>>>> Or default to bit 24 for all of them?
>>>>>
>>>>> Who is going to control the clkctrl clock for the timers if you just control
>>>>> the opt clock? Also, ain't the bit 24 the clksel mux setting? Tweaking that
>>>>> would seem wrong...
>>>>
>>>> Yeah OK.
>>>
>>> $ git grep TIMER arch/arm/boot/dts/* | grep CLKCTRL
>>>
>>> And that shows timer1 using bit 24 for omap4, omap5 and dra7
>>> dtsi files.
>>>
>>> So shouldn't that then be just bit 0 instead of bit 24 for
>>> those? And then we let omap_dm_timer_init_one() reparent it?
>>
>> Actually, that fck setting is because of this patch:
>>
>> 138f7ca78f5a0677f591fdf23d0309c2f4774bf7
>> ARM: OMAP2+: timer: add support for fetching fck handle from DT
>>
>> So, you need to provide the clock handle at bit offset 24.
>>
>> The main clkctrl clock is still handled via hwmod core, or via the
>> interconnect driver.
> 
> Yeah but in timer.c we just need to enable the module and it's
> clock and reparent if needed. There's nothing else to manage
> there, omap_dm_timer_init_one() just queries the rate.
> 
> So I now think something is is wrong. For the interconnect target
> module we need to provide the module clock (bit 0) in the dts,
> not the source mux clock (bit 24).
> 
> Then omap_dm_timer_init_one() can configure the source clock
> which is bit 24. My guess is that 138f7ca78f is really a
> workaround for set_parent() not working properly for clkctrl
> clock :)

set_parent() can't work for clkctrl clock, as it only has one parent. 
The mux is a separate component, so you need to fetch the parent of the 
clkctrl part and set the parent for that one; unless we want to 
implement some sort of composite clock support for it.

I'd say it is more like a transitional patch to support both legacy and 
clkctrl way of handling clocks. The patch can most likely be dropped 
once the transition is done, but this was the only way I could see how 
to get it fixed in short term.

> Then a board can specify it's desired source clock for system
> timer(s) by configure "assigned-lcok-parents" and set it to
> 32KiHz clock or SYS_CLK.

Yeah, that will definitely work.

-Tero
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH 0/3] clk: ti: add CLK_SET_RATE_PARENT support for clkctrl

[Tony Lindgren]

* Tero Kristo <t-kristo@ti.com> [180301 07:05]:
> On 28/02/18 23:58, Tony Lindgren wrote:
> > Then omap_dm_timer_init_one() can configure the source clock
> > which is bit 24. My guess is that 138f7ca78f is really a
> > workaround for set_parent() not working properly for clkctrl
> > clock :)
> 
> set_parent() can't work for clkctrl clock, as it only has one parent. The
> mux is a separate component, so you need to fetch the parent of the clkctrl
> part and set the parent for that one; unless we want to implement some sort
> of composite clock support for it.

Hmm OK probably good idea to avoid any composite clocks here :)

I guess in timer1 example, clkctrl bit 0 is gate for both GPT1_FCLK
and WKUP_L4_ICLK2? And then bit 24 sets the parent of WKUP_L4_ICLK2.
Or am I still confused?

So what should we call clkctrl bit 24 then? It seems we can have
up to 8 opt clocks and also "parent" clocks for the fck.

> I'd say it is more like a transitional patch to support both legacy and
> clkctrl way of handling clocks. The patch can most likely be dropped once
> the transition is done, but this was the only way I could see how to get it
> fixed in short term.

OK

> > Then a board can specify it's desired source clock for system
> > timer(s) by configure "assigned-lcok-parents" and set it to
> > 32KiHz clock or SYS_CLK.
> 
> Yeah, that will definitely work.

We can have those aliases for the timer driver, I guess we just
need a suitable name for the clkctrl parent bit(s).

Anyways, no objections to this series of fixes, just wondering:

Acked-by: Tony Lindgren <tony@atomide.com>