| Message ID | 20250110-limit-rate-range-of-clk-v1-2-dd618adc4aa8@amlogic.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Neil Armstrong |
| Headers | show |
| Series | clk: amlogic: Limit the rate boundaries of clk_hw | expand |
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, > }, > };
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
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
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 >
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 > >>
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 >>
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, }, };