diff for duplicates of <20170821215031.17d54ab5@bbrezillon> diff --git a/a/1.txt b/N1/1.txt index cdfbbaf..f32a301 100644 --- a/a/1.txt +++ b/N1/1.txt @@ -1,8 +1,8 @@ Le Mon, 21 Aug 2017 09:49:52 -0700, -Doug Anderson <dianders@chromium.org> a =C3=A9crit : +Doug Anderson <dianders@chromium.org> a écrit : > Hi, ->=20 +> > On Mon, Aug 21, 2017 at 8:39 AM, Boris Brezillon > <boris.brezillon@free-electrons.com> wrote: > > Hi Doug, @@ -10,74 +10,51 @@ Doug Anderson <dianders@chromium.org> a =C3=A9crit : > > Sorry for the late reply. > > > > Le Fri, 4 Aug 2017 09:22:56 -0700, -> > Doug Anderson <dianders@chromium.org> a =C3=A9crit : -> > =20 +> > Doug Anderson <dianders@chromium.org> a écrit : +> > > >> Hi, > >> -> >> On Fri, Aug 4, 2017 at 7:48 AM, Heiko Stuebner <heiko@sntech.de> wrote= -: =20 -> >> >> > We found a issue recently, if the pwm0 is not enabled at uboot an= -d pwm2 -> >> >> > is enabled at uboot, the PWM clock will be disabled at pwm0's pro= -be. It -> >> >> > is true to close the pwm clock, and the pwm2 can't work during a = -while, -> >> >> > until the pwm2 probe, because the pwm0 and pwm2 are the same cloc= -k for +> >> On Fri, Aug 4, 2017 at 7:48 AM, Heiko Stuebner <heiko@sntech.de> wrote: +> >> >> > We found a issue recently, if the pwm0 is not enabled at uboot and pwm2 +> >> >> > is enabled at uboot, the PWM clock will be disabled at pwm0's probe. It +> >> >> > is true to close the pwm clock, and the pwm2 can't work during a while, +> >> >> > until the pwm2 probe, because the pwm0 and pwm2 are the same clock for > >> >> > their work. In fact, the pwm0 can not know the pwm2's status. > >> >> > -> >> >> > So we need to get all the PWMs state in a public place where it's= - early +> >> >> > So we need to get all the PWMs state in a public place where it's early > >> >> > than the PWM probe, if that's the way it is. Then keep the PWM clk -> >> >> > enabled if theis is one PWM appears to be up and running. The pla= -ce -> >> >> > maybe in the clock core init, like adding pwm clock as critial on= -e. +> >> >> > enabled if theis is one PWM appears to be up and running. The place +> >> >> > maybe in the clock core init, like adding pwm clock as critial one. > >> >> > -> >> >> > Another way is that we don't enable pwm clock firstly at PWM prob= -e, -> >> >> > because whether or not the PWM state has been enabled in the Uboo= -t, like -> >> >> > other modules, our chip default PWM clock registers are enabled a= -t the -> >> >> > beginning, read the PWM registers directly to know the PWM state.= - Then -> >> >> > if the PWM state is enabled, call the enable_clk(pc->clk) to add = -the -> >> >> > clock count=3D1. If the PWM state is disabled, we do nothing. Aft= -er all -> >> >> > the PWMs are probed and all modules are probed, the clock core wi= -ll gate -> >> >> > the PWM clock if the clock count is 0, and keep clk enabled if th= -e clock +> >> >> > Another way is that we don't enable pwm clock firstly at PWM probe, +> >> >> > because whether or not the PWM state has been enabled in the Uboot, like +> >> >> > other modules, our chip default PWM clock registers are enabled at the +> >> >> > beginning, read the PWM registers directly to know the PWM state. Then +> >> >> > if the PWM state is enabled, call the enable_clk(pc->clk) to add the +> >> >> > clock count=1. If the PWM state is disabled, we do nothing. After all +> >> >> > the PWMs are probed and all modules are probed, the clock core will gate +> >> >> > the PWM clock if the clock count is 0, and keep clk enabled if the clock > >> >> > count is not 0. > >> >> > -> >> >> > How do you feel about it? =20 +> >> >> > How do you feel about it? > >> >> > >> >> Ouch. That's indeed hard to solve in a clean way. I may have -> >> >> something to suggest but I'm not sure clk maintainers will like it:= - what -> >> >> if we make clk_disable() and clk_unprepare() just decrement the ref= -count +> >> >> something to suggest but I'm not sure clk maintainers will like it: what +> >> >> if we make clk_disable() and clk_unprepare() just decrement the refcount > >> >> before the disable-unused-clks procedure has been executed (see -> >> >> proposed patch below)? This way all clks that have been enabled by = -the -> >> >> bootloader will stay in such state until all drivers have had a cha= -nce +> >> >> proposed patch below)? This way all clks that have been enabled by the +> >> >> bootloader will stay in such state until all drivers have had a chance > >> >> to retain them (IOW, call clk_prepare()+clk_enable()). > >> >> > >> >> BTW, I think the problem you're describing here is not unique to PWM -> >> >> devices, it's just that now, some PWM drivers are smart and try to = -keep -> >> >> clks enabled to prevent glitches. =20 +> >> >> devices, it's just that now, some PWM drivers are smart and try to keep +> >> >> clks enabled to prevent glitches. > >> > -> >> > Actually, Mike had patches that introduced so called "handoff" clock= -s [0]. -> >> > Clocks that were handled as critical until some driver picked them u= -p. +> >> > Actually, Mike had patches that introduced so called "handoff" clocks [0]. +> >> > Clocks that were handled as critical until some driver picked them up. > >> > > >> > It's not exactly the same as your change and still would require -> >> > intervention from clock-drivers to mark clocks in such a way. =20 +> >> > intervention from clock-drivers to mark clocks in such a way. > >> > >> Right. As you're saying handoff isn't enough because in this case a > >> driver _has_ picked up the clock. The whole issue is that it's a @@ -86,11 +63,11 @@ p. > >> know that another driver in the future might want to also pick up the > >> clock. > >> -> >> =20 +> >> > >> > So I really also like your approach, as it would make clock wiggling > >> > during early boot safe for everyone involved :-) . > >> > -> >> > And both seem to cater to slightly different use-cases as well. =20 +> >> > And both seem to cater to slightly different use-cases as well. > >> > >> One worry I have about not truly disabling any clocks at boot time is > >> that it could break someone who relies on a clock being disabled. I'm @@ -106,26 +83,26 @@ p. > >> stops the card clock directly through the dw_mmc IP block and it's > >> invisible to the common clock framework), but it illustrates the point > >> that there could plausibly be cases where deferring a clk_disable() -> >> might be unwise. =20 +> >> might be unwise. > > > > Right, I didn't consider this use case, but some drivers might rely on > > the fact that clk_disable() disables the clk right away instead of > > deferring it. -> > =20 +> > > >> > >> I suppose, though, that it would be possible to distinguish those two > >> cases via a 2nd API call. AKA: > >> > >> clk_disable() -- disable the clock eventually > >> clk_disable_sync() -- disable the clock but don't defer. Still won't -> >> actually disable if someone else explicitly holds a reference. =20 +> >> actually disable if someone else explicitly holds a reference. > > > > Actually, I'd recommend keeping the existing behavior for clk_disable() > > and adding a new function called clk_disable_async() (or > > clk_disable_deferrable()). This way we do not break existing users that > > rely on clk_disable() synchronicity and force users that actually allow -> > deferring clk gating to explicitly state it. =20 ->=20 +> > deferring clk gating to explicitly state it. +> > Yeah, there's a tradeoff. Your solution is definitely safer and > causes less short term churn. ...but it's at the expense of some > inconsistency between various similar APIs (some APIs the "default" is @@ -135,7 +112,7 @@ p. Yep. ->=20 +> > Is anyone volunteering to do this? Looks like you found a volunteer: Elaine already posted my patch to the diff --git a/a/content_digest b/N1/content_digest index c513c68..26f8154 100644 --- a/a/content_digest +++ b/N1/content_digest @@ -24,10 +24,10 @@ "\00:1\0" "b\0" "Le Mon, 21 Aug 2017 09:49:52 -0700,\n" - "Doug Anderson <dianders@chromium.org> a =C3=A9crit :\n" + "Doug Anderson <dianders@chromium.org> a \303\251crit :\n" "\n" "> Hi,\n" - ">=20\n" + "> \n" "> On Mon, Aug 21, 2017 at 8:39 AM, Boris Brezillon\n" "> <boris.brezillon@free-electrons.com> wrote:\n" "> > Hi Doug,\n" @@ -35,74 +35,51 @@ "> > Sorry for the late reply.\n" "> >\n" "> > Le Fri, 4 Aug 2017 09:22:56 -0700,\n" - "> > Doug Anderson <dianders@chromium.org> a =C3=A9crit :\n" - "> > =20\n" + "> > Doug Anderson <dianders@chromium.org> a \303\251crit :\n" + "> > \n" "> >> Hi,\n" "> >>\n" - "> >> On Fri, Aug 4, 2017 at 7:48 AM, Heiko Stuebner <heiko@sntech.de> wrote=\n" - ": =20\n" - "> >> >> > We found a issue recently, if the pwm0 is not enabled at uboot an=\n" - "d pwm2\n" - "> >> >> > is enabled at uboot, the PWM clock will be disabled at pwm0's pro=\n" - "be. It\n" - "> >> >> > is true to close the pwm clock, and the pwm2 can't work during a =\n" - "while,\n" - "> >> >> > until the pwm2 probe, because the pwm0 and pwm2 are the same cloc=\n" - "k for\n" + "> >> On Fri, Aug 4, 2017 at 7:48 AM, Heiko Stuebner <heiko@sntech.de> wrote: \n" + "> >> >> > We found a issue recently, if the pwm0 is not enabled at uboot and pwm2\n" + "> >> >> > is enabled at uboot, the PWM clock will be disabled at pwm0's probe. It\n" + "> >> >> > is true to close the pwm clock, and the pwm2 can't work during a while,\n" + "> >> >> > until the pwm2 probe, because the pwm0 and pwm2 are the same clock for\n" "> >> >> > their work. In fact, the pwm0 can not know the pwm2's status.\n" "> >> >> >\n" - "> >> >> > So we need to get all the PWMs state in a public place where it's=\n" - " early\n" + "> >> >> > So we need to get all the PWMs state in a public place where it's early\n" "> >> >> > than the PWM probe, if that's the way it is. Then keep the PWM clk\n" - "> >> >> > enabled if theis is one PWM appears to be up and running. The pla=\n" - "ce\n" - "> >> >> > maybe in the clock core init, like adding pwm clock as critial on=\n" - "e.\n" + "> >> >> > enabled if theis is one PWM appears to be up and running. The place\n" + "> >> >> > maybe in the clock core init, like adding pwm clock as critial one.\n" "> >> >> >\n" - "> >> >> > Another way is that we don't enable pwm clock firstly at PWM prob=\n" - "e,\n" - "> >> >> > because whether or not the PWM state has been enabled in the Uboo=\n" - "t, like\n" - "> >> >> > other modules, our chip default PWM clock registers are enabled a=\n" - "t the\n" - "> >> >> > beginning, read the PWM registers directly to know the PWM state.=\n" - " Then\n" - "> >> >> > if the PWM state is enabled, call the enable_clk(pc->clk) to add =\n" - "the\n" - "> >> >> > clock count=3D1. If the PWM state is disabled, we do nothing. Aft=\n" - "er all\n" - "> >> >> > the PWMs are probed and all modules are probed, the clock core wi=\n" - "ll gate\n" - "> >> >> > the PWM clock if the clock count is 0, and keep clk enabled if th=\n" - "e clock\n" + "> >> >> > Another way is that we don't enable pwm clock firstly at PWM probe,\n" + "> >> >> > because whether or not the PWM state has been enabled in the Uboot, like\n" + "> >> >> > other modules, our chip default PWM clock registers are enabled at the\n" + "> >> >> > beginning, read the PWM registers directly to know the PWM state. Then\n" + "> >> >> > if the PWM state is enabled, call the enable_clk(pc->clk) to add the\n" + "> >> >> > clock count=1. If the PWM state is disabled, we do nothing. After all\n" + "> >> >> > the PWMs are probed and all modules are probed, the clock core will gate\n" + "> >> >> > the PWM clock if the clock count is 0, and keep clk enabled if the clock\n" "> >> >> > count is not 0.\n" "> >> >> >\n" - "> >> >> > How do you feel about it? =20\n" + "> >> >> > How do you feel about it? \n" "> >> >>\n" "> >> >> Ouch. That's indeed hard to solve in a clean way. I may have\n" - "> >> >> something to suggest but I'm not sure clk maintainers will like it:=\n" - " what\n" - "> >> >> if we make clk_disable() and clk_unprepare() just decrement the ref=\n" - "count\n" + "> >> >> something to suggest but I'm not sure clk maintainers will like it: what\n" + "> >> >> if we make clk_disable() and clk_unprepare() just decrement the refcount\n" "> >> >> before the disable-unused-clks procedure has been executed (see\n" - "> >> >> proposed patch below)? This way all clks that have been enabled by =\n" - "the\n" - "> >> >> bootloader will stay in such state until all drivers have had a cha=\n" - "nce\n" + "> >> >> proposed patch below)? This way all clks that have been enabled by the\n" + "> >> >> bootloader will stay in such state until all drivers have had a chance\n" "> >> >> to retain them (IOW, call clk_prepare()+clk_enable()).\n" "> >> >>\n" "> >> >> BTW, I think the problem you're describing here is not unique to PWM\n" - "> >> >> devices, it's just that now, some PWM drivers are smart and try to =\n" - "keep\n" - "> >> >> clks enabled to prevent glitches. =20\n" + "> >> >> devices, it's just that now, some PWM drivers are smart and try to keep\n" + "> >> >> clks enabled to prevent glitches. \n" "> >> >\n" - "> >> > Actually, Mike had patches that introduced so called \"handoff\" clock=\n" - "s [0].\n" - "> >> > Clocks that were handled as critical until some driver picked them u=\n" - "p.\n" + "> >> > Actually, Mike had patches that introduced so called \"handoff\" clocks [0].\n" + "> >> > Clocks that were handled as critical until some driver picked them up.\n" "> >> >\n" "> >> > It's not exactly the same as your change and still would require\n" - "> >> > intervention from clock-drivers to mark clocks in such a way. =20\n" + "> >> > intervention from clock-drivers to mark clocks in such a way. \n" "> >>\n" "> >> Right. As you're saying handoff isn't enough because in this case a\n" "> >> driver _has_ picked up the clock. The whole issue is that it's a\n" @@ -111,11 +88,11 @@ "> >> know that another driver in the future might want to also pick up the\n" "> >> clock.\n" "> >>\n" - "> >> =20\n" + "> >> \n" "> >> > So I really also like your approach, as it would make clock wiggling\n" "> >> > during early boot safe for everyone involved :-) .\n" "> >> >\n" - "> >> > And both seem to cater to slightly different use-cases as well. =20\n" + "> >> > And both seem to cater to slightly different use-cases as well. \n" "> >>\n" "> >> One worry I have about not truly disabling any clocks at boot time is\n" "> >> that it could break someone who relies on a clock being disabled. I'm\n" @@ -131,26 +108,26 @@ "> >> stops the card clock directly through the dw_mmc IP block and it's\n" "> >> invisible to the common clock framework), but it illustrates the point\n" "> >> that there could plausibly be cases where deferring a clk_disable()\n" - "> >> might be unwise. =20\n" + "> >> might be unwise. \n" "> >\n" "> > Right, I didn't consider this use case, but some drivers might rely on\n" "> > the fact that clk_disable() disables the clk right away instead of\n" "> > deferring it.\n" - "> > =20\n" + "> > \n" "> >>\n" "> >> I suppose, though, that it would be possible to distinguish those two\n" "> >> cases via a 2nd API call. AKA:\n" "> >>\n" "> >> clk_disable() -- disable the clock eventually\n" "> >> clk_disable_sync() -- disable the clock but don't defer. Still won't\n" - "> >> actually disable if someone else explicitly holds a reference. =20\n" + "> >> actually disable if someone else explicitly holds a reference. \n" "> >\n" "> > Actually, I'd recommend keeping the existing behavior for clk_disable()\n" "> > and adding a new function called clk_disable_async() (or\n" "> > clk_disable_deferrable()). This way we do not break existing users that\n" "> > rely on clk_disable() synchronicity and force users that actually allow\n" - "> > deferring clk gating to explicitly state it. =20\n" - ">=20\n" + "> > deferring clk gating to explicitly state it. \n" + "> \n" "> Yeah, there's a tradeoff. Your solution is definitely safer and\n" "> causes less short term churn. ...but it's at the expense of some\n" "> inconsistency between various similar APIs (some APIs the \"default\" is\n" @@ -160,7 +137,7 @@ "\n" "Yep.\n" "\n" - ">=20\n" + "> \n" "> Is anyone volunteering to do this?\n" "\n" "Looks like you found a volunteer: Elaine already posted my patch to the\n" @@ -172,4 +149,4 @@ "Don't if that's what you meant by \"approval\" authority, but you'll\n" need Mike or Stephen (or both) to validate this approach. -92696b62d22674972d2ef63a6290efdf42ff7b23a862759eba08c6cbe34ddd85 +f11b4aaa7f50aa182f2d86d66e49010f46bd00f527ea0aea00a3800703dc4b28
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.