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?
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
>
