[PATCH 2.6] scx200_acb: Use PCI I/O resource when appropriate

[PATCH 2.6] scx200_acb: Use PCI I/O resource when appropriate

[Jordan Crouse]

[-- Attachment #1: Type: text/plain, Size: 397 bytes --]

The CS5535 and CS5536 both define the I/O base for the SMBUS device in a 
PCI header.  This patch uses that header for the I/O base rather then 
using the MSR backdoor.

This patch isn't as urgent as the other one, so it can probably take the 
usual trip up through Greg's tree.

-- 
Jordan Crouse
Senior Linux Engineer
AMD - Personal Connectivity Solutions Group
<www.amd.com/embeddedprocessors>

[-- Attachment #2: smb-pcifixup.patch --]
[-- Type: text/plain, Size: 5194 bytes --]

[PATCH] scx200_acb:  Use PCI I/O resource when appropriate

From: Jordan Crouse <jordan.crouse@amd.com>

On the CS5535 and CS5536, the I/O resource is allocated through PCI,
so use that instead of using the MSR backdoor.

Signed-off-by: Jordan Crouse <jordan.crouse@amd.com>
---

 drivers/i2c/busses/scx200_acb.c |  114 +++++++++++++++++++++++++++------------
 1 files changed, 78 insertions(+), 36 deletions(-)

diff --git a/drivers/i2c/busses/scx200_acb.c b/drivers/i2c/busses/scx200_acb.c
index e7a2225..454492f 100644
--- a/drivers/i2c/busses/scx200_acb.c
+++ b/drivers/i2c/busses/scx200_acb.c
@@ -33,7 +33,6 @@
 #include <linux/delay.h>
 #include <linux/mutex.h>
 #include <asm/io.h>
-#include <asm/msr.h>
 
 #include <linux/scx200.h>
 
@@ -85,6 +84,10 @@ struct scx200_acb_iface {
 	u8 *ptr;
 	char needs_reset;
 	unsigned len;
+
+	/* PCI device info */
+	struct pci_dev *pdev;
+	int bar;
 };
 
 /* Register Definitions */
@@ -417,7 +420,8 @@ static int scx200_acb_probe(struct scx20
 	return 0;
 }
 
-static int  __init scx200_acb_create(const char *text, int base, int index)
+static int __init scx200_acb_create(char *text, unsigned int base, int index,
+				    struct pci_dev *pdev, int bar)
 {
 	struct scx200_acb_iface *iface;
 	struct i2c_adapter *adapter;
@@ -444,13 +448,31 @@ static int  __init scx200_acb_create(con
 	snprintf(description, sizeof(description), "%s ACCESS.bus [%s]",
 		 text, adapter->name);
 
-	if (request_region(base, 8, description) == 0) {
-		printk(KERN_ERR NAME ": can't allocate io 0x%x-0x%x\n",
-			base, base + 8-1);
-		rc = -EBUSY;
-		goto errout_free;
+	if (pdev != NULL) {
+
+		iface->pdev = pdev;
+		iface->bar = bar;
+
+		pci_enable_device_bars(iface->pdev, 1 << iface->bar);
+
+		if (pci_request_region(iface->pdev, iface->bar, description)) {
+			printk(KERN_ERR NAME ": can't allocate PCI region %d\n",
+			       iface->bar);
+			rc = -EBUSY;
+			goto errout_free;
+		}
+
+		iface->base = pci_resource_start(iface->pdev, iface->bar);
+	} else {
+		if (request_region(base, 8, description) == 0) {
+			printk(KERN_ERR NAME ": can't allocate io 0x%x-0x%x\n",
+			       base, base + 8 - 1);
+			rc = -EBUSY;
+			goto errout_free;
+		}
+
+		iface->base = base;
 	}
-	iface->base = base;
 
 	rc = scx200_acb_probe(iface);
 	if (rc) {
@@ -474,7 +496,11 @@ static int  __init scx200_acb_create(con
 	return 0;
 
  errout_release:
-	release_region(iface->base, 8);
+	if (iface->pdev != NULL)
+		pci_release_region(iface->pdev, iface->bar);
+	else
+		release_region(iface->base, 8);
+
  errout_free:
 	kfree(iface);
  errout:
@@ -487,49 +513,60 @@ static struct pci_device_id scx200[] = {
 	{ },
 };
 
-static struct pci_device_id divil_pci[] = {
-	{ PCI_DEVICE(PCI_VENDOR_ID_NS,  PCI_DEVICE_ID_NS_CS5535_ISA) },
-	{ PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_CS5536_ISA) },
-	{ } /* NULL entry */
+/* On the CS5535 and CS5536, the SMBUS I/0 base is a PCI resource, so
+   we should allocate that resource through the PCI
+   subsystem. rather then going through the MSR back door.
+*/
+
+static struct {
+	unsigned int vendor;
+	unsigned int device;
+	char *name;
+	int bar;
+} divil_pci[] = {
+	{
+	PCI_VENDOR_ID_NS, PCI_DEVICE_ID_NS_CS5535_ISA, "CS5535", 0}, {
+	PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_CS5536_ISA, "CS5536", 0}
 };
 
-#define MSR_LBAR_SMB		0x5140000B
+#define DIVIL_LENGTH (sizeof(divil_pci) / sizeof(divil_pci[0]))
 
 static int scx200_add_cs553x(void)
 {
-	u32	low, hi;
-	u32	smb_base;
-
-	/* Grab & reserve the SMB I/O range */
-	rdmsr(MSR_LBAR_SMB, low, hi);
+	int dev;
+	struct pci_dev *pdev;
 
-	/* Check the IO mask and whether SMB is enabled */
-	if (hi != 0x0000F001) {
-		printk(KERN_WARNING NAME ": SMBus not enabled\n");
-		return -ENODEV;
+	for (dev = 0; dev < DIVIL_LENGTH; dev++) {
+		pdev =
+		    pci_find_device(divil_pci[dev].vendor,
+				    divil_pci[dev].device, NULL);
+		if (pdev != NULL)
+			break;
 	}
 
-	/* SMBus IO size is 8 bytes */
-	smb_base = low & 0x0000FFF8;
+	if (pdev == NULL)
+		return -ENODEV;
 
-	return scx200_acb_create("CS5535", smb_base, 0);
+	return scx200_acb_create(divil_pci[dev].name, 0, 0, pdev,
+				 divil_pci[dev].bar);
 }
 
 static int __init scx200_acb_init(void)
 {
 	int i;
-	int	rc = -ENODEV;
+	int rc     = -ENODEV;
 
 	pr_debug(NAME ": NatSemi SCx200 ACCESS.bus Driver\n");
 
-	/* Verify that this really is a SCx200 processor */
-	if (pci_dev_present(scx200)) {
-		for (i = 0; i < MAX_DEVICES; ++i) {
-			if (base[i] > 0)
-				rc = scx200_acb_create("SCx200", base[i], i);
-		}
-	} else if (pci_dev_present(divil_pci))
-		rc = scx200_add_cs553x();
+	/* If this is a CS5535 or CS5536, then probe the PCI header */
+
+	if (!pci_dev_present(scx200))
+		return scx200_add_cs553x();
+
+	for (i = 0; i < MAX_DEVICES; ++i) {
+		if (base[i] > 0)
+			rc = scx200_acb_create("SCx200", base[i], i, NULL, 0);
+	}
 
 	return rc;
 }
@@ -544,7 +581,12 @@ static void __exit scx200_acb_cleanup(vo
 		up(&scx200_acb_list_mutex);
 
 		i2c_del_adapter(&iface->adapter);
-		release_region(iface->base, 8);
+
+		if (iface->pdev != NULL)
+			pci_release_region(iface->pdev, iface->bar);
+		else
+			release_region(iface->base, 8);
+
 		kfree(iface);
 		down(&scx200_acb_list_mutex);
 	}

Re: [PATCH 2.6] scx200_acb: Use PCI I/O resource when appropriate

[Jean Delvare]

Hi Jordan,

Better late than never, I guess...

> The CS5535 and CS5536 both define the I/O base for the SMBUS device in a 
> PCI header.  This patch uses that header for the I/O base rather then 
> using the MSR backdoor.

I'm all for doing things the Right Way (TM) but in this case it seems
to make the code rather longer and slightly more complex. If you go
that way, wouldn't it be easier to have a proper pci driver?

On the code itself:

> -static int  __init scx200_acb_create(const char *text, int base, int index)
> +static int __init scx200_acb_create(char *text, unsigned int base, int index,
> +				    struct pci_dev *pdev, int bar)

Why are you removing that "const"? It looked alright to me.

Also, if you are changing base to be unsigned, you could make it an
unsigned long while you're at it - that's the type resource addresses
are supposed to be.

> +		if (pci_request_region(iface->pdev, iface->bar, description)) {
> +			printk(KERN_ERR NAME ": can't allocate PCI region %d\n",
> +			       iface->bar);
> +			rc = -EBUSY;
> +			goto errout_free;
> +		}

You could pass the error value returned by pci_request_region instead of
redefining your own.

> -static struct pci_device_id divil_pci[] = {
> -	{ PCI_DEVICE(PCI_VENDOR_ID_NS,  PCI_DEVICE_ID_NS_CS5535_ISA) },
> -	{ PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_CS5536_ISA) },
> -	{ } /* NULL entry */
> +/* On the CS5535 and CS5536, the SMBUS I/0 base is a PCI resource, so
> +   we should allocate that resource through the PCI
> +   subsystem. rather then going through the MSR back door.
> +*/
> +
> +static struct {
> +	unsigned int vendor;
> +	unsigned int device;
> +	char *name;
> +	int bar;
> +} divil_pci[] = {
> +	{
> +	PCI_VENDOR_ID_NS, PCI_DEVICE_ID_NS_CS5535_ISA, "CS5535", 0}, {
> +	PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_CS5536_ISA, "CS5536", 0}
>  };

It might not be a good idea to move away from pci_device_id. With it,
we could have made that module auto-loaded when the supported hardware
is found.

> +#define DIVIL_LENGTH (sizeof(divil_pci) / sizeof(divil_pci[0]))

Use the ARRAY_SIZE macro instead please.

>  static int __init scx200_acb_init(void)
>  {
>  	int i;
> -	int	rc = -ENODEV;
> +	int rc     = -ENODEV;

Eek. Changing broken coding style for even more broken coding style?
That initialization seems to be no more useful with the changes below,
BTW.

> -	/* Verify that this really is a SCx200 processor */
> -	if (pci_dev_present(scx200)) {
> -		for (i = 0; i < MAX_DEVICES; ++i) {
> -			if (base[i] > 0)
> -				rc = scx200_acb_create("SCx200", base[i], i);
> -		}
> -	} else if (pci_dev_present(divil_pci))
> -		rc = scx200_add_cs553x();
> +	/* If this is a CS5535 or CS5536, then probe the PCI header */
> +
> +	if (!pci_dev_present(scx200))
> +		return scx200_add_cs553x();
> +
> +	for (i = 0; i < MAX_DEVICES; ++i) {
> +		if (base[i] > 0)
> +			rc = scx200_acb_create("SCx200", base[i], i, NULL, 0);
> +	}

You could have made that change much smaller. What's the benefit of
swapping the order?

-- 
Jean Delvare

Re: scx200_acb: Use PCI I/O resource when appropriate

[Jordan Crouse]

On 11/04/06 14:07 +0200, Jean Delvare wrote:
> > The CS5535 and CS5536 both define the I/O base for the SMBUS device in a 
> > PCI header.  This patch uses that header for the I/O base rather then 
> > using the MSR backdoor.
> 
> I'm all for doing things the Right Way (TM) but in this case it seems
> to make the code rather longer and slightly more complex. If you go
> that way, wouldn't it be easier to have a proper pci driver?

And so our dirty laundry is out... :)

We have a sad history here - back a few parts, we had less descriptors to 
describe IO, so we opted to combine several components under the same PCI
header to save resources.  So the bottom line is that we have at least three
desirable devices (timers, GPIO pins and the SMBus) that all lay claim to
the same device.  So a proper PCI header would be difficult to implement - 
either we would have 1 device that registered with three different
subsystems, or we would have three individual devices, the first of which
would get the PCI resources to the ruin of the others.

So this is the solution I came up with, for older parts.  For newer parts,
I sat the BIOS guys down in a room with a guy name Bruno, and they agreed
to make a separate header for each component.

So thats the back story, and I think that Ben will agree, since he also wrote
the GPIO driver which suffers from the same problem.

> On the code itself:
> 
> > -static int  __init scx200_acb_create(const char *text, int base, int index)
> > +static int __init scx200_acb_create(char *text, unsigned int base, int index,
> > +				    struct pci_dev *pdev, int bar)
> 
> Why are you removing that "const"? It looked alright to me.
> 
> Also, if you are changing base to be unsigned, you could make it an
> unsigned long while you're at it - that's the type resource addresses
> are supposed to be.

Oops - right on both points.

> > +		if (pci_request_region(iface->pdev, iface->bar, description)) {
> > +			printk(KERN_ERR NAME ": can't allocate PCI region %d\n",
> > +			       iface->bar);
> > +			rc = -EBUSY;
> > +			goto errout_free;
> > +		}
> 
> You could pass the error value returned by pci_request_region instead of
> redefining your own.

Ok -  that makes sense.

> > -static struct pci_device_id divil_pci[] = {
> > -	{ PCI_DEVICE(PCI_VENDOR_ID_NS,  PCI_DEVICE_ID_NS_CS5535_ISA) },
> > -	{ PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_CS5536_ISA) },
> > -	{ } /* NULL entry */
> > +/* On the CS5535 and CS5536, the SMBUS I/0 base is a PCI resource, so
> > +   we should allocate that resource through the PCI
> > +   subsystem. rather then going through the MSR back door.
> > +*/
> > +
> > +static struct {
> > +	unsigned int vendor;
> > +	unsigned int device;
> > +	char *name;
> > +	int bar;
> > +} divil_pci[] = {
> > +	{
> > +	PCI_VENDOR_ID_NS, PCI_DEVICE_ID_NS_CS5535_ISA, "CS5535", 0}, {
> > +	PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_CS5536_ISA, "CS5536", 0}
> >  };
> 
> It might not be a good idea to move away from pci_device_id. With it,
> we could have made that module auto-loaded when the supported hardware
> is found.

I struggled with this.  By pure luck, the SMBus I/O address is available
on BAR 0 for both parts, but I left it open in case a BIOS vendor wanted
to do something different.  My fault probably for trying to design for
something that doesn't exist.

> > +#define DIVIL_LENGTH (sizeof(divil_pci) / sizeof(divil_pci[0]))
> 
> Use the ARRAY_SIZE macro instead please.

Yep, that would probably be better.

> >  static int __init scx200_acb_init(void)
> >  {
> >  	int i;
> > -	int	rc = -ENODEV;
> > +	int rc     = -ENODEV;
> 
> Eek. Changing broken coding style for even more broken coding style?
> That initialization seems to be no more useful with the changes below,
> BTW.

Argh - I can't believe I let that slip through. 

> > -	/* Verify that this really is a SCx200 processor */
> > -	if (pci_dev_present(scx200)) {
> > -		for (i = 0; i < MAX_DEVICES; ++i) {
> > -			if (base[i] > 0)
> > -				rc = scx200_acb_create("SCx200", base[i], i);
> > -		}
> > -	} else if (pci_dev_present(divil_pci))
> > -		rc = scx200_add_cs553x();
> > +	/* If this is a CS5535 or CS5536, then probe the PCI header */
> > +
> > +	if (!pci_dev_present(scx200))
> > +		return scx200_add_cs553x();
> > +
> > +	for (i = 0; i < MAX_DEVICES; ++i) {
> > +		if (base[i] > 0)
> > +			rc = scx200_acb_create("SCx200", base[i], i, NULL, 0);
> > +	}
> 
> You could have made that change much smaller. What's the benefit of
> swapping the order?

6 one, half a dozen the other.  Its probably more likely that this code
will be run on a CS5535 or CS5536.

I'll go ahead and work up some changes when I get back in the office next
week.  Like I said in my original e-mail, this isn't as urgent a change
as the other one, so we can probably wait until 2.6.18.

Jordan

-- 
Jordan Crouse
Senior Linux Engineer
AMD - Personal Connectivity Solutions Group
<www.amd.com/embeddedprocessors>

Re: [lm-sensors] scx200_acb: Use PCI I/O resource when appropriate

[Rudolf Marek]

Hello all,

> And so our dirty laundry is out... :)
> 
> We have a sad history here - back a few parts, we had less descriptors to 
> describe IO, so we opted to combine several components under the same PCI
> header to save resources.  So the bottom line is that we have at least three
> desirable devices (timers, GPIO pins and the SMBus) that all lay claim to
> the same device.  So a proper PCI header would be difficult to implement - 
> either we would have 1 device that registered with three different
> subsystems, or we would have three individual devices, the first of which
> would get the PCI resources to the ruin of the others.

Well we had the problem with i2c-viapro and vi686a. Best solution was
to fail from the probe function - like this:

         /* Always return failure here.  This is to allow other drivers to bind
          * to this pci device.  We don't really want to have control over the
          * pci device, we only wanted to read as few register values from it.
          */
         return -ENODEV;

release_region:
         release_region(vt596_smba, 8);
         return error;
}

> 
> So this is the solution I came up with, for older parts.  For newer parts,
> I sat the BIOS guys down in a room with a guy name Bruno, and they agreed
> to make a separate header for each component.

This is good ;)

Regards
Rudolf

Re: scx200_acb: Use PCI I/O resource when appropriate

[Jean Delvare]

Hi Rudolf, Jordan,

> Well we had the problem with i2c-viapro and vi686a. Best solution was
> to fail from the probe function - like this:
> 
>          /* Always return failure here.  This is to allow other drivers to bind
>           * to this pci device.  We don't really want to have control over the
>           * pci device, we only wanted to read as few register values from it.
>           */
>          return -ENODEV;
> 
> release_region:
>          release_region(vt596_smba, 8);
>          return error;
> }

True, but we were lucky. You can only do that if you don't need to
access the PCI configuration space during run-time (after the
initialization step) in either driver "sharing" the PCI device. The
i2c-viapro driver qualified, but for example we couldn't do that for
the i2c-i801 driver (block transactions in I2C mode require PCI
configuration register access.)

So depending on the exact hardware context, Jordan may or may not be
able to use the same trick. The other (cleaner) solution is to have an
additional module which registers the PCI device and exports some kind
of interface for the other modules to access the parts they need - but
this requires extra code and makes things much more complex too.

-- 
Jean Delvare

Re: scx200_acb: Use PCI I/O resource when appropriate

[Jean Delvare]

Hi Jordan,

> > > -static struct pci_device_id divil_pci[] = {
> > > -	{ PCI_DEVICE(PCI_VENDOR_ID_NS,  PCI_DEVICE_ID_NS_CS5535_ISA) },
> > > -	{ PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_CS5536_ISA) },
> > > -	{ } /* NULL entry */
> > > +/* On the CS5535 and CS5536, the SMBUS I/0 base is a PCI resource, so
> > > +   we should allocate that resource through the PCI
> > > +   subsystem. rather then going through the MSR back door.
> > > +*/
> > > +
> > > +static struct {
> > > +	unsigned int vendor;
> > > +	unsigned int device;
> > > +	char *name;
> > > +	int bar;
> > > +} divil_pci[] = {
> > > +	{
> > > +	PCI_VENDOR_ID_NS, PCI_DEVICE_ID_NS_CS5535_ISA, "CS5535", 0}, {
> > > +	PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_CS5536_ISA, "CS5536", 0}
> > >  };
> > 
> > It might not be a good idea to move away from pci_device_id. With it,
> > we could have made that module auto-loaded when the supported hardware
> > is found.
>
> I struggled with this.  By pure luck, the SMBus I/O address is available
> on BAR 0 for both parts, but I left it open in case a BIOS vendor wanted
> to do something different.  My fault probably for trying to design for
> something that doesn't exist.

You may not have to drop that information either, I agree that it is
convenient to make the BAR a parameter, and you still have the chip
name anyway. But you can use pci_device_id.driver_data to store an
arbitrary identifier for each device, and use that identifier to lookup
the chip name, BAR or any other information you'd want to attach to the
devices.

Something like this:

static struct pci_device_id divil_pci[] = {
	{ PCI_DEVICE(PCI_VENDOR_ID_NS,  PCI_DEVICE_ID_NS_CS5535_ISA),
		     .driver_data = 0 },
	{ PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_CS5536_ISA),
		     .driver_data = 1 },
	{ } /* NULL entry */
};

static struct {
	char *name;
	int bar;
} divil_data[] = {
	{ "CS5535", 0},
	{ "CS5536", 0},
};

That's just an idea anyway, there might be other ways to get there.

-- 
Jean Delvare