Re: [PATCH 2/4] clk: qcom: common: introduce qcom_cc_sync_state()

[Brian Masney <bmasney@redhat.com>]

Hi Konrad / Dmitry,

On Mon, Jun 15, 2026 at 11:04:49AM -0400, Brian Masney wrote:
> On Mon, Jun 15, 2026 at 04:51:35PM +0200, Konrad Dybcio wrote:
> > On 6/15/26 4:48 PM, Brian Masney wrote:
> > > On Mon, Jun 15, 2026 at 04:33:17PM +0200, Konrad Dybcio wrote:
> > >> On 6/15/26 4:24 PM, Brian Masney wrote:
> > >>> On Sun, Jun 07, 2026 at 01:30:03PM +0300, Dmitry Baryshkov wrote:
> > >>>> On Sun, Jun 07, 2026 at 01:43:06AM -0300, Val Packett wrote:
> > >>>>>
> > >>>>> On 6/6/26 8:15 AM, Dmitry Baryshkov wrote:
> > >>>>>> On Wed, Jun 03, 2026 at 10:21:47AM -0400, Brian Masney wrote:
> > >>>>>>> Several qcom clk providers currently have a sync_state helper set to
> > >>>>>>> icc_sync_state(). With an upcoming change to the clk framework, if
> > >>>>>>> sync_state is not defined for the device, then the clk framework sets it
> > >>>>>>> to clk_sync_state().
> > >>>>>>> [..]
> > >>>>>>> @@ -464,5 +466,12 @@ int qcom_cc_probe_by_index(struct platform_device *pdev, int index,
> > >>>>>>>   }
> > >>>>>>>   EXPORT_SYMBOL_GPL(qcom_cc_probe_by_index);
> > >>>>>>> +void qcom_cc_sync_state(struct device *dev)
> > >>>>>>> +{
> > >>>>>>> +	icc_sync_state(dev);
> > >>>>>> Only if desc->icc_hws != 0, otherwise it will mess the interconnect
> > >>>>>> internals. You might need to set drvdata to desc.
> > >>>>>
> > >>>>> Hmm…
> > >>>>>
> > >>>>> Currently icc_sync_state does not seem to use the dev argument at all.
> > >>>>>
> > >>>>> How would something get messed up, now or whenever icc_sync_state changes?
> > >>>>> o.0
> > >>>>
> > >>>> Yes :-(
> > >>>
> > >>> Sorry about the delayed response since I was out of town all last week.
> > >>> Just to be clear, the missing check for 'desc->icc_hws != 0' is a bug that
> > >>> existed prior to my change, and I should label it as such with a Fixes
> > >>> tag when I post my next version?
> > >>
> > >> Up until this change, having icc_hws > 0 but lacking icc_sync_state
> > >> (or the reverse) would be be considered programmer error
> > > 
> > > icc_hws > 0 but lacking icc_sync_state (or the reverse) makes sense as a
> > > programmer error. However...
> > > 
> > >> Starting with patch 4, this gets assigned unconditionally, so there's
> > >> no prior bug to be fixed
> > > 
> > > I don't see where that situation happens here. All of the places where
> > > icc_sync_state() was previously called, the new code now calls
> > > qcom_cc_sync_state() -> icc_sync_state(). (There is
> > > qcom_msm8996_cbf_icc_sync_state() that needs to be modified.)
> > > 
> > > In patch 4 of this series, it sets up a framework level sync_state()
> > > callback with dev_set_drv_sync_state(). If a sync_state already exists,
> > > then that call will fail with -EBUSY, and it will leave the existing
> > > sync_state() intact. So it's not calling sync_state twice. I will
> > > clarify that on the comment.
> > 
> > Dmitry and I are referring to the situation where the clock driver isn't
> > an interconnect provider but icc_sync_state() still executes. That could
> > not have been the case before, since most clock drivers didn't come with
> > any sort of .sync_state()
> 
> I'm sorry if I am being really dense here.
> 
> Let's ignore clk-cbf-8996.c since that has a separate issue. The other 6
> drivers in this patch today all have this pattern:
> 
> static struct platform_driver foo_driver = {
> 	...
>         .driver = {
> 		...
>                 .sync_state = icc_sync_state,
>         },
> };
> 
> I'm changing it to call qcom_cc_sync_state(), which will call
> icc_sync_state(). So everywhere where icc_sync_state() is called today
> it will still be called after the series is applied.
> 
> All of the other clk drivers will just call clk_sync_state() directly
> that's set at the framework level.

I posted a new version with the clk-cbf-8996.c fix, plus clarified a
comment on clk.c.

https://lore.kernel.org/linux-clk/20260616-clk-sync-state-v2-0-15f82c64d95c@redhat.com/T/#m23a1b61017d35020a311a83454331f758c386c8a

I'm not trying to ignore your feedback, but I'm not sure what's being
asked of me here since I believe what I have is correct. Anyways, let's
move this discussion to the v2. Thanks for your patience if I am being
overly dense here.

Brian