* RE: [PATCH] mmc: renesas-sdhi: Disable clock after tuning reset when possible
2023-11-05 22:42 ` [PATCH] mmc: renesas-sdhi: Disable clock after tuning reset when possible Marek Vasut
@ 2023-11-06 10:24 ` Jaehoon Chung
2023-11-11 16:14 ` Marek Vasut
2023-11-06 20:04 ` Paul Barker
2023-11-10 0:51 ` Yoshihiro Shimoda
2 siblings, 1 reply; 5+ messages in thread
From: Jaehoon Chung @ 2023-11-06 10:24 UTC (permalink / raw)
To: 'Marek Vasut', u-boot
Cc: 'Nobuhiro Iwamatsu', 'Paul Barker',
'Peng Fan'
> -----Original Message-----
> From: Marek Vasut <marek.vasut+renesas@mailbox.org>
> Sent: Monday, November 6, 2023 7:43 AM
> To: u-boot@lists.denx.de
> Cc: Marek Vasut <marek.vasut+renesas@mailbox.org>; Jaehoon Chung <jh80.chung@samsung.com>; Nobuhiro
> Iwamatsu <iwamatsu@nigauri.org>; Paul Barker <paul.barker.ct@bp.renesas.com>; Peng Fan
> <peng.fan@nxp.com>
> Subject: [PATCH] mmc: renesas-sdhi: Disable clock after tuning reset when possible
>
> Currently the renesas_sdhi_reset_tuning() unconditionally leaves SDHI
> clock enabled after the tuning reset. This is not always necessary.
>
> After the driver performed tuning reset at the end of probe function,
> or in the unlikely case that tuning failed during regular operation,
> the SDHI clock can be disabled after the tuning reset. The following
> set_ios call would reconfigure the clock as needed.
>
> In case of regular set_ios call which requires a tuning reset, keep
> the clock enabled or disabled according to the mmc->clk_disable state.
>
> With this in place, the controllers which have not been accessed via
> block subsystem after boot are left in quiescent state. However, if an
> MMC device is used e.g. for environment storage, that controller would
> be accessed during the environment load and left active, including its
> clock which would still be generated. This is due to the design of the
> MMC subsystem, which does not deinit a controller after it was started
> once, the controller is only deinited in case of mmc rescan, or before
> OS boot.
>
> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>
Best Regards,
Jaehoon Chung
> ---
> Note: To address the part where MMC device has been inited once and
> is never deinited until rescan or OS boot, it would likely be
> necessary to implement something like runtime PM, possibly
> based on the cyclic framework. Basically, keep track of when
> the MMC was accessed last, and if certain time elapsed, deinit
> the MMC. This could also be used to handle card detect polling
> at the same time.
> ---
> Cc: Jaehoon Chung <jh80.chung@samsung.com>
> Cc: Nobuhiro Iwamatsu <iwamatsu@nigauri.org>
> Cc: Paul Barker <paul.barker.ct@bp.renesas.com>
> Cc: Peng Fan <peng.fan@nxp.com>
> ---
> drivers/mmc/renesas-sdhi.c | 14 ++++++++++----
> 1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/mmc/renesas-sdhi.c b/drivers/mmc/renesas-sdhi.c
> index 8cd501c5f7c..97aaf1e4ec3 100644
> --- a/drivers/mmc/renesas-sdhi.c
> +++ b/drivers/mmc/renesas-sdhi.c
> @@ -318,7 +318,7 @@ static unsigned int renesas_sdhi_init_tuning(struct tmio_sd_priv *priv)
> RENESAS_SDHI_SCC_DTCNTL_TAPNUM_MASK;
> }
>
> -static void renesas_sdhi_reset_tuning(struct tmio_sd_priv *priv)
> +static void renesas_sdhi_reset_tuning(struct tmio_sd_priv *priv, bool clk_disable)
> {
> u32 reg;
>
> @@ -350,6 +350,12 @@ static void renesas_sdhi_reset_tuning(struct tmio_sd_priv *priv)
> reg = tmio_sd_readl(priv, RENESAS_SDHI_SCC_RVSCNTL);
> reg &= ~RENESAS_SDHI_SCC_RVSCNTL_RVSEN;
> tmio_sd_writel(priv, reg, RENESAS_SDHI_SCC_RVSCNTL);
> +
> + if (clk_disable) {
> + reg = tmio_sd_readl(priv, TMIO_SD_CLKCTL);
> + reg &= ~TMIO_SD_CLKCTL_SCLKEN;
> + tmio_sd_writel(priv, reg, TMIO_SD_CLKCTL);
> + }
> }
>
> static int renesas_sdhi_hs400(struct udevice *dev)
> @@ -629,7 +635,7 @@ int renesas_sdhi_execute_tuning(struct udevice *dev, uint opcode)
> out:
> if (ret < 0) {
> dev_warn(dev, "Tuning procedure failed\n");
> - renesas_sdhi_reset_tuning(priv);
> + renesas_sdhi_reset_tuning(priv, true);
> }
>
> return ret;
> @@ -668,7 +674,7 @@ static int renesas_sdhi_set_ios(struct udevice *dev)
> (mmc->selected_mode != UHS_SDR104) &&
> (mmc->selected_mode != MMC_HS_200) &&
> (mmc->selected_mode != MMC_HS_400)) {
> - renesas_sdhi_reset_tuning(priv);
> + renesas_sdhi_reset_tuning(priv, mmc->clk_disable);
> }
> #endif
>
> @@ -1095,7 +1101,7 @@ static int renesas_sdhi_probe(struct udevice *dev)
> CONFIG_IS_ENABLED(MMC_HS200_SUPPORT) || \
> CONFIG_IS_ENABLED(MMC_HS400_SUPPORT)
> if (priv->caps & TMIO_SD_CAP_RCAR_UHS)
> - renesas_sdhi_reset_tuning(priv);
> + renesas_sdhi_reset_tuning(priv, true);
> #endif
> return 0;
>
> --
> 2.42.0
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] mmc: renesas-sdhi: Disable clock after tuning reset when possible
2023-11-06 10:24 ` Jaehoon Chung
@ 2023-11-11 16:14 ` Marek Vasut
0 siblings, 0 replies; 5+ messages in thread
From: Marek Vasut @ 2023-11-11 16:14 UTC (permalink / raw)
To: Jaehoon Chung, 'Marek Vasut', u-boot
Cc: 'Nobuhiro Iwamatsu', 'Paul Barker',
'Peng Fan'
On 11/6/23 11:24, Jaehoon Chung wrote:
>
>
>> -----Original Message-----
>> From: Marek Vasut <marek.vasut+renesas@mailbox.org>
>> Sent: Monday, November 6, 2023 7:43 AM
>> To: u-boot@lists.denx.de
>> Cc: Marek Vasut <marek.vasut+renesas@mailbox.org>; Jaehoon Chung <jh80.chung@samsung.com>; Nobuhiro
>> Iwamatsu <iwamatsu@nigauri.org>; Paul Barker <paul.barker.ct@bp.renesas.com>; Peng Fan
>> <peng.fan@nxp.com>
>> Subject: [PATCH] mmc: renesas-sdhi: Disable clock after tuning reset when possible
>>
>> Currently the renesas_sdhi_reset_tuning() unconditionally leaves SDHI
>> clock enabled after the tuning reset. This is not always necessary.
>>
>> After the driver performed tuning reset at the end of probe function,
>> or in the unlikely case that tuning failed during regular operation,
>> the SDHI clock can be disabled after the tuning reset. The following
>> set_ios call would reconfigure the clock as needed.
>>
>> In case of regular set_ios call which requires a tuning reset, keep
>> the clock enabled or disabled according to the mmc->clk_disable state.
>>
>> With this in place, the controllers which have not been accessed via
>> block subsystem after boot are left in quiescent state. However, if an
>> MMC device is used e.g. for environment storage, that controller would
>> be accessed during the environment load and left active, including its
>> clock which would still be generated. This is due to the design of the
>> MMC subsystem, which does not deinit a controller after it was started
>> once, the controller is only deinited in case of mmc rescan, or before
>> OS boot.
>>
>> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
>
> Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>
Thanks. Do you want to take this via mmc tree or shall I take it via sh
tree ? I wouldn't mind the later, since it is isolated to renesas platforms.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] mmc: renesas-sdhi: Disable clock after tuning reset when possible
2023-11-05 22:42 ` [PATCH] mmc: renesas-sdhi: Disable clock after tuning reset when possible Marek Vasut
2023-11-06 10:24 ` Jaehoon Chung
@ 2023-11-06 20:04 ` Paul Barker
2023-11-10 0:51 ` Yoshihiro Shimoda
2 siblings, 0 replies; 5+ messages in thread
From: Paul Barker @ 2023-11-06 20:04 UTC (permalink / raw)
To: Marek Vasut; +Cc: u-boot, Jaehoon Chung, Nobuhiro Iwamatsu, Peng Fan
[-- Attachment #1: Type: text/plain, Size: 4093 bytes --]
On Sun, Nov 05, 2023 at 11:42:45PM +0100, Marek Vasut wrote:
> Currently the renesas_sdhi_reset_tuning() unconditionally leaves SDHI
> clock enabled after the tuning reset. This is not always necessary.
>
> After the driver performed tuning reset at the end of probe function,
> or in the unlikely case that tuning failed during regular operation,
> the SDHI clock can be disabled after the tuning reset. The following
> set_ios call would reconfigure the clock as needed.
>
> In case of regular set_ios call which requires a tuning reset, keep
> the clock enabled or disabled according to the mmc->clk_disable state.
>
> With this in place, the controllers which have not been accessed via
> block subsystem after boot are left in quiescent state. However, if an
> MMC device is used e.g. for environment storage, that controller would
> be accessed during the environment load and left active, including its
> clock which would still be generated. This is due to the design of the
> MMC subsystem, which does not deinit a controller after it was started
> once, the controller is only deinited in case of mmc rescan, or before
> OS boot.
>
> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
> ---
> Note: To address the part where MMC device has been inited once and
> is never deinited until rescan or OS boot, it would likely be
> necessary to implement something like runtime PM, possibly
> based on the cyclic framework. Basically, keep track of when
> the MMC was accessed last, and if certain time elapsed, deinit
> the MMC. This could also be used to handle card detect polling
> at the same time.
> ---
> Cc: Jaehoon Chung <jh80.chung@samsung.com>
> Cc: Nobuhiro Iwamatsu <iwamatsu@nigauri.org>
> Cc: Paul Barker <paul.barker.ct@bp.renesas.com>
> Cc: Peng Fan <peng.fan@nxp.com>
> ---
> drivers/mmc/renesas-sdhi.c | 14 ++++++++++----
> 1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/mmc/renesas-sdhi.c b/drivers/mmc/renesas-sdhi.c
> index 8cd501c5f7c..97aaf1e4ec3 100644
> --- a/drivers/mmc/renesas-sdhi.c
> +++ b/drivers/mmc/renesas-sdhi.c
> @@ -318,7 +318,7 @@ static unsigned int renesas_sdhi_init_tuning(struct tmio_sd_priv *priv)
> RENESAS_SDHI_SCC_DTCNTL_TAPNUM_MASK;
> }
>
> -static void renesas_sdhi_reset_tuning(struct tmio_sd_priv *priv)
> +static void renesas_sdhi_reset_tuning(struct tmio_sd_priv *priv, bool clk_disable)
> {
> u32 reg;
>
> @@ -350,6 +350,12 @@ static void renesas_sdhi_reset_tuning(struct tmio_sd_priv *priv)
> reg = tmio_sd_readl(priv, RENESAS_SDHI_SCC_RVSCNTL);
> reg &= ~RENESAS_SDHI_SCC_RVSCNTL_RVSEN;
> tmio_sd_writel(priv, reg, RENESAS_SDHI_SCC_RVSCNTL);
> +
> + if (clk_disable) {
> + reg = tmio_sd_readl(priv, TMIO_SD_CLKCTL);
> + reg &= ~TMIO_SD_CLKCTL_SCLKEN;
> + tmio_sd_writel(priv, reg, TMIO_SD_CLKCTL);
> + }
> }
>
> static int renesas_sdhi_hs400(struct udevice *dev)
> @@ -629,7 +635,7 @@ int renesas_sdhi_execute_tuning(struct udevice *dev, uint opcode)
> out:
> if (ret < 0) {
> dev_warn(dev, "Tuning procedure failed\n");
> - renesas_sdhi_reset_tuning(priv);
> + renesas_sdhi_reset_tuning(priv, true);
> }
>
> return ret;
> @@ -668,7 +674,7 @@ static int renesas_sdhi_set_ios(struct udevice *dev)
> (mmc->selected_mode != UHS_SDR104) &&
> (mmc->selected_mode != MMC_HS_200) &&
> (mmc->selected_mode != MMC_HS_400)) {
> - renesas_sdhi_reset_tuning(priv);
> + renesas_sdhi_reset_tuning(priv, mmc->clk_disable);
> }
> #endif
>
> @@ -1095,7 +1101,7 @@ static int renesas_sdhi_probe(struct udevice *dev)
> CONFIG_IS_ENABLED(MMC_HS200_SUPPORT) || \
> CONFIG_IS_ENABLED(MMC_HS400_SUPPORT)
> if (priv->caps & TMIO_SD_CAP_RCAR_UHS)
> - renesas_sdhi_reset_tuning(priv);
> + renesas_sdhi_reset_tuning(priv, true);
> #endif
> return 0;
Reviewed-by: Paul Barker <paul.barker.ct@bp.renesas.com>
Tested-by: Paul Barker <paul.barker.ct@bp.renesas.com>
(on RZ/G2L SMARC EVK)
Thanks,
Paul
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread* RE: [PATCH] mmc: renesas-sdhi: Disable clock after tuning reset when possible
2023-11-05 22:42 ` [PATCH] mmc: renesas-sdhi: Disable clock after tuning reset when possible Marek Vasut
2023-11-06 10:24 ` Jaehoon Chung
2023-11-06 20:04 ` Paul Barker
@ 2023-11-10 0:51 ` Yoshihiro Shimoda
2 siblings, 0 replies; 5+ messages in thread
From: Yoshihiro Shimoda @ 2023-11-10 0:51 UTC (permalink / raw)
To: Marek Vasut, u-boot@lists.denx.de
Cc: Jaehoon Chung, Nobuhiro Iwamatsu, Paul Barker, Peng Fan,
Nguyen Hong Thuan
Hello Marek-san,
> From: U-Boot On Behalf Of Marek Vasut, Sent: Monday, November 6, 2023 7:43 AM
>
> Currently the renesas_sdhi_reset_tuning() unconditionally leaves SDHI
> clock enabled after the tuning reset. This is not always necessary.
>
> After the driver performed tuning reset at the end of probe function,
> or in the unlikely case that tuning failed during regular operation,
> the SDHI clock can be disabled after the tuning reset. The following
> set_ios call would reconfigure the clock as needed.
>
> In case of regular set_ios call which requires a tuning reset, keep
> the clock enabled or disabled according to the mmc->clk_disable state.
>
> With this in place, the controllers which have not been accessed via
> block subsystem after boot are left in quiescent state. However, if an
> MMC device is used e.g. for environment storage, that controller would
> be accessed during the environment load and left active, including its
> clock which would still be generated. This is due to the design of the
> MMC subsystem, which does not deinit a controller after it was started
> once, the controller is only deinited in case of mmc rescan, or before
> OS boot.
>
> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
> ---
> Note: To address the part where MMC device has been inited once and
> is never deinited until rescan or OS boot, it would likely be
> necessary to implement something like runtime PM, possibly
> based on the cyclic framework. Basically, keep track of when
> the MMC was accessed last, and if certain time elapsed, deinit
> the MMC. This could also be used to handle card detect polling
> at the same time.
> ---
> Cc: Jaehoon Chung <jh80.chung@samsung.com>
> Cc: Nobuhiro Iwamatsu <iwamatsu@nigauri.org>
> Cc: Paul Barker <paul.barker.ct@bp.renesas.com>
> Cc: Peng Fan <peng.fan@nxp.com>
Thank you for the patch! This patch looks good to me. So,
Reviewed-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Also, our outsourcing member tested this patch on R-Car H3 Salvator-XS
And it works without any regression. So,
Tested-by: Thuan Nguyen Hong <thuan.nguyen-hong@banvien.com.vn>
Best regards,
Yoshihiro Shimoda
> ---
> drivers/mmc/renesas-sdhi.c | 14 ++++++++++----
> 1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/mmc/renesas-sdhi.c b/drivers/mmc/renesas-sdhi.c
> index 8cd501c5f7c..97aaf1e4ec3 100644
> --- a/drivers/mmc/renesas-sdhi.c
> +++ b/drivers/mmc/renesas-sdhi.c
> @@ -318,7 +318,7 @@ static unsigned int renesas_sdhi_init_tuning(struct tmio_sd_priv *priv)
> RENESAS_SDHI_SCC_DTCNTL_TAPNUM_MASK;
> }
>
> -static void renesas_sdhi_reset_tuning(struct tmio_sd_priv *priv)
> +static void renesas_sdhi_reset_tuning(struct tmio_sd_priv *priv, bool clk_disable)
> {
> u32 reg;
>
> @@ -350,6 +350,12 @@ static void renesas_sdhi_reset_tuning(struct tmio_sd_priv *priv)
> reg = tmio_sd_readl(priv, RENESAS_SDHI_SCC_RVSCNTL);
> reg &= ~RENESAS_SDHI_SCC_RVSCNTL_RVSEN;
> tmio_sd_writel(priv, reg, RENESAS_SDHI_SCC_RVSCNTL);
> +
> + if (clk_disable) {
> + reg = tmio_sd_readl(priv, TMIO_SD_CLKCTL);
> + reg &= ~TMIO_SD_CLKCTL_SCLKEN;
> + tmio_sd_writel(priv, reg, TMIO_SD_CLKCTL);
> + }
> }
>
> static int renesas_sdhi_hs400(struct udevice *dev)
> @@ -629,7 +635,7 @@ int renesas_sdhi_execute_tuning(struct udevice *dev, uint opcode)
> out:
> if (ret < 0) {
> dev_warn(dev, "Tuning procedure failed\n");
> - renesas_sdhi_reset_tuning(priv);
> + renesas_sdhi_reset_tuning(priv, true);
> }
>
> return ret;
> @@ -668,7 +674,7 @@ static int renesas_sdhi_set_ios(struct udevice *dev)
> (mmc->selected_mode != UHS_SDR104) &&
> (mmc->selected_mode != MMC_HS_200) &&
> (mmc->selected_mode != MMC_HS_400)) {
> - renesas_sdhi_reset_tuning(priv);
> + renesas_sdhi_reset_tuning(priv, mmc->clk_disable);
> }
> #endif
>
> @@ -1095,7 +1101,7 @@ static int renesas_sdhi_probe(struct udevice *dev)
> CONFIG_IS_ENABLED(MMC_HS200_SUPPORT) || \
> CONFIG_IS_ENABLED(MMC_HS400_SUPPORT)
> if (priv->caps & TMIO_SD_CAP_RCAR_UHS)
> - renesas_sdhi_reset_tuning(priv);
> + renesas_sdhi_reset_tuning(priv, true);
> #endif
> return 0;
>
> --
> 2.42.0
^ permalink raw reply [flat|nested] 5+ messages in thread