Skip to contentSkip to conversation

mmc: socfpga_dw_mmc: Enable calibration for drvsel and smplsel

PATCH Superseded Chin Liang See 5 participants 18 messages Nov 27, 2015 – Jan 27, 2016 MMC, SD and SDIO

lore
Share & tools
lore

Chin Liang See

author
#0 lore raw

PATCH v4 1/1 mmc: socfpga_dw_mmc: Enable calibration for drvsel and smplsel

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 <[email protected]>
Cc Dinh Nguyen <[email protected]>
Cc Dinh Nguyen <[email protected]>
Cc Pavel Machek <[email protected]>
Cc Marek Vasut <[email protected]>
Cc Stefan Roese <[email protected]>
Cc Pantelis Antoniou <[email protected]>
Cc Simon Glass <[email protected]>
Cc Jaehoon Chung <[email protected]>

---
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(-)
drivers/mmc/socfpga_dw_mmc.c +205 -3
index 2bd0ebd..a8e2660 100644
@@ -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);
Signature · 1 line
2.2.0
#1 lore raw

PATCH v4 1/1 mmc: socfpga_dw_mmc: Enable calibration for drvsel and smplsel

Hi,

Quoted 22 lines · 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 <[email protected]>
Cc Dinh Nguyen <[email protected]>
Cc Dinh Nguyen <[email protected]>
Cc Pavel Machek <[email protected]>
Cc Marek Vasut <[email protected]>
Cc Stefan Roese <[email protected]>
Cc Pantelis Antoniou <[email protected]>
Cc Simon Glass <[email protected]>
Cc Jaehoon Chung <[email protected]>

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

Quoted 3 lines
---
 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

#2 lore raw

PATCH v4 1/1 mmc: socfpga_dw_mmc: Enable calibration for drvsel and smplsel

Quoted 27 lines · Simon Glass

Hi,

Quoted 24 lines · 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 <[email protected]>
Cc Dinh Nguyen <[email protected]>
Cc Dinh Nguyen <[email protected]>
Cc Pavel Machek <[email protected]>
Cc Marek Vasut <[email protected]>
Cc Stefan Roese <[email protected]>
Cc Pantelis Antoniou <[email protected]>
Cc Simon Glass <[email protected]>
Cc Jaehoon Chung <[email protected]>

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

Quoted 5 lines
Quoted 3 lines
 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

#3 lore raw

PATCH v4 1/1 mmc: socfpga_dw_mmc: Enable calibration for drvsel and smplsel

Hi Marek,

Quoted 32 lines · Marek Vasut
Quoted 31 lines · Simon Glass

Hi,

Quoted 27 lines · 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 <[email protected]>
Cc Dinh Nguyen <[email protected]>
Cc Dinh Nguyen <[email protected]>
Cc Pavel Machek <[email protected]>
Cc Marek Vasut <[email protected]>
Cc Stefan Roese <[email protected]>
Cc Pantelis Antoniou <[email protected]>
Cc Simon Glass <[email protected]>
Cc Jaehoon Chung <[email protected]>

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

Quoted 7 lines
Quoted 1 line

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

#4 lore raw

PATCH v4 1/1 mmc: socfpga_dw_mmc: Enable calibration for drvsel and smplsel

Quoted 37 lines · Simon Glass

Hi Marek,

Quoted 34 lines · Marek Vasut
Quoted 32 lines · Simon Glass

Hi,

Quoted 24 lines · 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 <[email protected]>
Cc Dinh Nguyen <[email protected]>
Cc Dinh Nguyen <[email protected]>
Cc Pavel Machek <[email protected]>
Cc Marek Vasut <[email protected]>
Cc Stefan Roese <[email protected]>
Cc Pantelis Antoniou <[email protected]>
Cc Simon Glass <[email protected]>
Cc Jaehoon Chung <[email protected]>

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

Quoted 3 lines
 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?

Quoted 8 lines
Quoted 5 lines

*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

#5 lore raw

PATCH v4 1/1 mmc: socfpga_dw_mmc: Enable calibration for drvsel and smplsel

Hi Marek,

Quoted 44 lines · Marek Vasut
Quoted 43 lines · Simon Glass

