Message ID | 20250123074442.1617-1-andy-ld.lu@mediatek.com (mailing list archive) |
---|---|
State | New |
Headers | show |
Series | mmc: mtk-sd: optimize register settings for hs400(es) mode | expand |
Il 23/01/25 08:44, Andy-ld Lu ha scritto: > For hs400(es) mode, the 'hs400-ds-delay' is typically configured in the > dts. However, some projects may only define 'mediatek,hs400-ds-dly3', > which can lead to initialization failures in hs400es mode. CMD13 reported > response crc error in the mmc_switch_status() just after switching to > hs400es mode. > > [ 1.914038][ T82] mmc0: mmc_select_hs400es failed, error -84 > [ 1.914954][ T82] mmc0: error -84 whilst initialising MMC card > > Currently, the hs400_ds_dly3 value is set within the tuning function. This > means that the PAD_DS_DLY3 field is not configured before tuning process, > which is the reason for the above-mentioned CMD13 response crc error. > > Move the PAD_DS_DLY3 field configuration into msdc_prepare_hs400_tuning(), > and add a value check of hs400_ds_delay to prevent overwriting by zero when > the 'hs400-ds-delay' is not set in the dts. In addition, since hs400(es) > only tune the PAD_DS_DLY1, the PAD_DS_DLY2_SEL bit should be cleared to > bypass it. The commit title is a bit misleading: you're not "optimizing" the settings, but actually "fixing" a bug here! Please fix the commit title. > > Signed-off-by: Andy-ld Lu <andy-ld.lu@mediatek.com> This commit needs a Fixes tag, please add the relevant one and resend. > --- > drivers/mmc/host/mtk-sd.c | 37 +++++++++++++++++++++++-------------- > 1 file changed, 23 insertions(+), 14 deletions(-) > > diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c > index 4b6e91372526..4e3e31ddf84b 100644 > --- a/drivers/mmc/host/mtk-sd.c > +++ b/drivers/mmc/host/mtk-sd.c > @@ -273,6 +273,7 @@ > #define MSDC_PAD_TUNE_CMD2_SEL BIT(21) /* RW */ > > #define PAD_DS_TUNE_DLY_SEL BIT(0) /* RW */ > +#define PAD_DS_TUNE_DLY2_SEL BIT(1) /* RW */ > #define PAD_DS_TUNE_DLY1 GENMASK(6, 2) /* RW */ > #define PAD_DS_TUNE_DLY2 GENMASK(11, 7) /* RW */ > #define PAD_DS_TUNE_DLY3 GENMASK(16, 12) /* RW */ > @@ -317,9 +318,10 @@ > #define PAD_CMD_TX_DLY GENMASK(16, 12) /* RW */ > > /* EMMC50_PAD_DS_TUNE mask */ > -#define PAD_DS_DLY_SEL BIT(16) /* RW */ > -#define PAD_DS_DLY1 GENMASK(14, 10) /* RW */ > -#define PAD_DS_DLY3 GENMASK(4, 0) /* RW */ > +#define PAD_DS_DLY_SEL BIT(16) /* RW */ > +#define PAD_DS_DLY2_SEL BIT(15) /* RW */ > +#define PAD_DS_DLY1 GENMASK(14, 10) /* RW */ > +#define PAD_DS_DLY3 GENMASK(4, 0) /* RW */ Why are you changing tabulations with spaces? Please use tabs. Cheers, Angelo
On Thu, 2025-01-23 at 09:44 +0100, AngeloGioacchino Del Regno wrote: > External email : Please do not click links or open attachments until > you have verified the sender or the content. > > > Il 23/01/25 08:44, Andy-ld Lu ha scritto: > > For hs400(es) mode, the 'hs400-ds-delay' is typically configured in > > the > > dts. However, some projects may only define 'mediatek,hs400-ds- > > dly3', > > which can lead to initialization failures in hs400es mode. CMD13 > > reported > > response crc error in the mmc_switch_status() just after switching > > to > > hs400es mode. > > > > [ 1.914038][ T82] mmc0: mmc_select_hs400es failed, error -84 > > [ 1.914954][ T82] mmc0: error -84 whilst initialising MMC card > > > > Currently, the hs400_ds_dly3 value is set within the tuning > > function. This > > means that the PAD_DS_DLY3 field is not configured before tuning > > process, > > which is the reason for the above-mentioned CMD13 response crc > > error. > > > > Move the PAD_DS_DLY3 field configuration into > > msdc_prepare_hs400_tuning(), > > and add a value check of hs400_ds_delay to prevent overwriting by > > zero when > > the 'hs400-ds-delay' is not set in the dts. In addition, since > > hs400(es) > > only tune the PAD_DS_DLY1, the PAD_DS_DLY2_SEL bit should be > > cleared to > > bypass it. > > The commit title is a bit misleading: you're not "optimizing" the > settings, > but actually "fixing" a bug here! > > Please fix the commit title. > > > > > Signed-off-by: Andy-ld Lu <andy-ld.lu@mediatek.com> > > This commit needs a Fixes tag, please add the relevant one and > resend. > Thanks for your review, I will update in next change. > > > --- > > drivers/mmc/host/mtk-sd.c | 37 +++++++++++++++++++++++----------- > > --- > > 1 file changed, 23 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c > > index 4b6e91372526..4e3e31ddf84b 100644 > > --- a/drivers/mmc/host/mtk-sd.c > > +++ b/drivers/mmc/host/mtk-sd.c > > @@ -273,6 +273,7 @@ > > #define MSDC_PAD_TUNE_CMD2_SEL BIT(21) /* RW */ > > > > #define PAD_DS_TUNE_DLY_SEL BIT(0) /* RW */ > > +#define PAD_DS_TUNE_DLY2_SEL BIT(1) /* RW */ > > #define PAD_DS_TUNE_DLY1 GENMASK(6, 2) /* RW */ > > #define PAD_DS_TUNE_DLY2 GENMASK(11, 7) /* RW */ > > #define PAD_DS_TUNE_DLY3 GENMASK(16, 12) /* RW */ > > @@ -317,9 +318,10 @@ > > #define PAD_CMD_TX_DLY GENMASK(16, 12) /* RW */ > > > > /* EMMC50_PAD_DS_TUNE mask */ > > -#define PAD_DS_DLY_SEL BIT(16) /* RW */ > > -#define PAD_DS_DLY1 GENMASK(14, 10) /* RW */ > > -#define PAD_DS_DLY3 GENMASK(4, 0) /* RW */ > > +#define PAD_DS_DLY_SEL BIT(16) /* RW */ > > +#define PAD_DS_DLY2_SEL BIT(15) /* RW */ > > +#define PAD_DS_DLY1 GENMASK(14, 10) /* RW */ > > +#define PAD_DS_DLY3 GENMASK(4, 0) /* RW */ > > Why are you changing tabulations with spaces? > Please use tabs. > > Cheers, > Angelo > Because the new added line seems a little misaligned in the patch file, I have used spaces instead. However, I will change back to tabs in next change. Best regards Andy-ld Lu
diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c index 4b6e91372526..4e3e31ddf84b 100644 --- a/drivers/mmc/host/mtk-sd.c +++ b/drivers/mmc/host/mtk-sd.c @@ -273,6 +273,7 @@ #define MSDC_PAD_TUNE_CMD2_SEL BIT(21) /* RW */ #define PAD_DS_TUNE_DLY_SEL BIT(0) /* RW */ +#define PAD_DS_TUNE_DLY2_SEL BIT(1) /* RW */ #define PAD_DS_TUNE_DLY1 GENMASK(6, 2) /* RW */ #define PAD_DS_TUNE_DLY2 GENMASK(11, 7) /* RW */ #define PAD_DS_TUNE_DLY3 GENMASK(16, 12) /* RW */ @@ -317,9 +318,10 @@ #define PAD_CMD_TX_DLY GENMASK(16, 12) /* RW */ /* EMMC50_PAD_DS_TUNE mask */ -#define PAD_DS_DLY_SEL BIT(16) /* RW */ -#define PAD_DS_DLY1 GENMASK(14, 10) /* RW */ -#define PAD_DS_DLY3 GENMASK(4, 0) /* RW */ +#define PAD_DS_DLY_SEL BIT(16) /* RW */ +#define PAD_DS_DLY2_SEL BIT(15) /* RW */ +#define PAD_DS_DLY1 GENMASK(14, 10) /* RW */ +#define PAD_DS_DLY3 GENMASK(4, 0) /* RW */ /* LOOP_TEST_CONTROL mask */ #define TEST_LOOP_DSCLK_MUX_SEL BIT(0) /* RW */ @@ -2504,13 +2506,23 @@ static int msdc_execute_tuning(struct mmc_host *mmc, u32 opcode) static int msdc_prepare_hs400_tuning(struct mmc_host *mmc, struct mmc_ios *ios) { struct msdc_host *host = mmc_priv(mmc); + host->hs400_mode = true; - if (host->top_base) - writel(host->hs400_ds_delay, - host->top_base + EMMC50_PAD_DS_TUNE); - else - writel(host->hs400_ds_delay, host->base + PAD_DS_TUNE); + if (host->top_base) { + if (host->hs400_ds_dly3) + sdr_set_field(host->top_base + EMMC50_PAD_DS_TUNE, + PAD_DS_DLY3, host->hs400_ds_dly3); + if (host->hs400_ds_delay) + writel(host->hs400_ds_delay, + host->top_base + EMMC50_PAD_DS_TUNE); + } else { + if (host->hs400_ds_dly3) + sdr_set_field(host->base + PAD_DS_TUNE, + PAD_DS_TUNE_DLY3, host->hs400_ds_dly3); + if (host->hs400_ds_delay) + writel(host->hs400_ds_delay, host->base + PAD_DS_TUNE); + } /* hs400 mode must set it to 0 */ sdr_clr_bits(host->base + MSDC_PATCH_BIT2, MSDC_PATCH_BIT2_CFGCRCSTS); /* to improve read performance, set outstanding to 2 */ @@ -2530,14 +2542,11 @@ static int msdc_execute_hs400_tuning(struct mmc_host *mmc, struct mmc_card *card if (host->top_base) { sdr_set_bits(host->top_base + EMMC50_PAD_DS_TUNE, PAD_DS_DLY_SEL); - if (host->hs400_ds_dly3) - sdr_set_field(host->top_base + EMMC50_PAD_DS_TUNE, - PAD_DS_DLY3, host->hs400_ds_dly3); + sdr_clr_bits(host->top_base + EMMC50_PAD_DS_TUNE, + PAD_DS_DLY2_SEL); } else { sdr_set_bits(host->base + PAD_DS_TUNE, PAD_DS_TUNE_DLY_SEL); - if (host->hs400_ds_dly3) - sdr_set_field(host->base + PAD_DS_TUNE, - PAD_DS_TUNE_DLY3, host->hs400_ds_dly3); + sdr_clr_bits(host->base + PAD_DS_TUNE, PAD_DS_TUNE_DLY2_SEL); } host->hs400_tuning = true;
For hs400(es) mode, the 'hs400-ds-delay' is typically configured in the dts. However, some projects may only define 'mediatek,hs400-ds-dly3', which can lead to initialization failures in hs400es mode. CMD13 reported response crc error in the mmc_switch_status() just after switching to hs400es mode. [ 1.914038][ T82] mmc0: mmc_select_hs400es failed, error -84 [ 1.914954][ T82] mmc0: error -84 whilst initialising MMC card Currently, the hs400_ds_dly3 value is set within the tuning function. This means that the PAD_DS_DLY3 field is not configured before tuning process, which is the reason for the above-mentioned CMD13 response crc error. Move the PAD_DS_DLY3 field configuration into msdc_prepare_hs400_tuning(), and add a value check of hs400_ds_delay to prevent overwriting by zero when the 'hs400-ds-delay' is not set in the dts. In addition, since hs400(es) only tune the PAD_DS_DLY1, the PAD_DS_DLY2_SEL bit should be cleared to bypass it. Signed-off-by: Andy-ld Lu <andy-ld.lu@mediatek.com> --- drivers/mmc/host/mtk-sd.c | 37 +++++++++++++++++++++++-------------- 1 file changed, 23 insertions(+), 14 deletions(-)