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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




-----Original Message-----
From: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx> 
Sent: Saturday, February 12, 2022 1:23 AM
To: bjorn.andersson@xxxxxxxxxx; Prasad Malisetty (Temp) (QUIC) <quic_pmaliset@xxxxxxxxxxx>
Cc: Andy Gross <agross@xxxxxxxxxx>; Stanimir Varbanov <svarbanov@xxxxxxxxxx>; Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx>; Rob Herring <robh+dt@xxxxxxxxxx>; Krzysztof Wilczy??ski <kw@xxxxxxxxx>; Michael Turquette <mturquette@xxxxxxxxxxxx>; Stephen Boyd <swboyd@xxxxxxxxxxxx>; Bjorn Helgaas <bhelgaas@xxxxxxxxxx>; Prasad Malisetty <pmaliset@xxxxxxxxxxxxxx>; Vinod Koul <vkoul@xxxxxxxxxx>; linux-arm-msm@xxxxxxxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; linux-clk@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx
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 <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




[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux