serial: sh-sci: Support custom speed setting

Commit Message

Eugeniu Rosca Jan. 29, 2020, 4:19 p.m. UTC

From: Torii Kenichi <[e-mail]>

This patch is necessary to use BT module and XM module with DENSO TEN
development board.

This patch supports ASYNC_SPD_CUST flag by ioctl(TIOCSSERIAL), enables
custom speed setting with setserial(1).

The custom speed is calculated from uartclk and custom_divisor.
If custom_divisor is zero, custom speed setting is invalid.

Signed-off-by: Torii Kenichi <[e-mail]>
[erosca: rebase against v5.5]
Signed-off-by: Eugeniu Rosca <[e-mail]>
---
 drivers/tty/serial/sh-sci.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Geert Uytterhoeven Jan. 30, 2020, 12:32 p.m. UTC | #1

Hi Eugeniu,

On Wed, Jan 29, 2020 at 5:20 PM Eugeniu Rosca <[e-mail]> wrote:
> From: Torii Kenichi <[e-mail]>
>
> This patch is necessary to use BT module and XM module with DENSO TEN
> development board.
>
> This patch supports ASYNC_SPD_CUST flag by ioctl(TIOCSSERIAL), enables
> custom speed setting with setserial(1).
>
> The custom speed is calculated from uartclk and custom_divisor.
> If custom_divisor is zero, custom speed setting is invalid.
>
> Signed-off-by: Torii Kenichi <[e-mail]>
> [erosca: rebase against v5.5]
> Signed-off-by: Eugeniu Rosca <[e-mail]>

Thanks for your patch!

While this seems to work fine[*], I have a few comments/questions:
  1. This feature seems to be deprecated:

         sh-sci e6e68000.serial: setserial sets custom speed on
ttySC1. This is deprecated.

  2. As the wanted speed is specified as a divider, the resulting speed
     may be off, cfr. the example for 57600 below.
     Note that the SCIF device has multiple clock inputs, and can do
     57600 perfectly if the right crystal has been fitted.

 3. What to do with "[PATCH/RFC] serial: sh-sci: Update uartclk based
     on selected clock" (https://patchwork.kernel.org/patch/11103703/)?
     Combined with this, things become pretty complicated and
     unpredictable, as uartclk now always reflect the frequency of the
     last used base clock, which was the optimal one for the previously
     used speed....

I think it would be easier if we just had an API to specify a raw speed.
Perhaps that already exists?

BTW, what's the speed you need for your BT/XM modules?

[*] stty speed 38400 < /dev/ttySC1 followed by
    setserial /dev/ttySC1 spd_cust divisor 1128 gives 57624 bps on Koelsch.

Gr{oetje,eeting}s,

                        Geert

Greg Kroah-Hartman Feb. 10, 2020, 8:57 p.m. UTC | #2

On Thu, Jan 30, 2020 at 01:32:50PM +0100, Geert Uytterhoeven wrote:
> Hi Eugeniu,
> 
> On Wed, Jan 29, 2020 at 5:20 PM Eugeniu Rosca <[e-mail]> wrote:
> > From: Torii Kenichi <[e-mail]>
> >
> > This patch is necessary to use BT module and XM module with DENSO TEN
> > development board.
> >
> > This patch supports ASYNC_SPD_CUST flag by ioctl(TIOCSSERIAL), enables
> > custom speed setting with setserial(1).
> >
> > The custom speed is calculated from uartclk and custom_divisor.
> > If custom_divisor is zero, custom speed setting is invalid.
> >
> > Signed-off-by: Torii Kenichi <[e-mail]>
> > [erosca: rebase against v5.5]
> > Signed-off-by: Eugeniu Rosca <[e-mail]>
> 
> Thanks for your patch!
> 
> While this seems to work fine[*], I have a few comments/questions:
>   1. This feature seems to be deprecated:
> 
>          sh-sci e6e68000.serial: setserial sets custom speed on
> ttySC1. This is deprecated.
> 
>   2. As the wanted speed is specified as a divider, the resulting speed
>      may be off, cfr. the example for 57600 below.
>      Note that the SCIF device has multiple clock inputs, and can do
>      57600 perfectly if the right crystal has been fitted.
> 
>  3. What to do with "[PATCH/RFC] serial: sh-sci: Update uartclk based
>      on selected clock" (https://patchwork.kernel.org/patch/11103703/)?
>      Combined with this, things become pretty complicated and
>      unpredictable, as uartclk now always reflect the frequency of the
>      last used base clock, which was the optimal one for the previously
>      used speed....
> 
> I think it would be easier if we just had an API to specify a raw speed.
> Perhaps that already exists?

Yes, see:
	http://www.panix.com/~grante/arbitrary-baud.c

