In order to conserve battery energy, during the PS operation,
it is expected that the supply for the battery-powered domain
to be switched from the battery (VCC_PSBATT) to (VCC_PSAUX) and
automatically be switched back to battery when VCC_PSAUX voltage
drops below a limit, doing so prevents the logic within
the battery-powered domain from functioning incorrectly.
This patch enables that feature.
Signed-off-by: Anurag Kumar Vulisha <[email protected]>
---
drivers/rtc/rtc-zynqmp.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/rtc/rtc-zynqmp.c b/drivers/rtc/rtc-zynqmp.c
index 8b28762..6adb603 100644
--- a/drivers/rtc/rtc-zynqmp.c
+++ b/drivers/rtc/rtc-zynqmp.c
@@ -45,6 +45,7 @@
#define RTC_INT_SEC BIT(0)
#define RTC_INT_ALRM BIT(1)
#define RTC_OSC_EN BIT(24)
+#define RTC_BATT_EN BIT(31)
#define RTC_CALIB_DEF 0x198233
#define RTC_CALIB_MASK 0x1FFFFF
@@ -122,6 +123,13 @@ static int xlnx_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
static void xlnx_init_rtc(struct xlnx_rtc_dev *xrtcdev, u32 calibval)
{
+ u32 rtc_ctrl;
+
+ /* Enable RTC switch to battery when VCC_PSAUX is not available */
+ rtc_ctrl = readl(xrtcdev->reg_base + RTC_CTRL);
+ rtc_ctrl |= RTC_BATT_EN;
+ writel(rtc_ctrl, xrtcdev->reg_base + RTC_CTRL);
+
/*
* Based on crystal freq of 33.330 KHz
* set the seconds counter and enable, set fractions counter
--
2.1.2
It is suggested to programe CALIB_WRITE register with the calibration
value before updating the SET_TIME_WRITE register, doing so will
clear the Tick Counter and force the next second to be signaled
exactly in 1 second.
This patch updates the same.
Signed-off-by: Anurag Kumar Vulisha <[email protected]>
---
drivers/rtc/rtc-zynqmp.c | 21 ++++++++++++++-------
1 file changed, 14 insertions(+), 7 deletions(-)
diff --git a/drivers/rtc/rtc-zynqmp.c b/drivers/rtc/rtc-zynqmp.c
index 6adb603..f87f971 100644
--- a/drivers/rtc/rtc-zynqmp.c
+++ b/drivers/rtc/rtc-zynqmp.c
@@ -56,6 +56,7 @@ struct xlnx_rtc_dev {
void __iomem *reg_base;
int alarm_irq;
int sec_irq;
+ int calibval;
};
static int xlnx_rtc_set_time(struct device *dev, struct rtc_time *tm)
@@ -68,6 +69,13 @@ static int xlnx_rtc_set_time(struct device *dev, struct rtc_time *tm)
if (new_time > RTC_SEC_MAX_VAL)
return -EINVAL;
+ /*
+ * Writing into calibration register will clear the Tick Counter and
+ * force the next second to be signaled exactly in 1 second period
+ */
+ xrtcdev->calibval &= RTC_CALIB_MASK;
+ writel(xrtcdev->calibval, (xrtcdev->reg_base + RTC_CALIB_WR));
+
writel(new_time, xrtcdev->reg_base + RTC_SET_TM_WR);
return 0;
@@ -121,7 +129,7 @@ static int xlnx_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
return 0;
}
-static void xlnx_init_rtc(struct xlnx_rtc_dev *xrtcdev, u32 calibval)
+static void xlnx_init_rtc(struct xlnx_rtc_dev *xrtcdev)
{
u32 rtc_ctrl;
@@ -136,8 +144,8 @@ static void xlnx_init_rtc(struct xlnx_rtc_dev *xrtcdev, u32 calibval)
* to default value suggested as per design spec
* to correct RTC delay in frequency over period of time.
*/
- calibval &= RTC_CALIB_MASK;
- writel(calibval, (xrtcdev->reg_base + RTC_CALIB_WR));
+ xrtcdev->calibval &= RTC_CALIB_MASK;
+ writel(xrtcdev->calibval, (xrtcdev->reg_base + RTC_CALIB_WR));
}
static const struct rtc_class_ops xlnx_rtc_ops = {
@@ -174,7 +182,6 @@ static int xlnx_rtc_probe(struct platform_device *pdev)
struct xlnx_rtc_dev *xrtcdev;
struct resource *res;
int ret;
- unsigned int calibvalue;
xrtcdev = devm_kzalloc(&pdev->dev, sizeof(*xrtcdev), GFP_KERNEL);
if (!xrtcdev)
@@ -215,11 +222,11 @@ static int xlnx_rtc_probe(struct platform_device *pdev)
}
ret = of_property_read_u32(pdev->dev.of_node, "calibration",
- &calibvalue);
+ &xrtcdev->calibval);
if (ret)
- calibvalue = RTC_CALIB_DEF;
+ xrtcdev->calibval = RTC_CALIB_DEF;
- xlnx_init_rtc(xrtcdev, calibvalue);
+ xlnx_init_rtc(xrtcdev);
device_init_wakeup(&pdev->dev, 1);
--
2.1.2
We programe RTC time using SET_TIME_WRITE register and read the RTC
current time using CURRENT_TIME register. When we set the time by
writing into SET_TIME_WRITE Register and immediately try to read the
rtc time from CURRENT_TIME register, the previous old value is
returned instead of the new loaded time. This is because RTC takes
nearly 1 sec to update the new loaded value into the CURRENT_TIME
register. This behaviour is expected in our RTC IP.
This patch updates the driver to read the current time from SET_TIME_WRITE
register instead of CURRENT_TIME when rtc time is requested within an 1sec
period after setting the RTC time. Doing so will ensure the correct time
is given to the user.
Since there is an delay of 1sec in updating the CURRENT_TIME we are loading
set time +1sec while programming the SET_TIME_WRITE register, doing this
will give correct time without any delay when read from CURRENT_TIME.
This patch updates the above said.
Signed-off-by: Anurag Kumar Vulisha <[email protected]>
---
drivers/rtc/rtc-zynqmp.c | 40 ++++++++++++++++++++++++++++++++++++++--
1 file changed, 38 insertions(+), 2 deletions(-)
diff --git a/drivers/rtc/rtc-zynqmp.c b/drivers/rtc/rtc-zynqmp.c
index f87f971..b98cebe 100644
--- a/drivers/rtc/rtc-zynqmp.c
+++ b/drivers/rtc/rtc-zynqmp.c
@@ -57,6 +57,7 @@ struct xlnx_rtc_dev {
int alarm_irq;
int sec_irq;
int calibval;
+ int time_updated;
};
static int xlnx_rtc_set_time(struct device *dev, struct rtc_time *tm)
@@ -64,6 +65,12 @@ static int xlnx_rtc_set_time(struct device *dev, struct rtc_time *tm)
struct xlnx_rtc_dev *xrtcdev = dev_get_drvdata(dev);
unsigned long new_time;
+ /*
+ * The value written will be updated after 1 sec into the
+ * seconds read register, so we need to program time +1 sec
+ * to get the correct time on read.
+ */
+ tm->tm_sec += 1;
new_time = rtc_tm_to_time64(tm);
if (new_time > RTC_SEC_MAX_VAL)
@@ -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);
+ 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
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
On 12/04/2016 at 17:45:44 +0530, Anurag Kumar Vulisha wrote :
> In order to conserve battery energy, during the PS operation,
> it is expected that the supply for the battery-powered domain
> to be switched from the battery (VCC_PSBATT) to (VCC_PSAUX) and
> automatically be switched back to battery when VCC_PSAUX voltage
> drops below a limit, doing so prevents the logic within
> the battery-powered domain from functioning incorrectly.
>
> This patch enables that feature.
>
> Signed-off-by: Anurag Kumar Vulisha <[email protected]>
> ---
> drivers/rtc/rtc-zynqmp.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
Applied, thanks.
--
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
On 12/04/2016 at 17:45:45 +0530, Anurag Kumar Vulisha wrote :
> It is suggested to programe CALIB_WRITE register with the calibration
> value before updating the SET_TIME_WRITE register, doing so will
> clear the Tick Counter and force the next second to be signaled
> exactly in 1 second.
>
> This patch updates the same.
>
> Signed-off-by: Anurag Kumar Vulisha <[email protected]>
> ---
> drivers/rtc/rtc-zynqmp.c | 21 ++++++++++++++-------
> 1 file changed, 14 insertions(+), 7 deletions(-)
>
Applied, thanks.
--
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
Hi Alexandre,
> -----Original Message-----
> From: Alexandre Belloni [mailto:[email protected]]
> Sent: Wednesday, April 20, 2016 4:01 AM
> To: Anurag Kumar Vulisha <[email protected]>
> Cc: Alessandro Zummo <[email protected]>; Soren Brinkmann
> <[email protected]>; Michal Simek <[email protected]>; rtc-
> [email protected]; [email protected]; linux-
> [email protected]; Punnaiah Choudary Kalluri <[email protected]>;
> Anirudha Sarangi <[email protected]>; Srikanth Vemula
> <[email protected]>; Anurag Kumar Vulisha <[email protected]>
> 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
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
Hi Alexandre,
> -----Original Message-----
> From: Alexandre Belloni [mailto:[email protected]]
> Sent: Wednesday, April 20, 2016 1:28 PM
> To: Anurag Kumar Vulisha <[email protected]>
> Cc: Alessandro Zummo <[email protected]>; Soren Brinkmann
> <[email protected]>; Michal Simek <[email protected]>; rtc-
> [email protected]; [email protected]; linux-
> [email protected]; Punnaiah Choudary Kalluri <[email protected]>;
> Anirudha Sarangi <[email protected]>; Srikanth Vemula
> <[email protected]>; Srinivas Goud <[email protected]>
> 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.
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
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:[email protected]]
> Sent: Wednesday, April 20, 2016 5:32 PM
> To: Anurag Kumar Vulisha <[email protected]>
> Cc: Alessandro Zummo <[email protected]>; Soren Brinkmann
> <[email protected]>; Michal Simek <[email protected]>; rtc-
> [email protected]; [email protected]; linux-
> [email protected]; Punnaiah Choudary Kalluri <[email protected]>;
> Anirudha Sarangi <[email protected]>; Srikanth Vemula
> <[email protected]>; Srinivas Goud <[email protected]>
> 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