[PATCH] i2c-i801: use MEM resource instead of IO resource

[PATCH] i2c-i801: use MEM resource instead of IO resource

[Thilo Cestonaro]

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

> Whenever I load the i2c-i801 smbus controller module I get a resource
conflict with the ACPI:
> -----------------------
> ACPI Warning: SystemIO range 0x000000000000F040-0x000000000000F05F
> conflicts with OpRegion 0x000000000000F040-0x000000000000F04F
> (\_SB_.PCI0.SBUS.SMBI) (20150619/utaddress-254)
> -----------------------
> 

As the resource conflict is about the IO resource, I overworked the 
i2c-i801 module to use the MEM resource which is altough available.
This is IMHO the better alternative to using the 
"acpi_enforce_resources=lax" boot parameter.

The patched module is working on my machine.

cheers,
Thilo

[-- Attachment #2: 0001-i2c-i801-use-MEM-resource-instead-of-IO-resource.patch --]
[-- Type: text/plain, Size: 16311 bytes --]

From f242a59a2b2085f8e1acbd5f108c9c60de483291 Mon Sep 17 00:00:00 2001
From: Thilo Cestonaro <thilo@cestona.ro>
Date: Mon, 11 Apr 2016 10:38:48 +0200
Subject: [PATCH] i2c-i801: use MEM resource instead of IO resource

When using the IO resource the module has a conflict with some BIOS, as Intel
requests this resource in their ACPI ASL Code.
ACPI Warning: SystemIO range 0x000000000000F040-0x000000000000F05F conflicts
with OpRegion 0x000000000000F040-0x000000000000F04F (\_SB_.PCI0.SBUS.SMBI)

One can either use acpi_enforce_resources=lax to be able to use the IO resource
base module or use this patch to use the smbus controller via MEM resource.

Signed-off-by: Thilo Cestonaro <thilo@cestona.ro>
---
 drivers/i2c/busses/i2c-i801.c | 146 +++++++++++++++++++++---------------------
 1 file changed, 73 insertions(+), 73 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 585a3b7..5d05b5d 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -114,7 +114,7 @@
 #define SMBAUXCTL(p)	(13 + (p)->smba)	/* ICH4 and later */
 
 /* PCI Address Constants */
-#define SMBBAR		4
+#define SMBBAR		0 /* use MEM Resource */
 #define SMBPCICTL	0x004
 #define SMBPCISTS	0x006
 #define SMBHSTCFG	0x040
@@ -222,7 +222,7 @@ struct i801_mux_config {
 
 struct i801_priv {
 	struct i2c_adapter adapter;
-	unsigned long smba;
+	void __iomem *smba;
 	unsigned char original_hstcfg;
 	struct pci_dev *pci_dev;
 	unsigned int features;
@@ -277,7 +277,7 @@ static int i801_check_pre(struct i801_priv *priv)
 {
 	int status;
 
-	status = inb_p(SMBHSTSTS(priv));
+	status = readb(SMBHSTSTS(priv));
 	if (status & SMBHSTSTS_HOST_BUSY) {
 		dev_err(&priv->pci_dev->dev, "SMBus is busy, can't use it!\n");
 		return -EBUSY;
@@ -287,8 +287,8 @@ static int i801_check_pre(struct i801_priv *priv)
 	if (status) {
 		dev_dbg(&priv->pci_dev->dev, "Clearing status flags (%02x)\n",
 			status);
-		outb_p(status, SMBHSTSTS(priv));
-		status = inb_p(SMBHSTSTS(priv)) & STATUS_FLAGS;
+		writeb(status, SMBHSTSTS(priv));
+		status = readb(SMBHSTSTS(priv)) & STATUS_FLAGS;
 		if (status) {
 			dev_err(&priv->pci_dev->dev,
 				"Failed clearing status flags (%02x)\n",
@@ -319,19 +319,19 @@ static int i801_check_post(struct i801_priv *priv, int status)
 		dev_err(&priv->pci_dev->dev, "Transaction timeout\n");
 		/* 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,
+		writeb(readb(SMBHSTCNT(priv)) | SMBHSTCNT_KILL,
 		       SMBHSTCNT(priv));
 		usleep_range(1000, 2000);
-		outb_p(inb_p(SMBHSTCNT(priv)) & (~SMBHSTCNT_KILL),
+		writeb(readb(SMBHSTCNT(priv)) & (~SMBHSTCNT_KILL),
 		       SMBHSTCNT(priv));
 
 		/* Check if it worked */
-		status = inb_p(SMBHSTSTS(priv));
+		status = readb(SMBHSTSTS(priv));
 		if ((status & SMBHSTSTS_HOST_BUSY) ||
 		    !(status & SMBHSTSTS_FAILED))
 			dev_err(&priv->pci_dev->dev,
 				"Failed terminating the transaction\n");
-		outb_p(STATUS_FLAGS, SMBHSTSTS(priv));
+		writeb(STATUS_FLAGS, SMBHSTSTS(priv));
 		return -ETIMEDOUT;
 	}
 
@@ -349,7 +349,7 @@ static int i801_check_post(struct i801_priv *priv, int status)
 	}
 
 	/* Clear status flags except BYTE_DONE, to be cleared by caller */
-	outb_p(status, SMBHSTSTS(priv));
+	writeb(status, SMBHSTSTS(priv));
 
 	return result;
 }
@@ -363,7 +363,7 @@ static int i801_wait_intr(struct i801_priv *priv)
 	/* We will always wait for a fraction of a second! */
 	do {
 		usleep_range(250, 500);
-		status = inb_p(SMBHSTSTS(priv));
+		status = readb(SMBHSTSTS(priv));
 	} while (((status & SMBHSTSTS_HOST_BUSY) ||
 		  !(status & (STATUS_ERROR_FLAGS | SMBHSTSTS_INTR))) &&
 		 (timeout++ < MAX_RETRIES));
@@ -384,7 +384,7 @@ static int i801_wait_byte_done(struct i801_priv *priv)
 	/* We will always wait for a fraction of a second! */
 	do {
 		usleep_range(250, 500);
-		status = inb_p(SMBHSTSTS(priv));
+		status = readb(SMBHSTSTS(priv));
 	} while (!(status & (STATUS_ERROR_FLAGS | SMBHSTSTS_BYTE_DONE)) &&
 		 (timeout++ < MAX_RETRIES));
 
@@ -406,7 +406,7 @@ static int i801_transaction(struct i801_priv *priv, int xact)
 		return result;
 
 	if (priv->features & FEATURE_IRQ) {
-		outb_p(xact | SMBHSTCNT_INTREN | SMBHSTCNT_START,
+		writeb(xact | SMBHSTCNT_INTREN | SMBHSTCNT_START,
 		       SMBHSTCNT(priv));
 		result = wait_event_timeout(priv->waitq,
 					    (status = priv->status),
@@ -422,7 +422,7 @@ static int i801_transaction(struct i801_priv *priv, int xact)
 
 	/* the current contents of SMBHSTCNT can be overwritten, since PEC,
 	 * SMBSCMD are passed in xact */
-	outb_p(xact | SMBHSTCNT_START, SMBHSTCNT(priv));
+	writeb(xact | SMBHSTCNT_START, SMBHSTCNT(priv));
 
 	status = i801_wait_intr(priv);
 	return i801_check_post(priv, status);
@@ -435,14 +435,14 @@ static int i801_block_transaction_by_block(struct i801_priv *priv,
 	int i, len;
 	int status;
 
-	inb_p(SMBHSTCNT(priv)); /* reset the data buffer index */
+	readb(SMBHSTCNT(priv)); /* reset the data buffer index */
 
 	/* Use 32-byte buffer to process this transaction */
 	if (read_write == I2C_SMBUS_WRITE) {
 		len = data->block[0];
-		outb_p(len, SMBHSTDAT0(priv));
+		writeb(len, SMBHSTDAT0(priv));
 		for (i = 0; i < len; i++)
-			outb_p(data->block[i+1], SMBBLKDAT(priv));
+			writeb(data->block[i+1], SMBBLKDAT(priv));
 	}
 
 	status = i801_transaction(priv, I801_BLOCK_DATA |
@@ -451,13 +451,13 @@ static int i801_block_transaction_by_block(struct i801_priv *priv,
 		return status;
 
 	if (read_write == I2C_SMBUS_READ) {
-		len = inb_p(SMBHSTDAT0(priv));
+		len = readb(SMBHSTDAT0(priv));
 		if (len < 1 || len > I2C_SMBUS_BLOCK_MAX)
 			return -EPROTO;
 
 		data->block[0] = len;
 		for (i = 0; i < len; i++)
-			data->block[i + 1] = inb_p(SMBBLKDAT(priv));
+			data->block[i + 1] = readb(SMBBLKDAT(priv));
 	}
 	return 0;
 }
@@ -468,7 +468,7 @@ static void i801_isr_byte_done(struct i801_priv *priv)
 		/* For SMBus block reads, length is received with first byte */
 		if (((priv->cmd & 0x1c) == I801_BLOCK_DATA) &&
 		    (priv->count == 0)) {
-			priv->len = inb_p(SMBHSTDAT0(priv));
+			priv->len = readb(SMBHSTDAT0(priv));
 			if (priv->len < 1 || priv->len > I2C_SMBUS_BLOCK_MAX) {
 				dev_err(&priv->pci_dev->dev,
 					"Illegal SMBus block read size %d\n",
@@ -485,22 +485,22 @@ static void i801_isr_byte_done(struct i801_priv *priv)
 
 		/* Read next byte */
 		if (priv->count < priv->len)
-			priv->data[priv->count++] = inb(SMBBLKDAT(priv));
+			priv->data[priv->count++] = readb(SMBBLKDAT(priv));
 		else
 			dev_dbg(&priv->pci_dev->dev,
 				"Discarding extra byte on block read\n");
 
 		/* Set LAST_BYTE for last byte of read transaction */
 		if (priv->count == priv->len - 1)
-			outb_p(priv->cmd | SMBHSTCNT_LAST_BYTE,
+			writeb(priv->cmd | SMBHSTCNT_LAST_BYTE,
 			       SMBHSTCNT(priv));
 	} else if (priv->count < priv->len - 1) {
 		/* Write next byte, except for IRQ after last byte */
-		outb_p(priv->data[++priv->count], SMBBLKDAT(priv));
+		writeb(priv->data[++priv->count], SMBBLKDAT(priv));
 	}
 
 	/* Clear BYTE_DONE to continue with next byte */
-	outb_p(SMBHSTSTS_BYTE_DONE, SMBHSTSTS(priv));
+	writeb(SMBHSTSTS_BYTE_DONE, SMBHSTSTS(priv));
 }
 
 /*
@@ -528,7 +528,7 @@ static irqreturn_t i801_isr(int irq, void *dev_id)
 	if (!(pcists & SMBPCISTS_INTS))
 		return IRQ_NONE;
 
-	status = inb_p(SMBHSTSTS(priv));
+	status = readb(SMBHSTSTS(priv));
 	if (status & SMBHSTSTS_BYTE_DONE)
 		i801_isr_byte_done(priv);
 
@@ -538,7 +538,7 @@ static irqreturn_t i801_isr(int irq, void *dev_id)
 	 */
 	status &= SMBHSTSTS_INTR | STATUS_ERROR_FLAGS;
 	if (status) {
-		outb_p(status, SMBHSTSTS(priv));
+		writeb(status, SMBHSTSTS(priv));
 		priv->status |= status;
 		wake_up(&priv->waitq);
 	}
@@ -569,8 +569,8 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
 	len = data->block[0];
 
 	if (read_write == I2C_SMBUS_WRITE) {
-		outb_p(len, SMBHSTDAT0(priv));
-		outb_p(data->block[1], SMBBLKDAT(priv));
+		writeb(len, SMBHSTDAT0(priv));
+		writeb(data->block[1], SMBBLKDAT(priv));
 	}
 
 	if (command == I2C_SMBUS_I2C_BLOCK_DATA &&
@@ -588,7 +588,7 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
 		priv->count = 0;
 		priv->data = &data->block[1];
 
-		outb_p(priv->cmd | SMBHSTCNT_START, SMBHSTCNT(priv));
+		writeb(priv->cmd | SMBHSTCNT_START, SMBHSTCNT(priv));
 		result = wait_event_timeout(priv->waitq,
 					    (status = priv->status),
 					    adap->timeout);
@@ -604,10 +604,10 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
 	for (i = 1; i <= len; i++) {
 		if (i == len && read_write == I2C_SMBUS_READ)
 			smbcmd |= SMBHSTCNT_LAST_BYTE;
-		outb_p(smbcmd, SMBHSTCNT(priv));
+		writeb(smbcmd, SMBHSTCNT(priv));
 
 		if (i == 1)
-			outb_p(inb(SMBHSTCNT(priv)) | SMBHSTCNT_START,
+			writeb(readb(SMBHSTCNT(priv)) | SMBHSTCNT_START,
 			       SMBHSTCNT(priv));
 
 		status = i801_wait_byte_done(priv);
@@ -616,17 +616,17 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
 
 		if (i == 1 && read_write == I2C_SMBUS_READ
 		 && command != I2C_SMBUS_I2C_BLOCK_DATA) {
-			len = inb_p(SMBHSTDAT0(priv));
+			len = readb(SMBHSTDAT0(priv));
 			if (len < 1 || len > I2C_SMBUS_BLOCK_MAX) {
 				dev_err(&priv->pci_dev->dev,
 					"Illegal SMBus block read size %d\n",
 					len);
 				/* Recover */
-				while (inb_p(SMBHSTSTS(priv)) &
+				while (readb(SMBHSTSTS(priv)) &
 				       SMBHSTSTS_HOST_BUSY)
-					outb_p(SMBHSTSTS_BYTE_DONE,
+					writeb(SMBHSTSTS_BYTE_DONE,
 					       SMBHSTSTS(priv));
-				outb_p(SMBHSTSTS_INTR, SMBHSTSTS(priv));
+				writeb(SMBHSTSTS_INTR, SMBHSTSTS(priv));
 				return -EPROTO;
 			}
 			data->block[0] = len;
@@ -634,12 +634,12 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
 
 		/* Retrieve/store value in SMBBLKDAT */
 		if (read_write == I2C_SMBUS_READ)
-			data->block[i] = inb_p(SMBBLKDAT(priv));
+			data->block[i] = readb(SMBBLKDAT(priv));
 		if (read_write == I2C_SMBUS_WRITE && i+1 <= len)
-			outb_p(data->block[i+1], SMBBLKDAT(priv));
+			writeb(data->block[i+1], SMBBLKDAT(priv));
 
 		/* signals SMBBLKDAT ready */
-		outb_p(SMBHSTSTS_BYTE_DONE, SMBHSTSTS(priv));
+		writeb(SMBHSTSTS_BYTE_DONE, SMBHSTSTS(priv));
 	}
 
 	status = i801_wait_intr(priv);
@@ -649,8 +649,8 @@ exit:
 
 static int i801_set_block_buffer_mode(struct i801_priv *priv)
 {
-	outb_p(inb_p(SMBAUXCTL(priv)) | SMBAUXCTL_E32B, SMBAUXCTL(priv));
-	if ((inb_p(SMBAUXCTL(priv)) & SMBAUXCTL_E32B) == 0)
+	writeb(readb(SMBAUXCTL(priv)) | SMBAUXCTL_E32B, SMBAUXCTL(priv));
+	if ((readb(SMBAUXCTL(priv)) & SMBAUXCTL_E32B) == 0)
 		return -EIO;
 	return 0;
 }
@@ -723,51 +723,51 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
 
 	switch (size) {
 	case I2C_SMBUS_QUICK:
-		outb_p(((addr & 0x7f) << 1) | (read_write & 0x01),
+		writeb(((addr & 0x7f) << 1) | (read_write & 0x01),
 		       SMBHSTADD(priv));
 		xact = I801_QUICK;
 		break;
 	case I2C_SMBUS_BYTE:
-		outb_p(((addr & 0x7f) << 1) | (read_write & 0x01),
+		writeb(((addr & 0x7f) << 1) | (read_write & 0x01),
 		       SMBHSTADD(priv));
 		if (read_write == I2C_SMBUS_WRITE)
-			outb_p(command, SMBHSTCMD(priv));
+			writeb(command, SMBHSTCMD(priv));
 		xact = I801_BYTE;
 		break;
 	case I2C_SMBUS_BYTE_DATA:
-		outb_p(((addr & 0x7f) << 1) | (read_write & 0x01),
+		writeb(((addr & 0x7f) << 1) | (read_write & 0x01),
 		       SMBHSTADD(priv));
-		outb_p(command, SMBHSTCMD(priv));
+		writeb(command, SMBHSTCMD(priv));
 		if (read_write == I2C_SMBUS_WRITE)
-			outb_p(data->byte, SMBHSTDAT0(priv));
+			writeb(data->byte, SMBHSTDAT0(priv));
 		xact = I801_BYTE_DATA;
 		break;
 	case I2C_SMBUS_WORD_DATA:
-		outb_p(((addr & 0x7f) << 1) | (read_write & 0x01),
+		writeb(((addr & 0x7f) << 1) | (read_write & 0x01),
 		       SMBHSTADD(priv));
-		outb_p(command, SMBHSTCMD(priv));
+		writeb(command, SMBHSTCMD(priv));
 		if (read_write == I2C_SMBUS_WRITE) {
-			outb_p(data->word & 0xff, SMBHSTDAT0(priv));
-			outb_p((data->word & 0xff00) >> 8, SMBHSTDAT1(priv));
+			writeb(data->word & 0xff, SMBHSTDAT0(priv));
+			writeb((data->word & 0xff00) >> 8, SMBHSTDAT1(priv));
 		}
 		xact = I801_WORD_DATA;
 		break;
 	case I2C_SMBUS_BLOCK_DATA:
-		outb_p(((addr & 0x7f) << 1) | (read_write & 0x01),
+		writeb(((addr & 0x7f) << 1) | (read_write & 0x01),
 		       SMBHSTADD(priv));
-		outb_p(command, SMBHSTCMD(priv));
+		writeb(command, SMBHSTCMD(priv));
 		block = 1;
 		break;
 	case I2C_SMBUS_I2C_BLOCK_DATA:
 		/* NB: page 240 of ICH5 datasheet shows that the R/#W
 		 * bit should be cleared here, even when reading */
-		outb_p((addr & 0x7f) << 1, SMBHSTADD(priv));
+		writeb((addr & 0x7f) << 1, SMBHSTADD(priv));
 		if (read_write == I2C_SMBUS_READ) {
 			/* NB: page 240 of ICH5 datasheet also shows
 			 * that DATA1 is the cmd field when reading */
-			outb_p(command, SMBHSTDAT1(priv));
+			writeb(command, SMBHSTDAT1(priv));
 		} else
-			outb_p(command, SMBHSTCMD(priv));
+			writeb(command, SMBHSTCMD(priv));
 		block = 1;
 		break;
 	default:
@@ -777,9 +777,9 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
 	}
 
 	if (hwpec)	/* enable/disable hardware PEC */
-		outb_p(inb_p(SMBAUXCTL(priv)) | SMBAUXCTL_CRC, SMBAUXCTL(priv));
+		writeb(readb(SMBAUXCTL(priv)) | SMBAUXCTL_CRC, SMBAUXCTL(priv));
 	else
-		outb_p(inb_p(SMBAUXCTL(priv)) & (~SMBAUXCTL_CRC),
+		writeb(readb(SMBAUXCTL(priv)) & (~SMBAUXCTL_CRC),
 		       SMBAUXCTL(priv));
 
 	if (block)
@@ -792,7 +792,7 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
 	   time, so we forcibly disable it after every transaction. Turn off
 	   E32B for the same reason. */
 	if (hwpec || block)
-		outb_p(inb_p(SMBAUXCTL(priv)) &
+		writeb(readb(SMBAUXCTL(priv)) &
 		       ~(SMBAUXCTL_CRC | SMBAUXCTL_E32B), SMBAUXCTL(priv));
 
 	if (block)
@@ -805,11 +805,11 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
 	switch (xact & 0x7f) {
 	case I801_BYTE:	/* Result put in SMBHSTDAT0 */
 	case I801_BYTE_DATA:
-		data->byte = inb_p(SMBHSTDAT0(priv));
+		data->byte = readb(SMBHSTDAT0(priv));
 		break;
 	case I801_WORD_DATA:
-		data->word = inb_p(SMBHSTDAT0(priv)) +
-			     (inb_p(SMBHSTDAT1(priv)) << 8);
+		data->word = readb(SMBHSTDAT0(priv)) +
+			     (readb(SMBHSTDAT1(priv)) << 8);
 		break;
 	}
 	return 0;
@@ -1320,14 +1320,6 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	}
 	pcim_pin_device(dev);
 
-	/* Determine the address of the SMBus area */
-	priv->smba = pci_resource_start(dev, SMBBAR);
-	if (!priv->smba) {
-		dev_err(&dev->dev,
-			"SMBus base address uninitialized, upgrade BIOS\n");
-		return -ENODEV;
-	}
-
 	err = acpi_check_resource_conflict(&dev->resource[SMBBAR]);
 	if (err) {
 		return -ENODEV;
@@ -1337,12 +1329,20 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 				 dev_driver_string(&dev->dev));
 	if (err) {
 		dev_err(&dev->dev,
-			"Failed to request SMBus region 0x%lx-0x%Lx\n",
-			priv->smba,
+			"Failed to request SMBus region 0x%Lx-0x%Lx\n",
+			(unsigned long long)pci_resource_start(dev, SMBBAR),
 			(unsigned long long)pci_resource_end(dev, SMBBAR));
 		return err;
 	}
 
+	/* Determine the address of the SMBus area */
+	priv->smba = pcim_iomap_table(dev)[SMBBAR];
+	if (!priv->smba) {
+		dev_err(&dev->dev,
+			"SMBus base address uninitialized, upgrade BIOS\n");
+		return -ENODEV;
+	}
+
 	pci_read_config_byte(priv->pci_dev, SMBHSTCFG, &temp);
 	priv->original_hstcfg = temp;
 	temp &= ~SMBHSTCFG_I2C_EN;	/* SMBus timing */
@@ -1360,7 +1360,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 
 	/* Clear special mode bits */
 	if (priv->features & (FEATURE_SMBUS_PEC | FEATURE_BLOCK_BUFFER))
-		outb_p(inb_p(SMBAUXCTL(priv)) &
+		writeb(readb(SMBAUXCTL(priv)) &
 		       ~(SMBAUXCTL_CRC | SMBAUXCTL_E32B), SMBAUXCTL(priv));
 
 	/* Default timeout in interrupt mode: 200 ms */
@@ -1400,7 +1400,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	i801_add_tco(priv);
 
 	snprintf(priv->adapter.name, sizeof(priv->adapter.name),
-		"SMBus I801 adapter at %04lx", priv->smba);
+		"SMBus I801 adapter at 0x%p", priv->smba);
 	err = i2c_add_adapter(&priv->adapter);
 	if (err) {
 		dev_err(&dev->dev, "Failed to add SMBus adapter\n");
-- 
2.7.4

Re: [PATCH] i2c-i801: use MEM resource instead of IO resource

[Jean Delvare]

Hi Thilo,

On lun., 2016-04-11 at 11:24 +0200, Thilo Cestonaro wrote:
> > Whenever I load the i2c-i801 smbus controller module I get a resource
> conflict with the ACPI:
> > -----------------------
> > ACPI Warning: SystemIO range 0x000000000000F040-0x000000000000F05F
> > conflicts with OpRegion 0x000000000000F040-0x000000000000F04F
> > (\_SB_.PCI0.SBUS.SMBI) (20150619/utaddress-254)
> > -----------------------
> > 
> 
> As the resource conflict is about the IO resource, I overworked the 
> i2c-i801 module to use the MEM resource which is altough available.
> This is IMHO the better alternative to using the 
> "acpi_enforce_resources=lax" boot parameter.
> 
> The patched module is working on my machine.

To be honest, I didn't know the 82801 SMBus controller supported memory
access. Does it actually work since the first supported controller
(82801AA)?

But anyway, I can't see how using it solves any problem. I guess you
access the very same registers, just using a different method. If the
BIOS is actually using the SMBus controller, you have exactly the same
conflict as before, the only difference is that it is no longer
reported. This would be a regression, not an improvement.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH] i2c-i801: use MEM resource instead of IO resource

[Thilo Cestonaro]

Hey Jean,

Am 12.04.2016 09:02, schrieb Jean Delvare:
> Hi Thilo,
> 
> On lun., 2016-04-11 at 11:24 +0200, Thilo Cestonaro wrote:
>> > Whenever I load the i2c-i801 smbus controller module I get a resource
>> conflict with the ACPI:
>> > -----------------------
>> > ACPI Warning: SystemIO range 0x000000000000F040-0x000000000000F05F
>> > conflicts with OpRegion 0x000000000000F040-0x000000000000F04F
>> > (\_SB_.PCI0.SBUS.SMBI) (20150619/utaddress-254)
>> > -----------------------
>> >
>> 
>> As the resource conflict is about the IO resource, I overworked the
>> i2c-i801 module to use the MEM resource which is altough available.
>> This is IMHO the better alternative to using the
>> "acpi_enforce_resources=lax" boot parameter.
>> 
>> The patched module is working on my machine.
> 
> To be honest, I didn't know the 82801 SMBus controller supported memory
> access. Does it actually work since the first supported controller
> (82801AA)?
> 
Good question! Perhaps I will implement an IO and MEM access depending 
on the ACPI conflict prioizing the IO access.

> But anyway, I can't see how using it solves any problem. I guess you
> access the very same registers, just using a different method. If the
> BIOS is actually using the SMBus controller, you have exactly the same
> conflict as before, the only difference is that it is no longer
> reported. This would be a regression, not an improvement.
> 
You are absolutely right, it doesn't solve anything so it is just an 
alternative to the boot parameter.
The advantage IMHO is, that you don't need to add the boot parameter and 
that this change only affects the module itself,
instead of like the parameter the whole system.

I can still show the conflict by checking the MEM resource and the IO 
resource, so the user will be informed about the problem but can use 
it's smbus.
Currently if the conflict will be shown the probe returns an error.

Hmm, perhaps showing the conflict and not returning with an error is the 
more easier way. Many use the boot parameter and haven't reported 
problems with it,
so I think using it despite the conflict wouldn't do any harm.

Cheers,
Thilo

Re: [PATCH] i2c-i801: use MEM resource instead of IO resource

[Jean Delvare]

Hi Thilo,

On mar., 2016-04-12 at 09:33 +0200, Thilo Cestonaro wrote:
> Hey Jean,
> 
> Am 12.04.2016 09:02, schrieb Jean Delvare:
> > Hi Thilo,
> > 
> > On lun., 2016-04-11 at 11:24 +0200, Thilo Cestonaro wrote:
> >> > Whenever I load the i2c-i801 smbus controller module I get a resource
> >> conflict with the ACPI:
> >> > -----------------------
> >> > ACPI Warning: SystemIO range 0x000000000000F040-0x000000000000F05F
> >> > conflicts with OpRegion 0x000000000000F040-0x000000000000F04F
> >> > (\_SB_.PCI0.SBUS.SMBI) (20150619/utaddress-254)
> >> > -----------------------
> >> >
> >> 
> >> As the resource conflict is about the IO resource, I overworked the
> >> i2c-i801 module to use the MEM resource which is altough available.
> >> This is IMHO the better alternative to using the
> >> "acpi_enforce_resources=lax" boot parameter.
> >> 
> >> The patched module is working on my machine.
> > 
> > To be honest, I didn't know the 82801 SMBus controller supported memory
> > access. Does it actually work since the first supported controller
> > (82801AA)?
> > 
> Good question! Perhaps I will implement an IO and MEM access depending 
> on the ACPI conflict prioizing the IO access.
> 
> > But anyway, I can't see how using it solves any problem. I guess you
> > access the very same registers, just using a different method. If the
> > BIOS is actually using the SMBus controller, you have exactly the same
> > conflict as before, the only difference is that it is no longer
> > reported. This would be a regression, not an improvement.
> > 
> You are absolutely right, it doesn't solve anything so it is just an 
> alternative to the boot parameter.
> The advantage IMHO is, that you don't need to add the boot parameter and 
> that this change only affects the module itself,
> instead of like the parameter the whole system.

It is true that acpi_enforce_resources affects the whole system. But is
it a real problem in practice.

> I can still show the conflict by checking the MEM resource and the IO 
> resource, so the user will be informed about the problem but can use 
> it's smbus.

That's exactly what acpi_enforce_resources=lax does.

> Currently if the conflict will be shown the probe returns an error.

Only with acpi_enforce_resources=strict.

> Hmm, perhaps showing the conflict and not returning with an error is the 
> more easier way. Many use the boot parameter and haven't reported 
> problems with it,
> so I think using it despite the conflict wouldn't do any harm.

Just because they haven't hit any problem yet, doesn't mean it can't
happen. That is the nature of race conditions.

It is simply wrong for a native driver to access an ACPI-reserved
resource. If a BIOS reserves a resource for ACPI usage, it should offer
an OS interface to access the device safely, and the OS needs an ACPI
driver for that interface. Anything else is just a hack (be it
acpi_enforce_resources=lax or your proposal.)

The reason why we made acpi_enforce_resources=strict the default is so
that the awareness of the problem grows, and BIOS vendors finally start
caring about it. And it starts working, after many years, see:

https://bugzilla.kernel.org/show_bug.cgi?id=110041

If Intel starts caring then maybe we'll see better BIOSes in the future.
They provide the reference BIOS to board makers as far as I know.

So I'm not taking your patch, sorry. Doing so would break the whole
plan.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH] i2c-i801: use MEM resource instead of IO resource

[Thilo Cestonaro]

> The reason why we made acpi_enforce_resources=strict the default is so
> that the awareness of the problem grows, and BIOS vendors finally start
> caring about it. And it starts working, after many years, see:
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=110041
> 
> If Intel starts caring then maybe we'll see better BIOSes in the 
> future.
> They provide the reference BIOS to board makers as far as I know.

I like that you hope Intel will react on this one day :).

> 
> So I'm not taking your patch, sorry. Doing so would break the whole
> plan.

I see, no problem with this.

Cheers,
Thilo