Re: [PATCH] riscv: include generic support for MSI irqdomains

[Wesley Terpstra]

Signed.


On Tue, May 21, 2019 at 1:11 AM Paul Walmsley  wrote:
>
> On Mon, 20 May 2019, Christoph Hellwig wrote:
>
> > On Mon, May 20, 2019 at 11:25:28AM -0700, Paul Walmsley wrote:
> > > Some RISC-V systems include PCIe host controllers that support PCIe
> > > message-signaled interrupts.  For this to work on Linux, we need to
> > > enable PCI_MSI_IRQ_DOMAIN and define struct msi_alloc_info.  Support
> > > for the latter is enabled by including the architecture-generic msi.h
> > > include.
> > >
> > > Based on a patch from Wesley Terpstra :
> > >
> > > https://github.com/riscv/riscv-linux/commit/7d55f38fb79f459d2e88bcee7e147796400cafa8
> > >
> > > Signed-off-by: Paul Walmsley 
> > > Signed-off-by: Paul Walmsley 
> > > Cc: Wesley Terpstra 
> >
> > Well, this is very much Wes' patch as-is.  It should probably be
> > attributed to him and you should ask for his signoff.
>
> Yeah.  There aren't many other ways to do it.
>
> Wes, care to reply with your Signed-off-by: ?
>
>
> - Paul

Re: [RFC 1/4] pwm: sifive: Add DT documentation for SiFive PWM Controller.

[Wesley Terpstra]

On Mon, Oct 15, 2018 at 3:57 PM Atish Patra  wrote:
> >> +SiFive PWM controller
> >> +
> >> +Unlike most other PWM controllers, the SiFive PWM controller currently 
> >> only
> >> +supports one period for all channels in the PWM. This is set globally in 
> >> DTS.
> >> +The period also has significant restrictions on the values it can achieve,
> >> +which the driver rounds to the nearest achievable frequency.
> >
> > What restrictions are these? If "nearest achievable" is too far off the
> > target period it might be preferable to error out.
> >
>
> @Wes: Any comments?

When I last looked at this driver and hardware, I briefly considered
throwing up my hands and pretending that this PWM device had no period
control at all, only duty-cycle. There are several examples of PWM
controllers in linux already that behave that way.

Most of the uses of this PWM are only going to care about the
duty-cycle, not the period. So failing when the period is unachievable
seems worse to me than just completely eliminating access to period
control.

> I think yes. Since fu540 is the first Linux capable RISC-V core, SiFive
> Guys decided mark it as version 0.
>
> @Wesly: Please correct me if I am wrong.

Correct.

Re: [RFC 1/4] pwm: sifive: Add DT documentation for SiFive PWM Controller.

[Wesley Terpstra]

On Mon, Oct 15, 2018 at 3:57 PM Atish Patra  wrote:
> >> +SiFive PWM controller
> >> +
> >> +Unlike most other PWM controllers, the SiFive PWM controller currently 
> >> only
> >> +supports one period for all channels in the PWM. This is set globally in 
> >> DTS.
> >> +The period also has significant restrictions on the values it can achieve,
> >> +which the driver rounds to the nearest achievable frequency.
> >
> > What restrictions are these? If "nearest achievable" is too far off the
> > target period it might be preferable to error out.
> >
>
> @Wes: Any comments?

When I last looked at this driver and hardware, I briefly considered
throwing up my hands and pretending that this PWM device had no period
control at all, only duty-cycle. There are several examples of PWM
controllers in linux already that behave that way.

Most of the uses of this PWM are only going to care about the
duty-cycle, not the period. So failing when the period is unachievable
seems worse to me than just completely eliminating access to period
control.

> I think yes. Since fu540 is the first Linux capable RISC-V core, SiFive
> Guys decided mark it as version 0.
>
> @Wesly: Please correct me if I am wrong.

Correct.

Re: [PATCH 1/3] dt-bindings: added new pwm-sifive driver documentation

[Wesley Terpstra]

On Mon, Apr 30, 2018 at 1:27 AM, Thierry Reding
 wrote:
