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

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

On Tue, 15 Feb 2022 at 13:24, Prasad Malisetty (Temp) (QUIC)
<quic_pmaliset@quicinc.com> wrote:
>
>
>
> -----Original Message-----
> From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Sent: Saturday, February 12, 2022 1:23 AM
> To: bjorn.andersson@linaro.org; Prasad Malisetty (Temp) (QUIC) <quic_pmaliset@quicinc.com>
> Cc: Andy Gross <agross@kernel.org>; Stanimir Varbanov <svarbanov@mm-sol.com>; Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>; Rob Herring <robh+dt@kernel.org>; Krzysztof Wilczy??ski <kw@linux.com>; Michael Turquette <mturquette@baylibre.com>; Stephen Boyd <swboyd@chromium.org>; Bjorn Helgaas <bhelgaas@google.com>; Prasad Malisetty <pmaliset@codeaurora.org>; Vinod Koul <vkoul@kernel.org>; linux-arm-msm@vger.kernel.org; linux-pci@vger.kernel.org; linux-clk@vger.kernel.org; devicetree@vger.kernel.org
> Subject: Re: [PATCH v2 03/11] clk: qcom: gdsc: add support for clocks tied to the GDSC
>
> 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
> >
>
> Hi  Dmitry,
>
> Greetings !!!
>
> Sorry for the inconvenience,  there was an issue with my mail so I couldn’t receive the updates properly. Now issue is resolved.
> I am in discussion with internal team to know more about this. I will update my comments after this discussion.

Prasad, any updates on this topic?

>
> Thanks
> -Prasad
>
> >> 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
> >>
>
>
> --
> With best wishes
> Dmitry



-- 
With best wishes
Dmitry