[PATCH v3 0/2] Raspberry Pi 5 RTC driver

[PATCH v3 0/2] Raspberry Pi 5 RTC driver

[Sander Speetjens]

The Raspberry Pi 5 has a battery-backed RTC located on the DA9091 PMIC accessed via the I2C bus[1].
Access to the RTC is provided exclusively through the VideoCore firmware
mailbox interface (not via a direct I2C register interface).

As stated by jamesh on the Rasperry Pi forum [2], "There are no plans to release a public datasheet for the DA9091." So we are stuck with the firmware interface for now. PhilE also states that "The firmware needs full control of the DA9091, sometimes with extremely low latency, otherwise there's a risk the supply could just cut the power."

This series adds:
  - a DT property for the charging voltage of the RTC battery
  - the RTC driver that talks to the firmware

The driver and binding are based on (and largely taken from) the
downstream Raspberry Pi kernel. The main changes for upstream are
the addition of a dt binding property for setting the charging voltage and
binding the driver from the raspberrypi-firmware driver instead of from
the DT.

The driver supports time read/write, alarm, and optional trickle
charging voltage configuration via the device-tree.

Testing
-------
- Hardware: Raspberry Pi 5 Model B revC
- Kernel: v7.2
- dt_binding_check: passed
- Basic RTC operations (read/write time, alarm) verified

[1] https://www.raspberrypi.com/news/introducing-raspberry-pi-5/
[2] https://forums.raspberrypi.com/viewtopic.php?t=359073

Sander Speetjens (2):
  dt-bindings: rtc: Add property for Raspberry Pi 5 RTC
  rtc: Add Raspberry Pi 5 RTC driver

 .../arm/bcm/raspberrypi,bcm2835-firmware.yaml |   6 +
 drivers/firmware/raspberrypi.c                |  25 ++
 drivers/rtc/Kconfig                           |  11 +
 drivers/rtc/Makefile                          |   1 +
 drivers/rtc/rtc-raspberrypi.c                 | 272 ++++++++++++++++++
 5 files changed, 315 insertions(+)
 create mode 100644 drivers/rtc/rtc-raspberrypi.c


base-commit: fd73f4a6659897191fa0d40695fe370925dd3780
-- 
2.55.0

[PATCH v3 1/2] dt-bindings: rtc: Add property for Raspberry Pi 5 RTC

[Sander Speetjens]

Add trickle-voltage-millivolt property for the Raspberry Pi 5 RTC.

V2 -> V3: Fixed the firmware property naming

V1 -> V2: As discussed with Krzysztof Kozlowski, this property is folded into
the existing raspberrypi,bcm2835-firmware properties.

Signed-off-by: Sander Speetjens <sander.speetjens@gmail.com>
---
 .../bindings/arm/bcm/raspberrypi,bcm2835-firmware.yaml      | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/bcm/raspberrypi,bcm2835-firmware.yaml b/Documentation/devicetree/bindings/arm/bcm/raspberrypi,bcm2835-firmware.yaml
index 983ea80eaec9..dc71fd57834e 100644
--- a/Documentation/devicetree/bindings/arm/bcm/raspberrypi,bcm2835-firmware.yaml
+++ b/Documentation/devicetree/bindings/arm/bcm/raspberrypi,bcm2835-firmware.yaml
@@ -133,6 +133,11 @@ properties:
     required:
       - compatible
 
+  trickle-voltage-millivolt:
+    $ref: /schemas/rtc/rtc.yaml#/properties/trickle-voltage-millivolt
+    minimum: 1300
+    maximum: 5000
+
 required:
   - compatible
   - mboxes
