RE: [PATCH v2 2/9] clk: realtek: Add basic reset support

["Yu-Chun Lin [林祐君]" <eleanor.lin@realtek.com>]

Hi, Philipp,
Thanks for your review!

> On Di, 2026-01-13 at 19:23 +0800, Yu-Chun Lin wrote:
> > From: Cheng-Yu Lee <cylee12@realtek.com>
> >
> > Define the reset operations backed by a regmap-based register
> > interface and prepare the reset controller to be registered through
> > the reset framework.
> >
> > Signed-off-by: Cheng-Yu Lee <cylee12@realtek.com>
> > Co-developed-by: Yu-Chun Lin <eleanor.lin@realtek.com>
> > Signed-off-by: Yu-Chun Lin <eleanor.lin@realtek.com>
> > ---
> > Changes in v2:
> > - Added missing Co-developed-by tag.
> > - Added missing maintainer entry for driver/clk.
> > - Removed explicit of_xlate/of_reset_n_cells assignment as it matches
> defaults.
> > - Added error handling for rtk_reset_read() return value
> > ---
> >  MAINTAINERS                  |   1 +
> >  drivers/clk/Kconfig          |   1 +
> >  drivers/clk/Makefile         |   1 +
> >  drivers/clk/realtek/Kconfig  |  28 ++++++++
> >  drivers/clk/realtek/Makefile |   4 ++
> >  drivers/clk/realtek/reset.c  | 125
> > +++++++++++++++++++++++++++++++++++
> >  drivers/clk/realtek/reset.h  |  36 ++++++++++
> >  7 files changed, 196 insertions(+)
> >  create mode 100644 drivers/clk/realtek/Kconfig  create mode 100644
> > drivers/clk/realtek/Makefile  create mode 100644
> > drivers/clk/realtek/reset.c  create mode 100644
> > drivers/clk/realtek/reset.h
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS index
> > 66c0f4924c1e..de772e0026de 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -21975,6 +21975,7 @@ L:    devicetree@vger.kernel.org
> >  L:   linux-clk@vger.kernel.org
> >  S:   Supported
> >  F:   Documentation/devicetree/bindings/clock/realtek*
> > +F:   drivers/clk/realtek/*
> >  F:   include/dt-bindings/clock/realtek*
> >
> >  REALTEK SPI-NAND
> > diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig index
> > 3a1611008e48..2f2cacf87c38 100644
> > --- a/drivers/clk/Kconfig
> > +++ b/drivers/clk/Kconfig
> > @@ -530,6 +530,7 @@ source "drivers/clk/nuvoton/Kconfig"
> >  source "drivers/clk/pistachio/Kconfig"
> >  source "drivers/clk/qcom/Kconfig"
> >  source "drivers/clk/ralink/Kconfig"
> > +source "drivers/clk/realtek/Kconfig"
> >  source "drivers/clk/renesas/Kconfig"
> >  source "drivers/clk/rockchip/Kconfig"
> >  source "drivers/clk/samsung/Kconfig"
> > diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile index
> > 61ec08404442..075a1c410b90 100644
> > --- a/drivers/clk/Makefile
> > +++ b/drivers/clk/Makefile
> > @@ -141,6 +141,7 @@ obj-$(CONFIG_COMMON_CLK_PISTACHIO)
> += pistachio/
> >  obj-$(CONFIG_COMMON_CLK_PXA)         += pxa/
> >  obj-$(CONFIG_COMMON_CLK_QCOM)                += qcom/
> >  obj-y                                        += ralink/
> > +obj-$(CONFIG_COMMON_CLK_REALTEK)     += realtek/
> >  obj-y                                        += renesas/
> >  obj-$(CONFIG_ARCH_ROCKCHIP)          += rockchip/
> >  obj-$(CONFIG_COMMON_CLK_SAMSUNG)     += samsung/
> > diff --git a/drivers/clk/realtek/Kconfig b/drivers/clk/realtek/Kconfig
> > new file mode 100644 index 000000000000..121158f11dd1
> > --- /dev/null
> > +++ b/drivers/clk/realtek/Kconfig
> > @@ -0,0 +1,28 @@
> > +# SPDX-License-Identifier: GPL-2.0-only config COMMON_CLK_REALTEK
> > +     bool "Clock driver for Realtek SoCs"
> > +     depends on ARCH_REALTEK || COMPILE_TEST
> > +     select MFD_SYSCON
> > +     default y
> > +     help
> > +       Enable the common clock framework infrastructure for Realtek
> > +       system-on-chip platforms.
> > +
> > +       This provides the base support required by individual Realtek
> > +       clock controller drivers to expose clocks to peripheral devices.
> > +
> > +       If you have a Realtek-based platform, say Y.
> > +
> > +if COMMON_CLK_REALTEK
> > +
> > +config RTK_CLK_COMMON
> > +     tristate "Realtek Clock Common"
> > +     select RESET_CONTROLLER
> > +     help
> > +       Common helper code shared by Realtek clock controller drivers.
> > +
> > +       This provides utility functions and data structures used by
> > +       multiple Realtek clock implementations, and include integration
> > +       with reset controllers where required.
> > +
> > +endif
> > diff --git a/drivers/clk/realtek/Makefile
> > b/drivers/clk/realtek/Makefile new file mode 100644 index
> > 000000000000..52267de2eef4
> > --- /dev/null
> > +++ b/drivers/clk/realtek/Makefile
> > @@ -0,0 +1,4 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
> > +obj-$(CONFIG_RTK_CLK_COMMON) += clk-rtk.o
> > +
> > +clk-rtk-y += reset.o
> > diff --git a/drivers/clk/realtek/reset.c b/drivers/clk/realtek/reset.c
> > new file mode 100644 index 000000000000..0ba0d46811d4
> > --- /dev/null
> > +++ b/drivers/clk/realtek/reset.c
> > @@ -0,0 +1,125 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (C) 2019 Realtek Semiconductor Corporation  */
> > +
> > +#include <linux/of.h>
> > +#include <linux/device.h>
> > +#include "reset.h"
> > +
> > +#define RTK_RESET_BANK_SHIFT         8
> > +#define RTK_RESET_ID_MASK            0xff
> 
> Why are there 256 possible ids per bank? Each bank is only a 32-bit register,
> containing 32 (or in the case of write_en) 16 reset controls.
> 

