Hi Marcel, -----Original Message----- From: Marcel Holtmann [mailto:marcel@xxxxxxxxxxxx] Sent: Saturday, June 13, 2015 3:54 AM To: Ilya Faenson Cc: linux-bluetooth@xxxxxxxxxxxxxxx; Arend Van Spriel Subject: Re: [PATCH v2 5/5] BlueZ Broadcom UART Protocol Hi Ilya, >> Merge Broadcom protocol with the existing implementation, providing >> for UART setup, firmware download and power management. >> >> Signed-off-by: Ilya Faenson <ifaenson@xxxxxxxxxxxx> >> --- >> drivers/bluetooth/hci_bcm.c | 400 ++++++++++++++++++++++++++++++++++++++++++-- >> 1 file changed, 384 insertions(+), 16 deletions(-) >> >> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c >> index e4d66b6..5ff35b7 100644 >> --- a/drivers/bluetooth/hci_bcm.c >> +++ b/drivers/bluetooth/hci_bcm.c >> @@ -3,6 +3,7 @@ >> * Bluetooth HCI UART driver for Broadcom devices >> * >> * Copyright (C) 2015 Intel Corporation >> + * Copyright (C) 2015 Broadcom Corporation >> * >> * >> * This program is free software; you can redistribute it and/or modify >> @@ -24,6 +25,7 @@ >> #include <linux/kernel.h> >> #include <linux/errno.h> >> #include <linux/skbuff.h> >> +#include <linux/tty.h> >> #include <linux/firmware.h> >> >> #include <net/bluetooth/bluetooth.h> >> @@ -31,12 +33,170 @@ >> >> #include "btbcm.h" >> #include "hci_uart.h" >> +#include "btbcm_uart.h" >> >> struct bcm_data { >> struct sk_buff *rx_skb; >> struct sk_buff_head txq; >> + struct hci_uart *hu; >> + >> + bool is_suspended; /* suspend/resume flag */ >> + >> + struct timer_list timer; /* idle timer */ > > I prefer not to use timers if we can use delayed work instead. > > IF: Could you clarify how you want me to use the delayed work for the frequently refreshed 5 second timer? It would essentially be an extra thread running most of the time with us having to synchronize it with the device stops etc. Clear invite for race conditions in my opinion. refreshed means re-armed? I wonder how delayed work is different then? Maybe I need to understand a bit more than first. IF: Every time a new packet comes from either direction, the idle timer needs to be re-armed to fire in 5 seconds regardless of the current state of this timer. Hope that clarifies the logic. > >> + >> + struct btbcm_uart_parameters pars; /* device parameters */ > > btbcm_uart_params and params please. No pars and not point in spelling out parameters in a struct name. > > IF: Okay. > >> + void *device_context; /* ACPI/DT device context */ >> }; >> >> +/* Suspend/resume synchronization mutex */ >> +static DEFINE_MUTEX(plock); > > Please use a more descriptive name than plock. That is too generic for a global variable. > > IF: Okay. > >> + >> +/* Forward reference */ >> +static struct hci_uart_proto bcm_proto; > > Why do we need this? I am generally not in favor of having forward reference until really really needed. > > IF: That is needed to set the oper-speed in this structure once the device parameters are available. Intel patch has hard-coded it to a single value for all Broadcom devices which is not acceptable. So for Broadcom devices, I think in the static table, no oper-speed should be set. That means it has to come by other means. Either ACPI or DT. But as I mentioned in the other part, that table has to stay const. IF: The oper_speed comes from DT in my current code already. The BlueZ line discipline currently acts on the oper_speed for all manufacturers. Do you want an exception for Broadcom there? If not, the oper-speed changes should apply to everybody. > >> + >> +/* >> + * Callbacks from the BCMBT_UART device >> + */ >> + >> +/* >> + * The platform is suspending. Stop UART activity >> + */ >> +static void suspend_notification(void *context) >> +{ >> + struct hci_uart *hu = (struct hci_uart *)context; > > context is void * and so no need to cast. > > IF: Okay. > >> + struct bcm_data *h4 = hu->priv; >> + >> + BT_DBG("%s: is_suspended %d", __func__, h4->is_suspended); >> + >> + if (!h4->pars.configure_sleep) >> + return; >> + >> + if (!h4->is_suspended) { >> + if (h4->pars.manual_fc) >> + hci_uart_set_flow_control(hu, true); >> + >> + /* Once this callback returns, driver suspends BT via GPIO */ >> + h4->is_suspended = true; >> + } >> +} >> + >> +/* >> + * The platform is resuming. Resume UART activity. >> + */ >> +static void resume_notification(void *context) >> +{ >> + struct hci_uart *hu = (struct hci_uart *)context; >> + struct bcm_data *h4 = hu->priv; >> + >> + BT_DBG("%s: is_suspended %d", __func__, h4->is_suspended); >> + >> + if (!h4->pars.configure_sleep) >> + return; >> + >> + /* When this callback executes, the device has woken up already */ >> + if (h4->is_suspended) { >> + h4->is_suspended = false; >> + >> + if (h4->pars.manual_fc) >> + hci_uart_set_flow_control(hu, false); >> + } >> + >> + /* If we're resumed, the idle timer must be running */ >> + mod_timer(&h4->timer, jiffies + >> + msecs_to_jiffies(h4->pars.idle_timeout_in_secs * 1000)); > > Indention is violated here. > > IF: Okay, will change. > >> +} >> + >> +/* >> + * The BT device is resuming. Resume UART activity if suspended >> + */ >> +static void wakeup_notification(void *context) >> +{ >> + struct hci_uart *hu = (struct hci_uart *)context; >> + struct bcm_data *h4 = hu->priv; >> + >> + BT_DBG("%s: is_suspended %d", __func__, h4->is_suspended); >> + >> + if (!h4->pars.configure_sleep) >> + return; >> + >> + if (h4->is_suspended) { >> + if (h4->pars.manual_fc) >> + hci_uart_set_flow_control(hu, false); >> + >> + h4->is_suspended = false; >> + } >> + >> + /* If we're resumed, the idle timer must be running */ >> + mod_timer(&h4->timer, jiffies + msecs_to_jiffies( >> + h4->pars.idle_timeout_in_secs * 1000)); > > Same here actually and worse it is different from the other one. > > Splitting the rearming into its own helper might help here. > > IF: Okay, will do. > >> +} >> + >> +/* >> + * Make sure we're awake >> + * (called when the resumed state is required) >> + */ >> +static void bcm_ensure_wakeup(struct hci_uart *hu) >> +{ >> + struct bcm_data *h4 = hu->priv; >> + int status; >> + >> + if (!h4->pars.configure_sleep) >> + return; >> + >> + /* Suspend/resume operations are serialized */ >> + mutex_lock(&plock); >> + >> + /* Nothing to do if resumed already */ >> + if (!h4->is_suspended) { >> + mutex_unlock(&plock); >> + >> + /* Just reset the timer */ >> + status = mod_timer(&h4->timer, jiffies + msecs_to_jiffies( >> + h4->pars.idle_timeout_in_secs * 1000)); > > This indentation is still not correct and now we have a 3rd version ;) > > IF: Will change. > >> + return; >> + } >> + >> + /* Wakeup the device */ >> + status = btbcm_uart_control(BTBCM_UART_ACTION_RESUME, >> + h4->device_context, NULL, NULL); >> + if (status) >> + BT_DBG("%s: failed to resume driver %d", __func__, status); > > Conditional BT_DBG on the status is pretty useless in my mind. Either BT_DBG the status or don't at all. > > IF: No trace upon success sounds reasonable to me but I'll change it as per your preferences. They are debug entries and in all the other code we have used them mainly unconditional. I am even fine if you just use BT_ERR here since something really went wrong. Just a conditional debug seems odd. IF: Understood, thanks for the clarifications. > >> + >> + /* Unflow control the port if configured */ >> + resume_notification(hu); >> + >> + mutex_unlock(&plock); >> +} >> + >> +/* >> + * Idle timer callback >> + */ >> +static void bcm_idle_timeout(unsigned long arg) >> +{ >> + struct hci_uart *hu = (struct hci_uart *)arg; >> + struct bcm_data *h4 = hu->priv; >> + int status; >> + >> + BT_DBG("%s: hu %p", __func__, hu); >> + >> + /* Suspend/resume operations are serialized */ >> + mutex_lock(&plock); >> + >> + if (!h4->is_suspended) { >> + /* Flow control the port if configured */ >> + suspend_notification(hu); >> + >> + /* Suspend the device */ >> + status = btbcm_uart_control(BTBCM_UART_ACTION_SUSPEND, >> + h4->device_context, NULL, NULL); >> + if (status) >> + BT_DBG("%s: failed to suspend device %d", __func__, >> + status); >> + } >> + >> + mutex_unlock(&plock); >> +} >> + >> static int bcm_set_baudrate(struct hci_uart *hu, unsigned int speed) >> { >> struct hci_dev *hdev = hu->hdev; >> @@ -88,31 +248,138 @@ static int bcm_set_baudrate(struct hci_uart *hu, unsigned int speed) >> >> static int bcm_open(struct hci_uart *hu) >> { >> - struct bcm_data *bcm; >> + struct btbcm_uart_callbacks callbacks; >> + unsigned long callbacks_size = sizeof(callbacks); > > Not needed since it is all inside the kernel. And I think once this is split into individual exported functions, this will become a lot clearer. > > IF: Okay, will remove. > >> + int status; >> + struct bcm_data *h4; >> + struct tty_struct *tty = hu->tty; >> >> - BT_DBG("hu %p", hu); >> + BT_DBG("bcm_open hu %p", hu); >> >> - bcm = kzalloc(sizeof(*bcm), GFP_KERNEL); >> - if (!bcm) >> + h4 = kzalloc(sizeof(*h4), GFP_KERNEL); >> + if (!h4) >> return -ENOMEM; > > If we are doing renaming of variables, then I am expecting a cleanup patch first that does only that. If this is needed at all or just some left-over merge issue. > > IF: This is indeed a leftover merge issue. Will restore the original. > >> >> - skb_queue_head_init(&bcm->txq); >> + skb_queue_head_init(&h4->txq); >> + hu->priv = h4; >> + h4->hu = hu; >> + h4->is_suspended = false; >> + >> + /* Configure callbacks on the driver */ >> + callbacks.interface_version = BTBCM_UART_INTERFACE_VERSION; >> + callbacks.context = hu; >> + strcpy(callbacks.name, tty->name); >> + callbacks.p_suspend = suspend_notification; >> + callbacks.p_resume = resume_notification; >> + callbacks.p_wakeup = wakeup_notification; >> + status = btbcm_uart_control(BTBCM_UART_ACTION_CONFIGURE_CALLBACKS, >> + NULL, &callbacks, &callbacks_size); >> + if (status) { >> + BT_DBG("bcm_open failed to set driver callbacks %d", status); >> + return status; >> + } >> + if (callbacks_size != sizeof(void *)) { >> + BT_DBG("bcm_open got back %d bytes from callbacks?!", >> + (int)callbacks_size); >> + return -EMSGSIZE; >> + } >> + memcpy(&h4->device_context, &callbacks, sizeof(void *)); >> + BT_DBG("bcm_open callbacks context %p", h4->device_context); >> + >> + /* Retrieve device parameters */ >> + callbacks_size = sizeof(h4->pars); >> + status = btbcm_uart_control(BTBCM_UART_ACTION_GET_PARAMETERS, >> + h4->device_context, &h4->pars, >> + &callbacks_size); >> + if (status) { >> + BT_DBG("bcm_open failed to get dev parameters %d", status); >> + return status; >> + } >> + BT_DBG("Pars ver %d csleep %d dalow %d balow %d idle %d", >> + h4->pars.interface_version, h4->pars.configure_sleep, >> + h4->pars.dev_wake_active_low, h4->pars.bt_wake_active_low, >> + h4->pars.idle_timeout_in_secs); >> + bcm_proto.oper_speed = h4->pars.oper_speed; >> + >> + /* Cycle power to make sure the device is in the known state */ >> + status = btbcm_uart_control(BTBCM_UART_ACTION_POWER_OFF, >> + h4->device_context, NULL, NULL); >> + if (status) { >> + BT_DBG("bcm_open failed to power off %d", status); >> + } else { >> + status = btbcm_uart_control(BTBCM_UART_ACTION_POWER_ON, >> + h4->device_context, NULL, NULL); >> + if (status) >> + BT_DBG("bcm_open failed to power on %d", status); >> + } >> + >> + /* Start the idle timer */ >> + if (h4->pars.configure_sleep) { >> + setup_timer(&h4->timer, bcm_idle_timeout, (unsigned long)hu); >> + if (h4->pars.configure_sleep) >> + mod_timer(&h4->timer, jiffies + msecs_to_jiffies( >> + h4->pars.idle_timeout_in_secs * 1000)); >> + } >> >> - hu->priv = bcm; >> return 0; >> } >> >> static int bcm_close(struct hci_uart *hu) >> { >> - struct bcm_data *bcm = hu->priv; >> + struct btbcm_uart_callbacks callbacks; >> + unsigned long callbacks_size = sizeof(callbacks); >> + struct bcm_data *h4 = hu->priv; >> + int status; >> >> - BT_DBG("hu %p", hu); >> + hu->priv = NULL; >> >> - skb_queue_purge(&bcm->txq); >> - kfree_skb(bcm->rx_skb); >> - kfree(bcm); >> + BT_DBG("bcm_close hu %p", hu); >> + >> + /* If we're being closed, we must suspend */ >> + if (h4->pars.configure_sleep) { >> + mutex_lock(&plock); >> + >> + if (!h4->is_suspended) { >> + /* Flow control the port */ >> + suspend_notification(hu); >> + >> + /* Suspend the device */ >> + status = btbcm_uart_control(BTBCM_UART_ACTION_SUSPEND, >> + h4->device_context, NULL, >> + NULL); >> + if (status) { >> + BT_DBG("bcm_close suspend driver fail %d", >> + status); >> + } >> + } >> + >> + mutex_unlock(&plock); >> + >> + del_timer_sync(&h4->timer); >> + } >> + >> + /* Power off the device if possible */ >> + status = btbcm_uart_control(BTBCM_UART_ACTION_POWER_OFF, >> + h4->device_context, NULL, NULL); >> + if (status) >> + BT_DBG("bcm_close failed to power off %d", status); >> + >> + /* de-configure callbacks on the driver */ >> + callbacks.interface_version = BTBCM_UART_INTERFACE_VERSION; >> + callbacks.context = hu; >> + callbacks.p_suspend = NULL; >> + callbacks.p_resume = NULL; >> + callbacks.p_wakeup = NULL; >> + status = btbcm_uart_control(BTBCM_UART_ACTION_CONFIGURE_CALLBACKS, >> + h4->device_context, &callbacks, >> + &callbacks_size); >> + if (status) >> + BT_DBG("bcm_close failed to reset drv callbacks %d", status); >> + skb_queue_purge(&h4->txq); >> >> hu->priv = NULL; >> + kfree(h4); >> + >> return 0; >> } >> >> @@ -129,11 +396,42 @@ static int bcm_flush(struct hci_uart *hu) >> >> static int bcm_setup(struct hci_uart *hu) >> { >> - char fw_name[64]; >> + char fw_name[128]; >> const struct firmware *fw; >> int err; >> - >> - BT_DBG("hu %p", hu); >> + struct sk_buff *skb; >> + struct bcm_data *h4 = hu->priv; >> + unsigned char sleep_pars[] = { >> + 0x01, /* sleep mode 1=UART */ >> + 0x02, /* idle threshold HOST (value * 300ms) */ >> + 0x02, /* idle threshold HC (value * 300ms) */ >> + 0x01, /* BT_WAKE active mode - 1=active high, 0 = active low */ >> + 0x00, /* HOST_WAKE active mode - 1=active high, 0 = active low */ >> + 0x01, /* Allow host sleep during SCO - FALSE */ >> + 0x01, /* combine sleep mode and LPM - 1 == TRUE */ >> + 0x00, /* enable tristate control of UART TX line - FALSE */ >> + 0x00, /* USB auto-sleep on USB SUSPEND */ >> + 0x00, /* USB USB RESUME timeout (seconds) */ >> + 0x00, /* Pulsed Host Wake */ >> + 0x00 /* Enable Break To Host */ >> + }; > > Unless these are all single byte values, this is not endian safe. And it is horrible to read actually. Lets add the proper structs to btbcm.h like Fred did for the other commands. > > And in addition it might be good to add these structs in a separate patch. > > IF: These are all single byte values so it is safe for any architecture. Okay, I'll add separate structures since you wish so. I don't understand the need for a separate patch though as the sleep configuration is a primary objective of this driver. Separate patch makes my life easier to review. Keep in mind that a separate patch with a clean commit explain what things are is also easy to apply. Either I wait until I can apply all patches or I cherry-pick ones that are already fine on its own. Less re-basing for you if we can get things merged quickly. IF: Understood, will create a separate patch. > >> + unsigned char pcm_int_pars[] = { >> + 0x00, /* 0=PCM routing, 1=SCO over HCI */ >> + 0x02, /* 0=128Kbps,1=256Kbps,2=512Kbps,3=1024Kbps,4=2048Kbps */ >> + 0x00, /* short frame sync 0=short, 1=long */ >> + 0x00, /* sync mode 0=slave, 1=master */ >> + 0x00 /* clock mode 0=slave, 1=master */ >> + }; >> + unsigned char pcm_format_pars[] = { >> + 0x00, /* LSB_First 1=TRUE, 0=FALSE */ >> + 0x00, /* Fill_Value (use 0-7 for fill bits value) */ >> + 0x02, /* Fill_Method (2=sign extended) */ >> + 0x03, /* Fill_Num # of fill bits 0-3) */ >> + 0x01 /* Right_Justify 1=TRUE, 0=FALSE */ >> + }; >> + unsigned char time_slot_number = 0; >> + >> + BT_DBG("bcm_setup hu %p", hu); >> >> hu->hdev->set_bdaddr = btbcm_set_bdaddr; >> >> @@ -162,6 +460,67 @@ static int bcm_setup(struct hci_uart *hu) >> hci_uart_set_baudrate(hu, hu->proto->oper_speed); >> } >> >> + /* Configure SCO PCM parameters */ >> + if (h4->pars.configure_audio) { >> + pcm_int_pars[0] = h4->pars.pcm_routing; >> + pcm_int_pars[1] = h4->pars.pcm_incallbitclock; >> + pcm_int_pars[2] = h4->pars.pcm_shortframesync; >> + pcm_int_pars[3] = h4->pars.pcm_syncmode; >> + pcm_int_pars[4] = h4->pars.pcm_clockmode; > > I prefer if vendor drivers add comments above their vendor commands explaining what they do and way. See the Intel driver on how far we went to help others understand what is going here. > > IF: I will add comments here. > >> + skb = __hci_cmd_sync(hu->hdev, 0xfc1c, sizeof(pcm_int_pars), >> + pcm_int_pars, HCI_INIT_TIMEOUT); >> + if (IS_ERR(skb)) { >> + err = PTR_ERR(skb); >> + BT_ERR("bcm_setup pcm_ INT VSC failed (%d)", err); >> + goto finalize; >> + } >> + kfree_skb(skb); >> + BT_DBG("bcm_setup pcm_ INT Parameters VSC succeeded"); >> + >> + pcm_format_pars[0] = h4->pars.pcm_lsbfirst; >> + pcm_format_pars[1] = h4->pars.pcm_fillvalue; >> + pcm_format_pars[2] = h4->pars.pcm_fillmethod; >> + pcm_format_pars[3] = h4->pars.pcm_fillnum; >> + pcm_format_pars[4] = h4->pars.pcm_rightjustify; >> + skb = __hci_cmd_sync(hu->hdev, 0xfc1e, sizeof(pcm_format_pars), >> + pcm_format_pars, HCI_INIT_TIMEOUT); >> + if (IS_ERR(skb)) { >> + err = PTR_ERR(skb); >> + BT_ERR("bcm_setup pcm_ Format VSC failed (%d)", >> + err); >> + goto finalize; >> + } >> + kfree_skb(skb); >> + BT_DBG("bcm_setup pcm_ Format VSC succeeded"); >> + >> + skb = __hci_cmd_sync(hu->hdev, 0xfc22, sizeof(time_slot_number), >> + &time_slot_number, HCI_INIT_TIMEOUT); >> + if (IS_ERR(skb)) { >> + err = PTR_ERR(skb); >> + BT_ERR("bcm_setup SCO Time Slot VSC failed (%d)", >> + err); >> + goto finalize; >> + } >> + kfree_skb(skb); >> + BT_DBG("bcm_setup SCO Time Slot VSC succeeded"); >> + } >> + >> + /* Configure device's suspend/resume operation */ >> + if (h4->pars.configure_sleep) { >> + /* Override the default */ >> + sleep_pars[3] = (unsigned char)!h4->pars.bt_wake_active_low; >> + sleep_pars[4] = (unsigned char)!h4->pars.dev_wake_active_low; >> + skb = __hci_cmd_sync(hu->hdev, 0xfc27, sizeof(sleep_pars), >> + sleep_pars, HCI_INIT_TIMEOUT); >> + if (IS_ERR(skb)) { >> + err = PTR_ERR(skb); >> + BT_ERR("bcm_setup Sleep VSC failed (%d)", err); >> + goto finalize; >> + } >> + kfree_skb(skb); >> + BT_DBG("bcm_setup Set Sleep Parameters VSC succeeded"); >> + } >> + >> finalize: >> release_firmware(fw); >> >> @@ -183,6 +542,11 @@ static int bcm_recv(struct hci_uart *hu, const void *data, int count) >> if (!test_bit(HCI_UART_REGISTERED, &hu->flags)) >> return -EUNATCH; >> >> + BT_DBG("bcm_recv %d bytes", count); > > I would prefer not to have this BT_DBG here. This function will be called a lot. > > IF: This has been very useful to me to see whether anything has been received or not at the driver setup time. How would you want me to learn that? Because it will kept being call during every single operation. Including A2DP traffic. I just think that for a final patch that goes upstream this should not be there. We did not do it for the other drivers either. Or did we? IF: That's right: drivers for other manufacturers do not have that. I will remove it. > >> + >> + /* Make sure we're resumed */ >> + bcm_ensure_wakeup(hu); >> + >> bcm->rx_skb = h4_recv_buf(hu->hdev, bcm->rx_skb, data, count, >> bcm_recv_pkts, ARRAY_SIZE(bcm_recv_pkts)); >> if (IS_ERR(bcm->rx_skb)) { >> @@ -198,7 +562,10 @@ static int bcm_enqueue(struct hci_uart *hu, struct sk_buff *skb) >> { >> struct bcm_data *bcm = hu->priv; >> >> - BT_DBG("hu %p skb %p", hu, skb); >> + BT_DBG("bcm_enqueue hu %p skb %p", hu, skb); > > This is again that the function name is available with dynamic debug. So please do not change these or send separate patches with a commit message explaining why this change makes sense. > > IF: Okay, will remove although dynamic debug is not available on all platforms. > >> + >> + /* Make sure we're resumed */ >> + bcm_ensure_wakeup(hu); >> >> /* Prepend skb with frame type */ >> memcpy(skb_push(skb, 1), &bt_cb(skb)->pkt_type, 1); >> @@ -214,7 +581,8 @@ static struct sk_buff *bcm_dequeue(struct hci_uart *hu) >> return skb_dequeue(&bcm->txq); >> } >> >> -static const struct hci_uart_proto bcm_proto = { >> +/* This structure may not be const as speed may change at runtime */ > > No. This struct is const. These are the value that a protocol driver sets. > > If values are dynamic and change based on DT, then left them zero here and provide a method for overwriting them internally in hci_uart. Please do not hack this in. There are simple UART drivers that we want to enable in a really simple way. > > IF: As I said above, I've inherited the need to change the speed(s) from the recent Intel patches. Will try fixing that. I get where you are coming from. And for Broadcom the table will not contain an oper-speed. That will come from ACPI or DT. So you action should be to remove it here and introduce a variable in bcm_data that has it and that can be used instead. I have not figured out all the details, but that is the general idea. I leave the details up to you. IF: As I said above, this is slightly more complicated as the oper_speed is used by the BlueZ line discipline for all manufacturers. And any manufacturer with the oper-speed changeable among the devices will be affected. Since the oper-speed used is largely defined by the UART rather than BT device capabilities, I'm afraid it should be configurable and not reside in a const structure for all. 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
