Hi Paul,
Appreciate your comments.
> -----Original Message-----
> From: Paul Menzel <pmenzel@xxxxxxxxxxxxx>
> Sent: Monday, June 12, 2023 12:02 AM
> To: K, Kiran <kiran.k@xxxxxxxxx>
> Cc: linux-bluetooth@xxxxxxxxxxxxxxx; Srivatsa, Ravishankar
> <ravishankar.srivatsa@xxxxxxxxx>
> Subject: Re: [RESEND v3] Bluetooth: btintel: Add support to reset bluetooth
> via ACPI DSM
>
> [Cc: Remove chethan.tumkur.nayaran@xxxxxxxxx (550 #5.1.0 Address
> rejected. (in reply to RCPT TO command))]
Ack. I will fix the typo in mail id.
>
> Am 11.06.23 um 11:42 schrieb Paul Menzel:
> > Dear Kiran,
> >
> >
> > Thank you for your patch. Some minor nits.
> >
> > Am 11.06.23 um 08:43 schrieb Kiran K:
> >> New Intel platforms supports reset of Bluetooth device via ACPI DSM
> >
> > 1. support
> > 2. one space after device
> >
Ack.
> >> methods. The legacy reset mechanism via GPIO will be deprecated in
> >
> > Can you please name the new platform it started with.
> >
Ack. I will add the details in section describing DSM details.
> >> future. This patch checks the platform support for reset methods and
> >> if supported uses the same instead of legacy GPIO toggling method.
> >
> > Could you please document the datasheet name, version and section this
> > is documented in?
> >
I am afraid to share these details as these are internal documents.
> >> ACPI firmware supports two types of reset method based on NIC card.
> >> (Discrete or Integrated).
> >
> > I’d remove the dot/period before (.
Ack.
> >
> >> 1. VSEC Type - Vendor Specific Extended Capability. Here BT_EN and
> >
> > Only one space after Here.
Ack.
> >
> >> BT_IF_SELECT lines are driven by a register in PCH cluster. This
> >> interface is supported on discrete BT solution.
> >>
> >> 2. WDISABLE2 - In this soluton, W_DISABLE2 pin in M.2 is connected to
> >
> > solution
Ack.
> >
> >> physical GPIO from PCH. The DSM interface shall toggle this to
> >> recover
> >> from error.
> >
> > Only one space after from.
Ack.
> >
> > How did you test this? (Maybe also paste the new log messages?)
> >
I tested DSM methods using a test module which was loaded after bluetooth stack was initialized.
This test module enumerates for required hdev object (based on VID/PID) and invokes,
*acpi_reset_method* if it is set.
> > I’d also appreciated one paragraph about the implentation.
Ok. I will add the details in cover notes. Briefly, In *hdev->setup* callback, driver checks if the platform supports ACPI DSM method to reset BT and initializes the function pointer defined in private *struct btintel_data*.
struct btintel_data {
DECLARE_BITMAP(flags, __INTEL_NUM_FLAGS);
int (*acpi_reset_method)(struct hci_dev *hdev);
};
Log messages:
[ 1121.900331] Bluetooth: hci0: Got intel BT device
[ 1121.900336] Bluetooth: hci0: Dsm method: 000000007f5632e9 --> *Test module invokes DSM method here *
[ 1121.994471] usb 3-10: USB disconnect, device number 13
[ 1122.522017] usb 3-10: new full-speed USB device number 14 using xhci_hcd
[ 1122.671712] usb 3-10: New USB device found, idVendor=8087, idProduct=0036, bcdDevice= 0.00
[ 1122.671744] usb 3-10: New USB device strings: Mfr=0, Product=0, SerialNumber=0
[ 1122.675678] usb 3-10: GPIO lookup for consumer reset
[ 1122.675700] usb 3-10: using ACPI for GPIO lookup
[ 1122.675731] usb 3-10: using lookup tables for GPIO lookup
[ 1122.675740] usb 3-10: No GPIO consumer reset found
[ 1122.678489] Bluetooth: hci0: Device revision is 0
[ 1122.678516] Bluetooth: hci0: Secure boot is enabled
[ 1122.678521] Bluetooth: hci0: OTP lock is disabled
[ 1122.678522] Bluetooth: hci0: API lock is disabled
[ 1122.678524] Bluetooth: hci0: Debug lock is disabled
[ 1122.678525] Bluetooth: hci0: Minimum firmware build 1 week 10 2014
[ 1122.678527] Bluetooth: hci0: Bootloader timestamp 2022.18 buildtype 1 build 16362
[ 1122.678533] Bluetooth: hci0: DSM reset method type: 0x01
[ 1122.678649] Bluetooth: hci0: No device address configured
[ 1122.678864] Bluetooth: hci0: Found device firmware: intel/ibt-0191-0191.sfi
[ 1122.678882] Bluetooth: hci0: Boot Address: 0x100800
[ 1122.678884] Bluetooth: hci0: Firmware Version: 79-11.23
[ 1126.349677] Bluetooth: hci0: Waiting for firmware download to complete
[ 1126.350069] Bluetooth: hci0: Firmware loaded in 3585157 usecs
[ 1126.350517] Bluetooth: hci0: Waiting for device to boot
[ 1126.401492] Bluetooth: hci0: Device booted in 50130 usecs
[ 1126.404364] Bluetooth: hci0: Found Intel DDC parameters: intel/ibt-0191-0191.ddc
[ 1126.407172] Bluetooth: hci0: Applying Intel DDC parameters completed
[ 1126.408147] Bluetooth: hci0: ACPI String: \_SB_.PC00.XHCI.RHUB.HS10.SADX
[ 1126.408153] Bluetooth: hci0: ACPI String: \_SB_.PC00.XHCI.RHUB.HS10.BRDY
[ 1126.408155] Bluetooth: hci0: ACPI String: \_SB_.PC00.XHCI.RHUB.HS10.ECKY
[ 1126.408157] Bluetooth: hci0: ACPI String: \_SB_.PC00.XHCI.RHUB.HS10.GPCX
[ 1126.408159] Bluetooth: hci0: ACPI String: \_SB_.PC00.XHCI.RHUB.HS10.BTLY
[ 1126.408161] Bluetooth: hci0: PPAG-BT: ACPI entry not found
[ 1126.410153] Bluetooth: hci0: Firmware timestamp 2023.11 buildtype 3 build 34127
[ 1126.509431] Bluetooth: MGMT ver 1.22
When controller goes unresponsive for 5 successful commands reset method is invoked which is expected to re-enumerate USB port.
> >> + if (intel_data->acpi_reset_method) {
> >> + if (test_and_set_bit(INTEL_ACPI_RESET_ACTIVE,
> >> +intel_data->flags)) {
> >> + bt_dev_err(hdev, "acpi: last reset failed ? Not
> >> +resetting again");
>>
>> Why the question mark? (No space before it.)
On invoking DSM reset method, driver flags it. This is done to avoid calling DSM method again if for some reason DSM fails to reset BT controller.
>>
> >> + return;
> >> + }
Thanks,
Kiran