You are right. Using 256 IDs per bank creates a sparse ID map with unnecessary holes.
We plan to change the stride to 32 (<< 5). With this change, nr_resets will simply
be num_banks * 32.

> > +#define to_rtk_reset_controller(r) container_of(r, struct
> > +rtk_reset_data, rcdev)
> 
> Please make this an inline function.
> 

I will fix it in v3.

> > +static inline struct rtk_reset_bank * rtk_reset_get_bank(struct
> > +rtk_reset_data *data, unsigned long idx) {
> > +     int bank_id = idx >> RTK_RESET_BANK_SHIFT;
> > +
> > +     return &data->banks[bank_id];
> > +}
> > +
> > +static inline int rtk_reset_get_id(struct rtk_reset_data *data,
> > +                                unsigned long idx) {
> > +     return idx & RTK_RESET_ID_MASK;
> > +}
> > +
> > +static int rtk_reset_update_bits(struct rtk_reset_data *data, u32 offset,
> > +                              u32 mask, u32 val) {
> > +     int ret;
> > +
> > +     ret = regmap_update_bits(data->regmap, offset, mask, val);
> > +     return ret;
> > +}
> > +
> > +static int rtk_reset_read(struct rtk_reset_data *data, u32 offset,
> > +u32 *val) {
> > +     int ret;
> > +
> > +     ret = regmap_read(data->regmap, offset, val);
> > +     return ret;
> > +}
> 
> The local variable ret is superfluous. And why not use
> regmap_read/update_bits() directly below?
> 

I will drop the function.

> > +
> > +static int rtk_reset_assert(struct reset_controller_dev *rcdev,
> > +                         unsigned long idx) {
> > +     struct rtk_reset_data *data = to_rtk_reset_controller(rcdev);
> > +     struct rtk_reset_bank *bank = rtk_reset_get_bank(data, idx);
> > +     u32 id = rtk_reset_get_id(data, idx);
> > +     u32 mask = bank->write_en ? (0x3 << id) : BIT(id);
> > +     u32 val = bank->write_en ? (0x2 << id) : 0;
> 
> I'd use UL(0x3) / UL(0x2) to avoid UB when compiling for 32-bit.
> 

I will apply UL() as suggested.

> Only even ids are allowed for banks with write_en set?
> 

Yes, for banks with write_en, the hardware layout uses 2 bits per control (Bit N for
reset, Bit N+1 for enable).

> > +
> > +     return rtk_reset_update_bits(data, bank->ofs, mask, val); }
> > +
> > +static int rtk_reset_deassert(struct reset_controller_dev *rcdev,
> > +                           unsigned long idx) {
> > +     struct rtk_reset_data *data = to_rtk_reset_controller(rcdev);
> > +     struct rtk_reset_bank *bank = rtk_reset_get_bank(data, idx);
> > +     u32 id = rtk_reset_get_id(data, idx);
> > +     u32 mask = bank->write_en ? (0x3 << id) : BIT(id);
> > +     u32 val = mask;
> > +
> > +     return rtk_reset_update_bits(data, bank->ofs, mask, val); }
> > +
> > +static int rtk_reset_reset(struct reset_controller_dev *rcdev,
> > +                        unsigned long idx) {
> > +     int ret;
> > +
> > +     ret = rtk_reset_assert(rcdev, idx);
> > +     if (ret)
> > +             return ret;
> > +     return rtk_reset_deassert(rcdev, idx); }
> > +
> > +static int rtk_reset_status(struct reset_controller_dev *rcdev,
> > +                         unsigned long idx) {
> > +     struct rtk_reset_data *data = to_rtk_reset_controller(rcdev);
> > +     struct rtk_reset_bank *bank = &data->banks[idx >>
> RTK_RESET_BANK_SHIFT];
> > +     u32 id = idx & RTK_RESET_ID_MASK;
> > +     u32 val;
> > +     int ret;
> > +
> > +     ret = rtk_reset_read(data, bank->ofs, &val);
> > +     if (ret)
> > +             return ret;
> > +
> > +     return !((val >> id) & 1);
> > +}
> > +
> > +static const struct reset_control_ops rtk_reset_ops = {
> > +     .reset    = rtk_reset_reset,
> > +     .assert   = rtk_reset_assert,
> > +     .deassert = rtk_reset_deassert,
> > +     .status   = rtk_reset_status,
> > +};
> > +
> > +int rtk_reset_controller_add(struct device *dev,
> > +                          struct rtk_reset_initdata *initdata) {
> > +     struct rtk_reset_data *data;
> > +
> > +     data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> > +     if (!data)
> > +             return -ENOMEM;
> > +
> > +     data->dev = dev;
> > +     data->num_banks = initdata->num_banks;
> > +     data->banks = initdata->banks;
> > +     data->regmap = initdata->regmap;
> > +     data->rcdev.owner = THIS_MODULE;
> > +     data->rcdev.ops = &rtk_reset_ops;
> > +     data->rcdev.dev = dev;
> > +     data->rcdev.of_node = dev->of_node;
> > +     data->rcdev.nr_resets = initdata->num_banks * 0x100;
> 
> This is misleading. Surely there's not num_banks * 256 resets, since each bank
> can only carry 16 or 32 resets. It'd probably better to not use nr_resets at all
> and implement your own of_xlate.
> 
> 
> regards
> Philipp

As mentioned above, by changing the shift to 5, nr_resets becomes num_banks * 32.
Since the mapping becomes linear, We believe of_reset_simple_xlate is still suitable.

Best Regards,
Yu-Chun