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@xxxxxxxxxxx>
> >>>>>> ---
> >>>>>> 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
> >>>>>>
> >>>>>
> >>>
> >
--
With best wishes
Dmitry
