[PATCH RFT 0/6] serial: sh-sci: Fixes for earlycon and keep_bootcon

[PATCH RFT 0/6] serial: sh-sci: Fixes for earlycon and keep_bootcon

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Hi,

This series adds fixes for earlycon and keep_bootcon on sh-sci driver.

Patch 1/6 was initially part of [1], then posted as standalone
fix at [2].

Patch 5/6 was integrated but then reverted as issues were identified
after that with it as standalone patch.

I added it in this series to keep all the sh-sci
fixes in the same place. All these fixes are prerequisites for the
Renesas RZ/G3S SCI support.

Series was tested on the boards with the following device trees binaries:
- r8a7742-iwg21d-q7.dtb
- r8a7743-iwg20d-q7.dtb
- r8a7745-iwg22d-sodimm.dtb
- r8a77470-iwg23s-sbc.dtb
- r8a774a1-hihope-rzg2m-ex.dtb
- r8a774b1-hihope-rzg2n-ex.dtb
- r8a774e1-hihope-rzg2h-ex.dtb
- r9a07g043u11-smarc.dtb
- r9a07g044c2-smarc.dtb
- r9a07g044l2-smarc.dtb
- r9a07g054l2-smarc.dtb
- r9a08g045s33-smarc.dtb
- r9a08g045s33-smarc-pmod.dtb (not integrated in the latest kernel tree,
  but the device tree was posted at [3])

in the following scenarios:

1/ "earlycon keep_bootcon" were present in bootargs
2/ only "earlycon" was present in bootargs
3/ none of the "earlycon" or "earlycon keep_bootcon" were present in
   bootargs

1, 2, 3 were tested also with renesas_defconfig on
r9a08g045s33-smarc-pmod.dtb.

Please give it a try on your devices as well.

Thank you,
Claudiu Beznea

[1] https://lore.kernel.org/all/20241115134401.3893008-1-claudiu.beznea.uj@bp.renesas.com/
[2] https://lore.kernel.org/all/20241125115856.513642-1-claudiu.beznea.uj@bp.renesas.com/
[3] https://lore.kernel.org/all/20241115134401.3893008-9-claudiu.beznea.uj@bp.renesas.com/

Claudiu Beznea (6):
  serial: sh-sci: Check if TX data was written to device in .tx_empty()
  serial: sh-sci: Drop __initdata macro for port_cfg
  serial: sh-sci: Move runtime PM enable to sci_probe_single()
  serial: sh-sci: Do not probe the serial port if its slot in
    sci_ports[] is in use
  serial: sh-sci: Clean sci_ports[0] after at earlycon exit
  serial: sh-sci: Increment the runtime usage counter for the earlycon
    device

 drivers/tty/serial/sh-sci.c | 121 ++++++++++++++++++++++++++++++------
 1 file changed, 102 insertions(+), 19 deletions(-)

-- 
2.39.2

[PATCH RFT 1/6] serial: sh-sci: Check if TX data was written to device in .tx_empty()

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

On the Renesas RZ/G3S, when doing suspend to RAM, the uart_suspend_port()
is called. The uart_suspend_port() calls 3 times the
struct uart_port::ops::tx_empty() before shutting down the port.

According to the documentation, the struct uart_port::ops::tx_empty()
API tests whether the transmitter FIFO and shifter for the port is
empty.

The Renesas RZ/G3S SCIFA IP reports the number of data units stored in the
transmit FIFO through the FDR (FIFO Data Count Register). The data units
in the FIFOs are written in the shift register and transmitted from there.
The TEND bit in the Serial Status Register reports if the data was
transmitted from the shift register.

In the previous code, in the tx_empty() API implemented by the sh-sci
driver, it is considered that the TX is empty if the hardware reports the
TEND bit set and the number of data units in the FIFO is zero.

According to the HW manual, the TEND bit has the following meaning:

0: Transmission is in the waiting state or in progress.
1: Transmission is completed.

It has been noticed that when opening the serial device w/o using it and
then switch to a power saving mode, the tx_empty() call in the
uart_port_suspend() function fails, leading to the "Unable to drain
transmitter" message being printed on the console. This is because the
TEND=0 if nothing has been transmitted and the FIFOs are empty. As the
TEND=0 has double meaning (waiting state, in progress) we can't
determined the scenario described above.

Add a software workaround for this. This sets a variable if any data has
been sent on the serial console (when using PIO) or if the DMA callback has
been called (meaning something has been transmitted). In the tx_empty()
API the status of the DMA transaction is also checked and if it is
completed or in progress the code falls back in checking the hardware
registers instead of relying on the software variable.

Fixes: 73a19e4c0301 ("serial: sh-sci: Add DMA support.")
Cc: stable@vger.kernel.org
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
 drivers/tty/serial/sh-sci.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index df523c744423..924b803af440 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -157,6 +157,7 @@ struct sci_port {
 
 	bool has_rtscts;
 	bool autorts;
+	bool tx_occurred;
 };
 
 #define SCI_NPORTS CONFIG_SERIAL_SH_SCI_NR_UARTS
@@ -850,6 +851,7 @@ static void sci_transmit_chars(struct uart_port *port)
 {
 	struct tty_port *tport = &port->state->port;
 	unsigned int stopped = uart_tx_stopped(port);
+	struct sci_port *s = to_sci_port(port);
 	unsigned short status;
 	unsigned short ctrl;
 	int count;
@@ -885,6 +887,7 @@ static void sci_transmit_chars(struct uart_port *port)
 		}
 
 		sci_serial_out(port, SCxTDR, c);
+		s->tx_occurred = true;
 
 		port->icount.tx++;
 	} while (--count > 0);
@@ -1241,6 +1244,8 @@ static void sci_dma_tx_complete(void *arg)
 	if (kfifo_len(&tport->xmit_fifo) < WAKEUP_CHARS)
 		uart_write_wakeup(port);
 
+	s->tx_occurred = true;
+
 	if (!kfifo_is_empty(&tport->xmit_fifo)) {
 		s->cookie_tx = 0;
 		schedule_work(&s->work_tx);
@@ -1731,6 +1736,19 @@ static void sci_flush_buffer(struct uart_port *port)
 		s->cookie_tx = -EINVAL;
 	}
 }
