Re: [PATCH v6 6/8] clk: sunxi-ng: a733: Add mod clocks support

["Junhui Liu" <junhui.liu@pigmoral.tech>]

Hi Andre,

Thanks for your patient review.

Besides the issues you pointed out, I also found a few other places that
don't match the manual. I'll fix those in the next version as well.

On Mon Nov 2, 2026 at 11:32 PM CST, Andre Przywara wrote:
> Hi,
>
> judging from the "fun" this is to review, I very much appreciate your 
> efforts in putting this together ;-)
>
> On 10/9/26 17:52, Junhui Liu wrote:
>> Add the module clocks found in the Allwinner A733 SoC, including video,
>> storage, interfaces and others. While most of these clocks are similar
>> to those in the A523 SoC, this implementation accounts for changes in
>> register offsets and introduces support for new modules.
>> 
>> Tested-by: Jerome Brunet <jbrunet@baylibre.com>
>> Signed-off-by: Junhui Liu <junhui.liu@pigmoral.tech>
>> ---
>>   drivers/clk/sunxi-ng/ccu-sun60i-a733.c | 1103 ++++++++++++++++++++++++++++++++
>>   1 file changed, 1103 insertions(+)
>> 

[...]

>> +
>> +/* Undocumented, taken from the vendor kernel. */
>> +static const struct clk_parent_data avs_parents[] = {
>> +	{ .hw = &sys_24M_clk.hw },
>> +	{ .fw_name = "hosc" },
>> +};
>
> Those two parents appear two more times below (usb01_ref_parents and 
> hdmi_sfr_parents). Please rename this array to something more generic 
> and reuse it below.

Okay, I'll rename it to sys_24M_hosc_parents and reuse it for those
clocks.

