[PATCH 1/3] clk: rockchip: Fix wrong parents for MMC phase clock for rk3328

[PATCH 1/3] clk: rockchip: Fix wrong parents for MMC phase clock for rk3328

[Shawn Lin]

commit c420c1e4db22 ("clk: rockchip: Prevent calculating mmc phase if
clock rate is zero") catches some gremlins for clk-rk3328.c that the
parents of MMC phase clock should be clk_{sdmmc, sdio, emmc}, but not
sclk_{sdmmc, sdio, emmc}.

Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---

 drivers/clk/rockchip/clk-rk3328.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/clk/rockchip/clk-rk3328.c b/drivers/clk/rockchip/clk-rk3328.c
index f680b42..252366a 100644
--- a/drivers/clk/rockchip/clk-rk3328.c
+++ b/drivers/clk/rockchip/clk-rk3328.c
@@ -810,24 +810,24 @@ enum rk3328_plls {
 	GATE(0, "pclk_phy_niu", "pclk_phy_pre", 0, RK3328_CLKGATE_CON(15), 15, GFLAGS),
 
 	/* PD_MMC */
-	MMC(SCLK_SDMMC_DRV, "sdmmc_drv", "sclk_sdmmc",
+	MMC(SCLK_SDMMC_DRV, "sdmmc_drv", "clk_sdmmc",
 	    RK3328_SDMMC_CON0, 1),
-	MMC(SCLK_SDMMC_SAMPLE, "sdmmc_sample", "sclk_sdmmc",
+	MMC(SCLK_SDMMC_SAMPLE, "sdmmc_sample", "clk_sdmmc",
 	    RK3328_SDMMC_CON1, 1),
 
-	MMC(SCLK_SDIO_DRV, "sdio_drv", "sclk_sdio",
+	MMC(SCLK_SDIO_DRV, "sdio_drv", "clk_sdio",
 	    RK3328_SDIO_CON0, 1),
-	MMC(SCLK_SDIO_SAMPLE, "sdio_sample", "sclk_sdio",
+	MMC(SCLK_SDIO_SAMPLE, "sdio_sample", "clk_sdio",
 	    RK3328_SDIO_CON1, 1),
 
-	MMC(SCLK_EMMC_DRV, "emmc_drv", "sclk_emmc",
+	MMC(SCLK_EMMC_DRV, "emmc_drv", "clk_emmc",
 	    RK3328_EMMC_CON0, 1),
-	MMC(SCLK_EMMC_SAMPLE, "emmc_sample", "sclk_emmc",
+	MMC(SCLK_EMMC_SAMPLE, "emmc_sample", "clk_emmc",
 	    RK3328_EMMC_CON1, 1),
 
-	MMC(SCLK_SDMMC_EXT_DRV, "sdmmc_ext_drv", "sclk_sdmmc_ext",
+	MMC(SCLK_SDMMC_EXT_DRV, "sdmmc_ext_drv", "clk_sdmmc_ext",
 	    RK3328_SDMMC_EXT_CON0, 1),
-	MMC(SCLK_SDMMC_EXT_SAMPLE, "sdmmc_ext_sample", "sclk_sdmmc_ext",
+	MMC(SCLK_SDMMC_EXT_SAMPLE, "sdmmc_ext_sample", "clk_sdmmc_ext",
 	    RK3328_SDMMC_EXT_CON1, 1),
 };
 
-- 
1.9.1

[PATCH 1/3] clk: rockchip: Fix wrong parents for MMC phase clock for rk3328

[Shawn Lin]

commit c420c1e4db22 ("clk: rockchip: Prevent calculating mmc phase if
clock rate is zero") catches some gremlins for clk-rk3328.c that the
parents of MMC phase clock should be clk_{sdmmc, sdio, emmc}, but not
sclk_{sdmmc, sdio, emmc}.

Signed-off-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
---

 drivers/clk/rockchip/clk-rk3328.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/clk/rockchip/clk-rk3328.c b/drivers/clk/rockchip/clk-rk3328.c
index f680b42..252366a 100644
--- a/drivers/clk/rockchip/clk-rk3328.c
+++ b/drivers/clk/rockchip/clk-rk3328.c
@@ -810,24 +810,24 @@ enum rk3328_plls {
 	GATE(0, "pclk_phy_niu", "pclk_phy_pre", 0, RK3328_CLKGATE_CON(15), 15, GFLAGS),
 
 	/* PD_MMC */
-	MMC(SCLK_SDMMC_DRV, "sdmmc_drv", "sclk_sdmmc",
+	MMC(SCLK_SDMMC_DRV, "sdmmc_drv", "clk_sdmmc",
 	    RK3328_SDMMC_CON0, 1),
-	MMC(SCLK_SDMMC_SAMPLE, "sdmmc_sample", "sclk_sdmmc",
+	MMC(SCLK_SDMMC_SAMPLE, "sdmmc_sample", "clk_sdmmc",
 	    RK3328_SDMMC_CON1, 1),
 
-	MMC(SCLK_SDIO_DRV, "sdio_drv", "sclk_sdio",
+	MMC(SCLK_SDIO_DRV, "sdio_drv", "clk_sdio",
 	    RK3328_SDIO_CON0, 1),
-	MMC(SCLK_SDIO_SAMPLE, "sdio_sample", "sclk_sdio",
+	MMC(SCLK_SDIO_SAMPLE, "sdio_sample", "clk_sdio",
 	    RK3328_SDIO_CON1, 1),
 
-	MMC(SCLK_EMMC_DRV, "emmc_drv", "sclk_emmc",
+	MMC(SCLK_EMMC_DRV, "emmc_drv", "clk_emmc",
 	    RK3328_EMMC_CON0, 1),
-	MMC(SCLK_EMMC_SAMPLE, "emmc_sample", "sclk_emmc",
+	MMC(SCLK_EMMC_SAMPLE, "emmc_sample", "clk_emmc",
 	    RK3328_EMMC_CON1, 1),
 
-	MMC(SCLK_SDMMC_EXT_DRV, "sdmmc_ext_drv", "sclk_sdmmc_ext",
+	MMC(SCLK_SDMMC_EXT_DRV, "sdmmc_ext_drv", "clk_sdmmc_ext",
 	    RK3328_SDMMC_EXT_CON0, 1),
-	MMC(SCLK_SDMMC_EXT_SAMPLE, "sdmmc_ext_sample", "sclk_sdmmc_ext",
+	MMC(SCLK_SDMMC_EXT_SAMPLE, "sdmmc_ext_sample", "clk_sdmmc_ext",
 	    RK3328_SDMMC_EXT_CON1, 1),
 };
 
-- 
1.9.1

[PATCH 2/3] clk: rockchip: Fix wrong parent for SDMMC phase clock for rk3228

[Shawn Lin]

commit c420c1e4db22 ("clk: rockchip: Prevent calculating mmc phase
if clock rate is zero") catches one gremlin again for clk-rk3228.c
that the parent of SDMMC phase clock should be sclk_sdmmc0, but not
sclk_sdmmc. However, I don't like the name of sclk_sdmmc0, so I now
rename it to be sclk_sdmmc.

Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---

 drivers/clk/rockchip/clk-rk3228.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/rockchip/clk-rk3228.c b/drivers/clk/rockchip/clk-rk3228.c
index 11e7f2d..7af4818 100644
--- a/drivers/clk/rockchip/clk-rk3228.c
+++ b/drivers/clk/rockchip/clk-rk3228.c
@@ -387,7 +387,7 @@ enum rk3228_plls {
 			RK2928_CLKSEL_CON(23), 5, 2, MFLAGS, 0, 6, DFLAGS,
 			RK2928_CLKGATE_CON(2), 15, GFLAGS),
 
-	COMPOSITE(SCLK_SDMMC, "sclk_sdmmc0", mux_mmc_src_p, 0,
+	COMPOSITE(SCLK_SDMMC, "sclk_sdmmc", mux_mmc_src_p, 0,
 			RK2928_CLKSEL_CON(11), 8, 2, MFLAGS, 0, 8, DFLAGS,
 			RK2928_CLKGATE_CON(2), 11, GFLAGS),
 
-- 
1.9.1

[PATCH 2/3] clk: rockchip: Fix wrong parent for SDMMC phase clock for rk3228

[Shawn Lin]

commit c420c1e4db22 ("clk: rockchip: Prevent calculating mmc phase
if clock rate is zero") catches one gremlin again for clk-rk3228.c
that the parent of SDMMC phase clock should be sclk_sdmmc0, but not
sclk_sdmmc. However, I don't like the name of sclk_sdmmc0, so I now
rename it to be sclk_sdmmc.

Signed-off-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
---

 drivers/clk/rockchip/clk-rk3228.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/rockchip/clk-rk3228.c b/drivers/clk/rockchip/clk-rk3228.c
index 11e7f2d..7af4818 100644
--- a/drivers/clk/rockchip/clk-rk3228.c
+++ b/drivers/clk/rockchip/clk-rk3228.c
@@ -387,7 +387,7 @@ enum rk3228_plls {
 			RK2928_CLKSEL_CON(23), 5, 2, MFLAGS, 0, 6, DFLAGS,
 			RK2928_CLKGATE_CON(2), 15, GFLAGS),
 
-	COMPOSITE(SCLK_SDMMC, "sclk_sdmmc0", mux_mmc_src_p, 0,
+	COMPOSITE(SCLK_SDMMC, "sclk_sdmmc", mux_mmc_src_p, 0,
 			RK2928_CLKSEL_CON(11), 8, 2, MFLAGS, 0, 8, DFLAGS,
 			RK2928_CLKGATE_CON(2), 11, GFLAGS),
 
-- 
1.9.1

[PATCH 3/3] clk: rockchip: Correct the behaviour of restoring cached phase

[Shawn Lin]

We can't restore every phase, for instance the invalid phase and
the phase for coming rate which is out of the scope of boards'
ability. And this patch also corrects the error path to return
invalid pointer to clk if clk_notifier_register failed introduced
by the same offending commit.

Fixes: 60cf09e45fbc ("clk: rockchip: Restore the clock phase after the rate was changed")
Reported-by: wlq <wlq@rock-chips.com>
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
Tested-by: wlq <wlq@rock-chips.com>
---

 drivers/clk/rockchip/clk-mmc-phase.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/rockchip/clk-mmc-phase.c b/drivers/clk/rockchip/clk-mmc-phase.c
index dc4c227..026a26b 100644
--- a/drivers/clk/rockchip/clk-mmc-phase.c
+++ b/drivers/clk/rockchip/clk-mmc-phase.c
@@ -170,18 +170,30 @@ static int rockchip_mmc_clk_rate_notify(struct notifier_block *nb,
 					unsigned long event, void *data)
 {
 	struct rockchip_mmc_clock *mmc_clock = to_rockchip_mmc_clock(nb);
+	struct clk_notifier_data *ndata = data;
 
 	/*
 	 * rockchip_mmc_clk is mostly used by mmc controllers to sample
 	 * the intput data, which expects the fixed phase after the tuning
 	 * process. However if the clock rate is changed, the phase is stale
 	 * and may break the data sampling. So here we try to restore the phase
-	 * for that case.
+	 * for that case, except that
+	 * (1) cached_phase is invaild since we inevitably cached it when the
+	 * clock provider be reparented from orphan to its real parent in the
+	 * first place. Otherwise we may mess up the initialization of MMC cards
+	 * since we only set the default sample phase and drive phase later on.
+	 * (2) the new coming rate is higher than the older one since mmc driver
+	 * set the max-frequency to match the boards' ability but we can't go
+	 * over the heads of that, otherwise the tests smoke out the issue.
 	 */
+	if (ndata->old_rate <= ndata->new_rate)
+		return NOTIFY_DONE;
+
 	if (event == PRE_RATE_CHANGE)
 		mmc_clock->cached_phase =
 			rockchip_mmc_get_phase(&mmc_clock->hw);
-	else if (event == POST_RATE_CHANGE)
+	else if (mmc_clock->cached_phase != -EINVAL &&
+		 event == POST_RATE_CHANGE)
 		rockchip_mmc_set_phase(&mmc_clock->hw, mmc_clock->cached_phase);
 
 	return NOTIFY_DONE;
@@ -211,8 +223,10 @@ struct clk *rockchip_clk_register_mmc(const char *name,
 	mmc_clock->shift = shift;
 
 	clk = clk_register(NULL, &mmc_clock->hw);
-	if (IS_ERR(clk))
+	if (IS_ERR(clk)) {
+		ret = PTR_ERR(clk);
 		goto err_register;
+	}
 
 	mmc_clock->clk_rate_change_nb.notifier_call =
 				&rockchip_mmc_clk_rate_notify;
@@ -225,5 +239,5 @@ struct clk *rockchip_clk_register_mmc(const char *name,
 	clk_unregister(clk);
 err_register:
 	kfree(mmc_clock);
-	return clk;
+	return ERR_PTR(ret);
 }
-- 
1.9.1

[PATCH 3/3] clk: rockchip: Correct the behaviour of restoring cached phase

[Shawn Lin]

We can't restore every phase, for instance the invalid phase and
the phase for coming rate which is out of the scope of boards'
ability. And this patch also corrects the error path to return
invalid pointer to clk if clk_notifier_register failed introduced
by the same offending commit.

Fixes: 60cf09e45fbc ("clk: rockchip: Restore the clock phase after the rate was changed")
Reported-by: wlq <wlq-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
Signed-off-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
Tested-by: wlq <wlq-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
---

 drivers/clk/rockchip/clk-mmc-phase.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/rockchip/clk-mmc-phase.c b/drivers/clk/rockchip/clk-mmc-phase.c
index dc4c227..026a26b 100644
--- a/drivers/clk/rockchip/clk-mmc-phase.c
+++ b/drivers/clk/rockchip/clk-mmc-phase.c
@@ -170,18 +170,30 @@ static int rockchip_mmc_clk_rate_notify(struct notifier_block *nb,
 					unsigned long event, void *data)
 {
 	struct rockchip_mmc_clock *mmc_clock = to_rockchip_mmc_clock(nb);
+	struct clk_notifier_data *ndata = data;
 
 	/*
 	 * rockchip_mmc_clk is mostly used by mmc controllers to sample
 	 * the intput data, which expects the fixed phase after the tuning
 	 * process. However if the clock rate is changed, the phase is stale
 	 * and may break the data sampling. So here we try to restore the phase
-	 * for that case.
+	 * for that case, except that
+	 * (1) cached_phase is invaild since we inevitably cached it when the
+	 * clock provider be reparented from orphan to its real parent in the
+	 * first place. Otherwise we may mess up the initialization of MMC cards
+	 * since we only set the default sample phase and drive phase later on.
+	 * (2) the new coming rate is higher than the older one since mmc driver
+	 * set the max-frequency to match the boards' ability but we can't go
+	 * over the heads of that, otherwise the tests smoke out the issue.
 	 */
+	if (ndata->old_rate <= ndata->new_rate)
+		return NOTIFY_DONE;
+
 	if (event == PRE_RATE_CHANGE)
 		mmc_clock->cached_phase =
 			rockchip_mmc_get_phase(&mmc_clock->hw);
-	else if (event == POST_RATE_CHANGE)
+	else if (mmc_clock->cached_phase != -EINVAL &&
+		 event == POST_RATE_CHANGE)
 		rockchip_mmc_set_phase(&mmc_clock->hw, mmc_clock->cached_phase);
 
 	return NOTIFY_DONE;
@@ -211,8 +223,10 @@ struct clk *rockchip_clk_register_mmc(const char *name,
 	mmc_clock->shift = shift;
 
 	clk = clk_register(NULL, &mmc_clock->hw);
-	if (IS_ERR(clk))
+	if (IS_ERR(clk)) {
+		ret = PTR_ERR(clk);
 		goto err_register;
+	}
 
 	mmc_clock->clk_rate_change_nb.notifier_call =
 				&rockchip_mmc_clk_rate_notify;
@@ -225,5 +239,5 @@ struct clk *rockchip_clk_register_mmc(const char *name,
 	clk_unregister(clk);
 err_register:
 	kfree(mmc_clock);
-	return clk;
+	return ERR_PTR(ret);
 }
-- 
1.9.1

Re: [PATCH 2/3] clk: rockchip: Fix wrong parent for SDMMC phase clock for rk3228

[Heiko Stuebner]

Am Mittwoch, 21. März 2018, 03:39:19 CET schrieb Shawn Lin:
> commit c420c1e4db22 ("clk: rockchip: Prevent calculating mmc phase
> if clock rate is zero") catches one gremlin again for clk-rk3228.c
> that the parent of SDMMC phase clock should be sclk_sdmmc0, but not
> sclk_sdmmc. However, I don't like the name of sclk_sdmmc0, so I now
> rename it to be sclk_sdmmc.
> 
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>

applied to my clk-branch after adapting the commit message a bit,
so that it is less about personal taste and highlights the discrepancy
in the manual between clk_sdmmc0 and hclk_sdmmc (without 0)
and we thus standardize on the one without 0.


Heiko

Re: [PATCH 2/3] clk: rockchip: Fix wrong parent for SDMMC phase clock for rk3228

[Heiko Stuebner]

Am Mittwoch, 21. März 2018, 03:39:19 CET schrieb Shawn Lin:
> commit c420c1e4db22 ("clk: rockchip: Prevent calculating mmc phase
> if clock rate is zero") catches one gremlin again for clk-rk3228.c
> that the parent of SDMMC phase clock should be sclk_sdmmc0, but not
> sclk_sdmmc. However, I don't like the name of sclk_sdmmc0, so I now
> rename it to be sclk_sdmmc.
> 
> Signed-off-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>

applied to my clk-branch after adapting the commit message a bit,
so that it is less about personal taste and highlights the discrepancy
in the manual between clk_sdmmc0 and hclk_sdmmc (without 0)
and we thus standardize on the one without 0.


Heiko

Re: [PATCH 1/3] clk: rockchip: Fix wrong parents for MMC phase clock for rk3328

[Heiko Stuebner]

Am Mittwoch, 21. März 2018, 03:39:18 CET schrieb Shawn Lin:
> commit c420c1e4db22 ("clk: rockchip: Prevent calculating mmc phase if
> clock rate is zero") catches some gremlins for clk-rk3328.c that the
> parents of MMC phase clock should be clk_{sdmmc, sdio, emmc}, but not
> sclk_{sdmmc, sdio, emmc}.
> 
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>

applied for 4.17


Thanks
Heiko

Re: [PATCH 1/3] clk: rockchip: Fix wrong parents for MMC phase clock for rk3328

[Heiko Stuebner]

Am Mittwoch, 21. März 2018, 03:39:18 CET schrieb Shawn Lin:
> commit c420c1e4db22 ("clk: rockchip: Prevent calculating mmc phase if
> clock rate is zero") catches some gremlins for clk-rk3328.c that the
> parents of MMC phase clock should be clk_{sdmmc, sdio, emmc}, but not
> sclk_{sdmmc, sdio, emmc}.
> 
> Signed-off-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>

applied for 4.17


Thanks
Heiko

Re: [PATCH 3/3] clk: rockchip: Correct the behaviour of restoring cached phase

[Heiko Stuebner]

Am Mittwoch, 21. März 2018, 03:39:20 CET schrieb Shawn Lin:
> We can't restore every phase, for instance the invalid phase and
> the phase for coming rate which is out of the scope of boards'
> ability. And this patch also corrects the error path to return
> invalid pointer to clk if clk_notifier_register failed introduced
> by the same offending commit.
> 
> Fixes: 60cf09e45fbc ("clk: rockchip: Restore the clock phase after the rate was changed")
> Reported-by: wlq <wlq@rock-chips.com>
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> Tested-by: wlq <wlq@rock-chips.com>

I've split off the part below [fix for error handling] into a separate
patch and applied both patches for 4.17


Thanks
Heiko

> @@ -211,8 +223,10 @@ struct clk *rockchip_clk_register_mmc(const char *name,
>  	mmc_clock->shift = shift;
>  
>  	clk = clk_register(NULL, &mmc_clock->hw);
> -	if (IS_ERR(clk))
> +	if (IS_ERR(clk)) {
> +		ret = PTR_ERR(clk);
>  		goto err_register;
> +	}
>  
>  	mmc_clock->clk_rate_change_nb.notifier_call =
>  				&rockchip_mmc_clk_rate_notify;
> @@ -225,5 +239,5 @@ struct clk *rockchip_clk_register_mmc(const char *name,
>  	clk_unregister(clk);
>  err_register:
>  	kfree(mmc_clock);
> -	return clk;
> +	return ERR_PTR(ret);
>  }
> 

Re: [PATCH 3/3] clk: rockchip: Correct the behaviour of restoring cached phase

[Heiko Stuebner]

Am Mittwoch, 21. März 2018, 03:39:20 CET schrieb Shawn Lin:
> We can't restore every phase, for instance the invalid phase and
> the phase for coming rate which is out of the scope of boards'
> ability. And this patch also corrects the error path to return
> invalid pointer to clk if clk_notifier_register failed introduced
> by the same offending commit.
> 
> Fixes: 60cf09e45fbc ("clk: rockchip: Restore the clock phase after the rate was changed")
> Reported-by: wlq <wlq-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> Signed-off-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> Tested-by: wlq <wlq-TNX95d0MmH7DzftRWevZcw@public.gmane.org>

I've split off the part below [fix for error handling] into a separate
patch and applied both patches for 4.17


Thanks
Heiko

> @@ -211,8 +223,10 @@ struct clk *rockchip_clk_register_mmc(const char *name,
>  	mmc_clock->shift = shift;
>  
>  	clk = clk_register(NULL, &mmc_clock->hw);
> -	if (IS_ERR(clk))
> +	if (IS_ERR(clk)) {
> +		ret = PTR_ERR(clk);
>  		goto err_register;
> +	}
>  
>  	mmc_clock->clk_rate_change_nb.notifier_call =
>  				&rockchip_mmc_clk_rate_notify;
> @@ -225,5 +239,5 @@ struct clk *rockchip_clk_register_mmc(const char *name,
>  	clk_unregister(clk);
>  err_register:
>  	kfree(mmc_clock);
> -	return clk;
> +	return ERR_PTR(ret);
>  }
>