Re: [PATCH v4 2/3] drm/xe/i2c: Fix the interrupt handling

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

On Mon, Jul 13, 2026 at 05:56:00PM +0200, Heikki Krogerus wrote:
> The platforms that support the interrupt from the I2C
> adapter can not handle the amount of interrupts the adapter
> generates because of the way the IRQ is routed in the
> hardware. The I2C controller driver has to be kept in
> polling mode because of that.
> 
> The AMC MCU can still generate critical alerts that have to
> be handled. The interrupt from SMBus Alert is left enabled
> and handled separately in the Xe. The alerts from the AMC
> will cause the device to be declared wedged for now.

...

> +static void xe_amc_work(struct work_struct *work)
> +{
> +	struct xe_amc *amc = from_work(amc, work, work);
> +	struct i2c_client *client = amc->i2c->client[XE_I2C_CLIENT_AMC];
> +	const struct amc_request *request = &amc_get_alert_reason;
> +	struct amc_response response;
> +	int ret;
> +
> +	ret = i2c_master_send(client, (u8 *)request, sizeof(*request));
> +	if (ret < 0) {
> +		dev_err(&client->dev, "failed to send request (%d)\n", ret);
> +		return;
> +	}
> +
> +	fsleep(20 * USEC_PER_MSEC);

Nit: Probably worth an explanation.

> +	ret = i2c_master_recv(client, (u8 *)&response, sizeof(response));
> +	if (ret < 0) {
> +		dev_err(&client->dev, "failed to read response (%d)\n", ret);
> +		return;
> +	}
> +
> +	if (response.header.len == 0) {

Nit: Perhaps !response.header.len?

> +		dev_err(&client->dev, "empty response from AMC\n");
> +		return;
> +	}
> +
> +	if (response.header.command != request->header.command ||

Curious, what about the rest of the header? Would it be any different?

> +	    memcmp(&response.message, &request->message, sizeof(struct amc_message))) {
> +		dev_err(&client->dev, "response does not match the request\n");
> +		return;
> +	}
> +
> +	if (response.error) {
> +		dev_err(&client->dev, "AMC error 0x%02x\n", response.error);
> +		return;
> +	}
> +
> +	dev_dbg(&client->dev, "%s: Alert reason: %d\n", __func__, response.value);

See below [1].

> +	switch (response.value) {
> +	case AMC_ALERT_FW_DOWNLOAD:
> +	case AMC_ALERT_THERMAL_TRIP:
> +	case AMC_ALERT_OOB_REQUEST:
> +	case AMC_ALERT_OOB_RESET:
> +	case AMC_ALERT_CATERR:
> +		xe_device_declare_wedged(i2c_client_to_xe_device(client));
> +		break;
> +	default:
> +		break;
> +	}
> +}

...

> @@ -181,8 +187,7 @@ void xe_i2c_irq_handler(struct xe_device *xe, u32 master_ctl)
>  	if (!(master_ctl & I2C_IRQ) || !xe_i2c_irq_present(xe))
>  		return;
>  
> -	/* Forward interrupt to I2C adapter */
> -	generic_handle_irq_safe(xe->i2c->adapter_irq);
> +	xe_i2c_handle_smbus_alert(xe->i2c);

[1] Can we move the below re-assert code to wq now? Or do you suspect any
side-effects?

>  	/* Deassert after I2C adapter clears the interrupt */
>  	xe_mmio_rmw32(mmio, I2C_CONFIG_CMD, 0, PCI_COMMAND_INTX_DISABLE);
> @@ -212,45 +217,6 @@ void xe_i2c_irq_postinstall(struct xe_device *xe)
>  	xe_mmio_rmw32(mmio, I2C_CONFIG_CMD, PCI_COMMAND_INTX_DISABLE, 0);
>  }

...

> -#define XE_I2C_MAX_CLIENTS		3
> -
>  #define XE_I2C_EP_COOKIE_DEVICE		0xde
>  
>  /* Endpoint Capabilities */
>  #define XE_I2C_EP_CAP_IRQ		BIT(0)
>  
> +enum XE_I2C_CLIENT {
> +	XE_I2C_CLIENT_AMC,
> +	XE_I2C_MAX_CLIENTS = 3,

I know it was already like this but I probably missed why do we have 3
(atleast from driver POV)?

Raag

> +};