Received-SPF: pass (google.com: domain of linux-wireless+bounces-2252-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240117160748.37682-1-brgl@bgdev.pl> <CAA8EJpoQfPqoMVyTmUjPs4c1Uc-p4n7zNcG+USNjXX0Svp362w@mail.gmail.com>
 <CAA8EJpqyK=pkjEofWV595tp29vjkCeWKYr-KOJh_hBiBbkVBew@mail.gmail.com>
 <CAMRc=McUZh0jhjMW7H6aVKbw29WMCQ3wdkVAz=yOZVK5wc45OA@mail.gmail.com>
 <CAA8EJprFV6SS_dGF8tOHcBG+y8j74vO0B40Y=e7Kj1-ZThNqPA@mail.gmail.com> <CAMRc=MdOALzkDtpnbqF16suShvP5apGYy4LTQ4dTc3r9Rbb1kg@mail.gmail.com>
In-Reply-To: <CAMRc=MdOALzkDtpnbqF16suShvP5apGYy4LTQ4dTc3r9Rbb1kg@mail.gmail.com>
From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Date: Fri, 19 Jan 2024 16:07:17 +0200
Message-ID: <CAA8EJpr=PMdOWzp8fahL9e9QC-qgS=hSaTqT1XdUs8Dvvsxqgg@mail.gmail.com>
Subject: Re: [PATCH 0/9] PCI: introduce the concept of power sequencing of
 PCIe devices
To: brgl@bgdev.pl
Cc: Kalle Valo <kvalo@kernel.org>, "David S . Miller" <davem@davemloft.net>, 
	Eric Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>, 
	Rob Herring <robh+dt@kernel.org>, 
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Conor Dooley <conor+dt@kernel.org>, 
	Bjorn Andersson <andersson@kernel.org>, Konrad Dybcio <konrad.dybcio@linaro.org>, 
	Catalin Marinas <catalin.marinas@arm.com>, Will Deacon <will@kernel.org>, 
	Bjorn Helgaas <bhelgaas@google.com>, Heiko Stuebner <heiko@sntech.de>, 
	Jernej Skrabec <jernej.skrabec@gmail.com>, Chris Morgan <macromorgan@hotmail.com>, 
	Linus Walleij <linus.walleij@linaro.org>, Geert Uytterhoeven <geert+renesas@glider.be>, 
	Arnd Bergmann <arnd@arndb.de>, Neil Armstrong <neil.armstrong@linaro.org>, 
	=?UTF-8?B?TsOtY29sYXMgRiAuIFIgLiBBIC4gUHJhZG8=?= <nfraprado@collabora.com>, 
	Marek Szyprowski <m.szyprowski@samsung.com>, Peng Fan <peng.fan@nxp.com>, 
	Robert Richter <rrichter@amd.com>, Dan Williams <dan.j.williams@intel.com>, 
	Jonathan Cameron <Jonathan.Cameron@huawei.com>, Terry Bowman <terry.bowman@amd.com>, 
	Lukas Wunner <lukas@wunner.de>, Huacai Chen <chenhuacai@kernel.org>, Alex Elder <elder@linaro.org>, 
	Srini Kandagatla <srinivas.kandagatla@linaro.org>, 
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Abel Vesa <abel.vesa@linaro.org>, 
	linux-wireless@vger.kernel.org, netdev@vger.kernel.org, 
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, 
	linux-arm-msm@vger.kernel.org, linux-arm-kernel@lists.infradead.org, 
	linux-pci@vger.kernel.org, 
	Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Fri, 19 Jan 2024 at 15:35, <brgl@bgdev.pl> wrote:
>
> On Fri, 19 Jan 2024 13:31:53 +0100, Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> said:
> > On Fri, 19 Jan 2024 at 13:52, Bartosz Golaszewski <brgl@bgdev.pl> wrote=
:
> >>
> >> On Thu, Jan 18, 2024 at 7:53=E2=80=AFPM Dmitry Baryshkov
> >> <dmitry.baryshkov@linaro.org> wrote:
> >> >
> >>
> >> [snip]
> >>
> >> > >
> >> > > I'd still like to see how this can be extended to handle BT power =
up,
> >> > > having a single entity driving both of the BT and WiFI.
> >> > >
> >> > > The device tree changes behave in exactly the opposite way: they
> >> > > define regulators for the WiFi device, while the WiFi is not being
> >> > > powered by these regulators. Both WiFi and BT are powered by the P=
MU,
> >> > > which in turn consumes all specified regulators.
> >> >
> >> > Some additional justification, why I think that this should be
> >> > modelled as a single instance instead of two different items.
> >> >
> >> > This is from msm-5.10 kernel:
> >> >
> >> >
> >> > =3D=3D=3D=3D=3D CUT HERE =3D=3D=3D=3D=3D
> >> > /**
> >> >  * cnss_select_pinctrl_enable - select WLAN_GPIO for Active pinctrl =
status
> >> >  * @plat_priv: Platform private data structure pointer
> >> >  *
> >> >  * For QCA6490, PMU requires minimum 100ms delay between BT_EN_GPIO =
off and
> >> >  * WLAN_EN_GPIO on. This is done to avoid power up issues.
> >> >  *
> >> >  * Return: Status of pinctrl select operation. 0 - Success.
> >> >  */
> >> > static int cnss_select_pinctrl_enable(struct cnss_plat_data *plat_pr=
iv)
> >> > =3D=3D=3D=3D=3D CUT HERE =3D=3D=3D=3D=3D
> >> >
> >> >
> >> > Also see the bt_configure_gpios() function in the same kernel.
> >> >
> >>
> >> You are talking about a different problem. Unfortunately we're using
> >> similar naming here but I don't have a better alternative in mind.
> >>
> >> We have two separate issues: one is powering-up a PCI device so that
> >> it can be detected and the second is dealing with a device that has
> >> multiple modules in it which share a power sequence. The two are
> >> independent and this series isn't trying to solve the latter.
> >
> > I see it from a different angle: a power up of the WiFi+BT chips. This
> > includes devices like wcn3990 (which have platform + serial parts) and
> > qca6390 / qca6490 / wcn6750 / etc. (which have PCI and serial parts).
> >
> > From my point of view, the PCIe-only part was nice for an RFC, but for
> > v1 I have expected to see a final solution that we can reuse for
> > wcn3990.
> >
>
> The submodules are represented as independent devices on the DT and I don=
't
> think this will change. It's not even possible as they operate on differe=
nt
> buses so it's not like we can MFD it with a top-level platform device and=
 two
> sub-nodes of which one is PCI and another serdev. With that in mind, I'm
> insisting that there are two separate issues and a generic power sequenci=
ng
> can be built on top of the PCI-specific pwrseq added here.
>
> >>
> >> But I am aware of this and so I actually have an idea for a
> >> generalized power sequencing framework. Let's call it pwrseq as
> >> opposed to pci_pwrseq.
> >>
> >> Krzysztof is telling me that there cannot be any power sequencing
> >> information contained in DT. Also: modelling the PMU in DT would just
> >> over complicate stuff for now reason. We'd end up having the PMU node
> >> consuming the regulators but it too would need to expose regulators
> >> for WLAN and BT or be otherwise referenced by their nodes.
> >
> > Yes. And it is a correct representation of the device. The WiFi and BT
> > parts are powered up by the outputs from PMU. We happen to have three
> > different pieces (WiFi, BT and PMU) squashed on a single physical
> > device.
> >
>
> Alright, so let's imagine we do model the PMU on the device tree. It woul=
d
> look something like this:
>
> qca6390_pmu: pmic@0 {
>         compatible =3D "qcom,qca6390-pmu";
>
>         bt-gpios =3D <...>;
>         wlan-gpios =3D <...>;
>
>         vdd-supply =3D <&vreg...>;
>         ...
>
>         regulators-0 {
>                 vreg_x: foo {
>                         ...
>                 };
>
>                 ...
>         };
> };
>
> Then the WLAN and BT consume the regulators from &qca6390_pmu. Obviously =
we
> cannot go:
>
> wlan {
>         pwrseq =3D &qca6390_pmu;
> };
>
> But it's enough to:
>
> wlan {
>         vdd-supply =3D <&vreg_x>;
> };

I'm not sure this will fly. This means expecting that regulator
framework is reentrant, which I think is not the case.

> But the pwrseq driver for "qcom,qca6390-pmu" could map BT and WLAN compat=
ibles
> to the correct power sequence and then the relevant drivers could enable =
it
> using pwrseq_power_on().
>
> But that comes back to what I'm doing here: the PCI part for ath11k still
> needs the platform driver that will trigger the power sequence and that c=
ould
> be the PCI pwrseq driver for which the framework is introduced in this se=
ries.
>
> As I said: the two are largely orthogonal.

I'm fine with that as long as it stays as an RFC. We need to fix both
issues before committing qca6390 power up support.

>
> >>
> >> So I'm thinking that the DT representation should remain as it is:
> >> with separate WLAN and BT nodes consuming resources relevant to their
> >> functionality (BT does not need to enable PCIe regulators).
> >
> > Is it so? The QCA6390 docs clearly say that all regulators should be
> > enabled before asserting BT_EN / WLAN_EN. See the powerup timing
> > diagram and the t2 note to that diagram.
> >
>
> Fair enough.
>
> >> Now how to
> >> handle the QCA6490 model you brought up? How about pwrseq drivers that
> >> would handle the sequence based on compatibles?
> >
> > The QCA6490 is also known as WCN6855. So this problem applies to
> > Qualcomm sm8350 / sm8450 platforms.
> >
> > And strictly speaking I don't see any significant difference between
> > QCA6390 and WCN6855. The regulators might be different, but the
> > implementation should be the same.
> >
> >>
> >> We'd add a new subsystem at drivers/pwrseq/. Inside there would be:
> >> drivers/pwrseq/pwrseq-qca6490.c. The pwrseq framework would expose an
> >> API to "sub-drivers" (in this case: BT serdev driver and the qca6490
> >> power sequencing driver). Now the latter goes:
> >>
> >> struct pwrseq_desc *pwrseq =3D pwrseq_get(dev);
> >>
> >> And the pwrseq subsystem matches the device's compatible against the
> >> correct, *shared* sequence. The BT driver can do the same at any time.
> >> The pwrseq driver then gets regulators, GPIOs, clocks etc. and will be
> >> responsible for dealing with them.
> >>
> >> In sub-drivers we now do:
> >>
> >> ret =3D pwrseq_power_on(pwrseq);
> >>
> >> or
> >>
> >> ret =3D pwrseq_power_off(pwrseq);
> >>
> >> in the sub-device drivers and no longer interact with each regulator
> >> on our own. The pwrseq subsystem is now in charge of adding delays
> >> etc.
> >>
> >> That's only an idea and I haven't done any real work yet but I'm
> >> throwing it out there for discussion.
> >
> > I've been there and I had implemented it in the same way, but rather
> > having the pwrseq as a primary device in DT and parsing end-devices
> > only as a fallback / compatibility case.
> >
>
> Would you mind posting an example DT code here? I'm not sure if I underst=
and
> what "primary device" means in this context.

 qca_pwrseq: qca-pwrseq {
  compatible =3D "qcom,qca6390-pwrseq";

  #pwrseq-cells =3D <1>;

  vddaon-supply =3D <&vreg_s6a_0p95>;
  vddpmu-supply =3D <&vreg_s2f_0p95>;
  vddrfa1-supply =3D <&vreg_s2f_0p95>;
  vddrfa2-supply =3D <&vreg_s8c_1p3>;
  vddrfa3-supply =3D <&vreg_s5a_1p9>;
  vddpcie1-supply =3D <&vreg_s8c_1p3>;
  vddpcie2-supply =3D <&vreg_s5a_1p9>;
  vddio-supply =3D <&vreg_s4a_1p8>;

  bt-enable-gpios =3D <&tlmm 21 GPIO_ACTIVE_HIGH>;
  wifi-enable-gpios =3D <&tlmm 20 GPIO_ACTIVE_HIGH>;
  swctrl-gpios =3D <&tlmm 124 GPIO_ACTIVE_HIGH>;
 };

&uart6 {
 status =3D "okay";
 bluetooth {
  compatible =3D "qcom,qca6390-bt";
  clocks =3D <&sleep_clk>;

  bt-pwrseq =3D <&qca_pwrseq 1>;
 };
};

See https://lore.kernel.org/linux-arm-msm/20211006035407.1147909-13-dmitry.=
baryshkov@linaro.org/

--=20
With best wishes
Dmitry