Re: [PATCH v2 03/11] clk: qcom: gdsc: add support for clocks tied to the GDSC

[Prasad Malisetty <quic_pmaliset@quicinc.com>]

++ Taniya

Hi Dmitry,

Greetings !!!

I discussed with internal team. setting gcc_pcie_n_pipe_clk src in pcie 
driver doesn't have any relation with gdsc.

But we are making sure that gcc_pcie_n_pipe_clk src is bi_tcxo before 
enabling the clocks and switching to pipe_clk src after PHY is enalbe.

During suspend switching back to bi_tcxo as we enabling the clock as 
part of resume.

  Hi Taniya,

Please provide your inputs.

Thanks

-Prasad
On 2/12/2022 1:22 AM, Dmitry Baryshkov wrote:
> On 05/02/2022 01:05, Bjorn Andersson wrote:
>> On Fri 04 Feb 08:46 CST 2022, Dmitry Baryshkov wrote:
>>
>>> On newer Qualcomm platforms GCC_PCIE_n_PIPE_CLK_SRC should be 
>>> controlled
>>> together with the PCIE_n_GDSC. The clock should be fed from the TCXO
>>> before switching the GDSC off and can be fed from PCIE_n_PIPE_CLK once
>>> the GDSC is on.
>>>
>>> Since commit aa9c0df98c29 ("PCI: qcom: Switch pcie_1_pipe_clk_src after
>>> PHY init in SC7280") PCIe controller driver tries to manage this on 
>>> it's
>>> own, resulting in the non-optimal code. Furthermore, if the any of the
>>> drivers will have the same requirements, the code would have to be
>>> dupliacted there.
>>>
>>> Move handling of such clocks to the GDSC code, providing special GDSC
>>> type.
>>>
>>
>> As discussed on IRC, I'm inclined not to take this, because looks to me
>> to be the same situation that we have with all GDSCs in SM8350 and
>> onwards - that some clocks must be parked on a safe parent before the
>> associated GDSC can be toggled.
>>
>> Prasad, please advice on what the actual requirements are wrt the
>> gcc_pipe_clk_src. When does it need to provide a valid signal and when
>> does it need to be parked?
>
> [Excuse me for the duplicate, Prasad's email was bouncing]
>
> Prasad, any comments?
>
>>
>> Regards,
>> Bjorn
>>
>>> Cc: Prasad Malisetty <pmaliset@codeaurora.org>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>>>   drivers/clk/qcom/gdsc.c | 41 
>>> +++++++++++++++++++++++++++++++++++++++++
>>>   drivers/clk/qcom/gdsc.h | 14 ++++++++++++++
>>>   2 files changed, 55 insertions(+)
>>>
>>> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
>>> index 7e1dd8ccfa38..9913d1b70947 100644
>>> --- a/drivers/clk/qcom/gdsc.c
>>> +++ b/drivers/clk/qcom/gdsc.c
>>> @@ -45,6 +45,7 @@
>>>   #define TIMEOUT_US        500
>>>     #define domain_to_gdsc(domain) container_of(domain, struct gdsc, 
>>> pd)
>>> +#define domain_to_pipe_clk_gdsc(domain) container_of(domain, struct 
>>> pipe_clk_gdsc, base.pd)
>>>     enum gdsc_status {
>>>       GDSC_OFF,
>>> @@ -549,3 +550,43 @@ int gdsc_gx_do_nothing_enable(struct 
>>> generic_pm_domain *domain)
>>>       return 0;
>>>   }
>>>   EXPORT_SYMBOL_GPL(gdsc_gx_do_nothing_enable);
>>> +
>>> +/*
>>> + * Special operations for GDSCs with attached pipe clocks.
>>> + * The clock should be parked to safe source (tcxo) before turning 
>>> off the GDSC
>>> + * and can be switched on as soon as the GDSC is on.
>>> + *
>>> + * We remove respective clock sources from clocks map and handle 
>>> them manually.
>>> + */
>>> +int gdsc_pipe_enable(struct generic_pm_domain *domain)
>>> +{
>>> +    struct pipe_clk_gdsc *sc = domain_to_pipe_clk_gdsc(domain);
>>> +    int i, ret;
>>> +
>>> +    ret = gdsc_enable(domain);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    for (i = 0; i< sc->num_clocks; i++)
>>> +        regmap_update_bits(sc->base.regmap, sc->clocks[i].reg,
>>> +                BIT(sc->clocks[i].shift + sc->clocks[i].width) - 
>>> BIT(sc->clocks[i].shift),
>>> +                sc->clocks[i].on_value << sc->clocks[i].shift);
>>> +
>>> +    return 0;
>>> +}
>>> +EXPORT_SYMBOL_GPL(gdsc_pipe_enable);
>>> +
>>> +int gdsc_pipe_disable(struct generic_pm_domain *domain)
>>> +{
>>> +    struct pipe_clk_gdsc *sc = domain_to_pipe_clk_gdsc(domain);
>>> +    int i;
>>> +
>>> +    for (i = sc->num_clocks - 1; i >= 0; i--)
>>> +        regmap_update_bits(sc->base.regmap, sc->clocks[i].reg,
>>> +                BIT(sc->clocks[i].shift + sc->clocks[i].width) - 
>>> BIT(sc->clocks[i].shift),
>>> +                sc->clocks[i].off_value << sc->clocks[i].shift);
>>> +
>>> +    /* In case of an error do not try turning the clocks again. We 
>>> can not be sure about the GDSC state. */
>>> +    return gdsc_disable(domain);
>>> +}
>>> +EXPORT_SYMBOL_GPL(gdsc_pipe_disable);
>>> diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h
>>> index d7cc4c21a9d4..b1a2f0abe41c 100644
>>> --- a/drivers/clk/qcom/gdsc.h
>>> +++ b/drivers/clk/qcom/gdsc.h
>>> @@ -68,11 +68,25 @@ struct gdsc_desc {
>>>       size_t num;
>>>   };
>>>   +struct pipe_clk_gdsc {
>>> +    struct gdsc base;
>>> +    int num_clocks;
>>> +    struct {
>>> +        u32 reg;
>>> +        u32 shift;
>>> +        u32 width;
>>> +        u32 off_value;
>>> +        u32 on_value;
>>> +    } clocks[];
>>> +};
>>> +
>>>   #ifdef CONFIG_QCOM_GDSC
>>>   int gdsc_register(struct gdsc_desc *desc, struct 
>>> reset_controller_dev *,
>>>             struct regmap *);
>>>   void gdsc_unregister(struct gdsc_desc *desc);
>>>   int gdsc_gx_do_nothing_enable(struct generic_pm_domain *domain);
>>> +int gdsc_pipe_enable(struct generic_pm_domain *domain);
>>> +int gdsc_pipe_disable(struct generic_pm_domain *domain);
>>>   #else
>>>   static inline int gdsc_register(struct gdsc_desc *desc,
>>>                   struct reset_controller_dev *rcdev,
>>> -- 
>>> 2.34.1
>>>
>
>