Message-ID: <1e0fc6f8-9f5d-4f62-a379-ea9b0161fc84@quicinc.com>
Date: Tue, 10 Dec 2024 10:00:51 +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/10/2024 12:04 AM, Dmitry Baryshkov wrote:
> On Mon, 9 Dec 2024 at 15:59, Cheng Jiang (IOE)
> <quic_chejiang@...cinc.com> wrote:
>>
>> 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.
> 
> You are re-insmodding the driver. I was talking about a different scenario:
> - there is no BDF
> - modprobe the driver
> - wait for the hci0 to become available
> - hciconfig hci0 down
> - provide BDF
> - hciconfig hci0 up
> 
> Check the dmesg. If everything is implemented correctly, second
> hciconfig command should load the firmware files again (because BT was
> unpowered in between). Second time it should load the proper board
> file instead of loading the default or falling back to the ROM.
> 
Yes, the 'hciconfig hci0 up' will load the proper board file, since it also follows 
the flow described above. 

Here is the dmesg:

sh-5.1# mv hpnv21.b206 hpnv21.b2069                 -- Remove the board specific nvm
sh-5.1# rmmod hci_uart
sh-5.1# insmod /lib/modules/6.6.52-dirty/kernel/drivers/bluetooth/hci_uart.ko
sh-5.1# dmesg|grep -i bluetooth

[54781.019527] Bluetooth: HCI UART driver ver 2.3
[54781.019538] Bluetooth: HCI UART protocol H4 registered
[54781.019589] Bluetooth: HCI UART protocol LL registered
[54781.019612] Bluetooth: HCI UART protocol QCA registered
[54781.020893] Bluetooth: hci0: setting up wcn6855
[54781.087027] Bluetooth: hci0: QCA Product ID   :0x00000013
[54781.087037] Bluetooth: hci0: QCA SOC Version  :0x400c0210
[54781.087039] Bluetooth: hci0: QCA ROM Version  :0x00000201
[54781.087042] Bluetooth: hci0: QCA Patch Version:0x000038e6
[54781.104087] Bluetooth: hci0: QCA controller version 0x02100201
[54781.104097] Bluetooth: hci0: QCA Downloading qca/QCA6698/hpbtfw21.tlv
[54781.794628] Bluetooth: hci0: QCA Downloading qca/QCA6698/hpnv21.b206
[54781.794671] bluetooth hci0: Direct firmware load for qca/QCA6698/hpnv21.b206 failed with error -2
[54781.794677] Bluetooth: hci0: QCA Downloading qca/QCA6698/hpnv21.bin
[54781.958319] Bluetooth: hci0: QCA setup on UART is completed
[54781.981490] Bluetooth: MGMT ver 1.22

No board specific nvm found, use the default one.
Disable hci0 and add the board specific nvm, then enable hci0.

sh-5.1# hciconfig hci0 down
sh-5.1# mv hpnv21.b2069 hpnv21.b206
sh-5.1# hciconfig hci0 up
sh-5.1# dmesg|grep -i bluetooth
[54834.686170] Bluetooth: hci0: setting up wcn6855
[54834.750997] Bluetooth: hci0: QCA Product ID   :0x00000013
[54834.751006] Bluetooth: hci0: QCA SOC Version  :0x400c0210
[54834.751010] Bluetooth: hci0: QCA ROM Version  :0x00000201
[54834.751013] Bluetooth: hci0: QCA Patch Version:0x000038e6
[54834.761826] Bluetooth: hci0: QCA controller version 0x02100201
[54834.761833] Bluetooth: hci0: QCA Downloading qca/QCA6698/hpbtfw21.tlv
[54835.450621] Bluetooth: hci0: QCA Downloading qca/QCA6698/hpnv21.b206
[54835.614015] Bluetooth: hci0: QCA setup on UART is completed

Load the board-specific nvm when enable hci0.

>> [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