* [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>
_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic
^ 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 _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [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 _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ 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 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 _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ 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 _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ 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 _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ 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 > _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ 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 > >> _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ 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 >> _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2025-01-14 7:14 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
Powered by JetHome