Re: [PATCH] mmc: Add new via-sdmmc host controller driver
From: Harald Welte
Date: Sun Jun 14 2009 - 03:00:39 EST
Hi Pierre,
as usual, thanks for your review+comments.
On Sat, Jun 13, 2009 at 01:43:28PM +0200, Pierre Ossman wrote:
> On Fri, 12 Jun 2009 15:59:30 +0800
> Harald Welte <HaraldWelte@xxxxxxxxxxx> wrote:
>
> > +
> > +#define MMC_STOP_TRANSMISSION 12
>
> This define is no longer used.
thanks, removed.
> > + /* It seems that our DMA can not work normally with 375kHz clock */
> > + /* FIXME: don't brute-force 8MHz but use PIO at 375kHz !! */
> > + addrbase = host->pcictrl_mmiobase;
> > + if (readb(addrbase + VIA_CRDR_PCISDCCLK) == PCI_CLK_375K) {
> > + dev_info(host->mmc->parent, "forcing card speed to 8MHz\n");
> > + writeb(PCI_CLK_8M, addrbase + VIA_CRDR_PCISDCCLK);
> > + }
>
> This is pretty nasty. Please see if you can provide a workaround some
> time soon.
yes, I'm looking into this, definitely. Will need some
experimentation/testing, but I'll submit an incremental fix ASAP.
> > + if (ios->power_mode != MMC_POWER_OFF)
> > + via_sdc_set_power(host, ios->vdd);
>
> What if we actually do need to power off?
I see. Good spot. It seems like we actually leave the power on,
and instread only switch the clock off:
> if (ios->power_mode == MMC_POWER_OFF)
> org_data &= ~VIA_CRDR_SDMODE_POWER;
> else
> org_data |= VIA_CRDR_SDMODE_POWER;
according to the (public) docs, this bit is not about power but about
clock gating. I have renamed it to SDMODE_CLK_ON to make it more clear.
> > +static void __devexit via_sd_remove(struct pci_dev *pcidev)
> > +{
> > + struct via_crdr_mmc_host *sdhost;
> > + u8 gatt;
> > +
> > + pr_info(DRV_NAME
> > + ": VIA SDMMC controller at %s [%04x:%04x] has been removed\n",
> > + pci_name(pcidev), (int)pcidev->vendor, (int)pcidev->device);
> > +
> > + sdhost = pci_get_drvdata(pcidev);
> > +
> > + tasklet_kill(&sdhost->finish_tasklet);
> > +
> > + if (&sdhost->timer)
> > + del_timer_sync(&sdhost->timer);
> > +
> > + mmc_remove_host(sdhost->mmc);
> > +
>
> You need to do the removal as the first step
Thanks, I've fixed this in my tree.
Looking at other drivers, imxmmc seems to get this wrong, too.
> and you might need to explicitly kill any ongoing requests.
you mean something like the snippet in sdhci.c:
======
if (dead) {
spin_lock_irqsave(&host->lock, flags);
host->flags |= SDHCI_DEVICE_DEAD;
if (host->mrq) {
printk(KERN_ERR "%s: Controller removed during "
" transfer!\n", mmc_hostname(host->mmc));
host->mrq->cmd->error = -ENOMEDIUM;
tasklet_schedule(&host->finish_tasklet);
}
spin_unlock_irqrestore(&host->lock, flags);
}
======
Why is sdhci about the only driver that does it? What decides if we need
to kill ongoing requests or not?
Also, you stated that mmc_remove_host needs to be the first step. However,
sdhci first kills the ongoing rquest (if "dead"), and then removes the host.
--
- Harald Welte <HaraldWelte@xxxxxxxxxxx> http://linux.via.com.tw/
============================================================================
VIA Free and Open Source Software Liaison
Attachment:
signature.asc
Description: Digital signature