| Message ID | 20240621-imx95-bbm-misc-v2-v5-6-b85a6bf778cb@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | firmware: support i.MX95 SCMI BBM/MISC Extenstion | expand |
On Fri, Jun 21, 2024 at 03:04:41PM +0800, Peng Fan (OSS) wrote: > From: Peng Fan <peng.fan@nxp.com> > > The BBM module provides RTC feature. To i.MX95, this module is managed by > System Manager and exported System Control Management Interface(SCMI). > Linux could use i.MX SCMI BBM Extension protocol to use RTC feature. > Hi Peng, LGTM. Reviewed-by: Cristian Marussi <cristian.marussi@arm.com> Thanks, Cristian
Hello, On 21/06/2024 15:04:41+0800, Peng Fan (OSS) wrote: > + ret = bbnsm->ops->rtc_time_get(ph, 0, &val); > + if (ret) { > + dev_err(dev, "%s: %d\n", __func__, ret); This is not super useful, you should drop the various dev_err or pr_err as there is no action the user can take to solve the erro apart from retrying. > + return ret; > + } > + > + rtc_time64_to_tm(val, tm); > + > + return 0; > +} > + > +static int scmi_imx_bbm_set_time(struct device *dev, struct rtc_time *tm) > +{ > + struct scmi_imx_bbm *bbnsm = dev_get_drvdata(dev); > + struct scmi_protocol_handle *ph = bbnsm->ph; > + u64 val; > + int ret; > + > + val = rtc_tm_to_time64(tm); > + > + ret = bbnsm->ops->rtc_time_set(ph, 0, val); > + if (ret) > + dev_err(dev, "%s: %d\n", __func__, ret); > + > + return ret; > +} > + > +static int scmi_imx_bbm_alarm_irq_enable(struct device *dev, unsigned int enable) > +{ How can userspace disable the alarm? > + return 0; > +} > + > +static int scmi_imx_bbm_set_alarm(struct device *dev, struct rtc_wkalrm *alrm) > +{ > + struct scmi_imx_bbm *bbnsm = dev_get_drvdata(dev); > + struct scmi_protocol_handle *ph = bbnsm->ph; > + struct rtc_time *alrm_tm = &alrm->time; > + u64 val; > + int ret; > + > + val = rtc_tm_to_time64(alrm_tm); > + > + ret = bbnsm->ops->rtc_alarm_set(ph, 0, val); > + if (ret) > + dev_err(dev, "%s: %d\n", __func__, ret); > + > + return ret; > +} > + > +static const struct rtc_class_ops smci_imx_bbm_rtc_ops = { > + .read_time = scmi_imx_bbm_read_time, > + .set_time = scmi_imx_bbm_set_time, > + .set_alarm = scmi_imx_bbm_set_alarm, > + .alarm_irq_enable = scmi_imx_bbm_alarm_irq_enable, > +}; > + > +static int scmi_imx_bbm_rtc_notifier(struct notifier_block *nb, unsigned long event, void *data) > +{ > + struct scmi_imx_bbm *bbnsm = container_of(nb, struct scmi_imx_bbm, nb); > + struct scmi_imx_bbm_notif_report *r = data; > + > + if (r->is_rtc) > + rtc_update_irq(bbnsm->rtc_dev, 1, RTC_AF | RTC_IRQF); > + else > + pr_err("Unexpected bbm event: %s\n", __func__); > + > + return 0; > +} > + > +static int scmi_imx_bbm_rtc_init(struct scmi_device *sdev) > +{ > + const struct scmi_handle *handle = sdev->handle; > + struct device *dev = &sdev->dev; > + struct scmi_imx_bbm *bbnsm = dev_get_drvdata(dev); > + int ret; > + > + bbnsm->rtc_dev = devm_rtc_allocate_device(dev); > + if (IS_ERR(bbnsm->rtc_dev)) > + return PTR_ERR(bbnsm->rtc_dev); > + > + bbnsm->rtc_dev->ops = &smci_imx_bbm_rtc_ops; > + bbnsm->rtc_dev->range_min = 0; range_min is set to 0 by default, this is not necessary > + bbnsm->rtc_dev->range_max = U32_MAX; > + > + ret = devm_rtc_register_device(bbnsm->rtc_dev); > + if (ret) > + return ret; > + > + bbnsm->nb.notifier_call = &scmi_imx_bbm_rtc_notifier; > + return handle->notify_ops->devm_event_notifier_register(sdev, SCMI_PROTOCOL_IMX_BBM, > + SCMI_EVENT_IMX_BBM_RTC, > + NULL, &bbnsm->nb); Note that failing after devm_rtc_register_device opens the driver to a race condition as the character device will exist at that time.
> Subject: Re: [PATCH v5 6/7] rtc: support i.MX95 BBM RTC > > Hello, > > On 21/06/2024 15:04:41+0800, Peng Fan (OSS) wrote: > > + ret = bbnsm->ops->rtc_time_get(ph, 0, &val); > > + if (ret) { > > + dev_err(dev, "%s: %d\n", __func__, ret); > > This is not super useful, you should drop the various dev_err or pr_err > as there is no action the user can take to solve the erro apart from > retrying. Sure. I will remove the various dev_err or pr_err in v6. > > > + return ret; > > + } > > + > > + rtc_time64_to_tm(val, tm); > > + > > + return 0; > > +} > > + > > +static int scmi_imx_bbm_set_time(struct device *dev, struct > rtc_time > > +*tm) { > > + struct scmi_imx_bbm *bbnsm = dev_get_drvdata(dev); > > + struct scmi_protocol_handle *ph = bbnsm->ph; > > + u64 val; > > + int ret; > > + > > + val = rtc_tm_to_time64(tm); > > + > > + ret = bbnsm->ops->rtc_time_set(ph, 0, val); > > + if (ret) > > + dev_err(dev, "%s: %d\n", __func__, ret); > > + > > + return ret; > > +} > > + > > +static int scmi_imx_bbm_alarm_irq_enable(struct device *dev, > unsigned > > +int enable) { > > How can userspace disable the alarm? The SCMI notify has kernel level enable/disable drivers/firmware/arm_scmi/notify.c But indeed, userspace not able to disable the alarm. I need use if (!enable) return -EOPNOTSUPP; > > > + return 0; > > +} > > + > > +static int scmi_imx_bbm_set_alarm(struct device *dev, struct > > +rtc_wkalrm *alrm) { > > + struct scmi_imx_bbm *bbnsm = dev_get_drvdata(dev); > > + struct scmi_protocol_handle *ph = bbnsm->ph; > > + struct rtc_time *alrm_tm = &alrm->time; > > + u64 val; > > + int ret; > > + > > + val = rtc_tm_to_time64(alrm_tm); > > + > > + ret = bbnsm->ops->rtc_alarm_set(ph, 0, val); > > + if (ret) > > + dev_err(dev, "%s: %d\n", __func__, ret); > > + > > + return ret; > > +} > > + > > +static const struct rtc_class_ops smci_imx_bbm_rtc_ops = { > > + .read_time = scmi_imx_bbm_read_time, > > + .set_time = scmi_imx_bbm_set_time, > > + .set_alarm = scmi_imx_bbm_set_alarm, > > + .alarm_irq_enable = scmi_imx_bbm_alarm_irq_enable, }; > > + > > +static int scmi_imx_bbm_rtc_notifier(struct notifier_block *nb, > > +unsigned long event, void *data) { > > + struct scmi_imx_bbm *bbnsm = container_of(nb, struct > scmi_imx_bbm, nb); > > + struct scmi_imx_bbm_notif_report *r = data; > > + > > + if (r->is_rtc) > > + rtc_update_irq(bbnsm->rtc_dev, 1, RTC_AF | > RTC_IRQF); > > + else > > + pr_err("Unexpected bbm event: %s\n", __func__); > > + > > + return 0; > > +} > > + > > +static int scmi_imx_bbm_rtc_init(struct scmi_device *sdev) { > > + const struct scmi_handle *handle = sdev->handle; > > + struct device *dev = &sdev->dev; > > + struct scmi_imx_bbm *bbnsm = dev_get_drvdata(dev); > > + int ret; > > + > > + bbnsm->rtc_dev = devm_rtc_allocate_device(dev); > > + if (IS_ERR(bbnsm->rtc_dev)) > > + return PTR_ERR(bbnsm->rtc_dev); > > + > > + bbnsm->rtc_dev->ops = &smci_imx_bbm_rtc_ops; > > + bbnsm->rtc_dev->range_min = 0; > > range_min is set to 0 by default, this is not necessary > > > + bbnsm->rtc_dev->range_max = U32_MAX; > > + > > + ret = devm_rtc_register_device(bbnsm->rtc_dev); > > + if (ret) > > + return ret; > > + > > + bbnsm->nb.notifier_call = &scmi_imx_bbm_rtc_notifier; > > + return handle->notify_ops->devm_event_notifier_register(sdev, > SCMI_PROTOCOL_IMX_BBM, > > + > SCMI_EVENT_IMX_BBM_RTC, > > + > NULL, &bbnsm->nb); > > Note that failing after devm_rtc_register_device opens the driver to a > race condition as the character device will exist at that time. Could you please share more info on this? Won't the devm_rtc_register_device could automatically remove the rtc is notifier register fails? Thanks, Peng. > > > -- > Alexandre Belloni, co-owner and COO, Bootlin Embedded Linux and > Kernel engineering > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2F > bootlin.com%2F&data=05%7C02%7Cpeng.fan%40nxp.com%7C91ef3c1 > 72dc04c6ed2b908dca1e8befb%7C686ea1d3bc2b4c6fa92cd99c5c3016 > 35%7C0%7C0%7C638563268206282814%7CUnknown%7CTWFpbGZs > b3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXV > CI6Mn0%3D%7C0%7C%7C%7C&sdata=x8j754poti%2Bo07tbvO3p7XM > QB5jrbkBPWKORIKZdE%2BU%3D&reserved=0
On 14/07/2024 08:22:59+0000, Peng Fan wrote: > > Subject: Re: [PATCH v5 6/7] rtc: support i.MX95 BBM RTC > > > > Hello, > > > > On 21/06/2024 15:04:41+0800, Peng Fan (OSS) wrote: > > > + ret = bbnsm->ops->rtc_time_get(ph, 0, &val); > > > + if (ret) { > > > + dev_err(dev, "%s: %d\n", __func__, ret); > > > > This is not super useful, you should drop the various dev_err or pr_err > > as there is no action the user can take to solve the erro apart from > > retrying. > > Sure. I will remove the various dev_err or pr_err in v6. > > > > > > + return ret; > > > + } > > > + > > > + rtc_time64_to_tm(val, tm); > > > + > > > + return 0; > > > +} > > > + > > > +static int scmi_imx_bbm_set_time(struct device *dev, struct > > rtc_time > > > +*tm) { > > > + struct scmi_imx_bbm *bbnsm = dev_get_drvdata(dev); > > > + struct scmi_protocol_handle *ph = bbnsm->ph; > > > + u64 val; > > > + int ret; > > > + > > > + val = rtc_tm_to_time64(tm); > > > + > > > + ret = bbnsm->ops->rtc_time_set(ph, 0, val); > > > + if (ret) > > > + dev_err(dev, "%s: %d\n", __func__, ret); > > > + > > > + return ret; > > > +} > > > + > > > +static int scmi_imx_bbm_alarm_irq_enable(struct device *dev, > > unsigned > > > +int enable) { > > > > How can userspace disable the alarm? > > The SCMI notify has kernel level enable/disable > drivers/firmware/arm_scmi/notify.c > > But indeed, userspace not able to disable the alarm. > > I need use > if (!enable) return -EOPNOTSUPP; > > > > > > + return 0; > > > +} > > > + > > > +static int scmi_imx_bbm_set_alarm(struct device *dev, struct > > > +rtc_wkalrm *alrm) { > > > + struct scmi_imx_bbm *bbnsm = dev_get_drvdata(dev); > > > + struct scmi_protocol_handle *ph = bbnsm->ph; > > > + struct rtc_time *alrm_tm = &alrm->time; > > > + u64 val; > > > + int ret; > > > + > > > + val = rtc_tm_to_time64(alrm_tm); > > > + > > > + ret = bbnsm->ops->rtc_alarm_set(ph, 0, val); > > > + if (ret) > > > + dev_err(dev, "%s: %d\n", __func__, ret); > > > + > > > + return ret; > > > +} > > > + > > > +static const struct rtc_class_ops smci_imx_bbm_rtc_ops = { > > > + .read_time = scmi_imx_bbm_read_time, > > > + .set_time = scmi_imx_bbm_set_time, > > > + .set_alarm = scmi_imx_bbm_set_alarm, > > > + .alarm_irq_enable = scmi_imx_bbm_alarm_irq_enable, }; > > > + > > > +static int scmi_imx_bbm_rtc_notifier(struct notifier_block *nb, > > > +unsigned long event, void *data) { > > > + struct scmi_imx_bbm *bbnsm = container_of(nb, struct > > scmi_imx_bbm, nb); > > > + struct scmi_imx_bbm_notif_report *r = data; > > > + > > > + if (r->is_rtc) > > > + rtc_update_irq(bbnsm->rtc_dev, 1, RTC_AF | > > RTC_IRQF); > > > + else > > > + pr_err("Unexpected bbm event: %s\n", __func__); > > > + > > > + return 0; > > > +} > > > + > > > +static int scmi_imx_bbm_rtc_init(struct scmi_device *sdev) { > > > + const struct scmi_handle *handle = sdev->handle; > > > + struct device *dev = &sdev->dev; > > > + struct scmi_imx_bbm *bbnsm = dev_get_drvdata(dev); > > > + int ret; > > > + > > > + bbnsm->rtc_dev = devm_rtc_allocate_device(dev); > > > + if (IS_ERR(bbnsm->rtc_dev)) > > > + return PTR_ERR(bbnsm->rtc_dev); > > > + > > > + bbnsm->rtc_dev->ops = &smci_imx_bbm_rtc_ops; > > > + bbnsm->rtc_dev->range_min = 0; > > > > range_min is set to 0 by default, this is not necessary > > > > > + bbnsm->rtc_dev->range_max = U32_MAX; > > > + > > > + ret = devm_rtc_register_device(bbnsm->rtc_dev); > > > + if (ret) > > > + return ret; > > > + > > > + bbnsm->nb.notifier_call = &scmi_imx_bbm_rtc_notifier; > > > + return handle->notify_ops->devm_event_notifier_register(sdev, > > SCMI_PROTOCOL_IMX_BBM, > > > + > > SCMI_EVENT_IMX_BBM_RTC, > > > + > > NULL, &bbnsm->nb); > > > > Note that failing after devm_rtc_register_device opens the driver to a > > race condition as the character device will exist at that time. > > Could you please share more info on this? > > Won't the devm_rtc_register_device could automatically remove > the rtc is notifier register fails? Yes but once devm_rtc_register_device is called, the character device is available for userspace so it may be opened before probe finishes so while probe fails, the device is kept opened until userspace closes the file.
diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig index 2a95b05982ad..8122bfeba8c0 100644 --- a/drivers/rtc/Kconfig +++ b/drivers/rtc/Kconfig @@ -1827,6 +1827,14 @@ config RTC_DRV_BBNSM This driver can also be built as a module, if so, the module will be called "rtc-bbnsm". +config RTC_DRV_IMX_BBM_SCMI + depends on IMX_SCMI_BBM_EXT || COMPILE_TEST + default y if ARCH_MXC + tristate "NXP i.MX BBM SCMI RTC support" + help + If you say yes here you get support for the NXP i.MX BBSM SCMI + RTC module. + config RTC_DRV_IMX_SC depends on IMX_SCU depends on HAVE_ARM_SMCCC diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile index 3004e372f25f..8ee79cb18322 100644 --- a/drivers/rtc/Makefile +++ b/drivers/rtc/Makefile @@ -74,6 +74,7 @@ obj-$(CONFIG_RTC_DRV_HID_SENSOR_TIME) += rtc-hid-sensor-time.o obj-$(CONFIG_RTC_DRV_HYM8563) += rtc-hym8563.o obj-$(CONFIG_RTC_DRV_IMXDI) += rtc-imxdi.o obj-$(CONFIG_RTC_DRV_IMX_SC) += rtc-imx-sc.o +obj-$(CONFIG_RTC_DRV_IMX_BBM_SCMI) += rtc-imx-sm-bbm.o obj-$(CONFIG_RTC_DRV_ISL12022) += rtc-isl12022.o obj-$(CONFIG_RTC_DRV_ISL12026) += rtc-isl12026.o obj-$(CONFIG_RTC_DRV_ISL1208) += rtc-isl1208.o diff --git a/drivers/rtc/rtc-imx-sm-bbm.c b/drivers/rtc/rtc-imx-sm-bbm.c new file mode 100644 index 000000000000..a634e5915d2b --- /dev/null +++ b/drivers/rtc/rtc-imx-sm-bbm.c @@ -0,0 +1,168 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Copyright 2024 NXP. + */ + +#include <linux/jiffies.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/rtc.h> +#include <linux/scmi_protocol.h> +#include <linux/scmi_imx_protocol.h> + +struct scmi_imx_bbm { + const struct scmi_imx_bbm_proto_ops *ops; + struct rtc_device *rtc_dev; + struct scmi_protocol_handle *ph; + struct notifier_block nb; +}; + +static int scmi_imx_bbm_read_time(struct device *dev, struct rtc_time *tm) +{ + struct scmi_imx_bbm *bbnsm = dev_get_drvdata(dev); + struct scmi_protocol_handle *ph = bbnsm->ph; + u64 val; + int ret; + + ret = bbnsm->ops->rtc_time_get(ph, 0, &val); + if (ret) { + dev_err(dev, "%s: %d\n", __func__, ret); + return ret; + } + + rtc_time64_to_tm(val, tm); + + return 0; +} + +static int scmi_imx_bbm_set_time(struct device *dev, struct rtc_time *tm) +{ + struct scmi_imx_bbm *bbnsm = dev_get_drvdata(dev); + struct scmi_protocol_handle *ph = bbnsm->ph; + u64 val; + int ret; + + val = rtc_tm_to_time64(tm); + + ret = bbnsm->ops->rtc_time_set(ph, 0, val); + if (ret) + dev_err(dev, "%s: %d\n", __func__, ret); + + return ret; +} + +static int scmi_imx_bbm_alarm_irq_enable(struct device *dev, unsigned int enable) +{ + return 0; +} + +static int scmi_imx_bbm_set_alarm(struct device *dev, struct rtc_wkalrm *alrm) +{ + struct scmi_imx_bbm *bbnsm = dev_get_drvdata(dev); + struct scmi_protocol_handle *ph = bbnsm->ph; + struct rtc_time *alrm_tm = &alrm->time; + u64 val; + int ret; + + val = rtc_tm_to_time64(alrm_tm); + + ret = bbnsm->ops->rtc_alarm_set(ph, 0, val); + if (ret) + dev_err(dev, "%s: %d\n", __func__, ret); + + return ret; +} + +static const struct rtc_class_ops smci_imx_bbm_rtc_ops = { + .read_time = scmi_imx_bbm_read_time, + .set_time = scmi_imx_bbm_set_time, + .set_alarm = scmi_imx_bbm_set_alarm, + .alarm_irq_enable = scmi_imx_bbm_alarm_irq_enable, +}; + +static int scmi_imx_bbm_rtc_notifier(struct notifier_block *nb, unsigned long event, void *data) +{ + struct scmi_imx_bbm *bbnsm = container_of(nb, struct scmi_imx_bbm, nb); + struct scmi_imx_bbm_notif_report *r = data; + + if (r->is_rtc) + rtc_update_irq(bbnsm->rtc_dev, 1, RTC_AF | RTC_IRQF); + else + pr_err("Unexpected bbm event: %s\n", __func__); + + return 0; +} + +static int scmi_imx_bbm_rtc_init(struct scmi_device *sdev) +{ + const struct scmi_handle *handle = sdev->handle; + struct device *dev = &sdev->dev; + struct scmi_imx_bbm *bbnsm = dev_get_drvdata(dev); + int ret; + + bbnsm->rtc_dev = devm_rtc_allocate_device(dev); + if (IS_ERR(bbnsm->rtc_dev)) + return PTR_ERR(bbnsm->rtc_dev); + + bbnsm->rtc_dev->ops = &smci_imx_bbm_rtc_ops; + bbnsm->rtc_dev->range_min = 0; + bbnsm->rtc_dev->range_max = U32_MAX; + + ret = devm_rtc_register_device(bbnsm->rtc_dev); + if (ret) + return ret; + + bbnsm->nb.notifier_call = &scmi_imx_bbm_rtc_notifier; + return handle->notify_ops->devm_event_notifier_register(sdev, SCMI_PROTOCOL_IMX_BBM, + SCMI_EVENT_IMX_BBM_RTC, + NULL, &bbnsm->nb); +} + +static int scmi_imx_bbm_rtc_probe(struct scmi_device *sdev) +{ + const struct scmi_handle *handle = sdev->handle; + struct device *dev = &sdev->dev; + struct scmi_protocol_handle *ph; + struct scmi_imx_bbm *bbnsm; + int ret; + + if (!handle) + return -ENODEV; + + bbnsm = devm_kzalloc(dev, sizeof(*bbnsm), GFP_KERNEL); + if (!bbnsm) + return -ENOMEM; + + bbnsm->ops = handle->devm_protocol_get(sdev, SCMI_PROTOCOL_IMX_BBM, &ph); + if (IS_ERR(bbnsm->ops)) + return PTR_ERR(bbnsm->ops); + + bbnsm->ph = ph; + + device_init_wakeup(dev, true); + + dev_set_drvdata(dev, bbnsm); + + ret = scmi_imx_bbm_rtc_init(sdev); + if (ret) + device_init_wakeup(dev, false); + + return ret; +} + +static const struct scmi_device_id scmi_id_table[] = { + { SCMI_PROTOCOL_IMX_BBM, "imx-bbm-rtc" }, + { }, +}; +MODULE_DEVICE_TABLE(scmi, scmi_id_table); + +static struct scmi_driver scmi_imx_bbm_rtc_driver = { + .name = "scmi-imx-bbm-rtc", + .probe = scmi_imx_bbm_rtc_probe, + .id_table = scmi_id_table, +}; +module_scmi_driver(scmi_imx_bbm_rtc_driver); + +MODULE_AUTHOR("Peng Fan <peng.fan@nxp.com>"); +MODULE_DESCRIPTION("IMX SM BBM RTC driver"); +MODULE_LICENSE("GPL");