Re: [PATCH 03/17] base: fix order of OF initialization

Re: [PATCH 03/17] base: fix order of OF initialization

[Geert Uytterhoeven]

CC devicetree folks

On Wed, Jun 7, 2017 at 12:59 AM, Palmer Dabbelt <palmer@dabbelt.com> wrote:
> From: "Wesley W. Terpstra" <wesley@sifive.com>
>
> This fixes: [    0.010000] cpu cpu0: Error -2 creating of_node link
> ... which you get for every CPU on all architectures with a OF cpu/ node.
>
> This affects riscv, nios, etc.
>
> Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
> ---
>  drivers/base/init.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/base/init.c b/drivers/base/init.c
> index 48c0e220acc0..0dcd17e561d0 100644
> --- a/drivers/base/init.c
> +++ b/drivers/base/init.c
> @@ -31,9 +31,9 @@ void __init driver_init(void)
>         /* These are also core pieces, but must come after the
>          * core core pieces.
>          */
> +       of_core_init();
>         platform_bus_init();
>         cpu_dev_init();
>         memory_dev_init();
>         container_dev_init();
> -       of_core_init();
>  }
> --
> 2.13.0

Re: [PATCH 03/17] base: fix order of OF initialization

[Mark Rutland]

On Wed, Jun 07, 2017 at 09:07:20AM +0200, Geert Uytterhoeven wrote:
> CC devicetree folks
> 
> On Wed, Jun 7, 2017 at 12:59 AM, Palmer Dabbelt <palmer-96lFi9zoCfxBDgjK7y7TUQ@public.gmane.org> wrote:
> > From: "Wesley W. Terpstra" <wesley-SpMDHPYPyPbQT0dZR+AlfA@public.gmane.org>
> >
> > This fixes: [    0.010000] cpu cpu0: Error -2 creating of_node link
> > ... which you get for every CPU on all architectures with a OF cpu/ node.

I take it this means a /cpus node? Or the /cpus/cpu@* nodes?

I'm not seeing this on arm64 when booting v4.12-rc4 with DT, so clearly
this doesn't affect all such architectures.

What path are these errors happening in?

Thanks,
Mark.

> >
> > This affects riscv, nios, etc.
> >
> > Signed-off-by: Palmer Dabbelt <palmer-96lFi9zoCfxBDgjK7y7TUQ@public.gmane.org>
> > ---
> >  drivers/base/init.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/base/init.c b/drivers/base/init.c
> > index 48c0e220acc0..0dcd17e561d0 100644
> > --- a/drivers/base/init.c
> > +++ b/drivers/base/init.c
> > @@ -31,9 +31,9 @@ void __init driver_init(void)
> >         /* These are also core pieces, but must come after the
> >          * core core pieces.
> >          */
> > +       of_core_init();
> >         platform_bus_init();
> >         cpu_dev_init();
> >         memory_dev_init();
> >         container_dev_init();
> > -       of_core_init();
> >  }
> > --
> > 2.13.0
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 03/17] base: fix order of OF initialization

[Wesley Terpstra]

It was a while ago that I debugged this. I already reported this bug
to Benjamin Herrenschmidt (now in CC), and I believe he has a patch of
his own to fix the same issue.

As I understand it, of_core_init sets up the OF entries in
/sys/firmware/devicetree. During platform bringup, when the system
describes the cpu + cache hierarchy, it also makes an of_node symlink
into that directory. However, if it doesn't exist yet, you get the
warning.

# ls -l /sys/devices/system/cpu/cpu3/of_node
lrwxrwxrwx    1 root     root             0 Jan  1 00:00
/sys/devices/system/cpu/cpu3/of_node ->
../../../../firmware/devicetree/base/cpus/cpu@3

On Wed, Jun 7, 2017 at 2:35 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Wed, Jun 07, 2017 at 09:07:20AM +0200, Geert Uytterhoeven wrote:
>> CC devicetree folks
>>
>> On Wed, Jun 7, 2017 at 12:59 AM, Palmer Dabbelt <palmer@dabbelt.com> wrote:
>> > From: "Wesley W. Terpstra" <wesley@sifive.com>
>> >
>> > This fixes: [    0.010000] cpu cpu0: Error -2 creating of_node link
>> > ... which you get for every CPU on all architectures with a OF cpu/ node.
>
> I take it this means a /cpus node? Or the /cpus/cpu@* nodes?
>
> I'm not seeing this on arm64 when booting v4.12-rc4 with DT, so clearly
> this doesn't affect all such architectures.
>
> What path are these errors happening in?
>
> Thanks,
> Mark.
>
>> >
>> > This affects riscv, nios, etc.
>> >
>> > Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
>> > ---
>> >  drivers/base/init.c | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/base/init.c b/drivers/base/init.c
>> > index 48c0e220acc0..0dcd17e561d0 100644
>> > --- a/drivers/base/init.c
>> > +++ b/drivers/base/init.c
>> > @@ -31,9 +31,9 @@ void __init driver_init(void)
>> >         /* These are also core pieces, but must come after the
>> >          * core core pieces.
>> >          */
>> > +       of_core_init();
>> >         platform_bus_init();
>> >         cpu_dev_init();
>> >         memory_dev_init();
>> >         container_dev_init();
>> > -       of_core_init();
>> >  }
>> > --
>> > 2.13.0
>> --
>> To unsubscribe from this list: send the line "unsubscribe devicetree" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 03/17] base: fix order of OF initialization

[Benjamin Herrenschmidt]

On Wed, 2017-06-07 at 11:39 -0700, Wesley Terpstra wrote:
> It was a while ago that I debugged this. I already reported this bug
> to Benjamin Herrenschmidt (now in CC), and I believe he has a patch of
> his own to fix the same issue.
> 
> As I understand it, of_core_init sets up the OF entries in
> /sys/firmware/devicetree. During platform bringup, when the system
> describes the cpu + cache hierarchy, it also makes an of_node symlink
> into that directory. However, if it doesn't exist yet, you get the
> warning.

Ugh... yes I did a patch for that and I think it fell through the
cracks, I can't even find it anymore...

The patch quoted here is fine I think. Everything in the device model
can potentially use OF bits these days, it makes sense to have them
initialized earlier.

Cheers,
Ben.

> # ls -l /sys/devices/system/cpu/cpu3/of_node
> lrwxrwxrwx    1 root     root             0 Jan  1 00:00
> /sys/devices/system/cpu/cpu3/of_node ->
> ../../../../firmware/devicetree/base/cpus/cpu@3
> 
> On Wed, Jun 7, 2017 at 2:35 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> > On Wed, Jun 07, 2017 at 09:07:20AM +0200, Geert Uytterhoeven wrote:
> > > CC devicetree folks
> > > 
> > > On Wed, Jun 7, 2017 at 12:59 AM, Palmer Dabbelt <palmer@dabbelt.com> wrote:
> > > > From: "Wesley W. Terpstra" <wesley@sifive.com>
> > > > 
> > > > This fixes: [    0.010000] cpu cpu0: Error -2 creating of_node link
> > > > ... which you get for every CPU on all architectures with a OF cpu/ node.
> > 
> > I take it this means a /cpus node? Or the /cpus/cpu@* nodes?
> > 
> > I'm not seeing this on arm64 when booting v4.12-rc4 with DT, so clearly
> > this doesn't affect all such architectures.
> > 
> > What path are these errors happening in?
> > 
> > Thanks,
> > Mark.
> > 
> > > > 
> > > > This affects riscv, nios, etc.
> > > > 
> > > > Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
> > > > ---
> > > >  drivers/base/init.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/base/init.c b/drivers/base/init.c
> > > > index 48c0e220acc0..0dcd17e561d0 100644
> > > > --- a/drivers/base/init.c
> > > > +++ b/drivers/base/init.c
> > > > @@ -31,9 +31,9 @@ void __init driver_init(void)
> > > >         /* These are also core pieces, but must come after the
> > > >          * core core pieces.
> > > >          */
> > > > +       of_core_init();
> > > >         platform_bus_init();
> > > >         cpu_dev_init();
> > > >         memory_dev_init();
> > > >         container_dev_init();
> > > > -       of_core_init();
> > > >  }
> > > > --
> > > > 2.13.0
> > > 
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe devicetree" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 03/17] base: fix order of OF initialization

[Frank Rowand]

On 06/07/17 11:39, Wesley Terpstra wrote:
> It was a while ago that I debugged this. I already reported this bug
> to Benjamin Herrenschmidt (now in CC), and I believe he has a patch of
> his own to fix the same issue.
> 
> As I understand it, of_core_init sets up the OF entries in
> /sys/firmware/devicetree. During platform bringup, when the system
> describes the cpu + cache hierarchy, it also makes an of_node symlink
> into that directory. However, if it doesn't exist yet, you get the
> warning.
> 
> # ls -l /sys/devices/system/cpu/cpu3/of_node
> lrwxrwxrwx    1 root     root             0 Jan  1 00:00
> /sys/devices/system/cpu/cpu3/of_node ->
> ../../../../firmware/devicetree/base/cpus/cpu@3
> 
> On Wed, Jun 7, 2017 at 2:35 AM, Mark Rutland <mark.rutland@arm.com> wrote:
>> On Wed, Jun 07, 2017 at 09:07:20AM +0200, Geert Uytterhoeven wrote:
>>> CC devicetree folks
>>>
>>> On Wed, Jun 7, 2017 at 12:59 AM, Palmer Dabbelt <palmer@dabbelt.com> wrote:
>>>> From: "Wesley W. Terpstra" <wesley@sifive.com>
>>>>
>>>> This fixes: [    0.010000] cpu cpu0: Error -2 creating of_node link
>>>> ... which you get for every CPU on all architectures with a OF cpu/ node.
>>
>> I take it this means a /cpus node? Or the /cpus/cpu@* nodes?
>>
>> I'm not seeing this on arm64 when booting v4.12-rc4 with DT, so clearly
>> this doesn't affect all such architectures.
>>
>> What path are these errors happening in?

On the surface, the patch looks reasonable.  But it is not obvious to me why
the error message is occurring.  I would like to understand the cause before
saying the patch is good.

What kernel version is showing the error?  For a specific architecture
(the patch lists 'riscv, nios, etc'), which config and device tree source?

And again, what is the calling path?

- Frank

>>
>> Thanks,
>> Mark.
>>
>>>>
>>>> This affects riscv, nios, etc.
>>>>
>>>> Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
>>>> ---
>>>>  drivers/base/init.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/base/init.c b/drivers/base/init.c
>>>> index 48c0e220acc0..0dcd17e561d0 100644
>>>> --- a/drivers/base/init.c
>>>> +++ b/drivers/base/init.c
>>>> @@ -31,9 +31,9 @@ void __init driver_init(void)
>>>>         /* These are also core pieces, but must come after the
>>>>          * core core pieces.
>>>>          */
>>>> +       of_core_init();
>>>>         platform_bus_init();
>>>>         cpu_dev_init();
>>>>         memory_dev_init();
>>>>         container_dev_init();
>>>> -       of_core_init();
>>>>  }
>>>> --
>>>> 2.13.0
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe devicetree" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 03/17] base: fix order of OF initialization

[Mark Rutland]

On Wed, Jun 07, 2017 at 08:49:43PM -0700, Frank Rowand wrote:
> On 06/07/17 11:39, Wesley Terpstra wrote:
> > It was a while ago that I debugged this. I already reported this bug
> > to Benjamin Herrenschmidt (now in CC), and I believe he has a patch of
> > his own to fix the same issue.
> > 
> > As I understand it, of_core_init sets up the OF entries in
> > /sys/firmware/devicetree. During platform bringup, when the system
> > describes the cpu + cache hierarchy, it also makes an of_node symlink
> > into that directory. However, if it doesn't exist yet, you get the
> > warning.
> > 
> > # ls -l /sys/devices/system/cpu/cpu3/of_node
> > lrwxrwxrwx    1 root     root             0 Jan  1 00:00
> > /sys/devices/system/cpu/cpu3/of_node ->
> > ../../../../firmware/devicetree/base/cpus/cpu@3
> > 
> > On Wed, Jun 7, 2017 at 2:35 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> >> On Wed, Jun 07, 2017 at 09:07:20AM +0200, Geert Uytterhoeven wrote:
> >>> CC devicetree folks
> >>>
> >>> On Wed, Jun 7, 2017 at 12:59 AM, Palmer Dabbelt <palmer@dabbelt.com> wrote:
> >>>> From: "Wesley W. Terpstra" <wesley@sifive.com>
> >>>>
> >>>> This fixes: [    0.010000] cpu cpu0: Error -2 creating of_node link
> >>>> ... which you get for every CPU on all architectures with a OF cpu/ node.
> >>
> >> I take it this means a /cpus node? Or the /cpus/cpu@* nodes?
> >>
> >> I'm not seeing this on arm64 when booting v4.12-rc4 with DT, so clearly
> >> this doesn't affect all such architectures.
> >>
> >> What path are these errors happening in?
> 
> On the surface, the patch looks reasonable.  But it is not obvious to me why
> the error message is occurring.  I would like to understand the cause before
> saying the patch is good.
> 
> What kernel version is showing the error?  For a specific architecture
> (the patch lists 'riscv, nios, etc'), which config and device tree source?
> 
> And again, what is the calling path?