> I don't like the idea of specifying something in DT that is completely
> approximate because it doesn't give users any kind of control over what
> is considered acceptable. In some cases an approximation to within 10%
> might be acceptable, in other cases users may only want to allow 5%. In
> this case there's even no way to catch or report a deviation from the
> desired value.

My view was that you basically don't have period control on this IP.
You can give it a target that it tries to get as close to as possible,
but there's no guarantee of any kind wrt. the period.

I saw there were a couple other PWM drivers which had no period
control whatsoever. They just allowed duty cycle control. Because this
IP has such a broken period-control interface, I was essentially
trying to bin it in the same category as those drivers.

Perhaps I should just remove all pretense of supporting period and
just always default to the fastest period possible?

> How about you allow users to specify a valid frequency range with
> something like:
>
> frequency-range = ;
>
> or
>
> period-range = ;

Ok, but now you have to define what happens if a clock change prevents
you from staying within this range.

Rejecting the clock frequency change does not seem a good option for
the actual SoC for which I wrote this driver. There, all the PWM does
is drive an LED bank and clock changes are used to change the core
frequency.

> you could disable the PWM if it was fed with an invalid range.

Is that really desirable behavior? If the period is defined as being
best effort for this PWM IP, which is essentially what I've done, it's
clear you want the PWM to continue to operate. That's certainly the
behavior I want on the actual SoC with this IP.

I'll make all the DTS changes you guys suggested. ie: "-v0", clarified
clocks description, and remove unused interrupts comment.

Re: [PATCH 1/3] dt-bindings: added new pwm-sifive driver documentation

[Wesley Terpstra]

On Mon, Apr 30, 2018 at 1:27 AM, Thierry Reding
 wrote:
> I don't like the idea of specifying something in DT that is completely
> approximate because it doesn't give users any kind of control over what
> is considered acceptable. In some cases an approximation to within 10%
> might be acceptable, in other cases users may only want to allow 5%. In
> this case there's even no way to catch or report a deviation from the
> desired value.

My view was that you basically don't have period control on this IP.
You can give it a target that it tries to get as close to as possible,
but there's no guarantee of any kind wrt. the period.

I saw there were a couple other PWM drivers which had no period
control whatsoever. They just allowed duty cycle control. Because this
IP has such a broken period-control interface, I was essentially
trying to bin it in the same category as those drivers.

Perhaps I should just remove all pretense of supporting period and
just always default to the fastest period possible?

> How about you allow users to specify a valid frequency range with
> something like:
>
> frequency-range = ;
>
> or
>
> period-range = ;

Ok, but now you have to define what happens if a clock change prevents
you from staying within this range.

Rejecting the clock frequency change does not seem a good option for
the actual SoC for which I wrote this driver. There, all the PWM does
is drive an LED bank and clock changes are used to change the core
frequency.

> you could disable the PWM if it was fed with an invalid range.

Is that really desirable behavior? If the period is defined as being
best effort for this PWM IP, which is essentially what I've done, it's
clear you want the PWM to continue to operate. That's certainly the
behavior I want on the actual SoC with this IP.

I'll make all the DTS changes you guys suggested. ie: "-v0", clarified
clocks description, and remove unused interrupts comment.

Re: [PATCH 3/3] pwm-sifive: add a driver for SiFive SoC PWM

[Wesley Terpstra]

First of all, thank you very much for this detailed review! I really
appreciate it, as this is just the first driver of several we would
like to upstream and getting the reviews front-loaded like this will
really help us improve the subsequent drivers before trying to
upstream them.

On Mon, Apr 30, 2018 at 2:39 AM, Thierry Reding
 wrote:
> Please include a SPDX license identifier here. That's the preferred way
> to specify the license these days.

Done

>> +#include 
> There should be no need to include this in a driver.

Done

>> + if (state->polarity == PWM_POLARITY_NORMAL)
>> + state->duty_cycle = state->period - state->duty_cycle;
>
> That's not the right way to handle polarity. The above often has the
> same effect as inversed polarity, but it's not generally the right thing
> to do. Please only support polarity if your hardware can actually really
> reverse it. The above is something that PWM consumers (such as the
> backlight driver) should take care of.

I don't know how to declare non-support for polarity. How do I do that?

I still want DTS references to state whether or not the polarity
should be reversed, because the PWM might be connected with either
positive or negative logic to an LED, for example.

>> + state->polarity   = PWM_POLARITY_INVERSED;
> Is the polarity really reversed by default on this IP?

Yes. In the sense that the PWM is low while the counter is less than
the duty-cycle, and high when >= the duty-cycle.

Note that this also means it's possible to achieve a constant high
value, but impossible to achieve a constant low value, other than
approximate.

> I don't think this is a good idea. Nobody should be allowed to just
> change the PWM period without letting the PWM controller have a say in
> the matter.

I agree and I intend to fight hard to make sure that future chips don't do this.

> Is this ever really an issue?

Unfortunately, yes. This chip has only one PLL output that controls
almost everything. The core runs at the PLL's output. The bus runs at
half the PLL's output. Each peripheral clock is a divided down version
of the bus clock.

> Does the PWM controller use a shared clock that changes rate frequently?

When you want to change the CPU frequency, you have to update all the
peripheral device clock dividers. It sucks, but that's what has to
happen if the core clock is changed. Fortunately, this is not done
dynamically, but might be done during boot.

For PWM, this is not a big issue, because the period mostly does not
matter as it's broken already. However, for an active SPI transfer,
this can be problematic.

>> + ret = of_property_read_u32(node, "sifive,npwm", >npwm);
>> + if (ret < 0 || chip->npwm > MAX_PWM)
>> + chip->npwm = MAX_PWM;
>
> I don't see this documented in the DT bindings. I also don't see a need
> for it. Under what circumstances would you want to change this?

The PWM controller can have less than 4 channels. I could try to probe
how many there are by poking registers, but that seems risky if the
PWM is attached to something.

Shall I just add this to the dt-bindings?

>> + pwm->irq = platform_get_irq(pdev, 0);
>> + if (pwm->irq < 0) {
>> + dev_err(dev, "Unable to find interrupt\n");
>> + return pwm->irq;
>> + }
>
> You don't do anything with this, why bother retrieving it?

Mostly to ensure that it is included in the DTS so that I can rely on
it being there in any future driver updates.

>> + dev_info(dev, "SiFive PWM chip registered %d PWMs\n", chip->npwm);
> Please don't do this.

Removed.

Re: [PATCH 3/3] pwm-sifive: add a driver for SiFive SoC PWM

[Wesley Terpstra]

First of all, thank you very much for this detailed review! I really
appreciate it, as this is just the first driver of several we would
like to upstream and getting the reviews front-loaded like this will
really help us improve the subsequent drivers before trying to
upstream them.

On Mon, Apr 30, 2018 at 2:39 AM, Thierry Reding
 wrote:
> Please include a SPDX license identifier here. That's the preferred way
> to specify the license these days.

Done

>> +#include 
> There should be no need to include this in a driver.

Done

>> + if (state->polarity == PWM_POLARITY_NORMAL)
>> + state->duty_cycle = state->period - state->duty_cycle;
>
> That's not the right way to handle polarity. The above often has the
> same effect as inversed polarity, but it's not generally the right thing
> to do. Please only support polarity if your hardware can actually really
> reverse it. The above is something that PWM consumers (such as the
> backlight driver) should take care of.

I don't know how to declare non-support for polarity. How do I do that?

I still want DTS references to state whether or not the polarity
should be reversed, because the PWM might be connected with either
positive or negative logic to an LED, for example.

>> + state->polarity   = PWM_POLARITY_INVERSED;
> Is the polarity really reversed by default on this IP?

Yes. In the sense that the PWM is low while the counter is less than
the duty-cycle, and high when >= the duty-cycle.

Note that this also means it's possible to achieve a constant high
value, but impossible to achieve a constant low value, other than
approximate.

> I don't think this is a good idea. Nobody should be allowed to just
> change the PWM period without letting the PWM controller have a say in
> the matter.

I agree and I intend to fight hard to make sure that future chips don't do this.

> Is this ever really an issue?