@@ -144,6 +149,7 @@ examples:
     firmware {
         compatible = "raspberrypi,bcm2835-firmware", "simple-mfd";
         mboxes = <&mailbox>;
+        trickle-voltage-millivolt = <3300>;
 
         firmware_clocks: clocks {
             compatible = "raspberrypi,firmware-clocks";
-- 
2.55.0

[PATCH v3 2/2] rtc: Add Raspberry Pi 5 RTC driver

[Sander Speetjens]

Upstreaming the downstream Raspberry Pi 5 RTC driver.
This driver supports the custom DA9091, which is accessed through the firmware mailbox.

V2 -> V3:
- Move platform check to firmware and abort before registering a platform device if on another platform
- Add a dependency on the Raspberry Pi firmware to Kconfig
- Check return value of devm_device_init_wakeup
- Fix property name

V1 -> V2:
Instead of the original driver, which was directly bound to the device tree. This driver is bound to the Raspberry Pi firmware device by creating a child device in the firmware driver probe function. The child device is then bound to this driver, which uses the firmware mailbox to access the RTC.

A couple of minor changes have been made to the driver since it was originally written, including:
- Checking if the model is a Raspberry Pi 5, as the RTC is only present on that model.
- Using the new devm_rpi_firmware_get() and devm_init_wakeup() helper to get the firmware device and avoid leaking memory.
- Using millivolts instead of microvolts for the trickle charge voltage, to match the RTC standard.
- Instead of setting the trickle charge voltage to 0 to disable trickle charging, the property is now optional. If the property is not present, trickle charging is disabled, per the RTC standard.
- Renaming the driver dt match compatible to "raspberrypi,firmware-rtc" to match the other firmware bindings.
- Renaming the driver name to "raspberrypi-rtc" to match the other firmware drivers.

Signed-off-by: Jonathan Bell <jonathan@raspberrypi.com>
Signed-off-by: Dom Cobley <popcornmix@gmail.com>

Signed-off-by: Sander Speetjens <sander.speetjens@gmail.com>
---
 drivers/firmware/raspberrypi.c |  25 +++
 drivers/rtc/Kconfig            |  11 ++
 drivers/rtc/Makefile           |   1 +
 drivers/rtc/rtc-raspberrypi.c  | 272 +++++++++++++++++++++++++++++++++
 4 files changed, 309 insertions(+)
 create mode 100644 drivers/rtc/rtc-raspberrypi.c

diff --git a/drivers/firmware/raspberrypi.c b/drivers/firmware/raspberrypi.c
index 0aa322e9a2e7..b2b11d71bb0d 100644
--- a/drivers/firmware/raspberrypi.c
+++ b/drivers/firmware/raspberrypi.c
@@ -24,6 +24,7 @@
 
 static struct platform_device *rpi_hwmon;
 static struct platform_device *rpi_clk;
+static struct platform_device *rpi_rtc;
 
 struct rpi_firmware {
 	struct mbox_client cl;
@@ -231,6 +232,27 @@ static void rpi_register_clk_driver(struct device *dev)
 						-1, NULL, 0);
 }
 
+static void rpi_register_rtc_driver(struct device *dev)
+{
+	struct device_node *firmware;
+	
+	// Check if our model is a Raspberry Pi 5, as the RTC is only present on that model.
+        const char *model = of_get_property(of_root, "model", NULL);
+        if (!model || strncmp(model, "Raspberry Pi 5", 14) != 0) {
+                return;
+        }
+
+	firmware = of_get_compatible_child(dev->of_node,
+					   "raspberrypi,firmware-rtc");
+	if (firmware) {
+		of_node_put(firmware);
+		return;
+	}
+
+	rpi_rtc = platform_device_register_data(dev, "raspberrypi-rtc",
+						-1, NULL, 0);
+}
+
 unsigned int rpi_firmware_clk_get_max_rate(struct rpi_firmware *fw, unsigned int id)
 {
 	struct rpi_firmware_clk_rate_request msg =
@@ -305,6 +327,7 @@ static int rpi_firmware_probe(struct platform_device *pdev)
 	rpi_firmware_print_firmware_revision(fw);
 	rpi_register_hwmon_driver(dev, fw);
 	rpi_register_clk_driver(dev);
+	rpi_register_rtc_driver(dev);
 
 	return 0;
 }
@@ -327,6 +350,8 @@ static void rpi_firmware_remove(struct platform_device *pdev)
 	rpi_hwmon = NULL;
 	platform_device_unregister(rpi_clk);
 	rpi_clk = NULL;
+	platform_device_unregister(rpi_rtc);
+	rpi_rtc = NULL;
 
 	rpi_firmware_put(fw);
 }
diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 05b9233b9418..1a24655fc2d3 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -1999,6 +1999,17 @@ config RTC_DRV_R7301
 	   This driver can also be built as a module. If so, the module
 	   will be called rtc-r7301.
 
+config RTC_DRV_RPI
+        tristate "Raspberry Pi RTC"
+        depends on ARCH_BRCMSTB || RASPBERRYPI_FIRMWARE || COMPILE_TEST
+        default ARCH_BRCMSTB
+        help
+          If you say yes here you get support for the RTC found on
+          Raspberry Pi devices.
+
+          This driver can also be built as a module. If so, the module
+          will be called rtc-rpi.
+
 config RTC_DRV_STM32
 	tristate "STM32 RTC"
 	select REGMAP_MMIO
diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
index 0347645b021f..46f1a0fc2416 100644
--- a/drivers/rtc/Makefile
+++ b/drivers/rtc/Makefile
@@ -144,6 +144,7 @@ obj-$(CONFIG_RTC_DRV_PS3)	+= rtc-ps3.o
 obj-$(CONFIG_RTC_DRV_PXA)	+= rtc-pxa.o
 obj-$(CONFIG_RTC_DRV_R7301)	+= rtc-r7301.o
 obj-$(CONFIG_RTC_DRV_R9701)	+= rtc-r9701.o
+obj-$(CONFIG_RTC_DRV_RPI)	+= rtc-raspberrypi.o
 obj-$(CONFIG_RTC_DRV_RC5T583)	+= rtc-rc5t583.o
 obj-$(CONFIG_RTC_DRV_RC5T619)	+= rtc-rc5t619.o
 obj-$(CONFIG_RTC_DRV_RK808)	+= rtc-rk808.o
