Message-ID: <f7dd3758-c1c8-43bb-9a5c-4674077a5e1b@quicinc.com>
Date: Mon, 9 Dec 2024 21:59:29 +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.
> 
Thank you for the detail explanation. Current flow of BT enable in driver
likes this:

Enable the soc(Assert BT_EN) -->  read the SOC info --> Change baud rate --> 
get rampatch file name (based on soc info or dts) --> download rampatch --> 
get nvm file name(based on soc info or dts) --> download nvm file --> 
download default nvm (if the board-specific file not found).

Every time the driver probe or the BT interface is turned on, it follows the
flow described above. The rampatch and NVM file names are reconstructed by 
the SoC information each time, so the driver always attempts to download the 
board-specific file first.

Here is the log, there is no hpnv21.b206 and re-insmod the driver. 
[11850.644220] Bluetooth: HCI UART driver ver 2.3
[11850.644232] Bluetooth: HCI UART protocol H4 registered
[11850.644284] Bluetooth: HCI UART protocol LL registered
[11850.644314] Bluetooth: HCI UART protocol QCA registered
[11850.645055] Bluetooth: hci0: setting up wcn6855
[11850.706962] Bluetooth: hci0: QCA Product ID   :0x00000013
[11850.706975] Bluetooth: hci0: QCA SOC Version  :0x400c0210
[11850.706978] Bluetooth: hci0: QCA ROM Version  :0x00000201
[11850.706981] Bluetooth: hci0: QCA Patch Version:0x000038e6
[11850.714508] Bluetooth: hci0: QCA controller version 0x02100201
[11850.714518] Bluetooth: hci0: QCA Downloading qca/QCA6698/hpbtfw21.tlv
[11851.406475] Bluetooth: hci0: QCA Downloading qca/QCA6698/hpnv21.b206
[11851.406515] bluetooth hci0: Direct firmware load for qca/QCA6698/hpnv21.b206 failed with error -2
[11851.406522] Bluetooth: hci0: QCA Downloading qca/QCA6698/hpnv21.bin
[11851.570125] Bluetooth: hci0: QCA setup on UART is completed

hpnv21.b206 exists and then re-insmod the driver.
[11878.551494] Bluetooth: HCI UART driver ver 2.3
[11878.551505] Bluetooth: HCI UART protocol H4 registered
[11878.551553] Bluetooth: HCI UART protocol LL registered
[11878.551580] Bluetooth: HCI UART protocol QCA registered
[11878.552131] Bluetooth: hci0: setting up wcn6855
[11878.618865] Bluetooth: hci0: QCA Product ID   :0x00000013
[11878.618877] Bluetooth: hci0: QCA SOC Version  :0x400c0210
[11878.618881] Bluetooth: hci0: QCA ROM Version  :0x00000201
[11878.618884] Bluetooth: hci0: QCA Patch Version:0x000038e6
[11878.629674] Bluetooth: hci0: QCA controller version 0x02100201
[11878.629681] Bluetooth: hci0: QCA Downloading qca/QCA6698/hpbtfw21.tlv
[11879.318475] Bluetooth: hci0: QCA Downloading qca/QCA6698/hpnv21.b206
[11879.482082] Bluetooth: hci0: QCA setup on UART is completed
[11879.505086] Bluetooth: MGMT ver 1.22

Turn on BT has the similar log.
>>
>>>>>> +                    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.
> 
>>>>>
>>>>>> +            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