Return-Path: Content-Type: text/plain; charset=us-ascii Mime-Version: 1.0 (Mac OS X Mail 7.2 \(1874\)) Subject: Re: [PATCH] Bluetooth: btusb: Add Broadcom patch RAM support From: Marcel Holtmann In-Reply-To: <20140507223541.387FE1005DE@puck.mtv.corp.google.com> Date: Thu, 8 May 2014 13:29:59 -0700 Cc: linux-bluetooth@vger.kernel.org Message-Id: References: <20140507223541.387FE1005DE@puck.mtv.corp.google.com> To: Petri Gynther Sender: linux-bluetooth-owner@vger.kernel.org List-ID: Hi Petri, > After hardware reset, some BCM Bluetooth adapters obtain their initial firmware > from OTPROM chip. Once this initial firmware is running, the firmware can be > further upgraded over HCI interface with .hcd files provided by Broadcom. This > is also known as "patch RAM" support. This change implements that. > > If the .hcd file is not found in /lib/firmware, BCM Bluetooth adapter continues > to operate with the initial firmware. Sample kernel log: > hotplug: sys=firmware act=add fw=brcm/BCM20702A0-0a5c-22be.hcd dev=... > Bluetooth: hci0: BCM: patch brcm/BCM20702A0-0a5c-22be.hcd not found > Bluetooth: hci0: BCM: firmware hci_ver=06 hci_rev=1000 lmp_ver=06 lmp_subver=220e > > If the .hcd file is found, btusb driver pushes it to the BCM Bluetooth adapter and > it starts using the new firmware. Sample kernel log: > hotplug: sys=firmware act=add fw=brcm/BCM20702A0-0a5c-22be.hcd dev=... > Bluetooth: hci0: BCM: patching hci_ver=06 hci_rev=1000 lmp_ver=06 lmp_subver=220e > Bluetooth: hci0: BCM: firmware hci_ver=06 hci_rev=1389 lmp_ver=06 lmp_subver=220e > > Above, we can see that hci_rev goes from 1000 to 1389 as a result of the upgrade. > > Signed-off-by: Petri Gynther > --- > drivers/bluetooth/btusb.c | 158 +++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 157 insertions(+), 1 deletion(-) > > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c > index f338b0c..1b3e514 100644 > --- a/drivers/bluetooth/btusb.c > +++ b/drivers/bluetooth/btusb.c > @@ -49,6 +49,7 @@ static struct usb_driver btusb_driver; > #define BTUSB_WRONG_SCO_MTU 0x40 > #define BTUSB_ATH3012 0x80 > #define BTUSB_INTEL 0x100 > +#define BTUSB_BCM_PATCHRAM 0x200 > > static const struct usb_device_id btusb_table[] = { > /* Generic Bluetooth USB device */ > @@ -111,7 +112,8 @@ static const struct usb_device_id btusb_table[] = { > { USB_VENDOR_AND_INTERFACE_INFO(0x0489, 0xff, 0x01, 0x01) }, > > /* Broadcom devices with vendor specific id */ > - { USB_VENDOR_AND_INTERFACE_INFO(0x0a5c, 0xff, 0x01, 0x01) }, > + { USB_VENDOR_AND_INTERFACE_INFO(0x0a5c, 0xff, 0x01, 0x01), > + .driver_info = BTUSB_BCM_PATCHRAM }, > > /* Belkin F8065bf - Broadcom based */ > { USB_VENDOR_AND_INTERFACE_INFO(0x050d, 0xff, 0x01, 0x01) }, > @@ -1380,6 +1382,157 @@ exit_mfg_deactivate: > return 0; > } > > +static int btusb_setup_bcm_patchram(struct hci_dev *hdev) > +{ > + struct btusb_data *data = hci_get_drvdata(hdev); > + struct usb_device *udev = data->udev; > + char fw_name[64]; > + const struct firmware *fw; > + const u8 *fw_ptr; > + size_t fw_size; > + const struct hci_command_hdr *cmd; > + const u8 *cmd_param; > + u16 opcode; > + struct sk_buff *skb; > + struct hci_rp_read_local_version *ver; > + long ret; > + > + snprintf(fw_name, sizeof(fw_name), "brcm/%s-%04x-%04x.hcd", > + udev->product ? udev->product : "BCM", > + le16_to_cpu(udev->descriptor.idVendor), > + le16_to_cpu(udev->descriptor.idProduct)); > + > + ret = request_firmware(&fw, fw_name, &hdev->dev); > + if (ret < 0) { > + BT_INFO("%s: BCM: patch %s not found", hdev->name, > + fw_name); > + ret = 0; > + goto get_fw_ver; > + } why are we doing this? Is there a point in reading the HCI version when we do not find the firmware? Can we not just exit here. The local version will be read from init anyway and is available via HCI and also hciconfig and even debugfs. I see no point in wasting time in reading something twice if we do not have the firmware file. > + > + /* Reset */ > + skb = __hci_cmd_sync(hdev, HCI_OP_RESET, 0, NULL, HCI_INIT_TIMEOUT); > + if (IS_ERR(skb)) { > + ret = PTR_ERR(skb); > + BT_ERR("%s: HCI_OP_RESET failed (%ld)", hdev->name, ret); > + goto reset_fw; This one I also not get. If the HCI_Reset fails, what makes you think a second one will succeed. In this case I would just return with an error. > + } > + kfree_skb(skb); > + > + /* Read Local Version Info */ > + skb = __hci_cmd_sync(hdev, HCI_OP_READ_LOCAL_VERSION, 0, NULL, > + HCI_INIT_TIMEOUT); > + if (IS_ERR(skb)) { > + ret = PTR_ERR(skb); > + BT_ERR("%s: HCI_OP_READ_LOCAL_VERSION failed (%ld)", > + hdev->name, ret); > + goto reset_fw; Same here. If it fails, we have problems. Just return an error. > + } > + > + if (skb->len != sizeof(*ver)) { > + BT_ERR("%s: HCI_OP_READ_LOCAL_VERSION event length mismatch", > + hdev->name); > + kfree_skb(skb); > + ret = -EIO; > + goto reset_fw; I would also do the same here. Since I do not see how any chip could recover with a HCI_Reset if this happens. > + } > + > + ver = (struct hci_rp_read_local_version *) skb->data; > + BT_INFO("%s: BCM: patching hci_ver=%02x hci_rev=%04x lmp_ver=%02x " > + "lmp_subver=%04x", hdev->name, ver->hci_ver, ver->hci_rev, > + ver->lmp_ver, ver->lmp_subver); > + kfree_skb(skb); > + > + /* Start Download */ > + skb = __hci_cmd_sync(hdev, 0xfc2e, 0, NULL, HCI_INIT_TIMEOUT); > + if (IS_ERR(skb)) { > + ret = PTR_ERR(skb); > + BT_ERR("%s: BCM: Download Minidrv command failed (%ld)", > + hdev->name, ret); > + goto reset_fw; > + } > + kfree_skb(skb); > + > + /* 50 msec delay after Download Minidrv completes */ > + msleep(50); > + > + fw_ptr = fw->data; > + fw_size = fw->size; > + > + while (fw_size >= sizeof(*cmd)) { > + cmd = (struct hci_command_hdr *) fw_ptr; > + fw_ptr += sizeof(*cmd); > + fw_size -= sizeof(*cmd); > + > + if (fw_size < cmd->plen) { > + BT_ERR("%s: BCM: patch %s is corrupted", > + hdev->name, fw_name); > + ret = -EINVAL; > + goto reset_fw; > + } > + > + cmd_param = fw_ptr; > + fw_ptr += cmd->plen; > + fw_size -= cmd->plen; > + > + opcode = le16_to_cpu(cmd->opcode); > + > + skb = __hci_cmd_sync(hdev, opcode, cmd->plen, cmd_param, > + HCI_INIT_TIMEOUT); > + if (IS_ERR(skb)) { > + ret = PTR_ERR(skb); > + BT_ERR("%s: BCM: patch command %04x failed (%ld)", > + hdev->name, opcode, ret); > + goto reset_fw; > + } > + kfree_skb(skb); > + } > + > + /* 250 msec delay after Launch Ram completes */ > + msleep(250); > + > +reset_fw: I would free the firmware data right here. Then you do not have to worry about it anymore. > + /* Reset */ > + skb = __hci_cmd_sync(hdev, HCI_OP_RESET, 0, NULL, HCI_INIT_TIMEOUT); > + if (IS_ERR(skb)) { > + ret = PTR_ERR(skb); > + BT_ERR("%s: HCI_OP_RESET failed (%ld)", hdev->name, ret); > + goto done; If you free the firmware data above, then this can just become a return ret. > + } > + kfree_skb(skb); > + > +get_fw_ver: > + /* Read Local Version Info */ > + skb = __hci_cmd_sync(hdev, HCI_OP_READ_LOCAL_VERSION, 0, NULL, > + HCI_INIT_TIMEOUT); > + if (IS_ERR(skb)) { > + ret = PTR_ERR(skb); > + BT_ERR("%s: HCI_OP_READ_LOCAL_VERSION failed (%ld)", > + hdev->name, ret); > + goto done; Same here. Just return. > + } > + > + if (skb->len != sizeof(*ver)) { > + BT_ERR("%s: HCI_OP_READ_LOCAL_VERSION event length mismatch", > + hdev->name); > + kfree_skb(skb); > + ret = -EIO; > + goto done; And here as well. > + } > + > + ver = (struct hci_rp_read_local_version *) skb->data; > + BT_INFO("%s: BCM: firmware hci_ver=%02x hci_rev=%04x lmp_ver=%02x " > + "lmp_subver=%04x", hdev->name, ver->hci_ver, ver->hci_rev, > + ver->lmp_ver, ver->lmp_subver); > + kfree_skb(skb); > + > +done: > + if (fw) > + release_firmware(fw); > + > + return ret; > +} > + > static int btusb_probe(struct usb_interface *intf, > const struct usb_device_id *id) > { > @@ -1485,6 +1638,9 @@ static int btusb_probe(struct usb_interface *intf, > if (id->driver_info & BTUSB_BCM92035) > hdev->setup = btusb_setup_bcm92035; > > + if (id->driver_info & BTUSB_BCM_PATCHRAM) > + hdev->setup = btusb_setup_bcm_patchram; > + > if (id->driver_info & BTUSB_INTEL) { > usb_enable_autosuspend(data->udev); > hdev->setup = btusb_setup_intel; The changes are all cosmetic, but I prefer to have them done before applying the patch. I need to also test run this by myself and see what impact this has on all the devices we do not have firmware for. Regards Marcel