Messages in this thread | | | Date | Thu, 3 Aug 2023 12:13:07 +0800 | Subject | Re: [PATCH] mhi: host: Add tme supported image download functionality | From | Qiang Yu <> |
| |
On 8/2/2023 11:47 PM, Jeffrey Hugo wrote: > On 7/24/2023 1:42 AM, Qiang Yu wrote: >> >> On 7/21/2023 1:13 PM, Jeffrey Hugo wrote: >>> On 7/20/2023 8:39 PM, Qiang Yu wrote: >>>> Add tme supported image related flag which makes decision in terms >>>> of how >>>> FBC image based AMSS image is being downloaded with connected >>>> endpoint. >>>> FBC image is having 2 image combine: SBL image + AMSS image. >>>> 1. FBC image download using legacy 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 TME supported 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 TME supported image >>>> format. >>> >>> I know what TME is, but in the context of this patch, it doesn't >>> seem like relevant information. "tme" is just a name for this mode, >>> but it is not very descriptive. Also, I suspect that this mode is >>> not intrinsically related to the TME hardware on the endpoint, it >>> just happens to be used on targets where TME is present. >>> >>> Is there something else we can call this? >>> >> Hi Jeff, sorry, previous reply contains HTML content, not sure if you >> have got my reply. So reply again. >> >> How about below commit message? >> >> Currently, the FBC image is non-standard ELF file which contains one ELF >> header followed by segments for SBL/RDDM and AMSS. To support TME-L,we >> need to have separate ELF header for SBL/RDDM and AMSS due to limitation >> of TME-L. > > I don't think mentioning TME-L has any value. The host doesn't > interact with TME-L nor has any way to directly detect if TME-L is > present. > > I would suggest making this more generic - some devices (for example > XXX) are unable to handle the non-standard ELF format of the FBC image > and thus need special handling of the FBC image. This is really better and more generic. > >> >> 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. > > I wonder if it makes more sense to split the FBC image into two images > for these devices. One SBL image, and one AMSS image. Feels like we > would just need to detect if there is a separate AMSS image, and use > that for BHIe. Then we don't need to go manipulating the image (which > I feel might be fragile). With combine FBC image, we are able to accommodate new image request better way with less code change, and satisfy processing of RDDM without any modification. > > > Mani, do you have thoughts on this? > >> >> Thank you for your time and patience. >>>> >>>> Signed-off-by: Mayank Rana <quic_mrana@quicinc.com> >>>> Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com> >>> >>> This doesn't make sense. This patch is from you, which makes you >>> the author. But Mayank's SOB is listed first, which means he is the >>> author. Those two facts conflict. >>> >>> Did Mayank author this and you are just submitting it on his behalf, >>> or did the two of you co-author this? >> >> In downstream, Mayank made this change and I modified it for >> upstream. Will it be accepted if I write the SOBs as following? I see >> this example >> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v6.5-rc2&id=7450aa5153af55a0c63785a6917e35a989a4fdf5 >> >> >> 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> > > This would be good. However it doesn't fully address my concern. > > I hope you are using patch files and git send-email. Before you send > the patch file, please open it in your favorite editor and look at the > second line. It should start with "From:". Right now, I suspect it > looks like: > > From: Qiang Yu <quic_qianyu@quicinc.com> > > However, based on what you described, it should look like: > > From: Mayank Rana <quic_mrana@quicinc.com> > > The From: line tells us who originally authored the change (lets > ignore changes with multiple authors for this discussion) and must > match the first SOB listed. Since Mayank authored the change and is > the first SOB listed, the patch should be "from" him. Thank you for pointing it out. Will change to "From: Mayank Rana <quic_mrana@quicinc.com>" . > >> >>> >>>> --- >>>> drivers/bus/mhi/host/boot.c | 24 +++++++++++++++++------- >>>> include/linux/mhi.h | 2 ++ >>>> 2 files changed, 19 insertions(+), 7 deletions(-) >>>> >>>> diff --git a/drivers/bus/mhi/host/boot.c b/drivers/bus/mhi/host/boot.c >>>> index d2a19b07..563b011 100644 >>>> --- a/drivers/bus/mhi/host/boot.c >>>> +++ b/drivers/bus/mhi/host/boot.c >>>> @@ -365,12 +365,13 @@ int mhi_alloc_bhie_table(struct >>>> mhi_controller *mhi_cntrl, >>>> } >>>> static void mhi_firmware_copy(struct mhi_controller *mhi_cntrl, >>>> - const struct firmware *firmware, >>>> + const u8 *image_buf, >>>> + size_t img_size, >>>> struct image_info *img_info) >>>> { >>>> - size_t remainder = firmware->size; >>>> + size_t remainder = img_size; >>>> size_t to_cpy; >>>> - const u8 *buf = firmware->data; >>>> + const u8 *buf = image_buf; >>>> struct mhi_buf *mhi_buf = img_info->mhi_buf; >>>> struct bhi_vec_entry *bhi_vec = img_info->bhi_vec; >>>> @@ -395,8 +396,9 @@ void mhi_fw_load_handler(struct >>>> mhi_controller *mhi_cntrl) >>>> const char *fw_name; >>>> void *buf; >>>> dma_addr_t dma_addr; >>>> - size_t size; >>>> + size_t size, img_size; >>>> int i, ret; >>>> + const u8 *img_buf; >>>> if (MHI_PM_IN_ERROR_STATE(mhi_cntrl->pm_state)) { >>>> dev_err(dev, "Device MHI is not in valid state\n"); >>>> @@ -478,15 +480,23 @@ void mhi_fw_load_handler(struct >>>> mhi_controller *mhi_cntrl) >>>> * device transitioning into MHI READY state >>>> */ >>>> if (mhi_cntrl->fbc_download) { >>>> - ret = mhi_alloc_bhie_table(mhi_cntrl, &mhi_cntrl->fbc_image, >>>> - firmware->size); >>>> + img_size = firmware->size; >>>> + img_buf = firmware->data; >>>> + dev_dbg(dev, "tme_supported_image:%s\n", >>>> + (mhi_cntrl->tme_supported_image ? "True" : "False")); >>>> + if (mhi_cntrl->tme_supported_image) { >>>> + img_buf = firmware->data + mhi_cntrl->sbl_size; >>>> + img_size = img_size - mhi_cntrl->sbl_size; >>>> + } >>>> + >>>> + ret = mhi_alloc_bhie_table(mhi_cntrl, >>>> &mhi_cntrl->fbc_image, img_size); >>>> if (ret) { >>>> release_firmware(firmware); >>>> goto error_fw_load; >>>> } >>>> /* Load the firmware into BHIE vec table */ >>>> - mhi_firmware_copy(mhi_cntrl, firmware, mhi_cntrl->fbc_image); >>>> + mhi_firmware_copy(mhi_cntrl, img_buf, img_size, >>>> mhi_cntrl->fbc_image); >>>> } >>>> release_firmware(firmware); >>>> diff --git a/include/linux/mhi.h b/include/linux/mhi.h >>>> index f6de4b6..5f46dc9 100644 >>>> --- a/include/linux/mhi.h >>>> +++ b/include/linux/mhi.h >>>> @@ -306,6 +306,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 >>>> + * @tme_supported_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) >>>> @@ -391,6 +392,7 @@ struct mhi_controller { >>>> size_t reg_len; >>>> struct image_info *fbc_image; >>>> struct image_info *rddm_image; >>>> + bool tme_supported_image; >>> >>> A bool in the middle of several pointers? Surely that makes the >>> pahole output rather sad? A lot of work went into the organization >>> of this structure. >> Can I add the flag under bool wake_set, and change the flag name to >> standard_elf_image? >> bool bounce_buf; >> bool fbc_download; >> bool edl_download; >> >> bool wake_set; >> >> + bool standard_elf_image > > This seems good to me. Remember to adjust your addition to the > structure description comment when you do this.
Thanks, will also adjust the structure description comment.
BTW, "[PATCH v4 1/3] bus: mhi: host: allow MHI client drivers to provide the firmware via a pointer" from Kalle Valo has got reviewed-by tag, my patch will have conflicts with his, do I have to send my V2 patch until his patch get merged?
> >> >> Thanks again and looking forward to your further review. >>> >>>> struct mhi_chan *mhi_chan; >>>> struct list_head lpm_chans; >>>> int *irq; >>> >
| |