Message-ID: <CAMuHMdUvmTQeQXxhsXtj23-OS=aL3UgsyOtnawdmnusrEJ2JQw@mail.gmail.com>
Date: Thu, 28 Nov 2024 16:24:26 +0100
From: Geert Uytterhoeven <geert@...ux-m68k.org>
To: Claudiu <claudiu.beznea@...on.dev>
Cc: vkoul@...nel.org, kishon@...nel.org, robh@...nel.org, krzk+dt@...nel.org, 
	conor+dt@...nel.org, p.zabel@...gutronix.de, magnus.damm@...il.com, 
	gregkh@...uxfoundation.org, yoshihiro.shimoda.uh@...esas.com, 
	christophe.jaillet@...adoo.fr, linux-phy@...ts.infradead.org, 
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-renesas-soc@...r.kernel.org, linux-usb@...r.kernel.org, 
	Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
Subject: Re: [PATCH v2 02/15] soc: renesas: Add SYSC driver for Renesas RZ family

Hi Claudiu,

Thanks for your patch!

On Tue, Nov 26, 2024 at 10:21 AM Claudiu <claudiu.beznea@...on.dev> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
>
> 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@...renesas.com>

> --- /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@...ux-m68k.org

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

Powered by blists - more mailing lists