i801: I2C block read should be usable also on ICH/ICH0/ICH2/ICH3/ICH4

i801: I2C block read should be usable also on ICH/ICH0/ICH2/ICH3/ICH4

[Heiner Kallweit]

Currently we use the following feature definitions. However, according to
the respective datasheets, also ICH/ICH0/ICH2/ICH3/ICH4 support I2C block
read. The implementation we have should work also on these chip versions.

The commit message of 6342064cad7a ("i2c-i801: Implement I2C block read
support") states that i2c block read is supported from ICH5 only.
This doesn't seem to be true. Or is this feature broken on older chip
versions? 

To me it seems we could remove FEATURE_I2C_BLOCK_READ because all chip
versions support this feature. Below is an experimental patch, for the
ones with test hw. A test case could be to use decode-dimms that
uses i2c block read to read the EEPROM content.

* Supports the following Intel I/O Controller Hubs (ICH):
 *
 *					I/O			Block	I2C
 *					region	SMBus	Block	proc.	block
 * Chip name			PCI ID	size	PEC	buffer	call	read
 * ---------------------------------------------------------------------------
 * 82801AA (ICH)		0x2413	16	no	no	no	no
 * 82801AB (ICH0)		0x2423	16	no	no	no	no
 * 82801BA (ICH2)		0x2443	16	no	no	no	no
 * 82801CA (ICH3)		0x2483	32	soft	no	no	no
 * 82801DB (ICH4)		0x24c3	32	hard	yes	no	no
 * 82801E (ICH5)		0x24d3	32	hard	yes	yes	yes


diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 930c6edbe..fd9a19a80 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -294,7 +294,6 @@ struct i801_priv {
 #define FEATURE_SMBUS_PEC	BIT(0)
 #define FEATURE_BLOCK_BUFFER	BIT(1)
 #define FEATURE_BLOCK_PROC	BIT(2)
-#define FEATURE_I2C_BLOCK_READ	BIT(3)
 #define FEATURE_IRQ		BIT(4)
 #define FEATURE_HOST_NOTIFY	BIT(5)
 /* Not really a feature, but it's convenient to handle it as such */
@@ -780,10 +779,6 @@ static int i801_block_transaction(struct i801_priv *priv, union i2c_smbus_data *
 			pci_read_config_byte(priv->pci_dev, SMBHSTCFG, &hostc);
 			pci_write_config_byte(priv->pci_dev, SMBHSTCFG,
 					      hostc | SMBHSTCFG_I2C_EN);
-		} else if (!(priv->features & FEATURE_I2C_BLOCK_READ)) {
-			dev_err(&priv->pci_dev->dev,
-				"I2C block read is unsupported!\n");
-			return -EOPNOTSUPP;
 		}
 	}
 
@@ -956,11 +951,10 @@ static u32 i801_func(struct i2c_adapter *adapter)
 	return I2C_FUNC_SMBUS_QUICK | I2C_FUNC_SMBUS_BYTE |
 	       I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA |
 	       I2C_FUNC_SMBUS_BLOCK_DATA | I2C_FUNC_SMBUS_WRITE_I2C_BLOCK |
+	       I2C_FUNC_SMBUS_READ_I2C_BLOCK |
 	       ((priv->features & FEATURE_SMBUS_PEC) ? I2C_FUNC_SMBUS_PEC : 0) |
 	       ((priv->features & FEATURE_BLOCK_PROC) ?
 		I2C_FUNC_SMBUS_BLOCK_PROC_CALL : 0) |
-	       ((priv->features & FEATURE_I2C_BLOCK_READ) ?
-		I2C_FUNC_SMBUS_READ_I2C_BLOCK : 0) |
 	       ((priv->features & FEATURE_HOST_NOTIFY) ?
 		I2C_FUNC_SMBUS_HOST_NOTIFY : 0);
 }
@@ -997,7 +991,7 @@ static const struct i2c_algorithm smbus_algorithm = {
 	.functionality	= i801_func,
 };
 
-#define FEATURES_ICH5	(FEATURE_BLOCK_PROC | FEATURE_I2C_BLOCK_READ	| \
+#define FEATURES_ICH5	(FEATURE_BLOCK_PROC | 				  \
 			 FEATURE_IRQ | FEATURE_SMBUS_PEC		| \
 			 FEATURE_BLOCK_BUFFER | FEATURE_HOST_NOTIFY)
 #define FEATURES_ICH4	(FEATURE_SMBUS_PEC | FEATURE_BLOCK_BUFFER | \
