Linux Serial subsystem development
 help / color / mirror / Atom feed
* [PATCH 2/8] dt-bindings: serial: Add bindings for nvidia,tegra194-tcu
From: Mikko Perttunen @ 2018-05-08 11:43 UTC (permalink / raw)
  To: robh+dt, mark.rutland, jassisinghbrar, gregkh, thierry.reding,
	jonathanh
  Cc: araza, devicetree, linux-serial, linux-tegra, linux-arm-kernel,
	linux-kernel, Mikko Perttunen
In-Reply-To: <20180508114403.14499-1-mperttunen@nvidia.com>

Add bindings for the Tegra Combined UART device used to talk to the
UART console on Tegra194 systems.

Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
---
 .../bindings/serial/nvidia,tegra194-tcu.txt        | 35 ++++++++++++++++++++++
 1 file changed, 35 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/serial/nvidia,tegra194-tcu.txt

diff --git a/Documentation/devicetree/bindings/serial/nvidia,tegra194-tcu.txt b/Documentation/devicetree/bindings/serial/nvidia,tegra194-tcu.txt
new file mode 100644
index 000000000000..86763bc5d74f
--- /dev/null
+++ b/Documentation/devicetree/bindings/serial/nvidia,tegra194-tcu.txt
@@ -0,0 +1,35 @@
+NVIDIA Tegra Combined UART (TCU)
+
+The TCU is a system for sharing a hardware UART instance among multiple
+systems withing the Tegra SoC. It is implemented through a mailbox-
+based protocol where each "virtual UART" has a pair of mailboxes, one
+for transmitting and one for receiving, that is used to communicate
+with the hardware implementing the TCU.
+
+Required properties:
+- name : Should be tcu
+- compatible
+    Array of strings
+    One of:
+    - "nvidia,tegra194-tcu"
+- mbox-names:
+    "rx" - Mailbox for receiving data from hardware UART
+    "tx" - Mailbox for transmitting data to hardware UART
+- mboxes: Mailboxes corresponding to the mbox-names. 
+
+This node is a mailbox consumer. See the following files for details of
+the mailbox subsystem, and the specifiers implemented by the relevant
+provider(s):
+
+- .../mailbox/mailbox.txt
+- .../mailbox/nvidia,tegra186-hsp.txt
+
+Example bindings:
+-----------------
+
+tcu: tcu {
+	compatible = "nvidia,tegra194-tcu";
+	mboxes = <&hsp_top0 TEGRA_HSP_MBOX_TYPE_SM 0>,
+	         <&hsp_aon TEGRA_HSP_MBOX_TYPE_SM 1>;
+	mbox-names = "rx", "tx";
+};
-- 
2.16.1

^ permalink raw reply related

* [PATCH 1/8] dt-bindings: tegra186-hsp: Add shared interrupts
From: Mikko Perttunen @ 2018-05-08 11:43 UTC (permalink / raw)
  To: robh+dt, mark.rutland, jassisinghbrar, gregkh, thierry.reding,
	jonathanh
  Cc: araza, devicetree, linux-serial, linux-tegra, linux-arm-kernel,
	linux-kernel, Mikko Perttunen
In-Reply-To: <20180508114403.14499-1-mperttunen@nvidia.com>

Non-doorbell interrupts are routed through "shared interrupts". These
interrupts can be mapped to various internal interrupt lines. Add
interrupt properties for shared interrupts to the tegra186-hsp device
tree bindings.

Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
---
 Documentation/devicetree/bindings/mailbox/nvidia,tegra186-hsp.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/mailbox/nvidia,tegra186-hsp.txt b/Documentation/devicetree/bindings/mailbox/nvidia,tegra186-hsp.txt
index b99d25fc2f26..9edcdf82d719 100644
--- a/Documentation/devicetree/bindings/mailbox/nvidia,tegra186-hsp.txt
+++ b/Documentation/devicetree/bindings/mailbox/nvidia,tegra186-hsp.txt
@@ -21,6 +21,8 @@ Required properties:
     Contains a list of names for the interrupts described by the interrupt
     property. May contain the following entries, in any order:
     - "doorbell"
+    - "sharedN", where 'N' is a number from zero up to the number of
+      external interrupts supported by the HSP instance minus one.
     Users of this binding MUST look up entries in the interrupt property
     by name, using this interrupt-names property to do so.
 - interrupts
-- 
2.16.1

^ permalink raw reply related

* [PATCH 0/8] Tegra Combined UART driver
From: Mikko Perttunen @ 2018-05-08 11:43 UTC (permalink / raw)
  To: robh+dt, mark.rutland, jassisinghbrar, gregkh, thierry.reding,
	jonathanh
  Cc: araza, devicetree, linux-serial, linux-tegra, linux-arm-kernel,
	linux-kernel, Mikko Perttunen

Hi all,

on Tegra194, the primary console UART is the "Tegra Combined UART",
or TCU. This is a "virtual UART", where each consumer communicates
with a central implementation over mailboxes. The central
implementation then multiplexes the streams and arbitrates use of
a hardware serial port. This driver implements the consumer portion
to allow using the primary console.

The series is split into the following parts:
* patches 1 and 2 add the device tree bindings for mailbox and tcu
  itself.
* patch 3 adds a blocking transmission option to the mailbox
  framework.
* patches 4 and 5 add support for the "shared mailbox" primitive
  to the Tegra HSP driver.
* patch 6 adds the TCU driver itself
* patches 7 and 8 do the necessary device tree changes.

The series has been tested on the Tegra194 P2972 board.

Thanks,
Mikko

Mikko Perttunen (8):
  dt-bindings: tegra186-hsp: Add shared interrupts
  dt-bindings: serial: Add bindings for nvidia,tegra194-tcu
  mailbox: Add transmit done by blocking option
  mailbox: tegra-hsp: Refactor in preparation of mailboxes
  mailbox: tegra-hsp: Add support for shared mailboxes
  serial: Add Tegra Combined UART driver
  arm64: tegra: Add nodes for tcu on Tegra194
  arm64: tegra: Mark tcu as primary serial port on Tegra194 P2888

 .../bindings/mailbox/nvidia,tegra186-hsp.txt       |   2 +
 .../bindings/serial/nvidia,tegra194-tcu.txt        |  35 +++
 arch/arm64/boot/dts/nvidia/tegra194-p2888.dtsi     |   2 +-
 arch/arm64/boot/dts/nvidia/tegra194.dtsi           |  34 ++-
 drivers/mailbox/mailbox.c                          |  30 +-
 drivers/mailbox/mailbox.h                          |   1 +
 drivers/mailbox/tegra-hsp.c                        | 320 +++++++++++++++++----
 drivers/tty/serial/Kconfig                         |   9 +
 drivers/tty/serial/Makefile                        |   1 +
 drivers/tty/serial/tegra-tcu.c                     | 302 +++++++++++++++++++
 include/uapi/linux/serial_core.h                   |   3 +
 11 files changed, 671 insertions(+), 68 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/serial/nvidia,tegra194-tcu.txt
 create mode 100644 drivers/tty/serial/tegra-tcu.c

-- 
2.16.1

^ permalink raw reply

* Re: [PATCH 1/2] serial: imx: cleanup imx_uart_disable_dma()
From: Sebastian Reichel @ 2018-05-08 11:27 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Greg Kroah-Hartman, Jiri Slaby, linux-serial, Fabio Estevam,
	Shawn Guo, linux-kernel
In-Reply-To: <20180508064047.v7wz7b4t5ta55hv7@pengutronix.de>

