Re: [PATCH 2/2] clk: amlogic: c3: Limit the rate boundaries of clk_hw

[Jerome Brunet <jbrunet@baylibre.com>]

On Mon 13 Jan 2025 at 13:24, Chuan Liu <chuan.liu@amlogic.com> wrote:

> hi Jerome,
>
> Thanks for your prompt reply.
>
>
> On 1/10/2025 9:55 PM, Jerome Brunet wrote:
>> [ EXTERNAL EMAIL ]
>>
>> On Fri 10 Jan 2025 at 19:47, Chuan Liu via B4 Relay <devnull+chuan.liu.amlogic.com@kernel.org> wrote:
>>
>>> From: Chuan Liu <chuan.liu@amlogic.com>
>>>
>>> The PLL can only stably lock within a limited frequency range.
>>>
>>> Due to timing constraints, the maximum frequency of the peripheral clock
>>> cannot exceed the design specifications.
>>>
>>> Signed-off-by: Chuan Liu <chuan.liu@amlogic.com>
>>> ---
>>>   drivers/clk/meson/c3-peripherals.c | 21 +++++++++++++++++++++
>>>   drivers/clk/meson/c3-pll.c         |  4 ++++
>>>   2 files changed, 25 insertions(+)
>>>
>>> diff --git a/drivers/clk/meson/c3-peripherals.c b/drivers/clk/meson/c3-peripherals.c
>>> index 7dcbf4ebee07..9f0a3990f0d6 100644
>>> --- a/drivers/clk/meson/c3-peripherals.c
>>> +++ b/drivers/clk/meson/c3-peripherals.c
>>> @@ -568,6 +568,7 @@ static const struct clk_parent_data pwm_parent_data[] = {
>>>                .ops = &clk_regmap_gate_ops,                    \
>>>                .parent_names = (const char *[]) { #_name "_div" },\
>>>                .num_parents = 1,                               \
>>> +             .max_rate = 200000000,                          \
>>>                .flags = CLK_SET_RATE_PARENT,                   \
>>>        },                                                      \
>>>   }
>>> @@ -724,6 +725,7 @@ static struct clk_regmap spicc_a = {
>>>                        &spicc_a_div.hw
>>>                },
>>>                .num_parents = 1,
>>> +             .max_rate = 500000000,
>> I'm sorry but the whole thing is completly wrong.
>>
>> All the clocks I'm seeing here are gates. This type of HW hardly cares
>> what rates it handles. Same goes from mux, dividers, etc ...
>
>
> The purpose of the patch is to constrain the clock network between
> "clk_hw" and "clk_sonsumers". The output source of this clock network
> may come from gate, mux, divider, etc.
>
>
>>
>> All you are doing here is trying enforce some made up "safety" / use-case
>> defined limits that do not belong in the clock controller.
>
>
> Yes, the purpose is also to ensure "safety". From a strict perspective,
> this constraint indeed does not belong to the clock controller. However,
> the source of the potential hazard comes from the clock driver, and we
> have already identified this hazard. Therefore, I think it is better to
> avoid it in the clock driver?
>

No. The clock provider driver describe the how the clock are _provided_,
not how they are meant to used.

>
>>
>> The only piece of HW where limits could possibly make sense are PLL DCO,
>> and even there, you've got multiplier range which is way better as an
>> abstraction.
>
>
> From the perspective of HW, the timing constraints of the clock are for
> the entire clock network with the same name. The output source of this
> clock network may come from PLL, gate, mux, etc. The multiplier range
> of the PLL can also achieve a similar effect. If this approach works,
> we don't need to define the multiplier range for the PLL (PS: Our
> current multiplier range is limited to the case where "n" is not divided).
>
>
>>
>> So it's a nack on the series.
>>
>> If devices are have particular requirement on rate range, have the
>> related driver set it.
>
>
> I think that the clock configuration exceeding the timing constraints
> is a hidden danger that all chips have and face, but this hidden danger
> is not easy to be exposed?
>
> For instance, if the routing of a clock network is close to the clock
> or data bus of other modules, and this clock network is wrongly
> configured to a frequency beyond the constraints, causing crosstalk
> that affects the normal operation of other modules. If such a situation
> occurs, it will be very difficult to troubleshoot. How should this
> situation be handled more reasonably?

Fix your consumers drivers if you need to. Set range if you must.

Those are not clock provider constraints. Those are use-case ones. It
does belong here and CCF already provides the necessary infra to deal
with ranges.

>
>
>>
>>>                .flags = CLK_SET_RATE_PARENT,
>>>        },
>>>   };
>>> @@ -771,6 +773,7 @@ static struct clk_regmap spicc_b = {
>>>                        &spicc_b_div.hw
>>>                },
>>>                .num_parents = 1,
>>> +             .max_rate = 500000000,
>>>                .flags = CLK_SET_RATE_PARENT,
>>>        },
>>>   };
>>> @@ -829,6 +832,7 @@ static struct clk_regmap spifc = {
>>>                        &spifc_div.hw
>>>                },
>>>                .num_parents = 1,
>>> +             .max_rate = 167000000,
>>>                .flags = CLK_SET_RATE_PARENT,
>>>        },
>>>   };
>>> @@ -887,6 +891,7 @@ static struct clk_regmap sd_emmc_a = {
>>>                        &sd_emmc_a_div.hw
>>>                },
>>>                .num_parents = 1,
>>> +             .max_rate = 250000000,
>>>                .flags = CLK_SET_RATE_PARENT,
>>>        },
>>>   };
>>> @@ -934,6 +939,7 @@ static struct clk_regmap sd_emmc_b = {
>>>                        &sd_emmc_b_div.hw
>>>                },
>>>                .num_parents = 1,
>>> +             .max_rate = 250000000,
>>>                .flags = CLK_SET_RATE_PARENT,
>>>        },
>>>   };
>>> @@ -981,6 +987,7 @@ static struct clk_regmap sd_emmc_c = {
>>>                        &sd_emmc_c_div.hw
>>>                },
>>>                .num_parents = 1,
>>> +             .max_rate = 1200000000,
>>>                .flags = CLK_SET_RATE_PARENT,
>>>        },
>>>   };
>>> @@ -1074,6 +1081,7 @@ static struct clk_regmap eth_rmii = {
>>>                        &eth_rmii_div.hw
>>>                },
>>>                .num_parents = 1,
>>> +             .max_rate = 50000000,
>>>                .flags = CLK_SET_RATE_PARENT,
>>>        },
>>>   };
>>> @@ -1132,6 +1140,7 @@ static struct clk_regmap mipi_dsi_meas = {
>>>                        &mipi_dsi_meas_div.hw
>>>                },
>>>                .num_parents = 1,
>>> +             .max_rate = 200000000,
>>>                .flags = CLK_SET_RATE_PARENT,
>>>        },
>>>   };
>>> @@ -1190,6 +1199,7 @@ static struct clk_regmap dsi_phy = {
>>>                        &dsi_phy_div.hw
>>>                },
>>>                .num_parents = 1,
>>> +             .max_rate = 1500000000,
>>>                .flags = CLK_SET_RATE_PARENT,
>>>        },
>>>   };
>>> @@ -1248,6 +1258,7 @@ static struct clk_regmap vout_mclk = {
>>>                        &vout_mclk_div.hw
>>>                },
>>>                .num_parents = 1,
>>> +             .max_rate = 334000000,
>>>                .flags = CLK_SET_RATE_PARENT,
>>>        },
>>>   };
>>> @@ -1306,6 +1317,7 @@ static struct clk_regmap vout_enc = {
>>>                        &vout_enc_div.hw
>>>                },
>>>                .num_parents = 1,
>>> +             .max_rate = 200000000,
>>>                .flags = CLK_SET_RATE_PARENT,
>>>        },
>>>   };
>>> @@ -1431,6 +1443,7 @@ static struct clk_regmap hcodec = {
>>>                .ops = &clk_regmap_mux_ops,
>>>                .parent_data = hcodec_parent_data,
>>>                .num_parents = ARRAY_SIZE(hcodec_parent_data),
>>> +             .max_rate = 667000000,
>>>                .flags = CLK_SET_RATE_PARENT,
>>>        },
>>>   };
>>> @@ -1489,6 +1502,7 @@ static struct clk_regmap vc9000e_aclk = {
>>>                        &vc9000e_aclk_div.hw
>>>                },
>>>                .num_parents = 1,
>>> +             .max_rate = 667000000,
>>>                .flags = CLK_SET_RATE_PARENT,
>>>        },
>>>   };
>>> @@ -1536,6 +1550,7 @@ static struct clk_regmap vc9000e_core = {
>>>                        &vc9000e_core_div.hw
>>>                },
>>>                .num_parents = 1,
>>> +             .max_rate = 400000000,
>>>                .flags = CLK_SET_RATE_PARENT,
>>>        },
>>>   };
>>> @@ -1594,6 +1609,7 @@ static struct clk_regmap csi_phy0 = {
>>>                        &csi_phy0_div.hw
>>>                },
>>>                .num_parents = 1,
>>> +             .max_rate = 200000000,
>>>                .flags = CLK_SET_RATE_PARENT,
>>>        },
>>>   };
>>> @@ -1652,6 +1668,7 @@ static struct clk_regmap dewarpa = {
>>>                        &dewarpa_div.hw
>>>                },
>>>                .num_parents = 1,
>>> +             .max_rate = 800000000,
>>>                .flags = CLK_SET_RATE_PARENT,
>>>        },
>>>   };
>>> @@ -1710,6 +1727,7 @@ static struct clk_regmap isp0 = {
>>>                        &isp0_div.hw
>>>                },
>>>                .num_parents = 1,
>>> +             .max_rate = 400000000,
>>>                .flags = CLK_SET_RATE_PARENT,
>>>        },
>>>   };
>>> @@ -1768,6 +1786,7 @@ static struct clk_regmap nna_core = {
>>>                        &nna_core_div.hw
>>>                },
>>>                .num_parents = 1,
>>> +             .max_rate = 800000000,
>>>                .flags = CLK_SET_RATE_PARENT,
>>>        },
>>>   };
>>> @@ -1826,6 +1845,7 @@ static struct clk_regmap ge2d = {
>>>                        &ge2d_div.hw
>>>                },
>>>                .num_parents = 1,
>>> +             .max_rate = 667000000,
>>>                .flags = CLK_SET_RATE_PARENT,
>>>        },
>>>   };
>>> @@ -1884,6 +1904,7 @@ static struct clk_regmap vapb = {
>>>                        &vapb_div.hw
>>>                },
>>>                .num_parents = 1,
>>> +             .max_rate = 400000000,
>>>                .flags = CLK_SET_RATE_PARENT,
>>>        },
>>>   };
>>> diff --git a/drivers/clk/meson/c3-pll.c b/drivers/clk/meson/c3-pll.c
>>> index 35fda31a19e2..d80d6ee2409d 100644
>>> --- a/drivers/clk/meson/c3-pll.c
>>> +++ b/drivers/clk/meson/c3-pll.c
>>> @@ -286,6 +286,8 @@ static struct clk_regmap gp0_pll_dco = {
>>>                        .fw_name = "top",
>>>                },
>>>                .num_parents = 1,
>>> +             .min_rate = 3000000000,
>>> +             .max_rate = 6000000000,
>>>        },
>>>   };
>>>
>>> @@ -370,6 +372,8 @@ static struct clk_regmap hifi_pll_dco = {
>>>                        .fw_name = "top",
>>>                },
>>>                .num_parents = 1,
>>> +             .min_rate = 3000000000,
>>> +             .max_rate = 6000000000,
>>>        },
>>>   };
>> --
>> Jerome

-- 
Jerome

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic