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

[Yu Tu]

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?

> > > > +};
>
> .