Re: [PATCH v6 08/10] clk: realtek: Add RTD1625-CRT clock controller driver

[Yu-Chun Lin <eleanor.lin@realtek.com>]

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