Messages in this thread Patch in this message | | | Date | Thu, 21 Dec 2017 10:50:50 -0800 (PST) | Subject | Re: [patches] [PATCH] irqchip/riscv-plic: fix address alignment of the plic enable bits | From | Palmer Dabbelt <> |
| |
On Mon, 18 Dec 2017 23:44:38 PST (-0800), zongbox@gmail.com wrote: > When the interrupt sourece id > 31, the register address is not 4 byte > alignment. Arithmetic on void pointer has no size extension, it just > add the result of 'hwirq/32' directly. > > Signed-off-by: Zong Li <zongbox@gmail.com> > --- > drivers/irqchip/irq-riscv-plic.c | 15 +++++++++------ > 1 file changed, 9 insertions(+), 6 deletions(-) > > diff --git a/drivers/irqchip/irq-riscv-plic.c b/drivers/irqchip/irq-riscv-plic.c > index b5182b4..0a43763 100644 > --- a/drivers/irqchip/irq-riscv-plic.c > +++ b/drivers/irqchip/irq-riscv-plic.c > @@ -92,6 +92,8 @@ > */ > #define ENABLE_BASE 0x2000 > #define ENABLE_PER_HART 0x80 > +#define ENABLE_SOURCES 32 > +#define ENABLE_SOURCES_OFFSET 4 > > /* > * Each hart context has a set of control registers associated with it. Right > @@ -128,9 +130,10 @@ struct plic_data { > > /* Addressing helper functions. */ > static inline > -void __iomem *plic_enable_vector(struct plic_data *data, int contextid) > +void __iomem *plic_enable_vector(struct plic_data *data, int contextid, int hwirq) > { > - return data->reg + ENABLE_BASE + contextid * ENABLE_PER_HART; > + return data->reg + ENABLE_BASE + contextid * ENABLE_PER_HART + > + (hwirq / ENABLE_SOURCES) * ENABLE_SOURCES_OFFSET; > } > > static inline > @@ -172,8 +175,8 @@ void plic_complete(struct plic_data *data, int contextid, u32 claim) > /* Explicit interrupt masking. */ > static void plic_disable(struct plic_data *data, int contextid, int hwirq) > { > - void __iomem *reg = plic_enable_vector(data, contextid) + (hwirq / 32); > - u32 mask = ~(1 << (hwirq % 32)); > + void __iomem *reg = plic_enable_vector(data, contextid, hwirq); > + u32 mask = ~(1 << (hwirq % ENABLE_SOURCES)); > > spin_lock(&data->lock); > writel(readl(reg) & mask, reg); > @@ -182,8 +185,8 @@ static void plic_disable(struct plic_data *data, int contextid, int hwirq) > > static void plic_enable(struct plic_data *data, int contextid, int hwirq) > { > - void __iomem *reg = plic_enable_vector(data, contextid) + (hwirq / 32); > - u32 bit = 1 << (hwirq % 32); > + void __iomem *reg = plic_enable_vector(data, contextid, hwirq); > + u32 bit = 1 << (hwirq % ENABLE_SOURCES); > > spin_lock(&data->lock); > writel(readl(reg) | bit, reg);
Whoops, sorry about that. I think it's cleaner to use a 'u32 *' instead of 'void *' here, something more like this (which I haven't even compiled)
commit 2424a65ce079ecbe16fbedc704b88b6f04f8b97a Author: Palmer Dabbelt <palmer@sifive.com> Date: Wed Dec 20 19:31:41 2017 -0800
void* -> u32* in PLIC
diff --git a/drivers/irqchip/irq-riscv-plic.c b/drivers/irqchip/irq-riscv-plic.c index b5182b43b377..99e79713abc6 100644 --- a/drivers/irqchip/irq-riscv-plic.c +++ b/drivers/irqchip/irq-riscv-plic.c @@ -128,25 +128,25 @@ struct plic_data {
/* Addressing helper functions. */ static inline -void __iomem *plic_enable_vector(struct plic_data *data, int contextid) +u32 __iomem *plic_enable_vector(struct plic_data *data, int contextid) { return data->reg + ENABLE_BASE + contextid * ENABLE_PER_HART; }
static inline -void __iomem *plic_priority(struct plic_data *data, int hwirq) +u32 __iomem *plic_priority(struct plic_data *data, int hwirq) { return data->reg + PRIORITY_BASE + hwirq * PRIORITY_PER_ID; }
static inline -void __iomem *plic_hart_threshold(struct plic_data *data, int contextid) +u32 __iomem *plic_hart_threshold(struct plic_data *data, int contextid) { return data->reg + CONTEXT_BASE + CONTEXT_PER_HART * contextid + CONTEXT_THRESHOLD; }
static inline -void __iomem *plic_hart_claim(struct plic_data *data, int contextid) +u32 __iomem *plic_hart_claim(struct plic_data *data, int contextid) { return data->reg + CONTEXT_BASE + CONTEXT_PER_HART * contextid + CONTEXT_CLAIM; } @@ -172,7 +172,7 @@ void plic_complete(struct plic_data *data, int contextid, u32 claim) /* Explicit interrupt masking. */ static void plic_disable(struct plic_data *data, int contextid, int hwirq) { - void __iomem *reg = plic_enable_vector(data, contextid) + (hwirq / 32); + u32 __iomem *reg = plic_enable_vector(data, contextid) + (hwirq / 32); u32 mask = ~(1 << (hwirq % 32));
spin_lock(&data->lock); @@ -182,7 +182,7 @@ static void plic_disable(struct plic_data *data, int contextid, int hwirq)
static void plic_enable(struct plic_data *data, int contextid, int hwirq) { - void __iomem *reg = plic_enable_vector(data, contextid) + (hwirq / 32); + u32 __iomem *reg = plic_enable_vector(data, contextid) + (hwirq / 32); u32 bit = 1 << (hwirq % 32);
spin_lock(&data->lock); Note that the PLIC driver isn't upstream yet, and hasn't been reviewed because (as you've discovered) it's a bit of a mess. Thanks for the patch, I'll be sure to fix it up before sending out the PLIC driver for review.
| |