Re: [PATCH v2 2/4] irqchip: irq-eyeq5-msi: add EyeQ5 PCIe MSI controller driver

[Gregory CLEMENT <gregory.clement@bootlin.com>]

Hello Thomas,

> On Mon, Nov 02 2026 at 17:50, Gregory CLEMENT wrote:
>> +static void __eyeq_msi_mask_irq(struct eyeq_msi *msi, unsigned int bit)
>> +{
>> +	u16 value;
>> +
>> +	lockdep_assert_held(&msi->lock);
>> +
>> +	value = readw(msi->gcsr_base + GCSR_IRQMASK);
>> +	value &= ~FIELD_PREP(GCSR_IRQMASK_MASK, BIT(bit));
>> +	writew(value, msi->gcsr_base + GCSR_IRQMASK);
>> +}
>> +
>> +static void __eyeq_msi_unmask_irq(struct eyeq_msi *msi, unsigned int bit)
>> +{
>> +	u16 value;
>> +
>> +	lockdep_assert_held(&msi->lock);
>> +
>> +	value = readw(msi->gcsr_base + GCSR_IRQMASK);
>> +	value |= FIELD_PREP(GCSR_IRQMASK_MASK, BIT(bit));
>> +	writew(value, msi->gcsr_base + GCSR_IRQMASK);
>> +}
>> +
>> +static void eyeq_msi_mask_irq(struct irq_data *data)
>> +{
>> +	struct eyeq_msi *msi = irq_data_get_irq_chip_data(data);
>> +
>> +	guard(raw_spinlock_irqsave)(&msi->lock);
>
> What's the reason for ...irqsave() here? The mask/unmask callbacks are
> invoked with the interrupt descriptor lock held and interrupts disabled,
> so a plain raw_spinlock() is sufficient, no?


Indeed we can just use a raw_spinlock() .

>
>> +static void eyeq_msi_isr(struct irq_desc *desc)
>> +{
>> +	struct irq_chip *parent_chip = irq_desc_get_chip(desc);
>> +	struct eyeq_msi *msi = irq_desc_get_handler_data(desc);
>> +	void __iomem *status_reg;
>> +	unsigned long pending;
>> +	unsigned int bit;
>> +
>> +	chained_irq_enter(parent_chip, desc);
>
> https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?h=irq/drivers&id=6cf0d999a18e799965cf2cf4a0157e257c805946

It is not yet in v7.4-rc1,but I can rebase on tip/irq-drivers branch to
use it.

>
>> +static int eyeq_msi_domain_alloc(struct irq_domain *domain, unsigned int virq,
>> +				 unsigned int nr_irqs, void *args)
>> +{
>> +	DECLARE_BITMAP(reserved, EYEQ_MSI_MAX_VECTORS);
>> +	struct eyeq_msi *msi = domain->host_data;
>> +	unsigned long bit = 0;
>> +	unsigned int i;
>> +
>> +	bitmap_zero(reserved, EYEQ_MSI_MAX_VECTORS);
>> +
>> +	scoped_guard(raw_spinlock_irqsave, &msi->lock) {
>> +		for (i = 0; i < nr_irqs; i++) {
>
> This does not support MULTI_MSI, so nr_irqs is always 1. That allows to
> simplify both alloc() and free() by a lot, no?

Indeed you are right again. I will simplify it based on this.

>
>> +#define EYEQ_MSI_FLAGS_REQUIRED			\
>> +	(MSI_FLAG_USE_DEF_DOM_OPS |		\
>> +	 MSI_FLAG_USE_DEF_CHIP_OPS |		\
>> +	 MSI_FLAG_PCI_MSI_MASK_PARENT)
>
> Just a cosmetic nit:
>
> #define EYEQ_MSI_FLAGS_REQUIRED		(MSI_FLAG_USE_DEF_DOM_OPS |	\
> 					 MSI_FLAG_USE_DEF_CHIP_OPS |	\
> 					 MSI_FLAG_PCI_MSI_MASK_PARENT)
>
> is easier to read. But no strong opinion.

OK for me.
>
>> +#define EYEQ_MSI_FLAGS_SUPPORTED		\
>> +	(MSI_FLAG_PCI_MSIX |			\
>> +	 MSI_GENERIC_FLAGS_MASK)
>> +
>> +static const struct msi_parent_ops eyeq_msi_parent_ops = {
>> +	.required_flags		= EYEQ_MSI_FLAGS_REQUIRED,
>> +	.supported_flags	= EYEQ_MSI_FLAGS_SUPPORTED,
>> +	.bus_select_token	= DOMAIN_BUS_NEXUS,
>> +	.bus_select_mask	= MATCH_PCI_MSI,
>> +	.prefix			= "EYEQ-",
>> +	.init_dev_msi_info	= msi_lib_init_dev_msi_info,
>> +};
>> +
>> +/*
>> + * In endpoint mode the snooper is a platform MSI controller: the host writes
>> + * the doorbell and the snooper raises a local interrupt. Expose it as a
>> + * platform MSI parent so endpoint functions can allocate doorbells.
>> + */
>> +static const struct msi_parent_ops eyeq_msi_platform_parent_ops = {
>> +	.required_flags		= EYEQ_MSI_FLAGS_REQUIRED,
>> +	.supported_flags	= EYEQ_MSI_FLAGS_SUPPORTED,
>> +	.bus_select_token	= DOMAIN_BUS_NEXUS,
>> +	.bus_select_mask	= MATCH_PLATFORM_MSI,
>> +	.prefix			= "EYEQ-",
>
> It might be sensible to use a distinct prefix here, e.g. 'EYEQ-P-' or
> so.

OK

>
>> +static int eyeq_msi_create_domain(struct eyeq_msi *msi, struct device_node *node, bool is_rc)
>
> __init ?
>
>> +static int eyeq_msi_parse_vector_region(struct eyeq_msi *msi, struct device_node *node)
>
> __init ?
>
>> +static int eyeq_msi_parse_dt(struct eyeq_msi *msi, struct device_node *node)
>
> __init ?
>
>> +static void eyeq_msi_program_hardware(struct eyeq_msi *msi, bool is_rc)
>
> __init ?

The __init annotations no longer apply because v3 converted the driver
to a platform driver, so those helpers can run after init memory is
freed.


>
>> +
>> +static int __init eyeq_msi_of_init(struct device_node *node, struct device_node *parent)
>> +{
>
> Also please check the sashiko response vs. handle_simple_irq() and the
> affinity mechanism.

As you pointed on my v3 it was already taken into account.

I will send a v4 based on your review and asked to Vladimir if he can
also review it.

Thanks!

Gregory

-- 
Grégory CLEMENT, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com