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@xxxxxxxxxx/
> > 
> 
> 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