diff --git a/drivers/rtc/rtc-raspberrypi.c b/drivers/rtc/rtc-raspberrypi.c
new file mode 100644
index 000000000000..0f5c3f8e9731
--- /dev/null
+++ b/drivers/rtc/rtc-raspberrypi.c
@@ -0,0 +1,272 @@
+// SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause
+/**
+ * rtc-rpi.c
+ *
+ * RTC driver using firmware mailbox
+ * Supports battery backed RTC and wake alarms
+ *
+ * Based on rtc-meson-vrtc by Neil Armstrong
+ *
+ * Copyright (c) 2023, Raspberry Pi Ltd.
+ */
+
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/rtc.h>
+#include <linux/of.h>
+#include <soc/bcm2835/raspberrypi-firmware.h>
+
+struct rpi_rtc_data {
+	struct rtc_device *rtc;
+	struct rpi_firmware *fw;
+	u32 bbat_vchg_millivolts;
+};
+
+#define RPI_FIRMWARE_GET_RTC_REG 0x00030087
+#define RPI_FIRMWARE_SET_RTC_REG 0x00038087
+
+enum {
+	RTC_TIME,
+	RTC_ALARM,
+	RTC_ALARM_PENDING,
+	RTC_ALARM_ENABLE,
+	RTC_BBAT_CHG_VOLTS,
+	RTC_BBAT_CHG_VOLTS_MIN,
+	RTC_BBAT_CHG_VOLTS_MAX,
+	RTC_BBAT_VOLTS
+};
+
+static int rpi_rtc_read_time(struct device *dev, struct rtc_time *tm)
+{
+	struct rpi_rtc_data *vrtc = dev_get_drvdata(dev);
+	u32 data[2] = {RTC_TIME};
+	int err;
+
+	err = rpi_firmware_property(vrtc->fw, RPI_FIRMWARE_GET_RTC_REG,
+				    &data, sizeof(data));
+	rtc_time64_to_tm(data[1], tm);
+	return err;
+}
+
+static int rpi_rtc_set_time(struct device *dev, struct rtc_time *tm)
+{
+	struct rpi_rtc_data *vrtc = dev_get_drvdata(dev);
+	u32 data[2] = {RTC_TIME, rtc_tm_to_time64(tm)};
+
+	return rpi_firmware_property(vrtc->fw, RPI_FIRMWARE_SET_RTC_REG,
+				     &data, sizeof(data));
+}
+
+static int rpi_rtc_alarm_irq_is_enabled(struct device *dev, unsigned char *enabled)
+{
+	struct rpi_rtc_data *vrtc = dev_get_drvdata(dev);
+	u32 data[2] = {RTC_ALARM_ENABLE};
+	s32 err = 0;
+
+	err = rpi_firmware_property(vrtc->fw, RPI_FIRMWARE_GET_RTC_REG,
+				    &data, sizeof(data));
+	*enabled = data[1] & 0x1;
+	return err;
+}
+
+static int rpi_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled)
+{
+	struct rpi_rtc_data *vrtc = dev_get_drvdata(dev);
+	u32 data[2] = {RTC_ALARM_ENABLE, enabled};
+
+	return rpi_firmware_property(vrtc->fw, RPI_FIRMWARE_SET_RTC_REG,
+				     &data, sizeof(data));
+}
+
+static int rpi_rtc_alarm_clear_pending(struct device *dev)
+{
+	struct rpi_rtc_data *vrtc = dev_get_drvdata(dev);
+	u32 data[2] = {RTC_ALARM_PENDING, 1};
+
+	return rpi_firmware_property(vrtc->fw, RPI_FIRMWARE_SET_RTC_REG,
+				     &data, sizeof(data));
+}
+
+static int rpi_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alarm)
+{
+	struct rpi_rtc_data *vrtc = dev_get_drvdata(dev);
+	u32 data[2] = {RTC_ALARM};
+	s32 err = 0;
+
+	err = rpi_rtc_alarm_irq_is_enabled(dev, &alarm->enabled);
+	if (!err)
+		err = rpi_firmware_property(vrtc->fw, RPI_FIRMWARE_GET_RTC_REG,
+					    &data, sizeof(data));
+	rtc_time64_to_tm(data[1], &alarm->time);
+
+	return err;
+}
+
+static int rpi_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alarm)
+{
+	struct rpi_rtc_data *vrtc = dev_get_drvdata(dev);
+	u32 data[2] = {RTC_ALARM, rtc_tm_to_time64(&alarm->time)};
+	int err;
+
+	err = rpi_firmware_property(vrtc->fw, RPI_FIRMWARE_SET_RTC_REG,
+				    &data, sizeof(data));
+
+	if (err == 0)
+		err = rpi_rtc_alarm_irq_enable(dev, alarm->enabled);
+
+	return err;
+}
+
+static const struct rtc_class_ops rpi_rtc_ops = {
+	.read_time = rpi_rtc_read_time,
+	.set_time = rpi_rtc_set_time,
+	.read_alarm = rpi_rtc_read_alarm,
+	.set_alarm = rpi_rtc_set_alarm,
+	.alarm_irq_enable = rpi_rtc_alarm_irq_enable,
+};
+
+static int rpi_rtc_set_charge_voltage(struct device *dev)
+{
+	struct rpi_rtc_data *vrtc = dev_get_drvdata(dev);
+	u32 data[2] = {RTC_BBAT_CHG_VOLTS, vrtc->bbat_vchg_millivolts * 1000U};
+	int err;
+
+	err = rpi_firmware_property(vrtc->fw, RPI_FIRMWARE_SET_RTC_REG,
+				    &data, sizeof(data));
+
+	if (err)
+		dev_err(dev, "failed to set trickle charge voltage to %umV: %d\n",
+			vrtc->bbat_vchg_millivolts, err);
+	else if (vrtc->bbat_vchg_millivolts)
+		dev_info(dev, "trickle charging enabled at %umV\n",
+			 vrtc->bbat_vchg_millivolts);
+
+	return err;
+}
+
+static ssize_t rpi_rtc_print_uint_reg(struct device *dev, char *buf, u32 reg)
+{
+	struct rpi_rtc_data *vrtc = dev_get_drvdata(dev->parent);
+	u32 data[2] = {reg, 0};
+	int ret = 0;
+
+	ret = rpi_firmware_property(vrtc->fw, RPI_FIRMWARE_GET_RTC_REG,
+				    &data, sizeof(data));
+	if (ret < 0)
+		return ret;
+
+	return sprintf(buf, "%u\n", data[1]);
+}
+
+static ssize_t charging_voltage_show(struct device *dev,
+				     struct device_attribute *attr,
+				     char *buf)
+{
+	return rpi_rtc_print_uint_reg(dev, buf, RTC_BBAT_CHG_VOLTS);
+}
+static DEVICE_ATTR_RO(charging_voltage);
+
+static ssize_t charging_voltage_min_show(struct device *dev,
+					 struct device_attribute *attr,
+					 char *buf)
+{
+	return rpi_rtc_print_uint_reg(dev, buf, RTC_BBAT_CHG_VOLTS_MIN);
+}
+static DEVICE_ATTR_RO(charging_voltage_min);
+
+static ssize_t charging_voltage_max_show(struct device *dev,
+					 struct device_attribute *attr,
+					 char *buf)
+{
+	return rpi_rtc_print_uint_reg(dev, buf, RTC_BBAT_CHG_VOLTS_MAX);
+}
+static DEVICE_ATTR_RO(charging_voltage_max);
+
+static ssize_t battery_voltage_show(struct device *dev,
+				    struct device_attribute *attr,
+				    char *buf)
+{
+	return rpi_rtc_print_uint_reg(dev, buf, RTC_BBAT_VOLTS);
+}
+static DEVICE_ATTR_RO(battery_voltage);
+
+static struct attribute *rpi_rtc_attrs[] = {
+	&dev_attr_charging_voltage.attr,
+	&dev_attr_charging_voltage_min.attr,
+	&dev_attr_charging_voltage_max.attr,
+	&dev_attr_battery_voltage.attr,
+	NULL
+};
+
+static const struct attribute_group rpi_rtc_sysfs_files = {
+	.attrs = rpi_rtc_attrs,
+};
+
+static int rpi_rtc_probe(struct platform_device *pdev)
+{
+	struct rpi_rtc_data *vrtc;
+	struct device *dev = &pdev->dev;
+	struct rpi_firmware *firmware;
+	int ret;
+
+	// Get the firmware device from the parent device
+	firmware = devm_rpi_firmware_get(dev, dev->parent->of_node);
+
+	if (!firmware)
+		return -EPROBE_DEFER;
+
+	vrtc = devm_kzalloc(dev, sizeof(*vrtc), GFP_KERNEL);
+	if (!vrtc)
+		return -ENOMEM;
+
+	vrtc->fw = firmware;
+
+	ret = devm_device_init_wakeup(dev);
+	if (ret)
+		return ret;
+
+	platform_set_drvdata(pdev, vrtc);
+
+	vrtc->rtc = devm_rtc_allocate_device(dev);
+	if (IS_ERR(vrtc->rtc))
+		return PTR_ERR(vrtc->rtc);
+
+	vrtc->rtc->range_max = U32_MAX;           /* 2106-02-07 */
+
+	set_bit(RTC_FEATURE_ALARM_WAKEUP_ONLY, vrtc->rtc->features);
+	clear_bit(RTC_FEATURE_UPDATE_INTERRUPT, vrtc->rtc->features);
+
+	vrtc->rtc->ops = &rpi_rtc_ops;
+	ret = rtc_add_group(vrtc->rtc, &rpi_rtc_sysfs_files);
+	if (ret)
+		return ret;
+
+	rpi_rtc_alarm_clear_pending(dev);
+
+	vrtc->bbat_vchg_millivolts = 0;
+	of_property_read_u32(dev->parent->of_node, "trickle-voltage-millivolt",
+			&vrtc->bbat_vchg_millivolts);
+
+	rpi_rtc_set_charge_voltage(dev);
+
+	return devm_rtc_register_device(vrtc->rtc);
+}
+
+static const struct of_device_id rpi_rtc_dt_match[] = {
+	{ .compatible = "raspberrypi,firmware-rtc"},
+	{},
+};
+MODULE_DEVICE_TABLE(of, rpi_rtc_dt_match);
+
+static struct platform_driver rpi_rtc_driver = {
+	.probe = rpi_rtc_probe,
+	.driver = {
+		.name = "raspberrypi-rtc",
+		.of_match_table = rpi_rtc_dt_match,
+	},
+};
+
+module_platform_driver(rpi_rtc_driver);
+
+MODULE_DESCRIPTION("Raspberry Pi RTC driver");
+MODULE_LICENSE("GPL");
-- 
2.55.0

