Re: [PATCH V5 3/4] clk: meson: s4: add s4 SoC peripheral clock controller driver and bindings

On Mon 28 Nov 2022 at 16:08, Yu Tu [off-list ref] wrote:

quoted

quoted

+
+/*
+ * This RTC clock can be supplied by an external 32KHz crystal oscillator.
+ * If it is used, it should be documented in using fw_name and documented in the
+ * Bindings. Not currently in use on this board, so skip it.
+ */
+static u32 rtc_clk_sel[] = { 0, 1 };

No reason to do that

I'm going to change it to static u32 rtc_clk_sel[] = { 0, 1, 2 };.
I don't know if that's okay with you?

... then there is no need to specify this table.

quoted

quoted

+static const struct clk_parent_data rtc_clk_sel_parent_data[] = {
+	{ .hw = &s4_rtc_32k_by_oscin.hw },
+	{ .hw = &s4_rtc_32k_by_oscin_div.hw },
+	{ .fw_name = "ext_32k",  }
+};
+
+static struct clk_regmap s4_rtc_clk = {
+	.data = &(struct clk_regmap_mux_data) {
+		.offset = CLKCTRL_RTC_CTRL,
+		.mask = 0x3,
+		.shift = 0,
+		.table = rtc_clk_sel,
+		.flags = CLK_MUX_ROUND_CLOSEST,
+	},
+	.hw.init = &(struct clk_init_data){
+		.name = "rtc_clk_sel",
+		.ops = &clk_regmap_mux_ops,
+		.parent_data = rtc_clk_sel_parent_data,
+		.num_parents = 2,
+		.flags = CLK_SET_RATE_PARENT,
+	},
+};
+

[...]

quoted

quoted

+
+/* Video Clocks */
+static struct clk_regmap s4_vid_pll_div = {
+	.data = &(struct meson_vid_pll_div_data){
+		.val = {
+			.reg_off = CLKCTRL_VID_PLL_CLK_DIV,
+			.shift   = 0,
+			.width   = 15,
+		},
+		.sel = {
+			.reg_off = CLKCTRL_VID_PLL_CLK_DIV,
+			.shift   = 16,
+			.width   = 2,
+		},
+	},
+	.hw.init = &(struct clk_init_data) {
+		.name = "vid_pll_div",
+		/* Same to g12a */
+		.ops = &meson_vid_pll_div_ro_ops,

Please add an helpful explanation.
'Same to g12a' is not helpful.

"Because the vid_pll_div clock is a clock that does not need to change the
divisor, ops only provides meson_vid_pll_div_ro_ops."
I wonder if this description is ok for you?

I understand this divider will not change with RO ops.
I'm interrested why it does not change and how it is expected to be setup.

quoted

quoted

+		.parent_data = (const struct clk_parent_data []) {
+			{ .fw_name = "hdmi_pll", }
+		},
+		.num_parents = 1,
+		.flags = CLK_SET_RATE_PARENT,
+	},
+};

[...]

quoted

quoted

+
+static struct clk_regmap s4_vclk_sel = {
+	.data = &(struct clk_regmap_mux_data){
+		.offset = CLKCTRL_VID_CLK_CTRL,
+		.mask = 0x7,
+		.shift = 16,
+	},
+	.hw.init = &(struct clk_init_data){
+		.name = "vclk_sel",
+		.ops = &clk_regmap_mux_ops,
+		.parent_data = s4_vclk_parent_data,
+		.num_parents = ARRAY_SIZE(s4_vclk_parent_data),
+	},

You are stopping rate propagation here.
It deserves an explanation. Same goes below.

"When the driver uses this clock, needs to specify the patent clock he
wants in the dts."
Is ok for you?

Then you still don't understand the clock flag usage.

Preserving the parent selection (CLK_SET_RATE_NO_REPARENT) and rate
propagation (CLK_SET_RATE_PARENT) is not the same thing.

As it stands, your comment is not aliged with what you do.

quoted

quoted

+};

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel