| Message ID | 20220119230626.31560-8-terry.bowman@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | i2c: piix4: Replace cd6h/cd7h port I/O accesses with MMIO accesses | expand |
On Thu, Jan 20, 2022 at 1:07 AM Terry Bowman <terry.bowman@amd.com> wrote: > > The EFCH SMBus controller's base address is determined using details in > FCH::PM::DECODEEN[smbusasfiobase] and FCH::PM::DECODEEN[smbusasfioen]. > This code also writes to FCH::PM::ISACONTROL[mmioen] to enable MMIO > decoding. These register fields were accessed using cd6h/cd7h port I/O. > cd6h/cd7h port I/O is no longer available in later AMD processors. > Change base address detection to use MMIO instead of port I/O cd6h/cd7h. ... > + if (mmio_cfg.use_mmio) { > + iowrite32(ioread32(mmio_cfg.addr + 4) | SB800_PIIX4_FCH_PM_DECODEEN_MMIO, > + mmio_cfg.addr + 4); Can you split this to three lines (with the help of a temporary variable)? > + smba_en_lo = ioread8(mmio_cfg.addr); > + smba_en_hi = ioread8(mmio_cfg.addr + 1); This makes me wonder if we can replace these two by defining u16 smba_en; (below also may be easily adjusted for it). > + } else { > + outb_p(smb_en, SB800_PIIX4_SMB_IDX); > + smba_en_lo = inb_p(SB800_PIIX4_SMB_IDX + 1); > + outb_p(smb_en + 1, SB800_PIIX4_SMB_IDX); > + smba_en_hi = inb_p(SB800_PIIX4_SMB_IDX + 1); > + } > > piix4_sb800_region_release(&PIIX4_dev->dev, &mmio_cfg);
On 1/20/22 5:27 AM, Andy Shevchenko wrote: > On Thu, Jan 20, 2022 at 1:07 AM Terry Bowman <terry.bowman@amd.com> wrote: >> >> The EFCH SMBus controller's base address is determined using details in >> FCH::PM::DECODEEN[smbusasfiobase] and FCH::PM::DECODEEN[smbusasfioen]. >> This code also writes to FCH::PM::ISACONTROL[mmioen] to enable MMIO >> decoding. These register fields were accessed using cd6h/cd7h port I/O. >> cd6h/cd7h port I/O is no longer available in later AMD processors. >> Change base address detection to use MMIO instead of port I/O cd6h/cd7h. > > ... > >> + if (mmio_cfg.use_mmio) { > >> + iowrite32(ioread32(mmio_cfg.addr + 4) | SB800_PIIX4_FCH_PM_DECODEEN_MMIO, >> + mmio_cfg.addr + 4); > > Can you split this to three lines (with the help of a temporary variable)? Yes. > >> + smba_en_lo = ioread8(mmio_cfg.addr); >> + smba_en_hi = ioread8(mmio_cfg.addr + 1); > > This makes me wonder if we can replace these two by defining > > u16 smba_en; > > (below also may be easily adjusted for it). > Yes. Regards, Terry >> + } else { >> + outb_p(smb_en, SB800_PIIX4_SMB_IDX); >> + smba_en_lo = inb_p(SB800_PIIX4_SMB_IDX + 1); >> + outb_p(smb_en + 1, SB800_PIIX4_SMB_IDX); >> + smba_en_hi = inb_p(SB800_PIIX4_SMB_IDX + 1); >> + } >> >> piix4_sb800_region_release(&PIIX4_dev->dev, &mmio_cfg); >
On 1/20/22 5:27 AM, Andy Shevchenko wrote: > On Thu, Jan 20, 2022 at 1:07 AM Terry Bowman <terry.bowman@amd.com> wrote: >> >> The EFCH SMBus controller's base address is determined using details in >> FCH::PM::DECODEEN[smbusasfiobase] and FCH::PM::DECODEEN[smbusasfioen]. >> This code also writes to FCH::PM::ISACONTROL[mmioen] to enable MMIO >> decoding. These register fields were accessed using cd6h/cd7h port I/O. >> cd6h/cd7h port I/O is no longer available in later AMD processors. >> Change base address detection to use MMIO instead of port I/O cd6h/cd7h. > > ... > >> + if (mmio_cfg.use_mmio) { > >> + iowrite32(ioread32(mmio_cfg.addr + 4) | SB800_PIIX4_FCH_PM_DECODEEN_MMIO, >> + mmio_cfg.addr + 4); > > Can you split this to three lines (with the help of a temporary variable)? > I made these changes for upcoming v4. >> + smba_en_lo = ioread8(mmio_cfg.addr); >> + smba_en_hi = ioread8(mmio_cfg.addr + 1); > > This makes me wonder if we can replace these two by defining > > u16 smba_en; > Hi Alex, I was unable to make these changes without losing readability. By using a u16 smba_en I found casting required between u8 and u16 to access the byte offsets. This increased the number of lines and complexity. I should mention the port I/O here is fixed at a byte and does not support 16bit port I/O. I'll leave as-is if Ok with you. Regards, Terry > (below also may be easily adjusted for it). > >> + } else { >> + outb_p(smb_en, SB800_PIIX4_SMB_IDX); >> + smba_en_lo = inb_p(SB800_PIIX4_SMB_IDX + 1); >> + outb_p(smb_en + 1, SB800_PIIX4_SMB_IDX); >> + smba_en_hi = inb_p(SB800_PIIX4_SMB_IDX + 1); >> + } >> >> piix4_sb800_region_release(&PIIX4_dev->dev, &mmio_cfg); >
> I'll leave as-is if Ok with you.
Yes, it is OK.
diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c index 7aa6ceb27e4b..b85f926451b8 100644 --- a/drivers/i2c/busses/i2c-piix4.c +++ b/drivers/i2c/busses/i2c-piix4.c @@ -98,6 +98,7 @@ #define SB800_PIIX4_PORT_IDX_MASK_KERNCZ 0x18 #define SB800_PIIX4_PORT_IDX_SHIFT_KERNCZ 3 +#define SB800_PIIX4_FCH_PM_DECODEEN_MMIO BIT(1) #define SB800_PIIX4_FCH_PM_ADDR 0xFED80300 #define SB800_PIIX4_FCH_PM_SIZE 8 @@ -344,10 +345,18 @@ static int piix4_setup_sb800_smba(struct pci_dev *PIIX4_dev, if (retval) return retval; - outb_p(smb_en, SB800_PIIX4_SMB_IDX); - smba_en_lo = inb_p(SB800_PIIX4_SMB_IDX + 1); - outb_p(smb_en + 1, SB800_PIIX4_SMB_IDX); - smba_en_hi = inb_p(SB800_PIIX4_SMB_IDX + 1); + if (mmio_cfg.use_mmio) { + iowrite32(ioread32(mmio_cfg.addr + 4) | SB800_PIIX4_FCH_PM_DECODEEN_MMIO, + mmio_cfg.addr + 4); + + smba_en_lo = ioread8(mmio_cfg.addr); + smba_en_hi = ioread8(mmio_cfg.addr + 1); + } else { + outb_p(smb_en, SB800_PIIX4_SMB_IDX); + smba_en_lo = inb_p(SB800_PIIX4_SMB_IDX + 1); + outb_p(smb_en + 1, SB800_PIIX4_SMB_IDX); + smba_en_hi = inb_p(SB800_PIIX4_SMB_IDX + 1); + } piix4_sb800_region_release(&PIIX4_dev->dev, &mmio_cfg);
The EFCH SMBus controller's base address is determined using details in FCH::PM::DECODEEN[smbusasfiobase] and FCH::PM::DECODEEN[smbusasfioen]. This code also writes to FCH::PM::ISACONTROL[mmioen] to enable MMIO decoding. These register fields were accessed using cd6h/cd7h port I/O. cd6h/cd7h port I/O is no longer available in later AMD processors. Change base address detection to use MMIO instead of port I/O cd6h/cd7h. Signed-off-by: Terry Bowman <terry.bowman@amd.com> --- drivers/i2c/busses/i2c-piix4.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)