Re: [PATCH v4 4/7] platform: cznic: turris-omnia-mcu: Add support for poweroff and wakeup

["Marek Behún" <kabel@kernel.org>]

On Fri, 3 Nov 2023 20:23:16 +0100
Alexandre Belloni <alexandre.belloni@bootlin.com> wrote:

> On 26/10/2023 18:18:00+0200, Marek Behún wrote:
> > +static int omnia_get_uptime_wakeup(const struct i2c_client *client, u32 *uptime,
> > +				   u32 *wakeup)
> > +{
> > +	__le32 reply[2];
> > +	int err;
> > +
> > +	err = omnia_cmd_read(client, CMD_GET_UPTIME_AND_WAKEUP, reply,
> > +			     sizeof(reply));
> > +	if (err)
> > +		return err;
> > +
> > +	if (uptime)
> > +		*uptime = le32_to_cpu(reply[0]);
> > +
> > +	if (wakeup)
> > +		*wakeup = le32_to_cpu(reply[1]);
> > +
> > +	return 0;
> > +}
> > +
> > +static int omnia_read_time(struct device *dev, struct rtc_time *tm)
> > +{
> > +	u32 uptime;
> > +	int err;
> > +
> > +	err = omnia_get_uptime_wakeup(to_i2c_client(dev), &uptime, NULL);
> > +	if (err)
> > +		return err;  
> 
> Does this get the real time or the board uptime?

Hi Alexandre,

sorry I did not notice your email sooner.
This returns board uptime. The MCU does not remember real time.

> > +
> > +	rtc_time64_to_tm(uptime, tm);
> > +
> > +	return 0;
> > +}
> > +
> > +static int omnia_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> > +{
> > +	struct i2c_client *client = to_i2c_client(dev);
> > +	struct omnia_mcu *mcu = i2c_get_clientdata(client);
> > +	u32 wakeup;
> > +	int err;
> > +
> > +	err = omnia_get_uptime_wakeup(client, NULL, &wakeup);
> > +	if (err)
> > +		return err;
> > +
> > +	alrm->enabled = !!wakeup;
> > +	rtc_time64_to_tm(wakeup ?: mcu->rtc_alarm, &alrm->time);  
> 
> I don't think this works properly as on boot, mcu->rtc_alarm will not be
> set whereas wakeup could be.

mcu->rtc_alarm is the value that is to be written to MCU wakeup
register when we enable the RTC alarm with .alarm_irq_enable().

When the alarm is enabled by other means than the driver (prior boot or
by a userspace utility), the omnia_read_alarm() will determine it
correctly, since it will read via I2C the MCU wakeup value.

If it is non-zero, it will return that the alarm is enabled and the
alarm time is determined by this non-zero value.

If the alarm is disabled on the MCU, we return that the alarm is
disabled, but we fill in the alrm->time with the cached value of
mcu->rtc_alarm which was set in previous call to the .set_alarm method.
This is because we want to support both the new IOCTLs (RTC_WKALM_RD,
RTC_WKALM_SET) and the old (RTC_ALM_SET, RTC_AIE_ON, RTC_AIE_OFF).

> 
> Also, is wakeup actually an absolute time? I'm not sure I get how the
> MCU works then.

The wakeup value is in the same units as uptime: seconds elapsed since
MCU started.

Example:
- uptime is 2000 seconds (MCU has been powered on 2000 seconds ago)
- we are going to power off the board, but we want it to power on in
  one hour
- we will therefore set wakeup to uptime + 3600 = 2000 + 3600 = 5600

> > +	mcu->rtcdev->ops = &omnia_rtc_ops;
> > +	mcu->rtcdev->range_max = U32_MAX;  
> 
> You probably need to add:
> 
> set_bit(RTC_FEATURE_ALARM_WAKEUP_ONLY, mcu->rtcdev->features);
> 

Thx, will do. Is this bit documented? Could only find its definition
and one usage with git grep.

Marek