| Message ID | 20240930103041.49229-4-brgl@bgdev.pl (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | arm64: dts: qcom: sc8280xp: enable WLAN and Bluetooth | expand |
On Mon, Sep 30, 2024 at 12:30:39PM +0200, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > Add a node for the PMU of the WCN6855 and rework the inputs of the wifi > and bluetooth nodes to consume the PMU's outputs. > > With this we can drop the regulator-always-on properties from vreg_s11b > and vreg_s12b as they will now be enabled by the power sequencing > driver. > > Tested-by: Steev Klimaszewski <steev@kali.org> # Thinkpad X13s > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> Without this patch I'm seeing an indefinite probe deferral with 6.12-rc1: platform 1c00000.pcie:pcie@0:wifi@0: deferred probe pending: pci-pwrctl-pwrseq: Failed to get the power sequencer Can you please look into that and make sure that the existing DT continues to work without such warnings. > --- > .../qcom/sc8280xp-lenovo-thinkpad-x13s.dts | 100 +++++++++++++++--- > 1 file changed, 86 insertions(+), 14 deletions(-) > > diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts > index 6a28cab97189..7230d5420199 100644 > --- a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts > +++ b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts > @@ -400,6 +400,67 @@ usb1_sbu_mux: endpoint { > }; > }; > }; > + > + wcn6855-pmu { > + compatible = "qcom,wcn6855-pmu"; > + > + pinctrl-0 = <&bt_default>, <&wlan_en>; > + pinctrl-names = "default"; > + > + wlan-enable-gpios = <&tlmm 134 GPIO_ACTIVE_HIGH>; > + bt-enable-gpios = <&tlmm 133 GPIO_ACTIVE_HIGH>; > @@ -1258,20 +1327,16 @@ &uart2 { > bluetooth { > compatible = "qcom,wcn6855-bt"; > > - vddio-supply = <&vreg_s10b>; > - vddbtcxmx-supply = <&vreg_s12b>; > - vddrfacmn-supply = <&vreg_s12b>; > - vddrfa0p8-supply = <&vreg_s12b>; > - vddrfa1p2-supply = <&vreg_s11b>; > - vddrfa1p7-supply = <&vreg_s1c>; > + vddrfacmn-supply = <&vreg_pmu_rfa_cmn_0p8>; > + vddaon-supply = <&vreg_pmu_aon_0p8>; > + vddwlcx-supply = <&vreg_pmu_wlcx_0p8>; > + vddwlmx-supply = <&vreg_pmu_wlmx_0p8>; > + vddbtcmx-supply = <&vreg_pmu_btcmx_0p8>; > + vddrfa0p8-supply = <&vreg_pmu_rfa_0p8>; > + vddrfa1p2-supply = <&vreg_pmu_rfa_1p2>; > + vddrfa1p8-supply = <&vreg_pmu_rfa_1p7>; > > max-speed = <3200000>; > - > - enable-gpios = <&tlmm 133 GPIO_ACTIVE_HIGH>; > - swctrl-gpios = <&tlmm 132 GPIO_ACTIVE_HIGH>; What about swctrl? You're just removing this pin from DT now without any comment on why you think that is the right thing to do. Should this one also be an input to the PMU block? > - > - pinctrl-0 = <&bt_default>; > - pinctrl-names = "default"; > }; > }; > > @@ -1761,4 +1826,11 @@ reset-pins { > bias-disable; > }; > }; > + > + wlan_en: wlan-en-state { > + pins = "gpio134"; > + function = "gpio"; > + drive-strength = <8>; Yet another drive strength? Also from fw config? > + bias-pull-down; > + }; > }; Johan
On Thu, Oct 3, 2024 at 1:07 PM Johan Hovold <johan@kernel.org> wrote: > > On Mon, Sep 30, 2024 at 12:30:39PM +0200, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > Add a node for the PMU of the WCN6855 and rework the inputs of the wifi > > and bluetooth nodes to consume the PMU's outputs. > > > > With this we can drop the regulator-always-on properties from vreg_s11b > > and vreg_s12b as they will now be enabled by the power sequencing > > driver. > > > > Tested-by: Steev Klimaszewski <steev@kali.org> # Thinkpad X13s > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > Without this patch I'm seeing an indefinite probe deferral with > 6.12-rc1: > > platform 1c00000.pcie:pcie@0:wifi@0: deferred probe pending: pci-pwrctl-pwrseq: Failed to get the power sequencer > > Can you please look into that and make sure that the existing DT > continues to work without such warnings. > Ah, dammit, I missed the fact that X13s already has this node defined so PCI pwrctl will consume it and try to get the power sequencer handle. I'm wondering how to tackle it though... It will most likely require some kind of a driver quirk where we check if we have the PMU node and if not, then don't try to set up power sequencing. Any other ideas? > > - > > - enable-gpios = <&tlmm 133 GPIO_ACTIVE_HIGH>; > > - swctrl-gpios = <&tlmm 132 GPIO_ACTIVE_HIGH>; > > What about swctrl? You're just removing this pin from DT now without any > comment on why you think that is the right thing to do. > > Should this one also be an input to the PMU block? > I recently added it to the bindings as an optional property. It's technically an output of the PMU to the host indicating the state of the clock supply to the BT module. We're not really using it but I can keep it here if you prefer. Bart
On Thu, 3 Oct 2024 13:38:35 +0200, Bartosz Golaszewski <brgl@bgdev.pl> said: > On Thu, Oct 3, 2024 at 1:07 PM Johan Hovold <johan@kernel.org> wrote: >> >> Without this patch I'm seeing an indefinite probe deferral with >> 6.12-rc1: >> >> platform 1c00000.pcie:pcie@0:wifi@0: deferred probe pending: pci-pwrctl-pwrseq: Failed to get the power sequencer >> >> Can you please look into that and make sure that the existing DT >> continues to work without such warnings. >> > > Ah, dammit, I missed the fact that X13s already has this node defined > so PCI pwrctl will consume it and try to get the power sequencer > handle. I'm wondering how to tackle it though... It will most likely > require some kind of a driver quirk where we check if we have the PMU > node and if not, then don't try to set up power sequencing. Any other > ideas? > This is untested but would it make sense? diff --git a/drivers/pci/pwrctl/pci-pwrctl-pwrseq.c b/drivers/pci/pwrctl/pci-pwrctl-pwrseq.c index a23a4312574b..071ee77c763d 100644 --- a/drivers/pci/pwrctl/pci-pwrctl-pwrseq.c +++ b/drivers/pci/pwrctl/pci-pwrctl-pwrseq.c @@ -3,6 +3,7 @@ * Copyright (C) 2024 Linaro Ltd. */ +#include <linux/cleanup.h> #include <linux/device.h> #include <linux/mod_devicetable.h> #include <linux/module.h> @@ -87,7 +88,31 @@ static struct platform_driver pci_pwrctl_pwrseq_driver = { }, .probe = pci_pwrctl_pwrseq_probe, }; -module_platform_driver(pci_pwrctl_pwrseq_driver); + +static int __init pci_pwrctl_pwrseq_init(void) +{ + /* + * Old device trees for the Lenovo X13s have the "pci17cb,1103" node + * defined but don't use power sequencing yet. If we register this + * driver, it will match against this node and lead to emitting of + * a warning in the kernel log when we cannot get the power sequencing + * handle. Let's skip registering the platform driver if we're on X13s + * but don't have the PMU node. + */ + if (of_machine_is_compatible("lenovo,thinkpad-x13s")) { + struct device_node *dn __free(device_node) = + of_find_compatible_node(NULL, NULL, "qcom,wcn6855-pmu"); + if (!dn) + return 0; + } + + return platform_driver_register(&pci_pwrctl_pwrseq_driver); +} + +static void __exit pci_pwrctl_pwrseq_exit(void) +{ + platform_driver_unregister(&pci_pwrctl_pwrseq_driver); +} MODULE_AUTHOR("Bartosz Golaszewski <bartosz.golaszewski@linaro.org>"); MODULE_DESCRIPTION("Generic PCI Power Control module for power sequenced devices"); X13s is the only platform that would use one of the compatibles supported by this driver before power sequencing so it should be a one-off quirk. Bart
On Thu, Oct 03, 2024 at 05:16:59AM -0700, Bartosz Golaszewski wrote: > On Thu, 3 Oct 2024 13:38:35 +0200, Bartosz Golaszewski <brgl@bgdev.pl> said: > > On Thu, Oct 3, 2024 at 1:07 PM Johan Hovold <johan@kernel.org> wrote: > >> > >> Without this patch I'm seeing an indefinite probe deferral with > >> 6.12-rc1: > >> > >> platform 1c00000.pcie:pcie@0:wifi@0: deferred probe pending: pci-pwrctl-pwrseq: Failed to get the power sequencer > >> > >> Can you please look into that and make sure that the existing DT > >> continues to work without such warnings. > >> > > > > Ah, dammit, I missed the fact that X13s already has this node defined > > so PCI pwrctl will consume it and try to get the power sequencer > > handle. I'm wondering how to tackle it though... It will most likely > > require some kind of a driver quirk where we check if we have the PMU > > node and if not, then don't try to set up power sequencing. Any other > > ideas? > > > > This is untested but would it make sense? > > diff --git a/drivers/pci/pwrctl/pci-pwrctl-pwrseq.c > b/drivers/pci/pwrctl/pci-pwrctl-pwrseq.c > index a23a4312574b..071ee77c763d 100644 > --- a/drivers/pci/pwrctl/pci-pwrctl-pwrseq.c > +++ b/drivers/pci/pwrctl/pci-pwrctl-pwrseq.c > @@ -3,6 +3,7 @@ > * Copyright (C) 2024 Linaro Ltd. > */ > > +#include <linux/cleanup.h> > #include <linux/device.h> > #include <linux/mod_devicetable.h> > #include <linux/module.h> > @@ -87,7 +88,31 @@ static struct platform_driver pci_pwrctl_pwrseq_driver = { > }, > .probe = pci_pwrctl_pwrseq_probe, > }; > -module_platform_driver(pci_pwrctl_pwrseq_driver); > + > +static int __init pci_pwrctl_pwrseq_init(void) > +{ > + /* > + * Old device trees for the Lenovo X13s have the "pci17cb,1103" node > + * defined but don't use power sequencing yet. If we register this > + * driver, it will match against this node and lead to emitting of > + * a warning in the kernel log when we cannot get the power sequencing > + * handle. Let's skip registering the platform driver if we're on X13s > + * but don't have the PMU node. > + */ > + if (of_machine_is_compatible("lenovo,thinkpad-x13s")) { > + struct device_node *dn __free(device_node) = > + of_find_compatible_node(NULL, NULL, "qcom,wcn6855-pmu"); > + if (!dn) > + return 0; > + } > + > + return platform_driver_register(&pci_pwrctl_pwrseq_driver); > +} > + > +static void __exit pci_pwrctl_pwrseq_exit(void) > +{ > + platform_driver_unregister(&pci_pwrctl_pwrseq_driver); > +} > > MODULE_AUTHOR("Bartosz Golaszewski <bartosz.golaszewski@linaro.org>"); > MODULE_DESCRIPTION("Generic PCI Power Control module for power > sequenced devices"); > > X13s is the only platform that would use one of the compatibles supported by > this driver before power sequencing so it should be a one-off quirk. > I'm guessing the pci17cb,1107 node in x1e80100-lenovo-yoga-slim7x is also affected? Maybe you can check if the node has one of the -supply properties and return -ENODEV from pci_pwrctl_pwrseq_probe() otherwise? Thanks, Stephan
On Fri, Oct 4, 2024 at 1:04 PM Stephan Gerhold <stephan.gerhold@linaro.org> wrote: > > On Thu, Oct 03, 2024 at 05:16:59AM -0700, Bartosz Golaszewski wrote: > > On Thu, 3 Oct 2024 13:38:35 +0200, Bartosz Golaszewski <brgl@bgdev.pl> said: > > > On Thu, Oct 3, 2024 at 1:07 PM Johan Hovold <johan@kernel.org> wrote: > > >> > > >> Without this patch I'm seeing an indefinite probe deferral with > > >> 6.12-rc1: > > >> > > >> platform 1c00000.pcie:pcie@0:wifi@0: deferred probe pending: pci-pwrctl-pwrseq: Failed to get the power sequencer > > >> > > >> Can you please look into that and make sure that the existing DT > > >> continues to work without such warnings. > > >> > > > > > > Ah, dammit, I missed the fact that X13s already has this node defined > > > so PCI pwrctl will consume it and try to get the power sequencer > > > handle. I'm wondering how to tackle it though... It will most likely > > > require some kind of a driver quirk where we check if we have the PMU > > > node and if not, then don't try to set up power sequencing. Any other > > > ideas? > > > > > > > This is untested but would it make sense? > > > > diff --git a/drivers/pci/pwrctl/pci-pwrctl-pwrseq.c > > b/drivers/pci/pwrctl/pci-pwrctl-pwrseq.c > > index a23a4312574b..071ee77c763d 100644 > > --- a/drivers/pci/pwrctl/pci-pwrctl-pwrseq.c > > +++ b/drivers/pci/pwrctl/pci-pwrctl-pwrseq.c > > @@ -3,6 +3,7 @@ > > * Copyright (C) 2024 Linaro Ltd. > > */ > > > > +#include <linux/cleanup.h> > > #include <linux/device.h> > > #include <linux/mod_devicetable.h> > > #include <linux/module.h> > > @@ -87,7 +88,31 @@ static struct platform_driver pci_pwrctl_pwrseq_driver = { > > }, > > .probe = pci_pwrctl_pwrseq_probe, > > }; > > -module_platform_driver(pci_pwrctl_pwrseq_driver); > > + > > +static int __init pci_pwrctl_pwrseq_init(void) > > +{ > > + /* > > + * Old device trees for the Lenovo X13s have the "pci17cb,1103" node > > + * defined but don't use power sequencing yet. If we register this > > + * driver, it will match against this node and lead to emitting of > > + * a warning in the kernel log when we cannot get the power sequencing > > + * handle. Let's skip registering the platform driver if we're on X13s > > + * but don't have the PMU node. > > + */ > > + if (of_machine_is_compatible("lenovo,thinkpad-x13s")) { > > + struct device_node *dn __free(device_node) = > > + of_find_compatible_node(NULL, NULL, "qcom,wcn6855-pmu"); > > + if (!dn) > > + return 0; > > + } > > + > > + return platform_driver_register(&pci_pwrctl_pwrseq_driver); > > +} > > + > > +static void __exit pci_pwrctl_pwrseq_exit(void) > > +{ > > + platform_driver_unregister(&pci_pwrctl_pwrseq_driver); > > +} > > > > MODULE_AUTHOR("Bartosz Golaszewski <bartosz.golaszewski@linaro.org>"); > > MODULE_DESCRIPTION("Generic PCI Power Control module for power > > sequenced devices"); > > > > X13s is the only platform that would use one of the compatibles supported by > > this driver before power sequencing so it should be a one-off quirk. > > > > I'm guessing the pci17cb,1107 node in x1e80100-lenovo-yoga-slim7x is > also affected? > > Maybe you can check if the node has one of the -supply properties and > return -ENODEV from pci_pwrctl_pwrseq_probe() otherwise? > Yeah, that may be a better solution I suppose. Bart
diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts index 6a28cab97189..7230d5420199 100644 --- a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts +++ b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts @@ -400,6 +400,67 @@ usb1_sbu_mux: endpoint { }; }; }; + + wcn6855-pmu { + compatible = "qcom,wcn6855-pmu"; + + pinctrl-0 = <&bt_default>, <&wlan_en>; + pinctrl-names = "default"; + + wlan-enable-gpios = <&tlmm 134 GPIO_ACTIVE_HIGH>; + bt-enable-gpios = <&tlmm 133 GPIO_ACTIVE_HIGH>; + + vddio-supply = <&vreg_s10b>; + vddaon-supply = <&vreg_s12b>; + vddpmu-supply = <&vreg_s12b>; + vddrfa0p95-supply = <&vreg_s12b>; + vddrfa1p3-supply = <&vreg_s11b>; + vddrfa1p9-supply = <&vreg_s1c>; + vddpcie1p3-supply = <&vreg_s11b>; + vddpcie1p9-supply = <&vreg_s1c>; + + regulators { + vreg_pmu_rfa_cmn_0p8: ldo0 { + regulator-name = "vreg_pmu_rfa_cmn_0p8"; + }; + + vreg_pmu_aon_0p8: ldo1 { + regulator-name = "vreg_pmu_aon_0p8"; + }; + + vreg_pmu_wlcx_0p8: ldo2 { + regulator-name = "vreg_pmu_wlcx_0p8"; + }; + + vreg_pmu_wlmx_0p8: ldo3 { + regulator-name = "vreg_pmu_wlmx_0p8"; + }; + + vreg_pmu_btcmx_0p8: ldo4 { + regulator-name = "vreg_pmu_btcmx_0p8"; + }; + + vreg_pmu_pcie_1p8: ldo5 { + regulator-name = "vreg_pmu_pcie_1p8"; + }; + + vreg_pmu_pcie_0p9: ldo6 { + regulator-name = "vreg_pmu_pcie_0p9"; + }; + + vreg_pmu_rfa_0p8: ldo7 { + regulator-name = "vreg_pmu_rfa_0p8"; + }; + + vreg_pmu_rfa_1p2: ldo8 { + regulator-name = "vreg_pmu_rfa_1p2"; + }; + + vreg_pmu_rfa_1p7: ldo9 { + regulator-name = "vreg_pmu_rfa_1p7"; + }; + }; + }; }; &apps_rsc { @@ -426,7 +487,6 @@ vreg_s11b: smps11 { regulator-min-microvolt = <1272000>; regulator-max-microvolt = <1272000>; regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>; - regulator-always-on; }; vreg_s12b: smps12 { @@ -434,7 +494,6 @@ vreg_s12b: smps12 { regulator-min-microvolt = <984000>; regulator-max-microvolt = <984000>; regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>; - regulator-always-on; }; vreg_l1b: ldo1 { @@ -927,6 +986,16 @@ wifi@0 { compatible = "pci17cb,1103"; reg = <0x10000 0x0 0x0 0x0 0x0>; + vddrfacmn-supply = <&vreg_pmu_rfa_cmn_0p8>; + vddaon-supply = <&vreg_pmu_aon_0p8>; + vddwlcx-supply = <&vreg_pmu_wlcx_0p8>; + vddwlmx-supply = <&vreg_pmu_wlmx_0p8>; + vddpcie1p8-supply = <&vreg_pmu_pcie_1p8>; + vddpcie0p9-supply = <&vreg_pmu_pcie_0p9>; + vddrfa0p8-supply = <&vreg_pmu_rfa_0p8>; + vddrfa1p2-supply = <&vreg_pmu_rfa_1p2>; + vddrfa1p8-supply = <&vreg_pmu_rfa_1p7>; + qcom,ath11k-calibration-variant = "LE_X13S"; }; }; @@ -1258,20 +1327,16 @@ &uart2 { bluetooth { compatible = "qcom,wcn6855-bt"; - vddio-supply = <&vreg_s10b>; - vddbtcxmx-supply = <&vreg_s12b>; - vddrfacmn-supply = <&vreg_s12b>; - vddrfa0p8-supply = <&vreg_s12b>; - vddrfa1p2-supply = <&vreg_s11b>; - vddrfa1p7-supply = <&vreg_s1c>; + vddrfacmn-supply = <&vreg_pmu_rfa_cmn_0p8>; + vddaon-supply = <&vreg_pmu_aon_0p8>; + vddwlcx-supply = <&vreg_pmu_wlcx_0p8>; + vddwlmx-supply = <&vreg_pmu_wlmx_0p8>; + vddbtcmx-supply = <&vreg_pmu_btcmx_0p8>; + vddrfa0p8-supply = <&vreg_pmu_rfa_0p8>; + vddrfa1p2-supply = <&vreg_pmu_rfa_1p2>; + vddrfa1p8-supply = <&vreg_pmu_rfa_1p7>; max-speed = <3200000>; - - enable-gpios = <&tlmm 133 GPIO_ACTIVE_HIGH>; - swctrl-gpios = <&tlmm 132 GPIO_ACTIVE_HIGH>; - - pinctrl-0 = <&bt_default>; - pinctrl-names = "default"; }; }; @@ -1761,4 +1826,11 @@ reset-pins { bias-disable; }; }; + + wlan_en: wlan-en-state { + pins = "gpio134"; + function = "gpio"; + drive-strength = <8>; + bias-pull-down; + }; };