Re: [PATCH 04/13] clk: qcom: gpucc-sa8775p: Remove the CLK_IS_CRITICAL and ALWAYS_ON flags

[Taniya Das <quic_tdas@quicinc.com>]

On 6/2/2024 8:58 PM, Krzysztof Kozlowski wrote:
> On 02/06/2024 06:12, Bjorn Andersson wrote:
>> On Fri, May 31, 2024 at 11:59:04AM GMT, Krzysztof Kozlowski wrote:
>>> On 31/05/2024 11:02, Taniya Das wrote:
>>>> The gpu clocks and GDSC have been marked critical from the clock driver
>>>> which is not desired for functionality. Hence remove the CLK_IS_CRITICAL
>>>> and ALWAYS_ON flags.
>>>>
>>>> Fixes: 0afa16afc36d ("clk: qcom: add the GPUCC driver for sa8775p")
>>>> Signed-off-by: Taniya Das <quic_tdas@quicinc.com>
>>>> ---
>>>>   drivers/clk/qcom/gpucc-sa8775p.c | 27 +++++++++++----------------
>>>>   1 file changed, 11 insertions(+), 16 deletions(-)
>>>>
>>>> diff --git a/drivers/clk/qcom/gpucc-sa8775p.c b/drivers/clk/qcom/gpucc-sa8775p.c
>>>> index 1167c42da39d..f965babf4330 100644
>>>> --- a/drivers/clk/qcom/gpucc-sa8775p.c
>>>> +++ b/drivers/clk/qcom/gpucc-sa8775p.c
>>>> @@ -1,6 +1,6 @@
>>>>   // SPDX-License-Identifier: GPL-2.0-only
>>>>   /*
>>>> - * Copyright (c) 2021-2022, Qualcomm Innovation Center, Inc. All rights reserved.
>>>> + * Copyright (c) 2021-2022, 2024, Qualcomm Innovation Center, Inc. All rights reserved.
>>>
>>> That's not a fix.
>>>
>>>>    * Copyright (c) 2023, Linaro Limited
>>>>    */
>>>>   
>>>> @@ -280,7 +280,7 @@ static struct clk_branch gpu_cc_ahb_clk = {
>>>>   				&gpu_cc_hub_ahb_div_clk_src.clkr.hw,
>>>>   			},
>>>>   			.num_parents = 1,
>>>> -			.flags = CLK_SET_RATE_PARENT | CLK_IS_CRITICAL,
>>>> +			.flags = CLK_SET_RATE_PARENT,
>>>
>>> I fail to see why this is a fix. They were marked as critical on
>>> purpose. It was needed, wasn't it?
>>>
>>> Provide jsutification for commits, not just sprinkle Fixes tag all around.
>>>
>>
>> This is indeed a fix, as marking clocks CLK_IS_CRITICAL prevents any
>> power-domain associated with the clock controller from suspending. In
>> other words, the current behavior is broken.
>>
>> @Taniya, "not desired for functionality" does not carry any useful
>> information explaining why this change is made. Please update the commit
>> message.
> 
> Then please provide some sort of bug explanation in the commit msg. I
> assume the clocks were marked critical to solve some particular problem,
> e.g. missing parents, so marking this as fix sounds like both incorrect
> and error-prone when backported. Maybe that's not the case, so this is
> why there is commit msg to explain such details...
> 

Thanks for your review. Sure, I will update the next series with the 
below details.

The GPU clocks/GDSCs have been marked critical from the clock driver
but the GPU driver votes on these resources as per the HW requirement.
We don't require these clocks and GDSC's to be kept always ON which 
would have power impact and also GPU stability/corruptions.
Hence remove the CLK_IS_CRITICAL and ALWAYS_ON flags.

But I am not sure why the original author left the clocks/GDSCs always ON.


> Best regards,
> Krzysztof
> 

-- 
Thanks & Regards,
Taniya Das.