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

[Quentin Schulz]

Hi Jerome,

On 20/06/2017 14:47, Boris Brezillon wrote:
> +Quentin
> 
> On Tue, 20 Jun 2017 14:32:30 +0200
> Jerome Brunet <jbrunet at baylibre.com> 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.
> 
> This flag in at91 clk drivers really means that rate cannot be changed
> while the clk is enabled. We have other clock where this is not a
> problem (at least, nothing in mentioned in the datasheet).
> 
>>
>> If you could apply this series and just do things as usual, It'd be awesome !
> 
> Actually, this is a feature I was pushing for a while back [1],
> because I had the same problem (one user messing up with a PLL while
> others were relying on its rate). I'm glad someone finally had time to
> provide a solution for this problem.
> 
> Quentin tested the series, and I guess he'll soon add his Tested-by.
> Note that this series does not address all problems. For example, when
> several drivers are setting a rate on different clks that take the
> same parent, the first one to set the rate and enable the clk wins.
> 
> He has an hack-ish solution for our audio-pll case preventing non audio
> users to mess up with the audio PLL. It'd be great to have a generic
> solution, though I don't know how this can be solved without advanced
> description of the clk-rate setting policy.
> 
> [1]https://patchwork.kernel.org/patch/6204221/
> 

I've tested the patch series and it seems good to me.

Tested-by: Quentin Schulz <quentin.schulz at free-electrons.com>

Our hackish solution for the moment is to deny clock children to take
the audio-pll clock as parent except if it's classd (the audio IP). That
way, we take care of the driver probing "order" (i.e., the first driver
to enable the clock will lock the rate, but maybe that rate isn't the
one we want for a more critical driver) but it is definitely not a neat
solution.

Thanks,
Quentin

-- 
Quentin Schulz, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com