Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 20 Jan 2023 00:52:01 +0100
From:   Alexandre Belloni <alexandre.belloni@bootlin.com>
To:     Dennis Lambe <dennis@sparkcharge.io>
Cc:     Alessandro Zummo <a.zummo@towertech.it>,
        Atsushi Nemoto <atsushi.nemoto@sord.co.jp>,
        Gary Bisson <gary.bisson@boundarydevices.com>,
        Javier Martinez Canillas <javier@osg.samsung.com>,
        Troy Kisky <troy.kisky@boundarydevices.com>,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-rtc@vger.kernel.org
Subject: Re: [PATCH v3 0/3] rtc: Set M41T82 & M41T83 xtal load capacitance
 from DT
Message-ID: <Y8nXoZjco8uIrtFJ@mail.local>
References: <20230119213903.899756-1-dennis@sparkcharge.io>
 <Y8nBloQfBPK3t5ce@mail.local>
 <CAKYiA1Dr0TAmsqDf1wMeeysN4N8K+KJL6onCgQL98LVV5L7Vmg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAKYiA1Dr0TAmsqDf1wMeeysN4N8K+KJL6onCgQL98LVV5L7Vmg@mail.gmail.com>
Precedence: bulk

On 19/01/2023 18:27:44-0500, Dennis Lambe wrote:
> On Thu, Jan 19, 2023 at 5:18 PM Alexandre Belloni
> <alexandre.belloni@bootlin.com> wrote:
> 
> > I need to find time to think about it because while setting the analog
> > trimming statically from the device tree solves your immediate problem,
> > it will also remove the possibility to handle it from userspace later
> > on. I would really prefer this uses the offset interface or a better
> > interface that unfortunately doesn't exist yet.
> 
> Thanks for letting me know what you're thinking about this. I think I
> see what you're getting at.
> 
> However, I think this is more complex than either of us had
> considered. The M41T82 has two different calibration capabilities:
> 
> 1. Digital calibration. This looks to me like it behaves similarly to
> the digital calibration feature of the M41T00, which ds1307.c exposes
> through the offset interface. The M41T8x driver doesn't currently
> expose the digital calibration register at all, but if it did I would
> agree that the offset interface looks appropriate.
> 
> 2. Analog calibration -- that's what the datasheet calls it, but the
> range on it is very big -- 3.5 pF all the way up to 17.4 pF -- and
> their reference design uses it as the only xtal load capacitance in
> the circuit. Most of the values you could set for this would be wildly
> inappropriate for any given design's choice of xtal oscillator.
> 
> Between these, I don't know if you'd want to expose just one, the
> other, or some synthesis of both via the offset interface or some new
> interface.
> 
> I'd make the case that the xtal's required load capacitance is a
> hardware requirement that's appropriate to configure via the Device
> Tree. Even if you did want to allow some amount of runtime fine-tuning
> of this register, you'd still want to document a rational starting
> value chosen based on the hardware.
> 
> I agree with you, though, that if a runtime fine-tuning feature were
> added, we'd have to find a way to choose whether to initialize the
> register on boot or not, so that we didn't overwrite the fine-tuning.
> 
> Just to demonstrate something that could work, and would be
> backward-compatible with this patchset, here's a hypothetical design:
> * dt-bindings: add quartz-load-femtofarad-tuning-min and
> quartz-load-femtofarad-tuning-max
> * Limit run-time tuning adjustments to be within that range
> * Only overwrite the analog calibration register on start-up if its
> value is outside that range
> 
> After thinking through all this, I'd still advocate for merging this
> patchset in some form and leaving integration with runtime APIs as a
> potential future enhancement. I look forward to hearing your thoughts
> about it.

I specifically referred to analog trimming in my reply because I knew it
could do digital trimming and I also said that we will probably need a
new interface for this.
The main existing issue is that the register changes the capacitance but
the datasheet doesn't give the actual effect in ppm which doesn't
integrate well with the existing userspace tooling.

I advocate against merging as is without more thought because changing
anything later will mean breaking the DT ABI and this is not allowed.


-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com