[PATCH 0/2] Add support for Si549 programmable clock

[PATCH 0/2] Add support for Si549 programmable clock

[Pavel Löbl]

This adds support for Si549 programmable oscillator. It's almost the
same as already supported Si544, except it uses different internal
oscillator frequency.

So new compatible strings are added, and driver data is extended
to carry both maximum output frequency and internal xtal frequency.


Pavel Löbl (2):
  dt-bindings: clock: si544: add si549 compatible
  clk: si544: add support for si549

 .../bindings/clock/silabs,si544.yaml          |   8 +-
 drivers/clk/Kconfig                           |   6 +-
 drivers/clk/clk-si544.c                       | 100 +++++++++++++-----
 3 files changed, 84 insertions(+), 30 deletions(-)

-- 
2.53.0

[PATCH 1/2] dt-bindings: clock: si544: add si549 compatible

[Pavel Löbl]

Adding Si549 compatible, which uses different internal oscillator
frequency. Like in Si544 case, there are three types ending with
latters a,b and c. Each with different maximum output frequency.

Also fix datasheet links after Silicon Labs acquisition by
Skyworks Solutions.

Signed-off-by: Pavel Löbl <pavel@loebl.cz>
---
 Documentation/devicetree/bindings/clock/silabs,si544.yaml | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/clock/silabs,si544.yaml b/Documentation/devicetree/bindings/clock/silabs,si544.yaml
index f87e71867108..680bc71afd1e 100644
--- a/Documentation/devicetree/bindings/clock/silabs,si544.yaml
+++ b/Documentation/devicetree/bindings/clock/silabs,si544.yaml
@@ -13,8 +13,9 @@ description: >
   Silicon Labs 514/544 programmable I2C clock generator. Details about the device
   can be found in the datasheet:
 
-    https://www.silabs.com/Support%20Documents/TechnicalDocs/si514.pdf
-    https://www.silabs.com/documents/public/data-sheets/si544-datasheet.pdf
+    https://www.skyworksinc.com/-/media/Skyworks/SL/documents/public/data-sheets/Si514.pdf
+    https://www.skyworksinc.com/-/media/SkyWorks/SL/documents/public/data-sheets/si544-datasheet.pdf
+    https://www.skyworksinc.com/-/media/Skyworks/SL/documents/public/data-sheets/si549-datasheet.pdf
 
 properties:
   compatible:
@@ -23,6 +24,9 @@ properties:
       - silabs,si544a
       - silabs,si544b
       - silabs,si544c
+      - silabs,si549a
+      - silabs,si549b
+      - silabs,si549c
 
   reg:
     maxItems: 1
-- 
2.53.0

Re: [PATCH 1/2] dt-bindings: clock: si544: add si549 compatible

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] The updated datasheet URL for Si544 uses 'SkyWorks' with a capital 'W', which is inconsistent with the other URLs and may result in a broken link.
- [Low] The dt-binding title and description were not updated to include the newly added Si549 variants.
--

commit 4f4bdd2bd1e69ffb4c086f281e4a07d3aa408d53
Author: Pavel Löbl <pavel@loebl.cz>
Subject: dt-bindings: clock: si544: add si549 compatible

This commit adds the Si549 compatible strings to the silabs,si544 clock
binding. It also updates the datasheet URLs to point to Skyworks Solutions
following their acquisition of Silicon Labs.

> diff --git a/Documentation/devicetree/bindings/clock/silabs,si544.yaml b/Documentation/devicetree/bindings/clock/silabs,si544.yaml
> index f87e718671080..680bc71afd1e2 100644
> --- a/Documentation/devicetree/bindings/clock/silabs,si544.yaml
> +++ b/Documentation/devicetree/bindings/clock/silabs,si544.yaml
> @@ -13,8 +13,9 @@ description: >
>    Silicon Labs 514/544 programmable I2C clock generator. Details about the device

[Severity: Low]
Do the title and description in this document need to be updated to
include the newly added Si549 variants?

>    can be found in the datasheet:
>  
> -    https://www.silabs.com/Support%20Documents/TechnicalDocs/si514.pdf
> -    https://www.silabs.com/documents/public/data-sheets/si544-datasheet.pdf
> +    https://www.skyworksinc.com/-/media/Skyworks/SL/documents/public/data-sheets/Si514.pdf
> +    https://www.skyworksinc.com/-/media/SkyWorks/SL/documents/public/data-sheets/si544-datasheet.pdf

