Re: [PATCH 05/15] tty: serial: Add 8250-core based omap driver

2014-09-01 Thread Sebastian Andrzej Siewior
On 08/18/2014 03:46 PM, Heikki Krogerus wrote:
 On Fri, Aug 15, 2014 at 07:42:33PM +0200, Sebastian Andrzej Siewior wrote:
 diff --git a/drivers/tty/serial/8250/8250_core.c 
 b/drivers/tty/serial/8250/8250_core.c
 index cc90c19..ab003b6 100644
 --- a/drivers/tty/serial/8250/8250_core.c
 +++ b/drivers/tty/serial/8250/8250_core.c
 @@ -264,6 +264,12 @@ static const struct serial8250_config uart_config[] = {
  .fcr= UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_10,
  .flags  = UART_CAP_FIFO | UART_CAP_AFE,
  },
 +[PORT_OMAP_16750] = {
 +.name   = OMAP,
 +.fifo_size  = 64,
 +.tx_loadsz  = 64,
 +.flags  = UART_CAP_FIFO | UART_CAP_EFR | UART_CAP_SLEEP,
 +},
 
 I don't think you need this. Reasons below...

For those it works. However I have to this value to something because
it can't stay PORT_UNKNOWN. So for now I took PORT_8250 because it is
not used anywhere in the code. The only side effect of this is that I
can't specify the name. I can live with this…

 
  [PORT_TEGRA] = {
  .name   = Tegra,
  .fifo_size  = 32,
 @@ -1390,6 +1396,8 @@ static void serial8250_stop_rx(struct uart_port *port)
  serial8250_rpm_get(up);
  
  up-ier = ~UART_IER_RLSI;
 +if (port-type == PORT_OMAP_16750)
 +up-ier = ~UART_IER_RDI;
 
 I don't see any reason why this could not be always cleared regardless
 of the type:
 
 up-ier = ~(UART_IER_RLSI | UART_IRE_RDI);
 

I remember you brought that up recently asking Alan if it is okay doing
so. Since it looks sane to revert that bit on RX-stop, I will drop that
omap check here.

 [cut]
 
 Since you are not calling serial8250_do_set_termios, 8250_core.c newer
 overrides the FCR set in this driver. However, if the FCR is a
 problem, since Yoshihiro added the member for it to struct
 uart_8250_port (commit aef9a7bd9), just make it possible for the probe
 drivers to provide also it's value:
 
  static int
 
 So instead of using PORT_OMAP_16750:
 up.port.type = PORT_16750;
 up.capabilities = UART_CAP_FIFO | UART_CAP_EFR | UART_CAP_SLEEP;
 
 and the fcr if needed.
 up.fcr = ...
 

That fcr value looks nice so I can't drop my private copy of it.  But
this FCR works different for RX trigger (the way it is used). Which
means to support user configurable RX-level I would need to overwrite
that callback. However since PORT_8250 does not supply any FCR values,
I can just ignore it for now.

 +up.port.iotype = UPIO_MEM32;
 +up.port.flags = UPF_BOOT_AUTOCONF | UPF_FIXED_PORT | UPF_FIXED_TYPE |
 +UPF_SOFT_FLOW | UPF_HARD_FLOW;
 +up.port.private_data = priv;
 +
 +up.port.regshift = 2;
 +up.port.fifosize = 64;
 
 You don't need to set the fifosize unless you want to replace the
 value you get from uart_config array.
Since you made me drop my uart_config array entry I keep this and add
the other values, too

 +up.port.set_termios = omap_8250_set_termios;
 +up.port.pm = omap_8250_pm;
 +up.port.startup = omap_8250_startup;
 +up.port.shutdown = omap_8250_shutdown;
 +up.port.throttle = omap_8250_throttle;
 +up.port.unthrottle = omap_8250_unthrottle;
 +
 +if (pdev-dev.of_node) {
 +up.port.line = of_alias_get_id(pdev-dev.of_node, serial);
 +of_property_read_u32(pdev-dev.of_node, clock-frequency,
 +up.port.uartclk);
 +priv-wakeirq = irq_of_parse_and_map(pdev-dev.of_node, 1);
 +} else {
 +up.port.line = pdev-id;
 +}
 +
 +if (up.port.line  0) {
 +dev_err(pdev-dev, failed to get alias/pdev id, errno %d\n,
 +up.port.line);
 +return -ENODEV;
 +}
 +if (!up.port.uartclk) {
 +up.port.uartclk = DEFAULT_CLK_SPEED;
 +dev_warn(pdev-dev,
 +No clock speed specified: using default: %d\n,
 +DEFAULT_CLK_SPEED);
 +}
 +
 +#ifdef CONFIG_PM_RUNTIME
 +up.capabilities |= UART_CAP_RPM;
 +#endif
 
 By setting this here, you will not get the capabilities from the
 uart_config[type].flags if runtime PM is enabled in any case, right?

Yes. It was not plan to behave like this and is fixed, thanks.

 [cut]
 
 diff --git a/drivers/tty/serial/8250/Makefile 
 b/drivers/tty/serial/8250/Makefile
 index 36d68d0..4bac392 100644
 --- a/drivers/tty/serial/8250/Makefile
 +++ b/drivers/tty/serial/8250/Makefile
 @@ -20,3 +20,4 @@ obj-$(CONFIG_SERIAL_8250_HUB6) += 8250_hub6.o
  obj-$(CONFIG_SERIAL_8250_FSL)   += 8250_fsl.o
  obj-$(CONFIG_SERIAL_8250_DW)+= 8250_dw.o
  obj-$(CONFIG_SERIAL_8250_EM)+= 8250_em.o
 +obj-$(CONFIG_SERIAL_8250_OMAP)  += 8250_omap.o
 diff --git a/include/uapi/linux/serial_core.h 
 b/include/uapi/linux/serial_core.h
 index 5820269..74f9b11 100644
 --- 

Re: [PATCH 05/15] tty: serial: Add 8250-core based omap driver

2014-08-29 Thread Sebastian Andrzej Siewior
On 08/16/2014 12:44 AM, Tony Lindgren wrote:

 Oh and echo mem  /sys/power/state and then hitting a key on the serial
 console won't wake the system. Does that need to be manually configured
 for device_may_wakeup()?

This is what it looks like:

/# echo enabled 
/sys/devices/6800.ocp/4902.serial/tty/ttyS2/power/wakeup

/ # date; echo mem  /sys/power/state; date
Sat Jan  1 07:01:41 UTC 2000
[  249.916503] PM: Syncing filesystems ... done.
[  252.674652] Freezing user space processes ... (elapsed 0.001 seconds)
done.
[  252.683563] Freezing remaining freezable tasks ... (elapsed 0.001
seconds) done.
[  252.693084] Suspending console(s) (use no_console_suspend to debug)
[  252.715820] PM: suspend of devices complete after 11.688 msecs
[  252.722015] PM: late suspend of devices complete after 6.195 msecs
[  252.729187] PM: noirq suspend of devices complete after 7.110 msecs
[  252.729278] Successfully put all powerdomains to target state
[  252.733306] PM: noirq resume of devices complete after 3.967 msecs
[  252.738708] PM: early resume of devices complete after 4.425 msecs
[  252.910400] PM: resume of devices complete after 171.569 msecs
[  252.957855] Restarting tasks ... done.
Sat Jan  1 07:01:51 UTC 2000


  

Sebastian
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 05/15] tty: serial: Add 8250-core based omap driver

2014-08-29 Thread Tony Lindgren
* Sebastian Andrzej Siewior bige...@linutronix.de [140829 08:50]:
 On 08/16/2014 12:44 AM, Tony Lindgren wrote:
 
  Oh and echo mem  /sys/power/state and then hitting a key on the serial
  console won't wake the system. Does that need to be manually configured
  for device_may_wakeup()?
 
 This is what it looks like:
 
 /# echo enabled 
 /sys/devices/6800.ocp/4902.serial/tty/ttyS2/power/wakeup
 
 / # date; echo mem  /sys/power/state; date
 Sat Jan  1 07:01:41 UTC 2000
 [  249.916503] PM: Syncing filesystems ... done.
 [  252.674652] Freezing user space processes ... (elapsed 0.001 seconds)
 done.
 [  252.683563] Freezing remaining freezable tasks ... (elapsed 0.001
 seconds) done.
 [  252.693084] Suspending console(s) (use no_console_suspend to debug)
 [  252.715820] PM: suspend of devices complete after 11.688 msecs
 [  252.722015] PM: late suspend of devices complete after 6.195 msecs
 [  252.729187] PM: noirq suspend of devices complete after 7.110 msecs
 [  252.729278] Successfully put all powerdomains to target state
 [  252.733306] PM: noirq resume of devices complete after 3.967 msecs
 [  252.738708] PM: early resume of devices complete after 4.425 msecs
 [  252.910400] PM: resume of devices complete after 171.569 msecs
 [  252.957855] Restarting tasks ... done.
 Sat Jan  1 07:01:51 UTC 2000