>
>> +static SUNXI_CCU_MUX_DATA_WITH_GATE(avs_clk, "avs-clk", avs_parents, 0x880,
>> +				    24, 3,		/* mux */
>> +				    BIT(31),		/* gate */
>> +				    0);
>> +
>> +static const struct clk_hw *de_parents[] = {
>> +	&pll_de_3x_clk.common.hw,
>> +	&pll_de_4x_clk.common.hw,
>> +	&pll_periph0_480M_clk.common.hw,
>> +	&pll_periph0_400M_clk.hw,
>> +	&pll_periph0_300M_clk.hw,
>> +	&pll_video0_4x_clk.common.hw,
>> +	&pll_video2_4x_clk.common.hw,
>> +};
>> +static SUNXI_CCU_M_HW_WITH_MUX_GATE(de0_clk, "de0", de_parents, 0xa00,
>> +				    0, 5,	/* M */
>> +				    24, 3,	/* mux */
>> +				    BIT(31),	/* gate */
>> +				    CLK_SET_RATE_PARENT);
>> +
>> +static const struct clk_hw *di_parents[] = {
>> +	&pll_periph0_600M_clk.hw,
>> +	&pll_periph0_480M_clk.common.hw,
>> +	&pll_periph0_400M_clk.hw,
>> +	&pll_video0_4x_clk.common.hw,
>> +	&pll_video2_4x_clk.common.hw,
>> +};
>> +static SUNXI_CCU_M_HW_WITH_MUX_GATE(di_clk, "di", di_parents, 0xa20,
>> +				    0, 5,	/* M */
>> +				    24, 3,	/* mux */
>> +				    BIT(31),	/* gate */
>> +				    CLK_SET_RATE_PARENT);
>> +
>> +static const struct clk_hw *g2d_parents[] = {
>> +	&pll_periph0_400M_clk.hw,
>> +	&pll_periph0_300M_clk.hw,
>
> My copy of the manual puts the 300MHz clock first, then the 400 MHz one. 
> Can you please check this?

Yes, I checked the manual, and the first parent is indeed 300MHz.

The last parent should also be pll_video2_4x rather than pll_video1_4x.
I'll fix both.

[...]

>> +
>> +/*
>> + * GPU_CLK = ClockSource * ((16 - M) / 16)
>> + * Here we use a div_table to select M values that result in integer divisors.
>> + */
>> +static struct clk_div_table gpu_div_table[] = {
>> +	{ .val = 0, .div = 1 },
>> +	{ .val = 8, .div = 2 },
>> +	{ .val = 12, .div = 4 },
>> +	{ .val = 14, .div = 8 },
>> +	{ .val = 15, .div = 16 },
>> +	{ /* sentinel */ },
>> +};
>> +static const struct clk_parent_data gpu_parents[] = {
>> +	{ .hw = &pll_gpu0_clk.common.hw, },
>> +	{ .hw = &pll_periph0_800M_clk.common.hw, },
>> +	{ .hw = &pll_periph0_600M_clk.hw, },
>> +	{ .hw = &pll_periph0_400M_clk.hw, },
>> +	{ .hw = &pll_periph0_300M_clk.hw, },
>> +	{ .hw = &pll_periph0_200M_clk.hw, },
>> +};
>> +static struct ccu_div gpu0_clk = {
>> +	.enable		= BIT(31),
>> +	.div		= _SUNXI_CCU_DIV_TABLE(0, 4, gpu_div_table),
>> +	.mux		= _SUNXI_CCU_MUX(24, 3),
>> +	.common		= {
>> +		.reg		= 0xb20,
>> +		.features	= CCU_FEATURE_UPDATE_BIT,
>
> Just a heads up: in my A523 CPU clocks series, I reworked the update bit 
> handling, so depending on the order of merging this might need some rework.

I've seen that. I'd prefer to keep this series independent of your A523
CPU clocks series for now. Once your series has settled, I'll send a
separate patch to adapt the A733 update bit handling, or update this
series if yours lands first.

>
>> +		.hw.init	= CLK_HW_INIT_PARENTS_DATA("gpu0", gpu_parents,
>> +							   &ccu_div_ops, 0),
>> +	}
>> +};
>> +
>> +static const struct clk_parent_data dram_parents[] = {
>> +	{ .hw = &pll_ddr_clk.common.hw, },
>> +	{ .hw = &pll_periph0_800M_clk.common.hw, },
>> +	{ .hw = &pll_periph0_600M_clk.hw, },
>> +	{ .hw = &pll_de_clk.common.hw, },
>
> Should this be pll_de_3x instead?

You're right. The second and third parents should also be periph1 rather
than periph0. I'll fix them together.

[...]

>> +
>> +static const struct clk_parent_data irtx_parents[] = {
>> +	{ .fw_name = "losc"},
>
> I think this clock should be sys_24M_clk.

You're right. I'll fix it.

>
>> +	{ .hw = &pll_periph1_600M_clk.hw },
>> +	{ .fw_name = "hosc"},
>> +};
>> +static SUNXI_CCU_M_DATA_WITH_MUX_GATE(irtx_clk, "irtx", irtx_parents, 0x1008,
>> +				      0, 5,	/* M */
>> +				      24, 3,	/* mux */
>> +				      BIT(31),	/* gate */
>> +				      0);
>> +
>> +/* Undocumented, taken from the vendor kernel. */
>> +static const struct clk_parent_data sgpio_parents[] = {
>> +	{ .fw_name = "losc"},
>> +	{ .hw = &sys_24M_clk.hw },
>> +};
>> +static SUNXI_CCU_M_DATA_WITH_MUX_GATE(sgpio_clk, "sgpio", sgpio_parents, 0x1060,
>> +				      0, 5,	/* M */
>> +				      24, 3,	/* mux */
>> +				      BIT(31),	/* gate */
>> +				      0);
>> +
>> +/* Undocumented, taken from the vendor kernel. */
>> +static const struct clk_hw *lpc_parents[] = {
>> +	&pll_video0_3x_clk.common.hw,
>> +	&pll_video1_3x_clk.common.hw,
>> +	&pll_video2_3x_clk.common.hw,
>> +	&pll_periph0_300M_clk.hw,
>> +};
>> +static SUNXI_CCU_M_HW_WITH_MUX_GATE(lpc_clk, "lpc", lpc_parents, 0x1080,
>> +				    0, 5,	/* M */
>> +				    24, 3,	/* mux */
>> +				    BIT(31),	/* gate */
>> +				    0);
>> +
>> +static const struct clk_hw *i2spcm_parents[] = {
>> +	&pll_audio0_4x_clk.common.hw,
>> +	&pll_audio1_div2_clk.hw,
>> +	&pll_audio1_div5_clk.hw,
>> +	&pll_periph0_200M_clk.hw,
>> +};
>> +static SUNXI_CCU_M_HW_WITH_MUX_GATE(i2spcm0_clk, "i2spcm0", i2spcm_parents, 0x1200,
>> +				    0, 5,	/* M */
>> +				    24, 3,	/* mux */
>> +				    BIT(31),	/* gate */
>> +				    0);
>> +static SUNXI_CCU_M_HW_WITH_MUX_GATE(i2spcm1_clk, "i2spcm1", i2spcm_parents, 0x1210,
>> +				    0, 5,	/* M */
>> +				    24, 3,	/* mux */
>> +				    BIT(31),	/* gate */
>> +				    0);
>> +static SUNXI_CCU_M_HW_WITH_MUX_GATE(i2spcm2_clk, "i2spcm2", i2spcm_parents, 0x1220,
>> +				    0, 5,	/* M */
>> +				    24, 3,	/* mux */
>> +				    BIT(31),	/* gate */
>> +				    0);
>> +
>> +static const struct clk_hw *i2spcm2_asrc_parents[] = {
>> +	&pll_audio0_4x_clk.common.hw,
>> +	&pll_audio1_div2_clk.hw,
>> +	&pll_audio1_div5_clk.hw,
>> +	&pll_periph0_300M_clk.hw,
>> +	&pll_periph1_300M_clk.hw,
>> +};
>> +static SUNXI_CCU_M_HW_WITH_MUX_GATE(i2spcm2_asrc_clk, "i2spcm2_asrc", i2spcm2_asrc_parents, 0x1224,
>> +				    0, 5,	/* M */
>> +				    24, 3,	/* mux */
>> +				    BIT(31),	/* gate */
>> +				    0);
>> +static SUNXI_CCU_M_HW_WITH_MUX_GATE(i2spcm3_clk, "i2spcm3", i2spcm_parents, 0x1230,
>> +				    0, 5,	/* M */
>> +				    24, 3,	/* mux */
>> +				    BIT(31),	/* gate */
>> +				    0);
>> +static SUNXI_CCU_M_HW_WITH_MUX_GATE(i2spcm4_clk, "i2spcm4", i2spcm_parents, 0x1240,
>> +				    0, 5,	/* M */
>> +				    24, 3,	/* mux */
>> +				    BIT(31),	/* gate */
>> +				    0);
>> +
>> +static const struct clk_hw *spdif_tx_parents[] = {
>> +	&pll_audio0_4x_clk.common.hw,
>> +	&pll_audio1_div2_clk.hw,
>> +	&pll_audio1_div5_clk.hw,
>> +};
>> +static SUNXI_CCU_M_HW_WITH_MUX_GATE(spdif_tx_clk, "spdif-tx", spdif_tx_parents, 0x1280,
>> +				    0, 5,	/* M */
>> +				    24, 3,	/* mux */
>> +				    BIT(31),	/* gate */
>> +				    0);
>> +
>> +static const struct clk_hw *spdif_rx_parents[] = {
>> +	&pll_periph0_200M_clk.hw,
>> +	&pll_periph0_300M_clk.hw,
>
> The manual I am looking at says those two muxes are "Reserved". So are 
> those parents from the BSP? Can you please add a comment about it then?

Yes, I took them from the vendor BSP. I'll add a comment explaining that.

[...]

>> +
>> +static const struct clk_parent_data usb01_ref_parents[] = {
>> +	{ .hw = &sys_24M_clk.hw },
>> +	{ .fw_name = "hosc" },
>> +};
>> +static SUNXI_CCU_MUX_DATA(usb01_ref_clk, "usb01-ref", usb01_ref_parents, 0x1340, 24, 3, 0);
>> +
>> +static SUNXI_CCU_MUX_DATA(usb2_u2_ref_clk, "usb2-u2-ref", usb01_ref_parents, 0x1348, 24, 3, 0);
>
> Don't these two clocks miss the gate at bit 31? So it should be 
> SUNXI_CCU_MUX_DATA_WITH_GATE instead?

Oops, I missed them. I'll add them.

>
>  > +
>> +static const struct clk_parent_data usb2_suspend_parents[] = {
>> +	{ .fw_name = "losc" },
>> +	{ .hw = &sys_24M_clk.hw },
>> +};
>> +static SUNXI_CCU_M_DATA_WITH_MUX_GATE(usb2_suspend_clk, "usb2-suspend", usb2_suspend_parents,
>> +				      0x1350,
>> +				      0, 5,	/* M */
>> +				      24, 1,	/* mux */
>> +				      BIT(31),	/* gate */
>> +				      0);
>> +
>> +static const struct clk_parent_data usb2_mf_parents[] = {
>> +	{ .hw = &sys_24M_clk.hw },
>> +	{ .hw = &pll_periph0_300M_clk.hw },
>> +	{ .fw_name = "hosc" },
>> +};
>> +static SUNXI_CCU_M_DATA_WITH_MUX_GATE(usb2_mf_clk, "usb2-mf", usb2_mf_parents,
>> +				      0x1354,
>> +				      0, 5,	/* M */
>> +				      24, 3,	/* mux */
>> +				      BIT(31),	/* gate */
>> +				      0);
>> +
>> +static const struct clk_parent_data usb2_u3_utmi_parents[] = {
>> +	{ .hw = &sys_24M_clk.hw },
>> +	{ .hw = &pll_periph0_300M_clk.hw },
>> +	{ .fw_name = "hosc" },
>
> Aren't those parents exactly the same as the usb2_mf_parents[] above?

Yes, I'll reuse the array.

[...]

>> +
>> +static const struct clk_hw *edp_tv_parents[] = {
>> +	&pll_video0_4x_clk.common.hw,
>> +	&pll_video1_4x_clk.common.hw,
>> +	&pll_video2_4x_clk.common.hw,
>> +	&pll_periph0_2x_clk.common.hw,
>> +};
>> +static SUNXI_CCU_M_HW_WITH_MUX_GATE(edp_tv_clk, "edp-tv", edp_tv_parents, 0x1640,
>> +				    0, 5,	/* M */
>> +				    24, 3,	/* mux */
>> +				    BIT(31),	/* gate */
>> +				    0);
>
> There is a second divider range from bits [12:8].

Okay, I'll switch this to SUNXI_CCU_DUALDIV_MUX_GATE.

>
>> +
>> +static SUNXI_CCU_GATE_HWS_WITH_PREDIV(hdmi_cec_32k_clk, "hdmi-cec-32k", pll_periph0_2x_hws, 0x1680,
>> +				      BIT(30),	/* gate */
>> +				      36621,	/* pre div */
>> +				      0);
>> +
>> +static const struct clk_parent_data hdmi_cec_parents[] = {
>> +	{ .fw_name = "losc" },
>> +	{ .hw = &hdmi_cec_32k_clk.common.hw },
>> +};
>> +static SUNXI_CCU_MUX_DATA_WITH_GATE(hdmi_cec_clk, "hdmi-cec", hdmi_cec_parents, 0x1680,
>> +				    24, 1,	/* mux */
>
> Even though there are indeed only two choices, the manual still 
> specifies the mux bits as ranging from [26:24] (as for many other clocks).

Okay, I'll update the mux width.

>
>> +				    BIT(31),	/* gate */
>> +				    0);
>> +
>> +static const struct clk_parent_data hdmi_tv_parents[] = {
>> +	{ .hw = &pll_video0_4x_clk.common.hw },
>> +	{ .hw = &pll_video1_4x_clk.common.hw },
>> +	{ .hw = &pll_video2_4x_clk.common.hw },
>> +	{ .hw = &pll_periph0_2x_clk.common.hw },
>> +};
>> +static SUNXI_CCU_DUALDIV_MUX_GATE(hdmi_tv_clk, "hdmi-tv", hdmi_tv_parents, 0x1684,
>> +				  0, 5,		/* M */
>> +				  8, 5,		/* N */
>> +				  24, 3,	/* mux */
>> +				  BIT(31),	/* gate */
>> +				  0);
>> +
>> +static const struct clk_parent_data hdmi_sfr_parents[] = {
>> +	{ .hw = &sys_24M_clk.hw },
>> +	{ .fw_name = "hosc" },
>> +};
>> +static SUNXI_CCU_MUX_DATA_WITH_GATE(hdmi_sfr_clk, "hdmi-sfr", hdmi_sfr_parents, 0x1690,
>> +				    24, 1,	/* mux */
>> +				    BIT(31),	/* gate */
>> +				    0);
>> +
>> +static SUNXI_CCU_GATE_HWS(hdcp_esm_clk, "hdcp-esm", pll_periph0_300M_hws, 0x1694, BIT(31), 0);
>> +
>> +static const struct clk_parent_data ledc_parents[] = {
>> +	{ .hw = &sys_24M_clk.hw },
>> +	{ .hw = &pll_periph0_600M_clk.hw },
>> +	{ .fw_name = "hosc" },
>> +};
>> +static SUNXI_CCU_M_DATA_WITH_MUX_GATE(ledc_clk, "ledc", ledc_parents, 0x1700,
>> +				      0, 5,	/* M */
>> +				      24, 3,	/* mux */
>> +				      BIT(31),	/* gate */
>> +				      0);
>> +
>> +static const struct clk_parent_data csi_master_parents[] = {
>> +	{ .hw = &sys_24M_clk.hw },
>> +	{ .hw = &pll_video0_4x_clk.common.hw },
>> +	{ .hw = &pll_video0_3x_clk.common.hw },
>> +	{ .hw = &pll_video1_4x_clk.common.hw },
>> +	{ .hw = &pll_video1_3x_clk.common.hw },
>> +	{ .hw = &pll_video2_4x_clk.common.hw },
>> +	{ .hw = &pll_video2_3x_clk.common.hw },
>> +};
>> +static SUNXI_CCU_DUALDIV_MUX_GATE(csi_master0_clk, "csi_master0", csi_master_parents, 0x1800,
>> +				  0, 5,		/* M */
>> +				  8, 5,		/* N */
>> +				  24, 3,	/* mux */
>> +				  BIT(31),	/* gate */
>> +				  0);
>> +static SUNXI_CCU_DUALDIV_MUX_GATE(csi_master1_clk, "csi_master1", csi_master_parents, 0x1804,
>> +				  0, 5,		/* M */
>> +				  8, 5,		/* N */
>> +				  24, 3,	/* mux */
>> +				  BIT(31),	/* gate */
>> +				  0);
>> +static SUNXI_CCU_DUALDIV_MUX_GATE(csi_master2_clk, "csi_master2", csi_master_parents, 0x1808,
>> +				  0, 5,		/* M */
>> +				  8, 5,		/* N */
>> +				  24, 3,	/* mux */
>> +				  BIT(31),	/* gate */
>> +				  0);
>> +
>> +static const struct clk_hw *csi_parents[] = {
>> +	&pll_video2_4x_clk.common.hw,
>> +	&pll_de_4x_clk.common.hw,
>> +	&pll_periph0_480M_clk.common.hw,
>> +	&pll_periph0_400M_clk.hw,
>> +	&pll_periph0_600M_clk.hw,
>
> My manual lists two more parents: video0_4x and video1_4x.

I missed them. I'll add them.

>
>> +};
>> +static SUNXI_CCU_M_HW_WITH_MUX_GATE(csi_clk, "csi", csi_parents, 0x1840,
>> +				    0, 5,	/* M */
>> +				    24, 3,	/* mux */
>> +				    BIT(31),	/* gate */
>> +				    0);
>> +
>> +static const struct clk_hw *isp_parents[] = {
>> +	&pll_video2_4x_clk.common.hw,
>> +	&pll_periph0_480M_clk.common.hw,
>> +	&pll_periph0_400M_clk.hw,
>> +	&pll_periph0_600M_clk.hw,
>> +	&pll_video0_4x_clk.common.hw,
>> +	&pll_video1_4x_clk.common.hw,
>> +};
>> +static SUNXI_CCU_M_HW_WITH_MUX_GATE(isp_clk, "isp", isp_parents, 0x1860,
>> +				    0, 5,	/* M */
>> +				    24, 3,	/* mux */
>> +				    BIT(31),	/* gate */
>> +				    0);
>> +
>> +static SUNXI_CCU_GATE_DATA(res_dcap_24m_clk, "res-dcap-24m", hosc, 0x1a00, BIT(3), 0);
>> +
>> +static const struct clk_parent_data apb2jtag_parents[] = {
>> +	{ .hw = &sys_24M_clk.hw },
>> +	{ .fw_name = "losc" },
>> +	{ .fw_name = "iosc" },
>> +	{ .hw = &pll_periph0_480M_clk.common.hw },
>> +	{ .hw = &pll_periph1_480M_clk.common.hw },
>> +	{ .hw = &pll_periph0_200M_clk.hw },
>> +	{ .hw = &pll_periph1_200M_clk.hw },
>> +};
>> +static SUNXI_CCU_M_DATA_WITH_MUX_GATE(apb2jtag_clk, "apb2jtag", apb2jtag_parents, 0x1c00,
>> +				      0, 5,	/* M */
>> +				      24, 3,	/* mux */
>> +				      BIT(31),	/* gate */
>> +				      0);
>> +
>> +static SUNXI_CCU_GATE_HWS(fanout_24M_clk, "fanout-24M", sys_24M_hws, 0x1f30, BIT(0), 0);
>> +static SUNXI_CCU_GATE_HWS_WITH_PREDIV(fanout_12M_clk, "fanout-12M", sys_24M_hws, 0x1f30,
>> +				      BIT(1), 2, 0);
>> +static SUNXI_CCU_GATE_HWS_WITH_PREDIV(fanout_16M_clk, "fanout-16M", pll_periph0_480M_hws, 0x1f30,
>> +				      BIT(2), 30, 0);
>> +static SUNXI_CCU_GATE_HWS_WITH_PREDIV(fanout_25M_clk, "fanout-25M", pll_periph0_2x_hws, 0x1f30,
>> +				      BIT(3), 48, 0);
>
> Where do those parents come from? My manual just lists the frequencies.
> Is that matching the 0x1f3c register sources?

Yes, these match the sources listed in CCMU_FAN_REG (0x1f3c).

The current definitions fold the intermediate fixed dividers:
PERI0_160M / 10 becomes PERI0_480M / 30, and PERI0_150M / 6 becomes
PERI0PLL2X / 48.

I'll use PERI0_160M / 10 and PERI0_150M / 6 in the next revision to match
the documented clock tree more directly.

>
> The rest seems to be correct, as far as my mostly manual comparison goes.
>
> Cheers,
> Andre
>

-- 
Best regards,
Junhui Liu