* Re: [PATCH] serial: omap: fix the overrun case
From: Poddar, Sourav @ 2012-09-21 11:18 UTC (permalink / raw)
To: Shubhrajyoti D; +Cc: linux-serial, linux-omap, linux-arm-kernel
In-Reply-To: <1348222976-7241-1-git-send-email-shubhrajyoti@ti.com>
Hi,
On Fri, Sep 21, 2012 at 3:52 PM, Shubhrajyoti D <shubhrajyoti@ti.com> wrote:
> Overrun also causes an internal flag to be set, which disables further
> reception. Before the next frame can
> be received, the MPU must:
> • Reset the RX FIFO.
> • clear the internal flag.
>
> In the uart mode a dummy read is needed. Add the same.
>
> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
> ---
> - functional testing on omap4sdp
> - Verified idle and suspend path hits off on beagle.
>
> drivers/tty/serial/omap-serial.c | 7 +++++++
> 1 files changed, 7 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
> index a0d4460..bc22a2b 100644
> --- a/drivers/tty/serial/omap-serial.c
> +++ b/drivers/tty/serial/omap-serial.c
> @@ -334,6 +334,13 @@ static unsigned int check_modem_status(struct uart_omap_port *up)
> static void serial_omap_rlsi(struct uart_omap_port *up, unsigned int lsr)
> {
> unsigned int flag;
> + unsigned char ch = 0;
> +
> + if (!(lsr & UART_LSR_BRK_ERROR_BITS))
By using this flag, you are trying to take into account not just the
overrun case but also
frame, parity and break condition case as the flag is the OR of all these.
I suppose the commit log should reflect this.
> + return;
> +
> + if (likely(lsr & UART_LSR_DR))
> + ch = serial_in(up, UART_RX);
>
> up->port.icount.rx++;
> flag = TTY_NORMAL;
> --
> 1.7.5.4
>
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] serial: omap: fix the overrun case
From: Felipe Balbi @ 2012-09-21 11:35 UTC (permalink / raw)
To: Shubhrajyoti
Cc: balbi, linux-serial, linux-omap, linux-arm-kernel, sourav.poddar
In-Reply-To: <505C4CA4.6020507@ti.com>
[-- Attachment #1: Type: text/plain, Size: 1123 bytes --]
On Fri, Sep 21, 2012 at 04:46:52PM +0530, Shubhrajyoti wrote:
> On Friday 21 September 2012 04:30 PM, Felipe Balbi wrote:
> > On Fri, Sep 21, 2012 at 03:52:56PM +0530, Shubhrajyoti D wrote:
> >> Overrun also causes an internal flag to be set, which disables further
> >> reception. Before the next frame can
> >> be received, the MPU must:
> >> • Reset the RX FIFO.
> >> • clear the internal flag.
> >>
> >> In the uart mode a dummy read is needed. Add the same.
> > Very nice patch but I think commit log can be a bit more verbose.
> ok
> > Please make the problem a little clearer. Why do we even get that
> > interrupt fired if BRK_ERROR_BITS aren't set ?
> I did not get this point.
>
> it it is ! BRK_ERROR_BITS I return.
That's what I mean. rlsi handler is basically taking care of those
bits... So how come we get RLSI IRQ when those bits aren't set ?
Meaning, you shouldn't ever need that check, right ? Ideally, whenever
that handler is called, it's because BRK_ERROR_BITS are set.
Maybe add a WARN_ONCE() kinda thing just to see if that will ever really
happen ??
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH] serial: omap: fix the overrun case
From: Poddar, Sourav @ 2012-09-21 11:39 UTC (permalink / raw)
To: balbi; +Cc: Shubhrajyoti D, linux-serial, linux-omap, linux-arm-kernel
In-Reply-To: <20120921110050.GC16003@arwen.pp.htv.fi>
Hi Felipe,
On Fri, Sep 21, 2012 at 4:30 PM, Felipe Balbi <balbi@ti.com> wrote:
> On Fri, Sep 21, 2012 at 03:52:56PM +0530, Shubhrajyoti D wrote:
>> Overrun also causes an internal flag to be set, which disables further
>> reception. Before the next frame can
>> be received, the MPU must:
>> • Reset the RX FIFO.
>> • clear the internal flag.
>>
>> In the uart mode a dummy read is needed. Add the same.
>
> Very nice patch but I think commit log can be a bit more verbose.
>
> Please make the problem a little clearer. Why do we even get that
> interrupt fired if BRK_ERROR_BITS aren't set ?
>
According to LSR registers, there are few other bits like RX_FIFO_E( atleast 1
character in RX_FIFO or TX FIFO Empty), which might be the cause of an
interrupt. ?
>> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
>> ---
>> - functional testing on omap4sdp
>> - Verified idle and suspend path hits off on beagle.
>>
>> drivers/tty/serial/omap-serial.c | 7 +++++++
>> 1 files changed, 7 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
>> index a0d4460..bc22a2b 100644
>> --- a/drivers/tty/serial/omap-serial.c
>> +++ b/drivers/tty/serial/omap-serial.c
>> @@ -334,6 +334,13 @@ static unsigned int check_modem_status(struct uart_omap_port *up)
>> static void serial_omap_rlsi(struct uart_omap_port *up, unsigned int lsr)
>> {
>> unsigned int flag;
>> + unsigned char ch = 0;
>> +
>> + if (!(lsr & UART_LSR_BRK_ERROR_BITS))
>> + return;
>> +
>> + if (likely(lsr & UART_LSR_DR))
>> + ch = serial_in(up, UART_RX);
>
> Maybe add a comment before this condition stating why this character
> read is necessary ?
>
> --
> balbi
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] serial: omap: fix the overrun case
From: Shubhrajyoti Datta @ 2012-09-21 11:48 UTC (permalink / raw)
To: balbi
Cc: Shubhrajyoti, linux-serial, linux-omap, linux-arm-kernel,
sourav.poddar
In-Reply-To: <20120921113506.GF16003@arwen.pp.htv.fi>
On Fri, Sep 21, 2012 at 5:05 PM, Felipe Balbi <balbi@ti.com> wrote:
> On Fri, Sep 21, 2012 at 04:46:52PM +0530, Shubhrajyoti wrote:
>> On Friday 21 September 2012 04:30 PM, Felipe Balbi wrote:
>> > On Fri, Sep 21, 2012 at 03:52:56PM +0530, Shubhrajyoti D wrote:
[...]
>> it it is ! BRK_ERROR_BITS I return.
>
> That's what I mean. rlsi handler is basically taking care of those
> bits... So how come we get RLSI IRQ when those bits aren't set ?
>
> Meaning, you shouldn't ever need that check, right ? Ideally, whenever
> that handler is called, it's because BRK_ERROR_BITS are set.
>
> Maybe add a WARN_ONCE() kinda thing just to see if that will ever really
> happen ??
hmm yes. will get back.
>
> --
> balbi
^ permalink raw reply
* Re: [PATCH] serial: omap: fix the overrun case
From: Felipe Balbi @ 2012-09-21 12:07 UTC (permalink / raw)
To: Poddar, Sourav
Cc: balbi, Shubhrajyoti D, linux-serial, linux-omap, linux-arm-kernel
In-Reply-To: <CAKdam55VG8KTRZNvPgur9M-wkbD7wyJt27Jsvks=r4PNiuEGaQ@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1072 bytes --]
On Fri, Sep 21, 2012 at 05:09:56PM +0530, Poddar, Sourav wrote:
> Hi Felipe,
>
> On Fri, Sep 21, 2012 at 4:30 PM, Felipe Balbi <balbi@ti.com> wrote:
> > On Fri, Sep 21, 2012 at 03:52:56PM +0530, Shubhrajyoti D wrote:
> >> Overrun also causes an internal flag to be set, which disables further
> >> reception. Before the next frame can
> >> be received, the MPU must:
> >> • Reset the RX FIFO.
> >> • clear the internal flag.
> >>
> >> In the uart mode a dummy read is needed. Add the same.
> >
> > Very nice patch but I think commit log can be a bit more verbose.
> >
> > Please make the problem a little clearer. Why do we even get that
> > interrupt fired if BRK_ERROR_BITS aren't set ?
> >
> According to LSR registers, there are few other bits like RX_FIFO_E( atleast 1
> character in RX_FIFO or TX FIFO Empty), which might be the cause of an
> interrupt. ?
right. In that case, is it really correct to just return if
BRK_ERROR_BITS aren't set ? IRQ line will not toggle and we just might
end up with an IRQ storm, right ?
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 3/3] serial: pl011: allow very high baudrates
From: Linus Walleij @ 2012-09-21 13:41 UTC (permalink / raw)
To: Russell King - ARM Linux, Greg Kroah-Hartman, Alan Cox
Cc: Linus Walleij, linux-serial, Guillaume Jaunet,
Par-Gunnar Hjalmdahl, Anmar Oueja, Matthias Locher,
Rajanikanth H.V, Christophe Arnal, linux-arm-kernel
In-Reply-To: <20120920190034.GB15609@n2100.arm.linux.org.uk>
On Thu, Sep 20, 2012 at 9:00 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Thu, Sep 20, 2012 at 11:46:08AM +0200, Linus Walleij wrote:
>> From: Linus Walleij <linus.walleij@linaro.org>
>>
>> The ST Microelectronics variant of the PL011 is capable of supporting
>> very high non-standard baud rates, even above 4 Mbps. However the
>> uart_get_baud_rate() will not allow us to set these, so override that
>> calculation on very high speeds.
>
> You don't explain why it doesn't. It should in theory allow you to,
> because there's no limits within that function other than those which
> you pass in as the minimum and maximum.
So I looked closer: uart_get_baud_rate() in turn calls out to
tty_termios_baud_rate() which does some fuzzy matching to the
fix baudrate table and simply "snaps" the baudrate to the closest
match if it is "close enough".
I.e. 4050000 baud is close enough to 4000000 so it will round
off and "snap" to 4000000, whereas 3250000 baud will not
"close enough" to 3000000 so it will not "snap" to 3000000.
So we don't like that 4050000 snaps to 4000000.
I wonder how to fix this ... I'm planning to do what the comment
in tty_ioctl says:
/**
* tty_termios_baud_rate
* @termios: termios structure
*
* Convert termios baud rate data into a speed. This should be called
* with the termios lock held if this termios is a terminal termios
* structure. May change the termios data. Device drivers can call this
* function but should use ->c_[io]speed directly as they are updated.
*
* Locking: none
*/
Device drivers should use c_[io]speed directly!
Alan Cox wrote this, so Alan: should I just ditch the use of
uart_get_baud_rate() and program the divider directly from
c_[io]speed?
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH] serial: omap: fix the overrun case
From: Kevin Hilman @ 2012-09-21 14:18 UTC (permalink / raw)
To: Shubhrajyoti D; +Cc: linux-serial, linux-omap, linux-arm-kernel, sourav.poddar
In-Reply-To: <1348222976-7241-1-git-send-email-shubhrajyoti@ti.com>
Shubhrajyoti D <shubhrajyoti@ti.com> writes:
> Overrun also causes an internal flag to be set, which disables further
> reception. Before the next frame can
> be received, the MPU must:
> • Reset the RX FIFO.
> • clear the internal flag.
>
> In the uart mode a dummy read is needed. Add the same.
>
> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
> ---
> - functional testing on omap4sdp
> - Verified idle and suspend path hits off on beagle.
Tested-by: Kevin Hilman <khilman@ti.com>
This fixes the console hang I was seeing on 3530/Overo.
Thanks,
Kevin
> drivers/tty/serial/omap-serial.c | 7 +++++++
> 1 files changed, 7 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
> index a0d4460..bc22a2b 100644
> --- a/drivers/tty/serial/omap-serial.c
> +++ b/drivers/tty/serial/omap-serial.c
> @@ -334,6 +334,13 @@ static unsigned int check_modem_status(struct uart_omap_port *up)
> static void serial_omap_rlsi(struct uart_omap_port *up, unsigned int lsr)
> {
> unsigned int flag;
> + unsigned char ch = 0;
> +
> + if (!(lsr & UART_LSR_BRK_ERROR_BITS))
> + return;
> +
> + if (likely(lsr & UART_LSR_DR))
> + ch = serial_in(up, UART_RX);
>
> up->port.icount.rx++;
> flag = TTY_NORMAL;
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 3/3] serial: pl011: allow very high baudrates
From: Alan Cox @ 2012-09-21 14:37 UTC (permalink / raw)
To: Linus Walleij
Cc: Russell King - ARM Linux, Greg Kroah-Hartman, Linus Walleij,
linux-serial, Guillaume Jaunet, Par-Gunnar Hjalmdahl, Anmar Oueja,
Matthias Locher, Rajanikanth H.V, Christophe Arnal,
linux-arm-kernel
In-Reply-To: <CACRpkdY1ZxQoyGFO9OnKr0WYQzooxPGvZV8Gnr_YO_yT_+ddDw@mail.gmail.com>
> Device drivers should use c_[io]speed directly!
>
> Alan Cox wrote this, so Alan: should I just ditch the use of
> uart_get_baud_rate() and program the divider directly from
> c_[io]speed?
Yes.
The functions are designed to act as helpers for old devices. In fact
we can actually probably abolish tty_termios_baud_rate at this point as
I don't think there is much if anything left which blows up fed a non
Bxxx table entry.
I will have a look at that in fact see what it involves at this point.
Alan
^ permalink raw reply
* [PATCH v2] serial: omap: fix the reciever line error case
From: Shubhrajyoti D @ 2012-09-21 14:37 UTC (permalink / raw)
To: linux-serial; +Cc: linux-omap, linux-arm-kernel, Shubhrajyoti D
This patch does the following
- In case of errors if there least one data character in the RX FIFO
read it otherwise it may stall the receiver.
This is recommended in the interrupt reset method in the table 23-246 of
the omap4 TRM.
Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
---
- Tested on omap4sdp.
- pm tested hitting off in idle and suspend.
- v2 changes update the changelogs.
- Also remove the dummy check as FE , PE , BI and OE are the only
receiver errors.
drivers/tty/serial/omap-serial.c | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index a0d4460..4fea16b 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -334,6 +334,10 @@ static unsigned int check_modem_status(struct uart_omap_port *up)
static void serial_omap_rlsi(struct uart_omap_port *up, unsigned int lsr)
{
unsigned int flag;
+ unsigned char ch = 0;
+
+ if (likely(lsr & UART_LSR_DR))
+ ch = serial_in(up, UART_RX);
up->port.icount.rx++;
flag = TTY_NORMAL;
--
1.7.5.4
^ permalink raw reply related
* Re: [PATCH 3/3] serial: pl011: allow very high baudrates
From: Russell King - ARM Linux @ 2012-09-21 14:58 UTC (permalink / raw)
To: Alan Cox
Cc: Linus Walleij, Greg Kroah-Hartman, Linus Walleij, linux-serial,
Guillaume Jaunet, Par-Gunnar Hjalmdahl, Anmar Oueja,
Matthias Locher, Rajanikanth H.V, Christophe Arnal,
linux-arm-kernel
In-Reply-To: <20120921153710.220adb92@bob.linux.org.uk>
On Fri, Sep 21, 2012 at 03:37:10PM +0100, Alan Cox wrote:
> > Device drivers should use c_[io]speed directly!
> >
> > Alan Cox wrote this, so Alan: should I just ditch the use of
> > uart_get_baud_rate() and program the divider directly from
> > c_[io]speed?
>
> Yes.
>
> The functions are designed to act as helpers for old devices. In fact
> we can actually probably abolish tty_termios_baud_rate at this point as
> I don't think there is much if anything left which blows up fed a non
> Bxxx table entry.
>
> I will have a look at that in fact see what it involves at this point.
Alan - the only issue that remains is handling the invalid baud rate
situation - if left to individual drivers to do this, we will see them
doing stuff (as was the case with this very patch - and was the case
prior to serial_core) such as using dev_err() to print an error and
merely returning from their set_termios function, or clamping to some
speed and not feeding back to userspace what they're actually doing.
^ permalink raw reply
* Re: [PATCH 3/3] serial: pl011: allow very high baudrates
From: Alan Cox @ 2012-09-21 15:05 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Alan Cox, Linus Walleij, Greg Kroah-Hartman, Linus Walleij,
linux-serial, Guillaume Jaunet, Par-Gunnar Hjalmdahl, Anmar Oueja,
Matthias Locher, Rajanikanth H.V, Christophe Arnal,
linux-arm-kernel
In-Reply-To: <20120921145803.GF15609@n2100.arm.linux.org.uk>
> Alan - the only issue that remains is handling the invalid baud rate
> situation - if left to individual drivers to do this, we will see them
> doing stuff (as was the case with this very patch - and was the case
> prior to serial_core) such as using dev_err() to print an error and
> merely returning from their set_termios function, or clamping to some
> speed and not feeding back to userspace what they're actually doing.
They've had years to adapt. It's time they got with the program or just
broke horribly.
Alan
^ permalink raw reply
* Re: [PATCH 3/3] serial: pl011: allow very high baudrates
From: Russell King - ARM Linux @ 2012-09-21 15:06 UTC (permalink / raw)
To: Alan Cox
Cc: Alan Cox, Linus Walleij, Greg Kroah-Hartman, Linus Walleij,
linux-serial, Guillaume Jaunet, Par-Gunnar Hjalmdahl, Anmar Oueja,
Matthias Locher, Rajanikanth H.V, Christophe Arnal,
linux-arm-kernel
In-Reply-To: <20120921160503.53736fee@pyramind.ukuu.org.uk>
On Fri, Sep 21, 2012 at 04:05:03PM +0100, Alan Cox wrote:
> > Alan - the only issue that remains is handling the invalid baud rate
> > situation - if left to individual drivers to do this, we will see them
> > doing stuff (as was the case with this very patch - and was the case
> > prior to serial_core) such as using dev_err() to print an error and
> > merely returning from their set_termios function, or clamping to some
> > speed and not feeding back to userspace what they're actually doing.
>
> They've had years to adapt. It's time they got with the program or just
> broke horribly.
Sorry Alan, I don't think you really understand what I'm saying.
In the old days, lots of serial drivers just ignored termios settings
that they didn't support, or baud rates that they didn't know about -
so when userspace requests, eg 4Mbps on a UART port, the ioctl didn't
fail, and getting the termios back reported 4Mbps.
That is in violation of the POSIX general terminal interface standard,
and is something that I fixed with the uart baud rate functions - which
most serial_core using drivers are using.
Now you're pushing that logic back down into drivers, where driver authors
have to understand these details again, and that's precisely the reverse
of what should be happening. We should be helping driver authors get
things right by providing helpers to do that, which is exactly what has
been done.
We need helper functions so we don't end up with 10s of buggy
implementations of the same calculation of baud rate quotient, which
each do their own range checking, and then go on to set some random
unreported-back-to-userspace baud rate.
^ permalink raw reply
* Re: [PATCH] serial: omap: fix the overrun case
From: Shubhrajyoti Datta @ 2012-09-21 15:08 UTC (permalink / raw)
To: Kevin Hilman
Cc: Shubhrajyoti D, linux-serial, linux-omap, linux-arm-kernel,
sourav.poddar
In-Reply-To: <87a9wjfohd.fsf@deeprootsystems.com>
On Fri, Sep 21, 2012 at 7:48 PM, Kevin Hilman
<khilman@deeprootsystems.com> wrote:
> Shubhrajyoti D <shubhrajyoti@ti.com> writes:
>
[...]
>>
>> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
>> ---
>> - functional testing on omap4sdp
>> - Verified idle and suspend path hits off on beagle.
>
> Tested-by: Kevin Hilman <khilman@ti.com>
>
> This fixes the console hang I was seeing on 3530/Overo.
Thanks for the test.
Could you test the v2
http://www.spinics.net/lists/arm-kernel/msg197050.html
I have removed the redundant check.
Thanks,
>
> Thanks,
>
> Kevin
>
>> drivers/tty/serial/omap-serial.c | 7 +++++++
>> 1 files changed, 7 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
>> index a0d4460..bc22a2b 100644
>> --- a/drivers/tty/serial/omap-serial.c
>> +++ b/drivers/tty/serial/omap-serial.c
>> @@ -334,6 +334,13 @@ static unsigned int check_modem_status(struct uart_omap_port *up)
>> static void serial_omap_rlsi(struct uart_omap_port *up, unsigned int lsr)
>> {
>> unsigned int flag;
>> + unsigned char ch = 0;
>> +
>> + if (!(lsr & UART_LSR_BRK_ERROR_BITS))
>> + return;
>> +
>> + if (likely(lsr & UART_LSR_DR))
>> + ch = serial_in(up, UART_RX);
>>
>> up->port.icount.rx++;
>> flag = TTY_NORMAL;
> --
> To unsubscribe from this list: send the line "unsubscribe linux-serial" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 3/3] serial: pl011: allow very high baudrates
From: Alan Cox @ 2012-09-21 15:14 UTC (permalink / raw)
To: Linus Walleij
Cc: Russell King - ARM Linux, Linus Walleij, Greg Kroah-Hartman,
Christophe Arnal, Guillaume Jaunet, Par-Gunnar Hjalmdahl,
Anmar Oueja, Matthias Locher, linux-arm-kernel, linux-serial,
Rajanikanth H.V, Alan Cox
In-Reply-To: <CACRpkdY1ZxQoyGFO9OnKr0WYQzooxPGvZV8Gnr_YO_yT_+ddDw@mail.gmail.com>
> So I looked closer: uart_get_baud_rate() in turn calls out to
> tty_termios_baud_rate() which does some fuzzy matching to the
> fix baudrate table and simply "snaps" the baudrate to the closest
> match if it is "close enough".
The fuzzing is getting done when re-encoding and because the code
encodes/re-decodes stuff.
> I wonder how to fix this ... I'm planning to do what the comment
> in tty_ioctl says:
If you support the ancient alt speed hacks then its not quite so neat.
Alan
^ permalink raw reply
* Re: [PATCH 3/3] serial: pl011: allow very high baudrates
From: Alan Cox @ 2012-09-21 15:25 UTC (permalink / raw)
To: Linus Walleij
Cc: Russell King - ARM Linux, Greg Kroah-Hartman, Alan Cox,
Linus Walleij, linux-serial, Guillaume Jaunet,
Par-Gunnar Hjalmdahl, Anmar Oueja, Matthias Locher,
Rajanikanth H.V, Christophe Arnal, linux-arm-kernel
In-Reply-To: <CACRpkdY1ZxQoyGFO9OnKr0WYQzooxPGvZV8Gnr_YO_yT_+ddDw@mail.gmail.com>
Untested but I suspect the following may help
commit 44590a8cc4ee6e40737b843d1d9593345ef3bf04
Author: Alan Cox <alan@linux.intel.com>
Date: Fri Sep 21 16:36:36 2012 +0100
serial: avoid iteratively re-encoding the termios
If we do that then we will corrupt the termios data in some
situations and thus not nicely encode the baud rates as we should.
Signed-off-by: Alan Cox <alan@linux.intel.com>
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 78036c5..73a169e 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -348,8 +348,9 @@ uart_get_baud_rate(struct uart_port *port, struct ktermios *termios,
else if (flags == UPF_SPD_WARP)
altbaud = 460800;
+ baud = tty_termios_baud_rate(termios);
+
for (try = 0; try < 2; try++) {
- baud = tty_termios_baud_rate(termios);
/*
* The spd_hi, spd_vhi, spd_shi, spd_warp kludge...
@@ -359,26 +360,25 @@ uart_get_baud_rate(struct uart_port *port, struct ktermios *termios,
baud = altbaud;
/*
- * Special case: B0 rate.
+ * Special case: B0 rate. Note: this breaks if the
+ * device cannot support 9600 baud
*/
if (baud == 0) {
hung_up = 1;
baud = 9600;
}
- if (baud >= min && baud <= max)
+ if (baud >= min && baud <= max) {
+ tty_termios_encode_baud_rate(tty, baud, baud);
return baud;
+ }
/*
* Oops, the quotient was zero. Try again with
* the old baud rate if possible.
*/
- termios->c_cflag &= ~CBAUD;
if (old) {
baud = tty_termios_baud_rate(old);
- if (!hung_up)
- tty_termios_encode_baud_rate(termios,
- baud, baud);
old = NULL;
continue;
}
@@ -389,11 +389,9 @@ uart_get_baud_rate(struct uart_port *port, struct ktermios *termios,
*/
if (!hung_up) {
if (baud <= min)
- tty_termios_encode_baud_rate(termios,
- min + 1, min + 1);
+ baud = min + 1;
else
- tty_termios_encode_baud_rate(termios,
- max - 1, max - 1);
+ baud = max - 1;
}
}
/* Should never happen */
^ permalink raw reply related
* Re: [PATCH 3/3] serial: pl011: allow very high baudrates
From: Alan Cox @ 2012-09-21 15:36 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Alan Cox, Linus Walleij, Greg Kroah-Hartman, Linus Walleij,
linux-serial, Guillaume Jaunet, Par-Gunnar Hjalmdahl, Anmar Oueja,
Matthias Locher, Rajanikanth H.V, Christophe Arnal,
linux-arm-kernel
In-Reply-To: <20120921150656.GG15609@n2100.arm.linux.org.uk>
> Sorry Alan, I don't think you really understand what I'm saying.
I'm taking about getting rid of the tty_termios_baud_rate() helper not
the uart_get_baud_rate() helper. The tty one has no real purpose any more
because we always set c_ospeed.
The uart layer one just needs some bug fixing I think.
Alan
^ permalink raw reply
* Re: [PATCH v2] serial: omap: fix the reciever line error case
From: Felipe Balbi @ 2012-09-21 16:01 UTC (permalink / raw)
To: Shubhrajyoti D; +Cc: linux-serial, linux-omap, linux-arm-kernel, Greg KH
In-Reply-To: <1348238239-18510-1-git-send-email-shubhrajyoti@ti.com>
[-- Attachment #1: Type: text/plain, Size: 1548 bytes --]
On Fri, Sep 21, 2012 at 08:07:19PM +0530, Shubhrajyoti D wrote:
> This patch does the following
> - In case of errors if there least one data character in the RX FIFO
> read it otherwise it may stall the receiver.
>
> This is recommended in the interrupt reset method in the table 23-246 of
> the omap4 TRM.
>
> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
FWIW:
Reviewed-by: Felipe Balbi <balbi@ti.com>
> ---
> - Tested on omap4sdp.
> - pm tested hitting off in idle and suspend.
> - v2 changes update the changelogs.
> - Also remove the dummy check as FE , PE , BI and OE are the only
> receiver errors.
>
> drivers/tty/serial/omap-serial.c | 4 ++++
> 1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
> index a0d4460..4fea16b 100644
> --- a/drivers/tty/serial/omap-serial.c
> +++ b/drivers/tty/serial/omap-serial.c
> @@ -334,6 +334,10 @@ static unsigned int check_modem_status(struct uart_omap_port *up)
> static void serial_omap_rlsi(struct uart_omap_port *up, unsigned int lsr)
> {
> unsigned int flag;
> + unsigned char ch = 0;
> +
> + if (likely(lsr & UART_LSR_DR))
> + ch = serial_in(up, UART_RX);
>
> up->port.icount.rx++;
> flag = TTY_NORMAL;
> --
> 1.7.5.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* [PATCH] serial: pl011: handle corruption at high clock speeds
From: Linus Walleij @ 2012-09-21 16:38 UTC (permalink / raw)
To: linux-serial, Greg Kroah-Hartman
Cc: linux-arm-kernel, Anmar Oueja, Linus Walleij, stable, Bibek Basu,
Par-Gunnar Hjalmdahl, Guillaume Jaunet, Christophe Arnal,
Matthias Locher, Rajanikanth HV
From: Linus Walleij <linus.walleij@linaro.org>
This works around a few glitches in the ST version of the PL011
serial driver when using very high baud rates, as we do in the
Ux500: 3, 3.25, 4 and 4.05 Mbps.
Problem Observed/rootcause:
When using high baud-rates, and the baudrate*8 is getting close to
the provided clock frequency (so a division factor close to 1), when
using bursts of characters (so they are abutted), then it seems as if
there is not enough time to detect the beginning of the start-bit which
is a timing reference for the entire character, and thus the sampling
moment of character bits is moving towards the end of each bit, instead
of the middle.
Fix:
Increase slightly the RX baud rate of the UART above the theoretical
baudrate by 5%. This will definitely give more margin time to the
UART_RX to correctly sample the data at the middle of the bit period.
Also fix the ages old copy-paste error in the very stressed comment,
it's referencing the registers used in the PL010 driver rather than
the PL011 ones.
Cc: stable@kernel.org
Cc: Bibek Basu <bibek.basu@stericsson.com>
Cc: Par-Gunnar Hjalmdahl <par-gunnar.hjalmdahl@stericsson.com>
Signed-off-by: Guillaume Jaunet <guillaume.jaunet@stericsson.com>
Signed-off-by: Christophe Arnal <christophe.arnal@stericsson.com>
Signed-off-by: Matthias Locher <matthias.locher@stericsson.com>
Signed-off-by: Rajanikanth HV <rajanikanth.hv@stericsson.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
This patch fell off the earlier patch series so better send it
alone as it should go into stable.
---
drivers/tty/serial/amba-pl011.c | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index d626d84..cb9f694 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -1594,13 +1594,26 @@ pl011_set_termios(struct uart_port *port, struct ktermios *termios,
old_cr &= ~ST_UART011_CR_OVSFACT;
}
+ /*
+ * Workaround for the ST Micro oversampling variants to
+ * increase the bitrate slightly, by lowering the divisor,
+ * to avoid delayed sampling of start bit at high speeds,
+ * else we see data corruption.
+ */
+ if (uap->vendor->oversampling) {
+ if (baud > 3000000 && baud < 3250000 && quot > 1)
+ quot -= 1;
+ else if (baud > 3250000 && quot > 2)
+ quot -= 2;
+ }
/* Set baud rate */
writew(quot & 0x3f, port->membase + UART011_FBRD);
writew(quot >> 6, port->membase + UART011_IBRD);
/*
* ----------v----------v----------v----------v-----
- * NOTE: MUST BE WRITTEN AFTER UARTLCR_M & UARTLCR_L
+ * NOTE: lcrh_tx and lcrh_rx MUST BE WRITTEN AFTER
+ * UART011_FBRD & UART011_IBRD.
* ----------^----------^----------^----------^-----
*/
writew(lcr_h, port->membase + uap->lcrh_rx);
--
1.7.11.3
^ permalink raw reply related
* Re: [PATCH] serial: set correct baud_base for EXSYS EX-41092 Dual 16950
From: Greg KH @ 2012-09-21 16:49 UTC (permalink / raw)
To: Flavio Leitner; +Cc: linux-serial, linux-kernel, Niels de Vos, Alan Cox
In-Reply-To: <1347908021-13570-1-git-send-email-fbl@redhat.com>
On Mon, Sep 17, 2012 at 03:53:41PM -0300, Flavio Leitner wrote:
> Apparently the same card model has two IDs, so this patch
> complements the commit 39aced68d664291db3324d0fcf0985ab5626aac2
> adding the missing one.
>
> Signed-off-by: Flavio Leitner <fbl@redhat.com>
> ---
> drivers/tty/serial/8250/8250_pci.c | 7 +++++--
> include/linux/pci_ids.h | 3 ++-
Please read the top of the pci_ids.h file for why we don't want to add
new entries there.
Please fix that up and resend this.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 3/3] serial: pl011: allow very high baudrates
From: Linus Walleij @ 2012-09-21 17:52 UTC (permalink / raw)
To: Alan Cox
Cc: Russell King - ARM Linux, Greg Kroah-Hartman, Alan Cox,
Linus Walleij, linux-serial, Guillaume Jaunet,
Par-Gunnar Hjalmdahl, Anmar Oueja, Matthias Locher,
Rajanikanth H.V, Christophe Arnal, linux-arm-kernel
In-Reply-To: <20120921162543.1ed18775@pyramind.ukuu.org.uk>
On Fri, Sep 21, 2012 at 5:25 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> Untested but I suspect the following may help
Nope it doesn't, it's not this part that goes wrong, it's the call to
tty_termios_encode_baud_rate() that is the problem, not how it
gets called. It's that function that fuzzes and "snaps" the baudrate
to some rough-fit speed and screws things up for me.
But I looked a bit at the patch as such.
It doesn't compile, but when I fix it like this:
> - if (baud >= min && baud <= max)
> + if (baud >= min && baud <= max) {
> + tty_termios_encode_baud_rate(tty, baud, baud);
s/tty/termios/
Then it compiles, but regresses.
What's going wrong is that the termios encoding of zero does
not happen anymore, for baudrate 0, i.e whereas we used to
encode 0 into termios and then return 9600 this encodes
9600 and returns 9600.
So if I handle baudrate 9600 specially instead like this it works:
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 7d9fbb8..a2442fb 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -351,8 +351,9 @@ uart_get_baud_rate(struct uart_port *port, struct
ktermios *termios,
else if (flags == UPF_SPD_WARP)
altbaud = 460800;
+ baud = tty_termios_baud_rate(termios);
+
for (try = 0; try < 2; try++) {
- baud = tty_termios_baud_rate(termios);
/*
* The spd_hi, spd_vhi, spd_shi, spd_warp kludge...
@@ -362,26 +363,27 @@ uart_get_baud_rate(struct uart_port *port,
struct ktermios *termios,
baud = altbaud;
/*
- * Special case: B0 rate.
+ * Special case: B0 rate. Note: this breaks if the
+ * device cannot support 9600 baud
*/
if (baud == 0) {
hung_up = 1;
- baud = 9600;
+ /* Encode zeroes to preserve semantics */
+ tty_termios_encode_baud_rate(termios, 0, 0);
+ return 9600;
}
- if (baud >= min && baud <= max)
+ if (baud >= min && baud <= max) {
+ tty_termios_encode_baud_rate(termios, baud, baud);
return baud;
+ }
/*
* Oops, the quotient was zero. Try again with
* the old baud rate if possible.
*/
- termios->c_cflag &= ~CBAUD;
if (old) {
baud = tty_termios_baud_rate(old);
- if (!hung_up)
- tty_termios_encode_baud_rate(termios,
- baud, baud);
old = NULL;
continue;
}
@@ -392,11 +394,9 @@ uart_get_baud_rate(struct uart_port *port, struct
ktermios *termios,
*/
if (!hung_up) {
if (baud <= min)
- tty_termios_encode_baud_rate(termios,
- min + 1, min + 1);
+ baud = min + 1;
else
- tty_termios_encode_baud_rate(termios,
- max - 1, max - 1);
+ baud = max - 1;
}
}
/* Should never happen */
(FWIW Signed-off-by: Linus Walleij <linus.walleij@linaro.org> for the twoliner)
But as mentioned I get the same errors...
Yours,
Linus Walleij
^ permalink raw reply related
* [PATCH] RFC: serial: pl011: allow very high baudrates
From: Linus Walleij @ 2012-09-21 17:59 UTC (permalink / raw)
To: linux-serial, Greg Kroah-Hartman, Russell King, Alan Cox
Cc: linux-arm-kernel, Anmar Oueja, Linus Walleij,
Par-Gunnar Hjalmdahl, Guillaume Jaunet, Christophe Arnal,
Matthias Locher, Rajanikanth HV
From: Linus Walleij <linus.walleij@linaro.org>
The ST Microelectronics variant of the PL011 is capable of supporting
very high non-standard baud rates, even above 4 Mbps. Mostly this
works, but when we try to use 4050000 baud, the code in
tty_termios_encode_baud_rate(), which is called from
uart_get_baud_rate() will do some fuzzy matching and "snap" the
baudrate to 4000000 baud, which happens to be in the baud_table[].
However there is an encouraging comment from Alan Cox above the
tty_termios_input_baud_rate() function stating that device
drivers should use ->c_[io]speed directly as they are updated.
I tried this, but that requires the driver to handle a few
odd cases such as when the ->c_ispeed and ->c_ospeed differs,
or when they are both send it as 0 (which happens a lot in
the kernel) so that would require pushing a lot of duplicate
code into the driver to get the same behaviour.
Here I take a middle-ground approach: if the baud rate of
->c_ispeed and ->c_ospeed differs, or if either of them is
zero, I still call the uart_get_baud_rate() function, but
root the baudrate to what the block can handle at max.
Cc: Par-Gunnar Hjalmdahl <par-gunnar.hjalmdahl@stericsson.com>
Signed-off-by: Guillaume Jaunet <guillaume.jaunet@stericsson.com>
Signed-off-by: Christophe Arnal <christophe.arnal@stericsson.com>
Signed-off-by: Matthias Locher <matthias.locher@stericsson.com>
Signed-off-by: Rajanikanth HV <rajanikanth.hv@stericsson.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
After discussing with Alan and Russell I still don't feel this
is the right solution, but it demonstrates Russell's point of
how intelligence (here just some checking of ->c_[io]speed,
but you get the point) is moved out of the serial/tty core and
out into the drivers if we do this.
Maybe a new function is needed in the serial core, one that
will just call out to serial and tty like this if the c_[io]speed
differs or either is zero?
---
drivers/tty/serial/amba-pl011.c | 30 ++++++++++++++++++++++++------
1 file changed, 24 insertions(+), 6 deletions(-)
diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index cb9f694..778a6a5 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -1492,18 +1492,36 @@ pl011_set_termios(struct uart_port *port, struct ktermios *termios,
struct uart_amba_port *uap = (struct uart_amba_port *)port;
unsigned int lcr_h, old_cr;
unsigned long flags;
- unsigned int baud, quot, clkdiv;
+ unsigned int max_baud, baud, quot;
if (uap->vendor->oversampling)
- clkdiv = 8;
+ max_baud = port->uartclk / 8;
else
- clkdiv = 16;
+ max_baud = port->uartclk / 16;
/*
- * Ask the core to calculate the divisor for us.
+ * For "zero" speeds or in the case where in and output
+ * speed differs, fall back on using the serial core to
+ * determine applicable baudrate.
*/
- baud = uart_get_baud_rate(port, termios, old, 0,
- port->uartclk / clkdiv);
+ if ((termios->c_ispeed != termios->c_ospeed) ||
+ (termios->c_ispeed == 0) ||
+ (termios->c_ospeed == 0))
+ baud = uart_get_baud_rate(port, termios, old, 0,
+ max_baud);
+ else {
+ /*
+ * Else we just use the requested speed from
+ * termios. But we sanity check it so as not to
+ * exceed hardware limits.
+ */
+ baud = termios->c_ospeed;
+ if (baud > max_baud)
+ baud = max_baud;
+ }
+ dev_dbg(uap->port.dev, "c_ispeed: %u, c_ospeed: %u, "
+ "max_baud: %u, resulting baud rate %u bps\n",
+ termios->c_ispeed, termios->c_ospeed, max_baud, baud);
if (baud > port->uartclk/16)
quot = DIV_ROUND_CLOSEST(port->uartclk * 8, baud);
--
1.7.11.3
^ permalink raw reply related
* Re: [PATCH 3/3] serial: pl011: allow very high baudrates
From: Alan Cox @ 2012-09-21 19:56 UTC (permalink / raw)
To: Linus Walleij
Cc: Russell King - ARM Linux, Greg Kroah-Hartman, Alan Cox,
Linus Walleij, linux-serial, Guillaume Jaunet,
Par-Gunnar Hjalmdahl, Anmar Oueja, Matthias Locher,
Rajanikanth H.V, Christophe Arnal, linux-arm-kernel
In-Reply-To: <CACRpkdakLAPmD0TQwe2VNhdePDuFv2KXD2U1xoP1C0axkx2p7A@mail.gmail.com>
On Fri, 21 Sep 2012 19:52:04 +0200
Linus Walleij <linus.walleij@linaro.org> wrote:
> On Fri, Sep 21, 2012 at 5:25 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
>
> > Untested but I suspect the following may help
>
> Nope it doesn't, it's not this part that goes wrong, it's the call to
> tty_termios_encode_baud_rate() that is the problem, not how it
> gets called. It's that function that fuzzes and "snaps" the baudrate
> to some rough-fit speed and screws things up for me.
It should only snap the baud rate if your caller didn't ask for a specific
speed. With the older uart_get_baud_rate code it mangled the encoding
because BOTHER got mangled by uart_get_baud_rate.
The intended behaviour at tty layer is
Caller passes BOTHER and actual bit rate - we return BOTHER and a bit
rate
Caller does not pass BOTHER (may not be TCGETS2 aware) we snap to the
nearest Bfoo rate if within 5% otherwise we return BOTHER based rates.
If you are calling TCSETS2 passing BOTHER and an actual specific speed
you should always be getting handed back the speed requested as it'll see
the BOTHER flag is present and assume the caller is smart.
So how are you setting the speed and can you see at which point it is
getting mushed ?
Alan
^ permalink raw reply
* Re: Vizzini
From: Greg KH @ 2012-09-21 15:43 UTC (permalink / raw)
To: Alan Cox, Rob Duncan; +Cc: linux-serial
In-Reply-To: <20120919162349.1d9107ba@pyramind.ukuu.org.uk>
[Rob added, as he wrote this driver.]
On Wed, Sep 19, 2012 at 04:23:49PM +0100, Alan Cox wrote:
>
> /* This version of the Linux driver source contains a number of
> abominable conditional compilation sections to manage the API
> changes between kernel versions 2.6.18, 2.6.25, and the latest
> (currently 2.6.27). At some point we'll hand a version of this
> driver off to the mainline Linux source tree, and we'll strip
> all these sections out. For now it makes it much easier to
> keep it all in sync while the driver is being developed. */
oops, I fixed those up, but forgot to drop this comment, now fixed.
> It doesn't seem to
>
> #include <linux/usb/cdc.h>
> #ifndef CDC_DATA_INTERFACE_TYPE
> #define CDC_DATA_INTERFACE_TYPE 0x0a
> #endif
>
> I was wondering why this wasn't using CDC-ACM ?
Yeah, I don't know either. Rob, any ideas why you can't just use the
cdc-acm driver instead?
> struct vizzini_serial_private {
> struct usb_interface *data_interface;
> };
>
> Perhaps that can get simplified ?
Will do.
> static struct vizzini_baud_rate vizzini_baud_rates[] = {
>
> Some explanation might be useful ?
Seems to be how they set the baud rate in their chips, a static table
isn't very flexable :(
> static int vizzini_set_baud_rate(struct usb_serial_port *port,
>
> And the actual rate hould get passed back in the termios
> (tty_termios_encode*)
Ah, missed that.
> static void vizzini_set_termios(struct tty_struct *tty_param,
> struct usb_serial_port *port,
> struct ktermios *old_termios)
> {
>
> struct tty_struct *tty = port->port.tty;
>
> This mistake is all over the driver. port->port.tty is unsafe as it may
> change under you. That's *why* we pass a safe tty reference that the
> driver then ignores.
>
> If the driver disable/enable can lose bytes it also needs to figure out
> if a real hardware change has occurred or some apps will get upset.
Yes, I'll go fix those up.
> } else if ((cflag & CSIZE) == CS5) {
> /* Enabling 5-bit mode is really 9-bit mode! */
> format_size = UART_FORMAT_SIZE_9;
>
> If this is magic hackery for 9bit char then it needs doing properly,
> if it's a clever way to get 5bit then fine.
I don't know, Rob?
>
> } else {
> format_size = UART_FORMAT_SIZE_8;
> }
>
> And for unsupported formats we need to report the format we actually
> picked in the tty->termios data.
Good point.
> static int vizzini_ioctl(struct tty_struct *tty, unsigned int cmd, unsigned long arg)
> {
> struct usb_serial_port *port = tty->driver_data;
> struct vizzini_port_private *portdata = usb_get_serial_port_data(port);
> struct serial_struct ss;
>
> dev_dbg(&port->dev, "%s %08x\n", __func__, cmd);
>
> switch (cmd) {
> case TIOCGSERIAL:
> if (!arg)
> return -EFAULT;
> memset(&ss, 0, sizeof(ss));
> ss.baud_base = portdata->baud_base;
>
> This leaves lots of fields blank rather than correctly filled in as the
> apps will expect. If we support it then it needs to support it all for
> read even if only writing odd bits.
I'll drop it for now.
> if (copy_to_user((void __user *)arg, &ss, sizeof(ss)))
> return -EFAULT;
> break;
>
> case TIOCSSERIAL:
> if (!arg)
> return -EFAULT;
> if (copy_from_user(&ss, (void __user *)arg, sizeof(ss)))
> return -EFAULT;
> portdata->baud_base = ss.baud_base;
>
> Because setting stuff as non superuser without any validation is good.
> Should also support low latency config if we are doing this.
I'll drop it here also.
> #ifdef VIZZINI_IWA
> static const int vizzini_parity[] = {
> 0, 1, 1, 0, 1, 0, 0, 1, 1, 0, 0, 1, 0, 1, 1, 0,
> 1, 0, 0, 1, 0, 1, 1, 0, 0, 1, 1, 0, 1, 0, 0, 1,
> 1, 0, 0, 1, 0, 1, 1, 0, 0, 1, 1, 0, 1, 0, 0, 1,
> 0, 1, 1, 0, 1, 0, 0, 1, 1, 0, 0, 1, 0, 1, 1, 0,
> 1, 0, 0, 1, 0, 1, 1, 0, 0, 1, 1, 0, 1, 0, 0, 1,
> 0, 1, 1, 0, 1, 0, 0, 1, 1, 0, 0, 1, 0, 1, 1, 0,
> 0, 1, 1, 0, 1, 0, 0, 1, 1, 0, 0, 1, 0, 1, 1, 0,
> 1, 0, 0, 1, 0, 1, 1, 0, 0, 1, 1, 0, 1, 0, 0, 1,
> 1, 0, 0, 1, 0, 1, 1, 0, 0, 1, 1, 0, 1, 0, 0, 1,
> 0, 1, 1, 0, 1, 0, 0, 1, 1, 0, 0, 1, 0, 1, 1, 0,
> 0, 1, 1, 0, 1, 0, 0, 1, 1, 0, 0, 1, 0, 1, 1, 0,
> 1, 0, 0, 1, 0, 1, 1, 0, 0, 1, 1, 0, 1, 0, 0, 1,
> 0, 1, 1, 0, 1, 0, 0, 1, 1, 0, 0, 1, 0, 1, 1, 0,
> 1, 0, 0, 1, 0, 1, 1, 0, 0, 1, 1, 0, 1, 0, 0, 1,
> 1, 0, 0, 1, 0, 1, 1, 0, 0, 1, 1, 0, 1, 0, 0, 1,
> 0, 1, 1, 0, 1, 0, 0, 1, 1, 0, 0, 1, 0, 1, 1, 0
> };
> #endif
>
> Doing parity is something I never bothered with but if we do it then we
> ought to have a helper in the tty core instead. Also it's stupid to use
> ints for this as it just blows more cache. In fact you might as well use
> bits and just test_bit() for parity.
This #define isn't enabled, so I really don't know what is going on.
Rob, any ideas?
> static int vizzini_write_room(struct tty_struct *tty)
> {
>
> This seems dodgy - it must never report more than it can guarantee will
> fit. If it ever reports 2048 it can't un-report them as free until those
> 2048 bytes are written. Probably it should be using the kfifo code ?
Yes, odds are the whole driver should be using that instead, I'll see
about porting it to the generic serial buffer handling which does this.
> buffer = kmalloc(bufsize, GFP_ATOMIC);
>
> A fifo would also avoid most of these problems as you can just retry
> tidily. This whole path is really ugly to be honest as it gets called
> with irqs off by code expecting the queueing to be fast.
Agreed.
> static void vizzini_in_callback(struct urb *urb)
> {
> int endpoint = usb_pipeendpoint(urb->pipe);
> struct usb_serial_port *port = urb->context;
> struct vizzini_port_private *portdata = usb_get_serial_port_data(port);
> struct tty_struct *tty = port->port.tty;
>
> No kref taken so unsafe, could also be NULL, so you execute
> NULL->functiontptr. Not good on platforms that don't have low memory
> mapping blocked !
Good point.
> room = tty_buffer_request_room(tty, length);
> if (room != length)
> dev_dbg(&port->dev, "Not enough room in TTY buf, dropped %d chars.\n", length - room);
>
> if (room) {
>
> Not worth checking, just shovel it in - the buffer code will do the work
> and only fail when its really congested and the box is stuffed up. Plus
> your dev_dbg here will deadlock if this is a USB serial console I think ?
Yes, I think it will, but using usb serial consoles is so messy I don't
like thinking about it...
> static void vizzini_int_callback(struct urb *urb)
> {
> struct usb_serial_port *port = urb->context;
> struct vizzini_port_private *portdata =
> usb_get_serial_port_data(port); struct tty_struct *tty =
> port->port.tty;
>
> Again kref...
> :
> dev_dbg(&port->dev, "Resubmitting interrupt IN urb %p\n", urb);
> status = usb_submit_urb(urb, GFP_ATOMIC);
> if (status)
> dev_err(&port->dev, "usb_submit_urb failed with result %d", status);
>
> Whats the error recovery path for this - a single fail kills the port ?
That's pretty normal for usb-serial drivers, I haven't seen a submit
fail ever, unless the device is removed, so I don't think you need to
worry about it.
> }
>
> static int vizzini_open(struct tty_struct *tty_param, struct
> usb_serial_port *port) {
> struct vizzini_port_private *portdata;
> struct usb_serial *serial = port->serial;
> struct tty_struct *tty = port->port.tty;
>
> Again we pass the tty for a reason !
>
> static void vizzini_close(struct usb_serial_port *port)
> {
> int i;
> struct usb_serial *serial = port->serial;
> struct vizzini_port_private *portdata;
> struct tty_struct *tty = port->port.tty;
>
> You can't safelty reference tty from here, but it's not used anyway so
> just kill it off.
Ok I'll fix up both of these.
Thanks so much for the review, I'll work on these, and it would be great
if Rob could answer the above questions as well.
greg k-h
^ permalink raw reply
* Re: [PATCH 3/3] serial: pl011: allow very high baudrates
From: Russell King - ARM Linux @ 2012-09-21 20:34 UTC (permalink / raw)
To: Alan Cox
Cc: Linus Walleij, Greg Kroah-Hartman, Alan Cox, Linus Walleij,
linux-serial, Guillaume Jaunet, Par-Gunnar Hjalmdahl, Anmar Oueja,
Matthias Locher, Rajanikanth H.V, Christophe Arnal,
linux-arm-kernel
In-Reply-To: <20120921205603.436a0194@pyramind.ukuu.org.uk>
On Fri, Sep 21, 2012 at 08:56:03PM +0100, Alan Cox wrote:
> If you are calling TCSETS2 passing BOTHER and an actual specific speed
> you should always be getting handed back the speed requested as it'll see
> the BOTHER flag is present and assume the caller is smart.
Is this something that should be handled by glibc? If so, ARM for
whatever reason still seems to use the standard TCGETS and TCSETS
calls... at least stracing stty in ubuntu precise suggests that's
the case.
^ permalink raw reply
* Re: [PATCH 3/3] serial: pl011: allow very high baudrates
From: Alan Cox @ 2012-09-21 20:55 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Linus Walleij, Greg Kroah-Hartman, Alan Cox, Linus Walleij,
linux-serial, Guillaume Jaunet, Par-Gunnar Hjalmdahl, Anmar Oueja,
Matthias Locher, Rajanikanth H.V, Christophe Arnal,
linux-arm-kernel
In-Reply-To: <20120921203440.GA6796@n2100.arm.linux.org.uk>
On Fri, 21 Sep 2012 21:34:40 +0100
Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> On Fri, Sep 21, 2012 at 08:56:03PM +0100, Alan Cox wrote:
> > If you are calling TCSETS2 passing BOTHER and an actual specific speed
> > you should always be getting handed back the speed requested as it'll see
> > the BOTHER flag is present and assume the caller is smart.
>
> Is this something that should be handled by glibc? If so, ARM for
> whatever reason still seems to use the standard TCGETS and TCSETS
> calls... at least stracing stty in ubuntu precise suggests that's
> the case.
The design was agreed with the glibc people years ago. The glibc folks
then repeatedly ignored it and refused to add it. So I gave up on them.
If you are doing low level tty speed stuff, use ioctl directly.
Now that glibc has had a management change and a clue implant maybe it
can be fixed. Feel free to go kicking deal whales down beaches if you
care.
Alan
^ 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