| Message ID | 20241007-x1e80100-pwrseq-qcp-v1-3-f7166510ab17@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | arm64: dts: qcom: x1e80100-qcp: Add WiFi/BT pwrseq | expand |
On Thu, Oct 10, 2024 at 11:34:44AM +0200, Johan Hovold wrote: > On Mon, Oct 07, 2024 at 08:22:27PM +0200, Stephan Gerhold wrote: > > Add the WiFi/BT nodes for QCP and describe the regulators for the WCN7850 > > combo chip using the new power sequencing bindings. All voltages are > > derived from chained fixed regulators controlled using a single GPIO. > > > > The same setup also works for CRD (and likely most of the other X1E80100 > > laptops). However, unlike the QCP they use soldered or removable M.2 cards > > supplied by a single 3.3V fixed regulator. The other necessary voltages are > > then derived inside the M.2 card. Describing this properly requires > > new bindings, so this commit only adds QCP for now. > > Based on our discussions it seems we do not really need to describe the > internal PMU at all for WCN7850 (as the bluetooth and wlan blocks can be > enabled indepdendently) so perhaps we can just restore the old binding > and drop most of this boilerplate for all boards. > I think there is no clear conclusion on that yet. The old bindings didn't describe any power supplies for WiFi at all. The pwrseq bindings are currently the only way to do that. We could potentially move all the "PMU supplies" to the WiFi/BT nodes and rely on reference counting to handle them. But I think it's better to wait how the M.2/generic PCI power control discussion turns out before investing any time to refactor the current solution. There are existing users of qcom,wcn7850-pmu already in 6.11, so I think it does not hurt to take this patch as-is for now. We can clean them up together later if needed. Thanks, Stephan
On Fri, Oct 11, 2024 at 12:11:43PM +0200, Stephan Gerhold wrote: > On Thu, Oct 10, 2024 at 11:34:44AM +0200, Johan Hovold wrote: > > Based on our discussions it seems we do not really need to describe the > > internal PMU at all for WCN7850 (as the bluetooth and wlan blocks can be > > enabled indepdendently) so perhaps we can just restore the old binding > > and drop most of this boilerplate for all boards. > > > > I think there is no clear conclusion on that yet. The old bindings > didn't describe any power supplies for WiFi at all. The pwrseq bindings > are currently the only way to do that. > > We could potentially move all the "PMU supplies" to the WiFi/BT nodes > and rely on reference counting to handle them. But I think it's better > to wait how the M.2/generic PCI power control discussion turns out > before investing any time to refactor the current solution. > > There are existing users of qcom,wcn7850-pmu already in 6.11, so I think > it does not hurt to take this patch as-is for now. We can clean them up > together later if needed. Sounds good. But can you please address the following warning that I see with this series: pwrseq-qcom_wcn wcn7850-pmu: supply vddio1p2 not found, using dummy regulator Not sure if it's the dtsi that's missing a supply if it's the driver that needs fixing. Johan
On Tue, Oct 15, 2024 at 02:27:56PM +0200, Johan Hovold wrote: > On Fri, Oct 11, 2024 at 12:11:43PM +0200, Stephan Gerhold wrote: > > On Thu, Oct 10, 2024 at 11:34:44AM +0200, Johan Hovold wrote: > > > > Based on our discussions it seems we do not really need to describe the > > > internal PMU at all for WCN7850 (as the bluetooth and wlan blocks can be > > > enabled indepdendently) so perhaps we can just restore the old binding > > > and drop most of this boilerplate for all boards. > > > > > > > I think there is no clear conclusion on that yet. The old bindings > > didn't describe any power supplies for WiFi at all. The pwrseq bindings > > are currently the only way to do that. > > > > We could potentially move all the "PMU supplies" to the WiFi/BT nodes > > and rely on reference counting to handle them. But I think it's better > > to wait how the M.2/generic PCI power control discussion turns out > > before investing any time to refactor the current solution. > > > > There are existing users of qcom,wcn7850-pmu already in 6.11, so I think > > it does not hurt to take this patch as-is for now. We can clean them up > > together later if needed. > > Sounds good. > > But can you please address the following warning that I see with this > series: > > pwrseq-qcom_wcn wcn7850-pmu: supply vddio1p2 not found, using dummy regulator > > Not sure if it's the dtsi that's missing a supply if it's the driver > that needs fixing. > It's the driver, the DT should be correct. This supply exists on the WCN7850 chip, but nothing is connected there on the QCP. Unfortunately, it's not entirely straightforward to drop the warning since the pwrseq-qcom-wcn driver uses the bulk regulator APIs and (AFAIK) there is no good way to make only one of the regulators optional there. @Bartosz: Any thoughts on this? sm8550-qrd and sm8550-hdk are also missing the vddio1p2-supply, so they probably have the same warning in latest mainline. Thanks, Stephan
On Wed, Oct 16, 2024 at 5:34 PM Stephan Gerhold <stephan.gerhold@linaro.org> wrote: > > On Tue, Oct 15, 2024 at 02:27:56PM +0200, Johan Hovold wrote: > > On Fri, Oct 11, 2024 at 12:11:43PM +0200, Stephan Gerhold wrote: > > > On Thu, Oct 10, 2024 at 11:34:44AM +0200, Johan Hovold wrote: > > > > > > Based on our discussions it seems we do not really need to describe the > > > > internal PMU at all for WCN7850 (as the bluetooth and wlan blocks can be > > > > enabled indepdendently) so perhaps we can just restore the old binding > > > > and drop most of this boilerplate for all boards. > > > > > > > > > > I think there is no clear conclusion on that yet. The old bindings > > > didn't describe any power supplies for WiFi at all. The pwrseq bindings > > > are currently the only way to do that. > > > > > > We could potentially move all the "PMU supplies" to the WiFi/BT nodes > > > and rely on reference counting to handle them. But I think it's better > > > to wait how the M.2/generic PCI power control discussion turns out > > > before investing any time to refactor the current solution. > > > > > > There are existing users of qcom,wcn7850-pmu already in 6.11, so I think > > > it does not hurt to take this patch as-is for now. We can clean them up > > > together later if needed. > > > > Sounds good. > > > > But can you please address the following warning that I see with this > > series: > > > > pwrseq-qcom_wcn wcn7850-pmu: supply vddio1p2 not found, using dummy regulator > > > > Not sure if it's the dtsi that's missing a supply if it's the driver > > that needs fixing. > > > > It's the driver, the DT should be correct. This supply exists on the > WCN7850 chip, but nothing is connected there on the QCP. > > Unfortunately, it's not entirely straightforward to drop the warning > since the pwrseq-qcom-wcn driver uses the bulk regulator APIs and > (AFAIK) there is no good way to make only one of the regulators optional > there. > > @Bartosz: Any thoughts on this? sm8550-qrd and sm8550-hdk are also > missing the vddio1p2-supply, so they probably have the same warning in > latest mainline. > How do others deal with it? I'm asking because I've been seeing these warnings for years on many platforms which makes me think they are not a high priority for anyone. The best approach would be to provide an optional bulk get for the regulator API. Or extend struct regulator_bulk_data with bool optional and take this into account. Mark: Any thoughts on this? Bart
On Thu, Oct 17, 2024 at 11:28:18AM +0200, Bartosz Golaszewski wrote: > How do others deal with it? I'm asking because I've been seeing these > warnings for years on many platforms which makes me think they are not > a high priority for anyone. > The best approach would be to provide an optional bulk get for the > regulator API. Or extend struct regulator_bulk_data with bool optional > and take this into account. > Mark: Any thoughts on this? Fix your driver to request the supplies that actually exist on the device rather than just some random supplies you hope will work?
On Thu, Oct 17, 2024 at 12:59 PM Mark Brown <broonie@kernel.org> wrote: > > On Thu, Oct 17, 2024 at 11:28:18AM +0200, Bartosz Golaszewski wrote: > > > How do others deal with it? I'm asking because I've been seeing these > > warnings for years on many platforms which makes me think they are not > > a high priority for anyone. > > > The best approach would be to provide an optional bulk get for the > > regulator API. Or extend struct regulator_bulk_data with bool optional > > and take this into account. > > > Mark: Any thoughts on this? > > Fix your driver to request the supplies that actually exist on the > device rather than just some random supplies you hope will work? Let me rephrase: the device has this supply but on this particular board nothing is connected to it. It does sound to me like an example of an "optional" supply. Do you have anything against making it possible to define optional supplies when using the bulk regulator APIs? Bartosz
On Thu, Oct 17, 2024 at 01:28:00PM +0200, Bartosz Golaszewski wrote: > On Thu, Oct 17, 2024 at 12:59 PM Mark Brown <broonie@kernel.org> wrote: > > Fix your driver to request the supplies that actually exist on the > > device rather than just some random supplies you hope will work? > Let me rephrase: the device has this supply but on this particular > board nothing is connected to it. It does sound to me like an example > of an "optional" supply. Do you have anything against making it > possible to define optional supplies when using the bulk regulator > APIs? Oh, right - please if asking questions ask a complete question rather than having a long email thread and adding an "any thoughts" at the end which makes it unclear what the actual question is. In general the expectation for optional supplies is that you will need to do something different depending on if the supply is there, that will tend to mean that it's fairly natural to do a separate request for it as well. What's the concrete use case here?
On Thu, Oct 17, 2024 at 2:01 PM Mark Brown <broonie@kernel.org> wrote: > > On Thu, Oct 17, 2024 at 01:28:00PM +0200, Bartosz Golaszewski wrote: > > On Thu, Oct 17, 2024 at 12:59 PM Mark Brown <broonie@kernel.org> wrote: > > > > Fix your driver to request the supplies that actually exist on the > > > device rather than just some random supplies you hope will work? > > > Let me rephrase: the device has this supply but on this particular > > board nothing is connected to it. It does sound to me like an example > > of an "optional" supply. Do you have anything against making it > > possible to define optional supplies when using the bulk regulator > > APIs? > > Oh, right - please if asking questions ask a complete question rather Sure, sorry. > than having a long email thread and adding an "any thoughts" at the end > which makes it unclear what the actual question is. In general the > expectation for optional supplies is that you will need to do something > different depending on if the supply is there, that will tend to mean > that it's fairly natural to do a separate request for it as well. > What's the concrete use case here? A device is wired differently on different platforms. It requests a bunch of supplies using devm_regulator_bulk_get(). One of them is unconnected on one of the platforms resulting in the "using dummy regulator" warning. Concrete use-case is: make all but one regulator mandatory when calling regulator_bulk_get(). My proposal is extending struct regulator_bulk_data with a boolean flag called "optional" which would result in the underlying _regulator_get() receiving the OPTIONAL_GET flag only for the regulators that are marked as such. Bartosz
On Thu, Oct 17, 2024 at 02:21:08PM +0200, Bartosz Golaszewski wrote: > A device is wired differently on different platforms. It requests a > bunch of supplies using devm_regulator_bulk_get(). One of them is > unconnected on one of the platforms resulting in the "using dummy > regulator" warning. > Concrete use-case is: make all but one regulator mandatory when > calling regulator_bulk_get(). My proposal is extending struct > regulator_bulk_data with a boolean flag called "optional" which would > result in the underlying _regulator_get() receiving the OPTIONAL_GET > flag only for the regulators that are marked as such. Sure, but doesn't the device need to know that this supply isn't connected - if we can just ignore the result then why bother powering the supply on at all?
diff --git a/arch/arm64/boot/dts/qcom/x1e80100-qcp.dts b/arch/arm64/boot/dts/qcom/x1e80100-qcp.dts index 1c3a6a7b3ed6..9977c2a505b9 100644 --- a/arch/arm64/boot/dts/qcom/x1e80100-qcp.dts +++ b/arch/arm64/boot/dts/qcom/x1e80100-qcp.dts @@ -17,6 +17,7 @@ / { aliases { serial0 = &uart21; + serial1 = &uart14; }; wcd938x: audio-codec { @@ -254,6 +255,101 @@ vreg_nvme: regulator-nvme { pinctrl-names = "default"; pinctrl-0 = <&nvme_reg_en>; }; + + vreg_wcn_3p3: regulator-wcn-3p3 { + compatible = "regulator-fixed"; + + regulator-name = "VREG_WCN_3P3"; + regulator-min-microvolt = <3300000>; + regulator-max-microvolt = <3300000>; + + gpio = <&tlmm 214 GPIO_ACTIVE_HIGH>; + enable-active-high; + + pinctrl-0 = <&wcn_sw_en>; + pinctrl-names = "default"; + + regulator-boot-on; + }; + + vreg_wcn_0p95: regulator-wcn-0p95 { + compatible = "regulator-fixed"; + + regulator-name = "VREG_WCN_0P95"; + regulator-min-microvolt = <950000>; + regulator-max-microvolt = <950000>; + + vin-supply = <&vreg_wcn_3p3>; + }; + + vreg_wcn_1p9: regulator-wcn-1p9 { + compatible = "regulator-fixed"; + + regulator-name = "VREG_WCN_1P9"; + regulator-min-microvolt = <1900000>; + regulator-max-microvolt = <1900000>; + + vin-supply = <&vreg_wcn_3p3>; + }; + + wcn7850-pmu { + compatible = "qcom,wcn7850-pmu"; + + vdd-supply = <&vreg_wcn_0p95>; + vddio-supply = <&vreg_l15b_1p8>; + vddaon-supply = <&vreg_wcn_0p95>; + vdddig-supply = <&vreg_wcn_0p95>; + vddrfa1p2-supply = <&vreg_wcn_1p9>; + vddrfa1p8-supply = <&vreg_wcn_1p9>; + + 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"; + + regulators { + vreg_pmu_rfa_cmn: ldo0 { + regulator-name = "vreg_pmu_rfa_cmn"; + }; + + vreg_pmu_aon_0p59: ldo1 { + regulator-name = "vreg_pmu_aon_0p59"; + }; + + vreg_pmu_wlcx_0p8: ldo2 { + regulator-name = "vreg_pmu_wlcx_0p8"; + }; + + vreg_pmu_wlmx_0p85: ldo3 { + regulator-name = "vreg_pmu_wlmx_0p85"; + }; + + vreg_pmu_btcmx_0p85: ldo4 { + regulator-name = "vreg_pmu_btcmx_0p85"; + }; + + vreg_pmu_rfa_0p8: ldo5 { + regulator-name = "vreg_pmu_rfa_0p8"; + }; + + vreg_pmu_rfa_1p2: ldo6 { + regulator-name = "vreg_pmu_rfa_1p2"; + }; + + vreg_pmu_rfa_1p8: ldo7 { + regulator-name = "vreg_pmu_rfa_1p8"; + }; + + vreg_pmu_pcie_0p9: ldo8 { + regulator-name = "vreg_pmu_pcie_0p9"; + }; + + vreg_pmu_pcie_1p8: ldo9 { + regulator-name = "vreg_pmu_pcie_1p8"; + }; + }; + }; }; &apps_rsc { @@ -684,6 +780,23 @@ &pcie4_phy { status = "okay"; }; +&pcie4_port0 { + wifi@0 { + compatible = "pci17cb,1107"; + reg = <0x10000 0x0 0x0 0x0 0x0>; + + vddaon-supply = <&vreg_pmu_aon_0p59>; + vddwlcx-supply = <&vreg_pmu_wlcx_0p8>; + vddwlmx-supply = <&vreg_pmu_wlmx_0p85>; + vddrfacmn-supply = <&vreg_pmu_rfa_cmn>; + vddrfa0p8-supply = <&vreg_pmu_rfa_0p8>; + vddrfa1p2-supply = <&vreg_pmu_rfa_1p2>; + vddrfa1p8-supply = <&vreg_pmu_rfa_1p8>; + vddpcie0p9-supply = <&vreg_pmu_pcie_0p9>; + vddpcie1p8-supply = <&vreg_pmu_pcie_1p8>; + }; +}; + &pcie6a { perst-gpios = <&tlmm 152 GPIO_ACTIVE_LOW>; wake-gpios = <&tlmm 154 GPIO_ACTIVE_LOW>; @@ -877,6 +990,37 @@ wcd_default: wcd-reset-n-active-state { bias-disable; output-low; }; + + wcn_wlan_bt_en: wcn-wlan-bt-en-state { + pins = "gpio116", "gpio117"; + function = "gpio"; + drive-strength = <2>; + bias-disable; + }; + + wcn_sw_en: wcn-sw-en-state { + pins = "gpio214"; + function = "gpio"; + drive-strength = <2>; + bias-disable; + }; +}; + +&uart14 { + status = "okay"; + + bluetooth { + compatible = "qcom,wcn7850-bt"; + max-speed = <3200000>; + + vddaon-supply = <&vreg_pmu_aon_0p59>; + vddwlcx-supply = <&vreg_pmu_wlcx_0p8>; + vddwlmx-supply = <&vreg_pmu_wlmx_0p85>; + vddrfacmn-supply = <&vreg_pmu_rfa_cmn>; + vddrfa0p8-supply = <&vreg_pmu_rfa_0p8>; + vddrfa1p2-supply = <&vreg_pmu_rfa_1p2>; + vddrfa1p8-supply = <&vreg_pmu_rfa_1p8>; + }; }; &uart21 {
Add the WiFi/BT nodes for QCP and describe the regulators for the WCN7850 combo chip using the new power sequencing bindings. All voltages are derived from chained fixed regulators controlled using a single GPIO. The same setup also works for CRD (and likely most of the other X1E80100 laptops). However, unlike the QCP they use soldered or removable M.2 cards supplied by a single 3.3V fixed regulator. The other necessary voltages are then derived inside the M.2 card. Describing this properly requires new bindings, so this commit only adds QCP for now. Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org> --- arch/arm64/boot/dts/qcom/x1e80100-qcp.dts | 144 ++++++++++++++++++++++++++++++ 1 file changed, 144 insertions(+)