Revert "power: sequencing: request the WLAN enable GPIO as-is"

PATCH In review Bartosz Golaszewski 3 participants 7 messages Dec 3, 2024 – Dec 16, 2024

Subsystems (4)

Device tree bindings Arm Qualcomm mailing list Arm Qualcomm support Power sequencing

lore

Share & tools

lore

Bartosz Golaszewski

author

2024-12-03 14:13 UTC #0 lore raw

PATCH 1/1 Revert "power: sequencing: request the WLAN enable GPIO as-is"

From: Bartosz Golaszewski <[email protected]>

This reverts commit a9aaf1ff88a8cb99a1335c9eb76de637f0cf8c10.

With the changes recently merged into the PCI/pwrctrl/ we now have correct ordering between the pwrseq provider and the PCI-pwrctrl consumers. With that, the pwrseq WCN driver no longer needs to leave the GPIO state as-is and we can remove the workaround.

Signed-off-by Bartosz Golaszewski <[email protected]>

---
 drivers/power/sequencing/pwrseq-qcom-wcn.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/power/sequencing/pwrseq-qcom-wcn.c +1 -1

index 682a9beac69eb..bb8c47280b7bc 100644
@@ -379,7 +379,7 @@ static int pwrseq_qcom_wcn_probe(struct platform_device *pdev)
 				     "Failed to get the Bluetooth enable GPIO\n");
 
 	ctx->wlan_gpio = devm_gpiod_get_optional(dev, "wlan-enable",
-						 GPIOD_ASIS);
+						 GPIOD_OUT_LOW);
 	if (IS_ERR(ctx->wlan_gpio))
 		return dev_err_probe(dev, PTR_ERR(ctx->wlan_gpio),
 				     "Failed to get the WLAN enable GPIO\n");

Signature · 1 line

2.30.2

Stephan Gerhold

2024-12-13 18:19 UTC #1 lore raw

Quoted 9 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>
This reverts commit a9aaf1ff88a8cb99a1335c9eb76de637f0cf8c10.
With the changes recently merged into the PCI/pwrctrl/ we now have correct ordering between the pwrseq provider and the PCI-pwrctrl consumers. With that, the pwrseq WCN driver no longer needs to leave the GPIO state as-is and we can remove the workaround.

Should probably revert commit d8b762070c3f ("power: sequencing:
qcom-wcn: set the wlan-enable GPIO to output") as well?

Quoted 21 lines

Signed-off-by Bartosz Golaszewski <[email protected]>

---
 drivers/power/sequencing/pwrseq-qcom-wcn.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/power/sequencing/pwrseq-qcom-wcn.c +1 -1

index 682a9beac69eb..bb8c47280b7bc 100644
@@ -379,7 +379,7 @@ static int pwrseq_qcom_wcn_probe(struct platform_device *pdev)
 				     "Failed to get the Bluetooth enable GPIO\n");
 
 	ctx->wlan_gpio = devm_gpiod_get_optional(dev, "wlan-enable",
-						 GPIOD_ASIS);
+						 GPIOD_OUT_LOW);
 	if (IS_ERR(ctx->wlan_gpio))
 		return dev_err_probe(dev, PTR_ERR(ctx->wlan_gpio),
 				     "Failed to get the WLAN enable GPIO\n");

Signature · 1 line

2.30.2

I'm not sure why but applying this patch brings back the error I had before. It does seem like setting wlan-enable GPIO happens early enough, but maybe some timing is still wrong.

[   17.132161] <gpiod_set_value_cansleep(ctx->wlan_gpio, 1);>
[   17.480619] ath12k_pci 0004:01:00.0: of_irq_parse_pci: failed with rc=134
[   17.491997] ath12k_pci 0004:01:00.0: pci device id mismatch: 0xffff 0x1107
[   17.492000] ath12k_pci 0004:01:00.0: failed to claim device: -5
[   17.492075] ath12k_pci 0004:01:00.0: probe with driver ath12k_pci failed with error -5

