Messages in this thread | | | Subject | RE: [Patch 2/3] via-sdmmc: via-sdmmc.c | Date | Wed, 17 Dec 2008 17:45:42 +0800 | From | <> |
| |
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.
| |