| Message ID | 1429577494-15087-1-git-send-email-nm@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <nm@ti.com> > --- > 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 >
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 <nm@ti.com> >> --- >> 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 >> >
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.
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.
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.
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 <nm@ti.com> > --- > 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) >
On 04/22/2015 08:26 AM, Grygorii.Strashko@linaro.org 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 <nm@ti.com> >> --- >> 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) >> > >
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?
On 04/23/2015 03:00 AM, Nishanth Menon wrote: > On 04/22/2015 08:26 AM, Grygorii.Strashko@linaro.org 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 <nm@ti.com> >>> --- >>> 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? >
On 04/23/2015 05:17 AM, Grygorii.Strashko@linaro.org wrote: > On 04/23/2015 03:00 AM, Nishanth Menon wrote: >> On 04/22/2015 08:26 AM, Grygorii.Strashko@linaro.org 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?
On 04/23/2015 04:11 PM, Nishanth Menon wrote: > On 04/23/2015 05:17 AM, Grygorii.Strashko@linaro.org wrote: >> On 04/23/2015 03:00 AM, Nishanth Menon wrote: >>> On 04/22/2015 08:26 AM, Grygorii.Strashko@linaro.org 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 <grygorii.strashko@linaro.org>
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 <alexandre.belloni@free-electrons.com>
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)
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 <nm@ti.com> --- 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(-)