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

[Jerome Brunet]

On Tue, 2017-06-20 at 14:32 +0200, Jerome Brunet wrote:
> On Tue, 2017-06-20 at 13:54 +0200, Linus Walleij wrote:
> > On Tue, Jun 20, 2017 at 12:50 PM, Jerome Brunet <jbrunet at baylibre.com>
> > wrote:
> > > On Tue, 2017-06-20 at 11:07 +0200, Linus Walleij wrote:
> > > When the rate is critical to perform a particular task. My example is the
> > > audio
> > > and i2s output. You can't tolerate glitches during the playback, the end
> > > user
> > > would complain (longer description in the original RFC)
> > 
> > I see. Thanks for your detailed explanation!
> > 
> > > It also fixes the behavior of CLK_SET_RATE_GATE flag, which is why I put
> > > you
> > > in
> > > CC.
> > > 
> > > ux500 uses this flag several time, I'd like make sure people are not
> > > relying
> > > on
> > > its broken implementation.
> > 
> > Ux500 audio is broken, but I'm fixing it a little at a time...
> 
> No problem with Ux500 audio, don't worry :)
> Audio is just one application among others.
> 
> The concern regarding ux500 is that the clock controller declares clocks using
> the CLK_SET_RATE_GATE flag (like qcom, at91 and several other)
> 
> here is the definition:
> #define CLK_SET_RATE_GATE	BIT(0) /* must be gated across rate change */
> 
> My interpretation it that, as long as clock is enabled, rate can't change.
> 
> The implementation of this flag is currently broken:
> * If you call set_rate on directly on the clock while it is enabled, you will
> get -EBUSY, as expected
> * If you call set_rate on its parent, rate will change, changing the rate of
> the
> child clock as well ...
> 
> With this patchset applied, calling set_rate on the parent would also return
> -EBUSY, enforcing the "rate can't change while enabled" property.
> 
> To build confidence that this won't be causing regression, I'd like to check
> that platform using this flag are no relying on the broken behavior.
> 
> I've included the clock maintainers of at91, qcom, and ux500 (you) in this
> thread because they are the heaviest users of this flag, so the more likely to
> report a problem.
> 
> If you could apply this series and just do things as usual, It'd be awesome !
> 
> I'll also seek the help of the kci guys, see if it is possible to give the
> series spin on it.
> 

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-10-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.

> > 
> > Yours,
> > Linus Walleij
> 
>