Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 29 Jan 2020 12:26:46 +0530
From:   Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
To:     Jeffrey Hugo <jhugo@codeaurora.org>
Cc:     gregkh@linuxfoundation.org, arnd@arndb.de, smohanad@codeaurora.org,
        kvalo@codeaurora.org, bjorn.andersson@linaro.org,
        hemantk@codeaurora.org, linux-arm-msm@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 08/16] bus: mhi: core: Add support for downloading
 firmware over BHIe
Message-ID: <20200129065645.GA25437@mani>
References: <20200123111836.7414-1-manivannan.sadhasivam@linaro.org>
 <20200123111836.7414-9-manivannan.sadhasivam@linaro.org>
 <00e3d4f8-89ab-79f0-7094-90cc6d85fa41@codeaurora.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <00e3d4f8-89ab-79f0-7094-90cc6d85fa41@codeaurora.org>
User-Agent: Mutt/1.9.4 (2018-02-28)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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@codeaurora.org>
> > Signed-off-by: Siddartha Mohanadoss <smohanad@codeaurora.org>
> > [mani: splitted the data transfer patch and cleaned up for upstream]
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >   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.