Re: [PATCH v3 1/2] dt-bindings: rtc: Add property for Raspberry Pi 5 RTC

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] dt-bindings: constraint bypass due to missing scalar type on -millivolt property
--

commit a0ac03cb8b207820432d22540ca183fbaafbeb38
Author: Sander Speetjens <sander.speetjens@gmail.com>

dt-bindings: rtc: Add property for Raspberry Pi 5 RTC
    
This commit adds the trickle-voltage-millivolt property to the
raspberrypi,bcm2835-firmware binding for the Raspberry Pi 5 RTC. It also
introduces minimum and maximum voltage constraints for this property.

> diff --git a/Documentation/devicetree/bindings/arm/bcm/raspberrypi,bcm2835-firmware.yaml b/Documentation/devicetree/bindings/arm/bcm/raspberrypi,bcm2835-firmware.yaml
> index 983ea80eaec97..dc71fd57834e8 100644
> --- a/Documentation/devicetree/bindings/arm/bcm/raspberrypi,bcm2835-firmware.yaml
> +++ b/Documentation/devicetree/bindings/arm/bcm/raspberrypi,bcm2835-firmware.yaml
> @@ -133,6 +133,11 @@ properties:
>      required:
>        - compatible
>  
> +  trickle-voltage-millivolt:
> +    $ref: /schemas/rtc/rtc.yaml#/properties/trickle-voltage-millivolt
> +    minimum: 1300
> +    maximum: 5000

