Hi Marc, Thank you again. On Tue, 21 Jul 2020 at 12:10, Marc Zyngier <maz@xxxxxxxxxx> wrote: > > On 2020-07-21 10:27, Grzegorz Jaszczyk wrote: > > Hi Marc, > > > > First of all thank you very much for your review. I apologize in > > advance if the description below is too verbose or not detailed > > enough. > > > > On Fri, 17 Jul 2020 at 14:36, Marc Zyngier <maz@xxxxxxxxxx> wrote: > >> > >> Suman, Grzegorz, > >> > >> On Wed, 15 Jul 2020 14:38:05 +0100, > >> Grzegorz Jaszczyk <grzegorz.jaszczyk@xxxxxxxxxx> wrote: > >> > > >> > Hi Marc, > >> > > >> > > On 7/8/20 5:47 AM, Marc Zyngier wrote: > >> > > > On 2020-07-08 08:04, Grzegorz Jaszczyk wrote: > >> > > >> On Sun, 5 Jul 2020 at 22:45, Marc Zyngier <maz@xxxxxxxxxx> wrote: > >> > > >>> > >> > > >>> On 2020-07-05 14:26, Grzegorz Jaszczyk wrote: > >> > > >>> > On Sat, 4 Jul 2020 at 11:39, Marc Zyngier <maz@xxxxxxxxxx> wrote: > >> > > >>> >> > >> > > >>> >> On 2020-07-03 15:28, Grzegorz Jaszczyk wrote: > >> > > >>> > >> > > >>> [...] > >> > > >>> > >> > > >>> >> It still begs the question: if the HW can support both edge and level > >> > > >>> >> triggered interrupts, why isn't the driver supporting this diversity? > >> > > >>> >> I appreciate that your HW may only have level interrupts so far, but > >> > > >>> >> what guarantees that this will forever be true? It would imply a > >> > > >>> >> change > >> > > >>> >> in the DT binding, which isn't desirable. > >> > > >>> > > >> > > >>> > Ok, I've got your point. I will try to come up with something later > >> > > >>> > on. Probably extending interrupt-cells by one and passing interrupt > >> > > >>> > type will be enough for now. Extending this driver to actually support > >> > > >>> > it can be handled later if needed. Hope it works for you. > >> > > >>> > >> > > >>> Writing a set_type callback to deal with this should be pretty easy. > >> > > >>> Don't delay doing the right thing. > >> > > >> > >> > > >> Ok. > >> > > > >> > > Sorry for the typo in my comment causing this confusion. > >> > > > >> > > The h/w actually doesn't support the edge-interrupts. Likewise, the > >> > > polarity is always high. The individual register bit descriptions > >> > > mention what the bit values 0 and 1 mean, but there is additional > >> > > description in the TRMs on all the SoCs that says > >> > > "always write 1 to the bits of this register" for PRUSS_INTC_SIPR(x) and > >> > > "always write 0 to the bits of this register" for PRUSS_INTC_SITR(x). > >> > > FWIW, these are also the reset values. > >> > > > >> > > Eg: AM335x TRM - https://www.ti.com/lit/pdf/spruh73 > >> > > Please see Section 4.4.2.5 and the register descriptions in 4.5.3.49, > >> > > 4.5.3.51. Please also see Section 4.4.2.3 that explains the PRUSS INTC > >> > > methodology. > >> > > > >> > > >> > >> > > >>> > >> > > >>> [...] > >> > > >>> > >> > > >>> >> >> > + hwirq = hipir & GENMASK(9, 0); > >> > > >>> >> >> > + virq = irq_linear_revmap(intc->domain, hwirq); > >> > > >>> >> >> > >> > > >>> >> >> And this is where I worry. You seems to have a single irqdomain > >> > > >>> >> >> for all the muxes. Are you guaranteed that you will have no > >> > > >>> >> >> overlap between muxes? And please use irq_find_mapping(), as > >> > > >>> >> >> I have top-secret plans to kill irq_linear_revmap(). > >> > > >>> >> > > >> > > >>> >> > Regarding irq_find_mapping - sure. > >> > > >>> >> > > >> > > >>> >> > Regarding irqdomains: > >> > > >>> >> > It is a single irqdomain since the hwirq (system event) can be > >> > > >>> mapped > >> > > >>> >> > to different irq_host (muxes). Patch #6 > >> > > >>> >> > https://lkml.org/lkml/2020/7/2/616 implements and describes how > >> > > >>> input > >> > > >>> >> > events can be mapped to some output host interrupts through 2 > >> > > >>> levels > >> > > >>> >> > of many-to-one mapping i.e. events to channel mapping and > >> > > >>> channels to > >> > > >>> >> > host interrupts. Mentioned implementation ensures that specific > >> > > >>> system > >> > > >>> >> > event (hwirq) can be mapped through PRUSS specific channel into a > >> > > >>> >> > single host interrupt. > >> > > >>> >> > >> > > >>> >> Patch #6 is a nightmare of its own, and I haven't fully groked it > >> > > >>> yet. > >> > > >>> >> Also, this driver seems to totally ignore the 2-level routing. Where > >> > > >>> >> is it set up? map/unmap in this driver do exactly *nothing*, so > >> > > >>> >> something somewhere must set it up. > >> > > >>> > > >> > > >>> > The map/unmap is updated in patch #6 and it deals with those 2-level > >> > > >>> > routing setup. Map is responsible for programming the Channel Map > >> > > >>> > Registers (CMRx) and Host-Interrupt Map Registers (HMRx) basing on > >> > > >>> > provided configuration from the one parsed in the xlate function. > >> > > >>> > Unmap undo whatever was done on the map. More details can be found in > >> > > >>> > patch #6. > >> > > >>> > > >> > > >>> > Maybe it would be better to squash patch #6 with this one so it would > >> > > >>> > be less confusing. What is your advice? > >> > > >>> > >> > > >>> So am I right in understanding that without patch #6, this driver does > >> > > >>> exactly nothing? If so, it has been a waste of review time. > >> > > >>> > >> > > >>> Please split patch #6 so that this driver does something useful > >> > > >>> for Linux, without any of the PRU interrupt routing stuff. I want > >> > > >>> to see a Linux-only driver that works and doesn't rely on any other > >> > > >>> exotic feature. > >> > > >>> > >> > > >> > >> > > >> Patch #6 provides PRU specific 2-level routing setup. This step is > >> > > >> required and it is part of the entire patch-set. Theoretically routing > >> > > >> setup could be done by other platform driver (not irq one) or e.g. by > >> > > >> PRU firmware. In such case this driver would be functional without > >> > > >> patch #6 but I do not think it would be proper. > >> > > > > >> > > > Then this whole driver is non-functional until the last patch that > >> > > > comes with the PRU-specific "value-add". > >> > > > >> > > It is all moot actually and the interrupts work only when the PRU > >> > > remoteproc/clients have invoked the irq_create_fwspec_mapping() > >> > > for all of the desired system events. It does not make much difference > >> > > if it was a separate patch or squashed in, patch #6 is a replacement for > >> > > the previous logic, and since it was complex, it was done in a separate > >> > > patch to better explain the usage (same reason on v1 and v2 as > >> > > well). > >> > >> It may make no difference to you, but it does for me, as I'm the lucky > >> idiot reviewing this code. So I am going to say it again: please keep > >> anything that only exists for the PRU subsystem benefit out of the > >> initial patches. > >> > >> I want to see something that works for Linux, and only for Linux. Once > >> we have that working, we'll see to add more stuff. But stop throwing > >> the PRU business into the early patches, as all you are achieving is > >> to delay the whole thing. > >> > >> > > > >> > > > > >> > > > [...] > >> > > > > >> > > >> I am open to any suggestion if there is a better way of handling > >> > > >> 2-level routing. I will also appreciate if you could elaborate about > >> > > >> issues that you see with patch #6. > >> > > > > >> > > > The two level routing has to be part of this (or another) irqchip > >> > > > driver (specially given that it appears to me like another set of > >> > > > crossbar). There should only be a *single* binding for all interrupts, > >> > > > including those targeting the PRU (you seem to have two). > >> > > > > >> > > > >> > > Yeah, there hasn't been a clean way of doing this. Our previous attempt > >> > > was to do this through custom exported functions so that the PRU > >> > > remoteproc driver can set these up correctly, but that was shot down and > >> > > this is the direction we are pointed to. > >> > > > >> > > We do want to leverage the "interrupts" property in the PRU user nodes > >> > > instead of inventing our own paradigm through a non-irqchip driver, and > >> > > at the same time, be able to configure this at the run time only when > >> > > that PRU driver is running, and remove the mappings once that driver is > >> > > removed allowing another PRU application/driver. We treat PRUs as an > >> > > exclusive resource, so everything needs to go along with an appropriate > >> > > client user. > >> > > >> > I will just add an explanation about interrupt binding. So actually > >> > there is one dt-binding defined in yaml (interrupt-cells = 1). The > >> > reason why you see xlate allowing to proceed with 1 or 3 parameters is > >> > because linux can change the PRU firmware at run-time (thorough linux > >> > remoteproc framework) and different firmware may require different > >> > kinds of interrupt mapping. Therefore during firmware load, the new > >> > mapping is created through irq_create_fwspec_mapping() and in this > >> > case 3 parameters are passed: system event, channel and host irq. > >> > Similarly the mapping is disposed during remoteproc stop by invoking > >> > irq_dispose_mapping. This allows to create new mapping, in the same > >> > way, for next firmware loaded through Linux remote-proc at runtime > >> > (depending on the needs of new remoteproc firmware). > >> > > >> > On the other hand dt-bindings defines interrupt-cells = 1, so when the > >> > interrupt is registered the xlate function (proceed with 1 parameter) > >> > checks if this event already has valid mapping - if yes we are fine, > >> > if not we return -EINVAL. > >> > >> It means that interrupts declared in DT get their two-level routing > >> via the kernel driver, while PRU interrupts get their routing via some > >> external blob that Linux is not in control of? > > > > Actually with the current approach all two-level routing goes through > > this linux driver. The interrupts that should be routed to PRU are > > described in remoteproc firmware resource table [1] and it is under > > Linux remoteproc driver control. In general, the resource table > > contains system resources that the remote processor requires before it > > should be powered on. We treat the interrupt mapping (described in the > > resource table, which is a dedicated elf section defined in [1]) as > > one of system resources that linux has to provide before we power on > > the PRU core. Therefore the remoteproce driver will parse the resource > > table and trigger irq_create_fwspec_mapping() after validating > > resource table content. > > Validating the resource table says nothing of a potential conflict > with previous configured interrupts. Yes, that's why we introduced the logic in pruss_intc_irq_domain_xlate and pruss_intc_map triggered by irq_create_fwspec_mapping, which will check potential conflicts with previous configured interrupts. I understand that you do not like how it is done but I do not know how to do it in a different way so it will cover all caveats, please see below. > > > > > [1] https://www.kernel.org/doc/Documentation/remoteproc.txt (Binary > > Firmware Structure) > > > >> > >> If so, this looks broken. What if you get a resource allocation > >> conflict because the kernel and the blob are stepping into each > >> other's toes? Why should an end-point client decide on the routing of > >> the interrupt? > > > > The code in the pruss_intc_map function checks if there are no > > allocation conflicts: e.g. if the sysevent is already assigned it will > > throw -EBUSY. Similarly when some channel was already assigned to > > host_irq and a different assignment is requested it will again throw > > -EBUSY. > > But why should it? The allocation should take place based on constraints > (source, target, and as you mentioned below, priority). Instead, you > seem to be relying on static allocation coming from binary blobs, > standardized or not. > > I claim that this static allocation is madness and should be eliminated. > Instead, the Linux driver should perform the routing based on allocation > requirements (the above constraints), and only fail if it cannot satisfy > these constraints. I am not sure if I understood. The allocation requirements are as you've described: source (system event), target (host interrupt) and priority (channel). E.g.: - routing system event 3 with priority (chanell) 2 to PRU core 0 will be described as bellow: (3, 2, 0) (0 corresponds to PRU0) - routing system event 10 with priority (chanell) 3 to PRU core 1: (10, 3, 1) - routing system event 15 with priority (5) to MCPU interrupt 0*: (15, 5, 2) * interrupts 2 through 9 (host_intr0 through host_intr7) I am not sure but you probably refer to changing it to loosely dynamic allocation but this will not work for any of them since: - different system event is just a different sources (e.g. some of them are tightly coupled with PRUSS industrial ethernet peripheral, other with PRUSS UART, other are general purpose ones and so on). - lower number channels have higher priority (10 different channels, each with different priority). - host interrupt 0 is for PRU core 0; host interrupt 1 is for PRU core 1; host interrupts 2 through 9 are for main CPU. So the logic in patch #6 prevents mapping system events if it was already assigned to a different channel or target (host interrupt) and only fails if it cannot be satisfied. Moreover I do not see a way to relax the static description since picking different numbers for each individual: system event, channel and host interrupt will result with something unintentional and wrong. Sorry if I misunderstood you, if so could you please elaborate? Thank you, Grzegorz
