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

[Heikki Krogerus <heikki.krogerus@linux.intel.com>]

On Wed, Jul 15, 2026 at 08:26:22AM +0200, Raag Jadav wrote:
> 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.

Sure.

> > +	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?

OK.

> > +		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?

The command is the only field that matches. But that check is not
needed. The command is always the same. I'll drop that line.

> > +	    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?

I think that you know this better than I do. But at this point
interrupt is cleared, so why should we wait for the wq?

To play it safe, can we change this as a followup if necessary?

> >  	/* 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)?

That is the maximum number of clients these platforms can support.
The AMC address is actually at a fixed offset 1. I'll change this so
that XE_I2C_CLIENT_AMC matches the offset:

enum XE_I2C_CLIENT {
	XE_I2C_CLIENT_AMC = 1,
	XE_I2C_MAX_CLIENTS = 3,
};

That probable makes this a bit more clear (right?).

Thanks,

-- 
heikki