Re: [alsa-devel] [PATCH] Modify mpc5200 AC97 driver to use V9 of spin_event_timeout()
On Tue, May 26, 2009 at 7:25 PM, Jon Smirl jonsm...@gmail.com wrote: - spin_event_timeout(0, 10, 0, rc); + spin_event_timeout(0, 10, 0); out_8(regs-op0, MPC52xx_PSC_OP_RES); - spin_event_timeout(0, 50, 0, rc); + spin_event_timeout(0, 50, 0); Jon, I'm still hoping you'll explain why you're not using udelay() here. -- Timur Tabi Linux kernel developer at Freescale ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [alsa-devel] [PATCH] Modify mpc5200 AC97 driver to use V9 of spin_event_timeout()
On Tue, May 26, 2009 at 8:38 PM, Timur Tabi ti...@freescale.com wrote: On Tue, May 26, 2009 at 7:25 PM, Jon Smirl jonsm...@gmail.com wrote: - spin_event_timeout(0, 10, 0, rc); + spin_event_timeout(0, 10, 0); out_8(regs-op0, MPC52xx_PSC_OP_RES); - spin_event_timeout(0, 50, 0, rc); + spin_event_timeout(0, 50, 0); Jon, I'm still hoping you'll explain why you're not using udelay() here. Because Grant didn't want me doing udelay(50) just to delay things in order to give the AC97 controller time to initialize. Your function lets me loop on cpu_relax() for 50us. I have to delay 50us because ALSA tries to access the hardware immediately after the function returns. -- Timur Tabi Linux kernel developer at Freescale -- Jon Smirl jonsm...@gmail.com ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [alsa-devel] [PATCH] Modify mpc5200 AC97 driver to use V9 of spin_event_timeout()
On Tue, May 26, 2009 at 7:44 PM, Jon Smirl jonsm...@gmail.com wrote: Because Grant didn't want me doing udelay(50) just to delay things in order to give the AC97 controller time to initialize. Your function lets me loop on cpu_relax() for 50us. But udelay() calls HMT_low(), which is like cpu_relax(). -- Timur Tabi Linux kernel developer at Freescale ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [alsa-devel] [PATCH] Modify mpc5200 AC97 driver to use V9 of spin_event_timeout()
On Tue, May 26, 2009 at 8:53 PM, Timur Tabi ti...@freescale.com wrote: On Tue, May 26, 2009 at 7:44 PM, Jon Smirl jonsm...@gmail.com wrote: Because Grant didn't want me doing udelay(50) just to delay things in order to give the AC97 controller time to initialize. Your function lets me loop on cpu_relax() for 50us. But udelay() calls HMT_low(), which is like cpu_relax(). Then why did you need to make your routine that calls cpu_relax()? I don't know what goes on in the guts of HMT_low() and cpu_relax(), when you guys decide which one I should use let me know and I can adjust the patch. The hardware needs a minimum 50us pause. It doesn't matter if the pause is more than that. If the CPU has something to keep it busy for a few milliseconds that's fine. -- Timur Tabi Linux kernel developer at Freescale -- Jon Smirl jonsm...@gmail.com ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [alsa-devel] [PATCH] Modify mpc5200 AC97 driver to use V9 of spin_event_timeout()
On Tue, May 26, 2009 at 8:01 PM, Jon Smirl jonsm...@gmail.com wrote: Then why did you need to make your routine that calls cpu_relax()? That gets called only if delay == 0. udelay(0) is a no-op, so if the caller specifies no delay, then I need to manually call cpu_relax(). I don't know what goes on in the guts of HMT_low() and cpu_relax(), when you guys decide which one I should use let me know and I can adjust the patch. Grant, I don't see any reason why udelay(50) is unacceptable. -- Timur Tabi Linux kernel developer at Freescale ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [alsa-devel] [PATCH] Modify mpc5200 AC97 driver to use V9 of spin_event_timeout()
On Tue, May 26, 2009 at 6:44 PM, Jon Smirl jonsm...@gmail.com wrote: On Tue, May 26, 2009 at 8:38 PM, Timur Tabi ti...@freescale.com wrote: On Tue, May 26, 2009 at 7:25 PM, Jon Smirl jonsm...@gmail.com wrote: - spin_event_timeout(0, 10, 0, rc); + spin_event_timeout(0, 10, 0); out_8(regs-op0, MPC52xx_PSC_OP_RES); - spin_event_timeout(0, 50, 0, rc); + spin_event_timeout(0, 50, 0); Jon, I'm still hoping you'll explain why you're not using udelay() here. Because Grant didn't want me doing udelay(50) just to delay things in order to give the AC97 controller time to initialize. Your function lets me loop on cpu_relax() for 50us. No, you misunderstand. My issue is that udelay is far too coarse grained for what you want. ie. If your are using udelay(1) in your loop, and your event changes state in 1.01us, then you're still going to burn a full 2us of time. If you can loop over cpu_relax() instead, then less time will get burned. For a hard 50us delay, with no early exit condition, udelay() is the correct function. HOWEVER; I'm distrustful of *any* spins, and would rather see sleeps instead of spins wherever possible. There are two reasons for this: First, udelay just burns time, and if the delay is too large, then the it is wasting time that could be used for something else. That being said, it needs to be balanced with the context switch overhead. If the udelay() is less than double the context switch time, then the overhead is greater than the time spent spinning. Second, udelay() with IRQs disabled cause unbounded latencies. Even if it is less efficient, I'd rather see mutexes+msleep() than spin_lock_irqsave()+udelay(). Of course, this only works in non-atomic contexts. If the code is atomic, then you have no choice. There is no blanket rule here; but as the time delay requirement gets larger, the greater the benefit to overall system responsiveness by using sleep instead of delay. I'm not going to make a hard statement about how it should be done right now. It's a fiddly area and it can be tweaked after the driver is merged. However, Timur is absolutely right. A spin without an early exit condition should simply be a udelay(). I have to delay 50us because ALSA tries to access the hardware immediately after the function returns. Does the code run in atomic context? g. -- Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [alsa-devel] [PATCH] Modify mpc5200 AC97 driver to use V9 of spin_event_timeout()
On Tue, May 26, 2009 at 9:12 PM, Timur Tabi ti...@freescale.com wrote: On Tue, May 26, 2009 at 8:01 PM, Jon Smirl jonsm...@gmail.com wrote: Then why did you need to make your routine that calls cpu_relax()? That gets called only if delay == 0. udelay(0) is a no-op, so if the caller specifies no delay, then I need to manually call cpu_relax(). I don't know what goes on in the guts of HMT_low() and cpu_relax(), when you guys decide which one I should use let me know and I can adjust the patch. Grant, I don't see any reason why udelay(50) is unacceptable. It's not. See my last email. g. -- Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
