Re: [PATCH V2] drivers/rtc/rtc-ds1307.c: Enable the mcp794xx alarm after programming time

[Nishanth Menon]

On 04/22/2015 08:26 AM, Grygorii.Strashko@xxxxxxxxxx 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@xxxxxx>
>> ---
>> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/