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