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.
