i2c: spacemit: improve clock handling and cleanups

[PATCH v5 2/3] i2c: spacemit: configure ILCR for accurate SCL frequency

Posted by Troy Mitchell 1 month, 2 weeks ago

The SpacemiT I2C controller's SCL (Serial Clock Line) frequency for
master mode operations is determined by the ILCR (I2C Load Count Register).
Previously, the driver relied on the hardware's reset default
values for this register.

The hardware's default ILCR values (SLV=0x156, FLV=0x5d) yield SCL
frequencies lower than intended. For example, with the default
31.5 MHz input clock, these default settings result in an SCL
frequency of approximately 93 kHz (standard mode) when targeting 100 kHz,
and approximately 338 kHz (fast mode) when targeting 400 kHz.
These frequencies are below the 100 kHz/400 kHz nominal speeds.

This patch integrates the SCL frequency management into
the Common Clock Framework (CCF). Specifically, the ILCR register,
which acts as a frequency divider for the SCL clock, is now registered
as a managed clock (scl_clk) within the CCF.

This patch also cleans up unnecessary whitespace
in the included header files.

Reviewed-by: Yixun Lan <dlan@gentoo.org>
Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
---
Changelog in v5:
- use __ffs() instead of *_SHIFT
- remove useless *_SHIFT
- check return value when scl clk name array is truncated
- rebase to v6.19-rc1
- Link to v3: https://lore.kernel.org/all/20251017-k1-i2c-ilcr-v4-1-eed4903ecdb9@linux.spacemit.com/

Changelog in v4:
- initialize clk_init_data with {} so that init.flags is implicitly set to 0
- minor cleanup and style fixes for better readability
- remove unused spacemit_i2c_scl_clk_exclusive_put() cleanup callback
- replace clk_set_rate_exclusive()/clk_rate_exclusive_put() pair with clk_set_rate()
- simplify LCR LV field macros by using FIELD_GET/FIELD_MAX helpers
- Link to v3: https://lore.kernel.org/all/20250814-k1-i2c-ilcr-v3-1-317723e74bcd@linux.spacemit.com/

Changelog in v3:
- use MASK macro in `recalc_rate` function
- rename clock name
- Link to v2: https://lore.kernel.org/r/20250718-k1-i2c-ilcr-v2-1-b4c68f13dcb1@linux.spacemit.com

Changelog in v2:
- Align line breaks.
- Check `lv` in `clk_set_rate` function.
- Force fast mode when SCL frequency is illegal or unavailable.
- Change "linux/bits.h" to <linux/bits.h>
- Kconfig: Add dependency on CCF.
- Link to v1: https://lore.kernel.org/all/20250710-k1-i2c-ilcr-v1-1-188d1f460c7d@linux.spacemit.com/
---
 drivers/i2c/busses/Kconfig  |   2 +-
 drivers/i2c/busses/i2c-k1.c | 146 +++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 139 insertions(+), 9 deletions(-)

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index cea87fcb4a1a945f805e9eff25d2f7ab9f61ddf9..b13fb7155786c246229f5e17aa57603c5e755683 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -799,7 +799,7 @@ config I2C_JZ4780
 config I2C_K1
 	tristate "SpacemiT K1 I2C adapter"
 	depends on ARCH_SPACEMIT || COMPILE_TEST
-	depends on OF
+	depends on OF && COMMON_CLK
 	help
 	  This option enables support for the I2C interface on the SpacemiT K1
 	  platform.
diff --git a/drivers/i2c/busses/i2c-k1.c b/drivers/i2c/busses/i2c-k1.c
index 30cdf733deef264061c8ea565634c5a17f5aebfd..f0c35e23f4f2e139da0d09f314f3eb0e0462a382 100644
--- a/drivers/i2c/busses/i2c-k1.c
+++ b/drivers/i2c/busses/i2c-k1.c
@@ -4,7 +4,9 @@
  */
 
 #include <linux/bitfield.h>
+#include <linux/bits.h>
 #include <linux/clk.h>
+#include <linux/clk-provider.h>
 #include <linux/i2c.h>
 #include <linux/iopoll.h>
 #include <linux/module.h>
@@ -16,6 +18,7 @@
 #define SPACEMIT_ISR		 0x4		/* Status register */
 #define SPACEMIT_IDBR		 0xc		/* Data buffer register */
 #define SPACEMIT_IRCR		 0x18		/* Reset cycle counter */
+#define SPACEMIT_ILCR		 0x10		/* Load Count Register */
 #define SPACEMIT_IBMR		 0x1c		/* Bus monitor register */
 
 /* SPACEMIT_ICR register fields */
@@ -87,6 +90,11 @@
 #define SPACEMIT_BMR_SDA         BIT(0)		/* SDA line level */
 #define SPACEMIT_BMR_SCL         BIT(1)		/* SCL line level */
 
+#define SPACEMIT_LCR_LV_STANDARD_MASK		GENMASK(8, 0)
+#define SPACEMIT_LCR_LV_FAST_MASK		GENMASK(17, 9)
+#define SPACEMIT_LCR_LV_STANDARD_MAX_VALUE	FIELD_MAX(SPACEMIT_LCR_LV_STANDARD_MASK)
+#define SPACEMIT_LCR_LV_FAST_MAX_VALUE		FIELD_MAX(SPACEMIT_LCR_LV_FAST_MASK)
+
 /* i2c bus recover timeout: us */
 #define SPACEMIT_I2C_BUS_BUSY_TIMEOUT		100000
 
@@ -104,11 +112,20 @@ enum spacemit_i2c_state {
 	SPACEMIT_STATE_WRITE,
 };
 
