[PATCH v3 0/9] i2c: piix4: Replace cd6h/cd7h port I/O accesses with MMIO accesses

[PATCH v3 0/9] i2c: piix4: Replace cd6h/cd7h port I/O accesses with MMIO accesses

[Terry Bowman]

This series changes the piix4_smbus driver's cd6h/cd7h port I/O accesses
to use MMIO instead. This is necessary because cd6h/cd7h port I/O may be
disabled on later AMD processors.

This series includes patches with MMIO accesses to registers
FCH::PM::DECODEEN and FCH::PM::ISACONTROL. The same registers are also
accessed by the sp5100_tco driver.[1] Synchronization to the MMIO
registers is required in both drivers.

The first patch creates a macro to request MMIO region using the 'muxed'
retry logic. This is used in patch 6 to synchronize accesses to EFCH MMIO.

The second patch replaces a hardcoded region size with a #define. This is
to improve maintainability and was requested from v2 review.

The third patch moves duplicated region request/release code into
functions. This locates related code into functions and reduces code line
count. This will also make adding MMIO support in patch 6 easier.

The fourth patch moves SMBus controller address detection into a function. 
This is in preparation for adding MMIO region support.

The fifth patch moves EFCH port selection into a function. This is in
preparation for adding MMIO region support.

The sixth patch adds MMIO support for region requesting/releasing and
mapping. This is necessary for using MMIO to detect SMBus controller
address, enable SMBbus controller region, and control the port select.

The seventh patch updates the SMBus controller address detection to support
using MMIO. This is necessary because the driver accesses registers
FCH::PM::DECODEEN and FCH::PM::ISACONTOL during initialization and they are
only available using MMIO on later AMD processors.

The eighth patch updates the SMBus port selection to support MMIO. This is
required because port selection control resides in the
FCH::PM::DECODEEN[smbus0sel] and is only accessible using MMIO on later AMD
processors.

The ninth patch enables the EFCH MMIO functionality added earlier in this
series. The SMBus controller's PCI revision ID is used to check if EFCH
MMIO is supported by HW and should be enabled in the driver.

Based on v5.16.

Testing:
  Tested on family 19h using:
    i2cdetect -y 0
    i2cdetect -y 1
    i2cdetect -y 2

  - Results using v5.16 and this series applied:
    # i2cdetect -y 0 
         0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
    00:                         -- -- -- -- -- -- -- -- 
    10: 10 11 -- -- -- -- -- -- 18 -- -- -- -- -- -- -- 
    20: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
    30: 30 -- -- -- -- 35 36 -- -- -- -- -- -- -- -- -- 
    40: -- -- -- -- -- -- -- -- -- -- 4a -- -- -- -- -- 
    50: 50 -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
    60: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
    70: -- -- -- 73 -- -- -- --                         
    # i2cdetect -y 1
         0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
    00:                         -- -- -- -- -- -- -- -- 
    10: 10 11 -- -- -- -- -- -- 18 -- -- -- -- -- -- -- 
    20: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
    30: 30 -- -- -- -- 35 36 -- -- -- -- -- -- -- -- -- 
    40: -- -- -- -- -- -- -- -- -- -- 4a -- -- -- -- -- 
    50: 50 -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
    60: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
    70: -- -- -- 73 -- -- -- --                         
    # i2cdetect -y 2
         0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
    00:                         -- -- -- -- -- -- -- -- 
    10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
    20: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
    30: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
    40: -- -- -- -- -- -- -- -- -- -- -- -- 4c -- -- -- 
    50: -- 51 -- -- 54 -- -- -- -- -- -- -- -- -- -- -- 
    60: 60 -- -- 63 -- -- 66 -- -- -- -- 6b -- -- 6e -- 
    70: 70 71 72 73 74 75 -- 77
    
  - Results using v5.16 (w/o this series applied):
    # i2cdetect -y 0
         0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
    00:                         -- -- -- -- -- -- -- -- 
    10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
    20: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
    30: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
    40: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
    50: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
    60: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
    70: -- -- -- -- -- -- -- --                         
    # i2cdetect -y 1
         0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
    00:                         -- -- -- -- -- -- -- -- 
    10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
    20: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
    30: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
    40: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
    50: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
    60: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
    70: -- -- -- -- -- -- -- --                         
    # i2cdetect -y 2
         0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
    00:                         -- -- -- -- -- -- -- -- 
    10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
    20: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
    30: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
    40: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
    50: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
    60: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
    70: -- -- -- -- -- -- -- --

  Also tested using sp5100_tco submitted series listed below.[1]
  I applied the sp5100_tco v3 series and ran:
    cat  >> /dev/watchdog

[1] sp5100_tco v3 upstream review:
Link: https://lore.kernel.org/linux-watchdog/20220118202234.410555-1-terry.bowman@amd.com/

Changes in v3:
 - Added request_muxed_mem_region() patch (Wolfram, Guenter)
 - Reduced To/Cc list length. (Andy)
 
Changes in v2:
 - Split single patch. (Jean Delvare)
 - Replace constant 2 with SB800_PIIX4_SMB_MAP_SIZE where appropriate.
   (Jean Delvare)
 - Shorten SB800_PIIX4_FCH_PM_DECODEEN_MMIO_EN name length to
   SB800_PIIX4_FCH_PM_DECODEEN_MMIO. (Jean Delvare)
 - Change AMD_PCI_SMBUS_REVISION_MMIO from 0x59 to 0x51. (Terry Bowman)
 - Change piix4_sb800_region_setup() to piix4_sb800_region_request().
   (Jean Delvare)
 - Change 'SMB' text in  logging to 'SMBus' (Jean Delvare)
 - Remove unnecessary NULL assignment in piix4_sb800_region_release().
   (Jean Delvare)
 - Move 'u8' variable definitions to single line. (Jean Delvare)
 - Hardcode piix4_setup_sb800_smba() return value to 0 since it is always
   0. (Jean Delvare)

Terry Bowman (9):
  kernel/resource: Introduce request_muxed_mem_region()
  i2c: piix4: Replace hardcoded memory map size with a #define
  i2c: piix4: Move port I/O region request/release code into functions
  i2c: piix4: Move SMBus controller base address detect into function
  i2c: piix4: Move SMBus port selection into function
  i2c: piix4: Add EFCH MMIO support to region request and release
  i2c: piix4: Add EFCH MMIO support to SMBus base address detect
  i2c: piix4: Add EFCH MMIO support for SMBus port select
  i2c: piix4: Enable EFCH MMIO for Family 17h+

 drivers/i2c/busses/i2c-piix4.c | 208 ++++++++++++++++++++++++++-------
 include/linux/ioport.h         |   2 +
 2 files changed, 165 insertions(+), 45 deletions(-)

-- 
2.30.2

[PATCH v3 1/9] kernel/resource: Introduce request_muxed_mem_region()

[Terry Bowman]

Support for requesting muxed memory region is implemented but not
currently callable as a macro. Add the request muxed memory
region macro.

MMIO memory accesses can be synchronized using request_mem_region() which
is already available. This call will return failure if the resource is
busy. The 'muxed' version of this macro will handle a busy resource by
using a wait queue to retry until the resource is available.

Signed-off-by: Terry Bowman <terry.bowman@amd.com>
---
 include/linux/ioport.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/ioport.h b/include/linux/ioport.h
index 8359c50f9988..2a5567de318b 100644
--- a/include/linux/ioport.h
+++ b/include/linux/ioport.h
@@ -262,6 +262,8 @@ resource_union(struct resource *r1, struct resource *r2, struct resource *r)
 #define request_muxed_region(start,n,name)	__request_region(&ioport_resource, (start), (n), (name), IORESOURCE_MUXED)
 #define __request_mem_region(start,n,name, excl) __request_region(&iomem_resource, (start), (n), (name), excl)
 #define request_mem_region(start,n,name) __request_region(&iomem_resource, (start), (n), (name), 0)
+#define request_muxed_mem_region(start, n, name) \
+	__request_region(&iomem_resource, (start), (n), (name), IORESOURCE_MUXED)
 #define request_mem_region_exclusive(start,n,name) \
 	__request_region(&iomem_resource, (start), (n), (name), IORESOURCE_EXCLUSIVE)
 #define rename_region(region, newname) do { (region)->name = (newname); } while (0)
-- 
2.30.2

[PATCH v3 2/9] i2c: piix4: Replace hardcoded memory map size with a #define

[Terry Bowman]

Replace number constant with #define to improve readability and
maintainability.

Signed-off-by: Terry Bowman <terry.bowman@amd.com>
---
 drivers/i2c/busses/i2c-piix4.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index 8c1b31ed0c42..b88d990b013f 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -77,6 +77,7 @@
 
 /* SB800 constants */
 #define SB800_PIIX4_SMB_IDX		0xcd6
+#define SB800_PIIX4_SMB_MAP_SIZE        2
 
 #define KERNCZ_IMC_IDX			0x3e
 #define KERNCZ_IMC_DATA			0x3f
@@ -290,7 +291,9 @@ static int piix4_setup_sb800(struct pci_dev *PIIX4_dev,
 	else
 		smb_en = (aux) ? 0x28 : 0x2c;
 
-	if (!request_muxed_region(SB800_PIIX4_SMB_IDX, 2, "sb800_piix4_smb")) {
+	if (!request_muxed_region(SB800_PIIX4_SMB_IDX,
+				  SB800_PIIX4_SMB_MAP_SIZE,
+				  "sb800_piix4_smb")) {
 		dev_err(&PIIX4_dev->dev,
 			"SMB base address index region 0x%x already in use.\n",
 			SB800_PIIX4_SMB_IDX);
@@ -302,7 +305,7 @@ static int piix4_setup_sb800(struct pci_dev *PIIX4_dev,
 	outb_p(smb_en + 1, SB800_PIIX4_SMB_IDX);
 	smba_en_hi = inb_p(SB800_PIIX4_SMB_IDX + 1);
 
-	release_region(SB800_PIIX4_SMB_IDX, 2);
+	release_region(SB800_PIIX4_SMB_IDX, SB800_PIIX4_SMB_MAP_SIZE);
 
 	if (!smb_en) {
 		smb_en_status = smba_en_lo & 0x10;
@@ -371,7 +374,8 @@ static int piix4_setup_sb800(struct pci_dev *PIIX4_dev,
 			piix4_port_shift_sb800 = SB800_PIIX4_PORT_IDX_SHIFT;
 		}
 	} else {
-		if (!request_muxed_region(SB800_PIIX4_SMB_IDX, 2,
+		if (!request_muxed_region(SB800_PIIX4_SMB_IDX,
+					  SB800_PIIX4_SMB_MAP_SIZE,
 					  "sb800_piix4_smb")) {
 			release_region(piix4_smba, SMBIOSIZE);
 			return -EBUSY;
@@ -384,7 +388,7 @@ static int piix4_setup_sb800(struct pci_dev *PIIX4_dev,
 				       SB800_PIIX4_PORT_IDX;
 		piix4_port_mask_sb800 = SB800_PIIX4_PORT_IDX_MASK;
 		piix4_port_shift_sb800 = SB800_PIIX4_PORT_IDX_SHIFT;
-		release_region(SB800_PIIX4_SMB_IDX, 2);
+		release_region(SB800_PIIX4_SMB_IDX, SB800_PIIX4_SMB_MAP_SIZE);
 	}
 
 	dev_info(&PIIX4_dev->dev,
@@ -682,7 +686,9 @@ static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr,
 	u8 port;
 	int retval;
 
-	if (!request_muxed_region(SB800_PIIX4_SMB_IDX, 2, "sb800_piix4_smb"))
+	if (!request_muxed_region(SB800_PIIX4_SMB_IDX,
+				  SB800_PIIX4_SMB_MAP_SIZE,
+				  "sb800_piix4_smb"))
 		return -EBUSY;
 
 	/* Request the SMBUS semaphore, avoid conflicts with the IMC */
@@ -758,7 +764,7 @@ static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr,
 		piix4_imc_wakeup();
 
 release:
-	release_region(SB800_PIIX4_SMB_IDX, 2);
+	release_region(SB800_PIIX4_SMB_IDX, SB800_PIIX4_SMB_MAP_SIZE);
 	return retval;
 }
 
-- 
2.30.2

[PATCH v3 3/9] i2c: piix4: Move port I/O region request/release code into functions

[Terry Bowman]

Move duplicated region request and release code into a function. Move is
in preparation for following MMIO changes.

Signed-off-by: Terry Bowman <terry.bowman@amd.com>
---
 drivers/i2c/busses/i2c-piix4.c | 43 +++++++++++++++++++++++-----------
 1 file changed, 29 insertions(+), 14 deletions(-)

diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index b88d990b013f..14324e03fe24 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -165,6 +165,26 @@ struct i2c_piix4_adapdata {
 	u8 port;		/* Port number, shifted */
 };
 
+static int piix4_sb800_region_setup(struct device *dev)
+{
+	if (!request_muxed_region(SB800_PIIX4_SMB_IDX,
+				  SB800_PIIX4_SMB_MAP_SIZE,
+				  "sb800_piix4_smb")) {
+		dev_err(dev,
+			"SMB base address index region 0x%x already in use.\n",
+			SB800_PIIX4_SMB_IDX);
+		return -EBUSY;
+	}
+
+	return 0;
+}
+
+static void piix4_sb800_region_release(struct device *dev)
+{
+	release_region(SB800_PIIX4_SMB_IDX,
+		       SB800_PIIX4_SMB_MAP_SIZE);
+}
+
 static int piix4_setup(struct pci_dev *PIIX4_dev,
 		       const struct pci_device_id *id)
 {
@@ -270,6 +290,7 @@ static int piix4_setup_sb800(struct pci_dev *PIIX4_dev,
 	unsigned short piix4_smba;
 	u8 smba_en_lo, smba_en_hi, smb_en, smb_en_status, port_sel;
 	u8 i2ccfg, i2ccfg_offset = 0x10;
+	int retval;
 
 	/* SB800 and later SMBus does not support forcing address */
 	if (force || force_addr) {
@@ -291,21 +312,16 @@ static int piix4_setup_sb800(struct pci_dev *PIIX4_dev,
 	else
 		smb_en = (aux) ? 0x28 : 0x2c;
 
-	if (!request_muxed_region(SB800_PIIX4_SMB_IDX,
-				  SB800_PIIX4_SMB_MAP_SIZE,
-				  "sb800_piix4_smb")) {
-		dev_err(&PIIX4_dev->dev,
-			"SMB base address index region 0x%x already in use.\n",
-			SB800_PIIX4_SMB_IDX);
-		return -EBUSY;
-	}
+	retval = piix4_sb800_region_setup(&PIIX4_dev->dev);
+	if (retval)
+		return retval;
 
 	outb_p(smb_en, SB800_PIIX4_SMB_IDX);
 	smba_en_lo = inb_p(SB800_PIIX4_SMB_IDX + 1);
 	outb_p(smb_en + 1, SB800_PIIX4_SMB_IDX);
 	smba_en_hi = inb_p(SB800_PIIX4_SMB_IDX + 1);
 
-	release_region(SB800_PIIX4_SMB_IDX, SB800_PIIX4_SMB_MAP_SIZE);
+	piix4_sb800_region_release(&PIIX4_dev->dev);
 
 	if (!smb_en) {
 		smb_en_status = smba_en_lo & 0x10;
@@ -686,10 +702,9 @@ static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr,
 	u8 port;
 	int retval;
 
-	if (!request_muxed_region(SB800_PIIX4_SMB_IDX,
-				  SB800_PIIX4_SMB_MAP_SIZE,
-				  "sb800_piix4_smb"))
-		return -EBUSY;
+	retval = piix4_sb800_region_setup(&adap->dev);
+	if (retval)
+		return retval;
 
 	/* Request the SMBUS semaphore, avoid conflicts with the IMC */
 	smbslvcnt  = inb_p(SMBSLVCNT);
@@ -764,7 +779,7 @@ static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr,
 		piix4_imc_wakeup();
 
 release:
-	release_region(SB800_PIIX4_SMB_IDX, SB800_PIIX4_SMB_MAP_SIZE);
+	piix4_sb800_region_release(&adap->dev);
 	return retval;
 }
 
-- 
2.30.2

[PATCH v3 4/9] i2c: piix4: Move SMBus controller base address detect into function

[Terry Bowman]

Move SMBus controller base address detection into function. Refactor
is in preparation for following MMIO changes.

Signed-off-by: Terry Bowman <terry.bowman@amd.com>
---
 drivers/i2c/busses/i2c-piix4.c | 68 +++++++++++++++++++++-------------
 1 file changed, 43 insertions(+), 25 deletions(-)

diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index 14324e03fe24..35fcb61f4750 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -284,11 +284,51 @@ static int piix4_setup(struct pci_dev *PIIX4_dev,
 	return piix4_smba;
 }
 
+static int piix4_setup_sb800_smba(struct pci_dev *PIIX4_dev,
+				  u8 smb_en,
+				  u8 aux,
+				  u8 *smb_en_status,
+				  unsigned short *piix4_smba)
+{
+	u8 smba_en_lo;
+	u8 smba_en_hi;
+	int retval;
+
+	retval = piix4_sb800_region_setup(&PIIX4_dev->dev);
+	if (retval)
+		return retval;
+
+	outb_p(smb_en, SB800_PIIX4_SMB_IDX);
+	smba_en_lo = inb_p(SB800_PIIX4_SMB_IDX + 1);
+	outb_p(smb_en + 1, SB800_PIIX4_SMB_IDX);
+	smba_en_hi = inb_p(SB800_PIIX4_SMB_IDX + 1);
+
+	piix4_sb800_region_release(&PIIX4_dev->dev);
+
+	if (!smb_en) {
+		*smb_en_status = smba_en_lo & 0x10;
+		*piix4_smba = smba_en_hi << 8;
+		if (aux)
+			*piix4_smba |= 0x20;
+	} else {
+		*smb_en_status = smba_en_lo & 0x01;
+		*piix4_smba = ((smba_en_hi << 8) | smba_en_lo) & 0xffe0;
+	}
+
+	if (!*smb_en_status) {
+		dev_err(&PIIX4_dev->dev,
+			"SMBus Host Controller not enabled!\n");
+		return -ENODEV;
+	}
+
+	return retval;
+}
+
 static int piix4_setup_sb800(struct pci_dev *PIIX4_dev,
 			     const struct pci_device_id *id, u8 aux)
 {
 	unsigned short piix4_smba;
-	u8 smba_en_lo, smba_en_hi, smb_en, smb_en_status, port_sel;
+	u8 smb_en, smb_en_status, port_sel;
 	u8 i2ccfg, i2ccfg_offset = 0x10;
 	int retval;
 
@@ -312,33 +352,11 @@ static int piix4_setup_sb800(struct pci_dev *PIIX4_dev,
 	else
 		smb_en = (aux) ? 0x28 : 0x2c;
 
-	retval = piix4_sb800_region_setup(&PIIX4_dev->dev);
+	retval = piix4_setup_sb800_smba(PIIX4_dev, smb_en,
+					aux, &smb_en_status, &piix4_smba);
 	if (retval)
 		return retval;
 
-	outb_p(smb_en, SB800_PIIX4_SMB_IDX);
-	smba_en_lo = inb_p(SB800_PIIX4_SMB_IDX + 1);
-	outb_p(smb_en + 1, SB800_PIIX4_SMB_IDX);
-	smba_en_hi = inb_p(SB800_PIIX4_SMB_IDX + 1);
-
-	piix4_sb800_region_release(&PIIX4_dev->dev);
-
-	if (!smb_en) {
-		smb_en_status = smba_en_lo & 0x10;
-		piix4_smba = smba_en_hi << 8;
-		if (aux)
-			piix4_smba |= 0x20;
-	} else {
-		smb_en_status = smba_en_lo & 0x01;
-		piix4_smba = ((smba_en_hi << 8) | smba_en_lo) & 0xffe0;
-	}
-
-	if (!smb_en_status) {
-		dev_err(&PIIX4_dev->dev,
-			"SMBus Host Controller not enabled!\n");
-		return -ENODEV;
-	}
-
 	if (acpi_check_region(piix4_smba, SMBIOSIZE, piix4_driver.name))
 		return -ENODEV;
 
-- 
2.30.2

[PATCH v3 5/9] i2c: piix4: Move SMBus port selection into function

[Terry Bowman]

Move port selection code into a separate function. Refactor is in
preparation for following MMIO changes.

Signed-off-by: Terry Bowman <terry.bowman@amd.com>
---
 drivers/i2c/busses/i2c-piix4.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index 35fcb61f4750..58b613b78c97 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -700,6 +700,19 @@ static void piix4_imc_wakeup(void)
 	release_region(KERNCZ_IMC_IDX, 2);
 }
 
+static int piix4_sb800_port_sel(u8 port)
+{
+	u8 smba_en_lo;
+
+	outb_p(piix4_port_sel_sb800, SB800_PIIX4_SMB_IDX);
+	smba_en_lo = inb_p(SB800_PIIX4_SMB_IDX + 1);
+
+	if ((smba_en_lo & piix4_port_mask_sb800) != port)
+		outb_p((smba_en_lo & ~piix4_port_mask_sb800) | port,
+		       SB800_PIIX4_SMB_IDX + 1);
+
+	return (smba_en_lo & piix4_port_mask_sb800);
+}
 /*
  * Handles access to multiple SMBus ports on the SB800.
  * The port is selected by bits 2:1 of the smb_en register (0x2c).
@@ -716,8 +729,7 @@ static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr,
 	unsigned short piix4_smba = adapdata->smba;
 	int retries = MAX_TIMEOUT;
 	int smbslvcnt;
-	u8 smba_en_lo;
-	u8 port;
+	u8 prev_port;
 	int retval;
 
 	retval = piix4_sb800_region_setup(&adap->dev);
@@ -777,18 +789,12 @@ static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr,
 		}
 	}
 
-	outb_p(piix4_port_sel_sb800, SB800_PIIX4_SMB_IDX);
-	smba_en_lo = inb_p(SB800_PIIX4_SMB_IDX + 1);
-
-	port = adapdata->port;
-	if ((smba_en_lo & piix4_port_mask_sb800) != port)
-		outb_p((smba_en_lo & ~piix4_port_mask_sb800) | port,
-		       SB800_PIIX4_SMB_IDX + 1);
+	prev_port = piix4_sb800_port_sel(adapdata->port);
 
 	retval = piix4_access(adap, addr, flags, read_write,
 			      command, size, data);
 
-	outb_p(smba_en_lo, SB800_PIIX4_SMB_IDX + 1);
+	piix4_sb800_port_sel(prev_port);
 
 	/* Release the semaphore */
 	outb_p(smbslvcnt | 0x20, SMBSLVCNT);
-- 
2.30.2

[PATCH v3 6/9] i2c: piix4: Add EFCH MMIO support to region request and release

[Terry Bowman]

EFCH cd6h/cd7h port I/O may no longer be available on later AMD
processors and it is recommended to use MMIO instead. Update the
request and release functions to support MMIO.

MMIO request/release and mmapping require details during cleanup.
Add a MMIO configuration structure containing resource and vaddress
details for mapping the region, accessing the region, and releasing
the region.

Signed-off-by: Terry Bowman <terry.bowman@amd.com>
---
 drivers/i2c/busses/i2c-piix4.c | 76 +++++++++++++++++++++++++++-------
 1 file changed, 61 insertions(+), 15 deletions(-)

diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index 58b613b78c97..7aa6ceb27e4b 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -98,6 +98,9 @@
 #define SB800_PIIX4_PORT_IDX_MASK_KERNCZ	0x18
 #define SB800_PIIX4_PORT_IDX_SHIFT_KERNCZ	3
 
+#define SB800_PIIX4_FCH_PM_ADDR                 0xFED80300
+#define SB800_PIIX4_FCH_PM_SIZE                 8
+
 /* insmod parameters */
 
 /* If force is set to anything different from 0, we forcibly enable the
@@ -156,6 +159,12 @@ static const char *piix4_main_port_names_sb800[PIIX4_MAX_ADAPTERS] = {
 };
 static const char *piix4_aux_port_name_sb800 = " port 1";
 
+struct sb800_mmio_cfg {
+	void __iomem *addr;
+	struct resource *res;
+	bool use_mmio;
+};
+
 struct i2c_piix4_adapdata {
 	unsigned short smba;
 
@@ -163,26 +172,60 @@ struct i2c_piix4_adapdata {
 	bool sb800_main;
 	bool notify_imc;
 	u8 port;		/* Port number, shifted */
+	struct sb800_mmio_cfg mmio_cfg;
 };
 
-static int piix4_sb800_region_setup(struct device *dev)
+static int piix4_sb800_region_setup(struct device *dev,
+				    struct sb800_mmio_cfg *mmio_cfg)
 {
-	if (!request_muxed_region(SB800_PIIX4_SMB_IDX,
-				  SB800_PIIX4_SMB_MAP_SIZE,
-				  "sb800_piix4_smb")) {
-		dev_err(dev,
-			"SMB base address index region 0x%x already in use.\n",
-			SB800_PIIX4_SMB_IDX);
-		return -EBUSY;
+	if (mmio_cfg->use_mmio) {
+		struct resource *res;
+		void __iomem *addr;
+
+		res = request_muxed_mem_region(SB800_PIIX4_FCH_PM_ADDR,
+					       SB800_PIIX4_FCH_PM_SIZE,
+					       "sb800_piix4_smb");
+		if (!res) {
+			dev_err(dev,
+				"SMBus base address memory region 0x%x already in use.\n",
+				SB800_PIIX4_FCH_PM_ADDR);
+			return -EBUSY;
+		}
+
+		addr = ioremap(SB800_PIIX4_FCH_PM_ADDR,
+			       SB800_PIIX4_FCH_PM_SIZE);
+		if (!addr) {
+			release_resource(res);
+			dev_err(dev, "SMBus base address mapping failed.\n");
+			return -ENOMEM;
+		}
+
+		mmio_cfg->res = res;
+		mmio_cfg->addr = addr;
+	} else {
+		if (!request_muxed_region(SB800_PIIX4_SMB_IDX,
+					  SB800_PIIX4_SMB_MAP_SIZE,
+					  "sb800_piix4_smb")) {
+			dev_err(dev,
+				"SMBus base address index region 0x%x already in use.\n",
+				SB800_PIIX4_SMB_IDX);
+			return -EBUSY;
+		}
 	}
 
 	return 0;
 }
 
-static void piix4_sb800_region_release(struct device *dev)
+static void piix4_sb800_region_release(struct device *dev,
+				       struct sb800_mmio_cfg *mmio_cfg)
 {
-	release_region(SB800_PIIX4_SMB_IDX,
-		       SB800_PIIX4_SMB_MAP_SIZE);
+	if (mmio_cfg->use_mmio) {
+		iounmap(mmio_cfg->addr);
+		release_resource(mmio_cfg->res);
+	} else {
+		release_region(SB800_PIIX4_SMB_IDX,
+			       SB800_PIIX4_SMB_MAP_SIZE);
+	}
 }
 
 static int piix4_setup(struct pci_dev *PIIX4_dev,
@@ -290,11 +333,14 @@ static int piix4_setup_sb800_smba(struct pci_dev *PIIX4_dev,
 				  u8 *smb_en_status,
 				  unsigned short *piix4_smba)
 {
+	struct sb800_mmio_cfg mmio_cfg;
 	u8 smba_en_lo;
 	u8 smba_en_hi;
 	int retval;
 
-	retval = piix4_sb800_region_setup(&PIIX4_dev->dev);
+	mmio_cfg.use_mmio = 0;
+
+	retval = piix4_sb800_region_setup(&PIIX4_dev->dev, &mmio_cfg);
 	if (retval)
 		return retval;
 
@@ -303,7 +349,7 @@ static int piix4_setup_sb800_smba(struct pci_dev *PIIX4_dev,
 	outb_p(smb_en + 1, SB800_PIIX4_SMB_IDX);
 	smba_en_hi = inb_p(SB800_PIIX4_SMB_IDX + 1);
 
-	piix4_sb800_region_release(&PIIX4_dev->dev);
+	piix4_sb800_region_release(&PIIX4_dev->dev, &mmio_cfg);
 
 	if (!smb_en) {
 		*smb_en_status = smba_en_lo & 0x10;
@@ -732,7 +778,7 @@ static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr,
 	u8 prev_port;
 	int retval;
 
-	retval = piix4_sb800_region_setup(&adap->dev);
+	retval = piix4_sb800_region_setup(&adap->dev, &adapdata->mmio_cfg);
 	if (retval)
 		return retval;
 
@@ -803,7 +849,7 @@ static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr,
 		piix4_imc_wakeup();
 
 release:
-	piix4_sb800_region_release(&adap->dev);
+	piix4_sb800_region_release(&adap->dev, &adapdata->mmio_cfg);
 	return retval;
 }
 
-- 
2.30.2

[PATCH v3 7/9] i2c: piix4: Add EFCH MMIO support to SMBus base address detect

[Terry Bowman]

The EFCH SMBus controller's base address is determined using details in
FCH::PM::DECODEEN[smbusasfiobase] and FCH::PM::DECODEEN[smbusasfioen].
This code also writes to FCH::PM::ISACONTROL[mmioen] to enable MMIO
decoding. These register fields were accessed using cd6h/cd7h port I/O.
cd6h/cd7h port I/O is no longer available in later AMD processors.
Change base address detection to use MMIO instead of port I/O cd6h/cd7h.

Signed-off-by: Terry Bowman <terry.bowman@amd.com>
---
 drivers/i2c/busses/i2c-piix4.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index 7aa6ceb27e4b..b85f926451b8 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -98,6 +98,7 @@
 #define SB800_PIIX4_PORT_IDX_MASK_KERNCZ	0x18
 #define SB800_PIIX4_PORT_IDX_SHIFT_KERNCZ	3
 
+#define SB800_PIIX4_FCH_PM_DECODEEN_MMIO        BIT(1)
 #define SB800_PIIX4_FCH_PM_ADDR                 0xFED80300
 #define SB800_PIIX4_FCH_PM_SIZE                 8
 
@@ -344,10 +345,18 @@ static int piix4_setup_sb800_smba(struct pci_dev *PIIX4_dev,
 	if (retval)
 		return retval;
 
-	outb_p(smb_en, SB800_PIIX4_SMB_IDX);
-	smba_en_lo = inb_p(SB800_PIIX4_SMB_IDX + 1);
-	outb_p(smb_en + 1, SB800_PIIX4_SMB_IDX);
-	smba_en_hi = inb_p(SB800_PIIX4_SMB_IDX + 1);
+	if (mmio_cfg.use_mmio) {
+		iowrite32(ioread32(mmio_cfg.addr + 4) | SB800_PIIX4_FCH_PM_DECODEEN_MMIO,
+			  mmio_cfg.addr + 4);
+
+		smba_en_lo = ioread8(mmio_cfg.addr);
+		smba_en_hi = ioread8(mmio_cfg.addr + 1);
+	} else {
+		outb_p(smb_en, SB800_PIIX4_SMB_IDX);
+		smba_en_lo = inb_p(SB800_PIIX4_SMB_IDX + 1);
+		outb_p(smb_en + 1, SB800_PIIX4_SMB_IDX);
+		smba_en_hi = inb_p(SB800_PIIX4_SMB_IDX + 1);
+	}
 
 	piix4_sb800_region_release(&PIIX4_dev->dev, &mmio_cfg);
 
-- 
2.30.2

[PATCH v3 8/9] i2c: piix4: Add EFCH MMIO support for SMBus port select

[Terry Bowman]

AMD processors include registers capable of selecting between 2 SMBus
ports. Port selection is made during each user access by writing to
FCH::PM::DECODEEN[smbus0sel]. Change the driver to use MMIO during
SMBus port selection because cd6h/cd7h port I/O is not available on
later AMD processors.

Signed-off-by: Terry Bowman <terry.bowman@amd.com>
---
 drivers/i2c/busses/i2c-piix4.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index b85f926451b8..1811bdc96363 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -755,19 +755,28 @@ static void piix4_imc_wakeup(void)
 	release_region(KERNCZ_IMC_IDX, 2);
 }
 
-static int piix4_sb800_port_sel(u8 port)
+static int piix4_sb800_port_sel(u8 port, struct sb800_mmio_cfg *mmio_cfg)
 {
 	u8 smba_en_lo;
 
-	outb_p(piix4_port_sel_sb800, SB800_PIIX4_SMB_IDX);
-	smba_en_lo = inb_p(SB800_PIIX4_SMB_IDX + 1);
+	if (mmio_cfg->use_mmio) {
+		smba_en_lo = ioread8(mmio_cfg->addr + piix4_port_sel_sb800);
+
+		if ((smba_en_lo & piix4_port_mask_sb800) != port)
+			iowrite8((smba_en_lo & ~piix4_port_mask_sb800) | port,
+				 mmio_cfg->addr + piix4_port_sel_sb800);
+	} else {
+		outb_p(piix4_port_sel_sb800, SB800_PIIX4_SMB_IDX);
+		smba_en_lo = inb_p(SB800_PIIX4_SMB_IDX + 1);
 
-	if ((smba_en_lo & piix4_port_mask_sb800) != port)
-		outb_p((smba_en_lo & ~piix4_port_mask_sb800) | port,
-		       SB800_PIIX4_SMB_IDX + 1);
+		if ((smba_en_lo & piix4_port_mask_sb800) != port)
+			outb_p((smba_en_lo & ~piix4_port_mask_sb800) | port,
+			       SB800_PIIX4_SMB_IDX + 1);
+	}
 
 	return (smba_en_lo & piix4_port_mask_sb800);
 }
+
 /*
  * Handles access to multiple SMBus ports on the SB800.
  * The port is selected by bits 2:1 of the smb_en register (0x2c).
@@ -844,12 +853,12 @@ static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr,
 		}
 	}
 
-	prev_port = piix4_sb800_port_sel(adapdata->port);
+	prev_port = piix4_sb800_port_sel(adapdata->port, &adapdata->mmio_cfg);
 
 	retval = piix4_access(adap, addr, flags, read_write,
 			      command, size, data);
 
-	piix4_sb800_port_sel(prev_port);
+	piix4_sb800_port_sel(prev_port, &adapdata->mmio_cfg);
 
 	/* Release the semaphore */
 	outb_p(smbslvcnt | 0x20, SMBSLVCNT);
-- 
2.30.2

[PATCH v3 9/9] i2c: piix4: Enable EFCH MMIO for Family 17h+

[Terry Bowman]

Enable EFCH MMIO using check for SMBus PCI revision ID value 0x51 or
greater. SMBus PCI revision ID 0x51 is first used by family 17h. This
PCI revision ID check will also enable future AMD processors with the
same EFCH SMBus controller HW.

Signed-off-by: Terry Bowman <terry.bowman@amd.com>
---
 drivers/i2c/busses/i2c-piix4.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index 1811bdc96363..1e8061c46a89 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -102,6 +102,8 @@
 #define SB800_PIIX4_FCH_PM_ADDR                 0xFED80300
 #define SB800_PIIX4_FCH_PM_SIZE                 8
 
+#define AMD_PCI_SMBUS_REVISION_MMIO             0x51
+
 /* insmod parameters */
 
 /* If force is set to anything different from 0, we forcibly enable the
@@ -229,6 +231,13 @@ static void piix4_sb800_region_release(struct device *dev,
 	}
 }
 
+static bool piix4_sb800_use_mmio(struct pci_dev *PIIX4_dev)
+{
+	return (PIIX4_dev->vendor == PCI_VENDOR_ID_AMD &&
+		PIIX4_dev->device == PCI_DEVICE_ID_AMD_KERNCZ_SMBUS &&
+		PIIX4_dev->revision >= AMD_PCI_SMBUS_REVISION_MMIO);
+}
+
 static int piix4_setup(struct pci_dev *PIIX4_dev,
 		       const struct pci_device_id *id)
 {
@@ -339,7 +348,7 @@ static int piix4_setup_sb800_smba(struct pci_dev *PIIX4_dev,
 	u8 smba_en_hi;
 	int retval;
 
-	mmio_cfg.use_mmio = 0;
+	mmio_cfg.use_mmio = piix4_sb800_use_mmio(PIIX4_dev);
 
 	retval = piix4_sb800_region_setup(&PIIX4_dev->dev, &mmio_cfg);
 	if (retval)
-- 
2.30.2

Re: [PATCH v3 1/9] kernel/resource: Introduce request_muxed_mem_region()

[Andy Shevchenko]

On Thu, Jan 20, 2022 at 1:06 AM Terry Bowman <terry.bowman@amd.com> wrote:
>
> Support for requesting muxed memory region is implemented but not
> currently callable as a macro. Add the request muxed memory
> region macro.
>
> MMIO memory accesses can be synchronized using request_mem_region() which
> is already available. This call will return failure if the resource is
> busy. The 'muxed' version of this macro will handle a busy resource by
> using a wait queue to retry until the resource is available.

...

> @@ -262,6 +262,8 @@ resource_union(struct resource *r1, struct resource *r2, struct resource *r)
>  #define request_muxed_region(start,n,name)     __request_region(&ioport_resource, (start), (n), (name), IORESOURCE_MUXED)
>  #define __request_mem_region(start,n,name, excl) __request_region(&iomem_resource, (start), (n), (name), excl)
>  #define request_mem_region(start,n,name) __request_region(&iomem_resource, (start), (n), (name), 0)

> +#define request_muxed_mem_region(start, n, name) \
> +       __request_region(&iomem_resource, (start), (n), (name), IORESOURCE_MUXED)

Looking around tells me that this name is inconsistent, I would expect it to be

request_mem_region_muxed()

>  #define request_mem_region_exclusive(start,n,name) \
>         __request_region(&iomem_resource, (start), (n), (name), IORESOURCE_EXCLUSIVE)

If you are fine with this, take my
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 7/9] i2c: piix4: Add EFCH MMIO support to SMBus base address detect

[Andy Shevchenko]

On Thu, Jan 20, 2022 at 1:07 AM Terry Bowman <terry.bowman@amd.com> wrote:
>
> The EFCH SMBus controller's base address is determined using details in
> FCH::PM::DECODEEN[smbusasfiobase] and FCH::PM::DECODEEN[smbusasfioen].
> This code also writes to FCH::PM::ISACONTROL[mmioen] to enable MMIO
> decoding. These register fields were accessed using cd6h/cd7h port I/O.
> cd6h/cd7h port I/O is no longer available in later AMD processors.
> Change base address detection to use MMIO instead of port I/O cd6h/cd7h.

...

> +       if (mmio_cfg.use_mmio) {

> +               iowrite32(ioread32(mmio_cfg.addr + 4) | SB800_PIIX4_FCH_PM_DECODEEN_MMIO,
> +                         mmio_cfg.addr + 4);

Can you split this to three lines (with the help of a temporary variable)?

> +               smba_en_lo = ioread8(mmio_cfg.addr);
> +               smba_en_hi = ioread8(mmio_cfg.addr + 1);

This makes me wonder if we can replace these two by defining

  u16 smba_en;

(below also may be easily adjusted for it).

> +       } else {
> +               outb_p(smb_en, SB800_PIIX4_SMB_IDX);
> +               smba_en_lo = inb_p(SB800_PIIX4_SMB_IDX + 1);
> +               outb_p(smb_en + 1, SB800_PIIX4_SMB_IDX);
> +               smba_en_hi = inb_p(SB800_PIIX4_SMB_IDX + 1);
> +       }
>
>         piix4_sb800_region_release(&PIIX4_dev->dev, &mmio_cfg);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 8/9] i2c: piix4: Add EFCH MMIO support for SMBus port select

[Andy Shevchenko]

On Thu, Jan 20, 2022 at 1:08 AM Terry Bowman <terry.bowman@amd.com> wrote:
>
> AMD processors include registers capable of selecting between 2 SMBus
> ports. Port selection is made during each user access by writing to
> FCH::PM::DECODEEN[smbus0sel]. Change the driver to use MMIO during
> SMBus port selection because cd6h/cd7h port I/O is not available on
> later AMD processors.

...

>  }
> +
>  /*

Stray change.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 0/9] i2c: piix4: Replace cd6h/cd7h port I/O accesses with MMIO accesses

[Andy Shevchenko]

On Thu, Jan 20, 2022 at 1:06 AM Terry Bowman <terry.bowman@amd.com> wrote:
>
> This series changes the piix4_smbus driver's cd6h/cd7h port I/O accesses
> to use MMIO instead. This is necessary because cd6h/cd7h port I/O may be
> disabled on later AMD processors.
>
> This series includes patches with MMIO accesses to registers
> FCH::PM::DECODEEN and FCH::PM::ISACONTROL. The same registers are also
> accessed by the sp5100_tco driver.[1] Synchronization to the MMIO
> registers is required in both drivers.
>
> The first patch creates a macro to request MMIO region using the 'muxed'
> retry logic. This is used in patch 6 to synchronize accesses to EFCH MMIO.
>
> The second patch replaces a hardcoded region size with a #define. This is
> to improve maintainability and was requested from v2 review.
>
> The third patch moves duplicated region request/release code into
> functions. This locates related code into functions and reduces code line
> count. This will also make adding MMIO support in patch 6 easier.
>
> The fourth patch moves SMBus controller address detection into a function.
> This is in preparation for adding MMIO region support.
>
> The fifth patch moves EFCH port selection into a function. This is in
> preparation for adding MMIO region support.
>
> The sixth patch adds MMIO support for region requesting/releasing and
> mapping. This is necessary for using MMIO to detect SMBus controller
> address, enable SMBbus controller region, and control the port select.
>
> The seventh patch updates the SMBus controller address detection to support
> using MMIO. This is necessary because the driver accesses registers
> FCH::PM::DECODEEN and FCH::PM::ISACONTOL during initialization and they are
> only available using MMIO on later AMD processors.
>
> The eighth patch updates the SMBus port selection to support MMIO. This is
> required because port selection control resides in the
> FCH::PM::DECODEEN[smbus0sel] and is only accessible using MMIO on later AMD
> processors.
>
> The ninth patch enables the EFCH MMIO functionality added earlier in this
> series. The SMBus controller's PCI revision ID is used to check if EFCH
> MMIO is supported by HW and should be enabled in the driver.

In general looks good to me, but I believe it will be much better if
we agreed on converting driver to use iomap_port() +
ioreadXX()/iowriteXX() (means dropping I/O accessor _p variants for
good). This would make the series cleaner and less invasive.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 1/9] kernel/resource: Introduce request_muxed_mem_region()

[Terry Bowman]

On 1/20/22 5:16 AM, Andy Shevchenko wrote:
> On Thu, Jan 20, 2022 at 1:06 AM Terry Bowman <terry.bowman@amd.com> wrote:
>>
>> Support for requesting muxed memory region is implemented but not
>> currently callable as a macro. Add the request muxed memory
>> region macro.
>>
>> MMIO memory accesses can be synchronized using request_mem_region() which
>> is already available. This call will return failure if the resource is
>> busy. The 'muxed' version of this macro will handle a busy resource by
>> using a wait queue to retry until the resource is available.
> 
> ...
> 
>> @@ -262,6 +262,8 @@ resource_union(struct resource *r1, struct resource *r2, struct resource *r)
>>  #define request_muxed_region(start,n,name)     __request_region(&ioport_resource, (start), (n), (name), IORESOURCE_MUXED)
>>  #define __request_mem_region(start,n,name, excl) __request_region(&iomem_resource, (start), (n), (name), excl)
>>  #define request_mem_region(start,n,name) __request_region(&iomem_resource, (start), (n), (name), 0)
> 
>> +#define request_muxed_mem_region(start, n, name) \
>> +       __request_region(&iomem_resource, (start), (n), (name), IORESOURCE_MUXED)
> 
> Looking around tells me that this name is inconsistent, I would expect it to be
> 
> request_mem_region_muxed()
> 
>>  #define request_mem_region_exclusive(start,n,name) \
>>         __request_region(&iomem_resource, (start), (n), (name), IORESOURCE_EXCLUSIVE)
> 
> If you are fine with this, take my
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> 
                                                                      
I'm fine with the rename. I'll update as you requested in the next series
revision and add your "reviewed-by" to this patch.

Regards,
Terry

Re: [PATCH v3 7/9] i2c: piix4: Add EFCH MMIO support to SMBus base address detect

[Terry Bowman]

On 1/20/22 5:27 AM, Andy Shevchenko wrote:
> On Thu, Jan 20, 2022 at 1:07 AM Terry Bowman <terry.bowman@amd.com> wrote:
>>
>> The EFCH SMBus controller's base address is determined using details in
>> FCH::PM::DECODEEN[smbusasfiobase] and FCH::PM::DECODEEN[smbusasfioen].
>> This code also writes to FCH::PM::ISACONTROL[mmioen] to enable MMIO
>> decoding. These register fields were accessed using cd6h/cd7h port I/O.
>> cd6h/cd7h port I/O is no longer available in later AMD processors.
>> Change base address detection to use MMIO instead of port I/O cd6h/cd7h.
> 
> ...
> 
>> +       if (mmio_cfg.use_mmio) {
> 
>> +               iowrite32(ioread32(mmio_cfg.addr + 4) | SB800_PIIX4_FCH_PM_DECODEEN_MMIO,
>> +                         mmio_cfg.addr + 4);
> 
> Can you split this to three lines (with the help of a temporary variable)?

Yes.

> 
>> +               smba_en_lo = ioread8(mmio_cfg.addr);
>> +               smba_en_hi = ioread8(mmio_cfg.addr + 1);
> 
> This makes me wonder if we can replace these two by defining
> 
>   u16 smba_en;
> 
> (below also may be easily adjusted for it).
> 

Yes.

Regards,
Terry

>> +       } else {
>> +               outb_p(smb_en, SB800_PIIX4_SMB_IDX);
>> +               smba_en_lo = inb_p(SB800_PIIX4_SMB_IDX + 1);
>> +               outb_p(smb_en + 1, SB800_PIIX4_SMB_IDX);
>> +               smba_en_hi = inb_p(SB800_PIIX4_SMB_IDX + 1);
>> +       }
>>
>>         piix4_sb800_region_release(&PIIX4_dev->dev, &mmio_cfg);
> 

Re: [PATCH v3 8/9] i2c: piix4: Add EFCH MMIO support for SMBus port select

[Terry Bowman]

On 1/20/22 5:28 AM, Andy Shevchenko wrote:
> On Thu, Jan 20, 2022 at 1:08 AM Terry Bowman <terry.bowman@amd.com> wrote:
>>
>> AMD processors include registers capable of selecting between 2 SMBus
>> ports. Port selection is made during each user access by writing to
>> FCH::PM::DECODEEN[smbus0sel]. Change the driver to use MMIO during
>> SMBus port selection because cd6h/cd7h port I/O is not available on
>> later AMD processors.
> 
> ...
> 
>>  }
>> +
>>  /*
> 
> Stray change.
> 
> 

I'll remove it.

Re: [PATCH v3 8/9] i2c: piix4: Add EFCH MMIO support for SMBus port select

[Terry Bowman]

On 1/20/22 05:28, Andy Shevchenko wrote:
> On Thu, Jan 20, 2022 at 1:08 AM Terry Bowman <terry.bowman@amd.com> wrote:
>>
>> AMD processors include registers capable of selecting between 2 SMBus
>> ports. Port selection is made during each user access by writing to
>> FCH::PM::DECODEEN[smbus0sel]. Change the driver to use MMIO during
>> SMBus port selection because cd6h/cd7h port I/O is not available on
>> later AMD processors.
> 
> ...
> 
>>  }
>> +
>>  /*
> 
> Stray change.
> 
> 

Hi Andy,

Looking at this closer I find the added line separates the closing 
function brace from the next function's comment header. Are you sure I 
need to remove this line?

Regards,
Terry

Re: [PATCH v3 8/9] i2c: piix4: Add EFCH MMIO support for SMBus port select

[Wolfram Sang]

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


> Looking at this closer I find the added line separates the closing 
> function brace from the next function's comment header. Are you sure I 
> need to remove this line?

I agree you can leave it.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 7/9] i2c: piix4: Add EFCH MMIO support to SMBus base address detect

[Terry Bowman]

On 1/20/22 5:27 AM, Andy Shevchenko wrote:
> On Thu, Jan 20, 2022 at 1:07 AM Terry Bowman <terry.bowman@amd.com> wrote:
>>
>> The EFCH SMBus controller's base address is determined using details in
>> FCH::PM::DECODEEN[smbusasfiobase] and FCH::PM::DECODEEN[smbusasfioen].
>> This code also writes to FCH::PM::ISACONTROL[mmioen] to enable MMIO
>> decoding. These register fields were accessed using cd6h/cd7h port I/O.
>> cd6h/cd7h port I/O is no longer available in later AMD processors.
>> Change base address detection to use MMIO instead of port I/O cd6h/cd7h.
> 
> ...
> 
>> +       if (mmio_cfg.use_mmio) {
> 
>> +               iowrite32(ioread32(mmio_cfg.addr + 4) | SB800_PIIX4_FCH_PM_DECODEEN_MMIO,
>> +                         mmio_cfg.addr + 4);
> 
> Can you split this to three lines (with the help of a temporary variable)?
> 

I made these changes for upcoming v4.

>> +               smba_en_lo = ioread8(mmio_cfg.addr);
>> +               smba_en_hi = ioread8(mmio_cfg.addr + 1);
> 
> This makes me wonder if we can replace these two by defining
> 
>   u16 smba_en;
> 

Hi Alex,

I was unable to make these changes without losing readability. By using
a u16 smba_en I found casting required between u8 and u16 to access the byte 
offsets. This increased the number of lines and complexity. I should mention 
the port I/O here is fixed at a byte and does not support 16bit port I/O. 
I'll leave as-is if Ok with you.

Regards,
Terry


> (below also may be easily adjusted for it).
> 
>> +       } else {
>> +               outb_p(smb_en, SB800_PIIX4_SMB_IDX);
>> +               smba_en_lo = inb_p(SB800_PIIX4_SMB_IDX + 1);
>> +               outb_p(smb_en + 1, SB800_PIIX4_SMB_IDX);
>> +               smba_en_hi = inb_p(SB800_PIIX4_SMB_IDX + 1);
>> +       }
>>
>>         piix4_sb800_region_release(&PIIX4_dev->dev, &mmio_cfg);
> 

Re: [PATCH v3 7/9] i2c: piix4: Add EFCH MMIO support to SMBus base address detect

[Wolfram Sang]

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


> I'll leave as-is if Ok with you.

Yes, it is OK.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]