Re: [PATCH] Revert "clk: gcc-sc8280xp: keep PCIe power-domains always-on"

[Johan Hovold <johan@kernel.org>]

On Thu, Jul 06, 2023 at 06:17:00PM +0530, Manivannan Sadhasivam wrote:
> This reverts commit 12d2a4769380f0dc9ba6f827839869db2b81ef00.

Please update the commit summary and remove or rephrase the above as
direct reverts are typically used for patches that were broken or causes
trouble (e.g rephrase as "allow pcie gdsdc to be disabled" or similar).

The patch in question was correct at the time even if it may no longer
be required, but see below.

> With the minimal system suspend support in place for the PCIe driver that
> keeps the interconnect path active, the ALWAYS_ON flags can now be dropped.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  drivers/clk/qcom/gcc-sc8280xp.c | 14 +++++---------
>  1 file changed, 5 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/clk/qcom/gcc-sc8280xp.c b/drivers/clk/qcom/gcc-sc8280xp.c
> index 04a99dbaa57e..07eb6110442c 100644
> --- a/drivers/clk/qcom/gcc-sc8280xp.c
> +++ b/drivers/clk/qcom/gcc-sc8280xp.c
> @@ -6774,10 +6774,6 @@ static struct gdsc pcie_1_tunnel_gdsc = {
>  	.flags = VOTABLE,
>  };
>  
> -/*
> - * The Qualcomm PCIe driver does not yet implement suspend so to keep the
> - * PCIe power domains always-on for now.
> - */
>  static struct gdsc pcie_2a_gdsc = {
>  	.gdscr = 0x9d004,
>  	.collapse_ctrl = 0x52128,
> @@ -6786,7 +6782,7 @@ static struct gdsc pcie_2a_gdsc = {
>  		.name = "pcie_2a_gdsc",
>  	},
>  	.pwrsts = PWRSTS_OFF_ON,
> -	.flags = VOTABLE | ALWAYS_ON,
> +	.flags = VOTABLE,
>  };

Are you sure this is correct? Won't the controller state be lost if the
GDSC is powered off during suspend? Shouldn't this be PWRSTS_RET_ON at
least?

Johan