Hi Jean, On 2/8/22 11:11, Jean Delvare wrote: > Hi Terry, > > On Sun, 30 Jan 2022 12:41:21 -0600, Terry Bowman wrote: >> This series changes the piix4_smbus driver's cd6h/cd7h port I/O accesses >> to use MMIO instead. This is necessary because cd6h/cd7h port I/O may be >> disabled on later AMD processors. >> >> This series includes patches with MMIO accesses to register >> FCH::PM::DECODEEN. The same register is also accessed by the sp5100_tco >> driver.[1] Synchronization to the MMIO register is required in both >> drivers. >> >> The first patch creates a macro to request MMIO region using the 'muxed' >> retry logic. This is used in patch 6 to synchronize accesses to EFCH MMIO. >> >> The second patch replaces a hardcoded region size with a #define. This is >> to improve maintainability and was requested from v2 review. >> >> The third patch moves duplicated region request/release code into >> functions. This locates related code into functions and reduces code line >> count. This will also make adding MMIO support in patch 6 easier. >> >> The fourth patch moves SMBus controller address detection into a function. >> This is in preparation for adding MMIO region support. >> >> The fifth patch moves EFCH port selection into a function. This is in >> preparation for adding MMIO region support. >> >> The sixth patch adds MMIO support for region requesting/releasing and >> mapping. This is necessary for using MMIO to detect SMBus controller >> address, enable SMBbus controller region, and control the port select. >> >> The seventh patch updates the SMBus controller address detection to support >> using MMIO. This is necessary because the driver accesses register >> FCH::PM::DECODEEN during initialization and only available using MMIO on >> later AMD processors. >> >> The eighth patch updates the SMBus port selection to support MMIO. This is >> required because port selection control resides in the >> FCH::PM::DECODEEN[smbus0sel] and is only accessible using MMIO on later AMD >> processors. >> >> The ninth patch enables the EFCH MMIO functionality added earlier in this >> series. The SMBus controller's PCI revision ID is used to check if EFCH >> MMIO is supported by HW and should be enabled in the driver. > > Thank you for splitting the changes into small chunks for easier > review. Maybe it was even a bit too much in the end, as most patches > don't serve a purpose on their own. But well, that's still much better > than a monolithic patch. > >> Based on v5.16. >> >> Testing: >> Tested on family 19h using: >> i2cdetect -y 0 >> i2cdetect -y 2 >> >> - Results using v5.16 and this pachset applied. Below >> shows the devices detected on the busses: >> >> # i2cdetect -y 0 >> 0 1 2 3 4 5 6 7 8 9 a b c d e f >> 00: -- -- -- -- -- -- -- -- >> 10: 10 11 -- -- -- -- -- -- 18 -- -- -- -- -- -- -- >> 20: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- >> 30: 30 -- -- -- -- 35 36 -- -- -- -- -- -- -- -- -- >> 40: -- -- -- -- -- -- -- -- -- -- 4a -- -- -- -- -- >> 50: 50 -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- >> 60: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- >> 70: -- -- -- 73 -- -- -- -- >> # i2cdetect -y 2 >> 0 1 2 3 4 5 6 7 8 9 a b c d e f >> 00: -- -- -- -- -- -- -- -- >> 10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- >> 20: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- >> 30: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- >> 40: -- -- -- -- -- -- -- -- -- -- -- -- 4c -- -- -- >> 50: -- 51 -- -- 54 -- -- -- -- -- -- -- -- -- -- -- >> 60: 60 -- -- 63 -- -- 66 -- -- -- -- 6b -- -- 6e -- >> 70: 70 71 72 73 74 75 -- 77 > > Unfortunately I'm not really able to test this series. While I do have > an AMD-based laptop which uses the i2c-piix4 driver, the SMBus has > never been usable on it. The driver creates 3 i2c buses (port 0 at > 0b00, port 2 at 0b00 and port 1 at 0b20). The first 2 do not seem to > have any device on them (i2cdetect returns empty). The last one must > have some devices on it, I see address 0x1c answers the ping, > unfortunately as soon as probing reaches address 0x2c, all later pings > return success, regardless of the address. It seems that some I2C > device (probably the one at 0x2c, but I don't know what it is) is > holding SDA low forever, therefore preventing any use of the whole > SMBus port until the next reboot. > My understanding is the OEM may have different i2c devices because it is mainboard specific. >> (...) >> Changes in v4: >> (...) >> - Removed iowrite32(ioread32(...), ...). Unnecessary because MMIO is >> already enabled. (Terry Bowman) > > I'm curious, how can you be sure of that actually? > The removed code was using a MMIO region to write 1 to 'mmioen'. This was using the MMIO region to enable same MMIO region. Programmer's manual shows FCH::PM::ISACONTROL[mmioen] has a '1' reset value. Per programmer's manual, FCH::PM::ISACONTROL[mmioen] enables MMIO mapping at FED8_0000h..FED8_1FFFh. The FCH::PM::ISACONTROL register is MMIO mapped at FED8_0304h. 'mmioen' was intended to be set using port I/O to enable MMIO but, port I/O access to these registers is now disabled. >> (...) >> Terry Bowman (9): >> kernel/resource: Introduce request_mem_region_muxed() >> i2c: piix4: Replace hardcoded memory map size with a #define >> i2c: piix4: Move port I/O region request/release code into functions >> i2c: piix4: Move SMBus controller base address detect into function >> i2c: piix4: Move SMBus port selection into function >> i2c: piix4: Add EFCH MMIO support to region request and release >> i2c: piix4: Add EFCH MMIO support to SMBus base address detect >> i2c: piix4: Add EFCH MMIO support for SMBus port select >> i2c: piix4: Enable EFCH MMIO for Family 17h+ >> >> drivers/i2c/busses/i2c-piix4.c | 207 ++++++++++++++++++++++++++------- >> include/linux/ioport.h | 2 + >> 2 files changed, 164 insertions(+), 45 deletions(-) > > I'm done with my review, looks good overall, I made a few comments here > and there but no major issue. I'll leave it up to you (and Wolfram) to > either send a new series with (some of) my suggestions addressed or > just go with v4. In both cases you can add: > I'll add your requested fixes into v5 and will send for review this afternoon. > Reviewed-by: Jean Delvare <jdelvare@xxxxxxx> > > to all patches. > > Thank you very much for your work, and sorry for my late review. > Thanks for the review. Regards, Terry
