* Re: [PATCH] serial: tegra: Map the iir register to default defines
From: Olliver Schinagl @ 2017-03-30 15:37 UTC (permalink / raw)
To: Jon Hunter, Laxman Dewangan, Greg Kroah-Hartman, Jiri Slaby,
Stephen Warren, Thierry Reding, Alexandre Courbot
Cc: linux-serial, linux-tegra, linux-kernel
In-Reply-To: <9eed3c8e-a7af-1e1f-659a-a092a89d1679@nvidia.com>
Hey Jon,
On March 30, 2017 3:42:19 PM CEST, Jon Hunter <jonathanh@nvidia.com> wrote:
>
>On 29/03/17 19:48, Olliver Schinagl wrote:
>> The tegra serial IP seems to be following the common layout and the
>> interrupt ID's match up nicely. Replace the magic values to match the
>> common serial_reg defines, with the addition of the Tegra unique End
>of
>> Data interrupt.
>>
>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
>> ---
>> Note I do not own any tegra hardware and just noticed it while
>working on my
>> somewhat related previous patch,
>> "serial: Do not treat the IIR register as a bitfield"
>>
>> As such, this patch can only be applied after the aforementioned
>patch or the
>> iir variable will not have its mask applied yet.
>
>Nit-pick. If this is the case, then this should really be part of a
>patch series so it is obvious to everyone that this should only be
>applied after the other patch.
Yes, and it was, but I did not want to have the really big list of names in this much smaller group.
>
>Cheers
>Jon
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
^ permalink raw reply
* Re: [PATCH] serial: Do not treat the IIR register as a bitfield
From: Theodore Ts'o @ 2017-03-30 14:11 UTC (permalink / raw)
To: Olliver Schinagl
Cc: Vignesh R, Greg Kroah-Hartman, Jiri Slaby, Laxman Dewangan,
Stephen Warren, Thierry Reding, Alexandre Courbot,
David S . Miller, dev@linux-sunxi.org, Ed Blake, Andy Shevchenko,
Alexander Sverdlin, Yegor Yefremov, Wan Ahmad Zainie, Kefeng Wang,
Heikki Krogerus, Heiko Stuebner, Jason Uy
In-Reply-To: <D694FA74-1F31-4B57-8858-2DBE8DE57D45@schinagl.nl>
While you're fixing this, there's a bug in samples/vfio-mdev/mtty.c:
u8 ier = mdev_state->s[index].uart_reg[UART_IER];
*buf = 0;
mutex_lock(&mdev_state->rxtx_lock);
/* Interrupt priority 1: Parity, overrun, framing or break */
if ((ier & UART_IER_RLSI) && mdev_state->s[index].overrun)
*buf |= UART_IIR_RLSI;
/* Interrupt priority 2: Fifo trigger level reached */
if ((ier & UART_IER_RDI) &&
(mdev_state->s[index].rxtx.count ==
mdev_state->s[index].intr_trigger_level))
*buf |= UART_IIR_RDI;
/* Interrupt priotiry 3: transmitter holding register empty */
if ((ier & UART_IER_THRI) &&
(mdev_state->s[index].rxtx.head ==
mdev_state->s[index].rxtx.tail))
*buf |= UART_IIR_THRI;
/* Interrupt priotiry 4: Modem status: CTS, DSR, RI or DCD */
if ((ier & UART_IER_MSI) &&
(mdev_state->s[index].uart_reg[UART_MCR] &
(UART_MCR_RTS | UART_MCR_DTR)))
*buf |= UART_IIR_MSI;
/* bit0: 0=> interrupt pending, 1=> no interrupt is pending */
if (*buf == 0)
*buf = UART_IIR_NO_INT;
It's treating the UART_IIR_* fields as a bitmask which is bad enough,
but in the "Interrupt priority 4" case, UART_IIR_MSI is zero, so
"*buf |= UART_IIR_MSI" is a no-op. And in the case where the modem
status interrupt is the only thing set, *buf will be 0, and UART_IIR_NO_INT
gets set erroneously.
So this is another example of the bug of trying to treat the
UART_IIR_* fields as a bitmask....
Yes, it's only sample code, but best fix it now before it gets copied
elsewhere and metastisizes. :-)
- Ted
^ permalink raw reply
* Re: [PATCH] serial: tegra: Map the iir register to default defines
From: Jon Hunter @ 2017-03-30 13:42 UTC (permalink / raw)
To: Olliver Schinagl, Laxman Dewangan, Greg Kroah-Hartman, Jiri Slaby,
Stephen Warren, Thierry Reding, Alexandre Courbot
Cc: linux-serial, linux-tegra, linux-kernel
In-Reply-To: <20170329184806.6577-1-oliver@schinagl.nl>
On 29/03/17 19:48, Olliver Schinagl wrote:
> The tegra serial IP seems to be following the common layout and the
> interrupt ID's match up nicely. Replace the magic values to match the
> common serial_reg defines, with the addition of the Tegra unique End of
> Data interrupt.
>
> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
> ---
> Note I do not own any tegra hardware and just noticed it while working on my
> somewhat related previous patch,
> "serial: Do not treat the IIR register as a bitfield"
>
> As such, this patch can only be applied after the aforementioned patch or the
> iir variable will not have its mask applied yet.
Nit-pick. If this is the case, then this should really be part of a
patch series so it is obvious to everyone that this should only be
applied after the other patch.
Cheers
Jon
--
nvpublic
^ permalink raw reply
* Re: [PATCH v2] serdev: Replace serdev_device_write_buf with serdev_device_write
From: Andrey Smirnov @ 2017-03-30 12:41 UTC (permalink / raw)
To: Rob Herring
Cc: Andy Shevchenko, Chris Healy, Guenter Roeck,
linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <CAL_Jsq+e6jVko66vj+gM1GOUi_gaiwvj=DJRDMJFTJoo64uS8g@mail.gmail.com>
On Wed, Mar 29, 2017 at 2:25 PM, Rob Herring <robh@kernel.org> wrote:
> On Wed, Mar 29, 2017 at 9:16 AM, Andrey Smirnov
> <andrew.smirnov@gmail.com> wrote:
>> On Tue, Mar 28, 2017 at 10:07 AM, Andy Shevchenko
>> <andy.shevchenko@gmail.com> wrote:
>>> On Tue, Mar 28, 2017 at 7:01 PM, Andrey Smirnov
>>> <andrew.smirnov@gmail.com> wrote:
>>>> Convert serdev_device_write_buf's code to be able to transfer amount of
>>>> data potentially exceeding "write room" at the moment of invocation.
>>>>
>>>> To support that, also add serdev_device_write_wakeup.
>>>>
>>>> Drivers wanting to use full extent of serdev_device_write
>>>> functionality are expected to provide serdev_device_write_wakeup as a
>>>> sole handler of .write_wakeup event or call it as a part of driver's
>>>> custom .write_wakeup code.
>>>>
>>>> Drivers wanting to retain old serdev_device_write_buf behaviour can
>>>
>>>> replace those call to calls to serdev_device_write with timeout of
>>>> 0. Providing .write_wakeup handler in such case is optional.
>>>
>>> Some indentation would be better if, for example, 0 will be kept on
>>> previous line.
>>>
>>
>> OK, sure.
>>
>>> So, what I would see if no one objects is patch series of two:
>>> 1) introduction of new API
>>> 2) removing old one.
>>>
>>> It will benefit for easier review and any potential code anthropologist.
>>>
>>
>> Second version of the patch preserves the old API an just
>> re-implements it in terms of a new one. I am not sure I see the
>> benefit in splitting it into two patches, but I'll leave it up to Rob
>> to decide.
>
> I think it is fine as-is, but maybe the subject now is a bit misleading.
>
OK, I'll modify the subject to be more representative of the change.
>>>> --- a/drivers/tty/serdev/core.c
>>>> +++ b/drivers/tty/serdev/core.c
>>>> @@ -116,17 +116,41 @@ void serdev_device_close(struct serdev_device *serdev)
>>>> }
>>>> EXPORT_SYMBOL_GPL(serdev_device_close);
>>>>
>>>> -int serdev_device_write_buf(struct serdev_device *serdev,
>>>> - const unsigned char *buf, size_t count)
>>>> +void serdev_device_write_wakeup(struct serdev_device *serdev)
>>>> +{
>>>> + complete(&serdev->write_comp);
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(serdev_device_write_wakeup);
>>>> +
>>>> +int serdev_device_write(struct serdev_device *serdev,
>>>> + const unsigned char *buf, size_t count,
>>>> + unsigned long timeout)
>>>> {
>>>> struct serdev_controller *ctrl = serdev->ctrl;
>>>> + int ret;
>>>>
>>>> - if (!ctrl || !ctrl->ops->write_buf)
>>>> + if (!ctrl || !ctrl->ops->write_buf ||
>>>> + (timeout && !serdev->ops->write_wakeup))
>>>> return -EINVAL;
>>>>
>>>> - return ctrl->ops->write_buf(ctrl, buf, count);
>>>> + mutex_lock(&serdev->write_lock);
>>>> + do {
>>>> + reinit_completion(&serdev->write_comp);
>>>> +
>>>> + ret = ctrl->ops->write_buf(ctrl, buf, count);
>>>> + if (ret < 0)
>>>> + break;
>>>> +
>>>
>>>> + buf += ret;
>>>
>>> Extra white spaces.
>>
>> Which is there on purpose to re-align "+=" with "-=" on the next line.
>> I'll remove it.
>>
>>>
>>>> + count -= ret;
>>>> +
>>>
>>>> + } while (count &&
>>>> + (timeout = wait_for_completion_timeout(&serdev->write_comp,
>>>> + timeout)));
>>>
>>> So, would it be better to support interrupts here and return a
>>> corresponding error code to the user?
>>>
>>
>> I don't have a use-case for that and as far as I can tell, neither SPI
>> nor I2C slave device API offer such functionality universally, so I am
>> inclined to say no. Since the change from wait_for_completion to
>> wait_for_completion_timeout was made per Rob's request, I'd leave it
>> up to him to decided about this change as well.
>
> Honestly, I don't know. It's added easily enough if needed later.
OK, I'll keep things as is for now.
Thanks,
Andrey Smirnov
^ permalink raw reply
* Re: [PATCH v2] serdev: Replace serdev_device_write_buf with serdev_device_write
From: Andrey Smirnov @ 2017-03-30 12:40 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Rob Herring, Chris Healy, Guenter Roeck,
linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <CAHp75Vc4HPSnw6XtUO3Sq3hMQFtaGvK2TmvnWX2q5B=ixBjiig@mail.gmail.com>
On Wed, Mar 29, 2017 at 7:43 AM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Wed, Mar 29, 2017 at 5:16 PM, Andrey Smirnov
> <andrew.smirnov@gmail.com> wrote:
>> On Tue, Mar 28, 2017 at 10:07 AM, Andy Shevchenko
>> <andy.shevchenko@gmail.com> wrote:
>>> On Tue, Mar 28, 2017 at 7:01 PM, Andrey Smirnov
>>> <andrew.smirnov@gmail.com> wrote:
>
>>> So, what I would see if no one objects is patch series of two:
>>> 1) introduction of new API
>>> 2) removing old one.
>>>
>>> It will benefit for easier review and any potential code anthropologist.
>>>
>>
>> Second version of the patch preserves the old API an just
>> re-implements it in terms of a new one. I am not sure I see the
>> benefit in splitting it into two patches, but I'll leave it up to Rob
>> to decide.
>
> Sure. At least I posted benefits I see from splitting.
>
> + bisectability (in case we have to revert your new API by some reason
> it will be easier, hope will be not the case, though...)
>
>>>> + } while (count &&
>>>> + (timeout = wait_for_completion_timeout(&serdev->write_comp,
>>>> + timeout)));
>>>
>>> So, would it be better to support interrupts here and return a
>>> corresponding error code to the user?
>>>
>>
>> I don't have a use-case for that and as far as I can tell, neither SPI
>> nor I2C slave device API offer such functionality universally, so I am
>> inclined to say no. Since the change from wait_for_completion to
>> wait_for_completion_timeout was made per Rob's request, I'd leave it
>> up to him to decided about this change as well.
>
> OK.
>
>>> Besides that question, readability might be better if you use
>>> temporary variable and pack above on one line:
>>>
>>> unsigned long to = timeout;
>>>
>>> } while (count && (to = ...(to)));
>>>
>>
>> Even if you shorten 'timeout' to 'to', formatted as a single line,
>> it'd still exceed line length limitations.
>
> How many? If we are talking about 2-3 characters, that's okay to leave
> them on one line.
Even so, at this point we are definitely in "personal preference"
territory and I'd rather avoid line wrapping.
>
>>>> + * @write_lock Mutext used to esure exclusive access to the bus when
>>>> + * writing data with serdev_device_write()
>>>
>>> checkpatch.pl has integrated spellchecker AFAIU.
>>
>> My bad, forgot to enable it as a git hook, will fix.
>>
>>> Moreover, can you try harder to make that description shorter?
>>>
>>
>> I am all ears for suggestions alternative phrasing, otherwise, no,
>> that's about as hard as I try.
>
> First of all, "used to" is (closer) equivalent to was.
> Second, Mutex is one letter longer than Lock (here is important that
> is just a kind of lock).
> Third, "exclusive" is implied by Mutex / Lock word.
> Fourth, "access to the bus when writing data" too verbose.
>
> So, my suggestion is (two variants):
> a) "Lock to serialize bus access when writing data."
> b) "Lock to serialize access when writing data with serdev_device_write()."
>
I'll use option "a". Option "b" still exceeds line limit, and I'd
rather not have that.
Thanks,
Andrey Smirnov
^ permalink raw reply
* Re: [PATCH] serial: Do not treat the IIR register as a bitfield
From: kbuild test robot @ 2017-03-30 12:18 UTC (permalink / raw)
To: Olliver Schinagl
Cc: kbuild-all-JC7UmRfGjtg, Greg Kroah-Hartman, Jiri Slaby,
Laxman Dewangan, Stephen Warren, Thierry Reding,
Alexandre Courbot, David S . Miller, dev-3kdeTeqwOZ9EV1b7eY7vFQ,
Ed Blake, Andy Shevchenko, Alexander Sverdlin, Yegor Yefremov,
Wan Ahmad Zainie, Kefeng Wang, Heikki Krogerus, Heiko Stuebner,
Jason Uy, Douglas Anderson, Peter Hurley, Tony Lindgren,
Vignesh R, Thor
In-Reply-To: <20170329184431.6226-1-oliver-dxLnbx3+1qmEVqv0pETR8A@public.gmane.org>
Hi Olliver,
[auto build test WARNING on tty/tty-testing]
[also build test WARNING on v4.11-rc4 next-20170330]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Olliver-Schinagl/serial-Do-not-treat-the-IIR-register-as-a-bitfield/20170330-165826
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
reproduce:
# apt-get install sparse
make ARCH=x86_64 allmodconfig
make C=1 CF=-D__CHECK_ENDIAN__
sparse warnings: (new ones prefixed by >>)
include/linux/compiler.h:264:8: sparse: attribute 'no_sanitize_address': unknown attribute
drivers/staging/media/lirc/lirc_sir.c:285:44: sparse: undefined identifier 'UART_IIR_ID'
drivers/staging/media/lirc/lirc_sir.c:286:29: sparse: undefined identifier 'UART_IIR_ID'
>> drivers/staging/media/lirc/lirc_sir.c:287:22: sparse: incompatible types for 'case' statement
drivers/staging/media/lirc/lirc_sir.c:290:22: sparse: incompatible types for 'case' statement
drivers/staging/media/lirc/lirc_sir.c:293:22: sparse: incompatible types for 'case' statement
drivers/staging/media/lirc/lirc_sir.c:299:22: sparse: incompatible types for 'case' statement
drivers/staging/media/lirc/lirc_sir.c: In function 'sir_interrupt':
drivers/staging/media/lirc/lirc_sir.c:285:37: error: 'UART_IIR_ID' undeclared (first use in this function)
while ((iir = inb(io + UART_IIR) & UART_IIR_ID)) {
^~~~~~~~~~~
drivers/staging/media/lirc/lirc_sir.c:285:37: note: each undeclared identifier is reported only once for each function it appears in
vim +/case +287 drivers/staging/media/lirc/lirc_sir.c
34668350 drivers/staging/media/lirc/lirc_sir.c Ksenija Stanojevic 2015-05-22 279 ktime_t curr_time;
34668350 drivers/staging/media/lirc/lirc_sir.c Ksenija Stanojevic 2015-05-22 280 static unsigned long delt;
34668350 drivers/staging/media/lirc/lirc_sir.c Ksenija Stanojevic 2015-05-22 281 unsigned long deltintr;
404f3e95 drivers/staging/lirc/lirc_sir.c Jarod Wilson 2010-07-26 282 unsigned long flags;
404f3e95 drivers/staging/lirc/lirc_sir.c Jarod Wilson 2010-07-26 283 int iir, lsr;
404f3e95 drivers/staging/lirc/lirc_sir.c Jarod Wilson 2010-07-26 284
404f3e95 drivers/staging/lirc/lirc_sir.c Jarod Wilson 2010-07-26 @285 while ((iir = inb(io + UART_IIR) & UART_IIR_ID)) {
404f3e95 drivers/staging/lirc/lirc_sir.c Jarod Wilson 2010-07-26 286 switch (iir&UART_IIR_ID) { /* FIXME toto treba preriedit */
404f3e95 drivers/staging/lirc/lirc_sir.c Jarod Wilson 2010-07-26 @287 case UART_IIR_MSI:
404f3e95 drivers/staging/lirc/lirc_sir.c Jarod Wilson 2010-07-26 288 (void) inb(io + UART_MSR);
404f3e95 drivers/staging/lirc/lirc_sir.c Jarod Wilson 2010-07-26 289 break;
404f3e95 drivers/staging/lirc/lirc_sir.c Jarod Wilson 2010-07-26 290 case UART_IIR_RLSI:
:::::: The code at line 287 was first introduced by commit
:::::: 404f3e956bc7ab03ac604fabf136e69607315f60 V4L/DVB: staging/lirc: add lirc_sir driver
:::::: TO: Jarod Wilson <jarod-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
:::::: CC: Mauro Carvalho Chehab <mchehab-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
^ permalink raw reply
* Re: [kbuild-all] [PATCH] serial: Do not treat the IIR register as a bitfield
From: Fengguang Wu @ 2017-03-30 11:27 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Olliver Schinagl, Kefeng Wang, Heikki Krogerus, Heiko Stuebner,
Tony Lindgren, Jan Kiszka, Yegor Yefremov, Laxman Dewangan,
sparclinux-u79uwXL29TY76Z2rM5mHXA, Vignesh R, Alexandre Courbot,
Wan Ahmad Zainie, Jason Uy, Ed Blake, Thor Thayer,
linux-serial-u79uwXL29TY76Z2rM5mHXA, Jiri Slaby, David Lechner,
Stephen Warren, dev-3kdeTeqwOZ9EV1b7eY7vFQ, Alexander
In-Reply-To: <1490869971.708.66.camel-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
On Thu, Mar 30, 2017 at 01:32:51PM +0300, Andy Shevchenko wrote:
>On Thu, 2017-03-30 at 18:22 +0800, kbuild test robot wrote:
>
>> drivers/net//irda/smsc-ircc2.c:
>
>> > > drivers/net//irda/smsc-ircc2.c:
>
>Just out of my curiosity, why do we have // in some reports?
Log shows the bisect runs
make M=drivers/net/
That should explain the // in error messages.
Thanks,
Fengguang
^ permalink raw reply
* Re: [PATCH] serial: Do not treat the IIR register as a bitfield
From: Andy Shevchenko @ 2017-03-30 10:32 UTC (permalink / raw)
To: kbuild test robot, Olliver Schinagl
Cc: kbuild-all-JC7UmRfGjtg, Greg Kroah-Hartman, Jiri Slaby,
Laxman Dewangan, Stephen Warren, Thierry Reding,
Alexandre Courbot, David S . Miller, dev-3kdeTeqwOZ9EV1b7eY7vFQ,
Ed Blake, Alexander Sverdlin, Yegor Yefremov, Wan Ahmad Zainie,
Kefeng Wang, Heikki Krogerus, Heiko Stuebner, Jason Uy,
Douglas Anderson, Peter Hurley, Tony
In-Reply-To: <201703301843.OZzxWtxO%fengguang.wu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
On Thu, 2017-03-30 at 18:22 +0800, kbuild test robot wrote:
> drivers/net//irda/smsc-ircc2.c:
> > > drivers/net//irda/smsc-ircc2.c:
Just out of my curiosity, why do we have // in some reports?
--
Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Intel Finland Oy
^ permalink raw reply
* Re: [PATCH] serial: Do not treat the IIR register as a bitfield
From: kbuild test robot @ 2017-03-30 10:22 UTC (permalink / raw)
To: Olliver Schinagl
Cc: kbuild-all-JC7UmRfGjtg, Greg Kroah-Hartman, Jiri Slaby,
Laxman Dewangan, Stephen Warren, Thierry Reding,
Alexandre Courbot, David S . Miller, dev-3kdeTeqwOZ9EV1b7eY7vFQ,
Ed Blake, Andy Shevchenko, Alexander Sverdlin, Yegor Yefremov,
Wan Ahmad Zainie, Kefeng Wang, Heikki Krogerus, Heiko Stuebner,
Jason Uy, Douglas Anderson, Peter Hurley, Tony Lindgren,
Vignesh R, Thor
In-Reply-To: <20170329184431.6226-1-oliver-dxLnbx3+1qmEVqv0pETR8A@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 2510 bytes --]
Hi Olliver,
[auto build test ERROR on tty/tty-testing]
[also build test ERROR on v4.11-rc4 next-20170330]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Olliver-Schinagl/serial-Do-not-treat-the-IIR-register-as-a-bitfield/20170330-165826
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
config: x86_64-randconfig-i0-201713 (attached as .config)
compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
drivers/net//irda/smsc-ircc2.c: In function 'smsc_ircc_interrupt_sir':
>> drivers/net//irda/smsc-ircc2.c:1582:33: error: 'UART_IIR_ID' undeclared (first use in this function)
iir = inb(iobase + UART_IIR) & UART_IIR_ID;
^
drivers/net//irda/smsc-ircc2.c:1582:33: note: each undeclared identifier is reported only once for each function it appears in
vim +/UART_IIR_ID +1582 drivers/net//irda/smsc-ircc2.c
^1da177e Linus Torvalds 2005-04-16 1576
25985edc Lucas De Marchi 2011-03-30 1577 /* Already locked coming here in smsc_ircc_interrupt() */
^1da177e Linus Torvalds 2005-04-16 1578 /*spin_lock(&self->lock);*/
^1da177e Linus Torvalds 2005-04-16 1579
^1da177e Linus Torvalds 2005-04-16 1580 iobase = self->io.sir_base;
^1da177e Linus Torvalds 2005-04-16 1581
^1da177e Linus Torvalds 2005-04-16 @1582 iir = inb(iobase + UART_IIR) & UART_IIR_ID;
^1da177e Linus Torvalds 2005-04-16 1583 if (iir == 0)
^1da177e Linus Torvalds 2005-04-16 1584 return IRQ_NONE;
^1da177e Linus Torvalds 2005-04-16 1585 while (iir) {
:::::: The code at line 1582 was first introduced by commit
:::::: 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 Linux-2.6.12-rc2
:::::: TO: Linus Torvalds <torvalds-gWtpgVMusWVb5UGfqNBoRg@public.gmane.org>
:::::: CC: Linus Torvalds <torvalds-gWtpgVMusWVb5UGfqNBoRg@public.gmane.org>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
--
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 27137 bytes --]
^ permalink raw reply
* Re: [PATCH] serial: tegra: Map the iir register to default defines
From: Laxman Dewangan @ 2017-03-30 10:17 UTC (permalink / raw)
To: Olliver Schinagl, Greg Kroah-Hartman, Jiri Slaby, Stephen Warren,
Thierry Reding, Alexandre Courbot
Cc: linux-serial, linux-tegra, linux-kernel, Shardar Mohammed
In-Reply-To: <20170329184806.6577-1-oliver@schinagl.nl>
On Thursday 30 March 2017 12:18 AM, Olliver Schinagl wrote:
> The tegra serial IP seems to be following the common layout and the
> interrupt ID's match up nicely. Replace the magic values to match the
> common serial_reg defines, with the addition of the Tegra unique End of
> Data interrupt.
>
> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
> ---
Adding Shardar for verifications.
Acked-by: Laxman Dewangan <ldewangan@nvidia.com>
^ permalink raw reply
* Re: [PATCH] serial: Do not treat the IIR register as a bitfield
From: kbuild test robot @ 2017-03-30 10:06 UTC (permalink / raw)
To: Olliver Schinagl
Cc: kbuild-all-JC7UmRfGjtg, Greg Kroah-Hartman, Jiri Slaby,
Laxman Dewangan, Stephen Warren, Thierry Reding,
Alexandre Courbot, David S . Miller, dev-3kdeTeqwOZ9EV1b7eY7vFQ,
Ed Blake, Andy Shevchenko, Alexander Sverdlin, Yegor Yefremov,
Wan Ahmad Zainie, Kefeng Wang, Heikki Krogerus, Heiko Stuebner,
Jason Uy, Douglas Anderson, Peter Hurley, Tony Lindgren,
Vignesh R, Thor
In-Reply-To: <20170329184431.6226-1-oliver-dxLnbx3+1qmEVqv0pETR8A@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 2440 bytes --]
Hi Olliver,
[auto build test ERROR on tty/tty-testing]
[also build test ERROR on v4.11-rc4 next-20170329]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Olliver-Schinagl/serial-Do-not-treat-the-IIR-register-as-a-bitfield/20170330-165826
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
config: i386-randconfig-s0-201713 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
drivers/net/irda/ali-ircc.c: In function 'ali_ircc_sir_interrupt':
>> drivers/net/irda/ali-ircc.c:816:31: error: 'UART_IIR_ID' undeclared (first use in this function)
iir = inb(iobase+UART_IIR) & UART_IIR_ID;
^~~~~~~~~~~
drivers/net/irda/ali-ircc.c:816:31: note: each undeclared identifier is reported only once for each function it appears in
vim +/UART_IIR_ID +816 drivers/net/irda/ali-ircc.c
^1da177e Linus Torvalds 2005-04-16 810 int iobase;
^1da177e Linus Torvalds 2005-04-16 811 int iir, lsr;
^1da177e Linus Torvalds 2005-04-16 812
^1da177e Linus Torvalds 2005-04-16 813
^1da177e Linus Torvalds 2005-04-16 814 iobase = self->io.sir_base;
^1da177e Linus Torvalds 2005-04-16 815
^1da177e Linus Torvalds 2005-04-16 @816 iir = inb(iobase+UART_IIR) & UART_IIR_ID;
^1da177e Linus Torvalds 2005-04-16 817 if (iir) {
^1da177e Linus Torvalds 2005-04-16 818 /* Clear interrupt */
^1da177e Linus Torvalds 2005-04-16 819 lsr = inb(iobase+UART_LSR);
:::::: The code at line 816 was first introduced by commit
:::::: 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 Linux-2.6.12-rc2
:::::: TO: Linus Torvalds <torvalds-gWtpgVMusWVb5UGfqNBoRg@public.gmane.org>
:::::: CC: Linus Torvalds <torvalds-gWtpgVMusWVb5UGfqNBoRg@public.gmane.org>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
--
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 31211 bytes --]
^ permalink raw reply
* Re: [PATCH] serial: Do not treat the IIR register as a bitfield
From: Andy Shevchenko @ 2017-03-30 9:56 UTC (permalink / raw)
To: Olliver Schinagl, Greg Kroah-Hartman, Jiri Slaby, Laxman Dewangan,
Stephen Warren, Thierry Reding, Alexandre Courbot,
David S . Miller
Cc: dev, Ed Blake, Alexander Sverdlin, Yegor Yefremov,
Wan Ahmad Zainie, Kefeng Wang, Heikki Krogerus, Heiko Stuebner,
Jason Uy, Douglas Anderson, Peter Hurley, Tony Lindgren,
Vignesh R, Thor Thayer, David Lechner, Jan Kiszka, linux-serial,
linux-kernel, linux-tegra, sparclinux
In-Reply-To: <20170329184431.6226-1-oliver@schinagl.nl>
On Wed, 2017-03-29 at 20:44 +0200, Olliver Schinagl wrote:
> It seems that at some point, someone made the assumption that the UART
> Interrupt ID Register was a bitfield and started to check if certain
> bits where set.
>
> Actually however the register contains interrupt ID's where only the
> MSB
> seems to be used singular and the rest share at least one bit. Thus
> doing bitfield operations is wrong.
>
> This patch cleans up the serial_reg include file by ordering it and
> replacing the UART_IIR_ID 'mask' with a proper mask for the register.
> The OMAP uart appears to have used the two commonly 'reserved' bits 4
> and 5 and thus get an UART_IIR_EXT_MASK for these two bits.
>
> This patch then goes over all UART_IIR_* users and changes the code
> from
> bitfield checking, to ID checking instead.
Looking to implementation I would rather go with some helper like
int serial_in_IIR(port, [additional mask])
{
return port->serial_in(port, UART_IIR) & (_IIR_MASK [| additional
mask]);
}
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply
* Re: [PATCH] serial: Do not treat the IIR register as a bitfield
From: Vignesh R @ 2017-03-30 7:57 UTC (permalink / raw)
To: Olliver Schinagl, Greg Kroah-Hartman, Jiri Slaby, Laxman Dewangan,
Stephen Warren, Thierry Reding, Alexandre Courbot,
David S . Miller
Cc: dev-3kdeTeqwOZ9EV1b7eY7vFQ@public.gmane.org, Ed Blake,
Andy Shevchenko, Alexander Sverdlin, Yegor Yefremov,
Wan Ahmad Zainie, Kefeng Wang, Heikki Krogerus, Heiko Stuebner,
Jason Uy, Douglas Anderson, Peter Hurley, Tony Lindgren,
Thor Thayer, David Lechner, Jan Kiszka,
linux-serial-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel
In-Reply-To: <D694FA74-1F31-4B57-8858-2DBE8DE57D45-dxLnbx3+1qmEVqv0pETR8A@public.gmane.org>
On Thursday 30 March 2017 12:13 PM, Olliver Schinagl wrote:
>
>
> On March 30, 2017 8:15:29 AM CEST, Vignesh R <vigneshr-l0cyMroinI0@public.gmane.org> wrote:
>> Hi,
>>
>> On Thursday 30 March 2017 12:14 AM, Olliver Schinagl wrote:
>>> diff --git a/include/uapi/linux/serial_reg.h
>> b/include/uapi/linux/serial_reg.h
>>> index 5db76880b4ad..489522389a10 100644
>>> --- a/include/uapi/linux/serial_reg.h
>>> +++ b/include/uapi/linux/serial_reg.h
>>> @@ -31,18 +31,18 @@
>>> #define UART_IERX_SLEEP 0x10 /* Enable sleep mode */
>>>
>>> #define UART_IIR 2 /* In: Interrupt ID Register */
>>> -#define UART_IIR_NO_INT 0x01 /* No interrupts pending */
>>> -#define UART_IIR_ID 0x0e /* Mask for the interrupt ID */
>>> #define UART_IIR_MSI 0x00 /* Modem status interrupt */
>>> +#define UART_IIR_NO_INT 0x01 /* No interrupts pending */
>>> #define UART_IIR_THRI 0x02 /* Transmitter holding register empty */
>>> #define UART_IIR_RDI 0x04 /* Receiver data interrupt */
>>> #define UART_IIR_RLSI 0x06 /* Receiver line status interrupt */
>>> -
>>> #define UART_IIR_BUSY 0x07 /* DesignWare APB Busy Detect */
>>> +#define UART_IIR_RX_TIMEOUT 0x0c /* DesignWare RX Timeout interrupt
>> */
> It was moved due to sorting. The comment could be changed maybe? I renamed it from omap to dw as i believe the omap also uses the dw ip. But i think it is a defacto standard mapping of the irr register?
AFAIK, OMAP UART does not use DW core, please make these IDs generic to
avoid confusion.
>
>>> +#define UART_IIR_MASK 0x0f /* DesignWare IIR mask */
>>>
>>> -#define UART_IIR_RX_TIMEOUT 0x0c /* OMAP RX Timeout interrupt */
>>
>> You are removing UART_IIR_RX_TIMEOUT? Is this intended?
>>
>>> #define UART_IIR_XOFF 0x10 /* OMAP XOFF/Special Character */
>>> #define UART_IIR_CTS_RTS_DSR 0x20 /* OMAP CTS/RTS/DSR Change */
>>> +#define UART_IIR_EXT_MASK 0x30 /* OMAP extended IIR mask */
>
--
Regards
Vignesh
^ permalink raw reply
* Re: [PATCH] serial: Do not treat the IIR register as a bitfield
From: Olliver Schinagl @ 2017-03-30 6:43 UTC (permalink / raw)
To: Vignesh R, Greg Kroah-Hartman, Jiri Slaby, Laxman Dewangan,
Stephen Warren, Thierry Reding, Alexandre Courbot,
David S . Miller
Cc: dev@linux-sunxi.org, Ed Blake, Andy Shevchenko,
Alexander Sverdlin, Yegor Yefremov, Wan Ahmad Zainie, Kefeng Wang,
Heikki Krogerus, Heiko Stuebner, Jason Uy, Douglas Anderson,
Peter Hurley, Tony Lindgren, Thor Thayer, David Lechner,
Jan Kiszka, linux-serial@vger.kernel.org, linux-kernel@
In-Reply-To: <6be65e8b-eea4-08ed-0b30-5c0608764a83@ti.com>
On March 30, 2017 8:15:29 AM CEST, Vignesh R <vigneshr@ti.com> wrote:
>Hi,
>
>On Thursday 30 March 2017 12:14 AM, Olliver Schinagl wrote:
>> diff --git a/include/uapi/linux/serial_reg.h
>b/include/uapi/linux/serial_reg.h
>> index 5db76880b4ad..489522389a10 100644
>> --- a/include/uapi/linux/serial_reg.h
>> +++ b/include/uapi/linux/serial_reg.h
>> @@ -31,18 +31,18 @@
>> #define UART_IERX_SLEEP 0x10 /* Enable sleep mode */
>>
>> #define UART_IIR 2 /* In: Interrupt ID Register */
>> -#define UART_IIR_NO_INT 0x01 /* No interrupts pending */
>> -#define UART_IIR_ID 0x0e /* Mask for the interrupt ID */
>> #define UART_IIR_MSI 0x00 /* Modem status interrupt */
>> +#define UART_IIR_NO_INT 0x01 /* No interrupts pending */
>> #define UART_IIR_THRI 0x02 /* Transmitter holding register empty */
>> #define UART_IIR_RDI 0x04 /* Receiver data interrupt */
>> #define UART_IIR_RLSI 0x06 /* Receiver line status interrupt */
>> -
>> #define UART_IIR_BUSY 0x07 /* DesignWare APB Busy Detect */
>> +#define UART_IIR_RX_TIMEOUT 0x0c /* DesignWare RX Timeout interrupt
>*/
It was moved due to sorting. The comment could be changed maybe? I renamed it from omap to dw as i believe the omap also uses the dw ip. But i think it is a defacto standard mapping of the irr register?
>> +#define UART_IIR_MASK 0x0f /* DesignWare IIR mask */
>>
>> -#define UART_IIR_RX_TIMEOUT 0x0c /* OMAP RX Timeout interrupt */
>
>You are removing UART_IIR_RX_TIMEOUT? Is this intended?
>
>> #define UART_IIR_XOFF 0x10 /* OMAP XOFF/Special Character */
>> #define UART_IIR_CTS_RTS_DSR 0x20 /* OMAP CTS/RTS/DSR Change */
>> +#define UART_IIR_EXT_MASK 0x30 /* OMAP extended IIR mask */
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
^ permalink raw reply
* Re: [PATCH] serial: Do not treat the IIR register as a bitfield
From: Vignesh R @ 2017-03-30 6:15 UTC (permalink / raw)
To: Olliver Schinagl, Greg Kroah-Hartman, Jiri Slaby, Laxman Dewangan,
Stephen Warren, Thierry Reding, Alexandre Courbot,
David S . Miller
Cc: dev-3kdeTeqwOZ9EV1b7eY7vFQ@public.gmane.org, Ed Blake,
Andy Shevchenko, Alexander Sverdlin, Yegor Yefremov,
Wan Ahmad Zainie, Kefeng Wang, Heikki Krogerus, Heiko Stuebner,
Jason Uy, Douglas Anderson, Peter Hurley, Tony Lindgren,
Thor Thayer, David Lechner, Jan Kiszka,
linux-serial-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel
In-Reply-To: <20170329184431.6226-1-oliver-dxLnbx3+1qmEVqv0pETR8A@public.gmane.org>
Hi,
On Thursday 30 March 2017 12:14 AM, Olliver Schinagl wrote:
> diff --git a/include/uapi/linux/serial_reg.h b/include/uapi/linux/serial_reg.h
> index 5db76880b4ad..489522389a10 100644
> --- a/include/uapi/linux/serial_reg.h
> +++ b/include/uapi/linux/serial_reg.h
> @@ -31,18 +31,18 @@
> #define UART_IERX_SLEEP 0x10 /* Enable sleep mode */
>
> #define UART_IIR 2 /* In: Interrupt ID Register */
> -#define UART_IIR_NO_INT 0x01 /* No interrupts pending */
> -#define UART_IIR_ID 0x0e /* Mask for the interrupt ID */
> #define UART_IIR_MSI 0x00 /* Modem status interrupt */
> +#define UART_IIR_NO_INT 0x01 /* No interrupts pending */
> #define UART_IIR_THRI 0x02 /* Transmitter holding register empty */
> #define UART_IIR_RDI 0x04 /* Receiver data interrupt */
> #define UART_IIR_RLSI 0x06 /* Receiver line status interrupt */
> -
> #define UART_IIR_BUSY 0x07 /* DesignWare APB Busy Detect */
> +#define UART_IIR_RX_TIMEOUT 0x0c /* DesignWare RX Timeout interrupt */
> +#define UART_IIR_MASK 0x0f /* DesignWare IIR mask */
>
> -#define UART_IIR_RX_TIMEOUT 0x0c /* OMAP RX Timeout interrupt */
You are removing UART_IIR_RX_TIMEOUT? Is this intended?
> #define UART_IIR_XOFF 0x10 /* OMAP XOFF/Special Character */
> #define UART_IIR_CTS_RTS_DSR 0x20 /* OMAP CTS/RTS/DSR Change */
> +#define UART_IIR_EXT_MASK 0x30 /* OMAP extended IIR mask */
--
Regards
Vignesh
^ permalink raw reply
* Re: [PATCH v8 3/3] printk: fix double printing with earlycon
From: Sergey Senozhatsky @ 2017-03-30 5:55 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, Aleksey Makarov, linux-serial, linux-kernel,
Sudeep Holla, Greg Kroah-Hartman, Peter Hurley, Jiri Slaby,
Robin Murphy, Steven Rostedt, Nair, Jayachandran,
Sergey Senozhatsky
In-Reply-To: <20170328125657.GJ2846@pathway.suse.cz>
On (03/28/17 14:56), Petr Mladek wrote:
[..]
> > > Is it better? If not, I will send a version with console_cmdline_last.
> >
> > personally I'm fine with the nested loop. the latest version
> > "for (last = MAX_CMDLINECONSOLES - 1; last >= 0;..."
> >
> > is even easier to read.
>
> The number of elements is bumped on a single location, so there
> is not much to synchronize. The old approach was fine because
> the for cycles were needed anyway, they started on the 0th element,
> and NULL ended arrays are rather common practice.
>
> But we are searching the array from the end now. Also we use the
> for cycle just to get the number here. This is not a common
> practice and it makes the code more complicated and strange from
> my point of view.
I'm fine with either way :)
[..]
> > neither add_preferred_console() nor __add_preferred_console() have any
> > serialization. and I assume that we can call add_preferred_console()
> > concurrently, can't we?
[..]
> If I did not miss anything, it would seem that
> __add_preferred_console() are called synchronously
> and only during boot by design.
thanks. I think you are right. it's console_initcall or __init.
-ss
^ permalink raw reply
* Re: [PATCHv3 00/10] Nokia H4+ support
From: Marcel Holtmann @ 2017-03-29 21:33 UTC (permalink / raw)
To: Rob Herring
Cc: Sebastian Reichel, Gustavo F. Padovan, Johan Hedberg,
Samuel Thibault, Pavel Machek, Tony Lindgren, Greg Kroah-Hartman,
Jiri Slaby, Mark Rutland, open list:BLUETOOTH DRIVERS,
linux-serial@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org, David S. Miller
In-Reply-To: <CAL_JsqJcQ9ANtt71KRitOJ9spKD1SaeoL4RDftiCEPs8TjLJGA@mail.gmail.com>
Hi Rob,
>> Here is PATCHv3 for the Nokia bluetooth patchset. I addressed all comments from
>> Rob and Pavel regarding the serdev patches and dropped the *.dts patches, since
>> they were queued by Tony. I also changed the patch order, so that the serdev
>> patches come first. All of them have Acked-by from Rob, so I think it makes
>> sense to merge them to serdev subsystem (now) and provide an immutable branch
>> for the bluetooth subsystem.
>
> Greg doesn't read cover letters generally and since the serdev patches
> are Cc rather than To him, he's probably not planning to pick them up.
I wonder actually if we should merge all of these via bluetooth-next tree with proper Ack from Greg. However it would be good to also get buy in from Dave for merging this ultimately through net-next.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH v2] serdev: Replace serdev_device_write_buf with serdev_device_write
From: Rob Herring @ 2017-03-29 21:25 UTC (permalink / raw)
To: Andrey Smirnov
Cc: Andy Shevchenko, Chris Healy, Guenter Roeck,
linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <CAHQ1cqG8jD-aaoD2=jqierhx5+WfgqGQiO9b8qz2uzJyWxTPdQ@mail.gmail.com>
On Wed, Mar 29, 2017 at 9:16 AM, Andrey Smirnov
<andrew.smirnov@gmail.com> wrote:
> On Tue, Mar 28, 2017 at 10:07 AM, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>> On Tue, Mar 28, 2017 at 7:01 PM, Andrey Smirnov
>> <andrew.smirnov@gmail.com> wrote:
>>> Convert serdev_device_write_buf's code to be able to transfer amount of
>>> data potentially exceeding "write room" at the moment of invocation.
>>>
>>> To support that, also add serdev_device_write_wakeup.
>>>
>>> Drivers wanting to use full extent of serdev_device_write
>>> functionality are expected to provide serdev_device_write_wakeup as a
>>> sole handler of .write_wakeup event or call it as a part of driver's
>>> custom .write_wakeup code.
>>>
>>> Drivers wanting to retain old serdev_device_write_buf behaviour can
>>
>>> replace those call to calls to serdev_device_write with timeout of
>>> 0. Providing .write_wakeup handler in such case is optional.
>>
>> Some indentation would be better if, for example, 0 will be kept on
>> previous line.
>>
>
> OK, sure.
>
>> So, what I would see if no one objects is patch series of two:
>> 1) introduction of new API
>> 2) removing old one.
>>
>> It will benefit for easier review and any potential code anthropologist.
>>
>
> Second version of the patch preserves the old API an just
> re-implements it in terms of a new one. I am not sure I see the
> benefit in splitting it into two patches, but I'll leave it up to Rob
> to decide.
I think it is fine as-is, but maybe the subject now is a bit misleading.
>>> --- a/drivers/tty/serdev/core.c
>>> +++ b/drivers/tty/serdev/core.c
>>> @@ -116,17 +116,41 @@ void serdev_device_close(struct serdev_device *serdev)
>>> }
>>> EXPORT_SYMBOL_GPL(serdev_device_close);
>>>
>>> -int serdev_device_write_buf(struct serdev_device *serdev,
>>> - const unsigned char *buf, size_t count)
>>> +void serdev_device_write_wakeup(struct serdev_device *serdev)
>>> +{
>>> + complete(&serdev->write_comp);
>>> +}
>>> +EXPORT_SYMBOL_GPL(serdev_device_write_wakeup);
>>> +
>>> +int serdev_device_write(struct serdev_device *serdev,
>>> + const unsigned char *buf, size_t count,
>>> + unsigned long timeout)
>>> {
>>> struct serdev_controller *ctrl = serdev->ctrl;
>>> + int ret;
>>>
>>> - if (!ctrl || !ctrl->ops->write_buf)
>>> + if (!ctrl || !ctrl->ops->write_buf ||
>>> + (timeout && !serdev->ops->write_wakeup))
>>> return -EINVAL;
>>>
>>> - return ctrl->ops->write_buf(ctrl, buf, count);
>>> + mutex_lock(&serdev->write_lock);
>>> + do {
>>> + reinit_completion(&serdev->write_comp);
>>> +
>>> + ret = ctrl->ops->write_buf(ctrl, buf, count);
>>> + if (ret < 0)
>>> + break;
>>> +
>>
>>> + buf += ret;
>>
>> Extra white spaces.
>
> Which is there on purpose to re-align "+=" with "-=" on the next line.
> I'll remove it.
>
>>
>>> + count -= ret;
>>> +
>>
>>> + } while (count &&
>>> + (timeout = wait_for_completion_timeout(&serdev->write_comp,
>>> + timeout)));
>>
>> So, would it be better to support interrupts here and return a
>> corresponding error code to the user?
>>
>
> I don't have a use-case for that and as far as I can tell, neither SPI
> nor I2C slave device API offer such functionality universally, so I am
> inclined to say no. Since the change from wait_for_completion to
> wait_for_completion_timeout was made per Rob's request, I'd leave it
> up to him to decided about this change as well.
Honestly, I don't know. It's added easily enough if needed later.
Rob
^ permalink raw reply
* Re: [PATCHv3 00/10] Nokia H4+ support
From: Rob Herring @ 2017-03-29 21:17 UTC (permalink / raw)
To: Sebastian Reichel
Cc: Marcel Holtmann, Gustavo Padovan, Johan Hedberg, Samuel Thibault,
Pavel Machek, Tony Lindgren, Greg Kroah-Hartman, Jiri Slaby,
Mark Rutland, open list:BLUETOOTH DRIVERS,
linux-serial@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20170328155939.31566-1-sre@kernel.org>
On Tue, Mar 28, 2017 at 10:59 AM, Sebastian Reichel <sre@kernel.org> wrote:
> Hi,
>
> Here is PATCHv3 for the Nokia bluetooth patchset. I addressed all comments from
> Rob and Pavel regarding the serdev patches and dropped the *.dts patches, since
> they were queued by Tony. I also changed the patch order, so that the serdev
> patches come first. All of them have Acked-by from Rob, so I think it makes
> sense to merge them to serdev subsystem (now) and provide an immutable branch
> for the bluetooth subsystem.
Greg doesn't read cover letters generally and since the serdev patches
are Cc rather than To him, he's probably not planning to pick them up.
Rob
^ permalink raw reply
* [PATCH] serial: tegra: Map the iir register to default defines
From: Olliver Schinagl @ 2017-03-29 18:48 UTC (permalink / raw)
To: Laxman Dewangan, Greg Kroah-Hartman, Jiri Slaby, Stephen Warren,
Thierry Reding, Alexandre Courbot
Cc: linux-serial, linux-tegra, linux-kernel, Olliver Schinagl
The tegra serial IP seems to be following the common layout and the
interrupt ID's match up nicely. Replace the magic values to match the
common serial_reg defines, with the addition of the Tegra unique End of
Data interrupt.
Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
---
Note I do not own any tegra hardware and just noticed it while working on my
somewhat related previous patch,
"serial: Do not treat the IIR register as a bitfield"
As such, this patch can only be applied after the aforementioned patch or the
iir variable will not have its mask applied yet.
drivers/tty/serial/serial-tegra.c | 19 ++++++++++---------
1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/drivers/tty/serial/serial-tegra.c b/drivers/tty/serial/serial-tegra.c
index 4a084161d1d2..f765999dcbfe 100644
--- a/drivers/tty/serial/serial-tegra.c
+++ b/drivers/tty/serial/serial-tegra.c
@@ -83,6 +83,8 @@
#define TEGRA_TX_PIO 1
#define TEGRA_TX_DMA 2
+#define TEGRA_UART_IIR_EOD 0x8
+
/**
* tegra_uart_chip_data: SOC specific data.
*
@@ -707,20 +709,20 @@ static irqreturn_t tegra_uart_isr(int irq, void *data)
return IRQ_HANDLED;
}
- switch ((iir >> 1) & 0x7) {
- case 0: /* Modem signal change interrupt */
+ switch (iir) {
+ case UART_IIR_MSI: /* Modem signal change interrupt */
tegra_uart_handle_modem_signal_change(u);
break;
- case 1: /* Transmit interrupt only triggered when using PIO */
+ case UART_IIR_THRI: /* Transmit interrupt only triggered when using PIO */
tup->ier_shadow &= ~UART_IER_THRI;
tegra_uart_write(tup, tup->ier_shadow, UART_IER);
tegra_uart_handle_tx_pio(tup);
break;
- case 4: /* End of data */
- case 6: /* Rx timeout */
- case 2: /* Receive */
+ case TEGRA_UART_IIR_EOD: /* End of data */
+ case UART_IIR_RX_TIMEOUT: /* Rx timeout */
+ case UART_IIR_RDI: /* Receive */
if (!is_rx_int) {
is_rx_int = true;
/* Disable Rx interrupts */
@@ -734,13 +736,12 @@ static irqreturn_t tegra_uart_isr(int irq, void *data)
}
break;
- case 3: /* Receive error */
+ case UART_IIR_RLSI: /* Receive error */
tegra_uart_decode_rx_error(tup,
tegra_uart_read(tup, UART_LSR));
break;
- case 5: /* break nothing to handle */
- case 7: /* break nothing to handle */
+ default:
break;
}
}
--
2.11.0
^ permalink raw reply related
* [PATCH] serial: Do not treat the IIR register as a bitfield
From: Olliver Schinagl @ 2017-03-29 18:44 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby, Laxman Dewangan, Stephen Warren,
Thierry Reding, Alexandre Courbot, David S . Miller
Cc: dev-3kdeTeqwOZ9EV1b7eY7vFQ, Ed Blake, Andy Shevchenko,
Alexander Sverdlin, Yegor Yefremov, Wan Ahmad Zainie, Kefeng Wang,
Heikki Krogerus, Heiko Stuebner, Jason Uy, Douglas Anderson,
Peter Hurley, Tony Lindgren, Vignesh R, Thor Thayer,
David Lechner, Jan Kiszka, linux-serial-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA,
sparclinux-u79uwXL29TY76Z2rM5mHXA, Olliver Schinagl
It seems that at some point, someone made the assumption that the UART
Interrupt ID Register was a bitfield and started to check if certain
bits where set.
Actually however the register contains interrupt ID's where only the MSB
seems to be used singular and the rest share at least one bit. Thus
doing bitfield operations is wrong.
This patch cleans up the serial_reg include file by ordering it and
replacing the UART_IIR_ID 'mask' with a proper mask for the register.
The OMAP uart appears to have used the two commonly 'reserved' bits 4
and 5 and thus get an UART_IIR_EXT_MASK for these two bits.
This patch then goes over all UART_IIR_* users and changes the code from
bitfield checking, to ID checking instead.
Signed-off-by: Olliver Schinagl <oliver-dxLnbx3+1qmEVqv0pETR8A@public.gmane.org>
---
Note, that I do not have all this hardware and used the fact that UART_IIR_*
yields ID's rather then bitfields and thus mentioned as above, cannot be
bit-checked. Please carefully look at the changes, as they do make sense to me
a slip up is quickly made. Additionally, note the old code worked probably 100%
of the time, but was written wrong I think.
If I was wrong, I do apologize for the noise.
drivers/tty/serial/8250/8250_core.c | 8 +++++---
drivers/tty/serial/8250/8250_dw.c | 5 +++--
drivers/tty/serial/8250/8250_fsl.c | 3 ++-
drivers/tty/serial/8250/8250_omap.c | 4 ++--
drivers/tty/serial/8250/8250_port.c | 11 ++++++-----
drivers/tty/serial/omap-serial.c | 12 ++++++------
drivers/tty/serial/pxa.c | 2 +-
drivers/tty/serial/serial-tegra.c | 3 ++-
drivers/tty/serial/sunsu.c | 2 +-
drivers/tty/serial/vr41xx_siu.c | 2 +-
include/uapi/linux/serial_reg.h | 8 ++++----
11 files changed, 33 insertions(+), 27 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index 76e03a7de9cc..11b4e3fb0c1d 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -288,6 +288,7 @@ static void serial8250_backup_timeout(unsigned long data)
}
iir = serial_in(up, UART_IIR);
+ iir &= UART_IIR_MASK;
/*
* This should be a safe test for anyone who doesn't trust the
@@ -297,14 +298,15 @@ static void serial8250_backup_timeout(unsigned long data)
*/
lsr = serial_in(up, UART_LSR);
up->lsr_saved_flags |= lsr & LSR_SAVE_FLAGS;
- if ((iir & UART_IIR_NO_INT) && (up->ier & UART_IER_THRI) &&
+ if ((iir == UART_IIR_NO_INT) &&
+ (up->ier & UART_IER_THRI) &&
(!uart_circ_empty(&up->port.state->xmit) || up->port.x_char) &&
(lsr & UART_LSR_THRE)) {
- iir &= ~(UART_IIR_ID | UART_IIR_NO_INT);
+ iir &= ~(UART_IIR_MASK);
iir |= UART_IIR_THRI;
}
- if (!(iir & UART_IIR_NO_INT))
+ if (iir != UART_IIR_NO_INT)
serial8250_tx_chars(up);
if (up->port.irq)
diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 56aded887771..90cf0e6b4c5b 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -218,7 +218,8 @@ static int dw8250_handle_irq(struct uart_port *p)
* This problem has only been observed so far when not in DMA mode
* so we limit the workaround only to non-DMA mode.
*/
- if (!up->dma && ((iir & 0x3f) == UART_IIR_RX_TIMEOUT)) {
+ iir &= UART_IIR_MASK;
+ if (!up->dma && (iir == UART_IIR_RX_TIMEOUT)) {
spin_lock_irqsave(&p->lock, flags);
status = p->serial_in(p, UART_LSR);
@@ -231,7 +232,7 @@ static int dw8250_handle_irq(struct uart_port *p)
if (serial8250_handle_irq(p, iir))
return 1;
- if ((iir & UART_IIR_BUSY) == UART_IIR_BUSY) {
+ if (iir == UART_IIR_BUSY) {
/* Clear the USR */
(void)p->serial_in(p, d->usr_reg);
diff --git a/drivers/tty/serial/8250/8250_fsl.c b/drivers/tty/serial/8250/8250_fsl.c
index 910bfee5a88b..b866343bb485 100644
--- a/drivers/tty/serial/8250/8250_fsl.c
+++ b/drivers/tty/serial/8250/8250_fsl.c
@@ -33,7 +33,8 @@ int fsl8250_handle_irq(struct uart_port *port)
spin_lock_irqsave(&up->port.lock, flags);
iir = port->serial_in(port, UART_IIR);
- if (iir & UART_IIR_NO_INT) {
+ iir &= UART_IIR_MASK;
+ if (iir == UART_IIR_NO_INT) {
spin_unlock_irqrestore(&up->port.lock, flags);
return 0;
}
diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
index e7e64913a748..ab7c2327d410 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -996,7 +996,7 @@ static int omap_8250_tx_dma(struct uart_8250_port *p)
static bool handle_rx_dma(struct uart_8250_port *up, unsigned int iir)
{
- switch (iir & 0x3f) {
+ switch (iir & (UART_IIR_MASK | UART_IIR_EXT_MASK)) {
case UART_IIR_RLSI:
case UART_IIR_RX_TIMEOUT:
case UART_IIR_RDI:
@@ -1021,7 +1021,7 @@ static int omap_8250_dma_handle_irq(struct uart_port *port)
serial8250_rpm_get(up);
iir = serial_port_in(port, UART_IIR);
- if (iir & UART_IIR_NO_INT) {
+ if ((iir & UART_IIR_MASK) == UART_IIR_NO_INT) {
serial8250_rpm_put(up);
return 0;
}
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 6119516ef5fc..8ee3204d63ba 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -1806,7 +1806,7 @@ EXPORT_SYMBOL_GPL(serial8250_modem_status);
static bool handle_rx_dma(struct uart_8250_port *up, unsigned int iir)
{
- switch (iir & 0x3f) {
+ switch (iir & UART_IIR_MASK) {
case UART_IIR_RX_TIMEOUT:
serial8250_rx_dma_flush(up);
/* fall-through */
@@ -1825,7 +1825,7 @@ int serial8250_handle_irq(struct uart_port *port, unsigned int iir)
unsigned long flags;
struct uart_8250_port *up = up_to_u8250p(port);
- if (iir & UART_IIR_NO_INT)
+ if ((iir & UART_IIR_MASK) == UART_IIR_NO_INT)
return 0;
spin_lock_irqsave(&port->lock, flags);
@@ -1896,7 +1896,7 @@ static int serial8250_tx_threshold_handle_irq(struct uart_port *port)
unsigned int iir = serial_port_in(port, UART_IIR);
/* TX Threshold IRQ triggered so load up FIFO */
- if ((iir & UART_IIR_ID) == UART_IIR_THRI) {
+ if ((iir & UART_IIR_MASK) == UART_IIR_THRI) {
struct uart_8250_port *up = up_to_u8250p(port);
spin_lock_irqsave(&port->lock, flags);
@@ -2262,7 +2262,8 @@ int serial8250_do_startup(struct uart_port *port)
* don't trust the iir, setup a timer to kick the UART
* on a regular basis.
*/
- if ((!(iir1 & UART_IIR_NO_INT) && (iir & UART_IIR_NO_INT)) ||
+ if ((((iir1 & UART_IIR_MASK) != UART_IIR_NO_INT) &&
+ ((iir & UART_IIR_MASK) == UART_IIR_NO_INT)) ||
up->port.flags & UPF_BUG_THRE) {
up->bugs |= UART_BUG_THRE;
}
@@ -2313,7 +2314,7 @@ int serial8250_do_startup(struct uart_port *port)
iir = serial_port_in(port, UART_IIR);
serial_port_out(port, UART_IER, 0);
- if (lsr & UART_LSR_TEMT && iir & UART_IIR_NO_INT) {
+ if (lsr & UART_LSR_TEMT && ((iir & UART_IIR_MASK) == UART_IIR_NO_INT)) {
if (!(up->bugs & UART_BUG_TXEN)) {
up->bugs |= UART_BUG_TXEN;
pr_debug("ttyS%d - enabling bad tx status workarounds\n",
diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index 6c6f82ad8d5c..4e7269b8bf1b 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -569,7 +569,6 @@ static irqreturn_t serial_omap_irq(int irq, void *dev_id)
{
struct uart_omap_port *up = dev_id;
unsigned int iir, lsr;
- unsigned int type;
irqreturn_t ret = IRQ_NONE;
int max_count = 256;
@@ -578,16 +577,15 @@ static irqreturn_t serial_omap_irq(int irq, void *dev_id)
do {
iir = serial_in(up, UART_IIR);
- if (iir & UART_IIR_NO_INT)
+ iir &= (UART_IIR_MASK | UART_IIR_EXT_MASK);
+ if (iir == UART_IIR_NO_INT)
break;
ret = IRQ_HANDLED;
lsr = serial_in(up, UART_LSR);
/* extract IRQ type from IIR register */
- type = iir & 0x3e;
-
- switch (type) {
+ switch (iir) {
case UART_IIR_MSI:
check_modem_status(up);
break;
@@ -607,10 +605,12 @@ static irqreturn_t serial_omap_irq(int irq, void *dev_id)
break;
case UART_IIR_XOFF:
/* FALLTHROUGH */
+ case UART_IIR_BUSY:
+ /* FALLTHROUGH */
default:
break;
}
- } while (!(iir & UART_IIR_NO_INT) && max_count--);
+ } while ((iir != UART_IIR_NO_INT) && max_count--);
spin_unlock(&up->port.lock);
diff --git a/drivers/tty/serial/pxa.c b/drivers/tty/serial/pxa.c
index 905631df1f8b..37cb17a11b34 100644
--- a/drivers/tty/serial/pxa.c
+++ b/drivers/tty/serial/pxa.c
@@ -253,7 +253,7 @@ static inline irqreturn_t serial_pxa_irq(int irq, void *dev_id)
unsigned int iir, lsr;
iir = serial_in(up, UART_IIR);
- if (iir & UART_IIR_NO_INT)
+ if ((iir & UART_IIR_MASK) == UART_IIR_NO_INT)
return IRQ_NONE;
spin_lock(&up->port.lock);
lsr = serial_in(up, UART_LSR);
diff --git a/drivers/tty/serial/serial-tegra.c b/drivers/tty/serial/serial-tegra.c
index d92a150c8733..4a084161d1d2 100644
--- a/drivers/tty/serial/serial-tegra.c
+++ b/drivers/tty/serial/serial-tegra.c
@@ -691,7 +691,8 @@ static irqreturn_t tegra_uart_isr(int irq, void *data)
spin_lock_irqsave(&u->lock, flags);
while (1) {
iir = tegra_uart_read(tup, UART_IIR);
- if (iir & UART_IIR_NO_INT) {
+ iir &= UART_IIR_MASK;
+ if (iir == UART_IIR_NO_INT) {
if (is_rx_int) {
tegra_uart_handle_rx_dma(tup);
if (tup->rx_in_progress) {
diff --git a/drivers/tty/serial/sunsu.c b/drivers/tty/serial/sunsu.c
index 72df2e1b88af..74e195d4d17e 100644
--- a/drivers/tty/serial/sunsu.c
+++ b/drivers/tty/serial/sunsu.c
@@ -475,7 +475,7 @@ static irqreturn_t sunsu_serial_interrupt(int irq, void *dev_id)
spin_lock_irqsave(&up->port.lock, flags);
- } while (!(serial_in(up, UART_IIR) & UART_IIR_NO_INT));
+ } while ((serial_in(up, UART_IIR) & UART_IIR_MASK) != UART_IIR_NO_INT);
spin_unlock_irqrestore(&up->port.lock, flags);
diff --git a/drivers/tty/serial/vr41xx_siu.c b/drivers/tty/serial/vr41xx_siu.c
index 439057e8107a..fc3b9bd920e9 100644
--- a/drivers/tty/serial/vr41xx_siu.c
+++ b/drivers/tty/serial/vr41xx_siu.c
@@ -429,7 +429,7 @@ static irqreturn_t siu_interrupt(int irq, void *dev_id)
port = (struct uart_port *)dev_id;
iir = siu_read(port, UART_IIR);
- if (iir & UART_IIR_NO_INT)
+ if ((iir & UART_IIR_MASK) == UART_IIR_NO_INT)
return IRQ_NONE;
lsr = siu_read(port, UART_LSR);
diff --git a/include/uapi/linux/serial_reg.h b/include/uapi/linux/serial_reg.h
index 5db76880b4ad..489522389a10 100644
--- a/include/uapi/linux/serial_reg.h
+++ b/include/uapi/linux/serial_reg.h
@@ -31,18 +31,18 @@
#define UART_IERX_SLEEP 0x10 /* Enable sleep mode */
#define UART_IIR 2 /* In: Interrupt ID Register */
-#define UART_IIR_NO_INT 0x01 /* No interrupts pending */
-#define UART_IIR_ID 0x0e /* Mask for the interrupt ID */
#define UART_IIR_MSI 0x00 /* Modem status interrupt */
+#define UART_IIR_NO_INT 0x01 /* No interrupts pending */
#define UART_IIR_THRI 0x02 /* Transmitter holding register empty */
#define UART_IIR_RDI 0x04 /* Receiver data interrupt */
#define UART_IIR_RLSI 0x06 /* Receiver line status interrupt */
-
#define UART_IIR_BUSY 0x07 /* DesignWare APB Busy Detect */
+#define UART_IIR_RX_TIMEOUT 0x0c /* DesignWare RX Timeout interrupt */
+#define UART_IIR_MASK 0x0f /* DesignWare IIR mask */
-#define UART_IIR_RX_TIMEOUT 0x0c /* OMAP RX Timeout interrupt */
#define UART_IIR_XOFF 0x10 /* OMAP XOFF/Special Character */
#define UART_IIR_CTS_RTS_DSR 0x20 /* OMAP CTS/RTS/DSR Change */
+#define UART_IIR_EXT_MASK 0x30 /* OMAP extended IIR mask */
#define UART_FCR 2 /* Out: FIFO Control Register */
#define UART_FCR_ENABLE_FIFO 0x01 /* Enable the FIFO */
--
2.11.0
^ permalink raw reply related
* Re: [PATCH] serial: 8250_dw: Minor code cleanup
From: Doug Anderson @ 2017-03-29 18:38 UTC (permalink / raw)
To: Olliver Schinagl
Cc: Greg Kroah-Hartman, Jiri Slaby, Kefeng Wang, Andy Shevchenko,
Heikki Krogerus, Jason Uy, Heiko Stuebner, Ed Blake,
linux-serial-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
dev-3kdeTeqwOZ9EV1b7eY7vFQ
In-Reply-To: <86fcc347-6b9b-5046-b9fb-63a2c81f0a20-dxLnbx3+1qmEVqv0pETR8A@public.gmane.org>
Hi,
On Wed, Mar 29, 2017 at 10:10 AM, Olliver Schinagl
<o.schinagl-U3FVU11NWA554TAoqtyWWQ@public.gmane.org> wrote:
> Hey Doug,
>
> On 29-03-17 17:50, Doug Anderson wrote:
>>
>> Hi,
>>
>> On Wed, Mar 29, 2017 at 3:04 AM, Olliver Schinagl <oliver-dxLnbx3+1qmEVqv0pETR8A@public.gmane.org>
>> wrote:
>>>
>>> Commit 424d79183af0 ("serial: 8250_dw: Avoid "too much work" from bogus
>>> rx timeout interrupt")
>>> added a bit check with quite a wide mask. To be concise with the other
>>> similar calls in this driver, change it to mask against the flag we want
>>> to
>>> check only. This thus removes a magic value/mask.
>>
>>
>> How certain are you that your patch is correct? You are now basically
>> checking to see if the bits "0xc" are set in the IIR. Previously the
>> patch ensured that the bits 0x33 were clear.
>
>
> You raise a good point. And after writing two replies that made perfect
> sense, I just realized looking at the table you wisely posted below, I
> should have spotted that the interrupts are not bits! So very good catch and
> my bad indeed.
>
>>
>> Have you tried looking through the kernel for other places where
>> UART_IIR_RX_TIMEOUT is referenced? In 8250_omap.c and 8250_port.c I
>> believe you'll find it masking against 0x3f. In omap-serial.c you'll
>> see a mask against 0x3e.
>>
>> Looking at the TRM for rk3399, I see that bits 4 and 5 (bitmask 0x30)
>> as "reserved". I see the following definitions for bits 3:0:
>>
>> 0000 = modem status
>> 0001 = no interrupt pending
>> 0010 = THR empty
>> 0100 = received data available
>> 0110 = receiver line status
>> 0111 = busy detect
>> 1100 = character timeout
>>
>> ...so while your patch will probably function OK, it would also
>> function equally well to simply test bit 3 (0x80) and ignore
>> everything else. ...but IMHO it is more correct to at least mask the
>> IIR with 0x0F and confirm that bits 2 and 3 are set and bits 0 and 1
>> are zero. ...and since the main 8250 code uses 0x3f, that seems even
>> better to me (despite the fact that it seems to be relying on the fact
>> that the "reserved" bits come back as 0).
>
>
> I strongly agree with you here, I did it wrong, but 0x3f really is wrong too
> imo. The bits to look at are 3:0, bits 4:5 are reserved and we should never
> look at those, as as you rightfully put it are being relied on to be 0
> (which may always be the case) but imo is still wrong and thus the mask
> should be 0x0f.
Personally I can't predict which would be better: 0xf or 0x3f.
Certainly someone wrote handle_rx_dma() purposely masking against 0x3f
rather than 0xf, probably because they were trying to handle:
#define UART_IIR_XOFF 0x10 /* OMAP XOFF/Special Character */
#define UART_IIR_CTS_RTS_DSR 0x20 /* OMAP CTS/RTS/DSR Change */
...so in our case we don't have those special OMAP IIR bits, so we
likely don't need 0x3f and 0x0f would work fine.
...but one could also argue this:
1. If anyone enables DMA on a dw_8250 then they'll run the code in
handle_rx_dma() which masks against 0x3f. ...so that implies that if
those reserved bits are even non-zero we're already broken (assuming
we use DMA).
2. It might be better to be consistent than to have two different values here.
Perhaps the absolute "cleanest" way would be to store a "interrupt ID
mask" somewhere and have it default to 0xf. ...then OMAP can override
it to 0x3f or 0x3e or whatever.
Anyway, this is the type of discussion that takes a lot of time to
talk about but it probably won't actually affect correctness much one
way or the other. If you want to change this to 0xf then I'm not
opposed to it, though I personally wouldn't bother until it becomes a
problem.
> Going to the horse's mouth [0] which is the documentation of the IP block
> used in all these designs, they also say the same thing. 4 bits and while I
> don't have any of the other datasheets of other 8250 cores, I bet they are
> the same?
Everyone has extended 8250 in their own special ways, so I'm not
convinced they'll all be the same.
>
> And then, the following is actually wrong on the same grounds, from the
> 8250_dw.c
>
>
> if ((iir & UART_IIR_BUSY) == UART_IIR_BUSY) {
>
> This just happens to work as it is the only way this can match, but clearly
> it is wrong then, right?
>
> I'll check against the same mask of 0x0f here as well.
On the 8250 datasheet I have it is clearly wrong. There are lots and
lots of 8250 devices out there and they all have their own "special"
IP, so presumably you'll want lots of testing if you touch this.
^ permalink raw reply
* [PATCHv2] serial: 8250_dw: Minor code cleanup
From: Olliver Schinagl @ 2017-03-29 18:37 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby
Cc: dev-3kdeTeqwOZ9EV1b7eY7vFQ, Kefeng Wang, Andy Shevchenko,
Heikki Krogerus, Jason Uy, Douglas Anderson, Heiko Stuebner,
Ed Blake, linux-serial-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Olliver Schinagl
Some very minor code cleanups, such as including the bitops header for
DW_UART_MCR_SIRE, use the BIT() macro as suggested by checkpatch and
removed a white space to match other invocations.
Signed-off-by: Olliver Schinagl <oliver-dxLnbx3+1qmEVqv0pETR8A@public.gmane.org>
---
Changes since v1:
Split up these non-code changing changes into a separate patch.
drivers/tty/serial/8250/8250_dw.c | 23 ++++++++++++-----------
1 file changed, 12 insertions(+), 11 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index e65808c482f1..56aded887771 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -13,6 +13,7 @@
* LCR is written whilst busy. If it is, then a busy detect interrupt is
* raised, the LCR needs to be rewritten and the uart status register read.
*/
+#include <linux/bitops.h>
#include <linux/device.h>
#include <linux/io.h>
#include <linux/module.h>
@@ -39,16 +40,16 @@
/* Component Parameter Register bits */
#define DW_UART_CPR_ABP_DATA_WIDTH (3 << 0)
-#define DW_UART_CPR_AFCE_MODE (1 << 4)
-#define DW_UART_CPR_THRE_MODE (1 << 5)
-#define DW_UART_CPR_SIR_MODE (1 << 6)
-#define DW_UART_CPR_SIR_LP_MODE (1 << 7)
-#define DW_UART_CPR_ADDITIONAL_FEATURES (1 << 8)
-#define DW_UART_CPR_FIFO_ACCESS (1 << 9)
-#define DW_UART_CPR_FIFO_STAT (1 << 10)
-#define DW_UART_CPR_SHADOW (1 << 11)
-#define DW_UART_CPR_ENCODED_PARMS (1 << 12)
-#define DW_UART_CPR_DMA_EXTRA (1 << 13)
+#define DW_UART_CPR_AFCE_MODE BIT(4)
+#define DW_UART_CPR_THRE_MODE BIT(5)
+#define DW_UART_CPR_SIR_MODE BIT(6)
+#define DW_UART_CPR_SIR_LP_MODE BIT(7)
+#define DW_UART_CPR_ADDITIONAL_FEATURES BIT(8)
+#define DW_UART_CPR_FIFO_ACCESS BIT(9)
+#define DW_UART_CPR_FIFO_STAT BIT(10)
+#define DW_UART_CPR_SHADOW BIT(11)
+#define DW_UART_CPR_ENCODED_PARMS BIT(12)
+#define DW_UART_CPR_DMA_EXTRA BIT(13)
#define DW_UART_CPR_FIFO_MODE (0xff << 16)
/* Helper for fifo size calculation */
#define DW_UART_CPR_FIFO_SIZE(a) (((a >> 16) & 0xff) * 16)
@@ -222,7 +223,7 @@ static int dw8250_handle_irq(struct uart_port *p)
status = p->serial_in(p, UART_LSR);
if (!(status & (UART_LSR_DR | UART_LSR_BI)))
- (void) p->serial_in(p, UART_RX);
+ (void)p->serial_in(p, UART_RX);
spin_unlock_irqrestore(&p->lock, flags);
}
--
2.11.0
^ permalink raw reply related
* Re: [PATCH] serial: 8250_dw: Minor code cleanup
From: Olliver Schinagl @ 2017-03-29 17:20 UTC (permalink / raw)
To: Doug Anderson
Cc: Greg Kroah-Hartman, Jiri Slaby, Kefeng Wang, Andy Shevchenko,
Heikki Krogerus, Jason Uy, Heiko Stuebner, Ed Blake,
linux-serial-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
dev-3kdeTeqwOZ9EV1b7eY7vFQ
In-Reply-To: <86fcc347-6b9b-5046-b9fb-63a2c81f0a20-dxLnbx3+1qmEVqv0pETR8A@public.gmane.org>
Hey Doug,
On 29-03-17 19:10, Olliver Schinagl wrote:
> Hey Doug,
>
> On 29-03-17 17:50, Doug Anderson wrote:
>> Hi,
>>
>> On Wed, Mar 29, 2017 at 3:04 AM, Olliver Schinagl <oliver-dxLnbx3+1qmEVqv0pETR8A@public.gmane.org>
>> wrote:
>>> Commit 424d79183af0 ("serial: 8250_dw: Avoid "too much work" from
>>> bogus rx timeout interrupt")
>>> added a bit check with quite a wide mask. To be concise with the other
>>> similar calls in this driver, change it to mask against the flag we
>>> want to
>>> check only. This thus removes a magic value/mask.
>>
>> How certain are you that your patch is correct? You are now basically
>> checking to see if the bits "0xc" are set in the IIR. Previously the
>> patch ensured that the bits 0x33 were clear.
>
> You raise a good point. And after writing two replies that made perfect
> sense, I just realized looking at the table you wisely posted below, I
> should have spotted that the interrupts are not bits! So very good catch
> and my bad indeed.
>
>>
>> Have you tried looking through the kernel for other places where
>> UART_IIR_RX_TIMEOUT is referenced? In 8250_omap.c and 8250_port.c I
>> believe you'll find it masking against 0x3f. In omap-serial.c you'll
>> see a mask against 0x3e.
I just checked the 8250_omap case and the reason they mask against 0x3f
is because they have 2 extra bits in the iir, e.g. the 'reserved' bits
are not reserved there but used. So either 8250_omap is not based on the
same designware IP or they added it ontop of the DW ip.
And thus, would the define be UART_IIR_MASK or UART_IIR_DW_MASK and
UART_IIR_OMAP_MASK? Or do we, as mentioned earlier, rely on the fact
that it is supposed to be reserved but always read as 0?
The 0x3e case seems to be where the busy flag is to be always ignored,
so UART_IIR_NOBUSY_MASK? But also kind of ugly, as this is not a
bitfield irq! so it should be changed there to read:
UART_IIR_BUSY: /* fallthrough */
default:
break;
right?
>>
>> Looking at the TRM for rk3399, I see that bits 4 and 5 (bitmask 0x30)
>> as "reserved". I see the following definitions for bits 3:0:
>>
>> 0000 = modem status
>> 0001 = no interrupt pending
>> 0010 = THR empty
>> 0100 = received data available
>> 0110 = receiver line status
>> 0111 = busy detect
>> 1100 = character timeout
>>
>> ...so while your patch will probably function OK, it would also
>> function equally well to simply test bit 3 (0x80) and ignore
>> everything else. ...but IMHO it is more correct to at least mask the
>> IIR with 0x0F and confirm that bits 2 and 3 are set and bits 0 and 1
>> are zero. ...and since the main 8250 code uses 0x3f, that seems even
>> better to me (despite the fact that it seems to be relying on the fact
>> that the "reserved" bits come back as 0).
>
> I strongly agree with you here, I did it wrong, but 0x3f really is wrong
> too imo. The bits to look at are 3:0, bits 4:5 are reserved and we
> should never look at those, as as you rightfully put it are being relied
> on to be 0 (which may always be the case) but imo is still wrong and
> thus the mask should be 0x0f.
>
> Going to the horse's mouth [0] which is the documentation of the IP
> block used in all these designs, they also say the same thing. 4 bits
> and while I don't have any of the other datasheets of other 8250 cores,
> I bet they are the same?
>
> And then, the following is actually wrong on the same grounds, from the
> 8250_dw.c
>
>
> if ((iir & UART_IIR_BUSY) == UART_IIR_BUSY) {
>
> This just happens to work as it is the only way this can match, but
> clearly it is wrong then, right?
>
> I'll check against the same mask of 0x0f here as well.
>>
>>
>> If you want to make a fix, I'd suggest adding a #define for 0x3f and
>> using it in various places.
> Yeah I'll do that, add the define next to the others for 0x0f, unless
> someone says it is a good idea (or needed idea) to mask against the
> reserved bits as well.
>
>>
>>
>>> Some very minor code cleanups, such as including the bitops header for
>>> DW_UART_MCR_SIRE, use the BIT() macro as suggested by checkpatc and
>>> removed a whitespace to match other invocations.
>>
>> Maybe it's just me, but it seems like a bad idea to combine these
>> cleanups in the same patch with a functional change..
> Well with the oversight of the bit check above, it becomes obvious that
> it is a bigger change indeed. I will change it!
>
> Olliver
>
>>
>
> [0] http://linux-sunxi.org/images/d/d2/Dw_apb_uart_db.pdf
^ permalink raw reply
* Re: [PATCH] serial: 8250_dw: Minor code cleanup
From: Olliver Schinagl @ 2017-03-29 17:10 UTC (permalink / raw)
To: Doug Anderson
Cc: Greg Kroah-Hartman, Jiri Slaby, Kefeng Wang, Andy Shevchenko,
Heikki Krogerus, Jason Uy, Heiko Stuebner, Ed Blake,
linux-serial-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
dev-3kdeTeqwOZ9EV1b7eY7vFQ
In-Reply-To: <CAD=FV=XZFuZ-Zqv75wF7wj6AY35o9n4-zrbsVjNcFZ+DxS=fbQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
Hey Doug,
On 29-03-17 17:50, Doug Anderson wrote:
> Hi,
>
> On Wed, Mar 29, 2017 at 3:04 AM, Olliver Schinagl <oliver-dxLnbx3+1qmEVqv0pETR8A@public.gmane.org> wrote:
>> Commit 424d79183af0 ("serial: 8250_dw: Avoid "too much work" from bogus rx timeout interrupt")
>> added a bit check with quite a wide mask. To be concise with the other
>> similar calls in this driver, change it to mask against the flag we want to
>> check only. This thus removes a magic value/mask.
>
> How certain are you that your patch is correct? You are now basically
> checking to see if the bits "0xc" are set in the IIR. Previously the
> patch ensured that the bits 0x33 were clear.
You raise a good point. And after writing two replies that made perfect
sense, I just realized looking at the table you wisely posted below, I
should have spotted that the interrupts are not bits! So very good catch
and my bad indeed.
>
> Have you tried looking through the kernel for other places where
> UART_IIR_RX_TIMEOUT is referenced? In 8250_omap.c and 8250_port.c I
> believe you'll find it masking against 0x3f. In omap-serial.c you'll
> see a mask against 0x3e.
>
> Looking at the TRM for rk3399, I see that bits 4 and 5 (bitmask 0x30)
> as "reserved". I see the following definitions for bits 3:0:
>
> 0000 = modem status
> 0001 = no interrupt pending
> 0010 = THR empty
> 0100 = received data available
> 0110 = receiver line status
> 0111 = busy detect
> 1100 = character timeout
>
> ...so while your patch will probably function OK, it would also
> function equally well to simply test bit 3 (0x80) and ignore
> everything else. ...but IMHO it is more correct to at least mask the
> IIR with 0x0F and confirm that bits 2 and 3 are set and bits 0 and 1
> are zero. ...and since the main 8250 code uses 0x3f, that seems even
> better to me (despite the fact that it seems to be relying on the fact
> that the "reserved" bits come back as 0).
I strongly agree with you here, I did it wrong, but 0x3f really is wrong
too imo. The bits to look at are 3:0, bits 4:5 are reserved and we
should never look at those, as as you rightfully put it are being relied
on to be 0 (which may always be the case) but imo is still wrong and
thus the mask should be 0x0f.
Going to the horse's mouth [0] which is the documentation of the IP
block used in all these designs, they also say the same thing. 4 bits
and while I don't have any of the other datasheets of other 8250 cores,
I bet they are the same?
And then, the following is actually wrong on the same grounds, from the
8250_dw.c
if ((iir & UART_IIR_BUSY) == UART_IIR_BUSY) {
This just happens to work as it is the only way this can match, but
clearly it is wrong then, right?
I'll check against the same mask of 0x0f here as well.
>
>
> If you want to make a fix, I'd suggest adding a #define for 0x3f and
> using it in various places.
Yeah I'll do that, add the define next to the others for 0x0f, unless
someone says it is a good idea (or needed idea) to mask against the
reserved bits as well.
>
>
>> Some very minor code cleanups, such as including the bitops header for
>> DW_UART_MCR_SIRE, use the BIT() macro as suggested by checkpatc and
>> removed a whitespace to match other invocations.
>
> Maybe it's just me, but it seems like a bad idea to combine these
> cleanups in the same patch with a functional change..
Well with the oversight of the bit check above, it becomes obvious that
it is a bigger change indeed. I will change it!
Olliver
>
[0] http://linux-sunxi.org/images/d/d2/Dw_apb_uart_db.pdf
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox