[PATCHv3,10/10] CLK: TI: always enable DESHDCP clock
diff mbox

Message ID 1430906938-26128-11-git-send-email-tomi.valkeinen@ti.com
State New
Headers show

Commit Message

Tomi Valkeinen May 6, 2015, 10:08 a.m. UTC
DESHDCP clock is needed on DRA7 based SoCs to enable the DSS IP. That
clock is an odd one, as it is not supposed to be any kind of core clock
for DSS, and we don't even support HDCP, but the clock is still needed
even for the HWMOD framework to be able to reset the DSS IP.

As there's no support for multiple core clocks in the HWMOD framework,
we don't have any obvious place to enable this clock when DSS IP is
being enabled.

Furthermore, the HDMI on OMAP5 DSS is the same as on DRA7, and OMAP5
does not have any such clock configuration bit. This suggests that on
OMAP5 the DESHDCP clock is always enabled, and for DRA7 we have the
possibility to gate it.

So, as we don't have any clean way to enable and disable the clock
based on the need, this patch enables the clock at boot time, making it
work similarly to OMAP5.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/clk/ti/clk-7xx.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Tero Kristo May 20, 2015, 11:47 a.m. UTC | #1
On 05/06/2015 01:08 PM, Tomi Valkeinen wrote:
> DESHDCP clock is needed on DRA7 based SoCs to enable the DSS IP. That
> clock is an odd one, as it is not supposed to be any kind of core clock
> for DSS, and we don't even support HDCP, but the clock is still needed
> even for the HWMOD framework to be able to reset the DSS IP.
>
> As there's no support for multiple core clocks in the HWMOD framework,
> we don't have any obvious place to enable this clock when DSS IP is
> being enabled.
>
> Furthermore, the HDMI on OMAP5 DSS is the same as on DRA7, and OMAP5
> does not have any such clock configuration bit. This suggests that on
> OMAP5 the DESHDCP clock is always enabled, and for DRA7 we have the
> possibility to gate it.
>
> So, as we don't have any clean way to enable and disable the clock
> based on the need, this patch enables the clock at boot time, making it
> work similarly to OMAP5.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>   drivers/clk/ti/clk-7xx.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clk/ti/clk-7xx.c b/drivers/clk/ti/clk-7xx.c
> index 2dd956b9affa..63b8323df918 100644
> --- a/drivers/clk/ti/clk-7xx.c
> +++ b/drivers/clk/ti/clk-7xx.c
> @@ -312,7 +312,7 @@ static struct ti_dt_clk dra7xx_clks[] = {
>   int __init dra7xx_dt_clk_init(void)
>   {
>   	int rc;
> -	struct clk *abe_dpll_mux, *sys_clkin2, *dpll_ck;
> +	struct clk *abe_dpll_mux, *sys_clkin2, *dpll_ck, *hdcp_ck;
>
>   	ti_dt_clocks_register(dra7xx_clks);
>
> @@ -348,5 +348,10 @@ int __init dra7xx_dt_clk_init(void)
>   	if (rc)
>   		pr_err("%s: failed to set USB_DPLL M2 OUT\n", __func__);
>
> +	hdcp_ck = clk_get_sys(NULL, "dss_deshdcp_clk");
> +	rc = clk_prepare_enable(hdcp_ck);
> +	if (rc)
> +		pr_err("%s: failed to set dss_deshdcp_clk\n", __func__);
> +
>   	return rc;
>   }
>

You should rather use the assigned-clock properties in DT to accomplish 
this, the manual clock tweaks under the drivers/clk/ti/clk-* files 
should be converted to DT setup also.

-Tero

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tero Kristo May 20, 2015, 11:50 a.m. UTC | #2
On 05/20/2015 02:47 PM, Tero Kristo wrote:
> On 05/06/2015 01:08 PM, Tomi Valkeinen wrote:
>> DESHDCP clock is needed on DRA7 based SoCs to enable the DSS IP. That
>> clock is an odd one, as it is not supposed to be any kind of core clock
>> for DSS, and we don't even support HDCP, but the clock is still needed
>> even for the HWMOD framework to be able to reset the DSS IP.
>>
>> As there's no support for multiple core clocks in the HWMOD framework,
>> we don't have any obvious place to enable this clock when DSS IP is
>> being enabled.
>>
>> Furthermore, the HDMI on OMAP5 DSS is the same as on DRA7, and OMAP5
>> does not have any such clock configuration bit. This suggests that on
>> OMAP5 the DESHDCP clock is always enabled, and for DRA7 we have the
>> possibility to gate it.
>>
>> So, as we don't have any clean way to enable and disable the clock
>> based on the need, this patch enables the clock at boot time, making it
>> work similarly to OMAP5.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> ---
>>   drivers/clk/ti/clk-7xx.c | 7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/ti/clk-7xx.c b/drivers/clk/ti/clk-7xx.c
>> index 2dd956b9affa..63b8323df918 100644
>> --- a/drivers/clk/ti/clk-7xx.c
>> +++ b/drivers/clk/ti/clk-7xx.c
>> @@ -312,7 +312,7 @@ static struct ti_dt_clk dra7xx_clks[] = {
>>   int __init dra7xx_dt_clk_init(void)
>>   {
>>       int rc;
>> -    struct clk *abe_dpll_mux, *sys_clkin2, *dpll_ck;
>> +    struct clk *abe_dpll_mux, *sys_clkin2, *dpll_ck, *hdcp_ck;
>>
>>       ti_dt_clocks_register(dra7xx_clks);
>>
>> @@ -348,5 +348,10 @@ int __init dra7xx_dt_clk_init(void)
>>       if (rc)
>>           pr_err("%s: failed to set USB_DPLL M2 OUT\n", __func__);
>>
>> +    hdcp_ck = clk_get_sys(NULL, "dss_deshdcp_clk");
>> +    rc = clk_prepare_enable(hdcp_ck);
>> +    if (rc)
>> +        pr_err("%s: failed to set dss_deshdcp_clk\n", __func__);
>> +
>>       return rc;
>>   }
>>
>
> You should rather use the assigned-clock properties in DT to accomplish
> this, the manual clock tweaks under the drivers/clk/ti/clk-* files
> should be converted to DT setup also.

Now that I sent this, I realize we only have support to set_parent / 
set_rate through the assigned-clock props, no enable. Any plans to 
extend this support Mike/Stephen?

-Tero

>
> -Tero
>

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Boyd May 20, 2015, 7:34 p.m. UTC | #3
On 05/20/15 04:50, Tero Kristo wrote:
>
>>>
>>> @@ -348,5 +348,10 @@ int __init dra7xx_dt_clk_init(void)
>>>       if (rc)
>>>           pr_err("%s: failed to set USB_DPLL M2 OUT\n", __func__);
>>>
>>> +    hdcp_ck = clk_get_sys(NULL, "dss_deshdcp_clk");
>>> +    rc = clk_prepare_enable(hdcp_ck);
>>> +    if (rc)
>>> +        pr_err("%s: failed to set dss_deshdcp_clk\n", __func__);
>>> +
>>>       return rc;
>>>   }
>>>
>>
>> You should rather use the assigned-clock properties in DT to accomplish
>> this, the manual clock tweaks under the drivers/clk/ti/clk-* files
>> should be converted to DT setup also.
>
> Now that I sent this, I realize we only have support to set_parent /
> set_rate through the assigned-clock props, no enable. Any plans to
> extend this support Mike/Stephen?
>
>

Enable falls under the "critical clocks" discussion that is ongoing. I
assume that this is some sort of critical clock that can't be turned off?
Paul Walmsley May 21, 2015, 3:06 a.m. UTC | #4
On Wed, 20 May 2015, Stephen Boyd wrote:

> On 05/20/15 04:50, Tero Kristo wrote:
> >
> >>>
> >>> @@ -348,5 +348,10 @@ int __init dra7xx_dt_clk_init(void)
> >>>       if (rc)
> >>>           pr_err("%s: failed to set USB_DPLL M2 OUT\n", __func__);
> >>>
> >>> +    hdcp_ck = clk_get_sys(NULL, "dss_deshdcp_clk");
> >>> +    rc = clk_prepare_enable(hdcp_ck);
> >>> +    if (rc)
> >>> +        pr_err("%s: failed to set dss_deshdcp_clk\n", __func__);
> >>> +
> >>>       return rc;
> >>>   }
> >>>
> >>
> >> You should rather use the assigned-clock properties in DT to accomplish
> >> this, the manual clock tweaks under the drivers/clk/ti/clk-* files
> >> should be converted to DT setup also.
> >
> > Now that I sent this, I realize we only have support to set_parent /
> > set_rate through the assigned-clock props, no enable. Any plans to
> > extend this support Mike/Stephen?
> >
> >
> 
> Enable falls under the "critical clocks" discussion that is ongoing. I
> assume that this is some sort of critical clock that can't be turned off?

It only needs to be enabled for this particular display IP subsystem to 
function:

http://marc.info/?l=linux-omap&m=142071550111482&w=2

I believe Tomi is taking this approach (enabling it unconditionally) to 
avoid adding support for a secondary IP block "main clock" to the hwmod 
code.  Apparently, the chips that contain this clock gating bit are not 
intended to be used for power-critical use cases, so there's not much 
motivation to switch it on and off with the display controller.


- Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tomi Valkeinen May 22, 2015, 6:27 a.m. UTC | #5
On 21/05/15 06:06, Paul Walmsley wrote:

>> Enable falls under the "critical clocks" discussion that is ongoing. I
>> assume that this is some sort of critical clock that can't be turned off?
> 
> It only needs to be enabled for this particular display IP subsystem to 
> function:
> 
> http://marc.info/?l=linux-omap&m=142071550111482&w=2
> 
> I believe Tomi is taking this approach (enabling it unconditionally) to 
> avoid adding support for a secondary IP block "main clock" to the hwmod 

Right. I don't think that would be a simple task (correct me if I'm
wrong), and that would all be only for this one IP on this particular
SoC type.

> code.  Apparently, the chips that contain this clock gating bit are not 
> intended to be used for power-critical use cases, so there's not much 
> motivation to switch it on and off with the display controller.

Even in power-critical use cases the the power use difference should be
negligible.

 Tomi
Tomi Valkeinen May 27, 2015, 9:11 a.m. UTC | #6
On 20/05/15 14:47, Tero Kristo wrote:
> On 05/06/2015 01:08 PM, Tomi Valkeinen wrote:
>> DESHDCP clock is needed on DRA7 based SoCs to enable the DSS IP. That
>> clock is an odd one, as it is not supposed to be any kind of core clock
>> for DSS, and we don't even support HDCP, but the clock is still needed
>> even for the HWMOD framework to be able to reset the DSS IP.
>>
>> As there's no support for multiple core clocks in the HWMOD framework,
>> we don't have any obvious place to enable this clock when DSS IP is
>> being enabled.
>>
>> Furthermore, the HDMI on OMAP5 DSS is the same as on DRA7, and OMAP5
>> does not have any such clock configuration bit. This suggests that on
>> OMAP5 the DESHDCP clock is always enabled, and for DRA7 we have the
>> possibility to gate it.
>>
>> So, as we don't have any clean way to enable and disable the clock
>> based on the need, this patch enables the clock at boot time, making it
>> work similarly to OMAP5.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> ---
>>   drivers/clk/ti/clk-7xx.c | 7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/ti/clk-7xx.c b/drivers/clk/ti/clk-7xx.c
>> index 2dd956b9affa..63b8323df918 100644
>> --- a/drivers/clk/ti/clk-7xx.c
>> +++ b/drivers/clk/ti/clk-7xx.c
>> @@ -312,7 +312,7 @@ static struct ti_dt_clk dra7xx_clks[] = {
>>   int __init dra7xx_dt_clk_init(void)
>>   {
>>       int rc;
>> -    struct clk *abe_dpll_mux, *sys_clkin2, *dpll_ck;
>> +    struct clk *abe_dpll_mux, *sys_clkin2, *dpll_ck, *hdcp_ck;
>>
>>       ti_dt_clocks_register(dra7xx_clks);
>>
>> @@ -348,5 +348,10 @@ int __init dra7xx_dt_clk_init(void)
>>       if (rc)
>>           pr_err("%s: failed to set USB_DPLL M2 OUT\n", __func__);
>>
>> +    hdcp_ck = clk_get_sys(NULL, "dss_deshdcp_clk");
>> +    rc = clk_prepare_enable(hdcp_ck);
>> +    if (rc)
>> +        pr_err("%s: failed to set dss_deshdcp_clk\n", __func__);
>> +
>>       return rc;
>>   }
>>
> 
> You should rather use the assigned-clock properties in DT to accomplish
> this, the manual clock tweaks under the drivers/clk/ti/clk-* files
> should be converted to DT setup also.

So what should we do?

I got a confirmation from a HW guy that this clock is always enabled on
OMAP5, and that's why we haven't seen these issues there. I believe
there's a HW bug related to this, as having the HDCP clock disabled
should not prevent the use of DSS, but it does.

In my opinion the bit should just be always set, which is what this
patch does. Although I think even adding a clock node is kind of extra,
as it's clear the bit cannot be properly used.

 Tomi
Mike Turquette May 28, 2015, 4:22 a.m. UTC | #7
Quoting Stephen Boyd (2015-05-20 12:34:23)
> On 05/20/15 04:50, Tero Kristo wrote:
> >
> >>>
> >>> @@ -348,5 +348,10 @@ int __init dra7xx_dt_clk_init(void)
> >>>       if (rc)
> >>>           pr_err("%s: failed to set USB_DPLL M2 OUT\n", __func__);
> >>>
> >>> +    hdcp_ck = clk_get_sys(NULL, "dss_deshdcp_clk");
> >>> +    rc = clk_prepare_enable(hdcp_ck);
> >>> +    if (rc)
> >>> +        pr_err("%s: failed to set dss_deshdcp_clk\n", __func__);
> >>> +
> >>>       return rc;
> >>>   }
> >>>
> >>
> >> You should rather use the assigned-clock properties in DT to accomplish
> >> this, the manual clock tweaks under the drivers/clk/ti/clk-* files
> >> should be converted to DT setup also.
> >
> > Now that I sent this, I realize we only have support to set_parent /
> > set_rate through the assigned-clock props, no enable. Any plans to
> > extend this support Mike/Stephen?
> >
> >
> 
> Enable falls under the "critical clocks" discussion that is ongoing. I
> assume that this is some sort of critical clock that can't be turned off?

Just chiming in on the "critical clock" discussion. I'm not planning to
merge something that lets Devicetree nodes call clk_enable on a clock.
That's what drivers are for.

The assigned-rate and assigned-parent stuff that Tero mentioned is more
like configuration data for a downstream clock consumer. Clock
gating/ungating does not fall under this type of configuration data in
my opinion.

I think that Tomi's patch to call clk_prepare_enable from
dra7xx_dt_clk_init is a reasonable solution to the problem.

Regards,
Mike

> 
> -- 
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tero Kristo May 28, 2015, 6:25 a.m. UTC | #8
On 05/28/2015 07:22 AM, Michael Turquette wrote:
> Quoting Stephen Boyd (2015-05-20 12:34:23)
>> On 05/20/15 04:50, Tero Kristo wrote:
>>>
>>>>>
>>>>> @@ -348,5 +348,10 @@ int __init dra7xx_dt_clk_init(void)
>>>>>        if (rc)
>>>>>            pr_err("%s: failed to set USB_DPLL M2 OUT\n", __func__);
>>>>>
>>>>> +    hdcp_ck = clk_get_sys(NULL, "dss_deshdcp_clk");
>>>>> +    rc = clk_prepare_enable(hdcp_ck);
>>>>> +    if (rc)
>>>>> +        pr_err("%s: failed to set dss_deshdcp_clk\n", __func__);
>>>>> +
>>>>>        return rc;
>>>>>    }
>>>>>
>>>>
>>>> You should rather use the assigned-clock properties in DT to accomplish
>>>> this, the manual clock tweaks under the drivers/clk/ti/clk-* files
>>>> should be converted to DT setup also.
>>>
>>> Now that I sent this, I realize we only have support to set_parent /
>>> set_rate through the assigned-clock props, no enable. Any plans to
>>> extend this support Mike/Stephen?
>>>
>>>
>>
>> Enable falls under the "critical clocks" discussion that is ongoing. I
>> assume that this is some sort of critical clock that can't be turned off?
>
> Just chiming in on the "critical clock" discussion. I'm not planning to
> merge something that lets Devicetree nodes call clk_enable on a clock.
> That's what drivers are for.
>
> The assigned-rate and assigned-parent stuff that Tero mentioned is more
> like configuration data for a downstream clock consumer. Clock
> gating/ungating does not fall under this type of configuration data in
> my opinion.
>
> I think that Tomi's patch to call clk_prepare_enable from
> dra7xx_dt_clk_init is a reasonable solution to the problem.

Yea, after this discussion I am fine with this approach also, seeing it 
apparently doesn't cause any ill side-effects.

-Tero