+enum spacemit_i2c_mode {
+	SPACEMIT_MODE_STANDARD,
+	SPACEMIT_MODE_FAST
+};
+
 /* i2c-spacemit driver's main struct */
 struct spacemit_i2c_dev {
 	struct device *dev;
 	struct i2c_adapter adapt;
 
+	struct clk_hw scl_clk_hw;
+	struct clk *scl_clk;
+	enum spacemit_i2c_mode mode;
+
 	/* hardware resources */
 	void __iomem *base;
 	int irq;
@@ -129,6 +146,77 @@ struct spacemit_i2c_dev {
 	u32 status;
 };
 
+static void spacemit_i2c_scl_clk_disable_unprepare(void *data)
+{
+	struct spacemit_i2c_dev *i2c = data;
+
+	clk_disable_unprepare(i2c->scl_clk);
+}
+
+static int spacemit_i2c_clk_set_rate(struct clk_hw *hw, unsigned long rate,
+				     unsigned long parent_rate)
+{
+	struct spacemit_i2c_dev *i2c = container_of(hw, struct spacemit_i2c_dev, scl_clk_hw);
+	u32 lv, lcr, mask, max_lv;
+
+	lv = DIV_ROUND_UP(parent_rate, rate);
+
+	if (i2c->mode == SPACEMIT_MODE_STANDARD) {
+		mask = SPACEMIT_LCR_LV_STANDARD_MASK;
+		max_lv = SPACEMIT_LCR_LV_STANDARD_MAX_VALUE;
+	} else if (i2c->mode == SPACEMIT_MODE_FAST) {
+		mask = SPACEMIT_LCR_LV_FAST_MASK;
+		max_lv = SPACEMIT_LCR_LV_FAST_MAX_VALUE;
+	}
+
+	if (!lv || lv > max_lv) {
+		dev_err(i2c->dev, "set scl clock failed: lv 0x%x", lv);
+		return -EINVAL;
+	}
+
+	lcr = readl(i2c->base + SPACEMIT_ILCR);
+	lcr &= ~mask;
+	lcr |= lv << __ffs(mask);
+	writel(lcr, i2c->base + SPACEMIT_ILCR);
+
+	return 0;
+}
+
+static long spacemit_i2c_clk_round_rate(struct clk_hw *hw, unsigned long rate,
+					unsigned long *parent_rate)
+{
+	u32 lv, freq;
+
+	lv = DIV_ROUND_UP(*parent_rate, rate);
+	freq = DIV_ROUND_UP(*parent_rate, lv);
+
+	return freq;
+}
+
+static unsigned long spacemit_i2c_clk_recalc_rate(struct clk_hw *hw,
+						  unsigned long parent_rate)
+{
+	struct spacemit_i2c_dev *i2c = container_of(hw, struct spacemit_i2c_dev, scl_clk_hw);
+	u32 lcr, lv = 0;
+
+	lcr = readl(i2c->base + SPACEMIT_ILCR);
+
+	if (i2c->mode == SPACEMIT_MODE_STANDARD)
+		lv = FIELD_GET(SPACEMIT_LCR_LV_STANDARD_MASK, lcr);
+	else if (i2c->mode == SPACEMIT_MODE_FAST)
+		lv = FIELD_GET(SPACEMIT_LCR_LV_FAST_MASK, lcr);
+	else
+		return 0;
+
+	return DIV_ROUND_UP(parent_rate, lv);
+}
+
+static const struct clk_ops spacemit_i2c_clk_ops = {
+	.set_rate = spacemit_i2c_clk_set_rate,
+	.round_rate = spacemit_i2c_clk_round_rate,
+	.recalc_rate = spacemit_i2c_clk_recalc_rate,
+};
+
 static void spacemit_i2c_enable(struct spacemit_i2c_dev *i2c)
 {
 	u32 val;
@@ -147,6 +235,29 @@ static void spacemit_i2c_disable(struct spacemit_i2c_dev *i2c)
 	writel(val, i2c->base + SPACEMIT_ICR);
 }
 
+static struct clk *spacemit_i2c_register_scl_clk(struct spacemit_i2c_dev *i2c,
+						 struct clk *parent)
+{
+	struct clk_init_data init = {};
+	char name[64];
+	int ret;
+
+	ret = snprintf(name, sizeof(name), "%s_scl_clk", dev_name(i2c->dev));
+	if (ret >= ARRAY_SIZE(name))
+		dev_warn(i2c->dev, "scl clock name truncated");
+
+	init.name = name;
+	init.ops = &spacemit_i2c_clk_ops;
+	init.parent_data = (struct clk_parent_data[]) {
+		{ .fw_name = "func" },
+	};
+	init.num_parents = 1;
+
+	i2c->scl_clk_hw.init = &init;
+
+	return devm_clk_register(i2c->dev, &i2c->scl_clk_hw);
+}
+
 static void spacemit_i2c_reset(struct spacemit_i2c_dev *i2c)
 {
 	writel(SPACEMIT_CR_UR, i2c->base + SPACEMIT_ICR);
@@ -257,7 +368,7 @@ static void spacemit_i2c_init(struct spacemit_i2c_dev *i2c)
 	 */
 	val |= SPACEMIT_CR_DRFIE;
 
-	if (i2c->clock_freq == SPACEMIT_I2C_MAX_FAST_MODE_FREQ)
+	if (i2c->mode == SPACEMIT_MODE_FAST)
 		val |= SPACEMIT_CR_MODE_FAST;
 
 	/* disable response to general call */
@@ -545,14 +656,15 @@ static int spacemit_i2c_probe(struct platform_device *pdev)
 		dev_warn(dev, "failed to read clock-frequency property: %d\n", ret);
 
 	/* For now, this driver doesn't support high-speed. */
-	if (!i2c->clock_freq || i2c->clock_freq > SPACEMIT_I2C_MAX_FAST_MODE_FREQ) {
-		dev_warn(dev, "unsupported clock frequency %u; using %u\n",
-			 i2c->clock_freq, SPACEMIT_I2C_MAX_FAST_MODE_FREQ);
+	if (i2c->clock_freq > SPACEMIT_I2C_MAX_STANDARD_MODE_FREQ &&
+	    i2c->clock_freq <= SPACEMIT_I2C_MAX_FAST_MODE_FREQ) {
+		i2c->mode = SPACEMIT_MODE_FAST;
+	} else if (i2c->clock_freq && i2c->clock_freq <= SPACEMIT_I2C_MAX_STANDARD_MODE_FREQ) {
+		i2c->mode = SPACEMIT_MODE_STANDARD;
+	} else {
+		dev_warn(i2c->dev, "invalid clock-frequency, fallback to fast mode");
+		i2c->mode = SPACEMIT_MODE_FAST;
 		i2c->clock_freq = SPACEMIT_I2C_MAX_FAST_MODE_FREQ;
-	} else if (i2c->clock_freq < SPACEMIT_I2C_MAX_STANDARD_MODE_FREQ) {
-		dev_warn(dev, "unsupported clock frequency %u; using %u\n",
-			 i2c->clock_freq,  SPACEMIT_I2C_MAX_STANDARD_MODE_FREQ);
-		i2c->clock_freq = SPACEMIT_I2C_MAX_STANDARD_MODE_FREQ;
 	}
 
 	i2c->dev = &pdev->dev;
@@ -574,10 +686,28 @@ static int spacemit_i2c_probe(struct platform_device *pdev)
 	if (IS_ERR(clk))
 		return dev_err_probe(dev, PTR_ERR(clk), "failed to enable func clock");
 
+	i2c->scl_clk = spacemit_i2c_register_scl_clk(i2c, clk);
+	if (IS_ERR(i2c->scl_clk))
+		return dev_err_probe(&pdev->dev, PTR_ERR(i2c->scl_clk),
+				     "failed to register scl clock\n");
+
 	clk = devm_clk_get_enabled(dev, "bus");
 	if (IS_ERR(clk))
 		return dev_err_probe(dev, PTR_ERR(clk), "failed to enable bus clock");
 
+	ret = clk_set_rate(i2c->scl_clk, i2c->clock_freq);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret, "failed to set rate for SCL clock");
+
+	ret = clk_prepare_enable(i2c->scl_clk);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret, "failed to prepare and enable clock");
+
+	ret = devm_add_action_or_reset(dev, spacemit_i2c_scl_clk_disable_unprepare, i2c);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret,
+				     "failed to register cleanup action for clk disable and unprepare");
+
 	spacemit_i2c_reset(i2c);
 
 	i2c_set_adapdata(&i2c->adapt, i2c);

