Message-ID: <h7a537yfel7oq4hh4lz5mo4qt6bsy5az6xl4crusxlmoa5een3@iuvk5ckcta2y>
Date: Tue, 10 Dec 2024 13:12:53 +0200
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: "Cheng Jiang (IOE)" <quic_chejiang@...cinc.com>
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

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@...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.

Ack, thanks for the confirmation.

Please post the next iteration, I'll R-B it.

-- 
With best wishes
Dmitry

Powered by blists - more mailing lists