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

[Chin Liang See <clsee@altera.com>]

On Tue, 2015-12-01 at 16:38 +0100, Marek Vasut wrote:
> 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@altera.com>
> > > > Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
> > > > Cc: Dinh Nguyen <dinh.linux@gmail.com>
> > > > Cc: Pavel Machek <pavel@denx.de>
> > > > Cc: Marek Vasut <marex@denx.de>
> > > > Cc: Stefan Roese <sr@denx.de>
> > > > Cc: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> > > > Cc: Simon Glass <sjg@chromium.org>
> > > > Cc: Jaehoon Chung <jh80.chung@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 ?

If the compatibility failed, it will still invoke the calibration to
get the correct value. Just that its in the cost of boot time.

At same time, I am rethinking another email thread on suggesting to put
this into common dw_mmc code. Here are my proposal

1. Put back the correct default value for DT ABI compatiblity
2. Adding a node to enable calibration. If node not exist, calibration
will disabled as default.
3. Some common calibration algo will go into dw_mmc.c
4. platform specific such as on configuring the clksel and smplsel will
be part of platform specific dw_mmc file such as socfpga_dw_mmc.c

This hope will work for both everyone especially for socfpga and exynos
platform. Any thoughts?

Thanks
Chin Liang