Received-SPF: pass (google.com: domain of linux-wireless+bounces-2248-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
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>
In-Reply-To: <CAA8EJpqyK=pkjEofWV595tp29vjkCeWKYr-KOJh_hBiBbkVBew@mail.gmail.com>
From: Bartosz Golaszewski <brgl@bgdev.pl>
Date: Fri, 19 Jan 2024 12:52:00 +0100
Message-ID: <CAMRc=McUZh0jhjMW7H6aVKbw29WMCQ3wdkVAz=yOZVK5wc45OA@mail.gmail.com>
Subject: Re: [PATCH 0/9] PCI: introduce the concept of power sequencing of
 PCIe devices
To: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
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 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 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:
>
>
> =3D=3D=3D=3D=3D CUT HERE =3D=3D=3D=3D=3D
> /**
>  * cnss_select_pinctrl_enable - select WLAN_GPIO for Active pinctrl statu=
s
>  * @plat_priv: Platform private data structure pointer
>  *
>  * For QCA6490, PMU requires minimum 100ms delay between BT_EN_GPIO off a=
nd
>  * 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)
> =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.

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.

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). Now how to
handle the QCA6490 model you brought up? How about pwrseq drivers that
would handle the sequence based on compatibles?

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.

Bartosz

[snip]