Eugeniu Rosca Feb. 10, 2020, 9:44 p.m. UTC | #3

Dear Geert and Greg,

On Mon, Feb 10, 2020 at 12:57:35PM -0800, Greg Kroah-Hartman wrote:
> On Thu, Jan 30, 2020 at 01:32:50PM +0100, Geert Uytterhoeven wrote:
> > Hi Eugeniu,
> > 
> > On Wed, Jan 29, 2020 at 5:20 PM Eugeniu Rosca <[e-mail]> wrote:
> > > From: Torii Kenichi <[e-mail]>
> > >
> > > This patch is necessary to use BT module and XM module with DENSO TEN
> > > development board.
> > >
> > > This patch supports ASYNC_SPD_CUST flag by ioctl(TIOCSSERIAL), enables
> > > custom speed setting with setserial(1).
> > >
> > > The custom speed is calculated from uartclk and custom_divisor.
> > > If custom_divisor is zero, custom speed setting is invalid.
> > >
> > > Signed-off-by: Torii Kenichi <[e-mail]>
> > > [erosca: rebase against v5.5]
> > > Signed-off-by: Eugeniu Rosca <[e-mail]>
> > 
> > Thanks for your patch!
> > 
> > While this seems to work fine[*], I have a few comments/questions:
> >   1. This feature seems to be deprecated:
> > 
> >          sh-sci e6e68000.serial: setserial sets custom speed on
> > ttySC1. This is deprecated.
> > 
> >   2. As the wanted speed is specified as a divider, the resulting speed
> >      may be off, cfr. the example for 57600 below.
> >      Note that the SCIF device has multiple clock inputs, and can do
> >      57600 perfectly if the right crystal has been fitted.
> > 
> >  3. What to do with "[PATCH/RFC] serial: sh-sci: Update uartclk based
> >      on selected clock" (https://patchwork.kernel.org/patch/11103703/)?
> >      Combined with this, things become pretty complicated and
> >      unpredictable, as uartclk now always reflect the frequency of the
> >      last used base clock, which was the optimal one for the previously
> >      used speed....
> > 
> > I think it would be easier if we just had an API to specify a raw speed.
> > Perhaps that already exists?
> 
> Yes, see:
> 	http://www.panix.com/~grante/arbitrary-baud.c

This looks like a compelling piece of evidence users should stay away
from implementing and using the kludge (38400 baud) mechanism?

Unless the author and the users of this patch (CC-ed in this thread)
have a different opinion, I consider this topic closed. Thanks!

Geert Uytterhoeven Feb. 11, 2020, 8:15 a.m. UTC | #4

Hi Greg,

CC man

On Mon, Feb 10, 2020 at 9:57 PM Greg Kroah-Hartman
<[e-mail]> wrote:
> On Thu, Jan 30, 2020 at 01:32:50PM +0100, Geert Uytterhoeven wrote:
> > On Wed, Jan 29, 2020 at 5:20 PM Eugeniu Rosca <[e-mail]> wrote:
> > > From: Torii Kenichi <[e-mail]>
> > >
> > > This patch is necessary to use BT module and XM module with DENSO TEN
> > > development board.
> > >
> > > This patch supports ASYNC_SPD_CUST flag by ioctl(TIOCSSERIAL), enables
> > > custom speed setting with setserial(1).
> > >
> > > The custom speed is calculated from uartclk and custom_divisor.
> > > If custom_divisor is zero, custom speed setting is invalid.
> > >
> > > Signed-off-by: Torii Kenichi <[e-mail]>
> > > [erosca: rebase against v5.5]
> > > Signed-off-by: Eugeniu Rosca <[e-mail]>
> >
> > Thanks for your patch!
> >
> > While this seems to work fine[*], I have a few comments/questions:
> >   1. This feature seems to be deprecated:
> >
> >          sh-sci e6e68000.serial: setserial sets custom speed on
> > ttySC1. This is deprecated.
> >
> >   2. As the wanted speed is specified as a divider, the resulting speed
> >      may be off, cfr. the example for 57600 below.
> >      Note that the SCIF device has multiple clock inputs, and can do
> >      57600 perfectly if the right crystal has been fitted.
> >
> >  3. What to do with "[PATCH/RFC] serial: sh-sci: Update uartclk based
> >      on selected clock" (https://patchwork.kernel.org/patch/11103703/)?
> >      Combined with this, things become pretty complicated and
> >      unpredictable, as uartclk now always reflect the frequency of the
> >      last used base clock, which was the optimal one for the previously
> >      used speed....
> >
> > I think it would be easier if we just had an API to specify a raw speed.
> > Perhaps that already exists?
>
> Yes, see:
>         http://www.panix.com/~grante/arbitrary-baud.c

