[PATCH] clk: qcom: gcc: Update the SDCC clock to use shared_floor_ops

[PATCH] clk: qcom: gcc: Update the SDCC clock to use shared_floor_ops

[Taniya Das]

gcc_sdcc2_apps_clk_src: rcg didn't update its configuration" during
boot. This happens due to the floor_ops tries to update the rcg
configuration even if the clock is not enabled.
The shared_floor_ops ensures that the new parent configuration is
cached in the parked_cfg in the case where the clock is off.

Ensure to use the ops for the other SDCC clock instances as well.

Fixes: 39d6dcf67fe9 ("clk: qcom: gcc: Add support for QCS615 GCC clocks")
Signed-off-by: Taniya Das <taniya.das@oss.qualcomm.com>
---
 drivers/clk/qcom/gcc-qcs615.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/qcom/gcc-qcs615.c b/drivers/clk/qcom/gcc-qcs615.c
index 9695446bc2a3c81f63f6fc0c98d298270f3494cc..5b3b8dd4f114bdcb8911a9ce612c39a1c6e05b23 100644
--- a/drivers/clk/qcom/gcc-qcs615.c
+++ b/drivers/clk/qcom/gcc-qcs615.c
@@ -784,7 +784,7 @@ static struct clk_rcg2 gcc_sdcc1_apps_clk_src = {
 		.name = "gcc_sdcc1_apps_clk_src",
 		.parent_data = gcc_parent_data_1,
 		.num_parents = ARRAY_SIZE(gcc_parent_data_1),
-		.ops = &clk_rcg2_floor_ops,
+		.ops = &clk_rcg2_shared_floor_ops,
 	},
 };
 
@@ -806,7 +806,7 @@ static struct clk_rcg2 gcc_sdcc1_ice_core_clk_src = {
 		.name = "gcc_sdcc1_ice_core_clk_src",
 		.parent_data = gcc_parent_data_0,
 		.num_parents = ARRAY_SIZE(gcc_parent_data_0),
-		.ops = &clk_rcg2_floor_ops,
+		.ops = &clk_rcg2_shared_floor_ops,
 	},
 };
 
@@ -830,7 +830,7 @@ static struct clk_rcg2 gcc_sdcc2_apps_clk_src = {
 		.name = "gcc_sdcc2_apps_clk_src",
 		.parent_data = gcc_parent_data_8,
 		.num_parents = ARRAY_SIZE(gcc_parent_data_8),
-		.ops = &clk_rcg2_floor_ops,
+		.ops = &clk_rcg2_shared_floor_ops,
 	},
 };
 

---
base-commit: 5c5a10f0be967a8950a2309ea965bae54251b50e
change-id: 20250804-sdcc_rcg2_shared_ops-ea6e26185fe6

Best regards,
-- 
Taniya Das <taniya.das@oss.qualcomm.com>

Re: [PATCH] clk: qcom: gcc: Update the SDCC clock to use shared_floor_ops

[Dmitry Baryshkov]

On Mon, Aug 04, 2025 at 11:59:21PM +0530, Taniya Das wrote:
> gcc_sdcc2_apps_clk_src: rcg didn't update its configuration" during
> boot. This happens due to the floor_ops tries to update the rcg
> configuration even if the clock is not enabled.

This has been working for other platforms (I see Milos, SAR2130P,
SM6375, SC8280XP, SM8550, SM8650 using shared ops, all other platforms
seem to use non-shared ops). What's the difference? Should we switch all
platforms? Is it related to the hypervisor?

> The shared_floor_ops ensures that the new parent configuration is
> cached in the parked_cfg in the case where the clock is off.
> 
> Ensure to use the ops for the other SDCC clock instances as well.
> 
> Fixes: 39d6dcf67fe9 ("clk: qcom: gcc: Add support for QCS615 GCC clocks")
> Signed-off-by: Taniya Das <taniya.das@oss.qualcomm.com>
> ---
>  drivers/clk/qcom/gcc-qcs615.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 

-- 
With best wishes
Dmitry

Re: [PATCH] clk: qcom: gcc: Update the SDCC clock to use shared_floor_ops

[Taniya Das]

On 8/5/2025 10:52 AM, Dmitry Baryshkov wrote:
> On Mon, Aug 04, 2025 at 11:59:21PM +0530, Taniya Das wrote:
>> gcc_sdcc2_apps_clk_src: rcg didn't update its configuration" during
>> boot. This happens due to the floor_ops tries to update the rcg
>> configuration even if the clock is not enabled.
> 
> This has been working for other platforms (I see Milos, SAR2130P,
> SM6375, SC8280XP, SM8550, SM8650 using shared ops, all other platforms
> seem to use non-shared ops). What's the difference? Should we switch all
> platforms? Is it related to the hypervisor?
> 

If a set rate is called on a clock before clock enable, the
rcg2_shared_floor_ops ensures that the RCG configuration is skipped
unless the clock is ON and which is the correct behavior. Once the clock
is enabled, the parent would be taken care to be enabled. So it is
better to move to use shared_floor_ops.

Yes, I can submit to move all the shared_floor_ops.

>> The shared_floor_ops ensures that the new parent configuration is
>> cached in the parked_cfg in the case where the clock is off.
>>
>> Ensure to use the ops for the other SDCC clock instances as well.
>>
>> Fixes: 39d6dcf67fe9 ("clk: qcom: gcc: Add support for QCS615 GCC clocks")
>> Signed-off-by: Taniya Das <taniya.das@oss.qualcomm.com>
>> ---
>>  drivers/clk/qcom/gcc-qcs615.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
> 

-- 
Thanks,
Taniya Das

Re: [PATCH] clk: qcom: gcc: Update the SDCC clock to use shared_floor_ops

[Konrad Dybcio]

On 8/6/25 11:27 AM, Taniya Das wrote:
> 
> 
> On 8/5/2025 10:52 AM, Dmitry Baryshkov wrote:
>> On Mon, Aug 04, 2025 at 11:59:21PM +0530, Taniya Das wrote:
>>> gcc_sdcc2_apps_clk_src: rcg didn't update its configuration" during
>>> boot. This happens due to the floor_ops tries to update the rcg
>>> configuration even if the clock is not enabled.
>>
>> This has been working for other platforms (I see Milos, SAR2130P,
>> SM6375, SC8280XP, SM8550, SM8650 using shared ops, all other platforms
>> seem to use non-shared ops). What's the difference? Should we switch all
>> platforms? Is it related to the hypervisor?
>>
> 
> If a set rate is called on a clock before clock enable, the

Is this something we should just fix up the drivers not to do?

Konrad

Re: [PATCH] clk: qcom: gcc: Update the SDCC clock to use shared_floor_ops

[Taniya Das]

On 8/6/2025 3:00 PM, Konrad Dybcio wrote:
> On 8/6/25 11:27 AM, Taniya Das wrote:
>>
>>
>> On 8/5/2025 10:52 AM, Dmitry Baryshkov wrote:
>>> On Mon, Aug 04, 2025 at 11:59:21PM +0530, Taniya Das wrote:
>>>> gcc_sdcc2_apps_clk_src: rcg didn't update its configuration" during
>>>> boot. This happens due to the floor_ops tries to update the rcg
>>>> configuration even if the clock is not enabled.
>>>
>>> This has been working for other platforms (I see Milos, SAR2130P,
>>> SM6375, SC8280XP, SM8550, SM8650 using shared ops, all other platforms
>>> seem to use non-shared ops). What's the difference? Should we switch all
>>> platforms? Is it related to the hypervisor?
>>>
>>
>> If a set rate is called on a clock before clock enable, the
> 
> Is this something we should just fix up the drivers not to do?
> 

I do not think CCF has any such limitation where the clock should be
enabled and then a clock rate should be invoked. We should handle it
gracefully and that is what we have now when the caching capabilities
were added in the code. This has been already in our downstream drivers.

We can add the fix to do a check 'clk_hw_is_enabled(hw)' in the normal
rcg2_ops/rcg2_floor/ceil_ops as well, then we can use them.

AFAIK the eMMC framework has this code and this is not limited to drivers.

-- 
Thanks,
Taniya Das

