| Message ID | 20230119213903.899756-1-dennis@sparkcharge.io |
|---|---|
| Headers | show |
| Series | rtc: Set M41T82 & M41T83 xtal load capacitance from DT | expand |
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/20221219190915.3912384-1-dennis@sparkcharge.io/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 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.
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.
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 <alexandre.belloni@bootlin.com> 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.
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/20221219190915.3912384-1-dennis@sparkcharge.io/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(-)