[U-Boot] [PATCH v4] 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 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);
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
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
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
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
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
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
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);
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, indescending 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 rightposition */
if (cr_rect_cand == 1)continue;/** Put the candidate in right location tomaintain
* 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].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.
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);
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 ?
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 fitshere */
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 incase 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
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.
- Put back the correct default value for DT ABI compatiblity
- 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.
- Some common calibration algo will go into dw_mmc.c
- 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
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.
- Put back the correct default value for DT ABI compatiblity
- 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
- Some common calibration algo will go into dw_mmc.c
- 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
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.
- Put back the correct default value for DT ABI compatiblity
- 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
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.
- Put back the correct default value for DT ABI compatiblity
- 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
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.
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 +
It seems that you will always program drvsel to be 1 smaller than it should be.
Same as previous comment
Thanks Chin Liang
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 +
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.
participants (5)
-
Chin Liang See -
Marek Vasut -
Pavel Machek -
Simon Glass -
Trent Piepho