diff mbox series

[v2,02/11] mfd: renesas-vbattb: Add a MFD driver for the Renesas VBATTB IP

Message ID 20240716103025.1198495-3-claudiu.beznea.uj@bp.renesas.com
State Handled Elsewhere
Headers show
Series Add RTC support for the Renesas RZ/G3S SoC | expand

Commit Message

claudiu beznea July 16, 2024, 10:30 a.m. UTC
From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Renesas VBATTB IP has logic to control the RTC clock, tamper detection
and a small 128B memory. Add a MFD driver to do the basic initialization
of the VBATTB IP for the inner components to work.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v2:
- none; this driver is new

 drivers/mfd/Kconfig          |  8 ++++
 drivers/mfd/Makefile         |  1 +
 drivers/mfd/renesas-vbattb.c | 78 ++++++++++++++++++++++++++++++++++++
 3 files changed, 87 insertions(+)
 create mode 100644 drivers/mfd/renesas-vbattb.c

Comments

Biju Das July 16, 2024, 11 a.m. UTC | #1
Hi Claudiu,

Thanks for the patch.


> -----Original Message-----
> From: Claudiu <claudiu.beznea@tuxon.dev>
> Sent: Tuesday, July 16, 2024 11:30 AM
> Subject: [PATCH v2 02/11] mfd: renesas-vbattb: Add a MFD driver for the Renesas VBATTB IP
> 
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Renesas VBATTB IP has logic to control the RTC clock, tamper detection and a small 128B memory. Add a
> MFD driver to do the basic initialization of the VBATTB IP for the inner components to work.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---
> 
> Changes in v2:
> - none; this driver is new
> 
>  drivers/mfd/Kconfig          |  8 ++++
>  drivers/mfd/Makefile         |  1 +
>  drivers/mfd/renesas-vbattb.c | 78 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 87 insertions(+)
>  create mode 100644 drivers/mfd/renesas-vbattb.c
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index bc8be2e593b6..df93e8b05065 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1383,6 +1383,14 @@ config MFD_SC27XX_PMIC
>  	  This driver provides common support for accessing the SC27xx PMICs,
>  	  and it also adds the irq_chip parts for handling the PMIC chip events.
> 
> +config MFD_RENESAS_VBATTB
> +	tristate "Renesas VBATTB driver"
> +	depends on (ARCH_RZG2L && OF) || COMPILE_TEST
> +	select MFD_CORE

There is no MFD calls??  What is the purpose of selecting MFD_CORE??

