Re: [PATCH 1/2] mmc: mediatek: Use data tune for CMD line tune
On Thu, 2017-01-12 at 11:39 +0100, Ulf Hansson wrote:
> On 12 January 2017 at 11:04, Yong Mao wrote:
> > From: yong mao
> >
> > CMD response CRC error may cause cannot boot up
> > Change to use data tune for CMD line
> > Separate cmd internal delay for HS200/HS400 mode
>
> Please try to work a little bit on improving the change log. Moreover
> as this is a fix for a regression (it seems like so?), please try to
> make that clear.
This change can fix CMD respose CRC issue in our platform.
I will try to make it clear in next version.
>
> >
> > Signed-off-by: Yong Mao
> > Signed-off-by: Chaotian Jing
> > ---
> > arch/arm64/boot/dts/mediatek/mt8173-evb.dts |3 +
>
> Changes to the DTS files should be a separate change. Please split it
> into its own patch.
>
> > drivers/mmc/host/mtk-sd.c | 169
> > +++
> > 2 files changed, 149 insertions(+), 23 deletions(-)
> >
> > diff --git a/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
> > b/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
> > index 0ecaad4..29c3100 100644
> > --- a/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
> > +++ b/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
> > @@ -134,6 +134,9 @@
> > bus-width = <8>;
> > max-frequency = <5000>;
> > cap-mmc-highspeed;
> > + hs200-cmd-int-delay = <26>;
> > + hs400-cmd-int-delay = <14>;
> > + cmd-resp-sel = <0>; /* 0: rising, 1: falling */
> > vmmc-supply = <&mt6397_vemc_3v3_reg>;
> > vqmmc-supply = <&mt6397_vio18_reg>;
> > non-removable;
> > diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
> > index 80ba034..93eb395 100644
> > --- a/drivers/mmc/host/mtk-sd.c
> > +++ b/drivers/mmc/host/mtk-sd.c
> > @@ -75,6 +75,7 @@
> > #define MSDC_PATCH_BIT1 0xb4
> > #define MSDC_PAD_TUNE0xec
> > #define PAD_DS_TUNE 0x188
> > +#define PAD_CMD_TUNE 0x18c
> > #define EMMC50_CFG0 0x208
> >
> >
> > /*--*/
> > @@ -210,12 +211,17 @@
> > #define MSDC_PATCH_BIT_SPCPUSH(0x1 << 29) /* RW */
> > #define MSDC_PATCH_BIT_DECRCTMO (0x1 << 30) /* RW */
> >
> > -#define MSDC_PAD_TUNE_DATRRDLY (0x1f << 8) /* RW */
> > -#define MSDC_PAD_TUNE_CMDRDLY(0x1f << 16) /* RW */
> > +#define MSDC_PAD_TUNE_DATWRDLY(0x1f << 0) /* RW */
> > +#define MSDC_PAD_TUNE_DATRRDLY(0x1f << 8) /* RW */
> > +#define MSDC_PAD_TUNE_CMDRDLY (0x1f << 16) /* RW */
> > +#define MSDC_PAD_TUNE_CMDRRDLY(0x1f << 22) /* RW */
>
> Is there a white space change somewhere here? I don't see any changes
> to MSDC_PAD_TUNE_DATRRDLY and MSDC_PAD_TUNE_CMDRDLY.
>
Sorry. I will fix in next version.
> > +#define MSDC_PAD_TUNE_CLKTDLY (0x1f << 27) /* RW */
> >
> > -#define PAD_DS_TUNE_DLY1 (0x1f << 2) /* RW */
> > -#define PAD_DS_TUNE_DLY2 (0x1f << 7) /* RW */
> > -#define PAD_DS_TUNE_DLY3 (0x1f << 12) /* RW */
> > +#define PAD_DS_TUNE_DLY1 (0x1f << 2) /* RW */
> > +#define PAD_DS_TUNE_DLY2 (0x1f << 7) /* RW */
> > +#define PAD_DS_TUNE_DLY3 (0x1f << 12) /* RW */
> > +
>
> Ditto.
Ditto.
>
> > +#define PAD_CMD_TUNE_RX_DLY3 (0x1f << 1) /* RW */
> >
> > #define EMMC50_CFG_PADCMD_LATCHCK (0x1 << 0) /* RW */
> > #define EMMC50_CFG_CRCSTS_EDGE(0x1 << 3) /* RW */
> > @@ -236,7 +242,9 @@
> > #define CMD_TIMEOUT (HZ/10 * 5)/* 100ms x5 */
> > #define DAT_TIMEOUT (HZ* 5)/* 1000ms x5 */
> >
> > -#define PAD_DELAY_MAX 32 /* PAD delay cells */
> > +#define PAD_DELAY_MAX32 /* PAD delay cells */
>
> Ditto.
>
Ditto.
> > +#define ENOUGH_MARGIN_MIN12 /* Enough Margin */
> > +#define PREFER_START_POS_MAX 4 /* Prefer start position */
> >
> > /*--*/
> > /* Descriptor Structure
> > */
> >
> > /*--*/
> > @@ -284,12 +292,14 @@ struct msdc_save_para {
> > u32 patch_bit0;
> > u32 patch_bit1;
> > u32 pad_ds_tune;
> > + u32 pad_cmd_tune;
> > u32 emmc50_cfg0;
> > };
> >
> > struct msdc_tune_para {
> > u32 iocon;
> > u32 pad_tune;
> > + u32 pad_cmd_tune;
> > };
> >
> > struct msdc_delay_phase {
> > @@ -331,6 +341,9 @@ struct msdc_host {
> > unsigned char timing;
> > bool vqmmc_enabled;
> > u32 hs400_ds_delay;
> > + u32 hs200_cmd_int_delay; /* cmd internal delay for HS200/SDR104 */
> > + u32 hs400_cmd_int_delay; /* cmd internal delay for HS400 */
> > + u32 hs200_cmd_resp_sel; /* cmd response sample selection */
> > bool hs400_mode;/* current eMMC will run at hs400 mode */
> > struct msdc_save_para save_para; /* used when gate HCLK */
> > struct msdc_tune_para def_tune_para
Re: [PATCH 1/2] mmc: mediatek: Use data tune for CMD line tune
On 12 January 2017 at 11:04, Yong Mao wrote:
> From: yong mao
>
> CMD response CRC error may cause cannot boot up
> Change to use data tune for CMD line
> Separate cmd internal delay for HS200/HS400 mode
Please try to work a little bit on improving the change log. Moreover
as this is a fix for a regression (it seems like so?), please try to
make that clear.
>
> Signed-off-by: Yong Mao
> Signed-off-by: Chaotian Jing
> ---
> arch/arm64/boot/dts/mediatek/mt8173-evb.dts |3 +
Changes to the DTS files should be a separate change. Please split it
into its own patch.
> drivers/mmc/host/mtk-sd.c | 169
> +++
> 2 files changed, 149 insertions(+), 23 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
> b/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
> index 0ecaad4..29c3100 100644
> --- a/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
> +++ b/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
> @@ -134,6 +134,9 @@
> bus-width = <8>;
> max-frequency = <5000>;
> cap-mmc-highspeed;
> + hs200-cmd-int-delay = <26>;
> + hs400-cmd-int-delay = <14>;
> + cmd-resp-sel = <0>; /* 0: rising, 1: falling */
> vmmc-supply = <&mt6397_vemc_3v3_reg>;
> vqmmc-supply = <&mt6397_vio18_reg>;
> non-removable;
> diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
> index 80ba034..93eb395 100644
> --- a/drivers/mmc/host/mtk-sd.c
> +++ b/drivers/mmc/host/mtk-sd.c
> @@ -75,6 +75,7 @@
> #define MSDC_PATCH_BIT1 0xb4
> #define MSDC_PAD_TUNE0xec
> #define PAD_DS_TUNE 0x188
> +#define PAD_CMD_TUNE 0x18c
> #define EMMC50_CFG0 0x208
>
>
> /*--*/
> @@ -210,12 +211,17 @@
> #define MSDC_PATCH_BIT_SPCPUSH(0x1 << 29) /* RW */
> #define MSDC_PATCH_BIT_DECRCTMO (0x1 << 30) /* RW */
>
> -#define MSDC_PAD_TUNE_DATRRDLY (0x1f << 8) /* RW */
> -#define MSDC_PAD_TUNE_CMDRDLY(0x1f << 16) /* RW */
> +#define MSDC_PAD_TUNE_DATWRDLY(0x1f << 0) /* RW */
> +#define MSDC_PAD_TUNE_DATRRDLY(0x1f << 8) /* RW */
> +#define MSDC_PAD_TUNE_CMDRDLY (0x1f << 16) /* RW */
> +#define MSDC_PAD_TUNE_CMDRRDLY(0x1f << 22) /* RW */
Is there a white space change somewhere here? I don't see any changes
to MSDC_PAD_TUNE_DATRRDLY and MSDC_PAD_TUNE_CMDRDLY.
> +#define MSDC_PAD_TUNE_CLKTDLY (0x1f << 27) /* RW */
>
> -#define PAD_DS_TUNE_DLY1 (0x1f << 2) /* RW */
> -#define PAD_DS_TUNE_DLY2 (0x1f << 7) /* RW */
> -#define PAD_DS_TUNE_DLY3 (0x1f << 12) /* RW */
> +#define PAD_DS_TUNE_DLY1 (0x1f << 2) /* RW */
> +#define PAD_DS_TUNE_DLY2 (0x1f << 7) /* RW */
> +#define PAD_DS_TUNE_DLY3 (0x1f << 12) /* RW */
> +
Ditto.
> +#define PAD_CMD_TUNE_RX_DLY3 (0x1f << 1) /* RW */
>
> #define EMMC50_CFG_PADCMD_LATCHCK (0x1 << 0) /* RW */
> #define EMMC50_CFG_CRCSTS_EDGE(0x1 << 3) /* RW */
> @@ -236,7 +242,9 @@
> #define CMD_TIMEOUT (HZ/10 * 5)/* 100ms x5 */
> #define DAT_TIMEOUT (HZ* 5)/* 1000ms x5 */
>
> -#define PAD_DELAY_MAX 32 /* PAD delay cells */
> +#define PAD_DELAY_MAX32 /* PAD delay cells */
Ditto.
> +#define ENOUGH_MARGIN_MIN12 /* Enough Margin */
> +#define PREFER_START_POS_MAX 4 /* Prefer start position */
>
> /*--*/
> /* Descriptor Structure
> */
>
> /*--*/
> @@ -284,12 +292,14 @@ struct msdc_save_para {
> u32 patch_bit0;
> u32 patch_bit1;
> u32 pad_ds_tune;
> + u32 pad_cmd_tune;
> u32 emmc50_cfg0;
> };
>
> struct msdc_tune_para {
> u32 iocon;
> u32 pad_tune;
> + u32 pad_cmd_tune;
> };
>
> struct msdc_delay_phase {
> @@ -331,6 +341,9 @@ struct msdc_host {
> unsigned char timing;
> bool vqmmc_enabled;
> u32 hs400_ds_delay;
> + u32 hs200_cmd_int_delay; /* cmd internal delay for HS200/SDR104 */
> + u32 hs400_cmd_int_delay; /* cmd internal delay for HS400 */
> + u32 hs200_cmd_resp_sel; /* cmd response sample selection */
> bool hs400_mode;/* current eMMC will run at hs400 mode */
> struct msdc_save_para save_para; /* used when gate HCLK */
> struct msdc_tune_para def_tune_para; /* default tune setting */
> @@ -596,12 +609,21 @@ static void msdc_set_mclk(struct msdc_host *host,
> unsigned char timing, u32 hz)
> */
> if (host->sclk <= 5200) {
> writel(host->def_tune_para.iocon, host->base + MSDC_IOCON);
> - writel(host->def_tune_para.pad_tune, host->base +
> MSDC_PAD_TUNE);
> + writel(host->def_tune_para.pad_tune,
> + host->base + MSDC_PAD_TU
