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

[Heikki Krogerus]

On Thu, Jul 16, 2026 at 07:37:37AM +0200, Raag Jadav wrote:
> 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.

Makes sense. I'll fix this.

> > +}
> 
> ...
> 
> > +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.

Okay, good. I wanted to ask you why we do that after the i2c adapter
interrupt is cleared instead of before that, but now it does not
matter :)

Thanks Raag!

-- 
heikki