Return-Path: MIME-Version: 1.0 In-Reply-To: References: <20180101204217.26165-1-martin.blumenstingl@googlemail.com> <20180101204217.26165-10-martin.blumenstingl@googlemail.com> From: Martin Blumenstingl Date: Tue, 2 Jan 2018 22:27:37 +0100 Message-ID: Subject: Re: [RFC v2 9/9] Bluetooth: hci_h5: add support for Realtek UART Bluetooth modules To: Marcel Holtmann Cc: robh+dt@kernel.org, devicetree@vger.kernel.org, linux-bluetooth@vger.kernel.org, linux-serial@vger.kernel.org, mark.rutland@arm.com, "Gustavo F. Padovan" , Johan Hedberg , gregkh@linuxfoundation.org, jslaby@suse.com, johan@kernel.org, linux-amlogic@lists.infradead.org, Larry.Finger@lwfinger.net, carlo@endlessm.com, drake@endlessm.com Content-Type: text/plain; charset="UTF-8" List-ID: Hi Marcel, On Tue, Jan 2, 2018 at 12:11 PM, Marcel Holtmann wrot= e: > Hi Martin, > >> Realtek RTL8723BS and RTL8723DS are SDIO wifi chips with an embedded >> Bluetooth controller which connects to the host via UART. >> The H5 protocol is used for communication between host and device. >> >> The Realtek "rtl8723bs_bt" and "rtl8723ds_bt" userspace Bluetooth UART >> initialization tools (rtk_hciattach) use the following sequence: >> 1) send H5 sync pattern (already supported by hci_h5) >> 2) get LMP version (already supported by btrtl) >> 3) get ROM version (already supported by btrtl) >> 4) load the firmware and config for the current chipset (already >> supported by btrtl) >> 5) read UART settings from the config blob (already supported by btrtl) >> 6) send UART settings via a vendor command to the device (which changes >> the baudrate of the device and enables or disables flow control >> depending on the config) >> 7) change the baudrate and flow control settings on the host >> 8) send the firmware and config blob to the device (already supported by >> btrtl) >> >> This uses the serdev library as well as the existing btrtl driver to >> initialize the Bluetooth functionality, which consists of: >> - identifying the device and loading the corresponding firmware and >> config blobs (steps #2, #3 and #4) >> - configuring the baudrate and flow control (steps #6 and #7) >> - uploading the firmware to the device (step #8) >> >> Signed-off-by: Martin Blumenstingl >> --- >> drivers/bluetooth/Kconfig | 1 + >> drivers/bluetooth/hci_h5.c | 205 +++++++++++++++++++++++++++++++++++++++= ++++++ >> 2 files changed, 206 insertions(+) >> >> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig >> index 60e1c7d6986d..3001f1200c72 100644 >> --- a/drivers/bluetooth/Kconfig >> +++ b/drivers/bluetooth/Kconfig >> @@ -146,6 +146,7 @@ config BT_HCIUART_LL >> config BT_HCIUART_3WIRE >> bool "Three-wire UART (H5) protocol support" >> depends on BT_HCIUART >> + select BT_RTL if SERIAL_DEV_BUS >> help >> The HCI Three-wire UART Transport Layer makes it possible to >> user the Bluetooth HCI over a serial port interface. The HCI > > while this is fine initially, I think long term this is not sustainable. = So we need to abstract the H:5 part and the vendor specific setup part so t= hat you can have a hci_rtl.c that uses H:5 instead of H:4 and still is as t= iny and simple as our H:4 drivers. I had a look at hci_ath to see how hci_h4 is re-usable and I think I get your point do you have any specific solution already in mind or do you want to me make a suggestion (by providing another patch)? if you don't have any specific in mind: do you have anything you want me to "consider" or "avoid"? > One other think I am not really happy about here is the Kconfig entry its= elf. You need to know that you need 3WIRE and SERDEV to magically enable RT= L UART support. Maybe it would be better to just have a CONFIG_HCIUART_RTL = and duplicate the config entry just specific to RTL. It then can select RTL= , but in the Makefile it will result in using the same hci_h5.c file. if I understand you correctly this issue would go away once hci_h5 provides library functions (like hci_h4) since we would have a dedicated CONFIG_HCIUART_RTL anyways which could simply select BT_HCIUART_3WIRE >> diff --git a/drivers/bluetooth/hci_h5.c b/drivers/bluetooth/hci_h5.c >> index 6cfc2f276250..a03acc3b1b52 100644 >> --- a/drivers/bluetooth/hci_h5.c >> +++ b/drivers/bluetooth/hci_h5.c >> @@ -28,7 +28,14 @@ >> #include >> #include >> >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> #include "hci_uart.h" >> +#include "btrtl.h" >> >> #define HCI_3WIRE_ACK_PKT 0 >> #define HCI_3WIRE_LINK_PKT 15 >> @@ -97,6 +104,13 @@ struct h5 { >> } sleep; >> }; >> >> +struct h5_device { >> + struct hci_uart hu; >> + struct gpio_desc *enable_gpio; >> + struct gpio_desc *reset_gpio; >> + int (*vendor_setup)(struct h5_device *h5_dev); >> +}; >> + >> static void h5_reset_rx(struct h5 *h5); >> >> static void h5_link_control(struct hci_uart *hu, const void *data, size_= t len) >> @@ -190,6 +204,7 @@ static int h5_open(struct hci_uart *hu) >> { >> struct h5 *h5; >> const unsigned char sync[] =3D { 0x01, 0x7e }; >> + int err; >> >> BT_DBG("hu %p", hu); >> >> @@ -210,6 +225,14 @@ static int h5_open(struct hci_uart *hu) >> >> h5->tx_win =3D H5_TX_WIN_MAX; >> >> + if (hu->serdev) { >> + err =3D serdev_device_open(hu->serdev); >> + if (err) { >> + bt_dev_err(hu->hdev, "failed to open serdev: %d", = err); >> + return err; >> + } >> + } >> + >> /* Send initial sync request */ >> h5_link_control(hu, sync, sizeof(sync)); >> mod_timer(&h5->timer, jiffies + H5_SYNC_TIMEOUT); >> @@ -217,6 +240,25 @@ static int h5_open(struct hci_uart *hu) >> return 0; >> } >> >> +static int h5_setup(struct hci_uart *hu) >> +{ >> + int err; >> + struct h5_device *h5_dev; >> + >> + if (!hu->serdev) >> + return 0; >> + >> + h5_dev =3D serdev_device_get_drvdata(hu->serdev); >> + >> + if (h5_dev->vendor_setup) { >> + err =3D h5_dev->vendor_setup(h5_dev); >> + if (err) >> + return err; >> + } >> + >> + return 0; >> +} >> + >> static int h5_close(struct hci_uart *hu) >> { >> struct h5 *h5 =3D hu->priv; >> @@ -227,6 +269,15 @@ static int h5_close(struct hci_uart *hu) >> skb_queue_purge(&h5->rel); >> skb_queue_purge(&h5->unrel); >> >> + if (hu->serdev) { >> + struct h5_device *h5_dev; >> + >> + h5_dev =3D serdev_device_get_drvdata(hu->serdev); >> + gpiod_set_value_cansleep(h5_dev->enable_gpio, 0); >> + >> + serdev_device_close(hu->serdev); >> + } >> + >> kfree(h5); >> >> return 0; >> @@ -736,10 +787,160 @@ static int h5_flush(struct hci_uart *hu) >> return 0; >> } >> >> +#if IS_ENABLED(CONFIG_SERIAL_DEV_BUS) >> +static int h5_setup_realtek(struct h5_device *h5_dev) >> +{ >> + struct hci_uart *hu =3D &h5_dev->hu; >> + int err =3D 0, retry =3D 3; >> + struct sk_buff *skb; >> + struct btrtl_device_info *btrtl_dev; >> + __le32 baudrate_data; >> + u32 device_baudrate; >> + unsigned int controller_baudrate; >> + bool flow_control; >> + >> + /* devices always start with flow control disabled and even parity= */ >> + serdev_device_set_flow_control(hu->serdev, false); >> + serdev_device_set_parity(hu->serdev, true, false); >> + >> + do { >> + /* disable the device and put it into reset. some devices = only >> + * have one of these lines, so we toggle both here to supp= ort >> + * all combinations. >> + */ >> + gpiod_set_value_cansleep(h5_dev->reset_gpio, 1); >> + gpiod_set_value_cansleep(h5_dev->enable_gpio, 0); >> + >> + /* wait until the device is disabled and/or reset. 500ms a= re >> + * chosen by manually testing on a RTL8723BS. shorter wait >> + * times lead to a non-responding device. >> + */ >> + msleep(500); >> + >> + /* take the device out of reset and enable it. */ >> + gpiod_set_value_cansleep(h5_dev->reset_gpio, 0); >> + gpiod_set_value_cansleep(h5_dev->enable_gpio, 1); >> + >> + /* after that we need to wait 500ms, otherwise the device = might >> + * not respond in all cases. this was determined by testin= g >> + * with a RTL8723BS. >> + */ >> + msleep(500); >> + >> + btrtl_dev =3D btrtl_initialize(hu->hdev); >> + if (!IS_ERR(btrtl_dev)) >> + break; >> + >> + /* Toggle the enable and reset pins above and try again */ >> + } while (retry--); >> + >> + if (IS_ERR(btrtl_dev)) >> + return PTR_ERR(btrtl_dev); >> + >> + err =3D btrtl_get_uart_settings(hu->hdev, btrtl_dev, >> + &controller_baudrate, &device_baudra= te, >> + &flow_control); >> + if (err) >> + goto out_free; >> + >> + baudrate_data =3D cpu_to_le32(device_baudrate); >> + skb =3D __hci_cmd_sync(hu->hdev, 0xfc17, sizeof(baudrate_data), >> + &baudrate_data, HCI_INIT_TIMEOUT); >> + if (IS_ERR(skb)) { >> + bt_dev_err(hu->hdev, "set baud rate command failed"); >> + err =3D -PTR_ERR(skb); >> + goto out_free; >> + } else { >> + kfree_skb(skb); >> + } >> + >> + serdev_device_set_baudrate(hu->serdev, controller_baudrate); >> + serdev_device_set_flow_control(hu->serdev, flow_control); >> + >> + err =3D btrtl_download_firmware(hu->hdev, btrtl_dev); >> + >> +out_free: >> + btrtl_free(btrtl_dev); >> + >> + return err; >> +} >> + >> +static const struct hci_uart_proto h5p; >> + >> +static int hci_h5_probe(struct serdev_device *serdev) >> +{ >> + struct hci_uart *hu; >> + struct h5_device *h5_dev; >> + >> + h5_dev =3D devm_kzalloc(&serdev->dev, sizeof(*h5_dev), GFP_KERNEL)= ; >> + if (!h5_dev) >> + return -ENOMEM; >> + >> + hu =3D &h5_dev->hu; >> + hu->serdev =3D serdev; >> + >> + serdev_device_set_drvdata(serdev, h5_dev); >> + >> + h5_dev->vendor_setup =3D of_device_get_match_data(&serdev->dev); >> + >> + h5_dev->enable_gpio =3D devm_gpiod_get_optional(&serdev->dev, "ena= ble", >> + GPIOD_OUT_HIGH); > > Indentation mistake. I'll take care of that in the next version - thanks for spotting! >> + if (IS_ERR(h5_dev->enable_gpio)) >> + return PTR_ERR(h5_dev->enable_gpio); >> + >> + h5_dev->reset_gpio =3D devm_gpiod_get_optional(&serdev->dev, "rese= t", >> + GPIOD_OUT_LOW); >> + if (IS_ERR(h5_dev->reset_gpio)) >> + return PTR_ERR(h5_dev->reset_gpio); >> + >> + hci_uart_set_speeds(hu, 115200, 0); >> + >> + return hci_uart_register_device(hu, &h5p); >> +} >> + >> +static void hci_h5_remove(struct serdev_device *serdev) >> +{ >> + struct h5_device *h5_dev =3D serdev_device_get_drvdata(serdev); >> + struct hci_uart *hu =3D &h5_dev->hu; >> + struct hci_dev *hdev =3D hu->hdev; >> + >> + cancel_work_sync(&hu->write_work); >> + >> + hci_unregister_dev(hdev); >> + hci_free_dev(hdev); >> + hu->proto->close(hu); >> +} >> + >> +#ifdef CONFIG_OF >> +static const struct of_device_id hci_h5_of_match[] =3D { >> + { >> + .compatible =3D "realtek,rtl8723bs-bluetooth", >> + .data =3D h5_setup_realtek >> + }, >> + { >> + .compatible =3D "realtek,rtl8723ds-bluetooth", >> + .data =3D h5_setup_realtek >> + }, >> + {}, >> +}; >> +MODULE_DEVICE_TABLE(of, hci_h5_of_match); >> +#endif >> + >> +static struct serdev_device_driver hci_h5_drv =3D { >> + .driver =3D { >> + .name =3D "hci-h5", >> + .of_match_table =3D of_match_ptr(hci_h5_of_match), >> + }, >> + .probe =3D hci_h5_probe, >> + .remove =3D hci_h5_remove, >> +}; >> +#endif >> + >> static const struct hci_uart_proto h5p =3D { >> .id =3D HCI_UART_3WIRE, >> .name =3D "Three-wire (H5)", >> .open =3D h5_open, >> + .setup =3D h5_setup, >> .close =3D h5_close, >> .recv =3D h5_recv, >> .enqueue =3D h5_enqueue, >> @@ -749,10 +950,14 @@ static const struct hci_uart_proto h5p =3D { >> >> int __init h5_init(void) >> { >> + serdev_device_driver_register(&hci_h5_drv); >> + >> return hci_uart_register_proto(&h5p); >> } >> >> int __exit h5_deinit(void) >> { >> + serdev_device_driver_unregister(&hci_h5_drv); >> + >> return hci_uart_unregister_proto(&h5p); >> } Regards Martin