diff mbox series

[v1] Bluetooth: btintel: Add support for downloading secondary boot loader image

Message ID 20240301102412.11151-1-kiran.k@intel.com
State Superseded
Headers show
Series [v1] Bluetooth: btintel: Add support for downloading secondary boot loader image | expand

Commit Message

K, Kiran March 1, 2024, 10:24 a.m. UTC
Some variants of Intel controllers like BlazarI supports downloading of
secondary bootloader. This patch adds the support to download secondary
boot loader image before downloading operational firmware image.

Signed-off-by: Kiran K <kiran.k@intel.com>
---
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
[   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
[   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
[   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
[   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

 drivers/bluetooth/btintel.c | 38 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)

Comments

K, Kiran March 4, 2024, 8:21 a.m. UTC | #1
Hi Paul,

Thanks for your comments.

> -----Original Message-----
> From: Paul Menzel <pmenzel@molgen.mpg.de>
> Sent: Friday, March 1, 2024 4:38 PM
> To: K, Kiran <kiran.k@intel.com>
> Cc: linux-bluetooth@vger.kernel.org; Srivatsa, Ravishankar
> <ravishankar.srivatsa@intel.com>; Tumkur Narayan, Chethan
> <chethan.tumkur.narayan@intel.com>
> Subject: Re: [PATCH v1] Bluetooth: btintel: Add support for downloading
> secondary boot loader image
> 
> Dear Kiran,
> 
> 
> Thank you for your patch.
> 
> 
> 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.

> > Signed-off-by: Kiran K <kiran.k@intel.com>
> > ---
> > 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.
> 
> > [   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.

> > [   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.
> 
> >   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?
> 
> > +		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?
> 
> > +		if (ver->img_type == 1)
> 
> Below you write 0x0x. Should this be consistent?
> 
> > +			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?
> 
> > +		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
Paul Menzel March 4, 2024, 8:44 a.m. UTC | #2
Dear Kiran,


Thank you for your reply.

Am 04.03.24 um 09:21 schrieb K, Kiran:

>> -----Original Message-----
>> From: Paul Menzel <pmenzel@molgen.mpg.de>
>> 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.

>>> Signed-off-by: Kiran K <kiran.k@intel.com>
>>> ---
>>> 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?

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

>>>    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?
>>
>>> +		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?
>>
>>> +		if (ver->img_type == 1)
>>
>> Below you write 0x0x. Should this be consistent?
>>
>>> +			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?
>>
>>> +		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
K, Kiran March 5, 2024, 11:51 a.m. UTC | #3
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@molgen.mpg.de>
> >> 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@intel.com>
> >>> ---
> >>> 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
diff mbox series

Patch

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";
+		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:
+		if (ver->img_type == 1)
+			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) {
+		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");