On Wed, 2018-07-18 at 16:36 -0500, Aaron Sierra wrote: > > Yes, that's why I didn't submit change to mailing list. > > > > But I also have more comments to that patch: > > - pcim_iomap() is idempotent, thus it was no need to remove it > > inside > > default_setup() > > I moved the pcim_iomap() call to exar_pci_probe(). I removed it from > default_setup(), not because it wasn't idempotent, but because it's > superfluous based step #1. > > I moved pcim_iomap() in order to setup priv->virt, which makes the > interrupt handler cleaner. OK. > > - ioreadXX() is inconsistent with the rest readX() I/O accessors > > - perhaps we might put a debug print of the returned value of INTx > > > > > Reverting to the previous behavior of hitting INT0 12 or 16 times > > > *per interrupt* (depending on port count) seems very overkill to > > > me. > > > > I actually forgot to ask how did you derive this number? > > This number comes from the original implementation registering an IRQ > handler for every port. That handler reads INT0 from the port's > address space whenever the interrupt fires just-in-case it's a wake up > interrupt. > > A 4-port device had 4 registered ports and 4 interrupt handlers that > each do one read from INT0. A 12-port device had 12 ports/handlers, > etc. Ah, okay. Yes, it makes sense. > > > > > By the way, pay attention that INT0-INT3 is actually 32-bit > > > > register > > > > and > > > > ioread8() used in the mentioned patch is wrong, though might > > > > work, > > > > by two reasons: > > > > - register is 32-bit and we need a corresponding I/O accessor, > > > > - readl() would be consistent with the rest of the code. > > > > > > Why do you say that INT0-INT3 is 32-bit (as if exclusively)? > > > > From the data sheet where it's explicitly mentioned: > > > > "The XR17V358 has a 32-bit wide register [INT0, INT1, INT2 and > > INT3]... > > All 8 channel interrupts status are available with a single DWORD > > read > > operation..." > > > However, > > > if you *prefer* to use a 32-bit accessor, then that will impose no > > > adverse performance impact. > > > > ...Just found: > > " Wake-up indicator is cleared by a read to the INT0 register." > > Yes, exactly. > > > So, summarize what we have: > > - potential issue on expansion board > > Agreed, this may be an issue. > > > - ioreadXX() inconsistency with the rest of the code > > I fail to see the inconsistency when all of the UART port register > accesses already *safely* use 8-bit accessors as indicated by > UPIO_MEM. We don't need to use ioread8(). We always have MMIO and moreover, the rest of the code uses MMIO accessors. So, the idea of ARs is: - replace ioread8() with readb() - add one more readb() for the expansion board - (?) perhaps amend a comment to clarify 32-bit vs. 8-bit read for INT0 in case of wake IRQs > Are you willing to accept any of my criticisms of your criticisms, so > that we can move forward with addressing the real problem that my > patch may have introduced? Yes. -- Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> Intel Finland Oy -- To unsubscribe from this list: send the line "unsubscribe linux-serial" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
