[RESEND v3] Bluetooth: btintel: Add support to reset bluetooth via ACPI DSM

[RESEND v3] Bluetooth: btintel: Add support to reset bluetooth via ACPI DSM

[Kiran K]

New Intel platforms supports reset of Bluetooth device  via ACPI DSM
methods. The legacy reset mechanism via GPIO will be deprecated in
future. This patch checks the platform support for reset methods and if
supported uses the same instead of legacy GPIO toggling method.

ACPI firmware supports two types of reset method based on NIC card.
(Discrete or Integrated).

1. VSEC Type - Vendor Specific Extended Capability. Here  BT_EN and
   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
   physical GPIO from PCH. The DSM interface shall toggle this to recover
   from  error.

Signed-off-by: Kiran K <kiran.k@intel.com>
---
 drivers/bluetooth/btintel.c | 121 ++++++++++++++++++++++++++++++++++++
 drivers/bluetooth/btintel.h |   2 +
 drivers/bluetooth/btusb.c   |  16 +++++
 3 files changed, 139 insertions(+)

diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
index d9349ba48281..dd1e48808ee2 100644
--- a/drivers/bluetooth/btintel.c
+++ b/drivers/bluetooth/btintel.c
@@ -10,6 +10,7 @@
 #include <linux/firmware.h>
 #include <linux/regmap.h>
 #include <linux/acpi.h>
+#include <acpi/acpi_bus.h>
 #include <asm/unaligned.h>
 
 #include <net/bluetooth/bluetooth.h>
@@ -27,6 +28,11 @@
 
 #define BTINTEL_PPAG_NAME   "PPAG"
 
