| Message ID | 20220313115704.301718-1-aford173@gmail.com (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Headers | show |
| Series | clk: vc5: Enable VC5_HAS_PFD_FREQ_DBL on 5p49v6965 | expand |
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 */ 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,
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. > 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. 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. -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.
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?
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. 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.
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. 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.
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.
> -----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.
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, }; static const struct i2c_device_id vc5_id[] = {
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>