Yes works for me too now thanks.

Tony
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 05/15] tty: serial: Add 8250-core based omap driver

2014-08-21 Thread Sebastian Andrzej Siewior
On 08/15/2014 11:07 PM, Tony Lindgren wrote:
 Nice, now it mostly works for me with off-idle too :) That is as long
 as I have the DMA channels commented out in the .dts file.
 
 And I'm still seeing an occasional hang with pstore console just
 showing:
 
 [  289.076538] In-band Error seen by MPU  at address 0
 [  289.076538] [ cut here ]
 [  289.076568] WARNING: CPU: 0 PID: 99 at drivers/bus/omap_l3_smx.c:162 
 omap3_l3_app_irq+0xdc/0x134()
 [  289.076599] Modules linked in:
 [  289.076599] CPU: 0 PID: 99 Comm: test-idle-off-8 Tainted: GW  
 3.16.0+ #510
 [  289.076629] [c0016c44] (unwind_backtrace) from [c00129c8] 
 (show_stack+0x20/0x24)
 [  289.076660] [c00129c8] (show_stack) from [c0714cd4] 
 (dump_stack+0x88/0xa4)

Okay. So this backtrace does not show more like from where the access
is happening?

 Which most likely means there's still some glitch with the
 runtime PM somewhere and registers are being accessed when
 not clocked. I _think_ I did not see it when I did not have
 console=ttyS2,115200 in my cmdline but was using just pstore
 console.
 
 The device name is ttyS based instead of ttyO. If a ttyO based node name
 is required please ask udev for it. If both driver are activated (this
 and omap-serial) then this serial driver will take control over the
 device due to the link order
 
 That's still not going to help with the existing kernel cmdlines
 and existing installs.. I wonder if we can just do a minimal
 dummy serial-omap.c that just proxies all the ttyO read/write
 access to ttyS?

Hmm. So you are not a friend of the udev solution?. For now the driver
is default n and you have to explicit enable it and _then_ you should
be able to update your command line, etc.
If I introduce a kernel proxy for compatibility I assume that people
will wake up once that compatibility piece is gone.

However, if you insist… I tried to make a symlink but nobody does this
in kernel. The rtc - rtc0 and friends seem to come from udev _or_
distro. So I sff could a second device node with the same major/minor.
That would work for userland but not for the kernel console… So we need
a proxy-console for this.

Before I spent time on this proxy-console I would like to hear from Greg
that he is okay with this.

 
 Regards,
 
 Tony

Sebastian
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 05/15] tty: serial: Add 8250-core based omap driver

2014-08-21 Thread Tony Lindgren
* Sebastian Andrzej Siewior bige...@linutronix.de [140821 04:04]:
 On 08/15/2014 11:07 PM, Tony Lindgren wrote:
  Nice, now it mostly works for me with off-idle too :) That is as long
  as I have the DMA channels commented out in the .dts file.
  
  And I'm still seeing an occasional hang with pstore console just
  showing:
  
  [  289.076538] In-band Error seen by MPU  at address 0
  [  289.076538] [ cut here ]
  [  289.076568] WARNING: CPU: 0 PID: 99 at drivers/bus/omap_l3_smx.c:162 
  omap3_l3_app_irq+0xdc/0x134()
  [  289.076599] Modules linked in:
  [  289.076599] CPU: 0 PID: 99 Comm: test-idle-off-8 Tainted: GW 
   3.16.0+ #510
  [  289.076629] [c0016c44] (unwind_backtrace) from [c00129c8] 
  (show_stack+0x20/0x24)
  [  289.076660] [c00129c8] (show_stack) from [c0714cd4] 
  (dump_stack+0x88/0xa4)
 
 Okay. So this backtrace does not show more like from where the access
 is happening?