-- 
2.52.0

Re: [PATCH v5 2/3] i2c: spacemit: configure ILCR for accurate SCL frequency

Posted by Alex Elder 1 month, 1 week ago

On 12/26/25 2:32 AM, Troy Mitchell wrote:
> The SpacemiT I2C controller's SCL (Serial Clock Line) frequency for
> master mode operations is determined by the ILCR (I2C Load Count Register).
> Previously, the driver relied on the hardware's reset default
> values for this register.
> 
> The hardware's default ILCR values (SLV=0x156, FLV=0x5d) yield SCL
> frequencies lower than intended. For example, with the default
> 31.5 MHz input clock, these default settings result in an SCL
> frequency of approximately 93 kHz (standard mode) when targeting 100 kHz,
> and approximately 338 kHz (fast mode) when targeting 400 kHz.
> These frequencies are below the 100 kHz/400 kHz nominal speeds.
> 
> This patch integrates the SCL frequency management into
> the Common Clock Framework (CCF). Specifically, the ILCR register,
> which acts as a frequency divider for the SCL clock, is now registered
> as a managed clock (scl_clk) within the CCF.
> 
> This patch also cleans up unnecessary whitespace
> in the included header files.

Was the above sentence meant to describe what the first
patch did?

I have a few comments below.

> Reviewed-by: Yixun Lan <dlan@gentoo.org>
> Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
> ---
> Changelog in v5:
> - use __ffs() instead of *_SHIFT
> - remove useless *_SHIFT
> - check return value when scl clk name array is truncated
> - rebase to v6.19-rc1
> - Link to v3: https://lore.kernel.org/all/20251017-k1-i2c-ilcr-v4-1-eed4903ecdb9@linux.spacemit.com/
> 
> Changelog in v4:
> - initialize clk_init_data with {} so that init.flags is implicitly set to 0
> - minor cleanup and style fixes for better readability
> - remove unused spacemit_i2c_scl_clk_exclusive_put() cleanup callback
> - replace clk_set_rate_exclusive()/clk_rate_exclusive_put() pair with clk_set_rate()
> - simplify LCR LV field macros by using FIELD_GET/FIELD_MAX helpers
> - Link to v3: https://lore.kernel.org/all/20250814-k1-i2c-ilcr-v3-1-317723e74bcd@linux.spacemit.com/
> 
> Changelog in v3:
> - use MASK macro in `recalc_rate` function
> - rename clock name
> - Link to v2: https://lore.kernel.org/r/20250718-k1-i2c-ilcr-v2-1-b4c68f13dcb1@linux.spacemit.com
> 
> Changelog in v2:
> - Align line breaks.
> - Check `lv` in `clk_set_rate` function.
> - Force fast mode when SCL frequency is illegal or unavailable.
> - Change "linux/bits.h" to <linux/bits.h>
> - Kconfig: Add dependency on CCF.
> - Link to v1: https://lore.kernel.org/all/20250710-k1-i2c-ilcr-v1-1-188d1f460c7d@linux.spacemit.com/
> ---
>   drivers/i2c/busses/Kconfig  |   2 +-
>   drivers/i2c/busses/i2c-k1.c | 146 +++++++++++++++++++++++++++++++++++++++++---
>   2 files changed, 139 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index cea87fcb4a1a945f805e9eff25d2f7ab9f61ddf9..b13fb7155786c246229f5e17aa57603c5e755683 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -799,7 +799,7 @@ config I2C_JZ4780
>   config I2C_K1
>   	tristate "SpacemiT K1 I2C adapter"
>   	depends on ARCH_SPACEMIT || COMPILE_TEST
> -	depends on OF
> +	depends on OF && COMMON_CLK
>   	help
>   	  This option enables support for the I2C interface on the SpacemiT K1
>   	  platform.
> diff --git a/drivers/i2c/busses/i2c-k1.c b/drivers/i2c/busses/i2c-k1.c
> index 30cdf733deef264061c8ea565634c5a17f5aebfd..f0c35e23f4f2e139da0d09f314f3eb0e0462a382 100644
> --- a/drivers/i2c/busses/i2c-k1.c
> +++ b/drivers/i2c/busses/i2c-k1.c
> @@ -4,7 +4,9 @@
>    */
>   
>   #include <linux/bitfield.h>
> +#include <linux/bits.h>
>   #include <linux/clk.h>
> +#include <linux/clk-provider.h>
>   #include <linux/i2c.h>
>   #include <linux/iopoll.h>
>   #include <linux/module.h>
> @@ -16,6 +18,7 @@
>   #define SPACEMIT_ISR		 0x4		/* Status register */
>   #define SPACEMIT_IDBR		 0xc		/* Data buffer register */
>   #define SPACEMIT_IRCR		 0x18		/* Reset cycle counter */
> +#define SPACEMIT_ILCR		 0x10		/* Load Count Register */
>   #define SPACEMIT_IBMR		 0x1c		/* Bus monitor register */
>   
>   /* SPACEMIT_ICR register fields */
> @@ -87,6 +90,11 @@
>   #define SPACEMIT_BMR_SDA         BIT(0)		/* SDA line level */
>   #define SPACEMIT_BMR_SCL         BIT(1)		/* SCL line level */
>   
> +#define SPACEMIT_LCR_LV_STANDARD_MASK		GENMASK(8, 0)
> +#define SPACEMIT_LCR_LV_FAST_MASK		GENMASK(17, 9)

