[PATCH v2] mhi: host: Add standard ELF header image download functionality

[PATCH v2] mhi: host: Add standard ELF header image download functionality

[Qiang Yu]

From: Mayank Rana <quic_mrana@quicinc.com>

Some devices (e.g. WLAN chips) are unable to handle the non-standard ELF
format of the FBC image and thus need special handling of the FBC image.

Add standard_elf_image flag which makes decision in terms of how FBC image
based AMSS image is being downloaded with connected endpoint.
FBC image is having two image combine: SBL image + AMSS image.
1. FBC image download using legacy single ELF header image format:
- SBL image: 512KB of FBC image is downloaded using BHI.
- AMSS image: full FBC image is downloaded using BHIe.
2. FBC image download using separate ELF header image format:
- SBL image: 512 KB of FBC image is downloaded using BHI.
- AMSS image: 512 KB onward FBC image is downloaded using BHIe.
There is no change for SBL image download. Although AMSS image start
address is end address of SBL image while using separate ELF header format.

Signed-off-by: Mayank Rana <quic_mrana@quicinc.com>
[quic_qianyu@quicinc.com: Update commit message, minor updates]
Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com>
---
v1->v2: modify commit message
	correct author
	rebase on latest mhi-next branch, resolve conflicts

 drivers/bus/mhi/host/boot.c | 7 +++++++
 include/linux/mhi.h         | 2 ++
 2 files changed, 9 insertions(+)