Any ideas/suggestions?

Thanks,
Stephan

Manivannan Sadhasivam

2024-12-16 07:06 UTC #2 lore raw

Quoted 26 lines · Stephan Gerhold

Quoted 9 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
This reverts commit a9aaf1ff88a8cb99a1335c9eb76de637f0cf8c10.
With the changes recently merged into the PCI/pwrctrl/ we now have correct ordering between the pwrseq provider and the PCI-pwrctrl consumers. With that, the pwrseq WCN driver no longer needs to leave the GPIO state as-is and we can remove the workaround.
Should probably revert commit d8b762070c3f ("power: sequencing:
qcom-wcn: set the wlan-enable GPIO to output") as well?
Quoted 12 lines
Signed-off-by Bartosz Golaszewski <[email protected]>
---
 drivers/power/sequencing/pwrseq-qcom-wcn.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/power/sequencing/pwrseq-qcom-wcn.c +0 -0
index 682a9beac69eb..bb8c47280b7bc 100644
@@ -379,7 +379,7 @@ static int pwrseq_qcom_wcn_probe(struct platform_device *pdev)
 				     "Failed to get the Bluetooth enable GPIO\n");
 

Quoted 14 lines

Quoted 9 lines
drivers/power/sequencing/pwrseq-qcom-wcn.c +1 -1
 	ctx->wlan_gpio = devm_gpiod_get_optional(dev, "wlan-enable",
-						 GPIOD_ASIS);
+						 GPIOD_OUT_LOW);
 	if (IS_ERR(ctx->wlan_gpio))
 		return dev_err_probe(dev, PTR_ERR(ctx->wlan_gpio),
 				     "Failed to get the WLAN enable GPIO\n");
Signature · 1 line
2.30.2
I'm not sure why but applying this patch brings back the error I had before. It does seem like setting wlan-enable GPIO happens early enough, but maybe some timing is still wrong.

There should be no room for timing issue now :/

Quoted 6 lines

[   17.132161] <gpiod_set_value_cansleep(ctx->wlan_gpio, 1);>
[   17.480619] ath12k_pci 0004:01:00.0: of_irq_parse_pci: failed with rc=134
[   17.491997] ath12k_pci 0004:01:00.0: pci device id mismatch: 0xffff 0x1107
[   17.492000] ath12k_pci 0004:01:00.0: failed to claim device: -5
[   17.492075] ath12k_pci 0004:01:00.0: probe with driver ath12k_pci failed with error -5

Are you sure that this is the same error that you noticed before?

Quoted 2 lines

Any ideas/suggestions?

Can you verify that the pwrctrl driver's probe is completed *before* ath12k driver starting to probe by adding the debug prints in both drivers?

- Mani

Signature · 1 line

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

Stephan Gerhold

2024-12-16 10:50 UTC #3 lore raw

Quoted 44 lines · Manivannan Sadhasivam

Quoted 26 lines · Stephan Gerhold
Quoted 9 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
This reverts commit a9aaf1ff88a8cb99a1335c9eb76de637f0cf8c10.
With the changes recently merged into the PCI/pwrctrl/ we now have correct ordering between the pwrseq provider and the PCI-pwrctrl consumers. With that, the pwrseq WCN driver no longer needs to leave the GPIO state as-is and we can remove the workaround.
Should probably revert commit d8b762070c3f ("power: sequencing:
qcom-wcn: set the wlan-enable GPIO to output") as well?
Quoted 12 lines
Signed-off-by Bartosz Golaszewski <[email protected]>
---
 drivers/power/sequencing/pwrseq-qcom-wcn.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/power/sequencing/pwrseq-qcom-wcn.c +0 -0
index 682a9beac69eb..bb8c47280b7bc 100644
@@ -379,7 +379,7 @@ static int pwrseq_qcom_wcn_probe(struct platform_device *pdev)
 				     "Failed to get the Bluetooth enable GPIO\n");
 
