On 2023/1/20 17:47, Jerome Brunet wrote:
[ EXTERNAL EMAIL ] On Fri 20 Jan 2023 at 11:33, Yu Tu <yu.tu@xxxxxxxxxxx> wrote:Hi On 2023/1/19 19:37, Jerome Brunet wrote:[ EXTERNAL EMAIL ] On Mon 16 Jan 2023 at 15:42, Yu Tu <yu.tu@xxxxxxxxxxx> wrote:Add the peripherals clock controller driver in the s4 SoC family. Signed-off-by: Yu Tu <yu.tu@xxxxxxxxxxx>[...]+ +/* 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", + /* + * The frequency division from the hdmi_pll clock to the vid_pll_div + * clock is the default value of this register. When designing the + * video module of the chip, a default value that can meet the + * requirements of the video module will be solidified according + * to the usage requirements of the chip, so as to facilitate chip + * simulation. So this is ro_ops. + * It is important to note that this clock is not used on this + * chip and is described only for the integrity of the clock tree. + */If it is reset value and will be applicable to all the design, regarless of the use-case, then yes RO ops is OK >From what I understand here, the value will depend on the use-case requirements. This is a typical case where the DT prop "assigned-rate" should be used, not RO ops.Check the previous chip history, the actual scene is not used at all, basically is used in simulation. So the previous SOC was "ro_ops" without any problems. This S4 SOC is not actually useful either. So when you were upstream, you had no problem making "ro_ops". I wonder if I could delete this useless clock, so you don't have to worry about it.I don't know what to make of this. What is the point of adding a useless clock ?
As explained earlier this "vid_pll_div" is actually used in chip emulation. So next I'd like to know what you suggest to do with the clock?
By the way, the late S-Series chips removed this clock.
+ .ops = &meson_vid_pll_div_ro_ops, + .parent_data = (const struct clk_parent_data []) { + { .fw_name = "hdmi_pll", } + }, + .num_parents = 1, + .flags = CLK_SET_RATE_PARENT, + }, +}; ++ +/* VDEC clocks */ +static const struct clk_parent_data s4_dec_parent_data[] = { + { .fw_name = "fclk_div2p5", }, + { .fw_name = "fclk_div3", }, + { .fw_name = "fclk_div4", }, + { .fw_name = "fclk_div5", }, + { .fw_name = "fclk_div7", }, + { .fw_name = "hifi_pll", }, + { .fw_name = "gp0_pll", }, + { .fw_name = "xtal", } +}; + +static struct clk_regmap s4_vdec_p0_mux = { + .data = &(struct clk_regmap_mux_data){ + .offset = CLKCTRL_VDEC_CLK_CTRL, + .mask = 0x7, + .shift = 9, + .flags = CLK_MUX_ROUND_CLOSEST, + }, + .hw.init = &(struct clk_init_data) { + .name = "vdec_p0_mux", + .ops = &clk_regmap_mux_ops, + .parent_data = s4_dec_parent_data, + .num_parents = ARRAY_SIZE(s4_dec_parent_data), + /* + * When the driver uses this clock, needs to specify the patent clock + * he wants in the dts.s/patent/parent ? s/he wants/it requires ?Okay.+ */ + .flags = CLK_SET_RATE_NO_REPARENT | CLK_SET_RATE_PARENT, + }, +}; +[...]+static const struct clk_parent_data s4_vpu_clkc_parent_data[] = { + { .fw_name = "fclk_div4", }, + { .fw_name = "fclk_div3", }, + { .fw_name = "fclk_div5", }, + { .fw_name = "fclk_div7", }, + { .fw_name = "mpll1", }, + { .hw = &s4_vid_pll.hw }, + { .fw_name = "mpll2", }, + { .fw_name = "gp0_pll", }, +}; + +static struct clk_regmap s4_vpu_clkc_p0_mux = { + .data = &(struct clk_regmap_mux_data){ + .offset = CLKCTRL_VPU_CLKC_CTRL, + .mask = 0x7, + .shift = 9, + }, + .hw.init = &(struct clk_init_data) { + .name = "vpu_clkc_p0_mux", + .ops = &clk_regmap_mux_ops, + .parent_data = s4_vpu_clkc_parent_data, + .num_parents = ARRAY_SIZE(s4_vpu_clkc_parent_data), + /* + * When the driver uses this clock, needs to specify the patent clock + * he wants in the dts. + */That's quite a lot of occurences of the same comment. At the same time, other clocks with the same flag have no comment. Please make general comment, before the Video/VPU section, explaining which clocks needs on a use-case basis (through DT) and possibly how it should be set, what should drive the choices.The owner of the corresponding driver module wants to have a fixed clock, but I can't explain every specific reason.Why can't you ?So I'm going to change it all to.flags = CLK_SET_RATE_PARENT in the next version. Let CCF choose the appropriate clock as you suggested. If there is a corresponding module you want to change, ask him to give you a specific explanation. Do you think that's all right?If the flag is actually required and there is a reason, no it is not.
I will try to communicate with each module that uses the corresponding clock and provide an explanation if necessary in next edition.
I will not reply to you below.Noted.+ .flags = CLK_SET_RATE_NO_REPARENT | 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", }, + { .fw_name = "mpll2", }, + { .fw_name = "mpll3", }, + { .fw_name = "gp0_pll", }, +}; + +static struct clk_regmap s4_sd_emmc_c_clk0_sel = { + .data = &(struct clk_regmap_mux_data){ + .offset = CLKCTRL_NAND_CLK_CTRL, + .mask = 0x7, + .shift = 9, + }, + .hw.init = &(struct clk_init_data) { + .name = "sd_emmc_c_clk0_sel", + .ops = &clk_regmap_mux_ops, + .parent_data = s4_sd_emmc_clk0_parent_data, + .num_parents = ARRAY_SIZE(s4_sd_emmc_clk0_parent_data), + /* + * When the driver uses this clock, needs to specify the patent clock + * he wants in the dts. + */I'm getting a bit suspicious about the use (and abuse ...) of this flag. I don't quite get how selecting the base PLL for MMC should be done on use-case basis and should be up the board DT ... Other SoC have all used fdiv2 so far. Do you expect this setting to be part of the dtsi SoC file ?+ .flags = CLK_SET_RATE_NO_REPARENT | CLK_SET_RATE_PARENT, + }, +}; ++ +/* SPICC Clock */ +static const struct clk_parent_data s4_spicc_parent_data[] = { + { .fw_name = "xtal", }, + { .hw = &s4_sys_clk.hw }, + { .fw_name = "fclk_div4", }, + { .fw_name = "fclk_div3", }, + { .fw_name = "fclk_div2", }, + { .fw_name = "fclk_div5", }, + { .fw_name = "fclk_div7", }, +}; + +static struct clk_regmap s4_spicc0_mux = { + .data = &(struct clk_regmap_mux_data){ + .offset = CLKCTRL_SPICC_CLK_CTRL, + .mask = 0x7, + .shift = 7, + }, + .hw.init = &(struct clk_init_data) { + .name = "spicc0_mux", + .ops = &clk_regmap_mux_ops, + .parent_data = s4_spicc_parent_data, + .num_parents = ARRAY_SIZE(s4_spicc_parent_data), + /* + * When the driver uses this clock, needs to specify the patent clock + * he wants in the dts. + */This is getting too far. All the parent clocks are fixed. Let CCF do the job of picking the most adequate clock for the job instead of manually settings things+ .flags = CLK_SET_RATE_NO_REPARENT | CLK_SET_RATE_PARENT, + }, +}; ++ +/* PWM Clock */ +static const struct clk_parent_data s4_pwm_parent_data[] = { + { .fw_name = "xtal", }, + { .hw = &s4_vid_pll.hw }, + { .fw_name = "fclk_div4", }, + { .fw_name = "fclk_div3", }, +}; + +static struct clk_regmap s4_pwm_a_mux = { + .data = &(struct clk_regmap_mux_data) { + .offset = CLKCTRL_PWM_CLK_AB_CTRL, + .mask = 0x3, + .shift = 9, + }, + .hw.init = &(struct clk_init_data){ + .name = "pwm_a_mux", + .ops = &clk_regmap_mux_ops, + .parent_data = s4_pwm_parent_data, + .num_parents = ARRAY_SIZE(s4_pwm_parent_data), + /* + * When the driver uses this clock, needs to specify the patent clock + * he wants in the dts. + */Same here ... this is really going to far.+ .flags = CLK_SET_RATE_NO_REPARENT | CLK_SET_RATE_PARENT, + }, +}; ++ +static struct clk_regmap s4_saradc_mux = { + .data = &(struct clk_regmap_mux_data) { + .offset = CLKCTRL_SAR_CLK_CTRL, + .mask = 0x3, + .shift = 9, + }, + .hw.init = &(struct clk_init_data){ + .name = "saradc_mux", + .ops = &clk_regmap_mux_ops, + .parent_data = (const struct clk_parent_data []) { + { .fw_name = "xtal", }, + { .hw = &s4_sys_clk.hw }, + }, + .num_parents = 2, + /* + * When the driver uses this clock, needs to specify the patent clock + * he wants in the dts. + */For each clock type, if this flag is going to be used, I'd like a clear explanation about why it is use-case dependent and why you need manual control over this. Same applies to all the occurence.+ .flags = CLK_SET_RATE_NO_REPARENT | CLK_SET_RATE_PARENT, + }, +};
