[U-Boot] [PATCH v4] mmc: socfpga_dw_mmc: Enable calibration for drvsel and smplsel

[Trent Piepho]

On Fri, 2015-11-27 at 15:22 +0800, Chin Liang See wrote:
> Enable SDMMC calibration to determine the best setting for
> drvsel and smplsel. Calibration will be triggered if the
> drvsel and smplsel node are not available in DTS.

We have a Cyclone V based board and on the latest revision the SD card
interface has become unreliable at 50 MHz.  The SD card moved a few
inches further away from the SoC in this revision.  I though maybe
smplsel could be an issue and found this patch.

I ported this to our board, but found that this algorithm doesn't appear
to work that well for us.

In my testing I found the fastest way to produce SD errors on Linux was
to repeatedly issue two sector read multiple commands.  This produces
about one error per 10,000 commands.  Usually a CRC error on the
response but also data CRC errors and data stop bit errors.

This calibration code tests by using a single set block length command
to check if the combination of drvsel and smplsel is good.  Obviously if
0.01% of commands fail then checking via the result of one command is
nearly useless.  And indeed this is the case, every cal test passes and
one gets a table of all trues.

It is however worse than that.  Some cal settings are clearly bad.  For
instance, drvsel 3 smplsel 7 does not work at all on our board.  Every
read command tried with those settings will fail.  But the set block
length command succeeds.  Perhaps because it neither sends nor receives
any data?  So the calibration seems unlikely to improve anything, since
it will always choose 3, 3 for the settings.  The basic premise, that it
can tell if a pair of settings work or not, appears to not be true.


> +static int socfpga_dwmci_find_row_col_fit_rectangle(unsigned rect_width,
> +	unsigned rect_height,
> +	unsigned char cal_results[SOCFPGA_SD_DRVSEL][SOCFPGA_SD_SMPLSEL],
> +	unsigned int *cal_row, unsigned int *cal_col)

There is no documentation of what exactly parameters are here.  But one
can determine that cal_row will contain the zero based index of the
"best" row.


> +	for (row = 0; row < SOCFPGA_SD_DRVSEL; row++) {
> +		for (col = 0; col < SOCFPGA_SD_SMPLSEL; col++) {
> +			priv->drvsel = row + 1;
> +			priv->smplsel = col;

Here drvsel is set to row + 1.

> +
> +	err = socfpga_dwmci_find_calibration_point(cal_results, sum,
> +						   &priv->drvsel,
> +						   &priv->smplsel);

But this function returns the zero based row number (I think it would be
easier to notice if it were documented) in priv->drvsel.  Not row + 1.
It seems that you will always program drvsel to be 1 smaller than it
should be.