Re: [PATCH v7 3/3] drm/xe/i2c: Keep the i2c controller always enabled

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Tue, Aug 11, 2026 at 02:10:08PM +0200, Heikki Krogerus wrote:
> Some platforms make an assumption that the i2c controller's
> enabled state indicates also the power state of the
> controller. This can create a problem when the controller is
> in disabled state, because the hardware may assume
> incorrectly that it is then also in low-power state.
> 
> To fix this, the controller is kept enabled by taking over
> the IC_ENABLE register. The controller has to be disabled
> when the configuration is updated and when the target
> address or the slave address are assigned, so disabling it
> when IC_CON, IC_TAR or IC_SAR registers are programmed, and
> then re-enabling it again.
> 
> Fixes: f0e53aadd702 ("drm/xe: Support for I2C attached MCUs")
> Cc: stable@vger.kernel.org
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> ---
>  drivers/gpu/drm/xe/xe_i2c.c | 49 ++++++++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/xe/xe_i2c.h |  1 +
>  2 files changed, 49 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_i2c.c b/drivers/gpu/drm/xe/xe_i2c.c
> index 32767570e43d4..d8fa68206f418 100644
> --- a/drivers/gpu/drm/xe/xe_i2c.c
> +++ b/drivers/gpu/drm/xe/xe_i2c.c
> @@ -8,6 +8,7 @@
>  #include <drm/drm_print.h>
>  #include <linux/array_size.h>
>  #include <linux/container_of.h>
> +#include <linux/delay.h>
>  #include <linux/device.h>
>  #include <linux/err.h>
>  #include <linux/i2c.h>
> @@ -215,11 +216,40 @@ void xe_i2c_irq_postinstall(struct xe_device *xe)
>  	xe_mmio_rmw32(mmio, I2C_CONFIG_CMD, PCI_COMMAND_INTX_DISABLE, 0);
>  }
>  
> +/* See "Disabling DW_apb_i2c" in the DesignWare DW_abp_i2c databook. */
> +static void xe_i2c_disable(struct xe_i2c *i2c)
> +{
> +	int timeout = 100;
> +	u32 status;
> +
> +	xe_mmio_rmw32(i2c->mmio, I2C_REG(DW_IC_ENABLE), DW_IC_ENABLE_ENABLE, 0);
> +
> +	do {
> +		status = xe_mmio_read32(i2c->mmio, I2C_REG(DW_IC_ENABLE_STATUS));
> +		if (!(status & DW_IC_ENABLE_ENABLE))
> +			return;
> +		/* Can't sleep here. */
> +		udelay(25);
> +	} while (timeout--);
> +
> +	dev_warn(i2c->drm_dev, "timeout in disabling i2c adapter\n");
> +}
> +
>  static int xe_i2c_read(void *context, unsigned int reg, unsigned int *val)
>  {
>  	struct xe_i2c *i2c = context;
>  
> -	*val = xe_mmio_read32(i2c->mmio, XE_REG(reg + I2C_MEM_SPACE_OFFSET));
> +	*val = xe_mmio_read32(i2c->mmio, I2C_REG(reg));
> +
> +	switch (reg) {
> +	case DW_IC_ENABLE:
> +	case DW_IC_ENABLE_STATUS:
> +		FIELD_MODIFY(DW_IC_ENABLE_ENABLE, val,
> +			     i2c->ic_enable & DW_IC_ENABLE_ENABLE);
> +		break;
> +	default:
> +		break;
> +	}
>  
>  	return 0;
>  }
> @@ -229,6 +259,23 @@ static int xe_i2c_write(void *context, unsigned int reg, unsigned int val)
>  	struct xe_i2c *i2c = context;
>  
>  	switch (reg) {
> +	case DW_IC_CON:
> +	case DW_IC_TAR:
> +	case DW_IC_SAR:
> +		/* Disable the controller. */
> +		xe_i2c_disable(i2c);
> +
> +		/* Write the register. */
> +		xe_mmio_write32(i2c->mmio, I2C_REG(reg), val);
> +
> +		/* Enable the controller. */
> +		xe_mmio_rmw32(i2c->mmio, I2C_REG(DW_IC_ENABLE), 0, DW_IC_ENABLE_ENABLE);
> +		return 0;
> +	case DW_IC_ENABLE:
> +		i2c->ic_enable = val;
> +		/* Other fields can be updated except the enable bit. */
> +		val |= DW_IC_ENABLE_ENABLE;
> +		break;
>  	case DW_IC_SMBUS_INTR_MASK:
>  		/* Make sure the Alert is never masked. */
>  		val |= DW_IC_SMBUS_INTR_ALERT;
> diff --git a/drivers/gpu/drm/xe/xe_i2c.h b/drivers/gpu/drm/xe/xe_i2c.h
> index b200966b00484..d63adacfefe71 100644
> --- a/drivers/gpu/drm/xe/xe_i2c.h
> +++ b/drivers/gpu/drm/xe/xe_i2c.h
> @@ -37,6 +37,7 @@ struct xe_i2c {
>  	struct platform_device *pdev;
>  	struct i2c_adapter *adapter;
>  	struct i2c_client *client[XE_I2C_MAX_CLIENTS];
> +	unsigned int ic_enable;
>  
>  	struct notifier_block bus_notifier;
>  	struct work_struct work;
> -- 
> 2.50.1
>