Re: [PATCH v2 3/4] Bluetooth: hci_qca: Support QCA2066 on M.2 connector via pwrseq

[Bartosz Golaszewski]

On Tue, 7 Jul 2026 14:56:14 +0200, Manivannan Sadhasivam <mani@xxxxxxxxxx> said:
> On Tue, Jul 07, 2026 at 11:14:10AM +0200, Loic Poulain wrote:
>> On Mon, Jul 6, 2026 at 8:44 AM Manivannan Sadhasivam <mani@xxxxxxxxxx> wrote:
>> >
>> > On Sat, Jul 04, 2026 at 03:11:13AM +0300, Dmitry Baryshkov wrote:
>> > > On Thu, Jul 02, 2026 at 05:08:56PM +0200, Manivannan Sadhasivam wrote:
>> > > > On Thu, Jul 02, 2026 at 05:34:31PM +0300, Dmitry Baryshkov wrote:
>> > > > > On Thu, Jul 02, 2026 at 04:17:43PM +0200, Manivannan Sadhasivam wrote:
>> > > > > > On Thu, Jul 02, 2026 at 03:14:49PM +0300, Dmitry Baryshkov wrote:
>> > > > > > > On Thu, Jul 02, 2026 at 12:46:15PM +0200, Loic Poulain wrote:
>> > > > > > > > For QCA2066 (and other QCA chips) on M.2 connectors, the UART enable
>> > > > > > > > is controlled by the W_DISABLE2# signal managed by the pcie-m2 power
>> > > > > > > > sequencer rather than a dedicated BT enable GPIO.
>> > > > > > > >
>> > > > > > > > When the serdev controller has an OF graph (indicating it is connected
>> > > > > > > > to an M.2 connector), acquire the 'uart' pwrseq target from the
>> > > > > > > > connector's power sequencer and use it to control BT power instead of
>> > > > > > > > the bt-enable GPIO.
>> > > > > > > >
>> > > > > > > > Also allocate bt_power unconditionally for all SOC types since the
>> > > > > > >
>> > > > > > > Can we just fold it into the main struct?
>> > > > > > >
>> > > > > > > > pwrseq path is independent of the SOC type switch.
>> > > > > > > >
>> > > > > > > > Signed-off-by: Loic Poulain <loic.poulain@xxxxxxxxxxxxxxxx>
>> > > > > > > > ---
>> > > > > > > >  drivers/bluetooth/hci_qca.c | 81 ++++++++++++++++++++++++---------------------
>> > > > > > > >  1 file changed, 43 insertions(+), 38 deletions(-)
>> > > > > > > > @@ -2387,6 +2390,35 @@ static int qca_init_regulators(struct qca_power *qca,
>> > > > > > > >     return 0;
>> > > > > > > >  }
>> > > > > > > >
>> > > > > > > > +/*
>> > > > > > > > + * Acquire the M.2 connector power sequencer.
>> > > > > > > > + *
>> > > > > > > > + * An OF graph link on the serdev controller is only present when the BT
>> > > > > > > > + * device is attached through an M.2 Key E connector. In that case the UART
>> > > > > > > > + * enable (W_DISABLE2#) is driven by the pcie-m2 power sequencer instead of a
>> > > > > > > > + * dedicated BT enable GPIO, so grab the "uart" pwrseq target from it.
>> > > > > > > > + *
>> > > > > > > > + * Returns 0 if no M.2 connector is present (nothing to do), a negative errno
>> > > > > > > > + * on error, otherwise 0 with qcadev->bt_power->pwrseq populated.
>> > > > > > > > + */
>> > > > > > > > +static int qca_serdev_get_m2_pwrseq(struct qca_serdev *qcadev, bool *bt_en_available)
>> > > > > > > > +{
>> > > > > > > > +   struct serdev_device *serdev = qcadev->serdev_hu.serdev;
>> > > > > > > > +   struct device *dev;
>> > > > > > > > +
>> > > > > > > > +   if (!of_graph_is_present(dev_of_node(&serdev->ctrl->dev)))
>> > > > > > > > +           return 0;
>> > > > > > > > +
>> > > > > > > > +   qcadev->bt_power->pwrseq = devm_pwrseq_get(&serdev->ctrl->dev, "uart");
>> > > > > > > > +   if (IS_ERR(qcadev->bt_power->pwrseq))
>> > > > > > > > +           return PTR_ERR(qcadev->bt_power->pwrseq);
>> > > > > > > > +
>> > > > > > > > +   dev = pwrseq_to_device(qcadev->bt_power->pwrseq);
>> > > > > > > > +   *bt_en_available = device_property_present(dev, "w-disable2-gpios");
>> > > > > > >
>> > > > > > > I think here you are looking into the exact details of the other of the
>> > > > > > > graph. There might be other devices on that side, while the code now
>> > > > > > > assumes M.2. Or, consider having an M.2 controller which handles
>> > > > > > > W_DISABLE2# internally rather than through the GPIO.
>> > > > > > >
>> > > > > >
>> > > > > > This code only deals with M.2 connector in specific, so I'm not sure why we need
>> > > > > > to worry about *other* kind of devices. Let's worry about them when they show up
>> > > > > > (with graph interface ofc).
>> > > > >
>> > > > > I don't think we want to go through the drivers using M.2 connectors in
>> > > > > such a case. In the end, the contract should be that there is a power
>> > > > > sequencer on the other side of the graph, but the specifics of the
>> > > > > connector should be abstracted out. Do you know, if in the x86 world the
>> > > > > W_DISABLE2# is a GPIO or is controleed by the hub.
>> > > > >
>> > > >
>> > > > I tried to abstract out, but Bartosz didn't want pwrctrl APIs to do that level
>> > > > of abstraction as pwrctrl APIs should be generic and should not be bind to a
>> > > > specific connector and exposing its internals.
>> > > >
>> > > > That's why we ended up having pwrctrl core exposing the 'struct dev' using
>> > > > pwrseq_to_device() and letting the consumer extracting whatever information it
>> > > > needs.
>> > >
>> > > Do we have other ways to control M.2? For example on the x86 systems,
>> > > are those signals controlled via GPIOs (or GPIO-like registers) or are
>> > > they controlled separately by something like M.2 controller? Or do you
>> > > have an idea about other non-x86 systems?
>> > >
>> >
>> > Thre is no OS-visible M.2 power control in ACPI systems. ACPI defines PRx
>> > objects to control power to the PCI devices based on the D-state and the OS just
>> > evaluates the _ON/_OFF methods of the respective objects.
>> >
>> > So this API is not going to be useful on non-DT systems where the BT_EN GPIO
>> > handling is abstracted away. And also on platforms where BT_EN is not controlled
>> > by GPIOs. But I haven't seen DT platforms handling BT_EN (or W_DISABLE2#)
>> > signal in a non-GPIO way.
>> >
>> > I too prefer an API to query whether the connector supports BT_EN or not, but
>> > I'm not sure how to come up with a generic pwrseq API which also satisfies the
>> > requirement. If you have any suggestions, please let me know!
>> >
>> > FWIW, I tried adding pwrseq_is_fixed() API [1] earlier, which was turned down by
>> > Bartosz.
>>
>> So, I'll submit a new version incorporating Dmitry's request to fold
>> bt_power into the main structure.
>> However, what should we do about the powerseq point? Should we keep it as it is?
>>
>
> @Bartosz: Thoughts?
>

As discussed in private with Dmitry: I think we should go with a new interface:

  bool pwrseq_always_on(struct pwrseq_desc *pwrseq);

with the following semantics: if it returns true, the user can call
pwrseq_power_on() but a subsequent pwrseq_power_off() will fail with -ENOTSUPP.
That should give the bluetooth driver the knowledge it needs without breaking
the logical API boundaries while also being more precise than pwrseq_is_fixed().

Does it make sense?

Bartosz