On Wed, 2016-08-24 at 08:43 -0700, Tony Lindgren wrote: > * bigeasy@xxxxxxxxxxxxx <bigeasy@xxxxxxxxxxxxx> [160824 08:24]: > > > > On 2016-08-24 15:16:08 [+0000], Shevchenko, Andriy wrote: > > > > > > On Wed, 2016-08-24 at 17:03 +0200, bigeasy@xxxxxxxxxxxxx wrote: > > > > > > > > On 2016-08-24 14:56:12 [+0000], Shevchenko, Andriy wrote: > > > > > > > > > > > > > > > > > Today I discovered that runtime PM rather broken in 8250 _by > > > > > design_. > > > > > > > > > > > > > > > > > > > > It > > > > > definitely requires irq_safe flag to be set in runtime PM. But > > > > > that > > > > > flag > > > > > demolishes an effort since it keeps parent (and thus all other > > > > > devices > > > > > which parent needs to be on) always on. > > > > > > ^^^^^ (1) > > > > > > > > > > > > > > > > > > > > > > The question is is somebody is going to fix this and when? > > > > > > > > Why is it broken? > > > > > > I meant that without irq_safe flag set there are too many places > > > where > > > "sleep while in atomic" happens. On the other hand the irq_safe > > > flag > > > prevents parent to go to suspend. > > > > This must be new then. I don't remember _any_ "sleeping while > > atomic" > > warnings back then. Maybe Tony knows something. Maybe it does not > > occur > > on TI's PM code. I would have to reanimate my beagle bone to check > > that. > > PM runtime with UARTs has been working for me just fine for quite some > time now. I'm mostly using omap-serial still and have not checked with > 8250-omap recently. Yeah, they both are using irq_safe flag. Actually they two out of only 15 users in entire kernel. arch/arm/plat- omap/dmtimer.c:896: pm_runtime_irq_safe(dev); drivers/clk/ti/clk-dra7-atl.c:233: pm_runtime_irq_safe(cinfo->dev); drivers/clocksource/sh_cmt.c:1074: pm_runtime_irq_safe(&pde v->dev); drivers/clocksource/sh_mtu2.c:478: pm_runtime_irq_safe(&pde v->dev); drivers/clocksource/sh_tmu.c:634: pm_runtime_irq_safe(&pde v->dev); drivers/crypto/omap-des.c:1035: pm_runtime_irq_safe(dev); drivers/crypto/omap-sham.c:1959: pm_runtime_irq_safe(dev); drivers/dma/pl330.c:2972: pm_runtime_irq_safe(&adev->dev); drivers/dma/qcom/bam_dma.c:1259: pm_runtime_irq_safe(&pdev->dev); drivers/dma/ste_dma40.c:3661: pm_runtime_irq_safe(base->dev); drivers/gpio/gpio-omap.c:1214: pm_runtime_irq_safe(dev); drivers/iommu/omap-iommu.c:979: pm_runtime_irq_safe(obj->dev); drivers/power/avs/smartreflex.c:882: pm_runtime_irq_safe(&pdev->dev); drivers/tty/serial/8250/8250_omap.c:1195: pm_runtime_irq_safe(&pde v->dev); drivers/tty/serial/omap-serial.c:1696: pm_runtime_irq_safe(&pdev->dev); Looks like OMAP code is main user of the flag. > But yeah I agree that PM runtime should be improved and simplified for > 8250. Why not to do this once for all in serial_core.c directly? > We should not use irq_safe at all and instead fix up things so > runtime_resume wakes up things with pm_runtime_get without get_sync. > This adds latency only to the first wake-up event which can already > take a long time at least on omaps as things are getting powered back > on. > No idea how to implement that though for cases when there's no > dedicated wakeirq.. Maybe change the irq to a threaded irq for the > duration of idle for 8250 irq? I have no ideas either, but irq_safe should be not used if we are doing pm_runtime_get_sync(). $ git grep -n -w pm_runtime_get_sync | wc -l 738 $ git grep -n -w pm_runtime_get | wc -l 62 -- Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> Intel Finland Oy -- To unsubscribe from this list: send the line "unsubscribe linux-serial" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
