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;
Sender: bbhatt=codeaurora.org@mg.codeaurora.org
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8;
 format=flowed
Content-Transfer-Encoding: 8bit
Date:   Thu, 29 Oct 2020 11:17:15 -0700
From:   Bhaumik Bhatt <bbhatt@codeaurora.org>
To:     =?UTF-8?Q?Carl_Yin=28=E6=AE=B7=E5=BC=A0=E6=88=90=29?= 
        <carl.yin@quectel.com>
Cc:     Hemant Kumar <hemantk@codeaurora.org>,
        Jeffrey Hugo <jhugo@codeaurora.org>,
        manivannan.sadhasivam@linaro.org, sfr@canb.auug.org.au,
        linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org,
        Naveen Kumar <naveen.kumar@quectel.com>
Subject: Re: [PATCH] bus: mhi: core: Add support MHI EE FP for download
 firmware
Organization: Qualcomm Innovation Center, Inc.
Reply-To: bbhatt@codeaurora.org
Mail-Reply-To: bbhatt@codeaurora.org
In-Reply-To: <HK2PR06MB35078506D624FD03D699457E86140@HK2PR06MB3507.apcprd06.prod.outlook.com>
References: <HK2PR06MB35078506D624FD03D699457E86140@HK2PR06MB3507.apcprd06.prod.outlook.com>
Message-ID: <c3ad89980d37b29ce86f8be185e49ec6@codeaurora.org>
User-Agent: Roundcube Webmail/1.3.9
Precedence: bulk

