On Fri, May 08, 2020 at 07:26:45PM -0700, Bhaumik Bhatt wrote:
> Upon power up, driver queues firmware worker thread if the execution
> environment is PBL. Firmware worker is blocked with a timeout until
> state worker gets a chance to run and unblock firmware worker. An
> endpoint power up failure can be seen if state worker gets a chance to
> run after firmware worker has timed out. Remove this dependency and
> handle firmware load directly using state worker thread.
>
> Signed-off-by: Bhaumik Bhatt <bbhatt@xxxxxxxxxxxxxx>
> Reviewed-by: Jeffrey Hugo <jhugo@xxxxxxxxxxxxxx>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
Thanks,
Mani
> ---
> drivers/bus/mhi/core/boot.c | 18 +++---------------
> drivers/bus/mhi/core/init.c | 1 -
> drivers/bus/mhi/core/internal.h | 1 +
> drivers/bus/mhi/core/pm.c | 6 +-----
> include/linux/mhi.h | 2 --
> 5 files changed, 5 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/bus/mhi/core/boot.c b/drivers/bus/mhi/core/boot.c
> index ebad5eb..17c636b 100644
> --- a/drivers/bus/mhi/core/boot.c
> +++ b/drivers/bus/mhi/core/boot.c
> @@ -377,30 +377,18 @@ static void mhi_firmware_copy(struct mhi_controller *mhi_cntrl,
> }
> }
>
> -void mhi_fw_load_worker(struct work_struct *work)
> +void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl)
> {
> - struct mhi_controller *mhi_cntrl;
> const struct firmware *firmware = NULL;
> struct image_info *image_info;
> - struct device *dev;
> + struct device *dev = &mhi_cntrl->mhi_dev->dev;
> const char *fw_name;
> void *buf;
> dma_addr_t dma_addr;
> size_t size;
> int ret;
>
> - mhi_cntrl = container_of(work, struct mhi_controller, fw_worker);
> - dev = &mhi_cntrl->mhi_dev->dev;
> -
> - dev_dbg(dev, "Waiting for device to enter PBL from: %s\n",
> - TO_MHI_EXEC_STR(mhi_cntrl->ee));
> -
> - ret = wait_event_timeout(mhi_cntrl->state_event,
> - MHI_IN_PBL(mhi_cntrl->ee) ||
> - MHI_PM_IN_ERROR_STATE(mhi_cntrl->pm_state),
> - msecs_to_jiffies(mhi_cntrl->timeout_ms));
> -
> - if (!ret || MHI_PM_IN_ERROR_STATE(mhi_cntrl->pm_state)) {
> + if (MHI_PM_IN_ERROR_STATE(mhi_cntrl->pm_state)) {
> dev_err(dev, "Device MHI is not in valid state\n");
> return;
> }
> diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
> index 1a93d24..6882206 100644
> --- a/drivers/bus/mhi/core/init.c
> +++ b/drivers/bus/mhi/core/init.c
> @@ -835,7 +835,6 @@ int mhi_register_controller(struct mhi_controller *mhi_cntrl,
> spin_lock_init(&mhi_cntrl->wlock);
> INIT_WORK(&mhi_cntrl->st_worker, mhi_pm_st_worker);
> INIT_WORK(&mhi_cntrl->syserr_worker, mhi_pm_sys_err_worker);
> - INIT_WORK(&mhi_cntrl->fw_worker, mhi_fw_load_worker);
> init_waitqueue_head(&mhi_cntrl->state_event);
>
> mhi_cmd = mhi_cntrl->mhi_cmd;
> diff --git a/drivers/bus/mhi/core/internal.h b/drivers/bus/mhi/core/internal.h
> index 40c47f9..0965ca3 100644
> --- a/drivers/bus/mhi/core/internal.h
> +++ b/drivers/bus/mhi/core/internal.h
> @@ -627,6 +627,7 @@ void mhi_ring_chan_db(struct mhi_controller *mhi_cntrl,
> void mhi_deinit_free_irq(struct mhi_controller *mhi_cntrl);
> void 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,
> struct mhi_chan *mhi_chan);
> int mhi_init_chan_ctxt(struct mhi_controller *mhi_cntrl,
> diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
> index e7c8318..3cc238a 100644
> --- a/drivers/bus/mhi/core/pm.c
> +++ b/drivers/bus/mhi/core/pm.c
> @@ -528,7 +528,6 @@ static void mhi_pm_disable_transition(struct mhi_controller *mhi_cntrl,
> dev_dbg(dev, "Waiting for all pending threads to complete\n");
> wake_up_all(&mhi_cntrl->state_event);
> flush_work(&mhi_cntrl->st_worker);
> - flush_work(&mhi_cntrl->fw_worker);
>
> dev_dbg(dev, "Reset all active channels and remove MHI devices\n");
> device_for_each_child(mhi_cntrl->cntrl_dev, NULL, mhi_destroy_device);
> @@ -643,7 +642,7 @@ void mhi_pm_st_worker(struct work_struct *work)
> mhi_cntrl->ee = mhi_get_exec_env(mhi_cntrl);
> write_unlock_irq(&mhi_cntrl->pm_lock);
> if (MHI_IN_PBL(mhi_cntrl->ee))
> - wake_up_all(&mhi_cntrl->state_event);
> + mhi_fw_load_handler(mhi_cntrl);
> break;
> case DEV_ST_TRANSITION_SBL:
> write_lock_irq(&mhi_cntrl->pm_lock);
> @@ -976,9 +975,6 @@ int mhi_async_power_up(struct mhi_controller *mhi_cntrl)
> next_state = MHI_IN_PBL(current_ee) ?
> DEV_ST_TRANSITION_PBL : DEV_ST_TRANSITION_READY;
>
> - if (next_state == DEV_ST_TRANSITION_PBL)
> - schedule_work(&mhi_cntrl->fw_worker);
> -
> mhi_queue_state_transition(mhi_cntrl, next_state);
>
> mutex_unlock(&mhi_cntrl->pm_mutex);
> diff --git a/include/linux/mhi.h b/include/linux/mhi.h
> index e909b8f..2b20b9c 100644
> --- a/include/linux/mhi.h
> +++ b/include/linux/mhi.h
> @@ -331,7 +331,6 @@ struct mhi_controller_config {
> * @wlock: Lock for protecting device wakeup
> * @mhi_link_info: Device bandwidth info
> * @st_worker: State transition worker
> - * @fw_worker: Firmware download worker
> * @syserr_worker: System error worker
> * @state_event: State change event
> * @status_cb: CB function to notify power states of the device (required)
> @@ -412,7 +411,6 @@ struct mhi_controller {
> spinlock_t wlock;
> struct mhi_link_info mhi_link_info;
> struct work_struct st_worker;
> - struct work_struct fw_worker;
> struct work_struct syserr_worker;
> wait_queue_head_t state_event;
>
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
