* RE: [PATCH v2] bus: mhi: core: Add support MHI EE FP for download firmware
@ 2020-11-03 1:47 Carl Yin(殷张成)
2020-11-03 17:32 ` Bhaumik Bhatt
0 siblings, 1 reply; 7+ messages in thread
From: Carl Yin(殷张成) @ 2020-11-03 1:47 UTC (permalink / raw)
To: bbhatt
Cc: manivannan.sadhasivam, hemantk, sfr, linux-arm-msm, linux-kernel,
Naveen Kumar
Hi bbhatt:
On November 03, 2020 12:34 AM, Bhatt wrote:
> On 2020-11-02 04:27, carl.yin@quectel.com wrote:
> > From: "carl.yin" <carl.yin@quectel.com>
> >
> > MHI wwan modems support download firmware to nand or emmc by firehose
> > protocol, process as next:
> > 1. modem boot up and enter EE AMSS, create DIAG channels (4, 5) device
> > 2. user space tool send EDL command via DIAG channel,
> > then modem enter EE EDL
> > 3. boot.c download 'flash programmer image' via BHI interface 4. modem
> > enter EE FP, and create EDL channels (34, 35) device 5. user space
> > tool download 'firmware image' to modem via EDL channels
> > by firehose protocol
> >
> > Signed-off-by: carl.yin <carl.yin@quectel.com>
> > ---
> > drivers/bus/mhi/core/init.c | 2 ++
> > drivers/bus/mhi/core/internal.h | 1 +
> > drivers/bus/mhi/core/main.c | 5 ++++-
> > drivers/bus/mhi/core/pm.c | 13 ++++++++++++-
> > include/linux/mhi.h | 4 +++-
> > 5 files changed, 22 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
> > index ac4aa5c..e34616b 100644
> > --- a/drivers/bus/mhi/core/init.c
> > +++ b/drivers/bus/mhi/core/init.c
> > @@ -26,6 +26,7 @@ const char * const mhi_ee_str[MHI_EE_MAX] = {
> > [MHI_EE_WFW] = "WFW",
> > [MHI_EE_PTHRU] = "PASS THRU",
> > [MHI_EE_EDL] = "EDL",
> > + [MHI_EE_FP] = "FLASH PROGRAMMER",
> > [MHI_EE_DISABLE_TRANSITION] = "DISABLE",
> > [MHI_EE_NOT_SUPPORTED] = "NOT SUPPORTED", }; @@ -35,6 +36,7 @@
> > const char * const dev_state_tran_str[DEV_ST_TRANSITION_MAX] = {
> > [DEV_ST_TRANSITION_READY] = "READY",
> > [DEV_ST_TRANSITION_SBL] = "SBL",
> > [DEV_ST_TRANSITION_MISSION_MODE] = "MISSION_MODE",
> > + [DEV_ST_TRANSITION_FP] = "FLASH_PROGRAMMER",
> > [DEV_ST_TRANSITION_SYS_ERR] = "SYS_ERR",
> > [DEV_ST_TRANSITION_DISABLE] = "DISABLE", }; diff --git
> > a/drivers/bus/mhi/core/internal.h b/drivers/bus/mhi/core/internal.h
> > index 4abf0cf..6ae897a 100644
> > --- a/drivers/bus/mhi/core/internal.h
> > +++ b/drivers/bus/mhi/core/internal.h
> > @@ -386,6 +386,7 @@ enum dev_st_transition {
> > DEV_ST_TRANSITION_READY,
> > DEV_ST_TRANSITION_SBL,
> > DEV_ST_TRANSITION_MISSION_MODE,
> > + DEV_ST_TRANSITION_FP,
> > DEV_ST_TRANSITION_SYS_ERR,
> > DEV_ST_TRANSITION_DISABLE,
> > DEV_ST_TRANSITION_MAX,
> > diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
> > index 3950792..a1e1561 100644
> > --- a/drivers/bus/mhi/core/main.c
> > +++ b/drivers/bus/mhi/core/main.c
> > @@ -422,7 +422,7 @@ irqreturn_t mhi_intvec_threaded_handler(int
> > irq_number, void *priv)
> > wake_up_all(&mhi_cntrl->state_event);
> >
> > /* For fatal errors, we let controller decide next step */
> > - if (MHI_IN_PBL(ee))
> > + if (MHI_IN_PBL(mhi_cntrl->ee))
> Let's please have this as a separate patch with a fixes tag, as it fixes a pre-existing
> bug. I am sure Mani would want this.
> > mhi_cntrl->status_cb(mhi_cntrl, MHI_CB_FATAL_ERROR);
> > else
> > mhi_pm_sys_err_handler(mhi_cntrl);
> > @@ -782,6 +782,9 @@ int mhi_process_ctrl_ev_ring(struct mhi_controller
> > *mhi_cntrl,
> > case MHI_EE_SBL:
> > st = DEV_ST_TRANSITION_SBL;
> > break;
> > + case MHI_EE_FP:
> > + st = DEV_ST_TRANSITION_FP;
> > + break;
> When do you get this EE event on the control event ring? Does it come by after
> you have detected EE as FP from mhi_sync_power_up() and move to ready and
> then M0?
[carl.yin] it is from the log, next is the sdx24's log.
Another thing, I find mhi controller's status_cb() should not directly call mhi_power_down(),
The function stack is mhi_intvec_threaded_handler()->status_cb()->mhi_power_down()->free_irq().
Should not free irq in the irq thread.
[ 304.263111] mhi 0000:03:00.0: Preparing channel: 4
[ 304.275580] mhi 0000:03:00.0: Chan: 4 successfully moved to start state
[ 304.275580] mhi 0000:03:00.0: Preparing channel: 5
[ 304.285901] mhi 0000:03:00.0: Chan: 5 successfully moved to start state
[ 307.380999] mhi 0000:03:00.0: local ee:EDL device ee:AMSS dev_state:SYS_ERR
[ 307.381000] mhi 0000:03:00.0: System error detected
[ 307.381001] mhi-pci-generic 0000:03:00.0: mhi_pci_status_cb ee=6, cb=7
[ 307.381006] mhi 0000:03:00.0: Handling state transition: DISABLE
[ 307.381007] mhi 0000:03:00.0: Transitioning from PM state: SYS_ERR Detect to: SHUTDOWN Process
[ 307.381008] mhi 0000:03:00.0: Triggering MHI Reset in device
[ 307.381114] mhi 0000:03:00.0: local ee:EDL device ee:DISABLE dev_state:RESET
[ 307.381120] mhi 0000:03:00.0: Waiting for all pending event ring processing to complete
[ 307.381120] mhi 0000:03:00.0: Waiting for all pending threads to complete
[ 307.381121] mhi 0000:03:00.0: Reset all active channels and remove MHI devices
......
[ 307.402279] mhi 0000:03:00.0: Resetting EV CTXT and CMD CTXT
[ 307.402280] mhi 0000:03:00.0: Exiting with PM state: DISABLE, MHI state: RESET
[ 307.402567] mhi 0000:03:00.0: Requested to power ON
[ 307.402845] mhi 0000:03:00.0: Power on setup success
[ 307.402847] mhi 0000:03:00.0: Handling state transition: PBL
[ 307.402985] mhi 0000:03:00.0: Starting SBL download via BHI. Session ID:1048927456
[ 307.435259] mhi 0000:03:00.0: local ee:EDL device ee:EDL dev_state:RESET
[ 308.460322] mhi 0000:03:00.0: local ee:FLASH PROGRAMMER device ee:EDL dev_state:READY
[ 308.460326] mhi 0000:03:00.0: Handling state transition: READY
[ 308.460335] mhi 0000:03:00.0: Device in READY State
[ 308.460335] mhi 0000:03:00.0: Initializing MHI registers
[ 308.464301] mhi 0000:03:00.0: State change event to state: M0
[ 308.465303] mhi 0000:03:00.0: Received EE event: FLASH PROGRAMMER
[ 308.465311] mhi 0000:03:00.0: Handling state transition: FLASH_PROGRAMMER
[ 309.381506] mhi 0000:03:00.0: Preparing channel: 34
[ 309.384631] mhi 0000:03:00.0: Chan: 34 successfully moved to start state
[ 309.384631] mhi 0000:03:00.0: Preparing channel: 35
[ 309.387910] mhi 0000:03:00.0: Chan: 35 successfully moved to start state
> > case MHI_EE_WFW:
> > case MHI_EE_AMSS:
> > st = DEV_ST_TRANSITION_MISSION_MODE; diff --git
> > a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c index
> > 3de7b16..2d68812 100644
> > --- a/drivers/bus/mhi/core/pm.c
> > +++ b/drivers/bus/mhi/core/pm.c
> > @@ -658,6 +658,12 @@ void mhi_pm_st_worker(struct work_struct *work)
> > case DEV_ST_TRANSITION_MISSION_MODE:
> > mhi_pm_mission_mode_transition(mhi_cntrl);
> > break;
> > + case DEV_ST_TRANSITION_FP:
> > + write_lock_irq(&mhi_cntrl->pm_lock);
> > + mhi_cntrl->ee = MHI_EE_FP;
> > + write_unlock_irq(&mhi_cntrl->pm_lock);
> > + mhi_create_devices(mhi_cntrl);
> > + break;
> > case DEV_ST_TRANSITION_READY:
> > mhi_ready_state_transition(mhi_cntrl);
> > break;
> > @@ -1077,10 +1083,15 @@ int mhi_sync_power_up(struct mhi_controller
> > *mhi_cntrl)
> >
> > wait_event_timeout(mhi_cntrl->state_event,
> > MHI_IN_MISSION_MODE(mhi_cntrl->ee) ||
> > + mhi_cntrl->ee == MHI_EE_FP ||
> > MHI_PM_IN_ERROR_STATE(mhi_cntrl->pm_state),
> > msecs_to_jiffies(mhi_cntrl->timeout_ms));
> >
> > - ret = (MHI_IN_MISSION_MODE(mhi_cntrl->ee)) ? 0 : -ETIMEDOUT;
> > + if (mhi_cntrl->ee == MHI_EE_FP)
> > + mhi_queue_state_transition(mhi_cntrl,
> DEV_ST_TRANSITION_READY);
> > + else
> > + ret = (MHI_IN_MISSION_MODE(mhi_cntrl->ee)) ? 0 : -ETIMEDOUT;
> > +
> > if (ret)
> > mhi_power_down(mhi_cntrl, false);
> >
> We should come up with a better design for this later on.
> > diff --git a/include/linux/mhi.h b/include/linux/mhi.h index
> > 6e1122c..4620af8 100644
> > --- a/include/linux/mhi.h
> > +++ b/include/linux/mhi.h
> > @@ -120,6 +120,7 @@ struct mhi_link_info {
> > * @MHI_EE_WFW: WLAN firmware mode
> > * @MHI_EE_PTHRU: Passthrough
> > * @MHI_EE_EDL: Embedded downloader
> > + * @MHI_EE_FP, Flash Programmer Environment
> > */
> > enum mhi_ee_type {
> > MHI_EE_PBL,
> > @@ -129,7 +130,8 @@ enum mhi_ee_type {
> > MHI_EE_WFW,
> > MHI_EE_PTHRU,
> > MHI_EE_EDL,
> > - MHI_EE_MAX_SUPPORTED = MHI_EE_EDL,
> > + MHI_EE_FP,
> > + MHI_EE_MAX_SUPPORTED = MHI_EE_FP,
> > MHI_EE_DISABLE_TRANSITION, /* local EE, not related to mhi spec */
> > MHI_EE_NOT_SUPPORTED,
> > MHI_EE_MAX,
>
> Thanks,
> Bhaumik
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a
> Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] bus: mhi: core: Add support MHI EE FP for download firmware
2020-11-03 1:47 [PATCH v2] bus: mhi: core: Add support MHI EE FP for download firmware Carl Yin(殷张成)
@ 2020-11-03 17:32 ` Bhaumik Bhatt
2020-11-13 2:48 ` 答复: " Carl Yin(殷张成)
0 siblings, 1 reply; 7+ messages in thread
From: Bhaumik Bhatt @ 2020-11-03 17:32 UTC (permalink / raw)
To: Carl Yin(殷张成)
Cc: manivannan.sadhasivam, hemantk, sfr, linux-arm-msm, linux-kernel,
Naveen Kumar
On 2020-11-02 17:47, Carl Yin wrote:
> Hi bbhatt:
>
> On November 03, 2020 12:34 AM, Bhatt wrote:
>> On 2020-11-02 04:27, carl.yin@quectel.com wrote:
>> > From: "carl.yin" <carl.yin@quectel.com>
>> >
>> > MHI wwan modems support download firmware to nand or emmc by firehose
>> > protocol, process as next:
>> > 1. modem boot up and enter EE AMSS, create DIAG channels (4, 5) device
>> > 2. user space tool send EDL command via DIAG channel,
>> > then modem enter EE EDL
>> > 3. boot.c download 'flash programmer image' via BHI interface 4. modem
>> > enter EE FP, and create EDL channels (34, 35) device 5. user space
>> > tool download 'firmware image' to modem via EDL channels
>> > by firehose protocol
>> >
>> > Signed-off-by: carl.yin <carl.yin@quectel.com>
>> > ---
>> > drivers/bus/mhi/core/init.c | 2 ++
>> > drivers/bus/mhi/core/internal.h | 1 +
>> > drivers/bus/mhi/core/main.c | 5 ++++-
>> > drivers/bus/mhi/core/pm.c | 13 ++++++++++++-
>> > include/linux/mhi.h | 4 +++-
>> > 5 files changed, 22 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
>> > index ac4aa5c..e34616b 100644
>> > --- a/drivers/bus/mhi/core/init.c
>> > +++ b/drivers/bus/mhi/core/init.c
>> > @@ -26,6 +26,7 @@ const char * const mhi_ee_str[MHI_EE_MAX] = {
>> > [MHI_EE_WFW] = "WFW",
>> > [MHI_EE_PTHRU] = "PASS THRU",
>> > [MHI_EE_EDL] = "EDL",
>> > + [MHI_EE_FP] = "FLASH PROGRAMMER",
>> > [MHI_EE_DISABLE_TRANSITION] = "DISABLE",
>> > [MHI_EE_NOT_SUPPORTED] = "NOT SUPPORTED", }; @@ -35,6 +36,7 @@
>> > const char * const dev_state_tran_str[DEV_ST_TRANSITION_MAX] = {
>> > [DEV_ST_TRANSITION_READY] = "READY",
>> > [DEV_ST_TRANSITION_SBL] = "SBL",
>> > [DEV_ST_TRANSITION_MISSION_MODE] = "MISSION_MODE",
>> > + [DEV_ST_TRANSITION_FP] = "FLASH_PROGRAMMER",
>> > [DEV_ST_TRANSITION_SYS_ERR] = "SYS_ERR",
>> > [DEV_ST_TRANSITION_DISABLE] = "DISABLE", }; diff --git
>> > a/drivers/bus/mhi/core/internal.h b/drivers/bus/mhi/core/internal.h
>> > index 4abf0cf..6ae897a 100644
>> > --- a/drivers/bus/mhi/core/internal.h
>> > +++ b/drivers/bus/mhi/core/internal.h
>> > @@ -386,6 +386,7 @@ enum dev_st_transition {
>> > DEV_ST_TRANSITION_READY,
>> > DEV_ST_TRANSITION_SBL,
>> > DEV_ST_TRANSITION_MISSION_MODE,
>> > + DEV_ST_TRANSITION_FP,
>> > DEV_ST_TRANSITION_SYS_ERR,
>> > DEV_ST_TRANSITION_DISABLE,
>> > DEV_ST_TRANSITION_MAX,
>> > diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
>> > index 3950792..a1e1561 100644
>> > --- a/drivers/bus/mhi/core/main.c
>> > +++ b/drivers/bus/mhi/core/main.c
>> > @@ -422,7 +422,7 @@ irqreturn_t mhi_intvec_threaded_handler(int
>> > irq_number, void *priv)
>> > wake_up_all(&mhi_cntrl->state_event);
>> >
>> > /* For fatal errors, we let controller decide next step */
>> > - if (MHI_IN_PBL(ee))
>> > + if (MHI_IN_PBL(mhi_cntrl->ee))
>> Let's please have this as a separate patch with a fixes tag, as it
>> fixes a pre-existing
>> bug. I am sure Mani would want this.
>> > mhi_cntrl->status_cb(mhi_cntrl, MHI_CB_FATAL_ERROR);
>> > else
>> > mhi_pm_sys_err_handler(mhi_cntrl);
>> > @@ -782,6 +782,9 @@ int mhi_process_ctrl_ev_ring(struct mhi_controller
>> > *mhi_cntrl,
>> > case MHI_EE_SBL:
>> > st = DEV_ST_TRANSITION_SBL;
>> > break;
>> > + case MHI_EE_FP:
>> > + st = DEV_ST_TRANSITION_FP;
>> > + break;
>> When do you get this EE event on the control event ring? Does it come
>> by after
>> you have detected EE as FP from mhi_sync_power_up() and move to ready
>> and
>> then M0?
> [carl.yin] it is from the log, next is the sdx24's log.
> Another thing, I find mhi controller's status_cb() should not directly
> call mhi_power_down(),
> The function stack is
> mhi_intvec_threaded_handler()->status_cb()->mhi_power_down()->free_irq().
> Should not free irq in the irq thread.
>
> [ 304.263111] mhi 0000:03:00.0: Preparing channel: 4
> [ 304.275580] mhi 0000:03:00.0: Chan: 4 successfully moved to start
> state
> [ 304.275580] mhi 0000:03:00.0: Preparing channel: 5
> [ 304.285901] mhi 0000:03:00.0: Chan: 5 successfully moved to start
> state
> [ 307.380999] mhi 0000:03:00.0: local ee:EDL device ee:AMSS
> dev_state:SYS_ERR
> [ 307.381000] mhi 0000:03:00.0: System error detected
> [ 307.381001] mhi-pci-generic 0000:03:00.0: mhi_pci_status_cb ee=6,
> cb=7
> [ 307.381006] mhi 0000:03:00.0: Handling state transition: DISABLE
> [ 307.381007] mhi 0000:03:00.0: Transitioning from PM state: SYS_ERR
> Detect to: SHUTDOWN Process
> [ 307.381008] mhi 0000:03:00.0: Triggering MHI Reset in device
> [ 307.381114] mhi 0000:03:00.0: local ee:EDL device ee:DISABLE
> dev_state:RESET
> [ 307.381120] mhi 0000:03:00.0: Waiting for all pending event ring
> processing to complete
> [ 307.381120] mhi 0000:03:00.0: Waiting for all pending threads to
> complete
> [ 307.381121] mhi 0000:03:00.0: Reset all active channels and remove
> MHI devices
> ......
> [ 307.402279] mhi 0000:03:00.0: Resetting EV CTXT and CMD CTXT
> [ 307.402280] mhi 0000:03:00.0: Exiting with PM state: DISABLE, MHI
> state: RESET
> [ 307.402567] mhi 0000:03:00.0: Requested to power ON
> [ 307.402845] mhi 0000:03:00.0: Power on setup success
> [ 307.402847] mhi 0000:03:00.0: Handling state transition: PBL
> [ 307.402985] mhi 0000:03:00.0: Starting SBL download via BHI.
> Session ID:1048927456
> [ 307.435259] mhi 0000:03:00.0: local ee:EDL device ee:EDL
> dev_state:RESET
> [ 308.460322] mhi 0000:03:00.0: local ee:FLASH PROGRAMMER device
> ee:EDL dev_state:READY
> [ 308.460326] mhi 0000:03:00.0: Handling state transition: READY
> [ 308.460335] mhi 0000:03:00.0: Device in READY State
> [ 308.460335] mhi 0000:03:00.0: Initializing MHI registers
> [ 308.464301] mhi 0000:03:00.0: State change event to state: M0
> [ 308.465303] mhi 0000:03:00.0: Received EE event: FLASH PROGRAMMER
> [ 308.465311] mhi 0000:03:00.0: Handling state transition:
> FLASH_PROGRAMMER
> [ 309.381506] mhi 0000:03:00.0: Preparing channel: 34
> [ 309.384631] mhi 0000:03:00.0: Chan: 34 successfully moved to start
> state
> [ 309.384631] mhi 0000:03:00.0: Preparing channel: 35
> [ 309.387910] mhi 0000:03:00.0: Chan: 35 successfully moved to start
> state
>
Yes, you should not call mhi_power_down() from the status_cb() directly.
It is a
blocking function which should not called from a threaded IRQ handler,
if
possible and it also ends up freeing the IRQ from the handler itself
which should
not be done. Can you spawn a separate thread to do this work?
>> > case MHI_EE_WFW:
>> > case MHI_EE_AMSS:
>> > st = DEV_ST_TRANSITION_MISSION_MODE; diff --git
>> > a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c index
>> > 3de7b16..2d68812 100644
>> > --- a/drivers/bus/mhi/core/pm.c
>> > +++ b/drivers/bus/mhi/core/pm.c
>> > @@ -658,6 +658,12 @@ void mhi_pm_st_worker(struct work_struct *work)
>> > case DEV_ST_TRANSITION_MISSION_MODE:
>> > mhi_pm_mission_mode_transition(mhi_cntrl);
>> > break;
>> > + case DEV_ST_TRANSITION_FP:
>> > + write_lock_irq(&mhi_cntrl->pm_lock);
>> > + mhi_cntrl->ee = MHI_EE_FP;
>> > + write_unlock_irq(&mhi_cntrl->pm_lock);
>> > + mhi_create_devices(mhi_cntrl);
>> > + break;
>> > case DEV_ST_TRANSITION_READY:
>> > mhi_ready_state_transition(mhi_cntrl);
>> > break;
>> > @@ -1077,10 +1083,15 @@ int mhi_sync_power_up(struct mhi_controller
>> > *mhi_cntrl)
>> >
>> > wait_event_timeout(mhi_cntrl->state_event,
>> > MHI_IN_MISSION_MODE(mhi_cntrl->ee) ||
>> > + mhi_cntrl->ee == MHI_EE_FP ||
>> > MHI_PM_IN_ERROR_STATE(mhi_cntrl->pm_state),
>> > msecs_to_jiffies(mhi_cntrl->timeout_ms));
>> >
>> > - ret = (MHI_IN_MISSION_MODE(mhi_cntrl->ee)) ? 0 : -ETIMEDOUT;
>> > + if (mhi_cntrl->ee == MHI_EE_FP)
>> > + mhi_queue_state_transition(mhi_cntrl,
>> DEV_ST_TRANSITION_READY);
>> > + else
>> > + ret = (MHI_IN_MISSION_MODE(mhi_cntrl->ee)) ? 0 : -ETIMEDOUT;
>> > +
>> > if (ret)
>> > mhi_power_down(mhi_cntrl, false);
>> >
>> We should come up with a better design for this later on.
>> > diff --git a/include/linux/mhi.h b/include/linux/mhi.h index
>> > 6e1122c..4620af8 100644
>> > --- a/include/linux/mhi.h
>> > +++ b/include/linux/mhi.h
>> > @@ -120,6 +120,7 @@ struct mhi_link_info {
>> > * @MHI_EE_WFW: WLAN firmware mode
>> > * @MHI_EE_PTHRU: Passthrough
>> > * @MHI_EE_EDL: Embedded downloader
>> > + * @MHI_EE_FP, Flash Programmer Environment
>> > */
>> > enum mhi_ee_type {
>> > MHI_EE_PBL,
>> > @@ -129,7 +130,8 @@ enum mhi_ee_type {
>> > MHI_EE_WFW,
>> > MHI_EE_PTHRU,
>> > MHI_EE_EDL,
>> > - MHI_EE_MAX_SUPPORTED = MHI_EE_EDL,
>> > + MHI_EE_FP,
>> > + MHI_EE_MAX_SUPPORTED = MHI_EE_FP,
>> > MHI_EE_DISABLE_TRANSITION, /* local EE, not related to mhi spec */
>> > MHI_EE_NOT_SUPPORTED,
>> > MHI_EE_MAX,
>>
>> Thanks,
>> Bhaumik
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
>> Forum, a
>> Linux Foundation Collaborative Project
Thanks,
Bhaumik
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 7+ messages in thread
* 答复: [PATCH v2] bus: mhi: core: Add support MHI EE FP for download firmware
2020-11-03 17:32 ` Bhaumik Bhatt
@ 2020-11-13 2:48 ` Carl Yin(殷张成)
2020-11-17 3:27 ` Bhaumik Bhatt
0 siblings, 1 reply; 7+ messages in thread
From: Carl Yin(殷张成) @ 2020-11-13 2:48 UTC (permalink / raw)
To: bbhatt
Cc: manivannan.sadhasivam, hemantk, sfr, linux-arm-msm, linux-kernel,
Naveen Kumar
Hi bbhatt:
On November 04, 2020 1:32 AM, bbhatt wrote:
> On 2020-11-02 17:47, Carl Yin wrote:
> > Hi bbhatt:
> >
> > On November 03, 2020 12:34 AM, Bhatt wrote:
> >> On 2020-11-02 04:27, carl.yin@quectel.com wrote:
> >> > From: "carl.yin" <carl.yin@quectel.com>
> >> >
> >> > MHI wwan modems support download firmware to nand or emmc by
> >> > firehose protocol, process as next:
> >> > 1. modem boot up and enter EE AMSS, create DIAG channels (4, 5)
> >> > device 2. user space tool send EDL command via DIAG channel,
> >> > then modem enter EE EDL
> >> > 3. boot.c download 'flash programmer image' via BHI interface 4.
> >> > modem enter EE FP, and create EDL channels (34, 35) device 5. user
> >> > space tool download 'firmware image' to modem via EDL channels
> >> > by firehose protocol
> >> >
> >> > Signed-off-by: carl.yin <carl.yin@quectel.com>
[carl.yin] can you give some advice one above commit message? I am afraid it is clear and brief.
> >> > ---
> >> > drivers/bus/mhi/core/init.c | 2 ++
> >> > drivers/bus/mhi/core/internal.h | 1 +
> >> > drivers/bus/mhi/core/main.c | 5 ++++-
> >> > drivers/bus/mhi/core/pm.c | 13 ++++++++++++-
> >> > include/linux/mhi.h | 4 +++-
> >> > 5 files changed, 22 insertions(+), 3 deletions(-)
> >> >
> >> > diff --git a/drivers/bus/mhi/core/init.c
> >> > b/drivers/bus/mhi/core/init.c index ac4aa5c..e34616b 100644
> >> > --- a/drivers/bus/mhi/core/init.c
> >> > +++ b/drivers/bus/mhi/core/init.c
> >> > @@ -26,6 +26,7 @@ const char * const mhi_ee_str[MHI_EE_MAX] = {
> >> > [MHI_EE_WFW] = "WFW",
> >> > [MHI_EE_PTHRU] = "PASS THRU",
> >> > [MHI_EE_EDL] = "EDL",
> >> > + [MHI_EE_FP] = "FLASH PROGRAMMER",
> >> > [MHI_EE_DISABLE_TRANSITION] = "DISABLE",
> >> > [MHI_EE_NOT_SUPPORTED] = "NOT SUPPORTED", }; @@ -35,6 +36,7
> @@
> >> > const char * const dev_state_tran_str[DEV_ST_TRANSITION_MAX] = {
> >> > [DEV_ST_TRANSITION_READY] = "READY",
> >> > [DEV_ST_TRANSITION_SBL] = "SBL",
> >> > [DEV_ST_TRANSITION_MISSION_MODE] = "MISSION_MODE",
> >> > + [DEV_ST_TRANSITION_FP] = "FLASH_PROGRAMMER",
> >> > [DEV_ST_TRANSITION_SYS_ERR] = "SYS_ERR",
> >> > [DEV_ST_TRANSITION_DISABLE] = "DISABLE", }; diff --git
> >> > a/drivers/bus/mhi/core/internal.h b/drivers/bus/mhi/core/internal.h
> >> > index 4abf0cf..6ae897a 100644
> >> > --- a/drivers/bus/mhi/core/internal.h
> >> > +++ b/drivers/bus/mhi/core/internal.h
> >> > @@ -386,6 +386,7 @@ enum dev_st_transition {
> >> > DEV_ST_TRANSITION_READY,
> >> > DEV_ST_TRANSITION_SBL,
> >> > DEV_ST_TRANSITION_MISSION_MODE,
> >> > + DEV_ST_TRANSITION_FP,
> >> > DEV_ST_TRANSITION_SYS_ERR,
> >> > DEV_ST_TRANSITION_DISABLE,
> >> > DEV_ST_TRANSITION_MAX,
> >> > diff --git a/drivers/bus/mhi/core/main.c
> >> > b/drivers/bus/mhi/core/main.c index 3950792..a1e1561 100644
> >> > --- a/drivers/bus/mhi/core/main.c
> >> > +++ b/drivers/bus/mhi/core/main.c
> >> > @@ -422,7 +422,7 @@ irqreturn_t mhi_intvec_threaded_handler(int
> >> > irq_number, void *priv)
> >> > wake_up_all(&mhi_cntrl->state_event);
> >> >
> >> > /* For fatal errors, we let controller decide next step */
> >> > - if (MHI_IN_PBL(ee))
> >> > + if (MHI_IN_PBL(mhi_cntrl->ee))
> >> Let's please have this as a separate patch with a fixes tag, as it
> >> fixes a pre-existing bug. I am sure Mani would want this.
[carl.yin] should I wait this bug fixed, then submit patch v3?
> >> > mhi_cntrl->status_cb(mhi_cntrl, MHI_CB_FATAL_ERROR);
> >> > else
> >> > mhi_pm_sys_err_handler(mhi_cntrl);
> >> > @@ -782,6 +782,9 @@ int mhi_process_ctrl_ev_ring(struct
> >> > mhi_controller *mhi_cntrl,
> >> > case MHI_EE_SBL:
> >> > st = DEV_ST_TRANSITION_SBL;
> >> > break;
> >> > + case MHI_EE_FP:
> >> > + st = DEV_ST_TRANSITION_FP;
> >> > + break;
> >> When do you get this EE event on the control event ring? Does it come
> >> by after you have detected EE as FP from mhi_sync_power_up() and move
> >> to ready and then M0?
> > [carl.yin] it is from the log, next is the sdx24's log.
> > Another thing, I find mhi controller's status_cb() should not directly
> > call mhi_power_down(), The function stack is
> >
> mhi_intvec_threaded_handler()->status_cb()->mhi_power_down()->free_irq().
> > Should not free irq in the irq thread.
> >
> > [ 304.263111] mhi 0000:03:00.0: Preparing channel: 4 [ 304.275580]
> > mhi 0000:03:00.0: Chan: 4 successfully moved to start state [
> > 304.275580] mhi 0000:03:00.0: Preparing channel: 5 [ 304.285901] mhi
> > 0000:03:00.0: Chan: 5 successfully moved to start state [ 307.380999]
> > mhi 0000:03:00.0: local ee:EDL device ee:AMSS dev_state:SYS_ERR [
> > 307.381000] mhi 0000:03:00.0: System error detected [ 307.381001]
> > mhi-pci-generic 0000:03:00.0: mhi_pci_status_cb ee=6,
> > cb=7
> > [ 307.381006] mhi 0000:03:00.0: Handling state transition: DISABLE [
> > 307.381007] mhi 0000:03:00.0: Transitioning from PM state: SYS_ERR
> > Detect to: SHUTDOWN Process [ 307.381008] mhi 0000:03:00.0:
> > Triggering MHI Reset in device [ 307.381114] mhi 0000:03:00.0: local
> > ee:EDL device ee:DISABLE dev_state:RESET [ 307.381120] mhi
> > 0000:03:00.0: Waiting for all pending event ring processing to
> > complete [ 307.381120] mhi 0000:03:00.0: Waiting for all pending
> > threads to complete [ 307.381121] mhi 0000:03:00.0: Reset all active
> > channels and remove MHI devices ......
> > [ 307.402279] mhi 0000:03:00.0: Resetting EV CTXT and CMD CTXT [
> > 307.402280] mhi 0000:03:00.0: Exiting with PM state: DISABLE, MHI
> > state: RESET
> > [ 307.402567] mhi 0000:03:00.0: Requested to power ON [ 307.402845]
> > mhi 0000:03:00.0: Power on setup success [ 307.402847] mhi
> > 0000:03:00.0: Handling state transition: PBL [ 307.402985] mhi
> > 0000:03:00.0: Starting SBL download via BHI.
> > Session ID:1048927456
> > [ 307.435259] mhi 0000:03:00.0: local ee:EDL device ee:EDL
> > dev_state:RESET [ 308.460322] mhi 0000:03:00.0: local ee:FLASH
> > PROGRAMMER device ee:EDL dev_state:READY [ 308.460326] mhi
> > 0000:03:00.0: Handling state transition: READY [ 308.460335] mhi
> > 0000:03:00.0: Device in READY State [ 308.460335] mhi 0000:03:00.0:
> > Initializing MHI registers [ 308.464301] mhi 0000:03:00.0: State
> > change event to state: M0 [ 308.465303] mhi 0000:03:00.0: Received EE
> > event: FLASH PROGRAMMER [ 308.465311] mhi 0000:03:00.0: Handling
> > state transition:
> > FLASH_PROGRAMMER
> > [ 309.381506] mhi 0000:03:00.0: Preparing channel: 34 [ 309.384631]
> > mhi 0000:03:00.0: Chan: 34 successfully moved to start state [
> > 309.384631] mhi 0000:03:00.0: Preparing channel: 35 [ 309.387910] mhi
> > 0000:03:00.0: Chan: 35 successfully moved to start state
> >
> Yes, you should not call mhi_power_down() from the status_cb() directly.
> It is a
> blocking function which should not called from a threaded IRQ handler, if possible
> and it also ends up freeing the IRQ from the handler itself which should not be
> done. Can you spawn a separate thread to do this work?
> >> > case MHI_EE_WFW:
> >> > case MHI_EE_AMSS:
> >> > st = DEV_ST_TRANSITION_MISSION_MODE; diff --git
> >> > a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c index
> >> > 3de7b16..2d68812 100644
> >> > --- a/drivers/bus/mhi/core/pm.c
> >> > +++ b/drivers/bus/mhi/core/pm.c
> >> > @@ -658,6 +658,12 @@ void mhi_pm_st_worker(struct work_struct *work)
> >> > case DEV_ST_TRANSITION_MISSION_MODE:
> >> > mhi_pm_mission_mode_transition(mhi_cntrl);
> >> > break;
> >> > + case DEV_ST_TRANSITION_FP:
> >> > + write_lock_irq(&mhi_cntrl->pm_lock);
> >> > + mhi_cntrl->ee = MHI_EE_FP;
> >> > + write_unlock_irq(&mhi_cntrl->pm_lock);
> >> > + mhi_create_devices(mhi_cntrl);
> >> > + break;
> >> > case DEV_ST_TRANSITION_READY:
> >> > mhi_ready_state_transition(mhi_cntrl);
> >> > break;
> >> > @@ -1077,10 +1083,15 @@ int mhi_sync_power_up(struct mhi_controller
> >> > *mhi_cntrl)
> >> >
> >> > wait_event_timeout(mhi_cntrl->state_event,
> >> > MHI_IN_MISSION_MODE(mhi_cntrl->ee) ||
> >> > + mhi_cntrl->ee == MHI_EE_FP ||
> >> > MHI_PM_IN_ERROR_STATE(mhi_cntrl->pm_state),
> >> > msecs_to_jiffies(mhi_cntrl->timeout_ms));
> >> >
> >> > - ret = (MHI_IN_MISSION_MODE(mhi_cntrl->ee)) ? 0 : -ETIMEDOUT;
> >> > + if (mhi_cntrl->ee == MHI_EE_FP)
> >> > + mhi_queue_state_transition(mhi_cntrl,
> >> DEV_ST_TRANSITION_READY);
> >> > + else
> >> > + ret = (MHI_IN_MISSION_MODE(mhi_cntrl->ee)) ? 0 :
> -ETIMEDOUT;
> >> > +
> >> > if (ret)
> >> > mhi_power_down(mhi_cntrl, false);
> >> >
> >> We should come up with a better design for this later on.
[carl.yin] I move sync_power_up() to aysnc_power_up(), so modifies in this function is not need.
for I found some time the "wait" will block long time. "long time block" is not accept by other device's drivers.
I do the transition of EDL -> FP in next function.
--- a/drivers/bus/mhi/core/main.c
+++ b/drivers/bus/mhi/core/main.c
@@ -406,6 +406,9 @@ irqreturn_t mhi_intvec_threaded_handler(int irq_number, void *priv)
dev_dbg(dev, "System error detected\n");
pm_state = mhi_tryset_pm_state(mhi_cntrl,
MHI_PM_SYS_ERR_DETECT);
+ } else if (state == MHI_STATE_READY) {
+ if (ee == MHI_EE_EDL && mhi_cntrl->ee == MHI_EE_FP)
+ mhi_queue_state_transition(mhi_cntrl, DEV_ST_TRANSITION_READY);
}
write_unlock_irq(&mhi_cntrl->pm_lock);
and it also works well, next is the log.
[ 1048.424679] mhi 0000:03:00.0: Exiting with PM state: DISABLE, MHI state: RESET
[ 1048.424743] mhi 0000:03:00.0: Requested to power ON
[ 1048.424885] mhi 0000:03:00.0: Power on setup success
[ 1048.424887] mhi 0000:03:00.0: Handling state transition: PBL
[ 1048.425034] mhi 0000:03:00.0: Starting SBL download via BHI. Session ID:584611751
[ 1048.446610] mhi 0000:03:00.0: local ee:EDL device ee:EDL dev_state:RESET
[ 1049.469896] mhi 0000:03:00.0: local ee:FLASH PROGRAMMER device ee:EDL dev_state:READY
[ 1049.469919] mhi 0000:03:00.0: Handling state transition: READY
[ 1049.469930] mhi 0000:03:00.0: Device in READY State
[ 1049.469933] mhi 0000:03:00.0: Initializing MHI registers
[ 1049.473818] mhi 0000:03:00.0: State change event to state: M0
[ 1049.474850] mhi 0000:03:00.0: Received EE event: FLASH PROGRAMMER
[ 1049.474888] mhi 0000:03:00.0: Handling state transition: FLASH_PROGRAMMER
[ 1050.402805] mhi 0000:03:00.0: Preparing channel: 34
[ 1050.406175] mhi 0000:03:00.0: Chan: 34 successfully moved to start state
> >> > diff --git a/include/linux/mhi.h b/include/linux/mhi.h index
> >> > 6e1122c..4620af8 100644
> >> > --- a/include/linux/mhi.h
> >> > +++ b/include/linux/mhi.h
> >> > @@ -120,6 +120,7 @@ struct mhi_link_info {
> >> > * @MHI_EE_WFW: WLAN firmware mode
> >> > * @MHI_EE_PTHRU: Passthrough
> >> > * @MHI_EE_EDL: Embedded downloader
> >> > + * @MHI_EE_FP, Flash Programmer Environment
> >> > */
> >> > enum mhi_ee_type {
> >> > MHI_EE_PBL,
> >> > @@ -129,7 +130,8 @@ enum mhi_ee_type {
> >> > MHI_EE_WFW,
> >> > MHI_EE_PTHRU,
> >> > MHI_EE_EDL,
> >> > - MHI_EE_MAX_SUPPORTED = MHI_EE_EDL,
> >> > + MHI_EE_FP,
> >> > + MHI_EE_MAX_SUPPORTED = MHI_EE_FP,
> >> > MHI_EE_DISABLE_TRANSITION, /* local EE, not related to mhi spec */
> >> > MHI_EE_NOT_SUPPORTED,
> >> > MHI_EE_MAX,
> >>
> >> Thanks,
> >> Bhaumik
> >> --
> >> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
> >> Forum, a Linux Foundation Collaborative Project
>
> Thanks,
> Bhaumik
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a
> Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: 答复: [PATCH v2] bus: mhi: core: Add support MHI EE FP for download firmware
2020-11-13 2:48 ` 答复: " Carl Yin(殷张成)
@ 2020-11-17 3:27 ` Bhaumik Bhatt
0 siblings, 0 replies; 7+ messages in thread
From: Bhaumik Bhatt @ 2020-11-17 3:27 UTC (permalink / raw)
To: Carl Yin(殷张成)
Cc: manivannan.sadhasivam, hemantk, sfr, linux-arm-msm, linux-kernel,
Naveen Kumar
On 2020-11-12 06:48 PM, Carl Yin wrote:
> Hi bbhatt:
>
> On November 04, 2020 1:32 AM, bbhatt wrote:
>> On 2020-11-02 17:47, Carl Yin wrote:
>> > Hi bbhatt:
>> >
>> > On November 03, 2020 12:34 AM, Bhatt wrote:
>> >> On 2020-11-02 04:27, carl.yin@quectel.com wrote:
>> >> > From: "carl.yin" <carl.yin@quectel.com>
>> >> >
>> >> > MHI wwan modems support download firmware to nand or emmc by
>> >> > firehose protocol, process as next:
>> >> > 1. modem boot up and enter EE AMSS, create DIAG channels (4, 5)
>> >> > device 2. user space tool send EDL command via DIAG channel,
>> >> > then modem enter EE EDL
>> >> > 3. boot.c download 'flash programmer image' via BHI interface 4.
>> >> > modem enter EE FP, and create EDL channels (34, 35) device 5. user
>> >> > space tool download 'firmware image' to modem via EDL channels
>> >> > by firehose protocol
>> >> >
>> >> > Signed-off-by: carl.yin <carl.yin@quectel.com>
> [carl.yin] can you give some advice one above commit message? I am
> afraid it is clear and brief.
>
Sure. Can we sync-up on a separate thread to work on this patch?
>> >> > ---
>> >> > drivers/bus/mhi/core/init.c | 2 ++
>> >> > drivers/bus/mhi/core/internal.h | 1 +
>> >> > drivers/bus/mhi/core/main.c | 5 ++++-
>> >> > drivers/bus/mhi/core/pm.c | 13 ++++++++++++-
>> >> > include/linux/mhi.h | 4 +++-
>> >> > 5 files changed, 22 insertions(+), 3 deletions(-)
>> >> >
>> >> > diff --git a/drivers/bus/mhi/core/init.c
>> >> > b/drivers/bus/mhi/core/init.c index ac4aa5c..e34616b 100644
>> >> > --- a/drivers/bus/mhi/core/init.c
>> >> > +++ b/drivers/bus/mhi/core/init.c
>> >> > @@ -26,6 +26,7 @@ const char * const mhi_ee_str[MHI_EE_MAX] = {
>> >> > [MHI_EE_WFW] = "WFW",
>> >> > [MHI_EE_PTHRU] = "PASS THRU",
>> >> > [MHI_EE_EDL] = "EDL",
>> >> > + [MHI_EE_FP] = "FLASH PROGRAMMER",
>> >> > [MHI_EE_DISABLE_TRANSITION] = "DISABLE",
>> >> > [MHI_EE_NOT_SUPPORTED] = "NOT SUPPORTED", }; @@ -35,6 +36,7
>> @@
>> >> > const char * const dev_state_tran_str[DEV_ST_TRANSITION_MAX] = {
>> >> > [DEV_ST_TRANSITION_READY] = "READY",
>> >> > [DEV_ST_TRANSITION_SBL] = "SBL",
>> >> > [DEV_ST_TRANSITION_MISSION_MODE] = "MISSION_MODE",
>> >> > + [DEV_ST_TRANSITION_FP] = "FLASH_PROGRAMMER",
>> >> > [DEV_ST_TRANSITION_SYS_ERR] = "SYS_ERR",
>> >> > [DEV_ST_TRANSITION_DISABLE] = "DISABLE", }; diff --git
>> >> > a/drivers/bus/mhi/core/internal.h b/drivers/bus/mhi/core/internal.h
>> >> > index 4abf0cf..6ae897a 100644
>> >> > --- a/drivers/bus/mhi/core/internal.h
>> >> > +++ b/drivers/bus/mhi/core/internal.h
>> >> > @@ -386,6 +386,7 @@ enum dev_st_transition {
>> >> > DEV_ST_TRANSITION_READY,
>> >> > DEV_ST_TRANSITION_SBL,
>> >> > DEV_ST_TRANSITION_MISSION_MODE,
>> >> > + DEV_ST_TRANSITION_FP,
>> >> > DEV_ST_TRANSITION_SYS_ERR,
>> >> > DEV_ST_TRANSITION_DISABLE,
>> >> > DEV_ST_TRANSITION_MAX,
>> >> > diff --git a/drivers/bus/mhi/core/main.c
>> >> > b/drivers/bus/mhi/core/main.c index 3950792..a1e1561 100644
>> >> > --- a/drivers/bus/mhi/core/main.c
>> >> > +++ b/drivers/bus/mhi/core/main.c
>> >> > @@ -422,7 +422,7 @@ irqreturn_t mhi_intvec_threaded_handler(int
>> >> > irq_number, void *priv)
>> >> > wake_up_all(&mhi_cntrl->state_event);
>> >> >
>> >> > /* For fatal errors, we let controller decide next step */
>> >> > - if (MHI_IN_PBL(ee))
>> >> > + if (MHI_IN_PBL(mhi_cntrl->ee))
>> >> Let's please have this as a separate patch with a fixes tag, as it
>> >> fixes a pre-existing bug. I am sure Mani would want this.
> [carl.yin] should I wait this bug fixed, then submit patch v3?
>
I have introduced a fix for this, added you in CC, and waiting for
Mani's review.
It would be nice if you could wait until that is picked.
>> >> > mhi_cntrl->status_cb(mhi_cntrl, MHI_CB_FATAL_ERROR);
>> >> > else
>> >> > mhi_pm_sys_err_handler(mhi_cntrl);
>> >> > @@ -782,6 +782,9 @@ int mhi_process_ctrl_ev_ring(struct
>> >> > mhi_controller *mhi_cntrl,
>> >> > case MHI_EE_SBL:
>> >> > st = DEV_ST_TRANSITION_SBL;
>> >> > break;
>> >> > + case MHI_EE_FP:
>> >> > + st = DEV_ST_TRANSITION_FP;
>> >> > + break;
>> >> When do you get this EE event on the control event ring? Does it come
>> >> by after you have detected EE as FP from mhi_sync_power_up() and move
>> >> to ready and then M0?
>> > [carl.yin] it is from the log, next is the sdx24's log.
>> > Another thing, I find mhi controller's status_cb() should not directly
>> > call mhi_power_down(), The function stack is
>> >
>> mhi_intvec_threaded_handler()->status_cb()->mhi_power_down()->free_irq().
>> > Should not free irq in the irq thread.
>> >
>> > [ 304.263111] mhi 0000:03:00.0: Preparing channel: 4 [ 304.275580]
>> > mhi 0000:03:00.0: Chan: 4 successfully moved to start state [
>> > 304.275580] mhi 0000:03:00.0: Preparing channel: 5 [ 304.285901] mhi
>> > 0000:03:00.0: Chan: 5 successfully moved to start state [ 307.380999]
>> > mhi 0000:03:00.0: local ee:EDL device ee:AMSS dev_state:SYS_ERR [
>> > 307.381000] mhi 0000:03:00.0: System error detected [ 307.381001]
>> > mhi-pci-generic 0000:03:00.0: mhi_pci_status_cb ee=6,
>> > cb=7
>> > [ 307.381006] mhi 0000:03:00.0: Handling state transition: DISABLE [
>> > 307.381007] mhi 0000:03:00.0: Transitioning from PM state: SYS_ERR
>> > Detect to: SHUTDOWN Process [ 307.381008] mhi 0000:03:00.0:
>> > Triggering MHI Reset in device [ 307.381114] mhi 0000:03:00.0: local
>> > ee:EDL device ee:DISABLE dev_state:RESET [ 307.381120] mhi
>> > 0000:03:00.0: Waiting for all pending event ring processing to
>> > complete [ 307.381120] mhi 0000:03:00.0: Waiting for all pending
>> > threads to complete [ 307.381121] mhi 0000:03:00.0: Reset all active
>> > channels and remove MHI devices ......
>> > [ 307.402279] mhi 0000:03:00.0: Resetting EV CTXT and CMD CTXT [
>> > 307.402280] mhi 0000:03:00.0: Exiting with PM state: DISABLE, MHI
>> > state: RESET
>> > [ 307.402567] mhi 0000:03:00.0: Requested to power ON [ 307.402845]
>> > mhi 0000:03:00.0: Power on setup success [ 307.402847] mhi
>> > 0000:03:00.0: Handling state transition: PBL [ 307.402985] mhi
>> > 0000:03:00.0: Starting SBL download via BHI.
>> > Session ID:1048927456
>> > [ 307.435259] mhi 0000:03:00.0: local ee:EDL device ee:EDL
>> > dev_state:RESET [ 308.460322] mhi 0000:03:00.0: local ee:FLASH
>> > PROGRAMMER device ee:EDL dev_state:READY [ 308.460326] mhi
>> > 0000:03:00.0: Handling state transition: READY [ 308.460335] mhi
>> > 0000:03:00.0: Device in READY State [ 308.460335] mhi 0000:03:00.0:
>> > Initializing MHI registers [ 308.464301] mhi 0000:03:00.0: State
>> > change event to state: M0 [ 308.465303] mhi 0000:03:00.0: Received EE
>> > event: FLASH PROGRAMMER [ 308.465311] mhi 0000:03:00.0: Handling
>> > state transition:
>> > FLASH_PROGRAMMER
>> > [ 309.381506] mhi 0000:03:00.0: Preparing channel: 34 [ 309.384631]
>> > mhi 0000:03:00.0: Chan: 34 successfully moved to start state [
>> > 309.384631] mhi 0000:03:00.0: Preparing channel: 35 [ 309.387910] mhi
>> > 0000:03:00.0: Chan: 35 successfully moved to start state
>> >
>> Yes, you should not call mhi_power_down() from the status_cb()
>> directly.
>> It is a
>> blocking function which should not called from a threaded IRQ handler,
>> if possible
>> and it also ends up freeing the IRQ from the handler itself which
>> should not be
>> done. Can you spawn a separate thread to do this work?
>> >> > case MHI_EE_WFW:
>> >> > case MHI_EE_AMSS:
>> >> > st = DEV_ST_TRANSITION_MISSION_MODE; diff --git
>> >> > a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c index
>> >> > 3de7b16..2d68812 100644
>> >> > --- a/drivers/bus/mhi/core/pm.c
>> >> > +++ b/drivers/bus/mhi/core/pm.c
>> >> > @@ -658,6 +658,12 @@ void mhi_pm_st_worker(struct work_struct *work)
>> >> > case DEV_ST_TRANSITION_MISSION_MODE:
>> >> > mhi_pm_mission_mode_transition(mhi_cntrl);
>> >> > break;
>> >> > + case DEV_ST_TRANSITION_FP:
>> >> > + write_lock_irq(&mhi_cntrl->pm_lock);
>> >> > + mhi_cntrl->ee = MHI_EE_FP;
>> >> > + write_unlock_irq(&mhi_cntrl->pm_lock);
>> >> > + mhi_create_devices(mhi_cntrl);
>> >> > + break;
>> >> > case DEV_ST_TRANSITION_READY:
>> >> > mhi_ready_state_transition(mhi_cntrl);
>> >> > break;
>> >> > @@ -1077,10 +1083,15 @@ int mhi_sync_power_up(struct mhi_controller
>> >> > *mhi_cntrl)
>> >> >
>> >> > wait_event_timeout(mhi_cntrl->state_event,
>> >> > MHI_IN_MISSION_MODE(mhi_cntrl->ee) ||
>> >> > + mhi_cntrl->ee == MHI_EE_FP ||
>> >> > MHI_PM_IN_ERROR_STATE(mhi_cntrl->pm_state),
>> >> > msecs_to_jiffies(mhi_cntrl->timeout_ms));
>> >> >
>> >> > - ret = (MHI_IN_MISSION_MODE(mhi_cntrl->ee)) ? 0 : -ETIMEDOUT;
>> >> > + if (mhi_cntrl->ee == MHI_EE_FP)
>> >> > + mhi_queue_state_transition(mhi_cntrl,
>> >> DEV_ST_TRANSITION_READY);
>> >> > + else
>> >> > + ret = (MHI_IN_MISSION_MODE(mhi_cntrl->ee)) ? 0 :
>> -ETIMEDOUT;
>> >> > +
>> >> > if (ret)
>> >> > mhi_power_down(mhi_cntrl, false);
>> >> >
>> >> We should come up with a better design for this later on.
> [carl.yin] I move sync_power_up() to aysnc_power_up(), so modifies in
> this function is not need.
> for I found some time the "wait" will block long time. "long time
> block" is not accept by other device's drivers.
>
> I do the transition of EDL -> FP in next function.
> --- a/drivers/bus/mhi/core/main.c
> +++ b/drivers/bus/mhi/core/main.c
> @@ -406,6 +406,9 @@ irqreturn_t mhi_intvec_threaded_handler(int
> irq_number, void *priv)
> dev_dbg(dev, "System error detected\n");
> pm_state = mhi_tryset_pm_state(mhi_cntrl,
> MHI_PM_SYS_ERR_DETECT);
> + } else if (state == MHI_STATE_READY) {
> + if (ee == MHI_EE_EDL && mhi_cntrl->ee == MHI_EE_FP)
> + mhi_queue_state_transition(mhi_cntrl,
> DEV_ST_TRANSITION_READY);
This check for mhi_cntrl->ee == MHI_EE_FP will not work with my upcoming
fixes.
> }
> write_unlock_irq(&mhi_cntrl->pm_lock);
>
> and it also works well, next is the log.
I feel this should be in mhi_fw_load_handler() in function in boot.c
before we return
success and exit the function.
-if (mhi_cntrl->ee == MHI_EE_EDL)
+if (mhi_cntrl->ee == MHI_EE_EDL) {
+ mhi_queue_state_transition(mhi_cntrl, DEV_ST_TRANSITION_READY);
return;
+}
I can submit a patch for this as well so you can wait and have it picked
before
your current patch. There is no need to include the check for ee as
MHI_EE_FP, as it
should be implied.
So, basically the next power up with EDL will push EDL image, then wait
for READY and
after an M0 is done to the device, it will trigger the EE change to FP
on ctrl event
ring.
> [ 1048.424679] mhi 0000:03:00.0: Exiting with PM state: DISABLE, MHI
> state: RESET
> [ 1048.424743] mhi 0000:03:00.0: Requested to power ON
> [ 1048.424885] mhi 0000:03:00.0: Power on setup success
> [ 1048.424887] mhi 0000:03:00.0: Handling state transition: PBL
> [ 1048.425034] mhi 0000:03:00.0: Starting SBL download via BHI.
> Session ID:584611751
> [ 1048.446610] mhi 0000:03:00.0: local ee:EDL device ee:EDL
> dev_state:RESET
> [ 1049.469896] mhi 0000:03:00.0: local ee:FLASH PROGRAMMER device
> ee:EDL dev_state:READY
> [ 1049.469919] mhi 0000:03:00.0: Handling state transition: READY
> [ 1049.469930] mhi 0000:03:00.0: Device in READY State
> [ 1049.469933] mhi 0000:03:00.0: Initializing MHI registers
> [ 1049.473818] mhi 0000:03:00.0: State change event to state: M0
> [ 1049.474850] mhi 0000:03:00.0: Received EE event: FLASH PROGRAMMER
> [ 1049.474888] mhi 0000:03:00.0: Handling state transition:
> FLASH_PROGRAMMER
> [ 1050.402805] mhi 0000:03:00.0: Preparing channel: 34
> [ 1050.406175] mhi 0000:03:00.0: Chan: 34 successfully moved to start
> state
>
>> >> > diff --git a/include/linux/mhi.h b/include/linux/mhi.h index
>> >> > 6e1122c..4620af8 100644
>> >> > --- a/include/linux/mhi.h
>> >> > +++ b/include/linux/mhi.h
>> >> > @@ -120,6 +120,7 @@ struct mhi_link_info {
>> >> > * @MHI_EE_WFW: WLAN firmware mode
>> >> > * @MHI_EE_PTHRU: Passthrough
>> >> > * @MHI_EE_EDL: Embedded downloader
>> >> > + * @MHI_EE_FP, Flash Programmer Environment
>> >> > */
>> >> > enum mhi_ee_type {
>> >> > MHI_EE_PBL,
>> >> > @@ -129,7 +130,8 @@ enum mhi_ee_type {
>> >> > MHI_EE_WFW,
>> >> > MHI_EE_PTHRU,
>> >> > MHI_EE_EDL,
>> >> > - MHI_EE_MAX_SUPPORTED = MHI_EE_EDL,
>> >> > + MHI_EE_FP,
>> >> > + MHI_EE_MAX_SUPPORTED = MHI_EE_FP,
>> >> > MHI_EE_DISABLE_TRANSITION, /* local EE, not related to mhi spec */
>> >> > MHI_EE_NOT_SUPPORTED,
>> >> > MHI_EE_MAX,
>> >>
>> >> Thanks,
>> >> Bhaumik
>> >> --
>> >> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
>> >> Forum, a Linux Foundation Collaborative Project
>>
>> Thanks,
>> Bhaumik
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
>> Forum, a
>> Linux Foundation Collaborative Project
Thanks,
Bhaumik
---
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] bus: mhi: core: Add support MHI EE FP for download firmware
2020-11-02 16:34 ` Bhaumik Bhatt
@ 2020-11-16 6:08 ` Manivannan Sadhasivam
0 siblings, 0 replies; 7+ messages in thread
From: Manivannan Sadhasivam @ 2020-11-16 6:08 UTC (permalink / raw)
To: Bhaumik Bhatt
Cc: carl.yin, hemantk, sfr, linux-arm-msm, linux-kernel, naveen.kumar
On Mon, Nov 02, 2020 at 08:34:14AM -0800, Bhaumik Bhatt wrote:
> On 2020-11-02 04:27, carl.yin@quectel.com wrote:
> > From: "carl.yin" <carl.yin@quectel.com>
> >
> > MHI wwan modems support download firmware to nand or emmc
> > by firehose protocol, process as next:
> > 1. modem boot up and enter EE AMSS, create DIAG channels (4, 5) device
> > 2. user space tool send EDL command via DIAG channel,
> > then modem enter EE EDL
> > 3. boot.c download 'flash programmer image' via BHI interface
> > 4. modem enter EE FP, and create EDL channels (34, 35) device
> > 5. user space tool download 'firmware image' to modem via EDL channels
> > by firehose protocol
> >
> > Signed-off-by: carl.yin <carl.yin@quectel.com>
> > ---
> > drivers/bus/mhi/core/init.c | 2 ++
> > drivers/bus/mhi/core/internal.h | 1 +
> > drivers/bus/mhi/core/main.c | 5 ++++-
> > drivers/bus/mhi/core/pm.c | 13 ++++++++++++-
> > include/linux/mhi.h | 4 +++-
> > 5 files changed, 22 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
> > index ac4aa5c..e34616b 100644
> > --- a/drivers/bus/mhi/core/init.c
> > +++ b/drivers/bus/mhi/core/init.c
> > @@ -26,6 +26,7 @@ const char * const mhi_ee_str[MHI_EE_MAX] = {
> > [MHI_EE_WFW] = "WFW",
> > [MHI_EE_PTHRU] = "PASS THRU",
> > [MHI_EE_EDL] = "EDL",
> > + [MHI_EE_FP] = "FLASH PROGRAMMER",
> > [MHI_EE_DISABLE_TRANSITION] = "DISABLE",
> > [MHI_EE_NOT_SUPPORTED] = "NOT SUPPORTED",
> > };
> > @@ -35,6 +36,7 @@ const char * const
> > dev_state_tran_str[DEV_ST_TRANSITION_MAX] = {
> > [DEV_ST_TRANSITION_READY] = "READY",
> > [DEV_ST_TRANSITION_SBL] = "SBL",
> > [DEV_ST_TRANSITION_MISSION_MODE] = "MISSION_MODE",
> > + [DEV_ST_TRANSITION_FP] = "FLASH_PROGRAMMER",
> > [DEV_ST_TRANSITION_SYS_ERR] = "SYS_ERR",
> > [DEV_ST_TRANSITION_DISABLE] = "DISABLE",
> > };
> > diff --git a/drivers/bus/mhi/core/internal.h
> > b/drivers/bus/mhi/core/internal.h
> > index 4abf0cf..6ae897a 100644
> > --- a/drivers/bus/mhi/core/internal.h
> > +++ b/drivers/bus/mhi/core/internal.h
> > @@ -386,6 +386,7 @@ enum dev_st_transition {
> > DEV_ST_TRANSITION_READY,
> > DEV_ST_TRANSITION_SBL,
> > DEV_ST_TRANSITION_MISSION_MODE,
> > + DEV_ST_TRANSITION_FP,
> > DEV_ST_TRANSITION_SYS_ERR,
> > DEV_ST_TRANSITION_DISABLE,
> > DEV_ST_TRANSITION_MAX,
> > diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
> > index 3950792..a1e1561 100644
> > --- a/drivers/bus/mhi/core/main.c
> > +++ b/drivers/bus/mhi/core/main.c
> > @@ -422,7 +422,7 @@ irqreturn_t mhi_intvec_threaded_handler(int
> > irq_number, void *priv)
> > wake_up_all(&mhi_cntrl->state_event);
> >
> > /* For fatal errors, we let controller decide next step */
> > - if (MHI_IN_PBL(ee))
> > + if (MHI_IN_PBL(mhi_cntrl->ee))
> Let's please have this as a separate patch with a fixes tag, as it fixes a
> pre-existing bug. I am sure Mani would want this.
Yes. It is not recommended to club changes like this onto a single patch.
Thanks,
Mani
> > mhi_cntrl->status_cb(mhi_cntrl, MHI_CB_FATAL_ERROR);
> > else
> > mhi_pm_sys_err_handler(mhi_cntrl);
> > @@ -782,6 +782,9 @@ int mhi_process_ctrl_ev_ring(struct mhi_controller
> > *mhi_cntrl,
> > case MHI_EE_SBL:
> > st = DEV_ST_TRANSITION_SBL;
> > break;
> > + case MHI_EE_FP:
> > + st = DEV_ST_TRANSITION_FP;
> > + break;
> When do you get this EE event on the control event ring? Does it come by
> after you
> have detected EE as FP from mhi_sync_power_up() and move to ready and then
> M0?
> > case MHI_EE_WFW:
> > case MHI_EE_AMSS:
> > st = DEV_ST_TRANSITION_MISSION_MODE;
> > diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
> > index 3de7b16..2d68812 100644
> > --- a/drivers/bus/mhi/core/pm.c
> > +++ b/drivers/bus/mhi/core/pm.c
> > @@ -658,6 +658,12 @@ void mhi_pm_st_worker(struct work_struct *work)
> > case DEV_ST_TRANSITION_MISSION_MODE:
> > mhi_pm_mission_mode_transition(mhi_cntrl);
> > break;
> > + case DEV_ST_TRANSITION_FP:
> > + write_lock_irq(&mhi_cntrl->pm_lock);
> > + mhi_cntrl->ee = MHI_EE_FP;
> > + write_unlock_irq(&mhi_cntrl->pm_lock);
> > + mhi_create_devices(mhi_cntrl);
> > + break;
> > case DEV_ST_TRANSITION_READY:
> > mhi_ready_state_transition(mhi_cntrl);
> > break;
> > @@ -1077,10 +1083,15 @@ int mhi_sync_power_up(struct mhi_controller
> > *mhi_cntrl)
> >
> > wait_event_timeout(mhi_cntrl->state_event,
> > MHI_IN_MISSION_MODE(mhi_cntrl->ee) ||
> > + mhi_cntrl->ee == MHI_EE_FP ||
> > MHI_PM_IN_ERROR_STATE(mhi_cntrl->pm_state),
> > msecs_to_jiffies(mhi_cntrl->timeout_ms));
> >
> > - ret = (MHI_IN_MISSION_MODE(mhi_cntrl->ee)) ? 0 : -ETIMEDOUT;
> > + if (mhi_cntrl->ee == MHI_EE_FP)
> > + mhi_queue_state_transition(mhi_cntrl, DEV_ST_TRANSITION_READY);
> > + else
> > + ret = (MHI_IN_MISSION_MODE(mhi_cntrl->ee)) ? 0 : -ETIMEDOUT;
> > +
> > if (ret)
> > mhi_power_down(mhi_cntrl, false);
> >
> We should come up with a better design for this later on.
> > diff --git a/include/linux/mhi.h b/include/linux/mhi.h
> > index 6e1122c..4620af8 100644
> > --- a/include/linux/mhi.h
> > +++ b/include/linux/mhi.h
> > @@ -120,6 +120,7 @@ struct mhi_link_info {
> > * @MHI_EE_WFW: WLAN firmware mode
> > * @MHI_EE_PTHRU: Passthrough
> > * @MHI_EE_EDL: Embedded downloader
> > + * @MHI_EE_FP, Flash Programmer Environment
> > */
> > enum mhi_ee_type {
> > MHI_EE_PBL,
> > @@ -129,7 +130,8 @@ enum mhi_ee_type {
> > MHI_EE_WFW,
> > MHI_EE_PTHRU,
> > MHI_EE_EDL,
> > - MHI_EE_MAX_SUPPORTED = MHI_EE_EDL,
> > + MHI_EE_FP,
> > + MHI_EE_MAX_SUPPORTED = MHI_EE_FP,
> > MHI_EE_DISABLE_TRANSITION, /* local EE, not related to mhi spec */
> > MHI_EE_NOT_SUPPORTED,
> > MHI_EE_MAX,
>
> Thanks,
> Bhaumik
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] bus: mhi: core: Add support MHI EE FP for download firmware
2020-11-02 12:27 carl.yin
@ 2020-11-02 16:34 ` Bhaumik Bhatt
2020-11-16 6:08 ` Manivannan Sadhasivam
0 siblings, 1 reply; 7+ messages in thread
From: Bhaumik Bhatt @ 2020-11-02 16:34 UTC (permalink / raw)
To: carl.yin
Cc: manivannan.sadhasivam, hemantk, sfr, linux-arm-msm, linux-kernel,
naveen.kumar
On 2020-11-02 04:27, carl.yin@quectel.com wrote:
> From: "carl.yin" <carl.yin@quectel.com>
>
> MHI wwan modems support download firmware to nand or emmc
> by firehose protocol, process as next:
> 1. modem boot up and enter EE AMSS, create DIAG channels (4, 5) device
> 2. user space tool send EDL command via DIAG channel,
> then modem enter EE EDL
> 3. boot.c download 'flash programmer image' via BHI interface
> 4. modem enter EE FP, and create EDL channels (34, 35) device
> 5. user space tool download 'firmware image' to modem via EDL channels
> by firehose protocol
>
> Signed-off-by: carl.yin <carl.yin@quectel.com>
> ---
> drivers/bus/mhi/core/init.c | 2 ++
> drivers/bus/mhi/core/internal.h | 1 +
> drivers/bus/mhi/core/main.c | 5 ++++-
> drivers/bus/mhi/core/pm.c | 13 ++++++++++++-
> include/linux/mhi.h | 4 +++-
> 5 files changed, 22 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
> index ac4aa5c..e34616b 100644
> --- a/drivers/bus/mhi/core/init.c
> +++ b/drivers/bus/mhi/core/init.c
> @@ -26,6 +26,7 @@ const char * const mhi_ee_str[MHI_EE_MAX] = {
> [MHI_EE_WFW] = "WFW",
> [MHI_EE_PTHRU] = "PASS THRU",
> [MHI_EE_EDL] = "EDL",
> + [MHI_EE_FP] = "FLASH PROGRAMMER",
> [MHI_EE_DISABLE_TRANSITION] = "DISABLE",
> [MHI_EE_NOT_SUPPORTED] = "NOT SUPPORTED",
> };
> @@ -35,6 +36,7 @@ const char * const
> dev_state_tran_str[DEV_ST_TRANSITION_MAX] = {
> [DEV_ST_TRANSITION_READY] = "READY",
> [DEV_ST_TRANSITION_SBL] = "SBL",
> [DEV_ST_TRANSITION_MISSION_MODE] = "MISSION_MODE",
> + [DEV_ST_TRANSITION_FP] = "FLASH_PROGRAMMER",
> [DEV_ST_TRANSITION_SYS_ERR] = "SYS_ERR",
> [DEV_ST_TRANSITION_DISABLE] = "DISABLE",
> };
> diff --git a/drivers/bus/mhi/core/internal.h
> b/drivers/bus/mhi/core/internal.h
> index 4abf0cf..6ae897a 100644
> --- a/drivers/bus/mhi/core/internal.h
> +++ b/drivers/bus/mhi/core/internal.h
> @@ -386,6 +386,7 @@ enum dev_st_transition {
> DEV_ST_TRANSITION_READY,
> DEV_ST_TRANSITION_SBL,
> DEV_ST_TRANSITION_MISSION_MODE,
> + DEV_ST_TRANSITION_FP,
> DEV_ST_TRANSITION_SYS_ERR,
> DEV_ST_TRANSITION_DISABLE,
> DEV_ST_TRANSITION_MAX,
> diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
> index 3950792..a1e1561 100644
> --- a/drivers/bus/mhi/core/main.c
> +++ b/drivers/bus/mhi/core/main.c
> @@ -422,7 +422,7 @@ irqreturn_t mhi_intvec_threaded_handler(int
> irq_number, void *priv)
> wake_up_all(&mhi_cntrl->state_event);
>
> /* For fatal errors, we let controller decide next step */
> - if (MHI_IN_PBL(ee))
> + if (MHI_IN_PBL(mhi_cntrl->ee))
Let's please have this as a separate patch with a fixes tag, as it fixes
a
pre-existing bug. I am sure Mani would want this.
> mhi_cntrl->status_cb(mhi_cntrl, MHI_CB_FATAL_ERROR);
> else
> mhi_pm_sys_err_handler(mhi_cntrl);
> @@ -782,6 +782,9 @@ int mhi_process_ctrl_ev_ring(struct mhi_controller
> *mhi_cntrl,
> case MHI_EE_SBL:
> st = DEV_ST_TRANSITION_SBL;
> break;
> + case MHI_EE_FP:
> + st = DEV_ST_TRANSITION_FP;
> + break;
When do you get this EE event on the control event ring? Does it come by
after you
have detected EE as FP from mhi_sync_power_up() and move to ready and
then M0?
> case MHI_EE_WFW:
> case MHI_EE_AMSS:
> st = DEV_ST_TRANSITION_MISSION_MODE;
> diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
> index 3de7b16..2d68812 100644
> --- a/drivers/bus/mhi/core/pm.c
> +++ b/drivers/bus/mhi/core/pm.c
> @@ -658,6 +658,12 @@ void mhi_pm_st_worker(struct work_struct *work)
> case DEV_ST_TRANSITION_MISSION_MODE:
> mhi_pm_mission_mode_transition(mhi_cntrl);
> break;
> + case DEV_ST_TRANSITION_FP:
> + write_lock_irq(&mhi_cntrl->pm_lock);
> + mhi_cntrl->ee = MHI_EE_FP;
> + write_unlock_irq(&mhi_cntrl->pm_lock);
> + mhi_create_devices(mhi_cntrl);
> + break;
> case DEV_ST_TRANSITION_READY:
> mhi_ready_state_transition(mhi_cntrl);
> break;
> @@ -1077,10 +1083,15 @@ int mhi_sync_power_up(struct mhi_controller
> *mhi_cntrl)
>
> wait_event_timeout(mhi_cntrl->state_event,
> MHI_IN_MISSION_MODE(mhi_cntrl->ee) ||
> + mhi_cntrl->ee == MHI_EE_FP ||
> MHI_PM_IN_ERROR_STATE(mhi_cntrl->pm_state),
> msecs_to_jiffies(mhi_cntrl->timeout_ms));
>
> - ret = (MHI_IN_MISSION_MODE(mhi_cntrl->ee)) ? 0 : -ETIMEDOUT;
> + if (mhi_cntrl->ee == MHI_EE_FP)
> + mhi_queue_state_transition(mhi_cntrl, DEV_ST_TRANSITION_READY);
> + else
> + ret = (MHI_IN_MISSION_MODE(mhi_cntrl->ee)) ? 0 : -ETIMEDOUT;
> +
> if (ret)
> mhi_power_down(mhi_cntrl, false);
>
We should come up with a better design for this later on.
> diff --git a/include/linux/mhi.h b/include/linux/mhi.h
> index 6e1122c..4620af8 100644
> --- a/include/linux/mhi.h
> +++ b/include/linux/mhi.h
> @@ -120,6 +120,7 @@ struct mhi_link_info {
> * @MHI_EE_WFW: WLAN firmware mode
> * @MHI_EE_PTHRU: Passthrough
> * @MHI_EE_EDL: Embedded downloader
> + * @MHI_EE_FP, Flash Programmer Environment
> */
> enum mhi_ee_type {
> MHI_EE_PBL,
> @@ -129,7 +130,8 @@ enum mhi_ee_type {
> MHI_EE_WFW,
> MHI_EE_PTHRU,
> MHI_EE_EDL,
> - MHI_EE_MAX_SUPPORTED = MHI_EE_EDL,
> + MHI_EE_FP,
> + MHI_EE_MAX_SUPPORTED = MHI_EE_FP,
> MHI_EE_DISABLE_TRANSITION, /* local EE, not related to mhi spec */
> MHI_EE_NOT_SUPPORTED,
> MHI_EE_MAX,
Thanks,
Bhaumik
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2] bus: mhi: core: Add support MHI EE FP for download firmware
@ 2020-11-02 12:27 carl.yin
2020-11-02 16:34 ` Bhaumik Bhatt
0 siblings, 1 reply; 7+ messages in thread
From: carl.yin @ 2020-11-02 12:27 UTC (permalink / raw)
To: manivannan.sadhasivam, hemantk, sfr
Cc: linux-arm-msm, linux-kernel, carl.yin, naveen.kumar
From: "carl.yin" <carl.yin@quectel.com>
MHI wwan modems support download firmware to nand or emmc
by firehose protocol, process as next:
1. modem boot up and enter EE AMSS, create DIAG channels (4, 5) device
2. user space tool send EDL command via DIAG channel,
then modem enter EE EDL
3. boot.c download 'flash programmer image' via BHI interface
4. modem enter EE FP, and create EDL channels (34, 35) device
5. user space tool download 'firmware image' to modem via EDL channels
by firehose protocol
Signed-off-by: carl.yin <carl.yin@quectel.com>
---
drivers/bus/mhi/core/init.c | 2 ++
drivers/bus/mhi/core/internal.h | 1 +
drivers/bus/mhi/core/main.c | 5 ++++-
drivers/bus/mhi/core/pm.c | 13 ++++++++++++-
include/linux/mhi.h | 4 +++-
5 files changed, 22 insertions(+), 3 deletions(-)
diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
index ac4aa5c..e34616b 100644
--- a/drivers/bus/mhi/core/init.c
+++ b/drivers/bus/mhi/core/init.c
@@ -26,6 +26,7 @@ const char * const mhi_ee_str[MHI_EE_MAX] = {
[MHI_EE_WFW] = "WFW",
[MHI_EE_PTHRU] = "PASS THRU",
[MHI_EE_EDL] = "EDL",
+ [MHI_EE_FP] = "FLASH PROGRAMMER",
[MHI_EE_DISABLE_TRANSITION] = "DISABLE",
[MHI_EE_NOT_SUPPORTED] = "NOT SUPPORTED",
};
@@ -35,6 +36,7 @@ const char * const dev_state_tran_str[DEV_ST_TRANSITION_MAX] = {
[DEV_ST_TRANSITION_READY] = "READY",
[DEV_ST_TRANSITION_SBL] = "SBL",
[DEV_ST_TRANSITION_MISSION_MODE] = "MISSION_MODE",
+ [DEV_ST_TRANSITION_FP] = "FLASH_PROGRAMMER",
[DEV_ST_TRANSITION_SYS_ERR] = "SYS_ERR",
[DEV_ST_TRANSITION_DISABLE] = "DISABLE",
};
diff --git a/drivers/bus/mhi/core/internal.h b/drivers/bus/mhi/core/internal.h
index 4abf0cf..6ae897a 100644
--- a/drivers/bus/mhi/core/internal.h
+++ b/drivers/bus/mhi/core/internal.h
@@ -386,6 +386,7 @@ enum dev_st_transition {
DEV_ST_TRANSITION_READY,
DEV_ST_TRANSITION_SBL,
DEV_ST_TRANSITION_MISSION_MODE,
+ DEV_ST_TRANSITION_FP,
DEV_ST_TRANSITION_SYS_ERR,
DEV_ST_TRANSITION_DISABLE,
DEV_ST_TRANSITION_MAX,
diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
index 3950792..a1e1561 100644
--- a/drivers/bus/mhi/core/main.c
+++ b/drivers/bus/mhi/core/main.c
@@ -422,7 +422,7 @@ irqreturn_t mhi_intvec_threaded_handler(int irq_number, void *priv)
wake_up_all(&mhi_cntrl->state_event);
/* For fatal errors, we let controller decide next step */
- if (MHI_IN_PBL(ee))
+ if (MHI_IN_PBL(mhi_cntrl->ee))
mhi_cntrl->status_cb(mhi_cntrl, MHI_CB_FATAL_ERROR);
else
mhi_pm_sys_err_handler(mhi_cntrl);
@@ -782,6 +782,9 @@ int mhi_process_ctrl_ev_ring(struct mhi_controller *mhi_cntrl,
case MHI_EE_SBL:
st = DEV_ST_TRANSITION_SBL;
break;
+ case MHI_EE_FP:
+ st = DEV_ST_TRANSITION_FP;
+ break;
case MHI_EE_WFW:
case MHI_EE_AMSS:
st = DEV_ST_TRANSITION_MISSION_MODE;
diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
index 3de7b16..2d68812 100644
--- a/drivers/bus/mhi/core/pm.c
+++ b/drivers/bus/mhi/core/pm.c
@@ -658,6 +658,12 @@ void mhi_pm_st_worker(struct work_struct *work)
case DEV_ST_TRANSITION_MISSION_MODE:
mhi_pm_mission_mode_transition(mhi_cntrl);
break;
+ case DEV_ST_TRANSITION_FP:
+ write_lock_irq(&mhi_cntrl->pm_lock);
+ mhi_cntrl->ee = MHI_EE_FP;
+ write_unlock_irq(&mhi_cntrl->pm_lock);
+ mhi_create_devices(mhi_cntrl);
+ break;
case DEV_ST_TRANSITION_READY:
mhi_ready_state_transition(mhi_cntrl);
break;
@@ -1077,10 +1083,15 @@ int mhi_sync_power_up(struct mhi_controller *mhi_cntrl)
wait_event_timeout(mhi_cntrl->state_event,
MHI_IN_MISSION_MODE(mhi_cntrl->ee) ||
+ mhi_cntrl->ee == MHI_EE_FP ||
MHI_PM_IN_ERROR_STATE(mhi_cntrl->pm_state),
msecs_to_jiffies(mhi_cntrl->timeout_ms));
- ret = (MHI_IN_MISSION_MODE(mhi_cntrl->ee)) ? 0 : -ETIMEDOUT;
+ if (mhi_cntrl->ee == MHI_EE_FP)
+ mhi_queue_state_transition(mhi_cntrl, DEV_ST_TRANSITION_READY);
+ else
+ ret = (MHI_IN_MISSION_MODE(mhi_cntrl->ee)) ? 0 : -ETIMEDOUT;
+
if (ret)
mhi_power_down(mhi_cntrl, false);
diff --git a/include/linux/mhi.h b/include/linux/mhi.h
index 6e1122c..4620af8 100644
--- a/include/linux/mhi.h
+++ b/include/linux/mhi.h
@@ -120,6 +120,7 @@ struct mhi_link_info {
* @MHI_EE_WFW: WLAN firmware mode
* @MHI_EE_PTHRU: Passthrough
* @MHI_EE_EDL: Embedded downloader
+ * @MHI_EE_FP, Flash Programmer Environment
*/
enum mhi_ee_type {
MHI_EE_PBL,
@@ -129,7 +130,8 @@ enum mhi_ee_type {
MHI_EE_WFW,
MHI_EE_PTHRU,
MHI_EE_EDL,
- MHI_EE_MAX_SUPPORTED = MHI_EE_EDL,
+ MHI_EE_FP,
+ MHI_EE_MAX_SUPPORTED = MHI_EE_FP,
MHI_EE_DISABLE_TRANSITION, /* local EE, not related to mhi spec */
MHI_EE_NOT_SUPPORTED,
MHI_EE_MAX,
--
2.25.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-11-17 3:27 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-03 1:47 [PATCH v2] bus: mhi: core: Add support MHI EE FP for download firmware Carl Yin(殷张成)
2020-11-03 17:32 ` Bhaumik Bhatt
2020-11-13 2:48 ` 答复: " Carl Yin(殷张成)
2020-11-17 3:27 ` Bhaumik Bhatt
-- strict thread matches above, loose matches on Subject: below --
2020-11-02 12:27 carl.yin
2020-11-02 16:34 ` Bhaumik Bhatt
2020-11-16 6:08 ` Manivannan Sadhasivam
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.