-- 
2.34.1

Re: i801: I2C block read should be usable also on ICH/ICH0/ICH2/ICH3/ICH4

[Heiner Kallweit]

On 06.12.2021 22:18, Heiner Kallweit wrote:
> Currently we use the following feature definitions. However, according to
> the respective datasheets, also ICH/ICH0/ICH2/ICH3/ICH4 support I2C block
> read. The implementation we have should work also on these chip versions.
> 
Basically the same I found for interrupt support. It's available on all
chip versions. Any specific reason why driver uses interrupts from ICH5
only?


> The commit message of 6342064cad7a ("i2c-i801: Implement I2C block read
> support") states that i2c block read is supported from ICH5 only.
> This doesn't seem to be true. Or is this feature broken on older chip
> versions? 
> 
> To me it seems we could remove FEATURE_I2C_BLOCK_READ because all chip
> versions support this feature. Below is an experimental patch, for the
> ones with test hw. A test case could be to use decode-dimms that
> uses i2c block read to read the EEPROM content.
> 
> * Supports the following Intel I/O Controller Hubs (ICH):
>  *
>  *					I/O			Block	I2C
>  *					region	SMBus	Block	proc.	block
>  * Chip name			PCI ID	size	PEC	buffer	call	read
>  * ---------------------------------------------------------------------------
>  * 82801AA (ICH)		0x2413	16	no	no	no	no
>  * 82801AB (ICH0)		0x2423	16	no	no	no	no
>  * 82801BA (ICH2)		0x2443	16	no	no	no	no
>  * 82801CA (ICH3)		0x2483	32	soft	no	no	no
>  * 82801DB (ICH4)		0x24c3	32	hard	yes	no	no
>  * 82801E (ICH5)		0x24d3	32	hard	yes	yes	yes
> 
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 930c6edbe..fd9a19a80 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -294,7 +294,6 @@ struct i801_priv {
>  #define FEATURE_SMBUS_PEC	BIT(0)
>  #define FEATURE_BLOCK_BUFFER	BIT(1)
>  #define FEATURE_BLOCK_PROC	BIT(2)
> -#define FEATURE_I2C_BLOCK_READ	BIT(3)
>  #define FEATURE_IRQ		BIT(4)
>  #define FEATURE_HOST_NOTIFY	BIT(5)
>  /* Not really a feature, but it's convenient to handle it as such */
> @@ -780,10 +779,6 @@ static int i801_block_transaction(struct i801_priv *priv, union i2c_smbus_data *
>  			pci_read_config_byte(priv->pci_dev, SMBHSTCFG, &hostc);
>  			pci_write_config_byte(priv->pci_dev, SMBHSTCFG,
>  					      hostc | SMBHSTCFG_I2C_EN);
> -		} else if (!(priv->features & FEATURE_I2C_BLOCK_READ)) {
> -			dev_err(&priv->pci_dev->dev,
> -				"I2C block read is unsupported!\n");
> -			return -EOPNOTSUPP;
>  		}
>  	}
>  
> @@ -956,11 +951,10 @@ static u32 i801_func(struct i2c_adapter *adapter)
>  	return I2C_FUNC_SMBUS_QUICK | I2C_FUNC_SMBUS_BYTE |
>  	       I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA |
>  	       I2C_FUNC_SMBUS_BLOCK_DATA | I2C_FUNC_SMBUS_WRITE_I2C_BLOCK |
> +	       I2C_FUNC_SMBUS_READ_I2C_BLOCK |
>  	       ((priv->features & FEATURE_SMBUS_PEC) ? I2C_FUNC_SMBUS_PEC : 0) |
>  	       ((priv->features & FEATURE_BLOCK_PROC) ?
>  		I2C_FUNC_SMBUS_BLOCK_PROC_CALL : 0) |
> -	       ((priv->features & FEATURE_I2C_BLOCK_READ) ?
> -		I2C_FUNC_SMBUS_READ_I2C_BLOCK : 0) |
>  	       ((priv->features & FEATURE_HOST_NOTIFY) ?
>  		I2C_FUNC_SMBUS_HOST_NOTIFY : 0);
>  }
> @@ -997,7 +991,7 @@ static const struct i2c_algorithm smbus_algorithm = {
>  	.functionality	= i801_func,
>  };
>  
> -#define FEATURES_ICH5	(FEATURE_BLOCK_PROC | FEATURE_I2C_BLOCK_READ	| \
> +#define FEATURES_ICH5	(FEATURE_BLOCK_PROC | 				  \
>  			 FEATURE_IRQ | FEATURE_SMBUS_PEC		| \
>  			 FEATURE_BLOCK_BUFFER | FEATURE_HOST_NOTIFY)
>  #define FEATURES_ICH4	(FEATURE_SMBUS_PEC | FEATURE_BLOCK_BUFFER | \

