[i2c] Fwd: [PATCH 2 of 2] Adding an abort function to unwedge MCP51/55

Oleg Ryjkov olegr at olegr.ca
Sun Oct 7 08:21:52 CEST 2007


Hi Jean,
> Hi Oleg,
> 
> On Fri, 31 Aug 2007 00:21:33 -0700, Oleg Ryjkov wrote:
> > This patch is to add an abort function that will bring back the MCP51/55
> > controller if it was blocked by a block-read operation, in particular.
> > (When a slave sends a wrong byte count on a byte read, the host gets
> > locked up). I've only tested it on an MCP51 and MCP55. However, I'm
> > almost certain it will also work on MCP65, I just did not have the board
> > to test it on. Thus I for now the abort function will only be called
> > if an MCP51/55 was detected.
> 
> Given that we currently allow block transactions only for the MCP51/55
> (why, BTW?) it's fine to only implement the abort capability for these
> chips. When we support block transactions for other chips, we will have
> to support abort for these as well.
The reason that I only do the abort on MCP51/55 is that I had to way of
testing this feature on other controllers. Though I am fairly sure that
it is implemented on most of nvidia chips.
> 
> Your patch has many coding-style problems. Please run
> scripts/checkpatch.pl, address the reported problems, and resend your
> patch.
> 
> Do you think there's any chance that the abort feature will work on my
> nForce2? I could test your patch there.
As I said above, I definitely think that it is worth a shot. If you do
try it, please let me know of the results.
> 
> >
> > Signed-off-by: Oleg Ryjkov <olegr at google.com>
> >
> >
> > --- linux-2.6-untouched/drivers/i2c/busses/i2c-nforce2.c      2007-08-30 23:30:02.000000000 -0700
> > +++ linux-2.6/drivers/i2c/busses/i2c-nforce2.c        2007-08-30 23:45:10.000000000 -0700
> > @@ -62,6 +62,7 @@
> >       int base;
> >       int size;
> >       int blockops;
> > +     int can_abort;
> >  };
> >
> >
> > @@ -83,6 +84,14 @@
> >  #define NVIDIA_SMB_DATA              (smbus->base + 0x04)    /* 32 data registers */
> >  #define NVIDIA_SMB_BCNT              (smbus->base + 0x24)    /* number of data
> >                                                          bytes */
> > +#define NVIDIA_SMB_STATUS_ABRT       (smbus->base + 0x3c)    /* register used to
> > +                                                        reset controller */
> 
> Comment doesn't look right, this register is used to verify that the
> reset was successful, not to do the reset itself?
> 
It is used for both reading the status bit and then resetting it.
I've changing the comment to something more descriptive of the use of
this register.

> > +#define NVIDIA_SMB_CTRL              (smbus->base + 0x3e)    /* control register */
> > +
> > +#define NVIDIA_SMB_STATUS_ABRT_STS   0x01            /* Bit to notify that
> > +                                                           abort succeeded */
> > +#define NVIDIA_SMB_CTRL_ABORT        0x20                    /* Bit used to issue an
> > +                                                           abort command */
> 
> The name of the define is rather explicit, you can probably omit the
> comment for this one.
Dropped the comment.
> 
> >
> >  #define NVIDIA_SMB_STS_DONE  0x80
> >  #define NVIDIA_SMB_STS_ALRM  0x40
> > @@ -103,6 +112,25 @@
> >
> >  static struct pci_driver nforce2_driver;
> >
> > +static void nforce2_abort(struct i2c_adapter * adap) {
> > +     struct nforce2_smbus *smbus = adap->algo_data;
> > +     int timeout = 0;
> > +     unsigned char temp;
> > +
> > +     dev_dbg(&adap->dev, "Aborting current transaction\n");
> > +
> > +     outb_p(NVIDIA_SMB_CTRL_ABORT, NVIDIA_SMB_CTRL);
> > +     timeout = 0;
> 
> Double initialization.
Fixed.
> 
> > +     do {
> > +             msleep(1);
> > +             temp = inb_p(NVIDIA_SMB_STATUS_ABRT);
> > +     } while ( !(temp & NVIDIA_SMB_STATUS_ABRT_STS) &&
> > +                     (timeout++ < MAX_TIMEOUT));
> > +     if ((temp & NVIDIA_SMB_STATUS_ABRT_STS)==0)
> 
> Should be written !(temp & NVIDIA_SMB_STATUS_ABRT_STS) for consistency.
Changed.
> 
> > +             dev_dbg(&adap->dev, "Can't reset the smbus\n");
> 
> This probably deserves a dev_err(). After all the SMBus is stuck until
> next reboot, the user might want to know.
Fixed.
> 
> > +     outb_p(NVIDIA_SMB_STATUS_ABRT_STS, NVIDIA_SMB_STATUS_ABRT);
> > +}
> > +
> >  static int nforce2_check_status(struct i2c_adapter * adap) {
> >       struct nforce2_smbus *smbus = adap->algo_data;
> >       int timeout = 0;
> > @@ -115,6 +143,8 @@
> >
> >       if ((~temp & NVIDIA_SMB_STS_DONE) && (timeout >= MAX_TIMEOUT)) {
> >               dev_dbg(&adap->dev, "SMBus Timeout (0x%02x)!\n", temp);
> > +             if (smbus->can_abort == 1)
> 
> Preferably written (smbus->can_abort).
Fixed..
> 
> > +                     nforce2_abort(adap);
> >               return -1;
> >       }
> >       if ((~temp & NVIDIA_SMB_STS_DONE) || (temp & NVIDIA_SMB_STS_STATUS)) {
> > @@ -324,6 +354,8 @@
> >       case PCI_DEVICE_ID_NVIDIA_NFORCE_MCP55_SMBUS:
> >               smbuses[0].blockops = 1;
> >               smbuses[1].blockops = 1;
> > +             smbuses[0].can_abort = 1;
> > +             smbuses[1].can_abort = 1;
> >       }
> >
> >       /* SMBus adapter 1 */
> 
> Overall this patch looks very good, thanks for doing this. Making sure
> that a bad block transaction won't lock the SMBus forever is a very
> good thing!
> 
> --
> Jean Delvare

Signed-off-by: Oleg Ryjkov <olegr at olegr.ca>


--- linux-2.6-untouched/drivers/i2c/busses/i2c-nforce2.c	2007-10-07 00:39:59.000000000 -0400
+++ linux-2.6/drivers/i2c/busses/i2c-nforce2.c	2007-10-07 01:08:00.000000000 -0400
@@ -62,6 +62,7 @@
 	int base;
 	int size;
 	int blockops;
+	int can_abort;
 };
 
 
