Dear Peng Fan,
Some other remarks.
On Tue, May 29, 2018 at 3:45 AM, Peng Fan peng.fan@nxp.com wrote:
-----Original Message----- From: Benoît Thébaudeau [mailto:benoit.thebaudeau.dev@gmail.com] Sent: 2018年5月29日 6:32 To: Peng Fan peng.fan@nxp.com Cc: sbabic@denx.de; Fabio Estevam fabio.estevam@nxp.com; U-Boot u-boot@lists.denx.de; Bough Chen haibo.chen@nxp.com Subject: Re: [U-Boot] [PATCH 37/41] mmc: fsl_esdhc: fix sd/mmc ddr mode clock setting issue
Dear Peng Fan,
On Mon, May 28, 2018 at 2:25 PM, Peng Fan peng.fan@nxp.com wrote:
From: Ye Li ye.li@nxp.com
When sd/mmc work at DDR mode, like HS400/HS400ES/DDR52/DDR50 mode,
the
actual clock rate is just half of the expected clock.
This patch set the DDR_EN bit first for DDR mode, hardware divide the usdhc clock automatically, then follow the original sdr clock setting method.
Signed-off-by: Haibo Chen haibo.chen@nxp.com Signed-off-by: Ye Li ye.li@nxp.com
drivers/mmc/fsl_esdhc.c | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-)
diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c index 1b062ff06d..bf4ae74847 100644 --- a/drivers/mmc/fsl_esdhc.c +++ b/drivers/mmc/fsl_esdhc.c @@ -583,18 +583,32 @@ static void set_sysctl(struct fsl_esdhc_priv *priv, struct mmc *mmc, uint clock) #else int pre_div = 2; #endif
int ddr_pre_div = mmc->ddr_mode ? 2 : 1; int sdhc_clk = priv->sdhc_clk; uint clk;
/** For ddr mode, usdhc need to enable DDR mode first, after select* this DDR mode, usdhc will automatically divide the usdhc clock*/if (mmc->ddr_mode) {writel(readl(®s->mixctrl) | MIX_CTRL_DDREN,®s->mixctrl);
Isn't this redundant with what is done in esdhc_set_timing()? According to the reference manual, the prescalers might have to be set after DDREN (as done here), so perhaps DDREN does not have to be set too in esdhc_set_timing().
sdhc_clk >>= 1;}if (clock < mmc->cfg->f_min) clock = mmc->cfg->f_min;
while (sdhc_clk / (16 * pre_div * ddr_pre_div) > clock && pre_div <
pre_div *= 2;
if ((sdhc_clk / 16) > clock) {for (; pre_div < 256; pre_div *= 2)if ((sdhc_clk / pre_div) <= (clock * 16))break;} else {pre_div = 1;This value is not always available for this divider. See the conditions in the initialization of this variable giving its minimum value.
The else {pre_div = 1;} could be removed then.
And the if conditioning the for loop becomes useless. In the end, the new loop would be equivalent to the previous while loop.
}
while (sdhc_clk / (div * pre_div * ddr_pre_div) > clock && div < 16)div++;
for (div = 1; div <= 16; div++)
div is already initialized in its definition, so only one of these initializations can be kept.
if ((sdhc_clk / (div * pre_div)) <= clock)break;
The only difference with the previous while loop is the "<= 16", which is correct, but maybe the change could have been limited to that.
pre_div >>= 1; div -= 1;
Best regards, Benoît