diff for duplicates of <2250536.aM833QY2s7@phil> diff --git a/a/1.txt b/N1/1.txt index 36f6044..647551a 100644 --- a/a/1.txt +++ b/N1/1.txt @@ -2,53 +2,41 @@ Hi, Am Freitag, 4. August 2017, 16:07:01 CEST schrieb Boris Brezillon: > +Stephen, Mike and the linux-clk ML. ->=20 +> > On Fri, 4 Aug 2017 20:45:04 +0800 > "David.Wu" <david.wu@rock-chips.com> wrote: ->=20 +> > > Hi Boris & Heiko, -> >=20 -> > =E5=9C=A8 2016/3/31 4:03, Boris BREZILLON =E5=86=99=E9=81=93: -> > > + /* Keep the PWM clk enabled if the PWM appears to be up and running= -=2E */ +> > +> > 在 2016/3/31 4:03, Boris BREZILLON 写道: +> > > + /* Keep the PWM clk enabled if the PWM appears to be up and running. */ > > > + pwm_get_state(pc->chip.pwms, &pstate); > > > + if (!pstate.enabled) -> > > + clk_disable(pc->clk); =20 -> >=20 -> > We found a issue recently, if the pwm0 is not enabled at uboot and pwm2= -=20 -> > is enabled at uboot, the PWM clock will be disabled at pwm0's probe. It= -=20 -> > is true to close the pwm clock, and the pwm2 can't work during a while,= -=20 -> > until the pwm2 probe, because the pwm0 and pwm2 are the same clock for= -=20 +> > > + clk_disable(pc->clk); +> > +> > 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. -> >=20 -> > So we need to get all the PWMs state in a public place where it's early= -=20 -> > than the PWM probe, if that's the way it is. Then keep the PWM clk=20 -> > enabled if theis is one PWM appears to be up and running. The place=20 +> > +> > 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. -> >=20 -> > Another way is that we don't enable pwm clock firstly at PWM probe,=20 -> > because whether or not the PWM state has been enabled in the Uboot, lik= -e=20 -> > other modules, our chip default PWM clock registers are enabled at the= -=20 -> > beginning, read the PWM registers directly to know the PWM state. Then= -=20 -> > if the PWM state is enabled, call the enable_clk(pc->clk) to add the=20 -> > clock count=3D1. If the PWM state is disabled, we do nothing. After all= -=20 -> > the PWMs are probed and all modules are probed, the clock core will gat= -e=20 -> > the PWM clock if the clock count is 0, and keep clk enabled if the cloc= -k=20 +> > +> > 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. -> >=20 +> > > > How do you feel about it? ->=20 +> > 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 refcount @@ -56,7 +44,7 @@ k=20 > 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()). ->=20 +> > 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. @@ -83,25 +71,25 @@ Heiko > Date: Fri, 4 Aug 2017 15:55:49 +0200 > Subject: [PATCH] clk: Keep clocks in their initial state until > clk_disable_unused() is called ->=20 +> > Some drivers are briefly preparing+enabling the clock in their > ->probe() hook and disable+unprepare them before leaving the function. ->=20 +> > This can be problem if a clock is shared between different devices, and > one of these devices is critical to the system. If this clock is > enabled/disabled by a non-critical device before the driver of the > critical one had a chance to enable+prepare it, there might be a short > period of time during which the critical device is not clocked. ->=20 +> > To solve this problem, we save the initial clock state (at registration > time) and prevent the clock from being disabled until kernel init is done > (which is when clk_disable_unused() is called). ->=20 +> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> > --- > drivers/clk/clk.c | 29 +++++++++++++++++++++++++++-- > 1 file changed, 27 insertions(+), 2 deletions(-) ->=20 +> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index fc58c52a26b4..3f61374a364b 100644 > --- a/drivers/clk/clk.c @@ -116,68 +104,66 @@ Heiko > unsigned int prepare_count; > unsigned long min_rate; > @@ -486,7 +488,7 @@ static void clk_core_unprepare(struct clk_core *core) -> =20 +> > trace_clk_unprepare(core); -> =20 +> > - if (core->ops->unprepare) > + if (core->ops->unprepare && !core->keep_prepared) > core->ops->unprepare(core->hw); -> =20 +> > trace_clk_unprepare_complete(core); > @@ -602,7 +604,7 @@ static void clk_core_disable(struct clk_core *core) -> =20 +> > trace_clk_disable_rcuidle(core); -> =20 +> > - if (core->ops->disable) > + if (core->ops->disable && !core->keep_enabled) > core->ops->disable(core->hw); -> =20 +> > trace_clk_disable_complete_rcuidle(core); -> @@ -739,6 +741,12 @@ static void clk_unprepare_unused_subtree(struct clk_= -core *core) +> @@ -739,6 +741,12 @@ static void clk_unprepare_unused_subtree(struct clk_core *core) > hlist_for_each_entry(child, &core->children, child_node) > clk_unprepare_unused_subtree(child); -> =20 +> > + /* > + * Reset the ->keep_prepared flag so that subsequent calls to > + * clk_unprepare() on this clk actually unprepare it. > + */ -> + core->keep_prepared =3D false; +> + core->keep_prepared = false; > + > if (core->prepare_count) > return; -> =20 -> @@ -770,6 +778,12 @@ static void clk_disable_unused_subtree(struct clk_co= -re *core) -> =20 -> flags =3D clk_enable_lock(); -> =20 +> +> @@ -770,6 +778,12 @@ static void clk_disable_unused_subtree(struct clk_core *core) +> +> flags = clk_enable_lock(); +> > + /* > + * Reset the ->keep_enabled flag so that subsequent calls to > + * clk_disable() on this clk actually disable it. > + */ -> + core->keep_enabled =3D false; +> + core->keep_enabled = false; > + > if (core->enable_count) > goto unlock_out; -> =20 +> > @@ -2446,6 +2460,17 @@ static int __clk_core_init(struct clk_core *core) -> core->accuracy =3D 0; -> =20 +> core->accuracy = 0; +> > /* > + * We keep track of the initial clk status to keep clks in the state > + * they were left in by the bootloader until all drivers had a chance > + * to keep them prepared/enabled if they need to. > + */ > + if (core->ops->is_prepared && !clk_ignore_unused) -> + core->keep_prepared =3D core->ops->is_prepared(core->hw); +> + core->keep_prepared = core->ops->is_prepared(core->hw); > + > + if (core->ops->is_enabled && !clk_ignore_unused) -> + core->keep_enabled =3D core->ops->is_enabled(core->hw); +> + core->keep_enabled = core->ops->is_enabled(core->hw); > + > + /* > * Set clk's phase. > * Since a phase is by definition relative to its parent, just > * query the current clock phase, or just assume it's in phase. ->=20 ->=20 +> +> diff --git a/a/content_digest b/N1/content_digest index e4d4852..768f786 100644 --- a/a/content_digest +++ b/N1/content_digest @@ -23,53 +23,41 @@ "\n" "Am Freitag, 4. August 2017, 16:07:01 CEST schrieb Boris Brezillon:\n" "> +Stephen, Mike and the linux-clk ML.\n" - ">=20\n" + "> \n" "> On Fri, 4 Aug 2017 20:45:04 +0800\n" "> \"David.Wu\" <david.wu@rock-chips.com> wrote:\n" - ">=20\n" + "> \n" "> > Hi Boris & Heiko,\n" - "> >=20\n" - "> > =E5=9C=A8 2016/3/31 4:03, Boris BREZILLON =E5=86=99=E9=81=93:\n" - "> > > +\t/* Keep the PWM clk enabled if the PWM appears to be up and running=\n" - "=2E */\n" + "> > \n" + "> > \345\234\250 2016/3/31 4:03, Boris BREZILLON \345\206\231\351\201\223:\n" + "> > > +\t/* Keep the PWM clk enabled if the PWM appears to be up and running. */\n" "> > > +\tpwm_get_state(pc->chip.pwms, &pstate);\n" "> > > +\tif (!pstate.enabled)\n" - "> > > +\t\tclk_disable(pc->clk); =20\n" - "> >=20\n" - "> > We found a issue recently, if the pwm0 is not enabled at uboot and pwm2=\n" - "=20\n" - "> > is enabled at uboot, the PWM clock will be disabled at pwm0's probe. It=\n" - "=20\n" - "> > is true to close the pwm clock, and the pwm2 can't work during a while,=\n" - "=20\n" - "> > until the pwm2 probe, because the pwm0 and pwm2 are the same clock for=\n" - "=20\n" + "> > > +\t\tclk_disable(pc->clk); \n" + "> > \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" - "> >=20\n" - "> > So we need to get all the PWMs state in a public place where it's early=\n" - "=20\n" - "> > than the PWM probe, if that's the way it is. Then keep the PWM clk=20\n" - "> > enabled if theis is one PWM appears to be up and running. The place=20\n" + "> > \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" - "> >=20\n" - "> > Another way is that we don't enable pwm clock firstly at PWM probe,=20\n" - "> > because whether or not the PWM state has been enabled in the Uboot, lik=\n" - "e=20\n" - "> > other modules, our chip default PWM clock registers are enabled at the=\n" - "=20\n" - "> > beginning, read the PWM registers directly to know the PWM state. Then=\n" - "=20\n" - "> > if the PWM state is enabled, call the enable_clk(pc->clk) to add the=20\n" - "> > clock count=3D1. If the PWM state is disabled, we do nothing. After all=\n" - "=20\n" - "> > the PWMs are probed and all modules are probed, the clock core will gat=\n" - "e=20\n" - "> > the PWM clock if the clock count is 0, and keep clk enabled if the cloc=\n" - "k=20\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, 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" - "> >=20\n" + "> > \n" "> > How do you feel about it?\n" - ">=20\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: what\n" "> if we make clk_disable() and clk_unprepare() just decrement the refcount\n" @@ -77,7 +65,7 @@ "> 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" - ">=20\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 keep\n" "> clks enabled to prevent glitches.\n" @@ -104,25 +92,25 @@ "> Date: Fri, 4 Aug 2017 15:55:49 +0200\n" "> Subject: [PATCH] clk: Keep clocks in their initial state until\n" "> clk_disable_unused() is called\n" - ">=20\n" + "> \n" "> Some drivers are briefly preparing+enabling the clock in their\n" "> ->probe() hook and disable+unprepare them before leaving the function.\n" - ">=20\n" + "> \n" "> This can be problem if a clock is shared between different devices, and\n" "> one of these devices is critical to the system. If this clock is\n" "> enabled/disabled by a non-critical device before the driver of the\n" "> critical one had a chance to enable+prepare it, there might be a short\n" "> period of time during which the critical device is not clocked.\n" - ">=20\n" + "> \n" "> To solve this problem, we save the initial clock state (at registration\n" "> time) and prevent the clock from being disabled until kernel init is done\n" "> (which is when clk_disable_unused() is called).\n" - ">=20\n" + "> \n" "> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>\n" "> ---\n" "> drivers/clk/clk.c | 29 +++++++++++++++++++++++++++--\n" "> 1 file changed, 27 insertions(+), 2 deletions(-)\n" - ">=20\n" + "> \n" "> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c\n" "> index fc58c52a26b4..3f61374a364b 100644\n" "> --- a/drivers/clk/clk.c\n" @@ -137,70 +125,68 @@ "> \tunsigned int\t\tprepare_count;\n" "> \tunsigned long\t\tmin_rate;\n" "> @@ -486,7 +488,7 @@ static void clk_core_unprepare(struct clk_core *core)\n" - "> =20\n" + "> \n" "> \ttrace_clk_unprepare(core);\n" - "> =20\n" + "> \n" "> -\tif (core->ops->unprepare)\n" "> +\tif (core->ops->unprepare && !core->keep_prepared)\n" "> \t\tcore->ops->unprepare(core->hw);\n" - "> =20\n" + "> \n" "> \ttrace_clk_unprepare_complete(core);\n" "> @@ -602,7 +604,7 @@ static void clk_core_disable(struct clk_core *core)\n" - "> =20\n" + "> \n" "> \ttrace_clk_disable_rcuidle(core);\n" - "> =20\n" + "> \n" "> -\tif (core->ops->disable)\n" "> +\tif (core->ops->disable && !core->keep_enabled)\n" "> \t\tcore->ops->disable(core->hw);\n" - "> =20\n" + "> \n" "> \ttrace_clk_disable_complete_rcuidle(core);\n" - "> @@ -739,6 +741,12 @@ static void clk_unprepare_unused_subtree(struct clk_=\n" - "core *core)\n" + "> @@ -739,6 +741,12 @@ static void clk_unprepare_unused_subtree(struct clk_core *core)\n" "> \thlist_for_each_entry(child, &core->children, child_node)\n" "> \t\tclk_unprepare_unused_subtree(child);\n" - "> =20\n" + "> \n" "> +\t/*\n" "> +\t * Reset the ->keep_prepared flag so that subsequent calls to\n" "> +\t * clk_unprepare() on this clk actually unprepare it.\n" "> +\t */\n" - "> +\tcore->keep_prepared =3D false;\n" + "> +\tcore->keep_prepared = false;\n" "> +\n" "> \tif (core->prepare_count)\n" "> \t\treturn;\n" - "> =20\n" - "> @@ -770,6 +778,12 @@ static void clk_disable_unused_subtree(struct clk_co=\n" - "re *core)\n" - "> =20\n" - "> \tflags =3D clk_enable_lock();\n" - "> =20\n" + "> \n" + "> @@ -770,6 +778,12 @@ static void clk_disable_unused_subtree(struct clk_core *core)\n" + "> \n" + "> \tflags = clk_enable_lock();\n" + "> \n" "> +\t/*\n" "> +\t * Reset the ->keep_enabled flag so that subsequent calls to\n" "> +\t * clk_disable() on this clk actually disable it.\n" "> +\t */\n" - "> +\tcore->keep_enabled =3D false;\n" + "> +\tcore->keep_enabled = false;\n" "> +\n" "> \tif (core->enable_count)\n" "> \t\tgoto unlock_out;\n" - "> =20\n" + "> \n" "> @@ -2446,6 +2460,17 @@ static int __clk_core_init(struct clk_core *core)\n" - "> \t\tcore->accuracy =3D 0;\n" - "> =20\n" + "> \t\tcore->accuracy = 0;\n" + "> \n" "> \t/*\n" "> +\t * We keep track of the initial clk status to keep clks in the state\n" "> +\t * they were left in by the bootloader until all drivers had a chance\n" "> +\t * to keep them prepared/enabled if they need to.\n" "> +\t */\n" "> +\tif (core->ops->is_prepared && !clk_ignore_unused)\n" - "> +\t\tcore->keep_prepared =3D core->ops->is_prepared(core->hw);\n" + "> +\t\tcore->keep_prepared = core->ops->is_prepared(core->hw);\n" "> +\n" "> +\tif (core->ops->is_enabled && !clk_ignore_unused)\n" - "> +\t\tcore->keep_enabled =3D core->ops->is_enabled(core->hw);\n" + "> +\t\tcore->keep_enabled = core->ops->is_enabled(core->hw);\n" "> +\n" "> +\t/*\n" "> \t * Set clk's phase.\n" "> \t * Since a phase is by definition relative to its parent, just\n" "> \t * query the current clock phase, or just assume it's in phase.\n" - ">=20\n" - >=20 + "> \n" + > -7aae1d541d9fd9c4ca501b5f634b10576345a7a3c199441546cadd3acbec5df1 +d6e5e93078760b008fab45a6ee196492c6016bfa0219eeec95f9c8d666ff4598
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.