[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <C80EF34A3D2E494DBAF9AC29C7AE4EB808521780@exchtp03.taipei.via.com.tw>
Date: Wed, 17 Dec 2008 17:45:42 +0800
From: <JosephChan@....com.tw>
To: <arnd@...db.de>
Cc: <linux-kernel@...r.kernel.org>
Subject: RE: [Patch 2/3] via-sdmmc: via-sdmmc.c
Thanks, Arnd.
> > + pm_pcictrl_reg->pcitmoctrl_reg = readb(addrbase +
> PCITMOCTRL_REG); }
>
> Since you already define the data structure for the save
> area, how about using it for the register accesses as well?
> You could drop all the PCI*_REG macro definitions and do
>
> struct pcictrlreg __iomem *pcr =
> vcrdr_chip->pcictrl_mmiobase; pm_pcictrl_reg->pcisdclk_reg =
> readb(&pcr->pcisdclk_reg);
>
> Of course, your code is doing the same things effectively and
> entirely ok here.
We'll modify the code according to your suggestions..
> > + }
> > +
> > + via_set_ddma(host->chip, virt_to_phys(host->ddmabuf),
> count, dir,
> > +0);
>
> You can't use virt_to_phys here, since the physical address
> might not be the same as the bus address, in case you are
> using an IOMMU. The correct way to do it would be to drop the
> memcpy to the internal buffer, and do a
> dma_map_sg() to get the bus address.
>
We'll try to modify the code like below, but need more tests.
In via_sdc_preparedata() function:
int sg_cnt;
sg_cnt = dma_map_sg(mmc_dev(host->mmc), data->sg, data->sg_len, (data->flags & MMC_DATA_READ) ? DMA_FROM_DEVICE : DMA_TO_DEVICE);
WARN_ON(sg_cnt != 1);
via_set_ddma(host->chip, sg_dma_address(data->sg), sg_dma_len(data->sg), (data->flags & MMC_DATA_READ) ? DMA_FROM_DEVICE : DMA_TO_DEVICE );
In via_sdc_finish_data() function:
dma_unmap_sg(mmc_dev(host->mmc), data->sg, data->sg_len, (data->flags & MMC_DATA_READ) ? DMA_FROM_DEVICE : DMA_TO_DEVICE);
> > +
> > + sd_status &= ~(SD_STS_CMD_MASK | SD_STS_DATA_MASK);
> > + if (sd_status) {
> > + pr_err("%s: Unexpected interrupt 0x%x\n",
> > + mmc_hostname(sdhost->mmc), sd_status);
> > + writew(sd_status, addrbase + SDSTATUS_REG);
> > + }
>
> What are your criteria for deciding which events to handle in
> interrupt context or in tasklet context? Are some of them
> extremely performance critical?
The criteria are:
1. Because the SD card detect operations need delay about 1 ms, so it should not be implemented in interrupt context. So we implement it by via_sdc_tasklet_card.
2. The STSTATUS_REG register must be reset quickly, so it should be implemented in interrupt context.
3. In order to finish one “request” from the mmc_block layer quickly, all operations (that can finished quickly) before the end of the “request” should be implemented in interrupt context.
> If you can do all of them in a single tasklet function that
> you simply schedule every time without the spinlock, you
> don't need to disable interrupts every time you access a
> register but can instead use spin_lock_bh.
>
> To go even further, you could use a work queue instead of the
> tasklet and use a mutex instead of the spinlock.
We will try to optimize the ISR function.
> > + writel(0x0, addrbase + SDINTMASK_REG);
> > + mdelay(1);
>
> Since this function runs in task context, you should use
> msleep() here, not mdelay().
OK, we will replae it. Thanks.
Powered by blists - more mailing lists