Re: [PATCH] i2c: piix4: Replace piix4_smbus driver's cd6h/cd7h port io accesses with mmio accesses
From: Guenter Roeck
Date: Thu Jan 06 2022 - 13:18:27 EST
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.