Re: [PATCH v2 3/3] sam460ex: Clean up irq mapping

[BALATON Zoltan]

On Thu, 31 Dec 2020, BALATON Zoltan via wrote:
> On Thu, 31 Dec 2020, Peter Maydell wrote:
> > On Thu, 31 Dec 2020 at 15:11, Peter Maydell <peter.maydell@linaro.org> 
> > wrote:
> > > On Thu, 31 Dec 2020 at 11:20, BALATON Zoltan <balaton@eik.bme.hu> wrote:
> > > > Avoid mapping multiple interrupts to the same irq. Instead map them to
> > > > the 4 PCI interrupts and use an or-gate in the board to connect them
> > > > to the interrupt controller. This does not fix any known problem but
> > > > does not seem to cause a new problem either and may be cleaner at least.
> > > >
> > > > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> > > > Tested-by: Guenter Roeck <linux@roeck-us.net>
> > >
> > > So, this patch is a behavioural change, but I think it's
> > > probably a change to the right behaviour. The difference
> > > is relatively slight, but you would see it if there are two
> > > different PCI cards and they both assert a different PCI
> > > interrupt, and then one of them lowers the interrupt
> > > before the other:
> >
> > This turns out to be wrong -- I hadn't looked at the QEMU PCI
> > code, but it has an internal implementation of logic that
> > gives the same behaviour as an explicit OR gate. Basically
> > pci_change_irq_level() tracks how many assert/deasserts of
> > the (mapped) IRQ lines have happened, so it only calls the
> > controller's set_irq function when the count of asserted
> > inputs goes down to 0. So both the current code and this
> > patch's change are functionally correct.
>
> I've remembered we had this discussion before and arrived to the same 
> conclusion that current code was equivalently working but could not recall 
> the reason.
>
> > I'm not sure which would be nominally closer to the "real hardware":
> > the 440ex CPU/SoC datasheet lists a single PCI0INT signal, but
> > it says it is an output, not an input, so I'm pretty sure there's
> > something I don't understand about PCI here. (Also, unlike the
> > 440EP it provides PCI Express as well as PCI.)
>
> The SoC is called 460EX (despite having a PPC 440 core not 460 one) but I 
> think you've looked at the right data sheet and it's just a typo. I also 
> don't know how the board is wired so I think in this case I prefer dropping 
> this patch and keeping the current code just for simplicity but to avoid 
> going through this again maybe we should add a comment saying why it's 
> working. Can you please suggest a test for such comment pointing to the

I mean "text" not "test" above.

> relevant part of pci_change_irq_level() you refer to above? I don't think I 
> understand it enough to document it.
>
> Thank you,
> BALATON Zoltan