Re: [PATCH v2 3/5] pinctrl: exynos: Fix GPIO setup failure because domain clock being gated

[Krzysztof Kozlowski <k.kozlowski@xxxxxxxxxxx>]

On pon, 2014-12-01 at 23:34 +0900, Tomasz Figa wrote:
> 2014-12-01 17:37 GMT+09:00 Krzysztof Kozlowski <k.kozlowski@xxxxxxxxxxx>:
> >
> > On nie, 2014-11-30 at 21:19 +0900, Tomasz Figa wrote:
> >> Hi Krzysztof,
> >>
> >> 2014-11-28 23:08 GMT+09:00 Krzysztof Kozlowski <k.kozlowski@xxxxxxxxxxx>:
> >> > On pią, 2014-11-28 at 15:04 +0100, Linus Walleij wrote:
> >> >> On Wed, Nov 26, 2014 at 3:24 PM, Krzysztof Kozlowski
> >> >> <k.kozlowski@xxxxxxxxxxx> wrote:
> >> >>
> >> >> > The audio subsystem on Exynos 5420 has separate clocks and GPIO. To
> >> >> > operate properly on GPIOs the main block clock 'mau_epll' must be
> >> >> > enabled.
> >> >> >
> >> >> > This was observed on Peach Pi/Pit and Arndale Octa (after enabling i2s0)
> >> >> > after introducing runtime PM to pl330 DMA driver. After that commit the
> >> >> > 'mau_epll' was gated, because the "amba" clock was disabled and there
> >> >> > were no more users of mau_epll.
> >> >> >
> >> >> > The system hang just before probing i2s0 because
> >> >> > samsung_pinmux_setup() tried to access memory from audss block which was
> >> >> > gated.
> >> >> >
> >> >> > Add a clock property to the pinctrl driver and enable the clock during
> >> >> > GPIO setup. During normal GPIO operations (set, get, set_direction) the
> >> >> > clock is not enabled.
> >>
> >> Could you make sure that possibility of gating this clock is worth the
> >> effort of adding gating code to all affected drivers? If there is no
> >> significant change in power consumption maybe it could be simply keep
> >> running all the time?
> >
> > I had an impression that last time you disliked such idea:
> > http://www.spinics.net/lists/arm-kernel/msg338127.html
> > That's why I developed these patches. Because keeping a clock always on,
> > even when it is unused, is undesirable.
> 
> I was mostly concerned about the particular solution implemented by
> that patch that kept the clocks enabled on all platforms, not only the
> affected one. However...
> 
> >
> > Anyway, I did some simple measurements (after booting Arndale Octa
> > to /bin/sh, idle):
> >  - with mau_epll gated: ~523 mA
> >  - with mau_epll always on: ~531 mA
> >
> > Keeping it on increases energy usage by 1.5% in idle (with measurement
> > uncertainty ~0.4%).
> >
> 
> ...this definitely shows that the effort isn't worthless, which means
> that your patch goes in the right direction.

Great!

> 
> >
> >> Also isn't a similar problem happening due to power domains? I believe
> >> the whole maudio block is located in a separate power domain but
> >> somehow it doesn't get turned off?
> >
> > There is Maudio power domain... but I think it is not related here.
> 
> Could you somehow check what happens when the maudio power domain is
> turned off and maudio pin controller is accessed? Could you also check
> the difference in power consumption with this domain powered on and
> off?

I can try. I'll let you know the results.

> 
> > Pinctrl driver does not have runtime PM and is not attached to a domain.
> > I thought about other solution to this problem (with utilization of
> > power domains):
> >  - add runtime PM to pinctrl and audss clocks,
> >  - attach pinctrl and audss clocks to maudio power domain,
> >  - enable the clock when power domain is turned on.
> > However almost the same changes had to be added to pinctrl and audss
> > clocks drivers (replace clock_enable() with pm_runtime_get_sync()).
> 
> Well, if the dependency is there due to hardware design, I think it
> needs to be reflected in the drivers as well.
> 
> Few other issues that came to my mind:
> 
>  - Your previous patch added clock control only to pinctrl operations.
> Shouldn't GPIO operations be included as well?

Yeah... but does it make any sense? For example imagine GPIO is
configured as input. Enabling the clock won't change anything because
already that clock should be enabled by peripheral which writes to such
GPIO.

>  - If power domain dependency is there too, what happens with GPIO
> pins if the domain is powered off? If they lose their states, wouldn't
> it necessary to keep the domain powered on whenever there is some GPIO
> pin requested (which usually = in use)? (I'd assume that special
> functions shouldn't take a reference on runtime PM, because they are
> related to IP blocks in the same PM domain, which will implicitly keep
> the domain powered on.)

What you're saying makes sense but that look like job for sound soc
driver? Anyway the domain is not present in DTS so by default it is
turned on.

>  - Doesn't the clock controller also lose its state whenever the power
> domain is powered off? I guess that would be similar to ISP clock
> controller, issues of which are still not resolved in mainline.
> Sylwester could tell you more about this, I guess.

Once again - the domain.

Best regards,
Krzysztof


--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html