[PATCH v1] Bluetooth: btintel: Add support to configure TX power

[PATCH v1] Bluetooth: btintel: Add support to configure TX power

[Kiran K]

BRDS - Bluetooth Regulatory Domain Specific absorption rate
-----------------------------------------------------------

Bluetooth has regulatory limitations which prohibit or allow usage
of certain bands or channels as well as limiting Tx power. The Tx power
values can be configured in ACPI table. This patch reads ACPI entry of
Bluetooth SAR and configures the controller accordingly.

Signed-off-by: Kiran K <kiran.k@intel.com>
Signed-off-by: Seema S <seema.sreemantha@intel.com>
---
 drivers/bluetooth/btintel.c | 229 ++++++++++++++++++++++++++++++++++++
 drivers/bluetooth/btintel.h |  18 +++
 2 files changed, 247 insertions(+)

diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
index 818681c89db8..d3dc703eba78 100644
--- a/drivers/bluetooth/btintel.c
+++ b/drivers/bluetooth/btintel.c
@@ -9,6 +9,7 @@
 #include <linux/module.h>
 #include <linux/firmware.h>
 #include <linux/regmap.h>
+#include <linux/acpi.h>
 #include <asm/unaligned.h>
 
 #include <net/bluetooth/bluetooth.h>
@@ -32,6 +33,9 @@ struct cmd_write_boot_params {
 	u8  fw_build_yy;
 } __packed;
 
+#define BTINTEL_SAR_NAME	"BRDS"
+#define BTINTEL_SAR_PREFIX	"\\_SB_.PC00.XHCI.RHUB"
+
 int btintel_check_bdaddr(struct hci_dev *hdev)
 {
 	struct hci_rp_read_bd_addr *bda;
@@ -2250,6 +2254,228 @@ static int btintel_configure_offload(struct hci_dev *hdev)
 	return err;
 }
 
+static acpi_status btintel_sar_callback(acpi_handle handle, u32 lvl, void *data,
+					void **ret)
+{
+	acpi_status status;
+	int len;
+	struct btintel_sar *sar;
+	union acpi_object *p, *elements;
+	struct acpi_buffer string = {ACPI_ALLOCATE_BUFFER, NULL};
+	struct acpi_buffer buffer = {ACPI_ALLOCATE_BUFFER, NULL};
+
+	status = acpi_get_name(handle, ACPI_FULL_PATHNAME, &string);
+	if (ACPI_FAILURE(status)) {
+		BT_DBG("ACPI Failure: %s", acpi_format_exception(status));
+		return status;
+	}
+
+	if (strncmp(BTINTEL_SAR_PREFIX, string.pointer,
+		    strlen(BTINTEL_SAR_PREFIX))) {
+		kfree(string.pointer);
+		return AE_OK;
+	}
+
+	len = strlen(string.pointer);
+	if (strncmp((char *)string.pointer + len - 4, BTINTEL_SAR_NAME, 4)) {
+		kfree(string.pointer);
+		return AE_OK;
+	}
+	kfree(string.pointer);
+
+	status = acpi_evaluate_object(handle, NULL, NULL, &buffer);
+	if (ACPI_FAILURE(status)) {
+		BT_DBG("ACPI Failure: %s", acpi_format_exception(status));
+		return status;
+	}
+
+	p = buffer.pointer;
+	sar = data;
+
+	if (p->type != ACPI_TYPE_PACKAGE || p->package.count != 2) {
+		kfree(buffer.pointer);
+		BT_DBG("Invalid object type or package count");
+		return AE_ERROR;
+	}
+
+	elements = p->package.elements;
+
+	/* SAR table is located at element[1] */
+	p = &elements[1];
+
+	if (p->type != ACPI_TYPE_PACKAGE || p->package.count != 8) {
+		kfree(buffer.pointer);
+		return AE_ERROR;
+	}
+
+	sar->domain = (u8)p->package.elements[0].integer.value;
+	sar->type = (u8)p->package.elements[1].integer.value;
+	sar->br = (u32)p->package.elements[2].integer.value;
+	sar->edr2 = (u32)p->package.elements[3].integer.value;
+	sar->edr3 = (u32)p->package.elements[4].integer.value;
+	sar->le = (u32)p->package.elements[5].integer.value;
+	sar->le_2mhz = (u32)p->package.elements[6].integer.value;
+	sar->le_lr  = (u32)p->package.elements[7].integer.value;
+	kfree(buffer.pointer);
+	return AE_CTRL_TERMINATE;
+}
+
+static void btintel_send_sar_ddc(struct hci_dev *hdev, void *data, u8 len)
+{
+	struct sk_buff *skb;
+
+	skb = __hci_cmd_sync(hdev, 0xfc8b, len, data, HCI_CMD_TIMEOUT);
+	if (IS_ERR(skb)) {
+		bt_dev_warn(hdev, "Failed to send Intel Write SAR DDC (%ld)", PTR_ERR(skb));
+		return;
+	}
+	kfree_skb(skb);
+}
+
+static int btintel_set_legacy_sar(struct hci_dev *hdev, struct btintel_sar *sar)
+{
+	struct btintel_cp_ddc_write	*cmd;
+	u8	buffer[64];
+
+	if (!sar)
+		return -EINVAL;
+
+	cmd = (void *)buffer;
+
+	cmd->len = 3;
+	cmd->id = cpu_to_le16(0x0131);
+	cmd->data[0] = sar->br >> 3;
+	btintel_send_sar_ddc(hdev, cmd, 4);
+
+	cmd->len = 3;
+	cmd->id = cpu_to_le16(0x0132);
+	cmd->data[0] = sar->br >> 3;
+	btintel_send_sar_ddc(hdev, cmd, 4);
+
+	cmd->len = 5;
+	cmd->id = cpu_to_le16(0x0137);
+	cmd->data[0] = sar->br >> 3;
+	cmd->data[1] = sar->edr2 >> 3;
+	cmd->data[2] = sar->edr3 >> 3;
+	btintel_send_sar_ddc(hdev, cmd, 6);
+
+	cmd->len = 5;
+	cmd->id = cpu_to_le16(0x0138);
+	cmd->data[0] = sar->br >> 3;
+	cmd->data[1] = sar->edr2 >> 3;
+	cmd->data[2] = sar->edr3 >> 3;
+	btintel_send_sar_ddc(hdev, cmd, 6);
+
+	cmd->len = 5;
+	cmd->id = cpu_to_le16(0x013b);
+	cmd->data[0] = sar->br >> 3;
+	cmd->data[1] = sar->edr2 >> 3;
+	cmd->data[2] = sar->edr3 >> 3;
+	btintel_send_sar_ddc(hdev, cmd, 6);
+
+	cmd->len = 5;
+	cmd->id = cpu_to_le16(0x013c);
+	cmd->data[0] = sar->br >> 3;
+	cmd->data[1] = sar->edr2 >> 3;
+	cmd->data[2] = sar->edr3 >> 3;
+	btintel_send_sar_ddc(hdev, cmd, 6);
+
+	return 0;
+}
+
+static int btintel_set_mutual_sar(struct hci_dev *hdev, struct btintel_sar *sar)
+{
+	u8 buffer[64];
+	struct btintel_cp_ddc_write *cmd;
+	u8 enable[1] = {1};
+	struct sk_buff *skb;
+
+	if (!sar)
+		return -EINVAL;
+
+	cmd = (void *)buffer;
+
+	cmd->len = 3;
+	cmd->id = cpu_to_le16(0x019e);
+	if (!(sar->le_2mhz & BIT(7)))
+		cmd->data[0] = 0x01;
+	else
+		cmd->data[0] = 0x00;
+	btintel_send_sar_ddc(hdev, cmd, 4);
+
+	cmd->len = 3;
+	cmd->id = cpu_to_le16(0x019f);
+	cmd->data[0] = sar->le_lr;
+	btintel_send_sar_ddc(hdev, cmd, 4);
+
+	cmd->len = 3;
+	cmd->id = cpu_to_le16(0x01a0);
+	cmd->data[0] = sar->br;
+	btintel_send_sar_ddc(hdev, cmd, 4);
+
+	cmd->len = 3;
+	cmd->id = cpu_to_le16(0x01a1);
+	cmd->data[0] = sar->edr2;
+	btintel_send_sar_ddc(hdev, cmd, 4);
+
+	cmd->len = 3;
+	cmd->id = cpu_to_le16(0x01a2);
+	cmd->data[0] = sar->edr3;
+	btintel_send_sar_ddc(hdev, cmd, 4);
+
+	cmd->len = 3;
+	cmd->id = cpu_to_le16(0x01a3);
+	cmd->data[0] = sar->le;
+	btintel_send_sar_ddc(hdev, cmd, 4);
+
+	skb = __hci_cmd_sync(hdev, 0xfe25, 1, enable, HCI_CMD_TIMEOUT);
+	if (IS_ERR(skb)) {
+		bt_dev_warn(hdev, "Failed to send Intel SAR Enable (%ld)", PTR_ERR(skb));
+		return PTR_ERR(skb);
+	}
+	kfree_skb(skb);
+
+	return 0;
+}
+
+static int btintel_set_specific_absorption_rate(struct hci_dev *hdev,
+						struct intel_version_tlv *ver)
+{
+	acpi_status status;
+	struct btintel_sar sar;
+
+	switch (ver->cnvr_top & 0xfff) {
+	case 0x810: /* MsP */
+		break;
+	default:
+		return 0;
+	}
+
+	memset(&sar, 0, sizeof(sar));
+
+	status = acpi_walk_namespace(ACPI_TYPE_METHOD, ACPI_ROOT_OBJECT,
+				     ACPI_UINT32_MAX, NULL,
+				     btintel_sar_callback, &sar, NULL);
+
+	if (ACPI_FAILURE(status))
+		return -1;
+
+	if (sar.domain != 0x12)
+		return -1;
+
+	/* No need to configure controller if Bluetooth SAR is disabled in BIOS
+	 */
+	if (!sar.type)
+		return 0;
+
+	if (sar.type == 1) {
+		bt_dev_info(hdev, "Applying both legacy and mutual Bluetooth SAR");
+		btintel_set_legacy_sar(hdev, &sar);
+		btintel_set_mutual_sar(hdev, &sar);
+	}
+	return 0;
+}
+
 static int btintel_bootloader_setup_tlv(struct hci_dev *hdev,
 					struct intel_version_tlv *ver)
 {
@@ -2294,6 +2520,9 @@ static int btintel_bootloader_setup_tlv(struct hci_dev *hdev,
 	/* Read supported use cases and set callbacks to fetch datapath id */
 	btintel_configure_offload(hdev);
 
+	/* Set Specific Absorption Rate */
+	btintel_set_specific_absorption_rate(hdev, ver);
+
 	hci_dev_clear_flag(hdev, HCI_QUALITY_REPORT);
 
 	/* Read the Intel version information after loading the FW  */
diff --git a/drivers/bluetooth/btintel.h b/drivers/bluetooth/btintel.h
index e0060e58573c..7aa58fb7b02a 100644
--- a/drivers/bluetooth/btintel.h
+++ b/drivers/bluetooth/btintel.h
@@ -137,6 +137,24 @@ struct intel_offload_use_cases {
 	__u8	preset[8];
 } __packed;
 
+/* structure to store the data read from ACPI table */
+struct btintel_sar {
+	u8	domain;
+	u8	type;
+	u32	br;
+	u32	edr2;
+	u32	edr3;
+	u32	le;
+	u32	le_2mhz;
+	u32	le_lr;
+};
+
+struct btintel_cp_ddc_write {
+	u8	len;
+	__le16	id;
+	u8	data[0];
+} __packed;
+
 #define INTEL_HW_PLATFORM(cnvx_bt)	((u8)(((cnvx_bt) & 0x0000ff00) >> 8))
 #define INTEL_HW_VARIANT(cnvx_bt)	((u8)(((cnvx_bt) & 0x003f0000) >> 16))
 #define INTEL_CNVX_TOP_TYPE(cnvx_top)	((cnvx_top) & 0x00000fff)
-- 
2.17.1

RE: [v1] Bluetooth: btintel: Add support to configure TX power

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1097 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=630327

---Test result---

Test Summary:
CheckPatch                    PASS      1.91 seconds
GitLint                       PASS      1.00 seconds
SubjectPrefix                 PASS      0.82 seconds
BuildKernel                   PASS      31.71 seconds
BuildKernel32                 PASS      28.41 seconds
Incremental Build with patchesPASS      38.53 seconds
TestRunner: Setup             PASS      473.64 seconds
TestRunner: l2cap-tester      PASS      16.07 seconds
TestRunner: bnep-tester       PASS      6.37 seconds
TestRunner: mgmt-tester       PASS      106.73 seconds
TestRunner: rfcomm-tester     PASS      8.30 seconds
TestRunner: sco-tester        PASS      8.07 seconds
TestRunner: smp-tester        PASS      7.94 seconds
TestRunner: userchan-tester   PASS      6.61 seconds



---
Regards,
Linux Bluetooth

Re: [PATCH v1] Bluetooth: btintel: Add support to configure TX power

[Paul Menzel]

[Cc: +linux-acpi to check for best practices in ACPI table handling]


Dear Kiran,


Am 08.04.22 um 10:06 schrieb Kiran K:

Thank you for the patch. Maybe as commit message summary:

Support to configure TX power using BRDS ACPI table

> BRDS - Bluetooth Regulatory Domain Specific absorption rate
> -----------------------------------------------------------

Why this header? Integrate the long table name into the message below.

> 
> Bluetooth has regulatory limitations which prohibit or allow usage
> of certain bands or channels as well as limiting Tx power. The Tx power
> values can be configured in ACPI table. This patch reads ACPI entry of
> Bluetooth SAR and configures the controller accordingly.

It’d be great if you elaborated a little on the implementation? For 
example, what is the legacy SAR needed for?

How did you test this? What device and firmware providing the BRDS 
table? Maybe even paste the ASL table here as an example.

> Signed-off-by: Kiran K <kiran.k@intel.com>
> Signed-off-by: Seema S <seema.sreemantha@intel.com>
> ---
>   drivers/bluetooth/btintel.c | 229 ++++++++++++++++++++++++++++++++++++
>   drivers/bluetooth/btintel.h |  18 +++
>   2 files changed, 247 insertions(+)
> 
> diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
> index 818681c89db8..d3dc703eba78 100644
> --- a/drivers/bluetooth/btintel.c
> +++ b/drivers/bluetooth/btintel.c
> @@ -9,6 +9,7 @@
>   #include <linux/module.h>
>   #include <linux/firmware.h>
>   #include <linux/regmap.h>
> +#include <linux/acpi.h>
>   #include <asm/unaligned.h>
>   
>   #include <net/bluetooth/bluetooth.h>
> @@ -32,6 +33,9 @@ struct cmd_write_boot_params {
>   	u8  fw_build_yy;
>   } __packed;
>   
> +#define BTINTEL_SAR_NAME	"BRDS"
> +#define BTINTEL_SAR_PREFIX	"\\_SB_.PC00.XHCI.RHUB"
> +
>   int btintel_check_bdaddr(struct hci_dev *hdev)
>   {
>   	struct hci_rp_read_bd_addr *bda;
> @@ -2250,6 +2254,228 @@ static int btintel_configure_offload(struct hci_dev *hdev)
>   	return err;
>   }
>   
> +static acpi_status btintel_sar_callback(acpi_handle handle, u32 lvl, void *data,
> +					void **ret)
> +{
> +	acpi_status status;
> +	int len;

size_t

> +	struct btintel_sar *sar;
> +	union acpi_object *p, *elements;
> +	struct acpi_buffer string = {ACPI_ALLOCATE_BUFFER, NULL};
> +	struct acpi_buffer buffer = {ACPI_ALLOCATE_BUFFER, NULL};
> +
> +	status = acpi_get_name(handle, ACPI_FULL_PATHNAME, &string);
> +	if (ACPI_FAILURE(status)) {
> +		BT_DBG("ACPI Failure: %s", acpi_format_exception(status));
> +		return status;
> +	}
> +
> +	if (strncmp(BTINTEL_SAR_PREFIX, string.pointer,
> +		    strlen(BTINTEL_SAR_PREFIX))) {
> +		kfree(string.pointer);
> +		return AE_OK;
> +	}
> +
> +	len = strlen(string.pointer);
> +	if (strncmp((char *)string.pointer + len - 4, BTINTEL_SAR_NAME, 4)) {
> +		kfree(string.pointer);
> +		return AE_OK;
> +	}
> +	kfree(string.pointer);
> +
> +	status = acpi_evaluate_object(handle, NULL, NULL, &buffer);
> +	if (ACPI_FAILURE(status)) {
> +		BT_DBG("ACPI Failure: %s", acpi_format_exception(status));
> +		return status;
> +	}
> +
> +	p = buffer.pointer;
> +	sar = data;
> +
> +	if (p->type != ACPI_TYPE_PACKAGE || p->package.count != 2) {
> +		kfree(buffer.pointer);
> +		BT_DBG("Invalid object type or package count");

Please add the variable values to the debug line.

> +		return AE_ERROR;
> +	}
> +
> +	elements = p->package.elements;
> +
> +	/* SAR table is located at element[1] */
> +	p = &elements[1];
> +
> +	if (p->type != ACPI_TYPE_PACKAGE || p->package.count != 8) {
> +		kfree(buffer.pointer);
> +		return AE_ERROR;
> +	}
> +
> +	sar->domain = (u8)p->package.elements[0].integer.value;
> +	sar->type = (u8)p->package.elements[1].integer.value;
> +	sar->br = (u32)p->package.elements[2].integer.value;
> +	sar->edr2 = (u32)p->package.elements[3].integer.value;
> +	sar->edr3 = (u32)p->package.elements[4].integer.value;
> +	sar->le = (u32)p->package.elements[5].integer.value;
> +	sar->le_2mhz = (u32)p->package.elements[6].integer.value;
> +	sar->le_lr  = (u32)p->package.elements[7].integer.value;
> +	kfree(buffer.pointer);
> +	return AE_CTRL_TERMINATE;
> +}
> +
> +static void btintel_send_sar_ddc(struct hci_dev *hdev, void *data, u8 len)

Use native type for `len`? `unsigned int`?

> +{
> +	struct sk_buff *skb;
> +
> +	skb = __hci_cmd_sync(hdev, 0xfc8b, len, data, HCI_CMD_TIMEOUT);
> +	if (IS_ERR(skb)) {
> +		bt_dev_warn(hdev, "Failed to send Intel Write SAR DDC (%ld)", PTR_ERR(skb));
> +		return;
> +	}
> +	kfree_skb(skb);
> +}
> +
> +static int btintel_set_legacy_sar(struct hci_dev *hdev, struct btintel_sar *sar)
> +{
> +	struct btintel_cp_ddc_write	*cmd;
> +	u8	buffer[64];

Don’t try to align with tabs?

> +
> +	if (!sar)
> +		return -EINVAL;
> +
> +	cmd = (void *)buffer;
> +
> +	cmd->len = 3;
> +	cmd->id = cpu_to_le16(0x0131);

Add names for the command id’s?

> +	cmd->data[0] = sar->br >> 3;
> +	btintel_send_sar_ddc(hdev, cmd, 4);
> +
> +	cmd->len = 3;
> +	cmd->id = cpu_to_le16(0x0132);
> +	cmd->data[0] = sar->br >> 3;
> +	btintel_send_sar_ddc(hdev, cmd, 4);
> +
> +	cmd->len = 5;
> +	cmd->id = cpu_to_le16(0x0137);
> +	cmd->data[0] = sar->br >> 3;
> +	cmd->data[1] = sar->edr2 >> 3;
> +	cmd->data[2] = sar->edr3 >> 3;
> +	btintel_send_sar_ddc(hdev, cmd, 6);
> +
> +	cmd->len = 5;
> +	cmd->id = cpu_to_le16(0x0138);
> +	cmd->data[0] = sar->br >> 3;
> +	cmd->data[1] = sar->edr2 >> 3;
> +	cmd->data[2] = sar->edr3 >> 3;
> +	btintel_send_sar_ddc(hdev, cmd, 6);
> +
> +	cmd->len = 5;
> +	cmd->id = cpu_to_le16(0x013b);
> +	cmd->data[0] = sar->br >> 3;
> +	cmd->data[1] = sar->edr2 >> 3;
> +	cmd->data[2] = sar->edr3 >> 3;
> +	btintel_send_sar_ddc(hdev, cmd, 6);
> +
> +	cmd->len = 5;
> +	cmd->id = cpu_to_le16(0x013c);
> +	cmd->data[0] = sar->br >> 3;
> +	cmd->data[1] = sar->edr2 >> 3;
> +	cmd->data[2] = sar->edr3 >> 3;
> +	btintel_send_sar_ddc(hdev, cmd, 6);
> +
> +	return 0;
> +}
> +
> +static int btintel_set_mutual_sar(struct hci_dev *hdev, struct btintel_sar *sar)
> +{
> +	u8 buffer[64];
> +	struct btintel_cp_ddc_write *cmd;
> +	u8 enable[1] = {1};
> +	struct sk_buff *skb;
> +
> +	if (!sar)
> +		return -EINVAL;
> +
> +	cmd = (void *)buffer;
> +
> +	cmd->len = 3;
> +	cmd->id = cpu_to_le16(0x019e);
> +	if (!(sar->le_2mhz & BIT(7)))
> +		cmd->data[0] = 0x01;
> +	else
> +		cmd->data[0] = 0x00;

Use ternary operator?

> +	btintel_send_sar_ddc(hdev, cmd, 4);
> +
> +	cmd->len = 3;
> +	cmd->id = cpu_to_le16(0x019f);
> +	cmd->data[0] = sar->le_lr;
> +	btintel_send_sar_ddc(hdev, cmd, 4);
> +
> +	cmd->len = 3;
> +	cmd->id = cpu_to_le16(0x01a0);
> +	cmd->data[0] = sar->br;
> +	btintel_send_sar_ddc(hdev, cmd, 4);
> +
> +	cmd->len = 3;
> +	cmd->id = cpu_to_le16(0x01a1);
> +	cmd->data[0] = sar->edr2;
> +	btintel_send_sar_ddc(hdev, cmd, 4);
> +
> +	cmd->len = 3;
> +	cmd->id = cpu_to_le16(0x01a2);
> +	cmd->data[0] = sar->edr3;
> +	btintel_send_sar_ddc(hdev, cmd, 4);
> +
> +	cmd->len = 3;
> +	cmd->id = cpu_to_le16(0x01a3);
> +	cmd->data[0] = sar->le;
> +	btintel_send_sar_ddc(hdev, cmd, 4);
> +
> +	skb = __hci_cmd_sync(hdev, 0xfe25, 1, enable, HCI_CMD_TIMEOUT);
> +	if (IS_ERR(skb)) {
> +		bt_dev_warn(hdev, "Failed to send Intel SAR Enable (%ld)", PTR_ERR(skb));
> +		return PTR_ERR(skb);
> +	}
> +	kfree_skb(skb);
> +
> +	return 0;
> +}
> +
> +static int btintel_set_specific_absorption_rate(struct hci_dev *hdev,
> +						struct intel_version_tlv *ver)
> +{
> +	acpi_status status;
> +	struct btintel_sar sar;
> +
> +	switch (ver->cnvr_top & 0xfff) {
> +	case 0x810: /* MsP */
> +		break;
> +	default:
> +		return 0;
> +	}
> +
> +	memset(&sar, 0, sizeof(sar));
> +
> +	status = acpi_walk_namespace(ACPI_TYPE_METHOD, ACPI_ROOT_OBJECT,
> +				     ACPI_UINT32_MAX, NULL,
> +				     btintel_sar_callback, &sar, NULL);
> +
> +	if (ACPI_FAILURE(status))
> +		return -1;
> +
> +	if (sar.domain != 0x12)
> +		return -1;
> +
> +	/* No need to configure controller if Bluetooth SAR is disabled in BIOS
> +	 */

Put it on the line above?

> +	if (!sar.type)
> +		return 0;
> +
> +	if (sar.type == 1) {
> +		bt_dev_info(hdev, "Applying both legacy and mutual Bluetooth SAR");
> +		btintel_set_legacy_sar(hdev, &sar);
> +		btintel_set_mutual_sar(hdev, &sar);
> +	}
> +	return 0;
> +}
> +
>   static int btintel_bootloader_setup_tlv(struct hci_dev *hdev,
>   					struct intel_version_tlv *ver)
>   {
> @@ -2294,6 +2520,9 @@ static int btintel_bootloader_setup_tlv(struct hci_dev *hdev,
>   	/* Read supported use cases and set callbacks to fetch datapath id */
>   	btintel_configure_offload(hdev);
>   
> +	/* Set Specific Absorption Rate */
> +	btintel_set_specific_absorption_rate(hdev, ver);
> +
>   	hci_dev_clear_flag(hdev, HCI_QUALITY_REPORT);
>   
>   	/* Read the Intel version information after loading the FW  */
> diff --git a/drivers/bluetooth/btintel.h b/drivers/bluetooth/btintel.h
> index e0060e58573c..7aa58fb7b02a 100644
> --- a/drivers/bluetooth/btintel.h
> +++ b/drivers/bluetooth/btintel.h
> @@ -137,6 +137,24 @@ struct intel_offload_use_cases {
>   	__u8	preset[8];
>   } __packed;
>   
> +/* structure to store the data read from ACPI table */
> +struct btintel_sar {
> +	u8	domain;
> +	u8	type;
> +	u32	br;
> +	u32	edr2;
> +	u32	edr3;
> +	u32	le;
> +	u32	le_2mhz;
> +	u32	le_lr;

Can’t native types be used?

> +};
> +
> +struct btintel_cp_ddc_write {
> +	u8	len;
> +	__le16	id;
> +	u8	data[0];
> +} __packed;
> +
>   #define INTEL_HW_PLATFORM(cnvx_bt)	((u8)(((cnvx_bt) & 0x0000ff00) >> 8))
>   #define INTEL_HW_VARIANT(cnvx_bt)	((u8)(((cnvx_bt) & 0x003f0000) >> 16))
>   #define INTEL_CNVX_TOP_TYPE(cnvx_top)	((cnvx_top) & 0x00000fff)

Re: [PATCH v1] Bluetooth: btintel: Add support to configure TX power

[Luiz Augusto von Dentz]

Hi Kiran,

On Fri, Apr 8, 2022 at 1:02 AM Kiran K <kiran.k@intel.com> wrote:
>
> BRDS - Bluetooth Regulatory Domain Specific absorption rate
> -----------------------------------------------------------
>
> Bluetooth has regulatory limitations which prohibit or allow usage
> of certain bands or channels as well as limiting Tx power. The Tx power
> values can be configured in ACPI table. This patch reads ACPI entry of
> Bluetooth SAR and configures the controller accordingly.

It should be great to have a trace example of the commands in use, so
if we can have btmon decoding these to show what is being sent that
way we can also debug if something goes wrong.

> Signed-off-by: Kiran K <kiran.k@intel.com>
> Signed-off-by: Seema S <seema.sreemantha@intel.com>
> ---
>  drivers/bluetooth/btintel.c | 229 ++++++++++++++++++++++++++++++++++++
>  drivers/bluetooth/btintel.h |  18 +++
>  2 files changed, 247 insertions(+)
>
> diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
> index 818681c89db8..d3dc703eba78 100644
> --- a/drivers/bluetooth/btintel.c
> +++ b/drivers/bluetooth/btintel.c
> @@ -9,6 +9,7 @@
>  #include <linux/module.h>
>  #include <linux/firmware.h>
>  #include <linux/regmap.h>
> +#include <linux/acpi.h>
>  #include <asm/unaligned.h>
>
>  #include <net/bluetooth/bluetooth.h>
> @@ -32,6 +33,9 @@ struct cmd_write_boot_params {
>         u8  fw_build_yy;
>  } __packed;
>
> +#define BTINTEL_SAR_NAME       "BRDS"
> +#define BTINTEL_SAR_PREFIX     "\\_SB_.PC00.XHCI.RHUB"
> +
>  int btintel_check_bdaddr(struct hci_dev *hdev)
>  {
>         struct hci_rp_read_bd_addr *bda;
> @@ -2250,6 +2254,228 @@ static int btintel_configure_offload(struct hci_dev *hdev)
>         return err;
>  }
>
> +static acpi_status btintel_sar_callback(acpi_handle handle, u32 lvl, void *data,
> +                                       void **ret)
> +{
> +       acpi_status status;
> +       int len;
> +       struct btintel_sar *sar;
> +       union acpi_object *p, *elements;
> +       struct acpi_buffer string = {ACPI_ALLOCATE_BUFFER, NULL};
> +       struct acpi_buffer buffer = {ACPI_ALLOCATE_BUFFER, NULL};
> +
> +       status = acpi_get_name(handle, ACPI_FULL_PATHNAME, &string);
> +       if (ACPI_FAILURE(status)) {
> +               BT_DBG("ACPI Failure: %s", acpi_format_exception(status));
> +               return status;
> +       }
> +
> +       if (strncmp(BTINTEL_SAR_PREFIX, string.pointer,
> +                   strlen(BTINTEL_SAR_PREFIX))) {
> +               kfree(string.pointer);
> +               return AE_OK;
> +       }
> +
> +       len = strlen(string.pointer);
> +       if (strncmp((char *)string.pointer + len - 4, BTINTEL_SAR_NAME, 4)) {
> +               kfree(string.pointer);
> +               return AE_OK;
> +       }
> +       kfree(string.pointer);
> +
> +       status = acpi_evaluate_object(handle, NULL, NULL, &buffer);
> +       if (ACPI_FAILURE(status)) {
> +               BT_DBG("ACPI Failure: %s", acpi_format_exception(status));
> +               return status;
> +       }
> +
> +       p = buffer.pointer;
> +       sar = data;
> +
> +       if (p->type != ACPI_TYPE_PACKAGE || p->package.count != 2) {
> +               kfree(buffer.pointer);
> +               BT_DBG("Invalid object type or package count");
> +               return AE_ERROR;
> +       }
> +
> +       elements = p->package.elements;
> +
> +       /* SAR table is located at element[1] */
> +       p = &elements[1];
> +
> +       if (p->type != ACPI_TYPE_PACKAGE || p->package.count != 8) {
> +               kfree(buffer.pointer);
> +               return AE_ERROR;
> +       }
> +
> +       sar->domain = (u8)p->package.elements[0].integer.value;
> +       sar->type = (u8)p->package.elements[1].integer.value;
> +       sar->br = (u32)p->package.elements[2].integer.value;
> +       sar->edr2 = (u32)p->package.elements[3].integer.value;
> +       sar->edr3 = (u32)p->package.elements[4].integer.value;
> +       sar->le = (u32)p->package.elements[5].integer.value;
> +       sar->le_2mhz = (u32)p->package.elements[6].integer.value;
> +       sar->le_lr  = (u32)p->package.elements[7].integer.value;
> +       kfree(buffer.pointer);
> +       return AE_CTRL_TERMINATE;
> +}
> +
> +static void btintel_send_sar_ddc(struct hci_dev *hdev, void *data, u8 len)
> +{
> +       struct sk_buff *skb;
> +
> +       skb = __hci_cmd_sync(hdev, 0xfc8b, len, data, HCI_CMD_TIMEOUT);
> +       if (IS_ERR(skb)) {
> +               bt_dev_warn(hdev, "Failed to send Intel Write SAR DDC (%ld)", PTR_ERR(skb));
> +               return;
> +       }
> +       kfree_skb(skb);
> +}
> +
> +static int btintel_set_legacy_sar(struct hci_dev *hdev, struct btintel_sar *sar)
> +{
> +       struct btintel_cp_ddc_write     *cmd;
> +       u8      buffer[64];
> +
> +       if (!sar)
> +               return -EINVAL;

Add a memset to 0 as this may trigger uninitialized errors in the
likes of static analyzers.

> +       cmd = (void *)buffer;
> +
> +       cmd->len = 3;
> +       cmd->id = cpu_to_le16(0x0131);
> +       cmd->data[0] = sar->br >> 3;
> +       btintel_send_sar_ddc(hdev, cmd, 4);

We should probably handle the errors when sending these commands and
don't continue, also perhaps it would be better to have each command
in its own function with a proper name indicating what it is doing.

> +
> +       cmd->len = 3;
> +       cmd->id = cpu_to_le16(0x0132);
> +       cmd->data[0] = sar->br >> 3;
> +       btintel_send_sar_ddc(hdev, cmd, 4);
> +
> +       cmd->len = 5;
> +       cmd->id = cpu_to_le16(0x0137);
> +       cmd->data[0] = sar->br >> 3;
> +       cmd->data[1] = sar->edr2 >> 3;
> +       cmd->data[2] = sar->edr3 >> 3;
> +       btintel_send_sar_ddc(hdev, cmd, 6);
> +
> +       cmd->len = 5;
> +       cmd->id = cpu_to_le16(0x0138);
> +       cmd->data[0] = sar->br >> 3;
> +       cmd->data[1] = sar->edr2 >> 3;
> +       cmd->data[2] = sar->edr3 >> 3;
> +       btintel_send_sar_ddc(hdev, cmd, 6);
> +
> +       cmd->len = 5;
> +       cmd->id = cpu_to_le16(0x013b);
> +       cmd->data[0] = sar->br >> 3;
> +       cmd->data[1] = sar->edr2 >> 3;
> +       cmd->data[2] = sar->edr3 >> 3;
> +       btintel_send_sar_ddc(hdev, cmd, 6);
> +
> +       cmd->len = 5;
> +       cmd->id = cpu_to_le16(0x013c);
> +       cmd->data[0] = sar->br >> 3;
> +       cmd->data[1] = sar->edr2 >> 3;
> +       cmd->data[2] = sar->edr3 >> 3;
> +       btintel_send_sar_ddc(hdev, cmd, 6);
> +
> +       return 0;
> +}
> +
> +static int btintel_set_mutual_sar(struct hci_dev *hdev, struct btintel_sar *sar)
> +{
> +       u8 buffer[64];
> +       struct btintel_cp_ddc_write *cmd;
> +       u8 enable[1] = {1};
> +       struct sk_buff *skb;
> +
> +       if (!sar)
> +               return -EINVAL;
> +
> +       cmd = (void *)buffer;
> +
> +       cmd->len = 3;
> +       cmd->id = cpu_to_le16(0x019e);
> +       if (!(sar->le_2mhz & BIT(7)))
> +               cmd->data[0] = 0x01;
> +       else
> +               cmd->data[0] = 0x00;
> +       btintel_send_sar_ddc(hdev, cmd, 4);

Ditto as above.

> +       cmd->len = 3;
> +       cmd->id = cpu_to_le16(0x019f);
> +       cmd->data[0] = sar->le_lr;
> +       btintel_send_sar_ddc(hdev, cmd, 4);
> +
> +       cmd->len = 3;
> +       cmd->id = cpu_to_le16(0x01a0);
> +       cmd->data[0] = sar->br;
> +       btintel_send_sar_ddc(hdev, cmd, 4);
> +
> +       cmd->len = 3;
> +       cmd->id = cpu_to_le16(0x01a1);
> +       cmd->data[0] = sar->edr2;
> +       btintel_send_sar_ddc(hdev, cmd, 4);
> +
> +       cmd->len = 3;
> +       cmd->id = cpu_to_le16(0x01a2);
> +       cmd->data[0] = sar->edr3;
> +       btintel_send_sar_ddc(hdev, cmd, 4);
> +
> +       cmd->len = 3;
> +       cmd->id = cpu_to_le16(0x01a3);
> +       cmd->data[0] = sar->le;
> +       btintel_send_sar_ddc(hdev, cmd, 4);
> +
> +       skb = __hci_cmd_sync(hdev, 0xfe25, 1, enable, HCI_CMD_TIMEOUT);
> +       if (IS_ERR(skb)) {
> +               bt_dev_warn(hdev, "Failed to send Intel SAR Enable (%ld)", PTR_ERR(skb));
> +               return PTR_ERR(skb);
> +       }
> +       kfree_skb(skb);
> +
> +       return 0;
> +}
> +
> +static int btintel_set_specific_absorption_rate(struct hci_dev *hdev,
> +                                               struct intel_version_tlv *ver)
> +{
> +       acpi_status status;
> +       struct btintel_sar sar;
> +
> +       switch (ver->cnvr_top & 0xfff) {
> +       case 0x810: /* MsP */
> +               break;
> +       default:
> +               return 0;
> +       }
> +
> +       memset(&sar, 0, sizeof(sar));
> +
> +       status = acpi_walk_namespace(ACPI_TYPE_METHOD, ACPI_ROOT_OBJECT,
> +                                    ACPI_UINT32_MAX, NULL,
> +                                    btintel_sar_callback, &sar, NULL);
> +
> +       if (ACPI_FAILURE(status))
> +               return -1;
> +
> +       if (sar.domain != 0x12)
> +               return -1;
> +
> +       /* No need to configure controller if Bluetooth SAR is disabled in BIOS
> +        */
> +       if (!sar.type)
> +               return 0;
> +
> +       if (sar.type == 1) {
> +               bt_dev_info(hdev, "Applying both legacy and mutual Bluetooth SAR");
> +               btintel_set_legacy_sar(hdev, &sar);
> +               btintel_set_mutual_sar(hdev, &sar);
> +       }
> +       return 0;
> +}
> +
>  static int btintel_bootloader_setup_tlv(struct hci_dev *hdev,
>                                         struct intel_version_tlv *ver)
>  {
> @@ -2294,6 +2520,9 @@ static int btintel_bootloader_setup_tlv(struct hci_dev *hdev,
>         /* Read supported use cases and set callbacks to fetch datapath id */
>         btintel_configure_offload(hdev);
>
> +       /* Set Specific Absorption Rate */
> +       btintel_set_specific_absorption_rate(hdev, ver);
> +
>         hci_dev_clear_flag(hdev, HCI_QUALITY_REPORT);
>
>         /* Read the Intel version information after loading the FW  */
> diff --git a/drivers/bluetooth/btintel.h b/drivers/bluetooth/btintel.h
> index e0060e58573c..7aa58fb7b02a 100644
> --- a/drivers/bluetooth/btintel.h
> +++ b/drivers/bluetooth/btintel.h
> @@ -137,6 +137,24 @@ struct intel_offload_use_cases {
>         __u8    preset[8];
>  } __packed;
>
> +/* structure to store the data read from ACPI table */
> +struct btintel_sar {
> +       u8      domain;
> +       u8      type;
> +       u32     br;
> +       u32     edr2;
> +       u32     edr3;
> +       u32     le;
> +       u32     le_2mhz;
> +       u32     le_lr;
> +};

The above should also be using __packed, also for data packets the
multibyte field shall tell what is the expected endianness so given
the id below is using __le16 I assume the fields above shall be
__le32.

> +struct btintel_cp_ddc_write {
> +       u8      len;
> +       __le16  id;
> +       u8      data[0];
> +} __packed;
> +
>  #define INTEL_HW_PLATFORM(cnvx_bt)     ((u8)(((cnvx_bt) & 0x0000ff00) >> 8))
>  #define INTEL_HW_VARIANT(cnvx_bt)      ((u8)(((cnvx_bt) & 0x003f0000) >> 16))
>  #define INTEL_CNVX_TOP_TYPE(cnvx_top)  ((cnvx_top) & 0x00000fff)
> --
> 2.17.1
>


-- 
Luiz Augusto von Dentz

RE: [PATCH v1] Bluetooth: btintel: Add support to configure TX power

[K, Kiran]

Hi Paul,

Sorry for the delay. Thanks for your valuable comments.

> -----Original Message-----
> From: Paul Menzel <pmenzel@molgen.mpg.de>
> Sent: Saturday, April 9, 2022 12:53 PM
> To: K, Kiran <kiran.k@intel.com>
> Cc: linux-bluetooth@vger.kernel.org; Srivatsa, Ravishankar
> <ravishankar.srivatsa@intel.com>; Tumkur Narayan, Chethan
> <chethan.tumkur.narayan@intel.com>; Sreemantha, Seema
> <seema.sreemantha@intel.com>; linux-acpi@vger.kernel.org
> Subject: Re: [PATCH v1] Bluetooth: btintel: Add support to configure TX
> power
> 
> [Cc: +linux-acpi to check for best practices in ACPI table handling]
> 
> 
> Dear Kiran,
> 
> 
> Am 08.04.22 um 10:06 schrieb Kiran K:
> 
> Thank you for the patch. Maybe as commit message summary:
> 
> Support to configure TX power using BRDS ACPI table
> 
> > BRDS - Bluetooth Regulatory Domain Specific absorption rate
> > -----------------------------------------------------------
> 
> Why this header? Integrate the long table name into the message below.
> 
Ack. 

> >
> > Bluetooth has regulatory limitations which prohibit or allow usage of
> > certain bands or channels as well as limiting Tx power. The Tx power
> > values can be configured in ACPI table. This patch reads ACPI entry of
> > Bluetooth SAR and configures the controller accordingly.
> 
> It’d be great if you elaborated a little on the implementation? For example,
> what is the legacy SAR needed for?
> 
> How did you test this? What device and firmware providing the BRDS table?
> Maybe even paste the ASL table here as an example.
> 

Ack. I will update the commit message with details in v2 version of patch.

> > Signed-off-by: Kiran K <kiran.k@intel.com>
> > Signed-off-by: Seema S <seema.sreemantha@intel.com>
> > ---
> >   drivers/bluetooth/btintel.c | 229
> ++++++++++++++++++++++++++++++++++++
> >   drivers/bluetooth/btintel.h |  18 +++
> >   2 files changed, 247 insertions(+)
> >
> > diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
> > index 818681c89db8..d3dc703eba78 100644
> > --- a/drivers/bluetooth/btintel.c
> > +++ b/drivers/bluetooth/btintel.c
> > @@ -9,6 +9,7 @@
> >   #include <linux/module.h>
> >   #include <linux/firmware.h>
> >   #include <linux/regmap.h>
> > +#include <linux/acpi.h>
> >   #include <asm/unaligned.h>
> >
> >   #include <net/bluetooth/bluetooth.h> @@ -32,6 +33,9 @@ struct
> > cmd_write_boot_params {
> >   	u8  fw_build_yy;
> >   } __packed;
> >
> > +#define BTINTEL_SAR_NAME	"BRDS"
> > +#define BTINTEL_SAR_PREFIX	"\\_SB_.PC00.XHCI.RHUB"
> > +
> >   int btintel_check_bdaddr(struct hci_dev *hdev)
> >   {
> >   	struct hci_rp_read_bd_addr *bda;
> > @@ -2250,6 +2254,228 @@ static int btintel_configure_offload(struct
> hci_dev *hdev)
> >   	return err;
> >   }
> >
> > +static acpi_status btintel_sar_callback(acpi_handle handle, u32 lvl, void
> *data,
> > +					void **ret)
> > +{
> > +	acpi_status status;
> > +	int len;
> 
> size_t
Ack.
> 
> > +	struct btintel_sar *sar;
> > +	union acpi_object *p, *elements;
> > +	struct acpi_buffer string = {ACPI_ALLOCATE_BUFFER, NULL};
> > +	struct acpi_buffer buffer = {ACPI_ALLOCATE_BUFFER, NULL};
> > +
> > +	status = acpi_get_name(handle, ACPI_FULL_PATHNAME, &string);
> > +	if (ACPI_FAILURE(status)) {
> > +		BT_DBG("ACPI Failure: %s", acpi_format_exception(status));
> > +		return status;
> > +	}
> > +
> > +	if (strncmp(BTINTEL_SAR_PREFIX, string.pointer,
> > +		    strlen(BTINTEL_SAR_PREFIX))) {
> > +		kfree(string.pointer);
> > +		return AE_OK;
> > +	}
> > +
> > +	len = strlen(string.pointer);
> > +	if (strncmp((char *)string.pointer + len - 4, BTINTEL_SAR_NAME, 4)) {
> > +		kfree(string.pointer);
> > +		return AE_OK;
> > +	}
> > +	kfree(string.pointer);
> > +
> > +	status = acpi_evaluate_object(handle, NULL, NULL, &buffer);
> > +	if (ACPI_FAILURE(status)) {
> > +		BT_DBG("ACPI Failure: %s", acpi_format_exception(status));
> > +		return status;
> > +	}
> > +
> > +	p = buffer.pointer;
> > +	sar = data;
> > +
> > +	if (p->type != ACPI_TYPE_PACKAGE || p->package.count != 2) {
> > +		kfree(buffer.pointer);
> > +		BT_DBG("Invalid object type or package count");
> 
> Please add the variable values to the debug line.
> 
Ack.

> > +		return AE_ERROR;
> > +	}
> > +
> > +	elements = p->package.elements;
> > +
> > +	/* SAR table is located at element[1] */
> > +	p = &elements[1];
> > +
> > +	if (p->type != ACPI_TYPE_PACKAGE || p->package.count != 8) {
> > +		kfree(buffer.pointer);
> > +		return AE_ERROR;
> > +	}
> > +
> > +	sar->domain = (u8)p->package.elements[0].integer.value;
> > +	sar->type = (u8)p->package.elements[1].integer.value;
> > +	sar->br = (u32)p->package.elements[2].integer.value;
> > +	sar->edr2 = (u32)p->package.elements[3].integer.value;
> > +	sar->edr3 = (u32)p->package.elements[4].integer.value;
> > +	sar->le = (u32)p->package.elements[5].integer.value;
> > +	sar->le_2mhz = (u32)p->package.elements[6].integer.value;
> > +	sar->le_lr  = (u32)p->package.elements[7].integer.value;
> > +	kfree(buffer.pointer);
> > +	return AE_CTRL_TERMINATE;
> > +}
> > +
> > +static void btintel_send_sar_ddc(struct hci_dev *hdev, void *data, u8
> > +len)
> 
> Use native type for `len`? `unsigned int`?
> 
Ack.

> > +{
> > +	struct sk_buff *skb;
> > +
> > +	skb = __hci_cmd_sync(hdev, 0xfc8b, len, data, HCI_CMD_TIMEOUT);
> > +	if (IS_ERR(skb)) {
> > +		bt_dev_warn(hdev, "Failed to send Intel Write SAR DDC
> (%ld)", PTR_ERR(skb));
> > +		return;
> > +	}
> > +	kfree_skb(skb);
> > +}
> > +
> > +static int btintel_set_legacy_sar(struct hci_dev *hdev, struct
> > +btintel_sar *sar) {
> > +	struct btintel_cp_ddc_write	*cmd;
> > +	u8	buffer[64];
> 
> Don’t try to align with tabs?
> 
Ack.

> > +
> > +	if (!sar)
> > +		return -EINVAL;
> > +
> > +	cmd = (void *)buffer;
> > +
> > +	cmd->len = 3;
> > +	cmd->id = cpu_to_le16(0x0131);
> 
> Add names for the command id’s?
> 
Ack.

> > +	cmd->data[0] = sar->br >> 3;
> > +	btintel_send_sar_ddc(hdev, cmd, 4);
> > +
> > +	cmd->len = 3;
> > +	cmd->id = cpu_to_le16(0x0132);
> > +	cmd->data[0] = sar->br >> 3;
> > +	btintel_send_sar_ddc(hdev, cmd, 4);
> > +
> > +	cmd->len = 5;
> > +	cmd->id = cpu_to_le16(0x0137);
> > +	cmd->data[0] = sar->br >> 3;
> > +	cmd->data[1] = sar->edr2 >> 3;
> > +	cmd->data[2] = sar->edr3 >> 3;
> > +	btintel_send_sar_ddc(hdev, cmd, 6);
> > +
> > +	cmd->len = 5;
> > +	cmd->id = cpu_to_le16(0x0138);
> > +	cmd->data[0] = sar->br >> 3;
> > +	cmd->data[1] = sar->edr2 >> 3;
> > +	cmd->data[2] = sar->edr3 >> 3;
> > +	btintel_send_sar_ddc(hdev, cmd, 6);
> > +
> > +	cmd->len = 5;
> > +	cmd->id = cpu_to_le16(0x013b);
> > +	cmd->data[0] = sar->br >> 3;
> > +	cmd->data[1] = sar->edr2 >> 3;
> > +	cmd->data[2] = sar->edr3 >> 3;
> > +	btintel_send_sar_ddc(hdev, cmd, 6);
> > +
> > +	cmd->len = 5;
> > +	cmd->id = cpu_to_le16(0x013c);
> > +	cmd->data[0] = sar->br >> 3;
> > +	cmd->data[1] = sar->edr2 >> 3;
> > +	cmd->data[2] = sar->edr3 >> 3;
> > +	btintel_send_sar_ddc(hdev, cmd, 6);
> > +
> > +	return 0;
> > +}
> > +
> > +static int btintel_set_mutual_sar(struct hci_dev *hdev, struct
> > +btintel_sar *sar) {
> > +	u8 buffer[64];
> > +	struct btintel_cp_ddc_write *cmd;
> > +	u8 enable[1] = {1};
> > +	struct sk_buff *skb;
> > +
> > +	if (!sar)
> > +		return -EINVAL;
> > +
> > +	cmd = (void *)buffer;
> > +
> > +	cmd->len = 3;
> > +	cmd->id = cpu_to_le16(0x019e);
> > +	if (!(sar->le_2mhz & BIT(7)))
> > +		cmd->data[0] = 0x01;
> > +	else
> > +		cmd->data[0] = 0x00;
> 
> Use ternary operator?
Ack.
> 
> > +	btintel_send_sar_ddc(hdev, cmd, 4);
> > +
> > +	cmd->len = 3;
> > +	cmd->id = cpu_to_le16(0x019f);
> > +	cmd->data[0] = sar->le_lr;
> > +	btintel_send_sar_ddc(hdev, cmd, 4);
> > +
> > +	cmd->len = 3;
> > +	cmd->id = cpu_to_le16(0x01a0);
> > +	cmd->data[0] = sar->br;
> > +	btintel_send_sar_ddc(hdev, cmd, 4);
> > +
> > +	cmd->len = 3;
> > +	cmd->id = cpu_to_le16(0x01a1);
> > +	cmd->data[0] = sar->edr2;
> > +	btintel_send_sar_ddc(hdev, cmd, 4);
> > +
> > +	cmd->len = 3;
> > +	cmd->id = cpu_to_le16(0x01a2);
> > +	cmd->data[0] = sar->edr3;
> > +	btintel_send_sar_ddc(hdev, cmd, 4);
> > +
> > +	cmd->len = 3;
> > +	cmd->id = cpu_to_le16(0x01a3);
> > +	cmd->data[0] = sar->le;
> > +	btintel_send_sar_ddc(hdev, cmd, 4);
> > +
> > +	skb = __hci_cmd_sync(hdev, 0xfe25, 1, enable, HCI_CMD_TIMEOUT);
> > +	if (IS_ERR(skb)) {
> > +		bt_dev_warn(hdev, "Failed to send Intel SAR Enable (%ld)",
> PTR_ERR(skb));
> > +		return PTR_ERR(skb);
> > +	}
> > +	kfree_skb(skb);
> > +
> > +	return 0;
> > +}
> > +
> > +static int btintel_set_specific_absorption_rate(struct hci_dev *hdev,
> > +						struct intel_version_tlv *ver)
> > +{
> > +	acpi_status status;
> > +	struct btintel_sar sar;
> > +
> > +	switch (ver->cnvr_top & 0xfff) {
> > +	case 0x810: /* MsP */
> > +		break;
> > +	default:
> > +		return 0;
> > +	}
> > +
> > +	memset(&sar, 0, sizeof(sar));
> > +
> > +	status = acpi_walk_namespace(ACPI_TYPE_METHOD,
> ACPI_ROOT_OBJECT,
> > +				     ACPI_UINT32_MAX, NULL,
> > +				     btintel_sar_callback, &sar, NULL);
> > +
> > +	if (ACPI_FAILURE(status))
> > +		return -1;
> > +
> > +	if (sar.domain != 0x12)
> > +		return -1;
> > +
> > +	/* No need to configure controller if Bluetooth SAR is disabled in
> BIOS
> > +	 */
> 
> Put it on the line above?
> 
Ack.

> > +	if (!sar.type)
> > +		return 0;
> > +
> > +	if (sar.type == 1) {
> > +		bt_dev_info(hdev, "Applying both legacy and mutual
> Bluetooth SAR");
> > +		btintel_set_legacy_sar(hdev, &sar);
> > +		btintel_set_mutual_sar(hdev, &sar);
> > +	}
> > +	return 0;
> > +}
> > +
> >   static int btintel_bootloader_setup_tlv(struct hci_dev *hdev,
> >   					struct intel_version_tlv *ver)
> >   {
> > @@ -2294,6 +2520,9 @@ static int btintel_bootloader_setup_tlv(struct
> hci_dev *hdev,
> >   	/* Read supported use cases and set callbacks to fetch datapath id */
> >   	btintel_configure_offload(hdev);
> >
> > +	/* Set Specific Absorption Rate */
> > +	btintel_set_specific_absorption_rate(hdev, ver);
> > +
> >   	hci_dev_clear_flag(hdev, HCI_QUALITY_REPORT);
> >
> >   	/* Read the Intel version information after loading the FW  */ diff
> > --git a/drivers/bluetooth/btintel.h b/drivers/bluetooth/btintel.h
> > index e0060e58573c..7aa58fb7b02a 100644
> > --- a/drivers/bluetooth/btintel.h
> > +++ b/drivers/bluetooth/btintel.h
> > @@ -137,6 +137,24 @@ struct intel_offload_use_cases {
> >   	__u8	preset[8];
> >   } __packed;
> >
> > +/* structure to store the data read from ACPI table */ struct
> > +btintel_sar {
> > +	u8	domain;
> > +	u8	type;
> > +	u32	br;
> > +	u32	edr2;
> > +	u32	edr3;
> > +	u32	le;
> > +	u32	le_2mhz;
> > +	u32	le_lr;
> 
> Can’t native types be used?
No.  some fields size is 4 bytes. 
> 
> > +};
> > +
> > +struct btintel_cp_ddc_write {
> > +	u8	len;
> > +	__le16	id;
> > +	u8	data[0];
> > +} __packed;
> > +
> >   #define INTEL_HW_PLATFORM(cnvx_bt)	((u8)(((cnvx_bt) &
> 0x0000ff00) >> 8))
> >   #define INTEL_HW_VARIANT(cnvx_bt)	((u8)(((cnvx_bt) &
> 0x003f0000) >> 16))
> >   #define INTEL_CNVX_TOP_TYPE(cnvx_top)	((cnvx_top) & 0x00000fff)

Thanks,
Kiran

RE: [PATCH v1] Bluetooth: btintel: Add support to configure TX power

[K, Kiran]

Hi Luiz,

> -----Original Message-----
> From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
> Sent: Tuesday, April 12, 2022 12:15 AM
> To: K, Kiran <kiran.k@intel.com>
> Cc: linux-bluetooth@vger.kernel.org; Srivatsa, Ravishankar
> <ravishankar.srivatsa@intel.com>; Tumkur Narayan, Chethan
> <chethan.tumkur.narayan@intel.com>; Sreemantha, Seema
> <seema.sreemantha@intel.com>
> Subject: Re: [PATCH v1] Bluetooth: btintel: Add support to configure TX
> power
> 
> Hi Kiran,
> 
> On Fri, Apr 8, 2022 at 1:02 AM Kiran K <kiran.k@intel.com> wrote:
> >
> > BRDS - Bluetooth Regulatory Domain Specific absorption rate
> > -----------------------------------------------------------
> >
> > Bluetooth has regulatory limitations which prohibit or allow usage of
> > certain bands or channels as well as limiting Tx power. The Tx power
> > values can be configured in ACPI table. This patch reads ACPI entry of
> > Bluetooth SAR and configures the controller accordingly.
> 
> It should be great to have a trace example of the commands in use, so if we
> can have btmon decoding these to show what is being sent that way we can
> also debug if something goes wrong.
> 
Ack. I will add decoding of ddc commands to btmon and submit a separate patch.

> > Signed-off-by: Kiran K <kiran.k@intel.com>
> > Signed-off-by: Seema S <seema.sreemantha@intel.com>
> > ---
> >  drivers/bluetooth/btintel.c | 229
> > ++++++++++++++++++++++++++++++++++++
> >  drivers/bluetooth/btintel.h |  18 +++
> >  2 files changed, 247 insertions(+)
> >
> > diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
> > index 818681c89db8..d3dc703eba78 100644
> > --- a/drivers/bluetooth/btintel.c
> > +++ b/drivers/bluetooth/btintel.c
> > @@ -9,6 +9,7 @@
> >  #include <linux/module.h>
> >  #include <linux/firmware.h>
> >  #include <linux/regmap.h>
> > +#include <linux/acpi.h>
> >  #include <asm/unaligned.h>
> >
> >  #include <net/bluetooth/bluetooth.h>
> > @@ -32,6 +33,9 @@ struct cmd_write_boot_params {
> >         u8  fw_build_yy;
> >  } __packed;
> >
> > +#define BTINTEL_SAR_NAME       "BRDS"
> > +#define BTINTEL_SAR_PREFIX     "\\_SB_.PC00.XHCI.RHUB"
> > +
> >  int btintel_check_bdaddr(struct hci_dev *hdev)  {
> >         struct hci_rp_read_bd_addr *bda; @@ -2250,6 +2254,228 @@
> > static int btintel_configure_offload(struct hci_dev *hdev)
> >         return err;
> >  }
> >
> > +static acpi_status btintel_sar_callback(acpi_handle handle, u32 lvl, void
> *data,
> > +                                       void **ret) {
> > +       acpi_status status;
> > +       int len;
> > +       struct btintel_sar *sar;
> > +       union acpi_object *p, *elements;
> > +       struct acpi_buffer string = {ACPI_ALLOCATE_BUFFER, NULL};
> > +       struct acpi_buffer buffer = {ACPI_ALLOCATE_BUFFER, NULL};
> > +
> > +       status = acpi_get_name(handle, ACPI_FULL_PATHNAME, &string);
> > +       if (ACPI_FAILURE(status)) {
> > +               BT_DBG("ACPI Failure: %s", acpi_format_exception(status));
> > +               return status;
> > +       }
> > +
> > +       if (strncmp(BTINTEL_SAR_PREFIX, string.pointer,
> > +                   strlen(BTINTEL_SAR_PREFIX))) {
> > +               kfree(string.pointer);
> > +               return AE_OK;
> > +       }
> > +
> > +       len = strlen(string.pointer);
> > +       if (strncmp((char *)string.pointer + len - 4, BTINTEL_SAR_NAME, 4)) {
> > +               kfree(string.pointer);
> > +               return AE_OK;
> > +       }
> > +       kfree(string.pointer);
> > +
> > +       status = acpi_evaluate_object(handle, NULL, NULL, &buffer);
> > +       if (ACPI_FAILURE(status)) {
> > +               BT_DBG("ACPI Failure: %s", acpi_format_exception(status));
> > +               return status;
> > +       }
> > +
> > +       p = buffer.pointer;
> > +       sar = data;
> > +
> > +       if (p->type != ACPI_TYPE_PACKAGE || p->package.count != 2) {
> > +               kfree(buffer.pointer);
> > +               BT_DBG("Invalid object type or package count");
> > +               return AE_ERROR;
> > +       }
> > +
> > +       elements = p->package.elements;
> > +
> > +       /* SAR table is located at element[1] */
> > +       p = &elements[1];
> > +
> > +       if (p->type != ACPI_TYPE_PACKAGE || p->package.count != 8) {
> > +               kfree(buffer.pointer);
> > +               return AE_ERROR;
> > +       }
> > +
> > +       sar->domain = (u8)p->package.elements[0].integer.value;
> > +       sar->type = (u8)p->package.elements[1].integer.value;
> > +       sar->br = (u32)p->package.elements[2].integer.value;
> > +       sar->edr2 = (u32)p->package.elements[3].integer.value;
> > +       sar->edr3 = (u32)p->package.elements[4].integer.value;
> > +       sar->le = (u32)p->package.elements[5].integer.value;
> > +       sar->le_2mhz = (u32)p->package.elements[6].integer.value;
> > +       sar->le_lr  = (u32)p->package.elements[7].integer.value;
> > +       kfree(buffer.pointer);
> > +       return AE_CTRL_TERMINATE;
> > +}
> > +
> > +static void btintel_send_sar_ddc(struct hci_dev *hdev, void *data, u8
> > +len) {
> > +       struct sk_buff *skb;
> > +
> > +       skb = __hci_cmd_sync(hdev, 0xfc8b, len, data, HCI_CMD_TIMEOUT);
> > +       if (IS_ERR(skb)) {
> > +               bt_dev_warn(hdev, "Failed to send Intel Write SAR DDC (%ld)",
> PTR_ERR(skb));
> > +               return;
> > +       }
> > +       kfree_skb(skb);
> > +}
> > +
> > +static int btintel_set_legacy_sar(struct hci_dev *hdev, struct
> > +btintel_sar *sar) {
> > +       struct btintel_cp_ddc_write     *cmd;
> > +       u8      buffer[64];
> > +
> > +       if (!sar)
> > +               return -EINVAL;
> 
> Add a memset to 0 as this may trigger uninitialized errors in the likes of static
> analyzers.
> 
Ack.

> > +       cmd = (void *)buffer;
> > +
> > +       cmd->len = 3;
> > +       cmd->id = cpu_to_le16(0x0131);
> > +       cmd->data[0] = sar->br >> 3;
> > +       btintel_send_sar_ddc(hdev, cmd, 4);
> 
> We should probably handle the errors when sending these commands and
> don't continue, also perhaps it would be better to have each command in its
> own function with a proper name indicating what it is doing.
> 
The command structure is same for all the commands. To address one of Paul's comment, need to define marcos for command id. Let me know if this is OK. 
> > +
> > +       cmd->len = 3;
> > +       cmd->id = cpu_to_le16(0x0132);
> > +       cmd->data[0] = sar->br >> 3;
> > +       btintel_send_sar_ddc(hdev, cmd, 4);
> > +
> > +       cmd->len = 5;
> > +       cmd->id = cpu_to_le16(0x0137);
> > +       cmd->data[0] = sar->br >> 3;
> > +       cmd->data[1] = sar->edr2 >> 3;
> > +       cmd->data[2] = sar->edr3 >> 3;
> > +       btintel_send_sar_ddc(hdev, cmd, 6);
> > +
> > +       cmd->len = 5;
> > +       cmd->id = cpu_to_le16(0x0138);
> > +       cmd->data[0] = sar->br >> 3;
> > +       cmd->data[1] = sar->edr2 >> 3;
> > +       cmd->data[2] = sar->edr3 >> 3;
> > +       btintel_send_sar_ddc(hdev, cmd, 6);
> > +
> > +       cmd->len = 5;
> > +       cmd->id = cpu_to_le16(0x013b);
> > +       cmd->data[0] = sar->br >> 3;
> > +       cmd->data[1] = sar->edr2 >> 3;
> > +       cmd->data[2] = sar->edr3 >> 3;
> > +       btintel_send_sar_ddc(hdev, cmd, 6);
> > +
> > +       cmd->len = 5;
> > +       cmd->id = cpu_to_le16(0x013c);
> > +       cmd->data[0] = sar->br >> 3;
> > +       cmd->data[1] = sar->edr2 >> 3;
> > +       cmd->data[2] = sar->edr3 >> 3;
> > +       btintel_send_sar_ddc(hdev, cmd, 6);
> > +
> > +       return 0;
> > +}
> > +
> > +static int btintel_set_mutual_sar(struct hci_dev *hdev, struct
> > +btintel_sar *sar) {
> > +       u8 buffer[64];
> > +       struct btintel_cp_ddc_write *cmd;
> > +       u8 enable[1] = {1};
> > +       struct sk_buff *skb;
> > +
> > +       if (!sar)
> > +               return -EINVAL;
> > +
> > +       cmd = (void *)buffer;
> > +
> > +       cmd->len = 3;
> > +       cmd->id = cpu_to_le16(0x019e);
> > +       if (!(sar->le_2mhz & BIT(7)))
> > +               cmd->data[0] = 0x01;
> > +       else
> > +               cmd->data[0] = 0x00;
> > +       btintel_send_sar_ddc(hdev, cmd, 4);
> 
> Ditto as above.
> 
> > +       cmd->len = 3;
> > +       cmd->id = cpu_to_le16(0x019f);
> > +       cmd->data[0] = sar->le_lr;
> > +       btintel_send_sar_ddc(hdev, cmd, 4);
> > +
> > +       cmd->len = 3;
> > +       cmd->id = cpu_to_le16(0x01a0);
> > +       cmd->data[0] = sar->br;
> > +       btintel_send_sar_ddc(hdev, cmd, 4);
> > +
> > +       cmd->len = 3;
> > +       cmd->id = cpu_to_le16(0x01a1);
> > +       cmd->data[0] = sar->edr2;
> > +       btintel_send_sar_ddc(hdev, cmd, 4);
> > +
> > +       cmd->len = 3;
> > +       cmd->id = cpu_to_le16(0x01a2);
> > +       cmd->data[0] = sar->edr3;
> > +       btintel_send_sar_ddc(hdev, cmd, 4);
> > +
> > +       cmd->len = 3;
> > +       cmd->id = cpu_to_le16(0x01a3);
> > +       cmd->data[0] = sar->le;
> > +       btintel_send_sar_ddc(hdev, cmd, 4);
> > +
> > +       skb = __hci_cmd_sync(hdev, 0xfe25, 1, enable, HCI_CMD_TIMEOUT);
> > +       if (IS_ERR(skb)) {
> > +               bt_dev_warn(hdev, "Failed to send Intel SAR Enable (%ld)",
> PTR_ERR(skb));
> > +               return PTR_ERR(skb);
> > +       }
> > +       kfree_skb(skb);
> > +
> > +       return 0;
> > +}
> > +
> > +static int btintel_set_specific_absorption_rate(struct hci_dev *hdev,
> > +                                               struct
> > +intel_version_tlv *ver) {
> > +       acpi_status status;
> > +       struct btintel_sar sar;
> > +
> > +       switch (ver->cnvr_top & 0xfff) {
> > +       case 0x810: /* MsP */
> > +               break;
> > +       default:
> > +               return 0;
> > +       }
> > +
> > +       memset(&sar, 0, sizeof(sar));
> > +
> > +       status = acpi_walk_namespace(ACPI_TYPE_METHOD,
> ACPI_ROOT_OBJECT,
> > +                                    ACPI_UINT32_MAX, NULL,
> > +                                    btintel_sar_callback, &sar,
> > + NULL);
> > +
> > +       if (ACPI_FAILURE(status))
> > +               return -1;
> > +
> > +       if (sar.domain != 0x12)
> > +               return -1;
> > +
> > +       /* No need to configure controller if Bluetooth SAR is disabled in BIOS
> > +        */
> > +       if (!sar.type)
> > +               return 0;
> > +
> > +       if (sar.type == 1) {
> > +               bt_dev_info(hdev, "Applying both legacy and mutual Bluetooth
> SAR");
> > +               btintel_set_legacy_sar(hdev, &sar);
> > +               btintel_set_mutual_sar(hdev, &sar);
> > +       }
> > +       return 0;
> > +}
> > +
> >  static int btintel_bootloader_setup_tlv(struct hci_dev *hdev,
> >                                         struct intel_version_tlv *ver)
> > { @@ -2294,6 +2520,9 @@ static int btintel_bootloader_setup_tlv(struct
> > hci_dev *hdev,
> >         /* Read supported use cases and set callbacks to fetch datapath id */
> >         btintel_configure_offload(hdev);
> >
> > +       /* Set Specific Absorption Rate */
> > +       btintel_set_specific_absorption_rate(hdev, ver);
> > +
> >         hci_dev_clear_flag(hdev, HCI_QUALITY_REPORT);
> >
> >         /* Read the Intel version information after loading the FW  */
> > diff --git a/drivers/bluetooth/btintel.h b/drivers/bluetooth/btintel.h
> > index e0060e58573c..7aa58fb7b02a 100644
> > --- a/drivers/bluetooth/btintel.h
> > +++ b/drivers/bluetooth/btintel.h
> > @@ -137,6 +137,24 @@ struct intel_offload_use_cases {
> >         __u8    preset[8];
> >  } __packed;
> >
> > +/* structure to store the data read from ACPI table */ struct
> > +btintel_sar {
> > +       u8      domain;
> > +       u8      type;
> > +       u32     br;
> > +       u32     edr2;
> > +       u32     edr3;
> > +       u32     le;
> > +       u32     le_2mhz;
> > +       u32     le_lr;
> > +};
> 
> The above should also be using __packed, also for data packets the multibyte
> field shall tell what is the expected endianness so given the id below is using
> __le16 I assume the fields above shall be __le32.
> 
> > +struct btintel_cp_ddc_write {
> > +       u8      len;
> > +       __le16  id;
> > +       u8      data[0];
> > +} __packed;
> > +
> >  #define INTEL_HW_PLATFORM(cnvx_bt)     ((u8)(((cnvx_bt) & 0x0000ff00)
> >> 8))
> >  #define INTEL_HW_VARIANT(cnvx_bt)      ((u8)(((cnvx_bt) & 0x003f0000)
> >> 16))
> >  #define INTEL_CNVX_TOP_TYPE(cnvx_top)  ((cnvx_top) & 0x00000fff)
> > --
> > 2.17.1
> >
> 
> 
> --
> Luiz Augusto von Dentz

Thanks,
Kiran