Re: [PATCH v1 2/3] clk: qcom: gdsc: enable external switchable power domain

[Dmitry Baryshkov <dmitry.baryshkov@linaro.org>]

On 14/10/2020 05:15, Stephen Boyd wrote:
> Quoting Dmitry Baryshkov (2020-10-05 15:59:13)
>> On SM8250 MDSS_GDSC (and respective dispcc clocks) are children of MMCX
>> power domain. MMCX needs to be enabled to be able to access GDSC
>> registers and to enable display clocks. Use dev_pm/opp to enable
>> corresponding power domain.
>>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
> 
> A general question is why is this done in the gdsc code instead of
> somewhere generic? It seems that genpds may need to change the
> performance state of other genpds. I vaguely recall that genpd supports
> connecting different power domains together so maybe this could all be
> handled in the genpd layer instead of here? Then a regulator could be
> put behind a genpd and similarly be connected to the gdsc and turned on
> before turning on the gdsc?

Basically because we need not only to enable the genpd, but also to set 
performance state. This would mean creating a separate regulator driver 
calling dev_pm_genpd_set_performance_state() from enable/disable paths.
Does that seem like a better solution to you?

> 
>>   drivers/clk/qcom/gdsc.c | 56 ++++++++++++++++++++++++++++++++++++++---
>>   drivers/clk/qcom/gdsc.h |  5 ++++
>>   2 files changed, 57 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h
>> index bd537438c793..d58575f8f25f 100644
>> --- a/drivers/clk/qcom/gdsc.h
>> +++ b/drivers/clk/qcom/gdsc.h
>> @@ -57,6 +57,11 @@ struct gdsc {
>>   
>>          const char                      *supply;
>>          struct regulator                *rsupply;
>> +
>> +       const char                      *domain;
>> +       unsigned int                    perf_idx;
>> +       struct device                   *pd_dev;
>> +       int                             pd_opp;
> 
> Please document these fields.

Will do.

-- 
With best wishes
Dmitry