Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755095AbdLUSvC (ORCPT ); Thu, 21 Dec 2017 13:51:02 -0500 Received: from mail-pl0-f68.google.com ([209.85.160.68]:33301 "EHLO mail-pl0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752678AbdLUSuw (ORCPT ); Thu, 21 Dec 2017 13:50:52 -0500 X-Google-Smtp-Source: ACJfBouNBpXJYz+5EkU+ZVgB1N+XyauIgZN5qVMhI+LweS7EGJxCNWytXRQHJqTmfYuFLdvoO0XvPw== Date: Thu, 21 Dec 2017 10:50:50 -0800 (PST) X-Google-Original-Date: Thu, 21 Dec 2017 10:50:26 PST (-0800) Subject: Re: [patches] [PATCH] irqchip/riscv-plic: fix address alignment of the plic enable bits In-Reply-To: <1513669478-32438-1-git-send-email-zongbox@gmail.com> CC: tglx@linutronix.de, jason@lakedaemon.net, marc.zyngier@arm.com, albert@sifive.com, linux-kernel@vger.kernel.org, patches@groups.riscv.org, greentime@andestech.com, zong@andestech.com, zongbox@gmail.com From: Palmer Dabbelt To: zongbox@gmail.com Message-ID: Mime-Version: 1.0 (MHng) Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4849 Lines: 124 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 > --- > 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 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.