Unfortunately, yes. This chip has only one PLL output that controls
almost everything. The core runs at the PLL's output. The bus runs at
half the PLL's output. Each peripheral clock is a divided down version
of the bus clock.

> Does the PWM controller use a shared clock that changes rate frequently?

When you want to change the CPU frequency, you have to update all the
peripheral device clock dividers. It sucks, but that's what has to
happen if the core clock is changed. Fortunately, this is not done
dynamically, but might be done during boot.

For PWM, this is not a big issue, because the period mostly does not
matter as it's broken already. However, for an active SPI transfer,
this can be problematic.

>> + ret = of_property_read_u32(node, "sifive,npwm", >npwm);
>> + if (ret < 0 || chip->npwm > MAX_PWM)
>> + chip->npwm = MAX_PWM;
>
> I don't see this documented in the DT bindings. I also don't see a need
> for it. Under what circumstances would you want to change this?

The PWM controller can have less than 4 channels. I could try to probe
how many there are by poking registers, but that seems risky if the
PWM is attached to something.

Shall I just add this to the dt-bindings?

>> + pwm->irq = platform_get_irq(pdev, 0);
>> + if (pwm->irq < 0) {
>> + dev_err(dev, "Unable to find interrupt\n");
>> + return pwm->irq;
>> + }
>
> You don't do anything with this, why bother retrieving it?

Mostly to ensure that it is included in the DTS so that I can rely on
it being there in any future driver updates.

>> + dev_info(dev, "SiFive PWM chip registered %d PWMs\n", chip->npwm);
> Please don't do this.

Removed.

Re: [PATCH 1/3] dt-bindings: added new pwm-sifive driver documentation

[Wesley Terpstra]

On Sun, Apr 29, 2018 at 2:01 PM, Andreas Färber  wrote:
> "pwm0" sounds like a zero-indexed instance of some pwm block. If 0 is
> the version here, I'd suggest to make it "pwm-0" for example - you might
> want to take a look at the Xilinx bindings, which use a strict x.yy suffix.

That's fine. I'll change it to pwm-0.00 in the next patch series.

> Most SoCs don't have clearly versioned IP though, that's why for
> community-contributed bindings the first SoC we encounter the IP in
> usually gets the name.

In this particular case, we do have versioned IP, and we will be
contributing drivers for those which wind up in linux-capable chips.

Re: [PATCH 1/3] dt-bindings: added new pwm-sifive driver documentation

[Wesley Terpstra]

On Sun, Apr 29, 2018 at 2:01 PM, Andreas Färber  wrote:
> "pwm0" sounds like a zero-indexed instance of some pwm block. If 0 is
> the version here, I'd suggest to make it "pwm-0" for example - you might
> want to take a look at the Xilinx bindings, which use a strict x.yy suffix.

That's fine. I'll change it to pwm-0.00 in the next patch series.

> Most SoCs don't have clearly versioned IP though, that's why for
> community-contributed bindings the first SoC we encounter the IP in
> usually gets the name.

In this particular case, we do have versioned IP, and we will be
contributing drivers for those which wind up in linux-capable chips.

Re: [PATCH 1/3] dt-bindings: added new pwm-sifive driver documentation

[Wesley Terpstra]

On Sat, Apr 28, 2018 at 10:54 PM, Thierry Reding
 wrote:
> On Fri, Apr 27, 2018 at 03:59:56PM -0700, Wesley W. Terpstra wrote:
>> +Unlike most other PWM controllers, the SiFive PWM controller currently only
>> +supports one period for all channels in the PWM. This is set globally in 
>> DTS.
>> +The period also has significant restrictions on the values it can achieve,
>> +which the driver rounds to the nearest achievable frequency.
>
> How about you encode this in the driver rather than DT? We have several
> drivers with similar restrictions and they usually allow the first PWM
> channel to choose an arbitrary period and return an error if subsequent
> channels request a period that can't be supported.
>
> I think something similar could work with that second restriction. Why
> not return an error if the exact period can't be set? Or perhaps allow
> some percentage of deviation.

Interesting. There are two ways to use this PWM. In one mode you use
all channels of the PWM as outputs. This is the mode the driver
supports because the HiFive Unleashed board uses all channels
connected to LEDs.

