From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.3 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE, SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 51F80C433E9 for ; Wed, 13 Jan 2021 14:13:09 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 0E99B23432 for ; Wed, 13 Jan 2021 14:13:09 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726952AbhAMOM7 (ORCPT ); Wed, 13 Jan 2021 09:12:59 -0500 Received: from hostingweb31-40.netsons.net ([89.40.174.40]:33371 "EHLO hostingweb31-40.netsons.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725747AbhAMOM6 (ORCPT ); Wed, 13 Jan 2021 09:12:58 -0500 Received: from [185.56.157.72] (port=39402 helo=[192.168.101.73]) by hostingweb31.netsons.net with esmtpsa (TLS1.2) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.93) (envelope-from ) id 1kzgse-00DmxP-Vt; Wed, 13 Jan 2021 15:12:13 +0100 Subject: Re: [RFC 2/2] clk: vc5: Add support for optional load capacitance To: Adam Ford Cc: linux-clk , Adam Ford-BE , Michael Turquette , Stephen Boyd , Rob Herring , devicetree , Linux Kernel Mailing List References: <20210106173900.388758-1-aford173@gmail.com> <20210106173900.388758-2-aford173@gmail.com> From: Luca Ceresoli Message-ID: <10320483-c996-e33b-00cf-0629d899b82c@lucaceresoli.net> Date: Wed, 13 Jan 2021 15:12:12 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit X-AntiAbuse: This header was added to track abuse, please include it with any abuse report X-AntiAbuse: Primary Hostname - hostingweb31.netsons.net X-AntiAbuse: Original Domain - vger.kernel.org X-AntiAbuse: Originator/Caller UID/GID - [47 12] / [47 12] X-AntiAbuse: Sender Address Domain - lucaceresoli.net X-Get-Message-Sender-Via: hostingweb31.netsons.net: authenticated_id: luca+lucaceresoli.net/only user confirmed/virtual account not confirmed X-Authenticated-Sender: hostingweb31.netsons.net: luca@lucaceresoli.net X-Source: X-Source-Args: X-Source-Dir: Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Adam, On 12/01/21 18:00, Adam Ford wrote: > On Tue, Jan 12, 2021 at 10:45 AM Luca Ceresoli wrote: >> >> Hi Adam, >> >> On 11/01/21 17:40, Adam Ford wrote: >>> On Sat, Jan 9, 2021 at 12:02 PM Luca Ceresoli wrote: >>>> >>>> Hi Adam, >>>> >>>> On 09/01/21 04:00, Adam Ford wrote: >>>>> On Fri, Jan 8, 2021 at 4:49 PM Luca Ceresoli 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