| Message ID | 20170118172502.13876-2-chris.brandt@renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
On Wed, Jan 18, 2017 at 12:25:00PM -0500, Chris Brandt wrote: > Some controllers have 2 clock sources instead of 1, so they both need > to be turned on/off. > > Signed-off-by: Chris Brandt <chris.brandt@renesas.com> Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
On 18 January 2017 at 18:25, Chris Brandt <chris.brandt@renesas.com> wrote: > Some controllers have 2 clock sources instead of 1, so they both need > to be turned on/off. This doesn't tell me enough. Please elaborate. For example, tell how you treat the clocks, which of them that is optional and why. > > Signed-off-by: Chris Brandt <chris.brandt@renesas.com> > --- > 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 | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c > index 59db14b..a3f995e 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,10 @@ static int sh_mobile_sdhi_probe(struct platform_device *pdev) > goto eprobe; > } > > + priv->clk_cd = devm_clk_get(&pdev->dev, "cd"); > + if (IS_ERR(priv->clk_cd)) > + priv->clk_cd = NULL; Is this clock solely about card detection? So in cases when you have a GPIO card detect, the clock isn't needed? Just trying to understand things a bit better... > + > priv->pinctrl = devm_pinctrl_get(&pdev->dev); > if (!IS_ERR(priv->pinctrl)) { > priv->pins_default = pinctrl_lookup_state(priv->pinctrl, > -- > 2.10.1 > > Kind regards Uffe
Hello Ulf, On Friday, January 20, 2017, Ulf Hansson wrote: > On 18 January 2017 at 18:25, Chris Brandt <chris.brandt@renesas.com> > wrote: > > Some controllers have 2 clock sources instead of 1, so they both need > > to be turned on/off. > > This doesn't tell me enough. Please elaborate. > > For example, tell how you treat the clocks, which of them that is optional > and why. Basically, the chip designers went in and broke out the logic that just does the card detect (more or less it's probably just some simple IRQ logic) so that you could shut off the IP block, but if you put in a card, the internal interrupt signal still went to the interrupt controller and registered the interrupt even though the rest of the IP block (including the register space) was dead. The idea seems to be that you could put the entire chip into low power mode and wake it up if you stuck a card in. My guess is that this was for some customer request or something. Personally...you could have done the same thing if you laid out a board and tied one of the extra IRQ lines to the cd signal...but I'm not sure if anyone thought about that at the time. So to your request, I could put all this ugly info into the documentation, but basically all I'm trying to do is join the 2 clocks back together to make it work like all the other SoCs since this new 'feature' might not really be practical to every use in this environment. > > static int sh_mobile_sdhi_card_busy(struct mmc_host *mmc) @@ -572,6 > > +581,10 @@ static int sh_mobile_sdhi_probe(struct platform_device *pdev) > > goto eprobe; > > } > > > > + priv->clk_cd = devm_clk_get(&pdev->dev, "cd"); > > + if (IS_ERR(priv->clk_cd)) > > + priv->clk_cd = NULL; > > Is this clock solely about card detection? So in cases when you have a > GPIO card detect, the clock isn't needed? > > Just trying to understand things a bit better... According to the hardware manual, enabling the "core" clock but not the "cd" clock is not a valid setting. So in our case, it's always all or nothing. Chris
> > Is this clock solely about card detection? So in cases when you have a > > GPIO card detect, the clock isn't needed? > > > > Just trying to understand things a bit better... > > According to the hardware manual, enabling the "core" clock but not the > "cd" clock is not a valid setting. So in our case, it's always all or > nothing. It was my suggestion to either handle both clocks as "virtually" one clock so it simulates how other SDHI instances behave, or to implement proper and intended handling of the cd clock to save some power. Chris chose the first option and I have full understanding for that decision.
diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c index 59db14b..a3f995e 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,10 @@ static int sh_mobile_sdhi_probe(struct platform_device *pdev) goto eprobe; } + 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, so they both need to be turned on/off. Signed-off-by: Chris Brandt <chris.brandt@renesas.com> --- 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 | 13 +++++++++++++ 1 file changed, 13 insertions(+)