Message-ID: <1jldz9tpja.fsf@starbuckisacylon.baylibre.com>
Date: Mon, 30 Sep 2024 12:16:25 +0200
From: Jerome Brunet <jbrunet@...libre.com>
To: Xianwei Zhao <xianwei.zhao@...ogic.com>
Cc: Xianwei Zhao via B4 Relay <devnull+xianwei.zhao.amlogic.com@...nel.org>,
  Neil Armstrong <neil.armstrong@...aro.org>,  Michael Turquette
 <mturquette@...libre.com>,  Stephen Boyd <sboyd@...nel.org>,  Rob Herring
 <robh@...nel.org>,  Krzysztof Kozlowski <krzk+dt@...nel.org>,  Conor
 Dooley <conor+dt@...nel.org>,  Chuan Liu <chuan.liu@...ogic.com>,  Kevin
 Hilman <khilman@...libre.com>,  Martin Blumenstingl
 <martin.blumenstingl@...glemail.com>,  linux-amlogic@...ts.infradead.org,
  linux-clk@...r.kernel.org,  devicetree@...r.kernel.org,
  linux-kernel@...r.kernel.org,  linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 5/5] clk: meson: add A5 clock peripherals controller driver

On Sun 29 Sep 2024 at 16:44, Xianwei Zhao <xianwei.zhao@...ogic.com> wrote:

[...]

>>> +static A4_SYS_GATE(sys_eth_mac,              CLKCTRL_SYS_CLK_EN0_REG0, 26, 0);
>>> +
>>> +/*
>>> + * FIXME: sys_gic provides the clock for GIC(Generic Interrupt Controller).
>>> + * After clock is disabled, The GIC cannot work properly. At present, the driver
>>> + * used by our GIC is the public driver in kernel, and there is no management
>>> + * clock in the driver.
>>> + */
>>> +static A4_SYS_GATE(sys_gic,          CLKCTRL_SYS_CLK_EN0_REG0, 27, CLK_IS_CRITICAL);
>> The GIC has a driver. If it needs clock, maybe it should request it and
>> enable it, maybe as optional.
>> 
>
> This has been explained in C3. GIC is a public driver that does not
> reference clock, so you suggest setting it as CRITICAL and adding the
> "FIXME" annotation.

Yes indeed ... and at some point, it is expected something will be done
a actually fix the situation ... not just pile-up FIXME comments.

Adding a clock to a driver that should have one seems doable.

>
>>> +static A4_SYS_GATE(sys_rama,         CLKCTRL_SYS_CLK_EN0_REG0, 28, 0);
>>> +
>>> +/*
>>> + * NOTE: sys_big_nic provides the clock to the control bus of the NIC(Network
>>> + * Interface Controller) between multiple devices(CPU, DDR, RAM, ROM, GIC,
>>> + * SPIFC, CAPU, JTAG, EMMC, SDIO, sec_top, USB, Audio, ETH, SPICC) in the
>>> + * system. After clock is disabled, The NIC cannot work.
>>> + */
>> This comment looks like a clock that should be passed as ressource to a
>> bus or power-domain to be properly manage. This is a pattern that keeps
>> on repeating. I will not block you on it this time around but I strong
>> suggest you fix up the situation on the related platform. Next time
>> around, the reason won't be a valid one.
>> 
>
> There are too many modules associated with this clock... The most important
> is the inclusion of some system-level modules that are not managed by the
> driver in the kernel and cannot close their clocks, perhaps it is not
> appropriate to describe it here.
>
> In the next version I moved it to scmi-clk for management?

No. The number of module associated with a clock should not be a
concern and SCMI should not be a 'Hide all the things I don't want to do
in linux things'.

You've got a bus that needs a clocks. There are possibilities associate
a clock with bus in DT, either directly or through power-domains. Please
do it correctly.

