lkml.org 
[lkml]   [2018]   [Jun]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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

\
 
 \ /
  Last update: 2018-06-23 10:57    [W:0.117 / U:1.236 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site