Re: [PATCH 2/2] irqchip: introduce TC9564 SoC MSIGEN support

[Alex Elder <elder@riscstar.com>]

On 11/8/26 3:17 PM, Thomas Gleixner wrote:
> On Mon, Oct 12 2026 at 14:02, Alex Elder wrote:
>> +config TC9564_MSIGEN
>> +	tristate "Toshiba TC9564 MSI distributor"
>> +	depends on TC9564_PCI
>> +	depends on GENERIC_IRQ_CHIP
> 
>    s/depends/select/
> 
> Otherwise this relies on some other driver to select GENERIC_IRQ_CHIP...

The reason this uses "depends on" rather than "select" is
to avoid a recursive dependency.

     error: recursive dependency detected!
         symbol GENERIC_IRQ_CHIP is selected by TC9564_MSIGEN
         symbol TC9564_MSIGEN depends on TC9564_PCI
         symbol TC9564_PCI depends on PCI_DYNAMIC_OF_NODES
         symbol PCI_DYNAMIC_OF_NODES depends on OF_IRQ
         symbol OF_IRQ depends on IRQ_DOMAIN
         symbol IRQ_DOMAIN is selected by GENERIC_IRQ_CHIP
     For a resolution refer to Documentation/kbuild/kconfig-language.rst

We already depend on TC9564_PCI (and use it as the default value).

GENERIC_IRQ_CHIP gets selected by IRQ_DOMAIN, which we get from
OF_IRQ, from PCI_DYNAMIC_OF_NODES, from TC9564_PCI.

Should I just drop "depends on GENERIC_IRQ_CHIP"?

