Re: [PATCH 10/17] irqchip: New RISC-V PLIC Driver

[Palmer Dabbelt]

On Fri, 09 Jun 2017 06:47:48 PDT (-0700), will.dea...@arm.com wrote:
> On Wed, Jun 07, 2017 at 11:52:10AM +0100, Marc Zyngier wrote:
>> On 07/06/17 00:00, Palmer Dabbelt wrote:
>> > +static void plic_disable(struct plic_data *data, int i, int hwirq)
>> > +{
>> > +  struct plic_enable_context *enable = plic_enable_context(data, i);
>> > +
>> > +  atomic_and(~(1 << (hwirq % 32)), >mask[hwirq / 32]);
>>
>> This is still a device access, right? What does it mean to use the
>> atomic primitives on that? What are you racing against? I thought the
>> various context were private to an execution context...
>>
>> Adding Will and PeterZ to the CC list because they will probably have
>> their own views on this...
>
> atomic_* accesses to MMIO is almost certainly a bad idea. Is this atomic
> because you want to allow the function to run concurrently, or is it atomic
> because you want some guarantees from the endpoint's view?

Concurrency: while most operations on the PLIC are per-hart, in order to
disable an interrupt you have to go clear a bit for every hart.  The AMO ensure
the read-modify-write cycle didn't drop some other hart disabling a different
interrupt (as the bits are all in the same word).

I've done a big cleanup on this driver to avoid memory-mapped structures, use a
lock instead of AMOs, and use the FastEOI flow.  I'm getting close to getting
through all the code reviews for my v2 RISC-V patch set (which included this,
our arch support, and all our other driver patches).  I'm going to split out
the drivers for the next patch set, as it appears that's the better way to do
it.

Hopefully I can get the cleaned up patches out tonight.

Thanks!

Re: [PATCH 10/17] irqchip: New RISC-V PLIC Driver

[Palmer Dabbelt]

On Fri, 09 Jun 2017 06:47:48 PDT (-0700), will.dea...@arm.com wrote:
> On Wed, Jun 07, 2017 at 11:52:10AM +0100, Marc Zyngier wrote:
>> On 07/06/17 00:00, Palmer Dabbelt wrote:
>> > +static void plic_disable(struct plic_data *data, int i, int hwirq)
>> > +{
>> > +  struct plic_enable_context *enable = plic_enable_context(data, i);
>> > +
>> > +  atomic_and(~(1 << (hwirq % 32)), >mask[hwirq / 32]);
>>
>> This is still a device access, right? What does it mean to use the
>> atomic primitives on that? What are you racing against? I thought the
>> various context were private to an execution context...
>>
>> Adding Will and PeterZ to the CC list because they will probably have
>> their own views on this...
>
> atomic_* accesses to MMIO is almost certainly a bad idea. Is this atomic
> because you want to allow the function to run concurrently, or is it atomic
> because you want some guarantees from the endpoint's view?

Concurrency: while most operations on the PLIC are per-hart, in order to
disable an interrupt you have to go clear a bit for every hart.  The AMO ensure
the read-modify-write cycle didn't drop some other hart disabling a different
interrupt (as the bits are all in the same word).

I've done a big cleanup on this driver to avoid memory-mapped structures, use a
lock instead of AMOs, and use the FastEOI flow.  I'm getting close to getting
through all the code reviews for my v2 RISC-V patch set (which included this,
our arch support, and all our other driver patches).  I'm going to split out
the drivers for the next patch set, as it appears that's the better way to do
it.

Hopefully I can get the cleaned up patches out tonight.

Thanks!

Re: [PATCH 10/17] irqchip: New RISC-V PLIC Driver

[Palmer Dabbelt]

On Wed, 07 Jun 2017 03:52:10 PDT (-0700), marc.zyng...@arm.com wrote:
> Hi Palmer,
>
> On 07/06/17 00:00, Palmer Dabbelt wrote:
>> This patch adds a driver for the Platform Level Interrupt Controller
>> (PLIC) specified as part of the RISC-V supervisor level ISA manual.
>> The PLIC connocts global interrupt sources to the local interrupt
>> controller on each hart.  A PLIC is present on all RISC-V systems.
>>
>> Signed-off-by: Palmer Dabbelt 
>> ---
>>  drivers/irqchip/Kconfig  |  12 ++
>>  drivers/irqchip/Makefile |   1 +
>>  drivers/irqchip/irq-riscv-plic.c | 253 
>> +++
>>  3 files changed, 266 insertions(+)
>>  create mode 100644 drivers/irqchip/irq-riscv-plic.c
>>
>> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
>> index 478f8ace2664..2906d63934ef 100644
>> --- a/drivers/irqchip/Kconfig
>> +++ b/drivers/irqchip/Kconfig
>> @@ -301,3 +301,15 @@ config QCOM_IRQ_COMBINER
>>  help
>>Say yes here to add support for the IRQ combiner devices embedded
>>in Qualcomm Technologies chips.
>> +
>> +config RISCV_PLIC
>> +bool "Platform-Level Interrupt Controller"
>> +depends on RISCV
>> +default y
>> +help
>> +   This enables support for the PLIC chip found in standard RISC-V
>> +   systems. The PLIC is the top-most interrupt controller found in
>
> nit: this seems to slightly contradict what is being said in patch #8,
> where the HLIC is the top-level interrupt controller.

Sorry, I guess that was a bit confusing: by "top-level interrupt controller
found in the system" I meant the stuff outside the core complex -- in other
words, all the devices.  How does this sound instead?

This enables support for the PLIC chip found in standard RISC-V
systems.  The PLIC controls devices interrupts and connects them to
each core's local interrupt controller.  Aside from timer and
software interrupts, all other interrupt sources (MSI, GPIO, etc)
are subordinate to the PLIC.

https://github.com/riscv/riscv-linux/commit/2395b17a36c3cf5aabd9b019e1f8fc7efe67e9a0

>> +   the system, connected directly to the core complex. All other
>> +   interrupt sources (MSI, GPIO, etc) are subordinate to the PLIC.
>> +
>> +   If you don't know what to do here, say Y.
>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
>> index b64c59b838a0..bed94cc89146 100644
>> --- a/drivers/irqchip/Makefile
>> +++ b/drivers/irqchip/Makefile
>> @@ -76,3 +76,4 @@ obj-$(CONFIG_EZNPS_GIC)+= irq-eznps.o
>>  obj-$(CONFIG_ARCH_ASPEED)   += irq-aspeed-vic.o
>>  obj-$(CONFIG_STM32_EXTI)+= irq-stm32-exti.o
>>  obj-$(CONFIG_QCOM_IRQ_COMBINER) += qcom-irq-combiner.o
>> +obj-$(CONFIG_RISCV_PLIC)+= irq-riscv-plic.o
>> diff --git a/drivers/irqchip/irq-riscv-plic.c 
>> b/drivers/irqchip/irq-riscv-plic.c
>> new file mode 100644
>> index ..906c8a62a911
>> --- /dev/null
>> +++ b/drivers/irqchip/irq-riscv-plic.c
>> @@ -0,0 +1,253 @@
>> +/*
>> + * Copyright (C) 2017 SiFive
>> + *
>> + *   This program is free software; you can redistribute it and/or
>> + *   modify it under the terms of the GNU General Public License
>> + *   as published by the Free Software Foundation, version 2.
>> + *
>> + *   This program is distributed in the hope that it will be useful,
>> + *   but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + *   GNU General Public License for more details.
>> + */
>> +
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +/* From the RISC-V Privlidged Spec v1.10:
>> + *
>> + * Global interrupt sources are assigned small unsigned integer identifiers,
>> + * beginning at the value 1.  An interrupt ID of 0 is reserved to mean “no
>> + * interrupt”.  Interrupt identifiers are also used to break ties when 
>> two or
>> + * more interrupt sources have the same assigned priority. Smaller values of
>> + * interrupt ID take precedence over larger values of interrupt ID.
>> + *
>> + * It's not defined what the largest device ID is, so we're just fixing
>> + * MAX_DEVICES right here (which is named oddly, as there will never be a
>> + * device 0).
>> + */
>> +#define MAX_DEVICES 1024
>> +#define MAX_CONTEXTS15872
>
> If those are HW properties, they should probably come from the device
> tree instead of being hardcoded here.

Sorry, this is a bit confusing: the RISC-V supervisor spec (which is what this
comment was about) doesn't actually specify much about the PLIC, while this
driver is for the concrete 'riscv,plic0' device (which is SiFive's PLIC
implementation, and will be part of the platform specification).  Maybe this is
a better comment?

 * While the RISC-V supervisor spec doesn't define the maximum 

Re: [PATCH 10/17] irqchip: New RISC-V PLIC Driver

[Palmer Dabbelt]

On Wed, 07 Jun 2017 03:52:10 PDT (-0700), marc.zyng...@arm.com wrote:
> Hi Palmer,
>
> On 07/06/17 00:00, Palmer Dabbelt wrote:
>> This patch adds a driver for the Platform Level Interrupt Controller
>> (PLIC) specified as part of the RISC-V supervisor level ISA manual.
>> The PLIC connocts global interrupt sources to the local interrupt
>> controller on each hart.  A PLIC is present on all RISC-V systems.
>>
>> Signed-off-by: Palmer Dabbelt 
>> ---
>>  drivers/irqchip/Kconfig  |  12 ++
>>  drivers/irqchip/Makefile |   1 +
>>  drivers/irqchip/irq-riscv-plic.c | 253 
>> +++
>>  3 files changed, 266 insertions(+)
>>  create mode 100644 drivers/irqchip/irq-riscv-plic.c
>>
>> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
>> index 478f8ace2664..2906d63934ef 100644
>> --- a/drivers/irqchip/Kconfig
>> +++ b/drivers/irqchip/Kconfig
>> @@ -301,3 +301,15 @@ config QCOM_IRQ_COMBINER
>>  help
>>Say yes here to add support for the IRQ combiner devices embedded
>>in Qualcomm Technologies chips.
>> +
>> +config RISCV_PLIC
>> +bool "Platform-Level Interrupt Controller"
>> +depends on RISCV
>> +default y
>> +help
>> +   This enables support for the PLIC chip found in standard RISC-V
>> +   systems. The PLIC is the top-most interrupt controller found in
>
> nit: this seems to slightly contradict what is being said in patch #8,
> where the HLIC is the top-level interrupt controller.

Sorry, I guess that was a bit confusing: by "top-level interrupt controller
found in the system" I meant the stuff outside the core complex -- in other
words, all the devices.  How does this sound instead?

This enables support for the PLIC chip found in standard RISC-V
systems.  The PLIC controls devices interrupts and connects them to
each core's local interrupt controller.  Aside from timer and
software interrupts, all other interrupt sources (MSI, GPIO, etc)
are subordinate to the PLIC.

https://github.com/riscv/riscv-linux/commit/2395b17a36c3cf5aabd9b019e1f8fc7efe67e9a0

>> +   the system, connected directly to the core complex. All other
>> +   interrupt sources (MSI, GPIO, etc) are subordinate to the PLIC.
>> +
>> +   If you don't know what to do here, say Y.
>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
>> index b64c59b838a0..bed94cc89146 100644
>> --- a/drivers/irqchip/Makefile
>> +++ b/drivers/irqchip/Makefile
>> @@ -76,3 +76,4 @@ obj-$(CONFIG_EZNPS_GIC)+= irq-eznps.o
>>  obj-$(CONFIG_ARCH_ASPEED)   += irq-aspeed-vic.o
>>  obj-$(CONFIG_STM32_EXTI)+= irq-stm32-exti.o
>>  obj-$(CONFIG_QCOM_IRQ_COMBINER) += qcom-irq-combiner.o
>> +obj-$(CONFIG_RISCV_PLIC)+= irq-riscv-plic.o
>> diff --git a/drivers/irqchip/irq-riscv-plic.c 
>> b/drivers/irqchip/irq-riscv-plic.c
>> new file mode 100644
>> index ..906c8a62a911
>> --- /dev/null
>> +++ b/drivers/irqchip/irq-riscv-plic.c
>> @@ -0,0 +1,253 @@
>> +/*
>> + * Copyright (C) 2017 SiFive
>> + *
>> + *   This program is free software; you can redistribute it and/or
>> + *   modify it under the terms of the GNU General Public License
>> + *   as published by the Free Software Foundation, version 2.
>> + *
>> + *   This program is distributed in the hope that it will be useful,
>> + *   but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + *   GNU General Public License for more details.
>> + */
>> +
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +/* From the RISC-V Privlidged Spec v1.10:
>> + *
>> + * Global interrupt sources are assigned small unsigned integer identifiers,
>> + * beginning at the value 1.  An interrupt ID of 0 is reserved to mean “no
>> + * interrupt”.  Interrupt identifiers are also used to break ties when 
>> two or
>> + * more interrupt sources have the same assigned priority. Smaller values of
>> + * interrupt ID take precedence over larger values of interrupt ID.
>> + *
>> + * It's not defined what the largest device ID is, so we're just fixing
>> + * MAX_DEVICES right here (which is named oddly, as there will never be a
>> + * device 0).
>> + */
>> +#define MAX_DEVICES 1024
>> +#define MAX_CONTEXTS15872
>
> If those are HW properties, they should probably come from the device
> tree instead of being hardcoded here.

Sorry, this is a bit confusing: the RISC-V supervisor spec (which is what this
comment was about) doesn't actually specify much about the PLIC, while this
driver is for the concrete 'riscv,plic0' device (which is SiFive's PLIC
implementation, and will be part of the platform specification).  Maybe this is
a better comment?

 * While the RISC-V supervisor spec doesn't define the maximum number of
 * devices 

Re: [PATCH 10/17] irqchip: New RISC-V PLIC Driver

[Palmer Dabbelt]

On Wed, 07 Jun 2017 00:55:28 PDT (-0700), Arnd Bergmann wrote:
> On Wed, Jun 7, 2017 at 9:13 AM, Geert Uytterhoeven  
> wrote:
>>> +struct plic_enable_context {
>>> +   atomic_t mask[32]; // 32-bit * 32-entry
>>> +};
>
> You use many '//' style comments in this file, please change them all to '/* 
> */'
> for consistency with kernel coding style.

OK, I fixed them here and in all our other files that had them.

>>> +
>>> +struct plic_priority {
>>> +   u32 prio[MAX_DEVICES];
>>> +};
>>> +
>>> +struct plic_data {
>>> +   struct irq_chip chip;
>>> +   struct irq_domain   *domain;
>>> +   u32 ndev;
>>> +   void __iomem*reg;
>>> +   int handlers;
>>> +   struct plic_handler *handler;
>>> +   charname[30];
>>> +};
>>> +
>>> +struct plic_handler {
>>> +   struct plic_hart_context*context;
>>> +   struct plic_data*data;
>>> +};
>>> +
>>> +static inline
>>> +struct plic_hart_context *plic_hart_context(struct plic_data *data, size_t 
>>> i)
>>> +{
>>> +   return (struct plic_hart_context *)((char *)data->reg + HART_BASE + 
>>> HART_SIZE*i);
>>> +}
>
> 'data->reg' is an __iomem pointer, so when you build-test this with 'make 
> C=1',
> you should get a valid warning from sparse about an address space mismatch.
> Please address all the warning from sparse.

I didn't know about sparse.  I'll run it on our port and fix everything.

>>> +static void plic_disable(struct plic_data *data, int i, int hwirq)
>>> +{
>>> +   struct plic_enable_context *enable = plic_enable_context(data, i);
>>> +
>>> +   atomic_and(~(1 << (hwirq % 32)), >mask[hwirq / 32]);
>>> +}
>
> In particular, you must not do atomic operations on MMIO pointers.
> On most architectures these are explicitly disallowed and trap for
> a good reason, as the hardware implementation behind atomics tend
> to rely on the cache controller, while mmio registers are required
> to be uncached.

Sorry about that: the SiFive bus actually supports AMOs natively out to the
every device, even without caches, bit the RISC-V spec allows regions
to be marked as not supporting AMOs.  Sometimes a few SiFive-isms sneak in from
before the supervisor spec was written in this particular manner.  I've
converted this to a spinlock instead.

  
https://github.com/riscv/riscv-linux/commit/79b26ca800663399ea7d9dead73f3715deee1a99


>>> +   iowrite32(1, >prio[d->hwirq]);
>
> I would normally use 'readl' instead of 'iowrite32'. They may be the same
> on riscv, but they have slightly different meaning in portable drivers.

I assume you meant writel?  If so, that makes sense

  
https://github.com/riscv/riscv-linux/commit/45c968f1f068c35e0a5c8c90ba0776e7bdb6db78

Re: [PATCH 10/17] irqchip: New RISC-V PLIC Driver

[Palmer Dabbelt]

On Wed, 07 Jun 2017 00:55:28 PDT (-0700), Arnd Bergmann wrote:
> On Wed, Jun 7, 2017 at 9:13 AM, Geert Uytterhoeven  
> wrote:
>>> +struct plic_enable_context {
>>> +   atomic_t mask[32]; // 32-bit * 32-entry
>>> +};
>
> You use many '//' style comments in this file, please change them all to '/* 
> */'
> for consistency with kernel coding style.

OK, I fixed them here and in all our other files that had them.

>>> +
>>> +struct plic_priority {
>>> +   u32 prio[MAX_DEVICES];
>>> +};
>>> +
>>> +struct plic_data {
>>> +   struct irq_chip chip;
>>> +   struct irq_domain   *domain;
>>> +   u32 ndev;
>>> +   void __iomem*reg;
>>> +   int handlers;
>>> +   struct plic_handler *handler;
>>> +   charname[30];
>>> +};
>>> +
>>> +struct plic_handler {
>>> +   struct plic_hart_context*context;
>>> +   struct plic_data*data;
>>> +};
>>> +
>>> +static inline
>>> +struct plic_hart_context *plic_hart_context(struct plic_data *data, size_t 
>>> i)
>>> +{
>>> +   return (struct plic_hart_context *)((char *)data->reg + HART_BASE + 
>>> HART_SIZE*i);
>>> +}
>
> 'data->reg' is an __iomem pointer, so when you build-test this with 'make 
> C=1',
> you should get a valid warning from sparse about an address space mismatch.
> Please address all the warning from sparse.

I didn't know about sparse.  I'll run it on our port and fix everything.

>>> +static void plic_disable(struct plic_data *data, int i, int hwirq)
>>> +{
>>> +   struct plic_enable_context *enable = plic_enable_context(data, i);
>>> +
>>> +   atomic_and(~(1 << (hwirq % 32)), >mask[hwirq / 32]);
>>> +}
>
> In particular, you must not do atomic operations on MMIO pointers.
> On most architectures these are explicitly disallowed and trap for
> a good reason, as the hardware implementation behind atomics tend
> to rely on the cache controller, while mmio registers are required
> to be uncached.

Sorry about that: the SiFive bus actually supports AMOs natively out to the
every device, even without caches, bit the RISC-V spec allows regions
to be marked as not supporting AMOs.  Sometimes a few SiFive-isms sneak in from
before the supervisor spec was written in this particular manner.  I've
converted this to a spinlock instead.

  
https://github.com/riscv/riscv-linux/commit/79b26ca800663399ea7d9dead73f3715deee1a99


>>> +   iowrite32(1, >prio[d->hwirq]);
>
> I would normally use 'readl' instead of 'iowrite32'. They may be the same
> on riscv, but they have slightly different meaning in portable drivers.

I assume you meant writel?  If so, that makes sense

  
https://github.com/riscv/riscv-linux/commit/45c968f1f068c35e0a5c8c90ba0776e7bdb6db78

Re: [PATCH 10/17] irqchip: New RISC-V PLIC Driver

[Will Deacon]

On Wed, Jun 07, 2017 at 11:52:10AM +0100, Marc Zyngier wrote:
> On 07/06/17 00:00, Palmer Dabbelt wrote:
> > +static void plic_disable(struct plic_data *data, int i, int hwirq)
> > +{
> > +   struct plic_enable_context *enable = plic_enable_context(data, i);
> > +
> > +   atomic_and(~(1 << (hwirq % 32)), >mask[hwirq / 32]);
> 
> This is still a device access, right? What does it mean to use the
> atomic primitives on that? What are you racing against? I thought the
> various context were private to an execution context...
> 
> Adding Will and PeterZ to the CC list because they will probably have
> their own views on this...

atomic_* accesses to MMIO is almost certainly a bad idea. Is this atomic
because you want to allow the function to run concurrently, or is it atomic
because you want some guarantees from the endpoint's view?

Will

Re: [PATCH 10/17] irqchip: New RISC-V PLIC Driver

[Will Deacon]

On Wed, Jun 07, 2017 at 11:52:10AM +0100, Marc Zyngier wrote:
> On 07/06/17 00:00, Palmer Dabbelt wrote:
> > +static void plic_disable(struct plic_data *data, int i, int hwirq)
> > +{
> > +   struct plic_enable_context *enable = plic_enable_context(data, i);
> > +
> > +   atomic_and(~(1 << (hwirq % 32)), >mask[hwirq / 32]);
> 
> This is still a device access, right? What does it mean to use the
> atomic primitives on that? What are you racing against? I thought the
> various context were private to an execution context...
> 
> Adding Will and PeterZ to the CC list because they will probably have
> their own views on this...

atomic_* accesses to MMIO is almost certainly a bad idea. Is this atomic
because you want to allow the function to run concurrently, or is it atomic
because you want some guarantees from the endpoint's view?

Will

Re: [PATCH 10/17] irqchip: New RISC-V PLIC Driver

[Marc Zyngier]

Hi Palmer,

On 07/06/17 00:00, Palmer Dabbelt wrote:
> This patch adds a driver for the Platform Level Interrupt Controller
> (PLIC) specified as part of the RISC-V supervisor level ISA manual.
> The PLIC connocts global interrupt sources to the local interrupt
> controller on each hart.  A PLIC is present on all RISC-V systems.
> 
> Signed-off-by: Palmer Dabbelt 
> ---
>  drivers/irqchip/Kconfig  |  12 ++
>  drivers/irqchip/Makefile |   1 +
>  drivers/irqchip/irq-riscv-plic.c | 253 
> +++
>  3 files changed, 266 insertions(+)
>  create mode 100644 drivers/irqchip/irq-riscv-plic.c
> 
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index 478f8ace2664..2906d63934ef 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -301,3 +301,15 @@ config QCOM_IRQ_COMBINER
>   help
> Say yes here to add support for the IRQ combiner devices embedded
> in Qualcomm Technologies chips.
> +
> +config RISCV_PLIC
> +bool "Platform-Level Interrupt Controller"
> + depends on RISCV
> +default y
> +help
> +This enables support for the PLIC chip found in standard RISC-V
> +systems. The PLIC is the top-most interrupt controller found in

nit: this seems to slightly contradict what is being said in patch #8,
where the HLIC is the top-level interrupt controller.

> +the system, connected directly to the core complex. All other
> +interrupt sources (MSI, GPIO, etc) are subordinate to the PLIC.
> +
> +If you don't know what to do here, say Y.
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index b64c59b838a0..bed94cc89146 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -76,3 +76,4 @@ obj-$(CONFIG_EZNPS_GIC) += irq-eznps.o
>  obj-$(CONFIG_ARCH_ASPEED)+= irq-aspeed-vic.o
>  obj-$(CONFIG_STM32_EXTI) += irq-stm32-exti.o
>  obj-$(CONFIG_QCOM_IRQ_COMBINER)  += qcom-irq-combiner.o
> +obj-$(CONFIG_RISCV_PLIC) += irq-riscv-plic.o
> diff --git a/drivers/irqchip/irq-riscv-plic.c 
> b/drivers/irqchip/irq-riscv-plic.c
> new file mode 100644
> index ..906c8a62a911
> --- /dev/null
> +++ b/drivers/irqchip/irq-riscv-plic.c
> @@ -0,0 +1,253 @@
> +/*
> + * Copyright (C) 2017 SiFive
> + *
> + *   This program is free software; you can redistribute it and/or
> + *   modify it under the terms of the GNU General Public License
> + *   as published by the Free Software Foundation, version 2.
> + *
> + *   This program is distributed in the hope that it will be useful,
> + *   but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *   GNU General Public License for more details.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +/* From the RISC-V Privlidged Spec v1.10:
> + *
> + * Global interrupt sources are assigned small unsigned integer identifiers,
> + * beginning at the value 1.  An interrupt ID of 0 is reserved to mean “no
> + * interrupt”.  Interrupt identifiers are also used to break ties when two or
> + * more interrupt sources have the same assigned priority. Smaller values of
> + * interrupt ID take precedence over larger values of interrupt ID.
> + *
> + * It's not defined what the largest device ID is, so we're just fixing
> + * MAX_DEVICES right here (which is named oddly, as there will never be a
> + * device 0).
> + */
> +#define MAX_DEVICES  1024
> +#define MAX_CONTEXTS 15872

If those are HW properties, they should probably come from the device
tree instead of being hardcoded here.

> +
> +#define PRIORITY_BASE0
> +#define ENABLE_BASE  0x2000
> +#define ENABLE_SIZE  0x80
> +#define HART_BASE0x20
> +#define HART_SIZE0x1000
> +
> +struct plic_hart_context {
> + u32 threshold;
> + u32 claim;
> +};

Representing HW layout as a C structure is not something we usually do
in the kernel. It relies on the ABI (which may change over time), and
makes it harder to quickly distinguish what is a SW concept from what's
not. It also leads to some of the mistakes below...

> +
> +struct plic_enable_context {
> + atomic_t mask[32]; // 32-bit * 32-entry

/* */ comments please, placed above the field.

> +};
> +
> +struct plic_priority {
> + u32 prio[MAX_DEVICES];
> +};
> +
> +struct plic_data {
> + struct irq_chip chip;
> + struct irq_domain   *domain;
> + u32 ndev;
> + void __iomem*reg;
> + int handlers;
> + struct plic_handler *handler;
> + charname[30];

Why 30? #define?

> +};
> +
> +struct plic_handler {
> + struct plic_hart_context*context;
> + struct plic_data

Re: [PATCH 10/17] irqchip: New RISC-V PLIC Driver

[Marc Zyngier]

Hi Palmer,

On 07/06/17 00:00, Palmer Dabbelt wrote:
> This patch adds a driver for the Platform Level Interrupt Controller
> (PLIC) specified as part of the RISC-V supervisor level ISA manual.
> The PLIC connocts global interrupt sources to the local interrupt
> controller on each hart.  A PLIC is present on all RISC-V systems.
> 
> Signed-off-by: Palmer Dabbelt 
> ---
>  drivers/irqchip/Kconfig  |  12 ++
>  drivers/irqchip/Makefile |   1 +
>  drivers/irqchip/irq-riscv-plic.c | 253 
> +++
>  3 files changed, 266 insertions(+)
>  create mode 100644 drivers/irqchip/irq-riscv-plic.c
> 
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index 478f8ace2664..2906d63934ef 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -301,3 +301,15 @@ config QCOM_IRQ_COMBINER
>   help
> Say yes here to add support for the IRQ combiner devices embedded
> in Qualcomm Technologies chips.
> +
> +config RISCV_PLIC
> +bool "Platform-Level Interrupt Controller"
> + depends on RISCV
> +default y
> +help
> +This enables support for the PLIC chip found in standard RISC-V
> +systems. The PLIC is the top-most interrupt controller found in

nit: this seems to slightly contradict what is being said in patch #8,
where the HLIC is the top-level interrupt controller.

> +the system, connected directly to the core complex. All other
> +interrupt sources (MSI, GPIO, etc) are subordinate to the PLIC.
> +
> +If you don't know what to do here, say Y.
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index b64c59b838a0..bed94cc89146 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -76,3 +76,4 @@ obj-$(CONFIG_EZNPS_GIC) += irq-eznps.o
>  obj-$(CONFIG_ARCH_ASPEED)+= irq-aspeed-vic.o
>  obj-$(CONFIG_STM32_EXTI) += irq-stm32-exti.o
>  obj-$(CONFIG_QCOM_IRQ_COMBINER)  += qcom-irq-combiner.o
> +obj-$(CONFIG_RISCV_PLIC) += irq-riscv-plic.o
> diff --git a/drivers/irqchip/irq-riscv-plic.c 
> b/drivers/irqchip/irq-riscv-plic.c
> new file mode 100644
> index ..906c8a62a911
> --- /dev/null
> +++ b/drivers/irqchip/irq-riscv-plic.c
> @@ -0,0 +1,253 @@
> +/*
> + * Copyright (C) 2017 SiFive
> + *
> + *   This program is free software; you can redistribute it and/or
> + *   modify it under the terms of the GNU General Public License
> + *   as published by the Free Software Foundation, version 2.
> + *
> + *   This program is distributed in the hope that it will be useful,
> + *   but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *   GNU General Public License for more details.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +/* From the RISC-V Privlidged Spec v1.10:
> + *
> + * Global interrupt sources are assigned small unsigned integer identifiers,
> + * beginning at the value 1.  An interrupt ID of 0 is reserved to mean “no
> + * interrupt”.  Interrupt identifiers are also used to break ties when two or
> + * more interrupt sources have the same assigned priority. Smaller values of
> + * interrupt ID take precedence over larger values of interrupt ID.
> + *
> + * It's not defined what the largest device ID is, so we're just fixing
> + * MAX_DEVICES right here (which is named oddly, as there will never be a
> + * device 0).
> + */
> +#define MAX_DEVICES  1024
> +#define MAX_CONTEXTS 15872

If those are HW properties, they should probably come from the device
tree instead of being hardcoded here.

> +
> +#define PRIORITY_BASE0
> +#define ENABLE_BASE  0x2000
> +#define ENABLE_SIZE  0x80
> +#define HART_BASE0x20
> +#define HART_SIZE0x1000
> +
> +struct plic_hart_context {
> + u32 threshold;
> + u32 claim;
> +};

Representing HW layout as a C structure is not something we usually do
in the kernel. It relies on the ABI (which may change over time), and
makes it harder to quickly distinguish what is a SW concept from what's
not. It also leads to some of the mistakes below...

> +
> +struct plic_enable_context {
> + atomic_t mask[32]; // 32-bit * 32-entry

/* */ comments please, placed above the field.

> +};
> +
> +struct plic_priority {
> + u32 prio[MAX_DEVICES];
> +};
> +
> +struct plic_data {
> + struct irq_chip chip;
> + struct irq_domain   *domain;
> + u32 ndev;
> + void __iomem*reg;
> + int handlers;
> + struct plic_handler *handler;
> + charname[30];

Why 30? #define?

> +};
> +
> +struct plic_handler {
> + struct plic_hart_context*context;
> + struct plic_data*data;
> +};
> +
> 

Re: [PATCH 10/17] irqchip: New RISC-V PLIC Driver

[Arnd Bergmann]

On Wed, Jun 7, 2017 at 9:13 AM, Geert Uytterhoeven  wrote:
>> +struct plic_enable_context {
>> +   atomic_t mask[32]; // 32-bit * 32-entry
>> +};

You use many '//' style comments in this file, please change them all to '/* */'
for consistency with kernel coding style.

>> +
>> +struct plic_priority {
>> +   u32 prio[MAX_DEVICES];
>> +};
>> +
>> +struct plic_data {
>> +   struct irq_chip chip;
>> +   struct irq_domain   *domain;
>> +   u32 ndev;
>> +   void __iomem*reg;
>> +   int handlers;
>> +   struct plic_handler *handler;
>> +   charname[30];
>> +};
>> +
>> +struct plic_handler {
>> +   struct plic_hart_context*context;
>> +   struct plic_data*data;
>> +};
>> +
>> +static inline
>> +struct plic_hart_context *plic_hart_context(struct plic_data *data, size_t 
>> i)
>> +{
>> +   return (struct plic_hart_context *)((char *)data->reg + HART_BASE + 
>> HART_SIZE*i);
>> +}

'data->reg' is an __iomem pointer, so when you build-test this with 'make C=1',
you should get a valid warning from sparse about an address space mismatch.
Please address all the warning from sparse.

>> +static void plic_disable(struct plic_data *data, int i, int hwirq)
>> +{
>> +   struct plic_enable_context *enable = plic_enable_context(data, i);
>> +
>> +   atomic_and(~(1 << (hwirq % 32)), >mask[hwirq / 32]);
>> +}

In particular, you must not do atomic operations on MMIO pointers.
On most architectures these are explicitly disallowed and trap for
a good reason, as the hardware implementation behind atomics tend
to rely on the cache controller, while mmio registers are required
to be uncached.

>> +   iowrite32(1, >prio[d->hwirq]);

I would normally use 'readl' instead of 'iowrite32'. They may be the same
on riscv, but they have slightly different meaning in portable drivers.

   Arnd

Re: [PATCH 10/17] irqchip: New RISC-V PLIC Driver

[Arnd Bergmann]

On Wed, Jun 7, 2017 at 9:13 AM, Geert Uytterhoeven  wrote:
>> +struct plic_enable_context {
>> +   atomic_t mask[32]; // 32-bit * 32-entry
>> +};

You use many '//' style comments in this file, please change them all to '/* */'
for consistency with kernel coding style.

>> +
>> +struct plic_priority {
>> +   u32 prio[MAX_DEVICES];
>> +};
>> +
>> +struct plic_data {
>> +   struct irq_chip chip;
>> +   struct irq_domain   *domain;
>> +   u32 ndev;
>> +   void __iomem*reg;
>> +   int handlers;
>> +   struct plic_handler *handler;
>> +   charname[30];
>> +};
>> +
>> +struct plic_handler {
>> +   struct plic_hart_context*context;
>> +   struct plic_data*data;
>> +};
>> +
>> +static inline
>> +struct plic_hart_context *plic_hart_context(struct plic_data *data, size_t 
>> i)
>> +{
>> +   return (struct plic_hart_context *)((char *)data->reg + HART_BASE + 
>> HART_SIZE*i);
>> +}

'data->reg' is an __iomem pointer, so when you build-test this with 'make C=1',
you should get a valid warning from sparse about an address space mismatch.
Please address all the warning from sparse.

>> +static void plic_disable(struct plic_data *data, int i, int hwirq)
>> +{
>> +   struct plic_enable_context *enable = plic_enable_context(data, i);
>> +
>> +   atomic_and(~(1 << (hwirq % 32)), >mask[hwirq / 32]);
>> +}

In particular, you must not do atomic operations on MMIO pointers.
On most architectures these are explicitly disallowed and trap for
a good reason, as the hardware implementation behind atomics tend
to rely on the cache controller, while mmio registers are required
to be uncached.

>> +   iowrite32(1, >prio[d->hwirq]);

I would normally use 'readl' instead of 'iowrite32'. They may be the same
on riscv, but they have slightly different meaning in portable drivers.

   Arnd

Re: [PATCH 10/17] irqchip: New RISC-V PLIC Driver

[Geert Uytterhoeven]

CC irqchip folks

On Wed, Jun 7, 2017 at 1:00 AM, Palmer Dabbelt  wrote:
> This patch adds a driver for the Platform Level Interrupt Controller
> (PLIC) specified as part of the RISC-V supervisor level ISA manual.
> The PLIC connocts global interrupt sources to the local interrupt
> controller on each hart.  A PLIC is present on all RISC-V systems.
>
> Signed-off-by: Palmer Dabbelt 
> ---
>  drivers/irqchip/Kconfig  |  12 ++
>  drivers/irqchip/Makefile |   1 +
>  drivers/irqchip/irq-riscv-plic.c | 253 
> +++
>  3 files changed, 266 insertions(+)
>  create mode 100644 drivers/irqchip/irq-riscv-plic.c
>
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index 478f8ace2664..2906d63934ef 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -301,3 +301,15 @@ config QCOM_IRQ_COMBINER
> help
>   Say yes here to add support for the IRQ combiner devices embedded
>   in Qualcomm Technologies chips.
> +
> +config RISCV_PLIC
> +bool "Platform-Level Interrupt Controller"
> +   depends on RISCV
> +default y
> +help
> +  This enables support for the PLIC chip found in standard RISC-V
> +  systems. The PLIC is the top-most interrupt controller found in
> +  the system, connected directly to the core complex. All other
> +  interrupt sources (MSI, GPIO, etc) are subordinate to the PLIC.
> +
> +  If you don't know what to do here, say Y.
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index b64c59b838a0..bed94cc89146 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -76,3 +76,4 @@ obj-$(CONFIG_EZNPS_GIC)   += irq-eznps.o
>  obj-$(CONFIG_ARCH_ASPEED)  += irq-aspeed-vic.o
>  obj-$(CONFIG_STM32_EXTI)   += irq-stm32-exti.o
>  obj-$(CONFIG_QCOM_IRQ_COMBINER)+= qcom-irq-combiner.o
> +obj-$(CONFIG_RISCV_PLIC)   += irq-riscv-plic.o
> diff --git a/drivers/irqchip/irq-riscv-plic.c 
> b/drivers/irqchip/irq-riscv-plic.c
> new file mode 100644
> index ..906c8a62a911
> --- /dev/null
> +++ b/drivers/irqchip/irq-riscv-plic.c
> @@ -0,0 +1,253 @@
> +/*
> + * Copyright (C) 2017 SiFive
> + *
> + *   This program is free software; you can redistribute it and/or
> + *   modify it under the terms of the GNU General Public License
> + *   as published by the Free Software Foundation, version 2.
> + *
> + *   This program is distributed in the hope that it will be useful,
> + *   but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *   GNU General Public License for more details.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +/* From the RISC-V Privlidged Spec v1.10:
> + *
> + * Global interrupt sources are assigned small unsigned integer identifiers,
> + * beginning at the value 1.  An interrupt ID of 0 is reserved to mean “no
> + * interrupt”.  Interrupt identifiers are also used to break ties when two or
> + * more interrupt sources have the same assigned priority. Smaller values of
> + * interrupt ID take precedence over larger values of interrupt ID.
> + *
> + * It's not defined what the largest device ID is, so we're just fixing
> + * MAX_DEVICES right here (which is named oddly, as there will never be a
> + * device 0).
> + */
> +#define MAX_DEVICES1024
> +#define MAX_CONTEXTS   15872
> +
> +#define PRIORITY_BASE  0
> +#define ENABLE_BASE0x2000
> +#define ENABLE_SIZE0x80
> +#define HART_BASE  0x20
> +#define HART_SIZE  0x1000
> +
> +struct plic_hart_context {
> +   u32 threshold;
> +   u32 claim;
> +};
> +
> +struct plic_enable_context {
> +   atomic_t mask[32]; // 32-bit * 32-entry
> +};
> +
> +struct plic_priority {
> +   u32 prio[MAX_DEVICES];
> +};
> +
> +struct plic_data {
> +   struct irq_chip chip;
> +   struct irq_domain   *domain;
> +   u32 ndev;
> +   void __iomem*reg;
> +   int handlers;
> +   struct plic_handler *handler;
> +   charname[30];
> +};
> +
> +struct plic_handler {
> +   struct plic_hart_context*context;
> +   struct plic_data*data;
> +};
> +
> +static inline
> +struct plic_hart_context *plic_hart_context(struct plic_data *data, size_t i)
> +{
> +   return (struct plic_hart_context *)((char *)data->reg + HART_BASE + 
> HART_SIZE*i);
> +}
> +
> +static inline
> +struct plic_enable_context *plic_enable_context(struct plic_data *data, 
> size_t i)
> +{
> +   return (struct plic_enable_context *)((char *)data->reg + ENABLE_BASE 
> + ENABLE_SIZE*i);
> +}
> +
> +static inline
> +struct 

Re: [PATCH 10/17] irqchip: New RISC-V PLIC Driver

[Geert Uytterhoeven]

CC irqchip folks

On Wed, Jun 7, 2017 at 1:00 AM, Palmer Dabbelt  wrote:
> This patch adds a driver for the Platform Level Interrupt Controller
> (PLIC) specified as part of the RISC-V supervisor level ISA manual.
> The PLIC connocts global interrupt sources to the local interrupt
> controller on each hart.  A PLIC is present on all RISC-V systems.
>
> Signed-off-by: Palmer Dabbelt 
> ---
>  drivers/irqchip/Kconfig  |  12 ++
>  drivers/irqchip/Makefile |   1 +
>  drivers/irqchip/irq-riscv-plic.c | 253 
> +++
>  3 files changed, 266 insertions(+)
>  create mode 100644 drivers/irqchip/irq-riscv-plic.c
>
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index 478f8ace2664..2906d63934ef 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -301,3 +301,15 @@ config QCOM_IRQ_COMBINER
> help
>   Say yes here to add support for the IRQ combiner devices embedded
>   in Qualcomm Technologies chips.
> +
> +config RISCV_PLIC
> +bool "Platform-Level Interrupt Controller"
> +   depends on RISCV
> +default y
> +help
> +  This enables support for the PLIC chip found in standard RISC-V
> +  systems. The PLIC is the top-most interrupt controller found in
> +  the system, connected directly to the core complex. All other
> +  interrupt sources (MSI, GPIO, etc) are subordinate to the PLIC.
> +
> +  If you don't know what to do here, say Y.
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index b64c59b838a0..bed94cc89146 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -76,3 +76,4 @@ obj-$(CONFIG_EZNPS_GIC)   += irq-eznps.o
>  obj-$(CONFIG_ARCH_ASPEED)  += irq-aspeed-vic.o
>  obj-$(CONFIG_STM32_EXTI)   += irq-stm32-exti.o
>  obj-$(CONFIG_QCOM_IRQ_COMBINER)+= qcom-irq-combiner.o
> +obj-$(CONFIG_RISCV_PLIC)   += irq-riscv-plic.o
> diff --git a/drivers/irqchip/irq-riscv-plic.c 
> b/drivers/irqchip/irq-riscv-plic.c
> new file mode 100644
> index ..906c8a62a911
> --- /dev/null
> +++ b/drivers/irqchip/irq-riscv-plic.c
> @@ -0,0 +1,253 @@
> +/*
> + * Copyright (C) 2017 SiFive
> + *
> + *   This program is free software; you can redistribute it and/or
> + *   modify it under the terms of the GNU General Public License
> + *   as published by the Free Software Foundation, version 2.
> + *
> + *   This program is distributed in the hope that it will be useful,
> + *   but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *   GNU General Public License for more details.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +/* From the RISC-V Privlidged Spec v1.10:
> + *
> + * Global interrupt sources are assigned small unsigned integer identifiers,
> + * beginning at the value 1.  An interrupt ID of 0 is reserved to mean “no
> + * interrupt”.  Interrupt identifiers are also used to break ties when two or
> + * more interrupt sources have the same assigned priority. Smaller values of
> + * interrupt ID take precedence over larger values of interrupt ID.
> + *
> + * It's not defined what the largest device ID is, so we're just fixing
> + * MAX_DEVICES right here (which is named oddly, as there will never be a
> + * device 0).
> + */
> +#define MAX_DEVICES1024
> +#define MAX_CONTEXTS   15872
> +
> +#define PRIORITY_BASE  0
> +#define ENABLE_BASE0x2000
> +#define ENABLE_SIZE0x80
> +#define HART_BASE  0x20
> +#define HART_SIZE  0x1000
> +
> +struct plic_hart_context {
> +   u32 threshold;
> +   u32 claim;
> +};
> +
> +struct plic_enable_context {
> +   atomic_t mask[32]; // 32-bit * 32-entry
> +};
> +
> +struct plic_priority {
> +   u32 prio[MAX_DEVICES];
> +};
> +
> +struct plic_data {
> +   struct irq_chip chip;
> +   struct irq_domain   *domain;
> +   u32 ndev;
> +   void __iomem*reg;
> +   int handlers;
> +   struct plic_handler *handler;
> +   charname[30];
> +};
> +
> +struct plic_handler {
> +   struct plic_hart_context*context;
> +   struct plic_data*data;
> +};
> +
> +static inline
> +struct plic_hart_context *plic_hart_context(struct plic_data *data, size_t i)
> +{
> +   return (struct plic_hart_context *)((char *)data->reg + HART_BASE + 
> HART_SIZE*i);
> +}
> +
> +static inline
> +struct plic_enable_context *plic_enable_context(struct plic_data *data, 
> size_t i)
> +{
> +   return (struct plic_enable_context *)((char *)data->reg + ENABLE_BASE 
> + ENABLE_SIZE*i);
> +}
> +
> +static inline
> +struct plic_priority *plic_priority(struct 

[PATCH 10/17] irqchip: New RISC-V PLIC Driver

[Palmer Dabbelt]

This patch adds a driver for the Platform Level Interrupt Controller
(PLIC) specified as part of the RISC-V supervisor level ISA manual.
The PLIC connocts global interrupt sources to the local interrupt
controller on each hart.  A PLIC is present on all RISC-V systems.

Signed-off-by: Palmer Dabbelt 
---
 drivers/irqchip/Kconfig  |  12 ++
 drivers/irqchip/Makefile |   1 +
 drivers/irqchip/irq-riscv-plic.c | 253 +++
 3 files changed, 266 insertions(+)
 create mode 100644 drivers/irqchip/irq-riscv-plic.c

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 478f8ace2664..2906d63934ef 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -301,3 +301,15 @@ config QCOM_IRQ_COMBINER
help
  Say yes here to add support for the IRQ combiner devices embedded
  in Qualcomm Technologies chips.
+
+config RISCV_PLIC
+bool "Platform-Level Interrupt Controller"
+   depends on RISCV
+default y
+help
+  This enables support for the PLIC chip found in standard RISC-V
+  systems. The PLIC is the top-most interrupt controller found in
+  the system, connected directly to the core complex. All other
+  interrupt sources (MSI, GPIO, etc) are subordinate to the PLIC.
+
+  If you don't know what to do here, say Y.
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index b64c59b838a0..bed94cc89146 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -76,3 +76,4 @@ obj-$(CONFIG_EZNPS_GIC)   += irq-eznps.o
 obj-$(CONFIG_ARCH_ASPEED)  += irq-aspeed-vic.o
 obj-$(CONFIG_STM32_EXTI)   += irq-stm32-exti.o
 obj-$(CONFIG_QCOM_IRQ_COMBINER)+= qcom-irq-combiner.o
+obj-$(CONFIG_RISCV_PLIC)   += irq-riscv-plic.o
diff --git a/drivers/irqchip/irq-riscv-plic.c b/drivers/irqchip/irq-riscv-plic.c
new file mode 100644
index ..906c8a62a911
--- /dev/null
+++ b/drivers/irqchip/irq-riscv-plic.c
@@ -0,0 +1,253 @@
+/*
+ * Copyright (C) 2017 SiFive
+ *
+ *   This program is free software; you can redistribute it and/or
+ *   modify it under the terms of the GNU General Public License
+ *   as published by the Free Software Foundation, version 2.
+ *
+ *   This program is distributed in the hope that it will be useful,
+ *   but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *   GNU General Public License for more details.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+/* From the RISC-V Privlidged Spec v1.10:
+ *
+ * Global interrupt sources are assigned small unsigned integer identifiers,
+ * beginning at the value 1.  An interrupt ID of 0 is reserved to mean “no
+ * interrupt”.  Interrupt identifiers are also used to break ties when two or
+ * more interrupt sources have the same assigned priority. Smaller values of
+ * interrupt ID take precedence over larger values of interrupt ID.
+ *
+ * It's not defined what the largest device ID is, so we're just fixing
+ * MAX_DEVICES right here (which is named oddly, as there will never be a
+ * device 0).
+ */
+#define MAX_DEVICES1024
+#define MAX_CONTEXTS   15872
+
+#define PRIORITY_BASE  0
+#define ENABLE_BASE0x2000
+#define ENABLE_SIZE0x80
+#define HART_BASE  0x20
+#define HART_SIZE  0x1000
+
+struct plic_hart_context {
+   u32 threshold;
+   u32 claim;
+};
+
+struct plic_enable_context {
+   atomic_t mask[32]; // 32-bit * 32-entry
+};
+
+struct plic_priority {
+   u32 prio[MAX_DEVICES];
+};
+
+struct plic_data {
+   struct irq_chip chip;
+   struct irq_domain   *domain;
+   u32 ndev;
+   void __iomem*reg;
+   int handlers;
+   struct plic_handler *handler;
+   charname[30];
+};
+
+struct plic_handler {
+   struct plic_hart_context*context;
+   struct plic_data*data;
+};
+
+static inline
+struct plic_hart_context *plic_hart_context(struct plic_data *data, size_t i)
+{
+   return (struct plic_hart_context *)((char *)data->reg + HART_BASE + 
HART_SIZE*i);
+}
+
+static inline
+struct plic_enable_context *plic_enable_context(struct plic_data *data, size_t 
i)
+{
+   return (struct plic_enable_context *)((char *)data->reg + ENABLE_BASE + 
ENABLE_SIZE*i);
+}
+
+static inline
+struct plic_priority *plic_priority(struct plic_data *data)
+{
+   return (struct plic_priority *)((char *)data->reg + PRIORITY_BASE);
+}
+
+static void plic_disable(struct plic_data *data, int i, int hwirq)
+{
+   struct plic_enable_context *enable = plic_enable_context(data, i);
+
+   atomic_and(~(1 << (hwirq % 32)), >mask[hwirq / 32]);
+}
+
+static void 

[PATCH 10/17] irqchip: New RISC-V PLIC Driver

[Palmer Dabbelt]

This patch adds a driver for the Platform Level Interrupt Controller
(PLIC) specified as part of the RISC-V supervisor level ISA manual.
The PLIC connocts global interrupt sources to the local interrupt
controller on each hart.  A PLIC is present on all RISC-V systems.

Signed-off-by: Palmer Dabbelt 
---
 drivers/irqchip/Kconfig  |  12 ++
 drivers/irqchip/Makefile |   1 +
 drivers/irqchip/irq-riscv-plic.c | 253 +++
 3 files changed, 266 insertions(+)
 create mode 100644 drivers/irqchip/irq-riscv-plic.c

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 478f8ace2664..2906d63934ef 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -301,3 +301,15 @@ config QCOM_IRQ_COMBINER
help
  Say yes here to add support for the IRQ combiner devices embedded
  in Qualcomm Technologies chips.
+
+config RISCV_PLIC
+bool "Platform-Level Interrupt Controller"
+   depends on RISCV
+default y
+help
+  This enables support for the PLIC chip found in standard RISC-V
+  systems. The PLIC is the top-most interrupt controller found in
+  the system, connected directly to the core complex. All other
+  interrupt sources (MSI, GPIO, etc) are subordinate to the PLIC.
+
+  If you don't know what to do here, say Y.
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index b64c59b838a0..bed94cc89146 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -76,3 +76,4 @@ obj-$(CONFIG_EZNPS_GIC)   += irq-eznps.o
 obj-$(CONFIG_ARCH_ASPEED)  += irq-aspeed-vic.o
 obj-$(CONFIG_STM32_EXTI)   += irq-stm32-exti.o
 obj-$(CONFIG_QCOM_IRQ_COMBINER)+= qcom-irq-combiner.o
+obj-$(CONFIG_RISCV_PLIC)   += irq-riscv-plic.o
diff --git a/drivers/irqchip/irq-riscv-plic.c b/drivers/irqchip/irq-riscv-plic.c
new file mode 100644
index ..906c8a62a911
--- /dev/null
+++ b/drivers/irqchip/irq-riscv-plic.c
@@ -0,0 +1,253 @@
+/*
+ * Copyright (C) 2017 SiFive
+ *
+ *   This program is free software; you can redistribute it and/or
+ *   modify it under the terms of the GNU General Public License
+ *   as published by the Free Software Foundation, version 2.
+ *
+ *   This program is distributed in the hope that it will be useful,
+ *   but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *   GNU General Public License for more details.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+/* From the RISC-V Privlidged Spec v1.10:
+ *
+ * Global interrupt sources are assigned small unsigned integer identifiers,
+ * beginning at the value 1.  An interrupt ID of 0 is reserved to mean “no
+ * interrupt”.  Interrupt identifiers are also used to break ties when two or
+ * more interrupt sources have the same assigned priority. Smaller values of
+ * interrupt ID take precedence over larger values of interrupt ID.
+ *
+ * It's not defined what the largest device ID is, so we're just fixing
+ * MAX_DEVICES right here (which is named oddly, as there will never be a
+ * device 0).
+ */
+#define MAX_DEVICES1024
+#define MAX_CONTEXTS   15872
+
+#define PRIORITY_BASE  0
+#define ENABLE_BASE0x2000
+#define ENABLE_SIZE0x80
+#define HART_BASE  0x20
+#define HART_SIZE  0x1000
+
+struct plic_hart_context {
+   u32 threshold;
+   u32 claim;
+};
+
+struct plic_enable_context {
+   atomic_t mask[32]; // 32-bit * 32-entry
+};
+
+struct plic_priority {
+   u32 prio[MAX_DEVICES];
+};
+
+struct plic_data {
+   struct irq_chip chip;
+   struct irq_domain   *domain;
+   u32 ndev;
+   void __iomem*reg;
+   int handlers;
+   struct plic_handler *handler;
+   charname[30];
+};
+
+struct plic_handler {
+   struct plic_hart_context*context;
+   struct plic_data*data;
+};
+
+static inline
+struct plic_hart_context *plic_hart_context(struct plic_data *data, size_t i)
+{
+   return (struct plic_hart_context *)((char *)data->reg + HART_BASE + 
HART_SIZE*i);
+}
+
+static inline
+struct plic_enable_context *plic_enable_context(struct plic_data *data, size_t 
i)
+{
+   return (struct plic_enable_context *)((char *)data->reg + ENABLE_BASE + 
ENABLE_SIZE*i);
+}
+
+static inline
+struct plic_priority *plic_priority(struct plic_data *data)
+{
+   return (struct plic_priority *)((char *)data->reg + PRIORITY_BASE);
+}
+
+static void plic_disable(struct plic_data *data, int i, int hwirq)
+{
+   struct plic_enable_context *enable = plic_enable_context(data, i);
+
+   atomic_and(~(1 << (hwirq % 32)), >mask[hwirq / 32]);
+}
+
+static void plic_enable(struct plic_data