| Message ID | 20250218-videocc-pll-multi-pd-voting-v1-3-cfe6289ea29b@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | clk: qcom: Add support to attach multiple power domains in cc probe | expand |
On 2/18/2025 10:48 PM, Dmitry Baryshkov wrote: > On Tue, Feb 18, 2025 at 07:56:48PM +0530, Jagadeesh Kona wrote: >> Attach clock power domains in qcom_cc_really_probe() only >> if the clock controller has not already attached to them. > > Squash this to the previous patch and call the new function. No need to > duplicate the code. > I tried calling the new function here instead of duplicating code, but that is leading to below warning since the desc passed to qcom_cc_really_probe() has a const qualifier and hence we cannot update desc->pd_list inside qcom_cc_really_probe(). drivers/clk/qcom/common.c:305:33: WARNING : passing argument 2 of ‘qcom_cc_attach_pds’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers] Thanks, Jagadeesh >> >> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com> >> --- >> drivers/clk/qcom/common.c | 9 ++++++--- >> 1 file changed, 6 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c >> index ec27f70b24bdec24edd2f6b3df0d766fc1cdcbf0..eb7e2a56d1d135f839fd9bd470ba6231ce775a8c 100644 >> --- a/drivers/clk/qcom/common.c >> +++ b/drivers/clk/qcom/common.c >> @@ -300,9 +300,12 @@ int qcom_cc_really_probe(struct device *dev, >> if (!cc) >> return -ENOMEM; >> >> - ret = devm_pm_domain_attach_list(dev, NULL, &cc->pd_list); >> - if (ret < 0 && ret != -EEXIST) >> - return ret; >> + cc->pd_list = desc->pd_list; >> + if (!cc->pd_list) { >> + ret = devm_pm_domain_attach_list(dev, NULL, &cc->pd_list); >> + if (ret < 0 && ret != -EEXIST) >> + return ret; >> + } >> >> reset = &cc->reset; >> reset->rcdev.of_node = dev->of_node; >> >> -- >> 2.34.1 >> >
On Wed, Feb 19, 2025 at 05:06:11PM +0530, Jagadeesh Kona wrote: > > > On 2/18/2025 10:48 PM, Dmitry Baryshkov wrote: > > On Tue, Feb 18, 2025 at 07:56:48PM +0530, Jagadeesh Kona wrote: > >> Attach clock power domains in qcom_cc_really_probe() only > >> if the clock controller has not already attached to them. > > > > Squash this to the previous patch and call the new function. No need to > > duplicate the code. > > > > I tried calling the new function here instead of duplicating code, but that > is leading to below warning since the desc passed to qcom_cc_really_probe() > has a const qualifier and hence we cannot update desc->pd_list inside > qcom_cc_really_probe(). > > drivers/clk/qcom/common.c:305:33: WARNING : passing argument 2 of ‘qcom_cc_attach_pds’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers] It sounds like this can be fixed with a one-line patch. > > Thanks, > Jagadeesh > > >> > >> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com> > >> --- > >> drivers/clk/qcom/common.c | 9 ++++++--- > >> 1 file changed, 6 insertions(+), 3 deletions(-) > >> > >> diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c > >> index ec27f70b24bdec24edd2f6b3df0d766fc1cdcbf0..eb7e2a56d1d135f839fd9bd470ba6231ce775a8c 100644 > >> --- a/drivers/clk/qcom/common.c > >> +++ b/drivers/clk/qcom/common.c > >> @@ -300,9 +300,12 @@ int qcom_cc_really_probe(struct device *dev, > >> if (!cc) > >> return -ENOMEM; > >> > >> - ret = devm_pm_domain_attach_list(dev, NULL, &cc->pd_list); > >> - if (ret < 0 && ret != -EEXIST) > >> - return ret; > >> + cc->pd_list = desc->pd_list; > >> + if (!cc->pd_list) { > >> + ret = devm_pm_domain_attach_list(dev, NULL, &cc->pd_list); > >> + if (ret < 0 && ret != -EEXIST) > >> + return ret; > >> + } > >> > >> reset = &cc->reset; > >> reset->rcdev.of_node = dev->of_node; > >> > >> -- > >> 2.34.1 > >> > >
On 2/19/2025 5:27 PM, Dmitry Baryshkov wrote: > On Wed, Feb 19, 2025 at 05:06:11PM +0530, Jagadeesh Kona wrote: >> >> >> On 2/18/2025 10:48 PM, Dmitry Baryshkov wrote: >>> On Tue, Feb 18, 2025 at 07:56:48PM +0530, Jagadeesh Kona wrote: >>>> Attach clock power domains in qcom_cc_really_probe() only >>>> if the clock controller has not already attached to them. >>> >>> Squash this to the previous patch and call the new function. No need to >>> duplicate the code. >>> >> >> I tried calling the new function here instead of duplicating code, but that >> is leading to below warning since the desc passed to qcom_cc_really_probe() >> has a const qualifier and hence we cannot update desc->pd_list inside >> qcom_cc_really_probe(). >> >> drivers/clk/qcom/common.c:305:33: WARNING : passing argument 2 of ‘qcom_cc_attach_pds’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers] > > It sounds like this can be fixed with a one-line patch. > Removing const qualifier to qcom_cc_really_probe() will fix this, but that requires changes in many other drivers which are currently passing const descriptor to it. But I can squash this to my previous patch by updating my qcom_cc_attach_pds() function prototype as below and then calling that new function here -int qcom_cc_attach_pds(struct device *dev, struct qcom_cc_desc *desc) +int qcom_cc_attach_pds(struct device *dev, struct dev_pm_domain_list *pd_list) - ret = devm_pm_domain_attach_list(dev, NULL, &cc->pd_list); - if (ret < 0 && ret != -EEXIST) + ret = qcom_cc_attach_pds(dev, cc->pd_list); + if (ret) Thanks, Jagadeesh >> >> Thanks, >> Jagadeesh >> >>>> >>>> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com> >>>> --- >>>> drivers/clk/qcom/common.c | 9 ++++++--- >>>> 1 file changed, 6 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c >>>> index ec27f70b24bdec24edd2f6b3df0d766fc1cdcbf0..eb7e2a56d1d135f839fd9bd470ba6231ce775a8c 100644 >>>> --- a/drivers/clk/qcom/common.c >>>> +++ b/drivers/clk/qcom/common.c >>>> @@ -300,9 +300,12 @@ int qcom_cc_really_probe(struct device *dev, >>>> if (!cc) >>>> return -ENOMEM; >>>> >>>> - ret = devm_pm_domain_attach_list(dev, NULL, &cc->pd_list); >>>> - if (ret < 0 && ret != -EEXIST) >>>> - return ret; >>>> + cc->pd_list = desc->pd_list; >>>> + if (!cc->pd_list) { >>>> + ret = devm_pm_domain_attach_list(dev, NULL, &cc->pd_list); >>>> + if (ret < 0 && ret != -EEXIST) >>>> + return ret; >>>> + } >>>> >>>> reset = &cc->reset; >>>> reset->rcdev.of_node = dev->of_node; >>>> >>>> -- >>>> 2.34.1 >>>> >>> >
On Fri, Feb 21, 2025 at 05:12:58PM +0530, Jagadeesh Kona wrote: > > > On 2/20/2025 4:18 PM, Dmitry Baryshkov wrote: > > On Thu, Feb 20, 2025 at 12:43:42PM +0530, Jagadeesh Kona wrote: > >> > >> > >> On 2/19/2025 5:27 PM, Dmitry Baryshkov wrote: > >>> On Wed, Feb 19, 2025 at 05:06:11PM +0530, Jagadeesh Kona wrote: > >>>> > >>>> > >>>> On 2/18/2025 10:48 PM, Dmitry Baryshkov wrote: > >>>>> On Tue, Feb 18, 2025 at 07:56:48PM +0530, Jagadeesh Kona wrote: > >>>>>> Attach clock power domains in qcom_cc_really_probe() only > >>>>>> if the clock controller has not already attached to them. > >>>>> > >>>>> Squash this to the previous patch and call the new function. No need to > >>>>> duplicate the code. > >>>>> > >>>> > >>>> I tried calling the new function here instead of duplicating code, but that > >>>> is leading to below warning since the desc passed to qcom_cc_really_probe() > >>>> has a const qualifier and hence we cannot update desc->pd_list inside > >>>> qcom_cc_really_probe(). > >>>> > >>>> drivers/clk/qcom/common.c:305:33: WARNING : passing argument 2 of ‘qcom_cc_attach_pds’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers] > >>> > >>> It sounds like this can be fixed with a one-line patch. > >>> > >> > >> Removing const qualifier to qcom_cc_really_probe() will fix this, but that requires changes in > >> many other drivers which are currently passing const descriptor to it. > > > > And this points out that the pd_list should not be a part of the > > struct qcom_cc_desc. You are not using it in the code, so allocate that > > memory on the fly, pass it to devm_pm_domain_attach_list() and then > > forget about it. > > > > Above suggestion looks good, but we need to store the pd_list to pass it to GDSC driver to attach > the power domains as GDSC parent domains. Instead, we can add a new API wrapper for attaching PDs > and to map the regmap(qcom_cc_attach_pds_map) and all clock drivers that need multiple power domains > support can update to use below new API and all new clock drivers can just use the new API. > > The implementation would be something like below > > --- a/drivers/clk/qcom/common.c > +++ b/drivers/clk/qcom/common.c > +struct regmap *qcom_cc_attach_pds_map(struct platform_device *pdev, struct qcom_cc_desc *desc) I think it was established that qcom_cc_desc should be read-only. In the end it is a description. If you want to pass this data to gdsc probing, add new argument to qcom_cc_really_probe(). > +{ > + int ret; > + > + ret = devm_pm_domain_attach_list(&pdev->dev, NULL, &desc->pd_list); > + if (ret < 0 && ret != -EEXIST) > + return ERR_PTR(ret); > + > + return qcom_cc_map(pdev, desc); > +} > +EXPORT_SYMBOL_GPL(qcom_cc_attach_pds_map); > + > > > --- a/drivers/clk/qcom/videocc-sm8550.c > +++ b/drivers/clk/qcom/videocc-sm8550.c > @@ -542,6 +542,12 @@ static int video_cc_sm8550_probe(struct platform_device *pdev) > int ret; > u32 sleep_clk_offset = 0x8140; > > + regmap = qcom_cc_attach_pds_map(pdev, &video_cc_sm8550_desc); > + if (IS_ERR(regmap)) { > + pm_runtime_put(&pdev->dev); > + return PTR_ERR(regmap); > + } > + > ret = devm_pm_runtime_enable(&pdev->dev); > if (ret) > return ret; > @@ -550,12 +556,6 @@ static int video_cc_sm8550_probe(struct platform_device *pdev) > if (ret) > return ret; > > - regmap = qcom_cc_map(pdev, &video_cc_sm8550_desc); > - if (IS_ERR(regmap)) { > - pm_runtime_put(&pdev->dev); > - return PTR_ERR(regmap); > - } > - > > This way also, we are aligning more towards common code and the code will be uniform across all > clock drivers and this doesn't require separate callback in each individual clock driver. > > Thanks, > Jagadeesh > > >> > >> But I can squash this to my previous patch by updating my qcom_cc_attach_pds() function > >> prototype as below and then calling that new function here > >> > >> -int qcom_cc_attach_pds(struct device *dev, struct qcom_cc_desc *desc) > >> +int qcom_cc_attach_pds(struct device *dev, struct dev_pm_domain_list *pd_list) > >> > >> - ret = devm_pm_domain_attach_list(dev, NULL, &cc->pd_list); > >> - if (ret < 0 && ret != -EEXIST) > >> + ret = qcom_cc_attach_pds(dev, cc->pd_list); > >> + if (ret) > >> > >> Thanks, > >> Jagadeesh > >> > >>>> > >>>> Thanks, > >>>> Jagadeesh > >>>> > >>>>>> > >>>>>> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com> > >>>>>> --- > >>>>>> drivers/clk/qcom/common.c | 9 ++++++--- > >>>>>> 1 file changed, 6 insertions(+), 3 deletions(-) > >>>>>> > >>>>>> diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c > >>>>>> index ec27f70b24bdec24edd2f6b3df0d766fc1cdcbf0..eb7e2a56d1d135f839fd9bd470ba6231ce775a8c 100644 > >>>>>> --- a/drivers/clk/qcom/common.c > >>>>>> +++ b/drivers/clk/qcom/common.c > >>>>>> @@ -300,9 +300,12 @@ int qcom_cc_really_probe(struct device *dev, > >>>>>> if (!cc) > >>>>>> return -ENOMEM; > >>>>>> > >>>>>> - ret = devm_pm_domain_attach_list(dev, NULL, &cc->pd_list); > >>>>>> - if (ret < 0 && ret != -EEXIST) > >>>>>> - return ret; > >>>>>> + cc->pd_list = desc->pd_list; > >>>>>> + if (!cc->pd_list) { > >>>>>> + ret = devm_pm_domain_attach_list(dev, NULL, &cc->pd_list); > >>>>>> + if (ret < 0 && ret != -EEXIST) > >>>>>> + return ret; > >>>>>> + } > >>>>>> > >>>>>> reset = &cc->reset; > >>>>>> reset->rcdev.of_node = dev->of_node; > >>>>>> > >>>>>> -- > >>>>>> 2.34.1 > >>>>>> > >>>>> > >>> > >
diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c index ec27f70b24bdec24edd2f6b3df0d766fc1cdcbf0..eb7e2a56d1d135f839fd9bd470ba6231ce775a8c 100644 --- a/drivers/clk/qcom/common.c +++ b/drivers/clk/qcom/common.c @@ -300,9 +300,12 @@ int qcom_cc_really_probe(struct device *dev, if (!cc) return -ENOMEM; - ret = devm_pm_domain_attach_list(dev, NULL, &cc->pd_list); - if (ret < 0 && ret != -EEXIST) - return ret; + cc->pd_list = desc->pd_list; + if (!cc->pd_list) { + ret = devm_pm_domain_attach_list(dev, NULL, &cc->pd_list); + if (ret < 0 && ret != -EEXIST) + return ret; + } reset = &cc->reset; reset->rcdev.of_node = dev->of_node;
Attach clock power domains in qcom_cc_really_probe() only if the clock controller has not already attached to them. Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com> --- drivers/clk/qcom/common.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)