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

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

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...

Best regards,
Krzysztof