Hi Paul, Thanks for your comments. >-----Original Message----- >From: Paul Menzel <pmenzel@xxxxxxxxxxxxx> >Sent: Friday, June 21, 2024 12:46 PM >To: K, Kiran <kiran.k@xxxxxxxxx> >Cc: Srivatsa, Ravishankar <ravishankar.srivatsa@xxxxxxxxx>; Tumkur Narayan, >Chethan <chethan.tumkur.narayan@xxxxxxxxx>; Devegowda, Chandrashekar ><chandrashekar.devegowda@xxxxxxxxx>; Satija, Vijay <vijay.satija@xxxxxxxxx>; >linux-bluetooth@xxxxxxxxxxxxxxx >Subject: Re: [PATCH v2] Bluetooth: btintel: Allow lowering of drive strength of >BRI > >Dear Kiran, > > >Thank you for the patch. > > >Am 21.06.24 um 08:44 schrieb Kiran K: >> BRI (Bluetooth Radio Interface) traffic from CNVr to CNVi was found >> causing cross talk step errors to WiFi. > >Interesting. Can you please elaborate how that can be detected? Packet errors - Need special hardware to detect this issue. > >> As a workaround, driver needs to >> reduce the drive strength of BRI. During *setup*, driver reads the >> drive strength value from efi variable and passes it controller via >> vendor > >Where is this EFI variable described? https://www.kernel.org/doc/html//v5.13/filesystems/efivarfs.html > >… passes it *to the* controller … Ack. > >> specific command with opcode 0xfc0a. > >Please document your test system and also add the new log messages. Ack. > >> Signed-off-by: Kiran K <kiran.k@xxxxxxxxx> >> --- >> drivers/bluetooth/btintel.c | 114 >++++++++++++++++++++++++++++++++++++ >> 1 file changed, 114 insertions(+) >> >> diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c >> index 5d735391545a..3dc557aac43d 100644 >> --- a/drivers/bluetooth/btintel.c >> +++ b/drivers/bluetooth/btintel.c >> @@ -12,6 +12,8 @@ >> #include <linux/acpi.h> >> #include <acpi/acpi_bus.h> >> #include <asm/unaligned.h> >> +#include <linux/efi.h> >> + >> >> #include <net/bluetooth/bluetooth.h> >> #include <net/bluetooth/hci_core.h> >> @@ -49,6 +51,38 @@ static const guid_t btintel_guid_dsm = >> GUID_INIT(0xaa10f4e0, 0x81ac, 0x4233, >> 0xab, 0xf6, 0x3b, 0x2a, 0xc5, 0x0e, 0x28, 0xd9); >> >> +static void *btintel_uefi_get_variable(efi_char16_t *name, efi_guid_t >> +*guid) { >> + void *data; >> + efi_status_t status; >> + size_t data_size = 0; >> + >> + if (!IS_ENABLED(CONFIG_EFI)) >> + return ERR_PTR(-EOPNOTSUPP); >> + >> + if (!efi_rt_services_supported(EFI_RT_SUPPORTED_GET_VARIABLE)) >> + return ERR_PTR(-EOPNOTSUPP); >> + >> + status = efi.get_variable(name, guid, NULL, &data_size, NULL); >> + >> + if (status != EFI_BUFFER_TOO_SMALL || !data_size) >> + return ERR_PTR(-EIO); >> + >> + data = kmalloc(data_size, GFP_KERNEL); >> + >> + if (!data) >> + return ERR_PTR(-ENOMEM); >> + >> + status = efi.get_variable(name, guid, NULL, &data_size, data); >> + >> + if (status != EFI_SUCCESS) { >> + kfree(data); >> + return ERR_PTR(-ENXIO); >> + } >> + >> + return data; >> +} >> + >> int btintel_check_bdaddr(struct hci_dev *hdev) >> { >> struct hci_rp_read_bd_addr *bda; >> @@ -2615,6 +2649,80 @@ static u8 btintel_classify_pkt_type(struct hci_dev >*hdev, struct sk_buff *skb) >> return hci_skb_pkt_type(skb); >> } >> >> +static int btintel_set_dsbr(struct hci_dev *hdev, struct >> +intel_version_tlv *ver) > >Add a comment, what dsbr is? Ack. > >> +{ >> + struct btintel_dsbr_cmd { >> + u8 enable; >> + u8 dsbr; >> + } __packed; >> + >> + struct btintel_dsbr { >> + u8 header; >> + u32 dsbr; >> + } __packed; >> + >> + struct btintel_dsbr *dsbr; >> + struct btintel_dsbr_cmd cmd; >> + struct sk_buff *skb; >> + u8 status; >> + efi_guid_t guid = EFI_GUID(0xe65d8884, 0xd4af, 0x4b20, 0x8d, 0x03, >> + 0x77, 0x2e, 0xcc, 0x3d, 0xa5, 0x31); >> + >> + memset(&cmd, 0, sizeof(cmd)); >> + dsbr = btintel_uefi_get_variable(L"UefiCnvCommonDSBR", &guid); >> + if (IS_ERR(dsbr)) { >> + /* If efi variable is not present, driver still needs to send >> + * 0xfc0a command with default values >> + */ >> + bt_dev_dbg(hdev, "Error reading efi DSBR (%ld)", > >Maybe: Error reading EFI variable UefiCnvCommonDSBR (%ld) Ack. > >> + PTR_ERR(dsbr)); >> + dsbr = NULL; >> + } >> + >> + if (dsbr) { >> + /* bit0: 0 - Use firmware default value >> + * 1 - Override firmware value >> + * bit3:1 - Reserved >> + * bit7:4 - DSBR override values >> + * bt31:7 - Reserved >> + */ >> + cmd.enable = dsbr->dsbr & BIT(0); >> + if (cmd.enable) >> + cmd.dsbr = dsbr->dsbr >> 4 & 0xF; >> + kfree(dsbr); >> + } >> + >> + bt_dev_info(hdev, "dsbr: enabled: 0x%2.2x value: 0x%2.2x", >cmd.enable, >> + cmd.dsbr); >> + >> + skb = __hci_cmd_sync(hdev, 0xfc0a, sizeof(cmd), &cmd, >HCI_CMD_TIMEOUT); >> + if (IS_ERR(skb)) { >> + bt_dev_err(hdev, "Failed to send Intel DSBR command (%ld)", >> + PTR_ERR(skb)); >> + return -bt_to_errno(PTR_ERR(skb)); >> + } >> + >> + status = skb->data[0]; >> + kfree_skb(skb); >> + >> + if (status) { >> + bt_dev_err(hdev, "Set DSBR failed 0x%2.2x", status); >> + return -bt_to_errno(status); >> + } >> + return 0; >> +} >> + >> +static int btintel_apply_dsbr(struct hci_dev *hdev, >> + struct intel_version_tlv *ver) { >> + /* For BlazarI + B0 step, DSBR command needs to be sent just after >> + * downloading IML firmware > >Add the section of the datasheet? > >> + */ >> + return ver->img_type == BTINTEL_IMG_IML && >> + ((ver->cnvi_top & 0xfff) == BTINTEL_CNVI_BLAZARI) && >> + INTEL_CNVX_TOP_STEP(ver->cnvi_top) == 0x01; } >> + >> int btintel_bootloader_setup_tlv(struct hci_dev *hdev, >> struct intel_version_tlv *ver) >> { >> @@ -2649,6 +2757,12 @@ int btintel_bootloader_setup_tlv(struct hci_dev >*hdev, >> if (err) >> return err; >> >> + if (btintel_apply_dsbr(hdev, ver)) { >> + err = btintel_set_dsbr(hdev, ver); >> + if (err) >> + return err; >> + } >> + >> /* If image type returned is BTINTEL_IMG_IML, then controller >supports >> * intermediae loader image >> */ > > >Kind regards, > >Paul Thanks, Kiran
