| 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
| ||
|
Message-ID: <1jr05693nn.fsf@starbuckisacylon.baylibre.com>
Date: Mon, 13 Jan 2025 15:42:04 +0100
From: Jerome Brunet <jbrunet@...libre.com>
To: Chuan Liu <chuan.liu@...ogic.com>
Cc: Chuan Liu via B4 Relay <devnull+chuan.liu.amlogic.com@...nel.org>,
Michael Turquette <mturquette@...libre.com>, Stephen Boyd
<sboyd@...nel.org>, Neil Armstrong <neil.armstrong@...aro.org>, Kevin
Hilman <khilman@...libre.com>, Martin Blumenstingl
<martin.blumenstingl@...glemail.com>, linux-clk@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-amlogic@...ts.infradead.org,
linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 2/2] clk: amlogic: c3: Limit the rate boundaries of clk_hw
On Mon 13 Jan 2025 at 13:24, Chuan Liu <chuan.liu@...ogic.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@...nel.org> wrote:
>>
>>> From: Chuan Liu <chuan.liu@...ogic.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@...ogic.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 = {
>>> ð_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
Powered by blists - more mailing lists