| 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
| ||
|
Message-ID: <d143f7291ddc4f6fb5b3c3d15f4ec0e4@realtek.com>
Date: Wed, 14 Jan 2026 08:58:09 +0000
From: Yu-Chun Lin [林祐君] <eleanor.lin@...ltek.com>
To: Philipp Zabel <p.zabel@...gutronix.de>,
"mturquette@...libre.com"
<mturquette@...libre.com>,
"sboyd@...nel.org" <sboyd@...nel.org>,
"robh@...nel.org" <robh@...nel.org>,
"krzk+dt@...nel.org"
<krzk+dt@...nel.org>,
"conor+dt@...nel.org" <conor+dt@...nel.org>,
Edgar Lee [李承諭] <cylee12@...ltek.com>,
Jyan Chou [周芷安] <jyanchou@...ltek.com>
CC: "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-clk@...r.kernel.org" <linux-clk@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
James Tai [戴志峰] <james.tai@...ltek.com>,
CY_Huang[黃鉦晏] <cy.huang@...ltek.com>,
Stanley Chang[昌育德] <stanley_chang@...ltek.com>
Subject: RE: [PATCH v2 2/9] clk: realtek: Add basic reset support
Hi, Philipp,
Thanks for your review!
> On Di, 2026-01-13 at 19:23 +0800, Yu-Chun Lin wrote:
> > From: Cheng-Yu Lee <cylee12@...ltek.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@...ltek.com>
> > Co-developed-by: Yu-Chun Lin <eleanor.lin@...ltek.com>
> > Signed-off-by: Yu-Chun Lin <eleanor.lin@...ltek.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@...r.kernel.org
> > L: linux-clk@...r.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
Powered by blists - more mailing lists