[PATCH v4 0/4] Add support for the rest of Renesas RZ/G3S serial interfaces

[PATCH v4 0/4] Add support for the rest of Renesas RZ/G3S serial interfaces

[Claudiu]

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

Hi,

The Renesas RZ/G3S SoC has 6 serial interfaces. One of them is used
as debug console (and it is already enabled in the current code base).
Series adds support for the remaining ones.

Patches:
-    01/04 - extends suspend to RAM support on the serial driver for
             the RZ/G3S SoC
- 02-04/04 - add device tree support

Merge strategy, if any:
- patch 01/04 can go through the serial tree
- patches 02-04/04 can go through the Renesas tree

Thank you,
Claudiu Beznea

Changes in v4:
- dropped fixes and clock patches as they were applied independently
- dropped DT patches that were already applied
- addressed review comments

Changes in v3:
- in patch "serial: sh-sci: Check if TX data was written to device in
  .tx_empty()":
-- check the status of the DMA transaction in tx_empty()
-- changed the variable name that tracks if TX occurred

Changes in v2:
- drop patch "serial: sh-sci: Clean sci_ports[0] after at earlycon exit"
  from v1 as it was already applied
- used bool instead of atomic_t in patch
  "serial: sh-sci: Check if TX data was written to device in .tx_empty()"


Claudiu Beznea (4):
  serial: sh-sci: Update the suspend/resume support
  arm64: dts: renesas: rzg3s-smarc-switches: Add a header to describe
    different switches
  arm64: dts: renesas: rzg3s-smarc: Enable SCIF3
  arm64: dts: renesas: r9a08g045s33-smarc-pmod: Add overlay for SCIF1

 arch/arm64/boot/dts/renesas/Makefile          |  3 ++
 .../r9a08g045s33-smarc-pmod1-type-3a.dtso     | 48 +++++++++++++++++
 .../boot/dts/renesas/rzg3s-smarc-som.dtsi     | 20 +------
 .../boot/dts/renesas/rzg3s-smarc-switches.h   | 40 ++++++++++++++
 arch/arm64/boot/dts/renesas/rzg3s-smarc.dtsi  | 13 +++++
 drivers/tty/serial/sh-sci.c                   | 53 +++++++++++++++----
 6 files changed, 149 insertions(+), 28 deletions(-)
 create mode 100644 arch/arm64/boot/dts/renesas/r9a08g045s33-smarc-pmod1-type-3a.dtso
 create mode 100644 arch/arm64/boot/dts/renesas/rzg3s-smarc-switches.h

-- 
2.43.0

[PATCH v4 1/4] serial: sh-sci: Update the suspend/resume support

[Claudiu]

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

The Renesas RZ/G3S supports a power saving mode where power to most of the
SoC components is turned off. When returning from this power saving mode,
SoC components need to be re-configured.

The SCIFs on the Renesas RZ/G3S need to be re-configured as well when
returning from this power saving mode. The sh-sci code already configures
the SCIF clocks, power domain and registers by calling uart_resume_port()
in sci_resume(). On suspend path the SCIF UART ports are suspended
accordingly (by calling uart_suspend_port() in sci_suspend()). The only
missing setting is the reset signal. For this assert/de-assert the reset
signal on driver suspend/resume.

In case the no_console_suspend is specified by the user, the registers need
to be saved on suspend path and restore on resume path. To do this the
sci_console_setup() function was added. There is no need to cache/restore
the status or FIFO registers. Only the control registers. To differentiate
b/w these, the struct sci_port_params::regs was updated with a new member
that specifies if the register needs to be chached on suspend. Only the
RZ_SCIFA instances were updated with this new support as the hardware for
the rest of variants was missing for testing.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v4:
- none

Changes in v3:
- none

Changes in v2:
- rebased on top of the update version of patch 2/8 from
  this series

 drivers/tty/serial/sh-sci.c | 53 ++++++++++++++++++++++++++++++-------
 1 file changed, 44 insertions(+), 9 deletions(-)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index b1ea48f38248..ae43237dd684 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -101,7 +101,7 @@ enum SCI_CLKS {
 		if ((_port)->sampling_rate_mask & SCI_SR((_sr)))
 
 struct plat_sci_reg {
-	u8 offset, size;
+	u8 offset, size, suspend_cacheable;
 };
 
 struct sci_port_params {
@@ -134,6 +134,8 @@ struct sci_port {
 	struct dma_chan			*chan_tx;
 	struct dma_chan			*chan_rx;
 
+	struct reset_control		*rstc;
+
 #ifdef CONFIG_SERIAL_SH_SCI_DMA
 	struct dma_chan			*chan_tx_saved;
 	struct dma_chan			*chan_rx_saved;
@@ -153,6 +155,7 @@ struct sci_port {
 	int				rx_trigger;
 	struct timer_list		rx_fifo_timer;
 	int				rx_fifo_timeout;
+	unsigned int			console_cached_regs[SCIx_NR_REGS];
 	u16				hscif_tot;
 
 	bool has_rtscts;
@@ -300,17 +303,17 @@ static const struct sci_port_params sci_port_params[SCIx_NR_REGTYPES] = {
 	 */
 	[SCIx_RZ_SCIFA_REGTYPE] = {
 		.regs = {
-			[SCSMR]		= { 0x00, 16 },
-			[SCBRR]		= { 0x02,  8 },
-			[SCSCR]		= { 0x04, 16 },
+			[SCSMR]		= { 0x00, 16, 1 },
+			[SCBRR]		= { 0x02,  8, 1 },
+			[SCSCR]		= { 0x04, 16, 1 },
 			[SCxTDR]	= { 0x06,  8 },
 			[SCxSR]		= { 0x08, 16 },
 			[SCxRDR]	= { 0x0A,  8 },
-			[SCFCR]		= { 0x0C, 16 },
+			[SCFCR]		= { 0x0C, 16, 1 },
 			[SCFDR]		= { 0x0E, 16 },
-			[SCSPTR]	= { 0x10, 16 },
+			[SCSPTR]	= { 0x10, 16, 1 },
 			[SCLSR]		= { 0x12, 16 },
-			[SEMR]		= { 0x14, 8 },
+			[SEMR]		= { 0x14, 8, 1 },
 		},
 		.fifosize = 16,
 		.overrun_reg = SCLSR,
@@ -3374,6 +3377,7 @@ static struct plat_sci_port *sci_parse_dt(struct platform_device *pdev,
 	}
 
 	sp = &sci_ports[id];
+	sp->rstc = rstc;
 	*dev_id = id;
 
 	p->type = SCI_OF_TYPE(data);
@@ -3546,13 +3550,34 @@ static int sci_probe(struct platform_device *dev)
 	return 0;
 }
 
+static void sci_console_setup(struct sci_port *s, bool save)
+{
+	for (u16 i = 0; i < SCIx_NR_REGS; i++) {
+		struct uart_port *port = &s->port;
+
+		if (!s->params->regs[i].suspend_cacheable)
+			continue;
+
+		if (save)
+			s->console_cached_regs[i] = sci_serial_in(port, i);
+		else
+			sci_serial_out(port, i, s->console_cached_regs[i]);
+	}
+}
+
 static __maybe_unused int sci_suspend(struct device *dev)
 {
 	struct sci_port *sport = dev_get_drvdata(dev);
 
-	if (sport)
+	if (sport) {
 		uart_suspend_port(&sci_uart_driver, &sport->port);
 
+		if (!console_suspend_enabled && uart_console(&sport->port))
+			sci_console_setup(sport, true);
+		else
+			return reset_control_assert(sport->rstc);
+	}
+
 	return 0;
 }
 
@@ -3560,8 +3585,18 @@ static __maybe_unused int sci_resume(struct device *dev)
 {
 	struct sci_port *sport = dev_get_drvdata(dev);
 
-	if (sport)
+	if (sport) {
+		if (!console_suspend_enabled && uart_console(&sport->port)) {
+			sci_console_setup(sport, false);
+		} else {
+			int ret = reset_control_deassert(sport->rstc);
+
+			if (ret)
+				return ret;
+		}
+
 		uart_resume_port(&sci_uart_driver, &sport->port);
+	}
 
 	return 0;
 }
-- 
2.43.0

[PATCH v4 2/4] arm64: dts: renesas: rzg3s-smarc-switches: Add a header to describe different switches

[Claudiu]

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

There are different switches available on both the RZ/G3S SMARC Module and
RZ SMARC Carrier II boards. These switches are used to route different SoC
signals to different parts available on board.

These switches are described in device trees through macros. These macros
are set accordingly such that the resulted compiled dtb to describe the
on-board switches states.

The SCIF1 depends on the state of the SW_CONFIG3 and SW_OPT_MUX4 switches.
SCIF1 can be enabled through a device tree overlay. To manage all switches
in a unified state and allow users to configure the output device tree, add
a file that contains all switch definitions and states.

Commit prepares the code to enable SCIF1 on the RZ/G3S overlay.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v4:
- adjusted the patch description
- used GPL-2.0-only OR BSD-2-Clause license
- used proper description for SW_CONFIG3

Changes in v3:
- none

Changes in v2:
- none

 .../boot/dts/renesas/rzg3s-smarc-som.dtsi     | 20 +-----------
 .../boot/dts/renesas/rzg3s-smarc-switches.h   | 32 +++++++++++++++++++
 2 files changed, 33 insertions(+), 19 deletions(-)
 create mode 100644 arch/arm64/boot/dts/renesas/rzg3s-smarc-switches.h

diff --git a/arch/arm64/boot/dts/renesas/rzg3s-smarc-som.dtsi b/arch/arm64/boot/dts/renesas/rzg3s-smarc-som.dtsi
index ef12c1c462a7..39845faec894 100644
--- a/arch/arm64/boot/dts/renesas/rzg3s-smarc-som.dtsi
+++ b/arch/arm64/boot/dts/renesas/rzg3s-smarc-som.dtsi
@@ -9,25 +9,7 @@
 #include <dt-bindings/gpio/gpio.h>
 #include <dt-bindings/pinctrl/rzg2l-pinctrl.h>
 
-/*
- * On-board switches' states:
- * @SW_OFF: switch's state is OFF
- * @SW_ON:  switch's state is ON
- */
-#define SW_OFF		0
-#define SW_ON		1
-
-/*
- * SW_CONFIG[x] switches' states:
- * @SW_CONFIG2:
- *	SW_OFF - SD0 is connected to eMMC
- *	SW_ON  - SD0 is connected to uSD0 card
- * @SW_CONFIG3:
- *	SW_OFF - SD2 is connected to SoC
- *	SW_ON  - SCIF1, SSI0, IRQ0, IRQ1 connected to SoC
- */
-#define SW_CONFIG2	SW_OFF
-#define SW_CONFIG3	SW_ON
+#include "rzg3s-smarc-switches.h"
 
 / {
 	compatible = "renesas,rzg3s-smarcm", "renesas,r9a08g045s33", "renesas,r9a08g045";
diff --git a/arch/arm64/boot/dts/renesas/rzg3s-smarc-switches.h b/arch/arm64/boot/dts/renesas/rzg3s-smarc-switches.h
new file mode 100644
index 000000000000..514a8a6dc013
--- /dev/null
+++ b/arch/arm64/boot/dts/renesas/rzg3s-smarc-switches.h
@@ -0,0 +1,32 @@
+/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
+/*
+ * On-board switches for the Renesas RZ/G3S SMARC Module and RZ SMARC Carrier II
+ * boards.
+ *
+ * Copyright (C) 2024 Renesas Electronics Corp.
+ */
+
+#ifndef __RZG3S_SMARC_SWITCHES__
+#define __RZG3S_SMARC_SWITCHES__
+
+/*
+ * On-board switches' states:
+ * @SW_OFF: switch's state is OFF
+ * @SW_ON:  switch's state is ON
+ */
+#define SW_OFF		0
+#define SW_ON		1
+
+/*
+ * SW_CONFIG[x] switches' states:
+ * @SW_CONFIG2:
+ *	SW_OFF - SD0 is connected to eMMC
+ *	SW_ON  - SD0 is connected to uSD0 card
+ * @SW_CONFIG3:
+ *	SW_OFF - SD2 is connected to SoC
+ *	SW_ON  - SCIF1, SSI0, IRQ0, IRQ1 connected to SoC
+ */
+#define SW_CONFIG2	SW_OFF
+#define SW_CONFIG3	SW_ON
+
+#endif /* __RZG3S_SMARC_SWITCHES__ */
-- 
2.43.0

[PATCH v4 3/4] arm64: dts: renesas: rzg3s-smarc: Enable SCIF3

[Claudiu]

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

Enable SCIF3. It is routed on the RZ SMARC Carrier II board on SER1_UART
interface.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v4:
- dropped checking the SW_CONFIG3
- dropped the include of rzg3s-smarc-switches.h

Changes in v3:
- none

Changes in v2:
- none

 arch/arm64/boot/dts/renesas/rzg3s-smarc.dtsi | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/rzg3s-smarc.dtsi b/arch/arm64/boot/dts/renesas/rzg3s-smarc.dtsi
index 81b4ffd1417d..0851e0b7ed40 100644
--- a/arch/arm64/boot/dts/renesas/rzg3s-smarc.dtsi
+++ b/arch/arm64/boot/dts/renesas/rzg3s-smarc.dtsi
@@ -12,6 +12,7 @@
 / {
 	aliases {
 		i2c0 = &i2c0;
+		serial1 = &scif3;
 		serial3 = &scif0;
 		mmc1 = &sdhi1;
 	};
@@ -162,6 +163,11 @@ scif0_pins: scif0 {
 			 <RZG2L_PORT_PINMUX(6, 4, 1)>; /* TXD */
 	};
 
+	scif3_pins: scif3 {
+		pinmux = <RZG2L_PORT_PINMUX(17, 2, 7)>, /* RXD */
+			 <RZG2L_PORT_PINMUX(17, 3, 7)>; /* TXD */
+	};
+
 	sdhi1_pins: sd1 {
 		data {
 			pins = "SD1_DATA0", "SD1_DATA1", "SD1_DATA2", "SD1_DATA3";
@@ -208,6 +214,12 @@ &scif0 {
 	status = "okay";
 };
 
+&scif3 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&scif3_pins>;
+	status = "okay";
+};
+
 &sdhi1 {
 	pinctrl-0 = <&sdhi1_pins>;
 	pinctrl-1 = <&sdhi1_pins_uhs>;
-- 
2.43.0

[PATCH v4 4/4] arm64: dts: renesas: r9a08g045s33-smarc-pmod: Add overlay for SCIF1

[Claudiu]

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

Add DT overlay for SCIF1 (of the Renesas RZ/G3S SoC) routed through the
PMOD1_3A interface available on the Renesas RZ SMARC Carrier II board.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v4:
- rename overlay name to r9a08g045s33-smarc-pmod1-type-3a
- add note about the needed switches for SCIF1
- guard the scif1 node with #if SW_CONFIG3 == SW_ON && SW_OPT_MUX4 == SW_ON
- dropped the alias section from the overlay file and move it
  the board file
- document SW_OPT_MUX4 switch

Changes in v3:
- none

Changes in v2:
- none

 arch/arm64/boot/dts/renesas/Makefile          |  3 ++
 .../r9a08g045s33-smarc-pmod1-type-3a.dtso     | 48 +++++++++++++++++++
 .../boot/dts/renesas/rzg3s-smarc-switches.h   |  8 ++++
 arch/arm64/boot/dts/renesas/rzg3s-smarc.dtsi  |  1 +
 4 files changed, 60 insertions(+)
 create mode 100644 arch/arm64/boot/dts/renesas/r9a08g045s33-smarc-pmod1-type-3a.dtso

diff --git a/arch/arm64/boot/dts/renesas/Makefile b/arch/arm64/boot/dts/renesas/Makefile
index 928635f2e76b..ef7f7b55145d 100644
--- a/arch/arm64/boot/dts/renesas/Makefile
+++ b/arch/arm64/boot/dts/renesas/Makefile
@@ -143,6 +143,9 @@ r9a07g054l2-smarc-cru-csi-ov5645-dtbs := r9a07g054l2-smarc.dtb r9a07g054l2-smarc
 dtb-$(CONFIG_ARCH_R9A07G054) += r9a07g054l2-smarc-cru-csi-ov5645.dtb
 
 dtb-$(CONFIG_ARCH_R9A08G045) += r9a08g045s33-smarc.dtb
+dtb-$(CONFIG_ARCH_R9A07G043) += r9a08g045s33-smarc-pmod1-type-3a.dtbo
+r9a08g045s33-smarc-pmod1-type-3a-dtbs := r9a08g045s33-smarc.dtb r9a08g045s33-smarc-pmod1-type-3a.dtbo
+dtb-$(CONFIG_ARCH_R9A07G043) += r9a08g045s33-smarc-pmod1-type-3a.dtb
 
 dtb-$(CONFIG_ARCH_R9A09G011) += r9a09g011-v2mevk2.dtb
 
diff --git a/arch/arm64/boot/dts/renesas/r9a08g045s33-smarc-pmod1-type-3a.dtso b/arch/arm64/boot/dts/renesas/r9a08g045s33-smarc-pmod1-type-3a.dtso
new file mode 100644
index 000000000000..e4cb4449f190
--- /dev/null
+++ b/arch/arm64/boot/dts/renesas/r9a08g045s33-smarc-pmod1-type-3a.dtso
@@ -0,0 +1,48 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Device Tree Source for the RZ/G3S SMARC Carrier II EVK PMOD parts
+ *
+ * Copyright (C) 2024 Renesas Electronics Corp.
+ *
+ *
+ * [Connection]
+ *
+ * SMARC Carrier II EVK
+ * +--------------------------------------------+
+ * |PMOD1_3A (PMOD1 PIN HEADER)			|
+ * |	SCIF1_CTS# (pin1)  (pin7)  PMOD1_GPIO10	|
+ * |	SCIF1_TXD  (pin2)  (pin8)  PMOD1_GPIO11	|
+ * |	SCIF1_RXD  (pin3)  (pin9)  PMOD1_GPIO12	|
+ * |	SCIF1_RTS# (pin4)  (pin10) PMOD1_GPIO13	|
+ * |	GND	   (pin5)  (pin11) GND		|
+ * |	PWR_PMOD1  (pin6)  (pin12) GND		|
+ * +--------------------------------------------+
+ *
+ * The following switches should be set as follows for SCIF1:
+ * - SW_CONFIG2:  ON
+ * - SW_OPT_MUX4: ON
+ */
+
+/dts-v1/;
+/plugin/;
+
+#include <dt-bindings/pinctrl/rzg2l-pinctrl.h>
+#include "rzg3s-smarc-switches.h"
+
+&pinctrl {
+	scif1_pins: scif1-pins {
+		pinmux = <RZG2L_PORT_PINMUX(14, 0, 1)>, /* TXD */
+			 <RZG2L_PORT_PINMUX(14, 1, 1)>, /* RXD */
+			 <RZG2L_PORT_PINMUX(16, 0, 1)>, /* CTS */
+			 <RZG2L_PORT_PINMUX(16, 1, 1)>; /* RTS */
+	};
+};
+
+#if SW_CONFIG3 == SW_ON && SW_OPT_MUX4 == SW_ON
+&scif1 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&scif1_pins>;
+	uart-has-rtscts;
+	status = "okay";
+};
+#endif
diff --git a/arch/arm64/boot/dts/renesas/rzg3s-smarc-switches.h b/arch/arm64/boot/dts/renesas/rzg3s-smarc-switches.h
index 514a8a6dc013..9766cea55dc6 100644
--- a/arch/arm64/boot/dts/renesas/rzg3s-smarc-switches.h
+++ b/arch/arm64/boot/dts/renesas/rzg3s-smarc-switches.h
@@ -29,4 +29,12 @@
 #define SW_CONFIG2	SW_OFF
 #define SW_CONFIG3	SW_ON
 
+/*
+ * SW_OPT_MUX[x] switches' states:
+ * @SW_OPT_MUX4:
+ *	SW_OFF - The SMARC SER0 signals are routed to M.2 Key E UART
+ *	SW_ON  - The SMARC SER0 signals are routed to PMOD1
+ */
+#define SW_OPT_MUX4	SW_ON
+
 #endif /* __RZG3S_SMARC_SWITCHES__ */
diff --git a/arch/arm64/boot/dts/renesas/rzg3s-smarc.dtsi b/arch/arm64/boot/dts/renesas/rzg3s-smarc.dtsi
index 0851e0b7ed40..5e044a4d0234 100644
--- a/arch/arm64/boot/dts/renesas/rzg3s-smarc.dtsi
+++ b/arch/arm64/boot/dts/renesas/rzg3s-smarc.dtsi
@@ -12,6 +12,7 @@
 / {
 	aliases {
 		i2c0 = &i2c0;
+		serial0 = &scif1;
 		serial1 = &scif3;
 		serial3 = &scif0;
 		mmc1 = &sdhi1;
-- 
2.43.0

Re: [PATCH v4 1/4] serial: sh-sci: Update the suspend/resume support

[Krzysztof Kozlowski]

On 20/01/2025 14:09, Claudiu wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> The Renesas RZ/G3S supports a power saving mode where power to most of the
> SoC components is turned off. When returning from this power saving mode,
> SoC components need to be re-configured.
> 
> The SCIFs on the Renesas RZ/G3S need to be re-configured as well when
> returning from this power saving mode. The sh-sci code already configures
> the SCIF clocks, power domain and registers by calling uart_resume_port()
> in sci_resume(). On suspend path the SCIF UART ports are suspended
> accordingly (by calling uart_suspend_port() in sci_suspend()). The only
> missing setting is the reset signal. For this assert/de-assert the reset
> signal on driver suspend/resume.
> 
> In case the no_console_suspend is specified by the user, the registers need
> to be saved on suspend path and restore on resume path. To do this the
> sci_console_setup() function was added. There is no need to cache/restore
> the status or FIFO registers. Only the control registers. To differentiate
> b/w these, the struct sci_port_params::regs was updated with a new member
> that specifies if the register needs to be chached on suspend. Only the
> RZ_SCIFA instances were updated with this new support as the hardware for
> the rest of variants was missing for testing.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---
> 
> Changes in v4:
> - none


Why are you combining serial patches with DTS? Greg applies entire set
thus you *cannot* send him DTS.

Best regards,
Krzysztof

Re: [PATCH v4 1/4] serial: sh-sci: Update the suspend/resume support

[Claudiu Beznea]

On 21.01.2025 10:54, Krzysztof Kozlowski wrote:
> On 20/01/2025 14:09, Claudiu wrote:
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> The Renesas RZ/G3S supports a power saving mode where power to most of the
>> SoC components is turned off. When returning from this power saving mode,
>> SoC components need to be re-configured.
>>
>> The SCIFs on the Renesas RZ/G3S need to be re-configured as well when
>> returning from this power saving mode. The sh-sci code already configures
>> the SCIF clocks, power domain and registers by calling uart_resume_port()
>> in sci_resume(). On suspend path the SCIF UART ports are suspended
>> accordingly (by calling uart_suspend_port() in sci_suspend()). The only
>> missing setting is the reset signal. For this assert/de-assert the reset
>> signal on driver suspend/resume.
>>
>> In case the no_console_suspend is specified by the user, the registers need
>> to be saved on suspend path and restore on resume path. To do this the
>> sci_console_setup() function was added. There is no need to cache/restore
>> the status or FIFO registers. Only the control registers. To differentiate
>> b/w these, the struct sci_port_params::regs was updated with a new member
>> that specifies if the register needs to be chached on suspend. Only the
>> RZ_SCIFA instances were updated with this new support as the hardware for
>> the rest of variants was missing for testing.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>> ---
>>
>> Changes in v4:
>> - none
> 
> 
> Why are you combining serial patches with DTS? Greg applies entire set
> thus you *cannot* send him DTS.

It's v4. The initial set contained fixes for serial, support for RZ/G3S
(including clocks and dtsi), all that was needed for the enabled RZ/G3S
serial IPs. Fixes were posted separately (as requested), the other bringup
patches were integrated and this is what remained. I chose it like this for
version continuity.

Thank you,
Claudiu

> 
> Best regards,
> Krzysztof

Re: [PATCH v4 1/4] serial: sh-sci: Update the suspend/resume support

[Geert Uytterhoeven]

Hi Claudiu,

Thanks for your patch!

On Mon, Jan 20, 2025 at 2:09 PM Claudiu <claudiu.beznea@tuxon.dev> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> The Renesas RZ/G3S supports a power saving mode where power to most of the
> SoC components is turned off. When returning from this power saving mode,
> SoC components need to be re-configured.
>
> The SCIFs on the Renesas RZ/G3S need to be re-configured as well when
> returning from this power saving mode. The sh-sci code already configures
> the SCIF clocks, power domain and registers by calling uart_resume_port()
> in sci_resume(). On suspend path the SCIF UART ports are suspended
> accordingly (by calling uart_suspend_port() in sci_suspend()). The only
> missing setting is the reset signal. For this assert/de-assert the reset
> signal on driver suspend/resume.
>
> In case the no_console_suspend is specified by the user, the registers need
> to be saved on suspend path and restore on resume path. To do this the
> sci_console_setup() function was added. There is no need to cache/restore
> the status or FIFO registers. Only the control registers. To differentiate
> b/w these, the struct sci_port_params::regs was updated with a new member
> that specifies if the register needs to be chached on suspend. Only the

cached

> RZ_SCIFA instances were updated with this new support as the hardware for
> the rest of variants was missing for testing.
>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -101,7 +101,7 @@ enum SCI_CLKS {
>                 if ((_port)->sampling_rate_mask & SCI_SR((_sr)))
>
>  struct plat_sci_reg {
> -       u8 offset, size;
> +       u8 offset, size, suspend_cacheable;

This increases the size of sci_port_params[] by 300 bytes.
Using bitfields would mitigate that:

    struct plat_sci_reg {
            u16 offset:8;
            u16 size:5;
            u16 suspend_cacheable:1;
    };

(if we ever need more bits, the size member can store an enum value
 instead of the actual size (8 or 16 bits) of the register).

>  };
>
>  struct sci_port_params {
> @@ -134,6 +134,8 @@ struct sci_port {
>         struct dma_chan                 *chan_tx;
>         struct dma_chan                 *chan_rx;
>
> +       struct reset_control            *rstc;
> +
>  #ifdef CONFIG_SERIAL_SH_SCI_DMA
>         struct dma_chan                 *chan_tx_saved;
>         struct dma_chan                 *chan_rx_saved;
> @@ -153,6 +155,7 @@ struct sci_port {
>         int                             rx_trigger;
>         struct timer_list               rx_fifo_timer;
>         int                             rx_fifo_timeout;
> +       unsigned int                    console_cached_regs[SCIx_NR_REGS];

u16, as all registers are 8 or 16 bit wide.

We reserve space for 20 registers, but at most 6 will be used.
This has a rather big impact on the size of sci_ports[], as
CONFIG_SERIAL_SH_SCI_NR_UARTS defaults to 18.

Also, this space is used/needed only if:
  - CONFIG_PM_SLEEP=y,
  - CONFIG_SERIAL_CORE_CONSOLE=y (see uart_console()),
  - The port is actually used as a console (unfortunately the user
    can specify multiple console=ttySC<N> command line parameters, in
    addition to chosen/stdout-path).

>         u16                             hscif_tot;
>
>         bool has_rtscts;
> @@ -300,17 +303,17 @@ static const struct sci_port_params sci_port_params[SCIx_NR_REGTYPES] = {
>          */
>         [SCIx_RZ_SCIFA_REGTYPE] = {
>                 .regs = {
> -                       [SCSMR]         = { 0x00, 16 },
> -                       [SCBRR]         = { 0x02,  8 },
> -                       [SCSCR]         = { 0x04, 16 },
> +                       [SCSMR]         = { 0x00, 16, 1 },
> +                       [SCBRR]         = { 0x02,  8, 1 },
> +                       [SCSCR]         = { 0x04, 16, 1 },
>                         [SCxTDR]        = { 0x06,  8 },
>                         [SCxSR]         = { 0x08, 16 },
>                         [SCxRDR]        = { 0x0A,  8 },
> -                       [SCFCR]         = { 0x0C, 16 },
> +                       [SCFCR]         = { 0x0C, 16, 1 },
>                         [SCFDR]         = { 0x0E, 16 },
> -                       [SCSPTR]        = { 0x10, 16 },
> +                       [SCSPTR]        = { 0x10, 16, 1 },
>                         [SCLSR]         = { 0x12, 16 },
> -                       [SEMR]          = { 0x14, 8 },
> +                       [SEMR]          = { 0x14, 8, 1 },

Note that the driver always writes zero to SEMR.

>                 },
>                 .fifosize = 16,
>                 .overrun_reg = SCLSR,
> @@ -3374,6 +3377,7 @@ static struct plat_sci_port *sci_parse_dt(struct platform_device *pdev,
>         }
>
>         sp = &sci_ports[id];
> +       sp->rstc = rstc;
>         *dev_id = id;
>
>         p->type = SCI_OF_TYPE(data);
> @@ -3546,13 +3550,34 @@ static int sci_probe(struct platform_device *dev)
>         return 0;
>  }
>
> +static void sci_console_setup(struct sci_port *s, bool save)

sci_console_save_restore()?

> +{
> +       for (u16 i = 0; i < SCIx_NR_REGS; i++) {

unsigned int

> +               struct uart_port *port = &s->port;
> +
> +               if (!s->params->regs[i].suspend_cacheable)
> +                       continue;
> +
> +               if (save)
> +                       s->console_cached_regs[i] = sci_serial_in(port, i);
> +               else
> +                       sci_serial_out(port, i, s->console_cached_regs[i]);
> +       }
> +}

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 v4 2/4] arm64: dts: renesas: rzg3s-smarc-switches: Add a header to describe different switches

[Geert Uytterhoeven]

On Mon, Jan 20, 2025 at 2:09 PM Claudiu <claudiu.beznea@tuxon.dev> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> There are different switches available on both the RZ/G3S SMARC Module and
> RZ SMARC Carrier II boards. These switches are used to route different SoC
> signals to different parts available on board.
>
> These switches are described in device trees through macros. These macros
> are set accordingly such that the resulted compiled dtb to describe the
> on-board switches states.
>
> The SCIF1 depends on the state of the SW_CONFIG3 and SW_OPT_MUX4 switches.
> SCIF1 can be enabled through a device tree overlay. To manage all switches
> in a unified state and allow users to configure the output device tree, add
> a file that contains all switch definitions and states.
>
> Commit prepares the code to enable SCIF1 on the RZ/G3S overlay.
>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---
>
> Changes in v4:
> - adjusted the patch description
> - used GPL-2.0-only OR BSD-2-Clause license
> - used proper description for SW_CONFIG3

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
i.e. will queue in renesas-devel for v6.15.

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 v4 3/4] arm64: dts: renesas: rzg3s-smarc: Enable SCIF3

[Geert Uytterhoeven]

On Mon, Jan 20, 2025 at 2:09 PM Claudiu <claudiu.beznea@tuxon.dev> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> Enable SCIF3. It is routed on the RZ SMARC Carrier II board on SER1_UART
> interface.
>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---
>
> Changes in v4:
> - dropped checking the SW_CONFIG3
> - dropped the include of rzg3s-smarc-switches.h

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
i.e. will queue in renesas-devel for v6.15.

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 v4 4/4] arm64: dts: renesas: r9a08g045s33-smarc-pmod: Add overlay for SCIF1

[Geert Uytterhoeven]

Hi Claudiu,

On Mon, Jan 20, 2025 at 2:09 PM Claudiu <claudiu.beznea@tuxon.dev> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> Add DT overlay for SCIF1 (of the Renesas RZ/G3S SoC) routed through the
> PMOD1_3A interface available on the Renesas RZ SMARC Carrier II board.
>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---
>
> Changes in v4:
> - rename overlay name to r9a08g045s33-smarc-pmod1-type-3a
> - add note about the needed switches for SCIF1
> - guard the scif1 node with #if SW_CONFIG3 == SW_ON && SW_OPT_MUX4 == SW_ON
> - dropped the alias section from the overlay file and move it
>   the board file
> - document SW_OPT_MUX4 switch

Thanks for the update!

> --- /dev/null
> +++ b/arch/arm64/boot/dts/renesas/r9a08g045s33-smarc-pmod1-type-3a.dtso
> @@ -0,0 +1,48 @@
> +// SPDX-License-Identifier: GPL-2.0

Would you mind if I changed this to (GPL-2.0-only OR BSD-2-Clause) while
applying?

> +&pinctrl {
> +       scif1_pins: scif1-pins {
> +               pinmux = <RZG2L_PORT_PINMUX(14, 0, 1)>, /* TXD */
> +                        <RZG2L_PORT_PINMUX(14, 1, 1)>, /* RXD */
> +                        <RZG2L_PORT_PINMUX(16, 0, 1)>, /* CTS */
> +                        <RZG2L_PORT_PINMUX(16, 1, 1)>; /* RTS */

CTS# and RTS#

> +       };
> +};

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
i.e. will queue in renesas-devel for v6.15.

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 v4 4/4] arm64: dts: renesas: r9a08g045s33-smarc-pmod: Add overlay for SCIF1

[Claudiu Beznea]

Hi, Geert,

On 24.01.2025 14:56, Geert Uytterhoeven wrote:
> Hi Claudiu,
> 
> On Mon, Jan 20, 2025 at 2:09 PM Claudiu <claudiu.beznea@tuxon.dev> wrote:
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> Add DT overlay for SCIF1 (of the Renesas RZ/G3S SoC) routed through the
>> PMOD1_3A interface available on the Renesas RZ SMARC Carrier II board.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>> ---
>>
>> Changes in v4:
>> - rename overlay name to r9a08g045s33-smarc-pmod1-type-3a
>> - add note about the needed switches for SCIF1
>> - guard the scif1 node with #if SW_CONFIG3 == SW_ON && SW_OPT_MUX4 == SW_ON
>> - dropped the alias section from the overlay file and move it
>>   the board file
>> - document SW_OPT_MUX4 switch
> 
> Thanks for the update!
> 
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/renesas/r9a08g045s33-smarc-pmod1-type-3a.dtso
>> @@ -0,0 +1,48 @@
>> +// SPDX-License-Identifier: GPL-2.0
> 
> Would you mind if I changed this to (GPL-2.0-only OR BSD-2-Clause) while
> applying?

Not at all. Thank you for taking care of it.

> 
>> +&pinctrl {
>> +       scif1_pins: scif1-pins {
>> +               pinmux = <RZG2L_PORT_PINMUX(14, 0, 1)>, /* TXD */
>> +                        <RZG2L_PORT_PINMUX(14, 1, 1)>, /* RXD */
>> +                        <RZG2L_PORT_PINMUX(16, 0, 1)>, /* CTS */
>> +                        <RZG2L_PORT_PINMUX(16, 1, 1)>; /* RTS */
> 
> CTS# and RTS#
> 
>> +       };
>> +};
> 
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> i.e. will queue in renesas-devel for v6.15.
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 

Re: [PATCH v4 4/4] arm64: dts: renesas: r9a08g045s33-smarc-pmod: Add overlay for SCIF1

[Geert Uytterhoeven]

On Mon, Jan 20, 2025 at 2:09 PM Claudiu <claudiu.beznea@tuxon.dev> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> Add DT overlay for SCIF1 (of the Renesas RZ/G3S SoC) routed through the
> PMOD1_3A interface available on the Renesas RZ SMARC Carrier II board.
>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

> --- a/arch/arm64/boot/dts/renesas/Makefile
> +++ b/arch/arm64/boot/dts/renesas/Makefile
> @@ -143,6 +143,9 @@ r9a07g054l2-smarc-cru-csi-ov5645-dtbs := r9a07g054l2-smarc.dtb r9a07g054l2-smarc
>  dtb-$(CONFIG_ARCH_R9A07G054) += r9a07g054l2-smarc-cru-csi-ov5645.dtb
>
>  dtb-$(CONFIG_ARCH_R9A08G045) += r9a08g045s33-smarc.dtb
> +dtb-$(CONFIG_ARCH_R9A07G043) += r9a08g045s33-smarc-pmod1-type-3a.dtbo
> +r9a08g045s33-smarc-pmod1-type-3a-dtbs := r9a08g045s33-smarc.dtb r9a08g045s33-smarc-pmod1-type-3a.dtbo
> +dtb-$(CONFIG_ARCH_R9A07G043) += r9a08g045s33-smarc-pmod1-type-3a.dtb

s/7G043/8G045/ while applying...


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 v4 1/4] serial: sh-sci: Update the suspend/resume support

[Claudiu Beznea]

Hi, Geert,

On 24.01.2025 12:53, Geert Uytterhoeven wrote:
> Hi Claudiu,
> 
> Thanks for your patch!
> 
> On Mon, Jan 20, 2025 at 2:09 PM Claudiu <claudiu.beznea@tuxon.dev> wrote:
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> The Renesas RZ/G3S supports a power saving mode where power to most of the
>> SoC components is turned off. When returning from this power saving mode,
>> SoC components need to be re-configured.
>>
>> The SCIFs on the Renesas RZ/G3S need to be re-configured as well when
>> returning from this power saving mode. The sh-sci code already configures
>> the SCIF clocks, power domain and registers by calling uart_resume_port()
>> in sci_resume(). On suspend path the SCIF UART ports are suspended
>> accordingly (by calling uart_suspend_port() in sci_suspend()). The only
>> missing setting is the reset signal. For this assert/de-assert the reset
>> signal on driver suspend/resume.
>>
>> In case the no_console_suspend is specified by the user, the registers need
>> to be saved on suspend path and restore on resume path. To do this the
>> sci_console_setup() function was added. There is no need to cache/restore
>> the status or FIFO registers. Only the control registers. To differentiate
>> b/w these, the struct sci_port_params::regs was updated with a new member
>> that specifies if the register needs to be chached on suspend. Only the
> 
> cached
> 
>> RZ_SCIFA instances were updated with this new support as the hardware for
>> the rest of variants was missing for testing.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
>> --- a/drivers/tty/serial/sh-sci.c
>> +++ b/drivers/tty/serial/sh-sci.c
>> @@ -101,7 +101,7 @@ enum SCI_CLKS {
>>                 if ((_port)->sampling_rate_mask & SCI_SR((_sr)))
>>
>>  struct plat_sci_reg {
>> -       u8 offset, size;
>> +       u8 offset, size, suspend_cacheable;
> 
> This increases the size of sci_port_params[] by 300 bytes.
> Using bitfields would mitigate that:
> 
>     struct plat_sci_reg {
>             u16 offset:8;
>             u16 size:5;
>             u16 suspend_cacheable:1;
>     };
> 
> (if we ever need more bits, the size member can store an enum value
>  instead of the actual size (8 or 16 bits) of the register).
> 
>>  };

OK

>>
>>  struct sci_port_params {
>> @@ -134,6 +134,8 @@ struct sci_port {
>>         struct dma_chan                 *chan_tx;
>>         struct dma_chan                 *chan_rx;
>>
>> +       struct reset_control            *rstc;
>> +
>>  #ifdef CONFIG_SERIAL_SH_SCI_DMA
>>         struct dma_chan                 *chan_tx_saved;
>>         struct dma_chan                 *chan_rx_saved;
>> @@ -153,6 +155,7 @@ struct sci_port {
>>         int                             rx_trigger;
>>         struct timer_list               rx_fifo_timer;
>>         int                             rx_fifo_timeout;
>> +       unsigned int                    console_cached_regs[SCIx_NR_REGS];
> 
> u16, as all registers are 8 or 16 bit wide.

OK.

> 
> We reserve space for 20 registers, but at most 6 will be used.
> This has a rather big impact on the size of sci_ports[], as
> CONFIG_SERIAL_SH_SCI_NR_UARTS defaults to 18.

I agree, but this should keep the suspend/resume code sane in case
extensions will be added to the code. In general people forget about
suspend/resume code when extending. Please let me know if you prefer to
limit it (although, doing like this will complicate the code, I think).

> 
> Also, this space is used/needed only if:
>   - CONFIG_PM_SLEEP=y,
>   - CONFIG_SERIAL_CORE_CONSOLE=y (see uart_console()),
>   - The port is actually used as a console (unfortunately the user
>     can specify multiple console=ttySC<N> command line parameters, in
>     addition to chosen/stdout-path).

Would you prefer to guard the suspend/resume code with these flags?

> 
>>         u16                             hscif_tot;
>>
>>         bool has_rtscts;
>> @@ -300,17 +303,17 @@ static const struct sci_port_params sci_port_params[SCIx_NR_REGTYPES] = {
>>          */
>>         [SCIx_RZ_SCIFA_REGTYPE] = {
>>                 .regs = {
>> -                       [SCSMR]         = { 0x00, 16 },
>> -                       [SCBRR]         = { 0x02,  8 },
>> -                       [SCSCR]         = { 0x04, 16 },
>> +                       [SCSMR]         = { 0x00, 16, 1 },
>> +                       [SCBRR]         = { 0x02,  8, 1 },
>> +                       [SCSCR]         = { 0x04, 16, 1 },
>>                         [SCxTDR]        = { 0x06,  8 },
>>                         [SCxSR]         = { 0x08, 16 },
>>                         [SCxRDR]        = { 0x0A,  8 },
>> -                       [SCFCR]         = { 0x0C, 16 },
>> +                       [SCFCR]         = { 0x0C, 16, 1 },
>>                         [SCFDR]         = { 0x0E, 16 },
>> -                       [SCSPTR]        = { 0x10, 16 },
>> +                       [SCSPTR]        = { 0x10, 16, 1 },
>>                         [SCLSR]         = { 0x12, 16 },
>> -                       [SEMR]          = { 0x14, 8 },
>> +                       [SEMR]          = { 0x14, 8, 1 },
> 
> Note that the driver always writes zero to SEMR.

In case the IP is used on SoCs with sleep states where the resume is done
with the help of bootloader, the bootloader code might interact with
registers that the Linux code writes with zero.

Keeping it for registers where driver writes zero should also help if the
serial IPs power will be off during suspend, thus registers restored to non
zero default values (by HW) after resume.

> 
>>                 },
>>                 .fifosize = 16,
>>                 .overrun_reg = SCLSR,
>> @@ -3374,6 +3377,7 @@ static struct plat_sci_port *sci_parse_dt(struct platform_device *pdev,
>>         }
>>
>>         sp = &sci_ports[id];
>> +       sp->rstc = rstc;
>>         *dev_id = id;
>>
>>         p->type = SCI_OF_TYPE(data);
>> @@ -3546,13 +3550,34 @@ static int sci_probe(struct platform_device *dev)
>>         return 0;
>>  }
>>
>> +static void sci_console_setup(struct sci_port *s, bool save)
> 
> sci_console_save_restore()?

OK

> 
>> +{
>> +       for (u16 i = 0; i < SCIx_NR_REGS; i++) {
> 
> unsigned int

OK

> 
>> +               struct uart_port *port = &s->port;
>> +
>> +               if (!s->params->regs[i].suspend_cacheable)
>> +                       continue;
>> +
>> +               if (save)
>> +                       s->console_cached_regs[i] = sci_serial_in(port, i);
>> +               else
>> +                       sci_serial_out(port, i, s->console_cached_regs[i]);
>> +       }
>> +}
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 

Re: [PATCH v4 4/4] arm64: dts: renesas: r9a08g045s33-smarc-pmod: Add overlay for SCIF1

[Claudiu Beznea]

On 24.01.2025 21:15, Geert Uytterhoeven wrote:
> On Mon, Jan 20, 2025 at 2:09 PM Claudiu <claudiu.beznea@tuxon.dev> wrote:
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> Add DT overlay for SCIF1 (of the Renesas RZ/G3S SoC) routed through the
>> PMOD1_3A interface available on the Renesas RZ SMARC Carrier II board.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
>> --- a/arch/arm64/boot/dts/renesas/Makefile
>> +++ b/arch/arm64/boot/dts/renesas/Makefile
>> @@ -143,6 +143,9 @@ r9a07g054l2-smarc-cru-csi-ov5645-dtbs := r9a07g054l2-smarc.dtb r9a07g054l2-smarc
>>  dtb-$(CONFIG_ARCH_R9A07G054) += r9a07g054l2-smarc-cru-csi-ov5645.dtb
>>
>>  dtb-$(CONFIG_ARCH_R9A08G045) += r9a08g045s33-smarc.dtb
>> +dtb-$(CONFIG_ARCH_R9A07G043) += r9a08g045s33-smarc-pmod1-type-3a.dtbo
>> +r9a08g045s33-smarc-pmod1-type-3a-dtbs := r9a08g045s33-smarc.dtb r9a08g045s33-smarc-pmod1-type-3a.dtbo
>> +dtb-$(CONFIG_ARCH_R9A07G043) += r9a08g045s33-smarc-pmod1-type-3a.dtb
> 
> s/7G043/8G045/ while applying...

Sorry about that! Thank you for handling it.

> 
> 
> 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 v4 1/4] serial: sh-sci: Update the suspend/resume support

[Geert Uytterhoeven]

Hi Claudiu,

On Mon, 27 Jan 2025 at 09:44, Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
> On 24.01.2025 12:53, Geert Uytterhoeven wrote:
> > On Mon, Jan 20, 2025 at 2:09 PM Claudiu <claudiu.beznea@tuxon.dev> wrote:
> >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >>
> >> The Renesas RZ/G3S supports a power saving mode where power to most of the
> >> SoC components is turned off. When returning from this power saving mode,
> >> SoC components need to be re-configured.
> >>
> >> The SCIFs on the Renesas RZ/G3S need to be re-configured as well when
> >> returning from this power saving mode. The sh-sci code already configures
> >> the SCIF clocks, power domain and registers by calling uart_resume_port()
> >> in sci_resume(). On suspend path the SCIF UART ports are suspended
> >> accordingly (by calling uart_suspend_port() in sci_suspend()). The only
> >> missing setting is the reset signal. For this assert/de-assert the reset
> >> signal on driver suspend/resume.
> >>
> >> In case the no_console_suspend is specified by the user, the registers need
> >> to be saved on suspend path and restore on resume path. To do this the
> >> sci_console_setup() function was added. There is no need to cache/restore
> >> the status or FIFO registers. Only the control registers. To differentiate
> >> b/w these, the struct sci_port_params::regs was updated with a new member
> >> that specifies if the register needs to be chached on suspend. Only the
> >
> > cached
> >
> >> RZ_SCIFA instances were updated with this new support as the hardware for
> >> the rest of variants was missing for testing.
> >>
> >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >
> >> --- a/drivers/tty/serial/sh-sci.c
> >> +++ b/drivers/tty/serial/sh-sci.c
> >> @@ -101,7 +101,7 @@ enum SCI_CLKS {
> >>                 if ((_port)->sampling_rate_mask & SCI_SR((_sr)))
> >>
> >>  struct plat_sci_reg {
> >> -       u8 offset, size;
> >> +       u8 offset, size, suspend_cacheable;
> >
> > This increases the size of sci_port_params[] by 300 bytes.
> > Using bitfields would mitigate that:
> >
> >     struct plat_sci_reg {
> >             u16 offset:8;
> >             u16 size:5;
> >             u16 suspend_cacheable:1;
> >     };
> >
> > (if we ever need more bits, the size member can store an enum value
> >  instead of the actual size (8 or 16 bits) of the register).
> >
> >>  };
>
> OK
>
> >>
> >>  struct sci_port_params {
> >> @@ -134,6 +134,8 @@ struct sci_port {
> >>         struct dma_chan                 *chan_tx;
> >>         struct dma_chan                 *chan_rx;
> >>
> >> +       struct reset_control            *rstc;
> >> +
> >>  #ifdef CONFIG_SERIAL_SH_SCI_DMA
> >>         struct dma_chan                 *chan_tx_saved;
> >>         struct dma_chan                 *chan_rx_saved;
> >> @@ -153,6 +155,7 @@ struct sci_port {
> >>         int                             rx_trigger;
> >>         struct timer_list               rx_fifo_timer;
> >>         int                             rx_fifo_timeout;
> >> +       unsigned int                    console_cached_regs[SCIx_NR_REGS];
> >
> > u16, as all registers are 8 or 16 bit wide.
>
> OK.
>
> >
> > We reserve space for 20 registers, but at most 6 will be used.
> > This has a rather big impact on the size of sci_ports[], as
> > CONFIG_SERIAL_SH_SCI_NR_UARTS defaults to 18.
>
> I agree, but this should keep the suspend/resume code sane in case
> extensions will be added to the code. In general people forget about
> suspend/resume code when extending. Please let me know if you prefer to
> limit it (although, doing like this will complicate the code, I think).
>
> >
> > Also, this space is used/needed only if:
> >   - CONFIG_PM_SLEEP=y,
> >   - CONFIG_SERIAL_CORE_CONSOLE=y (see uart_console()),
> >   - The port is actually used as a console (unfortunately the user
> >     can specify multiple console=ttySC<N> command line parameters, in
> >     addition to chosen/stdout-path).
>
> Would you prefer to guard the suspend/resume code with these flags?

I was also thinking about console_cached_regs[]. But if you would
protect that by #ifdef, you also have to protect the code that uses it,
meaning less compile coverage.

If you just add a static inline helper function to check for
CONFIG_PM_SLEEP, !console_suspend_enabled, and
uart_console(&sport->port):

    static bool sci_console_keep_alive(struct sci_port *sport)
    {
            return IS_ENABLED(CONFIG_PM_SLEEP) &&
                   !console_suspend_enabled && uart_console(&sport->port);
    }

then most of the code will be validated but optimized away when unused.

> >>         u16                             hscif_tot;
> >>
> >>         bool has_rtscts;
> >> @@ -300,17 +303,17 @@ static const struct sci_port_params sci_port_params[SCIx_NR_REGTYPES] = {
> >>          */
> >>         [SCIx_RZ_SCIFA_REGTYPE] = {
> >>                 .regs = {
> >> -                       [SCSMR]         = { 0x00, 16 },
> >> -                       [SCBRR]         = { 0x02,  8 },
> >> -                       [SCSCR]         = { 0x04, 16 },
> >> +                       [SCSMR]         = { 0x00, 16, 1 },
> >> +                       [SCBRR]         = { 0x02,  8, 1 },
> >> +                       [SCSCR]         = { 0x04, 16, 1 },
> >>                         [SCxTDR]        = { 0x06,  8 },
> >>                         [SCxSR]         = { 0x08, 16 },
> >>                         [SCxRDR]        = { 0x0A,  8 },
> >> -                       [SCFCR]         = { 0x0C, 16 },
> >> +                       [SCFCR]         = { 0x0C, 16, 1 },
> >>                         [SCFDR]         = { 0x0E, 16 },
> >> -                       [SCSPTR]        = { 0x10, 16 },
> >> +                       [SCSPTR]        = { 0x10, 16, 1 },
> >>                         [SCLSR]         = { 0x12, 16 },
> >> -                       [SEMR]          = { 0x14, 8 },
> >> +                       [SEMR]          = { 0x14, 8, 1 },
> >
> > Note that the driver always writes zero to SEMR.
>
> In case the IP is used on SoCs with sleep states where the resume is done
> with the help of bootloader, the bootloader code might interact with
> registers that the Linux code writes with zero.
>
> Keeping it for registers where driver writes zero should also help if the
> serial IPs power will be off during suspend, thus registers restored to non
> zero default values (by HW) after resume.

Sure, the driver would have to write zero to the register anyway.

While storing the suspend_cacheable flag wouldn't cost any storage
space anymore using bitfields, I am wondering if it would be worthwhile
to have explicit code to save/restore registers, instead of looping
over all of them and checking the flag. I.e.

    u16 saved_scmsr;
    u16 saved_scscr;
    u8 saved_scbrr;
    ...
    u8 saved_semr;

    /* Save omnipresent registers */
    s->saved_scmsr = sci_serial_in(port, SCSMR);
    ...
    /* Save optional registers */
    if (sci_getreg(port, SEMR)->size)
            s->saved_semr = sci_serial_in(port, SEMR);

That would make it apply to all SCI variants, not just for SCIFA,
while increasing sci_port by only 10 bytes/port. And 10 bytes/port is
probably not worth to be protected by an #ifdef...

Thoughts?

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 v4 1/4] serial: sh-sci: Update the suspend/resume support

[Claudiu Beznea]

Hi, Geert,

On 27.01.2025 11:19, Geert Uytterhoeven wrote:
> Hi Claudiu,
> 
> On Mon, 27 Jan 2025 at 09:44, Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
>> On 24.01.2025 12:53, Geert Uytterhoeven wrote:
>>> On Mon, Jan 20, 2025 at 2:09 PM Claudiu <claudiu.beznea@tuxon.dev> wrote:
>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>>
>>>> The Renesas RZ/G3S supports a power saving mode where power to most of the
>>>> SoC components is turned off. When returning from this power saving mode,
>>>> SoC components need to be re-configured.
>>>>
>>>> The SCIFs on the Renesas RZ/G3S need to be re-configured as well when
>>>> returning from this power saving mode. The sh-sci code already configures
>>>> the SCIF clocks, power domain and registers by calling uart_resume_port()
>>>> in sci_resume(). On suspend path the SCIF UART ports are suspended
>>>> accordingly (by calling uart_suspend_port() in sci_suspend()). The only
>>>> missing setting is the reset signal. For this assert/de-assert the reset
>>>> signal on driver suspend/resume.
>>>>
>>>> In case the no_console_suspend is specified by the user, the registers need
>>>> to be saved on suspend path and restore on resume path. To do this the
>>>> sci_console_setup() function was added. There is no need to cache/restore
>>>> the status or FIFO registers. Only the control registers. To differentiate
>>>> b/w these, the struct sci_port_params::regs was updated with a new member
>>>> that specifies if the register needs to be chached on suspend. Only the
>>>
>>> cached
>>>
>>>> RZ_SCIFA instances were updated with this new support as the hardware for
>>>> the rest of variants was missing for testing.
>>>>
>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>
>>>> --- a/drivers/tty/serial/sh-sci.c
>>>> +++ b/drivers/tty/serial/sh-sci.c
>>>> @@ -101,7 +101,7 @@ enum SCI_CLKS {
>>>>                 if ((_port)->sampling_rate_mask & SCI_SR((_sr)))
>>>>
>>>>  struct plat_sci_reg {
>>>> -       u8 offset, size;
>>>> +       u8 offset, size, suspend_cacheable;
>>>
>>> This increases the size of sci_port_params[] by 300 bytes.
>>> Using bitfields would mitigate that:
>>>
>>>     struct plat_sci_reg {
>>>             u16 offset:8;
>>>             u16 size:5;
>>>             u16 suspend_cacheable:1;
>>>     };
>>>
>>> (if we ever need more bits, the size member can store an enum value
>>>  instead of the actual size (8 or 16 bits) of the register).
>>>
>>>>  };
>>
>> OK
>>
>>>>
>>>>  struct sci_port_params {
>>>> @@ -134,6 +134,8 @@ struct sci_port {
>>>>         struct dma_chan                 *chan_tx;
>>>>         struct dma_chan                 *chan_rx;
>>>>
>>>> +       struct reset_control            *rstc;
>>>> +
>>>>  #ifdef CONFIG_SERIAL_SH_SCI_DMA
>>>>         struct dma_chan                 *chan_tx_saved;
>>>>         struct dma_chan                 *chan_rx_saved;
>>>> @@ -153,6 +155,7 @@ struct sci_port {
>>>>         int                             rx_trigger;
>>>>         struct timer_list               rx_fifo_timer;
>>>>         int                             rx_fifo_timeout;
>>>> +       unsigned int                    console_cached_regs[SCIx_NR_REGS];
>>>
>>> u16, as all registers are 8 or 16 bit wide.
>>
>> OK.
>>
>>>
>>> We reserve space for 20 registers, but at most 6 will be used.
>>> This has a rather big impact on the size of sci_ports[], as
>>> CONFIG_SERIAL_SH_SCI_NR_UARTS defaults to 18.
>>
>> I agree, but this should keep the suspend/resume code sane in case
>> extensions will be added to the code. In general people forget about
>> suspend/resume code when extending. Please let me know if you prefer to
>> limit it (although, doing like this will complicate the code, I think).
>>
>>>
>>> Also, this space is used/needed only if:
>>>   - CONFIG_PM_SLEEP=y,
>>>   - CONFIG_SERIAL_CORE_CONSOLE=y (see uart_console()),
>>>   - The port is actually used as a console (unfortunately the user
>>>     can specify multiple console=ttySC<N> command line parameters, in
>>>     addition to chosen/stdout-path).
>>
>> Would you prefer to guard the suspend/resume code with these flags?
> 
> I was also thinking about console_cached_regs[].

Agree.

> But if you would
> protect that by #ifdef, you also have to protect the code that uses it,
> meaning less compile coverage.
> 
> If you just add a static inline helper function to check for
> CONFIG_PM_SLEEP, !console_suspend_enabled, and
> uart_console(&sport->port):
> 
>     static bool sci_console_keep_alive(struct sci_port *sport)
>     {
>             return IS_ENABLED(CONFIG_PM_SLEEP) &&
>                    !console_suspend_enabled && uart_console(&sport->port);
>     }
> 
> then most of the code will be validated but optimized away when unused.

I wasn't aware of this approach. I'll give it a try, if any.

> 
>>>>         u16                             hscif_tot;
>>>>
>>>>         bool has_rtscts;
>>>> @@ -300,17 +303,17 @@ static const struct sci_port_params sci_port_params[SCIx_NR_REGTYPES] = {
>>>>          */
>>>>         [SCIx_RZ_SCIFA_REGTYPE] = {
>>>>                 .regs = {
>>>> -                       [SCSMR]         = { 0x00, 16 },
>>>> -                       [SCBRR]         = { 0x02,  8 },
>>>> -                       [SCSCR]         = { 0x04, 16 },
>>>> +                       [SCSMR]         = { 0x00, 16, 1 },
>>>> +                       [SCBRR]         = { 0x02,  8, 1 },
>>>> +                       [SCSCR]         = { 0x04, 16, 1 },
>>>>                         [SCxTDR]        = { 0x06,  8 },
>>>>                         [SCxSR]         = { 0x08, 16 },
>>>>                         [SCxRDR]        = { 0x0A,  8 },
>>>> -                       [SCFCR]         = { 0x0C, 16 },
>>>> +                       [SCFCR]         = { 0x0C, 16, 1 },
>>>>                         [SCFDR]         = { 0x0E, 16 },
>>>> -                       [SCSPTR]        = { 0x10, 16 },
>>>> +                       [SCSPTR]        = { 0x10, 16, 1 },
>>>>                         [SCLSR]         = { 0x12, 16 },
>>>> -                       [SEMR]          = { 0x14, 8 },
>>>> +                       [SEMR]          = { 0x14, 8, 1 },
>>>
>>> Note that the driver always writes zero to SEMR.
>>
>> In case the IP is used on SoCs with sleep states where the resume is done
>> with the help of bootloader, the bootloader code might interact with
>> registers that the Linux code writes with zero.
>>
>> Keeping it for registers where driver writes zero should also help if the
>> serial IPs power will be off during suspend, thus registers restored to non
>> zero default values (by HW) after resume.
> 
> Sure, the driver would have to write zero to the register anyway.
> 
> While storing the suspend_cacheable flag wouldn't cost any storage
> space anymore using bitfields, I am wondering if it would be worthwhile
> to have explicit code to save/restore registers, instead of looping
> over all of them and checking the flag. I.e.
> 
>     u16 saved_scmsr;
>     u16 saved_scscr;
>     u8 saved_scbrr;
>     ...
>     u8 saved_semr;
> 
>     /* Save omnipresent registers */
>     s->saved_scmsr = sci_serial_in(port, SCSMR);
>     ...
>     /* Save optional registers */
>     if (sci_getreg(port, SEMR)->size)
>             s->saved_semr = sci_serial_in(port, SEMR);
> 
> That would make it apply to all SCI variants, not just for SCIFA,
> while increasing sci_port by only 10 bytes/port. And 10 bytes/port is
> probably not worth to be protected by an #ifdef...

That was the other approach I thought about when working on this patch. I
chose the one proposed in this patch as it looked to me simpler to extend
to other registers, if needed (just enable proper flag in
sci_port_params[]). And needed less changes for the code saving/restoring
the registers.

If you prefer your version let me know and I'll switch to it.

Thank you,
Claudiu

> 
> Thoughts?
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 

Re: [PATCH v4 1/4] serial: sh-sci: Update the suspend/resume support

[Geert Uytterhoeven]

Hi Claudiu,

On Mon, 27 Jan 2025 at 13:23, Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
> On 27.01.2025 11:19, Geert Uytterhoeven wrote:
> > On Mon, 27 Jan 2025 at 09:44, Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
> >> On 24.01.2025 12:53, Geert Uytterhoeven wrote:
> >>> On Mon, Jan 20, 2025 at 2:09 PM Claudiu <claudiu.beznea@tuxon.dev> wrote:
> >>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >>>>
> >>>> The Renesas RZ/G3S supports a power saving mode where power to most of the
> >>>> SoC components is turned off. When returning from this power saving mode,
> >>>> SoC components need to be re-configured.
> >>>>
> >>>> The SCIFs on the Renesas RZ/G3S need to be re-configured as well when
> >>>> returning from this power saving mode. The sh-sci code already configures
> >>>> the SCIF clocks, power domain and registers by calling uart_resume_port()
> >>>> in sci_resume(). On suspend path the SCIF UART ports are suspended
> >>>> accordingly (by calling uart_suspend_port() in sci_suspend()). The only
> >>>> missing setting is the reset signal. For this assert/de-assert the reset
> >>>> signal on driver suspend/resume.
> >>>>
> >>>> In case the no_console_suspend is specified by the user, the registers need
> >>>> to be saved on suspend path and restore on resume path. To do this the
> >>>> sci_console_setup() function was added. There is no need to cache/restore
> >>>> the status or FIFO registers. Only the control registers. To differentiate
> >>>> b/w these, the struct sci_port_params::regs was updated with a new member
> >>>> that specifies if the register needs to be chached on suspend. Only the
> >>>
> >>> cached
> >>>
> >>>> RZ_SCIFA instances were updated with this new support as the hardware for
> >>>> the rest of variants was missing for testing.
> >>>>
> >>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

> > While storing the suspend_cacheable flag wouldn't cost any storage
> > space anymore using bitfields, I am wondering if it would be worthwhile
> > to have explicit code to save/restore registers, instead of looping
> > over all of them and checking the flag. I.e.
> >
> >     u16 saved_scmsr;
> >     u16 saved_scscr;
> >     u8 saved_scbrr;
> >     ...
> >     u8 saved_semr;
> >
> >     /* Save omnipresent registers */
> >     s->saved_scmsr = sci_serial_in(port, SCSMR);
> >     ...
> >     /* Save optional registers */
> >     if (sci_getreg(port, SEMR)->size)
> >             s->saved_semr = sci_serial_in(port, SEMR);
> >
> > That would make it apply to all SCI variants, not just for SCIFA,
> > while increasing sci_port by only 10 bytes/port. And 10 bytes/port is
> > probably not worth to be protected by an #ifdef...
>
> That was the other approach I thought about when working on this patch. I
> chose the one proposed in this patch as it looked to me simpler to extend
> to other registers, if needed (just enable proper flag in
> sci_port_params[]). And needed less changes for the code saving/restoring
> the registers.
>
> If you prefer your version let me know and I'll switch to it.

As this driver is also used on smaller systems (e.g. SH), I'd go for
the solution that leads to the smallest increase of code size and
memory consumption.

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