Messages in this thread | | | Date | Sun, 14 Jun 2009 14:52:56 +0800 | From | Harald Welte <> | Subject | Re: [PATCH] mmc: Add new via-sdmmc host controller driver |
| |
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@viatech.com> 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@viatech.com> http://linux.via.com.tw/ ============================================================================ VIA Free and Open Source Software Liaison [unhandled content-type:application/pgp-signature] | |