On Tue, Dec 10, 2024 at 10:00:51AM +0800, Cheng Jiang (IOE) wrote: > 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. Ack, thanks for the confirmation. Please post the next iteration, I'll R-B it. -- With best wishes Dmitry