| Message ID | 20220130184130.176646-1-terry.bowman@amd.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | i2c: piix4: Replace cd6h/cd7h port I/O accesses with MMIO accesses | expand |
On Sun, Jan 30, 2022 at 8:41 PM Terry Bowman <terry.bowman@amd.com> 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. I'm not against the series, but I am still concerned that we are using _p IO without clear understanding why. With cleaning them up, this can be simpler and cleaner. For the i2c patches: Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> if it helps somebody. > Based on v5.16. v5.17-rc2 is out :-) > > 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 > > Also tested using sp5100_tco submitted series listed below.[1] > I applied the sp5100_tco v4 series and ran: > cat >> /dev/watchdog > > [1] sp5100_tco v4 patchset is in process but not sent yet. Below is v3 > upstream review: > Link: https://lore.kernel.org/linux-watchdog/20220118202234.410555-1-terry.bowman@amd.com/ > > Changes in v4: > - Changed request_muxed_mem_region() macro to request_mem_region_muxed() > in patch 1. (Andy Shevchenko) > - Removed unnecessary newline where possible in calls to > request_muxed_region() in patch 2. (Terry Bowman) > - Changed piix4_sb800_region_setup() to piix4_sb800_region_request(). > Patch 3. (Jean Delvare) > - Reordered piix4_setup_sb800() local variables from longest name length > to shortest name length. Patch 4. (Terry Bowman) > - Changed piix4_sb800_region_request() and piix4_sb800_region_release() by > adding early return() to remove 'else' improving readability. Patch 6. > (Terry Bowman) > - Removed iowrite32(ioread32(...), ...). Unnecessary because MMIO is > already enabled. (Terry Bowman) > - Refactored piix4_sb800_port_sel() to simplify the 'if' statement using > temp variable. Patch 8. (Terry Bowman) > - Added mmio_cfg.use_mmio assignment in piix4_add_adapter(). This is > needed for calls to piix4_sb800_port_sel() after initialization during > normal operation. Patch 9. (Terry Bowman) > > Changes in v3: > - Added request_muxed_mem_region() patch (Wolfram, Guenter) > - Reduced To/Cc list length. (Andy) > > Changes in v2: > - Split single patch. (Jean Delvare) > - Replace constant 2 with SB800_PIIX4_SMB_MAP_SIZE where appropriate. > (Jean Delvare) > - Shorten SB800_PIIX4_FCH_PM_DECODEEN_MMIO_EN name length to > SB800_PIIX4_FCH_PM_DECODEEN_MMIO. (Jean Delvare) > - Change AMD_PCI_SMBUS_REVISION_MMIO from 0x59 to 0x51. (Terry Bowman) > - Change piix4_sb800_region_setup() to piix4_sb800_region_request(). > (Jean Delvare) > - Change 'SMB' text in logging to 'SMBus' (Jean Delvare) > - Remove unnecessary NULL assignment in piix4_sb800_region_release(). > (Jean Delvare) > - Move 'u8' variable definitions to single line. (Jean Delvare) > - Hardcode piix4_setup_sb800_smba() return value to 0 since it is always > 0. (Jean Delvare) > > 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(-) > > -- > 2.30.2
Hi Andy, On 1/31/22 05:01, Andy Shevchenko wrote: > On Sun, Jan 30, 2022 at 8:41 PM Terry Bowman <terry.bowman@amd.com> 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. > > I'm not against the series, but I am still concerned that we are using > _p IO without clear understanding why. With cleaning them up, this can > be simpler and cleaner. > > For the i2c patches: > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > if it helps somebody. > Thanks for the review Andy! I plan to add the ioport_map changes you recommend in a future patchset. I will include you as "suggested-by". Regards, Terry >> Based on v5.16. > > v5.17-rc2 is out :-) > >> >> 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 >> >> Also tested using sp5100_tco submitted series listed below.[1] >> I applied the sp5100_tco v4 series and ran: >> cat >> /dev/watchdog >> >> [1] sp5100_tco v4 patchset is in process but not sent yet. Below is v3 >> upstream review: >> Link: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flinux-watchdog%2F20220118202234.410555-1-terry.bowman%40amd.com%2F&data=04%7C01%7Cterry.bowman%40amd.com%7Ca4a101d574724ba6958808d9e4a94579%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637792237972109034%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=LNxoYDcBRCT4Fih%2F8v1m9Xwbvbq1rqHbFlAcqrT4YDU%3D&reserved=0 >> >> Changes in v4: >> - Changed request_muxed_mem_region() macro to request_mem_region_muxed() >> in patch 1. (Andy Shevchenko) >> - Removed unnecessary newline where possible in calls to >> request_muxed_region() in patch 2. (Terry Bowman) >> - Changed piix4_sb800_region_setup() to piix4_sb800_region_request(). >> Patch 3. (Jean Delvare) >> - Reordered piix4_setup_sb800() local variables from longest name length >> to shortest name length. Patch 4. (Terry Bowman) >> - Changed piix4_sb800_region_request() and piix4_sb800_region_release() by >> adding early return() to remove 'else' improving readability. Patch 6. >> (Terry Bowman) >> - Removed iowrite32(ioread32(...), ...). Unnecessary because MMIO is >> already enabled. (Terry Bowman) >> - Refactored piix4_sb800_port_sel() to simplify the 'if' statement using >> temp variable. Patch 8. (Terry Bowman) >> - Added mmio_cfg.use_mmio assignment in piix4_add_adapter(). This is >> needed for calls to piix4_sb800_port_sel() after initialization during >> normal operation. Patch 9. (Terry Bowman) >> >> Changes in v3: >> - Added request_muxed_mem_region() patch (Wolfram, Guenter) >> - Reduced To/Cc list length. (Andy) >> >> Changes in v2: >> - Split single patch. (Jean Delvare) >> - Replace constant 2 with SB800_PIIX4_SMB_MAP_SIZE where appropriate. >> (Jean Delvare) >> - Shorten SB800_PIIX4_FCH_PM_DECODEEN_MMIO_EN name length to >> SB800_PIIX4_FCH_PM_DECODEEN_MMIO. (Jean Delvare) >> - Change AMD_PCI_SMBUS_REVISION_MMIO from 0x59 to 0x51. (Terry Bowman) >> - Change piix4_sb800_region_setup() to piix4_sb800_region_request(). >> (Jean Delvare) >> - Change 'SMB' text in logging to 'SMBus' (Jean Delvare) >> - Remove unnecessary NULL assignment in piix4_sb800_region_release(). >> (Jean Delvare) >> - Move 'u8' variable definitions to single line. (Jean Delvare) >> - Hardcode piix4_setup_sb800_smba() return value to 0 since it is always >> 0. (Jean Delvare) >> >> 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(-) >> >> -- >> 2.30.2 > > >
On Sun, Jan 30, 2022 at 12:41:21PM -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. Review from Andy is already great, I'll give Jean a few more days for comments.
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. > (...) > 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? > (...) > 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: Reviewed-by: Jean Delvare <jdelvare@suse.de> to all patches. Thank you very much for your work, and sorry for my late review.
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@suse.de> > > to all patches. > > Thank you very much for your work, and sorry for my late review. > Thanks for the review. Regards, Terry
On Tue, 8 Feb 2022 13:36:55 -0600, Terry Bowman wrote: > On 2/8/22 11:11, Jean Delvare wrote: > > 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. Yes, the devices on the SMBus could be anything Lenovo decided to use. Tomorrow I'll try to scan the SMBus but skipping the problematic address, to see if it works around the problem. > >> (...) > >> 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. Ah ah, I get it now. Nice chicken or the egg situation :-) > 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. Is my understanding correct that there is some overlapping of the access methods and there are certain chipsets where both legacy I/O and MMIO access is possible? If so, while there's indeed nothing to be done for the most recent systems where only MMIO access is possible, you may still need to enable MMIO access through legacy I/O if you try to use MMIO on chipsets where both are possible. I'm not sure what exactly where you set the limit. In the last patch you say that 0x51 is the first revision of the family 17h CPUs, but is family 17h the first where MMIO is available, or the first where legacy I/O isn't?
Hi Jean, On 2/8/22 15:46, Jean Delvare wrote: > On Tue, 8 Feb 2022 13:36:55 -0600, Terry Bowman wrote: >> On 2/8/22 11:11, Jean Delvare wrote: >>> 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. > > Yes, the devices on the SMBus could be anything Lenovo decided to use. > Tomorrow I'll try to scan the SMBus but skipping the problematic > address, to see if it works around the problem. > >>>> (...) >>>> 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. > > Ah ah, I get it now. Nice chicken or the egg situation :-) > >> 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. > > Is my understanding correct that there is some overlapping of the > access methods and there are certain chipsets where both legacy I/O and > MMIO access is possible? > Yes. > If so, while there's indeed nothing to be done for the most recent > systems where only MMIO access is possible, you may still need to > enable MMIO access through legacy I/O if you try to use MMIO on > chipsets where both are possible. I'm not sure what exactly where you > set the limit. In the last patch you say that 0x51 is the first > revision of the family 17h CPUs, but is family 17h the first where MMIO > is available, or the first where legacy I/O isn't? > Family 17h, SMBus PCI ID >= 0x51 is the first where cd6h/cd7h port I/O is disabled. If SMBus PCI ID < 0x51 then cd6h/cd7h port I/O is used. Regards, Terry
On Tue, 8 Feb 2022 17:03:09 -0600, Terry Bowman wrote: > On 2/8/22 15:46, Jean Delvare wrote: > > If so, while there's indeed nothing to be done for the most recent > > systems where only MMIO access is possible, you may still need to > > enable MMIO access through legacy I/O if you try to use MMIO on > > chipsets where both are possible. I'm not sure what exactly where you > > set the limit. In the last patch you say that 0x51 is the first > > revision of the family 17h CPUs, but is family 17h the first where MMIO > > is available, or the first where legacy I/O isn't? > > Family 17h, SMBus PCI ID >= 0x51 is the first where cd6h/cd7h port I/O is disabled. > If SMBus PCI ID < 0x51 then cd6h/cd7h port I/O is used. OK, we are safe then :-)
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. 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 Also tested using sp5100_tco submitted series listed below.[1] I applied the sp5100_tco v4 series and ran: cat >> /dev/watchdog [1] sp5100_tco v4 patchset is in process but not sent yet. Below is v3 upstream review: Link: https://lore.kernel.org/linux-watchdog/20220118202234.410555-1-terry.bowman@amd.com/ Changes in v4: - Changed request_muxed_mem_region() macro to request_mem_region_muxed() in patch 1. (Andy Shevchenko) - Removed unnecessary newline where possible in calls to request_muxed_region() in patch 2. (Terry Bowman) - Changed piix4_sb800_region_setup() to piix4_sb800_region_request(). Patch 3. (Jean Delvare) - Reordered piix4_setup_sb800() local variables from longest name length to shortest name length. Patch 4. (Terry Bowman) - Changed piix4_sb800_region_request() and piix4_sb800_region_release() by adding early return() to remove 'else' improving readability. Patch 6. (Terry Bowman) - Removed iowrite32(ioread32(...), ...). Unnecessary because MMIO is already enabled. (Terry Bowman) - Refactored piix4_sb800_port_sel() to simplify the 'if' statement using temp variable. Patch 8. (Terry Bowman) - Added mmio_cfg.use_mmio assignment in piix4_add_adapter(). This is needed for calls to piix4_sb800_port_sel() after initialization during normal operation. Patch 9. (Terry Bowman) Changes in v3: - Added request_muxed_mem_region() patch (Wolfram, Guenter) - Reduced To/Cc list length. (Andy) Changes in v2: - Split single patch. (Jean Delvare) - Replace constant 2 with SB800_PIIX4_SMB_MAP_SIZE where appropriate. (Jean Delvare) - Shorten SB800_PIIX4_FCH_PM_DECODEEN_MMIO_EN name length to SB800_PIIX4_FCH_PM_DECODEEN_MMIO. (Jean Delvare) - Change AMD_PCI_SMBUS_REVISION_MMIO from 0x59 to 0x51. (Terry Bowman) - Change piix4_sb800_region_setup() to piix4_sb800_region_request(). (Jean Delvare) - Change 'SMB' text in logging to 'SMBus' (Jean Delvare) - Remove unnecessary NULL assignment in piix4_sb800_region_release(). (Jean Delvare) - Move 'u8' variable definitions to single line. (Jean Delvare) - Hardcode piix4_setup_sb800_smba() return value to 0 since it is always 0. (Jean Delvare) 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(-)