* [PATCH v3 0/2] mmc: mediatek: add support for 64-steps tuning @ 2023-11-30 6:15 Axe Yang 2023-11-30 6:15 ` [PATCH v3 1/2] dt-bindings: mmc: mtk-sd: add tuning steps related property Axe Yang 2023-11-30 6:15 ` [PATCH v3 2/2] mmc: mediatek: extend number of tuning steps Axe Yang 0 siblings, 2 replies; 6+ messages in thread From: Axe Yang @ 2023-11-30 6:15 UTC (permalink / raw) To: Chaotian Jing, Ulf Hansson, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Matthias Brugger, AngeloGioacchino Del Regno, Wenbin Mei Cc: linux-mmc, devicetree, linux-kernel, linux-arm-kernel, linux-mediatek, Project_Global_Chrome_Upstream_Group, Axe Yang Change in v3: - use BIT_ULL() instead of BIT() to avoid potential left shift operations that could cause exceed boundary problem on 32-bit platforms Change in v2: - move the change made to document to the front - change mediatek,tune-step dts property type to enum for better scalability Axe Yang (2): dt-bindings: mmc: mtk-sd: add tuning steps related property mmc: mediatek: extend number of tuning steps .../devicetree/bindings/mmc/mtk-sd.yaml | 9 + drivers/mmc/host/mtk-sd.c | 155 ++++++++++++------ 2 files changed, 116 insertions(+), 48 deletions(-) -- 2.25.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v3 1/2] dt-bindings: mmc: mtk-sd: add tuning steps related property 2023-11-30 6:15 [PATCH v3 0/2] mmc: mediatek: add support for 64-steps tuning Axe Yang @ 2023-11-30 6:15 ` Axe Yang 2023-11-30 6:15 ` [PATCH v3 2/2] mmc: mediatek: extend number of tuning steps Axe Yang 1 sibling, 0 replies; 6+ messages in thread From: Axe Yang @ 2023-11-30 6:15 UTC (permalink / raw) To: Chaotian Jing, Ulf Hansson, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Matthias Brugger, AngeloGioacchino Del Regno, Wenbin Mei Cc: linux-mmc, devicetree, linux-kernel, linux-arm-kernel, linux-mediatek, Project_Global_Chrome_Upstream_Group, Axe Yang, Krzysztof Kozlowski Add 'mediatek,tuning-steps' setting. This property will give MSDC a chance to extend tuning steps up to 64. With more tuning steps, MSDC may achieve a more optimal calibration result, thus avoiding potential CRC issues. Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Signed-off-by: Axe Yang <axe.yang@mediatek.com> --- Documentation/devicetree/bindings/mmc/mtk-sd.yaml | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/Documentation/devicetree/bindings/mmc/mtk-sd.yaml b/Documentation/devicetree/bindings/mmc/mtk-sd.yaml index 3fffa467e4e1..c532ec92d2d9 100644 --- a/Documentation/devicetree/bindings/mmc/mtk-sd.yaml +++ b/Documentation/devicetree/bindings/mmc/mtk-sd.yaml @@ -145,6 +145,15 @@ properties: minimum: 0 maximum: 7 + mediatek,tuning-step: + $ref: /schemas/types.yaml#/definitions/uint32 + description: + Some SoCs need extend tuning step for better delay value to avoid CRC issue. + If not present, default tuning step is 32. For eMMC and SD, this can yield + satisfactory calibration results in most cases. + enum: [32, 64] + default: 32 + resets: maxItems: 1 -- 2.25.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v3 2/2] mmc: mediatek: extend number of tuning steps 2023-11-30 6:15 [PATCH v3 0/2] mmc: mediatek: add support for 64-steps tuning Axe Yang 2023-11-30 6:15 ` [PATCH v3 1/2] dt-bindings: mmc: mtk-sd: add tuning steps related property Axe Yang @ 2023-11-30 6:15 ` Axe Yang 2023-12-01 9:02 ` AngeloGioacchino Del Regno 1 sibling, 1 reply; 6+ messages in thread From: Axe Yang @ 2023-11-30 6:15 UTC (permalink / raw) To: Chaotian Jing, Ulf Hansson, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Matthias Brugger, AngeloGioacchino Del Regno, Wenbin Mei Cc: linux-mmc, devicetree, linux-kernel, linux-arm-kernel, linux-mediatek, Project_Global_Chrome_Upstream_Group, Axe Yang Previously, during the MSDC calibration process, a full clock cycle actually not be covered, which in some cases didn't yield the best results and could cause CRC errors. This problem is particularly evident when MSDC is used as an SDIO host. In fact, MSDC support tuning up to a maximum of 64 steps, but by default, the step number is 32. By increase the tuning step, we are more likely to cover more parts of a clock cycle, and get better calibration result. To illustrate, when tuning 32 steps, if the obtained window has a hole near the middle, like this: 0xffc07ff (hex), then the selected delay will be the 6 (counting from right to left). (32 <- 1) 1111 1111 1100 0000 0000 0111 11(1)1 1111 However, if we tune 64 steps, the window obtained may look like this: 0xfffffffffffc07ff. The final selected delay will be 44, which is safer as it is further away from the hole: (64 <- 1) 1111 ... (1)111 1111 1111 1111 1111 1100 0000 0000 0111 1111 1111 In this case, delay 6 selected through 32 steps tuning is obviously not optimal, and this delay is closer to the hole, using it would easily cause CRC problems. You will need to configure property "mediatek,tuning-step" in MSDC dts node to 64 to extend the steps. Signed-off-by: Axe Yang <axe.yang@mediatek.com> --- drivers/mmc/host/mtk-sd.c | 155 ++++++++++++++++++++++++++------------ 1 file changed, 107 insertions(+), 48 deletions(-) diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c index 97f7c3d4be6e..4cd306b3b295 100644 --- a/drivers/mmc/host/mtk-sd.c +++ b/drivers/mmc/host/mtk-sd.c @@ -252,12 +252,16 @@ #define MSDC_PAD_TUNE_DATWRDLY GENMASK(4, 0) /* RW */ #define MSDC_PAD_TUNE_DATRRDLY GENMASK(12, 8) /* RW */ +#define MSDC_PAD_TUNE_DATRRDLY2 GENMASK(12, 8) /* RW */ #define MSDC_PAD_TUNE_CMDRDLY GENMASK(20, 16) /* RW */ +#define MSDC_PAD_TUNE_CMDRDLY2 GENMASK(20, 16) /* RW */ #define MSDC_PAD_TUNE_CMDRRDLY GENMASK(26, 22) /* RW */ #define MSDC_PAD_TUNE_CLKTDLY GENMASK(31, 27) /* RW */ #define MSDC_PAD_TUNE_RXDLYSEL BIT(15) /* RW */ #define MSDC_PAD_TUNE_RD_SEL BIT(13) /* RW */ #define MSDC_PAD_TUNE_CMD_SEL BIT(21) /* RW */ +#define MSDC_PAD_TUNE_RD2_SEL BIT(13) /* RW */ +#define MSDC_PAD_TUNE_CMD2_SEL BIT(21) /* RW */ #define PAD_DS_TUNE_DLY_SEL BIT(0) /* RW */ #define PAD_DS_TUNE_DLY1 GENMASK(6, 2) /* RW */ @@ -325,7 +329,8 @@ #define DEFAULT_DEBOUNCE (8) /* 8 cycles CD debounce */ -#define PAD_DELAY_MAX 32 /* PAD delay cells */ +#define PAD_DELAY_HALF 32 /* PAD delay cells */ +#define PAD_DELAY_FULL 64 /*--------------------------------------------------------------------------*/ /* Descriptor Structure */ /*--------------------------------------------------------------------------*/ @@ -461,6 +466,7 @@ struct msdc_host { u32 hs400_ds_dly3; u32 hs200_cmd_int_delay; /* cmd internal delay for HS200/SDR104 */ u32 hs400_cmd_int_delay; /* cmd internal delay for HS400 */ + u32 tuning_step; bool hs400_cmd_resp_sel_rising; /* cmd response sample selection for HS400 */ bool hs400_mode; /* current eMMC will run at hs400 mode */ @@ -1615,7 +1621,7 @@ static irqreturn_t msdc_cmdq_irq(struct msdc_host *host, u32 intsts) } if (cmd_err || dat_err) { - dev_err(host->dev, "cmd_err = %d, dat_err =%d, intsts = 0x%x", + dev_err(host->dev, "cmd_err = %d, dat_err = %d, intsts = 0x%x", cmd_err, dat_err, intsts); } @@ -1780,10 +1786,20 @@ static void msdc_init_hw(struct msdc_host *host) DATA_K_VALUE_SEL); sdr_set_bits(host->top_base + EMMC_TOP_CMD, PAD_CMD_RD_RXDLY_SEL); + if (host->tuning_step > PAD_DELAY_HALF) { + sdr_set_bits(host->top_base + EMMC_TOP_CONTROL, + PAD_DAT_RD_RXDLY2_SEL); + sdr_set_bits(host->top_base + EMMC_TOP_CMD, + PAD_CMD_RD_RXDLY2_SEL); + } } else { sdr_set_bits(host->base + tune_reg, MSDC_PAD_TUNE_RD_SEL | MSDC_PAD_TUNE_CMD_SEL); + if (host->tuning_step > PAD_DELAY_HALF) + sdr_set_bits(host->base + tune_reg + 4, + MSDC_PAD_TUNE_RD2_SEL | + MSDC_PAD_TUNE_CMD2_SEL); } } else { /* choose clock tune */ @@ -1925,24 +1941,24 @@ static void msdc_ops_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) msdc_set_mclk(host, ios->timing, ios->clock); } -static u32 test_delay_bit(u32 delay, u32 bit) +static u64 test_delay_bit(u64 delay, u32 bit) { - bit %= PAD_DELAY_MAX; - return delay & BIT(bit); + bit %= PAD_DELAY_FULL; + return delay & BIT_ULL(bit); } -static int get_delay_len(u32 delay, u32 start_bit) +static int get_delay_len(u64 delay, u32 start_bit) { int i; - for (i = 0; i < (PAD_DELAY_MAX - start_bit); i++) { + for (i = 0; i < (PAD_DELAY_FULL - start_bit); i++) { if (test_delay_bit(delay, start_bit + i) == 0) return i; } - return PAD_DELAY_MAX - start_bit; + return PAD_DELAY_FULL - start_bit; } -static struct msdc_delay_phase get_best_delay(struct msdc_host *host, u32 delay) +static struct msdc_delay_phase get_best_delay(struct msdc_host *host, u64 delay) { int start = 0, len = 0; int start_final = 0, len_final = 0; @@ -1950,28 +1966,28 @@ static struct msdc_delay_phase get_best_delay(struct msdc_host *host, u32 delay) struct msdc_delay_phase delay_phase = { 0, }; if (delay == 0) { - dev_err(host->dev, "phase error: [map:%x]\n", delay); + dev_err(host->dev, "phase error: [map:%016llx]\n", delay); delay_phase.final_phase = final_phase; return delay_phase; } - while (start < PAD_DELAY_MAX) { + while (start < PAD_DELAY_FULL) { len = get_delay_len(delay, start); if (len_final < len) { start_final = start; len_final = len; } start += len ? len : 1; - if (len >= 12 && start_final < 4) + if (!upper_32_bits(delay) && len >= 12 && start_final < 4) break; } /* The rule is that to find the smallest delay cell */ if (start_final == 0) - final_phase = (start_final + len_final / 3) % PAD_DELAY_MAX; + final_phase = (start_final + len_final / 3) % PAD_DELAY_FULL; else - final_phase = (start_final + len_final / 2) % PAD_DELAY_MAX; - dev_dbg(host->dev, "phase: [map:%x] [maxlen:%d] [final:%d]\n", + final_phase = (start_final + len_final / 2) % PAD_DELAY_FULL; + dev_dbg(host->dev, "phase: [map:%016llx] [maxlen:%d] [final:%d]\n", delay, len_final, final_phase); delay_phase.maxlen = len_final; @@ -1984,30 +2000,68 @@ static inline void msdc_set_cmd_delay(struct msdc_host *host, u32 value) { u32 tune_reg = host->dev_comp->pad_tune_reg; - if (host->top_base) - sdr_set_field(host->top_base + EMMC_TOP_CMD, PAD_CMD_RXDLY, - value); - else - sdr_set_field(host->base + tune_reg, MSDC_PAD_TUNE_CMDRDLY, - value); + if (host->top_base) { + if (value < PAD_DELAY_HALF) { + sdr_set_field(host->top_base + EMMC_TOP_CMD, PAD_CMD_RXDLY, + value); + sdr_set_field(host->top_base + EMMC_TOP_CMD, PAD_CMD_RXDLY2, + 0); + } else { + sdr_set_field(host->top_base + EMMC_TOP_CMD, PAD_CMD_RXDLY, + PAD_DELAY_HALF - 1); + sdr_set_field(host->top_base + EMMC_TOP_CMD, PAD_CMD_RXDLY2, + value - PAD_DELAY_HALF); + } + } else { + if (value < PAD_DELAY_HALF) { + sdr_set_field(host->base + tune_reg, MSDC_PAD_TUNE_CMDRDLY, + value); + sdr_set_field(host->base + tune_reg + 4, MSDC_PAD_TUNE_CMDRDLY2, + 0); + } else { + sdr_set_field(host->base + tune_reg, MSDC_PAD_TUNE_CMDRDLY, + PAD_DELAY_HALF - 1); + sdr_set_field(host->base + tune_reg + 4, MSDC_PAD_TUNE_CMDRDLY2, + value - PAD_DELAY_HALF); + } + } } static inline void msdc_set_data_delay(struct msdc_host *host, u32 value) { u32 tune_reg = host->dev_comp->pad_tune_reg; - if (host->top_base) - sdr_set_field(host->top_base + EMMC_TOP_CONTROL, - PAD_DAT_RD_RXDLY, value); - else - sdr_set_field(host->base + tune_reg, MSDC_PAD_TUNE_DATRRDLY, - value); + if (host->top_base) { + if (value < PAD_DELAY_HALF) { + sdr_set_field(host->top_base + EMMC_TOP_CONTROL, + PAD_DAT_RD_RXDLY, value); + sdr_set_field(host->top_base + EMMC_TOP_CONTROL, + PAD_DAT_RD_RXDLY2, 0); + } else { + sdr_set_field(host->top_base + EMMC_TOP_CONTROL, + PAD_DAT_RD_RXDLY, PAD_DELAY_HALF - 1); + sdr_set_field(host->top_base + EMMC_TOP_CONTROL, + PAD_DAT_RD_RXDLY2, value - PAD_DELAY_HALF); + } + } else { + if (value < PAD_DELAY_HALF) { + sdr_set_field(host->base + tune_reg, MSDC_PAD_TUNE_DATRRDLY, + value); + sdr_set_field(host->base + tune_reg + 4, MSDC_PAD_TUNE_DATRRDLY2, + 0); + } else { + sdr_set_field(host->base + tune_reg, MSDC_PAD_TUNE_DATRRDLY, + PAD_DELAY_HALF - 1); + sdr_set_field(host->base + tune_reg + 4, MSDC_PAD_TUNE_DATRRDLY2, + value - PAD_DELAY_HALF); + } + } } static int msdc_tune_response(struct mmc_host *mmc, u32 opcode) { struct msdc_host *host = mmc_priv(mmc); - u32 rise_delay = 0, fall_delay = 0; + u64 rise_delay = 0, fall_delay = 0; struct msdc_delay_phase final_rise_delay, final_fall_delay = { 0,}; struct msdc_delay_phase internal_delay_phase; u8 final_delay, final_maxlen; @@ -2023,7 +2077,7 @@ static int msdc_tune_response(struct mmc_host *mmc, u32 opcode) host->hs200_cmd_int_delay); sdr_clr_bits(host->base + MSDC_IOCON, MSDC_IOCON_RSPL); - for (i = 0 ; i < PAD_DELAY_MAX; i++) { + for (i = 0; i < host->tuning_step; i++) { msdc_set_cmd_delay(host, i); /* * Using the same parameters, it may sometimes pass the test, @@ -2033,9 +2087,9 @@ static int msdc_tune_response(struct mmc_host *mmc, u32 opcode) for (j = 0; j < 3; j++) { mmc_send_tuning(mmc, opcode, &cmd_err); if (!cmd_err) { - rise_delay |= BIT(i); + rise_delay |= BIT_ULL(i); } else { - rise_delay &= ~BIT(i); + rise_delay &= ~BIT_ULL(i); break; } } @@ -2047,7 +2101,7 @@ static int msdc_tune_response(struct mmc_host *mmc, u32 opcode) goto skip_fall; sdr_set_bits(host->base + MSDC_IOCON, MSDC_IOCON_RSPL); - for (i = 0; i < PAD_DELAY_MAX; i++) { + for (i = 0; i < host->tuning_step; i++) { msdc_set_cmd_delay(host, i); /* * Using the same parameters, it may sometimes pass the test, @@ -2057,9 +2111,9 @@ static int msdc_tune_response(struct mmc_host *mmc, u32 opcode) for (j = 0; j < 3; j++) { mmc_send_tuning(mmc, opcode, &cmd_err); if (!cmd_err) { - fall_delay |= BIT(i); + fall_delay |= BIT_ULL(i); } else { - fall_delay &= ~BIT(i); + fall_delay &= ~BIT_ULL(i); break; } } @@ -2082,12 +2136,12 @@ static int msdc_tune_response(struct mmc_host *mmc, u32 opcode) if (host->dev_comp->async_fifo || host->hs200_cmd_int_delay) goto skip_internal; - for (i = 0; i < PAD_DELAY_MAX; i++) { + for (i = 0; i < host->tuning_step; i++) { sdr_set_field(host->base + tune_reg, MSDC_PAD_TUNE_CMDRRDLY, i); mmc_send_tuning(mmc, opcode, &cmd_err); if (!cmd_err) - internal_delay |= BIT(i); + internal_delay |= BIT_ULL(i); } dev_dbg(host->dev, "Final internal delay: 0x%x\n", internal_delay); internal_delay_phase = get_best_delay(host, internal_delay); @@ -2121,7 +2175,8 @@ static int hs400_tune_response(struct mmc_host *mmc, u32 opcode) sdr_clr_bits(host->base + MSDC_IOCON, MSDC_IOCON_RSPL); else sdr_set_bits(host->base + MSDC_IOCON, MSDC_IOCON_RSPL); - for (i = 0 ; i < PAD_DELAY_MAX; i++) { + + for (i = 0; i < PAD_DELAY_HALF; i++) { sdr_set_field(host->base + PAD_CMD_TUNE, PAD_CMD_TUNE_RX_DLY3, i); /* @@ -2151,7 +2206,7 @@ static int hs400_tune_response(struct mmc_host *mmc, u32 opcode) static int msdc_tune_data(struct mmc_host *mmc, u32 opcode) { struct msdc_host *host = mmc_priv(mmc); - u32 rise_delay = 0, fall_delay = 0; + u64 rise_delay = 0, fall_delay = 0; struct msdc_delay_phase final_rise_delay, final_fall_delay = { 0,}; u8 final_delay, final_maxlen; int i, ret; @@ -2160,11 +2215,11 @@ static int msdc_tune_data(struct mmc_host *mmc, u32 opcode) host->latch_ck); sdr_clr_bits(host->base + MSDC_IOCON, MSDC_IOCON_DSPL); sdr_clr_bits(host->base + MSDC_IOCON, MSDC_IOCON_W_DSPL); - for (i = 0 ; i < PAD_DELAY_MAX; i++) { + for (i = 0; i < host->tuning_step; i++) { msdc_set_data_delay(host, i); ret = mmc_send_tuning(mmc, opcode, NULL); if (!ret) - rise_delay |= BIT(i); + rise_delay |= BIT_ULL(i); } final_rise_delay = get_best_delay(host, rise_delay); /* if rising edge has enough margin, then do not scan falling edge */ @@ -2174,11 +2229,11 @@ static int msdc_tune_data(struct mmc_host *mmc, u32 opcode) sdr_set_bits(host->base + MSDC_IOCON, MSDC_IOCON_DSPL); sdr_set_bits(host->base + MSDC_IOCON, MSDC_IOCON_W_DSPL); - for (i = 0; i < PAD_DELAY_MAX; i++) { + for (i = 0; i < host->tuning_step; i++) { msdc_set_data_delay(host, i); ret = mmc_send_tuning(mmc, opcode, NULL); if (!ret) - fall_delay |= BIT(i); + fall_delay |= BIT_ULL(i); } final_fall_delay = get_best_delay(host, fall_delay); @@ -2206,7 +2261,7 @@ static int msdc_tune_data(struct mmc_host *mmc, u32 opcode) static int msdc_tune_together(struct mmc_host *mmc, u32 opcode) { struct msdc_host *host = mmc_priv(mmc); - u32 rise_delay = 0, fall_delay = 0; + u64 rise_delay = 0, fall_delay = 0; struct msdc_delay_phase final_rise_delay, final_fall_delay = { 0,}; u8 final_delay, final_maxlen; int i, ret; @@ -2217,12 +2272,12 @@ static int msdc_tune_together(struct mmc_host *mmc, u32 opcode) sdr_clr_bits(host->base + MSDC_IOCON, MSDC_IOCON_RSPL); sdr_clr_bits(host->base + MSDC_IOCON, MSDC_IOCON_DSPL | MSDC_IOCON_W_DSPL); - for (i = 0 ; i < PAD_DELAY_MAX; i++) { + for (i = 0; i < host->tuning_step; i++) { msdc_set_cmd_delay(host, i); msdc_set_data_delay(host, i); ret = mmc_send_tuning(mmc, opcode, NULL); if (!ret) - rise_delay |= BIT(i); + rise_delay |= BIT_ULL(i); } final_rise_delay = get_best_delay(host, rise_delay); /* if rising edge has enough margin, then do not scan falling edge */ @@ -2233,12 +2288,12 @@ static int msdc_tune_together(struct mmc_host *mmc, u32 opcode) sdr_set_bits(host->base + MSDC_IOCON, MSDC_IOCON_RSPL); sdr_set_bits(host->base + MSDC_IOCON, MSDC_IOCON_DSPL | MSDC_IOCON_W_DSPL); - for (i = 0; i < PAD_DELAY_MAX; i++) { + for (i = 0; i < host->tuning_step; i++) { msdc_set_cmd_delay(host, i); msdc_set_data_delay(host, i); ret = mmc_send_tuning(mmc, opcode, NULL); if (!ret) - fall_delay |= BIT(i); + fall_delay |= BIT_ULL(i); } final_fall_delay = get_best_delay(host, fall_delay); @@ -2346,7 +2401,7 @@ static int msdc_execute_hs400_tuning(struct mmc_host *mmc, struct mmc_card *card } host->hs400_tuning = true; - for (i = 0; i < PAD_DELAY_MAX; i++) { + for (i = 0; i < PAD_DELAY_HALF; i++) { if (host->top_base) sdr_set_field(host->top_base + EMMC50_PAD_DS_TUNE, PAD_DS_DLY1, i); @@ -2601,6 +2656,10 @@ static void msdc_of_property_parse(struct platform_device *pdev, else host->hs400_cmd_resp_sel_rising = false; + if (of_property_read_u32(pdev->dev.of_node, "mediatek,tuning-step", + &host->tuning_step)) + host->tuning_step = PAD_DELAY_HALF; + if (of_property_read_bool(pdev->dev.of_node, "supports-cqe")) host->cqhci = true; -- 2.25.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3 2/2] mmc: mediatek: extend number of tuning steps 2023-11-30 6:15 ` [PATCH v3 2/2] mmc: mediatek: extend number of tuning steps Axe Yang @ 2023-12-01 9:02 ` AngeloGioacchino Del Regno 2023-12-05 2:02 ` Axe Yang (杨磊) 0 siblings, 1 reply; 6+ messages in thread From: AngeloGioacchino Del Regno @ 2023-12-01 9:02 UTC (permalink / raw) To: Axe Yang, Chaotian Jing, Ulf Hansson, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Matthias Brugger, Wenbin Mei Cc: linux-mmc, devicetree, linux-kernel, linux-arm-kernel, linux-mediatek, Project_Global_Chrome_Upstream_Group Il 30/11/23 07:15, Axe Yang ha scritto: > Previously, during the MSDC calibration process, a full clock cycle > actually not be covered, which in some cases didn't yield the best > results and could cause CRC errors. This problem is particularly > evident when MSDC is used as an SDIO host. In fact, MSDC support > tuning up to a maximum of 64 steps, but by default, the step number > is 32. By increase the tuning step, we are more likely to cover more > parts of a clock cycle, and get better calibration result. > > To illustrate, when tuning 32 steps, if the obtained window has a hole > near the middle, like this: 0xffc07ff (hex), then the selected delay > will be the 6 (counting from right to left). > > (32 <- 1) > 1111 1111 1100 0000 0000 0111 11(1)1 1111 > > However, if we tune 64 steps, the window obtained may look like this: > 0xfffffffffffc07ff. The final selected delay will be 44, which is > safer as it is further away from the hole: > > (64 <- 1) > 1111 ... (1)111 1111 1111 1111 1111 1100 0000 0000 0111 1111 1111 > > In this case, delay 6 selected through 32 steps tuning is obviously > not optimal, and this delay is closer to the hole, using it would > easily cause CRC problems. > > You will need to configure property "mediatek,tuning-step" in MSDC > dts node to 64 to extend the steps. > > Signed-off-by: Axe Yang <axe.yang@mediatek.com> > --- > drivers/mmc/host/mtk-sd.c | 155 ++++++++++++++++++++++++++------------ > 1 file changed, 107 insertions(+), 48 deletions(-) > > diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c > index 97f7c3d4be6e..4cd306b3b295 100644 > --- a/drivers/mmc/host/mtk-sd.c > +++ b/drivers/mmc/host/mtk-sd.c > @@ -252,12 +252,16 @@ > > #define MSDC_PAD_TUNE_DATWRDLY GENMASK(4, 0) /* RW */ > #define MSDC_PAD_TUNE_DATRRDLY GENMASK(12, 8) /* RW */ > +#define MSDC_PAD_TUNE_DATRRDLY2 GENMASK(12, 8) /* RW */ > #define MSDC_PAD_TUNE_CMDRDLY GENMASK(20, 16) /* RW */ > +#define MSDC_PAD_TUNE_CMDRDLY2 GENMASK(20, 16) /* RW */ > #define MSDC_PAD_TUNE_CMDRRDLY GENMASK(26, 22) /* RW */ > #define MSDC_PAD_TUNE_CLKTDLY GENMASK(31, 27) /* RW */ > #define MSDC_PAD_TUNE_RXDLYSEL BIT(15) /* RW */ > #define MSDC_PAD_TUNE_RD_SEL BIT(13) /* RW */ > #define MSDC_PAD_TUNE_CMD_SEL BIT(21) /* RW */ > +#define MSDC_PAD_TUNE_RD2_SEL BIT(13) /* RW */ > +#define MSDC_PAD_TUNE_CMD2_SEL BIT(21) /* RW */ > > #define PAD_DS_TUNE_DLY_SEL BIT(0) /* RW */ > #define PAD_DS_TUNE_DLY1 GENMASK(6, 2) /* RW */ > @@ -325,7 +329,8 @@ > > #define DEFAULT_DEBOUNCE (8) /* 8 cycles CD debounce */ > > -#define PAD_DELAY_MAX 32 /* PAD delay cells */ > +#define PAD_DELAY_HALF 32 /* PAD delay cells */ > +#define PAD_DELAY_FULL 64 > /*--------------------------------------------------------------------------*/ > /* Descriptor Structure */ > /*--------------------------------------------------------------------------*/ > @@ -461,6 +466,7 @@ struct msdc_host { > u32 hs400_ds_dly3; > u32 hs200_cmd_int_delay; /* cmd internal delay for HS200/SDR104 */ > u32 hs400_cmd_int_delay; /* cmd internal delay for HS400 */ > + u32 tuning_step; > bool hs400_cmd_resp_sel_rising; > /* cmd response sample selection for HS400 */ > bool hs400_mode; /* current eMMC will run at hs400 mode */ > @@ -1615,7 +1621,7 @@ static irqreturn_t msdc_cmdq_irq(struct msdc_host *host, u32 intsts) > } > > if (cmd_err || dat_err) { > - dev_err(host->dev, "cmd_err = %d, dat_err =%d, intsts = 0x%x", > + dev_err(host->dev, "cmd_err = %d, dat_err = %d, intsts = 0x%x", > cmd_err, dat_err, intsts); > } > > @@ -1780,10 +1786,20 @@ static void msdc_init_hw(struct msdc_host *host) > DATA_K_VALUE_SEL); > sdr_set_bits(host->top_base + EMMC_TOP_CMD, > PAD_CMD_RD_RXDLY_SEL); > + if (host->tuning_step > PAD_DELAY_HALF) { > + sdr_set_bits(host->top_base + EMMC_TOP_CONTROL, > + PAD_DAT_RD_RXDLY2_SEL); > + sdr_set_bits(host->top_base + EMMC_TOP_CMD, > + PAD_CMD_RD_RXDLY2_SEL); > + } > } else { > sdr_set_bits(host->base + tune_reg, > MSDC_PAD_TUNE_RD_SEL | > MSDC_PAD_TUNE_CMD_SEL); > + if (host->tuning_step > PAD_DELAY_HALF) > + sdr_set_bits(host->base + tune_reg + 4, `tune_reg + 4` is a different register, please define it. Also, I can't find this in MT8192, MT8195 - as those bits seem to be undefined, so, which SoCs are actually compatible with this change? > + MSDC_PAD_TUNE_RD2_SEL | > + MSDC_PAD_TUNE_CMD2_SEL); > } > } else { > /* choose clock tune */ > @@ -1925,24 +1941,24 @@ static void msdc_ops_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) > msdc_set_mclk(host, ios->timing, ios->clock); > } > > -static u32 test_delay_bit(u32 delay, u32 bit) > +static u64 test_delay_bit(u64 delay, u32 bit) > { > - bit %= PAD_DELAY_MAX; > - return delay & BIT(bit); > + bit %= PAD_DELAY_FULL; > + return delay & BIT_ULL(bit); > } > > -static int get_delay_len(u32 delay, u32 start_bit) > +static int get_delay_len(u64 delay, u32 start_bit) > { > int i; > > - for (i = 0; i < (PAD_DELAY_MAX - start_bit); i++) { > + for (i = 0; i < (PAD_DELAY_FULL - start_bit); i++) { > if (test_delay_bit(delay, start_bit + i) == 0) > return i; > } > - return PAD_DELAY_MAX - start_bit; > + return PAD_DELAY_FULL - start_bit; > } > > -static struct msdc_delay_phase get_best_delay(struct msdc_host *host, u32 delay) > +static struct msdc_delay_phase get_best_delay(struct msdc_host *host, u64 delay) > { > int start = 0, len = 0; > int start_final = 0, len_final = 0; > @@ -1950,28 +1966,28 @@ static struct msdc_delay_phase get_best_delay(struct msdc_host *host, u32 delay) > struct msdc_delay_phase delay_phase = { 0, }; > > if (delay == 0) { > - dev_err(host->dev, "phase error: [map:%x]\n", delay); > + dev_err(host->dev, "phase error: [map:%016llx]\n", delay); > delay_phase.final_phase = final_phase; > return delay_phase; > } > > - while (start < PAD_DELAY_MAX) { > + while (start < PAD_DELAY_FULL) { > len = get_delay_len(delay, start); > if (len_final < len) { > start_final = start; > len_final = len; > } > start += len ? len : 1; > - if (len >= 12 && start_final < 4) > + if (!upper_32_bits(delay) && len >= 12 && start_final < 4) > break; > } > > /* The rule is that to find the smallest delay cell */ > if (start_final == 0) > - final_phase = (start_final + len_final / 3) % PAD_DELAY_MAX; > + final_phase = (start_final + len_final / 3) % PAD_DELAY_FULL; > else > - final_phase = (start_final + len_final / 2) % PAD_DELAY_MAX; > - dev_dbg(host->dev, "phase: [map:%x] [maxlen:%d] [final:%d]\n", > + final_phase = (start_final + len_final / 2) % PAD_DELAY_FULL; > + dev_dbg(host->dev, "phase: [map:%016llx] [maxlen:%d] [final:%d]\n", > delay, len_final, final_phase); > > delay_phase.maxlen = len_final; > @@ -1984,30 +2000,68 @@ static inline void msdc_set_cmd_delay(struct msdc_host *host, u32 value) > { > u32 tune_reg = host->dev_comp->pad_tune_reg; > > - if (host->top_base) > - sdr_set_field(host->top_base + EMMC_TOP_CMD, PAD_CMD_RXDLY, > - value); > - else > - sdr_set_field(host->base + tune_reg, MSDC_PAD_TUNE_CMDRDLY, > - value); > + if (host->top_base) { > + if (value < PAD_DELAY_HALF) { > + sdr_set_field(host->top_base + EMMC_TOP_CMD, PAD_CMD_RXDLY, > + value); This goes up to 92 columns, and it's fine, so fits in one line and it's more readable like that. I know that's not your fault, but since you're actually touching those lines it's a good occasion to also do that (not only here) :-) Cheers, Angelo ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 2/2] mmc: mediatek: extend number of tuning steps 2023-12-01 9:02 ` AngeloGioacchino Del Regno @ 2023-12-05 2:02 ` Axe Yang (杨磊) 2023-12-05 13:24 ` AngeloGioacchino Del Regno 0 siblings, 1 reply; 6+ messages in thread From: Axe Yang (杨磊) @ 2023-12-05 2:02 UTC (permalink / raw) To: robh+dt@kernel.org, Wenbin Mei (梅文彬), conor+dt@kernel.org, Chaotian Jing (井朝天), krzysztof.kozlowski+dt@linaro.org, matthias.bgg@gmail.com, ulf.hansson@linaro.org, angelogioacchino.delregno@collabora.com Cc: linux-arm-kernel@lists.infradead.org, linux-mmc@vger.kernel.org, linux-kernel@vger.kernel.org, linux-mediatek@lists.infradead.org, devicetree@vger.kernel.org, Project_Global_Chrome_Upstream_Group On Fri, 2023-12-01 at 10:02 +0100, AngeloGioacchino Del Regno wrote: > Il 30/11/23 07:15, Axe Yang ha scritto: > > Previously, during the MSDC calibration process, a full clock cycle > > actually not be covered, which in some cases didn't yield the best > > results and could cause CRC errors. This problem is particularly > > evident when MSDC is used as an SDIO host. In fact, MSDC support > > tuning up to a maximum of 64 steps, but by default, the step number > > is 32. By increase the tuning step, we are more likely to cover > > more > > parts of a clock cycle, and get better calibration result. > > > > To illustrate, when tuning 32 steps, if the obtained window has a > > hole > > near the middle, like this: 0xffc07ff (hex), then the selected > > delay > > will be the 6 (counting from right to left). > > > > (32 <- 1) > > 1111 1111 1100 0000 0000 0111 11(1)1 1111 > > > > However, if we tune 64 steps, the window obtained may look like > > this: > > 0xfffffffffffc07ff. The final selected delay will be 44, which is > > safer as it is further away from the hole: > > > > (64 <- 1) > > 1111 ... (1)111 1111 1111 1111 1111 1100 0000 0000 0111 1111 1111 > > > > In this case, delay 6 selected through 32 steps tuning is obviously > > not optimal, and this delay is closer to the hole, using it would > > easily cause CRC problems. > > > > You will need to configure property "mediatek,tuning-step" in MSDC > > dts node to 64 to extend the steps. > > > > Signed-off-by: Axe Yang <axe.yang@mediatek.com> > > --- > > drivers/mmc/host/mtk-sd.c | 155 ++++++++++++++++++++++++++------- > > ----- > > 1 file changed, 107 insertions(+), 48 deletions(-) > > > > diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c > > index 97f7c3d4be6e..4cd306b3b295 100644 > > --- a/drivers/mmc/host/mtk-sd.c > > +++ b/drivers/mmc/host/mtk-sd.c > > @@ -252,12 +252,16 @@ > > > > #define MSDC_PAD_TUNE_DATWRDLY GENMASK(4, 0) /* > > RW */ > > #define MSDC_PAD_TUNE_DATRRDLY GENMASK(12, 8) /* RW */ > > +#define MSDC_PAD_TUNE_DATRRDLY2 GENMASK(12, 8) /* RW */ > > #define MSDC_PAD_TUNE_CMDRDLY GENMASK(20, 16) /* RW */ > > +#define MSDC_PAD_TUNE_CMDRDLY2 GENMASK(20, 16) /* RW */ > > #define MSDC_PAD_TUNE_CMDRRDLY GENMASK(26, 22) /* RW */ > > #define MSDC_PAD_TUNE_CLKTDLY GENMASK(31, 27) /* RW */ > > #define MSDC_PAD_TUNE_RXDLYSEL BIT(15) /* RW */ > > #define MSDC_PAD_TUNE_RD_SEL BIT(13) /* RW */ > > #define MSDC_PAD_TUNE_CMD_SEL BIT(21) /* RW */ > > +#define MSDC_PAD_TUNE_RD2_SEL BIT(13) /* RW */ > > +#define MSDC_PAD_TUNE_CMD2_SEL BIT(21) /* RW */ > > > > #define PAD_DS_TUNE_DLY_SEL BIT(0) /* RW */ > > #define PAD_DS_TUNE_DLY1 GENMASK(6, 2) /* RW */ > > @@ -325,7 +329,8 @@ > > > > #define DEFAULT_DEBOUNCE (8) /* 8 cycles CD debounce */ > > > > -#define PAD_DELAY_MAX 32 /* PAD delay cells */ > > +#define PAD_DELAY_HALF 32 /* PAD delay cells */ > > +#define PAD_DELAY_FULL 64 > > /*--------------------------------------------------------------- > > -----------*/ > > /* Descriptor > > Structure */ > > /*--------------------------------------------------------------- > > -----------*/ > > @@ -461,6 +466,7 @@ struct msdc_host { > > u32 hs400_ds_dly3; > > u32 hs200_cmd_int_delay; /* cmd internal delay for HS200/SDR104 > > */ > > u32 hs400_cmd_int_delay; /* cmd internal delay for HS400 */ > > + u32 tuning_step; > > bool hs400_cmd_resp_sel_rising; > > /* cmd response sample selection for > > HS400 */ > > bool hs400_mode; /* current eMMC will run at hs400 mode */ > > @@ -1615,7 +1621,7 @@ static irqreturn_t msdc_cmdq_irq(struct > > msdc_host *host, u32 intsts) > > } > > > > if (cmd_err || dat_err) { > > - dev_err(host->dev, "cmd_err = %d, dat_err =%d, intsts = > > 0x%x", > > + dev_err(host->dev, "cmd_err = %d, dat_err = %d, intsts > > = 0x%x", > > cmd_err, dat_err, intsts); > > } > > > > @@ -1780,10 +1786,20 @@ static void msdc_init_hw(struct msdc_host > > *host) > > DATA_K_VALUE_SEL); > > sdr_set_bits(host->top_base + EMMC_TOP_CMD, > > PAD_CMD_RD_RXDLY_SEL); > > + if (host->tuning_step > PAD_DELAY_HALF) { > > + sdr_set_bits(host->top_base + > > EMMC_TOP_CONTROL, > > + PAD_DAT_RD_RXDLY2_SEL); > > + sdr_set_bits(host->top_base + > > EMMC_TOP_CMD, > > + PAD_CMD_RD_RXDLY2_SEL); > > + } > > } else { > > sdr_set_bits(host->base + tune_reg, > > MSDC_PAD_TUNE_RD_SEL | > > MSDC_PAD_TUNE_CMD_SEL); > > + if (host->tuning_step > PAD_DELAY_HALF) > > + sdr_set_bits(host->base + tune_reg + 4, > > `tune_reg + 4` is a different register, please define it. > The tune_reg is not a fixed register address, it is defined in compatible structures. So using the offset here will make the code more consise. The offset of registers related to 64 steps tuning is fixed relative to 32-steps tuning regsiter, the offset is always 4. However. using the magic number '4' here directly is not ideal. I think I can improve this part of code by defining '4' as a macro. What do you think about it? If you insist on redifning the registers, I can do it, but it will make the code a bit more complex than it is now. > Also, I can't find this in MT8192, MT8195 - as those bits seem to be > undefined, > so, which SoCs are actually compatible with this change? Sorry, which bits are you talking about? This change compatible for all SoCs. In fact, MSDC has always supported 64 step tuning. > > > > + MSDC_PAD_TUNE_RD2_SEL | > > + MSDC_PAD_TUNE_CMD2_SEL); > > } > > } else { > > /* choose clock tune */ > > @@ -1925,24 +1941,24 @@ static void msdc_ops_set_ios(struct > > mmc_host *mmc, struct mmc_ios *ios) > > msdc_set_mclk(host, ios->timing, ios->clock); > > } > > > > -static u32 test_delay_bit(u32 delay, u32 bit) > > +static u64 test_delay_bit(u64 delay, u32 bit) > > { > > - bit %= PAD_DELAY_MAX; > > - return delay & BIT(bit); > > + bit %= PAD_DELAY_FULL; > > + return delay & BIT_ULL(bit); > > } > > > > -static int get_delay_len(u32 delay, u32 start_bit) > > +static int get_delay_len(u64 delay, u32 start_bit) > > { > > int i; > > > > - for (i = 0; i < (PAD_DELAY_MAX - start_bit); i++) { > > + for (i = 0; i < (PAD_DELAY_FULL - start_bit); i++) { > > if (test_delay_bit(delay, start_bit + i) == 0) > > return i; > > } > > - return PAD_DELAY_MAX - start_bit; > > + return PAD_DELAY_FULL - start_bit; > > } > > > > -static struct msdc_delay_phase get_best_delay(struct msdc_host > > *host, u32 delay) > > +static struct msdc_delay_phase get_best_delay(struct msdc_host > > *host, u64 delay) > > { > > int start = 0, len = 0; > > int start_final = 0, len_final = 0; > > @@ -1950,28 +1966,28 @@ static struct msdc_delay_phase > > get_best_delay(struct msdc_host *host, u32 delay) > > struct msdc_delay_phase delay_phase = { 0, }; > > > > if (delay == 0) { > > - dev_err(host->dev, "phase error: [map:%x]\n", delay); > > + dev_err(host->dev, "phase error: [map:%016llx]\n", > > delay); > > delay_phase.final_phase = final_phase; > > return delay_phase; > > } > > > > - while (start < PAD_DELAY_MAX) { > > + while (start < PAD_DELAY_FULL) { > > len = get_delay_len(delay, start); > > if (len_final < len) { > > start_final = start; > > len_final = len; > > } > > start += len ? len : 1; > > - if (len >= 12 && start_final < 4) > > + if (!upper_32_bits(delay) && len >= 12 && start_final < > > 4) > > break; > > } > > > > /* The rule is that to find the smallest delay cell */ > > if (start_final == 0) > > - final_phase = (start_final + len_final / 3) % > > PAD_DELAY_MAX; > > + final_phase = (start_final + len_final / 3) % > > PAD_DELAY_FULL; > > else > > - final_phase = (start_final + len_final / 2) % > > PAD_DELAY_MAX; > > - dev_dbg(host->dev, "phase: [map:%x] [maxlen:%d] [final:%d]\n", > > + final_phase = (start_final + len_final / 2) % > > PAD_DELAY_FULL; > > + dev_dbg(host->dev, "phase: [map:%016llx] [maxlen:%d] > > [final:%d]\n", > > delay, len_final, final_phase); > > > > delay_phase.maxlen = len_final; > > @@ -1984,30 +2000,68 @@ static inline void > > msdc_set_cmd_delay(struct msdc_host *host, u32 value) > > { > > u32 tune_reg = host->dev_comp->pad_tune_reg; > > > > - if (host->top_base) > > - sdr_set_field(host->top_base + EMMC_TOP_CMD, > > PAD_CMD_RXDLY, > > - value); > > - else > > - sdr_set_field(host->base + tune_reg, > > MSDC_PAD_TUNE_CMDRDLY, > > - value); > > + if (host->top_base) { > > + if (value < PAD_DELAY_HALF) { > > + sdr_set_field(host->top_base + EMMC_TOP_CMD, > > PAD_CMD_RXDLY, > > + value); > > This goes up to 92 columns, and it's fine, so fits in one line and > it's more > readable like that. > > I know that's not your fault, but since you're actually touching > those lines > it's a good occasion to also do that (not only here) :-) > Sure, will update this part in v4, and thanks for your meticulous review. Regards, Axe > Angelo ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 2/2] mmc: mediatek: extend number of tuning steps 2023-12-05 2:02 ` Axe Yang (杨磊) @ 2023-12-05 13:24 ` AngeloGioacchino Del Regno 0 siblings, 0 replies; 6+ messages in thread From: AngeloGioacchino Del Regno @ 2023-12-05 13:24 UTC (permalink / raw) To: Axe Yang (杨磊), robh+dt@kernel.org, Wenbin Mei (梅文彬), conor+dt@kernel.org, Chaotian Jing (井朝天), krzysztof.kozlowski+dt@linaro.org, matthias.bgg@gmail.com, ulf.hansson@linaro.org Cc: linux-arm-kernel@lists.infradead.org, linux-mmc@vger.kernel.org, linux-kernel@vger.kernel.org, linux-mediatek@lists.infradead.org, devicetree@vger.kernel.org, Project_Global_Chrome_Upstream_Group Il 05/12/23 03:02, Axe Yang (杨磊) ha scritto: > On Fri, 2023-12-01 at 10:02 +0100, AngeloGioacchino Del Regno wrote: >> Il 30/11/23 07:15, Axe Yang ha scritto: >>> Previously, during the MSDC calibration process, a full clock cycle >>> actually not be covered, which in some cases didn't yield the best >>> results and could cause CRC errors. This problem is particularly >>> evident when MSDC is used as an SDIO host. In fact, MSDC support >>> tuning up to a maximum of 64 steps, but by default, the step number >>> is 32. By increase the tuning step, we are more likely to cover >>> more >>> parts of a clock cycle, and get better calibration result. >>> >>> To illustrate, when tuning 32 steps, if the obtained window has a >>> hole >>> near the middle, like this: 0xffc07ff (hex), then the selected >>> delay >>> will be the 6 (counting from right to left). >>> >>> (32 <- 1) >>> 1111 1111 1100 0000 0000 0111 11(1)1 1111 >>> >>> However, if we tune 64 steps, the window obtained may look like >>> this: >>> 0xfffffffffffc07ff. The final selected delay will be 44, which is >>> safer as it is further away from the hole: >>> >>> (64 <- 1) >>> 1111 ... (1)111 1111 1111 1111 1111 1100 0000 0000 0111 1111 1111 >>> >>> In this case, delay 6 selected through 32 steps tuning is obviously >>> not optimal, and this delay is closer to the hole, using it would >>> easily cause CRC problems. >>> >>> You will need to configure property "mediatek,tuning-step" in MSDC >>> dts node to 64 to extend the steps. >>> >>> Signed-off-by: Axe Yang <axe.yang@mediatek.com> >>> --- >>> drivers/mmc/host/mtk-sd.c | 155 ++++++++++++++++++++++++++------- >>> ----- >>> 1 file changed, 107 insertions(+), 48 deletions(-) >>> >>> diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c >>> index 97f7c3d4be6e..4cd306b3b295 100644 >>> --- a/drivers/mmc/host/mtk-sd.c >>> +++ b/drivers/mmc/host/mtk-sd.c >>> @@ -252,12 +252,16 @@ >>> >>> #define MSDC_PAD_TUNE_DATWRDLY GENMASK(4, 0) /* >>> RW */ >>> #define MSDC_PAD_TUNE_DATRRDLY GENMASK(12, 8) /* RW */ >>> +#define MSDC_PAD_TUNE_DATRRDLY2 GENMASK(12, 8) /* RW */ >>> #define MSDC_PAD_TUNE_CMDRDLY GENMASK(20, 16) /* RW */ >>> +#define MSDC_PAD_TUNE_CMDRDLY2 GENMASK(20, 16) /* RW */ >>> #define MSDC_PAD_TUNE_CMDRRDLY GENMASK(26, 22) /* RW */ >>> #define MSDC_PAD_TUNE_CLKTDLY GENMASK(31, 27) /* RW */ >>> #define MSDC_PAD_TUNE_RXDLYSEL BIT(15) /* RW */ >>> #define MSDC_PAD_TUNE_RD_SEL BIT(13) /* RW */ >>> #define MSDC_PAD_TUNE_CMD_SEL BIT(21) /* RW */ >>> +#define MSDC_PAD_TUNE_RD2_SEL BIT(13) /* RW */ >>> +#define MSDC_PAD_TUNE_CMD2_SEL BIT(21) /* RW */ >>> >>> #define PAD_DS_TUNE_DLY_SEL BIT(0) /* RW */ >>> #define PAD_DS_TUNE_DLY1 GENMASK(6, 2) /* RW */ >>> @@ -325,7 +329,8 @@ >>> >>> #define DEFAULT_DEBOUNCE (8) /* 8 cycles CD debounce */ >>> >>> -#define PAD_DELAY_MAX 32 /* PAD delay cells */ >>> +#define PAD_DELAY_HALF 32 /* PAD delay cells */ >>> +#define PAD_DELAY_FULL 64 >>> /*--------------------------------------------------------------- >>> -----------*/ >>> /* Descriptor >>> Structure */ >>> /*--------------------------------------------------------------- >>> -----------*/ >>> @@ -461,6 +466,7 @@ struct msdc_host { >>> u32 hs400_ds_dly3; >>> u32 hs200_cmd_int_delay; /* cmd internal delay for HS200/SDR104 >>> */ >>> u32 hs400_cmd_int_delay; /* cmd internal delay for HS400 */ >>> + u32 tuning_step; >>> bool hs400_cmd_resp_sel_rising; >>> /* cmd response sample selection for >>> HS400 */ >>> bool hs400_mode; /* current eMMC will run at hs400 mode */ >>> @@ -1615,7 +1621,7 @@ static irqreturn_t msdc_cmdq_irq(struct >>> msdc_host *host, u32 intsts) >>> } >>> >>> if (cmd_err || dat_err) { >>> - dev_err(host->dev, "cmd_err = %d, dat_err =%d, intsts = >>> 0x%x", >>> + dev_err(host->dev, "cmd_err = %d, dat_err = %d, intsts >>> = 0x%x", >>> cmd_err, dat_err, intsts); >>> } >>> >>> @@ -1780,10 +1786,20 @@ static void msdc_init_hw(struct msdc_host >>> *host) >>> DATA_K_VALUE_SEL); >>> sdr_set_bits(host->top_base + EMMC_TOP_CMD, >>> PAD_CMD_RD_RXDLY_SEL); >>> + if (host->tuning_step > PAD_DELAY_HALF) { >>> + sdr_set_bits(host->top_base + >>> EMMC_TOP_CONTROL, >>> + PAD_DAT_RD_RXDLY2_SEL); >>> + sdr_set_bits(host->top_base + >>> EMMC_TOP_CMD, >>> + PAD_CMD_RD_RXDLY2_SEL); >>> + } >>> } else { >>> sdr_set_bits(host->base + tune_reg, >>> MSDC_PAD_TUNE_RD_SEL | >>> MSDC_PAD_TUNE_CMD_SEL); >>> + if (host->tuning_step > PAD_DELAY_HALF) >>> + sdr_set_bits(host->base + tune_reg + 4, >> >> `tune_reg + 4` is a different register, please define it. >> > The tune_reg is not a fixed register address, it is defined in > compatible structures. So using the offset here will make the code more > consise. The offset of registers related to 64 steps tuning is fixed > relative to 32-steps tuning regsiter, the offset is always 4. > > However. using the magic number '4' here directly is not ideal. I think > I can improve this part of code by defining '4' as a macro. What do you > think about it? If you insist on redifning the registers, I can do it, > but it will make the code a bit more complex than it is now. > > >> Also, I can't find this in MT8192, MT8195 - as those bits seem to be >> undefined, >> so, which SoCs are actually compatible with this change? > > Sorry, which bits are you talking about? > This change compatible for all SoCs. In fact, MSDC has always supported > 64 step tuning. > > Sorry, found it under a slightly different name. Nevermind. >> >> >>> + MSDC_PAD_TUNE_RD2_SEL | >>> + MSDC_PAD_TUNE_CMD2_SEL); >>> } >>> } else { >>> /* choose clock tune */ >>> @@ -1925,24 +1941,24 @@ static void msdc_ops_set_ios(struct >>> mmc_host *mmc, struct mmc_ios *ios) >>> msdc_set_mclk(host, ios->timing, ios->clock); >>> } >>> >>> -static u32 test_delay_bit(u32 delay, u32 bit) >>> +static u64 test_delay_bit(u64 delay, u32 bit) >>> { >>> - bit %= PAD_DELAY_MAX; >>> - return delay & BIT(bit); >>> + bit %= PAD_DELAY_FULL; >>> + return delay & BIT_ULL(bit); >>> } >>> >>> -static int get_delay_len(u32 delay, u32 start_bit) >>> +static int get_delay_len(u64 delay, u32 start_bit) >>> { >>> int i; >>> >>> - for (i = 0; i < (PAD_DELAY_MAX - start_bit); i++) { >>> + for (i = 0; i < (PAD_DELAY_FULL - start_bit); i++) { >>> if (test_delay_bit(delay, start_bit + i) == 0) >>> return i; >>> } >>> - return PAD_DELAY_MAX - start_bit; >>> + return PAD_DELAY_FULL - start_bit; >>> } >>> >>> -static struct msdc_delay_phase get_best_delay(struct msdc_host >>> *host, u32 delay) >>> +static struct msdc_delay_phase get_best_delay(struct msdc_host >>> *host, u64 delay) >>> { >>> int start = 0, len = 0; >>> int start_final = 0, len_final = 0; >>> @@ -1950,28 +1966,28 @@ static struct msdc_delay_phase >>> get_best_delay(struct msdc_host *host, u32 delay) >>> struct msdc_delay_phase delay_phase = { 0, }; >>> >>> if (delay == 0) { >>> - dev_err(host->dev, "phase error: [map:%x]\n", delay); >>> + dev_err(host->dev, "phase error: [map:%016llx]\n", >>> delay); >>> delay_phase.final_phase = final_phase; >>> return delay_phase; >>> } >>> >>> - while (start < PAD_DELAY_MAX) { >>> + while (start < PAD_DELAY_FULL) { >>> len = get_delay_len(delay, start); >>> if (len_final < len) { >>> start_final = start; >>> len_final = len; >>> } >>> start += len ? len : 1; >>> - if (len >= 12 && start_final < 4) >>> + if (!upper_32_bits(delay) && len >= 12 && start_final < >>> 4) >>> break; >>> } >>> >>> /* The rule is that to find the smallest delay cell */ >>> if (start_final == 0) >>> - final_phase = (start_final + len_final / 3) % >>> PAD_DELAY_MAX; >>> + final_phase = (start_final + len_final / 3) % >>> PAD_DELAY_FULL; >>> else >>> - final_phase = (start_final + len_final / 2) % >>> PAD_DELAY_MAX; >>> - dev_dbg(host->dev, "phase: [map:%x] [maxlen:%d] [final:%d]\n", >>> + final_phase = (start_final + len_final / 2) % >>> PAD_DELAY_FULL; >>> + dev_dbg(host->dev, "phase: [map:%016llx] [maxlen:%d] >>> [final:%d]\n", >>> delay, len_final, final_phase); >>> >>> delay_phase.maxlen = len_final; >>> @@ -1984,30 +2000,68 @@ static inline void >>> msdc_set_cmd_delay(struct msdc_host *host, u32 value) >>> { >>> u32 tune_reg = host->dev_comp->pad_tune_reg; >>> >>> - if (host->top_base) >>> - sdr_set_field(host->top_base + EMMC_TOP_CMD, >>> PAD_CMD_RXDLY, >>> - value); >>> - else >>> - sdr_set_field(host->base + tune_reg, >>> MSDC_PAD_TUNE_CMDRDLY, >>> - value); >>> + if (host->top_base) { >>> + if (value < PAD_DELAY_HALF) { >>> + sdr_set_field(host->top_base + EMMC_TOP_CMD, >>> PAD_CMD_RXDLY, >>> + value); >> >> This goes up to 92 columns, and it's fine, so fits in one line and >> it's more >> readable like that. >> >> I know that's not your fault, but since you're actually touching >> those lines >> it's a good occasion to also do that (not only here) :-) >> > Sure, will update this part in v4, and thanks for your meticulous > review. > Thank you! Angelo ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-12-05 13:24 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-11-30 6:15 [PATCH v3 0/2] mmc: mediatek: add support for 64-steps tuning Axe Yang 2023-11-30 6:15 ` [PATCH v3 1/2] dt-bindings: mmc: mtk-sd: add tuning steps related property Axe Yang 2023-11-30 6:15 ` [PATCH v3 2/2] mmc: mediatek: extend number of tuning steps Axe Yang 2023-12-01 9:02 ` AngeloGioacchino Del Regno 2023-12-05 2:02 ` Axe Yang (杨磊) 2023-12-05 13:24 ` AngeloGioacchino Del Regno
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).