+
+static void sci_dma_check_tx_occurred(struct sci_port *s)
+{
+	struct dma_tx_state state;
+	enum dma_status status;
+
+	if (!s->chan_tx)
+		return;
+
+	status = dmaengine_tx_status(s->chan_tx, s->cookie_tx, &state);
+	if (status == DMA_COMPLETE || status == DMA_IN_PROGRESS)
+		s->tx_occurred = true;
+}
 #else /* !CONFIG_SERIAL_SH_SCI_DMA */
 static inline void sci_request_dma(struct uart_port *port)
 {
@@ -1740,6 +1758,10 @@ static inline void sci_free_dma(struct uart_port *port)
 {
 }
 
+static void sci_dma_check_tx_occurred(struct sci_port *s)
+{
+}
+
 #define sci_flush_buffer	NULL
 #endif /* !CONFIG_SERIAL_SH_SCI_DMA */
 
@@ -2076,6 +2098,12 @@ static unsigned int sci_tx_empty(struct uart_port *port)
 {
 	unsigned short status = sci_serial_in(port, SCxSR);
 	unsigned short in_tx_fifo = sci_txfill(port);
+	struct sci_port *s = to_sci_port(port);
+
+	sci_dma_check_tx_occurred(s);
+
+	if (!s->tx_occurred)
+		return TIOCSER_TEMT;
 
 	return (status & SCxSR_TEND(port)) && !in_tx_fifo ? TIOCSER_TEMT : 0;
 }
@@ -2247,6 +2275,7 @@ static int sci_startup(struct uart_port *port)
 
 	dev_dbg(port->dev, "%s(%d)\n", __func__, port->line);
 
+	s->tx_occurred = false;
 	sci_request_dma(port);
 
 	ret = sci_request_irq(s);
-- 
2.39.2

[PATCH RFT 2/6] serial: sh-sci: Drop __initdata macro for port_cfg

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

The port_cfg object is used by serial_console_write(), which serves as
the write function for the earlycon device. Marking port_cfg as __initdata
causes it to be freed after kernel initialization, resulting in earlycon
becoming unavailable thereafter. Remove the __initdata macro from port_cfg
to resolve this issue.

Fixes: dd076cffb8cd ("serial: sh-sci: Fix init data attribute for struct 'port_cfg'")
Cc: stable@vger.kernel.org
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
 drivers/tty/serial/sh-sci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 924b803af440..4f5da3254420 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -3562,7 +3562,7 @@ sh_early_platform_init_buffer("earlyprintk", &sci_driver,
 			   early_serial_buf, ARRAY_SIZE(early_serial_buf));
 #endif
 #ifdef CONFIG_SERIAL_SH_SCI_EARLYCON
-static struct plat_sci_port port_cfg __initdata;
+static struct plat_sci_port port_cfg;
 
 static int __init early_console_setup(struct earlycon_device *device,
 				      int type)
-- 
2.39.2

[PATCH RFT 3/6] serial: sh-sci: Move runtime PM enable to sci_probe_single()

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Relocate the runtime PM enable operation to sci_probe_single(). This change
prepares the codebase for upcoming fixes.

While at it, replace the existing logic with a direct call to
devm_pm_runtime_enable() and remove sci_cleanup_single(). The
devm_pm_runtime_enable() function automatically handles disabling runtime
PM during driver removal.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
 drivers/tty/serial/sh-sci.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 4f5da3254420..373195995d3b 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -3056,10 +3056,6 @@ static int sci_init_single(struct platform_device *dev,
 		ret = sci_init_clocks(sci_port, &dev->dev);
 		if (ret < 0)
 			return ret;
-
-		port->dev = &dev->dev;
-
-		pm_runtime_enable(&dev->dev);
 	}
 
 	port->type		= p->type;
@@ -3086,11 +3082,6 @@ static int sci_init_single(struct platform_device *dev,
 	return 0;
 }
 
-static void sci_cleanup_single(struct sci_port *port)
-{
-	pm_runtime_disable(port->port.dev);
-}
-
 #if defined(CONFIG_SERIAL_SH_SCI_CONSOLE) || \
     defined(CONFIG_SERIAL_SH_SCI_EARLYCON)
 static void serial_console_putchar(struct uart_port *port, unsigned char ch)
@@ -3260,8 +3251,6 @@ static void sci_remove(struct platform_device *dev)
 	sci_ports_in_use &= ~BIT(port->port.line);
 	uart_remove_one_port(&sci_uart_driver, &port->port);
 
-	sci_cleanup_single(port);
-
 	if (port->port.fifosize > 1)
 		device_remove_file(&dev->dev, &dev_attr_rx_fifo_trigger);
 	if (type == PORT_SCIFA || type == PORT_SCIFB || type == PORT_HSCIF)
@@ -3425,6 +3414,11 @@ static int sci_probe_single(struct platform_device *dev,
 	if (ret)
 		return ret;
 
+	sciport->port.dev = &dev->dev;
+	ret = devm_pm_runtime_enable(&dev->dev);
+	if (ret)
+		return ret;
+
 	sciport->gpios = mctrl_gpio_init(&sciport->port, 0);
 	if (IS_ERR(sciport->gpios))
 		return PTR_ERR(sciport->gpios);
@@ -3440,7 +3434,6 @@ static int sci_probe_single(struct platform_device *dev,
 
 	ret = uart_add_one_port(&sci_uart_driver, &sciport->port);
 	if (ret) {
-		sci_cleanup_single(sciport);
 		return ret;
 	}
 
-- 
2.39.2

[PATCH RFT 4/6] serial: sh-sci: Do not probe the serial port if its slot in sci_ports[] is in use

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

In the sh-sci driver, sci_ports[0] is used by earlycon. If the earlycon is
still active when sci_probe() is called and the new serial port is supposed
to map to sci_ports[0], return -EBUSY to prevent breaking the earlycon.

This situation should occurs in debug scenarios, and users should be
aware of the potential conflict.

Fixes: 0b0cced19ab1 ("serial: sh-sci: Add CONFIG_SERIAL_EARLYCON support")
Cc: stable@vger.kernel.org
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
 drivers/tty/serial/sh-sci.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 373195995d3b..e12fbc71082a 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -158,6 +158,7 @@ struct sci_port {
 	bool has_rtscts;
 	bool autorts;
 	bool tx_occurred;
+	bool earlycon;
 };
 
 #define SCI_NPORTS CONFIG_SERIAL_SH_SCI_NR_UARTS
@@ -3443,6 +3444,7 @@ static int sci_probe_single(struct platform_device *dev,
 static int sci_probe(struct platform_device *dev)
 {
 	struct plat_sci_port *p;
+	struct resource *res;
 	struct sci_port *sp;
 	unsigned int dev_id;
 	int ret;
@@ -3472,6 +3474,26 @@ static int sci_probe(struct platform_device *dev)
 	}
 
 	sp = &sci_ports[dev_id];
+
+	/*
+	 * In case:
+	 * - the probed port alias is zero (as the one used by earlycon), and
+	 * - the earlycon is still active (e.g., "earlycon keep_bootcon" in
+	 *   bootargs)
+	 *
+	 * defer the probe of this serial. This is a debug scenario and the user
+	 * must be aware of it.
+	 *
+	 * Except when the probed port is the same as the earlycon port.
+	 */
+
+	res = platform_get_resource(dev, IORESOURCE_MEM, 0);
+	if (!res)
+		return -ENODEV;
+
+	if (sp->earlycon && res->start != sp->port.mapbase)
+		return dev_err_probe(&dev->dev, -EBUSY, "sci_port[0] is used by earlycon!\n");
+
 	platform_set_drvdata(dev, sp);
 
 	ret = sci_probe_single(dev, dev_id, p, sp);
@@ -3568,6 +3590,7 @@ static int __init early_console_setup(struct earlycon_device *device,
 	port_cfg.type = type;
 	sci_ports[0].cfg = &port_cfg;
 	sci_ports[0].params = sci_probe_regmap(&port_cfg);
+	sci_ports[0].earlycon = true;
 	port_cfg.scscr = sci_serial_in(&sci_ports[0].port, SCSCR);
 	sci_serial_out(&sci_ports[0].port, SCSCR,
 		       SCSCR_RE | SCSCR_TE | port_cfg.scscr);
-- 
2.39.2

[PATCH RFT 5/6] serial: sh-sci: Clean sci_ports[0] after at earlycon exit

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

The early_console_setup() function initializes sci_ports[0].port with an
object of type struct uart_port obtained from the struct earlycon_device
passed as an argument to early_console_setup().

Later, during serial port probing, the serial port used as earlycon
(e.g., port A) might be remapped to a different position in the sci_ports[]
array, and a different serial port (e.g., port B) might be assigned to slot
0. For example:

sci_ports[0] = port B
sci_ports[X] = port A

In this scenario, the new port mapped at index zero (port B) retains the
data associated with the earlycon configuration. Consequently, after the
Linux boot process, any access to the serial port now mapped to
sci_ports[0] (port B) will block the original earlycon port (port A).

To address this, introduce an early_console_exit() function to clean up
sci_ports[0] when earlycon is exited.

To prevent the cleanup of sci_ports[0] while the serial device is still
being used by earlycon, introduce the struct sci_port::probing flag and
account for it in early_console_exit().

Fixes: 0b0cced19ab1 ("serial: sh-sci: Add CONFIG_SERIAL_EARLYCON support")
Cc: stable@vger.kernel.org
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes since the integrated patch:
- adjust the commit message to address Geert comments at [1]
- Introduce the struct sci_port::probing flag to prevent the cleanup
  of sci_ports[0] while the serial device is still being used by earlycon


[1] https://lore.kernel.org/all/CAMuHMdX57_AEYC_6CbrJn-+B+ivU8oFiXR0FXF7Lrqv5dWZWYA@mail.gmail.com/

 drivers/tty/serial/sh-sci.c | 33 +++++++++++++++++++++++++++------
 1 file changed, 27 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index e12fbc71082a..f74eb68774ca 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -159,6 +159,7 @@ struct sci_port {
 	bool autorts;
 	bool tx_occurred;
 	bool earlycon;
+	bool probing;
 };
 
 #define SCI_NPORTS CONFIG_SERIAL_SH_SCI_NR_UARTS
@@ -3386,7 +3387,8 @@ static struct plat_sci_port *sci_parse_dt(struct platform_device *pdev,
 static int sci_probe_single(struct platform_device *dev,
 				      unsigned int index,
 				      struct plat_sci_port *p,
-				      struct sci_port *sciport)
+				      struct sci_port *sciport,
+				      struct resource *sci_res)
 {
 	int ret;
 
@@ -3433,12 +3435,15 @@ static int sci_probe_single(struct platform_device *dev,
 		sciport->port.flags |= UPF_HARD_FLOW;
 	}
 
-	ret = uart_add_one_port(&sci_uart_driver, &sciport->port);
-	if (ret) {
-		return ret;
+	if (sci_ports[0].earlycon && sci_ports[0].port.mapbase == sci_res->start) {
+		/*
+		 * Skip cleanup up the sci_port[0] in early_console_exit(), this
+		 * port is the same as the earlycon one.
+		 */
+		sci_ports[0].probing = true;
 	}
 
-	return 0;
+	return uart_add_one_port(&sci_uart_driver, &sciport->port);
 }
 
 static int sci_probe(struct platform_device *dev)
@@ -3496,7 +3501,7 @@ static int sci_probe(struct platform_device *dev)
 
 	platform_set_drvdata(dev, sp);
 
-	ret = sci_probe_single(dev, dev_id, p, sp);
+	ret = sci_probe_single(dev, dev_id, p, sp, res);
 	if (ret)
 		return ret;
 
@@ -3579,6 +3584,20 @@ sh_early_platform_init_buffer("earlyprintk", &sci_driver,
 #ifdef CONFIG_SERIAL_SH_SCI_EARLYCON
 static struct plat_sci_port port_cfg;
 
+static int early_console_exit(struct console *co)
+{
+	struct sci_port *sci_port = &sci_ports[0];
+
+	/*
+	 * Clean the slot used by earlycon. A new SCI device might
+	 * map to this slot.
+	 */
+	if (sci_port->earlycon && !sci_port->probing)
+		memset(sci_port, 0, sizeof(*sci_port));
+
+	return 0;
+}
+
 static int __init early_console_setup(struct earlycon_device *device,
 				      int type)
 {
@@ -3596,6 +3615,8 @@ static int __init early_console_setup(struct earlycon_device *device,
 		       SCSCR_RE | SCSCR_TE | port_cfg.scscr);
 
 	device->con->write = serial_console_write;
+	device->con->exit = early_console_exit;
+
 	return 0;
 }
 static int __init sci_early_console_setup(struct earlycon_device *device,
-- 
2.39.2

[PATCH RFT 6/6] serial: sh-sci: Increment the runtime usage counter for the earlycon device

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

In the sh-sci driver, serial ports are mapped to the sci_ports[] array,
with earlycon mapped at index zero.

The uart_add_one_port() function eventually calls __device_attach(),
which, in turn, calls pm_request_idle(). The identified code path is as
follows:

uart_add_one_port() ->
  serial_ctrl_register_port() ->
    serial_core_register_port() ->
      serial_core_port_device_add() ->
        serial_base_port_add() ->
	  device_add() ->
	    bus_probe_device() ->
	      device_initial_probe() ->
	        __device_attach() ->
		  // ...
		  if (dev->p->dead) {
		    // ...
		  } else if (dev->driver) {
		    // ...
		  } else {
		    // ...
		    pm_request_idle(dev);
		    // ...
		  }

The earlycon device clocks are enabled by the bootloader. However, the
pm_request_idle() call in __device_attach() disables the SCI port clocks
while earlycon is still active.

The earlycon write function, serial_console_write(), calls
sci_poll_put_char() via serial_console_putchar(). If the SCI port clocks
are disabled, writing to earlycon may sometimes cause the SR.TDFE bit to
remain unset indefinitely, causing the while loop in sci_poll_put_char()
to never exit. On single-core SoCs, this can result in the system being
blocked during boot when this issue occurs.

To resolve this, increment the runtime PM usage counter for the earlycon
SCI device before registering the UART port.

Fixes: 0b0cced19ab1 ("serial: sh-sci: Add CONFIG_SERIAL_EARLYCON support")
Cc: stable@vger.kernel.org
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
 drivers/tty/serial/sh-sci.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index f74eb68774ca..6acdc8588d2d 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -3435,7 +3435,24 @@ static int sci_probe_single(struct platform_device *dev,
 		sciport->port.flags |= UPF_HARD_FLOW;
 	}
 
+	/*
+	 * In case:
+	 * - this is the earlycon port (mapped on index 0 in sci_ports[]) and
+	 * - it now maps to an alias other than zero and
+	 * - the earlycon is still alive (e.g., "earlycon keep_bootcon" is
+	 *   available in bootargs)
+	 *
+	 * we need to avoid disabling clocks and PM domains through the runtime
+	 * PM APIs called in __device_attach(). For this, increment the runtime
+	 * PM reference counter (the clocks and PM domains were already enabled
+	 * by the bootloader). Otherwise the earlycon may access the HW when it
+	 * has no clocks enabled leading to failures (infinite loop in
+	 * sci_poll_put_char()).
+	 */
+
 	if (sci_ports[0].earlycon && sci_ports[0].port.mapbase == sci_res->start) {
+		pm_runtime_get_noresume(&dev->dev);
+
 		/*
 		 * Skip cleanup up the sci_port[0] in early_console_exit(), this
 		 * port is the same as the earlycon one.
-- 
2.39.2

Re: [PATCH RFT 0/6] serial: sh-sci: Fixes for earlycon and keep_bootcon

[Wolfram Sang]

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

Hi Claudiu,

> in the following scenarios:
> 
> 1/ "earlycon keep_bootcon" were present in bootargs
> 2/ only "earlycon" was present in bootargs
> 3/ none of the "earlycon" or "earlycon keep_bootcon" were present in
>    bootargs
...
> Please give it a try on your devices as well.

Will happily do so. Is there something to look for? Except for "it
works"?

Happy hacking,

   Wolfram


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

Re: [PATCH RFT 0/6] serial: sh-sci: Fixes for earlycon and keep_bootcon

[Claudiu Beznea]

Hi, Wolfram,

On 04.12.2024 23:38, Wolfram Sang wrote:
> Hi Claudiu,
> 
>> in the following scenarios:
>>
>> 1/ "earlycon keep_bootcon" were present in bootargs
>> 2/ only "earlycon" was present in bootargs
>> 3/ none of the "earlycon" or "earlycon keep_bootcon" were present in
>>    bootargs
> ...
>> Please give it a try on your devices as well.
> 
> Will happily do so. Is there something to look for? Except for "it
> works"?

As this code touches the earlycon functionality, of interest are the 3
cases highlighted above:

1/ "earlycon keep_bootcon" are both present in bootargs
2/ only "earlycon" is present in bootargs
3/ none of the "earlycon" or "earlycon keep_bootcon" are present in
   bootargs

One other thing, that I was currently able to test only on RZ/G3S, is to
see how it behaves when the debug serial is described in DT with an alias
other than zero. E.g., on [1] the debug serial alias on RZ/G3S was changed
from 0 to 3. With the new alias (3) there were issues that I've tried to
fix with this series.

Thank you for checking it,
Claudiu

[1]
https://lore.kernel.org/all/20241115134401.3893008-6-claudiu.beznea.uj@bp.renesas.com/

> 
> Happy hacking,
> 
>    Wolfram
> 

Re: [PATCH RFT 0/6] serial: sh-sci: Fixes for earlycon and keep_bootcon

[Claudiu Beznea]

Hi, Wolfram,

On 05.12.2024 10:39, Claudiu Beznea wrote:
> Hi, Wolfram,
> 
> On 04.12.2024 23:38, Wolfram Sang wrote:
>> Hi Claudiu,
>>
>>> in the following scenarios:
>>>
>>> 1/ "earlycon keep_bootcon" were present in bootargs
>>> 2/ only "earlycon" was present in bootargs
>>> 3/ none of the "earlycon" or "earlycon keep_bootcon" were present in
>>>    bootargs
>> ...
>>> Please give it a try on your devices as well.
>>
>> Will happily do so. Is there something to look for? Except for "it
>> works"?

Sorry, I noticed I missed to provide a clear answer your question: if boot
works for this scenarios we should be OK.

> 
> As this code touches the earlycon functionality, of interest are the 3
> cases highlighted above:
> 
> 1/ "earlycon keep_bootcon" are both present in bootargs
> 2/ only "earlycon" is present in bootargs
> 3/ none of the "earlycon" or "earlycon keep_bootcon" are present in
>    bootargs
> 
> One other thing, that I was currently able to test only on RZ/G3S, is to
> see how it behaves when the debug serial is described in DT with an alias
> other than zero. E.g., on [1] the debug serial alias on RZ/G3S was changed
> from 0 to 3. With the new alias (3) there were issues that I've tried to
> fix with this series.

If you can also check:
- it boots in this case and
- the serial device with alias zero and the debug serial are both working
  (tx, rx are working) after boot

then we can declare it OK as well.

Thank you,
Claudiu

> 
> Thank you for checking it,
> Claudiu
> 
> [1]
> https://lore.kernel.org/all/20241115134401.3893008-6-claudiu.beznea.uj@bp.renesas.com/
> 
>>
>> Happy hacking,
>>
>>    Wolfram
>>

Re: [PATCH RFT 1/6] serial: sh-sci: Check if TX data was written to device in .tx_empty()

[Geert Uytterhoeven]

Hi Claudiu,

On Wed, Dec 4, 2024 at 4:58 PM Claudiu <claudiu.beznea@tuxon.dev> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> On the Renesas RZ/G3S, when doing suspend to RAM, the uart_suspend_port()
> is called. The uart_suspend_port() calls 3 times the
> struct uart_port::ops::tx_empty() before shutting down the port.
>
> According to the documentation, the struct uart_port::ops::tx_empty()
> API tests whether the transmitter FIFO and shifter for the port is
> empty.
>
> The Renesas RZ/G3S SCIFA IP reports the number of data units stored in the
> transmit FIFO through the FDR (FIFO Data Count Register). The data units
> in the FIFOs are written in the shift register and transmitted from there.
> The TEND bit in the Serial Status Register reports if the data was
> transmitted from the shift register.
>
> In the previous code, in the tx_empty() API implemented by the sh-sci
> driver, it is considered that the TX is empty if the hardware reports the
> TEND bit set and the number of data units in the FIFO is zero.
>
> According to the HW manual, the TEND bit has the following meaning:
>
> 0: Transmission is in the waiting state or in progress.
> 1: Transmission is completed.
>
> It has been noticed that when opening the serial device w/o using it and
> then switch to a power saving mode, the tx_empty() call in the
> uart_port_suspend() function fails, leading to the "Unable to drain
> transmitter" message being printed on the console. This is because the
> TEND=0 if nothing has been transmitted and the FIFOs are empty. As the
> TEND=0 has double meaning (waiting state, in progress) we can't
> determined the scenario described above.
>
> Add a software workaround for this. This sets a variable if any data has
> been sent on the serial console (when using PIO) or if the DMA callback has
> been called (meaning something has been transmitted). In the tx_empty()
> API the status of the DMA transaction is also checked and if it is
> completed or in progress the code falls back in checking the hardware
> registers instead of relying on the software variable.
>
> Fixes: 73a19e4c0301 ("serial: sh-sci: Add DMA support.")
> Cc: stable@vger.kernel.org
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Thanks for your patch, which is now commit 7cc0e0a43a910524 ("serial:
sh-sci: Check if TX data was written to device in .tx_empty()") in
v6.13-rc3.

> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -885,6 +887,7 @@ static void sci_transmit_chars(struct uart_port *port)
>                 }
>
>                 sci_serial_out(port, SCxTDR, c);
> +               s->tx_occurred = true;

And you cannot use the existing port->icount.tx below, as that is not
reset to zero on sci_startup(), right?

>
>                 port->icount.tx++;

Which brings me to the real reason for replying to this patch:
apparently port->icount.tx is updated only for PIO, not for DMA...

>         } while (--count > 0);

> @@ -2247,6 +2275,7 @@ static int sci_startup(struct uart_port *port)
>
>         dev_dbg(port->dev, "%s(%d)\n", __func__, port->line);
>
> +       s->tx_occurred = false;
>         sci_request_dma(port);
>
>         ret = sci_request_irq(s);

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

Re: [PATCH RFT 2/6] serial: sh-sci: Drop __initdata macro for port_cfg

[Geert Uytterhoeven]

Hi Claudiu,

On Wed, Dec 4, 2024 at 4:58 PM Claudiu <claudiu.beznea@tuxon.dev> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> The port_cfg object is used by serial_console_write(), which serves as
> the write function for the earlycon device. Marking port_cfg as __initdata
> causes it to be freed after kernel initialization, resulting in earlycon
> becoming unavailable thereafter. Remove the __initdata macro from port_cfg
> to resolve this issue.

Thanks for your patch!

> Fixes: dd076cffb8cd ("serial: sh-sci: Fix init data attribute for struct 'port_cfg'")

This commit is not the root cause, as it merely replaced __init by
__initdata.

Fixes: 0b0cced19ab15c9e ("serial: sh-sci: Add CONFIG_SERIAL_EARLYCON support")

For the patch contents:
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

Re: [PATCH RFT 3/6] serial: sh-sci: Move runtime PM enable to sci_probe_single()

[Geert Uytterhoeven]

Hi Claudiu,

On Wed, Dec 4, 2024 at 4:58 PM Claudiu <claudiu.beznea@tuxon.dev> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> Relocate the runtime PM enable operation to sci_probe_single(). This change
> prepares the codebase for upcoming fixes.
>
> While at it, replace the existing logic with a direct call to
> devm_pm_runtime_enable() and remove sci_cleanup_single(). The
> devm_pm_runtime_enable() function automatically handles disabling runtime
> PM during driver removal.
>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Thanks for your patch!

> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -3440,7 +3434,6 @@ static int sci_probe_single(struct platform_device *dev,
>
>         ret = uart_add_one_port(&sci_uart_driver, &sciport->port);
>         if (ret) {
> -               sci_cleanup_single(sciport);
>                 return ret;
>         }

Next line is:

    return 0;

so please just merge that into

    return uart_add_one_port(&sci_uart_driver, &sciport->port);

Actually [PATCH 5/6] makes that change, but there is no reason not
to do that here.

For the logical changes:
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

Re: [PATCH RFT 4/6] serial: sh-sci: Do not probe the serial port if its slot in sci_ports[] is in use

[Geert Uytterhoeven]

Hi Claudiu,

On Wed, Dec 4, 2024 at 4:58 PM Claudiu <claudiu.beznea@tuxon.dev> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> In the sh-sci driver, sci_ports[0] is used by earlycon. If the earlycon is
> still active when sci_probe() is called and the new serial port is supposed
> to map to sci_ports[0], return -EBUSY to prevent breaking the earlycon.
>
> This situation should occurs in debug scenarios, and users should be
> aware of the potential conflict.
>
> Fixes: 0b0cced19ab1 ("serial: sh-sci: Add CONFIG_SERIAL_EARLYCON support")
> Cc: stable@vger.kernel.org
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Thanks for your patch!

> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -158,6 +158,7 @@ struct sci_port {
>         bool has_rtscts;
>         bool autorts;
>         bool tx_occurred;
> +       bool earlycon;

This is only used in sci_ports[0], so it can be a single global flag,
instead of a flag embedded in each sci_port structure.

>  };
>
>  #define SCI_NPORTS CONFIG_SERIAL_SH_SCI_NR_UARTS
> @@ -3443,6 +3444,7 @@ static int sci_probe_single(struct platform_device *dev,
>  static int sci_probe(struct platform_device *dev)
>  {
>         struct plat_sci_port *p;
> +       struct resource *res;
>         struct sci_port *sp;
>         unsigned int dev_id;
>         int ret;
> @@ -3472,6 +3474,26 @@ static int sci_probe(struct platform_device *dev)
>         }
>
>         sp = &sci_ports[dev_id];
> +
> +       /*
> +        * In case:
> +        * - the probed port alias is zero (as the one used by earlycon), and
> +        * - the earlycon is still active (e.g., "earlycon keep_bootcon" in
> +        *   bootargs)

This is even true without "keep_bootcon", as nothing ever clears the
sci_port.earlycon flag once it is set.

> +        *
> +        * defer the probe of this serial. This is a debug scenario and the user
> +        * must be aware of it.
> +        *
> +        * Except when the probed port is the same as the earlycon port.
> +        */
> +
> +       res = platform_get_resource(dev, IORESOURCE_MEM, 0);
> +       if (!res)
> +               return -ENODEV;
> +
> +       if (sp->earlycon && res->start != sp->port.mapbase)
> +               return dev_err_probe(&dev->dev, -EBUSY, "sci_port[0] is used by earlycon!\n");
> +
>         platform_set_drvdata(dev, sp);
>
>         ret = sci_probe_single(dev, dev_id, p, sp);
> @@ -3568,6 +3590,7 @@ static int __init early_console_setup(struct earlycon_device *device,
>         port_cfg.type = type;
>         sci_ports[0].cfg = &port_cfg;
>         sci_ports[0].params = sci_probe_regmap(&port_cfg);
> +       sci_ports[0].earlycon = true;
>         port_cfg.scscr = sci_serial_in(&sci_ports[0].port, SCSCR);
>         sci_serial_out(&sci_ports[0].port, SCSCR,
>                        SCSCR_RE | SCSCR_TE | port_cfg.scscr);

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

Re: [PATCH RFT 5/6] serial: sh-sci: Clean sci_ports[0] after at earlycon exit

[Geert Uytterhoeven]

Hi Claudiu,

On Wed, Dec 4, 2024 at 4:58 PM Claudiu <claudiu.beznea@tuxon.dev> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> The early_console_setup() function initializes sci_ports[0].port with an
> object of type struct uart_port obtained from the struct earlycon_device
> passed as an argument to early_console_setup().
>
> Later, during serial port probing, the serial port used as earlycon
> (e.g., port A) might be remapped to a different position in the sci_ports[]
> array, and a different serial port (e.g., port B) might be assigned to slot
> 0. For example:
>
> sci_ports[0] = port B
> sci_ports[X] = port A
>
> In this scenario, the new port mapped at index zero (port B) retains the
> data associated with the earlycon configuration. Consequently, after the
> Linux boot process, any access to the serial port now mapped to
> sci_ports[0] (port B) will block the original earlycon port (port A).
>
> To address this, introduce an early_console_exit() function to clean up
> sci_ports[0] when earlycon is exited.
>
> To prevent the cleanup of sci_ports[0] while the serial device is still
> being used by earlycon, introduce the struct sci_port::probing flag and
> account for it in early_console_exit().
>
> Fixes: 0b0cced19ab1 ("serial: sh-sci: Add CONFIG_SERIAL_EARLYCON support")
> Cc: stable@vger.kernel.org
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---
>
> Changes since the integrated patch:
> - adjust the commit message to address Geert comments at [1]
> - Introduce the struct sci_port::probing flag to prevent the cleanup
>   of sci_ports[0] while the serial device is still being used by earlycon

Thanks for the update!

> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -159,6 +159,7 @@ struct sci_port {
>         bool autorts;
>         bool tx_occurred;
>         bool earlycon;
> +       bool probing;

This is only used in sci_ports[0], so it can be a single global flag,
instead of a flag embedded in each sci_port structure.

>  };
>
>  #define SCI_NPORTS CONFIG_SERIAL_SH_SCI_NR_UARTS
> @@ -3386,7 +3387,8 @@ static struct plat_sci_port *sci_parse_dt(struct platform_device *pdev,
>  static int sci_probe_single(struct platform_device *dev,
>                                       unsigned int index,
>                                       struct plat_sci_port *p,
> -                                     struct sci_port *sciport)
> +                                     struct sci_port *sciport,
> +                                     struct resource *sci_res)
>  {
>         int ret;
>
> @@ -3433,12 +3435,15 @@ static int sci_probe_single(struct platform_device *dev,
>                 sciport->port.flags |= UPF_HARD_FLOW;
>         }
>
> -       ret = uart_add_one_port(&sci_uart_driver, &sciport->port);
> -       if (ret) {
> -               return ret;
> +       if (sci_ports[0].earlycon && sci_ports[0].port.mapbase == sci_res->start) {
> +               /*
> +                * Skip cleanup up the sci_port[0] in early_console_exit(), this

Double up

> +                * port is the same as the earlycon one.
> +                */
> +               sci_ports[0].probing = true;
>         }
>
> -       return 0;
> +       return uart_add_one_port(&sci_uart_driver, &sciport->port);
>  }
>
>  static int sci_probe(struct platform_device *dev)
> @@ -3496,7 +3501,7 @@ static int sci_probe(struct platform_device *dev)
>
>         platform_set_drvdata(dev, sp);
>
> -       ret = sci_probe_single(dev, dev_id, p, sp);
> +       ret = sci_probe_single(dev, dev_id, p, sp, res);
>         if (ret)
>                 return ret;
>
> @@ -3579,6 +3584,20 @@ sh_early_platform_init_buffer("earlyprintk", &sci_driver,
>  #ifdef CONFIG_SERIAL_SH_SCI_EARLYCON
>  static struct plat_sci_port port_cfg;
>
> +static int early_console_exit(struct console *co)
> +{
> +       struct sci_port *sci_port = &sci_ports[0];
> +
> +       /*
> +        * Clean the slot used by earlycon. A new SCI device might
> +        * map to this slot.
> +        */
> +       if (sci_port->earlycon && !sci_port->probing)
> +               memset(sci_port, 0, sizeof(*sci_port));

Aha, so this clears sci_port.earlycon, too (cfr. my comment on
PATCH 4/6). Still, I don't think this is sufficient: shouldn't
sci_port.earlycon be cleared unconditionally?

> +
> +       return 0;
> +}
> +
>  static int __init early_console_setup(struct earlycon_device *device,
>                                       int type)
>  {
> @@ -3596,6 +3615,8 @@ static int __init early_console_setup(struct earlycon_device *device,
>                        SCSCR_RE | SCSCR_TE | port_cfg.scscr);
>
>         device->con->write = serial_console_write;
> +       device->con->exit = early_console_exit;
> +
>         return 0;
>  }
>  static int __init sci_early_console_setup(struct earlycon_device *device,

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

Re: [PATCH RFT 6/6] serial: sh-sci: Increment the runtime usage counter for the earlycon device

[Geert Uytterhoeven]

Hi Claudiu,

On Wed, Dec 4, 2024 at 4:58 PM Claudiu <claudiu.beznea@tuxon.dev> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> In the sh-sci driver, serial ports are mapped to the sci_ports[] array,
> with earlycon mapped at index zero.
>
> The uart_add_one_port() function eventually calls __device_attach(),
> which, in turn, calls pm_request_idle(). The identified code path is as
> follows:
>
> uart_add_one_port() ->
>   serial_ctrl_register_port() ->
>     serial_core_register_port() ->
>       serial_core_port_device_add() ->
>         serial_base_port_add() ->
>           device_add() ->
>             bus_probe_device() ->
>               device_initial_probe() ->
>                 __device_attach() ->
>                   // ...
>                   if (dev->p->dead) {
>                     // ...
>                   } else if (dev->driver) {
>                     // ...
>                   } else {
>                     // ...
>                     pm_request_idle(dev);
>                     // ...
>                   }
>
> The earlycon device clocks are enabled by the bootloader. However, the
> pm_request_idle() call in __device_attach() disables the SCI port clocks
> while earlycon is still active.
>
> The earlycon write function, serial_console_write(), calls
> sci_poll_put_char() via serial_console_putchar(). If the SCI port clocks
> are disabled, writing to earlycon may sometimes cause the SR.TDFE bit to
> remain unset indefinitely, causing the while loop in sci_poll_put_char()
> to never exit. On single-core SoCs, this can result in the system being
> blocked during boot when this issue occurs.
>
> To resolve this, increment the runtime PM usage counter for the earlycon
> SCI device before registering the UART port.
>
> Fixes: 0b0cced19ab1 ("serial: sh-sci: Add CONFIG_SERIAL_EARLYCON support")
> Cc: stable@vger.kernel.org
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Thanks for your patch!

> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -3435,7 +3435,24 @@ static int sci_probe_single(struct platform_device *dev,
>                 sciport->port.flags |= UPF_HARD_FLOW;
>         }
>
> +       /*
> +        * In case:
> +        * - this is the earlycon port (mapped on index 0 in sci_ports[]) and
> +        * - it now maps to an alias other than zero and
> +        * - the earlycon is still alive (e.g., "earlycon keep_bootcon" is
> +        *   available in bootargs)
> +        *
> +        * we need to avoid disabling clocks and PM domains through the runtime
> +        * PM APIs called in __device_attach(). For this, increment the runtime
> +        * PM reference counter (the clocks and PM domains were already enabled
> +        * by the bootloader). Otherwise the earlycon may access the HW when it
> +        * has no clocks enabled leading to failures (infinite loop in
> +        * sci_poll_put_char()).
> +        */
> +
>         if (sci_ports[0].earlycon && sci_ports[0].port.mapbase == sci_res->start) {

Now there are two tests for mapbase: here and in sci_probe()...

> +               pm_runtime_get_noresume(&dev->dev);
> +
>                 /*
>                  * Skip cleanup up the sci_port[0] in early_console_exit(), this
>                  * port is the same as the earlycon one.

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

Re: [PATCH RFT 0/6] serial: sh-sci: Fixes for earlycon and keep_bootcon

[Geert Uytterhoeven]

Hi Claudiu,

On Thu, Dec 5, 2024 at 9:39 AM Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
> On 04.12.2024 23:38, Wolfram Sang wrote:
> >> in the following scenarios:
> >>
> >> 1/ "earlycon keep_bootcon" were present in bootargs
> >> 2/ only "earlycon" was present in bootargs
> >> 3/ none of the "earlycon" or "earlycon keep_bootcon" were present in
> >>    bootargs
> > ...
> >> Please give it a try on your devices as well.
> >
> > Will happily do so. Is there something to look for? Except for "it
> > works"?
>
> As this code touches the earlycon functionality, of interest are the 3
> cases highlighted above:
>
> 1/ "earlycon keep_bootcon" are both present in bootargs
> 2/ only "earlycon" is present in bootargs
> 3/ none of the "earlycon" or "earlycon keep_bootcon" are present in
>    bootargs
>
> One other thing, that I was currently able to test only on RZ/G3S, is to
> see how it behaves when the debug serial is described in DT with an alias
> other than zero. E.g., on [1] the debug serial alias on RZ/G3S was changed
> from 0 to 3. With the new alias (3) there were issues that I've tried to
> fix with this series.

I gave this a try on Koelsch, which has two easily-accessible usb-serial
ports, for all three cases above.  Originally, I had CONFIG_VT_CONSOLE=y
(tty0 takes over from earlycon rather early), but I had to disable
that to exercise all code paths (ttySC0 takes over much later).

  A. CONFIG_VT_CONSOLE=y: OK
  B. CONFIG_VT_CONSOLE=y earlycon: OK
       early_console_setup: mapbase 0x00000000e6e60000
       earlycon: scif0 at MMIO 0x00000000e6e60000 (options '115200n8')
       printk: legacy bootconsole [scif0] enabled
       printk: legacy console [tty0] enabled
       printk: legacy bootconsole [scif0] disabled
       early_console_exit: Clearing sci_ports[0]
  C. CONFIG_VT_CONSOLE=n earlycon: OK
       early_console_setup: mapbase 0x00000000e6e60000
       earlycon: scif0 at MMIO 0x00000000e6e60000 (options '115200n8')
       printk: legacy bootconsole [scif0] enabled
       printk: legacy console [ttySC0] enabled
       printk: legacy bootconsole [scif0] disabled
       early_console_exit: Not clearing sci_ports[0]
  D. CONFIG_VT_CONSOLE=y earlycon keep_bootcon: OK
       early_console_setup: mapbase 0x00000000e6e60000
       earlycon: scif0 at MMIO 0x00000000e6e60000 (options '115200n8')
       printk: legacy bootconsole [scif0] enabled
       printk: legacy console [tty0] enabled

So all good, but note that these cases worked fine without your
series, too.

The real troublesome cases involve using earlycon on a different
serial port than serial0.  As I don't have any Renesas boards where
chosen/stdout-path does not use serial0, I tried exchanging the serial0
and serial1 DT aliases, and updating chosen/stdout-path accordingly.

  E. CONFIG_VT_CONSOLE=y: OK
  F. CONFIG_VT_CONSOLE=y earlycon: OK
       early_console_setup: mapbase 0x00000000e6e60000
       earlycon: scif0 at MMIO 0x00000000e6e60000 (options '115200n8')
       printk: legacy bootconsole [scif0] enabled
       printk: legacy console [tty0] enabled
       printk: legacy bootconsole [scif0] disabled
       early_console_exit: Clearing sci_ports[0]
  G. CONFIG_VT_CONSOLE=y earlycon keep_bootcon: SCIF1 missing
       early_console_setup: mapbase 0x00000000e6e60000
       earlycon: scif0 at MMIO 0x00000000e6e60000 (options '115200n8')
       printk: legacy bootconsole [scif0] enabled
       printk: legacy console [tty0] enabled
       sh-sci e6e68000.serial: error -EBUSY: sci_port[0] is used by earlycon!
  H. CONFIG_VT_CONSOLE=n earlycon: SCIF1 missing
       early_console_setup: mapbase 0x00000000e6e60000
       earlycon: scif0 at MMIO 0x00000000e6e60000 (options '115200n8')
       printk: legacy bootconsole [scif0] enabled
       printk: legacy console [ttySC1] enabled
       printk: legacy bootconsole [scif0] disabled
       early_console_exit: Not clearing sci_ports[0]
       sh-sci e6e68000.serial: error -EBUSY: sci_port[0] is used by earlycon!

Case G gives a missing SCIF1, because sci_port[0] is still
used for earlycon, as expected.
Case H also gives a missing SCIF1, but should succeed IMHO, as earlycon
is no longer active.  I think early_console_exit() should clear the
earlycon flag regardless.

Note that before your series, cases E-F worked too, but cases G-H gave
an initialized but broken SCIF1 instead.

Now, can we improve?
  - Can we use a proper id instead of zero for earlycon, e.g.
    sci_probe_earlyprintk() does fill in early_serial_console.index?
  - Alternatively, can we use a separate sci_port structure instead
    of abusing sci_ports[0]?

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

Re: [PATCH RFT 1/6] serial: sh-sci: Check if TX data was written to device in .tx_empty()

[Claudiu Beznea]

Hi, Geert,

On 19.12.2024 11:46, Geert Uytterhoeven wrote:
> Hi Claudiu,
> 
> On Wed, Dec 4, 2024 at 4:58 PM Claudiu <claudiu.beznea@tuxon.dev> wrote:
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> On the Renesas RZ/G3S, when doing suspend to RAM, the uart_suspend_port()
>> is called. The uart_suspend_port() calls 3 times the
>> struct uart_port::ops::tx_empty() before shutting down the port.
>>
>> According to the documentation, the struct uart_port::ops::tx_empty()
>> API tests whether the transmitter FIFO and shifter for the port is
>> empty.
>>
>> The Renesas RZ/G3S SCIFA IP reports the number of data units stored in the
>> transmit FIFO through the FDR (FIFO Data Count Register). The data units
>> in the FIFOs are written in the shift register and transmitted from there.
>> The TEND bit in the Serial Status Register reports if the data was
>> transmitted from the shift register.
>>
>> In the previous code, in the tx_empty() API implemented by the sh-sci
>> driver, it is considered that the TX is empty if the hardware reports the
>> TEND bit set and the number of data units in the FIFO is zero.
>>
>> According to the HW manual, the TEND bit has the following meaning:
>>
>> 0: Transmission is in the waiting state or in progress.
>> 1: Transmission is completed.
>>
>> It has been noticed that when opening the serial device w/o using it and
>> then switch to a power saving mode, the tx_empty() call in the
>> uart_port_suspend() function fails, leading to the "Unable to drain
>> transmitter" message being printed on the console. This is because the
>> TEND=0 if nothing has been transmitted and the FIFOs are empty. As the
>> TEND=0 has double meaning (waiting state, in progress) we can't
>> determined the scenario described above.
>>
>> Add a software workaround for this. This sets a variable if any data has
>> been sent on the serial console (when using PIO) or if the DMA callback has
>> been called (meaning something has been transmitted). In the tx_empty()
>> API the status of the DMA transaction is also checked and if it is
>> completed or in progress the code falls back in checking the hardware
>> registers instead of relying on the software variable.
>>
>> Fixes: 73a19e4c0301 ("serial: sh-sci: Add DMA support.")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> Thanks for your patch, which is now commit 7cc0e0a43a910524 ("serial:
> sh-sci: Check if TX data was written to device in .tx_empty()") in
> v6.13-rc3.
> 
>> --- a/drivers/tty/serial/sh-sci.c
>> +++ b/drivers/tty/serial/sh-sci.c
>> @@ -885,6 +887,7 @@ static void sci_transmit_chars(struct uart_port *port)
>>                 }
>>
>>                 sci_serial_out(port, SCxTDR, c);
>> +               s->tx_occurred = true;
> And you cannot use the existing port->icount.tx below, as that is not
> reset to zero on sci_startup(), right?

I missed that the driver is incrementing the port->icount.tx . I'm not sure
we can use it though, as it is not reset on sci_startup(), as you pointed out.

Thank you,
Claudiu

Re: [PATCH RFT 3/6] serial: sh-sci: Move runtime PM enable to sci_probe_single()

[Claudiu Beznea]

On 19.12.2024 12:18, Geert Uytterhoeven wrote:
> Hi Claudiu,
> 
> On Wed, Dec 4, 2024 at 4:58 PM Claudiu <claudiu.beznea@tuxon.dev> wrote:
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> Relocate the runtime PM enable operation to sci_probe_single(). This change
>> prepares the codebase for upcoming fixes.
>>
>> While at it, replace the existing logic with a direct call to
>> devm_pm_runtime_enable() and remove sci_cleanup_single(). The
>> devm_pm_runtime_enable() function automatically handles disabling runtime
>> PM during driver removal.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Thanks for your patch!
> 
>> --- a/drivers/tty/serial/sh-sci.c
>> +++ b/drivers/tty/serial/sh-sci.c
>> @@ -3440,7 +3434,6 @@ static int sci_probe_single(struct platform_device *dev,
>>
>>         ret = uart_add_one_port(&sci_uart_driver, &sciport->port);
>>         if (ret) {
>> -               sci_cleanup_single(sciport);
>>                 return ret;
>>         }
> 
> Next line is:
> 
>     return 0;
> 
> so please just merge that into
> 
>     return uart_add_one_port(&sci_uart_driver, &sciport->port);
> 

You're right with these.

> Actually [PATCH 5/6] makes that change, but there is no reason not
> to do that here.

I remember I chose to keep it like this as I had the impression that if I
format the patches as proposed by you the 5/6 will just revert what I will
have been done in this patch. But I think I was wrong.

Thank you,
Claudiu

> 
> For the logical changes:
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 

Re: [PATCH RFT 5/6] serial: sh-sci: Clean sci_ports[0] after at earlycon exit

[Claudiu Beznea]

Hi, Geert,

On 19.12.2024 16:26, Geert Uytterhoeven wrote:
> Hi Claudiu,
> 
> On Wed, Dec 4, 2024 at 4:58 PM Claudiu <claudiu.beznea@tuxon.dev> wrote:
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> The early_console_setup() function initializes sci_ports[0].port with an
>> object of type struct uart_port obtained from the struct earlycon_device
>> passed as an argument to early_console_setup().
>>
>> Later, during serial port probing, the serial port used as earlycon
>> (e.g., port A) might be remapped to a different position in the sci_ports[]
>> array, and a different serial port (e.g., port B) might be assigned to slot
>> 0. For example:
>>
>> sci_ports[0] = port B
>> sci_ports[X] = port A
>>
>> In this scenario, the new port mapped at index zero (port B) retains the
>> data associated with the earlycon configuration. Consequently, after the
>> Linux boot process, any access to the serial port now mapped to
>> sci_ports[0] (port B) will block the original earlycon port (port A).
>>
>> To address this, introduce an early_console_exit() function to clean up
>> sci_ports[0] when earlycon is exited.
>>
>> To prevent the cleanup of sci_ports[0] while the serial device is still
>> being used by earlycon, introduce the struct sci_port::probing flag and
>> account for it in early_console_exit().
>>
>> Fixes: 0b0cced19ab1 ("serial: sh-sci: Add CONFIG_SERIAL_EARLYCON support")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>> ---
>>
>> Changes since the integrated patch:
>> - adjust the commit message to address Geert comments at [1]
>> - Introduce the struct sci_port::probing flag to prevent the cleanup
>>   of sci_ports[0] while the serial device is still being used by earlycon
> 
> Thanks for the update!
> 
>> --- a/drivers/tty/serial/sh-sci.c
>> +++ b/drivers/tty/serial/sh-sci.c
>> @@ -159,6 +159,7 @@ struct sci_port {
>>         bool autorts;
>>         bool tx_occurred;
>>         bool earlycon;
>> +       bool probing;
> 
> This is only used in sci_ports[0], so it can be a single global flag,
> instead of a flag embedded in each sci_port structure.
> 
>>  };
>>
>>  #define SCI_NPORTS CONFIG_SERIAL_SH_SCI_NR_UARTS
>> @@ -3386,7 +3387,8 @@ static struct plat_sci_port *sci_parse_dt(struct platform_device *pdev,
>>  static int sci_probe_single(struct platform_device *dev,
>>                                       unsigned int index,
>>                                       struct plat_sci_port *p,
>> -                                     struct sci_port *sciport)
>> +                                     struct sci_port *sciport,
>> +                                     struct resource *sci_res)
>>  {
>>         int ret;
>>
>> @@ -3433,12 +3435,15 @@ static int sci_probe_single(struct platform_device *dev,
>>                 sciport->port.flags |= UPF_HARD_FLOW;
>>         }
>>
>> -       ret = uart_add_one_port(&sci_uart_driver, &sciport->port);
>> -       if (ret) {
>> -               return ret;
>> +       if (sci_ports[0].earlycon && sci_ports[0].port.mapbase == sci_res->start) {
>> +               /*
>> +                * Skip cleanup up the sci_port[0] in early_console_exit(), this
> 
> Double up
> 
>> +                * port is the same as the earlycon one.
>> +                */
>> +               sci_ports[0].probing = true;
>>         }
>>
>> -       return 0;
>> +       return uart_add_one_port(&sci_uart_driver, &sciport->port);
>>  }
>>
>>  static int sci_probe(struct platform_device *dev)
>> @@ -3496,7 +3501,7 @@ static int sci_probe(struct platform_device *dev)
>>
>>         platform_set_drvdata(dev, sp);
>>
>> -       ret = sci_probe_single(dev, dev_id, p, sp);
>> +       ret = sci_probe_single(dev, dev_id, p, sp, res);
>>         if (ret)
>>                 return ret;
>>
>> @@ -3579,6 +3584,20 @@ sh_early_platform_init_buffer("earlyprintk", &sci_driver,
>>  #ifdef CONFIG_SERIAL_SH_SCI_EARLYCON
>>  static struct plat_sci_port port_cfg;
>>
>> +static int early_console_exit(struct console *co)
>> +{
>> +       struct sci_port *sci_port = &sci_ports[0];
>> +
>> +       /*
>> +        * Clean the slot used by earlycon. A new SCI device might
>> +        * map to this slot.
>> +        */
>> +       if (sci_port->earlycon && !sci_port->probing)
>> +               memset(sci_port, 0, sizeof(*sci_port));
> 
> Aha, so this clears sci_port.earlycon, too (cfr. my comment on
> PATCH 4/6). Still, I don't think this is sufficient: shouldn't
> sci_port.earlycon be cleared unconditionally?

I remember I had failures with unconditional clear. I'll double check it
though and adjust accordingly, if needed.

Thank you,
Claudiu

> 
>> +
>> +       return 0;
>> +}
>> +
>>  static int __init early_console_setup(struct earlycon_device *device,
>>                                       int type)
>>  {
>> @@ -3596,6 +3615,8 @@ static int __init early_console_setup(struct earlycon_device *device,
>>                        SCSCR_RE | SCSCR_TE | port_cfg.scscr);
>>
>>         device->con->write = serial_console_write;
>> +       device->con->exit = early_console_exit;
>> +
>>         return 0;
>>  }
>>  static int __init sci_early_console_setup(struct earlycon_device *device,
> 
> 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

Re: [PATCH RFT 6/6] serial: sh-sci: Increment the runtime usage counter for the earlycon device

[Claudiu Beznea]

On 19.12.2024 16:30, Geert Uytterhoeven wrote:
> Hi Claudiu,
> 
> On Wed, Dec 4, 2024 at 4:58 PM Claudiu <claudiu.beznea@tuxon.dev> wrote:
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> In the sh-sci driver, serial ports are mapped to the sci_ports[] array,
>> with earlycon mapped at index zero.
>>
>> The uart_add_one_port() function eventually calls __device_attach(),
>> which, in turn, calls pm_request_idle(). The identified code path is as
>> follows:
>>
>> uart_add_one_port() ->
>>   serial_ctrl_register_port() ->
>>     serial_core_register_port() ->
>>       serial_core_port_device_add() ->
>>         serial_base_port_add() ->
>>           device_add() ->
>>             bus_probe_device() ->
>>               device_initial_probe() ->
>>                 __device_attach() ->
>>                   // ...
>>                   if (dev->p->dead) {
>>                     // ...
>>                   } else if (dev->driver) {
>>                     // ...
>>                   } else {
>>                     // ...
>>                     pm_request_idle(dev);
>>                     // ...
>>                   }
>>
>> The earlycon device clocks are enabled by the bootloader. However, the
>> pm_request_idle() call in __device_attach() disables the SCI port clocks
>> while earlycon is still active.
>>
>> The earlycon write function, serial_console_write(), calls
>> sci_poll_put_char() via serial_console_putchar(). If the SCI port clocks
>> are disabled, writing to earlycon may sometimes cause the SR.TDFE bit to
>> remain unset indefinitely, causing the while loop in sci_poll_put_char()
>> to never exit. On single-core SoCs, this can result in the system being
>> blocked during boot when this issue occurs.
>>
>> To resolve this, increment the runtime PM usage counter for the earlycon
>> SCI device before registering the UART port.
>>
>> Fixes: 0b0cced19ab1 ("serial: sh-sci: Add CONFIG_SERIAL_EARLYCON support")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Thanks for your patch!
> 
>> --- a/drivers/tty/serial/sh-sci.c
>> +++ b/drivers/tty/serial/sh-sci.c
>> @@ -3435,7 +3435,24 @@ static int sci_probe_single(struct platform_device *dev,
>>                 sciport->port.flags |= UPF_HARD_FLOW;
>>         }
>>
>> +       /*
>> +        * In case:
>> +        * - this is the earlycon port (mapped on index 0 in sci_ports[]) and
>> +        * - it now maps to an alias other than zero and
>> +        * - the earlycon is still alive (e.g., "earlycon keep_bootcon" is
>> +        *   available in bootargs)
>> +        *
>> +        * we need to avoid disabling clocks and PM domains through the runtime
>> +        * PM APIs called in __device_attach(). For this, increment the runtime
>> +        * PM reference counter (the clocks and PM domains were already enabled
>> +        * by the bootloader). Otherwise the earlycon may access the HW when it
>> +        * has no clocks enabled leading to failures (infinite loop in
>> +        * sci_poll_put_char()).
>> +        */
>> +
>>         if (sci_ports[0].earlycon && sci_ports[0].port.mapbase == sci_res->start) {
> 
> Now there are two tests for mapbase: here and in sci_probe()...

I'll adjust it!

Thank you for your review,
Claudiu

> 
>> +               pm_runtime_get_noresume(&dev->dev);
>> +
>>                 /*
>>                  * Skip cleanup up the sci_port[0] in early_console_exit(), this
>>                  * port is the same as the earlycon one.
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 

Re: [PATCH RFT 0/6] serial: sh-sci: Fixes for earlycon and keep_bootcon

[Claudiu Beznea]

Hi, Geert,

On 19.12.2024 17:11, Geert Uytterhoeven wrote:
> Hi Claudiu,
> 
> On Thu, Dec 5, 2024 at 9:39 AM Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
>> On 04.12.2024 23:38, Wolfram Sang wrote:
>>>> in the following scenarios:
>>>>
>>>> 1/ "earlycon keep_bootcon" were present in bootargs
>>>> 2/ only "earlycon" was present in bootargs
>>>> 3/ none of the "earlycon" or "earlycon keep_bootcon" were present in
>>>>    bootargs
>>> ...
>>>> Please give it a try on your devices as well.
>>>
>>> Will happily do so. Is there something to look for? Except for "it
>>> works"?
>>
>> As this code touches the earlycon functionality, of interest are the 3
>> cases highlighted above:
>>
>> 1/ "earlycon keep_bootcon" are both present in bootargs
>> 2/ only "earlycon" is present in bootargs
>> 3/ none of the "earlycon" or "earlycon keep_bootcon" are present in
>>    bootargs
>>
>> One other thing, that I was currently able to test only on RZ/G3S, is to
>> see how it behaves when the debug serial is described in DT with an alias
>> other than zero. E.g., on [1] the debug serial alias on RZ/G3S was changed
>> from 0 to 3. With the new alias (3) there were issues that I've tried to
>> fix with this series.
> 
> I gave this a try on Koelsch, which has two easily-accessible usb-serial
> ports, for all three cases above.  Originally, I had CONFIG_VT_CONSOLE=y
> (tty0 takes over from earlycon rather early), but I had to disable
> that to exercise all code paths (ttySC0 takes over much later).
> 
>   A. CONFIG_VT_CONSOLE=y: OK
>   B. CONFIG_VT_CONSOLE=y earlycon: OK
>        early_console_setup: mapbase 0x00000000e6e60000
>        earlycon: scif0 at MMIO 0x00000000e6e60000 (options '115200n8')
>        printk: legacy bootconsole [scif0] enabled
>        printk: legacy console [tty0] enabled
>        printk: legacy bootconsole [scif0] disabled
>        early_console_exit: Clearing sci_ports[0]
>   C. CONFIG_VT_CONSOLE=n earlycon: OK
>        early_console_setup: mapbase 0x00000000e6e60000
>        earlycon: scif0 at MMIO 0x00000000e6e60000 (options '115200n8')
>        printk: legacy bootconsole [scif0] enabled
>        printk: legacy console [ttySC0] enabled
>        printk: legacy bootconsole [scif0] disabled
>        early_console_exit: Not clearing sci_ports[0]
>   D. CONFIG_VT_CONSOLE=y earlycon keep_bootcon: OK
>        early_console_setup: mapbase 0x00000000e6e60000
>        earlycon: scif0 at MMIO 0x00000000e6e60000 (options '115200n8')
>        printk: legacy bootconsole [scif0] enabled
>        printk: legacy console [tty0] enabled
> 
> So all good, but note that these cases worked fine without your
> series, too.
> 
> The real troublesome cases involve using earlycon on a different
> serial port than serial0.  As I don't have any Renesas boards where
> chosen/stdout-path does not use serial0, I tried exchanging the serial0
> and serial1 DT aliases, and updating chosen/stdout-path accordingly.
> 
>   E. CONFIG_VT_CONSOLE=y: OK
>   F. CONFIG_VT_CONSOLE=y earlycon: OK
>        early_console_setup: mapbase 0x00000000e6e60000
>        earlycon: scif0 at MMIO 0x00000000e6e60000 (options '115200n8')
>        printk: legacy bootconsole [scif0] enabled
>        printk: legacy console [tty0] enabled
>        printk: legacy bootconsole [scif0] disabled
>        early_console_exit: Clearing sci_ports[0]
>   G. CONFIG_VT_CONSOLE=y earlycon keep_bootcon: SCIF1 missing
>        early_console_setup: mapbase 0x00000000e6e60000
>        earlycon: scif0 at MMIO 0x00000000e6e60000 (options '115200n8')
>        printk: legacy bootconsole [scif0] enabled
>        printk: legacy console [tty0] enabled
>        sh-sci e6e68000.serial: error -EBUSY: sci_port[0] is used by earlycon!
>   H. CONFIG_VT_CONSOLE=n earlycon: SCIF1 missing
>        early_console_setup: mapbase 0x00000000e6e60000
>        earlycon: scif0 at MMIO 0x00000000e6e60000 (options '115200n8')
>        printk: legacy bootconsole [scif0] enabled
>        printk: legacy console [ttySC1] enabled
>        printk: legacy bootconsole [scif0] disabled
>        early_console_exit: Not clearing sci_ports[0]
>        sh-sci e6e68000.serial: error -EBUSY: sci_port[0] is used by earlycon!
> 
> Case G gives a missing SCIF1, because sci_port[0] is still
> used for earlycon, as expected.
> Case H also gives a missing SCIF1, but should succeed IMHO, as earlycon
> is no longer active.  I think early_console_exit() should clear the
> earlycon flag regardless.

I'll double check it.

> 
> Note that before your series, cases E-F worked too, but cases G-H gave
> an initialized but broken SCIF1 instead.
> 
> Now, can we improve?
>   - Can we use a proper id instead of zero for earlycon, e.g.
>     sci_probe_earlyprintk() does fill in early_serial_console.index?

I looked into that but, as of my investigation, index zero is the one used
in the earlyprintk initialization process. sci_probe_earlyprintk() is
called from sci_probe(). I'll double checked it though, anyway.


>   - Alternatively, can we use a separate sci_port structure instead
>     of abusing sci_ports[0]?

I explored this too, but didn't manage to make it work.

Thank you for running all these tests,
Claudiu

> 
> 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

Re: [PATCH RFT 6/6] serial: sh-sci: Increment the runtime usage counter for the earlycon device

[Claudiu Beznea]

Hi, Geert,

On 19.12.2024 16:30, Geert Uytterhoeven wrote:
> Hi Claudiu,
> 
> On Wed, Dec 4, 2024 at 4:58 PM Claudiu <claudiu.beznea@tuxon.dev> wrote:
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> In the sh-sci driver, serial ports are mapped to the sci_ports[] array,
>> with earlycon mapped at index zero.
>>
>> The uart_add_one_port() function eventually calls __device_attach(),
>> which, in turn, calls pm_request_idle(). The identified code path is as
>> follows:
>>
>> uart_add_one_port() ->
>>   serial_ctrl_register_port() ->
>>     serial_core_register_port() ->
>>       serial_core_port_device_add() ->
>>         serial_base_port_add() ->
>>           device_add() ->
>>             bus_probe_device() ->
>>               device_initial_probe() ->
>>                 __device_attach() ->
>>                   // ...
>>                   if (dev->p->dead) {
>>                     // ...
>>                   } else if (dev->driver) {
>>                     // ...
>>                   } else {
>>                     // ...
>>                     pm_request_idle(dev);
>>                     // ...
>>                   }
>>
>> The earlycon device clocks are enabled by the bootloader. However, the
>> pm_request_idle() call in __device_attach() disables the SCI port clocks
>> while earlycon is still active.
>>
>> The earlycon write function, serial_console_write(), calls
>> sci_poll_put_char() via serial_console_putchar(). If the SCI port clocks
>> are disabled, writing to earlycon may sometimes cause the SR.TDFE bit to
>> remain unset indefinitely, causing the while loop in sci_poll_put_char()
>> to never exit. On single-core SoCs, this can result in the system being
>> blocked during boot when this issue occurs.
>>
>> To resolve this, increment the runtime PM usage counter for the earlycon
>> SCI device before registering the UART port.
>>
>> Fixes: 0b0cced19ab1 ("serial: sh-sci: Add CONFIG_SERIAL_EARLYCON support")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Thanks for your patch!
> 
>> --- a/drivers/tty/serial/sh-sci.c
>> +++ b/drivers/tty/serial/sh-sci.c
>> @@ -3435,7 +3435,24 @@ static int sci_probe_single(struct platform_device *dev,
>>                 sciport->port.flags |= UPF_HARD_FLOW;
>>         }
>>
>> +       /*
>> +        * In case:
>> +        * - this is the earlycon port (mapped on index 0 in sci_ports[]) and
>> +        * - it now maps to an alias other than zero and
>> +        * - the earlycon is still alive (e.g., "earlycon keep_bootcon" is
>> +        *   available in bootargs)
>> +        *
>> +        * we need to avoid disabling clocks and PM domains through the runtime
>> +        * PM APIs called in __device_attach(). For this, increment the runtime
>> +        * PM reference counter (the clocks and PM domains were already enabled
>> +        * by the bootloader). Otherwise the earlycon may access the HW when it
>> +        * has no clocks enabled leading to failures (infinite loop in
>> +        * sci_poll_put_char()).
>> +        */
>> +
>>         if (sci_ports[0].earlycon && sci_ports[0].port.mapbase == sci_res->start) {
> 
> Now there are two tests for mapbase: here and in sci_probe()...

I'm not sure how can we avoid it. We need to re-check it in this function
as the sci_probe_single() is the one that enables the runtime PM. Would you
prefer to move the devm_pm_runtime_enable() in sci_probe() and have the
pm_runtime_get_noresume() in sci_probe() as well?

Thank you,
Claudiu

> 
>> +               pm_runtime_get_noresume(&dev->dev);
>> +
>>                 /*
>>                  * Skip cleanup up the sci_port[0] in early_console_exit(), this
>>                  * port is the same as the earlycon one.
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 

Re: [PATCH RFT 0/6] serial: sh-sci: Fixes for earlycon and keep_bootcon

[Claudiu Beznea]

Hi, Geert,

On 21.12.2024 11:54, Claudiu Beznea wrote:
> Hi, Geert,
> 
> On 19.12.2024 17:11, Geert Uytterhoeven wrote:
>> Hi Claudiu,
>>
>> On Thu, Dec 5, 2024 at 9:39 AM Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
>>> On 04.12.2024 23:38, Wolfram Sang wrote:
>>>>> in the following scenarios:
>>>>>
>>>>> 1/ "earlycon keep_bootcon" were present in bootargs
>>>>> 2/ only "earlycon" was present in bootargs
>>>>> 3/ none of the "earlycon" or "earlycon keep_bootcon" were present in
>>>>>    bootargs
>>>> ...
>>>>> Please give it a try on your devices as well.
>>>>
>>>> Will happily do so. Is there something to look for? Except for "it
>>>> works"?
>>>
>>> As this code touches the earlycon functionality, of interest are the 3
>>> cases highlighted above:
>>>
>>> 1/ "earlycon keep_bootcon" are both present in bootargs
>>> 2/ only "earlycon" is present in bootargs
>>> 3/ none of the "earlycon" or "earlycon keep_bootcon" are present in
>>>    bootargs
>>>
>>> One other thing, that I was currently able to test only on RZ/G3S, is to
>>> see how it behaves when the debug serial is described in DT with an alias
>>> other than zero. E.g., on [1] the debug serial alias on RZ/G3S was changed
>>> from 0 to 3. With the new alias (3) there were issues that I've tried to
>>> fix with this series.
>>
>> I gave this a try on Koelsch, which has two easily-accessible usb-serial
>> ports, for all three cases above.  Originally, I had CONFIG_VT_CONSOLE=y
>> (tty0 takes over from earlycon rather early), but I had to disable
>> that to exercise all code paths (ttySC0 takes over much later).
>>
>>   A. CONFIG_VT_CONSOLE=y: OK
>>   B. CONFIG_VT_CONSOLE=y earlycon: OK
>>        early_console_setup: mapbase 0x00000000e6e60000
>>        earlycon: scif0 at MMIO 0x00000000e6e60000 (options '115200n8')
>>        printk: legacy bootconsole [scif0] enabled
>>        printk: legacy console [tty0] enabled
>>        printk: legacy bootconsole [scif0] disabled
>>        early_console_exit: Clearing sci_ports[0]
>>   C. CONFIG_VT_CONSOLE=n earlycon: OK
>>        early_console_setup: mapbase 0x00000000e6e60000
>>        earlycon: scif0 at MMIO 0x00000000e6e60000 (options '115200n8')
>>        printk: legacy bootconsole [scif0] enabled
>>        printk: legacy console [ttySC0] enabled
>>        printk: legacy bootconsole [scif0] disabled
>>        early_console_exit: Not clearing sci_ports[0]
>>   D. CONFIG_VT_CONSOLE=y earlycon keep_bootcon: OK
>>        early_console_setup: mapbase 0x00000000e6e60000
>>        earlycon: scif0 at MMIO 0x00000000e6e60000 (options '115200n8')
>>        printk: legacy bootconsole [scif0] enabled
>>        printk: legacy console [tty0] enabled
>>
>> So all good, but note that these cases worked fine without your
>> series, too.
>>
>> The real troublesome cases involve using earlycon on a different
>> serial port than serial0.  As I don't have any Renesas boards where
>> chosen/stdout-path does not use serial0, I tried exchanging the serial0
>> and serial1 DT aliases, and updating chosen/stdout-path accordingly.
>>
>>   E. CONFIG_VT_CONSOLE=y: OK
>>   F. CONFIG_VT_CONSOLE=y earlycon: OK
>>        early_console_setup: mapbase 0x00000000e6e60000
>>        earlycon: scif0 at MMIO 0x00000000e6e60000 (options '115200n8')
>>        printk: legacy bootconsole [scif0] enabled
>>        printk: legacy console [tty0] enabled
>>        printk: legacy bootconsole [scif0] disabled
>>        early_console_exit: Clearing sci_ports[0]
>>   G. CONFIG_VT_CONSOLE=y earlycon keep_bootcon: SCIF1 missing
>>        early_console_setup: mapbase 0x00000000e6e60000
>>        earlycon: scif0 at MMIO 0x00000000e6e60000 (options '115200n8')
>>        printk: legacy bootconsole [scif0] enabled
>>        printk: legacy console [tty0] enabled
>>        sh-sci e6e68000.serial: error -EBUSY: sci_port[0] is used by earlycon!
>>   H. CONFIG_VT_CONSOLE=n earlycon: SCIF1 missing
>>        early_console_setup: mapbase 0x00000000e6e60000
>>        earlycon: scif0 at MMIO 0x00000000e6e60000 (options '115200n8')
>>        printk: legacy bootconsole [scif0] enabled
>>        printk: legacy console [ttySC1] enabled
>>        printk: legacy bootconsole [scif0] disabled
>>        early_console_exit: Not clearing sci_ports[0]
>>        sh-sci e6e68000.serial: error -EBUSY: sci_port[0] is used by earlycon!
>>
>> Case G gives a missing SCIF1, because sci_port[0] is still
>> used for earlycon, as expected.
>> Case H also gives a missing SCIF1, but should succeed IMHO, as earlycon
>> is no longer active.  I think early_console_exit() should clear the
>> earlycon flag regardless.
> 
> I'll double check it.

Indeed, clearing the earlycon flag leads to case H succeeding as well.

> 
>>
>> Note that before your series, cases E-F worked too, but cases G-H gave
>> an initialized but broken SCIF1 instead.
>>
>> Now, can we improve?
>>   - Can we use a proper id instead of zero for earlycon, e.g.
>>     sci_probe_earlyprintk() does fill in early_serial_console.index?
> 
> I looked into that but, as of my investigation, index zero is the one used
> in the earlyprintk initialization process. sci_probe_earlyprintk() is
> called from sci_probe(). I'll double checked it though, anyway.


As of my investigation, the current code initializes the index based on the
name registered though the OF_EARLYCON_DECLARE(). That name is used in
earlycon_init() to initialize the early serial index:

of_setup_earlycon(match, ...)
   earlycon_init(&earlycon_console_dev, match->name)
      // ...
      earlycon->index = simple_strtoul(s, NULL, 10);
      // ...

On sh-sci driver that name is generic and have nothing to do with board setup.

Also, in the sh-sci early_console_setup() function we cannot use
of_alias_get_id() as we don't have a struct device node associated to the
struct earlycon_device object, argument of early_console_setup(), when the
earlycon is configured. And I think, at the moment the
early_console_setup() is called the alias list is not yet populated.

Parsing the linux,stdout-path/stdout-path again in early_console_setup()
will duplicate the code from early_init_dt_scan_chosen_stdout().

I have a PoC code that parses the alias from the stdout-chosen, in
early_init_dt_scan_chosen_stdout(), but introducing it may need testing on
all the currently enabled devices, to avoid breakage. This code takes into
account the device alias provided though the device tree (rudimentary
tested for the moment). Diff bellow:

diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 0121100372b4..ce65b9cc620d 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -876,10 +876,11 @@ void __init
early_init_dt_check_for_usable_mem_range(void)
 int __init early_init_dt_scan_chosen_stdout(void)
 {
        int offset;
-       const char *p, *q, *options = NULL;
-       int l;
+       const char *p, *q, *tmp, *options = NULL;
+       int l, alias_len;
        const struct earlycon_id *match;
        const void *fdt = initial_boot_params;
+       int alias;
        int ret;

        offset = fdt_path_offset(fdt, "/chosen");
@@ -898,6 +899,7 @@ int __init early_init_dt_scan_chosen_stdout(void)
        if (*q != '\0')
                options = q + 1;
        l = q - p;
+       alias_len = q - p;

        /* Get the node specified by stdout-path */
        offset = fdt_path_offset_namelen(fdt, p, l);
@@ -906,6 +908,16 @@ int __init early_init_dt_scan_chosen_stdout(void)
                return 0;
        }

+       /* Get the alias ID. */
+       /* scan backwards from end of string for first non-numeral */
+       for (tmp = p + alias_len;
+            tmp > p && tmp[-1] >= '0' && tmp[-1] <= '9';
+            tmp--)
+               ;
+
+       if (*tmp)
+               alias = *tmp - '0';
+
        for (match = __earlycon_table; match < __earlycon_table_end; match++) {
                if (!match->compatible[0])
                        continue;
@@ -913,7 +925,7 @@ int __init early_init_dt_scan_chosen_stdout(void)
                if (fdt_node_check_compatible(fdt, offset, match->compatible))
                        continue;

-               ret = of_setup_earlycon(match, offset, options);
+               ret = of_setup_earlycon(match, offset, options, alias);
                if (!ret || ret == -EALREADY)
                        return 0;
        }
diff --git a/drivers/tty/serial/earlycon.c b/drivers/tty/serial/earlycon.c
index ab9af37f6cda..52b4a5c56269 100644
--- a/drivers/tty/serial/earlycon.c
+++ b/drivers/tty/serial/earlycon.c
@@ -53,7 +53,7 @@ static void __iomem * __init earlycon_map(resource_size_t
paddr, size_t size)
 }

 static void __init earlycon_init(struct earlycon_device *device,
-                                const char *name)
+                                const char *name, int alias)
 {
        struct console *earlycon = device->con;
        const char *s;
@@ -66,6 +66,9 @@ static void __init earlycon_init(struct earlycon_device
*device,
                ;
        if (*s)
                earlycon->index = simple_strtoul(s, NULL, 10);
+       else if (alias >= 0)
+               earlycon->index = alias;
+
        len = s - name;
        strscpy(earlycon->name, name, min(len + 1, sizeof(earlycon->name)));
        earlycon->data = &early_console_dev;
@@ -150,7 +153,7 @@ static int __init register_earlycon(char *buf, const
struct earlycon_id *match)
        if (port->mapbase)
                port->membase = earlycon_map(port->mapbase, 64);

-       earlycon_init(&early_console_dev, match->name);
+       earlycon_init(&early_console_dev, match->name, -1);
        err = match->setup(&early_console_dev, buf);
        earlycon_print_info(&early_console_dev);
        if (err < 0)
@@ -275,7 +278,8 @@ early_param("console", param_setup_earlycon_console_alias);

 int __init of_setup_earlycon(const struct earlycon_id *match,
                             unsigned long node,
-                            const char *options)
+                            const char *options,
+                            int alias)
 {
        int err;
        struct uart_port *port = &early_console_dev.port;
@@ -337,7 +341,7 @@ int __init of_setup_earlycon(const struct earlycon_id
*match,
                strscpy(early_console_dev.options, options,
                        sizeof(early_console_dev.options));
        }
-       earlycon_init(&early_console_dev, match->name);
+       earlycon_init(&early_console_dev, match->name, alias);
        err = match->setup(&early_console_dev, options);
        earlycon_print_info(&early_console_dev);
        if (err < 0)
diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 17e0cd7642fc..10dff3053218 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -3655,17 +3655,19 @@ static int early_console_exit(struct console *co)
 static int __init early_console_setup(struct earlycon_device *device,
                                      int type)
 {
+       int index = device->con->index;
+
        if (!device->port.membase)
                return -ENODEV;

        device->port.type = type;
-       sci_ports[0].port = device->port;
+       sci_ports[index].port = device->port;
        port_cfg.type = type;
-       sci_ports[0].cfg = &port_cfg;
-       sci_ports[0].params = sci_probe_regmap(&port_cfg);
-       sci_ports[0].earlycon = true;
-       port_cfg.scscr = sci_serial_in(&sci_ports[0].port, SCSCR);
-       sci_serial_out(&sci_ports[0].port, SCSCR,
+       sci_ports[index].cfg = &port_cfg;
+       sci_ports[index].params = sci_probe_regmap(&port_cfg);
+       sci_uart_earlycon = true;
+       port_cfg.scscr = sci_serial_in(&sci_ports[index].port, SCSCR);
+       sci_serial_out(&sci_ports[index].port, SCSCR,
                       SCSCR_RE | SCSCR_TE | port_cfg.scscr);

        device->con->write = serial_console_write;
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 743b4afaad4c..06c46ade5482 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -1078,7 +1078,7 @@ extern const struct earlycon_id __earlycon_table_end[];
 #define EARLYCON_DECLARE(_name, fn)    OF_EARLYCON_DECLARE(_name, "", fn)

 int of_setup_earlycon(const struct earlycon_id *match, unsigned long node,
-                     const char *options);
+                     const char *options, int alias);

 #ifdef CONFIG_SERIAL_EARLYCON
 extern bool earlycon_acpi_spcr_enable __initdata;


What do you think? Are you OK with still handling it in the driver though
the index 0 (mostly as it has been proposed in the previous version) or
prefer to go with the PoC code that may change behavior on current devices?
Or do you have another idea?

Thank you,
Claudiu

> 
> 
>>   - Alternatively, can we use a separate sci_port structure instead
>>     of abusing sci_ports[0]?
> 
> I explored this too, but didn't manage to make it work.
> 
> Thank you for running all these tests,
> Claudiu
> 
>>
>> 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
>