On 12/4/19 8:07 AM, Andrew Murray wrote:
On Wed, Dec 04, 2019 at 10:29:51AM +0200, Andy Shevchenko wrote:
On Wed, Dec 4, 2019 at 12:09 AM Ray Jui <ray.jui@xxxxxxxxxxxx> wrote:
On 12/3/19 11:27 AM, Andy Shevchenko wrote:
On Tue, Dec 3, 2019 at 5:55 PM Andrew Murray <andrew.murray@xxxxxxx> wrote:
On Tue, Dec 03, 2019 at 10:27:02AM +0530, Srinath Mannam wrote:
+ /* go through INTx A, B, C, D until all interrupts are handled */
+ do {
+ status = iproc_pcie_read_reg(pcie, IPROC_PCIE_INTX_CSR);
By performing this read once and outside of the do/while loop you may improve
performance. I wonder how probable it is to get another INTx whilst handling
one?
May I ask how it can be improved?
One read will be needed any way, and so does this code.
I guess the current code will cause the IPROC_PCIE_INTX_CSR register to
be read TWICE, if it's ever set to start with.
But then if we do it outside of the while loop, if we ever receive an
interrupt while servicing one, the interrupt will still need to be
serviced, and in this case, it will cause additional context switch
overhead by going out and back in the interrupt context.
Yes it's a trade off - if you dropped the do/while loop and thus had a single
read you'd reduce the overhead on interrupt handling in every case except
where another INTx is received whilst in this function. But as you point out
each time that does happen you'll pay the penalty of a context switch.
Exactly, it's a tradeoff between: 1) saving one register read (which is
likely in the 10th of nanosecond range) in all INTx handling; and 2)
saving context switches (which is likely in 10th of microsecond range)
in cases when we have multiple INTx when servicing it.
The current implementation takes 2), which I thought it makes sense.
I don't have any knowledge of this platform so I have no idea if such a change
would be good/bad or material. However I thought I'd point it out. Looking at
the other controller drivers, some handle in a loop and some don't.
My take is that it's probably more ideal to leave this portion of code
as it is.
Can't we simple drop a do-while completely and leave only
for_each_set_bit() loop?
Like both Andrew and I pointed out. There's a tradeoff here. Could you
please help to justify why you favor 1) than 2)?
I'm happy either way.
Thanks,
Andrew Murray
+ for_each_set_bit(bit, &status, PCI_NUM_INTX) {
+ virq = irq_find_mapping(pcie->irq_domain, bit);
+ if (virq)
+ generic_handle_irq(virq);
+ else
+ dev_err(dev, "unexpected INTx%u\n", bit);
+ }
+ } while ((status & SYS_RC_INTX_MASK) != 0);
--
With Best Regards,
Andy Shevchenko
