Messages in this thread | | | Date | Sat, 23 Jun 2018 10:56:29 +0200 (CEST) | From | Thomas Gleixner <> | Subject | Re: [PATCH 3/3] irqchip: RISC-V Local Interrupt Controller Driver |
| |
On Fri, 22 Jun 2018, Palmer Dabbelt wrote: > +struct riscv_irq_data { > + struct irq_chip chip; > + struct irq_domain *domain; > + int hart; > + char name[20]; > +}; > +DEFINE_PER_CPU(struct riscv_irq_data, riscv_irq_data);
static ?
> +static void riscv_intc_irq(struct pt_regs *regs) > +{ > + struct pt_regs *old_regs = set_irq_regs(regs); > + struct irq_domain *domain; > + unsigned long cause = csr_read(scause); > + > + /* > + * The high order bit of the trap cause register is always set for > + * interrupts, which allows us to differentiate them from exceptions > + * quickly. The INTERRUPT_CAUSE_* macros don't contain that bit, so we > + * need to mask it off here. > + */ > + WARN_ON((cause & (1UL << (PTR_BITS - 1))) == 0);
So what's the point of continuing here?
> + cause = cause & ~(1UL << (PTR_BITS - 1));
Please define a proper CAUSE_MASK or such as this is really hard to read.
> +/* > + * On RISC-V systems local interrupts are masked or unmasked by writing the SIE > + * (Supervisor Interrupt Enable) CSR. As CSRs can only be written on the local > + * hart, these functions can only be called on the hart that corresponds to the > + * IRQ chip. They are only called internally to this module, so they BUG_ON if > + * this condition is violated rather than attempting to handle the error by > + * forwarding to the target hart, as that's already expected to have been done. > + */ > +static void riscv_irq_mask(struct irq_data *d) > +{ > + struct riscv_irq_data *data = irq_data_get_irq_chip_data(d); > + > + BUG_ON(smp_processor_id() != data->hart); > + csr_clear(sie, 1 << (long)d->hwirq);
What's the point of this type cast? Whether you shift by unsigned long or by long does not really matter, right? I'd rather expected to see 1U << d->hwirq
> +static int __init riscv_intc_init(struct device_node *node, struct device_node *parent) > +{ > + int hart; > + struct riscv_irq_data *data;
Nit. Reverse fir tree ordering please
struct riscv_irq_data *data; int hart;
Simpler to parse.
> + > + if (parent) > + return 0;
Hmm, that wants a comment because it's not clear why this is done for the casual reader.
> + > + hart = riscv_of_processor_hart(node->parent); > + if (hart < 0) > + return -EIO; > + > + data = &per_cpu(riscv_irq_data, hart); > + snprintf(data->name, sizeof(data->name), "riscv,cpu_intc,%d", hart); > + data->hart = hart; > + data->chip.name = data->name; > + data->chip.irq_mask = riscv_irq_mask; > + data->chip.irq_unmask = riscv_irq_unmask; > + data->chip.irq_enable = riscv_irq_enable; > + data->chip.irq_disable = riscv_irq_disable; > + data->domain = irq_domain_add_linear(node, PTR_BITS, > + &riscv_irqdomain_ops, data); > + if (!data->domain) > + goto error_add_linear; > + > + set_handle_irq(&riscv_intc_irq); > + > + pr_info("%s: %lu local interrupts mapped\n", data->name, PTR_BITS); > + return 0; > + > +error_add_linear: > + pr_warning("%s: unable to add IRQ domain\n", > + data->name);
One line please.
> + return -ENXIO;
Thanks,
tglx
| |