U-Boot Archive on lore.kernel.org
 help / color / mirror / Atom feed
From: "Junhui Liu" <junhui.liu@pigmoral.tech>
To: "Troy Mitchell" <troy.mitchell@linux.spacemit.com>,
	"Junhui Liu" <junhui.liu@pigmoral.tech>
Cc: "Heiko Schocher" <hs@nabladev.com>, <u-boot-spacemit@groups.io>,
	<u-boot@lists.u-boot-project.org>,
	"Huan Zhou" <pericycle.cc@gmail.com>,
	"Guodong Xu" <guodong.xu@oss.qualcomm.com>,
	"Tom Rini" <trini@konsulko.com>,
	"Raymond Mao" <raymond.mao@riscstar.com>,
	"Lukasz Majewski" <lukma@denx.de>, "Yixun Lan" <dlan@kernel.org>
Subject: Re: [PATCH v2 1/4] i2c: k1: fix wrong bus speed setting
Date: Mon, 17 Aug 2026 22:25:30 +0800	[thread overview]
Message-ID: <DKRADSUN4GN9.299FCK1VIS6EH@pigmoral.tech> (raw)
In-Reply-To: <DKR3Q808Z4E0.27KFK7LNAM3K7@linux.spacemit.com>

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

  reply	other threads:[~2026-08-17 14:26 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-08-15  3:40 [PATCH v2 0/4] i2c/clk: spacemit: k1: fix I2C bus configuration and clock handling Junhui Liu
2026-08-15  3:40 ` [PATCH v2 1/4] i2c: k1: fix wrong bus speed setting Junhui Liu
2026-08-17  8:29   ` Yixun Lan
2026-08-17  9:12   ` Troy Mitchell
2026-08-17 14:25     ` Junhui Liu [this message]
2026-08-18  1:03       ` Troy Mitchell
2026-08-15  3:40 ` [PATCH v2 2/4] clk: spacemit: k1: add TWSI bus clocks to SPL Junhui Liu
2026-08-15  3:40 ` [PATCH v2 3/4] i2c: k1: enable both functional and bus clocks Junhui Liu
2026-08-17  4:50   ` Heiko Schocher via U-Boot
2026-08-17  8:31   ` Yixun Lan
2026-08-17  8:56   ` Troy Mitchell
2026-08-17  9:18     ` Yixun Lan
2026-08-18  1:03       ` Troy Mitchell
2026-08-18  6:21   ` Yao Zi
2026-08-18  6:26     ` Yao Zi
2026-08-15  3:40 ` [PATCH v2 4/4] clk: spacemit: k1: remove bus gate from TWSI functional clocks Junhui Liu
2026-08-17  8:55   ` Troy Mitchell
2026-08-18  6:53 ` [PATCH v2 0/4] i2c/clk: spacemit: k1: fix I2C bus configuration and clock handling Yao Zi

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=DKRADSUN4GN9.299FCK1VIS6EH@pigmoral.tech \
    --to=junhui.liu@pigmoral.tech \
    --cc=dlan@kernel.org \
    --cc=guodong.xu@oss.qualcomm.com \
    --cc=hs@nabladev.com \
    --cc=lukma@denx.de \
    --cc=pericycle.cc@gmail.com \
    --cc=raymond.mao@riscstar.com \
    --cc=trini@konsulko.com \
    --cc=troy.mitchell@linux.spacemit.com \
    --cc=u-boot-spacemit@groups.io \
    --cc=u-boot@lists.u-boot-project.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox