| Message ID | 20240718-btaml-v2-0-1392b2e21183@amlogic.com |
|---|---|
| Headers | show |
| Series | Add support for Amlogic HCI UART | expand |
Hi Sai, On Thu, Jul 18, 2024 at 2:43 PM Sai Krishna Gajula <saikrishnag@marvell.com> wrote: > > > > -----Original Message----- > > From: Yang Li via B4 Relay <devnull+yang.li.amlogic.com@kernel.org> > > Sent: Thursday, July 18, 2024 1:12 PM > > To: Marcel Holtmann <marcel@holtmann.org>; Luiz Augusto von Dentz > > <luiz.dentz@gmail.com>; David S. Miller <davem@davemloft.net>; Eric > > Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo > > Abeni <pabeni@redhat.com>; Rob Herring <robh@kernel.org>; Krzysztof > > Kozlowski <krzk+dt@kernel.org>; Conor Dooley <conor+dt@kernel.org>; > > Catalin Marinas <catalin.marinas@arm.com>; Will Deacon <will@kernel.org> > > Cc: linux-bluetooth@vger.kernel.org; netdev@vger.kernel.org; > > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm- > > kernel@lists.infradead.org; Yang Li <yang.li@amlogic.com>; Ye He > > <ye.he@amlogic.com> > > 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@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 it's own setup address configuration. > > > > Co-developed-by: Ye He <ye.he@amlogic.com> > > Signed-off-by: Ye He <ye.he@amlogic.com> > > Signed-off-by: Yang Li <yang.li@amlogic.com> > > --- > > 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? > > > + > > + 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 That is exactly what he is doing, and actually it seems to only be doing that because it later calls kfree_skb. > > + 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. > > + 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. 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. > > + 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); > > +} > > ....... >
Dear Krzysztof Thanks for the review. On 2024/7/18 19:43, Krzysztof Kozlowski wrote: > On 18/07/2024 09:42, Yang Li via B4 Relay wrote: >> 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 it's own >> setup address configuration. >> > ... > >> +static int aml_parse_dt(struct aml_serdev *amldev) >> +{ >> + struct device *pdev = amldev->dev; >> + >> + amldev->bt_en_gpio = devm_gpiod_get(pdev, "bt-enable", >> + GPIOD_OUT_LOW); >> + if (IS_ERR(amldev->bt_en_gpio)) { >> + dev_err(pdev, "Failed to acquire bt-enable gpios"); >> + return PTR_ERR(amldev->bt_en_gpio); >> + } >> + >> + if (device_property_read_string(pdev, "firmware-name", >> + &amldev->firmware_name)) { >> + dev_err(pdev, "Failed to acquire firmware path"); >> + return -ENODEV; >> + } >> + >> + amldev->bt_supply = devm_regulator_get(pdev, "bt"); >> + if (IS_ERR(amldev->bt_supply)) { >> + dev_err(pdev, "Failed to acquire regulator"); >> + return PTR_ERR(amldev->bt_supply); >> + } >> + >> + amldev->lpo_clk = devm_clk_get(pdev, NULL); >> + if (IS_ERR(amldev->lpo_clk)) { >> + dev_err(pdev, "Failed to acquire clock source"); >> + return PTR_ERR(amldev->lpo_clk); >> + } >> + >> + /* get rf config parameter */ >> + if (device_property_read_u32(pdev, "antenna-number", >> + &amldev->ant_number)) { >> + dev_info(pdev, "No antenna-number, using default value"); >> + amldev->ant_number = 1; >> + } >> + >> + return 0; >> +} >> + >> +static int aml_power_on(struct aml_serdev *amldev) >> +{ >> + int err; >> + >> + if (!IS_ERR(amldev->bt_supply)) { > How is IS_ERR possible? Well, I got it. > >> + err = regulator_enable(amldev->bt_supply); >> + if (err) { >> + dev_err(amldev->dev, "Failed to enable regulator: (%d)", err); >> + return err; >> + } >> + } >> + >> + if (!IS_ERR(amldev->lpo_clk)) { > How is IS_ERR possible? I got it. > >> + err = clk_prepare_enable(amldev->lpo_clk); >> + if (err) { >> + dev_err(amldev->dev, "Failed to enable lpo clock: (%d)", err); >> + return err; >> + } >> + } >> + >> + if (!IS_ERR(amldev->bt_en_gpio)) > How is IS_ERR possible? I got it. > >> + gpiod_set_value_cansleep(amldev->bt_en_gpio, 1); >> + >> + /* wait 100ms for bluetooth controller power on */ >> + msleep(100); >> + return 0; >> +} > > Best regards, > Krzysztof >
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@marvell.com> wrote: >> >>> -----Original Message----- >>> From: Yang Li via B4 Relay <devnull+yang.li.amlogic.com@kernel.org> >>> Sent: Thursday, July 18, 2024 1:12 PM >>> To: Marcel Holtmann <marcel@holtmann.org>; Luiz Augusto von Dentz >>> <luiz.dentz@gmail.com>; David S. Miller <davem@davemloft.net>; Eric >>> Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo >>> Abeni <pabeni@redhat.com>; Rob Herring <robh@kernel.org>; Krzysztof >>> Kozlowski <krzk+dt@kernel.org>; Conor Dooley <conor+dt@kernel.org>; >>> Catalin Marinas <catalin.marinas@arm.com>; Will Deacon <will@kernel.org> >>> Cc: linux-bluetooth@vger.kernel.org; netdev@vger.kernel.org; >>> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm- >>> kernel@lists.infradead.org; Yang Li <yang.li@amlogic.com>; Ye He >>> <ye.he@amlogic.com> >>> 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@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 it's own setup address configuration. >>> >>> Co-developed-by: Ye He <ye.he@amlogic.com> >>> Signed-off-by: Ye He <ye.he@amlogic.com> >>> Signed-off-by: Yang Li <yang.li@amlogic.com> >>> --- >>> 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 error > That 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
Add support for Amlogic HCI UART, including dt-binding, and Amlogic Bluetooth driver. Signed-off-by: Yang Li <yang.li@amlogic.com> --- Changes in v2: - Introduce a regulator for powering up the Bluetooth chip instead of power sequencing. - Use the GPIO Consumer API to manipulate the GPIO pins instead of the legacy API. - Minor fixes. - Link to v1: https://lore.kernel.org/r/20240705-btaml-v1-0-7f1538f98cef@amlogic.com --- Yang Li (3): dt-bindings: net: bluetooth: Add support for Amlogic Bluetooth Bluetooth: hci_uart: Add support for Amlogic HCI UART MAINTAINERS: Add an entry for Amlogic HCI UART (M: Yang Li) .../bindings/net/bluetooth/amlogic,w155s2-bt.yaml | 66 ++ MAINTAINERS | 7 + 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 +- 7 files changed, 871 insertions(+), 3 deletions(-) --- base-commit: 54dd4796336de8ce5cbf344db837f9b8448ebcf8 change-id: 20240418-btaml-f9d7b19724ab Best regards,