[v2,02/15] soc: renesas: Add SYSC driver for Renesas RZ family

Commit Message

Claudiu Beznea Nov. 26, 2024, 9:20 a.m. UTC

From: Claudiu Beznea <[e-mail]>

The RZ/G3S system controller (SYSC) has various registers that control
signals specific to individual IPs. IP drivers must control these signals
at different configuration phases.

Add SYSC driver that allows individual SYSC consumers to control these
signals. The SYSC driver exports a syscon regmap enabling IP drivers to
use a specific SYSC offset and mask from the device tree, which can then be
accessed through regmap_update_bits().

Currently, the SYSC driver provides control to the USB PWRRDY signal, which
is routed to the USB PHY. This signal needs to be managed before or after
powering the USB PHY off or on.

Other SYSC signals candidates (as exposed in the the hardware manual of the
RZ/G3S SoC) include:

* PCIe:
- ALLOW_ENTER_L1 signal controlled through the SYS_PCIE_CFG register
- PCIE_RST_RSM_B signal controlled through the SYS_PCIE_RST_RSM_B
  register
- MODE_RXTERMINATION signal controlled through SYS_PCIE_PHY register

* SPI:
- SEL_SPI_OCTA signal controlled through SYS_IPCONT_SEL_SPI_OCTA
  register

* I2C/I3C:
- af_bypass I2C signals controlled through SYS_I2Cx_CFG registers
  (x=0..3)
- af_bypass I3C signal controlled through SYS_I3C_CFG register

* Ethernet:
- FEC_GIGA_ENABLE Ethernet signals controlled through SYS_GETHx_CFG
  registers (x=0..1)

As different Renesas RZ SoC shares most of the SYSC functionalities
available on the RZ/G3S SoC, the driver if formed of a SYSC core
part and a SoC specific part allowing individual SYSC SoC to provide
functionalities to the SYSC core.

Signed-off-by: Claudiu Beznea <[e-mail]>
---

Change in v2:
- this was patch 04/16 in v1
- dropped the initial approach proposed in v1 where a with a reset
  controller driver was proposed to handle the USB PWRRDY signal
- implemented it with syscon regmap and the SYSC signal concept
  (introduced in this patch)

 drivers/soc/renesas/Kconfig          |   7 +
 drivers/soc/renesas/Makefile         |   2 +
 drivers/soc/renesas/r9a08g045-sysc.c |  31 +++
 drivers/soc/renesas/rz-sysc.c        | 286 +++++++++++++++++++++++++++
 drivers/soc/renesas/rz-sysc.h        |  52 +++++
 5 files changed, 378 insertions(+)
 create mode 100644 drivers/soc/renesas/r9a08g045-sysc.c
 create mode 100644 drivers/soc/renesas/rz-sysc.c
 create mode 100644 drivers/soc/renesas/rz-sysc.h

Comments

Biju Das Nov. 26, 2024, 9:28 a.m. UTC | #1

Hi Claudiu,

Thanks for the patch.

> -----Original Message-----
> From: Claudiu <[e-mail]>
> Sent: 26 November 2024 09:21
> Subject: [PATCH v2 02/15] soc: renesas: Add SYSC driver for Renesas RZ family
> 
> From: Claudiu Beznea <[e-mail]>
> 
> The RZ/G3S system controller (SYSC) has various registers that control signals specific to individual
> IPs. IP drivers must control these signals at different configuration phases.
> 
> Add SYSC driver that allows individual SYSC consumers to control these signals. The SYSC driver
> exports a syscon regmap enabling IP drivers to use a specific SYSC offset and mask from the device
> tree, which can then be accessed through regmap_update_bits().
> 
> Currently, the SYSC driver provides control to the USB PWRRDY signal, which is routed to the USB PHY.
> This signal needs to be managed before or after powering the USB PHY off or on.
> 
> Other SYSC signals candidates (as exposed in the the hardware manual of the RZ/G3S SoC) include:
> 
> * PCIe:
> - ALLOW_ENTER_L1 signal controlled through the SYS_PCIE_CFG register
> - PCIE_RST_RSM_B signal controlled through the SYS_PCIE_RST_RSM_B
>   register
> - MODE_RXTERMINATION signal controlled through SYS_PCIE_PHY register
> 
> * SPI:
> - SEL_SPI_OCTA signal controlled through SYS_IPCONT_SEL_SPI_OCTA
>   register
> 
> * I2C/I3C:
> - af_bypass I2C signals controlled through SYS_I2Cx_CFG registers
>   (x=0..3)
> - af_bypass I3C signal controlled through SYS_I3C_CFG register
> 
> * Ethernet:
> - FEC_GIGA_ENABLE Ethernet signals controlled through SYS_GETHx_CFG
>   registers (x=0..1)
> 
> As different Renesas RZ SoC shares most of the SYSC functionalities available on the RZ/G3S SoC, the
> driver if formed of a SYSC core part and a SoC specific part allowing individual SYSC SoC to provide
> functionalities to the SYSC core.
> 
> Signed-off-by: Claudiu Beznea <[e-mail]>

Reviewed-by: Biju Das <[e-mail]>

Cheers,
Biju

