Message ID | 20210715221828.244536-1-Terry.Bowman@amd.com |
---|---|
State | Changes Requested |
Headers | show |
Series | i2c: piix4: Replace piix4_smbus driver's cd6h/cd7h port io accesses with mmio accesses | expand |
Hi Terry, Sorry for the late review. On Thu, 15 Jul 2021 17:18:28 -0500, Terry Bowman wrote: > cd6h/cd7h port io can be disabled on recent AMD hardware. Read accesses to > disabled cd6h/cd7h will return F's and written data is dropped. The > recommended workaround to handle disabled cd6h/cd7h port io is replacing > with MMIO accesses. Support for MMIO has been available as an alternative > cd6h/cd7h access method since at least smbus controller with PCI revision > 0x59. The piix4_smbus driver uses cd6h/cd7h port io in the following 2 > cases and requires updating to use MMIO: > > 1. The FCH::PM::DECODEEN[smbusasfiobase] and FCH::PM::DECODEEN[0..7] > register fields are used to discover the smbus port io address. > 2. During access requests the piix4_smbus driver enables the requested port > if it is not already enabled. The downstream port is enabled through the > FCH::PM::DECODEEN[smbus0sel] register. > > Signed-off-by: Terry Bowman <Terry.Bowman@amd.com> > Cc: Jean Delvare <jdelvare@suse.com> > Cc: Thomas Lendacky <Thomas.Lendacky@amd.com> > Cc: linux-i2c@vger.kernel.org > --- > diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c > index 8c1b31ed0c42..2d2105793944 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 Now that this is defined, it should be used consistently in the whole driver. As a general rule, do not hesitate to split your changes into smaller steps whenever possible. Small changes are easier to review. The introduction of SB800_PIIX4_SMB_MAP_SIZE is independent from the rest of your changes, so it could go into a separate patch. > > #define KERNCZ_IMC_IDX 0x3e > #define KERNCZ_IMC_DATA 0x3f > @@ -97,6 +98,12 @@ > #define SB800_PIIX4_PORT_IDX_MASK_KERNCZ 0x18 > #define SB800_PIIX4_PORT_IDX_SHIFT_KERNCZ 3 > > +#define SB800_PIIX4_FCH_PM_DECODEEN_MMIO_EN BIT(1) That's many "EN", seems redundant, can it be simplified? > +#define SB800_PIIX4_FCH_PM_ADDR 0xFED80300 Isn't this address supposed to be changeable? As I read the datasheet, you should get the value from PM register 24h? > +#define SB800_PIIX4_FCH_PM_SIZE 8 > + > +#define AMD_PCI_SMBUS_REVISION_MMIO 0x59 > + > /* insmod parameters */ > > /* If force is set to anything different from 0, we forcibly enable the > @@ -155,6 +162,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; > > @@ -162,8 +175,72 @@ 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, > + struct sb800_mmio_cfg *mmio_cfg) For symmetry, this function should be named piix4_sb800_region_request. > +{ > + if (mmio_cfg->use_mmio) { > + struct resource *res; > + void __iomem *addr; > + > + res = request_mem_region(SB800_PIIX4_FCH_PM_ADDR, > + SB800_PIIX4_FCH_PM_SIZE, > + "sb800_piix4_smb"); Is there any other driver which will be accessing this memory area? The old code path is using request_muxed_region(), so there must be. The git history shows that things were done that way by Guenter (Cc'd) in commit 04b6fcaba346e1ce76321ba9b0fd549da4c37ac2, to avoid a conflict with the sp5100_tco driver. So I suspect that this driver will need the same changes you are submitting to the i2c-piix4 driver now. Then the question is, what happens if both drivers request the mem region at the same time? Will the second be happy or will it fail with -EBUSY? Honestly I'm not sure. More on this at the end. > + if (!res) { > + dev_err(dev, > + "SMB base address memory region 0x%x already in use.\n", > + SB800_PIIX4_FCH_PM_ADDR); It is a good opportunity to use "SMBus" instead of "SMB" in these messages, as "SMB" is ambiguous. > + return -EBUSY; > + } > + > + addr = ioremap(SB800_PIIX4_FCH_PM_ADDR, > + SB800_PIIX4_FCH_PM_SIZE); > + if (!addr) { > + release_resource(res); > + dev_err(dev, "SMB 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, > + "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, > + struct sb800_mmio_cfg *mmio_cfg) > +{ > + if (mmio_cfg->use_mmio) { > + iounmap(mmio_cfg->addr); > + mmio_cfg->addr = NULL; I see no need to set it to NULL, as the code is never going to check it. > + > + release_resource(mmio_cfg->res); > + mmio_cfg->res = NULL; Ditto. > + } else { > + 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) > { > @@ -263,12 +340,58 @@ 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) > +{ > + struct sb800_mmio_cfg mmio_cfg; > + u8 smba_en_lo; > + u8 smba_en_hi; Could be declared on the same line. > + int retval; > + > + mmio_cfg.use_mmio = piix4_sb800_use_mmio(PIIX4_dev); > + > + retval = piix4_sb800_region_setup(&PIIX4_dev->dev, &mmio_cfg); > + if (retval) > + return retval; > + > + if (mmio_cfg.use_mmio) { > + iowrite32(ioread32(mmio_cfg.addr + 4) | SB800_PIIX4_FCH_PM_DECODEEN_MMIO_EN, > + 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); > + > + 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; > + } > + > + return retval; Value of retval is always 0 here, so you should hard-code it for clarity. > +} > + > 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; > > /* SB800 and later SMBus does not support forcing address */ > if (force || force_addr) { > @@ -290,29 +413,10 @@ 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")) { > - dev_err(&PIIX4_dev->dev, > - "SMB base address index region 0x%x already in use.\n", > - SB800_PIIX4_SMB_IDX); > - return -EBUSY; > - } > - > - 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, 2); > - > - 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; > - } > + retval = piix4_setup_sb800_smba(PIIX4_dev, smb_en, > + aux, &smb_en_status, &piix4_smba); > + if (retval) > + return retval; > > if (!smb_en_status) { > dev_err(&PIIX4_dev->dev, > @@ -662,6 +766,28 @@ static void piix4_imc_wakeup(void) > release_region(KERNCZ_IMC_IDX, 2); > } > > +static int piix4_sb800_port_sel(u8 port, struct sb800_mmio_cfg *mmio_cfg) > +{ > + u8 smba_en_lo; > + > + 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); > + } > + > + 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). > @@ -678,12 +804,12 @@ 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; > > - if (!request_muxed_region(SB800_PIIX4_SMB_IDX, 2, "sb800_piix4_smb")) > - return -EBUSY; > + retval = piix4_sb800_region_setup(&adap->dev, &adapdata->mmio_cfg); > + if (retval) > + return retval; > > /* Request the SMBUS semaphore, avoid conflicts with the IMC */ > smbslvcnt = inb_p(SMBSLVCNT); > @@ -738,18 +864,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, &adapdata->mmio_cfg); > > 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, &adapdata->mmio_cfg); While functionally correct, this change has a pretty high cost, as you turn a single I/O operation into a function call + 2 tests + 2 or 3 I/O operations. I'm not even sure why we restore the original port at this point. Other drivers (e.g. i2c-i801) only restore the original settings on suspend and shutdown. If something else (ACPI code, BIOS through SMI code) was to access the SMBus device at runtime, we would be in trouble anyway, as there is no synchronization in place. > > /* Release the semaphore */ > outb_p(smbslvcnt | 0x20, SMBSLVCNT); > @@ -758,7 +878,7 @@ static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr, > piix4_imc_wakeup(); > > release: > - release_region(SB800_PIIX4_SMB_IDX, 2); > + piix4_sb800_region_release(&adap->dev, &adapdata->mmio_cfg); > return retval; > } > > @@ -840,6 +960,7 @@ static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba, > adapdata->sb800_main = sb800_main; > adapdata->port = port << piix4_port_shift_sb800; > adapdata->notify_imc = notify_imc; > + adapdata->mmio_cfg.use_mmio = piix4_sb800_use_mmio(dev); > > /* set up the sysfs linkage to our parent device */ > adap->dev.parent = &dev->dev; More generally, I am worried about the overall design. The driver originally used per-access I/O port requesting because keeping the I/O ports busy all the time would prevent other drivers from working. Do we still need to do the same with the new code? If it is possible and safe to have a permanent mapping to the memory ports, that would be a lot faster. On the other hand, the read-modify-write cycle in piix4_setup_sb800_smba() is unsafe if 2 drivers can actually call request_mem_region() on the same memory area successfully. I'm not opposed to taking your patch with minimal changes (as long as the code is safe) and working on performance improvements later.
On Tue, 7 Sep 2021 18:37:20 +0200, Jean Delvare wrote: > More generally, I am worried about the overall design. The driver > originally used per-access I/O port requesting because keeping the I/O > ports busy all the time would prevent other drivers from working. Do we > still need to do the same with the new code? If it is possible and safe > to have a permanent mapping to the memory ports, that would be a lot > faster. > > On the other hand, the read-modify-write cycle in > piix4_setup_sb800_smba() is unsafe if 2 drivers can actually call > request_mem_region() on the same memory area successfully. > > I'm not opposed to taking your patch with minimal changes (as long as > the code is safe) and working on performance improvements later. I looked some more at the code. I was thinking that maybe if the registers accessed by the two drivers (i2c-piix4 and sp5100_tco) were disjoint, then each driver could simply request subsets of the mapped memory. Unfortunately, while most registers are indeed exclusively used by one of the drivers, there's one register (0x00 = IsaDecode) which is used by both. So this simple approach isn't possible. That being said, the register in question is only accessed at device initialization time (on the sp5100_tco side, that's in function sp5100_tco_setupdevice) and only for some devices (Embedded FCH). So one approach which may work is to let the i2c-piix4 driver instantiate the watchdog platform device in that case, instead of having sp5100_tco instantiate its own device as is currently the case. That way, the i2c-piix4 driver would request the "shared" memory area, perform the initialization steps for both functions (SMBus and watchdog) and then instantiate the watchdog device so that sp5100_tco gets loaded and goes on with the runtime management of the watchdog device. If I'm not mistaken, this is what the i2c-i801 driver is already doing for the watchdog device in all recent Intel chipsets. So maybe the same approach can work for the i2c-piix4 driver for the AMD chipsets. However I must confess that I did not try to do it nor am I familiar with the sp5100_tco driver details, so maybe it's not possible for some reason. If it's not possible then the only safe approach would be to migrate i2c-piix4 and sp5100_tco to a true MFD setup with 3 separate drivers: one new MFD PCI driver binding to the PCI device, providing access to the registers with proper locking, and instantiating the platform device, one driver for SMBus (basically i2c-piix4 converted to a platform driver and relying on the MFD driver for register access) and one driver for the watchdog (basically sp5100_tco converted to a platform driver and relying on the MFD driver for register access). That's a much larger change though, so I suppose we'd try avoid it if at all possible.
Hi Jean and Guenter, Jean, Thanks for your responses. I added comments below. I added Guenter to this email because his input is needed for adding the same changes to the sp5100_tco driver. The sp5100_tco and piix4_smbus driver must use the same synchronization logic for the shared register. On 11/5/21 11:05, Jean Delvare wrote: > On Tue, 7 Sep 2021 18:37:20 +0200, Jean Delvare wrote: >> More generally, I am worried about the overall design. The driver >> originally used per-access I/O port requesting because keeping the I/O >> ports busy all the time would prevent other drivers from working. Do we >> still need to do the same with the new code? If it is possible and safe >> to have a permanent mapping to the memory ports, that would be a lot >> faster. >> Permanent mapping would likely improve performance but will not provide the needed synchronization. As you mentioned below the sp5100 driver only uses the DECODEEN register during initialization but the access must be synchronized or an i2c transaction or sp5100_tco timer enable access may be lost. I considered alternatives but most lead to driver coupling or considerable complexity. >> On the other hand, the read-modify-write cycle in >> piix4_setup_sb800_smba() is unsafe if 2 drivers can actually call >> request_mem_region() on the same memory area successfully. >> >> I'm not opposed to taking your patch with minimal changes (as long as >> the code is safe) and working on performance improvements later. > I confirmed through testing the request_mem_region() and request_muxed_region() macros provide exclusive locking. One difference between the 2 macros is the flag parameter, IORESOURCE_MUXED. request_muxed_region() uses the IORESOURCE_MUXED flag to retry the region lock if it's already locked. request_mem_region() does not use the IORESOURCE_MUXED and as a result will return -EBUSY immediately if the region is already locked. I must clarify: the piix4_smbus v1 patch uses request_mem_region() which is not correct because it doesn't retry locking an already locked region. The driver must support retrying the lock or piix4_smbus and sp5100_tco drivers may potentially fail loading. I added proposed piix4_smbus v2 changes below to solve. I propose reusing the existing request_*() framework from include/linux/ioport.h and kernel/resource.c. A new helper macro will be required to provide an interface to the "muxed" iomem locking functionality already present in kernel/resource.c. The new macro will be similar to request_muxed_region() but will instead operate on iomem. This should provide the same performance while using the existing framework. My plan is to add the following to include/linux/ioport.h in v2. This macro will add the interface for using "muxed" iomem support: #define request_mem_muxed_region(start,n,name) __request_region(&iomem_resource, (start), (n), (name), IORESOURCE_MUXED) The proposed changes will need review from more than one subsystem maintainer. The macro addition in include/linux/ioport.h would reside in a different maintainer's tree than this driver. The change to use the request_mem_muxed_region() macro will also be made to the sp5100_tco driver. The v2 review will include maintainers from subsystems owning piix4_smbus driver, sp5100_tco driver, and include/linux/ioport.h. The details provided above are described in a piix4_smbus context but would also be applied to the sp5100_tco driver for synchronizing the shared register. Jean and Guenter, do you have concerns or changes you prefer to the proposal I described above? > I looked some more at the code. I was thinking that maybe if the > registers accessed by the two drivers (i2c-piix4 and sp5100_tco) were > disjoint, then each driver could simply request subsets of the mapped > memory. > > Unfortunately, while most registers are indeed exclusively used by one > of the drivers, there's one register (0x00 = IsaDecode) which is used > by both. So this simple approach isn't possible. > > That being said, the register in question is only accessed at device > initialization time (on the sp5100_tco side, that's in function > sp5100_tco_setupdevice) and only for some devices (Embedded FCH). So > one approach which may work is to let the i2c-piix4 driver instantiate > the watchdog platform device in that case, instead of having sp5100_tco > instantiate its own device as is currently the case. That way, the > i2c-piix4 driver would request the "shared" memory area, perform the > initialization steps for both functions (SMBus and watchdog) and then > instantiate the watchdog device so that sp5100_tco gets loaded and goes > on with the runtime management of the watchdog device. > > If I'm not mistaken, this is what the i2c-i801 driver is already doing > for the watchdog device in all recent Intel chipsets. So maybe the same > approach can work for the i2c-piix4 driver for the AMD chipsets. > However I must confess that I did not try to do it nor am I familiar > with the sp5100_tco driver details, so maybe it's not possible for some > reason. > > If it's not possible then the only safe approach would be to migrate > i2c-piix4 and sp5100_tco to a true MFD setup with 3 separate drivers: > one new MFD PCI driver binding to the PCI device, providing access to > the registers with proper locking, and instantiating the platform > device, one driver for SMBus (basically i2c-piix4 converted to a > platform driver and relying on the MFD driver for register access) and > one driver for the watchdog (basically sp5100_tco converted to a > platform driver and relying on the MFD driver for register access). > That's a much larger change though, so I suppose we'd try avoid it if > at all possible. >
Hi Jean and Guenter, This is a gentle reminder to review my previous response when possible. Thanks! Regards, Terry On 12/13/21 11:48 AM, Terry Bowman wrote: > Hi Jean and Guenter, > > Jean, Thanks for your responses. I added comments below. > > I added Guenter to this email because his input is needed for adding the same > changes to the sp5100_tco driver. The sp5100_tco and piix4_smbus driver > must use the same synchronization logic for the shared register. > > On 11/5/21 11:05, Jean Delvare wrote: >> On Tue, 7 Sep 2021 18:37:20 +0200, Jean Delvare wrote: >>> More generally, I am worried about the overall design. The driver >>> originally used per-access I/O port requesting because keeping the I/O >>> ports busy all the time would prevent other drivers from working. Do we >>> still need to do the same with the new code? If it is possible and safe >>> to have a permanent mapping to the memory ports, that would be a lot >>> faster. >>> > > Permanent mapping would likely improve performance but will not provide the > needed synchronization. As you mentioned below the sp5100 driver only uses > the DECODEEN register during initialization but the access must be > synchronized or an i2c transaction or sp5100_tco timer enable access may be > lost. I considered alternatives but most lead to driver coupling or considerable > complexity. > >>> On the other hand, the read-modify-write cycle in >>> piix4_setup_sb800_smba() is unsafe if 2 drivers can actually call >>> request_mem_region() on the same memory area successfully. >>> >>> I'm not opposed to taking your patch with minimal changes (as long as >>> the code is safe) and working on performance improvements later. >> > > I confirmed through testing the request_mem_region() and request_muxed_region() > macros provide exclusive locking. One difference between the 2 macros is the > flag parameter, IORESOURCE_MUXED. request_muxed_region() uses the > IORESOURCE_MUXED flag to retry the region lock if it's already locked. > request_mem_region() does not use the IORESOURCE_MUXED and as a result will > return -EBUSY immediately if the region is already locked. > > I must clarify: the piix4_smbus v1 patch uses request_mem_region() which is not > correct because it doesn't retry locking an already locked region. The driver > must support retrying the lock or piix4_smbus and sp5100_tco drivers may > potentially fail loading. I added proposed piix4_smbus v2 changes below to solve. > > I propose reusing the existing request_*() framework from include/linux/ioport.h > and kernel/resource.c. A new helper macro will be required to provide an > interface to the "muxed" iomem locking functionality already present in > kernel/resource.c. The new macro will be similar to request_muxed_region() > but will instead operate on iomem. This should provide the same performance > while using the existing framework. > > My plan is to add the following to include/linux/ioport.h in v2. This macro > will add the interface for using "muxed" iomem support: > #define request_mem_muxed_region(start,n,name) __request_region(&iomem_resource, (start), (n), (name), IORESOURCE_MUXED) > > The proposed changes will need review from more than one subsystem maintainer. > The macro addition in include/linux/ioport.h would reside in a > different maintainer's tree than this driver. The change to use the > request_mem_muxed_region() macro will also be made to the sp5100_tco driver. > The v2 review will include maintainers from subsystems owning piix4_smbus > driver, sp5100_tco driver, and include/linux/ioport.h. > > The details provided above are described in a piix4_smbus context but would also be > applied to the sp5100_tco driver for synchronizing the shared register. > > Jean and Guenter, do you have concerns or changes you prefer to the proposal I > described above? > >> I looked some more at the code. I was thinking that maybe if the >> registers accessed by the two drivers (i2c-piix4 and sp5100_tco) were >> disjoint, then each driver could simply request subsets of the mapped >> memory. >> >> Unfortunately, while most registers are indeed exclusively used by one >> of the drivers, there's one register (0x00 = IsaDecode) which is used >> by both. So this simple approach isn't possible. >> >> That being said, the register in question is only accessed at device >> initialization time (on the sp5100_tco side, that's in function >> sp5100_tco_setupdevice) and only for some devices (Embedded FCH). So >> one approach which may work is to let the i2c-piix4 driver instantiate >> the watchdog platform device in that case, instead of having sp5100_tco >> instantiate its own device as is currently the case. That way, the >> i2c-piix4 driver would request the "shared" memory area, perform the >> initialization steps for both functions (SMBus and watchdog) and then >> instantiate the watchdog device so that sp5100_tco gets loaded and goes >> on with the runtime management of the watchdog device. >> >> If I'm not mistaken, this is what the i2c-i801 driver is already doing >> for the watchdog device in all recent Intel chipsets. So maybe the same >> approach can work for the i2c-piix4 driver for the AMD chipsets. >> However I must confess that I did not try to do it nor am I familiar >> with the sp5100_tco driver details, so maybe it's not possible for some >> reason. >> >> If it's not possible then the only safe approach would be to migrate >> i2c-piix4 and sp5100_tco to a true MFD setup with 3 separate drivers: >> one new MFD PCI driver binding to the PCI device, providing access to >> the registers with proper locking, and instantiating the platform >> device, one driver for SMBus (basically i2c-piix4 converted to a >> platform driver and relying on the MFD driver for register access) and >> one driver for the watchdog (basically sp5100_tco converted to a >> platform driver and relying on the MFD driver for register access). >> That's a much larger change though, so I suppose we'd try avoid it if >> at all possible. >>
Hi Jean and Guenter, > This is a gentle reminder to review my previous response when possible. Thanks! Quite some modern AMD laptops seem to suffer from slow touchpads and this patch is part of the fix [1]. So, if you could comment on Terry's questions, this is highly appreciated! Thanks and all the best, Wolfram [1] https://lore.kernel.org/r/CAPoEpV0ZSidL6aMXvB6LN1uS-3CUHS4ggT8RwFgmkzzCiYJ-XQ@mail.gmail.com > > Regards, > Terry > > On 12/13/21 11:48 AM, Terry Bowman wrote: > > Hi Jean and Guenter, > > > > Jean, Thanks for your responses. I added comments below. > > > > I added Guenter to this email because his input is needed for adding the same > > changes to the sp5100_tco driver. The sp5100_tco and piix4_smbus driver > > must use the same synchronization logic for the shared register. > > > > On 11/5/21 11:05, Jean Delvare wrote: > >> On Tue, 7 Sep 2021 18:37:20 +0200, Jean Delvare wrote: > >>> More generally, I am worried about the overall design. The driver > >>> originally used per-access I/O port requesting because keeping the I/O > >>> ports busy all the time would prevent other drivers from working. Do we > >>> still need to do the same with the new code? If it is possible and safe > >>> to have a permanent mapping to the memory ports, that would be a lot > >>> faster. > >>> > > > > Permanent mapping would likely improve performance but will not provide the > > needed synchronization. As you mentioned below the sp5100 driver only uses > > the DECODEEN register during initialization but the access must be > > synchronized or an i2c transaction or sp5100_tco timer enable access may be > > lost. I considered alternatives but most lead to driver coupling or considerable > > complexity. > > > >>> On the other hand, the read-modify-write cycle in > >>> piix4_setup_sb800_smba() is unsafe if 2 drivers can actually call > >>> request_mem_region() on the same memory area successfully. > >>> > >>> I'm not opposed to taking your patch with minimal changes (as long as > >>> the code is safe) and working on performance improvements later. > >> > > > > I confirmed through testing the request_mem_region() and request_muxed_region() > > macros provide exclusive locking. One difference between the 2 macros is the > > flag parameter, IORESOURCE_MUXED. request_muxed_region() uses the > > IORESOURCE_MUXED flag to retry the region lock if it's already locked. > > request_mem_region() does not use the IORESOURCE_MUXED and as a result will > > return -EBUSY immediately if the region is already locked. > > > > I must clarify: the piix4_smbus v1 patch uses request_mem_region() which is not > > correct because it doesn't retry locking an already locked region. The driver > > must support retrying the lock or piix4_smbus and sp5100_tco drivers may > > potentially fail loading. I added proposed piix4_smbus v2 changes below to solve. > > > > I propose reusing the existing request_*() framework from include/linux/ioport.h > > and kernel/resource.c. A new helper macro will be required to provide an > > interface to the "muxed" iomem locking functionality already present in > > kernel/resource.c. The new macro will be similar to request_muxed_region() > > but will instead operate on iomem. This should provide the same performance > > while using the existing framework. > > > > My plan is to add the following to include/linux/ioport.h in v2. This macro > > will add the interface for using "muxed" iomem support: > > #define request_mem_muxed_region(start,n,name) __request_region(&iomem_resource, (start), (n), (name), IORESOURCE_MUXED) > > > > The proposed changes will need review from more than one subsystem maintainer. > > The macro addition in include/linux/ioport.h would reside in a > > different maintainer's tree than this driver. The change to use the > > request_mem_muxed_region() macro will also be made to the sp5100_tco driver. > > The v2 review will include maintainers from subsystems owning piix4_smbus > > driver, sp5100_tco driver, and include/linux/ioport.h. > > > > The details provided above are described in a piix4_smbus context but would also be > > applied to the sp5100_tco driver for synchronizing the shared register. > > > > Jean and Guenter, do you have concerns or changes you prefer to the proposal I > > described above? > > > >> I looked some more at the code. I was thinking that maybe if the > >> registers accessed by the two drivers (i2c-piix4 and sp5100_tco) were > >> disjoint, then each driver could simply request subsets of the mapped > >> memory. > >> > >> Unfortunately, while most registers are indeed exclusively used by one > >> of the drivers, there's one register (0x00 = IsaDecode) which is used > >> by both. So this simple approach isn't possible. > >> > >> That being said, the register in question is only accessed at device > >> initialization time (on the sp5100_tco side, that's in function > >> sp5100_tco_setupdevice) and only for some devices (Embedded FCH). So > >> one approach which may work is to let the i2c-piix4 driver instantiate > >> the watchdog platform device in that case, instead of having sp5100_tco > >> instantiate its own device as is currently the case. That way, the > >> i2c-piix4 driver would request the "shared" memory area, perform the > >> initialization steps for both functions (SMBus and watchdog) and then > >> instantiate the watchdog device so that sp5100_tco gets loaded and goes > >> on with the runtime management of the watchdog device. > >> > >> If I'm not mistaken, this is what the i2c-i801 driver is already doing > >> for the watchdog device in all recent Intel chipsets. So maybe the same > >> approach can work for the i2c-piix4 driver for the AMD chipsets. > >> However I must confess that I did not try to do it nor am I familiar > >> with the sp5100_tco driver details, so maybe it's not possible for some > >> reason. > >> > >> If it's not possible then the only safe approach would be to migrate > >> i2c-piix4 and sp5100_tco to a true MFD setup with 3 separate drivers: > >> one new MFD PCI driver binding to the PCI device, providing access to > >> the registers with proper locking, and instantiating the platform > >> device, one driver for SMBus (basically i2c-piix4 converted to a > >> platform driver and relying on the MFD driver for register access) and > >> one driver for the watchdog (basically sp5100_tco converted to a > >> platform driver and relying on the MFD driver for register access). > >> That's a much larger change though, so I suppose we'd try avoid it if > >> at all possible. > >>
On 1/4/22 11:34 AM, Terry Bowman wrote: > Hi Jean and Guenter, > > This is a gentle reminder to review my previous response when possible. Thanks! > > Regards, > Terry > > On 12/13/21 11:48 AM, Terry Bowman wrote: >> Hi Jean and Guenter, >> >> Jean, Thanks for your responses. I added comments below. >> >> I added Guenter to this email because his input is needed for adding the same >> changes to the sp5100_tco driver. The sp5100_tco and piix4_smbus driver >> must use the same synchronization logic for the shared register. >> >> On 11/5/21 11:05, Jean Delvare wrote: >>> On Tue, 7 Sep 2021 18:37:20 +0200, Jean Delvare wrote: >>>> More generally, I am worried about the overall design. The driver >>>> originally used per-access I/O port requesting because keeping the I/O >>>> ports busy all the time would prevent other drivers from working. Do we >>>> still need to do the same with the new code? If it is possible and safe >>>> to have a permanent mapping to the memory ports, that would be a lot >>>> faster. >>>> >> >> Permanent mapping would likely improve performance but will not provide the >> needed synchronization. As you mentioned below the sp5100 driver only uses >> the DECODEEN register during initialization but the access must be >> synchronized or an i2c transaction or sp5100_tco timer enable access may be >> lost. I considered alternatives but most lead to driver coupling or considerable >> complexity. >> >>>> On the other hand, the read-modify-write cycle in >>>> piix4_setup_sb800_smba() is unsafe if 2 drivers can actually call >>>> request_mem_region() on the same memory area successfully. >>>> >>>> I'm not opposed to taking your patch with minimal changes (as long as >>>> the code is safe) and working on performance improvements later. >>> >> >> I confirmed through testing the request_mem_region() and request_muxed_region() >> macros provide exclusive locking. One difference between the 2 macros is the >> flag parameter, IORESOURCE_MUXED. request_muxed_region() uses the >> IORESOURCE_MUXED flag to retry the region lock if it's already locked. >> request_mem_region() does not use the IORESOURCE_MUXED and as a result will >> return -EBUSY immediately if the region is already locked. >> >> I must clarify: the piix4_smbus v1 patch uses request_mem_region() which is not >> correct because it doesn't retry locking an already locked region. The driver >> must support retrying the lock or piix4_smbus and sp5100_tco drivers may >> potentially fail loading. I added proposed piix4_smbus v2 changes below to solve. >> >> I propose reusing the existing request_*() framework from include/linux/ioport.h >> and kernel/resource.c. A new helper macro will be required to provide an >> interface to the "muxed" iomem locking functionality already present in >> kernel/resource.c. The new macro will be similar to request_muxed_region() >> but will instead operate on iomem. This should provide the same performance >> while using the existing framework. >> >> My plan is to add the following to include/linux/ioport.h in v2. This macro >> will add the interface for using "muxed" iomem support: >> #define request_mem_muxed_region(start,n,name) __request_region(&iomem_resource, (start), (n), (name), IORESOURCE_MUXED) >> >> The proposed changes will need review from more than one subsystem maintainer. >> The macro addition in include/linux/ioport.h would reside in a >> different maintainer's tree than this driver. The change to use the >> request_mem_muxed_region() macro will also be made to the sp5100_tco driver. >> The v2 review will include maintainers from subsystems owning piix4_smbus >> driver, sp5100_tco driver, and include/linux/ioport.h. >> >> The details provided above are described in a piix4_smbus context but would also be >> applied to the sp5100_tco driver for synchronizing the shared register. >> >> Jean and Guenter, do you have concerns or changes you prefer to the proposal I >> described above? >> I think you'll need approval from someone with authority to accept the suggested change in include/linux/ioport.h. No idea who that would be. Guenter >>> I looked some more at the code. I was thinking that maybe if the >>> registers accessed by the two drivers (i2c-piix4 and sp5100_tco) were >>> disjoint, then each driver could simply request subsets of the mapped >>> memory. >>> >>> Unfortunately, while most registers are indeed exclusively used by one >>> of the drivers, there's one register (0x00 = IsaDecode) which is used >>> by both. So this simple approach isn't possible. >>> >>> That being said, the register in question is only accessed at device >>> initialization time (on the sp5100_tco side, that's in function >>> sp5100_tco_setupdevice) and only for some devices (Embedded FCH). So >>> one approach which may work is to let the i2c-piix4 driver instantiate >>> the watchdog platform device in that case, instead of having sp5100_tco >>> instantiate its own device as is currently the case. That way, the >>> i2c-piix4 driver would request the "shared" memory area, perform the >>> initialization steps for both functions (SMBus and watchdog) and then >>> instantiate the watchdog device so that sp5100_tco gets loaded and goes >>> on with the runtime management of the watchdog device. >>> >>> If I'm not mistaken, this is what the i2c-i801 driver is already doing >>> for the watchdog device in all recent Intel chipsets. So maybe the same >>> approach can work for the i2c-piix4 driver for the AMD chipsets. >>> However I must confess that I did not try to do it nor am I familiar >>> with the sp5100_tco driver details, so maybe it's not possible for some >>> reason. >>> >>> If it's not possible then the only safe approach would be to migrate >>> i2c-piix4 and sp5100_tco to a true MFD setup with 3 separate drivers: >>> one new MFD PCI driver binding to the PCI device, providing access to >>> the registers with proper locking, and instantiating the platform >>> device, one driver for SMBus (basically i2c-piix4 converted to a >>> platform driver and relying on the MFD driver for register access) and >>> one driver for the watchdog (basically sp5100_tco converted to a >>> platform driver and relying on the MFD driver for register access). >>> That's a much larger change though, so I suppose we'd try avoid it if >>> at all possible. >>>
> I think you'll need approval from someone with authority to accept the > suggested change in include/linux/ioport.h. No idea who that would be. ioport.h has no dedicated maintainer. I would modify it via my tree if we have enough review. I'd think Guenter, me, and maybe Andy (Shevchenko), and Rafael should be a good crowd. So, I suggest to do your v2 and add all these people to CC. It is usually easier to talk about existing code.
On Mon, Jan 10, 2022 at 6:25 AM Wolfram Sang <wsa@kernel.org> wrote: > > > I think you'll need approval from someone with authority to accept the > > suggested change in include/linux/ioport.h. No idea who that would be. > > ioport.h has no dedicated maintainer. I would modify it via my tree if > we have enough review. I'd think Guenter, me, and maybe Andy > (Shevchenko), and Rafael should be a good crowd. So, I suggest to do > your v2 and add all these people to CC. It is usually easier to talk > about existing code. I have briefly read the discussion by the link you provided above in this thread. I'm not sure I understand the issue and if Intel hardware is affected. Is there any summary of the problem?
> I have briefly read the discussion by the link you provided above in > this thread. I'm not sure I understand the issue and if Intel hardware > is affected. Is there any summary of the problem? I guess the original patch description should explain it. You can find it here: http://patchwork.ozlabs.org/project/linux-i2c/patch/20210715221828.244536-1-Terry.Bowman@amd.com/ If this is not sufficient, hopefully Terry can provide more information?
+Robert Richter Hi Andy, The cd6h/cd7h port I/O can be disabled on recent AMD processors and these changes replace the cd6h/cd7h port I/O accesses with with MMIO accesses. I can provide more details or answer questions. Regards, Terry On 1/11/22 6:39 AM, Wolfram Sang wrote: > >> I have briefly read the discussion by the link you provided above in >> this thread. I'm not sure I understand the issue and if Intel hardware >> is affected. Is there any summary of the problem? > > I guess the original patch description should explain it. You can find > it here: > > http://patchwork.ozlabs.org/project/linux-i2c/patch/20210715221828.244536-1-Terry.Bowman@amd.com/ > > If this is not sufficient, hopefully Terry can provide more information? >
On Tue, Jan 11, 2022 at 4:13 PM Terry Bowman <Terry.Bowman@amd.com> wrote: > The cd6h/cd7h port I/O can be disabled on recent AMD processors and these > changes replace the cd6h/cd7h port I/O accesses with with MMIO accesses. > I can provide more details or answer questions. AFAIU the issue the list of questions looks like this (correct me, if I'm wrong): - some chips switched from I/O to MMIO - the bus driver has shared resources with another (TCO) driver Now, technically what you are trying is to find a way to keep the original functionality on old machines and support new ones without much trouble. From what I see, the silver bullet may be the switch to regmap as we have done in I2C DesignWare driver implementation. Yes, it's a much more invasive solution, but at the same time it's much cleaner from my p.o.v. And you may easily split it to logical parts (prepare drivers, switch to regmap, add a new functionality). I might be missing something and above not gonna work, please tell me what I miss in that case. > On 1/11/22 6:39 AM, Wolfram Sang wrote: > > > >> I have briefly read the discussion by the link you provided above in > >> this thread. I'm not sure I understand the issue and if Intel hardware > >> is affected. Is there any summary of the problem? > > > > I guess the original patch description should explain it. You can find > > it here: > > > > http://patchwork.ozlabs.org/project/linux-i2c/patch/20210715221828.244536-1-Terry.Bowman@amd.com/ > > > > If this is not sufficient, hopefully Terry can provide more information?
On Tue, Jan 11, 2022 at 4:53 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Tue, Jan 11, 2022 at 4:13 PM Terry Bowman <Terry.Bowman@amd.com> wrote: > > The cd6h/cd7h port I/O can be disabled on recent AMD processors and these > > changes replace the cd6h/cd7h port I/O accesses with with MMIO accesses. > > I can provide more details or answer questions. > > AFAIU the issue the list of questions looks like this (correct me, if > I'm wrong): > - some chips switched from I/O to MMIO > - the bus driver has shared resources with another (TCO) driver > > Now, technically what you are trying is to find a way to keep the > original functionality on old machines and support new ones without > much trouble. > > From what I see, the silver bullet may be the switch to regmap as we > have done in I2C DesignWare driver implementation. > > Yes, it's a much more invasive solution, but at the same time it's > much cleaner from my p.o.v. And you may easily split it to logical > parts (prepare drivers, switch to regmap, add a new functionality). > > I might be missing something and above not gonna work, please tell me > what I miss in that case. On top of that I'm wondering why slow I/O is used? Do we have anything that really needs that or is it simply a cargo-cult? > > On 1/11/22 6:39 AM, Wolfram Sang wrote: > > > > > >> I have briefly read the discussion by the link you provided above in > > >> this thread. I'm not sure I understand the issue and if Intel hardware > > >> is affected. Is there any summary of the problem? > > > > > > I guess the original patch description should explain it. You can find > > > it here: > > > > > > http://patchwork.ozlabs.org/project/linux-i2c/patch/20210715221828.244536-1-Terry.Bowman@amd.com/ > > > > > > If this is not sufficient, hopefully Terry can provide more information?
On 1/11/22 8:54 AM, Andy Shevchenko wrote: > On Tue, Jan 11, 2022 at 4:53 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: >> On Tue, Jan 11, 2022 at 4:13 PM Terry Bowman <Terry.Bowman@amd.com> wrote: >>> The cd6h/cd7h port I/O can be disabled on recent AMD processors and these >>> changes replace the cd6h/cd7h port I/O accesses with with MMIO accesses. >>> I can provide more details or answer questions. >> >> AFAIU the issue the list of questions looks like this (correct me, if >> I'm wrong): >> - some chips switched from I/O to MMIO >> - the bus driver has shared resources with another (TCO) driver >> Correct >> Now, technically what you are trying is to find a way to keep the >> original functionality on old machines and support new ones without >> much trouble. >> >> From what I see, the silver bullet may be the switch to regmap as we >> have done in I2C DesignWare driver implementation. >> >> Yes, it's a much more invasive solution, but at the same time it's >> much cleaner from my p.o.v. And you may easily split it to logical >> parts (prepare drivers, switch to regmap, add a new functionality). >> >> I might be missing something and above not gonna work, please tell me >> what I miss in that case. > > On top of that I'm wondering why slow I/O is used? Do we have anything > that really needs that or is it simply a cargo-cult? The efch SMBUS & WDT previously only supported a port I/O interface (until recently) and thus dictated the HW access method. Wolfram pointed out some AMD laptops suffer from slow trackpad [1] and this is part of the fix. [1] https://lore.kernel.org/r/CAPoEpV0ZSidL6aMXvB6LN1uS-3CUHS4ggT8RwFgmkzzCiYJ-XQ@mail.gmail.com > >>> On 1/11/22 6:39 AM, Wolfram Sang wrote: >>>> >>>>> I have briefly read the discussion by the link you provided above in >>>>> this thread. I'm not sure I understand the issue and if Intel hardware >>>>> is affected. Is there any summary of the problem? >>>> >>>> I guess the original patch description should explain it. You can find >>>> it here: >>>> >>>> https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpatchwork.ozlabs.org%2Fproject%2Flinux-i2c%2Fpatch%2F20210715221828.244536-1-Terry.Bowman%40amd.com%2F&data=04%7C01%7CTerry.Bowman%40amd.com%7C89e551e0ebe94607beaf08d9d51288f9%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637775097863907004%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=gvJ0FC9MVacQunc8uMJ6oJEw0pGcisu9muQkE8u4rxY%3D&reserved=0 >>>> >>>> If this is not sufficient, hopefully Terry can provide more information? >
On Tue, Jan 11, 2022 at 5:50 PM Terry Bowman <Terry.Bowman@amd.com> wrote: > On 1/11/22 8:54 AM, Andy Shevchenko wrote: > > On Tue, Jan 11, 2022 at 4:53 PM Andy Shevchenko > > <andy.shevchenko@gmail.com> wrote: > >> On Tue, Jan 11, 2022 at 4:13 PM Terry Bowman <Terry.Bowman@amd.com> wrote: > >>> The cd6h/cd7h port I/O can be disabled on recent AMD processors and these > >>> changes replace the cd6h/cd7h port I/O accesses with with MMIO accesses. > >>> I can provide more details or answer questions. > >> > >> AFAIU the issue the list of questions looks like this (correct me, if > >> I'm wrong): > >> - some chips switched from I/O to MMIO > >> - the bus driver has shared resources with another (TCO) driver > >> > Correct > > >> Now, technically what you are trying is to find a way to keep the > >> original functionality on old machines and support new ones without > >> much trouble. > >> > >> From what I see, the silver bullet may be the switch to regmap as we > >> have done in I2C DesignWare driver implementation. > >> > >> Yes, it's a much more invasive solution, but at the same time it's > >> much cleaner from my p.o.v. And you may easily split it to logical > >> parts (prepare drivers, switch to regmap, add a new functionality). > >> > >> I might be missing something and above not gonna work, please tell me > >> what I miss in that case. > > On top of that I'm wondering why slow I/O is used? Do we have anything > > that really needs that or is it simply a cargo-cult? > > The efch SMBUS & WDT previously only supported a port I/O interface > (until recently) and thus dictated the HW access method. I believe you didn't get my question. Sorry for that. Elaboration below. The code is using in*_p() and out*_p() accessors (pay attention to the _p part). My question is about that. > Wolfram pointed out some AMD laptops suffer from slow trackpad [1] and > this is part of the fix. > > [1] https://lore.kernel.org/r/CAPoEpV0ZSidL6aMXvB6LN1uS-3CUHS4ggT8RwFgmkzzCiYJ-XQ@mail.gmail.com I see, but still it never worked, correct? > >>> On 1/11/22 6:39 AM, Wolfram Sang wrote: > >>>> > >>>>> I have briefly read the discussion by the link you provided above in > >>>>> this thread. I'm not sure I understand the issue and if Intel hardware > >>>>> is affected. Is there any summary of the problem? > >>>> > >>>> I guess the original patch description should explain it. You can find > >>>> it here: > >>>> > >>>> https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpatchwork.ozlabs.org%2Fproject%2Flinux-i2c%2Fpatch%2F20210715221828.244536-1-Terry.Bowman%40amd.com%2F&data=04%7C01%7CTerry.Bowman%40amd.com%7C89e551e0ebe94607beaf08d9d51288f9%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637775097863907004%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=gvJ0FC9MVacQunc8uMJ6oJEw0pGcisu9muQkE8u4rxY%3D&reserved=0 > >>>> > >>>> If this is not sufficient, hopefully Terry can provide more information? > >
On 1/11/22 10:17, Andy Shevchenko wrote: > On Tue, Jan 11, 2022 at 5:50 PM Terry Bowman <Terry.Bowman@amd.com> wrote: >> On 1/11/22 8:54 AM, Andy Shevchenko wrote: >>> On Tue, Jan 11, 2022 at 4:53 PM Andy Shevchenko >>> <andy.shevchenko@gmail.com> wrote: >>>> On Tue, Jan 11, 2022 at 4:13 PM Terry Bowman <Terry.Bowman@amd.com> wrote: >>>>> The cd6h/cd7h port I/O can be disabled on recent AMD processors and these >>>>> changes replace the cd6h/cd7h port I/O accesses with with MMIO accesses. >>>>> I can provide more details or answer questions. >>>> >>>> AFAIU the issue the list of questions looks like this (correct me, if >>>> I'm wrong): >>>> - some chips switched from I/O to MMIO >>>> - the bus driver has shared resources with another (TCO) driver >>>> >> Correct >> >>>> Now, technically what you are trying is to find a way to keep the >>>> original functionality on old machines and support new ones without >>>> much trouble. >>>> >>>> From what I see, the silver bullet may be the switch to regmap as we >>>> have done in I2C DesignWare driver implementation. >>>> >>>> Yes, it's a much more invasive solution, but at the same time it's >>>> much cleaner from my p.o.v. And you may easily split it to logical >>>> parts (prepare drivers, switch to regmap, add a new functionality). >>>> >>>> I might be missing something and above not gonna work, please tell me >>>> what I miss in that case. > >>> On top of that I'm wondering why slow I/O is used? Do we have anything >>> that really needs that or is it simply a cargo-cult? >> >> The efch SMBUS & WDT previously only supported a port I/O interface >> (until recently) and thus dictated the HW access method. > > I believe you didn't get my question. Sorry for that. Elaboration below. > > The code is using in*_p() and out*_p() accessors (pay attention to the _p part). > > My question is about that. > >> Wolfram pointed out some AMD laptops suffer from slow trackpad [1] and >> this is part of the fix. >> >> [1] https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fr%2FCAPoEpV0ZSidL6aMXvB6LN1uS-3CUHS4ggT8RwFgmkzzCiYJ-XQ%40mail.gmail.com&data=04%7C01%7CTerry.Bowman%40amd.com%7Cded2c3a486854ef44c3408d9d51e0cad%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637775147318596002%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=lKNVA1xFkS5bIxDS4%2BdCXVPKlrIOY9PV%2BW9sLtnR630%3D&reserved=0 > > I see, but still it never worked, correct? > I was not familiar with the trackpad issue until a few days ago. According to Miroslav's post, one of the issues is resolved but their is an interrupt flood still to be resolved. >>>>> On 1/11/22 6:39 AM, Wolfram Sang wrote: >>>>>> >>>>>>> I have briefly read the discussion by the link you provided above in >>>>>>> this thread. I'm not sure I understand the issue and if Intel hardware >>>>>>> is affected. Is there any summary of the problem? >>>>>> >>>>>> I guess the original patch description should explain it. You can find >>>>>> it here: >>>>>> >>>>>> https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpatchwork.ozlabs.org%2Fproject%2Flinux-i2c%2Fpatch%2F20210715221828.244536-1-Terry.Bowman%40amd.com%2F&data=04%7C01%7CTerry.Bowman%40amd.com%7Cded2c3a486854ef44c3408d9d51e0cad%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637775147318596002%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=oNfdc6ozDE57vqwnEH4n2KQfXdxcF9rAiI9R592CKv4%3D&reserved=0 >>>>>> >>>>>> If this is not sufficient, hopefully Terry can provide more information? >>> > > >
> > On top of that I'm wondering why slow I/O is used? Do we have anything > > that really needs that or is it simply a cargo-cult? > > The efch SMBUS & WDT previously only supported a port I/O interface > (until recently) and thus dictated the HW access method. Is this enough information to start v2 of this series? Or does the approach need more discussion?
On Thu, Jan 13, 2022 at 9:42 AM Wolfram Sang <wsa@kernel.org> wrote: > > > > > On top of that I'm wondering why slow I/O is used? Do we have anything > > > that really needs that or is it simply a cargo-cult? > > > > The efch SMBUS & WDT previously only supported a port I/O interface > > (until recently) and thus dictated the HW access method. > > Is this enough information to start v2 of this series? Or does the > approach need more discussion? I dunno why slow I/O is chosen, but it only affects design (read: ugliness) of the new code.
On Thu, 13 Jan 2022 12:24:41 +0200, Andy Shevchenko wrote: > On Thu, Jan 13, 2022 at 9:42 AM Wolfram Sang <wsa@kernel.org> wrote: > > > > On top of that I'm wondering why slow I/O is used? Do we have anything > > > > that really needs that or is it simply a cargo-cult? > > > > > > The efch SMBUS & WDT previously only supported a port I/O interface > > > (until recently) and thus dictated the HW access method. > > > > Is this enough information to start v2 of this series? Or does the > > approach need more discussion? > > I dunno why slow I/O is chosen, but it only affects design (read: > ugliness) of the new code. I've been wondering about the use of slow (*_p) I/O accessors for some time too. All the SMBus controller drivers doing that originate from the lm_sensors project (i2c-ali1535, i2c-ali1563, i2c-ali15x3, i2c-amd756, i2c-i801, i2c-nforce2, i2c-piix4 and i2c-viapro). So basically *all* SMBus controller drivers for non-embedded x86. I suspect that most of this is the result of copy-and-paste from one driver to the next as support for different chipsets was added in the late 90's and early 2000's. I wouldn't be surprised if most, if not all, can be replaced with non-pausing counterparts. But I've been too shy to give it a try so far. I must say I find it pretty funny that Andy is asking about it in the i2c-piix4 driver when the i2c-i801 driver, which he's been helping with quite a lot in the last few years, does exactly the same.
Hi Terry, Thank you for following up, and sorry for the later reply. On Mon, 13 Dec 2021 11:48:18 -0600, Terry Bowman wrote: > I added Guenter to this email because his input is needed for adding the same > changes to the sp5100_tco driver. The sp5100_tco and piix4_smbus driver > must use the same synchronization logic for the shared register. Correct. > On 11/5/21 11:05, Jean Delvare wrote: > > On Tue, 7 Sep 2021 18:37:20 +0200, Jean Delvare wrote: > >> More generally, I am worried about the overall design. The driver > >> originally used per-access I/O port requesting because keeping the I/O > >> ports busy all the time would prevent other drivers from working. Do we > >> still need to do the same with the new code? If it is possible and safe > >> to have a permanent mapping to the memory ports, that would be a lot > >> faster. > > Permanent mapping would likely improve performance but will not provide the > needed synchronization. Depends how it is implemented, see below. > (...) As you mentioned below the sp5100 driver only uses > the DECODEEN register during initialization but the access must be > synchronized or an i2c transaction or sp5100_tco timer enable access may be > lost. I considered alternatives but most lead to driver coupling or considerable > complexity. > > >> On the other hand, the read-modify-write cycle in > >> piix4_setup_sb800_smba() is unsafe if 2 drivers can actually call > >> request_mem_region() on the same memory area successfully. > >> > >> I'm not opposed to taking your patch with minimal changes (as long as > >> the code is safe) and working on performance improvements later. > > > > I confirmed through testing the request_mem_region() and request_muxed_region() > macros provide exclusive locking. One difference between the 2 macros is the > flag parameter, IORESOURCE_MUXED. request_muxed_region() uses the > IORESOURCE_MUXED flag to retry the region lock if it's already locked. > request_mem_region() does not use the IORESOURCE_MUXED and as a result will > return -EBUSY immediately if the region is already locked. > > I must clarify: the piix4_smbus v1 patch uses request_mem_region() which is not > correct because it doesn't retry locking an already locked region. The driver > must support retrying the lock or piix4_smbus and sp5100_tco drivers may > potentially fail loading. I added proposed piix4_smbus v2 changes below to solve. Yes, I mentioned that problem during my initial review (I think). > I propose reusing the existing request_*() framework from include/linux/ioport.h > and kernel/resource.c. A new helper macro will be required to provide an > interface to the "muxed" iomem locking functionality already present in > kernel/resource.c. The new macro will be similar to request_muxed_region() > but will instead operate on iomem. This should provide the same performance > while using the existing framework. > > My plan is to add the following to include/linux/ioport.h in v2. This macro > will add the interface for using "muxed" iomem support: > #define request_mem_muxed_region(start,n,name) __request_region(&iomem_resource, (start), (n), (name), IORESOURCE_MUXED) > > The proposed changes will need review from more than one subsystem maintainer. > The macro addition in include/linux/ioport.h would reside in a > different maintainer's tree than this driver. The change to use the > request_mem_muxed_region() macro will also be made to the sp5100_tco driver. > The v2 review will include maintainers from subsystems owning piix4_smbus > driver, sp5100_tco driver, and include/linux/ioport.h. > > The details provided above are described in a piix4_smbus context but would also be > applied to the sp5100_tco driver for synchronizing the shared register. > > Jean and Guenter, do you have concerns or changes you prefer to the proposal I > described above? To be honest, I have a conceptual disagreement with request_mem_muxed_region(). The reason why request_muxed_region() was implemented in the first place, is that muxed I/O port pairs allow access to different registers "behind" the ports, using what I would call "logical addressing", and while it is legitimate for different drivers to have to access different register ranges "behind" the ports, the I/O helper functions do not allow reserving these, and more generally the I/O requesting mechanism in Linux only deals with physical I/O ports and not logical ranges behind muxed pairs. So request_muxed_region() allows different drivers to use the same I/O ports with mutual exclusion in order to access the logical register ranges. My feeling is, that the expectation is still that the different drivers do not step on each other's toes and each driver only accesses their own registers, even though there is no way to enforce that (and as a matter of fact, we have found that the i2c-piix4 and sp5100_tco drivers do share one register). (Thankfully, the access to this "shared" register is always done with the muxed region owned for the whole duration of the read / modify / write cycles, so I think we are on the same side. But it's really up to the drivers to behave properly, as in theory it would be possible to own the muxed region just for reading, then just for writing, with no guarantee that another driver didn't change the register value in between.) Where I'm getting at is, request_muxed_region() works, but only with certain restrictions and assumptions. It's there because we need it and there's no easy way to implement it differently. On the other hand, there's no reason for request_mem_muxed_region() to exist in the first place, and as a matter of fact, it does not exist yet. You only want it in order to avoid having to redesign 2 drivers that have grown organically for 2 decades and that barely talk to each other even though they access the very same piece of hardware. But in fact there's nothing you would do with request_mem_muxed_region() that can't be done without it. As mentioned before, the i2c-i801 and iTCO_wdt drivers were in a similar situation and their design was changed slightly in order to solve the problem. See commit 9424693035a5 ("i2c: i801: Create iTCO device on newer Intel PCHs"). So clearly my personal preference would be to do the same for the i2c-piix4+sp5100_tco driver pair, for consistency and because we know it is a design that works. And it does allow permanent mapping of the registers, so performance should be better. My only concern is that you'll have to deal with older chipsets (legacy I/O, permanent mapping not possible) and newer chipsets (MMIO access, permanent mapping possible) and this is likely to clutter the code to some degree. Going with request_mem_muxed_region() is only the second choice as far as I'm concerned. I see the advantage (that's clearly the minimal effort to get your problem fixed) but I'm afraid to create an API that will later be abused to circumvent clean driver design. I'm also not sure if you will find someone willing to accept it upstream in the first place. The fact that there is no dedicated maintainer for <linux/ioport.h> is probably not going to help (unless you want to sneak the change in while nobody is paying attention, in which case it might actually help ^^). I see that Andy suggested a regmap approach instead. My only attempt to use regmap in one of my drivers ended up in miserable failure, I got totally lost in the too many abstraction layers. So I just can't say whether this is the right thing to do or even if it can solve the problem at hand. The resource reservation and/or locking issue we have is clearly not trivial, so the regmap infrastructure would have to be highly flexible in order to accommodate that. Feel free to give it a try and see for yourself if it's going to fit the bill. If it works, I won't object, and I'll finally have to do the effort to understand how it works. As a conclusion, everything I wrote above is about my opinions and preferences. At the end of the day, I understand that we need to fix the i2c-piix4 and sp5100_tco drivers, and the sooner the better, so any solution that works and nobody objects against will be OK with me.
diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c index 8c1b31ed0c42..2d2105793944 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 @@ -97,6 +98,12 @@ #define SB800_PIIX4_PORT_IDX_MASK_KERNCZ 0x18 #define SB800_PIIX4_PORT_IDX_SHIFT_KERNCZ 3 +#define SB800_PIIX4_FCH_PM_DECODEEN_MMIO_EN BIT(1) +#define SB800_PIIX4_FCH_PM_ADDR 0xFED80300 +#define SB800_PIIX4_FCH_PM_SIZE 8 + +#define AMD_PCI_SMBUS_REVISION_MMIO 0x59 + /* insmod parameters */ /* If force is set to anything different from 0, we forcibly enable the @@ -155,6 +162,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; @@ -162,8 +175,72 @@ 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, + struct sb800_mmio_cfg *mmio_cfg) +{ + if (mmio_cfg->use_mmio) { + struct resource *res; + void __iomem *addr; + + res = request_mem_region(SB800_PIIX4_FCH_PM_ADDR, + SB800_PIIX4_FCH_PM_SIZE, + "sb800_piix4_smb"); + if (!res) { + dev_err(dev, + "SMB 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, "SMB 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, + "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, + struct sb800_mmio_cfg *mmio_cfg) +{ + if (mmio_cfg->use_mmio) { + iounmap(mmio_cfg->addr); + mmio_cfg->addr = NULL; + + release_resource(mmio_cfg->res); + mmio_cfg->res = NULL; + } else { + 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) { @@ -263,12 +340,58 @@ 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) +{ + struct sb800_mmio_cfg mmio_cfg; + u8 smba_en_lo; + u8 smba_en_hi; + int retval; + + mmio_cfg.use_mmio = piix4_sb800_use_mmio(PIIX4_dev); + + retval = piix4_sb800_region_setup(&PIIX4_dev->dev, &mmio_cfg); + if (retval) + return retval; + + if (mmio_cfg.use_mmio) { + iowrite32(ioread32(mmio_cfg.addr + 4) | SB800_PIIX4_FCH_PM_DECODEEN_MMIO_EN, + 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); + + 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; + } + + 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; /* SB800 and later SMBus does not support forcing address */ if (force || force_addr) { @@ -290,29 +413,10 @@ 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")) { - dev_err(&PIIX4_dev->dev, - "SMB base address index region 0x%x already in use.\n", - SB800_PIIX4_SMB_IDX); - return -EBUSY; - } - - 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, 2); - - 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; - } + retval = piix4_setup_sb800_smba(PIIX4_dev, smb_en, + aux, &smb_en_status, &piix4_smba); + if (retval) + return retval; if (!smb_en_status) { dev_err(&PIIX4_dev->dev, @@ -662,6 +766,28 @@ static void piix4_imc_wakeup(void) release_region(KERNCZ_IMC_IDX, 2); } +static int piix4_sb800_port_sel(u8 port, struct sb800_mmio_cfg *mmio_cfg) +{ + u8 smba_en_lo; + + 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); + } + + 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). @@ -678,12 +804,12 @@ 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; - if (!request_muxed_region(SB800_PIIX4_SMB_IDX, 2, "sb800_piix4_smb")) - return -EBUSY; + retval = piix4_sb800_region_setup(&adap->dev, &adapdata->mmio_cfg); + if (retval) + return retval; /* Request the SMBUS semaphore, avoid conflicts with the IMC */ smbslvcnt = inb_p(SMBSLVCNT); @@ -738,18 +864,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, &adapdata->mmio_cfg); 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, &adapdata->mmio_cfg); /* Release the semaphore */ outb_p(smbslvcnt | 0x20, SMBSLVCNT); @@ -758,7 +878,7 @@ static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr, piix4_imc_wakeup(); release: - release_region(SB800_PIIX4_SMB_IDX, 2); + piix4_sb800_region_release(&adap->dev, &adapdata->mmio_cfg); return retval; } @@ -840,6 +960,7 @@ static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba, adapdata->sb800_main = sb800_main; adapdata->port = port << piix4_port_shift_sb800; adapdata->notify_imc = notify_imc; + adapdata->mmio_cfg.use_mmio = piix4_sb800_use_mmio(dev); /* set up the sysfs linkage to our parent device */ adap->dev.parent = &dev->dev;
cd6h/cd7h port io can be disabled on recent AMD hardware. Read accesses to disabled cd6h/cd7h will return F's and written data is dropped. The recommended workaround to handle disabled cd6h/cd7h port io is replacing with MMIO accesses. Support for MMIO has been available as an alternative cd6h/cd7h access method since at least smbus controller with PCI revision 0x59. The piix4_smbus driver uses cd6h/cd7h port io in the following 2 cases and requires updating to use MMIO: 1. The FCH::PM::DECODEEN[smbusasfiobase] and FCH::PM::DECODEEN[0..7] register fields are used to discover the smbus port io address. 2. During access requests the piix4_smbus driver enables the requested port if it is not already enabled. The downstream port is enabled through the FCH::PM::DECODEEN[smbus0sel] register. Signed-off-by: Terry Bowman <Terry.Bowman@amd.com> Cc: Jean Delvare <jdelvare@suse.com> Cc: Thomas Lendacky <Thomas.Lendacky@amd.com> Cc: linux-i2c@vger.kernel.org ---