diff mbox series

Bluetooth: qca: don't disable power management for QCA6390

Message ID 20240624194518.37458-1-brgl@bgdev.pl (mailing list archive)
State Accepted
Commit 561cb4e9a2384c4b327976aee53acd5bfccbcb4b
Headers show
Series Bluetooth: qca: don't disable power management for QCA6390 | expand

Checks

Context Check Description
tedd_an/pre-ci_am success Success
tedd_an/CheckPatch warning WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report #102: Reported-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> Fixes: 4029dba6b6f1 ("Bluetooth: qca: use the power sequencer for QCA6390") total: 0 errors, 1 warnings, 14 lines checked NOTE: For some of the reported defects, checkpatch may be able to mechanically convert to the typical style using --fix or --fix-inplace. /github/workspace/src/src/13710012.patch has style problems, please review. NOTE: Ignored message types: UNKNOWN_COMMIT_ID NOTE: If any of the errors are false positives, please report them to the maintainer, see CHECKPATCH in MAINTAINERS.
tedd_an/GitLint success Gitlint PASS
tedd_an/SubjectPrefix success Gitlint PASS
tedd_an/BuildKernel success BuildKernel PASS
tedd_an/CheckAllWarning success CheckAllWarning PASS
tedd_an/CheckSparse success CheckSparse PASS
tedd_an/CheckSmatch success CheckSparse PASS
tedd_an/BuildKernel32 success BuildKernel32 PASS
tedd_an/TestRunnerSetup success TestRunnerSetup PASS

Commit Message

Bartosz Golaszewski June 24, 2024, 7:45 p.m. UTC 
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

We unnecessarily fallthrough the case for QCA6390 when initializing the
device and hit the condition where - due to the lack of the enable-gpio
- we disable power management despite using the power sequencer. We don't
need to look for clocks on this model so it makes more sense to just
register the hci device and break the switch.

Reported-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Fixes: 4029dba6b6f1 ("Bluetooth: qca: use the power sequencer for QCA6390")
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/bluetooth/hci_qca.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Comments

Dmitry Baryshkov June 24, 2024, 8:17 p.m. UTC | #1 
On Mon, 24 Jun 2024 at 22:45, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> We unnecessarily fallthrough the case for QCA6390 when initializing the
> device and hit the condition where - due to the lack of the enable-gpio
> - we disable power management despite using the power sequencer. We don't
> need to look for clocks on this model so it makes more sense to just
> register the hci device and break the switch.
>
> Reported-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Fixes: 4029dba6b6f1 ("Bluetooth: qca: use the power sequencer for QCA6390")
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Is this going to break the QCA6390 as present on M.2 / PCIe cards? Or
the older DT which didn't have pwrseq entries?

> ---
>  drivers/bluetooth/hci_qca.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index 2b31f3dc33a9..bc6a49ba26f9 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -2402,7 +2402,13 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>                                                            "bluetooth");
>                 if (IS_ERR(qcadev->bt_power->pwrseq))
>                         return PTR_ERR(qcadev->bt_power->pwrseq);
> -               fallthrough;
> +
> +               err = hci_uart_register_device(&qcadev->serdev_hu, &qca_proto);
> +               if (err) {
> +                       BT_ERR("qca6390 serdev registration failed");
> +                       return err;
> +               }
> +               break;
>
>         default:
>                 qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable",
> --
> 2.43.0
>
Bartosz Golaszewski June 24, 2024, 8:46 p.m. UTC | #2 
On Mon, Jun 24, 2024 at 10:17 PM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Mon, 24 Jun 2024 at 22:45, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > We unnecessarily fallthrough the case for QCA6390 when initializing the
> > device and hit the condition where - due to the lack of the enable-gpio
> > - we disable power management despite using the power sequencer. We don't
> > need to look for clocks on this model so it makes more sense to just
> > register the hci device and break the switch.
> >
> > Reported-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > Fixes: 4029dba6b6f1 ("Bluetooth: qca: use the power sequencer for QCA6390")
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> Is this going to break the QCA6390 as present on M.2 / PCIe cards? Or
> the older DT which didn't have pwrseq entries?
>

Neither of these has clocks that need to be driven by linux. The only
user of QCA6390 Bluetooth in mainline is RB5. Bindings didn't exist
before so no commitment was ever made.

