Hi Dmitry,
On 12/9/2024 6:49 PM, Dmitry Baryshkov wrote:
> On Mon, Dec 09, 2024 at 05:03:55PM +0800, Cheng Jiang (IOE) wrote:
>> Hi Dmitry,
>>
>> On 12/6/2024 4:34 PM, Dmitry Baryshkov wrote:
>>> On Fri, 6 Dec 2024 at 05:05, Cheng Jiang (IOE)
>>> <quic_chejiang@xxxxxxxxxxx> wrote:
>>>>
>>>> Hi Dmitry,
>>>>
>>>> On 12/5/2024 8:00 PM, Dmitry Baryshkov wrote:
>>>>> 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.
>>>>>
>>>> Ack.
>>>>>>
>>>>>> 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.
>>>>>
>>>> Ack.
>>>>>> +{
>>>>>> + 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?
>>>>>
>>>> Yes, in current driver, the max f/w path length is 64.
>>>>
>>>>>> + fwname[suffix - path] = 0;
>>>>>
>>>>> with path = "qcom/sc7180/Oh.My.Device/name" this is broken.
>>>>>
>>>> Let me test this and fix in next patch.
>>>>>> +
>>>>>> + 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).
>>>>>
>>>> This tries to load a default NVM file if the board-specific NVM is not found.
>>>> It is called when request_firmware fails. It's safe to rewrite the config->fwname
>>>> here since we have already tried to load the board-specific NVM. The config
>>>> is a local variable in qca_uart_setup and will return after downloading the NVM.
>>>
>>> Please read my question before answering it.
>>>
>> Sorry, I'm not clear about your question. Could you please explain it in more detail?
>> I'm not quite sure how the situation you mentioned affects this code flow if you mean
>> not downloading another NVM file.
>>
>> The board-specific NVM and the default NVM should be in the same folder and should
>> appear simultaneously.
>>
>> From the Bluetooth firmware load flow perspective, the firmware is loaded either
>> when the kernel module is inserted (insmod) or when Bluetooth is turned off and
>> then on again via a user-space command. If the firmware is not found at this time,
>> the ROM code is used instead. It does not attempt to load the firmware automatically,
>> even if the firmware appears later.
>
> I was thinking about the following scenario:
>
> - BT firmware is attempted to load during driver probe, /lib/firmware is
> not fully populated, so the config is rewritten to use the default
> - rootfs is fully mounted and populated with the board-specific file
> - BT interface is being turned on. It is expected that the
> board-specific file will be loaded, however because the config was
> changed in one of the previous steps, the driver still loads the
> default one.
>
> That said, the driver should perform the fallback, etc, but the config
> should stay intact even in the fallback case.
>
>>
>>>>>> + 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?
>>>>>
>>>> Yes, miss this part. Thank you!
>>>>>> + }
>>>>>> +
>>>>>> + if (bid == 0x0)
>>>>>
>>>>> 0x0 or 0xff?
>>>> board is set to 0 by default, 0x0 means read board id fails, then we should use
>>>> the default one.
>>>
>>> What is the 'unprogrammed' board_id? On the WiFi side it's usually 0xff.
>>>
>> Yes, the 'unprogrammed' board_id should be 0xffff. Then 0 and 0xffff should use the
>> default nvm.
>
> Good. I think it's safe to safe board_id to 0xffff by default, then you
> don't have to handle '0' specially.
>
Sorry, I missed this comment, we have read board id of 0 from some boards in other project.
So it's better to check both 0 and 0xffff. It aligns with current driver implementation.
>>>>>
>>>>>> + 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?
>>>>>
>>>> No, board id are two bytes, it coudl be 0x0206, then we need .b206. Or it is
>>>> 0x000a, then we need .b0a.
>>>
>>> What will ".b%02x" write in those two cases?
>>>
>> Yes, it works for both cases. Thanks!
>
> :-)
>
>>>>>> + else
>>>>>> + snprintf(fwname, max_size, "qca/%s.b%02x", firmware_name, bid);
>>>>>> +}
>>>>>> +
>>>
>>>
>>
>
