diff mbox

[v5,1/3] mmc: sh_mobile_sdhi: add support for 2 clocks

Message ID 20170121030604.7672-2-chris.brandt@renesas.com (mailing list archive)
State New, archived
Headers show

Commit Message

Chris Brandt Jan. 21, 2017, 3:06 a.m. UTC
Some controllers have 2 clock sources instead of 1. The 2nd clock
is for the internal card detect logic and must be enabled/disabled
along with the main core clock for proper operation.

Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
---
v4:
* add technical explanation within probe routine
v3:
* add more clarification to the commit log
v2:
* changed clk2 to clk_cd
* disable clk if clk_cd enable fails
* changed clock name from "carddetect" to "cd"
---
 drivers/mmc/host/sh_mobile_sdhi.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

Comments

Wolfram Sang Jan. 21, 2017, 9:37 a.m. UTC | #1
On Fri, Jan 20, 2017 at 10:06:02PM -0500, Chris Brandt wrote:
> Some controllers have 2 clock sources instead of 1. The 2nd clock
> is for the internal card detect logic and must be enabled/disabled
> along with the main core clock for proper operation.
> 
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>

Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Geert Uytterhoeven Jan. 23, 2017, 9:22 a.m. UTC | #2
Hi Chris,

On Sat, Jan 21, 2017 at 4:06 AM, Chris Brandt <chris.brandt@renesas.com> wrote:
> Some controllers have 2 clock sources instead of 1. The 2nd clock
> is for the internal card detect logic and must be enabled/disabled
> along with the main core clock for proper operation.
>
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
> ---
> v4:
> * add technical explanation within probe routine
> v3:
> * add more clarification to the commit log
> v2:
> * changed clk2 to clk_cd
> * disable clk if clk_cd enable fails
> * changed clock name from "carddetect" to "cd"

Thanks for the updates!