> 
>> +/**
>> + * struct tc9564_msigen - Context for MSIGEN IRQ domain
>> + * @ioaddr:	Pointer to mapped MSIGEN memory
>> + * @irq:	IRQ number for MSIGEN
>> + */
>> +struct tc9564_msigen {
>> +	void __iomem *ioaddr;
>> +	unsigned int irq;
> 
> https://docs.kernel.org/process/maintainer-tip.html#struct-declarations-and-initializers

I'll fix the alignment for these.

>> +};
>> +
>> +#define HWIRQ_COUNT			25
>> +
>> +#define MSI_OUT_EN_OFFSET		0x0000
>> +#define MSI_MASK_CLR_OFFSET		0x000c
>> +#define MSI_MASK_VALUE			BIT(0)
>> +#define MSI_INT_STS_OFFSET		0x0010
>> +
>> +static void tc9564_msigen_irq_handler(struct irq_desc *desc)
>> +{
>> +	struct irq_domain *irq_domain = irq_desc_get_handler_data(desc);
>> +	struct irq_chip *chip = irq_desc_get_chip(desc);
>> +	struct irq_chip_generic *gc;
>> +	unsigned long status;
>> +	unsigned long hwirq;
>> +
>> +	gc = irq_get_domain_generic_chip(irq_domain, 0);
>> +
>> +	chained_irq_enter(chip, desc);
>> +
>> +	status = irq_reg_readl(gc, MSI_INT_STS_OFFSET);
>> +	for_each_set_bit(hwirq, &status, HWIRQ_COUNT)
>> +		generic_handle_domain_irq(irq_domain, hwirq);
> 
> You want to use generic_handle_demux_domain_irq(), which allows you to
> actually implement affinity settings for individual interrupts.

That sounds better, thanks for pointing it out.

>> +	/*
>> +	 * Clear the MSI flag. Most interrupts within TC9564 are level-high
>> +	 * type. If any interrupts are still asserted then clearing this flag
>> +	 * will cause the (edge-triggered) MSI to be regenerated.
>> +	 */
>> +	irq_reg_writel(gc, MSI_MASK_VALUE, MSI_MASK_CLR_OFFSET);
>> +
>> +	chained_irq_exit(chip, desc);
>> +}
>> +static int tc9564_msigen_irq_domain_init(struct irq_domain *irq_domain)
>> +{
>> +	struct tc9564_msigen *msigen = irq_domain->host_data;
>> +
>> +	irq_set_chained_handler_and_data(msigen->irq, tc9564_msigen_irq_handler,
>> +					 irq_domain);
> 
> No line break required. You have 100 characters. All over the place.

OK.  I normally stick with 80 columns but since you prefer 100
I'll adjust the formatting throughout this file to use that.

>> +/* Find the PCI device we're associated with (not necessarily the parent) */
> 
> What does that mean and why is this platform device driver dealing with
> a PCI device in the first place?

We have modeled this chip using PCI endpoint bus, which allows
a PCI endpoint to present a child bus that implements multiple
device functions via each of its BARs (or a single bus can
represent multiple BARs).  Device nodes within a pci-ep-bus
sub-node use a platform device that is matched by compatible
string.

This MSIGEN block collects interrupts from multiple sources
within the chip and translates them into MSIs delivered to
the host.  We know that this block is subordinate to a PCI
device.  If this were a direct child of the PCI device, we
would just look at the parent.  But because the pci-ep-bus
is in there, we need to go to the grandparent.  The code loops
as it travels up rather than assuming it's parent->parent.

(Daniel might have more to say on this--if so he'll chime in.)
> This really wants proper comments explaining this in the source _and_
> the change log of this patch. And no, your sloppy cover letter
> explanation does not cut it at all.

There are several distinct but interrelated drivers and it's
been a bit repetitive to explain some of these things.  I'll
incorporate better big-picture descriptions both in the code
and in the patch headers in the next version.

>> +static struct pci_dev *tc9564_msigen_find_pci_dev(struct device *dev)
> 
>> +static int tc9564_msigen_probe(struct platform_device *pdev)
>> +{
>> +	struct irq_domain_chip_generic_info dgc_info;
> 
> Struct is uninitialized.

You're right, these should have been zeroed.
> 
>> +	struct device *dev = &pdev->dev;
>> +	struct irq_domain *irq_domain;
>> +	struct tc9564_msigen *msigen;
>> +	struct irq_domain_info info;
> 
> Ditto. Look at other drivers how this is done correctly.
> 
>> +	struct reset_control *reset;
>> +	struct pci_dev *pci_dev;
>> +	struct clk *clk;
>> +	int ret;
>> +
>> +	pci_dev = tc9564_msigen_find_pci_dev(dev);
>> +	if (IS_ERR(pci_dev))
>> +		return dev_err_probe(dev, PTR_ERR(pci_dev),
>> +				     "failed to find PCI device\n");
> 
> See bracket rules in the documentation I linked to or simply get rid of
> that line break. Same all over the place.

So you want brackets around the muti-line body of the if statement.

Switching to 100 column line widths might make this become moot.
In any case, understood.
>> +	dgc_info.name = "tc9564-msigen";
>> +	dgc_info.handler = handle_level_irq;
>> +	dgc_info.irqs_per_chip = HWIRQ_COUNT;
>> +	dgc_info.num_ct = 1;
>> +	dgc_info.init = tc9564_msigen_irq_chip_init;
>> +	dgc_info.exit = tc9564_msigen_irq_chip_exit;
>> +
>> +	info.fwnode = of_fwnode_handle(dev->of_node);
>> +	info.domain_flags = IRQ_DOMAIN_FLAG_DESTROY_GC;
>> +	info.size = HWIRQ_COUNT;
>> +	info.hwirq_max = HWIRQ_COUNT;
>> +	info.ops = &irq_generic_chip_ops;
>> +	info.host_data = msigen;
>> +	info.dgc_info = &dgc_info;
>> +	info.init = tc9564_msigen_irq_domain_init;
>> +	info.exit = tc9564_msigen_irq_domain_exit;
> 
> Both initializations (except for info.host_data) can be done at the
> declaration with a static initialization, which avoids that parts of the
> structs contain random stack content. No?

Yes.

I personally don't prefer initializing structs and unions
where they are defined as stack variable.  But you're right,
they need to get zeroed in that case.

Anyway, I'll do what you suggest.

> Please look through the sashiko comments as well.

Yes I think I responded to all issues in both of them,
indicating how (and whether) I intend to address each.

Thank you for your review.

					-Alex

> 
> Thanks,
> 
>          tglx