| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 21 Mar 2022 21:11:46 +0000
From: "Fillion, Claude" <Claude.Fillion@...inst.com>
To: Adam Ford <aford173@...il.com>
CC: Luca Ceresoli <luca@...aceresoli.net>,
"linux-clk@...r.kernel.org" <linux-clk@...r.kernel.org>,
"aford@...conembedded.com" <aford@...conembedded.com>,
"cstevens@...conembedded.com" <cstevens@...conembedded.com>,
Michael Turquette <mturquette@...libre.com>,
Stephen Boyd <sboyd@...nel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Marek Vasut <marek.vasut@...il.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@...il.com>
> Sent: Saturday, March 19, 2022 5:24 PM
> To: Fillion, Claude <Claude.Fillion@...inst.com>
> Cc: Luca Ceresoli <luca@...aceresoli.net>; linux-clk@...r.kernel.org;
> aford@...conembedded.com; cstevens@...conembedded.com; Michael
> Turquette <mturquette@...libre.com>; Stephen Boyd
> <sboyd@...nel.org>; linux-kernel@...r.kernel.org; Marek Vasut
> <marek.vasut@...il.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@...inst.com> wrote:
> >
> > Hello Luca,
> >
> > > -----Original Message-----
> > > From: Luca Ceresoli <luca@...aceresoli.net>
> > > Sent: Tuesday, March 15, 2022 6:53 PM
> > > To: Fillion, Claude <Claude.Fillion@...inst.com>; Adam Ford
> > > <aford173@...il.com>; linux-clk@...r.kernel.org
> > > Cc: aford@...conembedded.com; cstevens@...conembedded.com;
> Michael
> > > Turquette <mturquette@...libre.com>; Stephen Boyd
> > > <sboyd@...nel.org>; linux-kernel@...r.kernel.org; Marek Vasut
> > > <marek.vasut@...il.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@...aceresoli.net>
> > > >> Sent: Tuesday, March 15, 2022 4:55 AM
> > > >> To: Adam Ford <aford173@...il.com>; linux-clk@...r.kernel.org
> > > >> Cc: aford@...conembedded.com;
> cstevens@...conembedded.com;
> > > Fillion,
> > > >> Claude <Claude.Fillion@...inst.com>; Michael Turquette
> > > >> <mturquette@...libre.com>; Stephen Boyd <sboyd@...nel.org>;
> > > >> linux- kernel@...r.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@...inst.com>
> > > >>> Signed-off-by: Adam Ford <aford173@...il.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.
Powered by blists - more mailing lists