Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 30 Apr 2020 22:27:40 +0530
From:   Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
To:     Jeffrey Hugo <jhugo@codeaurora.org>
Cc:     hemantk@codeaurora.org, bbhatt@codeaurora.org,
        linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 3/6] bus: mhi: core: Offload register accesses to the
 controller
Message-ID: <20200430165740.GF9449@Mani-XPS-13-9360>
References: <1588003153-13139-1-git-send-email-jhugo@codeaurora.org>
 <1588003153-13139-4-git-send-email-jhugo@codeaurora.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1588003153-13139-4-git-send-email-jhugo@codeaurora.org>
User-Agent: Mutt/1.9.4 (2018-02-28)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, Apr 27, 2020 at 09:59:10AM -0600, Jeffrey Hugo wrote:
> When reading or writing MHI registers, the core assumes that the physical
> link is a memory mapped PCI link.  This assumption may not hold for all
> MHI devices.  The controller knows what is the physical link (ie PCI, I2C,
> SPI, etc), and therefore knows the proper methods to access that link.
> The controller can also handle link specific error scenarios, such as
> reading -1 when the PCI link went down.
> 
> Therefore, it is appropriate that the MHI core requests the controller to
> make register accesses on behalf of the core, which abstracts the core
> from link specifics, and end up removing an unnecessary assumption.
> 
> Signed-off-by: Jeffrey Hugo <jhugo@codeaurora.org>

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

You know how much I like this patch ;)

Thanks,
Mani

> ---
>  drivers/bus/mhi/core/init.c     |  3 ++-
>  drivers/bus/mhi/core/internal.h |  3 ---
>  drivers/bus/mhi/core/main.c     | 12 ++----------
>  include/linux/mhi.h             |  6 ++++++
>  4 files changed, 10 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
> index 2af08d57..eb2ab05 100644
> --- a/drivers/bus/mhi/core/init.c
> +++ b/drivers/bus/mhi/core/init.c
> @@ -813,7 +813,8 @@ int mhi_register_controller(struct mhi_controller *mhi_cntrl,
>  		return -EINVAL;
>  
>  	if (!mhi_cntrl->runtime_get || !mhi_cntrl->runtime_put ||
> -	    !mhi_cntrl->status_cb)
> +	    !mhi_cntrl->status_cb || !mhi_cntrl->read_reg ||
> +	    !mhi_cntrl->write_reg)
>  		return -EINVAL;
>  
>  	ret = parse_config(mhi_cntrl, config);
> diff --git a/drivers/bus/mhi/core/internal.h b/drivers/bus/mhi/core/internal.h
> index 5deadfa..095d95b 100644
> --- a/drivers/bus/mhi/core/internal.h
> +++ b/drivers/bus/mhi/core/internal.h
> @@ -11,9 +11,6 @@
>  
>  extern struct bus_type mhi_bus_type;
>  
> -/* MHI MMIO register mapping */
> -#define PCI_INVALID_READ(val) (val == U32_MAX)
> -
>  #define MHIREGLEN (0x0)
>  #define MHIREGLEN_MHIREGLEN_MASK (0xFFFFFFFF)
>  #define MHIREGLEN_MHIREGLEN_SHIFT (0)
> diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
> index 473278b8..580d72b 100644
> --- a/drivers/bus/mhi/core/main.c
> +++ b/drivers/bus/mhi/core/main.c
> @@ -18,15 +18,7 @@
>  int __must_check mhi_read_reg(struct mhi_controller *mhi_cntrl,
>  			      void __iomem *base, u32 offset, u32 *out)
>  {
> -	u32 tmp = readl(base + offset);
> -
> -	/* If the value is invalid, the link is down */
> -	if (PCI_INVALID_READ(tmp))
> -		return -EIO;
> -
> -	*out = tmp;
> -
> -	return 0;
> +	return mhi_cntrl->read_reg(mhi_cntrl, base + offset, out);
>  }
>  
>  int __must_check mhi_read_reg_field(struct mhi_controller *mhi_cntrl,
> @@ -48,7 +40,7 @@ int __must_check mhi_read_reg_field(struct mhi_controller *mhi_cntrl,
>  void mhi_write_reg(struct mhi_controller *mhi_cntrl, void __iomem *base,
>  		   u32 offset, u32 val)
>  {
> -	writel(val, base + offset);
> +	mhi_cntrl->write_reg(mhi_cntrl, base + offset, val);
>  }
>  
>  void mhi_write_reg_field(struct mhi_controller *mhi_cntrl, void __iomem *base,
> diff --git a/include/linux/mhi.h b/include/linux/mhi.h
> index be704a4..225a03a 100644
> --- a/include/linux/mhi.h
> +++ b/include/linux/mhi.h
> @@ -342,6 +342,8 @@ struct mhi_controller_config {
>   * @runtimet_put: CB function to decrement pm usage (required)
>   * @map_single: CB function to create TRE buffer
>   * @unmap_single: CB function to destroy TRE buffer
> + * @read_reg: Read a MHI register via the physical link (required)
> + * @write_reg: Write a MHI register via the physical link (required)
>   * @buffer_len: Bounce buffer length
>   * @bounce_buf: Use of bounce buffer
>   * @fbc_download: MHI host needs to do complete image transfer (optional)
> @@ -425,6 +427,10 @@ struct mhi_controller {
>  			  struct mhi_buf_info *buf);
>  	void (*unmap_single)(struct mhi_controller *mhi_cntrl,
>  			     struct mhi_buf_info *buf);
> +	int (*read_reg)(struct mhi_controller *mhi_cntrl, void __iomem *addr,
> +			u32 *out);
> +	void (*write_reg)(struct mhi_controller *mhi_cntrl, void __iomem *addr,
> +			  u32 val);
>  
>  	size_t buffer_len;
>  	bool bounce_buf;
> -- 
> Qualcomm Technologies, Inc. is a member of the
> Code Aurora Forum, a Linux Foundation Collaborative Project.