On Tue, Jan 12, 2021 at 10:45 AM Luca Ceresoli [off-list ref] wrote:

Hi Adam,

On 11/01/21 17:40, Adam Ford wrote:

quoted

On Sat, Jan 9, 2021 at 12:02 PM Luca Ceresoli [off-list ref] wrote:

quoted

Hi Adam,

On 09/01/21 04:00, Adam Ford wrote:

quoted

On Fri, Jan 8, 2021 at 4:49 PM Luca Ceresoli [off-list ref] wrote:

quoted

Hi Adam,

On 06/01/21 18:39, Adam Ford wrote:

quoted

There are two registers which can set the load capacitance for
XTAL1 and XTAL2. These are optional registers when using an
external crystal.  Parse the device tree and set the
corresponding registers accordingly.

No need to repeat the first 2 sentences, they are already in patch 1.

The reason I did that was because if someone does a git log on the
individual file, they'd see the comment.  While it's redundant not, it
might not be as obvious in the future when looking back.   Not
everyone reviews the history of the binding, but the source files' git
logs usually have some value.   However, if you want me to drop it or
rephrase it, I can do that.

Makes sense, I had never considered that before.

quoted

quoted

quoted

+static int vc5_map_cap_value(u32 femtofarads)
+{
+     int mapped_value;
+
+     /* The datasheet explicitly states 9000 - 25000 */
+     if ((femtofarads < 9000) || (femtofarads > 25000))
+             return -EINVAL;
+
+     /* The lowest target we can hit is 9430, so exit if it's less */
+     if (femtofarads < 9430)
+             return 0;
+
+     /*
+      * According to VersaClock 6E Programming Guide, there are 6
+      * bits which translate to 64 entries in XTAL registers 12 and
+      * 13. Because bits 0 and 1 increase the capacitance the
+      * same, some of the values can be repeated.  Plugging this
+      * into a spreadsheet and generating a trendline, the output
+      * equation becomes x = (y-9098.29) / 216.44, where 'y' is
+      * the desired capacitance in femtofarads, and x is the value
+      * of XTAL[5:0].
+      * To help with rounding, do fixed point math
+      */
+     femtofarads *= 100;
+     mapped_value = (femtofarads - 909829) / 21644;

Thanks for the extensive comment, but I am confused. Not by your code
which is very clean and readable, but by the chip documentation
(disclaimer: I haven't read it in full depth).

I was confused too since the datasheet and programmers manual differ a bit.

quoted

The 5P49V6965 datasheet at page 17 clearly states capacitance can be
increased in 0.5 pF steps. The "VersaClock 6E Family Register
Descriptions and Programming Guide" at page 18 shows a table that allows
0.43 pF. Can you clarify how the thing works?

I used the Versaclock 6E doc which is based on the following:

BIT 5 - Add 6.92pF
BIT 4 - Add 3.46pF
BIT 3 - Add 1.73pF
BIT 2 - Add 0.86pF
Bit 1 - Add 0.43pF
Bit 0 - Add 0.43pF

Because the Datasheet starts at 9pF, the math I used, assumes these
numbers are added to 9pF.
Because the datasheet shows the increments are in .5pF increments, the
430nF seems close.  The datasheet shows 9pF - 25pF and based on the
programmer table, we could get close to 25pF by enabling all bits and
adding 9pF, however the math doesn't quite hit 25pF.

For what it's worth I needed around 11.5pF, and with this patch, the
hardware engineer said our ppm went from around 70 ppm to around 4ppm.

Did he measure what happens if you set the register according to the 0.5
pF interpretation? Does it improve? I understand the difference is
probably olwer than the noise, but who knows.

quoted

quoted

quoted

+
+     /*
+      * The datasheet states, the maximum capacitance is 25000,
+      * but the programmer guide shows a max value is 22832,
+      * so values higher values could overflow, so cap it.
+      */

The 22832 limit is if you assume 0.43 pF steps. Assuming 0.5 pF steps
leads to 25000. Now I am more confused than before.

I agree.  It would be nice to get some clarification from Renesas.

Definitely. Do you have access to some support from them?

Luca,

We reached out to  Renesas with the following questions:

1)
I'm seeing a discrepancy between the datasheet and programming guide
we have for the Versaclock 6e in regard to the crystal load
programming registers.  The datasheet for the 5P49V6965A000NLGI
indicates a 9pF minimum with 0.5pF steps, while the programming guide
says that the lower two register bits both add 0.43pF, which would
make the equation:

Ci = 9pF + 0.43pF * XTAL[5:1] instead of
Ci = 9pF + 0.5pF * XTAL[5:0] as is published in the datasheet.

2)  The programming guide shows that the default setting is 01b, but
the note says it's reserved, use D1 D0 = 00.  Can you confirm that we
should set switch mode to 00 instead of the default 01?

And we got the following answers:

1)
     The first one with 0.43pF steps is the correct one. Ci = 9pF +
0.43pF * XTAL[5:1]
     0.5pF steps was the design target.  When measuring actual
silicon, we found 0.43pF steps.

     There are 6 bits reserved for the CL setting but bits 0 and 1
have the same 0.43pF step.  So it is actually 5 bits with an extra LSB
of 0.43pF.

2)
      Please use D1 D0 = 01.   The “00” is a typo…..

Great thing you got all those info from Renesas!

quoted

Based on the above response I think we should always assume XTAL bit 0
is 0, and only use XTAL[5:1] which should make the math go easier,
because the desired value in femtofarads would just be offset by 9000
and divided by 430 and that value would be shifted 3 places instead fo
two, and the  fixed-point math calculation can go away.

In addition to that, I would also need to make sure that D0 is set to
1, so instead of just writing the shifted XTAL value, I would also
have to do a logic OR with 1 to set the low bit.

I talked with the hardware guys from work who also suggested that we
always write the same value to X1 and X2, so I can remove the X1 and
X2 references from the bindings.

Does that work for you?

Yes.

We are only losing the ability to set 9 + (0.43 * 32) pF using all bits.

We'd be doing this with XTAL[5:1] which mathematically makes more sense.

I'm OK with that. Should it be needed in the future we can just add it
as a special case, maybe just add a comment saying that, like "XTAL[5:0]
= b111111 not supported".

XTAL[0] won't be supported at all by my updated algorithm, not just b111111

adam

--
Luca