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.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
