Re: [PATCH v3 2/3] Bluetooth: qca: Expand firmware-name to load specific nvm and rampatch

From: Dmitry Baryshkov
Date: Thu Dec 05 2024 - 07:01:05 EST

Next message: Russell King (Oracle): "Re: [PATCH net-next v1 2/7] net: ethtool: add support for structured PHY statistics"
Previous message: Md Sadre Alam: "[PATCH v2] dmaengine: qcom: bam_dma: Avoid writing unavailable register"
In reply to: Cheng Jiang: "[PATCH v3 2/3] Bluetooth: qca: Expand firmware-name to load specific nvm and rampatch"
Next in thread: Cheng Jiang (IOE): "Re: [PATCH v3 2/3] Bluetooth: qca: Expand firmware-name to load specific nvm and rampatch"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Thu, Dec 05, 2024 at 06:22:12PM +0800, Cheng Jiang wrote:
> The firmware-name property has been expanded to specify the names of NVM
> and rampatch firmware for certain chips, such as the QCA6698 Bluetooth
> chip. Although it shares the same IP core as the WCN6855, the QCA6698
> has different RF components and RAM sizes, necessitating new firmware
> files. This change allows for the configuration of NVM and rampatch in
> DT.
>
> Different connectivity boards may be attached to the same platform. For
> example, QCA6698-based boards can support either a two-antenna or
> three-antenna solution, both of which work on the sa8775p-ride platform.
> Due to differences in connectivity boards and variations in RF
> performance from different foundries, different NVM configurations are
> used based on the board ID.

Two separate commits, one for NVM, another one for RAM patch.

>
> Therefore, in the firmware-name property, if the NVM file has an
> extension, the NVM file will be used. Otherwise, the system will first
> try the .bNN (board ID) file, and if that fails, it will fall back to
> the .bin file.
>
> Possible configurations:
> firmware-name = "QCA6698/hpnv21.bin", "QCA6698/hpbtfw21.tlv";
> firmware-name = "QCA6698/hpnv21", "QCA6698/hpbtfw21.tlv";
> firmware-name = "QCA6698/hpnv21.bin";
>
> Signed-off-by: Cheng Jiang <quic_chejiang@xxxxxxxxxxx>
> ---
> drivers/bluetooth/btqca.c | 154 ++++++++++++++++++++++++++----------
> drivers/bluetooth/btqca.h | 5 +-
> drivers/bluetooth/hci_qca.c | 21 ++++-
> 3 files changed, 134 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
> index dfbbac922..e8b89b8cc 100644
> --- a/drivers/bluetooth/btqca.c
> +++ b/drivers/bluetooth/btqca.c
> @@ -272,6 +272,31 @@ int qca_send_pre_shutdown_cmd(struct hci_dev *hdev)
> }
> EXPORT_SYMBOL_GPL(qca_send_pre_shutdown_cmd);
>
> +static int qca_get_alt_nvm_path(char *path, size_t max_size)

int is usually for errors, the code suggests bool return type.