Bart
Dmitry Baryshkov June 24, 2024, 9:19 p.m. UTC | #3 
On Mon, 24 Jun 2024 at 23:47, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> On Mon, Jun 24, 2024 at 10:17 PM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
> >
> > On Mon, 24 Jun 2024 at 22:45, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > >
> > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > >
> > > We unnecessarily fallthrough the case for QCA6390 when initializing the
> > > device and hit the condition where - due to the lack of the enable-gpio
> > > - we disable power management despite using the power sequencer. We don't
> > > need to look for clocks on this model so it makes more sense to just
> > > register the hci device and break the switch.
> > >
> > > Reported-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > Fixes: 4029dba6b6f1 ("Bluetooth: qca: use the power sequencer for QCA6390")
> > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > Is this going to break the QCA6390 as present on M.2 / PCIe cards? Or
> > the older DT which didn't have pwrseq entries?
> >
>
> Neither of these has clocks that need to be driven by linux. The only
> user of QCA6390 Bluetooth in mainline is RB5. Bindings didn't exist
> before so no commitment was ever made.

This might make some laptop users unhappy. But anyway, restarting the
hci0 now gives:

[   24.387344] Bluetooth: hci0: setting up ROME/QCA6390
[   24.387439] qcom_geni_serial 998000.serial: serial engine reports 0
RX bytes in!
[   24.554349] qcom_geni_serial 998000.serial: serial engine reports 0
RX bytes in!
[   24.562056] arm-smmu 15000000.iommu: Unhandled context fault:
fsr=0x402, iova=0xfffd1080, fsynr=0x750013, cbfrsynra=0x5a3, cb=3
[   26.914225] Bluetooth: hci0: command 0xfc00 tx timeout
[   35.042619] Bluetooth: hci0: Reading QCA version information failed (-110)
[   35.049721] Bluetooth: hci0: Retry BT power ON:0
[   37.539492] Bluetooth: hci0: command 0xfc00 tx timeout
[   45.539519] Bluetooth: hci0: Reading QCA version information failed (-110)
[   45.546667] Bluetooth: hci0: Retry BT power ON:1
[   48.035863] Bluetooth: hci0: command 0xfc00 tx timeout
[   56.034783] Bluetooth: hci0: Reading QCA version information failed (-110)
[   56.041901] Bluetooth: hci0: Retry BT power ON:2
[   58.532174] Bluetooth: hci0: command 0xfc00 tx timeout
[   66.531928] Bluetooth: hci0: Reading QCA version information failed (-110)




--
With best wishes
Dmitry
Bartosz Golaszewski June 25, 2024, 6:50 a.m. UTC | #4 
On Mon, 24 Jun 2024 23:19:55 +0200, Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> said:
> On Mon, 24 Jun 2024 at 23:47, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>>
>> On Mon, Jun 24, 2024 at 10:17 PM Dmitry Baryshkov
>> <dmitry.baryshkov@linaro.org> wrote:
>> >
>> > On Mon, 24 Jun 2024 at 22:45, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>> > >
>> > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>> > >
>> > > We unnecessarily fallthrough the case for QCA6390 when initializing the
>> > > device and hit the condition where - due to the lack of the enable-gpio
>> > > - we disable power management despite using the power sequencer. We don't
>> > > need to look for clocks on this model so it makes more sense to just
>> > > register the hci device and break the switch.
>> > >
>> > > Reported-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> > > Fixes: 4029dba6b6f1 ("Bluetooth: qca: use the power sequencer for QCA6390")
>> > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>> >
>> > Is this going to break the QCA6390 as present on M.2 / PCIe cards? Or
>> > the older DT which didn't have pwrseq entries?
>> >
>>
>> Neither of these has clocks that need to be driven by linux. The only
>> user of QCA6390 Bluetooth in mainline is RB5. Bindings didn't exist
>> before so no commitment was ever made.
>
> This might make some laptop users unhappy. But anyway, restarting the
> hci0 now gives:
>
> [   24.387344] Bluetooth: hci0: setting up ROME/QCA6390
> [   24.387439] qcom_geni_serial 998000.serial: serial engine reports 0
> RX bytes in!
> [   24.554349] qcom_geni_serial 998000.serial: serial engine reports 0
> RX bytes in!
> [   24.562056] arm-smmu 15000000.iommu: Unhandled context fault:
> fsr=0x402, iova=0xfffd1080, fsynr=0x750013, cbfrsynra=0x5a3, cb=3
> [   26.914225] Bluetooth: hci0: command 0xfc00 tx timeout
> [   35.042619] Bluetooth: hci0: Reading QCA version information failed (-110)
> [   35.049721] Bluetooth: hci0: Retry BT power ON:0
> [   37.539492] Bluetooth: hci0: command 0xfc00 tx timeout
> [   45.539519] Bluetooth: hci0: Reading QCA version information failed (-110)
> [   45.546667] Bluetooth: hci0: Retry BT power ON:1
> [   48.035863] Bluetooth: hci0: command 0xfc00 tx timeout
> [   56.034783] Bluetooth: hci0: Reading QCA version information failed (-110)
> [   56.041901] Bluetooth: hci0: Retry BT power ON:2
> [   58.532174] Bluetooth: hci0: command 0xfc00 tx timeout
> [   66.531928] Bluetooth: hci0: Reading QCA version information failed (-110)
>
>

