On 31/05/2017 22:36, Eric Anholt wrote:
> Phil Elwell <[email protected]> writes:
>
>> Fractional clock dividers generate accurate average frequencies but
>> with jitter, particularly when the integer divisor is small.
>>
>> Introduce a new metric of clock accuracy to penalise clocks with a good
>> average but worse jitter compared to clocks with an average which is no
>> better but with lower jitter. The metric is the ideal rate minus the
>> worse deviation from that ideal using the nearest integer divisors.
>
> "worst" the second time
According to the rules of English grammar, you should only use the superlative
("worst") when comparing something to a group. In this case we are only
comparing two things - the distance to the nearest-neighbour integers - so the
comparitive ("worse") is correct.
>> Use this metric for parent selection for clocks requiring low jitter
>> (currently just PCM).
>>
>> Signed-off-by: Phil Elwell <[email protected]>
>> ---
>> drivers/clk/bcm/clk-bcm2835.c | 40 +++++++++++++++++++++++++++++++++++-----
>> 1 file changed, 35 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c
>> index 81ecd4c..c7ee951 100644
>> --- a/drivers/clk/bcm/clk-bcm2835.c
>> +++ b/drivers/clk/bcm/clk-bcm2835.c
>> @@ -530,6 +530,7 @@ struct bcm2835_clock_data {
>>
>> bool is_vpu_clock;
>> bool is_mash_clock;
>> + bool low_jitter;
>>
>> u32 tcnt_mux;
>> };
>> @@ -1124,7 +1125,8 @@ static unsigned long bcm2835_clock_choose_div_and_prate(struct clk_hw *hw,
>> int parent_idx,
>> unsigned long rate,
>> u32 *div,
>> - unsigned long *prate)
>> + unsigned long *prate,
>> + unsigned long *avgrate)
>> {
>> struct bcm2835_clock *clock = bcm2835_clock_from_hw(hw);
>> struct bcm2835_cprman *cprman = clock->cprman;
>> @@ -1136,11 +1138,34 @@ static unsigned long bcm2835_clock_choose_div_and_prate(struct clk_hw *hw,
>> parent = clk_hw_get_parent_by_index(hw, parent_idx);
>>
>> if (!(BIT(parent_idx) & data->set_rate_parent)) {
>> + unsigned long tmp_rate;
>> +
>> *prate = clk_hw_get_rate(parent);
>> *div = bcm2835_clock_choose_div(hw, rate, *prate, true);
>>
>> - return bcm2835_clock_rate_from_divisor(clock, *prate,
>> - *div);
>> + tmp_rate = bcm2835_clock_rate_from_divisor(clock, *prate, *div);
>> + *avgrate = tmp_rate;
>> +
>> + if (data->low_jitter && (*div & CM_DIV_FRAC_MASK)) {
>> + unsigned long high, low;
>> + u32 int_div = *div & ~CM_DIV_FRAC_MASK;
>> +
>> + high = bcm2835_clock_rate_from_divisor(clock, *prate,
>> + int_div);
>> + int_div += CM_DIV_FRAC_MASK + 1;
>> + low = bcm2835_clock_rate_from_divisor(clock, *prate,
>> + int_div);
>> +
>> + /*
>> + * Return a value which is the maximum deviation
>> + * below the ideal rate, for use as a metric.
>> + */
>> + if ((tmp_rate - low) < (high - tmp_rate))
>> + tmp_rate = low;
>> + else
>> + tmp_rate -= high - tmp_rate;
>
> Simplification suggestion: Remove tmp_rate variable, just assign to
> rate_from_divisor result to *avgrate. At the end of the low_jitter
> block, just "return *avgrate - max(*avgrate - low, high - *avgrate)".
Yes, I like that.
> With that, feel free to add:
>
> Reviewed-by: Eric Anholt <[email protected]>
Thanks - I will.
Phil