> +{
> + char fwname[64];
> + const char *suffix;
> +
> + suffix = strrchr(path, '.');
> +
> + if (!suffix)
> + return 0;
> +
> + strscpy(fwname, path, strlen(path));

64 bytes ought to be enough for anybody, correct?

> + fwname[suffix - path] = 0;

with path = "qcom/sc7180/Oh.My.Device/name" this is broken.

> +
> + snprintf(fwname, sizeof(fwname), "%s.bin", fwname);
> +
> + /* If nvm file is already the default one, return false to
> + * skip the retry.
> + */
> + if (strcmp(fwname, path) == 0)
> + return 0;
> +
> + snprintf(path, max_size, "%s", fwname);
> + return 1;
> +}
> +
> static int qca_tlv_check_data(struct hci_dev *hdev,
> struct qca_fw_config *config,
> u8 *fw_data, size_t fw_size,
> @@ -564,6 +589,19 @@ static int qca_download_firmware(struct hci_dev *hdev,
> config->fwname, ret);
> return ret;
> }
> + }
> + /* For nvm, if desired nvm file is not present and it's not the
> + * default nvm file(ends with .bin), try to load the default nvm.
> + */
> + else if (config->type == TLV_TYPE_NVM &&
> + qca_get_alt_nvm_path(config->fwname, sizeof(config->fwname))) {

Please, don't rewrite the config. The file may be not present now, but
it will reappear later (e.g. when rootfs gets mounted).

> + bt_dev_info(hdev, "QCA Downloading %s", config->fwname);
> + ret = request_firmware(&fw, config->fwname, &hdev->dev);
> + if (ret) {
> + bt_dev_err(hdev, "QCA Failed to request file: %s (%d)",
> + config->fwname, ret);
> + return ret;
> + }
> } else {
> bt_dev_err(hdev, "QCA Failed to request file: %s (%d)",
> config->fwname, ret);
> @@ -730,15 +768,38 @@ static inline void qca_get_nvm_name_generic(struct qca_fw_config *cfg,
> "qca/%snv%02x.b%02x", stem, rom_ver, bid);
> }
>
> +static void qca_get_nvm_name_by_board(char *fwname, size_t max_size,
> + const char *firmware_name, struct qca_btsoc_version ver,
> + enum qca_btsoc_type soc_type, u16 bid)
> +{
> + const char *variant;
> +
> + /* Set the variant to empty by default */
> + variant = "";
> + /* hsp gf chip */
> + if (soc_type == QCA_WCN6855) {
> + if ((le32_to_cpu(ver.soc_id) & QCA_HSP_GF_SOC_MASK) == QCA_HSP_GF_SOC_ID)
> + variant = "g";

Didn't you get the 'set but unused' here?

> + }
> +
> + if (bid == 0x0)

0x0 or 0xff?

> + snprintf(fwname, max_size, "qca/%s.bin", firmware_name);
> + else if (bid & 0xff00)
> + snprintf(fwname, max_size, "qca/%s.b%x", firmware_name, bid);

Doesn't ".b%02x" work in this case too?

> + else
> + snprintf(fwname, max_size, "qca/%s.b%02x", firmware_name, bid);
> +}
> +
> int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
> enum qca_btsoc_type soc_type, struct qca_btsoc_version ver,
> - const char *firmware_name)
> + const char *firmware_name, const char *rampatch_name)
> {
> struct qca_fw_config config = {};
> int err;
> u8 rom_ver = 0;
> u32 soc_ver;
> u16 boardid = 0;
> + const char *suffix;
>
> bt_dev_dbg(hdev, "QCA setup on UART");
>
> @@ -761,44 +822,48 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>
> /* Download rampatch file */
> config.type = TLV_TYPE_PATCH;
> - switch (soc_type) {
> - case QCA_WCN3990:
> - case QCA_WCN3991:
> - case QCA_WCN3998:
> - snprintf(config.fwname, sizeof(config.fwname),
> - "qca/crbtfw%02x.tlv", rom_ver);
> - break;
> - case QCA_WCN3988:
> - snprintf(config.fwname, sizeof(config.fwname),
> - "qca/apbtfw%02x.tlv", rom_ver);
> - break;
> - case QCA_QCA2066:
> - snprintf(config.fwname, sizeof(config.fwname),
> - "qca/hpbtfw%02x.tlv", rom_ver);
> - break;
> - case QCA_QCA6390:
> - snprintf(config.fwname, sizeof(config.fwname),
> - "qca/htbtfw%02x.tlv", rom_ver);
> - break;
> - case QCA_WCN6750:
> - /* Choose mbn file by default.If mbn file is not found
> - * then choose tlv file
> - */
> - config.type = ELF_TYPE_PATCH;
> - snprintf(config.fwname, sizeof(config.fwname),
> - "qca/msbtfw%02x.mbn", rom_ver);
> - break;
> - case QCA_WCN6855:
> - snprintf(config.fwname, sizeof(config.fwname),
> - "qca/hpbtfw%02x.tlv", rom_ver);
> - break;
> - case QCA_WCN7850:
> - snprintf(config.fwname, sizeof(config.fwname),
> - "qca/hmtbtfw%02x.tlv", rom_ver);
> - break;
> - default:
> - snprintf(config.fwname, sizeof(config.fwname),
> - "qca/rampatch_%08x.bin", soc_ver);
> + if (rampatch_name) {
> + snprintf(config.fwname, sizeof(config.fwname), "qca/%s", rampatch_name);
> + } else {
> + switch (soc_type) {
> + case QCA_WCN3990:
> + case QCA_WCN3991:
> + case QCA_WCN3998:
> + snprintf(config.fwname, sizeof(config.fwname),
> + "qca/crbtfw%02x.tlv", rom_ver);
> + break;
> + case QCA_WCN3988:
> + snprintf(config.fwname, sizeof(config.fwname),
> + "qca/apbtfw%02x.tlv", rom_ver);
> + break;
> + case QCA_QCA2066:
> + snprintf(config.fwname, sizeof(config.fwname),
> + "qca/hpbtfw%02x.tlv", rom_ver);
> + break;
> + case QCA_QCA6390:
> + snprintf(config.fwname, sizeof(config.fwname),
> + "qca/htbtfw%02x.tlv", rom_ver);
> + break;
> + case QCA_WCN6750:
> + /* Choose mbn file by default.If mbn file is not found
> + * then choose tlv file
> + */
> + config.type = ELF_TYPE_PATCH;
> + snprintf(config.fwname, sizeof(config.fwname),
> + "qca/msbtfw%02x.mbn", rom_ver);
> + break;
> + case QCA_WCN6855:
> + snprintf(config.fwname, sizeof(config.fwname),
> + "qca/hpbtfw%02x.tlv", rom_ver);
> + break;
> + case QCA_WCN7850:
> + snprintf(config.fwname, sizeof(config.fwname),
> + "qca/hmtbtfw%02x.tlv", rom_ver);
> + break;
> + default:
> + snprintf(config.fwname, sizeof(config.fwname),
> + "qca/rampatch_%08x.bin", soc_ver);
> + }
> }
>
> err = qca_download_firmware(hdev, &config, soc_type, rom_ver);
> @@ -816,8 +881,15 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
> /* Download NVM configuration */
> config.type = TLV_TYPE_NVM;
> if (firmware_name) {
> - snprintf(config.fwname, sizeof(config.fwname),
> - "qca/%s", firmware_name);
> + /* The firmware name has suffix, use it directly */
> + suffix = strrchr(firmware_name, '.');
> + if (suffix && *(suffix + 1) != '\0' && *(suffix + 1) != '.') {
> + snprintf(config.fwname, sizeof(config.fwname), "qca/%s", firmware_name);
> + } else {
> + qca_read_fw_board_id(hdev, &boardid);
> + qca_get_nvm_name_by_board(config.fwname, sizeof(config.fwname),
> + firmware_name, ver, soc_type, boardid);
> + }
> } else {
> switch (soc_type) {
> case QCA_WCN3990:
> diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h
> index bb5207d7a..9d28c8800 100644
> --- a/drivers/bluetooth/btqca.h
> +++ b/drivers/bluetooth/btqca.h
> @@ -161,7 +161,7 @@ enum qca_btsoc_type {
> int qca_set_bdaddr_rome(struct hci_dev *hdev, const bdaddr_t *bdaddr);
> int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
> enum qca_btsoc_type soc_type, struct qca_btsoc_version ver,
> - const char *firmware_name);
> + const char *firmware_name, const char *rampatch_name);
> int qca_read_soc_version(struct hci_dev *hdev, struct qca_btsoc_version *ver,
> enum qca_btsoc_type);
> int qca_set_bdaddr(struct hci_dev *hdev, const bdaddr_t *bdaddr);
> @@ -176,7 +176,8 @@ static inline int qca_set_bdaddr_rome(struct hci_dev *hdev, const bdaddr_t *bdad
> static inline int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
> enum qca_btsoc_type soc_type,
> struct qca_btsoc_version ver,
> - const char *firmware_name)
> + const char *firmware_name,
> + const char *rampatch_name)
> {
> return -EOPNOTSUPP;
> }
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index 37129e6cb..c566d0bf2 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -229,6 +229,7 @@ struct qca_serdev {
> u32 oper_speed;
> bool bdaddr_property_broken;
> const char *firmware_name;
> + const char *rampatch_name;
> };
>
> static int qca_regulator_enable(struct qca_serdev *qcadev);
> @@ -264,6 +265,17 @@ static const char *qca_get_firmware_name(struct hci_uart *hu)
> }
> }
>
> +static const char *qca_get_rampatch_name(struct hci_uart *hu)
> +{
> + if (hu->serdev) {
> + struct qca_serdev *qsd = serdev_device_get_drvdata(hu->serdev);
> +
> + return qsd->rampatch_name;
> + } else {
> + return NULL;
> + }
> +}
> +
> static void __serial_clock_on(struct tty_struct *tty)
> {
> /* TODO: Some chipset requires to enable UART clock on client
> @@ -1855,6 +1867,7 @@ static int qca_setup(struct hci_uart *hu)
> unsigned int retries = 0;
> enum qca_btsoc_type soc_type = qca_soc_type(hu);
> const char *firmware_name = qca_get_firmware_name(hu);
> + const char *rampatch_name = qca_get_rampatch_name(hu);
> int ret;
> struct qca_btsoc_version ver;
> struct qca_serdev *qcadev;
> @@ -1963,7 +1976,7 @@ static int qca_setup(struct hci_uart *hu)
>
> /* Setup patch / NVM configurations */
> ret = qca_uart_setup(hdev, qca_baudrate, soc_type, ver,
> - firmware_name);
> + firmware_name, rampatch_name);
> if (!ret) {
> clear_bit(QCA_IBS_DISABLED, &qca->flags);
> qca_debugfs_init(hdev);
> @@ -2309,8 +2322,10 @@ static int qca_serdev_probe(struct serdev_device *serdev)
> qcadev->serdev_hu.serdev = serdev;
> data = device_get_match_data(&serdev->dev);
> serdev_device_set_drvdata(serdev, qcadev);
> - device_property_read_string(&serdev->dev, "firmware-name",
> - &qcadev->firmware_name);
> + of_property_read_string_index(serdev->dev.of_node, "firmware-name",
> + 0, &qcadev->firmware_name);
> + of_property_read_string_index(serdev->dev.of_node, "firmware-name",
> + 1, &qcadev->rampatch_name);
> device_property_read_u32(&serdev->dev, "max-speed",
> &qcadev->oper_speed);
> if (!qcadev->oper_speed)
> --
> 2.25.1
>

--
With best wishes
Dmitry

Next message: Russell King (Oracle): "Re: [PATCH net-next v1 2/7] net: ethtool: add support for structured PHY statistics"
Previous message: Md Sadre Alam: "[PATCH v2] dmaengine: qcom: bam_dma: Avoid writing unavailable register"
In reply to: Cheng Jiang: "[PATCH v3 2/3] Bluetooth: qca: Expand firmware-name to load specific nvm and rampatch"
Next in thread: Cheng Jiang (IOE): "Re: [PATCH v3 2/3] Bluetooth: qca: Expand firmware-name to load specific nvm and rampatch"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]