[PATCH v6 08/10] clk: realtek: Add RTD1625-CRT clock controller driver
Yu-Chun Lin
eleanor.lin at realtek.com
Fri Apr 17 00:45:48 PDT 2026
Hi Brian,
> Hi Yu-Chun,
>
(snip)
> > +
> > +static const struct reg_sequence pll_acpu_seq_power_off[] = {
> > + {RTD1625_REG_PLL_ACPU2, 0x4},
> > +};
> > +
> > +static const struct reg_sequence pll_acpu_seq_pre_set_freq[] = {
> > + {RTD1625_REG_PLL_SSC_DIG_ACPU0, 0x4},
> > +};
> > +
> > +static const struct reg_sequence pll_acpu_seq_post_set_freq[] = {
> > + {RTD1625_REG_PLL_SSC_DIG_ACPU0, 0x5},
> > +};
> > +
> > +static struct clk_pll pll_acpu = {
>
> static const?
>
The clock object should not be declared as const.
> > + .clkr.hw.init = CLK_HW_INIT("pll_acpu", "osc27m", &rtk_clk_pll_ops, CLK_GET_RATE_NOCACHE),
> > + .seq_power_on = pll_acpu_seq_power_on,
> > + .num_seq_power_on = ARRAY_SIZE(pll_acpu_seq_power_on),
> > + .seq_power_off = pll_acpu_seq_power_off,
> > + .num_seq_power_off = ARRAY_SIZE(pll_acpu_seq_power_off),
> > + .seq_pre_set_freq = pll_acpu_seq_pre_set_freq,
> > + .num_seq_pre_set_freq = ARRAY_SIZE(pll_acpu_seq_pre_set_freq),
> > + .seq_post_set_freq = pll_acpu_seq_post_set_freq,
> > + .num_seq_post_set_freq = ARRAY_SIZE(pll_acpu_seq_post_set_freq),
> > + .freq_reg = RTD1625_REG_PLL_SSC_DIG_ACPU1,
> > + .freq_tbl = acpu_tbl,
> > + .freq_mask = FREQ_NF_MASK,
> > + .freq_ready_reg = RTD1625_REG_PLL_SSC_DIG_ACPU_DBG2,
> > + .freq_ready_mask = BIT(20),
> > + .freq_ready_val = BIT(20),
> > + .power_reg = RTD1625_REG_PLL_ACPU2,
> > + .power_mask = 0x7,
> > + .power_val_on = 0x3,
> > +};
(snip)
> > +
> > +static const struct reg_sequence pll_ve1_seq_post_set_freq[] = {
> > + {RTD1625_REG_PLL_SSC_DIG_VE1_0, 0x5},
> > +};
> > +
> > +static struct clk_pll pll_ve1 = {
>
> Same here about static const, plus some others below?
>
No. The clock object cannot be const.
(snip)
> > +static const struct of_device_id rtd1625_crt_match[] = {
> > + {.compatible = "realtek,rtd1625-crt-clk", .data = &rtd1625_crt_desc,},
> > + {/* sentinel */}
>
> Add a space around the comment like so:
>
> { /* sentinel */ }
>
Ack.
>
> > +};
> > +
> > +static struct platform_driver rtd1625_crt_driver = {
> > + .probe = rtd1625_crt_probe,
> > + .driver = {
> > + .name = "rtk-rtd1625-crt-clk",
> > + .of_match_table = rtd1625_crt_match,
> > + },
> > +};
> > +
> > +static int __init rtd1625_crt_init(void)
> > +{
> > + return platform_driver_register(&rtd1625_crt_driver);
> > +}
> > +subsys_initcall(rtd1625_crt_init);
> > +
> > +MODULE_DESCRIPTION("Reatek RTD1625 CRT Controller Driver");
>
>s/Reatek/Realtex/
>
Will fix it.
> > +MODULE_AUTHOR("Cheng-Yu Lee <cylee12 at realtek.com>");
> > +MODULE_LICENSE("GPL");
> > +MODULE_IMPORT_NS("REALTEK_CLK");
> > diff --git a/drivers/reset/realtek/Kconfig b/drivers/reset/realtek/Kconfig
> > index 99a14d355803..a44c7834191c 100644
> > --- a/drivers/reset/realtek/Kconfig
> > +++ b/drivers/reset/realtek/Kconfig
> > @@ -1,3 +1,5 @@
> > # SPDX-License-Identifier: GPL-2.0-only
> > config RESET_RTK_COMMON
> > bool
> > + select AUXILIARY_BUS
> > + default COMMON_CLK_RTD1625
> > diff --git a/drivers/reset/realtek/Makefile b/drivers/reset/realtek/Makefile
> > index b59a3f7f2453..8ca1fa939f10 100644
> > --- a/drivers/reset/realtek/Makefile
> > +++ b/drivers/reset/realtek/Makefile
> > @@ -1,2 +1,2 @@
> > # SPDX-License-Identifier: GPL-2.0-only
> > -obj-$(CONFIG_RESET_RTK_COMMON) += common.o
> > +obj-$(CONFIG_RESET_RTK_COMMON) += common.o reset-rtd1625-crt.o
>
>CONFIG_RESET_RTK_COMMON is supposed to be common, right? If so, the
> SoC-specific driver shouldn't be included here.
>
This Makefile will change to
obj-$(CONFIG_RESET_RTK_COMMON) += common.o
obj-$(CONFIG_RESET_RTD1625) += reset-rtd1625-crt.o
> > diff --git a/drivers/reset/realtek/reset-rtd1625-crt.c b/drivers/reset/realtek/reset-rtd1625-crt.c
> > new file mode 100644
> > index 000000000000..ebb15bb68885
> > --- /dev/null
> > +++ b/drivers/reset/realtek/reset-rtd1625-crt.c
> > @@ -0,0 +1,186 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (C) 2026 Realtek Semiconductor Corporation
> > + */
> > +
> > +#include <dt-bindings/reset/realtek,rtd1625.h>
> > +#include <linux/auxiliary_bus.h>
> > +#include <linux/device.h>
> > +#include <linux/errno.h>
> > +#include <linux/slab.h>
> > +#include "common.h"
> > +
> > +#define RTD1625_CRT_RSTN_MAX 123
> > +
> > +static struct rtk_reset_desc rtd1625_crt_reset_descs[] = {
> > + /* Bank 0: offset 0x0 */
> > + [RTD1625_CRT_RSTN_MISC] = { .ofs = 0x0, .bit = 0, .write_en = 1 },
> > + [RTD1625_CRT_RSTN_DIP] = { .ofs = 0x0, .bit = 2, .write_en = 1 },
> > + [RTD1625_CRT_RSTN_GSPI] = { .ofs = 0x0, .bit = 4, .write_en = 1 },
> > + [RTD1625_CRT_RSTN_SDS] = { .ofs = 0x0, .bit = 6, .write_en = 1 },
> > + [RTD1625_CRT_RSTN_SDS_REG] = { .ofs = 0x0, .bit = 8, .write_en = 1 },
> > + [RTD1625_CRT_RSTN_SDS_PHY] = { .ofs = 0x0, .bit = 10, .write_en = 1 },
> > + [RTD1625_CRT_RSTN_GPU2D] = { .ofs = 0x0, .bit = 12, .write_en = 1 },
> > + [RTD1625_CRT_RSTN_DC_PHY] = { .ofs = 0x0, .bit = 22, .write_en = 1 },
> > + [RTD1625_CRT_RSTN_DCPHY_CRT] = { .ofs = 0x0, .bit = 24, .write_en = 1 },
> > + [RTD1625_CRT_RSTN_LSADC] = { .ofs = 0x0, .bit = 26, .write_en = 1 },
> > + [RTD1625_CRT_RSTN_SE] = { .ofs = 0x0, .bit = 28, .write_en = 1 },
> > + [RTD1625_CRT_RSTN_DLA] = { .ofs = 0x0, .bit = 30, .write_en = 1 },
>
> Sashiko reports:
> https://sashiko.dev/#/patchset/20260402073957.2742459-1-eleanor.lin%40realtek.com
>
> Can this cause undefined behavior during reset mask computation?
>
> Several reset array entries set .bit = 30 and .write_en = 1. In
> rtk_reset_assert() and rtk_reset_deassert(), if the bitmask is computed as
> 0x3 << desc->bit, 0x3 is a signed 32-bit integer literal. Left-shifting it by
> 30 results in 0xC0000000, which exceeds the maximum positive value for a
> signed 32-bit integer.
>
> Modifying the sign bit via left-shift on a signed type invokes undefined
> behavior in C. Would an unsigned literal (e.g., 0x3U << desc->bit) be needed
> to safely construct the mask?
Agreed, Will make it 0x3U.
(snip)
> > +
> > +static int rtd1625_crt_reset_probe(struct auxiliary_device *adev,
> > + const struct auxiliary_device_id *id)
> > +{
> > + struct device *dev = &adev->dev;
> > + struct rtk_reset_data *data;
> > +
> > + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> > + if (!data)
> > + return -ENOMEM;
> > +
> > + data->descs = rtd1625_crt_reset_descs;
> > + data->rcdev.nr_resets = RTD1625_CRT_RSTN_MAX;
> > + return rtk_reset_controller_add(dev, data);
>
> Sashiko reports:
> https://sashiko.dev/#/patchset/20260402073957.2742459-1-eleanor.lin%40realtek.com
>
> Will the reset controller driver unconditionally fail to probe with -ENODEV
> due to an incompatible regmap acquisition method?
>
> The rtk_reset_controller_add() helper attempts to retrieve the shared regmap
> from the parent clock device using dev_get_regmap(parent, NULL). However, the
> parent clock driver (rtk_clk_probe()) acquires its regmap via
> device_node_to_regmap().
>
> This syscon helper creates the regmap but does not associate it with the
> parent struct device via devres. Because the regmap is absent from the
> parent's devres list, dev_get_regmap() will always return NULL, causing the
> reset driver probe to fail unconditionally and leaving dependent peripherals
> without reset control.
>
> Brian
>
Thanks for identifying this issue. I've fixed the regmap passing mechanism:
Changes:
1. 'rtk_reset_controller_register()' in clk/realtek/common.c passes the
regmap as platform data via 'devm_auxiliary_device_create()'
2. 'rtk_reset_controller_add()' in reset/realtek/common.c retrieves it
using 'dev_get_platdata()' instead of 'dev_get_regmap()'
This ensures the reset controller can access the shared regmap regardless
of how the parent clock driver acquired it
Best Regards,
Yu-Chun
More information about the linux-arm-kernel
mailing list