Return-Path: MIME-Version: 1.0 In-Reply-To: <665B6C30-D115-437A-B991-999A862736FE@holtmann.org> References: <20171117223543.32429-1-martin.blumenstingl@googlemail.com> <20171117223543.32429-7-martin.blumenstingl@googlemail.com> <665B6C30-D115-437A-B991-999A862736FE@holtmann.org> From: Martin Blumenstingl Date: Sun, 19 Nov 2017 21:28:45 +0100 Message-ID: Subject: Re: [RFC v1 6/8] 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 Content-Type: text/plain; charset="UTF-8" List-ID: Hi Marcel, On Sun, Nov 19, 2017 at 9:29 AM, Marcel Holtmann wrote: > 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 | 195 ++++++++++++++++++++++++++++++++++++++++++++- >> 2 files changed, 195 insertions(+), 1 deletion(-) >> >> 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 >> diff --git a/drivers/bluetooth/hci_h5.c b/drivers/bluetooth/hci_h5.c >> index 6a8d0d06aba7..7d584e5928bf 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 *disable_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[] = { 0x01, 0x7e }; >> + int err; >> >> BT_DBG("hu %p", hu); >> >> @@ -210,6 +225,14 @@ static int h5_open(struct hci_uart *hu) >> >> h5->tx_win = H5_TX_WIN_MAX; >> >> + if (hu->serdev) { >> + err = serdev_device_open(hu->serdev); >> + if (err) { >> + bt_dev_err(hu->hdev, "failed to open serdev: %d", err); >> + return err; >> + } >> + } >> + >> set_bit(HCI_UART_INIT_PENDING, &hu->hdev_flags); >> >> /* Send initial sync request */ >> @@ -219,6 +242,23 @@ 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 = serdev_device_get_drvdata(hu->serdev); >> + >> + err = h5_dev->vendor_setup(h5_dev); >> + if (err) >> + return err; > > if (h5_dev->vendor_setup) > return h5_dev->vendor_setup(h5_dev); sounds reasonable, this way new devices can be added which don't need any vendor setup I'll fix this in the next version, thanks for spotting this >> + >> + return 0; >> +} >> + >> static int h5_close(struct hci_uart *hu) >> { >> struct h5 *h5 = hu->priv; >> @@ -229,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 = serdev_device_get_drvdata(hu->serdev); >> + gpiod_set_value_cansleep(h5_dev->disable_gpio, 1); >> + >> + serdev_device_close(hu->serdev); >> + } >> + >> kfree(h5); >> >> return 0; >> @@ -316,7 +365,10 @@ static void h5_handle_internal_rx(struct hci_uart *hu) >> h5->tx_win = (data[2] & 0x07); >> BT_DBG("Three-wire init complete. tx_win %u", h5->tx_win); >> h5->state = H5_ACTIVE; >> - hci_uart_init_ready(hu); >> + >> + /* serdev does not support the "init_ready" signal */ >> + if (!hu->serdev) >> + hci_uart_init_ready(hu); >> return; >> } else if (memcmp(data, sleep_req, 2) == 0) { >> BT_DBG("Peer went to sleep"); >> @@ -739,10 +791,147 @@ 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 = &h5_dev->hu; >> + int err = 0, retry = 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 { >> + /* Configure BT_DISn and BT_RST_N to LOW state */ >> + gpiod_set_value_cansleep(h5_dev->reset_gpio, 1); >> + gpiod_set_value_cansleep(h5_dev->disable_gpio, 1); >> + msleep(500); >> + gpiod_set_value_cansleep(h5_dev->reset_gpio, 0); >> + gpiod_set_value_cansleep(h5_dev->disable_gpio, 0); >> + msleep(500); > > I really hate random msleep() without comments. Explain in the comment block why this specific wait is good. OK, I'll add a comment *why* it's needed (without a delay between toggling the GPIOs the device does not respond in the following btrtl_initialize call) the numbers however are based on "trial and error" as I could not find any documentation for these >> + >> + btrtl_dev = btrtl_initialize(hu->hdev); >> + if (!IS_ERR(btrtl_dev)) >> + break; >> + >> + /* Toggle BT_DISn and retry */ >> + } while (retry--); >> + >> + if (IS_ERR(btrtl_dev)) >> + return PTR_ERR(btrtl_dev); >> + >> + err = btrtl_get_uart_settings(hu->hdev, btrtl_dev, >> + &controller_baudrate, &device_baudrate, >> + &flow_control); >> + if (err) >> + goto out_free; >> + >> + baudrate_data = cpu_to_le32(device_baudrate); >> + skb = __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 = -PTR_ERR(skb); >> + goto out_free; >> + } else { >> + kfree_skb(skb); >> + } >> + >> + msleep(500); > > Same here, explain why this time is the right time to wait. you are right, this may be obsolete since the __hci_cmd_sync call above is already waiting for a reply. I'll verify this and either remove this msleep or add a comment why it's needed >> + >> + serdev_device_set_baudrate(hu->serdev, controller_baudrate); >> + serdev_device_set_flow_control(hu->serdev, flow_control); >> + >> + err = 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 = devm_kzalloc(&serdev->dev, sizeof(*h5_dev), GFP_KERNEL); >> + if (!h5_dev) >> + return -ENOMEM; >> + >> + hu = &h5_dev->hu; >> + hu->serdev = serdev; >> + >> + serdev_device_set_drvdata(serdev, h5_dev); >> + >> + h5_dev->vendor_setup = of_device_get_match_data(&serdev->dev); >> + >> + h5_dev->disable_gpio = devm_gpiod_get_optional(&serdev->dev, "disable", >> + GPIOD_OUT_LOW); >> + if (IS_ERR(h5_dev->disable_gpio)) >> + return PTR_ERR(h5_dev->disable_gpio); >> + >> + h5_dev->reset_gpio = devm_gpiod_get_optional(&serdev->dev, "reset", >> + 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 = serdev_device_get_drvdata(serdev); >> + struct hci_uart *hu = &h5_dev->hu; >> + struct hci_dev *hdev = 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[] = { >> + { >> + .compatible = "realtek,rtl8723bs-bluetooth", >> + .data = h5_setup_realtek >> + }, >> + { >> + .compatible = "realtek,rtl8723ds-bluetooth", >> + .data = h5_setup_realtek >> + }, >> + {}, >> +}; >> +MODULE_DEVICE_TABLE(of, hci_h5_of_match); >> +#endif >> + >> +static struct serdev_device_driver hci_h5_drv = { >> + .driver = { >> + .name = "hci-h5", >> + .of_match_table = of_match_ptr(hci_h5_of_match), >> + }, >> + .probe = hci_h5_probe, >> + .remove = hci_h5_remove, >> +}; >> +#endif >> + >> static const struct hci_uart_proto h5p = { >> .id = HCI_UART_3WIRE, >> .name = "Three-wire (H5)", >> .open = h5_open, >> + .setup = h5_setup, >> .close = h5_close, >> .recv = h5_recv, >> .enqueue = h5_enqueue, >> @@ -752,10 +941,14 @@ static const struct hci_uart_proto h5p = { >> >> 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 > > Marcel > Regards Martin