Re: [PATCH v5 2/4] Bluetooth: qca: Update firmware-name to support board specific nvm
From: Konrad Dybcio
Date: Fri Dec 20 2024 - 08:47:43 EST
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