On Tue, Jan 28, 2020 at 12:36:04PM -0700, Jeffrey Hugo wrote: > On 1/23/2020 4:18 AM, Manivannan Sadhasivam wrote: > > MHI supports downloading the device firmware over BHI/BHIe (Boot Host > > Interface) protocol. Hence, this commit adds necessary helpers, which > > will be called during device power up stage. > > > > This is based on the patch submitted by Sujeev Dias: > > https://lkml.org/lkml/2018/7/9/989 > > > > Signed-off-by: Sujeev Dias <sdias@xxxxxxxxxxxxxx> > > Signed-off-by: Siddartha Mohanadoss <smohanad@xxxxxxxxxxxxxx> > > [mani: splitted the data transfer patch and cleaned up for upstream] > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx> > > --- > > drivers/bus/mhi/core/boot.c | 268 ++++++++++++++++++++++++++++++++ > > drivers/bus/mhi/core/init.c | 1 + > > drivers/bus/mhi/core/internal.h | 1 + > > 3 files changed, 270 insertions(+) > > > > diff --git a/drivers/bus/mhi/core/boot.c b/drivers/bus/mhi/core/boot.c > > index 0996f18c4281..36956fb6eff2 100644 > > --- a/drivers/bus/mhi/core/boot.c > > +++ b/drivers/bus/mhi/core/boot.c > > @@ -20,6 +20,121 @@ > > #include <linux/wait.h> > > #include "internal.h" > > +/* Download AMSS image to device */ > > Nit: I don't feel like this comment really adds any value. I feel like it > either should have more content, or be removed. What do you think? Okay, I think it can be removed. > > +static int mhi_fw_load_amss(struct mhi_controller *mhi_cntrl, > > + const struct mhi_buf *mhi_buf) > > +{ > > > > +/* Download SBL image to device */ > > Same here. Comment seems self evident from the function name. Ack. > > +static int mhi_fw_load_sbl(struct mhi_controller *mhi_cntrl, > > + dma_addr_t dma_addr, > > + size_t size) > > +{ > > + u32 tx_status, val, session_id; > > + int i, ret; > > + void __iomem *base = mhi_cntrl->bhi; > > + rwlock_t *pm_lock = &mhi_cntrl->pm_lock; > > + 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)) { > > + read_unlock_bh(pm_lock); > > + goto invalid_pm_state; > > + } > > + > > + /* Start SBL download via BHI protocol */ > > I'm wondering, what do you think about having a debug level message here > that SBL is being loaded? I think it would be handy for looking into the > device state. > Agree. will add. > > + mhi_write_reg(mhi_cntrl, base, BHI_STATUS, 0); > > + mhi_write_reg(mhi_cntrl, base, BHI_IMGADDR_HIGH, > > + upper_32_bits(dma_addr)); > > + mhi_write_reg(mhi_cntrl, base, BHI_IMGADDR_LOW, > > + lower_32_bits(dma_addr)); > > + mhi_write_reg(mhi_cntrl, base, BHI_IMGSIZE, size); > > + session_id = prandom_u32() & BHI_TXDB_SEQNUM_BMSK; > > + mhi_write_reg(mhi_cntrl, base, BHI_IMGTXDB, session_id); > > + read_unlock_bh(pm_lock); > > + > > + > > +static void mhi_firmware_copy(struct mhi_controller *mhi_cntrl, > > + const struct firmware *firmware, > > + struct image_info *img_info) > > Perhaps its just me, but the parameters on the second and third lines do not > look aligned in the style used in the rest of the file. > Yep, will fix it. Thanks, Mani > > -- > Jeffrey Hugo > Qualcomm Technologies, Inc. is a member of the > Code Aurora Forum, a Linux Foundation Collaborative Project.
