Re: [PATCH v3] mhi: pci_generic: Ensure device readiness before starting MHI

[Bhaumik Bhatt <bbhatt@codeaurora.org>]

Hi Loic,

On 2021-02-11 11:25 AM, Loic Poulain wrote:
> The PCI device may have not been bound from cold boot and be in
> undefined state, or simply not yet ready for MHI operations. This
> change ensures that the MHI layer is reset to initial state and
> ready for MHI initialization and power up.
> 
> Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
> ---
>  v2: reset only if necessary
>  v3: do not wait for MHI readiness in PBL context
> 
>  drivers/bus/mhi/pci_generic.c | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/drivers/bus/mhi/pci_generic.c 
> b/drivers/bus/mhi/pci_generic.c
> index c20f59e..87abd7c 100644
> --- a/drivers/bus/mhi/pci_generic.c
> +++ b/drivers/bus/mhi/pci_generic.c
> @@ -17,6 +17,8 @@
>  #include <linux/timer.h>
>  #include <linux/workqueue.h>
> 
> +#include "core/internal.h"
> +
>  #define MHI_PCI_DEFAULT_BAR_NUM 0
> 
>  #define MHI_POST_RESET_DELAY_MS 500
> @@ -256,6 +258,7 @@ static int mhi_pci_claim(struct mhi_controller 
> *mhi_cntrl,
>  		return err;
>  	}
>  	mhi_cntrl->regs = pcim_iomap_table(pdev)[bar_num];
> +	mhi_cntrl->bhi = mhi_cntrl->regs + readl(mhi_cntrl->regs + BHIOFF);
> 
>  	err = pci_set_dma_mask(pdev, dma_mask);
>  	if (err) {
> @@ -391,6 +394,31 @@ static void health_check(struct timer_list *t)
>  	mod_timer(&mhi_pdev->health_check_timer, jiffies + 
> HEALTH_CHECK_PERIOD);
>  }
> 
> +static void __mhi_sw_reset(struct mhi_controller *mhi_cntrl)
> +{
> +	unsigned int max_wait_ready = 100;
> +
> +	if (MHI_IN_PBL(mhi_get_exec_env(mhi_cntrl))) {
> +		/* nothing to do, ready for BHI */
> +		return;
> +	}
> +
> +	if (mhi_get_mhi_state(mhi_cntrl) >= MHI_STATE_M0) {
> +		dev_warn(mhi_cntrl->cntrl_dev, "Need reset\n");
> +		writel(MHICTRL_RESET_MASK, mhi_cntrl->regs + MHICTRL);
> +		msleep(10);
> +	}
> +
> +	while (mhi_get_mhi_state(mhi_cntrl) != MHI_STATE_READY) {
> +		if (!max_wait_ready--) {
> +			dev_warn(mhi_cntrl->cntrl_dev, "Not ready (state %u)\n",
> +				 mhi_get_mhi_state(mhi_cntrl));
> +			break;
> +		}
> +		msleep(50);
> +	}
> +}
> +
>  static int mhi_pci_probe(struct pci_dev *pdev, const struct 
> pci_device_id *id)
>  {
>  	const struct mhi_pci_dev_info *info = (struct mhi_pci_dev_info *)
> id->driver_data;
> @@ -451,6 +479,9 @@ static int mhi_pci_probe(struct pci_dev *pdev,
> const struct pci_device_id *id)
>  		goto err_unregister;
>  	}
> 
> +	/* Before starting MHI, ensure device is in good initial state */
> +	__mhi_sw_reset(mhi_cntrl);
> +
>  	err = mhi_sync_power_up(mhi_cntrl);
>  	if (err) {
>  		dev_err(&pdev->dev, "failed to power up MHI controller\n");
> @@ -532,6 +563,8 @@ static void mhi_pci_reset_done(struct pci_dev 
> *pdev)
>  		return;
>  	}
> 
> +	__mhi_sw_reset(mhi_cntrl);
> +
>  	err = mhi_sync_power_up(mhi_cntrl);
>  	if (err) {
>  		dev_err(&pdev->dev, "failed to power up MHI controller\n");

I have a set of changes coming in where you don't need to set BHI before 
the
mhi_sync_power_up() call. I will be moving it to 
mhi_prepare_for_power_up().

I will share the changes in the next patch series so you don't have to 
set it.

Another thing I was thinking: we could expose a function to issue an MHI 
RESET
so controllers have the freedom to use it if EE makes it possible.

This way we avoid the +#include "core/internal.h" and maintain a clear 
distinction
between controller and core.

Thanks,
Bhaumik
---
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum,
a Linux Foundation Collaborative Project