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

Commit Message

Dmitry Baryshkov Feb. 4, 2022, 2:46 p.m. UTC

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.

Cc: Prasad Malisetty <[e-mail]>
Signed-off-by: Dmitry Baryshkov <[e-mail]>
---
 drivers/clk/qcom/gdsc.c | 41 +++++++++++++++++++++++++++++++++++++++++
 drivers/clk/qcom/gdsc.h | 14 ++++++++++++++
 2 files changed, 55 insertions(+)

Comments

Bjorn Andersson Feb. 4, 2022, 10:05 p.m. UTC | #1

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 <[e-mail]>
> Signed-off-by: Dmitry Baryshkov <[e-mail]>
> ---
>  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
>

Dmitry Baryshkov Feb. 9, 2022, 10:41 p.m. UTC | #2

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?

Prasad, any comments?

> 
> Regards,
> Bjorn
> 
>> Cc: Prasad Malisetty <[e-mail]>
>> Signed-off-by: Dmitry Baryshkov <[e-mail]>
>> ---
>>   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
>>

Dmitry Baryshkov Feb. 11, 2022, 7:52 p.m. UTC | #3

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 <[e-mail]>
>> Signed-off-by: Dmitry Baryshkov <[e-mail]>
>> ---
>>   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
>>

Prasad Malisetty (Temp) (QUIC) Feb. 15, 2022, 10:24 a.m. UTC | #4

-----Original Message-----
From: Dmitry Baryshkov <[e-mail]> 
Sent: Saturday, February 12, 2022 1:23 AM
To: [e-mail]; Prasad Malisetty (Temp) (QUIC) <[e-mail]>
Cc: Andy Gross <[e-mail]>; Stanimir Varbanov <[e-mail]>; Lorenzo Pieralisi <[e-mail]>; Rob Herring <[e-mail]>; Krzysztof Wilczy??ski <[e-mail]>; Michael Turquette <[e-mail]>; Stephen Boyd <[e-mail]>; Bjorn Helgaas <[e-mail]>; Prasad Malisetty <[e-mail]>; Vinod Koul <[e-mail]>; [e-mail]; [e-mail]; [e-mail]; [e-mail]
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.

Thanks
-Prasad 

>> Cc: Prasad Malisetty <[e-mail]>
>> Signed-off-by: Dmitry Baryshkov <[e-mail]>
>> ---
>>   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
>>

Dmitry Baryshkov Feb. 25, 2022, 1:55 a.m. UTC | #5

On Tue, 15 Feb 2022 at 13:24, Prasad Malisetty (Temp) (QUIC)
<[e-mail]> wrote:
>
>
>
> -----Original Message-----
> From: Dmitry Baryshkov <[e-mail]>
> Sent: Saturday, February 12, 2022 1:23 AM
> To: [e-mail]; Prasad Malisetty (Temp) (QUIC) <[e-mail]>
> Cc: Andy Gross <[e-mail]>; Stanimir Varbanov <[e-mail]>; Lorenzo Pieralisi <[e-mail]>; Rob Herring <[e-mail]>; Krzysztof Wilczy??ski <[e-mail]>; Michael Turquette <[e-mail]>; Stephen Boyd <[e-mail]>; Bjorn Helgaas <[e-mail]>; Prasad Malisetty <[e-mail]>; Vinod Koul <[e-mail]>; [e-mail]; [e-mail]; [e-mail]; [e-mail]
> 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 <[e-mail]>
> >> Signed-off-by: Dmitry Baryshkov <[e-mail]>
> >> ---
> >>   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

Prasad Malisetty (Temp) (QUIC) March 1, 2022, 6:42 a.m. UTC | #6

++ 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 <[e-mail]>
>>> Signed-off-by: Dmitry Baryshkov <[e-mail]>
>>> ---
>>>   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
>>>
>
>

Dmitry Baryshkov March 1, 2022, 6:47 a.m. UTC | #7

Hi,

On Tue, 1 Mar 2022 at 09:42, Prasad Malisetty <[e-mail]> 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 <[e-mail]>
> >>> Signed-off-by: Dmitry Baryshkov <[e-mail]>
> >>> ---
> >>>   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
> >>>
> >
> >

Bjorn Andersson March 1, 2022, 5:43 p.m. UTC | #8

On Mon 28 Feb 22:47 PST 2022, Dmitry Baryshkov wrote:

> Hi,
> 
> On Tue, 1 Mar 2022 at 09:42, Prasad Malisetty <[e-mail]> 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?
> 

To me it really sounds like the need here is to "park" the pipe clock
source on bi_tcxo while the PHY isn't providing a valid clock signal
into GCC. If so "parking" the clock the same way as the rcg2_shared_ops
seems reasonable in that case.

Also, looking at downstream, the USB pipe clock seems to be handled in a
similar fashion.


But I'm still wondering what the actual requirement for the pipe clock
is. Per your description Prasad, it seems that the PHY doesn't need the
pipe clock coming back from GCC during initialization - and the PCIe
controller driver enables the pipe_clk after powering on the phy.

On platforms prior to there being a mux involved (e.g. SDM845) we have a
branch that is marked BRANCH_HALT_SKIP. But do we have that because we
incorrectly enable the gcc_pipe_clk before we power on the PHY?

I thought we did this because gcc_pipe_clk was part of some feedback
loop when calibrating the PHY PLL, but if sc7280 can feed tcxo that
doesn't make sense. Is the incoming pipe_clk part of the PHY
initialization or not?

Can we move the enablement of gcc_pipe_clk to be done after we bring up
the PHY and thereby drop the BRANCH_HALT_SKIP on these platforms?

Regards,
Bjorn

> >
> >   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 <[e-mail]>
> > >>> Signed-off-by: Dmitry Baryshkov <[e-mail]>
> > >>> ---
> > >>>   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

Patch

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,