On Fri, 19 Jan 2024 at 15:35, <brgl@xxxxxxxx> wrote:
>
> 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>;
> };
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@xxxxxxxxxx/
--
With best wishes
Dmitry
