Hi Arend,
>>>>>>>> Use btbcm helpers to perform controller setup.
>>>>>>>> Perform host UART reset to init speed between btbcm_patchram() and
>>>>>>>> btbcm_finalize(). This may be need because firmware loading may have
>>>>>>>> reseted controller UART to init speed.
>>>>>>>>
>>>>>>>> Signed-off-by: Frederic Danis<frederic.danis@xxxxxxxxxxxxxxx>
>>>>>>>> ---
>>>>>>>> drivers/bluetooth/hci_bcm.c | 19 ++++++++++++++++++-
>>>>>>>> 1 file changed, 18 insertions(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
>>>>>>>> index 1ec0b4a..cede445 100644
>>>>>>>> --- a/drivers/bluetooth/hci_bcm.c
>>>>>>>> +++ b/drivers/bluetooth/hci_bcm.c
>>>>>>>> @@ -79,11 +79,28 @@ static int bcm_flush(struct hci_uart *hu)
>>>>>>>>
>>>>>>>> static int bcm_setup(struct hci_uart *hu)
>>>>>>>> {
>>>>>>>> + char fw_name[64];
>>>>>>>> + int err;
>>>>>>>> +
>>>>>>>> BT_DBG("hu %p", hu);
>>>>>>>>
>>>>>>>> hu->hdev->set_bdaddr = btbcm_set_bdaddr;
>>>>>>>>
>>>>>>>> - return btbcm_setup_patchram(hu->hdev);
>>>>>>>> + err = btbcm_initialize(hu->hdev, fw_name, sizeof(fw_name));
>>>>>>>> + if (err)
>>>>>>>> + return err;
>>>>>>>> +
>>>>>>>> + err = btbcm_patchram(hu->hdev, fw_name);
>>>>>>>> + /* If there is no firmware (-ENOENT), discard the error and
>>>>>>>> continue */
>>>>>>>
>>>>>>> I guess -ENOENT means no firmware is required and not a
>>>>>>> request_firmware() failure. Not sure what is meant here.
>>>>>>>
>>>>>>> Regards,
>>>>>>> Arend
>>>>>>>
>>>>>>>> + if (err == -ENOENT)
>>>>>>>> + return 0;
>>>>>>>> +
>>>>>>>> + if (hu->proto->init_speed)
>>>>>>>> + hci_uart_set_baudrate(hu, hu->proto->init_speed);
>>>>>>>> +
>>>>>>>> + err = btbcm_finalize(hu->hdev);
>>>>>>>> +
>>>>>>>> + return err;
>>>>>>>> }
>>>>>>>>
>>>>>>>> static const struct h4_recv_pkt bcm_recv_pkts[] = {
>>>>>>
>>>>>> You're right, btbcm_patchram() return test does not work as I expected.
>>>>>>
>>>>>> I can change this by performing uart speed change only if it returns no
>>>>>> error.
>>>>>>
>>>>>> err = btbcm_patchram(hu->hdev, fw_name);
>>>>>> if (!err) {
>>>>>> /* Firmware loading may have reseted controller UART to init speed */
>>>>>> if (hu->proto->init_speed)
>>>>>> hci_uart_set_baudrate(hu, hu->proto->init_speed);
>>>>>> }
>>>>>>
>>>>>> err = btbcm_finalize(hu->hdev);
>>>>>>
>>>>>>
>>>>>> Or I can move request_firmware() out of btbcm_patchram() and test it
>>>>>> before calling btbcm_patchram(). This will imply to change
>>>>>> btbcm_patchram() to accept a firmware instead of firmware name.
>>>>>>
>>>>>> err = request_firmware(&fw, fw_name,&hu->hdev->dev);
>>>>>> if (err< 0) {
>>>>>> BT_INFO("%s: BCM: patch %s not found", hu->hdev->name, fw_name);
>>>>>> return 0;
>>>>>> }
>>>>>>
>>>>>> err = btbcm_patchram(hu->hdev, fw);
>>>>>> if (!err) {
>>>>>> /* Firmware loading may have reseted controller UART to init speed */
>>>>>> if (hu->proto->init_speed)
>>>>>> hci_uart_set_baudrate(hu, hu->proto->init_speed);
>>>>>> }
>>>>>>
>>>>>> err = btbcm_finalize(hu->hdev);
>>>>>>
>>>>>> Arend, Marcel, any advice regarding this ?
>>>>>
>>>>> Well, functionally it does not make a difference as the request_firmware
>>>>> is the first call done in btbcm_patchram(). So if it solves your problem
>>>>> I would go for option 2. Other question: is there a reason why the error
>>>>> code from request_firmware is not returned?
>>>>
>>>> If an error is returned it will stop Bluetooth setup.
>>>> So, when request_firmware() returns an error, the Bluetooth controller
>>>> will be used with current firmware and 0 is returned as bcm_setup is
>>>> completed.
>>>>
>>>> IF: There is generally no "current firmware" if the firmware download fails or not attempted. The device would run out of the ROM then which is not terribly useful if Bluetooth functionality is needed. Thanks, -Ilya
>>>
>>> The BCM4324B3 embedded in T100 is able to operate without firmware
>>> loading, at least performing Bluetooth discovery.
>>> I think it is better to run without firmware loading than not allowing
>>> to use Bluetooth controller at all.
>>>
>>> IF: Broadcom software elsewhere generally fails device start in this case as every Bluetooth feature may or may not work afterwards, with no guarantees whatsoever. The decision here is obviously up to BlueZ maintainers.
>>
>> I am fine with actually doing exactly that once we have a bit better infrastructure in place. However this also requires a commitment from Broadcom to actually publish the firmware files in linux-firmware tree and maintain the manifest document I have been talking about.
>
> What is the added value of having the manifest file? I can see it keeps mapping hardware info to firmware file out of the driver and as such keeps the driver from growing in size for new device support. Just wondering how big that issue is. It also poses other issues as it adds another interaction with the firmware api, which in my opinion is still a nasty api from driver perspective.
so a recent version of the Broadcom Bluetooth driver for Windows contains 167+ firmware files. Which means there are plenty of different files. I count at least 100+ individual USB dongles listed. Inside the btusb.c driver we started to match these devices with interface specific information.
/* Broadcom devices with vendor specific id */
{ USB_VENDOR_AND_INTERFACE_INFO(0x0a5c, 0xff, 0x01, 0x01),
.driver_info = BTUSB_BCM_PATCHRAM },
Otherwise we would just go insane to attempt to update the driver for every new dongle manufacture that comes along. This way this is limited to a handful big ones.
The request_firmware() API allows to builtin the file into the kernel image. While I would not do that for the actual firmware file itself, but for the manifest it might make sense. However keeping it outside and allowing update via linux-firmware package update seems beneficial for everybody.
Regards
Marcel
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
