Re: [PATCH 2/2] i2c: piix4: Use usleep_range()

[Jean Delvare]

Hi Guenter,

On Sat, 30 Dec 2017 08:50:58 -0800, Guenter Roeck wrote:
> The piix4 i2c driver is extremely slow. Replacing msleep()
> with usleep_range() increases its speed substantially.
> 
> Signed-off-by: Guenter Roeck <linux@xxxxxxxxxxxx>
> ---
>  drivers/i2c/busses/i2c-piix4.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
> index 78dd5951d6e7..52a8b1c5c110 100644
> --- a/drivers/i2c/busses/i2c-piix4.c
> +++ b/drivers/i2c/busses/i2c-piix4.c
> @@ -467,13 +467,13 @@ static int piix4_transaction(struct i2c_adapter *piix4_adapter)
>  
>  	/* We will always wait for a fraction of a second! (See PIIX4 docs errata) */
>  	if (srvrworks_csb5_delay) /* Extra delay for SERVERWORKS_CSB5 */
> -		msleep(2);
> +		usleep_range(2000, 2000);

Isn't this exactly the same? I'm fine using the same function for
consistency, just curious.

>  	else
> -		msleep(1);
> +		usleep_range(500, 1000);

Were you able to test this on older hardware? Unfortunately, the
specification errata of the original Intel PIIX4 is quite vague on the
amount of time you must wait before checking the Host Busy bit:

"Note that there may be moderate latency before the transaction begins
and the Host Busy bit gets set."

I guess we made it 1 ms at the time because it was the minimum we could
sleep anyway.

One option if you really care about the performance of the i2c-piix4
driver on recent hardware would be to lower the initial delay even more
for ATI and AMD chipsets. The errata was for Intel chipsets originally,
and while we know that at least some of the ServerWorks implementations
suffered from the same problem (worse actually) I don't think that
anybody ever bothered checking if it applied to more recent
implementations by other vendors.

For reference, at 93.75 kHz (the default SMBus frequency or the SB800),
an SMBus Quick transaction would be completed in 117 us, so I guess an
initial delay of 150 or 200 us would be optimum. And an SMBus Read Byte
transaction completes in 416 ms. I think this is the most popular SMBus
transaction, so ensuring that it is as fast as possible would make
sense.

And it might even work on older Intel chipsets, who knows. Plus I doubt
anyone is still using them anyway, so you have my approval to lower the
delays to whatever works for you.

As a comparison point, in the i2c-i801 driver we use:

	usleep_range(250, 500);

for both the initial sleep and the waiting loop.

>  
>  	while ((++timeout < MAX_TIMEOUT) &&
>  	       ((temp = inb_p(SMBHSTSTS)) & 0x01))
> -		msleep(1);
> +		usleep_range(200, 500);

Note that you are also drastically reducing the effective timeout value
here, because it is a counter and not an overall time. Beforehand, we
would wait for at least 501 ms for the transaction to complete. Now
this could be down to only 100 ms. That being said, if my math is
right, the longest supported transaction (Block Read) at the slowest
allowed SMBus clock speed (10 kHz) would be done in 33 ms, so we are
still good.

>  
>  	/* If the SMBus is still busy, we give up */
>  	if (timeout == MAX_TIMEOUT) {

Reviewed-by: Jean Delvare <jdelvare@xxxxxxx>

-- 
Jean Delvare
SUSE L3 Support