[U-Boot] [PATCH v2 1/2] arm: socfpga: mmc: Enable calibration for drvsel and smpsel

[U-Boot] [PATCH v2 1/2] arm: socfpga: mmc: Enable calibration for drvsel and smpsel

[Chin Liang See]

Enable SDMMC calibration to determine the best setting for
drvsel and smpsel. It will be triggered whenever there is
a change of card frequency and bus width. This is to ensure
reliable transmission between the controller and the card.

Signed-off-by: Chin Liang See <clsee@altera.com>
Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
Cc: Pavel Machek <pavel@denx.de>
Cc: Marek Vasut <marex@denx.de>
Cc: Stefan Roese <sr@denx.de>
---
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 |  194 ++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 187 insertions(+), 7 deletions(-)

diff --git a/drivers/mmc/socfpga_dw_mmc.c b/drivers/mmc/socfpga_dw_mmc.c
index eb69aed..0fbea49 100644
--- a/drivers/mmc/socfpga_dw_mmc.c
+++ b/drivers/mmc/socfpga_dw_mmc.c
@@ -11,25 +11,147 @@
 #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;
 static const struct socfpga_system_manager *system_manager_base =
 		(void *)SOCFPGA_SYSMGR_ADDRESS;
 
-static void socfpga_dwmci_clksel(struct dwmci_host *host)
+#define CAL_ROWS     7
+#define CAL_COLS     8
+
+int find_row_col_fit_rectangle(unsigned rect_width, unsigned rect_height,
+unsigned char cal_results[CAL_ROWS][CAL_COLS], 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 < (CAL_COLS - rect_width + 1);
+	     start_col++) {
+		for (start_row = 0; start_row < (CAL_ROWS - 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) && ok;
+			     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.
+ */
+int find_calibration_point(unsigned char cal_results[CAL_ROWS][CAL_COLS],
+unsigned int sum, unsigned int *cal_row, unsigned int *cal_col)
 {
-	unsigned int drvsel;
-	unsigned int smplsel;
+	/* 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[CAL_ROWS * CAL_COLS], 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 == (CAL_ROWS * CAL_COLS)) {
+		*cal_row = (CAL_ROWS - 1) / 2;
+		*cal_col = (CAL_COLS - 1) / 2;
+		return 0;
+	}
+
+	/*
+	 * Create list of all possible sub-rectangles, in descending area
+	 * order
+	 */
+	for (height = CAL_ROWS; height >= 1; height--) {
+		for (width = CAL_COLS; 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 < CAL_ROWS * CAL_COLS; k++) {
+		if (!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;
+}
+
+void socfpga_dwmmc_set_clksel(unsigned int drvsel, unsigned int smplsel)
+{
 	/* Disable SDMMC clock. */
 	clrbits_le32(&clock_manager_base->per_pll.en,
 		CLKMGR_PERPLLGRP_EN_SDMMCCLK_MASK);
 
-	/* Configures drv_sel and smpl_sel */
-	drvsel = CONFIG_SOCFPGA_DWMMC_DRVSEL;
-	smplsel = CONFIG_SOCFPGA_DWMMC_SMPSEL;
-
 	debug("%s: drvsel %d smplsel %d\n", __func__, drvsel, smplsel);
 	writel(SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel),
 		&system_manager_base->sdmmcgrp_ctrl);
@@ -42,6 +164,64 @@ static void socfpga_dwmci_clksel(struct dwmci_host *host)
 		CLKMGR_PERPLLGRP_EN_SDMMCCLK_MASK);
 }
 
+/*
+ * This functions calirates the drvsel and smplsel by trying all possible
+ * values and selecting the best combnation
+ */
+void socfpga_dwmmc_calibrate(struct dwmci_host *host)
+{
+	int err = 0;
+	unsigned char cal_results[CAL_ROWS][CAL_COLS];
+	unsigned int sum = 0;
+	unsigned int row, col, drvsel, smplsel;
+
+	struct mmc *mmc = (struct mmc *)host->mmc;
+	if (mmc == NULL) {
+		printf("struct mmc is NULL\n");
+		hang();
+	}
+
+	printf("%s: Calibration started.\n", __func__);
+
+	for (row = 0; row < CAL_ROWS; row++) {
+		for (col = 0; col < CAL_COLS; col++) {
+			drvsel = row + 1;
+			smplsel = col;
+			socfpga_dwmmc_set_clksel(drvsel, smplsel);
+			cal_results[row][col] = !(mmc_set_blocklen(mmc,
+						  mmc->read_bl_len));
+			sum += cal_results[row][col];
+		}
+	}
+
+	debug("%s: Calibration raw data:\n", __func__);
+	for (row = 0; row < CAL_ROWS; row++) {
+		debug("\t");
+		for (col = 0; col < CAL_COLS; col++)
+			debug("%d ", cal_results[row][col]);
+		debug("\n");
+	}
+
+	err = find_calibration_point(cal_results, sum, &drvsel, &smplsel);
+
+	if (err) {
+		printf("%s: Calibration failed.\n", __func__);
+		hang();
+	} else
+		printf("%s: Calibration passed: drvsel = %d "
+		       "smplsel = %d.\n", __func__, drvsel, smplsel);
+
+	socfpga_dwmmc_set_clksel(drvsel, smplsel);
+
+	/* re-write in case accidentally overwrite with junk */
+	mmc_set_blocklen(mmc, mmc->read_bl_len);
+}
+
+void socfpga_dwmci_clksel(struct dwmci_host *host)
+{
+	socfpga_dwmmc_calibrate(host);
+}
+
 int socfpga_dwmmc_init(u32 regbase, int bus_width, int index)
 {
 	struct dwmci_host *host;
-- 
1.7.7.4

[U-Boot] [PATCH v2 1/2] arm: socfpga: mmc: Enable calibration for drvsel and smpsel

[Chin Liang See]

Hi guys,

Any comment or ack for this patch?
Thanks

Chin Liang

On Thu, 2015-08-20 at 02:18 -0500, Chin Liang See wrote:
> Enable SDMMC calibration to determine the best setting for
> drvsel and smpsel. It will be triggered whenever there is
> a change of card frequency and bus width. This is to ensure
> reliable transmission between the controller and the card.
> 
> Signed-off-by: Chin Liang See <clsee@altera.com>
> Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
> Cc: Pavel Machek <pavel@denx.de>
> Cc: Marek Vasut <marex@denx.de>
> Cc: Stefan Roese <sr@denx.de>
> ---
> 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 |  194 ++++++++++++++++++++++++++++++++++++++++--
>  1 files changed, 187 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/mmc/socfpga_dw_mmc.c b/drivers/mmc/socfpga_dw_mmc.c
> index eb69aed..0fbea49 100644
> --- a/drivers/mmc/socfpga_dw_mmc.c
> +++ b/drivers/mmc/socfpga_dw_mmc.c
> @@ -11,25 +11,147 @@
>  #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;
>  static const struct socfpga_system_manager *system_manager_base =
>  		(void *)SOCFPGA_SYSMGR_ADDRESS;
>  
> -static void socfpga_dwmci_clksel(struct dwmci_host *host)
> +#define CAL_ROWS     7
> +#define CAL_COLS     8
> +
> +int find_row_col_fit_rectangle(unsigned rect_width, unsigned rect_height,
> +unsigned char cal_results[CAL_ROWS][CAL_COLS], 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 < (CAL_COLS - rect_width + 1);
> +	     start_col++) {
> +		for (start_row = 0; start_row < (CAL_ROWS - 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) && ok;
> +			     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.
> + */
> +int find_calibration_point(unsigned char cal_results[CAL_ROWS][CAL_COLS],
> +unsigned int sum, unsigned int *cal_row, unsigned int *cal_col)
>  {
> -	unsigned int drvsel;
> -	unsigned int smplsel;
> +	/* 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[CAL_ROWS * CAL_COLS], 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 == (CAL_ROWS * CAL_COLS)) {
> +		*cal_row = (CAL_ROWS - 1) / 2;
> +		*cal_col = (CAL_COLS - 1) / 2;
> +		return 0;
> +	}
> +
> +	/*
> +	 * Create list of all possible sub-rectangles, in descending area
> +	 * order
> +	 */
> +	for (height = CAL_ROWS; height >= 1; height--) {
> +		for (width = CAL_COLS; 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 < CAL_ROWS * CAL_COLS; k++) {
> +		if (!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;
> +}
> +
> +void socfpga_dwmmc_set_clksel(unsigned int drvsel, unsigned int smplsel)
> +{
>  	/* Disable SDMMC clock. */
>  	clrbits_le32(&clock_manager_base->per_pll.en,
>  		CLKMGR_PERPLLGRP_EN_SDMMCCLK_MASK);
>  
> -	/* Configures drv_sel and smpl_sel */
> -	drvsel = CONFIG_SOCFPGA_DWMMC_DRVSEL;
> -	smplsel = CONFIG_SOCFPGA_DWMMC_SMPSEL;
> -
>  	debug("%s: drvsel %d smplsel %d\n", __func__, drvsel, smplsel);
>  	writel(SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel),
>  		&system_manager_base->sdmmcgrp_ctrl);
> @@ -42,6 +164,64 @@ static void socfpga_dwmci_clksel(struct dwmci_host *host)
>  		CLKMGR_PERPLLGRP_EN_SDMMCCLK_MASK);
>  }
>  
> +/*
> + * This functions calirates the drvsel and smplsel by trying all possible
> + * values and selecting the best combnation
> + */
> +void socfpga_dwmmc_calibrate(struct dwmci_host *host)
> +{
> +	int err = 0;
> +	unsigned char cal_results[CAL_ROWS][CAL_COLS];
> +	unsigned int sum = 0;
> +	unsigned int row, col, drvsel, smplsel;
> +
> +	struct mmc *mmc = (struct mmc *)host->mmc;
> +	if (mmc == NULL) {
> +		printf("struct mmc is NULL\n");
> +		hang();
> +	}
> +
> +	printf("%s: Calibration started.\n", __func__);
> +
> +	for (row = 0; row < CAL_ROWS; row++) {
> +		for (col = 0; col < CAL_COLS; col++) {
> +			drvsel = row + 1;
> +			smplsel = col;
> +			socfpga_dwmmc_set_clksel(drvsel, smplsel);
> +			cal_results[row][col] = !(mmc_set_blocklen(mmc,
> +						  mmc->read_bl_len));
> +			sum += cal_results[row][col];
> +		}
> +	}
> +
> +	debug("%s: Calibration raw data:\n", __func__);
> +	for (row = 0; row < CAL_ROWS; row++) {
> +		debug("\t");
> +		for (col = 0; col < CAL_COLS; col++)
> +			debug("%d ", cal_results[row][col]);
> +		debug("\n");
> +	}
> +
> +	err = find_calibration_point(cal_results, sum, &drvsel, &smplsel);
> +
> +	if (err) {
> +		printf("%s: Calibration failed.\n", __func__);
> +		hang();
> +	} else
> +		printf("%s: Calibration passed: drvsel = %d "
> +		       "smplsel = %d.\n", __func__, drvsel, smplsel);
> +
> +	socfpga_dwmmc_set_clksel(drvsel, smplsel);
> +
> +	/* re-write in case accidentally overwrite with junk */
> +	mmc_set_blocklen(mmc, mmc->read_bl_len);
> +}
> +
> +void socfpga_dwmci_clksel(struct dwmci_host *host)
> +{
> +	socfpga_dwmmc_calibrate(host);
> +}
> +
>  int socfpga_dwmmc_init(u32 regbase, int bus_width, int index)
>  {
>  	struct dwmci_host *host;

[U-Boot] [PATCH v2 1/2] arm: socfpga: mmc: Enable calibration for drvsel and smpsel

[Marek Vasut]

On Friday, August 21, 2015 at 02:54:00 AM, Chin Liang See wrote:
> Hi guys,

Hi,

> Any comment or ack for this patch?

Please don't expect that the reviewers/maintainers have nothing else on
their plate but to review your patch. Besides, this change is really low
priority one, since thus far the SD/MMC works fine on all of the SoCFPGA
boards to my knowledge.

Also, I thought we agreed to wait for Simon to comment on this patch.
You don't have to repost a patch before the discussion settles a bit.
This doesn't help, it only puts burden on the receiving side, chill :)
Let alone the fact that this code should be part of the DWMMC core, as
it is not socfpga specific.

What I would be really interested in is a proper description of the
calibration algorithm. What is the goal and how do you achieve it ?

Best regards,
Marek Vasut

[U-Boot] [PATCH v2 1/2] arm: socfpga: mmc: Enable calibration for drvsel and smpsel

[Chin Liang See]

On Fri, 2015-08-21 at 03:07 +0200, marex at denx.de wrote:
> On Friday, August 21, 2015 at 02:54:00 AM, Chin Liang See wrote:
> > Hi guys,
> 
> Hi,
> 
> > Any comment or ack for this patch?
> 
> Please don't expect that the reviewers/maintainers have nothing else on
> their plate but to review your patch. Besides, this change is really low
> priority one, since thus far the SD/MMC works fine on all of the SoCFPGA
> boards to my knowledge.
> 
> Also, I thought we agreed to wait for Simon to comment on this patch.
> You don't have to repost a patch before the discussion settles a bit.
> This doesn't help, it only puts burden on the receiving side, chill :)
> Let alone the fact that this code should be part of the DWMMC core, as
> it is not socfpga specific.
> 
> What I would be really interested in is a proper description of the
> calibration algorithm. What is the goal and how do you achieve it ?
> 

Actually I am probing the efforts needed for this patch before moving to
next one. While for the SDMMC calibration, it is not needed if the SD
card is routed near to the chip. We have incident where SD access failed
as they routed few inches away.

With that, let wait for Simon and I shall move on next item.
Thanks

Chin Liang


> Best regards,
> Marek Vasut

[U-Boot] [PATCH v2 1/2] arm: socfpga: mmc: Enable calibration for drvsel and smpsel

[Marek Vasut]

On Friday, August 21, 2015 at 03:25:00 AM, Chin Liang See wrote:
> On Fri, 2015-08-21 at 03:07 +0200, marex at denx.de wrote:
> > On Friday, August 21, 2015 at 02:54:00 AM, Chin Liang See wrote:
> > > Hi guys,
> > 
> > Hi,

Hi,

> > > Any comment or ack for this patch?
> > 
> > Please don't expect that the reviewers/maintainers have nothing else on
> > their plate but to review your patch. Besides, this change is really low
> > priority one, since thus far the SD/MMC works fine on all of the SoCFPGA
> > boards to my knowledge.
> > 
> > Also, I thought we agreed to wait for Simon to comment on this patch.
> > You don't have to repost a patch before the discussion settles a bit.
> > This doesn't help, it only puts burden on the receiving side, chill :)
> > Let alone the fact that this code should be part of the DWMMC core, as
> > it is not socfpga specific.
> > 
> > What I would be really interested in is a proper description of the
> > calibration algorithm. What is the goal and how do you achieve it ?
> 
> Actually I am probing the efforts needed for this patch before moving to
> next one. While for the SDMMC calibration, it is not needed if the SD
> card is routed near to the chip. We have incident where SD access failed
> as they routed few inches away.

Does the calibration really help or is that a problem with the board ?
It'd be nice if you put effort into this patch to get it done correctly :)

> With that, let wait for Simon and I shall move on next item.

If you're waiting for Simon, it might be a good idea to actually CC him :-)

Best regards,
Marek Vasut

[U-Boot] [PATCH v2 1/2] arm: socfpga: mmc: Enable calibration for drvsel and smpsel

[Chin Liang See]

Hi,

On Fri, 2015-08-21 at 05:03 +0200, marex at denx.de wrote:
> On Friday, August 21, 2015 at 03:25:00 AM, Chin Liang See wrote:
> > On Fri, 2015-08-21 at 03:07 +0200, marex at denx.de wrote:
> > > On Friday, August 21, 2015 at 02:54:00 AM, Chin Liang See wrote:
> > > > Hi guys,
> > > 
> > > Hi,
> 
> Hi,
> 
> > > > Any comment or ack for this patch?
> > > 
> > > Please don't expect that the reviewers/maintainers have nothing else on
> > > their plate but to review your patch. Besides, this change is really low
> > > priority one, since thus far the SD/MMC works fine on all of the SoCFPGA
> > > boards to my knowledge.
> > > 
> > > Also, I thought we agreed to wait for Simon to comment on this patch.
> > > You don't have to repost a patch before the discussion settles a bit.
> > > This doesn't help, it only puts burden on the receiving side, chill :)
> > > Let alone the fact that this code should be part of the DWMMC core, as
> > > it is not socfpga specific.
> > > 
> > > What I would be really interested in is a proper description of the
> > > calibration algorithm. What is the goal and how do you achieve it ?
> > 
> > Actually I am probing the efforts needed for this patch before moving to
> > next one. While for the SDMMC calibration, it is not needed if the SD
> > card is routed near to the chip. We have incident where SD access failed
> > as they routed few inches away.
> 
> Does the calibration really help or is that a problem with the board ?
> It'd be nice if you put effort into this patch to get it done correctly :)

It's related to the board. When the trace route is not properly design,
it might failed when communication between the controller and card at
high speed. The calibration will ensure the correct data being driven
and sampled at correct window. As this seems applicable for dwmmc, let
me take a look into other vendor implementation. Thanks again for the
advise :)

Chin Liang

> 
> > With that, let wait for Simon and I shall move on next item.
> 
> If you're waiting for Simon, it might be a good idea to actually CC him :-)
> 
> Best regards,
> Marek Vasut

[U-Boot] [PATCH v2 1/2] arm: socfpga: mmc: Enable calibration for drvsel and smpsel

[Marek Vasut]

On Friday, August 21, 2015 at 07:00:46 AM, Chin Liang See wrote:
> Hi,

Hi,

> > > > > Any comment or ack for this patch?
> > > > 
> > > > Please don't expect that the reviewers/maintainers have nothing else
> > > > on their plate but to review your patch. Besides, this change is
> > > > really low priority one, since thus far the SD/MMC works fine on all
> > > > of the SoCFPGA boards to my knowledge.
> > > > 
> > > > Also, I thought we agreed to wait for Simon to comment on this patch.
> > > > You don't have to repost a patch before the discussion settles a bit.
> > > > This doesn't help, it only puts burden on the receiving side, chill
> > > > :) Let alone the fact that this code should be part of the DWMMC
> > > > core, as it is not socfpga specific.
> > > > 
> > > > What I would be really interested in is a proper description of the
> > > > calibration algorithm. What is the goal and how do you achieve it ?
> > > 
> > > Actually I am probing the efforts needed for this patch before moving
> > > to next one. While for the SDMMC calibration, it is not needed if the
> > > SD card is routed near to the chip. We have incident where SD access
> > > failed as they routed few inches away.
> > 
> > Does the calibration really help or is that a problem with the board ?
> > It'd be nice if you put effort into this patch to get it done correctly
> > :)
> 
> It's related to the board. When the trace route is not properly design,
> it might failed when communication between the controller and card at
> high speed. The calibration will ensure the correct data being driven
> and sampled at correct window. As this seems applicable for dwmmc, let
> me take a look into other vendor implementation. Thanks again for the
> advise :)

OK

Best regards,
Marek Vasut

[U-Boot] [PATCH v2 1/2] arm: socfpga: mmc: Enable calibration for drvsel and smpsel

[Pavel Machek]

On Thu 2015-08-20 02:18:29, Chin Liang See wrote:
> Enable SDMMC calibration to determine the best setting for
> drvsel and smpsel. It will be triggered whenever there is
> a change of card frequency and bus width. This is to ensure
> reliable transmission between the controller and the card.
> 
> Signed-off-by: Chin Liang See <clsee@altera.com>
> Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
> Cc: Pavel Machek <pavel@denx.de>
> Cc: Marek Vasut <marex@denx.de>
> Cc: Stefan Roese <sr@denx.de>



> +			/* Determine if the rectangle fits here */
> +			for (add_col = 0; (add_col < rect_width) && ok;
> +			     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;
> +			}

This should go into separate function.

The "&& ok" is not neccessary, since you break.

Thanks,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[U-Boot] [PATCH v2 1/2] arm: socfpga: mmc: Enable calibration for drvsel and smpsel

[Chin Liang See]

On Tue, 2015-08-25 at 23:01 +0200, ZY - pavel wrote:
> On Thu 2015-08-20 02:18:29, Chin Liang See wrote:
> > Enable SDMMC calibration to determine the best setting for
> > drvsel and smpsel. It will be triggered whenever there is
> > a change of card frequency and bus width. This is to ensure
> > reliable transmission between the controller and the card.
> > 
> > Signed-off-by: Chin Liang See <clsee@altera.com>
> > Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
> > Cc: Pavel Machek <pavel@denx.de>
> > Cc: Marek Vasut <marex@denx.de>
> > Cc: Stefan Roese <sr@denx.de>
> 
> 
> 
> > +			/* Determine if the rectangle fits here */
> > +			for (add_col = 0; (add_col < rect_width) && ok;
> > +			     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;
> > +			}
> 
> This should go into separate function.

Actually I already break down once to avoid deep identation :)

> 
> The "&& ok" is not neccessary, since you break.

Cool, let change that in v3. I shall do that once finish sync up with
Jaehoon.

Thanks
Chin Liang

> 
> Thanks,
> 									Pavel