In case of MHI MMIO writes, the register field write function needs to do reads
before writes are performed. Propagate read failures such that callers are made
aware of those and can take appropriate action instead of running blind.
Optimizing the MMIO initialization function to use mhi_write_reg() in most cases
should also be done to improve design.
These patches were tested on X86_64 architecture with Ubuntu 18.04 and SDX65
attach.
Bhaumik Bhatt (2):
bus: mhi: core: Bail on writing register fields if read fails
bus: mhi: core: Optimize and update MMIO register write method
drivers/bus/mhi/core/boot.c | 25 ++++++++++-----
drivers/bus/mhi/core/init.c | 70 +++++++++++++++++++++++------------------
drivers/bus/mhi/core/internal.h | 7 +++--
drivers/bus/mhi/core/main.c | 9 ++++--
4 files changed, 67 insertions(+), 44 deletions(-)
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
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 <[email protected]>
---
drivers/bus/mhi/core/boot.c | 25 +++++++++++++++++--------
drivers/bus/mhi/core/init.c | 24 ++++++++++++++++++------
drivers/bus/mhi/core/internal.h | 7 ++++---
drivers/bus/mhi/core/main.c | 9 ++++++---
4 files changed, 45 insertions(+), 20 deletions(-)
diff --git a/drivers/bus/mhi/core/boot.c b/drivers/bus/mhi/core/boot.c
index 0a97262..13eacda 100644
--- a/drivers/bus/mhi/core/boot.c
+++ b/drivers/bus/mhi/core/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,12 +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, BHIE_RXVECDB_SEQNUM_SHFT,
- sequence_id);
+ ret = mhi_write_reg_field(mhi_cntrl, base, BHIE_RXVECDB_OFFS,
+ BHIE_RXVECDB_SEQNUM_BMSK,
+ BHIE_RXVECDB_SEQNUM_SHFT, 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 */
@@ -202,11 +208,14 @@ 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, BHIE_TXVECDB_SEQNUM_SHFT,
- sequence_id);
+ ret = mhi_write_reg_field(mhi_cntrl, base, BHIE_TXVECDB_OFFS,
+ BHIE_TXVECDB_SEQNUM_BMSK,
+ BHIE_TXVECDB_SEQNUM_SHFT, 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/core/init.c b/drivers/bus/mhi/core/init.c
index 5aaca6d..0917465 100644
--- a/drivers/bus/mhi/core/init.c
+++ b/drivers/bus/mhi/core/init.c
@@ -544,10 +544,15 @@ 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].shift,
- 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].shift,
+ reg_info[i].val);
+ if (ret) {
+ dev_err(dev, "Unable to write to MMIO registers");
+ return ret;
+ }
+ }
return 0;
}
@@ -1118,8 +1123,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/core/internal.h b/drivers/bus/mhi/core/internal.h
index 721739c..3d17ec3 100644
--- a/drivers/bus/mhi/core/internal.h
+++ b/drivers/bus/mhi/core/internal.h
@@ -663,8 +663,9 @@ int __must_check mhi_poll_reg_field(struct mhi_controller *mhi_cntrl,
u32 shift, 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 shift, u32 val);
+int __must_check mhi_write_reg_field(struct mhi_controller *mhi_cntrl,
+ void __iomem *base, u32 offset,
+ u32 mask, u32 shift, 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);
@@ -678,7 +679,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);
int mhi_prepare_channel(struct mhi_controller *mhi_cntrl,
diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
index c01ec2f..902d854 100644
--- a/drivers/bus/mhi/core/main.c
+++ b/drivers/bus/mhi/core/main.c
@@ -66,19 +66,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 shift, u32 val)
+int __must_check mhi_write_reg_field(struct mhi_controller *mhi_cntrl,
+ void __iomem *base, u32 offset,
+ u32 mask, u32 shift, u32 val)
{
int ret;
u32 tmp;
ret = mhi_read_reg(mhi_cntrl, base, offset, &tmp);
if (ret)
- return;
+ return ret;
tmp &= ~mask;
tmp |= (val << shift);
mhi_write_reg(mhi_cntrl, base, offset, tmp);
+
+ return 0;
}
void mhi_write_db(struct mhi_controller *mhi_cntrl, void __iomem *db_addr,
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
As of now, MMIO writes done after ready state transition use the
mhi_write_reg_field() API even though the whole register is being
written in most cases. Optimize this process by using mhi_write_reg()
API instead for those writes and use the mhi_write_reg_field()
API for MHI config registers only.
Signed-off-by: Bhaumik Bhatt <[email protected]>
---
drivers/bus/mhi/core/init.c | 64 ++++++++++++++++++++++-----------------------
1 file changed, 31 insertions(+), 33 deletions(-)
diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
index 0917465..e4be171 100644
--- a/drivers/bus/mhi/core/init.c
+++ b/drivers/bus/mhi/core/init.c
@@ -433,75 +433,65 @@ int mhi_init_mmio(struct mhi_controller *mhi_cntrl)
struct device *dev = &mhi_cntrl->mhi_dev->dev;
struct {
u32 offset;
- u32 mask;
- u32 shift;
u32 val;
} reg_info[] = {
{
- CCABAP_HIGHER, U32_MAX, 0,
+ CCABAP_HIGHER,
upper_32_bits(mhi_cntrl->mhi_ctxt->chan_ctxt_addr),
},
{
- CCABAP_LOWER, U32_MAX, 0,
+ CCABAP_LOWER,
lower_32_bits(mhi_cntrl->mhi_ctxt->chan_ctxt_addr),
},
{
- ECABAP_HIGHER, U32_MAX, 0,
+ ECABAP_HIGHER,
upper_32_bits(mhi_cntrl->mhi_ctxt->er_ctxt_addr),
},
{
- ECABAP_LOWER, U32_MAX, 0,
+ ECABAP_LOWER,
lower_32_bits(mhi_cntrl->mhi_ctxt->er_ctxt_addr),
},
{
- CRCBAP_HIGHER, U32_MAX, 0,
+ CRCBAP_HIGHER,
upper_32_bits(mhi_cntrl->mhi_ctxt->cmd_ctxt_addr),
},
{
- CRCBAP_LOWER, U32_MAX, 0,
+ CRCBAP_LOWER,
lower_32_bits(mhi_cntrl->mhi_ctxt->cmd_ctxt_addr),
},
{
- MHICFG, MHICFG_NER_MASK, MHICFG_NER_SHIFT,
- mhi_cntrl->total_ev_rings,
- },
- {
- MHICFG, MHICFG_NHWER_MASK, MHICFG_NHWER_SHIFT,
- mhi_cntrl->hw_ev_rings,
- },
- {
- MHICTRLBASE_HIGHER, U32_MAX, 0,
+ MHICTRLBASE_HIGHER,
upper_32_bits(mhi_cntrl->iova_start),
},
{
- MHICTRLBASE_LOWER, U32_MAX, 0,
+ MHICTRLBASE_LOWER,
lower_32_bits(mhi_cntrl->iova_start),
},
{
- MHIDATABASE_HIGHER, U32_MAX, 0,
+ MHIDATABASE_HIGHER,
upper_32_bits(mhi_cntrl->iova_start),
},
{
- MHIDATABASE_LOWER, U32_MAX, 0,
+ MHIDATABASE_LOWER,
lower_32_bits(mhi_cntrl->iova_start),
},
{
- MHICTRLLIMIT_HIGHER, U32_MAX, 0,
+ MHICTRLLIMIT_HIGHER,
upper_32_bits(mhi_cntrl->iova_stop),
},
{
- MHICTRLLIMIT_LOWER, U32_MAX, 0,
+ MHICTRLLIMIT_LOWER,
lower_32_bits(mhi_cntrl->iova_stop),
},
{
- MHIDATALIMIT_HIGHER, U32_MAX, 0,
+ MHIDATALIMIT_HIGHER,
upper_32_bits(mhi_cntrl->iova_stop),
},
{
- MHIDATALIMIT_LOWER, U32_MAX, 0,
+ MHIDATALIMIT_LOWER,
lower_32_bits(mhi_cntrl->iova_stop),
},
- { 0, 0, 0 }
+ {0, 0}
};
dev_dbg(dev, "Initializing MHI registers\n");
@@ -544,14 +534,22 @@ 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++) {
- ret = mhi_write_reg_field(mhi_cntrl, base, reg_info[i].offset,
- reg_info[i].mask, reg_info[i].shift,
- reg_info[i].val);
- if (ret) {
- dev_err(dev, "Unable to write to MMIO registers");
- return ret;
- }
+ for (i = 0; reg_info[i].offset; i++)
+ mhi_write_reg(mhi_cntrl, base, reg_info[i].offset,
+ reg_info[i].val);
+
+ ret = mhi_write_reg_field(mhi_cntrl, base, MHICFG, MHICFG_NER_MASK,
+ MHICFG_NER_SHIFT, mhi_cntrl->total_ev_rings);
+ if (ret) {
+ dev_err(dev, "Unable to read MHICFG register\n");
+ return ret;
+ }
+
+ ret = mhi_write_reg_field(mhi_cntrl, base, MHICFG, MHICFG_NHWER_MASK,
+ MHICFG_NHWER_SHIFT, mhi_cntrl->hw_ev_rings);
+ if (ret) {
+ dev_err(dev, "Unable to read MHICFG register\n");
+ return ret;
}
return 0;
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
On 8/18/2021 4:50 PM, Bhaumik Bhatt wrote:
> 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 <[email protected]>
Reviewed-by: Hemant Kumar <[email protected]>
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum, a Linux Foundation Collaborative Project
On 8/18/2021 4:50 PM, Bhaumik Bhatt wrote:
> As of now, MMIO writes done after ready state transition use the
> mhi_write_reg_field() API even though the whole register is being
> written in most cases. Optimize this process by using mhi_write_reg()
> API instead for those writes and use the mhi_write_reg_field()
> API for MHI config registers only.
>
> Signed-off-by: Bhaumik Bhatt <[email protected]>
Reviewed-by: Hemant Kumar <[email protected]>
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum, a Linux Foundation Collaborative Project
Hi Bhaumik,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on next-20210818]
[cannot apply to linus/master v5.14-rc6 v5.14-rc5 v5.14-rc4 v5.14-rc6]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Bhaumik-Bhatt/MHI-MMIO-register-write-updates/20210819-075312
base: f26c3abc432a2026ba9ee7767061a1f88aead6ec
config: arm64-randconfig-r025-20210818 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project d2b574a4dea5b718e4386bf2e26af0126e5978ce)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install arm64 cross compiling tool for clang build
# apt-get install binutils-aarch64-linux-gnu
# https://github.com/0day-ci/linux/commit/034c38ae851193cbeec4b4538d1a47e75198b92a
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Bhaumik-Bhatt/MHI-MMIO-register-write-updates/20210819-075312
git checkout 034c38ae851193cbeec4b4538d1a47e75198b92a
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
>> drivers/bus/mhi/core/pm.c:133:3: warning: ignoring return value of function declared with 'warn_unused_result' attribute [-Wunused-result]
mhi_write_reg_field(mhi_cntrl, mhi_cntrl->regs, MHICTRL,
^~~~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/bus/mhi/core/pm.c:136:3: warning: ignoring return value of function declared with 'warn_unused_result' attribute [-Wunused-result]
mhi_write_reg_field(mhi_cntrl, mhi_cntrl->regs, MHICTRL,
^~~~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2 warnings generated.
vim +/warn_unused_result +133 drivers/bus/mhi/core/pm.c
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20 129
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20 130 void mhi_set_mhi_state(struct mhi_controller *mhi_cntrl, enum mhi_state state)
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20 131 {
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20 132 if (state == MHI_STATE_RESET) {
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20 @133 mhi_write_reg_field(mhi_cntrl, mhi_cntrl->regs, MHICTRL,
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20 134 MHICTRL_RESET_MASK, MHICTRL_RESET_SHIFT, 1);
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20 135 } else {
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20 136 mhi_write_reg_field(mhi_cntrl, mhi_cntrl->regs, MHICTRL,
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20 137 MHICTRL_MHISTATE_MASK,
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20 138 MHICTRL_MHISTATE_SHIFT, state);
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20 139 }
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20 140 }
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20 141
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
On 8/18/2021 7:40 PM, Hemant Kumar wrote:
>
>
> On 8/18/2021 4:50 PM, Bhaumik Bhatt wrote:
>> 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 <[email protected]>
> Reviewed-by: Hemant Kumar <[email protected]>
>
Reviewed-by: Jeffrey Hugo <[email protected]>
On 8/18/2021 7:41 PM, Hemant Kumar wrote:
>
>
> On 8/18/2021 4:50 PM, Bhaumik Bhatt wrote:
>> As of now, MMIO writes done after ready state transition use the
>> mhi_write_reg_field() API even though the whole register is being
>> written in most cases. Optimize this process by using mhi_write_reg()
>> API instead for those writes and use the mhi_write_reg_field()
>> API for MHI config registers only.
>>
>> Signed-off-by: Bhaumik Bhatt <[email protected]>
> Reviewed-by: Hemant Kumar <[email protected]>
>
Reviewed-by: Jeffrey Hugo <[email protected]>
On Wed, Aug 18, 2021 at 04:50:33PM -0700, Bhaumik Bhatt wrote:
> 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 <[email protected]>
As spotted by the test bot, can you please update pm.c as well?
Thanks,
Mani
> ---
> drivers/bus/mhi/core/boot.c | 25 +++++++++++++++++--------
> drivers/bus/mhi/core/init.c | 24 ++++++++++++++++++------
> drivers/bus/mhi/core/internal.h | 7 ++++---
> drivers/bus/mhi/core/main.c | 9 ++++++---
> 4 files changed, 45 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/bus/mhi/core/boot.c b/drivers/bus/mhi/core/boot.c
> index 0a97262..13eacda 100644
> --- a/drivers/bus/mhi/core/boot.c
> +++ b/drivers/bus/mhi/core/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,12 +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, BHIE_RXVECDB_SEQNUM_SHFT,
> - sequence_id);
> + ret = mhi_write_reg_field(mhi_cntrl, base, BHIE_RXVECDB_OFFS,
> + BHIE_RXVECDB_SEQNUM_BMSK,
> + BHIE_RXVECDB_SEQNUM_SHFT, 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 */
> @@ -202,11 +208,14 @@ 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, BHIE_TXVECDB_SEQNUM_SHFT,
> - sequence_id);
> + ret = mhi_write_reg_field(mhi_cntrl, base, BHIE_TXVECDB_OFFS,
> + BHIE_TXVECDB_SEQNUM_BMSK,
> + BHIE_TXVECDB_SEQNUM_SHFT, 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/core/init.c b/drivers/bus/mhi/core/init.c
> index 5aaca6d..0917465 100644
> --- a/drivers/bus/mhi/core/init.c
> +++ b/drivers/bus/mhi/core/init.c
> @@ -544,10 +544,15 @@ 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].shift,
> - 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].shift,
> + reg_info[i].val);
> + if (ret) {
> + dev_err(dev, "Unable to write to MMIO registers");
> + return ret;
> + }
> + }
>
> return 0;
> }
> @@ -1118,8 +1123,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/core/internal.h b/drivers/bus/mhi/core/internal.h
> index 721739c..3d17ec3 100644
> --- a/drivers/bus/mhi/core/internal.h
> +++ b/drivers/bus/mhi/core/internal.h
> @@ -663,8 +663,9 @@ int __must_check mhi_poll_reg_field(struct mhi_controller *mhi_cntrl,
> u32 shift, 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 shift, u32 val);
> +int __must_check mhi_write_reg_field(struct mhi_controller *mhi_cntrl,
> + void __iomem *base, u32 offset,
> + u32 mask, u32 shift, 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);
> @@ -678,7 +679,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);
> int mhi_prepare_channel(struct mhi_controller *mhi_cntrl,
> diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
> index c01ec2f..902d854 100644
> --- a/drivers/bus/mhi/core/main.c
> +++ b/drivers/bus/mhi/core/main.c
> @@ -66,19 +66,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 shift, u32 val)
> +int __must_check mhi_write_reg_field(struct mhi_controller *mhi_cntrl,
> + void __iomem *base, u32 offset,
> + u32 mask, u32 shift, u32 val)
> {
> int ret;
> u32 tmp;
>
> ret = mhi_read_reg(mhi_cntrl, base, offset, &tmp);
> if (ret)
> - return;
> + return ret;
>
> tmp &= ~mask;
> tmp |= (val << shift);
> mhi_write_reg(mhi_cntrl, base, offset, tmp);
> +
> + return 0;
> }
>
> void mhi_write_db(struct mhi_controller *mhi_cntrl, void __iomem *db_addr,
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
On Wed, Aug 18, 2021 at 04:50:34PM -0700, Bhaumik Bhatt wrote:
> As of now, MMIO writes done after ready state transition use the
> mhi_write_reg_field() API even though the whole register is being
> written in most cases. Optimize this process by using mhi_write_reg()
> API instead for those writes and use the mhi_write_reg_field()
> API for MHI config registers only.
>
> Signed-off-by: Bhaumik Bhatt <[email protected]>
Reviewed-by: Manivannan Sadhasivam <[email protected]>
Thanks,
Mani
> ---
> drivers/bus/mhi/core/init.c | 64 ++++++++++++++++++++++-----------------------
> 1 file changed, 31 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
> index 0917465..e4be171 100644
> --- a/drivers/bus/mhi/core/init.c
> +++ b/drivers/bus/mhi/core/init.c
> @@ -433,75 +433,65 @@ int mhi_init_mmio(struct mhi_controller *mhi_cntrl)
> struct device *dev = &mhi_cntrl->mhi_dev->dev;
> struct {
> u32 offset;
> - u32 mask;
> - u32 shift;
> u32 val;
> } reg_info[] = {
> {
> - CCABAP_HIGHER, U32_MAX, 0,
> + CCABAP_HIGHER,
> upper_32_bits(mhi_cntrl->mhi_ctxt->chan_ctxt_addr),
> },
> {
> - CCABAP_LOWER, U32_MAX, 0,
> + CCABAP_LOWER,
> lower_32_bits(mhi_cntrl->mhi_ctxt->chan_ctxt_addr),
> },
> {
> - ECABAP_HIGHER, U32_MAX, 0,
> + ECABAP_HIGHER,
> upper_32_bits(mhi_cntrl->mhi_ctxt->er_ctxt_addr),
> },
> {
> - ECABAP_LOWER, U32_MAX, 0,
> + ECABAP_LOWER,
> lower_32_bits(mhi_cntrl->mhi_ctxt->er_ctxt_addr),
> },
> {
> - CRCBAP_HIGHER, U32_MAX, 0,
> + CRCBAP_HIGHER,
> upper_32_bits(mhi_cntrl->mhi_ctxt->cmd_ctxt_addr),
> },
> {
> - CRCBAP_LOWER, U32_MAX, 0,
> + CRCBAP_LOWER,
> lower_32_bits(mhi_cntrl->mhi_ctxt->cmd_ctxt_addr),
> },
> {
> - MHICFG, MHICFG_NER_MASK, MHICFG_NER_SHIFT,
> - mhi_cntrl->total_ev_rings,
> - },
> - {
> - MHICFG, MHICFG_NHWER_MASK, MHICFG_NHWER_SHIFT,
> - mhi_cntrl->hw_ev_rings,
> - },
> - {
> - MHICTRLBASE_HIGHER, U32_MAX, 0,
> + MHICTRLBASE_HIGHER,
> upper_32_bits(mhi_cntrl->iova_start),
> },
> {
> - MHICTRLBASE_LOWER, U32_MAX, 0,
> + MHICTRLBASE_LOWER,
> lower_32_bits(mhi_cntrl->iova_start),
> },
> {
> - MHIDATABASE_HIGHER, U32_MAX, 0,
> + MHIDATABASE_HIGHER,
> upper_32_bits(mhi_cntrl->iova_start),
> },
> {
> - MHIDATABASE_LOWER, U32_MAX, 0,
> + MHIDATABASE_LOWER,
> lower_32_bits(mhi_cntrl->iova_start),
> },
> {
> - MHICTRLLIMIT_HIGHER, U32_MAX, 0,
> + MHICTRLLIMIT_HIGHER,
> upper_32_bits(mhi_cntrl->iova_stop),
> },
> {
> - MHICTRLLIMIT_LOWER, U32_MAX, 0,
> + MHICTRLLIMIT_LOWER,
> lower_32_bits(mhi_cntrl->iova_stop),
> },
> {
> - MHIDATALIMIT_HIGHER, U32_MAX, 0,
> + MHIDATALIMIT_HIGHER,
> upper_32_bits(mhi_cntrl->iova_stop),
> },
> {
> - MHIDATALIMIT_LOWER, U32_MAX, 0,
> + MHIDATALIMIT_LOWER,
> lower_32_bits(mhi_cntrl->iova_stop),
> },
> - { 0, 0, 0 }
> + {0, 0}
> };
>
> dev_dbg(dev, "Initializing MHI registers\n");
> @@ -544,14 +534,22 @@ 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++) {
> - ret = mhi_write_reg_field(mhi_cntrl, base, reg_info[i].offset,
> - reg_info[i].mask, reg_info[i].shift,
> - reg_info[i].val);
> - if (ret) {
> - dev_err(dev, "Unable to write to MMIO registers");
> - return ret;
> - }
> + for (i = 0; reg_info[i].offset; i++)
> + mhi_write_reg(mhi_cntrl, base, reg_info[i].offset,
> + reg_info[i].val);
> +
> + ret = mhi_write_reg_field(mhi_cntrl, base, MHICFG, MHICFG_NER_MASK,
> + MHICFG_NER_SHIFT, mhi_cntrl->total_ev_rings);
> + if (ret) {
> + dev_err(dev, "Unable to read MHICFG register\n");
> + return ret;
> + }
> +
> + ret = mhi_write_reg_field(mhi_cntrl, base, MHICFG, MHICFG_NHWER_MASK,
> + MHICFG_NHWER_SHIFT, mhi_cntrl->hw_ev_rings);
> + if (ret) {
> + dev_err(dev, "Unable to read MHICFG register\n");
> + return ret;
> }
>
> return 0;
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>