Other than adding a sign-off to one of the changelogs, this is a RESEND.
Alexandre Belloni, what do you need for this before you'd want to apply
it? In case it's additional reviewers, I have CC'd some more
potentially-interested parties this time and updated Atsushi Nemoto's
email address to one that's hopefully more current.
I think the original author listed in the header for this driver,
Alexander Bigga, is inaccurate. It looks to me like his name got copied
over by Atsushi Nemoto when he created m41t82.c by deriving it from a
similar driver. At any rate, Alexander Bigga's listed email address
bounces, I didn't find a newer one for him, and he doesn't show up in
the kernel commit log after 2007. I don't think he can be considered the
maintainer for this driver anymore if he ever was.
Changes in v3:
* dt-bindings: added Krzysztof Kozlowski sign-off to changelog
Changes in v2:
* dt-bindings: remove accidental wakeup-sources line
suggested by Krzysztof Kozlowski
* spelling fixes in changelogs
The m41t82 and m41t83 have an adjustable internal capacitance that
defaults to 25 pF per xtal pin. This patch series adds the ability to
configure it via the devicetree.
Patch 1 just changes `#ifdef CONFIG_OF` to `if (IS_ENABLED(CONFIG_OF))`
in m41t80_probe() so that I don't need to use __maybe_unused on my new
functions and variables.
Patch 2 is the dt-bindings.
Patch 3 is the actual feature implementation.
The desired capacitance comes from the quartz-load-femtofarads property,
following the example of two other RTC ICs that have adjustable internal
load capacitance, the NXP pcf85063 and pcf8523. The m41t82 and m41t83
support much finer-grained control over the capacitance than those
chips, and ST calls the feature "analog calibration", but it looks to me
like it's essentially the same kind of thing.
My use case for this is:
ST specifies not to add any additional external load capacitance[1], but
the MikroElektronika RTC 9 Click board[2] has a 22 pF cap on each xtal
pin[3]. The resulting combined capacitance appears to be outside of the
operating range of the xtal, because when power is removed from the
boards I'm testing with, the RTC reports an Oscillator-Fail flag on the
next power on.
I found I could work around the problem by reducing the internal load
capacitance as low as it will go.
References:
[1] https://www.st.com/resource/en/application_note/an3060-applications-guide-for-serial-realtime-clocks-rtcs-stmicroelectronics.pdf
[2] https://www.mikroe.com/rtc-9-click
[3] https://download.mikroe.com/documents/add-on-boards/click/rtc-9/rtc-9-click-schematic-v100.pdf
Previous versions:
v1: https://lore.kernel.org/linux-rtc/[email protected]/T/
Dennis Lambe Jr (3):
rtc: m41t80: probe: use IS_ENABLED for CONFIG_OF
dt-bindings: m41t80: add xtal load capacitance
rtc: m41t80: set xtal load capacitance from DT
.../devicetree/bindings/rtc/st,m41t80.yaml | 16 ++++
drivers/rtc/rtc-m41t80.c | 84 +++++++++++++++++--
2 files changed, 92 insertions(+), 8 deletions(-)
--
2.25.1
On 19/01/2023 21:39:00+0000, Dennis Lambe Jr wrote:
> Other than adding a sign-off to one of the changelogs, this is a RESEND.
>
> Alexandre Belloni, what do you need for this before you'd want to apply
> it? In case it's additional reviewers, I have CC'd some more
> potentially-interested parties this time and updated Atsushi Nemoto's
> email address to one that's hopefully more current.
>
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.
> I think the original author listed in the header for this driver,
> Alexander Bigga, is inaccurate. It looks to me like his name got copied
> over by Atsushi Nemoto when he created m41t82.c by deriving it from a
> similar driver. At any rate, Alexander Bigga's listed email address
> bounces, I didn't find a newer one for him, and he doesn't show up in
> the kernel commit log after 2007. I don't think he can be considered the
> maintainer for this driver anymore if he ever was.
>
> Changes in v3:
> * dt-bindings: added Krzysztof Kozlowski sign-off to changelog
>
> Changes in v2:
> * dt-bindings: remove accidental wakeup-sources line
> suggested by Krzysztof Kozlowski
> * spelling fixes in changelogs
>
> The m41t82 and m41t83 have an adjustable internal capacitance that
> defaults to 25 pF per xtal pin. This patch series adds the ability to
> configure it via the devicetree.
>
> Patch 1 just changes `#ifdef CONFIG_OF` to `if (IS_ENABLED(CONFIG_OF))`
> in m41t80_probe() so that I don't need to use __maybe_unused on my new
> functions and variables.
>
> Patch 2 is the dt-bindings.
>
> Patch 3 is the actual feature implementation.
>
> The desired capacitance comes from the quartz-load-femtofarads property,
> following the example of two other RTC ICs that have adjustable internal
> load capacitance, the NXP pcf85063 and pcf8523. The m41t82 and m41t83
> support much finer-grained control over the capacitance than those
> chips, and ST calls the feature "analog calibration", but it looks to me
> like it's essentially the same kind of thing.
>
> My use case for this is:
>
> ST specifies not to add any additional external load capacitance[1], but
> the MikroElektronika RTC 9 Click board[2] has a 22 pF cap on each xtal
> pin[3]. The resulting combined capacitance appears to be outside of the
> operating range of the xtal, because when power is removed from the
> boards I'm testing with, the RTC reports an Oscillator-Fail flag on the
> next power on.
>
> I found I could work around the problem by reducing the internal load
> capacitance as low as it will go.
>
> References:
> [1] https://www.st.com/resource/en/application_note/an3060-applications-guide-for-serial-realtime-clocks-rtcs-stmicroelectronics.pdf
> [2] https://www.mikroe.com/rtc-9-click
> [3] https://download.mikroe.com/documents/add-on-boards/click/rtc-9/rtc-9-click-schematic-v100.pdf
>
> Previous versions:
> v1: https://lore.kernel.org/linux-rtc/[email protected]/T/
>
> Dennis Lambe Jr (3):
> rtc: m41t80: probe: use IS_ENABLED for CONFIG_OF
> dt-bindings: m41t80: add xtal load capacitance
> rtc: m41t80: set xtal load capacitance from DT
>
> .../devicetree/bindings/rtc/st,m41t80.yaml | 16 ++++
> drivers/rtc/rtc-m41t80.c | 84 +++++++++++++++++--
> 2 files changed, 92 insertions(+), 8 deletions(-)
>
> --
> 2.25.1
>
--
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
On Thu, Jan 19, 2023 at 5:18 PM Alexandre Belloni
<[email protected]> 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.
--
Dennis Lambe (He/Him)
Lead Firmware Engineer
sparkcharge.io
On 19/01/2023 18:27:44-0500, Dennis Lambe wrote:
> On Thu, Jan 19, 2023 at 5:18 PM Alexandre Belloni
> <[email protected]> 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
On 2023/01/20 6:39, Dennis Lambe Jr wrote:
> In case it's additional reviewers, I have CC'd some more
> potentially-interested parties this time and updated Atsushi Nemoto's
> email address to one that's hopefully more current.
My addresses are both still active (dispite the slow response).
Ether is fine.
> I think the original author listed in the header for this driver,
> Alexander Bigga, is inaccurate. It looks to me like his name got copied
> over by Atsushi Nemoto when he created m41t82.c by deriving it from a
> similar driver. At any rate, Alexander Bigga's listed email address
> bounces, I didn't find a newer one for him, and he doesn't show up in
> the kernel commit log after 2007. I don't think he can be considered the
> maintainer for this driver anymore if he ever was.
In 2006-2007, I wrote rtc-m41t80 driver based on Alexander Bigga's
rtc-m41txx driver with his cooperation and review, then upstreamed it.
About your patchset, I have no idea, since I have not touched this device
and driver for a long time (>10 years) ...
---
Atsushi Nemoto
On Thu, Jan 19, 2023 at 6:52 PM Alexandre Belloni
<[email protected]> wrote:
> On 19/01/2023 18:27:44-0500, Dennis Lambe wrote:
> > 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.
I think I should start with an apology, almost everything I wrote
yesterday was in error one way or another. I was conflating this RTC
with I'm also working with. I see now that the datasheet for this one
clearly states that it's only to be used with 12.5 pF crystals, and
that the analog adjustments are meant exclusively for calibration. I
get what you mean about wanting it to be a new runtime interface and
it not making sense to put in the DeviceTree.
I also see what you mean about the datasheet not providing a good
capacitance vs. ppm table. The graph is approximate at best, and ST's
appnote recommends an iterative tuning procedure rather than just
assuming a certain value gives exactly a certain ppm adjustment. I see
why you would want to avoid using 'offset' for this.
I'll hold off on submitting any more patches for this until you've had
a chance to think about how you would want a new interface to work.
Would it be useful to you if I start working up a patchset that makes
a new rtc sysfs attribute and wires the m41t80 driver into it so that
I'm ready to adapt it to whatever naming, scaling, semantics,
interpretation, etc. you decide is right for it?
> I advocate against merging as is without more thought because changing
> anything later will mean breaking the DT ABI and this is not allowed.
Me too, thanks for taking the time to get through to me about it.
--
Dennis Lambe (He/Him)
Lead Firmware Engineer
sparkcharge.io