| Message ID | 20090527002530.16740.62502.stgit@terra (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Tue, May 26, 2009 at 7:25 PM, Jon Smirl <jonsmirl@gmail.com> wrote: > - spin_event_timeout(0, 10, 0, rc); > + spin_event_timeout(0, 10, 0); > out_8(®s->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.
On Tue, May 26, 2009 at 8:38 PM, Timur Tabi <timur@freescale.com> wrote: > On Tue, May 26, 2009 at 7:25 PM, Jon Smirl <jonsmirl@gmail.com> wrote: > >> - spin_event_timeout(0, 10, 0, rc); >> + spin_event_timeout(0, 10, 0); >> out_8(®s->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 >
On Tue, May 26, 2009 at 7:44 PM, Jon Smirl <jonsmirl@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().
On Tue, May 26, 2009 at 8:53 PM, Timur Tabi <timur@freescale.com> wrote: > On Tue, May 26, 2009 at 7:44 PM, Jon Smirl <jonsmirl@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 >
On Tue, May 26, 2009 at 8:01 PM, Jon Smirl <jonsmirl@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.
On Tue, May 26, 2009 at 6:44 PM, Jon Smirl <jonsmirl@gmail.com> wrote: > On Tue, May 26, 2009 at 8:38 PM, Timur Tabi <timur@freescale.com> wrote: >> On Tue, May 26, 2009 at 7:25 PM, Jon Smirl <jonsmirl@gmail.com> wrote: >> >>> - spin_event_timeout(0, 10, 0, rc); >>> + spin_event_timeout(0, 10, 0); >>> out_8(®s->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.
On Tue, May 26, 2009 at 9:12 PM, Timur Tabi <timur@freescale.com> wrote: > On Tue, May 26, 2009 at 8:01 PM, Jon Smirl <jonsmirl@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.
On Tue, May 26, 2009 at 9:48 PM, Grant Likely <grant.likely@secretlab.ca> wrote: > 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. BTW, lmbench lat_ctx test case tells me that context switch latency is measured somewhere around 30 to 60 us for user space processes sleeping on a file descriptor. That will be lower for kernel threads since there is no syscall overhead to account for. g.
diff --git a/sound/soc/fsl/mpc5200_psc_ac97.c b/sound/soc/fsl/mpc5200_psc_ac97.c index 9f2df15..bab80ac 100644 --- a/sound/soc/fsl/mpc5200_psc_ac97.c +++ b/sound/soc/fsl/mpc5200_psc_ac97.c @@ -31,13 +31,13 @@ static struct psc_dma *psc_dma; static unsigned short psc_ac97_read(struct snd_ac97 *ac97, unsigned short reg) { - int rc; + int status; unsigned int val; /* Wait for command send status zero = ready */ - spin_event_timeout(!(in_be16(&psc_dma->psc_regs->sr_csr.status) & - MPC52xx_PSC_SR_CMDSEND), 100, 0, rc); - if (rc == 0) { + status= spin_event_timeout(!(in_be16(&psc_dma->psc_regs->sr_csr.status) & + MPC52xx_PSC_SR_CMDSEND), 100, 0); + if (status == 0) { pr_err("timeout on ac97 bus (rdy)\n"); return -ENODEV; } @@ -45,9 +45,9 @@ static unsigned short psc_ac97_read(struct snd_ac97 *ac97, unsigned short reg) out_be32(&psc_dma->psc_regs->ac97_cmd, (1<<31) | ((reg & 0x7f) << 24)); /* Wait for the answer */ - spin_event_timeout((in_be16(&psc_dma->psc_regs->sr_csr.status) & - MPC52xx_PSC_SR_DATA_VAL), 100, 0, rc); - if (rc == 0) { + status = spin_event_timeout((in_be16(&psc_dma->psc_regs->sr_csr.status) & + MPC52xx_PSC_SR_DATA_VAL), 100, 0); + if (status == 0) { pr_err("timeout on ac97 read (val) %x\n", in_be16(&psc_dma->psc_regs->sr_csr.status)); return -ENODEV; @@ -66,12 +66,12 @@ static unsigned short psc_ac97_read(struct snd_ac97 *ac97, unsigned short reg) static void psc_ac97_write(struct snd_ac97 *ac97, unsigned short reg, unsigned short val) { - int rc; + int status; /* Wait for command status zero = ready */ - spin_event_timeout(!(in_be16(&psc_dma->psc_regs->sr_csr.status) & - MPC52xx_PSC_SR_CMDSEND), 100, 0, rc); - if (rc == 0) { + status = spin_event_timeout(!(in_be16(&psc_dma->psc_regs->sr_csr.status) & + MPC52xx_PSC_SR_CMDSEND), 100, 0); + if (status == 0) { pr_err("timeout on ac97 bus (write)\n"); return; } @@ -82,24 +82,22 @@ static void psc_ac97_write(struct snd_ac97 *ac97, static void psc_ac97_warm_reset(struct snd_ac97 *ac97) { - int rc; struct mpc52xx_psc __iomem *regs = psc_dma->psc_regs; out_be32(®s->sicr, psc_dma->sicr | MPC52xx_PSC_SICR_AWR); - spin_event_timeout(0, 3, 0, rc); + spin_event_timeout(0, 3, 0); out_be32(®s->sicr, psc_dma->sicr); } static void psc_ac97_cold_reset(struct snd_ac97 *ac97) { - int rc; struct mpc52xx_psc __iomem *regs = psc_dma->psc_regs; /* Do a cold reset */ out_8(®s->op1, MPC52xx_PSC_OP_RES); - spin_event_timeout(0, 10, 0, rc); + spin_event_timeout(0, 10, 0); out_8(®s->op0, MPC52xx_PSC_OP_RES); - spin_event_timeout(0, 50, 0, rc); + spin_event_timeout(0, 50, 0); psc_ac97_warm_reset(ac97); }
The function signature for spin_event_timeout() has changed in version V9. Adjust the mpc5200 AC97 driver to use the new function. Signed-off-by: Jon Smirl <jonsmirl@gmail.com> --- sound/soc/fsl/mpc5200_psc_ac97.c | 30 ++++++++++++++---------------- 1 files changed, 14 insertions(+), 16 deletions(-)