Quoted 14 lines
Quoted 9 lines
drivers/power/sequencing/pwrseq-qcom-wcn.c +1 -1
 	ctx->wlan_gpio = devm_gpiod_get_optional(dev, "wlan-enable",
-						 GPIOD_ASIS);
+						 GPIOD_OUT_LOW);
 	if (IS_ERR(ctx->wlan_gpio))
 		return dev_err_probe(dev, PTR_ERR(ctx->wlan_gpio),
 				     "Failed to get the WLAN enable GPIO\n");
Signature · 1 line
2.30.2
I'm not sure why but applying this patch brings back the error I had before. It does seem like setting wlan-enable GPIO happens early enough, but maybe some timing is still wrong.
There should be no room for timing issue now :/

Quoted 9 lines

Quoted 6 lines

[   17.132161] <gpiod_set_value_cansleep(ctx->wlan_gpio, 1);>
[   17.480619] ath12k_pci 0004:01:00.0: of_irq_parse_pci: failed with rc=134
[   17.491997] ath12k_pci 0004:01:00.0: pci device id mismatch: 0xffff 0x1107
[   17.492000] ath12k_pci 0004:01:00.0: failed to claim device: -5
[   17.492075] ath12k_pci 0004:01:00.0: probe with driver ath12k_pci failed with error -5

Are you sure that this is the same error that you noticed before?

Yes, "pci device id mismatch: 0xffff 0x1107" is definitely the same error I saw before.

Quoted 6 lines

Quoted 2 lines
Any ideas/suggestions?
Can you verify that the pwrctrl driver's probe is completed *before* ath12k driver starting to probe by adding the debug prints in both drivers?

I tried booting with "modprobe.blacklist=ath12k" and manually loaded the module several seconds after the boot completed. Error is unchanged:

[   16.628165] <gpiod_set_value_cansleep(ctx->wlan_gpio, 1);>
[   55.065794] ath12k_pci 0004:01:00.0: of_irq_parse_pci: failed with rc=134
[   55.073354] ath12k_pci 0004:01:00.0: pci device id mismatch: 0xffff 0x1107
[   55.080457] ath12k_pci 0004:01:00.0: failed to claim device: -5
[   55.088977] ath12k_pci 0004:01:00.0: probe with driver ath12k_pci failed with error -5

I played around a bit more and it looks like the problem is that the PCI device is still being enumerated during startup, before the pwrseq driver is loaded. Not with the ath12k driver, but the internal PCI subsystem state. And then the PCI link dies briefly when the pwrseq driver loads.

If I add some hacks to the DT to force the wlan-enable GPIO low before the entire PCI _controller_ is probed, then it works correctly:

[   16.607359] <gpiod_set_value_cansleep(ctx->wlan_gpio, 1);>
[   16.668533] pci 0004:01:00.0: [17cb:1107] type 00 class 0x028000 PCIe Endpoint
[   16.668606] pci 0004:01:00.0: BAR 0 [mem 0x00000000-0x001fffff 64bit]
[   16.669055] pci 0004:01:00.0: PME# supported from D0 D3hot D3cold
[   16.675546] pcieport 0004:00:00.0: bridge window [mem 0x7c400000-0x7c5fffff]: assigned
[   16.675555] pci 0004:01:00.0: BAR 0 [mem 0x7c400000-0x7c5fffff 64bit]: assigned
[   16.862083] ath12k_pci 0004:01:00.0: BAR 0 [mem 0x7c400000-0x7c5fffff 64bit]: assigned
[   16.870358] ath12k_pci 0004:01:00.0: enabling device (0000 -> 0002)
[   16.888792] ath12k_pci 0004:01:00.0: MSI vectors: 16
[   16.893954] ath12k_pci 0004:01:00.0: Hardware name: wcn7850 hw2.0

Note the pci messages after enabling the GPIO, before the first ath12k_pci messages. Without the hack, those appear already before the pwrseq driver is being loaded (during initramfs).

