On Thu, Feb 01, 2024 at 04:55:25PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <[email protected]>
>
> 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 <[email protected]>
> ---
> 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, Feb 02, 2024 at 06:59:48AM +0200, Dmitry Baryshkov wrote:
> On Fri, 2 Feb 2024 at 06:34, Bjorn Andersson <[email protected]> wrote:
> >
> > On Thu, Feb 01, 2024 at 04:55:25PM +0100, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <[email protected]>
> > >
> > > 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 <[email protected]>
> > > ---
> > > 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 <[email protected]> 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]