On Fri, May 23, 2014 at 03:10:08PM +0200, Rafael J. Wysocki wrote: > On Friday, May 23, 2014 03:30:53 PM Heikki Krogerus wrote: > > On Thu, May 22, 2014 at 01:28:16AM +0200, Rafael J. Wysocki wrote: > > > On Wednesday, May 21, 2014 01:52:58 PM Heikki Krogerus wrote: > > > > On Wed, May 21, 2014 at 01:01:31PM +0200, Rafael J. Wysocki wrote: > > > > > On Wednesday, May 21, 2014 01:05:11 PM Heikki Krogerus wrote: > > > > > > On Tue, May 20, 2014 at 11:33:09PM +0200, Rafael J. Wysocki wrote: > > > > > > > First, is the 10 ms sleep really necessary? I'd expect the AML > > > > > > > to take care of > > > > > > > such delays (this is not a PCI device formally). > > > > > > > > > > > > Unfortunately that is not the case. There is nothing in the AML for > > > > > > this. Mika, correct me if I'm wrong. > > > > > > > > > > > > > And because this is not a PCI device formally, why is the comment > > > > > > > talking about > > > > > > > the PCI spec? Why is PCI relevant in any way here? > > > > > > > > > > > > Under the hood the devices are still PCI devices, even if they > > > > > > formally aren't. Maybe I should point that out in the comment.. > > > > > > > > > > > > We put the sleep there because without it there was no guarantee if > > > > > > the device was properly resumed by the time the drivers resume hooks > > > > > > were called. The symptom in case of a failure was simply that the > > > > > > registers could not be written, which leads into timeouts at least > > > > > > in > > > > > > case of the I2C and UART and making them unusable until the next > > > > > > suspend followed by resume. > > > > > > > > > > OK, so the msleep() is functionally necessary. Instead of talking > > > > > about the > > > > > PCI in the comment, which will make a casual reader think "What the > > > > > heck?", > > > > > please say something like "the delay is necessary for the subsequent > > > > > register > > > > > writes to succeed on <example system>". > > > > > > > > OK. > > > > > > So I have one more concern. Namely, async suspend is not enabled for the > > > LPSS > > > devices, so the delays will accumulate for them and that may become a big > > > deal > > > at one point. > > > > > > This may be addressed either (1) by enabling async suspend for them or, > > > which would > > > be more complicated, by doing the msleep() once for the whole LPSS in > > > .resume_early() > > > and restoring the register values in .resume() without delaying. > > > > > > For (1) I have the following untested patch (on top of my bleeding-edge > > > branch, but > > > it should apply to the mainline too if I haven't overlooked anything). > > > Can you > > > please try it on boxes with LPSS and see if it doesn't break > > > suspend/resume on them? > > > > Done, and there were no problems. I tested it with HSW, BYT and also BDW. > > Great, thanks! > > So I'll add a changelog and I'm going to push it along with your series. > > Are you going to update the $subject patch, or send an update on top of > linux-next?
I'll send an update right away. On top of explaining the problem if left without the msleep(10), I still left the comment about the PCI spec plus a mention that all LPSS devices are actually PCI devices. I hope that's OK with you. It just felt like we need to point out that the 10ms is not pulled out of the hat. -- heikki -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
