From: Yu Tu <yu.tu@amlogic.com>
To: Jerome Brunet <jbrunet@baylibre.com>, <linux-clk@vger.kernel.org>,
<linux-arm-kernel@lists.infradead.org>,
<linux-amlogic@lists.infradead.org>,
<linux-kernel@vger.kernel.org>, <devicetree@vger.kernel.org>,
Rob Herring <robh+dt@kernel.org>,
Neil Armstrong <narmstrong@baylibre.com>,
Kevin Hilman <khilman@baylibre.com>,
Michael Turquette <mturquette@baylibre.com>,
Stephen Boyd <sboyd@kernel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Subject: Re: [PATCH V3 6/6] clk: meson: s4: add s4 SoC peripheral clock controller driver
Date: Tue, 30 Aug 2022 16:20:05 +0800 [thread overview]
Message-ID: <2b6035f3-8cbe-ab75-bed9-5751b141d3d6@amlogic.com> (raw)
In-Reply-To: <1j7d2rte33.fsf@starbuckisacylon.baylibre.com>
On 2022/8/29 20:19, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
>
>
> On Tue 16 Aug 2022 at 20:00, Yu Tu <yu.tu@amlogic.com> 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", },
>>>> +};
>
> .
next prev parent reply other threads:[~2022-08-30 8:20 UTC|newest]
Thread overview: 44+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-08-05 8:57 [PATCH V3 0/6] Add S4 SoC PLL and Peripheral clock controller Yu Tu
2022-08-05 8:57 ` [PATCH V3 1/6] dt-bindings: clock: meson: add S4 SoC PLL clock controller bindings Yu Tu
2022-08-05 9:13 ` Krzysztof Kozlowski
2022-08-05 8:57 ` [PATCH V3 2/6] arm64: dts: meson: add S4 Soc PLL clock controller in DT Yu Tu
2022-08-05 9:16 ` Krzysztof Kozlowski
2022-08-05 9:39 ` Yu Tu
2022-08-10 13:32 ` Jerome Brunet
2022-08-15 6:17 ` Yu Tu
2022-08-29 9:43 ` Jerome Brunet
2022-08-30 6:05 ` Yu Tu
2022-08-30 6:36 ` Jerome Brunet
2022-08-30 7:06 ` Yu Tu
2022-08-05 8:57 ` [PATCH V3 3/6] clk: meson: S4: add support for Amlogic S4 SoC PLL clock driver Yu Tu
2022-08-10 13:47 ` Jerome Brunet
2022-08-15 6:34 ` Yu Tu
2022-08-15 13:20 ` Yu Tu
2022-08-29 9:48 ` Jerome Brunet
2022-08-30 6:13 ` Yu Tu
2022-08-30 6:44 ` Jerome Brunet
2022-08-30 7:37 ` Yu Tu
2022-09-21 8:40 ` Yu Tu
2022-09-28 15:27 ` Jerome Brunet
2022-09-29 7:07 ` Yu Tu
2022-10-22 12:22 ` Jerome Brunet
2022-10-24 11:33 ` Yu Tu
2022-08-29 9:46 ` Jerome Brunet
2022-08-30 6:08 ` Yu Tu
2022-08-05 8:57 ` [PATCH V3 4/6] dt-bindings: clk: meson: add S4 SoC peripheral clock controller bindings Yu Tu
2022-08-05 9:15 ` Krzysztof Kozlowski
2022-08-05 9:33 ` Yu Tu
2022-08-08 6:16 ` Krzysztof Kozlowski
2022-08-08 10:00 ` Yu Tu
2022-08-05 8:57 ` [PATCH V3 5/6] arm64: dts: meson: add S4 Soc Peripheral clock controller in DT Yu Tu
2022-08-05 9:16 ` Krzysztof Kozlowski
2022-08-05 9:36 ` Yu Tu
2022-08-08 6:17 ` Krzysztof Kozlowski
2022-08-08 10:02 ` Yu Tu
2022-08-05 8:57 ` [PATCH V3 6/6] clk: meson: s4: add s4 SoC peripheral clock controller driver Yu Tu
2022-08-10 13:57 ` Jerome Brunet
2022-08-16 12:00 ` Yu Tu
2022-08-29 12:19 ` Jerome Brunet
2022-08-30 8:20 ` Yu Tu [this message]
2022-09-21 9:01 ` Yu Tu
2022-09-28 15:35 ` Jerome Brunet
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2b6035f3-8cbe-ab75-bed9-5751b141d3d6@amlogic.com \
--to=yu.tu@amlogic.com \
--cc=devicetree@vger.kernel.org \
--cc=jbrunet@baylibre.com \
--cc=khilman@baylibre.com \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=linux-amlogic@lists.infradead.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-clk@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=martin.blumenstingl@googlemail.com \
--cc=mturquette@baylibre.com \
--cc=narmstrong@baylibre.com \
--cc=robh+dt@kernel.org \
--cc=sboyd@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).