Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1382875 > unrolled thread

Re: [PATCH 3/3] RTC: Update seconds time programming logic

Started byAlexandre Belloni <alexandre.belloni@free-electrons.com>
First post2016-04-20 00:40 +0200
Last post2016-04-20 16:00 +0200
Articles 6 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 3/3] RTC: Update seconds time programming logic Alexandre Belloni <alexandre.belloni@free-electrons.com> - 2016-04-20 00:40 +0200
    RE: [PATCH 3/3] RTC: Update seconds time programming logic Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com> - 2016-04-20 09:20 +0200
      Re: [PATCH 3/3] RTC: Update seconds time programming logic Alexandre Belloni <alexandre.belloni@free-electrons.com> - 2016-04-20 10:00 +0200
        RE: [PATCH 3/3] RTC: Update seconds time programming logic Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com> - 2016-04-20 12:40 +0200
          Re: [PATCH 3/3] RTC: Update seconds time programming logic Alexandre Belloni <alexandre.belloni@free-electrons.com> - 2016-04-20 14:10 +0200
            RE: [PATCH 3/3] RTC: Update seconds time programming logic Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com> - 2016-04-20 16:00 +0200

#1382875 — Re: [PATCH 3/3] RTC: Update seconds time programming logic

FromAlexandre Belloni <alexandre.belloni@free-electrons.com>
Date2016-04-20 00:40 +0200
SubjectRe: [PATCH 3/3] RTC: Update seconds time programming logic
Message-ID<rpMtk-61D-21@gated-at.bofh.it>
Hi,

Please use rtc: zynqmp in your subject line.

On 12/04/2016 at 17:45:46 +0530, Anurag Kumar Vulisha wrote :
> @@ -78,6 +85,17 @@ static int xlnx_rtc_set_time(struct device *dev, struct rtc_time *tm)
>  
>  	writel(new_time, xrtcdev->reg_base + RTC_SET_TM_WR);
>  
> +	/*
> +	 * Clear the rtc interrupt status register after setting the
> +	 * time. During a read_time function, the code should read the
> +	 * RTC_INT_STATUS register and if bit 0 is still 0, it means
> +	 * that one second has not elapsed yet since RTC was set and
> +	 * the current time should be read from SET_TIME_READ register;
> +	 * otherwise, CURRENT_TIME register is read to report the time
> +	 */
> +	writel(RTC_INT_SEC | RTC_INT_ALRM, xrtcdev->reg_base + RTC_INT_STS);

You probably shouldn't clear RTC_INT_ALRM here but it should be done in
the init and when enabling/disabling alarm. Or maybe easier would be to
ignore RTC_INT_ALRM in xlnx_rtc_interrupt when it is not set in
RTC_INT_MASK.

Or, instead of using interrupts, can't you simply read RTC_INT_STS in
xlnx_rtc_read_time()? Is it updated even when the interrupt is not
enabled?

