Re: [Patch 1/1 v5] via-sdmmc: VIA MSP card reader driver support

[Harald Welte <HaraldWelte@viatech.com>]

Dear Joseph, Pierre and others.

Since the driver has still not made any progress in recent days, I took the
liberty of "adopting" it.  It can now be found in my linux-2.6-via.git
repository under the via-vx855-sdmmc branch (gitweb at
http://git.gnumonks.org/cgi-bin/gitweb.cgi?p=linux-2.6-via.git;a=shortlog;h=refs/heads/via-vx855-sdmmc)

As you can see, there are five incremental patches compared to the last v5
release that Joseph posted in April.

On Wed, Apr 22, 2009 at 01:35:29AM +0800, JosephChan@via.com.tw wrote:

> >> +nodata:
> >> +	if (cmd->opcode == MMC_STOP_TRANSMISSION)
> >> +		cmdctrl |= VIA_CRDR_SDCTRL_STOP;
> > >+
> 
> >I didn't check this part properly the last time. I'm assuming you're telling
> >the hardware that this is a command that terminates a data transmission? If
> >so, you cannot check the opcode like that. There might be other stop
> >commands.
> >What you need to do is to either pass another parameter to your function
> >when you're calling it with data->stop, or simply have a test in there if
> >cmd == data->stop (or mrq->stop, whichever is most convenient).
> 
> OK, we will change to 
>        if(cmd == host->mrq->stop)
>              cmdctrl |= VIA_CRDR_SDCTRL_STOP;

done.

> >> +	else if (ios->clock >=  5000000)
> >> +		clock = PCI_CLK_8M;
> >>+	else
> >>+		clock = PCI_CLK_375K;
> >>+
> 
> > This looks a bit strange. Why are you not comparing with the clock
> > frequency you'll be setting?
> 
> Do you mean that we should check the current clock frequency before setting?
> But our engineer thinks that the following lines can do that work normally
> too: Any comment? 

Well, if the "PCI_CLK" line and the corresponding register actually 1:1
correspond to the actual SD card clock rate, then 

>               else if (ios->clock >=  5000000)
>                      clock = PCI_CLK_8M;

would mean we use 8MHz even at the time the MMC core stack asks us to only
use 5MHz, whcih is a bug!

I have fixed this in my git tree now.

> >> +	mmc->caps = MMC_CAP_4_BIT_DATA;
> 
> > You also need to set the bits stating if the controller supports the
> > high-speed SD and/or MMC mode, otherwise the core won't go over 25/20 MHz.
> 
> 
> OK, we will add the following line in the via_init_mmc_host function:
> 
>        mmc->caps |= MMC_CAP_MMC_HIGHSPEED |MMC_CAP_SD_HIGHSPEED;

Since I cannot see any indication from the VX800 / VX855 data sheet that HS-MMC
is actually supported, I only use SD_HIGHSPEED for now.

Joseph: Can you find out if the controller actually supports HS-MMC, not only
SD highspeed?

>        org_data = readl(addrbase + VIA_CRDR_SDEXTCTRL);
> 
>        if ((ios->timing == MMC_TIMING_SD_HS) || (ios->timing == MMC_TIMING_MMC_HS))
>               org_data |= VIA_CRDR_SDEXTCTRL_HISPD;
>        else
>               org_data &= ~VIA_CRDR_SDEXTCTRL_HISPD;
> 
>        writel(org_data, addrbase + VIA_CRDR_SDEXTCTRL);       

tried that, unfortunately this is buggy, since org_data refers to SDBUSMODE rather than SDEXTCTRL. I've fixed it in this patch:
http://git.gnumonks.org/cgi-bin/gitweb.cgi?p=linux-2.6-via.git;a=commitdiff;h=bb1f9f2d9d51bbf294ae036d02e2546f2d8b36c3

The resulting driver runs so far fine on my VX855ES based board, but I'll give
it some more testing before re-submitting the patch.

There is also one case in the code where due to a DMA controller bug, we force
the clock to 8MHz despite the MMC core requesting 375kHz.  I think the proper
fix here is to fall back on PIO (non-DMA) mode for 375kHz, rather than
overclocking the poor card by more than 21 times.

After that has been fixed, plus some more testing, I'll re-submit.

Regards,
-- 
- Harald Welte <HaraldWelte@viatech.com>	    http://linux.via.com.tw/
============================================================================
VIA Open Source Liaison