[Severity: Low]
Is the capitalized SkyWorks intentional in this URL?

It is inconsistent with the lowercase Skyworks usage in the other URLs on
adjacent lines. This might lead to a broken link if the web server is
case-sensitive.

> +    https://www.skyworksinc.com/-/media/Skyworks/SL/documents/public/data-sheets/si549-datasheet.pdf

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260701130927.3858536-1-pavel@loebl.cz?part=1

[PATCH 2/2] clk: si544: add support for si549

[Pavel Löbl]

Add support for Si549, which only differs from Si544 in internal
oscilator frequency. Si549 also comes in three types A,B and C.
Each having different maximum output frequency.

This patch also fixes maximum frequency of Si544 C. Which should
be 325MHz according to datasheet.

Signed-off-by: Pavel Löbl <pavel@loebl.cz>
---
 drivers/clk/Kconfig     |   6 +--
 drivers/clk/clk-si544.c | 100 ++++++++++++++++++++++++++++++----------
 2 files changed, 78 insertions(+), 28 deletions(-)

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 1717ce75a907..159f546a48f0 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -155,12 +155,12 @@ config COMMON_CLK_SI514
 	  generator.
 
 config COMMON_CLK_SI544
-	tristate "Clock driver for SiLabs 544 devices"
+	tristate "Clock driver for SiLabs 544 and compatible devices"
 	depends on I2C
 	select REGMAP_I2C
 	help
-	  This driver supports the Silicon Labs 544 programmable clock
-	  generator.
+	  This driver supports the Silicon Labs 544/549 programmable clock
+	  generators.
 
 config COMMON_CLK_SI570
 	tristate "Clock driver for SiLabs 570 and compatible devices"
diff --git a/drivers/clk/clk-si544.c b/drivers/clk/clk-si544.c
index 09c06ecec1a5..2643546eb940 100644
--- a/drivers/clk/clk-si544.c
+++ b/drivers/clk/clk-si544.c
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 /*
- * Driver for Silicon Labs Si544 Programmable Oscillator
+ * Driver for Silicon Labs Si544/Si549 Programmable Oscillator
  * Copyright (C) 2018 Topic Embedded Products
  * Author: Mike Looijmans <mike.looijmans@topic.nl>
  */
@@ -40,7 +40,9 @@
 #define SI544_MIN_FREQ	    200000U
 
 /* Si544 Internal oscillator runs at 55.05 MHz */
-#define FXO		  55050000U
+#define SI544_XO_FREQ	55050000U
+/* Si549 Internal oscilator runs at 152.60 MHz */
+#define SI549_XO_FREQ	152600000U
 
 /* VCO range is 10.8 .. 12.1 GHz, max depends on speed grade */
 #define FVCO_MIN       10800000000ULL
@@ -56,11 +58,17 @@
 #define DELTA_M_FRAC_NUM	19
 #define DELTA_M_FRAC_DEN	20000
 
+struct si544_clk_desc {
+	unsigned long max_freq;
+	unsigned long xo_freq;
+};
+
 struct clk_si544 {
+	const struct si544_clk_desc *desc;
 	struct clk_hw hw;
 	struct regmap *regmap;
 	struct i2c_client *i2c_client;
-	unsigned long  max_freq;
+	const struct si544_clk_desc *chip_info;
 };
 #define to_clk_si544(_hw)	container_of(_hw, struct clk_si544, hw)
 
@@ -79,6 +87,7 @@ struct clk_si544_muldiv {
 	u16 hs_div;
 	u8 ls_div_bits;
 	s32 delta_m;
+	u32 xo_freq;
 };
 
 /* Enables or disables the output driver */
@@ -145,6 +154,8 @@ static int si544_get_muldiv(struct clk_si544 *data,
 	settings->delta_m = reg[0] << 8 | reg[1] << 16 | reg[2] << 24;
 	settings->delta_m >>= 8;
 
+	settings->xo_freq = data->chip_info->xo_freq;
+
 	return 0;
 }
 
@@ -193,14 +204,15 @@ static bool is_valid_frequency(const struct clk_si544 *data,
 	if (frequency < SI544_MIN_FREQ)
 		return false;
 
-	return frequency <= data->max_freq;
+	return frequency <= data->chip_info->max_freq;
 }
 
 /* Calculate divider settings for a given frequency */