On 2020-10-29 02:26, Carl Yin wrote:
> On October 28, 2020 7:02 AM, hemantk wrote:
>> Hi Jeff,
>> 
>> On 10/27/20 8:11 AM, Jeffrey Hugo wrote:
>> > On 10/27/2020 3:43 AM, 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. wwan modem normal bootup and enter EE AMSS, create mhi DIAG chan
>> >> device 2. send EDL cmd via DIAG chan, then modem enter EE EDL 3.
>> >> boot.c download 'firehose/prog_firehose_sdx55.mbn' via BHI interface
>> >> 4. modem enter EE FP, and create mhi EDL chan device 5. user space
>> >> tool download FW to modem via EDL chan by firehose protocol
>> >>
>> >> Signed-off-by: carl.yin <carl.yin@quectel.com>
>> >> ---
>> >>   drivers/bus/mhi/core/boot.c     |  4 +++-
>> >>   drivers/bus/mhi/core/init.c     |  2 ++
>> >>   drivers/bus/mhi/core/internal.h |  1 +
>> >>   drivers/bus/mhi/core/main.c     |  3 +++
>> >>   drivers/bus/mhi/core/pm.c       | 16 +++++++++++++++-
>> >>   include/linux/mhi.h             |  4 +++-
>> >>   6 files changed, 27 insertions(+), 3 deletions(-)
>> >>
>> >> diff --git a/drivers/bus/mhi/core/boot.c
>> >> b/drivers/bus/mhi/core/boot.c index 24422f5..ab39ad6 100644
>> >> --- a/drivers/bus/mhi/core/boot.c
>> >> +++ b/drivers/bus/mhi/core/boot.c
>> >> @@ -460,8 +460,10 @@ void mhi_fw_load_handler(struct mhi_controller
>> >> *mhi_cntrl)
>> >>           return;
>> >>       }
>> >> -    if (mhi_cntrl->ee == MHI_EE_EDL)
>> >> +    if (mhi_cntrl->ee == MHI_EE_EDL) {
>> >> +        mhi_ready_state_transition(mhi_cntrl);
>> >>           return;
>> >> +    }
>> >>       write_lock_irq(&mhi_cntrl->pm_lock);
>> >>       mhi_cntrl->dev_state = MHI_STATE_RESET; diff --git
>> >> a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c index
>> >> ac4aa5c..9c2c2f3 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] = "FP",
>> >>       [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] = "FP",
>> >>       [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..e307b58 100644
>> >> --- a/drivers/bus/mhi/core/main.c
>> >> +++ b/drivers/bus/mhi/core/main.c
>> >> @@ -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..3c95a5d 100644
>> >> --- a/drivers/bus/mhi/core/pm.c
>> >> +++ b/drivers/bus/mhi/core/pm.c
>> >> @@ -563,7 +563,15 @@ static void mhi_pm_disable_transition(struct
>> >> mhi_controller *mhi_cntrl,
>> >>       }
>> >>       if (cur_state == MHI_PM_SYS_ERR_PROCESS) {
>> >> -        mhi_ready_state_transition(mhi_cntrl);
>> >> +        if (mhi_get_exec_env(mhi_cntrl) == MHI_EE_EDL
>> >> +            && mhi_get_mhi_state(mhi_cntrl) == MHI_STATE_RESET) {
>> >> +            write_lock_irq(&mhi_cntrl->pm_lock);
>> >> +            cur_state = mhi_tryset_pm_state(mhi_cntrl,
>> MHI_PM_POR);
>> >> +            write_unlock_irq(&mhi_cntrl->pm_lock);
>> >> +            mhi_queue_state_transition(mhi_cntrl,
>> >> DEV_ST_TRANSITION_PBL);
>> >> +        } else {
>> >> +            mhi_ready_state_transition(mhi_cntrl);
>> >> +        }
>> >>       } else {
>> >>           /* Move to disable state */
>> >>           write_lock_irq(&mhi_cntrl->pm_lock);
>> >> @@ -658,6 +666,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;
>> >> 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,
>> >>
>> >
>> > This gets a NACK from me.  I don't see the FP_EE that this patch
>> > introduces defined in the spec.  Where did it come from?
>> >
>> There is indeed a FP EE, BHI spec will be updated with this EE next 
>> month.
>> 
>> Basically, once device goes to EDL, flash programmer image is 
>> downloaded using
>> BHI protocol (same as we download SBL image using BHI from PBL in 
>> current use
>> case). Once it is downloaded intvec sends EE change event for FP. Also 
>> event is
>> generated for the same which is used to create EDL channels (34, 35) 
>> which is
>> used by flash programmer to flash image for AMSS.
>> 
>>  >> 2. send EDL cmd via DIAG chan, then modem enter EE EDL
>> #2 needs to be done in cleaner way. From AMSS when diag cmd is sent to 
>> switch
>> to EDL, device would send SYS_ERR which we can use to do a call back 
>> to mhi
>> controller to perform power down and power up. Instead of moving pm 
>> state to
>> POR from disable transition :-
> [carl.yin] why I need move pm_state to POR, it is because if pm_state
> is SYS_ERR,
> 	mhi_fw_load_handler() will break, and will not download edl image to 
> device.
> 	We can change pm.c as next, and revert the change to boot.c
>      if (cur_state == MHI_PM_SYS_ERR_PROCESS) {
> 		if (mhi_get_exec_env(mhi_cntrl) == MHI_EE_EDL
>  			&& mhi_get_mhi_state(mhi_cntrl) == MHI_STATE_RESET) {
>               write_lock_irq(&mhi_cntrl->pm_lock);
>               mhi_cntrl->ee = MHI_EE_EDL;
>               write_unlock_irq(&mhi_cntrl->pm_lock);
>               mhi_fw_load_handler(mhi_cntrl);
>           }
>           mhi_ready_state_transition(mhi_cntrl);
>       }
With the explanation from my previous email, you don't need this any 
more.
>> 
>> @@ -563,7 +563,15 @@ static void mhi_pm_disable_transition(struct  >>
>> mhi_controller *mhi_cntrl,
>>  >>       }
>>  >>       if (cur_state == MHI_PM_SYS_ERR_PROCESS) {
>>  >> -        mhi_ready_state_transition(mhi_cntrl);
>>  >> +        if (mhi_get_exec_env(mhi_cntrl) == MHI_EE_EDL
>>  >> +            && mhi_get_mhi_state(mhi_cntrl) == MHI_STATE_RESET) {
>>  >> +            write_lock_irq(&mhi_cntrl->pm_lock);
>>  >> +            cur_state = mhi_tryset_pm_state(mhi_cntrl, 
>> MHI_PM_POR);
>>  >> +            write_unlock_irq(&mhi_cntrl->pm_lock);
>>  >> +            mhi_queue_state_transition(mhi_cntrl,
>>  >> DEV_ST_TRANSITION_PBL);
>>  >> +        } else {
>>  >> +            mhi_ready_state_transition(mhi_cntrl);
>>  >> +        }
>> 
>> Thanks,
>> Hemant
>> 
>> --
>> 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