linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Andre Przywara <andre.przywara@arm.com>
To: "Jernej Škrabec" <jernej.skrabec@gmail.com>
Cc: Maxime Ripard <mripard@kernel.org>, Chen-Yu Tsai <wens@csie.org>,
	Rob Herring <robh@kernel.org>, Icenowy Zheng <icenowy@aosc.io>,
	Samuel Holland <samuel@sholland.org>,
	linux-arm-kernel@lists.infradead.org,
	linux-sunxi@googlegroups.com, linux-sunxi@lists.linux.dev,
	linux-kernel@vger.kernel.org, Ondrej Jirman <megous@megous.com>,
	Alessandro Zummo <a.zummo@towertech.it>,
	Alexandre Belloni <alexandre.belloni@bootlin.com>,
	linux-rtc@vger.kernel.org
Subject: Re: [PATCH v8 05/11] rtc: sun6i: Add support for broken-down alarm registers
Date: Mon, 2 Aug 2021 01:39:02 +0100	[thread overview]
Message-ID: <20210802013902.7164876f@slackpad.fritz.box> (raw)
In-Reply-To: <35374855.gLGPhMbAMS@kista>

On Sun, 25 Jul 2021 08:11:49 +0200
Jernej Škrabec <jernej.skrabec@gmail.com> 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 <andre.przywara@arm.com>
> > ---
> >  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 = &wkalrm->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, &tm_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(&tm_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 <jernej.skrabec@gmail.com>

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, &tm_now);
> > +		if (ret < 0) {
> > +			dev_err(dev, "Error in getting time\n");
> > +			return -EINVAL;
> > +		}
> > +
> > +		time_now = rtc_tm_to_time64(&tm_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
> > 
> >   
> 
> 
> 


  reply	other threads:[~2021-08-02  0:39 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-23 15:38 [PATCH v8 00/11] arm64: sunxi: Initial Allwinner H616 SoC support Andre Przywara
2021-07-23 15:38 ` [PATCH v8 01/11] dt-bindings: mfd: axp20x: Add AXP305 compatible (plus optional IRQ) Andre Przywara
2021-08-16 12:39   ` Lee Jones
2021-07-23 15:38 ` [PATCH v8 02/11] dt-bindings: rtc: sun6i: Add H616 compatible string Andre Przywara
2021-07-23 22:34   ` Rob Herring
2021-07-26 14:41   ` Maxime Ripard
2021-08-02  0:39     ` Andre Przywara
2021-08-17  7:38       ` Maxime Ripard
2021-08-17  8:13         ` Alexandre Belloni
2021-08-19  7:56           ` Maxime Ripard
2021-08-18  9:04         ` Andre Przywara
2021-08-20  3:57           ` Samuel Holland
2021-09-01  7:21           ` Maxime Ripard
2021-07-23 15:38 ` [PATCH v8 03/11] rtc: sun6i: Fix time overflow handling Andre Przywara
2021-07-25  5:44   ` Jernej Škrabec
2021-07-23 15:38 ` [PATCH v8 04/11] rtc: sun6i: Add support for linear day storage Andre Przywara
2021-07-25  5:51   ` Jernej Škrabec
2021-07-23 15:38 ` [PATCH v8 05/11] rtc: sun6i: Add support for broken-down alarm registers Andre Przywara
2021-07-25  6:11   ` Jernej Škrabec
2021-08-02  0:39     ` Andre Przywara [this message]
2021-07-23 15:38 ` [PATCH v8 06/11] rtc: sun6i: Add support for RTCs without external LOSCs Andre Przywara
2021-07-25  6:18   ` Jernej Škrabec
2021-07-23 15:38 ` [PATCH v8 07/11] rtc: sun6i: Add Allwinner H616 support Andre Przywara
2021-07-25  6:19   ` Jernej Škrabec
2021-07-23 15:38 ` [PATCH v8 08/11] arm64: dts: allwinner: Add Allwinner H616 .dtsi file Andre Przywara
2021-07-23 15:38 ` [PATCH v8 09/11] dt-bindings: arm: sunxi: Add two H616 board compatible strings Andre Przywara
2021-07-23 15:38 ` [PATCH v8 10/11] arm64: dts: allwinner: h616: Add OrangePi Zero 2 board support Andre Przywara
2021-07-23 15:38 ` [PATCH v8 11/11] arm64: dts: allwinner: h616: Add X96 Mate TV box support Andre Przywara
2021-07-25 16:41 ` [PATCH v8 00/11] arm64: sunxi: Initial Allwinner H616 SoC support Icenowy Zheng
2021-07-26 14:52 ` Maxime Ripard
2021-08-02  0:38   ` Andre Przywara
2021-08-17  7:30     ` Maxime Ripard

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210802013902.7164876f@slackpad.fritz.box \
    --to=andre.przywara@arm.com \
    --cc=a.zummo@towertech.it \
    --cc=alexandre.belloni@bootlin.com \
    --cc=icenowy@aosc.io \
    --cc=jernej.skrabec@gmail.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-rtc@vger.kernel.org \
    --cc=linux-sunxi@googlegroups.com \
    --cc=linux-sunxi@lists.linux.dev \
    --cc=megous@megous.com \
    --cc=mripard@kernel.org \
    --cc=robh@kernel.org \
    --cc=samuel@sholland.org \
    --cc=wens@csie.org \
    --subject='Re: [PATCH v8 05/11] rtc: sun6i: Add support for broken-down alarm registers' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).