linux-kernel - Re: [PATCH v2 02/15] soc: renesas: Add SYSC driver for Renesas RZ family

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAMuHMdXPQnCPjKRxoSceYabWPHF9Z_A7qVN85yaUZjPG7-o7tg@mail.gmail.com>
Date: Fri, 29 Nov 2024 09:54:14 +0100
From: Geert Uytterhoeven <geert@...ux-m68k.org>
To: Claudiu Beznea <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,

On Fri, Nov 29, 2024 at 9:48 AM Claudiu Beznea <claudiu.beznea@...on.dev> wrote:
> On 28.11.2024 17:24, Geert Uytterhoeven wrote:
> > 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
> >>
> >> * 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.
>
> 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

-- 
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