lkml.org 
[lkml]   [2008]   [Dec]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRE: [Patch 2/3] via-sdmmc: via-sdmmc.c
Date
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.
\
 
 \ /
  Last update: 2008-12-17 10:49    [W:0.071 / U:3.112 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site