Re: [PATCH] mmc: Add new via-sdmmc host controller driver

[Harald Welte]

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