RE: [PATCH V5 4/4] rtc: imx-sc: add rtc alarm support

[Anson Huang]

Hi, Aisheng

Best Regards!
Anson Huang

> -----Original Message-----
> From: Aisheng Dong
> Sent: 2019年4月8日 19:09
> To: Anson Huang <anson.huang@xxxxxxx>; robh+dt@xxxxxxxxxx;
> mark.rutland@xxxxxxx; shawnguo@xxxxxxxxxx; s.hauer@xxxxxxxxxxxxxx;
> kernel@xxxxxxxxxxxxxx; festevam@xxxxxxxxx; a.zummo@xxxxxxxxxxxx;
> alexandre.belloni@xxxxxxxxxxx; ulf.hansson@xxxxxxxxxx; Daniel Baluta
> <daniel.baluta@xxxxxxx>; devicetree@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-
> rtc@xxxxxxxxxxxxxxx
> Cc: dl-linux-imx <linux-imx@xxxxxxx>
> Subject: RE: [PATCH V5 4/4] rtc: imx-sc: add rtc alarm support
> 
> > From: Anson Huang
> > Sent: Monday, March 18, 2019 11:10 AM
> >
> > Add i.MX system controller RTC alarm support, the RTC alarm is
> > implemented via SIP(silicon provider) runtime service call and
> > ARM-Trusted-Firmware will communicate with system controller via
> > MU(message unit) IPC to set RTC alarm. When RTC alarm fires, system
> > controller will generate a common MU irq event and notify system
> controller RTC driver to handle the irq event.
> >
> > Signed-off-by: Anson Huang <Anson.Huang@xxxxxxx>
> > ---
> > No function changes, just update imx_scu_irq_register_notifier()
> > function name according to SCU IRQ function name change.
> > ---
> >  drivers/rtc/rtc-imx-sc.c | 112
> > +++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 112 insertions(+)
> >
> > diff --git a/drivers/rtc/rtc-imx-sc.c b/drivers/rtc/rtc-imx-sc.c index
> > 19642bf..9df4990 100644
> > --- a/drivers/rtc/rtc-imx-sc.c
> > +++ b/drivers/rtc/rtc-imx-sc.c
> > @@ -3,6 +3,7 @@
> >   * Copyright 2018 NXP.
> >   */
> >
> > +#include <dt-bindings/firmware/imx/rsrc.h>
> >  #include <linux/arm-smccc.h>
> >  #include <linux/firmware/imx/sci.h>
> >  #include <linux/module.h>
> > @@ -11,11 +12,17 @@
> >  #include <linux/rtc.h>
> >
> >  #define IMX_SC_TIMER_FUNC_GET_RTC_SEC1970	9
> > +#define IMX_SC_TIMER_FUNC_SET_RTC_ALARM		8
> >  #define IMX_SC_TIMER_FUNC_SET_RTC_TIME		6
> >
> > +#define IMX_SC_IRQ_FUNC_ENABLE			1
> > +
> >  #define IMX_SIP_SRTC			0xC2000002
> >  #define IMX_SIP_SRTC_SET_TIME		0x0
> >
> > +#define SC_IRQ_GROUP_RTC    2
> > +#define SC_IRQ_RTC          1
> > +
> >  static struct imx_sc_ipc *rtc_ipc_handle;  static struct rtc_device
> > *imx_sc_rtc;
> >
> > @@ -24,6 +31,24 @@ struct imx_sc_msg_timer_get_rtc_time {
> >  	u32 time;
> >  } __packed;
> >
> > +struct imx_sc_msg_timer_enable_irq {
> > +	struct imx_sc_rpc_msg hdr;
> > +	u32 mask;
> > +	u16 resource;
> > +	u8 group;
> > +	u8 enable;
> > +} __packed;
> > +
> > +struct imx_sc_msg_timer_rtc_set_alarm {
> > +	struct imx_sc_rpc_msg hdr;
> > +	u16 year;
> > +	u8 mon;
> > +	u8 day;
> > +	u8 hour;
> > +	u8 min;
> > +	u8 sec;
> > +} __packed;
> > +
> >  static int imx_sc_rtc_read_time(struct device *dev, struct rtc_time *tm)  {
> >  	struct imx_sc_msg_timer_get_rtc_time msg; @@ -60,9 +85,92 @@
> static
> > int imx_sc_rtc_set_time(struct device *dev, struct rtc_time *tm)
> >  	return res.a0;
> >  }
> >
> > +static int imx_sc_rtc_alarm_irq_enable(struct device *dev, unsigned
> > +int
> > +enable) {
> 
> I think you shouldn't implement this generic function in rtc driver instead of
> Imx-scu-irq driver.

RTC driver has capability to enable/disable IRQ, so we have to implement this
callback, and considering the MU resource ID has to be parsed from DT, it does
NOT make sense to do it for all driver, so I will add another API in imx-scu-irq driver
to provide function of enabling/disabling irq, each driver can just call the API to
enable/disable its own IRQ, ONLY need to pass the corresponding arguments:

+void imx_scu_irq_enable(u8 group, u32 mask, u8 enable)
+{
+       struct imx_sc_msg_irq_enable msg;
+       struct imx_sc_rpc_msg *hdr = &msg.hdr;
+       int ret;
+
+       hdr->ver = IMX_SC_RPC_VERSION;
+       hdr->svc = IMX_SC_RPC_SVC_IRQ;
+       hdr->func = IMX_SC_IRQ_FUNC_ENABLE;
+       hdr->size = 3;
+
+       msg.resource = mu_resource_id;
+       msg.group = group;
+       msg.mask = mask;
+       msg.enable = enable;
+
+       ret = imx_scu_call_rpc(rtc_ipc_handle, &msg, true);
+       if (ret)
+               dev_err(dev, "enable irq failed, group %d, mask %d, ret %d\n",
+                       group, mask, ret);
+}

> 
> > +	struct imx_sc_msg_timer_enable_irq msg;
> > +	struct imx_sc_rpc_msg *hdr = &msg.hdr;
> > +	int ret;
> > +
> > +	hdr->ver = IMX_SC_RPC_VERSION;
> > +	hdr->svc = IMX_SC_RPC_SVC_IRQ;
> > +	hdr->func = IMX_SC_IRQ_FUNC_ENABLE;
> > +	hdr->size = 3;
> > +
> > +	msg.resource = IMX_SC_R_MU_1A;
> 
> Here may be wrong as it is not align with what you did in Patch 2 that MU
> resource is dynamically detected.

Fixed by upper comment using new API inside imx-scu-irq driver.

> 
> > +	msg.group = SC_IRQ_GROUP_RTC;
> > +	msg.mask = SC_IRQ_RTC;
> > +	msg.enable = enable;
> > +
> > +	ret = imx_scu_call_rpc(rtc_ipc_handle, &msg, true);
> > +	if (ret) {
> > +		dev_err(dev, "enable rtc irq failed, ret %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int imx_sc_rtc_read_alarm(struct device *dev, struct
> > +rtc_wkalrm
> > +*alrm) {
> > +	return 0;
> > +}
> 
> Can't avoid define NULL function?

We have to implement it as NULL function, as SCFW does NOT provide such feature,
But rtc alarm ONLY available when .read_alarm ops is implemented: 

147 static ssize_t
148 wakealarm_store(struct device *dev, struct device_attribute *attr,
149                 const char *buf, size_t n)
150 {

...

187                 retval = rtc_read_alarm(rtc, &alm);
188                 if (retval < 0)
189                         return retval;


> 
> > +
> > +static int imx_sc_rtc_set_alarm(struct device *dev, struct rtc_wkalrm
> > +*alrm) {
> > +	struct imx_sc_msg_timer_rtc_set_alarm msg;
> > +	struct imx_sc_rpc_msg *hdr = &msg.hdr;
> > +	int ret;
> > +	struct rtc_time *alrm_tm = &alrm->time;
> > +
> > +	hdr->ver = IMX_SC_RPC_VERSION;
> > +	hdr->svc = IMX_SC_RPC_SVC_TIMER;
> > +	hdr->func = IMX_SC_TIMER_FUNC_SET_RTC_ALARM;
> > +	hdr->size = 3;
> > +
> > +	msg.year = alrm_tm->tm_year + 1900;
> > +	msg.mon = alrm_tm->tm_mon + 1;
> > +	msg.day = alrm_tm->tm_mday;
> > +	msg.hour = alrm_tm->tm_hour;
> > +	msg.min = alrm_tm->tm_min;
> > +	msg.sec = alrm_tm->tm_sec;
> > +
> > +	ret = imx_scu_call_rpc(rtc_ipc_handle, &msg, true);
> > +	if (ret) {
> > +		dev_err(dev, "set rtc alarm failed, ret %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	ret = imx_sc_rtc_alarm_irq_enable(dev, alrm->enabled);
> 
> Just curious we already have .alarm_irq_enable().
> Why do we need call it again here?

That is because the  set_alarm function  pass alarm time and alarm->enabled argument,
it could be to enable alarm or to disable alarm, so we have to control the alarm
function here, all other rtc drivers also do it this way. The .alarm_irq_enable() callback
is for just enable or disable alarm.

Thanks,
Anson


> 
> Regards
> Dong Aisheng
> 
> > +	if (ret) {
> > +		dev_err(dev, "enable rtc alarm failed, ret %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  static const struct rtc_class_ops imx_sc_rtc_ops = {
> >  	.read_time = imx_sc_rtc_read_time,
> >  	.set_time = imx_sc_rtc_set_time,
> > +	.read_alarm = imx_sc_rtc_read_alarm,
> > +	.set_alarm = imx_sc_rtc_set_alarm,
> > +	.alarm_irq_enable = imx_sc_rtc_alarm_irq_enable, };
> > +
> > +static int imx_sc_rtc_alarm_sc_notify(struct notifier_block *nb,
> > +					unsigned long event, void *group) {
> > +	/* ignore non-rtc irq */
> > +	if (!((event & SC_IRQ_RTC) && (*(u8 *)group ==
> SC_IRQ_GROUP_RTC)))
> > +		return 0;
> > +
> > +	rtc_update_irq(imx_sc_rtc, 1, RTC_IRQF | RTC_AF);
> > +
> > +	return 0;
> > +}
> > +
> > +static struct notifier_block imx_sc_rtc_alarm_sc_notifier = {
> > +	.notifier_call = imx_sc_rtc_alarm_sc_notify,
> >  };
> >
> >  static int imx_sc_rtc_probe(struct platform_device *pdev) @@ -73,6
> > +181,8 @@ static int imx_sc_rtc_probe(struct platform_device *pdev)
> >  	if (ret)
> >  		return ret;
> >
> > +	device_init_wakeup(&pdev->dev, true);
> > +
> >  	imx_sc_rtc = devm_rtc_allocate_device(&pdev->dev);
> >  	if (IS_ERR(imx_sc_rtc))
> >  		return PTR_ERR(imx_sc_rtc);
> > @@ -87,6 +197,8 @@ static int imx_sc_rtc_probe(struct platform_device
> > *pdev)
> >  		return ret;
> >  	}
> >
> > +	imx_scu_irq_register_notifier(&imx_sc_rtc_alarm_sc_notifier);
> > +
> >  	return 0;
> >  }
> >
> > --
> > 2.7.4