* [PATCH] tty: serial_core: recover uport->cons->cflag on uart_close
From: kpark3469 @ 2019-06-16 14:57 UTC (permalink / raw)
To: gregkh; +Cc: jslaby, linux-serial, linux-kernel, keun-o.park
From: Sahara <keun-o.park@darkmatter.ae>
Since uart_close was converted to use tty_port_close, uart_shutdown
also moved to uart_tty_port_shutdown, which means it does not backup
tty's termios to uart_port.console.cflag when console is closed and
uart_console is true.
By losing this value, serial console was not set correctly especially
after suspend/resume when there is no consumer of console device.
This problem resets console driver's configuration to an unwanted value
and may give a performance regression in the system eventually.
This patch fixes the bug introduced from v4.9 kernel.
Fixes: 761ed4a94582 ("tty: serial_core: convert uart_close to use tty_port_close")
Reported-by: Jouni Linnamaa <Jouni.Linnamaa@darkmatter.ae>
Signed-off-by: Sahara <keun-o.park@darkmatter.ae>
---
drivers/tty/serial/serial_core.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 83f4dd0bfd74..a52afceb2f4e 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -1533,6 +1533,7 @@ static void uart_set_termios(struct tty_struct *tty,
static void uart_close(struct tty_struct *tty, struct file *filp)
{
struct uart_state *state = tty->driver_data;
+ struct uart_port *uport = uart_port_check(state);
if (!state) {
struct uart_driver *drv = tty->driver->driver_state;
@@ -1548,6 +1549,8 @@ static void uart_close(struct tty_struct *tty, struct file *filp)
pr_debug("uart_close(%d) called\n", tty->index);
+ if (uport && uart_console(uport))
+ uport->cons->cflag = tty->termios.c_cflag;
tty_port_close(tty->port, tty, filp);
}
--
2.17.1
^ permalink raw reply related
* Re: [PATCH 1/1] dt-bindings: stm32: serial: Add optional reset
From: Rob Herring @ 2019-06-14 17:51 UTC (permalink / raw)
Cc: Greg Kroah-Hartman, Rob Herring, Mark Rutland, Maxime Coquelin,
Alexandre Torgue, linux-serial, devicetree, linux-stm32,
linux-arm-kernel, linux-kernel, Fabrice Gasnier, Erwan Le Ray
In-Reply-To: <1558711838-21174-1-git-send-email-erwan.leray@st.com>
On Fri, 24 May 2019 17:30:38 +0200, Erwan Le Ray wrote:
> STM32 serial can be reset via reset controller.
> Add an optional reset property to stm32 usart bindings.
>
> Signed-off-by: Erwan Le Ray <erwan.leray@st.com>
>
Applied, thanks.
Rob
^ permalink raw reply
* Re: [PATCH 2/6] dt-bindings: serial: 8250_omap: Add compatible for J721E UART controller
From: Rob Herring @ 2019-06-14 16:45 UTC (permalink / raw)
Cc: Arnd Bergmann, Olof Johansson, Santosh Shilimkar, Will Deacon,
Catalin Marinas, Greg Kroah-Hartman, Mark Rutland, Rob Herring,
linux-serial, linux-kernel, devicetree, linux-arm-kernel,
Tony Lindgren, Russell King, Tero Kristo, Nishanth Menon,
Sekhar Nori, Vignesh R
In-Reply-To: <20190522161921.20750-3-nm@ti.com>
On Wed, 22 May 2019 11:19:17 -0500, Nishanth Menon wrote:
> J721e uses a UART controller that is compatible with AM654 UART.
> Introduce a specific compatible to help handle the differences if
> necessary.
>
> Cc: Sekhar Nori <nsekhar@ti.com>
> Cc: Vignesh R <vigneshr@ti.com>
> Signed-off-by: Nishanth Menon <nm@ti.com>
> ---
> NOTE:
> - If Greg is ok, we can pick up the uart compatibility via the k3 tree,
> else, I can spawn it off the series into it's own patch, but it
> seemed better in a logical order.
>
> Documentation/devicetree/bindings/serial/omap_serial.txt | 1 +
> 1 file changed, 1 insertion(+)
>
Reviewed-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* Re: [PATCH 1/6] dt-bindings: arm: ti: Add bindings for J721E SoC
From: Rob Herring @ 2019-06-14 16:45 UTC (permalink / raw)
Cc: Arnd Bergmann, Olof Johansson, Santosh Shilimkar, Will Deacon,
Catalin Marinas, Greg Kroah-Hartman, Mark Rutland, Rob Herring,
linux-serial, linux-kernel, devicetree, linux-arm-kernel,
Tony Lindgren, Russell King, Tero Kristo, Nishanth Menon
In-Reply-To: <20190522161921.20750-2-nm@ti.com>
On Wed, 22 May 2019 11:19:16 -0500, Nishanth Menon wrote:
> The J721E SoC belongs to the K3 Multicore SoC architecture platform,
> providing advanced system integration to enable lower system costs
> of automotive applications such as infotainment, cluster, premium
> Audio, Gateway, industrial and a range of broad market applications.
> This SoC is designed around reducing the system cost by eliminating
> the need of an external system MCU and is targeted towards ASIL-B/C
> certification/requirements in addition to allowing complex software
> and system use-cases.
>
> Some highlights of this SoC are:
> * Dual Cortex-A72s in a single cluster, three clusters of lockstep
> capable dual Cortex-R5F MCUs, Deep-learning Matrix Multiply Accelerator(MMA),
> C7x floating point Vector DSP, Two C66x floating point DSPs.
> * 3D GPU PowerVR Rogue 8XE GE8430
> * Vision Processing Accelerator (VPAC) with image signal processor and Depth
> and Motion Processing Accelerator (DMPAC)
> * Two Gigabit Industrial Communication Subsystems (ICSSG), each with dual
> PRUs and dual RTUs
> * Two CSI2.0 4L RX plus one CSI2.0 4L TX, one eDP/DP, One DSI Tx, and
> up to two DPI interfaces.
> * Integrated Ethernet switch supporting up to a total of 8 external ports in
> addition to legacy Ethernet switch of up to 2 ports.
> * System MMU (SMMU) Version 3.0 and advanced virtualisation
> capabilities.
> * Upto 4 PCIe-GEN3 controllers, 2 USB3.0 Dual-role device subsystems,
> 16 MCANs, 12 McASP, eMMC and SD, UFS, OSPI/HyperBus memory controller, QSPI,
> I3C and I2C, eCAP/eQEP, eHRPWM, MLB among other peripherals.
> * Two hardware accelerator block containing AES/DES/SHA/MD5 called SA2UL
> management.
> * Configurable L3 Cache and IO-coherent architecture with high data throughput
> capable distributed DMA architecture under NAVSS
> * Centralized System Controller for Security, Power, and Resource
> Management (DMSC)
>
> See J721E Technical Reference Manual (SPRUIL1, May 2019)
> for further details: http://www.ti.com/lit/pdf/spruil1
>
> Signed-off-by: Nishanth Menon <nm@ti.com>
> ---
> Documentation/devicetree/bindings/arm/ti/k3.txt | 3 +++
> 1 file changed, 3 insertions(+)
>
Reviewed-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* Re: [PATCH 1/6] dt-bindings: arm: ti: Add bindings for J721E SoC
From: Rob Herring @ 2019-06-14 16:45 UTC (permalink / raw)
To: Nishanth Menon
Cc: Arnd Bergmann, Olof Johansson, Santosh Shilimkar, Will Deacon,
Catalin Marinas, Greg Kroah-Hartman, Mark Rutland, linux-serial,
linux-kernel, devicetree, linux-arm-kernel, Tony Lindgren,
Russell King, Tero Kristo
In-Reply-To: <20190522161921.20750-2-nm@ti.com>
On Wed, May 22, 2019 at 11:19:16AM -0500, Nishanth Menon wrote:
> The J721E SoC belongs to the K3 Multicore SoC architecture platform,
> providing advanced system integration to enable lower system costs
> of automotive applications such as infotainment, cluster, premium
> Audio, Gateway, industrial and a range of broad market applications.
> This SoC is designed around reducing the system cost by eliminating
> the need of an external system MCU and is targeted towards ASIL-B/C
> certification/requirements in addition to allowing complex software
> and system use-cases.
>
> Some highlights of this SoC are:
> * Dual Cortex-A72s in a single cluster, three clusters of lockstep
> capable dual Cortex-R5F MCUs, Deep-learning Matrix Multiply Accelerator(MMA),
> C7x floating point Vector DSP, Two C66x floating point DSPs.
> * 3D GPU PowerVR Rogue 8XE GE8430
> * Vision Processing Accelerator (VPAC) with image signal processor and Depth
> and Motion Processing Accelerator (DMPAC)
> * Two Gigabit Industrial Communication Subsystems (ICSSG), each with dual
> PRUs and dual RTUs
> * Two CSI2.0 4L RX plus one CSI2.0 4L TX, one eDP/DP, One DSI Tx, and
> up to two DPI interfaces.
> * Integrated Ethernet switch supporting up to a total of 8 external ports in
> addition to legacy Ethernet switch of up to 2 ports.
> * System MMU (SMMU) Version 3.0 and advanced virtualisation
> capabilities.
> * Upto 4 PCIe-GEN3 controllers, 2 USB3.0 Dual-role device subsystems,
> 16 MCANs, 12 McASP, eMMC and SD, UFS, OSPI/HyperBus memory controller, QSPI,
> I3C and I2C, eCAP/eQEP, eHRPWM, MLB among other peripherals.
> * Two hardware accelerator block containing AES/DES/SHA/MD5 called SA2UL
> management.
> * Configurable L3 Cache and IO-coherent architecture with high data throughput
> capable distributed DMA architecture under NAVSS
> * Centralized System Controller for Security, Power, and Resource
> Management (DMSC)
>
> See J721E Technical Reference Manual (SPRUIL1, May 2019)
> for further details: http://www.ti.com/lit/pdf/spruil1
>
> Signed-off-by: Nishanth Menon <nm@ti.com>
> ---
> Documentation/devicetree/bindings/arm/ti/k3.txt | 3 +++
> 1 file changed, 3 insertions(+)
Okay for now, but please convert K3 and other TI SoCs to schema soon.
Reviewed-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* [PATCH v1] tty: serial: max310x: Add optional reset gpio
From: Mylène Josserand @ 2019-06-14 14:11 UTC (permalink / raw)
To: gregkh, robh+dt, mark.rutland
Cc: linux-serial, devicetree, linux-kernel, mylene.josserand,
thomas.petazzoni
Add the possibility to use a gpio as reset.
Signed-off-by: Mylène Josserand <mylene.josserand@bootlin.com>
---
Documentation/devicetree/bindings/serial/maxim,max310x.txt | 1 +
drivers/tty/serial/max310x.c | 7 +++++++
2 files changed, 8 insertions(+)
diff --git a/Documentation/devicetree/bindings/serial/maxim,max310x.txt b/Documentation/devicetree/bindings/serial/maxim,max310x.txt
index 79e10a05a96a..1d7d8a0b4260 100644
--- a/Documentation/devicetree/bindings/serial/maxim,max310x.txt
+++ b/Documentation/devicetree/bindings/serial/maxim,max310x.txt
@@ -15,6 +15,7 @@ Required properties:
"osc" if an external clock source is used.
Optional properties:
+- reset-gpios: Gpio to use for reset.
- gpio-controller: Marks the device node as a GPIO controller.
- #gpio-cells: Should be two. The first cell is the GPIO number and
the second cell is used to specify the GPIO polarity:
diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
index e5aebbf5f302..d056fa2eed1b 100644
--- a/drivers/tty/serial/max310x.c
+++ b/drivers/tty/serial/max310x.c
@@ -14,6 +14,7 @@
#include <linux/delay.h>
#include <linux/device.h>
#include <linux/gpio/driver.h>
+#include <linux/gpio/consumer.h>
#include <linux/module.h>
#include <linux/of.h>
#include <linux/of_device.h>
@@ -1413,12 +1414,18 @@ static int max310x_spi_probe(struct spi_device *spi)
return ret;
if (spi->dev.of_node) {
+ struct gpio_desc *reset_gpio;
const struct of_device_id *of_id =
of_match_device(max310x_dt_ids, &spi->dev);
if (!of_id)
return -ENODEV;
devtype = (struct max310x_devtype *)of_id->data;
+ reset_gpio = devm_gpiod_get_optional(&spi->dev, "reset",
+ GPIOD_OUT_HIGH);
+ if (IS_ERR(reset_gpio))
+ return PTR_ERR(reset_gpio);
+ gpiod_set_value_cansleep(reset_gpio, 0);
} else {
const struct spi_device_id *id_entry = spi_get_device_id(spi);
--
2.11.0
^ permalink raw reply related
* Re: [PATCH RFC 5/7] serial: imx: set_termios(): preserve RTS state
From: Sergey Organov @ 2019-06-14 13:28 UTC (permalink / raw)
To: Lothar Waßmann
Cc: linux-serial, Sascha Hauer, NXP Linux Team,
Pengutronix Kernel Team, Uwe Kleine-König, linux-arm-kernel
In-Reply-To: <20190614150551.1472b154@karo-electronics.de>
Lothar Waßmann <LW@KARO-electronics.de> writes:
> Hi,
>
> On Fri, 14 Jun 2019 15:11:32 +0300 Sergey Organov wrote:
>> imx_set_termios() cleared RTS on every call, now fixed.
>>
>> Signed-off-by: Sergey Organov <sorganov@gmail.com>
>> ---
>> drivers/tty/serial/imx.c | 12 ++++++++----
>> 1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
>> index 8ee910f..de23068 100644
>> --- a/drivers/tty/serial/imx.c
>> +++ b/drivers/tty/serial/imx.c
>> @@ -1564,6 +1564,13 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
>>
>> spin_lock_irqsave(&sport->port.lock, flags);
>>
>> + /*
>> + * Read current UCR2 and save it for future use, then clear all the bits
>> + * except those we will or may need to preserve.
>> + */
>> + old_ucr2 = imx_uart_readl(sport, UCR2);
>> + ucr2 = old_ucr2 & (UCR2_TXEN | UCR2_RXEN | UCR2_ATEN | UCR2_CTSC);
>> +
>> ucr2 = UCR2_SRST | UCR2_IRTS;
> s/=/|=/
Nice catch!
Thanks,
-- Sergey Organov
_______________________________________________
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 RFC 5/7] serial: imx: set_termios(): preserve RTS state
From: Lothar Waßmann @ 2019-06-14 13:05 UTC (permalink / raw)
To: Sergey Organov
Cc: linux-serial, Sascha Hauer, NXP Linux Team,
Pengutronix Kernel Team, Uwe Kleine-König, linux-arm-kernel
In-Reply-To: <1560514294-29111-6-git-send-email-sorganov@gmail.com>
Hi,
On Fri, 14 Jun 2019 15:11:32 +0300 Sergey Organov wrote:
> imx_set_termios() cleared RTS on every call, now fixed.
>
> Signed-off-by: Sergey Organov <sorganov@gmail.com>
> ---
> drivers/tty/serial/imx.c | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 8ee910f..de23068 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -1564,6 +1564,13 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
>
> spin_lock_irqsave(&sport->port.lock, flags);
>
> + /*
> + * Read current UCR2 and save it for future use, then clear all the bits
> + * except those we will or may need to preserve.
> + */
> + old_ucr2 = imx_uart_readl(sport, UCR2);
> + ucr2 = old_ucr2 & (UCR2_TXEN | UCR2_RXEN | UCR2_ATEN | UCR2_CTSC);
> +
> ucr2 = UCR2_SRST | UCR2_IRTS;
s/=/|=/
Lothar Waßmann
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH RFC 7/7] serial: imx: get rid of imx_uart_rts_auto()
From: Sergey Organov @ 2019-06-14 12:11 UTC (permalink / raw)
To: Sascha Hauer
Cc: Pengutronix Kernel Team, NXP Linux Team, linux-arm-kernel,
linux-serial, Uwe Kleine-König
In-Reply-To: <1560514294-29111-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.
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 bdb8b6a..cb28cff 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)
{
@@ -1598,8 +1591,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
* [PATCH RFC 6/7] serial: imx: set_mctrl(): correctly restore autoRTS state
From: Sergey Organov @ 2019-06-14 12:11 UTC (permalink / raw)
To: Sascha Hauer
Cc: Pengutronix Kernel Team, NXP Linux Team, linux-arm-kernel,
linux-serial, Uwe Kleine-König
In-Reply-To: <1560514294-29111-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.
Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
drivers/tty/serial/imx.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index de23068..bdb8b6a 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -970,10 +970,19 @@ 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 (UCR2_CTSC) if RTS is lowered and restore
+ * autoRTS setting if RTS is raised. Inverted UCR2_IRTS is set
+ * if and only if CRTSCTS bit is set for the port, so we use it
+ * to get the state to restore to.
+ */
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;
+ if (!(ucr2 & UCR2_IRTS))
+ ucr2 |= UCR2_CTSC;
+ }
imx_uart_writel(sport, ucr2, UCR2);
}
--
2.10.0.1.g57b01a3
^ permalink raw reply related
* [PATCH RFC 5/7] serial: imx: set_termios(): preserve RTS state
From: Sergey Organov @ 2019-06-14 12:11 UTC (permalink / raw)
To: Sascha Hauer
Cc: Pengutronix Kernel Team, NXP Linux Team, linux-arm-kernel,
linux-serial, Uwe Kleine-König
In-Reply-To: <1560514294-29111-1-git-send-email-sorganov@gmail.com>
imx_set_termios() cleared RTS on every call, now fixed.
Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
drivers/tty/serial/imx.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 8ee910f..de23068 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -1564,6 +1564,13 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
spin_lock_irqsave(&sport->port.lock, flags);
+ /*
+ * Read current UCR2 and save it for future use, then clear all the bits
+ * except those we will or may need to preserve.
+ */
+ old_ucr2 = imx_uart_readl(sport, UCR2);
+ ucr2 = old_ucr2 & (UCR2_TXEN | UCR2_RXEN | UCR2_ATEN | UCR2_CTSC);
+
ucr2 = UCR2_SRST | UCR2_IRTS;
if ((termios->c_cflag & CSIZE) == CS8)
ucr2 |= UCR2_WS;
@@ -1633,7 +1640,6 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
imx_uart_writel(sport,
old_ucr1 & ~(UCR1_TXMPTYEN | UCR1_RRDYEN | UCR1_RTSDEN),
UCR1);
- old_ucr2 = imx_uart_readl(sport, UCR2);
imx_uart_writel(sport, old_ucr2 & ~UCR2_ATEN, UCR2);
while (!(imx_uart_readl(sport, USR2) & USR2_TXDC))
@@ -1641,7 +1647,6 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
/* then, disable everything */
imx_uart_writel(sport, old_ucr2 & ~(UCR2_TXEN | UCR2_RXEN | UCR2_ATEN), UCR2);
- old_ucr2 &= (UCR2_TXEN | UCR2_RXEN | UCR2_ATEN);
/* custom-baudrate handling */
div = sport->port.uartclk / (baud * 16);
@@ -1679,8 +1684,7 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
imx_uart_writel(sport, old_ucr1, UCR1);
- /* set the parity, stop bits and data size */
- imx_uart_writel(sport, ucr2 | old_ucr2, UCR2);
+ imx_uart_writel(sport, ucr2, UCR2);
if (UART_ENABLE_MS(&sport->port, termios->c_cflag))
imx_uart_enable_ms(&sport->port);
--
2.10.0.1.g57b01a3
^ permalink raw reply related
* [PATCH RFC 4/7] serial: imx: set_termios(): do not enable autoRTS if RTS is unset
From: Sergey Organov @ 2019-06-14 12:11 UTC (permalink / raw)
To: Sascha Hauer
Cc: Pengutronix Kernel Team, NXP Linux Team, linux-arm-kernel,
linux-serial, Uwe Kleine-König
In-Reply-To: <1560514294-29111-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.
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 17e2322..8ee910f 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 RFC 3/7] serial: imx: set_termios(): clarify RTS/CTS bits calculation
From: Sergey Organov @ 2019-06-14 12:11 UTC (permalink / raw)
To: Sascha Hauer
Cc: Pengutronix Kernel Team, NXP Linux Team, linux-arm-kernel,
linux-serial, Uwe Kleine-König
In-Reply-To: <1560514294-29111-1-git-send-email-sorganov@gmail.com>
Avoid repeating the same code for rs485 twice.
Make it obvious we clear CRTSCTS bit in termios->c_cflag whenever
sport->have_rtscts is false.
Make it obvious we clear UCR2_IRTS whenever CRTSCTS is set.
Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
drivers/tty/serial/imx.c | 36 +++++++++++++-----------------------
1 file changed, 13 insertions(+), 23 deletions(-)
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 87802fd..17e2322 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -1567,35 +1567,25 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
if ((termios->c_cflag & CSIZE) == CS8)
ucr2 |= UCR2_WS;
- if (termios->c_cflag & CRTSCTS) {
- if (sport->have_rtscts) {
- ucr2 &= ~UCR2_IRTS;
+ if (!sport->have_rtscts)
+ termios->c_cflag &= ~CRTSCTS;
- if (port->rs485.flags & SER_RS485_ENABLED) {
- /*
- * RTS is mandatory for rs485 operation, so keep
- * it under manual control and keep transmitter
- * disabled.
- */
- if (port->rs485.flags &
- SER_RS485_RTS_AFTER_SEND)
- imx_uart_rts_active(sport, &ucr2);
- else
- imx_uart_rts_inactive(sport, &ucr2);
- } else {
- imx_uart_rts_auto(sport, &ucr2);
- }
- } else {
- termios->c_cflag &= ~CRTSCTS;
- }
- } else if (port->rs485.flags & SER_RS485_ENABLED) {
- /* disable transmitter */
+ if (port->rs485.flags & SER_RS485_ENABLED) {
+ /*
+ * RTS is mandatory for rs485 operation, so keep
+ * it under manual control and keep transmitter
+ * disabled.
+ */
if (port->rs485.flags & SER_RS485_RTS_AFTER_SEND)
imx_uart_rts_active(sport, &ucr2);
else
imx_uart_rts_inactive(sport, &ucr2);
- }
+ } else if (termios->c_cflag & CRTSCTS)
+ imx_uart_rts_auto(sport, &ucr2);
+
+ if (termios->c_cflag & CRTSCTS)
+ ucr2 &= ~UCR2_IRTS;
if (termios->c_cflag & CSTOPB)
ucr2 |= UCR2_STPB;
--
2.10.0.1.g57b01a3
^ permalink raw reply related
* [PATCH RFC 2/7] serial: imx: set_termios(): factor-out 'ucr2' initial value
From: Sergey Organov @ 2019-06-14 12:11 UTC (permalink / raw)
To: Sascha Hauer
Cc: Pengutronix Kernel Team, NXP Linux Team, linux-arm-kernel,
linux-serial, Uwe Kleine-König
In-Reply-To: <1560514294-29111-1-git-send-email-sorganov@gmail.com>
Set common bits in a separate statement to make initialization
explicit and not repeat the common part.
Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
drivers/tty/serial/imx.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 1055124..87802fd 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -1563,10 +1563,9 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
spin_lock_irqsave(&sport->port.lock, flags);
+ ucr2 = UCR2_SRST | UCR2_IRTS;
if ((termios->c_cflag & CSIZE) == CS8)
- ucr2 = UCR2_WS | UCR2_SRST | UCR2_IRTS;
- else
- ucr2 = UCR2_SRST | UCR2_IRTS;
+ ucr2 |= UCR2_WS;
if (termios->c_cflag & CRTSCTS) {
if (sport->have_rtscts) {
--
2.10.0.1.g57b01a3
^ permalink raw reply related
* [PATCH RFC 1/7] serial: imx: fix locking in set_termios()
From: Sergey Organov @ 2019-06-14 12:11 UTC (permalink / raw)
To: Sascha Hauer
Cc: Pengutronix Kernel Team, NXP Linux Team, linux-arm-kernel,
linux-serial, Uwe Kleine-König
In-Reply-To: <1560514294-29111-1-git-send-email-sorganov@gmail.com>
imx_uart_set_termios() called imx_uart_rts_active(), or
imx_uart_rts_inactive() before taking port->port.lock.
As a consequence, sport->port.mctrl that these functions modify
could have been changed without holding port->port.lock.
Moved locking of port->port.lock above the calls to fix the issue.
Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
drivers/tty/serial/imx.c | 23 +++++++++++++----------
1 file changed, 13 insertions(+), 10 deletions(-)
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index dff75dc..1055124 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -383,6 +383,7 @@ static void imx_uart_ucrs_restore(struct imx_port *sport,
}
#endif
+/* called with port.lock taken and irqs caller dependent */
static void imx_uart_rts_active(struct imx_port *sport, u32 *ucr2)
{
*ucr2 &= ~(UCR2_CTSC | UCR2_CTS);
@@ -391,6 +392,7 @@ static void imx_uart_rts_active(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_inactive(struct imx_port *sport, u32 *ucr2)
{
*ucr2 &= ~UCR2_CTSC;
@@ -400,6 +402,7 @@ 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)
{
*ucr2 |= UCR2_CTSC;
@@ -1550,6 +1553,16 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
old_csize = CS8;
}
+ del_timer_sync(&sport->timer);
+
+ /*
+ * Ask the core to calculate the divisor for us.
+ */
+ baud = uart_get_baud_rate(port, termios, old, 50, port->uartclk / 16);
+ quot = uart_get_divisor(port, baud);
+
+ spin_lock_irqsave(&sport->port.lock, flags);
+
if ((termios->c_cflag & CSIZE) == CS8)
ucr2 = UCR2_WS | UCR2_SRST | UCR2_IRTS;
else
@@ -1593,16 +1606,6 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
ucr2 |= UCR2_PROE;
}
- del_timer_sync(&sport->timer);
-
- /*
- * Ask the core to calculate the divisor for us.
- */
- baud = uart_get_baud_rate(port, termios, old, 50, port->uartclk / 16);
- quot = uart_get_divisor(port, baud);
-
- spin_lock_irqsave(&sport->port.lock, flags);
-
sport->port.read_status_mask = 0;
if (termios->c_iflag & INPCK)
sport->port.read_status_mask |= (URXD_FRMERR | URXD_PRERR);
--
2.10.0.1.g57b01a3
^ permalink raw reply related
* [PATCH RFC 0/7] serial: imx: fix RTS and RTS/CTS handling
From: Sergey Organov @ 2019-06-14 12:11 UTC (permalink / raw)
To: Sascha Hauer
Cc: Pengutronix Kernel Team, NXP Linux Team, linux-arm-kernel,
linux-serial, Uwe Kleine-König
In-Reply-To: <20190614072801.3187-1-s.hauer@pengutronix.de>
Dear Sascha,
I think these patches deliver simpler end result and are more complete
than what you just posted. In particular, the
serial: imx: set_mctrl(): correctly restore autoRTS state
addresses exactly the issue your patch is about, but in a slightly
simpler manner.
The patches are not tested yet, so I've put RFC in the header. Just my
2 cents. I can obviously re-roll them on top of your work later, if
required.
Sergey Organov (7):
serial: imx: fix locking in set_termios()
serial: imx: set_termios(): factor-out 'ucr2' initial value
serial: imx: set_termios(): clarify RTS/CTS bits calculation
serial: imx: set_termios(): do not enable autoRTS if RTS is unset
serial: imx: set_termios(): preserve RTS state
serial: imx: set_mctrl(): correctly restore autoRTS state
serial: imx: get rid of imx_uart_rts_auto()
drivers/tty/serial/imx.c | 93 ++++++++++++++++++++++++------------------------
1 file changed, 47 insertions(+), 46 deletions(-)
--
2.10.0.1.g57b01a3
^ permalink raw reply
* Re: [PATCH 1/3 v6] serial: mctrl_gpio: Check if GPIO property exisits before requesting it
From: Yegor Yefremov @ 2019-06-14 9:38 UTC (permalink / raw)
To: Stefan Roese
Cc: Andy Shevchenko, linux-serial, kernel list, Mika Westerberg,
Greg Kroah-Hartman, Giulio Benetti
In-Reply-To: <0bf6629c-cedf-e1dd-b42b-989c6711d390@denx.de>
On Fri, Jun 14, 2019 at 11:29 AM Stefan Roese <sr@denx.de> wrote:
>
> On 14.06.19 11:04, Yegor Yefremov wrote:
> > On Thu, Jun 13, 2019 at 7:08 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> >>
> >> On Thu, Jun 13, 2019 at 05:45:40PM +0200, Stefan Roese wrote:
> >>> This patch adds a check for the GPIOs property existence, before the
> >>> GPIO is requested. This fixes an issue seen when the 8250 mctrl_gpio
> >>> support is added (2nd patch in this patch series) on x86 platforms using
> >>> ACPI.
> >>>
> >>> Here Mika's comments from 2016-08-09:
> >>>
> >>> "
> >>> I noticed that with v4.8-rc1 serial console of some of our Broxton
> >>> systems does not work properly anymore. I'm able to see output but input
> >>> does not work.
> >>>
> >>> I bisected it down to commit 4ef03d328769eddbfeca1f1c958fdb181a69c341
> >>> ("tty/serial/8250: use mctrl_gpio helpers").
> >>>
> >>> The reason why it fails is that in ACPI we do not have names for GPIOs
> >>> (except when _DSD is used) so we use the "idx" to index into _CRS GPIO
> >>> resources. Now mctrl_gpio_init_noauto() goes through a list of GPIOs
> >>> calling devm_gpiod_get_index_optional() passing "idx" of 0 for each. The
> >>> UART device in Broxton has following (simplified) ACPI description:
> >>>
> >>> Device (URT4)
> >>> {
> >>> ...
> >>> Name (_CRS, ResourceTemplate () {
> >>> GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, IoRestrictionOutputOnly,
> >>> "\\_SB.GPO0", 0x00, ResourceConsumer)
> >>> {
> >>> 0x003A
> >>> }
> >>> GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, IoRestrictionOutputOnly,
> >>> "\\_SB.GPO0", 0x00, ResourceConsumer)
> >>> {
> >>> 0x003D
> >>> }
> >>> })
> >>>
> >>> In this case it finds the first GPIO (0x003A which happens to be RX pin
> >>> for that UART), turns it into GPIO which then breaks input for the UART
> >>> device. This also breaks systems with bluetooth connected to UART (those
> >>> typically have some GPIOs in their _CRS).
> >>>
> >>> Any ideas how to fix this?
> >>>
> >>> We cannot just drop the _CRS index lookup fallback because that would
> >>> break many existing machines out there so maybe we can limit this to
> >>> only DT enabled machines. Or alternatively probe if the property first
> >>> exists before trying to acquire the GPIOs (using
> >>> device_property_present()).
> >>> "
> >>>
> >>> This patch implements the fix suggested by Mika in his statement above.
> >>>
> >>
> >> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> >
> > I cannot compile the driver without adding #include <linux/property.h>
> > to drivers/tty/serial/serial_mctrl_gpio.c.
> >
> > My platform is AM335X (OMAP3). I've tried the patches both against the
> > main repo and also tty-next.
> >
> > Other than that everything is working.
>
> Thanks for reporting. I'll wait a bit for other review comments and
> tests (thanks Andy) and will then send v7 with this header included
> (and compile tested on OMAP3) later next week.
>
> BTW: Could you please add a Tested-by-tag with the next version?
Will do.
^ permalink raw reply
* Re: [PATCH 1/3 v6] serial: mctrl_gpio: Check if GPIO property exisits before requesting it
From: Stefan Roese @ 2019-06-14 9:29 UTC (permalink / raw)
To: Yegor Yefremov, Andy Shevchenko
Cc: linux-serial, kernel list, Mika Westerberg, Greg Kroah-Hartman,
Giulio Benetti
In-Reply-To: <CAGm1_ksNqh0ZVO+aHsdcS6XQHt3hfqWE3a=3waKWEp8Oc8HWcQ@mail.gmail.com>
On 14.06.19 11:04, Yegor Yefremov wrote:
> On Thu, Jun 13, 2019 at 7:08 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
>>
>> On Thu, Jun 13, 2019 at 05:45:40PM +0200, Stefan Roese wrote:
>>> This patch adds a check for the GPIOs property existence, before the
>>> GPIO is requested. This fixes an issue seen when the 8250 mctrl_gpio
>>> support is added (2nd patch in this patch series) on x86 platforms using
>>> ACPI.
>>>
>>> Here Mika's comments from 2016-08-09:
>>>
>>> "
>>> I noticed that with v4.8-rc1 serial console of some of our Broxton
>>> systems does not work properly anymore. I'm able to see output but input
>>> does not work.
>>>
>>> I bisected it down to commit 4ef03d328769eddbfeca1f1c958fdb181a69c341
>>> ("tty/serial/8250: use mctrl_gpio helpers").
>>>
>>> The reason why it fails is that in ACPI we do not have names for GPIOs
>>> (except when _DSD is used) so we use the "idx" to index into _CRS GPIO
>>> resources. Now mctrl_gpio_init_noauto() goes through a list of GPIOs
>>> calling devm_gpiod_get_index_optional() passing "idx" of 0 for each. The
>>> UART device in Broxton has following (simplified) ACPI description:
>>>
>>> Device (URT4)
>>> {
>>> ...
>>> Name (_CRS, ResourceTemplate () {
>>> GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, IoRestrictionOutputOnly,
>>> "\\_SB.GPO0", 0x00, ResourceConsumer)
>>> {
>>> 0x003A
>>> }
>>> GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, IoRestrictionOutputOnly,
>>> "\\_SB.GPO0", 0x00, ResourceConsumer)
>>> {
>>> 0x003D
>>> }
>>> })
>>>
>>> In this case it finds the first GPIO (0x003A which happens to be RX pin
>>> for that UART), turns it into GPIO which then breaks input for the UART
>>> device. This also breaks systems with bluetooth connected to UART (those
>>> typically have some GPIOs in their _CRS).
>>>
>>> Any ideas how to fix this?
>>>
>>> We cannot just drop the _CRS index lookup fallback because that would
>>> break many existing machines out there so maybe we can limit this to
>>> only DT enabled machines. Or alternatively probe if the property first
>>> exists before trying to acquire the GPIOs (using
>>> device_property_present()).
>>> "
>>>
>>> This patch implements the fix suggested by Mika in his statement above.
>>>
>>
>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
> I cannot compile the driver without adding #include <linux/property.h>
> to drivers/tty/serial/serial_mctrl_gpio.c.
>
> My platform is AM335X (OMAP3). I've tried the patches both against the
> main repo and also tty-next.
>
> Other than that everything is working.
Thanks for reporting. I'll wait a bit for other review comments and
tests (thanks Andy) and will then send v7 with this header included
(and compile tested on OMAP3) later next week.
BTW: Could you please add a Tested-by-tag with the next version?
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCH 1/3 v6] serial: mctrl_gpio: Check if GPIO property exisits before requesting it
From: Yegor Yefremov @ 2019-06-14 9:04 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Stefan Roese, linux-serial, kernel list, Mika Westerberg,
Greg Kroah-Hartman, Giulio Benetti
In-Reply-To: <20190613170802.GE9224@smile.fi.intel.com>
On Thu, Jun 13, 2019 at 7:08 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Jun 13, 2019 at 05:45:40PM +0200, Stefan Roese wrote:
> > This patch adds a check for the GPIOs property existence, before the
> > GPIO is requested. This fixes an issue seen when the 8250 mctrl_gpio
> > support is added (2nd patch in this patch series) on x86 platforms using
> > ACPI.
> >
> > Here Mika's comments from 2016-08-09:
> >
> > "
> > I noticed that with v4.8-rc1 serial console of some of our Broxton
> > systems does not work properly anymore. I'm able to see output but input
> > does not work.
> >
> > I bisected it down to commit 4ef03d328769eddbfeca1f1c958fdb181a69c341
> > ("tty/serial/8250: use mctrl_gpio helpers").
> >
> > The reason why it fails is that in ACPI we do not have names for GPIOs
> > (except when _DSD is used) so we use the "idx" to index into _CRS GPIO
> > resources. Now mctrl_gpio_init_noauto() goes through a list of GPIOs
> > calling devm_gpiod_get_index_optional() passing "idx" of 0 for each. The
> > UART device in Broxton has following (simplified) ACPI description:
> >
> > Device (URT4)
> > {
> > ...
> > Name (_CRS, ResourceTemplate () {
> > GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, IoRestrictionOutputOnly,
> > "\\_SB.GPO0", 0x00, ResourceConsumer)
> > {
> > 0x003A
> > }
> > GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, IoRestrictionOutputOnly,
> > "\\_SB.GPO0", 0x00, ResourceConsumer)
> > {
> > 0x003D
> > }
> > })
> >
> > In this case it finds the first GPIO (0x003A which happens to be RX pin
> > for that UART), turns it into GPIO which then breaks input for the UART
> > device. This also breaks systems with bluetooth connected to UART (those
> > typically have some GPIOs in their _CRS).
> >
> > Any ideas how to fix this?
> >
> > We cannot just drop the _CRS index lookup fallback because that would
> > break many existing machines out there so maybe we can limit this to
> > only DT enabled machines. Or alternatively probe if the property first
> > exists before trying to acquire the GPIOs (using
> > device_property_present()).
> > "
> >
> > This patch implements the fix suggested by Mika in his statement above.
> >
>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
I cannot compile the driver without adding #include <linux/property.h>
to drivers/tty/serial/serial_mctrl_gpio.c.
My platform is AM335X (OMAP3). I've tried the patches both against the
main repo and also tty-next.
Other than that everything is working.
Yegor
> > Signed-off-by: Stefan Roese <sr@denx.de>
> > Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Cc: Yegor Yefremov <yegorslists@googlemail.com>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Giulio Benetti <giulio.benetti@micronovasrl.com>
> > ---
> > v6:
> > - No change
> >
> > v5:
> > - Simplified the code a bit (Andy)
> > - Added gpio_str == NULL handling (Andy)
> >
> > v4:
> > - Add missing free() calls (Johan)
> > - Added Mika's reviewed by tag
> > - Added Johan to Cc
> >
> > v3:
> > - No change
> >
> > v2:
> > - Include the problem description and analysis from Mika into the commit
> > text, as suggested by Greg.
> >
> > drivers/tty/serial/serial_mctrl_gpio.c | 13 +++++++++++++
> > 1 file changed, 13 insertions(+)
> >
> > diff --git a/drivers/tty/serial/serial_mctrl_gpio.c b/drivers/tty/serial/serial_mctrl_gpio.c
> > index 39ed56214cd3..65348887a749 100644
> > --- a/drivers/tty/serial/serial_mctrl_gpio.c
> > +++ b/drivers/tty/serial/serial_mctrl_gpio.c
> > @@ -116,6 +116,19 @@ struct mctrl_gpios *mctrl_gpio_init_noauto(struct device *dev, unsigned int idx)
> >
> > for (i = 0; i < UART_GPIO_MAX; i++) {
> > enum gpiod_flags flags;
> > + char *gpio_str;
> > + bool present;
> > +
> > + /* Check if GPIO property exists and continue if not */
> > + gpio_str = kasprintf(GFP_KERNEL, "%s-gpios",
> > + mctrl_gpios_desc[i].name);
> > + if (!gpio_str)
> > + continue;
> > +
> > + present = device_property_present(dev, gpio_str);
> > + kfree(gpio_str);
> > + if (!present)
> > + continue;
> >
> > if (mctrl_gpios_desc[i].dir_out)
> > flags = GPIOD_OUT_LOW;
> > --
> > 2.22.0
> >
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
^ permalink raw reply
* Re: [PATCH] serial: imx: fix RTS/CTS setting
From: Uwe Kleine-König @ 2019-06-14 7:48 UTC (permalink / raw)
To: Sascha Hauer
Cc: Russell King, linux-serial, Sergey Organov, NXP Linux Team,
kernel, linux-arm-kernel
In-Reply-To: <20190614072801.3187-1-s.hauer@pengutronix.de>
[expanded Cc: a bit]
Hello Sascha,
On Fri, Jun 14, 2019 at 09:28:01AM +0200, Sascha Hauer wrote:
> The correct setting of the RTS pin depends on the CRTSCTS termios setting:
>
> - When CRTSCTS is disabled then RTS shall be controlled by the TIOCM_RTS
> flag.
> - When CRTSCTS is enabled the expected behaviour of the RTS pin is:
> - When TIOCM_RTS is set then let the receiver control RTS.
> - When the TIOCM_RTS flag is cleared then RTS shall be deasserted (to let
> the upper layers throttle the transfer even when the FIFO in the UART has
> enough space).
>
> This patch fixes this behaviour. Previously the RTS pin has always been
> controlled by the receiver once the TIOCM_RTS flag was set and the CRTSCTS
> setting hasn't been taken into account.
>
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
> drivers/tty/serial/imx.c | 19 +++++++++++++++++--
> 1 file changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 8b752e895053..0eddca6455ad 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -216,6 +216,7 @@ struct imx_port {
> unsigned int dma_is_enabled:1;
> unsigned int dma_is_rxing:1;
> unsigned int dma_is_txing:1;
> + unsigned int crtscts:1;
> struct dma_chan *dma_chan_rx, *dma_chan_tx;
> struct scatterlist rx_sgl, tx_sgl[2];
> void *rx_buf;
> @@ -967,9 +968,18 @@ static void imx_uart_set_mctrl(struct uart_port *port, unsigned int mctrl)
> u32 ucr2;
>
> ucr2 = imx_uart_readl(sport, UCR2);
> +
> ucr2 &= ~(UCR2_CTS | UCR2_CTSC);
> - if (mctrl & TIOCM_RTS)
> - ucr2 |= UCR2_CTS | UCR2_CTSC;
> +
> + if (mctrl & TIOCM_RTS) {
> + if (sport->crtscts)
> + /* let the receiver control RTS */
> + ucr2 |= UCR2_CTSC;
> + else
> + /* Force RTS active */
> + ucr2 |= UCR2_CTS;
> + }
> +
Other drivers check for
port->status & UPSTAT_AUTORTS
instead of CRTSCTS. I didn't manage to grasp all the details from the
quick look I took, but maybe you should better do the same?
> imx_uart_writel(sport, ucr2, UCR2);
> }
>
> @@ -1554,6 +1564,11 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
> else
> ucr2 = UCR2_SRST | UCR2_IRTS;
>
> + if (termios->c_cflag & CRTSCTS)
> + sport->crtscts = true;
> + else
> + sport->crtscts = false;
> +
> if (termios->c_cflag & CRTSCTS) {
I'd put setting sport->crtscts in the following if block, maybe even in
the if (sport->have_rtscts) part that starts below here?
> if (sport->have_rtscts) {
> ucr2 &= ~UCR2_IRTS;
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* [PATCH] serial: imx: fix RTS/CTS setting
From: Sascha Hauer @ 2019-06-14 7:28 UTC (permalink / raw)
To: linux-serial; +Cc: kernel, Sascha Hauer, NXP Linux Team, linux-arm-kernel
The correct setting of the RTS pin depends on the CRTSCTS termios setting:
- When CRTSCTS is disabled then RTS shall be controlled by the TIOCM_RTS
flag.
- When CRTSCTS is enabled the expected behaviour of the RTS pin is:
- When TIOCM_RTS is set then let the receiver control RTS.
- When the TIOCM_RTS flag is cleared then RTS shall be deasserted (to let
the upper layers throttle the transfer even when the FIFO in the UART has
enough space).
This patch fixes this behaviour. Previously the RTS pin has always been
controlled by the receiver once the TIOCM_RTS flag was set and the CRTSCTS
setting hasn't been taken into account.
Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
drivers/tty/serial/imx.c | 19 +++++++++++++++++--
1 file changed, 17 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 8b752e895053..0eddca6455ad 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -216,6 +216,7 @@ struct imx_port {
unsigned int dma_is_enabled:1;
unsigned int dma_is_rxing:1;
unsigned int dma_is_txing:1;
+ unsigned int crtscts:1;
struct dma_chan *dma_chan_rx, *dma_chan_tx;
struct scatterlist rx_sgl, tx_sgl[2];
void *rx_buf;
@@ -967,9 +968,18 @@ static void imx_uart_set_mctrl(struct uart_port *port, unsigned int mctrl)
u32 ucr2;
ucr2 = imx_uart_readl(sport, UCR2);
+
ucr2 &= ~(UCR2_CTS | UCR2_CTSC);
- if (mctrl & TIOCM_RTS)
- ucr2 |= UCR2_CTS | UCR2_CTSC;
+
+ if (mctrl & TIOCM_RTS) {
+ if (sport->crtscts)
+ /* let the receiver control RTS */
+ ucr2 |= UCR2_CTSC;
+ else
+ /* Force RTS active */
+ ucr2 |= UCR2_CTS;
+ }
+
imx_uart_writel(sport, ucr2, UCR2);
}
@@ -1554,6 +1564,11 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
else
ucr2 = UCR2_SRST | UCR2_IRTS;
+ if (termios->c_cflag & CRTSCTS)
+ sport->crtscts = true;
+ else
+ sport->crtscts = false;
+
if (termios->c_cflag & CRTSCTS) {
if (sport->have_rtscts) {
ucr2 &= ~UCR2_IRTS;
--
2.20.1
^ permalink raw reply related
* Re: [RFC v2 02/11] OPP: Make dev_pm_opp_set_rate() with freq=0 as valid
From: Viresh Kumar @ 2019-06-14 6:32 UTC (permalink / raw)
To: Rajendra Nayak
Cc: linux-kernel, linux-arm-msm, linux-pm, linux-serial, linux-spi,
dri-devel, linux-scsi, swboyd, ulf.hansson, dianders, rafael
In-Reply-To: <20190320094918.20234-3-rnayak@codeaurora.org>
On 20-03-19, 15:19, Rajendra Nayak wrote:
> For devices with performance state, we use dev_pm_opp_set_rate()
> to set the appropriate clk rate and the performance state.
> We do need a way to *remove* the performance state vote when
> we idle the device and turn the clocks off. Use dev_pm_opp_set_rate()
> with freq=0 to achieve this.
>
> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
> drivers/opp/core.c | 18 ++++++++++++------
> 1 file changed, 12 insertions(+), 6 deletions(-)
What about this instead ?
diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 2fe96c2363a3..9accf8bb6afc 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -711,7 +711,7 @@ static int _set_required_opps(struct device *dev,
/* Single genpd case */
if (!genpd_virt_devs) {
- pstate = opp->required_opps[0]->pstate;
+ pstate = likely(opp) ? opp->required_opps[0]->pstate : 0;
ret = dev_pm_genpd_set_performance_state(dev, pstate);
if (ret) {
dev_err(dev, "Failed to set performance state of %s: %d (%d)\n",
@@ -729,7 +729,7 @@ static int _set_required_opps(struct device *dev,
mutex_lock(&opp_table->genpd_virt_dev_lock);
for (i = 0; i < opp_table->required_opp_count; i++) {
- pstate = opp->required_opps[i]->pstate;
+ pstate = likely(opp) ? opp->required_opps[i]->pstate : 0;
if (!genpd_virt_devs[i])
continue;
@@ -770,14 +770,13 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
if (unlikely(!target_freq)) {
if (opp_table->required_opp_tables) {
- /* drop the performance state vote */
- dev_pm_genpd_set_performance_state(dev, 0);
- return 0;
+ ret = _set_required_opps(dev, opp_table, NULL);
} else {
- dev_err(dev, "%s: Invalid target frequency %lu\n", __func__,
- target_freq);
- return -EINVAL;
+ dev_err(dev, "target frequency can't be 0\n");
+ ret = -EINVAL;
}
+
+ goto put_opp_table;
}
clk = opp_table->clk;
^ permalink raw reply related
* Re: [RFC v2 01/11] OPP: Don't overwrite rounded clk rate
From: Rajendra Nayak @ 2019-06-14 5:54 UTC (permalink / raw)
To: Viresh Kumar, swboyd, vincent.guittot, mturquette
Cc: ulf.hansson, dianders, linux-scsi, linux-pm, linux-arm-msm,
rafael, linux-kernel, dri-devel, linux-spi, linux-serial
In-Reply-To: <20190613095419.lfjeko7nmxtix2n4@vireshk-i7>
> Now, the request to change the frequency starts from cpufreq
> governors, like schedutil when they calls:
>
> __cpufreq_driver_target(policy, 599 MHz, CPUFREQ_RELATION_L);
>
> CPUFREQ_RELATION_L means: lowest frequency at or above target. And so
> I would expect the frequency to get set to 600MHz (if we look at clock
> driver) or 700MHz (if we look at OPP table). I think we should decide
> this thing from the OPP table only as that's what the platform guys
> want us to use. So, we should end up with 700 MHz.
>
> Then we land into dev_pm_opp_set_rate(), which does this (which is
> code copied from earlier version of cpufreq-dt driver):
so before we land into dev_pm_opp_set_rate() from a __cpufreq_driver_target()
I guess we do have a cpufreq driver callback that gets called in between?
which is either .target_index or .target
In case of .target_index, the cpufreq core looks for a OPP index
and we would land up with 700Mhz i guess, so we are good.
In case of .target though the 'relation' CPUFREQ_RELATION_L does get passed over
to the cpufreq driver which I am guessing is expected to handle it in some way to
make sure the target frequency set is not less than whats requested? instead of
simply passing the requested frequency over to dev_pm_opp_set_rate()?
Looking at all the existing cpufreq drivers upstream, while most support .target_index
the 3 which do support .target seem to completely ignore this 'relation' input that's
passed to them.
drivers/cpufreq/cppc_cpufreq.c: .target = cppc_cpufreq_set_target,
drivers/cpufreq/cpufreq-nforce2.c: .target = nforce2_target,
drivers/cpufreq/pcc-cpufreq.c: .target = pcc_cpufreq_target,
> This kind of behavior (introduced by this patch) is important for
> other devices which want to run at the nearest frequency to target
> one, but not for CPUs/GPUs. So, we need to tag these IO devices
> separately, maybe from DT ? So we select the closest match instead of
> most optimal one.
yes we do need some way to distinguish between CPU/GPU devices and other
IO devices. CPU/GPU can always run at fmax for a given voltage, that's not true
for IO devices and I don't see how we can satisfy both cases without
clearly knowing if we are serving a processor or an IO device, unless the
higher layers (cpufreq/devfreq) are able to handle this somehow without
expecting the OPP layer to handle the differences.
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [RFC v2 01/11] OPP: Don't overwrite rounded clk rate
From: Viresh Kumar @ 2019-06-14 5:27 UTC (permalink / raw)
To: swboyd, Rajendra Nayak, vincent.guittot, mturquette
Cc: linux-kernel, linux-arm-msm, linux-pm, linux-serial, linux-spi,
dri-devel, linux-scsi, ulf.hansson, dianders, rafael
In-Reply-To: <20190613095419.lfjeko7nmxtix2n4@vireshk-i7>
On 13-06-19, 15:24, Viresh Kumar wrote:
> I am confused as hell on what we should be doing and what we are doing
> right now. And if we should do better.
>
> Let me explain with an example.
>
> - The clock provider supports following frequencies: 500, 600, 700,
> 800, 900, 1000 MHz.
>
> - The OPP table contains/supports only a subset: 500, 700, 1000 MHz.
>
> Now, the request to change the frequency starts from cpufreq
> governors, like schedutil when they calls:
>
> __cpufreq_driver_target(policy, 599 MHz, CPUFREQ_RELATION_L);
>
> CPUFREQ_RELATION_L means: lowest frequency at or above target. And so
> I would expect the frequency to get set to 600MHz (if we look at clock
> driver) or 700MHz (if we look at OPP table). I think we should decide
> this thing from the OPP table only as that's what the platform guys
> want us to use. So, we should end up with 700 MHz.
>
> Then we land into dev_pm_opp_set_rate(), which does this (which is
> code copied from earlier version of cpufreq-dt driver):
>
> - clk_round_rate(clk, 599 MHz).
>
> clk_round_rate() returns the highest frequency lower than target. So
> it must return 500 MHz (I haven't tested this yet, all theoretical).
>
> - _find_freq_ceil(opp_table, 500 MHz).
>
> This works like CPUFREQ_RELATION_L, so we find lowest frequency >=
> target freq. And so we should get: 500 MHz itself as OPP table has
> it.
>
> - clk_set_rate(clk, 500 MHz).
>
> This must be doing round-rate again, but I think we will settle with
> 500 MHz eventually.
>
>
> Now the questionnaire:
>
> - Is this whole exercise correct ?
No, I missed the call to cpufreq_frequency_table_target() in
__cpufreq_driver_target() which finds the exact frequency from cpufreq table
(which was created using opp table) and so we never screw up here. Sorry for
confusing everyone on this :(
> Now lets move to this patch, which makes it more confusing.
>
> The OPP tables for CPUs and GPUs should already be somewhat like fmax
> tables for particular voltage values and that's why both cpufreq and
> OPP core try to find a frequency higher than target so we choose the
> most optimum one power-efficiency wise.
>
> For cases where the OPP table is only a subset of the clk-providers
> table (almost always), if we let the clock provider to find the
> nearest frequency (which is lower) we will run the CPU/GPU at a
> not-so-optimal frequency. i.e. if 500, 600, 700 MHz all need voltage
> to be 1.2 V, we should be running at 700 always, while we may end up
> running at 500 MHz.
This won't happen for CPUs because of the reason I explained earlier. cpufreq
core does the rounding before calling dev_pm_opp_set_rate(). And no other
devices use dev_pm_opp_set_rate() right now apart from CPUs, so we are not going
to break anything.
> This kind of behavior (introduced by this patch) is important for
> other devices which want to run at the nearest frequency to target
> one, but not for CPUs/GPUs. So, we need to tag these IO devices
> separately, maybe from DT ? So we select the closest match instead of
> most optimal one.
Hmm, so this patch won't break anything and I am inclined to apply it again :)
Does anyone see any other issues with it, which I might be missing ?
--
viresh
^ permalink raw reply
* Re: [PATCH] serial: sh-sci: fix uninitialized variable warning
From: Geert Uytterhoeven @ 2019-06-13 20:13 UTC (permalink / raw)
To: Charles
Cc: Greg Kroah-Hartman, Jiri Slaby, open list:SERIAL DRIVERS,
Linux Kernel Mailing List, rodrigosiqueiramelo
In-Reply-To: <20190613180824.6ajwjelzr5fmjnie@debie>
Hi Charles,
On Thu, Jun 13, 2019 at 8:09 PM Charles <18oliveira.charles@gmail.com> wrote:
> Avoid following compiler warning on uninitialized variable
>
> In file included from ./include/linux/rwsem.h:16:0,
> from ./include/linux/notifier.h:15,
> from ./include/linux/clk.h:17,
> from drivers/tty/serial/sh-sci.c:24:
> drivers/tty/serial/sh-sci.c: In function ‘sci_dma_rx_submit’:
> ./include/linux/spinlock.h:288:3: warning: ‘flags’ may be used
> uninitialized in this function [-Wmaybe-uninitialized]
> _raw_spin_unlock_irqrestore(lock, flags); \
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/tty/serial/sh-sci.c:1353:16: note: ‘flags’ was declared here
> unsigned long flags;
> ^~~~~
>
> Signed-off-by: Charles Oliveira <18oliveira.charles@gmail.com>
Thanks for your patch, but this is a false positive: the compiler is not
smart enough to realize that both initialization and use depend on
the same condition.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ 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