No, or if the omap_l3_smx.c can show the address it's not implemented.
 
  Which most likely means there's still some glitch with the
  runtime PM somewhere and registers are being accessed when
  not clocked. I _think_ I did not see it when I did not have
  console=ttyS2,115200 in my cmdline but was using just pstore
  console.
  
  The device name is ttyS based instead of ttyO. If a ttyO based node name
  is required please ask udev for it. If both driver are activated (this
  and omap-serial) then this serial driver will take control over the
  device due to the link order
  
  That's still not going to help with the existing kernel cmdlines
  and existing installs.. I wonder if we can just do a minimal
  dummy serial-omap.c that just proxies all the ttyO read/write
  access to ttyS?
 
 Hmm. So you are not a friend of the udev solution?. For now the driver
 is default n and you have to explicit enable it and _then_ you should
 be able to update your command line, etc.
 If I introduce a kernel proxy for compatibility I assume that people
 will wake up once that compatibility piece is gone.

The udev solution will not help with all the existing cases of
console=ttyO2,115200. But maybe we just need to translate the
cmdline to ttS2 in those cases?

Then the udev rules could fix up most of the distro issues, but
not all of them. And still requires the userspace to be updated.
 
 However, if you insist… I tried to make a symlink but nobody does this
 in kernel. The rtc - rtc0 and friends seem to come from udev _or_
 distro. So I sff could a second device node with the same major/minor.
 That would work for userland but not for the kernel console… So we need
 a proxy-console for this.

Yeah the symlinks won't work, think read-only rootfs for example :)
 
 Before I spent time on this proxy-console I would like to hear from Greg
 that he is okay with this.

Yeah me too.

Tony
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 05/15] tty: serial: Add 8250-core based omap driver

2014-08-18 Thread Heikki Krogerus
Hi,

On Fri, Aug 15, 2014 at 07:42:33PM +0200, Sebastian Andrzej Siewior wrote:
 diff --git a/drivers/tty/serial/8250/8250_core.c 
 b/drivers/tty/serial/8250/8250_core.c
 index cc90c19..ab003b6 100644
 --- a/drivers/tty/serial/8250/8250_core.c
 +++ b/drivers/tty/serial/8250/8250_core.c
 @@ -264,6 +264,12 @@ static const struct serial8250_config uart_config[] = {
   .fcr= UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_10,
   .flags  = UART_CAP_FIFO | UART_CAP_AFE,
   },
 + [PORT_OMAP_16750] = {
 + .name   = OMAP,
 + .fifo_size  = 64,
 + .tx_loadsz  = 64,
 + .flags  = UART_CAP_FIFO | UART_CAP_EFR | UART_CAP_SLEEP,
 + },

