----- Original Message ----- > From: "Andy Shevchenko" <andriy.shevchenko@xxxxxxxxxxxxxxx> > To: "Aaron Sierra" <asierra@xxxxxxxxxxx> > Sent: Wednesday, July 18, 2018 12:15:06 PM > On Wed, 2018-07-18 at 11:22 -0500, Aaron Sierra wrote: >> ----- Original Message ----- >> > From: "Andy Shevchenko" <andriy.shevchenko@xxxxxxxxxxxxxxx> >> > To: "Aaron Sierra" <asierra@xxxxxxxxxxx> >> > Sent: Wednesday, July 18, 2018 9:38:46 AM >> > On Tue, 2018-07-17 at 11:12 +0300, Andy Shevchenko wrote: >> > >> > Aaron, while addressing your comment I noticed that your patch >> > (c7e1b4059075) actually might bring a regression in some cases. >> >> Andy, >> You may be right about that. I'll take a look. Please see my other >> comments below. > > Thanks for quick response. My answers below. > >> >> > See below what I have as a patch in my WIP tree (https://bitbucket.o >> > rg/a >> > ndy-shev/linux/branch/topic/uart/rpm) >> > >> > I wouldn't go further with 8250_exar until I hear from you about >> > this. >> > Some clarifications would be nice to have. >> > >> > --- 8< --- 8< --- >> > >> > From 19e0b71fe8bd7b2562c05c993f0e60315f2a3205 Mon Sep 17 00:00:00 >> > 2001 >> > From: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> >> > Date: Tue, 17 Jul 2018 19:31:06 +0300 >> > Subject: [PATCH] serial: 8250_exar: Revert "Relocate sleep wake-up >> > handling" >> > >> > While benefits listed in the commit c7e1b4059075 sounds plausible, >> > it >> > seems it breaks expansion board support. >> >> Do you have any evidence that this is causing breakage in the wild or >> is >> this speculation for the time being? > > No evidence, just some time spent on data sheet and trying to understand > how it might work all together. Andy, Ok, that's good to know. > >> > Unfortunately the data sheet doesn't show what happens to interrupts >> > for >> > the slave UARTs. However, the description of INT0-INT3 register is >> > clearly points to the fact that the register has covered only 8 >> > channels, and it's unclear how the rest would behave. >> >> If there is a problem, I'm sure that there is a simple solution that >> wouldn't necessitate a full revert. Making sure to hit INT0 in the >> slave device's address space would likely be sufficient. > > 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. > - 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. >> > 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..." > >> Exar's >> documentation for the XR17V358 describes them in 8-bit and 32-bit >> contexts >> in close proximity. >> >> They are tabulated as 8-bit registers: >> >> 0x80 - INT0 >> 0x81 - INT1 >> 0x82 - INT2 >> 0x83 - INT3 >> >> And they are tabulated as a 32-bit register: >> >> 0x0080-0x0083 - INTERRUPT >> > >> I believe my use of ioread8 is not wrong and is perfectly safe. > > It works because of design of the register in hardware... OK, and this driver is written to work with registers implemented by Exar for their hardware, so what's the issue? >> 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. > - (new) since it's wake up interrupt, perhaps we can use it as a wake up > source in the Linux? > - misc nitpicks (like unneeded changes that had been done) I think my pcim_iomap() changes are well-reasoned... 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? -Aaron S. > > -- > 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