[    5.888688] pci 0004:01:00.0: [17cb:1107] type 00 class 0x028000 PCIe Endpoint
[    5.888758] pci 0004:01:00.0: BAR 0 [mem 0x00000000-0x001fffff 64bit]
[    5.889207] pci 0004:01:00.0: PME# supported from D0 D3hot D3cold
[    5.902692] pci 0004:00:00.0: bridge window [mem 0x7c400000-0x7c5fffff]: assigned
[    5.910311] pci 0004:01:00.0: BAR 0 [mem 0x7c400000-0x7c5fffff 64bit]: assigned
...
[   21.227565] <gpiod_set_value_cansleep(ctx->wlan_gpio, 1);>
[   21.305496] ath12k_pci 0004:01:00.0: of_irq_parse_pci: failed with rc=134
[   21.318382] ath12k_pci 0004:01:00.0: pci device id mismatch: 0xffff 0x1107
[   21.338489] ath12k_pci 0004:01:00.0: failed to claim device: -5
[   21.338555] ath12k_pci 0004:01:00.0: probe with driver ath12k_pci failed with error -5

Can we skip scanning the PCI bus until the power sequencing is done?

The hack I used (see below) works, but is a bit odd since it requires assigning the wcn7850-pmu pinctrl to the PCI bus in the DT. Otherwise the GPIO is not forced low early enough.

Thanks,
Stephan

arch/arm64/boot/dts/qcom/x1e80100-crd.dts +4 -3

