[PATCH] irqchip: plic: Fix priority base offset

[PATCH] irqchip: plic: Fix priority base offset

[Alistair Francis]

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(-)

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);
 
-- 
2.21.0

[PATCH] irqchip: plic: Fix priority base offset

[Alistair Francis]

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(-)

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);
 
-- 
2.21.0


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH] irqchip: plic: Fix priority base offset

[Christoph Hellwig]

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.

Re: [PATCH] irqchip: plic: Fix priority base offset

[Christoph Hellwig]

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.

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH] irqchip: plic: Fix priority base offset

[Alistair Francis]

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

Re: [PATCH] irqchip: plic: Fix priority base offset

[Alistair Francis]

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH] irqchip: plic: Fix priority base offset

[Christoph Hellwig]

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.

Re: [PATCH] irqchip: plic: Fix priority base offset

[Christoph Hellwig]

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.

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH] irqchip: plic: Fix priority base offset

[Alistair Francis]

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

Re: [PATCH] irqchip: plic: Fix priority base offset

[Alistair Francis]

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv