Re: [PATCH 1/1] bus: mhi: host: allow SBL as initial EE

[Daniele Palmas <dnlplm@gmail.com>]

Il giorno mer 31 mag 2023 alle ore 15:01 Manivannan Sadhasivam
<mani@kernel.org> ha scritto:
>
> On Wed, May 31, 2023 at 01:44:24PM +0200, Daniele Palmas wrote:
> > Il giorno mar 30 mag 2023 alle ore 15:56 Manivannan Sadhasivam
> > <mani@kernel.org> ha scritto:
> > >
> > > On Tue, May 30, 2023 at 01:12:59PM +0200, Daniele Palmas wrote:
> > > > Hi Mani,
> > > >
> > > > Il giorno mar 30 mag 2023 alle ore 12:31 Manivannan Sadhasivam
> > > > <mani@kernel.org> ha scritto:
> > > > >
> > > > > On Tue, May 30, 2023 at 11:13:40AM +0200, Daniele Palmas wrote:
> > > > > > There are situations in which SBL is a legitimate initial execution
> > > > > > environment (e.g. modem stuck in SBL due to a firmware failure...), but
> > > > > > mhi refuses to start:
> > > > > >
> > > > > > mhi-pci-generic 0000:01:00.0: MHI PCI device found: foxconn-sdx55
> > > > > > mhi-pci-generic 0000:01:00.0: BAR 0: assigned
> > > > > > mhi-pci-generic 0000:01:00.0: enabling device (0000 -> 0002)
> > > > > > mhi mhi0: Requested to power ON
> > > > > > mhi mhi0: SECONDARY BOOTLOADER is not a valid EE for power on
> > > > > > mhi-pci-generic 0000:01:00.0: failed to power up MHI controller
> > > > > > mhi-pci-generic: probe of 0000:01:00.0 failed with error -5
> > > > > >
> > > > > > Fix this by adding SBL as an allowed initial execution environment.
> > > > > >
> > > > >
> > > > > What can you do with the modem when firmware failure happens? If there is a
> > > > > usecase, please explain.
> > > >
> > > > (removing Siddartha and Sujeev due to addresses not working)
> > > >
> > >
> > > Both left Qualcomm a while ago...
> > >
> > > > A possible scenario for a Telit modem not being able to go to mission
> > > > mode is when a firmware update does not work properly: in this case it
> > > > remains stuck in SBL, but the SAHARA device can be used for retrying
> > > > the firmware update.
> > > >
> > >
> > > So IIUC, while updating SBL over SAHARA channel, the firmware update could be
> > > corrupted and the device would get stuck in SBL EE. In that case, the SAHARA
> > > channel exposed by PBL will still be open and that could be used to retry the
> > > firmware update. Am I right?
> > >
> > > Looks like PBL is doing a fail safe upgrade!
> > >
> >
> > Yes, the scenario is a broken firmware update.
> >
> > Telit has implemented custom code in the SBL for allowing the firmware
> > update through a custom protocol using the SAHARA channels: this has
> > been mainly done to avoid sharing with customers Qualcomm IP about
> > firmware update, since it is usually problematic.
> >
> > Firmware update is fail safe in the sense that it is guaranteed that
> > the modem will always power-up at least in SBL, in which the firmware
> > update can always be retried. I don't have the exact details of what
> > is happening inside the modem, but, if required, I can get them.
> >
>
> Okay, I didn't now that it is a custom SBL, so yeah it may not benefit other
> platforms. But what is the userspace tool that you are using to download
> firmware over SAHARA?

The userspace tool is not public at the moment, but will check about
the plans for open-sourcing.

> Can QDL be used?

I will check that with firmware developers.

Reading also Jeffrey's reply, since this looks like a Telit modem
quirk, I'm wondering if it could make sense to deal with this again
once the quirk framework at
https://lore.kernel.org/mhi/20230519163902.4170-2-quic_jhugo@quicinc.com/T/#u
gets merged, otherwise I guess we can continue living with custom
patches.

Thanks,
Daniele

> - Mani
>
> > > > Telit FN990 supports the SAHARA channels in pci_generic. It's true
> > > > that there's still missing the exposed device for userspace, something
> > > > that we are currently managing with out of tree patches, but I see
> > > > that there's some ongoing effort for that
> > > > https://lore.kernel.org/mhi/20230522190459.13790-1-quic_jhugo@quicinc.com/
> > > >
> > > > I'm not sure if non-Telit modems have other reasonable use-cases.
> > > >
> > >
> > > If my above understanding is correct, then this patch will benefit other
> > > platforms too.
> > >
> >
> > Well... I can't comment on that, since I don't know if the above
> > described behavior is due to Telit custom SBL.
> >
> > Anyway, maybe there could be other scenarios like, for example, modem
> > falling into core dump mode in a very early stage? Not sure...
> >
> > Regards,
> > Daniele
> >
> > > - Mani
> > >
> > > > Regards,
> > > > Daniele
> > > >
> > > > >
> > > > > - Mani
> > > > >
> > > > > > Fixes: 3000f85b8f47 ("bus: mhi: core: Add support for basic PM operations")
> > > > > > Signed-off-by: Daniele Palmas <dnlplm@gmail.com>
> > > > > > ---
> > > > > >  drivers/bus/mhi/host/internal.h | 2 +-
> > > > > >  drivers/bus/mhi/host/pm.c       | 3 ++-
> > > > > >  2 files changed, 3 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/bus/mhi/host/internal.h b/drivers/bus/mhi/host/internal.h
> > > > > > index 2e139e76de4c..3bdcd2321aa5 100644
> > > > > > --- a/drivers/bus/mhi/host/internal.h
> > > > > > +++ b/drivers/bus/mhi/host/internal.h
> > > > > > @@ -56,7 +56,7 @@ extern const char * const mhi_ee_str[MHI_EE_MAX];
> > > > > >
> > > > > >  #define MHI_IN_PBL(ee) (ee == MHI_EE_PBL || ee == MHI_EE_PTHRU || \
> > > > > >                       ee == MHI_EE_EDL)
> > > > > > -#define MHI_POWER_UP_CAPABLE(ee) (MHI_IN_PBL(ee) || ee == MHI_EE_AMSS)
> > > > > > +#define MHI_POWER_UP_CAPABLE(ee) (MHI_IN_PBL(ee) || ee == MHI_EE_AMSS || ee == MHI_EE_SBL)
> > > > > >  #define MHI_FW_LOAD_CAPABLE(ee) (ee == MHI_EE_PBL || ee == MHI_EE_EDL)
> > > > > >  #define MHI_IN_MISSION_MODE(ee) (ee == MHI_EE_AMSS || ee == MHI_EE_WFW || \
> > > > > >                                ee == MHI_EE_FP)
> > > > > > diff --git a/drivers/bus/mhi/host/pm.c b/drivers/bus/mhi/host/pm.c
> > > > > > index 083459028a4b..18872c5017be 100644
> > > > > > --- a/drivers/bus/mhi/host/pm.c
> > > > > > +++ b/drivers/bus/mhi/host/pm.c
> > > > > > @@ -1203,10 +1203,11 @@ 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_SBL ||
> > > > > >                          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;
> > > > > > +     ret = (MHI_IN_MISSION_MODE(mhi_cntrl->ee) || mhi_cntrl->ee == MHI_EE_SBL) ? 0 : -ETIMEDOUT;
> > > > > >       if (ret)
> > > > > >               mhi_power_down(mhi_cntrl, false);
> > > > > >
> > > > > > --
> > > > > > 2.37.1
> > > > > >
> > > > >
> > > > > --
> > > > > மணிவண்ணன் சதாசிவம்
> > >
> > > --
> > > மணிவண்ணன் சதாசிவம்
>
> --
> மணிவண்ணன் சதாசிவம்