Re: [PATCH 05/15] tty: serial: Add 8250-core based omap driver
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)
Re: [PATCH 05/15] tty: serial: Add 8250-core based omap driver
* Sebastian Andrzej Siewior [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
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
* Sebastian Andrzej Siewior [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] [] (unwind_backtrace) from [] > > (show_stack+0x20/0x24) > > [ 289.076660] [] (show_stack) from [] > > (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
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] [] (unwind_backtrace) from [] > (show_stack+0x20/0x24) > [ 289.076660] [] (show_stack) from [] > (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
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
Re: [PATCH 05/15] tty: serial: Add 8250-core based omap driver
* Tony Lindgren [140815 14:10]: > * Sebastian Andrzej Siewior [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] [] (unwind_backtrace) from [] > (show_stack+0x20/0x24) > [ 289.076660] [] (show_stack) from [] > (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
Re: [PATCH 05/15] tty: serial: Add 8250-core based omap driver
* Sebastian Andrzej Siewior [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] [] (unwind_backtrace) from [] (show_stack+0x20/0x24) [ 289.076660] [] (show_stack) from [] (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
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
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
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
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