[linux-sunxi] Re: [PATCH v8 05/11] rtc: sun6i: Add support for broken-down alarm registers
On Sun, 25 Jul 2021 08:11:49 +0200
Jernej Škrabec wrote:
Hi Jernej,
> Dne petek, 23. julij 2021 ob 17:38:32 CEST je Andre Przywara napisal(a):
> > Newer versions of the Allwinner RTC, for instance as found in the H616
> > SoC, not only store the current day as a linear number, but also change
> > the way the alarm is handled: There are now two registers, that
> > explicitly store the wakeup time, in the same format as the current
> > time.
> >
> > Add support for that variant by writing the requested wakeup time
> > directly into the registers, instead of programming the seconds left, as
> > the old SoCs required.
> >
> > Signed-off-by: Andre Przywara
> > ---
> > drivers/rtc/rtc-sun6i.c | 60 +
> > 1 file changed, 43 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/rtc/rtc-sun6i.c b/drivers/rtc/rtc-sun6i.c
> > index a02be8bca6f3..f0ee20bfdccb 100644
> > --- a/drivers/rtc/rtc-sun6i.c
> > +++ b/drivers/rtc/rtc-sun6i.c
> > @@ -48,7 +48,8 @@
> >
> > /* Alarm 0 (counter) */
> > #define SUN6I_ALRM_COUNTER 0x0020
> > -#define SUN6I_ALRM_CUR_VAL 0x0024
> > +/* This holds the remaining alarm seconds on older SoCs (current value) */
> > +#define SUN6I_ALRM_COUNTER_HMS 0x0024
> > #define SUN6I_ALRM_EN 0x0028
> > #define SUN6I_ALRM_EN_CNT_EN BIT(0)
> > #define SUN6I_ALRM_IRQ_EN 0x002c
> > @@ -523,32 +524,57 @@ static int sun6i_rtc_setalarm(struct device *dev,
> struct rtc_wkalrm *wkalrm)
> > struct sun6i_rtc_dev *chip = dev_get_drvdata(dev);
> > struct rtc_time *alrm_tm = >time;
> > struct rtc_time tm_now;
> > - time64_t time_now, time_set;
> > + time64_t time_set;
> > + u32 counter_val, counter_val_hms;
> > int ret;
> >
> > - ret = sun6i_rtc_gettime(dev, _now);
> > - if (ret < 0) {
> > - dev_err(dev, "Error in getting time\n");
> > - return -EINVAL;
> > - }
> > -
> > time_set = rtc_tm_to_time64(alrm_tm);
> > - time_now = rtc_tm_to_time64(_now);
> > - if (time_set <= time_now) {
> > - dev_err(dev, "Date to set in the past\n");
> > - return -EINVAL;
> > - }
> >
> > - if ((time_set - time_now) > U32_MAX) {
> > - dev_err(dev, "Date too far in the future\n");
> > - return -EINVAL;
> > + if (chip->flags & RTC_LINEAR_DAY) {
> > + time64_t seconds;
> > +
> > + /*
> > +* The alarm registers hold the actual alarm time,
> encoded
> > +* in the same way (linear day + HMS) as the current
> time.
> > +*/
> > + counter_val_hms = SUN6I_TIME_SET_SEC_VALUE(alrm_tm-
> >tm_sec) |
> > +
> SUN6I_TIME_SET_MIN_VALUE(alrm_tm->tm_min) |
> > +
> SUN6I_TIME_SET_HOUR_VALUE(alrm_tm->tm_hour);
> > + seconds = mktime64(alrm_tm->tm_year + 1900, alrm_tm-
> >tm_mon,
> > + alrm_tm->tm_mday, 0, 0, 0);
>
> While above line should work, it's confusing why you're adding 1900 to years.
> In my opinion it would be better to use same trick you used in patch 4 -
> rtc_tm_to_time64() with hours, minutes and seconds set to 0.
IIRC setalarm does not want the time struct to be changed, but this is
OK in settime. But anyway ...
> Or maybe you
> don't even need to do that, since division by SECS_PER_DAY will round down
> anyway. In such way you hide RTC internal representation detail which doesn't
> need to be exposed here.
That is indeed a very clever idea! I changed both settime and setalarm
accordingly now, adding a comment about this.
>
> Once fixed:
> Reviewed by: Jernej Skrabec
Many thanks for the reviews!
Cheers,
Andre
> > + counter_val = div_u64(seconds, SECS_PER_DAY);
> > + } else {
> > + /* The alarm register holds the number of seconds left.
> */
> > + time64_t time_now;
> > +
> > + ret = sun6i_rtc_gettime(dev, _now);
> > + if (ret < 0) {
> > + dev_err(dev, "Error in getting time\n");
> > + return -EINVAL;
> > + }
> > +
> > + time_now = rtc_tm_to_time64(_now);
> > + if (time_set <= time_now) {
> > + dev_err(dev, "Date to set in the past\n");
> > + return -EINVAL;
> > + }
> > + if ((time_set - time_now) > U32_MAX) {
> > + dev_err(dev, "Date too far in the future\n");
> > + return -EINVAL;
> > + }
> > +
> > + counter_val = time_set - time_now;
> > }
> >
> > sun6i_rtc_setaie(0, chip);
> > writel(0, chip->base + SUN6I_ALRM_COUNTER);
> > + if (chip->flags & RTC_LINEAR_DAY)
> > + writel(0, chip->base + SUN6I_ALRM_COUNTER_HMS);
> > usleep_range(100, 300);
> >
> > - writel(time_set - time_now, chip->base +
[linux-sunxi] Re: [PATCH v8 05/11] rtc: sun6i: Add support for broken-down alarm registers
Hi Andre!
Dne petek, 23. julij 2021 ob 17:38:32 CEST je Andre Przywara napisal(a):
> Newer versions of the Allwinner RTC, for instance as found in the H616
> SoC, not only store the current day as a linear number, but also change
> the way the alarm is handled: There are now two registers, that
> explicitly store the wakeup time, in the same format as the current
> time.
>
> Add support for that variant by writing the requested wakeup time
> directly into the registers, instead of programming the seconds left, as
> the old SoCs required.
>
> Signed-off-by: Andre Przywara
> ---
> drivers/rtc/rtc-sun6i.c | 60 +
> 1 file changed, 43 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/rtc/rtc-sun6i.c b/drivers/rtc/rtc-sun6i.c
> index a02be8bca6f3..f0ee20bfdccb 100644
> --- a/drivers/rtc/rtc-sun6i.c
> +++ b/drivers/rtc/rtc-sun6i.c
> @@ -48,7 +48,8 @@
>
> /* Alarm 0 (counter) */
> #define SUN6I_ALRM_COUNTER 0x0020
> -#define SUN6I_ALRM_CUR_VAL 0x0024
> +/* This holds the remaining alarm seconds on older SoCs (current value) */
> +#define SUN6I_ALRM_COUNTER_HMS 0x0024
> #define SUN6I_ALRM_EN0x0028
> #define SUN6I_ALRM_EN_CNT_EN BIT(0)
> #define SUN6I_ALRM_IRQ_EN0x002c
> @@ -523,32 +524,57 @@ static int sun6i_rtc_setalarm(struct device *dev,
struct rtc_wkalrm *wkalrm)
> struct sun6i_rtc_dev *chip = dev_get_drvdata(dev);
> struct rtc_time *alrm_tm = >time;
> struct rtc_time tm_now;
> - time64_t time_now, time_set;
> + time64_t time_set;
> + u32 counter_val, counter_val_hms;
> int ret;
>
> - ret = sun6i_rtc_gettime(dev, _now);
> - if (ret < 0) {
> - dev_err(dev, "Error in getting time\n");
> - return -EINVAL;
> - }
> -
> time_set = rtc_tm_to_time64(alrm_tm);
> - time_now = rtc_tm_to_time64(_now);
> - if (time_set <= time_now) {
> - dev_err(dev, "Date to set in the past\n");
> - return -EINVAL;
> - }
>
> - if ((time_set - time_now) > U32_MAX) {
> - dev_err(dev, "Date too far in the future\n");
> - return -EINVAL;
> + if (chip->flags & RTC_LINEAR_DAY) {
> + time64_t seconds;
> +
> + /*
> + * The alarm registers hold the actual alarm time,
encoded
> + * in the same way (linear day + HMS) as the current
time.
> + */
> + counter_val_hms = SUN6I_TIME_SET_SEC_VALUE(alrm_tm-
>tm_sec) |
> +
SUN6I_TIME_SET_MIN_VALUE(alrm_tm->tm_min) |
> +
SUN6I_TIME_SET_HOUR_VALUE(alrm_tm->tm_hour);
> + seconds = mktime64(alrm_tm->tm_year + 1900, alrm_tm-
>tm_mon,
> +alrm_tm->tm_mday, 0, 0, 0);
While above line should work, it's confusing why you're adding 1900 to years.
In my opinion it would be better to use same trick you used in patch 4 -
rtc_tm_to_time64() with hours, minutes and seconds set to 0. Or maybe you
don't even need to do that, since division by SECS_PER_DAY will round down
anyway. In such way you hide RTC internal representation detail which doesn't
need to be exposed here.
Once fixed:
Reviewed by: Jernej Skrabec
Best regards,
Jernej
> + counter_val = div_u64(seconds, SECS_PER_DAY);
> + } else {
> + /* The alarm register holds the number of seconds left.
*/
> + time64_t time_now;
> +
> + ret = sun6i_rtc_gettime(dev, _now);
> + if (ret < 0) {
> + dev_err(dev, "Error in getting time\n");
> + return -EINVAL;
> + }
> +
> + time_now = rtc_tm_to_time64(_now);
> + if (time_set <= time_now) {
> + dev_err(dev, "Date to set in the past\n");
> + return -EINVAL;
> + }
> + if ((time_set - time_now) > U32_MAX) {
> + dev_err(dev, "Date too far in the future\n");
> + return -EINVAL;
> + }
> +
> + counter_val = time_set - time_now;
> }
>
> sun6i_rtc_setaie(0, chip);
> writel(0, chip->base + SUN6I_ALRM_COUNTER);
> + if (chip->flags & RTC_LINEAR_DAY)
> + writel(0, chip->base + SUN6I_ALRM_COUNTER_HMS);
> usleep_range(100, 300);
>
> - writel(time_set - time_now, chip->base + SUN6I_ALRM_COUNTER);
> + writel(counter_val, chip->base + SUN6I_ALRM_COUNTER);
> + if (chip->flags & RTC_LINEAR_DAY)
> + writel(counter_val_hms, chip->base +
SUN6I_ALRM_COUNTER_HMS);
> chip->alarm = time_set;
>
> sun6i_rtc_setaie(wkalrm->enabled, chip);
> --
> 2.17.6
>
>
--
You received this message because you are subscribed to the Google Groups
"linux-sunxi" group.
To unsubscribe from this group and
