| 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
| ||
|
Message-ID: <CAHCN7xKZDTdKyMgCUOmKXrceC8Qhk8ES6xn5GFag-8Vt=NgpAA@mail.gmail.com>
Date: Sat, 19 Mar 2022 16:24:05 -0500
From: Adam Ford <aford173@...il.com>
To: "Fillion, Claude" <Claude.Fillion@...inst.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
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.
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.
Powered by blists - more mailing lists