On Tue, Aug 13 2024 at 15:43, Kevin Chen wrote:
> There are 10 interrupt source of soc0_intc in CPU die INTC.
> 1. 6 interrupt sources in IO die of soc1_intc0~soc1_intc5.
> 2. 2 interrupt sources in LTPI of ltpi0_intc0 and ltpi0_intc1.
> 3. 2 interrupt sources in LTPI of ltpi1_intc0 and ltpi1_intc1.
> Request GIC interrupt to check each bit in status register to do next
> level INTC handler.
>
> In next level INTC handler of IO die or LTPI INTC using soc1_intcX combining
> 32 interrupt sources into soc0_intc11 in CPU die. In soc1_intcX, handler
> would check 32 bit of status register to do the requested device
> handler.
I can't figure out what this word salad is trying to tell me. Nothing in
the code does any combining. The handler reads the very same
INTC_INT_STATUS_REG.
>
This lacks a Signed-off-by: tag. See Documentation/process/
> ---
> drivers/irqchip/Makefile | 1 +
> drivers/irqchip/irq-aspeed-intc.c | 198 ++++++++++++++++++++++++++++++
> +
> +#define INTC_INT_ENABLE_REG 0x00
> +#define INTC_INT_STATUS_REG 0x04
> +
> +struct aspeed_intc_ic {
> + void __iomem *base;
> + raw_spinlock_t gic_lock;
> + raw_spinlock_t intc_lock;
> + struct irq_domain *irq_domain;
> +};
> +
> +static void aspeed_intc_ic_irq_handler(struct irq_desc *desc)
> +{
> + struct aspeed_intc_ic *intc_ic = irq_desc_get_handler_data(desc);
> + struct irq_chip *chip = irq_desc_get_chip(desc);
> + unsigned long bit, status, flags;
> +
> + chained_irq_enter(chip, desc);
> +
> + raw_spin_lock_irqsave(&intc_ic->gic_lock, flags);
There is no point for irqsave(). This code is invoked with interrupts
disabled and please convert to:
scoped_guard(raw_spinlock, &intc_ic->gic_lock) {
> + status = readl(intc_ic->base + INTC_INT_STATUS_REG);
> + for_each_set_bit(bit, &status, 32) {
Please use a define and not a hardcoded number.
> + generic_handle_domain_irq(intc_ic->irq_domain, bit);
> + writel(BIT(bit), intc_ic->base + INTC_INT_STATUS_REG);
> + }
}
> + raw_spin_unlock_irqrestore(&intc_ic->gic_lock, flags);
> +
> + chained_irq_exit(chip, desc);
> +}
> +
> +static void aspeed_intc_irq_mask(struct irq_data *data)
> +{
> + struct aspeed_intc_ic *intc_ic = irq_data_get_irq_chip_data(data);
> + unsigned int mask = readl(intc_ic->base + INTC_INT_ENABLE_REG) & ~BIT(data->hwirq);
> + unsigned long flags;
Invoked with interrupts disabled too.
> + raw_spin_lock_irqsave(&intc_ic->intc_lock, flags);
> + writel(mask, intc_ic->base + INTC_INT_ENABLE_REG);
> + raw_spin_unlock_irqrestore(&intc_ic->intc_lock, flags);
guard(raw_spinlock)(&intc_ic->intc_lock);
writel(mask, intc_ic->base + INTC_INT_ENABLE_REG);
> +}
> +
> +static void aspeed_intc_irq_unmask(struct irq_data *data)
> +{
> + struct aspeed_intc_ic *intc_ic = irq_data_get_irq_chip_data(data);
> + unsigned int unmask = readl(intc_ic->base + INTC_INT_ENABLE_REG) | BIT(data->hwirq);
> + unsigned long flags;
Ditto.
> + raw_spin_lock_irqsave(&intc_ic->intc_lock, flags);
> + writel(unmask, intc_ic->base + INTC_INT_ENABLE_REG);
> + raw_spin_unlock_irqrestore(&intc_ic->intc_lock, flags);
> +}
> +
> +static int aspeed_intc_irq_set_affinity(struct irq_data *data, const struct cpumask *dest,
> + bool force)
> +{
> + return -EINVAL;
> +}
No point for this stub, just leave irq_set_affinity uninitialized. The
core code checks that pointer for NULL. Aside of that this stub and the
assignment would need a #ifdef CONFIG_SMP guard.
> +static struct irq_chip aspeed_intc_chip = {
> + .name = "ASPEED INTC",
> + .irq_mask = aspeed_intc_irq_mask,
> + .irq_unmask = aspeed_intc_irq_unmask,
> + .irq_set_affinity = aspeed_intc_irq_set_affinity,
> +};
> +
> +static int aspeed_intc_ic_map_irq_domain(struct irq_domain *domain, unsigned int irq,
> + irq_hw_number_t hwirq)
> +{
> + irq_set_chip_and_handler(irq, &aspeed_intc_chip, handle_level_irq);
> + irq_set_chip_data(irq, domain->host_data);
> +
> + return 0;
> +}
> +
> +static const struct irq_domain_ops aspeed_intc_ic_irq_domain_ops = {
> + .map = aspeed_intc_ic_map_irq_domain,
.map = aspeed_intc_ic_map_irq_domain,
> +};
> +
> +static int __init aspeed_intc_ic_of_init(struct device_node *node, struct device_node *parent)
> +{
> + struct aspeed_intc_ic *intc_ic;
> + int ret = 0;
> + int irq;
int irq, ret;
No point in initializing ret.
> + intc_ic = kzalloc(sizeof(*intc_ic), GFP_KERNEL);
> + if (!intc_ic)
> + return -ENOMEM;
> +
> + intc_ic->base = of_iomap(node, 0);
> + if (!intc_ic->base) {
> + pr_err("Failed to iomap intc_ic base\n");
> + ret = -ENOMEM;
> + goto err_free_ic;
> + }
> + writel(0xffffffff, intc_ic->base + INTC_INT_STATUS_REG);
> + writel(0x0, intc_ic->base + INTC_INT_ENABLE_REG);
> +
> + irq = irq_of_parse_and_map(node, 0);
> + if (!irq) {
> + pr_err("Failed to get irq number\n");
> + ret = -EINVAL;
> + goto err_iounmap;
> + }
> +
> + intc_ic->irq_domain = irq_domain_add_linear(node, 32,
> + &aspeed_intc_ic_irq_domain_ops, intc_ic);
> + if (!intc_ic->irq_domain) {
> + ret = -ENOMEM;
> + goto err_iounmap;
> + }
> +
> + raw_spin_lock_init(&intc_ic->gic_lock);
> + raw_spin_lock_init(&intc_ic->intc_lock);
> +
> + intc_ic->irq_domain->name = "aspeed-intc-domain";
See above.
> + irq_set_chained_handler_and_data(irq,
> + aspeed_intc_ic_irq_handler, intc_ic);
> +
> + return 0;
> +
> +err_iounmap:
> + iounmap(intc_ic->base);
> +err_free_ic:
> + kfree(intc_ic);
> + return ret;
> +}
> +
> +static int __init aspeed_intc_ic_of_init_v2(struct device_node *node,
> + struct device_node *parent)
> +{
> + struct aspeed_intc_ic *intc_ic;
> + int ret = 0;
> + int irq, i;
> +
> + intc_ic = kzalloc(sizeof(*intc_ic), GFP_KERNEL);
> + if (!intc_ic)
> + return -ENOMEM;
> +
> + intc_ic->base = of_iomap(node, 0);
> + if (!intc_ic->base) {
> + pr_err("Failed to iomap intc_ic base\n");
> + ret = -ENOMEM;
> + goto err_free_ic;
> + }
> + writel(0xffffffff, intc_ic->base + INTC_INT_STATUS_REG);
> + writel(0x0, intc_ic->base + INTC_INT_ENABLE_REG);
> +
> + intc_ic->irq_domain = irq_domain_add_linear(node, 32,
> + &aspeed_intc_ic_irq_domain_ops, intc_ic);
> + if (!intc_ic->irq_domain) {
> + ret = -ENOMEM;
> + goto err_iounmap;
> + }
> +
> + raw_spin_lock_init(&intc_ic->gic_lock);
> + raw_spin_lock_init(&intc_ic->intc_lock);
> +
> + intc_ic->irq_domain->name = "aspeed-intc-domain";
So up to this point aspeed_intc_ic_of_init_v2() is a verbatim copy of
aspeed_intc_ic_of_init(). Why can't you reuse that function? It's not
rocket science to make that work.
> + for (i = 0; i < of_irq_count(node); i++) {
> + irq = irq_of_parse_and_map(node, i);
> + if (!irq) {
> + pr_err("Failed to get irq number\n");
> + ret = -EINVAL;
> + goto err_iounmap;
Assume #0 and #1 succeed. #2 fails and leaves the chained handlers and
the irqdomain around, but then unmaps the base and frees the data which
the handler and the domain code needs. Seriously?
> + } else {
Pointless else as the if clause terminates with a goto.
> + irq_set_chained_handler_and_data(irq, aspeed_intc_ic_irq_handler, intc_ic);
So if I understand the code correctly then the hardware coalesces the
pending bits into a single status register, but depending on which part
of the SoC raised the interrupt one of the demultiplex interrupts is
raised in the GIC.
Any of those demultiplex interrupt handles _all_ pending bits and
therefore you need gic_lock to serialize them, right?
Thanks,
tglx
