[PATCH v2 0/4] i2c/clk: spacemit: k1: fix I2C bus configuration and clock handling

[PATCH v2 0/4] i2c/clk: spacemit: k1: fix I2C bus configuration and clock handling

[Junhui Liu]

Fix two issues in the SpacemiT K1 I2C driver and clock provider.

The first issue is that the controller bus mode is selected using the
functional clock rate instead of the requested I2C bus speed from the
device tree.

The second issue is that the TWSI functional clocks incorrectly include
the bus clock enable bit in their gate masks. As a result the I2C driver
can operate with only the functional clock enabled, which diverges from
the device tree description and the Linux driver (both expect separate
"func" and "bus" clocks).

To fix the second issue without breaking intermediate states, the
patches are ordered as follows:

- First add the missing bus clocks (and their parent chain) to the SPL
  clock tree.
- Then update the I2C driver to acquire and enable both the functional
  and bus clocks by name.
- Finally remove the bus gate bit from the functional clock definitions
  (except TWSI8, whose write-only register requires the combined gate).

---
Changes in v2:
- Split the original patch 2/2 into three patches
- Acquire the functional and bus clocks explicitly by name instead of
  using the bulk clock API
- Link to v1: https://patch.msgid.link/20260814-k1-i2c-fix-v1-0-f2e19cbe0af6@pigmoral.tech

To: Heiko Schocher <hs@nabladev.com>
To: u-boot-spacemit@groups.io
To: u-boot@lists.u-boot-project.org
Cc: Huan Zhou <pericycle.cc@gmail.com>
Cc: Guodong Xu <guodong.xu@oss.qualcomm.com>
Cc: Tom Rini <trini@konsulko.com>
Cc: Junhui Liu <junhui.liu@pigmoral.tech>
Cc: Raymond Mao <raymond.mao@riscstar.com>
Cc: Lukasz Majewski <lukma@denx.de>
Cc: Yixun Lan <dlan@kernel.org>
Cc: Troy Mitchell <troy.mitchell@linux.spacemit.com>
Signed-off-by: Junhui Liu <junhui.liu@pigmoral.tech>

---
Junhui Liu (4):
      i2c: k1: fix wrong bus speed setting
      clk: spacemit: k1: add TWSI bus clocks to SPL
      i2c: k1: enable both functional and bus clocks
      clk: spacemit: k1: remove bus gate from TWSI functional clocks

 drivers/clk/spacemit/clk-k1.c | 41 +++++++++++++++++++++++++++++++++--------
 drivers/i2c/k1_i2c.c          | 27 ++++++++++++++++++---------
 2 files changed, 51 insertions(+), 17 deletions(-)
---
base-commit: 527115ef6783cec49e5610c523c124b399011361
change-id: 20260814-k1-i2c-fix-04dc9e3fd45a

Best regards,
--  
Junhui Liu <junhui.liu@pigmoral.tech>

[PATCH v2 1/4] i2c: k1: fix wrong bus speed setting

[Junhui Liu]

The controller bus mode should be selected according to the requested
I2C bus speed. However, the driver currently passes the functional clock
rate to k1_i2c_set_bus_speed(), so the selected mode does not reflect
the requested bus speed.

Fix this by reading the clock-frequency property from the Device Tree,
defaulting to standard speed, and drop the unused clk_rate field.

Fixes: 271546fb8e54 ("i2c: k1: add I2C driver support")
Reviewed-by: Heiko Schocher <hs@nabladev.com>
Signed-off-by: Junhui Liu <junhui.liu@pigmoral.tech>
---
 drivers/i2c/k1_i2c.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/k1_i2c.c b/drivers/i2c/k1_i2c.c