[-- Attachment #1: Type: text/plain, Size: 2385 bytes --]

Hi,

On Tue, May 08, 2018 at 08:40:47AM +0200, Uwe Kleine-König wrote:
> On Mon, May 07, 2018 at 11:36:09PM +0200, Sebastian Reichel wrote:
> > Remove unrelated CTSC/CTS disabling from imx_uart_disable_dma() and
> > move it to imx_uart_shutdown(), which is the only user of the DMA
> > disabling function. This should not change the driver's behaviour,
> > but improves readability. After this change imx_uart_disable_dma()
> > does the reverse thing of imx_uart_enable_dma().
> > 
> > Suggested-by: Nandor Han <nandor.han@ge.com>
> > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
> > ---
> >  drivers/tty/serial/imx.c | 9 ++-------
> >  1 file changed, 2 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > index c2fc6bef7a6f..3ca767b1162a 100644
> > --- a/drivers/tty/serial/imx.c
> > +++ b/drivers/tty/serial/imx.c
> > @@ -1291,18 +1291,13 @@ static void imx_uart_enable_dma(struct imx_port *sport)
> >  
> >  static void imx_uart_disable_dma(struct imx_port *sport)
> >  {
> > -	u32 ucr1, ucr2;
> > +	u32 ucr1;
> >  
> >  	/* clear UCR1 */
> >  	ucr1 = imx_uart_readl(sport, UCR1);
> >  	ucr1 &= ~(UCR1_RXDMAEN | UCR1_TXDMAEN | UCR1_ATDMAEN);
> >  	imx_uart_writel(sport, ucr1, UCR1);
> >  
> > -	/* clear UCR2 */
> > -	ucr2 = imx_uart_readl(sport, UCR2);
> > -	ucr2 &= ~(UCR2_CTSC | UCR2_CTS | UCR2_ATEN);
> > -	imx_uart_writel(sport, ucr2, UCR2);
> > -
> >  	imx_uart_setup_ufcr(sport, TXTL_DEFAULT, RXTL_DEFAULT);
> >  
> >  	sport->dma_is_enabled = 0;
> > @@ -1447,7 +1442,7 @@ static void imx_uart_shutdown(struct uart_port *port)
> >  
> >  	spin_lock_irqsave(&sport->port.lock, flags);
> >  	ucr2 = imx_uart_readl(sport, UCR2);
> > -	ucr2 &= ~(UCR2_TXEN | UCR2_ATEN);
> > +	ucr2 &= ~(UCR2_TXEN | UCR2_CTSC | UCR2_CTS | UCR2_ATEN);
> >  	imx_uart_writel(sport, ucr2, UCR2);
> >  	spin_unlock_irqrestore(&sport->port.lock, flags);
> 
> While this doesn't change behaviour (which is of course good and
> intended here) I wonder if changing RTS is right here.
> 
> According to Documentation/serial/driver it is not.

Yes, looks like it should be removed completly. I suggest to keep
this in two different patches (move UCR2 handling first, then remove
RTS handling), so that we end up with simple and comprehensible patches.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply

* Re: [PATCH v3 2/3] can: simplify LED trigger handling
From: Marc Kleine-Budde @ 2018-05-08 11:04 UTC (permalink / raw)
  To: Uwe Kleine-König, Greg Kroah-Hartman, Jiri Slaby,
	Johan Hovold, Jacek Anaszewski, Pavel Machek
  Cc: One Thousand Gnomes, Florian Fainelli, linux-serial,
	Mathieu Poirier, linux-kernel, linux-can, linux-arm-kernel,
	kernel, Robin Murphy, linux-leds
In-Reply-To: <20180508100543.12559-3-u.kleine-koenig@pengutronix.de>


[-- Attachment #1.1: Type: text/plain, Size: 569 bytes --]

On 05/08/2018 12:05 PM, Uwe Kleine-König wrote:
> The new function led_trigger_register_format allows one to simplify LED
> trigger handling considerably.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Acked-by: Marc Kleine-Budde <mkl@pengutronix.de>

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply

* Re: [PATCH] tty: pl011: Avoid spuriously stuck-off interrupts
From: Russell King - ARM Linux @ 2018-05-08 11:01 UTC (permalink / raw)
  To: Dave Martin
  Cc: Peter Maydell, Andrew Jones, Ciro Santilli, Linus Walleij, Wei Xu,
	linux-serial, linux-arm-kernel
In-Reply-To: <1524823545-11309-2-git-send-email-Dave.Martin@arm.com>

On Fri, Apr 27, 2018 at 11:05:45AM +0100, Dave Martin wrote:
> Commit 9b96fbacda34 ("serial: PL011: clear pending interrupts")
> clears the RX and receive timeout interrupts on pl011 startup, to
> avoid a screaming-interrupt scenario that can occur when the
> firmware or bootloader leaves these interrupts asserted.
> 
> This has been noted as an issue when running Linux on qemu [1].
> 
> Unfortunately, the above fix seems to lead to potential
> misbehaviour if the RX FIFO interrupt is asserted _non_ spuriously
> on driver startup, if the RX FIFO is also already full to the
> trigger level.
> 
> Clearing the RX FIFO interrupt does not change the FIFO fill level.
> In this scenario, because the interrupt is now clear and because
> the FIFO is already full to the trigger level, no new assertion of
> the RX FIFO interrupt can occur unless the FIFO is drained back
> below the trigger level.  This never occurs because the pl011
> driver is waiting for an RX FIFO interrupt to tell it that there is
> something to read, and does not read the FIFO at all until that
> interrupt occurs.
> 
> Thus, simply clearing "spurious" interrupts on startup may be
> misguided, since there is no way to be sure that the interrupts are
> truly spurious, and things can go wrong if they are not.
> 
> This patch instead clears the interrupt condition by draining the
> RX FIFO during UART startup, after clearing any potentially
> spurious interrupt.  This should ensure that an interrupt will
> definitely be asserted if the RX FIFO subsequently becomes
> sufficiently full.
> 
> The drain is done at the point of enabling interrupts only.  This
> means that it will occur any time the UART is newly opened through
> the tty layer.  It will not apply to polled-mode use of the UART by
> kgdboc: since that scenario cannot use interrupts by design, this
> should not matter.  kgdboc will interact badly with "normal" use of
> the UART in any case: this patch makes no attempt to paper over
> such issues.
> 
> This patch does not attempt to address the case where the RX FIFO
> fills faster than it can be drained: that is a pathological
> hardware design problem that is beyond the scope of the driver to
> work around.
> 
> [1] [Qemu-devel] [Qemu-arm] [PATCH] pl011: do not put into fifo
> before enabled the interruption
> https://lists.gnu.org/archive/html/qemu-devel/2018-01/msg06446.html
> 
> Reported-by: Wei Xu <xuwei5@hisilicon.com>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Fixes: 9b96fbacda34 ("serial: PL011: clear pending interrupts")
> Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> Tested-by: Andrew Jones <drjones@redhat.com>
> Reviewed-by: Andrew Jones <drjones@redhat.com>
> ---
>  drivers/tty/serial/amba-pl011.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> index 4b40a5b..a6ccb85 100644
> --- a/drivers/tty/serial/amba-pl011.c
> +++ b/drivers/tty/serial/amba-pl011.c
> @@ -1731,6 +1731,16 @@ static void pl011_enable_interrupts(struct uart_amba_port *uap)
>  
>  	/* Clear out any spuriously appearing RX interrupts */
>  	pl011_write(UART011_RTIS | UART011_RXIS, uap, REG_ICR);
> +
> +	/*
> +	 * RXIS is asserted only when the RX FIFO transitions from below
> +	 * to above the trigger threshold.  If the RX FIFO is already
> +	 * full to the threshold this can't happen and RXIS will now be
> +	 * stuck off.  Drain the RX FIFO explicitly to fix this:
> +	 */
> +	while (!(pl011_read(uap, REG_FR) & UART01x_FR_RXFE))
> +		pl011_read(uap, REG_DR);

You probably ought to limit this in case of a stuck receive not empty
case, maybe to (eg) twice the depth of the FIFO?

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

^ permalink raw reply

* Re: [PATCH v3 3/3] tty: implement led triggers
From: Uwe Kleine-König @ 2018-05-08 10:51 UTC (permalink / raw)
  To: Johan Hovold
  Cc: linux-arm-kernel, One Thousand Gnomes, Florian Fainelli,
	linux-kernel, Pavel Machek, Mathieu Poirier, Greg Kroah-Hartman,
	linux-can, Jacek Anaszewski, linux-serial, Jiri Slaby,
	Robin Murphy, kernel, linux-leds
In-Reply-To: <20180508100851.GY2285@localhost>

On Tue, May 08, 2018 at 12:08:51PM +0200, Johan Hovold wrote:
> On Tue, May 08, 2018 at 12:05:43PM +0200, Uwe Kleine-König wrote:
> > The rx trigger fires when data is pushed to the ldisc by the driver. This
> > is a bit later than the actual receiving of data but has the nice benefit
> > that it doesn't need adaption for each driver and isn't in the hot path.
> > 
> > Similarly the tx trigger fires when data was copied from userspace and is
> > given to the ldisc.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> >  arch/arm/boot/dts/imx25-logitech-baby.dts | 192 ++++++++++++++++++++++
> 
> Looks like you included more than intended in this patch.

Right, this doesn't belong here. Feel free to assume I didn't include it
in the patch and comment the rest :-)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

^ permalink raw reply

* Re: [PATCH v3 3/3] tty: implement led triggers
From: Johan Hovold @ 2018-05-08 10:08 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Greg Kroah-Hartman, Jiri Slaby, Johan Hovold, Jacek Anaszewski,
	Pavel Machek, linux-serial, linux-leds, linux-can, kernel,
	One Thousand Gnomes, Florian Fainelli, Mathieu Poirier,
	linux-kernel, Robin Murphy, linux-arm-kernel
In-Reply-To: <20180508100543.12559-4-u.kleine-koenig@pengutronix.de>

On Tue, May 08, 2018 at 12:05:43PM +0200, Uwe Kleine-König wrote:
> The rx trigger fires when data is pushed to the ldisc by the driver. This
> is a bit later than the actual receiving of data but has the nice benefit
> that it doesn't need adaption for each driver and isn't in the hot path.
> 
> Similarly the tx trigger fires when data was copied from userspace and is
> given to the ldisc.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  arch/arm/boot/dts/imx25-logitech-baby.dts | 192 ++++++++++++++++++++++

Looks like you included more than intended in this patch.

>  drivers/tty/Kconfig                       |   7 +
>  drivers/tty/tty_buffer.c                  |   2 +
>  drivers/tty/tty_io.c                      |   3 +
>  drivers/tty/tty_port.c                    |  32 +++-
>  include/linux/tty.h                       |  22 +++
>  6 files changed, 256 insertions(+), 2 deletions(-)
>  create mode 100644 arch/arm/boot/dts/imx25-logitech-baby.dts

Johan

^ permalink raw reply

* [PATCH v3 3/3] tty: implement led triggers
From: Uwe Kleine-König @ 2018-05-08 10:05 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Jiri Slaby, Johan Hovold, Jacek Anaszewski,
	Pavel Machek
  Cc: linux-serial, linux-leds, linux-can, kernel, One Thousand Gnomes,
	Florian Fainelli, Mathieu Poirier, linux-kernel, Robin Murphy,
	linux-arm-kernel
In-Reply-To: <20180508100543.12559-1-u.kleine-koenig@pengutronix.de>

The rx trigger fires when data is pushed to the ldisc by the driver. This
is a bit later than the actual receiving of data but has the nice benefit
that it doesn't need adaption for each driver and isn't in the hot path.

Similarly the tx trigger fires when data was copied from userspace and is
given to the ldisc.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 arch/arm/boot/dts/imx25-logitech-baby.dts | 192 ++++++++++++++++++++++
 drivers/tty/Kconfig                       |   7 +
 drivers/tty/tty_buffer.c                  |   2 +
 drivers/tty/tty_io.c                      |   3 +
 drivers/tty/tty_port.c                    |  32 +++-
 include/linux/tty.h                       |  22 +++
 6 files changed, 256 insertions(+), 2 deletions(-)
 create mode 100644 arch/arm/boot/dts/imx25-logitech-baby.dts

diff --git a/arch/arm/boot/dts/imx25-logitech-baby.dts b/arch/arm/boot/dts/imx25-logitech-baby.dts
new file mode 100644
index 000000000000..39cf763d228b
--- /dev/null
+++ b/arch/arm/boot/dts/imx25-logitech-baby.dts
@@ -0,0 +1,192 @@
+/dts-v1/;
+#include "imx25.dtsi"
+
+/ {
+	model = "Logitech MX25 Baby";
+	compatible = "logitech,baby", "fsl,imx25";
+
+	chosen {
+		linux,stdout-path = &uart2;
+	};
+};
+
+&i2c1 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_i2c1>;
+
+	clock-frequency = <100000>;
+
+	status = "okay";
+
+	codec: tlv320aic3104@18 {
+		compatible = "ti,tlv320aic310x";
+		reg = <0x18>;
+//		HPVDD-supply
+//		SPRVDD-supply
+//		SPLVDD-supply
+//		AVDD-supply
+//		IOVDD-supply
+//		DVDD-supply
+//		?gpio-reset
+//		?ai31xx-micbias-vg
+	};
+};
+
+&i2c2 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_i2c2>;
+
+	clock-frequency = <100000>;
+
+	status = "okay";
+
+	msp430@10 {
+		reg = <0x10>;
+	};
+};
+
+&iomuxc {
+	baby {
+		pinctrl_fec: fecgrp {
+			fsl,pins = <
+				MX25_PAD_FEC_MDC__FEC_MDC		0x040
+				MX25_PAD_FEC_MDIO__FEC_MDIO		0x1f0
+				MX25_PAD_FEC_TDATA0__FEC_TDATA0		0x040
+				MX25_PAD_FEC_TDATA1__FEC_TDATA1		0x040
+				MX25_PAD_A22__FEC_TDATA2		0x000
+				MX25_PAD_A23__FEC_TDATA3		0x000
+				MX25_PAD_FEC_TX_EN__FEC_TX_EN		0x040
+				MX25_PAD_FEC_RDATA0__FEC_RDATA0		0x0c0
+				MX25_PAD_FEC_RDATA1__FEC_RDATA1		0x0c0
+				MX25_PAD_A20__FEC_RDATA2		0x000
+				MX25_PAD_A21__FEC_RDATA3		0x000
+				MX25_PAD_FEC_RX_DV__FEC_RX_DV		0x0c0
+				MX25_PAD_FEC_TX_CLK__FEC_TX_CLK		0x1c0
+				MX25_PAD_A17__FEC_TX_ERR		0x080
+				MX25_PAD_A19__FEC_RX_ERR		0x080
+				MX25_PAD_A24__FEC_RX_CLK		0x000
+				MX25_PAD_A18__FEC_COL			0x080
+				MX25_PAD_A25__FEC_CRS			0x080
+
+				/*
+				 * PHY_RESET:
+				 * hwref 1: MX25_PIN_A10
+				 * hwref 2: MX25_PIN_CSI_D7
+				 * hwref 3+: MX25_PIN_PWM
+				 */
+				MX25_PAD_PWM__GPIO_1_26			0x0c0
+			>;
+		};
+
+		pinctrl_i2c1: i2c1grp {
+			fsl,pins = <
+				MX25_PAD_I2C1_CLK__I2C1_CLK		0x0a8
+				MX25_PAD_I2C1_DAT__I2C1_DAT		0x0a8
+			>;
+		};
+
+		pinctrl_i2c2: i2c2grp {
+			fsl,pins = <
+				MX25_PAD_GPIO_C__I2C2_SCL		0x0e8
+				MX25_PAD_GPIO_D__I2C2_SDA		0x0a8
+			>;
+		};
+
+		pinctrl_nfc: nfcgrp {
+			fsl,pins = <
+				MX25_PAD_NFRB__NFRB			0x0
+				MX25_PAD_NFWP_B__NFWP_B			0x0
+				MX25_PAD_NFRE_B__NFRE_B			0x0
+				MX25_PAD_NFWE_B__NFWE_B			0x0
+				MX25_PAD_NFALE__NFALE			0x0
+				MX25_PAD_NFCLE__NFCLE			0x0
+				MX25_PAD_NF_CE0__NF_CE0			0x0
+				MX25_PAD_D0__D0				0x0
+				MX25_PAD_D1__D1				0x0
+				MX25_PAD_D2__D2				0x0
+				MX25_PAD_D3__D3				0x0
+				MX25_PAD_D4__D4				0x0
+				MX25_PAD_D5__D5				0x0
+				MX25_PAD_D6__D6				0x0
+				MX25_PAD_D7__D7				0x0
+			>;
+		};
+
+		pinctrl_uart2: uart2grp {
+			fsl,pins = <
+				MX25_PAD_UART2_RXD__UART2_RXD	0x1e0
+				MX25_PAD_UART2_TXD__UART2_TXD	0x0e0
+				/*
+				 * These are configured in the vendor kernel,
+				 * but the corresponding lines don't seem to be
+				 * available:
+				 * MX25_PAD_UART2_RTS__UART2_RTS	0x1e0
+				 * MX25_PAD_UART2_CTS__UART2_CTS	0x0e0
+				 */
+			>;
+		};
+	};
+};
+
+&fec {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_fec>;
+
+	//phy-reset-gpios = <&gpio1 26 0>;
+	phy-handle = <&ethphy>;
+	phy-mode = "rmii";
+
+	status = "okay";
+
+	mdio {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		ethphy: ethernet-phy@1 {
+			compatible = "ethernet-phy-ieee802.3-c22";
+			reg = <1>;
+			max-speed = <100>;
+		};
+	};
+};
+
+&kpp {
+	linux,keymap = <
+		0x000000cf	/* KEY_PLAY */
+		0x0001004e	/* KEY_KPPLUS */
+		0x00020069	/* KEY_LEFT */
+		0x00030066	/* KEY_HOME */
+		0x0100003b	/* KEY_F1 */
+		0x010100a4	/* KEY_PLAYPAUSE */
+		0x010200a3	/* KEY_NEXTSONG */
+		0x010300a5	/* KEY_PREVIOUSSONG */
+		0x0200003f	/* KEY_F5 */
+		0x0201003e	/* KEY_F4 */
+		0x0202003d	/* KEY_F3 */
+		0x0203003c	/* KEY_F2 */
+		0x03000000	/* KEY_RESERVED */
+		0x03010000	/* KEY_RESERVED */
+		0x0302008e	/* KEY_SLEEP */
+		0x03030040	/* KEY_F6 */
+		>;
+
+	status = "okay";
+};
+
+&nfc {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_nfc>;
+
+	nand-on-flash-bbt;
+	nand-bus-width = <8>;
+	nand-ecc-mode = "hw";
+
+	status = "okay";
+};
+
+&uart2 {
+        pinctrl-names = "default";
+        pinctrl-0 = <&pinctrl_uart2>;
+
+        status = "okay";
+};
diff --git a/drivers/tty/Kconfig b/drivers/tty/Kconfig
index 0840d27381ea..b119c0fa1f5a 100644
--- a/drivers/tty/Kconfig
+++ b/drivers/tty/Kconfig
@@ -41,6 +41,13 @@ config VT
 	  If unsure, say Y, or else you won't be able to do much with your new
 	  shiny Linux system :-)
 
