Message-ID: <10320483-c996-e33b-00cf-0629d899b82c@lucaceresoli.net>
Date:   Wed, 13 Jan 2021 15:12:12 +0100
From:   Luca Ceresoli <luca@...aceresoli.net>
To:     Adam Ford <aford173@...il.com>
Cc:     linux-clk <linux-clk@...r.kernel.org>,
        Adam Ford-BE <aford@...conembedded.com>,
        Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        devicetree <devicetree@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [RFC 2/2] clk: vc5: Add support for optional load capacitance

Hi Adam,

On 12/01/21 18:00, Adam Ford wrote:
> On Tue, Jan 12, 2021 at 10:45 AM Luca Ceresoli <luca@...aceresoli.net> wrote:
>>
>> Hi Adam,
>>
>> On 11/01/21 17:40, Adam Ford wrote:
>>> On Sat, Jan 9, 2021 at 12:02 PM Luca Ceresoli <luca@...aceresoli.net> wrote:
>>>>
>>>> Hi Adam,
>>>>
>>>> On 09/01/21 04:00, Adam Ford wrote:
>>>>> On Fri, Jan 8, 2021 at 4:49 PM Luca Ceresoli <luca@...aceresoli.net> 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.
> 
> 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

I think we are saying the same thing in two different ways. According to
table 28 of the "Regiter description and programming guide", we have:

  Ci = 9 + (0.43 * N) pF

where N can be any integer between 0 and 32 (inclusive).

Your proposed implementation is:

reg_0x12 = (N << 3) | 0x1;

i.e. using only XTAL bits [5:1], not [0], so we can have any value in
the range [0..31], but not 32. This is OK for me.

Should we need value 32 at some point we can simply augment it as:

if (N == 32)
  reg_0x12 = (b111111 << 2) | 0x1;
else
  reg_0x12 = (N << 3) | 0x1; // your current proposal

Among all register XTAL values having XTAL[0] == 1, only b111111 is
interesting. All other values have an equivalent with XTAL[0] == 0.

Correct?

-- 
Luca

Powered by blists - more mailing lists