Thanks a lot!!
This must be one of the most guarded secrets of serial port programming ;-)

Implemented since 2006, commit edc6afc5496875a6 ("[PATCH] tty: switch to
ktermios and new framework"), not documented in today's man-pages.

Gr{oetje,eeting}s,

                        Geert

Greg Kroah-Hartman Feb. 11, 2020, 12:30 p.m. UTC | #5

On Tue, Feb 11, 2020 at 09:15:02AM +0100, Geert Uytterhoeven wrote:
> Hi Greg,
> 
> CC man
> 
> On Mon, Feb 10, 2020 at 9:57 PM Greg Kroah-Hartman
> <[e-mail]> wrote:
> > On Thu, Jan 30, 2020 at 01:32:50PM +0100, Geert Uytterhoeven wrote:
> > > On Wed, Jan 29, 2020 at 5:20 PM Eugeniu Rosca <[e-mail]> wrote:
> > > > From: Torii Kenichi <[e-mail]>
> > > >
> > > > This patch is necessary to use BT module and XM module with DENSO TEN
> > > > development board.
> > > >
> > > > This patch supports ASYNC_SPD_CUST flag by ioctl(TIOCSSERIAL), enables
> > > > custom speed setting with setserial(1).
> > > >
> > > > The custom speed is calculated from uartclk and custom_divisor.
> > > > If custom_divisor is zero, custom speed setting is invalid.
> > > >
> > > > Signed-off-by: Torii Kenichi <[e-mail]>
> > > > [erosca: rebase against v5.5]
> > > > Signed-off-by: Eugeniu Rosca <[e-mail]>
> > >
> > > Thanks for your patch!
> > >
> > > While this seems to work fine[*], I have a few comments/questions:
> > >   1. This feature seems to be deprecated:
> > >
> > >          sh-sci e6e68000.serial: setserial sets custom speed on
> > > ttySC1. This is deprecated.
> > >
> > >   2. As the wanted speed is specified as a divider, the resulting speed
> > >      may be off, cfr. the example for 57600 below.
> > >      Note that the SCIF device has multiple clock inputs, and can do
> > >      57600 perfectly if the right crystal has been fitted.
> > >
> > >  3. What to do with "[PATCH/RFC] serial: sh-sci: Update uartclk based
> > >      on selected clock" (https://patchwork.kernel.org/patch/11103703/)?
> > >      Combined with this, things become pretty complicated and
> > >      unpredictable, as uartclk now always reflect the frequency of the
> > >      last used base clock, which was the optimal one for the previously
> > >      used speed....
> > >
> > > I think it would be easier if we just had an API to specify a raw speed.
> > > Perhaps that already exists?
> >
> > Yes, see:
> >         http://www.panix.com/~grante/arbitrary-baud.c
> 
> Thanks a lot!!
> This must be one of the most guarded secrets of serial port programming ;-)

