Le Mon, 21 Aug 2017 09:49:52 -0700, Doug Anderson <dianders at chromium.org> a ?crit : > Hi, > > On Mon, Aug 21, 2017 at 8:39 AM, Boris Brezillon > <boris.brezillon at free-electrons.com> wrote: > > Hi Doug, > > > > Sorry for the late reply. > > > > Le Fri, 4 Aug 2017 09:22:56 -0700, > > Doug Anderson <dianders at chromium.org> a ?crit : > > > >> Hi, > >> > >> On Fri, Aug 4, 2017 at 7:48 AM, Heiko Stuebner <heiko at 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 > >> >> > 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 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? > >> >> > >> >> 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 > >> >> 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 > >> >> 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. > >> > > >> > 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. > >> > >> 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. > >> > >> > >> > 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. > >> > >> 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... > >> > >> ...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 > >> switch the actual transition is keyed off the card clock stopping. > >> That would break if there was a situation where clk_disable() didn't > >> actually do what it was supposed to. This is a bit of a contrived > >> case and probably isn't 100% relevant (I think dw_mmc, for instance, > >> 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. > > > > 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. > > > >> > >> 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. > > > > 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. > > 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 > sync and some the "default" is async). Although I guess if there's > one consistent thing about the kernel APIs it's that they're > inconsistent. :-P Yep. > > Is anyone volunteering to do this? Looks like you found a volunteer: Elaine already posted my patch to the linux-clk ML :-). > Do we want anyone with actual > "approval" authority to chime in? Don't if that's what you meant by "approval" authority, but you'll need Mike or Stephen (or both) to validate this approach.