+config TTY_LEDS_TRIGGERS
+	bool "Enable support for TTY actions making LEDs blink"
+	depends on LEDS_TRIGGERS
+	---help---
+	  This enable support for tty triggers. It provides two LED triggers
+	  (rx and tx) for each TTY.
+
 config CONSOLE_TRANSLATIONS
 	depends on VT
 	default y
diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
index c996b6859c5e..364080ce8e91 100644
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -521,6 +521,8 @@ static void flush_to_ldisc(struct work_struct *work)
 			continue;
 		}
 
+		tty_led_trigger_rx(port);
+
 		count = receive_buf(port, head, count);
 		if (!count)
 			break;
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 7c838b90a31d..8ef597dc0c3d 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -955,6 +955,9 @@ static inline ssize_t do_tty_write(
 		ret = -EFAULT;
 		if (copy_from_user(tty->write_buf, buf, size))
 			break;
+
+		tty_led_trigger_tx(tty->port);
+
 		ret = write(tty, file, tty->write_buf, size);
 		if (ret <= 0)
 			break;
diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
index 25d736880013..d313edfa6315 100644
--- a/drivers/tty/tty_port.c
+++ b/drivers/tty/tty_port.c
@@ -37,6 +37,8 @@ static int tty_port_default_receive_buf(struct tty_port *port,
 
 	ret = tty_ldisc_receive_buf(disc, p, (char *)f, count);
 
+	tty_led_trigger_rx(port);
+
 	tty_ldisc_deref(disc);
 
 	return ret;
@@ -163,8 +165,31 @@ struct device *tty_port_register_device_attr_serdev(struct tty_port *port,
 		return dev;
 	}
 
-	return tty_register_device_attr(driver, index, device, drvdata,
-			attr_grp);
+	if (IS_ENABLED(CONFIG_TTY_LEDS_TRIGGERS)) {
+		int ret;
+
+		ret = led_trigger_register_format(&port->led_trigger_rx,
+						  "%s%d-rx", driver->name, index);
+		if (ret < 0)
+			pr_warn("Failed to register rx trigger for %s%d (%d)\n",
+				driver->name, index, ret);
+
+		ret = led_trigger_register_format(&port->led_trigger_tx,
+						  "%s%d-tx", driver->name, index);
+		if (ret < 0)
+			pr_warn("Failed to register tx trigger for %s%d (%d)\n",
+				driver->name, index, ret);
+	}
+
+	dev = tty_register_device_attr(driver, index,
+				       device, drvdata, attr_grp);
+
+	if (IS_ENABLED(CONFIG_TTY_LEDS_TRIGGERS) && IS_ERR(dev)) {
+		led_trigger_unregister_simple(port->led_trigger_tx);
+		led_trigger_unregister_simple(port->led_trigger_rx);
+	}
+
+	return dev;
 }
 EXPORT_SYMBOL_GPL(tty_port_register_device_attr_serdev);
 
@@ -206,6 +231,9 @@ void tty_port_unregister_device(struct tty_port *port,
 	if (ret == 0)
 		return;
 
+	led_trigger_unregister_simple(port->led_trigger_rx);
+	led_trigger_unregister_simple(port->led_trigger_tx);
+
 	tty_unregister_device(driver, index);
 }
 EXPORT_SYMBOL_GPL(tty_port_unregister_device);
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 1dd587ba6d88..7e48de671bfa 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -13,6 +13,7 @@
 #include <uapi/linux/tty.h>
 #include <linux/rwsem.h>
 #include <linux/llist.h>
+#include <linux/leds.h>
 
 
 /*
@@ -249,8 +250,29 @@ struct tty_port {
 						   set to size of fifo */
 	struct kref		kref;		/* Ref counter */
 	void 			*client_data;
+
+	struct led_trigger	*led_trigger_rx;
+	struct led_trigger	*led_trigger_tx;
 };
 
