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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <54384E7E7AD6D77C+aHRZKYHMHTiHXPWW@LT-Guozexi>
Date: Mon, 14 Jul 2025 09:11:05 +0800
From: Troy Mitchell <troy.mitchell@...ux.spacemit.com>
To: Andi Shyti <andi.shyti@...nel.org>,
	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 Andi, Thanks for your review!

On Fri, Jul 11, 2025 at 11:20:48AM +0200, Andi Shyti wrote:
> 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?
I removed whitespaces

> 
> >  /* 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?
Yes, I'll check it in the next version.

> 
> > +	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.
Yes we can force the mode to fast.

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