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

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

[Chin Liang See]

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
+			 */
+			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;
+			}
+		}
+	}
+	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/184410529
+ * 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;
+				}
+			}
+		}
+	}
+
+	/* 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].width,
+						rect_cands[k].height,
+						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;
 
@@ -46,6 +184,70 @@ static void socfpga_dwmci_clksel(struct dwmci_host *host)
 		CLKMGR_PERPLLGRP_EN_SDMMCCLK_MASK);
 }
 
+/*
+ * This function calibrates the drvsel and smplsel by trying all possible
+ * values and selecting the best combnation
+ */
+void socfpga_dwmci_calibrate(struct dwmci_host *host)
+{
+	int err = 0;
+	unsigned char cal_results[SOCFPGA_SD_DRVSEL][SOCFPGA_SD_SMPLSEL];
+	unsigned int sum = 0;
+	unsigned int row, col;
+
+	struct mmc *mmc = host->mmc;
+	struct dwmci_socfpga_priv_data *priv = host->priv;
+
+	printf("%s: Calibration started.\n", __func__);
+
+	for (row = 0; row < SOCFPGA_SD_DRVSEL; row++) {
+		for (col = 0; col < SOCFPGA_SD_SMPLSEL; col++) {
+			priv->drvsel = row + 1;
+			priv->smplsel = col;
+			socfpga_dwmci_set_clksel(host);
+			cal_results[row][col] = !(mmc_set_blocklen(mmc,
+						  mmc->read_bl_len));
+			sum += cal_results[row][col];
+		}
+	}
+
+#if DEBUG
+	debug("%s: Calibration raw data:\n", __func__);
+	for (row = 0; row < SOCFPGA_SD_DRVSEL; row++) {
+		debug("\t");
+		for (col = 0; col < SOCFPGA_SD_SMPLSEL; col++)
+			debug("%d ", cal_results[row][col]);
+		debug("\n");
+	}
+#endif
+
+	err = socfpga_dwmci_find_calibration_point(cal_results, sum,
+						   &priv->drvsel,
+						   &priv->smplsel);
+
+	if (err) {
+		printf("%s: Calibration failed.\n", __func__);
+		hang();
+	} else
+		printf("%s: Calibration passed: drvsel = %d "
+		      "smplsel = %d.\n", __func__, priv->drvsel, priv->smplsel);
+
+	socfpga_dwmci_set_clksel(host);
+
+	/* re-write in case accidentally overwrite with junk */
+	mmc_set_blocklen(mmc, mmc->read_bl_len);
+}
+
+static void socfpga_dwmci_clksel(struct dwmci_host *host)
+{
+	struct dwmci_socfpga_priv_data *priv = host->priv;
+
+	if (priv->drvsel == 0xf || priv->smplsel == 0xf)
+		socfpga_dwmci_calibrate(host);
+	else
+		socfpga_dwmci_set_clksel(host);
+}
+
 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);
 	host->priv = priv;
 
 	return add_dwmci(host, host->bus_hz, 400000);
-- 
2.2.0

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

[Simon Glass]

Hi,

On 26 November 2015 at 23:22, Chin Liang See <clsee@altera.com> 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(-)

Should this code go in the generic dw_mmc.c file instead?

Regards,
Simon

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

[Marek Vasut]

On Friday, November 27, 2015 at 07:36:27 PM, Simon Glass wrote:
> Hi,
> 
> On 26 November 2015 at 23:22, Chin Liang See <clsee@altera.com> 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(-)
> 
> Should this code go in the generic dw_mmc.c file instead?

*Marek grabs popcorn*

You might want to read the discussion(s) below the previous versions of the 
patch, but TLDR, samsung doesn't want the calibration code to interfere with
their driver it seems.

Best regards,
Marek Vasut

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

[Simon Glass]

Hi Marek,

