* Re: [PATCH v2 1/6] serial: imx: only set DMA rx-ing when DMA starts
From: Uwe Kleine-König @ 2017-07-06 10:02 UTC (permalink / raw)
To: Romain Perier
Cc: Greg Kroah-Hartman, linux-serial, linux-arm-kernel, linux-kernel,
Nandor Han
In-Reply-To: <49bfa666-b036-3ed9-8209-9ea59468371b@collabora.com>
On Thu, Jul 06, 2017 at 10:31:38AM +0200, Romain Perier wrote:
> Hello,
>
>
> Le 05/07/2017 à 15:29, Uwe Kleine-König a écrit :
> > Hello,
> >
> > On Wed, Jul 05, 2017 at 03:07:01PM +0200, Romain Perier wrote:
> >> From: Nandor Han <nandor.han@ge.com>
> >>
> >> Avoid the situation when `dma_is_rxing` could incorrectly signal that
> >> DMA RX channel is receiving data in case DMA preparation or sg mapping
> >> fails.
> >>
> >> This commit fixes the issues by moving the assignment of dma_is_rxing
> >> out of imx_disable_rx_int(), then the variable is set to 1 from
> >> start_rx_dma() only when the preparation is correctly done.
> > I'd write:
> >
> > There are a few issues with setting dma_is_rxing to 1 in
> > imx_disable_rx_int:
> >
> > - Currently always after imx_disable_rx_int() the function
> > start_rx_dma() is called. This dependency isn't obvious though.
> > - start_rx_dma() does error checking and might exit without enabling
> > DMA but keeping dma_is_rxing 1.
> >
> > So the more natural place for setting dma_is_rxing to 1 is in
> > start_rx_dma after all errors are checked.
> >
> > If you use this, there is nothing left of Nandor Han's patch and you can
> > drop his authorship.
> >
> > Best regards
> > Uwe
> >
> Ok, will do. No other feedback for the rest of the series ? (just to
> know if I send a v3 or If I wait a bit...)
I didn't come around looking in the patches I didn't comment yet.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* Re: [PATCH v2 1/6] serial: imx: only set DMA rx-ing when DMA starts
From: Romain Perier @ 2017-07-06 8:31 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Greg Kroah-Hartman, linux-serial, linux-arm-kernel, linux-kernel,
Nandor Han
In-Reply-To: <20170705132910.nmixhnh4l6h2hlga@pengutronix.de>
Hello,
Le 05/07/2017 à 15:29, Uwe Kleine-König a écrit :
> Hello,
>
> On Wed, Jul 05, 2017 at 03:07:01PM +0200, Romain Perier wrote:
>> From: Nandor Han <nandor.han@ge.com>
>>
>> Avoid the situation when `dma_is_rxing` could incorrectly signal that
>> DMA RX channel is receiving data in case DMA preparation or sg mapping
>> fails.
>>
>> This commit fixes the issues by moving the assignment of dma_is_rxing
>> out of imx_disable_rx_int(), then the variable is set to 1 from
>> start_rx_dma() only when the preparation is correctly done.
> I'd write:
>
> There are a few issues with setting dma_is_rxing to 1 in
> imx_disable_rx_int:
>
> - Currently always after imx_disable_rx_int() the function
> start_rx_dma() is called. This dependency isn't obvious though.
> - start_rx_dma() does error checking and might exit without enabling
> DMA but keeping dma_is_rxing 1.
>
> So the more natural place for setting dma_is_rxing to 1 is in
> start_rx_dma after all errors are checked.
>
> If you use this, there is nothing left of Nandor Han's patch and you can
> drop his authorship.
>
> Best regards
> Uwe
>
Ok, will do. No other feedback for the rest of the series ? (just to
know if I send a v3 or If I wait a bit...)
Thanks,
Romain
^ permalink raw reply
* [PATCH 13/18] drivers: tty: serial: Move inline before return type
From: Joe Perches @ 2017-07-05 20:02 UTC (permalink / raw)
To: Andrew Morton, Pat Gefre, Greg Kroah-Hartman
Cc: Jiri Slaby, linux-serial, linux-ia64, linux-kernel
In-Reply-To: <cover.1499284835.git.joe@perches.com>
Make the code like the rest of the kernel.
Signed-off-by: Joe Perches <joe@perches.com>
---
drivers/tty/serial/ioc3_serial.c | 4 ++--
drivers/tty/serial/ioc4_serial.c | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/tty/serial/ioc3_serial.c b/drivers/tty/serial/ioc3_serial.c
index 2a61dd6b4009..906ee770ff4a 100644
--- a/drivers/tty/serial/ioc3_serial.c
+++ b/drivers/tty/serial/ioc3_serial.c
@@ -377,7 +377,7 @@ static struct ioc3_port *get_ioc3_port(struct uart_port *the_port)
* called per port from attach...
* @port: port to initialize
*/
-static int inline port_init(struct ioc3_port *port)
+static inline int port_init(struct ioc3_port *port)
{
uint32_t sio_cr;
struct port_hooks *hooks = port->ip_hooks;
@@ -1430,7 +1430,7 @@ static int receive_chars(struct uart_port *the_port)
* @pending: interrupts to handle
*/
-static int inline
+static inline int
ioc3uart_intr_one(struct ioc3_submodule *is,
struct ioc3_driver_data *idd,
unsigned int pending)
diff --git a/drivers/tty/serial/ioc4_serial.c b/drivers/tty/serial/ioc4_serial.c
index f96bcf9bee25..43d7d32eb150 100644
--- a/drivers/tty/serial/ioc4_serial.c
+++ b/drivers/tty/serial/ioc4_serial.c
@@ -824,7 +824,7 @@ pending_intrs(struct ioc4_soft *soft, int type)
* called per port from attach...
* @port: port to initialize
*/
-static int inline port_init(struct ioc4_port *port)
+static inline int port_init(struct ioc4_port *port)
{
uint32_t sio_cr;
struct hooks *hooks = port->ip_hooks;
@@ -1048,7 +1048,7 @@ static irqreturn_t ioc4_intr(int irq, void *arg)
* IOC4 with serial ports in the system.
* @idd: Master module data for this IOC4
*/
-static int inline ioc4_attach_local(struct ioc4_driver_data *idd)
+static inline int ioc4_attach_local(struct ioc4_driver_data *idd)
{
struct ioc4_port *port;
struct ioc4_port *ports[IOC4_NUM_SERIAL_PORTS];
--
2.10.0.rc2.1.g053435c
^ permalink raw reply related
* [PATCH 00/18] treewide: Move storage class before return type
From: Joe Perches @ 2017-07-05 20:02 UTC (permalink / raw)
To: Andrew Morton, kvm, linux-arm-kernel, kvmarm, linux-m68k,
linux-efi, linux-s390, linux-serial, linux-ia64, linux-usb,
linux-fbdev, linux-omap, alsa-devel
Cc: linux-mips, linux-kernel, linux-cris-kernel, linux-sh
Move the inline/asmlinkage keywords before the return types
Add a checkpatch test for this too.
Joe Perches (18):
checkpatch: improve the STORAGE_CLASS test
ARM: KVM: Move asmlinkage before type
ARM: HP Jornada 7XX: Move inline before return type
CRIS: gpio: Move inline before return type
FRV: tlbflush: Move asmlinkage before return type
ia64: Move inline before return type
ia64: sn: pci: Move inline before type
m68k: coldfire: Move inline before return type
MIPS: SMP: Move asmlinkage before return type
sh: Move inline before return type
x86/efi: Move asmlinkage before return type
drivers: s390: Move static and inline before return type
drivers: tty: serial: Move inline before return type
USB: serial: safe_serial: Move __inline__ before return type
video: fbdev: intelfb: Move inline before return type
video: fbdev: omap: Move inline before return type
ARM: samsung: usb-ohci: Move inline before return type
ALSA: opl4: Move inline before return type
arch/arm/include/asm/kvm_hyp.h | 8 ++++----
arch/arm/mach-sa1100/jornada720_ssp.c | 2 +-
arch/cris/arch-v10/drivers/gpio.c | 4 ++--
arch/frv/include/asm/tlbflush.h | 8 ++++----
arch/ia64/kernel/mca.c | 2 +-
arch/ia64/sn/pci/pcibr/pcibr_ate.c | 2 +-
arch/ia64/sn/pci/tioce_provider.c | 4 ++--
arch/m68k/coldfire/intc-simr.c | 4 ++--
arch/mips/include/asm/smp.h | 2 +-
arch/sh/mm/cache-sh5.c | 2 +-
arch/x86/include/asm/efi.h | 4 ++--
drivers/s390/net/ctcm_main.c | 2 +-
drivers/s390/net/qeth_l3_main.c | 2 +-
drivers/tty/serial/ioc3_serial.c | 4 ++--
drivers/tty/serial/ioc4_serial.c | 4 ++--
drivers/usb/serial/safe_serial.c | 2 +-
drivers/video/fbdev/intelfb/intelfbdrv.c | 2 +-
drivers/video/fbdev/omap/lcdc.c | 6 +++---
include/linux/platform_data/usb-ohci-s3c2410.h | 2 +-
scripts/checkpatch.pl | 12 ++++++++++--
sound/drivers/opl4/opl4_lib.c | 2 +-
21 files changed, 44 insertions(+), 36 deletions(-)
--
2.10.0.rc2.1.g053435c
^ permalink raw reply
* Re: [PATCH v2 3/6] serial: imx: remove CTSC and CTS handling
From: Clemens Gruber @ 2017-07-05 14:42 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Romain Perier, Greg Kroah-Hartman, linux-serial, linux-arm-kernel,
linux-kernel, Nandor Han, Fabio Estevam
In-Reply-To: <20170705133845.32ov37meqcea3wtz@pengutronix.de>
Hi,
On Wed, Jul 05, 2017 at 03:38:45PM +0200, Uwe Kleine-König wrote:
> Cc += Clemens Gruber + Fabio Estevam
>
> On Wed, Jul 05, 2017 at 03:07:03PM +0200, Romain Perier wrote:
> > From: Nandor Han <nandor.han@ge.com>
> >
> > CTSC and CTS are not related to DMA and might add
> > disruption in some cases.
> >
> > Signed-off-by: Romain Perier <romain.perier@collabora.com>
>
> If it was Nandor Han who created this patch, it would be great to get
> his sob. If it was you, drop the From: line above.
>
> > ---
> > drivers/tty/serial/imx.c | 5 -----
> > 1 file changed, 5 deletions(-)
> >
> > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > index 5291b86..dd3ebb4 100644
> > --- a/drivers/tty/serial/imx.c
> > +++ b/drivers/tty/serial/imx.c
> > @@ -1249,11 +1249,6 @@ static void imx_disable_dma(struct imx_port *sport)
> > imx_stop_rx_dma(sport);
> > imx_stop_tx_dma(sport);
> >
> > - /* clear UCR2 */
> > - temp = readl(sport->port.membase + UCR2);
> > - temp &= ~(UCR2_CTSC | UCR2_CTS | UCR2_ATEN);
> > - writel(temp, sport->port.membase + UCR2);
> > -
>
> Before this patch imx_disable_dma resulted in the #CTS pin being high
> (inactive).
>
> Does this qualify as a fix? If so, you should sort this patch to the
> beginning of the series. Did you do test this patch and its effects
> separately?
>
> @Clemens: maybe this patch makes a relevant difference when the port is
> operated in rs485 mode. Do you care to test?
I just finished testing it. The results are about the same as with v1 of
this patch series:
Applying v2 of patch 1/6, 2/6 and 3/6 (or even 3/6 alone) does not fix
the RS-485 DMA bug. It behaves exactly the same as without these
patches, meaning the whole xmit circ_buf (UART_XMIT_SIZE bytes) is sent
out, as seen in the logic analyzer screenshots from my first bug report:
https://pqgruber.com/rs485_results.png
Applying the whole series does make a (small) difference though:
The first few transmissions after a fresh boot work correctly! However,
after a few transmissions, characters are sent out twice and longer
transmissions are garbled, although not in the same way as before. The
whole circ_buf is no longer sent out.
With all patches from this series applied, I see the following on the
logic analyzer, when calling "echo Test > /dev/ttymxc4":
https://pqgruber.com/rs485txtest.png
(This pattern is not always the same, sometimes it is TeTesstt\n\n,
sometimes TeTesst\nt\n or TeTsestt\n\n and so on)
This behavior is reproducible on i.MX6Q and i.MX6D, not on i.MX6S/etc.,
I therefore assume that it is a SMP-/locking-related problem.
I will try to debug this further, and verify if - with this series
applied - xmit->tail is still jumping over xmit->head. And when exactly
this is happening.
Help is much appreciated!
Best regards,
Clemens
^ permalink raw reply
* Re: [PATCH 4/6] dt-bindings: serial: Document RDA Micro UART
From: Rob Herring @ 2017-07-05 14:12 UTC (permalink / raw)
To: Andreas Färber
Cc: linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, service, zhao_steven,
Greg Kroah-Hartman, Mark Rutland, linux-serial@vger.kernel.org,
devicetree@vger.kernel.org
In-Reply-To: <d9b00fd8-2e9e-fb01-9056-db0d32b3e80b@suse.de>
On Sun, Jul 02, 2017 at 03:52:39AM +0200, Andreas Färber wrote:
> Am 30.06.2017 um 16:27 schrieb Rob Herring:
> > On Fri, Jun 30, 2017 at 6:12 AM, Andreas Färber <afaerber@suse.de> wrote:
> >> Am 29.06.2017 um 22:10 schrieb Rob Herring:
> >>> On Tue, Jun 27, 2017 at 02:55:18AM +0200, Andreas Färber wrote:
> >>>> Add an initial binding for the RDA8810PL UART.
> >>>>
> >>>> Signed-off-by: Andreas Färber <afaerber@suse.de>
> >>>> ---
> >>>> Documentation/devicetree/bindings/serial/rda-uart.txt | 13 +++++++++++++
> >>>> 1 file changed, 13 insertions(+)
> >>>> create mode 100644 Documentation/devicetree/bindings/serial/rda-uart.txt
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/serial/rda-uart.txt b/Documentation/devicetree/bindings/serial/rda-uart.txt
> >>>> new file mode 100644
> >>>> index 000000000000..6840a8aee035
> >>>> --- /dev/null
> >>>> +++ b/Documentation/devicetree/bindings/serial/rda-uart.txt
> >>>> @@ -0,0 +1,13 @@
> >>>> +RDA Micro UART
> >>>> +
> >>>> +Required properties:
> >>>> +- compatible : "rda,8810pl-uart" for RDA8810PL
> >>>> +- reg : Offset and length of the register set for the device.
> >>>
> >>> No clocks or interrupts?
> >>
> >> Not yet. I've only pieced together an earlycon driver so far, no full
> >> serial driver. The .dtsi doesn't even have an interrupt-controller node
> >> yet - wasn't clear to me whether this SoC even has a GIC and, if so,
> >> where, from the downstream pre-DT code.
> >
> > How far can you boot with no interrupts?
>
> That was described in the cover letter, as usual.
>
> > DT bindings shouldn't unnecessarily evolve. Really, anything added
> > should be optional to maintain compatibility. Sometimes that's
> > unavoidable, but this isn't one of those cases.
>
> Makes sense. Maybe this series should've been marked RFC. It's too late
> for 4.13 anyway, so there's lots of time left to extend the bindings.
>
> BTW I saw that Kevin was adding a disclaimer to the Amlogic bindings
> that they are not stable. Is that required here, too?
I'm not really a fan of that. I've yet to see anyone remove an unstable
tag. I'd rather see it for specific bindings, rather than platforms as a
whole. It shouldn't be that hard to get most bindings complete enough
and to a point that can be extended in a compatible way. In the end,
decisions about breaking compatibility are up to the platform maintainer
regardless of any text in some document.
> > So submit this when
> > you have something more complete.
>
> Negative. I worked weeks and months towards perfect STM32 drivers, and
> despite showing it to ST and at a LinuxCon Japan, someone else sent less
> sophisticated patches (e.g., fixed-clocks) and took all the credit. Same
> with Turris Omnia. Therefore I am now submitting my new patches early,
> not just when everything is complete.
Fair enough. I should have said, we'll apply/ack when more complete.
Submit it as often as you want.
Rob
^ permalink raw reply
* Re: [PATCH v2 3/6] serial: imx: remove CTSC and CTS handling
From: Uwe Kleine-König @ 2017-07-05 13:38 UTC (permalink / raw)
To: Romain Perier
Cc: Greg Kroah-Hartman, linux-serial, linux-arm-kernel, linux-kernel,
Nandor Han, Clemens Gruber, Fabio Estevam
In-Reply-To: <20170705130706.10388-4-romain.perier@collabora.com>
Cc += Clemens Gruber + Fabio Estevam
On Wed, Jul 05, 2017 at 03:07:03PM +0200, Romain Perier wrote:
> From: Nandor Han <nandor.han@ge.com>
>
> CTSC and CTS are not related to DMA and might add
> disruption in some cases.
>
> Signed-off-by: Romain Perier <romain.perier@collabora.com>
If it was Nandor Han who created this patch, it would be great to get
his sob. If it was you, drop the From: line above.
> ---
> drivers/tty/serial/imx.c | 5 -----
> 1 file changed, 5 deletions(-)
>
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 5291b86..dd3ebb4 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -1249,11 +1249,6 @@ static void imx_disable_dma(struct imx_port *sport)
> imx_stop_rx_dma(sport);
> imx_stop_tx_dma(sport);
>
> - /* clear UCR2 */
> - temp = readl(sport->port.membase + UCR2);
> - temp &= ~(UCR2_CTSC | UCR2_CTS | UCR2_ATEN);
> - writel(temp, sport->port.membase + UCR2);
> -
Before this patch imx_disable_dma resulted in the #CTS pin being high
(inactive).
Does this qualify as a fix? If so, you should sort this patch to the
beginning of the series. Did you do test this patch and its effects
separately?
@Clemens: maybe this patch makes a relevant difference when the port is
operated in rs485 mode. Do you care to test?
> imx_setup_ufcr(sport, TXTL_DEFAULT, RXTL_DEFAULT);
>
> sport->dma_is_enabled = 0;
> --
> 1.8.3.1
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* Re: [PATCH v2 1/6] serial: imx: only set DMA rx-ing when DMA starts
From: Uwe Kleine-König @ 2017-07-05 13:29 UTC (permalink / raw)
To: Romain Perier
Cc: Greg Kroah-Hartman, linux-serial, linux-arm-kernel, linux-kernel,
Nandor Han
In-Reply-To: <20170705130706.10388-2-romain.perier@collabora.com>
Hello,
On Wed, Jul 05, 2017 at 03:07:01PM +0200, Romain Perier wrote:
> From: Nandor Han <nandor.han@ge.com>
>
> Avoid the situation when `dma_is_rxing` could incorrectly signal that
> DMA RX channel is receiving data in case DMA preparation or sg mapping
> fails.
>
> This commit fixes the issues by moving the assignment of dma_is_rxing
> out of imx_disable_rx_int(), then the variable is set to 1 from
> start_rx_dma() only when the preparation is correctly done.
I'd write:
There are a few issues with setting dma_is_rxing to 1 in
imx_disable_rx_int:
- Currently always after imx_disable_rx_int() the function
start_rx_dma() is called. This dependency isn't obvious though.
- start_rx_dma() does error checking and might exit without enabling
DMA but keeping dma_is_rxing 1.
So the more natural place for setting dma_is_rxing to 1 is in
start_rx_dma after all errors are checked.
If you use this, there is nothing left of Nandor Han's patch and you can
drop his authorship.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* [PATCH v2 6/6] serial: imx: Fix imx_shutdown procedure
From: Romain Perier @ 2017-07-05 13:07 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: linux-serial, linux-arm-kernel, linux-kernel, Nandor Han,
Romain Perier
In-Reply-To: <20170705130706.10388-1-romain.perier@collabora.com>
From: Nandor Han <nandor.han@ge.com>
In some cases, it looks that interrupts can happen after the dma was
disabled and port was not yet shutdown. This will result in interrupts
handled by imx_rxint.
This commits updates the shutdown function to ensure that underlying
components are disabled in the right order. This disables RX and TX
blocks, then it disabled interrupts. In case DMA is enabled, it disables
DMA and free corresponding resources. It disables UART port and stop
clocks.
Signed-off-by: Nandor Han <nandor.han@ge.com>
Signed-off-by: Romain Perier <romain.perier@collabora.com>
---
drivers/tty/serial/imx.c | 34 +++++++++++++++-------------------
1 file changed, 15 insertions(+), 19 deletions(-)
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index d35112f..bfe7180 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -1396,44 +1396,40 @@ static void imx_shutdown(struct uart_port *port)
unsigned long temp;
unsigned long flags;
- if (sport->dma_is_enabled) {
- sport->dma_is_rxing = 0;
- sport->dma_is_txing = 0;
- dmaengine_terminate_sync(sport->dma_chan_tx);
- dmaengine_terminate_sync(sport->dma_chan_rx);
-
+ if (!sport->port.suspended) {
spin_lock_irqsave(&sport->port.lock, flags);
imx_stop_tx(port);
imx_stop_rx(port);
- imx_disable_dma(sport);
+
+ if (sport->dma_is_inited && sport->dma_is_enabled)
+ imx_disable_dma(sport);
+
spin_unlock_irqrestore(&sport->port.lock, flags);
imx_uart_dma_exit(sport);
}
- mctrl_gpio_disable_ms(sport->gpios);
-
spin_lock_irqsave(&sport->port.lock, flags);
temp = readl(sport->port.membase + UCR2);
- temp &= ~(UCR2_TXEN);
+ temp &= ~(UCR2_TXEN | UCR2_RXEN);
writel(temp, sport->port.membase + UCR2);
+ temp = readl(sport->port.membase + UCR4);
+ temp &= ~UCR4_OREN;
+ writel(temp, sport->port.membase + UCR4);
spin_unlock_irqrestore(&sport->port.lock, flags);
- /*
- * Stop our timer.
- */
- del_timer_sync(&sport->timer);
+ mctrl_gpio_disable_ms(sport->gpios);
- /*
- * Disable all interrupts, port and break condition.
- */
+ /* Stop our timer. */
+ del_timer_sync(&sport->timer);
+ /* Disable port. */
spin_lock_irqsave(&sport->port.lock, flags);
temp = readl(sport->port.membase + UCR1);
- temp &= ~(UCR1_TXMPTYEN | UCR1_RRDYEN | UCR1_RTSDEN | UCR1_UARTEN);
-
+ temp &= ~UCR1_UARTEN;
writel(temp, sport->port.membase + UCR1);
spin_unlock_irqrestore(&sport->port.lock, flags);
+ /* Disable clocks. */
clk_disable_unprepare(sport->clk_per);
clk_disable_unprepare(sport->clk_ipg);
}
--
1.8.3.1
^ permalink raw reply related
* [PATCH v2 5/6] serial: imx: update the stop rx,tx procedures
From: Romain Perier @ 2017-07-05 13:07 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: linux-serial, linux-arm-kernel, linux-kernel, Nandor Han,
Romain Perier
In-Reply-To: <20170705130706.10388-1-romain.perier@collabora.com>
From: Nandor Han <nandor.han@ge.com>
According to "Documentation/serial/driver" both procedures should stop
receiving or sending data. Based on this the procedures should stop the
activity regardless if DMA is enabled or not.
This commit updates both imx_stop_{rx|tx} procedures to stop the
activity and disable the interrupts related to that.
Signed-off-by: Nandor Han <nandor.han@ge.com>
Signed-off-by: Romain Perier <romain.perier@collabora.com>
---
drivers/tty/serial/imx.c | 36 +++++++++++++++++-------------------
1 file changed, 17 insertions(+), 19 deletions(-)
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 3112d88..d35112f 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -398,15 +398,14 @@ static void imx_stop_tx(struct uart_port *port)
struct imx_port *sport = (struct imx_port *)port;
unsigned long temp;
- /*
- * We are maybe in the SMP context, so if the DMA TX thread is running
- * on other cpu, we have to wait for it to finish.
- */
- if (sport->dma_is_enabled && sport->dma_is_txing)
- return;
+ sport->tx_bytes = 0;
+
+ if (sport->dma_is_enabled)
+ imx_stop_tx_dma(sport);
temp = readl(port->membase + UCR1);
- writel(temp & ~UCR1_TXMPTYEN, port->membase + UCR1);
+ temp &= ~(UCR1_TXMPTYEN | UCR1_TRDYEN | UCR1_RTSDEN);
+ writel(temp, port->membase + UCR1);
/* in rs485 mode disable transmitter if shifter is empty */
if (port->rs485.flags & SER_RS485_ENABLED &&
@@ -433,21 +432,20 @@ static void imx_stop_rx(struct uart_port *port)
struct imx_port *sport = (struct imx_port *)port;
unsigned long temp;
- if (sport->dma_is_enabled && sport->dma_is_rxing) {
- if (sport->port.suspended) {
- dmaengine_terminate_all(sport->dma_chan_rx);
- sport->dma_is_rxing = 0;
- } else {
- return;
- }
- }
+ if (sport->dma_is_enabled)
+ imx_stop_rx_dma(sport);
+
+ temp = readl(sport->port.membase + UCR1);
+ temp &= ~UCR1_RRDYEN;
+ writel(temp, sport->port.membase + UCR1);
temp = readl(sport->port.membase + UCR2);
- writel(temp & ~UCR2_RXEN, sport->port.membase + UCR2);
+ temp &= ~UCR2_ATEN;
+ writel(temp, sport->port.membase + UCR2);
- /* disable the `Receiver Ready Interrrupt` */
- temp = readl(sport->port.membase + UCR1);
- writel(temp & ~UCR1_RRDYEN, sport->port.membase + UCR1);
+ temp = readl(sport->port.membase + UCR3);
+ temp &= ~UCR3_AWAKEN;
+ writel(temp, sport->port.membase + UCR3);
}
/*
--
1.8.3.1
^ permalink raw reply related
* [PATCH v2 4/6] serial: imx: unmap sg buffers when DMA channel is released
From: Romain Perier @ 2017-07-05 13:07 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: linux-serial, linux-arm-kernel, linux-kernel, Nandor Han,
Romain Perier
In-Reply-To: <20170705130706.10388-1-romain.perier@collabora.com>
From: Nandor Han <nandor.han@ge.com>
This commits unmaps sg buffers when the DMA channel is released. It also
sets to zero `dma_is_rxing` and `dma_is_txing` to state that the
corresponding channels cannot transmit/receive data, as these are
disabled.
Signed-off-by: Nandor Han <nandor.han@ge.com>
Signed-off-by: Romain Perier <romain.perier@collabora.com>
---
drivers/tty/serial/imx.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index dd3ebb4..3112d88 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -367,6 +367,12 @@ static void imx_stop_tx_dma(struct imx_port *sport)
temp = readl(sport->port.membase + UCR1);
temp &= ~UCR1_TDMAEN;
writel(temp, sport->port.membase + UCR1);
+
+ if (sport->dma_is_txing) {
+ dma_unmap_sg(sport->port.dev, &sport->tx_sgl[0],
+ sport->dma_tx_nents, DMA_TO_DEVICE);
+ sport->dma_is_txing = 0;
+ }
}
static void imx_stop_rx_dma(struct imx_port *sport)
@@ -376,6 +382,12 @@ static void imx_stop_rx_dma(struct imx_port *sport)
temp = readl(sport->port.membase + UCR1);
temp &= ~(UCR1_RDMAEN | UCR1_ATDMAEN);
writel(temp, sport->port.membase + UCR1);
+
+ if (sport->dma_is_rxing) {
+ dma_unmap_sg(sport->port.dev, &sport->rx_sgl, 1,
+ DMA_FROM_DEVICE);
+ sport->dma_is_rxing = 0;
+ }
}
/*
--
1.8.3.1
^ permalink raw reply related
* [PATCH v2 3/6] serial: imx: remove CTSC and CTS handling
From: Romain Perier @ 2017-07-05 13:07 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: linux-serial, linux-arm-kernel, linux-kernel, Nandor Han,
Romain Perier
In-Reply-To: <20170705130706.10388-1-romain.perier@collabora.com>
From: Nandor Han <nandor.han@ge.com>
CTSC and CTS are not related to DMA and might add
disruption in some cases.
Signed-off-by: Romain Perier <romain.perier@collabora.com>
---
drivers/tty/serial/imx.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 5291b86..dd3ebb4 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -1249,11 +1249,6 @@ static void imx_disable_dma(struct imx_port *sport)
imx_stop_rx_dma(sport);
imx_stop_tx_dma(sport);
- /* clear UCR2 */
- temp = readl(sport->port.membase + UCR2);
- temp &= ~(UCR2_CTSC | UCR2_CTS | UCR2_ATEN);
- writel(temp, sport->port.membase + UCR2);
-
imx_setup_ufcr(sport, TXTL_DEFAULT, RXTL_DEFAULT);
sport->dma_is_enabled = 0;
--
1.8.3.1
^ permalink raw reply related
* [PATCH v2 2/6] serial: imx: Simplify DMA disablement
From: Romain Perier @ 2017-07-05 13:07 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: linux-serial, linux-arm-kernel, linux-kernel, Nandor Han,
Romain Perier
In-Reply-To: <20170705130706.10388-1-romain.perier@collabora.com>
From: Nandor Han <nandor.han@ge.com>
This commits simplify the function imx_disable_dma() by moving
the code for disabling RX and TX DMAs to dedicated functions.
This is a preparation for the next commit.
Signed-off-by: Nandor Han <nandor.han@ge.com>
Signed-off-by: Romain Perier <romain.perier@collabora.com>
---
drivers/tty/serial/imx.c | 26 ++++++++++++++++++++------
1 file changed, 20 insertions(+), 6 deletions(-)
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 01df430..5291b86 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -360,6 +360,24 @@ static void imx_port_rts_auto(struct imx_port *sport, unsigned long *ucr2)
*ucr2 |= UCR2_CTSC;
}
+static void imx_stop_tx_dma(struct imx_port *sport)
+{
+ unsigned long temp;
+
+ temp = readl(sport->port.membase + UCR1);
+ temp &= ~UCR1_TDMAEN;
+ writel(temp, sport->port.membase + UCR1);
+}
+
+static void imx_stop_rx_dma(struct imx_port *sport)
+{
+ unsigned long temp;
+
+ temp = readl(sport->port.membase + UCR1);
+ temp &= ~(UCR1_RDMAEN | UCR1_ATDMAEN);
+ writel(temp, sport->port.membase + UCR1);
+}
+
/*
* interrupts disabled on entry
*/
@@ -1228,12 +1246,8 @@ static void imx_enable_dma(struct imx_port *sport)
static void imx_disable_dma(struct imx_port *sport)
{
- unsigned long temp;
-
- /* clear UCR1 */
- temp = readl(sport->port.membase + UCR1);
- temp &= ~(UCR1_RDMAEN | UCR1_TDMAEN | UCR1_ATDMAEN);
- writel(temp, sport->port.membase + UCR1);
+ imx_stop_rx_dma(sport);
+ imx_stop_tx_dma(sport);
/* clear UCR2 */
temp = readl(sport->port.membase + UCR2);
--
1.8.3.1
^ permalink raw reply related
* [PATCH v2 1/6] serial: imx: only set DMA rx-ing when DMA starts
From: Romain Perier @ 2017-07-05 13:07 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: linux-serial, linux-arm-kernel, linux-kernel, Nandor Han,
Romain Perier
In-Reply-To: <20170705130706.10388-1-romain.perier@collabora.com>
From: Nandor Han <nandor.han@ge.com>
Avoid the situation when `dma_is_rxing` could incorrectly signal that
DMA RX channel is receiving data in case DMA preparation or sg mapping
fails.
This commit fixes the issues by moving the assignment of dma_is_rxing
out of imx_disable_rx_int(), then the variable is set to 1 from
start_rx_dma() only when the preparation is correctly done.
Signed-off-by: Nandor Han <nandor.han@ge.com>
Signed-off-by: Romain Perier <romain.perier@collabora.com>
---
drivers/tty/serial/imx.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 3bae4ea..01df430 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -729,8 +729,6 @@ static void imx_disable_rx_int(struct imx_port *sport)
{
unsigned long temp;
- sport->dma_is_rxing = 1;
-
/* disable the receiver ready and aging timer interrupts */
temp = readl(sport->port.membase + UCR1);
temp &= ~(UCR1_RRDYEN);
@@ -1083,6 +1081,7 @@ static int start_rx_dma(struct imx_port *sport)
desc->callback_param = sport;
dev_dbg(dev, "RX: prepare for the DMA.\n");
+ sport->dma_is_rxing = 1;
sport->rx_cookie = dmaengine_submit(desc);
dma_async_issue_pending(chan);
return 0;
--
1.8.3.1
^ permalink raw reply related
* [PATCH v2 0/6] serial: imx: various improvements
From: Romain Perier @ 2017-07-05 13:07 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: linux-serial, linux-arm-kernel, linux-kernel, Romain Perier
During shutdown when a userspace service is disabled (which generates
an uart close), we got kernel crashes in the imx serial driver :
[ 1257.657423] Unhandled fault: external abort on non-linefetch (0x1008) at 0xf0938000
[ 1257.665122] pgd = ecf20000
[ 1257.667838] [f0938000] *pgd=de819811, *pte=53fc0653, *ppte=53fc0453
[ 1257.674179] Internal error: : 1008 [#1] SMP ARM
[ 1257.678722] Modules linked in:
[ 1257.681807] CPU: 0 PID: 3850 Comm: emerald_acq Not tainted 4.8.0 #10
[ 1257.688168] Hardware name: Freescale i.MX53 (Device Tree Support)
[ 1257.694269] task: e5c48000 task.stack: ed0b4000
[ 1257.698827] PC is at imx_rxint+0x5c/0x228
[ 1257.702859] LR is at lock_acquired+0x494/0x57c
[ 1257.707312] pc : [<80484884>] lr : [<80173aa0>] psr: 20070193
[ 1257.707312] sp : ed0b5c60 ip : ed0b5be8 fp : ed0b5c9c
[ 1257.718795] r10: 00000000 r9 : 00000000 r8 : 00000004
[ 1257.724027] r7 : 00000030 r6 : ee83e258 r5 : 00000000 r4 : ee09f410
[ 1257.730561] r3 : 0015c30c r2 : f0938000 r1 : 00000135 r0 : 40070193
[ 1257.737099] Flags: nzCv IRQs off FIQs on Mode SVC_32 ISA ARM Segment none
[ 1257.744327] Control: 10c5387d Table: dcf20019 DAC: 00000051
[ 1257.750080] Process emerald_acq (pid: 3850, stack limit = 0xed0b4210)
[ 1257.756527] Stack: (0xed0b5c60 to 0xed0b6000)
[ 1257.760898] 5c60: ed0b5cf4 40070193 80175f64 80faf384 e5c484c0 ee09f410 00007240 00005099
[ 1257.769087] 5c80: 00000030 00000030 80e025c4 00000000 ed0b5cf4 ed0b5ca0 80485b18 80484834
[ 1257.777275] 5ca0: 8012fe26 00000000 00000000 60070193 ee81b210 00000001 ed0b5cdc ed0b5cc8
[ 1257.785463] 5cc0: 80171d28 80171c3c 80e2d634 ee096740 ee81b200 ee81b210 00000001 00000030
[ 1257.793651] 5ce0: 80e025c4 00000000 ed0b5d34 ed0b5cf8 80182f10 804857fc e5c484c0 00000002
[ 1257.801839] 5d00: ee81b200 ed0b5d3c 60070193 ee81b200 ee81b200 ee81b210 00000001 ee81c400
[ 1257.810027] 5d20: 00000001 00000008 ed0b5d54 ed0b5d38 801832d8 80182ed0 808f4b60 00000000
[ 1257.818216] 5d40: ee81b200 ee81b260 ed0b5d74 ed0b5d58 8018335c 801832b8 ee81b200 ee81b260
[ 1257.826404] 5d60: ee81b210 00000001 ed0b5d94 ed0b5d78 80186ba0 80183320 80debe8c 00000030
[ 1257.834592] 5d80: 00000000 00000001 ed0b5da4 ed0b5d98 80182420 80186af4 ed0b5dcc ed0b5da8
[ 1257.842780] 5da0: 801827a0 801823fc 00000000 80e8350c 00000020 00000001 ed0b5df8 00000001
[ 1257.850968] 5dc0: ed0b5df4 ed0b5dd0 80101530 8018274c 808f4bec 20070013 ffffffff ed0b5e2c
[ 1257.859156] 5de0: ee09f410 ed0b4000 ed0b5e5c ed0b5df8 808f55f0 801014c8 00000001 00000130
[ 1257.867345] 5e00: 00000000 e5c48000 60070013 ee09f410 00000000 60070013 ee09f410 ed06a640
[ 1257.875533] 5e20: 00000008 ed0b5e5c ed0b5df0 ed0b5e48 801756a8 808f4bec 20070013 ffffffff
[ 1257.883722] 5e40: 00000051 7f000000 ee09f410 00000b01 ed0b5e7c ed0b5e60 80485d74 808f4bb4
[ 1257.891912] 5e60: ee83e258 ee09f410 ee83e3a4 80e2d634 ed0b5ea4 ed0b5e80 8047f514 80485c70
[ 1257.900100] 5e80: ee83e258 ed375000 ee09f410 ee83e310 ee83e3ac ed06a640 ed0b5ecc ed0b5ea8
[ 1257.908288] 5ea0: 80481304 8047f400 ed375000 eeabce60 00000000 ee7973e8 00000000 ed06a640
[ 1257.916477] 5ec0: ed0b5f14 ed0b5ed0 80462fe0 804811ac 00000008 eeabce60 00000001 00000001
[ 1257.924665] 5ee0: 00000000 802713dc ed0b5f54 ed06a640 eeabce60 ee2c6910 ee7973e8 00000000
[ 1257.932854] 5f00: eeabce60 00000008 ed0b5f54 ed0b5f18 80271404 80462ee8 00000000 00000000
[ 1257.941044] 5f20: ed06c640 ed06a648 ed0b5f4c e5c48400 00000000 80e84054 e5c48440 e5c48000
[ 1257.949232] 5f40: 00000000 00000000 ed0b5f64 ed0b5f58 802715c4 80271378 ed0b5f8c ed0b5f68
[ 1257.957420] 5f60: 80146034 802715b8 00000000 ed0b4000 ed0b5fb0 801086c4 801086c4 ed0b4000
[ 1257.965610] 5f80: ed0b5fac ed0b5f90 8010cc68 80145f78 0054756c 00000000 767474b4 00000006
[ 1257.973798] 5fa0: 00000000 ed0b5fb0 80108548 8010cbc4 00000000 76f2a084 00000002 00000000
[ 1257.981986] 5fc0: 0054756c 00000000 767474b4 00000006 0225e880 00000000 76f36000 7e836d34
[ 1257.990175] 5fe0: 00000000 7e836d10 76f2a4c0 76a5db68 80070010 00000062 00000000 00000000
[ 1257.998357] Backtrace:
[ 1258.000837] [<80484828>] (imx_rxint) from [<80485b18>] (imx_int+0x328/0x474)
[ 1258.007892] r10:00000000 r9:80e025c4 r8:00000030 r7:00000030 r6:00005099 r5:00007240
[ 1258.015815] r4:ee09f410
[ 1258.018386] [<804857f0>] (imx_int) from [<80182f10>] (__handle_irq_event_percpu+0x4c/0x3e8)
[ 1258.026742] r10:00000000 r9:80e025c4 r8:00000030 r7:00000001 r6:ee81b210 r5:ee81b200
[ 1258.034664] r4:ee096740
[ 1258.037226] [<80182ec4>] (__handle_irq_event_percpu) from [<801832d8>] (handle_irq_event_percpu+0x2c/0x68)
[ 1258.046885] r10:00000008 r9:00000001 r8:ee81c400 r7:00000001 r6:ee81b210 r5:ee81b200
[ 1258.054806] r4:ee81b200
[ 1258.057369] [<801832ac>] (handle_irq_event_percpu) from [<8018335c>] (handle_irq_event+0x48/0x6c)
[ 1258.066246] r5:ee81b260 r4:ee81b200
[ 1258.069866] [<80183314>] (handle_irq_event) from [<80186ba0>] (handle_level_irq+0xb8/0x154)
[ 1258.078222] r7:00000001 r6:ee81b210 r5:ee81b260 r4:ee81b200
[ 1258.083961] [<80186ae8>] (handle_level_irq) from [<80182420>] (generic_handle_irq+0x30/0x44)
[ 1258.092404] r7:00000001 r6:00000000 r5:00000030 r4:80debe8c
[ 1258.098143] [<801823f0>] (generic_handle_irq) from [<801827a0>] (__handle_domain_irq+0x60/0xc8)
[ 1258.106854] [<80182740>] (__handle_domain_irq) from [<80101530>] (tzic_handle_irq+0x74/0x9c)
[ 1258.115297] r9:00000001 r8:ed0b5df8 r7:00000001 r6:00000020 r5:80e8350c r4:00000000
[ 1258.123139] [<801014bc>] (tzic_handle_irq) from [<808f55f0>] (__irq_svc+0x70/0x98)
[ 1258.130715] Exception stack(0xed0b5df8 to 0xed0b5e40)
[ 1258.135773] 5de0: 00000001 00000130
[ 1258.143962] 5e00: 00000000 e5c48000 60070013 ee09f410 00000000 60070013 ee09f410 ed06a640
[ 1258.152151] 5e20: 00000008 ed0b5e5c ed0b5df0 ed0b5e48 801756a8 808f4bec 20070013 ffffffff
[ 1258.160333] r9:ed0b4000 r8:ee09f410 r7:ed0b5e2c r6:ffffffff r5:20070013 r4:808f4bec
[ 1258.168188] [<808f4ba8>] (_raw_spin_unlock_irqrestore) from [<80485d74>] (imx_shutdown+0x110/0x214)
[ 1258.177239] r5:00000b01 r4:ee09f410
[ 1258.180860] [<80485c64>] (imx_shutdown) from [<8047f514>] (uart_shutdown+0x120/0x17c)
[ 1258.188695] r7:80e2d634 r6:ee83e3a4 r5:ee09f410 r4:ee83e258
[ 1258.194434] [<8047f3f4>] (uart_shutdown) from [<80481304>] (uart_close+0x164/0x254)
[ 1258.202096] r9:ed06a640 r8:ee83e3ac r7:ee83e310 r6:ee09f410 r5:ed375000 r4:ee83e258
[ 1258.209948] [<804811a0>] (uart_close) from [<80462fe0>] (tty_release+0x104/0x498)
[ 1258.217439] r9:ed06a640 r8:00000000 r7:ee7973e8 r6:00000000 r5:eeabce60 r4:ed375000
[ 1258.225285] [<80462edc>] (tty_release) from [<80271404>] (__fput+0x98/0x1e8)
[ 1258.232339] r10:00000008 r9:eeabce60 r8:00000000 r7:ee7973e8 r6:ee2c6910 r5:eeabce60
[ 1258.240261] r4:ed06a640
[ 1258.242823] [<8027136c>] (__fput) from [<802715c4>] (____fput+0x18/0x1c)
[ 1258.249528] r10:00000000 r9:00000000 r8:e5c48000 r7:e5c48440 r6:80e84054 r5:00000000
[ 1258.257450] r4:e5c48400
[ 1258.260021] [<802715ac>] (____fput) from [<80146034>] (task_work_run+0xc8/0xf8)
[ 1258.267352] [<80145f6c>] (task_work_run) from [<8010cc68>] (do_work_pending+0xb0/0xd0)
[ 1258.275274] r9:ed0b4000 r8:801086c4 r7:801086c4 r6:ed0b5fb0 r5:ed0b4000 r4:00000000
[ 1258.283116] [<8010cbb8>] (do_work_pending) from [<80108548>] (slow_work_pending+0xc/0x20)
[ 1258.291298] r7:00000006 r6:767474b4 r5:00000000 r4:0054756c
[ 1258.297037] Code: e5943094 e594202c e2833001 e5843094 (e592a000)
[ 1258.303148] ---[ end trace 7a50198148a54c4d ]---
[ 1258.307776] Kernel panic - not syncing: Fatal exception in interrupt
[ 1258.314160] ---[ end Kernel panic - not syncing: Fatal exception in interrupt
After investigations, we found several issues:
It looks that interrupts can happen after the dma was disabled and
port was not yet shutdown. This will result in interrupts handled by
imx_rxint. Analyzed the imx_shutdown function, and found that:
- Some interrupts were not disabled during shutdown (AWAKEN, UCR4_OREN,
UCR1_TRDYEN)
- TX was not stopped in all situations (if dma is enabled and
transmitting)
- Using deprecated dmaengine_terminate_all method.
- Trying to close DMA channels several times.
This set of patches proposes fix regarding these issues and problems we
found during debugging.
Changes in v2:
- Re-worked assignment of `dma_is_rxing` in patch 1/7
- Removed commit "serial: imx: move log from error to debug type"
- Removed commit "serial: imx: init dma_is_{rx|tx}ing variables"
- Improved commit log for "serial: imx: Simplify DMA disablement"
and splitted it in two commits, with one for CTSC and CTS
- Added commit "serial: imx: remove CTSC and CTS handling"
- Fixed typo in commit "unmap sg buffers when DMA channel is released"
- Re-worded commit log for "serial: imx: update the stop rx,tx procedures" and
removed forward declaration
- Fixed typo in commit "serial: imx: Fix imx_shutdown procedure" and simplified
locking
Nandor Han (6):
serial: imx: only set DMA rx-ing when DMA starts
serial: imx: Simplify DMA disablement
serial: imx: remove CTSC and CTS handling
serial: imx: unmap sg buffers when DMA channel is released
serial: imx: update the stop rx,tx procedures
serial: imx: Fix imx_shutdown procedure
drivers/tty/serial/imx.c | 116 ++++++++++++++++++++++++++---------------------
1 file changed, 65 insertions(+), 51 deletions(-)
--
1.8.3.1
^ permalink raw reply
* Re: [PATCH 3/7] serial: imx: init dma_is_{rx|tx}ing variables
From: Uwe Kleine-König @ 2017-07-05 11:58 UTC (permalink / raw)
To: Romain Perier
Cc: Greg Kroah-Hartman, linux-kernel, linux-serial, Nandor Han,
linux-arm-kernel, Lothar Waßmann
In-Reply-To: <91cc9a48-58e9-98a9-58aa-1a949b9b5552@collabora.com>
Hello Romain,
On Wed, Jul 05, 2017 at 12:14:57PM +0200, Romain Perier wrote:
> Le 03/07/2017 à 08:52, Uwe Kleine-König a écrit :
> > On Fri, Jun 30, 2017 at 02:13:29PM +0200, Lothar Waßmann wrote:
> >> On Fri, 30 Jun 2017 14:04:42 +0200 Romain Perier wrote:
> >>> From: Nandor Han <nandor.han@ge.com>
> >>>
> >>> Initialize both dma_is_{rx|tx}ing variables when DMA is enabled to avoid
> >>> checking uninitialized variables if port shutdown is requested before
> >>> DMA channels get a chance to start.
> >>>
> >>> Signed-off-by: Nandor Han <nandor.han@ge.com>
> >>> Signed-off-by: Romain Perier <romain.perier@collabora.com>
> >>> ---
> >>> drivers/tty/serial/imx.c | 3 +++
> >>> 1 file changed, 3 insertions(+)
> >>>
> >>> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> >>> index 188063d..81fb413 100644
> >>> --- a/drivers/tty/serial/imx.c
> >>> +++ b/drivers/tty/serial/imx.c
> >>> @@ -1225,6 +1225,9 @@ static void imx_enable_dma(struct imx_port *sport)
> >>>
> >>> imx_setup_ufcr(sport, TXTL_DMA, RXTL_DMA);
> >>>
> >>> + sport->dma_is_rxing = 0;
> >>> + sport->dma_is_txing = 0;
> >>> +
> >>> sport->dma_is_enabled = 1;
> >>> }
> >>>
> >> sport is devm_kzalloc()ed, so the variables are initialized to 0 anyway.
> > I'd agree to Lothar's statement. Did you find this issue by inspection,
> > or does it fix a compiler warning? Do you think there is an actual
> > problem?
> >
> > Best regards
> > Uwe
> >
> What does happen if the UART port is shutdown and then re-enabled ? I
> don't think that kzalloc will work in this case
imx_shutdown has:
if (sport->dma_is_enabled) {
sport->dma_is_rxing = 0;
sport->dma_is_txing = 0;
which might be good enough. Can dma_is_[rt]xing be != 0 if
dma_is_enabled is false? It seems it cannot, the only place where
dma_is_enabled is set to 0 (apart from the kzalloc where dma_is_[rt]xing
is set to zero) is imx_disable_dma(). The only caller sets both
dma_is_[rt]xing to zero before.
So this patch should be dropped or its commit log improved to point out
the actual problem.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* Re: [PATCH 3/7] serial: imx: init dma_is_{rx|tx}ing variables
From: Romain Perier @ 2017-07-05 10:14 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Lothar Waßmann, Greg Kroah-Hartman, Nandor Han,
linux-arm-kernel, linux-serial, linux-kernel
In-Reply-To: <20170703065221.imbpbybsrd3of4ec@pengutronix.de>
Hello,
Le 03/07/2017 à 08:52, Uwe Kleine-König a écrit :
> On Fri, Jun 30, 2017 at 02:13:29PM +0200, Lothar Waßmann wrote:
>> Hi,
>>
>> On Fri, 30 Jun 2017 14:04:42 +0200 Romain Perier wrote:
>>> From: Nandor Han <nandor.han@ge.com>
>>>
>>> Initialize both dma_is_{rx|tx}ing variables when DMA is enabled to avoid
>>> checking uninitialized variables if port shutdown is requested before
>>> DMA channels get a chance to start.
>>>
>>> Signed-off-by: Nandor Han <nandor.han@ge.com>
>>> Signed-off-by: Romain Perier <romain.perier@collabora.com>
>>> ---
>>> drivers/tty/serial/imx.c | 3 +++
>>> 1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
>>> index 188063d..81fb413 100644
>>> --- a/drivers/tty/serial/imx.c
>>> +++ b/drivers/tty/serial/imx.c
>>> @@ -1225,6 +1225,9 @@ static void imx_enable_dma(struct imx_port *sport)
>>>
>>> imx_setup_ufcr(sport, TXTL_DMA, RXTL_DMA);
>>>
>>> + sport->dma_is_rxing = 0;
>>> + sport->dma_is_txing = 0;
>>> +
>>> sport->dma_is_enabled = 1;
>>> }
>>>
>> sport is devm_kzalloc()ed, so the variables are initialized to 0 anyway.
> I'd agree to Lothar's statement. Did you find this issue by inspection,
> or does it fix a compiler warning? Do you think there is an actual
> problem?
>
> Best regards
> Uwe
>
What does happen if the UART port is shutdown and then re-enabled ? I
don't think that kzalloc will work in this case
Regards,
Romain
^ permalink raw reply
* Re: [PATCH] serial: imx-serial - move DMA buffer configuration to DT
From: Greg Kroah-Hartman @ 2017-07-04 9:15 UTC (permalink / raw)
To: Romain Perier
Cc: Uwe Kleine-König, Jiri Slaby,
linux-serial-u79uwXL29TY76Z2rM5mHXA, Nandor Han,
devicetree-u79uwXL29TY76Z2rM5mHXA, kernel-bIcnvbaLZ9MEGnE8C9+IrQ
In-Reply-To: <4d222fcd-184d-92e6-7c9f-370614c23d38-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
On Tue, Jul 04, 2017 at 10:15:23AM +0200, Romain Perier wrote:
> Hello,
>
>
> Le 04/07/2017 à 09:41, Greg Kroah-Hartman a écrit :
> > On Mon, Jul 03, 2017 at 08:17:38PM +0200, Uwe Kleine-König wrote:
> >> Hello Greg,
> > Please don't top-post :(
> >
> >> I'm a bit disappointed that you didn't drop this patch. At the time I
> >> sent my review it was still in your tty-testing branch and now it's part
> >> of your pull request for 4.13-rc1 (as commit
> >> a3015affdf76ef279fbbb3710a220bab7e9ea04b). :-|
> > And now it's in Linus's tree! :)
> >
> >> I'm still convinced that this patch is wrong in its current form.
> > Should I revert it, or can I get a fix for it? Reviewing the patch
> > when it was submitted would have been best, not after it ends up in one
> > of my trees...
>
> I don't think there is a fix for it. Either you revert the patch and you
> code the fix differently, as I suggested by changing dma_rx_callback...
> but that's more intrusive... or you keep this patch.
I'll revert it after 4.13-rc1 is out, unless there are major objections
to that...
thanks,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 1/7] serial: imx: only set DMA rx-ing when DMA starts
From: Romain Perier @ 2017-07-04 8:55 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Greg Kroah-Hartman, Nandor Han, linux-arm-kernel, linux-serial,
linux-kernel
In-Reply-To: <20170703064840.pttfudcv7xsnbki3@pengutronix.de>
Hello,
Le 03/07/2017 à 08:48, Uwe Kleine-König a écrit :
> Hello,
>
> On Fri, Jun 30, 2017 at 02:04:40PM +0200, Romain Perier wrote:
>> From: Nandor Han <nandor.han@ge.com>
>>
>> Avoid the situation when `dma_is_rxing` could incorrectly signal that
>> DMA RX channel is receiving data in case DMA preparation or sg mapping
>> fails.
> Just from reading the commit log and the patch I didn't understand the
> problem that is supposed to be fixed here. After looking at it for some
> I understood. I don't suggest a new commit log here as it has to look
> different if you pick up my comment below.
>
>> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
>> index 5437b34..1d35293 100644
>> --- a/drivers/tty/serial/imx.c
>> +++ b/drivers/tty/serial/imx.c
>> @@ -725,11 +725,11 @@ static irqreturn_t imx_rxint(int irq, void *dev_id)
>> return IRQ_HANDLED;
>> }
>>
>> -static void imx_disable_rx_int(struct imx_port *sport)
>> +static void imx_disable_rx_int(struct imx_port *sport, bool is_rxing)
>> {
>> unsigned long temp;
>>
>> - sport->dma_is_rxing = 1;
>> + sport->dma_is_rxing = is_rxing;
>>
>> /* disable the receiver ready and aging timer interrupts */
>> temp = readl(sport->port.membase + UCR1);
>> @@ -762,7 +762,7 @@ static void imx_dma_rxint(struct imx_port *sport)
>> temp = readl(sport->port.membase + USR2);
>> if ((temp & USR2_RDR) && !sport->dma_is_rxing) {
>>
>> - imx_disable_rx_int(sport);
>> + imx_disable_rx_int(sport, false);
>>
>> /* tell the DMA to receive the data. */
>> start_rx_dma(sport);
>> @@ -1083,6 +1083,7 @@ static int start_rx_dma(struct imx_port *sport)
>> desc->callback_param = sport;
>>
>> dev_dbg(dev, "RX: prepare for the DMA.\n");
>> + sport->dma_is_rxing = 1;
>> sport->rx_cookie = dmaengine_submit(desc);
>> dma_async_issue_pending(chan);
>> return 0;
>> @@ -1362,7 +1363,7 @@ static int imx_startup(struct uart_port *port)
>> spin_unlock(&tty->files_lock);
>>
>> if (readcnt > 0) {
>> - imx_disable_rx_int(sport);
>> + imx_disable_rx_int(sport, true);
> I wonder if it would be saner to not assign to dma_is_rxing in
> imx_disable_rx_int() at all but do this only in start_rx_dma() and
> imx_dma_rxint().
It was "mostly" done, in 4.11 in fact. dma_is_rxing was set to 0 from
imx_dma_rxint() and was not set
at all from start_rx_dma(), resulting to the same issue. We could move
assignment of dma_is_rxing out of imx_disable_rx_int()
and set it to one from start_rx_dma() only. What do you think ?
Best Regards,
Romain
>
> Best regards
> Uwe
>
^ permalink raw reply
* Re: [PATCH] serial: imx-serial - move DMA buffer configuration to DT
From: Uwe Kleine-König @ 2017-07-04 8:36 UTC (permalink / raw)
To: Romain Perier
Cc: Greg Kroah-Hartman, Jiri Slaby,
linux-serial-u79uwXL29TY76Z2rM5mHXA, Nandor Han,
devicetree-u79uwXL29TY76Z2rM5mHXA, kernel-bIcnvbaLZ9MEGnE8C9+IrQ
In-Reply-To: <e1601aa2-6eaf-7212-369f-9c67a3159f64-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
On Tue, Jul 04, 2017 at 09:01:24AM +0200, Romain Perier wrote:
> Le 03/07/2017 à 20:17, Uwe Kleine-König a écrit :
> > On Thu, Jun 29, 2017 at 08:26:18PM +0200, Uwe Kleine-König wrote:
> >> On Wed, Jun 28, 2017 at 12:15:14PM +0200, Romain Perier wrote:
> >>> From: Nandor Han <nandor.han-JJi787mZWgc@public.gmane.org>
> >>>
> >>> The size of the DMA buffer can affect the delta time between data being
> >>> produced and data being consumed. Basically the DMA system will move
> >>> data to tty buffer when a) DMA buffer is full b) serial line is idle.
> >>> The situation is visible when producer generates data continuously and
> >>> there is no possibility for idle line. At this point the DMA buffer is
> >>> directly affecting the delta time.
> >> This doesn't look like a hw property but a configuration item. Also I
> >> don't understand the problematic case. The i.MX is sending continuously
> >> and then doesn't receive bytes until the DMA buffer is full? What is the
> >> DMA buffer? You don't mean the FIFO here, do you?
>
> by DMA buffer, I mean the size of the RX FIFO. Data will be moved to tty
> when the FIFO
> is full or the serial line is idle. Now suppose you have an important
> work load and the serial line is never idle,
That means you're waiting for urgent data that already sits in the
hardware's RX fifo but as you send in parallel the hardware is busy and
doesn't process the RX fifo?
Would it help to decrease UFCR.RXTL? Or is this a HW bug that the RX
fifo isn't considered idle when you send much?
Or is this a software problem and the urgent data sits somewhere in the
tty or dma layer already?
> with the current size of the DMA buffer (PAGE_SIZE) you might introduce
> latencies between data being produced and data being consumed.
Making DMA buffers smaller doesn't sound like the right approach to
minimize delays. If you choose to do so you increase the number of irqs
and so this is a user policy what you want and shouldn't fixed in the
oftree.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] serial: imx-serial - move DMA buffer configuration to DT
From: Romain Perier @ 2017-07-04 8:15 UTC (permalink / raw)
To: Greg Kroah-Hartman, Uwe Kleine-König
Cc: Jiri Slaby, linux-serial-u79uwXL29TY76Z2rM5mHXA, Nandor Han,
devicetree-u79uwXL29TY76Z2rM5mHXA, kernel-bIcnvbaLZ9MEGnE8C9+IrQ
In-Reply-To: <20170704074159.GA21747-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org>
Hello,
Le 04/07/2017 à 09:41, Greg Kroah-Hartman a écrit :
> On Mon, Jul 03, 2017 at 08:17:38PM +0200, Uwe Kleine-König wrote:
>> Hello Greg,
> Please don't top-post :(
>
>> I'm a bit disappointed that you didn't drop this patch. At the time I
>> sent my review it was still in your tty-testing branch and now it's part
>> of your pull request for 4.13-rc1 (as commit
>> a3015affdf76ef279fbbb3710a220bab7e9ea04b). :-|
> And now it's in Linus's tree! :)
>
>> I'm still convinced that this patch is wrong in its current form.
> Should I revert it, or can I get a fix for it? Reviewing the patch
> when it was submitted would have been best, not after it ends up in one
> of my trees...
I don't think there is a fix for it. Either you revert the patch and you
code the fix differently, as I suggested by changing dma_rx_callback...
but that's more intrusive... or you keep this patch.
>
> thanks,
>
> greg k-h
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] serial: imx-serial - move DMA buffer configuration to DT
From: Uwe Kleine-König @ 2017-07-04 8:13 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Romain Perier, Jiri Slaby, linux-serial-u79uwXL29TY76Z2rM5mHXA,
Nandor Han, devicetree-u79uwXL29TY76Z2rM5mHXA,
kernel-bIcnvbaLZ9MEGnE8C9+IrQ
In-Reply-To: <20170704074159.GA21747-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org>
On Tue, Jul 04, 2017 at 09:41:59AM +0200, Greg Kroah-Hartman wrote:
> On Mon, Jul 03, 2017 at 08:17:38PM +0200, Uwe Kleine-König wrote:
> > Hello Greg,
>
> Please don't top-post :(
I only kept the mail for reference, as I didn't reply to specific stuff
in the mail I consider writing my stuff at the top of the mail ok (and
even better than below the mail because you don't need to scroll down to
see what I wrote and I think I gave enough context).
> > I'm a bit disappointed that you didn't drop this patch. At the time I
> > sent my review it was still in your tty-testing branch and now it's part
> > of your pull request for 4.13-rc1 (as commit
> > a3015affdf76ef279fbbb3710a220bab7e9ea04b). :-|
>
> And now it's in Linus's tree! :)
Should I better answer to the pull request mail next time?
> > I'm still convinced that this patch is wrong in its current form.
>
> Should I revert it, or can I get a fix for it?
I'd say revert for now. At least it's not clear to me how it should be
fixed as the property added to dt shouldn't be there (given my
current understanding of the problem).
> Reviewing the patch when it was submitted would have been best, not
> after it ends up in one of my trees...
Well, I reviewed in less than 24h. I now that Linux development is
quick, but please don't make it that quick.
Otherwise I might consider a procmail rule that sends a "please don't
apply yet" mail until I come around looking at imx-uart patches. :-)
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] serial: imx-serial - move DMA buffer configuration to DT
From: Greg Kroah-Hartman @ 2017-07-04 7:41 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Romain Perier, Jiri Slaby, linux-serial-u79uwXL29TY76Z2rM5mHXA,
Nandor Han, devicetree-u79uwXL29TY76Z2rM5mHXA,
kernel-bIcnvbaLZ9MEGnE8C9+IrQ
In-Reply-To: <20170703181738.xyxtwe246cfrkyd7-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
On Mon, Jul 03, 2017 at 08:17:38PM +0200, Uwe Kleine-König wrote:
> Hello Greg,
Please don't top-post :(
> I'm a bit disappointed that you didn't drop this patch. At the time I
> sent my review it was still in your tty-testing branch and now it's part
> of your pull request for 4.13-rc1 (as commit
> a3015affdf76ef279fbbb3710a220bab7e9ea04b). :-|
And now it's in Linus's tree! :)
> I'm still convinced that this patch is wrong in its current form.
Should I revert it, or can I get a fix for it? Reviewing the patch
when it was submitted would have been best, not after it ends up in one
of my trees...
thanks,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] serial: imx-serial - move DMA buffer configuration to DT
From: Romain Perier @ 2017-07-04 7:01 UTC (permalink / raw)
To: Uwe Kleine-König, Greg Kroah-Hartman
Cc: Jiri Slaby, linux-serial-u79uwXL29TY76Z2rM5mHXA, Nandor Han,
devicetree-u79uwXL29TY76Z2rM5mHXA, kernel-bIcnvbaLZ9MEGnE8C9+IrQ
In-Reply-To: <20170703181738.xyxtwe246cfrkyd7-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Hello,
Le 03/07/2017 à 20:17, Uwe Kleine-König a écrit :
> Hello Greg,
>
> I'm a bit disappointed that you didn't drop this patch. At the time I
> sent my review it was still in your tty-testing branch and now it's part
> of your pull request for 4.13-rc1 (as commit
> a3015affdf76ef279fbbb3710a220bab7e9ea04b). :-|
>
> I'm still convinced that this patch is wrong in its current form.
>
> Best regards
> Uwe
>
> On Thu, Jun 29, 2017 at 08:26:18PM +0200, Uwe Kleine-König wrote:
>> Cc: += devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>>
>> On Wed, Jun 28, 2017 at 12:15:14PM +0200, Romain Perier wrote:
>>> From: Nandor Han <nandor.han-JJi787mZWgc@public.gmane.org>
>>>
>>> The size of the DMA buffer can affect the delta time between data being
>>> produced and data being consumed. Basically the DMA system will move
>>> data to tty buffer when a) DMA buffer is full b) serial line is idle.
>>> The situation is visible when producer generates data continuously and
>>> there is no possibility for idle line. At this point the DMA buffer is
>>> directly affecting the delta time.
>> This doesn't look like a hw property but a configuration item. Also I
>> don't understand the problematic case. The i.MX is sending continuously
>> and then doesn't receive bytes until the DMA buffer is full? What is the
>> DMA buffer? You don't mean the FIFO here, do you?
by DMA buffer, I mean the size of the RX FIFO. Data will be moved to tty
when the FIFO
is full or the serial line is idle. Now suppose you have an important
work load and the serial line is never idle,
with the current size of the DMA buffer (PAGE_SIZE) you might introduce
latencies between data being produced and data being consumed.
We could also modify dma_rx_callback(), but this current solution is
less intrusive, imho.
Regards,
Romain
>>> The patch will add the possibility to configure the DMA buffers in DT,
>>> which case by case can be configured separately for every driver
>>> instance. The DT configuration is optional and in case missing the
>>> driver will use the 4096 buffer with 4 periods (as before), therefore no
>>> clients are impacted by this change.
>>>
>>> Signed-off-by: Nandor Han <nandor.han-JJi787mZWgc@public.gmane.org>
>>> Signed-off-by: Romain Perier <romain.perier-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
>>> ---
>>> .../devicetree/bindings/serial/fsl-imx-uart.txt | 2 ++
>>> drivers/tty/serial/imx.c | 25 +++++++++++++++-------
>>> 2 files changed, 19 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/serial/fsl-imx-uart.txt b/Documentation/devicetree/bindings/serial/fsl-imx-uart.txt
>>> index 574c3a2..e6b5724 100644
>>> --- a/Documentation/devicetree/bindings/serial/fsl-imx-uart.txt
>>> +++ b/Documentation/devicetree/bindings/serial/fsl-imx-uart.txt
>>> @@ -9,6 +9,7 @@ Optional properties:
>>> - fsl,irda-mode : Indicate the uart supports irda mode
>>> - fsl,dte-mode : Indicate the uart works in DTE mode. The uart works
>>> in DCE mode by default.
>>> +- fsl,dma-size : Indicate the size of the DMA buffer and its periods
>> This is a sparse description, just from reading that I don't understand
>> what it does.
>>
>>> Please check Documentation/devicetree/bindings/serial/serial.txt
>>> for the complete list of generic properties.
>>> @@ -28,4 +29,5 @@ uart1: serial@73fbc000 {
>>> interrupts = <31>;
>>> uart-has-rtscts;
>>> fsl,dte-mode;
>>> + fsl,dma-size = <1024 4>;
>>> };
>>> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
>>> index bbefddd..7327477 100644
>>> --- a/drivers/tty/serial/imx.c
>>> +++ b/drivers/tty/serial/imx.c
>>> @@ -186,6 +186,11 @@
>>>
>>> #define UART_NR 8
>>>
>>> +/* RX DMA buffer periods */
>>> +#define RX_DMA_PERIODS 4
>>> +#define RX_BUF_SIZE (PAGE_SIZE)
>> These names should include "DEFAULT" in their name now to fit their new
>> semantic.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] serial: imx-serial - move DMA buffer configuration to DT
From: Uwe Kleine-König @ 2017-07-03 18:17 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Romain Perier, Jiri Slaby, linux-serial-u79uwXL29TY76Z2rM5mHXA,
Nandor Han, devicetree-u79uwXL29TY76Z2rM5mHXA,
kernel-bIcnvbaLZ9MEGnE8C9+IrQ
In-Reply-To: <20170629182618.jpahpmuq364ldcv2-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Hello Greg,
I'm a bit disappointed that you didn't drop this patch. At the time I
sent my review it was still in your tty-testing branch and now it's part
of your pull request for 4.13-rc1 (as commit
a3015affdf76ef279fbbb3710a220bab7e9ea04b). :-|
I'm still convinced that this patch is wrong in its current form.
Best regards
Uwe
On Thu, Jun 29, 2017 at 08:26:18PM +0200, Uwe Kleine-König wrote:
> Cc: += devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>
> On Wed, Jun 28, 2017 at 12:15:14PM +0200, Romain Perier wrote:
> > From: Nandor Han <nandor.han-JJi787mZWgc@public.gmane.org>
> >
> > The size of the DMA buffer can affect the delta time between data being
> > produced and data being consumed. Basically the DMA system will move
> > data to tty buffer when a) DMA buffer is full b) serial line is idle.
> > The situation is visible when producer generates data continuously and
> > there is no possibility for idle line. At this point the DMA buffer is
> > directly affecting the delta time.
>
> This doesn't look like a hw property but a configuration item. Also I
> don't understand the problematic case. The i.MX is sending continuously
> and then doesn't receive bytes until the DMA buffer is full? What is the
> DMA buffer? You don't mean the FIFO here, do you?
>
> That doesn't sound like a good fix but more like a work around. Which
> other options did you test to fix your problem?
>
> > The patch will add the possibility to configure the DMA buffers in DT,
> > which case by case can be configured separately for every driver
> > instance. The DT configuration is optional and in case missing the
> > driver will use the 4096 buffer with 4 periods (as before), therefore no
> > clients are impacted by this change.
> >
> > Signed-off-by: Nandor Han <nandor.han-JJi787mZWgc@public.gmane.org>
> > Signed-off-by: Romain Perier <romain.perier-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
> > ---
> > .../devicetree/bindings/serial/fsl-imx-uart.txt | 2 ++
> > drivers/tty/serial/imx.c | 25 +++++++++++++++-------
> > 2 files changed, 19 insertions(+), 8 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/serial/fsl-imx-uart.txt b/Documentation/devicetree/bindings/serial/fsl-imx-uart.txt
> > index 574c3a2..e6b5724 100644
> > --- a/Documentation/devicetree/bindings/serial/fsl-imx-uart.txt
> > +++ b/Documentation/devicetree/bindings/serial/fsl-imx-uart.txt
> > @@ -9,6 +9,7 @@ Optional properties:
> > - fsl,irda-mode : Indicate the uart supports irda mode
> > - fsl,dte-mode : Indicate the uart works in DTE mode. The uart works
> > in DCE mode by default.
> > +- fsl,dma-size : Indicate the size of the DMA buffer and its periods
>
> This is a sparse description, just from reading that I don't understand
> what it does.
>
> > Please check Documentation/devicetree/bindings/serial/serial.txt
> > for the complete list of generic properties.
> > @@ -28,4 +29,5 @@ uart1: serial@73fbc000 {
> > interrupts = <31>;
> > uart-has-rtscts;
> > fsl,dte-mode;
> > + fsl,dma-size = <1024 4>;
> > };
> > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > index bbefddd..7327477 100644
> > --- a/drivers/tty/serial/imx.c
> > +++ b/drivers/tty/serial/imx.c
> > @@ -186,6 +186,11 @@
> >
> > #define UART_NR 8
> >
> > +/* RX DMA buffer periods */
> > +#define RX_DMA_PERIODS 4
> > +#define RX_BUF_SIZE (PAGE_SIZE)
>
> These names should include "DEFAULT" in their name now to fit their new
> semantic.
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox