linux-kernel - Re: [PATCH 3/3] i2c: rcar: add FastMode+ support

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230905213710.3dv5h6zvwu4tpnby@zenone.zhora.eu>
Date:   Tue, 5 Sep 2023 23:37:10 +0200
From:   Andi Shyti <andi.shyti@...nel.org>
To:     Wolfram Sang <wsa+renesas@...g-engineering.com>
Cc:     linux-renesas-soc@...r.kernel.org, linux-i2c@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] i2c: rcar: add FastMode+ support

Hi Wolfram,

[...]

> @@ -217,7 +228,17 @@ static void rcar_i2c_init(struct rcar_i2c_priv *priv)
>  	rcar_i2c_write(priv, ICMCR, MDBS);
>  	rcar_i2c_write(priv, ICMSR, 0);
>  	/* start clock */
> -	rcar_i2c_write(priv, ICCCR, priv->icccr);
> +	if (priv->flags & ID_P_FMPLUS) {
> +		rcar_i2c_write(priv, ICCCR, 0);
> +		rcar_i2c_write(priv, ICMPR, priv->clock_val);
> +		rcar_i2c_write(priv, ICHPR, 3 * priv->clock_val);
> +		rcar_i2c_write(priv, ICLPR, 3 * priv->clock_val);
> +		rcar_i2c_write(priv, ICCCR2, FMPE | CDFD | HLSE | SME);
> +	} else {
> +		rcar_i2c_write(priv, ICCCR, priv->clock_val);
> +		if (priv->devtype >= I2C_RCAR_GEN3)
> +			rcar_i2c_write(priv, ICCCR2, 0);

is this last bit part of the FM+ enabling or is it part of the
GEN4 support?

> +	}

[...]

> +	if (priv->flags & ID_P_FMPLUS) {
> +		/*
> +		 * SMD should be smaller than SCLD and SCHD, we arbitrarily set
> +		 * the ratio 1:3. SCHD:SCLD ratio is 1:1, thus:
> +		 * SCL	= clkp / (8 + SMD * 2 + SCLD + SCHD + F[(ticf + tr + intd) * clkp])
> +		 * SCL	= clkp / (8 + SMD * 2 + SMD * 3 + SMD * 3 + F[...])
> +		 * SCL	= clkp / (8 + SMD * 8 + F[...])
> +		 */
> +		smd = DIV_ROUND_UP(ick / t.bus_freq_hz - 8 - round, 8);
> +		scl = ick / (8 + 8 * smd + round);
>  
> -	if (scgd == 0x40) {
> -		dev_err(dev, "it is impossible to calculate best SCL\n");
> -		return -EIO;
> -	}
> +		if (smd > 0xff) {
> +			dev_err(dev, "it is impossible to calculate best SCL\n");
> +			return -EINVAL;
> +		}
>  
> -	dev_dbg(dev, "clk %d/%d(%lu), round %u, CDF:0x%x, SCGD: 0x%x\n",
> -		scl, t.bus_freq_hz, rate, round, cdf, scgd);
> +		dev_dbg(dev, "clk %d/%d(%lu), round %u, SMD:0x%x, SCHD: 0x%x\n",
> +			scl, t.bus_freq_hz, rate, round, smd, 3 * smd);

I trust the formula application is right here... I can't say much :)
I don't see anything odd here.

>  
> -	/* keep icccr value */
> -	priv->icccr = scgd << cdf_width | cdf;
> +		priv->clock_val = smd;
> +	} else {
> +		/*
> +		 * SCL	= ick / (20 + SCGD * 8 + F[(ticf + tr + intd) * ick])
> +		 *
> +		 * Calculation result (= SCL) should be less than
> +		 * bus_speed for hardware safety
> +		 *
> +		 * We could use something along the lines of
> +		 *	div = ick / (bus_speed + 1) + 1;
> +		 *	scgd = (div - 20 - round + 7) / 8;
> +		 *	scl = ick / (20 + (scgd * 8) + round);
> +		 * (not fully verified) but that would get pretty involved
> +		 */
> +		for (scgd = 0; scgd < 0x40; scgd++) {
> +			scl = ick / (20 + (scgd * 8) + round);
> +			if (scl <= t.bus_freq_hz)
> +				break;
> +		}
> +
> +		if (scgd == 0x40) {

would be nice to give a meaning to this 0x40 constant... either
having it in a define or a comment, at least.

Andi

> +			dev_err(dev, "it is impossible to calculate best SCL\n");
> +			return -EINVAL;
> +		}