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
