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

[Marek Vasut]

On Tuesday, December 01, 2015 at 04:32:44 PM, Chin Liang See wrote:
> Hi Marek,

Hi!

> On Sun, 2015-11-29 at 14:39 +0100, Marek Vasut wrote:
> > On Friday, November 27, 2015 at 08:22:03 AM, 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.
> > > 
> > > Signed-off-by: Chin Liang See <clsee at altera.com>
> > > Cc: Dinh Nguyen <dinguyen at opensource.altera.com>
> > > Cc: Dinh Nguyen <dinh.linux at gmail.com>
> > > Cc: Pavel Machek <pavel at denx.de>
> > > Cc: Marek Vasut <marex at denx.de>
> > > Cc: Stefan Roese <sr at denx.de>
> > > Cc: Pantelis Antoniou <pantelis.antoniou at konsulko.com>
> > > Cc: Simon Glass <sjg at chromium.org>
> > > Cc: Jaehoon Chung <jh80.chung at samsung.com>
> > > ---
> > > Changes for v4
> > > - Calibration only run if node not in DTS
> > > Changes for v3
> > > - Remove the && ok as its redundant
> > > Changes for v2
> > > - Using standard error return macro
> > > - Split to small function to avoid deep identation
> > > - Fix coding standard
> > > ---
> > > 
> > >  drivers/mmc/socfpga_dw_mmc.c | 208
> > > 
> > > ++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 205
> > > insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/mmc/socfpga_dw_mmc.c
> > > b/drivers/mmc/socfpga_dw_mmc.c
> > > index 2bd0ebd..a8e2660 100644
> > > --- a/drivers/mmc/socfpga_dw_mmc.c
> > > +++ b/drivers/mmc/socfpga_dw_mmc.c
> > > @@ -13,6 +13,7 @@
> > > 
> > >  #include <asm/arch/dwmmc.h>
> > >  #include <asm/arch/clock_manager.h>
> > >  #include <asm/arch/system_manager.h>
> > > 
> > > +#include "mmc_private.h"
> > > 
> > >  static const struct socfpga_clock_manager *clock_manager_base =
> > >  
> > >  		(void *)SOCFPGA_CLKMGR_ADDRESS;
> > > 
> > > @@ -25,7 +26,144 @@ struct dwmci_socfpga_priv_data {
> > > 
> > >  	unsigned int smplsel;
> > >  
> > >  };
> > > 
> > > -static void socfpga_dwmci_clksel(struct dwmci_host *host)
> > > +/*
> > > + * rows and columns of calibration rectange. The values are based
> > > on the
> > > value + * range of drvsel and smplsel register in system manager.
> > > Note
> > > drvsel 0 is + * unusable as it has meta-stability issue.
> > > + */
> > > +#define SOCFPGA_SD_DRVSEL  7
> > > +#define SOCFPGA_SD_SMPLSEL 8
> > > +
> > > +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)
> > > +{
> > > +	unsigned char start_row, start_col;
> > > +
> > > +	/* Find the row and column where the candidate fits */
> > > +	for (start_col = 0; start_col < (SOCFPGA_SD_SMPLSEL -
> > > rect_width + 1);
> > > +	     start_col++) {
> > > +		for (start_row = 0;
> > > +		     start_row < (SOCFPGA_SD_DRVSEL - rect_height
> > > + 1);
> > > +		     start_row++) {
> > > +			unsigned ok = 1;
> > > +			unsigned add_col, add_row;
> > > +
> > > +			/* Determine if the rectangle fits here */
> > > +			for (add_col = 0; (add_col < rect_width);
> > > add_col++) {
> > > +				for (add_row = 0; add_row <
> > > rect_height;
> > > +				     add_row++) {
> > > +					if (!cal_results[start_row
> > > + add_row]
> > > +					    [start_col + add_col])
> > > {
> > > +						ok = 0;
> > > +						break;
> > > +					}
> > > +				}
> > > +			}
> > > +
> > > +			/*
> > > +			 * Return 'middle' of rectangle in case of
> > > +			 * success
> > > +			 */
> > 
> > I think you can refactor this indentation horror some more, right ?
> 
> Ok, let me use shorter variable to avoid going next line.
> 
> > > +			if (ok) {
> > > +				if (rect_width > 1)
> > > +					rect_width--;
> > > +
> > > +				if (rect_height > 1)
> > > +					rect_height--;
> > > +
> > > +				*cal_row = start_row +
> > > (rect_height / 2);
> > > +				*cal_col = start_col + (rect_width
> > > / 2);
> > > +
> > > +				return 0;
> > 
> > For example this condition can be inverted and it'd shave off one
> > level
> > of indent.
> 
> Ok let me take a look when doing the house keeping

That's really help, thanks/

> > > +			}
> > > +		}
> > > +	}
> > > +	return -EINVAL;
> > > +}

[...]

> > > @@ -102,8 +304,8 @@ static int socfpga_dwmci_of_probe(const void
> > > *blob, int
> > > node, const int idx) host->bus_hz = clk;
> > > 
> > >  	host->fifoth_val = MSIZE(0x2) |
> > >  	
> > >  		RX_WMARK(fifo_depth / 2 - 1) | TX_WMARK(fifo_depth
> > > 
> > > / 2);
> > > -	priv->drvsel = fdtdec_get_uint(blob, node, "drvsel", 3);
> > > -	priv->smplsel = fdtdec_get_uint(blob, node, "smplsel", 0);
> > > +	priv->drvsel = fdtdec_get_uint(blob, node, "drvsel", 0xf);
> > > +	priv->smplsel = fdtdec_get_uint(blob, node, "smplsel",
> > > 0xf);
> > 
> > This breaks multiple boards, since it misconfigures the default
> > values.
> 
> The 0xf is non valid value. When the node is missing, we will get 0xf
> during the probe. When the init start, the non valid value will trigger
> the calibration to get the correct value.

OK, this is bad. Originally, if we didn't specify these in the DT, we would
use the default values of 0x3 and 0x0 , but now we do the calibration. I wonder,
do we care about DT ABI compatibility on the U-Boot level or not ?