| Message ID | 20241212150232.3823088-3-quic_chejiang@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Expand firmware-name property to load specific | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | success | Success |
On 12.12.2024 4:02 PM, Cheng Jiang wrote: > 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. > > 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"; > firmware-name = "QCA6698/hpnv21.bin"; I think we should agree on one and then do some magic to look up the other variants. > > Signed-off-by: Cheng Jiang <quic_chejiang@quicinc.com> > --- > drivers/bluetooth/btqca.c | 112 ++++++++++++++++++++++++++++---------- > 1 file changed, 84 insertions(+), 28 deletions(-) > > diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c > index dfbbac922..4842f4335 100644 > --- a/drivers/bluetooth/btqca.c > +++ b/drivers/bluetooth/btqca.c > @@ -272,6 +272,38 @@ int qca_send_pre_shutdown_cmd(struct hci_dev *hdev) > } > EXPORT_SYMBOL_GPL(qca_send_pre_shutdown_cmd); > > +static bool qca_filename_has_extension(const char *filename) > +{ > + const char *suffix; > + > + suffix = strrchr(filename, '.'); > + if (suffix && suffix != filename && *(suffix + 1) != '\0' && strchr(suffix, '/') == NULL) > + return true; > + else > + return false; How about: const char *suffix = strrchr(filename, '.'); /* File extensions require a dot, but not as the last character */ if (!suffix || *(suffix + 1) == NULL) return false; /* Avoid matching directories with names that look like files with extensions */ return !(suffix, '/'); > + } > + /* 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. nvm appears 4 times in two lines, how about: /* * If the board-specific file is missing, try loading the default * one, unless that was attempted already */ But, even more importantly: a) do we want to load the "incorrect" file? b) why would we want to specify the .bin file if it's the default anyway? > + */ > + else if (config->type == TLV_TYPE_NVM && > + qca_get_alt_nvm_file(config->fwname, sizeof(config->fwname))) { > + 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); > @@ -700,34 +745,38 @@ static int qca_check_bdaddr(struct hci_dev *hdev, const struct qca_fw_config *co > return 0; > } > > -static void qca_generate_hsp_nvm_name(char *fwname, size_t max_size, > +static void qca_get_nvm_name_by_board(char *fwname, size_t max_size, > + const char *stem, enum qca_btsoc_type soc_type, > struct qca_btsoc_version ver, u8 rom_ver, u16 bid) > { > const char *variant; > + const char *prefix; > > - /* hsp gf chip */ > - if ((le32_to_cpu(ver.soc_id) & QCA_HSP_GF_SOC_MASK) == QCA_HSP_GF_SOC_ID) > - variant = "g"; > - else > - variant = ""; > + /* Set the defalut value to variant and prefixt */ typos: default, prefix > + variant = ""; > + prefix = "b"; > > - if (bid == 0x0) > - snprintf(fwname, max_size, "qca/hpnv%02x%s.bin", rom_ver, variant); > - else > - snprintf(fwname, max_size, "qca/hpnv%02x%s.%x", rom_ver, variant, bid); > -} > + if (soc_type == QCA_QCA2066) > + prefix = ""; > > -static inline void qca_get_nvm_name_generic(struct qca_fw_config *cfg, > - const char *stem, u8 rom_ver, u16 bid) > -{ > - if (bid == 0x0) > - snprintf(cfg->fwname, sizeof(cfg->fwname), "qca/%snv%02x.bin", stem, rom_ver); > - else if (bid & 0xff00) > - snprintf(cfg->fwname, sizeof(cfg->fwname), > - "qca/%snv%02x.b%x", stem, rom_ver, bid); > - else > - snprintf(cfg->fwname, sizeof(cfg->fwname), > - "qca/%snv%02x.b%02x", stem, rom_ver, bid); > + if (soc_type == QCA_WCN6855 || soc_type == QCA_QCA2066) { > + /* hsp gf chip */ This is a good opportunity to explain what that means Konrad
Hi Konrad, On 12/13/2024 8:17 AM, Konrad Dybcio wrote: > On 12.12.2024 4:02 PM, Cheng Jiang wrote: >> 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. >> >> 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"; >> firmware-name = "QCA6698/hpnv21.bin"; > > I think we should agree on one and then do some magic to look up > the other variants. > These two possible configurations in DT to specific the NVM file. In the existing driver implementation, the firmware-name will specify a exactly nvm file. Driver will loaded it directory. But on some platform, we may attach different board, then need load the board-specific nvm. We assume if the nvm file has an extension, user should know the exactly connectivity board to attach to the platform, then the exactly nvm file will be loaded. This keeps back compatible. If user think different connectivity boards will be attached to the platform, the nvm should depends on the board id. Then just leave the extension empty. Driver will append the board id info as extension. >> >> Signed-off-by: Cheng Jiang <quic_chejiang@quicinc.com> >> --- >> drivers/bluetooth/btqca.c | 112 ++++++++++++++++++++++++++++---------- >> 1 file changed, 84 insertions(+), 28 deletions(-) >> >> diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c >> index dfbbac922..4842f4335 100644 >> --- a/drivers/bluetooth/btqca.c >> +++ b/drivers/bluetooth/btqca.c >> @@ -272,6 +272,38 @@ int qca_send_pre_shutdown_cmd(struct hci_dev *hdev) >> } >> EXPORT_SYMBOL_GPL(qca_send_pre_shutdown_cmd); >> >> +static bool qca_filename_has_extension(const char *filename) >> +{ >> + const char *suffix; >> + >> + suffix = strrchr(filename, '.'); >> + if (suffix && suffix != filename && *(suffix + 1) != '\0' && strchr(suffix, '/') == NULL) >> + return true; >> + else >> + return false; > > How about: > > const char *suffix = strrchr(filename, '.'); > > /* File extensions require a dot, but not as the last character */ > if (!suffix || *(suffix + 1) == NULL) > return false; > > /* Avoid matching directories with names that look like files with extensions */ > return !(suffix, '/'); > > Ack. Thanks! >> + } >> + /* 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. > > nvm appears 4 times in two lines, how about: > > /* > * If the board-specific file is missing, try loading the default > * one, unless that was attempted already > */ > > But, even more importantly: > > a) do we want to load the "incorrect" file? Normally, there is a default NVM file ending with .bin, which is suitable for most boards. However, some boards have different configurations that require a specific NVM. If a board-specific NVM is not found, a default NVM is preferred over not loading any NVM. > b) why would we want to specify the .bin file if it's the default anyway? The default NVM directory is the root of qca. The 'firmware-name' property can specify an NVM file in another directory. This can be either a default NVM like 'QCA6698/hpnv21.bin' or a board-specific NVM like 'QCA6698/hpnv21.b205'. > >> + */ >> + else if (config->type == TLV_TYPE_NVM && >> + qca_get_alt_nvm_file(config->fwname, sizeof(config->fwname))) { >> + 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); >> @@ -700,34 +745,38 @@ static int qca_check_bdaddr(struct hci_dev *hdev, const struct qca_fw_config *co >> return 0; >> } >> >> -static void qca_generate_hsp_nvm_name(char *fwname, size_t max_size, >> +static void qca_get_nvm_name_by_board(char *fwname, size_t max_size, >> + const char *stem, enum qca_btsoc_type soc_type, >> struct qca_btsoc_version ver, u8 rom_ver, u16 bid) >> { >> const char *variant; >> + const char *prefix; >> >> - /* hsp gf chip */ >> - if ((le32_to_cpu(ver.soc_id) & QCA_HSP_GF_SOC_MASK) == QCA_HSP_GF_SOC_ID) >> - variant = "g"; >> - else >> - variant = ""; >> + /* Set the defalut value to variant and prefixt */ > > typos: default, prefix > Ack. >> + variant = ""; >> + prefix = "b"; >> >> - if (bid == 0x0) >> - snprintf(fwname, max_size, "qca/hpnv%02x%s.bin", rom_ver, variant); >> - else >> - snprintf(fwname, max_size, "qca/hpnv%02x%s.%x", rom_ver, variant, bid); >> -} >> + if (soc_type == QCA_QCA2066) >> + prefix = ""; >> >> -static inline void qca_get_nvm_name_generic(struct qca_fw_config *cfg, >> - const char *stem, u8 rom_ver, u16 bid) >> -{ >> - if (bid == 0x0) >> - snprintf(cfg->fwname, sizeof(cfg->fwname), "qca/%snv%02x.bin", stem, rom_ver); >> - else if (bid & 0xff00) >> - snprintf(cfg->fwname, sizeof(cfg->fwname), >> - "qca/%snv%02x.b%x", stem, rom_ver, bid); >> - else >> - snprintf(cfg->fwname, sizeof(cfg->fwname), >> - "qca/%snv%02x.b%02x", stem, rom_ver, bid); >> + if (soc_type == QCA_WCN6855 || soc_type == QCA_QCA2066) { >> + /* hsp gf chip */ > > This is a good opportunity to explain what that means > Ack. This means the chip is produced by GlobalFoundries. > Konrad
On 13.12.2024 8:05 AM, Cheng Jiang (IOE) wrote: [...] >> /* >> * If the board-specific file is missing, try loading the default >> * one, unless that was attempted already >> */ >> >> But, even more importantly: >> >> a) do we want to load the "incorrect" file? > Normally, there is a default NVM file ending with .bin, which is suitable > for most boards. However, some boards have different configurations that > require a specific NVM. If a board-specific NVM is not found, a default > NVM is preferred over not loading any NVM. So, if one is specified, but not found, this should either be a loud error, or a very loud warning & fallback. Otherwise, the device may provide subpar user experience without the user getting a chance to know the reason. I think failing is better here, as that sends a clearer message, and would only happen if the DT has a specific path (meaning the user put some intentions behind that choice) >> b) why would we want to specify the .bin file if it's the default anyway? > The default NVM directory is the root of qca. The 'firmware-name' property > can specify an NVM file in another directory. This can be either a default > NVM like 'QCA6698/hpnv21.bin' or a board-specific NVM like 'QCA6698/hpnv21.b205'. Do we expect QCA6698/hpnv21.bin and QCAabcd/hpnv21.bin to be compatible? [...] >>> -static inline void qca_get_nvm_name_generic(struct qca_fw_config *cfg, >>> - const char *stem, u8 rom_ver, u16 bid) >>> -{ >>> - if (bid == 0x0) >>> - snprintf(cfg->fwname, sizeof(cfg->fwname), "qca/%snv%02x.bin", stem, rom_ver); >>> - else if (bid & 0xff00) >>> - snprintf(cfg->fwname, sizeof(cfg->fwname), >>> - "qca/%snv%02x.b%x", stem, rom_ver, bid); >>> - else >>> - snprintf(cfg->fwname, sizeof(cfg->fwname), >>> - "qca/%snv%02x.b%02x", stem, rom_ver, bid); >>> + if (soc_type == QCA_WCN6855 || soc_type == QCA_QCA2066) { >>> + /* hsp gf chip */ >> >> This is a good opportunity to explain what that means >> > Ack. This means the chip is produced by GlobalFoundries. Please update the comment there Konrad
diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c index dfbbac922..4842f4335 100644 --- a/drivers/bluetooth/btqca.c +++ b/drivers/bluetooth/btqca.c @@ -272,6 +272,38 @@ int qca_send_pre_shutdown_cmd(struct hci_dev *hdev) } EXPORT_SYMBOL_GPL(qca_send_pre_shutdown_cmd); +static bool qca_filename_has_extension(const char *filename) +{ + const char *suffix; + + suffix = strrchr(filename, '.'); + if (suffix && suffix != filename && *(suffix + 1) != '\0' && strchr(suffix, '/') == NULL) + return true; + else + return false; +} + +static bool qca_get_alt_nvm_file(char *filename, size_t max_size) +{ + char fwname[64]; + const char *suffix; + + /* nvm file name has an extension, replace with .bin */ + if (qca_filename_has_extension(filename)) { + suffix = strrchr(filename, '.'); + strscpy(fwname, filename, suffix - filename + 1); + snprintf(fwname + (suffix - filename), + sizeof(fwname) - (suffix - filename), ".bin"); + /* If nvm file is already the default one, return false to skip the retry. */ + if (strcmp(fwname, filename) == 0) + return false; + + snprintf(filename, max_size, "%s", fwname); + return true; + } + return false; +} + static int qca_tlv_check_data(struct hci_dev *hdev, struct qca_fw_config *config, u8 *fw_data, size_t fw_size, @@ -564,6 +596,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_file(config->fwname, sizeof(config->fwname))) { + 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); @@ -700,34 +745,38 @@ static int qca_check_bdaddr(struct hci_dev *hdev, const struct qca_fw_config *co return 0; } -static void qca_generate_hsp_nvm_name(char *fwname, size_t max_size, +static void qca_get_nvm_name_by_board(char *fwname, size_t max_size, + const char *stem, enum qca_btsoc_type soc_type, struct qca_btsoc_version ver, u8 rom_ver, u16 bid) { const char *variant; + const char *prefix; - /* hsp gf chip */ - if ((le32_to_cpu(ver.soc_id) & QCA_HSP_GF_SOC_MASK) == QCA_HSP_GF_SOC_ID) - variant = "g"; - else - variant = ""; + /* Set the defalut value to variant and prefixt */ + variant = ""; + prefix = "b"; - if (bid == 0x0) - snprintf(fwname, max_size, "qca/hpnv%02x%s.bin", rom_ver, variant); - else - snprintf(fwname, max_size, "qca/hpnv%02x%s.%x", rom_ver, variant, bid); -} + if (soc_type == QCA_QCA2066) + prefix = ""; -static inline void qca_get_nvm_name_generic(struct qca_fw_config *cfg, - const char *stem, u8 rom_ver, u16 bid) -{ - if (bid == 0x0) - snprintf(cfg->fwname, sizeof(cfg->fwname), "qca/%snv%02x.bin", stem, rom_ver); - else if (bid & 0xff00) - snprintf(cfg->fwname, sizeof(cfg->fwname), - "qca/%snv%02x.b%x", stem, rom_ver, bid); - else - snprintf(cfg->fwname, sizeof(cfg->fwname), - "qca/%snv%02x.b%02x", stem, rom_ver, bid); + if (soc_type == QCA_WCN6855 || soc_type == QCA_QCA2066) { + /* hsp gf chip */ + if ((le32_to_cpu(ver.soc_id) & QCA_HSP_GF_SOC_MASK) == QCA_HSP_GF_SOC_ID) + variant = "g"; + } + + if (rom_ver != 0) { + if (bid == 0x0 || bid == 0xffff) + snprintf(fwname, max_size, "qca/%s%02x%s.bin", stem, rom_ver, variant); + else + snprintf(fwname, max_size, "qca/%s%02x%s.%s%02x", stem, rom_ver, + variant, prefix, bid); + } else { + if (bid == 0x0 || bid == 0xffff) + snprintf(fwname, max_size, "qca/%s%s.bin", stem, variant); + else + snprintf(fwname, max_size, "qca/%s%s.%s%02x", stem, variant, prefix, bid); + } } int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate, @@ -816,8 +865,14 @@ 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 an extension, use it directly */ + if (qca_filename_has_extension(firmware_name)) { + 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, soc_type, ver, 0, boardid); + } } else { switch (soc_type) { case QCA_WCN3990: @@ -836,8 +891,9 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate, "qca/apnv%02x.bin", rom_ver); break; case QCA_QCA2066: - qca_generate_hsp_nvm_name(config.fwname, - sizeof(config.fwname), ver, rom_ver, boardid); + qca_get_nvm_name_by_board(config.fwname, + sizeof(config.fwname), "hpnv", soc_type, ver, + rom_ver, boardid); break; case QCA_QCA6390: snprintf(config.fwname, sizeof(config.fwname), @@ -852,9 +908,9 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate, "qca/hpnv%02x.bin", rom_ver); break; case QCA_WCN7850: - qca_get_nvm_name_generic(&config, "hmt", rom_ver, boardid); + qca_get_nvm_name_by_board(config.fwname, sizeof(config.fwname), + "hmtnv", soc_type, ver, rom_ver, boardid); break; - default: snprintf(config.fwname, sizeof(config.fwname), "qca/nvm_%08x.bin", soc_ver);
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. 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"; firmware-name = "QCA6698/hpnv21.bin"; Signed-off-by: Cheng Jiang <quic_chejiang@quicinc.com> --- drivers/bluetooth/btqca.c | 112 ++++++++++++++++++++++++++++---------- 1 file changed, 84 insertions(+), 28 deletions(-)