i2c-i801: Re-read busy bit and wait for transaction to complete
PATCH 1/1 i2c-i801: Re-read busy bit and wait for transaction to complete
Intel ESB2 SMBus chip seems to have an issue where it momentarily resets the HOST_BUSY bit in the host status register. This confuses the driver waiting for an SMBus transaction to complete. This patch adds a workaround for the same.
--- drivers/i2c/busses/i2c-i801.c | 23 +++++++++++++++++++++++ 1 files changed, 23 insertions(+), 0 deletions(-)
drivers/i2c/busses/i2c-i801.c +23 -0
@@ -222,6 +222,7 @@ static int i801_transaction(int xact)int status;int result;int timeout =0 ; +int counter =0 ; result =i801_check_pre ();if (result <0 )
@@ -231,12 +232,34 @@ static int i801_transaction(int xact)* INTREN, SMBSCMD are passed in xact */ outb_p (xact |I801_START ,SMBHSTCNT ); +try_again:/* We will always wait for a fraction of a second! */ do {msleep (1 ); status =inb_p (SMBHSTSTS ); }while ((status &SMBHSTSTS_HOST_BUSY ) && (timeout++ <MAX_TIMEOUT )); +/* The i801 chip resets the HOST_BUSY bit +* to indicate that it has completed the transaction, +* but a moment later sets it again. Seems like a glitch. +* Changed code to check the value more times if its not a timeout. +*/ +if (timeout <MAX_TIMEOUT ) { +msleep (1 ); + status =inb_p (SMBHSTSTS ); +if (status &SMBHSTSTS_HOST_BUSY ) { +dev_warn (&I801_dev->dev,"Busy bit set again" +"(%02x)\n" , status); +if (++counter <3 ) { +dev_info (&I801_dev->dev,"Trying" +"again\n" ); + goto try_again; + } +dev_err (&I801_dev->dev,"No use" +" retrying-(%02x)\n" , status); + } + } + result =i801_check_post (status, timeout >MAX_TIMEOUT );if (result <0 )return result;
Signature · 1 line
1.6.0.4
Hi Chaitanya,
Quoted 2 lines · Chaitanya Lala
Intel ESB2 SMBus chip seems to have an issue where it momentarily resets the
HOST_BUSYbit in the host status register.
Are you certain? There is a known erratum on some of the ICH chips which is slightly different. The erratum is about the HOST_BUSY flag not being set immediately when starting a new transaction, so there is the chance that the first check concludes that the transaction is already over. This sounds somewhat similar to the problem you're seeing. One big difference though is that the glitch can only happen at the beginning of the transaction, according to the erratum.
Quoted 46 lines
This confuses
the driver waiting for an SMBus transaction to complete. This patch adds a workaround for the same.--- drivers/i2c/busses/i2c-i801.c | 23 +++++++++++++++++++++++ 1 files changed, 23 insertions(+), 0 deletions(-)drivers/i2c/busses/i2c-i801.c +16
-0@@ -222,6 +222,7 @@ static int i801_transaction(int xact)int status;int result;int timeout =0 ; +int counter =0 ; result =i801_check_pre ();if (result <0 )@@ -231,12 +232,34 @@ static int i801_transaction(int xact)* INTREN, SMBSCMD are passed in xact */ outb_p (xact |I801_START ,SMBHSTCNT ); +try_again:/* We will always wait for a fraction of a second! */ do {msleep (1 ); status =inb_p (SMBHSTSTS ); }while ((status &SMBHSTSTS_HOST_BUSY ) && (timeout++ <MAX_TIMEOUT )); +/* The i801 chip resets the HOST_BUSY bit +* to indicate that it has completed the transaction, +* but a moment later sets it again. Seems like a glitch. +* Changed code to check the value more times if its not a timeout. +*/ +if (timeout <MAX_TIMEOUT ) { +msleep (1 ); + status =inb_p (SMBHSTSTS ); +if (status &SMBHSTSTS_HOST_BUSY ) { +dev_warn (&I801_dev->dev,"Busy bit set again" +"(%02x)\n" , status); +if (++counter <3 ) { +dev_info (&I801_dev->dev,"Trying" +"again\n" );
drivers/i2c/busses/i2c-i801.c static int i801_transaction(int xact) · line 265
Quoted 10 lines
drivers/i2c/busses/i2c-i801.c +7
-0+ goto try_again; + } +dev_err (&I801_dev->dev,"No use" +" retrying-(%02x)\n" , status); + } + } + result =i801_check_post (status, timeout >MAX_TIMEOUT );if (result <0 )return result;
This workaround causes a huge performance penalty. You are basically doubling the delay for each and every transaction. I have just tested it. At HZ=1000 it doesn't matter too much because the transactions are reasonably fast, but at HZ=250 or lower, the impact is high. This is hardly acceptable. We need to better understand the exact conditions of the problem you have hit, and limit the performance hit as much as we can. Questions:
With which value of HZ do you hit the problem? Does the problem go away for lower values of HZ?
Do you hit the problem for all transaction types, or only specific ones?
Do you hit the problem with specific slaves, or all of them?
Did you ever hit the problem with other Intel chips than the ESB2?
I'm wondering if the following fix would be enough for you. It has almost no performance impact at HZ=250 and less (and could be improved to have none at all for these values of HZ.)
--- drivers/i2c/busses/i2c-i801.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/i2c/busses/i2c-i801.c +2 -2
@@ -233,7 +233,7 @@ static int i801_transaction(int xact)/* We will always wait for a fraction of a second! */ do { -msleep (1 ); +msleep (2 ); status =inb_p (SMBHSTSTS ); }while ((status &SMBHSTSTS_HOST_BUSY ) && (timeout++ <MAX_TIMEOUT ));
@@ -338,7 +338,7 @@ static int i801_block_transaction_byte_b/* We will always wait for a fraction of a second! */ timeout =0 ;do { -msleep (1 ); +msleep (2 ); status =inb_p (SMBHSTSTS ); } while ((!(status & SMBHSTSTS_BYTE_DONE))
Signature · 1 line
Jean Delvare
A message that is not in the archive (sent off-list or not yet received): [email protected]
Hi Jean,
Quoted 10 lines · Jean Delvare
Hi Chaitanya,
Quoted 3 lines · Chaitanya Lala
Intel ESB2 SMBus chip seems to have an issue where it momentarily resets the
HOST_BUSYbit in the host status register.Are you certain? There is a known erratum on some of the ICH chips which is slightly different.
Not really. I think, I might have hit the same issue.
Quoted 4 lines
The erratum is about the
HOST_BUSYflag not
being set immediately when starting a new transaction, so there is the chance that the first check concludes that the transaction is already over. This sounds somewhat similar to the problem you're seeing.
Your reasoning leads me to believe that I might have hit the same issue.
Quoted 71 lines
One
big difference though is that the glitch can only happen at the beginning of the transaction, according to the erratum.Quoted 47 lines
This confuses
the driver waiting for an SMBus transaction to complete. This patch adds a workaround for the same.--- drivers/i2c/busses/i2c-i801.c | 23 +++++++++++++++++++++++ 1 files changed, 23 insertions(+), 0 deletions(-)drivers/i2c/busses/i2c-i801.c +16
-0@@ -222,6 +222,7 @@ static int i801_transaction(int xact)int status;int result;int timeout =0 ; +int counter =0 ; result =i801_check_pre ();if (result <0 )@@ -231,12 +232,34 @@ static int i801_transaction(int xact)* INTREN, SMBSCMD are passed in xact */ outb_p (xact |I801_START ,SMBHSTCNT ); +try_again:/* We will always wait for a fraction of a second! */ do {msleep (1 ); status =inb_p (SMBHSTSTS ); }while ((status &SMBHSTSTS_HOST_BUSY ) && (timeout++ <MAX_TIMEOUT )); +/* The i801 chip resets the HOST_BUSY bit +* to indicate that it has completed the transaction, +* but a moment later sets it again. Seems like a glitch. +* Changed code to check the value more times if its not a timeout. +*/ +if (timeout <MAX_TIMEOUT ) { +msleep (1 ); + status =inb_p (SMBHSTSTS ); +if (status &SMBHSTSTS_HOST_BUSY ) { +dev_warn (&I801_dev->dev,"Busy bit set again" +"(%02x)\n" , status); +if (++counter <3 ) { +dev_info (&I801_dev->dev,"Trying" +"again\n" );drivers/i2c/busses/i2c-i801.c static int i801_transaction(int xact) · line 265
Quoted 10 lines
drivers/i2c/busses/i2c-i801.c +7
-0+ goto try_again; + } +dev_err (&I801_dev->dev,"No use" +" retrying-(%02x)\n" , status); + } + } + result =i801_check_post (status, timeout >MAX_TIMEOUT );if (result <0 )return result;This workaround causes a huge performance penalty. You are basically doubling the delay for each and every transaction. I have just tested it. At HZ=1000 it doesn't matter too much because the transactions are reasonably fast, but at HZ=250 or lower, the impact is high. This is hardly acceptable. We need to better understand the exact conditions of the problem you have hit, and limit the performance hit as much as we can. Questions:
Quoted 3 lines
With which value of HZ do you hit the problem? Does the problem go away for lower values of HZ?
I use 100 and 250. Since I do not use the bus for a lot of stuff, I did not see the performance issue.
Quoted 2 lines
Do you hit the problem for all transaction types, or only specific ones?
For all transactions.
Quoted 2 lines
Do you hit the problem with specific slaves, or all of them?
I hit it with a superIO chip and an eeprom chip.
Quoted 2 lines
Did you ever hit the problem with other Intel chips than the ESB2?
I just have a machine that uses ESB2. So cannot really say much.
Quoted 23 lines
I'm wondering if the following fix would be enough for you. It has almost no performance impact at HZ=250 and less (and could be improved to have none at all for these values of HZ.)
--- drivers/i2c/busses/i2c-i801.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)drivers/i2c/busses/i2c-i801.c +1
-1@@ -233,7 +233,7 @@ static int i801_transaction(int xact)/* We will always wait for a fraction of a second! */ do { -msleep (1 ); +msleep (2 ); status =inb_p (SMBHSTSTS ); }while ((status &SMBHSTSTS_HOST_BUSY ) && (timeout++ <MAX_TIMEOUT ));
drivers/i2c/busses/i2c-i801.c static int i801_block_transaction_byte_b · line 344
Quoted 8 lines
drivers/i2c/busses/i2c-i801.c +1
-1@@ -338,7 +338,7 @@ static int i801_block_transaction_byte_b/* We will always wait for a fraction of a second! */ timeout =0 ;do { -msleep (1 ); +msleep (2 ); status =inb_p (SMBHSTSTS ); } while ((!(status & SMBHSTSTS_BYTE_DONE))
I tried your patch and preliminary results look encouraging. Will let you know about the final results in a few days.
One question - Are we sure that msleep(2) would fix the glitch for good ? I am not very clear about the timings constraints of the i2c bus, hence the query.
Thanks for your help.
Regards,
Chaitanya
A message that is not in the archive (sent off-list or not yet received): 4AAFD040.6040702-DUeqMYwuH4dWk0Htik3J/[email protected]
Hi Chaitanya,
Quoted 2 lines · Chaitanya Lala
I tried your patch and preliminary results look encouraging. Will let you know about the final results in a few days.
OK, thanks.
Quoted 3 lines
One question - Are we sure that msleep(2) would fix the glitch for good ? I am not very clear about the timings constraints of the i2c bus, hence the query.
It doesn't have anything to do with I2C bus timings. The msleep() is between the beginning of the transaction and the polling for result. This is between the OS and the SMBus controller. The bus timings themselves are solely handled by the SMBus controller in hardware and we don't have to deal with it at all.
The msleep(1) has been there for a long time, back when HZ was hard-coded to 100. This means we used to wait for at least 10 ms. With HZ values increasing, the same code results in shorter sleeps (down to 1 ms). So maybe 1 ms wasn't sufficient and 2 ms will be. That being said, if you didn't use HZ=1000... At HZ=250 and HZ=100, msleep(1) and msleep(2) are the same, so I would be surprised if my patch really helps for these values of HZ. I expected your problems to happen at HZ=1000.
But please keep testing, and report what you find. If my patch doesn't help, you could try with msleep(3) or msleep(4) and see if that helps.
Signature · 1 line
Jean Delvare
A message that is not in the archive (sent off-list or not yet received): [email protected]
Hi Jean,
Quoted 21 lines · Jean Delvare
Hi Chaitanya,
Quoted 3 lines · Chaitanya Lala
I tried your patch and preliminary results look encouraging. Will let you know about the final results in a few days.
OK, thanks.
Quoted 3 lines
One question - Are we sure that msleep(2) would fix the glitch for good ? I am not very clear about the timings constraints of the i2c bus, hence the query.
It doesn't have anything to do with I2C bus timings. The
msleep()is between the beginning of the transaction and the polling for result. This is between the OS and the SMBus controller. The bus timings themselves are solely handled by the SMBus controller in hardware and we don't have to deal with it at all.
Quoted 9 lines
The msleep(1) has been there for a long time, back when HZ was hard-coded to 100. This means we used to wait for at least 10 ms. With HZ values increasing, the same code results in shorter sleeps (down to 1 ms). So maybe 1 ms wasn't sufficient and 2 ms will be. That being said, if you didn't use HZ=1000... At HZ=250 and HZ=100, msleep(1) and msleep(2) are the same, so I would be surprised if my patch really helps for these values of HZ. I expected your problems to happen at HZ=1000.
Thanks for the explanation.
Quoted 4 lines
But please keep testing, and report what you find. If my patch doesn't help, you could try with msleep(3) or msleep(4) and see if that helps.
Will continue testing and let you know soon.
Thanks,
Chaitanya
A message that is not in the archive (sent off-list or not yet received): 4AAFE438.60308-DUeqMYwuH4dWk0Htik3J/[email protected]
Quoted 35 lines · Chaitanya Lala
Hi Jean,
Quoted 22 lines · Jean Delvare
Hi Chaitanya,
Quoted 3 lines · Chaitanya Lala
I tried your patch and preliminary results look encouraging. Will let you know about the final results in a few days.
OK, thanks.
Quoted 4 lines
One question - Are we sure that msleep(2) would fix the glitch for good ? I am not very clear about the timings constraints of the i2c bus, hence the query.
It doesn't have anything to do with I2C bus timings. The
msleep()is between the beginning of the transaction and the polling for result. This is between the OS and the SMBus controller. The bus timings themselves are solely handled by the SMBus controller in hardware and we don't have to deal with it at all.Quoted 9 lines
The msleep(1) has been there for a long time, back when HZ was hard-coded to 100. This means we used to wait for at least 10 ms. With HZ values increasing, the same code results in shorter sleeps (down to 1 ms). So maybe 1 ms wasn't sufficient and 2 ms will be. That being said, if you didn't use HZ=1000... At HZ=250 and HZ=100, msleep(1) and msleep(2) are the same, so I would be surprised if my patch really helps for these values of HZ. I expected your problems to happen at HZ=1000.
Quoted 6 lines
Thanks for the explanation.
Quoted 2 lines
But please keep testing, and report what you find. If my patch doesn't help, you could try with msleep(3) or msleep(4) and see if that helps.
Will continue testing and let you know soon.
This was quite some times ago. Any news?
Signature · 1 line
Jean Delvare