Hi Claudiu,
Thanks for your patch!
On Tue, Nov 26, 2024 at 10:21 AM Claudiu <claudiu.beznea@xxxxxxxxx> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
>
> 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 <claudiu.beznea.uj@xxxxxxxxxxxxxx>
> --- /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 -- geert@xxxxxxxxxxxxxx
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