It really is, I have to look it up each time it comes up :(

> Implemented since 2006, commit edc6afc5496875a6 ("[PATCH] tty: switch to
> ktermios and new framework"), not documented in today's man-pages.

yeah, serial port control really needs to be documented better, there's
all sorts of nice ways of controlling them that people don't seem to
know about.  I used to have a link to a bunch of online examples, but
can't seem to find that anymore either.  Ugh, another thing for the long
TODO file...

greg k-h

[e-mail] March 12, 2020, 5:10 a.m. UTC | #6

Dear Greg,

On Tue, 11 Feb 2020 05:57:35 +0900,
Greg Kroah-Hartman wrote:
> 
> On Thu, Jan 30, 2020 at 01:32:50PM +0100, Geert Uytterhoeven wrote:
> > Hi Eugeniu,
> > 
> > On Wed, Jan 29, 2020 at 5:20 PM Eugeniu Rosca <[e-mail]> wrote:
> > > From: Torii Kenichi <[e-mail]>
> > >
> > > This patch is necessary to use BT module and XM module with DENSO TEN
> > > development board.
> > >
> > > This patch supports ASYNC_SPD_CUST flag by ioctl(TIOCSSERIAL), enables
> > > custom speed setting with setserial(1).
> > >
> > > The custom speed is calculated from uartclk and custom_divisor.
> > > If custom_divisor is zero, custom speed setting is invalid.
> > >
> > > Signed-off-by: Torii Kenichi <[e-mail]>
> > > [erosca: rebase against v5.5]
> > > Signed-off-by: Eugeniu Rosca <[e-mail]>
> > 
> > Thanks for your patch!
> > 
> > While this seems to work fine[*], I have a few comments/questions:
> >   1. This feature seems to be deprecated:
> > 
> >          sh-sci e6e68000.serial: setserial sets custom speed on
> > ttySC1. This is deprecated.
> > 
> >   2. As the wanted speed is specified as a divider, the resulting speed
> >      may be off, cfr. the example for 57600 below.
> >      Note that the SCIF device has multiple clock inputs, and can do
> >      57600 perfectly if the right crystal has been fitted.
> > 
> >  3. What to do with "[PATCH/RFC] serial: sh-sci: Update uartclk based
> >      on selected clock" (https://patchwork.kernel.org/patch/11103703/)?
> >      Combined with this, things become pretty complicated and
> >      unpredictable, as uartclk now always reflect the frequency of the
> >      last used base clock, which was the optimal one for the previously
> >      used speed....
> > 
> > I think it would be easier if we just had an API to specify a raw speed.
> > Perhaps that already exists?
> 
> Yes, see:
> 	http://www.panix.com/~grante/arbitrary-baud.c

I saw the code above, I thought I wouldn't write such code normally.

>#include <linux/termios.h>
>
>int ioctl(int d, int request, ...);

Do application programmers have to accept this bad code?

Greg Kroah-Hartman March 12, 2020, 8:55 a.m. UTC | #7

On Thu, Mar 12, 2020 at 05:10:05AM +0000, [e-mail] wrote:
> Dear Greg,
> 
> On Tue, 11 Feb 2020 05:57:35 +0900,
> Greg Kroah-Hartman wrote:
> > 
> > On Thu, Jan 30, 2020 at 01:32:50PM +0100, Geert Uytterhoeven wrote:
> > > Hi Eugeniu,
> > > 
> > > On Wed, Jan 29, 2020 at 5:20 PM Eugeniu Rosca <[e-mail]> wrote:
> > > > From: Torii Kenichi <[e-mail]>
> > > >
> > > > This patch is necessary to use BT module and XM module with DENSO TEN
> > > > development board.
> > > >
> > > > This patch supports ASYNC_SPD_CUST flag by ioctl(TIOCSSERIAL), enables
> > > > custom speed setting with setserial(1).
> > > >
> > > > The custom speed is calculated from uartclk and custom_divisor.
> > > > If custom_divisor is zero, custom speed setting is invalid.
> > > >
> > > > Signed-off-by: Torii Kenichi <[e-mail]>
> > > > [erosca: rebase against v5.5]
> > > > Signed-off-by: Eugeniu Rosca <[e-mail]>
> > > 
> > > Thanks for your patch!
> > > 
> > > While this seems to work fine[*], I have a few comments/questions:
> > >   1. This feature seems to be deprecated:
> > > 
> > >          sh-sci e6e68000.serial: setserial sets custom speed on
> > > ttySC1. This is deprecated.
> > > 
> > >   2. As the wanted speed is specified as a divider, the resulting speed
> > >      may be off, cfr. the example for 57600 below.
> > >      Note that the SCIF device has multiple clock inputs, and can do
> > >      57600 perfectly if the right crystal has been fitted.
> > > 
> > >  3. What to do with "[PATCH/RFC] serial: sh-sci: Update uartclk based
> > >      on selected clock" (https://patchwork.kernel.org/patch/11103703/)?
> > >      Combined with this, things become pretty complicated and
> > >      unpredictable, as uartclk now always reflect the frequency of the
> > >      last used base clock, which was the optimal one for the previously
> > >      used speed....
> > > 
> > > I think it would be easier if we just had an API to specify a raw speed.
> > > Perhaps that already exists?
> > 
> > Yes, see:
> > 	http://www.panix.com/~grante/arbitrary-baud.c
> 
> I saw the code above, I thought I wouldn't write such code normally.

Why not?

> >#include <linux/termios.h>
> >
> >int ioctl(int d, int request, ...);
> 
> Do application programmers have to accept this bad code?

I do not understand what you are asking here.

greg k-h

Geert Uytterhoeven March 12, 2020, 9:03 a.m. UTC | #8

Hi Torii-san,

On Thu, Mar 12, 2020 at 6:10 AM [e-mail]
<[e-mail]> wrote:
> On Tue, 11 Feb 2020 05:57:35 +0900,
> Greg Kroah-Hartman wrote:
> > On Thu, Jan 30, 2020 at 01:32:50PM +0100, Geert Uytterhoeven wrote:
> > > On Wed, Jan 29, 2020 at 5:20 PM Eugeniu Rosca <[e-mail]> wrote:
> > > > From: Torii Kenichi <[e-mail]>
> > > >
> > > > This patch is necessary to use BT module and XM module with DENSO TEN
> > > > development board.
> > > >
> > > > This patch supports ASYNC_SPD_CUST flag by ioctl(TIOCSSERIAL), enables
> > > > custom speed setting with setserial(1).
> > > >
> > > > The custom speed is calculated from uartclk and custom_divisor.
> > > > If custom_divisor is zero, custom speed setting is invalid.
> > > >
> > > > Signed-off-by: Torii Kenichi <[e-mail]>
> > > > [erosca: rebase against v5.5]
> > > > Signed-off-by: Eugeniu Rosca <[e-mail]>
> > >
> > > Thanks for your patch!
> > >
> > > While this seems to work fine[*], I have a few comments/questions:
> > >   1. This feature seems to be deprecated:
> > >
> > >          sh-sci e6e68000.serial: setserial sets custom speed on
> > > ttySC1. This is deprecated.
> > >
> > >   2. As the wanted speed is specified as a divider, the resulting speed
> > >      may be off, cfr. the example for 57600 below.
> > >      Note that the SCIF device has multiple clock inputs, and can do
> > >      57600 perfectly if the right crystal has been fitted.
> > >
> > >  3. What to do with "[PATCH/RFC] serial: sh-sci: Update uartclk based
> > >      on selected clock" (https://patchwork.kernel.org/patch/11103703/)?
> > >      Combined with this, things become pretty complicated and
> > >      unpredictable, as uartclk now always reflect the frequency of the
> > >      last used base clock, which was the optimal one for the previously
> > >      used speed....
> > >
> > > I think it would be easier if we just had an API to specify a raw speed.
> > > Perhaps that already exists?
> >
> > Yes, see:
> >       http://www.panix.com/~grante/arbitrary-baud.c
>
> I saw the code above, I thought I wouldn't write such code normally.
>
> >#include <linux/termios.h>
> >
> >int ioctl(int d, int request, ...);
>
> Do application programmers have to accept this bad code?

I guess you mean the forward declaration of ioctrl()?
No, they should include <sys/ioctl.h> instead.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Yuusuke Ashizuka March 12, 2020, 9:44 a.m. UTC | #9

Dear Greg, Geert,

> I guess you mean the forward declaration of ioctrl()?
> No, they should include <sys/ioctl.h> instead.

Right.
Adding "#include <sys/ioctl.h>" to Greg's sample code causes a 
compilation error.

> > I saw the code above, I thought I wouldn't write such code normally.
> Why not?

Is it normal to declare ioctl() without "#include <sys/ioctl.h>" ?

Thanks & Best Regards,
Yuusuke Ashiduka <[e-mail]>
Embedded System Development Dept. Embedded System Development Div.
FUJITSU COMPUTER TECHNOLOGIES Ltd.

> -----Original Message-----
> From: Geert Uytterhoeven <[e-mail]>
> Sent: Thursday, March 12, 2020 6:03 PM
> To: Torii, Kenichi/鳥居 健一 <[e-mail]>
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; Ashizuka, Yuusuke/
> 芦塚 雄介 <[e-mail]>; [e-mail]
> Subject: Re: [PATCH] serial: sh-sci: Support custom speed setting
> 
> Hi Torii-san,
> 
> On Thu, Mar 12, 2020 at 6:10 AM [e-mail]
> <[e-mail]> wrote:
> > On Tue, 11 Feb 2020 05:57:35 +0900,
> > Greg Kroah-Hartman wrote:
> > > On Thu, Jan 30, 2020 at 01:32:50PM +0100, Geert Uytterhoeven
> wrote:
> > > > On Wed, Jan 29, 2020 at 5:20 PM Eugeniu Rosca
> <[e-mail]> wrote:
> > > > > From: Torii Kenichi <[e-mail]>
> > > > >
> > > > > This patch is necessary to use BT module and XM module with
> DENSO TEN
> > > > > development board.
> > > > >
> > > > > This patch supports ASYNC_SPD_CUST flag by ioctl(TIOCSSERIAL),
> enables
> > > > > custom speed setting with setserial(1).
> > > > >
> > > > > The custom speed is calculated from uartclk and
> custom_divisor.
> > > > > If custom_divisor is zero, custom speed setting is invalid.
> > > > >
> > > > > Signed-off-by: Torii Kenichi <[e-mail]>
> > > > > [erosca: rebase against v5.5]
> > > > > Signed-off-by: Eugeniu Rosca <[e-mail]>
> > > >
> > > > Thanks for your patch!
> > > >
> > > > While this seems to work fine[*], I have a few
> comments/questions:
> > > >   1. This feature seems to be deprecated:
> > > >
> > > >          sh-sci e6e68000.serial: setserial sets custom speed
> on
> > > > ttySC1. This is deprecated.
> > > >
> > > >   2. As the wanted speed is specified as a divider, the resulting
> speed
> > > >      may be off, cfr. the example for 57600 below.
> > > >      Note that the SCIF device has multiple clock inputs, and
> can do
> > > >      57600 perfectly if the right crystal has been fitted.
> > > >
> > > >  3. What to do with "[PATCH/RFC] serial: sh-sci: Update uartclk
> based
> > > >      on selected clock"
> (https://patchwork.kernel.org/patch/11103703/)?
> > > >      Combined with this, things become pretty complicated and
> > > >      unpredictable, as uartclk now always reflect the frequency
> of the
> > > >      last used base clock, which was the optimal one for the
> previously
> > > >      used speed....
> > > >
> > > > I think it would be easier if we just had an API to specify
> a raw speed.
> > > > Perhaps that already exists?
> > >
> > > Yes, see:
> > >       http://www.panix.com/~grante/arbitrary-baud.c
> >
> > I saw the code above, I thought I wouldn't write such code normally.
> >
> > >#include <linux/termios.h>
> > >
> > >int ioctl(int d, int request, ...);
> >
> > Do application programmers have to accept this bad code?
> 
> I guess you mean the forward declaration of ioctrl()?
> No, they should include <sys/ioctl.h> instead.
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 --
> [e-mail]
> 
> In personal conversations with technical people, I call myself a
> hacker. But
> when I'm talking to journalists I just say "programmer" or something
> like that.
>                                 -- Linus Torvalds

Yuusuke Ashizuka March 30, 2020, 7:43 a.m. UTC | #10

Dear Greg, Geert,

> Right.
> Adding "#include <sys/ioctl.h>" to Greg's sample code causes a
> compilation error.
<snip>
> Is it normal to declare ioctl() without "#include <sys/ioctl.h>" ?

I would be happy if you could give me some comments.

> http://www.panix.com/~grante/arbitrary-baud.c

We think this sample code is no good.
Should I work on glibc changes instead of kernel fixes?

Thanks & Best Regards,
Yuusuke Ashiduka <[e-mail]>
Embedded System Development Dept. Embedded System Development Div.
FUJITSU COMPUTER TECHNOLOGIES Ltd.

> -----Original Message-----
> From: Ashizuka, Yuusuke/芦塚 雄介 <[e-mail]>
> Sent: Thursday, March 12, 2020 6:44 PM
> To: 'Geert Uytterhoeven' <[e-mail]>;
> [e-mail]
> Cc: [e-mail]; [e-mail];
> [e-mail];
> [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; Torii, Kenichi/鳥居 健一
> <[e-mail]>
> Subject: RE: [PATCH] serial: sh-sci: Support custom speed setting
> 
> Dear Greg, Geert,
> 
> > I guess you mean the forward declaration of ioctrl()?
> > No, they should include <sys/ioctl.h> instead.
> 
> Right.
> Adding "#include <sys/ioctl.h>" to Greg's sample code causes a
> compilation error.
> 
> > > I saw the code above, I thought I wouldn't write such code
> normally.
> > Why not?
> 
> Is it normal to declare ioctl() without "#include <sys/ioctl.h>" ?
> 
> Thanks & Best Regards,
> Yuusuke Ashiduka <[e-mail]>
> Embedded System Development Dept. Embedded System Development Div.
> FUJITSU COMPUTER TECHNOLOGIES Ltd.
> 
> > -----Original Message-----
> > From: Geert Uytterhoeven <[e-mail]>
> > Sent: Thursday, March 12, 2020 6:03 PM
> > To: Torii, Kenichi/鳥居 健一 <[e-mail]>
> > Cc: [e-mail]; [e-mail];
> > [e-mail];
> [e-mail];
> > [e-mail];
> > [e-mail]; [e-mail];
> > [e-mail]; [e-mail];
> > [e-mail]; [e-mail];
> > [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; Ashizuka, Yuusuke/
> > 芦塚 雄介 <[e-mail]>; [e-mail]
> > Subject: Re: [PATCH] serial: sh-sci: Support custom speed setting
> >
> > Hi Torii-san,
> >
> > On Thu, Mar 12, 2020 at 6:10 AM [e-mail]
> > <[e-mail]> wrote:
> > > On Tue, 11 Feb 2020 05:57:35 +0900,
> > > Greg Kroah-Hartman wrote:
> > > > On Thu, Jan 30, 2020 at 01:32:50PM +0100, Geert Uytterhoeven
> > wrote:
> > > > > On Wed, Jan 29, 2020 at 5:20 PM Eugeniu Rosca
> > <[e-mail]> wrote:
> > > > > > From: Torii Kenichi <[e-mail]>
> > > > > >
> > > > > > This patch is necessary to use BT module and XM module
> with
> > DENSO TEN
> > > > > > development board.
> > > > > >
> > > > > > This patch supports ASYNC_SPD_CUST flag by
> ioctl(TIOCSSERIAL),
> > enables
> > > > > > custom speed setting with setserial(1).
> > > > > >
> > > > > > The custom speed is calculated from uartclk and
> > custom_divisor.
> > > > > > If custom_divisor is zero, custom speed setting is invalid.
> > > > > >
> > > > > > Signed-off-by: Torii Kenichi <[e-mail]>
> > > > > > [erosca: rebase against v5.5]
> > > > > > Signed-off-by: Eugeniu Rosca <[e-mail]>
> > > > >
> > > > > Thanks for your patch!
> > > > >
> > > > > While this seems to work fine[*], I have a few
> > comments/questions:
> > > > >   1. This feature seems to be deprecated:
> > > > >
> > > > >          sh-sci e6e68000.serial: setserial sets custom speed
> > on
> > > > > ttySC1. This is deprecated.
> > > > >
> > > > >   2. As the wanted speed is specified as a divider, the
> resulting
> > speed
> > > > >      may be off, cfr. the example for 57600 below.
> > > > >      Note that the SCIF device has multiple clock inputs,
> and
> > can do
> > > > >      57600 perfectly if the right crystal has been fitted.
> > > > >
> > > > >  3. What to do with "[PATCH/RFC] serial: sh-sci: Update
> uartclk
> > based
> > > > >      on selected clock"
> > (https://patchwork.kernel.org/patch/11103703/)?
> > > > >      Combined with this, things become pretty complicated
> and
> > > > >      unpredictable, as uartclk now always reflect the
> frequency
> > of the
> > > > >      last used base clock, which was the optimal one for the
> > previously
> > > > >      used speed....
> > > > >
> > > > > I think it would be easier if we just had an API to specify
> > a raw speed.
> > > > > Perhaps that already exists?
> > > >
> > > > Yes, see:
> > > >       http://www.panix.com/~grante/arbitrary-baud.c
> > >
> > > I saw the code above, I thought I wouldn't write such code
> normally.
> > >
> > > >#include <linux/termios.h>
> > > >
> > > >int ioctl(int d, int request, ...);
> > >
> > > Do application programmers have to accept this bad code?
> >
> > I guess you mean the forward declaration of ioctrl()?
> > No, they should include <sys/ioctl.h> instead.
> >
> > Gr{oetje,eeting}s,
> >
> >                         Geert
> >
> > --
> > Geert Uytterhoeven -- There's lots of Linux beyond ia32 --
> > [e-mail]
> >
> > In personal conversations with technical people, I call myself
> a
> > hacker. But
> > when I'm talking to journalists I just say "programmer" or
> something
> > like that.
> >                                 -- Linus Torvalds

[e-mail] July 15, 2021, 9:16 p.m. UTC | #11

Mon, Mar 30, 2020 at 07:43:09AM +0000, [e-mail] kirjoitti:
> Dear Greg, Geert,
> 
> > Right.
> > Adding "#include <sys/ioctl.h>" to Greg's sample code causes a
> > compilation error.
> <snip>
> > Is it normal to declare ioctl() without "#include <sys/ioctl.h>" ?
> 
> I would be happy if you could give me some comments.
> 
> > http://www.panix.com/~grante/arbitrary-baud.c
> 
> We think this sample code is no good.
> Should I work on glibc changes instead of kernel fixes?

Side note: I hope introducing spd_cust hack hadn't made upstream.

To the point. Use BOTHER as in code excerpt. Yes, there is a problematic parts
with the headers regarding to this feature. But you may look how others solve
it.

https://github.com/npat-efault/picocom/blob/master/termios2.txt

> > Subject: RE: [PATCH] serial: sh-sci: Support custom speed setting
> > 
> > Dear Greg, Geert,
> > 
> > > I guess you mean the forward declaration of ioctrl()?
> > > No, they should include <sys/ioctl.h> instead.
> > 
> > Right.
> > Adding "#include <sys/ioctl.h>" to Greg's sample code causes a
> > compilation error.
> > 
> > > > I saw the code above, I thought I wouldn't write such code
> > normally.
> > > Why not?
> > 
> > Is it normal to declare ioctl() without "#include <sys/ioctl.h>" ?
> > 
> > Thanks & Best Regards,
> > Yuusuke Ashiduka <[e-mail]>
> > Embedded System Development Dept. Embedded System Development Div.
> > FUJITSU COMPUTER TECHNOLOGIES Ltd.
> > 
> > > -----Original Message-----
> > > From: Geert Uytterhoeven <[e-mail]>
> > > Sent: Thursday, March 12, 2020 6:03 PM
> > > To: Torii, Kenichi/鳥居 健一 <[e-mail]>
> > > Cc: [e-mail]; [e-mail];
> > > [e-mail];
> > [e-mail];
> > > [e-mail];
> > > [e-mail]; [e-mail];
> > > [e-mail]; [e-mail];
> > > [e-mail]; [e-mail];
> > > [e-mail]; [e-mail];
> > > [e-mail]; [e-mail]; Ashizuka, Yuusuke/
> > > 芦塚 雄介 <[e-mail]>; [e-mail]
> > > Subject: Re: [PATCH] serial: sh-sci: Support custom speed setting
> > >
> > > Hi Torii-san,
> > >
> > > On Thu, Mar 12, 2020 at 6:10 AM [e-mail]
> > > <[e-mail]> wrote:
> > > > On Tue, 11 Feb 2020 05:57:35 +0900,
> > > > Greg Kroah-Hartman wrote:
> > > > > On Thu, Jan 30, 2020 at 01:32:50PM +0100, Geert Uytterhoeven
> > > wrote:
> > > > > > On Wed, Jan 29, 2020 at 5:20 PM Eugeniu Rosca
> > > <[e-mail]> wrote:
> > > > > > > From: Torii Kenichi <[e-mail]>
> > > > > > >
> > > > > > > This patch is necessary to use BT module and XM module
> > with
> > > DENSO TEN
> > > > > > > development board.
> > > > > > >
> > > > > > > This patch supports ASYNC_SPD_CUST flag by
> > ioctl(TIOCSSERIAL),
> > > enables
> > > > > > > custom speed setting with setserial(1).
> > > > > > >
> > > > > > > The custom speed is calculated from uartclk and
> > > custom_divisor.
> > > > > > > If custom_divisor is zero, custom speed setting is invalid.
> > > > > > >
> > > > > > > Signed-off-by: Torii Kenichi <[e-mail]>
> > > > > > > [erosca: rebase against v5.5]
> > > > > > > Signed-off-by: Eugeniu Rosca <[e-mail]>
> > > > > >
> > > > > > Thanks for your patch!
> > > > > >
> > > > > > While this seems to work fine[*], I have a few
> > > comments/questions:
> > > > > >   1. This feature seems to be deprecated:
> > > > > >
> > > > > >          sh-sci e6e68000.serial: setserial sets custom speed
> > > on
> > > > > > ttySC1. This is deprecated.
> > > > > >
> > > > > >   2. As the wanted speed is specified as a divider, the
> > resulting
> > > speed
> > > > > >      may be off, cfr. the example for 57600 below.
> > > > > >      Note that the SCIF device has multiple clock inputs,
> > and
> > > can do
> > > > > >      57600 perfectly if the right crystal has been fitted.
> > > > > >
> > > > > >  3. What to do with "[PATCH/RFC] serial: sh-sci: Update
> > uartclk
> > > based
> > > > > >      on selected clock"
> > > (https://patchwork.kernel.org/patch/11103703/)?
> > > > > >      Combined with this, things become pretty complicated
> > and
> > > > > >      unpredictable, as uartclk now always reflect the
> > frequency
> > > of the
> > > > > >      last used base clock, which was the optimal one for the
> > > previously
> > > > > >      used speed....
> > > > > >
> > > > > > I think it would be easier if we just had an API to specify
> > > a raw speed.
> > > > > > Perhaps that already exists?
> > > > >
> > > > > Yes, see:
> > > > >       http://www.panix.com/~grante/arbitrary-baud.c
> > > >
> > > > I saw the code above, I thought I wouldn't write such code
> > normally.
> > > >
> > > > >#include <linux/termios.h>
> > > > >
> > > > >int ioctl(int d, int request, ...);
> > > >
> > > > Do application programmers have to accept this bad code?
> > >
> > > I guess you mean the forward declaration of ioctrl()?
> > > No, they should include <sys/ioctl.h> instead.
> > >
> > > Gr{oetje,eeting}s,
> > >
> > >                         Geert
> > >
> > > --
> > > Geert Uytterhoeven -- There's lots of Linux beyond ia32 --
> > > [e-mail]
> > >
> > > In personal conversations with technical people, I call myself
> > a
> > > hacker. But
> > > when I'm talking to journalists I just say "programmer" or
> > something
> > > like that.
> > >                                 -- Linus Torvalds

Patch

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 87ca6294de0e..dd468909b2c4 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -2405,6 +2405,10 @@  static void sci_set_termios(struct uart_port *port, struct ktermios *termios,
 	if (!baud)
 		goto done;
 
+	if (baud == 38400 && (port->flags & UPF_SPD_MASK) == UPF_SPD_CUST &&
+	    port->custom_divisor)
+		baud = port->uartclk / port->custom_divisor;
+
 	/*
 	 * There can be multiple sources for the sampling clock.  Find the one
 	 * that gives us the smallest deviation from the desired baud rate.