Messages in this thread | | | From | "Fillion, Claude" <> | Subject | RE: [EXTERNAL] Re: [PATCH] clk: vc5: Enable VC5_HAS_PFD_FREQ_DBL on 5p49v6965 | Date | Mon, 21 Mar 2022 21:11:46 +0000 |
| |
Hello Adam and Luca,
> -----Original Message----- > From: Adam Ford <aford173@gmail.com> > Sent: Saturday, March 19, 2022 5:24 PM > To: Fillion, Claude <Claude.Fillion@mksinst.com> > Cc: Luca Ceresoli <luca@lucaceresoli.net>; linux-clk@vger.kernel.org; > aford@beaconembedded.com; cstevens@beaconembedded.com; Michael > Turquette <mturquette@baylibre.com>; Stephen Boyd > <sboyd@kernel.org>; linux-kernel@vger.kernel.org; Marek Vasut > <marek.vasut@gmail.com> > Subject: Re: [EXTERNAL] Re: [PATCH] clk: vc5: Enable > VC5_HAS_PFD_FREQ_DBL on 5p49v6965 > > On Thu, Mar 17, 2022 at 1:57 PM Fillion, Claude > <Claude.Fillion@mksinst.com> wrote: > > > > Hello Luca, > > > > > -----Original Message----- > > > From: Luca Ceresoli <luca@lucaceresoli.net> > > > Sent: Tuesday, March 15, 2022 6:53 PM > > > To: Fillion, Claude <Claude.Fillion@mksinst.com>; Adam Ford > > > <aford173@gmail.com>; linux-clk@vger.kernel.org > > > Cc: aford@beaconembedded.com; cstevens@beaconembedded.com; > Michael > > > Turquette <mturquette@baylibre.com>; Stephen Boyd > > > <sboyd@kernel.org>; linux-kernel@vger.kernel.org; Marek Vasut > > > <marek.vasut@gmail.com> > > > Subject: Re: [EXTERNAL] Re: [PATCH] clk: vc5: Enable > > > VC5_HAS_PFD_FREQ_DBL on 5p49v6965 > > > > > > 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@lucaceresoli.net> > > > >> Sent: Tuesday, March 15, 2022 4:55 AM > > > >> To: Adam Ford <aford173@gmail.com>; linux-clk@vger.kernel.org > > > >> Cc: aford@beaconembedded.com; > cstevens@beaconembedded.com; > > > Fillion, > > > >> Claude <Claude.Fillion@mksinst.com>; Michael Turquette > > > >> <mturquette@baylibre.com>; Stephen Boyd <sboyd@kernel.org>; > > > >> linux- kernel@vger.kernel.org > > > >> 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@mksinst.com> > > > >>> Signed-off-by: Adam Ford <aford173@gmail.com> > > > >>> > > > >>> 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 > > > > Not sure what I did wrong with my earlier testing, but I am now seeing both > parts respond similarly to the doubler bit being set. > > > > With doubler bit disabled (register 0x10, value 0xa0), I set the output > frequencies to 1, 10, 100, and 46.8MHz. > > > > After setting doubler bit (0xa8), I saw frequencies of 1.260, 12.60, 126.0, > and 58.9 Mhz for both 6901 and 6965 parts. > > > > So from my testing the doubler bit seems to behave similarly for both > parts. > > > > At this point I will leave my unofficial testing and move on to writing a > consumer driver. > > I don't have a scope to measure the exact frequencies, but I was able to test > it with both USB and Ethernet, which are clock from the versaclock, and I can > check the output frequencies against the clk_summary in debugfs. > > Without this patch: > > clock-controller.mux 1 1 0 25000000 > 0 0 50000 Y > clock-controller.out0_sel_i2cb 0 0 0 > 25000000 0 0 50000 Y > clock-controller.pfd 1 1 0 25000000 > 0 0 50000 Y > clock-controller.pll 1 1 0 2800000000 > 0 0 50000 Y > clock-controller.fod3 1 1 0 > 24576000 0 0 50000 Y > clock-controller.out4 1 1 0 > 24576000 0 0 50000 Y > clock-controller.fod2 0 0 0 > 24000000 0 0 50000 Y > clock-controller.out3 0 0 0 > 24000000 0 0 50000 Y > clock-controller.fod1 0 0 0 > 24000000 0 0 50000 Y > clock-controller.out2 0 0 0 > 24000000 0 0 50000 Y > clock-controller.fod0 0 0 0 > 24000000 0 0 50000 Y > clock-controller.out1 0 0 0 > 24000000 0 0 50000 Y > > > With this patch: > > clock-controller.mux 1 1 0 25000000 > 0 0 50000 Y > clock-controller.out0_sel_i2cb 0 0 0 > 25000000 0 0 50000 Y > clock-controller.dbl 1 1 0 25000000 > 0 0 50000 Y > clock-controller.pfd 1 1 0 25000000 > 0 0 50000 Y > clock-controller.pll 1 1 0 > 2800000000 0 0 50000 Y > clock-controller.fod3 1 1 0 > 24576000 0 0 50000 Y > clock-controller.out4 1 1 0 > 24576000 0 0 50000 Y > clock-controller.fod2 0 0 0 > 24000000 0 0 50000 Y > clock-controller.out3 0 0 0 > 24000000 0 0 50000 Y > clock-controller.fod1 0 0 0 > 24000000 0 0 50000 Y > clock-controller.out2 0 0 0 > 24000000 0 0 50000 Y > clock-controller.fod0 0 0 0 > 24000000 0 0 50000 Y > clock-controller.out1 0 0 0 > 24000000 0 0 50000 Y > > From what I can tell, the only thing that changes is the introduction of clock- > controller.dbl into the clock dump. > In my interpretation of reading the programmer's manual, the frequency > that is doubled is the reference frequency, but based on looking at the clock > dump, it's not obvious what's happening. > > Having said that, if Claude is measuring incorrect frequencies, I am fine with > abandoning this patch.
For what it's worth, I went back and tested output frequencies with patch removed.
Here is what I observed(starting with frequencies of 1, 10, 100, 46.8MHz and changing register 0x10 from 0xa0 to 0xa8).
6901: 1.260, 12.60, 126.0, 58.9 6965 with patch: 1.260, 12.60, 126.0, 58.9 6965 without patch: 1.019, 10.19, 101.9, 47.68
From my limited testing (and knowledge for sure) it seems that in this test the patch has some benefit.
-Claude
> > adam > > > > > Regards, > > Claude > > > > > ========================================================== > ============ > > This message and any attachments are intended only for the designated > recipient(s) and may contain confidential or proprietary information and be > subject to the attorney-client privilege or other confidentiality protections. If > you are not a designated recipient, you may not review, use, copy or > distribute this message or any attachments. If you received this email in > error, please notify the sender by reply e-mail and permanently delete the > original and any copies of this message and any attachments thereto. Thank > you.
====================================================================== This message and any attachments are intended only for the designated recipient(s) and may contain confidential or proprietary information and be subject to the attorney-client privilege or other confidentiality protections. If you are not a designated recipient, you may not review, use, copy or distribute this message or any attachments. If you received this email in error, please notify the sender by reply e-mail and permanently delete the original and any copies of this message and any attachments thereto. Thank you.
| |