lkml.org 
[lkml]   [2021]   [Nov]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] i2c: piix4: Replace piix4_smbus driver's cd6h/cd7h port io accesses with mmio accesses
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

\
 
 \ /
  Last update: 2021-11-05 17:06    [W:0.205 / U:0.536 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site