I don't think you need this. Reasons below...

   [PORT_TEGRA] = {
   .name   = Tegra,
   .fifo_size  = 32,
 @@ -1390,6 +1396,8 @@ static void serial8250_stop_rx(struct uart_port *port)
   serial8250_rpm_get(up);
  
   up-ier = ~UART_IER_RLSI;
 + if (port-type == PORT_OMAP_16750)
 + up-ier = ~UART_IER_RDI;

I don't see any reason why this could not be always cleared regardless
of the type:

up-ier = ~(UART_IER_RLSI | UART_IRE_RDI);

   up-port.read_status_mask = ~UART_LSR_DR;
   serial_port_out(port, UART_IER, up-ier);
  
 diff --git a/drivers/tty/serial/8250/8250_omap.c 
 b/drivers/tty/serial/8250/8250_omap.c
 new file mode 100644
 index 000..368e9d8
 --- /dev/null
 +++ b/drivers/tty/serial/8250/8250_omap.c
 @@ -0,0 +1,911 @@
 +/*
 + * 8250-core based driver for the OMAP internal UART
 + *
 + *  Copyright (C) 2014 Sebastian Andrzej Siewior
 + *
 + */

[cut]

 +static int omap8250_probe(struct platform_device *pdev)
 +{
 + struct resource *regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 + struct resource *irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
 + struct omap8250_priv *priv;
 + struct uart_8250_port up;
 + int ret;
 + void __iomem *membase;
 +
 + if (!regs || !irq) {
 + dev_err(pdev-dev, missing registers or irq\n);
 + return -EINVAL;
 + }
 +
 + priv = devm_kzalloc(pdev-dev, sizeof(*priv), GFP_KERNEL);
 + if (!priv) {
 + dev_err(pdev-dev, unable to allocate private data\n);
 + return -ENOMEM;
 + }
 + membase = devm_ioremap_nocache(pdev-dev, regs-start,
 + resource_size(regs));
 + if (!membase)
 + return -ENODEV;
 +
 + memset(up, 0, sizeof(up));
 + up.port.dev = pdev-dev;
 + up.port.mapbase = regs-start;
 + up.port.membase = membase;
 + up.port.irq = irq-start;
 + /*
 +  * It claims to be 16C750 compatible however it is a little different.
 +  * It has EFR and has no FCR7_64byte bit. The AFE which it claims to
 +  * have is enabled via EFR instead of MCR.
 +  */
 + up.port.type = PORT_OMAP_16750;

Since you are not calling serial8250_do_set_termios, 8250_core.c newer
overrides the FCR set in this driver. However, if the FCR is a
problem, since Yoshihiro added the member for it to struct
uart_8250_port (commit aef9a7bd9), just make it possible for the probe
drivers to provide also it's value:

--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -2829,7 +2829,9 @@ static void serial8250_config_port(struct uart_port 
*port, int flags)
port-handle_irq = exar_handle_irq;
 
register_dev_spec_attr_grp(up);
-   up-fcr = uart_config[up-port.type].fcr;
+
+   if (!up-fcr)
+   up-fcr = uart_config[up-port.type].fcr;
 }
 
 static int

So instead of using PORT_OMAP_16750:
up.port.type = PORT_16750;
up.capabilities = UART_CAP_FIFO | UART_CAP_EFR | UART_CAP_SLEEP;

and the fcr if needed.
up.fcr = ...

 + up.port.iotype = UPIO_MEM32;
 + up.port.flags = UPF_BOOT_AUTOCONF | UPF_FIXED_PORT | UPF_FIXED_TYPE |
 + UPF_SOFT_FLOW | UPF_HARD_FLOW;
 + up.port.private_data = priv;
 +
 + up.port.regshift = 2;
 + up.port.fifosize = 64;

You don't need to set the fifosize unless you want to replace the
value you get from uart_config array.

 + up.port.set_termios = omap_8250_set_termios;
 + up.port.pm = omap_8250_pm;
 + up.port.startup = omap_8250_startup;
 + up.port.shutdown = omap_8250_shutdown;
 + up.port.throttle = omap_8250_throttle;
 + up.port.unthrottle = omap_8250_unthrottle;
 +
 + if (pdev-dev.of_node) {
 + up.port.line = of_alias_get_id(pdev-dev.of_node, serial);
 + of_property_read_u32(pdev-dev.of_node, clock-frequency,
 + up.port.uartclk);
 + priv-wakeirq = irq_of_parse_and_map(pdev-dev.of_node, 1);
 + } else {
 + up.port.line = pdev-id;
 + }
 +
 + if (up.port.line  0) {
 + dev_err(pdev-dev, failed to get alias/pdev id, errno %d\n,
 + 

Re: [PATCH 05/15] tty: serial: Add 8250-core based omap driver

2014-08-15 Thread Lennart Sorensen
On Fri, Aug 15, 2014 at 07:42:33PM +0200, Sebastian Andrzej Siewior wrote:
 This patch provides a 8250-core based UART driver for the internal OMAP
 UART. The long term goal is to provide the same functionality as the
 current OMAP uart driver and DMA support.
 I tried to merge omap-serial code together with the 8250-core code.
 There should should be hardly a noticable difference. The trigger levels
 are different compared to omap-serial:
 - omap serial
   TX: Interrupt comes after TX FIFO has room for 16 bytes.
   TX of 4096 bytes in one go results in 256 interrupts
 
   RX: Interrupt comes after there is on byte in the FIFO.
   one
   RX of 4096 bytes results in 4096 interrupts.
 
 - this driver
   TX: Interrupt comes once the TX FIFO is empty.
   TX of 4096 bytes results in 65 interrupts. That means there will
   be gaps on the line while the driver reloads the FIFO.

Any idea how long the gap is likely to be?  Probably not much.  I like
the reduction in the number of interrupts.

I suppose if you did an interrupt when half empty or 3/4 empty, you
would avoid the gap, and only increase the interrupt amount a little bit.
Waiting until completely empty gives you larger dma transfers and less
interrupts, but reduces your effective bandwidth on the port.  Is that
really the right tradeoff?  I think the original driver behaviour there
was fairly sane, although the 16 byte value could perhaps be increased
to 32 or 48.

   RX: Interrupt comes once there are 48 bytes in the FIFO or less over
   longer time frame. We have
   1 / 11520 * 10^3 * 16 = 1.38… ms
   1.38ms to react and purge the FIFO on 115200,8N1. Since the other
   driver fired after each byte it had ~5.47ms time to react. This
   _may_ cause problems if one relies on no missing bytes and has no
   flow control. On the other hand we get only 85 interrupts for the
   same amount of data.

Hmm, so if this was 32 instead of 48, it would double the amount of
time you have to react, while only increasing the interrupt rate by 50%
(1 every 32 rather than 1 every 48).  Could be interesting to tweak to
get the balance just right.  Maybe it could have an optional dtb entry
to control it if you don't like the default or is there a way to change
it from user space already?

I know for our system we would like to be able to tolerate 1ms at 230400
without data loss.

-- 
Len Sorensen
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 05/15] tty: serial: Add 8250-core based omap driver

2014-08-15 Thread Sebastian Andrzej Siewior
On 08/15/2014 08:37 PM, Lennart Sorensen wrote:
 On Fri, Aug 15, 2014 at 07:42:33PM +0200, Sebastian Andrzej Siewior wrote:
 This patch provides a 8250-core based UART driver for the internal OMAP
 UART. The long term goal is to provide the same functionality as the
 current OMAP uart driver and DMA support.
 I tried to merge omap-serial code together with the 8250-core code.
 There should should be hardly a noticable difference. The trigger levels
 are different compared to omap-serial:
 - omap serial
   TX: Interrupt comes after TX FIFO has room for 16 bytes.
   TX of 4096 bytes in one go results in 256 interrupts

   RX: Interrupt comes after there is on byte in the FIFO.
one
   RX of 4096 bytes results in 4096 interrupts.

 - this driver
   TX: Interrupt comes once the TX FIFO is empty.
   TX of 4096 bytes results in 65 interrupts. That means there will
   be gaps on the line while the driver reloads the FIFO.
 
 Any idea how long the gap is likely to be?  Probably not much.  I like
 the reduction in the number of interrupts.

If you want to change this to reduce the gap, then you have first
change 8250 core code. Currently it waits until the shift register is
empty.
On the other hand if you use DMA then it can handle transfers  64bytes
in one go and you can start transfers while the FIFO is not completely
empty.

 I suppose if you did an interrupt when half empty or 3/4 empty, you
 would avoid the gap, and only increase the interrupt amount a little bit.
 Waiting until completely empty gives you larger dma transfers and less
 interrupts, but reduces your effective bandwidth on the port.  Is that
 really the right tradeoff?  I think the original driver behaviour there
 was fairly sane, although the 16 byte value could perhaps be increased
 to 32 or 48.

If you use DMA. You program one transfer says 100 bytes. You get an
dma-transfer complete once the 100 bytes are transfered which means the
FIFO has 63 bytes. From this point on you could enqueue the next
transfer with say another 100 bytes. In that scenario you don't see the
gap.

You get only to the gap if you use the non-DMA mode (and not UARTs
support DMA). In that case, yes waiting till there only 16 bytes before
starting the refill would make sense if you want to utilize the port by
100%. But as I said in 0/15, you need to teach the core this first.
Otherwise it will return doing nothing until the shift register is
empty (i.e. until the FIFO is completely empty).

   RX: Interrupt comes once there are 48 bytes in the FIFO or less over
   longer time frame. We have
   1 / 11520 * 10^3 * 16 = 1.38… ms
   1.38ms to react and purge the FIFO on 115200,8N1. Since the other
   driver fired after each byte it had ~5.47ms time to react. This
   _may_ cause problems if one relies on no missing bytes and has no
   flow control. On the other hand we get only 85 interrupts for the
   same amount of data.
 
 Hmm, so if this was 32 instead of 48, it would double the amount of
 time you have to react, while only increasing the interrupt rate by 50%
 (1 every 32 rather than 1 every 48).  Could be interesting to tweak to
 get the balance just right.  Maybe it could have an optional dtb entry
 to control it if you don't like the default or is there a way to change
 it from user space already?

There is patch in Greg's tty tree already where you are able to
configure the RX trigger level. We could wire this up once we agree
which levels we want support. The OMAP supports all levels from 1…63.

 I know for our system we would like to be able to tolerate 1ms at 230400
 without data loss.

Yes, true. However this is only an issue without HW control. With DMA
the buffer is slightly larger. The DMA engine starts the transfer on
its own once there 48 bytes in the FIFO (except in the few cases where
it does not).

Sebastian
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 05/15] tty: serial: Add 8250-core based omap driver

2014-08-15 Thread Lennart Sorensen
On Fri, Aug 15, 2014 at 09:27:59PM +0200, Sebastian Andrzej Siewior wrote:
 If you want to change this to reduce the gap, then you have first
 change 8250 core code. Currently it waits until the shift register is
 empty.

Oh the 8250 normally works this way?  I didn't know that.

 On the other hand if you use DMA then it can handle transfers  64bytes
 in one go and you can start transfers while the FIFO is not completely
 empty.

You can dma more than the fifo size?

 If you use DMA. You program one transfer says 100 bytes. You get an
 dma-transfer complete once the 100 bytes are transfered which means the
 FIFO has 63 bytes. From this point on you could enqueue the next
 transfer with say another 100 bytes. In that scenario you don't see the
 gap.
 
 You get only to the gap if you use the non-DMA mode (and not UARTs
 support DMA). In that case, yes waiting till there only 16 bytes before
 starting the refill would make sense if you want to utilize the port by
 100%. But as I said in 0/15, you need to teach the core this first.
 Otherwise it will return doing nothing until the shift register is
 empty (i.e. until the FIFO is completely empty).

Well if DMA takes care of it, and the normal 8250 is already like this,
then I suppose it is already better than the typical case.

 There is patch in Greg's tty tree already where you are able to
 configure the RX trigger level. We could wire this up once we agree
 which levels we want support. The OMAP supports all levels from 1…63.

All? or just every 4 (that's what I just read in the DRA7xx docs).

 Yes, true. However this is only an issue without HW control. With DMA
 the buffer is slightly larger. The DMA engine starts the transfer on
 its own once there 48 bytes in the FIFO (except in the few cases where
 it does not).

That's nice of it.  I will have to give this a try.

-- 
Len Sorensen
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 05/15] tty: serial: Add 8250-core based omap driver

2014-08-15 Thread Sebastian Andrzej Siewior
On 08/15/2014 09:33 PM, Lennart Sorensen wrote:
 On the other hand if you use DMA then it can handle transfers  64bytes
 in one go and you can start transfers while the FIFO is not completely
 empty.
 
 You can dma more than the fifo size?

Yes. The UART asserts the DMA line as long as there is room for
$TRESHOLD number of bytes. So we never overflow the FIFO. That is why we
can't take any DMA channel but only *the* one.

 There is patch in Greg's tty tree already where you are able to
 configure the RX trigger level. We could wire this up once we agree
 which levels we want support. The OMAP supports all levels from 1…63.
 
 All? or just every 4 (that's what I just read in the DRA7xx docs).

All. Take a look at the RX_TRIGGER constant while comparing source vs
manual :)

Sebastian
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 05/15] tty: serial: Add 8250-core based omap driver