index a08ff15803d0..2c7a1e0d3775 100644
--- a/drivers/i2c/k1_i2c.c
+++ b/drivers/i2c/k1_i2c.c
@@ -52,7 +52,6 @@ struct k1_i2c_priv {
 	int id;
 	void __iomem *base;
 	struct clk clk;
-	u32 clk_rate;
 };
 
 /*
@@ -466,6 +465,7 @@ static int k1_i2c_probe(struct udevice *bus)
 {
 	struct k1_i2c_priv *priv = dev_get_priv(bus);
 	struct reset_ctl reset;
+	u32 speed;
 	int ret;
 
 	priv->id = dev_seq(bus);
@@ -496,10 +496,13 @@ static int k1_i2c_probe(struct udevice *bus)
 		debug("%s: failed to enable clock\n", __func__);
 		return ret;
 	}
-	priv->clk_rate = clk_get_rate(&priv->clk);
 
 	priv->base = (void *)devfdt_get_addr_ptr(bus);
-	k1_i2c_set_bus_speed(bus, priv->clk_rate);
+
+	speed = dev_read_u32_default(bus, "clock-frequency",
+				     I2C_SPEED_STANDARD_RATE);
+	k1_i2c_set_bus_speed(bus, speed);
+
 	return 0;
 }
 

-- 
2.55.0

Re: [PATCH v2 1/4] i2c: k1: fix wrong bus speed setting

[Yixun Lan]

hi Junhui,

On 11:40 Sat 15 Aug     , Junhui Liu wrote:
> The controller bus mode should be selected according to the requested
> I2C bus speed. However, the driver currently passes the functional clock
> rate to k1_i2c_set_bus_speed(), so the selected mode does not reflect
> the requested bus speed.
> 
> Fix this by reading the clock-frequency property from the Device Tree,
> defaulting to standard speed, and drop the unused clk_rate field.
> 
> Fixes: 271546fb8e54 ("i2c: k1: add I2C driver support")
> Reviewed-by: Heiko Schocher <hs@nabladev.com>
> Signed-off-by: Junhui Liu <junhui.liu@pigmoral.tech>
> ---
>  drivers/i2c/k1_i2c.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
Looks good to me
Reviewed-by: Yixun Lan <dlan@kernel.org>

-- 
Yixun Lan (dlan)

Re: [PATCH v2 1/4] i2c: k1: fix wrong bus speed setting

[Troy Mitchell]

[-- Attachment #1: Type: text/plain, Size: 1510 bytes --]

> The controller bus mode should be selected according to the requested
> I2C bus speed. However, the driver currently passes the functional clock
> rate to k1_i2c_set_bus_speed(), so the selected mode does not reflect
> the requested bus speed.
>
> Fix this by reading the clock-frequency property from the Device Tree,
> defaulting to standard speed, and drop the unused clk_rate field.

`clock-frequency` is the requested SCL rate, but this patch only changes the
value passed to `k1_i2c_set_bus_speed()`.

> @@ -496,10 +496,13 @@ static int k1_i2c_probe(struct udevice *bus)
>  		debug("%s: failed to enable clock\n", __func__);
>  		return ret;
>  	}
> -	priv->clk_rate = clk_get_rate(&priv->clk);
>
>  	priv->base = (void *)devfdt_get_addr_ptr(bus);
> -	k1_i2c_set_bus_speed(bus, priv->clk_rate);
> +
> +	speed = dev_read_u32_default(bus, "clock-frequency",
> +				     I2C_SPEED_STANDARD_RATE);
> +	k1_i2c_set_bus_speed(bus, speed);

`k1_i2c_set_bus_speed()` only changes `ICR_MODE_MASK`. It does not program
the ILCR divider or initialize IWCR, so the actual SCL rate still depends on
the register reset values and may not match the Device Tree.

Please calculate and program ILCR from the functional clock rate and the
requested SCL rate, initialize IWCR as required by the hardware, and reject
unsupported rates. The functional clock handle or rate therefore needs to
remain available to `k1_i2c_set_bus_speed()`.

                                            - Troy

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 248 bytes --]

Re: [PATCH v2 1/4] i2c: k1: fix wrong bus speed setting

[Junhui Liu]

Hi Troy,
Thanks for the review.

On Mon Aug 17, 2026 at 5:12 PM CST, Troy Mitchell wrote:
>> The controller bus mode should be selected according to the requested
>> I2C bus speed. However, the driver currently passes the functional clock
>> rate to k1_i2c_set_bus_speed(), so the selected mode does not reflect
>> the requested bus speed.
>>
>> Fix this by reading the clock-frequency property from the Device Tree,
>> defaulting to standard speed, and drop the unused clk_rate field.
>
> `clock-frequency` is the requested SCL rate, but this patch only changes the
> value passed to `k1_i2c_set_bus_speed()`.
>
>> @@ -496,10 +496,13 @@ static int k1_i2c_probe(struct udevice *bus)
>>  		debug("%s: failed to enable clock\n", __func__);
>>  		return ret;
>>  	}
>> -	priv->clk_rate = clk_get_rate(&priv->clk);
>>
>>  	priv->base = (void *)devfdt_get_addr_ptr(bus);
>> -	k1_i2c_set_bus_speed(bus, priv->clk_rate);
>> +
>> +	speed = dev_read_u32_default(bus, "clock-frequency",
>> +				     I2C_SPEED_STANDARD_RATE);
>> +	k1_i2c_set_bus_speed(bus, speed);
>
> `k1_i2c_set_bus_speed()` only changes `ICR_MODE_MASK`. It does not program
> the ILCR divider or initialize IWCR, so the actual SCL rate still depends on
> the register reset values and may not match the Device Tree.

Yes. This patch only aims to fix the ICR_MODE selection. On the K1 board
I tested, the register reset defaults are enough for both the PMIC and
the EEPROM to work under U-Boot.

>
> Please calculate and program ILCR from the functional clock rate and the
> requested SCL rate, initialize IWCR as required by the hardware, and reject
> unsupported rates. The functional clock handle or rate therefore needs to
> remain available to `k1_i2c_set_bus_speed()`.

That's a fair point. The current driver is aligned with the mainline
Linux K1 I2C driver. It does not include your later work to program
ILCR/IWCR:
https://lore.kernel.org/linux-riscv/20260508-k1-i2c-ilcr-v7-1-8c2dde5c3ed5@linux.spacemit.com/

Porting that change to U-Boot is not a small amount of work. I'd prefer
to keep that as a follow-up series rather than mix it into this ICR_MODE
fix.

>
>                                             - Troy

-- 
Best regards,
Junhui Liu

[PATCH v2 2/4] clk: spacemit: k1: add TWSI bus clocks to SPL

[Junhui Liu]

The K1 device tree describes separate functional and bus clocks for TWSI
controllers, but the SPL clock tree currently only provides their
functional clocks.

Add the bus clocks for TWSI2 and TWSI8, which are the TWSI controllers
currently used in SPL, along with their required parent clock chain.

TWSI8 uses a fixed-factor bus clock because its write-only clock control
register requires the functional and bus gate bits to remain combined in
a single clock instance.

Signed-off-by: Junhui Liu <junhui.liu@pigmoral.tech>
---
 drivers/clk/spacemit/clk-k1.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/clk/spacemit/clk-k1.c b/drivers/clk/spacemit/clk-k1.c
index 07adc126ee39..e239ffe6ba0d 100644
--- a/drivers/clk/spacemit/clk-k1.c
+++ b/drivers/clk/spacemit/clk-k1.c
@@ -154,11 +154,27 @@ CCU_GATE_DEFINE(CLK_PLL1_409P6, pll1_d6_409p6, pll1_d6_409p6, "pll1_d6",
 		MPMU_ACGR, BIT(0), 0);
 CCU_GATE_DEFINE(CLK_PLL1_307P2, pll1_d8_307p2, pll1_d8_307p2, "pll1_d8",
 		MPMU_ACGR, BIT(13), 0);
+CCU_FACTOR_GATE_DEFINE(CLK_PLL1_102P4, pll1_d24_102p4, pll1_d24_102p4,
+		       "pll1_d8", MPMU_ACGR, BIT(12), 3, 1);
+CCU_FACTOR_GATE_DEFINE(CLK_PLL1_51P2, pll1_d48_51p2, pll1_d48_51p2,
+		       "pll1_d8", MPMU_ACGR, BIT(7), 6, 1);
+CCU_FACTOR_GATE_DEFINE(CLK_PLL1_25P6, pll1_d96_25p6, pll1_d96_25p6,
+		       "pll1_d8", MPMU_ACGR, BIT(4), 12, 1);
 CCU_FACTOR_GATE_DEFINE(CLK_PLL1_31P5, pll1_d78_31p5, pll1_d78_31p5,
 		       "pll1_d4", MPMU_ACGR, BIT(6), 39, 2);
 CCU_DDN_DEFINE(CLK_SLOW_UART2, slow_uart2_48, slow_uart2_48,
 	       "pll1_d4_614p4", MPMU_SUCCR_1,
 	       CCU_DDN_MASK(16, 13), 16, CCU_DDN_MASK(0, 13), 0, 2, 0);
+
+static const char * const apb_parents[] = {
+	"pll1_d96_25p6",
+	"pll1_d48_51p2",
+	"pll1_d96_25p6",
+	"pll1_d24_102p4",
+};
+
+CCU_MUX_DEFINE(CLK_APB, apb_clk, apb_clk, apb_parents, ARRAY_SIZE(apb_parents),
+	       MPMU_APBCSCR, 0, 2, 0);
 #else
 CCU_GATE_DEFINE(CLK_PLL1_307P2, pll1_d8_307p2, pll1_d8_307p2, "pll1_d8",
 		MPMU_ACGR, BIT(13), 0);
@@ -306,6 +322,9 @@ CCU_MUX_GATE_DEFINE(CLK_TWSI2, twsi2_clk, twsi2_clk, twsi_parents,
  */
 CCU_GATE_DEFINE(CLK_TWSI8, twsi8_clk, twsi8_clk, "pll1_d78_31p5",
 		APBC_TWSI8_CLK_RST, BIT(1) | BIT(0), 0);