I think the code would be clearer if you just used these
FIELD_MAX() macros (below) where their values are used,
rather than defining this symbolic value.

If you had been assigning something different than the maximum,
I think a new symbol would be warranted, but since it's the max,
just say "we're using the maximum" in the code.

> +#define SPACEMIT_LCR_LV_STANDARD_MAX_VALUE	FIELD_MAX(SPACEMIT_LCR_LV_STANDARD_MASK)
> +#define SPACEMIT_LCR_LV_FAST_MAX_VALUE		FIELD_MAX(SPACEMIT_LCR_LV_FAST_MASK)
> +
>   /* i2c bus recover timeout: us */
>   #define SPACEMIT_I2C_BUS_BUSY_TIMEOUT		100000
>   
> @@ -104,11 +112,20 @@ enum spacemit_i2c_state {
>   	SPACEMIT_STATE_WRITE,
>   };
>   
> +enum spacemit_i2c_mode {
> +	SPACEMIT_MODE_STANDARD,
> +	SPACEMIT_MODE_FAST

Will there ever be a mode other than standard and fast?

Just use a Boolean variable to indicate the mode, maybe
named fast_mode, so false means standard mode.

> +};
> +
>   /* i2c-spacemit driver's main struct */
>   struct spacemit_i2c_dev {
>   	struct device *dev;
>   	struct i2c_adapter adapt;
>   
> +	struct clk_hw scl_clk_hw;
> +	struct clk *scl_clk;
> +	enum spacemit_i2c_mode mode;
> +
>   	/* hardware resources */
>   	void __iomem *base;
>   	int irq;
> @@ -129,6 +146,77 @@ struct spacemit_i2c_dev {
>   	u32 status;
>   };
>   

I don't think this (next) function is needed.

> +static void spacemit_i2c_scl_clk_disable_unprepare(void *data)
> +{
> +	struct spacemit_i2c_dev *i2c = data;
> +
> +	clk_disable_unprepare(i2c->scl_clk);
> +}
> +
> +static int spacemit_i2c_clk_set_rate(struct clk_hw *hw, unsigned long rate,
> +				     unsigned long parent_rate)
> +{
> +	struct spacemit_i2c_dev *i2c = container_of(hw, struct spacemit_i2c_dev, scl_clk_hw);
> +	u32 lv, lcr, mask, max_lv;
> +
> +	lv = DIV_ROUND_UP(parent_rate, rate);

Would DIV_ROUND_CLOSEST() produce more accurate rates?  This
question applies everywhere you use something like this.

Since clk_round_rate() returns the exact rate that the clock
would provide for a given rate, it can be nice to have
clk_set_rate() use clk_round_rate().  As it is, it looks
like your clk_round_rate() returns a frequency value even if
the requested rate is out of range, while clk_set_rate()
would return an error when provided the same out-of-range
requested rate.

> +
> +	if (i2c->mode == SPACEMIT_MODE_STANDARD) {
> +		mask = SPACEMIT_LCR_LV_STANDARD_MASK;
> +		max_lv = SPACEMIT_LCR_LV_STANDARD_MAX_VALUE;
> +	} else if (i2c->mode == SPACEMIT_MODE_FAST) {
> +		mask = SPACEMIT_LCR_LV_FAST_MASK;
> +		max_lv = SPACEMIT_LCR_LV_FAST_MAX_VALUE;
> +	}

Since the standard and fast masks are distinct, what
is the meaning when they both have non-zero values?
Does the SPACEMIT_CR_MODE_FAST determine which of these
two fields is used, and the other is ignored?

> +
> +	if (!lv || lv > max_lv) {
> +		dev_err(i2c->dev, "set scl clock failed: lv 0x%x", lv);
> +		return -EINVAL;
> +	}
> +
> +	lcr = readl(i2c->base + SPACEMIT_ILCR);
> +	lcr &= ~mask;
> +	lcr |= lv << __ffs(mask);
> +	writel(lcr, i2c->base + SPACEMIT_ILCR);

I suppose you can't use FIELD_MODIFY() here, because mask is
not constant.

I guess field_modify() should be defined; see c1c6ab80b25c8
("bitfield: Add non-constant field_{prep,get}() helpers").
But I'm not going to ask you to do that...

In any case, you could use this instead:

	lcr = readl(i2c->base + SPACEMIT_ILCR);
	lcr &= ~mask;
	lcr |= field_prep(mask, lv);
	writel(lcr, i2c->base + SPACEMIT_ILCR);

And if you're going to assign the max value, you could even do:

	lcr = readl(i2c->base + SPACEMIT_ILCR);
	lcr |= field_prep(mask, field_max(mask));
	writel(lcr, i2c->base + SPACEMIT_ILCR);

> +
> +	return 0;
> +}
> +
> +static long spacemit_i2c_clk_round_rate(struct clk_hw *hw, unsigned long rate,
> +					unsigned long *parent_rate)
> +{
> +	u32 lv, freq;
> +
> +	lv = DIV_ROUND_UP(*parent_rate, rate);

Could lv contain 0 at this point?

> +	freq = DIV_ROUND_UP(*parent_rate, lv);
> +
> +	return freq;
> +}
> +
> +static unsigned long spacemit_i2c_clk_recalc_rate(struct clk_hw *hw,
> +						  unsigned long parent_rate)
> +{
> +	struct spacemit_i2c_dev *i2c = container_of(hw, struct spacemit_i2c_dev, scl_clk_hw);
> +	u32 lcr, lv = 0;
> +
> +	lcr = readl(i2c->base + SPACEMIT_ILCR);
> +
> +	if (i2c->mode == SPACEMIT_MODE_STANDARD)
> +		lv = FIELD_GET(SPACEMIT_LCR_LV_STANDARD_MASK, lcr);
> +	else if (i2c->mode == SPACEMIT_MODE_FAST)
> +		lv = FIELD_GET(SPACEMIT_LCR_LV_FAST_MASK, lcr);
> +	else
> +		return 0;

What if lv contains 0 at this point?

> +	return DIV_ROUND_UP(parent_rate, lv);
> +}
> +
> +static const struct clk_ops spacemit_i2c_clk_ops = {
> +	.set_rate = spacemit_i2c_clk_set_rate,
> +	.round_rate = spacemit_i2c_clk_round_rate,
> +	.recalc_rate = spacemit_i2c_clk_recalc_rate,
> +};
> +
>   static void spacemit_i2c_enable(struct spacemit_i2c_dev *i2c)
>   {
>   	u32 val;
> @@ -147,6 +235,29 @@ static void spacemit_i2c_disable(struct spacemit_i2c_dev *i2c)
>   	writel(val, i2c->base + SPACEMIT_ICR);
>   }
>   
> +static struct clk *spacemit_i2c_register_scl_clk(struct spacemit_i2c_dev *i2c,
> +						 struct clk *parent)
> +{
> +	struct clk_init_data init = {};
> +	char name[64];
> +	int ret;
> +
> +	ret = snprintf(name, sizeof(name), "%s_scl_clk", dev_name(i2c->dev));
> +	if (ret >= ARRAY_SIZE(name))
> +		dev_warn(i2c->dev, "scl clock name truncated");
> +
> +	init.name = name;
> +	init.ops = &spacemit_i2c_clk_ops;
> +	init.parent_data = (struct clk_parent_data[]) {
> +		{ .fw_name = "func" },
> +	};
> +	init.num_parents = 1;
> +
> +	i2c->scl_clk_hw.init = &init;
> +
> +	return devm_clk_register(i2c->dev, &i2c->scl_clk_hw);
> +}
> +
>   static void spacemit_i2c_reset(struct spacemit_i2c_dev *i2c)
>   {
>   	writel(SPACEMIT_CR_UR, i2c->base + SPACEMIT_ICR);
> @@ -257,7 +368,7 @@ static void spacemit_i2c_init(struct spacemit_i2c_dev *i2c)
>   	 */
>   	val |= SPACEMIT_CR_DRFIE;
>   
> -	if (i2c->clock_freq == SPACEMIT_I2C_MAX_FAST_MODE_FREQ)
> +	if (i2c->mode == SPACEMIT_MODE_FAST)
>   		val |= SPACEMIT_CR_MODE_FAST;
>   
>   	/* disable response to general call */
> @@ -545,14 +656,15 @@ static int spacemit_i2c_probe(struct platform_device *pdev)
>   		dev_warn(dev, "failed to read clock-frequency property: %d\n", ret);
>   
>   	/* For now, this driver doesn't support high-speed. */
> -	if (!i2c->clock_freq || i2c->clock_freq > SPACEMIT_I2C_MAX_FAST_MODE_FREQ) {
> -		dev_warn(dev, "unsupported clock frequency %u; using %u\n",
> -			 i2c->clock_freq, SPACEMIT_I2C_MAX_FAST_MODE_FREQ);
> +	if (i2c->clock_freq > SPACEMIT_I2C_MAX_STANDARD_MODE_FREQ &&
> +	    i2c->clock_freq <= SPACEMIT_I2C_MAX_FAST_MODE_FREQ) {
> +		i2c->mode = SPACEMIT_MODE_FAST;
> +	} else if (i2c->clock_freq && i2c->clock_freq <= SPACEMIT_I2C_MAX_STANDARD_MODE_FREQ) {
> +		i2c->mode = SPACEMIT_MODE_STANDARD;
> +	} else {
> +		dev_warn(i2c->dev, "invalid clock-frequency, fallback to fast mode");
> +		i2c->mode = SPACEMIT_MODE_FAST;

I might just be missing something while reviewing, but if it's
out of range, what rate is used?  Does clk_set_rate() force it
to be valid?

You could combine the fast rate conditions:

	if (!i2c->clock_freq ||
	    i2c->clock_freq > SPACEMIT_I2C_MAX_STANDARD_MODE_FREQ) {
		if (i2c->clock_freq > SPACEMIT_I2C_MAX_FAST_MODE_FREQ) {
			dev_warn("too fast... using %u", MAX);
			i2c->clock_freq = MAX;
		}
		i2c->mode = SPACEMIT_MODE_FAST;
	} else {
		i2c->mode = SPACEMIT_MODE_STANDARD;
	}

>   		i2c->clock_freq = SPACEMIT_I2C_MAX_FAST_MODE_FREQ;
> -	} else if (i2c->clock_freq < SPACEMIT_I2C_MAX_STANDARD_MODE_FREQ) {
> -		dev_warn(dev, "unsupported clock frequency %u; using %u\n",
> -			 i2c->clock_freq,  SPACEMIT_I2C_MAX_STANDARD_MODE_FREQ);
> -		i2c->clock_freq = SPACEMIT_I2C_MAX_STANDARD_MODE_FREQ;
>   	}
>   
>   	i2c->dev = &pdev->dev;
> @@ -574,10 +686,28 @@ static int spacemit_i2c_probe(struct platform_device *pdev)
>   	if (IS_ERR(clk))
>   		return dev_err_probe(dev, PTR_ERR(clk), "failed to enable func clock");
>   
> +	i2c->scl_clk = spacemit_i2c_register_scl_clk(i2c, clk);
> +	if (IS_ERR(i2c->scl_clk))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(i2c->scl_clk),
> +				     "failed to register scl clock\n");
> +
>   	clk = devm_clk_get_enabled(dev, "bus");
>   	if (IS_ERR(clk))
>   		return dev_err_probe(dev, PTR_ERR(clk), "failed to enable bus clock");
>   
> +	ret = clk_set_rate(i2c->scl_clk, i2c->clock_freq);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret, "failed to set rate for SCL clock");
> +
> +	ret = clk_prepare_enable(i2c->scl_clk);

Just use:

	ret = devm_clk_get_enabled(i2c->scl_clk);

					-Alex

> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret, "failed to prepare and enable clock");
> +
> +	ret = devm_add_action_or_reset(dev, spacemit_i2c_scl_clk_disable_unprepare, i2c);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret,
> +				     "failed to register cleanup action for clk disable and unprepare");
> +
>   	spacemit_i2c_reset(i2c);
>   
>   	i2c_set_adapdata(&i2c->adapt, i2c);
>

