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

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

[Manivannan Sadhasivam]

This reverts commit 12d2a4769380f0dc9ba6f827839869db2b81ef00.

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,
 };
 
 static struct gdsc pcie_2b_gdsc = {
@@ -6797,7 +6793,7 @@ static struct gdsc pcie_2b_gdsc = {
 		.name = "pcie_2b_gdsc",
 	},
 	.pwrsts = PWRSTS_OFF_ON,
-	.flags = VOTABLE | ALWAYS_ON,
+	.flags = VOTABLE,
 };
 
 static struct gdsc pcie_3a_gdsc = {
@@ -6808,7 +6804,7 @@ static struct gdsc pcie_3a_gdsc = {
 		.name = "pcie_3a_gdsc",
 	},
 	.pwrsts = PWRSTS_OFF_ON,
-	.flags = VOTABLE | ALWAYS_ON,
+	.flags = VOTABLE,
 };
 
 static struct gdsc pcie_3b_gdsc = {
@@ -6819,7 +6815,7 @@ static struct gdsc pcie_3b_gdsc = {
 		.name = "pcie_3b_gdsc",
 	},
 	.pwrsts = PWRSTS_OFF_ON,
-	.flags = VOTABLE | ALWAYS_ON,
+	.flags = VOTABLE,
 };
 
 static struct gdsc pcie_4_gdsc = {
@@ -6830,7 +6826,7 @@ static struct gdsc pcie_4_gdsc = {
 		.name = "pcie_4_gdsc",
 	},
 	.pwrsts = PWRSTS_OFF_ON,
-	.flags = VOTABLE | ALWAYS_ON,
+	.flags = VOTABLE,
 };
 
 static struct gdsc ufs_card_gdsc = {
-- 
2.25.1

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

[Johan Hovold]

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

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

[Manivannan Sadhasivam]

On Thu, Jul 06, 2023 at 03:38:22PM +0200, Johan Hovold wrote:
> 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.
> 

Ok!

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

Doh! Yes, I missed that. Will add it in v2.

- Mani

> Johan

-- 
மணிவண்ணன் சதாசிவம்