Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-1582-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Fri, 2 Feb 2024 10:41:46 -0600
From: Bjorn Andersson <andersson@kernel.org>
To: Bartosz Golaszewski <brgl@bgdev.pl>
Cc: Konrad Dybcio <konrad.dybcio@linaro.org>, 
	Rob Herring <robh@kernel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, 
	Conor Dooley <conor+dt@kernel.org>, Marcel Holtmann <marcel@holtmann.org>, 
	Luiz Augusto von Dentz <luiz.dentz@gmail.com>, Bjorn Helgaas <bhelgaas@google.com>, 
	Neil Armstrong <neil.armstrong@linaro.org>, Alex Elder <elder@linaro.org>, 
	Srini Kandagatla <srinivas.kandagatla@linaro.org>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, 
	Arnd Bergmann <arnd@arndb.de>, Abel Vesa <abel.vesa@linaro.org>, 
	Manivannan Sadhasivam <mani@kernel.org>, Lukas Wunner <lukas@wunner.de>, linux-arm-msm@vger.kernel.org, 
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-bluetooth@vger.kernel.org, 
	linux-pci@vger.kernel.org, Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Subject: Re: Re: [RFC 2/9] arm64: dts: qcom: qrb5165-rb5: model the PMU of
 the QCA6391
Message-ID: <iifyazcucnghjpnmzq4qtyj6o25upntmmst6abyugpom4jrs3p@mly5h7s7owl3>
References: <20240201155532.49707-1-brgl@bgdev.pl>
 <20240201155532.49707-3-brgl@bgdev.pl>
 <5lirm5mnf7yqbripue5nyqu6ej54sx4rtmgmyqjrqanabsriyp@2pjiv5xbmxpk>
 <CAMRc=Mcq8a7T06DaX9nirfHOXPs+Bh51rKgO3FksxKH+Hph2FA@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAMRc=Mcq8a7T06DaX9nirfHOXPs+Bh51rKgO3FksxKH+Hph2FA@mail.gmail.com>

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]