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

[sashiko-bot@kernel.org]

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