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

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