| Message ID | 1439198906-31189-3-git-send-email-j-keerthy@ti.com |
|---|---|
| State | Superseded |
| Headers | show |
Hi, This seems better but: On 10/08/2015 at 14:58:22 +0530, Keerthy wrote : > diff --git a/arch/arm/boot/dts/am4372.dtsi b/arch/arm/boot/dts/am4372.dtsi > index d99b2ee..756819f 100644 > --- a/arch/arm/boot/dts/am4372.dtsi > +++ b/arch/arm/boot/dts/am4372.dtsi > @@ -336,6 +336,8 @@ > interrupts = <GIC_SPI 75 IRQ_TYPE_LEVEL_HIGH > GIC_SPI 76 IRQ_TYPE_LEVEL_HIGH>; > ti,hwmods = "rtc"; > + clocks = <&clk_32768_ck>, <&clk_32k_rtc>; > + clock-names = "int-clk", "ext-clk"; > status = "disabled"; > }; > This change has to be part of another patch. > @@ -698,6 +706,7 @@ static int __exit omap_rtc_remove(struct platform_device *pdev) > static int omap_rtc_suspend(struct device *dev) > { > struct omap_rtc *rtc = dev_get_drvdata(dev); > + u8 reg; > > rtc->interrupts_reg = rtc_read(rtc, OMAP_RTC_INTERRUPTS_REG); > > @@ -711,6 +720,18 @@ static int omap_rtc_suspend(struct device *dev) > enable_irq_wake(rtc->irq_alarm); > else > rtc_write(rtc, OMAP_RTC_INTERRUPTS_REG, 0); > + > + /* > + * If we have the luxury of external clock then > + * Switch to external clock so we can keep ticking > + * acorss suspend > + */ > + if (rtc->has_ext_clk) { > + reg = rtc_read(rtc, OMAP_RTC_OSC_REG); > + rtc_write(rtc, OMAP_RTC_OSC_REG, reg | > + OMAP_RTC_OSC_SEL_32KCLK_SRC); > + } > + You should probably prepare/enable the clock before switching to it. I know it doesn't matter right now but that may not be the case on other boards.
On Mon, 10 Aug 2015, Keerthy wrote: > Switch to external clock source during suspend and switch back > to internal source on resume. This helps rtc ticking across suspend. Doesn't this type of dynamic switching make it likely that ticks will be lost? If the external, optional source is present, isn't it best just to use the external source 100% of the time? - Paul
On Wednesday 12 August 2015 07:57 PM, Paul Walmsley wrote: > On Mon, 10 Aug 2015, Keerthy wrote: > >> Switch to external clock source during suspend and switch back >> to internal source on resume. This helps rtc ticking across suspend. > > Doesn't this type of dynamic switching make it likely that ticks will be > lost? > > If the external, optional source is present, isn't it best just to use the > external source 100% of the time? Paul, The intent here is to switch to a higher precision clock which is the internal clock when available. Alexandre, Is dynamic switching preferred over sticking to external clock always if present? Regards, Keerthy > > > - Paul >
Hi, On 13/08/2015 at 00:38:50 +0530, Keerthy wrote : > The intent here is to switch to a higher precision clock which is the > internal clock when available. > > Alexandre, > > Is dynamic switching preferred over sticking to external clock always if > present? > I'd say that I don't really care. I'd say the best would be to make a decision based on clock-accuracy but maybe that is an information you don't have yet. Anyway, this could be added at a later date.
Hi guys On Wed, 12 Aug 2015, Alexandre Belloni wrote: > On 13/08/2015 at 00:38:50 +0530, Keerthy wrote : > > The intent here is to switch to a higher precision clock which is the > > internal clock when available. > > > > Alexandre, > > > > Is dynamic switching preferred over sticking to external clock always if > > present? > > I'd say that I don't really care. I'd say the best would be to make a > decision based on clock-accuracy but maybe that is an information you > don't have yet. Anyway, this could be added at a later date. Either the clock mux logic is glitchless, in which case the RTC is likely to lose at least 31 microseconds per switch; or it's not glitchless, in which case it's unsafe to switch the RTC clock source while the clock isn't gated. Keerthy, before submitting this patch for merging, I'd suggest consulting your hardware folks to figure out which case it is. - Paul
On Friday 14 August 2015 01:47 AM, Paul Walmsley wrote: > Hi guys > > On Wed, 12 Aug 2015, Alexandre Belloni wrote: > >> On 13/08/2015 at 00:38:50 +0530, Keerthy wrote : >>> The intent here is to switch to a higher precision clock which is the >>> internal clock when available. >>> >>> Alexandre, >>> >>> Is dynamic switching preferred over sticking to external clock always if >>> present? >> >> I'd say that I don't really care. I'd say the best would be to make a >> decision based on clock-accuracy but maybe that is an information you >> don't have yet. Anyway, this could be added at a later date. > > Either the clock mux logic is glitchless, in which case the RTC is likely > to lose at least 31 microseconds per switch; or it's not glitchless, in > which case it's unsafe to switch the RTC clock source while the clock > isn't gated. Keerthy, before submitting this patch for merging, I'd > suggest consulting your hardware folks to figure out which case it is. Paul, Thanks. Yes so i am doing the static way first. Keeping external clock always if available. Switching dynamically can be done later with more clarifications. Thanks, Keerthy > > > - Paul >
On 13/08/2015 at 20:17:23 +0000, Paul Walmsley wrote : > > I'd say that I don't really care. I'd say the best would be to make a > > decision based on clock-accuracy but maybe that is an information you > > don't have yet. Anyway, this could be added at a later date. > > Either the clock mux logic is glitchless, in which case the RTC is likely > to lose at least 31 microseconds per switch; or it's not glitchless, in > which case it's unsafe to switch the RTC clock source while the clock > isn't gated. Keerthy, before submitting this patch for merging, I'd > suggest consulting your hardware folks to figure out which case it is. > Don't take me wrong, your point is perfectly valid. I was just saying that I didn't care what was done in the driver. Obviously, this has to match what is best from a hardware point of view. But since we agreed on the DT bindings, I'd say that we can still adjust the driver later.
diff --git a/arch/arm/boot/dts/am4372.dtsi b/arch/arm/boot/dts/am4372.dtsi index d99b2ee..756819f 100644 --- a/arch/arm/boot/dts/am4372.dtsi +++ b/arch/arm/boot/dts/am4372.dtsi @@ -336,6 +336,8 @@ interrupts = <GIC_SPI 75 IRQ_TYPE_LEVEL_HIGH GIC_SPI 76 IRQ_TYPE_LEVEL_HIGH>; ti,hwmods = "rtc"; + clocks = <&clk_32768_ck>, <&clk_32k_rtc>; + clock-names = "int-clk", "ext-clk"; status = "disabled"; }; diff --git a/drivers/rtc/rtc-omap.c b/drivers/rtc/rtc-omap.c index 8b6355f..cfc8558 100644 --- a/drivers/rtc/rtc-omap.c +++ b/drivers/rtc/rtc-omap.c @@ -25,6 +25,7 @@ #include <linux/of_device.h> #include <linux/pm_runtime.h> #include <linux/io.h> +#include <linux/clk.h> /* * The OMAP RTC is a year/month/day/hours/minutes/seconds BCD clock @@ -107,6 +108,7 @@ /* OMAP_RTC_OSC_REG bit fields: */ #define OMAP_RTC_OSC_32KCLK_EN BIT(6) +#define OMAP_RTC_OSC_SEL_32KCLK_SRC BIT(3) /* OMAP_RTC_IRQWAKEEN bit fields: */ #define OMAP_RTC_IRQWAKEEN_ALARM_WAKEEN BIT(1) @@ -136,6 +138,7 @@ struct omap_rtc { int irq_timer; u8 interrupts_reg; bool is_pmic_controller; + bool has_ext_clk; const struct omap_rtc_device_type *type; }; @@ -525,6 +528,7 @@ static int omap_rtc_probe(struct platform_device *pdev) { struct omap_rtc *rtc; struct resource *res; + struct clk *ext_clk; u8 reg, mask, new_ctrl; const struct platform_device_id *id_entry; const struct of_device_id *of_id; @@ -553,6 +557,10 @@ static int omap_rtc_probe(struct platform_device *pdev) if (rtc->irq_alarm <= 0) return -ENOENT; + ext_clk = devm_clk_get(&pdev->dev, "ext-clk"); + if (!IS_ERR(ext_clk)) + rtc->has_ext_clk = true; + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); rtc->base = devm_ioremap_resource(&pdev->dev, res); if (IS_ERR(rtc->base)) @@ -698,6 +706,7 @@ static int __exit omap_rtc_remove(struct platform_device *pdev) static int omap_rtc_suspend(struct device *dev) { struct omap_rtc *rtc = dev_get_drvdata(dev); + u8 reg; rtc->interrupts_reg = rtc_read(rtc, OMAP_RTC_INTERRUPTS_REG); @@ -711,6 +720,18 @@ static int omap_rtc_suspend(struct device *dev) enable_irq_wake(rtc->irq_alarm); else rtc_write(rtc, OMAP_RTC_INTERRUPTS_REG, 0); + + /* + * If we have the luxury of external clock then + * Switch to external clock so we can keep ticking + * acorss suspend + */ + if (rtc->has_ext_clk) { + reg = rtc_read(rtc, OMAP_RTC_OSC_REG); + rtc_write(rtc, OMAP_RTC_OSC_REG, reg | + OMAP_RTC_OSC_SEL_32KCLK_SRC); + } + rtc->type->lock(rtc); /* Disable the clock/module */ @@ -722,6 +743,7 @@ static int omap_rtc_suspend(struct device *dev) static int omap_rtc_resume(struct device *dev) { struct omap_rtc *rtc = dev_get_drvdata(dev); + u8 reg; /* Enable the clock/module so that we can access the registers */ pm_runtime_get_sync(dev); @@ -731,6 +753,15 @@ static int omap_rtc_resume(struct device *dev) disable_irq_wake(rtc->irq_alarm); else rtc_write(rtc, OMAP_RTC_INTERRUPTS_REG, rtc->interrupts_reg); + /* + * Switch back to internal source + */ + if (rtc->has_ext_clk) { + reg = rtc_read(rtc, OMAP_RTC_OSC_REG); + reg &= ~OMAP_RTC_OSC_SEL_32KCLK_SRC; + rtc_write(rtc, OMAP_RTC_OSC_REG, reg); + } + rtc->type->lock(rtc); return 0;
Switch to external clock source during suspend and switch back to internal source on resume. This helps rtc ticking across suspend. Signed-off-by: Keerthy <j-keerthy@ti.com> --- arch/arm/boot/dts/am4372.dtsi | 2 ++ drivers/rtc/rtc-omap.c | 31 +++++++++++++++++++++++++++++++ 2 files changed, 33 insertions(+)