diff --git a/drivers/bus/mhi/host/boot.c b/drivers/bus/mhi/host/boot.c
index edc0ec5..586d551 100644
--- a/drivers/bus/mhi/host/boot.c
+++ b/drivers/bus/mhi/host/boot.c
@@ -495,6 +495,13 @@ void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl)
 	 * device transitioning into MHI READY state
 	 */
 	if (mhi_cntrl->fbc_download) {
+		dev_dbg(dev, "standard_elf_image: %s\n",
+				(mhi_cntrl->standard_elf_image ? "True" : "False"));
+		if (mhi_cntrl->standard_elf_image) {
+			fw_data = firmware->data + mhi_cntrl->sbl_size;
+			fw_sz = fw_sz - mhi_cntrl->sbl_size;
+		}
+
 		ret = mhi_alloc_bhie_table(mhi_cntrl, &mhi_cntrl->fbc_image, fw_sz);
 		if (ret) {
 			release_firmware(firmware);
diff --git a/include/linux/mhi.h b/include/linux/mhi.h
index 039943e..e065101 100644
--- a/include/linux/mhi.h
+++ b/include/linux/mhi.h
@@ -310,6 +310,7 @@ struct mhi_controller_config {
  * @reg_len: Length of the MHI MMIO region (required)
  * @fbc_image: Points to firmware image buffer
  * @rddm_image: Points to RAM dump buffer
+ * @standard_elf_image: Flag to make decision about firmware download start address (optional)
  * @mhi_chan: Points to the channel configuration table
  * @lpm_chans: List of channels that require LPM notifications
  * @irq: base irq # to request (required)
@@ -456,6 +457,7 @@ struct mhi_controller {
 	bool bounce_buf;
 	bool fbc_download;
 	bool wake_set;
+	bool standard_elf_image;
 	unsigned long irq_flags;
 	u32 mru;
 };
-- 
2.7.4

Re: [PATCH v2] mhi: host: Add standard ELF header image download functionality

[Manivannan Sadhasivam]

On Mon, Aug 07, 2023 at 03:59:52PM +0800, Qiang Yu wrote:
> From: Mayank Rana <quic_mrana@quicinc.com>
> 

Subject prefix should be: "bus: mhi:..."

> Some devices (e.g. WLAN chips) are unable to handle the non-standard ELF
> format of the FBC image and thus need special handling of the FBC image.
> 

Which WLAN chip? Is the driver for the chip already upstreamed?

> Add standard_elf_image flag which makes decision in terms of how FBC image
> based AMSS image is being downloaded with connected endpoint.
> FBC image is having two image combine: SBL image + AMSS image.
> 1. FBC image download using legacy single ELF header image format:
> - SBL image: 512KB of FBC image is downloaded using BHI.
> - AMSS image: full FBC image is downloaded using BHIe.
> 2. FBC image download using separate ELF header image format:
> - SBL image: 512 KB of FBC image is downloaded using BHI.
> - AMSS image: 512 KB onward FBC image is downloaded using BHIe.
> There is no change for SBL image download. Although AMSS image start
> address is end address of SBL image while using separate ELF header format.
> 
> Signed-off-by: Mayank Rana <quic_mrana@quicinc.com>
> [quic_qianyu@quicinc.com: Update commit message, minor updates]
> Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com>
> ---
> v1->v2: modify commit message
> 	correct author
> 	rebase on latest mhi-next branch, resolve conflicts
> 
>  drivers/bus/mhi/host/boot.c | 7 +++++++
>  include/linux/mhi.h         | 2 ++
>  2 files changed, 9 insertions(+)
> 
> diff --git a/drivers/bus/mhi/host/boot.c b/drivers/bus/mhi/host/boot.c
> index edc0ec5..586d551 100644
> --- a/drivers/bus/mhi/host/boot.c
> +++ b/drivers/bus/mhi/host/boot.c
> @@ -495,6 +495,13 @@ void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl)
>  	 * device transitioning into MHI READY state
>  	 */
>  	if (mhi_cntrl->fbc_download) {
> +		dev_dbg(dev, "standard_elf_image: %s\n",
> +				(mhi_cntrl->standard_elf_image ? "True" : "False"));
> +		if (mhi_cntrl->standard_elf_image) {

Commit message is saying that the devices cannot handle non-standard ELF image.
But this check is for standard ELF image. I'm confused.

> +			fw_data = firmware->data + mhi_cntrl->sbl_size;
> +			fw_sz = fw_sz - mhi_cntrl->sbl_size;
> +		}
> +
>  		ret = mhi_alloc_bhie_table(mhi_cntrl, &mhi_cntrl->fbc_image, fw_sz);
>  		if (ret) {
>  			release_firmware(firmware);
> diff --git a/include/linux/mhi.h b/include/linux/mhi.h
> index 039943e..e065101 100644
> --- a/include/linux/mhi.h
> +++ b/include/linux/mhi.h
> @@ -310,6 +310,7 @@ struct mhi_controller_config {
>   * @reg_len: Length of the MHI MMIO region (required)
>   * @fbc_image: Points to firmware image buffer
>   * @rddm_image: Points to RAM dump buffer
> + * @standard_elf_image: Flag to make decision about firmware download start address (optional)
>   * @mhi_chan: Points to the channel configuration table
>   * @lpm_chans: List of channels that require LPM notifications
>   * @irq: base irq # to request (required)
> @@ -456,6 +457,7 @@ struct mhi_controller {
>  	bool bounce_buf;
>  	bool fbc_download;
>  	bool wake_set;
> +	bool standard_elf_image;

Which driver is making use of this flag?

- Mani

>  	unsigned long irq_flags;
>  	u32 mru;
>  };
> -- 
> 2.7.4
> 
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v2] mhi: host: Add standard ELF header image download functionality

[Kalle Valo]

Qiang Yu <quic_qianyu@quicinc.com> writes:

> From: Mayank Rana <quic_mrana@quicinc.com>
>
> Some devices (e.g. WLAN chips) are unable to handle the non-standard ELF
> format of the FBC image and thus need special handling of the FBC image.
>
> Add standard_elf_image flag which makes decision in terms of how FBC image
> based AMSS image is being downloaded with connected endpoint.
> FBC image is having two image combine: SBL image + AMSS image.
> 1. FBC image download using legacy single ELF header image format:
> - SBL image: 512KB of FBC image is downloaded using BHI.
> - AMSS image: full FBC image is downloaded using BHIe.
> 2. FBC image download using separate ELF header image format:
> - SBL image: 512 KB of FBC image is downloaded using BHI.
> - AMSS image: 512 KB onward FBC image is downloaded using BHIe.
> There is no change for SBL image download. Although AMSS image start
> address is end address of SBL image while using separate ELF header format.
>
> Signed-off-by: Mayank Rana <quic_mrana@quicinc.com>
> [quic_qianyu@quicinc.com: Update commit message, minor updates]
> Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com>

I guess this is for an out-of-tree driver? I haven't heard any such
requirements for ath11k or ath12k.

> --- a/drivers/bus/mhi/host/boot.c
> +++ b/drivers/bus/mhi/host/boot.c
> @@ -495,6 +495,13 @@ void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl)
>  	 * device transitioning into MHI READY state
>  	 */
>  	if (mhi_cntrl->fbc_download) {
> +		dev_dbg(dev, "standard_elf_image: %s\n",
> +				(mhi_cntrl->standard_elf_image ? "True" : "False"));
> +		if (mhi_cntrl->standard_elf_image) {
> +			fw_data = firmware->data + mhi_cntrl->sbl_size;
> +			fw_sz = fw_sz - mhi_cntrl->sbl_size;
> +		}

So you are basically skipping the first sbl_size bytes of the firmware
file? Why not just fix the firmware file in userspace? Or maybe you can
use the recently added[1] mhi_cntrl->fw_data pointer and handle this in
your driver instead?

[1] https://git.kernel.org/pub/scm/linux/kernel/git/mani/mhi.git/commit/?h=mhi-next&id=efe47a18e43f59f063a82ccaa464a3b4844bb8a8

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH v2] mhi: host: Add standard ELF header image download functionality

[Qiang Yu]

On 8/22/2023 7:51 PM, Kalle Valo wrote:
> Qiang Yu<quic_qianyu@quicinc.com>  writes:
>
>> From: Mayank Rana<quic_mrana@quicinc.com>
>>
>> Some devices (e.g. WLAN chips) are unable to handle the non-standard ELF
>> format of the FBC image and thus need special handling of the FBC image.
>>
>> Add standard_elf_image flag which makes decision in terms of how FBC image
>> based AMSS image is being downloaded with connected endpoint.
>> FBC image is having two image combine: SBL image + AMSS image.
>> 1. FBC image download using legacy single ELF header image format:
>> - SBL image: 512KB of FBC image is downloaded using BHI.
>> - AMSS image: full FBC image is downloaded using BHIe.
>> 2. FBC image download using separate ELF header image format:
>> - SBL image: 512 KB of FBC image is downloaded using BHI.
>> - AMSS image: 512 KB onward FBC image is downloaded using BHIe.
>> There is no change for SBL image download. Although AMSS image start
>> address is end address of SBL image while using separate ELF header format.
>>
>> Signed-off-by: Mayank Rana<quic_mrana@quicinc.com>
>> [quic_qianyu@quicinc.com: Update commit message, minor updates]
>> Signed-off-by: Qiang Yu<quic_qianyu@quicinc.com>
> I guess this is for an out-of-tree driver? I haven't heard any such
> requirements for ath11k or ath12k.
Yes, this is for next generation wlan chip. After confirming with WLAN 
team, we will hold this patch until WLAN team upstream the driver for 
next generation wlan chip.
>> --- a/drivers/bus/mhi/host/boot.c
>> +++ b/drivers/bus/mhi/host/boot.c
>> @@ -495,6 +495,13 @@ void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl)
>>   	 * device transitioning into MHI READY state
>>   	 */
>>   	if (mhi_cntrl->fbc_download) {
>> +		dev_dbg(dev, "standard_elf_image: %s\n",
>> +				(mhi_cntrl->standard_elf_image ? "True" : "False"));
>> +		if (mhi_cntrl->standard_elf_image) {
>> +			fw_data = firmware->data + mhi_cntrl->sbl_size;
>> +			fw_sz = fw_sz - mhi_cntrl->sbl_size;
>> +		}
> So you are basically skipping the first sbl_size bytes of the firmware
> file? Why not just fix the firmware file in userspace? Or maybe you can
> use the recently added[1] mhi_cntrl->fw_data pointer and handle this in
> your driver instead?

Thanks for your suggestions. As per Mayank, with current method, we are 
able to accommodate new image request better way with less code change, 
and satisfy processing of RDDM without any modification.

Maybe we can revisit it when the driver for next gen wlan driver comes 
in picture.

> [1]https://git.kernel.org/pub/scm/linux/kernel/git/mani/mhi.git/commit/?h=mhi-next&id=efe47a18e43f59f063a82ccaa464a3b4844bb8a8
>