Re: [PATCH v2 08/12] soc: mediatek: pm-domains: Add subsystem clocks
From: Enric Balletbo i Serra
Date: Mon Oct 26 2020 - 11:17:50 EST
Hi Matthias,
Thank you for your review.
On 2/10/20 11:04, Matthias Brugger wrote:
>
>
> On 01/10/2020 18:01, Enric Balletbo i Serra wrote:
>> From: Matthias Brugger <mbrugger@xxxxxxxx>
>>
>> For the bus protection operations, some subsystem clocks need to be enabled
>> before releasing the protection. This patch identifies the subsystem clocks
>> by it's name.
>>
>> Suggested-by: Weiyi Lu <weiyi.lu@xxxxxxxxxxxx>
>> [Adapted the patch to the mtk-pm-domains driver]
>> Signed-off-by: Matthias Brugger <mbrugger@xxxxxxxx>
>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@xxxxxxxxxxxxx>
>> ---
>>
>> Changes in v2:
>> - Use dev_err_probe if getting clocks fails, so an error is not printed
>> if is deferred.
>>
>> drivers/soc/mediatek/mtk-pm-domains.c | 89 ++++++++++++++++++++++-----
>> 1 file changed, 75 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/soc/mediatek/mtk-pm-domains.c
>> b/drivers/soc/mediatek/mtk-pm-domains.c
>> index e0a52d489fea..2075072f16da 100644
>> --- a/drivers/soc/mediatek/mtk-pm-domains.c
>> +++ b/drivers/soc/mediatek/mtk-pm-domains.c
> [...]
>> + for (i = 0; i < pd->num_subsys_clks; i++) {
>> + struct clk *clk = of_clk_get(node, i + clk_ind);
>> +
>> + if (IS_ERR(clk)) {
>> + ret = PTR_ERR(clk);
>> + dev_err_probe(scpsys->dev, PTR_ERR(clk),
>> + "%pOFn: failed to get clk at index %d: %d\n",
>> + node, i + clk_ind, ret);
>> + goto err_put_subsys_clocks;
>> + }
>> +
>> + pd->subsys_clks[i].clk = clk;
>> + }
>> +
>> + ret = clk_bulk_prepare(pd->num_subsys_clks, pd->subsys_clks);
>> + if (ret)
>> + goto err_put_subsys_clocks;
>> +
>
> Not sure if it really matters, but logically we should prepare the basic clocks
> before we prepare the subsystem clocks. And fix the error handling accordingly.
>
Changed in next version.
> Regards,
> Matthias
>
>> ret = clk_bulk_prepare(pd->num_clks, pd->clks);
>> if (ret)
>> - goto err_put_clocks;
>> + goto err_unprepare_subsys_clocks;
>> /*
>> * Initially turn on all domains to make the domains usable
>> @@ -462,6 +513,12 @@ static int scpsys_add_one_domain(struct scpsys *scpsys,
>> struct device_node *node
>> err_unprepare_clocks:
>> clk_bulk_unprepare(pd->num_clks, pd->clks);
>> +err_unprepare_subsys_clocks:
>> + clk_bulk_unprepare(pd->num_subsys_clks, pd->subsys_clks);
>> +err_put_subsys_clocks:
>> + clk_bulk_put(pd->num_subsys_clks, pd->subsys_clks);
>> + devm_kfree(scpsys->dev, pd->subsys_clks);
>> + pd->num_subsys_clks = 0;
>> err_put_clocks:
>> clk_bulk_put(pd->num_clks, pd->clks);
>> devm_kfree(scpsys->dev, pd->clks);
>> @@ -541,6 +598,10 @@ static void scpsys_remove_one_domain(struct scpsys_domain
>> *pd)
>> clk_bulk_unprepare(pd->num_clks, pd->clks);
>> clk_bulk_put(pd->num_clks, pd->clks);
>> pd->num_clks = 0;
>> +
>> + clk_bulk_unprepare(pd->num_subsys_clks, pd->subsys_clks);
>> + clk_bulk_put(pd->num_subsys_clks, pd->subsys_clks);
>> + pd->num_subsys_clks = 0;
>> }
>> static void scpsys_domain_cleanup(struct scpsys *scpsys)
>>