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

[Chin Liang See]

On Wed, 2016-01-27 at 00:01 +0000, Trent Piepho wrote:
> 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.


Do note that these cal value are not passed to Linux. Wonder your Linux
is using the cal value as obtained from U-Boot calibration?

> 
> 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?  


Per SD specifications, set block length (CMD16) will expect response R1
which is normal response from the card.


> 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.


Wonder any difference if you change the value in DTS (without the
calibration code)? I am referring U-Boot accessing the SD card.


> 
> > +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.
> 


Oh there is a link at the function's comment to explain the algo.


> 
> > +	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.


This is also part of comment. In quick, we are seeing meta stability
issue when drvsel = 0 and that why we skip it


> 
> > +
> > +	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.

Same as previous comment

Thanks
Chin Liang