Messages in this thread |  | | From | Jerome Brunet <> | | Subject | Re: [PATCH 2/2] clk: amlogic: c3: Limit the rate boundaries of clk_hw | | Date | Mon, 13 Jan 2025 15:42:04 +0100 |
| |
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
|  |