@@ -83,7 +84,14 @@
 #define NVIDIA_SMB_DATA		(smbus->base + 0x04)	/* 32 data registers */
 #define NVIDIA_SMB_BCNT		(smbus->base + 0x24)	/* number of data
 							   bytes */
-
+#define NVIDIA_SMB_STATUS_ABRT	(smbus->base + 0x3c)	/* register used to
+							   check the status of
+							   the abort command */
+#define NVIDIA_SMB_CTRL		(smbus->base + 0x3e)	/* control register */
+
+#define NVIDIA_SMB_STATUS_ABRT_STS	0x01		/* Bit to notify that
+							   abort succeeded */
+#define NVIDIA_SMB_CTRL_ABORT	0x20
 #define NVIDIA_SMB_STS_DONE	0x80
 #define NVIDIA_SMB_STS_ALRM	0x40
 #define NVIDIA_SMB_STS_RES	0x20
@@ -103,6 +111,25 @@
 
 static struct pci_driver nforce2_driver;
 
+static void nforce2_abort(struct i2c_adapter *adap)
+{
+	struct nforce2_smbus *smbus = adap->algo_data;
+	int timeout = 0;
+	unsigned char temp;
+
+	dev_dbg(&adap->dev, "Aborting current transaction\n");
+
+	outb_p(NVIDIA_SMB_CTRL_ABORT, NVIDIA_SMB_CTRL);
+	do {
+		msleep(1);
+		temp = inb_p(NVIDIA_SMB_STATUS_ABRT);
+	} while (!(temp & NVIDIA_SMB_STATUS_ABRT_STS) &&
+			(timeout++ < MAX_TIMEOUT));
+	if (!(temp & NVIDIA_SMB_STATUS_ABRT_STS))
+		dev_err(&adap->dev, "Can't reset the smbus\n");
+	outb_p(NVIDIA_SMB_STATUS_ABRT_STS, NVIDIA_SMB_STATUS_ABRT);
+}
+
 static int nforce2_check_status(struct i2c_adapter *adap)
 {
 	struct nforce2_smbus *smbus = adap->algo_data;
@@ -116,6 +143,8 @@
 
 	if (timeout >= MAX_TIMEOUT) {
 		dev_dbg(&adap->dev, "SMBus Timeout (0x%02x)!\n", temp);
+		if (smbus->can_abort)
+			nforce2_abort(adap);
 		return -1;
 	}
 	if (!(temp & NVIDIA_SMB_STS_DONE) || (temp & NVIDIA_SMB_STS_STATUS)) {
@@ -325,6 +354,8 @@
 	case PCI_DEVICE_ID_NVIDIA_NFORCE_MCP55_SMBUS:
 		smbuses[0].blockops = 1;
 		smbuses[1].blockops = 1;
+		smbuses[0].can_abort = 1;
+		smbuses[1].can_abort = 1;
 	}
 
 	/* SMBus adapter 1 */



More information about the i2c mailing list