Re: [PATCH V3 6/6] clk: meson: s4: add s4 SoC peripheral clock controller driver
From: Yu Tu
Date: Tue Aug 30 2022 - 04:20:19 EST
On 2022/8/29 20:19, Jerome Brunet wrote:
[ EXTERNAL EMAIL ]
On Tue 16 Aug 2022 at 20:00, Yu Tu <yu.tu@xxxxxxxxxxx> wrote:
Please trim your replies
diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
index f4244edc7b28..ec6beb9284d3 100644
--- a/drivers/clk/meson/Kconfig
+++ b/drivers/clk/meson/Kconfig
@@ -127,4 +127,17 @@ config COMMON_CLK_S4_PLL
Support for the pll clock controller on Amlogic S805X2 and S905Y4 devices,
aka s4. Amlogic S805X2 and S905Y4 devices include AQ222 and AQ229.
Say Y if you want peripherals and CPU frequency scaling to work.
+
+config COMMON_CLK_S4
+ tristate "S4 SoC Peripherals clock controllers support"
+ depends on ARM64
+ default y
+ select COMMON_CLK_MESON_REGMAP
+ select COMMON_CLK_MESON_DUALDIV
+ select COMMON_CLK_MESON_VID_PLL_DIV
+ select COMMON_CLK_S4_PLL
Do you really this ? your driver does not even include the related
header.
If the PLL driver is not turned on in DTS, will it not cause an error?
I don't get the question.
Kconfig list compile deps. S4 PLL is not a compile dep of the peripheral
controller.
If you really want to, you may use 'imply'.
V4 has been changed as you suggested.
+static const struct clk_parent_data sys_ab_clk_parent_data[] = {
+ { .fw_name = "xtal" },
+ { .fw_name = "fclk_div2" },
+ { .fw_name = "fclk_div3" },
+ { .fw_name = "fclk_div4" },
+ { .fw_name = "fclk_div5" },
+ { .fw_name = "fclk_div7" },
+ { .hw = &s4_rtc_clk.hw }
+};
+
+static struct clk_regmap s4_sysclk_b_sel = {
+ .data = &(struct clk_regmap_mux_data){
+ .offset = CLKCTRL_SYS_CLK_CTRL0,
+ .mask = 0x7,
+ .shift = 26,
+ .table = mux_table_sys_ab_clk_sel,
+ },
+ .hw.init = &(struct clk_init_data){
+ .name = "sysclk_b_sel",
+ .ops = &clk_regmap_mux_ro_ops,
Why is this using the RO ops ?
Sys_clk is initialized during the Uboot phase and is fixed at
166.666MHz. So I'm going to change it to ro.
That really much depends on the bootloader and is a pretty weak design.
The bootloader deps should be kept as minimal as possible.
I see no reason for RO.
You may cut rate propagation on the user if you need to and continue to
whatever you want in your u-boot
I think I know what you mean. But we let the user be in control and not
set the frequency, which can be risky. If you insist, I will change it
as you suggest.
+ .parent_data = sys_ab_clk_parent_data,
+ .num_parents = ARRAY_SIZE(sys_ab_clk_parent_data),
+ },
+};
+
+static struct clk_regmap s4_sysclk_b_div = {
+ .data = &(struct clk_regmap_div_data){
+ .offset = CLKCTRL_SYS_CLK_CTRL0,
+ .shift = 16,
+ .width = 10,
+ },
+ .hw.init = &(struct clk_init_data){
+ .name = "sysclk_b_div",
+ .ops = &clk_regmap_divider_ro_ops,
Same here and for the rest of the sys part
Same above.
We can play that game for a while
Ah, you're so funny.
+
+/* 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",
+ .ops = &meson_vid_pll_div_ro_ops,
Why RO ? applies to the rest of the video part.
Because vid_pll_div parent is HDMI_PLL, and HDMI_PLL is a fixed
frequency. Flags is CLK_SET_RATE_PARENT. So we use RO.
If the HDMI_PLL is fixed somehow, that is not reason for this clock to
be RO
Can I remove RO and use CLK_SET_RATE_NO_REPARENT instead, which one do you
think is more reasonable?
Neither. CLK_SET_RATE_NO_REPARENT makes no sense, it is not mux
"drivers/clk/meson/vid-pll-div.c"
This file only provides ro_ops. Maybe the submission records will give
us the answer.
In fact, our hardware design is the same as the G12 series.
+ .parent_data = (const struct clk_parent_data []) {
+ { .fw_name = "hdmi_pll", }
+ },
+ .num_parents = 1,
+ .flags = CLK_SET_RATE_PARENT,
+ },
+};
+
+static struct clk_regmap s4_vid_pll_sel = {
+ .data = &(struct clk_regmap_mux_data){
+ .offset = CLKCTRL_VID_PLL_CLK_DIV,
+ .mask = 0x1,
+ .shift = 18,
+ },
+ .hw.init = &(struct clk_init_data){
+ .name = "vid_pll_sel",
+ .ops = &clk_regmap_mux_ops,
+ /*
+ * bit 18 selects from 2 possible parents:
+ * vid_pll_div or hdmi_pll
+ */
+ .parent_data = (const struct clk_parent_data []) {
+ { .hw = &s4_vid_pll_div.hw },
+ { .fw_name = "hdmi_pll", }
+ },
+ .num_parents = 2,
+ .flags = CLK_SET_RATE_NO_REPARENT,
Why ? are you planning to DT assigned clocks to statically set this ?
Because vid_pll_sel one parent is HDMI_PLL, and HDMI_PLL is a fixed
frequency. To prevent modification, use CLK_SET_RATE_NO_REPARENT.
Again, this makes no sense.
Unfortunately you don't read V4, in fact I have corrected in V4.
".flags = CLK_SET_RATE_PARENT," in V4. Is that okay with you?
+ },
+};
+
+static struct clk_regmap s4_vid_pll = {
+ .data = &(struct clk_regmap_gate_data){
+ .offset = CLKCTRL_VID_PLL_CLK_DIV,
+ .bit_idx = 19,
+ },
+ .hw.init = &(struct clk_init_data) {
+ .name = "vid_pll",
+ .ops = &clk_regmap_gate_ops,
+ .parent_hws = (const struct clk_hw *[]) {
+ &s4_vid_pll_sel.hw
+ },
+ .num_parents = 1,
+ .flags = CLK_SET_RATE_PARENT,
+ },
+};
+
+static const struct clk_parent_data s4_vclk_parent_data[] = {
+ { .hw = &s4_vid_pll.hw },
+ { .fw_name = "gp0_pll", },
+ { .fw_name = "hifi_pll", },
+ { .fw_name = "mpll1", },
+ { .fw_name = "fclk_div3", },
+ { .fw_name = "fclk_div4", },
+ { .fw_name = "fclk_div5", },
+ { .fw_name = "fclk_div7", },
+};
+
+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),
+ .flags = CLK_SET_RATE_NO_REPARENT,
Same
Since fclk_div* is a fixed frequency value, mplL1 and hifi_pll and gp0_pll
are used by other specialized modules, vid_pll has CLK_SET_RATE_PARENT. The
parent of vid_pll is that vid_pll_sel uses CLK_SET_RATE_NO_REPARENT.
Still not good.
You don't have CLK_SET_RATE, propagation is stopped and parent clock
will not changed. The best parent will be picked but not changed.
If one parent MUST NOT be picked, just remove it from the list and add a
explaining why
[...]
Okay.
+
+static struct clk_regmap s4_ts_clk_div = {
+ .data = &(struct clk_regmap_div_data){
+ .offset = CLKCTRL_TS_CLK_CTRL,
+ .shift = 0,
+ .width = 8,
+ },
+ .hw.init = &(struct clk_init_data){
+ .name = "ts_clk_div",
+ .ops = &clk_regmap_divider_ops,
+ .parent_data = &(const struct clk_parent_data) {
+ .fw_name = "xtal",
+ },
+ .num_parents = 1,
propagation stopped ?
Its parent is xtal, so I should use CLK_SET_RATE_NO_REPARENT.
Still no. You seem to have problem with the meaning of
CLK_SET_RATE_NO_REPARENT.
* CLK_SET_RATE_NO_REPARENT: means the parent will no be changed, even if
selecting another parent would result in a closer rate to the
request. It makes sense only if the clock has several parents
* CLK_SET_RATE_PARENT: means rate change may propagate the parent,
meaning the rate of the parent may change if it help the child achieve
a closer rate to the request
Thank you for explaining.I got it.
+ },
+};
+
+static struct clk_regmap s4_ts_clk_gate = {
+ .data = &(struct clk_regmap_gate_data){
+ .offset = CLKCTRL_TS_CLK_CTRL,
+ .bit_idx = 8,
+ },
+ .hw.init = &(struct clk_init_data){
+ .name = "ts_clk",
+ .ops = &clk_regmap_gate_ops,
+ .parent_hws = (const struct clk_hw *[]) {
+ &s4_ts_clk_div.hw
+ },
+ .num_parents = 1,
+ },
propagation stopped ?
I will add CLK_SET_RATE_PARENT.
[...]
+/* EMMC/NAND clock */
+
+static const struct clk_parent_data s4_sd_emmc_clk0_parent_data[] = {
+ { .fw_name = "xtal", },
+ { .fw_name = "fclk_div2", },
+ { .fw_name = "fclk_div3", },
+ { .fw_name = "hifi_pll", },
+ { .fw_name = "fclk_div2p5", },
+ /*
+ * Following these parent clocks, we should also have had mpll2, mpll3
+ * and gp0_pll but these clocks are too precious to be used here. All
+ * the necessary rates for MMC and NAND operation can be acheived using
+ * hifi_pll or fclk_div clocks
+ */
You don't want to list mplls but hifi_pll is fine ? seems dangerous.
hifi pll is for EMMC and NAND on this SoC.
That deserve a better explanation.
Why can't it use fdiv2 and xtal like the previous SoCs ?
Which PLLs are you using for Audio then ?
Typical operation on these SoCs usually require 3 PLLs to acheive all rates
I'll list all the clocks and let the driver itself select Parent as needed.
+/*
+ * gen clk is designed for debug/monitor some internal clock quality. Some of the
+ * corresponding clock sources are not described in the clock tree, so they are skipped.
+ */
Still feels a bit light, don't you think ? Among all the clocks, can't
you add a bit more parents here ? It would certainly help debug down the road
[16:12] is gen_clk source select.All is:
0: cts_oscin_clk
1:cts_rtc_clk
2:sys_pll_div16 (internal clock)
3:ddr_pll_div32 (internal clock)
4: vid_pll
5: gp0_pll
7: hifi_pll
10:adc_dpll_clk_b3 (internal clock for debug)
11:adc_dpll_intclk (internal clock for debug)
12:clk_msr_src(select from all internal clock except PLLs);
16: no used
17: sys_cpu_clk_div16 (internal clock)
19: fclk_div2
20: fclk_div2p5
21: fclk_div3
22: fclk_div4
23: fclk_div5
24: fclk_div7
25: mpll0
26: mpll1
27: mpll2
28: mpll3
So i only added the clocks that will actually be used, and some debugging
clock peripherals will not be used.
you may at least add vid_pll
Okay.
+static u32 s4_gen_clk_mux_table[] = { 0, 5, 7, 19, 21, 22,
+ 23, 24, 25, 26, 27, 28 };
+static const struct clk_parent_data s4_gen_clk_parent_data[] = {
+ { .fw_name = "xtal", },
+ { .fw_name = "gp0_pll", },
+ { .fw_name = "hifi_pll", },
+ { .fw_name = "fclk_div2", },
+ { .fw_name = "fclk_div3", },
+ { .fw_name = "fclk_div4", },
+ { .fw_name = "fclk_div5", },
+ { .fw_name = "fclk_div7", },
+ { .fw_name = "mpll0", },
+ { .fw_name = "mpll1", },
+ { .fw_name = "mpll2", },
+ { .fw_name = "mpll3", },
+};
.