Re: i801: I2C block read should be usable also on ICH/ICH0/ICH2/ICH3/ICH4

[Wolfram Sang]

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

On Mon, Dec 06, 2021 at 10:18:49PM +0100, Heiner Kallweit wrote:
> Currently we use the following feature definitions. However, according to
> the respective datasheets, also ICH/ICH0/ICH2/ICH3/ICH4 support I2C block
> read. The implementation we have should work also on these chip versions.
> 
> The commit message of 6342064cad7a ("i2c-i801: Implement I2C block read
> support") states that i2c block read is supported from ICH5 only.
> This doesn't seem to be true. Or is this feature broken on older chip
> versions? 
> 
> To me it seems we could remove FEATURE_I2C_BLOCK_READ because all chip
> versions support this feature. Below is an experimental patch, for the
> ones with test hw. A test case could be to use decode-dimms that
> uses i2c block read to read the EEPROM content.
> 
> * Supports the following Intel I/O Controller Hubs (ICH):
>  *
>  *					I/O			Block	I2C
>  *					region	SMBus	Block	proc.	block
>  * Chip name			PCI ID	size	PEC	buffer	call	read
>  * ---------------------------------------------------------------------------
>  * 82801AA (ICH)		0x2413	16	no	no	no	no
>  * 82801AB (ICH0)		0x2423	16	no	no	no	no
>  * 82801BA (ICH2)		0x2443	16	no	no	no	no
>  * 82801CA (ICH3)		0x2483	32	soft	no	no	no
>  * 82801DB (ICH4)		0x24c3	32	hard	yes	no	no
>  * 82801E (ICH5)		0x24d3	32	hard	yes	yes	yes

Any reason you skipped this patch in your latest series?


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

Re: i801: I2C block read should be usable also on ICH/ICH0/ICH2/ICH3/ICH4

[Heiner Kallweit]

On 17.04.2022 04:27, Wolfram Sang wrote:
> On Mon, Dec 06, 2021 at 10:18:49PM +0100, Heiner Kallweit wrote:
>> Currently we use the following feature definitions. However, according to
>> the respective datasheets, also ICH/ICH0/ICH2/ICH3/ICH4 support I2C block
>> read. The implementation we have should work also on these chip versions.
>>
>> The commit message of 6342064cad7a ("i2c-i801: Implement I2C block read
>> support") states that i2c block read is supported from ICH5 only.
>> This doesn't seem to be true. Or is this feature broken on older chip
>> versions? 
>>
>> To me it seems we could remove FEATURE_I2C_BLOCK_READ because all chip
>> versions support this feature. Below is an experimental patch, for the
>> ones with test hw. A test case could be to use decode-dimms that
>> uses i2c block read to read the EEPROM content.
>>
>> * Supports the following Intel I/O Controller Hubs (ICH):
>>  *
>>  *					I/O			Block	I2C
>>  *					region	SMBus	Block	proc.	block
>>  * Chip name			PCI ID	size	PEC	buffer	call	read
>>  * ---------------------------------------------------------------------------
>>  * 82801AA (ICH)		0x2413	16	no	no	no	no
>>  * 82801AB (ICH0)		0x2423	16	no	no	no	no
>>  * 82801BA (ICH2)		0x2443	16	no	no	no	no
>>  * 82801CA (ICH3)		0x2483	32	soft	no	no	no
>>  * 82801DB (ICH4)		0x24c3	32	hard	yes	no	no
>>  * 82801E (ICH5)		0x24d3	32	hard	yes	yes	yes
> 
> Any reason you skipped this patch in your latest series?
> 
Supporting I2C block read on all chip versions is part of patch 4 in the
series. I didn't remove feature flag FEATURE_I2C_BLOCK_READ yet to give users
the chance to disable the feature via module parameter in case of problems.