+static inline void tty_led_trigger(struct led_trigger *trig)
+{
+	unsigned long delay_ms = 50;
+
+	if (IS_ENABLED(CONFIG_TTY_LEDS_TRIGGERS))
+		led_trigger_blink_oneshot(trig, &delay_ms, &delay_ms, 0);
+}
+
+static inline void tty_led_trigger_rx(struct tty_port *port)
+{
+	tty_led_trigger(port->led_trigger_rx);
+}
+
+static inline void tty_led_trigger_tx(struct tty_port *port)
+{
+	tty_led_trigger(port->led_trigger_tx);
+}
+
 /* tty_port::iflags bits -- use atomic bit ops */
 #define TTY_PORT_INITIALIZED	0	/* device is initialized */
 #define TTY_PORT_SUSPENDED	1	/* device is suspended */
-- 
2.17.0

^ permalink raw reply related

* [PATCH v3 2/3] can: simplify LED trigger handling
From: Uwe Kleine-König @ 2018-05-08 10:05 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Jiri Slaby, Johan Hovold, Jacek Anaszewski,
	Pavel Machek
  Cc: linux-serial, linux-leds, linux-can, kernel, One Thousand Gnomes,
	Florian Fainelli, Mathieu Poirier, linux-kernel, Robin Murphy,
	linux-arm-kernel
In-Reply-To: <20180508100543.12559-1-u.kleine-koenig@pengutronix.de>

The new function led_trigger_register_format allows one to simplify LED
trigger handling considerably.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/net/can/led.c   | 30 +++++++-----------------------
 include/linux/can/dev.h |  3 ---
 2 files changed, 7 insertions(+), 26 deletions(-)

diff --git a/drivers/net/can/led.c b/drivers/net/can/led.c
index 2d7d1b0d20f9..111f9769e9da 100644
--- a/drivers/net/can/led.c
+++ b/drivers/net/can/led.c
@@ -81,19 +81,9 @@ void devm_can_led_init(struct net_device *netdev)
 		return;
 	}
 
-	snprintf(priv->tx_led_trig_name, sizeof(priv->tx_led_trig_name),
-		 "%s-tx", netdev->name);
-	snprintf(priv->rx_led_trig_name, sizeof(priv->rx_led_trig_name),
-		 "%s-rx", netdev->name);
-	snprintf(priv->rxtx_led_trig_name, sizeof(priv->rxtx_led_trig_name),
-		 "%s-rxtx", netdev->name);
-
-	led_trigger_register_simple(priv->tx_led_trig_name,
-				    &priv->tx_led_trig);
-	led_trigger_register_simple(priv->rx_led_trig_name,
-				    &priv->rx_led_trig);
-	led_trigger_register_simple(priv->rxtx_led_trig_name,
-				    &priv->rxtx_led_trig);
+	led_trigger_register_format(&priv->tx_led_trig, "%s-tx", netdev->name);
+	led_trigger_register_format(&priv->rx_led_trig, "%s-rx", netdev->name);
+	led_trigger_register_format(&priv->rxtx_led_trig, "%s-rxtx", netdev->name);
 
 	devres_add(&netdev->dev, res);
 }
