Re: Sierra Wireless EM9191 integration issues in mhi+wwan

From: Bhaumik Bhatt <bbhatt@codeaurora.org>
To: Loic Poulain <loic.poulain@linaro.org>
Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>,
	Aleksander Morgado <aleksander@aleksander.es>,
	Hemant Kumar <hemantk@codeaurora.org>,
	Manivannan Sadhasivam <mani@kernel.org>,
	Thomas Perrot <thomas.perrot@bootlin.com>,
	Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
	linux-arm-msm <linux-arm-msm@vger.kernel.org>
Subject: Re: Sierra Wireless EM9191 integration issues in mhi+wwan
Date: Thu, 04 Nov 2021 15:50:57 -0700	[thread overview]
Message-ID: <bb0efc0dbc49aec3eaa099ca5b0d0e5a@codeaurora.org> (raw)
In-Reply-To: <CAMZdPi9+zrsDy9WTipamRWBXMOxUX1tfsk2W52b9wG-4q21fWA@mail.gmail.com>

Hi Loic,

On 2021-11-02 01:47 PM, Loic Poulain wrote:
> Hi Mani,
> 
> Le mar. 2 nov. 2021 à 19:09, Manivannan Sadhasivam
> <manivannan.sadhasivam@linaro.org> a écrit :
> 
>> On Tue, Nov 02, 2021 at 05:55:34PM +0100, Aleksander Morgado wrote:
>>>> > > > [    7.189547] mhi mhi0: Transitioning from PM state:
>> Linkdown or
>>>> > > > Error Fatal Detect to: SYS ERROR Process
>>>> > >
>>>> > > Hmm, I think the use of sync_power_up might be causing the
>> issue here as it
>>>> > > forces the MHI state to fatal error.
>>>> > >
>>>> > > Ignore the previous diff and try the below one:
>>>> > >
>>>> > > diff --git a/drivers/bus/mhi/pci_generic.c
>> b/drivers/bus/mhi/pci_generic.c
>>>> > > index 59a4896a8030..b1e8c7de4e54 100644
>>>> > > --- a/drivers/bus/mhi/pci_generic.c
>>>> > > +++ b/drivers/bus/mhi/pci_generic.c
>>>> > > @@ -637,7 +637,7 @@ static void mhi_pci_recovery_work(struct
>> work_struct *work)
>>>> > >         if (err)
>>>> > >                 goto err_try_reset;
>>>> > >
>>>> > > -       err = mhi_sync_power_up(mhi_cntrl);
>>>> > > +       err = mhi_async_power_up(mhi_cntrl);
>>>> 
>>>> Doh! Sorry, I modified the wrong function. Here is the correct
>> one:
>>>> 
>>>> diff --git a/drivers/bus/mhi/pci_generic.c
>> b/drivers/bus/mhi/pci_generic.c
>>>> index 59a4896a8030..1e3c74bfbe34 100644
>>>> --- a/drivers/bus/mhi/pci_generic.c
>>>> +++ b/drivers/bus/mhi/pci_generic.c
>>>> @@ -743,7 +743,7 @@ static int mhi_pci_probe(struct pci_dev
>> *pdev, const struct pci_device_id *id)
>>>>                 goto err_unregister;
>>>>         }
>>>> 
>>>> -       err = mhi_sync_power_up(mhi_cntrl);
>>>> +       err = mhi_async_power_up(mhi_cntrl);
>>>>         if (err) {
>>>>                 dev_err(&pdev->dev, "failed to power up MHI
>> controller\n");
>>>>                 goto err_unprepare;
>>>> 
>>>> Let's see how it goes :)
>>>> 
>>> 
>>> Oh, wow, what a difference a single extra byte in the correct
>> place makes! :D
>>> 
>>> This looks waaaaay better; I've rebooted the board at least 10
>> times
>>> now and all of them worked nicely, at least just the process to
>> probe
>>> the device and get the control and net ports exposed looks very
>>> reliable now. I'll test the setup with ModemManager in the next
>> days
>>> and see how everything goes. Thank you very much!
>>> 
>> 
>> That's great to hear! Let me know how your testing goes with
>> ModemManager. I'll
>> then submit a patch to fix the pci-generic driver.
> 
> I’ve not followed entirely what is going wrong, so I may miss the
> point here, but the sync and async function should behave  the same
> regarding device initialization, wouldn’t make sense to fix the core
> instead? Is the _sync variant broken?
> 
> Regards,
> Loic

Yes both variants should behave the same way. Mani, Hemant and I spoke 
about this
and saw there could be improvements in core mhi_async_power_up().

I believe Mani will come up with a patch soon.

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