linux-kernel - Re: [PATCH] i2c: spacemit: configure ILCR for accurate SCL frequency

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <bf6nkxdfctgz2tacf3vm6tv4qhd72vbqj52gu6kt2fc5erltkw@nufl7i3bddkb>
Date: Fri, 11 Jul 2025 11:20:48 +0200
From: Andi Shyti <andi.shyti@...nel.org>
To: Troy Mitchell <troy.mitchell@...ux.spacemit.com>
Cc: Yixun Lan <dlan@...too.org>, linux-i2c@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-riscv@...ts.infradead.org, spacemit@...ts.linux.dev
Subject: Re: [PATCH] i2c: spacemit: configure ILCR for accurate SCL frequency

Hi Troy,

> - #include <linux/clk.h>
> - #include <linux/i2c.h>
> - #include <linux/iopoll.h>
> - #include <linux/module.h>
> - #include <linux/of_address.h>
> - #include <linux/platform_device.h>
> +#include "linux/bits.h"

this should be <linux/bits.h>

> +#include <linux/clk.h>
> +#include <linux/clkdev.h>
> +#include <linux/clk-provider.h>
> +#include <linux/i2c.h>
> +#include <linux/iopoll.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>

Is the diff a bit noisy or am I failing to see some differences?

>  /* spacemit i2c registers */
>  #define SPACEMIT_ICR		 0x0		/* Control register */
>  #define SPACEMIT_ISR		 0x4		/* Status register */
>  #define SPACEMIT_IDBR		 0xc		/* Data buffer register */
> +#define SPACEMIT_ILCR		 0x10		/* Load Count Register */
>  #define SPACEMIT_IBMR		 0x1c		/* Bus monitor register */

...

> +static int spacemit_i2c_clk_set_rate(struct clk_hw *hw, unsigned long rate,
> +				unsigned long parent_rate)

here the alignment is a bit off.

> +{
> +	struct spacemit_i2c_dev *i2c = container_of(hw, struct spacemit_i2c_dev, scl_clk_hw);
> +	u32 lv, lcr;
> +
> +	lv = DIV_ROUND_UP(parent_rate, rate);

do we want to sanity check the lv value here?

> +	lcr = readl(i2c->base + SPACEMIT_ILCR);
> +	if (i2c->mode == SPACEMIT_MODE_STANDARD) {
> +		lcr &= ~SPACEMIT_LCR_LV_STANDARD_MASK;
> +		lcr |= lv << SPACEMIT_LCR_LV_STANDARD_SHIFT;
> +	} else if (i2c->mode == SPACEMIT_MODE_FAST) {
> +		lcr &= ~SPACEMIT_LCR_LV_FAST_MASK;
> +		lcr |= lv << SPACEMIT_LCR_LV_FAST_SHIFT;
> +	}
> +	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)

Alignment.

> +{
> +	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)

alignment.

> +{

...

> @@ -519,15 +636,13 @@ 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);
> -		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;
> -	}
> +	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
> +		return -EINVAL;

Should we print an error here? And how likely is this case to
happen? Perhaps we can force the mode even if the frequency is
off and move forward instead of forcing the failure.

Andi

>  
>  	i2c->dev = &pdev->dev;