>From having grepped around, I think this affects architectures which
select CONFIG_GENERIC_CPU_DEVICES, which includes nios2.

In that case, driver_init() calls cpu_dev_init() before calling
of_core_init(). Then we get the callchain:

   cpu_dev_init()
-> cpu_dev_register_generic()
-> register_cpu(cpu, i)
-> device_register(&cpu->dev)
-> device_add(dev)
-> device_add_class_symlinks(dev)

... in device_add_class_symlinks, we we dev->of_node, and call
sysfs_create_link(), which fails because we haven't called
of_core_init() to register the sysfs devicetree directory yet.

Given that, this patch makes sense to me.

FWIW, with the commit message updated to describe the particular
ordering problem:

Acked-by: Mark Rutland <mark.rutland@arm.com>

Thanks,
Mark.

Re: [PATCH 03/17] base: fix order of OF initialization

[Frank Rowand]

On 06/08/17 02:05, Mark Rutland wrote:
> On Wed, Jun 07, 2017 at 08:49:43PM -0700, Frank Rowand wrote:
>> On 06/07/17 11:39, Wesley Terpstra wrote:
>>> It was a while ago that I debugged this. I already reported this bug
>>> to Benjamin Herrenschmidt (now in CC), and I believe he has a patch of
>>> his own to fix the same issue.
>>>
>>> As I understand it, of_core_init sets up the OF entries in
>>> /sys/firmware/devicetree. During platform bringup, when the system
>>> describes the cpu + cache hierarchy, it also makes an of_node symlink
>>> into that directory. However, if it doesn't exist yet, you get the
>>> warning.
>>>
>>> # ls -l /sys/devices/system/cpu/cpu3/of_node
>>> lrwxrwxrwx    1 root     root             0 Jan  1 00:00
>>> /sys/devices/system/cpu/cpu3/of_node ->
>>> ../../../../firmware/devicetree/base/cpus/cpu@3
>>>
>>> On Wed, Jun 7, 2017 at 2:35 AM, Mark Rutland <mark.rutland@arm.com> wrote:
>>>> On Wed, Jun 07, 2017 at 09:07:20AM +0200, Geert Uytterhoeven wrote:
>>>>> CC devicetree folks
>>>>>
>>>>> On Wed, Jun 7, 2017 at 12:59 AM, Palmer Dabbelt <palmer@dabbelt.com> wrote:
>>>>>> From: "Wesley W. Terpstra" <wesley@sifive.com>
>>>>>>
>>>>>> This fixes: [    0.010000] cpu cpu0: Error -2 creating of_node link
>>>>>> ... which you get for every CPU on all architectures with a OF cpu/ node.
>>>>
>>>> I take it this means a /cpus node? Or the /cpus/cpu@* nodes?
>>>>
>>>> I'm not seeing this on arm64 when booting v4.12-rc4 with DT, so clearly
>>>> this doesn't affect all such architectures.
>>>>
>>>> What path are these errors happening in?
>>
>> On the surface, the patch looks reasonable.  But it is not obvious to me why
>> the error message is occurring.  I would like to understand the cause before
>> saying the patch is good.
>>
>> What kernel version is showing the error?  For a specific architecture
>> (the patch lists 'riscv, nios, etc'), which config and device tree source?
>>
>> And again, what is the calling path?
> 
>>From having grepped around, I think this affects architectures which
> select CONFIG_GENERIC_CPU_DEVICES, which includes nios2.

Thanks Mark!  The "#ifdef CONFIG_GENERIC_CPU_DEVICES" in cpu_dev_register_generic()
explains why we don't see the error on ARM, ARM64, etc.  Without the CONFIG
option, register_cpu(cpu, i) is not called.


> In that case, driver_init() calls cpu_dev_init() before calling
> of_core_init(). Then we get the callchain:
> 
>    cpu_dev_init()
> -> cpu_dev_register_generic()
> -> register_cpu(cpu, i)
> -> device_register(&cpu->dev)
> -> device_add(dev)
> -> device_add_class_symlinks(dev)
> 
> ... in device_add_class_symlinks, we we dev->of_node, and call
> sysfs_create_link(), which fails because we haven't called
> of_core_init() to register the sysfs devicetree directory yet.
> 
> Given that, this patch makes sense to me.
> 
> FWIW, with the commit message updated to describe the particular
> ordering problem:
> 
> Acked-by: Mark Rutland <mark.rutland@arm.com>

Agree with Mark's request to update the commit message, and also
I would like to see this shaken out in the -next tree.

Acked-by: Frank Rowand <frowand.list@gmail.com>

> 
> Thanks,
> Mark.
> 

Re: [PATCH 08/17] dts: include documentation for the RISC-V interrupt controllers

[Geert Uytterhoeven]

CC irqchip and devicetree folks

On Wed, Jun 7, 2017 at 12:59 AM, Palmer Dabbelt <palmer@dabbelt.com> wrote:
> From: "Wesley W. Terpstra" <wesley@sifive.com>
>
> Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
> ---
>  .../interrupt-controller/riscv,cpu-intc.txt        | 46 ++++++++++++++++++++++
>  .../bindings/interrupt-controller/riscv,plic0.txt  | 44 +++++++++++++++++++++
>  2 files changed, 90 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/riscv,cpu-intc.txt
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/riscv,plic0.txt
>
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/riscv,cpu-intc.txt b/Documentation/devicetree/bindings/interrupt-controller/riscv,cpu-intc.txt
> new file mode 100644
> index 000000000000..62f02e834ff9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/riscv,cpu-intc.txt
> @@ -0,0 +1,46 @@
> +RISC-V Hart-Level Interrupt Controller (HLIC)
> +---------------------------------------------
> +
> +RISC-V cores include Control Status Registers (CSRs) which are local to each
> +hart and can be read or written by software. Some of these CSRs are used to
> +control local interrupts connected to the core.
> +
> +Typical examples of local interrupts on a RISC-V core include: software IPI
> +interrupts, timer interrupts, and a link to the PLIC interrupt controller.
> +
> +Required properties:
> +- compatible : "riscv,cpu-intc"
> +- #interrupt-cells : should be <1>
> +- interrupt-controller : Identifies the node as an interrupt controller
> +
> +Furthermore, this interrupt-controller MUST be embedded inside the cpu
> +definition of the hart whose CSRs control these local interrupts.
> +
> +Example:
> +
> +       cpu1: cpu@1 {
> +               clock-frequency = <1600000000>;
> +               compatible = "riscv";
> +               d-cache-block-size = <64>;
> +               d-cache-sets = <64>;
> +               d-cache-size = <16384>;
> +               d-tlb-sets = <1>;
> +               d-tlb-size = <32>;
> +               device_type = "cpu";
> +               i-cache-block-size = <64>;
> +               i-cache-sets = <64>;
> +               i-cache-size = <16384>;
> +               i-tlb-sets = <1>;
> +               i-tlb-size = <32>;
> +               mmu-type = "riscv,sv39";
> +               next-level-cache = <&L2>;
> +               reg = <1>;
> +               riscv,isa = "rv64imac";
> +               status = "okay";
> +               tlb-split;
> +               cpu1-intc: interrupt-controller {
> +                       #interrupt-cells = <1>;
> +                       compatible = "riscv,cpu-intc";
> +                       interrupt-controller;
> +               };
> +       };
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/riscv,plic0.txt b/Documentation/devicetree/bindings/interrupt-controller/riscv,plic0.txt
> new file mode 100644
> index 000000000000..c05b5806f7d2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/riscv,plic0.txt
> @@ -0,0 +1,44 @@
> +RISC-V Platform-Level Interrupt Controller (PLIC)
> +-------------------------------------------------
> +
> +RISC-V cores typically include a PLIC, which route interrupts from multiple
> +devices to multiple hart contexts.  The PLIC is connected to the interrupt
> +controller embedded in a RISC-V core via the interrupt-related CSRs.
> +
> +A hart context is a priviledge mode in a hardware execution thread.  For
> +example, in an 4 core system with 2-way SMT, you have 8 harts and probably
> +at least two priviledge modes per hart; machine mode and supervisor mode.
> +
> +Each interrupt can be enabled on per-context basis. Any context can claim
> +a pending enabled interrupt and then release it once it has been handled.
> +
> +Each interrupt has a configurable priority. Higher priority interrupts are
> +serviced firs. Each context can specify a priority threshold. Interrupts
> +with priority below this threshold will not cause the PLIC to raise its
> +interrupt line leading to the context.
> +
> +Required properties:
> +- compatible : "riscv,plic0"
> +- #address-cells : should be <0>
> +- #interrupt-cells : should be <1>
> +- interrupt-controller : Identifies the node as an interrupt controller
> +- reg : Should contain 1 register range (address and length)
> +- riscv,ndev : Specifies the number of interrupts attached to the PLIC
> +- interrupts-extended : Specifies which contexts are connected to the PLIC
> +
> +Example:
> +
> +       plic: interrupt-controller@c000000 {
> +               #address-cells = <0>;
> +               #interrupt-cells = <1>;
> +               compatible = "riscv,plic0";
> +               interrupt-controller;
> +               interrupts-extended = <
> +                       &cpu0-intc 11
> +                       &cpu1-intc 11 &cpu1-intc 9
> +                       &cpu2-intc 11 &cpu2-intc 9
> +                       &cpu3-intc 11 &cpu3-intc 9
> +                       &cpu4-intc 11 &cpu4-intc 9>;
> +               reg = <0xc000000 0x4000000>;
> +               riscv,ndev = <10>;
> +       };
> --
> 2.13.0

Re: [PATCH 08/17] dts: include documentation for the RISC-V interrupt controllers

[Mark Rutland]

On Wed, Jun 07, 2017 at 09:11:31AM +0200, Geert Uytterhoeven wrote:
> CC irqchip and devicetree folks

Thanks Geert.

Palmer, in future, you can ensure (most) relevant parties are Cc'd by
using scripts/get_maintainer.pl to find them, and adding Cc: lines to
the relevant patches.

You can either hand that a patch or a path, e.g.

[mark@leverpostej:~/src/linux]% ./scripts/get_maintainer.pl -f Documentation/devicetree/bindings/interrupt-controller/                  
Thomas Gleixner <tglx@linutronix.de> (maintainer:IRQCHIP DRIVERS)
Jason Cooper <jason@lakedaemon.net> (maintainer:IRQCHIP DRIVERS)
Marc Zyngier <marc.zyngier@arm.com> (maintainer:IRQCHIP DRIVERS)
Rob Herring <robh+dt@kernel.org> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS)
Mark Rutland <mark.rutland@arm.com> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS)
linux-kernel@vger.kernel.org (open list:IRQCHIP DRIVERS)
devicetree@vger.kernel.org (open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS)

Otherwise, I have a few comments inline below.

> On Wed, Jun 7, 2017 at 12:59 AM, Palmer Dabbelt <palmer@dabbelt.com> wrote:
> > From: "Wesley W. Terpstra" <wesley@sifive.com>
> >
> > Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
> > ---
> >  .../interrupt-controller/riscv,cpu-intc.txt        | 46 ++++++++++++++++++++++
> >  .../bindings/interrupt-controller/riscv,plic0.txt  | 44 +++++++++++++++++++++
> >  2 files changed, 90 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/riscv,cpu-intc.txt
> >  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/riscv,plic0.txt
> >
> > diff --git a/Documentation/devicetree/bindings/interrupt-controller/riscv,cpu-intc.txt b/Documentation/devicetree/bindings/interrupt-controller/riscv,cpu-intc.txt
> > new file mode 100644
> > index 000000000000..62f02e834ff9
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/interrupt-controller/riscv,cpu-intc.txt
> > @@ -0,0 +1,46 @@
> > +RISC-V Hart-Level Interrupt Controller (HLIC)
> > +---------------------------------------------
> > +
> > +RISC-V cores include Control Status Registers (CSRs) which are local to each
> > +hart and can be read or written by software. Some of these CSRs are used to
> > +control local interrupts connected to the core.
> > +
> > +Typical examples of local interrupts on a RISC-V core include: software IPI
> > +interrupts, timer interrupts, and a link to the PLIC interrupt controller.

So IIUC those interrupts are routed directly to the HLIC, and are (only)
controlled thought the HLIC?

Is the HLIC architecturally mandated? i.e. is this guaranteed to be
present on any RISC-V implementation?

Does the presence of the HLIC imply the presence of a PLIC (or
vice/versa)? Typically, the per-cpu and platform-wide parts of the
top-level interrupt controller are fairly intimately coupled.

> > +
> > +Required properties:
> > +- compatible : "riscv,cpu-intc"

You'll need to allocate the "riscv" vendor prefix in 
Documentation/devicetree/bindings/vendor-prefixes.txt

... if that was done in some other patch, I didn't receive it.

> > +- #interrupt-cells : should be <1>

What about the flags?

Are all HLIC interrupts edge triggered (or level triggered)?