Re: [PATCH v5 2/3] i2c: spacemit: configure ILCR for accurate SCL frequency

Posted by Troy Mitchell 1 month, 1 week ago

On Sun, Dec 28, 2025 at 06:56:33PM -0600, Alex Elder wrote:
> On 12/26/25 2:32 AM, Troy Mitchell wrote:
> > The SpacemiT I2C controller's SCL (Serial Clock Line) frequency for
> > master mode operations is determined by the ILCR (I2C Load Count Register).
> > Previously, the driver relied on the hardware's reset default
> > values for this register.
> > 
> > The hardware's default ILCR values (SLV=0x156, FLV=0x5d) yield SCL
> > frequencies lower than intended. For example, with the default
> > 31.5 MHz input clock, these default settings result in an SCL
> > frequency of approximately 93 kHz (standard mode) when targeting 100 kHz,
> > and approximately 338 kHz (fast mode) when targeting 400 kHz.
> > These frequencies are below the 100 kHz/400 kHz nominal speeds.
> > 
> > This patch integrates the SCL frequency management into
> > the Common Clock Framework (CCF). Specifically, the ILCR register,
> > which acts as a frequency divider for the SCL clock, is now registered
> > as a managed clock (scl_clk) within the CCF.
> > 
> > This patch also cleans up unnecessary whitespace
> > in the included header files.
> 
> Was the above sentence meant to describe what the first
> patch did?
Yes, I forgot to remove.

