On 1/7/2025 10:24 PM, Manivannan Sadhasivam wrote:
On Fri, Dec 13, 2024 at 02:33:34PM -0700, Jeffrey Hugo wrote:From: Matthew Leung <quic_mattl...@quicinc.com>Refactor the firmware loading code to have distinct helper functions for BHI and BHIe operations. This lays the foundation for separating the firmware loading protocol from the firmware being loaded and the EE it is loaded in. Signed-off-by: Matthew Leung <quic_mattl...@quicinc.com> Reviewed-by: Youssef Samir <quic_yabdu...@quicinc.com> Reviewed-by: Jeffrey Hugo <quic_jh...@quicinc.com> Signed-off-by: Jeffrey Hugo <quic_jh...@quicinc.com> --- drivers/bus/mhi/host/boot.c | 155 +++++++++++++++++++++++++----------- 1 file changed, 110 insertions(+), 45 deletions(-) diff --git a/drivers/bus/mhi/host/boot.c b/drivers/bus/mhi/host/boot.c index e8c92972f9df..e3f3c07166ad 100644 --- a/drivers/bus/mhi/host/boot.c +++ b/drivers/bus/mhi/host/boot.c @@ -177,6 +177,37 @@ int mhi_download_rddm_image(struct mhi_controller *mhi_cntrl, bool in_panic) } EXPORT_SYMBOL_GPL(mhi_download_rddm_image);+static inline void mhi_fw_load_error_dump(struct mhi_controller *mhi_cntrl)No need to add 'inline' keyword in c files. You can trust the compiler.
Done.
+{ + struct device *dev = &mhi_cntrl->mhi_dev->dev; + rwlock_t *pm_lock = &mhi_cntrl->pm_lock; + void __iomem *base = mhi_cntrl->bhi; + int ret; + u32 val; + int i;int ret, i?
Done.
+ struct { + char *name; + u32 offset; + } error_reg[] = { + { "ERROR_CODE", BHI_ERRCODE }, + { "ERROR_DBG1", BHI_ERRDBG1 }, + { "ERROR_DBG2", BHI_ERRDBG2 }, + { "ERROR_DBG3", BHI_ERRDBG3 }, + { NULL }, + }; + + read_lock_bh(pm_lock); + if (MHI_REG_ACCESS_VALID(mhi_cntrl->pm_state)) { + for (i = 0; error_reg[i].name; i++) { + ret = mhi_read_reg(mhi_cntrl, base, error_reg[i].offset, &val); + if (ret) + break; + dev_err(dev, "Reg: %s value: 0x%x\n", error_reg[i].name, val); + } + } + read_unlock_bh(pm_lock); +} +[...]+static int mhi_alloc_bhi_buffer(struct mhi_controller *mhi_cntrl, + struct image_info **image_info, + size_t alloc_size) +{ + struct image_info *img_info; + struct mhi_buf *mhi_buf; + int segments = 1; + + img_info = kzalloc(sizeof(*img_info), GFP_KERNEL); + if (!img_info) + return -ENOMEM; + + /* Allocate memory for entry */ + img_info->mhi_buf = kcalloc(segments, sizeof(*img_info->mhi_buf), + GFP_KERNEL);Why do you need kcalloc for only 1 segment?
Symmetry with mhi_alloc_bhie_table(). Will change.
+ if (!img_info->mhi_buf) + goto error_alloc_mhi_buf; + + /* Allocate and populate vector table */ + mhi_buf = img_info->mhi_buf; + + mhi_buf->len = alloc_size; + mhi_buf->buf = dma_alloc_coherent(mhi_cntrl->cntrl_dev, mhi_buf->len, + &mhi_buf->dma_addr, GFP_KERNEL); + if (!mhi_buf->buf) + goto error_alloc_segment; + + img_info->bhi_vec = NULL; + img_info->entries = segments; + *image_info = img_info; + + return 0; + +error_alloc_segment: + kfree(mhi_buf); +error_alloc_mhi_buf: + kfree(img_info); + + return -ENOMEM; +} + int mhi_alloc_bhie_table(struct mhi_controller *mhi_cntrl, struct image_info **image_info, size_t alloc_size) @@ -364,9 +422,18 @@ int mhi_alloc_bhie_table(struct mhi_controller *mhi_cntrl, return -ENOMEM; }-static void mhi_firmware_copy(struct mhi_controller *mhi_cntrl,- const u8 *buf, size_t remainder, - struct image_info *img_info) +static void mhi_firmware_copy_bhi(struct mhi_controller *mhi_cntrl, + const u8 *buf, size_t size, + struct image_info *img_info) +{ + struct mhi_buf *mhi_buf = img_info->mhi_buf; + + memcpy(mhi_buf->buf, buf, size); +} + +static void mhi_firmware_copy_bhie(struct mhi_controller *mhi_cntrl, + const u8 *buf, size_t remainder, + struct image_info *img_info) { size_t to_cpy; struct mhi_buf *mhi_buf = img_info->mhi_buf; @@ -390,10 +457,9 @@ void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl) const struct firmware *firmware = NULL; struct device *dev = &mhi_cntrl->mhi_dev->dev; enum mhi_pm_state new_state; + struct image_info *image; const char *fw_name; const u8 *fw_data; - void *buf; - dma_addr_t dma_addr; size_t size, fw_sz; int ret;@@ -452,17 +518,16 @@ void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl)fw_sz = firmware->size;skip_req_fw:- buf = dma_alloc_coherent(mhi_cntrl->cntrl_dev, size, &dma_addr, - GFP_KERNEL); - if (!buf) { + ret = mhi_alloc_bhi_buffer(mhi_cntrl, &image, size); + if (ret) { release_firmware(firmware); goto error_fw_load; } + mhi_firmware_copy_bhi(mhi_cntrl, fw_data, size, image);Why can't you directly use memcpy here? I know what you want to keep symmetry with mhi_firmware_copy_bhie(), but it seems unnecessary to me. Adding a comment like "Load the firmware into BHI vec table" is enough.
Just symmetry. Jarek had the same comment. Will inline.