[Severity: Medium]
Are these constraints applied effectively here?

Since dtschema's property-units.yaml assigns a default array type to
properties ending in -millivolt, applying minimum and maximum constraints
here without explicitly redefining the property as a scalar type (e.g.,
via $ref: /schemas/types.yaml#/definitions/uint32) can cause JSON Schema
to silently bypass these constraints.

Could device trees with out-of-bounds voltages pass validation when
dt_binding_check evaluates the constraints against the default array type?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260917135841.841680-1-sander.speetjens@gmail.com?part=1

Re: [PATCH v3 2/2] rtc: Add Raspberry Pi 5 RTC driver

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Endianness mismatch in Raspberry Pi firmware mailbox payloads
- [Low] Kconfig dependency allows built-in RTC driver with modular firmware
--

commit b1ad8443ceac920191fe490d4fe9a93bc6d9205a
Author: Sander Speetjens <sander.speetjens@gmail.com>

rtc: Add Raspberry Pi 5 RTC driver

This commit upstreams the downstream Raspberry Pi 5 RTC driver for the custom
DA9091. The driver binds as a child device in the firmware driver probe
function and uses the firmware mailbox to access the RTC.

[ ... ]
> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index 05b9233b94189..1a24655fc2d39 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -1999,6 +1999,17 @@ config RTC_DRV_R7301
>  	   This driver can also be built as a module. If so, the module
>  	   will be called rtc-r7301.
>  
> +config RTC_DRV_RPI
> +        tristate "Raspberry Pi RTC"
> +        depends on ARCH_BRCMSTB || RASPBERRYPI_FIRMWARE || COMPILE_TEST

[Severity: Low]
Does this dependency allow the RTC driver to be compiled as built-in (=y)
while RASPBERRYPI_FIRMWARE is compiled as a loadable module (=m)?

If this driver is built-in and the firmware is modular, it looks like the
unconditional calls to devm_rpi_firmware_get() and rpi_firmware_property()
in rpi_rtc_probe() will result in a linker error.

> +        default ARCH_BRCMSTB
> +        help
> +          If you say yes here you get support for the RTC found on
> +          Raspberry Pi devices.
[ ... ]
> diff --git a/drivers/rtc/rtc-raspberrypi.c b/drivers/rtc/rtc-raspberrypi.c
> new file mode 100644
> index 0000000000000..0f5c3f8e9731e
> --- /dev/null
> +++ b/drivers/rtc/rtc-raspberrypi.c
[ ... ]
> +static int rpi_rtc_set_time(struct device *dev, struct rtc_time *tm)
> +{
> +	struct rpi_rtc_data *vrtc = dev_get_drvdata(dev);
> +	u32 data[2] = {RTC_TIME, rtc_tm_to_time64(tm)};

[Severity: High]
Can this payload format cause communication issues on big-endian architectures?

The Raspberry Pi firmware mailbox expects little-endian data. By passing
a host-endian u32 array directly to rpi_firmware_property(), both the
register ID (RTC_TIME) and the timestamp payload will be byte-swapped on
big-endian machines.

> +
> +	return rpi_firmware_property(vrtc->fw, RPI_FIRMWARE_SET_RTC_REG,
> +				     &data, sizeof(data));
> +}
[ ... ]
> +static int rpi_rtc_set_charge_voltage(struct device *dev)
> +{
> +	struct rpi_rtc_data *vrtc = dev_get_drvdata(dev);
> +	u32 data[2] = {RTC_BBAT_CHG_VOLTS, vrtc->bbat_vchg_millivolts * 1000U};

[Severity: High]
Will this conversion to a host-endian integer cause out-of-spec PMIC
voltages to be sent on big-endian machines?

Similar to rpi_rtc_set_time(), passing host-endian values to the mailbox
here will byte-swap the charge voltage target on big-endian systems,
potentially resulting in hardware misconfiguration.

> +	int err;
> +
> +	err = rpi_firmware_property(vrtc->fw, RPI_FIRMWARE_SET_RTC_REG,
> +				    &data, sizeof(data));
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260917135841.841680-1-sander.speetjens@gmail.com?part=2

Re: [PATCH v3 1/2] dt-bindings: rtc: Add property for Raspberry Pi 5 RTC

[Krzysztof Kozlowski]

On Thu, Sep 17, 2026 at 03:58:40PM +0200, Sander Speetjens wrote:
> Add trickle-voltage-millivolt property for the Raspberry Pi 5 RTC.

Why? Describe the hardware, the need, not the diff.

> 
> V2 -> V3: Fixed the firmware property naming


Changelog goes to the changelog area (under ---). Please look at other
patches how they do it if you are uncertain.

Best regards,
Krzysztof

Re: [PATCH v3 2/2] rtc: Add Raspberry Pi 5 RTC driver

[Krzysztof Kozlowski]

On Thu, Sep 17, 2026 at 03:58:41PM +0200, Sander Speetjens wrote:
> +	return devm_rtc_register_device(vrtc->rtc);
> +}
> +
> +static const struct of_device_id rpi_rtc_dt_match[] = {
> +	{ .compatible = "raspberrypi,firmware-rtc"},

Please fixup white space issues, see existing drivers (missing space
before }

> +	{},

and unnecessary ,.

Just take existing latest code as starting point, otherwise you
reintroduce discouraged patterns.

More important: undocumented ABI.

Please run scripts/checkpatch.pl on the patches and fix reported
warnings. After that, run also 'scripts/checkpatch.pl --strict' on the
patches and (probably) fix more warnings. Some warnings can be ignored,
especially from --strict run, but the code here looks like it needs a
fix. Feel free to get in touch if the warning is not clear.

Best regards,
Krzysztof

Re: [PATCH v3 1/2] dt-bindings: rtc: Add property for Raspberry Pi 5 RTC

[Stefan Wahren]

Am 18.09.26 um 13:44 schrieb Krzysztof Kozlowski:
> On Thu, Sep 17, 2026 at 03:58:40PM +0200, Sander Speetjens wrote:
>> Add trickle-voltage-millivolt property for the Raspberry Pi 5 RTC.
> Why? Describe the hardware, the need, not the diff.
Is this something we can avoid by making this configurable via userspace 
/ hw_clock --param-set=parameter=value ?

Re: [PATCH v3 1/2] dt-bindings: rtc: Add property for Raspberry Pi 5 RTC

[Sander Speetjens]

> On 9/18/26 15:27, Stefan Wahren wrote:
>> Am 18.09.26 um 13:44 schrieb Krzysztof Kozlowski:
>>> On Thu, Sep 17, 2026 at 03:58:40PM +0200, Sander Speetjens wrote:
>>>> Add trickle-voltage-millivolt property for the Raspberry Pi 5 RTC.
>>> Why? Describe the hardware, the need, not the diff.
>> Is this something we can avoid by making this configurable via 
>> userspace / hw_clock --param-set=parameter=value ? 
I think it would be handy to have both.
A cm5 board designer could choose to have a super-cap or lithium ion cell
and for that a device-tree property would be handy for it is always the 
same.
But for a user attachable battery on the pi 5b via headers or soldering
the userspace version would be easier.

Re: [PATCH v3 1/2] dt-bindings: rtc: Add property for Raspberry Pi 5 RTC

[Alexandre Belloni]

On 18/09/2026 15:27:55+0200, Stefan Wahren wrote:
> Am 18.09.26 um 13:44 schrieb Krzysztof Kozlowski:
> > On Thu, Sep 17, 2026 at 03:58:40PM +0200, Sander Speetjens wrote:
> > > Add trickle-voltage-millivolt property for the Raspberry Pi 5 RTC.
> > Why? Describe the hardware, the need, not the diff.
> Is this something we can avoid by making this configurable via userspace /
> hw_clock --param-set=parameter=value ?

I don't think so, the battery, once select is unlikely to change and so
having it is the device tree is describing the HW properly.


-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v3 2/2] rtc: Add Raspberry Pi 5 RTC driver

[Alexandre Belloni]

On 18/09/2026 13:47:22+0200, Krzysztof Kozlowski wrote:
> On Thu, Sep 17, 2026 at 03:58:41PM +0200, Sander Speetjens wrote:
> > +	return devm_rtc_register_device(vrtc->rtc);
> > +}
> > +
> > +static const struct of_device_id rpi_rtc_dt_match[] = {
> > +	{ .compatible = "raspberrypi,firmware-rtc"},
> 
> Please fixup white space issues, see existing drivers (missing space
> before }
> 
> > +	{},
> 
> and unnecessary ,.
> 
> Just take existing latest code as starting point, otherwise you
> reintroduce discouraged patterns.
> 
> More important: undocumented ABI.
> 

This was my comment on the previous revision, I'm not sure why it was
ignored.


-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v3 2/2] rtc: Add Raspberry Pi 5 RTC driver

[Sander Speetjens]

> This was my comment on the previous revision, I'm not sure why it was
> ignored.
Looks like that email got lost on my end, I found it in the archive.

> This undocumented sysfs interface has to be removed.
Just to make sure I understand correctly: does "This undocumented
sysfs interface has to be removed" mean that
   a) the attributes themselves are not wanted and should be dropped, or
   b) they are acceptable but must be properly documented under
      Documentation/ABI/ before they can stay?

Kind regards
Sander Speetjens

On 9/21/26 20:59, Alexandre Belloni wrote:
> On 18/09/2026 13:47:22+0200, Krzysztof Kozlowski wrote:
>> On Thu, Sep 17, 2026 at 03:58:41PM +0200, Sander Speetjens wrote:
>>> +	return devm_rtc_register_device(vrtc->rtc);
>>> +}
>>> +
>>> +static const struct of_device_id rpi_rtc_dt_match[] = {
>>> +	{ .compatible = "raspberrypi,firmware-rtc"},
>> Please fixup white space issues, see existing drivers (missing space
>> before }
>>
>>> +	{},
>> and unnecessary ,.
>>
>> Just take existing latest code as starting point, otherwise you
>> reintroduce discouraged patterns.
>>
>> More important: undocumented ABI.
>>
> This was my comment on the previous revision, I'm not sure why it was
> ignored.
>
>

Re: [PATCH v3 2/2] rtc: Add Raspberry Pi 5 RTC driver

[Alexandre Belloni]

On 21/09/2026 21:36:27+0200, Sander Speetjens wrote:
> > This was my comment on the previous revision, I'm not sure why it was
> > ignored.
> Looks like that email got lost on my end, I found it in the archive.
> 
> > This undocumented sysfs interface has to be removed.
> Just to make sure I understand correctly: does "This undocumented
> sysfs interface has to be removed" mean that
>   a) the attributes themselves are not wanted and should be dropped, or
>   b) they are acceptable but must be properly documented under
>      Documentation/ABI/ before they can stay?
> 

This won't get in unless the ABI is proven to be useful to multiple
RTCs. As-is, this can go to debugfs if you think you really need it but
I doubt this is going to be used ever.

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com