How do you reproduce it because it works fine for me:

root@qcom-armv8a:~# btmgmt --index 0 public-addr 00:11:22:33:44:55
[  429.096397] Bluetooth: MGMT ver 1.23
hci0 Set Public Address complete, options:
[  429.102024] Bluetooth: hci0: setting up ROME/QCA6390
root@qcom-armv8a:~# [  429.184052] Bluetooth: Received
HCI_IBS_WAKE_ACK in tx state 0
[  429.497264] Bluetooth: hci0: QCA Product ID   :0x00000010
[  429.502854] Bluetooth: hci0: QCA SOC Version  :0x400a0200
[  429.508412] Bluetooth: hci0: QCA ROM Version  :0x00000200
[  429.513974] Bluetooth: hci0: QCA Patch Version:0x00000d2b
[  429.519533] Bluetooth: hci0: QCA controller version 0x02000200
[  429.525534] Bluetooth: hci0: QCA Downloading qca/htbtfw20.tlv
[  430.449793] Bluetooth: hci0: QCA Downloading qca/htnv20.bin

root@qcom-armv8a:~# [  430.689052] Bluetooth: hci0: QCA setup on UART
is completed
[  430.752854] NET: Registered PF_ALG protocol family

root@qcom-armv8a:~#
root@qcom-armv8a:~# hciconfig hci0 up
[  437.505116] Bluetooth: hci0: setting up ROME/QCA6390
[  437.586969] Bluetooth: Received HCI_IBS_WAKE_ACK in tx state 0
[  437.912683] Bluetooth: hci0: QCA Product ID   :0x00000010
[  437.918286] Bluetooth: hci0: QCA SOC Version  :0x400a0200
[  437.923890] Bluetooth: hci0: QCA ROM Version  :0x00000200
[  437.929613] Bluetooth: hci0: QCA Patch Version:0x00000d2b
[  437.935184] Bluetooth: hci0: QCA controller version 0x02000200
[  437.941207] Bluetooth: hci0: QCA Downloading qca/htbtfw20.tlv
[  438.918868] Bluetooth: hci0: QCA Downloading qca/htnv20.bin
[  439.158671] Bluetooth: hci0: QCA setup on UART is completed
root@qcom-armv8a:~#
root@qcom-armv8a:~#
root@qcom-armv8a:~# hciconfig hci0 down
root@qcom-armv8a:~#
root@qcom-armv8a:~# cat /sys/kernel/debug/pwrseq
pwrseq.0:
  targets:
    target: [bluetooth] (target unit: [bluetooth-enable])
    target: [wlan] (target unit: [wlan-enable])
  units:
    unit: [regulators-enable] - enable count: 1
    unit: [clock-enable] - enable count: 1
    unit: [bluetooth-enable] - enable count: 0
      dependencies:
        [regulators-enable]
        [clock-enable]
    unit: [wlan-enable] - enable count: 1
      dependencies:
        [regulators-enable]
        [clock-enable]
