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@xxxxxxxxxxx>
>> ---
>> 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