> > +- interrupt-controller : Identifies the node as an interrupt controller
> > +
> > +Furthermore, this interrupt-controller MUST be embedded inside the cpu
> > +definition of the hart whose CSRs control these local interrupts.
> > +
> > +Example:
> > +
> > +       cpu1: cpu@1 {
> > +               clock-frequency = <1600000000>;
> > +               compatible = "riscv";
> > +               d-cache-block-size = <64>;
> > +               d-cache-sets = <64>;
> > +               d-cache-size = <16384>;
> > +               d-tlb-sets = <1>;
> > +               d-tlb-size = <32>;
> > +               device_type = "cpu";
> > +               i-cache-block-size = <64>;
> > +               i-cache-sets = <64>;
> > +               i-cache-size = <16384>;
> > +               i-tlb-sets = <1>;
> > +               i-tlb-size = <32>;
> > +               mmu-type = "riscv,sv39";
> > +               next-level-cache = <&L2>;
> > +               reg = <1>;
> > +               riscv,isa = "rv64imac";
> > +               status = "okay";
> > +               tlb-split;

We can probably replace most of these with a "...", as they're largely
irrelevany to this binding.

> > +               cpu1-intc: interrupt-controller {
> > +                       #interrupt-cells = <1>;
> > +                       compatible = "riscv,cpu-intc";
> > +                       interrupt-controller;
> > +               };
> > +       };
> > diff --git a/Documentation/devicetree/bindings/interrupt-controller/riscv,plic0.txt b/Documentation/devicetree/bindings/interrupt-controller/riscv,plic0.txt
> > new file mode 100644
> > index 000000000000..c05b5806f7d2
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/interrupt-controller/riscv,plic0.txt
> > @@ -0,0 +1,44 @@
> > +RISC-V Platform-Level Interrupt Controller (PLIC)
> > +-------------------------------------------------
> > +
> > +RISC-V cores typically include a PLIC, which route interrupts from multiple
> > +devices to multiple hart contexts.  The PLIC is connected to the interrupt
> > +controller embedded in a RISC-V core via the interrupt-related CSRs.

Do you mean that the PLIC is connected to the HLIC, or that the HLIC is
also managed in part through CSRs?

> > +
> > +A hart context is a priviledge mode in a hardware execution thread.  For
> > +example, in an 4 core system with 2-way SMT, you have 8 harts and probably
> > +at least two priviledge modes per hart; machine mode and supervisor mode.
> > +
> > +Each interrupt can be enabled on per-context basis. Any context can claim
> > +a pending enabled interrupt and then release it once it has been handled.
> > +
> > +Each interrupt has a configurable priority. Higher priority interrupts are
> > +serviced firs. Each context can specify a priority threshold. Interrupts
> > +with priority below this threshold will not cause the PLIC to raise its
> > +interrupt line leading to the context.
> > +
> > +Required properties:
> > +- compatible : "riscv,plic0"
> > +- #address-cells : should be <0>
> > +- #interrupt-cells : should be <1>

As with the HLIC, what about the flags?

> > +- interrupt-controller : Identifies the node as an interrupt controller
> > +- reg : Should contain 1 register range (address and length)
> > +- riscv,ndev : Specifies the number of interrupts attached to the PLIC

Why do we need to know this?

I suspect this ia actually the number of interrupts implemented in the
PLIC, rather than the number of interrupts attached. i.e. the PLIC can
be implemented with a subset of the potential registers/bits. Is that
correct?

If so, something like "riscv,num-interrupts" would be better, along with
a clearer description.

> > +- interrupts-extended : Specifies which contexts are connected to the PLIC

That description doesn't sound right.

I take it that these are the HLIC interrupts that the PLIC can raise?

You will need to be explicit about the order of interrupts in this
property. i.e. which interrupt is routed to which context?

Is the interrupt at the HLIC well known? From the example I see that
here local interrupts 11 adn 9 are used. Is that mandated, or just the
case for this particular implementation?

Also, please consider how you will handle the case when the Linux
logical CPU ID is not the same as the physical ID, and how you will
handle physical IDs being sparse.

We went though a lot of pain trying to do something similar for the ARM
PMU interrupts (see the interrupt-affinity property in
Documentation/devicetree/bindings/arm/pmu.txt), and it's still painful
to deal with.

> > +
> > +Example:
> > +
> > +       plic: interrupt-controller@c000000 {
> > +               #address-cells = <0>;

This can go, given you don't have sub-nodes, nor a #size-cells property.

Thanks,
Mark.

> > +               #interrupt-cells = <1>;
> > +               compatible = "riscv,plic0";
> > +               interrupt-controller;
> > +               interrupts-extended = <
> > +                       &cpu0-intc 11
> > +                       &cpu1-intc 11 &cpu1-intc 9
> > +                       &cpu2-intc 11 &cpu2-intc 9
> > +                       &cpu3-intc 11 &cpu3-intc 9
> > +                       &cpu4-intc 11 &cpu4-intc 9>;
> > +               reg = <0xc000000 0x4000000>;
> > +               riscv,ndev = <10>;
> > +       };
> > --
> > 2.13.0

Re: [PATCH 08/17] dts: include documentation for the RISC-V interrupt controllers

[Wesley Terpstra]

On Wed, Jun 7, 2017 at 3:13 AM, Mark Rutland <mark.rutland@arm.com> wrote:
>> > +RISC-V Hart-Level Interrupt Controller (HLIC)
>> > +---------------------------------------------
>> > +
>> > +RISC-V cores include Control Status Registers (CSRs) which are local to each
>> > +hart and can be read or written by software. Some of these CSRs are used to
>> > +control local interrupts connected to the core.
>> > +
>> > +Typical examples of local interrupts on a RISC-V core include: software IPI
>> > +interrupts, timer interrupts, and a link to the PLIC interrupt controller.
>
> So IIUC those interrupts are routed directly to the HLIC, and are (only)
> controlled thought the HLIC?

Yes. You can have a local interrupt that goes directly to a specific
core, not via the PLIC.

> Is the HLIC architecturally mandated? i.e. is this guaranteed to be
> present on any RISC-V implementation?

It's in the RISC-V privileged specification. Therefore, if a riscv
core can run linux it will have these CSRs.

> Does the presence of the HLIC imply the presence of a PLIC (or
> vice/versa)?

No. SiFive implementations always have a PLIC, though.

> Typically, the per-cpu and platform-wide parts of the
> top-level interrupt controller are fairly intimately coupled.

They are coupled if they both exist. The privileged specification does
explicitly call out interrupts 9 and 11 in the HLIC for attaching the
PLIC.

> You'll need to allocate the "riscv" vendor prefix in
> Documentation/devicetree/bindings/vendor-prefixes.txt

@palmer: Can you add this?

> What about the flags?

What flags?

> Are all HLIC interrupts edge triggered (or level triggered)?

HLIC = level. PLIC = both.

> We can probably replace most of these with a "...", as they're largely
> irrelevany to this binding.

Sure. I thought it would be nice to include a complete cpu example
somewhere, though.

>> > +RISC-V cores typically include a PLIC, which route interrupts from multiple
>> > +devices to multiple hart contexts.  The PLIC is connected to the interrupt
>> > +controller embedded in a RISC-V core via the interrupt-related CSRs.
>
> Do you mean that the PLIC is connected to the HLIC, or that the HLIC is
> also managed in part through CSRs?

Both. The HLIC is entirely manager through CSRs. The PLIC is managed
through a memory mapped interface. The PLIC is attached to the HLIC.

>> > +Required properties:
>> > +- compatible : "riscv,plic0"
>> > +- #address-cells : should be <0>
>> > +- #interrupt-cells : should be <1>
>
> As with the HLIC, what about the flags?

Still unsure what flags we're talking about.

>> > +- riscv,ndev : Specifies the number of interrupts attached to the PLIC
>
> Why do we need to know this?
>
> I suspect this ia actually the number of interrupts implemented in the
> PLIC, rather than the number of interrupts attached. i.e. the PLIC can
> be implemented with a subset of the potential registers/bits. Is that
> correct?

You're in principle correct, although these are probably always the same.

> If so, something like "riscv,num-interrupts" would be better, along with
> a clearer description.

Uhm. I suppose we can change this. However, it would requires changes
to quite a number of riscv repositories. I believe this is also
included in the riscv platform specification. A better description is
easy, do we really need to change the key?


>> > +- interrupts-extended : Specifies which contexts are connected to the PLIC
>
> That description doesn't sound right.
>
> I take it that these are the HLIC interrupts that the PLIC can raise?

Yes.

> You will need to be explicit about the order of interrupts in this
> property. i.e. which interrupt is routed to which context?

Yes. Order and position matter.

> Is the interrupt at the HLIC well known? From the example I see that
> here local interrupts 11 adn 9 are used. Is that mandated, or just the
> case for this particular implementation?

9 and 11 are in the privileged specification.

> Also, please consider how you will handle the case when the Linux
> logical CPU ID is not the same as the physical ID, and how you will
> handle physical IDs being sparse.

We already deal with this. If the interrupt is '-1', we skip it.
That's done in plic.c:
                if (parent.args[0] == -1) continue; // skip context holes

>> > +       plic: interrupt-controller@c000000 {
>> > +               #address-cells = <0>;
>
> This can go, given you don't have sub-nodes, nor a #size-cells property.

The device-tree-specification seems to indicate that this is mandatory
for an interrupt-controller. Or have I understood this wrongly? When
you use interrupts-extended, doesn't it use the address-cells of the
interrupt controller? We should add that size-cells = 0, though.

Re: [PATCH 08/17] dts: include documentation for the RISC-V interrupt controllers

[Rob Herring]

On Wed, Jun 7, 2017 at 1:57 PM, Wesley Terpstra <wesley@sifive.com> wrote:
> On Wed, Jun 7, 2017 at 3:13 AM, Mark Rutland <mark.rutland@arm.com> wrote:
>>> > +RISC-V Hart-Level Interrupt Controller (HLIC)
>>> > +---------------------------------------------

[...]

>>> > +       plic: interrupt-controller@c000000 {
>>> > +               #address-cells = <0>;
>>
>> This can go, given you don't have sub-nodes, nor a #size-cells property.
>
> The device-tree-specification seems to indicate that this is mandatory
> for an interrupt-controller. Or have I understood this wrongly? When
> you use interrupts-extended, doesn't it use the address-cells of the
> interrupt controller? We should add that size-cells = 0, though.

It's only needed if you have an interrupt-map property AIUI.
#size-cells should never be needed (unless you have child nodes of
this one).

Rob

Re: [PATCH 08/17] dts: include documentation for the RISC-V interrupt controllers

[Wesley Terpstra]

I've reread the relevant sections now, and you are correct. We should
remove the address-cells from the PLIC's dts.

On Wed, Jun 7, 2017 at 12:57 PM, Rob Herring <robh+dt@kernel.org> wrote:
> On Wed, Jun 7, 2017 at 1:57 PM, Wesley Terpstra <wesley@sifive.com> wrote:
>> On Wed, Jun 7, 2017 at 3:13 AM, Mark Rutland <mark.rutland@arm.com> wrote:
>>>> > +RISC-V Hart-Level Interrupt Controller (HLIC)
>>>> > +---------------------------------------------
>
> [...]
>
>>>> > +       plic: interrupt-controller@c000000 {
>>>> > +               #address-cells = <0>;
>>>
>>> This can go, given you don't have sub-nodes, nor a #size-cells property.
>>
>> The device-tree-specification seems to indicate that this is mandatory
>> for an interrupt-controller. Or have I understood this wrongly? When
>> you use interrupts-extended, doesn't it use the address-cells of the
>> interrupt controller? We should add that size-cells = 0, though.
>
> It's only needed if you have an interrupt-map property AIUI.
> #size-cells should never be needed (unless you have child nodes of
> this one).
>
> Rob

Re: [PATCH 08/17] dts: include documentation for the RISC-V interrupt controllers

[Mark Rutland]

On Wed, Jun 07, 2017 at 11:57:17AM -0700, Wesley Terpstra wrote:
> On Wed, Jun 7, 2017 at 3:13 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> >> > +RISC-V Hart-Level Interrupt Controller (HLIC)
> >> > +---------------------------------------------
> >> > +
> >> > +RISC-V cores include Control Status Registers (CSRs) which are local to each
> >> > +hart and can be read or written by software. Some of these CSRs are used to
> >> > +control local interrupts connected to the core.
> >> > +
> >> > +Typical examples of local interrupts on a RISC-V core include: software IPI
> >> > +interrupts, timer interrupts, and a link to the PLIC interrupt controller.
> >
> > So IIUC those interrupts are routed directly to the HLIC, and are (only)
> > controlled thought the HLIC?
> 
> Yes. You can have a local interrupt that goes directly to a specific
> core, not via the PLIC.
> 
> > Is the HLIC architecturally mandated? i.e. is this guaranteed to be
> > present on any RISC-V implementation?
> 
> It's in the RISC-V privileged specification. Therefore, if a riscv
> core can run linux it will have these CSRs.
> 
> > Does the presence of the HLIC imply the presence of a PLIC (or
> > vice/versa)?
> 
> No. SiFive implementations always have a PLIC, though.
> 
> > Typically, the per-cpu and platform-wide parts of the
> > top-level interrupt controller are fairly intimately coupled.
> 
> They are coupled if they both exist. The privileged specification does
> explicitly call out interrupts 9 and 11 in the HLIC for attaching the
> PLIC.
> 
> > You'll need to allocate the "riscv" vendor prefix in
> > Documentation/devicetree/bindings/vendor-prefixes.txt
> 
> @palmer: Can you add this?
> 
> > What about the flags?
> 
> What flags?

Edge vs level, active high vs active low. Typically some of these are
programmable, and are described as flags in the interrupt-specifier.

See the examples in:

Documentation/devicetree/bindings/interrupt-controller/interrupts.txt

> > Are all HLIC interrupts edge triggered (or level triggered)?
> 
> HLIC = level. PLIC = both.

Ok. Given that, flags aren't necessary for the HLIC, and the
interrupt-specifier is fine as-is.

> >> > +RISC-V cores typically include a PLIC, which route interrupts from multiple
> >> > +devices to multiple hart contexts.  The PLIC is connected to the interrupt
> >> > +controller embedded in a RISC-V core via the interrupt-related CSRs.
> >
> > Do you mean that the PLIC is connected to the HLIC, or that the HLIC is
> > also managed in part through CSRs?
> 
> Both. The HLIC is entirely manager through CSRs. The PLIC is managed
> through a memory mapped interface. The PLIC is attached to the HLIC.

So do any CSRs affect the state of the PLIC? If it's just MMIO, the
mention of CSRs above is just a little confusing.

It might be best to just say "The PLIC is connect to the HLIC embedded
in each RISC-V core".

> >> > +Required properties:
> >> > +- compatible : "riscv,plic0"
> >> > +- #address-cells : should be <0>
> >> > +- #interrupt-cells : should be <1>
> >
> > As with the HLIC, what about the flags?
> 
> Still unsure what flags we're talking about.

As covered above for the HLIC.

It sounds like we'd need these to distinguish edge/level interrupts,
unless that's fixed at integration time and you can determine it from
the PLIC itself?

> >> > +- riscv,ndev : Specifies the number of interrupts attached to the PLIC
> >
> > Why do we need to know this?
> >
> > I suspect this ia actually the number of interrupts implemented in the
> > PLIC, rather than the number of interrupts attached. i.e. the PLIC can
> > be implemented with a subset of the potential registers/bits. Is that
> > correct?
> 
> You're in principle correct, although these are probably always the same.

For now, perhaps. Let's not embed an assumption we cannot guarantee.

> > If so, something like "riscv,num-interrupts" would be better, along with
> > a clearer description.
> 
> Uhm. I suppose we can change this. However, it would requires changes
> to quite a number of riscv repositories. I believe this is also
> included in the riscv platform specification. A better description is
> easy, do we really need to change the key?

It's not too much of a problem, but if we end up having to change
anything else from the proposed bindings, those trees are going to
require updates anyway.

If we can, I would like to change this to keep things as clear as
possible from the outset.

[...]

> > You will need to be explicit about the order of interrupts in this
> > property. i.e. which interrupt is routed to which context?
> 
> Yes. Order and position matter.

Ok.

Please update the binding to explicitly define the ordering requirement.

[...]

> > Also, please consider how you will handle the case when the Linux
> > logical CPU ID is not the same as the physical ID, and how you will
> > handle physical IDs being sparse.
> 
> We already deal with this. If the interrupt is '-1', we skip it.
> That's done in plic.c:
>                 if (parent.args[0] == -1) continue; // skip context holes

If this is what we expect people to do, it needs to be documented in the
binding.

Does this mean that you expect Linux logical CPU IDs to be equal to
physical CPU IDs in all cases?

That's going to be painful for very sparse ID ranges, and won't work for
cases like kexec/kdump where you cannot guarantee which physical CPU
will end up being CPU0 in the new kernel.

I would strongly advise that you explicitly describe the relationship
using phandles to CPU nodes.

I would also advise that you try to decouple the physical CPU IDs and
Linux logical IDs. While assuming they're the same might simplify things
today, it will create longer term maintenance problems and get in the
way of a number of features.

Thanks,
Mark.

Re: [PATCH 08/17] dts: include documentation for the RISC-V interrupt controllers

[Wesley Terpstra]

On Thu, Jun 8, 2017 at 3:52 AM, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
>> What flags?
>
> Edge vs level, active high vs active low. Typically some of these are
> programmable, and are described as flags in the interrupt-specifier.
>
> See the examples in:
>
> Documentation/devicetree/bindings/interrupt-controller/interrupts.txt

Ok. Those are not applicable to the PLIC.

>
>> > Are all HLIC interrupts edge triggered (or level triggered)?
>>
>> HLIC = level. PLIC = both.
>
> Ok. Given that, flags aren't necessary for the HLIC, and the
> interrupt-specifier is fine as-is.
>
>> >> > +RISC-V cores typically include a PLIC, which route interrupts from multiple
>> >> > +devices to multiple hart contexts.  The PLIC is connected to the interrupt
>> >> > +controller embedded in a RISC-V core via the interrupt-related CSRs.
>> >
>> > Do you mean that the PLIC is connected to the HLIC, or that the HLIC is
>> > also managed in part through CSRs?
>>
>> Both. The HLIC is entirely manager through CSRs. The PLIC is managed
>> through a memory mapped interface. The PLIC is attached to the HLIC.
>
> So do any CSRs affect the state of the PLIC? If it's just MMIO, the
> mention of CSRs above is just a little confusing.
>
> It might be best to just say "The PLIC is connect to the HLIC embedded
> in each RISC-V core".
>
>> >> > +Required properties:
>> >> > +- compatible : "riscv,plic0"
>> >> > +- #address-cells : should be <0>
>> >> > +- #interrupt-cells : should be <1>
>> >
>> > As with the HLIC, what about the flags?
>>
>> Still unsure what flags we're talking about.
>
> As covered above for the HLIC.
>
> It sounds like we'd need these to distinguish edge/level interrupts,
> unless that's fixed at integration time and you can determine it from
> the PLIC itself?
>
>> >> > +- riscv,ndev : Specifies the number of interrupts attached to the PLIC
>> >
>> > Why do we need to know this?
>> >
>> > I suspect this ia actually the number of interrupts implemented in the
>> > PLIC, rather than the number of interrupts attached. i.e. the PLIC can
>> > be implemented with a subset of the potential registers/bits. Is that
>> > correct?
>>
>> You're in principle correct, although these are probably always the same.
>
> For now, perhaps. Let's not embed an assumption we cannot guarantee.
>
>> > If so, something like "riscv,num-interrupts" would be better, along with
>> > a clearer description.
>>
>> Uhm. I suppose we can change this. However, it would requires changes
>> to quite a number of riscv repositories. I believe this is also
>> included in the riscv platform specification. A better description is
>> easy, do we really need to change the key?
>
> It's not too much of a problem, but if we end up having to change
> anything else from the proposed bindings, those trees are going to
> require updates anyway.
>
> If we can, I would like to change this to keep things as clear as
> possible from the outset.
>
> [...]
>
>> > You will need to be explicit about the order of interrupts in this
>> > property. i.e. which interrupt is routed to which context?
>>
>> Yes. Order and position matter.
>
> Ok.
>
> Please update the binding to explicitly define the ordering requirement.
>
> [...]
>
>> > Also, please consider how you will handle the case when the Linux
>> > logical CPU ID is not the same as the physical ID, and how you will
>> > handle physical IDs being sparse.
>>
>> We already deal with this. If the interrupt is '-1', we skip it.
>> That's done in plic.c:
>>                 if (parent.args[0] == -1) continue; // skip context holes
>
> If this is what we expect people to do, it needs to be documented in the
> binding.
>
> Does this mean that you expect Linux logical CPU IDs to be equal to
> physical CPU IDs in all cases?
>
> That's going to be painful for very sparse ID ranges, and won't work for
> cases like kexec/kdump where you cannot guarantee which physical CPU
> will end up being CPU0 in the new kernel.
>
> I would strongly advise that you explicitly describe the relationship
> using phandles to CPU nodes.
>
> I would also advise that you try to decouple the physical CPU IDs and
> Linux logical IDs. While assuming they're the same might simplify things
> today, it will create longer term maintenance problems and get in the
> way of a number of features.
>
> Thanks,
> Mark.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 08/17] dts: include documentation for the RISC-V interrupt controllers

