mbox series

[v1,0/2] MHI MMIO register write updates

Message ID 1629330634-36465-1-git-send-email-bbhatt@codeaurora.org
Headers show
Series MHI MMIO register write updates | expand

Message

Bhaumik Bhatt Aug. 18, 2021, 11:50 p.m. UTC
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(-)

Comments

Hemant Kumar Aug. 19, 2021, 1:41 a.m. UTC | #1
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 <bbhatt@codeaurora.org>

Reviewed-by: Hemant Kumar <hemantk@codeaurora.org>


-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum, a Linux Foundation Collaborative Project
Jeffrey Hugo Aug. 19, 2021, 2:05 p.m. UTC | #2
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 <bbhatt@codeaurora.org>

> Reviewed-by: Hemant Kumar <hemantk@codeaurora.org>

> 

Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
Manivannan Sadhasivam Aug. 19, 2021, 5:03 p.m. UTC | #3
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 <bbhatt@codeaurora.org>


Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>


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

>