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

[Claudiu Beznea <claudiu.beznea@tuxon.dev>]

On 29.11.2024 10:54, Geert Uytterhoeven wrote:
> Hi Claudiu,
> 
> On Fri, Nov 29, 2024 at 9:48 AM Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
>> On 28.11.2024 17:24, Geert Uytterhoeven wrote:
>>> On Tue, Nov 26, 2024 at 10:21 AM Claudiu <claudiu.beznea@tuxon.dev> wrote:
>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.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@bp.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...

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
>