* [PATCH 0/2] clk: amlogic: Limit the rate boundaries of clk_hw
@ 2025-01-10 11:47 Chuan Liu via B4 Relay
2025-01-10 11:47 ` [PATCH 1/2] clk: Add initialize the rate boundaries of the clk provider Chuan Liu via B4 Relay
2025-01-10 11:47 ` [PATCH 2/2] clk: amlogic: c3: Limit the rate boundaries of clk_hw Chuan Liu via B4 Relay
0 siblings, 2 replies; 9+ messages in thread
From: Chuan Liu via B4 Relay @ 2025-01-10 11:47 UTC (permalink / raw)
To: Michael Turquette, Stephen Boyd, Neil Armstrong, Jerome Brunet,
Kevin Hilman, Martin Blumenstingl
Cc: linux-clk, linux-kernel, linux-amlogic, linux-arm-kernel,
Chuan Liu
The following figure shows a rough hardware connection block diagram
between "clk_hw" and "clk_consumer". The frequency of the clock is
affected by the following two situations:
1. The hardware limitations of "clk_hw" itself. For instance, the
effective output frequency range of some PLL designs is 3G to 6G.
2. The timing constraints for the electrical "line" between "clk_hw" and
"clk_consumer". Timing constraints are applied to each clock network
within the chip, which limits the maximum frequency of the "line".
________ ________________
| | line | |
| clk_hw |-----+---->| clk_consumer 1 |
|________| | |________________|
|
| ...
| ________________
| | |
+---->| clk_consumer n |
|________________|
The frequency boundaries specified for clk_hw in the patch are the
combined frequency boundaries of "clk_hw" and "line", just like the
frequency boundaries obtained after clk_core_get_boundaries() combines
each clks_node.
The min_rate/max_rate of the clk_hw can be described in a less strict
way as follows:
* min_rate = max("clk_hw", "line");
* max_rate = min("clk_hw", "line");
Signed-off-by: Chuan Liu <chuan.liu@amlogic.com>
---
Chuan Liu (2):
clk: Add initialize the rate boundaries of the clk provider
clk: amlogic: c3: Limit the rate boundaries of clk_hw
drivers/clk/clk.c | 4 ++--
drivers/clk/meson/c3-peripherals.c | 21 +++++++++++++++++++++
drivers/clk/meson/c3-pll.c | 4 ++++
include/linux/clk-provider.h | 4 ++++
4 files changed, 31 insertions(+), 2 deletions(-)
---
base-commit: 1e1fd26ed4ca05cc1f0e5857918da4dd54967f7d
change-id: 20250110-limit-rate-range-of-clk-61626c7b6dc5
Best regards,
--
Chuan Liu <chuan.liu@amlogic.com>
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/2] clk: Add initialize the rate boundaries of the clk provider
2025-01-10 11:47 [PATCH 0/2] clk: amlogic: Limit the rate boundaries of clk_hw Chuan Liu via B4 Relay
@ 2025-01-10 11:47 ` Chuan Liu via B4 Relay
2025-01-10 11:47 ` [PATCH 2/2] clk: amlogic: c3: Limit the rate boundaries of clk_hw Chuan Liu via B4 Relay
1 sibling, 0 replies; 9+ messages in thread
From: Chuan Liu via B4 Relay @ 2025-01-10 11:47 UTC (permalink / raw)
To: Michael Turquette, Stephen Boyd, Neil Armstrong, Jerome Brunet,
Kevin Hilman, Martin Blumenstingl
Cc: linux-clk, linux-kernel, linux-amlogic, linux-arm-kernel,
Chuan Liu
From: Chuan Liu <chuan.liu@amlogic.com>
The rate boundaries output by different clk providers vary due to
hardware limitations.
When registering the clk, limit the rate boundaries of the clk provider
to prevent setting the rate of the clk provider beyond the design
specifications and causing abnormal conditions.
Signed-off-by: Chuan Liu <chuan.liu@amlogic.com>
---
drivers/clk/clk.c | 4 ++--
include/linux/clk-provider.h | 4 ++++
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 9b45fa005030..36e4b4f16f1d 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -4359,8 +4359,8 @@ __clk_register(struct device *dev, struct device_node *np, struct clk_hw *hw)
core->hw = hw;
core->flags = init->flags;
core->num_parents = init->num_parents;
- core->min_rate = 0;
- core->max_rate = ULONG_MAX;
+ core->min_rate = init->min_rate ? init->min_rate : 0;
+ core->max_rate = init->max_rate ? init->max_rate : ULONG_MAX;
ret = clk_core_populate_parent_map(core, init);
if (ret)
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 2e6e603b7493..46cfc342819e 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -294,6 +294,8 @@ struct clk_parent_data {
* @parent_hws: array of pointers to all possible parents (when all parents
* are internal to the clk controller)
* @num_parents: number of possible parents
+ * @min_rate: min rate provided by the clk provider
+ * @max_rate: max rate provided by the clk provider
* @flags: framework-level hints and quirks
*/
struct clk_init_data {
@@ -304,6 +306,8 @@ struct clk_init_data {
const struct clk_parent_data *parent_data;
const struct clk_hw **parent_hws;
u8 num_parents;
+ unsigned long min_rate;
+ unsigned long max_rate;
unsigned long flags;
};
--
2.42.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/2] clk: amlogic: c3: Limit the rate boundaries of clk_hw
2025-01-10 11:47 [PATCH 0/2] clk: amlogic: Limit the rate boundaries of clk_hw Chuan Liu via B4 Relay
2025-01-10 11:47 ` [PATCH 1/2] clk: Add initialize the rate boundaries of the clk provider Chuan Liu via B4 Relay
@ 2025-01-10 11:47 ` Chuan Liu via B4 Relay
2025-01-10 13:55 ` Jerome Brunet
1 sibling, 1 reply; 9+ messages in thread
From: Chuan Liu via B4 Relay @ 2025-01-10 11:47 UTC (permalink / raw)
To: Michael Turquette, Stephen Boyd, Neil Armstrong, Jerome Brunet,
Kevin Hilman, Martin Blumenstingl
Cc: linux-clk, linux-kernel, linux-amlogic, linux-arm-kernel,
Chuan Liu
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,
.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,
},
};
--
2.42.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] clk: amlogic: c3: Limit the rate boundaries of clk_hw
2025-01-10 11:47 ` [PATCH 2/2] clk: amlogic: c3: Limit the rate boundaries of clk_hw Chuan Liu via B4 Relay
@ 2025-01-10 13:55 ` Jerome Brunet
2025-01-13 5:24 ` Chuan Liu
0 siblings, 1 reply; 9+ messages in thread
From: Jerome Brunet @ 2025-01-10 13:55 UTC (permalink / raw)
To: Chuan Liu via B4 Relay
Cc: Michael Turquette, Stephen Boyd, Neil Armstrong, Kevin Hilman,
Martin Blumenstingl, chuan.liu, linux-clk, linux-kernel,
linux-amlogic, linux-arm-kernel
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 ...
All you are doing here is trying enforce some made up "safety" / use-case
defined limits that do not belong in the clock controller.
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.
So it's a nack on the series.
If devices are have particular requirement on rate range, have the
related driver set it.
> .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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] clk: amlogic: c3: Limit the rate boundaries of clk_hw
2025-01-10 13:55 ` Jerome Brunet
@ 2025-01-13 5:24 ` Chuan Liu
2025-01-13 14:42 ` Jerome Brunet
0 siblings, 1 reply; 9+ messages in thread
From: Chuan Liu @ 2025-01-13 5:24 UTC (permalink / raw)
To: Jerome Brunet, Chuan Liu via B4 Relay
Cc: Michael Turquette, Stephen Boyd, Neil Armstrong, Kevin Hilman,
Martin Blumenstingl, linux-clk, linux-kernel, linux-amlogic,
linux-arm-kernel
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?
>
> 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?
>
>> .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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] clk: amlogic: c3: Limit the rate boundaries of clk_hw
2025-01-13 5:24 ` Chuan Liu
@ 2025-01-13 14:42 ` Jerome Brunet
2025-01-13 14:46 ` Neil Armstrong
0 siblings, 1 reply; 9+ messages in thread
From: Jerome Brunet @ 2025-01-13 14:42 UTC (permalink / raw)
To: Chuan Liu
Cc: Chuan Liu via B4 Relay, Michael Turquette, Stephen Boyd,
Neil Armstrong, Kevin Hilman, Martin Blumenstingl, linux-clk,
linux-kernel, linux-amlogic, linux-arm-kernel
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 = {
>>> ð_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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] clk: amlogic: c3: Limit the rate boundaries of clk_hw
2025-01-13 14:42 ` Jerome Brunet
@ 2025-01-13 14:46 ` Neil Armstrong
2025-01-13 15:49 ` Jerome Brunet
0 siblings, 1 reply; 9+ messages in thread
From: Neil Armstrong @ 2025-01-13 14:46 UTC (permalink / raw)
To: Jerome Brunet, Chuan Liu
Cc: Chuan Liu via B4 Relay, Michael Turquette, Stephen Boyd,
Kevin Hilman, Martin Blumenstingl, linux-clk, linux-kernel,
linux-amlogic, linux-arm-kernel
On 13/01/2025 15:42, Jerome Brunet wrote:
> 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.
I kind of disagree here, if the vendor has the data and is willing to share
the range for each clock path of the system, I think it should be welcome!
Usually those ranges are not disclosed, so we don't set them, but CCF will
certainly use all those range to make an even better decision on the lock
routing.
Neil
>
>>
>>
>>>
>>>> .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
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] clk: amlogic: c3: Limit the rate boundaries of clk_hw
2025-01-13 14:46 ` Neil Armstrong
@ 2025-01-13 15:49 ` Jerome Brunet
2025-01-14 7:13 ` Chuan Liu
0 siblings, 1 reply; 9+ messages in thread
From: Jerome Brunet @ 2025-01-13 15:49 UTC (permalink / raw)
To: Neil Armstrong
Cc: Chuan Liu, Chuan Liu via B4 Relay, Michael Turquette,
Stephen Boyd, Kevin Hilman, Martin Blumenstingl, linux-clk,
linux-kernel, linux-amlogic, linux-arm-kernel
On Mon 13 Jan 2025 at 15:46, Neil Armstrong <neil.armstrong@linaro.org> wrote:
>>>
>>> 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.
>
> I kind of disagree here, if the vendor has the data and is willing to share
> the range for each clock path of the system, I think it should be welcome!
>
> Usually those ranges are not disclosed, so we don't set them, but CCF will
> certainly use all those range to make an even better decision on the lock
> routing.
I did not say you should not use them, I say that a platform
use-case, which is what this is, should not be hard coded within the
clock provider driver.
This is no different than an assigned-rate, and like any other platform
description, it belong in DT.
We've already seen how these ranges may depend on what else you choose
to do on the system or even what package a particular SoC variant is
using.
>
> Neil
>
>>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] clk: amlogic: c3: Limit the rate boundaries of clk_hw
2025-01-13 15:49 ` Jerome Brunet
@ 2025-01-14 7:13 ` Chuan Liu
0 siblings, 0 replies; 9+ messages in thread
From: Chuan Liu @ 2025-01-14 7:13 UTC (permalink / raw)
To: Jerome Brunet, Neil Armstrong
Cc: Chuan Liu via B4 Relay, Michael Turquette, Stephen Boyd,
Kevin Hilman, Martin Blumenstingl, linux-clk, linux-kernel,
linux-amlogic, linux-arm-kernel
On 1/13/2025 11:49 PM, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
>
> On Mon 13 Jan 2025 at 15:46, Neil Armstrong <neil.armstrong@linaro.org> wrote:
>
>>>> 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.
I don't think it's reliable to have consumers drivers self-regulate.
They are very likely to overlook this constraint. Moreover, when the
clock configuration exceeds the constraint, if their own module can
handle it but it affects other modules, this situation can easily
mislead people to look for the problem in the wrong direction.
Setting the range offers relatively higher fault tolerance, but it
requires adding members to each "clk_regmap_**_data" and implementing
callback functions *init() in the ops of each type of clock (*init()
calls clk_hw_set_rate_range to set the range of the provider). This
seems to complicate the originally simple logic.
>>> 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.
>> I kind of disagree here, if the vendor has the data and is willing to share
>> the range for each clock path of the system, I think it should be welcome!
>>
>> Usually those ranges are not disclosed, so we don't set them, but CCF will
>> certainly use all those range to make an even better decision on the lock
>> routing.
> I did not say you should not use them, I say that a platform
> use-case, which is what this is, should not be hard coded within the
> clock provider driver.
>
> This is no different than an assigned-rate, and like any other platform
> description, it belong in DT.
>
> We've already seen how these ranges may depend on what else you choose
> to do on the system or even what package a particular SoC variant is
> using.
That makes sense. The information I have doesn't make a distinction,
I'm not sure if other manufacturers do.
I think it's more controllable to converge this clock constraint issue
within our clock driver. Should we implement this constraint in
Amlogic's clock driver?
>
>> Neil
>>
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2025-01-14 7:13 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-01-10 11:47 [PATCH 0/2] clk: amlogic: Limit the rate boundaries of clk_hw Chuan Liu via B4 Relay
2025-01-10 11:47 ` [PATCH 1/2] clk: Add initialize the rate boundaries of the clk provider Chuan Liu via B4 Relay
2025-01-10 11:47 ` [PATCH 2/2] clk: amlogic: c3: Limit the rate boundaries of clk_hw Chuan Liu via B4 Relay
2025-01-10 13:55 ` Jerome Brunet
2025-01-13 5:24 ` Chuan Liu
2025-01-13 14:42 ` Jerome Brunet
2025-01-13 14:46 ` Neil Armstrong
2025-01-13 15:49 ` Jerome Brunet
2025-01-14 7:13 ` Chuan Liu
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).