Dear Sai and Luiz Thanks for the review. On 2024/7/19 3:01, Luiz Augusto von Dentz wrote:
Hi Sai, On Thu, Jul 18, 2024 at 2:43 PM Sai Krishna Gajula <saikrishnag@xxxxxxxxxxx> wrote:-----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.First time I'm hearing about this, is that just for the sake of readability?
well, for the sake of readability.
+ + 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 errorThat is exactly what he is doing, and actually it seems to only be doing that because it later calls kfree_skb.
Yes, it is right, but there is another issue that it setting skb to NULL before kfree_skb(skb)
causes memory leaks, and I will delete "skb = NULL;"
+ bt_dev_err(hdev, "Failed to send TCI cmd:(%d)", err); + goto exit; + } + + rsp = (struct aml_tci_rsp *)(skb->data);This code is not safe, you need to check if skb->len because trying to access skb->data, anyway you are probably much better off using skb_pull_data instead.
well, I got it.
+ 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.
Okay, I will do.
+ /* 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.
Okay, I will do.
+ 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.
Okay, I will do.
+ 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.Ok, this is really the wrong order and will overwrite the error, that said replacing goto exit with return PTR_ERR(skb) would be enough here since there is nothing else that needs to be cleanup.
Well, I got it.
+ 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.
Okay, I got it.
+ + 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); +}.......-- Luiz Augusto von Dentz
