Re: [PATCH v9 4/7] Bluetooth: hci_qca: Add wrapper functions for setting UART speed

From: Balakrishna Godavarthi
Date: Tue Jul 10 2018 - 08:19:29 EST

Hi Matthias,

On 2018-07-07 01:10, Matthias Kaehlcke wrote:
On Thu, Jul 05, 2018 at 10:25:12PM +0530, Balakrishna Godavarthi wrote:
In function qca_setup, we set initial and operating speeds for Qualcomm
Bluetooth SoC's. This block of code is common across different
Qualcomm Bluetooth SoC's. Instead of duplicating the code, created
a wrapper function to set the speeds. So that future coming SoC's
can use these wrapper functions to set speeds.

Signed-off-by: Balakrishna Godavarthi <bgodavar@xxxxxxxxxxxxxx>
---
Changes in v9:
* added corner case to fail driver if speeds are missing.

Changes in v8:
* common function to set INIT and operating speeds.

Changes in v7:
* initial patch
* created wrapper functions for init and operating speeds.
---
drivers/bluetooth/hci_qca.c | 94 ++++++++++++++++++++++++++++---------
1 file changed, 71 insertions(+), 23 deletions(-)

diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index c02e1d465cca..9106547324fa 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -119,6 +119,11 @@ struct qca_data {
u64 votes_off;
};

+enum qca_speed_type {
+ QCA_INIT_SPEED = 1,
+ QCA_OPER_SPEED
+};
+
struct qca_serdev {
struct hci_uart serdev_hu;
struct gpio_desc *bt_en;
@@ -923,6 +928,62 @@ static inline void host_set_baudrate(struct hci_uart *hu, unsigned int speed)
hci_uart_set_baudrate(hu, speed);
}

+static unsigned int qca_get_speed(struct hci_uart *hu,
+ enum qca_speed_type speed_type)
+{
+ unsigned int speed = 0;
+
+ if (speed_type == QCA_INIT_SPEED) {
+ if (hu->init_speed)
+ speed = hu->init_speed;
+ else if (hu->proto->init_speed)
+ speed = hu->proto->init_speed;
+ } else {
+ if (hu->oper_speed)
+ speed = hu->oper_speed;
+ else if (hu->proto->oper_speed)
+ speed = hu->proto->oper_speed;
+ }
+
+ return speed;
+}
+
+static int qca_check_speeds(struct hci_uart *hu)
+{
+ /* One or the other speeds should be non zero. */
+ if (!qca_get_speed(hu, QCA_INIT_SPEED) &&
+ !qca_get_speed(hu, QCA_OPER_SPEED))
+ return -EINVAL;
+
+ return 0;
+}
+
+static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
+{
+ unsigned int speed, qca_baudrate;
+ int ret;
+
+ if (speed_type == QCA_INIT_SPEED) {
+ speed = qca_get_speed(hu, QCA_INIT_SPEED);
+ if (speed)
+ host_set_baudrate(hu, speed);

mega-nit: for consistency with the 'else' branch you could return if
'speed == 0'. Not important though, feel free to ignore.

+ } else {
+ speed = qca_get_speed(hu, QCA_OPER_SPEED);
+ if (!speed)
+ return 0;
+
+ qca_baudrate = qca_get_baudrate_value(speed);
+ bt_dev_info(hu->hdev, "Set UART speed to %d", speed);
+ ret = qca_set_baudrate(hu->hdev, qca_baudrate);
+ if (ret)
+ return ret;
+
+ host_set_baudrate(hu, speed);
+ }
+
+ return 0;
+}
+
static int qca_setup(struct hci_uart *hu)
{
struct hci_dev *hdev = hu->hdev;
@@ -933,37 +994,24 @@ static int qca_setup(struct hci_uart *hu)

bt_dev_info(hdev, "ROME setup");

+ ret = qca_check_speeds(hu);
+ if (ret)
+ return ret;
+
/* Patch downloading has to be done without IBS mode */
clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);

/* Setup initial baudrate */
- speed = 0;
- if (hu->init_speed)
- speed = hu->init_speed;
- else if (hu->proto->init_speed)
- speed = hu->proto->init_speed;
-
- if (speed)
- host_set_baudrate(hu, speed);
+ qca_set_speed(hu, QCA_INIT_SPEED);

