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:49:14 +0000 |
| |
> -----Original Message----- > From: Fillion, Claude > Sent: Monday, March 21, 2022 5:12 PM > To: Adam Ford <aford173@gmail.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 > > 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 >
I am really sorry but there appears to be something going on with my setup that is causing spurious results. After sending this email I re-enabled the 6965 patch and am seeing same results I had without the patch (1.019, 10.19, 101.9, 47.68 MHz).
Please disregard my earlier email.
-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.
| |