>
>>> +static A4_SYS_GATE(sys_big_nic,              CLKCTRL_SYS_CLK_EN0_REG0, 29, CLK_IS_CRITICAL);
>>> +static A4_SYS_GATE(sys_ramb,         CLKCTRL_SYS_CLK_EN0_REG0, 30, 0);
>>> +static A4_SYS_GATE(sys_audio_top,    CLKCTRL_SYS_CLK_EN0_REG1, 0, 0);
>>> +static A4_SYS_GATE(sys_audio_vad,    CLKCTRL_SYS_CLK_EN0_REG1, 1, 0);
>>> +static A4_SYS_GATE(sys_usb,          CLKCTRL_SYS_CLK_EN0_REG1, 2, 0);
>>> +static A4_SYS_GATE(sys_sd_emmc_a,    CLKCTRL_SYS_CLK_EN0_REG1, 3, 0);
>>> +static A4_SYS_GATE(sys_sd_emmc_c,    CLKCTRL_SYS_CLK_EN0_REG1, 4, 0);
>>> +static A4_SYS_GATE(sys_pwm_ab,               CLKCTRL_SYS_CLK_EN0_REG1, 5, 0);
>>> +static A4_SYS_GATE(sys_pwm_cd,               CLKCTRL_SYS_CLK_EN0_REG1, 6, 0);
>>> +static A4_SYS_GATE(sys_pwm_ef,               CLKCTRL_SYS_CLK_EN0_REG1, 7, 0);
>>> +static A4_SYS_GATE(sys_pwm_gh,               CLKCTRL_SYS_CLK_EN0_REG1, 8, 0);
>>> +static A4_SYS_GATE(sys_spicc_1,              CLKCTRL_SYS_CLK_EN0_REG1, 9, 0);
>>> +static A4_SYS_GATE(sys_spicc_0,              CLKCTRL_SYS_CLK_EN0_REG1, 10, 0);
>>> +static A4_SYS_GATE(sys_uart_a,               CLKCTRL_SYS_CLK_EN0_REG1, 11, 0);
>>> +static A4_SYS_GATE(sys_uart_b,               CLKCTRL_SYS_CLK_EN0_REG1, 12, 0);
>>> +static A4_SYS_GATE(sys_uart_c,               CLKCTRL_SYS_CLK_EN0_REG1, 13, 0);
>>> +static A4_SYS_GATE(sys_uart_d,               CLKCTRL_SYS_CLK_EN0_REG1, 14, 0);
>>> +static A4_SYS_GATE(sys_uart_e,               CLKCTRL_SYS_CLK_EN0_REG1, 15, 0);
>>> +static A4_SYS_GATE(sys_i2c_m_a,              CLKCTRL_SYS_CLK_EN0_REG1, 16, 0);
>>> +static A4_SYS_GATE(sys_i2c_m_b,              CLKCTRL_SYS_CLK_EN0_REG1, 17, 0);
>>> +static A4_SYS_GATE(sys_i2c_m_c,              CLKCTRL_SYS_CLK_EN0_REG1, 18, 0);
>>> +static A4_SYS_GATE(sys_i2c_m_d,              CLKCTRL_SYS_CLK_EN0_REG1, 19, 0);
>>> +static A4_SYS_GATE(sys_rtc,          CLKCTRL_SYS_CLK_EN0_REG1, 21, 0);
>>> +
>>> +#define A4_AXI_GATE(_name, _reg, _bit, _flags)                               \
>>> +     A4_CLK_GATE(_name, _reg, _bit, axiclk,                          \
>>> +                 &clk_regmap_gate_ops, _flags)
>>> +
>>> +static A4_AXI_GATE(axi_audio_vad,    CLKCTRL_AXI_CLK_EN0, 0, 0);
>>> +static A4_AXI_GATE(axi_audio_top,    CLKCTRL_AXI_CLK_EN0, 1, 0);
>>> +
>>> +/*
>>> + * NOTE: axi_sys_nic provides the clock to the AXI bus of the system NIC. After
>>> + * clock is disabled, The NIC cannot work.
>>> + */
>>> +static A4_AXI_GATE(axi_sys_nic,              CLKCTRL_AXI_CLK_EN0, 2, CLK_IS_CRITICAL);
>>> +static A4_AXI_GATE(axi_ramb,         CLKCTRL_AXI_CLK_EN0, 5, 0);
>>> +static A4_AXI_GATE(axi_rama,         CLKCTRL_AXI_CLK_EN0, 6, 0);
>>> +
>>> +/*
>>> + * NOTE: axi_cpu_dmc provides the clock to the AXI bus where the CPU accesses
>>> + * the DDR. After clock is disabled, The CPU will not have access to the DDR.
>>> + */
>>> +static A4_AXI_GATE(axi_cpu_dmc,              CLKCTRL_AXI_CLK_EN0, 7, CLK_IS_CRITICAL);
>>> +static A4_AXI_GATE(axi_nna,          CLKCTRL_AXI_CLK_EN0, 12, 0);
>>> +
>>> +/*
>>> + * NOTE: axi_dev1_dmc provides the clock for the peripherals(EMMC, SDIO,
>>> + * sec_top, USB, Audio) to access the AXI bus of the DDR.
>>> + */
>> Same.
>> 
>
> These normal peripheral drivers manage the clock without a problem.
>
>>> +static A4_AXI_GATE(axi_dev1_dmc,     CLKCTRL_AXI_CLK_EN0, 13, 0);
>>> +
>>> +/*
>>> + * NOTE: axi_dev0_dmc provides the clock for the peripherals(ETH and SPICC)
>>> + * to access the AXI bus of the DDR.
>>> + */
>>> +static A4_AXI_GATE(axi_dev0_dmc,     CLKCTRL_AXI_CLK_EN0, 14, 0);
>>> +static A4_AXI_GATE(axi_dsp_dmc,              CLKCTRL_AXI_CLK_EN0, 15, 0);
>>> +
>>> +static struct clk_regmap clk_12_24m_in = {
>>> +     .data = &(struct clk_regmap_gate_data) {
>>> +             .offset = CLKCTRL_CLK12_24_CTRL,
>>> +             .bit_idx = 11,
>>> +     },
>>> +     .hw.init = &(struct clk_init_data) {
>>> +             .name = "clk_12_24m_in",
>>> +             .ops = &clk_regmap_gate_ops,
>>> +             .parent_data = &(const struct clk_parent_data) {
>>> +                     .fw_name = "xtal_24m",
>>> +             },
>>> +             .num_parents = 1,
>>> +     },
>>> +};
>>> +
>>> +static struct clk_regmap clk_12_24m = {
>>> +     .data = &(struct clk_regmap_div_data) {
>>> +             .offset = CLKCTRL_CLK12_24_CTRL,
>>> +             .shift = 10,
>>> +             .width = 1,
>>> +     },
>>> +     .hw.init = &(struct clk_init_data) {
>>> +             .name = "clk_12_24m",
>>> +             .ops = &clk_regmap_divider_ops,
>>> +             .parent_hws = (const struct clk_hw *[]) {
>>> +                     &clk_12_24m_in.hw
>>> +             },
>>> +             .num_parents = 1,
>>> +     },
>>> +};
>>> +
>>> +/* FIXME: set value 0 will div by 2 like value 1 */
>> Again, it is fine when it happens once, like the c3.
>> When the pattern starts repeating, it is time to do something about it.
>> 
>
> The corresponding suggestions have been made to the hardware designer, but
> now the designed chip cannot be repaired.

