[PATCH v6 0/2] i2c: spacemit: improve clock handling and cleanups

[PATCH v6 0/2] i2c: spacemit: improve clock handling and cleanups

[Troy Mitchell]

This patch series contains a set of small improvements and cleanups for
the SpacemiT I2C controller driver.

The main change in this series is to improve SCL frequency accuracy by
configuring the ILCR register based on the requested clock rate, rather
than relying on the hardware reset defaults.

The ILCR is exposed as a managed clock via the Common Clock Framework,
allowing the driver to program appropriate divider values for standard
and fast modes.

In addition, the series removes a spurious warning when the optional
clock-frequency DT property is absent.

Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
---
Troy Mitchell (2):
      i2c: spacemit: configure ILCR/IWCR for accurate SCL frequency
      i2c: spacemit: drop warning when clock-frequency property is absent

 drivers/i2c/busses/Kconfig  |   2 +-
 drivers/i2c/busses/i2c-k1.c | 164 +++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 154 insertions(+), 12 deletions(-)
---
base-commit: 9b45dba17a6b3cede80706d287d8d2fb9087bfa9
change-id: 20251017-k1-i2c-ilcr-c928d50e407f

Best regards,
--  
Troy Mitchell <troy.mitchell@linux.spacemit.com>


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v6 1/2] i2c: spacemit: configure ILCR/IWCR for accurate SCL frequency

[Troy Mitchell]

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.

The actual hardware timing formulas are:
- standard mode: SCL = FCLK / (2 * SLV + 8)
- fast mode:     SCL = FCLK / (2 * FLV + 10)

These formulas are only valid when the IWCR (Wait Count Register) is
programmed to 0x142A, a value specified by the I2C IP designer. The
driver now initializes IWCR to this value during controller init.

Reviewed-by: Yixun Lan <dlan@gentoo.org>
Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
---
Changelog in v6:
- fix SCL frequency calculation to match hardware timing formulas
  (SCL = FCLK / (2*SLV+8) for standard, FCLK / (2*FLV+10) for fast)
- initialize IWCR to 0x142A during init(required by I2C IP for
  correct SCL timing)
- use DIV_ROUND_CLOSEST() instead of DIV_ROUND_UP() for more accurate frequency
- use field_prep() instead of manual bit shift for ILCR register programming
- replace .round_rate with .determine_rate (round_rate was removed from clk_ops)
- remove _MAX_VALUE macros (no longer needed after removing max_lv validation)
- remove redundant max_lv validation in set_rate (determine_rate guarantees valid values)
- simplify scl_clk_disable_unprepare callback to take clk pointer directly
- remove unused parent parameter from spacemit_i2c_register_scl_clk()
- remove stale description about whitespace cleanup from commit message
- Link to v5: https://lore.kernel.org/r/20251226-k1-i2c-ilcr-v5-0-b5807b7dd0e6@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 | 160 +++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 153 insertions(+), 9 deletions(-)

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 8c935f867a37..89898fff1967 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -793,7 +793,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 9152cf436bea..fad6a20bb43d 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>
@@ -17,6 +19,8 @@
 #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_IWCR		 0x14		/* Wait Count Register */
 #define SPACEMIT_IBMR		 0x1c		/* Bus monitor register */
 
 /* SPACEMIT_ICR register fields */
@@ -88,6 +92,12 @@
 #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)
+
+/* Required by I2C IP for correct SCL timing */
+#define SPACEMIT_IWCR_INIT_VALUE		0x142A
+
 /* i2c bus recover timeout: us */
 #define SPACEMIT_I2C_BUS_BUSY_TIMEOUT		100000
 
@@ -109,11 +119,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;
@@ -135,6 +154,82 @@ struct spacemit_i2c_dev {
 	u32 status;
 };
 