root@qcom-armv8a:~#
root@qcom-armv8a:~# hciconfig hci0 up
[  454.407376] Bluetooth: hci0: setting up ROME/QCA6390
[  454.485238] Bluetooth: Received HCI_IBS_WAKE_ACK in tx state 0
[  454.813289] Bluetooth: hci0: QCA Product ID   :0x00000010
[  454.818892] Bluetooth: hci0: QCA SOC Version  :0x400a0200
[  454.824461] Bluetooth: hci0: QCA ROM Version  :0x00000200
[  454.830035] Bluetooth: hci0: QCA Patch Version:0x00000d2b
[  454.835599] Bluetooth: hci0: QCA controller version 0x02000200
[  454.841651] Bluetooth: hci0: QCA Downloading qca/htbtfw20.tlv
[  455.773967] Bluetooth: hci0: QCA Downloading qca/htnv20.bin
[  456.011213] Bluetooth: hci0: QCA setup on UART is completed

Bart
Dmitry Baryshkov June 25, 2024, 6:57 a.m. UTC | #5 
On Tue, 25 Jun 2024 at 09:50, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> On Mon, 24 Jun 2024 23:19:55 +0200, Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> said:
> > On Mon, 24 Jun 2024 at 23:47, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >>
> >> On Mon, Jun 24, 2024 at 10:17 PM Dmitry Baryshkov
> >> <dmitry.baryshkov@linaro.org> wrote:
> >> >
> >> > On Mon, 24 Jun 2024 at 22:45, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >> > >
> >> > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >> > >
> >> > > We unnecessarily fallthrough the case for QCA6390 when initializing the
> >> > > device and hit the condition where - due to the lack of the enable-gpio
> >> > > - we disable power management despite using the power sequencer. We don't
> >> > > need to look for clocks on this model so it makes more sense to just
> >> > > register the hci device and break the switch.
> >> > >
> >> > > Reported-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >> > > Fixes: 4029dba6b6f1 ("Bluetooth: qca: use the power sequencer for QCA6390")
> >> > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >> >
> >> > Is this going to break the QCA6390 as present on M.2 / PCIe cards? Or
> >> > the older DT which didn't have pwrseq entries?
> >> >
> >>
> >> Neither of these has clocks that need to be driven by linux. The only
> >> user of QCA6390 Bluetooth in mainline is RB5. Bindings didn't exist
> >> before so no commitment was ever made.
> >
> > This might make some laptop users unhappy. But anyway, restarting the
> > hci0 now gives:
> >
> > [   24.387344] Bluetooth: hci0: setting up ROME/QCA6390
> > [   24.387439] qcom_geni_serial 998000.serial: serial engine reports 0
> > RX bytes in!
> > [   24.554349] qcom_geni_serial 998000.serial: serial engine reports 0
> > RX bytes in!
> > [   24.562056] arm-smmu 15000000.iommu: Unhandled context fault:
> > fsr=0x402, iova=0xfffd1080, fsynr=0x750013, cbfrsynra=0x5a3, cb=3
> > [   26.914225] Bluetooth: hci0: command 0xfc00 tx timeout
> > [   35.042619] Bluetooth: hci0: Reading QCA version information failed (-110)
> > [   35.049721] Bluetooth: hci0: Retry BT power ON:0
> > [   37.539492] Bluetooth: hci0: command 0xfc00 tx timeout
> > [   45.539519] Bluetooth: hci0: Reading QCA version information failed (-110)
> > [   45.546667] Bluetooth: hci0: Retry BT power ON:1
> > [   48.035863] Bluetooth: hci0: command 0xfc00 tx timeout
> > [   56.034783] Bluetooth: hci0: Reading QCA version information failed (-110)
> > [   56.041901] Bluetooth: hci0: Retry BT power ON:2
> > [   58.532174] Bluetooth: hci0: command 0xfc00 tx timeout
> > [   66.531928] Bluetooth: hci0: Reading QCA version information failed (-110)
> >
> >
>
> How do you reproduce it because it works fine for me:

Hmm, most likely I had a dirty kernel version somewhere. With the
current linux-next it works for me too.

Acked-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Tested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> # RB5

