RE: [PATCH v2 2/3] Bluetooth: hci_uart: Add support for Amlogic HCI UART

From: Sai Krishna Gajula
Date: Thu Jul 18 2024 - 14:45:49 EST

Next message: Rob Clark: "[PATCH 1/3] dt-bindings: display: panel: samsung,atna45dc02: Document ATNA45DC02"
Previous message: Breno Leitao: "[RFC PATCH 2/2] netconsole: Defer netpoll cleanup to avoid lock release during list traversal"
In reply to: Yang Li: "Re: [PATCH v2 2/3] Bluetooth: hci_uart: Add support for Amlogic HCI UART"
Next in thread: Luiz Augusto von Dentz: "Re: [PATCH v2 2/3] Bluetooth: hci_uart: Add support for Amlogic HCI UART"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

> -----Original Message-----
> From: Yang Li via B4 Relay <devnull+yang.li.amlogic.com@xxxxxxxxxx>
> Sent: Thursday, July 18, 2024 1:12 PM
> To: Marcel Holtmann <marcel@xxxxxxxxxxxx>; Luiz Augusto von Dentz
> <luiz.dentz@xxxxxxxxx>; David S. Miller <davem@xxxxxxxxxxxxx>; Eric
> Dumazet <edumazet@xxxxxxxxxx>; Jakub Kicinski <kuba@xxxxxxxxxx>; Paolo
> Abeni <pabeni@xxxxxxxxxx>; Rob Herring <robh@xxxxxxxxxx>; Krzysztof
> Kozlowski <krzk+dt@xxxxxxxxxx>; Conor Dooley <conor+dt@xxxxxxxxxx>;
> Catalin Marinas <catalin.marinas@xxxxxxx>; Will Deacon <will@xxxxxxxxxx>
> Cc: linux-bluetooth@xxxxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-arm-
> kernel@xxxxxxxxxxxxxxxxxxx; Yang Li <yang.li@xxxxxxxxxxx>; Ye He
> <ye.he@xxxxxxxxxxx>
> Subject: [PATCH v2 2/3] Bluetooth: hci_uart: Add support for
> Amlogic HCI UART
>
> From: Yang Li <yang. li@ amlogic. com> This patch introduces support for
> Amlogic Bluetooth controller over UART. In order to send the final firmware at
> full speed. It is a pretty straight forward H4 driver with exception of actually
> having
> From: Yang Li <yang.li@xxxxxxxxxxx>
>
> This patch introduces support for Amlogic Bluetooth controller over UART. In
> order to send the final firmware at full speed. It is a pretty straight forward H4
> driver with exception of actually having it's own setup address configuration.
>
> Co-developed-by: Ye He <ye.he@xxxxxxxxxxx>
> Signed-off-by: Ye He <ye.he@xxxxxxxxxxx>
> Signed-off-by: Yang Li <yang.li@xxxxxxxxxxx>
> ---
> drivers/bluetooth/Kconfig | 12 +
> drivers/bluetooth/Makefile | 1 +
> drivers/bluetooth/hci_aml.c | 772
> ++++++++++++++++++++++++++++++++++++++++++
> drivers/bluetooth/hci_ldisc.c | 8 +-
> drivers/bluetooth/hci_uart.h | 8 +-
> 5 files changed, 798 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig index
> 90a94a111e67..d9ff7a64d032 100644
> --- a/drivers/bluetooth/Kconfig
> +++ b/drivers/bluetooth/Kconfig
> @@ -274,6 +274,18 @@ config BT_HCIUART_MRVL
>
> Say Y here to compile support for HCI MRVL protocol.
>
> +config BT_HCIUART_AML
> + bool "Amlogic protocol support"
> + depends on BT_HCIUART
> + depends on BT_HCIUART_SERDEV
> + select BT_HCIUART_H4
> + select FW_LOADER
> + help
> + The Amlogic protocol support enables Bluetooth HCI over serial
> + port interface for Amlogic Bluetooth controllers.
> +
> + Say Y here to compile support for HCI AML protocol.
> +
> config BT_HCIBCM203X
> tristate "HCI BCM203x USB driver"
> depends on USB
> diff --git a/drivers/bluetooth/Makefile b/drivers/bluetooth/Makefile index
> 0730d6684d1a..81856512ddd0 100644
> --- a/drivers/bluetooth/Makefile
> +++ b/drivers/bluetooth/Makefile
> @@ -51,4 +51,5 @@ hci_uart-$(CONFIG_BT_HCIUART_BCM) += hci_bcm.o
> hci_uart-$(CONFIG_BT_HCIUART_QCA) += hci_qca.o
> hci_uart-$(CONFIG_BT_HCIUART_AG6XX) += hci_ag6xx.o
> hci_uart-$(CONFIG_BT_HCIUART_MRVL) += hci_mrvl.o
> +hci_uart-$(CONFIG_BT_HCIUART_AML) += hci_aml.o
> hci_uart-objs := $(hci_uart-y)
> diff --git a/drivers/bluetooth/hci_aml.c b/drivers/bluetooth/hci_aml.c new file
> mode 100644 index 000000000000..575b6361dad6
> --- /dev/null
> +++ b/drivers/bluetooth/hci_aml.c
> @@ -0,0 +1,772 @@
> +// SPDX-License-Identifier: (GPL-2.0-only OR MIT)
> +/*
> + * Copyright (C) 2024 Amlogic, Inc. All rights reserved */
> +
> +#include <linux/kernel.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/property.h>

