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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Jerome,

On 2022/11/28 20:23, Jerome Brunet wrote:
[ EXTERNAL EMAIL ]


On Mon 28 Nov 2022 at 16:08, Yu Tu <yu.tu@xxxxxxxxxxx> wrote:

+
+/*
+ * 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.


I got it.I'll change it as you suggest.





+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,
+	},
+};
+

[...]

+
+/* 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.


Maybe you can be more specific, I don't understand, you're interested in that part of it specifically.

I don't know if you have the document of chip. If not, I can provide it to you privately. You can ask specific questions in conjunction with your documentation and submission(The original submission came from you.). I can give you a specific answer or ask the chip designer to give you a reply.Do you think that's okay with you


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

[...]

+
+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.


Thanks for the explanation of flag.
My goal is to have the clock user describe themselves in DTS using the parent, or using the assigned-clocks and assigned-clock-parents settings in DTS. According to your explanation, some clocks like this should use CLK_SET_RATE_NO_REPARENT, right?



+};

.



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux