From mboxrd@z Thu Jan 1 00:00:00 1970 From: jbrunet@baylibre.com (Jerome Brunet) Date: Wed, 21 Jun 2017 15:15:03 +0200 Subject: [PATCH v3 00/10] clk: implement clock rate protection mechanism In-Reply-To: <1497961950.7387.7.camel@baylibre.com> References: <20170612194438.12298-1-jbrunet@baylibre.com> <1497955822.7387.3.camel@baylibre.com> <1497961950.7387.7.camel@baylibre.com> Message-ID: <1498050903.7387.12.camel@baylibre.com> To: linus-amlogic@lists.infradead.org List-Id: linus-amlogic.lists.infradead.org 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 > > 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 > >