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

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

Hi Marek,


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

> 
> > +			}
> > +		}
> > +	}
> > +	return -EINVAL;
> > +}
> > +
> > +/*
> > + * This function determines the largest rectangle filled with 1's
> > and
> > returns + * the middle. This functon can be optimized, for example
> > using
> > the algorithm + * from
> > http://www.drdobbs.com/database/the-maximal-rectangle-problem/18441
> > 0529 +
> > * It currently takes less than 1ms, while creating the input data
> > takes
> > ~5ms + * so there is not a real need to optimize it.
> > + */
> > +static int socfpga_dwmci_find_calibration_point(
> > +	unsigned char
> > cal_results[SOCFPGA_SD_DRVSEL][SOCFPGA_SD_SMPLSEL],
> > +	unsigned int sum, unsigned int *cal_row, unsigned int
> > *cal_col)
> > +{
> > +	/* Structure containing a rectangle candidate */
> > +	struct rect_cand_t {
> > +		unsigned char height;
> > +		unsigned char width;
> > +		unsigned short area;
> > +	};
> > +
> > +	/* Array with the rectangle candidates */
> > +	struct rect_cand_t rect_cands[SOCFPGA_SD_DRVSEL *
> > SOCFPGA_SD_SMPLSEL];
> > +	struct rect_cand_t tmp;
> > +	unsigned char cr_rect_cand = 0;
> > +	unsigned char height, width, k;
> > +
> > +	/* No solution if all combinations fail */
> > +	if (sum == 0)
> > +		return -EINVAL;
> > +
> > +	/* Simple solution if all combinations pass */
> > +	if (sum == (SOCFPGA_SD_DRVSEL * SOCFPGA_SD_SMPLSEL)) {
> > +		*cal_row = (SOCFPGA_SD_DRVSEL - 1) / 2;
> > +		*cal_col = (SOCFPGA_SD_SMPLSEL - 1) / 2;
> > +		return 0;
> > +	}
> > +
> > +	/*
> > +	 * Create list of all possible sub-rectangles, in
> > descending area
> > +	 * order
> > +	 */
> > +	for (height = SOCFPGA_SD_DRVSEL; height >= 1; height--) {
> > +		for (width = SOCFPGA_SD_SMPLSEL; width >= 1; width
> > --) {
> > +			/* Add a new rectangle candidate */
> > +			rect_cands[cr_rect_cand].height = height;
> > +			rect_cands[cr_rect_cand].width = width;
> > +			rect_cands[cr_rect_cand].area = height *
> > width;
> > +			cr_rect_cand++;
> > +
> > +			/* First candidate it always in the right
> > position */
> > +			if (cr_rect_cand == 1)
> > +				continue;
> > +
> > +			/*
> > +			 * Put the candidate in right location to
> > maintain
> > +			 * descending order
> > +			 */
> > +			for (k = cr_rect_cand - 1; k > 1; k--) {
> > +				if (rect_cands[k-1].area <
> > rect_cands[k].area) {
> > +					tmp = rect_cands[k-1];
> > +					rect_cands[k-1] =
> > rect_cands[k];
> > +					rect_cands[k] = tmp;
> > +				} else {
> > +					break;
> 
> Indent here as well ...
> if (!cond)
>  break;
> 
> do your job here.

Cool, this will save indentation here :)
Will update this.

> > +				}
> > +			}
> > +		}
> > +	}
> > +
> > +	/* Try to fit the rectangle candidates, in descending area
> > order */
> > +	for (k = 0; k < SOCFPGA_SD_DRVSEL * SOCFPGA_SD_SMPLSEL;
> > k++) {
> > +		if (!socfpga_dwmci_find_row_col_fit_rectangle(
> > +						rect_cands[k].widt
> > h,
> > +						rect_cands[k].heig
> > ht,
> > +						cal_results,
> > cal_row, cal_col))
> > +			return 0;
> > +	}
> > +
> > +	/* We could not fit any rectangle - return failure */
> > +	return -EINVAL;
> > +}
> > +
> > +static void socfpga_dwmci_set_clksel(struct dwmci_host *host)
> >  {
> >  	struct dwmci_socfpga_priv_data *priv = host->priv;
> > 
> 
> [...]
> 
> >  static int socfpga_dwmci_of_probe(const void *blob, int node,
> > const int
> > idx) {
> >  	/* FIXME: probe from DT eventually too/ */
> > @@ -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.

Thanks
Chin Liang


> 
> >  	host->priv = priv;
> > 
> >  	return add_dwmci(host, host->bus_hz, 400000);