[PATCH] clk: rockchip: add pclk_vio_grf to critical clock on the RK3399

[PATCH] clk: rockchip: add pclk_vio_grf to critical clock on the RK3399

[Xing Zheng]

The pclk_vio_grf supply power for GRF IOs, if it is disabled, will
cause abnormal operation of the GRF.

The clock tree of the pclk_vio like this:
             | --> pclk_vio_grf
... pclk_vio | --> pclk_mipi_dsi1
             | --> pclk_mipi_dsi0

and the pclk_mipi_dsi0 and pclk_mipi_dsi1 don't have the flag
CLK_IGNORE_UNUSED, and they will be disabled by clk_disable_unused
when startup:
clk_disable_unused
  --> clk_disable_unprepare
    --> clk_disable
      --> clk_core_disable(core->parent)

then, the pclk_vio_grf also is disabled. Therefore, we need to add
pclk_vio_grf to critical clock and avoid to disable pclk_vio and
pclk_vio_grf.

Tested-by: Yakir Yang <ykk@rock-chips.com>
Signed-off-by: Yakir Yang <ykk@rock-chips.com>
Signed-off-by: Brian Norris <briannorris@chromium.org>
Signed-off-by: Xing Zheng <zhengxing@rock-chips.com>
---

 drivers/clk/rockchip/clk-rk3399.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/clk/rockchip/clk-rk3399.c b/drivers/clk/rockchip/clk-rk3399.c
