Message-ID: <CAA8EJpr=PMdOWzp8fahL9e9QC-qgS=hSaTqT1XdUs8Dvvsxqgg@mail.gmail.com>
Date: Fri, 19 Jan 2024 16:07:17 +0200
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: brgl@...ev.pl
Cc: Kalle Valo <kvalo@...nel.org>, "David S . Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	Rob Herring <robh+dt@...nel.org>, 
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>, Conor Dooley <conor+dt@...nel.org>, 
	Bjorn Andersson <andersson@...nel.org>, Konrad Dybcio <konrad.dybcio@...aro.org>, 
	Catalin Marinas <catalin.marinas@....com>, Will Deacon <will@...nel.org>, 
	Bjorn Helgaas <bhelgaas@...gle.com>, Heiko Stuebner <heiko@...ech.de>, 
	Jernej Skrabec <jernej.skrabec@...il.com>, Chris Morgan <macromorgan@...mail.com>, 
	Linus Walleij <linus.walleij@...aro.org>, Geert Uytterhoeven <geert+renesas@...der.be>, 
	Arnd Bergmann <arnd@...db.de>, Neil Armstrong <neil.armstrong@...aro.org>, 
	Nícolas F . R . A . Prado <nfraprado@...labora.com>, 
	Marek Szyprowski <m.szyprowski@...sung.com>, Peng Fan <peng.fan@....com>, 
	Robert Richter <rrichter@....com>, Dan Williams <dan.j.williams@...el.com>, 
	Jonathan Cameron <Jonathan.Cameron@...wei.com>, Terry Bowman <terry.bowman@....com>, 
	Lukas Wunner <lukas@...ner.de>, Huacai Chen <chenhuacai@...nel.org>, Alex Elder <elder@...aro.org>, 
	Srini Kandagatla <srinivas.kandagatla@...aro.org>, 
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Abel Vesa <abel.vesa@...aro.org>, 
	linux-wireless@...r.kernel.org, netdev@...r.kernel.org, 
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-arm-msm@...r.kernel.org, linux-arm-kernel@...ts.infradead.org, 
	linux-pci@...r.kernel.org, 
	Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
Subject: Re: [PATCH 0/9] PCI: introduce the concept of power sequencing of
 PCIe devices

On Fri, 19 Jan 2024 at 15:35, <brgl@...ev.pl> wrote:
>
> On Fri, 19 Jan 2024 13:31:53 +0100, Dmitry Baryshkov
> <dmitry.baryshkov@...aro.org> said:
> > On Fri, 19 Jan 2024 at 13:52, Bartosz Golaszewski <brgl@...ev.pl> wrote:
> >>
> >> On Thu, Jan 18, 2024 at 7:53 PM Dmitry Baryshkov
> >> <dmitry.baryshkov@...aro.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 PMU,
> >> > > 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:
> >> >
> >> >
> >> > ===== CUT HERE =====
> >> > /**
> >> >  * 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_priv)
> >> > ===== CUT HERE =====
> >> >
> >> >
> >> > 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 different
> 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 sequencing
> 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 would
> look something like this:
>
> qca6390_pmu: pmic@0 {
>         compatible = "qcom,qca6390-pmu";
>
>         bt-gpios = <...>;
>         wlan-gpios = <...>;
>
>         vdd-supply = <&vreg...>;
>         ...
>
>         regulators-0 {
>                 vreg_x: foo {
>                         ...
>                 };
>
>                 ...
>         };
> };
>
> Then the WLAN and BT consume the regulators from &qca6390_pmu. Obviously we
> cannot go:
>
> wlan {
>         pwrseq = &qca6390_pmu;
> };
>
> But it's enough to:
>
> wlan {
>         vdd-supply = <&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 compatibles
> 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 could
> be the PCI pwrseq driver for which the framework is introduced in this series.
>
> 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 = 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 = pwrseq_power_on(pwrseq);
> >>
> >> or
> >>
> >> ret = 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 understand
> what "primary device" means in this context.

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

  #pwrseq-cells = <1>;

  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>;

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

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

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

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

-- 
With best wishes
Dmitry

Powered by blists - more mailing lists