[U-Boot-Users] [PATCH] Add support for M41T60 serial access real-time clock

[Lawrence R. Johnson]

Andrew Dyer wrote:
> On 6/18/07, Lawrence R. Johnson <lrj at arlinx.com> wrote:
>> Hi everyone,
>>
>> Here is a patch to add support to U-Boot for the STMicroelectronics
>> M41T60 serial RTC.  I used the driver for the M41T11 as a template.
>>
> 
> We also use this chip.  A few comments below:

Hi Andrew (and list).  I'm glad to here you are also using the chip.  Thank you for you comments.

[...]
>> +/*
>> +       Convert between century and "century bits" (CB1 and CB0).  These
>> +       routines assume years are in the range 1900 - 2299.
>> +*/
>> +
>> +static unsigned char year2cb(unsigned const year)
>> +{
>> +       if (year < 1900 || year >= 2300) {
>> +               printf("M41T60 RTC: year %d out of range\n", year);
>> +       }
>> +       return (year / 100) & 0x3;
>> +}
> 
> This doesn't match the defined mapping in the datasheet between the
> century bits and years which has '00' corresponding to 2000.

I believe this code does return "00" when "year" is 2000.  As an added test, I've confirmed that 28 February 2000 rolls over to 29 February, whereas 28 February 1900, 2100, and 2200 all roll over to 1 March.

[...] 

>> +#if defined(DEBUG)
>> +static void rtc_dump(char const *const label)
>> +{
>> +       uchar data[8];
>> +
>> +       if (i2c_read(CFG_I2C_RTC_ADDR, 0, 1, data, sizeof(data))) {
>> +               printf("I2C read failed in rtc_dump()\n");
>> +               return;
>> +       }
>> +       printf("RTC dump %s: %02X-%02X-%02X-%02X-%02X-%02X-%02X-%02X\n",
>> +              label, data[0], data[1], data[2], data[3],
>> +              data[4], data[5], data[6], data[7]);
>> +}
>> +#else                          /* !defined(DEBUG) */
>> +#define rtc_dump(label)
>> +#endif /* defined(DEBUG) */
> 
> I would suggest to use the u-boot debug() macro here

As far as I can determine, the debug() macro only provides a conditionally-compiled printf statement.  In some cases, it would still be necessary to define an array and perform the i2c_read, which would also need to be conditionally compiled.  I put the code into a static function to avoid duplicating code and cluttering the non-debug code with #ifdef's.

> 
>> +       if (0x00 != (data[RTC_CTRL] & 0x80)) {
> 
> couldn't this be written as
> if(data[RTC_CTRL] & 0x80)
> 

Yes, and I'd expect the compiler to generate the same object code either way.  The way I wrote it is more in keeping with the coding style I'm used to, but I don't have an objection to changing it if others prefer it the other way.

>> +       /*
>> +        * If the ocsillator is stopped or the date is invalid, then
>> set the OUT
>> +        * bit to "1", reset the date registers, and start the
>> oscillator.
>> +        */

Oops, this comment is wrong (and misspelled).  It should read:
+	/*
+	 * If the oscillator is stopped or the date is invalid, then reset the
+	 * OUT bit to "0", reset the date registers, and start the oscillator.
+	 */

>> +       min = data[RTC_MIN] & 0x7F;
>> +       date = data[RTC_DATE];
>> +       month = data[RTC_MONTH] & 0x3F;
>> +       years = data[RTC_YEARS];
>> +       if (0x59 < data[RTC_SEC] || 0x09 < (data[RTC_SEC] & 0x0F) ||
>> +           0x59 < min || 0x09 < (min & 0x0F) ||
>> +           0x23 < data[RTC_HOUR] || 0x09 < (data[RTC_HOUR] & 0x0F) ||
>> +           0x07 < data[RTC_DAY] || 0x00 == data[RTC_DAY] ||
>> +           0x12 < month ||
>> +           0x99 < years || 0x09 < (years & 0x0F) ||
>> +           daysInMonth[month] < date || 0x09 < (date & 0x0F) || 0x00
>> == date ||
>> +           (0x29 == date && 0x02 == month &&
>> +            ((0x00 != (years & 0x03)) ||
>> +             (0x00 == years && 0x00 != (data[RTC_MONTH] & 0xC0))))) {
>> +               printf("Resetting M41T60 RTC clock.\n");
>> +               /*
>> +                * Set to 00:00:00 1900-01-01 (Monday)
>> +                */
>> +               data[RTC_SEC] = 0x00;
>> +               data[RTC_MIN] = 0x00;
>> +               data[RTC_HOUR] = 0x00;
>> +               data[RTC_DAY] = 0x02;
>> +               data[RTC_DATE] = 0x01;
>> +               data[RTC_MONTH] = 0xC1;
>> +               data[RTC_YEARS] = 0x00;
>> +               data[RTC_CTRL] &= 0x7F;
> 
> here you are resetting the calibration bits.  It would be nice to have
> the option to try to fetch the calibration bits from the u-boot
> environment and restore them.  Also your code is clearing the FT and
> OUT/OFIE bits, which someone might be using for something.  This
> behaviour should at least be noted in the comments.  Better would be
> an option as to how to handle those bits on reset.
> 

This code is executed when it appears that the chip has lost (and regained) power.  In that case, the FT and OFIE bits are documented to come up reset to "0", so they are not changed.  The calibration bits also appear to all come up as "0", though I didn't find any mention of this in the data sheet.  I do use the OUT bit to detect power loss, as noted in the comments in the code.  If someone wanted to use the OUT bit for some other purpose, the driver would need to be modified, perhaps with a conditional compilation.

I like the idea of using an environmental variable to hold the RTC calibration, as this chip has no on-board EEPROM.  Can anyone tell me if there is such a variable defined now?  If not, is there a way to have one assigned?  Can the current patch be applied to the code base now, and the support for calibration bits added as an enhancement?  I'd appreciate any direction the list can give me.

-- Larry