| Message ID | 20240716103025.1198495-1-claudiu.beznea.uj@bp.renesas.com |
|---|---|
| Headers | show |
| Series | Add RTC support for the Renesas RZ/G3S SoC | expand |
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 >
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 >
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
Quoting Claudiu (2024-07-16 03:30:17) > diff --git a/drivers/clk/renesas/clk-vbattb.c b/drivers/clk/renesas/clk-vbattb.c > new file mode 100644 > index 000000000000..8effe141fc0b > --- /dev/null > +++ b/drivers/clk/renesas/clk-vbattb.c > @@ -0,0 +1,212 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * VBATTB clock driver > + * > + * Copyright (C) 2024 Renesas Electronics Corp. > + */ > + > +#include <linux/cleanup.h> > +#include <linux/clk.h> Prefer clk providers to not be clk consumers. > +#include <linux/clk-provider.h> > +#include <linux/device.h> > +#include <linux/io.h> > +#include <linux/of.h> > +#include <linux/of_platform.h> Is of_platform.h used? Include mod_devicetable.h for of_device_id. > +#include <linux/platform_device.h> > + > +#define VBATTB_BKSCCR 0x0 > +#define VBATTB_BKSCCR_SOSEL BIT(6) > +#define VBATTB_SOSCCR2 0x8 > +#define VBATTB_SOSCCR2_SOSTP2 BIT(0) [..] > + > +static int vbattb_clk_probe(struct platform_device *pdev) > +{ > + struct device_node *np = pdev->dev.of_node; > + struct clk_parent_data parent_data = {}; > + struct device *dev = &pdev->dev; > + struct clk_init_data init = {}; > + struct vbattb_clk *vbclk; > + u32 load_capacitance; > + struct clk_hw *hw; > + int ret, bypass; > + > + vbclk = devm_kzalloc(dev, sizeof(*vbclk), GFP_KERNEL); > + if (!vbclk) > + return -ENOMEM; > + > + vbclk->base = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(vbclk->base)) > + return PTR_ERR(vbclk->base); > + > + bypass = vbattb_clk_need_bypass(dev); This is a tri-state bool :( > + if (bypass < 0) { > + return bypass; > + } else if (bypass) { > + parent_data.fw_name = "clkin"; > + bypass = VBATTB_BKSCCR_SOSEL; And now it is a mask value. > + } else { > + parent_data.fw_name = "xin"; > + } > + > + ret = of_property_read_u32(np, "renesas,vbattb-load-nanofarads", &load_capacitance); > + if (ret) > + return ret; > + > + ret = vbattb_clk_validate_load_capacitance(vbclk, load_capacitance); > + if (ret) > + return ret; > + > + vbattb_clk_update_bits(vbclk->base, VBATTB_BKSCCR, VBATTB_BKSCCR_SOSEL, bypass); Please don't overload 'bypass'. Use two variables or a conditional. I also wonder if this is really a mux, and either assigned-clock-parents should be used, or the clk_ops should have an init routine that looks at which parent is present by determining the index and then use that to set the mux. The framework can take care of failing to set the other parent when it isn't present. > + > + spin_lock_init(&vbclk->lock); > + > + init.name = "vbattclk"; > + init.ops = &vbattb_clk_ops; > + init.parent_data = &parent_data; > + init.num_parents = 1; > + init.flags = 0; > + > + vbclk->hw.init = &init; > + hw = &vbclk->hw; > + > + ret = devm_clk_hw_register(dev, hw); > + if (ret) > + return ret; > + > + return of_clk_add_hw_provider(np, of_clk_hw_simple_get, hw); > +} > + > +static const struct of_device_id vbattb_clk_match[] = { > + { .compatible = "renesas,r9a08g045-vbattb-clk" }, > + { /* sentinel */ } > +}; Any MODULE_DEVICE_TABLE?
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 >> >
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 >> >
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 >
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
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
Hi, Stephen, On 17.07.2024 01:28, Stephen Boyd wrote: > Quoting Claudiu (2024-07-16 03:30:17) >> diff --git a/drivers/clk/renesas/clk-vbattb.c b/drivers/clk/renesas/clk-vbattb.c >> new file mode 100644 >> index 000000000000..8effe141fc0b >> --- /dev/null >> +++ b/drivers/clk/renesas/clk-vbattb.c >> @@ -0,0 +1,212 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * VBATTB clock driver >> + * >> + * Copyright (C) 2024 Renesas Electronics Corp. >> + */ >> + >> +#include <linux/cleanup.h> >> +#include <linux/clk.h> > > Prefer clk providers to not be clk consumers. I added it here to be able to use devm_clk_get_optional() as it was proposed to me in v1 to avoid adding a new binding for bypass and detect if it's needed by checking the input clock name. > >> +#include <linux/clk-provider.h> >> +#include <linux/device.h> >> +#include <linux/io.h> >> +#include <linux/of.h> >> +#include <linux/of_platform.h> > > Is of_platform.h used? > > Include mod_devicetable.h for of_device_id. Ok. > >> +#include <linux/platform_device.h> >> + >> +#define VBATTB_BKSCCR 0x0 >> +#define VBATTB_BKSCCR_SOSEL BIT(6) >> +#define VBATTB_SOSCCR2 0x8 >> +#define VBATTB_SOSCCR2_SOSTP2 BIT(0) > [..] >> + >> +static int vbattb_clk_probe(struct platform_device *pdev) >> +{ >> + struct device_node *np = pdev->dev.of_node; >> + struct clk_parent_data parent_data = {}; >> + struct device *dev = &pdev->dev; >> + struct clk_init_data init = {}; >> + struct vbattb_clk *vbclk; >> + u32 load_capacitance; >> + struct clk_hw *hw; >> + int ret, bypass; >> + >> + vbclk = devm_kzalloc(dev, sizeof(*vbclk), GFP_KERNEL); >> + if (!vbclk) >> + return -ENOMEM; >> + >> + vbclk->base = devm_platform_ioremap_resource(pdev, 0); >> + if (IS_ERR(vbclk->base)) >> + return PTR_ERR(vbclk->base); >> + >> + bypass = vbattb_clk_need_bypass(dev); > > This is a tri-state bool :( > >> + if (bypass < 0) { >> + return bypass; >> + } else if (bypass) { >> + parent_data.fw_name = "clkin"; >> + bypass = VBATTB_BKSCCR_SOSEL; > > And now it is a mask value. > >> + } else { >> + parent_data.fw_name = "xin"; >> + } >> + >> + ret = of_property_read_u32(np, "renesas,vbattb-load-nanofarads", &load_capacitance); >> + if (ret) >> + return ret; >> + >> + ret = vbattb_clk_validate_load_capacitance(vbclk, load_capacitance); >> + if (ret) >> + return ret; >> + >> + vbattb_clk_update_bits(vbclk->base, VBATTB_BKSCCR, VBATTB_BKSCCR_SOSEL, bypass); > > Please don't overload 'bypass'. Use two variables or a conditional. OK. > > I also wonder if this is really a mux, It's a way to determine what type of clock (crystal oscillator or device clock) is connected to RTXIN/RTXOUT pins of the module (the module block diagram at [1]) based on the clock name. Depending on the type of the clock connected to RTXIN/RTXOUT we need to select the XC or XBYP as input for the mux at [1]. [1] https://gcdnb.pbrd.co/images/QYsCvhfQlX6n.png > and either assigned-clock-parents should be used, > or the clk_ops should have an init routine that looks at > which parent is present by determining the index and then use that to > set the mux. The framework can take care of failing to set the other > parent when it isn't present. On the board, at any moment, it will be only one clock as input to the VBATTB clock (either crystal oscillator or a clock device). If I'm getting you correctly, this will involve describing both clocks in some scenarios. E.g. if want to use crystal osc, I can use this DT description: vbattclk: clock-controller@1c { compatible = "renesas,r9a08g045-vbattb-clk"; reg = <0 0x1c 0 0x10>; clocks = <&vbattb_xtal>; clock-names = "xin"; #clock-cells = <0>; status = "disabled"; }; vbattb_xtal: vbattb-xtal { compatible = "fixed-clock"; #clock-cells = <0>; clock-frequency = <32768>; }; If external clock device is to be used, I should describe a fake clock too: vbattclk: clock-controller@1c { compatible = "renesas,r9a08g045-vbattb-clk"; reg = <0 0x1c 0 0x10>; clocks = <&vbattb_xtal>, <&ext_clk>; clock-names = "xin", "clkin"; #clock-cells = <0>; status = "disabled"; }; vbattb_xtal: vbattb-xtal { compatible = "fixed-clock"; #clock-cells = <0>; clock-frequency = <0>; }; ext_clk: ext-clk { compatible = "fixed-clock"; #clock-cells = <0>; clock-frequency = <32768>; }; Is this what you are suggesting? > >> + >> + spin_lock_init(&vbclk->lock); >> + >> + init.name = "vbattclk"; >> + init.ops = &vbattb_clk_ops; >> + init.parent_data = &parent_data; >> + init.num_parents = 1; >> + init.flags = 0; >> + >> + vbclk->hw.init = &init; >> + hw = &vbclk->hw; >> + >> + ret = devm_clk_hw_register(dev, hw); >> + if (ret) >> + return ret; >> + >> + return of_clk_add_hw_provider(np, of_clk_hw_simple_get, hw); >> +} >> + >> +static const struct of_device_id vbattb_clk_match[] = { >> + { .compatible = "renesas,r9a08g045-vbattb-clk" }, >> + { /* sentinel */ } >> +}; > > Any MODULE_DEVICE_TABLE? I failed to added it. Thank you for your review, Claudiu Beznea
Quoting claudiu beznea (2024-07-17 01:31:20) > Hi, Stephen, > > On 17.07.2024 01:28, Stephen Boyd wrote: > > Quoting Claudiu (2024-07-16 03:30:17) > >> diff --git a/drivers/clk/renesas/clk-vbattb.c b/drivers/clk/renesas/clk-vbattb.c > >> new file mode 100644 > >> index 000000000000..8effe141fc0b > >> --- /dev/null > >> +++ b/drivers/clk/renesas/clk-vbattb.c > >> @@ -0,0 +1,212 @@ > >> +// SPDX-License-Identifier: GPL-2.0 > >> +/* > >> + * VBATTB clock driver > >> + * > >> + * Copyright (C) 2024 Renesas Electronics Corp. > >> + */ > >> + > >> +#include <linux/cleanup.h> > >> +#include <linux/clk.h> > > > > Prefer clk providers to not be clk consumers. > > I added it here to be able to use devm_clk_get_optional() as it was > proposed to me in v1 to avoid adding a new binding for bypass and detect if > it's needed by checking the input clock name. > Understood. > > > > > I also wonder if this is really a mux, > > It's a way to determine what type of clock (crystal oscillator or device > clock) is connected to RTXIN/RTXOUT pins of the module > (the module block diagram at [1]) based on the clock name. Depending on the > type of the clock connected to RTXIN/RTXOUT we need to select the XC or > XBYP as input for the mux at [1]. > > [1] https://gcdnb.pbrd.co/images/QYsCvhfQlX6n.png That diagram shows a mux block, so at least something in there is a mux. From what I can tell the binding uses clock-names to describe the mux. What I'd like to avoid is using clk_get() to determine how to configure the mux. That's because clk_get() is a clk consumer API, and because we want clk providers to be able to register clks without making sure that the entire parent chain has been registered first. Eventually, we'd like clk_get() to probe defer if the clk is an orphan. Having clk providers use clk_get() breaks that pretty quickly. > > > > and either assigned-clock-parents should be used, > > or the clk_ops should have an init routine that looks at > > which parent is present by determining the index and then use that to > > set the mux. The framework can take care of failing to set the other > > parent when it isn't present. > > > On the board, at any moment, it will be only one clock as input to the > VBATTB clock (either crystal oscillator or a clock device). If I'm getting > you correctly, this will involve describing both clocks in some scenarios. > > E.g. if want to use crystal osc, I can use this DT description: > > vbattclk: clock-controller@1c { > compatible = "renesas,r9a08g045-vbattb-clk"; > reg = <0 0x1c 0 0x10>; > clocks = <&vbattb_xtal>; > clock-names = "xin"; > #clock-cells = <0>; > status = "disabled"; > }; > > vbattb_xtal: vbattb-xtal { > compatible = "fixed-clock"; > #clock-cells = <0>; > clock-frequency = <32768>; > }; > > If external clock device is to be used, I should describe a fake clock too: > > vbattclk: clock-controller@1c { > compatible = "renesas,r9a08g045-vbattb-clk"; > reg = <0 0x1c 0 0x10>; > clocks = <&vbattb_xtal>, <&ext_clk>; Is vbattb_xtal the fake clk? If so, I'd expect this to be clocks = <0>, <&ext_clk>; so that we don't have a useless clk node. > clock-names = "xin", "clkin"; > #clock-cells = <0>; > status = "disabled"; > }; > > vbattb_xtal: vbattb-xtal { > compatible = "fixed-clock"; > #clock-cells = <0>; > clock-frequency = <0>; > }; > > ext_clk: ext-clk { > compatible = "fixed-clock"; > #clock-cells = <0>; > clock-frequency = <32768>; > }; > > Is this what you are suggesting? > Sort of. Ignoring the problem with the subnode for the clk driver, I don't really like having clock-names that don't match the hardware pin names. From the diagram you provided, it looks like clock-names should be "bclk" and "rtxin" for the bus clock and the rtxin signal. Then the clock-cells should be "1" instead of "0", and the mux should be one of those provided clks and "xc" and "xbyp" should be the other two. If that was done, then assigned-clocks could be used to assign the parent of the mux. #define VBATTBCLK 0 #define VBATTB_XBYP 1 #define VBATTB_XC 2 vbattb: vbattb@1005c000 { compatible = "renesas,r9a08g045-vbattb"; reg = <0x1005c000 0x1000>; ranges = <0 0 0x1005c000 0 0x1000>; interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>; interrupt-names = "tampdi"; clocks = <&cpg CPG_MOD R9A08G045_VBAT_BCLK>, <&ext_clk>; clock-names = "bclk", "rtxin"; power-domains = <&cpg>; resets = <&cpg R9A08G045_VBAT_BRESETN>; #clock-cells = <1>; assigned-clocks = <&vbattb VBATTBCLK>; assigned-clock-parents = <&vbattb VBATTB_XBYP>; renesas,vbattb-load-nanofarads = <12500>; }; One last thing that I don't really understand is why this needs to be a clk provider. In the diagram, the RTC is also part of vbattb, so it looks odd to have this node be a clk provider with #clock-cells at all. Is it the case that if the rtxin pin is connected, you mux that over, and if the pin is disconnected you mux over the internal oscillator? I'm really wondering why a clk provider is implemented at all. Why not just hit the registers directly from the RTC driver depending on a devm_clk_get_optional() call?
On 18.07.2024 03:39, Stephen Boyd wrote: > Quoting claudiu beznea (2024-07-17 01:31:20) >> Hi, Stephen, >> >> On 17.07.2024 01:28, Stephen Boyd wrote: >>> Quoting Claudiu (2024-07-16 03:30:17) >>>> diff --git a/drivers/clk/renesas/clk-vbattb.c b/drivers/clk/renesas/clk-vbattb.c >>>> new file mode 100644 >>>> index 000000000000..8effe141fc0b >>>> --- /dev/null >>>> +++ b/drivers/clk/renesas/clk-vbattb.c >>>> @@ -0,0 +1,212 @@ >>>> +// SPDX-License-Identifier: GPL-2.0 >>>> +/* >>>> + * VBATTB clock driver >>>> + * >>>> + * Copyright (C) 2024 Renesas Electronics Corp. >>>> + */ >>>> + >>>> +#include <linux/cleanup.h> >>>> +#include <linux/clk.h> >>> >>> Prefer clk providers to not be clk consumers. >> >> I added it here to be able to use devm_clk_get_optional() as it was >> proposed to me in v1 to avoid adding a new binding for bypass and detect if >> it's needed by checking the input clock name. >> > > Understood. > >> >>> >>> I also wonder if this is really a mux, >> >> It's a way to determine what type of clock (crystal oscillator or device >> clock) is connected to RTXIN/RTXOUT pins of the module >> (the module block diagram at [1]) based on the clock name. Depending on the >> type of the clock connected to RTXIN/RTXOUT we need to select the XC or >> XBYP as input for the mux at [1]. >> >> [1] https://gcdnb.pbrd.co/images/QYsCvhfQlX6n.png > > That diagram shows a mux block, so at least something in there is a mux. > From what I can tell the binding uses clock-names to describe the mux. > What I'd like to avoid is using clk_get() to determine how to configure > the mux. That's because clk_get() is a clk consumer API, and because we > want clk providers to be able to register clks without making sure that > the entire parent chain has been registered first. Eventually, we'd like > clk_get() to probe defer if the clk is an orphan. Having clk providers > use clk_get() breaks that pretty quickly. > >> >> >>> and either assigned-clock-parents should be used, >>> or the clk_ops should have an init routine that looks at >>> which parent is present by determining the index and then use that to >>> set the mux. The framework can take care of failing to set the other >>> parent when it isn't present. >> >> >> On the board, at any moment, it will be only one clock as input to the >> VBATTB clock (either crystal oscillator or a clock device). If I'm getting >> you correctly, this will involve describing both clocks in some scenarios. >> >> E.g. if want to use crystal osc, I can use this DT description: >> >> vbattclk: clock-controller@1c { >> compatible = "renesas,r9a08g045-vbattb-clk"; >> reg = <0 0x1c 0 0x10>; >> clocks = <&vbattb_xtal>; >> clock-names = "xin"; >> #clock-cells = <0>; >> status = "disabled"; >> }; >> >> vbattb_xtal: vbattb-xtal { >> compatible = "fixed-clock"; >> #clock-cells = <0>; >> clock-frequency = <32768>; >> }; >> >> If external clock device is to be used, I should describe a fake clock too: >> >> vbattclk: clock-controller@1c { >> compatible = "renesas,r9a08g045-vbattb-clk"; >> reg = <0 0x1c 0 0x10>; >> clocks = <&vbattb_xtal>, <&ext_clk>; > > Is vbattb_xtal the fake clk? If so, I'd expect this to be > > clocks = <0>, <&ext_clk>; > > so that we don't have a useless clk node. > >> clock-names = "xin", "clkin"; >> #clock-cells = <0>; >> status = "disabled"; >> }; >> >> vbattb_xtal: vbattb-xtal { >> compatible = "fixed-clock"; >> #clock-cells = <0>; >> clock-frequency = <0>; >> }; >> >> ext_clk: ext-clk { >> compatible = "fixed-clock"; >> #clock-cells = <0>; >> clock-frequency = <32768>; >> }; >> >> Is this what you are suggesting? >> > > Sort of. Ignoring the problem with the subnode for the clk driver, I > don't really like having clock-names that don't match the hardware pin > names. From the diagram you provided, it looks like clock-names should > be "bclk" and "rtxin" for the bus clock and the rtxin signal. Then the > clock-cells should be "1" instead of "0", and the mux should be one of > those provided clks and "xc" and "xbyp" should be the other two. If that > was done, then assigned-clocks could be used to assign the parent of the > mux. > > #define VBATTBCLK 0 > #define VBATTB_XBYP 1 > #define VBATTB_XC 2 > > vbattb: vbattb@1005c000 { > compatible = "renesas,r9a08g045-vbattb"; > reg = <0x1005c000 0x1000>; > ranges = <0 0 0x1005c000 0 0x1000>; > interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>; > interrupt-names = "tampdi"; > clocks = <&cpg CPG_MOD R9A08G045_VBAT_BCLK>, <&ext_clk>; > clock-names = "bclk", "rtxin"; > power-domains = <&cpg>; > resets = <&cpg R9A08G045_VBAT_BRESETN>; > #clock-cells = <1>; > assigned-clocks = <&vbattb VBATTBCLK>; > assigned-clock-parents = <&vbattb VBATTB_XBYP>; > renesas,vbattb-load-nanofarads = <12500>; > }; I think I got it now. Thank you for the detailed explanation. > > One last thing that I don't really understand is why this needs to be a > clk provider. In the diagram, the RTC is also part of vbattb, so it > looks odd to have this node be a clk provider with #clock-cells at all. I did it like this because the RTC is a different IP mapped at it's own address and considering the other VBATTB functionalities (tamper, SRAM) might be implemented at some point. I also failed to notice that RTC might not work w/o bclk being enabled (thanks for pointing it). I saw that diagram more like describing the always-on power domain (PD_VBATTB) where the VBATTB logic and RTC resides. That power domain is backed by battery. From HW manual [1]: "PD_VBATT domain is the area where the RTC/backup register is located, works on battery power when the power of PD_VCC and PD_ISOVCC domain are turned off." [1] https://www.renesas.com/us/en/products/microcontrollers-microprocessors/rz-mpus/rzg3s-general-purpose-microprocessors-single-core-arm-cortex-a55-11-ghz-cpu-and-dual-core-cortex-m33-250 > Is it the case that if the rtxin pin is connected, you mux that over, > and if the pin is disconnected you mux over the internal oscillator? From the description here at [2] I'm getting that the "32-KHz clock oscillator" block is used when crystal oscillator is connected to RTXIN, RTXOUT pins and it is skipped if external clock device is connected. [2] https://i2.paste.pics/RFKJ0.png?rand=Xq8w1RLDvZ > I'm > really wondering why a clk provider is implemented at all. Why not just > hit the registers directly from the RTC driver depending on a > devm_clk_get_optional() call? I did it like this because the RTC is a different IP mapped at it's own address with it's own interrupts, clock, power domain and considering that the other VBATTB functionalities (tamper, SRAM) might be used at some point in future. At the same time I failed to noticed the VBATTB clock might be needed for RTC. Do you consider better to just take a regmap to VBATTB from RTC driver and set the VBATTB from RTC driver itself? Thank you, Claudiu Beznea
Quoting claudiu beznea (2024-07-18 07:41:03) > > > On 18.07.2024 03:39, Stephen Boyd wrote: > > > > Sort of. Ignoring the problem with the subnode for the clk driver, I > > don't really like having clock-names that don't match the hardware pin > > names. From the diagram you provided, it looks like clock-names should > > be "bclk" and "rtxin" for the bus clock and the rtxin signal. Then the > > clock-cells should be "1" instead of "0", and the mux should be one of > > those provided clks and "xc" and "xbyp" should be the other two. If that > > was done, then assigned-clocks could be used to assign the parent of the > > mux. > > > > #define VBATTBCLK 0 > > #define VBATTB_XBYP 1 > > #define VBATTB_XC 2 > > > > vbattb: vbattb@1005c000 { > > compatible = "renesas,r9a08g045-vbattb"; > > reg = <0x1005c000 0x1000>; > > ranges = <0 0 0x1005c000 0 0x1000>; > > interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>; > > interrupt-names = "tampdi"; > > clocks = <&cpg CPG_MOD R9A08G045_VBAT_BCLK>, <&ext_clk>; > > clock-names = "bclk", "rtxin"; > > power-domains = <&cpg>; > > resets = <&cpg R9A08G045_VBAT_BRESETN>; > > #clock-cells = <1>; > > assigned-clocks = <&vbattb VBATTBCLK>; > > assigned-clock-parents = <&vbattb VBATTB_XBYP>; > > renesas,vbattb-load-nanofarads = <12500>; > > }; > > I think I got it now. Thank you for the detailed explanation. > > > > > One last thing that I don't really understand is why this needs to be a > > clk provider. In the diagram, the RTC is also part of vbattb, so it > > looks odd to have this node be a clk provider with #clock-cells at all. > > I did it like this because the RTC is a different IP mapped at it's own > address and considering the other VBATTB functionalities (tamper, SRAM) > might be implemented at some point. > > I also failed to notice that RTC might not work w/o bclk being enabled > (thanks for pointing it). > > I saw that diagram more like describing the always-on power domain > (PD_VBATTB) where the VBATTB logic and RTC resides. That power domain is > backed by battery. From HW manual [1]: "PD_VBATT domain is the area where > the RTC/backup register is located, works on battery power when the power > of PD_VCC and PD_ISOVCC domain are turned off." Ah ok, so PD_VBATTB is like a power domain/wrapper and not an IP block mapped on the bus at the same address as the RTC that it wraps. That changes things a bit. > > [1] > https://www.renesas.com/us/en/products/microcontrollers-microprocessors/rz-mpus/rzg3s-general-purpose-microprocessors-single-core-arm-cortex-a55-11-ghz-cpu-and-dual-core-cortex-m33-250 > > > Is it the case that if the rtxin pin is connected, you mux that over, > > and if the pin is disconnected you mux over the internal oscillator? > > From the description here at [2] I'm getting that the "32-KHz clock > oscillator" block is used when crystal oscillator is connected to RTXIN, > RTXOUT pins and it is skipped if external clock device is connected. > > [2] https://i2.paste.pics/RFKJ0.png?rand=Xq8w1RLDvZ It looks like in both cases something is connected to the pins. XC is to use an external crystal connected to both pins and XBYP is to use a single clk. Given that, perhaps naming the clk "rtx" is simplest and then using assigned-clock-parents is most direct because there's only really one "logical" clk input for this device. That means the XC and XBYP clks have the same parent, "rtx", and the XC clk is a gateable fixed rate clk while the XBYP clk is a fixed factor clk that does nothing besides pass on the rate of the "rtx" clk. > > > I'm > > really wondering why a clk provider is implemented at all. Why not just > > hit the registers directly from the RTC driver depending on a > > devm_clk_get_optional() call? > > I did it like this because the RTC is a different IP mapped at it's own > address with it's own interrupts, clock, power domain and considering that > the other VBATTB functionalities (tamper, SRAM) might be used at some point > in future. At the same time I failed to noticed the VBATTB clock might be > needed for RTC. The docs say VBATT in some places. Not sure if you want to rename it to vbatt and drop the extra b which probably stands for "backup"? > > Do you consider better to just take a regmap to VBATTB from RTC driver and > set the VBATTB from RTC driver itself? No, don't do that. The only change from the above DT node is that the assigned-clocks and assigned-clock-parents property should be moved to the RTC node. vbattb: vbattb@1005c000 { compatible = "renesas,r9a08g045-vbattb"; reg = <0x1005c000 0x1000>; ranges = <0 0 0x1005c000 0 0x1000>; interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>; interrupt-names = "tampdi"; clocks = <&cpg CPG_MOD R9A08G045_VBAT_BCLK>, <&ext_clk>; clock-names = "bclk", "rtx"; power-domains = <&cpg>; resets = <&cpg R9A08G045_VBAT_BRESETN>; #clock-cells = <1>; renesas,vbattb-load-nanofarads = <12500>; }; rtc@1004ec00 { compatible = "renesas,r9a08g045-rtc"; reg = <0x1004ec00 0x400>; clocks = <&vbattb VBATTBCLK>; assigned-clocks = <&vbattb VBATTBCLK>; assigned-clock-parents = <&vbattb VBATTB_XBYP>; // Or VBATTB_XC if external crystal connected };
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 >
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 >> >
From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> Hi, On the Renesas RZ/G3S SoC the RTC clock is provided by the VBATTB IP. A 32 KHz crystall oscillator could be connected to the VBATTB input pins. The logic to control this clock (and pass it to RTC) is inside the VBATTB IP. For this, the clk-vbattb driver was added (patches 01-03/11). Patches 04-05/11 add the RTC driver. Patches 06-09/11 update the device trees with proper nodes to enable RTC. Patches 10-11/11 enable proper config flags for RTC to work on RZ/G3S SoC. Thank you, Claudiu Beznea Changes in v2: - dropped patch "clk: renesas: r9a08g045: Add clock, reset and power domain support for the VBATTB IP" as it was already integrated - kept only a documentation file for both VBATT MFD and clock drivers as suggested - addressed review comments - used cleanup.h lock helpers - update startup sequence for the RTC driver - switch to 24 hours mode on the RTC driver - fixed range for the RTC driver - added a generic compatible for the RTC driver as this will also be used by RZ/V2H - used clkin/xin clock names for the VBATTB clock driver to determine if bypass should be configured on registers instead of having dedicated DT property - added mfd driver for VBATTB - updated Kconfig flag names to include vendor name - removed DT node labels from Documentation files - used items to describe the interrupts and clocks Claudiu Beznea (11): dt-bindings: mfd: renesas,r9a08g045-vbattb: Document VBATTB mfd: renesas-vbattb: Add a MFD driver for the Renesas VBATTB IP clk: renesas: clk-vbattb: Add VBATTB clock driver dt-bindings: rtc: renesas,rzg3s-rtc: Document the Renesas RTCA-3 IP rtc: renesas-rtca3: Add driver for RTCA-3 available on Renesas RZ/G3S SoC arm64: dts: renesas: r9a08g045: Add VBATTB node arm64: dts: renesas: r9a08g045: Add RTC node arm64: dts: renesas: rzg3s-smarc-som: Enable VBATTB clock arm64: dts: renesas: rzg3s-smarc-som: Enable RTC arm64: defconfig: Enable VBATTB arm64: defconfig: Enable Renesas RTCA-3 flag .../mfd/renesas,r9a08g045-vbattb.yaml | 136 +++ .../bindings/rtc/renesas,rz-rtca3.yaml | 69 ++ MAINTAINERS | 8 + arch/arm64/boot/dts/renesas/r9a08g045.dtsi | 43 + .../boot/dts/renesas/rzg3s-smarc-som.dtsi | 17 + arch/arm64/configs/defconfig | 3 + drivers/clk/renesas/Kconfig | 5 + drivers/clk/renesas/Makefile | 1 + drivers/clk/renesas/clk-vbattb.c | 212 +++++ drivers/mfd/Kconfig | 8 + drivers/mfd/Makefile | 1 + drivers/mfd/renesas-vbattb.c | 78 ++ drivers/rtc/Kconfig | 10 + drivers/rtc/Makefile | 1 + drivers/rtc/rtc-renesas-rtca3.c | 853 ++++++++++++++++++ 15 files changed, 1445 insertions(+) create mode 100644 Documentation/devicetree/bindings/mfd/renesas,r9a08g045-vbattb.yaml create mode 100644 Documentation/devicetree/bindings/rtc/renesas,rz-rtca3.yaml create mode 100644 drivers/clk/renesas/clk-vbattb.c create mode 100644 drivers/mfd/renesas-vbattb.c create mode 100644 drivers/rtc/rtc-renesas-rtca3.c