Hi Marek,

Quoted 39 lines · Marek Vasut
Quoted 27 lines · Simon Glass

Hi,

Quoted 24 lines · 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 <[email protected]>
Cc Dinh Nguyen <[email protected]>
Cc Dinh Nguyen <[email protected]>
Cc Pavel Machek <[email protected]>
Cc Marek Vasut <[email protected]>
Cc Stefan Roese <[email protected]>
Cc Pantelis Antoniou <[email protected]>
Cc Simon Glass <[email protected]>
Cc Jaehoon Chung <[email protected]>

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

Quoted 5 lines
Quoted 3 lines
 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.

Quoted 5 lines
Quoted 2 lines

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

#6 lore raw

PATCH v4 1/1 mmc: socfpga_dw_mmc: Enable calibration for drvsel and smplsel

Quoted 47 lines · Simon Glass

Hi Marek,

Quoted 44 lines · Marek Vasut
Quoted 43 lines · Simon Glass

Hi Marek,

Quoted 39 lines · Marek Vasut
Quoted 27 lines · Simon Glass

Hi,

Quoted 24 lines · 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 <[email protected]>
Cc Dinh Nguyen <[email protected]>
Cc Dinh Nguyen <[email protected]>
Cc Pavel Machek <[email protected]>
Cc Marek Vasut <[email protected]>
Cc Stefan Roese <[email protected]>
Cc Pantelis Antoniou <[email protected]>
Cc Simon Glass <[email protected]>
Cc Jaehoon Chung <[email protected]>

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

Quoted 5 lines
Quoted 3 lines
 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.

Quoted 7 lines
Quoted 5 lines
Quoted 2 lines

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

#7 lore raw

PATCH v4 1/1 mmc: socfpga_dw_mmc: Enable calibration for drvsel and smplsel

In reply to Chin Liang See
Quoted 74 lines · 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 <[email protected]>
Cc Dinh Nguyen <[email protected]>
Cc Dinh Nguyen <[email protected]>
Cc Pavel Machek <[email protected]>
Cc Marek Vasut <[email protected]>
Cc Stefan Roese <[email protected]>
Cc Pantelis Antoniou <[email protected]>
Cc Simon Glass <[email protected]>
Cc Jaehoon Chung <[email protected]>

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

drivers/mmc/socfpga_dw_mmc.c +3 -1
index 2bd0ebd..a8e2660 100644
@@ -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.
+ */

drivers/mmc/socfpga_dw_mmc.c +25 -0
+#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]) {

drivers/mmc/socfpga_dw_mmc.c struct dwmci_socfpga_priv_data { · line 68

Quoted 10 lines
drivers/mmc/socfpga_dw_mmc.c +10 -0
+						ok = 0;
+						break;
+					}
+				}
+			}
+
+			/*
+			 * Return 'middle' of rectangle in case of
+			 * success
+			 */

I think you can refactor this indentation horror some more, right ?

