diff for duplicates of <20170821173955.30b0f5a2@bbrezillon> diff --git a/a/1.txt b/N1/1.txt index fbf42f8..8b7513f 100644 --- a/a/1.txt +++ b/N1/1.txt @@ -3,50 +3,37 @@ Hi Doug, Sorry for the late reply. Le Fri, 4 Aug 2017 09:22:56 -0700, -Doug Anderson <dianders@chromium.org> a =C3=A9crit : +Doug Anderson <dianders@chromium.org> a écrit : > Hi, ->=20 +> > 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 p= -wm2 -> >> > 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 whi= -le, -> >> > until the pwm2 probe, because the pwm0 and pwm2 are the same clock f= -or +> >> > 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 ea= -rly +> >> > 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 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 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 t= -he -> >> > beginning, read the PWM registers directly to know the PWM state. Th= -en +> >> > 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=3D1. 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 c= -lock +> >> > 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: wh= -at -> >> if we make clk_disable() and clk_unprepare() just decrement the refcou= -nt +> >> 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 chance @@ -54,32 +41,31 @@ nt > >> > >> 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 +> >> clks enabled to prevent glitches. > > -> > Actually, Mike had patches that introduced so called "handoff" clocks [= -0]. +> > 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 ->=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 > shared clock between the 4 PLLs. One driver already claimed the > clock, enabled it, and disabled it. Really something would need to > know that another driver in the future might want to also pick up the > clock. ->=20 ->=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 ->=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 > not 100% sure that any of these would really affect someone, but... ->=20 +> > ...there is one example case I know of where you absolutely need a > clock to stop on command (AKA it wouldn't be OK to defer till late > init). That case is for SD Card Tuning. When you're doing a voltage @@ -94,12 +80,12 @@ nt 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 +deferring it. ->=20 +> > I suppose, though, that it would be possible to distinguish those two > cases via a 2nd API call. AKA: ->=20 +> > 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. diff --git a/a/content_digest b/N1/content_digest index 0119a85..443cc5b 100644 --- a/a/content_digest +++ b/N1/content_digest @@ -26,50 +26,37 @@ "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" + "Doug Anderson <dianders@chromium.org> a \303\251crit :\n" "\n" "> Hi,\n" - ">=20\n" + "> \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 p=\n" - "wm2\n" - "> >> > is enabled at uboot, the PWM clock will be disabled at pwm0's probe.=\n" - " It\n" - "> >> > is true to close the pwm clock, and the pwm2 can't work during a whi=\n" - "le,\n" - "> >> > until the pwm2 probe, because the pwm0 and pwm2 are the same clock f=\n" - "or\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 ea=\n" - "rly\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 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 probe,\n" - "> >> > because whether or not the PWM state has been enabled in the Uboot, =\n" - "like\n" - "> >> > other modules, our chip default PWM clock registers are enabled at t=\n" - "he\n" - "> >> > beginning, read the PWM registers directly to know the PWM state. Th=\n" - "en\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=3D1. If the PWM state is disabled, we do nothing. After =\n" - "all\n" - "> >> > the PWMs are probed and all modules are probed, the clock core will =\n" - "gate\n" - "> >> > the PWM clock if the clock count is 0, and keep clk enabled if the c=\n" - "lock\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: wh=\n" - "at\n" - "> >> if we make clk_disable() and clk_unprepare() just decrement the refcou=\n" - "nt\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 the\n" "> >> bootloader will stay in such state until all drivers have had a chance\n" @@ -77,32 +64,31 @@ "> >>\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 keep\n" - "> >> clks enabled to prevent glitches. =20\n" + "> >> clks enabled to prevent glitches. \n" "> >\n" - "> > Actually, Mike had patches that introduced so called \"handoff\" clocks [=\n" - "0].\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" - ">=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" "> shared clock between the 4 PLLs. One driver already claimed the\n" "> clock, enabled it, and disabled it. Really something would need to\n" "> know that another driver in the future might want to also pick up the\n" "> clock.\n" - ">=20\n" - ">=20\n" + "> \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" - ">=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" "> not 100% sure that any of these would really affect someone, but...\n" - ">=20\n" + "> \n" "> ...there is one example case I know of where you absolutely need a\n" "> clock to stop on command (AKA it wouldn't be OK to defer till late\n" "> init). That case is for SD Card Tuning. When you're doing a voltage\n" @@ -117,12 +103,12 @@ "\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.=20\n" + "deferring it. \n" "\n" - ">=20\n" + "> \n" "> I suppose, though, that it would be possible to distinguish those two\n" "> cases via a 2nd API call. AKA:\n" - ">=20\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.\n" @@ -133,4 +119,4 @@ "rely on clk_disable() synchronicity and force users that actually allow\n" deferring clk gating to explicitly state it. -ff7a334cbb8451f2042d0b976fb410fa4741610af61426ecaa61ff956484945d +9eee09fc2b5da3acdfc15da243505be16b7517e98faa891a67b1e104c965d022
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.