Re: [PATCH] mmc: dw_mmc-k3: Fix DDR52 mode by setting required clock divisor

[zhangfei]

+ Hisilicon colleague


On 2018å04æ05æ 08:51, Shawn Lin wrote:
> [+ Zhangfei Gao who added support for hi6220]
>
> On 2018/4/4 23:31, Ryan Grachek wrote:
> > On Tue, Apr 3, 2018 at 6:31 AM, Shawn Lin <shawn.lin@xxxxxxxxxxxxxx 
> > <mailto:shawn.lin@xxxxxxxxxxxxxx>> wrote:
> >
> > ÂÂÂ On 2018/3/30 2:24, oscardagrach wrote:
> >
> > ÂÂÂ Need at least one line commit body.
> >
> > ÂÂÂÂÂÂÂ Signed-off-by: oscardagrach <ryan@xxxxxxxxx 
> > <mailto:ryan@xxxxxxxxx>>
> > ÂÂÂÂÂÂÂ ---
> > ÂÂÂÂÂÂÂÂ Â drivers/mmc/host/dw_mmc-k3.c | 10 ++++++++--
> > ÂÂÂÂÂÂÂÂ Â 1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > ÂÂÂÂÂÂÂ diff --git a/drivers/mmc/host/dw_mmc-k3.c
> > ÂÂÂÂÂÂÂ b/drivers/mmc/host/dw_mmc-k3.c
> > ÂÂÂÂÂÂÂ index 89cdb3d533bb..efc546cb4db8 100644
> > ÂÂÂÂÂÂÂ --- a/drivers/mmc/host/dw_mmc-k3.c
> > ÂÂÂÂÂÂÂ +++ b/drivers/mmc/host/dw_mmc-k3.c
> > ÂÂÂÂÂÂÂ @@ -194,8 +194,14 @@ static void dw_mci_hi6220_set_ios(struct
> > ÂÂÂÂÂÂÂ dw_mci *host, struct mmc_ios *ios)
> > ÂÂÂÂÂÂÂÂ Â Â Â Â int ret;
> > ÂÂÂÂÂÂÂÂ Â Â Â Â unsigned int clock;
> > ÂÂÂÂÂÂÂÂ Â -Â Â Âclock = (ios->clock <= 25000000) ? 25000000 : 
> > ios->clock;
> > ÂÂÂÂÂÂÂ -
> > ÂÂÂÂÂÂÂ +Â Â Â Â/* CLKDIV must be 1 for DDR52/8-bit mode */
> > ÂÂÂÂÂÂÂ +Â Â Â Âif (ios->bus_width == MMC_BUS_WIDTH_8 &&
> > ÂÂÂÂÂÂÂ +Â Â Â Â Â Â Â Âios->timing == MMC_TIMING_MMC_DDR52) {
> > ÂÂÂÂÂÂÂ +Â Â Â Â Â Â Â Âmci_writel(host, CLKDIV, 0x1);
> > ÂÂÂÂÂÂÂ +Â Â Â Â Â Â Â Âclock = ios->clock;
> > ÂÂÂÂÂÂÂ +Â Â Â Â} else {
> > ÂÂÂÂÂÂÂ +Â Â Â Â Â Â Â Âclock = (ios->clock <= 25000000) ? 25000000 :
> > ÂÂÂÂÂÂÂ ios->clock;
> > ÂÂÂÂÂÂÂ +Â Â Â Â}
> >
> >
> > ÂÂÂ I undertand DDR52/8-bit need CLKDIV fixed 1, but shouldn't the 
> > following
> > ÂÂÂ change is more sensible?
> >
> > ÂÂÂ if (ios->bus_width == MMC_BUS_WIDTH_8 && ios->timing ==
> > ÂÂÂ MMC_TIMING_MMC_DDR52)
> > ÂÂÂÂ Â Â Â Â clock = ios->clock * 2;
> > ÂÂÂ else
> > ÂÂÂÂ Â Â Â Â clock = (ios->clock <= 25000000) ? 25000000 : ios->clock;
> >
> >
> > ÂÂÂ The reason is ios->clock is 52MHz and you could claim 104MHz from 
> > the
> > ÂÂÂ clock provider and let dw_mmc core take care of the divder to be 1.
> > ÂÂÂ Otherwise, you just force it to be DDR52/8-bit with a clk rate of 
> > 26MHz.
> >
> >
> > ÂÂÂÂÂÂÂÂ Â Â Â Â ret = clk_set_rate(host->biu_clk, clock);
> > ÂÂÂÂÂÂÂÂ Â Â Â Â if (ret)
> > ÂÂÂÂÂÂÂÂ Â Â Â Â Â Â Â Â dev_warn(host->dev, "failed to set rate
> > ÂÂÂÂÂÂÂ %uHz\n", clock);
>
> For future wise, please use plain mode mail, but not HTML format.
>
> > Your feedback is correct. I see the Rockchip dwmmc driver has a similar
> > implementation. After applying your suggested changes, however, my board
> > reports "dwmmc_k3 f723d000.dwmmc0: failed to set rate 104000000Hz"
> > during intialization of eMMC. In addition, I do not see CLKDIV being
> > set to 1. clk_set_rate fails and I wonder if this is out-of-scope of
> > the driver.
> >
> > If I set CLKDIV where I did prior, with your changes, the device fails
> > to set the clock and falls back to 52 MHz (26 MHz) and works fine, but
> > again, CLKDIV is reported as 0 (even though it is 1.) One thing of
> > interest to note is when I manually set the clock by doing:
> > (echo 104000000 > /sys/kernel/debug/mmc0/clock) the device reports back
> > 'mmc_host mmc0: Bus speed (slot 0) = 198400000Hz (slot req 104000000Hz,
> > ÂÂactual 99200000HZ div = 1)' which works reliably and clk_set_rate does
> > not report any error.
>
> When looking closely into the code, at least dw_mci_hi6220_set_ios
> goes wrong with the bus_hz, since it should be ciu_clk but not biu_clk.
> "b" stands for bus, and "c" stands for card IMHO, however bus_hz
> describs the clock to the card, provided by controller. Does the
> following patch help?
>
>
> diff --git a/drivers/mmc/host/dw_mmc-k3.c b/drivers/mmc/host/dw_mmc-k3.c
> index 89cdb3d..9e78cf2 100644
> --- a/drivers/mmc/host/dw_mmc-k3.c
> +++ b/drivers/mmc/host/dw_mmc-k3.c
> @@ -194,13 +194,21 @@ static void dw_mci_hi6220_set_ios(struct dw_mci 
> *host, struct mmc_ios *ios)
> ÂÂÂÂÂÂÂ int ret;
> ÂÂÂÂÂÂÂ unsigned int clock;
>
> -ÂÂÂÂÂÂ clock = (ios->clock <= 25000000) ? 25000000 : ios->clock;
> +ÂÂÂÂÂÂ if (ios->bus_width == MMC_BUS_WIDTH_8 &&
> +ÂÂÂÂÂÂÂÂÂÂ ios->timing == MMC_TIMING_MMC_DDR52)
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ clock = ios->clock * 2;
> +ÂÂÂÂÂÂ else
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ clock = (ios->clock <= 25000000) ? 25000000 : ios->clock;
>
> -ÂÂÂÂÂÂ ret = clk_set_rate(host->biu_clk, clock);
> +ÂÂÂÂÂÂ ret = clk_set_rate(host->ciu_clk, clock);
> ÂÂÂÂÂÂÂ if (ret)
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ dev_warn(host->dev, "failed to set rate %uHz\n", clock);
>
> -ÂÂÂÂÂÂ host->bus_hz = clk_get_rate(host->biu_clk);
> +ÂÂÂÂÂÂ clock = clk_get_rate(host->ciu_clk);
> +ÂÂÂÂÂÂ if (clock != host->bus_hz) {
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ host->bus_hz = clock;
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ host->current_speed = 0;
> +ÂÂÂÂÂÂ }
> Â}
>
>
> > I am not sure where to begin debugging these clock issues and welcome
> > any feedback.