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. > > 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() - 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? > > 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... > 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." So, summarize what we have: - potential issue on expansion board - ioreadXX() inconsistency with the rest of the code - (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) -- 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
