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

[Trent Piepho]

On Wed, 2016-01-27 at 21:13 +0800, Chin Liang See wrote:
> 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?

I'm aware of that.  Running the calibration code and seeing if it worked
is the first step.  Passing that to Linux would be next.

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

But the response is received using the CMD line.  The data line(s) are
not used.  My tests show that a set block length command will succeed
with clock phase adjustment settings that a read command will fail on
with a data CRC error.

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

Any difference in what?  How well the SD card works?  Changing the phase
settings does effect that.  As I said, "drvsel 3 smplsel 7 does not work
at all on our board. Every read command tried with those settings will
fail."


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

You set drvsel to one less than it should be.  Think about what the
drvsel value will be programmed into the register if row 0 is selected.