......

> + * op_code | op_len | op_addr | parameter |
> + * --------|-----------------------|---------|-------------|
> + * 2B | 1B len(addr+param) | 4B | len(param) |
> + */
> +static int aml_send_tci_cmd(struct hci_dev *hdev, u16 op_code, u32
> op_addr,
> + u32 *param, u32 param_len)
> +{
> + struct sk_buff *skb = NULL;
> + struct aml_tci_rsp *rsp = NULL;
> + u8 *buf = NULL;
> + u32 buf_len = 0;
> + int err = 0;

Please consider using reverse xmas tree order - longest line to shortest - for local variable declarations in Networking code.

> +
> + buf_len = sizeof(op_addr) + param_len;
> + buf = kmalloc(buf_len, GFP_KERNEL);
> + if (!buf) {
> + err = -ENOMEM;
> + goto exit;
> + }
> +
> + memcpy(buf, &op_addr, sizeof(op_addr));
> + if (param && param_len > 0)
> + memcpy(buf + sizeof(op_addr), param, param_len);
> +
> + skb = __hci_cmd_sync_ev(hdev, op_code, buf_len, buf,
> + HCI_EV_CMD_COMPLETE,
> HCI_INIT_TIMEOUT);
> + if (IS_ERR(skb)) {
> + err = PTR_ERR(skb);
> + skb = NULL;

Better to capture the error before nullifying skb, like below
err = PTR_ERR(skb);
skb = NULL; // Nullify after capturing the error

> + bt_dev_err(hdev, "Failed to send TCI cmd:(%d)", err);
> + goto exit;
> + }
> +
> + rsp = (struct aml_tci_rsp *)(skb->data);
> + if (rsp->opcode != op_code || rsp->status != 0x00) {
> + bt_dev_err(hdev, "send TCI cmd(0x%04X),
> response(0x%04X):(%d)",
> + op_code, rsp->opcode, rsp->status);
> + err = -EINVAL;
> + goto exit;
> + }
> +
> +exit:
> + kfree(buf);
> + kfree_skb(skb);
> + return err;
> +}
> +
> +static int aml_update_chip_baudrate(struct hci_dev *hdev, u32 baud) {
> + u32 value;
> +
> + value = ((AML_UART_CLK_SOURCE / baud) - 1) & 0x0FFF;
> + value |= AML_UART_XMIT_EN | AML_UART_RECV_EN |
> +AML_UART_TIMEOUT_INT_EN;
> +
> + return aml_send_tci_cmd(hdev, AML_TCI_CMD_UPDATE_BAUDRATE,
> + AML_OP_UART_MODE, &value,
> sizeof(value)); }
> +
> +static int aml_start_chip(struct hci_dev *hdev) {
> + u32 value = 0;
> + int ret;
> +
> + value = AML_MM_CTR_HARD_TRAS_EN;
> + ret = aml_send_tci_cmd(hdev, AML_TCI_CMD_WRITE,
> + AML_OP_MEM_HARD_TRANS_EN,
> + &value, sizeof(value));
> + if (ret)
> + return ret;
> +
> + /* controller hardware reset. */
> + value = AML_CTR_CPU_RESET | AML_CTR_MAC_RESET |
> AML_CTR_PHY_RESET;
> + ret = aml_send_tci_cmd(hdev, AML_TCI_CMD_HARDWARE_RESET,
> + AML_OP_HARDWARE_RST,
> + &value, sizeof(value));
> + return ret;
> +}
> +
> +static int aml_send_firmware_segment(struct hci_dev *hdev,
> + u8 fw_type,
> + u8 *seg,
> + u32 seg_size,
> + u32 offset)
> +{
> + u32 op_addr = 0;
> +
> + if (fw_type == FW_ICCM)
> + op_addr = AML_OP_ICCM_RAM_BASE + offset;
> + else if (fw_type == FW_DCCM)
> + op_addr = AML_OP_DCCM_RAM_BASE + offset;
> +
> + return aml_send_tci_cmd(hdev, AML_TCI_CMD_DOWNLOAD_BT_FW,
> + op_addr, (u32 *)seg, seg_size); }
> +
> +static int aml_send_firmware(struct hci_dev *hdev, u8 fw_type,
> + u8 *fw, u32 fw_size, u32 offset) {
> + u32 seg_size = 0;
> + u32 seg_off = 0;
> +
> + if (fw_size > AML_FIRMWARE_MAX_SIZE) {
> + bt_dev_err(hdev, "fw_size error, fw_size:%d, max_size: 512K",
> + fw_size);
> + return -EINVAL;
> + }
> + while (fw_size > 0) {
> + seg_size = (fw_size > AML_FIRMWARE_OPERATION_SIZE) ?
> + AML_FIRMWARE_OPERATION_SIZE : fw_size;
> + if (aml_send_firmware_segment(hdev, fw_type, (fw +
> seg_off),
> + seg_size, offset)) {
> + bt_dev_err(hdev, "Failed send firmware, type:%d,
> offset:0x%x",
> + fw_type, offset);
> + return -EINVAL;
> + }
> + seg_off += seg_size;
> + fw_size -= seg_size;
> + offset += seg_size;
> + }
> + return 0;
> +}
> +
> +static int aml_download_firmware(struct hci_dev *hdev, const char
> +*fw_name) {
> + struct hci_uart *hu = hci_get_drvdata(hdev);
> + struct aml_serdev *amldev = serdev_device_get_drvdata(hu-
> >serdev);
> + const struct firmware *firmware = NULL;
> + struct aml_fw_len *fw_len = NULL;
> + u8 *iccm_start = NULL, *dccm_start = NULL;
> + u32 iccm_len, dccm_len;
> + u32 value = 0;
> + int ret = 0;
> +

Please consider using reverse xmas tree order - longest line to shortest - for local variable declarations in Networking code.

> + /* Enable firmware download event. */
> + value = AML_EVT_EN;
> + ret = aml_send_tci_cmd(hdev, AML_TCI_CMD_WRITE,
> + AML_OP_EVT_ENABLE,
> + &value, sizeof(value));
> + if (ret)
> + goto exit;
> +
> + /* RAM power on */
> + value = AML_RAM_POWER_ON;
> + ret = aml_send_tci_cmd(hdev, AML_TCI_CMD_WRITE,
> + AML_OP_RAM_POWER_CTR,
> + &value, sizeof(value));
> + if (ret)
> + goto exit;
> +
> + /* Check RAM power status */
> + ret = aml_send_tci_cmd(hdev, AML_TCI_CMD_READ,
> + AML_OP_RAM_POWER_CTR, NULL, 0);
> + if (ret)
> + goto exit;
> +
> + ret = request_firmware(&firmware, fw_name, &hdev->dev);
> + if (ret < 0) {
> + bt_dev_err(hdev, "Failed to load <%s>:(%d)", fw_name, ret);
> + goto exit;
> + }
> +
> + fw_len = (struct aml_fw_len *)firmware->data;
> +
> + /* Download ICCM */
> + iccm_start = (u8 *)(firmware->data) + sizeof(struct aml_fw_len)
> + + amldev->aml_dev_data->iccm_offset;
> + iccm_len = fw_len->iccm_len - amldev->aml_dev_data->iccm_offset;
> + ret = aml_send_firmware(hdev, FW_ICCM, iccm_start, iccm_len,
> + amldev->aml_dev_data->iccm_offset);
> + if (ret) {
> + bt_dev_err(hdev, "Failed to send FW_ICCM (%d)", ret);
> + goto exit;
> + }
> +
> + /* Download DCCM */
> + dccm_start = (u8 *)(firmware->data) + sizeof(struct aml_fw_len) +
> fw_len->iccm_len;
> + dccm_len = fw_len->dccm_len;
> + ret = aml_send_firmware(hdev, FW_DCCM, dccm_start, dccm_len,
> + amldev->aml_dev_data->dccm_offset);
> + if (ret) {
> + bt_dev_err(hdev, "Failed to send FW_DCCM (%d)", ret);
> + goto exit;
> + }
> +
> + /* Disable firmware download event. */
> + value = 0;
> + ret = aml_send_tci_cmd(hdev, AML_TCI_CMD_WRITE,
> + AML_OP_EVT_ENABLE,
> + &value, sizeof(value));
> + if (ret)
> + goto exit;
> +
> +exit:
> + if (firmware)
> + release_firmware(firmware);
> + return ret;
> +}
> +
> +static int aml_send_reset(struct hci_dev *hdev) {
> + struct sk_buff *skb;
> + int err;
> +
> + skb = __hci_cmd_sync_ev(hdev, HCI_OP_RESET, 0, NULL,
> + HCI_EV_CMD_COMPLETE,
> HCI_INIT_TIMEOUT);
> + if (IS_ERR(skb)) {
> + err = PTR_ERR(skb);
> + bt_dev_err(hdev, "Failed to send hci reset cmd(%d)", err);
> + return err;
> + }
> +
> + kfree_skb(skb);
> + return 0;
> +}
> +
> +static int aml_dump_fw_version(struct hci_dev *hdev) {
> + struct sk_buff *skb;
> + struct aml_tci_rsp *rsp = NULL;
> + u8 value[6] = {0};
> + u8 *fw_ver = NULL;
> + int err = 0;
> +

Please consider using reverse xmas tree order - longest line to shortest - for local variable declarations in Networking code.

> + skb = __hci_cmd_sync_ev(hdev, AML_BT_HCI_VENDOR_CMD,
> sizeof(value), value,
> + HCI_EV_CMD_COMPLETE,
> HCI_INIT_TIMEOUT);
> + if (IS_ERR(skb)) {
> + skb = NULL;
> + err = PTR_ERR(skb);
> + bt_dev_err(hdev, "Failed get fw version:(%d)", err);
> + goto exit;
> + }
> +
> + rsp = (struct aml_tci_rsp *)(skb->data);
> + if (rsp->opcode != AML_BT_HCI_VENDOR_CMD || rsp->status !=
> 0x00) {
> + bt_dev_err(hdev, "dump version, error response
> (0x%04X):(%d)",
> + rsp->opcode, rsp->status);
> + err = -EINVAL;
> + goto exit;
> + }
> +
> + fw_ver = skb->data + AML_EVT_HEAD_SIZE;
> + bt_dev_info(hdev, "fw_version: date = %02x.%02x, number =
> 0x%02x%02x",
> + *(fw_ver + 1), *fw_ver, *(fw_ver + 3), *(fw_ver + 2));
> +
> +exit:
> + kfree_skb(skb);
> + return err;
> +}
> +
> +static int aml_set_bdaddr(struct hci_dev *hdev, const bdaddr_t *bdaddr)
> +{
> + struct sk_buff *skb;
> + struct aml_tci_rsp *rsp = NULL;
> + int err = 0;
> +

Please consider using reverse xmas tree order - longest line to shortest - for local variable declarations in Networking code.

> + bt_dev_info(hdev, "set bdaddr (%pM)", bdaddr);
> + skb = __hci_cmd_sync_ev(hdev, AML_BT_HCI_VENDOR_CMD,
> + sizeof(bdaddr_t), bdaddr,
> + HCI_EV_CMD_COMPLETE,
> HCI_INIT_TIMEOUT);
> + if (IS_ERR(skb)) {
> + skb = NULL;
> + err = PTR_ERR(skb);

Same here to capture error before making skb null.

> + bt_dev_err(hdev, "Failed to set bdaddr:(%d)", err);
> + goto exit;
> + }
> +
> + rsp = (struct aml_tci_rsp *)(skb->data);
> + if (rsp->opcode != AML_BT_HCI_VENDOR_CMD || rsp->status !=
> 0x00) {
> + bt_dev_err(hdev, "error response (0x%x):(%d)", rsp->opcode,
> rsp->status);
> + err = -EINVAL;
> + goto exit;
> + }
> +
> +exit:
> + kfree_skb(skb);
> + return err;
> +}
> +
> +static int aml_check_bdaddr(struct hci_dev *hdev) {

.......

> +
> +static int aml_close(struct hci_uart *hu) {
> + struct aml_data *aml_data = hu->priv;
> + struct aml_serdev *amldev = serdev_device_get_drvdata(hu-
> >serdev);

Please consider using reverse xmas tree order - longest line to shortest - for local variable declarations in Networking code.

> +
> + if (hu->serdev)
> + serdev_device_close(hu->serdev);
> +
> + skb_queue_purge(&aml_data->txq);
> + kfree_skb(aml_data->rx_skb);
> + kfree(aml_data);
> +
> + hu->priv = NULL;
> +
> + return aml_power_off(amldev);
> +}

.......

Next message: Rob Clark: "[PATCH 1/3] dt-bindings: display: panel: samsung,atna45dc02: Document ATNA45DC02"
Previous message: Breno Leitao: "[RFC PATCH 2/2] netconsole: Defer netpoll cleanup to avoid lock release during list traversal"
In reply to: Yang Li: "Re: [PATCH v2 2/3] Bluetooth: hci_uart: Add support for Amlogic HCI UART"
Next in thread: Luiz Augusto von Dentz: "Re: [PATCH v2 2/3] Bluetooth: hci_uart: Add support for Amlogic HCI UART"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]