+CCU_GATE_DEFINE(CLK_TWSI2_BUS, twsi2_bus_clk, twsi2_bus_clk, "apb_clk",
+		APBC_TWSI2_CLK_RST, BIT(0), 0);
+CCU_FACTOR_DEFINE(CLK_TWSI8_BUS, twsi8_bus_clk, twsi8_bus_clk, "apb_clk", 1, 1);
 
 #else
 static const char * const uart_clk_parents[] = {
@@ -1232,8 +1251,12 @@ static struct clk *k1_ccu_mpmu_clks[] = {
 	&pll1_d4_614p4.common.clk,
 	&pll1_d6_409p6.common.clk,
 	&pll1_d8_307p2.common.clk,
+	&pll1_d24_102p4.common.clk,
+	&pll1_d48_51p2.common.clk,
+	&pll1_d96_25p6.common.clk,
 	&pll1_d78_31p5.common.clk,
 	&slow_uart2_48.common.clk,
+	&apb_clk.common.clk,
 };
 #else
 static struct clk *k1_ccu_mpmu_clks[] = {
@@ -1288,6 +1311,8 @@ static struct clk *k1_ccu_apbc_clks[] = {
 	&uart0_clk.common.clk,
 	&twsi2_clk.common.clk,
 	&twsi8_clk.common.clk,
+	&twsi2_bus_clk.common.clk,
+	&twsi8_bus_clk.common.clk,
 };
 #else
 static struct clk *k1_ccu_apbc_clks[] = {

-- 
2.55.0

[PATCH v2 3/4] i2c: k1: enable both functional and bus clocks

[Junhui Liu]

The K1 I2C controller requires both its functional clock and APB bus
clock to operate. The device tree provides them as "func" and "bus", but
the driver currently acquires and enables only the first clock.

Acquire both clocks by name and enable them during probe. Use explicit
named lookups instead of the bulk clock API to align with the K1 Linux
driver and keep the roles of the two clocks clear if functional clock
rate configuration is needed later.

Fixes: 271546fb8e54 ("i2c: k1: add I2C driver support")
Signed-off-by: Junhui Liu <junhui.liu@pigmoral.tech>
---
 drivers/i2c/k1_i2c.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/k1_i2c.c b/drivers/i2c/k1_i2c.c
index 2c7a1e0d3775..e2e4f9e53447 100644
--- a/drivers/i2c/k1_i2c.c
+++ b/drivers/i2c/k1_i2c.c
@@ -51,7 +51,6 @@ struct k1_i2c {
 struct k1_i2c_priv {
 	int id;
 	void __iomem *base;
-	struct clk clk;
 };
 
 /*
@@ -465,6 +464,7 @@ static int k1_i2c_probe(struct udevice *bus)
 {
 	struct k1_i2c_priv *priv = dev_get_priv(bus);
 	struct reset_ctl reset;
+	struct clk clk;
 	u32 speed;
 	int ret;
 
@@ -487,15 +487,21 @@ static int k1_i2c_probe(struct udevice *bus)
 		return ret;
 	}
 
-	ret = clk_get_by_index(bus, 0, &priv->clk);
+	ret = clk_get_by_name(bus, "func", &clk);
 	if (ret)
 		return ret;
 
-	ret = clk_enable(&priv->clk);
-	if (ret && ret != -ENOSYS && ret != -EOPNOTSUPP) {
-		debug("%s: failed to enable clock\n", __func__);
+	ret = clk_enable(&clk);
+	if (ret)
+		return ret;
+
+	ret = clk_get_by_name(bus, "bus", &clk);
+	if (ret)
+		return ret;
+
+	ret = clk_enable(&clk);
+	if (ret)
 		return ret;
-	}
 
 	priv->base = (void *)devfdt_get_addr_ptr(bus);
 

-- 
2.55.0

Re: [PATCH v2 3/4] i2c: k1: enable both functional and bus clocks

[Heiko Schocher via U-Boot]

Hello Junhui Liu,

On 15.08.26 05:40, Junhui Liu wrote:
> The K1 I2C controller requires both its functional clock and APB bus
> clock to operate. The device tree provides them as "func" and "bus", but
> the driver currently acquires and enables only the first clock.
> 
> Acquire both clocks by name and enable them during probe. Use explicit
> named lookups instead of the bulk clock API to align with the K1 Linux
> driver and keep the roles of the two clocks clear if functional clock
> rate configuration is needed later.
> 
> Fixes: 271546fb8e54 ("i2c: k1: add I2C driver support")
> Signed-off-by: Junhui Liu <junhui.liu@pigmoral.tech>
> ---
>   drivers/i2c/k1_i2c.c | 18 ++++++++++++------
>   1 file changed, 12 insertions(+), 6 deletions(-)

Reviewed-by: Heiko Schocher <hs@nabladev.com>

bye,
Heiko
-- 
Nabla Software Engineering
HRB 40522 Augsburg
Phone: +49 821 45592596
E-Mail: office@nabladev.com
Geschäftsführer : Stefano Babic

Re: [PATCH v2 3/4] i2c: k1: enable both functional and bus clocks

[Yixun Lan]

Hi Junhui,

On 11:40 Sat 15 Aug     , Junhui Liu wrote:
> The K1 I2C controller requires both its functional clock and APB bus
> clock to operate. The device tree provides them as "func" and "bus", but
> the driver currently acquires and enables only the first clock.
> 
> Acquire both clocks by name and enable them during probe. Use explicit
> named lookups instead of the bulk clock API to align with the K1 Linux
> driver and keep the roles of the two clocks clear if functional clock
> rate configuration is needed later.
> 
> Fixes: 271546fb8e54 ("i2c: k1: add I2C driver support")
> Signed-off-by: Junhui Liu <junhui.liu@pigmoral.tech>

Reviewed-by: Yixun Lan <dlan@kernel.org>

> ---
>  drivers/i2c/k1_i2c.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 

-- 
Yixun Lan (dlan)

Re: [PATCH v2 3/4] i2c: k1: enable both functional and bus clocks

[Troy Mitchell]

[-- Attachment #1: Type: text/plain, Size: 1274 bytes --]

> The K1 I2C controller requires both its functional clock and APB bus
> clock to operate. The device tree provides them as "func" and "bus", but
> the driver currently acquires and enables only the first clock.
>
> Acquire both clocks by name and enable them during probe. Use explicit
> named lookups instead of the bulk clock API to align with the K1 Linux
> driver and keep the roles of the two clocks clear if functional clock
> rate configuration is needed later.

[...]

> @@ -487,15 +487,21 @@ static int k1_i2c_probe(struct udevice *bus)
>  		return ret;
>  	}
>
> -	ret = clk_get_by_index(bus, 0, &priv->clk);
> +	ret = clk_get_by_name(bus, "func", &clk);
>  	if (ret)
>  		return ret;
>
> -	ret = clk_enable(&priv->clk);
> -	if (ret && ret != -ENOSYS && ret != -EOPNOTSUPP) {
> -		debug("%s: failed to enable clock\n", __func__);
> +	ret = clk_enable(&clk);
> +	if (ret)
> +		return ret;
> +
> +	ret = clk_get_by_name(bus, "bus", &clk);
> +	if (ret)
> +		return ret;
> +
> +	ret = clk_enable(&clk);
> +	if (ret)
>  		return ret;
> -	}

Once the functional clock has been enabled, failure to acquire or enable
the bus clock returns without disabling the functional clock.

                                            - Troy

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 248 bytes --]

Re: [PATCH v2 3/4] i2c: k1: enable both functional and bus clocks

[Yixun Lan]

hi Troy,

On 16:56 Mon 17 Aug     , Troy Mitchell wrote:
> > The K1 I2C controller requires both its functional clock and APB bus
> > clock to operate. The device tree provides them as "func" and "bus", but
> > the driver currently acquires and enables only the first clock.
> >
> > Acquire both clocks by name and enable them during probe. Use explicit
> > named lookups instead of the bulk clock API to align with the K1 Linux
> > driver and keep the roles of the two clocks clear if functional clock
> > rate configuration is needed later.
> 
> [...]
> 
> > @@ -487,15 +487,21 @@ static int k1_i2c_probe(struct udevice *bus)
> >  		return ret;
> >  	}
> >
> > -	ret = clk_get_by_index(bus, 0, &priv->clk);
> > +	ret = clk_get_by_name(bus, "func", &clk);
> >  	if (ret)
> >  		return ret;
> >
> > -	ret = clk_enable(&priv->clk);
> > -	if (ret && ret != -ENOSYS && ret != -EOPNOTSUPP) {
> > -		debug("%s: failed to enable clock\n", __func__);
> > +	ret = clk_enable(&clk);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = clk_get_by_name(bus, "bus", &clk);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = clk_enable(&clk);
> > +	if (ret)
> >  		return ret;
> > -	}
> 
> Once the functional clock has been enabled, failure to acquire or enable
> the bus clock returns without disabling the functional clock.
> 
I'd not worry about this, as in uboot it isn't worth the effort to do the
resouce reclamation, instead I'm more concerned about the error handling..


-- 
Yixun Lan (dlan)

Re: [PATCH v2 3/4] i2c: k1: enable both functional and bus clocks

[Yao Zi]

On Sat, Aug 15, 2026 at 11:40:23AM +0800, Junhui Liu wrote:
> The K1 I2C controller requires both its functional clock and APB bus
> clock to operate. The device tree provides them as "func" and "bus", but
> the driver currently acquires and enables only the first clock.
> 
> Acquire both clocks by name and enable them during probe. Use explicit
> named lookups instead of the bulk clock API to align with the K1 Linux
> driver and keep the roles of the two clocks clear if functional clock
> rate configuration is needed later.
> 
> Fixes: 271546fb8e54 ("i2c: k1: add I2C driver support")
> Signed-off-by: Junhui Liu <junhui.liu@pigmoral.tech>
> ---
>  drivers/i2c/k1_i2c.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/i2c/k1_i2c.c b/drivers/i2c/k1_i2c.c
> index 2c7a1e0d3775..e2e4f9e53447 100644
> --- a/drivers/i2c/k1_i2c.c
> +++ b/drivers/i2c/k1_i2c.c
> @@ -51,7 +51,6 @@ struct k1_i2c {
>  struct k1_i2c_priv {
>  	int id;
>  	void __iomem *base;
> -	struct clk clk;
>  };
>  
>  /*
> @@ -465,6 +464,7 @@ static int k1_i2c_probe(struct udevice *bus)
>  {
>  	struct k1_i2c_priv *priv = dev_get_priv(bus);
>  	struct reset_ctl reset;
> +	struct clk clk;
>  	u32 speed;
>  	int ret;
>  
> @@ -487,15 +487,21 @@ static int k1_i2c_probe(struct udevice *bus)
>  		return ret;
>  	}
>  
> -	ret = clk_get_by_index(bus, 0, &priv->clk);
> +	ret = clk_get_by_name(bus, "func", &clk);
>  	if (ret)
>  		return ret;
>  
> -	ret = clk_enable(&priv->clk);
> -	if (ret && ret != -ENOSYS && ret != -EOPNOTSUPP) {
> -		debug("%s: failed to enable clock\n", __func__);
> +	ret = clk_enable(&clk);
> +	if (ret)
> +		return ret;
> +
> +	ret = clk_get_by_name(bus, "bus", &clk);
> +	if (ret)
> +		return ret;
> +
> +	ret = clk_enable(&clk);
> +	if (ret)

I prefer to have something logged when any of these operations fail so
it's easer to debug. With dev_* or log_* APIs, they should be optimized
out and taken no extra space if they don't reach the logging level.

But anyway, we currently have many drivers silently bail out in such
cases, so this isn't a hard requirement.

Best regards,
Yao Zi

>  		return ret;
> -	}
>  
>  	priv->base = (void *)devfdt_get_addr_ptr(bus);
>  
> 
> -- 
> 2.55.0
> 

Re: [PATCH v2 3/4] i2c: k1: enable both functional and bus clocks

[Yao Zi]

On Tue, Aug 18, 2026 at 06:21:17AM +0000, Yao Zi wrote:
> On Sat, Aug 15, 2026 at 11:40:23AM +0800, Junhui Liu wrote:
> > The K1 I2C controller requires both its functional clock and APB bus
> > clock to operate. The device tree provides them as "func" and "bus", but
> > the driver currently acquires and enables only the first clock.
> > 
> > Acquire both clocks by name and enable them during probe. Use explicit
> > named lookups instead of the bulk clock API to align with the K1 Linux
> > driver and keep the roles of the two clocks clear if functional clock
> > rate configuration is needed later.
> > 
> > Fixes: 271546fb8e54 ("i2c: k1: add I2C driver support")
> > Signed-off-by: Junhui Liu <junhui.liu@pigmoral.tech>
> > ---
> >  drivers/i2c/k1_i2c.c | 18 ++++++++++++------
> >  1 file changed, 12 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/i2c/k1_i2c.c b/drivers/i2c/k1_i2c.c
> > index 2c7a1e0d3775..e2e4f9e53447 100644
> > --- a/drivers/i2c/k1_i2c.c
> > +++ b/drivers/i2c/k1_i2c.c
> > @@ -51,7 +51,6 @@ struct k1_i2c {
> >  struct k1_i2c_priv {
> >  	int id;
> >  	void __iomem *base;
> > -	struct clk clk;
> >  };
> >  
> >  /*
> > @@ -465,6 +464,7 @@ static int k1_i2c_probe(struct udevice *bus)
> >  {
> >  	struct k1_i2c_priv *priv = dev_get_priv(bus);
> >  	struct reset_ctl reset;
> > +	struct clk clk;
> >  	u32 speed;
> >  	int ret;
> >  
> > @@ -487,15 +487,21 @@ static int k1_i2c_probe(struct udevice *bus)
> >  		return ret;
> >  	}
> >  
> > -	ret = clk_get_by_index(bus, 0, &priv->clk);
> > +	ret = clk_get_by_name(bus, "func", &clk);
> >  	if (ret)
> >  		return ret;
> >  
> > -	ret = clk_enable(&priv->clk);
> > -	if (ret && ret != -ENOSYS && ret != -EOPNOTSUPP) {
> > -		debug("%s: failed to enable clock\n", __func__);
> > +	ret = clk_enable(&clk);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = clk_get_by_name(bus, "bus", &clk);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = clk_enable(&clk);
> > +	if (ret)
> 
> I prefer to have something logged when any of these operations fail so
> it's easer to debug. With dev_* or log_* APIs, they should be optimized
> out and taken no extra space if they don't reach the logging level.

Oops, it seems drivers/clk/clk-uclass.c does some sort of logging already.
So please ignore this comment.

Regards,
Yao Zi

> But anyway, we currently have many drivers silently bail out in such
> cases, so this isn't a hard requirement.
> 
> Best regards,
> Yao Zi
> 
> >  		return ret;
> > -	}
> >  
> >  	priv->base = (void *)devfdt_get_addr_ptr(bus);
> >  
> > 
> > -- 
> > 2.55.0
 > 

[PATCH v2 4/4] clk: spacemit: k1: remove bus gate from TWSI functional clocks

[Junhui Liu]

The TWSI functional clocks incorrectly include both the functional clock
enable bit and the bus clock enable bit in their gate masks. The device
tree and the full clock tree model the bus clocks separately.

Remove the bus clock enable bit from the functional clock gate masks so
that the functional and bus clocks are controlled independently.

Keep the combined gate for TWSI8 because its clock control register is
write-only and therefore cannot be safely shared by two clock
instances.

Fixes: 3aa2882a3e1a ("clk: spacemit: Add support for K1 SoC")
Signed-off-by: Junhui Liu <junhui.liu@pigmoral.tech>
---
 drivers/clk/spacemit/clk-k1.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/clk/spacemit/clk-k1.c b/drivers/clk/spacemit/clk-k1.c
index e239ffe6ba0d..20b8595aa3c4 100644
--- a/drivers/clk/spacemit/clk-k1.c
+++ b/drivers/clk/spacemit/clk-k1.c
@@ -314,7 +314,7 @@ static const char * const twsi_parents[] = {
 
 CCU_MUX_GATE_DEFINE(CLK_TWSI2, twsi2_clk, twsi2_clk, twsi_parents,
 		    ARRAY_SIZE(twsi_parents), APBC_TWSI2_CLK_RST,
-		    4, 3, BIT(1) | BIT(0), 0);
+		    4, 3, BIT(1), 0);
 /*
  * APBC_TWSI8_CLK_RST has a quirk that reading always results in zero.
  * Combine functional and bus bits together as a gate to avoid sharing the
@@ -345,7 +345,7 @@ static const char * const twsi_parents[] = {
 
 CCU_MUX_GATE_DEFINE(CLK_TWSI2, twsi2_clk, twsi2_clk, twsi_parents,
 		    ARRAY_SIZE(twsi_parents), APBC_TWSI2_CLK_RST,
-		    4, 3, BIT(1) | BIT(0), 0);
+		    4, 3, BIT(1), 0);
 /*
  * APBC_TWSI8_CLK_RST has a quirk that reading always results in zero.
  * Combine functional and bus bits together as a gate to avoid sharing the
@@ -467,22 +467,22 @@ CCU_GATE_DEFINE(CLK_RTC, rtc_clk, rtc_clk, "clock-32k", APBC_RTC_CLK_RST,
 
 CCU_MUX_GATE_DEFINE(CLK_TWSI0, twsi0_clk, twsi0_clk, twsi_parents,
 		    ARRAY_SIZE(twsi_parents), APBC_TWSI0_CLK_RST,
-		    4, 3, BIT(1) | BIT(0), 0);
+		    4, 3, BIT(1), 0);
 CCU_MUX_GATE_DEFINE(CLK_TWSI1, twsi1_clk, twsi1_clk, twsi_parents,
 		    ARRAY_SIZE(twsi_parents), APBC_TWSI1_CLK_RST,
-		    4, 3, BIT(1) | BIT(0), 0);
+		    4, 3, BIT(1), 0);
 CCU_MUX_GATE_DEFINE(CLK_TWSI4, twsi4_clk, twsi4_clk, twsi_parents,
 		    ARRAY_SIZE(twsi_parents), APBC_TWSI4_CLK_RST,
-		    4, 3, BIT(1) | BIT(0), 0);
+		    4, 3, BIT(1), 0);
 CCU_MUX_GATE_DEFINE(CLK_TWSI5, twsi5_clk, twsi5_clk, twsi_parents,
 		    ARRAY_SIZE(twsi_parents), APBC_TWSI5_CLK_RST,
-		    4, 3, BIT(1) | BIT(0), 0);
+		    4, 3, BIT(1), 0);
 CCU_MUX_GATE_DEFINE(CLK_TWSI6, twsi6_clk, twsi6_clk, twsi_parents,
 		    ARRAY_SIZE(twsi_parents), APBC_TWSI6_CLK_RST,
-		    4, 3, BIT(1) | BIT(0), 0);
+		    4, 3, BIT(1), 0);
 CCU_MUX_GATE_DEFINE(CLK_TWSI7, twsi7_clk, twsi7_clk, twsi_parents,
 		    ARRAY_SIZE(twsi_parents), APBC_TWSI7_CLK_RST,
-		    4, 3, BIT(1) | BIT(0), 0);
+		    4, 3, BIT(1), 0);
 
 static const char * const timer_parents[] = {
 	"pll1_d192_12p8",

-- 
2.55.0

Re: [PATCH v2 4/4] clk: spacemit: k1: remove bus gate from TWSI functional clocks

[Troy Mitchell]

[-- Attachment #1: Type: text/plain, Size: 635 bytes --]

> The TWSI functional clocks incorrectly include both the functional clock
> enable bit and the bus clock enable bit in their gate masks. The device
> tree and the full clock tree model the bus clocks separately.
>
> Remove the bus clock enable bit from the functional clock gate masks so
> that the functional and bus clocks are controlled independently.
>
> Keep the combined gate for TWSI8 because its clock control register is
> write-only and therefore cannot be safely shared by two clock
> instances.
Reviewed-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>

                                            - Troy

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 248 bytes --]

Re: [PATCH v2 0/4] i2c/clk: spacemit: k1: fix I2C bus configuration and clock handling

[Yao Zi]

On Sat, Aug 15, 2026 at 11:40:20AM +0800, Junhui Liu wrote:
> Fix two issues in the SpacemiT K1 I2C driver and clock provider.
> 
> The first issue is that the controller bus mode is selected using the
> functional clock rate instead of the requested I2C bus speed from the
> device tree.
> 
> The second issue is that the TWSI functional clocks incorrectly include
> the bus clock enable bit in their gate masks. As a result the I2C driver
> can operate with only the functional clock enabled, which diverges from
> the device tree description and the Linux driver (both expect separate
> "func" and "bus" clocks).
> 
> To fix the second issue without breaking intermediate states, the
> patches are ordered as follows:
> 
> - First add the missing bus clocks (and their parent chain) to the SPL
>   clock tree.
> - Then update the I2C driver to acquire and enable both the functional
>   and bus clocks by name.
> - Finally remove the bus gate bit from the functional clock definitions
>   (except TWSI8, whose write-only register requires the combined gate).

Personally I think it's okay to break booting in this case as long as
you explicitly point it out. Merging the conceptually related PATCH 2
and PATCH 4 which together fix description of TWSI controller clocks
would make the commit description shorter and clearer. But it depends on
you.

For the whole series,

Reviewed-by: Yao Zi <me@ziyao.cc>

Regards,
Yao Zi