[Wesley Terpstra]

Ugh. Clicked reply without being done writing the reply!

On Thu, Jun 8, 2017 at 3:52 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> Edge vs level, active high vs active low. Typically some of these are
> programmable, and are described as flags in the interrupt-specifier.
>
> See the examples in:
>
> Documentation/devicetree/bindings/interrupt-controller/interrupts.txt

Ok. Those are not really relevant to the PLIC. It has a flat interrupt
namespace and the way you handle all four kinds of interrupts in the
driver is uniform. I don't think we want to architecturally expose
this to the operating system.

> So do any CSRs affect the state of the PLIC? If it's just MMIO, the
> mention of CSRs above is just a little confusing.

HLIC = CSR only. PLIC = MMIO only.

> It might be best to just say "The PLIC is connect to the HLIC embedded
> in each RISC-V core".

Fair enough.

> It sounds like we'd need these to distinguish edge/level interrupts,
> unless that's fixed at integration time and you can determine it from
> the PLIC itself?

They are fixed at integration time and the PLIC driver does not need to care.

> It's not too much of a problem, but if we end up having to change
> anything else from the proposed bindings, those trees are going to
> require updates anyway.

I'll talk to the other stakeholders.

> Please update the binding to explicitly define the ordering requirement.

The ordering requirement is that the first interrupts-extended entry
corresponds to the first context, second to second, etc. If a context
is unused for some reason, that's when you need a -1. The contexts are
linear and contiguous in the MMIO address map.

> Does this mean that you expect Linux logical CPU IDs to be equal to
> physical CPU IDs in all cases?

No. There is no 'physical CPU ID' anyway, except the mhartid CSR which
is unavailable to linux. The SBI conveys your CPU ID by passing it as
the first argument to the linux kernel.

The interrupts-extended in the PLIC uses a phandle to reference the
matching CPU in DTS. The num in cpu@<num> only need correspond to the
first register argument to the kernel.

If for some reason there end up too many -1 holes in the PLIC (b/c you
virtualized a 128-core machine down to say 2), you can always
virtualize the PLIC device and provide a matching simplified DTB.

Re: [PATCH 08/17] dts: include documentation for the RISC-V interrupt controllers

[Mark Rutland]

On Fri, Jun 09, 2017 at 02:58:14PM -0700, Wesley Terpstra wrote:
> Ugh. Clicked reply without being done writing the reply!
> 
> On Thu, Jun 8, 2017 at 3:52 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> > Edge vs level, active high vs active low. Typically some of these are
> > programmable, and are described as flags in the interrupt-specifier.
> >
> > See the examples in:
> >
> > Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
> 
> Ok. Those are not really relevant to the PLIC. It has a flat interrupt
> namespace and the way you handle all four kinds of interrupts in the
> driver is uniform. I don't think we want to architecturally expose
> this to the operating system.

Sure thing. I was just making sure I clarified my initial statement.

> > So do any CSRs affect the state of the PLIC? If it's just MMIO, the
> > mention of CSRs above is just a little confusing.
> 
> HLIC = CSR only. PLIC = MMIO only.
> 
> > It might be best to just say "The PLIC is connect to the HLIC embedded
> > in each RISC-V core".
> 
> Fair enough.
> 
> > It sounds like we'd need these to distinguish edge/level interrupts,
> > unless that's fixed at integration time and you can determine it from
> > the PLIC itself?
> 
> They are fixed at integration time and the PLIC driver does not need to care.

Ok.

> > Please update the binding to explicitly define the ordering requirement.
> 
> The ordering requirement is that the first interrupts-extended entry
> corresponds to the first context, second to second, etc. If a context
> is unused for some reason, that's when you need a -1. The contexts are
> linear and contiguous in the MMIO address map.

Ah. It wasn't at all clear to me that the order was in relation to the
MMIO space.

How is it possible to tell if an context is unused?

Are those mandated to be linear and contiguous in the address space?

How is the relationship between an MMIO context and hart determined?

I'm not sure that it makes sense to use -1 in this manner, since the
first cell of each interrupts-extended tuple entry is a phandle. IIRC
zero is currently de-facto invalid, and if we really need a NULL
phandle, we should get that clarified in the DT spec.

It might be better to describe each MMIO context separately, and then
associate IRQs with those in-order.

> 
> > Does this mean that you expect Linux logical CPU IDs to be equal to
> > physical CPU IDs in all cases?
> 
> No. There is no 'physical CPU ID' anyway, except the mhartid CSR which
> is unavailable to linux. The SBI conveys your CPU ID by passing it as
> the first argument to the linux kernel.
> 
> The interrupts-extended in the PLIC uses a phandle to reference the
> matching CPU in DTS. The num in cpu@<num> only need correspond to the
> first register argument to the kernel.

So you use the reference to the HLIC to implictly provide the CPU
affinity? Or do you assume that these align with the SBI-provided CPU
IDs?

Thanks,
Mark.