Thank you!
Bartosz Golaszewski June 25, 2024, 7:46 a.m. UTC | #6 
On Mon, Jun 24, 2024 at 11:20 PM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> >
> > Neither of these has clocks that need to be driven by linux. The only
> > user of QCA6390 Bluetooth in mainline is RB5. Bindings didn't exist
> > before so no commitment was ever made.
>
> This might make some laptop users unhappy.

Like I said: without upstreamed DT bindings, we have never made any
commitment about the device properties. I doubt anyone will complain
though, I haven't seen any DT with QCA6390 with clock properties yet.
I wouldn't stress it for now.

Bart
Dmitry Baryshkov June 25, 2024, 7:47 a.m. UTC | #7 
On Tue, 25 Jun 2024 at 10:46, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> On Mon, Jun 24, 2024 at 11:20 PM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
> >
> > >
> > > Neither of these has clocks that need to be driven by linux. The only
> > > user of QCA6390 Bluetooth in mainline is RB5. Bindings didn't exist
> > > before so no commitment was ever made.
> >
> > This might make some laptop users unhappy.
>
> Like I said: without upstreamed DT bindings, we have never made any
> commitment about the device properties. I doubt anyone will complain
> though, I haven't seen any DT with QCA6390 with clock properties yet.
> I wouldn't stress it for now.

I was thinking about x86 laptops / M.2 cards. I'll see if I can locate one.
Bartosz Golaszewski June 25, 2024, 8:50 a.m. UTC | #8 
On Tue, Jun 25, 2024 at 9:47 AM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Tue, 25 Jun 2024 at 10:46, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >
> > On Mon, Jun 24, 2024 at 11:20 PM Dmitry Baryshkov
> > <dmitry.baryshkov@linaro.org> wrote:
> > >
> > > >
> > > > Neither of these has clocks that need to be driven by linux. The only
> > > > user of QCA6390 Bluetooth in mainline is RB5. Bindings didn't exist
> > > > before so no commitment was ever made.
> > >
> > > This might make some laptop users unhappy.
> >
> > Like I said: without upstreamed DT bindings, we have never made any
> > commitment about the device properties. I doubt anyone will complain
> > though, I haven't seen any DT with QCA6390 with clock properties yet.
> > I wouldn't stress it for now.
>
> I was thinking about x86 laptops / M.2 cards. I'll see if I can locate one.
>

I don't get it, how could they ever get the clocks property without it
being defined in firmware?

Bart
Dmitry Baryshkov June 25, 2024, 8:55 a.m. UTC | #9 
On Tue, 25 Jun 2024 at 11:50, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> On Tue, Jun 25, 2024 at 9:47 AM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
> >
> > On Tue, 25 Jun 2024 at 10:46, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > >
> > > On Mon, Jun 24, 2024 at 11:20 PM Dmitry Baryshkov
> > > <dmitry.baryshkov@linaro.org> wrote:
> > > >
> > > > >
> > > > > Neither of these has clocks that need to be driven by linux. The only
> > > > > user of QCA6390 Bluetooth in mainline is RB5. Bindings didn't exist
> > > > > before so no commitment was ever made.
> > > >
> > > > This might make some laptop users unhappy.
> > >
> > > Like I said: without upstreamed DT bindings, we have never made any
> > > commitment about the device properties. I doubt anyone will complain
> > > though, I haven't seen any DT with QCA6390 with clock properties yet.
> > > I wouldn't stress it for now.
> >
> > I was thinking about x86 laptops / M.2 cards. I'll see if I can locate one.
> >
>
> I don't get it, how could they ever get the clocks property without it
> being defined in firmware?