> +	help
> +	  Select this option to enable Renesas RZ/G3S VBATTB driver which
> +	  provides support for the RTC clock, tamper detector and 128B SRAM.
> +
>  config RZ_MTU3
>  	tristate "Renesas RZ/G2L MTU3a core driver"
>  	depends on (ARCH_RZG2L && OF) || COMPILE_TEST diff --git a/drivers/mfd/Makefile
> b/drivers/mfd/Makefile index 02b651cd7535..cd2f27492df2 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -186,6 +186,7 @@ pcf50633-objs			:= pcf50633-core.o pcf50633-irq.o
>  obj-$(CONFIG_MFD_PCF50633)	+= pcf50633.o
>  obj-$(CONFIG_PCF50633_ADC)	+= pcf50633-adc.o
>  obj-$(CONFIG_PCF50633_GPIO)	+= pcf50633-gpio.o
> +obj-$(CONFIG_MFD_RENESAS_VBATTB)	+= renesas-vbattb.o
>  obj-$(CONFIG_RZ_MTU3)		+= rz-mtu3.o
>  obj-$(CONFIG_ABX500_CORE)	+= abx500-core.o
>  obj-$(CONFIG_MFD_DB8500_PRCMU)	+= db8500-prcmu.o
> diff --git a/drivers/mfd/renesas-vbattb.c b/drivers/mfd/renesas-vbattb.c new file mode 100644 index
> 000000000000..5d71565b8cbf
> --- /dev/null
> +++ b/drivers/mfd/renesas-vbattb.c
> @@ -0,0 +1,78 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * VBATTB driver
> + *
> + * Copyright (C) 2024 Renesas Electronics Corp.
> + */
> +
> +#include <linux/mod_devicetable.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/reset.h>
> +
> +static int vbattb_probe(struct platform_device *pdev) {
> +	struct device *dev = &pdev->dev;
> +	struct reset_control *rstc;
> +	int ret;
> +
> +	rstc = devm_reset_control_array_get_exclusive(dev);
> +	if (IS_ERR(rstc))
> +		return PTR_ERR(rstc);
> +
> +	ret = devm_pm_runtime_enable(dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = reset_control_deassert(rstc);
> +	if (ret)
> +		goto rpm_put;
> +
> +	platform_set_drvdata(pdev, rstc);
> +
> +	ret = devm_of_platform_populate(dev);
> +	if (ret)
> +		goto reset_assert;
> +
> +	return 0;
> +
> +reset_assert:
> +	reset_control_assert(rstc);
> +rpm_put:
> +	pm_runtime_put(dev);
> +	return ret;
> +}
> +
> +static void vbattb_remove(struct platform_device *pdev) {
> +	struct reset_control *rstc = platform_get_drvdata(pdev);
> +
> +	reset_control_assert(rstc);
> +	pm_runtime_put(&pdev->dev);
> +}
> +
> +static const struct of_device_id vbattb_match[] = {
> +	{ .compatible = "renesas,r9a08g045-vbattb" },
> +	{ /* sentinel */ },

Drop comma.

> +};
> +MODULE_DEVICE_TABLE(of, vbattb_match);
> +
> +static struct platform_driver vbattb_driver = {
> +	.probe = vbattb_probe,
> +	.remove_new = vbattb_remove,

Maybe remove canbe replaced with devm_add_action_or_reset()
That simplifies probe() aswell??

> +	.driver = {
> +		.name = "renesas-vbattb",
> +		.of_match_table = vbattb_match,
> +	},
> +};
> +module_platform_driver(vbattb_driver);
> +
> +MODULE_ALIAS("platform:renesas-vbattb");
> +MODULE_AUTHOR("Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>");
> +MODULE_DESCRIPTION("Renesas VBATTB driver"); MODULE_LICENSE("GPL");
> --
> 2.39.2
>
Biju Das July 16, 2024, 11:07 a.m. UTC | #2
Hi Claudiu,

> -----Original Message-----
> From: Claudiu <claudiu.beznea@tuxon.dev>
> Sent: Tuesday, July 16, 2024 11:30 AM
> Subject: [PATCH v2 02/11] mfd: renesas-vbattb: Add a MFD driver for the Renesas VBATTB IP
> 
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Renesas VBATTB IP has logic to control the RTC clock, tamper detection and a small 128B memory. Add a
> MFD driver to do the basic initialization of the VBATTB IP for the inner components to work.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---
> 
> Changes in v2:
> - none; this driver is new
> 
>  drivers/mfd/Kconfig          |  8 ++++
>  drivers/mfd/Makefile         |  1 +
>  drivers/mfd/renesas-vbattb.c | 78 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 87 insertions(+)
>  create mode 100644 drivers/mfd/renesas-vbattb.c
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index bc8be2e593b6..df93e8b05065 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1383,6 +1383,14 @@ config MFD_SC27XX_PMIC
>  	  This driver provides common support for accessing the SC27xx PMICs,
>  	  and it also adds the irq_chip parts for handling the PMIC chip events.
> 
> +config MFD_RENESAS_VBATTB
> +	tristate "Renesas VBATTB driver"
> +	depends on (ARCH_RZG2L && OF) || COMPILE_TEST
> +	select MFD_CORE
> +	help
> +	  Select this option to enable Renesas RZ/G3S VBATTB driver which
> +	  provides support for the RTC clock, tamper detector and 128B SRAM.
> +
>  config RZ_MTU3
>  	tristate "Renesas RZ/G2L MTU3a core driver"
>  	depends on (ARCH_RZG2L && OF) || COMPILE_TEST diff --git a/drivers/mfd/Makefile
> b/drivers/mfd/Makefile index 02b651cd7535..cd2f27492df2 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -186,6 +186,7 @@ pcf50633-objs			:= pcf50633-core.o pcf50633-irq.o
>  obj-$(CONFIG_MFD_PCF50633)	+= pcf50633.o
>  obj-$(CONFIG_PCF50633_ADC)	+= pcf50633-adc.o
>  obj-$(CONFIG_PCF50633_GPIO)	+= pcf50633-gpio.o
> +obj-$(CONFIG_MFD_RENESAS_VBATTB)	+= renesas-vbattb.o
>  obj-$(CONFIG_RZ_MTU3)		+= rz-mtu3.o
>  obj-$(CONFIG_ABX500_CORE)	+= abx500-core.o
>  obj-$(CONFIG_MFD_DB8500_PRCMU)	+= db8500-prcmu.o
> diff --git a/drivers/mfd/renesas-vbattb.c b/drivers/mfd/renesas-vbattb.c new file mode 100644 index
> 000000000000..5d71565b8cbf
> --- /dev/null
> +++ b/drivers/mfd/renesas-vbattb.c
> @@ -0,0 +1,78 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * VBATTB driver
> + *
> + * Copyright (C) 2024 Renesas Electronics Corp.
> + */
> +
> +#include <linux/mod_devicetable.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/reset.h>
> +
> +static int vbattb_probe(struct platform_device *pdev) {
> +	struct device *dev = &pdev->dev;
> +	struct reset_control *rstc;
> +	int ret;
> +
> +	rstc = devm_reset_control_array_get_exclusive(dev);
> +	if (IS_ERR(rstc))
> +		return PTR_ERR(rstc);
> +
> +	ret = devm_pm_runtime_enable(dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret)
> +		return ret;

Maybe as an optimization drop pm calls and use "devm_clk_get_enabled"
instead as it perfectly fits in this scenario??

> +
> +	ret = reset_control_deassert(rstc);
> +	if (ret)
> +		goto rpm_put;
> +
> +	platform_set_drvdata(pdev, rstc);
> +
> +	ret = devm_of_platform_populate(dev);
> +	if (ret)
> +		goto reset_assert;
> +
> +	return 0;
> +
> +reset_assert:
> +	reset_control_assert(rstc);
> +rpm_put:
> +	pm_runtime_put(dev);
> +	return ret;
> +}
> +
> +static void vbattb_remove(struct platform_device *pdev) {
> +	struct reset_control *rstc = platform_get_drvdata(pdev);
> +
> +	reset_control_assert(rstc);
> +	pm_runtime_put(&pdev->dev);
> +}
> +
> +static const struct of_device_id vbattb_match[] = {
> +	{ .compatible = "renesas,r9a08g045-vbattb" },
> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, vbattb_match);
> +
> +static struct platform_driver vbattb_driver = {
> +	.probe = vbattb_probe,
> +	.remove_new = vbattb_remove,
> +	.driver = {
> +		.name = "renesas-vbattb",
> +		.of_match_table = vbattb_match,
> +	},
> +};
> +module_platform_driver(vbattb_driver);
> +
> +MODULE_ALIAS("platform:renesas-vbattb");
> +MODULE_AUTHOR("Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>");
> +MODULE_DESCRIPTION("Renesas VBATTB driver"); MODULE_LICENSE("GPL");
> --
> 2.39.2
>
Krzysztof Kozlowski July 16, 2024, 7:29 p.m. UTC | #3
On 16/07/2024 12:30, Claudiu wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Renesas VBATTB IP has logic to control the RTC clock, tamper detection
> and a small 128B memory. Add a MFD driver to do the basic initialization
> of the VBATTB IP for the inner components to work.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---


> +
> +static struct platform_driver vbattb_driver = {
> +	.probe = vbattb_probe,
> +	.remove_new = vbattb_remove,
> +	.driver = {
> +		.name = "renesas-vbattb",
> +		.of_match_table = vbattb_match,
> +	},
> +};
> +module_platform_driver(vbattb_driver);
> +
> +MODULE_ALIAS("platform:renesas-vbattb");

You should not need MODULE_ALIAS() in normal cases. If you need it,
usually it means your device ID table is wrong (e.g. misses either
entries or MODULE_DEVICE_TABLE()). MODULE_ALIAS() is not a substitute
for incomplete ID table.


> +MODULE_AUTHOR("Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>");
> +MODULE_DESCRIPTION("Renesas VBATTB driver");
> +MODULE_LICENSE("GPL");

Best regards,
Krzysztof
claudiu beznea July 17, 2024, 7:37 a.m. UTC | #4
Hi, Biju,

On 16.07.2024 14:00, Biju Das wrote:
> Hi Claudiu,
> 
> Thanks for the patch.
> 
> 
>> -----Original Message-----
>> From: Claudiu <claudiu.beznea@tuxon.dev>
>> Sent: Tuesday, July 16, 2024 11:30 AM
>> Subject: [PATCH v2 02/11] mfd: renesas-vbattb: Add a MFD driver for the Renesas VBATTB IP
>>
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> Renesas VBATTB IP has logic to control the RTC clock, tamper detection and a small 128B memory. Add a
>> MFD driver to do the basic initialization of the VBATTB IP for the inner components to work.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>> ---
>>
>> Changes in v2:
>> - none; this driver is new
>>
>>  drivers/mfd/Kconfig          |  8 ++++
>>  drivers/mfd/Makefile         |  1 +
>>  drivers/mfd/renesas-vbattb.c | 78 ++++++++++++++++++++++++++++++++++++
>>  3 files changed, 87 insertions(+)
>>  create mode 100644 drivers/mfd/renesas-vbattb.c
>>
>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index bc8be2e593b6..df93e8b05065 100644
>> --- a/drivers/mfd/Kconfig
>> +++ b/drivers/mfd/Kconfig
>> @@ -1383,6 +1383,14 @@ config MFD_SC27XX_PMIC
>>  	  This driver provides common support for accessing the SC27xx PMICs,
>>  	  and it also adds the irq_chip parts for handling the PMIC chip events.
>>
>> +config MFD_RENESAS_VBATTB
>> +	tristate "Renesas VBATTB driver"
>> +	depends on (ARCH_RZG2L && OF) || COMPILE_TEST
>> +	select MFD_CORE
> 
> There is no MFD calls??  What is the purpose of selecting MFD_CORE??

I missed to remove it from here.

> 
>> +	help
>> +	  Select this option to enable Renesas RZ/G3S VBATTB driver which
>> +	  provides support for the RTC clock, tamper detector and 128B SRAM.
>> +
>>  config RZ_MTU3
>>  	tristate "Renesas RZ/G2L MTU3a core driver"
>>  	depends on (ARCH_RZG2L && OF) || COMPILE_TEST diff --git a/drivers/mfd/Makefile
>> b/drivers/mfd/Makefile index 02b651cd7535..cd2f27492df2 100644
>> --- a/drivers/mfd/Makefile
>> +++ b/drivers/mfd/Makefile
>> @@ -186,6 +186,7 @@ pcf50633-objs			:= pcf50633-core.o pcf50633-irq.o
>>  obj-$(CONFIG_MFD_PCF50633)	+= pcf50633.o
>>  obj-$(CONFIG_PCF50633_ADC)	+= pcf50633-adc.o
>>  obj-$(CONFIG_PCF50633_GPIO)	+= pcf50633-gpio.o
>> +obj-$(CONFIG_MFD_RENESAS_VBATTB)	+= renesas-vbattb.o
>>  obj-$(CONFIG_RZ_MTU3)		+= rz-mtu3.o
>>  obj-$(CONFIG_ABX500_CORE)	+= abx500-core.o
>>  obj-$(CONFIG_MFD_DB8500_PRCMU)	+= db8500-prcmu.o
>> diff --git a/drivers/mfd/renesas-vbattb.c b/drivers/mfd/renesas-vbattb.c new file mode 100644 index
>> 000000000000..5d71565b8cbf
>> --- /dev/null
>> +++ b/drivers/mfd/renesas-vbattb.c
>> @@ -0,0 +1,78 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * VBATTB driver
>> + *
>> + * Copyright (C) 2024 Renesas Electronics Corp.
>> + */
>> +
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/reset.h>
>> +
>> +static int vbattb_probe(struct platform_device *pdev) {
>> +	struct device *dev = &pdev->dev;
>> +	struct reset_control *rstc;
>> +	int ret;
>> +
>> +	rstc = devm_reset_control_array_get_exclusive(dev);
>> +	if (IS_ERR(rstc))
>> +		return PTR_ERR(rstc);
>> +
>> +	ret = devm_pm_runtime_enable(dev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = pm_runtime_resume_and_get(dev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = reset_control_deassert(rstc);
>> +	if (ret)
>> +		goto rpm_put;
>> +
>> +	platform_set_drvdata(pdev, rstc);
>> +
>> +	ret = devm_of_platform_populate(dev);
>> +	if (ret)
>> +		goto reset_assert;
>> +
>> +	return 0;
>> +
>> +reset_assert:
>> +	reset_control_assert(rstc);
>> +rpm_put:
>> +	pm_runtime_put(dev);
>> +	return ret;
>> +}
>> +
>> +static void vbattb_remove(struct platform_device *pdev) {
>> +	struct reset_control *rstc = platform_get_drvdata(pdev);
>> +
>> +	reset_control_assert(rstc);
>> +	pm_runtime_put(&pdev->dev);
>> +}
>> +
>> +static const struct of_device_id vbattb_match[] = {
>> +	{ .compatible = "renesas,r9a08g045-vbattb" },
>> +	{ /* sentinel */ },
> 
> Drop comma.
> 
>> +};
>> +MODULE_DEVICE_TABLE(of, vbattb_match);
>> +
>> +static struct platform_driver vbattb_driver = {
>> +	.probe = vbattb_probe,
>> +	.remove_new = vbattb_remove,
> 
> Maybe remove canbe replaced with devm_add_action_or_reset()
> That simplifies probe() aswell??

This approach needs a new structure to keep references to the rstc and dev,
to be able to handle reset and runtime PM in action function. I wanted to
avoid adding a new structure.

Thank you for your review,
Claudiu Beznea

> 
>> +	.driver = {
>> +		.name = "renesas-vbattb",
>> +		.of_match_table = vbattb_match,
>> +	},
>> +};
>> +module_platform_driver(vbattb_driver);
>> +
>> +MODULE_ALIAS("platform:renesas-vbattb");
>> +MODULE_AUTHOR("Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>");
>> +MODULE_DESCRIPTION("Renesas VBATTB driver"); MODULE_LICENSE("GPL");
>> --
>> 2.39.2
>>
>
claudiu beznea July 17, 2024, 7:46 a.m. UTC | #5
Hi, Biju,

On 16.07.2024 14:07, Biju Das wrote:
> Hi Claudiu,
> 
>> -----Original Message-----
>> From: Claudiu <claudiu.beznea@tuxon.dev>
>> Sent: Tuesday, July 16, 2024 11:30 AM
>> Subject: [PATCH v2 02/11] mfd: renesas-vbattb: Add a MFD driver for the Renesas VBATTB IP
>>
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> Renesas VBATTB IP has logic to control the RTC clock, tamper detection and a small 128B memory. Add a
>> MFD driver to do the basic initialization of the VBATTB IP for the inner components to work.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>> ---
>>
>> Changes in v2:
>> - none; this driver is new
>>
>>  drivers/mfd/Kconfig          |  8 ++++
>>  drivers/mfd/Makefile         |  1 +
>>  drivers/mfd/renesas-vbattb.c | 78 ++++++++++++++++++++++++++++++++++++
>>  3 files changed, 87 insertions(+)
>>  create mode 100644 drivers/mfd/renesas-vbattb.c
>>
>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index bc8be2e593b6..df93e8b05065 100644
>> --- a/drivers/mfd/Kconfig
>> +++ b/drivers/mfd/Kconfig
>> @@ -1383,6 +1383,14 @@ config MFD_SC27XX_PMIC
>>  	  This driver provides common support for accessing the SC27xx PMICs,
>>  	  and it also adds the irq_chip parts for handling the PMIC chip events.
>>
>> +config MFD_RENESAS_VBATTB
>> +	tristate "Renesas VBATTB driver"
>> +	depends on (ARCH_RZG2L && OF) || COMPILE_TEST
>> +	select MFD_CORE
>> +	help
>> +	  Select this option to enable Renesas RZ/G3S VBATTB driver which
>> +	  provides support for the RTC clock, tamper detector and 128B SRAM.
>> +
>>  config RZ_MTU3
>>  	tristate "Renesas RZ/G2L MTU3a core driver"
>>  	depends on (ARCH_RZG2L && OF) || COMPILE_TEST diff --git a/drivers/mfd/Makefile
>> b/drivers/mfd/Makefile index 02b651cd7535..cd2f27492df2 100644
>> --- a/drivers/mfd/Makefile
>> +++ b/drivers/mfd/Makefile
>> @@ -186,6 +186,7 @@ pcf50633-objs			:= pcf50633-core.o pcf50633-irq.o
>>  obj-$(CONFIG_MFD_PCF50633)	+= pcf50633.o
>>  obj-$(CONFIG_PCF50633_ADC)	+= pcf50633-adc.o
>>  obj-$(CONFIG_PCF50633_GPIO)	+= pcf50633-gpio.o
>> +obj-$(CONFIG_MFD_RENESAS_VBATTB)	+= renesas-vbattb.o
>>  obj-$(CONFIG_RZ_MTU3)		+= rz-mtu3.o
>>  obj-$(CONFIG_ABX500_CORE)	+= abx500-core.o
>>  obj-$(CONFIG_MFD_DB8500_PRCMU)	+= db8500-prcmu.o
>> diff --git a/drivers/mfd/renesas-vbattb.c b/drivers/mfd/renesas-vbattb.c new file mode 100644 index
>> 000000000000..5d71565b8cbf
>> --- /dev/null
>> +++ b/drivers/mfd/renesas-vbattb.c
>> @@ -0,0 +1,78 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * VBATTB driver
>> + *
>> + * Copyright (C) 2024 Renesas Electronics Corp.
>> + */
>> +
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/reset.h>
>> +
>> +static int vbattb_probe(struct platform_device *pdev) {
>> +	struct device *dev = &pdev->dev;
>> +	struct reset_control *rstc;
>> +	int ret;
>> +
>> +	rstc = devm_reset_control_array_get_exclusive(dev);
>> +	if (IS_ERR(rstc))
>> +		return PTR_ERR(rstc);
>> +
>> +	ret = devm_pm_runtime_enable(dev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = pm_runtime_resume_and_get(dev);
>> +	if (ret)
>> +		return ret;
> 
> Maybe as an optimization drop pm calls and use "devm_clk_get_enabled"
> instead as it perfectly fits in this scenario??

VBATTB is still part of a PM domain. That needs to be enabled as well.
pm_runtime_* APIs takes care of both clock enable and power domain on
operations.

One thing to notice is that on RZ/G3S the VBATTB clock is CRITICAL (thus
enabled in the probe of the clock driver), the PM domain is always ON (and
also enabled by clock driver). We can get rid of pm_runtime_* at all for
RZ/G3S but I think it would be better to have it here as well for future
platforms and to emphasize from driver that these resources are needed by
the VBATTB. The same approach is used for  other RZ/G2 drivers that
declares critical clocks/always-on domains (e.g. dma driver, IA55 driver).

Thank you,
Claudiu Beznea

> 
>> +
>> +	ret = reset_control_deassert(rstc);
>> +	if (ret)
>> +		goto rpm_put;
>> +
>> +	platform_set_drvdata(pdev, rstc);
>> +
>> +	ret = devm_of_platform_populate(dev);
>> +	if (ret)
>> +		goto reset_assert;
>> +
>> +	return 0;
>> +
>> +reset_assert:
>> +	reset_control_assert(rstc);
>> +rpm_put:
>> +	pm_runtime_put(dev);
>> +	return ret;
>> +}
>> +
>> +static void vbattb_remove(struct platform_device *pdev) {
>> +	struct reset_control *rstc = platform_get_drvdata(pdev);
>> +
>> +	reset_control_assert(rstc);
>> +	pm_runtime_put(&pdev->dev);
>> +}
>> +
>> +static const struct of_device_id vbattb_match[] = {
>> +	{ .compatible = "renesas,r9a08g045-vbattb" },
>> +	{ /* sentinel */ },
>> +};
>> +MODULE_DEVICE_TABLE(of, vbattb_match);
>> +
>> +static struct platform_driver vbattb_driver = {
>> +	.probe = vbattb_probe,
>> +	.remove_new = vbattb_remove,
>> +	.driver = {
>> +		.name = "renesas-vbattb",
>> +		.of_match_table = vbattb_match,
>> +	},
>> +};
>> +module_platform_driver(vbattb_driver);
>> +
>> +MODULE_ALIAS("platform:renesas-vbattb");
>> +MODULE_AUTHOR("Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>");
>> +MODULE_DESCRIPTION("Renesas VBATTB driver"); MODULE_LICENSE("GPL");
>> --
>> 2.39.2
>>
>
claudiu beznea July 17, 2024, 7:47 a.m. UTC | #6
Hi, Krzysztof,

On 16.07.2024 22:29, Krzysztof Kozlowski wrote:
> On 16/07/2024 12:30, Claudiu wrote:
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> Renesas VBATTB IP has logic to control the RTC clock, tamper detection
>> and a small 128B memory. Add a MFD driver to do the basic initialization
>> of the VBATTB IP for the inner components to work.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>> ---
> 
> 
>> +
>> +static struct platform_driver vbattb_driver = {
>> +	.probe = vbattb_probe,
>> +	.remove_new = vbattb_remove,
>> +	.driver = {
>> +		.name = "renesas-vbattb",
>> +		.of_match_table = vbattb_match,
>> +	},
>> +};
>> +module_platform_driver(vbattb_driver);
>> +
>> +MODULE_ALIAS("platform:renesas-vbattb");
> 
> You should not need MODULE_ALIAS() in normal cases. If you need it,
> usually it means your device ID table is wrong (e.g. misses either
> entries or MODULE_DEVICE_TABLE()). MODULE_ALIAS() is not a substitute
> for incomplete ID table.

It's not needed. I took another driver as reference and missed to removed
it from here in the end.

Thank  you for your review,
Claudiu Beznea


> 
> 
>> +MODULE_AUTHOR("Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>");
>> +MODULE_DESCRIPTION("Renesas VBATTB driver");
>> +MODULE_LICENSE("GPL");
> 
> Best regards,
> Krzysztof
>
Biju Das July 17, 2024, 8:07 a.m. UTC | #7
Hi Claudium

> -----Original Message-----
> From: claudiu beznea <claudiu.beznea@tuxon.dev>
> Sent: Wednesday, July 17, 2024 8:38 AM
@vger.kernel.org; Claudiu Beznea
> <claudiu.beznea.uj@bp.renesas.com>
> Subject: Re: [PATCH v2 02/11] mfd: renesas-vbattb: Add a MFD driver for
> the Renesas VBATTB IP
> 
> Hi, Biju,
> 
> On 16.07.2024 14:00, Biju Das wrote:
> > Hi Claudiu,
> >
> > Thanks for the patch.
> >
> >
> >> -----Original Message-----
> >> From: Claudiu <claudiu.beznea@tuxon.dev>
> >> Sent: Tuesday, July 16, 2024 11:30 AM
> >> Subject: [PATCH v2 02/11] mfd: renesas-vbattb: Add a MFD driver for
> >> the Renesas VBATTB IP
> >>
> >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >>
> >> Renesas VBATTB IP has logic to control the RTC clock, tamper
> >> detection and a small 128B memory. Add a MFD driver to do the basic
> initialization of the VBATTB IP for the inner components to work.
> >>
> >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >> ---
> >>
> >> Changes in v2:
> >> - none; this driver is new
> >>
> >>  drivers/mfd/Kconfig          |  8 ++++
> >>  drivers/mfd/Makefile         |  1 +
> >>  drivers/mfd/renesas-vbattb.c | 78
> >> ++++++++++++++++++++++++++++++++++++
> >>  3 files changed, 87 insertions(+)
> >>  create mode 100644 drivers/mfd/renesas-vbattb.c
> >>
> >> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index
> >> bc8be2e593b6..df93e8b05065 100644
> >> --- a/drivers/mfd/Kconfig
> >> +++ b/drivers/mfd/Kconfig
> >> @@ -1383,6 +1383,14 @@ config MFD_SC27XX_PMIC
> >>  	  This driver provides common support for accessing the SC27xx PMICs,
> >>  	  and it also adds the irq_chip parts for handling the PMIC chip
> events.
> >>
> >> +config MFD_RENESAS_VBATTB
> >> +	tristate "Renesas VBATTB driver"
> >> +	depends on (ARCH_RZG2L && OF) || COMPILE_TEST
> >> +	select MFD_CORE
> >
> > There is no MFD calls??  What is the purpose of selecting MFD_CORE??
> 
> I missed to remove it from here.
> 
> >
> >> +	help
> >> +	  Select this option to enable Renesas RZ/G3S VBATTB driver which
> >> +	  provides support for the RTC clock, tamper detector and 128B SRAM.
> >> +
> >>  config RZ_MTU3
> >>  	tristate "Renesas RZ/G2L MTU3a core driver"
> >>  	depends on (ARCH_RZG2L && OF) || COMPILE_TEST diff --git
> >> a/drivers/mfd/Makefile b/drivers/mfd/Makefile index
> >> 02b651cd7535..cd2f27492df2 100644
> >> --- a/drivers/mfd/Makefile
> >> +++ b/drivers/mfd/Makefile
> >> @@ -186,6 +186,7 @@ pcf50633-objs			:= pcf50633-core.o
> pcf50633-irq.o
> >>  obj-$(CONFIG_MFD_PCF50633)	+= pcf50633.o
> >>  obj-$(CONFIG_PCF50633_ADC)	+= pcf50633-adc.o
> >>  obj-$(CONFIG_PCF50633_GPIO)	+= pcf50633-gpio.o
> >> +obj-$(CONFIG_MFD_RENESAS_VBATTB)	+= renesas-vbattb.o
> >>  obj-$(CONFIG_RZ_MTU3)		+= rz-mtu3.o
> >>  obj-$(CONFIG_ABX500_CORE)	+= abx500-core.o
> >>  obj-$(CONFIG_MFD_DB8500_PRCMU)	+= db8500-prcmu.o
> >> diff --git a/drivers/mfd/renesas-vbattb.c
> >> b/drivers/mfd/renesas-vbattb.c new file mode 100644 index
> >> 000000000000..5d71565b8cbf
> >> --- /dev/null
> >> +++ b/drivers/mfd/renesas-vbattb.c
> >> @@ -0,0 +1,78 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * VBATTB driver
> >> + *
> >> + * Copyright (C) 2024 Renesas Electronics Corp.
> >> + */
> >> +
> >> +#include <linux/mod_devicetable.h>
> >> +#include <linux/of_platform.h>
> >> +#include <linux/platform_device.h>
> >> +#include <linux/pm_runtime.h>
> >> +#include <linux/reset.h>
> >> +
> >> +static int vbattb_probe(struct platform_device *pdev) {
> >> +	struct device *dev = &pdev->dev;
> >> +	struct reset_control *rstc;
> >> +	int ret;
> >> +
> >> +	rstc = devm_reset_control_array_get_exclusive(dev);
> >> +	if (IS_ERR(rstc))
> >> +		return PTR_ERR(rstc);
> >> +
> >> +	ret = devm_pm_runtime_enable(dev);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	ret = pm_runtime_resume_and_get(dev);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	ret = reset_control_deassert(rstc);
> >> +	if (ret)
> >> +		goto rpm_put;
> >> +
> >> +	platform_set_drvdata(pdev, rstc);
> >> +
> >> +	ret = devm_of_platform_populate(dev);
> >> +	if (ret)
> >> +		goto reset_assert;
> >> +
> >> +	return 0;
> >> +
> >> +reset_assert:
> >> +	reset_control_assert(rstc);
> >> +rpm_put:
> >> +	pm_runtime_put(dev);
> >> +	return ret;
> >> +}
> >> +
> >> +static void vbattb_remove(struct platform_device *pdev) {
> >> +	struct reset_control *rstc = platform_get_drvdata(pdev);
> >> +
> >> +	reset_control_assert(rstc);
> >> +	pm_runtime_put(&pdev->dev);
> >> +}
> >> +
> >> +static const struct of_device_id vbattb_match[] = {
> >> +	{ .compatible = "renesas,r9a08g045-vbattb" },
> >> +	{ /* sentinel */ },
> >
> > Drop comma.
> >
> >> +};
> >> +MODULE_DEVICE_TABLE(of, vbattb_match);
> >> +
> >> +static struct platform_driver vbattb_driver = {
> >> +	.probe = vbattb_probe,
> >> +	.remove_new = vbattb_remove,
> >
> > Maybe remove canbe replaced with devm_add_action_or_reset() That
> > simplifies probe() aswell??
> 
> This approach needs a new structure to keep references to the rstc and dev,
> to be able to handle reset and runtime PM in action function. I wanted to
> avoid adding a new structure.


You can pass dev as context parameter in devm_add_action_or_reset(), from that you get rstc=dev_get_drvdata(dev); 
So you don't need new structure.

Cheers,
Biju
Biju Das July 17, 2024, 8:16 a.m. UTC | #8
Hi Claudiu,

> -----Original Message-----
> From: claudiu beznea <claudiu.beznea@tuxon.dev>
> Sent: Wednesday, July 17, 2024 8:46 AM
> Subject: Re: [PATCH v2 02/11] mfd: renesas-vbattb: Add a MFD driver for
> the Renesas VBATTB IP
> 
> Hi, Biju,
> 
> On 16.07.2024 14:07, Biju Das wrote:
> > Hi Claudiu,
> >
> >> -----Original Message-----
> >> From: Claudiu <claudiu.beznea@tuxon.dev>
> >> Sent: Tuesday, July 16, 2024 11:30 AM
> >> Subject: [PATCH v2 02/11] mfd: renesas-vbattb: Add a MFD driver for
> >> the Renesas VBATTB IP
> >>
> >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >>
> >> Renesas VBATTB IP has logic to control the RTC clock, tamper
> >> detection and a small 128B memory. Add a MFD driver to do the basic
> initialization of the VBATTB IP for the inner components to work.
> >>
> >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >> ---
> >>
> >> Changes in v2:
> >> - none; this driver is new
> >>
> >>  drivers/mfd/Kconfig          |  8 ++++
> >>  drivers/mfd/Makefile         |  1 +
> >>  drivers/mfd/renesas-vbattb.c | 78
> >> ++++++++++++++++++++++++++++++++++++
> >>  3 files changed, 87 insertions(+)
> >>  create mode 100644 drivers/mfd/renesas-vbattb.c
> >>
> >> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index
> >> bc8be2e593b6..df93e8b05065 100644
> >> --- a/drivers/mfd/Kconfig
> >> +++ b/drivers/mfd/Kconfig
> >> @@ -1383,6 +1383,14 @@ config MFD_SC27XX_PMIC
> >>  	  This driver provides common support for accessing the SC27xx PMICs,
> >>  	  and it also adds the irq_chip parts for handling the PMIC chip
> events.
> >>
> >> +config MFD_RENESAS_VBATTB
> >> +	tristate "Renesas VBATTB driver"
> >> +	depends on (ARCH_RZG2L && OF) || COMPILE_TEST
> >> +	select MFD_CORE
> >> +	help
> >> +	  Select this option to enable Renesas RZ/G3S VBATTB driver which
> >> +	  provides support for the RTC clock, tamper detector and 128B SRAM.
> >> +
> >>  config RZ_MTU3
> >>  	tristate "Renesas RZ/G2L MTU3a core driver"
> >>  	depends on (ARCH_RZG2L && OF) || COMPILE_TEST diff --git
> >> a/drivers/mfd/Makefile b/drivers/mfd/Makefile index
> >> 02b651cd7535..cd2f27492df2 100644
> >> --- a/drivers/mfd/Makefile
> >> +++ b/drivers/mfd/Makefile
> >> @@ -186,6 +186,7 @@ pcf50633-objs			:= pcf50633-core.o
> pcf50633-irq.o
> >>  obj-$(CONFIG_MFD_PCF50633)	+= pcf50633.o
> >>  obj-$(CONFIG_PCF50633_ADC)	+= pcf50633-adc.o
> >>  obj-$(CONFIG_PCF50633_GPIO)	+= pcf50633-gpio.o
> >> +obj-$(CONFIG_MFD_RENESAS_VBATTB)	+= renesas-vbattb.o
> >>  obj-$(CONFIG_RZ_MTU3)		+= rz-mtu3.o
> >>  obj-$(CONFIG_ABX500_CORE)	+= abx500-core.o
> >>  obj-$(CONFIG_MFD_DB8500_PRCMU)	+= db8500-prcmu.o
> >> diff --git a/drivers/mfd/renesas-vbattb.c
> >> b/drivers/mfd/renesas-vbattb.c new file mode 100644 index
> >> 000000000000..5d71565b8cbf
> >> --- /dev/null
> >> +++ b/drivers/mfd/renesas-vbattb.c
> >> @@ -0,0 +1,78 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * VBATTB driver
> >> + *
> >> + * Copyright (C) 2024 Renesas Electronics Corp.
> >> + */
> >> +
> >> +#include <linux/mod_devicetable.h>
> >> +#include <linux/of_platform.h>
> >> +#include <linux/platform_device.h>
> >> +#include <linux/pm_runtime.h>
> >> +#include <linux/reset.h>
> >> +
> >> +static int vbattb_probe(struct platform_device *pdev) {
> >> +	struct device *dev = &pdev->dev;
> >> +	struct reset_control *rstc;
> >> +	int ret;
> >> +
> >> +	rstc = devm_reset_control_array_get_exclusive(dev);
> >> +	if (IS_ERR(rstc))
> >> +		return PTR_ERR(rstc);
> >> +
> >> +	ret = devm_pm_runtime_enable(dev);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	ret = pm_runtime_resume_and_get(dev);
> >> +	if (ret)
> >> +		return ret;
> >
> > Maybe as an optimization drop pm calls and use "devm_clk_get_enabled"
> > instead as it perfectly fits in this scenario??
> 
> VBATTB is still part of a PM domain. That needs to be enabled as well.
> pm_runtime_* APIs takes care of both clock enable and power domain on
> operations.
> 
> One thing to notice is that on RZ/G3S the VBATTB clock is CRITICAL (thus
> enabled in the probe of the clock driver), the PM domain is always ON (and
> also enabled by clock driver). We can get rid of pm_runtime_* at all for
> RZ/G3S but I think it would be better to have it here as well for future
> platforms and to emphasize from driver that these resources are needed by
> the VBATTB. The same approach is used for  other RZ/G2 drivers that
> declares critical clocks/always-on domains (e.g. dma driver, IA55 driver).


If it is critical clock, the parent has to be critical clock. So you can blindly
use devm_clk_get_enabled() that simplifies probe(), if the PM domain controls only clock.


If the PM domain controls voltage means you should use PM domain APIs

Cheers,
Biju
Lee Jones July 24, 2024, 2:53 p.m. UTC | #9
On Tue, 16 Jul 2024, Claudiu wrote:

> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Renesas VBATTB IP has logic to control the RTC clock, tamper detection
> and a small 128B memory. Add a MFD driver to do the basic initialization
> of the VBATTB IP for the inner components to work.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---
> 
> Changes in v2:
> - none; this driver is new
> 
>  drivers/mfd/Kconfig          |  8 ++++
>  drivers/mfd/Makefile         |  1 +
>  drivers/mfd/renesas-vbattb.c | 78 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 87 insertions(+)
>  create mode 100644 drivers/mfd/renesas-vbattb.c
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index bc8be2e593b6..df93e8b05065 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1383,6 +1383,14 @@ config MFD_SC27XX_PMIC
>  	  This driver provides common support for accessing the SC27xx PMICs,
>  	  and it also adds the irq_chip parts for handling the PMIC chip events.
>  
> +config MFD_RENESAS_VBATTB
> +	tristate "Renesas VBATTB driver"
> +	depends on (ARCH_RZG2L && OF) || COMPILE_TEST
> +	select MFD_CORE
> +	help
> +	  Select this option to enable Renesas RZ/G3S VBATTB driver which
> +	  provides support for the RTC clock, tamper detector and 128B SRAM.
> +
>  config RZ_MTU3
>  	tristate "Renesas RZ/G2L MTU3a core driver"
>  	depends on (ARCH_RZG2L && OF) || COMPILE_TEST
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 02b651cd7535..cd2f27492df2 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -186,6 +186,7 @@ pcf50633-objs			:= pcf50633-core.o pcf50633-irq.o
>  obj-$(CONFIG_MFD_PCF50633)	+= pcf50633.o
>  obj-$(CONFIG_PCF50633_ADC)	+= pcf50633-adc.o
>  obj-$(CONFIG_PCF50633_GPIO)	+= pcf50633-gpio.o
> +obj-$(CONFIG_MFD_RENESAS_VBATTB)	+= renesas-vbattb.o
>  obj-$(CONFIG_RZ_MTU3)		+= rz-mtu3.o
>  obj-$(CONFIG_ABX500_CORE)	+= abx500-core.o
>  obj-$(CONFIG_MFD_DB8500_PRCMU)	+= db8500-prcmu.o
> diff --git a/drivers/mfd/renesas-vbattb.c b/drivers/mfd/renesas-vbattb.c
> new file mode 100644
> index 000000000000..5d71565b8cbf
> --- /dev/null
> +++ b/drivers/mfd/renesas-vbattb.c
> @@ -0,0 +1,78 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * VBATTB driver
> + *
> + * Copyright (C) 2024 Renesas Electronics Corp.
> + */
> +
> +#include <linux/mod_devicetable.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/reset.h>
> +
> +static int vbattb_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct reset_control *rstc;
> +	int ret;
> +
> +	rstc = devm_reset_control_array_get_exclusive(dev);
> +	if (IS_ERR(rstc))
> +		return PTR_ERR(rstc);
> +
> +	ret = devm_pm_runtime_enable(dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = reset_control_deassert(rstc);
> +	if (ret)
> +		goto rpm_put;
> +
> +	platform_set_drvdata(pdev, rstc);

Where is this consumed?

> +	ret = devm_of_platform_populate(dev);


Which devices will this probe?

> +	if (ret)
> +		goto reset_assert;
> +
> +	return 0;
> +
> +reset_assert:
> +	reset_control_assert(rstc);
> +rpm_put:
> +	pm_runtime_put(dev);
> +	return ret;
> +}
> +
> +static void vbattb_remove(struct platform_device *pdev)
> +{
> +	struct reset_control *rstc = platform_get_drvdata(pdev);
> +
> +	reset_control_assert(rstc);
> +	pm_runtime_put(&pdev->dev);
> +}
> +
> +static const struct of_device_id vbattb_match[] = {
> +	{ .compatible = "renesas,r9a08g045-vbattb" },
> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, vbattb_match);
> +
> +static struct platform_driver vbattb_driver = {
> +	.probe = vbattb_probe,
> +	.remove_new = vbattb_remove,
> +	.driver = {
> +		.name = "renesas-vbattb",
> +		.of_match_table = vbattb_match,
> +	},
> +};
> +module_platform_driver(vbattb_driver);
> +
> +MODULE_ALIAS("platform:renesas-vbattb");
> +MODULE_AUTHOR("Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>");
> +MODULE_DESCRIPTION("Renesas VBATTB driver");
> +MODULE_LICENSE("GPL");
> -- 
> 2.39.2
>
claudiu beznea July 25, 2024, 8:03 a.m. UTC | #10
Hi, Lee,

On 24.07.2024 17:53, Lee Jones wrote:
> On Tue, 16 Jul 2024, Claudiu wrote:
> 
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> Renesas VBATTB IP has logic to control the RTC clock, tamper detection
>> and a small 128B memory. Add a MFD driver to do the basic initialization
>> of the VBATTB IP for the inner components to work.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>> ---
>>
>> Changes in v2:
>> - none; this driver is new
>>
>>  drivers/mfd/Kconfig          |  8 ++++
>>  drivers/mfd/Makefile         |  1 +
>>  drivers/mfd/renesas-vbattb.c | 78 ++++++++++++++++++++++++++++++++++++
>>  3 files changed, 87 insertions(+)
>>  create mode 100644 drivers/mfd/renesas-vbattb.c
>>
>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>> index bc8be2e593b6..df93e8b05065 100644
>> --- a/drivers/mfd/Kconfig
>> +++ b/drivers/mfd/Kconfig
>> @@ -1383,6 +1383,14 @@ config MFD_SC27XX_PMIC
>>  	  This driver provides common support for accessing the SC27xx PMICs,
>>  	  and it also adds the irq_chip parts for handling the PMIC chip events.
>>  
>> +config MFD_RENESAS_VBATTB
>> +	tristate "Renesas VBATTB driver"
>> +	depends on (ARCH_RZG2L && OF) || COMPILE_TEST
>> +	select MFD_CORE
>> +	help
>> +	  Select this option to enable Renesas RZ/G3S VBATTB driver which
>> +	  provides support for the RTC clock, tamper detector and 128B SRAM.
>> +
>>  config RZ_MTU3
>>  	tristate "Renesas RZ/G2L MTU3a core driver"
>>  	depends on (ARCH_RZG2L && OF) || COMPILE_TEST
>> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
>> index 02b651cd7535..cd2f27492df2 100644
>> --- a/drivers/mfd/Makefile
>> +++ b/drivers/mfd/Makefile
>> @@ -186,6 +186,7 @@ pcf50633-objs			:= pcf50633-core.o pcf50633-irq.o
>>  obj-$(CONFIG_MFD_PCF50633)	+= pcf50633.o
>>  obj-$(CONFIG_PCF50633_ADC)	+= pcf50633-adc.o
>>  obj-$(CONFIG_PCF50633_GPIO)	+= pcf50633-gpio.o
>> +obj-$(CONFIG_MFD_RENESAS_VBATTB)	+= renesas-vbattb.o
>>  obj-$(CONFIG_RZ_MTU3)		+= rz-mtu3.o
>>  obj-$(CONFIG_ABX500_CORE)	+= abx500-core.o
>>  obj-$(CONFIG_MFD_DB8500_PRCMU)	+= db8500-prcmu.o
>> diff --git a/drivers/mfd/renesas-vbattb.c b/drivers/mfd/renesas-vbattb.c
>> new file mode 100644
>> index 000000000000..5d71565b8cbf
>> --- /dev/null
>> +++ b/drivers/mfd/renesas-vbattb.c
>> @@ -0,0 +1,78 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * VBATTB driver
>> + *
>> + * Copyright (C) 2024 Renesas Electronics Corp.
>> + */
>> +
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/reset.h>
>> +
>> +static int vbattb_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct reset_control *rstc;
>> +	int ret;
>> +
>> +	rstc = devm_reset_control_array_get_exclusive(dev);
>> +	if (IS_ERR(rstc))
>> +		return PTR_ERR(rstc);
>> +
>> +	ret = devm_pm_runtime_enable(dev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = pm_runtime_resume_and_get(dev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = reset_control_deassert(rstc);
>> +	if (ret)
>> +		goto rpm_put;
>> +
>> +	platform_set_drvdata(pdev, rstc);
> 
> Where is this consumed?

In vbattb_remove().

> 
>> +	ret = devm_of_platform_populate(dev);
> 
> 
> Which devices will this probe?

In this version it is used by clock logic from VBATTB IP, modeled as
individual device. A schema of the blocks controlled in the VBATTB IP can
be found at [1] (please note that there is also the RTC mentioned there but
because it is on the PD_VBATT always-on power domain (backed by battery);
in fact, it is an individual device mapped at it's own address). Here:
- the 32KHz-clock oscillator, mux (with XC, XBYP inputs), CGC are used in
  the cock driver (introduced in this series)
- tamper detector module with gate controlled by TAMPICR1 being part of
  tamper detection logic (no driver for this ATM)
- backup registers being the SRAM (tried locally with mmio-sram driver
  (drives/misc/sram.c)), subnode described with (for internal development):

vbattb: vbattb@1005c000 {

	// ...

	backup_sram: sram@80 {
		compatible = "mmio-sram";
		reg = <0 0x80 0 0x80>;
		clocks = <&cpg CPG_MOD R9A08G045_VBAT_BCLK>;
		clock-names = "bclk";
		power-domains = <&cpg>;
		resets = <&cpg R9A08G045_VBAT_BRESETN>;
		no-memory-wc;

		#address-cells = <2>;
		#size-cells = <2>;
		ranges = <0 0 0 0x80 0 0x80>;

		pool@0 {
			reg = <0 0 0 0x80>;
			label = "sram-test";
			export;
		};
	};
};

My initial idea was to have logic blocks of the VBATTB IP grouped as
devices (clock for the moment and for future, at least the small SRAM, if
needed at some point). After the discussion with Stephen on clock driver I
tend to give up this model.

Please let me know if you have any hints on how to go forward.

Thank you for  your review,
Claudiu Beznea

[1] https://i2.paste.pics/RFKJ0.png?rand=Xq8w1RLDvZ

> 
>> +	if (ret)
>> +		goto reset_assert;
>> +
>> +	return 0;
>> +
>> +reset_assert:
>> +	reset_control_assert(rstc);
>> +rpm_put:
>> +	pm_runtime_put(dev);
>> +	return ret;
>> +}
>> +
>> +static void vbattb_remove(struct platform_device *pdev)
>> +{
>> +	struct reset_control *rstc = platform_get_drvdata(pdev);
>> +
>> +	reset_control_assert(rstc);
>> +	pm_runtime_put(&pdev->dev);
>> +}
>> +
>> +static const struct of_device_id vbattb_match[] = {
>> +	{ .compatible = "renesas,r9a08g045-vbattb" },
>> +	{ /* sentinel */ },
>> +};
>> +MODULE_DEVICE_TABLE(of, vbattb_match);
>> +
>> +static struct platform_driver vbattb_driver = {
>> +	.probe = vbattb_probe,
>> +	.remove_new = vbattb_remove,
>> +	.driver = {
>> +		.name = "renesas-vbattb",
>> +		.of_match_table = vbattb_match,
>> +	},
>> +};
>> +module_platform_driver(vbattb_driver);
>> +
>> +MODULE_ALIAS("platform:renesas-vbattb");
>> +MODULE_AUTHOR("Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>");
>> +MODULE_DESCRIPTION("Renesas VBATTB driver");
>> +MODULE_LICENSE("GPL");
>> -- 
>> 2.39.2
>>
>
diff mbox series

Patch

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index bc8be2e593b6..df93e8b05065 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1383,6 +1383,14 @@  config MFD_SC27XX_PMIC
 	  This driver provides common support for accessing the SC27xx PMICs,
 	  and it also adds the irq_chip parts for handling the PMIC chip events.
 
+config MFD_RENESAS_VBATTB
+	tristate "Renesas VBATTB driver"
+	depends on (ARCH_RZG2L && OF) || COMPILE_TEST
+	select MFD_CORE
+	help
+	  Select this option to enable Renesas RZ/G3S VBATTB driver which
+	  provides support for the RTC clock, tamper detector and 128B SRAM.
+
 config RZ_MTU3
 	tristate "Renesas RZ/G2L MTU3a core driver"
 	depends on (ARCH_RZG2L && OF) || COMPILE_TEST
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 02b651cd7535..cd2f27492df2 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -186,6 +186,7 @@  pcf50633-objs			:= pcf50633-core.o pcf50633-irq.o
 obj-$(CONFIG_MFD_PCF50633)	+= pcf50633.o
 obj-$(CONFIG_PCF50633_ADC)	+= pcf50633-adc.o
 obj-$(CONFIG_PCF50633_GPIO)	+= pcf50633-gpio.o
+obj-$(CONFIG_MFD_RENESAS_VBATTB)	+= renesas-vbattb.o
 obj-$(CONFIG_RZ_MTU3)		+= rz-mtu3.o
 obj-$(CONFIG_ABX500_CORE)	+= abx500-core.o
 obj-$(CONFIG_MFD_DB8500_PRCMU)	+= db8500-prcmu.o
diff --git a/drivers/mfd/renesas-vbattb.c b/drivers/mfd/renesas-vbattb.c
new file mode 100644
index 000000000000..5d71565b8cbf
--- /dev/null
+++ b/drivers/mfd/renesas-vbattb.c
@@ -0,0 +1,78 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * VBATTB driver
+ *
+ * Copyright (C) 2024 Renesas Electronics Corp.
+ */
+
+#include <linux/mod_devicetable.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/reset.h>
+
+static int vbattb_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct reset_control *rstc;
+	int ret;
+
+	rstc = devm_reset_control_array_get_exclusive(dev);
+	if (IS_ERR(rstc))
+		return PTR_ERR(rstc);
+
+	ret = devm_pm_runtime_enable(dev);
+	if (ret)
+		return ret;
+
+	ret = pm_runtime_resume_and_get(dev);
+	if (ret)
+		return ret;
+
+	ret = reset_control_deassert(rstc);
+	if (ret)
+		goto rpm_put;
+
+	platform_set_drvdata(pdev, rstc);
+
+	ret = devm_of_platform_populate(dev);
+	if (ret)
+		goto reset_assert;
+
+	return 0;
+
+reset_assert:
+	reset_control_assert(rstc);
+rpm_put:
+	pm_runtime_put(dev);
+	return ret;
+}
+
+static void vbattb_remove(struct platform_device *pdev)
+{
+	struct reset_control *rstc = platform_get_drvdata(pdev);
+
+	reset_control_assert(rstc);
+	pm_runtime_put(&pdev->dev);
+}
+
+static const struct of_device_id vbattb_match[] = {
+	{ .compatible = "renesas,r9a08g045-vbattb" },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, vbattb_match);
+
+static struct platform_driver vbattb_driver = {
+	.probe = vbattb_probe,
+	.remove_new = vbattb_remove,
+	.driver = {
+		.name = "renesas-vbattb",
+		.of_match_table = vbattb_match,
+	},
+};
+module_platform_driver(vbattb_driver);
+
+MODULE_ALIAS("platform:renesas-vbattb");
+MODULE_AUTHOR("Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>");
+MODULE_DESCRIPTION("Renesas VBATTB driver");
+MODULE_LICENSE("GPL");