13:32 <MoeIcenowy> smaeul: do your `dt-bindings: interrupt-controller: Require trigger type for T-HEAD PLIC` patch break DT compatibility?

13:36 <MoeIcenowy> the DT binding w/ 1 cell for c900-plic is available in 5.19 stable version

13:48 <MoeIcenowy> BTW should we just change the binding to suggest 2 cells for all PLICs?

13:48 <MoeIcenowy> It's only SiFive's PLIC assumes level for all interrupts

15:17 <smaeul> MoeIcenowy: technically yes, but nothing should have been using it yet. if you want, it would not be too hard to accept either number of cells for all variants

15:21 <MoeIcenowy> smaeul: well in fact I am doing my custom SoC with C906 on a FPGA

15:21 <MoeIcenowy> and discovered this ;-)

15:26 <smaeul> in that case you should fix the bugs in the PLIC and use a different compatible anyway ;-)

15:27 <smaeul> at least in openc906, neither the edge nor level trigger behavior is quite correct

16:37 <MoeIcenowy> smaeul: how are they incorrect?

16:38 <MoeIcenowy> smaeul: BTW what timer is you WIP D1 kernel using now?

16:38 <MoeIcenowy> riscv-timer or sun4i-timer?

16:38 <MoeIcenowy> if the former, how is it initialized?

17:04 <smaeul> MoeIcenowy: edge mode has no queue or double-pending state, so if a second edge arrives before the first EOI, it is lost entirely

17:05 <smaeul> level mode is implemented as edge mode with retriggering during EOI (if level is high during EOI, set pending again), so it doesn't have true level semantics of removing the pending state if the level goes low before the interrupt is handled

17:06 <smaeul> (if the interrupt goes away, due to external events or polling, the interrupt handler should never run)

17:08 <smaeul> MoeIcenowy: Linux currently chooses sun4i timer for both clocksource and clockevent. ideally it would choose riscv timer for clocksource

17:08 <apritzel> smaeul: so this is T-HEADs implementation not conforming to the PLIC spec? Or a flaw in the spec?

17:10 <smaeul> apritzel: the spec leaves the trigger type unspecified, with the intention that software should not have to know/care

17:11 <apritzel> so the PLIC input line handling has to match what the device implements, and handle this in hardware?

17:11 <smaeul> that's the intention, yes

17:12 <smaeul> so the C906 PLIC is technically conformant here, but in such a way that software needs to know the trigger type anyway to avoid missing interrupts

17:12 <smaeul> because the spec itself is bad and suggests that queueing edges is optional

17:13 <apritzel> but missing edge triggered IRQs is somewhat unavoidable, or at least coalescing is expected?

17:14 <smaeul> coalescing is fine, and is what you get if you reset the trigger at ack (at the beginning of the ISR), but you miss interrupts if you reset the trigger at eoi (at the end of the ISR)

17:16 <smaeul> https://lore.kernel.org/linux-kernel/9c0e2b5f-c8ce-aba9-8b2b-6d79f9624a7f@sholland.org/

17:17 <smaeul> > So an interrupt will never go pending while it is active.

17:17 <smaeul> that's the important part

17:18 <smaeul> s/ack/claim/ to use the PLIC terminology

17:19 <apritzel> ah, was just wondering how this matches the diagram on Github ;-)

17:20 <apritzel> so this "reset the trigger" is happening automatically at EOI? Or can a driver do this explicitly?

17:22 <smaeul> yes, the reset happens at EOI by writing the claim/completion register. so the driver can work around it by doing the EOI write before actually running the ISR

17:24 <smaeul> but that means the software flow differs based on the trigger type -- so you need the trigger type in the DT -- and that breaks the intention of software not having to know/care about the trigger type

17:26 <apritzel> isn't the trigger type something that the device dictates anyway? So a driver could know this without looking at firmware tables?

17:28 <smaeul> assuming the trigger type isn't configurable in the device's HDL, sure

17:29 <MoeIcenowy> smaeul: if the spec is written in this way, maybe we should make all device interrupt-cells = <2>

17:30 <MoeIcenowy> and keep sifive level-only things only as compatibility fallbacks

17:31 <smaeul> apritzel: but I'll bet there are some compatibles out there for devices with multiple variants, where the trigger type isn't discoverable via reading a register

17:31 <smaeul> so in a sense, the DT cell is there for the benefit of the client device driver

17:31 <smaeul> at least for the D1, I'm pretty sure everything is level anyway, so this is all just for "correctness"

17:34 <smaeul> MoeIcenowy: I think this is already handled by the base compatible, e.g. thead,c900-plic is not compatible with sifive,plic-1.0.0

17:35 <smaeul> so we could accomplish this by not using sifive,plic-1.0.0 for new devices

17:39 <apritzel> mmh, I would assume that the trigger type is more driven by the IRQ (and device) *semantic*, so for instance a UART FIFO full IRQ is inherently level