Re: [PATCH v2 2/2] mmc: mediatek: extend number of tuning steps

From: Axe Yang (杨磊)
Date: Tue Nov 28 2023 - 04:39:47 EST

Next message: Jiachen Zhang: "[PATCH 2/2] xfs: update dir3 leaf block metadata after swap"
Previous message: Tudor Ambarus: "Re: [PATCH] mtd: spi-nor: stop printing superfluous debug info"
In reply to: AngeloGioacchino Del Regno: "Re: [PATCH v2 2/2] mmc: mediatek: extend number of tuning steps"
Next in thread: AngeloGioacchino Del Regno: "Re: [PATCH v2 2/2] mmc: mediatek: extend number of tuning steps"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

> On Tue, 2023-11-28 at 09:53 +0100, AngeloGioacchino Del Regno wrote:
> Il 28/11/23 08:01, 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.
> >
>
> If we can run 64 tuning steps, why should we run 32?
>
> Why isn't it just better to *always* run 64 tuning steps, on SoCs
> supporting that?
>
> Thanks,
> Angelo

Hi Angelo,

That is a good question. The benefit of preserving 32 steps tuning is
that it can save time in certain scenarios.

On some platforms, when the delay selected through 64 steps tuning is
very close to that chosen through 32 steps, we can reduce the tuning
step from 64 to 32. This can save time sending the tuning block
commands.

Thus using 32 steps tuning can save kernel boot up time.

Another case where time can be saved is when accessing the RPMB
partition of eMMC. Each time switch to RPMB partition, there is a
retune action, causing a certain drop in performance. If we are certain
that the results of 32 steps tuning are usable and we use it, this can
in a sense also guarantee performance when accessing the RPMB
partition.

Regards,
Axe

>
> > Signed-off-by: Axe Yang <axe.yang@xxxxxxxxxxxx>
> > ---
> > drivers/mmc/host/mtk-sd.c | 135 +++++++++++++++++++++++++++----
> > -------
> > 1 file changed, 97 insertions(+), 38 deletions(-)
> >
> > diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
> > index 97f7c3d4be6e..c8297f501a1e 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;
> > + bit %= PAD_DELAY_FULL;
> > return delay & BIT(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:%llx]\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:%llx] [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,
> > @@ -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,
> > @@ -2082,7 +2136,7 @@ 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);
> > @@ -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,7 +2215,7 @@ 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)
> > @@ -2174,7 +2229,7 @@ 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)
> > @@ -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,7 +2272,7 @@ static int msdc_tune_together(struct mmc_host
> > *mmc, u32 opcode)
> > sdr_clr_bits(host->base + MSDC_IOCON, MSDC_IOCON_Ron the
> > supported SoCsSPL);
> > 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);
> > @@ -2233,7 +2288,7 @@ 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);
> > @@ -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;
>
>
>

Next message: Jiachen Zhang: "[PATCH 2/2] xfs: update dir3 leaf block metadata after swap"
Previous message: Tudor Ambarus: "Re: [PATCH] mtd: spi-nor: stop printing superfluous debug info"
In reply to: AngeloGioacchino Del Regno: "Re: [PATCH v2 2/2] mmc: mediatek: extend number of tuning steps"
Next in thread: AngeloGioacchino Del Regno: "Re: [PATCH v2 2/2] mmc: mediatek: extend number of tuning steps"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]