On Tue, Jul 3, 2012 at 4:19 PM, Jean Delvare <[email protected]> wrote:
>
> Come up with a consistent, driver-wide strategy for event polling. For
> intermediate steps of byte-by-byte block transactions, check for
> BYTE_DONE or any error flag being set. At the end of every transaction,
> check for both BUSY being cleared and INTR or any error flag being set.
> This avoids having to wait twice for the same event.
>
> Signed-off-by: Jean Delvare <[email protected]>
> Cc: Daniel Kurtz <[email protected]>
> ---
> Daniel, this is what I had in mind. This applies on top of your first 6
> patches (i.e. all the preliminary cleanup patches, not the interrupt
> patches.) It solves the performance regression problem and makes the
> code look better IMHO. Tested OK on ICH10 and ICH3-M. What do you
> think? Comments and suggestions for improvements are very welcome.
>
> It may be possible to combine i801_wait_intr() and
> i801_wait_byte_done() into a single function, as they do pretty much
> the same, but I'll only do that if it doesn't hurt readability. My
> first attempt was horrible.
>
> drivers/i2c/busses/i2c-i801.c | 88
> ++++++++++++++++++++---------------------
> 1 file changed, 43 insertions(+), 45 deletions(-)
>
> --- linux-3.5-rc5.orig/drivers/i2c/busses/i2c-i801.c 2012-07-02
> 18:09:59.000000000 +0200
> +++ linux-3.5-rc5/drivers/i2c/busses/i2c-i801.c 2012-07-03 10:07:42.254632389
> +0200
> @@ -207,13 +207,13 @@ static int i801_check_pre(struct i801_pr
> }
>
> /* Convert the status register to an error code, and clear it. */
> -static int i801_check_post(struct i801_priv *priv, int status, int timeout)
> +static int i801_check_post(struct i801_priv *priv, int status)
> {
> int result = 0;
> + int clear;
>
> /* If the SMBus is still busy, we give up */
> - if (timeout) {
> - dev_err(&priv->pci_dev->dev, "Transaction timeout\n");
> + if (unlikely(status < 0)) {
> /* try to stop the current command */
> dev_dbg(&priv->pci_dev->dev, "Terminating the current
> operation\n");
> outb_p(inb_p(SMBHSTCNT(priv)) | SMBHSTCNT_KILL,
> @@ -245,42 +245,60 @@ static int i801_check_post(struct i801_p
> dev_dbg(&priv->pci_dev->dev, "Lost arbitration\n");
> }
>
> - if (result) {
> - /* Clear error flags */
> - outb_p(status & STATUS_FLAGS, SMBHSTSTS(priv));
> - status = inb_p(SMBHSTSTS(priv)) & STATUS_FLAGS;
> - if (status) {
> - dev_warn(&priv->pci_dev->dev, "Failed clearing status
> "
> - "flags at end of transaction (%02x)\n",
> - status);
> - }
> - }
> + /* Clear status flags except BYTE_DONE, to be cleared by caller */
> + clear = STATUS_ERROR_FLAGS;
> + if (!(status & SMBHSTSTS_BYTE_DONE))
> + clear |= SMBHSTSTS_INTR;
> + outb_p(status & clear, SMBHSTSTS(priv));
This becomes simpler (see below...), since status is already masked by
(STATUS_ERROR_FLAGS | SMBHSTSTS_INTR).
+ outb_p(status, SMBHSTSTS(priv));
>
> return result;
> }
>
> -/* wait for INTR bit as advised by Intel */
> -static void i801_wait_intr(struct i801_priv *priv)
> +/* Wait for BUSY being cleared and either INTR or an error flag being set */
> +static int i801_wait_intr(struct i801_priv *priv)
> {
> int timeout = 0;
> int status;
>
> + /* We will always wait for a fraction of a second! */
> do {
> usleep_range(250, 500);
> status = inb_p(SMBHSTSTS(priv));
> - } while ((!(status & SMBHSTSTS_INTR)) && (timeout++ < MAX_RETRIES));
> + } while (((status & SMBHSTSTS_HOST_BUSY) ||
> + !(status & (STATUS_ERROR_FLAGS | SMBHSTSTS_INTR))) &&
> + (timeout++ < MAX_RETRIES));
>From your experiments, I think you saw that the final status (ERROR |
INTR) will be set before or at the same time as HOST_BUSY -> 0, so the
loop need only check HOST_BUSY, and then just return the final status
bits of the transfer (ie, ERROR_FLAGS | INTR). So, the function name
could be something like:
i801_wait_xfer_status()
> - if (timeout > MAX_RETRIES)
> + if (timeout > MAX_RETRIES) {
> dev_dbg(&priv->pci_dev->dev, "INTR Timeout!\n");
> + return -ETIMEDOUT;
> + }
> + return status;
> +}
>
> - outb_p(status & STATUS_FLAGS, SMBHSTSTS(priv));
> +/* Wait for either BYTE_DONE or an error flag being set */
> +static int i801_wait_byte_done(struct i801_priv *priv)
> +{
> + int timeout = 0;
> + int status;
> +
> + /* We will always wait for a fraction of a second! */
> + do {
> + usleep_range(250, 500);
> + status = inb_p(SMBHSTSTS(priv));
> + } while (!(status & (STATUS_ERROR_FLAGS | SMBHSTSTS_BYTE_DONE)) &&
> + (timeout++ < MAX_RETRIES));
> +
> + if (timeout > MAX_RETRIES) {
> + dev_dbg(&priv->pci_dev->dev, "BYTE_DONE Timeout!\n");
> + return -ETIMEDOUT;
> + }
> + return status;
> }
For i801_wait_byte_done(), on success I would return 0, if an error
was set, return (status & ERROR_FLAG), and of course return -ETIMEDOUT
on timeout.
Then, we only need to call i801_check_post() if i801_wait_byte_done()
!= 0... see below...
>
> static int i801_transaction(struct i801_priv *priv, int xact)
> {
> int status;
> int result;
> - int timeout = 0;
>
> result = i801_check_pre(priv);
> if (result < 0)
> @@ -290,19 +308,8 @@ static int i801_transaction(struct i801_
> * SMBSCMD are passed in xact */
> outb_p(xact | SMBHSTCNT_START, SMBHSTCNT(priv));
>
> - /* We will always wait for a fraction of a second! */
> - do {
> - usleep_range(250, 500);
> - status = inb_p(SMBHSTSTS(priv));
> - } while ((status & SMBHSTSTS_HOST_BUSY) && (timeout++ < MAX_RETRIES));
> -
> - result = i801_check_post(priv, status, timeout > MAX_RETRIES);
> - if (result < 0)
> - return result;
> -
> - i801_wait_intr(priv);
> -
> - return 0;
> + status = i801_wait_intr(priv);
> + return i801_check_post(priv, status);
> }
>
> static int i801_block_transaction_by_block(struct i801_priv *priv,
> @@ -353,7 +360,6 @@ static int i801_block_transaction_byte_b
> int smbcmd;
> int status;
> int result;
> - int timeout;
>
> result = i801_check_pre(priv);
> if (result < 0)
> @@ -381,15 +387,8 @@ static int i801_block_transaction_byte_b
> outb_p(inb(SMBHSTCNT(priv)) | SMBHSTCNT_START,
> SMBHSTCNT(priv));
>
> - /* We will always wait for a fraction of a second! */
> - timeout = 0;
> - do {
> - usleep_range(250, 500);
> - status = inb_p(SMBHSTSTS(priv));
> - } while (!(status & (SMBHSTSTS_BYTE_DONE |
> STATUS_ERROR_FLAGS))
> - && (timeout++ < MAX_RETRIES));
> -
> - result = i801_check_post(priv, status, timeout > MAX_RETRIES);
+ status = i801_wait_byte_done(priv);
+ if (status)
+ goto done;
> @@ -421,9 +420,8 @@ static int i801_block_transaction_byte_b
> outb_p(SMBHSTSTS_BYTE_DONE, SMBHSTSTS(priv));
> }
>
> - i801_wait_intr(priv);
> -
> - return 0;
> + status = i801_wait_intr(priv);
done:
> + return i801_check_post(priv, status);
> }
>
> static int i801_set_block_buffer_mode(struct i801_priv *priv)
>
>
> --
> Jean Delvare
While we are at this, let's clean up the "Illegal SMBus block read
size" error path, as well.
Why not use the "KILL" bit to terminate the transaction?
-Daniel
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
