Re: [PATCH] i2c: piix4: Replace piix4_smbus driver's cd6h/cd7h port io accesses with mmio accesses

[Jean Delvare <jdelvare@suse.de>]

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.

-- 
Jean Delvare
SUSE L3 Support