[PATCH v3 00/10] clk: implement clock rate protection mechanism

[sboyd@codeaurora.org (Stephen Boyd)]

On 07/26, Jerome Brunet wrote:
> On Tue, 2017-07-11 at 18:16 -0700, Stephen Boyd wrote:
> > On 06/21, Jerome Brunet wrote:
> > > 
> > > I managed to have a run on kci - based on v4.12-rc6:
> > > https://kernelci.org/boot/all/job/khilman/branch/to-build/kernel/v4.12-rc6-1
> > > 0-ge
> > > a373ddef830/
> > > 
> > > There was no build regression but kci did find one boot regression on qcom
> > > platforms:
> > > * qcom-apq8064-cm-qs600
> > > * qcom-apq8064-ifc6410
> > > 
> > > it seems the problem is coming from the clock used by the mmc driver
> > > (drivers/mmc/host/mmci.c)
> > > 
> > > the driver does following sequence:
> > > * get_clk
> > > * prepare_enable
> > > * get_rate
> > > * set_rate
> > > * ...
> > > 
> > > with clock SDCx_clk (qcom_apq8064.dtsi:1037). This clock has
> > > CLK_SET_RATE_PARENT
> > > flag so it will transmit the request to its parent.
> > > The parent of this clock is SDCx_src which has the CLK_SET_RATE_GATE flag.
> > > 
> > > This can't work with sequence above. The flag was clearly bypassed before,
> > > so I
> > > think it would be best to remove CLK_SET_RATE_GATE from the SDCx_src clocks
> > > 
> > > Stephen, would you agree ?
> > > 
> > > There was an issue with the chip board as well but this particular board has
> > > not
> > > been very reliable in this lab lately. It has failed on several other job.
> > > 
> > 
> > Yes the flag was never working before and we've been ignoring the
> > subtle problems that it causes to change the rate while the clk
> > is enabled. On these older SoCs, all the clks marked with
> > CLK_SET_RATE_GATE need to implement something to forcibly disable
> > all child clks across the rate change. That is the hardware
> > recommended procedure.
> 
> Seems that HW has been doing fine w/o recommended procedure so far ;)

Dubious at best. There's only light testing with upstream kernels
on these SoCs, and probably "doing fine" is more like "nobody has
reported a problem" in this case.

> 
> > 
> > So something like
> > 
> > 	get_clk
> > 	prepare_enable
> > 	get_rate
> > 	set_rate
> > 	 call ->set_rate() op
> > 	 forcibly disable children of rcgs
> > 	 actually change the rate of rcg
> > 	 forcibly enable all children that were disabled
> > 
> > and have that happen under one big spinlock local to the qcom
> > driver I suppose so that enable/disable can't leak into the rate
> > change. Fun! I'll make an attempt at implementing the right
> > solution tomorrow/tonight. Of course, we can remove the flag from
> > qcom drivers if it's blocking this series.
> > 
> 
> Yeah, it is probably blocking the series.
> 
> In the following mail, I've only replied where I wanted to discuss a bit more.
> For everything else, please consider the comments agreed and the modifications
> done as requested.
> 

Ok. Good that we can move forward in some way so this is not a
blocker. I've almost completed writing the qcom code to force
clks on and off so it will be resolved one way or the other soon.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project