| Message ID | 20170121030604.7672-2-chris.brandt@renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
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>
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
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 --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,
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(+)