Re: [PATCH 0/9] PCI: introduce the concept of power sequencing of PCIe devices

[brgl]

On Fri, 19 Jan 2024 13:31:53 +0100, Dmitry Baryshkov
<dmitry.baryshkov@xxxxxxxxxx> said:
> On Fri, 19 Jan 2024 at 13:52, Bartosz Golaszewski <brgl@xxxxxxxx> wrote:
>>
>> On Thu, Jan 18, 2024 at 7:53 PM Dmitry Baryshkov
>> <dmitry.baryshkov@xxxxxxxxxx> 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>;
};

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.

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

Bartosz

>
>
> --
> With best wishes
> Dmitry
>