drivers/mmc/socfpga_dw_mmc.c struct dwmci_socfpga_priv_data { · line 79

Quoted 11 lines
drivers/mmc/socfpga_dw_mmc.c +11 -0
+			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.

Quoted 59 lines
drivers/mmc/socfpga_dw_mmc.c +8 -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.
+ */

drivers/mmc/socfpga_dw_mmc.c +45 -0
+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;
+
+			/*

drivers/mmc/socfpga_dw_mmc.c struct dwmci_socfpga_priv_data { · line 147

Quoted 10 lines
drivers/mmc/socfpga_dw_mmc.c +10 -0
+			 * 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.

Quoted 22 lines
drivers/mmc/socfpga_dw_mmc.c +19 -0
+				}
+			}
+		}
+	}
+
+	/* 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;

[...]

drivers/mmc/socfpga_dw_mmc.c static int socfpga_dwmci_of_probe(const void *blob, int · line 308

Quoted 11 lines

static int socfpga_dwmci_of_probe(const void *blob, int node, const int
idx) {

drivers/mmc/socfpga_dw_mmc.c +2 -2
 	/* 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.

Quoted 3 lines
drivers/mmc/socfpga_dw_mmc.c +0 -0
 	host->priv = priv;
 
 	return add_dwmci(host, host->bus_hz, 400000);
author
#8 lore raw

PATCH v4 1/1 mmc: socfpga_dw_mmc: Enable calibration for drvsel and smplsel

Hi Marek,

Quoted 90 lines · Marek Vasut
Quoted 89 lines · 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 <[email protected]>
Cc Dinh Nguyen <[email protected]>
Cc Dinh Nguyen <[email protected]>
Cc Pavel Machek <[email protected]>
Cc Marek Vasut <[email protected]>
Cc Stefan Roese <[email protected]>
Cc Pantelis Antoniou <[email protected]>
Cc Simon Glass <[email protected]>
Cc Jaehoon Chung <[email protected]>

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

drivers/mmc/socfpga_dw_mmc.c +3 -1
index 2bd0ebd..a8e2660 100644
@@ -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.
+ */

drivers/mmc/socfpga_dw_mmc.c +4 -0
+#define SOCFPGA_SD_DRVSEL  7
+#define SOCFPGA_SD_SMPLSEL 8
+
+static int socfpga_dwmci_find_row_col_fit_rectangle(unsigned

rect_width,

drivers/mmc/socfpga_dw_mmc.c +2 -0
+	unsigned rect_height,
+	unsigned char

cal_results[SOCFPGA_SD_DRVSEL][SOCFPGA_SD_SMPLSEL],

drivers/mmc/socfpga_dw_mmc.c +6 -0
+	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);

drivers/mmc/socfpga_dw_mmc.c +10 -0
+	     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++) {

drivers/mmc/socfpga_dw_mmc.c +1 -0
+				for (add_row = 0; add_row <

rect_height;

drivers/mmc/socfpga_dw_mmc.c +4 -0
+				     add_row++) {
+					if (!cal_results[start_row
+ add_row]
+					    [start_col + add_col])

{

drivers/mmc/socfpga_dw_mmc.c +4 -0
+						ok = 0;
+						break;
+					}
+				}
Quoted 9 lines

drivers/mmc/socfpga_dw_mmc.c struct dwmci_socfpga_priv_data { · line 68

Quoted 6 lines
drivers/mmc/socfpga_dw_mmc.c +6 -0
+			}
+
+			/*
+			 * 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.

Quoted 9 lines
Quoted 9 lines
drivers/mmc/socfpga_dw_mmc.c +8 -0
+			if (ok) {
+				if (rect_width > 1)
+					rect_width--;
+
+				if (rect_height > 1)
+					rect_height--;
+
+				*cal_row = start_row +

(rect_height / 2);

Quoted 8 lines

drivers/mmc/socfpga_dw_mmc.c

Quoted 4 lines
drivers/mmc/socfpga_dw_mmc.c +1 -0
+				*cal_col = start_col + (rect_width

/ 2);

drivers/mmc/socfpga_dw_mmc.c +2 -0
+
+				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

Quoted 89 lines
Quoted 81 lines
drivers/mmc/socfpga_dw_mmc.c +8 -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/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.
+ */

drivers/mmc/socfpga_dw_mmc.c +2 -0
+static int socfpga_dwmci_find_calibration_point(
+	unsigned char

cal_results[SOCFPGA_SD_DRVSEL][SOCFPGA_SD_SMPLSEL],

drivers/mmc/socfpga_dw_mmc.c +1 -0
+	unsigned int sum, unsigned int *cal_row, unsigned int

*cal_col)

drivers/mmc/socfpga_dw_mmc.c +10 -0
+{
+	/* 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];

drivers/mmc/socfpga_dw_mmc.c +17 -0
+	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

drivers/mmc/socfpga_dw_mmc.c +8 -1
+	 * 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;

drivers/mmc/socfpga_dw_mmc.c +3 -0
+			cr_rect_cand++;
+
+			/* First candidate it always in the right

position */

drivers/mmc/socfpga_dw_mmc.c +5 -0
+			if (cr_rect_cand == 1)
+				continue;
+
+			/*
+			 * Put the candidate in right location to

maintain

drivers/mmc/socfpga_dw_mmc.c +4 -0
+			 * descending order
+			 */
+			for (k = cr_rect_cand - 1; k > 1; k--) {
+				if (rect_cands[k-1].area <

rect_cands[k].area) {

drivers/mmc/socfpga_dw_mmc.c +2 -0
+					tmp = rect_cands[k-1];
+					rect_cands[k-1] =

rect_cands[k];

drivers/mmc/socfpga_dw_mmc.c struct dwmci_socfpga_priv_data { · line 147

Quoted 3 lines
drivers/mmc/socfpga_dw_mmc.c +3 -0
+					rect_cands[k] = tmp;
+				} else {
+					break;

Indent here as well ...
if (!cond)
break;

Quoted 1 line

do your job here.

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

Quoted 40 lines
Quoted 27 lines
drivers/mmc/socfpga_dw_mmc.c +6 -0
+				}
+			}
+		}
+	}
+
+	/* Try to fit the rectangle candidates, in descending area

order */

drivers/mmc/socfpga_dw_mmc.c +1 -0
+	for (k = 0; k < SOCFPGA_SD_DRVSEL * SOCFPGA_SD_SMPLSEL;

k++) {

drivers/mmc/socfpga_dw_mmc.c +2 -0
+		if (!socfpga_dwmci_find_row_col_fit_rectangle(
+						rect_cands[k].widt

h,

drivers/mmc/socfpga_dw_mmc.c +1 -0
+						rect_cands[k].heig

ht,

drivers/mmc/socfpga_dw_mmc.c +1 -0
+						cal_results,

cal_row, cal_col))

drivers/mmc/socfpga_dw_mmc.c +8 -0
+			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;

[...]

Quoted 10 lines

static int socfpga_dwmci_of_probe(const void *blob, int node,
const int
idx) {

drivers/mmc/socfpga_dw_mmc.c +0 -0
 	/* 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);
Quoted 8 lines

drivers/mmc/socfpga_dw_mmc.c static int socfpga_dwmci_of_probe(const void *blob, int node, const int idx)

Quoted 5 lines
drivers/mmc/socfpga_dw_mmc.c +2 -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

Quoted 4 lines
Quoted 4 lines
drivers/mmc/socfpga_dw_mmc.c +0 -0
 	host->priv = priv;
 
 	return add_dwmci(host, host->bus_hz, 400000);
#9 lore raw

PATCH v4 1/1 mmc: socfpga_dw_mmc: Enable calibration for drvsel and smplsel

Quoted 1 line · Chin Liang See

Hi Marek,

Hi!

Quoted 128 lines · Marek Vasut
Quoted 98 lines · Chin Liang See
Quoted 97 lines

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 <[email protected]>
Cc Dinh Nguyen <[email protected]>
Cc Dinh Nguyen <[email protected]>
Cc Pavel Machek <[email protected]>
Cc Marek Vasut <[email protected]>
Cc Stefan Roese <[email protected]>
Cc Pantelis Antoniou <[email protected]>
Cc Simon Glass <[email protected]>
Cc Jaehoon Chung <[email protected]>

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

drivers/mmc/socfpga_dw_mmc.c +1 -0
index 2bd0ebd..a8e2660 100644
@@ -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 =

drivers/mmc/socfpga_dw_mmc.c +2 -1
 		(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.
+ */

drivers/mmc/socfpga_dw_mmc.c +4 -0
+#define SOCFPGA_SD_DRVSEL  7
+#define SOCFPGA_SD_SMPLSEL 8
+
+static int socfpga_dwmci_find_row_col_fit_rectangle(unsigned

rect_width,

drivers/mmc/socfpga_dw_mmc.c +2 -0
+	unsigned rect_height,
+	unsigned char

cal_results[SOCFPGA_SD_DRVSEL][SOCFPGA_SD_SMPLSEL],

drivers/mmc/socfpga_dw_mmc.c +6 -0
+	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);

drivers/mmc/socfpga_dw_mmc.c +10 -0
+	     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++) {

drivers/mmc/socfpga_dw_mmc.c +1 -0
+				for (add_row = 0; add_row <

rect_height;

drivers/mmc/socfpga_dw_mmc.c +4 -0
+				     add_row++) {
+					if (!cal_results[start_row
+ add_row]
+					    [start_col + add_col])

{

drivers/mmc/socfpga_dw_mmc.c +4 -0
+						ok = 0;
+						break;
+					}
+				}
Quoted 8 lines

drivers/mmc/socfpga_dw_mmc.c struct dwmci_socfpga_priv_data { · line 68

Quoted 6 lines
drivers/mmc/socfpga_dw_mmc.c +6 -0
+			}
+
+			/*
+			 * 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.

Quoted 9 lines
Quoted 9 lines
drivers/mmc/socfpga_dw_mmc.c +8 -0
+			if (ok) {
+				if (rect_width > 1)
+					rect_width--;
+
+				if (rect_height > 1)
+					rect_height--;
+
+				*cal_row = start_row +

(rect_height / 2);

Quoted 8 lines

drivers/mmc/socfpga_dw_mmc.c

Quoted 4 lines
drivers/mmc/socfpga_dw_mmc.c +1 -0
+				*cal_col = start_col + (rect_width

/ 2);

drivers/mmc/socfpga_dw_mmc.c +2 -0
+
+				return 0;

For example this condition can be inverted and it'd shave off one level
of indent.

Quoted 1 line

Ok let me take a look when doing the house keeping

That's really help, thanks/

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/mmc/socfpga_dw_mmc.c +5 -0
+			}
+		}
+	}
+	return -EINVAL;
+}

[...]

Quoted 18 lines
Quoted 8 lines
Quoted 8 lines
drivers/mmc/socfpga_dw_mmc.c +0 -0
@@ -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
Quoted 9 lines
Quoted 6 lines
drivers/mmc/socfpga_dw_mmc.c +2 -2
 / 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.

Quoted 3 lines

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 ?

author
#10 lore raw

PATCH v4 1/1 mmc: socfpga_dw_mmc: Enable calibration for drvsel and smplsel

Quoted 186 lines · Marek Vasut
Quoted 1 line · Chin Liang See

Hi Marek,

Hi!

Quoted 140 lines · Marek Vasut
Quoted 109 lines
Quoted 107 lines · 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 <[email protected]>
Cc Dinh Nguyen <[email protected]>
Cc Dinh Nguyen <[email protected]>
Cc Pavel Machek <[email protected]>
Cc Marek Vasut <[email protected]>
Cc Stefan Roese <[email protected]>
Cc Pantelis Antoniou <[email protected]>
Cc Simon Glass <[email protected]>
Cc Jaehoon Chung <[email protected]>

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

drivers/mmc/socfpga_dw_mmc.c +1 -0
index 2bd0ebd..a8e2660 100644
@@ -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
=

drivers/mmc/socfpga_dw_mmc.c +2 -1
 		(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.
+ */

drivers/mmc/socfpga_dw_mmc.c +4 -0
+#define SOCFPGA_SD_DRVSEL  7
+#define SOCFPGA_SD_SMPLSEL 8
+
+static int socfpga_dwmci_find_row_col_fit_rectangle(unsigned

rect_width,

drivers/mmc/socfpga_dw_mmc.c +2 -0
+	unsigned rect_height,
+	unsigned char

cal_results[SOCFPGA_SD_DRVSEL][SOCFPGA_SD_SMPLSEL],

drivers/mmc/socfpga_dw_mmc.c +6 -0
+	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);

drivers/mmc/socfpga_dw_mmc.c +3 -0
+	     start_col++) {
+		for (start_row = 0;
+		     start_row < (SOCFPGA_SD_DRVSEL -

rect_height
+ 1);

drivers/mmc/socfpga_dw_mmc.c +5 -0
+		     start_row++) {
+			unsigned ok = 1;
+			unsigned add_col, add_row;
+
+			/* Determine if the rectangle fits

here */

drivers/mmc/socfpga_dw_mmc.c +1 -0
+			for (add_col = 0; (add_col <

rect_width);
add_col++) {

drivers/mmc/socfpga_dw_mmc.c +1 -0
+				for (add_row = 0; add_row <

rect_height;

drivers/mmc/socfpga_dw_mmc.c +2 -0
+				     add_row++) {
+					if

(!cal_results[start_row
+ add_row]

drivers/mmc/socfpga_dw_mmc.c +1 -0
+					    [start_col +

add_col])
{

drivers/mmc/socfpga_dw_mmc.c +6 -0
+						ok = 0;
+						break;
+					}
+				}
+			}
+
Quoted 8 lines

drivers/mmc/socfpga_dw_mmc.c

Quoted 5 lines
drivers/mmc/socfpga_dw_mmc.c +2 -0
+			/*
+			 * Return 'middle' of rectangle in

case of

drivers/mmc/socfpga_dw_mmc.c +2 -0
+			 * success
+			 */

I think you can refactor this indentation horror some more, right ?

Ok, let me use shorter variable to avoid going next line.

Quoted 10 lines
Quoted 10 lines
drivers/mmc/socfpga_dw_mmc.c +8 -0
+			if (ok) {
+				if (rect_width > 1)
+					rect_width--;
+
+				if (rect_height > 1)
+					rect_height--;
+
+				*cal_row = start_row +

(rect_height / 2);

drivers/mmc/socfpga_dw_mmc.c +1 -0
+				*cal_col = start_col +
Quoted 8 lines

drivers/mmc/socfpga_dw_mmc.c

Quoted 4 lines

(rect_width
/ 2);

drivers/mmc/socfpga_dw_mmc.c +2 -0
+
+				return 0;

For example this condition can be inverted and it'd shave off one level
of indent.

Quoted 1 line

Ok let me take a look when doing the house keeping

That's really help, thanks/

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/mmc/socfpga_dw_mmc.c +5 -0
+			}
+		}
+	}
+	return -EINVAL;
+}

[...]

Quoted 23 lines
Quoted 10 lines
Quoted 10 lines
drivers/mmc/socfpga_dw_mmc.c +0 -0
@@ -102,8 +304,8 @@ static int socfpga_dwmci_of_probe(const

void
*blob, int
node, const int idx) host->bus_hz = clk;

drivers/mmc/socfpga_dw_mmc.c +0 -0
 	host->fifoth_val = MSIZE(0x2) |
 
 		RX_WMARK(fifo_depth / 2 - 1) |

TX_WMARK(fifo_depth

Quoted 12 lines
Quoted 9 lines

/ 2);

drivers/mmc/socfpga_dw_mmc.c +0 -1
-	priv->drvsel = fdtdec_get_uint(blob, node, "drvsel",

3);

drivers/mmc/socfpga_dw_mmc.c +0 -1
-	priv->smplsel = fdtdec_get_uint(blob, node, "smplsel",

0);

drivers/mmc/socfpga_dw_mmc.c +1 -0
+	priv->drvsel = fdtdec_get_uint(blob, node, "drvsel",

0xf);

drivers/mmc/socfpga_dw_mmc.c +1 -0
+	priv->smplsel = fdtdec_get_uint(blob, node, "smplsel",

0xf);

This breaks multiple boards, since it misconfigures the default values.

Quoted 5 lines

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.

Quoted 5 lines

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

#11 lore raw

PATCH v4 1/1 mmc: socfpga_dw_mmc: Enable calibration for drvsel and smplsel

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

[...]

Quoted 8 lines
Quoted 5 lines

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.

Quoted 2 lines

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.

Quoted 3 lines

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.

Quoted 3 lines

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

Quoted 5 lines

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

Thanks
Chin Liang

Best regards,
Marek Vasut

author
#12 lore raw

PATCH v4 1/1 mmc: socfpga_dw_mmc: Enable calibration for drvsel and smplsel

Quoted 29 lines · Marek Vasut

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

[...]

Quoted 11 lines
Quoted 7 lines

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.

Quoted 3 lines

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.

Quoted 4 lines

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.

Quoted 6 lines

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

Quoted 16 lines
Quoted 4 lines

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

Quoted 6 lines

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

Thanks
Chin Liang

Best regards,
Marek Vasut

#13 lore raw

PATCH v4 1/1 mmc: socfpga_dw_mmc: Enable calibration for drvsel and smplsel

Quoted 30 lines · Chin Liang See
Quoted 29 lines · Marek Vasut

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

[...]

Quoted 11 lines
Quoted 7 lines

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.

Quoted 3 lines

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.

Quoted 4 lines

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.

Quoted 8 lines
Quoted 5 lines

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

#14 lore raw

PATCH v4 1/1 mmc: socfpga_dw_mmc: Enable calibration for drvsel and smplsel

In reply to Marek Vasut

Hi!

Quoted 16 lines
Quoted 8 lines
Quoted 5 lines

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.

Quoted 2 lines

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.

Quoted 7 lines
Quoted 3 lines

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

Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
#15 lore raw

PATCH v4 1/1 mmc: socfpga_dw_mmc: Enable calibration for drvsel and smplsel

In reply to Chin Liang See
Quoted 3 lines · 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.

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.

drivers/mmc/socfpga_dw_mmc.c struct dwmci_socfpga_priv_data { · line 40

Quoted 4 lines
drivers/mmc/socfpga_dw_mmc.c +4 -0
+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.

drivers/mmc/socfpga_dw_mmc.c static void socfpga_dwmci_clksel(struct dwmci_host *host) · line 206

Quoted 4 lines
drivers/mmc/socfpga_dw_mmc.c +4 -0
+	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.

drivers/mmc/socfpga_dw_mmc.c static void socfpga_dwmci_clksel(struct dwmci_host *host) · line 226

Quoted 4 lines
drivers/mmc/socfpga_dw_mmc.c +4 -0
+
+	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.

author
#16 lore raw

PATCH v4 1/1 mmc: socfpga_dw_mmc: Enable calibration for drvsel and smplsel

Quoted 21 lines · Trent Piepho
Quoted 3 lines · 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.

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.

Quoted 8 lines

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?

Quoted 9 lines

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.

Quoted 4 lines

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.

Quoted 13 lines

drivers/mmc/socfpga_dw_mmc.c struct dwmci_socfpga_priv_data { · line 40

Quoted 7 lines

+static int socfpga_dwmci_find_row_col_fit_rectangle(unsigned rect_width,

(quoted diff) +2 -0
+	unsigned rect_height,
+	unsigned char

cal_results[SOCFPGA_SD_DRVSEL][SOCFPGA_SD_SMPLSEL],

drivers/mmc/socfpga_dw_mmc.c +1 -0
+	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.

Quoted 7 lines

drivers/mmc/socfpga_dw_mmc.c static void socfpga_dwmci_clksel(struct dwmci_host *host) · line 206

Quoted 5 lines
drivers/mmc/socfpga_dw_mmc.c +4 -0
+	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

Quoted 14 lines

Review

Quoted 7 lines
(quoted diff) +2 -0
+
+	err = socfpga_dwmci_find_calibration_point(cal_results,

sum,

(quoted diff) +2 -1
+						   &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

#17 lore raw

PATCH v4 1/1 mmc: socfpga_dw_mmc: Enable calibration for drvsel and smplsel

Quoted 32 lines · Chin Liang See
Quoted 21 lines · Trent Piepho
Quoted 3 lines · 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.

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.

Quoted 8 lines

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.

Quoted 2 lines

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.

Quoted 10 lines
Quoted 8 lines

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?

Quoted 2 lines

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.

Quoted 8 lines
Quoted 4 lines

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

Quoted 45 lines
Quoted 12 lines

drivers/mmc/socfpga_dw_mmc.c struct dwmci_socfpga_priv_data { · line 40

Quoted 6 lines

+static int socfpga_dwmci_find_row_col_fit_rectangle(unsigned rect_width,

(quoted diff) +2 -0
+	unsigned rect_height,
+	unsigned char

cal_results[SOCFPGA_SD_DRVSEL][SOCFPGA_SD_SMPLSEL],

drivers/mmc/socfpga_dw_mmc.c +1 -0
+	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.

Quoted 7 lines

drivers/mmc/socfpga_dw_mmc.c static void socfpga_dwmci_clksel(struct dwmci_host *host) · line 206

Quoted 5 lines
drivers/mmc/socfpga_dw_mmc.c +4 -0
+	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

Quoted 14 lines

Review

Quoted 7 lines
(quoted diff) +2 -0
+
+	err = socfpga_dwmci_find_calibration_point(cal_results,

sum,

(quoted diff) +2 -1
+						   &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.

Quoted 1 line

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.