2014-08-15 Thread Tony Lindgren
* Sebastian Andrzej Siewior bige...@linutronix.de [140815 10:46]:
 This patch provides a 8250-core based UART driver for the internal OMAP
 UART. The long term goal is to provide the same functionality as the
 current OMAP uart driver and DMA support.
 I tried to merge omap-serial code together with the 8250-core code.
 There should should be hardly a noticable difference. The trigger levels
 are different compared to omap-serial:

Nice, now it mostly works for me with off-idle too :) That is as long
as I have the DMA channels commented out in the .dts file.

And I'm still seeing an occasional hang with pstore console just
showing:

[  289.076538] In-band Error seen by MPU  at address 0
[  289.076538] [ cut here ]
[  289.076568] WARNING: CPU: 0 PID: 99 at drivers/bus/omap_l3_smx.c:162 
omap3_l3_app_irq+0xdc/0x134()
[  289.076599] Modules linked in:
[  289.076599] CPU: 0 PID: 99 Comm: test-idle-off-8 Tainted: GW  
3.16.0+ #510
[  289.076629] [c0016c44] (unwind_backtrace) from [c00129c8] 
(show_stack+0x20/0x24)
[  289.076660] [c00129c8] (show_stack) from [c0714cd4] 
(dump_stack+0x88/0xa4)

