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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 28 Nov 2022 13:23:08 +0100
From:   Jerome Brunet <jbrunet@...libre.com>
To:     Yu Tu <yu.tu@...ogic.com>, linux-clk@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-amlogic@...ts.infradead.org, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org, Rob Herring <robh+dt@...nel.org>,
        Neil Armstrong <narmstrong@...aro.org>,
        Kevin Hilman <khilman@...libre.com>,
        Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Martin Blumenstingl <martin.blumenstingl@...glemail.com>
Cc:     kelvin.zhang@...ogic.com
Subject: 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 <yu.tu@...ogic.com> 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.



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

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

>
>> 
>>> +};