On 27 November 2015 at 11:41, Marek Vasut <marex@denx.de> wrote:
> On Friday, November 27, 2015 at 07:36:27 PM, Simon Glass wrote:
>> Hi,
>>
>> On 26 November 2015 at 23:22, Chin Liang See <clsee@altera.com> 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(-)
>>
>> Should this code go in the generic dw_mmc.c file instead?
>
> *Marek grabs popcorn*
>
> You might want to read the discussion(s) below the previous versions of the
> patch, but TLDR, samsung doesn't want the calibration code to interfere with
> their driver it seems.

OK, fair enough. Let's see what happens when the next person wants to
add this feature.

Regards,
Simon

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

[Marek Vasut]

On Sunday, November 29, 2015 at 02:59:37 AM, Simon Glass wrote:
> Hi Marek,
> 
> On 27 November 2015 at 11:41, Marek Vasut <marex@denx.de> wrote:
> > On Friday, November 27, 2015 at 07:36:27 PM, Simon Glass wrote:
> >> Hi,
> >> 
> >> On 26 November 2015 at 23:22, Chin Liang See <clsee@altera.com> 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(-)
> >> 
> >> Should this code go in the generic dw_mmc.c file instead?
> > 
> > *Marek grabs popcorn*
> > 
> > You might want to read the discussion(s) below the previous versions of
> > the patch, but TLDR, samsung doesn't want the calibration code to
> > interfere with their driver it seems.
> 
> OK, fair enough. Let's see what happens when the next person wants to
> add this feature.

I'm reluctant to grab this patch altogether ... what do you think about this 
functionality ?

Best regards,
Marek Vasut

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

[Marek Vasut]

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 ?

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

> +			}
> +		}
> +	}
> +	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/184410529 +
> * 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.