In this case, the PWM period must always be a power-of-two reduction
from the core bus clock frequency. The core bus clock frequency can
vary. Therefore, even if the caller's frequency can be achieved at the
time of the method call, it may not remain achievable. You might say
this is a ridiculous PWM design, and I'd agree with you, but sadly
this is what is found in these chips. So effectively, the only thing
we can guarantee is that the period is within a multiple of sqrt(2)
from the requested period, except even that is not true due to
saturation restrictions that could push the period even further from
what you ask.

In the other mode (where you sacrifice one of the output channels),
you have finer control of the period, but it still affects all
channels. This mode might be a better fit for your proposed API,
except that the driver would still be subject to saturation
restrictions on the period. And those restrictions will change as the
core bus frequency is changed, which means that again, even if your
target period can be achieved (common to all channels) at invocation,
it might not be achievable later.

IMO the only real control this PWM can offer reliably is the duty
cycle, which is why I've written it how I have.

If you see a better solution to the above problems, I am open to suggestions.

>> +Required properties:
>> +- compatible: should be "sifive,pwm0"
>
> Why not simply "sifive,pwm"? If this is supposed to be some sort of
> version number, then it is more customary to use the name of the first
> SoC that integrates the IP. There are some exceptions, like for example
> when the IP is third-party and is integrated in a number of different
> SoCs. In such cases the IP is often properly versioned. But that doesn't
> seem to be the case here.

It is indeed a version number. The first SoC which integrated this IP
cannot run linux. We've put a version number like this into all of our
IP blocks. Isn't an increasing number, which clearly indicates
increased functionality, better than a reference to a sequence of SoCs
whose relationships are not all that clear?

Re: [PATCH 1/3] dt-bindings: added new pwm-sifive driver documentation

[Wesley Terpstra]

On Sat, Apr 28, 2018 at 10:54 PM, Thierry Reding
 wrote:
> On Fri, Apr 27, 2018 at 03:59:56PM -0700, Wesley W. Terpstra wrote:
>> +Unlike most other PWM controllers, the SiFive PWM controller currently only
>> +supports one period for all channels in the PWM. This is set globally in 
>> DTS.
>> +The period also has significant restrictions on the values it can achieve,
>> +which the driver rounds to the nearest achievable frequency.
>
> How about you encode this in the driver rather than DT? We have several
> drivers with similar restrictions and they usually allow the first PWM
> channel to choose an arbitrary period and return an error if subsequent
> channels request a period that can't be supported.
>
> I think something similar could work with that second restriction. Why
> not return an error if the exact period can't be set? Or perhaps allow
> some percentage of deviation.

Interesting. There are two ways to use this PWM. In one mode you use
all channels of the PWM as outputs. This is the mode the driver
supports because the HiFive Unleashed board uses all channels
connected to LEDs.

In this case, the PWM period must always be a power-of-two reduction
from the core bus clock frequency. The core bus clock frequency can
vary. Therefore, even if the caller's frequency can be achieved at the
time of the method call, it may not remain achievable. You might say
this is a ridiculous PWM design, and I'd agree with you, but sadly
this is what is found in these chips. So effectively, the only thing
we can guarantee is that the period is within a multiple of sqrt(2)
from the requested period, except even that is not true due to
saturation restrictions that could push the period even further from
what you ask.

In the other mode (where you sacrifice one of the output channels),
you have finer control of the period, but it still affects all
channels. This mode might be a better fit for your proposed API,
except that the driver would still be subject to saturation
restrictions on the period. And those restrictions will change as the
core bus frequency is changed, which means that again, even if your
target period can be achieved (common to all channels) at invocation,
it might not be achievable later.

IMO the only real control this PWM can offer reliably is the duty
cycle, which is why I've written it how I have.

If you see a better solution to the above problems, I am open to suggestions.

>> +Required properties:
>> +- compatible: should be "sifive,pwm0"
>
> Why not simply "sifive,pwm"? If this is supposed to be some sort of
> version number, then it is more customary to use the name of the first
> SoC that integrates the IP. There are some exceptions, like for example
> when the IP is third-party and is integrated in a number of different
> SoCs. In such cases the IP is often properly versioned. But that doesn't
> seem to be the case here.