Not asking to fix the HW (although that would be nice if you could)
The HW is what it is. The SW needs fixing. That you can do.

>
>>> +static struct clk_regmap fclk_25m_div = {
>>> +     .data = &(struct clk_regmap_div_data) {
>>> +             .offset = CLKCTRL_CLK12_24_CTRL,
>>> +             .shift = 0,
>>> +             .width = 8,
>>> +     },
>>> +     .hw.init = &(struct clk_init_data) {
>>> +             .name = "fclk_25m_div",
>>> +             .ops = &clk_regmap_divider_ops,
>>> +             .parent_data = &(const struct clk_parent_data) {
>>> +                     .fw_name = "fix",
>>> +             },
>>> +             .num_parents = 1,
>>> +     },
>>> +};
>>> +
>>> +static struct clk_regmap fclk_25m = {
>>> +     .data = &(struct clk_regmap_gate_data) {
>>> +             .offset = CLKCTRL_CLK12_24_CTRL,
>>> +             .bit_idx = 12,
>>> +     },
>>> +     .hw.init = &(struct clk_init_data) {
>>> +             .name = "fclk_25m",
>>> +             .ops = &clk_regmap_gate_ops,
>>> +             .parent_hws = (const struct clk_hw *[]) {
>>> +                     &fclk_25m_div.hw
>>> +             },
>>> +             .num_parents = 1,
>>> +             .flags = CLK_SET_RATE_PARENT,
>>> +     },
>>> +};
>>> +
>>> +/*
>>> + * Channel 3(ddr_dpll_pt_clk) is manged by the DDR module; channel 12(cts_msr_clk)
>>> + * is manged by clock measures module. Their hardware are out of clock tree.
>> Yet, they exist and should be part of the bindings since they are
>> obviously input to this clock.
>> 
>
> Will add it to bindings, as optional input clock source.
>
>>> + * Channel 4 5 8 9 10 11 13 14 15 16 18 are not connected.
>>> + *
>>> + * gp1 is designed for DSU (DynamIQ Shared Unit) alone. It cannot be changed
>>> + * arbitrarily. gp1 is read-only in the kernel and is only open for debug purposes.
>>> + */
>>> +static u32 gen_parent_table[] = { 0, 1, 2, 6, 7, 17, 19, 20, 21, 22, 23, 24, 25,
>>> +                               26, 27, 28};
>>> +
>>> +static const struct clk_parent_data gen_parent_data[] = {
>>> +     { .fw_name = "oscin" },
>>> +     { .hw = &rtc_clk.hw },
>>> +     { .fw_name = "sysplldiv16" },
>>> +     { .fw_name = "gp1" },
>>> +     { .fw_name = "hifi" },
>>> +     { .fw_name = "cpudiv16" },
>>> +     { .fw_name = "fdiv2" },
>>> +     { .fw_name = "fdiv2p5" },
>>> +     { .fw_name = "fdiv3" },
>>> +     { .fw_name = "fdiv4" },
>>> +     { .fw_name = "fdiv5" },
>>> +     { .fw_name = "fdiv7" },
>>> +     { .fw_name = "mpll0" },
>>> +     { .fw_name = "mpll1" },
>>> +     { .fw_name = "mpll2" },
>>> +     { .fw_name = "mpll3" }
>>> +};

