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

[Thomas Gleixner <tglx@kernel.org>]

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?

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

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

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

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

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

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

Thanks,

        tglx