diff mbox series

[4/4] Bluetooth: hci_bcm: Increase host baudrate for CYW55572 in autobaud mode

Message ID 386b205422099c795272ad8b792091b692def3cd.1655723462.git.hakan.jansson@infineon.com
State Superseded
Headers show
Series Bluetooth: hci_bcm: Improve FW load time on CYW55572 | expand

Commit Message

Hakan Jansson June 20, 2022, 12:01 p.m. UTC
Add device specific data for max baudrate in autobaud mode. This allows the
host to use a baudrate higher than "init speed" when loading FW in autobaud
mode.

Signed-off-by: Hakan Jansson <hakan.jansson@infineon.com>
---
 drivers/bluetooth/hci_bcm.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

Comments

Paul Menzel June 20, 2022, 12:21 p.m. UTC | #1
[Your To: header field has:

     To:     unlisted-recipients:; (no To-header on input)

which is added to the receiver list when replying to all in Mozilla 
Thunderbird 91.10.0.
]


Dear Hakan,


Am 20.06.22 um 14:01 schrieb Hakan Jansson:
> Add device specific data for max baudrate in autobaud mode. This allows the
> host to use a baudrate higher than "init speed" when loading FW in autobaud
> mode.

Please mention 921600 in the commit message, and maybe also document 
what the current default is.

Please also add the measurement data to the commit message, that means, 
how much is the time to load the firmware decreased.

