Hi Adam, On 11/01/21 17:40, Adam Ford wrote: > On Sat, Jan 9, 2021 at 12:02 PM Luca Ceresoli <luca@xxxxxxxxxxxxxxxx> wrote: >> >> Hi Adam, >> >> On 09/01/21 04:00, Adam Ford wrote: >>> On Fri, Jan 8, 2021 at 4:49 PM Luca Ceresoli <luca@xxxxxxxxxxxxxxxx> wrote: >>>> >>>> Hi Adam, >>>> >>>> On 06/01/21 18:39, Adam Ford wrote: >>>>> 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. >> >>>>> +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. >>>> >>>> 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. >> >>>>> + >>>>> + /* >>>>> + * 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! > > 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. 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". -- Luca