Re: [PATCH v10 3/3] rtc: Add Raspberry Pi 5 RTC driver

[Stefan Wahren <wahrenst@gmx.net>]

Hello Sander,

Am 02.10.26 um 12:24 schrieb Sander Speetjens:
> Upstreaming the downstream Raspberry Pi 5 RTC driver.
> This driver supports the custom DA9091,
> which is accessed through the firmware mailbox.
>
> Based on code originally written by Jonathan Bell <jonathan@raspberrypi.com>
> and Dom Cobley <popcornmix@gmail.com>
>
> Signed-off-by: Sander Speetjens <sander.speetjens@gmail.com>
in general this patch looks good except a few nits, maybe wait for 
Alexandre's feedback.
After fixing the nits you can add my:

Reviewed-by: Stefan Wahren <wahrenst@gmx.net>
> ---
> V9 -> V10:
> - Fix sign-off-by
> - Change enum to defines
> - Change comment in firmware
>
> V8 -> V9:
> - Fix whitespace in Kconfig
> - Move RTC regs to firmware header
> - Add a warn when getting the min or max charging values fails
> - Remove the unused return value of set charge voltage
>
> V7 -> V8:
> - Fix whitespacing errors
> - Use of_machine_is_compatible instead of strncmp
>
> V6 -> V7:
> - Move from u32 array to struct
> - Fix encapsulation as mentioned by sashiko-bot
>
> V5 -> V6:
> - Add MODULE_ALIAS, as mentioned by sashiko-bot
> - Encapsulate the mailbox data in le32_to_cpu or cpu_to_le32, as
>    mentioned by sashiko-bot. For good practice
>
> V4 -> V5: No changes
>
> V3 -> V4:
> - Fix Kconfig depends that disallows having Raspberrypi
> firmware built as a module while the rtc driver is built in
> - Remove sysfs
> - Add limits from firmware properties and check in rpi_rtc_set_charge_voltage
>
> 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.
>
>   drivers/firmware/raspberrypi.c             |  24 ++
>   drivers/rtc/Kconfig                        |  12 +
>   drivers/rtc/Makefile                       |   1 +
>   drivers/rtc/rtc-raspberrypi.c              | 276 +++++++++++++++++++++
>   include/soc/bcm2835/raspberrypi-firmware.h |   2 +
>   5 files changed, 315 insertions(+)
>   create mode 100644 drivers/rtc/rtc-raspberrypi.c
>
> diff --git a/drivers/firmware/raspberrypi.c b/drivers/firmware/raspberrypi.c
> index 0aa322e9a2e7..f8c83c25eccc 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,26 @@ 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 of Raspberry Pi board is from the 5th gen
> +	// as the RTC is only present on that model.
> +	if (!of_machine_is_compatible("brcm,bcm2712"))
> +		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 +326,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 +349,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..6dae4c069be2 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -1999,6 +1999,18 @@ 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 RASPBERRYPI_FIRMWARE || (COMPILE_TEST && !RASPBERRYPI_FIRMWARE)
> +	depends on ARCH_BRCMSTB || 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-raspberrypi.
Sorry for my misleading wording. I meant in this case 1 tab and align 
the rest with spaces just like the other.
...
> diff --git a/include/soc/bcm2835/raspberrypi-firmware.h b/include/soc/bcm2835/raspberrypi-firmware.h
> index 975bef529854..e13994d8281c 100644
> --- a/include/soc/bcm2835/raspberrypi-firmware.h
> +++ b/include/soc/bcm2835/raspberrypi-firmware.h
> @@ -75,6 +75,8 @@ enum rpi_firmware_property_tag {
>   	RPI_FIRMWARE_GET_THROTTLED =                          0x00030046,
>   	RPI_FIRMWARE_GET_CLOCK_MEASURED =                     0x00030047,
>   	RPI_FIRMWARE_NOTIFY_REBOOT =                          0x00030048,
> +	RPI_FIRMWARE_GET_RTC_REG =                            0x00030087,
> +	RPI_FIRMWARE_SET_RTC_REG =                            0x00038087,
Please try to keep the value order. I know it's already messed up, 
Gregor Herburger send a patch which already address the existing order 
("firmware: raspberrypi: reorder rpi_firmware_property_tag enum").

Best regards
>   	RPI_FIRMWARE_SET_CLOCK_STATE =                        0x00038001,
>   	RPI_FIRMWARE_SET_CLOCK_RATE =                         0x00038002,
>   	RPI_FIRMWARE_SET_VOLTAGE =                            0x00038003,