| Message ID | 20240201155532.49707-3-brgl@bgdev.pl (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | power: sequencing: implement the subsystem and add first users | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | fail | error: patch failed: arch/arm64/boot/dts/qcom/qrb5165-rb5.dts:1303 error: arch/arm64/boot/dts/qcom/qrb5165-rb5.dts: patch does not apply hint: Use 'git am --show-current-patch' to see the failed patch |
| tedd_an/CheckPatch | warning | WARNING: DT compatible string "qcom,qca6390-pmu" appears un-documented -- check ./Documentation/devicetree/bindings/ #138: FILE: arch/arm64/boot/dts/qcom/qrb5165-rb5.dts:112: + compatible = "qcom,qca6390-pmu"; WARNING: DT compatible string "pci17cb,1101" appears un-documented -- check ./Documentation/devicetree/bindings/ #227: FILE: arch/arm64/boot/dts/qcom/qrb5165-rb5.dts:820: + compatible = "pci17cb,1101"; WARNING: DT compatible string vendor "pci17cb" appears un-documented -- check ./Documentation/devicetree/bindings/vendor-prefixes.yaml #227: FILE: arch/arm64/boot/dts/qcom/qrb5165-rb5.dts:820: + compatible = "pci17cb,1101"; total: 0 errors, 3 warnings, 168 lines checked NOTE: For some of the reported defects, checkpatch may be able to mechanically convert to the typical style using --fix or --fix-inplace. /github/workspace/src/src/13541336.patch has style problems, please review. NOTE: Ignored message types: UNKNOWN_COMMIT_ID NOTE: If any of the errors are false positives, please report them to the maintainer, see CHECKPATCH in MAINTAINERS. |
| tedd_an/GitLint | success | Gitlint PASS |
| tedd_an/SubjectPrefix | fail | "Bluetooth: " prefix is not specified in the subject |
| tedd_an/IncrementalBuild | success | Incremental Build PASS |
On Thu, Feb 01, 2024 at 04:55:25PM +0100, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > Add a node for the PMU module of the QCA6391 present on the RB5 board. > Assign its LDO power outputs to the existing Bluetooth module. Add a > node for the PCIe port to sm8250.dtsi and define the WLAN node on it in > the board's .dts and also make it consume the power outputs of the PMU. > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > --- > arch/arm64/boot/dts/qcom/qrb5165-rb5.dts | 128 +++++++++++++++++++++-- > arch/arm64/boot/dts/qcom/sm8250.dtsi | 10 ++ > 2 files changed, 127 insertions(+), 11 deletions(-) > > diff --git a/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts b/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts > index cd0db4f31d4a..fab5bebafbad 100644 > --- a/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts > +++ b/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts > @@ -108,6 +108,87 @@ lt9611_3v3: lt9611-3v3 { > regulator-always-on; > }; > > + qca6390_pmu: pmu@0 { > + compatible = "qcom,qca6390-pmu"; > + > + pinctrl-names = "default"; > + pinctrl-0 = <&bt_en_state>, <&wlan_en_state>; > + > + vddaon-supply = <&vreg_s6a_0p95>; > + vddpmu-supply = <&vreg_s2f_0p95>; > + vddrfa1-supply = <&vreg_s2f_0p95>; > + vddrfa2-supply = <&vreg_s8c_1p3>; > + vddrfa3-supply = <&vreg_s5a_1p9>; > + vddpcie1-supply = <&vreg_s8c_1p3>; > + vddpcie2-supply = <&vreg_s5a_1p9>; > + vddio-supply = <&vreg_s4a_1p8>; > + > + wlan-enable-gpios = <&tlmm 20 GPIO_ACTIVE_HIGH>; > + bt-enable-gpios = <&tlmm 21 GPIO_ACTIVE_HIGH>; > + > + regulators { > + vreg_pmu_rfa_cmn: ldo0 { > + regulator-name = "vreg_pmu_rfa_cmn"; > + regulator-min-microvolt = <760000>; > + regulator-max-microvolt = <840000>; I'm still not convinced that the PMU has a set of LDOs, and looking at your implementation you neither register these with the regulator framework, nor provide any means of controlling the state or voltage of these "regulators". [..] > > &uart6 { > @@ -1311,17 +1418,16 @@ &uart6 { > bluetooth { > compatible = "qcom,qca6390-bt"; > > - pinctrl-names = "default"; > - pinctrl-0 = <&bt_en_state>; > - > - enable-gpios = <&tlmm 21 GPIO_ACTIVE_HIGH>; > - > - vddio-supply = <&vreg_s4a_1p8>; > - vddpmu-supply = <&vreg_s2f_0p95>; > - vddaon-supply = <&vreg_s6a_0p95>; > - vddrfa0p9-supply = <&vreg_s2f_0p95>; > - vddrfa1p3-supply = <&vreg_s8c_1p3>; > - vddrfa1p9-supply = <&vreg_s5a_1p9>; > + vddrfacmn-supply = <&vreg_pmu_rfa_cmn>; > + vddaon-supply = <&vreg_pmu_aon_0p59>; > + vddwlcx-supply = <&vreg_pmu_wlcx_0p8>; > + vddwlmx-supply = <&vreg_pmu_wlmx_0p85>; > + vddbtcmx-supply = <&vreg_pmu_btcmx_0p85>; > + vddrfa0-supply = <&vreg_pmu_rfa_0p8>; > + vddrfa1-supply = <&vreg_pmu_rfa_1p2>; > + vddrfa2-supply = <&vreg_pmu_rfa_1p7>; > + vddpcie0-supply = <&vreg_pmu_pcie_0p9>; > + vddpcie1-supply = <&vreg_pmu_pcie_1p8>; As I asked before, why does bluetooth suddenly care about PCIe supplies? Regards, Bjorn > }; > }; > > diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi > index 4d849e98bf9b..7cd21d4e7278 100644 > --- a/arch/arm64/boot/dts/qcom/sm8250.dtsi > +++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi > @@ -2203,6 +2203,16 @@ pcie0: pcie@1c00000 { > dma-coherent; > > status = "disabled"; > + > + pcieport0: pcie@0 { > + device_type = "pci"; > + reg = <0x0 0x0 0x0 0x0 0x0>; > + #address-cells = <3>; > + #size-cells = <2>; > + ranges; > + > + bus-range = <0x01 0xff>; > + }; > }; > > pcie0_phy: phy@1c06000 { > -- > 2.40.1 >
On Fri, 2 Feb 2024 at 06:34, Bjorn Andersson <andersson@kernel.org> wrote: > > On Thu, Feb 01, 2024 at 04:55:25PM +0100, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > Add a node for the PMU module of the QCA6391 present on the RB5 board. > > Assign its LDO power outputs to the existing Bluetooth module. Add a > > node for the PCIe port to sm8250.dtsi and define the WLAN node on it in > > the board's .dts and also make it consume the power outputs of the PMU. > > > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > --- > > arch/arm64/boot/dts/qcom/qrb5165-rb5.dts | 128 +++++++++++++++++++++-- > > arch/arm64/boot/dts/qcom/sm8250.dtsi | 10 ++ > > 2 files changed, 127 insertions(+), 11 deletions(-) > > > > diff --git a/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts b/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts > > index cd0db4f31d4a..fab5bebafbad 100644 > > --- a/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts > > +++ b/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts > > @@ -108,6 +108,87 @@ lt9611_3v3: lt9611-3v3 { > > regulator-always-on; > > }; > > > > + qca6390_pmu: pmu@0 { > > + compatible = "qcom,qca6390-pmu"; > > + > > + pinctrl-names = "default"; > > + pinctrl-0 = <&bt_en_state>, <&wlan_en_state>; > > + > > + vddaon-supply = <&vreg_s6a_0p95>; > > + vddpmu-supply = <&vreg_s2f_0p95>; > > + vddrfa1-supply = <&vreg_s2f_0p95>; > > + vddrfa2-supply = <&vreg_s8c_1p3>; > > + vddrfa3-supply = <&vreg_s5a_1p9>; > > + vddpcie1-supply = <&vreg_s8c_1p3>; > > + vddpcie2-supply = <&vreg_s5a_1p9>; > > + vddio-supply = <&vreg_s4a_1p8>; > > + > > + wlan-enable-gpios = <&tlmm 20 GPIO_ACTIVE_HIGH>; > > + bt-enable-gpios = <&tlmm 21 GPIO_ACTIVE_HIGH>; > > + > > + regulators { > > + vreg_pmu_rfa_cmn: ldo0 { > > + regulator-name = "vreg_pmu_rfa_cmn"; > > + regulator-min-microvolt = <760000>; > > + regulator-max-microvolt = <840000>; > > I'm still not convinced that the PMU has a set of LDOs, and looking at > your implementation you neither register these with the regulator > framework, nor provide any means of controlling the state or voltage of > these "regulators". Please take a look at the description of VDD08_PMU_RFA_CMN and VDD_PMU_AON_I pins in the spec (80-WL522-1, page 25). I'm not sure if I'm allowed to quote it, so I won't. But the spec clearly describes VDD_PMU_AON_I as 0.95V LDO input and VDD08_PMU_RFA_CMN as 0.8 LDO output generated using that input. I think this proves that the on-chip PMU has actual LDOs. I must admit, I find this representation very verbose, but on the other hand Bartosz is right, it represents actual hardware. Maybe we can drop some of the properties of corresponding regulator blocks, as we don't actually need them and they are internal properties of the hardware. > > [..] > > > > &uart6 { > > @@ -1311,17 +1418,16 @@ &uart6 { > > bluetooth { > > compatible = "qcom,qca6390-bt"; > > > > - pinctrl-names = "default"; > > - pinctrl-0 = <&bt_en_state>; > > - > > - enable-gpios = <&tlmm 21 GPIO_ACTIVE_HIGH>; > > - > > - vddio-supply = <&vreg_s4a_1p8>; > > - vddpmu-supply = <&vreg_s2f_0p95>; > > - vddaon-supply = <&vreg_s6a_0p95>; > > - vddrfa0p9-supply = <&vreg_s2f_0p95>; > > - vddrfa1p3-supply = <&vreg_s8c_1p3>; > > - vddrfa1p9-supply = <&vreg_s5a_1p9>; > > + vddrfacmn-supply = <&vreg_pmu_rfa_cmn>; > > + vddaon-supply = <&vreg_pmu_aon_0p59>; > > + vddwlcx-supply = <&vreg_pmu_wlcx_0p8>; > > + vddwlmx-supply = <&vreg_pmu_wlmx_0p85>; > > + vddbtcmx-supply = <&vreg_pmu_btcmx_0p85>; > > + vddrfa0-supply = <&vreg_pmu_rfa_0p8>; > > + vddrfa1-supply = <&vreg_pmu_rfa_1p2>; > > + vddrfa2-supply = <&vreg_pmu_rfa_1p7>; > > + vddpcie0-supply = <&vreg_pmu_pcie_0p9>; > > + vddpcie1-supply = <&vreg_pmu_pcie_1p8>; > > As I asked before, why does bluetooth suddenly care about PCIe supplies? Power sequencing in the same spec describes that PCIe voltages should be up even if only BT is being brought up. PMU itself handles distributing voltages according to the actual load needs. > > Regards, > Bjorn > > > }; > > }; > > > > diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi > > index 4d849e98bf9b..7cd21d4e7278 100644 > > --- a/arch/arm64/boot/dts/qcom/sm8250.dtsi > > +++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi > > @@ -2203,6 +2203,16 @@ pcie0: pcie@1c00000 { > > dma-coherent; > > > > status = "disabled"; > > + > > + pcieport0: pcie@0 { > > + device_type = "pci"; > > + reg = <0x0 0x0 0x0 0x0 0x0>; > > + #address-cells = <3>; > > + #size-cells = <2>; > > + ranges; > > + > > + bus-range = <0x01 0xff>; > > + }; > > }; > > > > pcie0_phy: phy@1c06000 { > > -- > > 2.40.1 > > >
On Fri, Feb 2, 2024 at 5:34 AM Bjorn Andersson <andersson@kernel.org> wrote: > [snip] > > + > > + wlan-enable-gpios = <&tlmm 20 GPIO_ACTIVE_HIGH>; > > + bt-enable-gpios = <&tlmm 21 GPIO_ACTIVE_HIGH>; > > + > > + regulators { > > + vreg_pmu_rfa_cmn: ldo0 { > > + regulator-name = "vreg_pmu_rfa_cmn"; > > + regulator-min-microvolt = <760000>; > > + regulator-max-microvolt = <840000>; > > I'm still not convinced that the PMU has a set of LDOs, and looking at > your implementation you neither register these with the regulator > framework, nor provide any means of controlling the state or voltage of > these "regulators". > Why are you so fixated on the driver implementation matching the device-tree 1:1? I asked that question before - what does it matter if we use the regulator subsystem or not? This is just what HW there is. What we do with that knowledge in C is irrelevant. Yes, I don't use the regulator subsystem because it's unnecessary and would actually get in the way of the power sequencing. But it doesn't change the fact that the regulators *are* there so let's show them. What isn't there is a "power sequencer device". This was the main concern about Dmitry's implementation before. We must not have "bt-pwrseq = <&...>;" -like properties in device-tree because there is no device that this would represent. But there *are* LDO outputs of the PMU which can be modelled and then used in C to retrieve the power sequencer and this is what I'm proposing. Bartosz > [..] > > > > &uart6 { > > @@ -1311,17 +1418,16 @@ &uart6 { > > bluetooth { > > compatible = "qcom,qca6390-bt"; > > > > - pinctrl-names = "default"; > > - pinctrl-0 = <&bt_en_state>; > > - > > - enable-gpios = <&tlmm 21 GPIO_ACTIVE_HIGH>; > > - > > - vddio-supply = <&vreg_s4a_1p8>; > > - vddpmu-supply = <&vreg_s2f_0p95>; > > - vddaon-supply = <&vreg_s6a_0p95>; > > - vddrfa0p9-supply = <&vreg_s2f_0p95>; > > - vddrfa1p3-supply = <&vreg_s8c_1p3>; > > - vddrfa1p9-supply = <&vreg_s5a_1p9>; > > + vddrfacmn-supply = <&vreg_pmu_rfa_cmn>; > > + vddaon-supply = <&vreg_pmu_aon_0p59>; > > + vddwlcx-supply = <&vreg_pmu_wlcx_0p8>; > > + vddwlmx-supply = <&vreg_pmu_wlmx_0p85>; > > + vddbtcmx-supply = <&vreg_pmu_btcmx_0p85>; > > + vddrfa0-supply = <&vreg_pmu_rfa_0p8>; > > + vddrfa1-supply = <&vreg_pmu_rfa_1p2>; > > + vddrfa2-supply = <&vreg_pmu_rfa_1p7>; > > + vddpcie0-supply = <&vreg_pmu_pcie_0p9>; > > + vddpcie1-supply = <&vreg_pmu_pcie_1p8>; > > As I asked before, why does bluetooth suddenly care about PCIe supplies? > Yes, I forgot to remove it, I'll do it next time. Bartosz [snip]
On Fri, Feb 02, 2024 at 06:59:48AM +0200, Dmitry Baryshkov wrote: > On Fri, 2 Feb 2024 at 06:34, Bjorn Andersson <andersson@kernel.org> wrote: > > > > On Thu, Feb 01, 2024 at 04:55:25PM +0100, Bartosz Golaszewski wrote: > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > > > Add a node for the PMU module of the QCA6391 present on the RB5 board. > > > Assign its LDO power outputs to the existing Bluetooth module. Add a > > > node for the PCIe port to sm8250.dtsi and define the WLAN node on it in > > > the board's .dts and also make it consume the power outputs of the PMU. > > > > > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > --- > > > arch/arm64/boot/dts/qcom/qrb5165-rb5.dts | 128 +++++++++++++++++++++-- > > > arch/arm64/boot/dts/qcom/sm8250.dtsi | 10 ++ > > > 2 files changed, 127 insertions(+), 11 deletions(-) > > > > > > diff --git a/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts b/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts > > > index cd0db4f31d4a..fab5bebafbad 100644 > > > --- a/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts > > > +++ b/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts > > > @@ -108,6 +108,87 @@ lt9611_3v3: lt9611-3v3 { > > > regulator-always-on; > > > }; > > > > > > + qca6390_pmu: pmu@0 { > > > + compatible = "qcom,qca6390-pmu"; > > > + > > > + pinctrl-names = "default"; > > > + pinctrl-0 = <&bt_en_state>, <&wlan_en_state>; > > > + > > > + vddaon-supply = <&vreg_s6a_0p95>; > > > + vddpmu-supply = <&vreg_s2f_0p95>; > > > + vddrfa1-supply = <&vreg_s2f_0p95>; > > > + vddrfa2-supply = <&vreg_s8c_1p3>; > > > + vddrfa3-supply = <&vreg_s5a_1p9>; > > > + vddpcie1-supply = <&vreg_s8c_1p3>; > > > + vddpcie2-supply = <&vreg_s5a_1p9>; > > > + vddio-supply = <&vreg_s4a_1p8>; > > > + > > > + wlan-enable-gpios = <&tlmm 20 GPIO_ACTIVE_HIGH>; > > > + bt-enable-gpios = <&tlmm 21 GPIO_ACTIVE_HIGH>; > > > + > > > + regulators { > > > + vreg_pmu_rfa_cmn: ldo0 { > > > + regulator-name = "vreg_pmu_rfa_cmn"; > > > + regulator-min-microvolt = <760000>; > > > + regulator-max-microvolt = <840000>; > > > > I'm still not convinced that the PMU has a set of LDOs, and looking at > > your implementation you neither register these with the regulator > > framework, nor provide any means of controlling the state or voltage of > > these "regulators". > > Please take a look at the description of VDD08_PMU_RFA_CMN and > VDD_PMU_AON_I pins in the spec (80-WL522-1, page 25). I'm not sure if > I'm allowed to quote it, so I won't. But the spec clearly describes > VDD_PMU_AON_I as 0.95V LDO input and VDD08_PMU_RFA_CMN as 0.8 LDO > output generated using that input. I think this proves that the > on-chip PMU has actual LDOs. > You're correct, thank you for the pointer and clarification. I now agree with you, the PMU consumes what I saw as the chip input supplies, and based on WL_EN and BT_EN will provide power on pads, which are then externally routed to respective block. > I must admit, I find this representation very verbose, but on the > other hand Bartosz is right, it represents actual hardware. I agree, this is actual hardware. > Maybe we > can drop some of the properties of corresponding regulator blocks, as > we don't actually need them and they are internal properties of the > hardware. > To me this really looks like a fancy "regulator-fixed" with multiple inputs, two gpios and multiple outputs. This would also imply that we don't need to invent the power sequence framework to tie WiFi and BT to the PMU's state. The PMU is a thing, so we can represent that in DeviceTree, it consumes M input power rails, and two gpios, it provides N WiFi supplies and O BT supplies (with some overlap between N and O). The WiFi node consumes its N supplies, the BT node consumes its O supplies. If any of the N regulators are requested enabled the qca6390-pmu driver enables all M input rails, then enables WL_EN. If any of the O BT regulators are requested enabled, the driver enables all M input rails, then enables BT_EN. > > > > [..] > > > > > > &uart6 { > > > @@ -1311,17 +1418,16 @@ &uart6 { > > > bluetooth { > > > compatible = "qcom,qca6390-bt"; > > > > > > - pinctrl-names = "default"; > > > - pinctrl-0 = <&bt_en_state>; > > > - > > > - enable-gpios = <&tlmm 21 GPIO_ACTIVE_HIGH>; > > > - > > > - vddio-supply = <&vreg_s4a_1p8>; > > > - vddpmu-supply = <&vreg_s2f_0p95>; > > > - vddaon-supply = <&vreg_s6a_0p95>; > > > - vddrfa0p9-supply = <&vreg_s2f_0p95>; > > > - vddrfa1p3-supply = <&vreg_s8c_1p3>; > > > - vddrfa1p9-supply = <&vreg_s5a_1p9>; > > > + vddrfacmn-supply = <&vreg_pmu_rfa_cmn>; > > > + vddaon-supply = <&vreg_pmu_aon_0p59>; > > > + vddwlcx-supply = <&vreg_pmu_wlcx_0p8>; > > > + vddwlmx-supply = <&vreg_pmu_wlmx_0p85>; > > > + vddbtcmx-supply = <&vreg_pmu_btcmx_0p85>; > > > + vddrfa0-supply = <&vreg_pmu_rfa_0p8>; > > > + vddrfa1-supply = <&vreg_pmu_rfa_1p2>; > > > + vddrfa2-supply = <&vreg_pmu_rfa_1p7>; > > > + vddpcie0-supply = <&vreg_pmu_pcie_0p9>; > > > + vddpcie1-supply = <&vreg_pmu_pcie_1p8>; > > > > As I asked before, why does bluetooth suddenly care about PCIe supplies? > > Power sequencing in the same spec describes that PCIe voltages should > be up even if only BT is being brought up. PMU itself handles > distributing voltages according to the actual load needs. > You're right, the power sequence diagram in the docs do indicate that VDD13_PMU_PCIE_I and VDD19_PMU_PCIE_I should be enabled before either WL_EN or BT_EN are driven high. But I don't see anything stating that the output from the PMU (VDD09_PMU_PCIE) in turn is fed to the bluetooth block. Regards, Bjorn > > > > Regards, > > Bjorn > > > > > }; > > > }; > > > > > > diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi > > > index 4d849e98bf9b..7cd21d4e7278 100644 > > > --- a/arch/arm64/boot/dts/qcom/sm8250.dtsi > > > +++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi > > > @@ -2203,6 +2203,16 @@ pcie0: pcie@1c00000 { > > > dma-coherent; > > > > > > status = "disabled"; > > > + > > > + pcieport0: pcie@0 { > > > + device_type = "pci"; > > > + reg = <0x0 0x0 0x0 0x0 0x0>; > > > + #address-cells = <3>; > > > + #size-cells = <2>; > > > + ranges; > > > + > > > + bus-range = <0x01 0xff>; > > > + }; > > > }; > > > > > > pcie0_phy: phy@1c06000 { > > > -- > > > 2.40.1 > > > > > > > > -- > With best wishes > Dmitry
On Fri, Feb 02, 2024 at 02:23:49PM +0100, Bartosz Golaszewski wrote: > On Fri, Feb 2, 2024 at 5:34 AM Bjorn Andersson <andersson@kernel.org> wrote: > > > > [snip] > > > > + > > > + wlan-enable-gpios = <&tlmm 20 GPIO_ACTIVE_HIGH>; > > > + bt-enable-gpios = <&tlmm 21 GPIO_ACTIVE_HIGH>; > > > + > > > + regulators { > > > + vreg_pmu_rfa_cmn: ldo0 { > > > + regulator-name = "vreg_pmu_rfa_cmn"; > > > + regulator-min-microvolt = <760000>; > > > + regulator-max-microvolt = <840000>; > > > > I'm still not convinced that the PMU has a set of LDOs, and looking at > > your implementation you neither register these with the regulator > > framework, nor provide any means of controlling the state or voltage of > > these "regulators". > > > > Why are you so fixated on the driver implementation matching the > device-tree 1:1? I asked that question before - what does it matter if > we use the regulator subsystem or not? I'm sorry, I must have missed this question. I'm not questioning why the DT needs to match the Linux implementation, I was really questioning if the hardware you describe here existed. > This is just what HW there is. > What we do with that knowledge in C is irrelevant. Yes, I don't use > the regulator subsystem because it's unnecessary and would actually > get in the way of the power sequencing. Then describe that in your commit messages. > But it doesn't change the fact > that the regulators *are* there so let's show them. > > What isn't there is a "power sequencer device". This was the main > concern about Dmitry's implementation before. I don't agree. The concerns that I saw being raised with Dmitry's proposed design was that he used connected the WiFi controller to the QCA6391 using power-domains, etc. > We must not have > "bt-pwrseq = <&...>;" -like properties in device-tree because there is > no device that this would represent. But there *are* LDO outputs of > the PMU which can be modelled and then used in C to retrieve the power > sequencer and this is what I'm proposing. > Performing device-specific power sequences is extremely common, but we so far don't have a separate abstraction of this because it's generally not an matter external to any given device. If we're going to introduce a power sequence framework, it needs to be made very clear that it is there to solve the problem that you have devices on separate busses that need to share that sequence. This also implies that for most examples out there where we have a need for doing "PCI power sequencing", I don't think we would use the power-sequence framework. Regards, Bjorn > Bartosz > > > [..] > > > > > > &uart6 { > > > @@ -1311,17 +1418,16 @@ &uart6 { > > > bluetooth { > > > compatible = "qcom,qca6390-bt"; > > > > > > - pinctrl-names = "default"; > > > - pinctrl-0 = <&bt_en_state>; > > > - > > > - enable-gpios = <&tlmm 21 GPIO_ACTIVE_HIGH>; > > > - > > > - vddio-supply = <&vreg_s4a_1p8>; > > > - vddpmu-supply = <&vreg_s2f_0p95>; > > > - vddaon-supply = <&vreg_s6a_0p95>; > > > - vddrfa0p9-supply = <&vreg_s2f_0p95>; > > > - vddrfa1p3-supply = <&vreg_s8c_1p3>; > > > - vddrfa1p9-supply = <&vreg_s5a_1p9>; > > > + vddrfacmn-supply = <&vreg_pmu_rfa_cmn>; > > > + vddaon-supply = <&vreg_pmu_aon_0p59>; > > > + vddwlcx-supply = <&vreg_pmu_wlcx_0p8>; > > > + vddwlmx-supply = <&vreg_pmu_wlmx_0p85>; > > > + vddbtcmx-supply = <&vreg_pmu_btcmx_0p85>; > > > + vddrfa0-supply = <&vreg_pmu_rfa_0p8>; > > > + vddrfa1-supply = <&vreg_pmu_rfa_1p2>; > > > + vddrfa2-supply = <&vreg_pmu_rfa_1p7>; > > > + vddpcie0-supply = <&vreg_pmu_pcie_0p9>; > > > + vddpcie1-supply = <&vreg_pmu_pcie_1p8>; > > > > As I asked before, why does bluetooth suddenly care about PCIe supplies? > > > > Yes, I forgot to remove it, I'll do it next time. > > Bartosz > > [snip]
On Thu, Feb 01, 2024 at 04:55:25PM +0100, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > Add a node for the PMU module of the QCA6391 present on the RB5 board. > Assign its LDO power outputs to the existing Bluetooth module. Add a > node for the PCIe port to sm8250.dtsi and define the WLAN node on it in > the board's .dts and also make it consume the power outputs of the PMU. > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > --- > arch/arm64/boot/dts/qcom/qrb5165-rb5.dts | 128 +++++++++++++++++++++-- > arch/arm64/boot/dts/qcom/sm8250.dtsi | 10 ++ > 2 files changed, 127 insertions(+), 11 deletions(-) > > diff --git a/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts b/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts > index cd0db4f31d4a..fab5bebafbad 100644 > --- a/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts > +++ b/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts > @@ -108,6 +108,87 @@ lt9611_3v3: lt9611-3v3 { > regulator-always-on; > }; > > + qca6390_pmu: pmu@0 { The node doesn't have an address, so why does it have a unit address? Also, the node isn't referenced, so please skip the label. > + compatible = "qcom,qca6390-pmu"; > + > + pinctrl-names = "default"; > + pinctrl-0 = <&bt_en_state>, <&wlan_en_state>; > + > + vddaon-supply = <&vreg_s6a_0p95>; > + vddpmu-supply = <&vreg_s2f_0p95>; > + vddrfa1-supply = <&vreg_s2f_0p95>; > + vddrfa2-supply = <&vreg_s8c_1p3>; > + vddrfa3-supply = <&vreg_s5a_1p9>; > + vddpcie1-supply = <&vreg_s8c_1p3>; > + vddpcie2-supply = <&vreg_s5a_1p9>; > + vddio-supply = <&vreg_s4a_1p8>; So, after studying the datasheet for this. The names of the pins seems to come from the existing binding(s). As you're introducing a new binding (and driver) for qcom,qca6390-pmu, please use the pad names from qca6390. > + > + wlan-enable-gpios = <&tlmm 20 GPIO_ACTIVE_HIGH>; > + bt-enable-gpios = <&tlmm 21 GPIO_ACTIVE_HIGH>; > + > + regulators { > + vreg_pmu_rfa_cmn: ldo0 { > + regulator-name = "vreg_pmu_rfa_cmn"; > + regulator-min-microvolt = <760000>; > + regulator-max-microvolt = <840000>; The min/max operating range of the regulator is something we provide in the implementation, in DeviceTree you should specify the expected operating voltage. Based on the TYP value this should be regulator-min-microvolt = regulator-max-microvolt = 0.8V. > + }; > + > + vreg_pmu_aon_0p59: ldo1 { > + regulator-name = "vreg_pmu_aon_0p59"; > + regulator-min-microvolt = <540000>; > + regulator-max-microvolt = <840000>; > + }; [..] > @@ -1303,6 +1402,14 @@ sdc2_card_det_n: sd-card-det-n-state { > function = "gpio"; > bias-pull-up; > }; > + > + wlan_en_state: wlan-default-state { > + pins = "gpio20"; > + function = "gpio"; > + drive-strength = <16>; > + output-low; Please omit output-low here. > + bias-pull-up; Why do you drive it low and pull it high? bias-disable sounds more appropriate. Regards, Bjorn > + }; > }; > > &uart6 { > @@ -1311,17 +1418,16 @@ &uart6 { > bluetooth { > compatible = "qcom,qca6390-bt"; > > - pinctrl-names = "default"; > - pinctrl-0 = <&bt_en_state>; > - > - enable-gpios = <&tlmm 21 GPIO_ACTIVE_HIGH>; > - > - vddio-supply = <&vreg_s4a_1p8>; > - vddpmu-supply = <&vreg_s2f_0p95>; > - vddaon-supply = <&vreg_s6a_0p95>; > - vddrfa0p9-supply = <&vreg_s2f_0p95>; > - vddrfa1p3-supply = <&vreg_s8c_1p3>; > - vddrfa1p9-supply = <&vreg_s5a_1p9>; > + vddrfacmn-supply = <&vreg_pmu_rfa_cmn>; > + vddaon-supply = <&vreg_pmu_aon_0p59>; > + vddwlcx-supply = <&vreg_pmu_wlcx_0p8>; > + vddwlmx-supply = <&vreg_pmu_wlmx_0p85>; > + vddbtcmx-supply = <&vreg_pmu_btcmx_0p85>; > + vddrfa0-supply = <&vreg_pmu_rfa_0p8>; > + vddrfa1-supply = <&vreg_pmu_rfa_1p2>; > + vddrfa2-supply = <&vreg_pmu_rfa_1p7>; > + vddpcie0-supply = <&vreg_pmu_pcie_0p9>; > + vddpcie1-supply = <&vreg_pmu_pcie_1p8>; > }; > }; > > diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi > index 4d849e98bf9b..7cd21d4e7278 100644 > --- a/arch/arm64/boot/dts/qcom/sm8250.dtsi > +++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi > @@ -2203,6 +2203,16 @@ pcie0: pcie@1c00000 { > dma-coherent; > > status = "disabled"; > + > + pcieport0: pcie@0 { > + device_type = "pci"; > + reg = <0x0 0x0 0x0 0x0 0x0>; > + #address-cells = <3>; > + #size-cells = <2>; > + ranges; > + > + bus-range = <0x01 0xff>; > + }; > }; > > pcie0_phy: phy@1c06000 { > -- > 2.40.1 >
On 01/02/2024 16:55, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > Add a node for the PMU module of the QCA6391 present on the RB5 board. > Assign its LDO power outputs to the existing Bluetooth module. Add a > node for the PCIe port to sm8250.dtsi and define the WLAN node on it in > the board's .dts and also make it consume the power outputs of the PMU. > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > --- > arch/arm64/boot/dts/qcom/qrb5165-rb5.dts | 128 +++++++++++++++++++++-- > arch/arm64/boot/dts/qcom/sm8250.dtsi | 10 ++ > 2 files changed, 127 insertions(+), 11 deletions(-) > > diff --git a/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts b/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts > index cd0db4f31d4a..fab5bebafbad 100644 > --- a/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts > +++ b/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts > @@ -108,6 +108,87 @@ lt9611_3v3: lt9611-3v3 { > regulator-always-on; > }; > > + qca6390_pmu: pmu@0 { It does not look like you tested the DTS against bindings. Please run `make dtbs_check W=1` (see Documentation/devicetree/bindings/writing-schema.rst or https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/ for instructions). Best regards, Krzysztof
diff --git a/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts b/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts index cd0db4f31d4a..fab5bebafbad 100644 --- a/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts +++ b/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts @@ -108,6 +108,87 @@ lt9611_3v3: lt9611-3v3 { regulator-always-on; }; + qca6390_pmu: pmu@0 { + compatible = "qcom,qca6390-pmu"; + + pinctrl-names = "default"; + pinctrl-0 = <&bt_en_state>, <&wlan_en_state>; + + vddaon-supply = <&vreg_s6a_0p95>; + vddpmu-supply = <&vreg_s2f_0p95>; + vddrfa1-supply = <&vreg_s2f_0p95>; + vddrfa2-supply = <&vreg_s8c_1p3>; + vddrfa3-supply = <&vreg_s5a_1p9>; + vddpcie1-supply = <&vreg_s8c_1p3>; + vddpcie2-supply = <&vreg_s5a_1p9>; + vddio-supply = <&vreg_s4a_1p8>; + + wlan-enable-gpios = <&tlmm 20 GPIO_ACTIVE_HIGH>; + bt-enable-gpios = <&tlmm 21 GPIO_ACTIVE_HIGH>; + + regulators { + vreg_pmu_rfa_cmn: ldo0 { + regulator-name = "vreg_pmu_rfa_cmn"; + regulator-min-microvolt = <760000>; + regulator-max-microvolt = <840000>; + }; + + vreg_pmu_aon_0p59: ldo1 { + regulator-name = "vreg_pmu_aon_0p59"; + regulator-min-microvolt = <540000>; + regulator-max-microvolt = <840000>; + }; + + vreg_pmu_wlcx_0p8: ldo2 { + regulator_name = "vreg_pmu_wlcx_0p8"; + regulator-min-microvolt = <760000>; + regulator-max-microvolt = <840000>; + }; + + vreg_pmu_wlmx_0p85: ldo3 { + regulator-name = "vreg_pmu_wlmx_0p85"; + regulator-min-microvolt = <810000>; + regulator-max-microvolt = <890000>; + }; + + vreg_pmu_btcmx_0p85: ldo4 { + regulator-name = "vreg_pmu_btcmx_0p85"; + regulator-min-microvolt = <810000>; + regulator-max-microvolt = <890000>; + }; + + vreg_pmu_rfa_0p8: ldo5 { + regulator-name = "vreg_pmu_rfa_0p8"; + regulator-min-microvolt = <760000>; + regulator-max-microvolt = <840000>; + }; + + vreg_pmu_rfa_1p2: ldo6 { + regulator-name = "vreg_pmu_rfa_1p2"; + regulator-min-microvolt = <1187000>; + regulator-max-microvolt = <1313000>; + }; + + vreg_pmu_rfa_1p7: ldo7 { + regulator_name = "vreg_pmu_rfa_1p7"; + regulator-min-microvolt = <1710000>; + regulator-max-microvolt = <1890000>; + }; + + vreg_pmu_pcie_0p9: ldo8 { + regulator_name = "vreg_pmu_pcie_0p9"; + regulator-min-microvolt = <870000>; + regulator-max-microvolt = <970000>; + }; + + vreg_pmu_pcie_1p8: ldo9 { + regulator_name = "vreg_pmu_pcie_1p8"; + regulator-min-microvolt = <1710000>; + regulator-max-microvolt = <1890000>; + }; + }; + }; + thermal-zones { conn-thermal { polling-delay-passive = <0>; @@ -734,6 +815,24 @@ &pcie0_phy { vdda-pll-supply = <&vreg_l9a_1p2>; }; +&pcieport0 { + wifi@0 { + compatible = "pci17cb,1101"; + reg = <0x10000 0x0 0x0 0x0 0x0>; + + vddrfacmn-supply = <&vreg_pmu_rfa_cmn>; + vddaon-supply = <&vreg_pmu_aon_0p59>; + vddwlcx-supply = <&vreg_pmu_wlcx_0p8>; + vddwlmx-supply = <&vreg_pmu_wlmx_0p85>; + vddbtcmx-supply = <&vreg_pmu_btcmx_0p85>; + vddrfa0-supply = <&vreg_pmu_rfa_0p8>; + vddrfa1-supply = <&vreg_pmu_rfa_1p2>; + vddrfa2-supply = <&vreg_pmu_rfa_1p7>; + vddpcie0-supply = <&vreg_pmu_pcie_0p9>; + vddpcie1-supply = <&vreg_pmu_pcie_1p8>; + }; +}; + &pcie1 { status = "okay"; }; @@ -1303,6 +1402,14 @@ sdc2_card_det_n: sd-card-det-n-state { function = "gpio"; bias-pull-up; }; + + wlan_en_state: wlan-default-state { + pins = "gpio20"; + function = "gpio"; + drive-strength = <16>; + output-low; + bias-pull-up; + }; }; &uart6 { @@ -1311,17 +1418,16 @@ &uart6 { bluetooth { compatible = "qcom,qca6390-bt"; - pinctrl-names = "default"; - pinctrl-0 = <&bt_en_state>; - - enable-gpios = <&tlmm 21 GPIO_ACTIVE_HIGH>; - - vddio-supply = <&vreg_s4a_1p8>; - vddpmu-supply = <&vreg_s2f_0p95>; - vddaon-supply = <&vreg_s6a_0p95>; - vddrfa0p9-supply = <&vreg_s2f_0p95>; - vddrfa1p3-supply = <&vreg_s8c_1p3>; - vddrfa1p9-supply = <&vreg_s5a_1p9>; + vddrfacmn-supply = <&vreg_pmu_rfa_cmn>; + vddaon-supply = <&vreg_pmu_aon_0p59>; + vddwlcx-supply = <&vreg_pmu_wlcx_0p8>; + vddwlmx-supply = <&vreg_pmu_wlmx_0p85>; + vddbtcmx-supply = <&vreg_pmu_btcmx_0p85>; + vddrfa0-supply = <&vreg_pmu_rfa_0p8>; + vddrfa1-supply = <&vreg_pmu_rfa_1p2>; + vddrfa2-supply = <&vreg_pmu_rfa_1p7>; + vddpcie0-supply = <&vreg_pmu_pcie_0p9>; + vddpcie1-supply = <&vreg_pmu_pcie_1p8>; }; }; diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi index 4d849e98bf9b..7cd21d4e7278 100644 --- a/arch/arm64/boot/dts/qcom/sm8250.dtsi +++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi @@ -2203,6 +2203,16 @@ pcie0: pcie@1c00000 { dma-coherent; status = "disabled"; + + pcieport0: pcie@0 { + device_type = "pci"; + reg = <0x0 0x0 0x0 0x0 0x0>; + #address-cells = <3>; + #size-cells = <2>; + ranges; + + bus-range = <0x01 0xff>; + }; }; pcie0_phy: phy@1c06000 {