2017-02-07 12:53:42

by Jonas Holmberg

[permalink] [raw]
Subject: [PATCH] Bluetooth: hci_bcm: Do not change uart speed

From: Jonas Holmberg <[email protected]>

Do not change from init_speed to oper_speed per default since it may
cause failures and timeouts if the uart cannot handle the new speed.
orig_speed should probably be set using device tree instead.
---
drivers/bluetooth/hci_bcm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 8f6c23c..7787f82 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -809,7 +809,7 @@ static const struct hci_uart_proto bcm_proto = {
.name = "Broadcom",
.manufacturer = 15,
.init_speed = 115200,
- .oper_speed = 4000000,
+ .oper_speed = 0,
.open = bcm_open,
.close = bcm_close,
.flush = bcm_flush,
--
2.10.2



2017-02-13 09:30:02

by Marcel Holtmann

[permalink] [raw]
Subject: Re: [PATCH] Bluetooth: hci_bcm: Do not change uart speed

Hi Jonas,

>>>> Do not change from init_speed to oper_speed per default since it may
>>>> cause failures and timeouts if the uart cannot handle the new speed.
>>>> orig_speed should probably be set using device tree instead.
>>>> ---
>>>> drivers/bluetooth/hci_bcm.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
>>>> index 8f6c23c..7787f82 100644
>>>> --- a/drivers/bluetooth/hci_bcm.c
>>>> +++ b/drivers/bluetooth/hci_bcm.c
>>>> @@ -809,7 +809,7 @@ static const struct hci_uart_proto bcm_proto = {
>>>> .name = "Broadcom",
>>>> .manufacturer = 15,
>>>> .init_speed = 115200,
>>>> - .oper_speed = 4000000,
>>>> + .oper_speed = 0,
>>>> .open = bcm_open,
>>>> .close = bcm_close,
>>>> .flush = bcm_flush,
>>> I am generally fine with this, but the DT support for Broadcom UART
>>> devices is not yet upstream. And we need to make sure this also works
>>> for ACPI based devices. If we can get the max UART speed from ACPI,
>>> then I am fine doing it this way.
>>
>> Afair, the max UART speed is nor provided from ACPI, only the initial
>> speed is provided, at least for the Asus T100TA.
>
> Does that mean that I will need to patch the kernel for uarts that does
> not support 4000000 baud until the DT patches have been merged, or is
> there anything else that I can do?
>
> It would be nice if this was a bit more obvious for the user somehow. I
> had to use git bisect to figure out why I got random errors and timeouts
> after upgrading to kernel version >= 4.2.

that is a good question and I do not know a clear answer to that. We have to set it to some value. Running these with the default speed of 115200 works, but is also not really acceptable if anybody wants to do A2DP streaming. So what value to pick?

Maybe we can do some ACPI overlay or DT overlay over ACPI to allow setting this externally. But I would have to refer to the ACPI and DT experts on this.

Regards

Marcel


2017-02-13 08:05:35

by Jonas Holmberg

[permalink] [raw]
Subject: Re: [PATCH] Bluetooth: hci_bcm: Do not change uart speed

Hi,

On 02/10/2017 01:01 PM, Fr?d?ric Danis wrote:
> Hello,
>
> Le 10/02/2017 ? 12:41, Marcel Holtmann a ?crit :
>> Hi Jonas,
>>
>>> Do not change from init_speed to oper_speed per default since it may
>>> cause failures and timeouts if the uart cannot handle the new speed.
>>> orig_speed should probably be set using device tree instead.
>>> ---
>>> drivers/bluetooth/hci_bcm.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
>>> index 8f6c23c..7787f82 100644
>>> --- a/drivers/bluetooth/hci_bcm.c
>>> +++ b/drivers/bluetooth/hci_bcm.c
>>> @@ -809,7 +809,7 @@ static const struct hci_uart_proto bcm_proto = {
>>> .name = "Broadcom",
>>> .manufacturer = 15,
>>> .init_speed = 115200,
>>> - .oper_speed = 4000000,
>>> + .oper_speed = 0,
>>> .open = bcm_open,
>>> .close = bcm_close,
>>> .flush = bcm_flush,
>> I am generally fine with this, but the DT support for Broadcom UART
>> devices is not yet upstream. And we need to make sure this also works
>> for ACPI based devices. If we can get the max UART speed from ACPI,
>> then I am fine doing it this way.
>
> Afair, the max UART speed is nor provided from ACPI, only the initial
> speed is provided, at least for the Asus T100TA.

Does that mean that I will need to patch the kernel for uarts that does
not support 4000000 baud until the DT patches have been merged, or is
there anything else that I can do?

It would be nice if this was a bit more obvious for the user somehow. I
had to use git bisect to figure out why I got random errors and timeouts
after upgrading to kernel version >= 4.2.

Regards
Jonas

2017-02-10 12:01:05

by Frédéric Danis

[permalink] [raw]
Subject: Re: [PATCH] Bluetooth: hci_bcm: Do not change uart speed

Hello,

Le 10/02/2017 ? 12:41, Marcel Holtmann a ?crit :
> Hi Jonas,
>
>> Do not change from init_speed to oper_speed per default since it may
>> cause failures and timeouts if the uart cannot handle the new speed.
>> orig_speed should probably be set using device tree instead.
>> ---
>> drivers/bluetooth/hci_bcm.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
>> index 8f6c23c..7787f82 100644
>> --- a/drivers/bluetooth/hci_bcm.c
>> +++ b/drivers/bluetooth/hci_bcm.c
>> @@ -809,7 +809,7 @@ static const struct hci_uart_proto bcm_proto = {
>> .name = "Broadcom",
>> .manufacturer = 15,
>> .init_speed = 115200,
>> - .oper_speed = 4000000,
>> + .oper_speed = 0,
>> .open = bcm_open,
>> .close = bcm_close,
>> .flush = bcm_flush,
> I am generally fine with this, but the DT support for Broadcom UART devices is not yet upstream. And we need to make sure this also works for ACPI based devices. If we can get the max UART speed from ACPI, then I am fine doing it this way.

Afair, the max UART speed is nor provided from ACPI, only the initial
speed is provided, at least for the Asus T100TA.

Regards,

Fr?d?ric

2017-02-10 11:41:09

by Marcel Holtmann

[permalink] [raw]
Subject: Re: [PATCH] Bluetooth: hci_bcm: Do not change uart speed

Hi Jonas,

> Do not change from init_speed to oper_speed per default since it may
> cause failures and timeouts if the uart cannot handle the new speed.
> orig_speed should probably be set using device tree instead.
> ---
> drivers/bluetooth/hci_bcm.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
> index 8f6c23c..7787f82 100644
> --- a/drivers/bluetooth/hci_bcm.c
> +++ b/drivers/bluetooth/hci_bcm.c
> @@ -809,7 +809,7 @@ static const struct hci_uart_proto bcm_proto = {
> .name = "Broadcom",
> .manufacturer = 15,
> .init_speed = 115200,
> - .oper_speed = 4000000,
> + .oper_speed = 0,
> .open = bcm_open,
> .close = bcm_close,
> .flush = bcm_flush,

I am generally fine with this, but the DT support for Broadcom UART devices is not yet upstream. And we need to make sure this also works for ACPI based devices. If we can get the max UART speed from ACPI, then I am fine doing it this way.

Regards

Marcel