The clock and bt_en are optional.
Bartosz Golaszewski June 25, 2024, 9:01 a.m. UTC | #10 
On Tue, Jun 25, 2024 at 10:55 AM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Tue, 25 Jun 2024 at 11:50, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >
> > On Tue, Jun 25, 2024 at 9:47 AM Dmitry Baryshkov
> > <dmitry.baryshkov@linaro.org> wrote:
> > >
> > > On Tue, 25 Jun 2024 at 10:46, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > > >
> > > > On Mon, Jun 24, 2024 at 11:20 PM Dmitry Baryshkov
> > > > <dmitry.baryshkov@linaro.org> wrote:
> > > > >
> > > > > >
> > > > > > Neither of these has clocks that need to be driven by linux. The only
> > > > > > user of QCA6390 Bluetooth in mainline is RB5. Bindings didn't exist
> > > > > > before so no commitment was ever made.
> > > > >
> > > > > This might make some laptop users unhappy.
> > > >
> > > > Like I said: without upstreamed DT bindings, we have never made any
> > > > commitment about the device properties. I doubt anyone will complain
> > > > though, I haven't seen any DT with QCA6390 with clock properties yet.
> > > > I wouldn't stress it for now.
> > >
> > > I was thinking about x86 laptops / M.2 cards. I'll see if I can locate one.
> > >
> >
> > I don't get it, how could they ever get the clocks property without it
> > being defined in firmware?
>
> The clock and bt_en are optional.
>

But you're worrying that the lack of this optional clock for QCA6390
will break it on some M.2 card? That cannot happen, can it? If it's on
an M.2 card then it would never be described in ACPI.
clk_get_optional() will always return NULL.

Bart
Dmitry Baryshkov June 25, 2024, 9:02 a.m. UTC | #11 
On Tue, 25 Jun 2024 at 12:01, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> On Tue, Jun 25, 2024 at 10:55 AM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
> >
> > On Tue, 25 Jun 2024 at 11:50, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > >
> > > On Tue, Jun 25, 2024 at 9:47 AM Dmitry Baryshkov
> > > <dmitry.baryshkov@linaro.org> wrote:
> > > >
> > > > On Tue, 25 Jun 2024 at 10:46, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > > > >
> > > > > On Mon, Jun 24, 2024 at 11:20 PM Dmitry Baryshkov
> > > > > <dmitry.baryshkov@linaro.org> wrote:
> > > > > >
> > > > > > >
> > > > > > > Neither of these has clocks that need to be driven by linux. The only
> > > > > > > user of QCA6390 Bluetooth in mainline is RB5. Bindings didn't exist
> > > > > > > before so no commitment was ever made.
> > > > > >
> > > > > > This might make some laptop users unhappy.
> > > > >
> > > > > Like I said: without upstreamed DT bindings, we have never made any
> > > > > commitment about the device properties. I doubt anyone will complain
> > > > > though, I haven't seen any DT with QCA6390 with clock properties yet.
> > > > > I wouldn't stress it for now.
> > > >
> > > > I was thinking about x86 laptops / M.2 cards. I'll see if I can locate one.
> > > >
> > >
> > > I don't get it, how could they ever get the clocks property without it
> > > being defined in firmware?
> >
> > The clock and bt_en are optional.
> >
>
> But you're worrying that the lack of this optional clock for QCA6390
> will break it on some M.2 card? That cannot happen, can it? If it's on
> an M.2 card then it would never be described in ACPI.
> clk_get_optional() will always return NULL.

Ack, thank you for the explanation.
patchwork-bot+bluetooth@kernel.org June 26, 2024, 8 p.m. UTC | #12 
Hello:

This patch was applied to bluetooth/bluetooth-next.git (master)
by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>:

On Mon, 24 Jun 2024 21:45:18 +0200 you wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> We unnecessarily fallthrough the case for QCA6390 when initializing the
> device and hit the condition where - due to the lack of the enable-gpio
> - we disable power management despite using the power sequencer. We don't
> need to look for clocks on this model so it makes more sense to just
> register the hci device and break the switch.
> 
> [...]

Here is the summary with links:
  - Bluetooth: qca: don't disable power management for QCA6390
    https://git.kernel.org/bluetooth/bluetooth-next/c/561cb4e9a238

You are awesome, thank you!
diff mbox series

Patch

diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 2b31f3dc33a9..bc6a49ba26f9 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -2402,7 +2402,13 @@  static int qca_serdev_probe(struct serdev_device *serdev)
 							   "bluetooth");
 		if (IS_ERR(qcadev->bt_power->pwrseq))
 			return PTR_ERR(qcadev->bt_power->pwrseq);
-		fallthrough;
+
+		err = hci_uart_register_device(&qcadev->serdev_hu, &qca_proto);
+		if (err) {
+			BT_ERR("qca6390 serdev registration failed");
+			return err;
+		}
+		break;
 
 	default:
 		qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable",