| Message ID | 20190320223853.1209-1-alistair.francis@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | irqchip: plic: Fix priority base offset | expand |
On Wed, Mar 20, 2019 at 10:39:52PM +0000, Alistair Francis wrote: > According to the FU540 and E31 manuals the PLIC source priority > address starts at an offset of 0x04 and not 0x00. To aviod confusion > update the address and source offset to match the documentation. This > causes no difference in functionality. Well, it starts at 0x00, but the first one is reserved. If you think that is too confusing I'd rather throw in a comment explaining this fact rather than making the calculating more complicated.
On Wed, Mar 20, 2019 at 4:49 PM Christoph Hellwig <hch@infradead.org> wrote: > > On Wed, Mar 20, 2019 at 10:39:52PM +0000, Alistair Francis wrote: > > According to the FU540 and E31 manuals the PLIC source priority > > address starts at an offset of 0x04 and not 0x00. To aviod confusion > > update the address and source offset to match the documentation. This > > causes no difference in functionality. > > Well, it starts at 0x00, but the first one is reserved. If you think > that is too confusing I'd rather throw in a comment explaining this > fact rather than making the calculating more complicated. It doesn't mention that it starts at 0 when you look here: https://sifive.cdn.prismic.io/sifive%2F834354f0-08e6-423c-bf1f-0cb58ef14061_fu540-c000-v1.0.pdf (pg. 61). I agree that it's the same as starting as 0 and reserving the first one, but this means that the macros are actually at the wrong address and we need to remember not to use 0. This ends up forgotten and I have seen bugs in OpenSBI and QEMU as they have a similar implementation. It's possible some future code update will forget this as well. I think it's much clearer to define the correct values and just subtract 1, the calculation really isn't more complicated. Alistair
On Wed, Mar 20, 2019 at 05:04:58PM -0700, Alistair Francis wrote: > > Well, it starts at 0x00, but the first one is reserved. If you think > > that is too confusing I'd rather throw in a comment explaining this > > fact rather than making the calculating more complicated. > > It doesn't mention that it starts at 0 when you look here: > https://sifive.cdn.prismic.io/sifive%2F834354f0-08e6-423c-bf1f-0cb58ef14061_fu540-c000-v1.0.pdf It doesn't say that. But it is completely obvious from the map, and from how everything else works. In this case I think the documentation is simply written in a confusing way, and we need to fix it once we have an official riscv spec level documentation of this hardware.
On Fri, Mar 22, 2019 at 6:27 AM Christoph Hellwig <hch@infradead.org> wrote: > > On Wed, Mar 20, 2019 at 05:04:58PM -0700, Alistair Francis wrote: > > > Well, it starts at 0x00, but the first one is reserved. If you think > > > that is too confusing I'd rather throw in a comment explaining this > > > fact rather than making the calculating more complicated. > > > > It doesn't mention that it starts at 0 when you look here: > > https://sifive.cdn.prismic.io/sifive%2F834354f0-08e6-423c-bf1f-0cb58ef14061_fu540-c000-v1.0.pdf > > It doesn't say that. But it is completely obvious from the map, > and from how everything else works. In this case I think the > documentation is simply written in a confusing way, and we need to fix > it once we have an official riscv spec level documentation of this > hardware. I agree that the documentation is written in a confusing way. In saying that we make it even more confusing by not following the documentation and doing something different, which is what we are doing now. If the documentation changes in the future we can update the code to make the new documentation but at the moment I think it makes more sense to match the documentation. It makes it a lot easier to compare the code and the documentation when they match. Hopefully that can avoid and off-by-one index issues as we have seen recently. Alistair
diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c index cf755964f2f8..826e7293d608 100644 --- a/drivers/irqchip/irq-sifive-plic.c +++ b/drivers/irqchip/irq-sifive-plic.c @@ -35,7 +35,7 @@ * Each interrupt source has a priority register associated with it. * We always hardwire it to one in Linux. */ -#define PRIORITY_BASE 0 +#define PRIORITY_BASE 0x04 #define PRIORITY_PER_ID 4 /* @@ -88,7 +88,7 @@ static inline void plic_irq_toggle(const struct cpumask *mask, { int cpu; - writel(enable, plic_regs + PRIORITY_BASE + hwirq * PRIORITY_PER_ID); + writel(enable, plic_regs + PRIORITY_BASE + (hwirq - 1) * PRIORITY_PER_ID); for_each_cpu(cpu, mask) { struct plic_handler *handler = per_cpu_ptr(&plic_handlers, cpu);
According to the FU540 and E31 manuals the PLIC source priority address starts at an offset of 0x04 and not 0x00. To aviod confusion update the address and source offset to match the documentation. This causes no difference in functionality. Signed-off-by: Alistair Francis <alistair.francis@wdc.com> --- drivers/irqchip/irq-sifive-plic.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)