[...]

>>> +
>>> +static struct clk_regmap pwm_h_sel =
>>> +     AML_PWM_CLK_MUX(pwm_h, CLKCTRL_PWM_CLK_GH_CTRL, 25);
>>> +static struct clk_regmap pwm_h_div =
>>> +     AML_PWM_CLK_DIV(pwm_h, CLKCTRL_PWM_CLK_GH_CTRL, 16);
>>> +static struct clk_regmap pwm_h =
>>> +     AML_PWM_CLK_GATE(pwm_h, CLKCTRL_PWM_CLK_GH_CTRL, 24);
>>> +
>>> +/* Channel 7 is gp1. */
>> and ? if GP1 is RO, why can't you add it here ?
>> 
>
> gp1_pll is a special clock for DSU, it is integrated into dsu_clk, we
> don't want other modules to reference it.
>
> My understanding is that gp1_pll should not be provided to the peripheral
> clock tree, perhaps this is a redundant design.

Then /* Channel 7 is gp1 which is reserved for DSU */

Note that if GP1 is actually RO and DSU does not change the rate at
runtime through other means, then listing GP1 here should not be a
problem, spi would just be user of an available PLL.

>
>>> +static const struct clk_parent_data spicc_parent_data[] = {
>>> +     { .fw_name = "oscin" },
>>> +     { .fw_name = "sysclk" },
>>> +     { .fw_name = "fdiv4" },
>>> +     { .fw_name = "fdiv3" },
>>> +     { .fw_name = "fdiv2" },
>>> +     { .fw_name = "fdiv5" },
>>> +     { .fw_name = "fdiv7" }
>>> +};
>>> +
>>> +static struct clk_regmap spicc_0_sel = {
>>> +     .data = &(struct clk_regmap_mux_data) {
>>> +             .offset = CLKCTRL_SPICC_CLK_CTRL,
>>> +             .mask = 0x7,
>>> +             .shift = 7,
>>> +     },
>>> +     .hw.init = &(struct clk_init_data) {
>>> +             .name = "spicc_0_sel",
>>> +             .ops = &clk_regmap_mux_ops,
>>> +             .parent_data = spicc_parent_data,
>>> +             .num_parents = ARRAY_SIZE(spicc_parent_data),
>>> +     },
>>> +};

[...]

>>> +
>>> +static struct clk_regmap dspa_1 = {
>>> +     .data = &(struct clk_regmap_gate_data) {
>>> +             .offset = CLKCTRL_DSPA_CLK_CTRL0,
>>> +             .bit_idx = 29,
>>> +     },
>>> +     .hw.init = &(struct clk_init_data) {
>>> +             .name = "dspa_1",
>>> +             .ops = &clk_regmap_gate_ops,
>>> +             .parent_hws = (const struct clk_hw *[]) {
>>> +                     &dspa_1_div.hw
>>> +             },
>>> +             .num_parents = 1,
>>> +             .flags = CLK_SET_RATE_GATE | CLK_SET_RATE_PARENT,
>> A word about SET_RATE_GATE ?
>> 
>
> Look at the response to one of the questions below.

Still a word ? your comment below does justify this flag clearly.

>
>>> +     },
>>> +};
>>> +
>>> +static struct clk_regmap dspa = {
>>> +     .data = &(struct clk_regmap_mux_data){
>>> +             .offset = CLKCTRL_DSPA_CLK_CTRL0,
>>> +             .mask = 0x1,
>>> +             .shift = 15,
>>> +     },
>>> +     .hw.init = &(struct clk_init_data) {
>>> +             .name = "dspa",
>>> +             .ops = &clk_regmap_mux_ops,
>>> +             .parent_hws = (const struct clk_hw *[]) {
>>> +                     &dspa_0.hw,
>>> +                     &dspa_1.hw
>>> +             },
>>> +             .num_parents = 2,
>>> +             /*
>>> +              * NOTE: This level of mux is "no glitch mux", and mux_0
>>> +              * (here dspa_0) is not only the clock source for mux, but also
>>> +              * provides a working clock for "no glitch mux". "no glitch mux"
>>> +              * can be switched only when mux_0 has a clock input. Therefore,
>>> +              * add flag CLK_OPS_PARENT_ENABLE to ensure that mux_0 has clock
>>> +              * when "no glitch mux" works.
>>> +              */
>>> +             .flags = CLK_SET_RATE_PARENT | CLK_OPS_PARENT_ENABLE,
>> Humm CLK_OPS_PARENT_ENABLE is not how we have handling glitch free mux
>> so far. What changed ?
>> 
>
> This is a hidden problem we have discovered in recent years, the previous
> chip use is ping-pong switching "no-glitch-mux/glitch free" have this
> problem. If mux_0 does not have a clock "no-glitch-mux", it will not be
> able to switch channels, and I will organize and submit patch to fix it
> later.

The NOTE comment and what the code does are not aligned.
* Your comment says that input #0 must be enabled for the mux to work
* The flag ensure that parent is enabled before switching to it. There
  is nothing specific about input #0

>
>>> +     },
>>> +};
>>> +
>>> +/* Channel 6 is gp1. */
>>> +static u32 nna_parent_table[] = { 0, 1, 2, 3, 4, 5, 7};
>>> +
>>> +static const struct clk_parent_data nna_parent_data[] = {
>>> +     { .fw_name = "oscin" },
>>> +     { .fw_name = "fdiv2p5" },
>>> +     { .fw_name = "fdiv4" },
>>> +     { .fw_name = "fdiv3" },
>>> +     { .fw_name = "fdiv5" },
>>> +     { .fw_name = "fdiv2" },
>>> +     { .fw_name = "hifi" }
>>> +};

Powered by blists - more mailing lists