| Message ID | 20241206070955.1503412-3-ciprianmarian.costea@oss.nxp.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | add NXP RTC driver support for S32G2/S32G3 SoCs | expand |
On Fri, Dec 6, 2024, at 08:09, Ciprian Costea wrote: > From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com> > > Add a RTC driver for NXP S32G2/S32G3 SoCs. > > RTC tracks clock time during system suspend. It can be a wakeup source > for the S32G2/S32G3 SoC based boards. > > The RTC module from S32G2/S32G3 is not battery-powered and it is not kept > alive during system reset. > > Co-developed-by: Bogdan Hamciuc <bogdan.hamciuc@nxp.com> > Signed-off-by: Bogdan Hamciuc <bogdan.hamciuc@nxp.com> > Co-developed-by: Ghennadi Procopciuc <Ghennadi.Procopciuc@nxp.com> > Signed-off-by: Ghennadi Procopciuc <Ghennadi.Procopciuc@nxp.com> > Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com> I read through the driver and this looks all good to me, but there are two fairly minor things I noticed: > + u64 rtc_hz; I see the clock rate is a 64-bit value, which is clearly what comes from the clk interface in the kernel > +static u64 cycles_to_sec(u64 hz, u64 cycles) > +{ > + return div_u64(cycles, hz); > +} and you divide by the clk rate to convert the register value to seconds (as expected) > + u32 delta_cnt; > + > + if (!seconds || seconds > cycles_to_sec(priv->rtc_hz, RTCCNT_MAX_VAL)) > + return -EINVAL; However, the range of the register value is only 32 bits, which means there is no need to ever divide it by a 64-bit number, and with the 32kHz clock in the binding example, you only have about 37 hours worth of range here. It would appear that this makes the rtc unsuitable for storing absolute time across reboots, and only serve during runtime, which is a limitation you should probably document. > +static s64 s32g_rtc_get_time_or_alrm(struct rtc_priv *priv, > + u32 offset) > +{ > + u32 counter; > + > + counter = ioread32(priv->rtc_base + offset); > + > + if (counter < priv->base.cycles) > + return -EINVAL; If 'counter' wraps every 37 hours, this will inevitably fail, right? E.g. if priv->base.cycles was already at a large 32-bit number, even reading it shortly later will produce a small value after the wraparound. Using something like time_before() should address this, but I think you may need a custom version that works on 32-bit numbers. > +static int s32g_rtc_resume(struct device *dev) > +{ > + struct rtc_priv *priv = dev_get_drvdata(dev); > + int ret; > + > + if (!device_may_wakeup(dev)) > + return 0; > + > + /* Disable wake-up interrupts */ > + s32g_enable_api_irq(dev, 0); > + > + ret = rtc_clk_src_setup(priv); > + if (ret) > + return ret; > + > + /* > + * Now RTCCNT has just been reset, and is out of sync with priv->base; > + * reapply the saved time settings. > + */ > + return s32g_rtc_set_time(dev, &priv->base.tm); > +} This also fails if the system has been suspended for more than 37 hours, right? One more minor comment: you are using ioread32()/iowrite32() to access the MMIO registers, which is a bit unusual. I would suggest changing those to the more common readl()/writel() that do the exact same thing on arm64. Arnd
On 12/6/2024 10:04 AM, Arnd Bergmann wrote: > [You don't often get email from arnd@arndb.de. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ] > > On Fri, Dec 6, 2024, at 08:09, Ciprian Costea wrote: >> From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com> >> >> Add a RTC driver for NXP S32G2/S32G3 SoCs. >> >> RTC tracks clock time during system suspend. It can be a wakeup source >> for the S32G2/S32G3 SoC based boards. >> >> The RTC module from S32G2/S32G3 is not battery-powered and it is not kept >> alive during system reset. >> >> Co-developed-by: Bogdan Hamciuc <bogdan.hamciuc@nxp.com> >> Signed-off-by: Bogdan Hamciuc <bogdan.hamciuc@nxp.com> >> Co-developed-by: Ghennadi Procopciuc <Ghennadi.Procopciuc@nxp.com> >> Signed-off-by: Ghennadi Procopciuc <Ghennadi.Procopciuc@nxp.com> >> Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com> Hello Arnd, Thanks you for your review on this patchset. > > I read through the driver and this looks all good to me, but there > are two fairly minor things I noticed: > >> + u64 rtc_hz; > > I see the clock rate is a 64-bit value, which is clearly what > comes from the clk interface in the kernel > >> +static u64 cycles_to_sec(u64 hz, u64 cycles) >> +{ >> + return div_u64(cycles, hz); >> +} > > and you divide by the clk rate to convert the register value > to seconds (as expected) > >> + u32 delta_cnt; >> + >> + if (!seconds || seconds > cycles_to_sec(priv->rtc_hz, RTCCNT_MAX_VAL)) >> + return -EINVAL; > > However, the range of the register value is only 32 bits, > which means there is no need to ever divide it by a 64-bit > number, and with the 32kHz clock in the binding example, > you only have about 37 hours worth of range here. > I am not sure what is the suggestion here. To cast 'cycles' variable to 32-bit ? If yes, indeed 'div_u64' converts 'cycles' (the divisor) to 32-bit so I agree it should be u32 instead of u64. If not, I would prefer to keep using a 64-by-32 division and avoid casting 'hz' to 32-bit. > It would appear that this makes the rtc unsuitable for > storing absolute time across reboots, and only serve during > runtime, which is a limitation you should probably document. > Actually there is the option to use DIV512 and/or DIV32 hardware divisors for the RTC clock. The driver uses a DIV512 divisor by default in order to achieve higher RTC count ranges (by achieving a smaller RTC freq). Therefore, the 37 hours become 37 * 512 => ~ 2 years. However, the rtc limitation of not being persistent during reboot remains, due to hardware RTC module registers present of NXP S32G2/S32G3 SoCs being reset during system reboot. On the other hand, during system suspend, the RTC module will keep counting if a clock source is available. Currently, this limittion is documented as follows: "RTC tracks clock time during system suspend. It can be a wakeup source for the S32G2/S32G3 SoC based boards. The RTC module from S32G2/S32G3 is not battery-powered and it is not kept alive during system reset." >> +static s64 s32g_rtc_get_time_or_alrm(struct rtc_priv *priv, >> + u32 offset) >> +{ >> + u32 counter; >> + >> + counter = ioread32(priv->rtc_base + offset); >> + >> + if (counter < priv->base.cycles) >> + return -EINVAL; > > If 'counter' wraps every 37 hours, this will inevitably fail, > right? E.g. if priv->base.cycles was already at a large > 32-bit number, even reading it shortly later will produce > a small value after the wraparound. > > Using something like time_before() should address this, > but I think you may need a custom version that works on > 32-bit numbers. > This is correct. Would the following change be acceptable ? - if (counter < priv->base.cycles) - return -EINVAL; - - counter -= priv->base.cycles; + if (counter < priv->base.cycles) { + /* A rollover on RTCCTN has occurred */ + counter += RTCCNT_MAX_VAL - priv->base.cycles; + priv->base.cycles = 0; + } else { + counter -= priv->base.cycles; + } >> +static int s32g_rtc_resume(struct device *dev) >> +{ >> + struct rtc_priv *priv = dev_get_drvdata(dev); >> + int ret; >> + >> + if (!device_may_wakeup(dev)) >> + return 0; >> + >> + /* Disable wake-up interrupts */ >> + s32g_enable_api_irq(dev, 0); >> + >> + ret = rtc_clk_src_setup(priv); >> + if (ret) >> + return ret; >> + >> + /* >> + * Now RTCCNT has just been reset, and is out of sync with priv->base; >> + * reapply the saved time settings. >> + */ >> + return s32g_rtc_set_time(dev, &priv->base.tm); >> +} > > This also fails if the system has been suspended for more than > 37 hours, right? Actually, the system would not go into suspend (returning with error) if the alarm setting passes the 32-bit / clk_freq range. The check is added in 'sec_to_rtcval' which is called from the suspend routine. > > One more minor comment: you are using ioread32()/iowrite32() > to access the MMIO registers, which is a bit unusual. I would > suggest changing those to the more common readl()/writel() > that do the exact same thing on arm64. > > Arnd Makes sense. I will change to readl/writel in V7. Best Regards, Ciprian
On Fri, Dec 6, 2024, at 13:05, Ciprian Marian Costea wrote: > On 12/6/2024 10:04 AM, Arnd Bergmann wrote: >> >> However, the range of the register value is only 32 bits, >> which means there is no need to ever divide it by a 64-bit >> number, and with the 32kHz clock in the binding example, >> you only have about 37 hours worth of range here. >> > > I am not sure what is the suggestion here. To cast 'cycles' variable to > 32-bit ? > If yes, indeed 'div_u64' converts 'cycles' (the divisor) to 32-bit so I > agree it should be u32 instead of u64. > If not, I would prefer to keep using a 64-by-32 division and avoid > casting 'hz' to 32-bit. The confusing bit here is that the extra function just serves to the dividend 'cycles' from 32-bit to 64-bit, and then calling div_u64() implicitly casts the dividend 'hz' from 64-bit to 32-bit, so you definitely get a 32-by-32 division already if the function is inlined properly. I think storing 'rtc_hz' as a u32 variable and adding a range check when filling it would help, mainly to save the next reader from having to understand what is going on. >> It would appear that this makes the rtc unsuitable for >> storing absolute time across reboots, and only serve during >> runtime, which is a limitation you should probably document. >> > > Actually there is the option to use DIV512 and/or DIV32 hardware > divisors for the RTC clock. The driver uses a DIV512 divisor by default > in order to achieve higher RTC count ranges (by achieving a smaller RTC > freq). Therefore, the 37 hours become 37 * 512 => ~ 2 years. Ah, makes sense. Can you add comments in an appropriate place in the code about this? > However, the rtc limitation of not being persistent during reboot > remains, due to hardware RTC module registers present of NXP S32G2/S32G3 > SoCs being reset during system reboot. On the other hand, during system > suspend, the RTC module will keep counting if a clock source is available. > > Currently, this limittion is documented as follows: > "RTC tracks clock time during system suspend. It can be a wakeup source > for the S32G2/S32G3 SoC based boards. > > The RTC module from S32G2/S32G3 is not battery-powered and it is not > kept alive during system reset." My bad, I really should not have skipped the patch description ;-) >> If 'counter' wraps every 37 hours, this will inevitably fail, >> right? E.g. if priv->base.cycles was already at a large >> 32-bit number, even reading it shortly later will produce >> a small value after the wraparound. >> >> Using something like time_before() should address this, >> but I think you may need a custom version that works on >> 32-bit numbers. >> > > This is correct. Would the following change be acceptable ? > > - if (counter < priv->base.cycles) > - return -EINVAL; > - > - counter -= priv->base.cycles; > + if (counter < priv->base.cycles) { > + /* A rollover on RTCCTN has occurred */ > + counter += RTCCNT_MAX_VAL - priv->base.cycles; > + priv->base.cycles = 0; > + } else { > + counter -= priv->base.cycles; > + } This is the same as just removing the error handling and relying on unsigned integer overflow semantics. The usual check we do in time_before()/time_after instead checks if the elapsed time is less than half the available range: #define time_after(a,b) \ (typecheck(unsigned long, a) && \ typecheck(unsigned long, b) && \ ((long)((b) - (a)) < 0)) >>> +static int s32g_rtc_resume(struct device *dev) >>> +{ >>> + struct rtc_priv *priv = dev_get_drvdata(dev); >>> + int ret; >>> + >>> + if (!device_may_wakeup(dev)) >>> + return 0; >>> + >>> + /* Disable wake-up interrupts */ >>> + s32g_enable_api_irq(dev, 0); >>> + >>> + ret = rtc_clk_src_setup(priv); >>> + if (ret) >>> + return ret; >>> + >>> + /* >>> + * Now RTCCNT has just been reset, and is out of sync with priv->base; >>> + * reapply the saved time settings. >>> + */ >>> + return s32g_rtc_set_time(dev, &priv->base.tm); >>> +} >> >> This also fails if the system has been suspended for more than >> 37 hours, right? > > Actually, the system would not go into suspend (returning with error) if > the alarm setting passes the 32-bit / clk_freq range. > The check is added in 'sec_to_rtcval' which is called from the suspend > routine. Who sets that alarm though? Are you relying on custom userspace for this, or is that something that the kernel already does that I'm missing? Arnd
On 12/6/2024 2:41 PM, Arnd Bergmann wrote: > [You don't often get email from arnd@arndb.de. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ] > > On Fri, Dec 6, 2024, at 13:05, Ciprian Marian Costea wrote: >> On 12/6/2024 10:04 AM, Arnd Bergmann wrote: >>> >>> However, the range of the register value is only 32 bits, >>> which means there is no need to ever divide it by a 64-bit >>> number, and with the 32kHz clock in the binding example, >>> you only have about 37 hours worth of range here. >>> >> >> I am not sure what is the suggestion here. To cast 'cycles' variable to >> 32-bit ? >> If yes, indeed 'div_u64' converts 'cycles' (the divisor) to 32-bit so I >> agree it should be u32 instead of u64. >> If not, I would prefer to keep using a 64-by-32 division and avoid >> casting 'hz' to 32-bit. > > The confusing bit here is that the extra function just serves to > the dividend 'cycles' from 32-bit to 64-bit, and then calling > div_u64() implicitly casts the dividend 'hz' from 64-bit to > 32-bit, so you definitely get a 32-by-32 division already > if the function is inlined properly. > > I think storing 'rtc_hz' as a u32 variable and adding a range > check when filling it would help, mainly to save the next reader > from having to understand what is going on. > The confusion on my end is that I cannot see where 'div_u64() implicitly casts the dividend 'hz' from 64-bit to 32-bit' by following the method's implementation [1] But I agree that 'rtc_hz' can be stored into a 32-bit variable with a range check added when it is taken from the Linux clock API to avoid any unneeded abstractions. [1] https://elixir.bootlin.com/linux/v6.12.4/source/include/linux/math64.h#L127 >>> It would appear that this makes the rtc unsuitable for >>> storing absolute time across reboots, and only serve during >>> runtime, which is a limitation you should probably document. >>> >> >> Actually there is the option to use DIV512 and/or DIV32 hardware >> divisors for the RTC clock. The driver uses a DIV512 divisor by default >> in order to achieve higher RTC count ranges (by achieving a smaller RTC >> freq). Therefore, the 37 hours become 37 * 512 => ~ 2 years. > > Ah, makes sense. Can you add comments in an appropriate place in > the code about this? > Sure. I will add such comment in the S32G RTC driver in V7. >> However, the rtc limitation of not being persistent during reboot >> remains, due to hardware RTC module registers present of NXP S32G2/S32G3 >> SoCs being reset during system reboot. On the other hand, during system >> suspend, the RTC module will keep counting if a clock source is available. >> >> Currently, this limittion is documented as follows: >> "RTC tracks clock time during system suspend. It can be a wakeup source >> for the S32G2/S32G3 SoC based boards. >> >> The RTC module from S32G2/S32G3 is not battery-powered and it is not >> kept alive during system reset." > > My bad, I really should not have skipped the patch > description ;-) > >>> If 'counter' wraps every 37 hours, this will inevitably fail, >>> right? E.g. if priv->base.cycles was already at a large >>> 32-bit number, even reading it shortly later will produce >>> a small value after the wraparound. >>> >>> Using something like time_before() should address this, >>> but I think you may need a custom version that works on >>> 32-bit numbers. >>> >> >> This is correct. Would the following change be acceptable ? >> >> - if (counter < priv->base.cycles) >> - return -EINVAL; >> - >> - counter -= priv->base.cycles; >> + if (counter < priv->base.cycles) { >> + /* A rollover on RTCCTN has occurred */ >> + counter += RTCCNT_MAX_VAL - priv->base.cycles; >> + priv->base.cycles = 0; >> + } else { >> + counter -= priv->base.cycles; >> + } > > This is the same as just removing the error handling and > relying on unsigned integer overflow semantics. > > The usual check we do in time_before()/time_after instead > checks if the elapsed time is less than half the available > range: > > #define time_after(a,b) \ > (typecheck(unsigned long, a) && \ > typecheck(unsigned long, b) && \ > ((long)((b) - (a)) < 0)) > Ok. Thanks for the suggestion. I will look into using 'time_before()/time_after()' API instead of directly checking via comparison operators. >>>> +static int s32g_rtc_resume(struct device *dev) >>>> +{ >>>> + struct rtc_priv *priv = dev_get_drvdata(dev); >>>> + int ret; >>>> + >>>> + if (!device_may_wakeup(dev)) >>>> + return 0; >>>> + >>>> + /* Disable wake-up interrupts */ >>>> + s32g_enable_api_irq(dev, 0); >>>> + >>>> + ret = rtc_clk_src_setup(priv); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + /* >>>> + * Now RTCCNT has just been reset, and is out of sync with priv->base; >>>> + * reapply the saved time settings. >>>> + */ >>>> + return s32g_rtc_set_time(dev, &priv->base.tm); >>>> +} >>> >>> This also fails if the system has been suspended for more than >>> 37 hours, right? >> >> Actually, the system would not go into suspend (returning with error) if >> the alarm setting passes the 32-bit / clk_freq range. >> The check is added in 'sec_to_rtcval' which is called from the suspend >> routine. > > Who sets that alarm though? Are you relying on custom userspace > for this, or is that something that the kernel already does > that I'm missing? > > Arnd The test usage is via 'rtcwake' [2] userspace tool. I've detailed a bit the testing scenario in the cover letter for this patchset [3]: " Following is an example of Suspend to RAM trigger on S32G2/S32G3 SoCs, using userspace tools such as rtcwake: # rtcwake -s 2 -m mem # rtcwake: assuming RTC uses UTC ... # rtcwake: wakeup from "mem" using /dev/rtc0 at Wed Feb 6 06:28:36 2036 # " [2] https://man7.org/linux/man-pages/man8/rtcwake.8.html [3] https://lore.kernel.org/all/20241206070955.1503412-1-ciprianmarian.costea@oss.nxp.com/ Best Regards, Ciprian
Hi Ciprian,
kernel test robot noticed the following build errors:
[auto build test ERROR on abelloni/rtc-next]
[also build test ERROR on robh/for-next linus/master v6.13-rc2 next-20241209]
[cannot apply to arm64/for-next/core]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Ciprian-Costea/dt-bindings-rtc-add-schema-for-NXP-S32G2-S32G3-SoCs/20241206-151308
base: https://git.kernel.org/pub/scm/linux/kernel/git/abelloni/linux.git rtc-next
patch link: https://lore.kernel.org/r/20241206070955.1503412-3-ciprianmarian.costea%40oss.nxp.com
patch subject: [PATCH v6 2/4] rtc: s32g: add NXP S32G2/S32G3 SoC support
config: i386-allmodconfig (https://download.01.org/0day-ci/archive/20241210/202412101248.CBSpbBCZ-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241210/202412101248.CBSpbBCZ-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202412101248.CBSpbBCZ-lkp@intel.com/
All errors (new ones prefixed by >>, old ones prefixed by <<):
ERROR: modpost: "__udivdi3" [drivers/rtc/rtc-s32g.ko] undefined!
>> ERROR: modpost: "__divdi3" [drivers/rtc/rtc-s32g.ko] undefined!
Kconfig warnings: (for reference only)
WARNING: unmet direct dependencies detected for MODVERSIONS
Depends on [n]: MODULES [=y] && !COMPILE_TEST [=y]
Selected by [y]:
- RANDSTRUCT_FULL [=y] && (CC_HAS_RANDSTRUCT [=n] || GCC_PLUGINS [=y]) && MODULES [=y]
WARNING: unmet direct dependencies detected for GET_FREE_REGION
Depends on [n]: SPARSEMEM [=n]
Selected by [m]:
- RESOURCE_KUNIT_TEST [=m] && RUNTIME_TESTING_MENU [=y] && KUNIT [=m]
On Mon, Dec 9, 2024, at 18:17, Ciprian Marian Costea wrote: > On 12/6/2024 2:41 PM, Arnd Bergmann wrote: >> I think storing 'rtc_hz' as a u32 variable and adding a range >> check when filling it would help, mainly to save the next reader >> from having to understand what is going on. >> > > The confusion on my end is that I cannot see where 'div_u64() implicitly > casts the dividend 'hz' from 64-bit to 32-bit' by following the method's > implementation [1] I mean passing a 64-bit variable into a function that takes a 32-bit argument truncates the range. > But I agree that 'rtc_hz' can be stored into a 32-bit variable with a > range check added when it is taken from the Linux clock API to avoid any > unneeded abstractions. ok >> >> This is the same as just removing the error handling and >> relying on unsigned integer overflow semantics. >> >> The usual check we do in time_before()/time_after instead >> checks if the elapsed time is less than half the available >> range: >> >> #define time_after(a,b) \ >> (typecheck(unsigned long, a) && \ >> typecheck(unsigned long, b) && \ >> ((long)((b) - (a)) < 0)) >> > > Ok. Thanks for the suggestion. I will look into using > 'time_before()/time_after()' API instead of directly checking via > comparison operators. To be clear: you can't directly use time_before() here because that takes an 'unsigned long' argument, so you want the same logic, but for u32 values. I have not found an existing helper for that, but it's possible I missed it. >> Who sets that alarm though? Are you relying on custom userspace >> for this, or is that something that the kernel already does >> that I'm missing? > > The test usage is via 'rtcwake' [2] userspace tool. > I've detailed a bit the testing scenario in the cover letter for this > patchset [3]: > > " > Following is an example of Suspend to RAM trigger on S32G2/S32G3 SoCs, > using userspace tools such as rtcwake: > # rtcwake -s 2 -m mem > # rtcwake: assuming RTC uses UTC ... > # rtcwake: wakeup from "mem" using /dev/rtc0 at Wed Feb 6 06:28:36 2036 Got it. I feel this also needs either some documentation in the source code, or some infrastructure in the rtc layer if this is a common problem in other drivers as well. If there is a maximum time that the system can be suspended for without a wakeup, why not just set an earlier wakeup in the kernel when you have all the information for it? Or maybe this should not actually be an 'rtc' driver at all? In the old days, we used drivers like arch/arm/mach-omap1/timer32k.c to register a handler for read_persistent_clock64(), which completely bypasses the RTC layer and provides both automatic wakeup and more accurate accounting of sleep time. Another example was the tegra clocksource driver, which used to use read_persistent_clock64() but changed to being a CLOCK_SOURCE_SUSPEND_NONSTOP source in 95170f0708f2 ("clocksource/drivers/tegra: Rework for compensation of suspend time"). The same seems true for timer-ti-32k.c and timer-sprd.c. Alexandre, Daniel, any recommendations here? Arnd
On 10/12/2024 09:22:51+0100, Arnd Bergmann wrote: > On Mon, Dec 9, 2024, at 18:17, Ciprian Marian Costea wrote: > > On 12/6/2024 2:41 PM, Arnd Bergmann wrote: > > >> I think storing 'rtc_hz' as a u32 variable and adding a range > >> check when filling it would help, mainly to save the next reader > >> from having to understand what is going on. > >> > > > > The confusion on my end is that I cannot see where 'div_u64() implicitly > > casts the dividend 'hz' from 64-bit to 32-bit' by following the method's > > implementation [1] > > I mean passing a 64-bit variable into a function that takes a > 32-bit argument truncates the range. > > > But I agree that 'rtc_hz' can be stored into a 32-bit variable with a > > range check added when it is taken from the Linux clock API to avoid any > > unneeded abstractions. > > ok > > >> > >> This is the same as just removing the error handling and > >> relying on unsigned integer overflow semantics. > >> > >> The usual check we do in time_before()/time_after instead > >> checks if the elapsed time is less than half the available > >> range: > >> > >> #define time_after(a,b) \ > >> (typecheck(unsigned long, a) && \ > >> typecheck(unsigned long, b) && \ > >> ((long)((b) - (a)) < 0)) > >> > > > > Ok. Thanks for the suggestion. I will look into using > > 'time_before()/time_after()' API instead of directly checking via > > comparison operators. > > To be clear: you can't directly use time_before() here because > that takes an 'unsigned long' argument, so you want the > same logic, but for u32 values. I have not found an existing > helper for that, but it's possible I missed it. > > >> Who sets that alarm though? Are you relying on custom userspace > >> for this, or is that something that the kernel already does > >> that I'm missing? > > > > The test usage is via 'rtcwake' [2] userspace tool. > > I've detailed a bit the testing scenario in the cover letter for this > > patchset [3]: > > > > " > > Following is an example of Suspend to RAM trigger on S32G2/S32G3 SoCs, > > using userspace tools such as rtcwake: > > # rtcwake -s 2 -m mem > > # rtcwake: assuming RTC uses UTC ... > > # rtcwake: wakeup from "mem" using /dev/rtc0 at Wed Feb 6 06:28:36 2036 > > Got it. I feel this also needs either some documentation in > the source code, or some infrastructure in the rtc layer if > this is a common problem in other drivers as well. If there > is a maximum time that the system can be suspended for without > a wakeup, why not just set an earlier wakeup in the kernel > when you have all the information for it? > > Or maybe this should not actually be an 'rtc' driver at all? > In the old days, we used drivers like > arch/arm/mach-omap1/timer32k.c to register a handler > for read_persistent_clock64(), which completely bypasses > the RTC layer and provides both automatic wakeup and more > accurate accounting of sleep time. > > Another example was the tegra clocksource driver, which used > to use read_persistent_clock64() but changed to being > a CLOCK_SOURCE_SUSPEND_NONSTOP source in 95170f0708f2 > ("clocksource/drivers/tegra: Rework for compensation of > suspend time"). The same seems true for timer-ti-32k.c and > timer-sprd.c. > > Alexandre, Daniel, any recommendations here? > We have a few driver for timers that are not actual RTCs but need the common alarm and wakeup API so they can use alarmtimers for example. The limitation is not super common as RTCs will generally have the same range support for alarm as what they have for time and date. However, I'm still super confused by how complex this driver is. There are only 3 interesting registers in this IP, one of those being read only...
On 06/12/2024 09:09:53+0200, Ciprian Costea wrote: > +/* > + * S32G2 and S32G3 SoCs have RTC clock source1 reserved and > + * should not be used. > + */ > +#define RTC_CLK_SRC1_RESERVED BIT(1) > + > +enum { > + DIV1 = 1, > + DIV32 = 32, > + DIV512 = 512, > + DIV512_32 = 16384 > +}; > + > +static const char *rtc_clk_src[RTC_CLK_MUX_SIZE] = { > + "source0", > + "source1", > + "source2", > + "source3" > +}; > + > +struct rtc_time_base { > + s64 sec; > + u64 cycles; > + struct rtc_time tm; I don't think storing an rtc_time is necessary. I don't even think cycles is necessary. > +}; > + > +struct rtc_priv { > + struct rtc_device *rdev; > + void __iomem *rtc_base; > + struct clk *ipg; > + struct clk *clk_src; > + const struct rtc_soc_data *rtc_data; > + struct rtc_time_base base; > + u64 rtc_hz; > + int irq; > + int clk_src_idx; > +}; > + > +struct rtc_soc_data { > + u32 clk_div; > + u32 reserved_clk_mask; > +}; > + > +static const struct rtc_soc_data rtc_s32g2_data = { > + .clk_div = DIV512, If you input clock rate is higher that 16kHz, why don't you divide by 16384? > + .reserved_clk_mask = RTC_CLK_SRC1_RESERVED, > +}; > + > +static u64 cycles_to_sec(u64 hz, u64 cycles) > +{ > + return div_u64(cycles, hz); > +} > + > +/** > + * sec_to_rtcval - Convert a number of seconds to a value suitable for > + * RTCVAL in our clock's > + * current configuration. > + * @priv: Pointer to the 'rtc_priv' structure > + * @seconds: Number of seconds to convert > + * @rtcval: The value to go into RTCVAL[RTCVAL] > + * > + * Return: 0 for success, -EINVAL if @seconds push the counter past the > + * 32bit register range > + */ > +static int sec_to_rtcval(const struct rtc_priv *priv, > + unsigned long seconds, u32 *rtcval) > +{ > + u32 delta_cnt; > + > + if (!seconds || seconds > cycles_to_sec(priv->rtc_hz, RTCCNT_MAX_VAL)) > + return -EINVAL; > + > + /* > + * RTCCNT is read-only; we must return a value relative to the > + * current value of the counter (and hope we don't linger around > + * too much before we get to enable the interrupt) > + */ > + delta_cnt = seconds * priv->rtc_hz; > + *rtcval = delta_cnt + ioread32(priv->rtc_base + RTCCNT_OFFSET); > + > + return 0; > +} > + > +static irqreturn_t s32g_rtc_handler(int irq, void *dev) > +{ > + struct rtc_priv *priv = platform_get_drvdata(dev); > + u32 status; > + > + status = ioread32(priv->rtc_base + RTCS_OFFSET); > + > + if (status & RTCS_RTCF) { > + iowrite32(0x0, priv->rtc_base + RTCVAL_OFFSET); > + iowrite32(status | RTCS_RTCF, priv->rtc_base + RTCS_OFFSET); > + rtc_update_irq(priv->rdev, 1, RTC_AF); > + } > + > + if (status & RTCS_APIF) { > + iowrite32(status | RTCS_APIF, priv->rtc_base + RTCS_OFFSET); > + rtc_update_irq(priv->rdev, 1, RTC_PF); I don't think you use APIF as a periodic interrupt so it doesn't really make sense to use RTC_PF instead of RTC_AF. > + } > + > + return IRQ_HANDLED; > +} > + > +static s64 s32g_rtc_get_time_or_alrm(struct rtc_priv *priv, > + u32 offset) > +{ > + u32 counter; > + > + counter = ioread32(priv->rtc_base + offset); > + > + if (counter < priv->base.cycles) > + return -EINVAL; > + > + counter -= priv->base.cycles; > + > + return priv->base.sec + cycles_to_sec(priv->rtc_hz, counter); > +} > + > +static int s32g_rtc_read_time(struct device *dev, > + struct rtc_time *tm) > +{ > + struct rtc_priv *priv = dev_get_drvdata(dev); > + s64 sec; > + > + sec = s32g_rtc_get_time_or_alrm(priv, RTCCNT_OFFSET); > + if (sec < 0) > + return -EINVAL; > + > + rtc_time64_to_tm(sec, tm); > + > + return 0; > +} > + > +static int s32g_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm) > +{ > + struct rtc_priv *priv = dev_get_drvdata(dev); > + u32 rtcc, rtccnt, rtcval; > + s64 sec; > + > + sec = s32g_rtc_get_time_or_alrm(priv, RTCVAL_OFFSET); > + if (sec < 0) > + return -EINVAL; > + > + rtc_time64_to_tm(sec, &alrm->time); > + > + rtcc = ioread32(priv->rtc_base + RTCC_OFFSET); > + alrm->enabled = sec && (rtcc & RTCC_RTCIE); > + > + alrm->pending = 0; > + if (alrm->enabled) { > + rtccnt = ioread32(priv->rtc_base + RTCCNT_OFFSET); > + rtcval = ioread32(priv->rtc_base + RTCVAL_OFFSET); > + > + if (rtccnt < rtcval) > + alrm->pending = 1; This limits the range of your alarm, why don't you simply check whether RTCF is set? > + } > + > + return 0; > +} > + > +static int s32g_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled) > +{ > + struct rtc_priv *priv = dev_get_drvdata(dev); > + u32 rtcc; > + > + if (!priv->irq) > + return -EIO; This will never happen as you are not letting probe finish when you can't request the irq. > + > + rtcc = ioread32(priv->rtc_base + RTCC_OFFSET); > + if (enabled) > + rtcc |= RTCC_RTCIE; > + > + iowrite32(rtcc, priv->rtc_base + RTCC_OFFSET); > + > + return 0; > +} > + > +static int s32g_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm) > +{ > + struct rtc_priv *priv = dev_get_drvdata(dev); > + struct rtc_time time_crt; > + long long t_crt, t_alrm; > + u32 rtcval, rtcs; > + int ret = 0; > + > + iowrite32(0x0, priv->rtc_base + RTCVAL_OFFSET); > + > + t_alrm = rtc_tm_to_time64(&alrm->time); > + > + /* > + * Assuming the alarm is being set relative to the same time > + * returned by our s32g_rtc_read_time callback > + */ > + ret = s32g_rtc_read_time(dev, &time_crt); > + if (ret) > + return ret; > + > + t_crt = rtc_tm_to_time64(&time_crt); > + ret = sec_to_rtcval(priv, t_alrm - t_crt, &rtcval); > + if (ret) { > + dev_warn(dev, "Alarm is set too far in the future\n"); > + return -ERANGE; > + } > + > + ret = read_poll_timeout(ioread32, rtcs, !(rtcs & RTCS_INV_RTC), > + 0, RTC_SYNCH_TIMEOUT, false, priv->rtc_base + RTCS_OFFSET); > + if (ret) > + return ret; > + > + iowrite32(rtcval, priv->rtc_base + RTCVAL_OFFSET); > + > + return 0; > +} > + > +static int s32g_rtc_set_time(struct device *dev, > + struct rtc_time *time) > +{ > + struct rtc_priv *priv = dev_get_drvdata(dev); > + > + priv->base.cycles = ioread32(priv->rtc_base + RTCCNT_OFFSET); > + priv->base.sec = rtc_tm_to_time64(time); > + To simplify all the calculations you are doing, I suggest you reset RTCCNT here and store the epoch of the rtc as a number of seconds. This wll allow you to avoid having to read the counter in set_alarm also, you then get a direct conversion for RTCVAL as this will simply be rtc_tm_to_time64(&alrm->time) - epoch that you have to convert in cycles You will also then know right away whether this is too large to fit in a 32bit register. > + return 0; > +} > + > +/* > + * Disable the 32-bit free running counter. > + * This allows Clock Source and Divisors selection > + * to be performed without causing synchronization issues. > + */ > +static void s32g_rtc_disable(struct rtc_priv *priv) > +{ > + u32 rtcc = ioread32(priv->rtc_base + RTCC_OFFSET); > + > + rtcc &= ~RTCC_CNTEN; > + iowrite32(rtcc, priv->rtc_base + RTCC_OFFSET); > +} > + > +static void s32g_rtc_enable(struct rtc_priv *priv) > +{ > + u32 rtcc = ioread32(priv->rtc_base + RTCC_OFFSET); > + > + rtcc |= RTCC_CNTEN; > + iowrite32(rtcc, priv->rtc_base + RTCC_OFFSET); > +} > + > +static int rtc_clk_src_setup(struct rtc_priv *priv) > +{ > + u32 rtcc = 0; > + > + if (priv->rtc_data->reserved_clk_mask & (1 << priv->clk_src_idx)) > + return -EOPNOTSUPP; > + > + rtcc = FIELD_PREP(RTCC_CLKSEL_MASK, priv->clk_src_idx); > + > + switch (priv->rtc_data->clk_div) { > + case DIV512_32: > + rtcc |= RTCC_DIV512EN; > + rtcc |= RTCC_DIV32EN; > + break; > + case DIV512: > + rtcc |= RTCC_DIV512EN; > + break; > + case DIV32: > + rtcc |= RTCC_DIV32EN; > + break; > + case DIV1: > + break; > + default: > + return -EINVAL; > + } > + > + rtcc |= RTCC_RTCIE; > + /* > + * Make sure the CNTEN is 0 before we configure > + * the clock source and dividers. > + */ > + s32g_rtc_disable(priv); > + iowrite32(rtcc, priv->rtc_base + RTCC_OFFSET); > + s32g_rtc_enable(priv); > + > + return 0; > +} > + > +static const struct rtc_class_ops rtc_ops = { > + .read_time = s32g_rtc_read_time, > + .set_time = s32g_rtc_set_time, > + .read_alarm = s32g_rtc_read_alarm, > + .set_alarm = s32g_rtc_set_alarm, > + .alarm_irq_enable = s32g_rtc_alarm_irq_enable, > +}; > + > +static int rtc_clk_dts_setup(struct rtc_priv *priv, > + struct device *dev) > +{ > + int i; > + > + priv->ipg = devm_clk_get_enabled(dev, "ipg"); > + if (IS_ERR(priv->ipg)) > + return dev_err_probe(dev, PTR_ERR(priv->ipg), > + "Failed to get 'ipg' clock\n"); > + > + for (i = 0; i < RTC_CLK_MUX_SIZE; i++) { > + priv->clk_src = devm_clk_get_enabled(dev, rtc_clk_src[i]); > + if (!IS_ERR(priv->clk_src)) { > + priv->clk_src_idx = i; > + break; > + } > + } > + > + if (IS_ERR(priv->clk_src)) > + return dev_err_probe(dev, PTR_ERR(priv->clk_src), > + "Failed to get rtc module clock source\n"); > + > + return 0; > +} > + > +static int s32g_rtc_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct rtc_priv *priv; > + int ret = 0; > + > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + priv->rtc_data = of_device_get_match_data(dev); > + if (!priv->rtc_data) > + return -ENODEV; > + > + priv->rtc_base = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(priv->rtc_base)) > + return PTR_ERR(priv->rtc_base); > + > + device_init_wakeup(dev, true); > + > + ret = rtc_clk_dts_setup(priv, dev); > + if (ret) > + return ret; > + > + priv->rdev = devm_rtc_allocate_device(dev); > + if (IS_ERR(priv->rdev)) > + return PTR_ERR(priv->rdev); > + > + ret = rtc_clk_src_setup(priv); > + if (ret) > + return ret; > + > + priv->rtc_hz = clk_get_rate(priv->clk_src); > + if (!priv->rtc_hz) { > + dev_err(dev, "Failed to get RTC frequency\n"); > + ret = -EINVAL; > + goto disable_rtc; > + } > + > + priv->rtc_hz /= priv->rtc_data->clk_div; > + > + platform_set_drvdata(pdev, priv); > + priv->rdev->ops = &rtc_ops; > + > + priv->irq = platform_get_irq(pdev, 0); > + if (priv->irq < 0) { > + ret = priv->irq; > + goto disable_rtc; > + } > + > + ret = devm_request_irq(dev, priv->irq, > + s32g_rtc_handler, 0, dev_name(dev), pdev); > + if (ret) { > + dev_err(dev, "Request interrupt %d failed, error: %d\n", > + priv->irq, ret); > + goto disable_rtc; > + } > + > + ret = devm_rtc_register_device(priv->rdev); > + if (ret) > + goto disable_rtc; > + > + return 0; > + > +disable_rtc: > + s32g_rtc_disable(priv); > + return ret; > +} > + > +static void s32g_enable_api_irq(struct device *dev, unsigned int enabled) > +{ > + struct rtc_priv *priv = dev_get_drvdata(dev); > + u32 api_irq = RTCC_APIEN | RTCC_APIIE; > + u32 rtcc; > + > + rtcc = ioread32(priv->rtc_base + RTCC_OFFSET); > + if (enabled) > + rtcc |= api_irq; > + else > + rtcc &= ~api_irq; > + iowrite32(rtcc, priv->rtc_base + RTCC_OFFSET); > +} > + > +static int s32g_rtc_suspend(struct device *dev) > +{ > + struct rtc_priv *init_priv = dev_get_drvdata(dev); > + struct rtc_priv priv; > + long long base_sec; > + u32 rtcval, rtccnt, offset; > + int ret = 0; > + u32 sec; > + > + if (!device_may_wakeup(dev)) > + return 0; > + > + /* Save last known timestamp */ > + ret = s32g_rtc_read_time(dev, &init_priv->base.tm); > + if (ret) > + return ret; I don't think that whole calculation is necessary as you are never actually resetting RTCCNT in suspend > + > + /* > + * Use a local copy of the RTC control block to > + * avoid restoring it on resume path. > + */ > + memcpy(&priv, init_priv, sizeof(priv)); > + > + rtccnt = ioread32(init_priv->rtc_base + RTCCNT_OFFSET); > + rtcval = ioread32(init_priv->rtc_base + RTCVAL_OFFSET); > + offset = rtcval - rtccnt; > + sec = cycles_to_sec(init_priv->rtc_hz, offset); > + > + /* Adjust for the number of seconds we'll be asleep */ > + base_sec = rtc_tm_to_time64(&init_priv->base.tm); > + base_sec += sec; > + rtc_time64_to_tm(base_sec, &init_priv->base.tm); > + > + ret = sec_to_rtcval(&priv, sec, &rtcval); > + if (ret) { > + dev_warn(dev, "Alarm is too far in the future\n"); > + return -ERANGE; > + } > + > + s32g_enable_api_irq(dev, 1); > + iowrite32(offset, priv.rtc_base + APIVAL_OFFSET); What about always using APIVAL instead of RTCVAL so you don't have anything to do in s32g_rtc_suspend. > + > + return ret; > +} > + > +static int s32g_rtc_resume(struct device *dev) > +{ > + struct rtc_priv *priv = dev_get_drvdata(dev); > + int ret; > + > + if (!device_may_wakeup(dev)) > + return 0; > + > + /* Disable wake-up interrupts */ > + s32g_enable_api_irq(dev, 0); > + > + ret = rtc_clk_src_setup(priv); > + if (ret) > + return ret; I don't think this is necessary. > + > + /* > + * Now RTCCNT has just been reset, and is out of sync with priv->base; > + * reapply the saved time settings. > + */ > + return s32g_rtc_set_time(dev, &priv->base.tm); And so this is useless too so yo udon't actually have anything to do in s32g_rtc_resume. > +} > + > +static const struct of_device_id rtc_dt_ids[] = { > + { .compatible = "nxp,s32g2-rtc", .data = &rtc_s32g2_data}, > + { /* sentinel */ }, > +}; > + > +static DEFINE_SIMPLE_DEV_PM_OPS(s32g_rtc_pm_ops, > + s32g_rtc_suspend, s32g_rtc_resume); > + > +static struct platform_driver s32g_rtc_driver = { > + .driver = { > + .name = "s32g-rtc", > + .pm = pm_sleep_ptr(&s32g_rtc_pm_ops), > + .of_match_table = rtc_dt_ids, > + }, > + .probe = s32g_rtc_probe, > +}; > +module_platform_driver(s32g_rtc_driver); > + > +MODULE_AUTHOR("NXP"); > +MODULE_DESCRIPTION("NXP RTC driver for S32G2/S32G3"); > +MODULE_LICENSE("GPL"); > -- > 2.45.2 >
diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig index a60bcc791a48..25ee7c6d8748 100644 --- a/drivers/rtc/Kconfig +++ b/drivers/rtc/Kconfig @@ -2103,4 +2103,15 @@ config RTC_DRV_AMLOGIC_A4 This driver can also be built as a module. If so, the module will be called "rtc-amlogic-a4". +config RTC_DRV_S32G + tristate "RTC driver for S32G2/S32G3 SoCs" + depends on ARCH_S32 || COMPILE_TEST + depends on COMMON_CLK + help + Say yes to enable RTC driver for platforms based on the + S32G2/S32G3 SoC family. + + This RTC module can be used as a wakeup source. + Please note that it is not battery-powered. + endif # RTC_CLASS diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile index 489b4ab07068..e4b616ecd5ce 100644 --- a/drivers/rtc/Makefile +++ b/drivers/rtc/Makefile @@ -161,6 +161,7 @@ obj-$(CONFIG_RTC_DRV_RX8111) += rtc-rx8111.o obj-$(CONFIG_RTC_DRV_RX8581) += rtc-rx8581.o obj-$(CONFIG_RTC_DRV_RZN1) += rtc-rzn1.o obj-$(CONFIG_RTC_DRV_RENESAS_RTCA3) += rtc-renesas-rtca3.o +obj-$(CONFIG_RTC_DRV_S32G) += rtc-s32g.o obj-$(CONFIG_RTC_DRV_S35390A) += rtc-s35390a.o obj-$(CONFIG_RTC_DRV_S3C) += rtc-s3c.o obj-$(CONFIG_RTC_DRV_S5M) += rtc-s5m.o diff --git a/drivers/rtc/rtc-s32g.c b/drivers/rtc/rtc-s32g.c new file mode 100644 index 000000000000..0989b6f2a613 --- /dev/null +++ b/drivers/rtc/rtc-s32g.c @@ -0,0 +1,529 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Copyright 2024 NXP + */ + +#include <linux/bitfield.h> +#include <linux/clk.h> +#include <linux/clk-provider.h> +#include <linux/err.h> +#include <linux/iopoll.h> +#include <linux/math64.h> +#include <linux/of_irq.h> +#include <linux/platform_device.h> +#include <linux/rtc.h> + +#define RTCC_OFFSET 0x4ul +#define RTCS_OFFSET 0x8ul +#define RTCCNT_OFFSET 0xCul +#define APIVAL_OFFSET 0x10ul +#define RTCVAL_OFFSET 0x14ul + +/* RTCC fields */ +#define RTCC_CNTEN BIT(31) +#define RTCC_RTCIE BIT(30) +#define RTCC_APIEN BIT(15) +#define RTCC_APIIE BIT(14) +#define RTCC_CLKSEL_MASK GENMASK(13, 12) +#define RTCC_DIV512EN BIT(11) +#define RTCC_DIV32EN BIT(10) + +/* RTCS fields */ +#define RTCS_RTCF BIT(29) +#define RTCS_INV_RTC BIT(18) +#define RTCS_APIF BIT(13) + +#define RTCCNT_MAX_VAL GENMASK(31, 0) +#define RTC_SYNCH_TIMEOUT (100 * USEC_PER_MSEC) + +#define RTC_CLK_MUX_SIZE 4 + +/* + * S32G2 and S32G3 SoCs have RTC clock source1 reserved and + * should not be used. + */ +#define RTC_CLK_SRC1_RESERVED BIT(1) + +enum { + DIV1 = 1, + DIV32 = 32, + DIV512 = 512, + DIV512_32 = 16384 +}; + +static const char *rtc_clk_src[RTC_CLK_MUX_SIZE] = { + "source0", + "source1", + "source2", + "source3" +}; + +struct rtc_time_base { + s64 sec; + u64 cycles; + struct rtc_time tm; +}; + +struct rtc_priv { + struct rtc_device *rdev; + void __iomem *rtc_base; + struct clk *ipg; + struct clk *clk_src; + const struct rtc_soc_data *rtc_data; + struct rtc_time_base base; + u64 rtc_hz; + int irq; + int clk_src_idx; +}; + +struct rtc_soc_data { + u32 clk_div; + u32 reserved_clk_mask; +}; + +static const struct rtc_soc_data rtc_s32g2_data = { + .clk_div = DIV512, + .reserved_clk_mask = RTC_CLK_SRC1_RESERVED, +}; + +static u64 cycles_to_sec(u64 hz, u64 cycles) +{ + return div_u64(cycles, hz); +} + +/** + * sec_to_rtcval - Convert a number of seconds to a value suitable for + * RTCVAL in our clock's + * current configuration. + * @priv: Pointer to the 'rtc_priv' structure + * @seconds: Number of seconds to convert + * @rtcval: The value to go into RTCVAL[RTCVAL] + * + * Return: 0 for success, -EINVAL if @seconds push the counter past the + * 32bit register range + */ +static int sec_to_rtcval(const struct rtc_priv *priv, + unsigned long seconds, u32 *rtcval) +{ + u32 delta_cnt; + + if (!seconds || seconds > cycles_to_sec(priv->rtc_hz, RTCCNT_MAX_VAL)) + return -EINVAL; + + /* + * RTCCNT is read-only; we must return a value relative to the + * current value of the counter (and hope we don't linger around + * too much before we get to enable the interrupt) + */ + delta_cnt = seconds * priv->rtc_hz; + *rtcval = delta_cnt + ioread32(priv->rtc_base + RTCCNT_OFFSET); + + return 0; +} + +static irqreturn_t s32g_rtc_handler(int irq, void *dev) +{ + struct rtc_priv *priv = platform_get_drvdata(dev); + u32 status; + + status = ioread32(priv->rtc_base + RTCS_OFFSET); + + if (status & RTCS_RTCF) { + iowrite32(0x0, priv->rtc_base + RTCVAL_OFFSET); + iowrite32(status | RTCS_RTCF, priv->rtc_base + RTCS_OFFSET); + rtc_update_irq(priv->rdev, 1, RTC_AF); + } + + if (status & RTCS_APIF) { + iowrite32(status | RTCS_APIF, priv->rtc_base + RTCS_OFFSET); + rtc_update_irq(priv->rdev, 1, RTC_PF); + } + + return IRQ_HANDLED; +} + +static s64 s32g_rtc_get_time_or_alrm(struct rtc_priv *priv, + u32 offset) +{ + u32 counter; + + counter = ioread32(priv->rtc_base + offset); + + if (counter < priv->base.cycles) + return -EINVAL; + + counter -= priv->base.cycles; + + return priv->base.sec + cycles_to_sec(priv->rtc_hz, counter); +} + +static int s32g_rtc_read_time(struct device *dev, + struct rtc_time *tm) +{ + struct rtc_priv *priv = dev_get_drvdata(dev); + s64 sec; + + sec = s32g_rtc_get_time_or_alrm(priv, RTCCNT_OFFSET); + if (sec < 0) + return -EINVAL; + + rtc_time64_to_tm(sec, tm); + + return 0; +} + +static int s32g_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm) +{ + struct rtc_priv *priv = dev_get_drvdata(dev); + u32 rtcc, rtccnt, rtcval; + s64 sec; + + sec = s32g_rtc_get_time_or_alrm(priv, RTCVAL_OFFSET); + if (sec < 0) + return -EINVAL; + + rtc_time64_to_tm(sec, &alrm->time); + + rtcc = ioread32(priv->rtc_base + RTCC_OFFSET); + alrm->enabled = sec && (rtcc & RTCC_RTCIE); + + alrm->pending = 0; + if (alrm->enabled) { + rtccnt = ioread32(priv->rtc_base + RTCCNT_OFFSET); + rtcval = ioread32(priv->rtc_base + RTCVAL_OFFSET); + + if (rtccnt < rtcval) + alrm->pending = 1; + } + + return 0; +} + +static int s32g_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled) +{ + struct rtc_priv *priv = dev_get_drvdata(dev); + u32 rtcc; + + if (!priv->irq) + return -EIO; + + rtcc = ioread32(priv->rtc_base + RTCC_OFFSET); + if (enabled) + rtcc |= RTCC_RTCIE; + + iowrite32(rtcc, priv->rtc_base + RTCC_OFFSET); + + return 0; +} + +static int s32g_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm) +{ + struct rtc_priv *priv = dev_get_drvdata(dev); + struct rtc_time time_crt; + long long t_crt, t_alrm; + u32 rtcval, rtcs; + int ret = 0; + + iowrite32(0x0, priv->rtc_base + RTCVAL_OFFSET); + + t_alrm = rtc_tm_to_time64(&alrm->time); + + /* + * Assuming the alarm is being set relative to the same time + * returned by our s32g_rtc_read_time callback + */ + ret = s32g_rtc_read_time(dev, &time_crt); + if (ret) + return ret; + + t_crt = rtc_tm_to_time64(&time_crt); + ret = sec_to_rtcval(priv, t_alrm - t_crt, &rtcval); + if (ret) { + dev_warn(dev, "Alarm is set too far in the future\n"); + return -ERANGE; + } + + ret = read_poll_timeout(ioread32, rtcs, !(rtcs & RTCS_INV_RTC), + 0, RTC_SYNCH_TIMEOUT, false, priv->rtc_base + RTCS_OFFSET); + if (ret) + return ret; + + iowrite32(rtcval, priv->rtc_base + RTCVAL_OFFSET); + + return 0; +} + +static int s32g_rtc_set_time(struct device *dev, + struct rtc_time *time) +{ + struct rtc_priv *priv = dev_get_drvdata(dev); + + priv->base.cycles = ioread32(priv->rtc_base + RTCCNT_OFFSET); + priv->base.sec = rtc_tm_to_time64(time); + + return 0; +} + +/* + * Disable the 32-bit free running counter. + * This allows Clock Source and Divisors selection + * to be performed without causing synchronization issues. + */ +static void s32g_rtc_disable(struct rtc_priv *priv) +{ + u32 rtcc = ioread32(priv->rtc_base + RTCC_OFFSET); + + rtcc &= ~RTCC_CNTEN; + iowrite32(rtcc, priv->rtc_base + RTCC_OFFSET); +} + +static void s32g_rtc_enable(struct rtc_priv *priv) +{ + u32 rtcc = ioread32(priv->rtc_base + RTCC_OFFSET); + + rtcc |= RTCC_CNTEN; + iowrite32(rtcc, priv->rtc_base + RTCC_OFFSET); +} + +static int rtc_clk_src_setup(struct rtc_priv *priv) +{ + u32 rtcc = 0; + + if (priv->rtc_data->reserved_clk_mask & (1 << priv->clk_src_idx)) + return -EOPNOTSUPP; + + rtcc = FIELD_PREP(RTCC_CLKSEL_MASK, priv->clk_src_idx); + + switch (priv->rtc_data->clk_div) { + case DIV512_32: + rtcc |= RTCC_DIV512EN; + rtcc |= RTCC_DIV32EN; + break; + case DIV512: + rtcc |= RTCC_DIV512EN; + break; + case DIV32: + rtcc |= RTCC_DIV32EN; + break; + case DIV1: + break; + default: + return -EINVAL; + } + + rtcc |= RTCC_RTCIE; + /* + * Make sure the CNTEN is 0 before we configure + * the clock source and dividers. + */ + s32g_rtc_disable(priv); + iowrite32(rtcc, priv->rtc_base + RTCC_OFFSET); + s32g_rtc_enable(priv); + + return 0; +} + +static const struct rtc_class_ops rtc_ops = { + .read_time = s32g_rtc_read_time, + .set_time = s32g_rtc_set_time, + .read_alarm = s32g_rtc_read_alarm, + .set_alarm = s32g_rtc_set_alarm, + .alarm_irq_enable = s32g_rtc_alarm_irq_enable, +}; + +static int rtc_clk_dts_setup(struct rtc_priv *priv, + struct device *dev) +{ + int i; + + priv->ipg = devm_clk_get_enabled(dev, "ipg"); + if (IS_ERR(priv->ipg)) + return dev_err_probe(dev, PTR_ERR(priv->ipg), + "Failed to get 'ipg' clock\n"); + + for (i = 0; i < RTC_CLK_MUX_SIZE; i++) { + priv->clk_src = devm_clk_get_enabled(dev, rtc_clk_src[i]); + if (!IS_ERR(priv->clk_src)) { + priv->clk_src_idx = i; + break; + } + } + + if (IS_ERR(priv->clk_src)) + return dev_err_probe(dev, PTR_ERR(priv->clk_src), + "Failed to get rtc module clock source\n"); + + return 0; +} + +static int s32g_rtc_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct rtc_priv *priv; + int ret = 0; + + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); + if (!priv) + return -ENOMEM; + + priv->rtc_data = of_device_get_match_data(dev); + if (!priv->rtc_data) + return -ENODEV; + + priv->rtc_base = devm_platform_ioremap_resource(pdev, 0); + if (IS_ERR(priv->rtc_base)) + return PTR_ERR(priv->rtc_base); + + device_init_wakeup(dev, true); + + ret = rtc_clk_dts_setup(priv, dev); + if (ret) + return ret; + + priv->rdev = devm_rtc_allocate_device(dev); + if (IS_ERR(priv->rdev)) + return PTR_ERR(priv->rdev); + + ret = rtc_clk_src_setup(priv); + if (ret) + return ret; + + priv->rtc_hz = clk_get_rate(priv->clk_src); + if (!priv->rtc_hz) { + dev_err(dev, "Failed to get RTC frequency\n"); + ret = -EINVAL; + goto disable_rtc; + } + + priv->rtc_hz /= priv->rtc_data->clk_div; + + platform_set_drvdata(pdev, priv); + priv->rdev->ops = &rtc_ops; + + priv->irq = platform_get_irq(pdev, 0); + if (priv->irq < 0) { + ret = priv->irq; + goto disable_rtc; + } + + ret = devm_request_irq(dev, priv->irq, + s32g_rtc_handler, 0, dev_name(dev), pdev); + if (ret) { + dev_err(dev, "Request interrupt %d failed, error: %d\n", + priv->irq, ret); + goto disable_rtc; + } + + ret = devm_rtc_register_device(priv->rdev); + if (ret) + goto disable_rtc; + + return 0; + +disable_rtc: + s32g_rtc_disable(priv); + return ret; +} + +static void s32g_enable_api_irq(struct device *dev, unsigned int enabled) +{ + struct rtc_priv *priv = dev_get_drvdata(dev); + u32 api_irq = RTCC_APIEN | RTCC_APIIE; + u32 rtcc; + + rtcc = ioread32(priv->rtc_base + RTCC_OFFSET); + if (enabled) + rtcc |= api_irq; + else + rtcc &= ~api_irq; + iowrite32(rtcc, priv->rtc_base + RTCC_OFFSET); +} + +static int s32g_rtc_suspend(struct device *dev) +{ + struct rtc_priv *init_priv = dev_get_drvdata(dev); + struct rtc_priv priv; + long long base_sec; + u32 rtcval, rtccnt, offset; + int ret = 0; + u32 sec; + + if (!device_may_wakeup(dev)) + return 0; + + /* Save last known timestamp */ + ret = s32g_rtc_read_time(dev, &init_priv->base.tm); + if (ret) + return ret; + + /* + * Use a local copy of the RTC control block to + * avoid restoring it on resume path. + */ + memcpy(&priv, init_priv, sizeof(priv)); + + rtccnt = ioread32(init_priv->rtc_base + RTCCNT_OFFSET); + rtcval = ioread32(init_priv->rtc_base + RTCVAL_OFFSET); + offset = rtcval - rtccnt; + sec = cycles_to_sec(init_priv->rtc_hz, offset); + + /* Adjust for the number of seconds we'll be asleep */ + base_sec = rtc_tm_to_time64(&init_priv->base.tm); + base_sec += sec; + rtc_time64_to_tm(base_sec, &init_priv->base.tm); + + ret = sec_to_rtcval(&priv, sec, &rtcval); + if (ret) { + dev_warn(dev, "Alarm is too far in the future\n"); + return -ERANGE; + } + + s32g_enable_api_irq(dev, 1); + iowrite32(offset, priv.rtc_base + APIVAL_OFFSET); + + return ret; +} + +static int s32g_rtc_resume(struct device *dev) +{ + struct rtc_priv *priv = dev_get_drvdata(dev); + int ret; + + if (!device_may_wakeup(dev)) + return 0; + + /* Disable wake-up interrupts */ + s32g_enable_api_irq(dev, 0); + + ret = rtc_clk_src_setup(priv); + if (ret) + return ret; + + /* + * Now RTCCNT has just been reset, and is out of sync with priv->base; + * reapply the saved time settings. + */ + return s32g_rtc_set_time(dev, &priv->base.tm); +} + +static const struct of_device_id rtc_dt_ids[] = { + { .compatible = "nxp,s32g2-rtc", .data = &rtc_s32g2_data}, + { /* sentinel */ }, +}; + +static DEFINE_SIMPLE_DEV_PM_OPS(s32g_rtc_pm_ops, + s32g_rtc_suspend, s32g_rtc_resume); + +static struct platform_driver s32g_rtc_driver = { + .driver = { + .name = "s32g-rtc", + .pm = pm_sleep_ptr(&s32g_rtc_pm_ops), + .of_match_table = rtc_dt_ids, + }, + .probe = s32g_rtc_probe, +}; +module_platform_driver(s32g_rtc_driver); + +MODULE_AUTHOR("NXP"); +MODULE_DESCRIPTION("NXP RTC driver for S32G2/S32G3"); +MODULE_LICENSE("GPL");