drivers/clk/qcom/gcc-qcs615.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
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>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
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
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
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
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. >
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
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
On 8/8/2025 5:48 PM, Dmitry Baryshkov wrote: > 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. Along with the floor functionality we require parking of the clock to safe source for SDCC clock. I am reusing the shared_floor_ops introduced recently for SAR2130P explicitly for SDCC clocks. > > If you are saying that we should be caching CFG value for all clock > controllers, then we should change instead the clk_rcg2_ops. > That is not required for all clock controllers and which ever clock controller's clock requires it we use rcg2_shared_ops which was updated to park the cfg. >> >>>> >>>> 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 >> > -- Thanks, Taniya Das
© 2016 - 2025 Red Hat, Inc.