| Message ID | 20220130184130.176646-6-terry.bowman@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | i2c: piix4: Replace cd6h/cd7h port I/O accesses with MMIO accesses | expand |
On Sun, 30 Jan 2022 12:41:26 -0600, Terry Bowman wrote: > Move port selection code into a separate function. Refactor is in > preparation for following MMIO changes. > > Signed-off-by: Terry Bowman <terry.bowman@amd.com> > --- > drivers/i2c/busses/i2c-piix4.c | 26 ++++++++++++++++---------- > 1 file changed, 16 insertions(+), 10 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c > index eab06e9e5fdf..32a30af5778a 100644 > --- a/drivers/i2c/busses/i2c-piix4.c > +++ b/drivers/i2c/busses/i2c-piix4.c > @@ -699,6 +699,19 @@ static void piix4_imc_wakeup(void) > release_region(KERNCZ_IMC_IDX, 2); > } > > +static int piix4_sb800_port_sel(u8 port) > +{ > + u8 smba_en_lo, val; > + > + outb_p(piix4_port_sel_sb800, SB800_PIIX4_SMB_IDX); > + smba_en_lo = inb_p(SB800_PIIX4_SMB_IDX + 1); > + > + val = (smba_en_lo & ~piix4_port_mask_sb800) | port; > + if (smba_en_lo != val) > + outb_p(val, SB800_PIIX4_SMB_IDX + 1); > + > + return (smba_en_lo & piix4_port_mask_sb800); > +} > /* > * Handles access to multiple SMBus ports on the SB800. > * The port is selected by bits 2:1 of the smb_en register (0x2c). > @@ -715,8 +728,7 @@ static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr, > unsigned short piix4_smba = adapdata->smba; > int retries = MAX_TIMEOUT; > int smbslvcnt; > - u8 smba_en_lo; > - u8 port; > + u8 prev_port; > int retval; > > retval = piix4_sb800_region_request(&adap->dev); > @@ -776,18 +788,12 @@ static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr, > } > } > > - outb_p(piix4_port_sel_sb800, SB800_PIIX4_SMB_IDX); > - smba_en_lo = inb_p(SB800_PIIX4_SMB_IDX + 1); > - > - port = adapdata->port; > - if ((smba_en_lo & piix4_port_mask_sb800) != port) > - outb_p((smba_en_lo & ~piix4_port_mask_sb800) | port, > - SB800_PIIX4_SMB_IDX + 1); > + prev_port = piix4_sb800_port_sel(adapdata->port); > > retval = piix4_access(adap, addr, flags, read_write, > command, size, data); > > - outb_p(smba_en_lo, SB800_PIIX4_SMB_IDX + 1); > + piix4_sb800_port_sel(prev_port); > > /* Release the semaphore */ > outb_p(smbslvcnt | 0x20, SMBSLVCNT); Hmm, this gets pretty inefficient. You set SB800_PIIX4_SMB_IDX to piix4_port_sel_sb800 twice and compare the new port with the previous port twice, even though the result will inevitably be the same. The original code would just unconditionally restore the original port value, which was also not always needed, but was cheaper in the end. I can't really think of a better way though, so, so be it :-( I wonder why we insist on restoring the port though. When you read multiple values from the same bus (and I2C device drivers do that a lot, obviously) you end up switching ports a lot for no good reason. I looked at the git history and it has been that way since support for the multiplexed buses was added back in 2015. There's a mutex protecting that section of code anyway, so I don't think restoring the port buys us anything we did not already have. The only benefit I can see is to leave the port setting in its original state on suspend and shutdown (and we know by experience that it can be important to the BIOS) but then we should just do it in the suspend and remove functions, instead of after every transfer.
diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c index eab06e9e5fdf..32a30af5778a 100644 --- a/drivers/i2c/busses/i2c-piix4.c +++ b/drivers/i2c/busses/i2c-piix4.c @@ -699,6 +699,19 @@ static void piix4_imc_wakeup(void) release_region(KERNCZ_IMC_IDX, 2); } +static int piix4_sb800_port_sel(u8 port) +{ + u8 smba_en_lo, val; + + outb_p(piix4_port_sel_sb800, SB800_PIIX4_SMB_IDX); + smba_en_lo = inb_p(SB800_PIIX4_SMB_IDX + 1); + + val = (smba_en_lo & ~piix4_port_mask_sb800) | port; + if (smba_en_lo != val) + outb_p(val, SB800_PIIX4_SMB_IDX + 1); + + return (smba_en_lo & piix4_port_mask_sb800); +} /* * Handles access to multiple SMBus ports on the SB800. * The port is selected by bits 2:1 of the smb_en register (0x2c). @@ -715,8 +728,7 @@ static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr, unsigned short piix4_smba = adapdata->smba; int retries = MAX_TIMEOUT; int smbslvcnt; - u8 smba_en_lo; - u8 port; + u8 prev_port; int retval; retval = piix4_sb800_region_request(&adap->dev); @@ -776,18 +788,12 @@ static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr, } } - outb_p(piix4_port_sel_sb800, SB800_PIIX4_SMB_IDX); - smba_en_lo = inb_p(SB800_PIIX4_SMB_IDX + 1); - - port = adapdata->port; - if ((smba_en_lo & piix4_port_mask_sb800) != port) - outb_p((smba_en_lo & ~piix4_port_mask_sb800) | port, - SB800_PIIX4_SMB_IDX + 1); + prev_port = piix4_sb800_port_sel(adapdata->port); retval = piix4_access(adap, addr, flags, read_write, command, size, data); - outb_p(smba_en_lo, SB800_PIIX4_SMB_IDX + 1); + piix4_sb800_port_sel(prev_port); /* Release the semaphore */ outb_p(smbslvcnt | 0x20, SMBSLVCNT);
Move port selection code into a separate function. Refactor is in preparation for following MMIO changes. Signed-off-by: Terry Bowman <terry.bowman@amd.com> --- drivers/i2c/busses/i2c-piix4.c | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-)