Re: [EXTERNAL] Re: [PATCH] clk: vc5: Enable VC5_HAS_PFD_FREQ_DBL on 5p49v6965

[Luca Ceresoli]

Hi Claude,

[adding Marek in Cc:, the original author of the driver and also of the
frequency doubler]

On 15/03/22 20:34, Fillion, Claude wrote:
> Hello Luca,
> 
> I will defer to Adam, but a few comments:
> 
>> -----Original Message-----
>> From: Luca Ceresoli <luca@xxxxxxxxxxxxxxxx>
>> Sent: Tuesday, March 15, 2022 4:55 AM
>> To: Adam Ford <aford173@xxxxxxxxx>; linux-clk@xxxxxxxxxxxxxxx
>> Cc: aford@xxxxxxxxxxxxxxxxxx; cstevens@xxxxxxxxxxxxxxxxxx;
>> Fillion, Claude <Claude.Fillion@xxxxxxxxxxx>; Michael Turquette
>> <mturquette@xxxxxxxxxxxx>; Stephen Boyd <sboyd@xxxxxxxxxx>; linux-
>> kernel@xxxxxxxxxxxxxxx
>> Subject: [EXTERNAL] Re: [PATCH] clk: vc5: Enable VC5_HAS_PFD_FREQ_DBL
>> on 5p49v6965
>>
>> This email originated outside of MKS.  Use caution when sharing information
>> or opening attachments and links.
>>
>> ----------------------------------------------------------------------------------------------
>> ----------------------------------------------
>> Hi Adam, Claude,
>>
>> thanks for your patch.
>>
>> On 13/03/22 12:57, Adam Ford wrote:
>>> The 5p49v6965 has a reference clock frequency doubler.
>>> Enabling it adds versaclock_som.dbl to the clock tree, but the output
>>> frequency remains correct.
>>>
>>> Suggested-by: Claude Fillion <Claude.Fillion@xxxxxxxxxxx>
>>> Signed-off-by: Adam Ford <aford173@xxxxxxxxx>
>>>
>>> diff --git a/drivers/clk/clk-versaclock5.c
>>> b/drivers/clk/clk-versaclock5.c index e7be3e54b9be..4d190579e874
>>> 100644
>>> --- a/drivers/clk/clk-versaclock5.c
>>> +++ b/drivers/clk/clk-versaclock5.c
>>> @@ -1211,7 +1211,7 @@ static const struct vc5_chip_info
>> idt_5p49v6965_info = {
>>>  	.model = IDT_VC6_5P49V6965,
>>>  	.clk_fod_cnt = 4,
>>>  	.clk_out_cnt = 5,
>>> -	.flags = VC5_HAS_BYPASS_SYNC_BIT,
>>> +	.flags = VC5_HAS_BYPASS_SYNC_BIT | VC5_HAS_PFD_FREQ_DBL,
>>
>>
>> If my understanding is correct, the doubler is not mentioned by the
>> datasheet, but it exists. Maybe it's worth a line of comment to help future
>> readers not waste their time in finding out:
>>   /* Frequency doubler not mentioned on datasheet */
>>
> 
> I see the doubler bit mentioned in Table 25 of both v6 and v6e specs.  It is named differently, but appears to have the same purpose. 

Well, literally speaking what I wrote is correct: the _datasheet_ does
not mention the doubler. Table 25 you mention is on the "Register
Description and Programming Guide".

Practically speaking I would expect the datasheet to mention the
hardware blocks including the doubler, but apparently Renesas has a
different opinion and perhaps they are not alone.

So I think you can forget about my proposal to add a comment.

>> Can you confirm that:
>>  - the en_ref_doubler bit value defaults to zero when reading it, as the
>>    register guide says?
>>  - if set to 1 the frequencies double?
>>
>> With that confirmed, the patch looks good.
>>
>> Thanks,
>> --
>> Luca
> 
> I played around a bit with the programming board today and did not see what I expected to see.
> 
> Using i2cget I see that the register in question (0x10) has a default value of 0xA0 for both 6901 and 6965.  Thus it seems disabled by default for both parts.

Coherently with the Register guide. OK.

> Starting at my base frequency of 46.8MHz, setting the bit to 1 (i2cset)  changes the output  frequency to 59.04MHz for the 6901 part, and to 47.7MHz for the 6965 part.  So setting the 'doubler' bit changes output frequency for both parts, but not the same amount.
> 
> Not sure of the meaning, just want to pass the information along.

Me neither.

I have no clever idea, only this one that I consider unlikely: by
enabling the doubler you may have increased some internal frequency
above its allowed range and thus the chip is not working properly
anymore. Can you use a lower base frequency or check the PLL settings to
ensure you are not exceeding some range?

What output frequency are you measuring? OUT0 or another one? What
frequency do you measure with en_ref_doubler = 0?

-- 
Luca