Hi Hemant, Bhaumik, On Tue, 6 Apr 2021 at 05:54, Hemant Kumar <hemantk@xxxxxxxxxxxxxx> wrote: > > Hi Loic, > > On 4/5/21 11:46 AM, Bhaumik Bhatt wrote: > > On 2021-03-31 11:27 AM, Manivannan Sadhasivam wrote: > >> On Wed, Mar 31, 2021 at 07:38:55PM +0200, Loic Poulain wrote: > >>> Hi Mani, > >>> > >>> Le mer. 31 mars 2021 à 19:12, Manivannan Sadhasivam < > >>> manivannan.sadhasivam@xxxxxxxxxx> a écrit : > >>> > >>> > On Fri, Mar 05, 2021 at 06:02:23PM +0100, Loic Poulain wrote: > >>> > > This change ensures that PM reference is always get during packet > >>> > > queueing and released either after queuing completion (RX) or once > >>> > > the buffer has been consumed (TX). This guarantees proper update for > >>> > > underlying MHI controller runtime status (e.g. last_busy timestamp) > >>> > > and prevents suspend to be triggered while TX packets are flying, > >>> > > or before we completed update of the RX ring. > >>> > > > >>> > > >>> > Any reason why you didn't wait for RX completion also? > >>> > >>> > >>> Because on TX we know the buffer completion is going to happen really > >>> quickly (we send data) whereas we never know when when RX packet will be > >>> completed (we are waiting for data), so we want to be able to put the > >>> MHI > >>> device in suspend while RX is pending (the device will wake up the > >>> host on > >>> incoming data) > >>> > >> > >> Device wakeup will only happen for device initiated suspend (M1) but for > >> host initiated suspend (M3), device will check for pending data to host > >> and will initiate wakeup request before going for suspend. So I think it > >> is safe to wait for RX data. > >> > >> Hemant/Bhaumik, any thoughts? > >> > >> Thanks, > >> Mani > >> > > Agree with Loic here. Let's not depend on the device to determine host side > > behavior and instead, assume that the device may or may not be following > > protocol so as to reduce chances of higher power draw by host. Host should > > not care when RX comes, but host should care about TX completion as that's > > where our requirement ends. > > > > There have been instances of delayed RX and in some cases, no TX completion > > from a certain client (I think DIAG), where device thinks they have > > received > > garbage and decide not to respond with a TX completion. > > > > We want to be able to put device in suspend or at least initiate it while > > host waits for incoming data. Once RX comes in, host will wake up to > > process it. > Agree with Bhaumik and Loic about not waiting for Rx data. > > > > What Loic does in this patch is done in one way using patch [1]. > > However, that > > does not update the last_busy timestamp. I am mostly in favor of this patch > > going in but would like Loic to answer one question: > > > > In mhi_reset_data_chan(), why not do a runtime_put(mhi_cntrl) inside the > > pre-existing if (mhi_chan->dir == DMA_TO_DEVICE) at the start of the > > while loop: > > while (tre_ring->rp != tre_ring->wp)? This would be balanced for each TX. > I got same question when i looked at the patch. Very true, I've not seen any issue, because there is 'usually' not TX pending on reset/remove but we indeed need to take care of balancing that here as well. I'm going to follow up with a new patch. Thanks, Loic