Re: [PATCH] clk: qcom: gcc: Update the SDCC clock to use shared_floor_ops

[Konrad Dybcio]

On 8/6/25 11:39 AM, Taniya Das wrote:
> 
> 
> On 8/6/2025 3:00 PM, Konrad Dybcio wrote:
>> On 8/6/25 11:27 AM, Taniya Das wrote:
>>>
>>>
>>> On 8/5/2025 10:52 AM, Dmitry Baryshkov wrote:
>>>> On Mon, Aug 04, 2025 at 11:59:21PM +0530, Taniya Das wrote:
>>>>> gcc_sdcc2_apps_clk_src: rcg didn't update its configuration" during
>>>>> boot. This happens due to the floor_ops tries to update the rcg
>>>>> configuration even if the clock is not enabled.
>>>>
>>>> This has been working for other platforms (I see Milos, SAR2130P,
>>>> SM6375, SC8280XP, SM8550, SM8650 using shared ops, all other platforms
>>>> seem to use non-shared ops). What's the difference? Should we switch all
>>>> platforms? Is it related to the hypervisor?
>>>>
>>>
>>> If a set rate is called on a clock before clock enable, the
>>
>> Is this something we should just fix up the drivers not to do?
>>
> 
> I do not think CCF has any such limitation where the clock should be
> enabled and then a clock rate should be invoked. We should handle it
> gracefully and that is what we have now when the caching capabilities
> were added in the code. This has been already in our downstream drivers.

Should we do CFG caching on *all* RCGs to avoid having to scratch our
heads over which ops to use with each clock individually?

> 
> We can add the fix to do a check 'clk_hw_is_enabled(hw)' in the normal
> rcg2_ops/rcg2_floor/ceil_ops as well, then we can use them.

FWIW this is not the first time this issue has popped up..

I don't remember the details other than what I sent in the thread

https://lore.kernel.org/linux-arm-msm/20240427-topic-8450sdc2-v1-1-631cbb59e0e5@linaro.org/

Konrad
> 
> AFAIK the eMMC framework has this code and this is not limited to drivers.
> 

Re: [PATCH] clk: qcom: gcc: Update the SDCC clock to use shared_floor_ops

[Taniya Das]

On 8/7/2025 10:32 PM, Konrad Dybcio wrote:
> On 8/6/25 11:39 AM, Taniya Das wrote:
>>
>>
>> On 8/6/2025 3:00 PM, Konrad Dybcio wrote:
>>> On 8/6/25 11:27 AM, Taniya Das wrote:
>>>>
>>>>
>>>> On 8/5/2025 10:52 AM, Dmitry Baryshkov wrote:
>>>>> On Mon, Aug 04, 2025 at 11:59:21PM +0530, Taniya Das wrote:
>>>>>> gcc_sdcc2_apps_clk_src: rcg didn't update its configuration" during
>>>>>> boot. This happens due to the floor_ops tries to update the rcg
>>>>>> configuration even if the clock is not enabled.
>>>>>
>>>>> This has been working for other platforms (I see Milos, SAR2130P,
>>>>> SM6375, SC8280XP, SM8550, SM8650 using shared ops, all other platforms
>>>>> seem to use non-shared ops). What's the difference? Should we switch all
>>>>> platforms? Is it related to the hypervisor?
>>>>>
>>>>
>>>> If a set rate is called on a clock before clock enable, the
>>>
>>> Is this something we should just fix up the drivers not to do?
>>>
>>
>> I do not think CCF has any such limitation where the clock should be
>> enabled and then a clock rate should be invoked. We should handle it
>> gracefully and that is what we have now when the caching capabilities
>> were added in the code. This has been already in our downstream drivers.
> 
> Should we do CFG caching on *all* RCGs to avoid having to scratch our
> heads over which ops to use with each clock individually?
> 

Yes, Konrad, that’s definitely the cleanest approach. If you're okay
with it, we can proceed with the current change first and then follow up
with a broader cleanup of the rcg2 ops. As part of that, we can also
transition the relevant SDCC clock targets to use floor_ops. This way,
we can avoid the rcg configuration failure logs in the boot sequence on
QCS615.