> ---
>  drivers/mmc/host/sh_mobile_sdhi.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
>
> diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c
> index 59db14b..360d922 100644
> --- a/drivers/mmc/host/sh_mobile_sdhi.c
> +++ b/drivers/mmc/host/sh_mobile_sdhi.c
> @@ -143,6 +143,7 @@ MODULE_DEVICE_TABLE(of, sh_mobile_sdhi_of_match);
>
>  struct sh_mobile_sdhi {
>         struct clk *clk;
> +       struct clk *clk_cd;
>         struct tmio_mmc_data mmc_data;
>         struct tmio_mmc_dma dma_priv;
>         struct pinctrl *pinctrl;
> @@ -190,6 +191,12 @@ static int sh_mobile_sdhi_clk_enable(struct tmio_mmc_host *host)
>         if (ret < 0)
>                 return ret;
>
> +       ret = clk_prepare_enable(priv->clk_cd);
> +       if (ret < 0) {
> +               clk_disable_unprepare(priv->clk);
> +               return ret;
> +       }
> +

As enabling the "core" clock but not the "cd" clock is not a valid setting,
shouldn't the cd clock be enabled first?

>         /*
>          * The clock driver may not know what maximum frequency
>          * actually works, so it should be set with the max-frequency
> @@ -255,6 +262,8 @@ static void sh_mobile_sdhi_clk_disable(struct tmio_mmc_host *host)
>         struct sh_mobile_sdhi *priv = host_to_priv(host);
>
>         clk_disable_unprepare(priv->clk);
> +       if (priv->clk_cd)

No need to check for a NULL pointer first.

> +               clk_disable_unprepare(priv->clk_cd);

Disabling is already done in the correct order ;-)

>  }

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Chris Brandt Jan. 23, 2017, 2:48 p.m. UTC | #3
Hi Geert,

On Monday, January 23, 2017, Geert Uytterhoeven:
> > @@ -190,6 +191,12 @@ static int sh_mobile_sdhi_clk_enable(struct

> tmio_mmc_host *host)

> >         if (ret < 0)

> >                 return ret;

> >

> > +       ret = clk_prepare_enable(priv->clk_cd);

> > +       if (ret < 0) {

> > +               clk_disable_unprepare(priv->clk);

> > +               return ret;

> > +       }

> > +

> 

> As enabling the "core" clock but not the "cd" clock is not a valid setting,

> shouldn't the cd clock be enabled first?


I'm pretty sure the chip designers just didn't want you to 'use' the
peripheral with that clock configuration, and we are not using it in
between those 2 lines so I personally think it's fine.

If I need to change it to get it approved, let me know and I'll change
the code around.

(In reality, I checked and the SDHI does work with the cd clock off,
but I don't know if the restriction in the spec was because they never
fully tested the use of it that way because it wasn't a design goal for
them.)



> >         /*

> >          * The clock driver may not know what maximum frequency

> >          * actually works, so it should be set with the max-frequency

> > @@ -255,6 +262,8 @@ static void sh_mobile_sdhi_clk_disable(struct

> tmio_mmc_host *host)

> >         struct sh_mobile_sdhi *priv = host_to_priv(host);

> >

> >         clk_disable_unprepare(priv->clk);

> > +       if (priv->clk_cd)

> 

> No need to check for a NULL pointer first.


OK, I see that IS_ERR_OR_NULL(clk) is done for clk_disable_unprepare so
I'll take the check out.
Thank you.


> > +               clk_disable_unprepare(priv->clk_cd);

> 

> Disabling is already done in the correct order ;-)


I guess you could get into some theoretical technical discussion around
if it is OK to enable/disable the clocks in any order (as long as you
don't use the SDHI in between), then you would want to enable the cd
circuit last, and disable it first so you would ensure that no cd
interrupts are being registered when the core clock is off?
In that case, the current enable order should remain but the disable
order should change.

Opinions???


Thank you,
Chris
diff mbox

Patch

diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c
index 59db14b..360d922 100644
--- a/drivers/mmc/host/sh_mobile_sdhi.c
+++ b/drivers/mmc/host/sh_mobile_sdhi.c
@@ -143,6 +143,7 @@  MODULE_DEVICE_TABLE(of, sh_mobile_sdhi_of_match);
 
 struct sh_mobile_sdhi {
 	struct clk *clk;
+	struct clk *clk_cd;
 	struct tmio_mmc_data mmc_data;
 	struct tmio_mmc_dma dma_priv;
 	struct pinctrl *pinctrl;
@@ -190,6 +191,12 @@  static int sh_mobile_sdhi_clk_enable(struct tmio_mmc_host *host)
 	if (ret < 0)
 		return ret;
 
+	ret = clk_prepare_enable(priv->clk_cd);
+	if (ret < 0) {
+		clk_disable_unprepare(priv->clk);
+		return ret;
+	}
+
 	/*
 	 * The clock driver may not know what maximum frequency
 	 * actually works, so it should be set with the max-frequency
@@ -255,6 +262,8 @@  static void sh_mobile_sdhi_clk_disable(struct tmio_mmc_host *host)
 	struct sh_mobile_sdhi *priv = host_to_priv(host);
 
 	clk_disable_unprepare(priv->clk);
+	if (priv->clk_cd)
+		clk_disable_unprepare(priv->clk_cd);
 }
 
 static int sh_mobile_sdhi_card_busy(struct mmc_host *mmc)
@@ -572,6 +581,21 @@  static int sh_mobile_sdhi_probe(struct platform_device *pdev)
 		goto eprobe;
 	}
 
+	/*
+	 * Some controllers provide a 2nd clock just to run the internal card
+	 * detection logic. Unfortunately, the existing driver architecture does
+	 * not support a separation of clocks for runtime PM usage. When
+	 * native hotplug is used, the tmio driver assumes that the core
+	 * must continue to run for card detect to stay active, so we cannot
+	 * disable it.
+	 * Additionally, it is prohibited to supply a clock to the core but not
+	 * to the card detect circuit. That leaves us with if separate clocks
+	 * are presented, we must treat them both as virtually 1 clock.
+	 */
+	priv->clk_cd = devm_clk_get(&pdev->dev, "cd");
+	if (IS_ERR(priv->clk_cd))
+		priv->clk_cd = NULL;
+
 	priv->pinctrl = devm_pinctrl_get(&pdev->dev);
 	if (!IS_ERR(priv->pinctrl)) {
 		priv->pins_default = pinctrl_lookup_state(priv->pinctrl,