linux-kernel - Re: [PATCH v3 5/6] i2c: npcm: use i2c frequency table

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: <ZuQWYabomv_xyYyj@smile.fi.intel.com>
Date: Fri, 13 Sep 2024 13:39:29 +0300
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: warp5tw@...il.com
Cc: avifishman70@...il.com, tmaimon77@...il.com, tali.perry1@...il.com,
	venture@...gle.com, yuenn@...gle.com, benjaminfair@...gle.com,
	andi.shyti@...nel.org, wsa@...nel.org, rand.sec96@...il.com,
	wsa+renesas@...g-engineering.com, tali.perry@...oton.com,
	Avi.Fishman@...oton.com, tomer.maimon@...oton.com,
	KWLIU@...oton.com, JJLIU0@...oton.com, kfting@...oton.com,
	openbmc@...ts.ozlabs.org, linux-i2c@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 5/6] i2c: npcm: use i2c frequency table

On Fri, Sep 13, 2024 at 06:14:45PM +0800, warp5tw@...il.com wrote:
> From: Tyrone Ting <kfting@...oton.com>
> 
> Modify i2c frequency from table parameters
> for NPCM i2c modules.
> 
> Supported frequencies are:
> 
> 1. 100KHz
> 2. 400KHz
> 3. 1MHz
> 
> The original equations were tested on a variety of chips and base clocks.
> Since we added devices that use higher frequencies of the module we
> saw that there is a mismatch between the equation and the actual
> results on the bus itself, measured on scope.
> 
> Meanwhile, the equations were not accurate to begin with.
> They are an approximation of the ideal value. The ideal value is
> calculated per frequency of the core module.
> 
> So instead of using the equations we did an optimization per module
> frequency, verified on a device.
> 
> Most of the work was focused on the rise time of the SCL and SDA,
> which depends on external load of the bus and PU.
> 
> Different PCB designs, or specifically to this case: the number
> and type of targets on the bus, impact the required values for
> the timing registers.
> 
> Users can recalculate the numbers for each bus and get an even better
> optimization, but our users chose not to.
> 
> We manually picked values per frequency that match the entire valid
> range of targets (from 1 to max number). Then we check against the
> AMR described in SMB spec and make sure that none of the values
> is exceeding.
> 
> This process was led by the chip architect and included a lot of testing.

...

> +	{.core_clk = 100000000, .hldt = 0x2A, .dbcnt = 0x4, .sclfrq = 0xFB, .scllt = 0x0,
> +	.sclht  = 0x0, .fast_mode = false },

It seems you have no leading space, while having trailing one. Also the split
seems a bit illogical to me, I would rather do like

	{
		.core_clk = 100000000, .hldt = 0x2A, .dbcnt = 0x4,
		.sclfrq = 0xFB, .scllt = 0x0, .sclht  = 0x0,
		.fast_mode = false,
	},

Yes, this takes more lines, but also more flexible to the updates in the
future. Also mind the trailing comma at the last member assignment.

...

> +	for (scl_table_cnt = 0 ; scl_table_cnt < table_size ; scl_table_cnt++)

Extra spaces...

> +		if (bus->apb_clk >= smb_timing[scl_table_cnt].core_clk)
> +			break;

...

>  	/* bits [8:7] are in I2CCTL3 reg */
> -	iowrite8(fast_mode | FIELD_PREP(I2CCTL3_SCLFRQ8_7, (sclfrq >> 7) & 0x3),
> +	iowrite8(fast_mode | FIELD_PREP(I2CCTL3_SCLFRQ8_7, (smb_timing[scl_table_cnt].sclfrq >> 7)
> +		 & 0x3),
>  		 bus->reg + NPCM_I2CCTL3);

This has broken (illogical) indentation. Consider

	iowrite8(FIELD_PREP(I2CCTL3_SCLFRQ8_7, (smb_timing[scl_table_cnt].sclfrq >> 7) & 0x3) |
		 fast_mode,
		 bus->reg + NPCM_I2CCTL3);

-- 
With Best Regards,
Andy Shevchenko