>
> Regards,
> Mike
>
>>
>> --
>> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
>> a Linux Foundation Collaborative Project
>>

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Boyd May 28, 2015, 10:49 p.m. UTC | #9
On 05/28, Tero Kristo wrote:
> On 05/28/2015 07:22 AM, Michael Turquette wrote:
> >Just chiming in on the "critical clock" discussion. I'm not planning to
> >merge something that lets Devicetree nodes call clk_enable on a clock.
> >That's what drivers are for.
> >
> >The assigned-rate and assigned-parent stuff that Tero mentioned is more
> >like configuration data for a downstream clock consumer. Clock
> >gating/ungating does not fall under this type of configuration data in
> >my opinion.
> >
> >I think that Tomi's patch to call clk_prepare_enable from
> >dra7xx_dt_clk_init is a reasonable solution to the problem.
> 
> Yea, after this discussion I am fine with this approach also, seeing
> it apparently doesn't cause any ill side-effects.

Well hopefully when the clk is prepared and enabled it isn't
orphaned. Or we're going to be in the same problem as we're
currently in with Sunxi and trying to make EPROBE_DEFER come out
of clk_get().
Mike Turquette June 3, 2015, 4:19 p.m. UTC | #10
Quoting Tomi Valkeinen (2015-05-06 03:08:58)
> DESHDCP clock is needed on DRA7 based SoCs to enable the DSS IP. That
> clock is an odd one, as it is not supposed to be any kind of core clock
> for DSS, and we don't even support HDCP, but the clock is still needed
> even for the HWMOD framework to be able to reset the DSS IP.
> 
> As there's no support for multiple core clocks in the HWMOD framework,
> we don't have any obvious place to enable this clock when DSS IP is
> being enabled.
> 
> Furthermore, the HDMI on OMAP5 DSS is the same as on DRA7, and OMAP5
> does not have any such clock configuration bit. This suggests that on
> OMAP5 the DESHDCP clock is always enabled, and for DRA7 we have the
> possibility to gate it.
> 
> So, as we don't have any clean way to enable and disable the clock
> based on the need, this patch enables the clock at boot time, making it
> work similarly to OMAP5.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

Acked-by: Michael Turquette <mturquette@linaro.org>

> ---
>  drivers/clk/ti/clk-7xx.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/ti/clk-7xx.c b/drivers/clk/ti/clk-7xx.c
> index 2dd956b9affa..63b8323df918 100644
> --- a/drivers/clk/ti/clk-7xx.c
> +++ b/drivers/clk/ti/clk-7xx.c
> @@ -312,7 +312,7 @@ static struct ti_dt_clk dra7xx_clks[] = {
>  int __init dra7xx_dt_clk_init(void)
>  {
>         int rc;
> -       struct clk *abe_dpll_mux, *sys_clkin2, *dpll_ck;
> +       struct clk *abe_dpll_mux, *sys_clkin2, *dpll_ck, *hdcp_ck;
>  
>         ti_dt_clocks_register(dra7xx_clks);
>  
> @@ -348,5 +348,10 @@ int __init dra7xx_dt_clk_init(void)
>         if (rc)
>                 pr_err("%s: failed to set USB_DPLL M2 OUT\n", __func__);
>  
> +       hdcp_ck = clk_get_sys(NULL, "dss_deshdcp_clk");
> +       rc = clk_prepare_enable(hdcp_ck);
> +       if (rc)
> +               pr_err("%s: failed to set dss_deshdcp_clk\n", __func__);
> +
>         return rc;
>  }
> -- 
> 2.1.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/drivers/clk/ti/clk-7xx.c b/drivers/clk/ti/clk-7xx.c
index 2dd956b9affa..63b8323df918 100644
--- a/drivers/clk/ti/clk-7xx.c
+++ b/drivers/clk/ti/clk-7xx.c
@@ -312,7 +312,7 @@  static struct ti_dt_clk dra7xx_clks[] = {
 int __init dra7xx_dt_clk_init(void)
 {
 	int rc;
-	struct clk *abe_dpll_mux, *sys_clkin2, *dpll_ck;
+	struct clk *abe_dpll_mux, *sys_clkin2, *dpll_ck, *hdcp_ck;
 
 	ti_dt_clocks_register(dra7xx_clks);
 
@@ -348,5 +348,10 @@  int __init dra7xx_dt_clk_init(void)
 	if (rc)
 		pr_err("%s: failed to set USB_DPLL M2 OUT\n", __func__);
 
+	hdcp_ck = clk_get_sys(NULL, "dss_deshdcp_clk");
+	rc = clk_prepare_enable(hdcp_ck);
+	if (rc)
+		pr_err("%s: failed to set dss_deshdcp_clk\n", __func__);
+
 	return rc;
 }