> 
> I have a few comments below.
[...]
> 
> > @@ -104,11 +112,20 @@ enum spacemit_i2c_state {
> >   	SPACEMIT_STATE_WRITE,
> >   };
> > +enum spacemit_i2c_mode {
> > +	SPACEMIT_MODE_STANDARD,
> > +	SPACEMIT_MODE_FAST
> 
> Will there ever be a mode other than standard and fast?
> 
> Just use a Boolean variable to indicate the mode, maybe
> named fast_mode, so false means standard mode.
I have talked about that [1]
> 
> > +};
> > +
> >   /* i2c-spacemit driver's main struct */
> >   struct spacemit_i2c_dev {
> >   	struct device *dev;
> >   	struct i2c_adapter adapt;
> > +	struct clk_hw scl_clk_hw;
> > +	struct clk *scl_clk;
> > +	enum spacemit_i2c_mode mode;
> > +
> >   	/* hardware resources */
> >   	void __iomem *base;
> >   	int irq;
> > @@ -129,6 +146,77 @@ struct spacemit_i2c_dev {
> >   	u32 status;
> >   };
> 
> I don't think this (next) function is needed.
I agree this is a very small wrapper.
But it is intended to be used as a devm_add_action_or_reset() callback, so
that we can avoid additional goto-based error handling in probe().

> 
> > +static void spacemit_i2c_scl_clk_disable_unprepare(void *data)
> > +{
> > +	struct spacemit_i2c_dev *i2c = data;
> > +
> > +	clk_disable_unprepare(i2c->scl_clk);
> > +}
> > +
> > +static int spacemit_i2c_clk_set_rate(struct clk_hw *hw, unsigned long rate,
> > +				     unsigned long parent_rate)
> > +{
> > +	struct spacemit_i2c_dev *i2c = container_of(hw, struct spacemit_i2c_dev, scl_clk_hw);
> > +	u32 lv, lcr, mask, max_lv;
> > +
> > +	lv = DIV_ROUND_UP(parent_rate, rate);
> 
> Would DIV_ROUND_CLOSEST() produce more accurate rates?  This
> question applies everywhere you use something like this.
> 
> Since clk_round_rate() returns the exact rate that the clock
> would provide for a given rate, it can be nice to have
> clk_set_rate() use clk_round_rate().  As it is, it looks
> like your clk_round_rate() returns a frequency value even if
> the requested rate is out of range, while clk_set_rate()
> would return an error when provided the same out-of-range
> requested rate.
Right, this implies that clk_round_rate() should only return rates that
clk_set_rate() can accept.
I will switch to using DIV_ROUND_CLOSEST() and drop the max_lv check in
clk_set_rate() to keep the behavior consistent.
> 
> > +
> > +	if (i2c->mode == SPACEMIT_MODE_STANDARD) {
> > +		mask = SPACEMIT_LCR_LV_STANDARD_MASK;
> > +		max_lv = SPACEMIT_LCR_LV_STANDARD_MAX_VALUE;
> > +	} else if (i2c->mode == SPACEMIT_MODE_FAST) {
> > +		mask = SPACEMIT_LCR_LV_FAST_MASK;
> > +		max_lv = SPACEMIT_LCR_LV_FAST_MAX_VALUE;
> > +	}
> 
> Since the standard and fast masks are distinct, what
> is the meaning when they both have non-zero values?
> Does the SPACEMIT_CR_MODE_FAST determine which of these
> two fields is used, and the other is ignored?
Yes, This is hardware behavior.
> 
> > +
> > +	if (!lv || lv > max_lv) {
> > +		dev_err(i2c->dev, "set scl clock failed: lv 0x%x", lv);
> > +		return -EINVAL;
> > +	}
> > +
> > +	lcr = readl(i2c->base + SPACEMIT_ILCR);
> > +	lcr &= ~mask;
> > +	lcr |= lv << __ffs(mask);
> > +	writel(lcr, i2c->base + SPACEMIT_ILCR);
> 
> I suppose you can't use FIELD_MODIFY() here, because mask is
> not constant.
> 
> I guess field_modify() should be defined; see c1c6ab80b25c8
> ("bitfield: Add non-constant field_{prep,get}() helpers").
> But I'm not going to ask you to do that...
> 
> In any case, you could use this instead:
> 
> 	lcr = readl(i2c->base + SPACEMIT_ILCR);
> 	lcr &= ~mask;
> 	lcr |= field_prep(mask, lv);
> 	writel(lcr, i2c->base + SPACEMIT_ILCR);
> 
> And if you're going to assign the max value, you could even do:
> 
> 	lcr = readl(i2c->base + SPACEMIT_ILCR);
> 	lcr |= field_prep(mask, field_max(mask));
> 	writel(lcr, i2c->base + SPACEMIT_ILCR);
Thanks! Since clk_round_rate() uses DIV_ROUND_CLOSEST(), clk_set_rate() no
longer needs to validate the divider value.
I will remove the lv check and use FIELD_MODIFY() instead.
> 
> > +
> > +	return 0;
> > +}
> > +
> > +static long spacemit_i2c_clk_round_rate(struct clk_hw *hw, unsigned long rate,
> > +					unsigned long *parent_rate)
> > +{
> > +	u32 lv, freq;
> > +
> > +	lv = DIV_ROUND_UP(*parent_rate, rate);
> 
> Could lv contain 0 at this point?
lv can't be 0.

> 
> > +	freq = DIV_ROUND_UP(*parent_rate, lv);
> > +
> > +	return freq;
> > +}
> > +
> > +static unsigned long spacemit_i2c_clk_recalc_rate(struct clk_hw *hw,
> > +						  unsigned long parent_rate)
> > +{
> > +	struct spacemit_i2c_dev *i2c = container_of(hw, struct spacemit_i2c_dev, scl_clk_hw);
> > +	u32 lcr, lv = 0;
> > +
> > +	lcr = readl(i2c->base + SPACEMIT_ILCR);
> > +
> > +	if (i2c->mode == SPACEMIT_MODE_STANDARD)
> > +		lv = FIELD_GET(SPACEMIT_LCR_LV_STANDARD_MASK, lcr);
> > +	else if (i2c->mode == SPACEMIT_MODE_FAST)
> > +		lv = FIELD_GET(SPACEMIT_LCR_LV_FAST_MASK, lcr);
> > +	else
> > +		return 0;
> 
> What if lv contains 0 at this point?
The scl clock is: parent_clk_rate / lv.
So idk the hardware behavior if lv is 0.
I'll check the lv value.
> 
> > +	return DIV_ROUND_UP(parent_rate, lv);
> > +}
> > +
> > +static const struct clk_ops spacemit_i2c_clk_ops = {
> > +	.set_rate = spacemit_i2c_clk_set_rate,
> > +	.round_rate = spacemit_i2c_clk_round_rate,
> > +	.recalc_rate = spacemit_i2c_clk_recalc_rate,
> > +};
> > +
> >   static void spacemit_i2c_enable(struct spacemit_i2c_dev *i2c)
> >   {
> >   	u32 val;
> > @@ -147,6 +235,29 @@ static void spacemit_i2c_disable(struct spacemit_i2c_dev *i2c)
> >   	writel(val, i2c->base + SPACEMIT_ICR);
> >   }
> > +static struct clk *spacemit_i2c_register_scl_clk(struct spacemit_i2c_dev *i2c,
> > +						 struct clk *parent)
> > +{
> > +	struct clk_init_data init = {};
> > +	char name[64];
> > +	int ret;
> > +
> > +	ret = snprintf(name, sizeof(name), "%s_scl_clk", dev_name(i2c->dev));
> > +	if (ret >= ARRAY_SIZE(name))
> > +		dev_warn(i2c->dev, "scl clock name truncated");
> > +
> > +	init.name = name;
> > +	init.ops = &spacemit_i2c_clk_ops;
> > +	init.parent_data = (struct clk_parent_data[]) {
> > +		{ .fw_name = "func" },
> > +	};
> > +	init.num_parents = 1;
> > +
> > +	i2c->scl_clk_hw.init = &init;
> > +
> > +	return devm_clk_register(i2c->dev, &i2c->scl_clk_hw);
> > +}
> > +
> >   static void spacemit_i2c_reset(struct spacemit_i2c_dev *i2c)
> >   {
> >   	writel(SPACEMIT_CR_UR, i2c->base + SPACEMIT_ICR);
> > @@ -257,7 +368,7 @@ static void spacemit_i2c_init(struct spacemit_i2c_dev *i2c)
> >   	 */
> >   	val |= SPACEMIT_CR_DRFIE;
> > -	if (i2c->clock_freq == SPACEMIT_I2C_MAX_FAST_MODE_FREQ)
> > +	if (i2c->mode == SPACEMIT_MODE_FAST)
> >   		val |= SPACEMIT_CR_MODE_FAST;
> >   	/* disable response to general call */
> > @@ -545,14 +656,15 @@ static int spacemit_i2c_probe(struct platform_device *pdev)
> >   		dev_warn(dev, "failed to read clock-frequency property: %d\n", ret);
> >   	/* For now, this driver doesn't support high-speed. */
> > -	if (!i2c->clock_freq || i2c->clock_freq > SPACEMIT_I2C_MAX_FAST_MODE_FREQ) {
> > -		dev_warn(dev, "unsupported clock frequency %u; using %u\n",
> > -			 i2c->clock_freq, SPACEMIT_I2C_MAX_FAST_MODE_FREQ);
> > +	if (i2c->clock_freq > SPACEMIT_I2C_MAX_STANDARD_MODE_FREQ &&
> > +	    i2c->clock_freq <= SPACEMIT_I2C_MAX_FAST_MODE_FREQ) {
> > +		i2c->mode = SPACEMIT_MODE_FAST;
> > +	} else if (i2c->clock_freq && i2c->clock_freq <= SPACEMIT_I2C_MAX_STANDARD_MODE_FREQ) {
> > +		i2c->mode = SPACEMIT_MODE_STANDARD;
> > +	} else {
> > +		dev_warn(i2c->dev, "invalid clock-frequency, fallback to fast mode");
> > +		i2c->mode = SPACEMIT_MODE_FAST;
> 
> I might just be missing something while reviewing, but if it's
> out of range,
>
> what rate is used?  Does clk_set_rate() force it
> to be valid?
If it's out of range, it's high-speed mode. up to 3.3MHZ
set|round|recal_rate don't support. bcs that's another filed in ILCR.
> 
> You could combine the fast rate conditions:
> 
> 	if (!i2c->clock_freq ||
> 	    i2c->clock_freq > SPACEMIT_I2C_MAX_STANDARD_MODE_FREQ) {
> 		if (i2c->clock_freq > SPACEMIT_I2C_MAX_FAST_MODE_FREQ) {
> 			dev_warn("too fast... using %u", MAX);
> 			i2c->clock_freq = MAX;
> 		}
No, when add high-speed mode, it must be destroy.

> 		i2c->mode = SPACEMIT_MODE_FAST;
> 	} else {
> 		i2c->mode = SPACEMIT_MODE_STANDARD;
> 	}
> 
> >   		i2c->clock_freq = SPACEMIT_I2C_MAX_FAST_MODE_FREQ;
> > -	} else if (i2c->clock_freq < SPACEMIT_I2C_MAX_STANDARD_MODE_FREQ) {
> > -		dev_warn(dev, "unsupported clock frequency %u; using %u\n",
> > -			 i2c->clock_freq,  SPACEMIT_I2C_MAX_STANDARD_MODE_FREQ);
> > -		i2c->clock_freq = SPACEMIT_I2C_MAX_STANDARD_MODE_FREQ;
> >   	}
> >   	i2c->dev = &pdev->dev;
> > @@ -574,10 +686,28 @@ static int spacemit_i2c_probe(struct platform_device *pdev)
> >   	if (IS_ERR(clk))
> >   		return dev_err_probe(dev, PTR_ERR(clk), "failed to enable func clock");
> > +	i2c->scl_clk = spacemit_i2c_register_scl_clk(i2c, clk);
> > +	if (IS_ERR(i2c->scl_clk))
> > +		return dev_err_probe(&pdev->dev, PTR_ERR(i2c->scl_clk),
> > +				     "failed to register scl clock\n");
> > +
> >   	clk = devm_clk_get_enabled(dev, "bus");
> >   	if (IS_ERR(clk))
> >   		return dev_err_probe(dev, PTR_ERR(clk), "failed to enable bus clock");
> > +	ret = clk_set_rate(i2c->scl_clk, i2c->clock_freq);
> > +	if (ret)
> > +		return dev_err_probe(&pdev->dev, ret, "failed to set rate for SCL clock");
> > +
> > +	ret = clk_prepare_enable(i2c->scl_clk);
> 
> Just use:
> 
> 	ret = devm_clk_get_enabled(i2c->scl_clk);
Thanks.

Link: https://lore.kernel.org/all/BD74A47E5BB66010+aU4j6CgGxebcBV5I@kernel.org/ [1]

                            - Troy
> 
> 					-Alex
> 
> > +	if (ret)
> > +		return dev_err_probe(&pdev->dev, ret, "failed to prepare and enable clock");
> > +
> > +	ret = devm_add_action_or_reset(dev, spacemit_i2c_scl_clk_disable_unprepare, i2c);
> > +	if (ret)
> > +		return dev_err_probe(&pdev->dev, ret,
> > +				     "failed to register cleanup action for clk disable and unprepare");
> > +
> >   	spacemit_i2c_reset(i2c);
> >   	i2c_set_adapdata(&i2c->adapt, i2c);
> > 
> 
>

[PATCH v5 1/3] i2c: spacemit: drop useless spaces
[PATCH v5 2/3] i2c: spacemit: configure ILCR for accurate SCL frequency
[PATCH v5 3/3] i2c: spacemit: drop warning when clock-frequency property is absent