From: Manivannan Sadhasivam <mani@kernel.org>
To: Jeffrey Hugo <quic_jhugo@quicinc.com>
Cc: quic_hemantk@quicinc.com, quic_bbhatt@quicinc.com,
mhi@lists.linux.dev, linux-arm-msm@vger.kernel.org,
linux-kernel@vger.kernel.org,
Bhaumik Bhatt <bbhatt@codeaurora.org>
Subject: Re: [PATCH v4 1/2] bus: mhi: host: Bail on writing register fields if read fails
Date: Sat, 23 Apr 2022 17:56:44 +0530 [thread overview]
Message-ID: <20220423122644.GL374560@thinkpad> (raw)
In-Reply-To: <1650304226-11080-2-git-send-email-quic_jhugo@quicinc.com>
On Mon, Apr 18, 2022 at 11:50:25AM -0600, Jeffrey Hugo wrote:
> From: Bhaumik Bhatt <bbhatt@codeaurora.org>
>
> Helper API to write register fields relies on successful reads
> of the register/address prior to the write. Bail out if a failure
> is seen when reading the register before the actual write is
> performed.
>
> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
> Reviewed-by: Hemant Kumar <hemantk@codeaurora.org>
> Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
> Signed-off-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Thanks,
Mani
> ---
> drivers/bus/mhi/host/boot.c | 22 ++++++++++++++++------
> drivers/bus/mhi/host/init.c | 22 +++++++++++++++++-----
> drivers/bus/mhi/host/internal.h | 7 ++++---
> drivers/bus/mhi/host/main.c | 9 ++++++---
> drivers/bus/mhi/host/pm.c | 15 +++++++++++----
> 5 files changed, 54 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/bus/mhi/host/boot.c b/drivers/bus/mhi/host/boot.c
> index b0da7ca..26d0edd 100644
> --- a/drivers/bus/mhi/host/boot.c
> +++ b/drivers/bus/mhi/host/boot.c
> @@ -19,8 +19,8 @@
> #include "internal.h"
>
> /* Setup RDDM vector table for RDDM transfer and program RXVEC */
> -void mhi_rddm_prepare(struct mhi_controller *mhi_cntrl,
> - struct image_info *img_info)
> +int mhi_rddm_prepare(struct mhi_controller *mhi_cntrl,
> + struct image_info *img_info)
> {
> struct mhi_buf *mhi_buf = img_info->mhi_buf;
> struct bhi_vec_entry *bhi_vec = img_info->bhi_vec;
> @@ -28,6 +28,7 @@ void mhi_rddm_prepare(struct mhi_controller *mhi_cntrl,
> struct device *dev = &mhi_cntrl->mhi_dev->dev;
> u32 sequence_id;
> unsigned int i;
> + int ret;
>
> for (i = 0; i < img_info->entries - 1; i++, mhi_buf++, bhi_vec++) {
> bhi_vec->dma_addr = mhi_buf->dma_addr;
> @@ -45,11 +46,17 @@ void mhi_rddm_prepare(struct mhi_controller *mhi_cntrl,
> mhi_write_reg(mhi_cntrl, base, BHIE_RXVECSIZE_OFFS, mhi_buf->len);
> sequence_id = MHI_RANDOM_U32_NONZERO(BHIE_RXVECSTATUS_SEQNUM_BMSK);
>
> - mhi_write_reg_field(mhi_cntrl, base, BHIE_RXVECDB_OFFS,
> - BHIE_RXVECDB_SEQNUM_BMSK, sequence_id);
> + ret = mhi_write_reg_field(mhi_cntrl, base, BHIE_RXVECDB_OFFS,
> + BHIE_RXVECDB_SEQNUM_BMSK, sequence_id);
> + if (ret) {
> + dev_err(dev, "Failed to write sequence ID for BHIE_RXVECDB\n");
> + return ret;
> + }
>
> dev_dbg(dev, "Address: %p and len: 0x%zx sequence: %u\n",
> &mhi_buf->dma_addr, mhi_buf->len, sequence_id);
> +
> + return 0;
> }
>
> /* Collect RDDM buffer during kernel panic */
> @@ -198,10 +205,13 @@ static int mhi_fw_load_bhie(struct mhi_controller *mhi_cntrl,
>
> mhi_write_reg(mhi_cntrl, base, BHIE_TXVECSIZE_OFFS, mhi_buf->len);
>
> - mhi_write_reg_field(mhi_cntrl, base, BHIE_TXVECDB_OFFS,
> - BHIE_TXVECDB_SEQNUM_BMSK, sequence_id);
> + ret = mhi_write_reg_field(mhi_cntrl, base, BHIE_TXVECDB_OFFS,
> + BHIE_TXVECDB_SEQNUM_BMSK, sequence_id);
> read_unlock_bh(pm_lock);
>
> + if (ret)
> + return ret;
> +
> /* Wait for the image download to complete */
> ret = wait_event_timeout(mhi_cntrl->state_event,
> MHI_PM_IN_ERROR_STATE(mhi_cntrl->pm_state) ||
> diff --git a/drivers/bus/mhi/host/init.c b/drivers/bus/mhi/host/init.c
> index a8c18c5f..3842611 100644
> --- a/drivers/bus/mhi/host/init.c
> +++ b/drivers/bus/mhi/host/init.c
> @@ -547,9 +547,14 @@ int mhi_init_mmio(struct mhi_controller *mhi_cntrl)
> mhi_cntrl->mhi_cmd[PRIMARY_CMD_RING].ring.db_addr = base + CRDB_LOWER;
>
> /* Write to MMIO registers */
> - for (i = 0; reg_info[i].offset; i++)
> - mhi_write_reg_field(mhi_cntrl, base, reg_info[i].offset,
> - reg_info[i].mask, reg_info[i].val);
> + for (i = 0; reg_info[i].offset; i++) {
> + ret = mhi_write_reg_field(mhi_cntrl, base, reg_info[i].offset,
> + reg_info[i].mask, reg_info[i].val);
> + if (ret) {
> + dev_err(dev, "Unable to write to MMIO registers\n");
> + return ret;
> + }
> + }
>
> return 0;
> }
> @@ -1117,8 +1122,15 @@ int mhi_prepare_for_power_up(struct mhi_controller *mhi_cntrl)
> */
> mhi_alloc_bhie_table(mhi_cntrl, &mhi_cntrl->rddm_image,
> mhi_cntrl->rddm_size);
> - if (mhi_cntrl->rddm_image)
> - mhi_rddm_prepare(mhi_cntrl, mhi_cntrl->rddm_image);
> + if (mhi_cntrl->rddm_image) {
> + ret = mhi_rddm_prepare(mhi_cntrl,
> + mhi_cntrl->rddm_image);
> + if (ret) {
> + mhi_free_bhie_table(mhi_cntrl,
> + mhi_cntrl->rddm_image);
> + goto error_reg_offset;
> + }
> + }
> }
>
> mutex_unlock(&mhi_cntrl->pm_mutex);
> diff --git a/drivers/bus/mhi/host/internal.h b/drivers/bus/mhi/host/internal.h
> index b47d8ef..01fd10a 100644
> --- a/drivers/bus/mhi/host/internal.h
> +++ b/drivers/bus/mhi/host/internal.h
> @@ -324,8 +324,9 @@ int __must_check mhi_poll_reg_field(struct mhi_controller *mhi_cntrl,
> u32 val, u32 delayus);
> void mhi_write_reg(struct mhi_controller *mhi_cntrl, void __iomem *base,
> u32 offset, u32 val);
> -void mhi_write_reg_field(struct mhi_controller *mhi_cntrl, void __iomem *base,
> - u32 offset, u32 mask, u32 val);
> +int __must_check mhi_write_reg_field(struct mhi_controller *mhi_cntrl,
> + void __iomem *base, u32 offset, u32 mask,
> + u32 val);
> void mhi_ring_er_db(struct mhi_event *mhi_event);
> void mhi_write_db(struct mhi_controller *mhi_cntrl, void __iomem *db_addr,
> dma_addr_t db_val);
> @@ -339,7 +340,7 @@ int mhi_init_dev_ctxt(struct mhi_controller *mhi_cntrl);
> void mhi_deinit_dev_ctxt(struct mhi_controller *mhi_cntrl);
> int mhi_init_irq_setup(struct mhi_controller *mhi_cntrl);
> void mhi_deinit_free_irq(struct mhi_controller *mhi_cntrl);
> -void mhi_rddm_prepare(struct mhi_controller *mhi_cntrl,
> +int mhi_rddm_prepare(struct mhi_controller *mhi_cntrl,
> struct image_info *img_info);
> void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl);
>
> diff --git a/drivers/bus/mhi/host/main.c b/drivers/bus/mhi/host/main.c
> index 9021be7..142eea1 100644
> --- a/drivers/bus/mhi/host/main.c
> +++ b/drivers/bus/mhi/host/main.c
> @@ -65,19 +65,22 @@ void mhi_write_reg(struct mhi_controller *mhi_cntrl, void __iomem *base,
> mhi_cntrl->write_reg(mhi_cntrl, base + offset, val);
> }
>
> -void mhi_write_reg_field(struct mhi_controller *mhi_cntrl, void __iomem *base,
> - u32 offset, u32 mask, u32 val)
> +int __must_check mhi_write_reg_field(struct mhi_controller *mhi_cntrl,
> + void __iomem *base, u32 offset, u32 mask,
> + u32 val)
> {
> int ret;
> u32 tmp;
>
> ret = mhi_read_reg(mhi_cntrl, base, offset, &tmp);
> if (ret)
> - return;
> + return ret;
>
> tmp &= ~mask;
> tmp |= (val << __ffs(mask));
> mhi_write_reg(mhi_cntrl, base, offset, tmp);
> +
> + return 0;
> }
>
> void mhi_write_db(struct mhi_controller *mhi_cntrl, void __iomem *db_addr,
> diff --git a/drivers/bus/mhi/host/pm.c b/drivers/bus/mhi/host/pm.c
> index c000a92..dc2e8ff 100644
> --- a/drivers/bus/mhi/host/pm.c
> +++ b/drivers/bus/mhi/host/pm.c
> @@ -129,13 +129,20 @@ enum mhi_pm_state __must_check mhi_tryset_pm_state(struct mhi_controller *mhi_cn
>
> void mhi_set_mhi_state(struct mhi_controller *mhi_cntrl, enum mhi_state state)
> {
> + struct device *dev = &mhi_cntrl->mhi_dev->dev;
> + int ret;
> +
> if (state == MHI_STATE_RESET) {
> - mhi_write_reg_field(mhi_cntrl, mhi_cntrl->regs, MHICTRL,
> - MHICTRL_RESET_MASK, 1);
> + ret = mhi_write_reg_field(mhi_cntrl, mhi_cntrl->regs, MHICTRL,
> + MHICTRL_RESET_MASK, 1);
> } else {
> - mhi_write_reg_field(mhi_cntrl, mhi_cntrl->regs, MHICTRL,
> - MHICTRL_MHISTATE_MASK, state);
> + ret = mhi_write_reg_field(mhi_cntrl, mhi_cntrl->regs, MHICTRL,
> + MHICTRL_MHISTATE_MASK, state);
> }
> +
> + if (ret)
> + dev_err(dev, "Failed to set MHI state to: %s\n",
> + mhi_state_str(state));
> }
>
> /* NOP for backward compatibility, host allowed to ring DB in M2 state */
> --
> 2.7.4
>
>
next prev parent reply other threads:[~2022-04-23 12:27 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-04-18 17:50 [PATCH v4 0/2] MHI MMIO register write updates Jeffrey Hugo
2022-04-18 17:50 ` [PATCH v4 1/2] bus: mhi: host: Bail on writing register fields if read fails Jeffrey Hugo
2022-04-23 12:26 ` Manivannan Sadhasivam [this message]
2022-04-18 17:50 ` [PATCH v4 2/2] bus: mhi: host: Optimize and update MMIO register write method Jeffrey Hugo
2022-04-23 13:26 ` [PATCH v4 0/2] MHI MMIO register write updates Manivannan Sadhasivam
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20220423122644.GL374560@thinkpad \
--to=mani@kernel.org \
--cc=bbhatt@codeaurora.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mhi@lists.linux.dev \
--cc=quic_bbhatt@quicinc.com \
--cc=quic_hemantk@quicinc.com \
--cc=quic_jhugo@quicinc.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox