Hi,
On Tue, 1 Mar 2022 at 09:42, Prasad Malisetty <quic_pmaliset@xxxxxxxxxxx> wrote:
> 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.
So... I assume that if we implement the enable/disable() ops in a way
similar to clk_rcg2_shared_ops, we can drop all manual handling of
pipe_clk sources.
Bjorn, Taniya WDYT?
>
> 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@xxxxxxxxxxxxxx>
> >>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> >>> ---
> >>> 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
