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