+static void spacemit_i2c_scl_clk_disable_unprepare(void *data)
+{
+	clk_disable_unprepare(data);
+}
+
+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, denom;
+
+	/*
+	 * Hardware timing formulas:
+	 * - standard mode: SCL = FCLK / (2 * SLV + 8)
+	 * - fast mode:     SCL = FCLK / (2 * FLV + 10)
+	 */
+	denom = DIV_ROUND_CLOSEST(parent_rate, rate);
+
+	if (i2c->mode == SPACEMIT_MODE_STANDARD) {
+		mask = SPACEMIT_LCR_LV_STANDARD_MASK;
+		lv = (denom <= 8) ? 0 : DIV_ROUND_CLOSEST(denom - 8, 2);
+	} else {
+		mask = SPACEMIT_LCR_LV_FAST_MASK;
+		lv = (denom <= 10) ? 0 : DIV_ROUND_CLOSEST(denom - 10, 2);
+	}
+
+	lcr = readl(i2c->base + SPACEMIT_ILCR);
+	lcr &= ~mask;
+	lcr |= field_prep(mask, lv);
+	writel(lcr, i2c->base + SPACEMIT_ILCR);
+
+	return 0;
+}
+
+static int spacemit_i2c_clk_determine_rate(struct clk_hw *hw,
+					   struct clk_rate_request *req)
+{
+	struct spacemit_i2c_dev *i2c = container_of(hw, struct spacemit_i2c_dev, scl_clk_hw);
+	u32 lv, denom;
+
+	denom = DIV_ROUND_CLOSEST(req->best_parent_rate, req->rate);
+
+	if (i2c->mode == SPACEMIT_MODE_STANDARD) {
+		lv = (denom <= 8) ? 0 : DIV_ROUND_CLOSEST(denom - 8, 2);
+		req->rate = DIV_ROUND_CLOSEST(req->best_parent_rate, lv * 2 + 8);
+	} else {
+		lv = (denom <= 10) ? 0 : DIV_ROUND_CLOSEST(denom - 10, 2);
+		req->rate = DIV_ROUND_CLOSEST(req->best_parent_rate, lv * 2 + 10);
+	}
+
+	return 0;
+}
+
+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);
+		return DIV_ROUND_CLOSEST(parent_rate, lv * 2 + 8);
+	}
+
+	lv = FIELD_GET(SPACEMIT_LCR_LV_FAST_MASK, lcr);
+	return DIV_ROUND_CLOSEST(parent_rate, lv * 2 + 10);
+}
+
+static const struct clk_ops spacemit_i2c_clk_ops = {
+	.set_rate = spacemit_i2c_clk_set_rate,
+	.determine_rate = spacemit_i2c_clk_determine_rate,
+	.recalc_rate = spacemit_i2c_clk_recalc_rate,
+};
+
 static void spacemit_i2c_enable(struct spacemit_i2c_dev *i2c)
 {
 	u32 val;
@@ -153,6 +248,28 @@ 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_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);
@@ -286,7 +403,7 @@ static void spacemit_i2c_init(struct spacemit_i2c_dev *i2c)
 		val |= SPACEMIT_CR_MSDIE;
 	}
 
-	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 */
@@ -309,6 +426,14 @@ static void spacemit_i2c_init(struct spacemit_i2c_dev *i2c)
 	writel(val, i2c->base + SPACEMIT_IRCR);
 
 	spacemit_i2c_clear_int_status(i2c, SPACEMIT_I2C_INT_STATUS_MASK);
+
+	/*
+	 * Initialize IWCR to the value specified by the I2C IP designer.
+	 * The SCL frequency formulas (SCL = FCLK / (2*SLV+8) for standard
+	 * mode, SCL = FCLK / (2*FLV+10) for fast mode) are only valid when
+	 * IWCR contains this specific value.
+	 */
+	writel(SPACEMIT_IWCR_INIT_VALUE, i2c->base + SPACEMIT_IWCR);
 }
 
 static void spacemit_i2c_start(struct spacemit_i2c_dev *i2c)
@@ -703,14 +828,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;
@@ -732,6 +858,11 @@ 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);
+	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");
@@ -741,6 +872,19 @@ static int spacemit_i2c_probe(struct platform_device *pdev)
 		return dev_err_probe(dev, PTR_ERR(rst),
 				     "failed to acquire deasserted reset\n");
 
+	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->scl_clk);
+	if (ret)
+		return ret;
+
 	spacemit_i2c_reset(i2c);
 
 	i2c_set_adapdata(&i2c->adapt, i2c);

