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 register
FCH::PM::DECODEEN. The same register is also accessed by the sp5100_tco
driver.[1] Synchronization to the MMIO register 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 register
FCH::PM::DECODEEN during initialization and 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 2
- Results using v5.16 and this pachset applied. Below
shows the devices detected on the busses:
# 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 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
Also tested using sp5100_tco submitted series listed below.[1]
I applied the sp5100_tco v4 series and ran:
cat >> /dev/watchdog
[1] sp5100_tco v4 patchset is in process but not sent yet. Below is v3
upstream review:
Link: https://lore.kernel.org/linux-watchdog/[email protected]/
Changes in v4:
- Changed request_muxed_mem_region() macro to request_mem_region_muxed()
in patch 1. (Andy Shevchenko)
- Removed unnecessary newline where possible in calls to
request_muxed_region() in patch 2. (Terry Bowman)
- Changed piix4_sb800_region_setup() to piix4_sb800_region_request().
Patch 3. (Jean Delvare)
- Reordered piix4_setup_sb800() local variables from longest name length
to shortest name length. Patch 4. (Terry Bowman)
- Changed piix4_sb800_region_request() and piix4_sb800_region_release() by
adding early return() to remove 'else' improving readability. Patch 6.
(Terry Bowman)
- Removed iowrite32(ioread32(...), ...). Unnecessary because MMIO is
already enabled. (Terry Bowman)
- Refactored piix4_sb800_port_sel() to simplify the 'if' statement using
temp variable. Patch 8. (Terry Bowman)
- Added mmio_cfg.use_mmio assignment in piix4_add_adapter(). This is
needed for calls to piix4_sb800_port_sel() after initialization during
normal operation. Patch 9. (Terry Bowman)
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_mem_region_muxed()
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 | 207 ++++++++++++++++++++++++++-------
include/linux/ioport.h | 2 +
2 files changed, 164 insertions(+), 45 deletions(-)
--
2.30.2
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.
Reviewed-by: Andy Shevchenko <[email protected]>
Signed-off-by: Terry Bowman <[email protected]>
---
include/linux/ioport.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/include/linux/ioport.h b/include/linux/ioport.h
index 8359c50f9988..ec5f71f7135b 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_mem_region_muxed(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
Replace number constant with #define to improve readability and
maintainability.
Signed-off-by: Terry Bowman <[email protected]>
---
drivers/i2c/busses/i2c-piix4.c | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index 8c1b31ed0c42..3ff68967034e 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,8 @@ 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 +304,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 +373,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 +387,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 +685,8 @@ 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 +762,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
Move duplicated region request and release code into a function. Move is
in preparation for following MMIO changes.
Signed-off-by: Terry Bowman <[email protected]>
---
drivers/i2c/busses/i2c-piix4.c | 39 +++++++++++++++++++++++-----------
1 file changed, 27 insertions(+), 12 deletions(-)
diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index 3ff68967034e..5a98970ac60a 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -165,6 +165,24 @@ struct i2c_piix4_adapdata {
u8 port; /* Port number, shifted */
};
+static int piix4_sb800_region_request(struct device *dev)
+{
+ 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)
+{
+ 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 +288,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,20 +310,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_request(&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;
@@ -685,9 +700,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_request(&adap->dev);
+ if (retval)
+ return retval;
/* Request the SMBUS semaphore, avoid conflicts with the IMC */
smbslvcnt = inb_p(SMBSLVCNT);
@@ -762,7 +777,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
Move SMBus controller base address detection into function. Refactor
is in preparation for following MMIO changes.
Signed-off-by: Terry Bowman <[email protected]>
---
drivers/i2c/busses/i2c-piix4.c | 71 +++++++++++++++++++++-------------
1 file changed, 45 insertions(+), 26 deletions(-)
diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index 5a98970ac60a..eab06e9e5fdf 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -282,12 +282,52 @@ 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_request(&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 0;
+}
+
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;
+ unsigned short piix4_smba;
int retval;
/* SB800 and later SMBus does not support forcing address */
@@ -310,33 +350,12 @@ static int piix4_setup_sb800(struct pci_dev *PIIX4_dev,
else
smb_en = (aux) ? 0x28 : 0x2c;
- retval = piix4_sb800_region_request(&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
Move port selection code into a separate function. Refactor is in
preparation for following MMIO changes.
Signed-off-by: Terry Bowman <[email protected]>
---
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 eab06e9e5fdf..32a30af5778a 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -699,6 +699,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, val;
+
+ outb_p(piix4_port_sel_sb800, SB800_PIIX4_SMB_IDX);
+ smba_en_lo = inb_p(SB800_PIIX4_SMB_IDX + 1);
+
+ val = (smba_en_lo & ~piix4_port_mask_sb800) | port;
+ if (smba_en_lo != val)
+ outb_p(val, 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).
@@ -715,8 +728,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_request(&adap->dev);
@@ -776,18 +788,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
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 <[email protected]>
---
drivers/i2c/busses/i2c-piix4.c | 61 ++++++++++++++++++++++++++++++----
1 file changed, 55 insertions(+), 6 deletions(-)
diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index 32a30af5778a..7defa0c5f1f9 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,10 +172,40 @@ 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_request(struct device *dev)
+static int piix4_sb800_region_request(struct device *dev,
+ struct sb800_mmio_cfg *mmio_cfg)
{
+ if (mmio_cfg->use_mmio) {
+ struct resource *res;
+ void __iomem *addr;
+
+ res = request_mem_region_muxed(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;
+
+ return 0;
+ }
+
if (!request_muxed_region(SB800_PIIX4_SMB_IDX, SB800_PIIX4_SMB_MAP_SIZE,
"sb800_piix4_smb")) {
dev_err(dev,
@@ -178,8 +217,15 @@ static int piix4_sb800_region_request(struct device *dev)
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)
{
+ if (mmio_cfg->use_mmio) {
+ iounmap(mmio_cfg->addr);
+ release_resource(mmio_cfg->res);
+ return;
+ }
+
release_region(SB800_PIIX4_SMB_IDX, SB800_PIIX4_SMB_MAP_SIZE);
}
@@ -288,11 +334,13 @@ 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_request(&PIIX4_dev->dev);
+ mmio_cfg.use_mmio = 0;
+ retval = piix4_sb800_region_request(&PIIX4_dev->dev, &mmio_cfg);
if (retval)
return retval;
@@ -301,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;
@@ -731,7 +779,7 @@ static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr,
u8 prev_port;
int retval;
- retval = piix4_sb800_region_request(&adap->dev);
+ retval = piix4_sb800_region_request(&adap->dev, &adapdata->mmio_cfg);
if (retval)
return retval;
@@ -802,7 +850,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;
}
@@ -880,6 +928,7 @@ static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
return -ENOMEM;
}
+ adapdata->mmio_cfg.use_mmio = 0;
adapdata->smba = smba;
adapdata->sb800_main = sb800_main;
adapdata->port = port << piix4_port_shift_sb800;
--
2.30.2
The EFCH SMBus controller's base address is determined using details in
FCH::PM::DECODEEN[smbusasfiobase] and FCH::PM::DECODEEN[smbusasfioen].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 <[email protected]>
---
drivers/i2c/busses/i2c-piix4.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index 7defa0c5f1f9..a23c0327e1f6 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -344,10 +344,15 @@ 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) {
+ 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
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 <[email protected]>
---
drivers/i2c/busses/i2c-piix4.c | 16 +++++++++++++---
1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index a23c0327e1f6..c5325cadaf55 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -752,10 +752,19 @@ 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, val;
+ if (mmio_cfg->use_mmio) {
+ smba_en_lo = ioread8(mmio_cfg->addr + piix4_port_sel_sb800);
+ val = (smba_en_lo & ~piix4_port_mask_sb800) | port;
+ if (smba_en_lo != val)
+ iowrite8(val, mmio_cfg->addr + piix4_port_sel_sb800);
+
+ return (smba_en_lo & piix4_port_mask_sb800);
+ }
+
outb_p(piix4_port_sel_sb800, SB800_PIIX4_SMB_IDX);
smba_en_lo = inb_p(SB800_PIIX4_SMB_IDX + 1);
@@ -765,6 +774,7 @@ static int piix4_sb800_port_sel(u8 port)
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).
@@ -841,12 +851,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
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 <[email protected]>
---
drivers/i2c/busses/i2c-piix4.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index c5325cadaf55..6a9495d994bc 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -101,6 +101,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,
release_region(SB800_PIIX4_SMB_IDX, SB800_PIIX4_SMB_MAP_SIZE);
}
+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_request(&PIIX4_dev->dev, &mmio_cfg);
if (retval)
return retval;
@@ -943,7 +952,7 @@ static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
return -ENOMEM;
}
- adapdata->mmio_cfg.use_mmio = 0;
+ adapdata->mmio_cfg.use_mmio = piix4_sb800_use_mmio(dev);
adapdata->smba = smba;
adapdata->sb800_main = sb800_main;
adapdata->port = port << piix4_port_shift_sb800;
--
2.30.2
On Sun, Jan 30, 2022 at 8:41 PM Terry Bowman <[email protected]> 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 register
> FCH::PM::DECODEEN. The same register is also accessed by the sp5100_tco
> driver.[1] Synchronization to the MMIO register 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 register
> FCH::PM::DECODEEN during initialization and 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.
I'm not against the series, but I am still concerned that we are using
_p IO without clear understanding why. With cleaning them up, this can
be simpler and cleaner.
For the i2c patches:
Reviewed-by: Andy Shevchenko <[email protected]>
if it helps somebody.
> Based on v5.16.
v5.17-rc2 is out :-)
>
> Testing:
> Tested on family 19h using:
> i2cdetect -y 0
> i2cdetect -y 2
>
> - Results using v5.16 and this pachset applied. Below
> shows the devices detected on the busses:
>
> # 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 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
>
> Also tested using sp5100_tco submitted series listed below.[1]
> I applied the sp5100_tco v4 series and ran:
> cat >> /dev/watchdog
>
> [1] sp5100_tco v4 patchset is in process but not sent yet. Below is v3
> upstream review:
> Link: https://lore.kernel.org/linux-watchdog/[email protected]/
>
> Changes in v4:
> - Changed request_muxed_mem_region() macro to request_mem_region_muxed()
> in patch 1. (Andy Shevchenko)
> - Removed unnecessary newline where possible in calls to
> request_muxed_region() in patch 2. (Terry Bowman)
> - Changed piix4_sb800_region_setup() to piix4_sb800_region_request().
> Patch 3. (Jean Delvare)
> - Reordered piix4_setup_sb800() local variables from longest name length
> to shortest name length. Patch 4. (Terry Bowman)
> - Changed piix4_sb800_region_request() and piix4_sb800_region_release() by
> adding early return() to remove 'else' improving readability. Patch 6.
> (Terry Bowman)
> - Removed iowrite32(ioread32(...), ...). Unnecessary because MMIO is
> already enabled. (Terry Bowman)
> - Refactored piix4_sb800_port_sel() to simplify the 'if' statement using
> temp variable. Patch 8. (Terry Bowman)
> - Added mmio_cfg.use_mmio assignment in piix4_add_adapter(). This is
> needed for calls to piix4_sb800_port_sel() after initialization during
> normal operation. Patch 9. (Terry Bowman)
>
> 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_mem_region_muxed()
> 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 | 207 ++++++++++++++++++++++++++-------
> include/linux/ioport.h | 2 +
> 2 files changed, 164 insertions(+), 45 deletions(-)
>
> --
> 2.30.2
--
With Best Regards,
Andy Shevchenko
Hi Andy,
On 1/31/22 05:01, Andy Shevchenko wrote:
> On Sun, Jan 30, 2022 at 8:41 PM Terry Bowman <[email protected]> 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 register
>> FCH::PM::DECODEEN. The same register is also accessed by the sp5100_tco
>> driver.[1] Synchronization to the MMIO register 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 register
>> FCH::PM::DECODEEN during initialization and 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.
>
> I'm not against the series, but I am still concerned that we are using
> _p IO without clear understanding why. With cleaning them up, this can
> be simpler and cleaner.
>
> For the i2c patches:
> Reviewed-by: Andy Shevchenko <[email protected]>
> if it helps somebody.
>
Thanks for the review Andy! I plan to add the ioport_map changes you recommend
in a future patchset. I will include you as "suggested-by".
Regards,
Terry
>> Based on v5.16.
>
> v5.17-rc2 is out :-)
>
>>
>> Testing:
>> Tested on family 19h using:
>> i2cdetect -y 0
>> i2cdetect -y 2
>>
>> - Results using v5.16 and this pachset applied. Below
>> shows the devices detected on the busses:
>>
>> # 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 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
>>
>> Also tested using sp5100_tco submitted series listed below.[1]
>> I applied the sp5100_tco v4 series and ran:
>> cat >> /dev/watchdog
>>
>> [1] sp5100_tco v4 patchset is in process but not sent yet. Below is v3
>> upstream review:
>> Link: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flinux-watchdog%2F20220118202234.410555-1-terry.bowman%40amd.com%2F&data=04%7C01%7Cterry.bowman%40amd.com%7Ca4a101d574724ba6958808d9e4a94579%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637792237972109034%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=LNxoYDcBRCT4Fih%2F8v1m9Xwbvbq1rqHbFlAcqrT4YDU%3D&reserved=0
>>
>> Changes in v4:
>> - Changed request_muxed_mem_region() macro to request_mem_region_muxed()
>> in patch 1. (Andy Shevchenko)
>> - Removed unnecessary newline where possible in calls to
>> request_muxed_region() in patch 2. (Terry Bowman)
>> - Changed piix4_sb800_region_setup() to piix4_sb800_region_request().
>> Patch 3. (Jean Delvare)
>> - Reordered piix4_setup_sb800() local variables from longest name length
>> to shortest name length. Patch 4. (Terry Bowman)
>> - Changed piix4_sb800_region_request() and piix4_sb800_region_release() by
>> adding early return() to remove 'else' improving readability. Patch 6.
>> (Terry Bowman)
>> - Removed iowrite32(ioread32(...), ...). Unnecessary because MMIO is
>> already enabled. (Terry Bowman)
>> - Refactored piix4_sb800_port_sel() to simplify the 'if' statement using
>> temp variable. Patch 8. (Terry Bowman)
>> - Added mmio_cfg.use_mmio assignment in piix4_add_adapter(). This is
>> needed for calls to piix4_sb800_port_sel() after initialization during
>> normal operation. Patch 9. (Terry Bowman)
>>
>> 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_mem_region_muxed()
>> 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 | 207 ++++++++++++++++++++++++++-------
>> include/linux/ioport.h | 2 +
>> 2 files changed, 164 insertions(+), 45 deletions(-)
>>
>> --
>> 2.30.2
>
>
>
On Sun, Jan 30, 2022 at 12:41:21PM -0600, Terry Bowman 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.
Review from Andy is already great, I'll give Jean a few more days for comments.
Hi Terry,
On Sun, 30 Jan 2022 12:41:30 -0600, Terry Bowman wrote:
> 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 <[email protected]>
> ---
> drivers/i2c/busses/i2c-piix4.c | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
> index c5325cadaf55..6a9495d994bc 100644
> --- a/drivers/i2c/busses/i2c-piix4.c
> +++ b/drivers/i2c/busses/i2c-piix4.c
> @@ -101,6 +101,8 @@
> #define SB800_PIIX4_FCH_PM_ADDR 0xFED80300
> #define SB800_PIIX4_FCH_PM_SIZE 8
>
> +#define AMD_PCI_SMBUS_REVISION_MMIO 0x51
> +
I don't think that was worth a define. You only use the value once, in
a context where the symbolic name doesn't add much value IMHO.
--
Jean Delvare
SUSE L3 Support
Hi Andy,
On 2/8/22 14:10, Andy Shevchenko wrote:
> On Tue, Feb 8, 2022 at 6:33 PM Jean Delvare <[email protected]> wrote:
>> On Sun, 30 Jan 2022 12:41:30 -0600, Terry Bowman wrote:
>
> ...
>
>>> +#define AMD_PCI_SMBUS_REVISION_MMIO 0x51
>>
>> I don't think that was worth a define. You only use the value once, in
>> a context where the symbolic name doesn't add much value IMHO.
>
> I don't remember the code context here, but it would be nice in such a
> case to convert this definition to a comment (if it's not crystal
> clear what this magic number is about).
>
>
diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index 239df17ce02b..218ed8efb83e 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -229,6 +229,13 @@ static void piix4_sb800_region_release(struct device *dev,
release_region(SB800_PIIX4_SMB_IDX, SB800_PIIX4_SMB_MAP_SIZE);
}
+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)
{
----
I added the context above. I'll add a comment in v5 to describe what and
why 0x51 is used.
Regards,
Terry
Hi Terry,
On Sun, 30 Jan 2022 12:41:27 -0600, Terry Bowman wrote:
> 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 <[email protected]>
> ---
> drivers/i2c/busses/i2c-piix4.c | 61 ++++++++++++++++++++++++++++++----
> 1 file changed, 55 insertions(+), 6 deletions(-)
> (...)
> @@ -880,6 +928,7 @@ static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
> return -ENOMEM;
> }
>
> + adapdata->mmio_cfg.use_mmio = 0;
Useless initialization, as the adapdata structure has just been zalloc'd.
> adapdata->smba = smba;
> adapdata->sb800_main = sb800_main;
> adapdata->port = port << piix4_port_shift_sb800;
--
Jean Delvare
SUSE L3 Support
Hi Terry,
On Sun, 30 Jan 2022 12:41:21 -0600, Terry Bowman 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 register
> FCH::PM::DECODEEN. The same register is also accessed by the sp5100_tco
> driver.[1] Synchronization to the MMIO register 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 register
> FCH::PM::DECODEEN during initialization and 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.
Thank you for splitting the changes into small chunks for easier
review. Maybe it was even a bit too much in the end, as most patches
don't serve a purpose on their own. But well, that's still much better
than a monolithic patch.
> Based on v5.16.
>
> Testing:
> Tested on family 19h using:
> i2cdetect -y 0
> i2cdetect -y 2
>
> - Results using v5.16 and this pachset applied. Below
> shows the devices detected on the busses:
>
> # 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 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
Unfortunately I'm not really able to test this series. While I do have
an AMD-based laptop which uses the i2c-piix4 driver, the SMBus has
never been usable on it. The driver creates 3 i2c buses (port 0 at
0b00, port 2 at 0b00 and port 1 at 0b20). The first 2 do not seem to
have any device on them (i2cdetect returns empty). The last one must
have some devices on it, I see address 0x1c answers the ping,
unfortunately as soon as probing reaches address 0x2c, all later pings
return success, regardless of the address. It seems that some I2C
device (probably the one at 0x2c, but I don't know what it is) is
holding SDA low forever, therefore preventing any use of the whole
SMBus port until the next reboot.
> (...)
> Changes in v4:
> (...)
> - Removed iowrite32(ioread32(...), ...). Unnecessary because MMIO is
> already enabled. (Terry Bowman)
I'm curious, how can you be sure of that actually?
> (...)
> Terry Bowman (9):
> kernel/resource: Introduce request_mem_region_muxed()
> 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 | 207 ++++++++++++++++++++++++++-------
> include/linux/ioport.h | 2 +
> 2 files changed, 164 insertions(+), 45 deletions(-)
I'm done with my review, looks good overall, I made a few comments here
and there but no major issue. I'll leave it up to you (and Wolfram) to
either send a new series with (some of) my suggestions addressed or
just go with v4. In both cases you can add:
Reviewed-by: Jean Delvare <[email protected]>
to all patches.
Thank you very much for your work, and sorry for my late review.
--
Jean Delvare
SUSE L3 Support
On Sun, 30 Jan 2022 12:41:26 -0600, Terry Bowman wrote:
> Move port selection code into a separate function. Refactor is in
> preparation for following MMIO changes.
>
> Signed-off-by: Terry Bowman <[email protected]>
> ---
> 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 eab06e9e5fdf..32a30af5778a 100644
> --- a/drivers/i2c/busses/i2c-piix4.c
> +++ b/drivers/i2c/busses/i2c-piix4.c
> @@ -699,6 +699,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, val;
> +
> + outb_p(piix4_port_sel_sb800, SB800_PIIX4_SMB_IDX);
> + smba_en_lo = inb_p(SB800_PIIX4_SMB_IDX + 1);
> +
> + val = (smba_en_lo & ~piix4_port_mask_sb800) | port;
> + if (smba_en_lo != val)
> + outb_p(val, 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).
> @@ -715,8 +728,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_request(&adap->dev);
> @@ -776,18 +788,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);
Hmm, this gets pretty inefficient. You set SB800_PIIX4_SMB_IDX to
piix4_port_sel_sb800 twice and compare the new port with the previous
port twice, even though the result will inevitably be the same. The
original code would just unconditionally restore the original port
value, which was also not always needed, but was cheaper in the end.
I can't really think of a better way though, so, so be it :-(
I wonder why we insist on restoring the port though. When you read
multiple values from the same bus (and I2C device drivers do that a
lot, obviously) you end up switching ports a lot for no good reason. I
looked at the git history and it has been that way since support for
the multiplexed buses was added back in 2015. There's a mutex
protecting that section of code anyway, so I don't think restoring the
port buys us anything we did not already have. The only benefit I can
see is to leave the port setting in its original state on suspend and
shutdown (and we know by experience that it can be important to the
BIOS) but then we should just do it in the suspend and remove
functions, instead of after every transfer.
--
Jean Delvare
SUSE L3 Support
Hi Jean,
On 2/8/22 15:46, Jean Delvare wrote:
> On Tue, 8 Feb 2022 13:36:55 -0600, Terry Bowman wrote:
>> On 2/8/22 11:11, Jean Delvare wrote:
>>> Unfortunately I'm not really able to test this series. While I do have
>>> an AMD-based laptop which uses the i2c-piix4 driver, the SMBus has
>>> never been usable on it. The driver creates 3 i2c buses (port 0 at
>>> 0b00, port 2 at 0b00 and port 1 at 0b20). The first 2 do not seem to
>>> have any device on them (i2cdetect returns empty). The last one must
>>> have some devices on it, I see address 0x1c answers the ping,
>>> unfortunately as soon as probing reaches address 0x2c, all later pings
>>> return success, regardless of the address. It seems that some I2C
>>> device (probably the one at 0x2c, but I don't know what it is) is
>>> holding SDA low forever, therefore preventing any use of the whole
>>> SMBus port until the next reboot.
>>
>> My understanding is the OEM may have different i2c devices because it
>> is mainboard specific.
>
> Yes, the devices on the SMBus could be anything Lenovo decided to use.
> Tomorrow I'll try to scan the SMBus but skipping the problematic
> address, to see if it works around the problem.
>
>>>> (...)
>>>> Changes in v4:
>>>> (...)
>>>> - Removed iowrite32(ioread32(...), ...). Unnecessary because MMIO is
>>>> already enabled. (Terry Bowman)
>>>
>>> I'm curious, how can you be sure of that actually?
>>
>> The removed code was using a MMIO region to write 1 to 'mmioen'. This was
>> using the MMIO region to enable same MMIO region.
>
> Ah ah, I get it now. Nice chicken or the egg situation :-)
>
>> Programmer's manual shows FCH::PM::ISACONTROL[mmioen] has a '1' reset value.
>> Per programmer's manual, FCH::PM::ISACONTROL[mmioen] enables MMIO mapping
>> at FED8_0000h..FED8_1FFFh. The FCH::PM::ISACONTROL register is MMIO
>> mapped at FED8_0304h. 'mmioen' was intended to be set using port I/O to
>> enable MMIO but, port I/O access to these registers is now disabled.
>
> Is my understanding correct that there is some overlapping of the
> access methods and there are certain chipsets where both legacy I/O and
> MMIO access is possible?
>
Yes.
> If so, while there's indeed nothing to be done for the most recent
> systems where only MMIO access is possible, you may still need to
> enable MMIO access through legacy I/O if you try to use MMIO on
> chipsets where both are possible. I'm not sure what exactly where you
> set the limit. In the last patch you say that 0x51 is the first
> revision of the family 17h CPUs, but is family 17h the first where MMIO
> is available, or the first where legacy I/O isn't?
>
Family 17h, SMBus PCI ID >= 0x51 is the first where cd6h/cd7h port I/O is disabled.
If SMBus PCI ID < 0x51 then cd6h/cd7h port I/O is used.
Regards,
Terry
On Sun, 30 Jan 2022 12:41:25 -0600, Terry Bowman wrote:
> 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;
> + unsigned short piix4_smba;
> int retval;
>
> /* SB800 and later SMBus does not support forcing address */
I suggest not moving variables around unless there's a compelling to do
that. Else you make your patches larger (and slightly harder to
review), plus it increases the risks of context conflicts when
backporting other changes.
Thanks,
--
Jean Delvare
SUSE L3 Support
Hi Jean,
On 2/8/22 08:45, Jean Delvare wrote:
> Hi Terry,
>
> On Sun, 30 Jan 2022 12:41:24 -0600, Terry Bowman wrote:
>> Move duplicated region request and release code into a function. Move is
>> in preparation for following MMIO changes.
>>
>> Signed-off-by: Terry Bowman <[email protected]>
>> ---
>> drivers/i2c/busses/i2c-piix4.c | 39 +++++++++++++++++++++++-----------
>> 1 file changed, 27 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
>> index 3ff68967034e..5a98970ac60a 100644
>> --- a/drivers/i2c/busses/i2c-piix4.c
>> +++ b/drivers/i2c/busses/i2c-piix4.c
>> @@ -165,6 +165,24 @@ struct i2c_piix4_adapdata {
>> u8 port; /* Port number, shifted */
>> };
>>
>> +static int piix4_sb800_region_request(struct device *dev)
>> +{
>> + 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)
>> +{
>> + 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 +288,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,20 +310,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_request(&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;
>> @@ -685,9 +700,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_request(&adap->dev);
>> + if (retval)
>> + return retval;
>>
>> /* Request the SMBUS semaphore, avoid conflicts with the IMC */
>> smbslvcnt = inb_p(SMBSLVCNT);
>> @@ -762,7 +777,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;
>> }
>>
>
> There's a third occurrence of request_muxed_region(SB800_PIIX4_SMB_IDX,
> ...) / release_region(SB800_PIIX4_SMB_IDX, ...) in function
> piix4_setup_sb800. Any reason why you don't make use of the new helper
> functions there as well?
>
I didn't update the other occurrence because it was outside the codepath
for the device we are addressing. At the time I wanted to minimize changes
particularly for other devices.
> OK, I see that this part of the code is specific to the original (ATI)
> SB800, so it can't use MMIO, therefore you don't *have* to call the
> helper functions. But for consistency, wouldn't it still make sense to
> use them?
>
Yes, it would be more consistent if it used the helper function.
Regards,
Terry
Hi Terry,
On Sun, 30 Jan 2022 12:41:24 -0600, Terry Bowman wrote:
> Move duplicated region request and release code into a function. Move is
> in preparation for following MMIO changes.
>
> Signed-off-by: Terry Bowman <[email protected]>
> ---
> drivers/i2c/busses/i2c-piix4.c | 39 +++++++++++++++++++++++-----------
> 1 file changed, 27 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
> index 3ff68967034e..5a98970ac60a 100644
> --- a/drivers/i2c/busses/i2c-piix4.c
> +++ b/drivers/i2c/busses/i2c-piix4.c
> @@ -165,6 +165,24 @@ struct i2c_piix4_adapdata {
> u8 port; /* Port number, shifted */
> };
>
> +static int piix4_sb800_region_request(struct device *dev)
> +{
> + 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)
> +{
> + 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 +288,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,20 +310,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_request(&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;
> @@ -685,9 +700,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_request(&adap->dev);
> + if (retval)
> + return retval;
>
> /* Request the SMBUS semaphore, avoid conflicts with the IMC */
> smbslvcnt = inb_p(SMBSLVCNT);
> @@ -762,7 +777,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;
> }
>
There's a third occurrence of request_muxed_region(SB800_PIIX4_SMB_IDX,
...) / release_region(SB800_PIIX4_SMB_IDX, ...) in function
piix4_setup_sb800. Any reason why you don't make use of the new helper
functions there as well?
OK, I see that this part of the code is specific to the original (ATI)
SB800, so it can't use MMIO, therefore you don't *have* to call the
helper functions. But for consistency, wouldn't it still make sense to
use them?
--
Jean Delvare
SUSE L3 Support
On Tue, Feb 8, 2022 at 6:33 PM Jean Delvare <[email protected]> wrote:
> On Sun, 30 Jan 2022 12:41:30 -0600, Terry Bowman wrote:
...
> > +#define AMD_PCI_SMBUS_REVISION_MMIO 0x51
>
> I don't think that was worth a define. You only use the value once, in
> a context where the symbolic name doesn't add much value IMHO.
I don't remember the code context here, but it would be nice in such a
case to convert this definition to a comment (if it's not crystal
clear what this magic number is about).
--
With Best Regards,
Andy Shevchenko
On Tue, 8 Feb 2022 13:36:55 -0600, Terry Bowman wrote:
> On 2/8/22 11:11, Jean Delvare wrote:
> > Unfortunately I'm not really able to test this series. While I do have
> > an AMD-based laptop which uses the i2c-piix4 driver, the SMBus has
> > never been usable on it. The driver creates 3 i2c buses (port 0 at
> > 0b00, port 2 at 0b00 and port 1 at 0b20). The first 2 do not seem to
> > have any device on them (i2cdetect returns empty). The last one must
> > have some devices on it, I see address 0x1c answers the ping,
> > unfortunately as soon as probing reaches address 0x2c, all later pings
> > return success, regardless of the address. It seems that some I2C
> > device (probably the one at 0x2c, but I don't know what it is) is
> > holding SDA low forever, therefore preventing any use of the whole
> > SMBus port until the next reboot.
>
> My understanding is the OEM may have different i2c devices because it
> is mainboard specific.
Yes, the devices on the SMBus could be anything Lenovo decided to use.
Tomorrow I'll try to scan the SMBus but skipping the problematic
address, to see if it works around the problem.
> >> (...)
> >> Changes in v4:
> >> (...)
> >> - Removed iowrite32(ioread32(...), ...). Unnecessary because MMIO is
> >> already enabled. (Terry Bowman)
> >
> > I'm curious, how can you be sure of that actually?
>
> The removed code was using a MMIO region to write 1 to 'mmioen'. This was
> using the MMIO region to enable same MMIO region.
Ah ah, I get it now. Nice chicken or the egg situation :-)
> Programmer's manual shows FCH::PM::ISACONTROL[mmioen] has a '1' reset value.
> Per programmer's manual, FCH::PM::ISACONTROL[mmioen] enables MMIO mapping
> at FED8_0000h..FED8_1FFFh. The FCH::PM::ISACONTROL register is MMIO
> mapped at FED8_0304h. 'mmioen' was intended to be set using port I/O to
> enable MMIO but, port I/O access to these registers is now disabled.
Is my understanding correct that there is some overlapping of the
access methods and there are certain chipsets where both legacy I/O and
MMIO access is possible?
If so, while there's indeed nothing to be done for the most recent
systems where only MMIO access is possible, you may still need to
enable MMIO access through legacy I/O if you try to use MMIO on
chipsets where both are possible. I'm not sure what exactly where you
set the limit. In the last patch you say that 0x51 is the first
revision of the family 17h CPUs, but is family 17h the first where MMIO
is available, or the first where legacy I/O isn't?
--
Jean Delvare
SUSE L3 Support
Hi Terry,
On Sun, 30 Jan 2022 12:41:29 -0600, Terry Bowman 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.
>
> Signed-off-by: Terry Bowman <[email protected]>
> ---
> drivers/i2c/busses/i2c-piix4.c | 16 +++++++++++++---
> 1 file changed, 13 insertions(+), 3 deletions(-)
>
> (...)
> @@ -765,6 +774,7 @@ static int piix4_sb800_port_sel(u8 port)
>
> 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).
We indeed want a blank line here, but it should be inserted in patch
[5/9] (which adds function piix4_sb800_port_sel), not in this patch.
--
Jean Delvare
SUSE L3 Support
Hi Jean,
On 2/8/22 11:11, Jean Delvare wrote:
> Hi Terry,
>
> On Sun, 30 Jan 2022 12:41:21 -0600, Terry Bowman 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 register
>> FCH::PM::DECODEEN. The same register is also accessed by the sp5100_tco
>> driver.[1] Synchronization to the MMIO register 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 register
>> FCH::PM::DECODEEN during initialization and 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.
>
> Thank you for splitting the changes into small chunks for easier
> review. Maybe it was even a bit too much in the end, as most patches
> don't serve a purpose on their own. But well, that's still much better
> than a monolithic patch.
>
>> Based on v5.16.
>>
>> Testing:
>> Tested on family 19h using:
>> i2cdetect -y 0
>> i2cdetect -y 2
>>
>> - Results using v5.16 and this pachset applied. Below
>> shows the devices detected on the busses:
>>
>> # 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 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
>
> Unfortunately I'm not really able to test this series. While I do have
> an AMD-based laptop which uses the i2c-piix4 driver, the SMBus has
> never been usable on it. The driver creates 3 i2c buses (port 0 at
> 0b00, port 2 at 0b00 and port 1 at 0b20). The first 2 do not seem to
> have any device on them (i2cdetect returns empty). The last one must
> have some devices on it, I see address 0x1c answers the ping,
> unfortunately as soon as probing reaches address 0x2c, all later pings
> return success, regardless of the address. It seems that some I2C
> device (probably the one at 0x2c, but I don't know what it is) is
> holding SDA low forever, therefore preventing any use of the whole
> SMBus port until the next reboot.
>
My understanding is the OEM may have different i2c devices because it
is mainboard specific.
>> (...)
>> Changes in v4:
>> (...)
>> - Removed iowrite32(ioread32(...), ...). Unnecessary because MMIO is
>> already enabled. (Terry Bowman)
>
> I'm curious, how can you be sure of that actually?
>
The removed code was using a MMIO region to write 1 to 'mmioen'. This was
using the MMIO region to enable same MMIO region.
Programmer's manual shows FCH::PM::ISACONTROL[mmioen] has a '1' reset value.
Per programmer's manual, FCH::PM::ISACONTROL[mmioen] enables MMIO mapping
at FED8_0000h..FED8_1FFFh. The FCH::PM::ISACONTROL register is MMIO
mapped at FED8_0304h. 'mmioen' was intended to be set using port I/O to
enable MMIO but, port I/O access to these registers is now disabled.
>> (...)
>> Terry Bowman (9):
>> kernel/resource: Introduce request_mem_region_muxed()
>> 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 | 207 ++++++++++++++++++++++++++-------
>> include/linux/ioport.h | 2 +
>> 2 files changed, 164 insertions(+), 45 deletions(-)
>
> I'm done with my review, looks good overall, I made a few comments here
> and there but no major issue. I'll leave it up to you (and Wolfram) to
> either send a new series with (some of) my suggestions addressed or
> just go with v4. In both cases you can add:
>
I'll add your requested fixes into v5 and will send for review this afternoon.
> Reviewed-by: Jean Delvare <[email protected]>
>
> to all patches.
>
> Thank you very much for your work, and sorry for my late review.
>
Thanks for the review.
Regards,
Terry
On Tue, 8 Feb 2022 17:03:09 -0600, Terry Bowman wrote:
> On 2/8/22 15:46, Jean Delvare wrote:
> > If so, while there's indeed nothing to be done for the most recent
> > systems where only MMIO access is possible, you may still need to
> > enable MMIO access through legacy I/O if you try to use MMIO on
> > chipsets where both are possible. I'm not sure what exactly where you
> > set the limit. In the last patch you say that 0x51 is the first
> > revision of the family 17h CPUs, but is family 17h the first where MMIO
> > is available, or the first where legacy I/O isn't?
>
> Family 17h, SMBus PCI ID >= 0x51 is the first where cd6h/cd7h port I/O is disabled.
> If SMBus PCI ID < 0x51 then cd6h/cd7h port I/O is used.
OK, we are safe then :-)
--
Jean Delvare
SUSE L3 Support