index b6742fa..7ecb12c3 100644
--- a/drivers/clk/rockchip/clk-rk3399.c
+++ b/drivers/clk/rockchip/clk-rk3399.c
@@ -1485,6 +1485,7 @@ static const char *const rk3399_cru_critical_clocks[] __initconst = {
 	"gpll_hclk_perilp1_src",
 	"gpll_aclk_perilp0_src",
 	"gpll_aclk_perihp_src",
+	"pclk_vio_grf",
 };
 
 static const char *const rk3399_pmucru_critical_clocks[] __initconst = {
-- 
1.7.9.5

Re: [PATCH] clk: rockchip: add pclk_vio_grf to critical clock on the RK3399

[Doug Anderson]

Xing,

On Sun, Jun 12, 2016 at 2:48 AM, Xing Zheng <zhengxing@rock-chips.com> wrote:
> The pclk_vio_grf supply power for GRF IOs, if it is disabled, will
> cause abnormal operation of the GRF.
>
> The clock tree of the pclk_vio like this:
>              | --> pclk_vio_grf
> ... pclk_vio | --> pclk_mipi_dsi1
>              | --> pclk_mipi_dsi0
>
> and the pclk_mipi_dsi0 and pclk_mipi_dsi1 don't have the flag
> CLK_IGNORE_UNUSED, and they will be disabled by clk_disable_unused
> when startup:
> clk_disable_unused
>   --> clk_disable_unprepare
>     --> clk_disable
>       --> clk_core_disable(core->parent)
>
> then, the pclk_vio_grf also is disabled. Therefore, we need to add
> pclk_vio_grf to critical clock and avoid to disable pclk_vio and
> pclk_vio_grf.
>
> Tested-by: Yakir Yang <ykk@rock-chips.com>
> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> Signed-off-by: Xing Zheng <zhengxing@rock-chips.com>
> ---
>
>  drivers/clk/rockchip/clk-rk3399.c |    1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/clk/rockchip/clk-rk3399.c b/drivers/clk/rockchip/clk-rk3399.c
> index b6742fa..7ecb12c3 100644
> --- a/drivers/clk/rockchip/clk-rk3399.c
> +++ b/drivers/clk/rockchip/clk-rk3399.c
> @@ -1485,6 +1485,7 @@ static const char *const rk3399_cru_critical_clocks[] __initconst = {
>         "gpll_hclk_perilp1_src",
>         "gpll_aclk_perilp0_src",
>         "gpll_aclk_perihp_src",
> +       "pclk_vio_grf",

This clock is only needed when doing video output (like eDP), right?
That means it is not really a critical clock.  Critical clocks are
supposed to be ones that are needed for the basic functioning of the
system and that can never be turned off in any circumstances.  In this
case, if someone were running a rk3399 device and didn't have any
video output they would want this clock off.

Can you figure out in exactly which circumstances this clock needs to
be on and then add a proper consumer of this clock?  For instance, if
this clock is needed whenever the VOP is outputting data, then the VOP
should be a client and should turn this clock on and off when video is
being output.  If this clock is needed whenever you access VOP
registers, then the VOP should be a client and turn this clock on
around register accesses.

-Doug

Re: [PATCH] clk: rockchip: add pclk_vio_grf to critical clock on the RK3399

[Xing Zheng]

Hi Doug,

On 2016年06月13日 05:32, Doug Anderson wrote:
> Xing,
>
> On Sun, Jun 12, 2016 at 2:48 AM, Xing Zheng<zhengxing@rock-chips.com>  wrote:
>> The pclk_vio_grf supply power for GRF IOs, if it is disabled, will
>> cause abnormal operation of the GRF.
>>
>> The clock tree of the pclk_vio like this:
>>               | --> pclk_vio_grf
>> ... pclk_vio | --> pclk_mipi_dsi1
>>               | --> pclk_mipi_dsi0
>>
>> and the pclk_mipi_dsi0 and pclk_mipi_dsi1 don't have the flag
>> CLK_IGNORE_UNUSED, and they will be disabled by clk_disable_unused
>> when startup:
>> clk_disable_unused
>>    --> clk_disable_unprepare
>>      --> clk_disable
>>        --> clk_core_disable(core->parent)
>>
>> then, the pclk_vio_grf also is disabled. Therefore, we need to add
>> pclk_vio_grf to critical clock and avoid to disable pclk_vio and
>> pclk_vio_grf.
>>
>> Tested-by: Yakir Yang<ykk@rock-chips.com>
>> Signed-off-by: Yakir Yang<ykk@rock-chips.com>
>> Signed-off-by: Brian Norris<briannorris@chromium.org>
>> Signed-off-by: Xing Zheng<zhengxing@rock-chips.com>
>> ---
>>
>>   drivers/clk/rockchip/clk-rk3399.c |    1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/clk/rockchip/clk-rk3399.c b/drivers/clk/rockchip/clk-rk3399.c
>> index b6742fa..7ecb12c3 100644
>> --- a/drivers/clk/rockchip/clk-rk3399.c
>> +++ b/drivers/clk/rockchip/clk-rk3399.c
>> @@ -1485,6 +1485,7 @@ static const char *const rk3399_cru_critical_clocks[] __initconst = {
>>          "gpll_hclk_perilp1_src",
>>          "gpll_aclk_perilp0_src",
>>          "gpll_aclk_perihp_src",
>> +       "pclk_vio_grf",
> This clock is only needed when doing video output (like eDP), right?
> That means it is not really a critical clock.  Critical clocks are
> supposed to be ones that are needed for the basic functioning of the
> system and that can never be turned off in any circumstances.  In this
> case, if someone were running a rk3399 device and didn't have any
> video output they would want this clock off.
>
> Can you figure out in exactly which circumstances this clock needs to
> be on and then add a proper consumer of this clock?  For instance, if
> this clock is needed whenever the VOP is outputting data, then the VOP
> should be a client and should turn this clock on and off when video is
> being output.  If this clock is needed whenever you access VOP
> registers, then the VOP should be a client and turn this clock on
> around register accesses.
>
> -Doug
>
Yes, the pclk_vio_grf is needed for doing video output.
andpclk_vio_grf supply for: grf_soc_con9, 20~26, grf_hdcp

 From our design folks, we have many GRF registers in different power 
domains,
and these GRF gates should be always enabled. In this case, we can avoid 
some
of the operations GRF registers exception problems, and it is only a 
very small
increase in  power consumption (aboult <=1ma).

I will refer the latest TRM to update a new patch for always enable 
these GRFs.

Please drop this patch.

Thanks.

-- 
- Xing Zheng

Re: [PATCH] clk: rockchip: add pclk_vio_grf to critical clock on the RK3399

[Xing Zheng]

Hi Doug,

On 2016年06月13日 11:10, Xing Zheng wrote:
> Hi Doug,
>
> On 2016年06月13日 05:32, Doug Anderson wrote:
>> Xing,
>>
>> On Sun, Jun 12, 2016 at 2:48 AM, Xing 
>> Zheng<zhengxing@rock-chips.com>  wrote:
>>> The pclk_vio_grf supply power for GRF IOs, if it is disabled, will
>>> cause abnormal operation of the GRF.
>>>
>>> The clock tree of the pclk_vio like this:
>>>               | --> pclk_vio_grf
>>> ... pclk_vio | --> pclk_mipi_dsi1
>>>               | --> pclk_mipi_dsi0
>>>
>>> and the pclk_mipi_dsi0 and pclk_mipi_dsi1 don't have the flag
>>> CLK_IGNORE_UNUSED, and they will be disabled by clk_disable_unused
>>> when startup:
>>> clk_disable_unused
>>>    --> clk_disable_unprepare
>>>      --> clk_disable
>>>        --> clk_core_disable(core->parent)
>>>
>>> then, the pclk_vio_grf also is disabled. Therefore, we need to add
>>> pclk_vio_grf to critical clock and avoid to disable pclk_vio and
>>> pclk_vio_grf.
>>>
>>> Tested-by: Yakir Yang<ykk@rock-chips.com>
>>> Signed-off-by: Yakir Yang<ykk@rock-chips.com>
>>> Signed-off-by: Brian Norris<briannorris@chromium.org>
>>> Signed-off-by: Xing Zheng<zhengxing@rock-chips.com>
>>> ---
>>>
>>>   drivers/clk/rockchip/clk-rk3399.c |    1 +
>>>   1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/clk/rockchip/clk-rk3399.c 
>>> b/drivers/clk/rockchip/clk-rk3399.c
>>> index b6742fa..7ecb12c3 100644
>>> --- a/drivers/clk/rockchip/clk-rk3399.c
>>> +++ b/drivers/clk/rockchip/clk-rk3399.c
>>> @@ -1485,6 +1485,7 @@ static const char *const 
>>> rk3399_cru_critical_clocks[] __initconst = {
>>>          "gpll_hclk_perilp1_src",
>>>          "gpll_aclk_perilp0_src",
>>>          "gpll_aclk_perihp_src",
>>> +       "pclk_vio_grf",
>> This clock is only needed when doing video output (like eDP), right?
>> That means it is not really a critical clock.  Critical clocks are
>> supposed to be ones that are needed for the basic functioning of the
>> system and that can never be turned off in any circumstances. In this
>> case, if someone were running a rk3399 device and didn't have any
>> video output they would want this clock off.
>>
>> Can you figure out in exactly which circumstances this clock needs to
>> be on and then add a proper consumer of this clock?  For instance, if
>> this clock is needed whenever the VOP is outputting data, then the VOP
>> should be a client and should turn this clock on and off when video is
>> being output.  If this clock is needed whenever you access VOP
>> registers, then the VOP should be a client and turn this clock on
>> around register accesses.
>>
>>
Additional, we are discussing that we should turn the "pclk_vio" on and 
off in
video drivers when the video consumer needs to this clock.

Thanks.

>>
> Yes, the pclk_vio_grf is needed for doing video output.
> andpclk_vio_grf supply for: grf_soc_con9, 20~26, grf_hdcp
>
> From our design folks, we have many GRF registers in different power 
> domains,
> and these GRF gates should be always enabled. In this case, we can 
> avoid some
> of the operations GRF registers exception problems, and it is only a 
> very small
> increase in  power consumption (aboult <=1ma).
>
> I will refer the latest TRM to update a new patch for always enable 
> these GRFs.
>
> Please drop this patch.

-- 
- Xing Zheng

Re: [PATCH] clk: rockchip: add pclk_vio_grf to critical clock on the RK3399

[Doug Anderson]

Xing,

On Sun, Jun 12, 2016 at 8:10 PM, Xing Zheng <zhengxing@rock-chips.com> wrote:
> Hi Doug,
>
>
> On 2016年06月13日 05:32, Doug Anderson wrote:
>>
>> Xing,
>>
>> On Sun, Jun 12, 2016 at 2:48 AM, Xing Zheng<zhengxing@rock-chips.com>
>> wrote:
>>>
>>> The pclk_vio_grf supply power for GRF IOs, if it is disabled, will
>>> cause abnormal operation of the GRF.
>>>
>>> The clock tree of the pclk_vio like this:
>>>               | --> pclk_vio_grf
>>> ... pclk_vio | --> pclk_mipi_dsi1
>>>               | --> pclk_mipi_dsi0
>>>
>>> and the pclk_mipi_dsi0 and pclk_mipi_dsi1 don't have the flag
>>> CLK_IGNORE_UNUSED, and they will be disabled by clk_disable_unused
>>> when startup:
>>> clk_disable_unused
>>>    --> clk_disable_unprepare
>>>      --> clk_disable
>>>        --> clk_core_disable(core->parent)
>>>
>>> then, the pclk_vio_grf also is disabled. Therefore, we need to add
>>> pclk_vio_grf to critical clock and avoid to disable pclk_vio and
>>> pclk_vio_grf.
>>>
>>> Tested-by: Yakir Yang<ykk@rock-chips.com>
>>> Signed-off-by: Yakir Yang<ykk@rock-chips.com>
>>> Signed-off-by: Brian Norris<briannorris@chromium.org>
>>> Signed-off-by: Xing Zheng<zhengxing@rock-chips.com>
>>> ---
>>>
>>>   drivers/clk/rockchip/clk-rk3399.c |    1 +
>>>   1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/clk/rockchip/clk-rk3399.c
>>> b/drivers/clk/rockchip/clk-rk3399.c
>>> index b6742fa..7ecb12c3 100644
>>> --- a/drivers/clk/rockchip/clk-rk3399.c
>>> +++ b/drivers/clk/rockchip/clk-rk3399.c
>>> @@ -1485,6 +1485,7 @@ static const char *const
>>> rk3399_cru_critical_clocks[] __initconst = {
>>>          "gpll_hclk_perilp1_src",
>>>          "gpll_aclk_perilp0_src",
>>>          "gpll_aclk_perihp_src",
>>> +       "pclk_vio_grf",
>>
>> This clock is only needed when doing video output (like eDP), right?
>> That means it is not really a critical clock.  Critical clocks are
>> supposed to be ones that are needed for the basic functioning of the
>> system and that can never be turned off in any circumstances.  In this
>> case, if someone were running a rk3399 device and didn't have any
>> video output they would want this clock off.
>>
>> Can you figure out in exactly which circumstances this clock needs to
>> be on and then add a proper consumer of this clock?  For instance, if
>> this clock is needed whenever the VOP is outputting data, then the VOP
>> should be a client and should turn this clock on and off when video is
>> being output.  If this clock is needed whenever you access VOP
>> registers, then the VOP should be a client and turn this clock on
>> around register accesses.
>>
>> -Doug
>>
> Yes, the pclk_vio_grf is needed for doing video output.
> andpclk_vio_grf supply for: grf_soc_con9, 20~26, grf_hdcp
>
> From our design folks, we have many GRF registers in different power
> domains,
> and these GRF gates should be always enabled. In this case, we can avoid
> some
> of the operations GRF registers exception problems, and it is only a very
> small
> increase in  power consumption (aboult <=1ma).

Even if it's not much power, it seems like we should still turn it off
and on in the right place.  Unless I'm mistaken it should be such a
simple patch provide the clock to the right driver and then get the
clock when appropriate.


> I will refer the latest TRM to update a new patch for always enable these
> GRFs.

Does that mean you're going to make these all critical clocks?  That
doesn't sound so great...

-Doug

Re: [PATCH] clk: rockchip: add pclk_vio_grf to critical clock on the RK3399

[Xing Zheng]

Hi Doug,

On 2016年06月14日 07:46, Doug Anderson wrote:
>
> Even if it's not much power, it seems like we should still turn it off
> and on in the right place.  Unless I'm mistaken it should be such a
> simple patch provide the clock to the right driver and then get the
> clock when appropriate.
Yes, I talked with Yakir and we intent to enable/disable the 
pclk_vio_grf in video drivers,
so this patch will be dropped.
>> I will refer the latest TRM to update a new patch for always enable these
>> GRFs.
> Does that mean you're going to make these all critical clocks?  That
> doesn't sound so great...
>
> -Doug
Maybe, I heard that they are removed in the updated TRM, but I have not 
got the TRM yet.
I will double check it, and it seems that you do not agree to remove 
these clock...

Thanks.

-- 
- Xing Zheng

Re: [PATCH] clk: rockchip: add pclk_vio_grf to critical clock on the RK3399

[Doug Anderson]

Hi,

On Mon, Jun 13, 2016 at 8:02 PM, Xing Zheng <zhengxing@rock-chips.com> wrote:
> Hi Doug,
>
> On 2016年06月14日 07:46, Doug Anderson wrote:
>>
>>
>> Even if it's not much power, it seems like we should still turn it off
>> and on in the right place.  Unless I'm mistaken it should be such a
>> simple patch provide the clock to the right driver and then get the
>> clock when appropriate.
>
> Yes, I talked with Yakir and we intent to enable/disable the pclk_vio_grf in
> video drivers,
> so this patch will be dropped.
>>>
>>> I will refer the latest TRM to update a new patch for always enable these
>>> GRFs.
>>
>> Does that mean you're going to make these all critical clocks?  That
>> doesn't sound so great...
>>
>> -Doug
>
> Maybe, I heard that they are removed in the updated TRM, but I have not got
> the TRM yet.
> I will double check it, and it seems that you do not agree to remove these
> clock...

Well, if it were to be removed from the TRM then that would be a
strong sign that the SoC designers think that this clock should never
ever be turned off.  If that were the case I don't think I could
object to leaving this clock on all the time.  Presumably then we'd
totally remove the clock from the clock tree and rely on firmware to
leave it on?  Technically removing this clock is not really
device-tree backward compatible, but I guess if there are no current
users...

...note: if the clock IS listed in the TRM and there's ever a chance
that we'd want to turn it off, it's much easier to set that up all
now.  Trying to later go in and decide that these clocks are no longer
"always on" gets into all sorts of weird device tree backward
compatibility corner cases.



-Doug

Re: [PATCH] clk: rockchip: add pclk_vio_grf to critical clock on the RK3399

[Heiko Stübner]

Am Montag, 13. Juni 2016, 20:49:39 schrieb Doug Anderson:
> Hi,
> 
> On Mon, Jun 13, 2016 at 8:02 PM, Xing Zheng <zhengxing@rock-chips.com> 
wrote:
> > Hi Doug,
> > 
> > On 2016年06月14日 07:46, Doug Anderson wrote:
> >> Even if it's not much power, it seems like we should still turn it off
> >> and on in the right place.  Unless I'm mistaken it should be such a
> >> simple patch provide the clock to the right driver and then get the
> >> clock when appropriate.
> > 
> > Yes, I talked with Yakir and we intent to enable/disable the pclk_vio_grf
> > in video drivers,
> > so this patch will be dropped.
> > 
> >>> I will refer the latest TRM to update a new patch for always enable
> >>> these
> >>> GRFs.
> >> 
> >> Does that mean you're going to make these all critical clocks?  That
> >> doesn't sound so great...
> >> 
> >> -Doug
> > 
> > Maybe, I heard that they are removed in the updated TRM, but I have not
> > got
> > the TRM yet.
> > I will double check it, and it seems that you do not agree to remove these
> > clock...
> 
> Well, if it were to be removed from the TRM then that would be a
> strong sign that the SoC designers think that this clock should never
> ever be turned off.  If that were the case I don't think I could
> object to leaving this clock on all the time.  Presumably then we'd
> totally remove the clock from the clock tree and rely on firmware to
> leave it on?  Technically removing this clock is not really
> device-tree backward compatible, but I guess if there are no current
> users...
> 
> ...note: if the clock IS listed in the TRM and there's ever a chance
> that we'd want to turn it off, it's much easier to set that up all
> now.  Trying to later go in and decide that these clocks are no longer
> "always on" gets into all sorts of weird device tree backward
> compatibility corner cases.

PCLK_VIO_GRF gets already exported as clock-id, so we already have the wired 
corner-case :-) . But we'll see how this plays out.