-- 
2.54.0


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v6 1/2] i2c: spacemit: configure ILCR/IWCR for accurate SCL frequency

[Alex Elder]

On 4/29/26 2:48 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.
> 
> The actual hardware timing formulas are:
> - standard mode: SCL = FCLK / (2 * SLV + 8)
> - fast mode:     SCL = FCLK / (2 * FLV + 10)
> 
> These formulas are only valid when the IWCR (Wait Count Register) is
> programmed to 0x142A, a value specified by the I2C IP designer. The
> driver now initializes IWCR to this value during controller init.
> 
> Reviewed-by: Yixun Lan <dlan@gentoo.org>
> Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>

I have a couple "I keep thinking the same thing when I see this"
comments...  But in summary, I make three suggestions below:
- Consider defining the magic SPACEMIT_IWCR_INIT_VALUE value
   based on the values in its fields
- Have your clk_set_rate() and clk_determine_rate() functions
   call a common helper, to eliminate some duplication of code.
- Use devm_clk_hw_register() and devm_clk_get_enabled() rather
   than the deprecated devm_clk_register() and rolling your own
   managed prepare-enable function.

> ---
> Changelog in v6:
> - fix SCL frequency calculation to match hardware timing formulas
>    (SCL = FCLK / (2*SLV+8) for standard, FCLK / (2*FLV+10) for fast)
> - initialize IWCR to 0x142A during init(required by I2C IP for
>    correct SCL timing)
> - use DIV_ROUND_CLOSEST() instead of DIV_ROUND_UP() for more accurate frequency
> - use field_prep() instead of manual bit shift for ILCR register programming
> - replace .round_rate with .determine_rate (round_rate was removed from clk_ops)
> - remove _MAX_VALUE macros (no longer needed after removing max_lv validation)
> - remove redundant max_lv validation in set_rate (determine_rate guarantees valid values)
> - simplify scl_clk_disable_unprepare callback to take clk pointer directly
> - remove unused parent parameter from spacemit_i2c_register_scl_clk()
> - remove stale description about whitespace cleanup from commit message
> - Link to v5: https://lore.kernel.org/r/20251226-k1-i2c-ilcr-v5-0-b5807b7dd0e6@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 | 160 +++++++++++++++++++++++++++++++++++++++++---
>   2 files changed, 153 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 8c935f867a37..89898fff1967 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -793,7 +793,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 9152cf436bea..fad6a20bb43d 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>
> @@ -17,6 +19,8 @@
>   #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_IWCR		 0x14		/* Wait Count Register */
>   #define SPACEMIT_IBMR		 0x1c		/* Bus monitor register */
>   
>   /* SPACEMIT_ICR register fields */
> @@ -88,6 +92,12 @@
>   #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)
> +
> +/* Required by I2C IP for correct SCL timing */
> +#define SPACEMIT_IWCR_INIT_VALUE		0x142A

Even if it's just used to "do the right thing", I still think it's
useful to break this numeric value into what it *represents*.  The
register is defined this way:
COUNT		GENMASK(4:0)
HS_COUNT1	GENMASK(9:5)
HS_COUNT2	GENMASK(14:10)
(the rest is reserved)

So 0x142a represents:
COUNT		10	33 MHz I2C functional clock (versus 66 MHz)
HS_COUNT1	2	Count defining start bit setup/hold times
HS_COUNT2	3	Count defining stop bit setup/hold times

The first one in particular *could* be managed in your clock
to support even more accurate clock rates.  But even if you
don't change its value I think it's meaningful to know what
the value represents.

That said--what I suggest won't change what the code does,
it just helps the reader understand what it's doing.

> +
>   /* i2c bus recover timeout: us */
>   #define SPACEMIT_I2C_BUS_BUSY_TIMEOUT		100000
>   
> @@ -109,11 +119,20 @@ enum spacemit_i2c_state {
>   	SPACEMIT_STATE_WRITE,
>   };
>   
> +enum spacemit_i2c_mode {
> +	SPACEMIT_MODE_STANDARD,
> +	SPACEMIT_MODE_FAST
> +};

