* [PATCH v4 0/3] serial: imx: fix RTS and RTS/CTS handling
From: Sergey Organov @ 2019-07-19 8:47 UTC (permalink / raw)
To: linux-serial
Cc: Greg Kroah-Hartman, Sascha Hauer, Sergey Organov, NXP Linux Team,
Pengutronix Kernel Team, Uwe Kleine-König, linux-arm-kernel
In-Reply-To: <20190614072801.3187-1-s.hauer@pengutronix.de>
This is rebase of v3 on top of 'tty-next', to get rid of commits that
are already adopted to mainline.
RTS signal and RTS/CTS handshake handling had a few problems these
patches fix.
In addition, minor cleanups are made to the involved code.
Changelog:
v4:
* rebased on top of 'tty-next', to get rid of commits that
are already adopted to mainline.
v3:
* Improved comments in "serial: imx: set_mctrl(): correctly
restore autoRTS state", as suggested by Uwe Kleine-König
<u.kleine-koenig@pengutronix.de>
v2:
* Appended: "Reviewed-by:" and "Tested-by:"
Sascha Hauer <s.hauer@pengutronix.de>
* Removed "RFC" from header
v1:
* Fixed in "serial: imx: set_termios(): preserve RTS state"
-+ ucr2 = UCR2_SRST | UCR2_IRTS;
++ ucr2 |= UCR2_SRST | UCR2_IRTS;
as noticed by Lothar Waßmann <LW@KARO-electronics.de>
* Fixed in "serial: imx: set_termios(): preserve RTS state"
-+ ucr2 = old_ucr2 & (UCR2_TXEN | UCR2_RXEN | UCR2_ATEN | UCR2_CTSC);
++ ucr2 = old_ucr2 & (UCR2_TXEN | UCR2_RXEN | UCR2_ATEN | UCR2_CTS);
as the fix for the problem found by Sascha Hauer
<s.hauer@pengutronix.de>
* Reordered:
serial: imx: set_termios(): preserve RTS state
serial: imx: set_termios(): do not enable autoRTS if RTS is unset
as the latter makes sense only provided the former is already
applied.
Sergey Organov (3):
serial: imx: set_termios(): do not enable autoRTS if RTS is unset
serial: imx: set_mctrl(): correctly restore autoRTS state
serial: imx: get rid of imx_uart_rts_auto()
drivers/tty/serial/imx.c | 28 ++++++++++++++++++----------
1 file changed, 18 insertions(+), 10 deletions(-)
--
2.10.0.1.g57b01a3
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH v4 1/3] serial: imx: set_termios(): do not enable autoRTS if RTS is unset
From: Sergey Organov @ 2019-07-19 8:47 UTC (permalink / raw)
To: linux-serial
Cc: Greg Kroah-Hartman, Sascha Hauer, Sergey Organov, NXP Linux Team,
Pengutronix Kernel Team, Uwe Kleine-König, linux-arm-kernel
In-Reply-To: <1563526074-20399-1-git-send-email-sorganov@gmail.com>
set_termios() shouldn't set UCR2_CTSC bit if UCR2_CTS (=TIOCM_RTS) is
cleared. Added corresponding check in imx_uart_rts_auto() to fix this.
Reviewed-by: Sascha Hauer <s.hauer@pengutronix.de>
Tested-by: Sascha Hauer <s.hauer@pengutronix.de>
Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
drivers/tty/serial/imx.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 57d6e6b..95d7984 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -405,7 +405,8 @@ static void imx_uart_rts_inactive(struct imx_port *sport, u32 *ucr2)
/* called with port.lock taken and irqs caller dependent */
static void imx_uart_rts_auto(struct imx_port *sport, u32 *ucr2)
{
- *ucr2 |= UCR2_CTSC;
+ if (*ucr2 & UCR2_CTS)
+ *ucr2 |= UCR2_CTSC;
}
/* called with port.lock taken and irqs off */
--
2.10.0.1.g57b01a3
^ permalink raw reply related
* [PATCH v4 2/3] serial: imx: set_mctrl(): correctly restore autoRTS state
From: Sergey Organov @ 2019-07-19 8:47 UTC (permalink / raw)
To: linux-serial
Cc: Greg Kroah-Hartman, Sascha Hauer, Sergey Organov, NXP Linux Team,
Pengutronix Kernel Team, Uwe Kleine-König, linux-arm-kernel
In-Reply-To: <1563526074-20399-1-git-send-email-sorganov@gmail.com>
imx_uart_set_mctrl() happened to set UCR2_CTSC bit whenever TIOCM_RTS
was set, no matter if RTS/CTS handshake is enabled or not. Now fixed by
turning handshake on only when CRTSCTS bit for the port is set.
Reviewed-by: Sascha Hauer <s.hauer@pengutronix.de>
Tested-by: Sascha Hauer <s.hauer@pengutronix.de>
Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
drivers/tty/serial/imx.c | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 95d7984..34d61c4 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -970,10 +970,22 @@ static void imx_uart_set_mctrl(struct uart_port *port, unsigned int mctrl)
if (!(port->rs485.flags & SER_RS485_ENABLED)) {
u32 ucr2;
+ /*
+ * Turn off autoRTS if RTS is lowered and restore autoRTS
+ * setting if RTS is raised.
+ */
ucr2 = imx_uart_readl(sport, UCR2);
ucr2 &= ~(UCR2_CTS | UCR2_CTSC);
- if (mctrl & TIOCM_RTS)
- ucr2 |= UCR2_CTS | UCR2_CTSC;
+ if (mctrl & TIOCM_RTS) {
+ ucr2 |= UCR2_CTS;
+ /*
+ * UCR2_IRTS is unset if and only if the port is
+ * configured for CRTSCTS, so we use inverted UCR2_IRTS
+ * to get the state to restore to.
+ */
+ if (!(ucr2 & UCR2_IRTS))
+ ucr2 |= UCR2_CTSC;
+ }
imx_uart_writel(sport, ucr2, UCR2);
}
--
2.10.0.1.g57b01a3
^ permalink raw reply related
* [PATCH v4 3/3] serial: imx: get rid of imx_uart_rts_auto()
From: Sergey Organov @ 2019-07-19 8:47 UTC (permalink / raw)
To: linux-serial
Cc: Greg Kroah-Hartman, Sascha Hauer, Sergey Organov, NXP Linux Team,
Pengutronix Kernel Team, Uwe Kleine-König, linux-arm-kernel
In-Reply-To: <1563526074-20399-1-git-send-email-sorganov@gmail.com>
Called in only one place, for RS232, it only obscures things, as it
doesn't go well with 2 similar named functions,
imx_uart_rts_inactive() and imx_uart_rts_active(), that both are
RS485-specific.
Reviewed-by: Sascha Hauer <s.hauer@pengutronix.de>
Tested-by: Sascha Hauer <s.hauer@pengutronix.de>
Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
drivers/tty/serial/imx.c | 13 ++++---------
1 file changed, 4 insertions(+), 9 deletions(-)
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 34d61c4..971055b 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -402,13 +402,6 @@ static void imx_uart_rts_inactive(struct imx_port *sport, u32 *ucr2)
mctrl_gpio_set(sport->gpios, sport->port.mctrl);
}
-/* called with port.lock taken and irqs caller dependent */
-static void imx_uart_rts_auto(struct imx_port *sport, u32 *ucr2)
-{
- if (*ucr2 & UCR2_CTS)
- *ucr2 |= UCR2_CTSC;
-}
-
/* called with port.lock taken and irqs off */
static void imx_uart_start_rx(struct uart_port *port)
{
@@ -1600,8 +1593,10 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
else
imx_uart_rts_inactive(sport, &ucr2);
- } else if (termios->c_cflag & CRTSCTS)
- imx_uart_rts_auto(sport, &ucr2);
+ } else if (termios->c_cflag & CRTSCTS) {
+ if (ucr2 & UCR2_CTS)
+ ucr2 |= UCR2_CTSC;
+ }
if (termios->c_cflag & CRTSCTS)
ucr2 &= ~UCR2_IRTS;
--
2.10.0.1.g57b01a3
^ permalink raw reply related
* Re: [PATCH v4 1/3] serial: imx: set_termios(): do not enable autoRTS if RTS is unset
From: Uwe Kleine-König @ 2019-07-19 9:11 UTC (permalink / raw)
To: Sergey Organov
Cc: linux-serial, Greg Kroah-Hartman, Sascha Hauer, NXP Linux Team,
Pengutronix Kernel Team, linux-arm-kernel
In-Reply-To: <1563526074-20399-2-git-send-email-sorganov@gmail.com>
On Fri, Jul 19, 2019 at 11:47:52AM +0300, Sergey Organov wrote:
> set_termios() shouldn't set UCR2_CTSC bit if UCR2_CTS (=TIOCM_RTS) is
> cleared. Added corresponding check in imx_uart_rts_auto() to fix this.
This is not understandable unless you read the reference manual.
In terms understandable without manual, this patch does:
Don't let the receiver hardware control the RTS output if it was
requested to be inactive.
> Reviewed-by: Sascha Hauer <s.hauer@pengutronix.de>
> Tested-by: Sascha Hauer <s.hauer@pengutronix.de>
> Signed-off-by: Sergey Organov <sorganov@gmail.com>
> ---
> drivers/tty/serial/imx.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 57d6e6b..95d7984 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -405,7 +405,8 @@ static void imx_uart_rts_inactive(struct imx_port *sport, u32 *ucr2)
> /* called with port.lock taken and irqs caller dependent */
> static void imx_uart_rts_auto(struct imx_port *sport, u32 *ucr2)
> {
> - *ucr2 |= UCR2_CTSC;
> + if (*ucr2 & UCR2_CTS)
> + *ucr2 |= UCR2_CTSC;
I think this patch is wrong or the commit log is insufficient.
imx_uart_rts_auto() has only a single caller and there ucr2 & UCR2_CTS is
never true. And CTSC isn't restored anywhere, is it?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* Re: [PATCH v3] serial: 8250-mtk: modify mtk uart power and clock management
From: Matthias Brugger @ 2019-07-19 11:16 UTC (permalink / raw)
To: Changqi Hu, Greg Kroah-Hartman, Jiri Slaby
Cc: Peter Shih, Gustavo A. R. Silva, linux-serial, linux-arm-kernel,
linux-mediatek, linux-kernel, srv_heupstream, Eddie Huang,
Yingjoe Chen
In-Reply-To: <1563505182-2408-1-git-send-email-changqi.hu@mediatek.com>
On 19/07/2019 04:59, Changqi Hu wrote:
> modify mtk uart runtime interface, add uart clock use count.
> merge patch v1 and patch v2 together.
>
Please try to explain better why we need this.
> Signed-off-by: Changqi Hu <changqi.hu@mediatek.com>
> ---
Changelog from one version to another (like that you merged v1 and v2) should go
here, as we don't want that to be part of the commit message once the patch is
applied.
Regards,
Matthias
> drivers/tty/serial/8250/8250_mtk.c | 50 ++++++++++++++++++++++++--------------
> 1 file changed, 32 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/tty/serial/8250/8250_mtk.c b/drivers/tty/serial/8250/8250_mtk.c
> index f470ded..a07c8ae 100644
> --- a/drivers/tty/serial/8250/8250_mtk.c
> +++ b/drivers/tty/serial/8250/8250_mtk.c
> @@ -31,6 +31,7 @@
> #define MTK_UART_RXTRI_AD 0x14 /* RX Trigger address */
> #define MTK_UART_FRACDIV_L 0x15 /* Fractional divider LSB address */
> #define MTK_UART_FRACDIV_M 0x16 /* Fractional divider MSB address */
> +#define MTK_UART_DEBUG0 0x18
> #define MTK_UART_IER_XOFFI 0x20 /* Enable XOFF character interrupt */
> #define MTK_UART_IER_RTSI 0x40 /* Enable RTS Modem status interrupt */
> #define MTK_UART_IER_CTSI 0x80 /* Enable CTS Modem status interrupt */
> @@ -386,9 +387,18 @@ static void mtk8250_set_flow_ctrl(struct uart_8250_port *up, int mode)
> static int __maybe_unused mtk8250_runtime_suspend(struct device *dev)
> {
> struct mtk8250_data *data = dev_get_drvdata(dev);
> + struct uart_8250_port *up = serial8250_get_port(data->line);
>
> - clk_disable_unprepare(data->uart_clk);
> - clk_disable_unprepare(data->bus_clk);
> + /* wait until UART in idle status */
> + while
> + (serial_in(up, MTK_UART_DEBUG0));
> +
> + if (data->clk_count == 0U) {
> + dev_dbg(dev, "%s clock count is 0\n", __func__);
> + } else {
> + clk_disable_unprepare(data->bus_clk);
> + data->clk_count--;
> + }
>
> return 0;
> }
> @@ -398,16 +408,16 @@ static int __maybe_unused mtk8250_runtime_resume(struct device *dev)
> struct mtk8250_data *data = dev_get_drvdata(dev);
> int err;
>
> - err = clk_prepare_enable(data->uart_clk);
> - if (err) {
> - dev_warn(dev, "Can't enable clock\n");
> - return err;
> - }
> -
> - err = clk_prepare_enable(data->bus_clk);
> - if (err) {
> - dev_warn(dev, "Can't enable bus clock\n");
> - return err;
> + if (data->clk_count > 0U) {
> + dev_dbg(dev, "%s clock count is %d\n", __func__,
> + data->clk_count);
> + } else {
> + err = clk_prepare_enable(data->bus_clk);
> + if (err) {
> + dev_warn(dev, "Can't enable bus clock\n");
> + return err;
> + }
> + data->clk_count++;
> }
>
> return 0;
> @@ -417,12 +427,14 @@ static int __maybe_unused mtk8250_runtime_resume(struct device *dev)
> mtk8250_do_pm(struct uart_port *port, unsigned int state, unsigned int old)
> {
> if (!state)
> - pm_runtime_get_sync(port->dev);
> + if (!mtk8250_runtime_resume(port->dev))
> + pm_runtime_get_sync(port->dev);
>
> serial8250_do_pm(port, state, old);
>
> if (state)
> - pm_runtime_put_sync_suspend(port->dev);
> + if (!pm_runtime_put_sync_suspend(port->dev))
> + mtk8250_runtime_suspend(port->dev);
> }
>
> #ifdef CONFIG_SERIAL_8250_DMA
> @@ -499,6 +511,8 @@ static int mtk8250_probe(struct platform_device *pdev)
> if (!data)
> return -ENOMEM;
>
> + data->clk_count = 0;
> +
> if (pdev->dev.of_node) {
> err = mtk8250_probe_of(pdev, &uart.port, data);
> if (err)
> @@ -531,6 +545,7 @@ static int mtk8250_probe(struct platform_device *pdev)
>
> platform_set_drvdata(pdev, data);
>
> + pm_runtime_enable(&pdev->dev);
> err = mtk8250_runtime_resume(&pdev->dev);
> if (err)
> return err;
> @@ -539,9 +554,6 @@ static int mtk8250_probe(struct platform_device *pdev)
> if (data->line < 0)
> return data->line;
>
> - pm_runtime_set_active(&pdev->dev);
> - pm_runtime_enable(&pdev->dev);
> -
> return 0;
> }
>
> @@ -552,11 +564,13 @@ static int mtk8250_remove(struct platform_device *pdev)
> pm_runtime_get_sync(&pdev->dev);
>
> serial8250_unregister_port(data->line);
> - mtk8250_runtime_suspend(&pdev->dev);
>
> pm_runtime_disable(&pdev->dev);
> pm_runtime_put_noidle(&pdev->dev);
>
> + if (!pm_runtime_status_suspended(&pdev->dev))
> + mtk8250_runtime_suspend(&pdev->dev);
> +
> return 0;
> }
>
>
^ permalink raw reply
* Re: [PATCH v4 1/3] serial: imx: set_termios(): do not enable autoRTS if RTS is unset
From: Sergey Organov @ 2019-07-19 12:18 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: linux-serial, Greg Kroah-Hartman, Sascha Hauer, NXP Linux Team,
Pengutronix Kernel Team, linux-arm-kernel
In-Reply-To: <20190719091143.uhjxeibtolgswq2l@pengutronix.de>
Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
> On Fri, Jul 19, 2019 at 11:47:52AM +0300, Sergey Organov wrote:
>> set_termios() shouldn't set UCR2_CTSC bit if UCR2_CTS (=TIOCM_RTS) is
>> cleared. Added corresponding check in imx_uart_rts_auto() to fix this.
>
> This is not understandable unless you read the reference manual.
>
> In terms understandable without manual, this patch does:
>
> Don't let the receiver hardware control the RTS output if it was
> requested to be inactive.
I'll fix it, thanks!
>
>> Reviewed-by: Sascha Hauer <s.hauer@pengutronix.de>
>> Tested-by: Sascha Hauer <s.hauer@pengutronix.de>
>> Signed-off-by: Sergey Organov <sorganov@gmail.com>
>> ---
>> drivers/tty/serial/imx.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
>> index 57d6e6b..95d7984 100644
>> --- a/drivers/tty/serial/imx.c
>> +++ b/drivers/tty/serial/imx.c
>> @@ -405,7 +405,8 @@ static void imx_uart_rts_inactive(struct imx_port *sport, u32 *ucr2)
>> /* called with port.lock taken and irqs caller dependent */
>> static void imx_uart_rts_auto(struct imx_port *sport, u32 *ucr2)
>> {
>> - *ucr2 |= UCR2_CTSC;
>> + if (*ucr2 & UCR2_CTS)
>> + *ucr2 |= UCR2_CTSC;
>
> I think this patch is wrong or the commit log is insufficient.
> imx_uart_rts_auto() has only a single caller and there ucr2 & UCR2_CTS is
> never true. And CTSC isn't restored anywhere, is it?
This is rebase to 'tty-next' branch, and you need to look at it in that
context. There, ucr2 & UCR2_CTS does already make sense, due to previous
fix that is already there.
Alternatively, look at v3 of the patches, but you basically already did.
It's that the first part of v3 patch series has been already accepted
upstream, and this is the rest of those patches.
Thanks!
-- Sergey
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [RFC PATCH 0/2] serial: pl011: Fix TX dropping race
From: Dave Martin @ 2019-07-19 13:35 UTC (permalink / raw)
To: linux-serial
Cc: Russell King, Greg Kroah-Hartman, Phil Elwell, linux-rpi-kernel,
Jiri Slaby, Rogier Wolff, linux-arm-kernel
When serial_core pushes some new TX chars via a call to
pl011_start_tx(), it can race with irqs triggered by ongoing
transmission, overfilling the FIFO and causing characters to be silently
dropped.
This was originally reported by Phil Elwell [1], who proposed an initial
fix.
This series aims for a simpler and more robust solution to the problem.
Any testing much appreciated! If all looks good, I can repost this on
v5.3-rc1 when that arrives.
As noted in the patches, I'm not sure that the second patch is necessary
(or even desirable). Please test both with and without the second
patch, and please comment if you have any thoughts on it :)
[1] [PATCH] tty: amba-pl011: Make TX optimisation conditional
http://lists.infradead.org/pipermail/linux-rpi-kernel/2019-July/008832.html
Dave Martin (2):
serial: pl011: Fix dropping of TX chars due to irq race
serial: pl011: Don't bother pushing more TX data while TX irq is
active
drivers/tty/serial/amba-pl011.c | 11 +++++++++++
1 file changed, 11 insertions(+)
--
2.1.4
^ permalink raw reply
* [RFC PATCH 1/2] serial: pl011: Fix dropping of TX chars due to irq race
From: Dave Martin @ 2019-07-19 13:35 UTC (permalink / raw)
To: linux-serial
Cc: Russell King, Greg Kroah-Hartman, Phil Elwell, linux-rpi-kernel,
Jiri Slaby, Rogier Wolff, linux-arm-kernel
In-Reply-To: <1563543325-12463-1-git-send-email-Dave.Martin@arm.com>
When serial_core pushes some new TX chars via a call to
pl011_start_tx(), it can race with irqs triggered by ongoing
transmission.
Normally the port lock protects against this kind of thing, but
temporary releasing of the lock during calls from pl011_int() to
pl011_{,dma_}rx_chars() allows pl011_start_tx() to race.
For performance reasons, pl011_tx_chars(, true) always assumes that
the TX FIFO interrupt trigger condition holds, i.e., the FIFO is
empty to the trigger threshold. This means that we can write chars
to fill the FIFO back up without the expense of polling the FIFO
fill status. However, this assumes that no data is written to the
FIFO in the meantime by other code: this is where the race with
pl011_start_tx_pio() breaks things.
Reorder pl011_int() so that no code releases the port lock in
between reading the interrupt status bits and calling
pl011_tx_chars(). This ensures that TXIS in the fetched status
accurately reflects the state of the TX FIFO, and ensures that
there is no race to fill the FIFO.
Fixes: 1e84d22322ce ("serial/amba-pl011: Refactor and simplify TX FIFO handling")
Reported-by: Phil Elwell <phil@raspberrypi.org>
Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
drivers/tty/serial/amba-pl011.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 89ade21..e24bbc0 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -1492,6 +1492,13 @@ static irqreturn_t pl011_int(int irq, void *dev_id)
UART011_RXIS),
uap, REG_ICR);
+ /*
+ * Don't unlock uap->port.lock before here:
+ * Stale TXIS status can lead to a FIFO overfill.
+ */
+ if (status & UART011_TXIS)
+ pl011_tx_chars(uap, true);
+
if (status & (UART011_RTIS|UART011_RXIS)) {
if (pl011_dma_rx_running(uap))
pl011_dma_rx_irq(uap);
--
2.1.4
^ permalink raw reply related
* [RFC PATCH 2/2] serial: pl011: Don't bother pushing more TX data while TX irq is active
From: Dave Martin @ 2019-07-19 13:35 UTC (permalink / raw)
To: linux-serial
Cc: Russell King, Greg Kroah-Hartman, Phil Elwell, linux-rpi-kernel,
Jiri Slaby, Rogier Wolff, linux-arm-kernel
In-Reply-To: <1563543325-12463-1-git-send-email-Dave.Martin@arm.com>
When the TX irq is active, writing chars to the TX FIFO from
anywhere except pl011_int() is pointless: the UART is already busy,
and new chars will be picked up by pl011_int() as soon as there is
FIFO space.
To reduce the scope for surprises, bail out of pl011_start_tx_pio()
without attempting to write to the FIFO or start TX DMA if the TX FIFO
interrupt is already in use.
This should also avoid pointless overhead in some situations.
Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
Please test both with and without this patch.
I believe with the previous patch in place, this patch is not strictly
necessary. However, if the UART is actively transmitting in the
background already, it does make sense not to waste time trying polling
the FIFO fill status or setting up DMA etc.
---
drivers/tty/serial/amba-pl011.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index e24bbc0..f28935a 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -1318,6 +1318,10 @@ static void pl011_start_tx(struct uart_port *port)
struct uart_amba_port *uap =
container_of(port, struct uart_amba_port, port);
+ /* It's pointless to kick the UART if it's already transmitting... */
+ if (uap->im & UART011_TXIM)
+ return;
+
if (!pl011_dma_tx_start(uap))
pl011_start_tx_pio(uap);
}
--
2.1.4
^ permalink raw reply related
* Re: [PATCH] 8250_lpss: check null return when calling pci_ioremap_bar
From: Andy Shevchenko @ 2019-07-19 13:37 UTC (permalink / raw)
To: Navid Emamdoost, Kangjie Lu, Aditya Pakki
Cc: emamd001, smccaman, Greg Kroah-Hartman, Jiri Slaby, Vinod Koul,
linux-serial, linux-kernel
In-Reply-To: <20190719025443.2368-1-navid.emamdoost@gmail.com>
On Thu, Jul 18, 2019 at 09:54:42PM -0500, Navid Emamdoost wrote:
> pci_ioremap_bar may return null. This is eventually de-referenced at
> drivers/dma/dw/core.c:1154 and drivers/dma/dw/core.c:1168. A null check is
> needed to prevent null de-reference. I am adding the check and in case of
> failure returning -ENOMEM (I am not sure this is the best errno, you may
> consider it as a placeholder), and subsequently changing the caller’s
> return type, and propagating the error.
Thanks for the patch, my comments below.
> chip->irq = pci_irq_vector(pdev, 0);
> chip->regs = pci_ioremap_bar(pdev, 1);
> + if (!chip->regs)
> + return -ENOMEM;
This is the same case as below, it's fine to go on without DMA support.
> chip->pdata = &qrk_serial_dma_pdata;
So, I would rather to put like this...
Hold on, I remember someone already tried to fix this [1].
I dunno why it wasn't v5, due to [2].
Also, similar to yours, but wrong [3].
Thus, please, collaborate guys, and send one compiling solution based on [1].
> /* Falling back to PIO mode if DMA probing fails */
> ret = dw_dma_probe(chip);
> if (ret)
> - return;
> + return 0;
[1]: https://www.spinics.net/lists/linux-serial/msg33965.html
[2]: https://lists.01.org/pipermail/kbuild-all/2019-March/059215.html
[3]: https://lore.kernel.org/patchwork/patch/1051000/
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH] 8250_lpss: check null return when calling pci_ioremap_bar
From: kbuild test robot @ 2019-07-19 13:58 UTC (permalink / raw)
Cc: kbuild-all, emamd001, kjlu, smccaman, Navid Emamdoost,
Greg Kroah-Hartman, Jiri Slaby, Andy Shevchenko, Vinod Koul,
linux-serial, linux-kernel
In-Reply-To: <20190719025443.2368-1-navid.emamdoost@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1135 bytes --]
Hi Navid,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master]
[cannot apply to v5.2 next-20190718]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Navid-Emamdoost/8250_lpss-check-null-return-when-calling-pci_ioremap_bar/20190719-120441
config: x86_64-randconfig-h002-201928 (attached as .config)
compiler: gcc-7 (Debian 7.4.0-10) 7.4.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
drivers/tty/serial/8250/8250_lpss.c: In function 'qrk_serial_setup':
>> drivers/tty/serial/8250/8250_lpss.c:225:6: error: void value not ignored as it ought to be
ret = qrk_serial_setup_dma(lpss, port);
^
vim +225 drivers/tty/serial/8250/8250_lpss.c
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 31667 bytes --]
^ permalink raw reply
* Re: [PATCH v4 1/3] serial: imx: set_termios(): do not enable autoRTS if RTS is unset
From: Uwe Kleine-König @ 2019-07-19 14:31 UTC (permalink / raw)
To: Sergey Organov
Cc: linux-serial, Greg Kroah-Hartman, Sascha Hauer, NXP Linux Team,
Pengutronix Kernel Team, linux-arm-kernel
In-Reply-To: <87h87idxq2.fsf@osv.gnss.ru>
Hello Sergey,
On Fri, Jul 19, 2019 at 03:18:13PM +0300, Sergey Organov wrote:
> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
> > On Fri, Jul 19, 2019 at 11:47:52AM +0300, Sergey Organov wrote:
> >> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> >> index 57d6e6b..95d7984 100644
> >> --- a/drivers/tty/serial/imx.c
> >> +++ b/drivers/tty/serial/imx.c
> >> @@ -405,7 +405,8 @@ static void imx_uart_rts_inactive(struct imx_port *sport, u32 *ucr2)
> >> /* called with port.lock taken and irqs caller dependent */
> >> static void imx_uart_rts_auto(struct imx_port *sport, u32 *ucr2)
> >> {
> >> - *ucr2 |= UCR2_CTSC;
> >> + if (*ucr2 & UCR2_CTS)
> >> + *ucr2 |= UCR2_CTSC;
> >
> > I think this patch is wrong or the commit log is insufficient.
> > imx_uart_rts_auto() has only a single caller and there ucr2 & UCR2_CTS is
> > never true. And CTSC isn't restored anywhere, is it?
>
> This is rebase to 'tty-next' branch, and you need to look at it in that
> context. There, ucr2 & UCR2_CTS does already make sense, due to previous
> fix that is already there.
I looked at 57d6e6b which is the file you patched. And there
imx_uart_rts_auto is only ever called with *ucr2 not having UCR2_CTS.
If you still think I'm wrong, please improve the commit log accordingly.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* Re: [PATCH v4 1/3] serial: imx: set_termios(): do not enable autoRTS if RTS is unset
From: Sergey Organov @ 2019-07-19 15:13 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: linux-serial, Greg Kroah-Hartman, Sascha Hauer, NXP Linux Team,
Pengutronix Kernel Team, linux-arm-kernel
In-Reply-To: <20190719143151.gx43ndn2oy35h247@pengutronix.de>
Hello Uwe,
Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
> Hello Sergey,
>
> On Fri, Jul 19, 2019 at 03:18:13PM +0300, Sergey Organov wrote:
>> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
>> > On Fri, Jul 19, 2019 at 11:47:52AM +0300, Sergey Organov wrote:
>> >> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
>> >> index 57d6e6b..95d7984 100644
>> >> --- a/drivers/tty/serial/imx.c
>> >> +++ b/drivers/tty/serial/imx.c
>> >> @@ -405,7 +405,8 @@ static void imx_uart_rts_inactive(struct imx_port *sport, u32 *ucr2)
>> >> /* called with port.lock taken and irqs caller dependent */
>> >> static void imx_uart_rts_auto(struct imx_port *sport, u32 *ucr2)
>> >> {
>> >> - *ucr2 |= UCR2_CTSC;
>> >> + if (*ucr2 & UCR2_CTS)
>> >> + *ucr2 |= UCR2_CTSC;
>> >
>> > I think this patch is wrong or the commit log is insufficient.
>> > imx_uart_rts_auto() has only a single caller and there ucr2 & UCR2_CTS is
>> > never true. And CTSC isn't restored anywhere, is it?
>>
>> This is rebase to 'tty-next' branch, and you need to look at it in that
>> context. There, ucr2 & UCR2_CTS does already make sense, due to previous
>> fix that is already there.
>
> I looked at 57d6e6b which is the file you patched. And there
> imx_uart_rts_auto is only ever called with *ucr2 not having UCR2_CTS.
>
> If you still think I'm wrong, please improve the commit log
> accordingly.
I still think you are wrong, but I don't know how to improve commit log.
To check it once again, I just did:
$ git show 57d6e6b > imx.c
There, in imx_uart_set_termios(), I see:
1569: old_ucr2 = imx_uart_readl(sport, UCR2);
1570: ucr2 = old_ucr2 & (UCR2_TXEN | UCR2_RXEN | UCR2_ATEN | UCR2_CTS);
Here, current UCR2 value is read into 'old_ucr2' and then its /current/
UCR2_CTS bit is copied into 'ucr2' (along with 3 other bits).
Then, later in the same function:
1591: imx_uart_rts_auto(sport, &ucr2);
is called that can check /current/ state of UCR2_CTS bit in '*ucr2'.
That's what the patch does, checks this bit.
Sorry, I fail to see how you came to conclusion that "*ucr2 not having
UCR2_CTS". Do we maybe still read different versions of the file?
-- Sergey
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] 8250_lpss: check null return when calling pci_ioremap_bar
From: Andy Shevchenko @ 2019-07-19 15:15 UTC (permalink / raw)
To: Navid Emamdoost, Kangjie Lu, Aditya Pakki
Cc: emamd001, smccaman, Greg Kroah-Hartman, Jiri Slaby, Vinod Koul,
linux-serial, linux-kernel
In-Reply-To: <20190719133735.GM9224@smile.fi.intel.com>
On Fri, Jul 19, 2019 at 04:37:35PM +0300, Andy Shevchenko wrote:
> On Thu, Jul 18, 2019 at 09:54:42PM -0500, Navid Emamdoost wrote:
> Thus, please, collaborate guys, and send one compiling solution based on [1].
> [1]: https://www.spinics.net/lists/linux-serial/msg33965.html
For your convenience (what has to be applied to v4):
diff --git a/drivers/tty/serial/8250/8250_lpss.c b/drivers/tty/serial/8250/8250_lpss.c
index 53ca9ba6ab4b..6214da67e9b3 100644
--- a/drivers/tty/serial/8250/8250_lpss.c
+++ b/drivers/tty/serial/8250/8250_lpss.c
@@ -195,11 +195,15 @@ static void qrk_serial_setup_dma(struct lpss8250 *lpss, struct uart_port *port)
static void qrk_serial_exit_dma(struct lpss8250 *lpss)
{
+ struct dw_dma_chip *chip = &lpss->dma_chip;
struct dw_dma_slave *param = &lpss->dma_param;
if (!param->dma_dev)
return;
- dw_dma_remove(&lpss->dma_chip);
+
+ dw_dma_remove(chip);
+
+ pci_iounmap(to_pci_dev(chip->dev), chip->regs);
}
#else /* CONFIG_SERIAL_8250_DMA */
static void qrk_serial_setup_dma(struct lpss8250 *lpss, struct uart_port *port) {}
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply related
* [PATCH v2] 8250_lpss: check null return when calling pci_ioremap_bar
From: Navid Emamdoost @ 2019-07-19 17:48 UTC (permalink / raw)
To: andriy.shevchenko
Cc: emamd001, kjlu, smccaman, Navid Emamdoost, Greg Kroah-Hartman,
Jiri Slaby, Vinod Koul, linux-serial, linux-kernel
In-Reply-To: <20190719151519.GO9224@smile.fi.intel.com>
pci_ioremap_bar may return null. This is eventually de-referenced at
drivers/dma/dw/core.c:1154 and drivers/dma/dw/core.c:1168. A null check
is needed to prevent null de-reference. I am adding the check and in case
of failure. Thanks to Andy Shevchenko for the hint on the necessity of
pci_iounmap when exiting.
Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>
---
drivers/tty/serial/8250/8250_lpss.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_lpss.c b/drivers/tty/serial/8250/8250_lpss.c
index 53ca9ba6ab4b..d07e431110d9 100644
--- a/drivers/tty/serial/8250/8250_lpss.c
+++ b/drivers/tty/serial/8250/8250_lpss.c
@@ -169,10 +169,12 @@ static void qrk_serial_setup_dma(struct lpss8250 *lpss, struct uart_port *port)
struct pci_dev *pdev = to_pci_dev(port->dev);
int ret;
+ chip->pdata = &qrk_serial_dma_pdata;
chip->dev = &pdev->dev;
chip->irq = pci_irq_vector(pdev, 0);
chip->regs = pci_ioremap_bar(pdev, 1);
- chip->pdata = &qrk_serial_dma_pdata;
+ if (!chip->regs)
+ return;
/* Falling back to PIO mode if DMA probing fails */
ret = dw_dma_probe(chip);
@@ -195,11 +197,15 @@ static void qrk_serial_setup_dma(struct lpss8250 *lpss, struct uart_port *port)
static void qrk_serial_exit_dma(struct lpss8250 *lpss)
{
+ struct dw_dma_chip *chip = &lpss->dma_chip;
struct dw_dma_slave *param = &lpss->dma_param;
if (!param->dma_dev)
return;
- dw_dma_remove(&lpss->dma_chip);
+
+ dw_dma_remove(chip);
+
+ pci_iounmap(to_pci_dev(chip->dev), chip->regs);
}
#else /* CONFIG_SERIAL_8250_DMA */
static void qrk_serial_setup_dma(struct lpss8250 *lpss, struct uart_port *port) {}
--
2.17.1
^ permalink raw reply related
* [PATCH] lpss8250_dma_setup: there is memory leak when second allocation fails
From: Navid Emamdoost @ 2019-07-19 18:11 UTC (permalink / raw)
Cc: emamd001, kjlu, smccaman, Navid Emamdoost, Greg Kroah-Hartman,
Jiri Slaby, Andy Shevchenko, Vinod Koul, linux-serial,
linux-kernel
in lpss8250_dma_setup, we need to release the first dma slave object
allocated in case of the second allocation failure.
Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>
---
drivers/tty/serial/8250/8250_lpss.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/serial/8250/8250_lpss.c b/drivers/tty/serial/8250/8250_lpss.c
index d07e431110d9..6e1f86db88b2 100644
--- a/drivers/tty/serial/8250/8250_lpss.c
+++ b/drivers/tty/serial/8250/8250_lpss.c
@@ -259,8 +259,10 @@ static int lpss8250_dma_setup(struct lpss8250 *lpss, struct uart_8250_port *port
return -ENOMEM;
tx_param = devm_kzalloc(dev, sizeof(*tx_param), GFP_KERNEL);
- if (!tx_param)
+ if (!tx_param) {
+ kfree(rx_param);
return -ENOMEM;
+ }
*rx_param = lpss->dma_param;
dma->rxconf.src_maxburst = lpss->dma_maxburst;
--
2.17.1
^ permalink raw reply related
* Re: [PATCH v4 1/3] serial: imx: set_termios(): do not enable autoRTS if RTS is unset
From: Uwe Kleine-König @ 2019-07-19 20:19 UTC (permalink / raw)
To: Sergey Organov
Cc: linux-serial, Greg Kroah-Hartman, Sascha Hauer, NXP Linux Team,
Pengutronix Kernel Team, linux-arm-kernel
In-Reply-To: <87woge9hvz.fsf@osv.gnss.ru>
On Fri, Jul 19, 2019 at 06:13:52PM +0300, Sergey Organov wrote:
> Hello Uwe,
>
> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
> > Hello Sergey,
> >
> > On Fri, Jul 19, 2019 at 03:18:13PM +0300, Sergey Organov wrote:
> >> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
> >> > On Fri, Jul 19, 2019 at 11:47:52AM +0300, Sergey Organov wrote:
> >> >> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> >> >> index 57d6e6b..95d7984 100644
> >> >> --- a/drivers/tty/serial/imx.c
> >> >> +++ b/drivers/tty/serial/imx.c
> >> >> @@ -405,7 +405,8 @@ static void imx_uart_rts_inactive(struct imx_port *sport, u32 *ucr2)
> >> >> /* called with port.lock taken and irqs caller dependent */
> >> >> static void imx_uart_rts_auto(struct imx_port *sport, u32 *ucr2)
> >> >> {
> >> >> - *ucr2 |= UCR2_CTSC;
> >> >> + if (*ucr2 & UCR2_CTS)
> >> >> + *ucr2 |= UCR2_CTSC;
> >> >
> >> > I think this patch is wrong or the commit log is insufficient.
> >> > imx_uart_rts_auto() has only a single caller and there ucr2 & UCR2_CTS is
> >> > never true. And CTSC isn't restored anywhere, is it?
> >>
> >> This is rebase to 'tty-next' branch, and you need to look at it in that
> >> context. There, ucr2 & UCR2_CTS does already make sense, due to previous
> >> fix that is already there.
> >
> > I looked at 57d6e6b which is the file you patched. And there
> > imx_uart_rts_auto is only ever called with *ucr2 not having UCR2_CTS.
> >
> > If you still think I'm wrong, please improve the commit log
> > accordingly.
>
> I still think you are wrong, but I don't know how to improve commit log.
>
> To check it once again, I just did:
>
> $ git show 57d6e6b > imx.c
>
> There, in imx_uart_set_termios(), I see:
>
> 1569: old_ucr2 = imx_uart_readl(sport, UCR2);
> 1570: ucr2 = old_ucr2 & (UCR2_TXEN | UCR2_RXEN | UCR2_ATEN | UCR2_CTS);
>
> Here, current UCR2 value is read into 'old_ucr2' and then its /current/
> UCR2_CTS bit is copied into 'ucr2' (along with 3 other bits).
>
> Then, later in the same function:
>
> 1591: imx_uart_rts_auto(sport, &ucr2);
>
> is called that can check /current/ state of UCR2_CTS bit in '*ucr2'.
>
> That's what the patch does, checks this bit.
>
> Sorry, I fail to see how you came to conclusion that "*ucr2 not having
> UCR2_CTS". Do we maybe still read different versions of the file?
No, it's just that I failed to see that UCR2_CTS is in the set of bits
that are retained even when looking twice :-|
So you convinced me that you are right and if you update the commit log
as agreed already before and even add a comment in imx_uart_rts_auto
along the lines of
/*
* Only let the receiver control RTS output if we were not
* requested to have RTS inactive (which then should take
* precedence).
*/
you can have my Ack.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* Re: [PATCH] lpss8250_dma_setup: there is memory leak when second allocation fails
From: Andy Shevchenko @ 2019-07-19 22:35 UTC (permalink / raw)
To: Navid Emamdoost
Cc: emamd001, Kangjie Lu, smccaman, Greg Kroah-Hartman, Jiri Slaby,
Andy Shevchenko, Vinod Koul, open list:SERIAL DRIVERS,
Linux Kernel Mailing List
In-Reply-To: <20190719181200.25607-1-navid.emamdoost@gmail.com>
On Sat, Jul 20, 2019 at 1:23 AM Navid Emamdoost
<navid.emamdoost@gmail.com> wrote:
>
> in lpss8250_dma_setup, we need to release the first dma slave object
> allocated in case of the second allocation failure.
>
This will bring a double free instead of fixing anything.
NAK.
> Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>
> ---
> drivers/tty/serial/8250/8250_lpss.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/8250/8250_lpss.c b/drivers/tty/serial/8250/8250_lpss.c
> index d07e431110d9..6e1f86db88b2 100644
> --- a/drivers/tty/serial/8250/8250_lpss.c
> +++ b/drivers/tty/serial/8250/8250_lpss.c
> @@ -259,8 +259,10 @@ static int lpss8250_dma_setup(struct lpss8250 *lpss, struct uart_8250_port *port
> return -ENOMEM;
>
> tx_param = devm_kzalloc(dev, sizeof(*tx_param), GFP_KERNEL);
> - if (!tx_param)
> + if (!tx_param) {
> + kfree(rx_param);
> return -ENOMEM;
> + }
>
> *rx_param = lpss->dma_param;
> dma->rxconf.src_maxburst = lpss->dma_maxburst;
> --
> 2.17.1
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v2] 8250_lpss: check null return when calling pci_ioremap_bar
From: Andy Shevchenko @ 2019-07-19 22:36 UTC (permalink / raw)
To: Navid Emamdoost
Cc: Andy Shevchenko, emamd001, Kangjie Lu, smccaman,
Greg Kroah-Hartman, Jiri Slaby, Vinod Koul,
open list:SERIAL DRIVERS, Linux Kernel Mailing List
In-Reply-To: <20190719174848.24216-1-navid.emamdoost@gmail.com>
On Sat, Jul 20, 2019 at 12:45 AM Navid Emamdoost
<navid.emamdoost@gmail.com> wrote:
>
> pci_ioremap_bar may return null. This is eventually de-referenced at
> drivers/dma/dw/core.c:1154 and drivers/dma/dw/core.c:1168. A null check
> is needed to prevent null de-reference. I am adding the check and in case
> of failure. Thanks to Andy Shevchenko for the hint on the necessity of
> pci_iounmap when exiting.
>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>
> ---
> drivers/tty/serial/8250/8250_lpss.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serial/8250/8250_lpss.c b/drivers/tty/serial/8250/8250_lpss.c
> index 53ca9ba6ab4b..d07e431110d9 100644
> --- a/drivers/tty/serial/8250/8250_lpss.c
> +++ b/drivers/tty/serial/8250/8250_lpss.c
> @@ -169,10 +169,12 @@ static void qrk_serial_setup_dma(struct lpss8250 *lpss, struct uart_port *port)
> struct pci_dev *pdev = to_pci_dev(port->dev);
> int ret;
>
> + chip->pdata = &qrk_serial_dma_pdata;
> chip->dev = &pdev->dev;
> chip->irq = pci_irq_vector(pdev, 0);
> chip->regs = pci_ioremap_bar(pdev, 1);
> - chip->pdata = &qrk_serial_dma_pdata;
> + if (!chip->regs)
> + return;
>
> /* Falling back to PIO mode if DMA probing fails */
> ret = dw_dma_probe(chip);
> @@ -195,11 +197,15 @@ static void qrk_serial_setup_dma(struct lpss8250 *lpss, struct uart_port *port)
>
> static void qrk_serial_exit_dma(struct lpss8250 *lpss)
> {
> + struct dw_dma_chip *chip = &lpss->dma_chip;
> struct dw_dma_slave *param = &lpss->dma_param;
>
> if (!param->dma_dev)
> return;
> - dw_dma_remove(&lpss->dma_chip);
> +
> + dw_dma_remove(chip);
> +
> + pci_iounmap(to_pci_dev(chip->dev), chip->regs);
> }
> #else /* CONFIG_SERIAL_8250_DMA */
> static void qrk_serial_setup_dma(struct lpss8250 *lpss, struct uart_port *port) {}
> --
> 2.17.1
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v4 1/3] serial: imx: set_termios(): do not enable autoRTS if RTS is unset
From: Sergey Organov @ 2019-07-22 7:42 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: linux-serial, Greg Kroah-Hartman, Sascha Hauer, NXP Linux Team,
Pengutronix Kernel Team, linux-arm-kernel
In-Reply-To: <20190719201949.ldqlcwjhcmt7wwhg@pengutronix.de>
Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
> On Fri, Jul 19, 2019 at 06:13:52PM +0300, Sergey Organov wrote:
>> Hello Uwe,
>>
>> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
>> > Hello Sergey,
>> >
>> > On Fri, Jul 19, 2019 at 03:18:13PM +0300, Sergey Organov wrote:
>> >> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
>> >> > On Fri, Jul 19, 2019 at 11:47:52AM +0300, Sergey Organov wrote:
>> >> >> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
>> >> >> index 57d6e6b..95d7984 100644
>> >> >> --- a/drivers/tty/serial/imx.c
>> >> >> +++ b/drivers/tty/serial/imx.c
>> >> >> @@ -405,7 +405,8 @@ static void imx_uart_rts_inactive(struct imx_port *sport, u32 *ucr2)
>> >> >> /* called with port.lock taken and irqs caller dependent */
>> >> >> static void imx_uart_rts_auto(struct imx_port *sport, u32 *ucr2)
>> >> >> {
>> >> >> - *ucr2 |= UCR2_CTSC;
>> >> >> + if (*ucr2 & UCR2_CTS)
>> >> >> + *ucr2 |= UCR2_CTSC;
>> >> >
>> >> > I think this patch is wrong or the commit log is insufficient.
>> >> > imx_uart_rts_auto() has only a single caller and there ucr2 & UCR2_CTS is
>> >> > never true. And CTSC isn't restored anywhere, is it?
>> >>
>> >> This is rebase to 'tty-next' branch, and you need to look at it in that
>> >> context. There, ucr2 & UCR2_CTS does already make sense, due to previous
>> >> fix that is already there.
>> >
>> > I looked at 57d6e6b which is the file you patched. And there
>> > imx_uart_rts_auto is only ever called with *ucr2 not having UCR2_CTS.
>> >
>> > If you still think I'm wrong, please improve the commit log
>> > accordingly.
>>
>> I still think you are wrong, but I don't know how to improve commit log.
>>
>> To check it once again, I just did:
>>
>> $ git show 57d6e6b > imx.c
>>
>> There, in imx_uart_set_termios(), I see:
>>
>> 1569: old_ucr2 = imx_uart_readl(sport, UCR2);
>> 1570: ucr2 = old_ucr2 & (UCR2_TXEN | UCR2_RXEN | UCR2_ATEN | UCR2_CTS);
>>
>> Here, current UCR2 value is read into 'old_ucr2' and then its /current/
>> UCR2_CTS bit is copied into 'ucr2' (along with 3 other bits).
>>
>> Then, later in the same function:
>>
>> 1591: imx_uart_rts_auto(sport, &ucr2);
>>
>> is called that can check /current/ state of UCR2_CTS bit in '*ucr2'.
>>
>> That's what the patch does, checks this bit.
>>
>> Sorry, I fail to see how you came to conclusion that "*ucr2 not having
>> UCR2_CTS". Do we maybe still read different versions of the file?
>
> No, it's just that I failed to see that UCR2_CTS is in the set of bits
> that are retained even when looking twice :-|
Ah, that one... How familiar :-)
> So you convinced me that you are right and if you update the commit log
> as agreed already before and even add a comment in imx_uart_rts_auto
> along the lines of
>
> /*
> * Only let the receiver control RTS output if we were not
> * requested to have RTS inactive (which then should take
> * precedence).
> */
>
> you can have my Ack.
OK, will do
Thanks!
-- Sergey
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v4 1/3] serial: imx: set_termios(): do not enable autoRTS if RTS is unset
From: Uwe Kleine-König @ 2019-07-22 7:51 UTC (permalink / raw)
To: Sergey Organov
Cc: linux-serial, Greg Kroah-Hartman, Sascha Hauer, NXP Linux Team,
Pengutronix Kernel Team, linux-arm-kernel
In-Reply-To: <87ftmy8qgu.fsf@osv.gnss.ru>
On Mon, Jul 22, 2019 at 10:42:57AM +0300, Sergey Organov wrote:
> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
>
> > On Fri, Jul 19, 2019 at 06:13:52PM +0300, Sergey Organov wrote:
> >> Hello Uwe,
> >>
> >> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
> >> > Hello Sergey,
> >> >
> >> > On Fri, Jul 19, 2019 at 03:18:13PM +0300, Sergey Organov wrote:
> >> >> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
> >> >> > On Fri, Jul 19, 2019 at 11:47:52AM +0300, Sergey Organov wrote:
> >> >> >> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> >> >> >> index 57d6e6b..95d7984 100644
> >> >> >> --- a/drivers/tty/serial/imx.c
> >> >> >> +++ b/drivers/tty/serial/imx.c
> >> >> >> @@ -405,7 +405,8 @@ static void imx_uart_rts_inactive(struct imx_port *sport, u32 *ucr2)
> >> >> >> /* called with port.lock taken and irqs caller dependent */
> >> >> >> static void imx_uart_rts_auto(struct imx_port *sport, u32 *ucr2)
> >> >> >> {
> >> >> >> - *ucr2 |= UCR2_CTSC;
> >> >> >> + if (*ucr2 & UCR2_CTS)
> >> >> >> + *ucr2 |= UCR2_CTSC;
> >> >> >
> >> >> > I think this patch is wrong or the commit log is insufficient.
> >> >> > imx_uart_rts_auto() has only a single caller and there ucr2 & UCR2_CTS is
> >> >> > never true. And CTSC isn't restored anywhere, is it?
> >> >>
> >> >> This is rebase to 'tty-next' branch, and you need to look at it in that
> >> >> context. There, ucr2 & UCR2_CTS does already make sense, due to previous
> >> >> fix that is already there.
> >> >
> >> > I looked at 57d6e6b which is the file you patched. And there
> >> > imx_uart_rts_auto is only ever called with *ucr2 not having UCR2_CTS.
> >> >
> >> > If you still think I'm wrong, please improve the commit log
> >> > accordingly.
> >>
> >> I still think you are wrong, but I don't know how to improve commit log.
> >>
> >> To check it once again, I just did:
> >>
> >> $ git show 57d6e6b > imx.c
> >>
> >> There, in imx_uart_set_termios(), I see:
> >>
> >> 1569: old_ucr2 = imx_uart_readl(sport, UCR2);
> >> 1570: ucr2 = old_ucr2 & (UCR2_TXEN | UCR2_RXEN | UCR2_ATEN | UCR2_CTS);
> >>
> >> Here, current UCR2 value is read into 'old_ucr2' and then its /current/
> >> UCR2_CTS bit is copied into 'ucr2' (along with 3 other bits).
> >>
> >> Then, later in the same function:
> >>
> >> 1591: imx_uart_rts_auto(sport, &ucr2);
> >>
> >> is called that can check /current/ state of UCR2_CTS bit in '*ucr2'.
> >>
> >> That's what the patch does, checks this bit.
> >>
> >> Sorry, I fail to see how you came to conclusion that "*ucr2 not having
> >> UCR2_CTS". Do we maybe still read different versions of the file?
> >
> > No, it's just that I failed to see that UCR2_CTS is in the set of bits
> > that are retained even when looking twice :-|
>
> Ah, that one... How familiar :-)
I thought again a bit over the weekend about this. I wonder if it's
correct to keep RTS active while going through .set_termios. Shouldn't
it maybe always be inactive to prevent the other side sending data while
we are changing the baud rate?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* Re: [PATCH v4 1/3] serial: imx: set_termios(): do not enable autoRTS if RTS is unset
From: Sergey Organov @ 2019-07-22 9:20 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: linux-serial, Greg Kroah-Hartman, Sascha Hauer, NXP Linux Team,
Pengutronix Kernel Team, linux-arm-kernel
In-Reply-To: <20190722075107.cc3tvf6gmxhaeh5m@pengutronix.de>
Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
> On Mon, Jul 22, 2019 at 10:42:57AM +0300, Sergey Organov wrote:
>> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
>>
>> > On Fri, Jul 19, 2019 at 06:13:52PM +0300, Sergey Organov wrote:
>> >> Hello Uwe,
>> >>
>> >> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
>> >> > Hello Sergey,
>> >> >
>> >> > On Fri, Jul 19, 2019 at 03:18:13PM +0300, Sergey Organov wrote:
>> >> >> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
>> >> >> > On Fri, Jul 19, 2019 at 11:47:52AM +0300, Sergey Organov wrote:
>> >> >> >> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
>> >> >> >> index 57d6e6b..95d7984 100644
>> >> >> >> --- a/drivers/tty/serial/imx.c
>> >> >> >> +++ b/drivers/tty/serial/imx.c
>> >> >> >> @@ -405,7 +405,8 @@ static void imx_uart_rts_inactive(struct imx_port *sport, u32 *ucr2)
>> >> >> >> /* called with port.lock taken and irqs caller dependent */
>> >> >> >> static void imx_uart_rts_auto(struct imx_port *sport, u32 *ucr2)
>> >> >> >> {
>> >> >> >> - *ucr2 |= UCR2_CTSC;
>> >> >> >> + if (*ucr2 & UCR2_CTS)
>> >> >> >> + *ucr2 |= UCR2_CTSC;
>> >> >> >
>> >> >> > I think this patch is wrong or the commit log is insufficient.
>> >> >> > imx_uart_rts_auto() has only a single caller and there ucr2 & UCR2_CTS is
>> >> >> > never true. And CTSC isn't restored anywhere, is it?
>> >> >>
>> >> >> This is rebase to 'tty-next' branch, and you need to look at it in that
>> >> >> context. There, ucr2 & UCR2_CTS does already make sense, due to previous
>> >> >> fix that is already there.
>> >> >
>> >> > I looked at 57d6e6b which is the file you patched. And there
>> >> > imx_uart_rts_auto is only ever called with *ucr2 not having UCR2_CTS.
>> >> >
>> >> > If you still think I'm wrong, please improve the commit log
>> >> > accordingly.
>> >>
>> >> I still think you are wrong, but I don't know how to improve commit log.
>> >>
>> >> To check it once again, I just did:
>> >>
>> >> $ git show 57d6e6b > imx.c
>> >>
>> >> There, in imx_uart_set_termios(), I see:
>> >>
>> >> 1569: old_ucr2 = imx_uart_readl(sport, UCR2);
>> >> 1570: ucr2 = old_ucr2 & (UCR2_TXEN | UCR2_RXEN | UCR2_ATEN | UCR2_CTS);
>> >>
>> >> Here, current UCR2 value is read into 'old_ucr2' and then its /current/
>> >> UCR2_CTS bit is copied into 'ucr2' (along with 3 other bits).
>> >>
>> >> Then, later in the same function:
>> >>
>> >> 1591: imx_uart_rts_auto(sport, &ucr2);
>> >>
>> >> is called that can check /current/ state of UCR2_CTS bit in '*ucr2'.
>> >>
>> >> That's what the patch does, checks this bit.
>> >>
>> >> Sorry, I fail to see how you came to conclusion that "*ucr2 not having
>> >> UCR2_CTS". Do we maybe still read different versions of the file?
>> >
>> > No, it's just that I failed to see that UCR2_CTS is in the set of bits
>> > that are retained even when looking twice :-|
>>
>> Ah, that one... How familiar :-)
>
> I thought again a bit over the weekend about this. I wonder if it's
> correct to keep RTS active while going through .set_termios. Shouldn't
> it maybe always be inactive to prevent the other side sending data while
> we are changing the baud rate?
I don't think it's a good idea to change RTS state over .set_terimios,
as it doesn't in fact solve anything (notice that the other end should
also change baud rate accordingly), and changes visible state (even if
temporarily) that it was not asked to change, that could in turn lead to
utter surprises.
Correct changing of baud rates, bits, etc., could only be implemented at
communication protocol level (read: application level), and there are
all the tools in the kernel to do it right, provided driver does not do
what it was not asked to do.
-- Sergey
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH v5 0/3] serial: imx: fix RTS and RTS/CTS handling
From: Sergey Organov @ 2019-07-22 9:22 UTC (permalink / raw)
To: linux-serial
Cc: Greg Kroah-Hartman, Sascha Hauer, Sergey Organov, NXP Linux Team,
Pengutronix Kernel Team, Uwe Kleine-König, linux-arm-kernel
In-Reply-To: <20190614072801.3187-1-s.hauer@pengutronix.de>
This is rebase of v3 on top of 'tty-next', to get rid of commits that
are already adopted to mainline.
RTS signal and RTS/CTS handshake handling had a few problems these
patches fix.
In addition, minor cleanups are made to the involved code.
Changelog:
v5:
* improved commit description and added more comments for
"serial: imx: set_termios(): do not enable autoRTS if RTS is
unset" as suggested by
Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
and added corresponding "Reviewed-by:"
v4:
* rebased on top of 'tty-next', to get rid of commits that
are already adopted to mainline.
v3:
* Improved comments in "serial: imx: set_mctrl(): correctly
restore autoRTS state", as suggested by Uwe Kleine-König
<u.kleine-koenig@pengutronix.de>
v2:
* Appended: "Reviewed-by:" and "Tested-by:"
Sascha Hauer <s.hauer@pengutronix.de>
* Removed "RFC" from header
v1:
* Fixed in "serial: imx: set_termios(): preserve RTS state"
-+ ucr2 = UCR2_SRST | UCR2_IRTS;
++ ucr2 |= UCR2_SRST | UCR2_IRTS;
as noticed by Lothar Waßmann <LW@KARO-electronics.de>
* Fixed in "serial: imx: set_termios(): preserve RTS state"
-+ ucr2 = old_ucr2 & (UCR2_TXEN | UCR2_RXEN | UCR2_ATEN | UCR2_CTSC);
++ ucr2 = old_ucr2 & (UCR2_TXEN | UCR2_RXEN | UCR2_ATEN | UCR2_CTS);
as the fix for the problem found by Sascha Hauer
<s.hauer@pengutronix.de>
* Reordered:
serial: imx: set_termios(): preserve RTS state
serial: imx: set_termios(): do not enable autoRTS if RTS is unset
as the latter makes sense only provided the former is already
applied.
Sergey Organov (3):
serial: imx: set_termios(): do not enable autoRTS if RTS is unset
serial: imx: set_mctrl(): correctly restore autoRTS state
serial: imx: get rid of imx_uart_rts_auto()
drivers/tty/serial/imx.c | 28 ++++++++++++++++++----------
1 file changed, 18 insertions(+), 10 deletions(-)
--
2.10.0.1.g57b01a3
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH v5 1/3] serial: imx: set_termios(): do not enable autoRTS if RTS is unset
From: Sergey Organov @ 2019-07-22 9:22 UTC (permalink / raw)
To: linux-serial
Cc: Greg Kroah-Hartman, Sascha Hauer, Sergey Organov, NXP Linux Team,
Pengutronix Kernel Team, Uwe Kleine-König, linux-arm-kernel
In-Reply-To: <1563787330-1394-1-git-send-email-sorganov@gmail.com>
Don't let receiver hardware automatically control RTS output if it
was requested to be inactive.
To ensure this, set_termios() shouldn't set UCR2_CTSC bit if UCR2_CTS
(=TIOCM_RTS) is cleared. Added corresponding check in imx_uart_rts_auto()
to fix this.
Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Reviewed-by: Sascha Hauer <s.hauer@pengutronix.de>
Tested-by: Sascha Hauer <s.hauer@pengutronix.de>
Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
drivers/tty/serial/imx.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 57d6e6b..32f36d8 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -405,7 +405,12 @@ static void imx_uart_rts_inactive(struct imx_port *sport, u32 *ucr2)
/* called with port.lock taken and irqs caller dependent */
static void imx_uart_rts_auto(struct imx_port *sport, u32 *ucr2)
{
- *ucr2 |= UCR2_CTSC;
+ /*
+ * Only let receiver control RTS output if we were not requested to have
+ * RTS inactive (which then should take precedence).
+ */
+ if (*ucr2 & UCR2_CTS)
+ *ucr2 |= UCR2_CTSC;
}
/* called with port.lock taken and irqs off */
--
2.10.0.1.g57b01a3
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
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