/* Setup user speed if needed */
- speed = 0;
- if (hu->oper_speed)
- speed = hu->oper_speed;
- else if (hu->proto->oper_speed)
- speed = hu->proto->oper_speed;
-
+ speed = qca_get_speed(hu, QCA_OPER_SPEED);
if (speed) {
- qca_baudrate = qca_get_baudrate_value(speed);
-
- bt_dev_info(hdev, "Set UART speed to %d", speed);
- ret = qca_set_baudrate(hdev, qca_baudrate);
- if (ret) {
- bt_dev_err(hdev, "Failed to change the baud rate (%d)",
- ret);
+ ret = qca_set_speed(hu, QCA_OPER_SPEED);
+ if (ret)
return ret;
- }
- host_set_baudrate(hu, speed);
+
+ qca_baudrate = qca_get_baudrate_value(speed);
}

One doubt, the outcome of this change is:

qca_set_speed(hu, QCA_INIT_SPEED);

/* Setup user speed if needed */
speed = qca_get_speed(hu, QCA_OPER_SPEED);
if (speed) {
ret = qca_set_speed(hu, QCA_OPER_SPEED);
if (ret)
return ret;

qca_baudrate = qca_get_baudrate_value(speed);
}

So we set the init speed and then directly switch to operating speed
if it is defined.

Couldn't we do this instead:

/* Setup user speed if needed */
speed = qca_get_speed(hu, QCA_OPER_SPEED);
if (speed) {
ret = qca_set_speed(hu, QCA_OPER_SPEED);
if (ret)
return ret;

qca_baudrate = qca_get_baudrate_value(speed);
} else {
qca_set_speed(hu, QCA_INIT_SPEED);
}



[Bala]: above else block is not required.
if we have operating speed to set, then we need to send the baud rate change request to BT chip on INIT baud rate. so before sending or setting operating baud rate.. HOST driver should be on the HOST init baud rate.
Pls refer below for more info how we set speed and hci driver works.

Or is setting the init speed needed before the operating speed can be
set? Sorry if we discussed this earlier in this series, I know I had a
few doubts about the speed management but don't recall this one
specifically.


[Bala]: Let me give a big picture how we make BT work via serdev HCI arch. (qca chip is taken as reference)

1. first of all, probe is called when we have compatible string matches in qca_bluetooth_of_match table.
2. that will call qca_serdev_probe() in which based on BT device connected to the platform, we decide IO's
which required from HOST to chip i.e.. controls required to turn ON/OFF BT. which includes clocks and regulators and GPIO's.
3. in qca_serdev_probe() we call hci_uart_register_device() and pass ops (operations handler) handler for driver registration.
4. in hci_uart_register_device(), we call qca_open().. where we open the port via serdev_device_open() that
intern will call Qualcomm specific lower level UART driver i.e.. where we read and write to port register address.
so serdev_device_open() is a common function to all BT chip vendor, but the open routine in the low level driver is completely vendor dependent.
for example, some vendors will set an default UART speed (speed can be any) while opening the uart_port(). where as other will not set. Just open the port with basic configurations.

5. coming back to qca_open(), we try to make chip to boot up by enabling clock from HOST for BT chip to operate.. it is purely chip defined.
6. moving back to hci_uart_register_device()... we will call hci_register_dev() in that function we call hci_power_on().. that will call hci_dev_do_open()
7. in hci_dev_do_open() we call hdev->setup() i.e. hci_uart_setup() in that function we set initial baud rate and operating bartender if we have protos defined for setting chip baud rate.(in our case it is null)

8. so it will call setup function will be vendor specific.. as we are using an UART, HOST and chip need to stick to the agreement of initial UART speed for commands to send until and change baud rate request is
sent.
9. now in qca_setup(), we setup an initial host baud rate i.e. what we have agreement with BT chip for initial communication. Now here the question comes why are we setting init speed again and again
i.e uart_open() and hci_uart_setup().. here is the answer, wt ever layer we have are common to all vendors may also tend to change. we can't take a risk, we will setup init baud rate in vendor layer i.e.
qca_setup().
10. then we need to change the baud rate of chip..so we need to send commands to chip. at wt speed we need to send change baud rate request is totally based on agreement with BT Chip. so here we use initial
baud rate.
11. we need to change baud rate request command to chip on init baud rate. if command sending is successful we change the host baud rate from init to operating baud rate.. else we operate on init baud rate.


the above is based on my understanding. Pls let me know if i have cleared your doubt.


Other than that:

Reviewed-by: Matthias Kaehlcke <mka@xxxxxxxxxxxx>



--
Regards
Balakrishna.