| Message ID | 20240621064419.2185652-1-kiran.k@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] Bluetooth: btintel: Allow lowering of drive strength of BRI | expand |
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? > 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? … passes it *to the* controller … > specific command with opcode 0xfc0a. Please document your test system and also add the new log messages. > Signed-off-by: Kiran K <kiran.k@intel.com> > --- > 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? > +{ > + 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) > + 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
Hi Paul, Thanks for your comments. >-----Original Message----- >From: Paul Menzel <pmenzel@molgen.mpg.de> >Sent: Friday, June 21, 2024 12:46 PM >To: K, Kiran <kiran.k@intel.com> >Cc: Srivatsa, Ravishankar <ravishankar.srivatsa@intel.com>; Tumkur Narayan, >Chethan <chethan.tumkur.narayan@intel.com>; Devegowda, Chandrashekar ><chandrashekar.devegowda@intel.com>; Satija, Vijay <vijay.satija@intel.com>; >linux-bluetooth@vger.kernel.org >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@intel.com> >> --- >> 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
Hi Kiran, kernel test robot noticed the following build errors: [auto build test ERROR on bluetooth-next/master] [also build test ERROR on next-20240625] [cannot apply to bluetooth/master linus/master v6.10-rc5] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Kiran-K/Bluetooth-btintel-Allow-lowering-of-drive-strength-of-BRI/20240625-161151 base: https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git master patch link: https://lore.kernel.org/r/20240621064419.2185652-1-kiran.k%40intel.com patch subject: [PATCH v2] Bluetooth: btintel: Allow lowering of drive strength of BRI config: m68k-allmodconfig (https://download.01.org/0day-ci/archive/20240626/202406260659.fzB4yn3X-lkp@intel.com/config) compiler: m68k-linux-gcc (GCC) 13.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240626/202406260659.fzB4yn3X-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202406260659.fzB4yn3X-lkp@intel.com/ All errors (new ones prefixed by >>): drivers/bluetooth/btintel.c: In function 'btintel_uefi_get_variable': >> drivers/bluetooth/btintel.c:66:53: error: passing argument 4 of 'efi.get_variable' from incompatible pointer type [-Werror=incompatible-pointer-types] 66 | status = efi.get_variable(name, guid, NULL, &data_size, NULL); | ^~~~~~~~~~ | | | size_t * {aka unsigned int *} drivers/bluetooth/btintel.c:66:53: note: expected 'long unsigned int *' but argument is of type 'size_t *' {aka 'unsigned int *'} drivers/bluetooth/btintel.c:76:53: error: passing argument 4 of 'efi.get_variable' from incompatible pointer type [-Werror=incompatible-pointer-types] 76 | status = efi.get_variable(name, guid, NULL, &data_size, data); | ^~~~~~~~~~ | | | size_t * {aka unsigned int *} drivers/bluetooth/btintel.c:76:53: note: expected 'long unsigned int *' but argument is of type 'size_t *' {aka 'unsigned int *'} cc1: some warnings being treated as errors vim +66 drivers/bluetooth/btintel.c 49 50 static const guid_t btintel_guid_dsm = 51 GUID_INIT(0xaa10f4e0, 0x81ac, 0x4233, 52 0xab, 0xf6, 0x3b, 0x2a, 0xc5, 0x0e, 0x28, 0xd9); 53 54 static void *btintel_uefi_get_variable(efi_char16_t *name, efi_guid_t *guid) 55 { 56 void *data; 57 efi_status_t status; 58 size_t data_size = 0; 59 60 if (!IS_ENABLED(CONFIG_EFI)) 61 return ERR_PTR(-EOPNOTSUPP); 62 63 if (!efi_rt_services_supported(EFI_RT_SUPPORTED_GET_VARIABLE)) 64 return ERR_PTR(-EOPNOTSUPP); 65 > 66 status = efi.get_variable(name, guid, NULL, &data_size, NULL); 67 68 if (status != EFI_BUFFER_TOO_SMALL || !data_size) 69 return ERR_PTR(-EIO); 70 71 data = kmalloc(data_size, GFP_KERNEL); 72 73 if (!data) 74 return ERR_PTR(-ENOMEM); 75 76 status = efi.get_variable(name, guid, NULL, &data_size, data); 77 78 if (status != EFI_SUCCESS) { 79 kfree(data); 80 return ERR_PTR(-ENXIO); 81 } 82 83 return data; 84 } 85
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) +{ + 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)", + 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 + */ + 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 */
BRI (Bluetooth Radio Interface) traffic from CNVr to CNVi was found causing cross talk step errors to WiFi. 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 specific command with opcode 0xfc0a. Signed-off-by: Kiran K <kiran.k@intel.com> --- drivers/bluetooth/btintel.c | 114 ++++++++++++++++++++++++++++++++++++ 1 file changed, 114 insertions(+)