@@ -105,23 +95,17 @@ static int can_led_notifier(struct notifier_block *nb, unsigned long msg,
 {
 	struct net_device *netdev = netdev_notifier_info_to_dev(ptr);
 	struct can_priv *priv = safe_candev_priv(netdev);
-	char name[CAN_LED_NAME_SZ];
 
 	if (!priv)
 		return NOTIFY_DONE;
 
-	if (!priv->tx_led_trig || !priv->rx_led_trig || !priv->rxtx_led_trig)
-		return NOTIFY_DONE;
-
 	if (msg == NETDEV_CHANGENAME) {
-		snprintf(name, sizeof(name), "%s-tx", netdev->name);
-		led_trigger_rename(priv->tx_led_trig, name);
+		led_trigger_rename(priv->tx_led_trig, "%s-tx", netdev->name);
 
-		snprintf(name, sizeof(name), "%s-rx", netdev->name);
-		led_trigger_rename(priv->rx_led_trig, name);
+		led_trigger_rename(priv->rx_led_trig, "%s-rx", netdev->name);
 
-		snprintf(name, sizeof(name), "%s-rxtx", netdev->name);
-		led_trigger_rename(priv->rxtx_led_trig, name);
+		led_trigger_rename(priv->rxtx_led_trig,
+				   "%s-rxtx", netdev->name);
 	}
 
 	return NOTIFY_DONE;
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 055aaf5ed9af..ff358269aa9c 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -73,11 +73,8 @@ struct can_priv {
 
 #ifdef CONFIG_CAN_LEDS
 	struct led_trigger *tx_led_trig;
-	char tx_led_trig_name[CAN_LED_NAME_SZ];
 	struct led_trigger *rx_led_trig;
-	char rx_led_trig_name[CAN_LED_NAME_SZ];
 	struct led_trigger *rxtx_led_trig;
-	char rxtx_led_trig_name[CAN_LED_NAME_SZ];
 #endif
 };
 
-- 
2.17.0

^ permalink raw reply related

* [PATCH v3 1/3] leds: triggers: provide led_trigger_register_format()
From: Uwe Kleine-König @ 2018-05-08 10:05 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Jiri Slaby, Johan Hovold, Jacek Anaszewski,
	Pavel Machek
  Cc: linux-serial, linux-leds, linux-can, kernel, One Thousand Gnomes,
	Florian Fainelli, Mathieu Poirier, linux-kernel, Robin Murphy,
	linux-arm-kernel
In-Reply-To: <20180508100543.12559-1-u.kleine-koenig@pengutronix.de>

This allows one to simplify drivers that provide a trigger with a
non-constant name (e.g. one trigger per device with the trigger name
depending on the device's name).

Internally the memory the name member of struct led_trigger points to
now always allocated dynamically instead of just taken from the caller.

The function led_trigger_rename_static() must be changed accordingly and
was renamed to led_trigger_rename() for consistency, with the only user
adapted.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/leds/led-triggers.c | 84 +++++++++++++++++++++++++++----------
 drivers/net/can/led.c       |  6 +--
 include/linux/leds.h        | 30 +++++++------
 3 files changed, 81 insertions(+), 39 deletions(-)

diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
index 431123b048a2..5d8bb504b07b 100644
--- a/drivers/leds/led-triggers.c
+++ b/drivers/leds/led-triggers.c
@@ -175,18 +175,34 @@ void led_trigger_set_default(struct led_classdev *led_cdev)
 }
 EXPORT_SYMBOL_GPL(led_trigger_set_default);
 
-void led_trigger_rename_static(const char *name, struct led_trigger *trig)
+int led_trigger_rename(struct led_trigger *trig, const char *fmt, ...)
 {
-	/* new name must be on a temporary string to prevent races */
-	BUG_ON(name == trig->name);
+	const char *prevname;
+	const char *newname;
+	va_list args;
+
+	if (!trig)
+		return 0;
+
+	va_start(args, fmt);
+	newname = kvasprintf_const(GFP_KERNEL, fmt, args);
+	va_end(args);
+
+	if (!newname) {
+		pr_err("Failed to allocate new name for trigger %s\n", trig->name);
+		return -ENOMEM;
+	}
 
 	down_write(&triggers_list_lock);
-	/* this assumes that trig->name was originaly allocated to
-	 * non constant storage */
-	strcpy((char *)trig->name, name);
+	prevname = trig->name;
+	trig->name = newname;
 	up_write(&triggers_list_lock);
+
+	kfree_const(prevname);
+
+	return 0;
 }
-EXPORT_SYMBOL_GPL(led_trigger_rename_static);
+EXPORT_SYMBOL_GPL(led_trigger_rename);
 
 /* LED Trigger Interface */
 
@@ -333,34 +349,56 @@ void led_trigger_blink_oneshot(struct led_trigger *trig,
 }
 EXPORT_SYMBOL_GPL(led_trigger_blink_oneshot);
 
-void led_trigger_register_simple(const char *name, struct led_trigger **tp)
+int led_trigger_register_format(struct led_trigger **tp, const char *fmt, ...)
 {
+	va_list args;
 	struct led_trigger *trig;
-	int err;
+	int err = -ENOMEM;
+	const char *name;
+
+	va_start(args, fmt);
+	name = kvasprintf_const(GFP_KERNEL, fmt, args);
+	va_end(args);
 
 	trig = kzalloc(sizeof(struct led_trigger), GFP_KERNEL);
 
-	if (trig) {
-		trig->name = name;
-		err = led_trigger_register(trig);
-		if (err < 0) {
-			kfree(trig);
-			trig = NULL;
-			pr_warn("LED trigger %s failed to register (%d)\n",
-				name, err);
-		}
-	} else {
-		pr_warn("LED trigger %s failed to register (no memory)\n",
-			name);
-	}
+	if (!name || !trig)
+		goto err;
+
+	trig->name = name;
+
+	err = led_trigger_register(trig);
+	if (err < 0)
+		goto err;
+
 	*tp = trig;
+
+	return 0;
+
+err:
+	kfree(trig);
+	kfree_const(name);
+
+	*tp = NULL;
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(led_trigger_register_format);
+
+void led_trigger_register_simple(const char *name, struct led_trigger **tp)
+{
+	int ret = led_trigger_register_format(tp, "%s", name);
+	if (ret < 0)
+		pr_warn("LED trigger %s failed to register (%d)\n", name, ret);
 }
 EXPORT_SYMBOL_GPL(led_trigger_register_simple);
 
 void led_trigger_unregister_simple(struct led_trigger *trig)
 {
-	if (trig)
+	if (trig) {
 		led_trigger_unregister(trig);
+		kfree_const(trig->name);
+	}
 	kfree(trig);
 }
 EXPORT_SYMBOL_GPL(led_trigger_unregister_simple);
diff --git a/drivers/net/can/led.c b/drivers/net/can/led.c
index c1b667675fa1..2d7d1b0d20f9 100644
--- a/drivers/net/can/led.c
+++ b/drivers/net/can/led.c
@@ -115,13 +115,13 @@ static int can_led_notifier(struct notifier_block *nb, unsigned long msg,
 
 	if (msg == NETDEV_CHANGENAME) {
 		snprintf(name, sizeof(name), "%s-tx", netdev->name);
-		led_trigger_rename_static(name, priv->tx_led_trig);
+		led_trigger_rename(priv->tx_led_trig, name);
 
 		snprintf(name, sizeof(name), "%s-rx", netdev->name);
-		led_trigger_rename_static(name, priv->rx_led_trig);
+		led_trigger_rename(priv->rx_led_trig, name);
 
 		snprintf(name, sizeof(name), "%s-rxtx", netdev->name);
-		led_trigger_rename_static(name, priv->rxtx_led_trig);
+		led_trigger_rename(priv->rxtx_led_trig, name);
 	}
 
 	return NOTIFY_DONE;
diff --git a/include/linux/leds.h b/include/linux/leds.h
index b7e82550e655..e706c28bb35b 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -275,6 +275,8 @@ extern void led_trigger_unregister(struct led_trigger *trigger);
 extern int devm_led_trigger_register(struct device *dev,
 				     struct led_trigger *trigger);
 
+extern int led_trigger_register_format(struct led_trigger **trigger,
+				       const char *fmt, ...);
 extern void led_trigger_register_simple(const char *name,
 				struct led_trigger **trigger);
 extern void led_trigger_unregister_simple(struct led_trigger *trigger);
@@ -298,28 +300,25 @@ static inline void *led_get_trigger_data(struct led_classdev *led_cdev)
 }
 
 /**
- * led_trigger_rename_static - rename a trigger
- * @name: the new trigger name
+ * led_trigger_rename - rename a trigger
  * @trig: the LED trigger to rename
+ * @fmt: format string for new name
  *
- * Change a LED trigger name by copying the string passed in
- * name into current trigger name, which MUST be large
- * enough for the new string.
- *
- * Note that name must NOT point to the same string used
- * during LED registration, as that could lead to races.
- *
- * This is meant to be used on triggers with statically
- * allocated name.
+ * rebaptize the given trigger.
  */
-extern void led_trigger_rename_static(const char *name,
-				      struct led_trigger *trig);
+extern int led_trigger_rename(struct led_trigger *trig, const char *fmt, ...);
 
 #else
 
 /* Trigger has no members */
 struct led_trigger {};
 
+static inline int led_trigger_register_format(struct led_trigger **trigger,
+					      const char *fmt, ...)
+{
+	return 0;
+}
+
 /* Trigger inline empty functions */
 static inline void led_trigger_register_simple(const char *name,
 					struct led_trigger **trigger) {}
@@ -342,6 +341,11 @@ static inline void *led_get_trigger_data(struct led_classdev *led_cdev)
 	return NULL;
 }
 
+static inline int led_trigger_rename(struct led_trigger *trig, const char *fmt, ...)
+{
+	return 0;
+}
+
 #endif /* CONFIG_LEDS_TRIGGERS */
 
 /* Trigger specific functions */
-- 
2.17.0

^ permalink raw reply related

* [PATCH v3 0/3] led_trigger_register_format and tty triggers
From: Uwe Kleine-König @ 2018-05-08 10:05 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Jiri Slaby, Johan Hovold, Jacek Anaszewski,
	Pavel Machek
  Cc: linux-serial, linux-leds, linux-can, kernel, One Thousand Gnomes,
	Florian Fainelli, Mathieu Poirier, linux-kernel, Robin Murphy,
	linux-arm-kernel

Hello,

while working on a patch that adds led triggers to drivers/tty (patch 3)
I thought that being able to pass a format string (and the respective
parameters) to led_trigger_register_simple instead of a constant string
would be nice. This is implemented in the first patch. The second patch
converts the can leds to this new function which demonstrates nicely the
added benefit for users. Both patches are new in v3.

The third patch finally implements the triggers for the tty framework.
Compared to v2 I reduced the need for #ifdefs, make use of
led_trigger_register_format() and excluded serdev devices from
triggering as suggested by Johan Hovold.

Also code cleanup in the error case is done now and hopefully the kbuild
test robot is happy now.

Best regards
Uwe

Uwe Kleine-König (3):
  leds: triggers: provide led_trigger_register_format()
  can: simplify LED trigger handling
  tty: implement led triggers

 arch/arm/boot/dts/imx25-logitech-baby.dts | 192 ++++++++++++++++++++++
 drivers/leds/led-triggers.c               |  84 +++++++---
 drivers/net/can/led.c                     |  30 +---
 drivers/tty/Kconfig                       |   7 +
 drivers/tty/tty_buffer.c                  |   2 +
 drivers/tty/tty_io.c                      |   3 +
 drivers/tty/tty_port.c                    |  32 +++-
 include/linux/can/dev.h                   |   3 -
 include/linux/leds.h                      |  30 ++--
 include/linux/tty.h                       |  22 +++
 10 files changed, 341 insertions(+), 64 deletions(-)
 create mode 100644 arch/arm/boot/dts/imx25-logitech-baby.dts

-- 
2.17.0

^ permalink raw reply

* Re: [PATCH v3] serial: sh-sci: Use spin_{try}lock_irqsave instead of open coding version
From: Geert Uytterhoeven @ 2018-05-08  9:52 UTC (permalink / raw)
  To: Daniel Wagner
  Cc: Linux Kernel Mailing List, linux-rt-users,
	open list:SERIAL DRIVERS, Linux-sh list, Greg KH, Daniel Wagner,
	Sebastian Andrzej Siewior
In-Reply-To: <20180508085509.31384-1-wagi@monom.org>

On Tue, May 8, 2018 at 10:55 AM, Daniel Wagner <wagi@monom.org> wrote:
> From: Daniel Wagner <daniel.wagner@siemens.com>
>
> Commit 40f70c03e33a ("serial: sh-sci: add locking to console write
> function to avoid SMP lockup") copied the strategy to avoid locking
> problems in conjuncture with the console from the UART8250
> driver. Instead using directly spin_{try}lock_irqsave(),
> local_irq_save() followed by spin_{try}lock() was used. While this is
> correct on mainline, for -rt it is a problem. spin_{try}lock() will
> check if it is running in a valid context. Since the local_irq_save()
> has already been executed, the context has changed and
> spin_{try}lock() will complain. The reason why spin_{try}lock()
> complains is that on -rt the spin locks are turned into mutexes and
> therefore can sleep. Sleeping with interrupts disabled is not valid.

[...]

> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Signed-off-by: Daniel Wagner <daniel.wagner@siemens.com>
> ---
> changes since v2:
>  - dropped the local_irq_save() it's wrong as Sebastian pointed out
>
> changes since v1:
>  - Ported to current mainline (initial version was against v4.4.y)
>  - Left local_irq_save() in place when spinlocks are not used as suggested
>    by Geert.

Thanks for the update!

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

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

* Re: [PATCH v2] serial: 8250_of: Add IO space support
From: John Garry @ 2018-05-08  9:15 UTC (permalink / raw)
  To: gregkh, jslaby, joel, p.zabel, arnd, fcooper, sergei.shtylyov,
	yamada.masahiro, khoroshilov
  Cc: linux-serial, linux-kernel, andriy.shevchenko, linuxarm
In-Reply-To: <1524825366-155559-1-git-send-email-john.garry@huawei.com>

On 27/04/2018 11:36, John Garry wrote:
> Currently the 8250_of driver only supports MEM IO type
> accesses.
>
> Some development boards (Huawei D03, specifically) require
> IO space access for 8250-compatible OF driver support, so
> add it.
>
> The modification is quite simple: just set the port iotype
> and associated flags depending on the device address
> resource type.
>
> Signed-off-by: John Garry <john.garry@huawei.com>
> ---

Hi Greg,

Just a gentle reminder of this patch. I didn't see any response.

BTW, I think that this patch should still cleanly apply to your 
tty-testing branch.

Thanks,
John

>
> Changes v1->v2:
> - rebase to Greg's tty-testing branch
> - use resource_type() helper
>
> diff --git a/drivers/tty/serial/8250/8250_of.c b/drivers/tty/serial/8250/8250_of.c
> index 3de8d6a..bfb37f0 100644
> --- a/drivers/tty/serial/8250/8250_of.c
> +++ b/drivers/tty/serial/8250/8250_of.c
> @@ -92,13 +92,43 @@ static int of_platform_serial_setup(struct platform_device *ofdev,
>  		goto err_unprepare;
>  	}
>
> +	port->flags = UPF_SHARE_IRQ | UPF_BOOT_AUTOCONF | UPF_FIXED_PORT |
> +				  UPF_FIXED_TYPE;
>  	spin_lock_init(&port->lock);
> -	port->mapbase = resource.start;
> -	port->mapsize = resource_size(&resource);
>
> -	/* Check for shifted address mapping */
> -	if (of_property_read_u32(np, "reg-offset", &prop) == 0)
> -		port->mapbase += prop;
> +	if (resource_type(&resource) == IORESOURCE_IO) {
> +		port->iotype = UPIO_PORT;
> +		port->iobase = resource.start;
> +	} else {
> +		port->mapbase = resource.start;
> +		port->mapsize = resource_size(&resource);
> +
> +		/* Check for shifted address mapping */
> +		if (of_property_read_u32(np, "reg-offset", &prop) == 0)
> +			port->mapbase += prop;
> +
> +		port->iotype = UPIO_MEM;
> +		if (of_property_read_u32(np, "reg-io-width", &prop) == 0) {
> +			switch (prop) {
> +			case 1:
> +				port->iotype = UPIO_MEM;
> +				break;
> +			case 2:
> +				port->iotype = UPIO_MEM16;
> +				break;
> +			case 4:
> +				port->iotype = of_device_is_big_endian(np) ?
> +					       UPIO_MEM32BE : UPIO_MEM32;
> +				break;
> +			default:
> +				dev_warn(&ofdev->dev, "unsupported reg-io-width (%d)\n",
> +					 prop);
> +				ret = -EINVAL;
> +				goto err_dispose;
> +			}
> +		}
> +		port->flags |= UPF_IOREMAP;
> +	}
>
>  	/* Check for registers offset within the devices address range */
>  	if (of_property_read_u32(np, "reg-shift", &prop) == 0)
> @@ -114,26 +144,6 @@ static int of_platform_serial_setup(struct platform_device *ofdev,
>  		port->line = ret;
>
>  	port->irq = irq_of_parse_and_map(np, 0);
> -	port->iotype = UPIO_MEM;
> -	if (of_property_read_u32(np, "reg-io-width", &prop) == 0) {
> -		switch (prop) {
> -		case 1:
> -			port->iotype = UPIO_MEM;
> -			break;
> -		case 2:
> -			port->iotype = UPIO_MEM16;
> -			break;
> -		case 4:
> -			port->iotype = of_device_is_big_endian(np) ?
> -				       UPIO_MEM32BE : UPIO_MEM32;
> -			break;
> -		default:
> -			dev_warn(&ofdev->dev, "unsupported reg-io-width (%d)\n",
> -				 prop);
> -			ret = -EINVAL;
> -			goto err_dispose;
> -		}
> -	}
>
>  	info->rst = devm_reset_control_get_optional_shared(&ofdev->dev, NULL);
>  	if (IS_ERR(info->rst)) {
> @@ -147,8 +157,6 @@ static int of_platform_serial_setup(struct platform_device *ofdev,
>
>  	port->type = type;
>  	port->uartclk = clk;
> -	port->flags = UPF_SHARE_IRQ | UPF_BOOT_AUTOCONF | UPF_IOREMAP
> -		| UPF_FIXED_PORT | UPF_FIXED_TYPE;
>  	port->irqflags |= IRQF_SHARED;
>
>  	if (of_property_read_bool(np, "no-loopback-test"))
>

^ permalink raw reply

* [PATCH v3] serial: sh-sci: Use spin_{try}lock_irqsave instead of open coding version
From: Daniel Wagner @ 2018-05-08  8:55 UTC (permalink / raw)
  To: linux-kernel
  Cc: linux-rt-users, linux-serial, linux-sh, gregkh, Daniel Wagner,
	Geert Uytterhoeven, Sebastian Andrzej Siewior

From: Daniel Wagner <daniel.wagner@siemens.com>

Commit 40f70c03e33a ("serial: sh-sci: add locking to console write
function to avoid SMP lockup") copied the strategy to avoid locking
problems in conjuncture with the console from the UART8250
driver. Instead using directly spin_{try}lock_irqsave(),
local_irq_save() followed by spin_{try}lock() was used. While this is
correct on mainline, for -rt it is a problem. spin_{try}lock() will
check if it is running in a valid context. Since the local_irq_save()
has already been executed, the context has changed and
spin_{try}lock() will complain. The reason why spin_{try}lock()
complains is that on -rt the spin locks are turned into mutexes and
therefore can sleep. Sleeping with interrupts disabled is not valid.

BUG: sleeping function called from invalid context at /home/wagi/work/rt/v4.4-cip-rt/kernel/locking/rtmutex.c:995
in_atomic(): 0, irqs_disabled(): 128, pid: 778, name: irq/76-eth0
CPU: 0 PID: 778 Comm: irq/76-eth0 Not tainted 4.4.126-test-cip22-rt14-00403-gcd03665c8318 #12
Hardware name: Generic RZ/G1 (Flattened Device Tree)
Backtrace:
[<c00140a0>] (dump_backtrace) from [<c001424c>] (show_stack+0x18/0x1c)
 r7:c06b01f0 r6:60010193 r5:00000000 r4:c06b01f0
[<c0014234>] (show_stack) from [<c01d3c94>] (dump_stack+0x78/0x94)
[<c01d3c1c>] (dump_stack) from [<c004c134>] (___might_sleep+0x134/0x194)
 r7:60010113 r6:c06d3559 r5:00000000 r4:ffffe000
[<c004c000>] (___might_sleep) from [<c04ded60>] (rt_spin_lock+0x20/0x74)
 r5:c06f4d60 r4:c06f4d60
[<c04ded40>] (rt_spin_lock) from [<c02577e4>] (serial_console_write+0x100/0x118)
 r5:c06f4d60 r4:c06f4d60
[<c02576e4>] (serial_console_write) from [<c0061060>] (call_console_drivers.constprop.15+0x10c/0x124)
 r10:c06d2894 r9:c04e18b0 r8:00000028 r7:00000000 r6:c06d3559 r5:c06d2798
 r4:c06b9914 r3:c02576e4
[<c0060f54>] (call_console_drivers.constprop.15) from [<c0062984>] (console_unlock+0x32c/0x430)
 r10:c06d30d8 r9:00000028 r8:c06dd518 r7:00000005 r6:00000000 r5:c06d2798
 r4:c06d2798 r3:00000028
[<c0062658>] (console_unlock) from [<c0062e1c>] (vprintk_emit+0x394/0x4f0)
 r10:c06d2798 r9:c06d30ee r8:00000006 r7:00000005 r6:c06a78fc r5:00000027
 r4:00000003
[<c0062a88>] (vprintk_emit) from [<c0062fa0>] (vprintk+0x28/0x30)
 r10:c060bd46 r9:00001000 r8:c06b9a90 r7:c06b9a90 r6:c06b994c r5:c06b9a3c
 r4:c0062fa8
[<c0062f78>] (vprintk) from [<c0062fb8>] (vprintk_default+0x10/0x14)
[<c0062fa8>] (vprintk_default) from [<c009cd30>] (printk+0x78/0x84)
[<c009ccbc>] (printk) from [<c025afdc>] (credit_entropy_bits+0x17c/0x2cc)
 r3:00000001 r2:decade60 r1:c061a5ee r0:c061a523
 r4:00000006
[<c025ae60>] (credit_entropy_bits) from [<c025bf74>] (add_interrupt_randomness+0x160/0x178)
 r10:466e7196 r9:1f536000 r8:fffeef74 r7:00000000 r6:c06b9a60 r5:c06b9a3c
 r4:dfbcf680
[<c025be14>] (add_interrupt_randomness) from [<c006536c>] (irq_thread+0x1e8/0x248)
 r10:c006537c r9:c06cdf21 r8:c0064fcc r7:df791c24 r6:df791c00 r5:ffffe000
 r4:df525180
[<c0065184>] (irq_thread) from [<c003fba4>] (kthread+0x108/0x11c)
 r10:00000000 r9:00000000 r8:c0065184 r7:df791c00 r6:00000000 r5:df791d00
 r4:decac000
[<c003fa9c>] (kthread) from [<c00101b8>] (ret_from_fork+0x14/0x3c)
 r8:00000000 r7:00000000 r6:00000000 r5:c003fa9c r4:df791d00

Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Daniel Wagner <daniel.wagner@siemens.com>
---
changes since v2:
 - dropped the local_irq_save() it's wrong as Sebastian pointed out

changes since v1:
 - Ported to current mainline (initial version was against v4.4.y)
 - Left local_irq_save() in place when spinlocks are not used as suggested
   by Geert.

 drivers/tty/serial/sh-sci.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index fdbbff547106..8c8dcced1c25 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -2890,16 +2890,15 @@ static void serial_console_write(struct console *co, const char *s,
 	unsigned long flags;
 	int locked = 1;
 
-	local_irq_save(flags);
 #if defined(SUPPORT_SYSRQ)
 	if (port->sysrq)
 		locked = 0;
 	else
 #endif
 	if (oops_in_progress)
-		locked = spin_trylock(&port->lock);
+		locked = spin_trylock_irqsave(&port->lock, flags);
 	else
-		spin_lock(&port->lock);
+		spin_lock_irqsave(&port->lock, flags);
 
 	/* first save SCSCR then disable interrupts, keep clock source */
 	ctrl = serial_port_in(port, SCSCR);
@@ -2919,8 +2918,7 @@ static void serial_console_write(struct console *co, const char *s,
 	serial_port_out(port, SCSCR, ctrl);
 
 	if (locked)
-		spin_unlock(&port->lock);
-	local_irq_restore(flags);
+		spin_unlock_irqrestore(&port->lock, flags);
 }
 
 static int serial_console_setup(struct console *co, char *options)
-- 
2.14.3

^ permalink raw reply related

* Re: [PATCH v2] serial: sh-sci: Use spin_{try}lock_irqsave instead of open coding version
From: Sebastian Andrzej Siewior @ 2018-05-08  7:40 UTC (permalink / raw)
  To: Daniel Wagner
  Cc: linux-kernel, Geert Uytterhoeven, linux-rt-users, linux-serial,
	linux-sh, gregkh, Daniel Wagner
In-Reply-To: <d37e074d-5e8c-84e8-5833-7ff595e6348c@monom.org>

On 2018-05-08 09:18:44 [+0200], Daniel Wagner wrote:
> Hi Sebastian,
Hi,

> Should 'echo t > /proc/sysrq' trigger the splat? At least I was so naive
> that think it would be enough.

No, this is a different interface. The thing is you send the BREAK
command and then, the next character (within a timeout) that comes over
the UART is the MAGIC request. While that character is being received
the sysrq request is answered and output is written to the console which
is probably the UART and so you can't acquire the lock again.

So you can't acquire the lock. I added a try_lock once to at least try
to acquire the lock because this may race against a printk() from
another CPU. But then someone complained about a failed try_lock on UP
(this can't happen on UP unless in a scenario like this where the lock
is already acquired) so this change to the 8250 was reverted.

So the whole situation of the console interface is not perfect and this
is the duct tape we have. It would good to have the same duct tape in
all drivers or come up with a different interface to cover corner cases
:)

To reproduce the sysrq code path: You need to the UART as a console and
send a BREAK (CTRL-A F in minicom) followed by `t' to match your
example.

> Thanks,
> Daniel

Sebastian

^ permalink raw reply

* Re: [PATCH] serial: sh-sci: Use spin_{try}lock_irqsave instead of open coding version
From: Geert Uytterhoeven @ 2018-05-08  7:29 UTC (permalink / raw)
  To: Daniel Wagner
  Cc: Sebastian Andrzej Siewior, Linux Kernel Mailing List,
	linux-rt-users, open list:SERIAL DRIVERS, Greg Kroah-Hartman,
	Linux-sh list, Daniel Wagner, Shinya Kuribayashi
In-Reply-To: <66cd1363-9f62-e2f5-2271-1fa78509f2cd@monom.org>

Hi Daniel,

On Tue, May 8, 2018 at 9:23 AM, Daniel Wagner <wagi@monom.org> wrote:
> On 05/07/2018 02:47 PM, Sebastian Andrzej Siewior wrote:
>> On 2018-05-03 09:43:33 [+0200], Geert Uytterhoeven wrote:
>>>> --- a/drivers/tty/serial/sh-sci.c
>>>> +++ b/drivers/tty/serial/sh-sci.c
>>>> @@ -2516,13 +2516,12 @@ static void serial_console_write(struct console
>>>> *co, const char *s,
>>>>          unsigned long flags;
>>>>          int locked = 1;
>>>>
>>>> -       local_irq_save(flags);
>>>
>>>
>>> Hence the below now runs with local interrupts enabled.
>>>
>>> For checking port->sysrq or oops_in_progress that probably isn't an
>>> issue.
>>> If oops_in_progress is set, you have other problems, and the race
>>> condition
>>> between checking the flag and calling spin_lock{,_irqsave}() existed
>>> before,
>>> and is hard to avoid.
>>
>> while oops_in_progress is an issue of its own, the port->sysrq isn't
>> avoided by by local_irq_save(). On SMP systems you can still receive a
>> `break' signal on the UART and have a `printk()' issued on another CPU.
>>
>>> For actual console printing, I think you want to keep interrupts
>>> disabled.
>>
>> why? They should be disabled as part of getting the lock and not for any
>> other reason.
>>
>>>>          if (port->sysrq)
>>>>                  locked = 0;
>>>>          else if (oops_in_progress)
>>>> -               locked = spin_trylock(&port->lock);
>>>> +               locked = spin_trylock_irqsave(&port->lock, flags);
>>>>          else
>>>> -               spin_lock(&port->lock);
>>>> +               spin_lock_irqsave(&port->lock, flags);
>>>
>>>
>>> Add
>>>
>>>          if (!locked
>>>                  local_irq_save(flags)
>>>
>>> here?
>>
>>
>> So for oops_in_progress you get here with interrupts disabled. And if
>> not, I don't see the point in disabling the interrupts without any kind
>> of locking.
>
>
> So I understand, the initial version of this patch was correct.
>
> @Geert if you don't object I'll send a v3 (v1 ported to mainline).

Please go ahead, thanks!

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

* Re: [PATCH] serial: sh-sci: Use spin_{try}lock_irqsave instead of open coding version
From: Daniel Wagner @ 2018-05-08  7:23 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior, Geert Uytterhoeven
  Cc: Linux Kernel Mailing List, linux-rt-users,
	open list:SERIAL DRIVERS, Greg Kroah-Hartman, Linux-sh list,
	Daniel Wagner, Shinya Kuribayashi
In-Reply-To: <20180507124704.s4qlrcc3leoky4r7@linutronix.de>

On 05/07/2018 02:47 PM, Sebastian Andrzej Siewior wrote:
> On 2018-05-03 09:43:33 [+0200], Geert Uytterhoeven wrote:
>>> --- a/drivers/tty/serial/sh-sci.c
>>> +++ b/drivers/tty/serial/sh-sci.c
>>> @@ -2516,13 +2516,12 @@ static void serial_console_write(struct console *co, const char *s,
>>>          unsigned long flags;
>>>          int locked = 1;
>>>
>>> -       local_irq_save(flags);
>>
>> Hence the below now runs with local interrupts enabled.
>>
>> For checking port->sysrq or oops_in_progress that probably isn't an issue.
>> If oops_in_progress is set, you have other problems, and the race condition
>> between checking the flag and calling spin_lock{,_irqsave}() existed before,
>> and is hard to avoid.
> 
> while oops_in_progress is an issue of its own, the port->sysrq isn't
> avoided by by local_irq_save(). On SMP systems you can still receive a
> `break' signal on the UART and have a `printk()' issued on another CPU.
> 
>> For actual console printing, I think you want to keep interrupts disabled.
> 
> why? They should be disabled as part of getting the lock and not for any
> other reason.
> 
>>>          if (port->sysrq)
>>>                  locked = 0;
>>>          else if (oops_in_progress)
>>> -               locked = spin_trylock(&port->lock);
>>> +               locked = spin_trylock_irqsave(&port->lock, flags);
>>>          else
>>> -               spin_lock(&port->lock);
>>> +               spin_lock_irqsave(&port->lock, flags);
>>
>> Add
>>
>>          if (!locked
>>                  local_irq_save(flags)
>>
>> here?
> 
> So for oops_in_progress you get here with interrupts disabled. And if
> not, I don't see the point in disabling the interrupts without any kind
> of locking.

So I understand, the initial version of this patch was correct.

@Geert if you don't object I'll send a v3 (v1 ported to mainline).

Thanks,
Daniel

^ permalink raw reply

* Re: [PATCH v2] serial: sh-sci: Use spin_{try}lock_irqsave instead of open coding version
From: Daniel Wagner @ 2018-05-08  7:18 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior
  Cc: linux-kernel, Geert Uytterhoeven, linux-rt-users, linux-serial,
	linux-sh, gregkh, Daniel Wagner
In-Reply-To: <20180507125146.75crpaj2scav7mql@linutronix.de>

Hi Sebastian,

On 05/07/2018 02:51 PM, Sebastian Andrzej Siewior wrote:
> On 2018-05-04 18:30:41 [+0200], Daniel Wagner wrote:
>> --- a/drivers/tty/serial/sh-sci.c
>> +++ b/drivers/tty/serial/sh-sci.c
>> @@ -2890,16 +2890,16 @@ static void serial_console_write(struct console *co, const char *s,
>>   	unsigned long flags;
>>   	int locked = 1;
>>   
>> -	local_irq_save(flags);
>>   #if defined(SUPPORT_SYSRQ)
>> -	if (port->sysrq)
>> +	if (port->sysrq) {
>>   		locked = 0;
>> -	else
>> +		local_irq_save(flags);
> 
> how is this helping? You should see a splat after a sysrq request.

You are right, I didn't really think this through.

Should 'echo t > /proc/sysrq' trigger the splat? At least I was so naive 
that think it would be enough.

Thanks,
Daniel

^ permalink raw reply

* Re: [PATCH v3 2/3] platform: move the early platform device support to arch/sh
From: Geert Uytterhoeven @ 2018-05-08  6:45 UTC (permalink / raw)
  To: Bartosz Golaszewski
  Cc: Sekhar Nori, Kevin Hilman, David Lechner, Michael Turquette,
	Stephen Boyd, Arnd Bergmann, Greg Kroah-Hartman, Mark Rutland,
	Yoshinori Sato, Rich Felker, Andy Shevchenko, Marc Zyngier,
	Rafael J . Wysocki, Peter Rosin, Jiri Slaby, Thomas Gleixner,
	Daniel Lezcano, Magnus Damm, Linux ARM
In-Reply-To: <20180504132731.14574-3-brgl@bgdev.pl>

Hi Bartosz,

On Fri, May 4, 2018 at 3:27 PM, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> SuperH is the only user of the current implementation of early platform
> device support. We want to introduce a more robust approach to early
> probing. As the first step - move all the current early platform code
> to arch/sh.
>
> In order not to export internal drivers/base functions to arch code for
> this temporary solution - copy the two needed routines for driver
> matching from drivers/base/platform.c to arch/sh/drivers/platform_early.c.
>
> Also: call early_platform_cleanup() from subsys_initcall() so that it's
> called after all early devices are probed.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

>  drivers/clocksource/sh_cmt.c           |   7 +
>  drivers/clocksource/sh_mtu2.c          |   7 +
>  drivers/clocksource/sh_tmu.c           |   8 +
>  drivers/tty/serial/sh-sci.c            |   7 +-
>  include/linux/platform_device.h        |  64 +----

The parts used on contemporary ARM/ARM64 Renesas SoCs look fine to me, so
Acked-by: Geert Uytterhoeven <geert+renesas@glider.be>

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

* Re: [PATCH 2/2] serial: imx: dma_unmap_sg buffers on shutdown
From: Uwe Kleine-König @ 2018-05-08  6:43 UTC (permalink / raw)
  To: Sebastian Reichel
  Cc: Greg Kroah-Hartman, Jiri Slaby, linux-serial, Fabio Estevam,
	Shawn Guo, linux-kernel
In-Reply-To: <20180507213610.17330-3-sebastian.reichel@collabora.co.uk>

On Mon, May 07, 2018 at 11:36:10PM +0200, Sebastian Reichel wrote:
> This properly unmaps DMA SG on device shutdown.
> 
> Reported-by: Nandor Han <nandor.han@ge.com>
> Suggested-by: Nandor Han <nandor.han@ge.com>
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
> ---
>  drivers/tty/serial/imx.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 3ca767b1162a..6c53e74244ec 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -1425,10 +1425,18 @@ static void imx_uart_shutdown(struct uart_port *port)
>  	u32 ucr1, ucr2;
>  
>  	if (sport->dma_is_enabled) {
> -		sport->dma_is_rxing = 0;
> -		sport->dma_is_txing = 0;
>  		dmaengine_terminate_sync(sport->dma_chan_tx);
> +		if (sport->dma_is_txing) {
> +			dma_unmap_sg(sport->port.dev, &sport->tx_sgl[0],
> +				     sport->dma_tx_nents, DMA_TO_DEVICE);
> +			sport->dma_is_txing = 0;
> +		}

did you find this because the kernel crashed or consumed more and more
memory, or is this "only" a finding of reading the source code? If the
former it would be great to point out in the commit log, if the latter,
I wonder if this is a real problem that warrants a stable backport.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

^ permalink raw reply

* Re: [PATCH 1/2] serial: imx: cleanup imx_uart_disable_dma()
From: Uwe Kleine-König @ 2018-05-08  6:40 UTC (permalink / raw)
  To: Sebastian Reichel
  Cc: Greg Kroah-Hartman, Jiri Slaby, linux-serial, Fabio Estevam,
	Shawn Guo, linux-kernel
In-Reply-To: <20180507213610.17330-2-sebastian.reichel@collabora.co.uk>

Hello,

On Mon, May 07, 2018 at 11:36:09PM +0200, Sebastian Reichel wrote:
> Remove unrelated CTSC/CTS disabling from imx_uart_disable_dma() and
> move it to imx_uart_shutdown(), which is the only user of the DMA
> disabling function. This should not change the driver's behaviour,
> but improves readability. After this change imx_uart_disable_dma()
> does the reverse thing of imx_uart_enable_dma().
> 
> Suggested-by: Nandor Han <nandor.han@ge.com>
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
> ---
>  drivers/tty/serial/imx.c | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index c2fc6bef7a6f..3ca767b1162a 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -1291,18 +1291,13 @@ static void imx_uart_enable_dma(struct imx_port *sport)
>  
>  static void imx_uart_disable_dma(struct imx_port *sport)
>  {
> -	u32 ucr1, ucr2;
> +	u32 ucr1;
>  
>  	/* clear UCR1 */
>  	ucr1 = imx_uart_readl(sport, UCR1);
>  	ucr1 &= ~(UCR1_RXDMAEN | UCR1_TXDMAEN | UCR1_ATDMAEN);
>  	imx_uart_writel(sport, ucr1, UCR1);
>  
> -	/* clear UCR2 */
> -	ucr2 = imx_uart_readl(sport, UCR2);
> -	ucr2 &= ~(UCR2_CTSC | UCR2_CTS | UCR2_ATEN);
> -	imx_uart_writel(sport, ucr2, UCR2);
> -
>  	imx_uart_setup_ufcr(sport, TXTL_DEFAULT, RXTL_DEFAULT);
>  
>  	sport->dma_is_enabled = 0;
> @@ -1447,7 +1442,7 @@ static void imx_uart_shutdown(struct uart_port *port)
>  
>  	spin_lock_irqsave(&sport->port.lock, flags);
>  	ucr2 = imx_uart_readl(sport, UCR2);
> -	ucr2 &= ~(UCR2_TXEN | UCR2_ATEN);
> +	ucr2 &= ~(UCR2_TXEN | UCR2_CTSC | UCR2_CTS | UCR2_ATEN);
>  	imx_uart_writel(sport, ucr2, UCR2);
>  	spin_unlock_irqrestore(&sport->port.lock, flags);

While this doesn't change behaviour (which is of course good and
intended here) I wonder if changing RTS is right here.

According to Documentation/serial/driver it is not.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

^ permalink raw reply

* Re: [PATCH v3 1/3] clocksource: timer-ti-dm: remove the early platform driver registration
From: Daniel Lezcano @ 2018-05-08  1:40 UTC (permalink / raw)
  To: Bartosz Golaszewski
  Cc: Sekhar Nori, Kevin Hilman, David Lechner, Michael Turquette,
	Stephen Boyd, Arnd Bergmann, Greg Kroah-Hartman, Mark Rutland,
	Yoshinori Sato, Rich Felker, Andy Shevchenko, Marc Zyngier,
	Rafael J . Wysocki, Peter Rosin, Jiri Slaby, Thomas Gleixner,
	Geert Uytterhoeven, Magnus Damm, linux-arm-kernel, linux-kernel,
	linux-serial
In-Reply-To: <20180504132731.14574-2-brgl@bgdev.pl>

On Fri, May 04, 2018 at 03:27:29PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> This driver is no longer used as an early platform driver. Remove the
> registration macro.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>

-- 

 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

^ permalink raw reply

* [PATCH 2/2] serial: imx: dma_unmap_sg buffers on shutdown
From: Sebastian Reichel @ 2018-05-07 21:36 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Jiri Slaby, linux-serial
  Cc: Uwe Kleine-König, Fabio Estevam, Shawn Guo, linux-kernel,
	Sebastian Reichel
In-Reply-To: <20180507213610.17330-1-sebastian.reichel@collabora.co.uk>

This properly unmaps DMA SG on device shutdown.

Reported-by: Nandor Han <nandor.han@ge.com>
Suggested-by: Nandor Han <nandor.han@ge.com>
Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
---
 drivers/tty/serial/imx.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 3ca767b1162a..6c53e74244ec 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -1425,10 +1425,18 @@ static void imx_uart_shutdown(struct uart_port *port)
 	u32 ucr1, ucr2;
 
 	if (sport->dma_is_enabled) {
-		sport->dma_is_rxing = 0;
-		sport->dma_is_txing = 0;
 		dmaengine_terminate_sync(sport->dma_chan_tx);
+		if (sport->dma_is_txing) {
+			dma_unmap_sg(sport->port.dev, &sport->tx_sgl[0],
+				     sport->dma_tx_nents, DMA_TO_DEVICE);
+			sport->dma_is_txing = 0;
+		}
 		dmaengine_terminate_sync(sport->dma_chan_rx);
+		if (sport->dma_is_rxing) {
+			dma_unmap_sg(sport->port.dev, &sport->rx_sgl,
+				     1, DMA_FROM_DEVICE);
+			sport->dma_is_rxing = 0;
+		}
 
 		spin_lock_irqsave(&sport->port.lock, flags);
 		imx_uart_stop_tx(port);
-- 
2.17.0

^ permalink raw reply related

* [PATCH 1/2] serial: imx: cleanup imx_uart_disable_dma()
From: Sebastian Reichel @ 2018-05-07 21:36 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Jiri Slaby, linux-serial
  Cc: Uwe Kleine-König, Fabio Estevam, Shawn Guo, linux-kernel,
	Sebastian Reichel
In-Reply-To: <20180507213610.17330-1-sebastian.reichel@collabora.co.uk>

Remove unrelated CTSC/CTS disabling from imx_uart_disable_dma() and
move it to imx_uart_shutdown(), which is the only user of the DMA
disabling function. This should not change the driver's behaviour,
but improves readability. After this change imx_uart_disable_dma()
does the reverse thing of imx_uart_enable_dma().

Suggested-by: Nandor Han <nandor.han@ge.com>
Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
---
 drivers/tty/serial/imx.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index c2fc6bef7a6f..3ca767b1162a 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -1291,18 +1291,13 @@ static void imx_uart_enable_dma(struct imx_port *sport)
 
 static void imx_uart_disable_dma(struct imx_port *sport)
 {
-	u32 ucr1, ucr2;
+	u32 ucr1;
 
 	/* clear UCR1 */
 	ucr1 = imx_uart_readl(sport, UCR1);
 	ucr1 &= ~(UCR1_RXDMAEN | UCR1_TXDMAEN | UCR1_ATDMAEN);
 	imx_uart_writel(sport, ucr1, UCR1);
 
-	/* clear UCR2 */
-	ucr2 = imx_uart_readl(sport, UCR2);
-	ucr2 &= ~(UCR2_CTSC | UCR2_CTS | UCR2_ATEN);
-	imx_uart_writel(sport, ucr2, UCR2);
-
 	imx_uart_setup_ufcr(sport, TXTL_DEFAULT, RXTL_DEFAULT);
 
 	sport->dma_is_enabled = 0;
@@ -1447,7 +1442,7 @@ static void imx_uart_shutdown(struct uart_port *port)
 
 	spin_lock_irqsave(&sport->port.lock, flags);
 	ucr2 = imx_uart_readl(sport, UCR2);
-	ucr2 &= ~(UCR2_TXEN | UCR2_ATEN);
+	ucr2 &= ~(UCR2_TXEN | UCR2_CTSC | UCR2_CTS | UCR2_ATEN);
 	imx_uart_writel(sport, ucr2, UCR2);
 	spin_unlock_irqrestore(&sport->port.lock, flags);
 
-- 
2.17.0

^ permalink raw reply related


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox