[PATCH] i2c: i801: Fix incorrect and needless software PEC disabling

[PATCH] i2c: i801: Fix incorrect and needless software PEC disabling

[Jarkko Nikula]

Commit a6b8bb6a813a ("i2c: i801: Fix handling SMBHSTCNT_PEC_EN")
attempts to disable software PEC by clearing the SMBHSTCNT_PEC_EN (bit 7)
in the SMBus Host Control register (I/O SMBHSTCNT) but incorrectly
clears it in the PCI Host Configuration register (PCI SMBHSTCFG).

This clearing is actually needless since after above commit the
SMBHSTCNT_PEC_EN is never set and the register is initialized with known
values.

Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
---
I didn't add Fixes tag and Cc stable@vger.kernel.org since I don't see
this causes any real issues. A few platforms I checked the PCI Host
Configuration register bit 7 was reserved 0.
---
 drivers/i2c/busses/i2c-i801.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 115660dce722..b6048a571543 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1657,7 +1657,6 @@ static void i801_setup_hstcfg(struct i801_priv *priv)
 	unsigned char hstcfg = priv->original_hstcfg;
 
 	hstcfg &= ~SMBHSTCFG_I2C_EN;	/* SMBus timing */
-	hstcfg &= ~SMBHSTCNT_PEC_EN;	/* Disable software PEC */
 	hstcfg |= SMBHSTCFG_HST_EN;
 	pci_write_config_byte(priv->pci_dev, SMBHSTCFG, hstcfg);
 }
-- 
2.33.0

Re: [PATCH] i2c: i801: Fix incorrect and needless software PEC disabling

[Heiner Kallweit]

On 26.10.2021 16:39, Jarkko Nikula wrote:
> Commit a6b8bb6a813a ("i2c: i801: Fix handling SMBHSTCNT_PEC_EN")
> attempts to disable software PEC by clearing the SMBHSTCNT_PEC_EN (bit 7)
> in the SMBus Host Control register (I/O SMBHSTCNT) but incorrectly
> clears it in the PCI Host Configuration register (PCI SMBHSTCFG).
> 
Indeed, my bad. Mixed up HSTCFG and HSTCNT here.

> This clearing is actually needless since after above commit the
> SMBHSTCNT_PEC_EN is never set and the register is initialized with known
> values.
> 
> Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> ---
> I didn't add Fixes tag and Cc stable@vger.kernel.org since I don't see
> this causes any real issues. A few platforms I checked the PCI Host
> Configuration register bit 7 was reserved 0.
> ---
>  drivers/i2c/busses/i2c-i801.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 115660dce722..b6048a571543 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -1657,7 +1657,6 @@ static void i801_setup_hstcfg(struct i801_priv *priv)
>  	unsigned char hstcfg = priv->original_hstcfg;
>  
>  	hstcfg &= ~SMBHSTCFG_I2C_EN;	/* SMBus timing */
> -	hstcfg &= ~SMBHSTCNT_PEC_EN;	/* Disable software PEC */
>  	hstcfg |= SMBHSTCFG_HST_EN;
>  	pci_write_config_byte(priv->pci_dev, SMBHSTCFG, hstcfg);
>  }
> 

Re: [PATCH] i2c: i801: Fix incorrect and needless software PEC disabling

[Jean Delvare]

On Tue, 26 Oct 2021 17:39:16 +0300, Jarkko Nikula wrote:
> Commit a6b8bb6a813a ("i2c: i801: Fix handling SMBHSTCNT_PEC_EN")
> attempts to disable software PEC by clearing the SMBHSTCNT_PEC_EN (bit 7)
> in the SMBus Host Control register (I/O SMBHSTCNT) but incorrectly
> clears it in the PCI Host Configuration register (PCI SMBHSTCFG).
> 
> This clearing is actually needless since after above commit the
> SMBHSTCNT_PEC_EN is never set and the register is initialized with known
> values.
> 
> Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

Oops, sorry for missing this and duplicating your work. I noticed the
bug while reviewing your other change and wrote a fix without realizing
you were already working on it.

Reviewed-by: Jean Delvare <jdelvare@suse.de>

> ---
> I didn't add Fixes tag and Cc stable@vger.kernel.org since I don't see
> this causes any real issues. A few platforms I checked the PCI Host
> Configuration register bit 7 was reserved 0.

I checked the datasheets too and came to the same conclusion, but added
the Fixes tag still, because the new fix is definitely safe and we have
no idea if the broken fix was actually harmless as it must have seen
very little deployment at this point (kernel 5.15 isn't out yet).
Better safe than sorry.

> ---
>  drivers/i2c/busses/i2c-i801.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 115660dce722..b6048a571543 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -1657,7 +1657,6 @@ static void i801_setup_hstcfg(struct i801_priv *priv)
>  	unsigned char hstcfg = priv->original_hstcfg;
>  
>  	hstcfg &= ~SMBHSTCFG_I2C_EN;	/* SMBus timing */
> -	hstcfg &= ~SMBHSTCNT_PEC_EN;	/* Disable software PEC */
>  	hstcfg |= SMBHSTCFG_HST_EN;
>  	pci_write_config_byte(priv->pci_dev, SMBHSTCFG, hstcfg);
>  }


-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH] i2c: i801: Fix incorrect and needless software PEC disabling

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 608 bytes --]

On Tue, Oct 26, 2021 at 05:39:16PM +0300, Jarkko Nikula wrote:
> Commit a6b8bb6a813a ("i2c: i801: Fix handling SMBHSTCNT_PEC_EN")
> attempts to disable software PEC by clearing the SMBHSTCNT_PEC_EN (bit 7)
> in the SMBus Host Control register (I/O SMBHSTCNT) but incorrectly
> clears it in the PCI Host Configuration register (PCI SMBHSTCFG).
> 
> This clearing is actually needless since after above commit the
> SMBHSTCNT_PEC_EN is never set and the register is initialized with known
> values.
> 
> Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

Applied to for-next, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]