Which most likely means there's still some glitch with the
runtime PM somewhere and registers are being accessed when
not clocked. I _think_ I did not see it when I did not have
console=ttyS2,115200 in my cmdline but was using just pstore
console.

 The device name is ttyS based instead of ttyO. If a ttyO based node name
 is required please ask udev for it. If both driver are activated (this
 and omap-serial) then this serial driver will take control over the
 device due to the link order

That's still not going to help with the existing kernel cmdlines
and existing installs.. I wonder if we can just do a minimal
dummy serial-omap.c that just proxies all the ttyO read/write
access to ttyS?

Regards,

Tony
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 05/15] tty: serial: Add 8250-core based omap driver

2014-08-15 Thread Tony Lindgren
* Tony Lindgren t...@atomide.com [140815 14:10]:
 * Sebastian Andrzej Siewior bige...@linutronix.de [140815 10:46]:
  This patch provides a 8250-core based UART driver for the internal OMAP
  UART. The long term goal is to provide the same functionality as the
  current OMAP uart driver and DMA support.
  I tried to merge omap-serial code together with the 8250-core code.
  There should should be hardly a noticable difference. The trigger levels
  are different compared to omap-serial:
 
 Nice, now it mostly works for me with off-idle too :) That is as long
 as I have the DMA channels commented out in the .dts file.
 
 And I'm still seeing an occasional hang with pstore console just
 showing:
 
 [  289.076538] In-band Error seen by MPU  at address 0
 [  289.076538] [ cut here ]
 [  289.076568] WARNING: CPU: 0 PID: 99 at drivers/bus/omap_l3_smx.c:162 
 omap3_l3_app_irq+0xdc/0x134()
 [  289.076599] Modules linked in:
 [  289.076599] CPU: 0 PID: 99 Comm: test-idle-off-8 Tainted: GW  
 3.16.0+ #510
 [  289.076629] [c0016c44] (unwind_backtrace) from [c00129c8] 
 (show_stack+0x20/0x24)
 [  289.076660] [c00129c8] (show_stack) from [c0714cd4] 
 (dump_stack+0x88/0xa4)
 
 Which most likely means there's still some glitch with the
 runtime PM somewhere and registers are being accessed when
 not clocked. I _think_ I did not see it when I did not have
 console=ttyS2,115200 in my cmdline but was using just pstore
 console.

Oh and echo mem  /sys/power/state and then hitting a key on the serial
console won't wake the system. Does that need to be manually configured
for device_may_wakeup()?
 
  The device name is ttyS based instead of ttyO. If a ttyO based node name
  is required please ask udev for it. If both driver are activated (this
  and omap-serial) then this serial driver will take control over the
  device due to the link order
 
 That's still not going to help with the existing kernel cmdlines
 and existing installs.. I wonder if we can just do a minimal
 dummy serial-omap.c that just proxies all the ttyO read/write
 access to ttyS?
 
 Regards,
 
 Tony
 --
 To unsubscribe from this list: send the line unsubscribe linux-kernel in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
 Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html