I have a consistent reaction to this--why not use a Boolean?
But I know the answer is that there are other modes that will
be supported, so the enum is the right thing to do...

> +
>   /* 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;
> @@ -135,6 +154,82 @@ struct spacemit_i2c_dev {
>   	u32 status;
>   };
>   
> +static void spacemit_i2c_scl_clk_disable_unprepare(void *data)
> +{
> +	clk_disable_unprepare(data);
> +}

I have a consistent reaction to this, too, but I saw that it's
a devm action (which also explains the void pointer argument).

> +
> +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, denom;
> +
> +	/*
> +	 * Hardware timing formulas:
> +	 * - standard mode: SCL = FCLK / (2 * SLV + 8)
> +	 * - fast mode:     SCL = FCLK / (2 * FLV + 10)
> +	 */
> +	denom = DIV_ROUND_CLOSEST(parent_rate, rate);
> +
> +	if (i2c->mode == SPACEMIT_MODE_STANDARD) {
> +		mask = SPACEMIT_LCR_LV_STANDARD_MASK;
> +		lv = (denom <= 8) ? 0 : DIV_ROUND_CLOSEST(denom - 8, 2);
> +	} else {
> +		mask = SPACEMIT_LCR_LV_FAST_MASK;
> +		lv = (denom <= 10) ? 0 : DIV_ROUND_CLOSEST(denom - 10, 2);
> +	}
> +
> +	lcr = readl(i2c->base + SPACEMIT_ILCR);
> +	lcr &= ~mask;
> +	lcr |= field_prep(mask, lv);
> +	writel(lcr, i2c->base + SPACEMIT_ILCR);
> +
> +	return 0;
> +}
> +
> +static int spacemit_i2c_clk_determine_rate(struct clk_hw *hw,
> +					   struct clk_rate_request *req)
> +{
> +	struct spacemit_i2c_dev *i2c = container_of(hw, struct spacemit_i2c_dev, scl_clk_hw);
> +	u32 lv, denom;

Can you have clk_set_rate() call clk_determine_rate(), or at
least have them both share a helper that does these (duplicated)
calculations?

> +
> +	denom = DIV_ROUND_CLOSEST(req->best_parent_rate, req->rate);
> +
> +	if (i2c->mode == SPACEMIT_MODE_STANDARD) {
> +		lv = (denom <= 8) ? 0 : DIV_ROUND_CLOSEST(denom - 8, 2);
> +		req->rate = DIV_ROUND_CLOSEST(req->best_parent_rate, lv * 2 + 8);
> +	} else {
> +		lv = (denom <= 10) ? 0 : DIV_ROUND_CLOSEST(denom - 10, 2);
> +		req->rate = DIV_ROUND_CLOSEST(req->best_parent_rate, lv * 2 + 10);
> +	}
> +
> +	return 0;
> +}
> +
> +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);
> +		return DIV_ROUND_CLOSEST(parent_rate, lv * 2 + 8);
> +	}
> +
> +	lv = FIELD_GET(SPACEMIT_LCR_LV_FAST_MASK, lcr);
> +	return DIV_ROUND_CLOSEST(parent_rate, lv * 2 + 10);
> +}
> +
> +static const struct clk_ops spacemit_i2c_clk_ops = {
> +	.set_rate = spacemit_i2c_clk_set_rate,
> +	.determine_rate = spacemit_i2c_clk_determine_rate,
> +	.recalc_rate = spacemit_i2c_clk_recalc_rate,
> +};
> +
>   static void spacemit_i2c_enable(struct spacemit_i2c_dev *i2c)
>   {
>   	u32 val;
> @@ -153,6 +248,28 @@ 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_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);

The comments above clk_register() indicate that this is a deprecated
interface, and you should use a form of clk_hw_register() instead.
devm_clk_hw_register() exists and takes the same arguments, but
returns an int rather than the clock pointer.  But I think then you
can call devm_clk_get_enabled() in the caller (spacemit_i2c_probe()).
That would allow you to get rid of your clk_disable_unprepare()
wrapper too.

					-Alex

