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@xxxxxxxxxxx>
>>> ---
>>> 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
