On Tue, 16 Aug 2011, Simon Horman wrote:
> This avoids the need to look up the masks each time an interrupt
> is handled.
Yes, almost... But I think, we can use the mask-caches even more
extensively. In your patch you actually hardly gain anything, you continue
reading the mask register instead of using the cache. Namely:
>
> As suggested by Guennadi.
>
> Cc: Guennadi Liakhovetski <g.liakhovetski@xxxxxx>
> Cc: Magnus Damm <magnus.damm@xxxxxxxxx>
> Signed-off-by: Simon Horman <horms@xxxxxxxxxxxx>
>
> ---
>
> * SDCARD portion tested on AP4/Mackerel
> * SDIO portion untested
> ---
> drivers/mmc/host/tmio_mmc.h | 4 ++++
> drivers/mmc/host/tmio_mmc_pio.c | 36 ++++++++++++++++++++----------------
> 2 files changed, 24 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
> index eeaf643..1cf8db5 100644
> --- a/drivers/mmc/host/tmio_mmc.h
> +++ b/drivers/mmc/host/tmio_mmc.h
> @@ -79,6 +79,10 @@ struct tmio_mmc_host {
> struct delayed_work delayed_reset_work;
> struct work_struct done;
>
> + /* Cache IRQ mask */
> + u32 sdcard_irq_mask;
> + u32 sdio_irq_mask;
> +
> spinlock_t lock; /* protect host private data */
> unsigned long last_req_ts;
> struct mutex ios_lock; /* protect set_ios() context */
> diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
> index 1f16357..c60b15f 100644
> --- a/drivers/mmc/host/tmio_mmc_pio.c
> +++ b/drivers/mmc/host/tmio_mmc_pio.c
> @@ -48,14 +48,16 @@
>
> void tmio_mmc_enable_mmc_irqs(struct tmio_mmc_host *host, u32 i)
> {
> - u32 mask = sd_ctrl_read32(host, CTL_IRQ_MASK) & ~(i & TMIO_MASK_IRQ);
> - sd_ctrl_write32(host, CTL_IRQ_MASK, mask);
> + host->sdcard_irq_mask = sd_ctrl_read32(host, CTL_IRQ_MASK) &
> + ~(i & TMIO_MASK_IRQ);
> + sd_ctrl_write32(host, CTL_IRQ_MASK, host->sdcard_irq_mask);
This function is used often - from each command and from the ISR. Don't
re-read the mask register, use the cached value.
> }
>
> void tmio_mmc_disable_mmc_irqs(struct tmio_mmc_host *host, u32 i)
> {
> - u32 mask = sd_ctrl_read32(host, CTL_IRQ_MASK) | (i & TMIO_MASK_IRQ);
> - sd_ctrl_write32(host, CTL_IRQ_MASK, mask);
> + host->sdcard_irq_mask = sd_ctrl_read32(host, CTL_IRQ_MASK) |
> + (i & TMIO_MASK_IRQ);
> + sd_ctrl_write32(host, CTL_IRQ_MASK, host->sdcard_irq_mask);
ditto
> }
>
> static void tmio_mmc_ack_mmc_irqs(struct tmio_mmc_host *host, u32 i)
> @@ -127,11 +129,13 @@ static void tmio_mmc_enable_sdio_irq(struct mmc_host *mmc, int enable)
>
> if (enable) {
> host->sdio_irq_enabled = 1;
> + host->sdio_irq_mask = TMIO_SDIO_MASK_ALL &
> + ~TMIO_SDIO_STAT_IOIRQ;
> sd_ctrl_write16(host, CTL_TRANSACTION_CTL, 0x0001);
> - sd_ctrl_write16(host, CTL_SDIO_IRQ_MASK,
> - (TMIO_SDIO_MASK_ALL & ~TMIO_SDIO_STAT_IOIRQ));
> + sd_ctrl_write16(host, CTL_SDIO_IRQ_MASK, host->sdio_irq_mask);
> } else {
> - sd_ctrl_write16(host, CTL_SDIO_IRQ_MASK, TMIO_SDIO_MASK_ALL);
> + host->sdio_irq_mask = TMIO_SDIO_MASK_ALL;
> + sd_ctrl_write16(host, CTL_SDIO_IRQ_MASK, host->sdio_irq_mask);
> sd_ctrl_write16(host, CTL_TRANSACTION_CTL, 0x0000);
> host->sdio_irq_enabled = 0;
> }
> @@ -548,26 +552,26 @@ irqreturn_t tmio_mmc_irq(int irq, void *devid)
> struct tmio_mmc_host *host = devid;
> struct mmc_host *mmc = host->mmc;
> struct tmio_mmc_data *pdata = host->pdata;
> - unsigned int ireg, irq_mask, status;
> - unsigned int sdio_ireg, sdio_irq_mask, sdio_status;
> + unsigned int ireg, status;
> + unsigned int sdio_ireg, sdio_status;
>
> pr_debug("MMC IRQ begin\n");
>
> status = sd_ctrl_read32(host, CTL_STATUS);
> - irq_mask = sd_ctrl_read32(host, CTL_IRQ_MASK);
> - ireg = status & TMIO_MASK_IRQ & ~irq_mask;
> + ireg = status & TMIO_MASK_IRQ & ~host->sdcard_irq_mask;
>
> sdio_ireg = 0;
> if (!ireg && pdata->flags & TMIO_MMC_SDIO_IRQ) {
> sdio_status = sd_ctrl_read16(host, CTL_SDIO_STATUS);
> - sdio_irq_mask = sd_ctrl_read16(host, CTL_SDIO_IRQ_MASK);
> - sdio_ireg = sdio_status & TMIO_SDIO_MASK_ALL & ~sdio_irq_mask;
> + host->sdio_irq_mask = sd_ctrl_read16(host, CTL_SDIO_IRQ_MASK);
Ditto - you're in ISR
> + sdio_ireg = sdio_status & TMIO_SDIO_MASK_ALL &
> + ~host->sdio_irq_mask;
>
> sd_ctrl_write16(host, CTL_SDIO_STATUS, sdio_status & ~TMIO_SDIO_MASK_ALL);
>
> if (sdio_ireg && !host->sdio_irq_enabled) {
> pr_warning("tmio_mmc: Spurious SDIO IRQ, disabling! 0x%04x 0x%04x 0x%04x\n",
> - sdio_status, sdio_irq_mask, sdio_ireg);
> + sdio_status, host->sdio_irq_mask, sdio_ireg);
> tmio_mmc_enable_sdio_irq(mmc, 0);
> goto out;
> }
> @@ -623,9 +627,9 @@ irqreturn_t tmio_mmc_irq(int irq, void *devid)
> }
>
> pr_warning("tmio_mmc: Spurious irq, disabling! "
> - "0x%08x 0x%08x 0x%08x\n", status, irq_mask, ireg);
> + "0x%08x 0x%08x 0x%08x\n", status, host->sdcard_irq_mask, ireg);
> pr_debug_status(status);
> - tmio_mmc_disable_mmc_irqs(host, status & ~irq_mask);
> + tmio_mmc_disable_mmc_irqs(host, status & ~host->sdcard_irq_mask);
>
> out:
> return IRQ_HANDLED;
> --
> 1.7.5.4
Instead, what I thought would be a good idea is to initialise the
.irq_mask and .sdio_irq_mask once in tmio_mmc_host_probe() before calling
tmio_mmc_disable_mmc_irqs() and then never read CTL_IRQ_MASK and
CTL_SDIO_IRQ_MASK again - ever!
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
