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

[Raag Jadav]

On Wed, Jul 15, 2026 at 05:31:52PM +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)
> +{
> +	const struct amc_request *request = &amc_get_alert_reason;
> +	struct xe_amc *amc = from_work(amc, work, work);
> +	struct amc_response response;
> +	struct i2c_client *client;
> +	int ret;
> +
> +	client = amc->i2c->client[XE_I2C_CLIENT_AMC];
> +	if (!client)
> +		goto out_reassert_interrupt;
> +
> +	ret = i2c_master_send(client, (u8 *)request, sizeof(*request));
> +	if (ret < 0) {
> +		dev_err(&client->dev, "failed to send request (%d)\n", ret);
> +		goto out_reassert_interrupt;
> +	}
> +
> +	/* AMC needs 20ms to generate the response. */
> +	fsleep(20 * USEC_PER_MSEC);
> +
> +	ret = i2c_master_recv(client, (u8 *)&response, sizeof(response));
> +	if (ret < 0) {
> +		dev_err(&client->dev, "failed to read response (%d)\n", ret);
> +		goto out_reassert_interrupt;
> +	}
> +
> +	if (!response.header.len) {
> +		dev_err(&client->dev, "empty response from AMC\n");
> +		goto out_reassert_interrupt;
> +	}
> +
> +	if (memcmp(&response.message, &request->message, sizeof(struct amc_message))) {
> +		dev_err(&client->dev, "response does not match the request\n");
> +		goto out_reassert_interrupt;
> +	}
> +
> +	if (response.error) {
> +		dev_err(&client->dev, "AMC error 0x%02x\n", response.error);
> +		goto out_reassert_interrupt;
> +	}

[1] See below.

> +	dev_dbg(&client->dev, "%s: Alert reason: %d\n", __func__, response.value);
> +
> +	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;
> +	}
> +
> +out_reassert_interrupt:
> +	xe_mmio_rmw32(amc->i2c->mmio, I2C_CONFIG_CMD, PCI_COMMAND_INTX_DISABLE, 0);

One of the rules of wedging is that it's a last resort and there shouldn't
be any hardware access afterwards, so I think a more suitable place for
re-assert is before[1] we process the response.

> +}

...

> +static void xe_i2c_handle_smbus_alert(struct xe_i2c *i2c)
> +{
> +	u32 stat;
> +
> +	stat = xe_mmio_read32(i2c->mmio, I2C_REG(DW_IC_SMBUS_INTR_STAT));
> +	if (!stat)
> +		return;
> +
> +	xe_mmio_write32(i2c->mmio, I2C_REG(DW_IC_CLR_SMBUS_INTR), stat);
> +
> +	xe_mmio_rmw32(i2c->mmio, I2C_CONFIG_CMD, 0, PCI_COMMAND_INTX_DISABLE);

As per updated arch, this is now taken care by the firmware and can be
dropped.

Raag

> +	if (stat & DW_IC_SMBUS_INTR_ALERT && i2c->amc)
> +		xe_amc_handle_alert(i2c);
> +	else
> +		xe_mmio_rmw32(i2c->mmio, I2C_CONFIG_CMD, PCI_COMMAND_INTX_DISABLE, 0);
> +}