> 
> Signed-off-by: Hakan Jansson <hakan.jansson@infineon.com>
> ---
>   drivers/bluetooth/hci_bcm.c | 20 ++++++++++++++++----
>   1 file changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
> index 0ae627c293c5..d7e0b75db8a6 100644
> --- a/drivers/bluetooth/hci_bcm.c
> +++ b/drivers/bluetooth/hci_bcm.c
> @@ -53,10 +53,12 @@
>    * struct bcm_device_data - device specific data
>    * @no_early_set_baudrate: Disallow set baudrate before driver setup()
>    * @drive_rts_on_open: drive RTS signal on ->open() when platform requires it
> + * @max_autobaud_speed: max baudrate supported by device in autobaud mode
>    */
>   struct bcm_device_data {
>   	bool	no_early_set_baudrate;
>   	bool	drive_rts_on_open;
> +	u32	max_autobaud_speed;

Why specify the length, and not just `unsigned int`? Maybe also add the 
unit to the variable name?

>   };
>   
>   /**
> @@ -100,6 +102,7 @@ struct bcm_device_data {
>    * @drive_rts_on_open: drive RTS signal on ->open() when platform requires it
>    * @pcm_int_params: keep the initial PCM configuration
>    * @use_autobaud_mode: start Bluetooth device in autobaud mode
> + * @max_autobaud_speed: max baudrate supported by device in autobaud mode
>    */
>   struct bcm_device {
>   	/* Must be the first member, hci_serdev.c expects this. */
> @@ -139,6 +142,7 @@ struct bcm_device {
>   	bool			drive_rts_on_open;
>   	bool			use_autobaud_mode;
>   	u8			pcm_int_params[5];
> +	u32			max_autobaud_speed;

Ditto.

>   };
>   
>   /* generic bcm uart resources */
> @@ -479,7 +483,10 @@ static int bcm_open(struct hci_uart *hu)
>   		else if (bcm->dev->drive_rts_on_open)
>   			hci_uart_set_flow_control(hu, true);
>   
> -		hu->init_speed = bcm->dev->init_speed;
> +		if (bcm->dev->use_autobaud_mode && bcm->dev->max_autobaud_speed)
> +			hu->init_speed = min(bcm->dev->oper_speed, bcm->dev->max_autobaud_speed);
> +		else
> +			hu->init_speed = bcm->dev->init_speed;
>   
>   		/* If oper_speed is set, ldisc/serdev will set the baudrate
>   		 * before calling setup()
> @@ -585,8 +592,8 @@ static int bcm_setup(struct hci_uart *hu)
>   		return 0;
>   
>   	/* Init speed if any */
> -	if (hu->init_speed)
> -		speed = hu->init_speed;
> +	if (bcm->dev && bcm->dev->init_speed)
> +		speed = bcm->dev->init_speed;
>   	else if (hu->proto->init_speed)
>   		speed = hu->proto->init_speed;
>   	else
> @@ -1519,6 +1526,7 @@ static int bcm_serdev_probe(struct serdev_device *serdev)
>   
>   	data = device_get_match_data(bcmdev->dev);
>   	if (data) {
> +		bcmdev->max_autobaud_speed = data->max_autobaud_speed;
>   		bcmdev->no_early_set_baudrate = data->no_early_set_baudrate;
>   		bcmdev->drive_rts_on_open = data->drive_rts_on_open;
>   	}
> @@ -1542,6 +1550,10 @@ static struct bcm_device_data bcm43438_device_data = {
>   	.drive_rts_on_open = true,
>   };
>   
> +static struct bcm_device_data cyw55572_device_data = {
> +	.max_autobaud_speed = 921600,
> +};
> +
>   static const struct of_device_id bcm_bluetooth_of_match[] = {
>   	{ .compatible = "brcm,bcm20702a1" },
>   	{ .compatible = "brcm,bcm4329-bt" },
> @@ -1554,7 +1566,7 @@ static const struct of_device_id bcm_bluetooth_of_match[] = {
>   	{ .compatible = "brcm,bcm4349-bt", .data = &bcm43438_device_data },
>   	{ .compatible = "brcm,bcm43540-bt", .data = &bcm4354_device_data },
>   	{ .compatible = "brcm,bcm4335a0" },
> -	{ .compatible = "infineon,cyw55572-bt" },
> +	{ .compatible = "infineon,cyw55572-bt", .data = &cyw55572_device_data },
>   	{ },
>   };
>   MODULE_DEVICE_TABLE(of, bcm_bluetooth_of_match);


Kind regards,

Paul
Hakan Jansson June 21, 2022, 5:15 p.m. UTC | #2
Hi Paul,

On 6/20/2022 2:21 PM, Paul Menzel wrote:
>> Add device specific data for max baudrate in autobaud mode. This 
>> allows the
>> host to use a baudrate higher than "init speed" when loading FW in 
>> autobaud
>> mode.
>
> Please mention 921600 in the commit message, and maybe also document
> what the current default is.

Sure, I can do that if I submit a new rev. The default is 115200.

> Please also add the measurement data to the commit message, that means,
> how much is the time to load the firmware decreased.

The actual load time will depend on the specific FW used but I could add 
an example. It would be in the order of seconds.

>> Signed-off-by: Hakan Jansson <hakan.jansson@infineon.com>
>> ---
>>   drivers/bluetooth/hci_bcm.c | 20 ++++++++++++++++----
>>   1 file changed, 16 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
>> index 0ae627c293c5..d7e0b75db8a6 100644
>> --- a/drivers/bluetooth/hci_bcm.c
>> +++ b/drivers/bluetooth/hci_bcm.c
>> @@ -53,10 +53,12 @@
>>    * struct bcm_device_data - device specific data
>>    * @no_early_set_baudrate: Disallow set baudrate before driver setup()
>>    * @drive_rts_on_open: drive RTS signal on ->open() when platform 
>> requires it
>> + * @max_autobaud_speed: max baudrate supported by device in autobaud 
>> mode
>>    */
>>   struct bcm_device_data {
>>       bool    no_early_set_baudrate;
>>       bool    drive_rts_on_open;
>> +     u32     max_autobaud_speed;
>
> Why specify the length, and not just `unsigned int`? Maybe also add the
> unit to the variable name?

See below.

>>   };
>>
>>   /**
>> @@ -100,6 +102,7 @@ struct bcm_device_data {
>>    * @drive_rts_on_open: drive RTS signal on ->open() when platform 
>> requires it
>>    * @pcm_int_params: keep the initial PCM configuration
>>    * @use_autobaud_mode: start Bluetooth device in autobaud mode
>> + * @max_autobaud_speed: max baudrate supported by device in autobaud 
>> mode
>>    */
>>   struct bcm_device {
>>       /* Must be the first member, hci_serdev.c expects this. */
>> @@ -139,6 +142,7 @@ struct bcm_device {
>>       bool                    drive_rts_on_open;
>>       bool                    use_autobaud_mode;
>>       u8                      pcm_int_params[5];
>> +     u32                     max_autobaud_speed;
>
> Ditto.

I'm trying to following the style of the existing code which already had 
struct members "oper_speed" and "init_speed" declared as u32.


Regards,
Håkan
diff mbox series

Patch

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 0ae627c293c5..d7e0b75db8a6 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -53,10 +53,12 @@ 
  * struct bcm_device_data - device specific data
  * @no_early_set_baudrate: Disallow set baudrate before driver setup()
  * @drive_rts_on_open: drive RTS signal on ->open() when platform requires it
+ * @max_autobaud_speed: max baudrate supported by device in autobaud mode
  */
 struct bcm_device_data {
 	bool	no_early_set_baudrate;
 	bool	drive_rts_on_open;
+	u32	max_autobaud_speed;
 };
 
 /**
@@ -100,6 +102,7 @@  struct bcm_device_data {
  * @drive_rts_on_open: drive RTS signal on ->open() when platform requires it
  * @pcm_int_params: keep the initial PCM configuration
  * @use_autobaud_mode: start Bluetooth device in autobaud mode
+ * @max_autobaud_speed: max baudrate supported by device in autobaud mode
  */
 struct bcm_device {
 	/* Must be the first member, hci_serdev.c expects this. */
@@ -139,6 +142,7 @@  struct bcm_device {
 	bool			drive_rts_on_open;
 	bool			use_autobaud_mode;
 	u8			pcm_int_params[5];
+	u32			max_autobaud_speed;
 };
 
 /* generic bcm uart resources */
@@ -479,7 +483,10 @@  static int bcm_open(struct hci_uart *hu)
 		else if (bcm->dev->drive_rts_on_open)
 			hci_uart_set_flow_control(hu, true);
 
-		hu->init_speed = bcm->dev->init_speed;
+		if (bcm->dev->use_autobaud_mode && bcm->dev->max_autobaud_speed)
+			hu->init_speed = min(bcm->dev->oper_speed, bcm->dev->max_autobaud_speed);
+		else
+			hu->init_speed = bcm->dev->init_speed;
 
 		/* If oper_speed is set, ldisc/serdev will set the baudrate
 		 * before calling setup()
@@ -585,8 +592,8 @@  static int bcm_setup(struct hci_uart *hu)
 		return 0;
 
 	/* Init speed if any */
-	if (hu->init_speed)
-		speed = hu->init_speed;
+	if (bcm->dev && bcm->dev->init_speed)
+		speed = bcm->dev->init_speed;
 	else if (hu->proto->init_speed)
 		speed = hu->proto->init_speed;
 	else
@@ -1519,6 +1526,7 @@  static int bcm_serdev_probe(struct serdev_device *serdev)
 
 	data = device_get_match_data(bcmdev->dev);
 	if (data) {
+		bcmdev->max_autobaud_speed = data->max_autobaud_speed;
 		bcmdev->no_early_set_baudrate = data->no_early_set_baudrate;
 		bcmdev->drive_rts_on_open = data->drive_rts_on_open;
 	}
@@ -1542,6 +1550,10 @@  static struct bcm_device_data bcm43438_device_data = {
 	.drive_rts_on_open = true,
 };
 
+static struct bcm_device_data cyw55572_device_data = {
+	.max_autobaud_speed = 921600,
+};
+
 static const struct of_device_id bcm_bluetooth_of_match[] = {
 	{ .compatible = "brcm,bcm20702a1" },
 	{ .compatible = "brcm,bcm4329-bt" },
@@ -1554,7 +1566,7 @@  static const struct of_device_id bcm_bluetooth_of_match[] = {
 	{ .compatible = "brcm,bcm4349-bt", .data = &bcm43438_device_data },
 	{ .compatible = "brcm,bcm43540-bt", .data = &bcm4354_device_data },
 	{ .compatible = "brcm,bcm4335a0" },
-	{ .compatible = "infineon,cyw55572-bt" },
+	{ .compatible = "infineon,cyw55572-bt", .data = &cyw55572_device_data },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, bcm_bluetooth_of_match);