diff mbox series

[v13,2/4] clk: scmi: reject SSC configuration out of the OEM field range

Message ID 20260903153836.373267-3-dario.binacchi@amarulasolutions.com
State New
Headers
Series Support spread spectrum clocking for i.MX8M PLLs |

Commit Message

Dario Binacchi Sept. 3, 2026, 3:32 p.m. UTC 
The i.MX SCMI OEM extension stores the spread in an 8-bit field, in
tenths of a percent, and the modulation frequency in a 16-bit field.

FIELD_PREP() silently truncates values that do not fit in the target
field. Moreover, the conversion from permyriad to tenths of a percent
turns values below 10 permyriad into zero, which is then passed to the
firmware as no spread at all.

Reject these cases with a warning instead of silently programming a
configuration different from the requested one.

Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>

---

Changes in v13:
 - New patch

 drivers/clk/clk-scmi-oem.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

Comments

Peng Fan Sept. 4, 2026, 4:38 a.m. UTC | #1 
On Thu, Sep 03, 2026 at 05:32:14PM +0200, Dario Binacchi wrote:
>The i.MX SCMI OEM extension stores the spread in an 8-bit field, in
>tenths of a percent, and the modulation frequency in a 16-bit field.
>
>FIELD_PREP() silently truncates values that do not fit in the target
>field. Moreover, the conversion from permyriad to tenths of a percent
>turns values below 10 permyriad into zero, which is then passed to the
>firmware as no spread at all.
>
>Reject these cases with a warning instead of silently programming a
>configuration different from the requested one.
>
>Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
>
>---
>
>Changes in v13:
> - New patch
>
> drivers/clk/clk-scmi-oem.c | 15 ++++++++++++++-
> 1 file changed, 14 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/clk/clk-scmi-oem.c b/drivers/clk/clk-scmi-oem.c
>index c1ebbdc6bbc5..4dac608edd4c 100644
>--- a/drivers/clk/clk-scmi-oem.c
>+++ b/drivers/clk/clk-scmi-oem.c
>@@ -35,6 +35,7 @@ scmi_clk_imx_set_spread_spectrum(struct clk_hw *hw,
> 				 const struct clk_spread_spectrum *ss_conf)
> {
> 	struct scmi_clk *clk = to_scmi_clk(hw);
>+	u32 spread_pm = ss_conf->spread_bp / 10;
> 	int ret;
> 	u32 val;
> 
>@@ -44,7 +45,19 @@ scmi_clk_imx_set_spread_spectrum(struct clk_hw *hw,
> 	 * extConfigValue[24]    - Enable/Disable
> 	 * extConfigValue[31:25] - Reserved
> 	 */
>-	val = FIELD_PREP(SCMI_CLOCK_IMX_SS_PERCENTAGE_MASK, ss_conf->spread_bp / 10);
>+	if (!spread_pm || spread_pm > FIELD_MAX(SCMI_CLOCK_IMX_SS_PERCENTAGE_MASK)) {
>+		dev_warn(clk->dev, "%s: spread (%u permyriad) out of range\n",
>+			 clk_hw_get_name(hw), ss_conf->spread_bp);
>+		return -EINVAL;

sashiko's comments is valid.

And patch 1 & 3 have fixes tag. Patch 2 and 4 are new patches.
If you would like patch 1 & 3 to be accepted in this 7.13, better post them
as separate patchset. Then CLK maintainer may pick them up for this release.

Regards
Peng

>+	}
>+
>+	if (ss_conf->modfreq_hz > FIELD_MAX(SCMI_CLOCK_IMX_SS_MOD_FREQ_MASK)) {
>+		dev_warn(clk->dev, "%s: modulation frequency (%u Hz) out of range\n",
>+			 clk_hw_get_name(hw), ss_conf->modfreq_hz);
>+		return -EINVAL;
>+	}
>+
>+	val = FIELD_PREP(SCMI_CLOCK_IMX_SS_PERCENTAGE_MASK, spread_pm);
> 	val |= FIELD_PREP(SCMI_CLOCK_IMX_SS_MOD_FREQ_MASK, ss_conf->modfreq_hz);
> 	if (ss_conf->method != CLK_SPREAD_NO)
> 		val |= SCMI_CLOCK_IMX_SS_ENABLE_MASK;
>-- 
>2.43.0
>
>

To unsubscribe from this group and stop receiving emails from it, send an email to linux-amarula+unsubscribe@amarulasolutions.com.
Dario Binacchi Sept. 4, 2026, 10:03 a.m. UTC | #2 
Hi Peng,

On Fri, Sep 4, 2026 at 6:34 AM Peng Fan <peng.fan@oss.nxp.com> wrote:
>
> On Thu, Sep 03, 2026 at 05:32:14PM +0200, Dario Binacchi wrote:
> >The i.MX SCMI OEM extension stores the spread in an 8-bit field, in
> >tenths of a percent, and the modulation frequency in a 16-bit field.
> >
> >FIELD_PREP() silently truncates values that do not fit in the target
> >field. Moreover, the conversion from permyriad to tenths of a percent
> >turns values below 10 permyriad into zero, which is then passed to the
> >firmware as no spread at all.
> >
> >Reject these cases with a warning instead of silently programming a
> >configuration different from the requested one.
> >
> >Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
> >
> >---
> >
> >Changes in v13:
> > - New patch
> >
> > drivers/clk/clk-scmi-oem.c | 15 ++++++++++++++-
> > 1 file changed, 14 insertions(+), 1 deletion(-)
> >
> >diff --git a/drivers/clk/clk-scmi-oem.c b/drivers/clk/clk-scmi-oem.c
> >index c1ebbdc6bbc5..4dac608edd4c 100644
> >--- a/drivers/clk/clk-scmi-oem.c
> >+++ b/drivers/clk/clk-scmi-oem.c
> >@@ -35,6 +35,7 @@ scmi_clk_imx_set_spread_spectrum(struct clk_hw *hw,
> >                                const struct clk_spread_spectrum *ss_conf)
> > {
> >       struct scmi_clk *clk = to_scmi_clk(hw);
> >+      u32 spread_pm = ss_conf->spread_bp / 10;
> >       int ret;
> >       u32 val;
> >
> >@@ -44,7 +45,19 @@ scmi_clk_imx_set_spread_spectrum(struct clk_hw *hw,
> >        * extConfigValue[24]    - Enable/Disable
> >        * extConfigValue[31:25] - Reserved
> >        */
> >-      val = FIELD_PREP(SCMI_CLOCK_IMX_SS_PERCENTAGE_MASK, ss_conf->spread_bp / 10);
> >+      if (!spread_pm || spread_pm > FIELD_MAX(SCMI_CLOCK_IMX_SS_PERCENTAGE_MASK)) {
> >+              dev_warn(clk->dev, "%s: spread (%u permyriad) out of range\n",
> >+                       clk_hw_get_name(hw), ss_conf->spread_bp);
> >+              return -EINVAL;
>
> sashiko's comments is valid.
>
> And patch 1 & 3 have fixes tag. Patch 2 and 4 are new patches.
> If you would like patch 1 & 3 to be accepted in this 7.13, better post them
> as separate patchset. Then CLK maintainer may pick them up for this release.

I'd rather keep a single series, since patch 2 depends on patch 1.
In v14 the two fixes come first so they can be picked up separately.

Sashiko's comment is addressed in v14.

Thanks and regards,
Dario

>
> Regards
> Peng
>
> >+      }
> >+
> >+      if (ss_conf->modfreq_hz > FIELD_MAX(SCMI_CLOCK_IMX_SS_MOD_FREQ_MASK)) {
> >+              dev_warn(clk->dev, "%s: modulation frequency (%u Hz) out of range\n",
> >+                       clk_hw_get_name(hw), ss_conf->modfreq_hz);
> >+              return -EINVAL;
> >+      }
> >+
> >+      val = FIELD_PREP(SCMI_CLOCK_IMX_SS_PERCENTAGE_MASK, spread_pm);
> >       val |= FIELD_PREP(SCMI_CLOCK_IMX_SS_MOD_FREQ_MASK, ss_conf->modfreq_hz);
> >       if (ss_conf->method != CLK_SPREAD_NO)
> >               val |= SCMI_CLOCK_IMX_SS_ENABLE_MASK;
> >--
> >2.43.0
> >
> >
diff mbox series

Patch

diff --git a/drivers/clk/clk-scmi-oem.c b/drivers/clk/clk-scmi-oem.c
index c1ebbdc6bbc5..4dac608edd4c 100644
--- a/drivers/clk/clk-scmi-oem.c
+++ b/drivers/clk/clk-scmi-oem.c
@@ -35,6 +35,7 @@  scmi_clk_imx_set_spread_spectrum(struct clk_hw *hw,
 				 const struct clk_spread_spectrum *ss_conf)
 {
 	struct scmi_clk *clk = to_scmi_clk(hw);
+	u32 spread_pm = ss_conf->spread_bp / 10;
 	int ret;
 	u32 val;
 
@@ -44,7 +45,19 @@  scmi_clk_imx_set_spread_spectrum(struct clk_hw *hw,
 	 * extConfigValue[24]    - Enable/Disable
 	 * extConfigValue[31:25] - Reserved
 	 */
-	val = FIELD_PREP(SCMI_CLOCK_IMX_SS_PERCENTAGE_MASK, ss_conf->spread_bp / 10);
+	if (!spread_pm || spread_pm > FIELD_MAX(SCMI_CLOCK_IMX_SS_PERCENTAGE_MASK)) {
+		dev_warn(clk->dev, "%s: spread (%u permyriad) out of range\n",
+			 clk_hw_get_name(hw), ss_conf->spread_bp);
+		return -EINVAL;
+	}
+
+	if (ss_conf->modfreq_hz > FIELD_MAX(SCMI_CLOCK_IMX_SS_MOD_FREQ_MASK)) {
+		dev_warn(clk->dev, "%s: modulation frequency (%u Hz) out of range\n",
+			 clk_hw_get_name(hw), ss_conf->modfreq_hz);
+		return -EINVAL;
+	}
+
+	val = FIELD_PREP(SCMI_CLOCK_IMX_SS_PERCENTAGE_MASK, spread_pm);
 	val |= FIELD_PREP(SCMI_CLOCK_IMX_SS_MOD_FREQ_MASK, ss_conf->modfreq_hz);
 	if (ss_conf->method != CLK_SPREAD_NO)
 		val |= SCMI_CLOCK_IMX_SS_ENABLE_MASK;