Re: [PATCH 2/2] clk: bcm2835: Minimise clock jitter for PCM clock

[Phil Elwell]

Hi Stefan,

On 30/05/2017 20:04, Stefan Wahren wrote:
> Hi Phil,
> 
>> Phil Elwell <phil@xxxxxxxxxxxxxxx> hat am 30. Mai 2017 um 18:28 geschrieben:
>>
>>
>> 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.
>>
>> Use this metric for parent selection for clocks requiring low jitter
>> (currently just PCM).
>>
>> Signed-off-by: Phil Elwell <phil@xxxxxxxxxxxxxxx>
>> ---
>>  drivers/clk/bcm/clk-bcm2835.c | 39 ++++++++++++++++++++++++++++++++++-----
>>  1 file changed, 34 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c
>> index facc346..e0ce5e7 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,33 @@ 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 idiv = *div & ~CM_DIV_FRAC_MASK;
>> +
>> +			high = bcm2835_clock_rate_from_divisor(clock, *prate,
>> +							       idiv);
>> +			idiv += CM_DIV_FRAC_MASK + 1;
>> +			low = bcm2835_clock_rate_from_divisor(clock, *prate,
>> +							      idiv);
> 
> What about int_div or intdiv instead of idiv as variable name?

OK - int_div matches tmp_rate.

>> +
>> +			/* Return a value which is the maximum deviation
>> +			 * below the ideal rate, for use as a metric.
>> +			 */
> 
> Please fix coding style of the comment.

Oops. I'm surprised checkpatch didn't complain about that.

Thanks,

Phil