Alarm interrupt enable register is at offset 0x7, while the time
registers for the alarm follow that. When we program Alarm interrupt
enable prior to programming the time, it is possible that previous
time value could be close or match at the time of alarm enable
resulting in interrupt trigger which is unexpected (and does not match
the time we expect it to trigger).
To prevent this scenario from occuring, program the ALM0_EN bit only
after the alarm time is appropriately programmed.
Ofcourse, I2C programming is non-atomic, so there are loopholes where
the interrupt wont trigger if the time requested is in the past at
the time of programming the ALM0_EN bit. However, we will not have
unexpected interrupts while the time is programmed after the interrupt
are enabled.
Signed-off-by: Nishanth Menon <[email protected]>
---
Changes in v2:
- minor typo fix in comments
- merged up code that I missed committing in
V1: https://patchwork.kernel.org/patch/6245041/
drivers/rtc/rtc-ds1307.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index 4ffabb322a9a..3cd4783375a5 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -742,17 +742,17 @@ static int mcp794xx_set_alarm(struct device *dev, struct rtc_wkalrm *t)
regs[6] &= ~MCP794XX_BIT_ALMX_IF;
/* Set alarm match: second, minute, hour, day, date, month. */
regs[6] |= MCP794XX_MSK_ALMX_MATCH;
-
- if (t->enabled)
- regs[0] |= MCP794XX_BIT_ALM0_EN;
- else
- regs[0] &= ~MCP794XX_BIT_ALM0_EN;
+ /* Disable interrupt. We will not enable until completely programmed */
+ regs[0] &= ~MCP794XX_BIT_ALM0_EN;
ret = ds1307->write_block_data(client, MCP794XX_REG_CONTROL, 10, regs);
if (ret < 0)
return ret;
- return 0;
+ if (!t->enabled)
+ return 0;
+ regs[0] |= MCP794XX_BIT_ALM0_EN;
+ return i2c_smbus_write_byte_data(client, MCP794XX_REG_CONTROL, regs[0]);
}
static int mcp794xx_alarm_irq_enable(struct device *dev, unsigned int enabled)
--
1.7.9.5
Hi,
On 20/04/2015 at 19:51:34 -0500, Nishanth Menon wrote :
> Alarm interrupt enable register is at offset 0x7, while the time
> registers for the alarm follow that. When we program Alarm interrupt
> enable prior to programming the time, it is possible that previous
> time value could be close or match at the time of alarm enable
> resulting in interrupt trigger which is unexpected (and does not match
> the time we expect it to trigger).
>
> To prevent this scenario from occuring, program the ALM0_EN bit only
> after the alarm time is appropriately programmed.
>
> Ofcourse, I2C programming is non-atomic, so there are loopholes where
> the interrupt wont trigger if the time requested is in the past at
> the time of programming the ALM0_EN bit. However, we will not have
> unexpected interrupts while the time is programmed after the interrupt
> are enabled.
>
Do you have more details about the issue you are trying to solve?
Consider the following use case: a platform is setting the RTC alarm
before going to suspend to ram. Before your patch, it may be woken up
quite quickly, before expected. After your patch, it may never wake at
all.
> Signed-off-by: Nishanth Menon <[email protected]>
> ---
> Changes in v2:
> - minor typo fix in comments
> - merged up code that I missed committing in
>
> V1: https://patchwork.kernel.org/patch/6245041/
>
> drivers/rtc/rtc-ds1307.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
> index 4ffabb322a9a..3cd4783375a5 100644
> --- a/drivers/rtc/rtc-ds1307.c
> +++ b/drivers/rtc/rtc-ds1307.c
> @@ -742,17 +742,17 @@ static int mcp794xx_set_alarm(struct device *dev, struct rtc_wkalrm *t)
> regs[6] &= ~MCP794XX_BIT_ALMX_IF;
> /* Set alarm match: second, minute, hour, day, date, month. */
> regs[6] |= MCP794XX_MSK_ALMX_MATCH;
> -
> - if (t->enabled)
> - regs[0] |= MCP794XX_BIT_ALM0_EN;
> - else
> - regs[0] &= ~MCP794XX_BIT_ALM0_EN;
> + /* Disable interrupt. We will not enable until completely programmed */
> + regs[0] &= ~MCP794XX_BIT_ALM0_EN;
>
> ret = ds1307->write_block_data(client, MCP794XX_REG_CONTROL, 10, regs);
> if (ret < 0)
> return ret;
>
> - return 0;
> + if (!t->enabled)
> + return 0;
> + regs[0] |= MCP794XX_BIT_ALM0_EN;
> + return i2c_smbus_write_byte_data(client, MCP794XX_REG_CONTROL, regs[0]);
> }
>
> static int mcp794xx_alarm_irq_enable(struct device *dev, unsigned int enabled)
> --
> 1.7.9.5
>
--
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
On 04/21/2015 06:41 PM, Alexandre Belloni wrote:
> Hi,
>
> On 20/04/2015 at 19:51:34 -0500, Nishanth Menon wrote :
>> Alarm interrupt enable register is at offset 0x7, while the time
>> registers for the alarm follow that. When we program Alarm interrupt
>> enable prior to programming the time, it is possible that previous
>> time value could be close or match at the time of alarm enable
>> resulting in interrupt trigger which is unexpected (and does not match
>> the time we expect it to trigger).
>>
>> To prevent this scenario from occuring, program the ALM0_EN bit only
>> after the alarm time is appropriately programmed.
>>
>> Ofcourse, I2C programming is non-atomic, so there are loopholes where
>> the interrupt wont trigger if the time requested is in the past at
>> the time of programming the ALM0_EN bit. However, we will not have
>> unexpected interrupts while the time is programmed after the interrupt
>> are enabled.
>>
>
> Do you have more details about the issue you are trying to solve?
Testcase: rtctest /dev/rtc0
waveform capture: http://goo.gl/S8Z54x
Corresponding decode: http://pastebin.ubuntu.com/10863880/
>
> Consider the following use case: a platform is setting the RTC alarm
> before going to suspend to ram. Before your patch, it may be woken up
^^ precisely what I am trying to solve.
> quite quickly, before expected. After your patch, it may never wake at
> all.
Why is that so? when set alarm is requested for time X, you want
interrupt at time X, not an interrupt for previous configured RTC
alarm time!
If the time X is > the point when ALM0 is programmed, then you will
get an interrupt.
If you get an interrupt (like my screenshot shows) because the new
value has not yet been programmed (just because we enabled interrupt
before programming time), it is unexpected event and wrong!
Another scenario: Take the following time points A < B < C < D
we program at time (A), an interrupt for time (C).
but at time B, we intiate a new time request for time (D).
if we happen to send the first ALM0EN at time C (before programming
D), you will generate an interrupt, but before the irq handler can
handle (since we are doing burst i2c), we program D which clears the
irq status (as can be seen in waveform).
This does not make sense for a predictable behavior! Yeah, it will
wakeup quickly, but when we go and read irqstatus (ALM0IF), it will be
0 and nothing will get reported to rtc subsystem. So:
a) we woke up at a time not requested - this is wrong
b) our irq handler has nothing to handle! - this is wrong as well.
in short, the behavior you are asking for is quiet the wrong behavior!
>
>
>> Signed-off-by: Nishanth Menon <[email protected]>
>> ---
>> Changes in v2:
>> - minor typo fix in comments
>> - merged up code that I missed committing in
>>
>> V1: https://patchwork.kernel.org/patch/6245041/
>>
>> drivers/rtc/rtc-ds1307.c | 12 ++++++------
>> 1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
>> index 4ffabb322a9a..3cd4783375a5 100644
>> --- a/drivers/rtc/rtc-ds1307.c
>> +++ b/drivers/rtc/rtc-ds1307.c
>> @@ -742,17 +742,17 @@ static int mcp794xx_set_alarm(struct device *dev, struct rtc_wkalrm *t)
>> regs[6] &= ~MCP794XX_BIT_ALMX_IF;
>> /* Set alarm match: second, minute, hour, day, date, month. */
>> regs[6] |= MCP794XX_MSK_ALMX_MATCH;
>> -
>> - if (t->enabled)
>> - regs[0] |= MCP794XX_BIT_ALM0_EN;
>> - else
>> - regs[0] &= ~MCP794XX_BIT_ALM0_EN;
>> + /* Disable interrupt. We will not enable until completely programmed */
>> + regs[0] &= ~MCP794XX_BIT_ALM0_EN;
>>
>> ret = ds1307->write_block_data(client, MCP794XX_REG_CONTROL, 10, regs);
>> if (ret < 0)
>> return ret;
>>
>> - return 0;
>> + if (!t->enabled)
>> + return 0;
>> + regs[0] |= MCP794XX_BIT_ALM0_EN;
>> + return i2c_smbus_write_byte_data(client, MCP794XX_REG_CONTROL, regs[0]);
>> }
>>
>> static int mcp794xx_alarm_irq_enable(struct device *dev, unsigned int enabled)
>> --
>> 1.7.9.5
>>
>
--
Regards,
Nishanth Menon
On 21/04/2015 at 18:58:43 -0500, Nishanth Menon wrote :
> >
> > Consider the following use case: a platform is setting the RTC alarm
> > before going to suspend to ram. Before your patch, it may be woken up
> ^^ precisely what I am trying to solve.
>
> > quite quickly, before expected. After your patch, it may never wake at
> > all.
>
> Why is that so? when set alarm is requested for time X, you want
> interrupt at time X, not an interrupt for previous configured RTC
> alarm time!
>
You expect at least an interrupt.
> If the time X is > the point when ALM0 is programmed, then you will
> get an interrupt.
>
You are eluding my point. What happens if the alarm expires before ALM0
is programmed? Your system is probably dead because it will never wake
up.
> If you get an interrupt (like my screenshot shows) because the new
> value has not yet been programmed (just because we enabled interrupt
> before programming time), it is unexpected event and wrong!
>
> Another scenario: Take the following time points A < B < C < D
> we program at time (A), an interrupt for time (C).
> but at time B, we intiate a new time request for time (D).
> if we happen to send the first ALM0EN at time C (before programming
> D), you will generate an interrupt, but before the irq handler can
> handle (since we are doing burst i2c), we program D which clears the
> irq status (as can be seen in waveform).
>
> This does not make sense for a predictable behavior! Yeah, it will
> wakeup quickly, but when we go and read irqstatus (ALM0IF), it will be
> 0 and nothing will get reported to rtc subsystem. So:
> a) we woke up at a time not requested - this is wrong
> b) our irq handler has nothing to handle! - this is wrong as well.
>
> in short, the behavior you are asking for is quiet the wrong behavior!
>
I agree that an unexpected event is wrong but it is still better than a
dead system. I'm not asking to keep the current behaviour. I'm just
wanting to try to not introduce another race condition.
What about setting ALM0MTH to 0x1F before reading the control registers?
You could also read only the first 3 registers as all the others are
overwritten. And finally, you only need to write 9 bytes instead of 10
(register 0x10 is reserved). While not eliminating it completely, this
will definitively reduce the race condition window.
--
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
On 04/21/2015 08:09 PM, Alexandre Belloni wrote:
> On 21/04/2015 at 18:58:43 -0500, Nishanth Menon wrote :
>>>
>>> Consider the following use case: a platform is setting the RTC alarm
>>> before going to suspend to ram. Before your patch, it may be woken up
>> ^^ precisely what I am trying to solve.
>>
>>> quite quickly, before expected. After your patch, it may never wake at
>>> all.
>>
>> Why is that so? when set alarm is requested for time X, you want
>> interrupt at time X, not an interrupt for previous configured RTC
>> alarm time!
>>
>
> You expect at least an interrupt.
And you will get an interrupt if the event occurs before the i2c burst
starts. Once the i2cburst does start, you are committing to the new time.
>
>> If the time X is > the point when ALM0 is programmed, then you will
>> get an interrupt.
>>
>
> You are eluding my point. What happens if the alarm expires before ALM0
> is programmed? Your system is probably dead because it will never wake
> up.
if the previous alarm does expire before the new alarm is programmed
(before the burst is started), you will get an event - why does this
patch change this? the case you are concerned seem to be the case where
the time you program is in the past at the point when alarm interrupt is
enabled.
Lets see the current behavior(before this patch): time currently
requested is in the past, you are dead anyways. this can happen since
the decision of programming the time is done in userspace(example with
rtcwake) and there are latencies as part of suspend path (file sync)
that can cause the possible cases - consider "rtcwake -s 1" - and for
the moment, lets assume that rtc event is the only wakeup event possible
for the system (to address your point).
a) If the rtcwake progams time before initiating suspend state and prior
to suspend_no_irq is hit event triggers, irq is handled by rtc driver,
and we proceed on to suspend state - but no further irqs can be expected
"dead state"
b) predicted time occurs before suspend_no_irq level is reached, but
before the system actually enters suspend -> event occurs with irq
disabled and depending on smartness of platform code, it can detect that
the wakeup event occurred prior to attempting suspend (hence "dead state")
c) If rtcwake's set_alarm invocation is slowed due to filesystem or
other load activities, the time requested to wakeup is already in the
past, you are in deadstate as the wakeup event has already occurred
prior to entering suspend path.
Note: even with android alarm driver(at least the last time i looked at
it a few years ago), this situation is probably escaping (a), but (b)
and (c) is still real and has to be properly anticipated.
My patch does nothing different to this behavior - only point changed is
*if the previous event does not take place prior to new request being
initiated, then we have to assume that the new time is what we wanted
wakeup event for*. That I strongly believe is necessary.
Point being - using a time for wake attempted to be predicted ahead of
time of suspend path is a a bunch of empirical data analysis - you can
have a dead system today and this code does not change that behavior - I
doubt you can ever get a clean fix without a constant latency.
>> If you get an interrupt (like my screenshot shows) because the new
>> value has not yet been programmed (just because we enabled interrupt
>> before programming time), it is unexpected event and wrong!
>>
>> Another scenario: Take the following time points A < B < C < D
>> we program at time (A), an interrupt for time (C).
>> but at time B, we intiate a new time request for time (D).
>> if we happen to send the first ALM0EN at time C (before programming
>> D), you will generate an interrupt, but before the irq handler can
>> handle (since we are doing burst i2c), we program D which clears the
>> irq status (as can be seen in waveform).
>>
>> This does not make sense for a predictable behavior! Yeah, it will
>> wakeup quickly, but when we go and read irqstatus (ALM0IF), it will be
>> 0 and nothing will get reported to rtc subsystem. So:
>> a) we woke up at a time not requested - this is wrong
>> b) our irq handler has nothing to handle! - this is wrong as well.
>>
>> in short, the behavior you are asking for is quiet the wrong behavior!
>>
>
> I agree that an unexpected event is wrong but it is still better than a
> dead system. I'm not asking to keep the current behaviour. I'm just
> wanting to try to not introduce another race condition.
>
> What about setting ALM0MTH to 0x1F before reading the control registers?
Ummm.... why is that any different? In effect, you are just disabling
the compare logic inside the RTC differently and that said, I am not
entirely sure if I want to test the MCP794xx RTL designer's logic with
invalid dates being programmed for month/date etc.. ;)
> You could also read only the first 3 registers as all the others are
> overwritten. And finally, you only need to write 9 bytes instead of 10
True - had noticed that as well. have'nt had time to optimize the path yet.
> (register 0x10 is reserved). While not eliminating it completely, this
> will definitively reduce the race condition window.
Yep - was planning on fixing 10 to 9 - But that is not related to this
patch anyways. I will do that follow on patch next time I get a break :)
yet another patch that I need to do is to check for OSCRUN before
attempting to program time or alarm -> no point in setting time is
oscillator is non-functional even after waiting for oscillator
stabilization time after ST bit is set.. again.. need to do that
separate from this patch.
--
Regards,
Nishanth Menon
On 21/04/2015 at 20:59:15 -0500, Nishanth Menon wrote :
> >> Why is that so? when set alarm is requested for time X, you want
> >> interrupt at time X, not an interrupt for previous configured RTC
> >> alarm time!
> >>
> >
> > You expect at least an interrupt.
>
> And you will get an interrupt if the event occurs before the i2c burst
> starts. Once the i2cburst does start, you are committing to the new time.
>
You mean that even if ALM0EN is set after ALM0IF was set to 1, it will
trigger the interrupt? I had a look at the MFP output block diagram
would let me think that this is the case. I was thinking otherwise
before. If that is so, then indeed, your patch is OK.
My concern was about the time between ds1307->write_block_data() and
i2c_smbus_write_byte_data() which actually calls cond_sched().
I fully agree that your patch doesn't change the behaviour for the other
cases you presented and further clean up is to be done in a separate set
of patches.
--
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
Hi,
On 04/21/2015 03:51 AM, Nishanth Menon wrote:
> Alarm interrupt enable register is at offset 0x7, while the time
> registers for the alarm follow that. When we program Alarm interrupt
> enable prior to programming the time, it is possible that previous
> time value could be close or match at the time of alarm enable
> resulting in interrupt trigger which is unexpected (and does not match
> the time we expect it to trigger).
>
> To prevent this scenario from occuring, program the ALM0_EN bit only
> after the alarm time is appropriately programmed.
>
> Ofcourse, I2C programming is non-atomic, so there are loopholes where
> the interrupt wont trigger if the time requested is in the past at
> the time of programming the ALM0_EN bit. However, we will not have
> unexpected interrupts while the time is programmed after the interrupt
> are enabled.
I think it will be nice if you will mention that you going to follow
vendor recommendations - AN1491 Configuring the MCP794XX RTCC Family
http://ww1.microchip.com/downloads/en/AppNotes/01491A.pdf
;)
"Also, it is recommended that the alarm registers be loaded
before the alarm is enabled."
>
> Signed-off-by: Nishanth Menon <[email protected]>
> ---
> Changes in v2:
> - minor typo fix in comments
> - merged up code that I missed committing in
>
> V1: https://patchwork.kernel.org/patch/6245041/
>
> drivers/rtc/rtc-ds1307.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
> index 4ffabb322a9a..3cd4783375a5 100644
> --- a/drivers/rtc/rtc-ds1307.c
> +++ b/drivers/rtc/rtc-ds1307.c
> @@ -742,17 +742,17 @@ static int mcp794xx_set_alarm(struct device *dev, struct rtc_wkalrm *t)
> regs[6] &= ~MCP794XX_BIT_ALMX_IF;
> /* Set alarm match: second, minute, hour, day, date, month. */
> regs[6] |= MCP794XX_MSK_ALMX_MATCH;
> -
> - if (t->enabled)
> - regs[0] |= MCP794XX_BIT_ALM0_EN;
> - else
> - regs[0] &= ~MCP794XX_BIT_ALM0_EN;
> + /* Disable interrupt. We will not enable until completely programmed */
> + regs[0] &= ~MCP794XX_BIT_ALM0_EN;
>
> ret = ds1307->write_block_data(client, MCP794XX_REG_CONTROL, 10, regs);
> if (ret < 0)
> return ret;
>
> - return 0;
> + if (!t->enabled)
> + return 0;
> + regs[0] |= MCP794XX_BIT_ALM0_EN;
> + return i2c_smbus_write_byte_data(client, MCP794XX_REG_CONTROL, regs[0]);
So, It seems, that right sequence should be:
- disable alarmX
- read alarmX regs
- configure alarmX regs
- load alarmX regs
- enable alarmX
More over, looks like, alarm/alarm IRQ should be enabled/disabled separately from set_alarm/RTC_ALM_SET
by RTC_AIE_ON, RTC_AIE_OFF. Should it?
> }
>
> static int mcp794xx_alarm_irq_enable(struct device *dev, unsigned int enabled)
>
--
regards,
-grygorii
On 04/22/2015 08:26 AM, [email protected] wrote:
> Hi,
>
> On 04/21/2015 03:51 AM, Nishanth Menon wrote:
>> Alarm interrupt enable register is at offset 0x7, while the time
>> registers for the alarm follow that. When we program Alarm interrupt
>> enable prior to programming the time, it is possible that previous
>> time value could be close or match at the time of alarm enable
>> resulting in interrupt trigger which is unexpected (and does not match
>> the time we expect it to trigger).
>>
>> To prevent this scenario from occuring, program the ALM0_EN bit only
>> after the alarm time is appropriately programmed.
>>
>> Ofcourse, I2C programming is non-atomic, so there are loopholes where
>> the interrupt wont trigger if the time requested is in the past at
>> the time of programming the ALM0_EN bit. However, we will not have
>> unexpected interrupts while the time is programmed after the interrupt
>> are enabled.
>
> I think it will be nice if you will mention that you going to follow
> vendor recommendations - AN1491 Configuring the MCP794XX RTCC Family
> http://ww1.microchip.com/downloads/en/AppNotes/01491A.pdf
> ;)
> "Also, it is recommended that the alarm registers be loaded
> before the alarm is enabled."
>
Hmm... i did not know that existed, thanks for digging it up.. that
teaches me to look for docs before putting a scope/LA on the board
(not that I regret doing that)... That said, reading the app note, I
kind of realized:
a) that playing with ST bit for programming time is not done, but
then, that implies that oscillator will have to be restarted (upto a
few seconds for certain crystals).. but that said, it does not seem
mandatory or seem to (yet seen) functional issues...
b) We dont have flexibility yet to describe if we do indeed have a
backup battery or not - VBATEN should be set only if we have a backup
battery on the platform :( - on some it might even be optional thanks
to certain compliance requirements of shipping boards internationally
and general "unlike" of lithium ion in cargo hold..
c) we dont have capability to control the alarm polarity in the driver
which, by the way, we probably should also control OUT polarity (when
ALARM is not asserted)..
d) we dont have support for external 32k oscillator(X1 only) instead
of assuming we always have a 32k crystal(X1 and X2)...
Ugghhh... more cleaning up to do for the future..
that said, the sequence it does recommend (in page 4):
The following steps show how the Alarm 0 is config-
ured. Alarm 1 can be configured in the same manner.
1. Write 0x23 to the Alarm0 Seconds register
[0x0A].
2. Write 0x47 to the Alarm0 Minutes register
[0x0B].
3. Write 0x71 to the Alarm0 Hours register [0x0C]
? 11 hours in 12-hour format.
4. Write 0x72 to the Alarm0 Day register [0x0D] ?
Tuesday + Alarm Polarity Low + Match on all.
The Alarm0 Interrupt Flag is also cleared.
5. Write 0x14 to the Alarm0 Date register [0x0E].
6. Write 0x08 to the Alarm0 Month register [0x0F].
With all the Alarm0 registers set we can now activate
the Alarm0 on the Control register.
7. Write 0x10 to the Control register [0x07] ?
Alarm0 enabled no CLKOUT, Alarm1 disabled
before this patch we do ( http://pastebin.ubuntu.com/10863880/)
CONTROL r[7] = 0x90 (OUT=1, ALM0EN=1)
OSCTRIM r[8] = 0x00
EEUNLOCK r[9] = 0x00
ALM0SEC r[A] = 0x01
ALM0MIN r[B] = 0x45
ALM0HOUR r[C] = 0x23
ALM0WKDAY r[D] = 0x75 <-ALMOIF is cleared
ALM0DATE r[E] = 0x09
ALM0MTH r[F] = 0x04
RSRVED r[10] = 0x01
with this patch, we do:
burst( CONTROL r[7] = 0x80 (OUT=1)
OSCTRIM r[8] = 0x00
EEUNLOCK r[9] = 0x00
ALM0SEC r[A] = 0x01
ALM0MIN r[B] = 0x45
ALM0HOUR r[C] = 0x23
ALM0WKDAY r[D] = 0x75 <-ALMOIF is cleared
ALM0DATE r[E] = 0x09
ALM0MTH r[F] = 0x04
RSRVED r[10] = 0x01
)
CONTROL r[7] = 0x90 (OUT=1, ALM0EN=1)
Which is slightly unoptimal way of what the app note recommends. - as
I mentioned earlier in this thread, I will try and do optimizations in
a later patch.
Given that Andrew had picked up this patch, I dont see a reason to
respin this yet.. but will include the app note for future patches -
thanks for pointing it out to me.
>>
>> Signed-off-by: Nishanth Menon <[email protected]>
>> ---
>> Changes in v2:
>> - minor typo fix in comments
>> - merged up code that I missed committing in
>>
>> V1: https://patchwork.kernel.org/patch/6245041/
>>
>> drivers/rtc/rtc-ds1307.c | 12 ++++++------
>> 1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
>> index 4ffabb322a9a..3cd4783375a5 100644
>> --- a/drivers/rtc/rtc-ds1307.c
>> +++ b/drivers/rtc/rtc-ds1307.c
>> @@ -742,17 +742,17 @@ static int mcp794xx_set_alarm(struct device *dev, struct rtc_wkalrm *t)
>> regs[6] &= ~MCP794XX_BIT_ALMX_IF;
>> /* Set alarm match: second, minute, hour, day, date, month. */
>> regs[6] |= MCP794XX_MSK_ALMX_MATCH;
>> -
>> - if (t->enabled)
>> - regs[0] |= MCP794XX_BIT_ALM0_EN;
>> - else
>> - regs[0] &= ~MCP794XX_BIT_ALM0_EN;
>> + /* Disable interrupt. We will not enable until completely programmed */
>> + regs[0] &= ~MCP794XX_BIT_ALM0_EN;
>>
>> ret = ds1307->write_block_data(client, MCP794XX_REG_CONTROL, 10, regs);
>> if (ret < 0)
>> return ret;
>>
>> - return 0;
>> + if (!t->enabled)
>> + return 0;
>> + regs[0] |= MCP794XX_BIT_ALM0_EN;
>> + return i2c_smbus_write_byte_data(client, MCP794XX_REG_CONTROL, regs[0]);
>
> So, It seems, that right sequence should be:
> - disable alarmX
> - read alarmX regs
> - configure alarmX regs
> - load alarmX regs
> - enable alarmX
Not exactly.... see above. we can optimize this for a better sequence
as follows - since there are already un-necessary reads being
performed. probably just a couple of reads might be
sufficient..(ALM0WKDAY has some control bits as well.. Ugggh..
anyways..)...
Will have to think more about optimizing more later.
>
> More over, looks like, alarm/alarm IRQ should be enabled/disabled separately from set_alarm/RTC_ALM_SET
> by RTC_AIE_ON, RTC_AIE_OFF. Should it?
>
>> }
>>
>> static int mcp794xx_alarm_irq_enable(struct device *dev, unsigned int enabled)
>>
>
>
--
Regards,
Nishanth Menon
On 04/22/2015 06:30 AM, Alexandre Belloni wrote:
Apologies on a tardy response, got dragged into another issue and got
cooped up in lab whole day.
> On 21/04/2015 at 20:59:15 -0500, Nishanth Menon wrote :
>>>> Why is that so? when set alarm is requested for time X, you want
>>>> interrupt at time X, not an interrupt for previous configured RTC
>>>> alarm time!
>>>>
>>>
>>> You expect at least an interrupt.
>>
>> And you will get an interrupt if the event occurs before the i2c burst
>> starts. Once the i2cburst does start, you are committing to the new time.
>>
>
> You mean that even if ALM0EN is set after ALM0IF was set to 1, it will
> trigger the interrupt? I had a look at the MFP output block diagram
> would let me think that this is the case. I was thinking otherwise
> before. If that is so, then indeed, your patch is OK.
At least based on my testing it seems to be the case, and as you can
see in the block diagram, the ALM0EN mux comes after ALM0IF.. agreed
that I am not sure the mux to have some internal buffers/latches etc..
dont have access to rtl to make more comments.
>
> My concern was about the time between ds1307->write_block_data() and
> i2c_smbus_write_byte_data() which actually calls cond_sched().
>
> I fully agree that your patch doesn't change the behaviour for the other
> cases you presented and further clean up is to be done in a separate set
> of patches.
>
Can I take this as an Acked-by?
--
Regards,
Nishanth Menon
On 04/23/2015 03:00 AM, Nishanth Menon wrote:
> On 04/22/2015 08:26 AM, [email protected] wrote:
>> Hi,
>>
>> On 04/21/2015 03:51 AM, Nishanth Menon wrote:
>>> Alarm interrupt enable register is at offset 0x7, while the time
>>> registers for the alarm follow that. When we program Alarm interrupt
>>> enable prior to programming the time, it is possible that previous
>>> time value could be close or match at the time of alarm enable
>>> resulting in interrupt trigger which is unexpected (and does not match
>>> the time we expect it to trigger).
>>>
>>> To prevent this scenario from occuring, program the ALM0_EN bit only
>>> after the alarm time is appropriately programmed.
>>>
>>> Ofcourse, I2C programming is non-atomic, so there are loopholes where
>>> the interrupt wont trigger if the time requested is in the past at
>>> the time of programming the ALM0_EN bit. However, we will not have
>>> unexpected interrupts while the time is programmed after the interrupt
>>> are enabled.
>>
>> I think it will be nice if you will mention that you going to follow
>> vendor recommendations - AN1491 Configuring the MCP794XX RTCC Family
>> http://ww1.microchip.com/downloads/en/AppNotes/01491A.pdf
>> ;)
>> "Also, it is recommended that the alarm registers be loaded
>> before the alarm is enabled."
>>
>
> Hmm... i did not know that existed, thanks for digging it up.. that
> teaches me to look for docs before putting a scope/LA on the board
> (not that I regret doing that)... That said, reading the app note, I
> kind of realized:
> a) that playing with ST bit for programming time is not done, but
> then, that implies that oscillator will have to be restarted (upto a
> few seconds for certain crystals).. but that said, it does not seem
> mandatory or seem to (yet seen) functional issues...
>
> b) We dont have flexibility yet to describe if we do indeed have a
> backup battery or not - VBATEN should be set only if we have a backup
> battery on the platform :( - on some it might even be optional thanks
> to certain compliance requirements of shipping boards internationally
> and general "unlike" of lithium ion in cargo hold..
>
> c) we dont have capability to control the alarm polarity in the driver
> which, by the way, we probably should also control OUT polarity (when
> ALARM is not asserted)..
>
> d) we dont have support for external 32k oscillator(X1 only) instead
> of assuming we always have a 32k crystal(X1 and X2)...
>
> Ugghhh... more cleaning up to do for the future..
>
> that said, the sequence it does recommend (in page 4):
> The following steps show how the Alarm 0 is config-
> ured. Alarm 1 can be configured in the same manner.
> 1. Write 0x23 to the Alarm0 Seconds register
> [0x0A].
> 2. Write 0x47 to the Alarm0 Minutes register
> [0x0B].
> 3. Write 0x71 to the Alarm0 Hours register [0x0C]
> ? 11 hours in 12-hour format.
> 4. Write 0x72 to the Alarm0 Day register [0x0D] ?
> Tuesday + Alarm Polarity Low + Match on all.
> The Alarm0 Interrupt Flag is also cleared.
> 5. Write 0x14 to the Alarm0 Date register [0x0E].
> 6. Write 0x08 to the Alarm0 Month register [0x0F].
> With all the Alarm0 registers set we can now activate
> the Alarm0 on the Control register.
> 7. Write 0x10 to the Control register [0x07] ?
> Alarm0 enabled no CLKOUT, Alarm1 disabled
>
> before this patch we do ( http://pastebin.ubuntu.com/10863880/)
> CONTROL r[7] = 0x90 (OUT=1, ALM0EN=1)
> OSCTRIM r[8] = 0x00
> EEUNLOCK r[9] = 0x00
> ALM0SEC r[A] = 0x01
> ALM0MIN r[B] = 0x45
> ALM0HOUR r[C] = 0x23
> ALM0WKDAY r[D] = 0x75 <-ALMOIF is cleared
> ALM0DATE r[E] = 0x09
> ALM0MTH r[F] = 0x04
> RSRVED r[10] = 0x01
>
> with this patch, we do:
> burst( CONTROL r[7] = 0x80 (OUT=1)
> OSCTRIM r[8] = 0x00
> EEUNLOCK r[9] = 0x00
> ALM0SEC r[A] = 0x01
> ALM0MIN r[B] = 0x45
> ALM0HOUR r[C] = 0x23
> ALM0WKDAY r[D] = 0x75 <-ALMOIF is cleared
> ALM0DATE r[E] = 0x09
> ALM0MTH r[F] = 0x04
> RSRVED r[10] = 0x01
> )
> CONTROL r[7] = 0x90 (OUT=1, ALM0EN=1)
>
> Which is slightly unoptimal way of what the app note recommends. - as
> I mentioned earlier in this thread, I will try and do optimizations in
> a later patch.
>
> Given that Andrew had picked up this patch, I dont see a reason to
> respin this yet. but will include the app note for future patches -
> thanks for pointing it out to me.
^^ Up to you. Np, Always yours!
>>>
>>> Signed-off-by: Nishanth Menon <[email protected]>
>>> ---
>>> Changes in v2:
>>> - minor typo fix in comments
>>> - merged up code that I missed committing in
>>>
>>> V1: https://patchwork.kernel.org/patch/6245041/
>>>
>>> drivers/rtc/rtc-ds1307.c | 12 ++++++------
>>> 1 file changed, 6 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
>>> index 4ffabb322a9a..3cd4783375a5 100644
>>> --- a/drivers/rtc/rtc-ds1307.c
>>> +++ b/drivers/rtc/rtc-ds1307.c
>>> @@ -742,17 +742,17 @@ static int mcp794xx_set_alarm(struct device *dev, struct rtc_wkalrm *t)
>>> regs[6] &= ~MCP794XX_BIT_ALMX_IF;
>>> /* Set alarm match: second, minute, hour, day, date, month. */
>>> regs[6] |= MCP794XX_MSK_ALMX_MATCH;
>>> -
>>> - if (t->enabled)
>>> - regs[0] |= MCP794XX_BIT_ALM0_EN;
>>> - else
>>> - regs[0] &= ~MCP794XX_BIT_ALM0_EN;
>>> + /* Disable interrupt. We will not enable until completely programmed */
>>> + regs[0] &= ~MCP794XX_BIT_ALM0_EN;
>>>
>>> ret = ds1307->write_block_data(client, MCP794XX_REG_CONTROL, 10, regs);
>>> if (ret < 0)
>>> return ret;
>>>
>>> - return 0;
>>> + if (!t->enabled)
>>> + return 0;
>>> + regs[0] |= MCP794XX_BIT_ALM0_EN;
>>> + return i2c_smbus_write_byte_data(client, MCP794XX_REG_CONTROL, regs[0]);
>>
>> So, It seems, that right sequence should be:
>> - disable alarmX
>> - read alarmX regs
>> - configure alarmX regs
>> - load alarmX regs
>> - enable alarmX
> Not exactly.... see above. we can optimize this for a better sequence
> as follows - since there are already un-necessary reads being
> performed. probably just a couple of reads might be
> sufficient..(ALM0WKDAY has some control bits as well.. Ugggh..
> anyways..)...
>
>
> Will have to think more about optimizing more later.
Also I've done some fast investigation and I found that ~half of RTC drivers disable
ALM IRQ before start accessing Alarm regs (twl-rtc.c) while another half don't do that :)
(just FYI)
>
>>
>> More over, looks like, alarm/alarm IRQ should be enabled/disabled separately from set_alarm/RTC_ALM_SET
>> by RTC_AIE_ON, RTC_AIE_OFF. Should it?
>
--
regards,
-grygorii
On 04/23/2015 05:17 AM, [email protected] wrote:
> On 04/23/2015 03:00 AM, Nishanth Menon wrote:
>> On 04/22/2015 08:26 AM, [email protected] wrote:
>>> Hi,
>>>
>>> On 04/21/2015 03:51 AM, Nishanth Menon wrote:
>>>> Alarm interrupt enable register is at offset 0x7, while the time
>>>> registers for the alarm follow that. When we program Alarm interrupt
>>>> enable prior to programming the time, it is possible that previous
>>>> time value could be close or match at the time of alarm enable
>>>> resulting in interrupt trigger which is unexpected (and does not match
>>>> the time we expect it to trigger).
>>>>
>>>> To prevent this scenario from occuring, program the ALM0_EN bit only
>>>> after the alarm time is appropriately programmed.
>>>>
>>>> Ofcourse, I2C programming is non-atomic, so there are loopholes where
>>>> the interrupt wont trigger if the time requested is in the past at
>>>> the time of programming the ALM0_EN bit. However, we will not have
>>>> unexpected interrupts while the time is programmed after the interrupt
>>>> are enabled.
>>>
>>> I think it will be nice if you will mention that you going to follow
>>> vendor recommendations - AN1491 Configuring the MCP794XX RTCC Family
>>> http://ww1.microchip.com/downloads/en/AppNotes/01491A.pdf
>>> ;)
>>> "Also, it is recommended that the alarm registers be loaded
>>> before the alarm is enabled."
>>>
>>
>> Hmm... i did not know that existed, thanks for digging it up.. that
>> teaches me to look for docs before putting a scope/LA on the board
>> (not that I regret doing that)... That said, reading the app note, I
>> kind of realized:
>> a) that playing with ST bit for programming time is not done, but
>> then, that implies that oscillator will have to be restarted (upto a
>> few seconds for certain crystals).. but that said, it does not seem
>> mandatory or seem to (yet seen) functional issues...
>>
>> b) We dont have flexibility yet to describe if we do indeed have a
>> backup battery or not - VBATEN should be set only if we have a backup
>> battery on the platform :( - on some it might even be optional thanks
>> to certain compliance requirements of shipping boards internationally
>> and general "unlike" of lithium ion in cargo hold..
>>
>> c) we dont have capability to control the alarm polarity in the driver
>> which, by the way, we probably should also control OUT polarity (when
>> ALARM is not asserted)..
>>
>> d) we dont have support for external 32k oscillator(X1 only) instead
>> of assuming we always have a 32k crystal(X1 and X2)...
>>
>> Ugghhh... more cleaning up to do for the future..
>>
>> that said, the sequence it does recommend (in page 4):
>> The following steps show how the Alarm 0 is config-
>> ured. Alarm 1 can be configured in the same manner.
>> 1. Write 0x23 to the Alarm0 Seconds register
>> [0x0A].
>> 2. Write 0x47 to the Alarm0 Minutes register
>> [0x0B].
>> 3. Write 0x71 to the Alarm0 Hours register [0x0C]
>> ? 11 hours in 12-hour format.
>> 4. Write 0x72 to the Alarm0 Day register [0x0D] ?
>> Tuesday + Alarm Polarity Low + Match on all.
>> The Alarm0 Interrupt Flag is also cleared.
>> 5. Write 0x14 to the Alarm0 Date register [0x0E].
>> 6. Write 0x08 to the Alarm0 Month register [0x0F].
>> With all the Alarm0 registers set we can now activate
>> the Alarm0 on the Control register.
>> 7. Write 0x10 to the Control register [0x07] ?
>> Alarm0 enabled no CLKOUT, Alarm1 disabled
>>
>> before this patch we do ( http://pastebin.ubuntu.com/10863880/)
>> CONTROL r[7] = 0x90 (OUT=1, ALM0EN=1)
>> OSCTRIM r[8] = 0x00
>> EEUNLOCK r[9] = 0x00
>> ALM0SEC r[A] = 0x01
>> ALM0MIN r[B] = 0x45
>> ALM0HOUR r[C] = 0x23
>> ALM0WKDAY r[D] = 0x75 <-ALMOIF is cleared
>> ALM0DATE r[E] = 0x09
>> ALM0MTH r[F] = 0x04
>> RSRVED r[10] = 0x01
>>
>> with this patch, we do:
>> burst( CONTROL r[7] = 0x80 (OUT=1)
>> OSCTRIM r[8] = 0x00
>> EEUNLOCK r[9] = 0x00
>> ALM0SEC r[A] = 0x01
>> ALM0MIN r[B] = 0x45
>> ALM0HOUR r[C] = 0x23
>> ALM0WKDAY r[D] = 0x75 <-ALMOIF is cleared
>> ALM0DATE r[E] = 0x09
>> ALM0MTH r[F] = 0x04
>> RSRVED r[10] = 0x01
>> )
>> CONTROL r[7] = 0x90 (OUT=1, ALM0EN=1)
>>
>> Which is slightly unoptimal way of what the app note recommends. - as
>> I mentioned earlier in this thread, I will try and do optimizations in
>> a later patch.
>>
>> Given that Andrew had picked up this patch, I dont see a reason to
>> respin this yet. but will include the app note for future patches -
>> thanks for pointing it out to me.
>
> ^^ Up to you. Np, Always yours!
Considering the narrow focus of the current patch (which does fix an
issue that it attempts to), can I get an Ack?
--
Regards,
Nishanth Menon
On 04/23/2015 04:11 PM, Nishanth Menon wrote:
> On 04/23/2015 05:17 AM, [email protected] wrote:
>> On 04/23/2015 03:00 AM, Nishanth Menon wrote:
>>> On 04/22/2015 08:26 AM, [email protected] wrote:
>>>> Hi,
>>>>
>>>> On 04/21/2015 03:51 AM, Nishanth Menon wrote:
>>>>> Alarm interrupt enable register is at offset 0x7, while the time
>>>>> registers for the alarm follow that. When we program Alarm interrupt
>>>>> enable prior to programming the time, it is possible that previous
>>>>> time value could be close or match at the time of alarm enable
>>>>> resulting in interrupt trigger which is unexpected (and does not match
>>>>> the time we expect it to trigger).
>>>>>
>>>>> To prevent this scenario from occuring, program the ALM0_EN bit only
>>>>> after the alarm time is appropriately programmed.
>>>>>
>>>>> Ofcourse, I2C programming is non-atomic, so there are loopholes where
>>>>> the interrupt wont trigger if the time requested is in the past at
>>>>> the time of programming the ALM0_EN bit. However, we will not have
>>>>> unexpected interrupts while the time is programmed after the interrupt
>>>>> are enabled.
>>>>
>>>> I think it will be nice if you will mention that you going to follow
>>>> vendor recommendations - AN1491 Configuring the MCP794XX RTCC Family
>>>> http://ww1.microchip.com/downloads/en/AppNotes/01491A.pdf
>>>> ;)
>>>> "Also, it is recommended that the alarm registers be loaded
>>>> before the alarm is enabled."
>>>>
>>>
>>> Hmm... i did not know that existed, thanks for digging it up.. that
>>> teaches me to look for docs before putting a scope/LA on the board
>>> (not that I regret doing that)... That said, reading the app note, I
>>> kind of realized:
>>> a) that playing with ST bit for programming time is not done, but
>>> then, that implies that oscillator will have to be restarted (upto a
>>> few seconds for certain crystals).. but that said, it does not seem
>>> mandatory or seem to (yet seen) functional issues...
>>>
>>> b) We dont have flexibility yet to describe if we do indeed have a
>>> backup battery or not - VBATEN should be set only if we have a backup
>>> battery on the platform :( - on some it might even be optional thanks
>>> to certain compliance requirements of shipping boards internationally
>>> and general "unlike" of lithium ion in cargo hold..
>>>
>>> c) we dont have capability to control the alarm polarity in the driver
>>> which, by the way, we probably should also control OUT polarity (when
>>> ALARM is not asserted)..
>>>
>>> d) we dont have support for external 32k oscillator(X1 only) instead
>>> of assuming we always have a 32k crystal(X1 and X2)...
>>>
>>> Ugghhh... more cleaning up to do for the future..
>>>
>>> that said, the sequence it does recommend (in page 4):
>>> The following steps show how the Alarm 0 is config-
>>> ured. Alarm 1 can be configured in the same manner.
>>> 1. Write 0x23 to the Alarm0 Seconds register
>>> [0x0A].
>>> 2. Write 0x47 to the Alarm0 Minutes register
>>> [0x0B].
>>> 3. Write 0x71 to the Alarm0 Hours register [0x0C]
>>> ? 11 hours in 12-hour format.
>>> 4. Write 0x72 to the Alarm0 Day register [0x0D] ?
>>> Tuesday + Alarm Polarity Low + Match on all.
>>> The Alarm0 Interrupt Flag is also cleared.
>>> 5. Write 0x14 to the Alarm0 Date register [0x0E].
>>> 6. Write 0x08 to the Alarm0 Month register [0x0F].
>>> With all the Alarm0 registers set we can now activate
>>> the Alarm0 on the Control register.
>>> 7. Write 0x10 to the Control register [0x07] ?
>>> Alarm0 enabled no CLKOUT, Alarm1 disabled
>>>
>>> before this patch we do ( http://pastebin.ubuntu.com/10863880/)
>>> CONTROL r[7] = 0x90 (OUT=1, ALM0EN=1)
>>> OSCTRIM r[8] = 0x00
>>> EEUNLOCK r[9] = 0x00
>>> ALM0SEC r[A] = 0x01
>>> ALM0MIN r[B] = 0x45
>>> ALM0HOUR r[C] = 0x23
>>> ALM0WKDAY r[D] = 0x75 <-ALMOIF is cleared
>>> ALM0DATE r[E] = 0x09
>>> ALM0MTH r[F] = 0x04
>>> RSRVED r[10] = 0x01
>>>
>>> with this patch, we do:
>>> burst( CONTROL r[7] = 0x80 (OUT=1)
>>> OSCTRIM r[8] = 0x00
>>> EEUNLOCK r[9] = 0x00
>>> ALM0SEC r[A] = 0x01
>>> ALM0MIN r[B] = 0x45
>>> ALM0HOUR r[C] = 0x23
>>> ALM0WKDAY r[D] = 0x75 <-ALMOIF is cleared
>>> ALM0DATE r[E] = 0x09
>>> ALM0MTH r[F] = 0x04
>>> RSRVED r[10] = 0x01
>>> )
>>> CONTROL r[7] = 0x90 (OUT=1, ALM0EN=1)
>>>
>>> Which is slightly unoptimal way of what the app note recommends. - as
>>> I mentioned earlier in this thread, I will try and do optimizations in
>>> a later patch.
>>>
>>> Given that Andrew had picked up this patch, I dont see a reason to
>>> respin this yet. but will include the app note for future patches -
>>> thanks for pointing it out to me.
>>
>> ^^ Up to you. Np, Always yours!
>
> Considering the narrow focus of the current patch (which does fix an
> issue that it attempts to), can I get an Ack?
>
>
Reviewed-by: Grygorii Strashko <[email protected]>
--
regards,
-grygorii
On 22/04/2015 at 19:04:52 -0500, Nishanth Menon wrote :
> > I fully agree that your patch doesn't change the behaviour for the other
> > cases you presented and further clean up is to be done in a separate set
> > of patches.
> >
>
Sure,
Acked-by: Alexandre Belloni <[email protected]>
--
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com