| Message ID | 20240531090249.10293-5-quic_tdas@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for SA8775P Multimedia clock controllers | expand |
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. Best regards, Krzysztof
On 5/31/2024 2:59 AM, 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. Taniya - please separate fixes into another series?
On Fri, May 31, 2024 at 10:46:39AM GMT, Trilok Soni wrote: > On 5/31/2024 2:59 AM, 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. > > Taniya - please separate fixes into another series? > There's no problem including fixes in a series with other changes, but keeping them at the beginning of the series is generally preferred - as this both enable them being picked for some -fixes branch and avoid potential issues when backporting. Regards, Bjorn
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. Regards, Bjorn > > Best regards, > Krzysztof >
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
On 6/1/2024 9:08 PM, Bjorn Andersson wrote: >>> 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. >> Taniya - please separate fixes into another series? >> > There's no problem including fixes in a series with other changes, but > keeping them at the beginning of the series is generally preferred - as > this both enable them being picked for some -fixes branch and avoid > potential issues when backporting. Sure, but I prefer that these fixes are not blocked by open items and feedback to other drivers in the series. Once the fixes are reviewed and picked up independently of the drivers later in the series then I am fine.
On 6/2/2024 9:42 AM, 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. > Sure, Bjorn, will update the commit in the next series. 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. > Regards, > Bjorn > >> >> Best regards, >> Krzysztof >>
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 >
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. * 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, .ops = &clk_branch2_ops, }, }, @@ -294,7 +294,6 @@ static struct clk_branch gpu_cc_cb_clk = { .enable_mask = BIT(0), .hw.init = &(const struct clk_init_data){ .name = "gpu_cc_cb_clk", - .flags = CLK_IS_CRITICAL, .ops = &clk_branch2_ops, }, }, @@ -312,7 +311,7 @@ static struct clk_branch gpu_cc_crc_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, .ops = &clk_branch2_ops, }, }, @@ -330,7 +329,7 @@ static struct clk_branch gpu_cc_cx_ff_clk = { &gpu_cc_ff_clk_src.clkr.hw, }, .num_parents = 1, - .flags = CLK_SET_RATE_PARENT | CLK_IS_CRITICAL, + .flags = CLK_SET_RATE_PARENT, .ops = &clk_branch2_ops, }, }, @@ -348,7 +347,7 @@ static struct clk_branch gpu_cc_cx_gmu_clk = { &gpu_cc_gmu_clk_src.clkr.hw, }, .num_parents = 1, - .flags = CLK_SET_RATE_PARENT | CLK_IS_CRITICAL, + .flags = CLK_SET_RATE_PARENT, .ops = &clk_branch2_aon_ops, }, }, @@ -362,7 +361,6 @@ static struct clk_branch gpu_cc_cx_snoc_dvm_clk = { .enable_mask = BIT(0), .hw.init = &(const struct clk_init_data){ .name = "gpu_cc_cx_snoc_dvm_clk", - .flags = CLK_IS_CRITICAL, .ops = &clk_branch2_ops, }, }, @@ -380,7 +378,7 @@ static struct clk_branch gpu_cc_cxo_aon_clk = { &gpu_cc_xo_clk_src.clkr.hw, }, .num_parents = 1, - .flags = CLK_SET_RATE_PARENT | CLK_IS_CRITICAL, + .flags = CLK_SET_RATE_PARENT, .ops = &clk_branch2_ops, }, }, @@ -398,7 +396,7 @@ static struct clk_branch gpu_cc_cxo_clk = { &gpu_cc_xo_clk_src.clkr.hw, }, .num_parents = 1, - .flags = CLK_SET_RATE_PARENT | CLK_IS_CRITICAL, + .flags = CLK_SET_RATE_PARENT, .ops = &clk_branch2_ops, }, }, @@ -416,7 +414,7 @@ static struct clk_branch gpu_cc_demet_clk = { &gpu_cc_demet_div_clk_src.clkr.hw, }, .num_parents = 1, - .flags = CLK_SET_RATE_PARENT | CLK_IS_CRITICAL, + .flags = CLK_SET_RATE_PARENT, .ops = &clk_branch2_aon_ops, }, }, @@ -430,7 +428,6 @@ static struct clk_branch gpu_cc_hlos1_vote_gpu_smmu_clk = { .enable_mask = BIT(0), .hw.init = &(const struct clk_init_data){ .name = "gpu_cc_hlos1_vote_gpu_smmu_clk", - .flags = CLK_IS_CRITICAL, .ops = &clk_branch2_ops, }, }, @@ -448,7 +445,7 @@ static struct clk_branch gpu_cc_hub_aon_clk = { &gpu_cc_hub_clk_src.clkr.hw, }, .num_parents = 1, - .flags = CLK_SET_RATE_PARENT | CLK_IS_CRITICAL, + .flags = CLK_SET_RATE_PARENT, .ops = &clk_branch2_aon_ops, }, }, @@ -466,7 +463,7 @@ static struct clk_branch gpu_cc_hub_cx_int_clk = { &gpu_cc_hub_cx_int_div_clk_src.clkr.hw, }, .num_parents = 1, - .flags = CLK_SET_RATE_PARENT | CLK_IS_CRITICAL, + .flags = CLK_SET_RATE_PARENT, .ops = &clk_branch2_aon_ops, }, }, @@ -480,7 +477,6 @@ static struct clk_branch gpu_cc_memnoc_gfx_clk = { .enable_mask = BIT(0), .hw.init = &(const struct clk_init_data){ .name = "gpu_cc_memnoc_gfx_clk", - .flags = CLK_IS_CRITICAL, .ops = &clk_branch2_ops, }, }, @@ -494,7 +490,6 @@ static struct clk_branch gpu_cc_sleep_clk = { .enable_mask = BIT(0), .hw.init = &(const struct clk_init_data){ .name = "gpu_cc_sleep_clk", - .flags = CLK_IS_CRITICAL, .ops = &clk_branch2_ops, }, }, @@ -533,7 +528,7 @@ static struct gdsc cx_gdsc = { .name = "cx_gdsc", }, .pwrsts = PWRSTS_OFF_ON, - .flags = VOTABLE | RETAIN_FF_ENABLE | ALWAYS_ON, + .flags = VOTABLE | RETAIN_FF_ENABLE, }; static struct gdsc gx_gdsc = {
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(-)