index 054ae260218e..8b658d15f185 100644
@@ -562,8 +562,8 @@ wcn7850-pmu {
 		wlan-enable-gpios = <&tlmm 117 GPIO_ACTIVE_HIGH>;
 		bt-enable-gpios = <&tlmm 116 GPIO_ACTIVE_HIGH>;
 
-		pinctrl-0 = <&wcn_wlan_bt_en>;
-		pinctrl-names = "default";
+		//pinctrl-0 = <&wcn_wlan_bt_en>;
+		//pinctrl-names = "default";
 
 		regulators {
 			vreg_pmu_rfa_cmn: ldo0 {

@@ -1298,7 +1298,7 @@ &pcie4 {
 	perst-gpios = <&tlmm 146 GPIO_ACTIVE_LOW>;
 	wake-gpios = <&tlmm 148 GPIO_ACTIVE_LOW>;
 
-	pinctrl-0 = <&pcie4_default>;
+	pinctrl-0 = <&pcie4_default>, <&wcn_wlan_bt_en>;
 	pinctrl-names = "default";
 
 	status = "okay";

@@ -1757,6 +1757,7 @@ wcn_wlan_bt_en: wcn-wlan-bt-en-state {
 		function = "gpio";
 		drive-strength = <2>;
 		bias-disable;
+		output-low;
 	};
 
 	wwan_sw_en: wwan-sw-en-state {

Manivannan Sadhasivam

2024-12-16 13:24 UTC #4 lore raw

Quoted 74 lines · Stephan Gerhold

Quoted 44 lines · Manivannan Sadhasivam
Quoted 26 lines · Stephan Gerhold
Quoted 9 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
This reverts commit a9aaf1ff88a8cb99a1335c9eb76de637f0cf8c10.
With the changes recently merged into the PCI/pwrctrl/ we now have correct ordering between the pwrseq provider and the PCI-pwrctrl consumers. With that, the pwrseq WCN driver no longer needs to leave the GPIO state as-is and we can remove the workaround.
Should probably revert commit d8b762070c3f ("power: sequencing:
qcom-wcn: set the wlan-enable GPIO to output") as well?
Quoted 12 lines
Signed-off-by Bartosz Golaszewski <[email protected]>
---
 drivers/power/sequencing/pwrseq-qcom-wcn.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/power/sequencing/pwrseq-qcom-wcn.c +0 -0
index 682a9beac69eb..bb8c47280b7bc 100644
@@ -379,7 +379,7 @@ static int pwrseq_qcom_wcn_probe(struct platform_device *pdev)
 				     "Failed to get the Bluetooth enable GPIO\n");
 
Quoted 14 lines
Quoted 9 lines
drivers/power/sequencing/pwrseq-qcom-wcn.c +1 -1
 	ctx->wlan_gpio = devm_gpiod_get_optional(dev, "wlan-enable",
-						 GPIOD_ASIS);
+						 GPIOD_OUT_LOW);
 	if (IS_ERR(ctx->wlan_gpio))
 		return dev_err_probe(dev, PTR_ERR(ctx->wlan_gpio),
 				     "Failed to get the WLAN enable GPIO\n");
Signature · 1 line
2.30.2
I'm not sure why but applying this patch brings back the error I had before. It does seem like setting wlan-enable GPIO happens early enough, but maybe some timing is still wrong.
There should be no room for timing issue now :/
Quoted 9 lines
Quoted 6 lines
[   17.132161] <gpiod_set_value_cansleep(ctx->wlan_gpio, 1);>
[   17.480619] ath12k_pci 0004:01:00.0: of_irq_parse_pci: failed with rc=134
[   17.491997] ath12k_pci 0004:01:00.0: pci device id mismatch: 0xffff 0x1107
[   17.492000] ath12k_pci 0004:01:00.0: failed to claim device: -5
[   17.492075] ath12k_pci 0004:01:00.0: probe with driver ath12k_pci failed with error -5
Are you sure that this is the same error that you noticed before?
Yes, "pci device id mismatch: 0xffff 0x1107" is definitely the same error I saw before.
Quoted 6 lines
Quoted 2 lines
Any ideas/suggestions?
Can you verify that the pwrctrl driver's probe is completed *before* ath12k driver starting to probe by adding the debug prints in both drivers?
I tried booting with "modprobe.blacklist=ath12k" and manually loaded the module several seconds after the boot completed. Error is unchanged:
[   16.628165] <gpiod_set_value_cansleep(ctx->wlan_gpio, 1);>
[   55.065794] ath12k_pci 0004:01:00.0: of_irq_parse_pci: failed with rc=134
[   55.073354] ath12k_pci 0004:01:00.0: pci device id mismatch: 0xffff 0x1107
[   55.080457] ath12k_pci 0004:01:00.0: failed to claim device: -5
[   55.088977] ath12k_pci 0004:01:00.0: probe with driver ath12k_pci failed with error -5

Quoted 6 lines

I played around a bit more and it looks like the problem is that the PCI device is still being enumerated during startup, before the pwrseq driver is loaded. Not with the ath12k driver, but the internal PCI subsystem state. And then the PCI link dies briefly when the pwrseq driver loads.

Ok, this seems to be the cause. There is a known issue with Qcom PCIe controllers where if the device is powered off abrubtly (without controller driver noticing) the PCIe link moves to link down state. Then we need to bring the link back by reinitializing the controller. I have it in my todo list but no one noticed this issue unless they tried powering down and powering up the device (which is rare except on hot pluggable slots).

Quoted 30 lines

If I add some hacks to the DT to force the wlan-enable GPIO low before the entire PCI _controller_ is probed, then it works correctly:

[   16.607359] <gpiod_set_value_cansleep(ctx->wlan_gpio, 1);>
[   16.668533] pci 0004:01:00.0: [17cb:1107] type 00 class 0x028000 PCIe Endpoint
[   16.668606] pci 0004:01:00.0: BAR 0 [mem 0x00000000-0x001fffff 64bit]
[   16.669055] pci 0004:01:00.0: PME# supported from D0 D3hot D3cold
[   16.675546] pcieport 0004:00:00.0: bridge window [mem 0x7c400000-0x7c5fffff]: assigned
[   16.675555] pci 0004:01:00.0: BAR 0 [mem 0x7c400000-0x7c5fffff 64bit]: assigned
[   16.862083] ath12k_pci 0004:01:00.0: BAR 0 [mem 0x7c400000-0x7c5fffff 64bit]: assigned
[   16.870358] ath12k_pci 0004:01:00.0: enabling device (0000 -> 0002)
[   16.888792] ath12k_pci 0004:01:00.0: MSI vectors: 16
[   16.893954] ath12k_pci 0004:01:00.0: Hardware name: wcn7850 hw2.0

Note the pci messages after enabling the GPIO, before the first ath12k_pci messages. Without the hack, those appear already before the pwrseq driver is being loaded (during initramfs).

[    5.888688] pci 0004:01:00.0: [17cb:1107] type 00 class 0x028000 PCIe Endpoint
[    5.888758] pci 0004:01:00.0: BAR 0 [mem 0x00000000-0x001fffff 64bit]
[    5.889207] pci 0004:01:00.0: PME# supported from D0 D3hot D3cold
[    5.902692] pci 0004:00:00.0: bridge window [mem 0x7c400000-0x7c5fffff]: assigned
[    5.910311] pci 0004:01:00.0: BAR 0 [mem 0x7c400000-0x7c5fffff 64bit]: assigned
...
[   21.227565] <gpiod_set_value_cansleep(ctx->wlan_gpio, 1);>
[   21.305496] ath12k_pci 0004:01:00.0: of_irq_parse_pci: failed with rc=134
[   21.318382] ath12k_pci 0004:01:00.0: pci device id mismatch: 0xffff 0x1107
[   21.338489] ath12k_pci 0004:01:00.0: failed to claim device: -5
[   21.338555] ath12k_pci 0004:01:00.0: probe with driver ath12k_pci failed with error -5

Quoted 2 lines

Can we skip scanning the PCI bus until the power sequencing is done?

This won't help (but a good idea anyway that I'll implement). See below...

Quoted 4 lines

The hack I used (see below) works, but is a bit odd since it requires assigning the wcn7850-pmu pinctrl to the PCI bus in the DT. Otherwise the GPIO is not forced low early enough.

Your hack is making sure that the default state of the GPIO is not changed at all after initializing the controller. So even if the pwrctrl driver probes later, it will try to enable the module by doing, 'gpiod_set_value_cansleep(ctx->wlan_gpio, 1)', which would do nothing to the device state.

So the issue is not with the pwrctrl driver but with the controller implementation. Ideally, once the device is removed, the PCIe link should move to Detect state and then to Polling state once the receiver is detected on the lanes. But the DWC and Qcom glue has other logics that prevents the controller from doing so.

So until the link down handling is implemented in the controller driver, we need to carry this hack that preserves the GPIO state.

- Mani

Signature · 1 line

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

Bartosz Golaszewski

author

2024-12-16 13:36 UTC #5 lore raw

Quoted 136 lines · Manivannan Sadhasivam

Quoted 74 lines · Stephan Gerhold
Quoted 44 lines · Manivannan Sadhasivam
Quoted 26 lines · Stephan Gerhold
Quoted 9 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
This reverts commit a9aaf1ff88a8cb99a1335c9eb76de637f0cf8c10.
With the changes recently merged into the PCI/pwrctrl/ we now have correct ordering between the pwrseq provider and the PCI-pwrctrl consumers. With that, the pwrseq WCN driver no longer needs to leave the GPIO state as-is and we can remove the workaround.
Should probably revert commit d8b762070c3f ("power: sequencing:
qcom-wcn: set the wlan-enable GPIO to output") as well?
Quoted 12 lines
Signed-off-by Bartosz Golaszewski <[email protected]>
---
 drivers/power/sequencing/pwrseq-qcom-wcn.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/power/sequencing/pwrseq-qcom-wcn.c +0 -0
index 682a9beac69eb..bb8c47280b7bc 100644
@@ -379,7 +379,7 @@ static int pwrseq_qcom_wcn_probe(struct platform_device *pdev)
                                     "Failed to get the Bluetooth enable GPIO\n");
 
Quoted 14 lines
Quoted 9 lines
drivers/power/sequencing/pwrseq-qcom-wcn.c +1 -2
        ctx->wlan_gpio = devm_gpiod_get_optional(dev, "wlan-enable",
-                                                GPIOD_ASIS);
+                                                GPIOD_OUT_LOW);
        if (IS_ERR(ctx->wlan_gpio))
                return dev_err_probe(dev, PTR_ERR(ctx->wlan_gpio),
                                     "Failed to get the WLAN enable GPIO\n");
--
2.30.2
I'm not sure why but applying this patch brings back the error I had before. It does seem like setting wlan-enable GPIO happens early enough, but maybe some timing is still wrong.
There should be no room for timing issue now :/
Quoted 9 lines
Quoted 6 lines
[   17.132161] <gpiod_set_value_cansleep(ctx->wlan_gpio, 1);>
[   17.480619] ath12k_pci 0004:01:00.0: of_irq_parse_pci: failed with rc=134
[   17.491997] ath12k_pci 0004:01:00.0: pci device id mismatch: 0xffff 0x1107
[   17.492000] ath12k_pci 0004:01:00.0: failed to claim device: -5
[   17.492075] ath12k_pci 0004:01:00.0: probe with driver ath12k_pci failed with error -5
Are you sure that this is the same error that you noticed before?
Yes, "pci device id mismatch: 0xffff 0x1107" is definitely the same error I saw before.
Quoted 6 lines
Quoted 2 lines
Any ideas/suggestions?
Can you verify that the pwrctrl driver's probe is completed *before* ath12k driver starting to probe by adding the debug prints in both drivers?
I tried booting with "modprobe.blacklist=ath12k" and manually loaded the module several seconds after the boot completed. Error is unchanged:
[   16.628165] <gpiod_set_value_cansleep(ctx->wlan_gpio, 1);>
[   55.065794] ath12k_pci 0004:01:00.0: of_irq_parse_pci: failed with rc=134
[   55.073354] ath12k_pci 0004:01:00.0: pci device id mismatch: 0xffff 0x1107
[   55.080457] ath12k_pci 0004:01:00.0: failed to claim device: -5
[   55.088977] ath12k_pci 0004:01:00.0: probe with driver ath12k_pci failed with error -5
Quoted 6 lines
I played around a bit more and it looks like the problem is that the PCI device is still being enumerated during startup, before the pwrseq driver is loaded. Not with the ath12k driver, but the internal PCI subsystem state. And then the PCI link dies briefly when the pwrseq driver loads.
Ok, this seems to be the cause. There is a known issue with Qcom PCIe controllers where if the device is powered off abrubtly (without controller driver noticing) the PCIe link moves to link down state. Then we need to bring the link back by reinitializing the controller. I have it in my todo list but no one noticed this issue unless they tried powering down and powering up the device (which is rare except on hot pluggable slots).
Quoted 30 lines
If I add some hacks to the DT to force the wlan-enable GPIO low before the entire PCI _controller_ is probed, then it works correctly:
[   16.607359] <gpiod_set_value_cansleep(ctx->wlan_gpio, 1);>
[   16.668533] pci 0004:01:00.0: [17cb:1107] type 00 class 0x028000 PCIe Endpoint
[   16.668606] pci 0004:01:00.0: BAR 0 [mem 0x00000000-0x001fffff 64bit]
[   16.669055] pci 0004:01:00.0: PME# supported from D0 D3hot D3cold
[   16.675546] pcieport 0004:00:00.0: bridge window [mem 0x7c400000-0x7c5fffff]: assigned
[   16.675555] pci 0004:01:00.0: BAR 0 [mem 0x7c400000-0x7c5fffff 64bit]: assigned
[   16.862083] ath12k_pci 0004:01:00.0: BAR 0 [mem 0x7c400000-0x7c5fffff 64bit]: assigned
[   16.870358] ath12k_pci 0004:01:00.0: enabling device (0000 -> 0002)
[   16.888792] ath12k_pci 0004:01:00.0: MSI vectors: 16
[   16.893954] ath12k_pci 0004:01:00.0: Hardware name: wcn7850 hw2.0
Note the pci messages after enabling the GPIO, before the first ath12k_pci messages. Without the hack, those appear already before the pwrseq driver is being loaded (during initramfs).
[    5.888688] pci 0004:01:00.0: [17cb:1107] type 00 class 0x028000 PCIe Endpoint
[    5.888758] pci 0004:01:00.0: BAR 0 [mem 0x00000000-0x001fffff 64bit]
[    5.889207] pci 0004:01:00.0: PME# supported from D0 D3hot D3cold
[    5.902692] pci 0004:00:00.0: bridge window [mem 0x7c400000-0x7c5fffff]: assigned
[    5.910311] pci 0004:01:00.0: BAR 0 [mem 0x7c400000-0x7c5fffff 64bit]: assigned
...
[   21.227565] <gpiod_set_value_cansleep(ctx->wlan_gpio, 1);>
[   21.305496] ath12k_pci 0004:01:00.0: of_irq_parse_pci: failed with rc=134
[   21.318382] ath12k_pci 0004:01:00.0: pci device id mismatch: 0xffff 0x1107
[   21.338489] ath12k_pci 0004:01:00.0: failed to claim device: -5
[   21.338555] ath12k_pci 0004:01:00.0: probe with driver ath12k_pci failed with error -5
Quoted 2 lines
Can we skip scanning the PCI bus until the power sequencing is done?
This won't help (but a good idea anyway that I'll implement). See below...
Quoted 4 lines
The hack I used (see below) works, but is a bit odd since it requires assigning the wcn7850-pmu pinctrl to the PCI bus in the DT. Otherwise the GPIO is not forced low early enough.
Your hack is making sure that the default state of the GPIO is not changed at all after initializing the controller. So even if the pwrctrl driver probes later, it will try to enable the module by doing, 'gpiod_set_value_cansleep(ctx->wlan_gpio, 1)', which would do nothing to the device state.

Quoted 9 lines

So the issue is not with the pwrctrl driver but with the controller implementation. Ideally, once the device is removed, the PCIe link should move to Detect state and then to Polling state once the receiver is detected on the lanes. But the DWC and Qcom glue has other logics that prevents the controller from doing so.
So until the link down handling is implemented in the controller driver, we need to carry this hack that preserves the GPIO state.

Thanks for the explanation Mani. Regarding this patch: I suggest we keep it for now but maybe I'll add a comment saying why it's still necessary?

Bart

Manivannan Sadhasivam

2024-12-16 13:40 UTC #6 lore raw

On Mon, Dec 16, 2024 at 02:36:24PM +0100, Bartosz Golaszewski wrote:

[...]

Quoted 12 lines

Quoted 12 lines
Your hack is making sure that the default state of the GPIO is not changed at all after initializing the controller. So even if the pwrctrl driver probes later, it will try to enable the module by doing, 'gpiod_set_value_cansleep(ctx->wlan_gpio, 1)', which would do nothing to the device state.
So the issue is not with the pwrctrl driver but with the controller implementation. Ideally, once the device is removed, the PCIe link should move to Detect state and then to Polling state once the receiver is detected on the lanes. But the DWC and Qcom glue has other logics that prevents the controller from doing so.

Quoted 8 lines

Quoted 3 lines
So until the link down handling is implemented in the controller driver, we need to carry this hack that preserves the GPIO state.
Thanks for the explanation Mani. Regarding this patch: I suggest we keep it for now but maybe I'll add a comment saying why it's still necessary?

Yeah, that makes sense.

- Mani

Signature · 1 line

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