> +}
> +
>   static void spacemit_i2c_reset(struct spacemit_i2c_dev *i2c)
>   {
>   	writel(SPACEMIT_CR_UR, i2c->base + SPACEMIT_ICR);
> @@ -286,7 +403,7 @@ static void spacemit_i2c_init(struct spacemit_i2c_dev *i2c)
>   		val |= SPACEMIT_CR_MSDIE;
>   	}
>   
> -	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 */
> @@ -309,6 +426,14 @@ static void spacemit_i2c_init(struct spacemit_i2c_dev *i2c)
>   	writel(val, i2c->base + SPACEMIT_IRCR);
>   
>   	spacemit_i2c_clear_int_status(i2c, SPACEMIT_I2C_INT_STATUS_MASK);
> +
> +	/*
> +	 * Initialize IWCR to the value specified by the I2C IP designer.
> +	 * The SCL frequency formulas (SCL = FCLK / (2*SLV+8) for standard
> +	 * mode, SCL = FCLK / (2*FLV+10) for fast mode) are only valid when
> +	 * IWCR contains this specific value.
> +	 */
> +	writel(SPACEMIT_IWCR_INIT_VALUE, i2c->base + SPACEMIT_IWCR);
>   }
>   
>   static void spacemit_i2c_start(struct spacemit_i2c_dev *i2c)
> @@ -703,14 +828,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;
> @@ -732,6 +858,11 @@ 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);
> +	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");
> @@ -741,6 +872,19 @@ static int spacemit_i2c_probe(struct platform_device *pdev)
>   		return dev_err_probe(dev, PTR_ERR(rst),
>   				     "failed to acquire deasserted reset\n");
>   
> +	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->scl_clk);
> +	if (ret)
> +		return ret;
> +
>   	spacemit_i2c_reset(i2c);
>   
>   	i2c_set_adapdata(&i2c->adapt, i2c);
> 


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v6 1/2] i2c: spacemit: configure ILCR/IWCR for accurate SCL frequency

[Troy Mitchell]

On Wed Apr 29, 2026 at 8:40 PM CST, Alex Elder wrote:
> On 4/29/26 2:48 AM, Troy Mitchell wrote:

[...]

>>   #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_IWCR		 0x14		/* Wait Count Register */
>>   #define SPACEMIT_IBMR		 0x1c		/* Bus monitor register */
>>   
>>   /* SPACEMIT_ICR register fields */
>> @@ -88,6 +92,12 @@
>>   #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)
>> +
>> +/* Required by I2C IP for correct SCL timing */
>> +#define SPACEMIT_IWCR_INIT_VALUE		0x142A
>
> Even if it's just used to "do the right thing", I still think it's
> useful to break this numeric value into what it *represents*.  The
> register is defined this way:
> COUNT		GENMASK(4:0)
> HS_COUNT1	GENMASK(9:5)
> HS_COUNT2	GENMASK(14:10)
> (the rest is reserved)
>
> So 0x142a represents:
> COUNT		10	33 MHz I2C functional clock (versus 66 MHz)
> HS_COUNT1	2	Count defining start bit setup/hold times
> HS_COUNT2	3	Count defining stop bit setup/hold times
>
> The first one in particular *could* be managed in your clock
> to support even more accurate clock rates.  But even if you
> don't change its value I think it's meaningful to know what
> the value represents.
>
> That said--what I suggest won't change what the code does,
> it just helps the reader understand what it's doing.
Thanks for the suggestion.
I've broken SPACEMIT_IWCR_INIT_VALUE into its constituent fields
using FIELD_PREP. One small correction: 0x142A actually decodes
to COUNT=10, HS_COUNT1=1, HS_COUNT2=5 (not 2 and 3).
>
>> +
>>   /* i2c bus recover timeout: us */
>>   #define SPACEMIT_I2C_BUS_BUSY_TIMEOUT		100000
>>   
>>
[...]