+enum {
+	DSM_SET_WDISABLE2_DELAY = 1,
+	DSM_SET_RESET_METHOD = 3,
+};
+
 /* structure to store the PPAG data read from ACPI table */
 struct btintel_ppag {
 	u32	domain;
@@ -49,6 +55,10 @@ static struct {
 	u32        fw_build_num;
 } coredump_info;
 
+static const guid_t btintel_guid_dsm =
+	GUID_INIT(0xaa10f4e0, 0x81ac, 0x4233,
+		  0xab, 0xf6, 0x3b, 0x2a, 0xc5, 0x0e, 0x28, 0xd9);
+
 int btintel_check_bdaddr(struct hci_dev *hdev)
 {
 	struct hci_rp_read_bd_addr *bda;
@@ -2444,6 +2454,116 @@ static void btintel_set_ppag(struct hci_dev *hdev, struct intel_version_tlv *ver
 	kfree_skb(skb);
 }
 
+static int btintel_acpi_reset_method(struct hci_dev *hdev)
+{
+	int ret = 0;
+	acpi_status status;
+	union acpi_object *p, *ref;
+	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
+
+	status = acpi_evaluate_object(ACPI_HANDLE(GET_HCIDEV_DEV(hdev)), "_PRR", NULL, &buffer);
+	if (ACPI_FAILURE(status)) {
+		bt_dev_err(hdev, "Failed to run _PRR method");
+		ret = -ENODEV;
+		return ret;
+	}
+	p = buffer.pointer;
+
+	if (p->package.count != 1 || p->type != ACPI_TYPE_PACKAGE) {
+		bt_dev_err(hdev, "Invalid arguments");
+		ret = -EINVAL;
+		goto exit_on_error;
+	}
+
+	ref = &p->package.elements[0];
+	if (ref->type != ACPI_TYPE_LOCAL_REFERENCE) {
+		bt_dev_err(hdev, "Invalid object type: 0x%x", ref->type);
+		ret = -EINVAL;
+		goto exit_on_error;
+	}
+
+	status = acpi_evaluate_object(ref->reference.handle, "_RST", NULL, NULL);
+	if (ACPI_FAILURE(status)) {
+		bt_dev_err(hdev, "Failed to run_RST method");
+		ret = -ENODEV;
+		goto exit_on_error;
+	}
+
+exit_on_error:
+	kfree(buffer.pointer);
+	return ret;
+}
+
+static void btintel_set_dsm_reset_method(struct hci_dev *hdev,
+					 struct intel_version_tlv *ver_tlv)
+{
+	struct btintel_data *data = hci_get_priv(hdev);
+	acpi_handle handle = ACPI_HANDLE(GET_HCIDEV_DEV(hdev));
+	u8 reset_payload[4] = {0x01, 0x00, 0x01, 0x00};
+	union acpi_object *obj, argv4;
+	enum {
+		RESET_TYPE_WDISABLE2,
+		RESET_TYPE_VSEC
+	};
+
+	handle = ACPI_HANDLE(GET_HCIDEV_DEV(hdev));
+
+	if (!handle) {
+		bt_dev_dbg(hdev, "No support for bluetooth device in ACPI firmware");
+		return;
+	}
+
+	if (!acpi_has_method(handle, "_PRR")) {
+		bt_dev_err(hdev, "No support for _PRR ACPI method");
+		return;
+	}
+
+	switch (ver_tlv->cnvi_top & 0xfff) {
+	case 0x910: /* GalePeak2 */
+		reset_payload[2] = RESET_TYPE_VSEC;
+		break;
+	default:
+		/* WDISABLE2 is the default reset method */
+		reset_payload[2] = RESET_TYPE_WDISABLE2;
+
+		if (!acpi_check_dsm(handle, &btintel_guid_dsm, 0,
+				    BIT(DSM_SET_WDISABLE2_DELAY))) {
+			bt_dev_err(hdev, "No dsm support to set reset delay");
+			return;
+		}
+		argv4.integer.type = ACPI_TYPE_INTEGER;
+		/* delay required to toggle BT power */
+		argv4.integer.value = 160;
+		obj = acpi_evaluate_dsm(handle, &btintel_guid_dsm, 0,
+					DSM_SET_WDISABLE2_DELAY, &argv4);
+		if (!obj) {
+			bt_dev_err(hdev, "Failed to call dsm to set reset delay");
+			return;
+		}
+		ACPI_FREE(obj);
+	}
+
+	bt_dev_info(hdev, "DSM reset method type: 0x%02x", reset_payload[2]);
+
+	if (!acpi_check_dsm(handle, &btintel_guid_dsm, 0,
+			    DSM_SET_RESET_METHOD)) {
+		bt_dev_warn(hdev, "No support for dsm to set reset method");
+		return;
+	}
+	argv4.buffer.type = ACPI_TYPE_BUFFER;
+	argv4.buffer.length = sizeof(reset_payload);
+	argv4.buffer.pointer = reset_payload;
+
+	obj = acpi_evaluate_dsm(handle, &btintel_guid_dsm, 0,
+				DSM_SET_RESET_METHOD, &argv4);
+	if (!obj) {
+		bt_dev_err(hdev, "Failed to call dsm to set reset method");
+		return;
+	}
+	ACPI_FREE(obj);
+	data->acpi_reset_method = btintel_acpi_reset_method;
+}
+
 static int btintel_bootloader_setup_tlv(struct hci_dev *hdev,
 					struct intel_version_tlv *ver)
 {
@@ -2757,6 +2877,7 @@ static int btintel_setup_combined(struct hci_dev *hdev)
 		/* Setup MSFT Extension support */
 		btintel_set_msft_opcode(hdev,
 					INTEL_HW_VARIANT(ver_tlv.cnvi_bt));
+		btintel_set_dsm_reset_method(hdev, &ver_tlv);
 
 		err = btintel_bootloader_setup_tlv(hdev, &ver_tlv);
 		btintel_register_devcoredump_support(hdev);
diff --git a/drivers/bluetooth/btintel.h b/drivers/bluetooth/btintel.h
index d6a1dc8d8a82..7fd29ef038bd 100644
--- a/drivers/bluetooth/btintel.h
+++ b/drivers/bluetooth/btintel.h
@@ -166,12 +166,14 @@ enum {
 	INTEL_BROKEN_SHUTDOWN_LED,
 	INTEL_ROM_LEGACY,
 	INTEL_ROM_LEGACY_NO_WBS_SUPPORT,
+	INTEL_ACPI_RESET_ACTIVE,
 
 	__INTEL_NUM_FLAGS,
 };
 
 struct btintel_data {
 	DECLARE_BITMAP(flags, __INTEL_NUM_FLAGS);
+	int (*acpi_reset_method)(struct hci_dev *hdev);
 };
 
 #define btintel_set_flag(hdev, nr)					\
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 8776e0f93c73..c6ac63fdecfa 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -857,10 +857,26 @@ static void btusb_intel_cmd_timeout(struct hci_dev *hdev)
 {
 	struct btusb_data *data = hci_get_drvdata(hdev);
 	struct gpio_desc *reset_gpio = data->reset_gpio;
+	struct btintel_data *intel_data = hci_get_priv(hdev);
 
 	if (++data->cmd_timeout_cnt < 5)
 		return;
 
+	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");
+			return;
+		}
+
+		bt_dev_err(hdev, "Initiating acpi reset method");
+		/* If ACPI reset method fails, lets try with legacy GPIO
+		 * toggling
+		 */
+		if (!intel_data->acpi_reset_method(hdev)) {
+			return;
+		}
+	}
+
 	if (!reset_gpio) {
 		btusb_reset(hdev);
 		return;
-- 
2.25.1

RE: [RESEND,v3] Bluetooth: btintel: Add support to reset bluetooth via ACPI DSM

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1424 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=756042

---Test result---

Test Summary:
CheckPatch                    PASS      1.36 seconds
GitLint                       PASS      0.27 seconds
SubjectPrefix                 PASS      0.07 seconds
BuildKernel                   PASS      45.98 seconds
CheckAllWarning               PASS      50.33 seconds
CheckSparse                   PASS      56.27 seconds
CheckSmatch                   PASS      152.06 seconds
BuildKernel32                 PASS      44.53 seconds
TestRunnerSetup               PASS      627.65 seconds
TestRunner_l2cap-tester       PASS      21.93 seconds
TestRunner_iso-tester         PASS      32.39 seconds
TestRunner_bnep-tester        PASS      7.78 seconds
TestRunner_mgmt-tester        PASS      149.93 seconds
TestRunner_rfcomm-tester      PASS      12.25 seconds
TestRunner_sco-tester         PASS      11.30 seconds
TestRunner_ioctl-tester       PASS      13.60 seconds
TestRunner_mesh-tester        PASS      9.98 seconds
TestRunner_smp-tester         PASS      11.07 seconds
TestRunner_userchan-tester    PASS      8.27 seconds
IncrementalBuild              PASS      41.65 seconds



---
Regards,
Linux Bluetooth

Re: [RESEND v3] Bluetooth: btintel: Add support to reset bluetooth via ACPI DSM

[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

> methods. The legacy reset mechanism via GPIO will be deprecated in

Can you please name the new platform it started with.

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

> ACPI firmware supports two types of reset method based on NIC card.
> (Discrete or Integrated).

I’d remove the dot/period before (.

> 1. VSEC Type - Vendor Specific Extended Capability. Here  BT_EN and

Only one space after Here.

>     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

>     physical GPIO from PCH. The DSM interface shall toggle this to recover
>     from  error.

Only one space after from.

How did you test this? (Maybe also paste the new log messages?)

I’d also appreciated one paragraph about the implentation.

> Signed-off-by: Kiran K <kiran.k@intel.com>
> ---
>   drivers/bluetooth/btintel.c | 121 ++++++++++++++++++++++++++++++++++++
>   drivers/bluetooth/btintel.h |   2 +
>   drivers/bluetooth/btusb.c   |  16 +++++
>   3 files changed, 139 insertions(+)
> 
> diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
> index d9349ba48281..dd1e48808ee2 100644
> --- a/drivers/bluetooth/btintel.c
> +++ b/drivers/bluetooth/btintel.c
> @@ -10,6 +10,7 @@
>   #include <linux/firmware.h>
>   #include <linux/regmap.h>
>   #include <linux/acpi.h>
> +#include <acpi/acpi_bus.h>
>   #include <asm/unaligned.h>
>   
>   #include <net/bluetooth/bluetooth.h>
> @@ -27,6 +28,11 @@
>   
>   #define BTINTEL_PPAG_NAME   "PPAG"
>   
> +enum {
> +	DSM_SET_WDISABLE2_DELAY = 1,
> +	DSM_SET_RESET_METHOD = 3,
> +};
> +
>   /* structure to store the PPAG data read from ACPI table */
>   struct btintel_ppag {
>   	u32	domain;
> @@ -49,6 +55,10 @@ static struct {
>   	u32        fw_build_num;
>   } coredump_info;
>   
> +static const guid_t btintel_guid_dsm =
> +	GUID_INIT(0xaa10f4e0, 0x81ac, 0x4233,
> +		  0xab, 0xf6, 0x3b, 0x2a, 0xc5, 0x0e, 0x28, 0xd9);
> +
>   int btintel_check_bdaddr(struct hci_dev *hdev)
>   {
>   	struct hci_rp_read_bd_addr *bda;
> @@ -2444,6 +2454,116 @@ static void btintel_set_ppag(struct hci_dev *hdev, struct intel_version_tlv *ver
>   	kfree_skb(skb);
>   }
>   
> +static int btintel_acpi_reset_method(struct hci_dev *hdev)
> +{
> +	int ret = 0;
> +	acpi_status status;
> +	union acpi_object *p, *ref;
> +	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> +
> +	status = acpi_evaluate_object(ACPI_HANDLE(GET_HCIDEV_DEV(hdev)), "_PRR", NULL, &buffer);
> +	if (ACPI_FAILURE(status)) {
> +		bt_dev_err(hdev, "Failed to run _PRR method");

Will the failure code(?) be printed?

> +		ret = -ENODEV;
> +		return ret;
> +	}
> +	p = buffer.pointer;
> +
> +	if (p->package.count != 1 || p->type != ACPI_TYPE_PACKAGE) {
> +		bt_dev_err(hdev, "Invalid arguments");

Output the values?

> +		ret = -EINVAL;
> +		goto exit_on_error;
> +	}
> +
> +	ref = &p->package.elements[0];
> +	if (ref->type != ACPI_TYPE_LOCAL_REFERENCE) {
> +		bt_dev_err(hdev, "Invalid object type: 0x%x", ref->type);

… should be ACPI_TYPE_LOCAL_REFERENCE.

> +		ret = -EINVAL;
> +		goto exit_on_error;
> +	}
> +
> +	status = acpi_evaluate_object(ref->reference.handle, "_RST", NULL, NULL);
> +	if (ACPI_FAILURE(status)) {
> +		bt_dev_err(hdev, "Failed to run_RST method");

Will the failure code(?) be printed?

> +		ret = -ENODEV;
> +		goto exit_on_error;
> +	}
> +
> +exit_on_error:
> +	kfree(buffer.pointer);
> +	return ret;
> +}
> +
> +static void btintel_set_dsm_reset_method(struct hci_dev *hdev,
> +					 struct intel_version_tlv *ver_tlv)
> +{
> +	struct btintel_data *data = hci_get_priv(hdev);
> +	acpi_handle handle = ACPI_HANDLE(GET_HCIDEV_DEV(hdev));
> +	u8 reset_payload[4] = {0x01, 0x00, 0x01, 0x00};

In other parts you add spaces after { and before }.

> +	union acpi_object *obj, argv4;
> +	enum {
> +		RESET_TYPE_WDISABLE2,
> +		RESET_TYPE_VSEC
> +	};
> +
> +	handle = ACPI_HANDLE(GET_HCIDEV_DEV(hdev));
> +
> +	if (!handle) {
> +		bt_dev_dbg(hdev, "No support for bluetooth device in ACPI firmware");
> +		return;
> +	}
> +
> +	if (!acpi_has_method(handle, "_PRR")) {
> +		bt_dev_err(hdev, "No support for _PRR ACPI method");
> +		return;
> +	}
> +
> +	switch (ver_tlv->cnvi_top & 0xfff) {
> +	case 0x910: /* GalePeak2 */
> +		reset_payload[2] = RESET_TYPE_VSEC;
> +		break;
> +	default:
> +		/* WDISABLE2 is the default reset method */
> +		reset_payload[2] = RESET_TYPE_WDISABLE2;
> +
> +		if (!acpi_check_dsm(handle, &btintel_guid_dsm, 0,
> +				    BIT(DSM_SET_WDISABLE2_DELAY))) {
> +			bt_dev_err(hdev, "No dsm support to set reset delay");
> +			return;
> +		}
> +		argv4.integer.type = ACPI_TYPE_INTEGER;
> +		/* delay required to toggle BT power */
> +		argv4.integer.value = 160;

Where does that 160 come from?

> +		obj = acpi_evaluate_dsm(handle, &btintel_guid_dsm, 0,
> +					DSM_SET_WDISABLE2_DELAY, &argv4);
> +		if (!obj) {
> +			bt_dev_err(hdev, "Failed to call dsm to set reset delay");
> +			return;
> +		}
> +		ACPI_FREE(obj);
> +	}
> +
> +	bt_dev_info(hdev, "DSM reset method type: 0x%02x", reset_payload[2]);
> +
> +	if (!acpi_check_dsm(handle, &btintel_guid_dsm, 0,
> +			    DSM_SET_RESET_METHOD)) {

Does this fit in one line?

> +		bt_dev_warn(hdev, "No support for dsm to set reset method");
> +		return;
> +	}
> +	argv4.buffer.type = ACPI_TYPE_BUFFER;
> +	argv4.buffer.length = sizeof(reset_payload);
> +	argv4.buffer.pointer = reset_payload;
> +
> +	obj = acpi_evaluate_dsm(handle, &btintel_guid_dsm, 0,
> +				DSM_SET_RESET_METHOD, &argv4);
> +	if (!obj) {
> +		bt_dev_err(hdev, "Failed to call dsm to set reset method");
> +		return;
> +	}
> +	ACPI_FREE(obj);
> +	data->acpi_reset_method = btintel_acpi_reset_method;
> +}
> +
>   static int btintel_bootloader_setup_tlv(struct hci_dev *hdev,
>   					struct intel_version_tlv *ver)
>   {
> @@ -2757,6 +2877,7 @@ static int btintel_setup_combined(struct hci_dev *hdev)
>   		/* Setup MSFT Extension support */
>   		btintel_set_msft_opcode(hdev,
>   					INTEL_HW_VARIANT(ver_tlv.cnvi_bt));
> +		btintel_set_dsm_reset_method(hdev, &ver_tlv);
>   
>   		err = btintel_bootloader_setup_tlv(hdev, &ver_tlv);
>   		btintel_register_devcoredump_support(hdev);
> diff --git a/drivers/bluetooth/btintel.h b/drivers/bluetooth/btintel.h
> index d6a1dc8d8a82..7fd29ef038bd 100644
> --- a/drivers/bluetooth/btintel.h
> +++ b/drivers/bluetooth/btintel.h
> @@ -166,12 +166,14 @@ enum {
>   	INTEL_BROKEN_SHUTDOWN_LED,
>   	INTEL_ROM_LEGACY,
>   	INTEL_ROM_LEGACY_NO_WBS_SUPPORT,
> +	INTEL_ACPI_RESET_ACTIVE,
>   
>   	__INTEL_NUM_FLAGS,
>   };
>   
>   struct btintel_data {
>   	DECLARE_BITMAP(flags, __INTEL_NUM_FLAGS);
> +	int (*acpi_reset_method)(struct hci_dev *hdev);
>   };
>   
>   #define btintel_set_flag(hdev, nr)					\
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index 8776e0f93c73..c6ac63fdecfa 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -857,10 +857,26 @@ static void btusb_intel_cmd_timeout(struct hci_dev *hdev)
>   {
>   	struct btusb_data *data = hci_get_drvdata(hdev);
>   	struct gpio_desc *reset_gpio = data->reset_gpio;
> +	struct btintel_data *intel_data = hci_get_priv(hdev);
>   
>   	if (++data->cmd_timeout_cnt < 5)
>   		return;
>   
> +	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.)

> +			return;
> +		}
> +
> +		bt_dev_err(hdev, "Initiating acpi reset method");
> +		/* If ACPI reset method fails, lets try with legacy GPIO
> +		 * toggling
> +		 */
> +		if (!intel_data->acpi_reset_method(hdev)) {
> +			return;
> +		}
> +	}
> +
>   	if (!reset_gpio) {
>   		btusb_reset(hdev);
>   		return;


Kind regards,

Paul

Re: [RESEND v3] Bluetooth: btintel: Add support to reset bluetooth via ACPI DSM

[Paul Menzel]

[Cc: Remove chethan.tumkur.nayaran@intel.com (550 #5.1.0 Address 
rejected. (in reply to RCPT TO command))]

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
> 
>> methods. The legacy reset mechanism via GPIO will be deprecated in
> 
> Can you please name the new platform it started with.
> 
>> 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?
> 
>> ACPI firmware supports two types of reset method based on NIC card.
>> (Discrete or Integrated).
> 
> I’d remove the dot/period before (.
> 
>> 1. VSEC Type - Vendor Specific Extended Capability. Here  BT_EN and
> 
> Only one space after Here.
> 
>>     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
> 
>>     physical GPIO from PCH. The DSM interface shall toggle this to recover
>>     from  error.
> 
> Only one space after from.
> 
> How did you test this? (Maybe also paste the new log messages?)
> 
> I’d also appreciated one paragraph about the implentation.
> 
>> Signed-off-by: Kiran K <kiran.k@intel.com>
>> ---
>>   drivers/bluetooth/btintel.c | 121 ++++++++++++++++++++++++++++++++++++
>>   drivers/bluetooth/btintel.h |   2 +
>>   drivers/bluetooth/btusb.c   |  16 +++++
>>   3 files changed, 139 insertions(+)
>>
>> diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
>> index d9349ba48281..dd1e48808ee2 100644
>> --- a/drivers/bluetooth/btintel.c
>> +++ b/drivers/bluetooth/btintel.c
>> @@ -10,6 +10,7 @@
>>   #include <linux/firmware.h>
>>   #include <linux/regmap.h>
>>   #include <linux/acpi.h>
>> +#include <acpi/acpi_bus.h>
>>   #include <asm/unaligned.h>
>>   #include <net/bluetooth/bluetooth.h>
>> @@ -27,6 +28,11 @@
>>   #define BTINTEL_PPAG_NAME   "PPAG"
>> +enum {
>> +    DSM_SET_WDISABLE2_DELAY = 1,
>> +    DSM_SET_RESET_METHOD = 3,
>> +};
>> +
>>   /* structure to store the PPAG data read from ACPI table */
>>   struct btintel_ppag {
>>       u32    domain;
>> @@ -49,6 +55,10 @@ static struct {
>>       u32        fw_build_num;
>>   } coredump_info;
>> +static const guid_t btintel_guid_dsm =
>> +    GUID_INIT(0xaa10f4e0, 0x81ac, 0x4233,
>> +          0xab, 0xf6, 0x3b, 0x2a, 0xc5, 0x0e, 0x28, 0xd9);
>> +
>>   int btintel_check_bdaddr(struct hci_dev *hdev)
>>   {
>>       struct hci_rp_read_bd_addr *bda;
>> @@ -2444,6 +2454,116 @@ static void btintel_set_ppag(struct hci_dev 
>> *hdev, struct intel_version_tlv *ver
>>       kfree_skb(skb);
>>   }
>> +static int btintel_acpi_reset_method(struct hci_dev *hdev)
>> +{
>> +    int ret = 0;
>> +    acpi_status status;
>> +    union acpi_object *p, *ref;
>> +    struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
>> +
>> +    status = acpi_evaluate_object(ACPI_HANDLE(GET_HCIDEV_DEV(hdev)), "_PRR", NULL, &buffer);
>> +    if (ACPI_FAILURE(status)) {
>> +        bt_dev_err(hdev, "Failed to run _PRR method");
> 
> Will the failure code(?) be printed?
> 
>> +        ret = -ENODEV;
>> +        return ret;
>> +    }
>> +    p = buffer.pointer;
>> +
>> +    if (p->package.count != 1 || p->type != ACPI_TYPE_PACKAGE) {
>> +        bt_dev_err(hdev, "Invalid arguments");
> 
> Output the values?
> 
>> +        ret = -EINVAL;
>> +        goto exit_on_error;
>> +    }
>> +
>> +    ref = &p->package.elements[0];
>> +    if (ref->type != ACPI_TYPE_LOCAL_REFERENCE) {
>> +        bt_dev_err(hdev, "Invalid object type: 0x%x", ref->type);
> 
> … should be ACPI_TYPE_LOCAL_REFERENCE.
> 
>> +        ret = -EINVAL;
>> +        goto exit_on_error;
>> +    }
>> +
>> +    status = acpi_evaluate_object(ref->reference.handle, "_RST", NULL, NULL);
>> +    if (ACPI_FAILURE(status)) {
>> +        bt_dev_err(hdev, "Failed to run_RST method");
> 
> Will the failure code(?) be printed?
> 
>> +        ret = -ENODEV;
>> +        goto exit_on_error;
>> +    }
>> +
>> +exit_on_error:
>> +    kfree(buffer.pointer);
>> +    return ret;
>> +}
>> +
>> +static void btintel_set_dsm_reset_method(struct hci_dev *hdev,
>> +                     struct intel_version_tlv *ver_tlv)
>> +{
>> +    struct btintel_data *data = hci_get_priv(hdev);
>> +    acpi_handle handle = ACPI_HANDLE(GET_HCIDEV_DEV(hdev));
>> +    u8 reset_payload[4] = {0x01, 0x00, 0x01, 0x00};
> 
> In other parts you add spaces after { and before }.
> 
>> +    union acpi_object *obj, argv4;
>> +    enum {
>> +        RESET_TYPE_WDISABLE2,
>> +        RESET_TYPE_VSEC
>> +    };
>> +
>> +    handle = ACPI_HANDLE(GET_HCIDEV_DEV(hdev));
>> +
>> +    if (!handle) {
>> +        bt_dev_dbg(hdev, "No support for bluetooth device in ACPI firmware");
>> +        return;
>> +    }
>> +
>> +    if (!acpi_has_method(handle, "_PRR")) {
>> +        bt_dev_err(hdev, "No support for _PRR ACPI method");
>> +        return;
>> +    }
>> +
>> +    switch (ver_tlv->cnvi_top & 0xfff) {
>> +    case 0x910: /* GalePeak2 */
>> +        reset_payload[2] = RESET_TYPE_VSEC;
>> +        break;
>> +    default:
>> +        /* WDISABLE2 is the default reset method */
>> +        reset_payload[2] = RESET_TYPE_WDISABLE2;
>> +
>> +        if (!acpi_check_dsm(handle, &btintel_guid_dsm, 0,
>> +                    BIT(DSM_SET_WDISABLE2_DELAY))) {
>> +            bt_dev_err(hdev, "No dsm support to set reset delay");
>> +            return;
>> +        }
>> +        argv4.integer.type = ACPI_TYPE_INTEGER;
>> +        /* delay required to toggle BT power */
>> +        argv4.integer.value = 160;
> 
> Where does that 160 come from?
> 
>> +        obj = acpi_evaluate_dsm(handle, &btintel_guid_dsm, 0,
>> +                    DSM_SET_WDISABLE2_DELAY, &argv4);
>> +        if (!obj) {
>> +            bt_dev_err(hdev, "Failed to call dsm to set reset delay");
>> +            return;
>> +        }
>> +        ACPI_FREE(obj);
>> +    }
>> +
>> +    bt_dev_info(hdev, "DSM reset method type: 0x%02x", reset_payload[2]);
>> +
>> +    if (!acpi_check_dsm(handle, &btintel_guid_dsm, 0,
>> +                DSM_SET_RESET_METHOD)) {
> 
> Does this fit in one line?
> 
>> +        bt_dev_warn(hdev, "No support for dsm to set reset method");
>> +        return;
>> +    }
>> +    argv4.buffer.type = ACPI_TYPE_BUFFER;
>> +    argv4.buffer.length = sizeof(reset_payload);
>> +    argv4.buffer.pointer = reset_payload;
>> +
>> +    obj = acpi_evaluate_dsm(handle, &btintel_guid_dsm, 0,
>> +                DSM_SET_RESET_METHOD, &argv4);
>> +    if (!obj) {
>> +        bt_dev_err(hdev, "Failed to call dsm to set reset method");
>> +        return;
>> +    }
>> +    ACPI_FREE(obj);
>> +    data->acpi_reset_method = btintel_acpi_reset_method;
>> +}
>> +
>>   static int btintel_bootloader_setup_tlv(struct hci_dev *hdev,
>>                       struct intel_version_tlv *ver)
>>   {
>> @@ -2757,6 +2877,7 @@ static int btintel_setup_combined(struct hci_dev 
>> *hdev)
>>           /* Setup MSFT Extension support */
>>           btintel_set_msft_opcode(hdev,
>>                       INTEL_HW_VARIANT(ver_tlv.cnvi_bt));
>> +        btintel_set_dsm_reset_method(hdev, &ver_tlv);
>>           err = btintel_bootloader_setup_tlv(hdev, &ver_tlv);
>>           btintel_register_devcoredump_support(hdev);
>> diff --git a/drivers/bluetooth/btintel.h b/drivers/bluetooth/btintel.h
>> index d6a1dc8d8a82..7fd29ef038bd 100644
>> --- a/drivers/bluetooth/btintel.h
>> +++ b/drivers/bluetooth/btintel.h
>> @@ -166,12 +166,14 @@ enum {
>>       INTEL_BROKEN_SHUTDOWN_LED,
>>       INTEL_ROM_LEGACY,
>>       INTEL_ROM_LEGACY_NO_WBS_SUPPORT,
>> +    INTEL_ACPI_RESET_ACTIVE,
>>       __INTEL_NUM_FLAGS,
>>   };
>>   struct btintel_data {
>>       DECLARE_BITMAP(flags, __INTEL_NUM_FLAGS);
>> +    int (*acpi_reset_method)(struct hci_dev *hdev);
>>   };
>>   #define btintel_set_flag(hdev, nr)                    \
>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
>> index 8776e0f93c73..c6ac63fdecfa 100644
>> --- a/drivers/bluetooth/btusb.c
>> +++ b/drivers/bluetooth/btusb.c
>> @@ -857,10 +857,26 @@ static void btusb_intel_cmd_timeout(struct 
>> hci_dev *hdev)
>>   {
>>       struct btusb_data *data = hci_get_drvdata(hdev);
>>       struct gpio_desc *reset_gpio = data->reset_gpio;
>> +    struct btintel_data *intel_data = hci_get_priv(hdev);
>>       if (++data->cmd_timeout_cnt < 5)
>>           return;
>> +    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.)
> 
>> +            return;
>> +        }
>> +
>> +        bt_dev_err(hdev, "Initiating acpi reset method");
>> +        /* If ACPI reset method fails, lets try with legacy GPIO
>> +         * toggling
>> +         */
>> +        if (!intel_data->acpi_reset_method(hdev)) {
>> +            return;
>> +        }
>> +    }
>> +
>>       if (!reset_gpio) {
>>           btusb_reset(hdev);
>>           return;
> 
> 
> Kind regards,
> 
> Paul

RE: [RESEND v3] Bluetooth: btintel: Add support to reset bluetooth via ACPI DSM

[K, Kiran]

Hi Paul,

Appreciate your comments.

> -----Original Message-----
> From: Paul Menzel <pmenzel@molgen.mpg.de>
> Sent: Monday, June 12, 2023 12:02 AM
> To: K, Kiran <kiran.k@intel.com>
> Cc: linux-bluetooth@vger.kernel.org; Srivatsa, Ravishankar
> <ravishankar.srivatsa@intel.com>
> Subject: Re: [RESEND v3] Bluetooth: btintel: Add support to reset bluetooth
> via ACPI DSM
> 
> [Cc: Remove chethan.tumkur.nayaran@intel.com (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

Re: [RESEND v3] Bluetooth: btintel: Add support to reset bluetooth via ACPI DSM

[patchwork-bot+bluetooth]

Hello:

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

On Sun, 11 Jun 2023 12:13:42 +0530 you wrote:
> New Intel platforms supports reset of Bluetooth device  via ACPI DSM
> methods. The legacy reset mechanism via GPIO will be deprecated in
> future. This patch checks the platform support for reset methods and if
> supported uses the same instead of legacy GPIO toggling method.
> 
> ACPI firmware supports two types of reset method based on NIC card.
> (Discrete or Integrated).
> 
> [...]

Here is the summary with links:
  - [RESEND,v3] Bluetooth: btintel: Add support to reset bluetooth via ACPI DSM
    https://git.kernel.org/bluetooth/bluetooth-next/c/41022e05ccf6

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html