>>
>> We can add the fix to do a check 'clk_hw_is_enabled(hw)' in the normal
>> rcg2_ops/rcg2_floor/ceil_ops as well, then we can use them.
> 
> FWIW this is not the first time this issue has popped up..
> 
> I don't remember the details other than what I sent in the thread
> 
> https://lore.kernel.org/linux-arm-msm/20240427-topic-8450sdc2-v1-1-631cbb59e0e5@linaro.org/
> 

Yes, but as I mentioned the new ops looks much cleaner, so wanted to
take this approach.

> Konrad
>>
>> AFAIK the eMMC framework has this code and this is not limited to drivers.
>>
> 

-- 
Thanks,
Taniya Das

Re: [PATCH] clk: qcom: gcc: Update the SDCC clock to use shared_floor_ops

[Dmitry Baryshkov]

On Fri, Aug 08, 2025 at 02:51:50PM +0530, Taniya Das wrote:
> 
> 
> On 8/7/2025 10:32 PM, Konrad Dybcio wrote:
> > On 8/6/25 11:39 AM, Taniya Das wrote:
> >>
> >>
> >> On 8/6/2025 3:00 PM, Konrad Dybcio wrote:
> >>> On 8/6/25 11:27 AM, Taniya Das wrote:
> >>>>
> >>>>
> >>>> On 8/5/2025 10:52 AM, Dmitry Baryshkov wrote:
> >>>>> On Mon, Aug 04, 2025 at 11:59:21PM +0530, Taniya Das wrote:
> >>>>>> gcc_sdcc2_apps_clk_src: rcg didn't update its configuration" during
> >>>>>> boot. This happens due to the floor_ops tries to update the rcg
> >>>>>> configuration even if the clock is not enabled.
> >>>>>
> >>>>> This has been working for other platforms (I see Milos, SAR2130P,
> >>>>> SM6375, SC8280XP, SM8550, SM8650 using shared ops, all other platforms
> >>>>> seem to use non-shared ops). What's the difference? Should we switch all
> >>>>> platforms? Is it related to the hypervisor?
> >>>>>
> >>>>
> >>>> If a set rate is called on a clock before clock enable, the
> >>>
> >>> Is this something we should just fix up the drivers not to do?
> >>>
> >>
> >> I do not think CCF has any such limitation where the clock should be
> >> enabled and then a clock rate should be invoked. We should handle it
> >> gracefully and that is what we have now when the caching capabilities
> >> were added in the code. This has been already in our downstream drivers.
> > 
> > Should we do CFG caching on *all* RCGs to avoid having to scratch our
> > heads over which ops to use with each clock individually?
> > 
> 
> Yes, Konrad, that’s definitely the cleanest approach. If you're okay
> with it, we can proceed with the current change first and then follow up
> with a broader cleanup of the rcg2 ops. As part of that, we can also
> transition the relevant SDCC clock targets to use floor_ops. This way,
> we can avoid the rcg configuration failure logs in the boot sequence on
> QCS615.

the rcg2_shared_ops have one main usecase - parking of the clock to the
safe source. If it is not required for the SDCC clock, then it is
incorrect to land this patch.

If you are saying that we should be caching CFG value for all clock
controllers, then we should change instead the clk_rcg2_ops.

> 
> >>
> >> We can add the fix to do a check 'clk_hw_is_enabled(hw)' in the normal
> >> rcg2_ops/rcg2_floor/ceil_ops as well, then we can use them.
> > 
> > FWIW this is not the first time this issue has popped up..
> > 
> > I don't remember the details other than what I sent in the thread
> > 
> > https://lore.kernel.org/linux-arm-msm/20240427-topic-8450sdc2-v1-1-631cbb59e0e5@linaro.org/
> > 
> 
> Yes, but as I mentioned the new ops looks much cleaner, so wanted to
> take this approach.
> 
> > Konrad
> >>
> >> AFAIK the eMMC framework has this code and this is not limited to drivers.
> >>
> > 
> 
> -- 
> Thanks,
> Taniya Das
> 

-- 
With best wishes
Dmitry