Hi Paul,
Appreciate your comments.
>
> Dear Kiran,
>
>
> Thank you for your reply.
>
> Am 04.03.24 um 09:21 schrieb K, Kiran:
>
> >> -----Original Message-----
> >> From: Paul Menzel <pmenzel@xxxxxxxxxxxxx>
> >> Sent: Friday, March 1, 2024 4:38 PM
>
> […]
>
> >> Am 01.03.24 um 11:24 schrieb Kiran K:
> >>> Some variants of Intel controllers like BlazarI supports downloading
> >>> of
> >>
> >> support
> >>
> >> In the diff you write Blazar-I.
> >
> > Ok. I will fix it in the next patch.
> >>
> >>> secondary bootloader. This patch adds the support to download
> >>> secondary boot loader image before downloading operational firmware
> image.
> >>
> >> What is the secondary bootloader needed for?
> >
> > Primary bootloader is flashed over ROM and any issues found once the
> > product released to market is hard / impossible to fix. So idea is to
> > keep primary bootloader minimal and have secondary bootloader.
> Thank you. I think, that’d be good to have in the commit message.
>
Ack. I will update commit message.
> >>> Signed-off-by: Kiran K <kiran.k@xxxxxxxxx>
> >>> ---
> >>> dmesg logs:
> >>> [ 16.537130] Bluetooth: Core ver 2.22
> >>> [ 16.537135] Bluetooth: Starting self testing
> >>> [ 16.540021] Bluetooth: ECDH test passed in 2818 usecs
> >>> [ 16.560666] Bluetooth: SMP test passed in 602 usecs
> >>> [ 16.560674] Bluetooth: Finished self testing
> >>> [ 16.560690] Bluetooth: HCI device and connection manager initialized
> >>> [ 16.560695] Bluetooth: HCI socket layer initialized
> >>> [ 16.560697] Bluetooth: L2CAP socket layer initialized
> >>> [ 16.560700] Bluetooth: SCO socket layer initialized
> >>> [ 16.571934] Bluetooth: hci0: Device revision is 0
> >>> [ 16.571940] Bluetooth: hci0: Secure boot is disabled
> >>> [ 16.571941] Bluetooth: hci0: OTP lock is disabled
> >>> [ 16.571942] Bluetooth: hci0: API lock is enabled
> >>> [ 16.571943] Bluetooth: hci0: Debug lock is disabled
> >>> [ 16.571943] Bluetooth: hci0: Minimum firmware build 1 week 10 2014
> >>> [ 16.571945] Bluetooth: hci0: Bootloader timestamp 2022.46 buildtype 1
> build 26590
> >>> [ 16.572189] Bluetooth: hci0: DSM reset method type: 0x00
> >>> [ 16.575002] Bluetooth: hci0: Found device firmware: intel/ibt-0090-
> 0291-02.sfi
> >>> [ 16.575007] Bluetooth: hci0: Boot Address: 0x30099000
> >>> [ 16.575008] Bluetooth: hci0: Firmware Version: 200-10.24
> >>> [ 16.705698] Bluetooth: hci0: Waiting for firmware download to
> complete
> >>> [ 16.705927] Bluetooth: hci0: Firmware loaded in 127852 usecs
> >>
> >> Unrelated, but this is quite long.
> > I can pass on this information to firmware. I feel this seems to be OK
> > as the maximum timeout for firmware download is 5 seconds.
>
> Starting my system, in an ideal world it’s ready after at most one second
> (system firmware 0.5 seconds, Linux kernel 0.2 seconds + 0.3 user space). ;-)
> Ideally, Bluetooth would be operational by then.
>
> >>> [ 16.705952] Bluetooth: hci0: Waiting for device to boot
> >>> [ 16.708519] Bluetooth: hci0: Device booted in 2522 usecs
> >>> [ 16.708538] Bluetooth: hci0: Malformed MSFT vendor event: 0x02
> >>
> >> (Unrelated, but this is shown on a lot of devices. One more time
> >> below.)
> >>
> >>> [ 16.710296] Bluetooth: hci0: No device address configured
> >>> [ 16.712483] Bluetooth: hci0: Found device firmware: intel/ibt-0090-
> 0291.sfi
> >>> [ 16.712497] Bluetooth: hci0: Boot Address: 0x10000800
> >>> [ 16.712498] Bluetooth: hci0: Firmware Version: 211-10.24
> >
> >> It’s unclear from the logs, why two firmware files (with different
> >> versions) are loaded.
> >
> > One is secondary bootloader (ibt-0090-0291-02.sfi) and the other one
> > is operational firmware (ibt-0090-0291.sfi) . It's possible to have
> > different version.
> Understood. Could that be made more clear in the logging output?
>
Ack. I will use the string "iml" instead of image type - 02.
> >>> [ 16.930834] Bluetooth: BNEP (Ethernet Emulation) ver 1.3
> >>> [ 16.930840] Bluetooth: BNEP filters: protocol multicast
> >>> [ 16.930844] Bluetooth: BNEP socket layer initialized
> >>> [ 18.494137] Bluetooth: hci0: Waiting for firmware download to
> complete
> >>> [ 18.494897] Bluetooth: hci0: Firmware loaded in 1740634 usecs
> >>
> >> Hmm, 1.7 seconds is very long.
> >>
> >>> [ 18.494972] Bluetooth: hci0: Waiting for device to boot
> >>> [ 18.529089] Bluetooth: hci0: Device booted in 33371 usecs
> >>> [ 18.529121] Bluetooth: hci0: Malformed MSFT vendor event: 0x02
> >>> [ 18.529914] Bluetooth: hci0: Found Intel DDC parameters: intel/ibt-
> 0090-0291.ddc
> >>> [ 18.532158] Bluetooth: hci0: Applying Intel DDC parameters completed
> >>> [ 18.532582] Bluetooth: hci0: Found Intel DDC parameters:
> intel/bdaddress.cfg
> >>> [ 18.534109] Bluetooth: hci0: Applying Intel DDC parameters completed
> >>> [ 18.537170] Bluetooth: hci0: Firmware timestamp 2024.9 buildtype 0
> build 58067
> >>> [ 18.537177] Bluetooth: hci0: Firmware SHA1: 0x81abf1ea
> >>> [ 18.540985] Bluetooth: hci0: Fseq status: Success (0x00)
> >>> [ 18.540992] Bluetooth: hci0: Fseq executed: 00.00.00.00
> >>> [ 18.540993] Bluetooth: hci0: Fseq BT Top: 00.00.00.00
> >>> [ 18.631360] Bluetooth: MGMT ver 1.22
> >>> [ 18.673023] Bluetooth: RFCOMM TTY layer initialized
> >>> [ 18.673031] Bluetooth: RFCOMM socket layer initialized
> >>> [ 18.673039] Bluetooth: RFCOMM ver 1.11
> >>
> >> Thank you for pasting this. It’d be great if you added it to the
> >> commit message, so above ---.
> >
> > Ok. I will have it part of commit message.
>
> Thank you.
>
> By the way, I had written some more comments below. It looks like you
> overlooked them.
>
Yes. My bad. Please see the response below.
> >>> drivers/bluetooth/btintel.c | 38
> ++++++++++++++++++++++++++++++++++++-
> >>> 1 file changed, 37 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/bluetooth/btintel.c
> >>> b/drivers/bluetooth/btintel.c index 6ba7f5d1b837..934aad89bbf1
> >>> 100644
> >>> --- a/drivers/bluetooth/btintel.c
> >>> +++ b/drivers/bluetooth/btintel.c
> >>> @@ -521,6 +521,9 @@ static int btintel_version_info_tlv(struct
> >>> hci_dev
> >> *hdev,
> >>> version->min_fw_build_nn, version-
> min_fw_build_cw,
> >>> 2000 + version->min_fw_build_yy);
> >>> break;
> >>> + case 0x02:
> >>> + variant = "IML";
> >>
> >> What does IML mean?
Intermediate loader.
> >>
> >>> + break;
> >>> case 0x03:
> >>> variant = "Firmware";
> >>> break;
> >>> @@ -2194,10 +2197,26 @@ static void btintel_get_fw_name_tlv(const
> >> struct intel_version_tlv *ver,
> >>> char *fw_name, size_t len,
> >>> const char *suffix)
> >>> {
> >>> + const char *format;
> >>> /* The firmware file name for new generation controllers will be
> >>> * ibt-<cnvi_top type+cnvi_top step>-<cnvr_top type+cnvr_top step>
> >>> */
> >>> - snprintf(fw_name, len, "intel/ibt-%04x-%04x.%s",
> >>> + switch (INTEL_HW_VARIANT(ver->cnvi_bt)) {
> >>> + /* Only Blazar-I (0x1e) product supports downloading of secondary
> boot
> >>> + * loader image
> >>> + */
> >>> + case 0x1e:
> >>
> >> Should a macro be defined for 0x1e?
Ack.
> >>
> >>> + if (ver->img_type == 1)
> >>
> >> Below you write 0x0x. Should this be consistent?
Ack
> >>
> >>> + format = "intel/ibt-%04x-%04x-02.%s";
> >>> + else
> >>> + format = "intel/ibt-%04x-%04x.%s";
> >>> + break;
> >>> + default:
> >>> + format = "intel/ibt-%04x-%04x.%s";
> >>> + break;
> >>> + }
> >>> +
> >>> + snprintf(fw_name, len, format,
> >>> INTEL_CNVX_TOP_PACK_SWAB(INTEL_CNVX_TOP_TYPE(ver-
> cnvi_top),
> >>> INTEL_CNVX_TOP_STEP(ver-
> cnvi_top)),
> >>> INTEL_CNVX_TOP_PACK_SWAB(INTEL_CNVX_TOP_TYPE(ver-
> cnvr_top),
> >>> @@ -2607,6 +2626,23 @@ static int
> >>> btintel_bootloader_setup_tlv(struct
> >> hci_dev *hdev,
> >>> if (err)
> >>> return err;
> >>>
> >>> + err = btintel_read_version_tlv(hdev, ver);
> >>> + if (err)
> >>> + return err;
> >>> +
> >>> + /* If image type returned is 0x02, then controller supports secondary
> >>> + * boot loader image
> >>> + */
> >>> + if (ver->img_type == 0x02) {
> >>
> >> Could a macro be defined for 0x02?
Ack
> >>
> >>> + err = btintel_prepare_fw_download_tlv(hdev, ver,
> &boot_param);
> >>> + if (err)
> >>> + return err;
> >>> +
> >>> + err = btintel_boot(hdev, boot_param);
> >>> + if (err)
> >>> + return err;
> >>> + }
> >>> +
> >>> btintel_clear_flag(hdev, INTEL_BOOTLOADER);
> >>>
> >>> btintel_get_fw_name_tlv(ver, ddcname, sizeof(ddcname), "ddc");
>
>
> Kind regards,
>
> Paul
Thanks,
Kiran
