Message-ID: <ccce8ed3-1bf9-434c-bcb9-943d380544a2@quicinc.com>
Date: Tue, 10 Dec 2024 23:03:35 +0800
From: "Cheng Jiang (IOE)" <quic_chejiang@...cinc.com>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
CC: Marcel Holtmann <marcel@...tmann.org>,
        Luiz Augusto von Dentz
	<luiz.dentz@...il.com>,
        Rob Herring <robh@...nel.org>,
        Krzysztof Kozlowski
	<krzk+dt@...nel.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Bjorn Andersson
	<andersson@...nel.org>,
        Konrad Dybcio <konradybcio@...nel.org>,
        "Balakrishna
 Godavarthi" <quic_bgodavar@...cinc.com>,
        Rocky Liao
	<quic_rjliao@...cinc.com>,
        <linux-bluetooth@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <linux-arm-msm@...r.kernel.org>,
        <quic_jiaymao@...cinc.com>, <quic_shuaz@...cinc.com>,
        <quic_zijuhu@...cinc.com>, <quic_mohamull@...cinc.com>
Subject: Re: [PATCH v3 2/3] Bluetooth: qca: Expand firmware-name to load
 specific nvm and rampatch

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@...cinc.com> 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@...cinc.com>
>>>>>> ---
>>>>>>  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);
>>>>>> +}
>>>>>> +
>>>
>>>
>>
> 

Powered by blists - more mailing lists