Re: [PATCH v1 2/2] drm/xe/mcu_i2c: Take over control of the controller enabling

[Raag Jadav <raag.jadav@intel.com>]

On Mon, Jun 22, 2026 at 01:47:59PM +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.

...

>  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));
> +	switch (reg) {

Curious, should I expect DW_IC_INTR_MASK case here which skips the MMIO?

> +	case IC_ENABLE:
> +		*val = i2c->ic_enable;
> +		break;
> +	case IC_ENABLE_STATUS:
> +		*val = i2c->ic_enable & 1; /* NOTE: Checking only the enable bit */
> +		break;
> +	default:
> +		*val = xe_mmio_read32(i2c->mmio, XE_REG(reg + I2C_MEM_SPACE_OFFSET));
> +		break;
> +	}
>  
>  	return 0;
>  }
> @@ -273,8 +312,30 @@ static int xe_i2c_read(void *context, unsigned int reg, unsigned int *val)
>  static int xe_i2c_write(void *context, unsigned int reg, unsigned int val)
>  {
>  	struct xe_i2c *i2c = context;
> +	int ret;
>  
> -	xe_mmio_write32(i2c->mmio, XE_REG(reg + I2C_MEM_SPACE_OFFSET), val);
> +	switch (reg) {

Ditto.

Raag

> +	case IC_CON:
> +	case IC_TAR:
> +	case IC_SAR:
> +		/* Disable the controller. */
> +		ret = xe_i2c_disable(i2c);
> +		if (ret)
> +			return ret;
> +
> +		/* Write the register. */
> +		xe_mmio_write32(i2c->mmio, XE_REG(reg + I2C_MEM_SPACE_OFFSET), val);
> +
> +		/* Enable the controller. */
> +		xe_mmio_rmw32(i2c->mmio, XE_REG(IC_ENABLE + I2C_MEM_SPACE_OFFSET), 0, 1);
> +		break;
> +	case IC_ENABLE:
> +		i2c->ic_enable = val;
> +		break;
> +	default:
> +		xe_mmio_write32(i2c->mmio, XE_REG(reg + I2C_MEM_SPACE_OFFSET), val);
> +		break;
> +	}
>  
>  	return 0;
>  }