> +	xrtcdev->time_updated = 0;
> +
>  	return 0;
>  }
>  
> @@ -85,7 +103,17 @@ static int xlnx_rtc_read_time(struct device *dev, struct rtc_time *tm)
>  {
>  	struct xlnx_rtc_dev *xrtcdev = dev_get_drvdata(dev);
>  
> -	rtc_time64_to_tm(readl(xrtcdev->reg_base + RTC_CUR_TM), tm);
> +	if (xrtcdev->time_updated == 0) {
> +		/*
> +		 * Time written in SET_TIME_WRITE has not yet updated into
> +		 * the seconds read register, so read the time from the
> +		 * SET_TIME_WRITE instead of CURRENT_TIME register.
> +		 */
> +		rtc_time64_to_tm(readl(xrtcdev->reg_base + RTC_SET_TM_RD), tm);
> +		tm->tm_sec -= 1;
> +	} else {
> +		rtc_time64_to_tm(readl(xrtcdev->reg_base + RTC_CUR_TM), tm);
> +	}
>  
>  	return rtc_valid_tm(tm);
>  }
> @@ -133,6 +161,9 @@ static void xlnx_init_rtc(struct xlnx_rtc_dev *xrtcdev)
>  {
>  	u32 rtc_ctrl;
>  
> +	/* Enable RTC SEC interrupts */
> +	writel(RTC_INT_SEC, xrtcdev->reg_base + RTC_INT_EN);
> +
>  	/* Enable RTC switch to battery when VCC_PSAUX is not available */
>  	rtc_ctrl = readl(xrtcdev->reg_base + RTC_CTRL);
>  	rtc_ctrl |= RTC_BATT_EN;
> @@ -169,8 +200,13 @@ static irqreturn_t xlnx_rtc_interrupt(int irq, void *id)
>  	/* Clear interrupt */
>  	writel(status, xrtcdev->reg_base + RTC_INT_STS);
>  
> -	if (status & RTC_INT_SEC)
> +	if (status & RTC_INT_SEC) {
> +		if (xrtcdev->time_updated == 0) {
> +			/* RTC updated the seconds read register */
> +			xrtcdev->time_updated = 1;
> +		}
>  		rtc_update_irq(xrtcdev->rtc, 1, RTC_IRQF | RTC_UF);
> +	}
>  	if (status & RTC_INT_ALRM)
>  		rtc_update_irq(xrtcdev->rtc, 1, RTC_IRQF | RTC_AF);
>  
> -- 
> 2.1.2
> 

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[toc] | [next] | [standalone]

#1383101

FromAnurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com>
Date2016-04-20 09:20 +0200
Message-ID<rpUAy-3Ut-7@gated-at.bofh.it>
In reply to#1382875
Hi Alexandre,

> -----Original Message-----
> From: Alexandre Belloni [mailto:alexandre.belloni@free-electrons.com]
> Sent: Wednesday, April 20, 2016 4:01 AM
> To: Anurag Kumar Vulisha <anuragku@xilinx.com>
> Cc: Alessandro Zummo <a.zummo@towertech.it>; Soren Brinkmann
> <sorenb@xilinx.com>; Michal Simek <michals@xilinx.com>; rtc-
> linux@googlegroups.com; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org; Punnaiah Choudary Kalluri <punnaia@xilinx.com>;
> Anirudha Sarangi <anirudh@xilinx.com>; Srikanth Vemula
> <svemula@xilinx.com>; Anurag Kumar Vulisha <anuragku@xilinx.com>
> Subject: Re: [PATCH 3/3] RTC: Update seconds time programming logic
> 
> 
> Hi,
> 
> Please use rtc: zynqmp in your subject line.
> 
> On 12/04/2016 at 17:45:46 +0530, Anurag Kumar Vulisha wrote :
> > @@ -78,6 +85,17 @@ static int xlnx_rtc_set_time(struct device *dev,
> > struct rtc_time *tm)
> >
> >  	writel(new_time, xrtcdev->reg_base + RTC_SET_TM_WR);
> >
> > +	/*
> > +	 * Clear the rtc interrupt status register after setting the
> > +	 * time. During a read_time function, the code should read the
> > +	 * RTC_INT_STATUS register and if bit 0 is still 0, it means
> > +	 * that one second has not elapsed yet since RTC was set and
> > +	 * the current time should be read from SET_TIME_READ register;
> > +	 * otherwise, CURRENT_TIME register is read to report the time
> > +	 */
> > +	writel(RTC_INT_SEC | RTC_INT_ALRM, xrtcdev->reg_base +
> RTC_INT_STS);
> 
> You probably shouldn't clear RTC_INT_ALRM here but it should be done in
> the init and when enabling/disabling alarm. Or maybe easier would be to
> ignore RTC_INT_ALRM in xlnx_rtc_interrupt when it is not set in
> RTC_INT_MASK.
>

Thanks for reviewing the patch. I will remove this clearing  RTC_INT_ALRM interrupt logic
and send the next version of the patch.
 
> Or, instead of using interrupts, can't you simply read RTC_INT_STS in
> xlnx_rtc_read_time()? Is it updated even when the interrupt is not enabled?
> 

The reason for me keeping this logic is, our RTC controller updates the read register after 1 sec
delay, so when read , it gives 1 sec delay(correct time - 1 sec). So to avoid that we are programming
load time + 1sec into the write register. So when read we would be getting the correct time without
any delay. If any request comes from user to read time before RTC updating the read register, we
need to give the previous loaded time instead of giving the time from the read register. 
For doing the above said, we are relaying on seconds interrupt in  RTC_INT_STS register. We
Clear the RTC_INT_STS register while programming the time into the write register . If we get a
request from user to read time within the 1 sec period i.e before the RTC_INT_SEC interrupt bit
is set in RTC_INT_STS, we need to give the previous loaded time.
This should be done if time is requested from user space within  1 sec period after writing time, after
the 1 sec  delay if user requested the time , we can  give the give time from read register . This is because
the correct time is being updated in the read register after 1 sec delay. For this logic to happen we are
depending on  xrtcdev->time_updated  variable to get updated after the very fist RTC_INT_SEC interrupt
occurance in the interrupt handler.
Since we are relaying on  xrtcdev->time_updated  to get updated from interrupt handler, I think reading
the RTC_INT_STS in xlnx_rtc_read_time() is not helpful.

Thanks,
Anurag Kumar V
  
> > +	xrtcdev->time_updated = 0;
> > +
> >  	return 0;
> >  }
> >
> > @@ -85,7 +103,17 @@ static int xlnx_rtc_read_time(struct device *dev,
> > struct rtc_time *tm)  {
> >  	struct xlnx_rtc_dev *xrtcdev = dev_get_drvdata(dev);
> >
> > -	rtc_time64_to_tm(readl(xrtcdev->reg_base + RTC_CUR_TM), tm);
> > +	if (xrtcdev->time_updated == 0) {
> > +		/*
> > +		 * Time written in SET_TIME_WRITE has not yet updated into
> > +		 * the seconds read register, so read the time from the
> > +		 * SET_TIME_WRITE instead of CURRENT_TIME register.
> > +		 */
> > +		rtc_time64_to_tm(readl(xrtcdev->reg_base +
> RTC_SET_TM_RD), tm);
> > +		tm->tm_sec -= 1;
> > +	} else {
> > +		rtc_time64_to_tm(readl(xrtcdev->reg_base +
> RTC_CUR_TM), tm);
> > +	}
> >
> >  	return rtc_valid_tm(tm);
> >  }
> > @@ -133,6 +161,9 @@ static void xlnx_init_rtc(struct xlnx_rtc_dev
> > *xrtcdev)  {
> >  	u32 rtc_ctrl;
> >
> > +	/* Enable RTC SEC interrupts */
> > +	writel(RTC_INT_SEC, xrtcdev->reg_base + RTC_INT_EN);
> > +
> >  	/* Enable RTC switch to battery when VCC_PSAUX is not available */
> >  	rtc_ctrl = readl(xrtcdev->reg_base + RTC_CTRL);
> >  	rtc_ctrl |= RTC_BATT_EN;
> > @@ -169,8 +200,13 @@ static irqreturn_t xlnx_rtc_interrupt(int irq, void
> *id)
> >  	/* Clear interrupt */
> >  	writel(status, xrtcdev->reg_base + RTC_INT_STS);
> >
> > -	if (status & RTC_INT_SEC)
> > +	if (status & RTC_INT_SEC) {
> > +		if (xrtcdev->time_updated == 0) {
> > +			/* RTC updated the seconds read register */
> > +			xrtcdev->time_updated = 1;
> > +		}
> >  		rtc_update_irq(xrtcdev->rtc, 1, RTC_IRQF | RTC_UF);
> > +	}
> >  	if (status & RTC_INT_ALRM)
> >  		rtc_update_irq(xrtcdev->rtc, 1, RTC_IRQF | RTC_AF);
> >
> > --
> > 2.1.2
> >
> 
> --
> Alexandre Belloni, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com

[toc] | [prev] | [next] | [standalone]

#1383127

FromAlexandre Belloni <alexandre.belloni@free-electrons.com>
Date2016-04-20 10:00 +0200
Message-ID<rpVdg-4bE-13@gated-at.bofh.it>
In reply to#1383101
On 20/04/2016 at 07:10:22 +0000, Anurag Kumar Vulisha wrote :
> The reason for me keeping this logic is, our RTC controller updates the read register after 1 sec
> delay, so when read , it gives 1 sec delay(correct time - 1 sec). So to avoid that we are programming
> load time + 1sec into the write register. So when read we would be getting the correct time without
> any delay. If any request comes from user to read time before RTC updating the read register, we
> need to give the previous loaded time instead of giving the time from the read register. 
> For doing the above said, we are relaying on seconds interrupt in  RTC_INT_STS register. We
> Clear the RTC_INT_STS register while programming the time into the write register . If we get a
> request from user to read time within the 1 sec period i.e before the RTC_INT_SEC interrupt bit
> is set in RTC_INT_STS, we need to give the previous loaded time.
> This should be done if time is requested from user space within  1 sec period after writing time, after
> the 1 sec  delay if user requested the time , we can  give the give time from read register . This is because
> the correct time is being updated in the read register after 1 sec delay. For this logic to happen we are
> depending on  xrtcdev->time_updated  variable to get updated after the very fist RTC_INT_SEC interrupt
> occurance in the interrupt handler.
> Since we are relaying on  xrtcdev->time_updated  to get updated from interrupt handler, I think reading
> the RTC_INT_STS in xlnx_rtc_read_time() is not helpful.
> 

Yeas, I understood that. But my question was whether the interrupt
handling was necessary at all.
Instead of waiting for an interrupt to set time_updated, can't you
simply read RTC_INT_STS and check for the RTC_INT_SEC bit in
xlnx_rtc_read_time() ?

Something like:

status = readl(xrtcdev->reg_base + RTC_INT_STS)
if (status & RTC_INT_SEC)
	rtc_time64_to_tm(readl(xrtcdev->reg_base + RTC_CUR_TM), tm);
else
	rtc_time64_to_tm(readl(xrtcdev->reg_base + RTC_SET_TM_RD) - 1, tm);

It all depends on whether the RTC_INT_SEC bit in RTC_INT_STS is being
updated even when it is not enabled as an interrupt.

> Thanks,
> Anurag Kumar V
>   
> > > +	xrtcdev->time_updated = 0;
> > > +
> > >  	return 0;
> > >  }
> > >
> > > @@ -85,7 +103,17 @@ static int xlnx_rtc_read_time(struct device *dev,
> > > struct rtc_time *tm)  {
> > >  	struct xlnx_rtc_dev *xrtcdev = dev_get_drvdata(dev);
> > >
> > > -	rtc_time64_to_tm(readl(xrtcdev->reg_base + RTC_CUR_TM), tm);
> > > +	if (xrtcdev->time_updated == 0) {
> > > +		/*
> > > +		 * Time written in SET_TIME_WRITE has not yet updated into
> > > +		 * the seconds read register, so read the time from the
> > > +		 * SET_TIME_WRITE instead of CURRENT_TIME register.
> > > +		 */
> > > +		rtc_time64_to_tm(readl(xrtcdev->reg_base +
> > RTC_SET_TM_RD), tm);
> > > +		tm->tm_sec -= 1;
> > > +	} else {
> > > +		rtc_time64_to_tm(readl(xrtcdev->reg_base +
> > RTC_CUR_TM), tm);
> > > +	}
> > >
> > >  	return rtc_valid_tm(tm);
> > >  }
> > > @@ -133,6 +161,9 @@ static void xlnx_init_rtc(struct xlnx_rtc_dev
> > > *xrtcdev)  {
> > >  	u32 rtc_ctrl;
> > >
> > > +	/* Enable RTC SEC interrupts */
> > > +	writel(RTC_INT_SEC, xrtcdev->reg_base + RTC_INT_EN);
> > > +
> > >  	/* Enable RTC switch to battery when VCC_PSAUX is not available */
> > >  	rtc_ctrl = readl(xrtcdev->reg_base + RTC_CTRL);
> > >  	rtc_ctrl |= RTC_BATT_EN;
> > > @@ -169,8 +200,13 @@ static irqreturn_t xlnx_rtc_interrupt(int irq, void
> > *id)
> > >  	/* Clear interrupt */
> > >  	writel(status, xrtcdev->reg_base + RTC_INT_STS);
> > >
> > > -	if (status & RTC_INT_SEC)
> > > +	if (status & RTC_INT_SEC) {
> > > +		if (xrtcdev->time_updated == 0) {
> > > +			/* RTC updated the seconds read register */
> > > +			xrtcdev->time_updated = 1;
> > > +		}
> > >  		rtc_update_irq(xrtcdev->rtc, 1, RTC_IRQF | RTC_UF);
> > > +	}
> > >  	if (status & RTC_INT_ALRM)
> > >  		rtc_update_irq(xrtcdev->rtc, 1, RTC_IRQF | RTC_AF);
> > >
> > > --
> > > 2.1.2
> > >
> > 
> > --
> > Alexandre Belloni, Free Electrons
> > Embedded Linux, Kernel and Android engineering
> > http://free-electrons.com

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[toc] | [prev] | [next] | [standalone]

#1383250

FromAnurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com>
Date2016-04-20 12:40 +0200
Message-ID<rpXI6-6qG-9@gated-at.bofh.it>
In reply to#1383127
Hi Alexandre,

> -----Original Message-----
> From: Alexandre Belloni [mailto:alexandre.belloni@free-electrons.com]
> Sent: Wednesday, April 20, 2016 1:28 PM
> To: Anurag Kumar Vulisha <anuragku@xilinx.com>
> Cc: Alessandro Zummo <a.zummo@towertech.it>; Soren Brinkmann
> <sorenb@xilinx.com>; Michal Simek <michals@xilinx.com>; rtc-
> linux@googlegroups.com; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org; Punnaiah Choudary Kalluri <punnaia@xilinx.com>;
> Anirudha Sarangi <anirudh@xilinx.com>; Srikanth Vemula
> <svemula@xilinx.com>; Srinivas Goud <sgoud@xilinx.com>
> Subject: Re: [PATCH 3/3] RTC: Update seconds time programming logic
>
> On 20/04/2016 at 07:10:22 +0000, Anurag Kumar Vulisha wrote :
> > The reason for me keeping this logic is, our RTC controller updates
> > the read register after 1 sec delay, so when read , it gives 1 sec
> > delay(correct time - 1 sec). So to avoid that we are programming load
> > time + 1sec into the write register. So when read we would be getting
> > the correct time without any delay. If any request comes from user to read
> time before RTC updating the read register, we need to give the previous
> loaded time instead of giving the time from the read register.
> > For doing the above said, we are relaying on seconds interrupt in
> > RTC_INT_STS register. We Clear the RTC_INT_STS register while
> > programming the time into the write register . If we get a request
> > from user to read time within the 1 sec period i.e before the RTC_INT_SEC
> interrupt bit is set in RTC_INT_STS, we need to give the previous loaded
> time.
> > This should be done if time is requested from user space within  1 sec
> > period after writing time, after the 1 sec  delay if user requested
> > the time , we can  give the give time from read register . This is
> > because the correct time is being updated in the read register after 1
> > sec delay. For this logic to happen we are depending on  xrtcdev-
> >time_updated  variable to get updated after the very fist RTC_INT_SEC
> interrupt occurance in the interrupt handler.
> > Since we are relaying on  xrtcdev->time_updated  to get updated from
> > interrupt handler, I think reading the RTC_INT_STS in xlnx_rtc_read_time()
> is not helpful.
> >
>
> Yeas, I understood that. But my question was whether the interrupt handling
> was necessary at all.
> Instead of waiting for an interrupt to set time_updated, can't you simply read
> RTC_INT_STS and check for the RTC_INT_SEC bit in
> xlnx_rtc_read_time() ?
>
> Something like:
>
> status = readl(xrtcdev->reg_base + RTC_INT_STS) if (status & RTC_INT_SEC)
>       rtc_time64_to_tm(readl(xrtcdev->reg_base + RTC_CUR_TM), tm);
> else
>       rtc_time64_to_tm(readl(xrtcdev->reg_base + RTC_SET_TM_RD) - 1,
> tm);
>
> It all depends on whether the RTC_INT_SEC bit in RTC_INT_STS is being
> updated even when it is not enabled as an interrupt.
>

The above said logic will work if we doesn't clear the RTC_INT_STS register after the
RTC_INT_SEC bit is set, this happens only if interrupts are not enabled. If interrupts
are enabled we will be clearing the RTC_INT_STS every time in the interrupt handler.
And moreover we need to return time from RTC_SET_TM_RD only if time is requested
within 1 sec span after programming the time only , so this is required only for one time.
Since we are clearing the RTC_INT_STS in our interrupt handler, we might end up in giving
the wrong time to the user when requested.So I think this logic might not work.
Please correct me if am wrong.

Thanks,
Anurag Kumar V

> > Thanks,
> > Anurag Kumar V
> >
> > > > +       xrtcdev->time_updated = 0;
> > > > +
> > > >         return 0;
> > > >  }
> > > >
> > > > @@ -85,7 +103,17 @@ static int xlnx_rtc_read_time(struct device
> > > > *dev, struct rtc_time *tm)  {
> > > >         struct xlnx_rtc_dev *xrtcdev = dev_get_drvdata(dev);
> > > >
> > > > -       rtc_time64_to_tm(readl(xrtcdev->reg_base + RTC_CUR_TM), tm);
> > > > +       if (xrtcdev->time_updated == 0) {
> > > > +               /*
> > > > +                * Time written in SET_TIME_WRITE has not yet updated into
> > > > +                * the seconds read register, so read the time from the
> > > > +                * SET_TIME_WRITE instead of CURRENT_TIME register.
> > > > +                */
> > > > +               rtc_time64_to_tm(readl(xrtcdev->reg_base +
> > > RTC_SET_TM_RD), tm);
> > > > +               tm->tm_sec -= 1;
> > > > +       } else {
> > > > +               rtc_time64_to_tm(readl(xrtcdev->reg_base +
> > > RTC_CUR_TM), tm);
> > > > +       }
> > > >
> > > >         return rtc_valid_tm(tm);
> > > >  }
> > > > @@ -133,6 +161,9 @@ static void xlnx_init_rtc(struct xlnx_rtc_dev
> > > > *xrtcdev)  {
> > > >         u32 rtc_ctrl;
> > > >
> > > > +       /* Enable RTC SEC interrupts */
> > > > +       writel(RTC_INT_SEC, xrtcdev->reg_base + RTC_INT_EN);
> > > > +
> > > >         /* Enable RTC switch to battery when VCC_PSAUX is not available */
> > > >         rtc_ctrl = readl(xrtcdev->reg_base + RTC_CTRL);
> > > >         rtc_ctrl |= RTC_BATT_EN;
> > > > @@ -169,8 +200,13 @@ static irqreturn_t xlnx_rtc_interrupt(int
> > > > irq, void
> > > *id)
> > > >         /* Clear interrupt */
> > > >         writel(status, xrtcdev->reg_base + RTC_INT_STS);
> > > >
> > > > -       if (status & RTC_INT_SEC)
> > > > +       if (status & RTC_INT_SEC) {
> > > > +               if (xrtcdev->time_updated == 0) {
> > > > +                       /* RTC updated the seconds read register */
> > > > +                       xrtcdev->time_updated = 1;
> > > > +               }
> > > >                 rtc_update_irq(xrtcdev->rtc, 1, RTC_IRQF | RTC_UF);
> > > > +       }
> > > >         if (status & RTC_INT_ALRM)
> > > >                 rtc_update_irq(xrtcdev->rtc, 1, RTC_IRQF | RTC_AF);
> > > >
> > > > --
> > > > 2.1.2
> > > >
> > >
> > > --
> > > Alexandre Belloni, Free Electrons
> > > Embedded Linux, Kernel and Android engineering
> > > http://free-electrons.com
>
> --
> Alexandre Belloni, Free Electrons
> Embedded Linux, Kernel and Android engineering http://free-electrons.com


This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.

[toc] | [prev] | [next] | [standalone]

#1383328

FromAlexandre Belloni <alexandre.belloni@free-electrons.com>
Date2016-04-20 14:10 +0200
Message-ID<rpZ7b-7FX-1@gated-at.bofh.it>
In reply to#1383250
On 20/04/2016 at 10:31:06 +0000, Anurag Kumar Vulisha wrote :
> > Yeas, I understood that. But my question was whether the interrupt handling
> > was necessary at all.
> > Instead of waiting for an interrupt to set time_updated, can't you simply read
> > RTC_INT_STS and check for the RTC_INT_SEC bit in
> > xlnx_rtc_read_time() ?
> >
> > Something like:
> >
> > status = readl(xrtcdev->reg_base + RTC_INT_STS) if (status & RTC_INT_SEC)
> >       rtc_time64_to_tm(readl(xrtcdev->reg_base + RTC_CUR_TM), tm);
> > else
> >       rtc_time64_to_tm(readl(xrtcdev->reg_base + RTC_SET_TM_RD) - 1,
> > tm);
> >
> > It all depends on whether the RTC_INT_SEC bit in RTC_INT_STS is being
> > updated even when it is not enabled as an interrupt.
> >
> 
> The above said logic will work if we doesn't clear the RTC_INT_STS register after the
> RTC_INT_SEC bit is set, this happens only if interrupts are not enabled. If interrupts
> are enabled we will be clearing the RTC_INT_STS every time in the interrupt handler.
> And moreover we need to return time from RTC_SET_TM_RD only if time is requested
> within 1 sec span after programming the time only , so this is required only for one time.
> Since we are clearing the RTC_INT_STS in our interrupt handler, we might end up in giving
> the wrong time to the user when requested.So I think this logic might not work.
> Please correct me if am wrong.
> 

Simply stop clearing RTC_INT_SEC from RTC_INT_STS in the interrupt
handler.

You can also remove
	if (status & RTC_INT_SEC)
		rtc_update_irq(xrtcdev->rtc, 1, RTC_IRQF | RTC_UF);

as it will not be used. Update interrupts are handled by the core using
timers anyway. And as you can see, there was no code enabling
RTC_INT_SEC in RTC_INT_EN before your patch.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[toc] | [prev] | [next] | [standalone]

#1383412

FromAnurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com>
Date2016-04-20 16:00 +0200
Message-ID<rq0PD-jH-5@gated-at.bofh.it>
In reply to#1383328
Hi Alexandre,

Thanks for reviewing the patch, I will sent v2 with the changes updated.

Thanks,
Anurag Kumar V

> -----Original Message-----
> From: Alexandre Belloni [mailto:alexandre.belloni@free-electrons.com]
> Sent: Wednesday, April 20, 2016 5:32 PM
> To: Anurag Kumar Vulisha <anuragku@xilinx.com>
> Cc: Alessandro Zummo <a.zummo@towertech.it>; Soren Brinkmann
> <sorenb@xilinx.com>; Michal Simek <michals@xilinx.com>; rtc-
> linux@googlegroups.com; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org; Punnaiah Choudary Kalluri <punnaia@xilinx.com>;
> Anirudha Sarangi <anirudh@xilinx.com>; Srikanth Vemula
> <svemula@xilinx.com>; Srinivas Goud <sgoud@xilinx.com>
> Subject: Re: [PATCH 3/3] RTC: Update seconds time programming logic
> 
> On 20/04/2016 at 10:31:06 +0000, Anurag Kumar Vulisha wrote :
> > > Yeas, I understood that. But my question was whether the interrupt
> > > handling was necessary at all.
> > > Instead of waiting for an interrupt to set time_updated, can't you
> > > simply read RTC_INT_STS and check for the RTC_INT_SEC bit in
> > > xlnx_rtc_read_time() ?
> > >
> > > Something like:
> > >
> > > status = readl(xrtcdev->reg_base + RTC_INT_STS) if (status &
> RTC_INT_SEC)
> > >       rtc_time64_to_tm(readl(xrtcdev->reg_base + RTC_CUR_TM), tm);
> > > else
> > >       rtc_time64_to_tm(readl(xrtcdev->reg_base + RTC_SET_TM_RD) - 1,
> > > tm);
> > >
> > > It all depends on whether the RTC_INT_SEC bit in RTC_INT_STS is
> > > being updated even when it is not enabled as an interrupt.
> > >
> >
> > The above said logic will work if we doesn't clear the RTC_INT_STS
> > register after the RTC_INT_SEC bit is set, this happens only if
> > interrupts are not enabled. If interrupts are enabled we will be clearing the
> RTC_INT_STS every time in the interrupt handler.
> > And moreover we need to return time from RTC_SET_TM_RD only if time is
> > requested within 1 sec span after programming the time only , so this is
> required only for one time.
> > Since we are clearing the RTC_INT_STS in our interrupt handler, we
> > might end up in giving the wrong time to the user when requested.So I
> think this logic might not work.
> > Please correct me if am wrong.
> >
> 
> Simply stop clearing RTC_INT_SEC from RTC_INT_STS in the interrupt
> handler.
> 
> You can also remove
> 	if (status & RTC_INT_SEC)
> 		rtc_update_irq(xrtcdev->rtc, 1, RTC_IRQF | RTC_UF);
> 
> as it will not be used. Update interrupts are handled by the core using timers
> anyway. And as you can see, there was no code enabling RTC_INT_SEC in
> RTC_INT_EN before your patch.
> 
> --
> Alexandre Belloni, Free Electrons
> Embedded Linux, Kernel and Android engineering http://free-electrons.com

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web