| Message ID | 1348134368-25663-1-git-send-email-linus.walleij@stericsson.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. If your userspace hasn't been updated to use the integer baud rate setting mechanisms, then that could be where the problem lies. Alternatively, if some Bxxxx setting is not being respected by tty_termios_baud_rate(), that also would need fixing. But the fix you propose in this patch just looks wrong.
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
> 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
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.
> 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
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.
> 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
> 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
diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c index 32240a7..8ac1a42 100644 --- a/drivers/tty/serial/amba-pl011.c +++ b/drivers/tty/serial/amba-pl011.c @@ -1507,10 +1507,19 @@ pl011_set_termios(struct uart_port *port, struct ktermios *termios, } /* - * Ask the core to calculate the divisor for us. + * Override baud calculation for very high non-standard baudrates + * on the ST Micro oversampling variant. This must override the + * uart_get_baud_rate() call, because this function changes + * baudrate to the default on these baudrates. */ - baud = uart_get_baud_rate(port, termios, old, 0, - max_baud); + if (uap->vendor->oversampling && + ((termios->c_ispeed > 4000000) || + (termios->c_ospeed > 4000000))) + baud = termios->c_ispeed; + else + /* Ask the core to calculate the divisor for us. */ + baud = uart_get_baud_rate(port, termios, old, 0, + max_baud); if (baud > port->uartclk/16) quot = DIV_ROUND_CLOSEST(port->uartclk * 8, baud);