> +				}
> +			}
> +		}
> +	}
> +
> +	/* 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].width,
> +						rect_cands[k].height,
> +						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.

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

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

[Simon Glass]

Hi Marek,

On 28 November 2015 at 18:05, Marek Vasut <marex@denx.de> wrote:
> On Sunday, November 29, 2015 at 02:59:37 AM, Simon Glass wrote:
>> Hi Marek,
>>
>> On 27 November 2015 at 11:41, Marek Vasut <marex@denx.de> wrote:
>> > On Friday, November 27, 2015 at 07:36:27 PM, Simon Glass wrote:
>> >> Hi,
>> >>
>> >> On 26 November 2015 at 23:22, Chin Liang See <clsee@altera.com> 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(-)
>> >>
>> >> Should this code go in the generic dw_mmc.c file instead?
>> >
>> > *Marek grabs popcorn*
>> >
>> > You might want to read the discussion(s) below the previous versions of
>> > the patch, but TLDR, samsung doesn't want the calibration code to
>> > interfere with their driver it seems.
>>
>> OK, fair enough. Let's see what happens when the next person wants to
>> add this feature.
>
> I'm reluctant to grab this patch altogether ... what do you think about this
> functionality ?

It seems reasonable to me - high speed MMC requires it.

Regards,
Simon

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

[Marek Vasut]

On Sunday, November 29, 2015 at 09:19:14 PM, Simon Glass wrote:
> Hi Marek,
> 
> On 28 November 2015 at 18:05, Marek Vasut <marex@denx.de> wrote:
> > On Sunday, November 29, 2015 at 02:59:37 AM, Simon Glass wrote:
> >> Hi Marek,
> >> 
> >> On 27 November 2015 at 11:41, Marek Vasut <marex@denx.de> wrote:
> >> > On Friday, November 27, 2015 at 07:36:27 PM, Simon Glass wrote:
> >> >> Hi,
> >> >> 
> >> >> On 26 November 2015 at 23:22, Chin Liang See <clsee@altera.com> 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(-)
> >> >> 
> >> >> Should this code go in the generic dw_mmc.c file instead?
> >> > 
> >> > *Marek grabs popcorn*
> >> > 
> >> > You might want to read the discussion(s) below the previous versions
> >> > of the patch, but TLDR, samsung doesn't want the calibration code to
> >> > interfere with their driver it seems.
> >> 
> >> OK, fair enough. Let's see what happens when the next person wants to
> >> add this feature.
> > 
> > I'm reluctant to grab this patch altogether ... what do you think about
> > this functionality ?
> 
> It seems reasonable to me - high speed MMC requires it.

In that case, it should go into common dw_mmc code .

Best regards,
Marek Vasut

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

[Chin Liang See]

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);

[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@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 ?

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

[Chin Liang See]

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

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

[Marek Vasut]

On Wednesday, December 02, 2015 at 06:52:17 AM, Chin Liang See wrote:

[...]

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

Hopefully the calibration does it's job then.

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

Having this in common code would make sense.

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

But "enable calibration" is not really a hardware property, so it shouldn't
be in the DT. The DT is purely a hardware description and only the smplsel
and drvsel fit into this, some "enable-calibration" does not.

> 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

OK

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

Best regards,
Marek Vasut

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

[Chin Liang See]

On Wed, 2015-12-02 at 15:39 +0100, Marek Vasut wrote:
> On Wednesday, December 02, 2015 at 06:52:17 AM, Chin Liang See wrote:
> 
> [...]
> 
> > > 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.
> 
> Hopefully the calibration does it's job then.
> 
> > At same time, I am rethinking another email thread on suggesting to
> > put
> > this into common dw_mmc code. Here are my proposal
> 
> Having this in common code would make sense.
> 
> > 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.
> 
> But "enable calibration" is not really a hardware property, so it
> shouldn't
> be in the DT. The DT is purely a hardware description and only the
> smplsel
> and drvsel fit into this, some "enable-calibration" does not.
> 

Sound reasonable. In this case, I can put this into KConfig where user
can configure this easily through menuconfig. 

Thanks
Chin Liang


> > 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
> 
> OK
> 
> > This hope will work for both everyone especially for socfpga and
> > exynos
> > platform. Any thoughts?
> > 
> > Thanks
> > Chin Liang
> 
> Best regards,
> Marek Vasut

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

[Marek Vasut]

On Wednesday, December 02, 2015 at 04:10:19 PM, Chin Liang See wrote:
> On Wed, 2015-12-02 at 15:39 +0100, Marek Vasut wrote:
> > On Wednesday, December 02, 2015 at 06:52:17 AM, Chin Liang See wrote:
> > 
> > [...]
> > 
> > > > 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.
> > 
> > Hopefully the calibration does it's job then.
> > 
> > > At same time, I am rethinking another email thread on suggesting to
> > > put
> > > this into common dw_mmc code. Here are my proposal
> > 
> > Having this in common code would make sense.
> > 
> > > 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.
> > 
> > But "enable calibration" is not really a hardware property, so it
> > shouldn't
> > be in the DT. The DT is purely a hardware description and only the
> > smplsel
> > and drvsel fit into this, some "enable-calibration" does not.
> 
> Sound reasonable. In this case, I can put this into KConfig where user
> can configure this easily through menuconfig.

OK, this would be much better.

Best regards,
Marek Vasut

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

[Pavel Machek]

Hi!
> > > 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.
> 
> Hopefully the calibration does it's job then.
> 
> > At same time, I am rethinking another email thread on suggesting to put
> > this into common dw_mmc code. Here are my proposal
> 
> Having this in common code would make sense.
> 
> > 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.
> 
> But "enable calibration" is not really a hardware property, so it shouldn't
> be in the DT. The DT is purely a hardware description and only the smplsel
> and drvsel fit into this, some "enable-calibration" does not.

Well... "enable calibration" is not hardware property, but "well-known
autocalibration works on this hardware" is.. Anyway, config option is
probably fine, too.

How does Linux solve this? It needs to do some calibration, too, no?
We should use whatever dts Linux uses...
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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

[Trent Piepho]

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.

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


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


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

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

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

[Chin Liang See]

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?

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


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


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

Thanks
Chin Liang

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