>> +
>> +static int spacemit_i2c_clk_determine_rate(struct clk_hw *hw,
>> +					   struct clk_rate_request *req)
>> +{
>> +	struct spacemit_i2c_dev *i2c = container_of(hw, struct spacemit_i2c_dev, scl_clk_hw);
>> +	u32 lv, denom;
>
> Can you have clk_set_rate() call clk_determine_rate(), or at
> least have them both share a helper that does these (duplicated)
> calculations?
Done. Extracted spacemit_i2c_calc_lv() as a shared helper used
by both set_rate and determine_rate.

>
>> +
>> +	denom = DIV_ROUND_CLOSEST(req->best_parent_rate, req->rate);
>> +
>> +	if (i2c->mode == SPACEMIT_MODE_STANDARD) {
>> +		lv = (denom <= 8) ? 0 : DIV_ROUND_CLOSEST(denom - 8, 2);
>> +		req->rate = DIV_ROUND_CLOSEST(req->best_parent_rate, lv * 2 + 8);
>> +	} else {
>> +		lv = (denom <= 10) ? 0 : DIV_ROUND_CLOSEST(denom - 10, 2);
>> +		req->rate = DIV_ROUND_CLOSEST(req->best_parent_rate, lv * 2 + 10);
>> +	}
>> +
>> +	return 0;
>> +}
>> +

[...]

>>   
>> +static struct clk *spacemit_i2c_register_scl_clk(struct spacemit_i2c_dev *i2c)
>> +{
>> +	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);
>
> The comments above clk_register() indicate that this is a deprecated
> interface, and you should use a form of clk_hw_register() instead.
> devm_clk_hw_register() exists and takes the same arguments, but
> returns an int rather than the clock pointer.  But I think then you
> can call devm_clk_get_enabled() in the caller (spacemit_i2c_probe()).
> That would allow you to get rid of your clk_disable_unprepare()
> wrapper too.
I've switched to devm_clk_hw_register() as suggested. However,
devm_clk_get_enabled() isn't applicable here because the SCL clock is
registered internally by the driver rather than being described in the
Device Tree. Since devm_clk_get_enabled() relies on clk_get(),
it would fail to find the clock without a DT entry or a clkdev lookup.

Instead, I'm using devm_clk_hw_get_clk() to retrieve the struct clk
directly from the clk_hw, followed by a manual clk_prepare_enable()
with a devm action for automated cleanup.

                            - Troy

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v6 2/2] i2c: spacemit: drop warning when clock-frequency property is absent

[Troy Mitchell]

The clock-frequency property is optional according to the DT binding.
Do not emit a warning when the property is missing and fall back to the
default frequency instead.

Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
---
Changelog in v6:
- drop Fixes tag per maintainer feedback (this is not a bug fix)
- change dev_warn to dev_info when clock-frequency is absent (it is optional)
- Link to v5: https://lore.kernel.org/r/20251226-k1-i2c-ilcr-v5-0-b5807b7dd0e6@linux.spacemit.com
---
 drivers/i2c/busses/i2c-k1.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-k1.c b/drivers/i2c/busses/i2c-k1.c
index fad6a20bb43d..8b0a215dfb29 100644
--- a/drivers/i2c/busses/i2c-k1.c
+++ b/drivers/i2c/busses/i2c-k1.c
@@ -823,9 +823,7 @@ static int spacemit_i2c_probe(struct platform_device *pdev)
 	if (!i2c)
 		return -ENOMEM;
 
-	ret = of_property_read_u32(of_node, "clock-frequency", &i2c->clock_freq);
-	if (ret && ret != -EINVAL)
-		dev_warn(dev, "failed to read clock-frequency property: %d\n", ret);
+	of_property_read_u32(of_node, "clock-frequency", &i2c->clock_freq);
 
 	/* For now, this driver doesn't support high-speed. */
 	if (i2c->clock_freq > SPACEMIT_I2C_MAX_STANDARD_MODE_FREQ &&
@@ -834,7 +832,7 @@ static int spacemit_i2c_probe(struct platform_device *pdev)
 	} 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");
+		dev_info(dev, "clock-frequency not set or out of range, using fast mode\n");
 		i2c->mode = SPACEMIT_MODE_FAST;
 		i2c->clock_freq = SPACEMIT_I2C_MAX_FAST_MODE_FREQ;
 	}

-- 
2.54.0


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv