With PWRSTS_OFF_ON, PCIe GDSCs are turned off during gdsc_disable(). This can happen during scenarios such as system suspend and breaks the resume of PCIe controllers from suspend.
So use PWRSTS_RET_ON to indicate the GDSC driver to not turn off the GDSCs during gdsc_disable() and allow the hardware to transition the GDSCs to retention when the parent domain enters low power state during system suspend.
Signed-off-by: Krishna Chaitanya Chundru krishna.chundru@oss.qualcomm.com --- Krishna Chaitanya Chundru (7): clk: qcom: gcc-sc7280: Do not turn off PCIe GDSCs during gdsc_disable() clk: qcom: gcc-sa8775p: Do not turn off PCIe GDSCs during gdsc_disable() clk: qcom: gcc-sm8750: Do not turn off PCIe GDSCs during gdsc_disable() clk: qcom: gcc-glymur: Do not turn off PCIe GDSCs during gdsc_disable() clk: qcom: gcc-qcs8300: Do not turn off PCIe GDSCs during gdsc_disable() clk: qcom: gcc-x1e80100: Do not turn off PCIe GDSCs during gdsc_disable() clk: qcom: gcc-kaanapali: Do not turn off PCIe GDSCs during gdsc_disable()
drivers/clk/qcom/gcc-glymur.c | 16 ++++++++-------- drivers/clk/qcom/gcc-kaanapali.c | 2 +- drivers/clk/qcom/gcc-qcs8300.c | 4 ++-- drivers/clk/qcom/gcc-sa8775p.c | 4 ++-- drivers/clk/qcom/gcc-sc7280.c | 2 +- drivers/clk/qcom/gcc-sm8750.c | 2 +- drivers/clk/qcom/gcc-x1e80100.c | 16 ++++++++-------- 7 files changed, 23 insertions(+), 23 deletions(-) --- base-commit: 98e506ee7d10390b527aeddee7bbeaf667129646 change-id: 20260102-pci_gdsc_fix-1dcf08223922
Best regards,
With PWRSTS_OFF_ON, PCIe GDSCs are turned off during gdsc_disable(). This can happen during scenarios such as system suspend and breaks the resume of PCIe controllers from suspend.
So use PWRSTS_RET_ON to indicate the GDSC driver to not turn off the GDSCs during gdsc_disable() and allow the hardware to transition the GDSCs to retention when the parent domain enters low power state during system suspend.
Fixes: a3cc092196ef ("clk: qcom: Add Global Clock controller (GCC) driver for SC7280") Cc: stable@vger.kernel.org Signed-off-by: Krishna Chaitanya Chundru krishna.chundru@oss.qualcomm.com --- drivers/clk/qcom/gcc-sc7280.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/clk/qcom/gcc-sc7280.c b/drivers/clk/qcom/gcc-sc7280.c index 4502926a2691a773003631f822c121a043607a64..2432abcf487b9c813326adac24277054cc59cfa5 100644 --- a/drivers/clk/qcom/gcc-sc7280.c +++ b/drivers/clk/qcom/gcc-sc7280.c @@ -3101,7 +3101,7 @@ static struct gdsc gcc_pcie_0_gdsc = { .pd = { .name = "gcc_pcie_0_gdsc", }, - .pwrsts = PWRSTS_OFF_ON, + .pwrsts = PWRSTS_RET_ON, .flags = VOTABLE, };
With PWRSTS_OFF_ON, PCIe GDSCs are turned off during gdsc_disable(). This can happen during scenarios such as system suspend and breaks the resume of PCIe controllers from suspend.
So use PWRSTS_RET_ON to indicate the GDSC driver to not turn off the GDSCs during gdsc_disable() and allow the hardware to transition the GDSCs to retention when the parent domain enters low power state during system suspend.
Fixes: 08c51ceb12f7 ("clk: qcom: add the GCC driver for sa8775p") Cc: stable@vger.kernel.org Signed-off-by: Krishna Chaitanya Chundru krishna.chundru@oss.qualcomm.com --- drivers/clk/qcom/gcc-sa8775p.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/clk/qcom/gcc-sa8775p.c b/drivers/clk/qcom/gcc-sa8775p.c index e7425e82c54f2355015b58f5a25f11d2fb5020e6..b2e8639e9f09194fccde927466dab0f179e08e01 100644 --- a/drivers/clk/qcom/gcc-sa8775p.c +++ b/drivers/clk/qcom/gcc-sa8775p.c @@ -4211,7 +4211,7 @@ static struct gdsc pcie_0_gdsc = { .pd = { .name = "pcie_0_gdsc", }, - .pwrsts = PWRSTS_OFF_ON, + .pwrsts = PWRSTS_RET_ON, .flags = VOTABLE | RETAIN_FF_ENABLE | POLL_CFG_GDSCR, };
@@ -4225,7 +4225,7 @@ static struct gdsc pcie_1_gdsc = { .pd = { .name = "pcie_1_gdsc", }, - .pwrsts = PWRSTS_OFF_ON, + .pwrsts = PWRSTS_RET_ON, .flags = VOTABLE | RETAIN_FF_ENABLE | POLL_CFG_GDSCR, };
With PWRSTS_OFF_ON, PCIe GDSCs are turned off during gdsc_disable(). This can happen during scenarios such as system suspend and breaks the resume of PCIe controllers from suspend.
So use PWRSTS_RET_ON to indicate the GDSC driver to not turn off the GDSCs during gdsc_disable() and allow the hardware to transition the GDSCs to retention when the parent domain enters low power state during system suspend.
Fixes: 3267c774f3ff ("clk: qcom: Add support for GCC on SM8750") Cc: stable@vger.kernel.org Signed-off-by: Krishna Chaitanya Chundru krishna.chundru@oss.qualcomm.com --- drivers/clk/qcom/gcc-sm8750.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/clk/qcom/gcc-sm8750.c b/drivers/clk/qcom/gcc-sm8750.c index db81569dd4b17de1c70ab5058d4ea186e08ce09e..ef072e6e4d9aeac5bf24116407ec75aad290a571 100644 --- a/drivers/clk/qcom/gcc-sm8750.c +++ b/drivers/clk/qcom/gcc-sm8750.c @@ -2891,7 +2891,7 @@ static struct gdsc gcc_pcie_0_gdsc = { .pd = { .name = "gcc_pcie_0_gdsc", }, - .pwrsts = PWRSTS_OFF_ON, + .pwrsts = PWRSTS_RET_ON, .flags = POLL_CFG_GDSCR | RETAIN_FF_ENABLE | VOTABLE, };
With PWRSTS_OFF_ON, PCIe GDSCs are turned off during gdsc_disable(). This can happen during scenarios such as system suspend and breaks the resume of PCIe controllers from suspend.
So use PWRSTS_RET_ON to indicate the GDSC driver to not turn off the GDSCs during gdsc_disable() and allow the hardware to transition the GDSCs to retention when the parent domain enters low power state during system suspend.
Fixes: efe504300a17 ("clk: qcom: gcc: Add support for Global Clock Controller") Cc: stable@vger.kernel.org Signed-off-by: Krishna Chaitanya Chundru krishna.chundru@oss.qualcomm.com --- drivers/clk/qcom/gcc-glymur.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/clk/qcom/gcc-glymur.c b/drivers/clk/qcom/gcc-glymur.c index 238e205735ed594618b8526651968a4f73b1104e..5c66c1264f35b083d046d2c11f430f0f113001ef 100644 --- a/drivers/clk/qcom/gcc-glymur.c +++ b/drivers/clk/qcom/gcc-glymur.c @@ -7647,7 +7647,7 @@ static struct gdsc gcc_pcie_0_tunnel_gdsc = { .pd = { .name = "gcc_pcie_0_tunnel_gdsc", }, - .pwrsts = PWRSTS_OFF_ON, + .pwrsts = PWRSTS_RET_ON, .flags = POLL_CFG_GDSCR | RETAIN_FF_ENABLE | VOTABLE, };
@@ -7659,7 +7659,7 @@ static struct gdsc gcc_pcie_1_tunnel_gdsc = { .pd = { .name = "gcc_pcie_1_tunnel_gdsc", }, - .pwrsts = PWRSTS_OFF_ON, + .pwrsts = PWRSTS_RET_ON, .flags = POLL_CFG_GDSCR | RETAIN_FF_ENABLE | VOTABLE, };
@@ -7671,7 +7671,7 @@ static struct gdsc gcc_pcie_2_tunnel_gdsc = { .pd = { .name = "gcc_pcie_2_tunnel_gdsc", }, - .pwrsts = PWRSTS_OFF_ON, + .pwrsts = PWRSTS_RET_ON, .flags = POLL_CFG_GDSCR | RETAIN_FF_ENABLE | VOTABLE, };
@@ -7683,7 +7683,7 @@ static struct gdsc gcc_pcie_3a_gdsc = { .pd = { .name = "gcc_pcie_3a_gdsc", }, - .pwrsts = PWRSTS_OFF_ON, + .pwrsts = PWRSTS_RET_ON, .flags = POLL_CFG_GDSCR | RETAIN_FF_ENABLE | VOTABLE, };
@@ -7707,7 +7707,7 @@ static struct gdsc gcc_pcie_3b_gdsc = { .pd = { .name = "gcc_pcie_3b_gdsc", }, - .pwrsts = PWRSTS_OFF_ON, + .pwrsts = PWRSTS_RET_ON, .flags = POLL_CFG_GDSCR | RETAIN_FF_ENABLE | VOTABLE, };
@@ -7731,7 +7731,7 @@ static struct gdsc gcc_pcie_4_gdsc = { .pd = { .name = "gcc_pcie_4_gdsc", }, - .pwrsts = PWRSTS_OFF_ON, + .pwrsts = PWRSTS_RET_ON, .flags = POLL_CFG_GDSCR | RETAIN_FF_ENABLE | VOTABLE, };
@@ -7755,7 +7755,7 @@ static struct gdsc gcc_pcie_5_gdsc = { .pd = { .name = "gcc_pcie_5_gdsc", }, - .pwrsts = PWRSTS_OFF_ON, + .pwrsts = PWRSTS_RET_ON, .flags = POLL_CFG_GDSCR | RETAIN_FF_ENABLE | VOTABLE, };
@@ -7779,7 +7779,7 @@ static struct gdsc gcc_pcie_6_gdsc = { .pd = { .name = "gcc_pcie_6_gdsc", }, - .pwrsts = PWRSTS_OFF_ON, + .pwrsts = PWRSTS_RET_ON, .flags = POLL_CFG_GDSCR | RETAIN_FF_ENABLE | VOTABLE, };
With PWRSTS_OFF_ON, PCIe GDSCs are turned off during gdsc_disable(). This can happen during scenarios such as system suspend and breaks the resume of PCIe controllers from suspend.
So use PWRSTS_RET_ON to indicate the GDSC driver to not turn off the GDSCs during gdsc_disable() and allow the hardware to transition the GDSCs to retention when the parent domain enters low power state during system suspend.
Fixes: 95eeb2ffce73 ("clk: qcom: Add support for Global Clock Controller on QCS8300") Cc: stable@vger.kernel.org Signed-off-by: Krishna Chaitanya Chundru krishna.chundru@oss.qualcomm.com --- drivers/clk/qcom/gcc-qcs8300.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/clk/qcom/gcc-qcs8300.c b/drivers/clk/qcom/gcc-qcs8300.c index 80831c7dea3bcde0ced46054783df02b07a985db..009672b75fb9099cb0c6db7af3863654f2fa6648 100644 --- a/drivers/clk/qcom/gcc-qcs8300.c +++ b/drivers/clk/qcom/gcc-qcs8300.c @@ -3268,7 +3268,7 @@ static struct gdsc gcc_pcie_0_gdsc = { .pd = { .name = "gcc_pcie_0_gdsc", }, - .pwrsts = PWRSTS_OFF_ON, + .pwrsts = PWRSTS_RET_ON, .flags = VOTABLE | RETAIN_FF_ENABLE | POLL_CFG_GDSCR, };
@@ -3282,7 +3282,7 @@ static struct gdsc gcc_pcie_1_gdsc = { .pd = { .name = "gcc_pcie_1_gdsc", }, - .pwrsts = PWRSTS_OFF_ON, + .pwrsts = PWRSTS_RET_ON, .flags = VOTABLE | RETAIN_FF_ENABLE | POLL_CFG_GDSCR, };
With PWRSTS_OFF_ON, PCIe GDSCs are turned off during gdsc_disable(). This can happen during scenarios such as system suspend and breaks the resume of PCIe controllers from suspend.
So use PWRSTS_RET_ON to indicate the GDSC driver to not turn off the GDSCs during gdsc_disable() and allow the hardware to transition the GDSCs to retention when the parent domain enters low power state during system suspend.
Fixes: 161b7c401f4b ("clk: qcom: Add Global Clock controller (GCC) driver for X1E80100") Cc: stable@vger.kernel.org Signed-off-by: Krishna Chaitanya Chundru krishna.chundru@oss.qualcomm.com --- drivers/clk/qcom/gcc-x1e80100.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/clk/qcom/gcc-x1e80100.c b/drivers/clk/qcom/gcc-x1e80100.c index e46e65e631513e315de2f663f3dab73e1eb70604..d659d988660ea5e548fcae6f9f2a9a25081e6dda 100644 --- a/drivers/clk/qcom/gcc-x1e80100.c +++ b/drivers/clk/qcom/gcc-x1e80100.c @@ -6490,7 +6490,7 @@ static struct gdsc gcc_pcie_0_tunnel_gdsc = { .pd = { .name = "gcc_pcie_0_tunnel_gdsc", }, - .pwrsts = PWRSTS_OFF_ON, + .pwrsts = PWRSTS_RET_ON, .flags = POLL_CFG_GDSCR | RETAIN_FF_ENABLE | VOTABLE, };
@@ -6502,7 +6502,7 @@ static struct gdsc gcc_pcie_1_tunnel_gdsc = { .pd = { .name = "gcc_pcie_1_tunnel_gdsc", }, - .pwrsts = PWRSTS_OFF_ON, + .pwrsts = PWRSTS_RET_ON, .flags = POLL_CFG_GDSCR | RETAIN_FF_ENABLE | VOTABLE, };
@@ -6514,7 +6514,7 @@ static struct gdsc gcc_pcie_2_tunnel_gdsc = { .pd = { .name = "gcc_pcie_2_tunnel_gdsc", }, - .pwrsts = PWRSTS_OFF_ON, + .pwrsts = PWRSTS_RET_ON, .flags = POLL_CFG_GDSCR | RETAIN_FF_ENABLE | VOTABLE, };
@@ -6526,7 +6526,7 @@ static struct gdsc gcc_pcie_3_gdsc = { .pd = { .name = "gcc_pcie_3_gdsc", }, - .pwrsts = PWRSTS_OFF_ON, + .pwrsts = PWRSTS_RET_ON, .flags = POLL_CFG_GDSCR | RETAIN_FF_ENABLE | VOTABLE, };
@@ -6550,7 +6550,7 @@ static struct gdsc gcc_pcie_4_gdsc = { .pd = { .name = "gcc_pcie_4_gdsc", }, - .pwrsts = PWRSTS_OFF_ON, + .pwrsts = PWRSTS_RET_ON, .flags = POLL_CFG_GDSCR | RETAIN_FF_ENABLE | VOTABLE, };
@@ -6574,7 +6574,7 @@ static struct gdsc gcc_pcie_5_gdsc = { .pd = { .name = "gcc_pcie_5_gdsc", }, - .pwrsts = PWRSTS_OFF_ON, + .pwrsts = PWRSTS_RET_ON, .flags = POLL_CFG_GDSCR | RETAIN_FF_ENABLE | VOTABLE, };
@@ -6610,7 +6610,7 @@ static struct gdsc gcc_pcie_6a_gdsc = { .pd = { .name = "gcc_pcie_6a_gdsc", }, - .pwrsts = PWRSTS_OFF_ON, + .pwrsts = PWRSTS_RET_ON, .flags = POLL_CFG_GDSCR | RETAIN_FF_ENABLE | VOTABLE, };
@@ -6622,7 +6622,7 @@ static struct gdsc gcc_pcie_6b_gdsc = { .pd = { .name = "gcc_pcie_6b_gdsc", }, - .pwrsts = PWRSTS_OFF_ON, + .pwrsts = PWRSTS_RET_ON, .flags = POLL_CFG_GDSCR | RETAIN_FF_ENABLE | VOTABLE, };
On 02/01/2026 09:43, Krishna Chaitanya Chundru wrote:
With PWRSTS_OFF_ON, PCIe GDSCs are turned off during gdsc_disable(). This can happen during scenarios such as system suspend and breaks the resume of PCIe controllers from suspend.
So use PWRSTS_RET_ON to indicate the GDSC driver to not turn off the GDSCs during gdsc_disable() and allow the hardware to transition the GDSCs to retention when the parent domain enters low power state during system suspend.
Fixes: 161b7c401f4b ("clk: qcom: Add Global Clock controller (GCC) driver for X1E80100") Cc: stable@vger.kernel.org Signed-off-by: Krishna Chaitanya Chundru krishna.chundru@oss.qualcomm.com
drivers/clk/qcom/gcc-x1e80100.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/clk/qcom/gcc-x1e80100.c b/drivers/clk/qcom/gcc-x1e80100.c index e46e65e631513e315de2f663f3dab73e1eb70604..d659d988660ea5e548fcae6f9f2a9a25081e6dda 100644 --- a/drivers/clk/qcom/gcc-x1e80100.c +++ b/drivers/clk/qcom/gcc-x1e80100.c @@ -6490,7 +6490,7 @@ static struct gdsc gcc_pcie_0_tunnel_gdsc = { .pd = { .name = "gcc_pcie_0_tunnel_gdsc", },
- .pwrsts = PWRSTS_OFF_ON,
- .pwrsts = PWRSTS_RET_ON, .flags = POLL_CFG_GDSCR | RETAIN_FF_ENABLE | VOTABLE, };
@@ -6502,7 +6502,7 @@ static struct gdsc gcc_pcie_1_tunnel_gdsc = { .pd = { .name = "gcc_pcie_1_tunnel_gdsc", },
- .pwrsts = PWRSTS_OFF_ON,
- .pwrsts = PWRSTS_RET_ON, .flags = POLL_CFG_GDSCR | RETAIN_FF_ENABLE | VOTABLE, };
@@ -6514,7 +6514,7 @@ static struct gdsc gcc_pcie_2_tunnel_gdsc = { .pd = { .name = "gcc_pcie_2_tunnel_gdsc", },
- .pwrsts = PWRSTS_OFF_ON,
- .pwrsts = PWRSTS_RET_ON, .flags = POLL_CFG_GDSCR | RETAIN_FF_ENABLE | VOTABLE, };
@@ -6526,7 +6526,7 @@ static struct gdsc gcc_pcie_3_gdsc = { .pd = { .name = "gcc_pcie_3_gdsc", },
- .pwrsts = PWRSTS_OFF_ON,
- .pwrsts = PWRSTS_RET_ON, .flags = POLL_CFG_GDSCR | RETAIN_FF_ENABLE | VOTABLE, };
@@ -6550,7 +6550,7 @@ static struct gdsc gcc_pcie_4_gdsc = { .pd = { .name = "gcc_pcie_4_gdsc", },
- .pwrsts = PWRSTS_OFF_ON,
- .pwrsts = PWRSTS_RET_ON, .flags = POLL_CFG_GDSCR | RETAIN_FF_ENABLE | VOTABLE, };
@@ -6574,7 +6574,7 @@ static struct gdsc gcc_pcie_5_gdsc = { .pd = { .name = "gcc_pcie_5_gdsc", },
- .pwrsts = PWRSTS_OFF_ON,
- .pwrsts = PWRSTS_RET_ON, .flags = POLL_CFG_GDSCR | RETAIN_FF_ENABLE | VOTABLE, };
@@ -6610,7 +6610,7 @@ static struct gdsc gcc_pcie_6a_gdsc = { .pd = { .name = "gcc_pcie_6a_gdsc", },
- .pwrsts = PWRSTS_OFF_ON,
- .pwrsts = PWRSTS_RET_ON, .flags = POLL_CFG_GDSCR | RETAIN_FF_ENABLE | VOTABLE, };
@@ -6622,7 +6622,7 @@ static struct gdsc gcc_pcie_6b_gdsc = { .pd = { .name = "gcc_pcie_6b_gdsc", },
- .pwrsts = PWRSTS_OFF_ON,
- .pwrsts = PWRSTS_RET_ON, .flags = POLL_CFG_GDSCR | RETAIN_FF_ENABLE | VOTABLE, };
Retitle patch "Switch PCIe GDSCs to retention mode" you're really telling firmware to switch these GDSCs to retention mode in suspend - "power collapse" might be a better word.
Anyway I think you should switch from a "don't switch off" to a positive and more accurate description of what you're doing which is switching the GDSCs to retention mode.
With that
Reviewed-by: Bryan O'Donoghue bryan.odonoghue@linaro.org
For the series.
--- bod
With PWRSTS_OFF_ON, PCIe GDSCs are turned off during gdsc_disable(). This can happen during scenarios such as system suspend and breaks the resume of PCIe controllers from suspend.
So use PWRSTS_RET_ON to indicate the GDSC driver to not turn off the GDSCs during gdsc_disable() and allow the hardware to transition the GDSCs to retention when the parent domain enters low power state during system suspend.
Fixes: d1919c375f21 ("clk: qcom: Add support for Global clock controller on Kaanapali") Cc: stable@vger.kernel.org Signed-off-by: Krishna Chaitanya Chundru krishna.chundru@oss.qualcomm.com --- drivers/clk/qcom/gcc-kaanapali.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/clk/qcom/gcc-kaanapali.c b/drivers/clk/qcom/gcc-kaanapali.c index 182b152df14c252035fb28adb2e652bbfa22114a..1bae1c9dbc7764996e7c0228f9fab72d5e630cfa 100644 --- a/drivers/clk/qcom/gcc-kaanapali.c +++ b/drivers/clk/qcom/gcc-kaanapali.c @@ -3141,7 +3141,7 @@ static struct gdsc gcc_pcie_0_gdsc = { .pd = { .name = "gcc_pcie_0_gdsc", }, - .pwrsts = PWRSTS_OFF_ON, + .pwrsts = PWRSTS_RET_ON, .flags = POLL_CFG_GDSCR | RETAIN_FF_ENABLE | VOTABLE, };
On 1/2/26 10:43 AM, Krishna Chaitanya Chundru wrote:
With PWRSTS_OFF_ON, PCIe GDSCs are turned off during gdsc_disable(). This can happen during scenarios such as system suspend and breaks the resume of PCIe controllers from suspend.
Isn't turning the GDSCs off what we want though? At least during system suspend?
Konrad
On 1/2/2026 5:04 PM, Konrad Dybcio wrote:
On 1/2/26 10:43 AM, Krishna Chaitanya Chundru wrote:
With PWRSTS_OFF_ON, PCIe GDSCs are turned off during gdsc_disable(). This can happen during scenarios such as system suspend and breaks the resume of PCIe controllers from suspend.
Isn't turning the GDSCs off what we want though? At least during system suspend?
If we are keeping link in D3cold it makes sense, but currently we are not keeping in D3cold so we don't expect them to get off.
- Krishna Chaitanya.
Konrad
On 1/2/26 12:36 PM, Krishna Chaitanya Chundru wrote:
On 1/2/2026 5:04 PM, Konrad Dybcio wrote:
On 1/2/26 10:43 AM, Krishna Chaitanya Chundru wrote:
With PWRSTS_OFF_ON, PCIe GDSCs are turned off during gdsc_disable(). This can happen during scenarios such as system suspend and breaks the resume of PCIe controllers from suspend.
Isn't turning the GDSCs off what we want though? At least during system suspend?
If we are keeping link in D3cold it makes sense, but currently we are not keeping in D3cold so we don't expect them to get off.
Since we seem to be tackling that in parallel, it seems to make sense that adding a mechanism to let the PCIe driver select "on" vs "ret" vs "off" could be useful for us
FWIW I recall I could turn off the GDSCs on at least makena with the old suspend patches and the controllers would come back to life afterwards
Konrad
On 1/2/2026 5:09 PM, Konrad Dybcio wrote:
On 1/2/26 12:36 PM, Krishna Chaitanya Chundru wrote:
On 1/2/2026 5:04 PM, Konrad Dybcio wrote:
On 1/2/26 10:43 AM, Krishna Chaitanya Chundru wrote:
With PWRSTS_OFF_ON, PCIe GDSCs are turned off during gdsc_disable(). This can happen during scenarios such as system suspend and breaks the resume of PCIe controllers from suspend.
Isn't turning the GDSCs off what we want though? At least during system suspend?
If we are keeping link in D3cold it makes sense, but currently we are not keeping in D3cold so we don't expect them to get off.
Since we seem to be tackling that in parallel, it seems to make sense that adding a mechanism to let the PCIe driver select "on" vs "ret" vs "off" could be useful for us
At least I am not aware of such API where we can tell genpd not to turn off gdsc at runtime if we are keeping the device in D3cold state. But anyway the PCIe gdsc supports Retention, in that case adding this flag here makes more sense as it represents HW. sm8450,sm8650 also had similar problem which are fixed by mani[1].
FWIW I recall I could turn off the GDSCs on at least makena with the old suspend patches and the controllers would come back to life afterwards
In the suspend patches, we are keeping link in D3cold, so turning off gdsc will not have any effect. But if some reason we skipped D3cold like in S2idle case then gdsc should not be off, in that case in resume PCIe link will be broken.
Link [1]: clk: qcom: gcc-sm8650: Do not turn off PCIe GDSCs during gdsc_disable() - kernel/git/torvalds/linux.git - Linux kernel source tree https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/clk/qcom/gcc-sm8650.c?id=a57465766a91c6e173876f9cbb424340e214313f - Krishna Chaitanya.
Konrad
On 1/2/26 2:19 PM, Krishna Chaitanya Chundru wrote:
On 1/2/2026 5:09 PM, Konrad Dybcio wrote:
On 1/2/26 12:36 PM, Krishna Chaitanya Chundru wrote:
On 1/2/2026 5:04 PM, Konrad Dybcio wrote:
On 1/2/26 10:43 AM, Krishna Chaitanya Chundru wrote:
With PWRSTS_OFF_ON, PCIe GDSCs are turned off during gdsc_disable(). This can happen during scenarios such as system suspend and breaks the resume of PCIe controllers from suspend.
Isn't turning the GDSCs off what we want though? At least during system suspend?
If we are keeping link in D3cold it makes sense, but currently we are not keeping in D3cold so we don't expect them to get off.
Since we seem to be tackling that in parallel, it seems to make sense that adding a mechanism to let the PCIe driver select "on" vs "ret" vs "off" could be useful for us
At least I am not aware of such API where we can tell genpd not to turn off gdsc at runtime if we are keeping the device in D3cold state. But anyway the PCIe gdsc supports Retention, in that case adding this flag here makes more sense as it represents HW. sm8450,sm8650 also had similar problem which are fixed by mani[1].
Perhaps I should ask for a clarification - is retention superior to powering the GDSC off? Does it have any power costs?
FWIW I recall I could turn off the GDSCs on at least makena with the old suspend patches and the controllers would come back to life afterwards
In the suspend patches, we are keeping link in D3cold, so turning off gdsc will not have any effect.
What do you mean by it won't have any effect?
But if some reason we skipped D3cold like in S2idle case then gdsc should not be off, in that case in resume PCIe link will be broken.
Right, obviously
Konrad
On 02/01/2026 13:57, Konrad Dybcio wrote:
On 1/2/26 2:19 PM, Krishna Chaitanya Chundru wrote:
On 1/2/2026 5:09 PM, Konrad Dybcio wrote:
On 1/2/26 12:36 PM, Krishna Chaitanya Chundru wrote:
On 1/2/2026 5:04 PM, Konrad Dybcio wrote:
On 1/2/26 10:43 AM, Krishna Chaitanya Chundru wrote:
With PWRSTS_OFF_ON, PCIe GDSCs are turned off during gdsc_disable(). This can happen during scenarios such as system suspend and breaks the resume of PCIe controllers from suspend.
Isn't turning the GDSCs off what we want though? At least during system suspend?
If we are keeping link in D3cold it makes sense, but currently we are not keeping in D3cold so we don't expect them to get off.
Since we seem to be tackling that in parallel, it seems to make sense that adding a mechanism to let the PCIe driver select "on" vs "ret" vs "off" could be useful for us
At least I am not aware of such API where we can tell genpd not to turn off gdsc at runtime if we are keeping the device in D3cold state. But anyway the PCIe gdsc supports Retention, in that case adding this flag here makes more sense as it represents HW. sm8450,sm8650 also had similar problem which are fixed by mani[1].
Perhaps I should ask for a clarification - is retention superior to powering the GDSC off? Does it have any power costs?
In retention you'd expect any/all registers to remain powered, such that configuration changes persist through your own power state.
So any PLL or other clock marked as critical would require retention as would any other clock register setting you setup in probe() initially.
TBH should probably have retention on all of the clocks as a default..
FWIW I recall I could turn off the GDSCs on at least makena with the old suspend patches and the controllers would come back to life afterwards
In the suspend patches, we are keeping link in D3cold, so turning off gdsc will not have any effect.
What do you mean by it won't have any effect?
But if some reason we skipped D3cold like in S2idle case then gdsc should not be off, in that case in resume PCIe link will be broken.
Right, obviously
Konrad
On 1/2/2026 7:27 PM, Konrad Dybcio wrote:
On 1/2/26 2:19 PM, Krishna Chaitanya Chundru wrote:
On 1/2/2026 5:09 PM, Konrad Dybcio wrote:
On 1/2/26 12:36 PM, Krishna Chaitanya Chundru wrote:
On 1/2/2026 5:04 PM, Konrad Dybcio wrote:
On 1/2/26 10:43 AM, Krishna Chaitanya Chundru wrote:
With PWRSTS_OFF_ON, PCIe GDSCs are turned off during gdsc_disable(). This can happen during scenarios such as system suspend and breaks the resume of PCIe controllers from suspend.
Isn't turning the GDSCs off what we want though? At least during system suspend?
If we are keeping link in D3cold it makes sense, but currently we are not keeping in D3cold so we don't expect them to get off.
Since we seem to be tackling that in parallel, it seems to make sense that adding a mechanism to let the PCIe driver select "on" vs "ret" vs "off" could be useful for us
At least I am not aware of such API where we can tell genpd not to turn off gdsc at runtime if we are keeping the device in D3cold state. But anyway the PCIe gdsc supports Retention, in that case adding this flag here makes more sense as it represents HW. sm8450,sm8650 also had similar problem which are fixed by mani[1].
Perhaps I should ask for a clarification - is retention superior to powering the GDSC off? Does it have any power costs?
FWIW I recall I could turn off the GDSCs on at least makena with the old suspend patches and the controllers would come back to life afterwards
In the suspend patches, we are keeping link in D3cold, so turning off gdsc will not have any effect.
What do you mean by it won't have any effect?
I meant to say that in this case we can turn off the gdsc and it won't have any effect in SW. But in non D3cold case the link will be broken and will become non functional.
- Krishna Chaitanya.
But if some reason we skipped D3cold like in S2idle case then gdsc should not be off, in that case in resume PCIe link will be broken.
Right, obviously
Konrad
On Fri, Jan 02, 2026 at 02:57:56PM +0100, Konrad Dybcio wrote:
On 1/2/26 2:19 PM, Krishna Chaitanya Chundru wrote:
On 1/2/2026 5:09 PM, Konrad Dybcio wrote:
On 1/2/26 12:36 PM, Krishna Chaitanya Chundru wrote:
On 1/2/2026 5:04 PM, Konrad Dybcio wrote:
On 1/2/26 10:43 AM, Krishna Chaitanya Chundru wrote:
With PWRSTS_OFF_ON, PCIe GDSCs are turned off during gdsc_disable(). This can happen during scenarios such as system suspend and breaks the resume of PCIe controllers from suspend.
Isn't turning the GDSCs off what we want though? At least during system suspend?
If we are keeping link in D3cold it makes sense, but currently we are not keeping in D3cold so we don't expect them to get off.
Since we seem to be tackling that in parallel, it seems to make sense that adding a mechanism to let the PCIe driver select "on" vs "ret" vs "off" could be useful for us
At least I am not aware of such API where we can tell genpd not to turn off gdsc at runtime if we are keeping the device in D3cold state. But anyway the PCIe gdsc supports Retention, in that case adding this flag here makes more sense as it represents HW. sm8450,sm8650 also had similar problem which are fixed by mani[1].
Perhaps I should ask for a clarification - is retention superior to powering the GDSC off? Does it have any power costs?
In terms of power saving it is not superior, but that's not the only factor we should consider here. If we keep GDSCs PWRSTS_OFF_ON, then the devices (PCIe) need to be be in D3Cold. Sure we can change that using the new genpd API dev_pm_genpd_rpm_always_on() dynamically, but I would prefer to avoid doing that.
In my POV, GDSCs default state should be retention, so that the GDSCs will stay ON if the rentention is not entered in hw and enter retention otherwise. This requires no extra modification in the genpd client drivers. One more benefit is, the hw can enter low power state even when the device is not in D3Cold state i.e., during s2idle (provided we unvote other resources).
If the hw doesn't support retention like Makena PCIe GDSC, then PWRSTS_OFF_ON is the only option.
- Mani
On Mon, Jan 05, 2026 at 10:44:39AM +0530, Manivannan Sadhasivam wrote:
On Fri, Jan 02, 2026 at 02:57:56PM +0100, Konrad Dybcio wrote:
On 1/2/26 2:19 PM, Krishna Chaitanya Chundru wrote:
On 1/2/2026 5:09 PM, Konrad Dybcio wrote:
On 1/2/26 12:36 PM, Krishna Chaitanya Chundru wrote:
On 1/2/2026 5:04 PM, Konrad Dybcio wrote:
On 1/2/26 10:43 AM, Krishna Chaitanya Chundru wrote: > With PWRSTS_OFF_ON, PCIe GDSCs are turned off during gdsc_disable(). This > can happen during scenarios such as system suspend and breaks the resume > of PCIe controllers from suspend. Isn't turning the GDSCs off what we want though? At least during system suspend?
If we are keeping link in D3cold it makes sense, but currently we are not keeping in D3cold so we don't expect them to get off.
Since we seem to be tackling that in parallel, it seems to make sense that adding a mechanism to let the PCIe driver select "on" vs "ret" vs "off" could be useful for us
At least I am not aware of such API where we can tell genpd not to turn off gdsc at runtime if we are keeping the device in D3cold state. But anyway the PCIe gdsc supports Retention, in that case adding this flag here makes more sense as it represents HW. sm8450,sm8650 also had similar problem which are fixed by mani[1].
Perhaps I should ask for a clarification - is retention superior to powering the GDSC off? Does it have any power costs?
In terms of power saving it is not superior, but that's not the only factor we should consider here. If we keep GDSCs PWRSTS_OFF_ON, then the devices (PCIe) need to be be in D3Cold. Sure we can change that using the new genpd API dev_pm_genpd_rpm_always_on() dynamically, but I would prefer to avoid doing that.
In my POV, GDSCs default state should be retention, so that the GDSCs will stay ON if the rentention is not entered in hw and enter retention otherwise. This requires no extra modification in the genpd client drivers. One more benefit is, the hw can enter low power state even when the device is not in D3Cold state i.e., during s2idle (provided we unvote other resources).
What about PCIe instances that are completely unused? The boot firmware on X1E for example is notorious for powering on completely unused PCIe links and powering them down in some half-baked off state (the &pcie3 instance, in particular). I'm not sure if the GDSC remains on, but if it does then the unused PD cleanup would also only put them in retention state. I can't think of a good reason to keep those on at all.
The implementation of PWRSTS_RET_ON essentially makes the PD power_off() callback a no-op. Everything in Linux (sysfs, debugfs, ...) will tell you that the power domain has been shut down, but at the end it will remain fully powered until you manage to reach a retention state for the parent power domain. Due to other consumers, that will likely happen only if you reach VDDmin or some equivalent SoC-wide low-power state, something barely any (or none?) of the platforms supported upstream is capable of today.
PWRSTS_RET_ON is actually pretty close to setting GENPD_FLAG_ALWAYS_ON, the only advantage of PWRSTS_RET_ON I can think of is that unused GDSCs remain off iff you are lucky enough that the boot firmware has not already turned them on.
IMHO, for GDSCs that support OFF state in the hardware, PWRSTS_RET_ON is a hack to workaround limitations in the consumer drivers. They should either save/restore registers and handle the power collapse or they should vote for the power domain to stay on. That way, sysfs/debugfs will show the real votes held by Linux and you won't be mislead when looking at those while trying to optimize power consumption.
Thanks, Stephan
On Mon, Jan 05, 2026 at 10:47:29AM +0100, Stephan Gerhold wrote:
On Mon, Jan 05, 2026 at 10:44:39AM +0530, Manivannan Sadhasivam wrote:
On Fri, Jan 02, 2026 at 02:57:56PM +0100, Konrad Dybcio wrote:
On 1/2/26 2:19 PM, Krishna Chaitanya Chundru wrote:
On 1/2/2026 5:09 PM, Konrad Dybcio wrote:
On 1/2/26 12:36 PM, Krishna Chaitanya Chundru wrote:
On 1/2/2026 5:04 PM, Konrad Dybcio wrote: > On 1/2/26 10:43 AM, Krishna Chaitanya Chundru wrote: >> With PWRSTS_OFF_ON, PCIe GDSCs are turned off during gdsc_disable(). This >> can happen during scenarios such as system suspend and breaks the resume >> of PCIe controllers from suspend. > Isn't turning the GDSCs off what we want though? At least during system > suspend? If we are keeping link in D3cold it makes sense, but currently we are not keeping in D3cold so we don't expect them to get off.
Since we seem to be tackling that in parallel, it seems to make sense that adding a mechanism to let the PCIe driver select "on" vs "ret" vs "off" could be useful for us
At least I am not aware of such API where we can tell genpd not to turn off gdsc at runtime if we are keeping the device in D3cold state. But anyway the PCIe gdsc supports Retention, in that case adding this flag here makes more sense as it represents HW. sm8450,sm8650 also had similar problem which are fixed by mani[1].
Perhaps I should ask for a clarification - is retention superior to powering the GDSC off? Does it have any power costs?
In terms of power saving it is not superior, but that's not the only factor we should consider here. If we keep GDSCs PWRSTS_OFF_ON, then the devices (PCIe) need to be be in D3Cold. Sure we can change that using the new genpd API dev_pm_genpd_rpm_always_on() dynamically, but I would prefer to avoid doing that.
In my POV, GDSCs default state should be retention, so that the GDSCs will stay ON if the rentention is not entered in hw and enter retention otherwise. This requires no extra modification in the genpd client drivers. One more benefit is, the hw can enter low power state even when the device is not in D3Cold state i.e., during s2idle (provided we unvote other resources).
What about PCIe instances that are completely unused? The boot firmware on X1E for example is notorious for powering on completely unused PCIe links and powering them down in some half-baked off state (the &pcie3 instance, in particular). I'm not sure if the GDSC remains on, but if it does then the unused PD cleanup would also only put them in retention state. I can't think of a good reason to keep those on at all.
This is a good point. I didn't think of it.
The implementation of PWRSTS_RET_ON essentially makes the PD power_off() callback a no-op. Everything in Linux (sysfs, debugfs, ...) will tell you that the power domain has been shut down, but at the end it will remain fully powered until you manage to reach a retention state for the parent power domain. Due to other consumers, that will likely happen only if you reach VDDmin or some equivalent SoC-wide low-power state, something barely any (or none?) of the platforms supported upstream is capable of today.
Unfortunately, that's the current state of retention today. It is only a firmware visible state. Ofc, the OS could query SMEM and figure out after resume, but there is no way currently to translate that to individual power domains.
PWRSTS_RET_ON is actually pretty close to setting GENPD_FLAG_ALWAYS_ON, the only advantage of PWRSTS_RET_ON I can think of is that unused GDSCs remain off iff you are lucky enough that the boot firmware has not already turned them on.
IMHO, for GDSCs that support OFF state in the hardware, PWRSTS_RET_ON is a hack to workaround limitations in the consumer drivers. They should either save/restore registers and handle the power collapse or they should vote for the power domain to stay on. That way, sysfs/debugfs will show the real votes held by Linux and you won't be mislead when looking at those while trying to optimize power consumption.
Maybe we should just use dev_pm_genpd_rpm_always_on() in the client drivers if they know for sure that the device context should be preserved and keep PWRSTS_OFF_ON flag.
- Mani
On Fri, Jan 02, 2026 at 03:13:00PM +0530, Krishna Chaitanya Chundru wrote:
With PWRSTS_OFF_ON, PCIe GDSCs are turned off during gdsc_disable(). This can happen during scenarios such as system suspend and breaks the resume of PCIe controllers from suspend.
So use PWRSTS_RET_ON to indicate the GDSC driver to not turn off the GDSCs during gdsc_disable() and allow the hardware to transition the GDSCs to retention when the parent domain enters low power state during system suspend.
Signed-off-by: Krishna Chaitanya Chundru krishna.chundru@oss.qualcomm.com
Reviewed-by: Manivannan Sadhasivam mani@kernel.org
- Mani
Krishna Chaitanya Chundru (7): clk: qcom: gcc-sc7280: Do not turn off PCIe GDSCs during gdsc_disable() clk: qcom: gcc-sa8775p: Do not turn off PCIe GDSCs during gdsc_disable() clk: qcom: gcc-sm8750: Do not turn off PCIe GDSCs during gdsc_disable() clk: qcom: gcc-glymur: Do not turn off PCIe GDSCs during gdsc_disable() clk: qcom: gcc-qcs8300: Do not turn off PCIe GDSCs during gdsc_disable() clk: qcom: gcc-x1e80100: Do not turn off PCIe GDSCs during gdsc_disable() clk: qcom: gcc-kaanapali: Do not turn off PCIe GDSCs during gdsc_disable()
drivers/clk/qcom/gcc-glymur.c | 16 ++++++++-------- drivers/clk/qcom/gcc-kaanapali.c | 2 +- drivers/clk/qcom/gcc-qcs8300.c | 4 ++-- drivers/clk/qcom/gcc-sa8775p.c | 4 ++-- drivers/clk/qcom/gcc-sc7280.c | 2 +- drivers/clk/qcom/gcc-sm8750.c | 2 +- drivers/clk/qcom/gcc-x1e80100.c | 16 ++++++++-------- 7 files changed, 23 insertions(+), 23 deletions(-)
base-commit: 98e506ee7d10390b527aeddee7bbeaf667129646 change-id: 20260102-pci_gdsc_fix-1dcf08223922
Best regards,
Krishna Chaitanya Chundru krishna.chundru@oss.qualcomm.com
linux-stable-mirror@lists.linaro.org
-
Bryan O'Donoghue -
Konrad Dybcio -
Krishna Chaitanya Chundru -
Manivannan Sadhasivam -
Stephan Gerhold