-static int si544_calc_muldiv(struct clk_si544_muldiv *settings,
-	unsigned long frequency)
+static int si544_calc_muldiv(const struct clk_si544 *data,
+	struct clk_si544_muldiv *settings, unsigned long frequency)
 {
 	u64 vco;
+	u32 fxo = settings->xo_freq;
 	u32 ls_freq;
 	u32 tmp;
 	u8 res;
@@ -238,13 +250,13 @@ static int si544_calc_muldiv(struct clk_si544_muldiv *settings,
 	vco = (u64)ls_freq * settings->hs_div;
 
 	/* Calculate the integer part of the feedback divider */
-	tmp = do_div(vco, FXO);
+	tmp = do_div(vco, fxo);
 	settings->fb_div_int = vco;
 
 	/* And the fractional bits using the remainder */
 	vco = (u64)tmp << 32;
-	vco += FXO / 2; /* Round to nearest multiple */
-	do_div(vco, FXO);
+	vco += fxo / 2; /* Round to nearest multiple */
+	do_div(vco, fxo);
 	settings->fb_div_frac = vco;
 
 	/* Reset the frequency adjustment */
@@ -254,19 +266,20 @@ static int si544_calc_muldiv(struct clk_si544_muldiv *settings,
 }
 
 /* Calculate resulting frequency given the register settings */
-static unsigned long si544_calc_center_rate(
+static unsigned long si544_calc_center_rate(const struct clk_si544 *data,
 		const struct clk_si544_muldiv *settings)
 {
 	u32 d = settings->hs_div * BIT(settings->ls_div_bits);
+	u32 fxo = settings->xo_freq;
 	u64 vco;
 
 	/* Calculate VCO from the fractional part */
-	vco = (u64)settings->fb_div_frac * FXO;
-	vco += (FXO / 2);
+	vco = (u64)settings->fb_div_frac * fxo;
+	vco += (fxo / 2);
 	vco >>= 32;
 
 	/* Add the integer part of the VCO frequency */
-	vco += (u64)settings->fb_div_int * FXO;
+	vco += (u64)settings->fb_div_int * fxo;
 
 	/* Apply divider to obtain the generated frequency */
 	do_div(vco, d);
@@ -274,9 +287,10 @@ static unsigned long si544_calc_center_rate(
 	return vco;
 }
 
-static unsigned long si544_calc_rate(const struct clk_si544_muldiv *settings)
+static unsigned long si544_calc_rate(const struct clk_si544 *data,
+				const struct clk_si544_muldiv *settings)
 {
-	unsigned long rate = si544_calc_center_rate(settings);
+	unsigned long rate = si544_calc_center_rate(data, settings);
 	s64 delta = (s64)rate * (DELTA_M_FRAC_NUM * settings->delta_m);
 
 	/*
@@ -304,7 +318,7 @@ static unsigned long si544_recalc_rate(struct clk_hw *hw,
 	if (err)
 		return 0;
 
-	return si544_calc_rate(&settings);
+	return si544_calc_rate(data, &settings);
 }
 
 static int si544_determine_rate(struct clk_hw *hw,
@@ -356,7 +370,7 @@ static int si544_set_rate(struct clk_hw *hw, unsigned long rate,
 	if (err)
 		return err;
 
-	center = si544_calc_center_rate(&settings);
+	center = si544_calc_center_rate(data, &settings);
 	max_delta = si544_max_delta(center);
 	delta = rate - center;
 
@@ -365,7 +379,7 @@ static int si544_set_rate(struct clk_hw *hw, unsigned long rate,
 					 si544_calc_delta(delta, max_delta));
 
 	/* Too big for the delta adjustment, need to reprogram */
-	err = si544_calc_muldiv(&settings, rate);
+	err = si544_calc_muldiv(data, &settings, rate);
 	if (err)
 		return err;
 
@@ -446,7 +460,7 @@ static int si544_probe(struct i2c_client *client)
 	init.num_parents = 0;
 	data->hw.init = &init;
 	data->i2c_client = client;
-	data->max_freq = (uintptr_t)i2c_get_match_data(client);
+	data->chip_info = i2c_get_match_data(client);
 
 	if (of_property_read_string(client->dev.of_node, "clock-output-names",
 			&init.name))
@@ -478,18 +492,54 @@ static int si544_probe(struct i2c_client *client)
 	return 0;
 }
 
+static const struct si544_clk_desc clk_si544a_info = {
+	.xo_freq = SI544_XO_FREQ,
+	.max_freq = 1500000000,
+};
+
+static const struct si544_clk_desc clk_si544b_info = {
+	.xo_freq = SI544_XO_FREQ,
+	.max_freq = 800000000,
+};
+
+static const struct si544_clk_desc clk_si544c_info = {
+	.xo_freq = SI544_XO_FREQ,
+	.max_freq = 325000000,
+};
+
+static const struct si544_clk_desc clk_si549a_info = {
+	.xo_freq = SI549_XO_FREQ,
+	.max_freq = 1500000000,
+};
+
+static const struct si544_clk_desc clk_si549b_info = {
+	.xo_freq = SI549_XO_FREQ,
+	.max_freq = 800000000,
+};
+
+static const struct si544_clk_desc clk_si549c_info = {
+	.xo_freq = SI549_XO_FREQ,
+	.max_freq = 325000000,
+};
+
 static const struct i2c_device_id si544_id[] = {
-	{ "si544a", 1500000000 },
-	{ "si544b", 800000000 },
-	{ "si544c", 350000000 },
+	{ "si544a", (kernel_ulong_t)&clk_si544a_info },
+	{ "si544b", (kernel_ulong_t)&clk_si544b_info },
+	{ "si544c", (kernel_ulong_t)&clk_si544c_info },
+	{ "si549a", (kernel_ulong_t)&clk_si549a_info },
+	{ "si549b", (kernel_ulong_t)&clk_si549b_info },
+	{ "si549c", (kernel_ulong_t)&clk_si549c_info },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, si544_id);
 
 static const struct of_device_id clk_si544_of_match[] = {
-	{ .compatible = "silabs,si544a", .data = (void *)1500000000 },
-	{ .compatible = "silabs,si544b", .data = (void *)800000000 },
-	{ .compatible = "silabs,si544c", .data = (void *)350000000 },
+	{ .compatible = "silabs,si544a", .data = &clk_si544a_info },
+	{ .compatible = "silabs,si544b", .data = &clk_si544b_info },
+	{ .compatible = "silabs,si544c", .data = &clk_si544c_info },
+	{ .compatible = "silabs,si549a", .data = &clk_si549a_info },
+	{ .compatible = "silabs,si549b", .data = &clk_si549b_info },
+	{ .compatible = "silabs,si549c", .data = &clk_si549c_info },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, clk_si544_of_match);
-- 
2.53.0

Re: [PATCH 2/2] clk: si544: add support for si549

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [High] Missing validation of `i2c_get_match_data()` result leads to a NULL pointer dereference.
- [Low] Unused 'desc' field added to 'struct clk_si544'.
- [Low] Calculation functions take an unused 'data' parameter but rely on caller-initialized output structs.

Pre-existing issues:
- [High] Unconditional dereference of `client->dev.of_node` if Device Tree is not used.
- [High] Unvalidated hardware register reads can lead to a hardware divide-by-zero exception.
--

commit cd7f9cb7be2c12aa433f045f924c907e1d3f9ecc
Author: Pavel Löbl <pavel@loebl.cz>

clk: si544: add support for si549

This commit adds support for the Si549 programmable clock generator,
which differs from the Si544 in its internal oscillator frequency. It
also adds support for three speed grade variants and fixes the maximum
frequency limit for the Si544 C variant.

> diff --git a/drivers/clk/clk-si544.c b/drivers/clk/clk-si544.c
> index 09c06ecec1a52..2643546eb9405 100644
> --- a/drivers/clk/clk-si544.c
> +++ b/drivers/clk/clk-si544.c
> @@ -56,11 +58,17 @@
>  #define DELTA_M_FRAC_NUM	19
>  #define DELTA_M_FRAC_DEN	20000
>  
> +struct si544_clk_desc {
> +	unsigned long max_freq;
> +	unsigned long xo_freq;
> +};
> +
>  struct clk_si544 {
> +	const struct si544_clk_desc *desc;

[Severity: Low]
Is this desc field needed? It appears to be declared but never populated
or accessed anywhere in the driver, while chip_info is used instead.

>  	struct clk_hw hw;
>  	struct regmap *regmap;
>  	struct i2c_client *i2c_client;
> -	unsigned long  max_freq;
> +	const struct si544_clk_desc *chip_info;
>  };
>  #define to_clk_si544(_hw)	container_of(_hw, struct clk_si544, hw)

[ ... ]

> @@ -193,14 +204,15 @@ static bool is_valid_frequency(const struct clk_si544 *data,
>  	if (frequency < SI544_MIN_FREQ)
>  		return false;
>  
> -	return frequency <= data->max_freq;
> +	return frequency <= data->chip_info->max_freq;
>  }
>  
>  /* Calculate divider settings for a given frequency */
> -static int si544_calc_muldiv(struct clk_si544_muldiv *settings,
> -	unsigned long frequency)
> +static int si544_calc_muldiv(const struct clk_si544 *data,
> +	struct clk_si544_muldiv *settings, unsigned long frequency)
>  {
>  	u64 vco;
> +	u32 fxo = settings->xo_freq;

[Severity: Low]
Does this code need to read xo_freq from the settings output struct rather
than utilizing the explicitly provided data context pointer? The data
parameter seems completely unused in this function.

>  	u32 ls_freq;
>  	u32 tmp;
>  	u8 res;

[ ... ]

> @@ -254,19 +266,20 @@ static int si544_calc_muldiv(struct clk_si544_muldiv *settings,
>  }
>  
>  /* Calculate resulting frequency given the register settings */
> -static unsigned long si544_calc_center_rate(
> +static unsigned long si544_calc_center_rate(const struct clk_si544 *data,
>  		const struct clk_si544_muldiv *settings)
>  {
>  	u32 d = settings->hs_div * BIT(settings->ls_div_bits);
> +	u32 fxo = settings->xo_freq;
>  	u64 vco;
>  
>  	/* Calculate VCO from the fractional part */
> -	vco = (u64)settings->fb_div_frac * FXO;
> -	vco += (FXO / 2);
> +	vco = (u64)settings->fb_div_frac * fxo;
> +	vco += (fxo / 2);
>  	vco >>= 32;
>  
>  	/* Add the integer part of the VCO frequency */
> -	vco += (u64)settings->fb_div_int * FXO;
> +	vco += (u64)settings->fb_div_int * fxo;
>  
>  	/* Apply divider to obtain the generated frequency */
>  	do_div(vco, d);

[Severity: High]
This is a pre-existing issue, but could this do_div() trigger a hardware
divide-by-zero exception?

If the hardware is faulty or unresponsive, si544_get_muldiv() might read
0 for SI544_REG_HS_DIV from I2C without validating it. This would cause
settings->hs_div to evaluate to 0, which then results in d being 0 here.

>  	return vco;
>  }

[ ... ]

> @@ -446,7 +460,7 @@ static int si544_probe(struct i2c_client *client)
>  	init.num_parents = 0;
>  	data->hw.init = &init;
>  	data->i2c_client = client;
> -	data->max_freq = (uintptr_t)i2c_get_match_data(client);
> +	data->chip_info = i2c_get_match_data(client);

[Severity: High]
Can i2c_get_match_data() return NULL here if the device is instantiated via
sysfs new_device with a generic fallback name or driver_override?

If so, storing NULL in data->chip_info could lead to a NULL pointer
dereference, as it is dereferenced unconditionally in si544_get_muldiv():

drivers/clk/clk-si544.c:si544_get_muldiv() {
    ...
    settings->xo_freq = data->chip_info->xo_freq;
    ...
}

Should the return value of i2c_get_match_data() be validated?

>  
>  	if (of_property_read_string(client->dev.of_node, "clock-output-names",
>  			&init.name))

[Severity: High]
This is a pre-existing issue, but does this fallback path unconditionally
dereference client->dev.of_node if Device Tree is not used?

If the device is instantiated on a non-DT system (e.g. via I2C board info,
ACPI, or sysfs instantiation), client->dev.of_node will be NULL.
of_property_read_string() will return an error, causing the fallback path
to dereference client->dev.of_node->name unconditionally:

drivers/clk/clk-si544.c:si544_probe() {
    ...
    if (of_property_read_string(client->dev.of_node, "clock-output-names",
            &init.name))
        init.name = client->dev.of_node->name;
    ...
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260701130927.3858536-1-pavel@loebl.cz?part=2