> ---
> 
> Change in v2:
> - this was patch 04/16 in v1
> - dropped the initial approach proposed in v1 where a with a reset
>   controller driver was proposed to handle the USB PWRRDY signal
> - implemented it with syscon regmap and the SYSC signal concept
>   (introduced in this patch)
> 
>  drivers/soc/renesas/Kconfig          |   7 +
>  drivers/soc/renesas/Makefile         |   2 +
>  drivers/soc/renesas/r9a08g045-sysc.c |  31 +++
>  drivers/soc/renesas/rz-sysc.c        | 286 +++++++++++++++++++++++++++
>  drivers/soc/renesas/rz-sysc.h        |  52 +++++
>  5 files changed, 378 insertions(+)
>  create mode 100644 drivers/soc/renesas/r9a08g045-sysc.c
>  create mode 100644 drivers/soc/renesas/rz-sysc.c  create mode 100644 drivers/soc/renesas/rz-sysc.h
> 
> diff --git a/drivers/soc/renesas/Kconfig b/drivers/soc/renesas/Kconfig index
> 9f7fe02310b9..0686c3ad9e27 100644
> --- a/drivers/soc/renesas/Kconfig
> +++ b/drivers/soc/renesas/Kconfig
> @@ -378,4 +378,11 @@ config PWC_RZV2M
>  config RST_RCAR
>  	bool "Reset Controller support for R-Car" if COMPILE_TEST
> 
> +config SYSC_RZ
> +	bool "System controller for RZ SoCs" if COMPILE_TEST
> +
> +config SYSC_R9A08G045
> +	bool "Renesas RZ/G3S System controller support" if COMPILE_TEST
> +	select SYSC_RZ
> +
>  endif # SOC_RENESAS
> diff --git a/drivers/soc/renesas/Makefile b/drivers/soc/renesas/Makefile index
> 734f8f8cefa4..8cd139b3dd0a 100644
> --- a/drivers/soc/renesas/Makefile
> +++ b/drivers/soc/renesas/Makefile
> @@ -6,7 +6,9 @@ obj-$(CONFIG_SOC_RENESAS)	+= renesas-soc.o
>  ifdef CONFIG_SMP
>  obj-$(CONFIG_ARCH_R9A06G032)	+= r9a06g032-smp.o
>  endif
> +obj-$(CONFIG_SYSC_R9A08G045)	+= r9a08g045-sysc.o
> 
>  # Family
>  obj-$(CONFIG_PWC_RZV2M)		+= pwc-rzv2m.o
>  obj-$(CONFIG_RST_RCAR)		+= rcar-rst.o
> +obj-$(CONFIG_SYSC_RZ)		+= rz-sysc.o
> diff --git a/drivers/soc/renesas/r9a08g045-sysc.c b/drivers/soc/renesas/r9a08g045-sysc.c
> new file mode 100644
> index 000000000000..ceea738aee72
> --- /dev/null
> +++ b/drivers/soc/renesas/r9a08g045-sysc.c
> @@ -0,0 +1,31 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * RZ/G3S System controller driver
> + *
> + * Copyright (C) 2024 Renesas Electronics Corp.
> + */
> +
> +#include <linux/array_size.h>
> +#include <linux/bits.h>
> +#include <linux/init.h>
> +
> +#include "rz-sysc.h"
> +
> +#define SYS_USB_PWRRDY		0xd70
> +#define SYS_USB_PWRRDY_PWRRDY_N	BIT(0)
> +#define SYS_MAX_REG		0xe20
> +
> +static const struct rz_sysc_signal_init_data rzg3s_sysc_signals_init_data[] __initconst = {
> +	{
> +		.name = "usb-pwrrdy",
> +		.offset = SYS_USB_PWRRDY,
> +		.mask = SYS_USB_PWRRDY_PWRRDY_N,
> +		.refcnt_incr_val = 0
> +	}
> +};
> +
> +const struct rz_sysc_init_data rzg3s_sysc_init_data = {
> +	.signals_init_data = rzg3s_sysc_signals_init_data,
> +	.num_signals = ARRAY_SIZE(rzg3s_sysc_signals_init_data),
> +	.max_register_offset = SYS_MAX_REG,
> +};
> diff --git a/drivers/soc/renesas/rz-sysc.c b/drivers/soc/renesas/rz-sysc.c new file mode 100644 index
> 000000000000..dc0edacd7170
> --- /dev/null
> +++ b/drivers/soc/renesas/rz-sysc.c
> @@ -0,0 +1,286 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * RZ System controller driver
> + *
> + * Copyright (C) 2024 Renesas Electronics Corp.
> + */
> +
> +#include <linux/dcache.h>
> +#include <linux/debugfs.h>
> +#include <linux/io.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/refcount.h>
> +#include <linux/regmap.h>
> +#include <linux/seq_file.h>
> +
> +#include "rz-sysc.h"
> +
> +/**
> + * struct rz_sysc - RZ SYSC private data structure
> + * @base: SYSC base address
> + * @dev: SYSC device pointer
> + * @signals: SYSC signals
> + * @num_signals: number of SYSC signals  */ struct rz_sysc {
> +	void __iomem *base;
> +	struct device *dev;
> +	struct rz_sysc_signal *signals;
> +	u8 num_signals;
> +};
> +
> +static int rz_sysc_reg_read(void *context, unsigned int off, unsigned
> +int *val) {
> +	struct rz_sysc *sysc = context;
> +
> +	*val = readl(sysc->base + off);
> +
> +	return 0;
> +}
> +
> +static struct rz_sysc_signal *rz_sysc_off_to_signal(struct rz_sysc *sysc, unsigned int offset,
> +						    unsigned int mask)
> +{
> +	struct rz_sysc_signal *signals = sysc->signals;
> +
> +	for (u32 i = 0; i < sysc->num_signals; i++) {
> +		if (signals[i].init_data->offset != offset)
> +			continue;
> +
> +		/*
> +		 * In case mask == 0 we just return the signal data w/o checking the mask.
> +		 * This is useful when calling through rz_sysc_reg_write() to check
> +		 * if the requested setting is for a mapped signal or not.
> +		 */
> +		if (mask) {
> +			if (signals[i].init_data->mask == mask)
> +				return &signals[i];
> +		} else {
> +			return &signals[i];
> +		}
> +	}
> +
> +	return NULL;
> +}
> +
> +static int rz_sysc_reg_update_bits(void *context, unsigned int off,
> +				   unsigned int mask, unsigned int val) {
> +	struct rz_sysc *sysc = context;
> +	struct rz_sysc_signal *signal;
> +	bool update = false;
> +
> +	signal = rz_sysc_off_to_signal(sysc, off, mask);
> +	if (signal) {
> +		if (signal->init_data->refcnt_incr_val == val) {
> +			if (!refcount_read(&signal->refcnt)) {
> +				refcount_set(&signal->refcnt, 1);
> +				update = true;
> +			} else {
> +				refcount_inc(&signal->refcnt);
> +			}
> +		} else {
> +			update = refcount_dec_and_test(&signal->refcnt);
> +		}
> +	} else {
> +		update = true;
> +	}
> +
> +	if (update) {
> +		u32 tmp;
> +
> +		tmp = readl(sysc->base + off);
> +		tmp &= ~mask;
> +		tmp |= val & mask;
> +		writel(tmp, sysc->base + off);
> +	}
> +
> +	return 0;
> +}
> +
> +static int rz_sysc_reg_write(void *context, unsigned int off, unsigned
> +int val) {
> +	struct rz_sysc *sysc = context;
> +	struct rz_sysc_signal *signal;
> +
> +	/*
> +	 * Force using regmap_update_bits() for signals to have reference counter
> +	 * per individual signal in case there are multiple signals controlled
> +	 * through the same register.
> +	 */
> +	signal = rz_sysc_off_to_signal(sysc, off, 0);
> +	if (signal) {
> +		dev_err(sysc->dev,
> +			"regmap_write() not allowed on register controlling a signal. Use
> regmap_update_bits()!");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	writel(val, sysc->base + off);
> +
> +	return 0;
> +}
> +
> +static bool rz_sysc_writeable_reg(struct device *dev, unsigned int off)
> +{
> +	struct rz_sysc *sysc = dev_get_drvdata(dev);
> +	struct rz_sysc_signal *signal;
> +
> +	/* Any register containing a signal is writeable. */
> +	signal = rz_sysc_off_to_signal(sysc, off, 0);
> +	if (signal)
> +		return true;
> +
> +	return false;
> +}
> +
> +static bool rz_sysc_readable_reg(struct device *dev, unsigned int off)
> +{
> +	struct rz_sysc *sysc = dev_get_drvdata(dev);
> +	struct rz_sysc_signal *signal;
> +
> +	/* Any register containing a signal is readable. */
> +	signal = rz_sysc_off_to_signal(sysc, off, 0);
> +	if (signal)
> +		return true;
> +
> +	return false;
> +}
> +
> +static int rz_sysc_signals_show(struct seq_file *s, void *what) {
> +	struct rz_sysc *sysc = s->private;
> +
> +	seq_printf(s, "%-20s Enable count\n", "Signal");
> +	seq_printf(s, "%-20s ------------\n", "--------------------");
> +
> +	for (u8 i = 0; i < sysc->num_signals; i++) {
> +		seq_printf(s, "%-20s %d\n", sysc->signals[i].init_data->name,
> +			   refcount_read(&sysc->signals[i].refcnt));
> +	}
> +
> +	return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(rz_sysc_signals);
> +
> +static void rz_sysc_debugfs_remove(void *data) {
> +	debugfs_remove_recursive(data);
> +}
> +
> +static int rz_sysc_signals_init(struct rz_sysc *sysc,
> +				const struct rz_sysc_signal_init_data *init_data,
> +				u32 num_signals)
> +{
> +	struct dentry *root;
> +	int ret;
> +
> +	sysc->signals = devm_kcalloc(sysc->dev, num_signals, sizeof(*sysc->signals),
> +				     GFP_KERNEL);
> +	if (!sysc->signals)
> +		return -ENOMEM;
> +
> +	for (u32 i = 0; i < num_signals; i++) {
> +		struct rz_sysc_signal_init_data *id;
> +
> +		id = devm_kzalloc(sysc->dev, sizeof(*id), GFP_KERNEL);
> +		if (!id)
> +			return -ENOMEM;
> +
> +		id->name = devm_kstrdup(sysc->dev, init_data->name, GFP_KERNEL);
> +		if (!id->name)
> +			return -ENOMEM;
> +
> +		id->offset = init_data->offset;
> +		id->mask = init_data->mask;
> +		id->refcnt_incr_val = init_data->refcnt_incr_val;
> +
> +		sysc->signals[i].init_data = id;
> +		refcount_set(&sysc->signals[i].refcnt, 0);
> +	}
> +
> +	sysc->num_signals = num_signals;
> +
> +	root = debugfs_create_dir("renesas-rz-sysc", NULL);
> +	ret = devm_add_action_or_reset(sysc->dev, rz_sysc_debugfs_remove, root);
> +	if (ret)
> +		return ret;
> +	debugfs_create_file("signals", 0444, root, sysc,
> +&rz_sysc_signals_fops);
> +
> +	return 0;
> +}
> +
> +static struct regmap_config rz_sysc_regmap = {
> +	.name = "rz_sysc_regs",
> +	.reg_bits = 32,
> +	.reg_stride = 4,
> +	.val_bits = 32,
> +	.fast_io = true,
> +	.reg_read = rz_sysc_reg_read,
> +	.reg_write = rz_sysc_reg_write,
> +	.reg_update_bits = rz_sysc_reg_update_bits,
> +	.writeable_reg = rz_sysc_writeable_reg,
> +	.readable_reg = rz_sysc_readable_reg,
> +};
> +
> +static const struct of_device_id rz_sysc_match[] = { #ifdef
> +CONFIG_SYSC_R9A08G045
> +	{ .compatible = "renesas,r9a08g045-sysc", .data =
> +&rzg3s_sysc_init_data }, #endif
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, rz_sysc_match);
> +
> +static int rz_sysc_probe(struct platform_device *pdev) {
> +	const struct rz_sysc_init_data *data;
> +	struct device *dev = &pdev->dev;
> +	struct rz_sysc *sysc;
> +	struct regmap *regmap;
> +	int ret;
> +
> +	data = device_get_match_data(dev);
> +	if (!data || !data->max_register_offset)
> +		return -EINVAL;
> +
> +	sysc = devm_kzalloc(dev, sizeof(*sysc), GFP_KERNEL);
> +	if (!sysc)
> +		return -ENOMEM;
> +
> +	sysc->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(sysc->base))
> +		return PTR_ERR(sysc->base);
> +
> +	sysc->dev = dev;
> +
> +	ret = rz_sysc_signals_init(sysc, data->signals_init_data, data->num_signals);
> +	if (ret)
> +		return ret;
> +
> +	dev_set_drvdata(dev, sysc);
> +	rz_sysc_regmap.max_register = data->max_register_offset;
> +	regmap = devm_regmap_init(dev, NULL, sysc, &rz_sysc_regmap);
> +	if (IS_ERR(regmap))
> +		return PTR_ERR(regmap);
> +
> +	return of_syscon_register_regmap(dev->of_node, regmap); }
> +
> +static struct platform_driver rz_sysc_driver = {
> +	.driver = {
> +		.name = "renesas-rz-sysc",
> +		.of_match_table = rz_sysc_match
> +	},
> +	.probe = rz_sysc_probe
> +};
> +
> +static int __init rz_sysc_init(void)
> +{
> +	return platform_driver_register(&rz_sysc_driver);
> +}
> +subsys_initcall(rz_sysc_init);
> +
> +MODULE_DESCRIPTION("Renesas RZ System Controller Driver");
> +MODULE_AUTHOR("Claudiu Beznea <[e-mail]>");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/soc/renesas/rz-sysc.h b/drivers/soc/renesas/rz-sysc.h new file mode 100644 index
> 000000000000..bb850310c931
> --- /dev/null
> +++ b/drivers/soc/renesas/rz-sysc.h
> @@ -0,0 +1,52 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Renesas RZ System Controller
> + *
> + * Copyright (C) 2024 Renesas Electronics Corp.
> + */
> +
> +#ifndef __SOC_RENESAS_RZ_SYSC_H__
> +#define __SOC_RENESAS_RZ_SYSC_H__
> +
> +#include <linux/refcount.h>
> +#include <linux/types.h>
> +
> +/**
> + * struct rz_sysc_signal_init_data - RZ SYSC signals init data
> + * @name: signal name
> + * @offset: register offset controling this signal
> + * @mask: bitmask in register specific to this signal
> + * @refcnt_incr_val: increment refcnt when setting this value  */
> +struct rz_sysc_signal_init_data {
> +	const char *name;
> +	u32 offset;
> +	u32 mask;
> +	u32 refcnt_incr_val;
> +};
> +
> +/**
> + * struct rz_sysc_signal - RZ SYSC signals
> + * @init_data: signals initialization data
> + * @refcnt: reference counter
> + */
> +struct rz_sysc_signal {
> +	const struct rz_sysc_signal_init_data *init_data;
> +	refcount_t refcnt;
> +};
> +
> +/**
> + * struct rz_sysc_init_data - RZ SYSC initialization data
> + * @signals_init_data: RZ SYSC signals initialization data
> + * @num_signals: number of SYSC signals
> + * @max_register_offset: Maximum SYSC register offset to be used by the
> +regmap config  */ struct rz_sysc_init_data {
> +	const struct rz_sysc_signal_init_data *signals_init_data;
> +	u32 num_signals;
> +	u32 max_register_offset;
> +};
> +
> +extern const struct rz_sysc_init_data rzg3s_sysc_init_data;
> +
> +#endif /* __SOC_RENESAS_RZ_SYSC_H__ */
> --
> 2.39.2
>

Geert Uytterhoeven Nov. 26, 2024, 1:42 p.m. UTC | #2

Hi Biju,

On Tue, Nov 26, 2024 at 10:28 AM Biju Das <[e-mail]> wrote:
> > -----Original Message-----
> > From: Claudiu <[e-mail]>
> > Sent: 26 November 2024 09:21
> > Subject: [PATCH v2 02/15] soc: renesas: Add SYSC driver for Renesas RZ family
> >
> > From: Claudiu Beznea <[e-mail]>
> >
> > The RZ/G3S system controller (SYSC) has various registers that control signals specific to individual
> > IPs. IP drivers must control these signals at different configuration phases.
> >
> > Add SYSC driver that allows individual SYSC consumers to control these signals. The SYSC driver
> > exports a syscon regmap enabling IP drivers to use a specific SYSC offset and mask from the device
> > tree, which can then be accessed through regmap_update_bits().
> >
> > Currently, the SYSC driver provides control to the USB PWRRDY signal, which is routed to the USB PHY.
> > This signal needs to be managed before or after powering the USB PHY off or on.
> >
> > Other SYSC signals candidates (as exposed in the the hardware manual of the RZ/G3S SoC) include:
> >
> > * PCIe:
> > - ALLOW_ENTER_L1 signal controlled through the SYS_PCIE_CFG register
> > - PCIE_RST_RSM_B signal controlled through the SYS_PCIE_RST_RSM_B
> >   register
> > - MODE_RXTERMINATION signal controlled through SYS_PCIE_PHY register
> >
> > * SPI:
> > - SEL_SPI_OCTA signal controlled through SYS_IPCONT_SEL_SPI_OCTA
> >   register
> >
> > * I2C/I3C:
> > - af_bypass I2C signals controlled through SYS_I2Cx_CFG registers
> >   (x=0..3)
> > - af_bypass I3C signal controlled through SYS_I3C_CFG register
> >
> > * Ethernet:
> > - FEC_GIGA_ENABLE Ethernet signals controlled through SYS_GETHx_CFG
> >   registers (x=0..1)
> >
> > As different Renesas RZ SoC shares most of the SYSC functionalities available on the RZ/G3S SoC, the
> > driver if formed of a SYSC core part and a SoC specific part allowing individual SYSC SoC to provide
> > functionalities to the SYSC core.
> >
> > Signed-off-by: Claudiu Beznea <[e-mail]>
>
> Reviewed-by: Biju Das <[e-mail]>

Thanks for your review!

> > ---
> >
> > Change in v2:
> > - this was patch 04/16 in v1
> > - dropped the initial approach proposed in v1 where a with a reset
> >   controller driver was proposed to handle the USB PWRRDY signal
> > - implemented it with syscon regmap and the SYSC signal concept
> >   (introduced in this patch)

[...]

When reviewing, please trim your response, so other people don't have
to scroll through hundreds of lines of quoted text, to find any other
comments (if any).

Thanks!

Gr{oetje,eeting}s,

                        Geert

Biju Das Nov. 26, 2024, 1:44 p.m. UTC | #3

Hi Geert,

> -----Original Message-----
> From: Geert Uytterhoeven <[e-mail]>
> Sent: 26 November 2024 13:42
> Subject: Re: [PATCH v2 02/15] soc: renesas: Add SYSC driver for Renesas RZ family
> 
> Hi Biju,
> 
> On Tue, Nov 26, 2024 at 10:28 AM Biju Das <[e-mail]> wrote:
> > > -----Original Message-----
> > > From: Claudiu <[e-mail]>
> > > Sent: 26 November 2024 09:21
> > > Subject: [PATCH v2 02/15] soc: renesas: Add SYSC driver for Renesas
> > > RZ family
> > >
> > > From: Claudiu Beznea <[e-mail]>
> > >
> > > The RZ/G3S system controller (SYSC) has various registers that
> > > control signals specific to individual IPs. IP drivers must control these signals at different
> configuration phases.
> > >
> > > Add SYSC driver that allows individual SYSC consumers to control
> > > these signals. The SYSC driver exports a syscon regmap enabling IP
> > > drivers to use a specific SYSC offset and mask from the device tree, which can then be accessed
> through regmap_update_bits().
> > >
> > > Currently, the SYSC driver provides control to the USB PWRRDY signal, which is routed to the USB
> PHY.
> > > This signal needs to be managed before or after powering the USB PHY off or on.
> > >
> > > Other SYSC signals candidates (as exposed in the the hardware manual of the RZ/G3S SoC) include:
> > >
> > > * PCIe:
> > > - ALLOW_ENTER_L1 signal controlled through the SYS_PCIE_CFG register
> > > - PCIE_RST_RSM_B signal controlled through the SYS_PCIE_RST_RSM_B
> > >   register
> > > - MODE_RXTERMINATION signal controlled through SYS_PCIE_PHY register
> > >
> > > * SPI:
> > > - SEL_SPI_OCTA signal controlled through SYS_IPCONT_SEL_SPI_OCTA
> > >   register
> > >
> > > * I2C/I3C:
> > > - af_bypass I2C signals controlled through SYS_I2Cx_CFG registers
> > >   (x=0..3)
> > > - af_bypass I3C signal controlled through SYS_I3C_CFG register
> > >
> > > * Ethernet:
> > > - FEC_GIGA_ENABLE Ethernet signals controlled through SYS_GETHx_CFG
> > >   registers (x=0..1)
> > >
> > > As different Renesas RZ SoC shares most of the SYSC functionalities
> > > available on the RZ/G3S SoC, the driver if formed of a SYSC core
> > > part and a SoC specific part allowing individual SYSC SoC to provide functionalities to the SYSC
> core.
> > >
> > > Signed-off-by: Claudiu Beznea <[e-mail]>
> >
> > Reviewed-by: Biju Das <[e-mail]>
> 
> Thanks for your review!
> 
> > > ---
> > >
> > > Change in v2:
> > > - this was patch 04/16 in v1
> > > - dropped the initial approach proposed in v1 where a with a reset
> > >   controller driver was proposed to handle the USB PWRRDY signal
> > > - implemented it with syscon regmap and the SYSC signal concept
> > >   (introduced in this patch)
> 
> [...]
> 
> When reviewing, please trim your response, so other people don't have to scroll through hundreds of
> lines of quoted text, to find any other comments (if any).

Sorry for that. Will take care next time.

Cheers,
Biju

Geert Uytterhoeven Nov. 28, 2024, 3:24 p.m. UTC | #4

Hi Claudiu,

Thanks for your patch!

On Tue, Nov 26, 2024 at 10:21 AM Claudiu <[e-mail]> wrote:
> From: Claudiu Beznea <[e-mail]>
>
> The RZ/G3S system controller (SYSC) has various registers that control
> signals specific to individual IPs. IP drivers must control these signals
> at different configuration phases.
>
> Add SYSC driver that allows individual SYSC consumers to control these
> signals. The SYSC driver exports a syscon regmap enabling IP drivers to
> use a specific SYSC offset and mask from the device tree, which can then be
> accessed through regmap_update_bits().
>
> Currently, the SYSC driver provides control to the USB PWRRDY signal, which
> is routed to the USB PHY. This signal needs to be managed before or after
> powering the USB PHY off or on.
>
> Other SYSC signals candidates (as exposed in the the hardware manual of the

s/the the/the/

> RZ/G3S SoC) include:
>
> * PCIe:
> - ALLOW_ENTER_L1 signal controlled through the SYS_PCIE_CFG register
> - PCIE_RST_RSM_B signal controlled through the SYS_PCIE_RST_RSM_B
>   register
> - MODE_RXTERMINATION signal controlled through SYS_PCIE_PHY register
>
> * SPI:
> - SEL_SPI_OCTA signal controlled through SYS_IPCONT_SEL_SPI_OCTA
>   register
>
> * I2C/I3C:
> - af_bypass I2C signals controlled through SYS_I2Cx_CFG registers
>   (x=0..3)
> - af_bypass I3C signal controlled through SYS_I3C_CFG register
>
> * Ethernet:
> - FEC_GIGA_ENABLE Ethernet signals controlled through SYS_GETHx_CFG
>   registers (x=0..1)
>
> As different Renesas RZ SoC shares most of the SYSC functionalities
> available on the RZ/G3S SoC, the driver if formed of a SYSC core
> part and a SoC specific part allowing individual SYSC SoC to provide
> functionalities to the SYSC core.
>
> Signed-off-by: Claudiu Beznea <[e-mail]>

> --- /dev/null
> +++ b/drivers/soc/renesas/r9a08g045-sysc.c
> @@ -0,0 +1,31 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * RZ/G3S System controller driver
> + *
> + * Copyright (C) 2024 Renesas Electronics Corp.
> + */
> +
> +#include <linux/array_size.h>
> +#include <linux/bits.h>
> +#include <linux/init.h>
> +
> +#include "rz-sysc.h"
> +
> +#define SYS_USB_PWRRDY         0xd70
> +#define SYS_USB_PWRRDY_PWRRDY_N        BIT(0)
> +#define SYS_MAX_REG            0xe20
> +
> +static const struct rz_sysc_signal_init_data rzg3s_sysc_signals_init_data[] __initconst = {

This is marked __initconst...

> +       {
> +               .name = "usb-pwrrdy",
> +               .offset = SYS_USB_PWRRDY,
> +               .mask = SYS_USB_PWRRDY_PWRRDY_N,
> +               .refcnt_incr_val = 0
> +       }
> +};
> +
> +const struct rz_sysc_init_data rzg3s_sysc_init_data = {

... but this is not __init, causing a section mismatch.

> +       .signals_init_data = rzg3s_sysc_signals_init_data,
> +       .num_signals = ARRAY_SIZE(rzg3s_sysc_signals_init_data),
> +       .max_register_offset = SYS_MAX_REG,
> +};

> --- /dev/null
> +++ b/drivers/soc/renesas/rz-sysc.c

> +/**
> + * struct rz_sysc - RZ SYSC private data structure
> + * @base: SYSC base address
> + * @dev: SYSC device pointer
> + * @signals: SYSC signals
> + * @num_signals: number of SYSC signals
> + */
> +struct rz_sysc {
> +       void __iomem *base;
> +       struct device *dev;
> +       struct rz_sysc_signal *signals;
> +       u8 num_signals;

You could change signals to a flexible array at the end, tag it with
__counted_by(num_signals), and allocate space for both struct rz_sysc
and the signals array using struct_size(), reducing the number of
allocations.

> +};

> +static struct rz_sysc_signal *rz_sysc_off_to_signal(struct rz_sysc *sysc, unsigned int offset,
> +                                                   unsigned int mask)
> +{
> +       struct rz_sysc_signal *signals = sysc->signals;
> +
> +       for (u32 i = 0; i < sysc->num_signals; i++) {

s/u32/unsigned int/

> +               if (signals[i].init_data->offset != offset)
> +                       continue;
> +
> +               /*
> +                * In case mask == 0 we just return the signal data w/o checking the mask.
> +                * This is useful when calling through rz_sysc_reg_write() to check
> +                * if the requested setting is for a mapped signal or not.
> +                */
> +               if (mask) {
> +                       if (signals[i].init_data->mask == mask)
> +                               return &signals[i];
> +               } else {
> +                       return &signals[i];
> +               }

if (!mask || signals[i].init_data->mask == mask)
        return &signals[i];

> +       }
> +
> +       return NULL;
> +}
> +
> +static int rz_sysc_reg_update_bits(void *context, unsigned int off,
> +                                  unsigned int mask, unsigned int val)
> +{
> +       struct rz_sysc *sysc = context;
> +       struct rz_sysc_signal *signal;
> +       bool update = false;
> +
> +       signal = rz_sysc_off_to_signal(sysc, off, mask);
> +       if (signal) {
> +               if (signal->init_data->refcnt_incr_val == val) {
> +                       if (!refcount_read(&signal->refcnt)) {
> +                               refcount_set(&signal->refcnt, 1);
> +                               update = true;
> +                       } else {
> +                               refcount_inc(&signal->refcnt);
> +                       }
> +               } else {
> +                       update = refcount_dec_and_test(&signal->refcnt);
> +               }
> +       } else {
> +               update = true;
> +       }

You could reduce indentation/number of lines by reordering the logic:

    if (!signal) {
            update = true;
    } else if (signal->init_data->refcnt_incr_val != val) {
            update = refcount_dec_and_test(&signal->refcnt);
    } else if (!refcount_read(&signal->refcnt)) {
            refcount_set(&signal->refcnt, 1);
            update = true;
    } else {
            refcount_inc(&signal->refcnt);
    }

> +
> +       if (update) {
> +               u32 tmp;
> +
> +               tmp = readl(sysc->base + off);
> +               tmp &= ~mask;
> +               tmp |= val & mask;
> +               writel(tmp, sysc->base + off);
> +       }
> +
> +       return 0;
> +}
> +
> +static int rz_sysc_reg_write(void *context, unsigned int off, unsigned int val)
> +{
> +       struct rz_sysc *sysc = context;
> +       struct rz_sysc_signal *signal;
> +
> +       /*
> +        * Force using regmap_update_bits() for signals to have reference counter
> +        * per individual signal in case there are multiple signals controlled
> +        * through the same register.
> +        */
> +       signal = rz_sysc_off_to_signal(sysc, off, 0);
> +       if (signal) {
> +               dev_err(sysc->dev,
> +                       "regmap_write() not allowed on register controlling a signal. Use regmap_update_bits()!");
> +               return -EOPNOTSUPP;
> +       }
> +

Can you ever get here, given rz_sysc_writeable_reg() below would have
returned false? If not, is there any point in having this function?

> +       writel(val, sysc->base + off);
> +
> +       return 0;
> +}
> +
> +static bool rz_sysc_writeable_reg(struct device *dev, unsigned int off)
> +{
> +       struct rz_sysc *sysc = dev_get_drvdata(dev);
> +       struct rz_sysc_signal *signal;
> +
> +       /* Any register containing a signal is writeable. */
> +       signal = rz_sysc_off_to_signal(sysc, off, 0);
> +       if (signal)
> +               return true;
> +
> +       return false;
> +}

> +static int rz_sysc_signals_show(struct seq_file *s, void *what)
> +{
> +       struct rz_sysc *sysc = s->private;
> +
> +       seq_printf(s, "%-20s Enable count\n", "Signal");
> +       seq_printf(s, "%-20s ------------\n", "--------------------");
> +
> +       for (u8 i = 0; i < sysc->num_signals; i++) {
> +               seq_printf(s, "%-20s %d\n", sysc->signals[i].init_data->name,
> +                          refcount_read(&sysc->signals[i].refcnt));
> +       }
> +
> +       return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(rz_sysc_signals);

What is the use-case for this? Just (initial) debugging?

> +
> +static void rz_sysc_debugfs_remove(void *data)
> +{
> +       debugfs_remove_recursive(data);
> +}
> +
> +static int rz_sysc_signals_init(struct rz_sysc *sysc,
> +                               const struct rz_sysc_signal_init_data *init_data,
> +                               u32 num_signals)
> +{
> +       struct dentry *root;
> +       int ret;
> +
> +       sysc->signals = devm_kcalloc(sysc->dev, num_signals, sizeof(*sysc->signals),
> +                                    GFP_KERNEL);
> +       if (!sysc->signals)
> +               return -ENOMEM;
> +
> +       for (u32 i = 0; i < num_signals; i++) {

unsigned int

> +               struct rz_sysc_signal_init_data *id;
> +
> +               id = devm_kzalloc(sysc->dev, sizeof(*id), GFP_KERNEL);
> +               if (!id)
> +                       return -ENOMEM;
> +
> +               id->name = devm_kstrdup(sysc->dev, init_data->name, GFP_KERNEL);
> +               if (!id->name)
> +                       return -ENOMEM;
> +
> +               id->offset = init_data->offset;
> +               id->mask = init_data->mask;
> +               id->refcnt_incr_val = init_data->refcnt_incr_val;
> +
> +               sysc->signals[i].init_data = id;
> +               refcount_set(&sysc->signals[i].refcnt, 0);
> +       }
> +
> +       sysc->num_signals = num_signals;
> +
> +       root = debugfs_create_dir("renesas-rz-sysc", NULL);
> +       ret = devm_add_action_or_reset(sysc->dev, rz_sysc_debugfs_remove, root);
> +       if (ret)
> +               return ret;
> +       debugfs_create_file("signals", 0444, root, sysc, &rz_sysc_signals_fops);
> +
> +       return 0;
> +}

> --- /dev/null
> +++ b/drivers/soc/renesas/rz-sysc.h
> @@ -0,0 +1,52 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Renesas RZ System Controller
> + *
> + * Copyright (C) 2024 Renesas Electronics Corp.
> + */
> +
> +#ifndef __SOC_RENESAS_RZ_SYSC_H__
> +#define __SOC_RENESAS_RZ_SYSC_H__
> +
> +#include <linux/refcount.h>
> +#include <linux/types.h>
> +
> +/**
> + * struct rz_sysc_signal_init_data - RZ SYSC signals init data
> + * @name: signal name
> + * @offset: register offset controling this signal
> + * @mask: bitmask in register specific to this signal
> + * @refcnt_incr_val: increment refcnt when setting this value
> + */
> +struct rz_sysc_signal_init_data {
> +       const char *name;
> +       u32 offset;
> +       u32 mask;
> +       u32 refcnt_incr_val;
> +};
> +
> +/**
> + * struct rz_sysc_signal - RZ SYSC signals
> + * @init_data: signals initialization data
> + * @refcnt: reference counter
> + */
> +struct rz_sysc_signal {
> +       const struct rz_sysc_signal_init_data *init_data;

Can't you just embed struct rz_sysc_signal_init_data?
That way you could allocate the rz_sysc_signal and
rz_sysc_signal_init_data structures in a single allocation.

> +       refcount_t refcnt;
> +};

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Claudiu Beznea Nov. 29, 2024, 8:48 a.m. UTC | #5

Hi, Geert,

On 28.11.2024 17:24, Geert Uytterhoeven wrote:
> Hi Claudiu,
> 
> Thanks for your patch!
> 
> On Tue, Nov 26, 2024 at 10:21 AM Claudiu <[e-mail]> wrote:
>> From: Claudiu Beznea <[e-mail]>
>>
>> The RZ/G3S system controller (SYSC) has various registers that control
>> signals specific to individual IPs. IP drivers must control these signals
>> at different configuration phases.
>>
>> Add SYSC driver that allows individual SYSC consumers to control these
>> signals. The SYSC driver exports a syscon regmap enabling IP drivers to
>> use a specific SYSC offset and mask from the device tree, which can then be
>> accessed through regmap_update_bits().
>>
>> Currently, the SYSC driver provides control to the USB PWRRDY signal, which
>> is routed to the USB PHY. This signal needs to be managed before or after
>> powering the USB PHY off or on.
>>
>> Other SYSC signals candidates (as exposed in the the hardware manual of the
> 
> s/the the/the/
> 
>> RZ/G3S SoC) include:
>>
>> * PCIe:
>> - ALLOW_ENTER_L1 signal controlled through the SYS_PCIE_CFG register
>> - PCIE_RST_RSM_B signal controlled through the SYS_PCIE_RST_RSM_B
>>   register
>> - MODE_RXTERMINATION signal controlled through SYS_PCIE_PHY register
>>
>> * SPI:
>> - SEL_SPI_OCTA signal controlled through SYS_IPCONT_SEL_SPI_OCTA
>>   register
>>
>> * I2C/I3C:
>> - af_bypass I2C signals controlled through SYS_I2Cx_CFG registers
>>   (x=0..3)
>> - af_bypass I3C signal controlled through SYS_I3C_CFG register
>>
>> * Ethernet:
>> - FEC_GIGA_ENABLE Ethernet signals controlled through SYS_GETHx_CFG
>>   registers (x=0..1)
>>
>> As different Renesas RZ SoC shares most of the SYSC functionalities
>> available on the RZ/G3S SoC, the driver if formed of a SYSC core
>> part and a SoC specific part allowing individual SYSC SoC to provide
>> functionalities to the SYSC core.
>>
>> Signed-off-by: Claudiu Beznea <[e-mail]>
> 
>> --- /dev/null
>> +++ b/drivers/soc/renesas/r9a08g045-sysc.c
>> @@ -0,0 +1,31 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * RZ/G3S System controller driver
>> + *
>> + * Copyright (C) 2024 Renesas Electronics Corp.
>> + */
>> +
>> +#include <linux/array_size.h>
>> +#include <linux/bits.h>
>> +#include <linux/init.h>
>> +
>> +#include "rz-sysc.h"
>> +
>> +#define SYS_USB_PWRRDY         0xd70
>> +#define SYS_USB_PWRRDY_PWRRDY_N        BIT(0)
>> +#define SYS_MAX_REG            0xe20
>> +
>> +static const struct rz_sysc_signal_init_data rzg3s_sysc_signals_init_data[] __initconst = {
> 
> This is marked __initconst...
> 
>> +       {
>> +               .name = "usb-pwrrdy",
>> +               .offset = SYS_USB_PWRRDY,
>> +               .mask = SYS_USB_PWRRDY_PWRRDY_N,
>> +               .refcnt_incr_val = 0
>> +       }
>> +};
>> +
>> +const struct rz_sysc_init_data rzg3s_sysc_init_data = {
> 
> ... but this is not __init, causing a section mismatch.

Do you know if there is a way to detect this?

> 
>> +       .signals_init_data = rzg3s_sysc_signals_init_data,
>> +       .num_signals = ARRAY_SIZE(rzg3s_sysc_signals_init_data),
>> +       .max_register_offset = SYS_MAX_REG,
>> +};
> 
>> --- /dev/null
>> +++ b/drivers/soc/renesas/rz-sysc.c
> 
>> +/**
>> + * struct rz_sysc - RZ SYSC private data structure
>> + * @base: SYSC base address
>> + * @dev: SYSC device pointer
>> + * @signals: SYSC signals
>> + * @num_signals: number of SYSC signals
>> + */
>> +struct rz_sysc {
>> +       void __iomem *base;
>> +       struct device *dev;
>> +       struct rz_sysc_signal *signals;
>> +       u8 num_signals;
> 
> You could change signals to a flexible array at the end, tag it with
> __counted_by(num_signals), and allocate space for both struct rz_sysc
> and the signals array using struct_size(), reducing the number of
> allocations.

I'll look into this.

> 
>> +};
> 
>> +static struct rz_sysc_signal *rz_sysc_off_to_signal(struct rz_sysc *sysc, unsigned int offset,
>> +                                                   unsigned int mask)
>> +{
>> +       struct rz_sysc_signal *signals = sysc->signals;
>> +
>> +       for (u32 i = 0; i < sysc->num_signals; i++) {
> 
> s/u32/unsigned int/
> 
>> +               if (signals[i].init_data->offset != offset)
>> +                       continue;
>> +
>> +               /*
>> +                * In case mask == 0 we just return the signal data w/o checking the mask.
>> +                * This is useful when calling through rz_sysc_reg_write() to check
>> +                * if the requested setting is for a mapped signal or not.
>> +                */
>> +               if (mask) {
>> +                       if (signals[i].init_data->mask == mask)
>> +                               return &signals[i];
>> +               } else {
>> +                       return &signals[i];
>> +               }
> 
> if (!mask || signals[i].init_data->mask == mask)
>         return &signals[i];

Looks better, indeed!

> 
>> +       }
>> +
>> +       return NULL;
>> +}
>> +
>> +static int rz_sysc_reg_update_bits(void *context, unsigned int off,
>> +                                  unsigned int mask, unsigned int val)
>> +{
>> +       struct rz_sysc *sysc = context;
>> +       struct rz_sysc_signal *signal;
>> +       bool update = false;
>> +
>> +       signal = rz_sysc_off_to_signal(sysc, off, mask);
>> +       if (signal) {
>> +               if (signal->init_data->refcnt_incr_val == val) {
>> +                       if (!refcount_read(&signal->refcnt)) {
>> +                               refcount_set(&signal->refcnt, 1);
>> +                               update = true;
>> +                       } else {
>> +                               refcount_inc(&signal->refcnt);
>> +                       }
>> +               } else {
>> +                       update = refcount_dec_and_test(&signal->refcnt);
>> +               }
>> +       } else {
>> +               update = true;
>> +       }
> 
> You could reduce indentation/number of lines by reordering the logic:
> 
>     if (!signal) {
>             update = true;
>     } else if (signal->init_data->refcnt_incr_val != val) {
>             update = refcount_dec_and_test(&signal->refcnt);
>     } else if (!refcount_read(&signal->refcnt)) {
>             refcount_set(&signal->refcnt, 1);
>             update = true;
>     } else {
>             refcount_inc(&signal->refcnt);
>     }

Looks better, indeed!

> 
>> +
>> +       if (update) {
>> +               u32 tmp;
>> +
>> +               tmp = readl(sysc->base + off);
>> +               tmp &= ~mask;
>> +               tmp |= val & mask;
>> +               writel(tmp, sysc->base + off);
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static int rz_sysc_reg_write(void *context, unsigned int off, unsigned int val)
>> +{
>> +       struct rz_sysc *sysc = context;
>> +       struct rz_sysc_signal *signal;
>> +
>> +       /*
>> +        * Force using regmap_update_bits() for signals to have reference counter
>> +        * per individual signal in case there are multiple signals controlled
>> +        * through the same register.
>> +        */
>> +       signal = rz_sysc_off_to_signal(sysc, off, 0);
>> +       if (signal) {
>> +               dev_err(sysc->dev,
>> +                       "regmap_write() not allowed on register controlling a signal. Use regmap_update_bits()!");
>> +               return -EOPNOTSUPP;
>> +       }
>> +
> 
> Can you ever get here, given rz_sysc_writeable_reg() below would have
> returned false? 

Yes, in case the user wants to access a register having a signal associated.

If user does this:

regmap_write(regmap, signal_offset, value);

The rz_sysc_writeable_reg() returns true (as it checks only the signal
offset but not the mask) and the rz_sysc_reg_write() reach this point. If
we allow it to continue we can break reference counting for the existing
implemented signals, or, if there are multiple signals handled thorough the
same register, we can break other signals.

> If not, is there any point in having this function?

I chose to have it for (currently unexisting) other use cases of the syscon
+ regmap exported by this driver. For the moment, it is not necessary,
indeed. I can drop it if you prefer.

> 
>> +       writel(val, sysc->base + off);
>> +
>> +       return 0;
>> +}
>> +
>> +static bool rz_sysc_writeable_reg(struct device *dev, unsigned int off)
>> +{
>> +       struct rz_sysc *sysc = dev_get_drvdata(dev);
>> +       struct rz_sysc_signal *signal;
>> +
>> +       /* Any register containing a signal is writeable. */
>> +       signal = rz_sysc_off_to_signal(sysc, off, 0);
>> +       if (signal)
>> +               return true;
>> +
>> +       return false;
>> +}
> 
>> +static int rz_sysc_signals_show(struct seq_file *s, void *what)
>> +{
>> +       struct rz_sysc *sysc = s->private;
>> +
>> +       seq_printf(s, "%-20s Enable count\n", "Signal");
>> +       seq_printf(s, "%-20s ------------\n", "--------------------");
>> +
>> +       for (u8 i = 0; i < sysc->num_signals; i++) {
>> +               seq_printf(s, "%-20s %d\n", sysc->signals[i].init_data->name,
>> +                          refcount_read(&sysc->signals[i].refcnt));
>> +       }
>> +
>> +       return 0;
>> +}
>> +DEFINE_SHOW_ATTRIBUTE(rz_sysc_signals);
> 
> What is the use-case for this? Just (initial) debugging?

Only debugging.

> 
>> +
>> +static void rz_sysc_debugfs_remove(void *data)
>> +{
>> +       debugfs_remove_recursive(data);
>> +}
>> +
>> +static int rz_sysc_signals_init(struct rz_sysc *sysc,
>> +                               const struct rz_sysc_signal_init_data *init_data,
>> +                               u32 num_signals)
>> +{
>> +       struct dentry *root;
>> +       int ret;
>> +
>> +       sysc->signals = devm_kcalloc(sysc->dev, num_signals, sizeof(*sysc->signals),
>> +                                    GFP_KERNEL);
>> +       if (!sysc->signals)
>> +               return -ENOMEM;
>> +
>> +       for (u32 i = 0; i < num_signals; i++) {
> 
> unsigned int
> 
>> +               struct rz_sysc_signal_init_data *id;
>> +
>> +               id = devm_kzalloc(sysc->dev, sizeof(*id), GFP_KERNEL);
>> +               if (!id)
>> +                       return -ENOMEM;
>> +
>> +               id->name = devm_kstrdup(sysc->dev, init_data->name, GFP_KERNEL);
>> +               if (!id->name)
>> +                       return -ENOMEM;
>> +
>> +               id->offset = init_data->offset;
>> +               id->mask = init_data->mask;
>> +               id->refcnt_incr_val = init_data->refcnt_incr_val;
>> +
>> +               sysc->signals[i].init_data = id;
>> +               refcount_set(&sysc->signals[i].refcnt, 0);
>> +       }
>> +
>> +       sysc->num_signals = num_signals;
>> +
>> +       root = debugfs_create_dir("renesas-rz-sysc", NULL);
>> +       ret = devm_add_action_or_reset(sysc->dev, rz_sysc_debugfs_remove, root);
>> +       if (ret)
>> +               return ret;
>> +       debugfs_create_file("signals", 0444, root, sysc, &rz_sysc_signals_fops);
>> +
>> +       return 0;
>> +}
> 
>> --- /dev/null
>> +++ b/drivers/soc/renesas/rz-sysc.h
>> @@ -0,0 +1,52 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Renesas RZ System Controller
>> + *
>> + * Copyright (C) 2024 Renesas Electronics Corp.
>> + */
>> +
>> +#ifndef __SOC_RENESAS_RZ_SYSC_H__
>> +#define __SOC_RENESAS_RZ_SYSC_H__
>> +
>> +#include <linux/refcount.h>
>> +#include <linux/types.h>
>> +
>> +/**
>> + * struct rz_sysc_signal_init_data - RZ SYSC signals init data
>> + * @name: signal name
>> + * @offset: register offset controling this signal
>> + * @mask: bitmask in register specific to this signal
>> + * @refcnt_incr_val: increment refcnt when setting this value
>> + */
>> +struct rz_sysc_signal_init_data {
>> +       const char *name;
>> +       u32 offset;
>> +       u32 mask;
>> +       u32 refcnt_incr_val;
>> +};
>> +
>> +/**
>> + * struct rz_sysc_signal - RZ SYSC signals
>> + * @init_data: signals initialization data
>> + * @refcnt: reference counter
>> + */
>> +struct rz_sysc_signal {
>> +       const struct rz_sysc_signal_init_data *init_data;
> 
> Can't you just embed struct rz_sysc_signal_init_data?

Meaning to have directly the members of struct rz_sysc_signal_init_data
here or to drop the const qualifier along with __initconst on
rzg3s_sysc_signals_init_data[]  and re-use the platfom data w/o allocate
new memory?

Thank you for your review,
Claudiu


> That way you could allocate the rz_sysc_signal and
> rz_sysc_signal_init_data structures in a single allocation.
> 
>> +       refcount_t refcnt;
>> +};
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

Geert Uytterhoeven Nov. 29, 2024, 8:54 a.m. UTC | #6

Hi Claudiu,

On Fri, Nov 29, 2024 at 9:48 AM Claudiu Beznea <[e-mail]> wrote:
> On 28.11.2024 17:24, Geert Uytterhoeven wrote:
> > On Tue, Nov 26, 2024 at 10:21 AM Claudiu <[e-mail]> wrote:
> >> From: Claudiu Beznea <[e-mail]>
> >>
> >> The RZ/G3S system controller (SYSC) has various registers that control
> >> signals specific to individual IPs. IP drivers must control these signals
> >> at different configuration phases.
> >>
> >> Add SYSC driver that allows individual SYSC consumers to control these
> >> signals. The SYSC driver exports a syscon regmap enabling IP drivers to
> >> use a specific SYSC offset and mask from the device tree, which can then be
> >> accessed through regmap_update_bits().
> >>
> >> Currently, the SYSC driver provides control to the USB PWRRDY signal, which
> >> is routed to the USB PHY. This signal needs to be managed before or after
> >> powering the USB PHY off or on.
> >>
> >> Other SYSC signals candidates (as exposed in the the hardware manual of the
> >>
> >> * PCIe:
> >> - ALLOW_ENTER_L1 signal controlled through the SYS_PCIE_CFG register
> >> - PCIE_RST_RSM_B signal controlled through the SYS_PCIE_RST_RSM_B
> >>   register
> >> - MODE_RXTERMINATION signal controlled through SYS_PCIE_PHY register
> >>
> >> * SPI:
> >> - SEL_SPI_OCTA signal controlled through SYS_IPCONT_SEL_SPI_OCTA
> >>   register
> >>
> >> * I2C/I3C:
> >> - af_bypass I2C signals controlled through SYS_I2Cx_CFG registers
> >>   (x=0..3)
> >> - af_bypass I3C signal controlled through SYS_I3C_CFG register
> >>
> >> * Ethernet:
> >> - FEC_GIGA_ENABLE Ethernet signals controlled through SYS_GETHx_CFG
> >>   registers (x=0..1)
> >>
> >> As different Renesas RZ SoC shares most of the SYSC functionalities
> >> available on the RZ/G3S SoC, the driver if formed of a SYSC core
> >> part and a SoC specific part allowing individual SYSC SoC to provide
> >> functionalities to the SYSC core.
> >>
> >> Signed-off-by: Claudiu Beznea <[e-mail]>
> >
> >> --- /dev/null
> >> +++ b/drivers/soc/renesas/r9a08g045-sysc.c
> >> @@ -0,0 +1,31 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * RZ/G3S System controller driver
> >> + *
> >> + * Copyright (C) 2024 Renesas Electronics Corp.
> >> + */
> >> +
> >> +#include <linux/array_size.h>
> >> +#include <linux/bits.h>
> >> +#include <linux/init.h>
> >> +
> >> +#include "rz-sysc.h"
> >> +
> >> +#define SYS_USB_PWRRDY         0xd70
> >> +#define SYS_USB_PWRRDY_PWRRDY_N        BIT(0)
> >> +#define SYS_MAX_REG            0xe20
> >> +
> >> +static const struct rz_sysc_signal_init_data rzg3s_sysc_signals_init_data[] __initconst = {
> >
> > This is marked __initconst...
> >
> >> +       {
> >> +               .name = "usb-pwrrdy",
> >> +               .offset = SYS_USB_PWRRDY,
> >> +               .mask = SYS_USB_PWRRDY_PWRRDY_N,
> >> +               .refcnt_incr_val = 0
> >> +       }
> >> +};
> >> +
> >> +const struct rz_sysc_init_data rzg3s_sysc_init_data = {
> >
> > ... but this is not __init, causing a section mismatch.
>
> Do you know if there is a way to detect this?

The kernel should tell you during the build...

>
> >
> >> +       .signals_init_data = rzg3s_sysc_signals_init_data,
> >> +       .num_signals = ARRAY_SIZE(rzg3s_sysc_signals_init_data),
> >> +       .max_register_offset = SYS_MAX_REG,
> >> +};
> >
> >> --- /dev/null
> >> +++ b/drivers/soc/renesas/rz-sysc.c
> >
> >> +/**
> >> + * struct rz_sysc - RZ SYSC private data structure
> >> + * @base: SYSC base address
> >> + * @dev: SYSC device pointer
> >> + * @signals: SYSC signals
> >> + * @num_signals: number of SYSC signals
> >> + */
> >> +struct rz_sysc {
> >> +       void __iomem *base;
> >> +       struct device *dev;
> >> +       struct rz_sysc_signal *signals;
> >> +       u8 num_signals;
> >
> > You could change signals to a flexible array at the end, tag it with
> > __counted_by(num_signals), and allocate space for both struct rz_sysc
> > and the signals array using struct_size(), reducing the number of
> > allocations.
>
> I'll look into this.

> >> --- /dev/null
> >> +++ b/drivers/soc/renesas/rz-sysc.h
> >> @@ -0,0 +1,52 @@
> >> +/* SPDX-License-Identifier: GPL-2.0 */
> >> +/*
> >> + * Renesas RZ System Controller
> >> + *
> >> + * Copyright (C) 2024 Renesas Electronics Corp.
> >> + */
> >> +
> >> +#ifndef __SOC_RENESAS_RZ_SYSC_H__
> >> +#define __SOC_RENESAS_RZ_SYSC_H__
> >> +
> >> +#include <linux/refcount.h>
> >> +#include <linux/types.h>
> >> +
> >> +/**
> >> + * struct rz_sysc_signal_init_data - RZ SYSC signals init data
> >> + * @name: signal name
> >> + * @offset: register offset controling this signal
> >> + * @mask: bitmask in register specific to this signal
> >> + * @refcnt_incr_val: increment refcnt when setting this value
> >> + */
> >> +struct rz_sysc_signal_init_data {
> >> +       const char *name;
> >> +       u32 offset;
> >> +       u32 mask;
> >> +       u32 refcnt_incr_val;
> >> +};
> >> +
> >> +/**
> >> + * struct rz_sysc_signal - RZ SYSC signals
> >> + * @init_data: signals initialization data
> >> + * @refcnt: reference counter
> >> + */
> >> +struct rz_sysc_signal {
> >> +       const struct rz_sysc_signal_init_data *init_data;
> >
> > Can't you just embed struct rz_sysc_signal_init_data?
>
> Meaning to have directly the members of struct rz_sysc_signal_init_data
> here or to drop the const qualifier along with __initconst on
> rzg3s_sysc_signals_init_data[]  and re-use the platfom data w/o allocate
> new memory?

I mean

    struct rz_sysc_signal {
          struct rz_sysc_signal_init_data init_data;
          ...
    };

Currently you allocate rz_sysc_signal_init_data separately.
When embedded, it will be part of rz_sysc, cfr. above.

> > That way you could allocate the rz_sysc_signal and
> > rz_sysc_signal_init_data structures in a single allocation.

Gr{oetje,eeting}s,

                        Geert

Claudiu Beznea Nov. 29, 2024, 9:12 a.m. UTC | #7

On 29.11.2024 10:54, Geert Uytterhoeven wrote:
> Hi Claudiu,
> 
> On Fri, Nov 29, 2024 at 9:48 AM Claudiu Beznea <[e-mail]> wrote:
>> On 28.11.2024 17:24, Geert Uytterhoeven wrote:
>>> On Tue, Nov 26, 2024 at 10:21 AM Claudiu <[e-mail]> wrote:
>>>> From: Claudiu Beznea <[e-mail]>
>>>>
>>>> The RZ/G3S system controller (SYSC) has various registers that control
>>>> signals specific to individual IPs. IP drivers must control these signals
>>>> at different configuration phases.
>>>>
>>>> Add SYSC driver that allows individual SYSC consumers to control these
>>>> signals. The SYSC driver exports a syscon regmap enabling IP drivers to
>>>> use a specific SYSC offset and mask from the device tree, which can then be
>>>> accessed through regmap_update_bits().
>>>>
>>>> Currently, the SYSC driver provides control to the USB PWRRDY signal, which
>>>> is routed to the USB PHY. This signal needs to be managed before or after
>>>> powering the USB PHY off or on.
>>>>
>>>> Other SYSC signals candidates (as exposed in the the hardware manual of the
>>>>
>>>> * PCIe:
>>>> - ALLOW_ENTER_L1 signal controlled through the SYS_PCIE_CFG register
>>>> - PCIE_RST_RSM_B signal controlled through the SYS_PCIE_RST_RSM_B
>>>>   register
>>>> - MODE_RXTERMINATION signal controlled through SYS_PCIE_PHY register
>>>>
>>>> * SPI:
>>>> - SEL_SPI_OCTA signal controlled through SYS_IPCONT_SEL_SPI_OCTA
>>>>   register
>>>>
>>>> * I2C/I3C:
>>>> - af_bypass I2C signals controlled through SYS_I2Cx_CFG registers
>>>>   (x=0..3)
>>>> - af_bypass I3C signal controlled through SYS_I3C_CFG register
>>>>
>>>> * Ethernet:
>>>> - FEC_GIGA_ENABLE Ethernet signals controlled through SYS_GETHx_CFG
>>>>   registers (x=0..1)
>>>>
>>>> As different Renesas RZ SoC shares most of the SYSC functionalities
>>>> available on the RZ/G3S SoC, the driver if formed of a SYSC core
>>>> part and a SoC specific part allowing individual SYSC SoC to provide
>>>> functionalities to the SYSC core.
>>>>
>>>> Signed-off-by: Claudiu Beznea <[e-mail]>
>>>
>>>> --- /dev/null
>>>> +++ b/drivers/soc/renesas/r9a08g045-sysc.c
>>>> @@ -0,0 +1,31 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +/*
>>>> + * RZ/G3S System controller driver
>>>> + *
>>>> + * Copyright (C) 2024 Renesas Electronics Corp.
>>>> + */
>>>> +
>>>> +#include <linux/array_size.h>
>>>> +#include <linux/bits.h>
>>>> +#include <linux/init.h>
>>>> +
>>>> +#include "rz-sysc.h"
>>>> +
>>>> +#define SYS_USB_PWRRDY         0xd70
>>>> +#define SYS_USB_PWRRDY_PWRRDY_N        BIT(0)
>>>> +#define SYS_MAX_REG            0xe20
>>>> +
>>>> +static const struct rz_sysc_signal_init_data rzg3s_sysc_signals_init_data[] __initconst = {
>>>
>>> This is marked __initconst...
>>>
>>>> +       {
>>>> +               .name = "usb-pwrrdy",
>>>> +               .offset = SYS_USB_PWRRDY,
>>>> +               .mask = SYS_USB_PWRRDY_PWRRDY_N,
>>>> +               .refcnt_incr_val = 0
>>>> +       }
>>>> +};
>>>> +
>>>> +const struct rz_sysc_init_data rzg3s_sysc_init_data = {
>>>
>>> ... but this is not __init, causing a section mismatch.
>>
>> Do you know if there is a way to detect this?
> 
> The kernel should tell you during the build...

I'll look carefully, I haven't noticed it. Thank you!

> 
>>
>>>
>>>> +       .signals_init_data = rzg3s_sysc_signals_init_data,
>>>> +       .num_signals = ARRAY_SIZE(rzg3s_sysc_signals_init_data),
>>>> +       .max_register_offset = SYS_MAX_REG,
>>>> +};
>>>
>>>> --- /dev/null
>>>> +++ b/drivers/soc/renesas/rz-sysc.c
>>>
>>>> +/**
>>>> + * struct rz_sysc - RZ SYSC private data structure
>>>> + * @base: SYSC base address
>>>> + * @dev: SYSC device pointer
>>>> + * @signals: SYSC signals
>>>> + * @num_signals: number of SYSC signals
>>>> + */
>>>> +struct rz_sysc {
>>>> +       void __iomem *base;
>>>> +       struct device *dev;
>>>> +       struct rz_sysc_signal *signals;
>>>> +       u8 num_signals;
>>>
>>> You could change signals to a flexible array at the end, tag it with
>>> __counted_by(num_signals), and allocate space for both struct rz_sysc
>>> and the signals array using struct_size(), reducing the number of
>>> allocations.
>>
>> I'll look into this.
> 
>>>> --- /dev/null
>>>> +++ b/drivers/soc/renesas/rz-sysc.h
>>>> @@ -0,0 +1,52 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>>> +/*
>>>> + * Renesas RZ System Controller
>>>> + *
>>>> + * Copyright (C) 2024 Renesas Electronics Corp.
>>>> + */
>>>> +
>>>> +#ifndef __SOC_RENESAS_RZ_SYSC_H__
>>>> +#define __SOC_RENESAS_RZ_SYSC_H__
>>>> +
>>>> +#include <linux/refcount.h>
>>>> +#include <linux/types.h>
>>>> +
>>>> +/**
>>>> + * struct rz_sysc_signal_init_data - RZ SYSC signals init data
>>>> + * @name: signal name
>>>> + * @offset: register offset controling this signal
>>>> + * @mask: bitmask in register specific to this signal
>>>> + * @refcnt_incr_val: increment refcnt when setting this value
>>>> + */
>>>> +struct rz_sysc_signal_init_data {
>>>> +       const char *name;
>>>> +       u32 offset;
>>>> +       u32 mask;
>>>> +       u32 refcnt_incr_val;
>>>> +};
>>>> +
>>>> +/**
>>>> + * struct rz_sysc_signal - RZ SYSC signals
>>>> + * @init_data: signals initialization data
>>>> + * @refcnt: reference counter
>>>> + */
>>>> +struct rz_sysc_signal {
>>>> +       const struct rz_sysc_signal_init_data *init_data;
>>>
>>> Can't you just embed struct rz_sysc_signal_init_data?
>>
>> Meaning to have directly the members of struct rz_sysc_signal_init_data
>> here or to drop the const qualifier along with __initconst on
>> rzg3s_sysc_signals_init_data[]  and re-use the platfom data w/o allocate
>> new memory?
> 
> I mean
> 
>     struct rz_sysc_signal {
>           struct rz_sysc_signal_init_data init_data;
>           ...
>     };
> 
> Currently you allocate rz_sysc_signal_init_data separately.
> When embedded, it will be part of rz_sysc, cfr. above.

Ah, your right. I initially had this as a pointer and re-used the init data
(rzg3s_sysc_signals_init_data[], w/o having __initconst qualifier for it).
I dropped that approach but missed to drop the pointer here.

Thank you,
Claudiu

> 
>>> That way you could allocate the rz_sysc_signal and
>>> rz_sysc_signal_init_data structures in a single allocation.
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
>

Geert Uytterhoeven Nov. 29, 2024, 10:23 a.m. UTC | #8

On Fri, Nov 29, 2024 at 9:54 AM Geert Uytterhoeven <[e-mail]> wrote:
> On Fri, Nov 29, 2024 at 9:48 AM Claudiu Beznea <[e-mail]> wrote:
> > On 28.11.2024 17:24, Geert Uytterhoeven wrote:
> > > On Tue, Nov 26, 2024 at 10:21 AM Claudiu <[e-mail]> wrote:
> > >> From: Claudiu Beznea <[e-mail]>
> > >>
> > >> The RZ/G3S system controller (SYSC) has various registers that control
> > >> signals specific to individual IPs. IP drivers must control these signals
> > >> at different configuration phases.
> > >>
> > >> Add SYSC driver that allows individual SYSC consumers to control these
> > >> signals. The SYSC driver exports a syscon regmap enabling IP drivers to
> > >> use a specific SYSC offset and mask from the device tree, which can then be
> > >> accessed through regmap_update_bits().
> > >>
> > >> Currently, the SYSC driver provides control to the USB PWRRDY signal, which
> > >> is routed to the USB PHY. This signal needs to be managed before or after
> > >> powering the USB PHY off or on.
> > >>
> > >> Other SYSC signals candidates (as exposed in the the hardware manual of the
> > >>
> > >> * PCIe:
> > >> - ALLOW_ENTER_L1 signal controlled through the SYS_PCIE_CFG register
> > >> - PCIE_RST_RSM_B signal controlled through the SYS_PCIE_RST_RSM_B
> > >>   register
> > >> - MODE_RXTERMINATION signal controlled through SYS_PCIE_PHY register
> > >>
> > >> * SPI:
> > >> - SEL_SPI_OCTA signal controlled through SYS_IPCONT_SEL_SPI_OCTA
> > >>   register
> > >>
> > >> * I2C/I3C:
> > >> - af_bypass I2C signals controlled through SYS_I2Cx_CFG registers
> > >>   (x=0..3)
> > >> - af_bypass I3C signal controlled through SYS_I3C_CFG register
> > >>
> > >> * Ethernet:
> > >> - FEC_GIGA_ENABLE Ethernet signals controlled through SYS_GETHx_CFG
> > >>   registers (x=0..1)
> > >>
> > >> As different Renesas RZ SoC shares most of the SYSC functionalities
> > >> available on the RZ/G3S SoC, the driver if formed of a SYSC core
> > >> part and a SoC specific part allowing individual SYSC SoC to provide
> > >> functionalities to the SYSC core.
> > >>
> > >> Signed-off-by: Claudiu Beznea <[e-mail]>
> > >
> > >> --- /dev/null
> > >> +++ b/drivers/soc/renesas/r9a08g045-sysc.c
> > >> @@ -0,0 +1,31 @@
> > >> +// SPDX-License-Identifier: GPL-2.0
> > >> +/*
> > >> + * RZ/G3S System controller driver
> > >> + *
> > >> + * Copyright (C) 2024 Renesas Electronics Corp.
> > >> + */
> > >> +
> > >> +#include <linux/array_size.h>
> > >> +#include <linux/bits.h>
> > >> +#include <linux/init.h>
> > >> +
> > >> +#include "rz-sysc.h"
> > >> +
> > >> +#define SYS_USB_PWRRDY         0xd70
> > >> +#define SYS_USB_PWRRDY_PWRRDY_N        BIT(0)
> > >> +#define SYS_MAX_REG            0xe20
> > >> +
> > >> +static const struct rz_sysc_signal_init_data rzg3s_sysc_signals_init_data[] __initconst = {
> > >
> > > This is marked __initconst...
> > >
> > >> +       {
> > >> +               .name = "usb-pwrrdy",
> > >> +               .offset = SYS_USB_PWRRDY,
> > >> +               .mask = SYS_USB_PWRRDY_PWRRDY_N,
> > >> +               .refcnt_incr_val = 0
> > >> +       }
> > >> +};
> > >> +
> > >> +const struct rz_sysc_init_data rzg3s_sysc_init_data = {
> > >
> > > ... but this is not __init, causing a section mismatch.
> >
> > Do you know if there is a way to detect this?
>
> The kernel should tell you during the build...

Sorry, I hit send too early; I was still verifying this...
And it indeed doesn't trigger, strange...

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Patch

diff --git a/drivers/soc/renesas/Kconfig b/drivers/soc/renesas/Kconfig
index 9f7fe02310b9..0686c3ad9e27 100644
--- a/drivers/soc/renesas/Kconfig
+++ b/drivers/soc/renesas/Kconfig
@@ -378,4 +378,11 @@  config PWC_RZV2M
 config RST_RCAR
 	bool "Reset Controller support for R-Car" if COMPILE_TEST
 
+config SYSC_RZ
+	bool "System controller for RZ SoCs" if COMPILE_TEST
+
+config SYSC_R9A08G045
+	bool "Renesas RZ/G3S System controller support" if COMPILE_TEST
+	select SYSC_RZ
+
 endif # SOC_RENESAS
diff --git a/drivers/soc/renesas/Makefile b/drivers/soc/renesas/Makefile
index 734f8f8cefa4..8cd139b3dd0a 100644
--- a/drivers/soc/renesas/Makefile
+++ b/drivers/soc/renesas/Makefile
@@ -6,7 +6,9 @@  obj-$(CONFIG_SOC_RENESAS)	+= renesas-soc.o
 ifdef CONFIG_SMP
 obj-$(CONFIG_ARCH_R9A06G032)	+= r9a06g032-smp.o
 endif
+obj-$(CONFIG_SYSC_R9A08G045)	+= r9a08g045-sysc.o
 
 # Family
 obj-$(CONFIG_PWC_RZV2M)		+= pwc-rzv2m.o
 obj-$(CONFIG_RST_RCAR)		+= rcar-rst.o
+obj-$(CONFIG_SYSC_RZ)		+= rz-sysc.o
diff --git a/drivers/soc/renesas/r9a08g045-sysc.c b/drivers/soc/renesas/r9a08g045-sysc.c
new file mode 100644
index 000000000000..ceea738aee72
--- /dev/null
+++ b/drivers/soc/renesas/r9a08g045-sysc.c
@@ -0,0 +1,31 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * RZ/G3S System controller driver
+ *
+ * Copyright (C) 2024 Renesas Electronics Corp.
+ */
+
+#include <linux/array_size.h>
+#include <linux/bits.h>
+#include <linux/init.h>
+
+#include "rz-sysc.h"
+
+#define SYS_USB_PWRRDY		0xd70
+#define SYS_USB_PWRRDY_PWRRDY_N	BIT(0)
+#define SYS_MAX_REG		0xe20
+
+static const struct rz_sysc_signal_init_data rzg3s_sysc_signals_init_data[] __initconst = {
+	{
+		.name = "usb-pwrrdy",
+		.offset = SYS_USB_PWRRDY,
+		.mask = SYS_USB_PWRRDY_PWRRDY_N,
+		.refcnt_incr_val = 0
+	}
+};
+
+const struct rz_sysc_init_data rzg3s_sysc_init_data = {
+	.signals_init_data = rzg3s_sysc_signals_init_data,
+	.num_signals = ARRAY_SIZE(rzg3s_sysc_signals_init_data),
+	.max_register_offset = SYS_MAX_REG,
+};
diff --git a/drivers/soc/renesas/rz-sysc.c b/drivers/soc/renesas/rz-sysc.c
new file mode 100644
index 000000000000..dc0edacd7170
--- /dev/null
+++ b/drivers/soc/renesas/rz-sysc.c
@@ -0,0 +1,286 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * RZ System controller driver
+ *
+ * Copyright (C) 2024 Renesas Electronics Corp.
+ */
+
+#include <linux/dcache.h>
+#include <linux/debugfs.h>
+#include <linux/io.h>
+#include <linux/mfd/syscon.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/refcount.h>
+#include <linux/regmap.h>
+#include <linux/seq_file.h>
+
+#include "rz-sysc.h"
+
+/**
+ * struct rz_sysc - RZ SYSC private data structure
+ * @base: SYSC base address
+ * @dev: SYSC device pointer
+ * @signals: SYSC signals
+ * @num_signals: number of SYSC signals
+ */
+struct rz_sysc {
+	void __iomem *base;
+	struct device *dev;
+	struct rz_sysc_signal *signals;
+	u8 num_signals;
+};
+
+static int rz_sysc_reg_read(void *context, unsigned int off, unsigned int *val)
+{
+	struct rz_sysc *sysc = context;
+
+	*val = readl(sysc->base + off);
+
+	return 0;
+}
+
+static struct rz_sysc_signal *rz_sysc_off_to_signal(struct rz_sysc *sysc, unsigned int offset,
+						    unsigned int mask)
+{
+	struct rz_sysc_signal *signals = sysc->signals;
+
+	for (u32 i = 0; i < sysc->num_signals; i++) {
+		if (signals[i].init_data->offset != offset)
+			continue;
+
+		/*
+		 * In case mask == 0 we just return the signal data w/o checking the mask.
+		 * This is useful when calling through rz_sysc_reg_write() to check
+		 * if the requested setting is for a mapped signal or not.
+		 */
+		if (mask) {
+			if (signals[i].init_data->mask == mask)
+				return &signals[i];
+		} else {
+			return &signals[i];
+		}
+	}
+
+	return NULL;
+}
+
+static int rz_sysc_reg_update_bits(void *context, unsigned int off,
+				   unsigned int mask, unsigned int val)
+{
+	struct rz_sysc *sysc = context;
+	struct rz_sysc_signal *signal;
+	bool update = false;
+
+	signal = rz_sysc_off_to_signal(sysc, off, mask);
+	if (signal) {
+		if (signal->init_data->refcnt_incr_val == val) {
+			if (!refcount_read(&signal->refcnt)) {
+				refcount_set(&signal->refcnt, 1);
+				update = true;
+			} else {
+				refcount_inc(&signal->refcnt);
+			}
+		} else {
+			update = refcount_dec_and_test(&signal->refcnt);
+		}
+	} else {
+		update = true;
+	}
+
+	if (update) {
+		u32 tmp;
+
+		tmp = readl(sysc->base + off);
+		tmp &= ~mask;
+		tmp |= val & mask;
+		writel(tmp, sysc->base + off);
+	}
+
+	return 0;
+}
+
+static int rz_sysc_reg_write(void *context, unsigned int off, unsigned int val)
+{
+	struct rz_sysc *sysc = context;
+	struct rz_sysc_signal *signal;
+
+	/*
+	 * Force using regmap_update_bits() for signals to have reference counter
+	 * per individual signal in case there are multiple signals controlled
+	 * through the same register.
+	 */
+	signal = rz_sysc_off_to_signal(sysc, off, 0);
+	if (signal) {
+		dev_err(sysc->dev,
+			"regmap_write() not allowed on register controlling a signal. Use regmap_update_bits()!");
+		return -EOPNOTSUPP;
+	}
+
+	writel(val, sysc->base + off);
+
+	return 0;
+}
+
+static bool rz_sysc_writeable_reg(struct device *dev, unsigned int off)
+{
+	struct rz_sysc *sysc = dev_get_drvdata(dev);
+	struct rz_sysc_signal *signal;
+
+	/* Any register containing a signal is writeable. */
+	signal = rz_sysc_off_to_signal(sysc, off, 0);
+	if (signal)
+		return true;
+
+	return false;
+}
+
+static bool rz_sysc_readable_reg(struct device *dev, unsigned int off)
+{
+	struct rz_sysc *sysc = dev_get_drvdata(dev);
+	struct rz_sysc_signal *signal;
+
+	/* Any register containing a signal is readable. */
+	signal = rz_sysc_off_to_signal(sysc, off, 0);
+	if (signal)
+		return true;
+
+	return false;
+}
+
+static int rz_sysc_signals_show(struct seq_file *s, void *what)
+{
+	struct rz_sysc *sysc = s->private;
+
+	seq_printf(s, "%-20s Enable count\n", "Signal");
+	seq_printf(s, "%-20s ------------\n", "--------------------");
+
+	for (u8 i = 0; i < sysc->num_signals; i++) {
+		seq_printf(s, "%-20s %d\n", sysc->signals[i].init_data->name,
+			   refcount_read(&sysc->signals[i].refcnt));
+	}
+
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(rz_sysc_signals);
+
+static void rz_sysc_debugfs_remove(void *data)
+{
+	debugfs_remove_recursive(data);
+}
+
+static int rz_sysc_signals_init(struct rz_sysc *sysc,
+				const struct rz_sysc_signal_init_data *init_data,
+				u32 num_signals)
+{
+	struct dentry *root;
+	int ret;
+
+	sysc->signals = devm_kcalloc(sysc->dev, num_signals, sizeof(*sysc->signals),
+				     GFP_KERNEL);
+	if (!sysc->signals)
+		return -ENOMEM;
+
+	for (u32 i = 0; i < num_signals; i++) {
+		struct rz_sysc_signal_init_data *id;
+
+		id = devm_kzalloc(sysc->dev, sizeof(*id), GFP_KERNEL);
+		if (!id)
+			return -ENOMEM;
+
+		id->name = devm_kstrdup(sysc->dev, init_data->name, GFP_KERNEL);
+		if (!id->name)
+			return -ENOMEM;
+
+		id->offset = init_data->offset;
+		id->mask = init_data->mask;
+		id->refcnt_incr_val = init_data->refcnt_incr_val;
+
+		sysc->signals[i].init_data = id;
+		refcount_set(&sysc->signals[i].refcnt, 0);
+	}
+
+	sysc->num_signals = num_signals;
+
+	root = debugfs_create_dir("renesas-rz-sysc", NULL);
+	ret = devm_add_action_or_reset(sysc->dev, rz_sysc_debugfs_remove, root);
+	if (ret)
+		return ret;
+	debugfs_create_file("signals", 0444, root, sysc, &rz_sysc_signals_fops);
+
+	return 0;
+}
+
+static struct regmap_config rz_sysc_regmap = {
+	.name = "rz_sysc_regs",
+	.reg_bits = 32,
+	.reg_stride = 4,
+	.val_bits = 32,
+	.fast_io = true,
+	.reg_read = rz_sysc_reg_read,
+	.reg_write = rz_sysc_reg_write,
+	.reg_update_bits = rz_sysc_reg_update_bits,
+	.writeable_reg = rz_sysc_writeable_reg,
+	.readable_reg = rz_sysc_readable_reg,
+};
+
+static const struct of_device_id rz_sysc_match[] = {
+#ifdef CONFIG_SYSC_R9A08G045
+	{ .compatible = "renesas,r9a08g045-sysc", .data = &rzg3s_sysc_init_data },
+#endif
+	{ }
+};
+MODULE_DEVICE_TABLE(of, rz_sysc_match);
+
+static int rz_sysc_probe(struct platform_device *pdev)
+{
+	const struct rz_sysc_init_data *data;
+	struct device *dev = &pdev->dev;
+	struct rz_sysc *sysc;
+	struct regmap *regmap;
+	int ret;
+
+	data = device_get_match_data(dev);
+	if (!data || !data->max_register_offset)
+		return -EINVAL;
+
+	sysc = devm_kzalloc(dev, sizeof(*sysc), GFP_KERNEL);
+	if (!sysc)
+		return -ENOMEM;
+
+	sysc->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(sysc->base))
+		return PTR_ERR(sysc->base);
+
+	sysc->dev = dev;
+
+	ret = rz_sysc_signals_init(sysc, data->signals_init_data, data->num_signals);
+	if (ret)
+		return ret;
+
+	dev_set_drvdata(dev, sysc);
+	rz_sysc_regmap.max_register = data->max_register_offset;
+	regmap = devm_regmap_init(dev, NULL, sysc, &rz_sysc_regmap);
+	if (IS_ERR(regmap))
+		return PTR_ERR(regmap);
+
+	return of_syscon_register_regmap(dev->of_node, regmap);
+}
+
+static struct platform_driver rz_sysc_driver = {
+	.driver = {
+		.name = "renesas-rz-sysc",
+		.of_match_table = rz_sysc_match
+	},
+	.probe = rz_sysc_probe
+};
+
+static int __init rz_sysc_init(void)
+{
+	return platform_driver_register(&rz_sysc_driver);
+}
+subsys_initcall(rz_sysc_init);
+
+MODULE_DESCRIPTION("Renesas RZ System Controller Driver");
+MODULE_AUTHOR("Claudiu Beznea <[e-mail]>");
+MODULE_LICENSE("GPL");
diff --git a/drivers/soc/renesas/rz-sysc.h b/drivers/soc/renesas/rz-sysc.h
new file mode 100644
index 000000000000..bb850310c931
--- /dev/null
+++ b/drivers/soc/renesas/rz-sysc.h
@@ -0,0 +1,52 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Renesas RZ System Controller
+ *
+ * Copyright (C) 2024 Renesas Electronics Corp.
+ */
+
+#ifndef __SOC_RENESAS_RZ_SYSC_H__
+#define __SOC_RENESAS_RZ_SYSC_H__
+
+#include <linux/refcount.h>
+#include <linux/types.h>
+
+/**
+ * struct rz_sysc_signal_init_data - RZ SYSC signals init data
+ * @name: signal name
+ * @offset: register offset controling this signal
+ * @mask: bitmask in register specific to this signal
+ * @refcnt_incr_val: increment refcnt when setting this value
+ */
+struct rz_sysc_signal_init_data {
+	const char *name;
+	u32 offset;
+	u32 mask;
+	u32 refcnt_incr_val;
+};
+
+/**
+ * struct rz_sysc_signal - RZ SYSC signals
+ * @init_data: signals initialization data
+ * @refcnt: reference counter
+ */
+struct rz_sysc_signal {
+	const struct rz_sysc_signal_init_data *init_data;
+	refcount_t refcnt;
+};
+
+/**
+ * struct rz_sysc_init_data - RZ SYSC initialization data
+ * @signals_init_data: RZ SYSC signals initialization data
+ * @num_signals: number of SYSC signals
+ * @max_register_offset: Maximum SYSC register offset to be used by the regmap config
+ */
+struct rz_sysc_init_data {
+	const struct rz_sysc_signal_init_data *signals_init_data;
+	u32 num_signals;
+	u32 max_register_offset;
+};
+
+extern const struct rz_sysc_init_data rzg3s_sysc_init_data;
+
+#endif /* __SOC_RENESAS_RZ_SYSC_H__ */