It is indeed a version number. The first SoC which integrated this IP
cannot run linux. We've put a version number like this into all of our
IP blocks. Isn't an increasing number, which clearly indicates
increased functionality, better than a reference to a sequence of SoCs
whose relationships are not all that clear?

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  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@ 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

[Wesley Terpstra]

Ugh. Clicked reply without being done writing the reply!

On Thu, Jun 8, 2017 at 3:52 AM, Mark Rutland  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@ 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

[Wesley Terpstra]

On Thu, Jun 8, 2017 at 3:52 AM, Mark Rutland  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.

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  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.

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...@kernel.org> wrote:
> On Wed, Jun 7, 2017 at 1:57 PM, Wesley Terpstra <wes...@sifive.com> wrote:
>> On Wed, Jun 7, 2017 at 3:13 AM, Mark Rutland <mark.rutl...@arm.com> wrote:
>>>> > +RISC-V Hart-Level Interrupt Controller (HLIC)
>>>> > +-
>
> [...]
>
>>>> > +   plic: interrupt-controller@c00 {
>>>> > +   #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  wrote:
> On Wed, Jun 7, 2017 at 1:57 PM, Wesley Terpstra  wrote:
>> On Wed, Jun 7, 2017 at 3:13 AM, Mark Rutland  wrote:
>>>> > +RISC-V Hart-Level Interrupt Controller (HLIC)
>>>> > +-
>
> [...]
>
>>>> > +   plic: interrupt-controller@c00 {
>>>> > +   #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 02/17] pcie-xilinx: add missing 5th legacy interrupt

[Wesley Terpstra]

On Wed, Jun 7, 2017 at 2:24 AM, Marc Zyngier  wrote:
> This is a common problem with the current OF code that numbers INTx from
> 1 instead of zero (there is no 5th legacy interrupts in the PCI spec,
> despite what $SUBJECT says). I'd be inclined to fix this at the core
> level rather than papering over it in the various drivers...

While I agree that it's a problem with OF, every other driver has
already been changed to paper over the issue. This patch just brings
this one remaining OF-PCIe driver to the same level as the others.
Without the patch, the driver doesn't work at all if there is a bridge
chip on the other end of the controller, so this is not just a
hypothetical concern for us.

Couldn't the eventual OF fix just refactor this driver along with all
of the others? Doing such a sweeping OF change is outside my current
comfort zone. I am not familiar enough with the code to understand all
the parts that would need to be touched.

Re: [PATCH 02/17] pcie-xilinx: add missing 5th legacy interrupt

[Wesley Terpstra]

On Wed, Jun 7, 2017 at 2:24 AM, Marc Zyngier  wrote:
> This is a common problem with the current OF code that numbers INTx from
> 1 instead of zero (there is no 5th legacy interrupts in the PCI spec,
> despite what $SUBJECT says). I'd be inclined to fix this at the core
> level rather than papering over it in the various drivers...

While I agree that it's a problem with OF, every other driver has
already been changed to paper over the issue. This patch just brings
this one remaining OF-PCIe driver to the same level as the others.
Without the patch, the driver doesn't work at all if there is a bridge
chip on the other end of the controller, so this is not just a
hypothetical concern for us.

Couldn't the eventual OF fix just refactor this driver along with all
of the others? Doing such a sweeping OF change is outside my current
comfort zone. I am not familiar enough with the code to understand all
the parts that would need to be touched.

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  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@c00 {
>> > +   #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

[Wesley Terpstra]

On Wed, Jun 7, 2017 at 3:13 AM, Mark Rutland  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@c00 {
>> > +   #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 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
lrwxrwxrwx1 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  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  wrote:
>> > From: "Wesley W. Terpstra" 
>> >
>> > This fixes: [0.01] 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 
>> > ---
>> >  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 majord...@vger.kernel.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
lrwxrwxrwx1 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  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  wrote:
>> > From: "Wesley W. Terpstra" 
>> >
>> > This fixes: [0.01] 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 
>> > ---
>> >  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 majord...@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html